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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.07.2023 14:48, Daniel P. Smith wrote:
> 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.

No. I was trying to clarify that we're talking about physical addresses
here. Which you still seem to have trouble with, ...

>>>   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)

... according to all of this and ...

>>> 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).

... this. What C talks about is what the CPU can address (within a single
address space, i.e. normally virtual addresses). With 32-bit addresses
you can address at most 4G, when the system you're running on may have
much more memory. Yet in an OS or hypervisor you need to deal with this
larger amount of memory, no matter that you can't address all of it in
one go.

Jan