[PATCH v7 0/2] x86/pvh: Support relocating dom0 kernel

[PATCH v7 0/2] x86/pvh: Support relocating dom0 kernel

[Jason Andryuk]

Xen tries to load a PVH dom0 kernel at the fixed guest physical address
from the elf headers.  For Linux, this defaults to 0x1000000 (16MB), but
it can be configured.

Unfortunately there exist firmwares that have reserved regions at this
address, so Xen fails to load the dom0 kernel since it's not RAM.

The other issue is that the Linux PVH entry point is not
position-independent.  It expects to run at the compiled
CONFIG_PHYSICAL_ADDRESS.

This patch set expands the PVH dom0 builder to try to relocate the
kernel if needed and possible.  XEN_ELFNOTE_PHYS32_RELOC is added for
kernels to indicate they are relocatable and their acceptable address
range and alignment.

Choose the alignment from the Note if specified, or the Maximum PHDR
p_align value if greater than PAGE_SIZE.  Otherwise, it falls back to
the default 2MB.

Patches from v6 commited:
  tools/init-xenstore-domain: Replace variable MB() usage
  tools: Move MB/GB() to common-macros.h

The first patch stores the maximum p_align value from the ELF PHDRs.

The second patch expands the pvh dom0 kernel placement code.

I'll post an additional patch showing the Linux changes to make PVH
relocatable.

Jason Andryuk (2):
  libelf: Store maximum PHDR p_align
  x86/PVH: Support relocatable dom0 kernels

 xen/arch/x86/hvm/dom0_build.c      | 108 +++++++++++++++++++++++++++++
 xen/common/libelf/libelf-dominfo.c |  35 ++++++++++
 xen/common/libelf/libelf-loader.c  |   5 ++
 xen/common/libelf/libelf-private.h |   1 +
 xen/include/public/elfnote.h       |  20 +++++-
 xen/include/xen/libelf.h           |   5 ++
 6 files changed, 173 insertions(+), 1 deletion(-)

-- 
2.44.0

[PATCH v7 1/2] libelf: Store maximum PHDR p_align

[Jason Andryuk]

While parsing the PHDRs, store the maximum p_align value.  This may be
consulted for moving a PVH image's load address.

Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
v6:
New

v7:
Remove printing p_align.  It may be confusing and is only useful when
a PVH kernel needs to be moved.
---
 xen/common/libelf/libelf-loader.c | 5 +++++
 xen/include/xen/libelf.h          | 1 +
 2 files changed, 6 insertions(+)

diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 629cc0d3e6..e571ea670e 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -468,6 +468,7 @@ void elf_parse_binary(struct elf_binary *elf)
 {
     ELF_HANDLE_DECL(elf_phdr) phdr;
     uint64_t low = -1, high = 0, paddr, memsz;
+    uint64_t max_align = 0, palign;
     unsigned i, count;
 
     count = elf_phdr_count(elf);
@@ -481,15 +482,19 @@ void elf_parse_binary(struct elf_binary *elf)
             continue;
         paddr = elf_uval(elf, phdr, p_paddr);
         memsz = elf_uval(elf, phdr, p_memsz);
+        palign = elf_uval(elf, phdr, p_align);
         elf_msg(elf, "ELF: phdr: paddr=%#" PRIx64 " memsz=%#" PRIx64 "\n",
                 paddr, memsz);
         if ( low > paddr )
             low = paddr;
         if ( high < paddr + memsz )
             high = paddr + memsz;
+        if ( max_align < palign )
+            max_align = palign;
     }
     elf->pstart = low;
     elf->pend = high;
+    elf->palign = max_align;
     elf_msg(elf, "ELF: memory: %#" PRIx64 " -> %#" PRIx64 "\n",
             elf->pstart, elf->pend);
 }
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 1c77e3df31..2d971f958e 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -196,6 +196,7 @@ struct elf_binary {
     size_t dest_size;
     uint64_t pstart;
     uint64_t pend;
+    uint64_t palign;
     uint64_t reloc_offset;
 
     uint64_t bsd_symtab_pstart;
-- 
2.44.0

[PATCH v7 2/2] x86/PVH: Support relocatable dom0 kernels

[Jason Andryuk]

Xen tries to load a PVH dom0 kernel at the fixed guest physical address
from the elf headers.  For Linux, this defaults to 0x1000000 (16MB), but
it can be configured.

Unfortunately there exist firmwares that have reserved regions at this
address, so Xen fails to load the dom0 kernel since it's not RAM.

The PVH entry code is not relocatable - it loads from absolute
addresses, which fail when the kernel is loaded at a different address.
With a suitably modified kernel, a reloctable entry point is possible.

Add XEN_ELFNOTE_PHYS32_RELOC which specifies optional alignment,
minimum, and maximum addresses needed for the kernel.  The presence of
the NOTE indicates the kernel supports a relocatable entry path.

Change the loading to check for an acceptable load address.  If the
kernel is relocatable, support finding an alternate load address.

The primary motivation for an explicit align field is that Linux has a
configurable CONFIG_PHYSICAL_ALIGN field.  This value is present in the
bzImage setup header, but not the ELF program headers p_align, which
report 2MB even when CONFIG_PHYSICAL_ALIGN is greater.  Since a kernel
is only considered relocatable if the PHYS32_RELOC elf note is present,
the alignment contraints can just be specified within the note instead
of searching for an alignment value via a heuristic.

Load alignment uses the PHYS32_RELOC note value if specified.
Otherwise, the maxmum ELF PHDR p_align value is selected if greater than
or equal to PAGE_SIZE.  Finally, the fallback default is 2MB.

libelf-private.h includes common-macros.h to satisfy the fuzzer build.

Link: https://gitlab.com/xen-project/xen/-/issues/180
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
ELF Note printing looks like:
(XEN) ELF: note: PHYS32_RELOC align: 0x200000 min: 0x1000000 max: 0x3fffffff

v2:
Use elfnote for min, max & align - use 64bit values.
Print original and relocated memory addresses
Use check_and_adjust_load_address() name
Return relocated base instead of offset
Use PAGE_ALIGN
Don't load above max_phys (expected to be 4GB in kernel elf note)
Use single line comments
Exit check_load_address loop earlier
Add __init to find_kernel_memory()

v3:
Remove kernel_start/end page rounding
Change loop comment to rely on a sorted memory map.
Reorder E820_RAM check first
Use %p for dest_base
Use PRIpaddr
Use 32bit phys_min/max/align
Consolidate to if ( x || y ) continue
Use max_t
Add parms->phys_reloc
Use common "%pd kernel: " prefix for messages
Re-order phys_entry assignment
Print range ends inclusively
Remove extra "Unable to load kernel" message
s/PVH_RELOCATION/PHYS32_RELOC/
Make PHYS32_RELOC contents optional
Use 2MB default alignment
Update ELF Note comment
Update XEN_ELFNOTE_MAX

v4:
Cast dest_base to uintptr_t
Use local start variable
Mention e820 requiring adjacent entries merged
Remove extra whitespace
Re-word elfnote comment & mention x86
Use ELFNOTE_NAME
Return -ENOSPC
Use ! instead of == 0
Check kend - 1 to avoid off by one
libelf: Use MB/GB() to define the size.
Use ARCH_PHYS_* defines

v5:
Place kernel in higher memory addresses
Remove stray semicolons
ELFNOTE_NAME comment about newline
Make PHYS32_RELOC element order align, min, max
Re-word PHYS32_RELOC comment
Move phys_align next to other bool variables

v6:
Select alignment from, in order, Note, PHDRs, then default

v7:
Re-format phys_entry assignment
Mention align selection in PHYS_RELOC comment
Use palign >= PAGE_SIZE
---
 xen/arch/x86/hvm/dom0_build.c      | 108 +++++++++++++++++++++++++++++
 xen/common/libelf/libelf-dominfo.c |  35 ++++++++++
 xen/common/libelf/libelf-private.h |   1 +
 xen/include/public/elfnote.h       |  20 +++++-
 xen/include/xen/libelf.h           |   4 ++
 5 files changed, 167 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 0ceda4140b..d158f4d241 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -537,6 +537,111 @@ static paddr_t __init find_memory(
     return INVALID_PADDR;
 }
 
+static bool __init check_load_address(
+    const struct domain *d, const struct elf_binary *elf)
+{
+    paddr_t kernel_start = (uintptr_t)elf->dest_base;
+    paddr_t kernel_end = kernel_start + elf->dest_size;
+    unsigned int i;
+
+    /* Relies on a sorted memory map with adjacent entries merged. */
+    for ( i = 0; i < d->arch.nr_e820; i++ )
+    {
+        paddr_t start = d->arch.e820[i].addr;
+        paddr_t end = start + d->arch.e820[i].size;
+
+        if ( start >= kernel_end )
+            return false;
+
+        if ( d->arch.e820[i].type == E820_RAM &&
+             start <= kernel_start &&
+             end >= kernel_end )
+            return true;
+    }
+
+    return false;
+}
+
+/* Find an e820 RAM region that fits the kernel at a suitable alignment. */
+static paddr_t __init find_kernel_memory(
+    const struct domain *d, struct elf_binary *elf,
+    const struct elf_dom_parms *parms)
+{
+    paddr_t kernel_size = elf->dest_size;
+    unsigned int align;
+    int i;
+
+    if ( parms->phys_align != UNSET_ADDR32 )
+        align = parms->phys_align;
+    else if ( elf->palign >= PAGE_SIZE )
+        align = elf->palign;
+    else
+        align = MB(2);
+
+    /* Search backwards to find the highest address. */
+    for ( i = d->arch.nr_e820 - 1; i >= 0 ; i-- )
+    {
+        paddr_t start = d->arch.e820[i].addr;
+        paddr_t end = start + d->arch.e820[i].size;
+        paddr_t kstart, kend;
+
+        if ( d->arch.e820[i].type != E820_RAM ||
+             d->arch.e820[i].size < kernel_size )
+            continue;
+
+        if ( start > parms->phys_max )
+            continue;
+
+        if ( end - 1 > parms->phys_max )
+            end = parms->phys_max + 1;
+
+        kstart = (end - kernel_size) & ~(align - 1);
+        kend = kstart + kernel_size;
+
+        if ( kstart < parms->phys_min )
+            return 0;
+
+        if ( kstart >= start && kend <= end )
+            return kstart;
+    }
+
+    return 0;
+}
+
+/* Check the kernel load address, and adjust if necessary and possible. */
+static bool __init check_and_adjust_load_address(
+    const struct domain *d, struct elf_binary *elf, struct elf_dom_parms *parms)
+{
+    paddr_t reloc_base;
+
+    if ( check_load_address(d, elf) )
+        return true;
+
+    if ( !parms->phys_reloc )
+    {
+        printk("%pd kernel: Address conflict and not relocatable\n", d);
+        return false;
+    }
+
+    reloc_base = find_kernel_memory(d, elf, parms);
+    if ( !reloc_base )
+    {
+        printk("%pd kernel: Failed find a load address\n", d);
+        return false;
+    }
+
+    if ( opt_dom0_verbose )
+        printk("%pd kernel: Moving [%p, %p] -> [%"PRIpaddr", %"PRIpaddr"]\n", d,
+               elf->dest_base, elf->dest_base + elf->dest_size - 1,
+               reloc_base, reloc_base + elf->dest_size - 1);
+
+    parms->phys_entry =
+        reloc_base + (parms->phys_entry - (uintptr_t)elf->dest_base);
+    elf->dest_base = (char *)reloc_base;
+
+    return true;
+}
+
 static int __init pvh_load_kernel(struct domain *d, const module_t *image,
                                   unsigned long image_headroom,
                                   module_t *initrd, void *image_base,
@@ -585,6 +690,9 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image,
     elf.dest_base = (void *)(parms.virt_kstart - parms.virt_base);
     elf.dest_size = parms.virt_kend - parms.virt_kstart;
 
+    if ( !check_and_adjust_load_address(d, &elf, &parms) )
+        return -ENOSPC;
+
     elf_set_vcpu(&elf, v);
     rc = elf_load_binary(&elf);
     if ( rc < 0 )
diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 0cdb419b8a..260294d0ed 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -17,6 +17,14 @@
 
 #include "libelf-private.h"
 
+#if defined(__i386__) || defined(__x86_64__)
+#define ARCH_PHYS_MIN_DEFAULT   0
+#define ARCH_PHYS_MAX_DEFAULT   (GB(4) - 1)
+#else
+#define ARCH_PHYS_MIN_DEFAULT   0
+#define ARCH_PHYS_MAX_DEFAULT   0
+#endif
+
 /* ------------------------------------------------------------------------ */
 /* xen features                                                             */
 
@@ -125,6 +133,7 @@ elf_errorstatus elf_xen_parse_note(struct elf_binary *elf,
         [XEN_ELFNOTE_SUSPEND_CANCEL] = { "SUSPEND_CANCEL", ELFNOTE_INT },
         [XEN_ELFNOTE_MOD_START_PFN] = { "MOD_START_PFN", ELFNOTE_INT },
         [XEN_ELFNOTE_PHYS32_ENTRY] = { "PHYS32_ENTRY", ELFNOTE_INT },
+        [XEN_ELFNOTE_PHYS32_RELOC] = { "PHYS32_RELOC", ELFNOTE_NAME },
     };
 /* *INDENT-ON* */
 
@@ -132,6 +141,7 @@ elf_errorstatus elf_xen_parse_note(struct elf_binary *elf,
     uint64_t val = 0;
     unsigned int i;
     unsigned type = elf_uval(elf, note, type);
+    unsigned descsz = elf_uval(elf, note, descsz);
 
     if ( (type >= sizeof(note_desc) / sizeof(note_desc[0])) ||
          (note_desc[type].name == NULL) )
@@ -228,6 +238,27 @@ elf_errorstatus elf_xen_parse_note(struct elf_binary *elf,
     case XEN_ELFNOTE_PHYS32_ENTRY:
         parms->phys_entry = val;
         break;
+
+    case XEN_ELFNOTE_PHYS32_RELOC:
+        parms->phys_reloc = true;
+
+        if ( descsz >= 4 )
+        {
+            parms->phys_align = elf_note_numeric_array(elf, note, 4, 0);
+            elf_msg(elf, " align: %#"PRIx32, parms->phys_align);
+        }
+        if ( descsz >= 8 )
+        {
+            parms->phys_min = elf_note_numeric_array(elf, note, 4, 1);
+            elf_msg(elf, " min: %#"PRIx32, parms->phys_min);
+        }
+        if ( descsz >= 12 )
+        {
+            parms->phys_max = elf_note_numeric_array(elf, note, 4, 2);
+            elf_msg(elf, " max: %#"PRIx32, parms->phys_max);
+        }
+
+        break;
     }
 
     if ( note_desc[type].type == ELFNOTE_NAME)
@@ -543,6 +574,10 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
     parms->p2m_base = UNSET_ADDR;
     parms->elf_paddr_offset = UNSET_ADDR;
     parms->phys_entry = UNSET_ADDR32;
+    parms->phys_align = UNSET_ADDR32;
+    parms->phys_min = ARCH_PHYS_MIN_DEFAULT;
+    parms->phys_max = ARCH_PHYS_MAX_DEFAULT;
+    parms->phys_reloc = false;
 
     /* Find and parse elf notes. */
     count = elf_phdr_count(elf);
diff --git a/xen/common/libelf/libelf-private.h b/xen/common/libelf/libelf-private.h
index 47db679966..98cac65bc5 100644
--- a/xen/common/libelf/libelf-private.h
+++ b/xen/common/libelf/libelf-private.h
@@ -71,6 +71,7 @@
 #endif
 #include <xen/elfnote.h>
 #include <xen/libelf/libelf.h>
+#include <xen-tools/common-macros.h>
 
 #ifndef FUZZ_NO_LIBXC
 #include "xenctrl.h"
diff --git a/xen/include/public/elfnote.h b/xen/include/public/elfnote.h
index 1d84b05f44..5dfed6bc3e 100644
--- a/xen/include/public/elfnote.h
+++ b/xen/include/public/elfnote.h
@@ -196,10 +196,28 @@
  */
 #define XEN_ELFNOTE_PHYS32_ENTRY 18
 
+/*
+ * Physical loading constraints for PVH kernels
+ *
+ * The presence of this note indicates the kernel supports relocating itself.
+ *
+ * The note may include up to three 32bit values to place constraints on the
+ * guest physical loading addresses and alignment for a PVH kernel.  Values
+ * are read in the following order:
+ *  - a required start alignment (default 0x200000)
+ *  - a minimum address for the start of the image (default 0)
+ *  - a maximum address for the last byte of the image (default 0xffffffff)
+ *
+ *  When this note specifies an alignment value, it is used.  Otherwise the
+ *  maximum p_align value from loadable ELF Program Headers is used, if it is
+ *  greater than or equal to 4k (0x1000).  Otherwise, the default is used.
+ */
+#define XEN_ELFNOTE_PHYS32_RELOC 19
+
 /*
  * The number of the highest elfnote defined.
  */
-#define XEN_ELFNOTE_MAX XEN_ELFNOTE_PHYS32_ENTRY
+#define XEN_ELFNOTE_MAX XEN_ELFNOTE_PHYS32_RELOC
 
 /*
  * System information exported through crash notes.
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 2d971f958e..9ac530acc2 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -426,6 +426,7 @@ struct elf_dom_parms {
     enum xen_pae_type pae;
     bool bsd_symtab;
     bool unmapped_initrd;
+    bool phys_reloc;
     uint64_t virt_base;
     uint64_t virt_entry;
     uint64_t virt_hypercall;
@@ -435,6 +436,9 @@ struct elf_dom_parms {
     uint32_t f_supported[XENFEAT_NR_SUBMAPS];
     uint32_t f_required[XENFEAT_NR_SUBMAPS];
     uint32_t phys_entry;
+    uint32_t phys_align;
+    uint32_t phys_min;
+    uint32_t phys_max;
 
     /* calculated */
     uint64_t virt_kstart;
-- 
2.44.0

[PATCH v7] RFC: x86/pvh: Make Xen PVH entrypoint PIC for x86-64

[Jason Andryuk]

The Xen PVH entrypoint is 32bit non-PIC code running at a default load
address of 0x1000000 (16MB) (CONFIG_PHYSICAL_START).  Xen loads the
kernel at that physical address inside the PVH container.

When running a PVH Dom0, the system reserved addresses are mapped 1-1
into the PVH container.  There exist system firmwares (Coreboot/EDK2)
with reserved memory at 16MB.  This creates a conflict where the PVH
kernel cannot be loaded at that address.

Modify the PVH entrypoint to be position-indepedent to allow flexibility
in load address.  Only the 64bit entry path is converted.  A 32bit
kernel is not PIC, so calling into other parts of the kernel, like
xen_prepare_pvh() and mk_pgtable_32(), don't work properly when
relocated.

Initial PVH entry runs at the physical addresses and then transitions to
the identity mapped address.  While executing xen_prepare_pvh() calls
through pv_ops function pointers transition to the high mapped
addresses.  Additionally, __va() is called on some hvm_start_info
physical addresses, we need the directmap address range is used.  So we
need to run page tables with all of those ranges mapped.

Modifying init_top_pgt tables ran into issue since
startup_64/__startup_64() will modify those page tables again.  Create
dedicated set of page tables - pvh_init_top_pgt  - for the PVH entry to
avoid unwanted interactions.

To avoid statically allocating space, the .bss is used.  It is unused on
entry and only cleared later.  This saves 32kb that would otherwise be
added to vmlinux.   A minimum of 20kb would be needed, which would only
map the lower 1GB or ram.

In xen_pvh_init(), __pa() is called to find the physical address of the
hypercall page.  Set phys_base temporarily before calling into
xen_prepare_pvh(), which calls xen_pvh_init(), and clear it afterwards.
__startup_64() assumes phys_base is zero and adds load_delta to it.  If
phys_base is already set, the calculation results in an incorrect cr3.

TODO: Sync elfnote.h from xen.git commit xxxxxxxxxx when it is
commited.

Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
Put this out as an example for the Xen modifications

UNWIND_HINT_END_OF_STACK after lret is to silence:
vmlinux.o: warning: objtool: pvh_start_xen+0x168: unreachable instruction

Instead of setting and clearing phys_base, add a dedicated variable?
Clearing phys_base is a little weird, but it leaves the kernel more
consistent when running non-entry code.

Make __startup_64() exit if phys_base is already set to allow calling
multiple times, and use that and init_top_pgt instead of adding
additional page tables?  That won't work.  __startup_64 is 64bit code,
and pvh needs to create page tables in 32bit code before it can
transition to 64bit long mode.  Hence it can't be re-used to relocate
page tables.
---
 arch/x86/platform/pvh/head.S    | 168 +++++++++++++++++++++++++++++---
 include/xen/interface/elfnote.h |  20 +++-
 2 files changed, 175 insertions(+), 13 deletions(-)

diff --git a/arch/x86/platform/pvh/head.S b/arch/x86/platform/pvh/head.S
index f7235ef87bc3..48480fc8d786 100644
--- a/arch/x86/platform/pvh/head.S
+++ b/arch/x86/platform/pvh/head.S
@@ -50,11 +50,32 @@
 #define PVH_CS_SEL		(PVH_GDT_ENTRY_CS * 8)
 #define PVH_DS_SEL		(PVH_GDT_ENTRY_DS * 8)
 
+#define rva(x) ((x) - pvh_start_xen)
+
 SYM_CODE_START_LOCAL(pvh_start_xen)
 	UNWIND_HINT_END_OF_STACK
 	cld
 
-	lgdt (_pa(gdt))
+	/*
+	 * See the comment for startup_32 for more details.  We need to
+	 * execute a call to get the execution address to be position
+	 * independent, but we don't have a stack.  Save and restore the
+	 * magic field of start_info in ebx, and use that as the stack.
+	 */
+	mov	(%ebx), %eax
+	leal	4(%ebx), %esp
+	ANNOTATE_INTRA_FUNCTION_CALL
+	call	1f
+1:	popl	%ebp
+	mov	%eax, (%ebx)
+	subl	$ rva(1b), %ebp
+	movl	$0, %esp
+
+	leal	rva(gdt)(%ebp), %eax
+	movl	%eax, %ecx
+	leal	rva(gdt_start)(%ebp), %ecx
+	movl	%ecx, 2(%eax)
+	lgdt	(%eax)
 
 	mov $PVH_DS_SEL,%eax
 	mov %eax,%ds
@@ -62,14 +83,14 @@ SYM_CODE_START_LOCAL(pvh_start_xen)
 	mov %eax,%ss
 
 	/* Stash hvm_start_info. */
-	mov $_pa(pvh_start_info), %edi
+	leal rva(pvh_start_info)(%ebp), %edi
 	mov %ebx, %esi
-	mov _pa(pvh_start_info_sz), %ecx
+	movl rva(pvh_start_info_sz)(%ebp), %ecx
 	shr $2,%ecx
 	rep
 	movsl
 
-	mov $_pa(early_stack_end), %esp
+	leal rva(early_stack_end)(%ebp), %esp
 
 	/* Enable PAE mode. */
 	mov %cr4, %eax
@@ -83,29 +104,145 @@ SYM_CODE_START_LOCAL(pvh_start_xen)
 	btsl $_EFER_LME, %eax
 	wrmsr
 
-	/* Enable pre-constructed page tables. */
-	mov $_pa(init_top_pgt), %eax
+/*
+ * Xen PVH needs a set of identity mapped and kernel high mapping
+ * page tables.  pvh_start_xen starts running on the identity mapped
+ * page tables, but xen_prepare_pvh calls into the high mapping.  Inside,
+ * __va is used, so the L4_PAGE_OFFSET needs to be populated.
+ *
+ * Instead of allocating memory memory in .init.data, we just use
+ * space in the BSS.  This will only be used until startup_64 switches
+ * to the main page tables.  Later, the bss will be cleared.
+ */
+#define pvh_init_top_pgt      rva(__bss_start)
+#define pvh_level3_ident_pgt  (pvh_init_top_pgt + PAGE_SIZE)
+#define pvh_level2_ident_pgt  (pvh_level3_ident_pgt + PAGE_SIZE)
+#define pvh_level3_kernel_pgt (pvh_level2_ident_pgt + 4 * PAGE_SIZE)
+#define pvh_level2_kernel_pgt (pvh_level3_kernel_pgt + PAGE_SIZE)
+
+#define l4_index(x)     (((x) >> 39) & 511)
+#define pud_index(x)    (((x) >> PUD_SHIFT) & (PTRS_PER_PUD-1))
+
+L4_PAGE_OFFSET  = l4_index(__PAGE_OFFSET_BASE_L4)
+L4_START_KERNEL = l4_index(__START_KERNEL_map)
+L3_START_KERNEL = pud_index(__START_KERNEL_map)
+
+	/* Clear pvh_init_top_pgt */
+	leal pvh_init_top_pgt(%ebp), %edi
+	movl $(PAGE_SIZE / 4), %ecx
+	xorl %eax, %eax
+	rep stosl
+
+	/* pvh_init_top_pgt[0] = pvh_level3_ident_pgt */
+	leal pvh_init_top_pgt(%ebp), %edi
+	leal pvh_level3_ident_pgt(%ebp), %esi
+	movl %esi, 0x00(%edi)
+	addl $_KERNPG_TABLE_NOENC, 0x00(%edi)
+
+	/* pvh_init_top_pgt[L4_PAGE_OFFSET] = pvh_level3_ident_pgt */
+	movl %esi, (L4_PAGE_OFFSET * 8)(%edi)
+	addl $_KERNPG_TABLE_NOENC, (L4_PAGE_OFFSET * 8)(%edi)
+
+	/* pvh_init_top_pgt[L4_START_KERNEL] = pvh_level3_kernel_pgt */
+	leal pvh_level3_kernel_pgt(%ebp), %esi
+	movl %esi, (L4_START_KERNEL * 8)(%edi)
+	addl $_PAGE_TABLE_NOENC, (L4_START_KERNEL * 8)(%edi)
+
+	/* Clear pvh_level3_ident_pgt */
+	leal pvh_level3_ident_pgt(%ebp), %edi
+	movl $(PAGE_SIZE / 4), %ecx
+	xorl %eax, %eax
+	rep stosl
+
+	/* Set pvh_level3_ident_pgt[0] = pvh_level2_ident_pgt */
+	leal pvh_level3_ident_pgt(%ebp), %edi
+	leal pvh_level2_ident_pgt(%ebp), %esi
+	addl $_KERNPG_TABLE_NOENC, %esi
+	movl %esi, 0x00(%edi)
+	addl $PAGE_SIZE, %esi
+	/* ... pvh_level3_ident_pgt[1] = pvh_level2_ident_pgt+0x1000 */
+	movl %esi, 0x08(%edi)
+	addl $PAGE_SIZE, %esi
+	/* ... pvh_level3_ident_pgt[2] = pvh_level2_ident_pgt+0x2000 */
+	movl %esi, 0x10(%edi)
+	addl $PAGE_SIZE, %esi
+	/* ... pvh_level3_ident_pgt[3] = pvh_level2_ident_pgt+0x3000 */
+	movl %esi, 0x18(%edi)
+
+	/* Fill pvh_level2_ident_pgt with large ident pages. */
+	leal pvh_level2_ident_pgt(%ebp), %edi
+	movl $__PAGE_KERNEL_IDENT_LARGE_EXEC, %esi
+	movl $(PTRS_PER_PMD*4), %ecx
+1:	movl %esi, 0(%edi)
+	addl $(1 << PMD_SHIFT), %esi
+	addl $8, %edi
+	decl %ecx
+	jnz 1b
+
+	/* Clear pvh_level3_kernel_pgt */
+	leal pvh_level3_kernel_pgt(%ebp), %edi
+	movl $(PAGE_SIZE / 4), %ecx
+	xorl %eax, %eax
+	rep stosl
+
+	/* pvh_level3_kernel_pgt[L3_START_KERNEL] = pvh_level2_kernel_pgt */
+	leal pvh_level3_kernel_pgt(%ebp), %edi
+	leal pvh_level2_kernel_pgt(%ebp), %esi
+	movl %esi, (L3_START_KERNEL * 8)(%edi)
+	addl $_KERNPG_TABLE_NOENC, (L3_START_KERNEL * 8)(%edi)
+
+	mov %ebp, %ebx
+	subl $LOAD_PHYSICAL_ADDR, %ebx /* offset */
+
+	/* Fill pvh_level2_kernel_pgt with large pages. */
+	leal pvh_level2_kernel_pgt(%ebp), %edi
+	movl $__PAGE_KERNEL_LARGE_EXEC, %esi
+	addl %ebx, %esi
+	movl $(KERNEL_IMAGE_SIZE / PMD_SIZE), %ecx
+1:	movl %esi, 0(%edi)
+	addl $(1 << PMD_SHIFT), %esi
+	addl $8, %edi
+	decl %ecx
+	jnz 1b
+
+	/* Switch to page tables. */
+	leal pvh_init_top_pgt(%ebp), %eax
 	mov %eax, %cr3
 	mov $(X86_CR0_PG | X86_CR0_PE), %eax
 	mov %eax, %cr0
 
 	/* Jump to 64-bit mode. */
-	ljmp $PVH_CS_SEL, $_pa(1f)
+	pushl $PVH_CS_SEL
+	leal  rva(1f)(%ebp), %eax
+	pushl %eax
+	lretl
 
 	/* 64-bit entry point. */
 	.code64
 1:
+	UNWIND_HINT_END_OF_STACK
+
 	/* Set base address in stack canary descriptor. */
 	mov $MSR_GS_BASE,%ecx
-	mov $_pa(canary), %eax
+	leal rva(canary)(%ebp), %eax
 	xor %edx, %edx
 	wrmsr
 
+	/* Calculate load offset from LOAD_PHYSICAL_ADDR and store in
+	 * phys_base.  __pa() needs phys_base set to calculate the the
+	 * hypercall page in xen_pvh_init(). */
+	movq %rbp, %rbx
+	subq $LOAD_PHYSICAL_ADDR, %rbx
+	movq %rbx, phys_base(%rip)
 	call xen_prepare_pvh
+	/* Clear phys_base.  startup_64/__startup_64 will *add* to its value,
+	   so start from 0. */
+	xor  %rbx, %rbx
+	movq %rbx, phys_base(%rip)
 
 	/* startup_64 expects boot_params in %rsi. */
-	mov $_pa(pvh_bootparams), %rsi
-	mov $_pa(startup_64), %rax
+	lea rva(pvh_bootparams)(%ebp), %rsi
+	lea rva(startup_64)(%ebp), %rax
 	ANNOTATE_RETPOLINE_SAFE
 	jmp *%rax
 
@@ -137,13 +274,14 @@ SYM_CODE_START_LOCAL(pvh_start_xen)
 
 	ljmp $PVH_CS_SEL, $_pa(startup_32)
 #endif
+
 SYM_CODE_END(pvh_start_xen)
 
 	.section ".init.data","aw"
 	.balign 8
 SYM_DATA_START_LOCAL(gdt)
-	.word gdt_end - gdt_start
-	.long _pa(gdt_start)
+	.word gdt_end - gdt_start - 1
+	.long _pa(gdt_start) /* x86-64 will overwrite if relocated. */
 	.word 0
 SYM_DATA_END(gdt)
 SYM_DATA_START_LOCAL(gdt_start)
@@ -163,5 +301,11 @@ SYM_DATA_START_LOCAL(early_stack)
 	.fill BOOT_STACK_SIZE, 1, 0
 SYM_DATA_END_LABEL(early_stack, SYM_L_LOCAL, early_stack_end)
 
+#ifdef CONFIG_X86_64
+	ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_RELOC,
+		     .long CONFIG_PHYSICAL_ALIGN;
+		     .long LOAD_PHYSICAL_ADDR;
+		     .long 0xffffffff)
+#endif
 	ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_ENTRY,
 	             _ASM_PTR (pvh_start_xen - __START_KERNEL_map))
diff --git a/include/xen/interface/elfnote.h b/include/xen/interface/elfnote.h
index 38deb1214613..4deb63ca7633 100644
--- a/include/xen/interface/elfnote.h
+++ b/include/xen/interface/elfnote.h
@@ -185,9 +185,27 @@
  */
 #define XEN_ELFNOTE_PHYS32_ENTRY 18
 
+/*
+ * Physical loading constraints for PVH kernels
+ *
+ * The presence of this note indicates the kernel supports relocating itself.
+ *
+ * The note may include up to three 32bit values to place constraints on the
+ * guest physical loading addresses and alignment for a PVH kernel.  Values
+ * are read in the following order:
+ *  - a required start alignment (default 0x200000)
+ *  - a minimum address for the start of the image (default 0)
+ *  - a maximum address for the last byte of the image (default 0xffffffff)
+ *
+ *  When this note specifies an alignment value, it is used.  Otherwise the
+ *  maximum p_align value from loadable ELF Program Headers is used, if it is
+ *  greater than or equal to 4k (0x1000).  Otherwise, the default is used.
+ */
+#define XEN_ELFNOTE_PHYS32_RELOC 19
+
 /*
  * The number of the highest elfnote defined.
  */
-#define XEN_ELFNOTE_MAX XEN_ELFNOTE_PHYS32_ENTRY
+#define XEN_ELFNOTE_MAX XEN_ELFNOTE_PHYS32_RELOC
 
 #endif /* __XEN_PUBLIC_ELFNOTE_H__ */
-- 
2.44.0

Re: [PATCH v7 1/2] libelf: Store maximum PHDR p_align

[Jan Beulich]

On 04.04.2024 23:25, Jason Andryuk wrote:
> While parsing the PHDRs, store the maximum p_align value.  This may be
> consulted for moving a PVH image's load address.
> 
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v7 2/2] x86/PVH: Support relocatable dom0 kernels

