diff mbox series

[v1,14/17] intel_iommu: piotlb invalidation should notify unmap

Message ID 20240718081636.879544-15-zhenzhong.duan@intel.com (mailing list archive)
State New, archived
Headers show
Series intel_iommu: Enable stage-1 translation for emulated device | expand

Commit Message

Duan, Zhenzhong July 18, 2024, 8:16 a.m. UTC 
This is used by some emulated devices which caches address
translation result. When piotlb invalidation issued in guest,
those caches should be refreshed.

Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/i386/intel_iommu.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

Comments

CLEMENT MATHIEU--DRIF July 24, 2024, 5:45 a.m. UTC | #1 
Maybe I'm missing something but why do we invalidate device IOTLB
upon piotlb receipt of a regular IOTLB inv desc?
I don't get why we don't wait for a device IOTLB inv desc?

On 18/07/2024 10:16, Zhenzhong Duan wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
> This is used by some emulated devices which caches address
> translation result. When piotlb invalidation issued in guest,
> those caches should be refreshed.
>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.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 8b66d6cfa5..c0116497b1 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2910,7 +2910,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);
>               }
>           }
> @@ -2922,6 +2922,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;
> @@ -2932,6 +2935,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
>
CLEMENT MATHIEU--DRIF July 24, 2024, 6:04 a.m. UTC | #2 
On 24/07/2024 07:45, CLEMENT MATHIEU--DRIF wrote:
> Maybe I'm missing something but why do we invalidate device IOTLB
> upon piotlb receipt of a regular IOTLB inv desc?
> I don't get why we don't wait for a device IOTLB inv desc?
I thought you were planning to remove that after the last rfc version
>
> On 18/07/2024 10:16, Zhenzhong Duan wrote:
>> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>>
>>
>> This is used by some emulated devices which caches address
>> translation result. When piotlb invalidation issued in guest,
>> those caches should be refreshed.
>>
>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.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 8b66d6cfa5..c0116497b1 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -2910,7 +2910,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);
>>                }
>>            }
>> @@ -2922,6 +2922,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;
>> @@ -2932,6 +2935,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
>>
Duan, Zhenzhong July 24, 2024, 6:07 a.m. UTC | #3 
>-----Original Message-----
>From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>Subject: Re: [PATCH v1 14/17] intel_iommu: piotlb invalidation should
>notify unmap
>
>
>
>On 24/07/2024 07:45, CLEMENT MATHIEU--DRIF wrote:
>> Maybe I'm missing something but why do we invalidate device IOTLB
>> upon piotlb receipt of a regular IOTLB inv desc?
>> I don't get why we don't wait for a device IOTLB inv desc?
>I thought you were planning to remove that after the last rfc version

Look at vtd_iotlb_page_invalidate(), it has same operation.
Reason is even if we don't enable device IOTLB, devices such as vhost may still caches IOTLB entries. So we need to flush those stale IOTLB entries in this case.

Thanks
Zhenzhong

>>
>> On 18/07/2024 10:16, Zhenzhong Duan wrote:
>>> Caution: External email. Do not open attachments or click links, unless
>this email comes from a known sender and you know the content is safe.
>>>
>>>
>>> This is used by some emulated devices which caches address
>>> translation result. When piotlb invalidation issued in guest,
>>> those caches should be refreshed.
>>>
>>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.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 8b66d6cfa5..c0116497b1 100644
>>> --- a/hw/i386/intel_iommu.c
>>> +++ b/hw/i386/intel_iommu.c
>>> @@ -2910,7 +2910,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);
>>>                }
>>>            }
>>> @@ -2922,6 +2922,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;
>>> @@ -2932,6 +2935,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
>>>
CLEMENT MATHIEU--DRIF July 24, 2024, 6:11 a.m. UTC | #4 
On 24/07/2024 08:07, Duan, Zhenzhong wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
>> -----Original Message-----
>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>> Subject: Re: [PATCH v1 14/17] intel_iommu: piotlb invalidation should
>> notify unmap
>>
>>
>>
>> On 24/07/2024 07:45, CLEMENT MATHIEU--DRIF wrote:
>>> Maybe I'm missing something but why do we invalidate device IOTLB
>>> upon piotlb receipt of a regular IOTLB inv desc?
>>> I don't get why we don't wait for a device IOTLB inv desc?
>> I thought you were planning to remove that after the last rfc version
> Look at vtd_iotlb_page_invalidate(), it has same operation.
> Reason is even if we don't enable device IOTLB, devices such as vhost may still caches IOTLB entries. So we need to flush those stale IOTLB entries in this case.
>
> Thanks
> Zhenzhong
Ok fine,

Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com>

>
>>> On 18/07/2024 10:16, Zhenzhong Duan wrote:
>>>> Caution: External email. Do not open attachments or click links, unless
>> this email comes from a known sender and you know the content is safe.
>>>>
>>>> This is used by some emulated devices which caches address
>>>> translation result. When piotlb invalidation issued in guest,
>>>> those caches should be refreshed.
>>>>
>>>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.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 8b66d6cfa5..c0116497b1 100644
>>>> --- a/hw/i386/intel_iommu.c
>>>> +++ b/hw/i386/intel_iommu.c
>>>> @@ -2910,7 +2910,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);
>>>>                 }
>>>>             }
>>>> @@ -2922,6 +2922,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;
>>>> @@ -2932,6 +2935,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
>>>>
diff mbox series

Patch

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 8b66d6cfa5..c0116497b1 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2910,7 +2910,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);
             }
         }
@@ -2922,6 +2922,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;
@@ -2932,6 +2935,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,