[RFC,v7,04/11] vfio: add check host bus reset is support or not

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(+)

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.
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)

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)
>
>
> .
>

On Wed, 2015-05-20 at 15:24 +0800, Chen Fan wrote:
> 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.

So if we have the scenario I describe below and we assume those
functions are the only things in our affected device list, we'd skip the
one that matches our target device.  For the other one, we'd find the
group, traverse the group device list to find the vdev and test whether
it's on the same bus (or a subordinate of the same bus).  Ok, that does
seem to work if we add the subordinate support.  Thanks,

Alex

> > 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)
> >
> >
> > .
> >
>