From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39421) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zx51o-0006iM-HR for qemu-devel@nongnu.org; Thu, 12 Nov 2015 22:27:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zx51j-0002SW-SX for qemu-devel@nongnu.org; Thu, 12 Nov 2015 22:27:56 -0500 Received: from [59.151.112.132] (port=6111 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zx51i-0002Qf-DN for qemu-devel@nongnu.org; Thu, 12 Nov 2015 22:27:51 -0500 References: <7e222e2840fe58fe26d3bd73f626c1da029ca981.1447231392.git.chen.fan.fnst@cn.fujitsu.com> <20151112134713-mutt-send-email-mst@redhat.com> From: Cao jin Message-ID: <564558CA.6030102@cn.fujitsu.com> Date: Fri, 13 Nov 2015 11:28:10 +0800 MIME-Version: 1.0 In-Reply-To: <20151112134713-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v13 09/13] add check reset mechanism when hotplug vfio device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Chen Fan , alex.williamson@redhat.com, qemu-devel@nongnu.org On 11/12/2015 07:51 PM, Michael S. Tsirkin wrote: > On Wed, Nov 11, 2015 at 06:34:27PM +0800, Cao jin wrote: >> From: Chen Fan >> >> Since we support multi-function hotplug. the function 0 indicate >> the closure of the slot, so we have the chance to do the check. >> >> Signed-off-by: Chen Fan >> --- >> hw/pci/pci.c | 29 +++++++++++++++++++++++++++++ >> hw/vfio/pci.c | 19 +++++++++++++++++++ >> hw/vfio/pci.h | 2 ++ >> include/hw/pci/pci_bus.h | 5 +++++ >> 4 files changed, 55 insertions(+) >> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index 168b9cc..f6ca6ef 100644 >> --- a/hw/pci/pci.c >> +++ b/hw/pci/pci.c >> @@ -81,6 +81,7 @@ static void pci_bus_realize(BusState *qbus, Error **er= rp) >> PCIBus *bus =3D PCI_BUS(qbus); >> >> vmstate_register(NULL, -1, &vmstate_pcibus, bus); >> + notifier_with_return_list_init(&bus->hotplug_notifiers); >> } >> >> static void pci_bus_unrealize(BusState *qbus, Error **errp) >> @@ -1835,6 +1836,22 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_n= um, uint8_t devfn) >> return bus->devices[devfn]; >> } >> >> +void pci_bus_add_hotplug_notifier(PCIBus *bus, NotifierWithReturn *noti= fy) >> +{ >> + notifier_with_return_list_add(&bus->hotplug_notifiers, notify); >> +} >> + >> +void pci_bus_remove_hotplug_notifier(NotifierWithReturn *notifier) >> +{ >> + notifier_with_return_remove(notifier); >> +} >> + >> +static int pci_bus_hotplug_notifier(PCIBus *bus, void *opaque) >> +{ >> + return notifier_with_return_list_notify(&bus->hotplug_notifiers, >> + opaque); >> +} >> + >> static void pci_qdev_realize(DeviceState *qdev, Error **errp) >> { >> PCIDevice *pci_dev =3D (PCIDevice *)qdev; >> @@ -1877,6 +1894,18 @@ static void pci_qdev_realize(DeviceState *qdev, E= rror **errp) >> pci_qdev_unrealize(DEVICE(pci_dev), NULL); >> return; >> } >> + >> + /* >> + * If the function is func 0, indicate the closure of the slot. >> + * signal the callback. >> + */ >> + if (DEVICE(pci_dev)->hotplugged && >> + pci_get_function_0(pci_dev) =3D=3D pci_dev && >> + pci_bus_hotplug_notifier(bus, pci_dev)) { >> + error_setg(errp, "failed to hotplug function 0"); >> + pci_qdev_unrealize(DEVICE(pci_dev), NULL); >> + return; >> + } > > I don't understand why this is required in pci core. > PCI Device is already constructed anyway. > Just do the checks and call unrealize in vfio. Because when do multi-function hotplug, the function 0 on the pcie bus=20 probably is not a vfio device. so we should trigger the check from pci=20 core. > I also don't see why you are tying this to hotplug. > I would check each function as it's added. > But that's a vfio thing, if both you and Alex think > it's a good idea, fine by me. The device is initialized one by one no matter it is cold plugged or=20 hot plugged, but for the vfio with aer that need to get depended devices=20 required by bus reset, so need to make sure the reset depended devices=20 are assigned to qemu, in vfio, there is a machine done callback to check=20 the bus reset for boot up, so it also should be done in hotplug=E3=80=82 it looks little complicated, Alex, any idea? > >> } >> >> static void pci_default_realize(PCIDevice *dev, Error **errp) >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 31ffd44..e619998 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -1990,6 +1990,19 @@ static int vfio_check_devices_host_bus_reset(void= ) >> return 0; >> } >> >> +static int vfio_check_bus_reset(NotifierWithReturn *n, void *opaque) >> +{ >> + VFIOPCIDevice *vdev =3D container_of(n, VFIOPCIDevice, hotplug_noti= fier); >> + PCIDevice *pci_dev =3D PCI_DEVICE(vdev); >> + PCIDevice *pci_func0 =3D opaque; >> + >> + if (pci_get_function_0(pci_dev) !=3D pci_func0) { >> + return 0; >> + } >> + >> + return vfio_check_host_bus_reset(vdev); >> +} >> + >> static int vfio_setup_aer(VFIOPCIDevice *vdev, uint8_t cap_ver, >> int pos, uint16_t size) >> { >> @@ -2044,6 +2057,9 @@ static int vfio_setup_aer(VFIOPCIDevice *vdev, uin= t8_t cap_ver, >> return ret; >> } >> >> + vdev->hotplug_notifier.notify =3D vfio_check_bus_reset; >> + pci_bus_add_hotplug_notifier(pdev->bus, &vdev->hotplug_notifier); >> + >> return 0; >> >> error: >> @@ -2919,6 +2935,9 @@ static void vfio_exitfn(PCIDevice *pdev) >> vfio_unregister_req_notifier(vdev); >> vfio_unregister_err_notifier(vdev); >> pci_device_set_intx_routing_notifier(&vdev->pdev, NULL); >> + if (vdev->features & VFIO_FEATURE_ENABLE_AER) { >> + pci_bus_remove_hotplug_notifier(&vdev->hotplug_notifier); >> + } >> vfio_disable_interrupts(vdev); >> if (vdev->intx.mmap_timer) { >> timer_free(vdev->intx.mmap_timer); >> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h >> index 59ae194..b385f07 100644 >> --- a/hw/vfio/pci.h >> +++ b/hw/vfio/pci.h >> @@ -142,6 +142,8 @@ typedef struct VFIOPCIDevice { >> bool no_kvm_intx; >> bool no_kvm_msi; >> bool no_kvm_msix; >> + >> + NotifierWithReturn hotplug_notifier; >> } VFIOPCIDevice; >> >> uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len)= ; >> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h >> index 403fec6..7812fa9 100644 >> --- a/include/hw/pci/pci_bus.h >> +++ b/include/hw/pci/pci_bus.h >> @@ -39,8 +39,13 @@ struct PCIBus { >> Keep a count of the number of devices with raised IRQs. */ >> int nirq; >> int *irq_count; >> + >> + NotifierWithReturnList hotplug_notifiers; >> }; >> >> +void pci_bus_add_hotplug_notifier(PCIBus *bus, NotifierWithReturn *noti= fy); >> +void pci_bus_remove_hotplug_notifier(NotifierWithReturn *notify); >> + >> typedef struct PCIBridgeWindows PCIBridgeWindows; >> >> /* >> -- >> 1.9.3 > . > --=20 Yours Sincerely, Cao Jin