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

[Baolu Lu <baolu.lu@linux.intel.com>]

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