[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v14 Resend 08/13] vfio: add check host bus reset
From: |
Alex Williamson |
Subject: |
Re: [Qemu-devel] [PATCH v14 Resend 08/13] vfio: add check host bus reset is support or not |
Date: |
Thu, 24 Dec 2015 11:41:15 -0700 |
On Thu, 2015-12-24 at 20:23 +0200, Michael S. Tsirkin wrote:
> On Thu, Dec 24, 2015 at 11:20:26AM -0700, Alex Williamson wrote:
> > 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.
>
> How does this patch help solve this problem?
I believe there's a patch in this series that re-tests on the
occurrence of an error, before injecting the AER into the guest.
> > It's unlikely, but worth considering I
> > think.
>
> I suspect vfio will have to solve this in kernel
> (e.g. automatically add all new devices in the same group
> wrt reset).
Nope, the user simply loses their ability to reset the bus if they
don't own all the groups at the time they attempt to do a bus reset.
Mixing bus isolation and DMA isolation would cause a mess of groups.
- Re: [Qemu-devel] [PATCH v14 Resend 09/13] add check reset mechanism when hotplug vfio device, (continued)
- [Qemu-devel] [PATCH v14 Resend 07/13] vfio: add aer support for vfio device, Cao jin, 2015/12/16
- [Qemu-devel] [PATCH v14 Resend 08/13] vfio: add check host bus reset is support or not, Cao jin, 2015/12/16
- Re: [Qemu-devel] [PATCH v14 Resend 08/13] vfio: add check host bus reset is support or not, Alex Williamson, 2015/12/17
- Re: [Qemu-devel] [PATCH v14 Resend 08/13] vfio: add check host bus reset is support or not, Michael S. Tsirkin, 2015/12/24
- Re: [Qemu-devel] [PATCH v14 Resend 08/13] vfio: add check host bus reset is support or not, Alex Williamson, 2015/12/24
- Re: [Qemu-devel] [PATCH v14 Resend 08/13] vfio: add check host bus reset is support or not, Michael S. Tsirkin, 2015/12/24
- Re: [Qemu-devel] [PATCH v14 Resend 08/13] vfio: add check host bus reset is support or not, Alex Williamson, 2015/12/24
- Re: [Qemu-devel] [PATCH v14 Resend 08/13] vfio: add check host bus reset is support or not, Michael S. Tsirkin, 2015/12/24
- Re: [Qemu-devel] [PATCH v14 Resend 08/13] vfio: add check host bus reset is support or not,
Alex Williamson <=
- Re: [Qemu-devel] [PATCH v14 Resend 08/13] vfio: add check host bus reset is support or not, Michael S. Tsirkin, 2015/12/24