[PATCH RFC 0/1] To add HMP interface to dump PCI MSI-X table/PBA

[PATCH RFC 0/1] To add HMP interface to dump PCI MSI-X table/PBA

[Dongli Zhang]

This is inspired by the discussion with Jason on below patchset.

https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg09020.html

The new HMP command is introduced to dump the MSI-X table and PBA.

Initially, I was going to add new option to "info pci". However, as the
number of entries is not determined and the output of MSI-X table is much
more similar to the output of hmp_info_tlb()/hmp_info_mem(), this patch
adds interface for only HMP.

The patch is tagged with RFC because I am looking for suggestions on:

1. Is it fine to add new "info msix <dev>" command?

2. Is there any issue with output format?

3. Is it fine to add only for HMP, but not QMP?

Thank you very much!

Dongli Zhang

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

[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().

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

Re: [PATCH RFC 0/1] To add HMP interface to dump PCI MSI-X table/PBA

[Jason Wang]

在 2021/4/23 下午12:47, Dongli Zhang 写道:
> This is inspired by the discussion with Jason on below patchset.
>
> https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg09020.html
>
> The new HMP command is introduced to dump the MSI-X table and PBA.
>
> Initially, I was going to add new option to "info pci". However, as the
> number of entries is not determined and the output of MSI-X table is much
> more similar to the output of hmp_info_tlb()/hmp_info_mem(), this patch
> adds interface for only HMP.
>
> The patch is tagged with RFC because I am looking for suggestions on:
>
> 1. Is it fine to add new "info msix <dev>" command?


I wonder the reason for not simply reusing "info pci"?


>
> 2. Is there any issue with output format?


If it's not for QMP, I guess it's not a part of ABI so it should be fine.


>
> 3. Is it fine to add only for HMP, but not QMP?


I think so.

Thanks


>
> Thank you very much!
>
> Dongli Zhang
>
>
>

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

[Jason Wang]

在 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


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

Re: [PATCH RFC 0/1] To add HMP interface to dump PCI MSI-X table/PBA

[Dongli Zhang]

On 4/22/21 11:01 PM, Jason Wang wrote:
> 
> 在 2021/4/23 下午12:47, Dongli Zhang 写道:
>> This is inspired by the discussion with Jason on below patchset.
>>
>> https://urldefense.com/v3/__https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg09020.html__;!!GqivPVa7Brio!KbGQZW5lq3JZ60k12NuWZ6Th1lT6AwmBTF0pBgoWUKKQ4-2UhdW57PtvXUN5XQnZ2NU$
>>
>> The new HMP command is introduced to dump the MSI-X table and PBA.
>>
>> Initially, I was going to add new option to "info pci". However, as the
>> number of entries is not determined and the output of MSI-X table is much
>> more similar to the output of hmp_info_tlb()/hmp_info_mem(), this patch
>> adds interface for only HMP.
>>
>> The patch is tagged with RFC because I am looking for suggestions on:
>>
>> 1. Is it fine to add new "info msix <dev>" command?
> 
> 
> I wonder the reason for not simply reusing "info pci"?

The "info pci" will show PCI data for all devices and it does not accept any
argument to print for a specific device.

In addition, the "info pci" relies on qmp_query_pci(), where this patch will not
implement the interface for QMP considering the number of MSI-X entries is not
determined.

Suppose we have 10 NVMe (emulated by QEMU with default number of queues), we
will have about 600+ lines of output.

Dongli Zhang

> 
> 
>>
>> 2. Is there any issue with output format?
> 
> 
> If it's not for QMP, I guess it's not a part of ABI so it should be fine.
> 
> 
>>
>> 3. Is it fine to add only for HMP, but not QMP?
> 
> 
> I think so.
> 
> Thanks
> 
> 
>>
>> Thank you very much!
>>
>> Dongli Zhang
>>
>>
>>
> 

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

Re: [PATCH RFC 0/1] To add HMP interface to dump PCI MSI-X table/PBA

[Jason Wang]

在 2021/4/24 上午1:26, Dongli Zhang 写道:
>
> On 4/22/21 11:01 PM, Jason Wang wrote:
>> 在 2021/4/23 下午12:47, Dongli Zhang 写道:
>>> This is inspired by the discussion with Jason on below patchset.
>>>
>>> https://urldefense.com/v3/__https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg09020.html__;!!GqivPVa7Brio!KbGQZW5lq3JZ60k12NuWZ6Th1lT6AwmBTF0pBgoWUKKQ4-2UhdW57PtvXUN5XQnZ2NU$
>>>
>>> The new HMP command is introduced to dump the MSI-X table and PBA.
>>>
>>> Initially, I was going to add new option to "info pci". However, as the
>>> number of entries is not determined and the output of MSI-X table is much
>>> more similar to the output of hmp_info_tlb()/hmp_info_mem(), this patch
>>> adds interface for only HMP.
>>>
>>> The patch is tagged with RFC because I am looking for suggestions on:
>>>
>>> 1. Is it fine to add new "info msix <dev>" command?
>>
>> I wonder the reason for not simply reusing "info pci"?
> The "info pci" will show PCI data for all devices and it does not accept any
> argument to print for a specific device.
>
> In addition, the "info pci" relies on qmp_query_pci(), where this patch will not
> implement the interface for QMP considering the number of MSI-X entries is not
> determined.
>
> Suppose we have 10 NVMe (emulated by QEMU with default number of queues), we
> will have about 600+ lines of output.


I see.

Thanks


>
> Dongli Zhang
>
>>
>>> 2. Is there any issue with output format?
>>
>> If it's not for QMP, I guess it's not a part of ABI so it should be fine.
>>
>>
>>> 3. Is it fine to add only for HMP, but not QMP?
>>
>> I think so.
>>
>> Thanks
>>
>>
>>> Thank you very much!
>>>
>>> Dongli Zhang
>>>
>>>
>>>

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

[Jason Wang]

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

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

[Dongli Zhang]

On 4/24/21 8:36 PM, Jason Wang wrote:
> 
> 在 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.

I prefer to print the raw data as I think the user of this interface is able to
understand it as MSI-X messages.

For instance, below is the data printed by "info msix".

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

The 1st column is Message Lower Address.

The 2nd column is Message Upper Address.

The 3rd column is Message Data.

The 4th column is Vector Control.


In order to verify whether a vector is masked, usually I would just check the
bit 0 of Vector Control.

Therefore, I think we can assume the user understands the MSI-X table format
well and we just prints the MSI-X table memory data.

Thank you very much!

Dongli Zhang

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

Re: [PATCH RFC 0/1] To add HMP interface to dump PCI MSI-X table/PBA

[Dr. David Alan Gilbert]

* Dongli Zhang (dongli.zhang@oracle.com) wrote:
> 
> 
> On 4/22/21 11:01 PM, Jason Wang wrote:
> > 
> > 在 2021/4/23 下午12:47, Dongli Zhang 写道:
> >> This is inspired by the discussion with Jason on below patchset.
> >>
> >> https://urldefense.com/v3/__https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg09020.html__;!!GqivPVa7Brio!KbGQZW5lq3JZ60k12NuWZ6Th1lT6AwmBTF0pBgoWUKKQ4-2UhdW57PtvXUN5XQnZ2NU$
> >>
> >> The new HMP command is introduced to dump the MSI-X table and PBA.
> >>
> >> Initially, I was going to add new option to "info pci". However, as the
> >> number of entries is not determined and the output of MSI-X table is much
> >> more similar to the output of hmp_info_tlb()/hmp_info_mem(), this patch
> >> adds interface for only HMP.
> >>
> >> The patch is tagged with RFC because I am looking for suggestions on:
> >>
> >> 1. Is it fine to add new "info msix <dev>" command?
> > 
> > 
> > I wonder the reason for not simply reusing "info pci"?
> 
> The "info pci" will show PCI data for all devices and it does not accept any
> argument to print for a specific device.
> 
> In addition, the "info pci" relies on qmp_query_pci(), where this patch will not
> implement the interface for QMP considering the number of MSI-X entries is not
> determined.
> 
> Suppose we have 10 NVMe (emulated by QEMU with default number of queues), we
> will have about 600+ lines of output.

From an HMP perspective I'm happy, so:

Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

but since I don't know much about MSI I'd like to see Jason's reply.

Adding an optional option to 'info pci' to limit to one device would be easy
though; that bit is probably easier than adding a new command.
Figuring out the QMP representation of your entries might be harder -
and if this is strictly for debug, probably not worth it?

Dave


> Dongli Zhang
> 
> > 
> > 
> >>
> >> 2. Is there any issue with output format?
> > 
> > 
> > If it's not for QMP, I guess it's not a part of ABI so it should be fine.
> > 
> > 
> >>
> >> 3. Is it fine to add only for HMP, but not QMP?
> > 
> > 
> > I think so.
> > 
> > Thanks
> > 
> > 
> >>
> >> Thank you very much!
> >>
> >> Dongli Zhang
> >>
> >>
> >>
> > 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH RFC 0/1] To add HMP interface to dump PCI MSI-X table/PBA

[Jason Wang]

在 2021/4/27 下午4:53, Dr. David Alan Gilbert 写道:
> * Dongli Zhang (dongli.zhang@oracle.com) wrote:
>>
>> On 4/22/21 11:01 PM, Jason Wang wrote:
>>> 在 2021/4/23 下午12:47, Dongli Zhang 写道:
>>>> This is inspired by the discussion with Jason on below patchset.
>>>>
>>>> https://urldefense.com/v3/__https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg09020.html__;!!GqivPVa7Brio!KbGQZW5lq3JZ60k12NuWZ6Th1lT6AwmBTF0pBgoWUKKQ4-2UhdW57PtvXUN5XQnZ2NU$
>>>>
>>>> The new HMP command is introduced to dump the MSI-X table and PBA.
>>>>
>>>> Initially, I was going to add new option to "info pci". However, as the
>>>> number of entries is not determined and the output of MSI-X table is much
>>>> more similar to the output of hmp_info_tlb()/hmp_info_mem(), this patch
>>>> adds interface for only HMP.
>>>>
>>>> The patch is tagged with RFC because I am looking for suggestions on:
>>>>
>>>> 1. Is it fine to add new "info msix <dev>" command?
>>>
>>> I wonder the reason for not simply reusing "info pci"?
>> The "info pci" will show PCI data for all devices and it does not accept any
>> argument to print for a specific device.
>>
>> In addition, the "info pci" relies on qmp_query_pci(), where this patch will not
>> implement the interface for QMP considering the number of MSI-X entries is not
>> determined.
>>
>> Suppose we have 10 NVMe (emulated by QEMU with default number of queues), we
>> will have about 600+ lines of output.
>  From an HMP perspective I'm happy, so:
>
> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> but since I don't know much about MSI I'd like to see Jason's reply.


