From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53039) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YuyOK-00030k-ME for qemu-devel@nongnu.org; Wed, 20 May 2015 03:26:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YuyOG-0006I7-S2 for qemu-devel@nongnu.org; Wed, 20 May 2015 03:26:12 -0400 Received: from [59.151.112.132] (port=1491 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YuyOA-00069F-7K for qemu-devel@nongnu.org; Wed, 20 May 2015 03:26:08 -0400 Message-ID: <555C36A0.3060307@cn.fujitsu.com> Date: Wed, 20 May 2015 15:24:16 +0800 From: Chen Fan MIME-Version: 1.0 References: <0a0a9cca04fd00560d356133f7b89512c567e5fa.1432008287.git.chen.fan.fnst@cn.fujitsu.com> <1432064082.11375.248.camel@redhat.com> In-Reply-To: <1432064082.11375.248.camel@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed 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: Alex Williamson Cc: izumi.taku@jp.fujitsu.com, qemu-devel@nongnu.org On 05/20/2015 03:34 AM, Alex Williamson wrote: > 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. why? if a bus reset affect both groups, the upper step ought to find the exact group, we only need to check the affected devices is below or on the bus. > 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. yes. > 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? yes, the notify only was registered on parent bus, when exist multiple subordinate buses, we also need to notify the all vfio-pci devices under the bus. Thanks, Chen > > 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) > > > . >