Re: [Qemu-stable] [PATCH] ACPI: don't call acpi_pcihp_device_plug_cb on xen

[Stefano Stabellini]

On Thu, 27 Apr 2017, Bruce Rogers wrote:
> >>> On 4/27/2017 at 03:09 PM, Stefano Stabellini <address@hidden> wrote: 
> > On Thu, 27 Apr 2017, Bruce Rogers wrote:
> >> Commit f0c9d64a exposed the issue that with a xenfv machine using
> >> pci passthrough, acpi cpi hotplug code was being executed by mistake.
> >> Guard calls to acpi_pcihp_device_plug_cb (and corresponding
> >> acpi_pcihp_device_unplug_cb) with a check for xen_enabled(). Without
> >> this check I am seeing an error that the bus doesn't have the
> >> acpi-pcihp-bsel property set.
> >> 
> >> Signed-off-by: Bruce Rogers <address@hidden>
> >> ---
> >>  hw/acpi/piix4.c | 11 ++++++++---
> >>  1 file changed, 8 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> >> index a553a7e..c409374 100644
> >> --- a/hw/acpi/piix4.c
> >> +++ b/hw/acpi/piix4.c
> >> @@ -385,7 +385,10 @@ static void piix4_device_plug_cb(HotplugHandler 
> > *hotplug_dev,
> >>                                  dev, errp);
> >>          }
> >>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> >> -        acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, 
> > errp);
> >> +        if (!xen_enabled()) {
> >> +            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);
> >> @@ -408,8 +411,10 @@ 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)) {
> >> -        acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, 
> >> dev,
> >> -                                    errp);
> >> +        if (!xen_enabled()) {
> >> +            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);
> > 
> > Wouldn't it be better to simply turn the error_setg("Unsupported bus...)
> > in acpi_pcihp_device_plug_cb and acpi_pcihp_device_unplug_cb into debug
> > printfs? After all, from this description it is clear that they are not
> > an error conditions. PCI Passthough works correctly, right?
> > 
> > I realize that I should have suggested this earlier, sorry about that.
> 
> I don't think we want to be running through qemu acpi code any more than is
> necessary when using xen hvm.

Fair enough, but at the same time it is not good to disseminate if
(!xen_enabled()) around. I am happy either way, I'll let Michael and
Igor decide.