[PATCH v4 0/2] tools/libacpi: fix boot flags passed to PVHv2 guests

[PATCH v4 0/2] tools/libacpi: fix boot flags passed to PVHv2 guests

[Roger Pau Monne]

Hello,

Those are the remaining two patches in the series, the first one adds supports 
for FADT v5 to libacpi, and the second one introduces the usage of the FADT no 
RTC boot flag for PVHv2 guests. Note that ACPI tables for HVM guests should 
remain unmodified.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v4 1/2] tools/libacpi: update FADT layout to support version 5

[Roger Pau Monne]

Update the structure of the FADT table to version 5, and use that version for
PVHv2 guests. Note that HVM guests will continue to use FADT 4. In order to do
this, add a new field to acpi_config that contains the ACPI revision to use by
libacpi. Note that currently this only applies to the FADT.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: boris.ostrovsky@oracle.com
Cc: konrad.wilk@oracle.com
---
Changes since v3:
 - Add a new field to acpi_config that contains the ACPI revision to use.
 - Rename acpi_50_fadt acpi_fadt.
 - Make fadt_size unsigned int.
 - Don't pre-initialize the length and revision fields of the Fadt static table.
 - Rebase on top of staging.
---
 tools/firmware/hvmloader/util.c |  1 +
 tools/libacpi/acpi2_0.h         |  8 +++++---
 tools/libacpi/build.c           | 35 ++++++++++++++++++++++++++++-------
 tools/libacpi/libacpi.h         |  1 +
 tools/libacpi/static_tables.c   |  6 ++----
 tools/libxl/libxl_x86_acpi.c    |  1 +
 6 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index c05de53..f2bec51 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -952,6 +952,7 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
                             ACPI_HAS_WAET | ACPI_HAS_PMTIMER |
                             ACPI_HAS_BUTTONS | ACPI_HAS_VGA |
                             ACPI_HAS_8042);
+    config->acpi_revision = 4;
 
     config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS;
 
diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h
index 500f95e..5715197 100644
--- a/tools/libacpi/acpi2_0.h
+++ b/tools/libacpi/acpi2_0.h
@@ -169,7 +169,7 @@ struct acpi_10_fadt {
 /*
  * Fixed ACPI Description Table Structure (FADT).
  */
-struct acpi_20_fadt {
+struct acpi_fadt {
     struct acpi_header header;
     uint32_t firmware_ctrl;
     uint32_t dsdt;
@@ -222,6 +222,9 @@ struct acpi_20_fadt {
     struct acpi_20_generic_address x_pm_tmr_blk;
     struct acpi_20_generic_address x_gpe0_blk;
     struct acpi_20_generic_address x_gpe1_blk;
+    /* Only available starting from FADT revision 5. */
+    struct acpi_20_generic_address sleep_control;
+    struct acpi_20_generic_address sleep_status;
 };
 
 /*
@@ -422,7 +425,7 @@ struct acpi_20_slit {
  */
 #define ACPI_2_0_RSDP_SIGNATURE ASCII64('R','S','D',' ','P','T','R',' ')
 #define ACPI_2_0_FACS_SIGNATURE ASCII32('F','A','C','S')
-#define ACPI_2_0_FADT_SIGNATURE ASCII32('F','A','C','P')
+#define ACPI_FADT_SIGNATURE     ASCII32('F','A','C','P')
 #define ACPI_2_0_MADT_SIGNATURE ASCII32('A','P','I','C')
 #define ACPI_2_0_RSDT_SIGNATURE ASCII32('R','S','D','T')
 #define ACPI_2_0_XSDT_SIGNATURE ASCII32('X','S','D','T')
@@ -436,7 +439,6 @@ struct acpi_20_slit {
  * Table revision numbers.
  */
 #define ACPI_2_0_RSDP_REVISION 0x02
-#define ACPI_2_0_FADT_REVISION 0x04
 #define ACPI_2_0_MADT_REVISION 0x02
 #define ACPI_2_0_RSDT_REVISION 0x01
 #define ACPI_2_0_XSDT_REVISION 0x01
diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
index 2bdfaab..6bd50b3 100644
--- a/tools/libacpi/build.c
+++ b/tools/libacpi/build.c
@@ -33,7 +33,7 @@
 extern struct acpi_20_rsdp Rsdp;
 extern struct acpi_20_rsdt Rsdt;
 extern struct acpi_20_xsdt Xsdt;
-extern struct acpi_20_fadt Fadt;
+extern struct acpi_fadt Fadt;
 extern struct acpi_20_facs Facs;
 extern struct acpi_20_waet Waet;
 
@@ -503,12 +503,13 @@ int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config)
     struct acpi_20_rsdp *rsdp;
     struct acpi_20_rsdt *rsdt;
     struct acpi_20_xsdt *xsdt;
-    struct acpi_20_fadt *fadt;
+    struct acpi_fadt    *fadt;
     struct acpi_10_fadt *fadt_10;
     struct acpi_20_facs *facs;
     unsigned char       *dsdt;
     unsigned long        secondary_tables[ACPI_MAX_SECONDARY_TABLES];
     int                  nr_secondaries, i;
+    unsigned int         fadt_size;
 
     acpi_info = (struct acpi_info *)config->infop;
     memset(acpi_info, 0, sizeof(*acpi_info));
@@ -572,7 +573,23 @@ int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config)
                  offsetof(struct acpi_header, checksum),
                  sizeof(struct acpi_10_fadt));
 
-    fadt = ctxt->mem_ops.alloc(ctxt, sizeof(struct acpi_20_fadt), 16);
+    switch ( config->acpi_revision )
+    {
+    case 4:
+        /*
+         * NB: we can use offsetof because there's no padding between
+         * x_gpe1_blk and sleep_control.
+         */
+        fadt_size = offsetof(struct acpi_fadt, sleep_control);
+        break;
+    case 5:
+        fadt_size = sizeof(*fadt);
+        break;
+    default:
+        printf("ACPI revision %u not supported\n", config->acpi_revision);
+        return -1;
+    }
+    fadt = ctxt->mem_ops.alloc(ctxt, fadt_size, 16);
     if (!fadt) goto oom;
     if ( !(config->table_flags & ACPI_HAS_PMTIMER) )
     {
@@ -581,7 +598,13 @@ int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config)
     }
     if ( !(config->table_flags & ACPI_HAS_BUTTONS) )
         Fadt.flags |= (ACPI_PWR_BUTTON | ACPI_SLP_BUTTON);
-    memcpy(fadt, &Fadt, sizeof(struct acpi_20_fadt));
+    memcpy(fadt, &Fadt, fadt_size);
+    /*
+     * For both ACPI 4 and 5 the revision of the FADT matches the ACPI
+     * revision.
+     */
+    fadt->header.revision = config->acpi_revision;
+    fadt->header.length = fadt_size;
     fadt->dsdt   = ctxt->mem_ops.v2p(ctxt, dsdt);
     fadt->x_dsdt = ctxt->mem_ops.v2p(ctxt, dsdt);
     fadt->firmware_ctrl   = ctxt->mem_ops.v2p(ctxt, facs);
@@ -590,9 +613,7 @@ int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config)
         fadt->iapc_boot_arch |= ACPI_FADT_NO_VGA;
     if ( config->table_flags & ACPI_HAS_8042 )
         fadt->iapc_boot_arch |= ACPI_FADT_8042;
-    set_checksum(fadt,
-                 offsetof(struct acpi_header, checksum),
-                 sizeof(struct acpi_20_fadt));
+    set_checksum(fadt, offsetof(struct acpi_header, checksum), fadt_size);
 
     nr_secondaries = construct_secondary_tables(ctxt, secondary_tables,
                  config, acpi_info);
diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
index db2d7b2..dbc6c8b 100644
--- a/tools/libacpi/libacpi.h
+++ b/tools/libacpi/libacpi.h
@@ -63,6 +63,7 @@ struct acpi_config {
     uint64_t pci_hi_start, pci_hi_len;
 
     uint32_t table_flags;
+    uint8_t acpi_revision;
 
     uint64_t vm_gid[2];
     unsigned long vm_gid_addr; /* OUT parameter */
diff --git a/tools/libacpi/static_tables.c b/tools/libacpi/static_tables.c
index 1f6247d..13946aa 100644
--- a/tools/libacpi/static_tables.c
+++ b/tools/libacpi/static_tables.c
@@ -38,11 +38,9 @@ struct acpi_20_facs Facs = {
 #define ACPI_PM_TMR_BLK_BIT_WIDTH           0x20
 #define ACPI_PM_TMR_BLK_BIT_OFFSET          0x00
 
-struct acpi_20_fadt Fadt = {
+struct acpi_fadt Fadt = {
     .header = {
-        .signature    = ACPI_2_0_FADT_SIGNATURE,
-        .length       = sizeof(struct acpi_20_fadt),
-        .revision     = ACPI_2_0_FADT_REVISION,
+        .signature    = ACPI_FADT_SIGNATURE,
         .oem_id       = ACPI_OEM_ID, 
         .oem_table_id = ACPI_OEM_TABLE_ID,
         .oem_revision = ACPI_OEM_REVISION,
diff --git a/tools/libxl/libxl_x86_acpi.c b/tools/libxl/libxl_x86_acpi.c
index ff0e2df..686ac8e 100644
--- a/tools/libxl/libxl_x86_acpi.c
+++ b/tools/libxl/libxl_x86_acpi.c
@@ -151,6 +151,7 @@ static int init_acpi_config(libxl__gc *gc,
 
     config->lapic_base_address = LAPIC_BASE_ADDRESS;
     config->lapic_id = acpi_lapic_id;
+    config->acpi_revision = 5;
 
 out:
     return 0;
-- 
2.9.3 (Apple Git-75)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v4 2/2] tools/libacpi: announce that PVHv2 has no CMOS RTC in FADT

[Roger Pau Monne]

At the moment this flag is unconditionally set for PVHv2 domains. Note that
using this boot flag requires that the FADT table revision is at least 5 (or any
later version), so bump the current FADT version from 4 to 5 and add two new
fields that will be unused.

Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: boris.ostrovsky@oracle.com
Cc: konrad.wilk@oracle.com
---
Changes since v3:
 - Rebase on top of previous patch.
---
Ideally this should be somehow fetched from the emulation_flags set of flags,
but sadly there's no way for hvmloader (which runs in guest context) to fetch
this information. If at some point a rtc option is added to libxl, we should see
about passing it through to init_acpi_config in libxl_x86_acpi.c so that we can
then appropriately set this flag for PVHv2 guests.
---
 tools/firmware/hvmloader/util.c | 2 +-
 tools/libacpi/acpi2_0.h         | 1 +
 tools/libacpi/build.c           | 9 +++++++++
 tools/libacpi/libacpi.h         | 1 +
 4 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index f2bec51..03cfb79 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -951,7 +951,7 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
     config->table_flags |= (ACPI_HAS_TCPA | ACPI_HAS_IOAPIC |
                             ACPI_HAS_WAET | ACPI_HAS_PMTIMER |
                             ACPI_HAS_BUTTONS | ACPI_HAS_VGA |
-                            ACPI_HAS_8042);
+                            ACPI_HAS_8042 | ACPI_HAS_CMOS_RTC);
     config->acpi_revision = 4;
 
     config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS;
diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h
index 5715197..2619ba3 100644
--- a/tools/libacpi/acpi2_0.h
+++ b/tools/libacpi/acpi2_0.h
@@ -233,6 +233,7 @@ struct acpi_fadt {
 #define ACPI_FADT_LEGACY_DEVICES    (1 << 0)
 #define ACPI_FADT_8042              (1 << 1)
 #define ACPI_FADT_NO_VGA            (1 << 2)
+#define ACPI_FADT_NO_CMOS_RTC       (1 << 5)
 
 /*
  * FADT Fixed Feature Flags.
diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
index 6bd50b3..a02ffbf 100644
--- a/tools/libacpi/build.c
+++ b/tools/libacpi/build.c
@@ -613,6 +613,15 @@ int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config)
         fadt->iapc_boot_arch |= ACPI_FADT_NO_VGA;
     if ( config->table_flags & ACPI_HAS_8042 )
         fadt->iapc_boot_arch |= ACPI_FADT_8042;
+    if ( !(config->table_flags & ACPI_HAS_CMOS_RTC) )
+    {
+        if ( fadt->header.revision < 5 )
+        {
+            printf("ACPI_FADT_NO_CMOS_RTC requires FADT revision 5\n");
+            return -1;
+        }
+        fadt->iapc_boot_arch |= ACPI_FADT_NO_CMOS_RTC;
+    }
     set_checksum(fadt, offsetof(struct acpi_header, checksum), fadt_size);
 
     nr_secondaries = construct_secondary_tables(ctxt, secondary_tables,
diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
index dbc6c8b..67bd67f 100644
--- a/tools/libacpi/libacpi.h
+++ b/tools/libacpi/libacpi.h
@@ -34,6 +34,7 @@
 #define ACPI_HAS_BUTTONS     (1<<11)
 #define ACPI_HAS_VGA         (1<<12)
 #define ACPI_HAS_8042        (1<<13)
+#define ACPI_HAS_CMOS_RTC    (1<<14)
 
 struct xen_vmemrange;
 struct acpi_numa {
-- 
2.9.3 (Apple Git-75)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 1/2] tools/libacpi: update FADT layout to support version 5

[Wei Liu]

On Mon, Dec 12, 2016 at 04:04:24PM +0000, Roger Pau Monne wrote:
> Update the structure of the FADT table to version 5, and use that version for
> PVHv2 guests. Note that HVM guests will continue to use FADT 4. In order to do
> this, add a new field to acpi_config that contains the ACPI revision to use by
> libacpi. Note that currently this only applies to the FADT.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: boris.ostrovsky@oracle.com
> Cc: konrad.wilk@oracle.com
> ---
> Changes since v3:
>  - Add a new field to acpi_config that contains the ACPI revision to use.
>  - Rename acpi_50_fadt acpi_fadt.
>  - Make fadt_size unsigned int.
>  - Don't pre-initialize the length and revision fields of the Fadt static table.
>  - Rebase on top of staging.
> ---
>  tools/firmware/hvmloader/util.c |  1 +
>  tools/libacpi/acpi2_0.h         |  8 +++++---
>  tools/libacpi/build.c           | 35 ++++++++++++++++++++++++++++-------
>  tools/libacpi/libacpi.h         |  1 +
>  tools/libacpi/static_tables.c   |  6 ++----
>  tools/libxl/libxl_x86_acpi.c    |  1 +

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 1/2] tools/libacpi: update FADT layout to support version 5

[Jan Beulich]

>>> On 12.12.16 at 17:04, <roger.pau@citrix.com> wrote:
> Update the structure of the FADT table to version 5, and use that version for
> PVHv2 guests. Note that HVM guests will continue to use FADT 4. In order to do
> this, add a new field to acpi_config that contains the ACPI revision to use by
> libacpi. Note that currently this only applies to the FADT.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

To be honest I'm of two minds here. On one hand the patch is fine
as is for the immediate purpose. On the other hand I would have
hoped for a little more complete handling of this:
- The spec is at version 6.1 - I don't see why we should give anything
  less to PVHv2 guests, causing possible compatibility issues going
  forward.
- The version consists of both a major and a minor part, yet no
  provisions are being made to allow for this.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 2/2] tools/libacpi: announce that PVHv2 has no CMOS RTC in FADT

[Jan Beulich]

>>> On 12.12.16 at 17:04, <roger.pau@citrix.com> wrote:
> At the moment this flag is unconditionally set for PVHv2 domains. Note that
> using this boot flag requires that the FADT table revision is at least 5 (or any
> later version), so bump the current FADT version from 4 to 5 and add two new
> fields that will be unused.

That last part is now stale, but could easily be dropped while
committing (pending resolution of my uncertainty on patch 1).

> Reported-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 1/2] tools/libacpi: update FADT layout to support version 5

[Roger Pau Monne]

On Tue, Dec 13, 2016 at 02:03:06AM -0700, Jan Beulich wrote:
> >>> On 12.12.16 at 17:04, <roger.pau@citrix.com> wrote:
> > Update the structure of the FADT table to version 5, and use that version for
> > PVHv2 guests. Note that HVM guests will continue to use FADT 4. In order to do
> > this, add a new field to acpi_config that contains the ACPI revision to use by
> > libacpi. Note that currently this only applies to the FADT.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> To be honest I'm of two minds here. On one hand the patch is fine
> as is for the immediate purpose. On the other hand I would have
> hoped for a little more complete handling of this:
> - The spec is at version 6.1 - I don't see why we should give anything
>   less to PVHv2 guests, causing possible compatibility issues going
>   forward.

I'm not opposed to bumping the ACPI version to 6.1, it's just that this needs to 
be properly done IMHO, and I would like to see all tables bumped to the revision 
specified in 6.1. This is the minor bump needed in order to use the no RTC flag, 
so I feel it's safer, and other tables should not get that much off-sync 
compared to FADT.

> - The version consists of both a major and a minor part, yet no
>   provisions are being made to allow for this.

Given the nature of this "fix" (which initially only focused on being able to 
use the no RTC flag) I didn't see the need to introduce the minor version. TBH, 
the FADT field used for the minor version is still marked as "reserved" in our 
current layout. I would prefer that to be done in a proper patch series that 
fixes all this minor issues and properly bumps the version to 6.1.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel