[Qemu-devel] [PATCH 0/3] s390x/pci: fixup and optimize IOTLB code

[Qemu-devel] [PATCH 0/3] s390x/pci: fixup and optimize IOTLB code

[Yi Min Zhao]

This series contains three patches,
1) optimizes the code including walking DMA tables and rpcit handler
2) fixes the issue caused by IOTLB global refresh 
3) uses the right pal and pba when registering ioat

The issue mentioned above was found when we tested SMC-r tools. This
behavior has been introduced when linux guest started using a global
refresh to purge the whole IOTLB of invalid entries in a lazy fashion
instead of flushing each entry when invalidating table entries.

The previous QEMU implementation didn't keep track of the mapping,
didn't handle correctly the global flush demand from the guest and a
major part of the IOTLB entries were not flushed.

Consequently linux kernel on the host keeping the previous mapping
reports, as it should, -EEXIST DMA mapping error on the next mapping
with the same IOVA. The second patch fixes this issue.

During the investigation, we noticed that the current code walking
PCI IOMMU page tables didn't check important flags of table entries,
including:
1) protection bit
2) table length
3) table offset
4) intermediate tables' invalid bit
5) format control bit

We implement the checking in the first patch before handling the
IOTLB global refresh issue. To keep track of the mapped IOTLB entries
and be able to check if the host IOTLB entries need to be refreshed
we implement a IOTLB cache in QEMU, and introduce some helper
functions to check these bits. All S390IOTLBEntry instances are stored
in a new hashtable which are indexed by IOVA. Each PCI device has its
own IOMMU. Therefore each IOMMU also has its own hashtable caching
corresponding PCI device's DMA entries. Finally, we split 1M
contiguous DMA range into 4K pages to do DMA map, and the code about
error notification is also optimized.

Yi Min Zhao (3):
  s390x/pci: fixup the code walking IOMMU tables
  s390x/pci: fixup global refresh
  s390x/pci: use the right pal and pba in reg_ioat()

 hw/s390x/s390-pci-bus.c  | 233 ++++++++++++++++++++++++++++++++++++++---------
 hw/s390x/s390-pci-bus.h  |  13 +++
 hw/s390x/s390-pci-inst.c | 103 ++++++++++++++-------
 3 files changed, 271 insertions(+), 78 deletions(-)

-- 
2.14.3 (Apple Git-98)

[Qemu-devel] [PATCH 1/3] s390x/pci: fixup the code walking IOMMU tables

[Yi Min Zhao]

Current s390x PCI IOMMU code is lack of flags' checking, including:
1) protection bit
2) table length
3) table offset
4) intermediate tables' invalid bit
5) format control bit

This patch introduces a new struct named S390IOTLBEntry, and makes up
these missed checkings. At the same time, inform the guest with the
corresponding error number when the check fails.

Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
---
 hw/s390x/s390-pci-bus.c  | 223 ++++++++++++++++++++++++++++++++++++++---------
 hw/s390x/s390-pci-bus.h  |  10 +++
 hw/s390x/s390-pci-inst.c |  10 ---
 3 files changed, 190 insertions(+), 53 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 2b1e1409bf..e349d73abe 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -309,49 +309,186 @@ static uint64_t get_st_pto(uint64_t entry)
             : 0;
 }
 
