Re: [PATCH v2 1/5] libqos: pci: Handle zero-sized BARs gracefully

[Navid Emamdoost <navidem@google.com>]

Hi Peter,

On Thu, Nov 27, 2025 at 5:17 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 27 Nov 2025 at 00:12, Navid Emamdoost <navidem@google.com> 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 navidem@google.com
> > ---
> >  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.