[PATCH] acpi: set correct address of the control/event blocks in the FADT

[PATCH] acpi: set correct address of the control/event blocks in the FADT

[Roger Pau Monne]

Commit 149c6b unmasked an issue long present in Xen: the control/event
block addresses provided in the ACPI FADT table where hardcoded to the
V1 version. This was papered over because hvmloader would also always
set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
version.

Fix this by passing the address of the control/event blocks to
acpi_build_tables, so the values can be properly set in the FADT
table provided to the guest.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Igor Druzhinin <igor.druzhinin@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>
---
This commit should fix the qumu-trad Windows errors seen by osstest.
---
 tools/firmware/hvmloader/ovmf.c    |  7 ++++++-
 tools/firmware/hvmloader/rombios.c |  6 ++++++
 tools/firmware/hvmloader/seabios.c |  5 +++++
 tools/firmware/hvmloader/util.c    | 10 ++++++++++
 tools/libacpi/build.c              |  9 +++++++++
 tools/libacpi/libacpi.h            |  7 +++++++
 6 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
index 17bd0fe95f..64806b6764 100644
--- a/tools/firmware/hvmloader/ovmf.c
+++ b/tools/firmware/hvmloader/ovmf.c
@@ -121,7 +121,12 @@ static void ovmf_acpi_build_tables(void)
         .dsdt_anycpu = dsdt_anycpu_qemu_xen,
         .dsdt_anycpu_len = dsdt_anycpu_qemu_xen_len,
         .dsdt_15cpu = NULL, 
-        .dsdt_15cpu_len = 0
+        .dsdt_15cpu_len = 0,
+        .pm1a_evt = ACPI_PM1A_EVT_BLK_ADDRESS_V1,
+        .pm1a_cnt = ACPI_PM1A_CNT_BLK_ADDRESS_V1,
+        .pm_tmr = ACPI_PM_TMR_BLK_ADDRESS_V1,
+        .gpe0 = ACPI_GPE0_BLK_ADDRESS_V1,
+        .gpe0_len = ACPI_GPE0_BLK_LEN_V1,
     };
 
     hvm_param_set(HVM_PARAM_ACPI_IOPORTS_LOCATION, 1);
diff --git a/tools/firmware/hvmloader/rombios.c b/tools/firmware/hvmloader/rombios.c
index b14d1f2af3..9591bbb0aa 100644
--- a/tools/firmware/hvmloader/rombios.c
+++ b/tools/firmware/hvmloader/rombios.c
@@ -31,6 +31,7 @@
 
 #include <libacpi.h>
 #include <xen/hvm/params.h>
+#include <xen/hvm/ioreq.h>
 
 #define ROM_INCLUDE_ROMBIOS
 #define ROM_INCLUDE_VGABIOS
@@ -176,6 +177,11 @@ static void rombios_acpi_build_tables(void)
         .dsdt_anycpu_len = dsdt_anycpu_len,
         .dsdt_15cpu = dsdt_15cpu,
         .dsdt_15cpu_len = dsdt_15cpu_len,
+        .pm1a_evt = ACPI_PM1A_EVT_BLK_ADDRESS_V0,
+        .pm1a_cnt = ACPI_PM1A_CNT_BLK_ADDRESS_V0,
+        .pm_tmr = ACPI_PM_TMR_BLK_ADDRESS_V0,
+        .gpe0 = ACPI_GPE0_BLK_ADDRESS_V0,
+        .gpe0_len = ACPI_GPE0_BLK_LEN_V0,
     };
 
     hvm_param_set(HVM_PARAM_ACPI_IOPORTS_LOCATION, 0);
diff --git a/tools/firmware/hvmloader/seabios.c b/tools/firmware/hvmloader/seabios.c
index c8792cd42b..0375407ca7 100644
--- a/tools/firmware/hvmloader/seabios.c
+++ b/tools/firmware/hvmloader/seabios.c
@@ -95,6 +95,11 @@ static void seabios_acpi_build_tables(void)
         .dsdt_anycpu_len = dsdt_anycpu_qemu_xen_len,
         .dsdt_15cpu = NULL,
         .dsdt_15cpu_len = 0,
+        .pm1a_evt = ACPI_PM1A_EVT_BLK_ADDRESS_V1,
+        .pm1a_cnt = ACPI_PM1A_CNT_BLK_ADDRESS_V1,
+        .pm_tmr = ACPI_PM_TMR_BLK_ADDRESS_V1,
+        .gpe0 = ACPI_GPE0_BLK_ADDRESS_V1,
+        .gpe0_len = ACPI_GPE0_BLK_LEN_V1,
     };
 
     hvm_param_set(HVM_PARAM_ACPI_IOPORTS_LOCATION, 1);
diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 934b566a5d..82171252a1 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -905,6 +905,11 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
         config->dsdt_anycpu_len = dsdt_anycpu_len;
         config->dsdt_15cpu = dsdt_15cpu;
         config->dsdt_15cpu_len = dsdt_15cpu_len;
+        config->pm1a_evt = ACPI_PM1A_EVT_BLK_ADDRESS_V0;
+        config->pm1a_cnt = ACPI_PM1A_CNT_BLK_ADDRESS_V0;
+        config->pm_tmr = ACPI_PM_TMR_BLK_ADDRESS_V0;
+        config->gpe0 = ACPI_GPE0_BLK_ADDRESS_V0;
+        config->gpe0_len = ACPI_GPE0_BLK_LEN_V0;
 
         hvm_param_set(HVM_PARAM_ACPI_IOPORTS_LOCATION, 0);
     }
@@ -914,6 +919,11 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
         config->dsdt_anycpu_len = dsdt_anycpu_qemu_xen_len;
         config->dsdt_15cpu = NULL;
         config->dsdt_15cpu_len = 0;
+        config->pm1a_evt = ACPI_PM1A_EVT_BLK_ADDRESS_V1;
+        config->pm1a_cnt = ACPI_PM1A_CNT_BLK_ADDRESS_V1;
+        config->pm_tmr = ACPI_PM_TMR_BLK_ADDRESS_V1;
+        config->gpe0 = ACPI_GPE0_BLK_ADDRESS_V1;
+        config->gpe0_len = ACPI_GPE0_BLK_LEN_V1;
 
         hvm_param_set(HVM_PARAM_ACPI_IOPORTS_LOCATION, 1);
     }
diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
index f9881c9604..50242f34c5 100644
--- a/tools/libacpi/build.c
+++ b/tools/libacpi/build.c
@@ -537,6 +537,15 @@ int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config)
     }
 
     /*
+     * Set location of the control/event registers.
+     */
+    Fadt.pm1a_evt_blk = config->pm1a_evt;
+    Fadt.pm1a_cnt_blk = config->pm1a_cnt;
+    Fadt.pm_tmr_blk = config->pm_tmr;
+    Fadt.gpe0_blk = config->gpe0;
+    Fadt.gpe0_blk_len = config->gpe0_len;
+
+    /*
      * Fill in high-memory data structures, starting at @buf.
      */
 
diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
index 2ed1ecfc8e..2c56fb7646 100644
--- a/tools/libacpi/libacpi.h
+++ b/tools/libacpi/libacpi.h
@@ -96,6 +96,13 @@ struct acpi_config {
     uint32_t ioapic_base_address;
     uint16_t pci_isa_irq_mask;
     uint8_t ioapic_id;
+
+    /* Location of the control/event registers */
+    uint32_t pm1a_evt;
+    uint32_t pm1a_cnt;
+    uint32_t pm_tmr;
+    uint32_t gpe0;
+    uint8_t gpe0_len;
 };
 
 int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config);
-- 
2.11.0 (Apple Git-81)


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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Jan Beulich]

>>> On 29.08.17 at 10:50, <roger.pau@citrix.com> wrote:
> Commit 149c6b unmasked an issue long present in Xen: the control/event
> block addresses provided in the ACPI FADT table where hardcoded to the
> V1 version. This was papered over because hvmloader would also always
> set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
> version.
> 
> Fix this by passing the address of the control/event blocks to
> acpi_build_tables, so the values can be properly set in the FADT
> table provided to the guest.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

The patch here looks good, i.e. could have my R-b, just that it is
entirely unclear to me how things did work before the quoted
commit: Ports used by Xen and qemu-trad must have been out of
sync, or am I overlooking something?

Jan

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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Roger Pau Monne]

On Tue, Aug 29, 2017 at 03:08:57AM -0600, Jan Beulich wrote:
> >>> On 29.08.17 at 10:50, <roger.pau@citrix.com> wrote:
> > Commit 149c6b unmasked an issue long present in Xen: the control/event
> > block addresses provided in the ACPI FADT table where hardcoded to the
> > V1 version. This was papered over because hvmloader would also always
> > set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
> > version.
> > 
> > Fix this by passing the address of the control/event blocks to
> > acpi_build_tables, so the values can be properly set in the FADT
> > table provided to the guest.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> The patch here looks good, i.e. could have my R-b, just that it is
> entirely unclear to me how things did work before the quoted
> commit: Ports used by Xen and qemu-trad must have been out of
> sync, or am I overlooking something?

