Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

virt: Add PCI host controller #10

Merged
merged 1 commit into from
Feb 4, 2021
Merged

virt: Add PCI host controller #10

merged 1 commit into from
Feb 4, 2021

Conversation

abrodkin
Copy link
Member

@abrodkin abrodkin commented Feb 3, 2021

This adds PCI bridge support to ARC's virt platform, which in its turn allows to both connect complex Virtio devices like VirtGPU, VirtRNG as well as pass-through host PCI devices including USB.

Note this requires corresponding support in the Linux kernel like this:

diff --git a/arch/arc/Kconfig b/arch/arc/Kconfig
index ba00c4e1e1c2..09efd393a0b2 100644
--- a/arch/arc/Kconfig
+++ b/arch/arc/Kconfig
@@ -48,6 +48,7 @@ config ARC
        select PCI_SYSCALL if PCI
        select PERF_USE_VMALLOC if ARC_CACHE_VIPT_ALIASING
        select HAVE_ARCH_JUMP_LABEL if ISA_ARCV2 && !CPU_ENDIAN_BE32
+       select HAVE_PCI

 config ARCH_HAS_CACHE_LINE_SIZE
        def_bool y
diff --git a/arch/arc/boot/dts/haps_hs.dts b/arch/arc/boot/dts/haps_hs.dts
index 60d578e2781f..c57c7381c468 100644
--- a/arch/arc/boot/dts/haps_hs.dts
+++ b/arch/arc/boot/dts/haps_hs.dts
@@ -95,5 +95,45 @@ virtio4: virtio@f0108000 {
                        reg = <0xf0108000 0x2000>;
                        interrupts = <35>;
                };
+
+               pci {
+                       compatible = "pci-host-ecam-generic";
+                       device_type = "pci";
+                       #address-cells = <3>;
+                       #size-cells = <2>;
+                       #interrupt-cells = <0x1>;
+
+                       bus-range = <0x0 0x0>;
+                       reg = <0xe0000000 0x1000000>;
+
+                       // BUS_ADDRESS(3)  CPU_PHYSICAL(1)  SIZE(2)
+                       ranges = <0x01000000 0x0 0x00000000  0xc0000000  0x0 0x00010000>, /* PIO */
+                                <0x02000000 0x0 0xd0000000  0xd0000000  0x0 0x10000000>; /* MMIO */
+
+                       // PCI_DEVICE(3)  INT#(1)  CONTROLLER(PHANDLE)  CONTROLLER_IRQ(1)
+                       interrupt-map = <
+                               0x0000 0x0 0x0  0x1  &core_intc  40 /* 1st slot */
+                               0x0000 0x0 0x0  0x2  &core_intc  41
+                               0x0000 0x0 0x0  0x3  &core_intc  42
+                               0x0000 0x0 0x0  0x4  &core_intc  43
+
+                               0x0800 0x0 0x0  0x1  &core_intc  41 /* 2nd slot */
+                               0x0800 0x0 0x0  0x2  &core_intc  42
+                               0x0800 0x0 0x0  0x3  &core_intc  43
+                               0x0800 0x0 0x0  0x4  &core_intc  40
+
+                               0x1000 0x0 0x0  0x1  &core_intc  42 /* 3rd slot */
+                               0x1000 0x0 0x0  0x2  &core_intc  43
+                               0x1000 0x0 0x0  0x3  &core_intc  40
+                               0x1000 0x0 0x0  0x4  &core_intc  41
+
+                               0x1800 0x0 0x0  0x1  &core_intc  43 /* 4th slot */
+                               0x1800 0x0 0x0  0x2  &core_intc  40
+                               0x1800 0x0 0x0  0x3  &core_intc  41
+                               0x1800 0x0 0x0  0x4  &core_intc  42
+                               >;
+
+                       interrupt-map-mask = <0x1800 0x0 0x0  0x7>;
+               };
        };
 };
diff --git a/arch/arc/configs/haps_hs_defconfig b/arch/arc/configs/haps_hs_defconfig
index 86cc5aa4537c..25b0bcd34782 100644
--- a/arch/arc/configs/haps_hs_defconfig
+++ b/arch/arc/configs/haps_hs_defconfig
@@ -31,6 +31,8 @@ CONFIG_NET_KEY=y
 CONFIG_INET=y
 # CONFIG_IPV6 is not set
 # CONFIG_WIRELESS is not set
+CONFIG_PCI=y
+CONFIG_PCI_HOST_GENERIC=y
 CONFIG_DEVTMPFS=y
 CONFIG_DEVTMPFS_MOUNT=y
 # CONFIG_STANDALONE is not set
@@ -51,10 +53,16 @@ CONFIG_SERIAL_8250_NR_UARTS=1
 CONFIG_SERIAL_8250_RUNTIME_UARTS=1
 CONFIG_SERIAL_8250_DW=y
 CONFIG_SERIAL_OF_PLATFORM=y
