RE: [PATCH v1 04/19] intel_iommu: Fill the PASID field when creating an IOMMUTLBEntry

[Duan, Zhenzhong]

Hi Clement,

>-----Original Message-----
>From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>Subject: [PATCH v1 04/19] intel_iommu: Fill the PASID field when creating an
>IOMMUTLBEntry
>
>From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>
>PASID value must be used by devices as a key (or part of a key)
>when populating their ATC with the IOTLB entries returned by the IOMMU.
>
>Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>---
> hw/i386/intel_iommu.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
>diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>index aad132e367..a92ef9fe74 100644
>--- a/hw/i386/intel_iommu.c
>+++ b/hw/i386/intel_iommu.c
>@@ -2119,6 +2119,9 @@ static bool vtd_do_iommu_translate(VTDAddressSpace
>*vtd_as, PCIBus *bus,
>
>     vtd_iommu_lock(s);
>
>+    /* fill the pasid before getting rid2pasid */
>+    entry->pasid = pasid;

Seems unnecessary because vtd_iommu_translate() already assigned it.

>+
>     cc_entry = &vtd_as->context_cache_entry;
>
>     /* Try to fetch pte from IOTLB, we don't need RID2PASID logic */
>@@ -2260,6 +2263,7 @@ out:
>     entry->translated_addr = vtd_get_pte_addr(pte, s->aw_bits) & page_mask;
>     entry->addr_mask = ~page_mask;
>     entry->perm = access_flags;
>+    /* pasid already set */
>     return true;
>
> error:
>@@ -2268,6 +2272,7 @@ error:
>     entry->translated_addr = 0;
>     entry->addr_mask = 0;
>     entry->perm = IOMMU_NONE;
>+    entry->pasid = PCI_NO_PASID;

Shouldn't we keep original pasid value?

>     return false;
> }
>
>@@ -2511,6 +2516,7 @@ static void
>vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
>                         .translated_addr = 0,
>                         .addr_mask = size - 1,
>                         .perm = IOMMU_NONE,
>+                        .pasid = pasid,

Shouldn't we assign vtd_dev_as->pasid?

>                     },
>                 };
>                 memory_region_notify_iommu(&vtd_as->iommu, 0, event);
>@@ -3098,6 +3104,7 @@ static void do_invalidate_device_tlb(VTDAddressSpace
>*vtd_dev_as,
>     event.entry.iova = addr;
>     event.entry.perm = IOMMU_NONE;
>     event.entry.translated_addr = 0;
>+    event.entry.pasid = vtd_dev_as->pasid;
>     memory_region_notify_iommu(&vtd_dev_as->iommu, 0, event);
> }
>
>@@ -3680,6 +3687,7 @@ static IOMMUTLBEntry
>vtd_iommu_translate(IOMMUMemoryRegion *iommu, hwaddr addr,
>     IOMMUTLBEntry iotlb = {
>         /* We'll fill in the rest later. */
>         .target_as = &address_space_memory,
>+        .pasid = vtd_as->pasid,
>     };
>     bool success;
>
>@@ -3692,6 +3700,7 @@ static IOMMUTLBEntry
>vtd_iommu_translate(IOMMUMemoryRegion *iommu, hwaddr addr,
>         iotlb.translated_addr = addr & VTD_PAGE_MASK_4K;
>         iotlb.addr_mask = ~VTD_PAGE_MASK_4K;
>         iotlb.perm = IOMMU_RW;
>+        iotlb.pasid = PCI_NO_PASID;

Shouldn't we keep earlier assigned pasid value as above?

Thanks
Zhenzhong