Re: [Qemu-devel] [PATCH v14 Resend 08/13] vfio: add check host bus reset is support or not

[Alex Williamson]

On Thu, 2015-12-24 at 20:06 +0200, Michael S. Tsirkin wrote:
> On Thu, Dec 24, 2015 at 10:47:06AM -0700, Alex Williamson wrote:
> > On Thu, 2015-12-24 at 16:32 +0200, Michael S. Tsirkin wrote:
> > > On Thu, Dec 17, 2015 at 09:41:49AM +0800, Cao jin wrote:
> > > > From: Chen Fan <address@hidden>
> > > > 
> > > > when init vfio devices done, we should test all the devices
> > > > supported
> > > > aer whether conflict with others. For each one, get the hot
> > > > reset
> > > > info for the affected device list.  For each affected device,
> > > > all
> > > > should attach to the VM and on/below the same bus. also, we
> > > > should
> > > > test
> > > > all of the non-AER supporting vfio-pci devices on or below the
> > > > target
> > > > bus to verify they have a reset mechanism.
> > > > 
> > > > Signed-off-by: Chen Fan <address@hidden>
> > > > ---
> > > >  hw/vfio/pci.c | 236
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > > >  hw/vfio/pci.h |   1 +
> > > >  2 files changed, 230 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > > > index d00b0e4..6926dcc 100644
> > > > --- a/hw/vfio/pci.c
> > > > +++ b/hw/vfio/pci.c
> > > > @@ -1806,6 +1806,216 @@ static int
> > > > vfio_add_std_cap(VFIOPCIDevice
> > > > *vdev, uint8_t pos)
> > > >      return 0;
> > > >  }
> > > >  
> > > > +static bool vfio_pci_host_slot_match(PCIHostDeviceAddress
> > > > *host1,
> > > > +                                     PCIHostDeviceAddress
> > > > *host2)
> > > > +{
> > > > +    return (host1->domain == host2->domain && host1->bus ==
> > > > host2-
> > > > > bus &&
> > > > +            host1->slot == host2->slot);
> > > > +}
> > > > +
> > > > +static bool vfio_pci_host_match(PCIHostDeviceAddress *host1,
> > > > +                                PCIHostDeviceAddress *host2)
> > > > +{
> > > > +    return (vfio_pci_host_slot_match(host1, host2) &&
> > > > +            host1->function == host2->function);
> > > > +}
> > > > +
> > > > +struct VFIODeviceFind {
> > > > +    PCIDevice *pdev;
> > > > +    bool found;
> > > > +};
> > > > +
> > > > +static void vfio_check_device_noreset(PCIBus *bus, PCIDevice
> > > > *pdev,
> > > > +                                      void *opaque)
> > > > +{
> > > > +    DeviceState *dev = DEVICE(pdev);
> > > > +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
> > > > +    VFIOPCIDevice *vdev;
> > > > +    struct VFIODeviceFind *find = opaque;
> > > > +
> > > > +    if (find->found) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    if (!object_dynamic_cast(OBJECT(dev), "vfio-pci")) {
> > > > +        if (!dc->reset) {
> > > > +            goto found;
> > > > +        }
> > > > +        return;
> > > > +    }
> > > > +    vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> > > > +    if (!(vdev->features & VFIO_FEATURE_ENABLE_AER) &&
> > > > +        !vdev->vbasedev.reset_works) {
> > > > +        goto found;
> > > > +    }
> > > > +
> > > > +    return;
> > > > +found:
> > > > +    find->pdev = pdev;
> > > > +    find->found = true;
> > > > +}
> > > > +
> > > > +static void device_find(PCIBus *bus, PCIDevice *pdev, void
> > > > *opaque)
> > > > +{
> > > > +    struct VFIODeviceFind *find = opaque;
> > > > +
> > > > +    if (find->found) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    if (pdev == find->pdev) {
> > > > +        find->found = true;
> > > > +    }
> > > > +}
> > > > +
> > > > +static int vfio_check_host_bus_reset(VFIOPCIDevice *vdev)
> > > > +{
> > > > +    PCIBus *bus = vdev->pdev.bus;
> > > > +    struct vfio_pci_hot_reset_info *info = NULL;
> > > > +    struct vfio_pci_dependent_device *devices;
> > > > +    VFIOGroup *group;
> > > > +    struct VFIODeviceFind find;
> > > > +    int ret, i;
> > > > +
> > > > +    ret = vfio_get_hot_reset_info(vdev, &info);
> > > > +    if (ret) {
> > > > +        error_report("vfio: Cannot enable AER for device %s,"
> > > > +                     " device does not support hot reset.",
> > > > +                     vdev->vbasedev.name);
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    /* List all affected devices by bus reset */
> > > > +    devices = &info->devices[0];
> > > > +
> > > > +    /* Verify that we have all the groups required */
> > > > +    for (i = 0; i < info->count; i++) {
> > > > +        PCIHostDeviceAddress host;
> > > > +        VFIOPCIDevice *tmp;
> > > > +        VFIODevice *vbasedev_iter;
> > > > +        bool found = false;
> > > > +
> > > > +        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;
> > > > +        }
> > > > +
> > > > +        /* Ensure we own the group of the affected device */
> > > > +        QLIST_FOREACH(group, &vfio_group_list, next) {
> > > > +            if (group->groupid == devices[i].group_id) {
> > > > +                break;
> > > > +            }
> > > > +        }
> > > > +
> > > > +        if (!group) {
> > > > +            error_report("vfio: Cannot enable AER for device
> > > > %s, "
> > > > +                         "depends on group %d which is not
> > > > owned.",
> > > > +                         vdev->vbasedev.name,
> > > > devices[i].group_id);
> > > > +            ret = -1;
> > > > +            goto out;
> > > > +        }
> > > > +
> > > > +        /* Ensure affected devices for reset on/blow the bus
> > > > */
> > > > +        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)) {
> > > > +                PCIDevice *pci = PCI_DEVICE(tmp);
> > > > +
> > > > +                /*
> > > > +                 * For multifunction device, due to vfio
> > > > driver
> > > > signal all
> > > > +                 * functions under the upstream link of the
> > > > end
> > > > point. here
> > > > +                 * we validate all functions whether enable
> > > > AER.
> > > > +                 */
> > > > +                if (vfio_pci_host_slot_match(&vdev->host,
> > > > &tmp-
> > > > > host) &&
> > > > +                    !(tmp->features &
> > > > VFIO_FEATURE_ENABLE_AER)) {
> > > > +                    error_report("vfio: Cannot enable AER for
> > > > device %s, on same slot"
> > > > +                                 " the dependent device %s
> > > > which
> > > > does not enable AER.",
> > > > +                                 vdev->vbasedev.name, tmp-
> > > > > vbasedev.name);
> > > > +                    ret = -1;
> > > > +                    goto out;
> > > > +                }
> > > > +
> > > > +                find.pdev = pci;
> > > > +                find.found = false;
> > > > +                pci_for_each_device(bus, pci_bus_num(bus),
> > > > +                                    device_find, &find);
> > > > +                if (!find.found) {
> > > > +                    error_report("vfio: Cannot enable AER for
> > > > device %s, "
> > > > +                                 "the dependent device %s is
> > > > not
> > > > under the same bus",
> > > > +                                 vdev->vbasedev.name, tmp-
> > > > > vbasedev.name);
> > > > +                    ret = -1;
> > > > +                    goto out;
> > > > +                }
> > > > +                found = true;
> > > > +                break;
> > > > +            }
> > > > +        }
> > > > +
> > > > +        /* Ensure all affected devices assigned to VM */
> > > 
> > > I am puzzled.
> > > Does not kernel enforce this already?
> > > If not it's a security problem.
> > > If yes why does userspace need to check this?
> > 
> > DMA isolation and bus level isolation are separate concepts.  Each
> > function of a multi-function device can have DMA isolation, but a
> > user
> > needs to own all of the functions affected by a bus reset in order
> > to
> > perform one.  An AER configuration can only be created if the user
> > can
> > translate a guest bus reset into a host bus reset and therefore
> > needs
> > to test whether it has the permissions to do so.  I believe over
> > the
> > course of reviews we've also added some simplifying constraints
> > around
> > this to reduce the problem set, things like all the groups being
> > assigned rather than just owned by the user.  However, I believe
> > the
> > kernel is sound in how it provides security for bus resets.
> >  Thanks,
> > 
> > Alex
> 
> Yes, sounds good.
> 
> So how about just trying to do bus reset at setup time?
> If kernel allows this, we know it is safe ...

The host may support hotplug, what's possible at setup time may not be
possible when an error occurs.  It's unlikely, but worth considering I
think.  Thanks,

Alex