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

[Vasant Hegde <vasant.hegde@amd.com>]

On 1/22/2024 11:56 PM, Jason Gunthorpe wrote:
> 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!?

Its already fixed. If its per-device-domain-ID it comes from dev_data/gcr3_info.
Else it will come from protection_domain->domain_id.

domain_id_is_per_dev() decides how to allocate domain ID. Right now for V2 and
pass through mode we allocate per-device-domain-ID as they can switch to SVA.



> 
> 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.

We build DTE as soon as we attach device to domain.

Now moving domain ID allocation to setup_gcr3_table complicates things.
  - In attach_device() path we want to allocate domain ID but not GCR3 table
    We can allocate GCR3 table, but if we don't use it its waste of memory.
  - In SVA enablement path we want to allocate GCR3 table
    But by then domain ID should have been allocated. We don't want to allocate
another domain_ID and change ID in SVA enablement path as our domain ID is not
specific to GCR3.



> 
> (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 :\

We are not making anything complicated here!

> 
>>> It is missing some error handling too
>>
>> Where?
> 
> See my diff I sent, it was error unwinds around gcr3 table failure.

Ok.

>  
>>> 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.


We don't want to add condition check that depends on code path:
  like attach_device : allocate domain ID but not GCR3
  SVA path : Allocate GCR3 but not domain ID.

-Vasant


> 
>>> @@ -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