Yes, the GPE port used by qemu-trad was out of sync with the one
reported in the FADT.

AFAICT the only thing that didn't work with qemu-trad was ACPI vCPU
hotplug, but we don't test that in osstest (not even sure if we test
xenstore vCPU hotplug).

PM1a and TMR worked fine because the V1 address was hardcoded in the
FADT, and HVM_PARAM_ACPI_IOPORTS_LOCATION was unconditionally set to 1
by hvmloader.

Roger.

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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Wei Liu]

On Tue, Aug 29, 2017 at 10:17:24AM +0100, Roger Pau Monne wrote:
> On Tue, Aug 29, 2017 at 03:08:57AM -0600, Jan Beulich wrote:
> > >>> On 29.08.17 at 10:50, <roger.pau@citrix.com> wrote:
> > > Commit 149c6b unmasked an issue long present in Xen: the control/event
> > > block addresses provided in the ACPI FADT table where hardcoded to the
> > > V1 version. This was papered over because hvmloader would also always
> > > set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
> > > version.
> > > 
> > > Fix this by passing the address of the control/event blocks to
> > > acpi_build_tables, so the values can be properly set in the FADT
> > > table provided to the guest.
> > > 
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > 
> > The patch here looks good, i.e. could have my R-b, just that it is
> > entirely unclear to me how things did work before the quoted
> > commit: Ports used by Xen and qemu-trad must have been out of
> > sync, or am I overlooking something?
> 
> Yes, the GPE port used by qemu-trad was out of sync with the one
> reported in the FADT.
> 
> AFAICT the only thing that didn't work with qemu-trad was ACPI vCPU
> hotplug, but we don't test that in osstest (not even sure if we test
> xenstore vCPU hotplug).
> 
> PM1a and TMR worked fine because the V1 address was hardcoded in the
> FADT, and HVM_PARAM_ACPI_IOPORTS_LOCATION was unconditionally set to 1
> by hvmloader.

Do you maybe want to put some of the above into the commit message?

You can provide me a new one here in a reply, no need to resend. I want
to cimmit this asap.

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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Roger Pau Monne]

On Tue, Aug 29, 2017 at 10:25:23AM +0100, Wei Liu wrote:
> On Tue, Aug 29, 2017 at 10:17:24AM +0100, Roger Pau Monne wrote:
> > On Tue, Aug 29, 2017 at 03:08:57AM -0600, Jan Beulich wrote:
> > > >>> On 29.08.17 at 10:50, <roger.pau@citrix.com> wrote:
> > > > Commit 149c6b unmasked an issue long present in Xen: the control/event
> > > > block addresses provided in the ACPI FADT table where hardcoded to the
> > > > V1 version. This was papered over because hvmloader would also always
> > > > set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
> > > > version.
> > > > 
> > > > Fix this by passing the address of the control/event blocks to
> > > > acpi_build_tables, so the values can be properly set in the FADT
> > > > table provided to the guest.
> > > > 
> > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > 
> > > The patch here looks good, i.e. could have my R-b, just that it is
> > > entirely unclear to me how things did work before the quoted
> > > commit: Ports used by Xen and qemu-trad must have been out of
> > > sync, or am I overlooking something?
> > 
> > Yes, the GPE port used by qemu-trad was out of sync with the one
> > reported in the FADT.
> > 
> > AFAICT the only thing that didn't work with qemu-trad was ACPI vCPU
> > hotplug, but we don't test that in osstest (not even sure if we test
> > xenstore vCPU hotplug).
> > 
> > PM1a and TMR worked fine because the V1 address was hardcoded in the
> > FADT, and HVM_PARAM_ACPI_IOPORTS_LOCATION was unconditionally set to 1
> > by hvmloader.
> 
> Do you maybe want to put some of the above into the commit message?
> 
> You can provide me a new one here in a reply, no need to resend. I want
> to cimmit this asap.

OK, I think the following is clearer:

Commit 149c6b unmasked an issue long present in Xen: the control/event
block addresses provided in the ACPI FADT table where hardcoded to the
V1 version. This was papered over because hvmloader would also always
set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
version.

The most notable issue caused by the above bug was that the QEMU
traditional GPE0 block was out of sync: the address provided in the
FADT didn't match the address QEMU was using.

Note that PM1a and TMR worked fine because the V1 address was
hardcoded in the FADT and HVM_PARAM_ACPI_IOPORTS_LOCATION was
unconditionally set to 1 by hvmloader.

