[Qemu-devel] [PATCH 0/7] s390x/pci: Improve zPCI to cover more cases

[Qemu-devel] [PATCH 0/7] s390x/pci: Improve zPCI to cover more cases

[Pierre Morel]

Right now the PCI support is very limited (e.g. pass through of a
host vfio device)
To enable features like virtio-pci several modifications needs to be
done.

Virtio-PCI uses subregions, which may eventually be discontinuous
inside bars instead of a single flat region.
The address offset being formerly calculated from the BAR base address
must be adapted to the subregions instead of to the single region.

This patch provides the new calculation for the three kind of BAR
access, zPCI STORE, zPCI LOAD and zPCI STORE BLOCK.

We use the opportunity to
 - enhance the fault detection for zPCI STORE and LOAD,
 - enhance the fault detection and to provide the maximum STORE BLOCK
   block size, maxstbl, for zPCI STORE BLOCK
 - factor out part of the code used to calculate the offset and
   access the BARs,
 - factor out the code for endianess conversion.


Pierre Morel (7):
  s390x/pci: factor out endianess conversion
  s390x/pci: rework PCI STORE
  s390x/pci: rework PCI LOAD
  s390x/pci: rework PCI STORE BLOCK
  s390x/pci: move the memory region read from pcilg
  s390x/pci: move the memory region write from pcistg
  s390x/pci: search for subregion inside the BARs

 hw/s390x/s390-pci-bus.h  |   1 +
 hw/s390x/s390-pci-inst.c | 250 ++++++++++++++++++++++++++++-------------------
 hw/s390x/s390-pci-inst.h |   2 +-
 3 files changed, 153 insertions(+), 100 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/7] s390x/pci: factor out endianess conversion

[Pierre Morel]

There are two places where the same endianness conversion
is done.
Let's factor this out into a static function.

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

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 8e088f3..8fcb02d 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -314,6 +314,35 @@ out:
     return 0;
 }
 
+/**
+ * This function swaps the data at ptr according from one
+ * endianness to the other.
+ * valid data in the uint64_t data field.
+ * @ptr: a pointer to a uint64_t data field
+ * @len: the length of the valid data, must be 1,2,4 or 8
+ */
+static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
+{
+    uint64_t data = *ptr;
+    switch (len) {
+    case 1:
+        break;
+    case 2:
+        data = bswap16(data);
+        break;
+    case 4:
+        data = bswap32(data);
+        break;
+    case 8:
+        data = bswap64(data);
+        break;
+    default:
+        return -EINVAL;
+    }
+    *ptr = data;
+    return 0;
+}
+
 int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
 {
     CPUS390XState *env = &cpu->env;
@@ -385,19 +414,7 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
         data =  pci_host_config_read_common(
                    pbdev->pdev, offset, pci_config_size(pbdev->pdev), len);
 
-        switch (len) {
-        case 1:
-            break;
-        case 2:
-            data = bswap16(data);
-            break;
-        case 4:
-            data = bswap32(data);
-            break;
-        case 8:
-            data = bswap64(data);
-            break;
-        default:
+        if (zpci_endian_swap(&data, len)) {
             program_interrupt(env, PGM_OPERAND, 4);
             return 0;
         }
@@ -500,19 +517,8 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
             program_interrupt(env, PGM_OPERAND, 4);
             return 0;
         }
-        switch (len) {
-        case 1:
-            break;
-        case 2:
-            data = bswap16(data);
-            break;
-        case 4:
-            data = bswap32(data);
-            break;
-        case 8:
-            data = bswap64(data);
-            break;
-        default:
+
+        if (zpci_endian_swap(&data, len)) {
             program_interrupt(env, PGM_OPERAND, 4);
             return 0;
         }
-- 
2.7.4

[Qemu-devel] [PATCH 2/7] s390x/pci: rework PCI STORE

[Pierre Morel]

Enhance the fault detection, correction of the fault reporting.

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

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 8fcb02d..4a2f996 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -469,6 +469,12 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
     pcias = (env->regs[r2] >> 16) & 0xf;
     len = env->regs[r2] & 0xf;
     offset = env->regs[r2 + 1];
+    data = env->regs[r1];
+
+    if (!(fh & FH_MASK_ENABLE)) {
+        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
+        return 0;
+    }
 
     pbdev = s390_pci_find_dev_by_fh(s390_get_phb(), fh);
     if (!pbdev) {
@@ -478,12 +484,7 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
     }
 
     switch (pbdev->state) {
-    case ZPCI_FS_RESERVED:
-    case ZPCI_FS_STANDBY:
-    case ZPCI_FS_DISABLED:
     case ZPCI_FS_PERMANENT_ERROR:
-        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
-        return 0;
     case ZPCI_FS_ERROR:
         setcc(cpu, ZPCI_PCI_LS_ERR);
         s390_set_status_code(env, r2, ZPCI_PCI_ST_BLOCKED);
@@ -492,9 +493,13 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
         break;
     }
 
-    data = env->regs[r1];
-    if (pcias < 6) {
-        if ((8 - (offset & 0x7)) < len) {
+    /* Test that the pcias is a valid pcias and do the according operations */
+    switch (pcias) {
+    case 0 ... 5:
+        /* Check length:
+         * A length of 0 is invalid and length should not cross a double word
+         */
+        if (!len || (len > (8 - (offset & 0x7)))) {
             program_interrupt(env, PGM_OPERAND, 4);
             return 0;
         }
@@ -512,21 +517,23 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
             program_interrupt(env, PGM_OPERAND, 4);
             return 0;
         }
-    } else if (pcias == 15) {
-        if ((4 - (offset & 0x3)) < len) {
-            program_interrupt(env, PGM_OPERAND, 4);
-            return 0;
-        }
-
-        if (zpci_endian_swap(&data, len)) {
+        break;
+    case 15:
+        /* ZPCI uses the pseudo BAR number 15 as configuration space */
+        /* possible access length are 1,2,4 and must not cross a word */
+        if (!len || (len > (4 - (offset & 0x3))) || len == 3) {
             program_interrupt(env, PGM_OPERAND, 4);
             return 0;
         }
-
+        /* len = 1,2,4 so we do not need to test */
+        zpci_endian_swap(&data, len);
         pci_host_config_write_common(pbdev->pdev, offset,
                                      pci_config_size(pbdev->pdev),
                                      data, len);
-    } else {
+        break;
+    case PCI_ROM_SLOT:
+        /* Fall through */
+    default:
         DPRINTF("pcistg invalid space\n");
         setcc(cpu, ZPCI_PCI_LS_ERR);
         s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);
-- 
2.7.4

[Qemu-devel] [PATCH 3/7] s390x/pci: rework PCI LOAD

[Pierre Morel]

Enhance the fault detection, correction of the fault reporting.

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

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 4a2f996..62ebaa0 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -372,6 +372,11 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
     len = env->regs[r2] & 0xf;
     offset = env->regs[r2 + 1];
 
+    if (!(fh & FH_MASK_ENABLE)) {
+        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
+        return 0;
+    }
+
     pbdev = s390_pci_find_dev_by_fh(s390_get_phb(), fh);
     if (!pbdev) {
         DPRINTF("pcilg no pci dev\n");
@@ -380,12 +385,7 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
     }
 
     switch (pbdev->state) {
-    case ZPCI_FS_RESERVED:
-    case ZPCI_FS_STANDBY:
-    case ZPCI_FS_DISABLED:
     case ZPCI_FS_PERMANENT_ERROR:
-        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
-        return 0;
     case ZPCI_FS_ERROR:
         setcc(cpu, ZPCI_PCI_LS_ERR);
         s390_set_status_code(env, r2, ZPCI_PCI_ST_BLOCKED);
@@ -394,8 +394,9 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
         break;
     }
 
-    if (pcias < 6) {
-        if ((8 - (offset & 0x7)) < len) {
+    switch (pcias) {
+    case 0 ... 5:
+        if (!len || (len > (8 - (offset & 0x7)))) {
             program_interrupt(env, PGM_OPERAND, 4);
             return 0;
         }
@@ -406,8 +407,9 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
             program_interrupt(env, PGM_OPERAND, 4);
             return 0;
         }
-    } else if (pcias == 15) {
-        if ((4 - (offset & 0x3)) < len) {
+        break;
+    case 15:
+        if (!len || (len > (4 - (offset & 0x3))) || len == 3) {
             program_interrupt(env, PGM_OPERAND, 4);
             return 0;
         }
@@ -418,8 +420,11 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
             program_interrupt(env, PGM_OPERAND, 4);
             return 0;
         }
-    } else {
-        DPRINTF("invalid space\n");
+        break;
+    case PCI_ROM_SLOT:
+        /* Fall through */
+    default:
+        DPRINTF("pcilg invalid space\n");
         setcc(cpu, ZPCI_PCI_LS_ERR);
         s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);
         return 0;
-- 
2.7.4

[Qemu-devel] [PATCH 4/7] s390x/pci: rework PCI STORE BLOCK

[Pierre Morel]

Enhance the fault detection.

Add the maxstbl entry to both the Query PCI Function Group
response and the PCIBusDevice structure.

Initialize the maxstbl to 128 per default until we get
the actual data from the hardware.

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

diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
index 560bd82..2993f0d 100644
--- a/hw/s390x/s390-pci-bus.h
+++ b/hw/s390x/s390-pci-bus.h
@@ -284,6 +284,7 @@ struct S390PCIBusDevice {
     uint64_t fmb_addr;
     uint8_t isc;
     uint16_t noi;
+    uint16_t maxstbl;
     uint8_t sum;
     S390MsixInfo msix;
     AdapterRoutes routes;
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 62ebaa0..646137e 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -294,6 +294,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2)
         stq_p(&resgrp->msia, ZPCI_MSI_ADDR);
         stw_p(&resgrp->mui, 0);
         stw_p(&resgrp->i, 128);
+        stw_p(&resgrp->maxstbl, 128);
         resgrp->version = 0;
 
         stw_p(&resgrp->hdr.rsp, CLP_RC_OK);
@@ -648,6 +649,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
     S390PCIBusDevice *pbdev;
     MemoryRegion *mr;
     MemTxResult result;
+    uint64_t offset;
     int i;
     uint32_t fh;
     uint8_t pcias;
@@ -662,22 +664,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
     fh = env->regs[r1] >> 32;
     pcias = (env->regs[r1] >> 16) & 0xf;
     len = env->regs[r1] & 0xff;
+    offset = env->regs[r3];
 
-    if (pcias > 5) {
-        DPRINTF("pcistb invalid space\n");
-        setcc(cpu, ZPCI_PCI_LS_ERR);
-        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
-        return 0;
-    }
-
-    switch (len) {
-    case 16:
-    case 32:
-    case 64:
-    case 128:
-        break;
-    default:
-        program_interrupt(env, PGM_SPECIFICATION, 6);
+    if (!(fh & FH_MASK_ENABLE)) {
+        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
         return 0;
     }
 
@@ -689,12 +679,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
     }
 
     switch (pbdev->state) {
-    case ZPCI_FS_RESERVED:
-    case ZPCI_FS_STANDBY:
-    case ZPCI_FS_DISABLED:
     case ZPCI_FS_PERMANENT_ERROR:
-        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
-        return 0;
     case ZPCI_FS_ERROR:
         setcc(cpu, ZPCI_PCI_LS_ERR);
         s390_set_status_code(env, r1, ZPCI_PCI_ST_BLOCKED);
@@ -703,8 +688,33 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
         break;
     }
 
+    if (pcias > 5) {
+        DPRINTF("pcistb invalid space\n");
+        setcc(cpu, ZPCI_PCI_LS_ERR);
+        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
+        return 0;
+    }
+
+    /* Verify the address, offset and length */
+    /* offset must be a multiple of 8 */
+    if (offset % 8) {
+        goto addressing_error;
+    }
+    /* Length must be greater than 8, a multiple of 8, lower than maxstbl */
+    if ((len <= 8) || (len % 8) || (len > pbdev->maxstbl)) {
+        goto addressing_error;
+    }
+    /* Do not cross a 4K-byte boundary */
+    if (((offset & 0xfff) + len) > 0x1000) {
+        goto addressing_error;
+    }
+    /* Guest address must be double word aligned */
+    if (gaddr & 0x07UL) {
+        goto addressing_error;
+    }
+
     mr = pbdev->pdev->io_regions[pcias].memory;
-    if (!memory_region_access_valid(mr, env->regs[r3], len, true)) {
+    if (!memory_region_access_valid(mr, offset, len, true)) {
         program_interrupt(env, PGM_OPERAND, 6);
         return 0;
     }
@@ -714,9 +724,9 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
     }
 
     for (i = 0; i < len / 8; i++) {
-        result = memory_region_dispatch_write(mr, env->regs[r3] + i * 8,
-                                     ldq_p(buffer + i * 8), 8,
-                                     MEMTXATTRS_UNSPECIFIED);
+        result = memory_region_dispatch_write(mr, offset + i * 8,
+                                              ldq_p(buffer + i * 8), 8,
+                                              MEMTXATTRS_UNSPECIFIED);
         if (result != MEMTX_OK) {
             program_interrupt(env, PGM_OPERAND, 6);
             return 0;
@@ -725,6 +735,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
 
     setcc(cpu, ZPCI_PCI_LS_OK);
     return 0;
+
+addressing_error:
+    program_interrupt(env, PGM_SPECIFICATION, 6);
+    return 0;
 }
 
 static int reg_irqs(CPUS390XState *env, S390PCIBusDevice *pbdev, ZpciFib fib)
diff --git a/hw/s390x/s390-pci-inst.h b/hw/s390x/s390-pci-inst.h
index 94a959f..d6aeadd 100644
--- a/hw/s390x/s390-pci-inst.h
+++ b/hw/s390x/s390-pci-inst.h
@@ -162,7 +162,7 @@ typedef struct ClpRspQueryPciGrp {
 #define CLP_RSP_QPCIG_MASK_FRAME   0x2
 #define CLP_RSP_QPCIG_MASK_REFRESH 0x1
     uint8_t fr;
-    uint16_t reserved2;
+    uint16_t maxstbl;
     uint16_t mui;
     uint64_t reserved3;
     uint64_t dasm; /* dma address space mask */
-- 
2.7.4

[Qemu-devel] [PATCH 5/7] s390x/pci: move the memory region read from pcilg

[Pierre Morel]

Let's move the memory region read from pcilg into a dedicated function.
This allows us to prepare a later patch.

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

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 646137e..50135a0 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -344,13 +344,22 @@ static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
     return 0;
 }
 
+static MemTxResult zpci_read_bar(S390PCIBusDevice *pbdev, uint8_t pcias,
+                                 uint64_t offset, uint64_t *data, uint8_t len)
+{
+    MemoryRegion *mr;
+
+    mr = pbdev->pdev->io_regions[pcias].memory;
+    return memory_region_dispatch_read(mr, offset, data, len,
+                                       MEMTXATTRS_UNSPECIFIED);
+}
+
 int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
 {
     CPUS390XState *env = &cpu->env;
     S390PCIBusDevice *pbdev;
     uint64_t offset;
     uint64_t data;
-    MemoryRegion *mr;
     MemTxResult result;
     uint8_t len;
     uint32_t fh;
@@ -401,9 +410,7 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
             program_interrupt(env, PGM_OPERAND, 4);
             return 0;
         }
-        mr = pbdev->pdev->io_regions[pcias].memory;
-        result = memory_region_dispatch_read(mr, offset, &data, len,
-                                             MEMTXATTRS_UNSPECIFIED);
+        result = zpci_read_bar(pbdev, pcias, offset, &data, len);
         if (result != MEMTX_OK) {
             program_interrupt(env, PGM_OPERAND, 4);
             return 0;
-- 
2.7.4

[Qemu-devel] [PATCH 6/7] s390x/pci: move the memory region write from pcistg

[Pierre Morel]

Let's move the memory region write from pcistg into a dedicated
function.
This allows us to prepare a later patch searching for subregions
inside of the memory region.

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

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 50135a0..97f62b5 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -455,12 +455,27 @@ static int trap_msix(S390PCIBusDevice *pbdev, uint64_t offset, uint8_t pcias)
     }
 }
 
+static MemTxResult zpci_write_bar(S390PCIBusDevice *pbdev, uint8_t pcias,
+                                  uint64_t offset, uint64_t data, uint8_t len)
+{
+    MemoryRegion *mr;
+
+    if (trap_msix(pbdev, offset, pcias)) {
+        offset = offset - pbdev->msix.table_offset;
+        mr = &pbdev->pdev->msix_table_mmio;
+    } else {
+        mr = pbdev->pdev->io_regions[pcias].memory;
+    }
+
+    return memory_region_dispatch_write(mr, offset, data, len,
+                                        MEMTXATTRS_UNSPECIFIED);
+}
+
 int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
 {
     CPUS390XState *env = &cpu->env;
     uint64_t offset, data;
     S390PCIBusDevice *pbdev;
-    MemoryRegion *mr;
     MemTxResult result;
     uint8_t len;
     uint32_t fh;
@@ -517,15 +532,7 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
             return 0;
         }
 
-        if (trap_msix(pbdev, offset, pcias)) {
-            offset = offset - pbdev->msix.table_offset;
-            mr = &pbdev->pdev->msix_table_mmio;
-        } else {
-            mr = pbdev->pdev->io_regions[pcias].memory;
-        }
-
-        result = memory_region_dispatch_write(mr, offset, data, len,
-                                     MEMTXATTRS_UNSPECIFIED);
+        result = zpci_write_bar(pbdev, pcias, offset, data, len);
         if (result != MEMTX_OK) {
             program_interrupt(env, PGM_OPERAND, 4);
             return 0;
-- 
2.7.4

[Qemu-devel] [PATCH 7/7] s390x/pci: search for subregion inside the BARs

[Pierre Morel]

When dispatching memory access to PCI BAR region, we must
look for eventual subregion, used by the PCI device to map
different memory areas inside the same PCI BAR.

Since the data offset we received is calculated starting at the
region start address we need to adjust the offset for the subregion.

The data offset inside the subregion is calculated by substracting
the subregion's starting address from the data offset in the region.

The function trap_msix() was used, despite not really useful since we
removed the index from the msix message. With this patch we can
definitively suppress this function.

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

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 97f62b5..b59ceef 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -344,12 +344,31 @@ static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
     return 0;
 }
 
+static MemoryRegion *s390_get_subregion(MemoryRegion *mr, uint64_t offset,
+                                        uint8_t len)
+{
+    MemoryRegion *other;
+    uint64_t subregion_size;
+
+    QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
+        subregion_size = int128_get64(other->size);
+        if ((offset >= other->addr) &&
+            (offset + len) <= (other->addr + subregion_size)) {
+            mr = other;
+            break;
+        }
+    }
+    return mr;
+}
+
 static MemTxResult zpci_read_bar(S390PCIBusDevice *pbdev, uint8_t pcias,
                                  uint64_t offset, uint64_t *data, uint8_t len)
 {
     MemoryRegion *mr;
 
     mr = pbdev->pdev->io_regions[pcias].memory;
+    mr = s390_get_subregion(mr, offset, len);
+    offset -= mr->addr;
     return memory_region_dispatch_read(mr, offset, data, len,
                                        MEMTXATTRS_UNSPECIFIED);
 }
@@ -443,30 +462,14 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
     return 0;
 }
 
-static int trap_msix(S390PCIBusDevice *pbdev, uint64_t offset, uint8_t pcias)
-{
-    if (pbdev->msix.available && pbdev->msix.table_bar == pcias &&
-        offset >= pbdev->msix.table_offset &&
-        offset < (pbdev->msix.table_offset +
-                  pbdev->msix.entries * PCI_MSIX_ENTRY_SIZE)) {
-        return 1;
-    } else {
-        return 0;
-    }
-}
-
 static MemTxResult zpci_write_bar(S390PCIBusDevice *pbdev, uint8_t pcias,
                                   uint64_t offset, uint64_t data, uint8_t len)
 {
     MemoryRegion *mr;
 
-    if (trap_msix(pbdev, offset, pcias)) {
-        offset = offset - pbdev->msix.table_offset;
-        mr = &pbdev->pdev->msix_table_mmio;
-    } else {
-        mr = pbdev->pdev->io_regions[pcias].memory;
-    }
-
+    mr = pbdev->pdev->io_regions[pcias].memory;
+    mr = s390_get_subregion(mr, offset, len);
+    offset -= mr->addr;
     return memory_region_dispatch_write(mr, offset, data, len,
                                         MEMTXATTRS_UNSPECIFIED);
 }
@@ -728,6 +731,9 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
     }
 
     mr = pbdev->pdev->io_regions[pcias].memory;
+    mr = s390_get_subregion(mr, offset, len);
+    offset -= mr->addr;
+
     if (!memory_region_access_valid(mr, offset, len, true)) {
         program_interrupt(env, PGM_OPERAND, 6);
         return 0;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/7] s390x/pci: Improve zPCI to cover more cases

[Cornelia Huck]

On Tue,  7 Nov 2017 18:24:32 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> Right now the PCI support is very limited (e.g. pass through of a
> host vfio device)
> To enable features like virtio-pci several modifications needs to be
> done.
> 
> Virtio-PCI uses subregions, which may eventually be discontinuous
> inside bars instead of a single flat region.
> The address offset being formerly calculated from the BAR base address
> must be adapted to the subregions instead of to the single region.
> 
> This patch provides the new calculation for the three kind of BAR
> access, zPCI STORE, zPCI LOAD and zPCI STORE BLOCK.
> 
> We use the opportunity to
>  - enhance the fault detection for zPCI STORE and LOAD,
>  - enhance the fault detection and to provide the maximum STORE BLOCK
>    block size, maxstbl, for zPCI STORE BLOCK
>  - factor out part of the code used to calculate the offset and
>    access the BARs,
>  - factor out the code for endianess conversion.

Will look at this later. Just a quick question: What kind of further
functionality is enabled by this? E.g., I can attach a virtio-net-pci
device right now, does this enable more virtio devices?

> 
> 
> Pierre Morel (7):
>   s390x/pci: factor out endianess conversion
>   s390x/pci: rework PCI STORE
>   s390x/pci: rework PCI LOAD
>   s390x/pci: rework PCI STORE BLOCK
>   s390x/pci: move the memory region read from pcilg
>   s390x/pci: move the memory region write from pcistg
>   s390x/pci: search for subregion inside the BARs
> 
>  hw/s390x/s390-pci-bus.h  |   1 +
>  hw/s390x/s390-pci-inst.c | 250 ++++++++++++++++++++++++++++-------------------
>  hw/s390x/s390-pci-inst.h |   2 +-
>  3 files changed, 153 insertions(+), 100 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/7] s390x/pci: Improve zPCI to cover more cases

[Christian Borntraeger]

On 11/07/2017 06:31 PM, Cornelia Huck wrote:
> On Tue,  7 Nov 2017 18:24:32 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> Right now the PCI support is very limited (e.g. pass through of a
>> host vfio device)
>> To enable features like virtio-pci several modifications needs to be
>> done.
>>
>> Virtio-PCI uses subregions, which may eventually be discontinuous
>> inside bars instead of a single flat region.
>> The address offset being formerly calculated from the BAR base address
>> must be adapted to the subregions instead of to the single region.
>>
>> This patch provides the new calculation for the three kind of BAR
>> access, zPCI STORE, zPCI LOAD and zPCI STORE BLOCK.
>>
>> We use the opportunity to
>>  - enhance the fault detection for zPCI STORE and LOAD,
>>  - enhance the fault detection and to provide the maximum STORE BLOCK
>>    block size, maxstbl, for zPCI STORE BLOCK
>>  - factor out part of the code used to calculate the offset and
>>    access the BARs,
>>  - factor out the code for endianess conversion.
> 
> Will look at this later. Just a quick question: What kind of further
> functionality is enabled by this? E.g., I can attach a virtio-net-pci
> device right now, does this enable more virtio devices?

You can attach a virtio-net-pci today, with these patches it now starts to work ;-)
In essence to me this all looks like a bugfix (a big one though)

