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

["Michael S. Tsirkin" <mst@redhat.com>]

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 <chen.fan.fnst@cn.fujitsu.com>
> > > 
> > > 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 <chen.fan.fnst@cn.fujitsu.com>
> > > ---
> > >  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 ...

-- 
MST