Re: [PATCH v2 5/6] xen: modify page table construction

[Daniel Kiper]

On Thu, Feb 11, 2016 at 03:35:45PM +0100, Juergen Gross wrote:
> On 11/02/16 13:47, Daniel Kiper wrote:
> > On Thu, Feb 11, 2016 at 08:53:25AM +0100, Juergen Gross wrote:
> >> Modify the page table construction to allow multiple virtual regions
> >> to be mapped. This is done as preparation for removing the p2m list
> >> from the initial kernel mapping in order to support huge pv domains.
> >>
> >> This allows a cleaner approach for mapping the relocator page by
> >> using this capability.
> >>
> >> The interface to the assembler level of the relocator has to be changed
> >> in order to be able to process multiple page table areas.
> >
> > I hope that older kernels could be loaded as is and everything works as 
> > expected.
>
> See Patch 0/6. I have tested old kernels, too.

That is great! I just wanted to be sure.

> >> Signed-off-by: Juergen Gross <address@hidden>
> >> ---
> >>  grub-core/lib/i386/xen/relocator.S   |  47 +++---
> >>  grub-core/lib/x86_64/xen/relocator.S |  44 +++--
> >>  grub-core/lib/xen/relocator.c        |  22 ++-
> >>  grub-core/loader/i386/xen.c          | 313 
> >> +++++++++++++++++++++++------------
> >>  include/grub/xen/relocator.h         |   6 +-
> >>  5 files changed, 277 insertions(+), 155 deletions(-)
> >>
> >> diff --git a/grub-core/lib/i386/xen/relocator.S 
> >> b/grub-core/lib/i386/xen/relocator.S
> >> index 694a54c..c23b405 100644
> >> --- a/grub-core/lib/i386/xen/relocator.S
> >> +++ b/grub-core/lib/i386/xen/relocator.S
> >> @@ -50,41 +50,45 @@ VARIABLE(grub_relocator_xen_remapper_map_high)
> >>    jmp *%ebx
> >>
> >>  LOCAL(cont):
> >> -  xorl    %eax, %eax
> >> -  movl    %eax, %ebp
> >> +  /* mov imm32, %eax */
> >> +  .byte   0xb8
> >> +VARIABLE(grub_relocator_xen_paging_areas_addr)
> >> +  .long   0
> >> +  movl    %eax, %ebx
> >>  1:
> >> -
> >> +  movl    0(%ebx), %ebp
> >> +  movl    4(%ebx), %ecx
> >
> > Oh... No, please use constants not plain numbers and
> > explain in comments what is going on. Otherwise it
> > is unreadable.
>
> Hmm, yes, some comments wouldn't hurt. :-)
> Regarding constants: do you really think I should introduce their
> first usage in this file with my patch?

I would not afraid that. At least in code you change/touch. If you
are not sure please ask maintainer about that.

> >> +  testl   %ecx, %ecx
> >> +  jz      3f
> >> +  addl    $8, %ebx
> >> +  movl    %ebx, %esp
> >> +
> >> +2:
> >> +  movl    %ecx, %edi
> >>    /* mov imm32, %eax */
> >>    .byte   0xb8
> >>  VARIABLE(grub_relocator_xen_mfn_list)
> >>    .long   0
> >> -  movl    %eax, %edi
> >> -  movl    %ebp, %eax
> >> -  movl    0(%edi, %eax, 4), %ecx
> >> -
> >> -  /* mov imm32, %ebx */
> >> -  .byte   0xbb
> >> -VARIABLE(grub_relocator_xen_paging_start)
> >> -  .long   0
> >> -  shll    $12, %eax
> >> -  addl    %eax, %ebx
> >> +  movl    0(%eax, %ebp, 4), %ecx
> >> +  movl    %ebp, %ebx
> >> +  shll    $12, %ebx
> >
> > Ditto.
>
> Look at the removed line above: I just switched register usage.

It does not hurt to change number to constant here too.