> 
>>
>>
>> Pierre Morel (7):
>>   s390x/pci: factor out endianess conversion
>>   s390x/pci: rework PCI STORE
>>   s390x/pci: rework PCI LOAD
>>   s390x/pci: rework PCI STORE BLOCK
>>   s390x/pci: move the memory region read from pcilg
>>   s390x/pci: move the memory region write from pcistg
>>   s390x/pci: search for subregion inside the BARs
>>
>>  hw/s390x/s390-pci-bus.h  |   1 +
>>  hw/s390x/s390-pci-inst.c | 250 ++++++++++++++++++++++++++++-------------------
>>  hw/s390x/s390-pci-inst.h |   2 +-
>>  3 files changed, 153 insertions(+), 100 deletions(-)
>>
> 

Re: [Qemu-devel] [PATCH 0/7] s390x/pci: Improve zPCI to cover more cases

[Cornelia Huck]

On Tue, 7 Nov 2017 18:50:10 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 11/07/2017 06:31 PM, Cornelia Huck wrote:
> > On Tue,  7 Nov 2017 18:24:32 +0100
> > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> >   
> >> Right now the PCI support is very limited (e.g. pass through of a
> >> host vfio device)
> >> To enable features like virtio-pci several modifications needs to be
> >> done.
> >>
> >> Virtio-PCI uses subregions, which may eventually be discontinuous
> >> inside bars instead of a single flat region.
> >> The address offset being formerly calculated from the BAR base address
> >> must be adapted to the subregions instead of to the single region.
> >>
> >> This patch provides the new calculation for the three kind of BAR
> >> access, zPCI STORE, zPCI LOAD and zPCI STORE BLOCK.
> >>
> >> We use the opportunity to
> >>  - enhance the fault detection for zPCI STORE and LOAD,
> >>  - enhance the fault detection and to provide the maximum STORE BLOCK
> >>    block size, maxstbl, for zPCI STORE BLOCK
> >>  - factor out part of the code used to calculate the offset and
> >>    access the BARs,
> >>  - factor out the code for endianess conversion.  
> > 
> > Will look at this later. Just a quick question: What kind of further
> > functionality is enabled by this? E.g., I can attach a virtio-net-pci
> > device right now, does this enable more virtio devices?  
> 
> You can attach a virtio-net-pci today, with these patches it now starts to work ;-)

