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é <phi...@linaro.org> >> Signed-off-by: Nicholas Piggin <npig...@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
