在 2018/9/20 下午6:06, Cornelia Huck 写道:
On Tue, 4 Sep 2018 17:15:49 +0800
Yi Min Zhao <zyi...@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 <zyi...@linux.ibm.com>
Reviewed-by: Pierre Morel<pmo...@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
