From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54837) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zpcz3-00063W-Tj for qemu-devel@nongnu.org; Fri, 23 Oct 2015 10:06:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zpcyz-0003e7-6a for qemu-devel@nongnu.org; Fri, 23 Oct 2015 10:06:17 -0400 Received: from [59.151.112.132] (port=29425 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zpcyx-0003bz-AB for qemu-devel@nongnu.org; Fri, 23 Oct 2015 10:06:13 -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> <5629EC4F.5070108@cn.fujitsu.com> <20151023163952-mutt-send-email-mst@redhat.com> From: Cao jin Message-ID: <562A403F.104@cn.fujitsu.com> Date: Fri, 23 Oct 2015 22:12:15 +0800 MIME-Version: 1.0 In-Reply-To: <20151023163952-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 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 >>>>>> --- >>>>>> 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