Re: About unmap pages and set dirty tracking on nested parent domain

[Yi Liu <yi.l.liu@intel.com>]

On 2024/1/25 22:03, Jason Gunthorpe wrote:
> On Thu, Jan 25, 2024 at 09:55:46PM +0800, Yi Liu wrote:
>> Hi Jason, Kevin,
>>
>> Today, Intel iommu driver only tracks attached devices/iommus in the nested
>> domain. While the nested parent domain does not.
> 
> Heh, I was just looking at this bug on my ARM implemention too :)
> 
>> This makes cache flush on nested parent domain be a nop if it's only
>> used as parent.
> 
> Yep, this is wrong.
> 
>> 1) Do we want to allow unmap pages on nested parent domain?
> 
> Yes. It is needed for memory unplug.
> 
>>     Today there is
>>     no PRQ support on the nested parent domain (stage-2). That's why both
>>     VFIO and IOMMUFD pins the page in the DMA_MAP. As a result, VMMs like
>>     Qemu cannot not unamp pages in nested parent domain after VM is
>>     running.
> 
> No, qemu can do an explicit unmap command to iommufd. PRQ is not relavent

Indeed!!!

> 
>> 2) If answer of 1) is yes. Should the owner of stage-1 be notified about
>>     the unmap event on its nested parent, hence owner is able to flush the
>>     corresponding stage-1 cache explicitly?
> 
> No. We don't support "mdev" "access" operations on nests so there is
> no reason to notify anyone. If qemu hot unplugs memory from a VM then
> it should already have some idea that the VM is not doing DMA to that
> memory.

makes sense.

> 
>> 3) Is it enough to fix this gap within iommu driver? or need to be handled
>>     in the generic layer? e.g. let the iommufd layer to track stage-1 hwpts
>>     in stage-2 hwpt. In this way, iommufd can flush stage-1 cache when
>>     unmapping pages on stage-2.
> 
> I think the iommu driver should fix it

okay.

> Notice there is also an ATC requirement here, when the nesting parent
> changes it needs to issue a full ATC flush on PASID 0, not a range
> flush.

right. I missed this part. Thanks for pointing it out.

> Also notice the iommu probably has to zap the entire IOTLB for any
> nesting child if the parent changes, unless it has amazing HW :)

VT-d seems to be the amazing HW :) It can flush the stage-1 caches that 
refers the stage-2 mapping during stage-2 cache invalidation.

> It would be really awkward to try to lift this detail out of the
> driver.

ok. let's do it in iommu driver.

> Lets add Suravee to be sure the AMD driver is aware of this detail
> too.
> 
> My plan is to have the nesting attach add the device to the parent
> domain's invalidation list and have a flag in the master_domain to
> indicate this attachment has the special ATC invalidation.
> 
> This will allow the S2 to be used normally as well, eg for the
> identity map.

I've considered this way as well. However, this means a single device (say
device_domain_info) at least needs two list_head to link into the
stage-1 and stage-2 domain. It may result in some inconvenience in
the existing single stage (e.g. stage-2) code. Also, this means we
need to track the iommu in the stage-2 domain as well. This means
in nested attach, there will be two domain/iommu association. This
will get some extra complexity in the domain ID determination. Need
to ensure the two association uses the same domain ID. To be simple,
I planned to have a list tracking stage-1 domains in its parent domain.
When flushing cache, set dirty tracking, the helper should loop the
stage-1 domain list and loop the devices/iommus accordingly. While for
the device tlb flush, just flush the entire addr range (0 - MAX). My
code is in the below branch with some other fixes. Need more tests.. :)

a3b8450962b6 iommu/vt-d: Set up dirty tracking for nested parent domain
85879c330286 iommu/vt-d: Add missing cache flush on nested parent domain
ac22f14b2612 iommu/vt-d: Wrap page selective iotlb flush for domain
7311a31614dc iommu/vt-d: Track nested domains on the same parent

https://github.com/yiliu1765/iommufd/tree/wip/iommufd_nesting_fixes

-- 
Regards,
Yi Liu