-static uint64_t s390_guest_io_table_walk(uint64_t guest_iota,
-                                  uint64_t guest_dma_address)
+static bool rt_entry_isvalid(uint64_t entry)
 {
-    uint64_t sto_a, pto_a, px_a;
-    uint64_t sto, pto, pte;
-    uint32_t rtx, sx, px;
-
-    rtx = calc_rtx(guest_dma_address);
-    sx = calc_sx(guest_dma_address);
-    px = calc_px(guest_dma_address);
-
-    sto_a = guest_iota + rtx * sizeof(uint64_t);
-    sto = address_space_ldq(&address_space_memory, sto_a,
-                            MEMTXATTRS_UNSPECIFIED, NULL);
-    sto = get_rt_sto(sto);
-    if (!sto) {
-        pte = 0;
+    return (entry & ZPCI_TABLE_VALID_MASK) == ZPCI_TABLE_VALID;
+}
+
+static bool pt_entry_isvalid(uint64_t entry)
+{
+    return (entry & ZPCI_PTE_VALID_MASK) == ZPCI_PTE_VALID;
+}
+
+static bool entry_isprotected(uint64_t entry)
+{
+    return (entry & ZPCI_TABLE_PROT_MASK) == ZPCI_TABLE_PROTECTED;
+}
+
+/* ett is expected table type, -1 page table, 0 segment table, 1 region table */
+static uint64_t get_table_index(uint64_t iova, int8_t ett)
+{
+    switch (ett) {
+    case -1:
+        return calc_px(iova);
+    case 0:
+        return calc_sx(iova);
+    case 1:
+        return calc_rtx(iova);
+    }
+
+    return -1;
+}
+
+static bool entry_isvalid(uint64_t entry, int8_t ett)
+{
+    switch (ett) {
+    case -1:
+        return pt_entry_isvalid(entry);
+    case 0:
+    case 1:
+        return rt_entry_isvalid(entry);
+    }
+
+    return false;
+}
+
+/* Return true if address translation is done */
+static bool translate_iscomplete(uint64_t entry, int8_t ett)
+{
+    switch (ett) {
+    case 0:
+        return (entry & ZPCI_TABLE_FC) ? true : false;
+    case 1:
+        return false;
+    }
+
+    return true;
+}
+
+static uint64_t get_frame_size(int8_t ett)
+{
+    switch (ett) {
+    case -1:
+        return 1ULL << 12;
+    case 0:
+        return 1ULL << 20;
+    case 1:
+        return 1ULL << 31;
+    }
+
+    return 0;
+}
+
+static uint64_t get_next_table_origin(uint64_t entry, int8_t ett)
+{
+    switch (ett) {
+    case -1:
+        return entry & ZPCI_PTE_ADDR_MASK;
+    case 0:
+        return get_st_pto(entry);
+    case 1:
+        return get_rt_sto(entry);
+    }
+
+    return 0;
+}
+
+/**
+ * table_translate: do translation within one table and return the following
+ *                  table origin
+ *
+ * @entry: the entry being traslated, the result is stored in this.
+ * @to: the address of table origin.
+ * @ett: expected table type, 1 region table, 0 segment table and -1 page table.
+ * @error: error code
+ */
+static uint64_t table_translate(S390IOTLBEntry *entry, uint64_t to, int8_t ett,
+                                uint16_t *error)
+{
+    uint64_t tx, te, nto = 0;
+    uint16_t err = 0;
+
+    tx = get_table_index(entry->iova, ett);
+    te = address_space_ldq(&address_space_memory, to + tx * sizeof(uint64_t),
+                           MEMTXATTRS_UNSPECIFIED, NULL);
+
+    if (!te) {
+        err = ERR_EVENT_INVALTE;
         goto out;
     }
 
-    pto_a = sto + sx * sizeof(uint64_t);
-    pto = address_space_ldq(&address_space_memory, pto_a,
-                            MEMTXATTRS_UNSPECIFIED, NULL);
-    pto = get_st_pto(pto);
-    if (!pto) {
-        pte = 0;
+    if (!entry_isvalid(te, ett)) {
+        entry->perm &= IOMMU_NONE;
         goto out;
     }
 
-    px_a = pto + px * sizeof(uint64_t);
-    pte = address_space_ldq(&address_space_memory, px_a,
-                            MEMTXATTRS_UNSPECIFIED, NULL);
+    if (ett == 1 && ((te & ZPCI_TABLE_LEN_RTX) != ZPCI_TABLE_LEN_RTX ||
+                     te & ZPCI_TABLE_OFFSET_MASK)) {
+        err = ERR_EVENT_INVALTL;
+        goto out;
+    }
 
+    nto = get_next_table_origin(te, ett);
+    if (!nto) {
+        err = ERR_EVENT_TT;
+        goto out;
+    }
+
+    if (entry_isprotected(te)) {
+        entry->perm &= IOMMU_RO;
+    } else {
+        entry->perm &= IOMMU_RW;
+    }
+
+    if (translate_iscomplete(te, ett)) {
+        switch (ett) {
+        case -1:
+            entry->translated_addr = te & ZPCI_PTE_ADDR_MASK;
+            break;
+        case 0:
+            entry->translated_addr = (te & ZPCI_SFAA_MASK) |
+                (entry->iova & ~ZPCI_SFAA_MASK);
+            break;
+        }
+        nto = 0;
+    }
 out:
-    return pte;
+    if (err) {
+        entry->perm = IOMMU_NONE;
+        *error = err;
+    }
+    entry->len = get_frame_size(ett);
+    return nto;
+}
+
+static uint16_t s390_guest_io_table_walk(uint64_t g_iota, hwaddr addr,
+                                         S390IOTLBEntry *entry)
+{
+    uint64_t to = s390_pci_get_table_origin(g_iota);
+    int8_t ett = 1;
+    uint16_t error = 0;
+
+    entry->iova = addr & PAGE_MASK;
+    entry->translated_addr = 0;
+    entry->perm = IOMMU_RW;
+
+    if (entry_isprotected(g_iota)) {
+        entry->perm &= IOMMU_RO;
+    }
+
+    while (to) {
+        to = table_translate(entry, to, ett--, &error);
+    }
+
+    return error;
 }
 
 static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
                                           IOMMUAccessFlags flag)
 {
-    uint64_t pte;
-    uint32_t flags;
     S390PCIIOMMU *iommu = container_of(mr, S390PCIIOMMU, iommu_mr);
+    S390IOTLBEntry entry;
+    uint16_t error = 0;
     IOMMUTLBEntry ret = {
         .target_as = &address_space_memory,
         .iova = 0,
@@ -374,26 +511,26 @@ static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
     DPRINTF("iommu trans addr 0x%" PRIx64 "\n", addr);
 
     if (addr < iommu->pba || addr > iommu->pal) {
-        return ret;
+        error = ERR_EVENT_OORANGE;
+        goto err;
     }
 
-    pte = s390_guest_io_table_walk(s390_pci_get_table_origin(iommu->g_iota),
-                                   addr);
-    if (!pte) {
-        return ret;
-    }
+    error = s390_guest_io_table_walk(iommu->g_iota, addr, &entry);
 
-    flags = pte & ZPCI_PTE_FLAG_MASK;
-    ret.iova = addr;
-    ret.translated_addr = pte & ZPCI_PTE_ADDR_MASK;
-    ret.addr_mask = 0xfff;
+    ret.iova = entry.iova;
+    ret.translated_addr = entry.translated_addr;
+    ret.addr_mask = entry.len - 1;
+    ret.perm = entry.perm;
 
-    if (flags & ZPCI_PTE_INVALID) {
-        ret.perm = IOMMU_NONE;
-    } else {
-        ret.perm = IOMMU_RW;
+    if ((flag != IOMMU_NONE) && !(flag & ret.perm)) {
+        error = ERR_EVENT_TPROTE;
+    }
+err:
+    if (error) {
+        iommu->pbdev->state = ZPCI_FS_ERROR;
+        s390_pci_generate_error_event(error, iommu->pbdev->fh,
+                                      iommu->pbdev->fid, addr, 0);
     }
-
     return ret;
 }
 
diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
index 2993f0ddef..ca22ef393b 100644
--- a/hw/s390x/s390-pci-bus.h
+++ b/hw/s390x/s390-pci-bus.h
@@ -148,6 +148,8 @@ enum ZpciIoatDtype {
 #define ZPCI_STE_FLAG_MASK      0x7ffULL
 #define ZPCI_STE_ADDR_MASK      (~ZPCI_STE_FLAG_MASK)
 
+#define ZPCI_SFAA_MASK          (~((1ULL << 20) - 1))
+
 /* I/O Page tables */
 #define ZPCI_PTE_VALID_MASK             0x400
 #define ZPCI_PTE_INVALID                0x400
@@ -165,6 +167,7 @@ enum ZpciIoatDtype {
 #define ZPCI_TABLE_INVALID              0x20
 #define ZPCI_TABLE_PROTECTED            0x200
 #define ZPCI_TABLE_UNPROTECTED          0x000
+#define ZPCI_TABLE_FC                   0x400
 
 #define ZPCI_TABLE_VALID_MASK           0x20
 #define ZPCI_TABLE_PROT_MASK            0x200
@@ -253,6 +256,13 @@ typedef struct S390MsixInfo {
     uint32_t pba_offset;
 } S390MsixInfo;
 
+typedef struct S390IOTLBEntry {
+    uint64_t iova;
+    uint64_t translated_addr;
+    uint64_t len;
+    uint64_t perm;
+} S390IOTLBEntry;
+
 typedef struct S390PCIBusDevice S390PCIBusDevice;
 typedef struct S390PCIIOMMU {
     Object parent_obj;
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index be449210d9..63fa06fb97 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -644,16 +644,6 @@ int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
 
     while (start < end) {
         entry = imrc->translate(iommu_mr, start, IOMMU_NONE);
-
-        if (!entry.translated_addr) {
-            pbdev->state = ZPCI_FS_ERROR;
-            setcc(cpu, ZPCI_PCI_LS_ERR);
-            s390_set_status_code(env, r1, ZPCI_PCI_ST_INSUF_RES);
-            s390_pci_generate_error_event(ERR_EVENT_SERR, pbdev->fh, pbdev->fid,
-                                          start, ERR_EVENT_Q_BIT);
-            goto out;
-        }
-
         memory_region_notify_iommu(iommu_mr, entry);
         start += entry.addr_mask + 1;
     }
-- 
2.14.3 (Apple Git-98)

[Qemu-devel] [PATCH 2/3] s390x/pci: fixup global refresh

[Yi Min Zhao]

The VFIO common code doesn't provide the possibility to modify a
previous mapping entry in another way than unmapping and mapping again
with new properties.

To avoid -EEXIST DMA mapping error, this we introduce a GHashTable to
store S390IOTLBEntry instances in order to cache the mapped entries.
When intercepting rpcit instruction, ignore the identical mapped
entries to avoid doing map operations multiple times and do unmap and
re-map operations for the case of updating the valid entries. To
achieve that goal, we also export the DMA walking function and
optimize the code handling errors in rpcit handler.

Acked-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
---
 hw/s390x/s390-pci-bus.c  | 28 +++++++++-----
 hw/s390x/s390-pci-bus.h  |  3 ++
 hw/s390x/s390-pci-inst.c | 95 ++++++++++++++++++++++++++++++++++--------------
 3 files changed, 90 insertions(+), 36 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index e349d73abe..b75af26db7 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -461,8 +461,8 @@ out:
     return nto;
 }
 
-static uint16_t s390_guest_io_table_walk(uint64_t g_iota, hwaddr addr,
-                                         S390IOTLBEntry *entry)
+uint16_t s390_guest_io_table_walk(uint64_t g_iota, hwaddr addr,
+                                  S390IOTLBEntry *entry)
 {
     uint64_t to = s390_pci_get_table_origin(g_iota);
     int8_t ett = 1;
@@ -487,7 +487,8 @@ static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
                                           IOMMUAccessFlags flag)
 {
     S390PCIIOMMU *iommu = container_of(mr, S390PCIIOMMU, iommu_mr);
-    S390IOTLBEntry entry;
+    S390IOTLBEntry *entry;
+    uint64_t iova = addr & PAGE_MASK;
     uint16_t error = 0;
     IOMMUTLBEntry ret = {
         .target_as = &address_space_memory,
@@ -515,12 +516,17 @@ static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
         goto err;
     }
 
-    error = s390_guest_io_table_walk(iommu->g_iota, addr, &entry);
-
-    ret.iova = entry.iova;
-    ret.translated_addr = entry.translated_addr;
-    ret.addr_mask = entry.len - 1;
-    ret.perm = entry.perm;
+    entry = g_hash_table_lookup(iommu->iotlb, &iova);
+    if (entry) {
+        ret.iova = entry->iova;
+        ret.translated_addr = entry->translated_addr;
+        ret.addr_mask = entry->len - 1;
+        ret.perm = entry->perm;
+    } else {
+        ret.iova = iova;
+        ret.addr_mask = ~PAGE_MASK;
+        ret.perm = IOMMU_NONE;
+    }
 
     if ((flag != IOMMU_NONE) && !(flag & ret.perm)) {
         error = ERR_EVENT_TPROTE;
@@ -572,6 +578,8 @@ static S390PCIIOMMU *s390_pci_get_iommu(S390pciState *s, PCIBus *bus,
                                         PCI_FUNC(devfn));
         memory_region_init(&iommu->mr, OBJECT(iommu), mr_name, UINT64_MAX);
         address_space_init(&iommu->as, &iommu->mr, as_name);
+        iommu->iotlb = g_hash_table_new_full(g_int64_hash, g_int64_equal,
+                                             NULL, g_free);
         table->iommu[PCI_SLOT(devfn)] = iommu;
 
         g_free(mr_name);
@@ -661,6 +669,7 @@ void s390_pci_iommu_enable(S390PCIIOMMU *iommu)
 void s390_pci_iommu_disable(S390PCIIOMMU *iommu)
 {
     iommu->enabled = false;
+    g_hash_table_remove_all(iommu->iotlb);
     memory_region_del_subregion(&iommu->mr, MEMORY_REGION(&iommu->iommu_mr));
     object_unparent(OBJECT(&iommu->iommu_mr));
 }
@@ -676,6 +685,7 @@ static void s390_pci_iommu_free(S390pciState *s, PCIBus *bus, int32_t devfn)
     }
 
     table->iommu[PCI_SLOT(devfn)] = NULL;
+    g_hash_table_destroy(iommu->iotlb);
     address_space_destroy(&iommu->as);
     object_unparent(OBJECT(&iommu->mr));
     object_unparent(OBJECT(iommu));
diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
index ca22ef393b..395bbf0e13 100644
--- a/hw/s390x/s390-pci-bus.h
+++ b/hw/s390x/s390-pci-bus.h
@@ -274,6 +274,7 @@ typedef struct S390PCIIOMMU {
     uint64_t g_iota;
     uint64_t pba;
     uint64_t pal;
+    GHashTable *iotlb;
 } S390PCIIOMMU;
 
 typedef struct S390PCIIOMMUTable {
@@ -330,6 +331,8 @@ void s390_pci_iommu_enable(S390PCIIOMMU *iommu);
 void s390_pci_iommu_disable(S390PCIIOMMU *iommu);
 void s390_pci_generate_error_event(uint16_t pec, uint32_t fh, uint32_t fid,
                                    uint64_t faddr, uint32_t e);
+uint16_t s390_guest_io_table_walk(uint64_t g_iota, hwaddr addr,
+                                  S390IOTLBEntry *entry);
 S390PCIBusDevice *s390_pci_find_dev_by_idx(S390pciState *s, uint32_t idx);
 S390PCIBusDevice *s390_pci_find_dev_by_fh(S390pciState *s, uint32_t fh);
 S390PCIBusDevice *s390_pci_find_dev_by_fid(S390pciState *s, uint32_t fid);
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 63fa06fb97..997a9cc2e9 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -571,27 +571,65 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
     return 0;
 }
 
+static void s390_pci_update_iotlb(S390PCIIOMMU *iommu, S390IOTLBEntry *entry)
+{
+    S390IOTLBEntry *cache = g_hash_table_lookup(iommu->iotlb, &entry->iova);
+    IOMMUTLBEntry notify = {
+        .target_as = &address_space_memory,
+        .iova = entry->iova,
+        .translated_addr = entry->translated_addr,
+        .perm = entry->perm,
+        .addr_mask = ~PAGE_MASK,
+    };
+
+    if (entry->perm == IOMMU_NONE) {
+        if (!cache) {
+            return;
+        }
+        g_hash_table_remove(iommu->iotlb, &entry->iova);
+    } else {
+        if (cache) {
+            if (cache->perm == entry->perm &&
+                cache->translated_addr == entry->translated_addr) {
+                return;
+            }
+
+            notify.perm = IOMMU_NONE;
+            memory_region_notify_iommu(&iommu->iommu_mr, notify);
+            notify.perm = entry->perm;
+        }
+
+        cache = g_new(S390IOTLBEntry, 1);
+        cache->iova = entry->iova;
+        cache->translated_addr = entry->translated_addr;
+        cache->len = PAGE_SIZE;
+        cache->perm = entry->perm;
+        g_hash_table_replace(iommu->iotlb, &cache->iova, cache);
+    }
+
+    memory_region_notify_iommu(&iommu->iommu_mr, notify);
+}
+
 int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
 {
     CPUS390XState *env = &cpu->env;
     uint32_t fh;
+    uint16_t error = 0;
     S390PCIBusDevice *pbdev;
     S390PCIIOMMU *iommu;
+    S390IOTLBEntry entry;
     hwaddr start, end;
-    IOMMUTLBEntry entry;
-    IOMMUMemoryRegion *iommu_mr;
-    IOMMUMemoryRegionClass *imrc;
 
     cpu_synchronize_state(CPU(cpu));
 
     if (env->psw.mask & PSW_MASK_PSTATE) {
         s390_program_interrupt(env, PGM_PRIVILEGED, 4, ra);
-        goto out;
+        return 0;
     }
 
     if (r2 & 0x1) {
         s390_program_interrupt(env, PGM_SPECIFICATION, 4, ra);
-        goto out;
+        return 0;
     }
 
     fh = env->regs[r1] >> 32;
@@ -602,7 +640,7 @@ int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
     if (!pbdev) {
         DPRINTF("rpcit no pci dev\n");
         setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
-        goto out;
+        return 0;
     }
 
     switch (pbdev->state) {
@@ -622,34 +660,37 @@ int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
 
     iommu = pbdev->iommu;
     if (!iommu->g_iota) {
-        pbdev->state = ZPCI_FS_ERROR;
-        setcc(cpu, ZPCI_PCI_LS_ERR);
-        s390_set_status_code(env, r1, ZPCI_PCI_ST_INSUF_RES);
-        s390_pci_generate_error_event(ERR_EVENT_INVALAS, pbdev->fh, pbdev->fid,
-                                      start, 0);
-        goto out;
+        error = ERR_EVENT_INVALAS;
+        goto err;
     }
 
     if (end < iommu->pba || start > iommu->pal) {
-        pbdev->state = ZPCI_FS_ERROR;
-        setcc(cpu, ZPCI_PCI_LS_ERR);
-        s390_set_status_code(env, r1, ZPCI_PCI_ST_INSUF_RES);
-        s390_pci_generate_error_event(ERR_EVENT_OORANGE, pbdev->fh, pbdev->fid,
-                                      start, 0);
-        goto out;
+        error = ERR_EVENT_OORANGE;
+        goto err;
     }
 
-    iommu_mr = &iommu->iommu_mr;
-    imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr);
-
     while (start < end) {
-        entry = imrc->translate(iommu_mr, start, IOMMU_NONE);
-        memory_region_notify_iommu(iommu_mr, entry);
-        start += entry.addr_mask + 1;
-    }
+        error = s390_guest_io_table_walk(iommu->g_iota, start, &entry);
+        if (error) {
+            break;
+        }
 
-    setcc(cpu, ZPCI_PCI_LS_OK);
-out:
+        start += entry.len;
+        while (entry.iova < start && entry.iova < end) {
+            s390_pci_update_iotlb(iommu, &entry);
+            entry.iova += PAGE_SIZE;
+            entry.translated_addr += PAGE_SIZE;
+        }
+    }
+err:
+    if (error) {
+        pbdev->state = ZPCI_FS_ERROR;
+        setcc(cpu, ZPCI_PCI_LS_ERR);
+        s390_set_status_code(env, r1, ZPCI_PCI_ST_FUNC_IN_ERR);
+        s390_pci_generate_error_event(error, pbdev->fh, pbdev->fid, start, 0);
+    } else {
+        setcc(cpu, ZPCI_PCI_LS_OK);
+    }
     return 0;
 }
 
-- 
2.14.3 (Apple Git-98)

[Qemu-devel] [PATCH 3/3] s390x/pci: use the right pal and pba in reg_ioat()

[Yi Min Zhao]

When registering ioat, pba should be comprised of leftmost 52 bits and
rightmost 12 binary zeros, and pal should be comprised of leftmost 52
bits and right most 12 binary ones. Let's fixup this.

Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
---
 hw/s390x/s390-pci-inst.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 997a9cc2e9..3fcc330fe3 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -865,6 +865,8 @@ static int reg_ioat(CPUS390XState *env, S390PCIIOMMU *iommu, ZpciFib fib,
     uint8_t dt = (g_iota >> 2) & 0x7;
     uint8_t t = (g_iota >> 11) & 0x1;
 
+    pba &= ~0xfff;
+    pal |= 0xfff;
     if (pba > pal || pba < ZPCI_SDMA_ADDR || pal > ZPCI_EDMA_ADDR) {
         s390_program_interrupt(env, PGM_OPERAND, 6, ra);
         return -EINVAL;
-- 
2.14.3 (Apple Git-98)

Re: [Qemu-devel] [PATCH 1/3] s390x/pci: fixup the code walking IOMMU tables

[Thomas Huth]

On 30.01.2018 10:47, Yi Min Zhao wrote:
> Current s390x PCI IOMMU code is lack of flags' checking, including:
> 1) protection bit
> 2) table length
> 3) table offset
> 4) intermediate tables' invalid bit
> 5) format control bit
> 
> This patch introduces a new struct named S390IOTLBEntry, and makes up
> these missed checkings. At the same time, inform the guest with the
> corresponding error number when the check fails.
> 
> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-pci-bus.c  | 223 ++++++++++++++++++++++++++++++++++++++---------
>  hw/s390x/s390-pci-bus.h  |  10 +++
>  hw/s390x/s390-pci-inst.c |  10 ---
>  3 files changed, 190 insertions(+), 53 deletions(-)
[...]
> @@ -374,26 +511,26 @@ static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
>      DPRINTF("iommu trans addr 0x%" PRIx64 "\n", addr);
>  
>      if (addr < iommu->pba || addr > iommu->pal) {
> -        return ret;
> +        error = ERR_EVENT_OORANGE;
> +        goto err;
>      }
>  
> -    pte = s390_guest_io_table_walk(s390_pci_get_table_origin(iommu->g_iota),
> -                                   addr);
> -    if (!pte) {
> -        return ret;
> -    }
> +    error = s390_guest_io_table_walk(iommu->g_iota, addr, &entry);
>  
> -    flags = pte & ZPCI_PTE_FLAG_MASK;
> -    ret.iova = addr;
> -    ret.translated_addr = pte & ZPCI_PTE_ADDR_MASK;
> -    ret.addr_mask = 0xfff;
> +    ret.iova = entry.iova;
> +    ret.translated_addr = entry.translated_addr;
> +    ret.addr_mask = entry.len - 1;
> +    ret.perm = entry.perm;
>  
> -    if (flags & ZPCI_PTE_INVALID) {
> -        ret.perm = IOMMU_NONE;
> -    } else {
> -        ret.perm = IOMMU_RW;
> +    if ((flag != IOMMU_NONE) && !(flag & ret.perm)) {

You could drop the parentheses around "flag != IOMMU_NONE".

For the rest of the patch: Sorry, can't review due to missing PCI spec :-(

 Thomas

Re: [Qemu-devel] [PATCH 1/3] s390x/pci: fixup the code walking IOMMU tables

[Yi Min Zhao]

在 2018/1/31 下午3:42, Thomas Huth 写道:
> On 30.01.2018 10:47, Yi Min Zhao wrote:
>> Current s390x PCI IOMMU code is lack of flags' checking, including:
>> 1) protection bit
>> 2) table length
>> 3) table offset
>> 4) intermediate tables' invalid bit
>> 5) format control bit
>>
>> This patch introduces a new struct named S390IOTLBEntry, and makes up
>> these missed checkings. At the same time, inform the guest with the
>> corresponding error number when the check fails.
>>
>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>> ---
>>   hw/s390x/s390-pci-bus.c  | 223 ++++++++++++++++++++++++++++++++++++++---------
>>   hw/s390x/s390-pci-bus.h  |  10 +++
>>   hw/s390x/s390-pci-inst.c |  10 ---
>>   3 files changed, 190 insertions(+), 53 deletions(-)
> [...]
>> @@ -374,26 +511,26 @@ static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
>>       DPRINTF("iommu trans addr 0x%" PRIx64 "\n", addr);
>>   
>>       if (addr < iommu->pba || addr > iommu->pal) {
>> -        return ret;
>> +        error = ERR_EVENT_OORANGE;
>> +        goto err;
>>       }
>>   
>> -    pte = s390_guest_io_table_walk(s390_pci_get_table_origin(iommu->g_iota),
>> -                                   addr);
>> -    if (!pte) {
>> -        return ret;
>> -    }
>> +    error = s390_guest_io_table_walk(iommu->g_iota, addr, &entry);
>>   
>> -    flags = pte & ZPCI_PTE_FLAG_MASK;
>> -    ret.iova = addr;
>> -    ret.translated_addr = pte & ZPCI_PTE_ADDR_MASK;
>> -    ret.addr_mask = 0xfff;
>> +    ret.iova = entry.iova;
>> +    ret.translated_addr = entry.translated_addr;
>> +    ret.addr_mask = entry.len - 1;
>> +    ret.perm = entry.perm;
>>   
>> -    if (flags & ZPCI_PTE_INVALID) {
>> -        ret.perm = IOMMU_NONE;
>> -    } else {
>> -        ret.perm = IOMMU_RW;
>> +    if ((flag != IOMMU_NONE) && !(flag & ret.perm)) {
> You could drop the parentheses around "flag != IOMMU_NONE".
OK. Will update.
>
> For the rest of the patch: Sorry, can't review due to missing PCI spec :-(
Thanks for your review anyway!
>
>   Thomas
>
>

Re: [Qemu-devel] [PATCH 1/3] s390x/pci: fixup the code walking IOMMU tables

[Cornelia Huck]

On Tue, 30 Jan 2018 10:47:13 +0100
Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:

> Current s390x PCI IOMMU code is lack of flags' checking, including:
> 1) protection bit
> 2) table length
> 3) table offset
> 4) intermediate tables' invalid bit
> 5) format control bit
> 
> This patch introduces a new struct named S390IOTLBEntry, and makes up
> these missed checkings. At the same time, inform the guest with the
> corresponding error number when the check fails.

