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

[Stefan Hajnoczi]

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.

Does anyone have other ideas?

> 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
> 
> 
>

signature.asc
Description: PGP signature