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

[Qemu-devel] [PATCH for-2.10 v2 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 | 25 ++++++++++++++++---------
 2 files changed, 19 insertions(+), 17 deletions(-)

-- 
Anthony PERARD

[Qemu-devel] [PATCH for-2.10 v2 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.

Assign bsel=0 property only to the root bus on Xen as there is no
support in the Xen ACPI tables for a different value.

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

---
Changes in V2:
  - check for acpi_enabled before calling acpi_set_pci_info.
  - set the property on the root bus only.

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 | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 98dd424678..c0483b96cf 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -46,6 +46,7 @@
 #include "sysemu/tpm_backend.h"
 #include "hw/timer/mc146818rtc_regs.h"
 #include "sysemu/numa.h"
+#include "hw/xen/xen.h"
 
 /* Supported chipsets: */
 #include "hw/acpi/piix4.h"
@@ -518,8 +519,13 @@ static void acpi_set_pci_info(void)
     unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
 
     if (bus) {
-        /* Scan all PCI buses. Set property to enable acpi based hotplug. */
-        pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
+        if (xen_enabled()) {
+            /* Assign BSEL property to root bus only. */
+            acpi_set_bsel(bus, &bsel_alloc);
+        } else {
+            /* Scan all PCI buses. Set property to enable acpi based hotplug. */
+            pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
+        }
     }
 }
 
@@ -2871,6 +2877,14 @@ void acpi_setup(void)
     AcpiBuildState *build_state;
     Object *vmgenid_dev;
 
+    if (!acpi_enabled) {
+        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
+        return;
+    }
+
+    /* Assign BSEL property on hotpluggable PCI buses. */
+    acpi_set_pci_info();
+
     if (!pcms->fw_cfg) {
         ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
         return;
@@ -2881,15 +2895,8 @@ void acpi_setup(void)
         return;
     }
 
-    if (!acpi_enabled) {
-        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
-        return;
-    }
-
     build_state = g_malloc0(sizeof *build_state);
 
-    acpi_set_pci_info();
-
     acpi_build_tables_init(&tables);
     acpi_build(&tables, MACHINE(pcms));
 
-- 
Anthony PERARD

[Qemu-devel] [PATCH for-2.10 v2 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

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

[Igor Mammedov]

On Tue, 15 Aug 2017 12:15:48 +0100
Anthony PERARD <anthony.perard@citrix.com> 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.
> 
> Assign bsel=0 property only to the root bus on Xen as there is no
> support in the Xen ACPI tables for a different value.

looking at hw/acpi/pcihp.c and bsel usage there it looks like
bsel property is owned by it and not by ACPI tables, so instead of
shuffling it in acpi_setup(), how about moving bsel initialization
to hw/acpi/pcihp.c and initialize it there unconditionally?

It could be as simple as moving acpi_set_pci_info()/acpi_set_bsel()
there and calling it from acpi_pcihp_reset().

Then there won't be need for Xen specific branches, as root bus
will have bsel set automatically which is sufficient for Xen and
the rest of bsel-s (bridges) will be just unused by Xen,
which could later extend its ACPI table implementation to utilize them. 


> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> ---
> Changes in V2:
>   - check for acpi_enabled before calling acpi_set_pci_info.
>   - set the property on the root bus only.
> 
> 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 | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 98dd424678..c0483b96cf 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -46,6 +46,7 @@
>  #include "sysemu/tpm_backend.h"
>  #include "hw/timer/mc146818rtc_regs.h"
>  #include "sysemu/numa.h"
> +#include "hw/xen/xen.h"
>  
>  /* Supported chipsets: */
>  #include "hw/acpi/piix4.h"
> @@ -518,8 +519,13 @@ static void acpi_set_pci_info(void)
>      unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
>  
>      if (bus) {
> -        /* Scan all PCI buses. Set property to enable acpi based hotplug. */
> -        pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
> +        if (xen_enabled()) {
> +            /* Assign BSEL property to root bus only. */
> +            acpi_set_bsel(bus, &bsel_alloc);
> +        } else {
> +            /* Scan all PCI buses. Set property to enable acpi based hotplug. */
> +            pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
> +        }
>      }
>  }
>  
> @@ -2871,6 +2877,14 @@ void acpi_setup(void)
>      AcpiBuildState *build_state;
>      Object *vmgenid_dev;
>  
> +    if (!acpi_enabled) {
> +        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
> +        return;
> +    }
> +
> +    /* Assign BSEL property on hotpluggable PCI buses. */
> +    acpi_set_pci_info();
> +
>      if (!pcms->fw_cfg) {
>          ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
>          return;
> @@ -2881,15 +2895,8 @@ void acpi_setup(void)
>          return;
>      }
>  
> -    if (!acpi_enabled) {
> -        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
> -        return;
> -    }
> -
>      build_state = g_malloc0(sizeof *build_state);
>  
> -    acpi_set_pci_info();
> -
>      acpi_build_tables_init(&tables);
>      acpi_build(&tables, MACHINE(pcms));
>  

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

[Michael S. Tsirkin]

On Tue, Aug 15, 2017 at 02:07:51PM +0200, Igor Mammedov wrote:
> On Tue, 15 Aug 2017 12:15:48 +0100
> Anthony PERARD <anthony.perard@citrix.com> 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.
> > 
> > Assign bsel=0 property only to the root bus on Xen as there is no
> > support in the Xen ACPI tables for a different value.
> 
> looking at hw/acpi/pcihp.c and bsel usage there it looks like
> bsel property is owned by it and not by ACPI tables, so instead of
> shuffling it in acpi_setup(), how about moving bsel initialization
> to hw/acpi/pcihp.c and initialize it there unconditionally?
> 
> It could be as simple as moving acpi_set_pci_info()/acpi_set_bsel()
> there and calling it from acpi_pcihp_reset().
> 
> Then there won't be need for Xen specific branches, as root bus
> will have bsel set automatically which is sufficient for Xen and
> the rest of bsel-s (bridges) will be just unused by Xen,
> which could later extend its ACPI table implementation to utilize them. 

Later is exactly what I'd like to try to avoid.
Whoever wants acpi hotplug for bridges needs to get
the bsel info from qemu supplied acpi tables.

> 
> > Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > 
> > ---
> > Changes in V2:
> >   - check for acpi_enabled before calling acpi_set_pci_info.
> >   - set the property on the root bus only.
> > 
> > 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 | 25 ++++++++++++++++---------
> >  1 file changed, 16 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 98dd424678..c0483b96cf 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -46,6 +46,7 @@
> >  #include "sysemu/tpm_backend.h"
> >  #include "hw/timer/mc146818rtc_regs.h"
> >  #include "sysemu/numa.h"
> > +#include "hw/xen/xen.h"
> >  
> >  /* Supported chipsets: */
> >  #include "hw/acpi/piix4.h"
> > @@ -518,8 +519,13 @@ static void acpi_set_pci_info(void)
> >      unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
> >  
> >      if (bus) {
> > -        /* Scan all PCI buses. Set property to enable acpi based hotplug. */
> > -        pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
> > +        if (xen_enabled()) {
> > +            /* Assign BSEL property to root bus only. */
> > +            acpi_set_bsel(bus, &bsel_alloc);
> > +        } else {
> > +            /* Scan all PCI buses. Set property to enable acpi based hotplug. */
> > +            pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
> > +        }
> >      }
> >  }
> >  
> > @@ -2871,6 +2877,14 @@ void acpi_setup(void)
> >      AcpiBuildState *build_state;
> >      Object *vmgenid_dev;
> >  
> > +    if (!acpi_enabled) {
> > +        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
> > +        return;
> > +    }
> > +
> > +    /* Assign BSEL property on hotpluggable PCI buses. */
> > +    acpi_set_pci_info();
> > +
> >      if (!pcms->fw_cfg) {
> >          ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
> >          return;
> > @@ -2881,15 +2895,8 @@ void acpi_setup(void)
> >          return;
> >      }
> >  
> > -    if (!acpi_enabled) {
> > -        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
> > -        return;
> > -    }
> > -
> >      build_state = g_malloc0(sizeof *build_state);
> >  
> > -    acpi_set_pci_info();
> > -
> >      acpi_build_tables_init(&tables);
> >      acpi_build(&tables, MACHINE(pcms));
> >  

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