Fix this by passing the address of the control/event blocks to
acpi_build_tables, so the values can be properly set in the FADT table
provided to the guest.

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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Jan Beulich]

>>> On 29.08.17 at 11:31, <roger.pau@citrix.com> wrote:
> On Tue, Aug 29, 2017 at 10:25:23AM +0100, Wei Liu wrote:
>> On Tue, Aug 29, 2017 at 10:17:24AM +0100, Roger Pau Monne wrote:
>> > On Tue, Aug 29, 2017 at 03:08:57AM -0600, Jan Beulich wrote:
>> > > >>> On 29.08.17 at 10:50, <roger.pau@citrix.com> wrote:
>> > > > Commit 149c6b unmasked an issue long present in Xen: the control/event
>> > > > block addresses provided in the ACPI FADT table where hardcoded to the
>> > > > V1 version. This was papered over because hvmloader would also always
>> > > > set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
>> > > > version.
>> > > > 
>> > > > Fix this by passing the address of the control/event blocks to
>> > > > acpi_build_tables, so the values can be properly set in the FADT
>> > > > table provided to the guest.
>> > > > 
>> > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> > > 
>> > > The patch here looks good, i.e. could have my R-b, just that it is
>> > > entirely unclear to me how things did work before the quoted
>> > > commit: Ports used by Xen and qemu-trad must have been out of
>> > > sync, or am I overlooking something?
>> > 
>> > Yes, the GPE port used by qemu-trad was out of sync with the one
>> > reported in the FADT.
>> > 
>> > AFAICT the only thing that didn't work with qemu-trad was ACPI vCPU
>> > hotplug, but we don't test that in osstest (not even sure if we test
>> > xenstore vCPU hotplug).
>> > 
>> > PM1a and TMR worked fine because the V1 address was hardcoded in the
>> > FADT, and HVM_PARAM_ACPI_IOPORTS_LOCATION was unconditionally set to 1
>> > by hvmloader.
>> 
>> Do you maybe want to put some of the above into the commit message?
>> 
>> You can provide me a new one here in a reply, no need to resend. I want
>> to cimmit this asap.
> 
> OK, I think the following is clearer:
> 
> Commit 149c6b unmasked an issue long present in Xen: the control/event
> block addresses provided in the ACPI FADT table where hardcoded to the
> V1 version. This was papered over because hvmloader would also always
> set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
> version.
> 
> The most notable issue caused by the above bug was that the QEMU
> traditional GPE0 block was out of sync: the address provided in the
> FADT didn't match the address QEMU was using.
> 
> Note that PM1a and TMR worked fine because the V1 address was
> hardcoded in the FADT and HVM_PARAM_ACPI_IOPORTS_LOCATION was
> unconditionally set to 1 by hvmloader.
> 
> Fix this by passing the address of the control/event blocks to
> acpi_build_tables, so the values can be properly set in the FADT table
> provided to the guest.

LGTM

Jan

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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Wei Liu]

On Tue, Aug 29, 2017 at 03:41:05AM -0600, Jan Beulich wrote:
> >>> On 29.08.17 at 11:31, <roger.pau@citrix.com> wrote:
> > On Tue, Aug 29, 2017 at 10:25:23AM +0100, Wei Liu wrote:
> >> On Tue, Aug 29, 2017 at 10:17:24AM +0100, Roger Pau Monne wrote:
> >> > On Tue, Aug 29, 2017 at 03:08:57AM -0600, Jan Beulich wrote:
> >> > > >>> On 29.08.17 at 10:50, <roger.pau@citrix.com> wrote:
> >> > > > Commit 149c6b unmasked an issue long present in Xen: the control/event
> >> > > > block addresses provided in the ACPI FADT table where hardcoded to the
> >> > > > V1 version. This was papered over because hvmloader would also always
> >> > > > set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
> >> > > > version.
> >> > > > 
> >> > > > Fix this by passing the address of the control/event blocks to
> >> > > > acpi_build_tables, so the values can be properly set in the FADT
> >> > > > table provided to the guest.
> >> > > > 
> >> > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> > > 
> >> > > The patch here looks good, i.e. could have my R-b, just that it is
> >> > > entirely unclear to me how things did work before the quoted
> >> > > commit: Ports used by Xen and qemu-trad must have been out of
> >> > > sync, or am I overlooking something?
> >> > 
> >> > Yes, the GPE port used by qemu-trad was out of sync with the one
> >> > reported in the FADT.
> >> > 
> >> > AFAICT the only thing that didn't work with qemu-trad was ACPI vCPU
> >> > hotplug, but we don't test that in osstest (not even sure if we test
> >> > xenstore vCPU hotplug).
> >> > 
> >> > PM1a and TMR worked fine because the V1 address was hardcoded in the
> >> > FADT, and HVM_PARAM_ACPI_IOPORTS_LOCATION was unconditionally set to 1
> >> > by hvmloader.
> >> 
> >> Do you maybe want to put some of the above into the commit message?
> >> 
> >> You can provide me a new one here in a reply, no need to resend. I want
> >> to cimmit this asap.
> > 
> > OK, I think the following is clearer:
> > 
> > Commit 149c6b unmasked an issue long present in Xen: the control/event
> > block addresses provided in the ACPI FADT table where hardcoded to the
> > V1 version. This was papered over because hvmloader would also always
> > set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
> > version.
> > 
> > The most notable issue caused by the above bug was that the QEMU
> > traditional GPE0 block was out of sync: the address provided in the
> > FADT didn't match the address QEMU was using.
> > 
> > Note that PM1a and TMR worked fine because the V1 address was
> > hardcoded in the FADT and HVM_PARAM_ACPI_IOPORTS_LOCATION was
> > unconditionally set to 1 by hvmloader.
> > 
> > Fix this by passing the address of the control/event blocks to
> > acpi_build_tables, so the values can be properly set in the FADT table
> > provided to the guest.
> 
> LGTM

