Re: [Qemu-devel] [PATCH v16 00/14] vfio-pci: pass the aer error to guest

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 4 Feb 2016 13:21:57 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Feb 04, 2016 at 10:04:01AM +0800, Chen Fan wrote:
> > 
> > On 02/03/2016 09:57 PM, Michael S. Tsirkin wrote:  
> > >On Wed, Feb 03, 2016 at 04:54:01PM +0800, Chen Fan wrote:  
> > >>On 01/17/2016 02:34 AM, Michael S. Tsirkin wrote:  
> > >>>On Tue, Jan 12, 2016 at 10:43:01AM +0800, Cao jin wrote:  
> > >>>>From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > >>>>
> > >>>>For now, for vfio pci passthough devices when qemu receives
> > >>>>an error from host aer report, currentlly just terminate the
> > >>>>guest, but usually user want to know what error occurred but
> > >>>>stopping the guest, so this patches add aer capability support
> > >>>>for vfio device, and pass the error to guest, and have guest
> > >>>>driver to recover from the error.  
> > >>>I would like to see a version of this patchset that doesn't
> > >>>depend on pci core changes.
> > >>>I think that if you make this simplifying assumption:
> > >>>
> > >>>- all devices on same bus in guest are on same bus in host
> > >>>
> > >>>then you can handle both reset and hotplug simply in function 0
> > >>>since it will belong to vfio.
> > >>>
> > >>>So we can have a version without pci core changes that simply
> > >>>assumes this, and things will just work.
> > >>>
> > >>>
> > >>>Now, if we wanted to enforce this limitation, I think the
> > >>>cleanest way would be to add a callback in struct PCIDevice:
> > >>>
> > >>>	bool is_valid_function(PCIDevice *newfunction)
> > >>>
> > >>>and call it as each function is added.
> > >>>This way aer function can validate that each function
> > >>>added shares the same bus.
> > >>>And this way issues will be detected directly and not when
> > >>>function 0 is added.
> > >>>
> > >>>I would prefer this validation code to be a patch on top so we
> > >>>can merge the functionality directly and avoid blocking it while
> > >>>we figure out the best api to validate things.
> > >>>
> > >>>I don't see why making guest topology match host would
> > >>>ever be a problem, but if it's required to support
> > >>>configurations where these differ, I'd like to see
> > >>>an attempt to address that be split out, after aer
> > >>>is supported.  
> > >>Hi Michael,
> > >>
> > >>Just think about this more,  I think we also should check the vfio
> > >>devices whether on the same bus at the time of function 0 is
> > >>added. because we don't know the affected devices by a bus reset
> > >>have already all been assigned to VM.  
> > >This is something vfio in kernel should check.
> > >You can't rely on qemu being well behaved, so don't
> > >even try to catch cases which would break host in userspace.
> > >
> > >qemu should only worry about not breaking guest.
> > >
> > >  
> > >>for example, the multi-function's hotplug.
> > >>devices on same bus in host are added to VM one by one. when we
> > >>test one device, we haven't yet added the other devices.
> > >>so I think
> > >>the patch should like below. then we could add a
> > >>vfio_is_valid_function in vfio
> > >>to test each device whether the affected devices on the same bus.
> > >>
> > >>Thanks,
> > >>Chen
> > >>
> > >>diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > >>index d940f79..7163b56 100644
> > >>--- a/hw/pci/pci.c
> > >>+++ b/hw/pci/pci.c
> > >>@@ -1836,6 +1836,38 @@ PCIDevice *pci_find_device(PCIBus *bus,
> > >>int bus_num, uint8_t devfn)
> > >>      return bus->devices[devfn];
> > >>  }
> > >>
> > >>+static int pci_bus_check_devices(PCIBus *bus)
> > >>+{
> > >>+    PCIDeviceClass *pc;
> > >>+    int i, ret = 0;
> > >>+
> > >>+    for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
> > >>+        if (!bus->devices[i]) {
> > >>+            continue;
> > >>+        }
> > >>+
> > >>+        pc = PCI_DEVICE_GET_CLASS(bus->devices[i]);
> > >>+        if (!pc->is_valid_func) {
> > >>+            continue;
> > >>+        }
> > >>+
> > >>+        ret = pc->is_valid_func(bus->devices[i], bus);
> > >>+        if (!ret) {
> > >>+            return -1;
> > >>+        }
> > >>+    }
> > >>+    return 0;
> > >>+}
> > >>+
> > >>+static bool pci_is_valid_function(PCIDevice *pdev, PCIBus *bus)
> > >>+{
> > >>+    if (pdev->bus == bus) {
> > >>+        return true;
> > >>+    }
> > >>+
> > >>+    return false;
> > >>+}
> > >>+  
> > >I don't really understand what is this one doing.
> > >Why do we need a default function?  
> > if the vfio driver in kernel can handle the bus reset for any one
> > device in qemu without the affected devices assigned. I think
> > we don't need this default one.
> > BTW, IIRC at present the devices on the same bus in host can
> > be assigned to different VM, so if we want to support this kind of
> > bus reset for an independent device when enable aer, aren't we
> > limiting the case that others devices on the same bus must be
> > assigned to current VM?
> > 
> > Thanks,
> > Chen  
> 
> I don't believe this works at the moment, and
> I'd expect kernel to prevent this,
> so we should not rely on userspace code for this.
> Alex, could you comment please?

DMA isolation and bus isolation are separate things.  So long as
devices on the same bus are DMA isolated then they can be assigned to
separate VMs or split between host and VM.  However, certain features
like bus reset are not available to the user unless they "own" all of
the DMA isolated sets affected by a bus reset.  The kernel doesn't care
how the user is using them, but they must prove they own them through
vfio group file descriptors.

I thought in previous discussions we decided that unused devices made
the problem set more complicated for userspace so we simplified by
requiring them to be assigned.  For instance imagine a two function
device, with DMA isolation between functions, where we only want one
function assigned to the VM. QEMU would need to learn to take ownership
of the other function without exposing it to the VM simply for the
purpose of being able to perform a bus reset.  Another simplification
was to expose them in the same slot, so we don't need to worry about VM
configurations where one device could be hot-unplugged and the
ownership released, breaking QEMU's ability to do a bus reset on the
remaining device.  There are a lot of configuration restrictions
imposed by adding the requirement that QEMU needs to be able to perform
a host bus reset on a device in order to support this feature.  Thanks,

Alex