Common function measurement block is used to report counters of
successfully issued pcilg/stg/stb and rpcit instructions. This patch
introduces a new struct ZpciFmb and schedules a timer callback to
copy fmb to the guest memory at a interval time which is set to
4s by default. While attemping to update fmb failed, an event error
would be generated. After pcilg/stg/stb and rpcit interception
handlers issue successfully, increase the related counter. The guest
could pass null address to switch off FMB and stop corresponding
timer.

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

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index e3e0ebb7f6..7bd0b9d1e5 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -967,6 +967,7 @@ static void s390_pcihost_hot_unplug(HotplugHandler *hotplug_dev,
     bus = pci_get_bus(pci_dev);
     devfn = pci_dev->devfn;
     object_unparent(OBJECT(pci_dev));
+    fmb_timer_free(pbdev);
     s390_pci_msix_free(pbdev);
     s390_pci_iommu_free(s, bus, devfn);
     pbdev->pdev = NULL;
@@ -1139,7 +1140,7 @@ static void s390_pci_device_reset(DeviceState *dev)
         pci_dereg_ioat(pbdev->iommu);
     }
 
-    pbdev->fmb_addr = 0;
+    fmb_timer_free(pbdev);
 }
 
 static void s390_pci_get_fid(Object *obj, Visitor *v, const char *name,
diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
index 1f7f9b5814..fdf13a19c0 100644
--- a/hw/s390x/s390-pci-bus.h
+++ b/hw/s390x/s390-pci-bus.h
@@ -286,6 +286,28 @@ typedef struct S390PCIIOMMUTable {
     S390PCIIOMMU *iommu[PCI_SLOT_MAX];
 } S390PCIIOMMUTable;
 
+/* Function Measurement Block */
+#define DEFAULT_MUI 4000
+#define UPDATE_U_BIT 0x1ULL
+#define FMBK_MASK 0xfULL
+
+typedef struct ZpciFmbFmt0 {
+    uint64_t dma_rbytes;
+    uint64_t dma_wbytes;
+} ZpciFmbFmt0;
+
+typedef struct ZpciFmb {
+    uint8_t format;
+    uint8_t fmt_ind[3];
+    uint32_t sample;
+    uint64_t last_update;
+    uint64_t ld_ops;
+    uint64_t st_ops;
+    uint64_t stb_ops;
+    uint64_t rpcit_ops;
+    ZpciFmbFmt0 fmt0;
+} QEMU_PACKED __attribute((__aligned__(8))) ZpciFmb;
+
 struct S390PCIBusDevice {
     DeviceState qdev;
     PCIDevice *pdev;
@@ -297,6 +319,8 @@ struct S390PCIBusDevice {
     uint32_t fid;
     bool fid_defined;
     uint64_t fmb_addr;
+    ZpciFmb fmb;
+    QEMUTimer *fmb_timer;
     uint8_t isc;
     uint16_t noi;
     uint16_t maxstbl;
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 7b61367ee3..1ed5cb91d0 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -19,6 +19,7 @@
 #include "exec/memory-internal.h"
 #include "qemu/error-report.h"
 #include "sysemu/hw_accel.h"
+#include "hw/s390x/tod.h"
 
 #ifndef DEBUG_S390PCI_INST
 #define DEBUG_S390PCI_INST  0
@@ -293,7 +294,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
         resgrp->fr = 1;
         stq_p(&resgrp->dasm, 0);
         stq_p(&resgrp->msia, ZPCI_MSI_ADDR);
-        stw_p(&resgrp->mui, 0);
+        stw_p(&resgrp->mui, DEFAULT_MUI);
         stw_p(&resgrp->i, 128);
         stw_p(&resgrp->maxstbl, 128);
         resgrp->version = 0;
@@ -456,6 +457,10 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
         return 0;
     }
 
+    if (pbdev->fmb_addr) {
+        pbdev->fmb.ld_ops++;
+    }
+
     env->regs[r1] = data;
     setcc(cpu, ZPCI_PCI_LS_OK);
     return 0;
@@ -561,6 +566,10 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
         return 0;
     }
 
+    if (pbdev->fmb_addr) {
+        pbdev->fmb.st_ops++;
+    }
+
     setcc(cpu, ZPCI_PCI_LS_OK);
     return 0;
 }
@@ -681,6 +690,9 @@ err:
         s390_set_status_code(env, r1, ZPCI_PCI_ST_FUNC_IN_ERR);
         s390_pci_generate_error_event(error, pbdev->fh, pbdev->fid, start, 0);
     } else {
+        if (pbdev->fmb_addr) {
+            pbdev->fmb.rpcit_ops++;
+        }
         setcc(cpu, ZPCI_PCI_LS_OK);
     }
     return 0;
@@ -783,6 +795,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
         }
     }
 
+    if (pbdev->fmb_addr) {
+        pbdev->fmb.stb_ops++;
+    }
+
     setcc(cpu, ZPCI_PCI_LS_OK);
     return 0;
 
@@ -889,6 +905,63 @@ void pci_dereg_ioat(S390PCIIOMMU *iommu)
     iommu->g_iota = 0;
 }
 
+void fmb_timer_free(S390PCIBusDevice *pbdev)
+{
+    if (pbdev->fmb_timer) {
+        timer_del(pbdev->fmb_timer);
+        timer_free(pbdev->fmb_timer);
+        pbdev->fmb_timer = NULL;
+    }
+    pbdev->fmb_addr = 0;
+    memset(&pbdev->fmb, 0, sizeof(ZpciFmb));
+}
+
+static int fmb_do_update(S390PCIBusDevice *pbdev, uint8_t offset, int len)
+{
+    MemTxResult ret;
+
+    ret = address_space_write(&address_space_memory,
+                              pbdev->fmb_addr + (uint64_t)offset,
+                              MEMTXATTRS_UNSPECIFIED,
+                              (uint8_t *)&pbdev->fmb + offset,
+                              len);
+    if (ret) {
+        s390_pci_generate_error_event(ERR_EVENT_FMBA, pbdev->fh, pbdev->fid,
+                                      pbdev->fmb_addr, 0);
+        fmb_timer_free(pbdev);
+    }
+
+    return ret;
+}
+
+static void fmb_update(void *opaque)
+{
+    S390PCIBusDevice *pbdev = opaque;
+    int64_t t = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
+    uint8_t offset = offsetof(ZpciFmb, last_update);
+
+    /* Update U bit */
+    pbdev->fmb.last_update |= UPDATE_U_BIT;
+    if (fmb_do_update(pbdev, offset, sizeof(uint64_t))) {
+        return;
+    }
+
+    /* Update FMB counters */
+    pbdev->fmb.sample++;
+    if (fmb_do_update(pbdev, 0, sizeof(ZpciFmb))) {
+        return;
+    }
+
+    /* Clear U bit and update the time */
+    pbdev->fmb.last_update = time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+    pbdev->fmb.last_update &= ~UPDATE_U_BIT;
+    if (fmb_do_update(pbdev, offset, sizeof(uint64_t))) {
+        return;
+    }
+
+    timer_mod(pbdev->fmb_timer, t + DEFAULT_MUI);
+}
+
 int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
                         uintptr_t ra)
 {
@@ -1018,9 +1091,35 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
             s390_set_status_code(env, r1, ZPCI_MOD_ST_SEQUENCE);
         }
         break;
