[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/21/23 18:08, Stefano Stabellini wrote:
On Fri, 21 Jul 2023, 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.

Right. And I think size_t is defined as 32-bit in Xen which is a
problem.

In x86 32bit early boot code it is 32bits, but in Xen proper it is 64bits. That is why in the 32bit HL code, a second set of structures was used with macros to ensure the structures used 64bit values for field types that are not 64bits in 32bit mode code.

v/r,
dps



 


Rackspace

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