Re: [Qemu-devel] [RFC v7 05/11] vfio: do hot bus reset when do virtual secondary bus reset

[Alex Williamson]

On Wed, 2015-05-20 at 17:59 +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 do virtual secondary bus reset, the vfio device under
> >> this bus need to do host bus reset to reset the device.
> >> so add this case.
> >>
> >> Signed-off-by: Chen Fan <address@hidden>
> >> ---
> >>   hw/vfio/pci.c                 | 12 +++++++++++-
> >>   include/hw/vfio/vfio-common.h |  1 -
> >>   2 files changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index ff639db..cc169ea 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -169,6 +169,7 @@ typedef struct VFIOPCIDevice {
> >>       bool has_flr;
> >>       bool has_pm_reset;
> >>       bool has_bus_reset;
> >> +    bool needs_bus_reset;
> >>       bool rom_read_failed;
> >>   } VFIOPCIDevice;
> >>   
> >> @@ -3531,7 +3532,8 @@ static void vfio_pci_host_needs_bus_reset(Notifier 
> >> *n, void *opaque)
> >>       VFIOPCIDevice *vdev = container_of(n, VFIOPCIDevice, 
> >> sec_bus_reset_notifier);
> >>       VFIODevice *vbasedev = &vdev->vbasedev;
> >>   
> >> -    vbasedev->needs_bus_reset = true;
> >> +    vdev->needs_bus_reset = true;
> >> +    vbasedev->needs_reset = true;
> > The above should have happened in patch 03.
> >
> >>   }
> >>   
> >>   static void vfio_check_host_bus_reset(VFIODevice *vbasedev)
> >> @@ -3807,6 +3809,14 @@ static void vfio_pci_reset(DeviceState *dev)
> >>   
> >>       trace_vfio_pci_reset(vdev->vbasedev.name);
> >>   
> >> +    if (vdev->has_bus_reset && vdev->needs_bus_reset) {
> >> +        vdev->needs_bus_reset = false;
> >> +        if (vdev->vbasedev.needs_reset) {
> >> +            vfio_pci_hot_reset(vdev, false);
> > So we're not even going to check to see whether that worked?  How can we
> > know that the configuration is still the same as after machine init?  An
> > affected device could have been hot-unplugged an now in use by the host
> > (in which case this function would fail).  An affected device could have
> > been hot unplugged and added back to the VM under a different bus, now
> > we have an arbitrary second device affected by the bus reset from the VM
> > perspective.
> Due to we may have the arbitrary configuration manner after hotplug
> /unplug vfio-pci devices, the enabled aer feature probably became to
> disabled. so shall we consider not to expose the aer feature to user,
> and enable/disable it automatically?

I think that opportunistically enabling it would result in something so
complicated and unpredictable to the user that it would be
indistinguishable from a bug.  We also want to enforce that we can
support it if the user specifies they want it for a particular device.
We can manage that for guest configurations, verifying that each device
that requires it supports hotplug or if it doesn't require it, doesn't
interfere with devices that do.  We can reject hot-adds of devices that
compromise our bus reset capabilities.  The part that's out of our
control is changes on the host, but I think those are sufficiently rare
that we can verify before aer message injection that the configuration
is still viable and either inject or perform the safe halt that we do
now.  Thanks,

Alex

> >> +        }
> >> +        return;
> >> +    }
> >> +
> >>       vfio_pci_pre_reset(vdev);
> >>   
> >>       if (vdev->resetfn && !vdev->resetfn(vdev)) {
> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> >> index 5f5691a..0d1fb80 100644
> >> --- a/include/hw/vfio/vfio-common.h
> >> +++ b/include/hw/vfio/vfio-common.h
> >> @@ -102,7 +102,6 @@ typedef struct VFIODevice {
> >>       bool reset_works;
> >>       bool needs_reset;
> >>       bool allow_mmap;
> >> -    bool needs_bus_reset;
> >>       VFIODeviceOps *ops;
> >>       unsigned int num_irqs;
> >>       unsigned int num_regions;
> >
> >
> > .
> >
>