From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46130) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZbjZW-0000Ol-TN for qemu-devel@nongnu.org; Tue, 15 Sep 2015 02:18:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZbjZS-0006Rt-TF for qemu-devel@nongnu.org; Tue, 15 Sep 2015 02:18:30 -0400 Received: from [59.151.112.132] (port=57252 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZbjZS-0006NY-85 for qemu-devel@nongnu.org; Tue, 15 Sep 2015 02:18:26 -0400 References: <1441887143-26756-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1441887143-26756-2-git-send-email-caoj.fnst@cn.fujitsu.com> <1441898967.20355.578.camel@redhat.com> From: Cao jin Message-ID: <55F7B944.1090108@cn.fujitsu.com> Date: Tue, 15 Sep 2015 14:23:00 +0800 MIME-Version: 1.0 In-Reply-To: <1441898967.20355.578.camel@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] PCI-e device multi-function hot-add support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: pbonzini@redhat.com, qemu-devel@nongnu.org Hi, Alex, Thanks very much for your quick review and I am sorry for my late response. On 09/10/2015 11:29 PM, Alex Williamson wrote: > On Thu, 2015-09-10 at 20:12 +0800, Cao jin wrote: >> Enable PCI-e 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/pcie.c | 22 +++++++++++++--------- >> 1 file changed, 13 insertions(+), 9 deletions(-) >> >> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >> index 6e28985..61ebefd 100644 >> --- a/hw/pci/pcie.c >> +++ b/hw/pci/pcie.c >> @@ -249,16 +249,20 @@ 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. >> */ >> - 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_dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) && >> + PCI_FUNC(pci_dev->devfn) > 0) { > > The PCI spec is actually not entirely clear about whether each function > in a multifunction device needs to have the multifunction bit set or if > only function 0 needs to set it. From a discovery perspective, a kernel > should only scan for function 0 and only scan non-zero funcions if > function 0 reports the multifunction bit set. Therefore, it doesn't > particularly matter what non-zero functions report for the multifunction > bit. QEMU allows either interpretation (see comment in > pci_init_multifunction), so this appears to be a new requirement that > breaks assumptions elsewhere. Thanks, > > Alex > Yes, PCI Spec does not describe it clearly. The if() here, is based on the condition like following: (qemu) device_add e1000,bus=pb1,id=net1,addr=00.1 (qemu) device_add e1000,bus=pb1,id=net0,addr=00.0 PCI: 0.0 indicates single function, but 0.1 is already populated. Now, we can see the net1 via "info pci" while can`t see net1 in guest via "lspci -t", so I think maybe it is not good condition, but no big deal, so I use the if() to prevent the condition. will remove the if() in next version. >> + error_setg(errp, "single function device, function number must be 0," >> + "but got %d", PCI_FUNC(pci_dev->devfn)); >> + } else if (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); >> + } >> } >> >> void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev, > > > > . > -- Yours Sincerely, Cao Jin