[PATCH 1/1] efi/libstub: describe efi_relocate_kernel()

[PATCH 1/1] efi/libstub: describe efi_relocate_kernel()

[Heinrich Schuchardt]

Update the description of of efi_relocate_kernel() to match Sphinx style.

Update parameter references in the description of other memory functions
to use @param style.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 drivers/firmware/efi/libstub/mem.c | 38 +++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/firmware/efi/libstub/mem.c b/drivers/firmware/efi/libstub/mem.c
index 0d57078e5e62..7efe3ed2d5a6 100644
--- a/drivers/firmware/efi/libstub/mem.c
+++ b/drivers/firmware/efi/libstub/mem.c
@@ -86,7 +86,7 @@ efi_status_t efi_get_memory_map(struct efi_boot_memmap *map)
  *
  * Allocate pages as EFI_LOADER_DATA. The allocated pages are aligned according
  * to EFI_ALLOC_ALIGN. The last allocated page will not exceed the address
- * given by 'max'.
+ * given by @max.
  *
  * Return:	status code
  */
@@ -126,10 +126,10 @@ efi_status_t efi_allocate_pages(unsigned long size, unsigned long *addr,
  * @addr:	on exit the address of the allocated memory
  * @min:	minimum address to used for the memory allocation
  *
- * Allocate at the lowest possible address that is not below 'min' as
- * EFI_LOADER_DATA. The allocated pages are aligned according to 'align' but at
+ * Allocate at the lowest possible address that is not below @min as
+ * EFI_LOADER_DATA. The allocated pages are aligned according to @align but at
  * least EFI_ALLOC_ALIGN. The first allocated page will not below the address
- * given by 'min'.
+ * given by @min.
  *
  * Return:	status code
  */
@@ -214,7 +214,7 @@ efi_status_t efi_low_alloc_above(unsigned long size, unsigned long align,
  * @addr:	start of the memory area to free (must be EFI_PAGE_SIZE
  *		aligned)
  *
- * 'size' is rounded up to a multiple of EFI_ALLOC_ALIGN which is an
+ * @size is rounded up to a multiple of EFI_ALLOC_ALIGN which is an
  * architecture specific multiple of EFI_PAGE_SIZE. So this function should
  * only be used to return pages allocated with efi_allocate_pages() or
  * efi_low_alloc_above().
@@ -230,15 +230,25 @@ void efi_free(unsigned long size, unsigned long addr)
 	efi_bs_call(free_pages, addr, nr_pages);
 }

-/*
- * Relocate a kernel image, either compressed or uncompressed.
- * In the ARM64 case, all kernel images are currently
- * uncompressed, and as such when we relocate it we need to
- * allocate additional space for the BSS segment. Any low
- * memory that this function should avoid needs to be
- * unavailable in the EFI memory map, as if the preferred
- * address is not available the lowest available address will
- * be used.
+/**
+ * efi_relocate_kernel() - copy memory area
+ * @image_addr:		address of memory area to copy, on exit target address
+ * @image_size:		size of memory area to copy
+ * @alloc_size:		minimum size of memory to allocate, must be greater or
+ *			equal to image_size
+ * @preferred_addr:	preferred target address
+ * @alignment:		minimum alignment of the allocated memory area. It
+ *			should be a power of two.
+ * @min_addr:		minimum target address
+ *
+ * Copy a memory area to a newly allocated memory area aligned according
+ * to @alignment but at least EFI_ALLOC_ALIGN. If the preferred address
+ * is not available, the allocated address will not be below @min_addr.
+ *
+ * This function is used to copy the Linux kernel verbatim. It does not apply
+ * any relocation changes.
+ *
+ * Return:		status code
  */
 efi_status_t efi_relocate_kernel(unsigned long *image_addr,
 				 unsigned long image_size,
--
2.25.0

Re: [PATCH 1/1] efi/libstub: describe efi_relocate_kernel()

[Randy Dunlap]

Hi,
Mostly looks good.  One comment below:

On 2/19/20 10:53 PM, Heinrich Schuchardt wrote:
> Update the description of of efi_relocate_kernel() to match Sphinx style.
> 
> Update parameter references in the description of other memory functions
> to use @param style.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  drivers/firmware/efi/libstub/mem.c | 38 +++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/mem.c b/drivers/firmware/efi/libstub/mem.c
> index 0d57078e5e62..7efe3ed2d5a6 100644
> --- a/drivers/firmware/efi/libstub/mem.c
> +++ b/drivers/firmware/efi/libstub/mem.c
> @@ -86,7 +86,7 @@ efi_status_t efi_get_memory_map(struct efi_boot_memmap *map)
>   *
>   * Allocate pages as EFI_LOADER_DATA. The allocated pages are aligned according
>   * to EFI_ALLOC_ALIGN. The last allocated page will not exceed the address
> - * given by 'max'.
> + * given by @max.
>   *
>   * Return:	status code
>   */
> @@ -126,10 +126,10 @@ efi_status_t efi_allocate_pages(unsigned long size, unsigned long *addr,
>   * @addr:	on exit the address of the allocated memory
>   * @min:	minimum address to used for the memory allocation
>   *
> - * Allocate at the lowest possible address that is not below 'min' as
> - * EFI_LOADER_DATA. The allocated pages are aligned according to 'align' but at
> + * Allocate at the lowest possible address that is not below @min as
> + * EFI_LOADER_DATA. The allocated pages are aligned according to @align but at
>   * least EFI_ALLOC_ALIGN. The first allocated page will not below the address
> - * given by 'min'.
> + * given by @min.
>   *
>   * Return:	status code
>   */
> @@ -214,7 +214,7 @@ efi_status_t efi_low_alloc_above(unsigned long size, unsigned long align,
>   * @addr:	start of the memory area to free (must be EFI_PAGE_SIZE
>   *		aligned)
>   *
> - * 'size' is rounded up to a multiple of EFI_ALLOC_ALIGN which is an
> + * @size is rounded up to a multiple of EFI_ALLOC_ALIGN which is an
>   * architecture specific multiple of EFI_PAGE_SIZE. So this function should
>   * only be used to return pages allocated with efi_allocate_pages() or
>   * efi_low_alloc_above().
> @@ -230,15 +230,25 @@ void efi_free(unsigned long size, unsigned long addr)
>  	efi_bs_call(free_pages, addr, nr_pages);
>  }
> 
> -/*
> - * Relocate a kernel image, either compressed or uncompressed.
> - * In the ARM64 case, all kernel images are currently
> - * uncompressed, and as such when we relocate it we need to
> - * allocate additional space for the BSS segment. Any low
> - * memory that this function should avoid needs to be
> - * unavailable in the EFI memory map, as if the preferred
> - * address is not available the lowest available address will
> - * be used.
> +/**
> + * efi_relocate_kernel() - copy memory area
> + * @image_addr:		address of memory area to copy, on exit target address

The "on exit target address" is a little bit confusing IMO.
Is it like this?

  On exit, @image_addr is updated to the target copy address that was used.

?  or some other better description?

Thanks.

Acked-by: Randy Dunlap <rdunlap@infradead.org>


> + * @image_size:		size of memory area to copy
> + * @alloc_size:		minimum size of memory to allocate, must be greater or
> + *			equal to image_size
> + * @preferred_addr:	preferred target address
> + * @alignment:		minimum alignment of the allocated memory area. It
> + *			should be a power of two.
> + * @min_addr:		minimum target address
> + *
> + * Copy a memory area to a newly allocated memory area aligned according
> + * to @alignment but at least EFI_ALLOC_ALIGN. If the preferred address
> + * is not available, the allocated address will not be below @min_addr.
> + *
> + * This function is used to copy the Linux kernel verbatim. It does not apply
> + * any relocation changes.
> + *
> + * Return:		status code
>   */
>  efi_status_t efi_relocate_kernel(unsigned long *image_addr,
>  				 unsigned long image_size,
> --
> 2.25.0
> 


-- 
~Randy

Re: [PATCH 1/1] efi/libstub: describe efi_relocate_kernel()

[Ard Biesheuvel]

On Thu, 20 Feb 2020 at 08:02, Randy Dunlap <rdunlap@infradead.org> wrote:
>
> Hi,
> Mostly looks good.  One comment below:
>
> On 2/19/20 10:53 PM, Heinrich Schuchardt wrote:
> > Update the description of of efi_relocate_kernel() to match Sphinx style.
> >
> > Update parameter references in the description of other memory functions
> > to use @param style.
> >
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> >  drivers/firmware/efi/libstub/mem.c | 38 +++++++++++++++++++-----------
> >  1 file changed, 24 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/mem.c b/drivers/firmware/efi/libstub/mem.c
> > index 0d57078e5e62..7efe3ed2d5a6 100644
> > --- a/drivers/firmware/efi/libstub/mem.c
> > +++ b/drivers/firmware/efi/libstub/mem.c
> > @@ -86,7 +86,7 @@ efi_status_t efi_get_memory_map(struct efi_boot_memmap *map)
> >   *
> >   * Allocate pages as EFI_LOADER_DATA. The allocated pages are aligned according
> >   * to EFI_ALLOC_ALIGN. The last allocated page will not exceed the address
> > - * given by 'max'.
> > + * given by @max.
> >   *
> >   * Return:   status code
> >   */
> > @@ -126,10 +126,10 @@ efi_status_t efi_allocate_pages(unsigned long size, unsigned long *addr,
> >   * @addr:    on exit the address of the allocated memory
> >   * @min:     minimum address to used for the memory allocation
> >   *
> > - * Allocate at the lowest possible address that is not below 'min' as
> > - * EFI_LOADER_DATA. The allocated pages are aligned according to 'align' but at
> > + * Allocate at the lowest possible address that is not below @min as
> > + * EFI_LOADER_DATA. The allocated pages are aligned according to @align but at
> >   * least EFI_ALLOC_ALIGN. The first allocated page will not below the address
> > - * given by 'min'.
> > + * given by @min.
> >   *
> >   * Return:   status code
> >   */
> > @@ -214,7 +214,7 @@ efi_status_t efi_low_alloc_above(unsigned long size, unsigned long align,
> >   * @addr:    start of the memory area to free (must be EFI_PAGE_SIZE
> >   *           aligned)
> >   *
> > - * 'size' is rounded up to a multiple of EFI_ALLOC_ALIGN which is an
> > + * @size is rounded up to a multiple of EFI_ALLOC_ALIGN which is an
> >   * architecture specific multiple of EFI_PAGE_SIZE. So this function should
> >   * only be used to return pages allocated with efi_allocate_pages() or
> >   * efi_low_alloc_above().
> > @@ -230,15 +230,25 @@ void efi_free(unsigned long size, unsigned long addr)
> >       efi_bs_call(free_pages, addr, nr_pages);
> >  }
> >
> > -/*
> > - * Relocate a kernel image, either compressed or uncompressed.
> > - * In the ARM64 case, all kernel images are currently
> > - * uncompressed, and as such when we relocate it we need to
> > - * allocate additional space for the BSS segment. Any low
> > - * memory that this function should avoid needs to be
> > - * unavailable in the EFI memory map, as if the preferred
> > - * address is not available the lowest available address will
> > - * be used.
> > +/**
> > + * efi_relocate_kernel() - copy memory area
> > + * @image_addr:              address of memory area to copy, on exit target address
>
> The "on exit target address" is a little bit confusing IMO.
> Is it like this?
>
>   On exit, @image_addr is updated to the target copy address that was used.
>

Agreed.

> ?  or some other better description?
>
> Thanks.
>
> Acked-by: Randy Dunlap <rdunlap@infradead.org>
>

Applied to efi/next with Randy's suggestion folded in

Heinrich: I folded the first few hunks into the respective changes of
yours that were already queued up

Thanks,


>
> > + * @image_size:              size of memory area to copy
> > + * @alloc_size:              minimum size of memory to allocate, must be greater or
> > + *                   equal to image_size
> > + * @preferred_addr:  preferred target address
> > + * @alignment:               minimum alignment of the allocated memory area. It
> > + *                   should be a power of two.
> > + * @min_addr:                minimum target address
> > + *
> > + * Copy a memory area to a newly allocated memory area aligned according
> > + * to @alignment but at least EFI_ALLOC_ALIGN. If the preferred address
> > + * is not available, the allocated address will not be below @min_addr.
> > + *
> > + * This function is used to copy the Linux kernel verbatim. It does not apply
> > + * any relocation changes.
> > + *
> > + * Return:           status code
> >   */
> >  efi_status_t efi_relocate_kernel(unsigned long *image_addr,
> >                                unsigned long image_size,
> > --
> > 2.25.0
> >
>
>
> --
> ~Randy
>