I think we'd better have more information, e.g the device can optionally 
report how the MSI-X vector is used.

Virtio-pci could be the first user for this.


>
> Adding an optional option to 'info pci' to limit to one device would be easy
> though; that bit is probably easier than adding a new command.


One interesting point is that MSI could be extended for other bus, (e.g 
MMIO). So "info msi" should be better I guess.


> Figuring out the QMP representation of your entries might be harder -
> and if this is strictly for debug, probably not worth it?


I think so.

Thanks


>
> Dave
>
>
>> Dongli Zhang
>>
>>>
>>>> 2. Is there any issue with output format?
>>>
>>> If it's not for QMP, I guess it's not a part of ABI so it should be fine.
>>>
>>>
>>>> 3. Is it fine to add only for HMP, but not QMP?
>>>
>>> I think so.
>>>
>>> Thanks
>>>
>>>
>>>> Thank you very much!
>>>>
>>>> Dongli Zhang
>>>>
>>>>
>>>>

Re: [PATCH RFC 0/1] To add HMP interface to dump PCI MSI-X table/PBA

[Dongli Zhang]

Hi Jason,

On 4/27/21 7:31 PM, Jason Wang wrote:
> 
> 在 2021/4/27 下午4:53, Dr. David Alan Gilbert 写道:
>> * Dongli Zhang (dongli.zhang@oracle.com) wrote:
>>>
>>> On 4/22/21 11:01 PM, Jason Wang wrote:
>>>> 在 2021/4/23 下午12:47, Dongli Zhang 写道:
>>>>> This is inspired by the discussion with Jason on below patchset.
>>>>>
>>>>> https://urldefense.com/v3/__https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg09020.html__;!!GqivPVa7Brio!KbGQZW5lq3JZ60k12NuWZ6Th1lT6AwmBTF0pBgoWUKKQ4-2UhdW57PtvXUN5XQnZ2NU$
>>>>>
>>>>>
>>>>> The new HMP command is introduced to dump the MSI-X table and PBA.
>>>>>
>>>>> Initially, I was going to add new option to "info pci". However, as the
>>>>> number of entries is not determined and the output of MSI-X table is much
>>>>> more similar to the output of hmp_info_tlb()/hmp_info_mem(), this patch
>>>>> adds interface for only HMP.
>>>>>
>>>>> The patch is tagged with RFC because I am looking for suggestions on:
>>>>>
>>>>> 1. Is it fine to add new "info msix <dev>" command?
>>>>
>>>> I wonder the reason for not simply reusing "info pci"?
>>> The "info pci" will show PCI data for all devices and it does not accept any
>>> argument to print for a specific device.
>>>
>>> In addition, the "info pci" relies on qmp_query_pci(), where this patch will not
>>> implement the interface for QMP considering the number of MSI-X entries is not
>>> determined.
>>>
>>> Suppose we have 10 NVMe (emulated by QEMU with default number of queues), we
>>> will have about 600+ lines of output.
>>  From an HMP perspective I'm happy, so:
>>
>> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>
>> but since I don't know much about MSI I'd like to see Jason's reply.
> 
> 
> I think we'd better have more information, e.g the device can optionally report
> how the MSI-X vector is used.
> 
> Virtio-pci could be the first user for this.

