From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55344) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YunTF-0006tg-Po for qemu-devel@nongnu.org; Tue, 19 May 2015 15:46:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YunTC-0002a8-J3 for qemu-devel@nongnu.org; Tue, 19 May 2015 15:46:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35423) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YunTC-0002a1-BF for qemu-devel@nongnu.org; Tue, 19 May 2015 15:46:30 -0400 Message-ID: <1432064082.11375.248.camel@redhat.com> From: Alex Williamson Date: Tue, 19 May 2015 13:34:42 -0600 In-Reply-To: <0a0a9cca04fd00560d356133f7b89512c567e5fa.1432008287.git.chen.fan.fnst@cn.fujitsu.com> References: <0a0a9cca04fd00560d356133f7b89512c567e5fa.1432008287.git.chen.fan.fnst@cn.fujitsu.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC v7 04/11] vfio: add check host bus reset is support or not List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chen Fan Cc: izumi.taku@jp.fujitsu.com, qemu-devel@nongnu.org On Tue, 2015-05-19 at 12:42 +0800, Chen Fan wrote: > when machine is done, we should check the all vfio devices > whether support host bus reset, then when need virtual secondary > bus reset, we should reset host bus. > > Signed-off-by: Chen Fan > --- > hw/vfio/pci.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 83 insertions(+) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 43869e9..ff639db 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -168,6 +168,7 @@ typedef struct VFIOPCIDevice { > bool req_enabled; > bool has_flr; > bool has_pm_reset; > + bool has_bus_reset; > bool rom_read_failed; > } VFIOPCIDevice; > > @@ -3533,6 +3534,82 @@ static void vfio_pci_host_needs_bus_reset(Notifier *n, void *opaque) > vbasedev->needs_bus_reset = true; > } > This function really needs some comments > +static void vfio_check_host_bus_reset(VFIODevice *vbasedev) > +{ > + VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev); > + struct vfio_pci_hot_reset_info *info = NULL; > + struct vfio_pci_dependent_device *devices; > + VFIOGroup *group; > + int ret, i; > + > + ret = vfio_get_hot_reset_info(vdev, &info); > + if (ret < 0) { > + goto out; > + } > + > + devices = &info->devices[0]; So we have the list of host devices affected by a bus reset... > + > + /* Verify that we have all the groups required */ > + for (i = 0; i < info->count; i++) { > + PCIHostDeviceAddress host; > + VFIOPCIDevice *tmp; > + VFIODevice *vbasedev_iter; > + > + host.domain = devices[i].segment; > + host.bus = devices[i].bus; > + host.slot = PCI_SLOT(devices[i].devfn); > + host.function = PCI_FUNC(devices[i].devfn); > + Skip the current device > + if (vfio_pci_host_match(&host, &vdev->host)) { > + continue; > + } > + Make sure we own the group for the device > + QLIST_FOREACH(group, &vfio_group_list, next) { > + if (group->groupid == devices[i].group_id) { > + break; > + } > + } > + > + if (!group) { > + goto out; > + } > + Search the group device list for the device, if found make sure it's on the same virtual bus as the target device... This shouldn't be limited to the group device list, it should be done for all vfio-pci devices. For instance, a dual-port card could have 2 separate physical functions that are each isolated and appear in different groups. Obviously a bus reset will affect both. Anything affected by the reset needs to be on (or below) the bus of the target device. That also exposes another issue that I don't see addressed here; a physical bus reset will cascade to all subordinate buses, but your notifier only works via access to the bus reset bit in the control register. Do we also need to make sure the reset function for PCI bridges triggers the notify? Also, this assumes that the host and guest PCI configuration is static, which is not necessarily true for either. So I think that caching the bus reset capability is wrong. > + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { > + if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) { > + continue; > + } > + tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev); > + if (vfio_pci_host_match(&host, &tmp->host)) { > + if (PCI_BUS(vdev->pdev.bus) != > + PCI_BUS(tmp->pdev.bus)) { > + goto out; > + } > + } > + } > + } > + > + vdev->has_bus_reset = true; > + > +out: > + g_free(info); > +} > + > +static void vfio_pci_machine_done_notify(Notifier *notifier, void *unused) > +{ > + VFIOGroup *group; > + VFIODevice *vbasedev; > + > + QLIST_FOREACH(group, &vfio_group_list, next) { > + QLIST_FOREACH(vbasedev, &group->device_list, next) { > + vfio_check_host_bus_reset(vbasedev); > + } > + } > +} > + > +static Notifier machine_notifier = { > + .notify = vfio_pci_machine_done_notify, > +}; > + > static int vfio_initfn(PCIDevice *pdev) > { > VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); > @@ -3821,6 +3898,12 @@ static const TypeInfo vfio_pci_dev_info = { > static void register_vfio_pci_dev_type(void) > { > type_register_static(&vfio_pci_dev_info); > + > + /* > + * Register notifier when machine init is done, since we need > + * check the configration manner after all vfio device are inited. > + */ > + qemu_add_machine_init_done_notifier(&machine_notifier); > } > > type_init(register_vfio_pci_dev_type)