Re: [PATCH RESEND 03/10] iommu/amd: Initial SVA support for AMD IOMMU

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

Jason,


On 8/30/2023 10:37 PM, Jason Gunthorpe wrote:
> On Mon, Aug 28, 2023 at 04:09:16PM +0530, Vasant Hegde wrote:
> 
>>>> +static void *sva_pasid_private_find(u32 pasid)
>>>> +{
>>>> +	return xa_load(&sva_pasid_array, pasid);
>>>> +}
>>>
>>> No PASID stuff in SVA code at all please, all of this is wrong.
>>
>> This is based on upstream code!
> 
> Yes, and we are changing all of this because how wrong the drivers
> went with stuff like this. :(
> 
>>> No per-PASID struct. AMD enablement should go after this series:
>>>
>>>  https://lore.kernel.org/linux-iommu/20230808074944.7825-1-tina.zhang@intel.com/
>>>
>>> Put the mmu_notifier directly into the protection_domain. Assume you
>>> have a single protection_domain per mm.
>>
>> Actually we have protection domain for each IOMMU group (or multiple group in
>> case of VM). 
> 
> Why would you do that for SVA? That is not the direction we are going.
> One iommu_domain per mm, managed by the core code.
> 
> Driver assumes this and optimizes based on it.
> 
> If driver needs per device/pasid/whatever data then it keeps only that
> data in a list hanging off the single iommu_domain.
> 
> It should be *exactly the same* data that an unmanaged domain needs to
> manage its invalidations and ATC.
> 
>>> Also amd_sva_dev is not appropriate, the list of PASIDs (and masters)
>>> a domain is associated with is part of the generic PASID support in
>>> the protection_domain itself.
>>
>> You mean to say, we maintain PASID list in device protection_domain
>> and then in
> 
> yes, a linked list of devices RID and PASIDs that the domain is
> attached to.
> 
> This is mandatory to issue any form of invalidation, an UNMANAGED
> domains should use the same list, and same mechanism to invalidate
> their PASID attachments as well.
> 
>> invalidation path (somehow) we retrieve protection domain and use it
>> for
> 
> Yes, obtain the domain trivially via container_of(mmu_notifier). Store
> the notifier in the SVA iommu_domain's driver struct
> (protection_domain) to do this.

Reading through the discussion so far again and the other series, my
understanding is :
  - set_dev_pasid() will check the compatibility and bind device/pasid only if
its compatibility. In AMD case we will check against protection domain. Ex:
 If we have two devices (devA and devB) in two different protection domain then:
   set_dev_pasid(sva_domain, devA, pasidX) - SUCCESS
   set_dev_pasid(sva_domain, devB, pasidX) - Compatibility check fail
   Core will allocate new SVA domain (sva_domain_new)
   set_dev_pasid(sva_domain_new, devB, pasidX) - SUCCESS

  - We will track mmu notifier and other data required for invalidation in SVA
protection domain.

  - During invalidation, we will retrieve SVA protection domain using mmu
notifier. Use device protection domain which was tracked in this SVA domain for
invalidation.

Does above flow makes sense? I do have a code based on above flow. I will try to
post soon.



> 
> This is why the core code helps the driver by de-duplicating the SVA
> domains, it can assume the iommu_domain is already minimal and it can
> then safely place the notifier there. The drivers should not try to
> de-duplicate the notifier with refcounting/etc.
> 
>>> These details would be clearer if you start from enabling PASID
>>> support for an UNMANAGED domain.
>>
>> We don't support PASID with V1 page table.
> 
> I didn't say V1 page table, I said enabling PASID support for
> UNMANGED domains. This means you need a flavour of UNMANAGED domain
> that is V2 page table. Just like ARM does. You already have this
> support in the driver.
> 
>>>> +static void sva_mn_invalidate_range(struct mmu_notifier *mn,
>>>> +				    struct mm_struct *mm,
>>>> +				    unsigned long start, unsigned long end)
>>>> +{
>>>> +	struct amd_sva_pasid *sva_pasid;
>>>> +	struct amd_sva_dev *sva_dev;
>>>> +
>>>> +	rcu_read_lock();
>>>> +
>>>> +	sva_pasid = container_of(mn, struct amd_sva_pasid, mn);
>>>> +	if (!sva_pasid) {
>>>> +		rcu_read_unlock();
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	list_for_each_entry_rcu(sva_dev, &sva_pasid->dev_list, list) {
>>>> +		if ((start ^ (end - 1)) < PAGE_SIZE)
>>>> +			amd_iommu_flush_page(sva_dev->dev_data->domain, sva_pasid->pasid, start);
>>>> +		else
>>>> +			amd_iommu_flush_tlb(sva_dev->dev_data->domain, sva_pasid->pasid);
>>>> +	}
>>>
>>> SVA invalidation should be the same as normal PASID invalidation. You
>>
>> Its same (Currently we have two different functions based on PAGE_SIZE. We have
>> a separate series to improve our invalidation logic).
> 
> It is not the same, you have this weird sva_pasid thing in here. PASID
> is NOT part of the SVA layer.
> 
> The API expects UNMANAGED domains will support PASID attach as well,
> that is a significant use case.

Can you elaborate the use cases you are referring here?

We do have use cases for PASID and PASID+PRI. But I am not aware of any use case
for UNMANAGED domain.


> 
>>> Use container_of(mn) to get back to the SVA protection domain and then
>>> you can access the protection domains list of PASIDs & masters.
>>
>> I don't think I understood this. Even with Tina's patch series, we can get the
>> SVA domain list. But we don't have a way to get device base protection domain.
> 
> Tina's patch series allows you to place the mmu_notifier struct
> directly in the protection_domain struct.
> 
> When you get the invalidate() callback you go back to the
> iommu_domain/protection domain that is affiliated with this MM.
> 
> From there you have all the information you can possibly need:
> 
>  - Global information in the protect_domain shared by all the devices
>    (eg ARM uses this to put the shared cache tag)
> 
>  - Per attachment information (RID & PASID/etc) for each attachment.
>    Iterate over this list to get the PASID/etc.

Got it. Thanks.


> 
>>>> +static int sva_bind_mm(struct device *dev, struct mm_struct *mm)
>>>> +{
>>>> +	struct amd_sva_pasid *sva_pasid;
>>>> +	struct amd_sva_dev *sva_dev;
>>>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>>>> +	int ret = -EINVAL;
>>>> +
>>>> +	sva_dev = sva_dev_alloc(dev);
>>>> +	if (!sva_dev)
>>>> +		return ret;
>>>> +
>>>> +	sva_pasid = sva_pasid_private_find(mm->pasid);
>>>> +	if (!sva_pasid) {
>>>> +		sva_pasid = sva_pasid_alloc(mm);
>>>> +		if (!sva_pasid)
>>>> +			goto out_sva_dev;
>>>> +
>>>> +		ret = sva_pasid_private_add(sva_pasid->pasid, sva_pasid);
>>>> +		if (ret)
>>>> +			goto out_sva_pasid;
>>>> +
>>>> +		ret = amd_iommu_set_gcr3(dev_data, sva_pasid->pasid,
>>>> +					 iommu_virt_to_phys(sva_pasid->mm->pgd));
>>>> +		if (ret)
>>>> +			goto out_pasid_remove;
>>>> +
>>>> +		ret = mmu_notifier_register(&sva_pasid->mn, mm);
>>>> +		if (ret)
>>>> +			goto out_clear_gcr3;
>>>> +	}
>>>
>>> The mmu_notifier should be setup when the domain is allocated, not
>>> during bind.
>>
>> We need to program (at least with AMD IOMMU) device PASID table before it can
>> handle the invalidation notifications. Not sure how it will work if we move mmu
>> notifier setup to domain allocation path.
> 
> The device list will be empty so there will be no PASIDs to
> invalidate at this point. There will be seperate locking to protect
> the device list, ensure that locking properly covers populating the
> device list and setting up the HW to respond to the PASID.
> 
>>>> +void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>>>> +{
>>>> +	struct iommu_domain *domain;
>>>> +
>>>> +	if (pasid == 0 || pasid >= dev->iommu->max_pasids)
>>>> +		return;
>>>
>>> We should probably have the core code pass in the old domain to this
>>> function, it is looking more like a mistake we didn't do that.
>>
>> I don't think I understood this. Well, honestly I never understood why
>> remove_dev_pasid() is part of iommu_ops while set_dev_pasid is part of domain ops.
> 
> Yeah, that is a bit weird, not sure there is a good reason
> 
> Regardless, the API should take in the current domain parmeter and
> drivers shouldn't call the core to look it up in the xarray if that is
> what drivers need to do.

Yeah. All I care is retrieving SVA domain. Using SVA domain I can get the
protection domain and do rest of the stuff.

With all new changes, it makes sense to pass sva domain to remove_dev_pasid.

-Vasant