The memory allocator currently calculates alignment in libxl's virtual
address space, rather than guest physical address space. This results
in the FACS being commonly misaligned.

Furthermore, the allocator has several other bugs.

The opencoded align-up calculation is currently susceptible to a bug
that occurs in the corner case that the buffer is already aligned to
begin with. In that case, an align-sized memory hole is introduced.

The while loop is dead logic because its effects are entirely and
unconditionally overwritten immediately after it.

Rework the memory allocator to align in guest physical address space
instead of libxl's virtual memory and improve the calculation, drop
errant extra page in allocated buffer for ACPI tables, and give some
of the variables better names/types.

Fixes: 14c0d328da2b ("libxl/acpi: Build ACPI tables for HVMlite guests")
Signed-off-by: Kevin Stefanov <kevin.stefanov@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Jackson <iwj@xenproject.org>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>

v2: Rewrite completely, to align in guest physical address space
---
 tools/libs/light/libxl_x86_acpi.c | 47 +++++++++++++------------------
 1 file changed, 19 insertions(+), 28 deletions(-)

diff --git a/tools/libs/light/libxl_x86_acpi.c b/tools/libs/light/libxl_x86_acpi.c
index 3eca1c7a9f..8b6dee2e05 100644
--- a/tools/libs/light/libxl_x86_acpi.c
+++ b/tools/libs/light/libxl_x86_acpi.c
@@ -20,6 +20,7 @@
 
  /* Number of pages holding ACPI tables */
 #define NUM_ACPI_PAGES 16
