[PATCH 0/3] Fix alignment of FACS in guests

[PATCH 0/3] Fix alignment of FACS in guests

[Kevin Stefanov]

When booting Xen as a PVH guest, it currently complains:

  (XEN) ACPI: SLEEP INFO: pm1x_cnt[1:b004,1:0], pm1x_evt[1:b000,1:0]
  (XEN) ACPI: FACS is not 64-byte aligned: 0xfc001010
  (XEN) ACPI: wakeup_vec[fc00101c], vec_size[20]
  (XEN) ACPI: Local APIC address 0xfee00000

This is caused by several bugs in the toolstack whilst writing ACPI
tables.

Kevin Stefanov (3):
  tools/libacpi: Use 64-byte alignment for FACS
  tools/libxl: Correctly aligned buffer for ACPI tables
  tools/libxl: Only allocate 64 bytes for RSDP

 tools/libacpi/build.c             | 2 +-
 tools/libs/light/libxl_x86_acpi.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH 1/3] tools/libacpi: Use 64-byte alignment for FACS

[Kevin Stefanov]

The spec requires 64-byte alignment, not 16.

Signed-off-by: Kevin Stefanov <kevin.stefanov@citrix.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>

Note: This does not fix the FACS alignment inside guests yet. See next
patch.
---
 tools/libacpi/build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
index a61dd5583a..fe2db66a62 100644
--- a/tools/libacpi/build.c
+++ b/tools/libacpi/build.c
@@ -532,7 +532,7 @@ int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config)
      * Fill in high-memory data structures, starting at @buf.
      */
 
-    facs = ctxt->mem_ops.alloc(ctxt, sizeof(struct acpi_20_facs), 16);
+    facs = ctxt->mem_ops.alloc(ctxt, sizeof(struct acpi_20_facs), 64);
     if (!facs) goto oom;
     memcpy(facs, &Facs, sizeof(struct acpi_20_facs));
 
-- 
2.25.1

[PATCH 2/3] tools/libxl: Correctly aligned buffer for ACPI tables

[Kevin Stefanov]

The pointer resulting from libxl__malloc() has no explicit alignment.
As an implementation detail, it has 16-byte alignment.

When this buffer is used by libacpi aligning ACPI tables to greater
than 16 does not work correctly.  This causes the FACS to not be
64-byte aligned when the ACPI tables are copied into guest memory.

Align the ACPI tables buffer to a page, to match the alignment
inside guest memory. The buffer is already one page too large,
presumably intended for this purpose originally.

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>
---
 tools/libs/light/libxl_x86_acpi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/libs/light/libxl_x86_acpi.c b/tools/libs/light/libxl_x86_acpi.c
index 3eca1c7a9f..0a82e7cacd 100644
--- a/tools/libs/light/libxl_x86_acpi.c
+++ b/tools/libs/light/libxl_x86_acpi.c
@@ -193,6 +193,7 @@ int libxl__dom_load_acpi(libxl__gc *gc,
      * Set up allocator memory.
      * Start next to acpi_info page to avoid fracturing e820.
      */
+    acpi_pages = (void *)ROUNDUP((unsigned long)acpi_pages, libxl_ctxt.page_shift);
     libxl_ctxt.alloc_base_paddr = ACPI_INFO_PHYSICAL_ADDRESS +
         libxl_ctxt.page_size;
     libxl_ctxt.alloc_base_vaddr = libxl_ctxt.alloc_currp =
-- 
2.25.1

[PATCH 3/3] tools/libxl: Only allocate 64 bytes for RSDP

[Kevin Stefanov]

RSDP's size is 64 bytes and later in the function, its buffer is
hardcoded to be 64 bytes long. Don't bother to allocate a whole page.

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>
---
 tools/libs/light/libxl_x86_acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libs/light/libxl_x86_acpi.c b/tools/libs/light/libxl_x86_acpi.c
index 0a82e7cacd..2aea1eca31 100644
--- a/tools/libs/light/libxl_x86_acpi.c
+++ b/tools/libs/light/libxl_x86_acpi.c
@@ -183,7 +183,7 @@ int libxl__dom_load_acpi(libxl__gc *gc,
         goto out;
     }
 
-    config.rsdp = (unsigned long)libxl__malloc(gc, libxl_ctxt.page_size);
+    config.rsdp = (unsigned long)libxl__malloc(gc, 64);
     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) *
-- 
2.25.1

Re: [PATCH 1/3] tools/libacpi: Use 64-byte alignment for FACS

[Andrew Cooper]

On 09/09/2021 17:34, Kevin Stefanov wrote:
> The spec requires 64-byte alignment, not 16.
>
> Signed-off-by: Kevin Stefanov <kevin.stefanov@citrix.com>
> ---
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Note: This does not fix the FACS alignment inside guests yet. See next
> patch.

The history here is complex.

c/s 938cee9d41d3 in 2006 deleted a comment saying

// FACS: should be 64-bit alignment
// so it is put at the start of buffer
// as the buffer is 64 bit alignment

Clearly it means byte rather than bit, but the change also introduced
logic to the effect of buf += ROUNDUP(sizeof(facs), 16).

This (incorrect) alignment survived several morphs of the logic and was
moved from hvmloader into libacpi by c/s 73b72736e6ca.

The current code is clearly wrong, but happens to work correctly in
hvmloader because FACS is the first table written and it starts on a
page boundary.  The logic does not work correctly when libxl passes a
buffer which doesn't start on a page boundary.

As a consequence, I'm not sure what to suggest as a Fixes tag here,
except "the logic has been wrong for 15 years - patch everything".

~Andrew

Re: [PATCH 1/3] tools/libacpi: Use 64-byte alignment for FACS

[Jan Beulich]

On 09.09.2021 18:34, Kevin Stefanov wrote:
> The spec requires 64-byte alignment, not 16.
> 
> Signed-off-by: Kevin Stefanov <kevin.stefanov@citrix.com>

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

Re: [PATCH 2/3] tools/libxl: Correctly aligned buffer for ACPI tables

[Jan Beulich]

On 09.09.2021 18:34, Kevin Stefanov wrote:
> The pointer resulting from libxl__malloc() has no explicit alignment.
> As an implementation detail, it has 16-byte alignment.