[Igor Mammedov]

On Tue, 15 Aug 2017 22:24:08 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Aug 15, 2017 at 02:07:51PM +0200, Igor Mammedov wrote:
> > On Tue, 15 Aug 2017 12:15:48 +0100
> > Anthony PERARD <anthony.perard@citrix.com> 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.
> > > 
> > > Assign bsel=0 property only to the root bus on Xen as there is no
> > > support in the Xen ACPI tables for a different value.  
> > 
> > looking at hw/acpi/pcihp.c and bsel usage there it looks like
> > bsel property is owned by it and not by ACPI tables, so instead of
> > shuffling it in acpi_setup(), how about moving bsel initialization
> > to hw/acpi/pcihp.c and initialize it there unconditionally?
> > 
> > It could be as simple as moving acpi_set_pci_info()/acpi_set_bsel()
> > there and calling it from acpi_pcihp_reset().
> > 
> > Then there won't be need for Xen specific branches, as root bus
> > will have bsel set automatically which is sufficient for Xen and
> > the rest of bsel-s (bridges) will be just unused by Xen,
> > which could later extend its ACPI table implementation to utilize them.   
> 
> Later is exactly what I'd like to try to avoid.
> Whoever wants acpi hotplug for bridges needs to get
> the bsel info from qemu supplied acpi tables.