Yes, that's definitely an improvement ;)

> In essence to me this all looks like a bugfix (a big one though)

I'll look at it, but it probably is too big for 2.11.

> 
> >   
> >>
> >>
> >> Pierre Morel (7):
> >>   s390x/pci: factor out endianess conversion
> >>   s390x/pci: rework PCI STORE
> >>   s390x/pci: rework PCI LOAD
> >>   s390x/pci: rework PCI STORE BLOCK
> >>   s390x/pci: move the memory region read from pcilg
> >>   s390x/pci: move the memory region write from pcistg
> >>   s390x/pci: search for subregion inside the BARs
> >>
> >>  hw/s390x/s390-pci-bus.h  |   1 +
> >>  hw/s390x/s390-pci-inst.c | 250 ++++++++++++++++++++++++++++-------------------
> >>  hw/s390x/s390-pci-inst.h |   2 +-
> >>  3 files changed, 153 insertions(+), 100 deletions(-)
> >>  
> >   
> 

Re: [Qemu-devel] [PATCH 1/7] s390x/pci: factor out endianess conversion

[Cornelia Huck]

On Tue,  7 Nov 2017 18:24:33 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> There are two places where the same endianness conversion
> is done.
> Let's factor this out into a static function.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-pci-inst.c | 58 ++++++++++++++++++++++++++----------------------
>  1 file changed, 32 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 8e088f3..8fcb02d 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -314,6 +314,35 @@ out:
>      return 0;
>  }
>  
> +/**
> + * This function swaps the data at ptr according from one
> + * endianness to the other.
> + * valid data in the uint64_t data field.

I'm not sure what that line is supposed to mean?

> + * @ptr: a pointer to a uint64_t data field
> + * @len: the length of the valid data, must be 1,2,4 or 8
> + */
> +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
> +{
> +    uint64_t data = *ptr;
> +    switch (len) {
> +    case 1:
> +        break;
> +    case 2:
> +        data = bswap16(data);
> +        break;
> +    case 4:
> +        data = bswap32(data);
> +        break;
> +    case 8:
> +        data = bswap64(data);
> +        break;
> +    default:
> +        return -EINVAL;
> +    }
> +    *ptr = data;
> +    return 0;
> +}
> +

I was expecting more code to use a similar pattern, but it seems
surprisingly uncommon.

Re: [Qemu-devel] [PATCH 2/7] s390x/pci: rework PCI STORE

[Cornelia Huck]

