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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 21 Jul 2023, Jan Beulich wrote:
> On 21.07.2023 00:12, Christopher Clark wrote:
> > 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.
> 
> "Any object in memory" implies virtual addresses (or more generally addresses
> which can be used for accessing objects). This isn't the case when considering
> physical addresses - there may be far more memory in a system than can be made
> accessible all in one go.

Right. And I think size_t is defined as 32-bit in Xen which is a
problem.