[Qemu-devel] [PATCH for-2.10 0/2] Fix hotplug of PCI passthrought device on Xen

[Qemu-devel] [PATCH for-2.10 0/2] Fix hotplug of PCI passthrought device on Xen

[Anthony PERARD]

Adding PCI passthrough before the guest start works fine (broken in 2.9 but now
fixed), but hotplug does not work anymore.

Anthony PERARD (2):
  hw/acpi: Call acpi_set_pci_info when no ACPI tables needed
  Revert "ACPI: don't call acpi_pcihp_device_plug_cb on xen"

 hw/acpi/piix4.c      | 11 +++--------
 hw/i386/acpi-build.c |  4 ++--
 2 files changed, 5 insertions(+), 10 deletions(-)

-- 
Anthony PERARD

[Qemu-devel] [PATCH for-2.10 1/2] hw/acpi: Call acpi_set_pci_info when no ACPI tables needed

[Anthony PERARD]

To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be
set, but this was done only when ACPI tables are built which is not
needed for a Xen guest. The need for the property starts with commit
"pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice"
(f0c9d64a68b776374ec4732424a3e27753ce37b6).

Set pci info before checking for the needs to build ACPI tables.

Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Tested-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

---
In this patch rather than always calling acpi_set_pci_info() when
acpi_setup() is called, we could check first for acpi_enabled? (which is
true for Xen.)

This patch would be a canditade to backport to 2.9.

CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Bruce Rogers <brogers@suse.com>
---
 hw/i386/acpi-build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 98dd424678..e1b7797408 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2871,6 +2871,8 @@ void acpi_setup(void)
     AcpiBuildState *build_state;
     Object *vmgenid_dev;
 
+    acpi_set_pci_info();
+
     if (!pcms->fw_cfg) {
         ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
         return;
@@ -2888,8 +2890,6 @@ void acpi_setup(void)
 
     build_state = g_malloc0(sizeof *build_state);
 
-    acpi_set_pci_info();
-
     acpi_build_tables_init(&tables);
     acpi_build(&tables, MACHINE(pcms));
 
-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH for-2.10 1/2] hw/acpi: Call acpi_set_pci_info when no ACPI tables needed

[Michael S. Tsirkin]

On Fri, Aug 11, 2017 at 04:11:37PM +0100, Anthony PERARD wrote:
> To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be
> set, but this was done only when ACPI tables are built which is not
> needed for a Xen guest. The need for the property starts with commit
> "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice"
> (f0c9d64a68b776374ec4732424a3e27753ce37b6).
> 
> Set pci info before checking for the needs to build ACPI tables.
> 
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Tested-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

I am worried that Xen will come to depend on specific
assignment of bsel which isn't guaranteed. Thoughts on
how to avoid that?



> 
> ---
> In this patch rather than always calling acpi_set_pci_info() when
> acpi_setup() is called, we could check first for acpi_enabled? (which is
> true for Xen.)

Yes, please change it like this. Also, please add
a comment explainging what it does.


Thanks!

> 
> This patch would be a canditade to backport to 2.9.
> 
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Bruce Rogers <brogers@suse.com>
> ---
>  hw/i386/acpi-build.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 98dd424678..e1b7797408 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2871,6 +2871,8 @@ void acpi_setup(void)
>      AcpiBuildState *build_state;
>      Object *vmgenid_dev;
>  
> +    acpi_set_pci_info();
> +
>      if (!pcms->fw_cfg) {
>          ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
>          return;
> @@ -2888,8 +2890,6 @@ void acpi_setup(void)
>  
>      build_state = g_malloc0(sizeof *build_state);
>  
> -    acpi_set_pci_info();
> -
>      acpi_build_tables_init(&tables);
>      acpi_build(&tables, MACHINE(pcms));
>  
> -- 
> Anthony PERARD

Re: [Qemu-devel] [PATCH for-2.10 1/2] hw/acpi: Call acpi_set_pci_info when no ACPI tables needed

[Anthony PERARD]

On Fri, Aug 11, 2017 at 08:18:28PM +0300, Michael S. Tsirkin wrote:
> On Fri, Aug 11, 2017 at 04:11:37PM +0100, Anthony PERARD wrote:
> > To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be
> > set, but this was done only when ACPI tables are built which is not
> > needed for a Xen guest. The need for the property starts with commit
> > "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice"
> > (f0c9d64a68b776374ec4732424a3e27753ce37b6).
> > 
> > Set pci info before checking for the needs to build ACPI tables.
> > 
> > Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> > Tested-by: Sander Eikelenboom <linux@eikelenboom.it>
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> I am worried that Xen will come to depend on specific
> assignment of bsel which isn't guaranteed. Thoughts on
> how to avoid that?

Is it possible to have a different BSEL than 0 with PIIX ?
Also, I don't known if having more than on PCI bus is going to work on
Xen, there is nothing in our ACPI tables beyond _SB.PCI0, and nothing to
use a different BSEL.

> > 
> > ---
> > In this patch rather than always calling acpi_set_pci_info() when
> > acpi_setup() is called, we could check first for acpi_enabled? (which is
> > true for Xen.)
> 
> Yes, please change it like this. Also, please add
> a comment explainging what it does.

Will do.

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH for-2.10 1/2] hw/acpi: Call acpi_set_pci_info when no ACPI tables needed

