[PATCH rc] vfio: Move IOMMU_CAP_CACHE_COHERENCY test to after we know we have a group

[PATCH rc] vfio: Move IOMMU_CAP_CACHE_COHERENCY test to after we know we have a group

[Jason Gunthorpe]

The test isn't going to work if a group doesn't exist. Normally this isn't
a problem since VFIO isn't going to create a device if there is no group,
but the special CONFIG_VFIO_NOIOMMU behavior allows bypassing this
prevention. The new cap test effectively forces a group and breaks this
config option.

Move the cap test to vfio_group_find_or_alloc() which is the earliest time
we know we have a group available and thus are not running in noiommu mode.

Fixes: e8ae0e140c05 ("vfio: Require that devices support DMA cache coherence")
Reported-by "chenxiang (M)" <chenxiang66@hisilicon.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

This should fixe the issue with dpdk on noiommu, but I've left PPC out.

I think the right way to fix PPC is to provide the iommu_ops for the devices
groups it is creating. They don't have to be fully functional - eg they don't
have to to create domains, but if the ops exist they can correctly respond to
iommu_capable() and we don't need special code here to work around PPC being
weird.

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index e43b9496464bbf..cbb693359502d9 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -552,6 +552,16 @@ static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
 	if (!iommu_group)
 		return ERR_PTR(-EINVAL);
 
+	/*
+	 * VFIO always sets IOMMU_CACHE because we offer no way for userspace to
+	 * restore cache coherency. It has to be checked here because it is only
+	 * valid for cases where we are using iommu groups.
+	 */
+	if (!iommu_capable(dev->bus, IOMMU_CAP_CACHE_COHERENCY)) {
+		iommu_group_put(iommu_group);
+		return ERR_PTR(-EINVAL);
+	}
+
 	group = vfio_group_get_from_iommu(iommu_group);
 	if (!group)
 		group = vfio_create_group(iommu_group, VFIO_IOMMU);
