Hi, Alex
On 10/13/2015 11:27 PM, Alex Williamson wrote:
On Tue, 2015-10-13 at 16:41 +0800, Cao jin wrote:
In case user regret when hot-adding multi-function, should roll back,
device_del the function added but not exposed to the guest.
As Michael suggests, this patch should come first, before we actually
enable multi-function hot-add.
OK.
Signed-off-by: Cao jin <[email protected]>
---
hw/pci/pci_host.c | 6 +++++-
hw/pci/pcie.c | 22 +++++++++++++++++-----
2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 3e26f92..35e5cf3 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -20,6 +20,7 @@
#include "hw/pci/pci.h"
#include "hw/pci/pci_host.h"
+#include "hw/pci/pci_bus.h"
#include "trace.h"
/* debug PCI */
@@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val,
int len)
uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
{
PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
+ PCIDevice *f0 = NULL;
uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
uint32_t val;
+ uint8_t slot = (addr >> 11) & 0x1F;
- if (!pci_dev) {
+ f0 = s->devices[PCI_DEVFN(slot, 0)];
+ if (!pci_dev || (!f0 && pci_dev)) {
This uses a lot more variables and operations than it needs to:
if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
Ok. variables is intended to make the line shorter.
Shouldn't we do the same on pci_data_write()? A well behaved guest
won't blindly write to config space, but not all guests are well
behaved.
Yup, agree. I missed the consideration of bad behavior. I thought anyone
use the device should read the vendor ID first(good behavior), then do
anything he/she want. Thanks for reminding
Comments in the code would be nice here to explain that non-zero
functions are only exposed when function zero is present, allowing
direct removal of unexposed devices.
OK
I imagine that due to qemu locking that we don't have a race here, but
note that devices[] is populated early in the core pci realize function,
prior to the device initialize function, and there are any number of
reasons that failure could still occur, which would create a window
where the function is accessible. I doubt this is an issue, but simply
note it for completeness.
Ok, will consider the "function access window" condition, to see what I
can do with it
return ~0x0;
}
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 89bf61b..58d2153 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler
*hotplug_dev, DeviceState *dev,
}
}
+static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
+{
+ object_unparent(OBJECT(dev));
+}
+
void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
uint8_t *exp_cap;
+ PCIDevice *pci_dev = PCI_DEVICE(dev);
+ PCIBus *bus = pci_dev->bus;
pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap,
errp);
+ /* In case user regret when hot-adding multi function, remove the function
+ * that is unexposed to guest individually, without interaction with guest.
+ */
+ if (PCI_FUNC(pci_dev->devfn) > 0 &&
+ bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) {
Similarly,
if (PCI_FUNC(pci_dev->devfn) &&
!bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
Ok
+ pcie_unplug_device(bus, pci_dev, NULL);
+
+ return;
+ }
+
pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
}
@@ -378,11 +395,6 @@ void pcie_cap_slot_reset(PCIDevice *dev)
hotplug_event_update_event_status(dev);
}
-static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
-{
- object_unparent(OBJECT(dev));
-}
-
void pcie_cap_slot_write_config(PCIDevice *dev,
uint32_t addr, uint32_t val, int len)
{
.
--
Yours Sincerely,
Cao Jin
