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

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

[Pierre Morel]

This patch fixes the following BUG:
Even a guest is able to detect virtio_pci device, the init function
the Linux virtio_pci driver will hang because zPCI does not support
the subregions used by virtio_pci.

It follows that 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.

As already stated above, Virtio-PCI uses subregions, which may eventually
be discontinuous inside bars instead of a single flat region often used
by real devices.
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 done by zPCI.

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 | 247 ++++++++++++++++++++++++++++-------------------
 hw/s390x/s390-pci-inst.h |   2 +-
 3 files changed, 150 insertions(+), 100 deletions(-)

-- 
2.7.4

Changelog:

- suppress fallthrough to PCI_ROM_SLOT to handle it in the default case.
- reword most of the patch commit messages
- add comments to the endianness conversion
- reword somme comments inside the patches

[Qemu-devel] [PATCH v2 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>
Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 hw/s390x/s390-pci-inst.c | 59 +++++++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 8e088f3..ded1556 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -314,6 +314,36 @@ out:
     return 0;
 }
 
+/**
+ * This function is called when endianess is fixed, whatever the host endianess
+ * is, like in our case from s390x BIG endian registers to little endian PCI
+ * Bars, to translate the uint64_t pointed from one endianess to the other.
+ *
+ * @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 +415,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 +518,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 v2 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>
Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 hw/s390x/s390-pci-inst.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index ded1556..df0c8f0 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -470,6 +470,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) {
@@ -479,12 +485,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);
@@ -493,9 +494,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 valid and do the appropriates 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;
         }
@@ -513,21 +518,21 @@ 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 lengths 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;
+    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 v2 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>
Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
 hw/s390x/s390-pci-inst.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index df0c8f0..cbc6421 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -373,6 +373,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");
@@ -381,12 +386,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);
@@ -395,8 +395,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;
         }
@@ -407,8 +408,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;
         }
@@ -419,8 +421,9 @@ 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;
+    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 v2 4/7] s390x/pci: rework PCI STORE BLOCK

[Pierre Morel]

Enhance the fault detection.

Fixup the precedence to check the destination path existance
before checking for the source accessibility.

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 cbc6421..83b68a9 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);
@@ -645,6 +646,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;
@@ -659,22 +661,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;
     }
 
@@ -686,12 +676,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);
@@ -700,8 +685,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, not greater 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;
     }
@@ -711,9 +721,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;
@@ -722,6 +732,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 v2 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 83b68a9..d46d4cd 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -345,13 +345,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;
@@ -402,9 +411,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 v2 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 d46d4cd..eef3b89 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -454,12 +454,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;
@@ -516,15 +531,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 v2 7/7] s390x/pci: search for subregion inside the BARs

[Pierre Morel]

When dispatching memory access to PCI BAR region, we must
look for possible subregions, 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 access to the MSIX region is now handled in a generic way,
we do not need the specific trap_msix() function anymore.

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 eef3b89..33d08c6 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -345,12 +345,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);
 }
@@ -442,30 +461,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);
 }
@@ -725,6 +728,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 v2 1/7] s390x/pci: factor out endianess conversion

[Cornelia Huck]

On Thu, 16 Nov 2017 18:51:49 +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>
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>

Your two s-o-bs look a bit odd...

> ---
>  hw/s390x/s390-pci-inst.c | 59 +++++++++++++++++++++++++++---------------------
>  1 file changed, 33 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 8e088f3..ded1556 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -314,6 +314,36 @@ out:
>      return 0;
>  }
>  
> +/**
> + * This function is called when endianess is fixed, whatever the host endianess
> + * is, like in our case from s390x BIG endian registers to little endian PCI
> + * Bars, to translate the uint64_t pointed from one endianess to the other.

I would rephrase this a bit:

"Swap data contained in s390x big endian registers to little endian PCI
bars."

That makes it clear that this function is for a specialized use case.

> + *
> + * @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;

Please add an extra empty line.

> +    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;

Other than the nits above, looks good.

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

[Cornelia Huck]

On Thu, 16 Nov 2017 18:51:50 +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>
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>

Same double s-o-b.

> ---
>  hw/s390x/s390-pci-inst.c | 39 ++++++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index ded1556..df0c8f0 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -470,6 +470,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) {
> @@ -479,12 +485,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);
> @@ -493,9 +494,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 valid and do the appropriates operations */
> +    switch (pcias) {
> +    case 0 ... 5:

Make this
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;
>          }
> @@ -513,21 +518,21 @@ 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 lengths 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;
> +    default:
>          DPRINTF("pcistg invalid space\n");
>          setcc(cpu, ZPCI_PCI_LS_ERR);
>          s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);

