|
From: | Yi Min Zhao |
Subject: | Re: [Qemu-devel] [PATCH] s390x/pci: add common fmb |
Date: | Tue, 25 Sep 2018 16:18:33 +0800 |
User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 |
在 2018/9/20 下午6:06, Cornelia Huck 写道:
non-NULL fmb_addr means fmb update is active. So update it if the instructionsOn Tue, 4 Sep 2018 17:15:49 +0800 Yi Min Zhao <address@hidden> 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 <address@hidden> Reviewed-by: Pierre Morel<address@hidden> --- 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.
is successfully handled.
I think it's better to keep clearing code here. As spec, buffer should be cleared+ 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.)
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.
It's not specially described in the spec. This is the result of our discussion.+ } + + 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?
@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
[Prev in Thread] | Current Thread | [Next in Thread] |