Pushed. Thanks.

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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Andrew Cooper]

On 29/08/17 09:50, Roger Pau Monne wrote:
> Commit 149c6b unmasked an issue long present in Xen: the control/event
> block addresses provided in the ACPI FADT table where hardcoded to the
> V1 version. This was papered over because hvmloader would also always
> set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
> version.
>
> Fix this by passing the address of the control/event blocks to
> acpi_build_tables, so the values can be properly set in the FADT
> table provided to the guest.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Igor Druzhinin <igor.druzhinin@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>
> ---
> This commit should fix the qumu-trad Windows errors seen by osstest.

This changes windows behaviour, but does not fix windows.  Windows now
boots, but waits forever while trying to reboot after installing PV
drivers.  There is no hint in the qemu log that the ACPI shutdown event
was received.

Unless someone has some very quick clever ideas, the original fix will
need reverting.

~Andrew

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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Wei Liu]

On Tue, Aug 29, 2017 at 02:24:49PM +0100, Andrew Cooper wrote:
> On 29/08/17 09:50, Roger Pau Monne wrote:
> > Commit 149c6b unmasked an issue long present in Xen: the control/event
> > block addresses provided in the ACPI FADT table where hardcoded to the
> > V1 version. This was papered over because hvmloader would also always
> > set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
> > version.
> >
> > Fix this by passing the address of the control/event blocks to
> > acpi_build_tables, so the values can be properly set in the FADT
> > table provided to the guest.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Cc: Igor Druzhinin <igor.druzhinin@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>
> > ---
> > This commit should fix the qumu-trad Windows errors seen by osstest.
> 
> This changes windows behaviour, but does not fix windows.  Windows now
> boots, but waits forever while trying to reboot after installing PV
> drivers.  There is no hint in the qemu log that the ACPI shutdown event
> was received.
> 
> Unless someone has some very quick clever ideas, the original fix will
> need reverting.

If I don't get a new fix by the end of today I'm going to revert Igor's
patch (but keep Roger's patch in tree).

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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Igor Druzhinin]

On 29/08/17 14:33, Wei Liu wrote:
> On Tue, Aug 29, 2017 at 02:24:49PM +0100, Andrew Cooper wrote:
>> On 29/08/17 09:50, Roger Pau Monne wrote:
>>> Commit 149c6b unmasked an issue long present in Xen: the control/event
>>> block addresses provided in the ACPI FADT table where hardcoded to the
>>> V1 version. This was papered over because hvmloader would also always
>>> set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
>>> version.
>>>
>>> Fix this by passing the address of the control/event blocks to
>>> acpi_build_tables, so the values can be properly set in the FADT
>>> table provided to the guest.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> Cc: Igor Druzhinin <igor.druzhinin@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>
>>> ---
>>> This commit should fix the qumu-trad Windows errors seen by osstest.
>>
>> This changes windows behaviour, but does not fix windows.  Windows now
>> boots, but waits forever while trying to reboot after installing PV
>> drivers.  There is no hint in the qemu log that the ACPI shutdown event
>> was received.
>>
>> Unless someone has some very quick clever ideas, the original fix will
>> need reverting.
> 
> If I don't get a new fix by the end of today I'm going to revert Igor's
> patch (but keep Roger's patch in tree).
> 

I guess the easiest way to overcome it would be to set "qemu-xen" as a
device-model in libxl unconditionally.

Igor

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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Jan Beulich]

