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

[sathyanarayanan kuppuswamy]

On 2/13/19 12:26 AM, Tian, Kevin wrote:
> > 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.
yes, makes sense. I will fix it in next version.
> >   	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

--
Sathyanarayanan Kuppuswamy
Linux kernel developer