I'd prefer to have only one behavior in QEMU (on hw interface)
side and let Xen to maintain their own ACPI tables  dealing
with issues that arise from it since they insist on doing job twice.

The point is bsel is so embedded in HW part of impl.
that it should be allocated/manged there, otherwise it leads
to hacks where acpi_setup() is called but does partial init
and then bails out to fix code pcihp.c that depend on it being run,
pcihp.c (hw part) shouldn't depend on on ACPI tables generation
(bios part).

Anyway if you insist on capping Xen, it probably could be done
with comat machinery, something like this:

(where the 1st hunk should been there since, we've introduced
"acpi-pci-hotplug-with-bridge-support")

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index c420a38..a55f022 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -273,7 +273,7 @@ static void pci_write(void *opaque, hwaddr addr, uint64_t data,
                       addr, data);
         break;
     case PCI_SEL_BASE:
-        s->hotplug_select = data;
+        s->hotplug_select = s->legacy_piix ? 0 : data;
         ACPI_PCIHP_DPRINTF("pcisel write %" HWADDR_PRIx " <== %" PRIu64 "\n",
                       addr, data);
     default:
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 22dbef6..81b8c3e 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -1117,6 +1117,13 @@ static void xenfv_machine_options(MachineClass *m)
     m->max_cpus = HVM_MAX_VCPUS;
     m->default_machine_opts = "accel=xen";
     m->hot_add_cpu = pc_hot_add_cpu;