>>> On 29.08.17 at 15:24, <andrew.cooper3@citrix.com> wrote:
> On 29/08/17 09:50, Roger Pau Monne wrote:
>> Commit 149c6b unmasked an issue long present in Xen: the control/event
>> block addresses provided in the ACPI FADT table where hardcoded to the
>> V1 version. This was papered over because hvmloader would also always
>> set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
>> version.
>>
>> Fix this by passing the address of the control/event blocks to
>> acpi_build_tables, so the values can be properly set in the FADT
>> table provided to the guest.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> Cc: Igor Druzhinin <igor.druzhinin@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>
>> ---
>> This commit should fix the qumu-trad Windows errors seen by osstest.
> 
> This changes windows behaviour, but does not fix windows.  Windows now
> boots, but waits forever while trying to reboot after installing PV
> drivers.  There is no hint in the qemu log that the ACPI shutdown event
> was received.

Sounds to me like matching exactly the question I've raised: It
would help to understand why things have worked originally.
While PM1a/b are generally meant to help split brain environments
like our Xen/qemu one, iirc we don't make use of PM1b, and hence
it seems quite likely that both Xen and qemu monitor PM1a port
accesses. If that's the case and things have worked before, it's
quite possible that qemu-trad is no servicing the wrong port.

> Unless someone has some very quick clever ideas, the original fix will
> need reverting.

I agree.

Jan

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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Jan Beulich]

>>> On 29.08.17 at 15:33, <wei.liu2@citrix.com> wrote:
> If I don't get a new fix by the end of today I'm going to revert Igor's
> patch (but keep Roger's patch in tree).

I'm not convinced keep the non-conflicting parts of Roger's patch
is going to work; see the other reply just sent.

Jan


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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Wei Liu]

On Tue, Aug 29, 2017 at 07:43:41AM -0600, Jan Beulich wrote:
> >>> On 29.08.17 at 15:33, <wei.liu2@citrix.com> wrote:
> > If I don't get a new fix by the end of today I'm going to revert Igor's
> > patch (but keep Roger's patch in tree).
> 
> I'm not convinced keep the non-conflicting parts of Roger's patch
> is going to work; see the other reply just sent.

Hmm... I was under the impression that Roger's fix was for a latent bug
unmasked by Igor's patch.

Anyway, I think I'm going to revert both patches if necessary.

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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Wei Liu]

On Tue, Aug 29, 2017 at 02:37:50PM +0100, Igor Druzhinin wrote:
> On 29/08/17 14:33, Wei Liu wrote:
> > On Tue, Aug 29, 2017 at 02:24:49PM +0100, Andrew Cooper wrote:
> >> On 29/08/17 09:50, Roger Pau Monne wrote:
> >>> Commit 149c6b unmasked an issue long present in Xen: the control/event
> >>> block addresses provided in the ACPI FADT table where hardcoded to the
> >>> V1 version. This was papered over because hvmloader would also always
> >>> set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
> >>> version.
> >>>
> >>> Fix this by passing the address of the control/event blocks to
> >>> acpi_build_tables, so the values can be properly set in the FADT
> >>> table provided to the guest.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>> ---
> >>> Cc: Igor Druzhinin <igor.druzhinin@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>
> >>> ---
> >>> This commit should fix the qumu-trad Windows errors seen by osstest.
> >>
> >> This changes windows behaviour, but does not fix windows.  Windows now
> >> boots, but waits forever while trying to reboot after installing PV
> >> drivers.  There is no hint in the qemu log that the ACPI shutdown event
> >> was received.
> >>
> >> Unless someone has some very quick clever ideas, the original fix will
> >> need reverting.
> > 
> > If I don't get a new fix by the end of today I'm going to revert Igor's
> > patch (but keep Roger's patch in tree).
> > 
> 
> I guess the easiest way to overcome it would be to set "qemu-xen" as a
> device-model in libxl unconditionally.

I don't think that's right because libxl does support both qemu-xen and
qemu-trad. The value written in xenstore should reflect the reality.

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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Igor Druzhinin]

On 29/08/17 14:51, Wei Liu wrote:
> On Tue, Aug 29, 2017 at 02:37:50PM +0100, Igor Druzhinin wrote:
>> On 29/08/17 14:33, Wei Liu wrote:
>>> On Tue, Aug 29, 2017 at 02:24:49PM +0100, Andrew Cooper wrote:
>>>> On 29/08/17 09:50, Roger Pau Monne wrote:
>>>>> Commit 149c6b unmasked an issue long present in Xen: the control/event
>>>>> block addresses provided in the ACPI FADT table where hardcoded to the
>>>>> V1 version. This was papered over because hvmloader would also always
>>>>> set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
>>>>> version.
>>>>>
>>>>> Fix this by passing the address of the control/event blocks to
>>>>> acpi_build_tables, so the values can be properly set in the FADT
>>>>> table provided to the guest.
>>>>>
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>> ---
>>>>> Cc: Igor Druzhinin <igor.druzhinin@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>
>>>>> ---
>>>>> This commit should fix the qumu-trad Windows errors seen by osstest.
>>>>
>>>> This changes windows behaviour, but does not fix windows.  Windows now
>>>> boots, but waits forever while trying to reboot after installing PV
>>>> drivers.  There is no hint in the qemu log that the ACPI shutdown event
>>>> was received.
>>>>
>>>> Unless someone has some very quick clever ideas, the original fix will
>>>> need reverting.
>>>
>>> If I don't get a new fix by the end of today I'm going to revert Igor's
>>> patch (but keep Roger's patch in tree).
>>>
>>
>> I guess the easiest way to overcome it would be to set "qemu-xen" as a
>> device-model in libxl unconditionally.
> 
> I don't think that's right because libxl does support both qemu-xen and
> qemu-trad. The value written in xenstore should reflect the reality.
> 

In that case, probably worth reverting until we figure out why setting
the right port location causes such an effect.

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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Jan Beulich]

>>> On 29.08.17 at 15:46, <wei.liu2@citrix.com> wrote:
> On Tue, Aug 29, 2017 at 07:43:41AM -0600, Jan Beulich wrote:
>> >>> On 29.08.17 at 15:33, <wei.liu2@citrix.com> wrote:
>> > If I don't get a new fix by the end of today I'm going to revert Igor's
>> > patch (but keep Roger's patch in tree).
>> 
>> I'm not convinced keep the non-conflicting parts of Roger's patch
>> is going to work; see the other reply just sent.
> 
> Hmm... I was under the impression that Roger's fix was for a latent bug
> unmasked by Igor's patch.

It certainly was, but besides changing code the earlier patch
introduced it also cannot have been fully correct/complete,
or else things would be working again now.

Jan


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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Wei Liu]

On Tue, Aug 29, 2017 at 02:53:56PM +0100, Igor Druzhinin wrote:
> On 29/08/17 14:51, Wei Liu wrote:
> > On Tue, Aug 29, 2017 at 02:37:50PM +0100, Igor Druzhinin wrote:
> >> On 29/08/17 14:33, Wei Liu wrote:
> >>> On Tue, Aug 29, 2017 at 02:24:49PM +0100, Andrew Cooper wrote:
> >>>> On 29/08/17 09:50, Roger Pau Monne wrote:
> >>>>> Commit 149c6b unmasked an issue long present in Xen: the control/event
> >>>>> block addresses provided in the ACPI FADT table where hardcoded to the
> >>>>> V1 version. This was papered over because hvmloader would also always
> >>>>> set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
> >>>>> version.
> >>>>>
> >>>>> Fix this by passing the address of the control/event blocks to
> >>>>> acpi_build_tables, so the values can be properly set in the FADT
> >>>>> table provided to the guest.
> >>>>>
> >>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>>>> ---
> >>>>> Cc: Igor Druzhinin <igor.druzhinin@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>
> >>>>> ---
> >>>>> This commit should fix the qumu-trad Windows errors seen by osstest.
> >>>>
> >>>> This changes windows behaviour, but does not fix windows.  Windows now
> >>>> boots, but waits forever while trying to reboot after installing PV
> >>>> drivers.  There is no hint in the qemu log that the ACPI shutdown event
> >>>> was received.
> >>>>
> >>>> Unless someone has some very quick clever ideas, the original fix will
> >>>> need reverting.
> >>>
> >>> If I don't get a new fix by the end of today I'm going to revert Igor's
> >>> patch (but keep Roger's patch in tree).
> >>>
> >>
> >> I guess the easiest way to overcome it would be to set "qemu-xen" as a
> >> device-model in libxl unconditionally.
> > 
> > I don't think that's right because libxl does support both qemu-xen and
> > qemu-trad. The value written in xenstore should reflect the reality.
> > 
> 
> In that case, probably worth reverting until we figure out why setting
> the right port location causes such an effect.

No problem. I will do that now.

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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Igor Druzhinin]