@@ -604,13 +614,6 @@ static int __vfio_register_dev(struct vfio_device *device,
 
 int vfio_register_group_dev(struct vfio_device *device)
 {
-	/*
-	 * VFIO always sets IOMMU_CACHE because we offer no way for userspace to
-	 * restore cache coherency.
-	 */
-	if (!iommu_capable(device->dev->bus, IOMMU_CAP_CACHE_COHERENCY))
-		return -EINVAL;
-
 	return __vfio_register_dev(device,
 		vfio_group_find_or_alloc(device->dev));
 }

base-commit: e2475f7b57209e3c67bf856e1ce07d60d410fb40
-- 
2.37.0

Re: [PATCH rc] vfio: Move IOMMU_CAP_CACHE_COHERENCY test to after we know we have a group

[chenxiang (M)]

在 2022/7/5 9:10, Jason Gunthorpe 写道:

> The test isn't going to work if a group doesn't exist. Normally this isn't
> a problem since VFIO isn't going to create a device if there is no group,
> but the special CONFIG_VFIO_NOIOMMU behavior allows bypassing this
> prevention. The new cap test effectively forces a group and breaks this
> config option.
>
> Move the cap test to vfio_group_find_or_alloc() which is the earliest time
> we know we have a group available and thus are not running in noiommu mode.
>
> Fixes: e8ae0e140c05 ("vfio: Require that devices support DMA cache coherence")
> Reported-by "chenxiang (M)" <chenxiang66@hisilicon.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

I have tested the patch separately in iommu mode and in noiommu mode, 
and it works well, so please feel free to add:
Tested-by: Xiang Chen <chenxiang66@hisilicon.com>

> ---
>   drivers/vfio/vfio.c | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
>
> This should fixe the issue with dpdk on noiommu, but I've left PPC out.
>
> I think the right way to fix PPC is to provide the iommu_ops for the devices
> groups it is creating. They don't have to be fully functional - eg they don't
> have to to create domains, but if the ops exist they can correctly respond to
> iommu_capable() and we don't need special code here to work around PPC being
> weird.
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index e43b9496464bbf..cbb693359502d9 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -552,6 +552,16 @@ static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
>   	if (!iommu_group)
>   		return ERR_PTR(-EINVAL);
>   
> +	/*
> +	 * VFIO always sets IOMMU_CACHE because we offer no way for userspace to
> +	 * restore cache coherency. It has to be checked here because it is only
> +	 * valid for cases where we are using iommu groups.
> +	 */
> +	if (!iommu_capable(dev->bus, IOMMU_CAP_CACHE_COHERENCY)) {
> +		iommu_group_put(iommu_group);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>   	group = vfio_group_get_from_iommu(iommu_group);
>   	if (!group)
>   		group = vfio_create_group(iommu_group, VFIO_IOMMU);
> @@ -604,13 +614,6 @@ static int __vfio_register_dev(struct vfio_device *device,
>   
>   int vfio_register_group_dev(struct vfio_device *device)
>   {
> -	/*
> -	 * VFIO always sets IOMMU_CACHE because we offer no way for userspace to
> -	 * restore cache coherency.
> -	 */
> -	if (!iommu_capable(device->dev->bus, IOMMU_CAP_CACHE_COHERENCY))
> -		return -EINVAL;
> -
>   	return __vfio_register_dev(device,
>   		vfio_group_find_or_alloc(device->dev));
>   }
>
> base-commit: e2475f7b57209e3c67bf856e1ce07d60d410fb40

Re: [PATCH rc] vfio: Move IOMMU_CAP_CACHE_COHERENCY test to after we know we have a group

[Alexey Kardashevskiy]

On 7/5/22 11:10, Jason Gunthorpe wrote:
> The test isn't going to work if a group doesn't exist. Normally this isn't
> a problem since VFIO isn't going to create a device if there is no group,
> but the special CONFIG_VFIO_NOIOMMU behavior allows bypassing this
> prevention. The new cap test effectively forces a group and breaks this
> config option.
> 
> Move the cap test to vfio_group_find_or_alloc() which is the earliest time
> we know we have a group available and thus are not running in noiommu mode.
> 
> Fixes: e8ae0e140c05 ("vfio: Require that devices support DMA cache coherence")
> Reported-by "chenxiang (M)" <chenxiang66@hisilicon.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/vfio/vfio.c | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
> 
> This should fixe the issue with dpdk on noiommu, but I've left PPC out.
> 
> I think the right way to fix PPC is to provide the iommu_ops for the devices
> groups it is creating. They don't have to be fully functional - eg they don't
> have to to create domains, but if the ops exist they can correctly respond to
> iommu_capable() and we don't need special code here to work around PPC being
> weird.


This is what I've been doing since Friday and while the coherency thing 
is easy to do, the iommu_group_claim_dma_owner() is not - it wants to 
allocate domains which leads to more hooks and even when I do all that - 
for example, iommu_group_claim_dma_owner() won't do what it supposed to 
as nothing outside VFIO is going to try mapping DMA and fail to report 
that the group is not "viable".

I think I'll post the iommu_ops PPC patch for coherency and continue 
poking the other thing. Thanks,


> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index e43b9496464bbf..cbb693359502d9 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -552,6 +552,16 @@ static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
>   	if (!iommu_group)
>   		return ERR_PTR(-EINVAL);
>   
> +	/*
> +	 * VFIO always sets IOMMU_CACHE because we offer no way for userspace to
> +	 * restore cache coherency. It has to be checked here because it is only
> +	 * valid for cases where we are using iommu groups.
> +	 */
> +	if (!iommu_capable(dev->bus, IOMMU_CAP_CACHE_COHERENCY)) {
> +		iommu_group_put(iommu_group);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>   	group = vfio_group_get_from_iommu(iommu_group);
>   	if (!group)
>   		group = vfio_create_group(iommu_group, VFIO_IOMMU);
> @@ -604,13 +614,6 @@ static int __vfio_register_dev(struct vfio_device *device,
>   
>   int vfio_register_group_dev(struct vfio_device *device)
>   {
> -	/*
> -	 * VFIO always sets IOMMU_CACHE because we offer no way for userspace to
> -	 * restore cache coherency.
> -	 */
> -	if (!iommu_capable(device->dev->bus, IOMMU_CAP_CACHE_COHERENCY))
> -		return -EINVAL;
> -
>   	return __vfio_register_dev(device,
>   		vfio_group_find_or_alloc(device->dev));
>   }
> 
> base-commit: e2475f7b57209e3c67bf856e1ce07d60d410fb40

-- 
Alexey

Re: [PATCH rc] vfio: Move IOMMU_CAP_CACHE_COHERENCY test to after we know we have a group

[Alex Williamson]

On Mon,  4 Jul 2022 22:10:50 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> The test isn't going to work if a group doesn't exist. Normally this isn't
> a problem since VFIO isn't going to create a device if there is no group,
> but the special CONFIG_VFIO_NOIOMMU behavior allows bypassing this
> prevention. The new cap test effectively forces a group and breaks this
> config option.
> 
> Move the cap test to vfio_group_find_or_alloc() which is the earliest time
> we know we have a group available and thus are not running in noiommu mode.
> 
> Fixes: e8ae0e140c05 ("vfio: Require that devices support DMA cache coherence")
> Reported-by "chenxiang (M)" <chenxiang66@hisilicon.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/vfio.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)

Fixed-up Reported-by, added Tested-by, and pushed to vfio for-linus
branch for v5.19.  Thanks,

Alex

> 
> This should fixe the issue with dpdk on noiommu, but I've left PPC out.
> 
> I think the right way to fix PPC is to provide the iommu_ops for the devices
> groups it is creating. They don't have to be fully functional - eg they don't
> have to to create domains, but if the ops exist they can correctly respond to
> iommu_capable() and we don't need special code here to work around PPC being
> weird.
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index e43b9496464bbf..cbb693359502d9 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -552,6 +552,16 @@ static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
>  	if (!iommu_group)
>  		return ERR_PTR(-EINVAL);
>  
> +	/*
> +	 * VFIO always sets IOMMU_CACHE because we offer no way for userspace to
> +	 * restore cache coherency. It has to be checked here because it is only
> +	 * valid for cases where we are using iommu groups.
> +	 */
> +	if (!iommu_capable(dev->bus, IOMMU_CAP_CACHE_COHERENCY)) {
> +		iommu_group_put(iommu_group);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	group = vfio_group_get_from_iommu(iommu_group);
>  	if (!group)
>  		group = vfio_create_group(iommu_group, VFIO_IOMMU);
> @@ -604,13 +614,6 @@ static int __vfio_register_dev(struct vfio_device *device,
>  
>  int vfio_register_group_dev(struct vfio_device *device)
>  {
> -	/*
> -	 * VFIO always sets IOMMU_CACHE because we offer no way for userspace to
> -	 * restore cache coherency.
> -	 */
> -	if (!iommu_capable(device->dev->bus, IOMMU_CAP_CACHE_COHERENCY))
> -		return -EINVAL;
> -
>  	return __vfio_register_dev(device,
>  		vfio_group_find_or_alloc(device->dev));
>  }
> 
> base-commit: e2475f7b57209e3c67bf856e1ce07d60d410fb40