Other than the nits, looks good.

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

[Cornelia Huck]

On Thu, 16 Nov 2017 18:51:51 +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>
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>

Same as last patch...

> ---
>  hw/s390x/s390-pci-inst.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index df0c8f0..cbc6421 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -373,6 +373,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");
> @@ -381,12 +386,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);
> @@ -395,8 +395,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:

...and here...

> +        if (!len || (len > (8 - (offset & 0x7)))) {
>              program_interrupt(env, PGM_OPERAND, 4);
>              return 0;
>          }
> @@ -407,8 +408,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;
>          }
> @@ -419,8 +421,9 @@ 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;
> +    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;

...and again here :) (good other than the nits)

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

[Cornelia Huck]

On Thu, 16 Nov 2017 18:51:52 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> Enhance the fault detection.
> 
> Fixup the precedence to check the destination path existance
> before checking for the source accessibility.
> 
> 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(-)

> @@ -700,8 +685,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, not greater maxstbl */

"not greater than maxstlb"

> +    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;
>      }

Looks good.

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

[Cornelia Huck]

On Thu, 16 Nov 2017 18:51:53 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> 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(-)

Looks good.

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

[Cornelia Huck]

On Thu, 16 Nov 2017 18:51:54 +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.
> 
> 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(-)

Looks good.

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

[Cornelia Huck]

On Thu, 16 Nov 2017 18:51:55 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> When dispatching memory access to PCI BAR region, we must
> look for possible subregions, 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 access to the MSIX region is now handled in a generic way,
> we do not need the specific trap_msix() function anymore.
> 
> 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(-)

This one also looks fine.

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

[Cornelia Huck]

On Thu, 16 Nov 2017 18:51:48 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> This patch fixes the following BUG:
> Even a guest is able to detect virtio_pci device, the init function
> the Linux virtio_pci driver will hang because zPCI does not support
> the subregions used by virtio_pci.
> 
> It follows that 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.
> 
> As already stated above, Virtio-PCI uses subregions, which may eventually
> be discontinuous inside bars instead of a single flat region often used
> by real devices.
> 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 done by zPCI.
> 
> 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.

I'll play with this a bit, but I think this is good for s390-next.

