On Fri, 14 Dec 2018 17:53:42 +0100
Pierre Morel <pmo...@linux.ibm.com> wrote:
> From: Yi Min Zhao <zyi...@linux.ibm.com>
>
> Common function measurement block is used to report zPCI internal
> counters of successful pcilg/stg/stb and rpcit instructions to
> a memory location provided by the program.
>
> This patch introduces a new ZpciFmb structure and schedules a timer
> callback to copy the zPCI measures to the FMB in the guest memory
> at an interval time set to 4s by default.
Hm, is there any way to change the interval? If not, just drop the "by
default"?
>
> An error while attemping to update the FMB, would generated an error
s/generated/generate/
> event to the guest.
>
> The pcilg/stg/stb and rpcit interception handlers issue, increase
> the related counter on success.
"When the ... handlers are called, ..." ?
> The guest shall pass a null FMBA (FMB address) in the FIB (Function
> Information Block) when it issues a Modify PCI Function Control
> instrcuction to switch off FMB and stop the corresponding timer.
>
> Signed-off-by: Yi Min Zhao <zyi...@linux.ibm.com>
> Signed-off-by: Pierre Morel <pmo...@linux.ibm.com>
> ---
> hw/s390x/s390-pci-bus.c | 4 +-
> hw/s390x/s390-pci-bus.h | 29 ++++++++++
> hw/s390x/s390-pci-inst.c | 141
> ++++++++++++++++++++++++++++++++++++++++++++++-
> hw/s390x/s390-pci-inst.h | 1 +
> 4 files changed, 171 insertions(+), 4 deletions(-)
>
> +static int fmb_do_update64(S390PCIBusDevice *pbdev, int offset, int cnt)
> +{
> + MemTxResult ret = MEMTX_OK;
> + uint64_t dst = pbdev->fmb_addr + offset;
> + uint64_t *src = (uint64_t *) ((unsigned long)(&pbdev->fmb) + offset);
> + int i;
> +
> + for (i = 0; i < cnt; i++, dst += 8, src++) {
> + address_space_stq_be(&address_space_memory, dst, *src,
> + MEMTXATTRS_UNSPECIFIED, &ret);
> + if (ret != MEMTX_OK) {
> + s390_pci_generate_error_event(ERR_EVENT_FMBA, pbdev->fh,
> pbdev->fid,
> + pbdev->fmb_addr, 0);
> + fmb_timer_free(pbdev);
> + return ret;
> + }
> + }
> + return ret;
> +}
> +
> +static int fmb_do_update(S390PCIBusDevice *pbdev, int offset, int val, int
> len)
> +{
> + MemTxResult ret;
> + uint64_t dst = pbdev->fmb_addr + offset;
> +
> + switch (len) {
> + case 4:
> + address_space_stl_be(&address_space_memory, dst, val,
> + MEMTXATTRS_UNSPECIFIED,
> + &ret);
> + break;
> + case 2:
> + address_space_stw_be(&address_space_memory, dst, val,
> + MEMTXATTRS_UNSPECIFIED,
> + &ret);
> + break;
> + case 1:
> + address_space_stb(&address_space_memory, dst, val,
> + MEMTXATTRS_UNSPECIFIED,
> + &ret);
> + break;
> + default:
> + ret = MEMTX_ERROR;
> + break;
> + }
> + if (ret != MEMTX_OK) {
> + 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;
> +
> + /* Update U bit */
> + pbdev->fmb.last_update |= UPDATE_U_BIT;
> + offset = offsetof(ZpciFmb, last_update);
> + if (fmb_do_update64(pbdev, offset, 1)) {
> + return;
> + }
> +
> + /* Update FMB sample count */
> + offset = offsetof(ZpciFmb, sample);
> + if (fmb_do_update(pbdev, offset, pbdev->fmb.sample++,
> + sizeof(pbdev->fmb.sample))) {
This is the only caller of fmb_do_update(), right? Any chance that a
new format of the block would introduce new callers?
> + return;
> + }
> + /* Update FMB counters */
> + offset = offsetof(ZpciFmb, counter);
> + if (fmb_do_update64(pbdev, offset, ZPCI_FMB_CNT_MAX)) {
> + return;
> + }
> +
> + /* Clear U bit and update the time */
> + pbdev->fmb.last_update = time2tod(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> + pbdev->fmb.last_update <<= 1;
> + offset = offsetof(ZpciFmb, last_update);
> + if (fmb_do_update64(pbdev, offset, 1)) {
> + return;
> + }
Hm, one thing that I don't quite like about the update code is the odd
split between fmb_do_update() (which always updates one value) and
fmb_do_update64() (which may update multiple values).
What does the code look like if you:
- have a fmb_do_update() that can also handle 64bit values,
- have the update of the counters loop and break out if you get an
error?
Of course, you may have already tried that ;) If it looks ugly, I don't
have a real issue with this code, either.
> +
> + 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)
> {
