RE: [PATCH v4 13/17] intel_iommu: piotlb invalidation should notify unmap

[Duan, Zhenzhong]

>-----Original Message-----
>From: Liu, Yi L <yi.l.liu@intel.com>
>Sent: Monday, November 4, 2024 11:05 AM
>Subject: Re: [PATCH v4 13/17] intel_iommu: piotlb invalidation should notify
>unmap
>
>On 2024/9/30 17:26, Zhenzhong Duan wrote:
>> This is used by some emulated devices which caches address
>> translation result. When piotlb invalidation issued in guest,
>> those caches should be refreshed.
>>
>> For device that does not implement ATS capability or disable
>> it but still caches the translation result, it is better to
>> implement ATS cap or enable it if there is need to cache the
>> translation result.
>
>Is there a list of such devices? Though I don't object this patch,
>but it may be helpful to list such devices. One day we may remove
>this when the list becomes empty.

Currently only virtio pci device support ATS and only vhost variant caches
translation result even if ATS disabled.

>
>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com>
>> ---
>>   hw/i386/intel_iommu.c | 35 ++++++++++++++++++++++++++++++++++-
>>   1 file changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 5ea59167b3..91d7b1abfa 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -2908,7 +2908,7 @@ static void
>vtd_piotlb_pasid_invalidate(IntelIOMMUState *s,
>>                   continue;
>>               }
>>
>> -            if (!s->scalable_modern) {
>> +            if (!s->scalable_modern || !vtd_as_has_map_notifier(vtd_as)) {
>>                   vtd_address_space_sync(vtd_as);
>>               }
>>           }
>> @@ -2920,6 +2920,9 @@ static void
>vtd_piotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,
>>                                          bool ih)
>>   {
>>       VTDIOTLBPageInvInfo info;
>> +    VTDAddressSpace *vtd_as;
>> +    VTDContextEntry ce;
>> +    hwaddr size = (1 << am) * VTD_PAGE_SIZE;
>>
>>       info.domain_id = domain_id;
>>       info.pasid = pasid;
>> @@ -2930,6 +2933,36 @@ static void
>vtd_piotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,
>>       g_hash_table_foreach_remove(s->iotlb,
>>                                   vtd_hash_remove_by_page_piotlb, &info);
>>       vtd_iommu_unlock(s);
>> +
>> +    QLIST_FOREACH(vtd_as, &s->vtd_as_with_notifiers, next) {
>> +        if (!vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),
>> +                                      vtd_as->devfn, &ce) &&
>> +            domain_id == vtd_get_domain_id(s, &ce, vtd_as->pasid)) {
>> +            uint32_t rid2pasid = VTD_CE_GET_RID2PASID(&ce);
>> +            IOMMUTLBEvent event;
>> +
>> +            if ((vtd_as->pasid != PCI_NO_PASID || pasid != rid2pasid) &&
>> +                vtd_as->pasid != pasid) {
>> +                continue;
>
>not quite get the logic here. patch 4 has a similar logic.

This code means we need to invalidate device tlb either when pasid matches 
address space's pasid or when pasid matches rid2pasid if this address space has 
no pasid.

Yes, patch4 only deal with stage-2, while this patch deal with stage-1.

Thanks
Zhenzhong

>
>> +            }
>> +
>> +            /*
>> +             * Page-Selective-within-PASID PASID-based-IOTLB Invalidation
>> +             * does not flush stage-2 entries. See spec section 6.5.2.4
>> +             */
>> +            if (!s->scalable_modern) {
>> +                continue;
>> +            }
>> +
>> +            event.type = IOMMU_NOTIFIER_UNMAP;
>> +            event.entry.target_as = &address_space_memory;
>> +            event.entry.iova = addr;
>> +            event.entry.perm = IOMMU_NONE;
>> +            event.entry.addr_mask = size - 1;
>> +            event.entry.translated_addr = 0;
>> +            memory_region_notify_iommu(&vtd_as->iommu, 0, event);
>> +        }
>> +    }
>>   }
>>
>>   static bool vtd_process_piotlb_desc(IntelIOMMUState *s,
>
>--
>Regards,
>Yi Liu