| Message ID | 9-v1-eaf3ccbba33c+1add0-vfio_reflck_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Provide core infrastructure for managing open/release | expand |
On Wed, 14 Jul 2021 21:20:38 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > +/* > + * We need to get memory_lock for each device, but devices can share mmap_lock, > + * therefore we need to zap and hold the vma_lock for each device, and only then > + * get each memory_lock. > + */ > +static int vfio_hot_reset_device_set(struct vfio_pci_device *vdev, > + struct vfio_pci_group_info *groups) > +{ > + struct vfio_device_set *dev_set = vdev->vdev.dev_set; > + struct vfio_pci_device *cur_mem = > + list_first_entry(&dev_set->device_list, struct vfio_pci_device, > + vdev.dev_set_list); We shouldn't be looking at the list outside of the lock, if the first entry got removed we'd break our unwind code. > + struct vfio_pci_device *cur_vma; > + struct vfio_pci_device *cur; > + bool is_mem = true; > + int ret; > > - if (pci_dev_driver(pdev) != &vfio_pci_driver) { > - vfio_device_put(device); > - return -EBUSY; > + mutex_lock(&dev_set->lock); ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > + > + /* All devices in the group to be reset need VFIO devices */ > + if (vfio_pci_for_each_slot_or_bus( > + vdev->pdev, vfio_pci_check_all_devices_bound, dev_set, > + !pci_probe_reset_slot(vdev->pdev->slot))) { > + ret = -EINVAL; > + goto err_unlock; > } > > - vdev = container_of(device, struct vfio_pci_device, vdev); > + list_for_each_entry(cur_vma, &dev_set->device_list, vdev.dev_set_list) { > + /* > + * Test whether all the affected devices are contained by the > + * set of groups provided by the user. > + */ > + if (!vfio_dev_in_groups(cur_vma, groups)) { > + ret = -EINVAL; > + goto err_undo; > + } > > - /* > - * Locking multiple devices is prone to deadlock, runaway and > - * unwind if we hit contention. > - */ > - if (!vfio_pci_zap_and_vma_lock(vdev, true)) { > - vfio_device_put(device); > - return -EBUSY; > + /* > + * Locking multiple devices is prone to deadlock, runaway and > + * unwind if we hit contention. > + */ > + if (!vfio_pci_zap_and_vma_lock(cur_vma, true)) { > + ret = -EBUSY; > + goto err_undo; > + } > } > > - devs->devices[devs->cur_index++] = vdev; > - return 0; > + list_for_each_entry(cur_mem, &dev_set->device_list, vdev.dev_set_list) { > + if (!down_write_trylock(&cur_mem->memory_lock)) { > + ret = -EBUSY; > + goto err_undo; > + } > + mutex_unlock(&cur_mem->vma_lock); > + } > + > + ret = pci_reset_bus(vdev->pdev); > + > + list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) > + up_write(&cur->memory_lock); > + mutex_unlock(&dev_set->lock); > + > + return ret; Isn't the above section actually redundant to below, ie. we could just fall through after the pci_reset_bus()? Thanks, Alex > + > +err_undo: > + list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) { > + if (cur == cur_mem) > + is_mem = false; > + if (cur == cur_vma) > + break; > + if (is_mem) > + up_write(&cur->memory_lock); > + else > + mutex_unlock(&cur->vma_lock); > + } > +err_unlock: > + mutex_unlock(&dev_set->lock); > + return ret; > } > > /*
On Thu, Jul 15, 2021 at 03:00:55PM -0600, Alex Williamson wrote: > On Wed, 14 Jul 2021 21:20:38 -0300 > Jason Gunthorpe <jgg@nvidia.com> wrote: > > +/* > > + * We need to get memory_lock for each device, but devices can share mmap_lock, > > + * therefore we need to zap and hold the vma_lock for each device, and only then > > + * get each memory_lock. > > + */ > > +static int vfio_hot_reset_device_set(struct vfio_pci_device *vdev, > > + struct vfio_pci_group_info *groups) > > +{ > > + struct vfio_device_set *dev_set = vdev->vdev.dev_set; > > + struct vfio_pci_device *cur_mem = > > + list_first_entry(&dev_set->device_list, struct vfio_pci_device, > > + vdev.dev_set_list); > > We shouldn't be looking at the list outside of the lock, if the first > entry got removed we'd break our unwind code. > > > + struct vfio_pci_device *cur_vma; > > + struct vfio_pci_device *cur; > > + bool is_mem = true; > > + int ret; > > > > - if (pci_dev_driver(pdev) != &vfio_pci_driver) { > > - vfio_device_put(device); > > - return -EBUSY; > > + mutex_lock(&dev_set->lock); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Oh, righto, this is an oopsie! > > + list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) > > + up_write(&cur->memory_lock); > > + mutex_unlock(&dev_set->lock); > > + > > + return ret; > > > Isn't the above section actually redundant to below, ie. we could just > fall through after the pci_reset_bus()? Thanks, It could, but I thought it was less confusing this way due to how oddball the below is: > > +err_undo: > > + list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) { > > + if (cur == cur_mem) > > + is_mem = false; > > + if (cur == cur_vma) > > + break; > > + if (is_mem) > > + up_write(&cur->memory_lock); > > + else > > + mutex_unlock(&cur->vma_lock); > > + } But either works, do want it switch in v2? Thanks, Jason
On Thu, 15 Jul 2021 19:11:49 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > On Thu, Jul 15, 2021 at 03:00:55PM -0600, Alex Williamson wrote: > > On Wed, 14 Jul 2021 21:20:38 -0300 > > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > +/* > > > + * We need to get memory_lock for each device, but devices can share mmap_lock, > > > + * therefore we need to zap and hold the vma_lock for each device, and only then > > > + * get each memory_lock. > > > + */ > > > +static int vfio_hot_reset_device_set(struct vfio_pci_device *vdev, > > > + struct vfio_pci_group_info *groups) > > > +{ > > > + struct vfio_device_set *dev_set = vdev->vdev.dev_set; > > > + struct vfio_pci_device *cur_mem = > > > + list_first_entry(&dev_set->device_list, struct vfio_pci_device, > > > + vdev.dev_set_list); > > > > We shouldn't be looking at the list outside of the lock, if the first > > entry got removed we'd break our unwind code. > > > > > + struct vfio_pci_device *cur_vma; > > > + struct vfio_pci_device *cur; > > > + bool is_mem = true; > > > + int ret; > > > > > > - if (pci_dev_driver(pdev) != &vfio_pci_driver) { > > > - vfio_device_put(device); > > > - return -EBUSY; > > > + mutex_lock(&dev_set->lock); > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Oh, righto, this is an oopsie! > > > > + list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) > > > + up_write(&cur->memory_lock); > > > + mutex_unlock(&dev_set->lock); > > > + > > > + return ret; > > > > > > Isn't the above section actually redundant to below, ie. we could just > > fall through after the pci_reset_bus()? Thanks, > > It could, but I thought it was less confusing this way due to how > oddball the below is: > > > > +err_undo: > > > + list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) { > > > + if (cur == cur_mem) > > > + is_mem = false; > > > + if (cur == cur_vma) > > > + break; > > > + if (is_mem) > > > + up_write(&cur->memory_lock); > > > + else > > > + mutex_unlock(&cur->vma_lock); > > > + } > > But either works, do want it switch in v2? Yeah, I think the simpler version just adds to the confusion of what this oddball logic does. It already handles all cases, up to and including success, so let's give it more exercise by always using it. Thanks, Alex
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c index fbc20f6d2dd412..ff23168b8dc551 100644 --- a/drivers/vfio/pci/vfio_pci.c +++ b/drivers/vfio/pci/vfio_pci.c @@ -223,9 +223,11 @@ static void vfio_pci_probe_mmaps(struct vfio_pci_device *vdev) } } +struct vfio_pci_group_info; static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev); static void vfio_pci_disable(struct vfio_pci_device *vdev); -static int vfio_pci_try_zap_and_vma_lock_cb(struct pci_dev *pdev, void *data); +static int vfio_hot_reset_device_set(struct vfio_pci_device *vdev, + struct vfio_pci_group_info *groups); /* * INTx masking requires the ability to disable INTx signaling via PCI_COMMAND @@ -645,37 +647,11 @@ static int vfio_pci_fill_devs(struct pci_dev *pdev, void *data) return 0; } -struct vfio_pci_group_entry { - struct vfio_group *group; - int id; -}; - struct vfio_pci_group_info { int count; - struct vfio_pci_group_entry *groups; + struct vfio_group **groups; }; -static int vfio_pci_validate_devs(struct pci_dev *pdev, void *data) -{ - struct vfio_pci_group_info *info = data; - struct iommu_group *group; - int id, i; - - group = iommu_group_get(&pdev->dev); - if (!group) - return -EPERM; - - id = iommu_group_id(group); - - for (i = 0; i < info->count; i++) - if (info->groups[i].id == id) - break; - - iommu_group_put(group); - - return (i == info->count) ? -EINVAL : 0; -} - static bool vfio_pci_dev_below_slot(struct pci_dev *pdev, struct pci_slot *slot) { for (; pdev; pdev = pdev->bus->self) @@ -753,12 +729,6 @@ int vfio_pci_register_dev_region(struct vfio_pci_device *vdev, return 0; } -struct vfio_devices { - struct vfio_pci_device **devices; - int cur_index; - int max_index; -}; - static long vfio_pci_ioctl(struct vfio_device *core_vdev, unsigned int cmd, unsigned long arg) { @@ -1127,11 +1097,10 @@ static long vfio_pci_ioctl(struct vfio_device *core_vdev, } else if (cmd == VFIO_DEVICE_PCI_HOT_RESET) { struct vfio_pci_hot_reset hdr; int32_t *group_fds; - struct vfio_pci_group_entry *groups; + struct vfio_group **groups; struct vfio_pci_group_info info; - struct vfio_devices devs = { .cur_index = 0 }; bool slot = false; - int i, group_idx, mem_idx = 0, count = 0, ret = 0; + int group_idx, count = 0, ret = 0; minsz = offsetofend(struct vfio_pci_hot_reset, count); @@ -1198,9 +1167,7 @@ static long vfio_pci_ioctl(struct vfio_device *core_vdev, break; } - groups[group_idx].group = group; - groups[group_idx].id = - vfio_external_user_iommu_id(group); + groups[group_idx] = group; } kfree(group_fds); @@ -1212,64 +1179,11 @@ static long vfio_pci_ioctl(struct vfio_device *core_vdev, info.count = hdr.count; info.groups = groups; - /* - * Test whether all the affected devices are contained - * by the set of groups provided by the user. - */ - ret = vfio_pci_for_each_slot_or_bus(vdev->pdev, - vfio_pci_validate_devs, - &info, slot); - if (ret) - goto hot_reset_release; - - devs.max_index = count; - devs.devices = kcalloc(count, sizeof(struct vfio_device *), - GFP_KERNEL); - if (!devs.devices) { - ret = -ENOMEM; - goto hot_reset_release; - } - - /* - * We need to get memory_lock for each device, but devices - * can share mmap_lock, therefore we need to zap and hold - * the vma_lock for each device, and only then get each - * memory_lock. - */ - ret = vfio_pci_for_each_slot_or_bus(vdev->pdev, - vfio_pci_try_zap_and_vma_lock_cb, - &devs, slot); - if (ret) - goto hot_reset_release; - - for (; mem_idx < devs.cur_index; mem_idx++) { - struct vfio_pci_device *tmp = devs.devices[mem_idx]; - - ret = down_write_trylock(&tmp->memory_lock); - if (!ret) { - ret = -EBUSY; - goto hot_reset_release; - } - mutex_unlock(&tmp->vma_lock); - } - - /* User has access, do the reset */ - ret = pci_reset_bus(vdev->pdev); + ret = vfio_hot_reset_device_set(vdev, &info); hot_reset_release: - for (i = 0; i < devs.cur_index; i++) { - struct vfio_pci_device *tmp = devs.devices[i]; - - if (i < mem_idx) - up_write(&tmp->memory_lock); - else - mutex_unlock(&tmp->vma_lock); - vfio_device_put(&tmp->vdev); - } - kfree(devs.devices); - for (group_idx--; group_idx >= 0; group_idx--) - vfio_group_put_external_user(groups[group_idx].group); + vfio_group_put_external_user(groups[group_idx]); kfree(groups); return ret; @@ -2155,37 +2069,94 @@ static int vfio_pci_check_all_devices_bound(struct pci_dev *pdev, void *data) return -EBUSY; } -static int vfio_pci_try_zap_and_vma_lock_cb(struct pci_dev *pdev, void *data) +static bool vfio_dev_in_groups(struct vfio_pci_device *vdev, + struct vfio_pci_group_info *groups) { - struct vfio_devices *devs = data; - struct vfio_device *device; - struct vfio_pci_device *vdev; + unsigned int i; - if (devs->cur_index == devs->max_index) - return -ENOSPC; + for (i = 0; i < groups->count; i++) + if (groups->groups[i] == vdev->vdev.group) + return true; + return false; +} - device = vfio_device_get_from_dev(&pdev->dev); - if (!device) - return -EINVAL; +/* + * We need to get memory_lock for each device, but devices can share mmap_lock, + * therefore we need to zap and hold the vma_lock for each device, and only then + * get each memory_lock. + */ +static int vfio_hot_reset_device_set(struct vfio_pci_device *vdev, + struct vfio_pci_group_info *groups) +{ + struct vfio_device_set *dev_set = vdev->vdev.dev_set; + struct vfio_pci_device *cur_mem = + list_first_entry(&dev_set->device_list, struct vfio_pci_device, + vdev.dev_set_list); + struct vfio_pci_device *cur_vma; + struct vfio_pci_device *cur; + bool is_mem = true; + int ret; - if (pci_dev_driver(pdev) != &vfio_pci_driver) { - vfio_device_put(device); - return -EBUSY; + mutex_lock(&dev_set->lock); + + /* All devices in the group to be reset need VFIO devices */ + if (vfio_pci_for_each_slot_or_bus( + vdev->pdev, vfio_pci_check_all_devices_bound, dev_set, + !pci_probe_reset_slot(vdev->pdev->slot))) { + ret = -EINVAL; + goto err_unlock; } - vdev = container_of(device, struct vfio_pci_device, vdev); + list_for_each_entry(cur_vma, &dev_set->device_list, vdev.dev_set_list) { + /* + * Test whether all the affected devices are contained by the + * set of groups provided by the user. + */ + if (!vfio_dev_in_groups(cur_vma, groups)) { + ret = -EINVAL; + goto err_undo; + } - /* - * Locking multiple devices is prone to deadlock, runaway and - * unwind if we hit contention. - */ - if (!vfio_pci_zap_and_vma_lock(vdev, true)) { - vfio_device_put(device); - return -EBUSY; + /* + * Locking multiple devices is prone to deadlock, runaway and + * unwind if we hit contention. + */ + if (!vfio_pci_zap_and_vma_lock(cur_vma, true)) { + ret = -EBUSY; + goto err_undo; + } } - devs->devices[devs->cur_index++] = vdev; - return 0; + list_for_each_entry(cur_mem, &dev_set->device_list, vdev.dev_set_list) { + if (!down_write_trylock(&cur_mem->memory_lock)) { + ret = -EBUSY; + goto err_undo; + } + mutex_unlock(&cur_mem->vma_lock); + } + + ret = pci_reset_bus(vdev->pdev); + + list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) + up_write(&cur->memory_lock); + mutex_unlock(&dev_set->lock); + + return ret; + +err_undo: + list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) { + if (cur == cur_mem) + is_mem = false; + if (cur == cur_vma) + break; + if (is_mem) + up_write(&cur->memory_lock); + else + mutex_unlock(&cur->vma_lock); + } +err_unlock: + mutex_unlock(&dev_set->lock); + return ret; } /*
Like vfio_pci_try_bus_reset() this code wants to reset all of the devices in the "reset group" which is the same membership as the device set. Instead of trying to reconstruct the device set from the PCI list go directly from the device set's device list to execute the reset. The same basic structure as vfio_pci_try_bus_reset() is used. The 'vfio_devices' struct is replaced with the device set linked list and we simply sweep it multiple times under the lock. This eliminates a memory allocation and get/put traffic and another improperly locked test of pci_dev_driver(). Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/vfio/pci/vfio_pci.c | 205 ++++++++++++++++-------------------- 1 file changed, 88 insertions(+), 117 deletions(-)