As discussed in another thread, you were talking about to print MSIMessage.

However, I prefer to print the raw data as I think the user of this interface
should be able to understand it as MSI-X messages.

For instance, below is the data printed by "info msix".

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

The 1st column is Message Lower Address.

The 2nd column is Message Upper Address.

The 3rd column is Message Data.

The 4th column is Vector Control.

In my opinion, this is equivalent to MSIMessage.

26 struct MSIMessage {
27     uint64_t address; --> column 1 and 2
28     uint32_t data;    --> column 3
29 };


We use the similar way to read from Linux OS, e,g., given the address of MSI-X
cap, here is how we read from OS side.

# busybox devmem 0xc1001000 32
0xFEE00000
# busybox devmem 0xc1001004 32
0x00000000
# busybox devmem 0xc1001008 32
0x00004049
# busybox devmem 0xc100100c 32
0x00000000

Thank you very much!

Dongli Zhang

> 
> 
>>
>> Adding an optional option to 'info pci' to limit to one device would be easy
>> though; that bit is probably easier than adding a new command.
> 
> 
> One interesting point is that MSI could be extended for other bus, (e.g MMIO).
> So "info msi" should be better I guess.
> 
> 
>> Figuring out the QMP representation of your entries might be harder -
>> and if this is strictly for debug, probably not worth it?
> 
> 
> I think so.
> 
> Thanks
> 
> 
>>
>> Dave
>>
>>
>>> Dongli Zhang
>>>
>>>>
>>>>> 2. Is there any issue with output format?
>>>>
>>>> If it's not for QMP, I guess it's not a part of ABI so it should be fine.
>>>>
>>>>
>>>>> 3. Is it fine to add only for HMP, but not QMP?
>>>>
>>>> I think so.
>>>>
>>>> Thanks
>>>>
>>>>
>>>>> Thank you very much!
>>>>>
>>>>> Dongli Zhang
>>>>>
>>>>>
>>>>>
> 

Re: [PATCH RFC 0/1] To add HMP interface to dump PCI MSI-X table/PBA

[Jason Wang]

在 2021/4/28 下午1:10, Dongli Zhang 写道:
> Hi Jason,
>
> On 4/27/21 7:31 PM, Jason Wang wrote:
>> 在 2021/4/27 下午4:53, Dr. David Alan Gilbert 写道:
>>> * Dongli Zhang (dongli.zhang@oracle.com) wrote:
>>>> On 4/22/21 11:01 PM, Jason Wang wrote:
>>>>> 在 2021/4/23 下午12:47, Dongli Zhang 写道:
>>>>>> This is inspired by the discussion with Jason on below patchset.
>>>>>>
>>>>>> https://urldefense.com/v3/__https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg09020.html__;!!GqivPVa7Brio!KbGQZW5lq3JZ60k12NuWZ6Th1lT6AwmBTF0pBgoWUKKQ4-2UhdW57PtvXUN5XQnZ2NU$
>>>>>>
>>>>>>
>>>>>> The new HMP command is introduced to dump the MSI-X table and PBA.
>>>>>>
>>>>>> Initially, I was going to add new option to "info pci". However, as the
>>>>>> number of entries is not determined and the output of MSI-X table is much
>>>>>> more similar to the output of hmp_info_tlb()/hmp_info_mem(), this patch
>>>>>> adds interface for only HMP.
>>>>>>
>>>>>> The patch is tagged with RFC because I am looking for suggestions on:
>>>>>>
>>>>>> 1. Is it fine to add new "info msix <dev>" command?
>>>>> I wonder the reason for not simply reusing "info pci"?
>>>> The "info pci" will show PCI data for all devices and it does not accept any
>>>> argument to print for a specific device.
>>>>
>>>> In addition, the "info pci" relies on qmp_query_pci(), where this patch will not
>>>> implement the interface for QMP considering the number of MSI-X entries is not
>>>> determined.
>>>>
>>>> Suppose we have 10 NVMe (emulated by QEMU with default number of queues), we
>>>> will have about 600+ lines of output.
>>>   From an HMP perspective I'm happy, so:
>>>
>>> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>
>>> but since I don't know much about MSI I'd like to see Jason's reply.
>>
>> I think we'd better have more information, e.g the device can optionally report
>> how the MSI-X vector is used.
>>
>> Virtio-pci could be the first user for this.
> As discussed in another thread, you were talking about to print MSIMessage.
>
> However, I prefer to print the raw data as I think the user of this interface
> should be able to understand it as MSI-X messages.
>
> For instance, below is the data printed by "info msix".


