[PATCH v2 0/4] tools/libacpi: fix boot flags passed to PVHv2 guests

[PATCH v2 0/4] tools/libacpi: fix boot flags passed to PVHv2 guests

[Roger Pau Monne]

Hello,

There are a couple of boot flags that should be passed to PVHv2 guests, that 
report the lack of VGA and CMOS RTC, and we shouldn't also pass the 8042 flag, 
because PVHv2 guests don't have access to such controller. The no CMOS RTC flags 
requires that the FADT table is bumped to version 5 at least, which is done in 
the same patch where the flag is added.

Thanks, Roger.

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

[PATCH v2 1/4] tools/libacpi: add _FADT_ to the FADT boot flags definitions

[Roger Pau Monne]

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
---
 tools/libacpi/acpi2_0.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h
index 775eb7a..5ddef8a 100644
--- a/tools/libacpi/acpi2_0.h
+++ b/tools/libacpi/acpi2_0.h
@@ -227,9 +227,8 @@ struct acpi_20_fadt {
 /*
  * FADT Boot Architecture Flags.
  */
-#define ACPI_LEGACY_DEVICES (1 << 0)
-#define ACPI_8042           (1 << 1)
-
+#define ACPI_FADT_LEGACY_DEVICES    (1 << 0)
+#define ACPI_FADT_8042              (1 << 1)
 /*
  * FADT Fixed Feature Flags.
  */
-- 
2.9.3 (Apple Git-75)


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

[PATCH v2 2/4] tools/libacpi: set FADT boot flag to notify lack of VGA for PVHv2 guests

[Roger Pau Monne]

PVHv2 guests don't have any VGA card, and as so it must be notified in the FADT.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.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
---
 tools/firmware/hvmloader/util.c | 3 ++-
 tools/libacpi/acpi2_0.h         | 2 ++
 tools/libacpi/build.c           | 2 ++
 tools/libacpi/libacpi.h         | 1 +
 4 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 6e0cfe7..3e192bf 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -948,7 +948,8 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
     if ( !strncmp(xenstore_read("platform/acpi_s4", "1"), "1", 1)  )
         config->table_flags |= ACPI_HAS_SSDT_S4;
 
-    config->table_flags |= (ACPI_HAS_TCPA | ACPI_HAS_IOAPIC | ACPI_HAS_WAET);
+    config->table_flags |= (ACPI_HAS_TCPA | ACPI_HAS_IOAPIC | ACPI_HAS_WAET |
+                            ACPI_HAS_VGA);
 
     config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS;
 
diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h
index 5ddef8a..500f95e 100644
--- a/tools/libacpi/acpi2_0.h
+++ b/tools/libacpi/acpi2_0.h
@@ -229,6 +229,8 @@ struct acpi_20_fadt {
  */
 #define ACPI_FADT_LEGACY_DEVICES    (1 << 0)
 #define ACPI_FADT_8042              (1 << 1)
+#define ACPI_FADT_NO_VGA            (1 << 2)
+
 /*
  * FADT Fixed Feature Flags.
  */
diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
index 47dae01..8799e2c 100644
--- a/tools/libacpi/build.c
+++ b/tools/libacpi/build.c
@@ -579,6 +579,8 @@ int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config)
     fadt->x_dsdt = ctxt->mem_ops.v2p(ctxt, dsdt);
     fadt->firmware_ctrl   = ctxt->mem_ops.v2p(ctxt, facs);
     fadt->x_firmware_ctrl = ctxt->mem_ops.v2p(ctxt, facs);
+    if ( !(config->table_flags & ACPI_HAS_VGA) )
+        fadt->iapc_boot_arch |= ACPI_FADT_NO_VGA;
     set_checksum(fadt,
                  offsetof(struct acpi_header, checksum),
                  sizeof(struct acpi_20_fadt));
diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
index 1d388f9..d7ea6e1 100644
--- a/tools/libacpi/libacpi.h
+++ b/tools/libacpi/libacpi.h
@@ -30,6 +30,7 @@
 #define ACPI_HAS_TCPA        (1<<7)
 #define ACPI_HAS_IOAPIC      (1<<8)
 #define ACPI_HAS_WAET        (1<<9)
+#define ACPI_HAS_VGA         (1<<10)
 
 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

[PATCH v2 3/4] tools/libacpi: don't announce a 8042 controller in the FADT for PVHv2 guests

[Roger Pau Monne]

There's no such controler available for PVHv2 guests.

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
---
 tools/firmware/hvmloader/util.c | 2 +-
 tools/libacpi/build.c           | 2 ++
 tools/libacpi/libacpi.h         | 1 +
 tools/libacpi/static_tables.c   | 1 -
 4 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 3e192bf..3f86c88 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -949,7 +949,7 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
         config->table_flags |= ACPI_HAS_SSDT_S4;
 
     config->table_flags |= (ACPI_HAS_TCPA | ACPI_HAS_IOAPIC | ACPI_HAS_WAET |
-                            ACPI_HAS_VGA);
+                            ACPI_HAS_VGA | ACPI_HAS_8042);
 
     config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS;
 
diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
index 8799e2c..844dea1 100644
--- a/tools/libacpi/build.c
+++ b/tools/libacpi/build.c
@@ -581,6 +581,8 @@ int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config)
     fadt->x_firmware_ctrl = ctxt->mem_ops.v2p(ctxt, facs);
     if ( !(config->table_flags & ACPI_HAS_VGA) )
         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));
diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
index d7ea6e1..e0f1537 100644
--- a/tools/libacpi/libacpi.h
+++ b/tools/libacpi/libacpi.h
@@ -31,6 +31,7 @@
 #define ACPI_HAS_IOAPIC      (1<<8)
 #define ACPI_HAS_WAET        (1<<9)
 #define ACPI_HAS_VGA         (1<<10)
+#define ACPI_HAS_8042        (1<<11)
 
 struct xen_vmemrange;
 struct acpi_numa {
diff --git a/tools/libacpi/static_tables.c b/tools/libacpi/static_tables.c
index 617bf68..1f6247d 100644
--- a/tools/libacpi/static_tables.c
+++ b/tools/libacpi/static_tables.c
@@ -63,7 +63,6 @@ struct acpi_20_fadt Fadt = {
 
     .p_lvl2_lat = 0x0fff, /* >100,  means we do not support C2 state */
     .p_lvl3_lat = 0x0fff, /* >1000, means we do not support C3 state */
-    .iapc_boot_arch = ACPI_8042,
     .flags = (ACPI_PROC_C1 |
               ACPI_WBINVD |
               ACPI_FIX_RTC | ACPI_TMR_VAL_EXT |
-- 
2.9.3 (Apple Git-75)


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

[PATCH v2 4/4] 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
---
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         | 5 ++++-
 tools/libacpi/build.c           | 2 ++
 tools/libacpi/libacpi.h         | 1 +
 4 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 3f86c88..cadfbd8 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -949,7 +949,7 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
         config->table_flags |= ACPI_HAS_SSDT_S4;
 
     config->table_flags |= (ACPI_HAS_TCPA | ACPI_HAS_IOAPIC | ACPI_HAS_WAET |
-                            ACPI_HAS_VGA | ACPI_HAS_8042);
+                            ACPI_HAS_VGA | ACPI_HAS_8042 | ACPI_HAS_CMOS_RTC);
 
     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..6db374b 100644
--- a/tools/libacpi/acpi2_0.h
+++ b/tools/libacpi/acpi2_0.h
@@ -222,6 +222,8 @@ 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;
+    struct acpi_20_generic_address sleep_control;
+    struct acpi_20_generic_address sleep_status;
 };
 
 /*
@@ -230,6 +232,7 @@ struct acpi_20_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.
@@ -436,7 +439,7 @@ 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_FADT_REVISION 0x05
 #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 844dea1..10f7cc2 100644
--- a/tools/libacpi/build.c
+++ b/tools/libacpi/build.c
@@ -583,6 +583,8 @@ 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) )
+        fadt->iapc_boot_arch |= ACPI_FADT_NO_CMOS_RTC;
     set_checksum(fadt,
                  offsetof(struct acpi_header, checksum),
                  sizeof(struct acpi_20_fadt));
diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
index e0f1537..5a0ec76 100644
--- a/tools/libacpi/libacpi.h
+++ b/tools/libacpi/libacpi.h
@@ -32,6 +32,7 @@
 #define ACPI_HAS_WAET        (1<<9)
 #define ACPI_HAS_VGA         (1<<10)
 #define ACPI_HAS_8042        (1<<11)
+#define ACPI_HAS_CMOS_RTC    (1<<12)
 
 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 v2 1/4] tools/libacpi: add _FADT_ to the FADT boot flags definitions

[Andrew Cooper]

On 02/12/16 13:48, Roger Pau Monne wrote:
> 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
> ---
>  tools/libacpi/acpi2_0.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h
> index 775eb7a..5ddef8a 100644
> --- a/tools/libacpi/acpi2_0.h
> +++ b/tools/libacpi/acpi2_0.h
> @@ -227,9 +227,8 @@ struct acpi_20_fadt {
>  /*
>   * FADT Boot Architecture Flags.
>   */
> -#define ACPI_LEGACY_DEVICES (1 << 0)
> -#define ACPI_8042           (1 << 1)
> -
> +#define ACPI_FADT_LEGACY_DEVICES    (1 << 0)
> +#define ACPI_FADT_8042              (1 << 1)

It would be better to retain the newline here,  Otherwise, Reviewed-by:
Andrew Cooper <andrew.cooper3@citrix.com>

>  /*
>   * FADT Fixed Feature Flags.
>   */


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

Re: [PATCH v2 2/4] tools/libacpi: set FADT boot flag to notify lack of VGA for PVHv2 guests

[Andrew Cooper]

On 02/12/16 13:48, Roger Pau Monne wrote:
> diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h
> index 5ddef8a..500f95e 100644
> --- a/tools/libacpi/acpi2_0.h
> +++ b/tools/libacpi/acpi2_0.h
> @@ -229,6 +229,8 @@ struct acpi_20_fadt {
>   */
>  #define ACPI_FADT_LEGACY_DEVICES    (1 << 0)
>  #define ACPI_FADT_8042              (1 << 1)
> +#define ACPI_FADT_NO_VGA            (1 << 2)
> +

Ah - this extra newline looks like a rebasing error with patch 1.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2 3/4] tools/libacpi: don't announce a 8042 controller in the FADT for PVHv2 guests

[Andrew Cooper]

On 02/12/16 13:48, Roger Pau Monne wrote:
> There's no such controler available for PVHv2 guests.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2 1/4] tools/libacpi: add _FADT_ to the FADT boot flags definitions

[Roger Pau Monne]

On Fri, Dec 02, 2016 at 01:53:44PM +0000, Andrew Cooper wrote:
> On 02/12/16 13:48, Roger Pau Monne wrote:
> > 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
> > ---
> >  tools/libacpi/acpi2_0.h | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h
> > index 775eb7a..5ddef8a 100644
> > --- a/tools/libacpi/acpi2_0.h
> > +++ b/tools/libacpi/acpi2_0.h
> > @@ -227,9 +227,8 @@ struct acpi_20_fadt {
> >  /*
> >   * FADT Boot Architecture Flags.
> >   */
> > -#define ACPI_LEGACY_DEVICES (1 << 0)
> > -#define ACPI_8042           (1 << 1)
> > -
> > +#define ACPI_FADT_LEGACY_DEVICES    (1 << 0)
> > +#define ACPI_FADT_8042              (1 << 1)
> 
> It would be better to retain the newline here,  Otherwise, Reviewed-by:
> Andrew Cooper <andrew.cooper3@citrix.com>

Right, I somehow managed to screw it. I've pushed a new version with the newline 
issue fixed, it's at:

git://xenbits.xen.org/people/royger/xen.git libacpi_bootflags_v2

Sorry, Roger.

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

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

[Andrew Cooper]

On 02/12/16 13:48, Roger Pau Monne 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.
>
> 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
> ---
> 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.

(I forget if there is a good reason, but) why do we still have hvmloader
writing the ACPI tables for HVM guests?  We should consolidate on one
consistent way of making things like this happen.

We definitely shouldn't be exposing emulation_flags to the guest, just
so a bit of early firmware can write the proper information into the
ACPI tables.  That is a broken interface.

~Andrew

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

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

[Boris Ostrovsky]

On 12/02/2016 09:06 AM, Andrew Cooper wrote:
> On 02/12/16 13:48, Roger Pau Monne 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.
>>
>> 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
>> ---
>> 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.
> (I forget if there is a good reason, but) why do we still have hvmloader
> writing the ACPI tables for HVM guests?  We should consolidate on one
> consistent way of making things like this happen.

I think one of the reasons was that MMIO addresses (which are needed by
the tables) are determined by hvmloader during runtime.

It would be nice to do this in the toolstack because we also use this
information in cacheattr_init() which appears to be the only reason why
we need to bring all possible VCPUs up and then offlining them.

-boris

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

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

[Andrew Cooper]

On 02/12/16 14:21, Boris Ostrovsky wrote:
> On 12/02/2016 09:06 AM, Andrew Cooper wrote:
>> On 02/12/16 13:48, Roger Pau Monne 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.
>>>
>>> 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
>>> ---
>>> 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.
>> (I forget if there is a good reason, but) why do we still have hvmloader
>> writing the ACPI tables for HVM guests?  We should consolidate on one
>> consistent way of making things like this happen.
> I think one of the reasons was that MMIO addresses (which are needed by
> the tables) are determined by hvmloader during runtime.
>
> It would be nice to do this in the toolstack because we also use this
> information in cacheattr_init() which appears to be the only reason why
> we need to bring all possible VCPUs up and then offlining them.

Ah yes.  This ties in with the other needs to actually know the proper
layout of the guest in one consistent location.

This can be fixed properly when the hypervisor is the one authoritative
source of guest-layout information.

Lets fudge it for now if we can; no point expending effort fixing a
corner case which is going to be fixed differently anyway.

~Andrew

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

Re: [PATCH v2 1/4] tools/libacpi: add _FADT_ to the FADT boot flags definitions

[Jan Beulich]

>>> On 02.12.16 at 14:48, <roger.pau@citrix.com> wrote:
> --- a/tools/libacpi/acpi2_0.h
> +++ b/tools/libacpi/acpi2_0.h
> @@ -227,9 +227,8 @@ struct acpi_20_fadt {
>  /*
>   * FADT Boot Architecture Flags.
>   */
> -#define ACPI_LEGACY_DEVICES (1 << 0)
> -#define ACPI_8042           (1 << 1)
> -
> +#define ACPI_FADT_LEGACY_DEVICES    (1 << 0)
> +#define ACPI_FADT_8042              (1 << 1)
>  /*
>   * FADT Fixed Feature Flags.
>   */

Afaict this breaks the build (until patch 3).

Jan


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

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

[Jan Beulich]

>>> On 02.12.16 at 14:48, <roger.pau@citrix.com> wrote:
> @@ -436,7 +439,7 @@ 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_FADT_REVISION 0x05

Do we really want to make this change unconditionally, rather than
only for PVH guests? I'm not sure which (older) OSes look at table
revisions (and may hence end up being incompatible), or whether
OSes may expect certain table versions together with certain base
ACPI versions. I think I had pointed out before that we really
should have the guest config file "acpi=" setting mean a version
number, and table revisions should then be selected according to
that base version. As that's a larger change, simply using one
fixed version for HVM and another for PVH would look fine to me.

Jan


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

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

[Roger Pau Monne]

On Fri, Dec 02, 2016 at 10:20:59AM -0700, Jan Beulich wrote:
> >>> On 02.12.16 at 14:48, <roger.pau@citrix.com> wrote:
> > @@ -436,7 +439,7 @@ 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_FADT_REVISION 0x05
> 
> Do we really want to make this change unconditionally, rather than
> only for PVH guests? I'm not sure which (older) OSes look at table
> revisions (and may hence end up being incompatible), or whether
> OSes may expect certain table versions together with certain base
> ACPI versions. I think I had pointed out before that we really
> should have the guest config file "acpi=" setting mean a version
> number, and table revisions should then be selected according to
> that base version. As that's a larger change, simply using one
> fixed version for HVM and another for PVH would look fine to me.

I don't mind using different revision numbers for HVM and PVH, but that means 
that we would need to also have two different structures, one for FADT 4.0 and 
one for FADT 5.0, which is kind of redundant, or maybe play tricks with size 
and the checksum. Also the current FADT table strcuture is named acpi_20_fadt, 
which seems to have gotten out-of-sync with the version we are using (4).

Roger.

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

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

[Jan Beulich]

>>> On 02.12.16 at 18:32, <roger.pau@citrix.com> wrote:
> On Fri, Dec 02, 2016 at 10:20:59AM -0700, Jan Beulich wrote:
>> >>> On 02.12.16 at 14:48, <roger.pau@citrix.com> wrote:
>> > @@ -436,7 +439,7 @@ 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_FADT_REVISION 0x05
>> 
>> Do we really want to make this change unconditionally, rather than
>> only for PVH guests? I'm not sure which (older) OSes look at table
>> revisions (and may hence end up being incompatible), or whether
>> OSes may expect certain table versions together with certain base
>> ACPI versions. I think I had pointed out before that we really
>> should have the guest config file "acpi=" setting mean a version
>> number, and table revisions should then be selected according to
>> that base version. As that's a larger change, simply using one
>> fixed version for HVM and another for PVH would look fine to me.
> 
> I don't mind using different revision numbers for HVM and PVH, but that means 
> that we would need to also have two different structures, one for FADT 4.0 and 
> one for FADT 5.0, which is kind of redundant, or maybe play tricks with size 

I wouldn't call this "playing tricks" - using sizeof() or offsetof() there
depending on version doesn't seem all that tricky to me, and is
common practice elsewhere.

> and the checksum. Also the current FADT table strcuture is named acpi_20_fadt, 
> which seems to have gotten out-of-sync with the version we are using (4).

Looks like so, yes. The header file's name has the same issue.

Jan


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