From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47401) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZpXOg-000556-72 for qemu-devel@nongnu.org; Fri, 23 Oct 2015 04:08:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZpXOb-0003fF-Hb for qemu-devel@nongnu.org; Fri, 23 Oct 2015 04:08:22 -0400 Received: from [59.151.112.132] (port=30200 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZpXOa-0003bO-Hg for qemu-devel@nongnu.org; Fri, 23 Oct 2015 04:08:17 -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> <5629B401.7030707@cn.fujitsu.com> <20151023094525-mutt-send-email-mst@redhat.com> From: Cao jin Message-ID: <5629EC4F.5070108@cn.fujitsu.com> Date: Fri, 23 Oct 2015 16:14:07 +0800 MIME-Version: 1.0 In-Reply-To: <20151023094525-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 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 >>>> --- >>>> 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? 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? > >>> >>>> + 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