There are a lot of things in this patch I cannot review due to -ENODOC,
but some comments below.

> 
> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-pci-bus.c  | 223 ++++++++++++++++++++++++++++++++++++++---------
>  hw/s390x/s390-pci-bus.h  |  10 +++
>  hw/s390x/s390-pci-inst.c |  10 ---
>  3 files changed, 190 insertions(+), 53 deletions(-)

(...)

> +/* ett is expected table type, -1 page table, 0 segment table, 1 region table */
> +static uint64_t get_table_index(uint64_t iova, int8_t ett)
> +{
> +    switch (ett) {
> +    case -1:
> +        return calc_px(iova);
> +    case 0:
> +        return calc_sx(iova);
> +    case 1:
> +        return calc_rtx(iova);
> +    }
> +
> +    return -1;

You use ett to differentiate between the three table types a lot. Is
this an architectured value, or an internal construct?

If you introduced it yourself, it might make sense to switch to an enum
instead. Otherwise, using some #defines would improve readability of
the code.

> +}

(...)

> +/**
> + * table_translate: do translation within one table and return the following
> + *                  table origin
> + *
> + * @entry: the entry being traslated, the result is stored in this.

s/traslated/translated/

> + * @to: the address of table origin.
> + * @ett: expected table type, 1 region table, 0 segment table and -1 page table.
> + * @error: error code
> + */
> +static uint64_t table_translate(S390IOTLBEntry *entry, uint64_t to, int8_t ett,
> +                                uint16_t *error)

(...)

> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index be449210d9..63fa06fb97 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -644,16 +644,6 @@ int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
>  
>      while (start < end) {
>          entry = imrc->translate(iommu_mr, start, IOMMU_NONE);
> -
> -        if (!entry.translated_addr) {
> -            pbdev->state = ZPCI_FS_ERROR;
> -            setcc(cpu, ZPCI_PCI_LS_ERR);
> -            s390_set_status_code(env, r1, ZPCI_PCI_ST_INSUF_RES);
> -            s390_pci_generate_error_event(ERR_EVENT_SERR, pbdev->fh, pbdev->fid,
> -                                          start, ERR_EVENT_Q_BIT);
> -            goto out;
> -        }
> -
>          memory_region_notify_iommu(iommu_mr, entry);
>          start += entry.addr_mask + 1;

You're now progressing even though you might have generated an error
event. Is that what's intended?

>      }

Re: [Qemu-devel] [PATCH 2/3] s390x/pci: fixup global refresh

[Cornelia Huck]

On Tue, 30 Jan 2018 10:47:14 +0100
Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:

> The VFIO common code doesn't provide the possibility to modify a
> previous mapping entry in another way than unmapping and mapping again
> with new properties.

I'm wondering why other architectures don't need that. Is this
refreshing an unique ability on s390 (due to that instruction)?

> 
> To avoid -EEXIST DMA mapping error, this we introduce a GHashTable to

s/this//

> store S390IOTLBEntry instances in order to cache the mapped entries.
> When intercepting rpcit instruction, ignore the identical mapped
> entries to avoid doing map operations multiple times and do unmap and
> re-map operations for the case of updating the valid entries. To
> achieve that goal, we also export the DMA walking function and
> optimize the code handling errors in rpcit handler.

How often does such a thing happen in practice?

> 
> Acked-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-pci-bus.c  | 28 +++++++++-----
>  hw/s390x/s390-pci-bus.h  |  3 ++
>  hw/s390x/s390-pci-inst.c | 95 ++++++++++++++++++++++++++++++++++--------------
>  3 files changed, 90 insertions(+), 36 deletions(-)

Can't really review the rest due to -ENODOC, sorry.

Re: [Qemu-devel] [PATCH 3/3] s390x/pci: use the right pal and pba in reg_ioat()

[Cornelia Huck]

On Tue, 30 Jan 2018 10:47:15 +0100
Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:

> When registering ioat, pba should be comprised of leftmost 52 bits and
> rightmost 12 binary zeros, and pal should be comprised of leftmost 52
> bits and right most 12 binary ones. Let's fixup this.
> 
> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-pci-inst.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 997a9cc2e9..3fcc330fe3 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -865,6 +865,8 @@ static int reg_ioat(CPUS390XState *env, S390PCIIOMMU *iommu, ZpciFib fib,
>      uint8_t dt = (g_iota >> 2) & 0x7;
>      uint8_t t = (g_iota >> 11) & 0x1;
>  
> +    pba &= ~0xfff;
> +    pal |= 0xfff;
>      if (pba > pal || pba < ZPCI_SDMA_ADDR || pal > ZPCI_EDMA_ADDR) {
>          s390_program_interrupt(env, PGM_OPERAND, 6, ra);
>          return -EINVAL;

It seems like pba and pal are part of the fib, which in turn seems to
be provided by the caller. Is that correct? If yes, is it valid for
them to not have the rightmost 12 bits as 0s resp. 1s?

(Probably answered in the architecture, I know. Might make sense to be
a tad more explicit in the description.)

Re: [Qemu-devel] [PATCH 1/3] s390x/pci: fixup the code walking IOMMU tables

[Yi Min Zhao]

在 2018/1/31 下午6:58, Cornelia Huck 写道:
> On Tue, 30 Jan 2018 10:47:13 +0100
> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
>
>> Current s390x PCI IOMMU code is lack of flags' checking, including:
>> 1) protection bit
>> 2) table length
>> 3) table offset
>> 4) intermediate tables' invalid bit
>> 5) format control bit
>>
>> This patch introduces a new struct named S390IOTLBEntry, and makes up
>> these missed checkings. At the same time, inform the guest with the
>> corresponding error number when the check fails.
> There are a lot of things in this patch I cannot review due to -ENODOC,
> but some comments below.
>
>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>> ---
>>   hw/s390x/s390-pci-bus.c  | 223 ++++++++++++++++++++++++++++++++++++++---------
>>   hw/s390x/s390-pci-bus.h  |  10 +++
>>   hw/s390x/s390-pci-inst.c |  10 ---
>>   3 files changed, 190 insertions(+), 53 deletions(-)
> (...)
>
>> +/* ett is expected table type, -1 page table, 0 segment table, 1 region table */
>> +static uint64_t get_table_index(uint64_t iova, int8_t ett)
>> +{
>> +    switch (ett) {
>> +    case -1:
>> +        return calc_px(iova);
>> +    case 0:
>> +        return calc_sx(iova);
>> +    case 1:
>> +        return calc_rtx(iova);
>> +    }
>> +
>> +    return -1;
> You use ett to differentiate between the three table types a lot. Is
> this an architectured value, or an internal construct?
It's an architectured value to some degree, because it's used to descript
the translation more clearly in the doc.
>
> If you introduced it yourself, it might make sense to switch to an enum
> instead. Otherwise, using some #defines would improve readability of
> the code.
OK. I will add macros in next version.
>
>> +}
> (...)
>
>> +/**
>> + * table_translate: do translation within one table and return the following
>> + *                  table origin
>> + *
>> + * @entry: the entry being traslated, the result is stored in this.
> s/traslated/translated/
OK.
>
>> + * @to: the address of table origin.
>> + * @ett: expected table type, 1 region table, 0 segment table and -1 page table.
>> + * @error: error code
>> + */
>> +static uint64_t table_translate(S390IOTLBEntry *entry, uint64_t to, int8_t ett,
>> +                                uint16_t *error)
> (...)
>
>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>> index be449210d9..63fa06fb97 100644
>> --- a/hw/s390x/s390-pci-inst.c
>> +++ b/hw/s390x/s390-pci-inst.c
>> @@ -644,16 +644,6 @@ int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
>>   
>>       while (start < end) {
>>           entry = imrc->translate(iommu_mr, start, IOMMU_NONE);
>> -
>> -        if (!entry.translated_addr) {
>> -            pbdev->state = ZPCI_FS_ERROR;
>> -            setcc(cpu, ZPCI_PCI_LS_ERR);
>> -            s390_set_status_code(env, r1, ZPCI_PCI_ST_INSUF_RES);
>> -            s390_pci_generate_error_event(ERR_EVENT_SERR, pbdev->fh, pbdev->fid,
>> -                                          start, ERR_EVENT_Q_BIT);
>> -            goto out;
>> -        }
>> -
>>           memory_region_notify_iommu(iommu_mr, entry);
>>           start += entry.addr_mask + 1;
> You're now progressing even though you might have generated an error
> event. Is that what's intended?
Yes, this is wrong. The right thing is only delete the code generating 
error event,
and keep the if check here in this patch.
>
>>       }
>

Re: [Qemu-devel] [PATCH 3/3] s390x/pci: use the right pal and pba in reg_ioat()

[Pierre Morel]

On 31/01/2018 12:44, Cornelia Huck wrote:
> On Tue, 30 Jan 2018 10:47:15 +0100
> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
>
>> When registering ioat, pba should be comprised of leftmost 52 bits and
>> rightmost 12 binary zeros, and pal should be comprised of leftmost 52
>> bits and right most 12 binary ones. Let's fixup this.
>>
>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>> ---
>>   hw/s390x/s390-pci-inst.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>> index 997a9cc2e9..3fcc330fe3 100644
>> --- a/hw/s390x/s390-pci-inst.c
>> +++ b/hw/s390x/s390-pci-inst.c
>> @@ -865,6 +865,8 @@ static int reg_ioat(CPUS390XState *env, S390PCIIOMMU *iommu, ZpciFib fib,
>>       uint8_t dt = (g_iota >> 2) & 0x7;
>>       uint8_t t = (g_iota >> 11) & 0x1;
>>   
>> +    pba &= ~0xfff;
>> +    pal |= 0xfff;
>>       if (pba > pal || pba < ZPCI_SDMA_ADDR || pal > ZPCI_EDMA_ADDR) {
>>           s390_program_interrupt(env, PGM_OPERAND, 6, ra);
>>           return -EINVAL;
> It seems like pba and pal are part of the fib, which in turn seems to
> be provided by the caller. Is that correct? If yes, is it valid for
> them to not have the rightmost 12 bits as 0s resp. 1s?
>
> (Probably answered in the architecture, I know. Might make sense to be
> a tad more explicit in the description.)
>
Yes it is, only word6 and the bits 0-19 of word 7 are used for PAL and
the zPCI facility treats the right most 12 bits of the PAL as containing 
ones.

For PBA words 4 and 0-19 bits of word 5 with 12 0 append on the right 
provides the PBA.

The lower 12 bits of words 5 and 7 of the FIB are ignored by the facility.

@Yi Min: may be add the last sentence to the commit message.

@Conny: Is it clearer?

Regards,

Pierre


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH 1/3] s390x/pci: fixup the code walking IOMMU tables

[Pierre Morel]

On 01/02/2018 12:28, Yi Min Zhao wrote:
>
>
> 在 2018/1/31 下午6:58, Cornelia Huck 写道:
>> On Tue, 30 Jan 2018 10:47:13 +0100
>> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
>>
>>> Current s390x PCI IOMMU code is lack of flags' checking, including:
>>> 1) protection bit
>>> 2) table length
>>> 3) table offset
>>> 4) intermediate tables' invalid bit
>>> 5) format control bit
>>>
>>> This patch introduces a new struct named S390IOTLBEntry, and makes up
>>> these missed checkings. At the same time, inform the guest with the
>>> corresponding error number when the check fails.
>> There are a lot of things in this patch I cannot review due to -ENODOC,
>> but some comments below.
>>
>>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>> ---
>>>   hw/s390x/s390-pci-bus.c  | 223 
>>> ++++++++++++++++++++++++++++++++++++++---------
>>>   hw/s390x/s390-pci-bus.h  |  10 +++
>>>   hw/s390x/s390-pci-inst.c |  10 ---
>>>   3 files changed, 190 insertions(+), 53 deletions(-)
>> (...)
>>
>>> +/* ett is expected table type, -1 page table, 0 segment table, 1 
>>> region table */
>>> +static uint64_t get_table_index(uint64_t iova, int8_t ett)
>>> +{
>>> +    switch (ett) {
>>> +    case -1:
>>> +        return calc_px(iova);
>>> +    case 0:
>>> +        return calc_sx(iova);
>>> +    case 1:
>>> +        return calc_rtx(iova);
>>> +    }
>>> +
>>> +    return -1;
>> You use ett to differentiate between the three table types a lot. Is
>> this an architectured value, or an internal construct?
> It's an architectured value to some degree, because it's used to descript
> the translation more clearly in the doc.
>>
>> If you introduced it yourself, it might make sense to switch to an enum
>> instead. Otherwise, using some #defines would improve readability of
>> the code.
> OK. I will add macros in next version.
>>
>>> +}
>> (...)
>>
>>> +/**
>>> + * table_translate: do translation within one table and return the 
>>> following
>>> + *                  table origin
>>> + *
>>> + * @entry: the entry being traslated, the result is stored in this.
>> s/traslated/translated/
> OK.
>>
>>> + * @to: the address of table origin.
>>> + * @ett: expected table type, 1 region table, 0 segment table and 
>>> -1 page table.
>>> + * @error: error code
>>> + */
>>> +static uint64_t table_translate(S390IOTLBEntry *entry, uint64_t to, 
>>> int8_t ett,
>>> +                                uint16_t *error)
>> (...)
>>
>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>>> index be449210d9..63fa06fb97 100644
>>> --- a/hw/s390x/s390-pci-inst.c
>>> +++ b/hw/s390x/s390-pci-inst.c
>>> @@ -644,16 +644,6 @@ int rpcit_service_call(S390CPU *cpu, uint8_t 
>>> r1, uint8_t r2, uintptr_t ra)
>>>         while (start < end) {
>>>           entry = imrc->translate(iommu_mr, start, IOMMU_NONE);
>>> -
>>> -        if (!entry.translated_addr) {
>>> -            pbdev->state = ZPCI_FS_ERROR;
>>> -            setcc(cpu, ZPCI_PCI_LS_ERR);
>>> -            s390_set_status_code(env, r1, ZPCI_PCI_ST_INSUF_RES);
>>> -            s390_pci_generate_error_event(ERR_EVENT_SERR, 
>>> pbdev->fh, pbdev->fid,
>>> -                                          start, ERR_EVENT_Q_BIT);
>>> -            goto out;
>>> -        }
>>> -
>>>           memory_region_notify_iommu(iommu_mr, entry);
>>>           start += entry.addr_mask + 1;
>> You're now progressing even though you might have generated an error
>> event. Is that what's intended?
> Yes, this is wrong. The right thing is only delete the code generating 
> error event,
> and keep the if check here in this patch.

Hum, I do not see any problem with having a translated address being 0.

I would say it is one of the fixup ;) .

Regards,
Pierre

>>
>>>       }
>>
>

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH 3/3] s390x/pci: use the right pal and pba in reg_ioat()

[Cornelia Huck]

On Thu, 1 Feb 2018 12:33:01 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> On 31/01/2018 12:44, Cornelia Huck wrote:
> > On Tue, 30 Jan 2018 10:47:15 +0100
> > Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
> >  
> >> When registering ioat, pba should be comprised of leftmost 52 bits and
> >> rightmost 12 binary zeros, and pal should be comprised of leftmost 52
> >> bits and right most 12 binary ones. Let's fixup this.
> >>
> >> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> >> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >> ---
> >>   hw/s390x/s390-pci-inst.c | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> >> index 997a9cc2e9..3fcc330fe3 100644
> >> --- a/hw/s390x/s390-pci-inst.c
> >> +++ b/hw/s390x/s390-pci-inst.c
> >> @@ -865,6 +865,8 @@ static int reg_ioat(CPUS390XState *env, S390PCIIOMMU *iommu, ZpciFib fib,
> >>       uint8_t dt = (g_iota >> 2) & 0x7;
> >>       uint8_t t = (g_iota >> 11) & 0x1;
> >>   
> >> +    pba &= ~0xfff;
> >> +    pal |= 0xfff;
> >>       if (pba > pal || pba < ZPCI_SDMA_ADDR || pal > ZPCI_EDMA_ADDR) {
> >>           s390_program_interrupt(env, PGM_OPERAND, 6, ra);
> >>           return -EINVAL;  
> > It seems like pba and pal are part of the fib, which in turn seems to
> > be provided by the caller. Is that correct? If yes, is it valid for
> > them to not have the rightmost 12 bits as 0s resp. 1s?
> >
> > (Probably answered in the architecture, I know. Might make sense to be
> > a tad more explicit in the description.)
> >  
> Yes it is, only word6 and the bits 0-19 of word 7 are used for PAL and
> the zPCI facility treats the right most 12 bits of the PAL as containing 
> ones.
> 
> For PBA words 4 and 0-19 bits of word 5 with 12 0 append on the right 
> provides the PBA.
> 
> The lower 12 bits of words 5 and 7 of the FIB are ignored by the facility.
> 
> @Yi Min: may be add the last sentence to the commit message.
> 
> @Conny: Is it clearer?

Yes, adding the last sentence makes it clearer. Thanks!

Re: [Qemu-devel] [PATCH 1/3] s390x/pci: fixup the code walking IOMMU tables

[Cornelia Huck]

On Thu, 1 Feb 2018 12:56:01 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> On 01/02/2018 12:28, Yi Min Zhao wrote:
> >
> >
> > 在 2018/1/31 下午6:58, Cornelia Huck 写道:  
> >> On Tue, 30 Jan 2018 10:47:13 +0100
> >> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:

> >>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> >>> index be449210d9..63fa06fb97 100644
> >>> --- a/hw/s390x/s390-pci-inst.c
> >>> +++ b/hw/s390x/s390-pci-inst.c
> >>> @@ -644,16 +644,6 @@ int rpcit_service_call(S390CPU *cpu, uint8_t 
> >>> r1, uint8_t r2, uintptr_t ra)
> >>>         while (start < end) {
> >>>           entry = imrc->translate(iommu_mr, start, IOMMU_NONE);
> >>> -
> >>> -        if (!entry.translated_addr) {
> >>> -            pbdev->state = ZPCI_FS_ERROR;
> >>> -            setcc(cpu, ZPCI_PCI_LS_ERR);
> >>> -            s390_set_status_code(env, r1, ZPCI_PCI_ST_INSUF_RES);
> >>> -            s390_pci_generate_error_event(ERR_EVENT_SERR, 
> >>> pbdev->fh, pbdev->fid,
> >>> -                                          start, ERR_EVENT_Q_BIT);
> >>> -            goto out;
> >>> -        }
> >>> -
> >>>           memory_region_notify_iommu(iommu_mr, entry);
> >>>           start += entry.addr_mask + 1;  
> >> You're now progressing even though you might have generated an error
> >> event. Is that what's intended?  
> > Yes, this is wrong. The right thing is only delete the code generating 
> > error event,
> > and keep the if check here in this patch.  
> 
> Hum, I do not see any problem with having a translated address being 0.
> 
> I would say it is one of the fixup ;) .

Isn't this in response to an instruction that is supposed to
register/... something? IOW, the caller of the instruction tried to
make something happen, but something did not work out as intended (and
doesn't the code stumble over this later? Or do we get an error then?)
Shouldn't the caller get an indication of that? Of course, this again
depends on what the architecture says :)

Re: [Qemu-devel] [PATCH 2/3] s390x/pci: fixup global refresh

[Pierre Morel]

On 31/01/2018 12:35, Cornelia Huck wrote:
> On Tue, 30 Jan 2018 10:47:14 +0100
> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
>
>> The VFIO common code doesn't provide the possibility to modify a
>> previous mapping entry in another way than unmapping and mapping again
>> with new properties.
> I'm wondering why other architectures don't need that. Is this
> refreshing an unique ability on s390 (due to that instruction)?

It is not specific to S390 but to the iommu_type1.

May be we should have different VFIO_IOMMU for mediated devices and
architectures with SR_IOV capabilities to suppress this restriction.

Other architectures using vfio_iommu_spapr_tce do not seem to have
the problem (AFAIU the code).

>
>> To avoid -EEXIST DMA mapping error, this we introduce a GHashTable to
> s/this//
>
>> store S390IOTLBEntry instances in order to cache the mapped entries.
>> When intercepting rpcit instruction, ignore the identical mapped
>> entries to avoid doing map operations multiple times and do unmap and
>> re-map operations for the case of updating the valid entries. To
>> achieve that goal, we also export the DMA walking function and
>> optimize the code handling errors in rpcit handler.
> How often does such a thing happen in practice?

It depends on the size of the guest memory and of the guest OS.
Linux allows an IOMMU IOVA size up to the minimal of high_memory, iommu 
aperture
and maximal size of RT IOMMU entry.
Linux guest use lazy TLB flush and waits for the allowed IOMMU size is 
used to
flush the TLB.
When the flush is done it is a global flush not a global invalidation, 
it follows
that some entries in the IOTLB are still valid and must be kept while other
are to be invalidated.

Since we have no possibility to know which the entries of the IOMMU TLB
have been modified, we must check all entries.

When does the -EEXIST error happen?
- Linux before lazy flush : (c60d1ae4e 2014... we did not have zPCI at 
that time)
     never
- Linux with lazy flush and new IOMMU_TYPE1:
     always as soon as the global flush occurs
     which depends of the number of allowed IOMMU IOVA size and mapped 
entries in IOTLB

When does the implemented cache be used and avoid errors?
- an entry in the cache exist for all mapped IOTLB entries
- a miss -> add the entry and issue a DMA_MAP to the host
- a hit for IOMMU_NONE -> issue a DMA_UNMAP and remove the entry
- a hit for valid unmodified entry -> do nothing (already mapped)
- hit for a valid but nmodified entry -> issue UNMAP/MAP update the entry

Regards,

Pierre


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH 1/3] s390x/pci: fixup the code walking IOMMU tables

[Pierre Morel]

On 01/02/2018 13:15, Cornelia Huck wrote:
> On Thu, 1 Feb 2018 12:56:01 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>
>> On 01/02/2018 12:28, Yi Min Zhao wrote:
>>>
>>> 在 2018/1/31 下午6:58, Cornelia Huck 写道:
>>>> On Tue, 30 Jan 2018 10:47:13 +0100
>>>> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
>>>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>>>>> index be449210d9..63fa06fb97 100644
>>>>> --- a/hw/s390x/s390-pci-inst.c
>>>>> +++ b/hw/s390x/s390-pci-inst.c
>>>>> @@ -644,16 +644,6 @@ int rpcit_service_call(S390CPU *cpu, uint8_t
>>>>> r1, uint8_t r2, uintptr_t ra)
>>>>>          while (start < end) {
>>>>>            entry = imrc->translate(iommu_mr, start, IOMMU_NONE);
>>>>> -
>>>>> -        if (!entry.translated_addr) {
>>>>> -            pbdev->state = ZPCI_FS_ERROR;
>>>>> -            setcc(cpu, ZPCI_PCI_LS_ERR);
>>>>> -            s390_set_status_code(env, r1, ZPCI_PCI_ST_INSUF_RES);
>>>>> -            s390_pci_generate_error_event(ERR_EVENT_SERR,
>>>>> pbdev->fh, pbdev->fid,
>>>>> -                                          start, ERR_EVENT_Q_BIT);
>>>>> -            goto out;
>>>>> -        }
>>>>> -
>>>>>            memory_region_notify_iommu(iommu_mr, entry);
>>>>>            start += entry.addr_mask + 1;
>>>> You're now progressing even though you might have generated an error
>>>> event. Is that what's intended?
>>> Yes, this is wrong. The right thing is only delete the code generating
>>> error event,
>>> and keep the if check here in this patch.
>> Hum, I do not see any problem with having a translated address being 0.
>>
>> I would say it is one of the fixup ;) .
> Isn't this in response to an instruction that is supposed to
> register/... something? IOW, the caller of the instruction tried to
> make something happen, but something did not work out as intended (and
> doesn't the code stumble over this later? Or do we get an error then?)
> Shouldn't the caller get an indication of that? Of course, this again
> depends on what the architecture says :)
>
I don't think so, I think it was really an implementation error in the
previous code because we have no way to return an error from the
translate() function, we just return the TLB entry.

If we have an R or W or RW entry we return the entry in all other cases,
including a RW protected, invalid or on internal error entry we return 
IOMMU_NONE

IMHO it would have been clearer if this function have been void.

Regards,

Pierre

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH 3/3] s390x/pci: use the right pal and pba in reg_ioat()

[Yi Min Zhao]

在 2018/2/1 下午8:02, Cornelia Huck 写道:
> On Thu, 1 Feb 2018 12:33:01 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>
>> On 31/01/2018 12:44, Cornelia Huck wrote:
>>> On Tue, 30 Jan 2018 10:47:15 +0100
>>> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
>>>   
>>>> When registering ioat, pba should be comprised of leftmost 52 bits and
>>>> rightmost 12 binary zeros, and pal should be comprised of leftmost 52
>>>> bits and right most 12 binary ones. Let's fixup this.
>>>>
>>>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>> ---
>>>>    hw/s390x/s390-pci-inst.c | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>>>> index 997a9cc2e9..3fcc330fe3 100644
>>>> --- a/hw/s390x/s390-pci-inst.c
>>>> +++ b/hw/s390x/s390-pci-inst.c
>>>> @@ -865,6 +865,8 @@ static int reg_ioat(CPUS390XState *env, S390PCIIOMMU *iommu, ZpciFib fib,
>>>>        uint8_t dt = (g_iota >> 2) & 0x7;
>>>>        uint8_t t = (g_iota >> 11) & 0x1;
>>>>    
>>>> +    pba &= ~0xfff;
>>>> +    pal |= 0xfff;
>>>>        if (pba > pal || pba < ZPCI_SDMA_ADDR || pal > ZPCI_EDMA_ADDR) {
>>>>            s390_program_interrupt(env, PGM_OPERAND, 6, ra);
>>>>            return -EINVAL;
>>> It seems like pba and pal are part of the fib, which in turn seems to
>>> be provided by the caller. Is that correct? If yes, is it valid for
>>> them to not have the rightmost 12 bits as 0s resp. 1s?
>>>
>>> (Probably answered in the architecture, I know. Might make sense to be
>>> a tad more explicit in the description.)
>>>   
>> Yes it is, only word6 and the bits 0-19 of word 7 are used for PAL and
>> the zPCI facility treats the right most 12 bits of the PAL as containing
>> ones.
>>
>> For PBA words 4 and 0-19 bits of word 5 with 12 0 append on the right
>> provides the PBA.
>>
>> The lower 12 bits of words 5 and 7 of the FIB are ignored by the facility.
>>
>> @Yi Min: may be add the last sentence to the commit message.
>>
>> @Conny: Is it clearer?
> Yes, adding the last sentence makes it clearer. Thanks!
>
>
OK. Thanks!