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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 7/21/23 18:08, Stefano Stabellini wrote:
> 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.
In x86 32bit early boot code it is 32bits, but in Xen proper it is 
64bits. That is why in the 32bit HL code, a second set of structures was 
used with macros to ensure the structures used 64bit values for field 
types that are not 64bits in 32bit mode code.
v/r,
dps