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

Zhenzhong Duan posted 17 patches 1 month, 2 weeks ago
There is a newer version of this series
[PATCH v4 13/17] intel_iommu: piotlb invalidation should notify unmap
Posted by Zhenzhong Duan 1 month, 2 weeks ago
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.

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;
+            }
+
+            /*
+             * 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,
-- 
2.34.1


Re: [PATCH v4 13/17] intel_iommu: piotlb invalidation should notify unmap
Posted by Jason Wang 5 days, 23 hours ago
On Mon, Sep 30, 2024 at 5:30 PM Zhenzhong Duan <zhenzhong.duan@intel.com> 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.
>
> 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>
> ---

Acked-by: Jason Wang <jasowang@redhat.com>

Thank
Re: [PATCH v4 13/17] intel_iommu: piotlb invalidation should notify unmap
Posted by Yi Liu 1 week, 3 days ago
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.

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

> +            }
> +
> +            /*
> +             * 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

RE: [PATCH v4 13/17] intel_iommu: piotlb invalidation should notify unmap
Posted by Duan, Zhenzhong 1 week, 2 days ago

>-----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
Re: [PATCH v4 13/17] intel_iommu: piotlb invalidation should notify unmap
Posted by Yi Liu 1 week, 1 day ago
On 2024/11/4 16:15, Duan, Zhenzhong wrote:
> 
>> 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.

vtd_iotlb_page_invalidate_notify() has a similar check as well. But
it checks PCI_NO_PASID against the pasid instead of vtd_pas->pasid.
So it looks confusing to me.


-- 
Regards,
Yi Liu
RE: [PATCH v4 13/17] intel_iommu: piotlb invalidation should notify unmap
Posted by Duan, Zhenzhong 1 week, 1 day ago

>-----Original Message-----
>From: Liu, Yi L <yi.l.liu@intel.com>
>Sent: Tuesday, November 5, 2024 2:30 PM
>Subject: Re: [PATCH v4 13/17] intel_iommu: piotlb invalidation should notify
>unmap
>
>On 2024/11/4 16:15, Duan, Zhenzhong wrote:
>>
>>> 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.
>
>vtd_iotlb_page_invalidate_notify() has a similar check as well. But
>it checks PCI_NO_PASID against the pasid instead of vtd_pas->pasid.
>So it looks confusing to me.

Yes, it's about difference of piotlb invalidation vs. iotlb invalidation.

vtd_piotlb_page_invalidate() has a parameter pasid which come from piotlb
invalidation descriptor and should never be PCI_NO_PASID(-1).

For vtd_iotlb_page_invalidate_notify(), pasid param is always PCI_NO_PASID
for iotlb Invalidation, reason is iotlb invalidation doesn't support pasid.

I had ever thought about expending and reusing vtd_iotlb_page_invalidate_notify() for
piotlb invalidation, but it's cleaner to have a separate code for piotlb invalidation.

Thanks
Zhenzhong