Re: [PATCH v6 08/15] iommu/amd: Enable PCI features based on attached domain capability

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Mar 05, 2024 at 08:40:08PM +0530, Vasant Hegde wrote:
> Jason,
> 
> 
> On 3/5/2024 6:02 AM, Jason Gunthorpe wrote:
> > On Fri, Feb 09, 2024 at 11:29:23AM +0000, Vasant Hegde wrote:
> >> Commit eda8c2860ab ("iommu/amd: Enable device ATS/PASID/PRI capabilities
> >> independently") changed the way it enables device capability while
> >> attaching devices. I missed to account the attached domain capability.
> >> Meaning if domain is not capable of handling PASID/PRI (ex: paging
> >> domain with v1 page table) then enabling device feature is not required.
> >>
> >> This patch enables PASID/PRI only if domain is capable of handling SVA.
> >> Also move pci feature enablement to do_attach() function so that we make
> >> SVA capability in one place. Finally make PRI enable/disable functions as
> >> static functions.
> > 
> >> @@ -2036,6 +2038,7 @@ static int do_attach(struct iommu_dev_data *dev_data,
> >>  		     struct protection_domain *domain)
> >>  {
> >>  	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
> >> +	struct pci_dev *pdev;
> >>  	int ret = 0;
> >>  
> >>  	/* Update data structures */
> >> @@ -2050,10 +2053,16 @@ static int do_attach(struct iommu_dev_data *dev_data,
> >>  	domain->dev_iommu[iommu->index] += 1;
> >>  	domain->dev_cnt                 += 1;
> >>  
> >> +	pdev = dev_is_pci(dev_data->dev) ? to_pci_dev(dev_data->dev) : NULL;
> >>  	if (pdom_is_sva_capable(domain)) {
> >>  		ret = init_gcr3_table(dev_data, domain);
> >>  		if (ret)
> >>  			return ret;
> >> +
> >> +		if (pdev)
> >> +			pdev_enable_caps(pdev);
> > 
> > But here we are turning PRI on for an IDENTITY domain and for a v2
> > domain that doesn't have a PRI handler. This is not technically what
> > we want to see, the PRI capability at the device should only be turned
> > on when a PRI handler is available in the SW side. Otherwise PRI
> > should be off and non-present ATS responses should fail in the device.
> 
> We are setting up handler right after enabling capability for both IDENTIFY and
> v2 domain as both can support SVA mode.

It is enabling PRI when the device can enable PRI but before any
driver has actually asked for any PRI features. It is technically too
early.

It should be enabled when a SVA domain is first attached.

Like we don't want the HW to generate PRI faults from a VM
deliberately banging on non-translated page table memory. This is a
pure DOS attack on the hypervisor kernel and should be prevented by
having PRI turned off.

> > Also, PASID enablement looks like the wrong spot too, it should be
> > done in the probe_device() callback and stay enabled. The bit is set
> > if the IOMMU HW can decode the PASID TLP. Ie don't enable it on old
> > IOMMU HW that can't understand PASID.
> 
> Our V1 page table is not compatible with PASID. Hence currently its done in
> attach_device path.

What does "compatible" mean exactly?

The PASID cap on the device is only about the IOMMU HW ability to
properly *parse* PASID, it can fail every PASID tagged TLP and it
would still be fine to turn it on at the device always.

v1 is clearly not able to route PASID to different translations, but
as long as a v1 DTE fails every single PASID tagged TLP you can always
turn PASID on at the source just fine.

It would be a very weird design if v2 HW makes the DTE in v1 mode just
discards the PASID and translates everything with the v1 table. I'd
also wonder if the identity/blocked cases are similarly weird?

IOW, I would expect to see probe_device check if the HW supports V2,
if it does it knows the HW can parse the PASID in the TLP and will
abort PASID's without translations (ie v1 DTEs). Then you turn on
PASID support always. If the HW supports only v1 then assume it
doesn't know how to parse the PASID and will ignore it and fold all
PASID requests into the RID. Leave PASID always off.

Jason