Re: [PATCH RFC 1/1] msix: add hmp interface to dump MSI-X info

[Dongli Zhang]

On 4/23/21 12:59 AM, Jason Wang wrote:
> 
> 在 2021/4/23 下午12:47, Dongli Zhang 写道:
>> This patch is to add the HMP interface to dump MSI-X table and PBA, in
>> order to help diagnose the loss of IRQ issue in VM (e.g., if an MSI-X
>> vector is erroneously masked permanently). Here is the example with
>> vhost-scsi:
>>
>> (qemu) info msix /machine/peripheral/vscsi0
>> MSI-X Table
>> 0xfee01004 0x00000000 0x00000022 0x00000000
>> 0xfee02004 0x00000000 0x00000023 0x00000000
>> 0xfee01004 0x00000000 0x00000023 0x00000000
>> 0xfee01004 0x00000000 0x00000021 0x00000000
>> 0xfee02004 0x00000000 0x00000022 0x00000000
>> 0x00000000 0x00000000 0x00000000 0x00000001
>> 0x00000000 0x00000000 0x00000000 0x00000001
>> MSI-X PBA
>> 0 0 0 0 0 0 0
>>
>> Since the number of MSI-X entries is not determined and might be very
>> large, it is sometimes inappropriate to dump via QMP.
>>
>> Therefore, this patch dumps MSI-X information only via HMP, which is
>> similar to the implementation of hmp_info_mem().
> 
> 
> Besides PBA, I think it should be also useful to introduce device specifc
> callbacks for dump the MSI messages used by the device.
> 
> Thanks

Would you please help confirm if you meant MSI messages or MSI-X messages?

About about MSI-X, I assume we are able to derive the message from the MSI-X
table, as in msix_get_message().  Therefore, the user of "info msix <dev>"
should be able to parse the output and convert it to messages.

34 MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
35 {
36     uint8_t *table_entry = dev->msix_table + vector * PCI_MSIX_ENTRY_SIZE;
37     MSIMessage msg;
38
39     msg.address = pci_get_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR);
40     msg.data = pci_get_long(table_entry + PCI_MSIX_ENTRY_DATA);
41     return msg;
42 }


Perhaps I should remove the inner for-loop for MSI-X table in the patch, and
call pci_get_long() for 4 times, for PCI_MSIX_ENTRY_LOWER_ADDR,
PCI_MSIX_ENTRY_UPPER_ADDR, PCI_MSIX_ENTRY_DATA and PCI_MSIX_ENTRY_VECTOR_CTRL,
respectively.

Thank you very much!

Dongli Zhang

