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

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

On 7/21/23 02:14, 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.
That is not my understanding of it, but I could be wrong. My 
understanding based on all the debates I have read online around this 
topic is that the intent in the spec is that size_t has to be able to 
hold a value that represents the largest object the CPU can manipulate 
with general purpose operations. From which I understand to mean as 
large as the largest register a CPU instruction may use for a size 
argument to a general purpose instruction. On x86_64, that is a 64bit 
register, as I don't believe the SSE/AVX registers are counted even 
though the are used by compiler/libc implementations to optimize some 
memory operations.
From what I have seen for Xen, this is currently reflected in the x86 
code base, as size_t is 32bits for the early 32bit code and 64bits for 
Xen proper.
That aside, another objection I have to the use of paddr_t is that it is 
type abuse. Types are meant to convey context to the intended use of the 
variable and enable the ability to enforce proper usage of the variable, 
otherwise we might as well just use u64/uint64_t and be done. The 
field's purpose is to convey a size of an object, and labeling it a type 
that is intended for physical address objects violates both intents 
behind declaring a type, it asserts an invalid context and enables 
violations of type checking.
V/r,
Daniel P. Smith