-# CONFIG_HW_RANDOM is not set
+CONFIG_HW_RANDOM=y
+CONFIG_HW_RANDOM_VIRTIO=y
 # CONFIG_HWMON is not set
-# CONFIG_HID is not set
-# CONFIG_USB_SUPPORT is not set
+CONFIG_DRM=y
+CONFIG_DRM_VIRTIO_GPU=y
+CONFIG_USB=y
+CONFIG_USB_XHCI_HCD=y
+CONFIG_USB_EHCI_HCD=y
+CONFIG_USB_OHCI_HCD=y
+CONFIG_VIRTIO_PCI=y
 CONFIG_VIRTIO_MMIO=y
 # CONFIG_IOMMU_SUPPORT is not set
 CONFIG_EXT2_FS=y
./build/qemu-system-arc -M virt -monitor none -serial mon:stdio -kernel vmlinux -cpu archs \
    -append "root=/dev/vda ro" -drive file=rootfs.ext2,format=raw,id=hd0 -device virtio-blk-device,drive=hd0 \
    -netdev user,id=eth0 -device virtio-net-pci,netdev=eth0 -device virtio-rng-pci -device usb-ehci,id=ehci \
    --global cpu.freq_hz=50000000 -device virtio-gpu-pci,id=video0 -display gtk,gl=on \
    -device usb-host,vendorid=0x8564,productid=0x1000

And that's what we get:

  1. Host's random number generator in the guest for faster entropy generation.
  2. Real USB flash-drive attached to host passed-through to the guest
  3. 3D rendering off-loaded to the host and so working even faster than on the original HSDK, see 100+ FPS

render1612390013418-min

Screenshot from 2021-02-03 14-50-09 (3)

Screenshot from 2021-02-03 14-50-56 (2)

Screenshot from 2021-02-03 14-51-15 (2)

Signed-off-by: Alexey Brodkin <[email protected]>
@abrodkin
Copy link
Member Author

abrodkin commented Feb 3, 2021

@vineetgarc indeed with that fresh QEMU base the only change which is needed for the kernel is pure PCI-related stuff pasted as a diff for 5.10.y above. Should I send it as a formal patch for v5.11?

@vineetgarc
Copy link
Contributor

You are adding more stuff in the minimal defconfig - all of this builds more code, loads slower in nSIM and haps

@cupertinomiranda
Copy link

cupertinomiranda commented Feb 4, 2021

I tried the code and at least we can get the random number generator maping working, which speeds up SSH significantly.
If no one has any objections, I will merge the pull request in our meeting tonight.

@cupertinomiranda
Copy link

All seems good, this is tested and works !

@cupertinomiranda cupertinomiranda merged commit 59cfed5 into master Feb 4, 2021
abrodkin added a commit that referenced this pull request Feb 6, 2021
AUX registers are used in ARC processors to deal with settings or
internal states of different internals like built-in timers, interrupt
controller(s), caches etc.

Though for us here interrupts and timers are of the main interest as
those are very good examples of IO operations and we do need explicitly
allow it to make "icount" subsystem happy, as otherwise on the first
attempt to set ARC built-in timer LIMIT register we see icount_get_raw_locked()
barking: "qemu-system-arc: Bad icount read", and that's because:
------------------>8-----------------
| gdb --args ./build/qemu-system-arc ... -icount auto
|
| ...
|
| (gdb) b icount.c:116
| Breakpoint 1 at 0x54178a: file ../softmmu/icount.c, line 116.
|
| (gdb) r
|
| Thread 3 "qemu-system-arc" hit Breakpoint 1, icount_get_raw_locked () at ../softmmu/icount.c:116
| 116                 error_report("Bad icount read");
| (gdb) bt
| #0  icount_get_raw_locked () at ../softmmu/icount.c:116
| #1  0x0000555555a957d5 in icount_get_locked () at ../softmmu/icount.c:128
| #2  0x0000555555a9586d in icount_get () at ../softmmu/icount.c:154
| #3  0x0000555555a901fd in tcg_get_virtual_clock () at ../accel/tcg/tcg-cpus.c:524
| #4  0x0000555555a0fe05 in cpus_get_virtual_clock () at ../softmmu/cpus.c:211
| #5  0x0000555555c88fd4 in qemu_clock_get_ns (type=QEMU_CLOCK_VIRTUAL) at ../util/qemu-timer.c:638
| #6  0x00005555559aa8eb in cpu_arc_timer_update (env=0x5555565280a0, timer=0) at ../target/arc/timer.c:42
| #7  0x00005555559ab272 in cpu_arc_store_limit (env=0x5555565280a0, timer=0, value=500000) at ../target/arc/timer.c:246
| #8  0x00005555559aba04 in aux_timer_set (aux_reg_detail=0x5555562eedb0 <arc_aux_regs_detail+2352>, val=500000, data=0x5555565280a0) at ../target/arc/timer.c:436
| #9  0x00005555559a4cb2 in helper_sr (env=0x5555565280a0, val=500000, aux=35) at ../target/arc/op_helper.c:209
| #10 0x00007fffb041dc6a in code_gen_buffer ()
| #11 0x0000555555a2f6c8 in cpu_tb_exec (cpu=0x55555651f960, itb=0x7fffb041db00 <code_gen_buffer+4315859>) at ../accel/tcg/cpu-exec.c:178
| #12 0x0000555555a304ae in cpu_loop_exec_tb (cpu=0x55555651f960, tb=0x7fffb041db00 <code_gen_buffer+4315859>, last_tb=0x7ffff6013928, tb_exit=0x7ffff6013920)
|     at ../accel/tcg/cpu-exec.c:658
| #13 0x0000555555a307a6 in cpu_exec (cpu=0x55555651f960) at ../accel/tcg/cpu-exec.c:771
| #14 0x0000555555a8f911 in tcg_cpu_exec (cpu=0x55555651f960) at ../accel/tcg/tcg-cpus.c:243
| #15 0x0000555555a8fc12 in tcg_rr_cpu_thread_fn (arg=0x55555651f960) at ../accel/tcg/tcg-cpus.c:346
| #16 0x0000555555c9562e in qemu_thread_start (args=0x5555565339e0) at ../util/qemu-thread-posix.c:521
| #17 0x00007ffff7899609 in start_thread (arg=<optimized out>) at pthread_create.c:477
| #18 0x00007ffff77c0293 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
------------------>8-----------------

Fix that, hinting QEMU about possible IO on access of AUX regs,
which are only possible via LR & SR instructions.

Signed-off-by: Alexey Brodkin <[email protected]>
abrodkin added a commit that referenced this pull request Feb 6, 2021
AUX registers are used in ARC processors to deal with settings or
internal states of different internals like built-in timers, interrupt
controller(s), caches etc.

Though for us here interrupts and timers are of the main interest as
those are very good examples of IO operations and we do need explicitly
allow it to make "icount" subsystem happy, as otherwise on the first
attempt to set ARC built-in timer LIMIT register we see icount_get_raw_locked()
barking: "qemu-system-arc: Bad icount read", and that's because:
------------------>8-----------------
| gdb --args ./build/qemu-system-arc ... -icount auto
|
| ...
|
| (gdb) b icount.c:116
| Breakpoint 1 at 0x54178a: file ../softmmu/icount.c, line 116.
|
| (gdb) r
|
| Thread 3 "qemu-system-arc" hit Breakpoint 1, icount_get_raw_locked () at ../softmmu/icount.c:116
| 116                 error_report("Bad icount read");
| (gdb) bt
| #0  icount_get_raw_locked () at ../softmmu/icount.c:116
| #1  0x0000555555a957d5 in icount_get_locked () at ../softmmu/icount.c:128
| #2  0x0000555555a9586d in icount_get () at ../softmmu/icount.c:154
| #3  0x0000555555a901fd in tcg_get_virtual_clock () at ../accel/tcg/tcg-cpus.c:524
| #4  0x0000555555a0fe05 in cpus_get_virtual_clock () at ../softmmu/cpus.c:211
| #5  0x0000555555c88fd4 in qemu_clock_get_ns (type=QEMU_CLOCK_VIRTUAL) at ../util/qemu-timer.c:638
| #6  0x00005555559aa8eb in cpu_arc_timer_update (env=0x5555565280a0, timer=0) at ../target/arc/timer.c:42
| #7  0x00005555559ab272 in cpu_arc_store_limit (env=0x5555565280a0, timer=0, value=500000) at ../target/arc/timer.c:246
| #8  0x00005555559aba04 in aux_timer_set (aux_reg_detail=0x5555562eedb0 <arc_aux_regs_detail+2352>, val=500000, data=0x5555565280a0) at ../target/arc/timer.c:436
| #9  0x00005555559a4cb2 in helper_sr (env=0x5555565280a0, val=500000, aux=35) at ../target/arc/op_helper.c:209
| #10 0x00007fffb041dc6a in code_gen_buffer ()
| #11 0x0000555555a2f6c8 in cpu_tb_exec (cpu=0x55555651f960, itb=0x7fffb041db00 <code_gen_buffer+4315859>) at ../accel/tcg/cpu-exec.c:178
| #12 0x0000555555a304ae in cpu_loop_exec_tb (cpu=0x55555651f960, tb=0x7fffb041db00 <code_gen_buffer+4315859>, last_tb=0x7ffff6013928, tb_exit=0x7ffff6013920)
|     at ../accel/tcg/cpu-exec.c:658
| #13 0x0000555555a307a6 in cpu_exec (cpu=0x55555651f960) at ../accel/tcg/cpu-exec.c:771
| #14 0x0000555555a8f911 in tcg_cpu_exec (cpu=0x55555651f960) at ../accel/tcg/tcg-cpus.c:243
| #15 0x0000555555a8fc12 in tcg_rr_cpu_thread_fn (arg=0x55555651f960) at ../accel/tcg/tcg-cpus.c:346
| #16 0x0000555555c9562e in qemu_thread_start (args=0x5555565339e0) at ../util/qemu-thread-posix.c:521
| #17 0x00007ffff7899609 in start_thread (arg=<optimized out>) at pthread_create.c:477
| #18 0x00007ffff77c0293 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
------------------>8-----------------

Fix that, hinting QEMU about possible IO on access of AUX regs,
which are only possible via LR & SR instructions.

Signed-off-by: Alexey Brodkin <[email protected]>
@abrodkin abrodkin deleted the abrodkin-pci branch March 3, 2021 13:26
cupertinomiranda pushed a commit that referenced this pull request Apr 5, 2021
Incoming enabled bitmaps are busy, because we do
bdrv_dirty_bitmap_create_successor() for them. But disabled bitmaps
being migrated are not marked busy, and user can remove them during the
incoming migration. Then we may crash in cancel_incoming_locked() when
try to remove the bitmap that was already removed by user, like this:

 #0  qemu_mutex_lock_impl (mutex=0x5593d88c50d1, file=0x559680554b20
   "../block/dirty-bitmap.c", line=64) at ../util/qemu-thread-posix.c:77
 #1  bdrv_dirty_bitmaps_lock (bs=0x5593d88c0ee9)
   at ../block/dirty-bitmap.c:64
 #2  bdrv_release_dirty_bitmap (bitmap=0x5596810e9570)
   at ../block/dirty-bitmap.c:362
 #3  cancel_incoming_locked (s=0x559680be8208 <dbm_state+40>)
   at ../migration/block-dirty-bitmap.c:918
 #4  dirty_bitmap_load (f=0x559681d02b10, opaque=0x559680be81e0
   <dbm_state>, version_id=1) at ../migration/block-dirty-bitmap.c:1194
 #5  vmstate_load (f=0x559681d02b10, se=0x559680fb5810)
   at ../migration/savevm.c:908
 #6  qemu_loadvm_section_part_end (f=0x559681d02b10,
   mis=0x559680fb4a30) at ../migration/savevm.c:2473
 #7  qemu_loadvm_state_main (f=0x559681d02b10, mis=0x559680fb4a30)
   at ../migration/savevm.c:2626
 #8  postcopy_ram_listen_thread (opaque=0x0)
   at ../migration/savevm.c:1871
 #9  qemu_thread_start (args=0x5596817ccd10)
   at ../util/qemu-thread-posix.c:521
 #10 start_thread () at /lib64/libpthread.so.0
 #11 clone () at /lib64/libc.so.6

Note bs pointer taken from bitmap: it's definitely bad aligned. That's
because we are in use after free, bitmap is already freed.

So, let's make disabled bitmaps (being migrated) busy during incoming
migration.

Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]>
Signed-off-by: Stefan Hajnoczi <[email protected]>
Message-Id: <[email protected]>
cupertinomiranda pushed a commit that referenced this pull request Apr 5, 2021
When building with --enable-sanitizers we get:

  Direct leak of 32 byte(s) in 2 object(s) allocated from:
      #0 0x5618479ec7cf in malloc (qemu-system-aarch64+0x233b7cf)
      #1 0x7f675745f958 in g_malloc (/lib64/libglib-2.0.so.0+0x58958)
      #2 0x561847f02ca2 in usb_packet_init hw/usb/core.c:531:5
      #3 0x561848df4df4 in usb_ehci_init hw/usb/hcd-ehci.c:2575:5
      #4 0x561847c119ac in ehci_sysbus_init hw/usb/hcd-ehci-sysbus.c:73:5
      #5 0x56184a5bdab8 in object_init_with_type qom/object.c:375:9
      #6 0x56184a5bd955 in object_init_with_type qom/object.c:371:9
      #7 0x56184a5a2bda in object_initialize_with_type qom/object.c:517:5
      #8 0x56184a5a24d5 in object_initialize qom/object.c:536:5
      #9 0x56184a5a2f6c in object_initialize_child_with_propsv qom/object.c:566:5
      #10 0x56184a5a2e60 in object_initialize_child_with_props qom/object.c:549:10
      #11 0x56184a5a3a1e in object_initialize_child_internal qom/object.c:603:5
      #12 0x561849542d18 in npcm7xx_init hw/arm/npcm7xx.c:427:5

Similarly to commit d710e1e ("usb: ehci: fix memory leak in
ehci"), fix by calling usb_ehci_finalize() to free the USBPacket.

Fixes: 7341ea0
Signed-off-by: Philippe Mathieu-Daudé <[email protected]>
Reviewed-by: Thomas Huth <[email protected]>
Message-Id: <[email protected]>
Signed-off-by: Gerd Hoffmann <[email protected]>
cupertinomiranda pushed a commit that referenced this pull request Jul 21, 2021
Incoming enabled bitmaps are busy, because we do
bdrv_dirty_bitmap_create_successor() for them. But disabled bitmaps
being migrated are not marked busy, and user can remove them during the
incoming migration. Then we may crash in cancel_incoming_locked() when
try to remove the bitmap that was already removed by user, like this:

 #0  qemu_mutex_lock_impl (mutex=0x5593d88c50d1, file=0x559680554b20
   "../block/dirty-bitmap.c", line=64) at ../util/qemu-thread-posix.c:77
 #1  bdrv_dirty_bitmaps_lock (bs=0x5593d88c0ee9)
   at ../block/dirty-bitmap.c:64
 #2  bdrv_release_dirty_bitmap (bitmap=0x5596810e9570)
   at ../block/dirty-bitmap.c:362
 #3  cancel_incoming_locked (s=0x559680be8208 <dbm_state+40>)
   at ../migration/block-dirty-bitmap.c:918
 #4  dirty_bitmap_load (f=0x559681d02b10, opaque=0x559680be81e0
   <dbm_state>, version_id=1) at ../migration/block-dirty-bitmap.c:1194
 #5  vmstate_load (f=0x559681d02b10, se=0x559680fb5810)
   at ../migration/savevm.c:908
 #6  qemu_loadvm_section_part_end (f=0x559681d02b10,
   mis=0x559680fb4a30) at ../migration/savevm.c:2473
 #7  qemu_loadvm_state_main (f=0x559681d02b10, mis=0x559680fb4a30)
   at ../migration/savevm.c:2626
 #8  postcopy_ram_listen_thread (opaque=0x0)
   at ../migration/savevm.c:1871
 #9  qemu_thread_start (args=0x5596817ccd10)
   at ../util/qemu-thread-posix.c:521
 #10 start_thread () at /lib64/libpthread.so.0
 #11 clone () at /lib64/libc.so.6

Note bs pointer taken from bitmap: it's definitely bad aligned. That's
because we are in use after free, bitmap is already freed.

So, let's make disabled bitmaps (being migrated) busy during incoming
migration.

Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]>
Signed-off-by: Stefan Hajnoczi <[email protected]>
Message-Id: <[email protected]>
cupertinomiranda pushed a commit that referenced this pull request Jul 21, 2021
When building with --enable-sanitizers we get:

  Direct leak of 32 byte(s) in 2 object(s) allocated from:
      #0 0x5618479ec7cf in malloc (qemu-system-aarch64+0x233b7cf)
      #1 0x7f675745f958 in g_malloc (/lib64/libglib-2.0.so.0+0x58958)
      #2 0x561847f02ca2 in usb_packet_init hw/usb/core.c:531:5
      #3 0x561848df4df4 in usb_ehci_init hw/usb/hcd-ehci.c:2575:5
      #4 0x561847c119ac in ehci_sysbus_init hw/usb/hcd-ehci-sysbus.c:73:5
      #5 0x56184a5bdab8 in object_init_with_type qom/object.c:375:9
      #6 0x56184a5bd955 in object_init_with_type qom/object.c:371:9
      #7 0x56184a5a2bda in object_initialize_with_type qom/object.c:517:5
      #8 0x56184a5a24d5 in object_initialize qom/object.c:536:5
      #9 0x56184a5a2f6c in object_initialize_child_with_propsv qom/object.c:566:5
      #10 0x56184a5a2e60 in object_initialize_child_with_props qom/object.c:549:10
      #11 0x56184a5a3a1e in object_initialize_child_internal qom/object.c:603:5
      #12 0x561849542d18 in npcm7xx_init hw/arm/npcm7xx.c:427:5

Similarly to commit d710e1e ("usb: ehci: fix memory leak in
ehci"), fix by calling usb_ehci_finalize() to free the USBPacket.

Fixes: 7341ea0
Signed-off-by: Philippe Mathieu-Daudé <[email protected]>
Reviewed-by: Thomas Huth <[email protected]>
Message-Id: <[email protected]>
Signed-off-by: Gerd Hoffmann <[email protected]>
kolerov pushed a commit that referenced this pull request Jul 13, 2024
Otherwise tcg_handle_interrupt() triggers an assertion failure:

  #5  0x0000555555c97369 in tcg_handle_interrupt (cpu=0x555557434cb0, mask=2) at ../accel/tcg/tcg-accel-ops.c:83
  #6  tcg_handle_interrupt (cpu=0x555557434cb0, mask=2) at ../accel/tcg/tcg-accel-ops.c:81
  #7  0x0000555555b4d58b in pic_irq_request (opaque=<optimized out>, irq=<optimized out>, level=1) at ../hw/i386/x86.c:555
  #8  0x0000555555b4f218 in gsi_handler (opaque=0x5555579423d0, n=13, level=1) at ../hw/i386/x86.c:611
  #9  0x00007fffa42bde14 in code_gen_buffer ()
  #10 0x0000555555c724bb in cpu_tb_exec (cpu=cpu@entry=0x555557434cb0, itb=<optimized out>, tb_exit=tb_exit@entry=0x7fffe9bfd658) at ../accel/tcg/cpu-exec.c:457

Cc: [email protected]
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1808
Reported-by: NyanCatTW1 <https://gitlab.com/a0939712328>
Co-developed-by: Richard Henderson <[email protected]>'
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
(cherry picked from commit c1f27a0)
Signed-off-by: Michael Tokarev <[email protected]>
kolerov pushed a commit that referenced this pull request Jul 13, 2024
virtio_load() as a whole should run in coroutine context because it
reads from the migration stream and we don't want this to block.

However, it calls virtio_set_features_nocheck() and devices don't
expect their .set_features callback to run in a coroutine and therefore
call functions that may not be called in coroutine context. To fix this,
drop out of coroutine context for calling virtio_set_features_nocheck().

Without this fix, the following crash was reported:

  #0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
  #1  0x00007efc738c05d3 in __pthread_kill_internal (signo=6, threadid=<optimized out>) at pthread_kill.c:78
  #2  0x00007efc73873d26 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
  #3  0x00007efc738477f3 in __GI_abort () at abort.c:79
  #4  0x00007efc7384771b in __assert_fail_base (fmt=0x7efc739dbcb8 "", assertion=assertion@entry=0x560aebfbf5cf "!qemu_in_coroutine()",
     file=file@entry=0x560aebfcd2d4 "../block/graph-lock.c", line=line@entry=275, function=function@entry=0x560aebfcd34d "void bdrv_graph_rdlock_main_loop(void)") at assert.c:92
  #5  0x00007efc7386ccc6 in __assert_fail (assertion=0x560aebfbf5cf "!qemu_in_coroutine()", file=0x560aebfcd2d4 "../block/graph-lock.c", line=275,
     function=0x560aebfcd34d "void bdrv_graph_rdlock_main_loop(void)") at assert.c:101
  #6  0x0000560aebcd8dd6 in bdrv_register_buf ()
  #7  0x0000560aeb97ed97 in ram_block_added.llvm ()
  #8  0x0000560aebb8303f in ram_block_add.llvm ()
  #9  0x0000560aebb834fa in qemu_ram_alloc_internal.llvm ()
  #10 0x0000560aebb2ac98 in vfio_region_mmap ()
  #11 0x0000560aebb3ea0f in vfio_bars_register ()
  #12 0x0000560aebb3c628 in vfio_realize ()
  #13 0x0000560aeb90f0c2 in pci_qdev_realize ()
  #14 0x0000560aebc40305 in device_set_realized ()
  #15 0x0000560aebc48e07 in property_set_bool.llvm ()
  #16 0x0000560aebc46582 in object_property_set ()
  #17 0x0000560aebc4cd58 in object_property_set_qobject ()
  #18 0x0000560aebc46ba7 in object_property_set_bool ()
  #19 0x0000560aeb98b3ca in qdev_device_add_from_qdict ()
  #20 0x0000560aebb1fbaf in virtio_net_set_features ()
  #21 0x0000560aebb46b51 in virtio_set_features_nocheck ()
  #22 0x0000560aebb47107 in virtio_load ()
  #23 0x0000560aeb9ae7ce in vmstate_load_state ()
  #24 0x0000560aeb9d2ee9 in qemu_loadvm_state_main ()
  #25 0x0000560aeb9d45e1 in qemu_loadvm_state ()
  #26 0x0000560aeb9bc32c in process_incoming_migration_co.llvm ()
  #27 0x0000560aebeace56 in coroutine_trampoline.llvm ()

Cc: [email protected]
Buglink: https://issues.redhat.com/browse/RHEL-832
Signed-off-by: Kevin Wolf <[email protected]>
Message-ID: <[email protected]>
Reviewed-by: Stefan Hajnoczi <[email protected]>
Signed-off-by: Kevin Wolf <[email protected]>
(cherry picked from commit 92e2e6a)
Signed-off-by: Michael Tokarev <[email protected]>
kolerov pushed a commit that referenced this pull request Jul 13, 2024
If there is a pending DMA operation during ide_bus_reset(), the fact
that the IDEState is already reset before the operation is canceled
can be problematic. In particular, ide_dma_cb() might be called and
then use the reset IDEState which contains the signature after the
reset. When used to construct the IO operation this leads to
ide_get_sector() returning 0 and nsector being 1. This is particularly
bad, because a write command will thus destroy the first sector which
often contains a partition table or similar.

Traces showing the unsolicited write happening with IDEState
0x5595af6949d0 being used after reset:

> ahci_port_write ahci(0x5595af6923f0)[0]: port write [reg:PxSCTL] @ 0x2c: 0x00000300
> ahci_reset_port ahci(0x5595af6923f0)[0]: reset port
> ide_reset IDEstate 0x5595af6949d0
> ide_reset IDEstate 0x5595af694da8
> ide_bus_reset_aio aio_cancel
> dma_aio_cancel dbs=0x7f64600089a0
> dma_blk_cb dbs=0x7f64600089a0 ret=0
> dma_complete dbs=0x7f64600089a0 ret=0 cb=0x5595acd40b30
> ahci_populate_sglist ahci(0x5595af6923f0)[0]
> ahci_dma_prepare_buf ahci(0x5595af6923f0)[0]: prepare buf limit=512 prepared=512
> ide_dma_cb IDEState 0x5595af6949d0; sector_num=0 n=1 cmd=DMA WRITE
> dma_blk_io dbs=0x7f6420802010 bs=0x5595ae2c6c30 offset=0 to_dev=1
> dma_blk_cb dbs=0x7f6420802010 ret=0

> (gdb) p *qiov
> $11 = {iov = 0x7f647c76d840, niov = 1, {{nalloc = 1, local_iov = {iov_base = 0x0,
>       iov_len = 512}}, {__pad = "\001\000\000\000\000\000\000\000\000\000\000",
>       size = 512}}}
> (gdb) bt
> #0  blk_aio_pwritev (blk=0x5595ae2c6c30, offset=0, qiov=0x7f6420802070, flags=0,
>     cb=0x5595ace6f0b0 <dma_blk_cb>, opaque=0x7f6420802010)
>     at ../block/block-backend.c:1682
> #1  0x00005595ace6f185 in dma_blk_cb (opaque=0x7f6420802010, ret=<optimized out>)
>     at ../softmmu/dma-helpers.c:179
> #2  0x00005595ace6f778 in dma_blk_io (ctx=0x5595ae0609f0,
>     sg=sg@entry=0x5595af694d00, offset=offset@entry=0, align=align@entry=512,
>     io_func=io_func@entry=0x5595ace6ee30 <dma_blk_write_io_func>,
>     io_func_opaque=io_func_opaque@entry=0x5595ae2c6c30,
>     cb=0x5595acd40b30 <ide_dma_cb>, opaque=0x5595af6949d0,
>     dir=DMA_DIRECTION_TO_DEVICE) at ../softmmu/dma-helpers.c:244
> #3  0x00005595ace6f90a in dma_blk_write (blk=0x5595ae2c6c30,
>     sg=sg@entry=0x5595af694d00, offset=offset@entry=0, align=align@entry=512,
>     cb=cb@entry=0x5595acd40b30 <ide_dma_cb>, opaque=opaque@entry=0x5595af6949d0)
>     at ../softmmu/dma-helpers.c:280
> #4  0x00005595acd40e18 in ide_dma_cb (opaque=0x5595af6949d0, ret=<optimized out>)
>     at ../hw/ide/core.c:953
> #5  0x00005595ace6f319 in dma_complete (ret=0, dbs=0x7f64600089a0)
>     at ../softmmu/dma-helpers.c:107
> #6  dma_blk_cb (opaque=0x7f64600089a0, ret=0) at ../softmmu/dma-helpers.c:127
> #7  0x00005595ad12227d in blk_aio_complete (acb=0x7f6460005b10)
>     at ../block/block-backend.c:1527
> #8  blk_aio_complete (acb=0x7f6460005b10) at ../block/block-backend.c:1524
> #9  blk_aio_write_entry (opaque=0x7f6460005b10) at ../block/block-backend.c:1594
> #10 0x00005595ad258cfb in coroutine_trampoline (i0=<optimized out>,
>     i1=<optimized out>) at ../util/coroutine-ucontext.c:177

Signed-off-by: Fiona Ebner <[email protected]>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
Tested-by: [email protected]
Message-ID: <[email protected]>
Signed-off-by: Philippe Mathieu-Daudé <[email protected]>
(cherry picked from commit 7d75120)
Signed-off-by: Michael Tokarev <[email protected]>
kolerov pushed a commit that referenced this pull request Jul 13, 2024
There is no architectural requirement that SME implies SVE, but
our implementation currently assumes it. (FEAT_SME_FA64 does
imply SVE.) So if you try to run a CPU with eg "-cpu max,sve=off"
you quickly run into an assert when the guest tries to write to
SMCR_EL1:

#6  0x00007ffff4b38e96 in __GI___assert_fail
    (assertion=0x5555566e69cb "sm", file=0x5555566e5b24 "../../target/arm/helper.c", line=6865, function=0x5555566e82f0 <__PRETTY_FUNCTION__.31> "sve_vqm1_for_el_sm") at ./assert/assert.c:101
#7  0x0000555555ee33aa in sve_vqm1_for_el_sm (env=0x555557d291f0, el=2, sm=false) at ../../target/arm/helper.c:6865
#8  0x0000555555ee3407 in sve_vqm1_for_el (env=0x555557d291f0, el=2) at ../../target/arm/helper.c:6871
#9  0x0000555555ee3724 in smcr_write (env=0x555557d291f0, ri=0x555557da23b0, value=2147483663) at ../../target/arm/helper.c:6995
#10 0x0000555555fd1dba in helper_set_cp_reg64 (env=0x555557d291f0, rip=0x555557da23b0, value=2147483663) at ../../target/arm/tcg/op_helper.c:839
#11 0x00007fff60056781 in code_gen_buffer ()

Avoid this unsupported and slightly odd combination by
disabling SME when SVE is not present.

Cc: [email protected]
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2005
Signed-off-by: Peter Maydell <[email protected]>
Reviewed-by: Richard Henderson <[email protected]>
Message-id: [email protected]
(cherry picked from commit f7767ca)
Signed-off-by: Michael Tokarev <[email protected]>
kolerov pushed a commit that referenced this pull request Jul 13, 2024
…ock_status

Using fleecing backup like in [0] on a qcow2 image (with metadata
preallocation) can lead to the following assertion failure:

> bdrv_co_do_block_status: Assertion `!(ret & BDRV_BLOCK_ZERO)' failed.

In the reproducer [0], it happens because the BDRV_BLOCK_RECURSE flag
will be set by the qcow2 driver, so the caller will recursively check
the file child. Then the BDRV_BLOCK_ZERO set too. Later up the call
chain, in bdrv_co_do_block_status() for the snapshot-access driver,
the assertion failure will happen, because both flags are set.

To fix it, clear the recurse flag after the recursive check was done.

In detail:

> #0  qcow2_co_block_status

Returns 0x45 = BDRV_BLOCK_RECURSE | BDRV_BLOCK_DATA |
BDRV_BLOCK_OFFSET_VALID.

> #1  bdrv_co_do_block_status

Because of the data flag, bdrv_co_do_block_status() will now also set
BDRV_BLOCK_ALLOCATED. Because of the recurse flag,
bdrv_co_do_block_status() for the bdrv_file child will be called,
which returns 0x16 = BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID |
BDRV_BLOCK_ZERO. Now the return value inherits the zero flag.

Returns 0x57 = BDRV_BLOCK_RECURSE | BDRV_BLOCK_DATA |
BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_ZERO.

> #2  bdrv_co_common_block_status_above
> #3  bdrv_co_block_status_above
> #4  bdrv_co_block_status
> #5  cbw_co_snapshot_block_status
> #6  bdrv_co_snapshot_block_status
> #7  snapshot_access_co_block_status
> #8  bdrv_co_do_block_status

Return value is propagated all the way up to here, where the assertion
failure happens, because BDRV_BLOCK_RECURSE and BDRV_BLOCK_ZERO are
both set.

> #9  bdrv_co_common_block_status_above
> #10 bdrv_co_block_status_above
> #11 block_copy_block_status
> #12 block_copy_dirty_clusters
> #13 block_copy_common
> #14 block_copy_async_co_entry
> #15 coroutine_trampoline

[0]:

> #!/bin/bash
> rm /tmp/disk.qcow2
> ./qemu-img create /tmp/disk.qcow2 -o preallocation=metadata -f qcow2 1G
> ./qemu-img create /tmp/fleecing.qcow2 -f qcow2 1G
> ./qemu-img create /tmp/backup.qcow2 -f qcow2 1G
> ./qemu-system-x86_64 --qmp stdio \
> --blockdev qcow2,node-name=node0,file.driver=file,file.filename=/tmp/disk.qcow2 \
> --blockdev qcow2,node-name=node1,file.driver=file,file.filename=/tmp/fleecing.qcow2 \
> --blockdev qcow2,node-name=node2,file.driver=file,file.filename=/tmp/backup.qcow2 \
> <<EOF
> {"execute": "qmp_capabilities"}
> {"execute": "blockdev-add", "arguments": { "driver": "copy-before-write", "file": "node0", "target": "node1", "node-name": "node3" } }
> {"execute": "blockdev-add", "arguments": { "driver": "snapshot-access", "file": "node3", "node-name": "snap0" } }
> {"execute": "blockdev-backup", "arguments": { "device": "snap0", "target": "node1", "sync": "full", "job-id": "backup0" } }
> EOF

Signed-off-by: Fiona Ebner <[email protected]>
Reviewed-by: Vladimir Sementsov-Ogievskiy <[email protected]>
Message-id: [email protected]
Signed-off-by: Stefan Hajnoczi <[email protected]>
(cherry picked from commit 8a9be79)
Signed-off-by: Michael Tokarev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants