From patchwork Wed Feb 3 08:54:01 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chen Fan X-Patchwork-Id: 8199351 Return-Path: X-Original-To: patchwork-qemu-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 71A619F37A for ; Wed, 3 Feb 2016 08:59:18 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 904F920251 for ; Wed, 3 Feb 2016 08:59:17 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 6505B2022D for ; Wed, 3 Feb 2016 08:59:16 +0000 (UTC) Received: from localhost ([::1]:33373 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQtHP-0000nJ-Fy for patchwork-qemu-devel@patchwork.kernel.org; Wed, 03 Feb 2016 03:59:15 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47938) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQtHH-0000mu-82 for qemu-devel@nongnu.org; Wed, 03 Feb 2016 03:59:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQtHC-0008OI-JQ for qemu-devel@nongnu.org; Wed, 03 Feb 2016 03:59:07 -0500 Received: from [59.151.112.132] (port=55775 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQtHB-0008MK-Mu for qemu-devel@nongnu.org; Wed, 03 Feb 2016 03:59:02 -0500 X-IronPort-AV: E=Sophos;i="5.20,346,1444665600"; d="scan'208";a="3272797" Received: from unknown (HELO cn.fujitsu.com) ([10.167.33.5]) by heian.cn.fujitsu.com with ESMTP; 03 Feb 2016 16:58:51 +0800 Received: from G08CNEXCHPEKD03.g08.fujitsu.local (unknown [10.167.33.85]) by cn.fujitsu.com (Postfix) with ESMTP id DC98441896E9; Wed, 3 Feb 2016 16:57:56 +0800 (CST) Received: from [10.167.226.78] (10.167.226.78) by G08CNEXCHPEKD03.g08.fujitsu.local (10.167.33.89) with Microsoft SMTP Server id 14.3.181.6; Wed, 3 Feb 2016 17:02:48 +0800 Message-ID: <56B1C029.3080901@cn.fujitsu.com> Date: Wed, 3 Feb 2016 16:54:01 +0800 From: Chen Fan User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: "Michael S. Tsirkin" , Cao jin References: <20160116202201-mutt-send-email-mst@redhat.com> In-Reply-To: <20160116202201-mutt-send-email-mst@redhat.com> X-Originating-IP: [10.167.226.78] X-yoursite-MailScanner-ID: DC98441896E9.A5F02 X-yoursite-MailScanner: Found to be clean X-yoursite-MailScanner-From: chen.fan.fnst@cn.fujitsu.com X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 59.151.112.132 Cc: izumi.taku@jp.fujitsu.com, alex.williamson@redhat.com, qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [PATCH v16 00/14] vfio-pci: pass the aer error to guest X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 01/17/2016 02:34 AM, Michael S. Tsirkin wrote: > On Tue, Jan 12, 2016 at 10:43:01AM +0800, Cao jin wrote: >> From: Chen Fan >> >> For now, for vfio pci passthough devices when qemu receives >> an error from host aer report, currentlly just terminate the guest, >> but usually user want to know what error occurred but stopping the >> guest, so this patches add aer capability support for vfio device, >> and pass the error to guest, and have guest driver to recover >> from the error. > I would like to see a version of this patchset that doesn't > depend on pci core changes. > I think that if you make this simplifying assumption: > > - all devices on same bus in guest are on same bus in host > > then you can handle both reset and hotplug simply in function 0 > since it will belong to vfio. > > So we can have a version without pci core changes that simply assumes > this, and things will just work. > > > Now, if we wanted to enforce this limitation, I think the > cleanest way would be to add a callback in struct PCIDevice: > > bool is_valid_function(PCIDevice *newfunction) > > and call it as each function is added. > This way aer function can validate that each function > added shares the same bus. > And this way issues will be detected directly and not when > function 0 is added. > > I would prefer this validation code to be a patch on top so we can merge > the functionality directly and avoid blocking it while we figure out the > best api to validate things. > > I don't see why making guest topology match host would > ever be a problem, but if it's required to support > configurations where these differ, I'd like to see > an attempt to address that be split out, after aer > is supported. Hi Michael, Just think about this more, I think we also should check the vfio devices whether on the same bus at the time of function 0 is added. because we don't know the affected devices by a bus reset have already all been assigned to VM. for example, the multi-function's hotplug. devices on same bus in host are added to VM one by one. when we test one device, we haven't yet added the other devices. so I think the patch should like below. then we could add a vfio_is_valid_function in vfio to test each device whether the affected devices on the same bus. Thanks, Chen > > > >> v15-v16: >> 10/14, 11/14 are new to introduce a reset sequence id to specify the >> vfio devices has been reset for that reset. other patches aren't modified. >> >> v14-v15: >> 1. add device hot reset callback >> 2. add bus_in_reset for vfio device to avoid multi do host bus reset >> >> v13-v14: >> 1. for multifunction device, requiring all functions enable AER.(9/13) >> 2. due to all affected functions receive error signal, ignore no >> error occurred function. (12/13) >> >> v12-v13: >> 1. since support multifuncion hotplug, here add callback to enable aer. >> 2. add pci device pre+post reset for aer host reset. >> >> Chen Fan (14): >> vfio: extract vfio_get_hot_reset_info as a single function >> vfio: squeeze out vfio_pci_do_hot_reset for support bus reset >> pcie: modify the capability size assert >> vfio: make the 4 bytes aligned for capability size >> vfio: add pcie extanded capability support >> aer: impove pcie_aer_init to support vfio device >> vfio: add aer support for vfio device >> vfio: add check host bus reset is support or not >> add check reset mechanism when hotplug vfio device >> pci: introduce pci bus pre reset >> vfio: introduce last reset sequence id >> pcie_aer: expose pcie_aer_msg() interface >> vfio-pci: pass the aer error to guest >> vfio: add 'aer' property to expose aercap >> >> hw/pci-bridge/ioh3420.c | 2 +- >> hw/pci-bridge/xio3130_downstream.c | 2 +- >> hw/pci-bridge/xio3130_upstream.c | 2 +- >> hw/pci/pci.c | 42 +++ >> hw/pci/pci_bridge.c | 3 + >> hw/pci/pcie.c | 2 +- >> hw/pci/pcie_aer.c | 6 +- >> hw/vfio/pci.c | 616 +++++++++++++++++++++++++++++++++---- >> hw/vfio/pci.h | 9 + >> include/hw/pci/pci.h | 1 + >> include/hw/pci/pci_bus.h | 8 + >> include/hw/pci/pcie_aer.h | 3 +- >> 12 files changed, 624 insertions(+), 72 deletions(-) >> >> -- >> 1.9.3 >> >> > > . > diff --git a/hw/pci/pci.c b/hw/pci/pci.c index d940f79..7163b56 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -1836,6 +1836,38 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn) return bus->devices[devfn]; } +static int pci_bus_check_devices(PCIBus *bus) +{ + PCIDeviceClass *pc; + int i, ret = 0; + + for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) { + if (!bus->devices[i]) { + continue; + } + + pc = PCI_DEVICE_GET_CLASS(bus->devices[i]); + if (!pc->is_valid_func) { + continue; + } + + ret = pc->is_valid_func(bus->devices[i], bus); + if (!ret) { + return -1; + } + } + return 0; +} + +static bool pci_is_valid_function(PCIDevice *pdev, PCIBus *bus) +{ + if (pdev->bus == bus) { + return true; + } + + return false; +} + static void pci_qdev_realize(DeviceState *qdev, Error **errp) { PCIDevice *pci_dev = (PCIDevice *)qdev; @@ -1878,6 +1910,14 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp) pci_qdev_unrealize(DEVICE(pci_dev), NULL); return; } + + if (DEVICE(pci_dev)->hotplugged && + pci_get_function_0(pci_dev) == pci_dev && + pci_bus_check_devices(bus)) { + error_setg(errp, "failed to hotplug function 0"); + pci_qdev_unrealize(DEVICE(pci_dev), NULL); + return; + } } static void pci_default_realize(PCIDevice *dev, Error **errp) @@ -2390,6 +2430,7 @@ static void pci_device_class_init(ObjectClass *klass, void *data) k->bus_type = TYPE_PCI_BUS; k->props = pci_props; pc->realize = pci_default_realize; + pc->is_valid_func = pci_is_valid_function; } AddressSpace *pci_device_iommu_address_space(PCIDevice *dev) diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index dedf277..a89580f 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -191,6 +191,7 @@ typedef struct PCIDeviceClass { void (*realize)(PCIDevice *dev, Error **errp); int (*init)(PCIDevice *dev);/* TODO convert to realize() and remove */ + bool (*is_valid_func)(PCIDevice *dev, PCIBus *bus); PCIUnregisterFunc *exit; PCIConfigReadFunc *config_read; PCIConfigWriteFunc *config_write;