Re: [Qemu-devel] [PATCH] hmp: add 'info virtio-pci-status id' command

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (address@hidden) wrote:
> On Tue, May 02, 2017 at 10:38:11AM +0800, ZhiPeng Lu wrote:
> > Add command to  query a virtio pci device status.
> > we can get id of the virtio pci device by 'info pci' command.
> > HMP Test case:
> >     ==============
> >     virsh # qemu-monitor-command --hmp 3 info pci
> >       Bus  0, device   3, function 0:
> >         Ethernet controller: PCI device 1af4:1000
> >           IRQ 11.
> >           BAR0: I/O at 0xc000 [0xc03f].
> >           BAR1: 32 bit memory at 0xfebd1000 [0xfebd1fff].
> >           BAR4: 64 bit prefetchable memory at 0xfe000000 [0xfe003fff].
> >           BAR6: 32 bit memory at 0xffffffffffffffff [0x0003fffe].
> >           id "net0"
> >       Bus  0, device   4, function 0:
> >         USB controller: PCI device 8086:24cd
> >           IRQ 11.
> >           BAR0: 32 bit memory at 0xfebd2000 [0xfebd2fff].
> >           id "usb"
> >     virsh # qemu-monitor-command 3  --hmp "info virtio-pci-status net0"
> >     status=15
> > 
> >     virsh # qemu-monitor-command 3  --hmp "info virtio-pci-status usb"
> >     the 'info virtio_pci_status' command only supports virtio pci devices
> 
> This information can be useful for troubleshooting virtio devices, but
> I'm concerned about adding new ad-hoc HMP commands.
> 
> Adding new commands for one specific field of device state lead to
> inconsistent user interfaces.  It adds a maintenance burden because all
> future versions of QEMU must continue to support this command for
> compatibility.
> 
> QMP should also be supported by new commands unless there is a reason to
> make the command HMP-only.
> 
> There is already a trace event called "virtio_set_status" so you can
> observe status changes from the host without adding a new monitor
> command.  Unfortunately tracing does not allow you to query the current
> value so it might not be possible to trigger the trace event at the
> appropriate time.
> 
> One option is to make the virtio status a QOM property so the existing
> qom-get command can be used to fetch the value without adding a new
> monitor command.

Unfortunately there is no HMP equivalent of qom-get, my previous attempts
at it didn't get anywhere.

> Does anyone have other ideas?

I think it should be at least at the level of 'info virtio-pci'.

Dave

