On Fri, Mar 01, 2019 at 08:17:02AM +0100, Markus Armbruster wrote:
Marcel Apfelbaum <address@hidden> writes:
Hi Yuval,
On 2/27/19 4:06 PM, Yuval Shaia wrote:
Allow interrogating device internals through HMP interface.
The exposed indicators can be used for troubleshooting by developers or
sysadmin.
There is no need to expose these attributes to a management system (e.x.
libvirt) because (1) most of them are not "device-management' related
info and (2) there is no guarantee the interface is stable.
Signed-off-by: Yuval Shaia <address@hidden>
---
hmp-commands-info.hx | 16 ++++++++
hw/rdma/rdma_backend.c | 70 ++++++++++++++++++++++++++---------
hw/rdma/rdma_rm.c | 7 ++++
hw/rdma/rdma_rm_defs.h | 27 +++++++++++++-
hw/rdma/vmw/pvrdma.h | 5 +++
hw/rdma/vmw/pvrdma_hmp.h | 21 +++++++++++
hw/rdma/vmw/pvrdma_main.c | 77 +++++++++++++++++++++++++++++++++++++++
monitor.c | 10 +++++
8 files changed, 215 insertions(+), 18 deletions(-)
create mode 100644 hw/rdma/vmw/pvrdma_hmp.h
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index cbee8b944d..9153c33974 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -524,6 +524,22 @@ STEXI
Show CPU statistics.
ETEXI
+#if defined(CONFIG_PVRDMA)
+ {
+ .name = "pvrdmacounters",
+ .args_type = "",
+ .params = "",
+ .help = "show pvrdma device counters",
+ .cmd = hmp_info_pvrdmacounters,
+ },
+
+STEXI
address@hidden info pvrdmacounters
address@hidden info pvrdmacounters
+Show pvrdma device counters.
+ETEXI
+#endif
+
#if defined(CONFIG_SLIRP)
{
.name = "usernet",
[...]
diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index b6061f4b6e..85101368c5 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -14,6 +14,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/units.h"
#include "qapi/error.h"
#include "hw/hw.h"
#include "hw/pci/pci.h"
@@ -25,6 +26,7 @@
#include "cpu.h"
#include "trace.h"
#include "sysemu/sysemu.h"
+#include "monitor/monitor.h"
#include "../rdma_rm.h"
#include "../rdma_backend.h"
@@ -32,10 +34,13 @@
#include <infiniband/verbs.h>
#include "pvrdma.h"
+#include "pvrdma_hmp.h"
#include "standard-headers/rdma/vmw_pvrdma-abi.h"
#include "standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h"
#include "pvrdma_qp_ops.h"
+GSList *devices;
"devices" is far too generic for an external identifier. Are you
missing a 'static' here? Even if static, I'd recommend "rdma_devices".
Yep, thanks.
+
static Property pvrdma_dev_properties[] = {
DEFINE_PROP_STRING("netdev", PVRDMADev, backend_eth_device_name),
DEFINE_PROP_STRING("ibdev", PVRDMADev, backend_device_name),
@@ -55,6 +60,71 @@ static Property pvrdma_dev_properties[] = {
[...]
+}
+
+void pvrdma_dump_counters(Monitor *mon)
+{
+ g_slist_foreach(devices, pvrdma_dump_device_counters, mon);
+}
Note for later: does nothing when @devices is empty.
But that is fine, isn't it?
+
static void free_dev_ring(PCIDevice *pci_dev, PvrdmaRing *ring,
void *ring_state)
{
@@ -304,6 +374,8 @@ static void pvrdma_fini(PCIDevice *pdev)
rdma_info_report("Device %s %x.%x is down", pdev->name,
PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+
+ devices = g_slist_remove(devices, pdev);
}
static void pvrdma_stop(PVRDMADev *dev)
@@ -394,6 +466,7 @@ static void pvrdma_regs_write(void *opaque, hwaddr addr,
uint64_t val,
if (val == 0) {
trace_pvrdma_regs_write(addr, val, "REQUEST", "");
pvrdma_exec_cmd(dev);
+ dev->stats.commands++;
}
break;
default:
@@ -612,9 +685,13 @@ static void pvrdma_realize(PCIDevice *pdev, Error **errp)
goto out;
}
+ memset(&dev->stats, 0, sizeof(dev->stats));
+
dev->shutdown_notifier.notify = pvrdma_shutdown_notifier;
qemu_register_shutdown_notifier(&dev->shutdown_notifier);
+ devices = g_slist_append(devices, pdev);
+
out:
if (rc) {
pvrdma_fini(pdev);
Note for later: @devices contains the realized "pvrdma" devices.
This happens only when rc indicates an error and we jumped here before
adding the device to the list.
You could find these devices with object_child_foreach_recursive()
instead of maintaining a separate list.
Hmmm...interesting.
I will check if it fits my needs and will change accordingly if yes.
diff --git a/monitor.c b/monitor.c
index defa129319..53ecb5bc70 100644
--- a/monitor.c
+++ b/monitor.c
@@ -85,6 +85,9 @@
#include "sysemu/iothread.h"
#include "qemu/cutils.h"
#include "tcg/tcg.h"
+#ifdef CONFIG_PVRDMA
+#include "hw/rdma/vmw/pvrdma_hmp.h"
+#endif
#if defined(TARGET_S390X)
#include "hw/s390x/storage-keys.h"
@@ -1362,6 +1365,13 @@ static void hmp_info_cpustats(Monitor *mon, const QDict
*qdict)
cpu_dump_statistics(cs, (FILE *)mon, &monitor_fprintf, 0);
}
+#ifdef CONFIG_PVRDMA
+static void hmp_info_pvrdmacounters(Monitor *mon, const QDict *qdict)
+{
+ pvrdma_dump_counters(mon);
Compilation fails on archs with no PCI support:
/usr/bin/ld: monitor.o: in function `hmp_info_pvrdmacounters':
/home/marcel/git/qemu/monitor.c:1371: undefined reference to
`pvrdma_dump_counters'
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:210: qemu-system-m68k] Error 1
The below patch solves it by adding a pci stub:
diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
index b941a0e842..cab364f93d 100644
--- a/hw/pci/pci-stub.c
+++ b/hw/pci/pci-stub.c
@@ -26,6 +26,7 @@
#include "qapi/qmp/qerror.h"
#include "hw/pci/pci.h"
#include "hw/pci/msi.h"
+#include "hw/rdma/vmw/pvrdma_hmp.h"
bool msi_nonbroken;
bool pci_available;
@@ -53,3 +54,9 @@ uint16_t pci_requester_id(PCIDevice *dev)
g_assert(false);
return 0;
}
+
+void pvrdma_dump_counters(Monitor *mon)
+{
+ monitor_printf(mon, "PVRDMA requires PCI support\n");
+}
+
When CONFIG_PCI is enabled, "info pvrdmacounters" does nothing when
there are no "pvrdma" devices.
When CONFIG_PCI is disabled, there are no "pvrdma" devices. Therefore,
"info pvrdmacounters" should also do nothing then, shouldn't it?
Yeah, problem was in case that pvrdma was selected in ./configure phase but
platform does not have PCI -> CONFIG_PCI is diabeled -> pvrdma code is not
compiled -> pvrdma_dump_counters is missing in link phase.
If you have a better alternative then i'm fine, meanwhile i'm taking
Marcel's proposal.