[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 03/10] x86 setup: change bootstrap map to accept new boot module structures
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |