|
From: | Cao jin |
Subject: | Re: [Qemu-devel] [PATCH v4 2/2] enable multi-function hot-add |
Date: | Fri, 23 Oct 2015 22:12:15 +0800 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 |
On 10/23/2015 09:41 PM, Michael S. Tsirkin wrote:
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?
Yup, seems it does is. it redefined the concept of device in my mind, because never seen a figure shows that there is a port on a ARI device, like the figure of "ARI Device X" I draw below
------------------------------- | _______________ | | |_upstream port_| | | | | | | | | F0 F1 F2 ... .. F255 | ------------------------------|Thanks for your time for this question, I will go on dealing with the other comments.
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.
-- Yours Sincerely, Cao Jin
[Prev in Thread] | Current Thread | [Next in Thread] |