Re: [Qemu-devel] [RFC v7 04/11] vfio: add check host bus reset is support or not

[Chen Fan <chen.fan.fnst@cn.fujitsu.com>]

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 <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>   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)
>
>
> .
>