+    SET_MACHINE_COMPAT(m,
+        {\
+            .driver   = "PIIX4_PM",\
+            .property = "acpi-pci-hotplug-with-bridge-support",\
+            .value    = "off",\
+        },
+    );
 }
 
 DEFINE_PC_MACHINE(xenfv, "xenfv", pc_xen_hvm_init,



> > > Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > > 
> > > ---
> > > Changes in V2:
> > >   - check for acpi_enabled before calling acpi_set_pci_info.
> > >   - set the property on the root bus only.
> > > 
> > > 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 | 25 ++++++++++++++++---------
> > >  1 file changed, 16 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 98dd424678..c0483b96cf 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -46,6 +46,7 @@
> > >  #include "sysemu/tpm_backend.h"
> > >  #include "hw/timer/mc146818rtc_regs.h"
> > >  #include "sysemu/numa.h"
> > > +#include "hw/xen/xen.h"
> > >  
> > >  /* Supported chipsets: */
> > >  #include "hw/acpi/piix4.h"
> > > @@ -518,8 +519,13 @@ static void acpi_set_pci_info(void)
> > >      unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
> > >  
> > >      if (bus) {
> > > -        /* Scan all PCI buses. Set property to enable acpi based hotplug. */
> > > -        pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
> > > +        if (xen_enabled()) {
> > > +            /* Assign BSEL property to root bus only. */
> > > +            acpi_set_bsel(bus, &bsel_alloc);
> > > +        } else {
> > > +            /* Scan all PCI buses. Set property to enable acpi based hotplug. */
> > > +            pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
> > > +        }
> > >      }
> > >  }
> > >  
> > > @@ -2871,6 +2877,14 @@ void acpi_setup(void)
> > >      AcpiBuildState *build_state;
> > >      Object *vmgenid_dev;
> > >  
> > > +    if (!acpi_enabled) {
> > > +        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
> > > +        return;
> > > +    }
> > > +
> > > +    /* Assign BSEL property on hotpluggable PCI buses. */
> > > +    acpi_set_pci_info();
> > > +
> > >      if (!pcms->fw_cfg) {
> > >          ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
> > >          return;
> > > @@ -2881,15 +2895,8 @@ void acpi_setup(void)
> > >          return;
> > >      }
> > >  
> > > -    if (!acpi_enabled) {
> > > -        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
> > > -        return;
> > > -    }
> > > -
> > >      build_state = g_malloc0(sizeof *build_state);
> > >  
> > > -    acpi_set_pci_info();
> > > -
> > >      acpi_build_tables_init(&tables);
> > >      acpi_build(&tables, MACHINE(pcms));
> > >    

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

[Anthony PERARD]