> >>    movl    %ecx, %edx
> >>    shll    $12,  %ecx
> >>    shrl    $20,  %edx
> >>    orl     $5, %ecx
> >>    movl    $2, %esi
> >>    movl    $__HYPERVISOR_update_va_mapping, %eax
> >> -  int     $0x82
> >> +  int     $0x82   /* parameters: eax, ebx, ecx, edx, esi */
> >
> > Please use more verbose comments.
>
> :-)
>
> >
> >>
> >>    incl    %ebp
> >> -  /* mov imm32, %ecx */
> >> -  .byte   0xb9
> >> -VARIABLE(grub_relocator_xen_paging_size)
> >> -  .long   0
> >> -  cmpl    %ebp, %ecx
> >> +  movl    %edi, %ecx
> >> +
> >> +  loop    2b
> >>
> >> -  ja      1b
> >> +  mov     %esp, %ebx
> >> +  jmp     1b
> >>
> >> +3:
> >>    /* mov imm32, %ebx */
> >>    .byte   0xbb
> >>  VARIABLE(grub_relocator_xen_mmu_op_addr)
> >> @@ -102,6 +106,9 @@ VARIABLE(grub_relocator_xen_remap_continue)
> >>
> >>    jmp *%eax
> >>
> >> +VARIABLE(grub_relocator_xen_paging_areas)
> >> +  .long   0, 0, 0, 0, 0, 0, 0, 0
> >> +
> >>  VARIABLE(grub_relocator_xen_mmu_op)
> >>    .space 256
> >>
> >> diff --git a/grub-core/lib/x86_64/xen/relocator.S 
> >> b/grub-core/lib/x86_64/xen/relocator.S
> >> index 92e9e72..dbb90c7 100644
> >> --- a/grub-core/lib/x86_64/xen/relocator.S
> >> +++ b/grub-core/lib/x86_64/xen/relocator.S
> >
> > Is to possible to split this patch to i386 and x86-64 stuff patches?
>
> I don't think so. The C-part is common and assembler sources must both
> match.

Yep, I am aware of that. However, I was able to that in my patch series.
So, maybe it is possible here. Could you try it?

> >> @@ -50,31 +50,24 @@ VARIABLE(grub_relocator_xen_remapper_map)
> >>
> >>  LOCAL(cont):
> >>
> >> -  /* mov imm64, %rcx */
> >> -  .byte   0x48
> >> -  .byte   0xb9
> >> -VARIABLE(grub_relocator_xen_paging_size)
> >> -  .quad   0
> >> -
> >> -  /* mov imm64, %rax */
> >> -  .byte   0x48
> >> -  .byte   0xb8
> >> -VARIABLE(grub_relocator_xen_paging_start)
> >> -  .quad   0
> >> -
> >> -  movq    %rax, %r12
> >> -
> >>    /* mov imm64, %rax */
> >>    .byte   0x48
> >>    .byte   0xb8
> >>  VARIABLE(grub_relocator_xen_mfn_list)
> >>    .quad   0
> >>
> >> -  movq    %rax, %rsi
> >> +  movq    %rax, %rbx
> >> +  leaq    EXT_C(grub_relocator_xen_paging_areas) (%rip), %r8
> >> +
> >>  1:
> >> +  movq    0(%r8), %r12
> >> +  movq    8(%r8), %rcx
> >
> > Ditto.
> >
> >> +  testq   %rcx, %rcx
> >> +  jz      3f
> >> +2:
> >>    movq    %r12, %rdi
> >> -  movq    %rsi, %rbx
> >> -  movq    0(%rsi), %rsi
> >> +  shlq    $12, %rdi
> >> +  movq    (%rbx, %r12, 8), %rsi
> >
> > Ditto.
> >
> >>    shlq    $12,  %rsi
> >>    orq     $5, %rsi
> >>    movq    $2, %rdx
> >> @@ -83,13 +76,15 @@ VARIABLE(grub_relocator_xen_mfn_list)
> >>    syscall
> >>
> >>    movq    %r9, %rcx
> >> -  addq    $8, %rbx
> >> -  addq    $4096, %r12
> >> -  movq    %rbx, %rsi
> >> +  incq    %r12
> >> +
> >> +  loop 2b
> >>
> >> -  loop 1b
> >> +  addq    $16, %r8
> >
> > Ditto.
> >
> >> +  jmp     1b
> >>
> >> -  leaq   LOCAL(mmu_op) (%rip), %rdi
> >> +3:
> >> +  leaq   EXT_C(grub_relocator_xen_mmu_op) (%rip), %rdi
> >>    movq   $3, %rsi
> >>    movq   $0, %rdx
> >>    movq   $0x7FF0, %r10
> >
> > Ugh... Please fix this too. Probably in separate patch.
>
> I don't mind cleaning the assembler sources by using more comments
> and constants. I just don't think this should be part of this series.

