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

[Jason Wang <jasowang@redhat.com>]

在 2021/4/24 上午1:32, 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?


E.g for virtio-pci, you need a way to know how the MSI-X vector is used 
by each virtqueue?


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


Something like this.

Thanks


>
> 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);
>>> +}