> > Signed-off-by: ZhiPeng Lu <address@hidden>
> > ---
> >  hmp-commands-info.hx    | 14 ++++++++++++++
> >  hw/pci/pci-stub.c       |  6 ++++++
> >  hw/virtio/virtio-pci.c  | 47 
> > +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/sysemu/sysemu.h |  1 +
> >  4 files changed, 68 insertions(+)
> > 
> > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> > index a53f105..7b9c4db 100644
> > --- a/hmp-commands-info.hx
> > +++ b/hmp-commands-info.hx
> > @@ -815,6 +815,20 @@ ETEXI
> >          .cmd = hmp_info_vm_generation_id,
> >      },
> >  
> > +    {
> > +        .name       = "virtio-pci-status",
> > +        .args_type  = "id:s",
> > +        .params     = "id",
> > +        .help       = "show virtio pci device  status",
> > +        .cmd        = hmp_info_virtio_pci_status,
> > +    },
> > +
> > +STEXI
> > address@hidden info virtio-pci-status  @var{id}
> > address@hidden virtio-pci-status
> > +Show status about virtio pci device
> > +ETEXI
> > +
> >  STEXI
> >  @end table
> >  ETEXI
> > diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
> > index 36d2c43..3609a52 100644
> > --- a/hw/pci/pci-stub.c
> > +++ b/hw/pci/pci-stub.c
> > @@ -35,3 +35,9 @@ void hmp_pcie_aer_inject_error(Monitor *mon, const QDict 
> > *qdict)
> >  {
> >      monitor_printf(mon, "PCI devices not supported\n");
> >  }
> > +
> > +void hmp_info_virtio_pci_status(Monitor *mon, const QDict *qdict)
> > +{
> > +    monitor_printf(mon, "PCI devices not supported\n");
> > +}
> > +
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index f9b7244..5fa862b 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -37,6 +37,7 @@
> >  #include "qemu/range.h"
> >  #include "hw/virtio/virtio-bus.h"
> >  #include "qapi/visitor.h"
> > +#include "monitor/monitor.h"
> >  
> >  #define VIRTIO_PCI_REGION_SIZE(dev)     
> > VIRTIO_PCI_CONFIG_OFF(msix_present(dev))
> >  
> > @@ -50,6 +51,7 @@ static void virtio_pci_bus_new(VirtioBusState *bus, 
> > size_t bus_size,
> >                                 VirtIOPCIProxy *dev);
> >  static void virtio_pci_reset(DeviceState *qdev);
> >  
> > +static void query_virtio_pci_status(Monitor *mon, const char *id);
> >  /* virtio device */
> >  /* DeviceState to VirtIOPCIProxy. For use off data-path. TODO: use QOM. */
> >  static inline VirtIOPCIProxy *to_virtio_pci_proxy(DeviceState *d)
> > @@ -2556,6 +2558,51 @@ static void virtio_pci_bus_new(VirtioBusState *bus, 
> > size_t bus_size,
> >                          virtio_bus_name);
> >  }
> >  
> > +static void query_virtio_pci_status(Monitor *mon, const char *id)
> > +{
> > +    int ret = 0, i = 0;
> > +    PCIDevice *dev = NULL;
> > +    hwaddr addr = 0;
> > +    uint8_t val = 0;
> > +    const char *devtype = NULL;
> > +
> > +    ret = pci_qdev_find_device(id, &dev);
> > +    if (ret) {
> > +        monitor_printf(mon, "Can not find device %s\n", id);
> > +        return;
> > +    }
> > +    devtype = object_get_typename(OBJECT(dev));
> > +    if (strncmp("virtio-", devtype, 7) == 0) {
> > +        for (i = 0; i < PCI_NUM_REGIONS; i++) {
> > +            if (dev->io_regions[i].type == PCI_BASE_ADDRESS_SPACE_IO) {
> > +                addr = dev->io_regions[i].addr;
> > +                break;
> > +            }
> > +        }
> > +        if (addr != -1 && addr != 0) {
> > +            address_space_rw(&address_space_io, addr + VIRTIO_PCI_STATUS,
> > +            MEMTXATTRS_UNSPECIFIED, &val, 1, 0);
> > +            if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
> > +                fprintf(stderr, "driver is ok\n");
> > +            } else {
> > +                fprintf(stderr, "driver is not ok\n");
> > +            }
> > +            monitor_printf(mon, "status=%d", val);
> > +        } else {
> > +            monitor_printf(mon, "status=%d", val);
> > +        }
> > +    } else {
> > +        monitor_printf(mon, "the 'info virtio_pci_status' command "
> > +           "only supports virtio pci devices");
> > +    }
> > +}
> > +
> > +void hmp_info_virtio_pci_status(Monitor *mon, const QDict *qdict)
> > +{
> > +    const char *id = qdict_get_try_str(qdict, "id");
> > +    query_virtio_pci_status(mon, id);
> > +}
> > +
> >  static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
> >  {
> >      BusClass *bus_class = BUS_CLASS(klass);
> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > index 576c7ce..de66cc0 100644
> > --- a/include/sysemu/sysemu.h
> > +++ b/include/sysemu/sysemu.h
> > @@ -238,4 +238,5 @@ extern QemuOptsList qemu_net_opts;
> >  extern QemuOptsList qemu_global_opts;
> >  extern QemuOptsList qemu_mon_opts;
> >  
> > +extern void hmp_info_virtio_pci_status(Monitor *mon, const QDict *qdict);
> >  #endif
> > -- 
> > 1.8.3.1
> > 
> > 
> > 


--
Dr. David Alan Gilbert / address@hidden / Manchester, UK