[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 4/9] {monitor, hw/pvrdma}: Expose device inte
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v3 4/9] {monitor, hw/pvrdma}: Expose device internals via monitor interface |
Date: |
Fri, 01 Mar 2019 08:17:02 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
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".
>> +
>> 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.
>> +
>> 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.
You could find these devices with object_child_foreach_recursive()
instead of maintaining a separate list.
>> 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?
> However you should include a generic rdma header as hw/rdma/rdma_hmp.h
> and not a vmw specific one.
>
>
> Thanks,
> Marcel
>
>
>> +}
>> +#endif
>> +
>> static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
>> {
>> const char *name = qdict_get_try_str(qdict, "name");
- Re: [Qemu-devel] [PATCH v3 4/9] {monitor, hw/pvrdma}: Expose device internals via monitor interface,
Markus Armbruster <=