On Wed, Aug 16, 2017 at 11:10:46AM +0200, Igor Mammedov wrote:
> On Tue, 15 Aug 2017 22:24:08 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Aug 15, 2017 at 02:07:51PM +0200, Igor Mammedov wrote:
> > > On Tue, 15 Aug 2017 12:15:48 +0100
> > > Anthony PERARD <anthony.perard@citrix.com> 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.
> > > > 
> > > > Assign bsel=0 property only to the root bus on Xen as there is no
> > > > support in the Xen ACPI tables for a different value.  
> > > 
> > > looking at hw/acpi/pcihp.c and bsel usage there it looks like
> > > bsel property is owned by it and not by ACPI tables, so instead of
> > > shuffling it in acpi_setup(), how about moving bsel initialization
> > > to hw/acpi/pcihp.c and initialize it there unconditionally?
> > > 
> > > It could be as simple as moving acpi_set_pci_info()/acpi_set_bsel()
> > > there and calling it from acpi_pcihp_reset().
> > > 
> > > Then there won't be need for Xen specific branches, as root bus
> > > will have bsel set automatically which is sufficient for Xen and
> > > the rest of bsel-s (bridges) will be just unused by Xen,
> > > which could later extend its ACPI table implementation to utilize them.   
> > 
> > Later is exactly what I'd like to try to avoid.
> > Whoever wants acpi hotplug for bridges needs to get
> > the bsel info from qemu supplied acpi tables.
> 
> I'd prefer to have only one behavior in QEMU (on hw interface)
> side and let Xen to maintain their own ACPI tables  dealing
> with issues that arise from it since they insist on doing job twice.
> 
> The point is bsel is so embedded in HW part of impl.
> that it should be allocated/manged there, otherwise it leads
> to hacks where acpi_setup() is called but does partial init
> and then bails out to fix code pcihp.c that depend on it being run,
> pcihp.c (hw part) shouldn't depend on on ACPI tables generation
> (bios part).
> 
> Anyway if you insist on capping Xen, it probably could be done
> with comat machinery, something like this:
> 
> (where the 1st hunk should been there since, we've introduced
> "acpi-pci-hotplug-with-bridge-support")
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index c420a38..a55f022 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -273,7 +273,7 @@ static void pci_write(void *opaque, hwaddr addr, uint64_t data,
>                        addr, data);
>          break;
>      case PCI_SEL_BASE:
> -        s->hotplug_select = data;
> +        s->hotplug_select = s->legacy_piix ? 0 : data;
>          ACPI_PCIHP_DPRINTF("pcisel write %" HWADDR_PRIx " <== %" PRIu64 "\n",
>                        addr, data);
>      default:
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 22dbef6..81b8c3e 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -1117,6 +1117,13 @@ static void xenfv_machine_options(MachineClass *m)
>      m->max_cpus = HVM_MAX_VCPUS;
>      m->default_machine_opts = "accel=xen";
>      m->hot_add_cpu = pc_hot_add_cpu;
> +    SET_MACHINE_COMPAT(m,
> +        {\
> +            .driver   = "PIIX4_PM",\
> +            .property = "acpi-pci-hotplug-with-bridge-support",\
> +            .value    = "off",\

That property is actually already turned off for Xen, but this is done
in piix4_pm_init(). Also, having the property only for xenfv would not
be enought because we can use -machine pc,accel=xen.

Maybe we could use s->legacy_piix in acpi_pcihp_device_{,un}plug_cb() to
find out if the missing bsel property is an issue or not?


diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index c420a388ea..79b7ed9900 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -186,6 +186,9 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
     PCIDevice *pdev = PCI_DEVICE(dev);
     int slot = PCI_SLOT(pdev->devfn);
     int bsel = acpi_pcihp_get_bsel(pdev->bus);
+
+    if (bsel < 0 && s->legacy_piix)
+        bsel = 0;
     if (bsel < 0) {
         error_setg(errp, "Unsupported bus. Bus doesn't have property '"
                    ACPI_PCIHP_PROP_BSEL "' set");
@@ -209,6 +212,9 @@ void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
     PCIDevice *pdev = PCI_DEVICE(dev);
     int slot = PCI_SLOT(pdev->devfn);
     int bsel = acpi_pcihp_get_bsel(pdev->bus);
+
+    if (bsel < 0 && s->legacy_piix)
+        bsel = 0;
     if (bsel < 0) {
         error_setg(errp, "Unsupported bus. Bus doesn't have property '"
                    ACPI_PCIHP_PROP_BSEL "' set");

-- 
Anthony PERARD

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

[Igor Mammedov]

On Thu, 17 Aug 2017 15:23:27 +0100
Anthony PERARD <anthony.perard@citrix.com> wrote:

> On Wed, Aug 16, 2017 at 11:10:46AM +0200, Igor Mammedov wrote:
> > On Tue, 15 Aug 2017 22:24:08 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Tue, Aug 15, 2017 at 02:07:51PM +0200, Igor Mammedov wrote:  
> > > > On Tue, 15 Aug 2017 12:15:48 +0100
> > > > Anthony PERARD <anthony.perard@citrix.com> 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.
> > > > > 
> > > > > Assign bsel=0 property only to the root bus on Xen as there is no
> > > > > support in the Xen ACPI tables for a different value.    
> > > > 
> > > > looking at hw/acpi/pcihp.c and bsel usage there it looks like
> > > > bsel property is owned by it and not by ACPI tables, so instead of
> > > > shuffling it in acpi_setup(), how about moving bsel initialization
> > > > to hw/acpi/pcihp.c and initialize it there unconditionally?
> > > > 
> > > > It could be as simple as moving acpi_set_pci_info()/acpi_set_bsel()
> > > > there and calling it from acpi_pcihp_reset().
> > > > 
> > > > Then there won't be need for Xen specific branches, as root bus
> > > > will have bsel set automatically which is sufficient for Xen and
> > > > the rest of bsel-s (bridges) will be just unused by Xen,
> > > > which could later extend its ACPI table implementation to utilize them.     
> > > 
> > > Later is exactly what I'd like to try to avoid.
> > > Whoever wants acpi hotplug for bridges needs to get
> > > the bsel info from qemu supplied acpi tables.  
> > 
> > I'd prefer to have only one behavior in QEMU (on hw interface)
> > side and let Xen to maintain their own ACPI tables  dealing
> > with issues that arise from it since they insist on doing job twice.
> > 
> > The point is bsel is so embedded in HW part of impl.
> > that it should be allocated/manged there, otherwise it leads
> > to hacks where acpi_setup() is called but does partial init
> > and then bails out to fix code pcihp.c that depend on it being run,
> > pcihp.c (hw part) shouldn't depend on on ACPI tables generation
> > (bios part).
> > 
> > Anyway if you insist on capping Xen, it probably could be done
> > with comat machinery, something like this:
> > 
> > (where the 1st hunk should been there since, we've introduced
> > "acpi-pci-hotplug-with-bridge-support")
> > 
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index c420a38..a55f022 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -273,7 +273,7 @@ static void pci_write(void *opaque, hwaddr addr, uint64_t data,
> >                        addr, data);
> >          break;
> >      case PCI_SEL_BASE:
> > -        s->hotplug_select = data;
> > +        s->hotplug_select = s->legacy_piix ? 0 : data;
> >          ACPI_PCIHP_DPRINTF("pcisel write %" HWADDR_PRIx " <== %" PRIu64 "\n",
> >                        addr, data);
> >      default:
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 22dbef6..81b8c3e 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -1117,6 +1117,13 @@ static void xenfv_machine_options(MachineClass *m)
> >      m->max_cpus = HVM_MAX_VCPUS;
> >      m->default_machine_opts = "accel=xen";
> >      m->hot_add_cpu = pc_hot_add_cpu;
> > +    SET_MACHINE_COMPAT(m,
> > +        {\
> > +            .driver   = "PIIX4_PM",\
> > +            .property = "acpi-pci-hotplug-with-bridge-support",\
> > +            .value    = "off",\  
> 
> That property is actually already turned off for Xen, but this is done
> in piix4_pm_init(). Also, having the property only for xenfv would not
> be enought because we can use -machine pc,accel=xen.
> 
> Maybe we could use s->legacy_piix in acpi_pcihp_device_{,un}plug_cb() to
> find out if the missing bsel property is an issue or not?
> 
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index c420a388ea..79b7ed9900 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -186,6 +186,9 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
>      PCIDevice *pdev = PCI_DEVICE(dev);
>      int slot = PCI_SLOT(pdev->devfn);
>      int bsel = acpi_pcihp_get_bsel(pdev->bus);
> +
> +    if (bsel < 0 && s->legacy_piix)
> +        bsel = 0;
>      if (bsel < 0) {
>          error_setg(errp, "Unsupported bus. Bus doesn't have property '"
>                     ACPI_PCIHP_PROP_BSEL "' set");
> @@ -209,6 +212,9 @@ void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
>      PCIDevice *pdev = PCI_DEVICE(dev);
>      int slot = PCI_SLOT(pdev->devfn);
>      int bsel = acpi_pcihp_get_bsel(pdev->bus);
> +
> +    if (bsel < 0 && s->legacy_piix)
> +        bsel = 0;
>      if (bsel < 0) {
>          error_setg(errp, "Unsupported bus. Bus doesn't have property '"
>                     ACPI_PCIHP_PROP_BSEL "' set");
> 
I'm afraid that's not sufficient as bsel is used in may places withing pcihp.c,
safest bet is to use hunk 1 from above patch to limit legacy mode to root bus
and call acpi_set_pci_info() from acpi_pcihp_reset() once
to keep current behavior where bsel is set only once and not re-set on reset.

Then hotplug code in pcihp.c will have bsel property in place as it expects
and won't depend on APCI tables init code path.