From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34405) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZpTe6-0007AO-If for qemu-devel@nongnu.org; Fri, 23 Oct 2015 00:08:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZpTe3-0007xs-AU for qemu-devel@nongnu.org; Fri, 23 Oct 2015 00:08:02 -0400 Received: from [59.151.112.132] (port=32146 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZpTe2-0007vc-CD for qemu-devel@nongnu.org; Fri, 23 Oct 2015 00:07:59 -0400 References: <1445515072-7968-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1445515072-7968-3-git-send-email-caoj.fnst@cn.fujitsu.com> <20151022171959-mutt-send-email-mst@redhat.com> From: Cao jin Message-ID: <5629B401.7030707@cn.fujitsu.com> Date: Fri, 23 Oct 2015 12:13:53 +0800 MIME-Version: 1.0 In-Reply-To: <20151022171959-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 2/2] enable multi-function hot-add List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: izumi.taku@jp.fujitsu.com, alex.williamson@redhat.com, qemu-devel@nongnu.org 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 >> --- >> 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? > >> + 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