On Mon, Jul 22, 2024 at 4:42 PM Cindy Lu <l...@redhat.com> wrote: > > > > On Mon, 22 Jul 2024 at 15:24, Jason Wang <jasow...@redhat.com> wrote: > > > > Hi Cindy > > > > On Fri, Jul 19, 2024 at 1:25 PM Cindy Lu <l...@redhat.com> wrote: > > > > > > The crash was reported in MAC OS and NixOS, here is the link for this bug > > > https://gitlab.com/qemu-project/qemu/-/issues/2334 > > > https://gitlab.com/qemu-project/qemu/-/issues/2321 > > > > > > In this bug, they are using the virtio_input device. The guest notifier > > > was > > > not supported for this device, The function > > > virtio_pci_set_guest_notifiers() > > > was not called, and the vector_irqfd was not initialized. > > > > > > So the fix is adding the check for vector_irqfd in > > > virtio_pci_get_notifier() > > > > > > But The function virtio_pci_get_notifier(),it can also be used in all > > > kinds of device. > > > If the vector_irqfd still didn't initial after the > > > VIRTIO_CONFIG_S_DRIVER_OK is set > > > means this device is not using guest notifier. We can let the check fail > > > here > > > > > > This fix is verified in vyatta,MacOS,NixOS,fedora system. > > > > > > The bt tree for this bug is: > > > Thread 6 "CPU 0/KVM" received signal SIGSEGV, Segmentation fault. > > > [Switching to Thread 0x7c817be006c0 (LWP 1269146)] > > > kvm_virtio_pci_vq_vector_use () at > > > ../qemu-9.0.0/hw/virtio/virtio-pci.c:817 > > > 817 if (irqfd->users == 0) { > > > (gdb) thread apply all bt > > > ... > > > Thread 6 (Thread 0x7c817be006c0 (LWP 1269146) "CPU 0/KVM"): > > > 0 kvm_virtio_pci_vq_vector_use () at > > > ../qemu-9.0.0/hw/virtio/virtio-pci.c:817 > > > 1 kvm_virtio_pci_vector_use_one () at > > > ../qemu-9.0.0/hw/virtio/virtio-pci.c:893 > > > 2 0x00005983657045e2 in memory_region_write_accessor () at > > > ../qemu-9.0.0/system/memory.c:497 > > > 3 0x0000598365704ba6 in access_with_adjusted_size () at > > > ../qemu-9.0.0/system/memory.c:573 > > > 4 0x0000598365705059 in memory_region_dispatch_write () at > > > ../qemu-9.0.0/system/memory.c:1528 > > > 5 0x00005983659b8e1f in flatview_write_continue_step.isra.0 () at > > > ../qemu-9.0.0/system/physmem.c:2713 > > > 6 0x000059836570ba7d in flatview_write_continue () at > > > ../qemu-9.0.0/system/physmem.c:2743 > > > 7 flatview_write () at ../qemu-9.0.0/system/physmem.c:2774 > > > 8 0x000059836570bb76 in address_space_write () at > > > ../qemu-9.0.0/system/physmem.c:2894 > > > 9 0x0000598365763afe in address_space_rw () at > > > ../qemu-9.0.0/system/physmem.c:2904 > > > 10 kvm_cpu_exec () at ../qemu-9.0.0/accel/kvm/kvm-all.c:2917 > > > 11 0x000059836576656e in kvm_vcpu_thread_fn () at > > > ../qemu-9.0.0/accel/kvm/kvm-accel-ops.c:50 > > > 12 0x0000598365926ca8 in qemu_thread_start () at > > > ../qemu-9.0.0/util/qemu-thread-posix.c:541 > > > 13 0x00007c8185bcd1cf in ??? () at /usr/lib/libc.so.6 > > > 14 0x00007c8185c4e504 in clone () at /usr/lib/libc.so.6 > > > > > > Fixes: 2ce6cff94d ("virtio-pci: fix use of a released vector") > > > Cc: qemu-sta...@nongnu.org > > > Signed-off-by: Cindy Lu <l...@redhat.com> > > > --- > > > hw/virtio/virtio-pci.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > > index 592fdaa10f..dc31a37ec0 100644 > > > --- a/hw/virtio/virtio-pci.c > > > +++ b/hw/virtio/virtio-pci.c > > > @@ -860,6 +860,9 @@ static int virtio_pci_get_notifier(VirtIOPCIProxy > > > *proxy, int queue_no, > > > VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); > > > VirtQueue *vq; > > > > > > + if (!proxy->vector_irqfd && vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) > > > + return -1; > > > + > > > > Did this mean !proxy->vector_irqfd && !(vdev->status & > > VIRTIO_CONFIG_S_DRIVER_OK)) is legal? > > > > Thanks > > > yes, for my test, I didn't meet this kind of environment.
This needs to be checked, we can use qtest to poc this condition if you wish. But I meant we need to explain why there is a check for DRIVER_OK here. For example, did the current Qemu guarantee that? > However, since this function is used widely in many environments, I cannot > cover all devices for testing. > > I think we can change the patch back to the first version, which is to check > the proxy->vector_irqfd in virtio_pci_set_vector(). This will have a lower > risk and make more sense > thanks > cindy Thanks > > > On Mon, 22 Jul 2024 at 15:24, Jason Wang <jasow...@redhat.com> wrote: >> >> Hi Cindy >> >> On Fri, Jul 19, 2024 at 1:25 PM Cindy Lu <l...@redhat.com> wrote: >> > >> > The crash was reported in MAC OS and NixOS, here is the link for this bug >> > https://gitlab.com/qemu-project/qemu/-/issues/2334 >> > https://gitlab.com/qemu-project/qemu/-/issues/2321 >> > >> > In this bug, they are using the virtio_input device. The guest notifier was >> > not supported for this device, The function >> > virtio_pci_set_guest_notifiers() >> > was not called, and the vector_irqfd was not initialized. >> > >> > So the fix is adding the check for vector_irqfd in >> > virtio_pci_get_notifier() >> > >> > But The function virtio_pci_get_notifier(),it can also be used in all >> > kinds of device. >> > If the vector_irqfd still didn't initial after the >> > VIRTIO_CONFIG_S_DRIVER_OK is set >> > means this device is not using guest notifier. We can let the check fail >> > here >> > >> > This fix is verified in vyatta,MacOS,NixOS,fedora system. >> > >> > The bt tree for this bug is: >> > Thread 6 "CPU 0/KVM" received signal SIGSEGV, Segmentation fault. >> > [Switching to Thread 0x7c817be006c0 (LWP 1269146)] >> > kvm_virtio_pci_vq_vector_use () at ../qemu-9.0.0/hw/virtio/virtio-pci.c:817 >> > 817 if (irqfd->users == 0) { >> > (gdb) thread apply all bt >> > ... >> > Thread 6 (Thread 0x7c817be006c0 (LWP 1269146) "CPU 0/KVM"): >> > 0 kvm_virtio_pci_vq_vector_use () at >> > ../qemu-9.0.0/hw/virtio/virtio-pci.c:817 >> > 1 kvm_virtio_pci_vector_use_one () at >> > ../qemu-9.0.0/hw/virtio/virtio-pci.c:893 >> > 2 0x00005983657045e2 in memory_region_write_accessor () at >> > ../qemu-9.0.0/system/memory.c:497 >> > 3 0x0000598365704ba6 in access_with_adjusted_size () at >> > ../qemu-9.0.0/system/memory.c:573 >> > 4 0x0000598365705059 in memory_region_dispatch_write () at >> > ../qemu-9.0.0/system/memory.c:1528 >> > 5 0x00005983659b8e1f in flatview_write_continue_step.isra.0 () at >> > ../qemu-9.0.0/system/physmem.c:2713 >> > 6 0x000059836570ba7d in flatview_write_continue () at >> > ../qemu-9.0.0/system/physmem.c:2743 >> > 7 flatview_write () at ../qemu-9.0.0/system/physmem.c:2774 >> > 8 0x000059836570bb76 in address_space_write () at >> > ../qemu-9.0.0/system/physmem.c:2894 >> > 9 0x0000598365763afe in address_space_rw () at >> > ../qemu-9.0.0/system/physmem.c:2904 >> > 10 kvm_cpu_exec () at ../qemu-9.0.0/accel/kvm/kvm-all.c:2917 >> > 11 0x000059836576656e in kvm_vcpu_thread_fn () at >> > ../qemu-9.0.0/accel/kvm/kvm-accel-ops.c:50 >> > 12 0x0000598365926ca8 in qemu_thread_start () at >> > ../qemu-9.0.0/util/qemu-thread-posix.c:541 >> > 13 0x00007c8185bcd1cf in ??? () at /usr/lib/libc.so.6 >> > 14 0x00007c8185c4e504 in clone () at /usr/lib/libc.so.6 >> > >> > Fixes: 2ce6cff94d ("virtio-pci: fix use of a released vector") >> > Cc: qemu-sta...@nongnu.org >> > Signed-off-by: Cindy Lu <l...@redhat.com> >> > --- >> > hw/virtio/virtio-pci.c | 3 +++ >> > 1 file changed, 3 insertions(+) >> > >> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c >> > index 592fdaa10f..dc31a37ec0 100644 >> > --- a/hw/virtio/virtio-pci.c >> > +++ b/hw/virtio/virtio-pci.c >> > @@ -860,6 +860,9 @@ static int virtio_pci_get_notifier(VirtIOPCIProxy >> > *proxy, int queue_no, >> > VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); >> > VirtQueue *vq; >> > >> > + if (!proxy->vector_irqfd && vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) >> > + return -1; >> > + >> >> Did this mean !proxy->vector_irqfd && !(vdev->status & >> VIRTIO_CONFIG_S_DRIVER_OK)) is legal? >> >> Thanks >> >> > if (queue_no == VIRTIO_CONFIG_IRQ_IDX) { >> > *n = virtio_config_get_guest_notifier(vdev); >> > *vector = vdev->config_vector; >> > -- >> > 2.45.0 >> > >>
