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

[Cornelia Huck]

On Wed, 24 Oct 2018 11:58:33 +0800
Yi Min Zhao <address@hidden> wrote:

> 在 2018/10/24 上午5:25, Cornelia Huck 写道:
> > On Mon, 22 Oct 2018 13:17:34 +0100
> > Thomas Huth <address@hidden> wrote:
> >  
> >> On 2018-10-22 10:02, 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.
> >>>
> >>> Signed-off-by: Yi Min Zhao <address@hidden>
> >>> Reviewed-by: Pierre Morel <address@hidden>
> >>> ---  
> >> [...]  
> >>> +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);
> >>> +}  
> >> Sorry for noticing this in v1 already, but is this code endianess-safe?
> >> I.e. can this also work with qemu-system-s390x running with TCG on a x86
> >> host? I think you might have to use something like this here instead:
> >>
> >>    pbdev->fmb.sample = cpu_to_be32(be32_to_cpu(pbdev->fmb.sample) + 1);
> >>
> >> etc.  
> > Agreed, that may need some endianness handling.
> >
> > I would test this with tcg on a LE host, but how can I verify this? Yi
> > Min, do you have some kind of test tooling you can share?
> >
> >  
> There's no tool now. You could startup a guest. And then in the guest, 
> install
> PCI driver and read FMB values from /sys/kernel/debug/pci/****/statistics.
> 
> If endianness has error, I think the values must looks wrong.
> The right thing is that values increase from 0 and intervally.
> 

Thanks for pointing me to that file; when I run under tcg, the values
indeed look like they have an endianness issue:

Update interval: 4000 ms
Samples: 637534208
Last update TOD: f4c01d0098000000
           Load operations:     10520408729537478656
          Store operations:     5980780305148018688
    Store block operations:     0
        Refresh operations:     0
           Allocated pages:     0
              Mapped pages:     0
            Unmapped pages:     0

(virtio-net-pci device on a just-booted guest)