[Top] [All Lists]
[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 08:54, Jan Beulich wrote:

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, ...
No, I perfectly understand what you are saying and am not having difficulties 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.


Nothing I said disagrees with your statement.
Let's bring this back to the actual implementation instead of the theoretical. Your position is that Xen's paddr_t is desired because it can store larger values than that of size_t. Now if you look in Xen proper (main 64bit code on x86), paddr_t is a typedef for a 64bit unsigned integer. And if you look at size_t, it is also a typedef to a 64bit unsigned integer, they are literally a couple of lines apart in types.h. Thus they are the same size and can only represent the same maximum size. The only area of issue for x86 is during the short bit of code that runs in 32bit mode during startup. In this series, we address this by using a set of macros in the 32bit code to provide 64bit clean definition of the structures. This approach is acceptable because as far as I am aware, x86 is the only platform where the hypervisor has to transition from one bit size to another, e.g. Arm just starts in 64bit mode when on a 64bit device.
At the end of the day, size_t is the same size as paddr_t for the end execution environments and I would levy a guess that should x86 suddenly find itself having a 128bit mode which would likely drive paddr_t to 128bits, so would follow size_t. 

v/r,
dps

 

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.

Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.