| Message ID | 1493323148-32461-1-git-send-email-brogers@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <brogers@suse.com> > --- > 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.
>>> On 4/27/2017 at 03:09 PM, Stefano Stabellini <sstabellini@kernel.org> 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 <brogers@suse.com> >> --- >> 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.
On Thu, 27 Apr 2017, Bruce Rogers wrote: > >>> On 4/27/2017 at 03:09 PM, Stefano Stabellini <sstabellini@kernel.org> 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 <brogers@suse.com> > >> --- > >> 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.
On Thu, 27 Apr 2017 14:09:11 -0700 (PDT) Stefano Stabellini <sstabellini@kernel.org> 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 <brogers@suse.com> > > --- > > 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? it's error condition if acpi hotplug is in use, so we should keep it
On Thu, 27 Apr 2017 13:59:08 -0600 Bruce Rogers <brogers@suse.com> wrote: > Commit f0c9d64a exposed the issue that with a xenfv machine using > pci passthrough, acpi cpi hotplug code was being executed by mistake. s/cpi/pci/ > 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. it would be better to put actual error here. > > Signed-off-by: Bruce Rogers <brogers@suse.com> with above fixed up Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > 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);
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);
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 <brogers@suse.com> --- hw/acpi/piix4.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)