[I'd still like to play with this under tcg as well to make sure we are
endian-clean, but I'm making slower progress there than I hoped.]

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

[Cornelia Huck]

On Tue, 21 Nov 2017 11:38:45 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Thu, 16 Nov 2017 18:51:50 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> > @@ -493,9 +494,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 valid and do the appropriates operations */
> > +    switch (pcias) {
> > +    case 0 ... 5:  
> 
> Make this
> case 0...5:
> ?

...only that it confuses the compiler when using numbers. We can either
keep the slightly ugly version, or introduce #defines.
ZPCI_PCIAS_IOREGION_{MIN,MAX} (and maybe ZPCI_PCIAS_CONFIG for 15)?

> 
> > +        /* 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;
> >          }
> > @@ -513,21 +518,21 @@ 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 lengths 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;
> > +    default:
> >          DPRINTF("pcistg invalid space\n");
> >          setcc(cpu, ZPCI_PCI_LS_ERR);
> >          s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);  
> 
> Other than the nits, looks good.

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

[Pierre Morel]

On 21/11/2017 11:35, Cornelia Huck wrote:
> On Thu, 16 Nov 2017 18:51:49 +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>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> 
> Your two s-o-bs look a bit odd...
> 
>> ---
>>   hw/s390x/s390-pci-inst.c | 59 +++++++++++++++++++++++++++---------------------
>>   1 file changed, 33 insertions(+), 26 deletions(-)
>>
>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>> index 8e088f3..ded1556 100644
>> --- a/hw/s390x/s390-pci-inst.c
>> +++ b/hw/s390x/s390-pci-inst.c
>> @@ -314,6 +314,36 @@ out:
>>       return 0;
>>   }
>>   
>> +/**
>> + * This function is called when endianess is fixed, whatever the host endianess
>> + * is, like in our case from s390x BIG endian registers to little endian PCI
>> + * Bars, to translate the uint64_t pointed from one endianess to the other.
> 
> I would rephrase this a bit:
> 
> "Swap data contained in s390x big endian registers to little endian PCI
> bars."

Simpler and clearer. thanks.

> 
> That makes it clear that this function is for a specialized use case.
> 
>> + *
>> + * @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;
> 
> Please add an extra empty line.

yes.

> 
>> +    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;
> 
> Other than the nits above, looks good.
> 

Thanks for the review

Pierre

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

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

[Pierre Morel]

On 21/11/2017 11:38, Cornelia Huck wrote:
> On Thu, 16 Nov 2017 18:51:50 +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>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> 
> Same double s-o-b.

removed.
Thanks

> 
>> ---
>>   hw/s390x/s390-pci-inst.c | 39 ++++++++++++++++++++++-----------------
>>   1 file changed, 22 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>> index ded1556..df0c8f0 100644
>> --- a/hw/s390x/s390-pci-inst.c
>> +++ b/hw/s390x/s390-pci-inst.c
>> @@ -470,6 +470,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) {
>> @@ -479,12 +485,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);
>> @@ -493,9 +494,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 valid and do the appropriates operations */
>> +    switch (pcias) {
>> +    case 0 ... 5:
> 
> Make this
> case 0...5:
> ?

Compiler complains.
a blanc around the three points seems right.


> 
>> +        /* 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;
>>           }
>> @@ -513,21 +518,21 @@ 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 lengths 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;
>> +    default:
>>           DPRINTF("pcistg invalid space\n");
>>           setcc(cpu, ZPCI_PCI_LS_ERR);
>>           s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);
> 
> Other than the nits, looks good.
> 


Thanks for reviewing, I remove the double signature

Regards,

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

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

[Pierre Morel]

On 21/11/2017 15:25, Cornelia Huck wrote:
> On Tue, 21 Nov 2017 11:38:45 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Thu, 16 Nov 2017 18:51:50 +0100
>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>>> @@ -493,9 +494,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 valid and do the appropriates operations */
>>> +    switch (pcias) {
>>> +    case 0 ... 5:
>>
>> Make this
>> case 0...5:
>> ?
> 
> ...only that it confuses the compiler when using numbers. We can either

I did not see this as I replied to the previous email, sorry.

> keep the slightly ugly version, or introduce #defines.
> ZPCI_PCIAS_IOREGION_{MIN,MAX} (and maybe ZPCI_PCIAS_CONFIG for 15)?


I agree something is missing.
But I am not sure that a #define brings clarity.
I would prefer to add a comment.
	/* A ZPCI PCI card may use any BAR from BAR 0 to BAR 5 */

?


>>> +         * 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;
>>>           }
>>> @@ -513,21 +518,21 @@ 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 lengths 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;
>>> +    default:
>>>           DPRINTF("pcistg invalid space\n");
>>>           setcc(cpu, ZPCI_PCI_LS_ERR);
>>>           s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);
>>
>> Other than the nits, looks good.
> 


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

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

[Pierre Morel]

On 21/11/2017 11:42, Cornelia Huck wrote:
> On Thu, 16 Nov 2017 18:51:52 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> Enhance the fault detection.
>>
>> Fixup the precedence to check the destination path existance
>> before checking for the source accessibility.
>>
>> 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(-)
> 
>> @@ -700,8 +685,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, not greater maxstbl */
> 
> "not greater than maxstlb"

Better I know but greater that 80 characters, this is why I preferred 
broken English.
What do I do ? break the line or English ?

> 
>> +    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;
>>       }
> 
> Looks good.
> 


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

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

[Pierre Morel]

On 21/11/2017 11:52, Cornelia Huck wrote:
> On Thu, 16 Nov 2017 18:51:48 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> This patch fixes the following BUG:
>> Even a guest is able to detect virtio_pci device, the init function
>> the Linux virtio_pci driver will hang because zPCI does not support
>> the subregions used by virtio_pci.
>>
>> It follows that 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.
>>
>> As already stated above, Virtio-PCI uses subregions, which may eventually
>> be discontinuous inside bars instead of a single flat region often used
>> by real devices.
>> 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 done by zPCI.
>>
>> 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.
> 
> I'll play with this a bit, but I think this is good for s390-next.
> 
> [I'd still like to play with this under tcg as well to make sure we are
> endian-clean, but I'm making slower progress there than I hoped.]
> 

OK, thanks.
I wait for the answers for the points I am still not sure, (#define and 
broken English) and send a v3 with the corrections.

Best regards,

Pierre

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

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

[Yi Min Zhao]

在 2017/11/22 上午2:07, Pierre Morel 写道:
> On 21/11/2017 11:42, Cornelia Huck wrote:
>> On Thu, 16 Nov 2017 18:51:52 +0100
>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>
>>> Enhance the fault detection.
>>>
>>> Fixup the precedence to check the destination path existance
>>> before checking for the source accessibility.
>>>
>>> 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(-)
>>
>>> @@ -700,8 +685,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, not greater 
>>> maxstbl */
>>
>> "not greater than maxstlb"
>
> Better I know but greater that 80 characters, this is why I preferred 
> broken English.
> What do I do ? break the line or English ?
less than?
>
>>
>>> +    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;
>>>       }
>>
>> Looks good.
>>
>
>

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

[Cornelia Huck]

On Tue, 21 Nov 2017 19:03:17 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> On 21/11/2017 15:25, Cornelia Huck wrote:
> > On Tue, 21 Nov 2017 11:38:45 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> >> On Thu, 16 Nov 2017 18:51:50 +0100
> >> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:  
> >   
> >>> @@ -493,9 +494,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 valid and do the appropriates operations */
> >>> +    switch (pcias) {
> >>> +    case 0 ... 5:  
> >>
> >> Make this
> >> case 0...5:
> >> ?  
> > 
> > ...only that it confuses the compiler when using numbers. We can either  
> 
> I did not see this as I replied to the previous email, sorry.
> 
> > keep the slightly ugly version, or introduce #defines.
> > ZPCI_PCIAS_IOREGION_{MIN,MAX} (and maybe ZPCI_PCIAS_CONFIG for 15)?  
> 
> 
> I agree something is missing.
> But I am not sure that a #define brings clarity.
> I would prefer to add a comment.
> 	/* A ZPCI PCI card may use any BAR from BAR 0 to BAR 5 */
> 
> ?

It's more a speaking value vs. magic numbers thing. A comment does not
hurt either, though :)

(And we get rid of the spacing as well.)

> 
> 
> >>> +         * 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;
> >>>           }
> >>> @@ -513,21 +518,21 @@ 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 lengths 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;
> >>> +    default:
> >>>           DPRINTF("pcistg invalid space\n");
> >>>           setcc(cpu, ZPCI_PCI_LS_ERR);
> >>>           s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);  
> >>
> >> Other than the nits, looks good.  
> >   
> 
> 

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

[Cornelia Huck]

On Wed, 22 Nov 2017 13:15:21 +0800
Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:

> 在 2017/11/22 上午2:07, Pierre Morel 写道:
> > On 21/11/2017 11:42, Cornelia Huck wrote:  
> >> On Thu, 16 Nov 2017 18:51:52 +0100
> >> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> >>  
> >>> Enhance the fault detection.
> >>>
> >>> Fixup the precedence to check the destination path existance
> >>> before checking for the source accessibility.
> >>>
> >>> 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(-)  
> >>  
> >>> @@ -700,8 +685,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, not greater 
> >>> maxstbl */  
> >>
> >> "not greater than maxstlb"  
> >
> > Better I know but greater that 80 characters, this is why I preferred 
> > broken English.
> > What do I do ? break the line or English ?  
> less than?

It's less or equal, no?

In any case, I prefer a multi-line comment over awkward language.

> >  
> >>  
> >>> +    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;
> >>>       }  
> >>
> >> Looks good.
> >>  
> >
> >  
> 

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

[Pierre Morel]

On 22/11/2017 14:28, Cornelia Huck wrote:
> On Wed, 22 Nov 2017 13:15:21 +0800
> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
> 
>> 在 2017/11/22 上午2:07, Pierre Morel 写道:
>>> On 21/11/2017 11:42, Cornelia Huck wrote:
>>>> On Thu, 16 Nov 2017 18:51:52 +0100
>>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>>>   
>>>>> Enhance the fault detection.
>>>>>
>>>>> Fixup the precedence to check the destination path existance
>>>>> before checking for the source accessibility.
>>>>>
>>>>> 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(-)
>>>>   
>>>>> @@ -700,8 +685,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, not greater
>>>>> maxstbl */
>>>>
>>>> "not greater than maxstlb"
>>>
>>> Better I know but greater that 80 characters, this is why I preferred
>>> broken English.
>>> What do I do ? break the line or English ?
>> less than?
> 
> It's less or equal, no?
> 
> In any case, I prefer a multi-line comment over awkward language.
> 