I think that you can safely fix that issue in code you touch. I do not
insist on fixing rest of the code in the same patch. However, it would
be nice if you remove such worst/obvious warts at least at the end of
this patch series.

> >> @@ -104,7 +99,10 @@ VARIABLE(grub_relocator_xen_remap_continue)
> >>
> >>    jmp *%rax
> >>
> >> -LOCAL(mmu_op):
> >> +VARIABLE(grub_relocator_xen_paging_areas)
> >> +  /* array of start, size pairs, size 0 is end marker */
> >> +  .quad   0, 0, 0, 0, 0, 0, 0, 0
> >> +
> >>  VARIABLE(grub_relocator_xen_mmu_op)
> >>    .space 256
> >>
> >> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
> >> index 8f427d3..bc29055 100644
> >> --- a/grub-core/lib/xen/relocator.c
> >> +++ b/grub-core/lib/xen/relocator.c
> >> @@ -36,15 +36,15 @@ extern grub_uint8_t grub_relocator_xen_remap_end;
> >>  extern grub_xen_reg_t grub_relocator_xen_stack;
> >>  extern grub_xen_reg_t grub_relocator_xen_start_info;
> >>  extern grub_xen_reg_t grub_relocator_xen_entry_point;
> >> -extern grub_xen_reg_t grub_relocator_xen_paging_start;
> >> -extern grub_xen_reg_t grub_relocator_xen_paging_size;
> >>  extern grub_xen_reg_t grub_relocator_xen_remapper_virt;
> >>  extern grub_xen_reg_t grub_relocator_xen_remapper_virt2;
> >>  extern grub_xen_reg_t grub_relocator_xen_remapper_map;
> >>  extern grub_xen_reg_t grub_relocator_xen_mfn_list;
> >> +extern grub_xen_reg_t grub_relocator_xen_paging_areas[2 * 
> >> XEN_MAX_MAPPINGS];
> >>  extern grub_xen_reg_t grub_relocator_xen_remap_continue;
> >>  #ifdef __i386__
> >>  extern grub_xen_reg_t grub_relocator_xen_mmu_op_addr;
> >> +extern grub_xen_reg_t grub_relocator_xen_paging_areas_addr;
> >>  extern grub_xen_reg_t grub_relocator_xen_remapper_map_high;
> >>  #endif
> >>  extern mmuext_op_t grub_relocator_xen_mmu_op[3];
> >> @@ -61,6 +61,7 @@ grub_relocator_xen_boot (struct grub_relocator *rel,
> >>  {
> >>    grub_err_t err;
> >>    void *relst;
> >> +  int i;
> >>    grub_relocator_chunk_t ch, ch_tramp;
> >>    grub_xen_mfn_t *mfn_list =
> >>      (grub_xen_mfn_t *) grub_xen_start_page_addr->mfn_list;
> >> @@ -77,8 +78,11 @@ grub_relocator_xen_boot (struct grub_relocator *rel,
> >>    grub_relocator_xen_stack = state.stack;
> >>    grub_relocator_xen_start_info = state.start_info;
> >>    grub_relocator_xen_entry_point = state.entry_point;
> >> -  grub_relocator_xen_paging_start = state.paging_start << 12;
> >> -  grub_relocator_xen_paging_size = state.paging_size;
> >> +  for (i = 0; i < XEN_MAX_MAPPINGS; i++)
> >> +    {
> >> +      grub_relocator_xen_paging_areas[2 * i] = state.paging_start[i];
> >> +      grub_relocator_xen_paging_areas[2 * i + 1] = state.paging_size[i];
> >
> > Why 2 and 1? Constants?
>
> I guess I should probably define grub_relocator_xen_paging_areas as
> an array of struct { grub_xen_reg_t start; grub_xen_reg_t size; };

Make sense for me.

> >> +    }
> >>    grub_relocator_xen_remapper_virt = remapper_virt;
> >>    grub_relocator_xen_remapper_virt2 = remapper_virt;
> >>    grub_relocator_xen_remap_continue = trampoline_virt;
> >> @@ -88,10 +92,12 @@ grub_relocator_xen_boot (struct grub_relocator *rel,
> >>    grub_relocator_xen_remapper_map_high = (mfn_list[remapper_pfn] >> 20);
> >>    grub_relocator_xen_mmu_op_addr = (char *) &grub_relocator_xen_mmu_op
> >>      - (char *) &grub_relocator_xen_remap_start + remapper_virt;
> >> +  grub_relocator_xen_paging_areas_addr =
> >> +    (char *) &grub_relocator_xen_paging_areas
> >> +    - (char *) &grub_relocator_xen_remap_start + remapper_virt;
> >>  #endif
> >>
> >> -  grub_relocator_xen_mfn_list = state.mfn_list
> >> -    + state.paging_start * sizeof (grub_addr_t);
> >> +  grub_relocator_xen_mfn_list = state.mfn_list;
> >>
> >>    grub_memset (grub_relocator_xen_mmu_op, 0,
> >>           sizeof (grub_relocator_xen_mmu_op));
> >> @@ -100,9 +106,9 @@ grub_relocator_xen_boot (struct grub_relocator *rel,
> >>  #else
> >>    grub_relocator_xen_mmu_op[0].cmd = MMUEXT_PIN_L4_TABLE;
> >>  #endif
> >> -  grub_relocator_xen_mmu_op[0].arg1.mfn = mfn_list[state.paging_start];
> >> +  grub_relocator_xen_mmu_op[0].arg1.mfn = mfn_list[state.paging_start[0]];
> >>    grub_relocator_xen_mmu_op[1].cmd = MMUEXT_NEW_BASEPTR;
> >> -  grub_relocator_xen_mmu_op[1].arg1.mfn = mfn_list[state.paging_start];
> >> +  grub_relocator_xen_mmu_op[1].arg1.mfn = mfn_list[state.paging_start[0]];
> >>    grub_relocator_xen_mmu_op[2].cmd = MMUEXT_UNPIN_TABLE;
> >>    grub_relocator_xen_mmu_op[2].arg1.mfn =
> >>      mfn_list[grub_xen_start_page_addr->pt_base >> 12];
> >> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
> >> index 0f41048..5e10420 100644
> >> --- a/grub-core/loader/i386/xen.c
> >> +++ b/grub-core/loader/i386/xen.c
> >> @@ -42,6 +42,29 @@
> >>
> >>  GRUB_MOD_LICENSE ("GPLv3+");
> >>
> >> +#ifdef __x86_64__
> >> +#define NUMBER_OF_LEVELS 4
> >> +#define INTERMEDIATE_OR 7
> >> +#define VIRT_MASK 0x0000ffffffffffffULL
> >> +#else
> >> +#define NUMBER_OF_LEVELS 3
> >> +#define INTERMEDIATE_OR 3
> >> +#define VIRT_MASK 0x00000000ffffffffULL
> >
> > Long expected constants... Nice!
>
> Just to please you. :-)
> They were just moved up some lines as they are used in the new
> structure definitions.
>
> Using constants was unavoidable here. ;-)

Thanks a lot! :-)))