Just to clarify, I meant e.g for virtio-pci device, we can let it to 
print the mapping between vq and msix vectors:

vq[0].msix_vector = 0
vq[1].msix_vector = 1
config.msix_vector = 2
...

But this could be added on top if you wish.

Does this make sense?

Thanks


>
> 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
>
> The 1st column is Message Lower Address.
>
> The 2nd column is Message Upper Address.
>
> The 3rd column is Message Data.
>
> The 4th column is Vector Control.
>
> In my opinion, this is equivalent to MSIMessage.
>
> 26 struct MSIMessage {
> 27     uint64_t address; --> column 1 and 2
> 28     uint32_t data;    --> column 3
> 29 };
>
>
> We use the similar way to read from Linux OS, e,g., given the address of MSI-X
> cap, here is how we read from OS side.
>
> # busybox devmem 0xc1001000 32
> 0xFEE00000
> # busybox devmem 0xc1001004 32
> 0x00000000
> # busybox devmem 0xc1001008 32
> 0x00004049
> # busybox devmem 0xc100100c 32
> 0x00000000
>
> Thank you very much!
>
> Dongli Zhang
>
>>
>>> Adding an optional option to 'info pci' to limit to one device would be easy
>>> though; that bit is probably easier than adding a new command.
>>
>> One interesting point is that MSI could be extended for other bus, (e.g MMIO).
>> So "info msi" should be better I guess.
>>
>>
>>> Figuring out the QMP representation of your entries might be harder -
>>> and if this is strictly for debug, probably not worth it?
>>
>> I think so.
>>
>> Thanks
>>
>>
>>> Dave
>>>
>>>
>>>> Dongli Zhang
>>>>
>>>>>> 2. Is there any issue with output format?
>>>>> If it's not for QMP, I guess it's not a part of ABI so it should be fine.
>>>>>
>>>>>
>>>>>> 3. Is it fine to add only for HMP, but not QMP?
>>>>> I think so.
>>>>>
>>>>> Thanks
>>>>>
>>>>>
>>>>>> Thank you very much!
>>>>>>
>>>>>> Dongli Zhang
>>>>>>
>>>>>>
>>>>>>

Re: [PATCH RFC 0/1] To add HMP interface to dump PCI MSI-X table/PBA

[Dongli Zhang]

On 4/27/21 10:55 PM, Jason Wang wrote:
> 
> 在 2021/4/28 下午1:10, Dongli Zhang 写道:
>> Hi Jason,
>>
>> On 4/27/21 7:31 PM, Jason Wang wrote:
>>> 在 2021/4/27 下午4:53, Dr. David Alan Gilbert 写道:
>>>> * Dongli Zhang (dongli.zhang@oracle.com) wrote:
>>>>> On 4/22/21 11:01 PM, Jason Wang wrote:
>>>>>> 在 2021/4/23 下午12:47, Dongli Zhang 写道:
>>>>>>> This is inspired by the discussion with Jason on below patchset.
>>>>>>>
>>>>>>> https://urldefense.com/v3/__https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg09020.html__;!!GqivPVa7Brio!KbGQZW5lq3JZ60k12NuWZ6Th1lT6AwmBTF0pBgoWUKKQ4-2UhdW57PtvXUN5XQnZ2NU$
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The new HMP command is introduced to dump the MSI-X table and PBA.
>>>>>>>
>>>>>>> Initially, I was going to add new option to "info pci". However, as the
>>>>>>> number of entries is not determined and the output of MSI-X table is much
>>>>>>> more similar to the output of hmp_info_tlb()/hmp_info_mem(), this patch
>>>>>>> adds interface for only HMP.
>>>>>>>
>>>>>>> The patch is tagged with RFC because I am looking for suggestions on:
>>>>>>>
>>>>>>> 1. Is it fine to add new "info msix <dev>" command?
>>>>>> I wonder the reason for not simply reusing "info pci"?
>>>>> The "info pci" will show PCI data for all devices and it does not accept any
>>>>> argument to print for a specific device.
>>>>>
>>>>> In addition, the "info pci" relies on qmp_query_pci(), where this patch
>>>>> will not
>>>>> implement the interface for QMP considering the number of MSI-X entries is not
>>>>> determined.
>>>>>
>>>>> Suppose we have 10 NVMe (emulated by QEMU with default number of queues), we
>>>>> will have about 600+ lines of output.
>>>>   From an HMP perspective I'm happy, so:
>>>>
>>>> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>>
>>>> but since I don't know much about MSI I'd like to see Jason's reply.
>>>
>>> I think we'd better have more information, e.g the device can optionally report
>>> how the MSI-X vector is used.
>>>
>>> Virtio-pci could be the first user for this.
>> As discussed in another thread, you were talking about to print MSIMessage.
>>
>> However, I prefer to print the raw data as I think the user of this interface
>> should be able to understand it as MSI-X messages.
>>
>> For instance, below is the data printed by "info msix".
> 
> 
> Just to clarify, I meant e.g for virtio-pci device, we can let it to print the
> mapping between vq and msix vectors:
> 
> vq[0].msix_vector = 0
> vq[1].msix_vector = 1
> config.msix_vector = 2
> ...
> 
> But this could be added on top if you wish.
> 
> Does this make sense?

Yes, this makes since. For QEMU they are:

- vdev->vq[n].vector
- vdev->config_vector


I will introduce a callback and implement for virtio-pci to dump the vector mapping.

By default, "info msix <dev>" prints only msix table/PBA.

"info msix -d <dev>" will print device specific data.

Thank you very much!

Dongli Zhang

> 
> Thanks
> 
> 
>>
>> 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
>>
>> The 1st column is Message Lower Address.
>>
>> The 2nd column is Message Upper Address.
>>
>> The 3rd column is Message Data.
>>
>> The 4th column is Vector Control.
>>
>> In my opinion, this is equivalent to MSIMessage.
>>
>> 26 struct MSIMessage {
>> 27     uint64_t address; --> column 1 and 2
>> 28     uint32_t data;    --> column 3
>> 29 };
>>
>>
>> We use the similar way to read from Linux OS, e,g., given the address of MSI-X
>> cap, here is how we read from OS side.
>>
>> # busybox devmem 0xc1001000 32
>> 0xFEE00000
>> # busybox devmem 0xc1001004 32
>> 0x00000000
>> # busybox devmem 0xc1001008 32
>> 0x00004049
>> # busybox devmem 0xc100100c 32
>> 0x00000000
>>
>> Thank you very much!
>>
>> Dongli Zhang
>>
>>>
>>>> Adding an optional option to 'info pci' to limit to one device would be easy
>>>> though; that bit is probably easier than adding a new command.
>>>
>>> One interesting point is that MSI could be extended for other bus, (e.g MMIO).
>>> So "info msi" should be better I guess.
>>>
>>>
>>>> Figuring out the QMP representation of your entries might be harder -
>>>> and if this is strictly for debug, probably not worth it?
>>>
>>> I think so.
>>>
>>> Thanks
>>>
>>>
>>>> Dave
>>>>
>>>>
>>>>> Dongli Zhang
>>>>>
>>>>>>> 2. Is there any issue with output format?
>>>>>> If it's not for QMP, I guess it's not a part of ABI so it should be fine.
>>>>>>
>>>>>>
>>>>>>> 3. Is it fine to add only for HMP, but not QMP?
>>>>>> I think so.
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>>
>>>>>>> Thank you very much!
>>>>>>>
>>>>>>> Dongli Zhang
>>>>>>>
>>>>>>>
>>>>>>>
> 

Re: [PATCH RFC 0/1] To add HMP interface to dump PCI MSI-X table/PBA

[Dr. David Alan Gilbert]