[Jan Beulich]

On 04.04.2024 23:25, Jason Andryuk wrote:
> Xen tries to load a PVH dom0 kernel at the fixed guest physical address
> from the elf headers.  For Linux, this defaults to 0x1000000 (16MB), but
> it can be configured.
> 
> Unfortunately there exist firmwares that have reserved regions at this
> address, so Xen fails to load the dom0 kernel since it's not RAM.
> 
> The PVH entry code is not relocatable - it loads from absolute
> addresses, which fail when the kernel is loaded at a different address.
> With a suitably modified kernel, a reloctable entry point is possible.
> 
> Add XEN_ELFNOTE_PHYS32_RELOC which specifies optional alignment,
> minimum, and maximum addresses needed for the kernel.  The presence of
> the NOTE indicates the kernel supports a relocatable entry path.
> 
> Change the loading to check for an acceptable load address.  If the
> kernel is relocatable, support finding an alternate load address.
> 
> The primary motivation for an explicit align field is that Linux has a
> configurable CONFIG_PHYSICAL_ALIGN field.  This value is present in the
> bzImage setup header, but not the ELF program headers p_align, which
> report 2MB even when CONFIG_PHYSICAL_ALIGN is greater.  Since a kernel
> is only considered relocatable if the PHYS32_RELOC elf note is present,
> the alignment contraints can just be specified within the note instead
> of searching for an alignment value via a heuristic.
> 
> Load alignment uses the PHYS32_RELOC note value if specified.
> Otherwise, the maxmum ELF PHDR p_align value is selected if greater than
> or equal to PAGE_SIZE.  Finally, the fallback default is 2MB.
> 
> libelf-private.h includes common-macros.h to satisfy the fuzzer build.
> 
> Link: https://gitlab.com/xen-project/xen/-/issues/180
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
yet still with two remarks:

> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -537,6 +537,111 @@ static paddr_t __init find_memory(
>      return INVALID_PADDR;
>  }
>  
> +static bool __init check_load_address(
> +    const struct domain *d, const struct elf_binary *elf)
> +{
> +    paddr_t kernel_start = (uintptr_t)elf->dest_base;
> +    paddr_t kernel_end = kernel_start + elf->dest_size;
> +    unsigned int i;

While properly typed here, ...

> +static paddr_t __init find_kernel_memory(
> +    const struct domain *d, struct elf_binary *elf,
> +    const struct elf_dom_parms *parms)
> +{
> +    paddr_t kernel_size = elf->dest_size;
> +    unsigned int align;
> +    int i;

... I must have missed when this was changed to plain int. It should have
been unsigned int here, too, ...

> +    if ( parms->phys_align != UNSET_ADDR32 )
> +        align = parms->phys_align;
> +    else if ( elf->palign >= PAGE_SIZE )
> +        align = elf->palign;
> +    else
> +        align = MB(2);
> +
> +    /* Search backwards to find the highest address. */
> +    for ( i = d->arch.nr_e820 - 1; i >= 0 ; i-- )

... with this suitably adjusted. However, I'm not going to change this while
committing, to avoid screwing up.

> --- a/xen/include/public/elfnote.h
> +++ b/xen/include/public/elfnote.h
> @@ -196,10 +196,28 @@
>   */
>  #define XEN_ELFNOTE_PHYS32_ENTRY 18
>  
> +/*
> + * Physical loading constraints for PVH kernels
> + *
> + * The presence of this note indicates the kernel supports relocating itself.
> + *
> + * The note may include up to three 32bit values to place constraints on the
> + * guest physical loading addresses and alignment for a PVH kernel.  Values
> + * are read in the following order:
> + *  - a required start alignment (default 0x200000)

"... (default 0x200000; see below)", i.e. ...

> + *  - a minimum address for the start of the image (default 0)
> + *  - a maximum address for the last byte of the image (default 0xffffffff)
> + *
> + *  When this note specifies an alignment value, it is used.  Otherwise the
> + *  maximum p_align value from loadable ELF Program Headers is used, if it is
> + *  greater than or equal to 4k (0x1000).  Otherwise, the default is used.

... referring to this (which, btw, has one too many padding blanks on the
entire paragraph). I will take the liberty to make this adjustment while
committing.

Jan

Re: [PATCH v7 2/2] x86/PVH: Support relocatable dom0 kernels

[Jason Andryuk]

On 2024-04-08 03:00, Jan Beulich wrote:
> On 04.04.2024 23:25, Jason Andryuk wrote:
>> Xen tries to load a PVH dom0 kernel at the fixed guest physical address
>> from the elf headers.  For Linux, this defaults to 0x1000000 (16MB), but
>> it can be configured.
>>
>> Unfortunately there exist firmwares that have reserved regions at this
>> address, so Xen fails to load the dom0 kernel since it's not RAM.
>>
>> The PVH entry code is not relocatable - it loads from absolute
>> addresses, which fail when the kernel is loaded at a different address.
>> With a suitably modified kernel, a reloctable entry point is possible.
>>
>> Add XEN_ELFNOTE_PHYS32_RELOC which specifies optional alignment,
>> minimum, and maximum addresses needed for the kernel.  The presence of
>> the NOTE indicates the kernel supports a relocatable entry path.
>>
>> Change the loading to check for an acceptable load address.  If the
>> kernel is relocatable, support finding an alternate load address.
>>
>> The primary motivation for an explicit align field is that Linux has a
>> configurable CONFIG_PHYSICAL_ALIGN field.  This value is present in the
>> bzImage setup header, but not the ELF program headers p_align, which
>> report 2MB even when CONFIG_PHYSICAL_ALIGN is greater.  Since a kernel
>> is only considered relocatable if the PHYS32_RELOC elf note is present,
>> the alignment contraints can just be specified within the note instead
>> of searching for an alignment value via a heuristic.
>>
>> Load alignment uses the PHYS32_RELOC note value if specified.
>> Otherwise, the maxmum ELF PHDR p_align value is selected if greater than
>> or equal to PAGE_SIZE.  Finally, the fallback default is 2MB.
>>
>> libelf-private.h includes common-macros.h to satisfy the fuzzer build.
>>
>> Link: https://gitlab.com/xen-project/xen/-/issues/180
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> yet still with two remarks:

Thanks.

>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -537,6 +537,111 @@ static paddr_t __init find_memory(
>>       return INVALID_PADDR;
>>   }
>>   
>> +static bool __init check_load_address(
>> +    const struct domain *d, const struct elf_binary *elf)
>> +{
>> +    paddr_t kernel_start = (uintptr_t)elf->dest_base;
>> +    paddr_t kernel_end = kernel_start + elf->dest_size;
>> +    unsigned int i;
> 
> While properly typed here, ...
> 
>> +static paddr_t __init find_kernel_memory(
>> +    const struct domain *d, struct elf_binary *elf,
>> +    const struct elf_dom_parms *parms)
>> +{
>> +    paddr_t kernel_size = elf->dest_size;
>> +    unsigned int align;
>> +    int i;
> 
> ... I must have missed when this was changed to plain int. It should have
> been unsigned int here, too, ...
> 
>> +    if ( parms->phys_align != UNSET_ADDR32 )
>> +        align = parms->phys_align;
>> +    else if ( elf->palign >= PAGE_SIZE )
>> +        align = elf->palign;
>> +    else
>> +        align = MB(2);
>> +
>> +    /* Search backwards to find the highest address. */
>> +    for ( i = d->arch.nr_e820 - 1; i >= 0 ; i-- )
> 
> ... with this suitably adjusted. However, I'm not going to change this while
> committing, to avoid screwing up.

I intentionally changed this.  Looping downwards, a signed int allows 
writing the check naturally with i >= 0.  I think it's clearer when 
written this way.

>> --- a/xen/include/public/elfnote.h
>> +++ b/xen/include/public/elfnote.h
>> @@ -196,10 +196,28 @@
>>    */
>>   #define XEN_ELFNOTE_PHYS32_ENTRY 18
>>   
>> +/*
>> + * Physical loading constraints for PVH kernels
>> + *
>> + * The presence of this note indicates the kernel supports relocating itself.
>> + *
>> + * The note may include up to three 32bit values to place constraints on the
>> + * guest physical loading addresses and alignment for a PVH kernel.  Values
>> + * are read in the following order:
>> + *  - a required start alignment (default 0x200000)
> 
> "... (default 0x200000; see below)", i.e. ...
> 
>> + *  - a minimum address for the start of the image (default 0)
>> + *  - a maximum address for the last byte of the image (default 0xffffffff)
>> + *
>> + *  When this note specifies an alignment value, it is used.  Otherwise the
>> + *  maximum p_align value from loadable ELF Program Headers is used, if it is
>> + *  greater than or equal to 4k (0x1000).  Otherwise, the default is used.
> 
> ... referring to this (which, btw, has one too many padding blanks on the
> entire paragraph). I will take the liberty to make this adjustment while
> committing.

Thanks.  vim must have kept the indent from the list above, and I didn't 
notice.

Regards,
Jason

Re: [PATCH v7 2/2] x86/PVH: Support relocatable dom0 kernels

[Jan Beulich]

On 08.04.2024 18:56, Jason Andryuk wrote:
> On 2024-04-08 03:00, Jan Beulich wrote:
>> On 04.04.2024 23:25, Jason Andryuk wrote:
>>> --- a/xen/arch/x86/hvm/dom0_build.c
>>> +++ b/xen/arch/x86/hvm/dom0_build.c
>>> @@ -537,6 +537,111 @@ static paddr_t __init find_memory(
>>>       return INVALID_PADDR;
>>>   }
>>>   
>>> +static bool __init check_load_address(
>>> +    const struct domain *d, const struct elf_binary *elf)
>>> +{
>>> +    paddr_t kernel_start = (uintptr_t)elf->dest_base;
>>> +    paddr_t kernel_end = kernel_start + elf->dest_size;
>>> +    unsigned int i;
>>
>> While properly typed here, ...
>>
>>> +static paddr_t __init find_kernel_memory(
>>> +    const struct domain *d, struct elf_binary *elf,
>>> +    const struct elf_dom_parms *parms)
>>> +{
>>> +    paddr_t kernel_size = elf->dest_size;
>>> +    unsigned int align;
>>> +    int i;
>>
>> ... I must have missed when this was changed to plain int. It should have
>> been unsigned int here, too, ...
>>
>>> +    if ( parms->phys_align != UNSET_ADDR32 )
>>> +        align = parms->phys_align;
>>> +    else if ( elf->palign >= PAGE_SIZE )
>>> +        align = elf->palign;
>>> +    else
>>> +        align = MB(2);
>>> +
>>> +    /* Search backwards to find the highest address. */
>>> +    for ( i = d->arch.nr_e820 - 1; i >= 0 ; i-- )
>>
>> ... with this suitably adjusted. However, I'm not going to change this while
>> committing, to avoid screwing up.
> 
> I intentionally changed this.  Looping downwards, a signed int allows 
> writing the check naturally with i >= 0.  I think it's clearer when 
> written this way.

Just to clarify: Is

    for ( i = d->arch.nr_e820; i--; )

any less clear? (While replying I also notice there's a stray blank
in the for() you have, ahead of the 2nd semicolon.)

Jan

Re: [PATCH v7 2/2] x86/PVH: Support relocatable dom0 kernels

[Jason Andryuk]

On 2024-04-17 09:24, Jan Beulich wrote:
> On 08.04.2024 18:56, Jason Andryuk wrote:
>> On 2024-04-08 03:00, Jan Beulich wrote:
>>> On 04.04.2024 23:25, Jason Andryuk wrote:
>>>> --- a/xen/arch/x86/hvm/dom0_build.c
>>>> +++ b/xen/arch/x86/hvm/dom0_build.c
>>>> @@ -537,6 +537,111 @@ static paddr_t __init find_memory(
>>>>        return INVALID_PADDR;
>>>>    }
>>>>    
>>>> +static bool __init check_load_address(
>>>> +    const struct domain *d, const struct elf_binary *elf)
>>>> +{
>>>> +    paddr_t kernel_start = (uintptr_t)elf->dest_base;
>>>> +    paddr_t kernel_end = kernel_start + elf->dest_size;
>>>> +    unsigned int i;
>>>
>>> While properly typed here, ...
>>>
>>>> +static paddr_t __init find_kernel_memory(
>>>> +    const struct domain *d, struct elf_binary *elf,
>>>> +    const struct elf_dom_parms *parms)
>>>> +{
>>>> +    paddr_t kernel_size = elf->dest_size;
>>>> +    unsigned int align;
>>>> +    int i;
>>>
>>> ... I must have missed when this was changed to plain int. It should have
>>> been unsigned int here, too, ...
>>>
>>>> +    if ( parms->phys_align != UNSET_ADDR32 )
>>>> +        align = parms->phys_align;
>>>> +    else if ( elf->palign >= PAGE_SIZE )
>>>> +        align = elf->palign;
>>>> +    else
>>>> +        align = MB(2);
>>>> +
>>>> +    /* Search backwards to find the highest address. */
>>>> +    for ( i = d->arch.nr_e820 - 1; i >= 0 ; i-- )
>>>
>>> ... with this suitably adjusted. However, I'm not going to change this while
>>> committing, to avoid screwing up.
>>
>> I intentionally changed this.  Looping downwards, a signed int allows
>> writing the check naturally with i >= 0.  I think it's clearer when
>> written this way.
> 
> Just to clarify: Is
> 
>      for ( i = d->arch.nr_e820; i--; )
> 
> any less clear?

It's not something I normally write, so I had to think about it more. If 
you are already familiar with such a construct, then that isn't an issue 
for you.

Your way is more subtle in my opinion because it relies on the post 
decrement to ensure correct bounds within the loop body.  I prefer i >= 
0 because it clearly states the valid index values.

Is your main concern that you only want unsigned values as array indices?

> (While replying I also notice there's a stray blank
> in the for() you have, ahead of the 2nd semicolon.)

Sorry about that.

Regards,
Jason

Re: [PATCH v7 2/2] x86/PVH: Support relocatable dom0 kernels

[Jan Beulich]

On 18.04.2024 16:34, Jason Andryuk wrote:
> On 2024-04-17 09:24, Jan Beulich wrote:
>> On 08.04.2024 18:56, Jason Andryuk wrote:
>>> On 2024-04-08 03:00, Jan Beulich wrote:
>>>> On 04.04.2024 23:25, Jason Andryuk wrote:
>>>>> --- a/xen/arch/x86/hvm/dom0_build.c
>>>>> +++ b/xen/arch/x86/hvm/dom0_build.c
>>>>> @@ -537,6 +537,111 @@ static paddr_t __init find_memory(
>>>>>        return INVALID_PADDR;
>>>>>    }
>>>>>    
>>>>> +static bool __init check_load_address(
>>>>> +    const struct domain *d, const struct elf_binary *elf)
>>>>> +{
>>>>> +    paddr_t kernel_start = (uintptr_t)elf->dest_base;
>>>>> +    paddr_t kernel_end = kernel_start + elf->dest_size;
>>>>> +    unsigned int i;
>>>>
>>>> While properly typed here, ...
>>>>
>>>>> +static paddr_t __init find_kernel_memory(
>>>>> +    const struct domain *d, struct elf_binary *elf,
>>>>> +    const struct elf_dom_parms *parms)
>>>>> +{
>>>>> +    paddr_t kernel_size = elf->dest_size;
>>>>> +    unsigned int align;
>>>>> +    int i;
>>>>
>>>> ... I must have missed when this was changed to plain int. It should have
>>>> been unsigned int here, too, ...
>>>>
>>>>> +    if ( parms->phys_align != UNSET_ADDR32 )
>>>>> +        align = parms->phys_align;
>>>>> +    else if ( elf->palign >= PAGE_SIZE )
>>>>> +        align = elf->palign;
>>>>> +    else
>>>>> +        align = MB(2);
>>>>> +
>>>>> +    /* Search backwards to find the highest address. */
>>>>> +    for ( i = d->arch.nr_e820 - 1; i >= 0 ; i-- )
>>>>
>>>> ... with this suitably adjusted. However, I'm not going to change this while
>>>> committing, to avoid screwing up.
>>>
>>> I intentionally changed this.  Looping downwards, a signed int allows
>>> writing the check naturally with i >= 0.  I think it's clearer when
>>> written this way.
>>
>> Just to clarify: Is
>>
>>      for ( i = d->arch.nr_e820; i--; )
>>
>> any less clear?
> 
> It's not something I normally write, so I had to think about it more. If 
> you are already familiar with such a construct, then that isn't an issue 
> for you.
> 
> Your way is more subtle in my opinion because it relies on the post 
> decrement to ensure correct bounds within the loop body.  I prefer i >= 
> 0 because it clearly states the valid index values.
> 
> Is your main concern that you only want unsigned values as array indices?

Yes. Besides eliminating any concerns towards possible underruns, that also
often allows the compiler to produce better code.

Jan