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

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

On 7/27/23 07:58, Jan Beulich wrote:
> On 27.07.2023 13:46, Daniel P. Smith wrote:
> > 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.
> I can't see how this relates to my earlier remark.
Perhaps I misunderstood what your point was then. I thought you were 
taking the position that size_t could not be used to represent the 
largest object in memory addressable by a single CPU operation.
> >   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,
> You use "object" here again, when in physical address space (with paging
> enabled) this isn't an appropriate term.
Because that is the language used in the C spec to refer to instances in 
memory,
"Object: region of data storage in the execution environment, the 
contents of which can represent values"
ISO/IEC 9899:1999(E) - 3.14: 
https://www.dii.uchile.cl/~daespino/files/Iso_C_1999_definition.pdf


With the following two interpretations of the spec for size_t to mean 
(any emphasis being mine),

"size_t is an unsigned integer type used to represent the size of any 
**object** (including arrays) in the particular implementation."
Wikipedia - size_t: https://en.wikipedia.org/wiki/C_data_types#stddef.h


"size_t can store the maximum size of a theoretically possible 
**object** of any type (including array)."
CPP Ref - size_t: (https://en.cppreference.com/w/c/types/size_t)


> > 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.
> It is type abuse to a certain extent, yes, but what do you do? We could
> invent psize_t, but that would (afaics) always match paddr_t. uint64_t
> otoh may be too larger for 32-bit platforms which only know a 32-bit
> wide physical address space.
Why invent a new type? That is the purpose of `size_t`, and it should be 
of the correct size, otherwise Xen's implementation is incorrect (which 
it is not).
dps