* Dongli Zhang (dongli.zhang@oracle.com) wrote:
> Hi Jason,
> 
> On 4/27/21 7:31 PM, Jason Wang wrote:
> > 
> > 在 2021/4/27 下午4:53, Dr. David Alan Gilbert 写道:
> >> * Dongli Zhang (dongli.zhang@oracle.com) wrote:
> >>>
> >>> On 4/22/21 11:01 PM, Jason Wang wrote:
> >>>> 在 2021/4/23 下午12:47, Dongli Zhang 写道:
> >>>>> This is inspired by the discussion with Jason on below patchset.
> >>>>>
> >>>>> https://urldefense.com/v3/__https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg09020.html__;!!GqivPVa7Brio!KbGQZW5lq3JZ60k12NuWZ6Th1lT6AwmBTF0pBgoWUKKQ4-2UhdW57PtvXUN5XQnZ2NU$
> >>>>>
> >>>>>
> >>>>> The new HMP command is introduced to dump the MSI-X table and PBA.
> >>>>>
> >>>>> Initially, I was going to add new option to "info pci". However, as the
> >>>>> number of entries is not determined and the output of MSI-X table is much
> >>>>> more similar to the output of hmp_info_tlb()/hmp_info_mem(), this patch
> >>>>> adds interface for only HMP.
> >>>>>
> >>>>> The patch is tagged with RFC because I am looking for suggestions on:
> >>>>>
> >>>>> 1. Is it fine to add new "info msix <dev>" command?
> >>>>
> >>>> I wonder the reason for not simply reusing "info pci"?
> >>> The "info pci" will show PCI data for all devices and it does not accept any
> >>> argument to print for a specific device.
> >>>
> >>> In addition, the "info pci" relies on qmp_query_pci(), where this patch will not
> >>> implement the interface for QMP considering the number of MSI-X entries is not
> >>> determined.
> >>>
> >>> Suppose we have 10 NVMe (emulated by QEMU with default number of queues), we
> >>> will have about 600+ lines of output.
> >>  From an HMP perspective I'm happy, so:
> >>
> >> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >>
> >> but since I don't know much about MSI I'd like to see Jason's reply.
> > 
> > 
> > I think we'd better have more information, e.g the device can optionally report
> > how the MSI-X vector is used.
> > 
> > Virtio-pci could be the first user for this.
> 
> As discussed in another thread, you were talking about to print MSIMessage.
> 
> However, I prefer to print the raw data as I think the user of this interface
> should be able to understand it as MSI-X messages.
> 
> For instance, below is the data printed by "info msix".
> 
> 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
> 
> The 1st column is Message Lower Address.
> 
> The 2nd column is Message Upper Address.
> 
> The 3rd column is Message Data.
> 
> The 4th column is Vector Control.

So why not add a heading:

  Msg L.Addr Msg U.Addr Msg Data   Vect Ctrl
  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

(if I'm understanding what you said).

Dave

> In my opinion, this is equivalent to MSIMessage.
> 
> 26 struct MSIMessage {
> 27     uint64_t address; --> column 1 and 2
> 28     uint32_t data;    --> column 3
> 29 };
> 
> 
> We use the similar way to read from Linux OS, e,g., given the address of MSI-X
> cap, here is how we read from OS side.
> 
> # busybox devmem 0xc1001000 32
> 0xFEE00000
> # busybox devmem 0xc1001004 32
> 0x00000000
> # busybox devmem 0xc1001008 32
> 0x00004049
> # busybox devmem 0xc100100c 32
> 0x00000000
> 
> Thank you very much!
> 
> Dongli Zhang
> 
> > 
> > 
> >>
> >> Adding an optional option to 'info pci' to limit to one device would be easy
> >> though; that bit is probably easier than adding a new command.
> > 
> > 
> > One interesting point is that MSI could be extended for other bus, (e.g MMIO).
> > So "info msi" should be better I guess.
> > 
> > 
> >> Figuring out the QMP representation of your entries might be harder -
> >> and if this is strictly for debug, probably not worth it?
> > 
> > 
> > I think so.
> > 
> > Thanks
> > 
> > 
> >>
> >> Dave
> >>
> >>
> >>> Dongli Zhang
> >>>
> >>>>
> >>>>> 2. Is there any issue with output format?
> >>>>
> >>>> If it's not for QMP, I guess it's not a part of ABI so it should be fine.
> >>>>
> >>>>
> >>>>> 3. Is it fine to add only for HMP, but not QMP?
> >>>>
> >>>> I think so.
> >>>>
> >>>> Thanks
> >>>>
> >>>>
> >>>>> Thank you very much!
> >>>>>
> >>>>> Dongli Zhang
> >>>>>
> >>>>>
> >>>>>
> > 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH RFC 0/1] To add HMP interface to dump PCI MSI-X table/PBA

[Dongli Zhang]