> 
> 
>>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: Joe Jin <joe.jin@oracle.com>
>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
>> ---
>>   hmp-commands-info.hx   | 13 +++++++++++
>>   hw/pci/msix.c          | 49 ++++++++++++++++++++++++++++++++++++++++++
>>   include/hw/pci/msix.h  |  2 ++
>>   include/monitor/hmp.h  |  1 +
>>   softmmu/qdev-monitor.c | 25 +++++++++++++++++++++
>>   5 files changed, 90 insertions(+)
>>
>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
>> index ab0c7aa5ee..cbd056442b 100644
>> --- a/hmp-commands-info.hx
>> +++ b/hmp-commands-info.hx
>> @@ -221,6 +221,19 @@ SRST
>>       Show PCI information.
>>   ERST
>>   +    {
>> +        .name       = "msix",
>> +        .args_type  = "dev:s",
>> +        .params     = "dev",
>> +        .help       = "dump MSI-X information",
>> +        .cmd        = hmp_info_msix,
>> +    },
>> +
>> +SRST
>> +  ``info msix`` *dev*
>> +    Dump MSI-X information for device *dev*.
>> +ERST
>> +
>>   #if defined(TARGET_I386) || defined(TARGET_SH4) || defined(TARGET_SPARC) 
>> || \
>>       defined(TARGET_PPC) || defined(TARGET_XTENSA) || defined(TARGET_M68K)
>>       {
>> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
>> index ae9331cd0b..a93d31da9f 100644
>> --- a/hw/pci/msix.c
>> +++ b/hw/pci/msix.c
>> @@ -22,6 +22,7 @@
>>   #include "sysemu/xen.h"
>>   #include "migration/qemu-file-types.h"
>>   #include "migration/vmstate.h"
>> +#include "monitor/monitor.h"
>>   #include "qemu/range.h"
>>   #include "qapi/error.h"
>>   #include "trace.h"
>> @@ -669,3 +670,51 @@ const VMStateDescription vmstate_msix = {
>>           VMSTATE_END_OF_LIST()
>>       }
>>   };
>> +
>> +static void msix_dump_table(Monitor *mon, PCIDevice *dev)
>> +{
>> +    int vector, i, offset;
>> +    uint32_t val;
>> +
>> +    monitor_printf(mon, "MSI-X Table\n");
>> +
>> +    for (vector = 0; vector < dev->msix_entries_nr; vector++) {
>> +        for (i = 0; i < 4; i++) {
>> +            offset = vector * PCI_MSIX_ENTRY_SIZE + i * 4;
>> +            val = pci_get_long(dev->msix_table + offset);
>> +
>> +            monitor_printf(mon, "0x%08x ", val);
>> +        }
>> +        monitor_printf(mon, "\n");
>> +    }
>> +}
>> +
>> +static void msix_dump_pba(Monitor *mon, PCIDevice *dev)
>> +{
>> +    int vector;
>> +
>> +    monitor_printf(mon, "MSI-X PBA\n");
>> +
>> +    for (vector = 0; vector < dev->msix_entries_nr; vector++) {
>> +        monitor_printf(mon, "%d ", !!msix_is_pending(dev, vector));
>> +
>> +        if (vector % 16 == 15) {
>> +            monitor_printf(mon, "\n");
>> +        }
>> +    }
>> +
>> +    if (vector % 16 != 15) {
>> +        monitor_printf(mon, "\n");
>> +    }
>> +}
>> +
>> +void msix_dump_info(Monitor *mon, PCIDevice *dev, Error **errp)
>> +{
>> +    if (!msix_present(dev)) {
>> +        error_setg(errp, "MSI-X not available");
>> +        return;
>> +    }
>> +
>> +    msix_dump_table(mon, dev);
>> +    msix_dump_pba(mon, dev);
>> +}
>> diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
>> index 4c4a60c739..10a4500295 100644
>> --- a/include/hw/pci/msix.h
>> +++ b/include/hw/pci/msix.h
>> @@ -47,6 +47,8 @@ int msix_set_vector_notifiers(PCIDevice *dev,
>>                                 MSIVectorPollNotifier poll_notifier);
>>   void msix_unset_vector_notifiers(PCIDevice *dev);
>>   +void msix_dump_info(Monitor *mon, PCIDevice *dev, Error **errp);
>> +
>>   extern const VMStateDescription vmstate_msix;
>>     #define VMSTATE_MSIX_TEST(_field, _state, _test) {                   \
>> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
>> index 605d57287a..46e0efc213 100644
>> --- a/include/monitor/hmp.h
>> +++ b/include/monitor/hmp.h
>> @@ -36,6 +36,7 @@ void hmp_info_irq(Monitor *mon, const QDict *qdict);
>>   void hmp_info_pic(Monitor *mon, const QDict *qdict);
>>   void hmp_info_rdma(Monitor *mon, const QDict *qdict);
>>   void hmp_info_pci(Monitor *mon, const QDict *qdict);
>> +void hmp_info_msix(Monitor *mon, const QDict *qdict);
>>   void hmp_info_tpm(Monitor *mon, const QDict *qdict);
>>   void hmp_info_iothreads(Monitor *mon, const QDict *qdict);
>>   void hmp_quit(Monitor *mon, const QDict *qdict);
>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>> index a9955b97a0..2a37d03fb7 100644
>> --- a/softmmu/qdev-monitor.c
>> +++ b/softmmu/qdev-monitor.c
>> @@ -19,6 +19,7 @@
>>     #include "qemu/osdep.h"
>>   #include "hw/sysbus.h"
>> +#include "hw/pci/msix.h"
>>   #include "monitor/hmp.h"
>>   #include "monitor/monitor.h"
>>   #include "monitor/qdev.h"
>> @@ -1006,3 +1007,27 @@ bool qmp_command_available(const QmpCommand *cmd, 
>> Error
>> **errp)
>>       }
>>       return true;
>>   }
>> +
>> +void hmp_info_msix(Monitor *mon, const QDict *qdict)
>> +{
>> +    const char *name = qdict_get_str(qdict, "dev");
>> +    DeviceState *dev = find_device_state(name, NULL);
>> +    PCIDevice *pci_dev;
>> +    Error *err = NULL;
>> +
>> +    if (!dev) {
>> +        error_setg(&err, "Device %s not found", name);
>> +        goto exit;
>> +    }
>> +
>> +    if (!object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>> +        error_setg(&err, "Not a PCI device");
>> +        goto exit;
>> +    }
>> +
>> +    pci_dev = PCI_DEVICE(dev);
>> +    msix_dump_info(mon, pci_dev, &err);
>> +
>> +exit:
>> +    hmp_handle_error(mon, err);
>> +}
>