[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [qemu-s390x] [Qemu-devel] [PATCH] s390x/pci: add common fmb
|
From: |
Yi Min Zhao |
|
Subject: |
Re: [qemu-s390x] [Qemu-devel] [PATCH] s390x/pci: add common fmb |
|
Date: |
Sat, 29 Sep 2018 13:48:50 +0800 |
|
User-agent: |
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 |
在 2018/9/19 下午3:53, 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.
Currently we only implement FMT0. There're other FMTs to be implemented
in future.
So here there would be a union and we can't give a specific size to
QEMU_BUILD_BUG_MSG.
Can we use the max size for checking?
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
Ah...I didn't test the code with gcc 8+. GCC I used is 7.2.
It should get the same warining.
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).
I want to copy the entire structure to the guest memory during updating FMB.
It's not a good idea to do copy for all members multiple times.
As your comment, I think we have to allocate memory for ZpciFmb.
+ 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...
Yes.
+{
+ MemTxResult ret;
+
+ ret = address_space_write(&address_space_memory,
+ pbdev->fmb_addr + (uint64_t)offset,
... then you can also remove this (uint64_t) cast.
Right.
+ 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
Thank you very much!
--
Yi Min