On 4/28/21 1:45 AM, Dr. David Alan Gilbert wrote:
> * Dongli Zhang (dongli.zhang@oracle.com) wrote:
>> Hi Jason,
>>
>> On 4/27/21 7:31 PM, Jason Wang wrote:
>>>
>>> 在 2021/4/27 下午4:53, Dr. David Alan Gilbert 写道:
>>>> * Dongli Zhang (dongli.zhang@oracle.com) wrote:
>>>>>
>>>>> On 4/22/21 11:01 PM, Jason Wang wrote:
>>>>>> 在 2021/4/23 下午12:47, Dongli Zhang 写道:
>>>>>>> This is inspired by the discussion with Jason on below patchset.
>>>>>>>
>>>>>>> https://urldefense.com/v3/__https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg09020.html__;!!GqivPVa7Brio!KbGQZW5lq3JZ60k12NuWZ6Th1lT6AwmBTF0pBgoWUKKQ4-2UhdW57PtvXUN5XQnZ2NU$
>>>>>>>
>>>>>>>
>>>>>>> The new HMP command is introduced to dump the MSI-X table and PBA.
>>>>>>>
>>>>>>> Initially, I was going to add new option to "info pci". However, as the
>>>>>>> number of entries is not determined and the output of MSI-X table is much
>>>>>>> more similar to the output of hmp_info_tlb()/hmp_info_mem(), this patch
>>>>>>> adds interface for only HMP.
>>>>>>>
>>>>>>> The patch is tagged with RFC because I am looking for suggestions on:
>>>>>>>
>>>>>>> 1. Is it fine to add new "info msix <dev>" command?
>>>>>>
>>>>>> I wonder the reason for not simply reusing "info pci"?
>>>>> The "info pci" will show PCI data for all devices and it does not accept any
>>>>> argument to print for a specific device.
>>>>>
>>>>> In addition, the "info pci" relies on qmp_query_pci(), where this patch will not
>>>>> implement the interface for QMP considering the number of MSI-X entries is not
>>>>> determined.
>>>>>
>>>>> Suppose we have 10 NVMe (emulated by QEMU with default number of queues), we
>>>>> will have about 600+ lines of output.
>>>>  From an HMP perspective I'm happy, so:
>>>>
>>>> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>>
>>>> but since I don't know much about MSI I'd like to see Jason's reply.
>>>
>>>
>>> I think we'd better have more information, e.g the device can optionally report
>>> how the MSI-X vector is used.
>>>
>>> Virtio-pci could be the first user for this.
>>
>> As discussed in another thread, you were talking about to print MSIMessage.
>>
>> However, I prefer to print the raw data as I think the user of this interface
>> should be able to understand it as MSI-X messages.
>>
>> For instance, below is the data printed by "info msix".
>>
>> 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
>>
>> The 1st column is Message Lower Address.
>>
>> The 2nd column is Message Upper Address.
>>
>> The 3rd column is Message Data.
>>
>> The 4th column is Vector Control.
> 
> So why not add a heading:
> 
>   Msg L.Addr Msg U.Addr Msg Data   Vect Ctrl
>   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
> 
> (if I'm understanding what you said).

Thank you very much! I will add that in the patch.

Dongli Zhang

> 
> Dave
> 
>> In my opinion, this is equivalent to MSIMessage.
>>
>> 26 struct MSIMessage {
>> 27     uint64_t address; --> column 1 and 2
>> 28     uint32_t data;    --> column 3
>> 29 };
>>
>>
>> We use the similar way to read from Linux OS, e,g., given the address of MSI-X
>> cap, here is how we read from OS side.
>>
>> # busybox devmem 0xc1001000 32
>> 0xFEE00000
>> # busybox devmem 0xc1001004 32
>> 0x00000000
>> # busybox devmem 0xc1001008 32
>> 0x00004049
>> # busybox devmem 0xc100100c 32
>> 0x00000000
>>
>> Thank you very much!
>>
>> Dongli Zhang
>>
>>>
>>>
>>>>
>>>> Adding an optional option to 'info pci' to limit to one device would be easy
>>>> though; that bit is probably easier than adding a new command.
>>>
>>>
>>> One interesting point is that MSI could be extended for other bus, (e.g MMIO).
>>> So "info msi" should be better I guess.
>>>
>>>
>>>> Figuring out the QMP representation of your entries might be harder -
>>>> and if this is strictly for debug, probably not worth it?
>>>
>>>
>>> I think so.
>>>
>>> Thanks
>>>
>>>
>>>>
>>>> Dave
>>>>
>>>>
>>>>> Dongli Zhang
>>>>>
>>>>>>
>>>>>>> 2. Is there any issue with output format?
>>>>>>
>>>>>> If it's not for QMP, I guess it's not a part of ABI so it should be fine.
>>>>>>
>>>>>>
>>>>>>> 3. Is it fine to add only for HMP, but not QMP?
>>>>>>
>>>>>> I think so.
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>>
>>>>>>> Thank you very much!
>>>>>>>
>>>>>>> Dongli Zhang
>>>>>>>
>>>>>>>
>>>>>>>
>>>
>>