Re: [PATCH v4 10/16] iommu/amd: Refactor helper function for setting / clearing GCR3

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Jan 11, 2024 at 05:22:02PM +0530, Vasant Hegde wrote:
> 
> 
> On 1/6/2024 12:42 AM, Jason Gunthorpe wrote:
> > On Tue, Dec 12, 2023 at 08:52:18AM +0000, Vasant Hegde wrote:
> >> +static int __set_gcr3(struct iommu_dev_data *dev_data,
> >> +		      ioasid_t pasid, unsigned long gcr3)
> >> +{
> > 
> > IMHO you've got the ATS layering wrong here.. The ATS invalidation
> > should be pushed out by attach/detach functions and has to be
> > carefully sequenced with the ATS disable bit in the PCI control
> > register. I don't think you can do all of this right with things
> > organized like this. Indeed this looks like it over invalidates the
> > ATS quite a bit.
> 
> Currently if device is capable of ATS we just enable it. Then we invalidate
> while setting/clearing GCR3. This is extra invalidation if we always clear the
> PASID before using it (as clear path would have invalidated IOTLB).

It is simpler if ATS can just be left on, but I'm not sure you can
actually do that when you get to nesting.

The guest and the hypervisor need to agree on the ATS state, if the
guest thinks ATS is off then the guest will not generate ATC
invalidations, which means the physical ATS has to be off too.

Thus all this needs to be carefully sequenced to be dynamic, and this
doesn't look layered well for that.

> > You should only need to invalidate prior to doing the enable and when
> > a GCR3 value is changed while ATS is turned on, which is something
> > that the attach op can caculate.
> > 
> > So, these functions should have a signature of:
> > 
> > (struct amd_iommu *iommu, struct struct gcr3_tbl_info *gcr3_info, ...)
> 
> Not sure I understood this. This path is adding PASID to devices GCR3 table.
> Hence I pass device_data. It will set GCR[PASID] and invalidates TLB (Because
> its per device things, we can get the iommu details from dev_data itself).

I think it is wrong layering to make the GCR3 table linked to the
device, it should be an object indepdent of the device. Any place you
are passing a device into a gcr3 layer function looks suspect to me.

This is why I said you should put the domain_id in the gcr3 table,
because that is the proper layers for the objects and data.

> > [and the locking can implicitly rely on the core's group lock, don't
> > need more locks]
> 
> I was waiting for group->mutex to export so that I can add lockdep_asset with
> that and remove device lock here. May be I can just remove it and expand the
> description.

If you want to use it just add a 'iommu_group_mutex_assert(dev)'. It
is a couple of lines

> >> +static int __clear_gcr3(struct iommu_dev_data *dev_data, ioasid_t pasid)
> >> +{
> >> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
> >> +	u64 *pte;
> >> +
> >> +	lockdep_assert_held(&dev_data->lock);
> >> +
> >> +	pte = __get_gcr3_pte(gcr3_info, pasid, false);
> >> +	if (pte == NULL)
> >> +		return -EINVAL;
> >> +
> >> +	*pte = 0;
> >> +	amd_iommu_dev_flush_pasid_all(dev_data, pasid);
> >> +
> >> +	return 0;
> >> +}
> > 
> > What is the point of clear? By the time the attach ops will want to do
> > clear it is already certain that a non-zero value was installed in
> > pasid, and this doesn't free any memory, so what is the point of
> > 'alloc=false'?
> 
> This will fetch GCR3[PASID] and clears it.
> 
> We use same __get_gcr3_pte() in set/clear path. Hence alloc=false is passed.

Just call set with 0 - again there is no point in having alloc=false
since this isn't called in any case where we don't already know the
that the entry was already allocated.

Also, most likely this eventally needs splitting up like SMMU did so
that allocating memory to get an entry pointer is separate from
setting that entry pointer as a driver should strive for "fail means
no change" implemenation of their domain attach function.

Jason