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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan