Re: [PATCH v3 1/1] virtio-pci: Add lookup subregion of VirtIOPCIRegion MR

[Gao,Shiyuan]

> > Now virtio_address_space_lookup only lookup common/isr/device/notify

> > MR and exclude their subregions.

> >

> > When VHOST_USER_PROTOCOL_F_HOST_NOTIFIER enable, the notify MR has

> > host-notifier subregions and we need use host-notifier MR to

> > notify the hardware accelerator directly instead of eventfd notify.

> >

> > Further more, maybe common/isr/device MR also has subregions in

> > the future, so need memory_region_find for each MR incluing

> > their subregions.

> >

> > Add lookup subregion of VirtIOPCIRegion MR instead of only lookup container MR.

> >

> > Fixes: a93c8d8 ("virtio-pci: Replace modern_as with direct access to modern_bar")

> >

> > Co-developed-by: Zuo Boqun <zuoboqun@baidu.com>

> > Signed-off-by: Gao Shiyuan <gaoshiyuan@baidu.com>

> > Signed-off-by: Zuo Boqun <zuoboqun@baidu.com>

> > ---

> >   hw/virtio/virtio-pci.c | 8 ++++++--

> >   1 file changed, 6 insertions(+), 2 deletions(-)

> >

> > ---

> > v2 -> v3:

> > * modify commit message

> > * remove unused variable and move mrs to the inner block

> > * replace error_report with assert

> >

> > v1 -> v2:

> > * modify commit message

> > * replace direct iteration over subregions with memory_region_find.

> >

> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c

> > index 524b63e5c7..4d832fe845 100644

> > --- a/hw/virtio/virtio-pci.c

> > +++ b/hw/virtio/virtio-pci.c

> > @@ -615,8 +615,12 @@ static MemoryRegion *virtio_address_space_lookup(VirtIOPCIProxy *proxy,

> >           reg = &proxy->regs[i];

> >           if (*off >= reg->offset &&

> >               *off + len <= reg->offset + reg->size) {

> > -            *off -= reg->offset;

> > -            return &reg->mr;

> > +            MemoryRegionSection mrs = memory_region_find(&reg->mr,

> > +                                        *off - reg->offset, len);

> > +            assert(mrs.mr);

>

> We are able to trigger that assert:

>

> https://gitlab.com/qemu-project/qemu/-/issues/2576

>

> Can you take a look and send a fix?

>

> --

> Cheers,

>

> David / dhildenb

>

Sorry for this, Peter and David's Command line can reproduce in my environment.

But this patch works fine in my testing environment, it can start VM(both q35 and i440fx) success and

I will compare the differences in command line parameters with Peter's.

As for qtest, When I use this command line(Change the machine from q35 to i440fx), it looks ok

    cat << EOF | ./qemu-system-x86_64 -display none -machine accel=qtest, -m \

    512M -machine pc -nodefaults -device virtio-balloon -qtest stdio

    outl 0xcf8 0x80000890

    outl 0xcfc 0x2

    outl 0xcf8 0x80000891

    inl 0xcfc

    EOF

So I think there might exists other bug, and this patch just happens to trigger them.

I will find it as soon as possible.