On Fri, Oct 23, 2015 at 12:13:53PM +0800, Cao jin wrote:
> Hi Michael
>
> On 10/22/2015 10:37 PM, Michael S. Tsirkin wrote:
> >On Thu, Oct 22, 2015 at 07:57:52PM +0800, Cao jin wrote:
> >>Enable pcie device multifunction hot, just ensure the function 0
> >>added last, then driver will got the notification to scan all the
> >>function in the slot.
> >>
> >>Signed-off-by: Cao jin <caoj.f...@cn.fujitsu.com>
> >>---
> >> hw/pci/pci.c | 29 +++++++++++++++++++++++++++++
> >> hw/pci/pci_host.c | 11 +++++++++--
> >> hw/pci/pcie.c | 18 +++++++++---------
> >> include/hw/pci/pci.h | 1 +
> >> 4 files changed, 48 insertions(+), 11 deletions(-)
> >>
> >>diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>index b0bf540..c068248 100644
> >>--- a/hw/pci/pci.c
> >>+++ b/hw/pci/pci.c
> >>@@ -847,6 +847,7 @@ static PCIDevice *do_pci_register_device(PCIDevice
> >>*pci_dev, PCIBus *bus,
> >> PCIConfigWriteFunc *config_write = pc->config_write;
> >> Error *local_err = NULL;
> >> AddressSpace *dma_as;
> >>+ DeviceState *dev = DEVICE(pci_dev);
> >>
> >> if (devfn < 0) {
> >> for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
> >>@@ -864,6 +865,16 @@ static PCIDevice *do_pci_register_device(PCIDevice
> >>*pci_dev, PCIBus *bus,
> >> PCI_SLOT(devfn), PCI_FUNC(devfn), name,
> >> bus->devices[devfn]->name);
> >> return NULL;
> >>+ } else if (dev->hotplugged &&
> >>+ ((!pc->is_express &&
> >>bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]) ||
> >>+ (pc->is_express && bus->devices[0]))) {
> >
> >So let's change pci_is_function_0 to pci_get_function_0 and reuse here?
>
> ok, will modify it and all the following comment.
>
> >
> >>+ error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
> >>+ " new func %s cannot be exposed to guest.",
> >>+ PCI_SLOT(devfn),
> >>+ bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]->name,
> >>+ name);
> >>+
> >>+ return NULL;
> >> }
> >>
> >> pci_dev->bus = bus;
> >>@@ -2454,6 +2465,24 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range)
> >> pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> >> }
> >>
> >>+/* ARI device function number range is 0-255, means has only 1 function0;
> >>+ * while PCI device has 1 function0 in each slot(up to 32 slot) */
> >>+bool pci_is_function_0(PCIDevice *pci_dev)
> >>+{
> >>+ PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
> >>+ bool is_func0 = false;
> >>+
> >>+ if (pc->is_express) {
> >
> >
> >This is not what the spec says. It says:
> >devices connected to an upstream port.
> >
> Yes, I am wrong here. I got confused about the ARI device definition in
> spec:"a device associated with an Upstream Port". Because as I understand,
> ARI device is a EP immediately below a ARI downstream port, just as Figure
> 6-13(Example System Topology with ARI Devices) shows in the spec, but where
> is upstream port in the definition come from, what the relationship between
> upstream port and a ARI device?
See Terms and Acronyms:
The Port on a
component that contains only Endpoint or Bridge Functions is an Upstream
Port.
> >
> >>+ if (!pci_dev->devfn)
> >>+ is_func0 = true;
> >
> >Just do return !pci_dev->devfn here.
> >
> >>+ } else {
> >>+ if(!PCI_FUNC(pci_dev->devfn))
> >>+ is_func0 = true;
> >>+ }
> >>+
> >>+ return is_func0;
> >>+}
> >>+
> >> static const TypeInfo pci_device_type_info = {
> >> .name = TYPE_PCI_DEVICE,
> >> .parent = TYPE_DEVICE,
> >>diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> >>index 3e26f92..40087c9 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 */
> >>@@ -75,7 +76,10 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t
> >>val, int len)
> >> PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
> >> uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
> >>
> >>- if (!pci_dev) {
> >>+ /* non-zero functions are only exposed when function 0 is present,
> >>+ * allowing direct removal of unexposed functions.
> >>+ */
> >>+ if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
> >
> >Reuse pci_get_function_0 here too?
> >
> >> return;
> >> }
> >>
> >>@@ -91,7 +95,10 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
> >> uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
> >> uint32_t val;
> >>
> >>- if (!pci_dev) {
> >>+ /* non-zero functions are only exposed when function 0 is present,
> >>+ * allowing direct removal of unexposed functions.
> >>+ */
> >>+ if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
> >
> >And here?
> >
> >> return ~0x0;
> >> }
> >>
> >>diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> >>index b1adeaf..d0a8006 100644
> >>--- a/hw/pci/pcie.c
> >>+++ b/hw/pci/pcie.c
> >>@@ -249,16 +249,16 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler
> >>*hotplug_dev, DeviceState *dev,
> >> return;
> >> }
> >>
> >>- /* TODO: multifunction hot-plug.
> >>- * Right now, only a device of function = 0 is allowed to be
> >>- * hot plugged/unplugged.
> >>+ /* To enable multifunction hot-plug, we just ensure the function
> >>+ * 0 added last. Until function 0 added, we set the sltsta and
> >>+ * inform OS via event notification.
> >
> >Should be "when function 0 is added".
> >
> >> */
> >>- assert(PCI_FUNC(pci_dev->devfn) == 0);
> >>-
> >>- pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> >>- PCI_EXP_SLTSTA_PDS);
> >>- pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
> >>- PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
> >>+ if (pci_is_function_0(pci_dev)) {
> >>+ pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> >>+ PCI_EXP_SLTSTA_PDS);
> >>+ pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
> >>+ PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
> >>+ }
> >> }
> >>
> >> static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
> >>diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> >>index f5e7fd8..2820cfd 100644
> >>--- a/include/hw/pci/pci.h
> >>+++ b/include/hw/pci/pci.h
> >>@@ -397,6 +397,7 @@ void pci_for_each_bus_depth_first(PCIBus *bus,
> >> void *(*begin)(PCIBus *bus, void
> >> *parent_state),
> >> void (*end)(PCIBus *bus, void *state),
> >> void *parent_state);
> >>+bool pci_is_function_0(PCIDevice *pci_dev);
> >>
> >> /* Use this wrapper when specific scan order is not required. */
> >> static inline
> >>--
> >>2.1.0
> >.
> >
>
> --
> Yours Sincerely,
>
> Cao Jin
