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