Re: [Qemu-devel] [v15 12/15] vfio: add bus in reset flag

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

On 01/06/2016 03:58 AM, Alex Williamson wrote:
> On Tue, 2016-01-05 at 09:20 +0800, Cao jin wrote:
>> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>
>> mark the host bus be in reset. avoid multiple devices trigger the
>> host bus reset many times.
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>   hw/vfio/pci.c                 | 6 ++++++
>>   include/hw/vfio/vfio-common.h | 1 +
>>   2 files changed, 7 insertions(+)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index ee88db3..aa0d945 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2249,6 +2249,11 @@ static int vfio_pci_hot_reset(VFIOPCIDevice
>> *vdev, bool single)
>>   
>>       trace_vfio_pci_hot_reset(vdev->vbasedev.name, single ? "one" :
>> "multi");
>>   
>> +    if (vdev->vbasedev.bus_in_reset) {
>> +        vdev->vbasedev.bus_in_reset = false;
>> +        return 0;
>> +    }
>> +
>>       vfio_pci_pre_reset(vdev);
>>       vdev->vbasedev.needs_reset = false;
>>   
>> @@ -2312,6 +2317,7 @@ static int vfio_pci_hot_reset(VFIOPCIDevice
>> *vdev, bool single)
>>                   }
>>                   vfio_pci_pre_reset(tmp);
>>                   tmp->vbasedev.needs_reset = false;
>> +                tmp->vbasedev.bus_in_reset = true;
>>                   multi = true;
>>                   break;
>>               }
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>> common.h
>> index f037f3c..44b19d7 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -95,6 +95,7 @@ typedef struct VFIODevice {
>>       bool reset_works;
>>       bool needs_reset;
>>       bool no_mmap;
>> +    bool bus_in_reset;
>>       VFIODeviceOps *ops;
>>       unsigned int num_irqs;
>>       unsigned int num_regions;
> I imagine this should be a VFIOPCIDevice field, it has no use in the
> common code.  The name is also a bit confusing; when I suggested a
> bus_in_reset flag, I was referring to a property on the bus itself that
> the existing device_reset could query to switch modes rather than add a
> separate callback as you've done in this series.  This works, but it's
> perhaps more intrusive than I was thinking.  It will need to get
> approval by qdev folks.
maybe I don't get your point. I just think add a bus_in_reset flag in bus
has no much sense. for instance, if assigning device A and B from
different host bus into a same virtual bus. assume all check passed.
then if device A aer occurs. we should reset virtual bus to recover
the device A, we also need to reset the device B and do device B host
bus reset. but here the bus_in_reset just denote the device B not need
to do host bus reset, it's incorrect. right?

>
> In any case, this bus_in_reset field is tracking whether a device has
> already been reset as part of a hot reset, sort of a more bus-based
> version with opposite polarity of needs_reset.  It doesn't actually
> track the bus reset state at all, it tracks whether we should skip the
> next call to hot reset for that device.  So it should probably be
> something like VFIOPCIDevice.skip_hot_reset (though that's not a great
> name either).
>
> I also wonder if a "hot" reset callback in qbus is really too PCI
> centered, should it just be "bus_reset"?
>
> Finally, it would be great if you could mention in the cover email
> which patches are new or more than superficially modified from the
> previous version so we can more easily focus on the new code to review.
>   Thanks!

Oh I am sorry, thanks for your mention. I will detail the change
from next version.

Thanks,
Chen

>
> Alex
>
>
> .
>