[for-2.10,v2,1/2] hw/acpi: Call acpi_set_pci_info when no ACPI tables needed

Commit Message

Anthony PERARD Aug. 17, 2017, 2:23 p.m. UTC

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" <[e-mail]> 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 <[e-mail]> 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?

Comments

Igor Mammedov Aug. 17, 2017, 3:11 p.m. UTC | #1

On Thu, 17 Aug 2017 15:23:27 +0100
Anthony PERARD <[e-mail]> 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" <[e-mail]> 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 <[e-mail]> 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.

Patch

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");