On 29/08/17 14:42, Jan Beulich wrote:
>>>> On 29.08.17 at 15:24, <andrew.cooper3@citrix.com> wrote:
>> On 29/08/17 09:50, Roger Pau Monne wrote:
>>> Commit 149c6b unmasked an issue long present in Xen: the control/event
>>> block addresses provided in the ACPI FADT table where hardcoded to the
>>> V1 version. This was papered over because hvmloader would also always
>>> set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
>>> version.
>>>
>>> Fix this by passing the address of the control/event blocks to
>>> acpi_build_tables, so the values can be properly set in the FADT
>>> table provided to the guest.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> Cc: Igor Druzhinin <igor.druzhinin@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>
>>> ---
>>> This commit should fix the qumu-trad Windows errors seen by osstest.
>>
>> This changes windows behaviour, but does not fix windows.  Windows now
>> boots, but waits forever while trying to reboot after installing PV
>> drivers.  There is no hint in the qemu log that the ACPI shutdown event
>> was received.
> 
> Sounds to me like matching exactly the question I've raised: It
> would help to understand why things have worked originally.
> While PM1a/b are generally meant to help split brain environments
> like our Xen/qemu one, iirc we don't make use of PM1b, and hence
> it seems quite likely that both Xen and qemu monitor PM1a port
> accesses. If that's the case and things have worked before, it's
> quite possible that qemu-trad is no servicing the wrong port.
> 

That's what actually happen. It seems that modern versions of qemu-trad
service V1 port location. I was initially confused by comments in Xen
suggesting that V0 is the right choice for qemu-trad. Seems they need to
be updated or removed.

I'll prepare an updated version of the fix today. And it looks like we
don't need Roger's fix in that case.

>> Unless someone has some very quick clever ideas, the original fix will
>> need reverting.
> 
> I agree.
> 
> Jan
> 

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

Re: [PATCH] acpi: set correct address of the control/event blocks in the FADT

[Roger Pau Monne]

On Tue, Aug 29, 2017 at 03:26:11PM +0100, Igor Druzhinin wrote:
> 
> 
> On 29/08/17 14:42, Jan Beulich wrote:
> >>>> On 29.08.17 at 15:24, <andrew.cooper3@citrix.com> wrote:
> >> On 29/08/17 09:50, Roger Pau Monne wrote:
> >>> Commit 149c6b unmasked an issue long present in Xen: the control/event
> >>> block addresses provided in the ACPI FADT table where hardcoded to the
> >>> V1 version. This was papered over because hvmloader would also always
> >>> set HVM_PARAM_ACPI_IOPORTS_LOCATION to 1 regardless of the BIOS
> >>> version.
> >>>
> >>> Fix this by passing the address of the control/event blocks to
> >>> acpi_build_tables, so the values can be properly set in the FADT
> >>> table provided to the guest.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>> ---
> >>> Cc: Igor Druzhinin <igor.druzhinin@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>
> >>> ---
> >>> This commit should fix the qumu-trad Windows errors seen by osstest.
> >>
> >> This changes windows behaviour, but does not fix windows.  Windows now
> >> boots, but waits forever while trying to reboot after installing PV
> >> drivers.  There is no hint in the qemu log that the ACPI shutdown event
> >> was received.
> > 
> > Sounds to me like matching exactly the question I've raised: It
> > would help to understand why things have worked originally.
> > While PM1a/b are generally meant to help split brain environments
> > like our Xen/qemu one, iirc we don't make use of PM1b, and hence
> > it seems quite likely that both Xen and qemu monitor PM1a port
> > accesses. If that's the case and things have worked before, it's
> > quite possible that qemu-trad is no servicing the wrong port.
> > 
> 
> That's what actually happen. It seems that modern versions of qemu-trad
> service V1 port location. I was initially confused by comments in Xen
> suggesting that V0 is the right choice for qemu-trad. Seems they need to
> be updated or removed.

Right, I just came to the same conclusion after looking at qemu-trad
code. Can you also fix the comments in ioreq.h so that in the future
we don't fall into the same trap.

> I'll prepare an updated version of the fix today. And it looks like we
> don't need Roger's fix in that case.

Agreed, my fix is not needed, and you just need to unconditionally set
HVM_PARAM_ACPI_IOPORTS_LOCATION to 1, and there's no need to set it
based on BIOS version.

Wei has already reverted your patch and my fix, so please fix/expand
your original change accordingly, and post it against current staging.

Thanks, Roger.

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