From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44508) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alX2l-0003Ff-2w for qemu-devel@nongnu.org; Thu, 31 Mar 2016 03:29:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1alX2h-0007Wi-RY for qemu-devel@nongnu.org; Thu, 31 Mar 2016 03:29:27 -0400 Received: from [59.151.112.132] (port=3956 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alX2g-0007W4-22 for qemu-devel@nongnu.org; Thu, 31 Mar 2016 03:29:23 -0400 Message-ID: <56FCD081.9070600@cn.fujitsu.com> Date: Thu, 31 Mar 2016 15:23:45 +0800 From: Chen Fan MIME-Version: 1.0 References: <1458727927-15082-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1458727927-15082-8-git-send-email-caoj.fnst@cn.fujitsu.com> <20160324165433.4806cba1@t450s.home> <20160327121910.GA26438@redhat.com> In-Reply-To: <20160327121910.GA26438@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [patch v5 07/12] pci: add a pci_function_is_valid callback to check function if valid List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" , Alex Williamson Cc: izumi.taku@jp.fujitsu.com, Cao jin , qemu-devel@nongnu.org On 03/27/2016 08:19 PM, Michael S. Tsirkin wrote: > On Thu, Mar 24, 2016 at 04:54:33PM -0600, Alex Williamson wrote: >> On Wed, 23 Mar 2016 18:12:02 +0800 >> Cao jin wrote: >> >>> From: Chen Fan >>> >>> PCI hotplug requires that function 0 is added last to close the >>> slot. Since vfio supporting AER, we require that the VM bus >>> contains the same set of devices as the host bus to support AER, >>> we can perform an AER validation test whenever a function 0 in >>> the VM is hot-added. >>> >>> Signed-off-by: Chen Fan >>> --- >>> hw/pci/pci.c | 32 ++++++++++++++++++++++++++++++++ >>> include/hw/pci/pci.h | 1 + >>> 2 files changed, 33 insertions(+) >> >> This would of course need Michael's ack. >> >> Michael, we can't do this in vfio-pci code because we can't guarantee >> that function 0 is a vfio-pci device. That would allow users to bypass >> our configuration requirements by putting any random non-vfio-pci >> device at function 0 of a hot-added multifunction device. > I got that. What I don't understand is how is the non hotplug > variant handled, given that is_valid_func is not called > in that case. for non-hotplug case, we used machine_done_notifier to check the bus reset functionality. because we can't ensure the function 0 was initialized at last when cold boot up. > > Also, why does it scan all devices on bus, not all functions > of the device? I can fix this by testing the bridge whether support ARI. Thanks. Chen > >>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >>> index e67664d..2a5291a 100644 >>> --- a/hw/pci/pci.c >>> +++ b/hw/pci/pci.c >>> @@ -1840,6 +1840,22 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn) >>> return bus->devices[devfn]; >>> } >>> >>> +static void pci_function_is_valid(PCIBus *bus, PCIDevice *d, void *opaque) >>> +{ >>> + PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(d); >>> + Error **errp = opaque; >>> + >>> + if (*errp) { >>> + return; >>> + } >>> + >>> + if (!pc->is_valid_func) { >>> + return; >>> + } >>> + >>> + pc->is_valid_func(d, errp); >>> +} >>> + >>> static void pci_qdev_realize(DeviceState *qdev, Error **errp) >>> { >>> PCIDevice *pci_dev = (PCIDevice *)qdev; >>> @@ -1882,6 +1898,22 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp) >>> pci_qdev_unrealize(DEVICE(pci_dev), NULL); >>> return; >>> } >>> + >>> + /* >>> + * If the function number is 0, indicate the closure of the slot. >>> + * then we get the chance to check all functions on same device >>> + * if valid. >>> + */ >>> + if (DEVICE(pci_dev)->hotplugged && >>> + pci_get_function_0(pci_dev) == pci_dev) { >>> + pci_for_each_device(bus, pci_bus_num(bus), >>> + pci_function_is_valid, &local_err); >>> + if (local_err) { >>> + error_propagate(errp, local_err); >>> + pci_qdev_unrealize(DEVICE(pci_dev), NULL); >>> + return; >>> + } >>> + } >>> } >>> >>> static void pci_default_realize(PCIDevice *dev, Error **errp) >>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h >>> index 0be07c8..4a2f7d4 100644 >>> --- a/include/hw/pci/pci.h >>> +++ b/include/hw/pci/pci.h >>> @@ -190,6 +190,7 @@ typedef struct PCIDeviceClass { >>> >>> void (*realize)(PCIDevice *dev, Error **errp); >>> int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */ >>> + void (*is_valid_func)(PCIDevice *dev, Error **errp); >>> PCIUnregisterFunc *exit; >>> PCIConfigReadFunc *config_read; >>> PCIConfigWriteFunc *config_write; > > . >