Re: [Qemu-devel] [PATCH v3 4/9] {monitor, hw/pvrdma}: Expose device internals via monitor interface

[Yuval Shaia <yuval.shaia@oracle.com>]

On Fri, Mar 01, 2019 at 04:31:59PM +0100, Markus Armbruster wrote:
> Yuval Shaia <yuval.shaia@oracle.com> writes:
> 
> > On Fri, Mar 01, 2019 at 08:17:02AM +0100, Markus Armbruster wrote:
> >> Marcel Apfelbaum <marcel.apfelbaum@gmail.com> 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 <yuval.shaia@oracle.com>
> >> >> ---
> >> >>   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
> >> >> +@item info pvrdmacounters
> >> >> +@findex 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?
> 
> Yes.  It's just an observation for use in a later comment.
> 
> >> 
> >> >> +
> >> >>   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.
> 
> I'm referring to the update of @devices, not the out: label.
> 
> >> 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.
> 
> Examples include hmp_info_irq() and hmp_info_pic().

Hi Markus,
Can you take a look at my v4 to see if is what you meant?

Thanks,
Yuval

> 
> >> >> 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.
> 
> Marcel's idea to fix compilation with a stub is spot on.  But should it
> print a message?  I don't think so.
> 
> >> 
> >> > 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");
> >> 
>