Re: [PATCH 1/3] grub efi memory map patch

[Johan Rydberg]

"bibo,mao" <address@hidden> writes:

> This patch moves find_mmap_size from i386/efi/linux.c to
> kern/efi/mm.c, and renamed as grub_efi_find_mmap_size, so that
> other arch on EFI platform can use this function.

Good call.

> Also this function solves memory map too small problem,
> on some EFI platform MEMORY_MAP_SIZE(0x1000) is a little smaller
> than EFI memory map table.
>
> any suggestion is welcome.

Just a few comments here.  I will leave a full review to Okuji.

> +#define GRUB_EFI_PAGE_SHIFT          12
> +#define GRUB_EFI_PAGE_SIZE           (0x1UL << GRUB_EFI_PAGE_SHIFT)
> +#define GRUB_EFI_PAGES(addr)                 (addr >> GRUB_EFI_PAGE_SHIFT)
> +#define GRUB_EFI_PAGES_UP(addr)      ((addr + GRUB_EFI_PAGE_SIZE - 1) >> 
> GRUB_EFI_PAGE_SHIFT)
> +#define PAGE_DOWN(addr)              ((addr) & (~(GRUB_EFI_PAGE_SIZE - 1)))
> +#define PAGE_UP(addr)                PAGE_DOWN(addr + GRUB_EFI_PAGE_SIZE - 1)

The PAGE_DOWN and PAGE_UP macros should be named something else, with
a GRUB_EFI prefix.  What about GRUB_EFI_PAGE_TRUNC and
GRUB_EFI_PAGE_ROUND?  Not sure GRUB_EFI_PAGES_UP is needed, since
GRUB_EFI_PAGES (GRUB_EFI_PAGE_ROUND (value)) can be used instead.  If
you find it too long, add a local macro to the file where you use it.

Also, please slap a few parenthesis round marco arguments.

> +/* Find the optimal number of pages for the memory map. */
> +grub_efi_uintn_t
> +grub_efi_find_mmap_size (void)
> +{     +  static grub_efi_uintn_t mmap_size = 0;
> +    +  if (mmap_size != 0)
> +    return mmap_size;
> +    +  mmap_size = GRUB_EFI_PAGE_SIZE;
> +  while (1)
> +    {
> +      int ret;
> +      grub_efi_memory_descriptor_t *mmap;
> +      grub_efi_uintn_t desc_size;
> +      +      mmap = grub_efi_allocate_pages ( 0,
> GRUB_EFI_PAGES_UP(mmap_size));
> +      if (! mmap)
> +        return 0;
> + +      ret = grub_efi_get_memory_map (&mmap_size, mmap, 0,
> &desc_size, 0);
> +      grub_efi_free_pages ((grub_addr_t) mmap, GRUB_EFI_PAGES_UP(mmap_size));
> +       +      if (ret < 0)
> +        grub_fatal ("cannot get memory map");
> +      else if (ret > 0)
> +        break;
> + +      mmap_size += GRUB_EFI_PAGE_SIZE;
> +    }
> + +  /* Increase the size a bit for safety, because GRUB allocates
> more on
> +     later, and EFI itself may allocate more.  */
> +  mmap_size += GRUB_EFI_PAGE_SIZE;
> +  mmap_size = PAGE_UP(mmap_size);
> + +  return mmap_size;
> +}

Is this some patching gone wrong?  What are all the "+"'s doing there?

The specification states that GetMemoryMap will return
EFI_BUFFER_TOO_SMALL and the memory map size in *MemoryMapSize if the
memory map could not fit in the buffer (which may be NULL if provided
buffer size is too small).  Meaning that you can get the size of the
memory map by simply doing;

grub_efi_uintn_t
grub_efi_find_mmap_size (void)
{
  mmap_size = 0;
  err = grub_efi_get_memory_map (&mmap_size, NULL, 0, NULL, 0);
  if (err != GRUB_EFI_BUFFER_TOO_SMALL)
    return err;
  return mmap_size;
}

(code not tested, nor compiled.)

I think Tristan did this in his patch that addresses the same problem
of that the memory map does not fit in a single page.

> +
> void
> grub_efi_mm_init (void)

Whitespace changes are normally not welcome.  But they could be
removed by the maintainer who commits it.  No worries.

B> {
> @@ -346,6 +381,8 @@ grub_efi_mm_init (void)
>   grub_efi_uintn_t desc_size;
>   grub_efi_uint64_t total_pages;
>   grub_efi_uint64_t required_pages;
> +  grub_efi_uintn_t memory_map_size;
> +  int res;
>
>   /* First of all, allocate pages to maintain allocations.  */
>   allocated_pages
> @@ -355,16 +392,20 @@ grub_efi_mm_init (void)
>
>   grub_memset (allocated_pages, 0, ALLOCATED_PAGES_SIZE);
>   -  /* Prepare a memory region to store two memory maps.  */
> -  memory_map = grub_efi_allocate_pages (0,
> -                                     2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
> +  /* Prepare a memory region to store two memory maps.  Obtain size of
> +     memory map by passing a NULL buffer and a buffer size of
> +     zero.  */
> +  memory_map_size = grub_efi_find_mmap_size( );
> +  memory_map = grub_efi_allocate_pages(0, 2 * PAGE_UP(memory_map_size));
> +

One space between function name and parenthesis, and no spaces within
a empty argument list.

>   if (! memory_map)
>     grub_fatal ("cannot allocate memory");
>
> -  filtered_memory_map = NEXT_MEMORY_DESCRIPTOR (memory_map, MEMORY_MAP_SIZE);
> +  filtered_memory_map = NEXT_MEMORY_DESCRIPTOR (memory_map,
> +
> PAGE_UP(memory_map_size));

I suppose this is a line-wrapped patch?

pgpn8y65myNNi.pgp
Description: PGP signature