Hi Peter,
On Thu, Nov 27, 2025 at 5:17 AM Peter Maydell <[email protected]> wrote:
>
> On Thu, 27 Nov 2025 at 00:12, Navid Emamdoost <[email protected]> wrote:
> >
> > The qpci_iomap() function would previously fail with a fatal assertion
> > if it probed a PCI BAR that had a size of zero. This is, however,
> > expected behavior for some devices like the Q35 host bridge, and the
> > assertion blocked the creation of new fuzzing targets.
> >
> > Instead of asserting at map time, modify the QPCIBar struct to store
> > the BAR's size. Defer the safety check to the accessor functions
> > (qpci_io_readb, qpci_memread, etc.), which now assert that any
> > access is within the BAR's bounds.
> >
> > Signed-off-by: Navid Emamdoost [email protected]
> > ---
> > tests/qtest/libqos/pci.c | 25 ++++++++++++++++++++++++-
> > tests/qtest/libqos/pci.h | 1 +
> > 2 files changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
> > index a59197b992..70caf382cc 100644
> > --- a/tests/qtest/libqos/pci.c
> > +++ b/tests/qtest/libqos/pci.c
> > @@ -396,6 +396,7 @@ void qpci_config_writel(QPCIDevice *dev, uint8_t
> > offset, uint32_t value)
> >
> > uint8_t qpci_io_readb(QPCIDevice *dev, QPCIBar token, uint64_t off)
> > {
> > + g_assert(off + 1 <= token.size);
> > QPCIBus *bus = dev->bus;
>
> The indent seems to be wrong for all your changes to these functions?
>
> Also, we need "make check" to pass for every commit in the
> patchset, not just after it has all been applied. So we need
> to make the fixes that you have in patches 2-4 before we
> can start enforcing the size limits with assertions.
Do you think it's better to squash all changes of patch1-4 into
a single commit that reworks the libqos PCI API and fixes all affected tests.
>
> > @@ -541,6 +550,19 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno,
> > uint64_t *sizeptr)
> > addr &= PCI_BASE_ADDRESS_MEM_MASK;
> > }
> >
> > + if (!addr){
>
> Missing space before "{". (scripts/checkpatch.pl will
> probably catch this kind of style error.)
>
> > + /*
> > + * This is an unimplemented BAR. It is not a fatal error.
> > + * We model it as a BAR with a size of zero. Any attempt to
> > + * access it will be caught by assertions in the accessors.
> > + */
> > + if (sizeptr) {
> > + *sizeptr = 0;
> > + }
> > + memset(&bar, 0, sizeof(bar));
> > + return bar;
> > + }
> > +
> > g_assert(addr); /* Must have *some* size bits */
>
> We can drop this assert now, because we just dealt with
> the addr == 0 case.
>
> > size = 1U << ctz32(addr);
> > @@ -572,6 +594,7 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t
> > *sizeptr)
> > }
> >
> > bar.addr = loc;
> > + bar.size = size;
> > return bar;
> > }
>
> thanks
> -- PMM
--
Thank you,
Navid.
