Re: [Qemu-devel] [PATCH] s390x/pci: add common fmb

2018-09-28 Thread Yi Min Zhao 




在 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 
Reviewed-by: Pierre Morel
---
   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(_space_memory,
+  pbdev->fmb_addr + (uint64_t)offset,

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

Right.



+  MEMTXATTRS_UNSPECIFIED,
+ 

Re: [Qemu-devel] [PATCH] s390x/pci: add common fmb

2018-09-25 Thread Yi Min Zhao 




在 2018/9/20 下午6:06, Cornelia Huck 写道:

On Tue,  4 Sep 2018 17:15:49 +0800
Yi Min Zhao  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 
Reviewed-by: Pierre Morel
---
  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(>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(_space_memory,
+  pbdev->fmb_addr + (uint64_t)offset,
+  MEMTXATTRS_UNSPECIFIED,
+  (uint8_t *)>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(_addr);
+case ZPCI_MOD_FC_SET_MEASURE: {
+uint64_t fmb_addr = ldq_p(_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

Re: [Qemu-devel] [PATCH] s390x/pci: add common fmb

2018-09-20 Thread Cornelia Huck 
On Tue,  4 Sep 2018 17:15:49 +0800
Yi Min Zhao  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 
> Reviewed-by: Pierre Morel
> ---
>  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(>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(_space_memory,
> +  pbdev->fmb_addr + (uint64_t)offset,
> +  MEMTXATTRS_UNSPECIFIED,
> +  (uint8_t *)>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(_addr);
> +case ZPCI_MOD_FC_SET_MEASURE: {
> +uint64_t fmb_addr = ldq_p(_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);
>

Re: [Qemu-devel] [PATCH] s390x/pci: add common fmb

2018-09-19 Thread 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 
>> Reviewed-by: Pierre Morel
>> ---
>>   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(_space_memory,
>> +  pbdev->fmb_addr + (uint64_t)offset,

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

>> +  MEMTXATTRS_UNSPECIFIED,
>> +  (uint8_t *)>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

Re: [Qemu-devel] [PATCH] s390x/pci: add common fmb

2018-09-19 Thread Yi Min Zhao 

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 
Reviewed-by: Pierre Morel
---
  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(>dasm, 0);
  stq_p(>msia, ZPCI_MSI_ADDR);
-stw_p(>mui, 0);
+stw_p(>mui, DEFAULT_MUI);
  stw_p(>i, 128);
  stw_p(>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(>fmb, 0, sizeof(ZpciFmb));
+}
+
+static int fmb_do_update(S390PCIBusDevice *pbdev, uint8_t offset, int len)
+{
+MemTxResult ret;
+
+ret =

[Qemu-devel] [PATCH] s390x/pci: add common fmb

2018-09-04 Thread 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 
Reviewed-by: Pierre Morel
---
 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(>dasm, 0);
 stq_p(>msia, ZPCI_MSI_ADDR);
-stw_p(>mui, 0);
+stw_p(>mui, DEFAULT_MUI);
 stw_p(>i, 128);
 stw_p(>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(>fmb, 0, sizeof(ZpciFmb));
+}
+
+static int fmb_do_update(S390PCIBusDevice *pbdev, uint8_t offset, int len)
+{
+MemTxResult ret;
+
+ret = address_space_write(_space_memory,
+  pbdev->fmb_addr + (uint64_t)offset,
+