Re: [PATCH v2] tools/libxl: Correctly align the ACPI tables

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Ian Jackson <iwj@xxxxxxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
>
> 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