On Tue,  7 Nov 2017 18:24:34 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> Enhance the fault detection, correction of the fault reporting.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-pci-inst.c | 41 ++++++++++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 8fcb02d..4a2f996 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -469,6 +469,12 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
>      pcias = (env->regs[r2] >> 16) & 0xf;
>      len = env->regs[r2] & 0xf;
>      offset = env->regs[r2 + 1];
> +    data = env->regs[r1];
> +
> +    if (!(fh & FH_MASK_ENABLE)) {

This covers the reserved/standby/disabled states, right?

> +        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
> +        return 0;
> +    }
>  
>      pbdev = s390_pci_find_dev_by_fh(s390_get_phb(), fh);
>      if (!pbdev) {
> @@ -478,12 +484,7 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
>      }
>  
>      switch (pbdev->state) {
> -    case ZPCI_FS_RESERVED:
> -    case ZPCI_FS_STANDBY:
> -    case ZPCI_FS_DISABLED:
>      case ZPCI_FS_PERMANENT_ERROR:
> -        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
> -        return 0;
>      case ZPCI_FS_ERROR:
>          setcc(cpu, ZPCI_PCI_LS_ERR);
>          s390_set_status_code(env, r2, ZPCI_PCI_ST_BLOCKED);
> @@ -492,9 +493,13 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
>          break;
>      }
>  
> -    data = env->regs[r1];
> -    if (pcias < 6) {
> -        if ((8 - (offset & 0x7)) < len) {
> +    /* Test that the pcias is a valid pcias and do the according operations */

s/according/appropriate/ ?

> +    switch (pcias) {
> +    case 0 ... 5:
> +        /* Check length:
> +         * A length of 0 is invalid and length should not cross a double word
> +         */
> +        if (!len || (len > (8 - (offset & 0x7)))) {
>              program_interrupt(env, PGM_OPERAND, 4);
>              return 0;
>          }
> @@ -512,21 +517,23 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
>              program_interrupt(env, PGM_OPERAND, 4);
>              return 0;
>          }
> -    } else if (pcias == 15) {
> -        if ((4 - (offset & 0x3)) < len) {
> -            program_interrupt(env, PGM_OPERAND, 4);
> -            return 0;
> -        }
> -
> -        if (zpci_endian_swap(&data, len)) {
> +        break;
> +    case 15:
> +        /* ZPCI uses the pseudo BAR number 15 as configuration space */
> +        /* possible access length are 1,2,4 and must not cross a word */

s/length/lengths/

> +        if (!len || (len > (4 - (offset & 0x3))) || len == 3) {
>              program_interrupt(env, PGM_OPERAND, 4);
>              return 0;
>          }
> -
> +        /* len = 1,2,4 so we do not need to test */
> +        zpci_endian_swap(&data, len);
>          pci_host_config_write_common(pbdev->pdev, offset,
>                                       pci_config_size(pbdev->pdev),
>                                       data, len);
> -    } else {
> +        break;
> +    case PCI_ROM_SLOT:

So, will this be filled in a later patch? (Reading from the top.)

> +        /* Fall through */
> +    default:
>          DPRINTF("pcistg invalid space\n");
>          setcc(cpu, ZPCI_PCI_LS_ERR);
>          s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);

On the whole, I think this improves readability, especially for folks
that do not have access to the spec.

Re: [Qemu-devel] [PATCH 3/7] s390x/pci: rework PCI LOAD

[Cornelia Huck]

On Tue,  7 Nov 2017 18:24:35 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> Enhance the fault detection, correction of the fault reporting.

Basically the same comments as for the previous patch (but looks good
in general).

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

Re: [Qemu-devel] [PATCH 1/7] s390x/pci: factor out endianess conversion

[Philippe Mathieu-Daudé]

On 11/09/2017 01:38 PM, Cornelia Huck wrote:
> On Tue,  7 Nov 2017 18:24:33 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> There are two places where the same endianness conversion
>> is done.
>> Let's factor this out into a static function.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/s390-pci-inst.c | 58 ++++++++++++++++++++++++++----------------------
>>  1 file changed, 32 insertions(+), 26 deletions(-)
>>
>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>> index 8e088f3..8fcb02d 100644
>> --- a/hw/s390x/s390-pci-inst.c
>> +++ b/hw/s390x/s390-pci-inst.c
>> @@ -314,6 +314,35 @@ out:
>>      return 0;
>>  }
>>  
>> +/**
>> + * This function swaps the data at ptr according from one
>> + * endianness to the other.
>> + * valid data in the uint64_t data field.
> 
> I'm not sure what that line is supposed to mean?
> 
>> + * @ptr: a pointer to a uint64_t data field
>> + * @len: the length of the valid data, must be 1,2,4 or 8
>> + */
>> +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
>> +{
>> +    uint64_t data = *ptr;
>> +    switch (len) {
>> +    case 1:
>> +        break;
>> +    case 2:
>> +        data = bswap16(data);
>> +        break;
>> +    case 4:
>> +        data = bswap32(data);
>> +        break;
>> +    case 8:
>> +        data = bswap64(data);
>> +        break;
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +    *ptr = data;
>> +    return 0;
>> +}

This is usually care taken by memory::adjust_endianness() ...

> I was expecting more code to use a similar pattern, but it seems
> surprisingly uncommon.

Which ring a bell for latent bug?

This remind me of a similar issue on ppc:

http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05121.html
...
http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05666.html

Re: [Qemu-devel] [PATCH 1/7] s390x/pci: factor out endianess conversion

[Cornelia Huck]

On Thu, 9 Nov 2017 15:55:46 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> On 11/09/2017 01:38 PM, Cornelia Huck wrote:
> > On Tue,  7 Nov 2017 18:24:33 +0100
> > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> >   
> >> There are two places where the same endianness conversion
> >> is done.
> >> Let's factor this out into a static function.
> >>
> >> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> >> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >> ---
> >>  hw/s390x/s390-pci-inst.c | 58 ++++++++++++++++++++++++++----------------------
> >>  1 file changed, 32 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> >> index 8e088f3..8fcb02d 100644
> >> --- a/hw/s390x/s390-pci-inst.c
> >> +++ b/hw/s390x/s390-pci-inst.c
> >> @@ -314,6 +314,35 @@ out:
> >>      return 0;
> >>  }
> >>  
> >> +/**
> >> + * This function swaps the data at ptr according from one
> >> + * endianness to the other.
> >> + * valid data in the uint64_t data field.  
> > 
> > I'm not sure what that line is supposed to mean?
> >   
> >> + * @ptr: a pointer to a uint64_t data field
> >> + * @len: the length of the valid data, must be 1,2,4 or 8
> >> + */
> >> +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
> >> +{
> >> +    uint64_t data = *ptr;
> >> +    switch (len) {
> >> +    case 1:
> >> +        break;
> >> +    case 2:
> >> +        data = bswap16(data);
> >> +        break;
> >> +    case 4:
> >> +        data = bswap32(data);
> >> +        break;
> >> +    case 8:
> >> +        data = bswap64(data);
> >> +        break;
> >> +    default:
> >> +        return -EINVAL;
> >> +    }
> >> +    *ptr = data;
> >> +    return 0;
> >> +}  
> 
> This is usually care taken by memory::adjust_endianness() ...

Yes, but that's not a memory region write.

> 
> > I was expecting more code to use a similar pattern, but it seems
> > surprisingly uncommon.  
> 
> Which ring a bell for latent bug?

Looking at this, it seems there *is* a latent bug, which has not popped
up so far as the pci instructions are not wired up in tcg yet. This
code is only called from the kvm path...

> 
> This remind me of a similar issue on ppc:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05121.html
> ...
> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05666.html

Re: [Qemu-devel] [PATCH 6/7] s390x/pci: move the memory region write from pcistg

[Cornelia Huck]

On Tue,  7 Nov 2017 18:24:38 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> Let's move the memory region write from pcistg into a dedicated
> function.
> This allows us to prepare a later patch searching for subregions
> inside of the memory region.

OK, so here is the memory region write. Do we have any sleeping
endianness bugs in there for when we wire up tcg? I'm not sure how this
plays with the bswaps (see patch 1). 

But maybe I've just gotten lost somewhere.

> 
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-pci-inst.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 50135a0..97f62b5 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -455,12 +455,27 @@ static int trap_msix(S390PCIBusDevice *pbdev, uint64_t offset, uint8_t pcias)
>      }
>  }
>  
> +static MemTxResult zpci_write_bar(S390PCIBusDevice *pbdev, uint8_t pcias,
> +                                  uint64_t offset, uint64_t data, uint8_t len)
> +{
> +    MemoryRegion *mr;
> +
> +    if (trap_msix(pbdev, offset, pcias)) {
> +        offset = offset - pbdev->msix.table_offset;
> +        mr = &pbdev->pdev->msix_table_mmio;
> +    } else {
> +        mr = pbdev->pdev->io_regions[pcias].memory;
> +    }
> +
> +    return memory_region_dispatch_write(mr, offset, data, len,
> +                                        MEMTXATTRS_UNSPECIFIED);
> +}
> +
>  int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
>  {
>      CPUS390XState *env = &cpu->env;
>      uint64_t offset, data;
>      S390PCIBusDevice *pbdev;
> -    MemoryRegion *mr;
>      MemTxResult result;
>      uint8_t len;
>      uint32_t fh;
> @@ -517,15 +532,7 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
>              return 0;
>          }
>  
> -        if (trap_msix(pbdev, offset, pcias)) {
> -            offset = offset - pbdev->msix.table_offset;
> -            mr = &pbdev->pdev->msix_table_mmio;
> -        } else {
> -            mr = pbdev->pdev->io_regions[pcias].memory;
> -        }
> -
> -        result = memory_region_dispatch_write(mr, offset, data, len,
> -                                     MEMTXATTRS_UNSPECIFIED);
> +        result = zpci_write_bar(pbdev, pcias, offset, data, len);
>          if (result != MEMTX_OK) {
>              program_interrupt(env, PGM_OPERAND, 4);
>              return 0;

Re: [Qemu-devel] [PATCH 2/7] s390x/pci: rework PCI STORE

[Yi Min Zhao]

在 2017/11/10 上午12:50, Cornelia Huck 写道:
> On Tue,  7 Nov 2017 18:24:34 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>
>> Enhance the fault detection, correction of the fault reporting.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>> ---
>>   hw/s390x/s390-pci-inst.c | 41 ++++++++++++++++++++++++-----------------
>>   1 file changed, 24 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>> index 8fcb02d..4a2f996 100644
>> --- a/hw/s390x/s390-pci-inst.c
>> +++ b/hw/s390x/s390-pci-inst.c
>> @@ -469,6 +469,12 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
>>       pcias = (env->regs[r2] >> 16) & 0xf;
>>       len = env->regs[r2] & 0xf;
>>       offset = env->regs[r2 + 1];
>> +    data = env->regs[r1];
>> +
>> +    if (!(fh & FH_MASK_ENABLE)) {
> This covers the reserved/standby/disabled states, right?
yes

[...]

Re: [Qemu-devel] [PATCH 6/7] s390x/pci: move the memory region write from pcistg

[Yi Min Zhao]

在 2017/11/10 上午3:23, Cornelia Huck 写道:
> On Tue,  7 Nov 2017 18:24:38 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>
>> Let's move the memory region write from pcistg into a dedicated
>> function.
>> This allows us to prepare a later patch searching for subregions
>> inside of the memory region.
> OK, so here is the memory region write. Do we have any sleeping
> endianness bugs in there for when we wire up tcg? I'm not sure how this
> plays with the bswaps (see patch 1).
>
> But maybe I've just gotten lost somewhere.
I think there's no error. For PCI bars' MRs, we got the little-endian data
that is exactly fit to the byte ordering of pcilg instruction. For PCI 
config
space, the data has been swapped according to the cpu byte ordering.
So we use zpci_swap_endian() to swap the data back to the little-endian
ordering.
>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>> ---
>>   hw/s390x/s390-pci-inst.c | 27 +++++++++++++++++----------
>>   1 file changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>> index 50135a0..97f62b5 100644
>> --- a/hw/s390x/s390-pci-inst.c
>> +++ b/hw/s390x/s390-pci-inst.c
>> @@ -455,12 +455,27 @@ static int trap_msix(S390PCIBusDevice *pbdev, uint64_t offset, uint8_t pcias)
>>       }
>>   }
>>   
>> +static MemTxResult zpci_write_bar(S390PCIBusDevice *pbdev, uint8_t pcias,
>> +                                  uint64_t offset, uint64_t data, uint8_t len)
>> +{
>> +    MemoryRegion *mr;
>> +
>> +    if (trap_msix(pbdev, offset, pcias)) {
>> +        offset = offset - pbdev->msix.table_offset;
>> +        mr = &pbdev->pdev->msix_table_mmio;
>> +    } else {
>> +        mr = pbdev->pdev->io_regions[pcias].memory;
>> +    }
>> +
>> +    return memory_region_dispatch_write(mr, offset, data, len,
>> +                                        MEMTXATTRS_UNSPECIFIED);
>> +}
>> +
>>   int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
>>   {
>>       CPUS390XState *env = &cpu->env;
>>       uint64_t offset, data;
>>       S390PCIBusDevice *pbdev;
>> -    MemoryRegion *mr;
>>       MemTxResult result;
>>       uint8_t len;
>>       uint32_t fh;
>> @@ -517,15 +532,7 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
>>               return 0;
>>           }
>>   
>> -        if (trap_msix(pbdev, offset, pcias)) {
>> -            offset = offset - pbdev->msix.table_offset;
>> -            mr = &pbdev->pdev->msix_table_mmio;
>> -        } else {
>> -            mr = pbdev->pdev->io_regions[pcias].memory;
>> -        }
>> -
>> -        result = memory_region_dispatch_write(mr, offset, data, len,
>> -                                     MEMTXATTRS_UNSPECIFIED);
>> +        result = zpci_write_bar(pbdev, pcias, offset, data, len);
>>           if (result != MEMTX_OK) {
>>               program_interrupt(env, PGM_OPERAND, 4);
>>               return 0;
>

Re: [Qemu-devel] [PATCH 6/7] s390x/pci: move the memory region write from pcistg

[Cornelia Huck]

On Fri, 10 Nov 2017 17:40:12 +0800
Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:

> 在 2017/11/10 上午3:23, Cornelia Huck 写道:
> > On Tue,  7 Nov 2017 18:24:38 +0100
> > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> >  
> >> Let's move the memory region write from pcistg into a dedicated
> >> function.
> >> This allows us to prepare a later patch searching for subregions
> >> inside of the memory region.  
> > OK, so here is the memory region write. Do we have any sleeping
> > endianness bugs in there for when we wire up tcg? I'm not sure how this
> > plays with the bswaps (see patch 1).
> >
> > But maybe I've just gotten lost somewhere.  
> I think there's no error. For PCI bars' MRs, we got the little-endian data
> that is exactly fit to the byte ordering of pcilg instruction. For PCI 
> config
> space, the data has been swapped according to the cpu byte ordering.

Host or target cpu?

> So we use zpci_swap_endian() to swap the data back to the little-endian
> ordering.

That swap is unconditional. If we were running on a little-endian host,
it would be wrong, wouldn't it?

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

Re: [Qemu-devel] [PATCH 2/7] s390x/pci: rework PCI STORE

[Pierre Morel]

On 09/11/2017 17:50, Cornelia Huck wrote:
> On Tue,  7 Nov 2017 18:24:34 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> Enhance the fault detection, correction of the fault reporting.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>> ---
>>   hw/s390x/s390-pci-inst.c | 41 ++++++++++++++++++++++++-----------------
>>   1 file changed, 24 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>> index 8fcb02d..4a2f996 100644
>> --- a/hw/s390x/s390-pci-inst.c
>> +++ b/hw/s390x/s390-pci-inst.c
>> @@ -469,6 +469,12 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
>>       pcias = (env->regs[r2] >> 16) & 0xf;
>>       len = env->regs[r2] & 0xf;
>>       offset = env->regs[r2 + 1];
>> +    data = env->regs[r1];
>> +
>> +    if (!(fh & FH_MASK_ENABLE)) {
> 
> This covers the reserved/standby/disabled states, right?

yes it does, all these states have the FH_MASK_ENABLE set to 0

> 
>> +        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
>> +        return 0;
>> +    }
>>   
>>       pbdev = s390_pci_find_dev_by_fh(s390_get_phb(), fh);
>>       if (!pbdev) {
>> @@ -478,12 +484,7 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
>>       }
>>   
>>       switch (pbdev->state) {
>> -    case ZPCI_FS_RESERVED:
>> -    case ZPCI_FS_STANDBY:
>> -    case ZPCI_FS_DISABLED:
>>       case ZPCI_FS_PERMANENT_ERROR:
>> -        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
>> -        return 0;
>>       case ZPCI_FS_ERROR:
>>           setcc(cpu, ZPCI_PCI_LS_ERR);
>>           s390_set_status_code(env, r2, ZPCI_PCI_ST_BLOCKED);
>> @@ -492,9 +493,13 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
>>           break;
>>       }
>>   
>> -    data = env->regs[r1];
>> -    if (pcias < 6) {
>> -        if ((8 - (offset & 0x7)) < len) {
>> +    /* Test that the pcias is a valid pcias and do the according operations */
> 
> s/according/appropriate/ ?

yes, better, thanks

> 
>> +    switch (pcias) {
>> +    case 0 ... 5:
>> +        /* Check length:
>> +         * A length of 0 is invalid and length should not cross a double word
>> +         */
>> +        if (!len || (len > (8 - (offset & 0x7)))) {
>>               program_interrupt(env, PGM_OPERAND, 4);
>>               return 0;
>>           }
>> @@ -512,21 +517,23 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
>>               program_interrupt(env, PGM_OPERAND, 4);
>>               return 0;
>>           }
>> -    } else if (pcias == 15) {
>> -        if ((4 - (offset & 0x3)) < len) {
>> -            program_interrupt(env, PGM_OPERAND, 4);
>> -            return 0;
>> -        }
>> -
>> -        if (zpci_endian_swap(&data, len)) {
>> +        break;
>> +    case 15:
>> +        /* ZPCI uses the pseudo BAR number 15 as configuration space */
>> +        /* possible access length are 1,2,4 and must not cross a word */
> 
> s/length/lengths/
yes, thanks

> 
>> +        if (!len || (len > (4 - (offset & 0x3))) || len == 3) {
>>               program_interrupt(env, PGM_OPERAND, 4);
>>               return 0;
>>           }
>> -
>> +        /* len = 1,2,4 so we do not need to test */
>> +        zpci_endian_swap(&data, len);
>>           pci_host_config_write_common(pbdev->pdev, offset,
>>                                        pci_config_size(pbdev->pdev),
>>                                        data, len);
>> -    } else {
>> +        break;
>> +    case PCI_ROM_SLOT:
> 
> So, will this be filled in a later patch? (Reading from the top.)

No it will not it is only a reminder that we explicitly do not support it.
Since I do not know if we ever will support it do you think I better let 
it fall?

> 
>> +        /* Fall through */
>> +    default:
>>           DPRINTF("pcistg invalid space\n");
>>           setcc(cpu, ZPCI_PCI_LS_ERR);
>>           s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);
> 
> On the whole, I think this improves readability, especially for folks
> that do not have access to the spec.
> 


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

Re: [Qemu-devel] [PATCH 3/7] s390x/pci: rework PCI LOAD

[Pierre Morel]

On 09/11/2017 17:51, Cornelia Huck wrote:
> On Tue,  7 Nov 2017 18:24:35 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> Enhance the fault detection, correction of the fault reporting.
> 
> Basically the same comments as for the previous patch (but looks good
> in general).

thanks I will process it the same way as patch 2 then.

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


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

Re: [Qemu-devel] [PATCH 6/7] s390x/pci: move the memory region write from pcistg

[Pierre Morel]

On 10/11/2017 10:51, Cornelia Huck wrote:
> On Fri, 10 Nov 2017 17:40:12 +0800
> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
> 
>> 在 2017/11/10 上午3:23, Cornelia Huck 写道:
>>> On Tue,  7 Nov 2017 18:24:38 +0100
>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>>   
>>>> Let's move the memory region write from pcistg into a dedicated
>>>> function.
>>>> This allows us to prepare a later patch searching for subregions
>>>> inside of the memory region.
>>> OK, so here is the memory region write. Do we have any sleeping
>>> endianness bugs in there for when we wire up tcg? I'm not sure how this
>>> plays with the bswaps (see patch 1).
>>>
>>> But maybe I've just gotten lost somewhere.
>> I think there's no error. For PCI bars' MRs, we got the little-endian data
>> that is exactly fit to the byte ordering of pcilg instruction. For PCI
>> config
>> space, the data has been swapped according to the cpu byte ordering.
> 
> Host or target cpu?
> 
>> So we use zpci_swap_endian() to swap the data back to the little-endian
>> ordering.

I do not see where we use the zpci_swap_endian() function in this patch 
or in the zpci_write_bar() function.

> 
> That swap is unconditional. If we were running on a little-endian host,
> it would be wrong, wouldn't it?

I think there is no problem here, we do not use this function but we use 
the memory_region_dispatch_write() to access a subregion of the PCI 
device which is defined as DEVICE_LITTLE_ENDIAN

AFAIU The memory access process the endianness correctly.

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


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

Re: [Qemu-devel] [PATCH 1/7] s390x/pci: factor out endianess conversion

[Pierre Morel]

On 09/11/2017 19:55, Philippe Mathieu-Daudé wrote:
> On 11/09/2017 01:38 PM, Cornelia Huck wrote:
>> On Tue,  7 Nov 2017 18:24:33 +0100
>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>
>>> There are two places where the same endianness conversion
>>> is done.
>>> Let's factor this out into a static function.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>> ---
>>>   hw/s390x/s390-pci-inst.c | 58 ++++++++++++++++++++++++++----------------------
>>>   1 file changed, 32 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>>> index 8e088f3..8fcb02d 100644
>>> --- a/hw/s390x/s390-pci-inst.c
>>> +++ b/hw/s390x/s390-pci-inst.c
>>> @@ -314,6 +314,35 @@ out:
>>>       return 0;
>>>   }
>>>   
>>> +/**
>>> + * This function swaps the data at ptr according from one
>>> + * endianness to the other.
>>> + * valid data in the uint64_t data field.
>>
>> I'm not sure what that line is supposed to mean?
>>
>>> + * @ptr: a pointer to a uint64_t data field
>>> + * @len: the length of the valid data, must be 1,2,4 or 8
>>> + */
>>> +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
>>> +{
>>> +    uint64_t data = *ptr;
>>> +    switch (len) {
>>> +    case 1:
>>> +        break;
>>> +    case 2:
>>> +        data = bswap16(data);
>>> +        break;
>>> +    case 4:
>>> +        data = bswap32(data);
>>> +        break;
>>> +    case 8:
>>> +        data = bswap64(data);
>>> +        break;
>>> +    default:
>>> +        return -EINVAL;
>>> +    }
>>> +    *ptr = data;
>>> +    return 0;
>>> +}
> 
> This is usually care taken by memory::adjust_endianness() ...

We are here intercepting an instruction with the data in a register.
That is what troubles me, but I will take a deeper look.

> 
>> I was expecting more code to use a similar pattern, but it seems
>> surprisingly uncommon.
> 
> Which ring a bell for latent bug?
> 
> This remind me of a similar issue on ppc:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05121.html
> ...
> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05666.html
> 

Thanks for the pointers.



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

Re: [Qemu-devel] [PATCH 1/7] s390x/pci: factor out endianess conversion

[Pierre Morel]

On 09/11/2017 19:55, Philippe Mathieu-Daudé wrote:
> On 11/09/2017 01:38 PM, Cornelia Huck wrote:
>> On Tue,  7 Nov 2017 18:24:33 +0100
>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>
>>> There are two places where the same endianness conversion
>>> is done.
>>> Let's factor this out into a static function.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>> ---
>>>   hw/s390x/s390-pci-inst.c | 58 ++++++++++++++++++++++++++----------------------
>>>   1 file changed, 32 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>>> index 8e088f3..8fcb02d 100644
>>> --- a/hw/s390x/s390-pci-inst.c
>>> +++ b/hw/s390x/s390-pci-inst.c
>>> @@ -314,6 +314,35 @@ out:
>>>       return 0;
>>>   }
>>>   
>>> +/**
>>> + * This function swaps the data at ptr according from one
>>> + * endianness to the other.
>>> + * valid data in the uint64_t data field.
>>
>> I'm not sure what that line is supposed to mean?
>>
>>> + * @ptr: a pointer to a uint64_t data field
>>> + * @len: the length of the valid data, must be 1,2,4 or 8
>>> + */
>>> +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
>>> +{
>>> +    uint64_t data = *ptr;
>>> +    switch (len) {
>>> +    case 1:
>>> +        break;
>>> +    case 2:
>>> +        data = bswap16(data);
>>> +        break;
>>> +    case 4:
>>> +        data = bswap32(data);
>>> +        break;
>>> +    case 8:
>>> +        data = bswap64(data);
>>> +        break;
>>> +    default:
>>> +        return -EINVAL;
>>> +    }
>>> +    *ptr = data;
>>> +    return 0;
>>> +}
> 
> This is usually care taken by memory::adjust_endianness() ...


We are here intercepting an instruction with the data in a register.
That is what troubles me, but I will take a deeper look.

> 
>> I was expecting more code to use a similar pattern, but it seems
>> surprisingly uncommon.
> 
> Which ring a bell for latent bug?
> 
> This remind me of a similar issue on ppc:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05121.html
> ...
> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05666.html
> 

Thanks for the pointers.


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

Re: [Qemu-devel] [PATCH 6/7] s390x/pci: move the memory region write from pcistg

[Pierre Morel]

On 10/11/2017 10:51, Cornelia Huck wrote:
> On Fri, 10 Nov 2017 17:40:12 +0800
> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
> 
>> 在 2017/11/10 上午3:23, Cornelia Huck 写道:
>>> On Tue,  7 Nov 2017 18:24:38 +0100
>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>>   
>>>> Let's move the memory region write from pcistg into a dedicated
>>>> function.
>>>> This allows us to prepare a later patch searching for subregions
>>>> inside of the memory region.
>>> OK, so here is the memory region write. Do we have any sleeping
>>> endianness bugs in there for when we wire up tcg? I'm not sure how this
>>> plays with the bswaps (see patch 1).
>>>
>>> But maybe I've just gotten lost somewhere.
>> I think there's no error. For PCI bars' MRs, we got the little-endian data
>> that is exactly fit to the byte ordering of pcilg instruction. For PCI
>> config
>> space, the data has been swapped according to the cpu byte ordering.
> 
> Host or target cpu?
> 
>> So we use zpci_swap_endian() to swap the data back to the little-endian
>> ordering.
> 


I do not see where we use the zpci_swap_endian() function in this patch 
or in the zpci_write_bar() function.



> That swap is unconditional. If we were running on a little-endian host,
> it would be wrong, wouldn't it?

I think there is no problem here, we do not use the swap function but we 
use the memory_region_dispatch_write() to access a subregion of the PCI 
device which is defined as DEVICE_LITTLE_ENDIAN

AFAIU The memory access process the endianness correctly.


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


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

Re: [Qemu-devel] [PATCH 3/7] s390x/pci: rework PCI LOAD

[Pierre Morel]

On 09/11/2017 17:51, Cornelia Huck wrote:
> On Tue,  7 Nov 2017 18:24:35 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> Enhance the fault detection, correction of the fault reporting.
> 
> Basically the same comments as for the previous patch (but looks good
> in general).

thanks I will process it the same way as patch 2/7 then.


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


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

Re: [Qemu-devel] [PATCH 2/7] s390x/pci: rework PCI STORE

[Cornelia Huck]

On Mon, 13 Nov 2017 10:03:37 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> On 09/11/2017 17:50, Cornelia Huck wrote:
> > On Tue,  7 Nov 2017 18:24:34 +0100
> > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> >> +    case PCI_ROM_SLOT:  
> > 
> > So, will this be filled in a later patch? (Reading from the top.)  
> 
> No it will not it is only a reminder that we explicitly do not support it.
> Since I do not know if we ever will support it do you think I better let 
> it fall?

Either add a comment, or drop it. Otherwise I will ask again in the
future :)

Re: [Qemu-devel] [PATCH 6/7] s390x/pci: move the memory region write from pcistg

[Cornelia Huck]

On Mon, 13 Nov 2017 10:39:50 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> On 10/11/2017 10:51, Cornelia Huck wrote:
> > On Fri, 10 Nov 2017 17:40:12 +0800
> > Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
> >   
> >> 在 2017/11/10 上午3:23, Cornelia Huck 写道:  
> >>> On Tue,  7 Nov 2017 18:24:38 +0100
> >>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> >>>     
> >>>> Let's move the memory region write from pcistg into a dedicated
> >>>> function.
> >>>> This allows us to prepare a later patch searching for subregions
> >>>> inside of the memory region.  
> >>> OK, so here is the memory region write. Do we have any sleeping
> >>> endianness bugs in there for when we wire up tcg? I'm not sure how this
> >>> plays with the bswaps (see patch 1).
> >>>
> >>> But maybe I've just gotten lost somewhere.  
> >> I think there's no error. For PCI bars' MRs, we got the little-endian data
> >> that is exactly fit to the byte ordering of pcilg instruction. For PCI
> >> config
> >> space, the data has been swapped according to the cpu byte ordering.  
> > 
> > Host or target cpu?
> >   
> >> So we use zpci_swap_endian() to swap the data back to the little-endian
> >> ordering.  
> >   
> 
> 
> I do not see where we use the zpci_swap_endian() function in this patch 
> or in the zpci_write_bar() function.

So, is that swap function only ever used to convert BE register
contents to LE?

> 
> 
> 
> > That swap is unconditional. If we were running on a little-endian host,
> > it would be wrong, wouldn't it?  
> 
> I think there is no problem here, we do not use the swap function but we 
> use the memory_region_dispatch_write() to access a subregion of the PCI 
> device which is defined as DEVICE_LITTLE_ENDIAN
> 
> AFAIU The memory access process the endianness correctly.

OK, if this is all going through generic memory mechanisms, we should
be fine.

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

Re: [Qemu-devel] [PATCH 2/7] s390x/pci: rework PCI STORE

[Pierre Morel]

On 13/11/2017 12:48, Cornelia Huck wrote:
> On Mon, 13 Nov 2017 10:03:37 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> On 09/11/2017 17:50, Cornelia Huck wrote:
>>> On Tue,  7 Nov 2017 18:24:34 +0100
>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>>>> +    case PCI_ROM_SLOT:
>>>
>>> So, will this be filled in a later patch? (Reading from the top.)
>>
>> No it will not it is only a reminder that we explicitly do not support it.
>> Since I do not know if we ever will support it do you think I better let
>> it fall?
> 
> Either add a comment, or drop it. Otherwise I will ask again in the
> future :)
> 

OK Ill do the easier, just suppress it and its brother in PCI LOAD. :)

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

Re: [Qemu-devel] [PATCH 6/7] s390x/pci: move the memory region write from pcistg

[Pierre Morel]

On 13/11/2017 12:54, Cornelia Huck wrote:
> On Mon, 13 Nov 2017 10:39:50 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> On 10/11/2017 10:51, Cornelia Huck wrote:
>>> On Fri, 10 Nov 2017 17:40:12 +0800
>>> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
>>>    
>>>> 在 2017/11/10 上午3:23, Cornelia Huck 写道:
>>>>> On Tue,  7 Nov 2017 18:24:38 +0100
>>>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>>>>      
>>>>>> Let's move the memory region write from pcistg into a dedicated
>>>>>> function.
>>>>>> This allows us to prepare a later patch searching for subregions
>>>>>> inside of the memory region.
>>>>> OK, so here is the memory region write. Do we have any sleeping
>>>>> endianness bugs in there for when we wire up tcg? I'm not sure how this
>>>>> plays with the bswaps (see patch 1).
>>>>>
>>>>> But maybe I've just gotten lost somewhere.
>>>> I think there's no error. For PCI bars' MRs, we got the little-endian data
>>>> that is exactly fit to the byte ordering of pcilg instruction. For PCI
>>>> config
>>>> space, the data has been swapped according to the cpu byte ordering.
>>>
>>> Host or target cpu?
>>>    
>>>> So we use zpci_swap_endian() to swap the data back to the little-endian
>>>> ordering.
>>>    
>>
>>
>> I do not see where we use the zpci_swap_endian() function in this patch
>> or in the zpci_write_bar() function.
> 
> So, is that swap function only ever used to convert BE register
> contents to LE?

Yes that is what I understood.
The value in the register is read from memory somehow but it may be an 
immediate value setup by another instruction, I think the TCG should 
have already make sure it is big endian.

On the other side the PCI memory is always Little endian AFAIK.

So the swap function is always from BIG to little.

But as I said, we do not use the zpci_swap_endian in this function, so I 
write this answer again in the thread where the definition of the 
function is to continue the discussion on the register to PCI 
translation there.

> 
>>
>>
>>
>>> That swap is unconditional. If we were running on a little-endian host,
>>> it would be wrong, wouldn't it?
>>
>> I think there is no problem here, we do not use the swap function but we
>> use the memory_region_dispatch_write() to access a subregion of the PCI
>> device which is defined as DEVICE_LITTLE_ENDIAN
>>
>> AFAIU The memory access process the endianness correctly.
> 
> OK, if this is all going through generic memory mechanisms, we should
> be fine. >
>>
>>
>>>    
>>>>>      
>>>>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>>>>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>     hw/s390x/s390-pci-inst.c | 27 +++++++++++++++++----------
>>>>>>     1 file changed, 17 insertions(+), 10 deletions(-)
>>>    
>>
>>
> 


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

Re: [Qemu-devel] [PATCH 4/7] s390x/pci: rework PCI STORE BLOCK

[Cornelia Huck]

On Tue,  7 Nov 2017 18:24:36 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> Enhance the fault detection.
> 
> Add the maxstbl entry to both the Query PCI Function Group
> response and the PCIBusDevice structure.
> 
> Initialize the maxstbl to 128 per default until we get
> the actual data from the hardware.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-pci-bus.h  |  1 +
>  hw/s390x/s390-pci-inst.c | 62 +++++++++++++++++++++++++++++-------------------
>  hw/s390x/s390-pci-inst.h |  2 +-
>  3 files changed, 40 insertions(+), 25 deletions(-)
> 

> @@ -662,22 +664,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>      fh = env->regs[r1] >> 32;
>      pcias = (env->regs[r1] >> 16) & 0xf;
>      len = env->regs[r1] & 0xff;
> +    offset = env->regs[r3];
>  
> -    if (pcias > 5) {
> -        DPRINTF("pcistb invalid space\n");
> -        setcc(cpu, ZPCI_PCI_LS_ERR);
> -        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
> -        return 0;
> -    }
> -
> -    switch (len) {
> -    case 16:
> -    case 32:
> -    case 64:
> -    case 128:
> -        break;
> -    default:
> -        program_interrupt(env, PGM_SPECIFICATION, 6);
> +    if (!(fh & FH_MASK_ENABLE)) {
> +        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);

So this means you move checking for the device before checking for the
parameters or the as.

>          return 0;
>      }
>  
> @@ -689,12 +679,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>      }
>  
>      switch (pbdev->state) {
> -    case ZPCI_FS_RESERVED:
> -    case ZPCI_FS_STANDBY:
> -    case ZPCI_FS_DISABLED:
>      case ZPCI_FS_PERMANENT_ERROR:
> -        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
> -        return 0;
>      case ZPCI_FS_ERROR:
>          setcc(cpu, ZPCI_PCI_LS_ERR);
>          s390_set_status_code(env, r1, ZPCI_PCI_ST_BLOCKED);
> @@ -703,8 +688,33 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>          break;
>      }
>  
> +    if (pcias > 5) {
> +        DPRINTF("pcistb invalid space\n");
> +        setcc(cpu, ZPCI_PCI_LS_ERR);
> +        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
> +        return 0;
> +    }
> +
> +    /* Verify the address, offset and length */
> +    /* offset must be a multiple of 8 */
> +    if (offset % 8) {
> +        goto addressing_error;
> +    }
> +    /* Length must be greater than 8, a multiple of 8, lower than maxstbl */
> +    if ((len <= 8) || (len % 8) || (len > pbdev->maxstbl)) {
> +        goto addressing_error;
> +    }
> +    /* Do not cross a 4K-byte boundary */
> +    if (((offset & 0xfff) + len) > 0x1000) {
> +        goto addressing_error;
> +    }
> +    /* Guest address must be double word aligned */
> +    if (gaddr & 0x07UL) {
> +        goto addressing_error;
> +    }

So the checks here are only evaluated if the instruction actually pokes
at a valid region?

> +
>      mr = pbdev->pdev->io_regions[pcias].memory;
> -    if (!memory_region_access_valid(mr, env->regs[r3], len, true)) {
> +    if (!memory_region_access_valid(mr, offset, len, true)) {
>          program_interrupt(env, PGM_OPERAND, 6);
>          return 0;
>      }
> @@ -714,9 +724,9 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>      }
>  
>      for (i = 0; i < len / 8; i++) {
> -        result = memory_region_dispatch_write(mr, env->regs[r3] + i * 8,
> -                                     ldq_p(buffer + i * 8), 8,
> -                                     MEMTXATTRS_UNSPECIFIED);
> +        result = memory_region_dispatch_write(mr, offset + i * 8,
> +                                              ldq_p(buffer + i * 8), 8,
> +                                              MEMTXATTRS_UNSPECIFIED);
>          if (result != MEMTX_OK) {
>              program_interrupt(env, PGM_OPERAND, 6);
>              return 0;
> @@ -725,6 +735,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>  
>      setcc(cpu, ZPCI_PCI_LS_OK);
>      return 0;
> +
> +addressing_error:
> +    program_interrupt(env, PGM_SPECIFICATION, 6);
> +    return 0;
>  }

This seems more readable; I can't verify whether it is actually correct
without access to the architecture, though :(

Re: [Qemu-devel] [PATCH 1/7] s390x/pci: factor out endianess conversion

[Pierre Morel]

On 09/11/2017 20:20, Cornelia Huck wrote:
> On Thu, 9 Nov 2017 15:55:46 -0300
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
>> On 11/09/2017 01:38 PM, Cornelia Huck wrote:
>>> On Tue,  7 Nov 2017 18:24:33 +0100
>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>>    
>>>> There are two places where the same endianness conversion
>>>> is done.
>>>> Let's factor this out into a static function.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>> ---
>>>>   hw/s390x/s390-pci-inst.c | 58 ++++++++++++++++++++++++++----------------------
>>>>   1 file changed, 32 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>>>> index 8e088f3..8fcb02d 100644
>>>> --- a/hw/s390x/s390-pci-inst.c
>>>> +++ b/hw/s390x/s390-pci-inst.c
>>>> @@ -314,6 +314,35 @@ out:
>>>>       return 0;
>>>>   }
>>>>   
>>>> +/**
>>>> + * This function swaps the data at ptr according from one
>>>> + * endianness to the other.
>>>> + * valid data in the uint64_t data field.
>>>
>>> I'm not sure what that line is supposed to mean?
>>>    
>>>> + * @ptr: a pointer to a uint64_t data field
>>>> + * @len: the length of the valid data, must be 1,2,4 or 8
>>>> + */
>>>> +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
>>>> +{
>>>> +    uint64_t data = *ptr;
>>>> +    switch (len) {
>>>> +    case 1:
>>>> +        break;
>>>> +    case 2:
>>>> +        data = bswap16(data);
>>>> +        break;
>>>> +    case 4:
>>>> +        data = bswap32(data);
>>>> +        break;
>>>> +    case 8:
>>>> +        data = bswap64(data);
>>>> +        break;
>>>> +    default:
>>>> +        return -EINVAL;
>>>> +    }
>>>> +    *ptr = data;
>>>> +    return 0;
>>>> +}
>>
>> This is usually care taken by memory::adjust_endianness() ...
> 
> Yes, but that's not a memory region write.
> 
>>
>>> I was expecting more code to use a similar pattern, but it seems
>>> surprisingly uncommon.
>>
>> Which ring a bell for latent bug?
> 
> Looking at this, it seems there *is* a latent bug, which has not popped
> up so far as the pci instructions are not wired up in tcg yet. This
> code is only called from the kvm path...


The value in the register may be read from memory somehow but it may 
also be an immediate value, setup previously by another instruction.

AFAIU the TCG would have already make sure that the value read from 
memory has already been translated to big endian if read from a little 
endian memory region.
So that the value in register is always big endian.

OTOH the PCI memory is always little endian.

So AFAIU we always need to translate from BIG to little, no mater if KVM 
or TCG.

But I am not sure that I did understand right what the TCG does.

@Philippe, It does not seems to be the same problem as you encountered, 
AFAIU your problem was between memory and a LE device and our is between 
a BE register and a LE device.

Did I understood correctly what TCG does when emulating S390 ?


Pierre

> 
>>
>> This remind me of a similar issue on ppc:
>>
>> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05121.html
>> ...
>> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05666.html
> 
> 


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

Re: [Qemu-devel] [PATCH 7/7] s390x/pci: search for subregion inside the BARs

[Cornelia Huck]

On Tue,  7 Nov 2017 18:24:39 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> When dispatching memory access to PCI BAR region, we must
> look for eventual subregion, used by the PCI device to map

s/eventual subregion/possible subregions/ ?

> different memory areas inside the same PCI BAR.
> 
> Since the data offset we received is calculated starting at the
> region start address we need to adjust the offset for the subregion.
> 
> The data offset inside the subregion is calculated by substracting
> the subregion's starting address from the data offset in the region.
> 
> The function trap_msix() was used, despite not really useful since we
> removed the index from the msix message. With this patch we can
> definitively suppress this function.

Suggest rewording this (see below).

> 
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-pci-inst.c | 44 +++++++++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 97f62b5..b59ceef 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -344,12 +344,31 @@ static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
>      return 0;
>  }
>  
> +static MemoryRegion *s390_get_subregion(MemoryRegion *mr, uint64_t offset,
> +                                        uint8_t len)
> +{
> +    MemoryRegion *other;
> +    uint64_t subregion_size;
> +
> +    QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
> +        subregion_size = int128_get64(other->size);
> +        if ((offset >= other->addr) &&
> +            (offset + len) <= (other->addr + subregion_size)) {
> +            mr = other;
> +            break;
> +        }
> +    }
> +    return mr;
> +}
> +

So, this works for the msix stuff because it is a subregion?

This means you can remove trap_msix() because it is handled in a
generic way now, doesn't it?

>  static MemTxResult zpci_read_bar(S390PCIBusDevice *pbdev, uint8_t pcias,
>                                   uint64_t offset, uint64_t *data, uint8_t len)
>  {
>      MemoryRegion *mr;
>  
>      mr = pbdev->pdev->io_regions[pcias].memory;
> +    mr = s390_get_subregion(mr, offset, len);
> +    offset -= mr->addr;
>      return memory_region_dispatch_read(mr, offset, data, len,
>                                         MEMTXATTRS_UNSPECIFIED);
>  }
> @@ -443,30 +462,14 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
>      return 0;
>  }
>  
> -static int trap_msix(S390PCIBusDevice *pbdev, uint64_t offset, uint8_t pcias)
> -{
> -    if (pbdev->msix.available && pbdev->msix.table_bar == pcias &&
> -        offset >= pbdev->msix.table_offset &&
> -        offset < (pbdev->msix.table_offset +
> -                  pbdev->msix.entries * PCI_MSIX_ENTRY_SIZE)) {
> -        return 1;
> -    } else {
> -        return 0;
> -    }
> -}
> -
>  static MemTxResult zpci_write_bar(S390PCIBusDevice *pbdev, uint8_t pcias,
>                                    uint64_t offset, uint64_t data, uint8_t len)
>  {
>      MemoryRegion *mr;
>  
> -    if (trap_msix(pbdev, offset, pcias)) {
> -        offset = offset - pbdev->msix.table_offset;
> -        mr = &pbdev->pdev->msix_table_mmio;
> -    } else {
> -        mr = pbdev->pdev->io_regions[pcias].memory;
> -    }
> -
> +    mr = pbdev->pdev->io_regions[pcias].memory;
> +    mr = s390_get_subregion(mr, offset, len);
> +    offset -= mr->addr;
>      return memory_region_dispatch_write(mr, offset, data, len,
>                                          MEMTXATTRS_UNSPECIFIED);
>  }
> @@ -728,6 +731,9 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>      }
>  
>      mr = pbdev->pdev->io_regions[pcias].memory;
> +    mr = s390_get_subregion(mr, offset, len);
> +    offset -= mr->addr;
> +
>      if (!memory_region_access_valid(mr, offset, len, true)) {
>          program_interrupt(env, PGM_OPERAND, 6);
>          return 0;

Re: [Qemu-devel] [PATCH 1/7] s390x/pci: factor out endianess conversion

[Cornelia Huck]

On Mon, 13 Nov 2017 16:36:34 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> On 09/11/2017 20:20, Cornelia Huck wrote:
> > On Thu, 9 Nov 2017 15:55:46 -0300
> > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >   
> >> On 11/09/2017 01:38 PM, Cornelia Huck wrote:  
> >>> On Tue,  7 Nov 2017 18:24:33 +0100
> >>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> >>>      
> >>>> There are two places where the same endianness conversion
> >>>> is done.
> >>>> Let's factor this out into a static function.
> >>>>
> >>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> >>>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >>>> ---
> >>>>   hw/s390x/s390-pci-inst.c | 58 ++++++++++++++++++++++++++----------------------
> >>>>   1 file changed, 32 insertions(+), 26 deletions(-)
> >>>>
> >>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> >>>> index 8e088f3..8fcb02d 100644
> >>>> --- a/hw/s390x/s390-pci-inst.c
> >>>> +++ b/hw/s390x/s390-pci-inst.c
> >>>> @@ -314,6 +314,35 @@ out:
> >>>>       return 0;
> >>>>   }
> >>>>   
> >>>> +/**
> >>>> + * This function swaps the data at ptr according from one
> >>>> + * endianness to the other.
> >>>> + * valid data in the uint64_t data field.  
> >>>
> >>> I'm not sure what that line is supposed to mean?
> >>>      
> >>>> + * @ptr: a pointer to a uint64_t data field
> >>>> + * @len: the length of the valid data, must be 1,2,4 or 8
> >>>> + */
> >>>> +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
> >>>> +{
> >>>> +    uint64_t data = *ptr;
> >>>> +    switch (len) {
> >>>> +    case 1:
> >>>> +        break;
> >>>> +    case 2:
> >>>> +        data = bswap16(data);
> >>>> +        break;
> >>>> +    case 4:
> >>>> +        data = bswap32(data);
> >>>> +        break;
> >>>> +    case 8:
> >>>> +        data = bswap64(data);
> >>>> +        break;
> >>>> +    default:
> >>>> +        return -EINVAL;
> >>>> +    }
> >>>> +    *ptr = data;
> >>>> +    return 0;
> >>>> +}  
> >>
> >> This is usually care taken by memory::adjust_endianness() ...  
> > 
> > Yes, but that's not a memory region write.
> >   
> >>  
> >>> I was expecting more code to use a similar pattern, but it seems
> >>> surprisingly uncommon.  
> >>
> >> Which ring a bell for latent bug?  
> > 
> > Looking at this, it seems there *is* a latent bug, which has not popped
> > up so far as the pci instructions are not wired up in tcg yet. This
> > code is only called from the kvm path...  
> 
> 
> The value in the register may be read from memory somehow but it may 
> also be an immediate value, setup previously by another instruction.
> 
> AFAIU the TCG would have already make sure that the value read from 
> memory has already been translated to big endian if read from a little 
> endian memory region.
> So that the value in register is always big endian.
> 
> OTOH the PCI memory is always little endian.
> 
> So AFAIU we always need to translate from BIG to little, no mater if KVM 
> or TCG.
> 
> But I am not sure that I did understand right what the TCG does.
> 
> @Philippe, It does not seems to be the same problem as you encountered, 
> AFAIU your problem was between memory and a LE device and our is between 
> a BE register and a LE device.
> 
> Did I understood correctly what TCG does when emulating S390 ?

So, if this function is supposed to work on a known-BE value, I think
this should be fine. But a comment in the function description would be
good...

> 
> 
> Pierre
> 
> >   
> >>
> >> This remind me of a similar issue on ppc:
> >>
> >> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05121.html
> >> ...
> >> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05666.html  
> > 
> >   
> 
> 

Re: [Qemu-devel] [PATCH 4/7] s390x/pci: rework PCI STORE BLOCK

[Pierre Morel]

On 13/11/2017 16:23, Cornelia Huck wrote:
> On Tue,  7 Nov 2017 18:24:36 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> Enhance the fault detection.
>>
>> Add the maxstbl entry to both the Query PCI Function Group
>> response and the PCIBusDevice structure.
>>
>> Initialize the maxstbl to 128 per default until we get
>> the actual data from the hardware.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>> ---
>>   hw/s390x/s390-pci-bus.h  |  1 +
>>   hw/s390x/s390-pci-inst.c | 62 +++++++++++++++++++++++++++++-------------------
>>   hw/s390x/s390-pci-inst.h |  2 +-
>>   3 files changed, 40 insertions(+), 25 deletions(-)
>>
> 
>> @@ -662,22 +664,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>       fh = env->regs[r1] >> 32;
>>       pcias = (env->regs[r1] >> 16) & 0xf;
>>       len = env->regs[r1] & 0xff;
>> +    offset = env->regs[r3];
>>   
>> -    if (pcias > 5) {
>> -        DPRINTF("pcistb invalid space\n");
>> -        setcc(cpu, ZPCI_PCI_LS_ERR);
>> -        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
>> -        return 0;
>> -    }
>> -
>> -    switch (len) {
>> -    case 16:
>> -    case 32:
>> -    case 64:
>> -    case 128:
>> -        break;
>> -    default:
>> -        program_interrupt(env, PGM_SPECIFICATION, 6);
>> +    if (!(fh & FH_MASK_ENABLE)) {
>> +        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
> 
> So this means you move checking for the device before checking for the
> parameters or the as.

Yes, this is clearly following the specifications that CC=3 has priority 
over CC=1 or CC=2.

By the way I find that defining ZPCI_PCI_LS_INVAL_HANDLE is obfuscating, 
we have the information from the test we just made but we loose the 
information about if it is a 1, 2 or 3 CC value.
May be in another patch?

> 
>>           return 0;
>>       }
>>   
>> @@ -689,12 +679,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>       }
>>   
>>       switch (pbdev->state) {
>> -    case ZPCI_FS_RESERVED:
>> -    case ZPCI_FS_STANDBY:
>> -    case ZPCI_FS_DISABLED:
>>       case ZPCI_FS_PERMANENT_ERROR:
>> -        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
>> -        return 0;
>>       case ZPCI_FS_ERROR:
>>           setcc(cpu, ZPCI_PCI_LS_ERR);
>>           s390_set_status_code(env, r1, ZPCI_PCI_ST_BLOCKED);
>> @@ -703,8 +688,33 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>           break;
>>       }
>>   
>> +    if (pcias > 5) {
>> +        DPRINTF("pcistb invalid space\n");
>> +        setcc(cpu, ZPCI_PCI_LS_ERR);
>> +        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
>> +        return 0;
>> +    }
>> +
>> +    /* Verify the address, offset and length */
>> +    /* offset must be a multiple of 8 */
>> +    if (offset % 8) {
>> +        goto addressing_error;
>> +    }
>> +    /* Length must be greater than 8, a multiple of 8, lower than maxstbl */
>> +    if ((len <= 8) || (len % 8) || (len > pbdev->maxstbl)) {
>> +        goto addressing_error;
>> +    }
>> +    /* Do not cross a 4K-byte boundary */
>> +    if (((offset & 0xfff) + len) > 0x1000) {
>> +        goto addressing_error;
>> +    }
>> +    /* Guest address must be double word aligned */
>> +    if (gaddr & 0x07UL) {
>> +        goto addressing_error;
>> +    }
> 
> So the checks here are only evaluated if the instruction actually pokes
> at a valid region?

hum, I did not find the precedence of execution for PCI STORE BLOCK.

My logic is that you must find a destination before you start reading 
the source, so I would say it is the right way to do.
But the experience as already shown that my logic may not always be 
compatible with the internals of S390x :)

However, the documentation specifies that if an error condition is 
detected that precludes the *initiation* of the store operation a CC=1 
is set.
The fact that the *initiation* is focused on enforce the idea I have on 
the precedence between the low level operations.

> 
>> +
>>       mr = pbdev->pdev->io_regions[pcias].memory;
>> -    if (!memory_region_access_valid(mr, env->regs[r3], len, true)) {
>> +    if (!memory_region_access_valid(mr, offset, len, true)) {
>>           program_interrupt(env, PGM_OPERAND, 6);
>>           return 0;
>>       }
>> @@ -714,9 +724,9 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>       }
>>   
>>       for (i = 0; i < len / 8; i++) {
>> -        result = memory_region_dispatch_write(mr, env->regs[r3] + i * 8,
>> -                                     ldq_p(buffer + i * 8), 8,
>> -                                     MEMTXATTRS_UNSPECIFIED);
>> +        result = memory_region_dispatch_write(mr, offset + i * 8,
>> +                                              ldq_p(buffer + i * 8), 8,
>> +                                              MEMTXATTRS_UNSPECIFIED);
>>           if (result != MEMTX_OK) {
>>               program_interrupt(env, PGM_OPERAND, 6);
>>               return 0;
>> @@ -725,6 +735,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>   
>>       setcc(cpu, ZPCI_PCI_LS_OK);
>>       return 0;
>> +
>> +addressing_error:
>> +    program_interrupt(env, PGM_SPECIFICATION, 6);
>> +    return 0;
>>   }
> 
> This seems more readable; I can't verify whether it is actually correct
> without access to the architecture, though :(
> 



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

Re: [Qemu-devel] [PATCH 1/7] s390x/pci: factor out endianess conversion

[Pierre Morel]

On 13/11/2017 17:38, Cornelia Huck wrote:
> On Mon, 13 Nov 2017 16:36:34 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> On 09/11/2017 20:20, Cornelia Huck wrote:
>>> On Thu, 9 Nov 2017 15:55:46 -0300
>>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>    
>>>> On 11/09/2017 01:38 PM, Cornelia Huck wrote:
>>>>> On Tue,  7 Nov 2017 18:24:33 +0100
>>>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>>>>       
>>>>>> There are two places where the same endianness conversion
>>>>>> is done.
>>>>>> Let's factor this out into a static function.
>>>>>>
>>>>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>>>>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>    hw/s390x/s390-pci-inst.c | 58 ++++++++++++++++++++++++++----------------------
>>>>>>    1 file changed, 32 insertions(+), 26 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>>>>>> index 8e088f3..8fcb02d 100644
>>>>>> --- a/hw/s390x/s390-pci-inst.c
>>>>>> +++ b/hw/s390x/s390-pci-inst.c
>>>>>> @@ -314,6 +314,35 @@ out:
>>>>>>        return 0;
>>>>>>    }
>>>>>>    
>>>>>> +/**
>>>>>> + * This function swaps the data at ptr according from one
>>>>>> + * endianness to the other.
>>>>>> + * valid data in the uint64_t data field.
>>>>>
>>>>> I'm not sure what that line is supposed to mean?
>>>>>       
>>>>>> + * @ptr: a pointer to a uint64_t data field
>>>>>> + * @len: the length of the valid data, must be 1,2,4 or 8
>>>>>> + */
>>>>>> +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
>>>>>> +{
>>>>>> +    uint64_t data = *ptr;
>>>>>> +    switch (len) {
>>>>>> +    case 1:
>>>>>> +        break;
>>>>>> +    case 2:
>>>>>> +        data = bswap16(data);
>>>>>> +        break;
>>>>>> +    case 4:
>>>>>> +        data = bswap32(data);
>>>>>> +        break;
>>>>>> +    case 8:
>>>>>> +        data = bswap64(data);
>>>>>> +        break;
>>>>>> +    default:
>>>>>> +        return -EINVAL;
>>>>>> +    }
>>>>>> +    *ptr = data;
>>>>>> +    return 0;
>>>>>> +}
>>>>
>>>> This is usually care taken by memory::adjust_endianness() ...
>>>
>>> Yes, but that's not a memory region write.
>>>    
>>>>   
>>>>> I was expecting more code to use a similar pattern, but it seems
>>>>> surprisingly uncommon.
>>>>
>>>> Which ring a bell for latent bug?
>>>
>>> Looking at this, it seems there *is* a latent bug, which has not popped
>>> up so far as the pci instructions are not wired up in tcg yet. This
>>> code is only called from the kvm path...
>>
>>
>> The value in the register may be read from memory somehow but it may
>> also be an immediate value, setup previously by another instruction.
>>
>> AFAIU the TCG would have already make sure that the value read from
>> memory has already been translated to big endian if read from a little
>> endian memory region.
>> So that the value in register is always big endian.
>>
>> OTOH the PCI memory is always little endian.
>>
>> So AFAIU we always need to translate from BIG to little, no mater if KVM
>> or TCG.
>>
>> But I am not sure that I did understand right what the TCG does.
>>
>> @Philippe, It does not seems to be the same problem as you encountered,
>> AFAIU your problem was between memory and a LE device and our is between
>> a BE register and a LE device.
>>
>> Did I understood correctly what TCG does when emulating S390 ?
> 
> So, if this function is supposed to work on a known-BE value, I think
> this should be fine. But a comment in the function description would be
> good...

Yes, I will do.
Thanks,

Pierre

> 
>>
>>
>> Pierre
>>
>>>    
>>>>
>>>> This remind me of a similar issue on ppc:
>>>>
>>>> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05121.html
>>>> ...
>>>> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05666.html
>>>
>>>    
>>
>>
> 
> 


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

Re: [Qemu-devel] [PATCH 4/7] s390x/pci: rework PCI STORE BLOCK

[Cornelia Huck]

On Mon, 13 Nov 2017 17:38:40 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> On 13/11/2017 16:23, Cornelia Huck wrote:
> > On Tue,  7 Nov 2017 18:24:36 +0100
> > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> >   
> >> Enhance the fault detection.
> >>
> >> Add the maxstbl entry to both the Query PCI Function Group
> >> response and the PCIBusDevice structure.
> >>
> >> Initialize the maxstbl to 128 per default until we get
> >> the actual data from the hardware.
> >>
> >> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> >> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >> ---
> >>   hw/s390x/s390-pci-bus.h  |  1 +
> >>   hw/s390x/s390-pci-inst.c | 62 +++++++++++++++++++++++++++++-------------------
> >>   hw/s390x/s390-pci-inst.h |  2 +-
> >>   3 files changed, 40 insertions(+), 25 deletions(-)
> >>  
> >   
> >> @@ -662,22 +664,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
> >>       fh = env->regs[r1] >> 32;
> >>       pcias = (env->regs[r1] >> 16) & 0xf;
> >>       len = env->regs[r1] & 0xff;
> >> +    offset = env->regs[r3];
> >>   
> >> -    if (pcias > 5) {
> >> -        DPRINTF("pcistb invalid space\n");
> >> -        setcc(cpu, ZPCI_PCI_LS_ERR);
> >> -        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
> >> -        return 0;
> >> -    }
> >> -
> >> -    switch (len) {
> >> -    case 16:
> >> -    case 32:
> >> -    case 64:
> >> -    case 128:
> >> -        break;
> >> -    default:
> >> -        program_interrupt(env, PGM_SPECIFICATION, 6);
> >> +    if (!(fh & FH_MASK_ENABLE)) {
> >> +        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);  
> > 
> > So this means you move checking for the device before checking for the
> > parameters or the as.  
> 
> Yes, this is clearly following the specifications that CC=3 has priority 
> over CC=1 or CC=2.

OK, it would then make sense to mention in the patch description that
you fixed up the precedence as well.

> 
> By the way I find that defining ZPCI_PCI_LS_INVAL_HANDLE is obfuscating, 
> we have the information from the test we just made but we loose the 
> information about if it is a 1, 2 or 3 CC value.
> May be in another patch?

Let's keep this for now, we can revisit that later.

> 
> >   
> >>           return 0;
> >>       }
> >>   
> >> @@ -689,12 +679,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
> >>       }
> >>   
> >>       switch (pbdev->state) {
> >> -    case ZPCI_FS_RESERVED:
> >> -    case ZPCI_FS_STANDBY:
> >> -    case ZPCI_FS_DISABLED:
> >>       case ZPCI_FS_PERMANENT_ERROR:
> >> -        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
> >> -        return 0;
> >>       case ZPCI_FS_ERROR:
> >>           setcc(cpu, ZPCI_PCI_LS_ERR);
> >>           s390_set_status_code(env, r1, ZPCI_PCI_ST_BLOCKED);
> >> @@ -703,8 +688,33 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
> >>           break;
> >>       }
> >>   
> >> +    if (pcias > 5) {
> >> +        DPRINTF("pcistb invalid space\n");
> >> +        setcc(cpu, ZPCI_PCI_LS_ERR);
> >> +        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
> >> +        return 0;
> >> +    }
> >> +
> >> +    /* Verify the address, offset and length */
> >> +    /* offset must be a multiple of 8 */
> >> +    if (offset % 8) {
> >> +        goto addressing_error;
> >> +    }
> >> +    /* Length must be greater than 8, a multiple of 8, lower than maxstbl */
> >> +    if ((len <= 8) || (len % 8) || (len > pbdev->maxstbl)) {
> >> +        goto addressing_error;
> >> +    }
> >> +    /* Do not cross a 4K-byte boundary */
> >> +    if (((offset & 0xfff) + len) > 0x1000) {
> >> +        goto addressing_error;
> >> +    }
> >> +    /* Guest address must be double word aligned */
> >> +    if (gaddr & 0x07UL) {
> >> +        goto addressing_error;
> >> +    }  
> > 
> > So the checks here are only evaluated if the instruction actually pokes
> > at a valid region?  
> 
> hum, I did not find the precedence of execution for PCI STORE BLOCK.
> 
> My logic is that you must find a destination before you start reading 
> the source, so I would say it is the right way to do.
> But the experience as already shown that my logic may not always be 
> compatible with the internals of S390x :)
> 
> However, the documentation specifies that if an error condition is 
> detected that precludes the *initiation* of the store operation a CC=1 
> is set.
> The fact that the *initiation* is focused on enforce the idea I have on 
> the precedence between the low level operations.

OK, this interpretation makes sense. It might be a good idea to poke the
architecture authors if it is ambiguous, though :)

> 
> >   
> >> +
> >>       mr = pbdev->pdev->io_regions[pcias].memory;
> >> -    if (!memory_region_access_valid(mr, env->regs[r3], len, true)) {
> >> +    if (!memory_region_access_valid(mr, offset, len, true)) {
> >>           program_interrupt(env, PGM_OPERAND, 6);
> >>           return 0;
> >>       }
> >> @@ -714,9 +724,9 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
> >>       }
> >>   
> >>       for (i = 0; i < len / 8; i++) {
> >> -        result = memory_region_dispatch_write(mr, env->regs[r3] + i * 8,
> >> -                                     ldq_p(buffer + i * 8), 8,
> >> -                                     MEMTXATTRS_UNSPECIFIED);
> >> +        result = memory_region_dispatch_write(mr, offset + i * 8,
> >> +                                              ldq_p(buffer + i * 8), 8,
> >> +                                              MEMTXATTRS_UNSPECIFIED);
> >>           if (result != MEMTX_OK) {
> >>               program_interrupt(env, PGM_OPERAND, 6);
> >>               return 0;
> >> @@ -725,6 +735,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
> >>   
> >>       setcc(cpu, ZPCI_PCI_LS_OK);
> >>       return 0;
> >> +
> >> +addressing_error:
> >> +    program_interrupt(env, PGM_SPECIFICATION, 6);
> >> +    return 0;
> >>   }  
> > 
> > This seems more readable; I can't verify whether it is actually correct
> > without access to the architecture, though :(
> >   
> 
> 
> 

Re: [Qemu-devel] [PATCH 0/7] s390x/pci: Improve zPCI to cover more cases

[Cornelia Huck]

On Tue,  7 Nov 2017 18:24:32 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> Right now the PCI support is very limited (e.g. pass through of a
> host vfio device)
> To enable features like virtio-pci several modifications needs to be
> done.
> 
> Virtio-PCI uses subregions, which may eventually be discontinuous
> inside bars instead of a single flat region.
> The address offset being formerly calculated from the BAR base address
> must be adapted to the subregions instead of to the single region.
> 
> This patch provides the new calculation for the three kind of BAR
> access, zPCI STORE, zPCI LOAD and zPCI STORE BLOCK.
> 
> We use the opportunity to
>  - enhance the fault detection for zPCI STORE and LOAD,
>  - enhance the fault detection and to provide the maximum STORE BLOCK
>    block size, maxstbl, for zPCI STORE BLOCK
>  - factor out part of the code used to calculate the offset and
>    access the BARs,
>  - factor out the code for endianess conversion.
> 
> 
> Pierre Morel (7):
>   s390x/pci: factor out endianess conversion
>   s390x/pci: rework PCI STORE
>   s390x/pci: rework PCI LOAD
>   s390x/pci: rework PCI STORE BLOCK
>   s390x/pci: move the memory region read from pcilg
>   s390x/pci: move the memory region write from pcistg
>   s390x/pci: search for subregion inside the BARs
> 
>  hw/s390x/s390-pci-bus.h  |   1 +
>  hw/s390x/s390-pci-inst.c | 250 ++++++++++++++++++++++++++++-------------------
>  hw/s390x/s390-pci-inst.h |   2 +-
>  3 files changed, 153 insertions(+), 100 deletions(-)
> 

I assume you'll send a v2?

I'll see if I can find some time to wire up pci in tcg (as this would
get us additional test coverage, especially regarding endianness), but
I won't complain should someone beat me to it.

Re: [Qemu-devel] [PATCH 0/7] s390x/pci: Improve zPCI to cover more cases

[Pierre Morel]

On 13/11/2017 18:13, Cornelia Huck wrote:
> On Tue,  7 Nov 2017 18:24:32 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> Right now the PCI support is very limited (e.g. pass through of a
>> host vfio device)
>> To enable features like virtio-pci several modifications needs to be
>> done.
>>
>> Virtio-PCI uses subregions, which may eventually be discontinuous
>> inside bars instead of a single flat region.
>> The address offset being formerly calculated from the BAR base address
>> must be adapted to the subregions instead of to the single region.
>>
>> This patch provides the new calculation for the three kind of BAR
>> access, zPCI STORE, zPCI LOAD and zPCI STORE BLOCK.
>>
>> We use the opportunity to
>>   - enhance the fault detection for zPCI STORE and LOAD,
>>   - enhance the fault detection and to provide the maximum STORE BLOCK
>>     block size, maxstbl, for zPCI STORE BLOCK
>>   - factor out part of the code used to calculate the offset and
>>     access the BARs,
>>   - factor out the code for endianess conversion.
>>
>>
>> Pierre Morel (7):
>>    s390x/pci: factor out endianess conversion
>>    s390x/pci: rework PCI STORE
>>    s390x/pci: rework PCI LOAD
>>    s390x/pci: rework PCI STORE BLOCK
>>    s390x/pci: move the memory region read from pcilg
>>    s390x/pci: move the memory region write from pcistg
>>    s390x/pci: search for subregion inside the BARs
>>
>>   hw/s390x/s390-pci-bus.h  |   1 +
>>   hw/s390x/s390-pci-inst.c | 250 ++++++++++++++++++++++++++++-------------------
>>   hw/s390x/s390-pci-inst.h |   2 +-
>>   3 files changed, 153 insertions(+), 100 deletions(-)
>>
> 
> I assume you'll send a v2?

Yes, I have a lot of mater to do this now.

Thanks for reviewing,

Pierre

> 
> I'll see if I can find some time to wire up pci in tcg (as this would
> get us additional test coverage, especially regarding endianness), but
> I won't complain should someone beat me to it.
> 
Yes, would be fine to have it to increase test coverage.


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

Re: [Qemu-devel] [PATCH 4/7] s390x/pci: rework PCI STORE BLOCK

[Pierre Morel]

On 13/11/2017 18:10, Cornelia Huck wrote:
> On Mon, 13 Nov 2017 17:38:40 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> On 13/11/2017 16:23, Cornelia Huck wrote:
>>> On Tue,  7 Nov 2017 18:24:36 +0100
>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>>    
>>>> Enhance the fault detection.
>>>>
>>>> Add the maxstbl entry to both the Query PCI Function Group
>>>> response and the PCIBusDevice structure.
>>>>
>>>> Initialize the maxstbl to 128 per default until we get
>>>> the actual data from the hardware.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>> ---
>>>>    hw/s390x/s390-pci-bus.h  |  1 +
>>>>    hw/s390x/s390-pci-inst.c | 62 +++++++++++++++++++++++++++++-------------------
>>>>    hw/s390x/s390-pci-inst.h |  2 +-
>>>>    3 files changed, 40 insertions(+), 25 deletions(-)
>>>>   
>>>    
>>>> @@ -662,22 +664,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>>>        fh = env->regs[r1] >> 32;
>>>>        pcias = (env->regs[r1] >> 16) & 0xf;
>>>>        len = env->regs[r1] & 0xff;
>>>> +    offset = env->regs[r3];
>>>>    
>>>> -    if (pcias > 5) {
>>>> -        DPRINTF("pcistb invalid space\n");
>>>> -        setcc(cpu, ZPCI_PCI_LS_ERR);
>>>> -        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
>>>> -        return 0;
>>>> -    }
>>>> -
>>>> -    switch (len) {
>>>> -    case 16:
>>>> -    case 32:
>>>> -    case 64:
>>>> -    case 128:
>>>> -        break;
>>>> -    default:
>>>> -        program_interrupt(env, PGM_SPECIFICATION, 6);
>>>> +    if (!(fh & FH_MASK_ENABLE)) {
>>>> +        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
>>>
>>> So this means you move checking for the device before checking for the
>>> parameters or the as.
>>
>> Yes, this is clearly following the specifications that CC=3 has priority
>> over CC=1 or CC=2.
> 
> OK, it would then make sense to mention in the patch description that
> you fixed up the precedence as well.

I will do
thanks

> 
>>
>> By the way I find that defining ZPCI_PCI_LS_INVAL_HANDLE is obfuscating,
>> we have the information from the test we just made but we loose the
>> information about if it is a 1, 2 or 3 CC value.
>> May be in another patch?
> 
> Let's keep this for now, we can revisit that later.

OK

> 
>>
>>>    
>>>>            return 0;
>>>>        }
>>>>    
>>>> @@ -689,12 +679,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>>>        }
>>>>    
>>>>        switch (pbdev->state) {
>>>> -    case ZPCI_FS_RESERVED:
>>>> -    case ZPCI_FS_STANDBY:
>>>> -    case ZPCI_FS_DISABLED:
>>>>        case ZPCI_FS_PERMANENT_ERROR:
>>>> -        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
>>>> -        return 0;
>>>>        case ZPCI_FS_ERROR:
>>>>            setcc(cpu, ZPCI_PCI_LS_ERR);
>>>>            s390_set_status_code(env, r1, ZPCI_PCI_ST_BLOCKED);
>>>> @@ -703,8 +688,33 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>>>            break;
>>>>        }
>>>>    
>>>> +    if (pcias > 5) {
>>>> +        DPRINTF("pcistb invalid space\n");
>>>> +        setcc(cpu, ZPCI_PCI_LS_ERR);
>>>> +        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    /* Verify the address, offset and length */
>>>> +    /* offset must be a multiple of 8 */
>>>> +    if (offset % 8) {
>>>> +        goto addressing_error;
>>>> +    }
>>>> +    /* Length must be greater than 8, a multiple of 8, lower than maxstbl */
>>>> +    if ((len <= 8) || (len % 8) || (len > pbdev->maxstbl)) {
>>>> +        goto addressing_error;
>>>> +    }
>>>> +    /* Do not cross a 4K-byte boundary */
>>>> +    if (((offset & 0xfff) + len) > 0x1000) {
>>>> +        goto addressing_error;
>>>> +    }
>>>> +    /* Guest address must be double word aligned */
>>>> +    if (gaddr & 0x07UL) {
>>>> +        goto addressing_error;
>>>> +    }
>>>
>>> So the checks here are only evaluated if the instruction actually pokes
>>> at a valid region?
>>
>> hum, I did not find the precedence of execution for PCI STORE BLOCK.
>>
>> My logic is that you must find a destination before you start reading
>> the source, so I would say it is the right way to do.
>> But the experience as already shown that my logic may not always be
>> compatible with the internals of S390x :)
>>
>> However, the documentation specifies that if an error condition is
>> detected that precludes the *initiation* of the store operation a CC=1
>> is set.
>> The fact that the *initiation* is focused on enforce the idea I have on
>> the precedence between the low level operations.
> 
> OK, this interpretation makes sense. It might be a good idea to poke the
> architecture authors if it is ambiguous, though :)

Yes.

> 
>>
>>>    
>>>> +
>>>>        mr = pbdev->pdev->io_regions[pcias].memory;
>>>> -    if (!memory_region_access_valid(mr, env->regs[r3], len, true)) {
>>>> +    if (!memory_region_access_valid(mr, offset, len, true)) {
>>>>            program_interrupt(env, PGM_OPERAND, 6);
>>>>            return 0;
>>>>        }
>>>> @@ -714,9 +724,9 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>>>        }
>>>>    
>>>>        for (i = 0; i < len / 8; i++) {
>>>> -        result = memory_region_dispatch_write(mr, env->regs[r3] + i * 8,
>>>> -                                     ldq_p(buffer + i * 8), 8,
>>>> -                                     MEMTXATTRS_UNSPECIFIED);
>>>> +        result = memory_region_dispatch_write(mr, offset + i * 8,
>>>> +                                              ldq_p(buffer + i * 8), 8,
>>>> +                                              MEMTXATTRS_UNSPECIFIED);
>>>>            if (result != MEMTX_OK) {
>>>>                program_interrupt(env, PGM_OPERAND, 6);
>>>>                return 0;
>>>> @@ -725,6 +735,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>>>    
>>>>        setcc(cpu, ZPCI_PCI_LS_OK);
>>>>        return 0;
>>>> +
>>>> +addressing_error:
>>>> +    program_interrupt(env, PGM_SPECIFICATION, 6);
>>>> +    return 0;
>>>>    }
>>>
>>> This seems more readable; I can't verify whether it is actually correct
>>> without access to the architecture, though :(
>>>    
>>
>>
>>
> 
> 


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