-    case ZPCI_MOD_FC_SET_MEASURE:
-        pbdev->fmb_addr = ldq_p(&fib.fmb_addr);
+    case ZPCI_MOD_FC_SET_MEASURE: {
+        uint64_t fmb_addr = ldq_p(&fib.fmb_addr);
+
+        if (fmb_addr & FMBK_MASK) {
+            cc = ZPCI_PCI_LS_ERR;
+            s390_pci_generate_error_event(ERR_EVENT_FMBPRO, pbdev->fh,
+                                          pbdev->fid, fmb_addr, 0);
+            fmb_timer_free(pbdev);
+            break;
+        }
+
+        if (!fmb_addr) {
+            /* Stop updating FMB. */
+            fmb_timer_free(pbdev);
+            break;
+        }
+
+        pbdev->fmb_addr = fmb_addr;
+        if (!pbdev->fmb_timer) {
+            pbdev->fmb_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
+                                            fmb_update, pbdev);
+        } else if (timer_pending(pbdev->fmb_timer)) {
+            /* Remove pending timer to update FMB address. */
+            timer_del(pbdev->fmb_timer);
+        }
+        timer_mod(pbdev->fmb_timer,
+                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + DEFAULT_MUI);
         break;
+    }
     default:
         s390_program_interrupt(&cpu->env, PGM_OPERAND, 6, ra);
         cc = ZPCI_PCI_LS_ERR;
diff --git a/hw/s390x/s390-pci-inst.h b/hw/s390x/s390-pci-inst.h
index 91c3d61f2a..fa3bf8b5aa 100644
--- a/hw/s390x/s390-pci-inst.h
+++ b/hw/s390x/s390-pci-inst.h
@@ -303,6 +303,7 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
                         uintptr_t ra);
 int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
                          uintptr_t ra);
+void fmb_timer_free(S390PCIBusDevice *pbdev);
 
 #define ZPCI_IO_BAR_MIN 0
 #define ZPCI_IO_BAR_MAX 5
-- 
Yi Min

No comment?


在 2018/9/4 下午5:15, Yi Min Zhao 写道:
> Common function measurement block is used to report counters of
> successfully issued pcilg/stg/stb and rpcit instructions. This patch
> introduces a new struct ZpciFmb and schedules a timer callback to
> copy fmb to the guest memory at a interval time which is set to
> 4s by default. While attemping to update fmb failed, an event error
> would be generated. After pcilg/stg/stb and rpcit interception
> handlers issue successfully, increase the related counter. The guest
> could pass null address to switch off FMB and stop corresponding
> timer.
>
> Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com>
> Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>
> ---
>   hw/s390x/s390-pci-bus.c  |   3 +-
>   hw/s390x/s390-pci-bus.h  |  24 +++++++++++
>   hw/s390x/s390-pci-inst.c | 105 +++++++++++++++++++++++++++++++++++++++++++++--
>   hw/s390x/s390-pci-inst.h |   1 +
>   4 files changed, 129 insertions(+), 4 deletions(-)
>
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index e3e0ebb7f6..7bd0b9d1e5 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -967,6 +967,7 @@ static void s390_pcihost_hot_unplug(HotplugHandler *hotplug_dev,
>       bus = pci_get_bus(pci_dev);
>       devfn = pci_dev->devfn;
>       object_unparent(OBJECT(pci_dev));
> +    fmb_timer_free(pbdev);
>       s390_pci_msix_free(pbdev);
>       s390_pci_iommu_free(s, bus, devfn);
>       pbdev->pdev = NULL;
> @@ -1139,7 +1140,7 @@ static void s390_pci_device_reset(DeviceState *dev)
>           pci_dereg_ioat(pbdev->iommu);
>       }
>   
> -    pbdev->fmb_addr = 0;
> +    fmb_timer_free(pbdev);
>   }
>   
>   static void s390_pci_get_fid(Object *obj, Visitor *v, const char *name,
> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
> index 1f7f9b5814..fdf13a19c0 100644
> --- a/hw/s390x/s390-pci-bus.h
> +++ b/hw/s390x/s390-pci-bus.h
> @@ -286,6 +286,28 @@ typedef struct S390PCIIOMMUTable {
>       S390PCIIOMMU *iommu[PCI_SLOT_MAX];
>   } S390PCIIOMMUTable;
>   
> +/* Function Measurement Block */
> +#define DEFAULT_MUI 4000
> +#define UPDATE_U_BIT 0x1ULL
> +#define FMBK_MASK 0xfULL
> +
> +typedef struct ZpciFmbFmt0 {
> +    uint64_t dma_rbytes;
> +    uint64_t dma_wbytes;
> +} ZpciFmbFmt0;
> +
> +typedef struct ZpciFmb {
> +    uint8_t format;
> +    uint8_t fmt_ind[3];
> +    uint32_t sample;
> +    uint64_t last_update;
> +    uint64_t ld_ops;
> +    uint64_t st_ops;
> +    uint64_t stb_ops;
> +    uint64_t rpcit_ops;
> +    ZpciFmbFmt0 fmt0;
> +} QEMU_PACKED __attribute((__aligned__(8))) ZpciFmb;
> +
>   struct S390PCIBusDevice {
>       DeviceState qdev;
>       PCIDevice *pdev;
> @@ -297,6 +319,8 @@ struct S390PCIBusDevice {
>       uint32_t fid;
>       bool fid_defined;
>       uint64_t fmb_addr;
> +    ZpciFmb fmb;
> +    QEMUTimer *fmb_timer;
>       uint8_t isc;
>       uint16_t noi;
>       uint16_t maxstbl;
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 7b61367ee3..1ed5cb91d0 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -19,6 +19,7 @@
>   #include "exec/memory-internal.h"
>   #include "qemu/error-report.h"
>   #include "sysemu/hw_accel.h"
> +#include "hw/s390x/tod.h"
>   
>   #ifndef DEBUG_S390PCI_INST
>   #define DEBUG_S390PCI_INST  0
> @@ -293,7 +294,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
>           resgrp->fr = 1;
>           stq_p(&resgrp->dasm, 0);
>           stq_p(&resgrp->msia, ZPCI_MSI_ADDR);
> -        stw_p(&resgrp->mui, 0);
> +        stw_p(&resgrp->mui, DEFAULT_MUI);
>           stw_p(&resgrp->i, 128);
>           stw_p(&resgrp->maxstbl, 128);
>           resgrp->version = 0;
> @@ -456,6 +457,10 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
>           return 0;
>       }
>   
> +    if (pbdev->fmb_addr) {
> +        pbdev->fmb.ld_ops++;
> +    }
> +
>       env->regs[r1] = data;
>       setcc(cpu, ZPCI_PCI_LS_OK);
>       return 0;
> @@ -561,6 +566,10 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
>           return 0;
>       }
>   
> +    if (pbdev->fmb_addr) {
> +        pbdev->fmb.st_ops++;
> +    }
> +
>       setcc(cpu, ZPCI_PCI_LS_OK);
>       return 0;
>   }
> @@ -681,6 +690,9 @@ err:
>           s390_set_status_code(env, r1, ZPCI_PCI_ST_FUNC_IN_ERR);
>           s390_pci_generate_error_event(error, pbdev->fh, pbdev->fid, start, 0);
>       } else {
> +        if (pbdev->fmb_addr) {
> +            pbdev->fmb.rpcit_ops++;
> +        }
>           setcc(cpu, ZPCI_PCI_LS_OK);
>       }
>       return 0;
> @@ -783,6 +795,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>           }
>       }
>   
> +    if (pbdev->fmb_addr) {
> +        pbdev->fmb.stb_ops++;
> +    }
> +
>       setcc(cpu, ZPCI_PCI_LS_OK);
>       return 0;
>   
> @@ -889,6 +905,63 @@ void pci_dereg_ioat(S390PCIIOMMU *iommu)
>       iommu->g_iota = 0;
>   }
>   
> +void fmb_timer_free(S390PCIBusDevice *pbdev)
> +{
> +    if (pbdev->fmb_timer) {
> +        timer_del(pbdev->fmb_timer);
> +        timer_free(pbdev->fmb_timer);
> +        pbdev->fmb_timer = NULL;
> +    }
> +    pbdev->fmb_addr = 0;
> +    memset(&pbdev->fmb, 0, sizeof(ZpciFmb));
> +}
> +
> +static int fmb_do_update(S390PCIBusDevice *pbdev, uint8_t offset, int len)
> +{
> +    MemTxResult ret;
> +
> +    ret = address_space_write(&address_space_memory,
> +                              pbdev->fmb_addr + (uint64_t)offset,
> +                              MEMTXATTRS_UNSPECIFIED,
> +                              (uint8_t *)&pbdev->fmb + offset,
> +                              len);
> +    if (ret) {
> +        s390_pci_generate_error_event(ERR_EVENT_FMBA, pbdev->fh, pbdev->fid,
> +                                      pbdev->fmb_addr, 0);
> +        fmb_timer_free(pbdev);
> +    }
> +
> +    return ret;
> +}
> +
> +static void fmb_update(void *opaque)
> +{
> +    S390PCIBusDevice *pbdev = opaque;
> +    int64_t t = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
> +    uint8_t offset = offsetof(ZpciFmb, last_update);
> +
> +    /* Update U bit */
> +    pbdev->fmb.last_update |= UPDATE_U_BIT;
> +    if (fmb_do_update(pbdev, offset, sizeof(uint64_t))) {
> +        return;
> +    }
> +
> +    /* Update FMB counters */
> +    pbdev->fmb.sample++;
> +    if (fmb_do_update(pbdev, 0, sizeof(ZpciFmb))) {
> +        return;
> +    }
> +
> +    /* Clear U bit and update the time */
> +    pbdev->fmb.last_update = time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> +    pbdev->fmb.last_update &= ~UPDATE_U_BIT;
> +    if (fmb_do_update(pbdev, offset, sizeof(uint64_t))) {
> +        return;
> +    }
> +
> +    timer_mod(pbdev->fmb_timer, t + DEFAULT_MUI);
> +}
> +
>   int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
>                           uintptr_t ra)
>   {
> @@ -1018,9 +1091,35 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
>               s390_set_status_code(env, r1, ZPCI_MOD_ST_SEQUENCE);
>           }
>           break;
> -    case ZPCI_MOD_FC_SET_MEASURE:
> -        pbdev->fmb_addr = ldq_p(&fib.fmb_addr);
> +    case ZPCI_MOD_FC_SET_MEASURE: {
> +        uint64_t fmb_addr = ldq_p(&fib.fmb_addr);
> +
> +        if (fmb_addr & FMBK_MASK) {
> +            cc = ZPCI_PCI_LS_ERR;
> +            s390_pci_generate_error_event(ERR_EVENT_FMBPRO, pbdev->fh,
> +                                          pbdev->fid, fmb_addr, 0);
> +            fmb_timer_free(pbdev);
> +            break;
> +        }
> +
> +        if (!fmb_addr) {
> +            /* Stop updating FMB. */
> +            fmb_timer_free(pbdev);
> +            break;
> +        }
> +
> +        pbdev->fmb_addr = fmb_addr;
> +        if (!pbdev->fmb_timer) {
> +            pbdev->fmb_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
> +                                            fmb_update, pbdev);
> +        } else if (timer_pending(pbdev->fmb_timer)) {
> +            /* Remove pending timer to update FMB address. */
> +            timer_del(pbdev->fmb_timer);
> +        }
> +        timer_mod(pbdev->fmb_timer,
> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + DEFAULT_MUI);
>           break;
> +    }
>       default:
>           s390_program_interrupt(&cpu->env, PGM_OPERAND, 6, ra);
>           cc = ZPCI_PCI_LS_ERR;
> diff --git a/hw/s390x/s390-pci-inst.h b/hw/s390x/s390-pci-inst.h
> index 91c3d61f2a..fa3bf8b5aa 100644
> --- a/hw/s390x/s390-pci-inst.h
> +++ b/hw/s390x/s390-pci-inst.h
> @@ -303,6 +303,7 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
>                           uintptr_t ra);
>   int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
>                            uintptr_t ra);
> +void fmb_timer_free(S390PCIBusDevice *pbdev);
>   
>   #define ZPCI_IO_BAR_MIN 0
>   #define ZPCI_IO_BAR_MAX 5

-- 
Yi Min

On 2018-09-19 09:08, Yi Min Zhao wrote:
> No comment?

Since the zPCI spec is not available to the public, it's quite hard to
give any valuable comments here... I'll try anyway...

> 在 2018/9/4 下午5:15, Yi Min Zhao 写道:
>> Common function measurement block is used to report counters of
>> successfully issued pcilg/stg/stb and rpcit instructions. This patch
>> introduces a new struct ZpciFmb and schedules a timer callback to
>> copy fmb to the guest memory at a interval time which is set to
>> 4s by default. While attemping to update fmb failed, an event error
>> would be generated. After pcilg/stg/stb and rpcit interception
>> handlers issue successfully, increase the related counter. The guest
>> could pass null address to switch off FMB and stop corresponding
>> timer.
>>
>> Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com>
>> Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/s390-pci-bus.c  |   3 +-
>>   hw/s390x/s390-pci-bus.h  |  24 +++++++++++
>>   hw/s390x/s390-pci-inst.c | 105
>> +++++++++++++++++++++++++++++++++++++++++++++--
>>   hw/s390x/s390-pci-inst.h |   1 +
>>   4 files changed, 129 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index e3e0ebb7f6..7bd0b9d1e5 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -967,6 +967,7 @@ static void s390_pcihost_hot_unplug(HotplugHandler
>> *hotplug_dev,
>>       bus = pci_get_bus(pci_dev);
>>       devfn = pci_dev->devfn;
>>       object_unparent(OBJECT(pci_dev));
>> +    fmb_timer_free(pbdev);
>>       s390_pci_msix_free(pbdev);
>>       s390_pci_iommu_free(s, bus, devfn);
>>       pbdev->pdev = NULL;
>> @@ -1139,7 +1140,7 @@ static void s390_pci_device_reset(DeviceState *dev)
>>           pci_dereg_ioat(pbdev->iommu);
>>       }
>>   -    pbdev->fmb_addr = 0;
>> +    fmb_timer_free(pbdev);
>>   }
>>     static void s390_pci_get_fid(Object *obj, Visitor *v, const char
>> *name,
>> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
>> index 1f7f9b5814..fdf13a19c0 100644
>> --- a/hw/s390x/s390-pci-bus.h
>> +++ b/hw/s390x/s390-pci-bus.h
>> @@ -286,6 +286,28 @@ typedef struct S390PCIIOMMUTable {
>>       S390PCIIOMMU *iommu[PCI_SLOT_MAX];
>>   } S390PCIIOMMUTable;
>>   +/* Function Measurement Block */
>> +#define DEFAULT_MUI 4000
>> +#define UPDATE_U_BIT 0x1ULL
>> +#define FMBK_MASK 0xfULL
>> +
>> +typedef struct ZpciFmbFmt0 {
>> +    uint64_t dma_rbytes;
>> +    uint64_t dma_wbytes;
>> +} ZpciFmbFmt0;
>> +
>> +typedef struct ZpciFmb {
>> +    uint8_t format;
>> +    uint8_t fmt_ind[3];
>> +    uint32_t sample;
>> +    uint64_t last_update;
>> +    uint64_t ld_ops;
>> +    uint64_t st_ops;
>> +    uint64_t stb_ops;
>> +    uint64_t rpcit_ops;
>> +    ZpciFmbFmt0 fmt0;
>> +} QEMU_PACKED __attribute((__aligned__(8))) ZpciFmb;

All the fields in this struct are naturally aligned already, so I'd
maybe rather drop the QEMU_PACKED and add a
QEMU_BUILD_BUG_MSG(sizeof(ZpciFmb) != xx, ...) statement afterwards.

Also the __aligned__(8) is likely not going to work as expected...

>>   struct S390PCIBusDevice {
>>       DeviceState qdev;
>>       PCIDevice *pdev;
>> @@ -297,6 +319,8 @@ struct S390PCIBusDevice {
>>       uint32_t fid;
>>       bool fid_defined;
>>       uint64_t fmb_addr;
>> +    ZpciFmb fmb;

... since you embed it here in another struct which does not have any
alignment requirements. This is likely going to cause an error with GCC
8.1, we've had this problem in the past already:

https://git.qemu.org/?p=qemu.git;a=commitdiff;h=a6e4385dea94850d7b06b0

Is the __align__(8) required at all? As far as I understand the code,
the struct is not living inside the guest memory, is it? So you could
simply drop the __align__(8).
But if you need it, I think you have to allocate the memory for ZpciFmb
separately (and use a "ZpciFmb *fmb" here instead).

>> +    QEMUTimer *fmb_timer;
>>       uint8_t isc;
>>       uint16_t noi;
>>       uint16_t maxstbl;
>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>> index 7b61367ee3..1ed5cb91d0 100644
>> --- a/hw/s390x/s390-pci-inst.c
>> +++ b/hw/s390x/s390-pci-inst.c
[...]
>> +static int fmb_do_update(S390PCIBusDevice *pbdev, uint8_t offset, int
>> len)

Any reason for making "offset" an uint8_t only? Seems unnecessary to me
... since you use it for an "offsetof()" value below, I'd like to
suggest to use size_t instead...

>> +{
>> +    MemTxResult ret;
>> +
>> +    ret = address_space_write(&address_space_memory,
>> +                              pbdev->fmb_addr + (uint64_t)offset,

... then you can also remove this (uint64_t) cast.

>> +                              MEMTXATTRS_UNSPECIFIED,
>> +                              (uint8_t *)&pbdev->fmb + offset,
>> +                              len);
>> +    if (ret) {
>> +        s390_pci_generate_error_event(ERR_EVENT_FMBA, pbdev->fh,
>> pbdev->fid,
>> +                                      pbdev->fmb_addr, 0);
>> +        fmb_timer_free(pbdev);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static void fmb_update(void *opaque)
>> +{
>> +    S390PCIBusDevice *pbdev = opaque;
>> +    int64_t t = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
>> +    uint8_t offset = offsetof(ZpciFmb, last_update);
>> +
>> +    /* Update U bit */
>> +    pbdev->fmb.last_update |= UPDATE_U_BIT;
>> +    if (fmb_do_update(pbdev, offset, sizeof(uint64_t))) {
>> +        return;
>> +    }
[...]

 Cheers,
  Thomas

在 2018/9/19 下午3:53, Thomas Huth 写道:
> On 2018-09-19 09:08, Yi Min Zhao wrote:
>> No comment?
> Since the zPCI spec is not available to the public, it's quite hard to
> give any valuable comments here... I'll try anyway...
>
>> 在 2018/9/4 下午5:15, Yi Min Zhao 写道:
>>> Common function measurement block is used to report counters of
>>> successfully issued pcilg/stg/stb and rpcit instructions. This patch
>>> introduces a new struct ZpciFmb and schedules a timer callback to
>>> copy fmb to the guest memory at a interval time which is set to
>>> 4s by default. While attemping to update fmb failed, an event error
>>> would be generated. After pcilg/stg/stb and rpcit interception
>>> handlers issue successfully, increase the related counter. The guest
>>> could pass null address to switch off FMB and stop corresponding
>>> timer.
>>>
>>> Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com>
>>> Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>
>>> ---
>>>    hw/s390x/s390-pci-bus.c  |   3 +-
>>>    hw/s390x/s390-pci-bus.h  |  24 +++++++++++
>>>    hw/s390x/s390-pci-inst.c | 105
>>> +++++++++++++++++++++++++++++++++++++++++++++--
>>>    hw/s390x/s390-pci-inst.h |   1 +
>>>    4 files changed, 129 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>> index e3e0ebb7f6..7bd0b9d1e5 100644
>>> --- a/hw/s390x/s390-pci-bus.c
>>> +++ b/hw/s390x/s390-pci-bus.c
>>> @@ -967,6 +967,7 @@ static void s390_pcihost_hot_unplug(HotplugHandler
>>> *hotplug_dev,
>>>        bus = pci_get_bus(pci_dev);
>>>        devfn = pci_dev->devfn;
>>>        object_unparent(OBJECT(pci_dev));
>>> +    fmb_timer_free(pbdev);
>>>        s390_pci_msix_free(pbdev);
>>>        s390_pci_iommu_free(s, bus, devfn);
>>>        pbdev->pdev = NULL;
>>> @@ -1139,7 +1140,7 @@ static void s390_pci_device_reset(DeviceState *dev)
>>>            pci_dereg_ioat(pbdev->iommu);
>>>        }
>>>    -    pbdev->fmb_addr = 0;
>>> +    fmb_timer_free(pbdev);
>>>    }
>>>      static void s390_pci_get_fid(Object *obj, Visitor *v, const char
>>> *name,
>>> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
>>> index 1f7f9b5814..fdf13a19c0 100644
>>> --- a/hw/s390x/s390-pci-bus.h
>>> +++ b/hw/s390x/s390-pci-bus.h
>>> @@ -286,6 +286,28 @@ typedef struct S390PCIIOMMUTable {
>>>        S390PCIIOMMU *iommu[PCI_SLOT_MAX];
>>>    } S390PCIIOMMUTable;
>>>    +/* Function Measurement Block */
>>> +#define DEFAULT_MUI 4000
>>> +#define UPDATE_U_BIT 0x1ULL
>>> +#define FMBK_MASK 0xfULL
>>> +
>>> +typedef struct ZpciFmbFmt0 {
>>> +    uint64_t dma_rbytes;
>>> +    uint64_t dma_wbytes;
>>> +} ZpciFmbFmt0;
>>> +
>>> +typedef struct ZpciFmb {
>>> +    uint8_t format;
>>> +    uint8_t fmt_ind[3];
>>> +    uint32_t sample;
>>> +    uint64_t last_update;
>>> +    uint64_t ld_ops;
>>> +    uint64_t st_ops;
>>> +    uint64_t stb_ops;
>>> +    uint64_t rpcit_ops;
>>> +    ZpciFmbFmt0 fmt0;
>>> +} QEMU_PACKED __attribute((__aligned__(8))) ZpciFmb;
> All the fields in this struct are naturally aligned already, so I'd
> maybe rather drop the QEMU_PACKED and add a
> QEMU_BUILD_BUG_MSG(sizeof(ZpciFmb) != xx, ...) statement afterwards.
Currently we only implement FMT0. There're other FMTs to be implemented 
in future.
So here there would be a union and we can't give a specific size to 
QEMU_BUILD_BUG_MSG.
Can we use the max size for checking?
>
> Also the __aligned__(8) is likely not going to work as expected...
>
>>>    struct S390PCIBusDevice {
>>>        DeviceState qdev;
>>>        PCIDevice *pdev;
>>> @@ -297,6 +319,8 @@ struct S390PCIBusDevice {
>>>        uint32_t fid;
>>>        bool fid_defined;
>>>        uint64_t fmb_addr;
>>> +    ZpciFmb fmb;
> ... since you embed it here in another struct which does not have any
> alignment requirements. This is likely going to cause an error with GCC
> 8.1, we've had this problem in the past already:
>
> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=a6e4385dea94850d7b06b0
Ah...I didn't test the code with gcc 8+. GCC I used is 7.2.
It should get the same warining.
>
> Is the __align__(8) required at all? As far as I understand the code,
> the struct is not living inside the guest memory, is it? So you could
> simply drop the __align__(8).
> But if you need it, I think you have to allocate the memory for ZpciFmb
> separately (and use a "ZpciFmb *fmb" here instead).
I want to copy the entire structure to the guest memory during updating FMB.
It's not a good idea to do copy for all members multiple times.
As your comment, I think we have to allocate memory for ZpciFmb.
>
>>> +    QEMUTimer *fmb_timer;
>>>        uint8_t isc;
>>>        uint16_t noi;
>>>        uint16_t maxstbl;
>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>>> index 7b61367ee3..1ed5cb91d0 100644
>>> --- a/hw/s390x/s390-pci-inst.c
>>> +++ b/hw/s390x/s390-pci-inst.c
> [...]
>>> +static int fmb_do_update(S390PCIBusDevice *pbdev, uint8_t offset, int
>>> len)
> Any reason for making "offset" an uint8_t only? Seems unnecessary to me
> ... since you use it for an "offsetof()" value below, I'd like to
> suggest to use size_t instead...
Yes.
>
>>> +{
>>> +    MemTxResult ret;
>>> +
>>> +    ret = address_space_write(&address_space_memory,
>>> +                              pbdev->fmb_addr + (uint64_t)offset,
> ... then you can also remove this (uint64_t) cast.
Right.
>
>>> +                              MEMTXATTRS_UNSPECIFIED,
>>> +                              (uint8_t *)&pbdev->fmb + offset,
>>> +                              len);
>>> +    if (ret) {
>>> +        s390_pci_generate_error_event(ERR_EVENT_FMBA, pbdev->fh,
>>> pbdev->fid,
>>> +                                      pbdev->fmb_addr, 0);
>>> +        fmb_timer_free(pbdev);
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static void fmb_update(void *opaque)
>>> +{
>>> +    S390PCIBusDevice *pbdev = opaque;
>>> +    int64_t t = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
>>> +    uint8_t offset = offsetof(ZpciFmb, last_update);
>>> +
>>> +    /* Update U bit */
>>> +    pbdev->fmb.last_update |= UPDATE_U_BIT;
>>> +    if (fmb_do_update(pbdev, offset, sizeof(uint64_t))) {
>>> +        return;
>>> +    }
> [...]
>
>   Cheers,
>    Thomas
>
>
Thank you very much!

-- 
Yi Min

On 2018-09-29 07:48, Yi Min Zhao wrote:
> 
> 在 2018/9/19 下午3:53, Thomas Huth 写道:
>> On 2018-09-19 09:08, Yi Min Zhao wrote:
[...]
>>>> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
>>>> index 1f7f9b5814..fdf13a19c0 100644
>>>> --- a/hw/s390x/s390-pci-bus.h
>>>> +++ b/hw/s390x/s390-pci-bus.h
>>>> @@ -286,6 +286,28 @@ typedef struct S390PCIIOMMUTable {
>>>>        S390PCIIOMMU *iommu[PCI_SLOT_MAX];
>>>>    } S390PCIIOMMUTable;
>>>>    +/* Function Measurement Block */
>>>> +#define DEFAULT_MUI 4000
>>>> +#define UPDATE_U_BIT 0x1ULL
>>>> +#define FMBK_MASK 0xfULL
>>>> +
>>>> +typedef struct ZpciFmbFmt0 {
>>>> +    uint64_t dma_rbytes;
>>>> +    uint64_t dma_wbytes;
>>>> +} ZpciFmbFmt0;
>>>> +
>>>> +typedef struct ZpciFmb {
>>>> +    uint8_t format;
>>>> +    uint8_t fmt_ind[3];
>>>> +    uint32_t sample;
>>>> +    uint64_t last_update;
>>>> +    uint64_t ld_ops;
>>>> +    uint64_t st_ops;
>>>> +    uint64_t stb_ops;
>>>> +    uint64_t rpcit_ops;
>>>> +    ZpciFmbFmt0 fmt0;
>>>> +} QEMU_PACKED __attribute((__aligned__(8))) ZpciFmb;
>> All the fields in this struct are naturally aligned already, so I'd
>> maybe rather drop the QEMU_PACKED and add a
>> QEMU_BUILD_BUG_MSG(sizeof(ZpciFmb) != xx, ...) statement afterwards.
> Currently we only implement FMT0. There're other FMTs to be implemented
> in future.
> So here there would be a union and we can't give a specific size to
> QEMU_BUILD_BUG_MSG.
> Can we use the max size for checking?

I think you could use this to check the beginning of the struct:

QEMU_BUILD_BUG_MSG(offsetof(ZpciFmb, fmt0) != 48, "padding in ZpciFmb");

>>>>    struct S390PCIBusDevice {
>>>>        DeviceState qdev;
>>>>        PCIDevice *pdev;
>>>> @@ -297,6 +319,8 @@ struct S390PCIBusDevice {
>>>>        uint32_t fid;
>>>>        bool fid_defined;
>>>>        uint64_t fmb_addr;
>>>> +    ZpciFmb fmb;
>> ... since you embed it here in another struct which does not have any
>> alignment requirements. This is likely going to cause an error with GCC
>> 8.1, we've had this problem in the past already:
>>
>> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=a6e4385dea94850d7b06b0
> Ah...I didn't test the code with gcc 8+. GCC I used is 7.2.
> It should get the same warining.

Nobody reported the warning in the s390-ccw bios until GCC 8 had been
released, so I assume this is a new warning in GCC 8.

>> Is the __align__(8) required at all? As far as I understand the code,
>> the struct is not living inside the guest memory, is it? So you could
>> simply drop the __align__(8).
>> But if you need it, I think you have to allocate the memory for ZpciFmb
>> separately (and use a "ZpciFmb *fmb" here instead).
> I want to copy the entire structure to the guest memory during updating
> FMB.
> It's not a good idea to do copy for all members multiple times.

Sure, but you're doing the updates through address_space_write(), so as
far as I can see there is currently no need for the
attribute((__aligned__(8))) here (or did I miss something?). Thus I'd
like to suggest to simply remove that attribute here.

 Thomas

在 2018/10/1 下午5:22, Thomas Huth 写道:
> On 2018-09-29 07:48, Yi Min Zhao wrote:
>> 在 2018/9/19 下午3:53, Thomas Huth 写道:
>>> On 2018-09-19 09:08, Yi Min Zhao wrote:
> [...]
>>>>> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
>>>>> index 1f7f9b5814..fdf13a19c0 100644
>>>>> --- a/hw/s390x/s390-pci-bus.h
>>>>> +++ b/hw/s390x/s390-pci-bus.h
>>>>> @@ -286,6 +286,28 @@ typedef struct S390PCIIOMMUTable {
>>>>>         S390PCIIOMMU *iommu[PCI_SLOT_MAX];
>>>>>     } S390PCIIOMMUTable;
>>>>>     +/* Function Measurement Block */
>>>>> +#define DEFAULT_MUI 4000
>>>>> +#define UPDATE_U_BIT 0x1ULL
>>>>> +#define FMBK_MASK 0xfULL
>>>>> +
>>>>> +typedef struct ZpciFmbFmt0 {
>>>>> +    uint64_t dma_rbytes;
>>>>> +    uint64_t dma_wbytes;
>>>>> +} ZpciFmbFmt0;
>>>>> +
>>>>> +typedef struct ZpciFmb {
>>>>> +    uint8_t format;
>>>>> +    uint8_t fmt_ind[3];
>>>>> +    uint32_t sample;
>>>>> +    uint64_t last_update;
>>>>> +    uint64_t ld_ops;
>>>>> +    uint64_t st_ops;
>>>>> +    uint64_t stb_ops;
>>>>> +    uint64_t rpcit_ops;
>>>>> +    ZpciFmbFmt0 fmt0;
>>>>> +} QEMU_PACKED __attribute((__aligned__(8))) ZpciFmb;
>>> All the fields in this struct are naturally aligned already, so I'd
>>> maybe rather drop the QEMU_PACKED and add a
>>> QEMU_BUILD_BUG_MSG(sizeof(ZpciFmb) != xx, ...) statement afterwards.
>> Currently we only implement FMT0. There're other FMTs to be implemented
>> in future.
>> So here there would be a union and we can't give a specific size to
>> QEMU_BUILD_BUG_MSG.
>> Can we use the max size for checking?
> I think you could use this to check the beginning of the struct:
At the beginning of the struct? not after it?
>
> QEMU_BUILD_BUG_MSG(offsetof(ZpciFmb, fmt0) != 48, "padding in ZpciFmb");
I think this could satisfy our requirement.
>
>>>>>     struct S390PCIBusDevice {
>>>>>         DeviceState qdev;
>>>>>         PCIDevice *pdev;
>>>>> @@ -297,6 +319,8 @@ struct S390PCIBusDevice {
>>>>>         uint32_t fid;
>>>>>         bool fid_defined;
>>>>>         uint64_t fmb_addr;
>>>>> +    ZpciFmb fmb;
>>> ... since you embed it here in another struct which does not have any
>>> alignment requirements. This is likely going to cause an error with GCC
>>> 8.1, we've had this problem in the past already:
>>>
>>> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=a6e4385dea94850d7b06b0
>> Ah...I didn't test the code with gcc 8+. GCC I used is 7.2.
>> It should get the same warining.
> Nobody reported the warning in the s390-ccw bios until GCC 8 had been
> released, so I assume this is a new warning in GCC 8.
>
>>> Is the __align__(8) required at all? As far as I understand the code,
>>> the struct is not living inside the guest memory, is it? So you could
>>> simply drop the __align__(8).
>>> But if you need it, I think you have to allocate the memory for ZpciFmb
>>> separately (and use a "ZpciFmb *fmb" here instead).
>> I want to copy the entire structure to the guest memory during updating
>> FMB.
>> It's not a good idea to do copy for all members multiple times.
> Sure, but you're doing the updates through address_space_write(), so as
> far as I can see there is currently no need for the
> attribute((__aligned__(8))) here (or did I miss something?). Thus I'd
> like to suggest to simply remove that attribute here.
>
>   Thomas
>
>
Agree to remove it.

-- 
Yi Min

On Tue,  4 Sep 2018 17:15:49 +0800
Yi Min Zhao <zyimin@linux.ibm.com> wrote:

> Common function measurement block is used to report counters of
> successfully issued pcilg/stg/stb and rpcit instructions. This patch
> introduces a new struct ZpciFmb and schedules a timer callback to
> copy fmb to the guest memory at a interval time which is set to
> 4s by default. While attemping to update fmb failed, an event error
> would be generated. After pcilg/stg/stb and rpcit interception
> handlers issue successfully, increase the related counter. The guest
> could pass null address to switch off FMB and stop corresponding
> timer.

Hard to review without documentation, but some comments below.

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

(...)

> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 7b61367ee3..1ed5cb91d0 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c

> @@ -456,6 +457,10 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
>          return 0;
>      }
>  
> +    if (pbdev->fmb_addr) {
> +        pbdev->fmb.ld_ops++;
> +    }

As fmb is a part of the structure, just update it unconditionally?
You'll only copy it to the guest if measurements are active anyway.

> +
>      env->regs[r1] = data;
>      setcc(cpu, ZPCI_PCI_LS_OK);
>      return 0;

(...)

> @@ -889,6 +905,63 @@ void pci_dereg_ioat(S390PCIIOMMU *iommu)
>      iommu->g_iota = 0;
>  }
>  
> +void fmb_timer_free(S390PCIBusDevice *pbdev)
> +{
> +    if (pbdev->fmb_timer) {
> +        timer_del(pbdev->fmb_timer);
> +        timer_free(pbdev->fmb_timer);
> +        pbdev->fmb_timer = NULL;
> +    }
> +    pbdev->fmb_addr = 0;
> +    memset(&pbdev->fmb, 0, sizeof(ZpciFmb));

Maybe move clearing the buffer to before you enable measurements
instead? (Needed to make my suggestion above work correctly.)

> +}
> +
> +static int fmb_do_update(S390PCIBusDevice *pbdev, uint8_t offset, int len)
> +{
> +    MemTxResult ret;
> +
> +    ret = address_space_write(&address_space_memory,
> +                              pbdev->fmb_addr + (uint64_t)offset,
> +                              MEMTXATTRS_UNSPECIFIED,
> +                              (uint8_t *)&pbdev->fmb + offset,
> +                              len);
> +    if (ret) {
> +        s390_pci_generate_error_event(ERR_EVENT_FMBA, pbdev->fh, pbdev->fid,
> +                                      pbdev->fmb_addr, 0);
> +        fmb_timer_free(pbdev);

So, failure to update the guest-provided area is supposed to disable
measurements?

> +    }
> +
> +    return ret;
> +}
> +
> +static void fmb_update(void *opaque)
> +{
> +    S390PCIBusDevice *pbdev = opaque;
> +    int64_t t = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
> +    uint8_t offset = offsetof(ZpciFmb, last_update);
> +
> +    /* Update U bit */
> +    pbdev->fmb.last_update |= UPDATE_U_BIT;
> +    if (fmb_do_update(pbdev, offset, sizeof(uint64_t))) {
> +        return;
> +    }
> +
> +    /* Update FMB counters */
> +    pbdev->fmb.sample++;
> +    if (fmb_do_update(pbdev, 0, sizeof(ZpciFmb))) {
> +        return;
> +    }
> +
> +    /* Clear U bit and update the time */
> +    pbdev->fmb.last_update = time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> +    pbdev->fmb.last_update &= ~UPDATE_U_BIT;
> +    if (fmb_do_update(pbdev, offset, sizeof(uint64_t))) {
> +        return;
> +    }
> +
> +    timer_mod(pbdev->fmb_timer, t + DEFAULT_MUI);
> +}
> +
>  int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
>                          uintptr_t ra)
>  {
> @@ -1018,9 +1091,35 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
>              s390_set_status_code(env, r1, ZPCI_MOD_ST_SEQUENCE);
>          }
>          break;
> -    case ZPCI_MOD_FC_SET_MEASURE:
> -        pbdev->fmb_addr = ldq_p(&fib.fmb_addr);
> +    case ZPCI_MOD_FC_SET_MEASURE: {
> +        uint64_t fmb_addr = ldq_p(&fib.fmb_addr);
> +
> +        if (fmb_addr & FMBK_MASK) {
> +            cc = ZPCI_PCI_LS_ERR;
> +            s390_pci_generate_error_event(ERR_EVENT_FMBPRO, pbdev->fh,
> +                                          pbdev->fid, fmb_addr, 0);
> +            fmb_timer_free(pbdev);
> +            break;
> +        }
> +
> +        if (!fmb_addr) {
> +            /* Stop updating FMB. */
> +            fmb_timer_free(pbdev);
> +            break;
> +        }
> +
> +        pbdev->fmb_addr = fmb_addr;
> +        if (!pbdev->fmb_timer) {
> +            pbdev->fmb_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
> +                                            fmb_update, pbdev);
> +        } else if (timer_pending(pbdev->fmb_timer)) {
> +            /* Remove pending timer to update FMB address. */
> +            timer_del(pbdev->fmb_timer);
> +        }

So, setting fmb_addr to another address will not reset the counters,
but just cause the blocks to be stored into a different address?

> +        timer_mod(pbdev->fmb_timer,
> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + DEFAULT_MUI);
>          break;
> +    }
>      default:
>          s390_program_interrupt(&cpu->env, PGM_OPERAND, 6, ra);
>          cc = ZPCI_PCI_LS_ERR;

在 2018/9/20 下午6:06, Cornelia Huck 写道:
> On Tue,  4 Sep 2018 17:15:49 +0800
> Yi Min Zhao <zyimin@linux.ibm.com> wrote:
>
>> Common function measurement block is used to report counters of
>> successfully issued pcilg/stg/stb and rpcit instructions. This patch
>> introduces a new struct ZpciFmb and schedules a timer callback to
>> copy fmb to the guest memory at a interval time which is set to
>> 4s by default. While attemping to update fmb failed, an event error
>> would be generated. After pcilg/stg/stb and rpcit interception
>> handlers issue successfully, increase the related counter. The guest
>> could pass null address to switch off FMB and stop corresponding
>> timer.
> Hard to review without documentation, but some comments below.
>
>> Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com>
>> Reviewed-by: Pierre Morel<pmorel@linux.ibm.com>
>> ---
>>   hw/s390x/s390-pci-bus.c  |   3 +-
>>   hw/s390x/s390-pci-bus.h  |  24 +++++++++++
>>   hw/s390x/s390-pci-inst.c | 105 +++++++++++++++++++++++++++++++++++++++++++++--
>>   hw/s390x/s390-pci-inst.h |   1 +
>>   4 files changed, 129 insertions(+), 4 deletions(-)
> (...)
>
>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>> index 7b61367ee3..1ed5cb91d0 100644
>> --- a/hw/s390x/s390-pci-inst.c
>> +++ b/hw/s390x/s390-pci-inst.c
>> @@ -456,6 +457,10 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
>>           return 0;
>>       }
>>   
>> +    if (pbdev->fmb_addr) {
>> +        pbdev->fmb.ld_ops++;
>> +    }
> As fmb is a part of the structure, just update it unconditionally?
> You'll only copy it to the guest if measurements are active anyway.
non-NULL fmb_addr means fmb update is active. So update it if the 
instructions
is successfully handled.
>
>> +
>>       env->regs[r1] = data;
>>       setcc(cpu, ZPCI_PCI_LS_OK);
>>       return 0;
> (...)
>
>> @@ -889,6 +905,63 @@ void pci_dereg_ioat(S390PCIIOMMU *iommu)
>>       iommu->g_iota = 0;
>>   }
>>   
>> +void fmb_timer_free(S390PCIBusDevice *pbdev)
>> +{
>> +    if (pbdev->fmb_timer) {
>> +        timer_del(pbdev->fmb_timer);
>> +        timer_free(pbdev->fmb_timer);
>> +        pbdev->fmb_timer = NULL;
>> +    }
>> +    pbdev->fmb_addr = 0;
>> +    memset(&pbdev->fmb, 0, sizeof(ZpciFmb));
> Maybe move clearing the buffer to before you enable measurements
> instead? (Needed to make my suggestion above work correctly.)
I think it's better to keep clearing code here. As spec, buffer should 
be cleared
if it's disabled although doing as your suggestion could work correctly.
>
>> +}
>> +
>> +static int fmb_do_update(S390PCIBusDevice *pbdev, uint8_t offset, int len)
>> +{
>> +    MemTxResult ret;
>> +
>> +    ret = address_space_write(&address_space_memory,
>> +                              pbdev->fmb_addr + (uint64_t)offset,
>> +                              MEMTXATTRS_UNSPECIFIED,
>> +                              (uint8_t *)&pbdev->fmb + offset,
>> +                              len);
>> +    if (ret) {
>> +        s390_pci_generate_error_event(ERR_EVENT_FMBA, pbdev->fh, pbdev->fid,
>> +                                      pbdev->fmb_addr, 0);
>> +        fmb_timer_free(pbdev);
> So, failure to update the guest-provided area is supposed to disable
> measurements?
As spec, we should stop fmb update.
>
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static void fmb_update(void *opaque)
>> +{
>> +    S390PCIBusDevice *pbdev = opaque;
>> +    int64_t t = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
>> +    uint8_t offset = offsetof(ZpciFmb, last_update);
>> +
>> +    /* Update U bit */
>> +    pbdev->fmb.last_update |= UPDATE_U_BIT;
>> +    if (fmb_do_update(pbdev, offset, sizeof(uint64_t))) {
>> +        return;
>> +    }
>> +
>> +    /* Update FMB counters */
>> +    pbdev->fmb.sample++;
>> +    if (fmb_do_update(pbdev, 0, sizeof(ZpciFmb))) {
>> +        return;
>> +    }
>> +
>> +    /* Clear U bit and update the time */
>> +    pbdev->fmb.last_update = time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
>> +    pbdev->fmb.last_update &= ~UPDATE_U_BIT;
>> +    if (fmb_do_update(pbdev, offset, sizeof(uint64_t))) {
>> +        return;
>> +    }
>> +
>> +    timer_mod(pbdev->fmb_timer, t + DEFAULT_MUI);
>> +}
>> +
>>   int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
>>                           uintptr_t ra)
>>   {
>> @@ -1018,9 +1091,35 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
>>               s390_set_status_code(env, r1, ZPCI_MOD_ST_SEQUENCE);
>>           }
>>           break;
>> -    case ZPCI_MOD_FC_SET_MEASURE:
>> -        pbdev->fmb_addr = ldq_p(&fib.fmb_addr);
>> +    case ZPCI_MOD_FC_SET_MEASURE: {
>> +        uint64_t fmb_addr = ldq_p(&fib.fmb_addr);
>> +
>> +        if (fmb_addr & FMBK_MASK) {
>> +            cc = ZPCI_PCI_LS_ERR;
>> +            s390_pci_generate_error_event(ERR_EVENT_FMBPRO, pbdev->fh,
>> +                                          pbdev->fid, fmb_addr, 0);
>> +            fmb_timer_free(pbdev);
>> +            break;
>> +        }
>> +
>> +        if (!fmb_addr) {
>> +            /* Stop updating FMB. */
>> +            fmb_timer_free(pbdev);
>> +            break;
>> +        }
>> +
>> +        pbdev->fmb_addr = fmb_addr;
>> +        if (!pbdev->fmb_timer) {
>> +            pbdev->fmb_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
>> +                                            fmb_update, pbdev);
>> +        } else if (timer_pending(pbdev->fmb_timer)) {
>> +            /* Remove pending timer to update FMB address. */
>> +            timer_del(pbdev->fmb_timer);
>> +        }
> So, setting fmb_addr to another address will not reset the counters,
> but just cause the blocks to be stored into a different address?
It's not specially described in the spec. This is the result of our 
discussion.
@Pierre, could you please express your opinion?
>
>> +        timer_mod(pbdev->fmb_timer,
>> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + DEFAULT_MUI);
>>           break;
>> +    }
>>       default:
>>           s390_program_interrupt(&cpu->env, PGM_OPERAND, 6, ra);
>>           cc = ZPCI_PCI_LS_ERR;
>

-- 
Yi Min