[Michael S. Tsirkin]

On Mon, Aug 14, 2017 at 03:55:50PM +0100, Anthony PERARD wrote:
> On Fri, Aug 11, 2017 at 08:18:28PM +0300, Michael S. Tsirkin wrote:
> > On Fri, Aug 11, 2017 at 04:11:37PM +0100, Anthony PERARD wrote:
> > > To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be
> > > set, but this was done only when ACPI tables are built which is not
> > > needed for a Xen guest. The need for the property starts with commit
> > > "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice"
> > > (f0c9d64a68b776374ec4732424a3e27753ce37b6).
> > > 
> > > Set pci info before checking for the needs to build ACPI tables.
> > > 
> > > Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> > > Tested-by: Sander Eikelenboom <linux@eikelenboom.it>
> > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > 
> > I am worried that Xen will come to depend on specific
> > assignment of bsel which isn't guaranteed. Thoughts on
> > how to avoid that?
> 
> Is it possible to have a different BSEL than 0 with PIIX ?

With PCI to PCI bridges, yes.

> Also, I don't known if having more than on PCI bus is going to work on
> Xen, there is nothing in our ACPI tables beyond _SB.PCI0, and nothing to
> use a different BSEL.

My worry is someone might decide to implement hotplug for pci to pci
bridges on Xen. If doing that, it's important to use the qemu supplied
acpi tables.

> > > 
> > > ---
> > > In this patch rather than always calling acpi_set_pci_info() when
> > > acpi_setup() is called, we could check first for acpi_enabled? (which is
> > > true for Xen.)
> > 
> > Yes, please change it like this. Also, please add
> > a comment explainging what it does.
> 
> Will do.
> 
> -- 
> Anthony PERARD

Re: [Qemu-devel] [PATCH for-2.10 1/2] hw/acpi: Call acpi_set_pci_info when no ACPI tables needed

[Anthony PERARD]

On Mon, Aug 14, 2017 at 06:53:14PM +0300, Michael S. Tsirkin wrote:
> On Mon, Aug 14, 2017 at 03:55:50PM +0100, Anthony PERARD wrote:
> > On Fri, Aug 11, 2017 at 08:18:28PM +0300, Michael S. Tsirkin wrote:
> > > On Fri, Aug 11, 2017 at 04:11:37PM +0100, Anthony PERARD wrote:
> > > > To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be
> > > > set, but this was done only when ACPI tables are built which is not
> > > > needed for a Xen guest. The need for the property starts with commit
> > > > "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice"
> > > > (f0c9d64a68b776374ec4732424a3e27753ce37b6).
> > > > 
> > > > Set pci info before checking for the needs to build ACPI tables.
> > > > 
> > > > Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> > > > Tested-by: Sander Eikelenboom <linux@eikelenboom.it>
> > > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > > 
> > > I am worried that Xen will come to depend on specific
> > > assignment of bsel which isn't guaranteed. Thoughts on
> > > how to avoid that?
> > 
> > Is it possible to have a different BSEL than 0 with PIIX ?
> 
> With PCI to PCI bridges, yes.
> 
> > Also, I don't known if having more than on PCI bus is going to work on
> > Xen, there is nothing in our ACPI tables beyond _SB.PCI0, and nothing to
> > use a different BSEL.
> 
> My worry is someone might decide to implement hotplug for pci to pci
> bridges on Xen. If doing that, it's important to use the qemu supplied
> acpi tables.