+#define ALIGN(p, a) (((p) + ((a) - 1)) & ~((a) - 1))
 
 struct libxl_acpi_ctxt {
     struct acpi_ctxt c;
@@ -28,10 +29,10 @@ struct libxl_acpi_ctxt {
     unsigned int page_shift;
 
     /* Memory allocator */
-    unsigned long alloc_base_paddr;
-    unsigned long alloc_base_vaddr;
-    unsigned long alloc_currp;
-    unsigned long alloc_end;
+    unsigned long guest_start;
+    unsigned long guest_curr;
+    unsigned long guest_end;
+    void *buf;
 };
 
 extern const unsigned char dsdt_pvh[];
@@ -43,8 +44,7 @@ static unsigned long virt_to_phys(struct acpi_ctxt *ctxt, void *v)
     struct libxl_acpi_ctxt *libxl_ctxt =
         CONTAINER_OF(ctxt, struct libxl_acpi_ctxt, c);
 
-    return (((unsigned long)v - libxl_ctxt->alloc_base_vaddr) +
-            libxl_ctxt->alloc_base_paddr);
+    return libxl_ctxt->guest_start + (v - libxl_ctxt->buf);
 }
 
 static void *mem_alloc(struct acpi_ctxt *ctxt,
@@ -58,20 +58,16 @@ static void *mem_alloc(struct acpi_ctxt *ctxt,
     if (align < 16)
         align = 16;
 
-    s = (libxl_ctxt->alloc_currp + align) & ~((unsigned long)align - 1);
+    s = ALIGN(libxl_ctxt->guest_curr, align);
     e = s + size - 1;
 
     /* TODO: Reallocate memory */
-    if ((e < s) || (e >= libxl_ctxt->alloc_end))
+    if ((e < s) || (e >= libxl_ctxt->guest_end))
         return NULL;
 
-    while (libxl_ctxt->alloc_currp >> libxl_ctxt->page_shift != 
-           e >> libxl_ctxt->page_shift)
-        libxl_ctxt->alloc_currp += libxl_ctxt->page_size;
+    libxl_ctxt->guest_curr = e;
 
-    libxl_ctxt->alloc_currp = e;
-
-    return (void *)s;
+    return libxl_ctxt->buf + (s - libxl_ctxt->guest_start);
 }
 
 static void acpi_mem_free(struct acpi_ctxt *ctxt,
@@ -163,7 +159,6 @@ int libxl__dom_load_acpi(libxl__gc *gc,
     struct acpi_config config = {0};
     struct libxl_acpi_ctxt libxl_ctxt;
     int rc = 0, acpi_pages_num;
-    void *acpi_pages;
     unsigned long page_mask;
 
     if (b_info->type != LIBXL_DOMAIN_TYPE_PVH)
@@ -186,19 +181,17 @@ int libxl__dom_load_acpi(libxl__gc *gc,
     config.rsdp = (unsigned long)libxl__malloc(gc, libxl_ctxt.page_size);
     config.infop = (unsigned long)libxl__malloc(gc, libxl_ctxt.page_size);
     /* Pages to hold ACPI tables */
-    acpi_pages =  libxl__malloc(gc, (NUM_ACPI_PAGES + 1) *
-                                libxl_ctxt.page_size);
+    libxl_ctxt.buf = libxl__malloc(gc, NUM_ACPI_PAGES *
+                                   libxl_ctxt.page_size);
 
     /*
      * Set up allocator memory.
      * Start next to acpi_info page to avoid fracturing e820.
      */
-    libxl_ctxt.alloc_base_paddr = ACPI_INFO_PHYSICAL_ADDRESS +
-        libxl_ctxt.page_size;
-    libxl_ctxt.alloc_base_vaddr = libxl_ctxt.alloc_currp =
-        (unsigned long)acpi_pages;
-    libxl_ctxt.alloc_end = (unsigned long)acpi_pages +
-        (NUM_ACPI_PAGES * libxl_ctxt.page_size);
+    libxl_ctxt.guest_start = libxl_ctxt.guest_curr = libxl_ctxt.guest_end =
+        ACPI_INFO_PHYSICAL_ADDRESS + libxl_ctxt.page_size;
+
+    libxl_ctxt.guest_end += NUM_ACPI_PAGES * libxl_ctxt.page_size;
 
     /* Build the tables. */
     rc = acpi_build_tables(&libxl_ctxt.c, &config);
@@ -208,10 +201,8 @@ int libxl__dom_load_acpi(libxl__gc *gc,
     }
 
     /* Calculate how many pages are needed for the tables. */
-    acpi_pages_num =
-        ((libxl_ctxt.alloc_currp - (unsigned long)acpi_pages)
-         >> libxl_ctxt.page_shift) +
-        ((libxl_ctxt.alloc_currp & page_mask) ? 1 : 0);
+    acpi_pages_num = (ALIGN(libxl_ctxt.guest_curr, libxl_ctxt.page_size) -
+                      libxl_ctxt.guest_start) >> libxl_ctxt.page_shift;
 
     dom->acpi_modules[0].data = (void *)config.rsdp;
     dom->acpi_modules[0].length = 64;
@@ -231,7 +222,7 @@ int libxl__dom_load_acpi(libxl__gc *gc,
     dom->acpi_modules[1].length = 4096;
     dom->acpi_modules[1].guest_addr_out = ACPI_INFO_PHYSICAL_ADDRESS;
 
-    dom->acpi_modules[2].data = acpi_pages;
+    dom->acpi_modules[2].data = libxl_ctxt.buf;
     dom->acpi_modules[2].length = acpi_pages_num  << libxl_ctxt.page_shift;
     dom->acpi_modules[2].guest_addr_out = ACPI_INFO_PHYSICAL_ADDRESS +
         libxl_ctxt.page_size;
-- 
2.25.1

On 14/09/2021 17:43, Kevin Stefanov wrote:
> The memory allocator currently calculates alignment in libxl's virtual
> address space, rather than guest physical address space. This results
> in the FACS being commonly misaligned.
>
> Furthermore, the allocator has several other bugs.
>
> The opencoded align-up calculation is currently susceptible to a bug
> that occurs in the corner case that the buffer is already aligned to
> begin with. In that case, an align-sized memory hole is introduced.
>
> The while loop is dead logic because its effects are entirely and
> unconditionally overwritten immediately after it.
>
> Rework the memory allocator to align in guest physical address space
> instead of libxl's virtual memory and improve the calculation, drop
> errant extra page in allocated buffer for ACPI tables, and give some
> of the variables better names/types.
>
> Fixes: 14c0d328da2b ("libxl/acpi: Build ACPI tables for HVMlite guests")
> Signed-off-by: Kevin Stefanov <kevin.stefanov@citrix.com>
> ---
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Jackson <iwj@xenproject.org>
> CC: Wei Liu <wl@xen.org>
> CC: Anthony PERARD <anthony.perard@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
>
> v2: Rewrite completely, to align in guest physical address space

It appears that patchew isn't running properly right now, but ...

> diff --git a/tools/libs/light/libxl_x86_acpi.c b/tools/libs/light/libxl_x86_acpi.c
> index 3eca1c7a9f..8b6dee2e05 100644
> --- a/tools/libs/light/libxl_x86_acpi.c
> +++ b/tools/libs/light/libxl_x86_acpi.c
> @@ -208,10 +201,8 @@ int libxl__dom_load_acpi(libxl__gc *gc,
>      }
>  
>      /* Calculate how many pages are needed for the tables. */
> -    acpi_pages_num =
> -        ((libxl_ctxt.alloc_currp - (unsigned long)acpi_pages)
> -         >> libxl_ctxt.page_shift) +
> -        ((libxl_ctxt.alloc_currp & page_mask) ? 1 : 0);
> +    acpi_pages_num = (ALIGN(libxl_ctxt.guest_curr, libxl_ctxt.page_size) -
> +                      libxl_ctxt.guest_start) >> libxl_ctxt.page_shift;


... this hunk means page_mask lost its only user, and is now a
written-but-not-used variable, which some compilers will warn about.

The fix is easy, and just to drop that local variable too, which is
another 2 lines deleted in the resulting patch.

~Andrew

On 14.09.2021 18:43, Kevin Stefanov wrote:
> --- a/tools/libs/light/libxl_x86_acpi.c
> +++ b/tools/libs/light/libxl_x86_acpi.c
> @@ -20,6 +20,7 @@
>  
>   /* Number of pages holding ACPI tables */
>  #define NUM_ACPI_PAGES 16
> +#define ALIGN(p, a) (((p) + ((a) - 1)) & ~((a) - 1))

Wouldn't this better live in xen-tools/libs.h?

> @@ -163,7 +159,6 @@ int libxl__dom_load_acpi(libxl__gc *gc,
>      struct acpi_config config = {0};
>      struct libxl_acpi_ctxt libxl_ctxt;
>      int rc = 0, acpi_pages_num;
> -    void *acpi_pages;
>      unsigned long page_mask;

With, as pointed out by Andrew, this now dead variable also removed
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

On 15/09/2021 10:30, Jan Beulich wrote:
> On 14.09.2021 18:43, Kevin Stefanov wrote:
>> --- a/tools/libs/light/libxl_x86_acpi.c
>> +++ b/tools/libs/light/libxl_x86_acpi.c
>> @@ -20,6 +20,7 @@
>>  
>>   /* Number of pages holding ACPI tables */
>>  #define NUM_ACPI_PAGES 16
>> +#define ALIGN(p, a) (((p) + ((a) - 1)) & ~((a) - 1))
> Wouldn't this better live in xen-tools/libs.h?

In this case, not really.  That file doesn't exist on all versions this
bugfix needs backporting to, and there is an unknown chance of collision
in older trees.

This would be a whole lot easier if ROUNDUP() wasn't in a dubious state...

~Andrew