> >> +#endif
> >> +
> >> +struct grub_xen_mapping_lvl {
> >> +  grub_uint64_t virt_start;
> >> +  grub_uint64_t virt_end;
> >> +  grub_uint64_t pfn_start;
> >> +  grub_uint64_t n_pt_pages;
> >> +};
> >> +
> >> +struct grub_xen_mapping {
> >> +  grub_uint64_t *where;
> >> +  struct grub_xen_mapping_lvl area;
> >> +  struct grub_xen_mapping_lvl lvls[NUMBER_OF_LEVELS];
> >> +};
> >> +
> >>  static struct grub_relocator *relocator = NULL;
> >>  static grub_uint64_t max_addr;
> >>  static grub_dl_t my_mod;
> >> @@ -56,9 +79,10 @@ static struct grub_relocator_xen_state state;
> >>  static grub_xen_mfn_t *virt_mfn_list;
> >>  static struct start_info *virt_start_info;
> >>  static grub_xen_mfn_t console_pfn;
> >> -static grub_uint64_t *virt_pgtable;
> >> -static grub_uint64_t pgtbl_start;
> >>  static grub_uint64_t pgtbl_end;
> >> +static struct grub_xen_mapping mappings[XEN_MAX_MAPPINGS];
> >> +static int n_mappings;
> >> +static struct grub_xen_mapping *map_reloc;
> >>
> >>  #define PAGE_SIZE 4096
> >>  #define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list))
> >> @@ -75,100 +99,162 @@ page2offset (grub_uint64_t page)
> >>    return page << PAGE_SHIFT;
> >>  }
> >>
> >> -#ifdef __x86_64__
> >> -#define NUMBER_OF_LEVELS 4
> >> -#define INTERMEDIATE_OR 7
> >> -#else
> >> -#define NUMBER_OF_LEVELS 3
> >> -#define INTERMEDIATE_OR 3
> >> -#endif
> >> -
> >> -static grub_uint64_t
> >> -get_pgtable_size (grub_uint64_t total_pages, grub_uint64_t virt_base)
> >> +static grub_err_t
> >> +get_pgtable_size (grub_uint64_t from, grub_uint64_t to, grub_uint64_t pfn)
> >>  {
> >> -  if (!virt_base)
> >> -    total_pages++;
> >> -  grub_uint64_t ret = 0;
> >> -  grub_uint64_t ll = total_pages;
> >> -  int i;
> >> -  for (i = 0; i < NUMBER_OF_LEVELS; i++)
> >> -    {
> >> -      ll = (ll + POINTERS_PER_PAGE - 1) >> LOG_POINTERS_PER_PAGE;
> >> -      /* PAE wants all 4 root directories present.  */
> >> -#ifdef __i386__
> >> -      if (i == 1)
> >> -  ll = 4;
> >> -#endif
> >> -      ret += ll;
> >> -    }
> >> -  for (i = 1; i < NUMBER_OF_LEVELS; i++)
> >> -    if (virt_base >> (PAGE_SHIFT + i * LOG_POINTERS_PER_PAGE))
> >> -      ret++;
> >> -  return ret;
> >> -}
> >> +  struct grub_xen_mapping *map, *map_cmp;
> >> +  grub_uint64_t mask, bits;
> >> +  int i, m;
> >>
> >> -static void
> >> -generate_page_table (grub_uint64_t *where, grub_uint64_t paging_start,
> >> -               grub_uint64_t paging_end, grub_uint64_t total_pages,
> >> -               grub_uint64_t virt_base, grub_xen_mfn_t *mfn_list)
> >> -{
> >> -  if (!virt_base)
> >> -    paging_end++;
> >> +  if (n_mappings == XEN_MAX_MAPPINGS)
> >> +    return grub_error (GRUB_ERR_BUG, "too many mapped areas");
> >> +
> >> +  grub_dprintf ("xen", "get_pgtable_size %d from=%llx, to=%llx, 
> >> pfn=%llx\n",
> >> +          n_mappings, (unsigned long long) from, (unsigned long long) to,
> >> +          (unsigned long long) pfn);
> >>
> >> -  grub_uint64_t lx[NUMBER_OF_LEVELS], lxs[NUMBER_OF_LEVELS];
> >> -  grub_uint64_t nlx, nls, sz = 0;
> >> -  int l;
> >> +  map = mappings + n_mappings;
> >> +  grub_memset (map, 0, sizeof (*map));
> >>
> >> -  nlx = paging_end;
> >> -  nls = virt_base >> PAGE_SHIFT;
> >> -  for (l = 0; l < NUMBER_OF_LEVELS; l++)
> >> +  map->area.virt_start = from & VIRT_MASK;
> >> +  map->area.virt_end = (to - 1) & VIRT_MASK;
> >> +  map->area.n_pt_pages = 0;
> >> +
> >> +  for (i = NUMBER_OF_LEVELS - 1; i >= 0; i--)
> >>      {
> >> -      nlx = (nlx + POINTERS_PER_PAGE - 1) >> LOG_POINTERS_PER_PAGE;
> >> -      /* PAE wants all 4 root directories present.  */
> >> +      map->lvls[i].pfn_start = pfn + map->area.n_pt_pages;
> >> +      if (i == NUMBER_OF_LEVELS - 1)
> >> +  {
> >> +    if (n_mappings == 0)
> >> +      {
> >> +        map->lvls[i].virt_start = 0;
> >> +        map->lvls[i].virt_end = VIRT_MASK;
> >> +        map->lvls[i].n_pt_pages = 1;
> >> +        map->area.n_pt_pages++;
> >> +      }
> >> +    continue;
> >> +  }
> >> +
> >> +      bits = PAGE_SHIFT + (i + 1) * LOG_POINTERS_PER_PAGE;
> >> +      mask = (1ULL << bits) - 1;
> >> +      map->lvls[i].virt_start = map->area.virt_start & ~mask;
> >> +      map->lvls[i].virt_end = map->area.virt_end | mask;
> >>  #ifdef __i386__
> >> -      if (l == 1)
> >> -  nlx = 4;
> >> +      /* PAE wants last root directory present. */
> >> +      if (i == 1 && to <= 0xc0000000 && n_mappings == 0)
> >
> > Ditto.
> >
> >> +  map->lvls[i].virt_end = VIRT_MASK;
> >>  #endif
> >> -      lx[l] = nlx;
> >> -      sz += lx[l];
> >> -      lxs[l] = nls & (POINTERS_PER_PAGE - 1);
> >> -      if (nls && l != 0)
> >> -  sz++;
> >> -      nls >>= LOG_POINTERS_PER_PAGE;
> >> +      for (m = 0; m < n_mappings; m++)
> >> +  {
> >> +    map_cmp = mappings + m;
> >> +    if (map_cmp->lvls[i].virt_start == map_cmp->lvls[i].virt_end)
> >> +      continue;
> >> +    if (map->lvls[i].virt_start >= map_cmp->lvls[i].virt_start &&
> >> +        map->lvls[i].virt_end <= map_cmp->lvls[i].virt_end)
> >> +     {
> >> +       map->lvls[i].virt_start = 0;
> >> +       map->lvls[i].virt_end = 0;
> >> +       break;
> >> +     }
> >> +     if (map->lvls[i].virt_start >= map_cmp->lvls[i].virt_start &&
> >> +         map->lvls[i].virt_start <= map_cmp->lvls[i].virt_end)
> >> +       map->lvls[i].virt_start = map_cmp->lvls[i].virt_end + 1;
> >> +     if (map->lvls[i].virt_end >= map_cmp->lvls[i].virt_start &&
> >> +         map->lvls[i].virt_end <= map_cmp->lvls[i].virt_end)
> >> +       map->lvls[i].virt_end = map_cmp->lvls[i].virt_start - 1;
> >> +  }
> >> +      if (map->lvls[i].virt_start < map->lvls[i].virt_end)
> >> +  map->lvls[i].n_pt_pages =
> >> +    ((map->lvls[i].virt_end - map->lvls[i].virt_start) >> bits) + 1;
> >> +      map->area.n_pt_pages += map->lvls[i].n_pt_pages;
> >> +      grub_dprintf ("xen", "get_pgtable_size level %d: virt %llx-%llx %d 
> >> pts\n",
> >> +              i, (unsigned long long)  map->lvls[i].virt_start,
> >> +              (unsigned long long)  map->lvls[i].virt_end,
> >> +              (int) map->lvls[i].n_pt_pages);
> >>      }
> >>
> >> -  grub_uint64_t lp;
> >> -  grub_uint64_t j;
> >> -  grub_uint64_t *pg = (grub_uint64_t *) where;
> >> -  int pr = 0;
> >> +  grub_dprintf ("xen", "get_pgtable_size return: %d page tables\n",
> >> +          (int) map->area.n_pt_pages);
> >> +
> >> +  state.paging_start[n_mappings] = pfn;
> >> +  state.paging_size[n_mappings] = map->area.n_pt_pages;
> >> +
> >> +  return GRUB_ERR_NONE;
> >> +}
> >> +
> >> +static grub_uint64_t *
> >> +get_pg_table_virt (int mapping, int level)
> >> +{
> >> +  grub_uint64_t pfn;
> >> +  struct grub_xen_mapping *map;
> >> +
> >> +  map = mappings + mapping;
> >> +  pfn = map->lvls[level].pfn_start - map->lvls[NUMBER_OF_LEVELS - 
> >> 1].pfn_start;
> >> +  return map->where + pfn * POINTERS_PER_PAGE;
> >> +}
> >>
> >> -  grub_memset (pg, 0, sz * PAGE_SIZE);
> >> +static grub_uint64_t
> >> +get_pg_table_prot (int level, grub_uint64_t pfn)
> >> +{
> >> +  int m;
> >> +  grub_uint64_t pfn_s, pfn_e;
> >>
> >> -  lp = paging_start + lx[NUMBER_OF_LEVELS - 1];
> >> -  for (l = NUMBER_OF_LEVELS - 1; l >= 1; l--)
> >> +  if (level > 0)
> >> +    return INTERMEDIATE_OR;
> >> +  for (m = 0; m < n_mappings; m++)
> >>      {
> >> -      if (lxs[l] || pr)
> >> -  pg[0] = page2offset (mfn_list[lp++]) | INTERMEDIATE_OR;
> >> -      if (pr)
> >> -  pg += POINTERS_PER_PAGE;
> >> -      for (j = 0; j < lx[l - 1]; j++)
> >> -  pg[j + lxs[l]] = page2offset (mfn_list[lp++]) | INTERMEDIATE_OR;
> >> -      pg += lx[l] * POINTERS_PER_PAGE;
> >> -      if (lxs[l])
> >> -  pr = 1;
> >> +      pfn_s = mappings[m].lvls[NUMBER_OF_LEVELS - 1].pfn_start;
> >> +      pfn_e = mappings[m].area.n_pt_pages + pfn_s;
> >> +      if (pfn >= pfn_s && pfn < pfn_e)
> >> +  return 5;
> >>      }
> >> +  return 7;
> >
> > What 7 and 5 means?
>
> Page table protection bits.

Constants or at least comments please. Yep, I am boring. I know.
That is my style.

> > I am exhausted. Please fix above stuff and I will review this patch
> > again after fixes.
>
> I'd really like to have the maintainer's opinion on usage of constants
> vs. pure numbers. Up to now I have the impression that constants are
> used only to abstract i386 vs. x86-64. I wouldn't mind changing that,
> but I don't like modifying all my patches which are then rejected due to
> that change.

OK, let's wait for maintainer opinion. However, I feel that using
constants ease code reading. Though it does not mean that I think
that every number/string should be changed to constant. We should
find a proper balance and if plain numbers are used then there
should be at least one line comment what this number means.

> Thanks for doing the review,

You are welcome.

Daniel