I can always add assert((xen_enable() && !bsel) || !xen_enable()) in
acpi_set_bsel(), so if someone was going to make any change, he would
find out about bsel quicker. But I don't see Xen using QEMU supplied
ACPI tables anytime soon.

Re: [Qemu-devel] [PATCH for-2.10 1/2] hw/acpi: Call acpi_set_pci_info when no ACPI tables needed

[Michael S. Tsirkin]

On Mon, Aug 14, 2017 at 05:45:02PM +0100, Anthony PERARD wrote:
> On Mon, Aug 14, 2017 at 06:53:14PM +0300, Michael S. Tsirkin wrote:
> > On Mon, Aug 14, 2017 at 03:55:50PM +0100, Anthony PERARD wrote:
> > > On Fri, Aug 11, 2017 at 08:18:28PM +0300, Michael S. Tsirkin wrote:
> > > > On Fri, Aug 11, 2017 at 04:11:37PM +0100, Anthony PERARD wrote:
> > > > > To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be
> > > > > set, but this was done only when ACPI tables are built which is not
> > > > > needed for a Xen guest. The need for the property starts with commit
> > > > > "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice"
> > > > > (f0c9d64a68b776374ec4732424a3e27753ce37b6).
> > > > > 
> > > > > Set pci info before checking for the needs to build ACPI tables.
> > > > > 
> > > > > Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> > > > > Tested-by: Sander Eikelenboom <linux@eikelenboom.it>
> > > > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > > > 
> > > > I am worried that Xen will come to depend on specific
> > > > assignment of bsel which isn't guaranteed. Thoughts on
> > > > how to avoid that?
> > > 
> > > Is it possible to have a different BSEL than 0 with PIIX ?
> > 
> > With PCI to PCI bridges, yes.
> > 
> > > Also, I don't known if having more than on PCI bus is going to work on
> > > Xen, there is nothing in our ACPI tables beyond _SB.PCI0, and nothing to
> > > use a different BSEL.
> > 
> > My worry is someone might decide to implement hotplug for pci to pci
> > bridges on Xen. If doing that, it's important to use the qemu supplied
> > acpi tables.
> 
> I can always add assert((xen_enable() && !bsel) || !xen_enable()) in
> acpi_set_bsel(), so if someone was going to make any change, he would
> find out about bsel quicker. But I don't see Xen using QEMU supplied
> ACPI tables anytime soon.

In that case I'd prefer assigning bsel 0 just for the root bus on xen.

-- 
MST

[Qemu-devel] [PATCH for-2.10 2/2] Revert "ACPI: don't call acpi_pcihp_device_plug_cb on xen"

[Anthony PERARD]

This reverts commit 153eba4726dfa1bdfc31d1fe973b2a61b9035492.

This patch prevents PCI passthrough hotplug on Xen. Even if the Xen tool
stack prepares its own ACPI tables, we still rely on QEMU for hotplug
ACPI notifications.

The original issue is fixed by the previous patch (hw/acpi: Call
acpi_set_pci_info when no ACPI tables needed).

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Bruce Rogers <brogers@suse.com>
---
 hw/acpi/piix4.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index f276967365..f4fd5907b8 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -385,10 +385,7 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
                                 dev, errp);
         }
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
-        if (!xen_enabled()) {
-            acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
-                                      errp);
-        }
+        acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         if (s->cpu_hotplug_legacy) {
             legacy_acpi_cpu_plug_cb(hotplug_dev, &s->gpe_cpu, dev, errp);
@@ -411,10 +408,8 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
         acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug,
                                       dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
-        if (!xen_enabled()) {
-            acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
-                                        errp);
-        }
+        acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
+                                    errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
                !s->cpu_hotplug_legacy) {
         acpi_cpu_unplug_request_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
-- 
Anthony PERARD