Re: [PATCH 03/10] x86 setup: change bootstrap map to accept new boot module structures

[Christopher Clark <christopher.w.clark@xxxxxxxxx>]

On Thu, Jul 13, 2023 at 11:51 PM Christopher Clark <christopher.w.clark@xxxxxxxxx> wrote:

On Sat, Jul 8, 2023 at 11:47 AM Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:

On Sat, 1 Jul 2023, Christopher Clark wrote:
> To convert the x86 boot logic from multiboot to boot module structures,
> change the bootstrap map function to accept a boot module parameter.
>
> To allow incremental change from multiboot to boot modules across all
> x86 setup logic, provide a temporary inline wrapper that still accepts a
> multiboot module parameter and use it where necessary. The wrapper is
> placed in a new arch/x86 header <asm/boot.h> to avoid putting a static
> inline function into an existing header that has no such functions
> already. This new header will be expanded with additional functions in
> subsequent patches in this series.
>
> No functional change intended.
>
> Signed-off-by: Christopher Clark <christopher.w.clark@xxxxxxxxx>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>

[...]

> diff --git a/xen/include/xen/bootinfo.h b/xen/include/xen/bootinfo.h
> index b72ae31a66..eb93cc3439 100644
> --- a/xen/include/xen/bootinfo.h
> +++ b/xen/include/xen/bootinfo.h
> @@ -10,6 +10,9 @@
>  #endif
> 
>  struct boot_module {
> +    paddr_t start;
> +    size_t size;

I think size should be paddr_t (instead of size_t) to make sure it is
the right size on both 64-bit and 32-bit architectures that support
64-bit addresses.

Thanks, that explanation does make sense - ack.

I've come back to reconsider this as it doesn't seem right to me to store a non-address value (which this will always be) in a type explicitly defined to hold an address: addresses may have architectural alignment requirements whereas a size value is just a number of bytes so will not. The point of a size_t value is that size_t is defined to be large enough to hold the size of any valid object in memory, so I think this was right as-is.

Christopher

 

Christopher

 

>      struct arch_bootmodule *arch;
>  };