But the buffers obtained via libxl__malloc() have no association with
guest address space layout (or at least they aren't supposed to have).
That's solely determined by mem_alloc(). I think it is a bug of
mem_alloc() to determine padding from alloc_currp; it should
rather/also maintain a current address in guest address space (e.g.
by having separate alloc_currp and alloc_currv). Not doing so leaves
us prone to encountering the same issue again down the road. For
example if higher than page alignment was requested from somewhere in
libacpi.

> --- a/tools/libs/light/libxl_x86_acpi.c
> +++ b/tools/libs/light/libxl_x86_acpi.c
> @@ -193,6 +193,7 @@ int libxl__dom_load_acpi(libxl__gc *gc,
>       * Set up allocator memory.
>       * Start next to acpi_info page to avoid fracturing e820.
>       */
> +    acpi_pages = (void *)ROUNDUP((unsigned long)acpi_pages, libxl_ctxt.page_shift);
>      libxl_ctxt.alloc_base_paddr = ACPI_INFO_PHYSICAL_ADDRESS +
>          libxl_ctxt.page_size;
>      libxl_ctxt.alloc_base_vaddr = libxl_ctxt.alloc_currp =

Below from here there's also a suspicious use of 4096. From surrounding
code I would conclude libxl_ctxt.page_size is meant instead. Could you
consider taking care of this as well at this occasion (possibly in yet
another tiny patch), while playing in this area?

Jan

Re: [PATCH 3/3] tools/libxl: Only allocate 64 bytes for RSDP

[Jan Beulich]

On 09.09.2021 18:34, Kevin Stefanov wrote:
> RSDP's size is 64 bytes and later in the function, its buffer is
> hardcoded to be 64 bytes long. Don't bother to allocate a whole page.
> 
> Signed-off-by: Kevin Stefanov <kevin.stefanov@citrix.com>

Purely technically and within the constraints of the present code:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
However, ...

> --- a/tools/libs/light/libxl_x86_acpi.c
> +++ b/tools/libs/light/libxl_x86_acpi.c
> @@ -183,7 +183,7 @@ int libxl__dom_load_acpi(libxl__gc *gc,
>          goto out;
>      }
>  
> -    config.rsdp = (unsigned long)libxl__malloc(gc, libxl_ctxt.page_size);
> +    config.rsdp = (unsigned long)libxl__malloc(gc, 64);

... this is the 4th literal 64 in the function (including one in a
comment), all of which are meant to represent the same abstract thing.
And none of which look to be really correct:
sizeof(struct acpi_20_rsdp) == 36 according to my counting. While I
don't mind using 64 (for the time being), I think this should then be
via a #define, which would be accompanied by a respective comment. Or
else via sizeof(struct acpi_20_rsdp).

But of course hard-coding the size isn't really forward compatible
anyway. It should rather be libacpi to specify what size the blob is.
And then it might be safer to stick to allocating a full page here, as
the actual size won't be known up front. Or the allocated size would
need to become an input to acpi_build_tables(), so that it wouldn't
risk overrunning the space.

Jan

Re: [PATCH 1/3] tools/libacpi: Use 64-byte alignment for FACS [and 1 more messages]

[Ian Jackson]

Andrew Cooper writes ("Re: [PATCH 1/3] tools/libacpi: Use 64-byte alignment for FACS"):
> The current code is clearly wrong, but happens to work correctly in
> hvmloader because FACS is the first table written and it starts on a
> page boundary.  The logic does not work correctly when libxl passes a
> buffer which doesn't start on a page boundary.
> 
> As a consequence, I'm not sure what to suggest as a Fixes tag here,
> except "the logic has been wrong for 15 years - patch everything".

Jan Beulich writes ("Re: [PATCH 2/3] tools/libxl: Correctly aligned buffer for ACPI tables"):
> But the buffers obtained via libxl__malloc() have no association with
> guest address space layout (or at least they aren't supposed to have).
> That's solely determined by mem_alloc(). I think it is a bug of
> mem_alloc() to determine padding from alloc_currp; it should
> rather/also maintain a current address in guest address space (e.g.
> by having separate alloc_currp and alloc_currv). Not doing so leaves
> us prone to encountering the same issue again down the road. For
> example if higher than page alignment was requested from somewhere in
> libacpi.

I think the two of you are saying roughly the same thing here ?

There was a question about how (and if) this should be backported.
I'm afraid I don't quite follow all the implications, but I doubt that
a a substantial overhaul as described would be a good idea for a
backport.  What is the impact for backports, and can we have a more
targeted fix there ?  Are, in fact, Kevin's original patches, suitable
to fix the issue for stable trees ?

Thanks,
Ian.

Re: [PATCH 1/3] tools/libacpi: Use 64-byte alignment for FACS [and 1 more messages]

[Jan Beulich]

On 10.09.2021 16:50, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH 1/3] tools/libacpi: Use 64-byte alignment for FACS"):
>> The current code is clearly wrong, but happens to work correctly in
>> hvmloader because FACS is the first table written and it starts on a
>> page boundary.  The logic does not work correctly when libxl passes a
>> buffer which doesn't start on a page boundary.
>>
>> As a consequence, I'm not sure what to suggest as a Fixes tag here,
>> except "the logic has been wrong for 15 years - patch everything".
> 
> Jan Beulich writes ("Re: [PATCH 2/3] tools/libxl: Correctly aligned buffer for ACPI tables"):
>> But the buffers obtained via libxl__malloc() have no association with
>> guest address space layout (or at least they aren't supposed to have).
>> That's solely determined by mem_alloc(). I think it is a bug of
>> mem_alloc() to determine padding from alloc_currp; it should
>> rather/also maintain a current address in guest address space (e.g.
>> by having separate alloc_currp and alloc_currv). Not doing so leaves
>> us prone to encountering the same issue again down the road. For
>> example if higher than page alignment was requested from somewhere in
>> libacpi.
> 
> I think the two of you are saying roughly the same thing here ?

That's my view of it, indeed.

> There was a question about how (and if) this should be backported.
> I'm afraid I don't quite follow all the implications, but I doubt that
> a a substantial overhaul as described would be a good idea for a
> backport.  What is the impact for backports, and can we have a more
> targeted fix there ?  Are, in fact, Kevin's original patches, suitable
> to fix the issue for stable trees ?

I think they are; the risk with not (also) backporting the more complete
fix to do away with the bad assumptions is that eventual subsequent
backports may then run into the same issue again. Like Andrew suggested,
I think we want to first see how intrusive a "proper" fix is. If it's
really "bad", we can still decide to backport only the original change
(which I'd rather view as a workaround) while trying to make sure future
backports won't end up hitting the underlying problem.

Of course there's also the option to simply declare "future backports
here are unlikely"; I wouldn't really like such, though.

Jan