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

[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 <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-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(&address_space_memory,
>> +                              pbdev->fmb_addr + (uint64_t)offset,

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

>> +                              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;
>> +    }
[...]

 Cheers,
  Thomas