Re: [PATCH v6 09/10] vfio/pci: Extend VFIO_DEVICE_GET_PCI_HOT_RESET_INFO for vfio device cdev

Baolu Lu
2023-05-26T02:05:25Z

On 5/25/23 9:02 PM, Liu, Yi L wrote:
>> It's possible that requirement
>> might be relaxed in the new DMA ownership model, but as it is right
>> now, the code enforces that requirement and any new discussion about
>> what makes hot-reset available should note both the ownership and
>> dev_set requirement. Thanks,
> I think your point is that if an iommufd_ctx has acquired DMA ownerhisp
> of an iommu_group, it means the device is owned. And it should not
> matter whether all the devices in the iommu_group is present in the
> dev_set. It is allowed that some devices are bound to pci-stub or
> pcieport driver. Is it?
>
> Actually I have a doubt on it. IIUC, the above requirement on dev_set
> is to ensure the reset to the devices are protected by the dev_set->lock.
> So that either the reset issued by driver itself or a hot reset request
> from user, there is no race. But if a device is not in the dev_set, then
> hot reset request from user might race with the bound driver. DMA ownership
> only guarantees the drivers won't handle DMA via DMA API which would have
> conflict with DMA mappings from user. I'm not sure if it is able to
> guarantee reset is exclusive as well. I see pci-stub and pcieport driver
> are the only two drivers that set the driver_managed_dma flag besides the
> vfio drivers. pci-stub may be fine. not sure about pcieport driver.

commit c7d469849747 ("PCI: portdrv: Set driver_managed_dma") described
the criteria of adding driver_managed_dma to the pcieport driver.

"
We achieve this by setting ".driver_managed_dma = true" in pci_driver
structure. It is safe because the portdrv driver meets below criteria:

- This driver doesn't use DMA, as you can't find any related calls like
pci_set_master() or any kernel DMA API (dma_map_*() and etc.).
- It doesn't use MMIO as you can't find ioremap() or similar calls. It's
tolerant to userspace possibly also touching the same MMIO registers
via P2P DMA access.
"

pci_rest_device() definitely shouldn't be done by the kernel drivers
that have driver_managed_dma set.

>
> # line filename / context / line
> 1 39 drivers/pci/pci-stub.c <<GLOBAL>>
> .driver_managed_dma = true,
> 2 796 drivers/pci/pcie/portdrv.c <<GLOBAL>>
> .driver_managed_dma = true,
> 3 607 drivers/vfio/fsl-mc/vfio_fsl_mc.c <<GLOBAL>>
> .driver_managed_dma = true,
> 4 1459 drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c <<GLOBAL>>
> .driver_managed_dma = true,
> 5 1374 drivers/vfio/pci/mlx5/main.c <<GLOBAL>>
> .driver_managed_dma = true,
> 6 203 drivers/vfio/pci/vfio_pci.c <<GLOBAL>>
> .driver_managed_dma = true,
> 7 139 drivers/vfio/platform/vfio_amba.c <<GLOBAL>>
> .driver_managed_dma = true,
> 8 120 drivers/vfio/platform/vfio_platform.c <<GLOBAL>>
> .driver_managed_dma = true,
>
> Anyhow, I think this is not a must so far. is it? Even doable, it shall
> be done in the future. 😄

Perhaps we can take it in this way: it's a bug if any driver sets its
driver_managed_dma but still resets the hardware during it's life cycle?

Best regards,
baolu