Re: [PATCH v5 03/11] tests/qtest: Add libqos function for testing msix interrupt status

["Nicholas Piggin" <npiggin@gmail.com>]

On Mon May 5, 2025 at 3:37 PM AEST, Akihiko Odaki wrote:
> On 2025/05/02 12:04, Nicholas Piggin wrote:
>> This function is duplicated 3 times, with more potential future users.
>> Factor it into libqos, using qtest_memset instead of qtest_writel to
>> clear the message just because that looks nicer with the qtest_memread
>> used to read it.
>> 
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   tests/qtest/libqos/pci.h               |  2 ++
>>   tests/qtest/libqos/pci.c               | 48 ++++++++++++++++++++++++++
>>   tests/qtest/libqos/virtio-pci-modern.c | 31 +++--------------
>>   tests/qtest/libqos/virtio-pci.c        | 40 ++++-----------------
>>   4 files changed, 62 insertions(+), 59 deletions(-)
>> 
>> diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
>> index 83896145235..9f8f154c301 100644
>> --- a/tests/qtest/libqos/pci.h
>> +++ b/tests/qtest/libqos/pci.h
>> @@ -92,6 +92,8 @@ void qpci_msix_enable(QPCIDevice *dev);
>>   void qpci_msix_disable(QPCIDevice *dev);
>>   bool qpci_msix_pending(QPCIDevice *dev, uint16_t entry);
>>   bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry);
>> +bool qpci_msix_test_interrupt(QPCIDevice *dev, uint32_t msix_entry,
>> +                              uint64_t msix_addr, uint32_t msix_data);
>>   uint16_t qpci_msix_table_size(QPCIDevice *dev);
>>   
>>   uint8_t qpci_config_readb(QPCIDevice *dev, uint8_t offset);
>> diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
>> index a59197b9922..773fd1fb6cf 100644
>> --- a/tests/qtest/libqos/pci.c
>> +++ b/tests/qtest/libqos/pci.c
>> @@ -351,6 +351,54 @@ bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry)
>>       }
>>   }
>>   
>> +/**
>> + * qpci_msix_test_interrupt - test whether msix interrupt has been raised
>
> Nitpick: Let's write as "MSI-X" instead of msix in documentation.

Okay.

>> + * @dev: PCI device
>> + * @msix_entry: msix entry to test
>> + * @msix_addr: address of msix message
>
> Perhaps deriving the address in this function may make things simpler by 
> removing the documentation and assertion code and not requiring callers 
> to pass it.

addr and data could both be derived from the MSI-X table, but passing
them in here is how some of the existing helpers are structured, so I
will leave it like this. I think I have slight preference for this way
but if there is strong preference for deriving them implicitly then it
could be a follow up patch.

>
>> + * @msix_data: expected msix message payload
>> + *
>> + * This tests whether the msix source has raised an interrupt. If the msix
>
> Another nitpick: "whether the device has raised an MSI-X interrupt" - 
> "msix source" is not a pharsed used elsewhere and it can raise other 
> kind of interrupts too so let's make the kind of interrupt specific.

Sure.

Thanks,
Nick