Re: [PATCH v5 14/17] iommu/amd: Refactor GCR3 table helper functions

[Jason Gunthorpe <jgg@ziepe.ca>]

On Mon, Jan 22, 2024 at 03:53:18PM +0530, Vasant Hegde wrote:
> Jason,
> 
> 
> On 1/20/2024 1:29 AM, Jason Gunthorpe wrote:
> > On Tue, Jan 16, 2024 at 04:53:32PM +0000, Vasant Hegde wrote:
> > 
> >> @@ -1738,22 +1744,22 @@ static int get_gcr3_levels(int pasids)
> >>  	return levels ? (DIV_ROUND_UP(levels, 9) - 1) : levels;
> >>  }
> >>  
> >> -/* Note: This function expects iommu_domain->lock to be held prior calling the function. */
> >> -static int setup_gcr3_table(struct protection_domain *domain, int pasids)
> >> +static int setup_gcr3_table(struct gcr3_tbl_info *gcr3_info,
> >> +			    int nid, int pasids)
> >>  {
> >>  	int levels = get_gcr3_levels(pasids);
> >>  
> >>  	if (levels > amd_iommu_max_glx_val)
> >>  		return -EINVAL;
> >>  
> >> -	domain->gcr3_tbl = alloc_pgtable_page(domain->nid, GFP_ATOMIC);
> >> -	if (domain->gcr3_tbl == NULL)
> >> -		return -ENOMEM;
> >> +	if (gcr3_info->gcr3_tbl)
> >> +		return -EBUSY;
> >>  
> >> -	domain->glx      = levels;
> >> -	domain->flags   |= PD_IOMMUV2_MASK;
> >> +	gcr3_info->gcr3_tbl = alloc_pgtable_page(nid, GFP_KERNEL);
> >> +	if (gcr3_info->gcr3_tbl == NULL)
> >> +		return -ENOMEM;
> >>  
> >> -	amd_iommu_domain_update(domain);
> >> +	gcr3_info->glx = levels;
> > 
> > I think this patch should also move the domain_id
> > allocation/deallocation into setup_gcr3_table()/free_gcr3_table()
> 
> We do domain allocation in device attach path only. But GCR3 table
> allocation/setup can happen while enabling SVA (like passthrough -> SVA path).
> Hence I have kept it in attach() path itself.

Huh? That doesn't sound right. Any time you install a GCR3 table into
a DTE you need to get a domain_id *for that GCR3 table*. There is no
other place to get a domain id!?

All this logic should be shared between the pasid and rid attach
paths.

The passthrough thing is only an issue of DTE construction.

If you build a DTE with a GCR3 table and RID=IDENTITY then you set
some bits, and that is it. Detect that case directly when you build
the DTE. It should have no effect on what domain ID is used to tag
translations retrived from a GCR3 table.

(I say that with some trepidation because it isn't clear to me how the
AMD IOMMU caches the DTE entries themselves)

I've said before the DTE construction should be fixed up before making
things more complicated :\

> > It is missing some error handling too
> 
> Where?

See my diff I sent, it was error unwinds around gcr3 table failure.
 
> > And domain_id_is_per_dev() is pretty redundant once you do that. See below
> 
> We need to handle passthrough as well. It doesn't make sense to allocate and
> keep GCR3 table when we are not going to use it.

Then don't, and my diff didn't - but check for the passthrough case
directly against the attached domain as identity.

> > @@ -1902,7 +1911,7 @@ static void set_dte_entry(struct amd_iommu *iommu,
> >  	struct dev_table_entry *dev_table = get_dev_table(iommu);
> >  	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
> >  
> > -	if (domain_id_is_per_dev(domain))
> > +	if (gcr3_info && gcr3_info->gcr3_tbl) {
> >  		domid = dev_data->gcr3_info.domid;
> >  	else
> >  		domid = domain->id;

Because here it already has the (gcr3_info && gcr3_info->gcr3_tbl)
test below to decide if the gcr3 will be written to the DTE, so of
course it should be the same test to decide where the domain id comes
from.

> > @@ -2018,15 +2027,10 @@ static int do_attach(struct iommu_dev_data *dev_data,
> >  	domain->dev_iommu[iommu->index] += 1;
> >  	domain->dev_cnt                 += 1;
> >  
> > -	/* Allocate per device domain ID */
> > -	if (domain_id_is_per_dev(domain))
> > -		dev_data->gcr3_info.domid = domain_id_alloc();
> > -

And this gets moved into this test:

> >  	/* Init GCR3 table and update device table */
> >  	if (domain->pd_mode == PD_MODE_V2) {

Here, which is obviously correct at this point as you only need a GCR3
table when installing a V2 table on the RID.

> > @@ -2073,10 +2077,6 @@ static void do_detach(struct iommu_dev_data *dev_data)
> >  	/* decrease reference counters - needs to happen after the flushes */
> >  	domain->dev_iommu[iommu->index] -= 1;
> >  	domain->dev_cnt                 -= 1;
> > -
> > -	/* Free per device domain ID */
> > -	if (domain_id_is_per_dev(domain))
> > -		domain_id_free(dev_data->gcr3_info.domid);

And this got moved into the gcr3 free a few lines up that checked the
pd_mode.

Jason