OK, thanks

>>>   
>>>>   
>>>>> +    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;
>>>>>        }
>>>>
>>>> Looks good.
>>>>   
>>>
>>>   
>>
> 


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

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

[Pierre Morel]

On 22/11/2017 14:25, Cornelia Huck wrote:
> On Tue, 21 Nov 2017 19:03:17 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> On 21/11/2017 15:25, Cornelia Huck wrote:
>>> On Tue, 21 Nov 2017 11:38:45 +0100
>>> Cornelia Huck <cohuck@redhat.com> wrote:
>>>    
>>>> On Thu, 16 Nov 2017 18:51:50 +0100
>>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>>    
>>>>> @@ -493,9 +494,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 valid and do the appropriates operations */
>>>>> +    switch (pcias) {
>>>>> +    case 0 ... 5:
>>>>
>>>> Make this
>>>> case 0...5:
>>>> ?
>>>
>>> ...only that it confuses the compiler when using numbers. We can either
>>
>> I did not see this as I replied to the previous email, sorry.
>>
>>> keep the slightly ugly version, or introduce #defines.
>>> ZPCI_PCIAS_IOREGION_{MIN,MAX} (and maybe ZPCI_PCIAS_CONFIG for 15)?
>>
>>
>> I agree something is missing.
>> But I am not sure that a #define brings clarity.
>> I would prefer to add a comment.
>> 	/* A ZPCI PCI card may use any BAR from BAR 0 to BAR 5 */
>>
>> ?
> 
> It's more a speaking value vs. magic numbers thing. A comment does not
> hurt either, though :)
> 
> (And we get rid of the spacing as well.)

OK, thanks.

> 
>>
>>
>>>>> +         * 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;
>>>>>            }
>>>>> @@ -513,21 +518,21 @@ 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 lengths 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;
>>>>> +    default:
>>>>>            DPRINTF("pcistg invalid space\n");
>>>>>            setcc(cpu, ZPCI_PCI_LS_ERR);
>>>>>            s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);
>>>>
>>>> Other than the nits, looks good.
>>>    
>>
>>
> 


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