RE: [PATCH v2 2/2] iommu/vt-d: Enable PASID only if device expects PASID in PRG Response.

[Tian, Kevin]

> From: iommu-bounces@xxxxxxxxxxxxxxxxxxxxxxxxxx [mailto:iommu-
> bounces@xxxxxxxxxxxxxxxxxxxxxxxxxx] On Behalf Of
> sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx
> Sent: Tuesday, February 12, 2019 5:51 AM
> To: bhelgaas@xxxxxxxxxx; joro@xxxxxxxxxx; dwmw2@xxxxxxxxxxxxx
> Cc: Raj, Ashok <ashok.raj@xxxxxxxxx>; linux-pci@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Busch, Keith <keith.busch@xxxxxxxxx>;
> iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx; Pan, Jacob jun
> <jacob.jun.pan@xxxxxxxxx>
> Subject: [PATCH v2 2/2] iommu/vt-d: Enable PASID only if device expects
> PASID in PRG Response.
> 
> From: Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> 
> In Intel IOMMU, if the Page Request Queue (PRQ) is full, it will
> automatically respond to the device with a success message as a keep
> alive. And when sending the success message, IOMMU will include PASID in
> the Response Message when the Page Request has a PASID in Request
> Message and It does not check against the PRG Response PASID
> requirement
> of the device before sending the response. Also, If the device receives the
> PRG response with PASID when its not expecting it then the device behavior
> is undefined. So enable PASID support only if device expects PASID in PRG
> response message.
> 
> Cc: Ashok Raj <ashok.raj@xxxxxxxxx>
> Cc: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> Cc: Keith Busch <keith.busch@xxxxxxxxx>
> Suggested-by: Ashok Raj <ashok.raj@xxxxxxxxx>
> Signed-off-by: Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> ---
>  drivers/iommu/intel-iommu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 1457f931218e..af2e4a011787 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -1399,7 +1399,8 @@ static void iommu_enable_dev_iotlb(struct
> device_domain_info *info)
>  	   undefined. So always enable PASID support on devices which
>  	   have it, even if we can't yet know if we're ever going to
>  	   use it. */
> -	if (info->pasid_supported && !pci_enable_pasid(pdev, info-
> >pasid_supported & ~1))
> +	if (info->pasid_supported && pci_prg_resp_pasid_required(pdev)
> &&
> +	    !pci_enable_pasid(pdev, info->pasid_supported & ~1))
>  		info->pasid_enabled = 1;

Above logic looks problematic. As Dave commented in another thread,
PRI and PASID are orthogonal capabilities. Especially with introduction
of VT-d scalable mode, PASID will be used alone even w/o PRI...

Why not doing the check when PRI is actually enabled? At that point
you can fail the request if above condition is false. 

> 
>  	if (info->pri_supported && !pci_reset_pri(pdev)
> && !pci_enable_pri(pdev, 32))
> --
> 2.20.1
> 
> _______________________________________________
> iommu mailing list
> iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx
> https://lists.linuxfoundation.org/mailman/listinfo/iommu