On Thu, Nov 13, 2025 at 6:03 AM Peter Maydell <[email protected]> wrote: > > On Wed, 8 Oct 2025 at 20:21, Navid Emamdoost <[email protected]> wrote: > > > > The qpci_iomap() function fails with a fatal g_assert(addr) if it > > probes a PCI BAR that has a size of zero. This is expected behavior > > for certain devices, like the Q35 PCI Host Bridge, which have valid but > > unimplemented BARs. > > This assertion blocks the creation of fuzz targets for complex machine > > types that include these devices. > > Make the check conditional on !CONFIG_FUZZ. In fuzzing builds, a > > zero-sized BAR is now handled gracefully by returning an empty BAR > > struct, allowing fuzzing to proceed. The original assertion is kept for > > all other builds to maintain strict checking for qtest and production > > environments. > > > > Signed-off-by: Navid Emamdoost <[email protected]> > > --- > > tests/qtest/libqos/pci.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c > > index a59197b992..df9e2a3993 100644 > > --- a/tests/qtest/libqos/pci.c > > +++ b/tests/qtest/libqos/pci.c > > @@ -541,6 +541,22 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, > > uint64_t *sizeptr) > > addr &= PCI_BASE_ADDRESS_MEM_MASK; > > } > > > > +#ifdef CONFIG_FUZZ > > + /* > > + * During fuzzing runs, an unimplemented BAR (addr=0) is not a fatal > > + * error. This occurs when probing devices like the Q35 host bridge. We > > + * return gracefully to allow fuzzing to continue. In non-fuzzing > > builds, > > + * we retain the original g_assert() to catch unexpected behavior. > > + */ > > + if (!addr) { > > + if (sizeptr) { > > + *sizeptr = 0; > > + } > > + memset(&bar, 0, sizeof(bar)); > > + return bar; > > + } > > +#endif > > Personally I think I'd prefer it if we didn't make this > dependent on CONFIG_FUZZ. If BARs with no size are > a valid thing then the libqos API should handle them > (e.g. in theory one should be able to write a test that the > Q35 host bridge provides the addr=0 BARs it is supposed to). > > I think if we added the size to the QPCIBar struct then we > could assert in the accessors like qpci_io_readb() and > friends that the offset provided was in range. That would > catch both the unlikely "we implemented the BAR with no > size" case and the rather more likely "we got the size too > small" case (and also the "bug in the test and it got the > offset too big" case), and would mean that we don't lose > anything by not asserting that we have a non-zero-size BAR here. > > What do you think? > > thanks > -- PMM
Hi Peter, That's a much more elegant approach. Thank you for the suggestion! I've done an initial investigation into the impact of adding a size field to QPCIBar and checking it in the accessors. As you anticipated, making the API safer immediately and correctly flushed out a few latent issues in the existing qtest suite. Before I prepare and send the full v2 patch series, I wanted to run my plan for fixing the test failures by you: - Issue with qpci_legacy_iomap: Several tests (like ide-test and tco-test) fail because they use qpci_legacy_iomap, which has no way to provide a BAR size. My plan is to change its signature to qpci_legacy_iomap(dev, addr, size) and then update the handful of failing call sites to provide the correct, explicit I/O region size. This seems like the cleanest way to make them compatible and safer. - Issue with nvme-test: The qos-test fails during the nvmetest_oob_cmb_test because the test logic was performing an illegal out-of-bounds access on the entire PCI BAR. My plan is to rewrite this specific test to correctly read the CMB registers and perform a valid out-of-bounds check within the BAR's limits, which is what the test was originally intended to do. Does this plan for handling the test failures seem correct to you? If yes, I will go ahead and prepare the v2 series with the above fixes. -- Thank you, Navid.
