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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

Jan