On Mon, 23 Aug 2021, Michael S. Tsirkin wrote:
> On Sat, Aug 21, 2021 at 08:35:35PM +0530, Ani Sinha wrote:
> > Bsel property of the pci bus indicates whether the bus supports acpi
> > hotplug.
> > We need to validate the presence of this property before performing any
> > hotplug
> > related callback operations. Currently validation of the existence of this
> > property was absent from acpi_pcihp_device_unplug_cb() function but is
> > present
> > in other hotplug/unplug callback functions. Hence, this change adds the
> > missing
> > check for the above function.
> >
> > Signed-off-by: Ani Sinha <a...@anisinha.ca>
>
> I queued this but I have a general question:
> are all these errors logged with LOG_GUEST_ERROR?
I do not think they are logged that way. These logs go to stderr which can
end up in the qemu guest specific log file when qemu is run daemonized.
That being said, other platforms, for example virtio-pci also seems to do
what we do. They use error_setg() as well under similar conditions.
> Because if not we have a security problem.
> I also note that bsel is an internal property,
yeah this I think is an issue. We can change the log so as to not say
anything about bsel. I will let Igor comment. I can send out a separate
patch to fix this.
> I am not sure we should be printing this to users,
> it might just confuse them.
>
> Same question for all the other places validating bsel.
>
> > ---
> > hw/acpi/pcihp.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index 0fd0c1d811..9982815a87 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -372,9 +372,15 @@ void acpi_pcihp_device_unplug_cb(HotplugHandler
> > *hotplug_dev, AcpiPciHpState *s,
> > DeviceState *dev, Error **errp)
> > {
> > PCIDevice *pdev = PCI_DEVICE(dev);
> > + int bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev));
> > +
> > + trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), bsel);
> >
> > - trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn),
> > - acpi_pcihp_get_bsel(pci_get_bus(pdev)));
> > + if (bsel < 0) {
> > + error_setg(errp, "Unsupported bus. Bus doesn't have property '"
> > + ACPI_PCIHP_PROP_BSEL "' set");
> > + return;
> > + }
> >
> > /*
> > * clean up acpi-index so it could reused by another device
> > --
> > 2.25.1
>
>
