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

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

----- Original Message -----
> On Thu, Feb 04, 2016 at 10:46:52AM -0700, Alex Williamson wrote:
> > 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.
> 
> OK this makes sense.
> So I think the solution is for userspace to make sure bus reset
> is available before exposing aer to guest.
> For example, attempt a bus reset.

The API includes a mechanism for retrieving the affected devices without
making it necessary to actually perform a bus reset.

> > 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.
> 
> Hmm this might be a security problem.

How so?  We don't simply simply trust that the user previously validated
the ability to do a bus reset and allow it any time they ask, each reset
needs to pass the file descriptors related to the affected devices.

> Ideally we should not touch devices we don't *need* to touch.
> But given kernel requires this at the moment (IIUC)
> QEMU could open the device
> but not expose it to guest until actually asked.

Well, a bus reset counts as needing to touch the device.  I don't see
how it could not.  Requiring QEMU to add a new mode of holding a device
only for ownership purposes sounds like an expansion of the scope of
these changes, which is exactly what we don't need at this point.

> >  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.
> 
> Same would apply here.

Again, expanding scope versus configuration requirement, I take the
latter.  Thanks,

Alex