[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 2/2] enable multi-function hot-add
|
From: |
Michael S. Tsirkin |
|
Subject: |
Re: [Qemu-devel] [PATCH v4 2/2] enable multi-function hot-add |
|
Date: |
Fri, 23 Oct 2015 16:41:41 +0300 |
On Fri, Oct 23, 2015 at 04:14:07PM +0800, Cao jin wrote:
>
>
> On 10/23/2015 02:50 PM, Michael S. Tsirkin wrote:
> >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 <address@hidden>
> >>>>---
> >>>> 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.
> >
> Thanks michael. if I understand right, ARI device has a upstream port on the
> device, while on-ARI device doesn`t have?
That's what it says, isn't it?
> But in Figure 6-13(Example System Topology with ARI Devices) in the PCIe
> spec, don`t see any notation of upstream port on ARI Device X & Y, so maybe
> figure 6-13 is incomplete?
You can see that they are connected to downstream or root ports.
> >
> >>>
> >>>>+ 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
> >.
> >
>
> --
> Yours Sincerely,
>
> Cao Jin