Re: [PATCH v4] virtio-pci: Fix the crash that the vector was used after released.

[Jason Wang]

On Thu, Apr 11, 2024 at 12:11 PM Cindy Lu  wrote:
>
> During the booting process of the non-standard image, the behavior of the
> called function in qemu is as follows:
>
> 1. vhost_net_stop() was triggered by guest image. This will call the function
> virtio_pci_set_guest_notifiers() with assgin= false,
> virtio_pci_set_guest_notifiers(） will release the irqfd for vector 0
>
> 2. virtio_reset() was triggered, this will set configure vector to 
> VIRTIO_NO_VECTOR
>
> 3.vhost_net_start() was called (at this time, the configure vector is
> still VIRTIO_NO_VECTOR) and then call virtio_pci_set_guest_notifiers() with
> assgin=true, so the irqfd for vector 0 is still not "init" during this process
>
> 4. The system continues to boot and sets the vector back to 0. After that
> msix_fire_vector_notifier() was triggered to unmask the vector 0 and  meet 
> the crash
>
> To fix the issue, we need to support changing the vector after 
> VIRTIO_CONFIG_S_DRIVER_OK is set.
>
> (gdb) bt
> 0  __pthread_kill_implementation (threadid=, 
> signo=signo@entry=6, no_tid=no_tid@entry=0)
> at pthread_kill.c:44
> 1  0x7fc87148ec53 in __pthread_kill_internal (signo=6, 
> threadid=) at pthread_kill.c:78
> 2  0x7fc87143e956 in __GI_raise (sig=sig@entry=6) at 
> ../sysdeps/posix/raise.c:26
> 3  0x7fc8714287f4 in __GI_abort () at abort.c:79
> 4  0x7fc87142871b in __assert_fail_base
> (fmt=0x7fc8715bbde0 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
> assertion=0x5606413efd53 "ret == 0", file=0x5606413ef87d 
> "../accel/kvm/kvm-all.c", line=1837, function=) at assert.c:92
> 5  0x7fc871437536 in __GI___assert_fail
> (assertion=0x5606413efd53 "ret == 0", file=0x5606413ef87d 
> "../accel/kvm/kvm-all.c", line=1837, function=0x5606413f06f0 
> <__PRETTY_FUNCTION__.19> "kvm_irqchip_commit_routes") at assert.c:101
> 6  0x560640f884b5 in kvm_irqchip_commit_routes (s=0x560642cae1f0) at 
> ../accel/kvm/kvm-all.c:1837
> 7  0x560640c98f8e in virtio_pci_one_vector_unmask
> (proxy=0x560643c65f00, queue_no=4294967295, vector=0, msg=..., 
> n=0x560643c6e4c8)
> at ../hw/virtio/virtio-pci.c:1005
> 8  0x560640c99201 in virtio_pci_vector_unmask (dev=0x560643c65f00, 
> vector=0, msg=...)
> at ../hw/virtio/virtio-pci.c:1070
> 9  0x560640bc402e in msix_fire_vector_notifier (dev=0x560643c65f00, 
> vector=0, is_masked=false)
> at ../hw/pci/msix.c:120
> 10 0x560640bc40f1 in msix_handle_mask_update (dev=0x560643c65f00, 
> vector=0, was_masked=true)
> at ../hw/pci/msix.c:140
> 11 0x560640bc4503 in msix_table_mmio_write (opaque=0x560643c65f00, 
> addr=12, val=0, size=4)
> at ../hw/pci/msix.c:231
> 12 0x560640f26d83 in memory_region_write_accessor
> (mr=0x560643c66540, addr=12, value=0x7fc86b7bc628, size=4, shift=0, 
> mask=4294967295, attrs=...)
> at ../system/memory.c:497
> 13 0x560640f270a6 in access_with_adjusted_size
>
>  (addr=12, value=0x7fc86b7bc628, size=4, access_size_min=1, 
> access_size_max=4, access_fn=0x560640f26c8d , 
> mr=0x560643c66540, attrs=...) at ../system/memory.c:573
> 14 0x560640f2a2b5 in memory_region_dispatch_write (mr=0x560643c66540, 
> addr=12, data=0, op=MO_32, attrs=...)
> at ../system/memory.c:1521
> 15 0x560640f37bac in flatview_write_continue
> (fv=0x7fc65805e0b0, addr=4273803276, attrs=..., ptr=0x7fc871e9c028, 
> len=4, addr1=12, l=4, mr=0x560643c66540)
> at ../system/physmem.c:2714
> 16 0x560640f37d0f in flatview_write
> (fv=0x7fc65805e0b0, addr=4273803276, attrs=..., buf=0x7fc871e9c028, 
> len=4) at ../system/physmem.c:2756
> 17 0x560640f380bf in address_space_write
> (as=0x560642161ae0 , addr=4273803276, attrs=..., 
> buf=0x7fc871e9c028, len=4)
> at ../system/physmem.c:2863
> 18 0x560640f3812c in address_space_rw
> (as=0x560642161ae0 , addr=4273803276, attrs=..., 
> buf=0x7fc871e9c028, len=4, is_write=true) at ../system/physmem.c:2873
> --Type  for more, q to quit, c to continue without paging--
> 19 0x560640f8aa55 in kvm_cpu_exec (cpu=0x560642f205e0) at 
> ../accel/kvm/kvm-all.c:2915
> 20 0x560640f8d731 in kvm_vcpu_thread_fn (arg=0x560642f205e0) at 
> ../accel/kvm/kvm-accel-ops.c:51
> 21 0x5606411949f4 in qemu_thread_start (args=0x560642f292b0) at 
> ../util/qemu-thread-posix.c:541
> 22 0x7fc87148cdcd in start_thread (arg=) at 
> pthread_create.c:442
> 23 0x7fc871512630 in clone3 () at 
> ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
> (gdb)
> Signed-off-by: Cindy Lu 

Fixes: f9a09ca3ea ("vhost: add support for configure interrupt")
Cc: qemu-sta...@nongnu.org

Acked-by: Jason Wang 

Thanks

> ---
>  hw/virtio/virtio-pci.c | 30 ++
>  1 file changed, 30 insertions(+)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 1a7039fb0c..f124a9e20b 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1570,7 +1570,22 @@ static void virtio_pci_common_write(void *opaque, 
> hwaddr addr,
>  } else {

Re: [PATCH v4] virtio-pci: Fix the crash that the vector was used after released.

[Michael S. Tsirkin]

On Thu, Apr 11, 2024 at 12:11:30PM +0800, Cindy Lu wrote:
> During the booting process of the non-standard image, the behavior of the
> called function in qemu is as follows:
> 
> 1. vhost_net_stop() was triggered by guest image. This will call the function
> virtio_pci_set_guest_notifiers() with assgin= false,
> virtio_pci_set_guest_notifiers(） will release the irqfd for vector 0
> 
> 2. virtio_reset() was triggered, this will set configure vector to 
> VIRTIO_NO_VECTOR
> 
> 3.vhost_net_start() was called (at this time, the configure vector is
> still VIRTIO_NO_VECTOR) and then call virtio_pci_set_guest_notifiers() with
> assgin=true, so the irqfd for vector 0 is still not "init" during this process
> 
> 4. The system continues to boot and sets the vector back to 0. After that
> msix_fire_vector_notifier() was triggered to unmask the vector 0 and  meet 
> the crash
> 
> To fix the issue, we need to support changing the vector after 
> VIRTIO_CONFIG_S_DRIVER_OK is set.
> 
> (gdb) bt
> 0  __pthread_kill_implementation (threadid=, 
> signo=signo@entry=6, no_tid=no_tid@entry=0)
> at pthread_kill.c:44
> 1  0x7fc87148ec53 in __pthread_kill_internal (signo=6, 
> threadid=) at pthread_kill.c:78
> 2  0x7fc87143e956 in __GI_raise (sig=sig@entry=6) at 
> ../sysdeps/posix/raise.c:26
> 3  0x7fc8714287f4 in __GI_abort () at abort.c:79
> 4  0x7fc87142871b in __assert_fail_base
> (fmt=0x7fc8715bbde0 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
> assertion=0x5606413efd53 "ret == 0", file=0x5606413ef87d 
> "../accel/kvm/kvm-all.c", line=1837, function=) at assert.c:92
> 5  0x7fc871437536 in __GI___assert_fail
> (assertion=0x5606413efd53 "ret == 0", file=0x5606413ef87d 
> "../accel/kvm/kvm-all.c", line=1837, function=0x5606413f06f0 
> <__PRETTY_FUNCTION__.19> "kvm_irqchip_commit_routes") at assert.c:101
> 6  0x560640f884b5 in kvm_irqchip_commit_routes (s=0x560642cae1f0) at 
> ../accel/kvm/kvm-all.c:1837
> 7  0x560640c98f8e in virtio_pci_one_vector_unmask
> (proxy=0x560643c65f00, queue_no=4294967295, vector=0, msg=..., 
> n=0x560643c6e4c8)
> at ../hw/virtio/virtio-pci.c:1005
> 8  0x560640c99201 in virtio_pci_vector_unmask (dev=0x560643c65f00, 
> vector=0, msg=...)
> at ../hw/virtio/virtio-pci.c:1070
> 9  0x560640bc402e in msix_fire_vector_notifier (dev=0x560643c65f00, 
> vector=0, is_masked=false)
> at ../hw/pci/msix.c:120
> 10 0x560640bc40f1 in msix_handle_mask_update (dev=0x560643c65f00, 
> vector=0, was_masked=true)
> at ../hw/pci/msix.c:140
> 11 0x560640bc4503 in msix_table_mmio_write (opaque=0x560643c65f00, 
> addr=12, val=0, size=4)
> at ../hw/pci/msix.c:231
> 12 0x560640f26d83 in memory_region_write_accessor
> (mr=0x560643c66540, addr=12, value=0x7fc86b7bc628, size=4, shift=0, 
> mask=4294967295, attrs=...)
> at ../system/memory.c:497
> 13 0x560640f270a6 in access_with_adjusted_size
> 
>  (addr=12, value=0x7fc86b7bc628, size=4, access_size_min=1, 
> access_size_max=4, access_fn=0x560640f26c8d , 
> mr=0x560643c66540, attrs=...) at ../system/memory.c:573
> 14 0x560640f2a2b5 in memory_region_dispatch_write (mr=0x560643c66540, 
> addr=12, data=0, op=MO_32, attrs=...)
> at ../system/memory.c:1521
> 15 0x560640f37bac in flatview_write_continue
> (fv=0x7fc65805e0b0, addr=4273803276, attrs=..., ptr=0x7fc871e9c028, 
> len=4, addr1=12, l=4, mr=0x560643c66540)
> at ../system/physmem.c:2714
> 16 0x560640f37d0f in flatview_write
> (fv=0x7fc65805e0b0, addr=4273803276, attrs=..., buf=0x7fc871e9c028, 
> len=4) at ../system/physmem.c:2756
> 17 0x560640f380bf in address_space_write
> (as=0x560642161ae0 , addr=4273803276, attrs=..., 
> buf=0x7fc871e9c028, len=4)
> at ../system/physmem.c:2863
> 18 0x560640f3812c in address_space_rw
> (as=0x560642161ae0 , addr=4273803276, attrs=..., 
> buf=0x7fc871e9c028, len=4, is_write=true) at ../system/physmem.c:2873
> --Type  for more, q to quit, c to continue without paging--
> 19 0x560640f8aa55 in kvm_cpu_exec (cpu=0x560642f205e0) at 
> ../accel/kvm/kvm-all.c:2915
> 20 0x560640f8d731 in kvm_vcpu_thread_fn (arg=0x560642f205e0) at 
> ../accel/kvm/kvm-accel-ops.c:51
> 21 0x5606411949f4 in qemu_thread_start (args=0x560642f292b0) at 
> ../util/qemu-thread-posix.c:541
> 22 0x7fc87148cdcd in start_thread (arg=) at 
> pthread_create.c:442
> 23 0x7fc871512630 in clone3 () at 
> ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
> (gdb)
> Signed-off-by: Cindy Lu 

Empty line is needed before the signature :)


> ---
>  hw/virtio/virtio-pci.c | 30 ++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 1a7039fb0c..f124a9e20b 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1570,7 +1570,22 @@ static void virtio_pci_common_write(void *opaque, 
> hwaddr addr,
>  } else {
>  val = VIRTIO_NO_VECTOR;
>  }
> +

[PATCH v4] virtio-pci: Fix the crash that the vector was used after released.

[Cindy Lu]

During the booting process of the non-standard image, the behavior of the
called function in qemu is as follows:

1. vhost_net_stop() was triggered by guest image. This will call the function
virtio_pci_set_guest_notifiers() with assgin= false,
virtio_pci_set_guest_notifiers(） will release the irqfd for vector 0

2. virtio_reset() was triggered, this will set configure vector to 
VIRTIO_NO_VECTOR

3.vhost_net_start() was called (at this time, the configure vector is
still VIRTIO_NO_VECTOR) and then call virtio_pci_set_guest_notifiers() with
assgin=true, so the irqfd for vector 0 is still not "init" during this process

4. The system continues to boot and sets the vector back to 0. After that
msix_fire_vector_notifier() was triggered to unmask the vector 0 and  meet the 
crash

To fix the issue, we need to support changing the vector after 
VIRTIO_CONFIG_S_DRIVER_OK is set.

(gdb) bt
0  __pthread_kill_implementation (threadid=, 
signo=signo@entry=6, no_tid=no_tid@entry=0)
at pthread_kill.c:44
1  0x7fc87148ec53 in __pthread_kill_internal (signo=6, threadid=) at pthread_kill.c:78
2  0x7fc87143e956 in __GI_raise (sig=sig@entry=6) at 
../sysdeps/posix/raise.c:26
3  0x7fc8714287f4 in __GI_abort () at abort.c:79
4  0x7fc87142871b in __assert_fail_base
(fmt=0x7fc8715bbde0 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
assertion=0x5606413efd53 "ret == 0", file=0x5606413ef87d 
"../accel/kvm/kvm-all.c", line=1837, function=) at assert.c:92
5  0x7fc871437536 in __GI___assert_fail
(assertion=0x5606413efd53 "ret == 0", file=0x5606413ef87d 
"../accel/kvm/kvm-all.c", line=1837, function=0x5606413f06f0 
<__PRETTY_FUNCTION__.19> "kvm_irqchip_commit_routes") at assert.c:101
6  0x560640f884b5 in kvm_irqchip_commit_routes (s=0x560642cae1f0) at 
../accel/kvm/kvm-all.c:1837
7  0x560640c98f8e in virtio_pci_one_vector_unmask
(proxy=0x560643c65f00, queue_no=4294967295, vector=0, msg=..., 
n=0x560643c6e4c8)
at ../hw/virtio/virtio-pci.c:1005
8  0x560640c99201 in virtio_pci_vector_unmask (dev=0x560643c65f00, 
vector=0, msg=...)
at ../hw/virtio/virtio-pci.c:1070
9  0x560640bc402e in msix_fire_vector_notifier (dev=0x560643c65f00, 
vector=0, is_masked=false)
at ../hw/pci/msix.c:120
10 0x560640bc40f1 in msix_handle_mask_update (dev=0x560643c65f00, vector=0, 
was_masked=true)
at ../hw/pci/msix.c:140
11 0x560640bc4503 in msix_table_mmio_write (opaque=0x560643c65f00, addr=12, 
val=0, size=4)
at ../hw/pci/msix.c:231
12 0x560640f26d83 in memory_region_write_accessor
(mr=0x560643c66540, addr=12, value=0x7fc86b7bc628, size=4, shift=0, 
mask=4294967295, attrs=...)
at ../system/memory.c:497
13 0x560640f270a6 in access_with_adjusted_size

 (addr=12, value=0x7fc86b7bc628, size=4, access_size_min=1, 
access_size_max=4, access_fn=0x560640f26c8d , 
mr=0x560643c66540, attrs=...) at ../system/memory.c:573
14 0x560640f2a2b5 in memory_region_dispatch_write (mr=0x560643c66540, 
addr=12, data=0, op=MO_32, attrs=...)
at ../system/memory.c:1521
15 0x560640f37bac in flatview_write_continue
(fv=0x7fc65805e0b0, addr=4273803276, attrs=..., ptr=0x7fc871e9c028, len=4, 
addr1=12, l=4, mr=0x560643c66540)
at ../system/physmem.c:2714
16 0x560640f37d0f in flatview_write
(fv=0x7fc65805e0b0, addr=4273803276, attrs=..., buf=0x7fc871e9c028, len=4) 
at ../system/physmem.c:2756
17 0x560640f380bf in address_space_write
(as=0x560642161ae0 , addr=4273803276, attrs=..., 
buf=0x7fc871e9c028, len=4)
at ../system/physmem.c:2863
18 0x560640f3812c in address_space_rw
(as=0x560642161ae0 , addr=4273803276, attrs=..., 
buf=0x7fc871e9c028, len=4, is_write=true) at ../system/physmem.c:2873
--Type  for more, q to quit, c to continue without paging--
19 0x560640f8aa55 in kvm_cpu_exec (cpu=0x560642f205e0) at 
../accel/kvm/kvm-all.c:2915
20 0x560640f8d731 in kvm_vcpu_thread_fn (arg=0x560642f205e0) at 
../accel/kvm/kvm-accel-ops.c:51
21 0x5606411949f4 in qemu_thread_start (args=0x560642f292b0) at 
../util/qemu-thread-posix.c:541
22 0x7fc87148cdcd in start_thread (arg=) at 
pthread_create.c:442
23 0x7fc871512630 in clone3 () at 
../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
(gdb)
Signed-off-by: Cindy Lu 
---
 hw/virtio/virtio-pci.c | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 1a7039fb0c..f124a9e20b 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1570,7 +1570,22 @@ static void virtio_pci_common_write(void *opaque, hwaddr 
addr,
 } else {
 val = VIRTIO_NO_VECTOR;
 }
+vector = vdev->config_vector;
 vdev->config_vector = val;
+/*
+ * If the device uses irqfd and the vector changes after DRIVER_OK is
+ * set, we need to release the old vector and set up the new one.
+ */
+if ((vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) &&
+

?????? [PATCH RFC v1]display: fix heap use after free in cursor_put

[?glym]

Hi


During the test with logging, I found that there may be a conflict between the 
logic of updating the refcount in vnc_dpy_cursor_define() and 
QXL_CURSOR_SETaction, same as dpy_cursor_define() after 
commit385ac97f,and the atomic operation needs to be ensured;


The first thoughts are as follows??only lock cursor_unref/cursor_ref with 
ssd.lock??Butit seems we can't get ssd.lock within dpy_cursor_define??so 
if we can't lock The top-level functionqemu_spice_cursor_refresh_bh()??


--- a/hw/display/qxl-render.c
+++ b/hw/display/qxl-render.c
@@ -336,6 +336,7 @@ int qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
}
qemu_mutex_lock(qxl-ssd.lock);
if (qxl-ssd.cursor) {
+  // other thread
  
cursor_unref(qxl-ssd.cursor);
}
qxl-ssd.cursor = c;
diff --git a/ui/console.c b/ui/console.c
index 43226c5c14..31dbd8fc6f 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -985,8 +985,10 @@ void dpy_cursor_define(QemuConsole *c, QEMUCursor *cursor)
  DisplayState *s = c-ds;
  DisplayChangeListener *dcl;

+  //lock, main thread
  cursor_unref(con-cursor);
  con-cursor = cursor_ref(cursor);
+  //unlock
  if (!qemu_console_is_visible(c)) {
return;
  }





----
??: 
   "Marc-Andr?? Lureau" 
   
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#do-not-send-as-an-attachment

The patch is doing too much changes to the ssd.lock usage without
explaining in detail which race and how it solved it.

In particular, ui/spice-display.c usage seems safer before your
change, since it takes the lock on display_refresh and
display_mouse_define. It properly temporarily releases the lock before
calling the dpy_mouse_set() and dpy_cursor_define() as well.

To me, it looks like the only offender is qxl_spice_reset_cursor(),
which lacks locking before unrefing.

Could you confirm this hypothesis if you are able to reproduce the issue?

thanks

Re: [PATCH v3] virtio-pci: Fix the crash that the vector was used after released.

[Cindy Lu]

On Thu, Apr 11, 2024 at 12:18 AM Michael S. Tsirkin  wrote:
>
> On Thu, Apr 11, 2024 at 12:12:00AM +0800, Cindy Lu wrote:
> > During the booting process of the non-standard image, the behavior of the
> > called function in qemu is as follows:
> >
> > 1. vhost_net_stop() was triggered by guest image. This will call the 
> > function
> > virtio_pci_set_guest_notifiers() with assgin= false,
> > virtio_pci_set_guest_notifiers(） will release the irqfd for vector 0
> >
> > 2. virtio_reset() was triggered, this will set configure vector to 
> > VIRTIO_NO_VECTOR
> >
> > 3.vhost_net_start() was called (at this time, the configure vector is
> > still VIRTIO_NO_VECTOR) and then call virtio_pci_set_guest_notifiers() with
> > assgin=true, so the irqfd for vector 0 is still not "init" during this 
> > process
> >
> > 4. The system continues to boot and sets the vector back to 0. After that
> > msix_fire_vector_notifier() was triggered to unmask the vector 0 and  meet 
> > the crash
> >
> > To fix the issue, we need to support changing the vector after 
> > VIRTIO_CONFIG_S_DRIVER_OK is set.
> >
> > (gdb) bt
> > 0  __pthread_kill_implementation (threadid=, 
> > signo=signo@entry=6, no_tid=no_tid@entry=0)
> > at pthread_kill.c:44
> > 1  0x7fc87148ec53 in __pthread_kill_internal (signo=6, 
> > threadid=) at pthread_kill.c:78
> > 2  0x7fc87143e956 in __GI_raise (sig=sig@entry=6) at 
> > ../sysdeps/posix/raise.c:26
> > 3  0x7fc8714287f4 in __GI_abort () at abort.c:79
> > 4  0x7fc87142871b in __assert_fail_base
> > (fmt=0x7fc8715bbde0 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
> > assertion=0x5606413efd53 "ret == 0", file=0x5606413ef87d 
> > "../accel/kvm/kvm-all.c", line=1837, function=) at 
> > assert.c:92
> > 5  0x7fc871437536 in __GI___assert_fail
> > (assertion=0x5606413efd53 "ret == 0", file=0x5606413ef87d 
> > "../accel/kvm/kvm-all.c", line=1837, function=0x5606413f06f0 
> > <__PRETTY_FUNCTION__.19> "kvm_irqchip_commit_routes") at assert.c:101
> > 6  0x560640f884b5 in kvm_irqchip_commit_routes (s=0x560642cae1f0) at 
> > ../accel/kvm/kvm-all.c:1837
> > 7  0x560640c98f8e in virtio_pci_one_vector_unmask
> > (proxy=0x560643c65f00, queue_no=4294967295, vector=0, msg=..., 
> > n=0x560643c6e4c8)
> > at ../hw/virtio/virtio-pci.c:1005
> > 8  0x560640c99201 in virtio_pci_vector_unmask (dev=0x560643c65f00, 
> > vector=0, msg=...)
> > at ../hw/virtio/virtio-pci.c:1070
> > 9  0x560640bc402e in msix_fire_vector_notifier (dev=0x560643c65f00, 
> > vector=0, is_masked=false)
> > at ../hw/pci/msix.c:120
> > 10 0x560640bc40f1 in msix_handle_mask_update (dev=0x560643c65f00, 
> > vector=0, was_masked=true)
> > at ../hw/pci/msix.c:140
> > 11 0x560640bc4503 in msix_table_mmio_write (opaque=0x560643c65f00, 
> > addr=12, val=0, size=4)
> > at ../hw/pci/msix.c:231
> > 12 0x560640f26d83 in memory_region_write_accessor
> > (mr=0x560643c66540, addr=12, value=0x7fc86b7bc628, size=4, shift=0, 
> > mask=4294967295, attrs=...)
> > at ../system/memory.c:497
> > 13 0x560640f270a6 in access_with_adjusted_size
> >
> >  (addr=12, value=0x7fc86b7bc628, size=4, access_size_min=1, 
> > access_size_max=4, access_fn=0x560640f26c8d , 
> > mr=0x560643c66540, attrs=...) at ../system/memory.c:573
> > 14 0x560640f2a2b5 in memory_region_dispatch_write (mr=0x560643c66540, 
> > addr=12, data=0, op=MO_32, attrs=...)
> > at ../system/memory.c:1521
> > 15 0x560640f37bac in flatview_write_continue
> > (fv=0x7fc65805e0b0, addr=4273803276, attrs=..., ptr=0x7fc871e9c028, 
> > len=4, addr1=12, l=4, mr=0x560643c66540)
> > at ../system/physmem.c:2714
> > 16 0x560640f37d0f in flatview_write
> > (fv=0x7fc65805e0b0, addr=4273803276, attrs=..., buf=0x7fc871e9c028, 
> > len=4) at ../system/physmem.c:2756
> > 17 0x560640f380bf in address_space_write
> > (as=0x560642161ae0 , addr=4273803276, attrs=..., 
> > buf=0x7fc871e9c028, len=4)
> > at ../system/physmem.c:2863
> > 18 0x560640f3812c in address_space_rw
> > (as=0x560642161ae0 , addr=4273803276, attrs=..., 
> > buf=0x7fc871e9c028, len=4, is_write=true) at ../system/physmem.c:2873
> > --Type  for more, q to quit, c to continue without paging--
> > 19 0x560640f8aa55 in kvm_cpu_exec (cpu=0x560642f205e0) at 
> > ../accel/kvm/kvm-all.c:2915
> > 20 0x560640f8d731 in kvm_vcpu_thread_fn (arg=0x560642f205e0) at 
> > ../accel/kvm/kvm-accel-ops.c:51
> > 21 0x5606411949f4 in qemu_thread_start (args=0x560642f292b0) at 
> > ../util/qemu-thread-posix.c:541
> > 22 0x7fc87148cdcd in start_thread (arg=) at 
> > pthread_create.c:442
> > 23 0x7fc871512630 in clone3 () at 
> > ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
> > (gdb)
> > Signed-off-by: Cindy Lu 
> > ---
> >  hw/virtio/virtio-pci.c | 30 ++
> >  1 file changed, 30 insertions(+)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index 1a7039fb0c..b3b1a4a66f 100644
> > --- 

Re: [PATCH v13 00/24] target/arm: Implement FEAT_NMI and FEAT_GICv3_NMI

[Jinjie Ruan via]

On 2024/4/10 20:58, Peter Maydell wrote:
> On Wed, 10 Apr 2024 at 07:19, Jinjie Ruan via  wrote:
>>
>> Ping.
> 
> As I said in my reply on the previous version, we're in
> freeze at the moment, so this patchset is not going anywhere
> until 9.0 releases. I think it's in shape to apply after that.

Thank you! I don't know if there's any other minor issues.

> 
> thanks
> -- PMM

Re: [PATCH 00/12] misc: Remove sprintf() due to macOS deprecation

[BALATON Zoltan]

On Wed, 10 Apr 2024, Richard Henderson wrote:

On 4/10/24 06:06, Philippe Mathieu-Daudé wrote:

Hi,

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.


Is snprintf also deprecated?
It might be easier to convert some of these fixed buffer cases that way, if 
allowed.


I had the same thought as some of these might also have performance 
implications (although most of them are in rarely called places).


Regards,
BALATON Zoltan

[PATCH] hw/isa/vt82c686: Keep track of PIRQ/PINT pins separately

[BALATON Zoltan]

Move calculation of mask after the switch which sets the function
number for PIRQ/PINT pins to make sure the state of these pins are
kept track of separately and IRQ is raised if any of them is active.

Fixes: 7e01bd80c1 hw/isa/vt82c686: Bring back via_isa_set_irq()
Signed-off-by: BALATON Zoltan 
---
Preferably for 9.0 if there will be another RC.

 hw/isa/vt82c686.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index aa91942745..8582ac0322 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -658,7 +658,7 @@ void via_isa_set_irq(PCIDevice *d, int pin, int level)
 ViaISAState *s = VIA_ISA(pci_get_function_0(d));
 uint8_t irq = d->config[PCI_INTERRUPT_LINE], max_irq = 15;
 int f = PCI_FUNC(d->devfn);
-uint16_t mask = BIT(f);
+uint16_t mask;
 
 switch (f) {
 case 0: /* PIRQ/PINT inputs */
@@ -673,6 +673,7 @@ void via_isa_set_irq(PCIDevice *d, int pin, int level)
 }
 
 /* Keep track of the state of all sources */
+mask = BIT(f);
 if (level) {
 s->irq_state[0] |= mask;
 } else {
-- 
2.30.9

Re: Point where target instructions are read

[Gautam Bhat]

On Tue, Apr 9, 2024 at 2:23 PM Peter Maydell  wrote:

> That sounds like a problem with your binary. If the reset vector
> needs to be at 0xFFFE then it needs to be there, and you
> should arrange for it to be built correctly. It doesn't matter
> whether it's an ELF file or a raw binary file, the data has
> to be in the right place. (Generally when objcopy creates a raw
> binary from an ELF file it doesn't change the address where
> the data is, assuming you load the resulting raw binary to the
> right starting address.)
>
> -- PMM

It was a problem with me loading it to the right address. Went through
the manual again and I found that the ROM address starts at 0xC000.
Hence I was supposed to load at that address. Loading at that address
places the reset vector interrupt in the right location. Now I get the
right program counter value which is 0xC000 which is the code in the
ROM.

I went ahead to check if I now get the right opcode but I am still
getting zeroes. Digging through the code when I do the following for
translate:

static void translate(DisasContext *ctx)
{
uint32_t opcode;

opcode = cpu_lduw_code(ctx->msp430_cpu_state,
ctx->base.pc_next);
qemu_fprintf(stderr, "Opcode: 0x%x\n", opcode);
}


cpu_lduw_code calls mmu_index callback. In my callback I have

static int msp430_cpu_mmu_index(CPUState *cp, bool ifetch)
{
return MMU_CODE_DATA_IDX;
}

Here I have just set the MMU_CODE_DATA_IDX to 1 which I know does not
make sense. I am not sure how this index is supposed to be computed.
Any idea on what to look at to understand it?

-Gautam.

Re: [PATCH for-9.0] target/riscv/debug: set tval=pc in breakpoint exceptions

[Daniel Henrique Barboza]

On 3/22/24 00:59, Alistair Francis wrote:

On Wed, Mar 20, 2024 at 7:33 PM Daniel Henrique Barboza
 wrote:


We're not setting (s/m)tval when triggering breakpoints of type 2
(mcontrol) and 6 (mcontrol6). According to the debug spec section
5.7.12, "Match Control Type 6":

"The Privileged Spec says that breakpoint exceptions that occur on
instruction fetches, loads, or stores update the tval CSR with either
zero or the faulting virtual address. The faulting virtual address for
an mcontrol6 trigger with action = 0 is the address being accessed and
which caused that trigger to fire."


Setting zero is valid though. I agree it's probably worth setting a
value, but I don't think we need this for 9.0


In fact, when fixing another related bug I learned that this except I mentioned
from the debug spec doesn't tell the whole story. The section for mtval, priv
spec 3.1.16, says:

"When a trap is taken into M-mode, mtval is either set to zero or written with
exception-specific information to assist software in handling the trap. 
Otherwise,
mtval is never written by the implementation (...)"


So "mtval" zero is valid. However the section for stval, priv spec 4.1.9, says:

"When a trap is taken into S-mode, stval is written with exception-specific 
information
to assist software in handling the trap. Otherwise, stval is never written by 
the
implementation, though it may be explicitly written by software. The hardware 
platform
will specify which exceptions must set stval informatively and which may 
unconditionally
set it to zero."

This a bit more cryptic to me. It doesn't mention "it's either zero or 
written", giving
the impression that we must set it to a valid addr. But then in the end it 
follows up
with  "The hardware platform will specify which exceptions must set stval 
informatively
and which may unconditionally set it to zero.". So I'm not sure whether stval 
can be
zero or not. I *think* zero is kind of valid ...

Due to the apparent ambiguity I can't claim that this patch, and another patch 
I'm about
to send that kind of does the same thing, is 9.0 material. I'll send both as 
9.1.


Thanks,

Daniel






Alistair



A similar text is also found in the Debug spec section 5.7.11 w.r.t.
mcontrol.

Given that we always use action = 0, save the faulting address for the
mcontrol and mcontrol6 trigger breakpoints into env->badaddr, which is
used as as scratch area for traps with address information. 'tval' is
then set during riscv_cpu_do_interrupt().

Reported-by: Joseph Chan 
Fixes: b5f6379d13 ("target/riscv: debug: Implement debug related TCGCPUOps")
Fixes: c472c142a7 ("target/riscv: debug: Add initial support of type 6 trigger")
Signed-off-by: Daniel Henrique Barboza 
---
  target/riscv/cpu_helper.c | 1 +
  target/riscv/debug.c  | 3 +++
  2 files changed, 4 insertions(+)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index ce7322011d..492ca63b1a 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1717,6 +1717,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
  tval = env->bins;
  break;
  case RISCV_EXCP_BREAKPOINT:
+tval = env->badaddr;
  if (cs->watchpoint_hit) {
  tval = cs->watchpoint_hit->hitaddr;
  cs->watchpoint_hit = NULL;
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index e30d99cc2f..b110370ea6 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -798,6 +798,7 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
  if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
  /* check U/S/M bit against current privilege level */
  if ((ctrl >> 3) & BIT(env->priv)) {
+env->badaddr = pc;
  return true;
  }
  }
@@ -810,11 +811,13 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
  if (env->virt_enabled) {
  /* check VU/VS bit against current privilege level */
  if ((ctrl >> 23) & BIT(env->priv)) {
+env->badaddr = pc;
  return true;
  }
  } else {
  /* check U/S/M bit against current privilege level */
  if ((ctrl >> 3) & BIT(env->priv)) {
+env->badaddr = pc;
  return true;
  }
  }
--
2.44.0

Re: [PATCH 12/12] backends/tpm: Use qemu_hexdump_line() to avoid sprintf()

[Stefan Berger]

On 4/10/24 12:06, Philippe Mathieu-Daudé wrote:

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Use qemu_hexdump_line() to avoid sprintf() calls, silencing:

   backends/tpm/tpm_util.c:357:14: warning: 'sprintf' is deprecated:
 This function is provided for compatibility reasons only.
 Due to security concerns inherent in the design of sprintf(3),
 it is highly recommended that you use snprintf(3) instead.
 [-Wdeprecated-declarations]
 p += sprintf(p, "%.2X ", buffer[i]);
  ^ >
Signed-off-by: Philippe Mathieu-Daudé 


Reviewed-by: Stefan Berger 


---
  backends/tpm/tpm_util.c | 24 
  1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
index 1856589c3b..0747af2d1c 100644
--- a/backends/tpm/tpm_util.c
+++ b/backends/tpm/tpm_util.c
@@ -21,6 +21,7 @@
  
  #include "qemu/osdep.h"

  #include "qemu/error-report.h"
+#include "qemu/cutils.h"
  #include "qapi/error.h"
  #include "qapi/visitor.h"
  #include "tpm_int.h"
@@ -336,27 +337,18 @@ void tpm_sized_buffer_reset(TPMSizedBuffer *tsb)
  void tpm_util_show_buffer(const unsigned char *buffer,
size_t buffer_size, const char *string)
  {
-size_t len, i;
-char *line_buffer, *p;
+size_t len;
+char *line, *lineup;
  
  if (!trace_event_get_state_backends(TRACE_TPM_UTIL_SHOW_BUFFER)) {

  return;
  }
  len = MIN(tpm_cmd_get_size(buffer), buffer_size);
  
-/*

- * allocate enough room for 3 chars per buffer entry plus a
- * newline after every 16 chars and a final null terminator.
- */
-line_buffer = g_malloc(len * 3 + (len / 16) + 1);
+line = qemu_hexdump_line(buffer, 0, len, false);
+lineup = g_ascii_strup(line, -1);
+trace_tpm_util_show_buffer(string, len, lineup);
  
-for (i = 0, p = line_buffer; i < len; i++) {

-if (i && !(i % 16)) {
-p += sprintf(p, "\n");
-}
-p += sprintf(p, "%.2X ", buffer[i]);
-}
-trace_tpm_util_show_buffer(string, len, line_buffer);
-
-g_free(line_buffer);
+g_free(line);
+g_free(lineup);
  }

Re: [PATCH v6 09/12] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents

[Jonathan Cameron via]

On Tue, 9 Apr 2024 14:26:51 -0700
fan  wrote:

> On Fri, Apr 05, 2024 at 01:18:56PM +0100, Jonathan Cameron wrote:
> > On Mon, 25 Mar 2024 12:02:27 -0700
> > nifan@gmail.com wrote:
> >   
> > > From: Fan Ni 
> > > 
> > > To simulate FM functionalities for initiating Dynamic Capacity Add
> > > (Opcode 5604h) and Dynamic Capacity Release (Opcode 5605h) as in CXL spec
> > > r3.1 7.6.7.6.5 and 7.6.7.6.6, we implemented two QMP interfaces to issue
> > > add/release dynamic capacity extents requests.
> > > 
> > > With the change, we allow to release an extent only when its DPA range
> > > is contained by a single accepted extent in the device. That is to say,
> > > extent superset release is not supported yet.
> > > 
> > > 1. Add dynamic capacity extents:
> > > 
> > > For example, the command to add two continuous extents (each 128MiB long)
> > > to region 0 (starting at DPA offset 0) looks like below:
> > > 
> > > { "execute": "qmp_capabilities" }
> > > 
> > > { "execute": "cxl-add-dynamic-capacity",
> > >   "arguments": {
> > >   "path": "/machine/peripheral/cxl-dcd0",
> > >   "region-id": 0,
> > >   "extents": [
> > >   {
> > >   "offset": 0,
> > >   "len": 134217728
> > >   },
> > >   {
> > >   "offset": 134217728,
> > >   "len": 134217728
> > >   }  
> > 
> > Hi Fan,
> > 
> > I talk more on this inline, but to me this interface takes multiple extents
> > so that we can treat them as a single 'offer' of capacity. That is they
> > should be linked in the event log with the more flag and the host should
> > have to handle them in one go (I known Ira and Navneet's code doesn't handle
> > this yet, but that doesn't mean QEMU shouldn't).
> > 
> > Alternative for now would be to only support a single entry.  Keep the
> > interface defined to take multiple entries but reject it at runtime.
> > 
> > I don't want to end up with a more complex interface in the end just
> > because we allowed this form to not set the MORE flag today.
> > We will need this to do tagged handling and ultimately sharing, so good
> > to get it right from the start.
> > 
> > For tagged handling I think the right option is to have the tag alongside
> > region-id not in the individual extents.  That way the interface is 
> > naturally
> > used to generate the right description to the host.
> >   
> > >   ]
> > >   }
> > > }  
> Hi Jonathan,
> Thanks for the detailed comments.
> 
> For the QMP interface, I have one question. 
> Do we want the interface to follow exactly as shown in
> Table 7-70 and Table 7-71 in cxl r3.1?

I don't mind if it doesn't as long as it lets us pass reasonable
things in to test the kernel code.  I'd have the interface designed
to allow us to generate the set of records associate with a given
'request'.  E.g. All same tag in the same QMP command.

If we want multiple sets of such records (and the extents to back
them) we can issue multiple calls.

Jonathan



> 
> Fan
> 
> > > 
> > > 2. Release dynamic capacity extents:
> > > 
> > > For example, the command to release an extent of size 128MiB from region 0
> > > (DPA offset 128MiB) looks like below:
> > > 
> > > { "execute": "cxl-release-dynamic-capacity",
> > >   "arguments": {
> > >   "path": "/machine/peripheral/cxl-dcd0",
> > >   "region-id": 0,
> > >   "extents": [
> > >   {
> > >   "offset": 134217728,
> > >   "len": 134217728
> > >   }
> > >   ]
> > >   }
> > > }
> > > 
> > > Signed-off-by: Fan Ni   
> > 
> > 
> >   
> > >  /* to-be-added range should not overlap with range already 
> > > accepted */
> > >  QTAILQ_FOREACH(ent, >dc.extents, node) {
> > > @@ -1585,9 +1586,13 @@ static CXLRetCode cmd_dcd_add_dyn_cap_rsp(const 
> > > struct cxl_cmd *cmd,
> > >  CXLDCExtentList *extent_list = >dc.extents;
> > >  uint32_t i;
> > >  uint64_t dpa, len;
> > > +CXLDCExtent *ent;
> > >  CXLRetCode ret;
> > >  
> > >  if (in->num_entries_updated == 0) {
> > > +/* Always remove the first pending extent when response 
> > > received. */
> > > +ent = QTAILQ_FIRST(>dc.extents_pending);
> > > +cxl_remove_extent_from_extent_list(>dc.extents_pending, 
> > > ent);
> > >  return CXL_MBOX_SUCCESS;
> > >  }
> > >  
> > > @@ -1604,6 +1609,8 @@ static CXLRetCode cmd_dcd_add_dyn_cap_rsp(const 
> > > struct cxl_cmd *cmd,
> > >  
> > >  ret = cxl_dcd_add_dyn_cap_rsp_dry_run(ct3d, in);
> > >  if (ret != CXL_MBOX_SUCCESS) {
> > > +ent = QTAILQ_FIRST(>dc.extents_pending);
> > > +cxl_remove_extent_from_extent_list(>dc.extents_pending, 
> > > ent);  
> > 
> > Ah this deals with the todo I suggest you add to the earlier patch.
> > I'd not mind so much if you hadn't been so thorough on other todo notes ;)
> > Add one in the earlier patch and get rid of ti here like you do below.
> > 
> > However as I note below I think we need to handle these as groups of extents
> > not single extents. That 

Re: [PATCH 00/12] misc: Remove sprintf() due to macOS deprecation

[Richard Henderson]

On 4/10/24 06:06, Philippe Mathieu-Daudé wrote:

Hi,

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.


Is snprintf also deprecated?
It might be easier to convert some of these fixed buffer cases that way, if 
allowed.


r~

[PATCH RFC v1]display: fix heap use after free in cursor_put

[?glym]

0001-display-fix-heap-use-after-free-in-cursor_put.patch
Description: Binary data

Re: [QEMU][PATCH v3 5/7] memory: add MemoryRegion map and unmap callbacks

[Peter Xu]

On Wed, Apr 10, 2024 at 06:44:38PM +0200, Edgar E. Iglesias wrote:
> On Tue, Feb 27, 2024 at 11:37 PM Vikram Garhwal 
> wrote:
> 
> > From: Juergen Gross 
> >
> > In order to support mapping and unmapping guest memory dynamically to
> > and from qemu during address_space_[un]map() operations add the map()
> > and unmap() callbacks to MemoryRegionOps.
> >
> > Those will be used e.g. for Xen grant mappings when performing guest
> > I/Os.
> >
> > Signed-off-by: Juergen Gross 
> > Signed-off-by: Vikram Garhwal 
> >
> 
> 
> Paolo, Peter, David, Phiippe, do you guys have any concerns with this patch?

This introduces a 3rd memory type afaict, neither direct nor !direct.

What happens if someone does address_space_write() to it?  I didn't see it
covered here..

OTOH, the cover letter didn't mention too much either on the big picture:

https://lore.kernel.org/all/20240227223501.28475-1-vikram.garh...@amd.com/

I want to have a quick grasp on whether it's justified worthwhile we should
introduce this complexity to qemu memory core.

Could I request a better cover letter when repost?  It'll be great to
mention things like:

  - what is grant mapping, why it needs to be used, when it can be used (is
it only relevant to vIOMMU=on)?  Some more information on the high
level design using this type or MR would be great.

  - why a 3rd memory type is required?  Do we have other alternatives?

So it's all based on my very limited understanding of reading this:
https://xenbits.xenproject.org/docs/4.3-testing/misc/grant-tables.txt

If it's about cross-vm sharing memory, does it mean that in reality
there are RAMs allocated, but it's only about permission management?
In that case, is there any option we implement it with direct access
mode (however with some extra dynamic permissions applied on top using
some mechanism)?

  - perhaps sold old links would be great too so people can read about the
context when it's not obvious, without a need to copy-paste.

Thanks,

-- 
Peter Xu

[Bug 1926249] Re: postcopy migration fails in hirsute (solved)

[Felipe Alencastro]

Hi @ajkavanagh, this affects focal-hwe, jammy and will affect any new
releases unless this sysctl is set to 1.

** No longer affects: charm-nova-compute

** Also affects: qemu
   Importance: Undecided
   Status: New

** No longer affects: qemu

** Also affects: charm-nova-compute
   Importance: Undecided
   Status: New

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1926249

Title:
  postcopy migration fails in hirsute (solved)

Status in OpenStack Nova Compute Charm:
  New
Status in qemu package in Ubuntu:
  Invalid

Bug description:
  FYI: this is an intended change, can be overwritten via config and
  this bug is mostly to have something puzzled users can find via search
  engines to explain and solve their issue.

  postcopy migration can in some cases be very useful
  => https://wiki.qemu.org/Features/PostCopyLiveMigration

  But with Hirsute kernel being 5.11 that now contains the following upstream 
change
  => 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d0d4730ac2

  Due to that postcopy migration will fail like:

  + lxc exec testkvm-focal-from -- virsh migrate --unsafe --live --postcopy 
--postcopy-after-precopy kvmguest-focal-postcopy qemu+ssh://10.85.93.248/system
  error: internal error: unable to execute QEMU command 
'migrate-set-capabilities': Postcopy is not supported

  This will also apply to e.g. a Focal-HWE kernel once on v5.11 or to
  Focal userspaces in a container under a Hirsute kernel (that is the
  example above).

  This was done for security reasons, if you want/need to re-enable un-limited 
userfault handling to be able to use postcopy again you'd want/need to set the 
control knob to one like:
  $ sudo sysctl -w "vm.unprivileged_userfaultfd=1"

To manage notifications about this bug go to:
https://bugs.launchpad.net/charm-nova-compute/+bug/1926249/+subscriptions

Re: [PATCH for-9.1 04/19] target/i386: do not use s->tmp0 and s->tmp4 to compute flags

[Paolo Bonzini]

Il mer 10 apr 2024, 08:35 Richard Henderson 
ha scritto:

> On 4/9/24 06:43, Paolo Bonzini wrote:
> > Create a new temporary whenever flags have to use one, instead of using
> > s->tmp0 or s->tmp4.  NULL can now be passed as the scratch register
> > to gen_prepare_*.
> >
> > Signed-off-by: Paolo Bonzini 
> > ---
> >   target/i386/tcg/translate.c | 54 +
> >   1 file changed, 31 insertions(+), 23 deletions(-)
> >
> > diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> > index 197cccb6c96..debc1b27283 100644
> > --- a/target/i386/tcg/translate.c
> > +++ b/target/i386/tcg/translate.c
> > @@ -947,9 +947,9 @@ static CCPrepare gen_prepare_eflags_c(DisasContext
> *s, TCGv reg)
> >   case CC_OP_SUBB ... CC_OP_SUBQ:
> >   /* (DATA_TYPE)CC_SRCT < (DATA_TYPE)CC_SRC */
> >   size = s->cc_op - CC_OP_SUBB;
> > -t1 = gen_ext_tl(s->tmp0, cpu_cc_src, size, false);
> > -/* If no temporary was used, be careful not to alias t1 and
> t0.  */
> > -t0 = t1 == cpu_cc_src ? s->tmp0 : reg;
> > +/* Be careful not to alias t1 and t0.  */
> > +t1 = gen_ext_tl(NULL, cpu_cc_src, size, false);
> > +t0 = (reg == t1 || !reg) ? tcg_temp_new() : reg;
> >   tcg_gen_mov_tl(t0, s->cc_srcT);
> >   gen_extu(size, t0);
>
> The tcg_temp_new, mov, and extu can be had with gen_ext_tl...
>

There's actually a lot more that can be done now that I looked more closely
at gen_ext_tl. It is fine (modulo bugs elsewhere) to just extend cc_* in
place. In fact this lets the optimizer work better, even allows (rare)
cross tb optimization because it effectively bumps CC_OP_ADD* to
target_long size, and is just as effective in removing tmp0/tmp4.

Paolo


> >   goto add_sub;
> > @@ -957,8 +957,9 @@ static CCPrepare gen_prepare_eflags_c(DisasContext
> *s, TCGv reg)
> >   case CC_OP_ADDB ... CC_OP_ADDQ:
> >   /* (DATA_TYPE)CC_DST < (DATA_TYPE)CC_SRC */
> >   size = s->cc_op - CC_OP_ADDB;
> > -t1 = gen_ext_tl(s->tmp0, cpu_cc_src, size, false);
> > -t0 = gen_ext_tl(reg, cpu_cc_dst, size, false);
> > +/* Be careful not to alias t1 and t0.  */
> > +t1 = gen_ext_tl(NULL, cpu_cc_src, size, false);
> > +t0 = gen_ext_tl(reg == t1 ? NULL : reg, cpu_cc_dst, size,
> false);
>
> ... like this.
>
> It would be helpful to update the function comments (nothing is 'compute
> ... to reg' in
> these functions).  Future cleanup, perhaps rename 'reg' to 'scratch', or
> remove the
> argument entirely where applicable.
>
> > @@ -1109,11 +1113,13 @@ static CCPrepare gen_prepare_cc(DisasContext *s,
> int b, TCGv reg)
> >   size = s->cc_op - CC_OP_SUBB;
> >   switch (jcc_op) {
> >   case JCC_BE:
> > -tcg_gen_mov_tl(s->tmp4, s->cc_srcT);
> > -gen_extu(size, s->tmp4);
> > -t0 = gen_ext_tl(s->tmp0, cpu_cc_src, size, false);
> > -cc = (CCPrepare) { .cond = TCG_COND_LEU, .reg = s->tmp4,
> > -   .reg2 = t0, .use_reg2 = true };
> > +/* Be careful not to alias t1 and t0.  */
> > +t1 = gen_ext_tl(NULL, cpu_cc_src, size, false);
> > +t0 = (reg == t1 || !reg) ? tcg_temp_new() : reg;
> > +tcg_gen_mov_tl(t0, s->cc_srcT);
> > +gen_extu(size, t0);
>
> gen_ext_tl
>
> > +cc = (CCPrepare) { .cond = TCG_COND_LEU, .reg = t0,
> > +   .reg2 = t1, .use_reg2 = true };
> >   break;
> >
> >   case JCC_L:
> > @@ -1122,11 +1128,13 @@ static CCPrepare gen_prepare_cc(DisasContext *s,
> int b, TCGv reg)
> >   case JCC_LE:
> >   cond = TCG_COND_LE;
> >   fast_jcc_l:
> > -tcg_gen_mov_tl(s->tmp4, s->cc_srcT);
> > -gen_exts(size, s->tmp4);
> > -t0 = gen_ext_tl(s->tmp0, cpu_cc_src, size, true);
> > -cc = (CCPrepare) { .cond = cond, .reg = s->tmp4,
> > -   .reg2 = t0, .use_reg2 = true };
> > +/* Be careful not to alias t1 and t0.  */
> > +t1 = gen_ext_tl(NULL, cpu_cc_src, size, true);
> > +t0 = (reg == t1 || !reg) ? tcg_temp_new() : reg;
> > +tcg_gen_mov_tl(t0, s->cc_srcT);
> > +gen_exts(size, t0);
>
> gen_ext_tl
>
> With that,
> Reviewed-by: Richard Henderson 
>
>
> r~
>
>

Re: [PATCH] hw/misc/applesmc: Simplify DeviceReset handler

[Peter Maydell]

On Wed, 10 Apr 2024 at 19:08, Philippe Mathieu-Daudé  wrote:
>
> Have applesmc_find_key() return a const pointer.
> Since the returned buffers are not modified in
> applesmc_io_data_write(), it is pointless to
> delete and re-add the keys in the DeviceReset
> handler. Add them once in DeviceRealize, and
> discard them in the DeviceUnrealize handler.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> As discussed in
> https://lore.kernel.org/qemu-devel/6fbcf565-f12c-4196-b6c8-559843c7a...@linaro.org/

Reviewed-by: Peter Maydell 

thanks
-- PMM

[PATCH] hw/misc/applesmc: Simplify DeviceReset handler

[Philippe Mathieu-Daudé]

Have applesmc_find_key() return a const pointer.
Since the returned buffers are not modified in
applesmc_io_data_write(), it is pointless to
delete and re-add the keys in the DeviceReset
handler. Add them once in DeviceRealize, and
discard them in the DeviceUnrealize handler.

Signed-off-by: Philippe Mathieu-Daudé 
---
As discussed in
https://lore.kernel.org/qemu-devel/6fbcf565-f12c-4196-b6c8-559843c7a...@linaro.org/
---
 hw/misc/applesmc.c | 36 +---
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index 14e3ef667d..59a4899312 100644
--- a/hw/misc/applesmc.c
+++ b/hw/misc/applesmc.c
@@ -145,7 +145,7 @@ static void applesmc_io_cmd_write(void *opaque, hwaddr 
addr, uint64_t val,
 s->data_pos = 0;
 }
 
-static struct AppleSMCData *applesmc_find_key(AppleSMCState *s)
+static const struct AppleSMCData *applesmc_find_key(AppleSMCState *s)
 {
 struct AppleSMCData *d;
 
@@ -161,7 +161,7 @@ static void applesmc_io_data_write(void *opaque, hwaddr 
addr, uint64_t val,
unsigned size)
 {
 AppleSMCState *s = opaque;
-struct AppleSMCData *d;
+const struct AppleSMCData *d;
 
 smc_debug("DATA received: 0x%02x\n", (uint8_t)val);
 switch (s->cmd) {
@@ -269,23 +269,10 @@ static void applesmc_add_key(AppleSMCState *s, const char 
*key,
 static void qdev_applesmc_isa_reset(DeviceState *dev)
 {
 AppleSMCState *s = APPLE_SMC(dev);
-struct AppleSMCData *d, *next;
 
-/* Remove existing entries */
-QLIST_FOREACH_SAFE(d, >data_def, node, next) {
-QLIST_REMOVE(d, node);
-g_free(d);
-}
 s->status = 0x00;
 s->status_1e = 0x00;
 s->last_ret = 0x00;
-
-applesmc_add_key(s, "REV ", 6, "\x01\x13\x0f\x00\x00\x03");
-applesmc_add_key(s, "OSK0", 32, s->osk);
-applesmc_add_key(s, "OSK1", 32, s->osk + 32);
-applesmc_add_key(s, "NATJ", 1, "\0");
-applesmc_add_key(s, "MSSP", 1, "\0");
-applesmc_add_key(s, "MSSD", 1, "\0x3");
 }
 
 static const MemoryRegionOps applesmc_data_io_ops = {
@@ -343,6 +330,24 @@ static void applesmc_isa_realize(DeviceState *dev, Error 
**errp)
 }
 
 QLIST_INIT(>data_def);
+applesmc_add_key(s, "REV ", 6, "\x01\x13\x0f\x00\x00\x03");
+applesmc_add_key(s, "OSK0", 32, s->osk);
+applesmc_add_key(s, "OSK1", 32, s->osk + 32);
+applesmc_add_key(s, "NATJ", 1, "\0");
+applesmc_add_key(s, "MSSP", 1, "\0");
+applesmc_add_key(s, "MSSD", 1, "\0x3");
+}
+
+static void applesmc_unrealize(DeviceState *dev)
+{
+AppleSMCState *s = APPLE_SMC(dev);
+struct AppleSMCData *d, *next;
+
+/* Remove existing entries */
+QLIST_FOREACH_SAFE(d, >data_def, node, next) {
+QLIST_REMOVE(d, node);
+g_free(d);
+}
 }
 
 static Property applesmc_isa_properties[] = {
@@ -377,6 +382,7 @@ static void qdev_applesmc_class_init(ObjectClass *klass, 
void *data)
 AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(klass);
 
 dc->realize = applesmc_isa_realize;
+dc->unrealize = applesmc_unrealize;
 dc->reset = qdev_applesmc_isa_reset;
 device_class_set_props(dc, applesmc_isa_properties);
 set_bit(DEVICE_CATEGORY_MISC, dc->categories);
-- 
2.41.0

Re: [PULL v2 00/20] misc patch queue

[Michael Tokarev]

10.04.2024 19:38, Richard Henderson:


  target/hppa: Fix IIAOQ, IIASQ for pa2.0


This is for hppa64, so not further back than 8.2, or not at all -- hppa64 is 
really still in development.


We had a few other fixes for hppa64 for 8.2.
I was unsure about this since hppa64 appeared in 8.2
for the first time and sure thing it contains a ton
of bugs.

This one is a bit more difficult as it also needs
7d50b69660 "target/hppa: Use gva_offset_mask() everywhere", -
I wont force it in, despite it being a small change.


 From sh4 mac* et al, this can be picked too:

  target/sh4: mac.w: memory accesses are 16-bit words

but I dunno about the others:

  target/sh4: Merge mach and macl into a union
  target/sh4: Fix mac.l with saturation enabled
  target/sh4: Fix mac.w with saturation enabled
  target/sh4: add missing CHECK_NOT_DELAY_SLOT
  target/m68k: Map FPU exceptions to FPSR register


Yes.


I picked all this for 8.2 but not for 7.2.  For 7.2,
there's quite some context missing in there.  Only
"add missing CHECK_NOT_DELAY_SLOT" is okay. Dunno
how relevant all these 6 changes for 7.2 are.

Thank you for the info!

/mjt

Re: [External] Re: [PATCH v11 2/2] memory tier: create CPUless memory tiers after obtaining HMAT info

[Jonathan Cameron via]

On Tue, 9 Apr 2024 12:02:31 -0700
"Ho-Ren (Jack) Chuang"  wrote:

> Hi Jonathan,
> 
> On Tue, Apr 9, 2024 at 9:12 AM Jonathan Cameron
>  wrote:
> >
> > On Fri, 5 Apr 2024 15:43:47 -0700
> > "Ho-Ren (Jack) Chuang"  wrote:
> >  
> > > On Fri, Apr 5, 2024 at 7:03 AM Jonathan Cameron
> > >  wrote:  
> > > >
> > > > On Fri,  5 Apr 2024 00:07:06 +
> > > > "Ho-Ren (Jack) Chuang"  wrote:
> > > >  
> > > > > The current implementation treats emulated memory devices, such as
> > > > > CXL1.1 type3 memory, as normal DRAM when they are emulated as normal 
> > > > > memory
> > > > > (E820_TYPE_RAM). However, these emulated devices have different
> > > > > characteristics than traditional DRAM, making it important to
> > > > > distinguish them. Thus, we modify the tiered memory initialization 
> > > > > process
> > > > > to introduce a delay specifically for CPUless NUMA nodes. This delay
> > > > > ensures that the memory tier initialization for these nodes is 
> > > > > deferred
> > > > > until HMAT information is obtained during the boot process. Finally,
> > > > > demotion tables are recalculated at the end.
> > > > >
> > > > > * late_initcall(memory_tier_late_init);
> > > > > Some device drivers may have initialized memory tiers between
> > > > > `memory_tier_init()` and `memory_tier_late_init()`, potentially 
> > > > > bringing
> > > > > online memory nodes and configuring memory tiers. They should be 
> > > > > excluded
> > > > > in the late init.
> > > > >
> > > > > * Handle cases where there is no HMAT when creating memory tiers
> > > > > There is a scenario where a CPUless node does not provide HMAT 
> > > > > information.
> > > > > If no HMAT is specified, it falls back to using the default DRAM tier.
> > > > >
> > > > > * Introduce another new lock `default_dram_perf_lock` for adist 
> > > > > calculation
> > > > > In the current implementation, iterating through CPUlist nodes 
> > > > > requires
> > > > > holding the `memory_tier_lock`. However, `mt_calc_adistance()` will 
> > > > > end up
> > > > > trying to acquire the same lock, leading to a potential deadlock.
> > > > > Therefore, we propose introducing a standalone 
> > > > > `default_dram_perf_lock` to
> > > > > protect `default_dram_perf_*`. This approach not only avoids deadlock
> > > > > but also prevents holding a large lock simultaneously.
> > > > >
> > > > > * Upgrade `set_node_memory_tier` to support additional cases, 
> > > > > including
> > > > >   default DRAM, late CPUless, and hot-plugged initializations.
> > > > > To cover hot-plugged memory nodes, `mt_calc_adistance()` and
> > > > > `mt_find_alloc_memory_type()` are moved into `set_node_memory_tier()` 
> > > > > to
> > > > > handle cases where memtype is not initialized and where HMAT 
> > > > > information is
> > > > > available.
> > > > >
> > > > > * Introduce `default_memory_types` for those memory types that are not
> > > > >   initialized by device drivers.
> > > > > Because late initialized memory and default DRAM memory need to be 
> > > > > managed,
> > > > > a default memory type is created for storing all memory types that are
> > > > > not initialized by device drivers and as a fallback.
> > > > >
> > > > > Signed-off-by: Ho-Ren (Jack) Chuang 
> > > > > Signed-off-by: Hao Xiang 
> > > > > Reviewed-by: "Huang, Ying"   
> > > >
> > > > Hi - one remaining question. Why can't we delay init for all nodes
> > > > to either drivers or your fallback late_initcall code.
> > > > It would be nice to reduce possible code paths.  
> > >
> > > I try not to change too much of the existing code structure in
> > > this patchset.
> > >
> > > To me, postponing/moving all memory tier registrations to
> > > late_initcall() is another possible action item for the next patchset.
> > >
> > > After tier_mem(), hmat_init() is called, which requires registering
> > > `default_dram_type` info. This is when `default_dram_type` is needed.
> > > However, it is indeed possible to postpone the latter part,
> > > set_node_memory_tier(), to `late_init(). So, memory_tier_init() can
> > > indeed be split into two parts, and the latter part can be moved to
> > > late_initcall() to be processed together.
> > >
> > > Doing this all memory-type drivers have to call late_initcall() to
> > > register a memory tier. I’m not sure how many they are?
> > >
> > > What do you guys think?  
> >
> > Gut feeling - if you are going to move it for some cases, move it for
> > all of them.  Then we only have to test once ;)
> >
> > J  
> 
> Thank you for your reminder! I agree~ That's why I'm considering
> changing them in the next patchset because of the amount of changes.
> And also, this patchset already contains too many things.

Makes sense.  (Interestingly we are reaching the same conclusion
for the thread that motivated suggesting bringing them all together
in the first place!)

Get things work in a clean fashion, then consider moving everything to
happen at the same time to simplify testing etc.

Jonathan

Re: [QEMU][PATCH v3 5/7] memory: add MemoryRegion map and unmap callbacks

[Edgar E. Iglesias]

On Tue, Feb 27, 2024 at 11:37 PM Vikram Garhwal 
wrote:

> From: Juergen Gross 
>
> In order to support mapping and unmapping guest memory dynamically to
> and from qemu during address_space_[un]map() operations add the map()
> and unmap() callbacks to MemoryRegionOps.
>
> Those will be used e.g. for Xen grant mappings when performing guest
> I/Os.
>
> Signed-off-by: Juergen Gross 
> Signed-off-by: Vikram Garhwal 
>


Paolo, Peter, David, Phiippe, do you guys have any concerns with this patch?

Cheers,
Edgar



> ---
>  include/exec/memory.h | 21 ++
>  system/physmem.c  | 50 +--
>  2 files changed, 60 insertions(+), 11 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 8626a355b3..9f7dfe59c7 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -282,6 +282,27 @@ struct MemoryRegionOps {
>  unsigned size,
>  MemTxAttrs attrs);
>
> +/*
> + * Dynamically create mapping. @addr is the guest address to map;
> @plen
> + * is the pointer to the usable length of the buffer.
> + * @mr contents can be changed in case a new memory region is created
> for
> + * the mapping.
> + * Returns the buffer address for accessing the data.
> + */
> +void *(*map)(MemoryRegion **mr,
> + hwaddr addr,
> + hwaddr *plen,
> + bool is_write,
> + MemTxAttrs attrs);
> +
> +/* Unmap an area obtained via map() before. */
> +void (*unmap)(MemoryRegion *mr,
> +  void *buffer,
> +  ram_addr_t addr,
> +  hwaddr len,
> +  bool is_write,
> +  hwaddr access_len);
> +
>  enum device_endian endianness;
>  /* Guest-visible constraints: */
>  struct {
> diff --git a/system/physmem.c b/system/physmem.c
> index 949dcb20ba..d989e9fc1f 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -3141,6 +3141,7 @@ void *address_space_map(AddressSpace *as,
>  hwaddr len = *plen;
>  hwaddr l, xlat;
>  MemoryRegion *mr;
> +void *ptr = NULL;
>  FlatView *fv;
>
>  if (len == 0) {
> @@ -3174,12 +3175,20 @@ void *address_space_map(AddressSpace *as,
>  return bounce.buffer;
>  }
>
> -
>  memory_region_ref(mr);
> +
> +if (mr->ops && mr->ops->map) {
> +ptr = mr->ops->map(, addr, plen, is_write, attrs);
> +}
> +
>  *plen = flatview_extend_translation(fv, addr, len, mr, xlat,
>  l, is_write, attrs);
>  fuzz_dma_read_cb(addr, *plen, mr);
> -return qemu_ram_ptr_length(mr->ram_block, xlat, plen, true);
> +if (ptr == NULL) {
> +ptr = qemu_ram_ptr_length(mr->ram_block, xlat, plen, true);
> +}
> +
> +return ptr;
>  }
>
>  /* Unmaps a memory region previously mapped by address_space_map().
> @@ -3195,11 +3204,16 @@ void address_space_unmap(AddressSpace *as, void
> *buffer, hwaddr len,
>
>  mr = memory_region_from_host(buffer, );
>  assert(mr != NULL);
> -if (is_write) {
> -invalidate_and_set_dirty(mr, addr1, access_len);
> -}
> -if (xen_enabled()) {
> -xen_invalidate_map_cache_entry(buffer);
> +
> +if (mr->ops && mr->ops->unmap) {
> +mr->ops->unmap(mr, buffer, addr1, len, is_write, access_len);
> +} else {
> +if (is_write) {
> +invalidate_and_set_dirty(mr, addr1, access_len);
> +}
> +if (xen_enabled()) {
> +xen_invalidate_map_cache_entry(buffer);
> +}
>  }
>  memory_region_unref(mr);
>  return;
> @@ -3272,10 +3286,18 @@ int64_t address_space_cache_init(MemoryRegionCache
> *cache,
>   * doing this if we found actual RAM, which behaves the same
>   * regardless of attributes; so UNSPECIFIED is fine.
>   */
> +if (mr->ops && mr->ops->map) {
> +cache->ptr = mr->ops->map(, addr, , is_write,
> +  MEMTXATTRS_UNSPECIFIED);
> +}
> +
>  l = flatview_extend_translation(cache->fv, addr, len, mr,
>  cache->xlat, l, is_write,
>  MEMTXATTRS_UNSPECIFIED);
> -cache->ptr = qemu_ram_ptr_length(mr->ram_block, cache->xlat, ,
> true);
> +if (!cache->ptr) {
> +cache->ptr = qemu_ram_ptr_length(mr->ram_block, cache->xlat,
> ,
> + true);
> +}
>  } else {
>  cache->ptr = NULL;
>  }
> @@ -3297,14 +3319,20 @@ void
> address_space_cache_invalidate(MemoryRegionCache *cache,
>
>  void address_space_cache_destroy(MemoryRegionCache *cache)
>  {
> -if (!cache->mrs.mr) {
> +MemoryRegion *mr = cache->mrs.mr;
> +
> +if (!mr) {
>  

Re: [PULL v2 00/20] misc patch queue

[Richard Henderson]

On 4/10/24 06:10, Michael Tokarev wrote:

 From this list, do we pick something for stable?
It looks like

  tcg/optimize: Do not attempt to constant fold neg_vec
  linux-user: Fix waitid return of siginfo_t and rusage


Yes.


  target/hppa: Fix IIAOQ, IIASQ for pa2.0


This is for hppa64, so not further back than 8.2, or not at all -- hppa64 is really still 
in development.




 From sh4 mac* et al, this can be picked too:

  target/sh4: mac.w: memory accesses are 16-bit words

but I dunno about the others:

  target/sh4: Merge mach and macl into a union
  target/sh4: Fix mac.l with saturation enabled
  target/sh4: Fix mac.w with saturation enabled
  target/sh4: add missing CHECK_NOT_DELAY_SLOT
  target/m68k: Map FPU exceptions to FPSR register


Yes.


A long(ish) DisasContextBase series leading to

   accel/tcg: Improve can_do_io management

probably should not go to stable.


Probably not, or at least not further back than 8.2.


r~

Re: [PATCH v3] virtio-pci: Fix the crash that the vector was used after released.

[Michael S. Tsirkin]

On Thu, Apr 11, 2024 at 12:12:00AM +0800, Cindy Lu wrote:
> During the booting process of the non-standard image, the behavior of the
> called function in qemu is as follows:
> 
> 1. vhost_net_stop() was triggered by guest image. This will call the function
> virtio_pci_set_guest_notifiers() with assgin= false,
> virtio_pci_set_guest_notifiers(） will release the irqfd for vector 0
> 
> 2. virtio_reset() was triggered, this will set configure vector to 
> VIRTIO_NO_VECTOR
> 
> 3.vhost_net_start() was called (at this time, the configure vector is
> still VIRTIO_NO_VECTOR) and then call virtio_pci_set_guest_notifiers() with
> assgin=true, so the irqfd for vector 0 is still not "init" during this process
> 
> 4. The system continues to boot and sets the vector back to 0. After that
> msix_fire_vector_notifier() was triggered to unmask the vector 0 and  meet 
> the crash
> 
> To fix the issue, we need to support changing the vector after 
> VIRTIO_CONFIG_S_DRIVER_OK is set.
> 
> (gdb) bt
> 0  __pthread_kill_implementation (threadid=, 
> signo=signo@entry=6, no_tid=no_tid@entry=0)
> at pthread_kill.c:44
> 1  0x7fc87148ec53 in __pthread_kill_internal (signo=6, 
> threadid=) at pthread_kill.c:78
> 2  0x7fc87143e956 in __GI_raise (sig=sig@entry=6) at 
> ../sysdeps/posix/raise.c:26
> 3  0x7fc8714287f4 in __GI_abort () at abort.c:79
> 4  0x7fc87142871b in __assert_fail_base
> (fmt=0x7fc8715bbde0 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
> assertion=0x5606413efd53 "ret == 0", file=0x5606413ef87d 
> "../accel/kvm/kvm-all.c", line=1837, function=) at assert.c:92
> 5  0x7fc871437536 in __GI___assert_fail
> (assertion=0x5606413efd53 "ret == 0", file=0x5606413ef87d 
> "../accel/kvm/kvm-all.c", line=1837, function=0x5606413f06f0 
> <__PRETTY_FUNCTION__.19> "kvm_irqchip_commit_routes") at assert.c:101
> 6  0x560640f884b5 in kvm_irqchip_commit_routes (s=0x560642cae1f0) at 
> ../accel/kvm/kvm-all.c:1837
> 7  0x560640c98f8e in virtio_pci_one_vector_unmask
> (proxy=0x560643c65f00, queue_no=4294967295, vector=0, msg=..., 
> n=0x560643c6e4c8)
> at ../hw/virtio/virtio-pci.c:1005
> 8  0x560640c99201 in virtio_pci_vector_unmask (dev=0x560643c65f00, 
> vector=0, msg=...)
> at ../hw/virtio/virtio-pci.c:1070
> 9  0x560640bc402e in msix_fire_vector_notifier (dev=0x560643c65f00, 
> vector=0, is_masked=false)
> at ../hw/pci/msix.c:120
> 10 0x560640bc40f1 in msix_handle_mask_update (dev=0x560643c65f00, 
> vector=0, was_masked=true)
> at ../hw/pci/msix.c:140
> 11 0x560640bc4503 in msix_table_mmio_write (opaque=0x560643c65f00, 
> addr=12, val=0, size=4)
> at ../hw/pci/msix.c:231
> 12 0x560640f26d83 in memory_region_write_accessor
> (mr=0x560643c66540, addr=12, value=0x7fc86b7bc628, size=4, shift=0, 
> mask=4294967295, attrs=...)
> at ../system/memory.c:497
> 13 0x560640f270a6 in access_with_adjusted_size
> 
>  (addr=12, value=0x7fc86b7bc628, size=4, access_size_min=1, 
> access_size_max=4, access_fn=0x560640f26c8d , 
> mr=0x560643c66540, attrs=...) at ../system/memory.c:573
> 14 0x560640f2a2b5 in memory_region_dispatch_write (mr=0x560643c66540, 
> addr=12, data=0, op=MO_32, attrs=...)
> at ../system/memory.c:1521
> 15 0x560640f37bac in flatview_write_continue
> (fv=0x7fc65805e0b0, addr=4273803276, attrs=..., ptr=0x7fc871e9c028, 
> len=4, addr1=12, l=4, mr=0x560643c66540)
> at ../system/physmem.c:2714
> 16 0x560640f37d0f in flatview_write
> (fv=0x7fc65805e0b0, addr=4273803276, attrs=..., buf=0x7fc871e9c028, 
> len=4) at ../system/physmem.c:2756
> 17 0x560640f380bf in address_space_write
> (as=0x560642161ae0 , addr=4273803276, attrs=..., 
> buf=0x7fc871e9c028, len=4)
> at ../system/physmem.c:2863
> 18 0x560640f3812c in address_space_rw
> (as=0x560642161ae0 , addr=4273803276, attrs=..., 
> buf=0x7fc871e9c028, len=4, is_write=true) at ../system/physmem.c:2873
> --Type  for more, q to quit, c to continue without paging--
> 19 0x560640f8aa55 in kvm_cpu_exec (cpu=0x560642f205e0) at 
> ../accel/kvm/kvm-all.c:2915
> 20 0x560640f8d731 in kvm_vcpu_thread_fn (arg=0x560642f205e0) at 
> ../accel/kvm/kvm-accel-ops.c:51
> 21 0x5606411949f4 in qemu_thread_start (args=0x560642f292b0) at 
> ../util/qemu-thread-posix.c:541
> 22 0x7fc87148cdcd in start_thread (arg=) at 
> pthread_create.c:442
> 23 0x7fc871512630 in clone3 () at 
> ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
> (gdb)
> Signed-off-by: Cindy Lu 
> ---
>  hw/virtio/virtio-pci.c | 30 ++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 1a7039fb0c..b3b1a4a66f 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1570,7 +1570,22 @@ static void virtio_pci_common_write(void *opaque, 
> hwaddr addr,
>  } else {
>  val = VIRTIO_NO_VECTOR;
>  }
> +vector = vdev->config_vector;
>  

[PATCH v3] virtio-pci: Fix the crash that the vector was used after released.

[Cindy Lu]

During the booting process of the non-standard image, the behavior of the
called function in qemu is as follows:

1. vhost_net_stop() was triggered by guest image. This will call the function
virtio_pci_set_guest_notifiers() with assgin= false,
virtio_pci_set_guest_notifiers(） will release the irqfd for vector 0

2. virtio_reset() was triggered, this will set configure vector to 
VIRTIO_NO_VECTOR

3.vhost_net_start() was called (at this time, the configure vector is
still VIRTIO_NO_VECTOR) and then call virtio_pci_set_guest_notifiers() with
assgin=true, so the irqfd for vector 0 is still not "init" during this process

4. The system continues to boot and sets the vector back to 0. After that
msix_fire_vector_notifier() was triggered to unmask the vector 0 and  meet the 
crash

To fix the issue, we need to support changing the vector after 
VIRTIO_CONFIG_S_DRIVER_OK is set.

(gdb) bt
0  __pthread_kill_implementation (threadid=, 
signo=signo@entry=6, no_tid=no_tid@entry=0)
at pthread_kill.c:44
1  0x7fc87148ec53 in __pthread_kill_internal (signo=6, threadid=) at pthread_kill.c:78
2  0x7fc87143e956 in __GI_raise (sig=sig@entry=6) at 
../sysdeps/posix/raise.c:26
3  0x7fc8714287f4 in __GI_abort () at abort.c:79
4  0x7fc87142871b in __assert_fail_base
(fmt=0x7fc8715bbde0 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
assertion=0x5606413efd53 "ret == 0", file=0x5606413ef87d 
"../accel/kvm/kvm-all.c", line=1837, function=) at assert.c:92
5  0x7fc871437536 in __GI___assert_fail
(assertion=0x5606413efd53 "ret == 0", file=0x5606413ef87d 
"../accel/kvm/kvm-all.c", line=1837, function=0x5606413f06f0 
<__PRETTY_FUNCTION__.19> "kvm_irqchip_commit_routes") at assert.c:101
6  0x560640f884b5 in kvm_irqchip_commit_routes (s=0x560642cae1f0) at 
../accel/kvm/kvm-all.c:1837
7  0x560640c98f8e in virtio_pci_one_vector_unmask
(proxy=0x560643c65f00, queue_no=4294967295, vector=0, msg=..., 
n=0x560643c6e4c8)
at ../hw/virtio/virtio-pci.c:1005
8  0x560640c99201 in virtio_pci_vector_unmask (dev=0x560643c65f00, 
vector=0, msg=...)
at ../hw/virtio/virtio-pci.c:1070
9  0x560640bc402e in msix_fire_vector_notifier (dev=0x560643c65f00, 
vector=0, is_masked=false)
at ../hw/pci/msix.c:120
10 0x560640bc40f1 in msix_handle_mask_update (dev=0x560643c65f00, vector=0, 
was_masked=true)
at ../hw/pci/msix.c:140
11 0x560640bc4503 in msix_table_mmio_write (opaque=0x560643c65f00, addr=12, 
val=0, size=4)
at ../hw/pci/msix.c:231
12 0x560640f26d83 in memory_region_write_accessor
(mr=0x560643c66540, addr=12, value=0x7fc86b7bc628, size=4, shift=0, 
mask=4294967295, attrs=...)
at ../system/memory.c:497
13 0x560640f270a6 in access_with_adjusted_size

 (addr=12, value=0x7fc86b7bc628, size=4, access_size_min=1, 
access_size_max=4, access_fn=0x560640f26c8d , 
mr=0x560643c66540, attrs=...) at ../system/memory.c:573
14 0x560640f2a2b5 in memory_region_dispatch_write (mr=0x560643c66540, 
addr=12, data=0, op=MO_32, attrs=...)
at ../system/memory.c:1521
15 0x560640f37bac in flatview_write_continue
(fv=0x7fc65805e0b0, addr=4273803276, attrs=..., ptr=0x7fc871e9c028, len=4, 
addr1=12, l=4, mr=0x560643c66540)
at ../system/physmem.c:2714
16 0x560640f37d0f in flatview_write
(fv=0x7fc65805e0b0, addr=4273803276, attrs=..., buf=0x7fc871e9c028, len=4) 
at ../system/physmem.c:2756
17 0x560640f380bf in address_space_write
(as=0x560642161ae0 , addr=4273803276, attrs=..., 
buf=0x7fc871e9c028, len=4)
at ../system/physmem.c:2863
18 0x560640f3812c in address_space_rw
(as=0x560642161ae0 , addr=4273803276, attrs=..., 
buf=0x7fc871e9c028, len=4, is_write=true) at ../system/physmem.c:2873
--Type  for more, q to quit, c to continue without paging--
19 0x560640f8aa55 in kvm_cpu_exec (cpu=0x560642f205e0) at 
../accel/kvm/kvm-all.c:2915
20 0x560640f8d731 in kvm_vcpu_thread_fn (arg=0x560642f205e0) at 
../accel/kvm/kvm-accel-ops.c:51
21 0x5606411949f4 in qemu_thread_start (args=0x560642f292b0) at 
../util/qemu-thread-posix.c:541
22 0x7fc87148cdcd in start_thread (arg=) at 
pthread_create.c:442
23 0x7fc871512630 in clone3 () at 
../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
(gdb)
Signed-off-by: Cindy Lu 
---
 hw/virtio/virtio-pci.c | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 1a7039fb0c..b3b1a4a66f 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1570,7 +1570,22 @@ static void virtio_pci_common_write(void *opaque, hwaddr 
addr,
 } else {
 val = VIRTIO_NO_VECTOR;
 }
+vector = vdev->config_vector;
 vdev->config_vector = val;
+/*
+ * If the value was changed after DRIVER_OK was set, it means that
+ * we need to release the old vector and set up the new vector.
+ */
+if ((vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) &&
+/*check if 

[PATCH 10/12] hw/ide/atapi: Use qemu_hexdump_line() to avoid sprintf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Use qemu_hexdump_line() to avoid sprintf() calls, silencing:

  [1367/1604] Compiling C object libcommon.fa.p/backends_tpm_tpm_util.c.o
  backends/tpm/tpm_util.c:355:18: warning: 'sprintf' is deprecated:
This function is provided for compatibility reasons only.
Due to security concerns inherent in the design of sprintf(3),
it is highly recommended that you use snprintf(3) instead.
[-Wdeprecated-declarations]
p += sprintf(p, "\n");
 ^

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/ide/atapi.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 73ec373184..27b6aa59fd 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -24,6 +24,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include "hw/scsi/scsi.h"
 #include "sysemu/block-backend.h"
 #include "scsi/constants.h"
@@ -1309,12 +1310,7 @@ void ide_atapi_cmd(IDEState *s)
 trace_ide_atapi_cmd(s, s->io_buffer[0]);
 
 if (trace_event_get_state_backends(TRACE_IDE_ATAPI_CMD_PACKET)) {
-/* Each pretty-printed byte needs two bytes and a space; */
-char *ppacket = g_malloc(ATAPI_PACKET_SIZE * 3 + 1);
-int i;
-for (i = 0; i < ATAPI_PACKET_SIZE; i++) {
-sprintf(ppacket + (i * 3), "%02x ", buf[i]);
-}
+char *ppacket = qemu_hexdump_line(buf, 0, ATAPI_PACKET_SIZE, false);
 trace_ide_atapi_cmd_packet(s, s->lcyl | (s->hcyl << 8), ppacket);
 g_free(ppacket);
 }
-- 
2.41.0

[PATCH 03/12] hw/ppc/spapr: Replace sprintf() by g_strdup_printf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Replace sprintf() by g_strdup_printf() in order to avoid:

  hw/ppc/spapr.c:385:5: warning: 'sprintf' is deprecated:
This function is provided for compatibility reasons only.
Due to security concerns inherent in the design of sprintf(3),
it is highly recommended that you use snprintf(3) instead.
[-Wdeprecated-declarations]
  sprintf(mem_name, "memory@%" HWADDR_PRIx, start);
  ^
  1 warning generated.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/ppc/spapr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e9bc97fee0..9807f47690 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -375,14 +375,14 @@ static void add_str(GString *s, const gchar *s1)
 static int spapr_dt_memory_node(SpaprMachineState *spapr, void *fdt, int 
nodeid,
 hwaddr start, hwaddr size)
 {
-char mem_name[32];
+g_autofree char *mem_name = NULL;
 uint64_t mem_reg_property[2];
 int off;
 
 mem_reg_property[0] = cpu_to_be64(start);
 mem_reg_property[1] = cpu_to_be64(size);
 
-sprintf(mem_name, "memory@%" HWADDR_PRIx, start);
+mem_name = g_strdup_printf("memory@%" HWADDR_PRIx, start);
 off = fdt_add_subnode(fdt, 0, mem_name);
 _FDT(off);
 _FDT((fdt_setprop_string(fdt, off, "device_type", "memory")));
-- 
2.41.0

Re: [PULL v2 00/20] misc patch queue

[Michael Tokarev]

09.04.2024 22:35, Richard Henderson wrote:


target/m68k: Fix fp accrued exception reporting
target/hppa: Fix IIAOQ, IIASQ for pa2.0
target/sh4: Fixes to mac.l and mac.w saturation
target/sh4: Fixes to illegal delay slot reporting
linux-user: Fix waitid return of siginfo_t and rusage
linux-user: Preserve unswapped siginfo_t for strace
tcg/optimize: Do not attempt to constant fold neg_vec
accel/tcg: Improve can_do_io management, mmio bug fix


From this list, do we pick something for stable?
It looks like

 tcg/optimize: Do not attempt to constant fold neg_vec
 linux-user: Fix waitid return of siginfo_t and rusage
 target/hppa: Fix IIAOQ, IIASQ for pa2.0

should be picked.

From sh4 mac* et al, this can be picked too:

 target/sh4: mac.w: memory accesses are 16-bit words

but I dunno about the others:

 target/sh4: Merge mach and macl into a union
 target/sh4: Fix mac.l with saturation enabled
 target/sh4: Fix mac.w with saturation enabled
 target/sh4: add missing CHECK_NOT_DELAY_SLOT
 target/m68k: Map FPU exceptions to FPSR register

A long(ish) DisasContextBase series leading to

  accel/tcg: Improve can_do_io management

probably should not go to stable.

Adding changes authors and reviewers to the Cc list
for comments.

Thanks,

/mjt



Keith Packard (1):
   target/m68k: Map FPU exceptions to FPSR register

Nguyen Dinh Phi (1):
   linux-user: replace calloc() with g_new0()

Richard Henderson (14):
   tcg/optimize: Do not attempt to constant fold neg_vec
   linux-user: Fix waitid return of siginfo_t and rusage
   target/hppa: Fix IIAOQ, IIASQ for pa2.0
   target/sh4: Merge mach and macl into a union
   tcg: Add TCGContext.emit_before_op
   accel/tcg: Add insn_start to DisasContextBase
   target/arm: Use insn_start from DisasContextBase
   target/hppa: Use insn_start from DisasContextBase
   target/i386: Preserve DisasContextBase.insn_start across rewind
   target/microblaze: Use insn_start from DisasContextBase
   target/riscv: Use insn_start from DisasContextBase
   target/s390x: Use insn_start from DisasContextBase
   accel/tcg: Improve can_do_io management
   linux-user: Preserve unswapped siginfo_t for strace

Zack Buhman (4):
   target/sh4: mac.w: memory accesses are 16-bit words
   target/sh4: Fix mac.l with saturation enabled
   target/sh4: Fix mac.w with saturation enabled
   target/sh4: add missing CHECK_NOT_DELAY_SLOT

[PATCH 04/12] hw/mips/malta: Replace sprintf() by g_string_append_printf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Extract common code to get_rng_seed_hex(), replacing the
sprintf() calls by GString API ones in order to avoid:

  [120/169] Compiling C object libcommon.fa.p/system_qtest.c.o
  hw/mips/malta.c:860:9: warning: 'sprintf' is deprecated:
  sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]);
  ^
This function is provided for compatibility reasons only.
Due to security concerns inherent in the design of sprintf(3),
it is highly recommended that you use snprintf(3) instead.
[-Wdeprecated-declarations]
  hw/mips/malta.c:951:9: warning: 'sprintf' is deprecated:
  sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]);
  ^

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/mips/malta.c | 22 +-
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index af74008c82..095abbf0ec 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -850,15 +850,24 @@ static void G_GNUC_PRINTF(3, 4) prom_set(uint32_t 
*prom_buf, int index,
 va_end(ap);
 }
 
-static void reinitialize_rng_seed(void *opaque)
+static char *get_rng_seed_hex(void)
 {
-char *rng_seed_hex = opaque;
+g_autoptr(GString) gs = g_string_new("");
 uint8_t rng_seed[32];
 
 qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
 for (size_t i = 0; i < sizeof(rng_seed); ++i) {
-sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]);
+g_string_append_printf(gs, "%02x", rng_seed[i]);
 }
+
+return g_strdup(gs->str);
+}
+
+static void reinitialize_rng_seed(void *opaque)
+{
+g_autofree char *rng_seed_hex = get_rng_seed_hex();
+
+strcpy(opaque, rng_seed_hex);
 }
 
 /* Kernel */
@@ -870,8 +879,7 @@ static uint64_t load_kernel(void)
 uint32_t *prom_buf;
 long prom_size;
 int prom_index = 0;
-uint8_t rng_seed[32];
-char rng_seed_hex[sizeof(rng_seed) * 2 + 1];
+g_autofree char *rng_seed_hex = get_rng_seed_hex();
 size_t rng_seed_prom_offset;
 
 kernel_size = load_elf(loaderparams.kernel_filename, NULL,
@@ -946,10 +954,6 @@ static uint64_t load_kernel(void)
 prom_set(prom_buf, prom_index++, "modetty0");
 prom_set(prom_buf, prom_index++, "38400n8r");
 
-qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
-for (size_t i = 0; i < sizeof(rng_seed); ++i) {
-sprintf(rng_seed_hex + i * 2, "%02x", rng_seed[i]);
-}
 prom_set(prom_buf, prom_index++, "rngseed");
 rng_seed_prom_offset = prom_index * ENVP_ENTRY_SIZE +
sizeof(uint32_t) * ENVP_NB_ENTRIES;
-- 
2.41.0

[PATCH 11/12] hw/dma/pl330: Use qemu_hexdump_line() to avoid sprintf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Use qemu_hexdump_line() to avoid sprintf() calls, silencing:

  [5/8] Compiling C object libcommon.fa.p/hw_dma_pl330.c.o
  hw/dma/pl330.c:333:13: warning: 'sprintf' is deprecated:
This function is provided for compatibility reasons only.
Due to security concerns inherent in the design of sprintf(3),
it is highly recommended that you use snprintf(3) instead.
[-Wdeprecated-declarations]
sprintf(tmpbuf + strlen(tmpbuf), " %02x", buf[b + i]);
^

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/dma/pl330.c | 12 +++-
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
index 70a502d245..0435378b7e 100644
--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -15,6 +15,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
 #include "hw/sysbus.h"
@@ -317,21 +318,14 @@ typedef struct PL330InsnDesc {
 
 static void pl330_hexdump(uint8_t *buf, size_t size)
 {
-unsigned int b, i, len;
-char tmpbuf[80];
+unsigned int b, len;
 
 for (b = 0; b < size; b += 16) {
 len = size - b;
 if (len > 16) {
 len = 16;
 }
-tmpbuf[0] = '\0';
-for (i = 0; i < len; i++) {
-if ((i % 4) == 0) {
-strcat(tmpbuf, " ");
-}
-sprintf(tmpbuf + strlen(tmpbuf), " %02x", buf[b + i]);
-}
+g_autofree char *tmpbuf = qemu_hexdump_line(buf, b, len, false);
 trace_pl330_hexdump(b, tmpbuf);
 }
 }
-- 
2.41.0

[PATCH 09/12] hw/scsi/scsi-disk: Use qemu_hexdump_line() to avoid sprintf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Use qemu_hexdump_line() to avoid sprintf() calls, silencing:

  [105/169] Compiling C object libcommon.fa.p/hw_scsi_scsi-disk.c.o
  hw/scsi/scsi-disk.c:2659:14: warning: 'sprintf' is deprecated:
This function is provided for compatibility reasons only.
Due to security concerns inherent in the design of sprintf(3),
it is highly recommended that you use snprintf(3) instead.
[-Wdeprecated-declarations]
p += sprintf(p, " 0x%02x", buf[i]);
 ^

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/scsi/scsi-disk.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 4bd7af9d0c..4f914df5c2 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2648,16 +2648,12 @@ static const SCSIReqOps *const 
scsi_disk_reqops_dispatch[256] = {
 
 static void scsi_disk_new_request_dump(uint32_t lun, uint32_t tag, uint8_t 
*buf)
 {
-int i;
 int len = scsi_cdb_length(buf);
-char *line_buffer, *p;
+char *line_buffer;
 
 assert(len > 0 && len <= 16);
-line_buffer = g_malloc(len * 5 + 1);
 
-for (i = 0, p = line_buffer; i < len; i++) {
-p += sprintf(p, " 0x%02x", buf[i]);
-}
+line_buffer = qemu_hexdump_line(buf, 0, len, false);
 trace_scsi_disk_new_request(lun, tag, line_buffer);
 
 g_free(line_buffer);
-- 
2.41.0

[PATCH 07/12] util/hexdump: Have qemu_hexdump_line() return heap allocated buffer

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/qemu/cutils.h  | 10 +++---
 hw/virtio/vhost-vdpa.c |  5 +++--
 util/hexdump.c | 12 
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 70ca4b876b..e8d6b86098 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -254,18 +254,22 @@ int parse_debug_env(const char *name, int max, int 
initial);
 
 /**
  * qemu_hexdump_line:
- * @line: Buffer to be filled by the hexadecimal/ASCII dump
  * @bufptr: Buffer to dump
  * @offset: Offset within @bufptr to start the dump
  * @len: Length of the bytes do dump
  * @ascii: Replace non-ASCII characters by the dot symbol
  *
  * Hexdump a line of a byte buffer into a hexadecimal/ASCII buffer
+ *
+ * The caller must use g_free() to free the returned data when it is
+ * no longer required.
+ *
+ * Returns: Hexadecimal/ASCII dump
  */
 #define QEMU_HEXDUMP_LINE_BYTES 16 /* Number of bytes to dump */
 #define QEMU_HEXDUMP_LINE_LEN 75   /* Number of characters in line */
-void qemu_hexdump_line(char *line, const void *bufptr, unsigned offset,
-   unsigned int len, bool ascii);
+char *qemu_hexdump_line(const void *bufptr, unsigned offset,
+unsigned int len, bool ascii);
 
 /*
  * Hexdump a buffer to a file. An optional string prefix is added to every line
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index cf7cfa3f16..e61af86d9d 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -942,12 +942,13 @@ static void vhost_vdpa_dump_config(struct vhost_dev *dev, 
const uint8_t *config,
uint32_t config_len)
 {
 int ofs, len;
-char line[QEMU_HEXDUMP_LINE_LEN];
+char *line;
 
 for (ofs = 0; ofs < config_len; ofs += 16) {
 len = config_len - ofs;
-qemu_hexdump_line(line, config, ofs, len, false);
+line = qemu_hexdump_line(config, ofs, len, false);
 trace_vhost_vdpa_dump_config(dev, line);
+g_free(line);
 }
 }
 
diff --git a/util/hexdump.c b/util/hexdump.c
index 469083d8c0..b6f70e93bb 100644
--- a/util/hexdump.c
+++ b/util/hexdump.c
@@ -16,9 +16,10 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 
-void qemu_hexdump_line(char *line, const void *bufptr, unsigned offset,
-   unsigned int len, bool ascii)
+char *qemu_hexdump_line(const void *bufptr, unsigned offset,
+unsigned int len, bool ascii)
 {
+char linebuf[QEMU_HEXDUMP_LINE_BYTES], *line = linebuf;
 const char *buf = bufptr;
 int i, c;
 
@@ -48,18 +49,21 @@ void qemu_hexdump_line(char *line, const void *bufptr, 
unsigned offset,
 }
 }
 *line = '\0';
+
+return g_strdup(linebuf);
 }
 
 void qemu_hexdump(FILE *fp, const char *prefix,
   const void *bufptr, size_t size)
 {
 unsigned int ofs, len;
-char line[QEMU_HEXDUMP_LINE_LEN];
+char *line;
 
 for (ofs = 0; ofs < size; ofs += QEMU_HEXDUMP_LINE_BYTES) {
 len = size - ofs;
-qemu_hexdump_line(line, bufptr, ofs, len, true);
+line = qemu_hexdump_line(bufptr, ofs, len, true);
 fprintf(fp, "%s: %s\n", prefix, line);
+g_free(line);
 }
 
 }
-- 
2.41.0

[PATCH 06/12] util/hexdump: Rename @offset argument in qemu_hexdump_line()

[Philippe Mathieu-Daudé]

@offset argument is more descriptive than @b.

Inverse @bufptr <-> @offset arguments order.

Document qemu_hexdump_line().

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/qemu/cutils.h  | 11 +--
 hw/virtio/vhost-vdpa.c |  8 
 util/hexdump.c | 16 
 3 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 92c927a6a3..70ca4b876b 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -252,12 +252,19 @@ static inline const char *yes_no(bool b)
  */
 int parse_debug_env(const char *name, int max, int initial);
 
-/*
+/**
+ * qemu_hexdump_line:
+ * @line: Buffer to be filled by the hexadecimal/ASCII dump
+ * @bufptr: Buffer to dump
+ * @offset: Offset within @bufptr to start the dump
+ * @len: Length of the bytes do dump
+ * @ascii: Replace non-ASCII characters by the dot symbol
+ *
  * Hexdump a line of a byte buffer into a hexadecimal/ASCII buffer
  */
 #define QEMU_HEXDUMP_LINE_BYTES 16 /* Number of bytes to dump */
 #define QEMU_HEXDUMP_LINE_LEN 75   /* Number of characters in line */
-void qemu_hexdump_line(char *line, unsigned int b, const void *bufptr,
+void qemu_hexdump_line(char *line, const void *bufptr, unsigned offset,
unsigned int len, bool ascii);
 
 /*
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index e827b9175f..cf7cfa3f16 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -941,12 +941,12 @@ static int vhost_vdpa_set_config_call(struct vhost_dev 
*dev,
 static void vhost_vdpa_dump_config(struct vhost_dev *dev, const uint8_t 
*config,
uint32_t config_len)
 {
-int b, len;
+int ofs, len;
 char line[QEMU_HEXDUMP_LINE_LEN];
 
-for (b = 0; b < config_len; b += 16) {
-len = config_len - b;
-qemu_hexdump_line(line, b, config, len, false);
+for (ofs = 0; ofs < config_len; ofs += 16) {
+len = config_len - ofs;
+qemu_hexdump_line(line, config, ofs, len, false);
 trace_vhost_vdpa_dump_config(dev, line);
 }
 }
diff --git a/util/hexdump.c b/util/hexdump.c
index 9921114b3c..469083d8c0 100644
--- a/util/hexdump.c
+++ b/util/hexdump.c
@@ -16,7 +16,7 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 
-void qemu_hexdump_line(char *line, unsigned int b, const void *bufptr,
+void qemu_hexdump_line(char *line, const void *bufptr, unsigned offset,
unsigned int len, bool ascii)
 {
 const char *buf = bufptr;
@@ -26,13 +26,13 @@ void qemu_hexdump_line(char *line, unsigned int b, const 
void *bufptr,
 len = QEMU_HEXDUMP_LINE_BYTES;
 }
 
-line += snprintf(line, 6, "%04x:", b);
+line += snprintf(line, 6, "%04x:", offset);
 for (i = 0; i < QEMU_HEXDUMP_LINE_BYTES; i++) {
 if ((i % 4) == 0) {
 *line++ = ' ';
 }
 if (i < len) {
-line += sprintf(line, " %02x", (unsigned char)buf[b + i]);
+line += sprintf(line, " %02x", (unsigned char)buf[offset + i]);
 } else {
 line += sprintf(line, "   ");
 }
@@ -40,7 +40,7 @@ void qemu_hexdump_line(char *line, unsigned int b, const void 
*bufptr,
 if (ascii) {
 *line++ = ' ';
 for (i = 0; i < len; i++) {
-c = buf[b + i];
+c = buf[offset + i];
 if (c < ' ' || c > '~') {
 c = '.';
 }
@@ -53,12 +53,12 @@ void qemu_hexdump_line(char *line, unsigned int b, const 
void *bufptr,
 void qemu_hexdump(FILE *fp, const char *prefix,
   const void *bufptr, size_t size)
 {
-unsigned int b, len;
+unsigned int ofs, len;
 char line[QEMU_HEXDUMP_LINE_LEN];
 
-for (b = 0; b < size; b += QEMU_HEXDUMP_LINE_BYTES) {
-len = size - b;
-qemu_hexdump_line(line, b, bufptr, len, true);
+for (ofs = 0; ofs < size; ofs += QEMU_HEXDUMP_LINE_BYTES) {
+len = size - ofs;
+qemu_hexdump_line(line, bufptr, ofs, len, true);
 fprintf(fp, "%s: %s\n", prefix, line);
 }
 
-- 
2.41.0

[PATCH 12/12] backends/tpm: Use qemu_hexdump_line() to avoid sprintf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Use qemu_hexdump_line() to avoid sprintf() calls, silencing:

  backends/tpm/tpm_util.c:357:14: warning: 'sprintf' is deprecated:
This function is provided for compatibility reasons only.
Due to security concerns inherent in the design of sprintf(3),
it is highly recommended that you use snprintf(3) instead.
[-Wdeprecated-declarations]
p += sprintf(p, "%.2X ", buffer[i]);
 ^

Signed-off-by: Philippe Mathieu-Daudé 
---
 backends/tpm/tpm_util.c | 24 
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
index 1856589c3b..0747af2d1c 100644
--- a/backends/tpm/tpm_util.c
+++ b/backends/tpm/tpm_util.c
@@ -21,6 +21,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/error-report.h"
+#include "qemu/cutils.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
 #include "tpm_int.h"
@@ -336,27 +337,18 @@ void tpm_sized_buffer_reset(TPMSizedBuffer *tsb)
 void tpm_util_show_buffer(const unsigned char *buffer,
   size_t buffer_size, const char *string)
 {
-size_t len, i;
-char *line_buffer, *p;
+size_t len;
+char *line, *lineup;
 
 if (!trace_event_get_state_backends(TRACE_TPM_UTIL_SHOW_BUFFER)) {
 return;
 }
 len = MIN(tpm_cmd_get_size(buffer), buffer_size);
 
-/*
- * allocate enough room for 3 chars per buffer entry plus a
- * newline after every 16 chars and a final null terminator.
- */
-line_buffer = g_malloc(len * 3 + (len / 16) + 1);
+line = qemu_hexdump_line(buffer, 0, len, false);
+lineup = g_ascii_strup(line, -1);
+trace_tpm_util_show_buffer(string, len, lineup);
 
-for (i = 0, p = line_buffer; i < len; i++) {
-if (i && !(i % 16)) {
-p += sprintf(p, "\n");
-}
-p += sprintf(p, "%.2X ", buffer[i]);
-}
-trace_tpm_util_show_buffer(string, len, line_buffer);
-
-g_free(line_buffer);
+g_free(line);
+g_free(lineup);
 }
-- 
2.41.0

[PATCH 01/12] ui/console-vc: Replace sprintf() by g_strdup_printf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Replace sprintf() by g_strdup_printf() in order to avoid:

  [702/1310] Compiling C object libcommon.fa.p/ui_console-vc.c.o
  ui/console-vc.c:824:21: warning: 'sprintf' is deprecated:
This function is provided for compatibility reasons only.
Due to security concerns inherent in the design of sprintf(3),
it is highly recommended that you use snprintf(3) instead.
[-Wdeprecated-declarations]
sprintf(response, "\033[%d;%dR",
^
  1 warning generated.

Signed-off-by: Philippe Mathieu-Daudé 
---
 ui/console-vc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/console-vc.c b/ui/console-vc.c
index 899fa11c94..b455db436d 100644
--- a/ui/console-vc.c
+++ b/ui/console-vc.c
@@ -648,7 +648,7 @@ static void vc_putchar(VCChardev *vc, int ch)
 QemuTextConsole *s = vc->console;
 int i;
 int x, y;
-char response[40];
+g_autofree char *response = NULL;
 
 switch(vc->state) {
 case TTY_STATE_NORM:
@@ -821,7 +821,7 @@ static void vc_putchar(VCChardev *vc, int ch)
 break;
 case 6:
 /* report cursor position */
-sprintf(response, "\033[%d;%dR",
+response = g_strdup_printf("\033[%d;%dR",
(s->y_base + s->y) % s->total_height + 1,
 s->x + 1);
 vc_respond_str(vc, response);
-- 
2.41.0

[PATCH 08/12] util/hexdump: Replace sprintf() by g_string_append_printf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Replace sprintf() by GString API in order to avoid:

  [426/1310] Compiling C object libqemuutil.a.p/util_hexdump.c.o
  util/hexdump.c:35:21: warning: 'sprintf' is deprecated:
This function is provided for compatibility reasons only.
Due to security concerns inherent in the design of sprintf(3),
it is highly recommended that you use snprintf(3) instead.
[-Wdeprecated-declarations]
line += sprintf(line, " %02x", (unsigned char)buf[b + i]);
^
  util/hexdump.c:37:21: warning: 'sprintf' is deprecated:
line += sprintf(line, "   ");
^
  2 warnings generated.

Signed-off-by: Philippe Mathieu-Daudé 
---
 util/hexdump.c | 17 -
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/util/hexdump.c b/util/hexdump.c
index b6f70e93bb..2ec1171de3 100644
--- a/util/hexdump.c
+++ b/util/hexdump.c
@@ -19,7 +19,7 @@
 char *qemu_hexdump_line(const void *bufptr, unsigned offset,
 unsigned int len, bool ascii)
 {
-char linebuf[QEMU_HEXDUMP_LINE_BYTES], *line = linebuf;
+g_autoptr(GString) gs = g_string_sized_new(QEMU_HEXDUMP_LINE_BYTES);
 const char *buf = bufptr;
 int i, c;
 
@@ -27,30 +27,29 @@ char *qemu_hexdump_line(const void *bufptr, unsigned offset,
 len = QEMU_HEXDUMP_LINE_BYTES;
 }
 
-line += snprintf(line, 6, "%04x:", offset);
+g_string_append_printf(gs, "%04x:", offset);
 for (i = 0; i < QEMU_HEXDUMP_LINE_BYTES; i++) {
 if ((i % 4) == 0) {
-*line++ = ' ';
+g_string_append_c(gs, ' ');
 }
 if (i < len) {
-line += sprintf(line, " %02x", (unsigned char)buf[offset + i]);
+g_string_append_printf(gs, " %02x", (unsigned char)buf[offset + 
i]);
 } else {
-line += sprintf(line, "   ");
+g_string_append(gs, "   ");
 }
 }
 if (ascii) {
-*line++ = ' ';
+g_string_append_c(gs, ' ');
 for (i = 0; i < len; i++) {
 c = buf[offset + i];
 if (c < ' ' || c > '~') {
 c = '.';
 }
-*line++ = c;
+g_string_append_c(gs, c);
 }
 }
-*line = '\0';
 
-return g_strdup(linebuf);
+return g_strdup(gs->str);
 }
 
 void qemu_hexdump(FILE *fp, const char *prefix,
-- 
2.41.0

[PATCH 05/12] system/qtest: Replace sprintf() by g_string_append_printf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Replace sprintf() by GString API uses in order to avoid:

  [120/169] Compiling C object libcommon.fa.p/system_qtest.c.o
  system/qtest.c:623:13: warning: 'sprintf' is deprecated:
This function is provided for compatibility reasons only.
Due to security concerns inherent in the design of sprintf(3),
it is highly recommended that you use snprintf(3) instead.
[-Wdeprecated-declarations]
sprintf([i * 2], "%02x", data[i]);
^
  1 warning generated.

Signed-off-by: Philippe Mathieu-Daudé 
---
 system/qtest.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/system/qtest.c b/system/qtest.c
index 6da58b3874..22bf1a33dc 100644
--- a/system/qtest.c
+++ b/system/qtest.c
@@ -601,9 +601,9 @@ static void qtest_process_command(CharBackend *chr, gchar 
**words)
 qtest_send_prefix(chr);
 qtest_sendf(chr, "OK 0x%016" PRIx64 "\n", value);
 } else if (strcmp(words[0], "read") == 0) {
+g_autoptr(GString) enc = g_string_new("");
 uint64_t addr, len, i;
 uint8_t *data;
-char *enc;
 int ret;
 
 g_assert(words[1] && words[2]);
@@ -618,16 +618,14 @@ static void qtest_process_command(CharBackend *chr, gchar 
**words)
 address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
len);
 
-enc = g_malloc(2 * len + 1);
 for (i = 0; i < len; i++) {
-sprintf([i * 2], "%02x", data[i]);
+g_string_append_printf(enc, "%02x", data[i]);
 }
 
 qtest_send_prefix(chr);
-qtest_sendf(chr, "OK 0x%s\n", enc);
+qtest_sendf(chr, "OK 0x%s\n", enc->str);
 
 g_free(data);
-g_free(enc);
 } else if (strcmp(words[0], "b64read") == 0) {
 uint64_t addr, len;
 uint8_t *data;
-- 
2.41.0

[PATCH 00/12] misc: Remove sprintf() due to macOS deprecation

[Philippe Mathieu-Daudé]

Hi,

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Suggestion to avoid the super-noisy warning on macOS forum are [*]:

* use -Wno-deprecated-declarations on the whole build
* surgically add #pragma clang diagnostic around each use.

None of these options seem reasonable, so we are somehow forced to
spend time converting each sprintf() call, even if they are safe
enough.

I'm so tired of seeing them than I started the conversion. This is
the first part. Help for the rest is welcomed.

Regards,

Phil.

[*] https://forums.developer.apple.com/forums/thread/714675

Philippe Mathieu-Daudé (12):
  ui/console-vc: Replace sprintf() by g_strdup_printf()
  hw/vfio/pci: Replace sprintf() by g_strdup_printf()
  hw/ppc/spapr: Replace sprintf() by g_strdup_printf()
  hw/mips/malta: Replace sprintf() by g_string_append_printf()
  system/qtest: Replace sprintf() by g_string_append_printf()
  util/hexdump: Rename @offset argument in qemu_hexdump_line()
  util/hexdump: Have qemu_hexdump_line() return heap allocated buffer
  util/hexdump: Replace sprintf() by g_string_append_printf()
  hw/scsi/scsi-disk: Use qemu_hexdump_line() to avoid sprintf()
  hw/ide/atapi: Use qemu_hexdump_line() to avoid sprintf()
  hw/dma/pl330: Use qemu_hexdump_line() to avoid sprintf()
  backends/tpm: Use qemu_hexdump_line() to avoid sprintf()

 include/qemu/cutils.h   | 17 ++---
 backends/tpm/tpm_util.c | 24 
 hw/dma/pl330.c  | 12 +++-
 hw/ide/atapi.c  |  8 ++--
 hw/mips/malta.c | 22 +-
 hw/ppc/spapr.c  |  4 ++--
 hw/scsi/scsi-disk.c |  8 ++--
 hw/vfio/pci.c   |  7 +++
 hw/virtio/vhost-vdpa.c  | 11 ++-
 system/qtest.c  |  8 +++-
 ui/console-vc.c |  4 ++--
 util/hexdump.c  | 33 ++---
 12 files changed, 76 insertions(+), 82 deletions(-)

-- 
2.41.0

[PATCH 02/12] hw/vfio/pci: Replace sprintf() by g_strdup_printf()

[Philippe Mathieu-Daudé]

sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience. Use g_strdup_printf()
instead.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/vfio/pci.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 64780d1b79..cc3cc89122 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2442,10 +2442,9 @@ void vfio_pci_post_reset(VFIOPCIDevice *vdev)
 
 bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name)
 {
-char tmp[13];
-
-sprintf(tmp, "%04x:%02x:%02x.%1x", addr->domain,
-addr->bus, addr->slot, addr->function);
+g_autofree char *tmp = g_strdup_printf("%04x:%02x:%02x.%1x",
+   addr->domain, addr->bus,
+   addr->slot, addr->function);
 
 return (strcmp(tmp, name) == 0);
 }
-- 
2.41.0

Re: [PULL 00/16] Misc HW patches for 2024-04-10

[Peter Maydell]

On Wed, 10 Apr 2024 at 10:14, Philippe Mathieu-Daudé  wrote:
>
> The following changes since commit 927284d65bce63ab1495d3febe7c7b5b6d563874:
>
>   Merge tag 'edk2-20240409-pull-request' of https://gitlab.com/kraxel/qemu 
> into staging (2024-04-09 17:36:40 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/philmd/qemu.git tags/hw-misc-20240410
>
> for you to fetch changes up to dcb0a1ac03d6b5ba6c7fcbe467f0215738006113:
>
>   hw/audio/virtio-snd: Remove unused assignment (2024-04-10 11:07:37 +0200)
>
> 
> Misc HW patch queue
>
> - Fix CXL Fixed Memory Window interleave-granularity typo
> - Fix for DMA re-entrancy abuse with VirtIO devices (CVE-2024-3446)
> - Fix out-of-bound access in NAND block buffer
> - Fix memory leak in AppleSMC reset() handler
> - Avoid VirtIO crypto backends abort o invalid session ID
> - Fix overflow in LAN9118 MIL TX FIFO
> - Fix overflow when abusing SDHCI TRNMOD register (CVE-2024-3447)
> - Fix overrun in short fragmented packet SCTP checksum (CVE-2024-3567)
> - Remove unused assignment in virtio-snd model (Coverity 1542933 & 1542934)
>
> 


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/9.0
for any user-visible changes.

-- PMM

Re: secure boot & direct kernel load (was: Re: [PATCH] x86/loader: only patch linux kernels)

[Gerd Hoffmann]

> > > Options I see:
> > > 
> > >   (a) Stop using direct kernel boot, let virt-install & other tools
> > >   create vfat boot media with shim+kernel+initrd instead.
> > > 
> > >   (b) Enroll the distro signing keys in the efi variable store, so
> > >   booting the kernel without shim.efi works.
> > > 
> > >   (c) Add support for loading shim to qemu (and ovmf), for example
> > >   with a new '-shim' command line option which stores shim.efi
> > >   in some new fw_cfg file.
> > > 
> > > (b) + (c) both require a fix for the patching issue.  The options
> > > I see here are:
> > > 
> > >   (A) Move the patching from qemu to the linuxboot option rom.
> > >   Strictly speaking it belongs there anyway.  It doesn't look
> > >   that easy though, for qemu it is easier to gather all
> > >   information needed ...
> > > 
> > >   (B) Provide both patched and unpatched setup header, so the
> > >   guest can choose what it needs.
> > > 
> > >   (C) When implementing (c) above we can piggyback on the -shim
> > >   switch and skip patching in case it is present.
> > > 
> > >   (D) Add a flag to skip the patching.
> > > 
> > > Comments?  Other/better ideas?
> > > 
> > > take care,
> > >   Gerd
> > 
> > So if you didn't decide whether to do b or c then I guess D is
> > easiest and covers both cases?
> 
> Easiest if you look at qemu only.  Adding a new config option adds
> burdens elsewhere though.  Users and the management stack have to
> learn to use the new option.
> 
> Both (A) and (B) work automatically and can be combined with both (b)
> and (c).  (B) is probably much easier to implement, drawback is it
> requires an firmware update too.

Sneak preview for (c) + (B) is here:
  https://git.kraxel.org/cgit/qemu/log/?h=sirius/direct-secure-boot

(well, almost, instead of unpatched setup header it exposes an unpatched
kernel binary).

Currently looking at the ovmf side of things to make sure the idea
actually works before posting patches to the list.

take care,
  Gerd

Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"

[Peter Xu]

On Wed, Apr 10, 2024 at 10:04:33AM -0300, Fabiano Rosas wrote:
> Het Gala  writes:
> 
> > This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb
> >
> > After addition of 'channels' as the starting argument of new QAPI
> > syntax inside postcopy test, even if the user entered the old QAPI
> > syntax, test used the new syntax.
> > It was a temporary patch added to have some presence of the new syntax
> > since the migration qtest framework lacked any logic for introducing
> > 'channels' argument.
> 
> That wasn't clear to me when we merged that. Was that really the case?

Yeah these look all a bit confusing..

I'm wondering whether do we need the new interface to cover both precopy
and postcopy, or one would suffice?

Both should share the same interface.  I think it means if we covered the
channels interface in precopy, then perhaps we don't need to test anywhere
else, as we got the code paths all covered.

We actually do the same already for all kinds of channels for postcopy,
where we stick with either tcp/unix but don't cover the rest.

-- 
Peter Xu

Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling

[Peter Xu]

On Wed, Apr 10, 2024 at 02:28:59AM +, Zhijian Li (Fujitsu) via wrote:
> 
> 
> on 4/10/2024 3:46 AM, Peter Xu wrote:
> 
> >> Is there document/link about the unittest/CI for migration tests, Why
> >> are those tests missing?
> >> Is it hard or very special to set up an environment for that? maybe we
> >> can help in this regards.
> > See tests/qtest/migration-test.c.  We put most of our migration tests
> > there and that's covered in CI.
> >
> > I think one major issue is CI systems don't normally have rdma devices.
> > Can rdma migration test be carried out without a real hardware?
> 
> Yeah,  RXE aka. SOFT-RoCE is able to emulate the RDMA, for example
> $ sudo rdma link add rxe_eth0 type rxe netdev eth0  # on host
> then we can get a new RDMA interface "rxe_eth0".
> This new RDMA interface is able to do the QEMU RDMA migration.
> 
> Also, the loopback(lo) device is able to emulate the RDMA interface 
> "rxe_lo", however when
> I tried(years ago) to do RDMA migration over this 
> interface(rdma:127.0.0.1:) , it got something wrong.
> So i gave up enabling the RDMA migration qtest at that time.

Thanks, Zhijian.

I'm not sure adding an emu-link for rdma is doable for CI systems, though.
Maybe someone more familiar with how CI works can chim in.

-- 
Peter Xu

Re: [PATCH] tests/qtest: Standardize qtest function caller strings.

[Fabiano Rosas]

Het Gala  writes:

> On 05/04/24 7:58 pm, Fabiano Rosas wrote:
>> !---|
>>CAUTION: External Email
>>
>> |---!
>>
>> Het Gala  writes:
>>
>>> On 27/03/24 2:37 am, Fabiano Rosas wrote:
 Het Gala   writes:

 Some comments, mostly just thinking out loud...

> For  --> migrate
> //
> //O:/...
>
> For  --> validate
> ///O:/O:/
> /O:/O:/...
 Do we need an optional 'capability' element? I'm not sure how practical
 is to leave that as 'others', because that puts it at the end of the
 string. We'd want the element that's more important/with more variants
 to be towards the start of the string so we can run all tests of the
 same kind with the -r option.
>>> While also looking at different functions for figuring out the transport
>>> and invocation, my observation was that, there might be many capabilities
>>> added to the same test, while it might not be important also.
>>> Ex: /migrate/multifd/tcp/plain
>>> 1. multifd is defined as a migration mode.
>>> 2. It is also a capability, and comes in 2 parts [multifd, multifd-channels]
>>>      though one is a capability and another is parameter
>>> Similarly in other examples of compression, there are many capabilities
>>> and parameters added, but it might be not important to mention that ?
>>>
>>> Secondly, there are multiple migration capabilities IIRC (> 15). And a test
>>> requiring multiple capabilities, the overall string would be too long, and
>>> not that important also to mention all capabilities.
>>>
>>> Just thinking out of mind - Can we have selective list of capabilities ?
>>> 1. multifd 2. compress (again, there might be confusion with multifd
>>> compression methods like zstd, zlib and just 'compress') 3. zero-page
>>> (This will have sub capabilities ?)
>> I was thinking of keeping that part more open-ended. So not specifying
>> capabilities one by one, but more like "if you're testing a capability,
>> it comes here".
>>
>> About multifd, it's a bit special since it cannot be seen as just a
>> "feature" anymore. It's a core part of the migration code. I wouldn't
>> classify it as capability for the purposes of the tests.
> Ack, got it.

Meta: it's a good idea to add a blank line before and after your reply
when replying inline like this, it makes it easier on the eyes to spot
the various snippets in a wall of text (note how this reply itself has
extra blank lines before and after it).

> test-type:: migrate | validate
 We could alternatively drop migration|migrate|validate. They are kind of
 superfluous.
>>> I agree with the above comment. 'migrate' and 'validate' have a different
>>> set of variables required, some necessary, while other optional. IMO this
>>> will help is in streamlining the design further.
> migration-mode
> a. migrate --> :: precopy | postcopy | multifd
> b. validate -->:: (what to validate)
> methods  :: preempt | recovery | reboot | suspend | simple
>>> I want some inputs here.
>>> 1. is there a better variable name rather than 'methods'
>> Does this fall into the "mode" terminology that Steven introduced?
> Yes, as we decided that we don't want 'migration-mode' key-value pair,
> naming 'mode' would be a better term.
>
> In cases, where multiple modes are to be used ex: postcopy_preempt_recovery
> I feel it might be a good idea to separate multiple modes by '-'
> For example - .../preempty-recovery/...
> Similarly for other keys too if required

Possibly... as long as we don't lose the ability of running subsets of
tests in one command, i.e. "all postcopy tests", "all postcopy recovery
tests", etc.

>>> 2. 'simple' does not fit perfect here IMO.
>> Can we go without it?
> You mean omit the key itself in case of a no-op ?

Yes

> transport:: tcp | fd | unix | file
> invocation   :: uri | channels | both
> CompressionType  :: zlib | zstd | none
 s/none/nocomp/ ? We're already familiar with that.
>>> Ack. Will change that.
> encryptionType   :: tls | plain
 s/plain/notls/ ?
>>> What if there is another encryption technique in future ?
 Or maybe we simply omit the noop options. It would make the string way
 shorter in most cases.
>>> This might be a better approach. Can have some keys/variables as optional
>>> while some necessary. For ex: for 'migrate' - transport and invocation
>>> might be necessary while it might not be necessary for 'validate' qtests
>> Yep
> Ack, will do that!
> validate-test-result :: success | failure
> others   :: other comments/capability that needs to be
>   addressed. Can be multiple
>
> (more than one applicable, separated by using '-' in between)
> O: optional
>
> Signed-off-by: Het Gala
> Suggested-by: Fabiano 

Re: [PULL v2 00/20] misc patch queue

[Peter Maydell]

On Tue, 9 Apr 2024 at 20:39, Richard Henderson
 wrote:
>
> Defer 16 patches to 9.1; add fix for -strace.
>
> r~
>
>
> The following changes since commit bc0cd4ae881dff47e81581a8fea93a50b1d1dbe7:
>
>   Merge tag 'for_upstream' of 
> https://git.kernel.org/pub/scm/virt/kvm/mst/qemu into staging (2024-04-09 
> 09:51:07 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/rth7680/qemu.git tags/pull-misc-20240409
>
> for you to fetch changes up to 143bcc1d59f174b6c6743bd4ca8f99415ed1aba2:
>
>   linux-user: Preserve unswapped siginfo_t for strace (2024-04-09 07:47:11 
> -1000)
>
> 
> target/m68k: Fix fp accrued exception reporting
> target/hppa: Fix IIAOQ, IIASQ for pa2.0
> target/sh4: Fixes to mac.l and mac.w saturation
> target/sh4: Fixes to illegal delay slot reporting
> linux-user: Fix waitid return of siginfo_t and rusage
> linux-user: Preserve unswapped siginfo_t for strace
> tcg/optimize: Do not attempt to constant fold neg_vec
> accel/tcg: Improve can_do_io management, mmio bug fix
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/9.0
for any user-visible changes.

-- PMM

Re: [PATCH RFC v1]display: fix heap use after free in cursor_put

[Marc-André Lureau]

Hi

On Wed, Apr 10, 2024 at 2:06 PM ゞlym <707242...@qq.com> wrote:
>
>

Please send the patch as inline:
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#do-not-send-as-an-attachment

The patch is doing too much changes to the ssd.lock usage without
explaining in detail which race and how it solved it.

In particular, ui/spice-display.c usage seems safer before your
change, since it takes the lock on display_refresh and
display_mouse_define. It properly temporarily releases the lock before
calling the dpy_mouse_set() and dpy_cursor_define() as well.

To me, it looks like the only offender is qxl_spice_reset_cursor(),
which lacks locking before unrefing.

Could you confirm this hypothesis if you are able to reproduce the issue?

thanks

Re: [PATCH 4/4] tests/qtest/migration: Add postcopy migration qtests to use 'channels' argument instead of uri

[Fabiano Rosas]

Het Gala  writes:

> Add qtests to perform postcopy live migration by having list of
> 'channels' argument as the starting point instead of uri string.
> (Note: length of the list is restricted to 1 for now)
>
> Signed-off-by: Het Gala 
> ---
>  tests/qtest/migration-test.c | 38 ++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index fa8a860811..599018baa0 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1296,13 +1296,17 @@ static int migrate_postcopy_prepare(QTestState 
> **from_ptr,
>  migrate_ensure_non_converge(from);
>  
>  migrate_prepare_for_dirty_mem(from);
> -migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
> +if (args->connect_channels) {
> +migrate_incoming_qmp(to, NULL, args->connect_channels, "{}");
> +} else {
> +migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
> +}

>From an interface perspective it's a bit unexpected to need this
conditional when the migrate_qmp below doesn't need it.

>  
>  /* Wait for the first serial output from the source */
>  wait_for_serial("src_serial");
>  wait_for_suspend(from, _state);
>  
> -migrate_qmp(from, to, NULL, NULL, "{}");
> +migrate_qmp(from, to, args->connect_uri, args->connect_channels, "{}");
>  
>  migrate_wait_for_dirty_mem(from, to);
>  
> @@ -1355,6 +1359,20 @@ static void test_postcopy(void)
>  test_postcopy_common();
>  }
>  
> +static void test_postcopy_channels(void)
> +{
> +MigrateCommon args = {
> +.listen_uri = "defer",
> +.connect_channels = "[ { 'channel-type': 'main',"
> +"'addr': { 'transport': 'socket',"
> +"  'type': 'inet',"
> +"  'host': '127.0.0.1',"
> +"  'port': '0' } } ]",
> +};
> +
> +test_postcopy_common();
> +}
> +
>  static void test_postcopy_suspend(void)
>  {
>  MigrateCommon args = {
> @@ -1555,6 +1573,18 @@ static void test_postcopy_recovery(void)
>  test_postcopy_recovery_common();
>  }
>  
> +static void test_postcopy_recovery_channels(void)
> +{
> +MigrateCommon args = {
> +.connect_channels = "[ { 'channel-type': 'main',"
> +"'addr': { 'transport': 'socket',"
> +"  'type': 'inet',"
> +"  'host': '127.0.0.1',"
> +"  'port': '0' } } ]",
> +};
> +
> +test_postcopy_recovery_common();
> +}
>  static void test_postcopy_recovery_compress(void)
>  {
>  MigrateCommon args = {
> @@ -3585,8 +3615,12 @@ int main(int argc, char **argv)
>  
>  if (has_uffd) {
>  migration_test_add("/migration/postcopy/plain", test_postcopy);
> +migration_test_add("/migration/postcopy/channels/plain",
> +   test_postcopy_channels);
>  migration_test_add("/migration/postcopy/recovery/plain",
> test_postcopy_recovery);
> +migration_test_add("/migration/postcopy/recovery/channels/plain",
> +   test_postcopy_recovery_channels);
>  migration_test_add("/migration/postcopy/preempt/plain",
> test_postcopy_preempt);
>  migration_test_add("/migration/postcopy/preempt/recovery/plain",

Re: [PATCH 3/4] tests/qtest/migration: Add channels parameter in migrate_incoming_qmp

[Fabiano Rosas]

Het Gala  writes:

> Alter migrate_incoming_qmp() to allow both uri and channels
> independently. For channels, convert string to a QDict.
>
> Signed-off-by: Het Gala 
> ---
>  tests/qtest/migration-helpers.c   | 13 +++--
>  tests/qtest/migration-helpers.h   |  4 ++--
>  tests/qtest/migration-test.c  | 12 ++--
>  tests/qtest/virtio-net-failover.c |  8 
>  4 files changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index 3b72cad6c1..cbd91719ae 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -245,7 +245,8 @@ void migrate_set_capability(QTestState *who, const char 
> *capability,
>   capability, value);
>  }
>  
> -void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, 
> ...)
> +void migrate_incoming_qmp(QTestState *to, const char *uri,
> +  const char *channels, const char *fmt, ...)
>  {
>  va_list ap;
>  QDict *args, *rsp, *data;
> @@ -255,7 +256,15 @@ void migrate_incoming_qmp(QTestState *to, const char 
> *uri, const char *fmt, ...)
>  va_end(ap);
>  
>  g_assert(!qdict_haskey(args, "uri"));
> -qdict_put_str(args, "uri", uri);
> +if (uri) {
> +qdict_put_str(args, "uri", uri);
> +}
> +
> +g_assert(!qdict_haskey(args, "channels"));
> +if (channels) {
> +QObject *channels_obj = qobject_from_json(channels, _abort);
> +qdict_put_obj(args, "channels", channels_obj);
> +}

Do you think it makes sense to have channels take precedence here? It
would make the next patch cleaner without having to check for channels
presence. I don't think we need a 'both' test for incoming.

>  
>  migrate_set_capability(to, "events", true);
>  
> diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
> index 1339835698..9f74281aea 100644
> --- a/tests/qtest/migration-helpers.h
> +++ b/tests/qtest/migration-helpers.h
> @@ -29,9 +29,9 @@ G_GNUC_PRINTF(5, 6)
>  void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>   const char *channels, const char *fmt, ...);
>  
> -G_GNUC_PRINTF(3, 4)
> +G_GNUC_PRINTF(4, 5)
>  void migrate_incoming_qmp(QTestState *who, const char *uri,
> -  const char *fmt, ...);
> +  const char *channels, const char *fmt, ...);
>  
>  G_GNUC_PRINTF(4, 5)
>  void migrate_qmp_fail(QTestState *who, const char *uri,
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index f2c27d611c..fa8a860811 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1296,7 +1296,7 @@ static int migrate_postcopy_prepare(QTestState 
> **from_ptr,
>  migrate_ensure_non_converge(from);
>  
>  migrate_prepare_for_dirty_mem(from);
> -migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
> +migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
>  
>  /* Wait for the first serial output from the source */
>  wait_for_serial("src_serial");
> @@ -1824,7 +1824,7 @@ static void test_file_common(MigrateCommon *args, bool 
> stop_src)
>   * We need to wait for the source to finish before starting the
>   * destination.
>   */
> -migrate_incoming_qmp(to, args->connect_uri, "{}");
> +migrate_incoming_qmp(to, args->connect_uri, NULL, "{}");
>  wait_for_migration_complete(to);
>  
>  if (stop_src) {
> @@ -2405,7 +2405,7 @@ static void *test_migrate_fd_start_hook(QTestState 
> *from,
>  close(pair[0]);
>  
>  /* Start incoming migration from the 1st socket */
> -migrate_incoming_qmp(to, "fd:fd-mig", "{}");
> +migrate_incoming_qmp(to, "fd:fd-mig", NULL, "{}");
>  
>  /* Send the 2nd socket to the target */
>  qtest_qmp_fds_assert_success(from, [1], 1,
> @@ -2715,7 +2715,7 @@ 
> test_migrate_precopy_tcp_multifd_start_common(QTestState *from,
>  migrate_set_capability(to, "multifd", true);
>  
>  /* Start incoming migration from the 1st socket */
> -migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
> +migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
>  
>  return NULL;
>  }
> @@ -3040,7 +3040,7 @@ static void test_multifd_tcp_cancel(void)
>  migrate_set_capability(to, "multifd", true);
>  
>  /* Start incoming migration from the 1st socket */
> -migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
> +migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
>  
>  /* Wait for the first serial output from the source */
>  wait_for_serial("src_serial");
> @@ -3068,7 +3068,7 @@ static void test_multifd_tcp_cancel(void)
>  migrate_set_capability(to2, "multifd", true);
>  
>  /* Start incoming migration from the 1st socket */
> -migrate_incoming_qmp(to2, "tcp:127.0.0.1:0", "{}");
> +migrate_incoming_qmp(to2, "tcp:127.0.0.1:0", NULL, "{}");
>  
>  

Re: [PATCH 2/4] tests/qtest/migration: Replace 'migrate-incoming' qtest_qmp_assert_success with migrate_incoming_qmp

[Fabiano Rosas]

Het Gala  writes:

> Already have a migrate_incoming_qmp helper function to initiate
> 'migrate-incoming' QMP command with some additional checks.
> Replace 'migrate-incoming' qtest_qmp_assert_success command with
> calling migrate_incoming_qmp helper function for postcopy qtests.
>
> Signed-off-by: Het Gala 

Reviewed-by: Fabiano Rosas 

Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"

[Fabiano Rosas]

Het Gala  writes:

> This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb
>
> After addition of 'channels' as the starting argument of new QAPI
> syntax inside postcopy test, even if the user entered the old QAPI
> syntax, test used the new syntax.
> It was a temporary patch added to have some presence of the new syntax
> since the migration qtest framework lacked any logic for introducing
> 'channels' argument.

That wasn't clear to me when we merged that. Was that really the case?

>
> Now that the qtest framework has logic to introduce uri and channel
> argument separately, we can remove this temporary change.
>
> Signed-off-by: Het Gala 

Anyway, I can see the point of this.

Reviewed-by: Fabiano Rosas 

Re: [PATCH v13 00/24] target/arm: Implement FEAT_NMI and FEAT_GICv3_NMI

[Peter Maydell]

On Wed, 10 Apr 2024 at 07:19, Jinjie Ruan via  wrote:
>
> Ping.

As I said in my reply on the previous version, we're in
freeze at the moment, so this patchset is not going anywhere
until 9.0 releases. I think it's in shape to apply after that.

thanks
-- PMM

Re: [PATCH v5 2/2] nbd/server: Mark negotiation functions as coroutine_fn

[Eric Blake]

On Wed, Apr 10, 2024 at 10:05:28AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > @@ -755,7 +764,8 @@ struct NBDTLSServerHandshakeData {
> > > >Coroutine *co;
> > > >};
> > > > 
> > > > -static void nbd_server_tls_handshake(QIOTask *task, void *opaque)
> > > > +static coroutine_fn void
> > > 
> > > This is not, that's a callback for tls handshake, which is not coroutine 
> > > context as I understand.
> > 
> > The call chain is:
> > 
> > nbd_negotiate() - coroutine_fn before this patch
> >nbd_negotiate_options() - marked coroutine_fn here
> >  nbd_negotiate_handle_starttls() - marked coroutine_fn here
> >qio_channel_tls_handshake() - in io/channel-tls.c; not marked 
> > coroutine_fn, but
> >  works both in or out of coroutine 
> > context
> >  ...
> >  nbd_server_tls_handshake() - renamed in 1/2 of this series
> > 
> > > without this hunk:
> > 
> > I can drop that particular marking.  There are cases where the
> > callback is reached without having done the qemu_coroutine_yield() in
> > nbd_negotiate_handle_starttls; but there are also cases where the IO
> > took enough time that the coroutine yielded and it is that callback
> > that reawakens it.
> 
> The key thing for me is that in this case (when coroutine yielded), 
> nbd_server_tls_handshake() would finally be called from glib IO handler, set 
> in
> 
>qio_channel_tls_handshake()
>   qio_channel_tls_handshake_task()
>  qio_channel_add_watch_full()
> g_source_set_callback(source, (GSourceFunc)func, user_data, 
> notify);   <<<
> 
> , which would definitely not be a coroutine context.
> 
> 
> Do I understand correctly, that "coroutine_fn" means "only call from 
> coroutine context"[1], or does it mean "could be called from coroutine 
> context"[2]?

I'm always fuzzy on that distinction myself.  But reading the docs helps...


> 
> The comment in osdep.h says:
> 
>  * Mark a function that executes in coroutine context 
> |}
>  *
> |
>  * Functions that execute in coroutine context cannot be called directly from 
> |
>  * normal functions. ...
> 
> So I assume, we mean [1].

...[1] sounds more appropriate.  Adding the marker is more of a
promise that "I've audited that this function does not block and is
therefore safe for a function in coroutine context to use", but
sometimes additionally implies "this function assumes a coroutine is
present; if you are not in a coroutine, things might break".  But with
a bit of thought, it is obvious that a coroutine can call functions
that are not marked with coroutine_fn: any non-blocking syscall fits
in this category (since we don't have control over system headers to
add coroutine_fn annotations to those).  So on that grounds, it is
okay for a function marked coroutine_fn to call another function not
marked coroutine_fn - it just makes the auditing harder to ensure that
you aren't violating your promise of no non-blocking calls.

It's too close to 9.0-rc3 for my comfort to include this patch series.
Even though this bug can cause wedged migrations, I'd feel more
comfortable with preparing the pull request for this series (including
your fix for dropping this one annotation) for 9.1 and for qemu-stable
once the release is complete.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [QEMU][PATCH v3 0/7] Xen: support grant mappings.

[Edgar E. Iglesias]

On Wed, Feb 28, 2024 at 8:00 PM Vikram Garhwal 
wrote:

> Hi Manos,
> On Wed, Feb 28, 2024 at 03:27:12PM +0200, Manos Pitsidianakis wrote:
> > Hello Vikram,
> >
> > Series doesn't apply on master. Can you rebase and also provide a
> > base-commit with --base= when you use git-format-patch? This
> > will help git rebase if there are any conflicts found locally.
> I rebased it with latest master and it works fine. Series is based on
> following
> commit: bfe8020c814a30479a4241aaa78b63960655962b.
>
> For v4, I will send a version with base-commit id.
>
> Can you please share what is base-commit id on your side?
>
> Thanks!
> >
> > Thanks,
>

Hi all,

I'll send a v4 on behalf of Vikram.

Stefano, I saw your comments here:
https://marc.info/?l=qemu-devel=16555103080
I've manage to loose the email so I can't reply but you indicated that
you're OK with the current patch but perhaps would have preferred another
approach.
I like what Juergen did (so I RB tagged it) but if you feel strongly about
finding another approach we can have a look.

Best regards,
Edgar

Re: [PATCH] tests/qtest: Standardize qtest function caller strings.

[Het Gala]

On 05/04/24 7:58 pm, Fabiano Rosas wrote:

!---|
   CAUTION: External Email

|---!

Het Gala  writes:


On 27/03/24 2:37 am, Fabiano Rosas wrote:

Het Gala   writes:

Some comments, mostly just thinking out loud...


For  --> migrate
//
//O:/...

For  --> validate
///O:/O:/
/O:/O:/...

Do we need an optional 'capability' element? I'm not sure how practical
is to leave that as 'others', because that puts it at the end of the
string. We'd want the element that's more important/with more variants
to be towards the start of the string so we can run all tests of the
same kind with the -r option.

While also looking at different functions for figuring out the transport
and invocation, my observation was that, there might be many capabilities
added to the same test, while it might not be important also.
Ex: /migrate/multifd/tcp/plain
1. multifd is defined as a migration mode.
2. It is also a capability, and comes in 2 parts [multifd, multifd-channels]
     though one is a capability and another is parameter
Similarly in other examples of compression, there are many capabilities
and parameters added, but it might be not important to mention that ?

Secondly, there are multiple migration capabilities IIRC (> 15). And a test
requiring multiple capabilities, the overall string would be too long, and
not that important also to mention all capabilities.

Just thinking out of mind - Can we have selective list of capabilities ?
1. multifd 2. compress (again, there might be confusion with multifd
compression methods like zstd, zlib and just 'compress') 3. zero-page
(This will have sub capabilities ?)

I was thinking of keeping that part more open-ended. So not specifying
capabilities one by one, but more like "if you're testing a capability,
it comes here".

About multifd, it's a bit special since it cannot be seen as just a
"feature" anymore. It's a core part of the migration code. I wouldn't
classify it as capability for the purposes of the tests.

Ack, got it.

test-type:: migrate | validate

We could alternatively drop migration|migrate|validate. They are kind of
superfluous.

I agree with the above comment. 'migrate' and 'validate' have a different
set of variables required, some necessary, while other optional. IMO this
will help is in streamlining the design further.

migration-mode
a. migrate --> :: precopy | postcopy | multifd
b. validate -->:: (what to validate)
methods  :: preempt | recovery | reboot | suspend | simple

I want some inputs here.
1. is there a better variable name rather than 'methods'

Does this fall into the "mode" terminology that Steven introduced?

Yes, as we decided that we don't want 'migration-mode' key-value pair,
naming 'mode' would be a better term.

In cases, where multiple modes are to be used ex: postcopy_preempt_recovery
I feel it might be a good idea to separate multiple modes by '-'
For example - .../preempty-recovery/...
Similarly for other keys too if required

2. 'simple' does not fit perfect here IMO.

Can we go without it?

You mean omit the key itself in case of a no-op ?

transport:: tcp | fd | unix | file
invocation   :: uri | channels | both
CompressionType  :: zlib | zstd | none

s/none/nocomp/ ? We're already familiar with that.

Ack. Will change that.

encryptionType   :: tls | plain

s/plain/notls/ ?

What if there is another encryption technique in future ?

Or maybe we simply omit the noop options. It would make the string way
shorter in most cases.

This might be a better approach. Can have some keys/variables as optional
while some necessary. For ex: for 'migrate' - transport and invocation
might be necessary while it might not be necessary for 'validate' qtests

Yep

Ack, will do that!

validate-test-result :: success | failure
others   :: other comments/capability that needs to be
  addressed. Can be multiple

(more than one applicable, separated by using '-' in between)
O: optional

Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
   tests/qtest/migration-test.c | 143 ++-
   1 file changed, 72 insertions(+), 71 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index bd9f4b9dbb..bf4d000b76 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c

Regards,
Het Gala

I'm wondering whether we should leave the existing tests untouched and
require the new format only for new tests. Going through a git bisection
with a change in the middle that alters test names would be infuriating.

Hmm yup. I had this doubt on, how would we be enforcing the new design
for any new qtests that gets added from now on ?
Can we have this design started for validation tests maybe for now, the
number is low and might get some feedback to improve this ?

[PATCH] tests/vm: update openbsd image to 7.5

[Brad Smith]

tests/vm: update openbsd to release 7.5

Signed-off-by: Brad Smith 
---
This exposes a further issue with Clang 16 and
the ROP exploits flag usage at the moment..

https://gitlab.com/qemu-project/qemu/-/issues/2278

 tests/vm/openbsd | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/vm/openbsd b/tests/vm/openbsd
index 85c9863633..5e646f7c51 100755
--- a/tests/vm/openbsd
+++ b/tests/vm/openbsd
@@ -22,8 +22,8 @@ class OpenBSDVM(basevm.BaseVM):
 name = "openbsd"
 arch = "x86_64"
 
-link = "https://cdn.openbsd.org/pub/OpenBSD/7.4/amd64/install74.iso;
-csum = "a1001736ed9fe2307965b5fcdb426ae11f9b80d26eb21e404a705144a0a224a0"
+link = "https://cdn.openbsd.org/pub/OpenBSD/7.5/amd64/install75.iso;
+csum = "034435c6e27405d5a7fafb058162943c194eb793dafdc412c08d49bb56b3892a"
 size = "20G"
 pkgs = [
 # tools
@@ -124,7 +124,7 @@ class OpenBSDVM(basevm.BaseVM):
 self.console_wait_send("Allow root ssh login","yes\n")
 self.console_wait_send("timezone","UTC\n")
 self.console_wait_send("root disk",   "\n")
-self.console_wait_send("Encrypt the root disk with a passphrase", 
"no\n")
+self.console_wait_send("Encrypt the root disk with a (p)assphrase", 
"no\n")
 self.console_wait_send("(W)hole disk","\n")
 self.console_wait_send("(A)uto layout",   "c\n")
 
-- 
2.44.0

Re: secure boot & direct kernel load (was: Re: [PATCH] x86/loader: only patch linux kernels)

[Gerd Hoffmann]

On Wed, Apr 10, 2024 at 07:10:22AM -0400, Michael S. Tsirkin wrote:
> On Wed, Apr 10, 2024 at 12:35:13PM +0200, Gerd Hoffmann wrote:
> > On Wed, Apr 10, 2024 at 03:26:29AM -0400, Michael S. Tsirkin wrote:
> > > On Wed, Apr 10, 2024 at 09:21:26AM +0200, Gerd Hoffmann wrote:
> > > > If the binary loaded via -kernel is *not* a linux kernel (in which
> > > > case protocol == 0), do not patch the linux kernel header fields.
> > > > 
> > > > It's (a) pointless and (b) might break binaries by random patching
> > > > and (c) changes the binary hash which in turn breaks secure boot
> > > > verification.
> > > > 
> > > > Background: OVMF happily loads and runs not only linux kernels but
> > > > any efi binary via direct kernel boot.
> > > > 
> > > > Note: Breaking the secure boot verification is a problem for linux
> > > > kernels too, but fixed that is left for another day ...
> > > 
> > > Um we kind of care about Linux ;)
> > > 
> > > What's the plan?  I suspect we should just add a command line flag
> > > to skip patching? And once we do that, it seems safer to just
> > > always rely on the flag?
> > 
> > Well, there are more problems to solve here than just the patching.  So
> > lets have a look at the bigger picture before discussion the details ...
> > 
> > [ Cc'ing Daniel + Cole ]
> > 
> > Current state of affairs is that OVMF supports two ways to boot a linux
> > kernel:
> > 
> >  (1) Just load it as EFI binary and boot via linux kernel EFI stub,
> >  which is the modern way to load a linux kernel (which is why you
> >  can boot not only linux kernels but any efi binary).
> > 
> >  (2) Use the old EFI handover protocol.  Which is the RHEL-6 era way to
> >  boot a linux kernel on EFI.
> > 
> > For method (1) secure boot verification must pass.  For (2) not.  So if
> > you try to use direct kernel boot with secure boot enabled OVMF will
> > first try (1), which will fail, then go fallback to (2).
> > 
> > The reason for the failure is not only the patching, but also the fact
> > that the linux kernel is typically verified by shim.efi (and the distro
> > keys compiled into the binary) instead of the firmware.
> > 
> > Going though (2) is not ideal for multiple reasons, so we need some
> > strategy how we'll go handle direct kernel load with uefi and secure
> > boot in a way that (1) works.
> > 
> > Options I see:
> > 
> >   (a) Stop using direct kernel boot, let virt-install & other tools
> >   create vfat boot media with shim+kernel+initrd instead.
> > 
> >   (b) Enroll the distro signing keys in the efi variable store, so
> >   booting the kernel without shim.efi works.
> > 
> >   (c) Add support for loading shim to qemu (and ovmf), for example
> >   with a new '-shim' command line option which stores shim.efi
> >   in some new fw_cfg file.
> > 
> > (b) + (c) both require a fix for the patching issue.  The options
> > I see here are:
> > 
> >   (A) Move the patching from qemu to the linuxboot option rom.
> >   Strictly speaking it belongs there anyway.  It doesn't look
> >   that easy though, for qemu it is easier to gather all
> >   information needed ...
> > 
> >   (B) Provide both patched and unpatched setup header, so the
> >   guest can choose what it needs.
> > 
> >   (C) When implementing (c) above we can piggyback on the -shim
> >   switch and skip patching in case it is present.
> > 
> >   (D) Add a flag to skip the patching.
> > 
> > Comments?  Other/better ideas?
> > 
> > take care,
> >   Gerd
> 
> So if you didn't decide whether to do b or c then I guess D is
> easiest and covers both cases?

Easiest if you look at qemu only.  Adding a new config option adds
burdens elsewhere though.  Users and the management stack have to
learn to use the new option.

Both (A) and (B) work automatically and can be combined with both (b)
and (c).  (B) is probably much easier to implement, drawback is it
requires an firmware update too.

take care,
  Gerd

[PATCH 3/4] tests/qtest/migration: Add channels parameter in migrate_incoming_qmp

[Het Gala]

Alter migrate_incoming_qmp() to allow both uri and channels
independently. For channels, convert string to a QDict.

Signed-off-by: Het Gala 
---
 tests/qtest/migration-helpers.c   | 13 +++--
 tests/qtest/migration-helpers.h   |  4 ++--
 tests/qtest/migration-test.c  | 12 ++--
 tests/qtest/virtio-net-failover.c |  8 
 4 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 3b72cad6c1..cbd91719ae 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -245,7 +245,8 @@ void migrate_set_capability(QTestState *who, const char 
*capability,
  capability, value);
 }
 
-void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, 
...)
+void migrate_incoming_qmp(QTestState *to, const char *uri,
+  const char *channels, const char *fmt, ...)
 {
 va_list ap;
 QDict *args, *rsp, *data;
@@ -255,7 +256,15 @@ void migrate_incoming_qmp(QTestState *to, const char *uri, 
const char *fmt, ...)
 va_end(ap);
 
 g_assert(!qdict_haskey(args, "uri"));
-qdict_put_str(args, "uri", uri);
+if (uri) {
+qdict_put_str(args, "uri", uri);
+}
+
+g_assert(!qdict_haskey(args, "channels"));
+if (channels) {
+QObject *channels_obj = qobject_from_json(channels, _abort);
+qdict_put_obj(args, "channels", channels_obj);
+}
 
 migrate_set_capability(to, "events", true);
 
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 1339835698..9f74281aea 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -29,9 +29,9 @@ G_GNUC_PRINTF(5, 6)
 void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
  const char *channels, const char *fmt, ...);
 
-G_GNUC_PRINTF(3, 4)
+G_GNUC_PRINTF(4, 5)
 void migrate_incoming_qmp(QTestState *who, const char *uri,
-  const char *fmt, ...);
+  const char *channels, const char *fmt, ...);
 
 G_GNUC_PRINTF(4, 5)
 void migrate_qmp_fail(QTestState *who, const char *uri,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index f2c27d611c..fa8a860811 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1296,7 +1296,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
 migrate_ensure_non_converge(from);
 
 migrate_prepare_for_dirty_mem(from);
-migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
+migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
 
 /* Wait for the first serial output from the source */
 wait_for_serial("src_serial");
@@ -1824,7 +1824,7 @@ static void test_file_common(MigrateCommon *args, bool 
stop_src)
  * We need to wait for the source to finish before starting the
  * destination.
  */
-migrate_incoming_qmp(to, args->connect_uri, "{}");
+migrate_incoming_qmp(to, args->connect_uri, NULL, "{}");
 wait_for_migration_complete(to);
 
 if (stop_src) {
@@ -2405,7 +2405,7 @@ static void *test_migrate_fd_start_hook(QTestState *from,
 close(pair[0]);
 
 /* Start incoming migration from the 1st socket */
-migrate_incoming_qmp(to, "fd:fd-mig", "{}");
+migrate_incoming_qmp(to, "fd:fd-mig", NULL, "{}");
 
 /* Send the 2nd socket to the target */
 qtest_qmp_fds_assert_success(from, [1], 1,
@@ -2715,7 +2715,7 @@ test_migrate_precopy_tcp_multifd_start_common(QTestState 
*from,
 migrate_set_capability(to, "multifd", true);
 
 /* Start incoming migration from the 1st socket */
-migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
+migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
 
 return NULL;
 }
@@ -3040,7 +3040,7 @@ static void test_multifd_tcp_cancel(void)
 migrate_set_capability(to, "multifd", true);
 
 /* Start incoming migration from the 1st socket */
-migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
+migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
 
 /* Wait for the first serial output from the source */
 wait_for_serial("src_serial");
@@ -3068,7 +3068,7 @@ static void test_multifd_tcp_cancel(void)
 migrate_set_capability(to2, "multifd", true);
 
 /* Start incoming migration from the 1st socket */
-migrate_incoming_qmp(to2, "tcp:127.0.0.1:0", "{}");
+migrate_incoming_qmp(to2, "tcp:127.0.0.1:0", NULL, "{}");
 
 wait_for_migration_status(from, "cancelled", NULL);
 
diff --git a/tests/qtest/virtio-net-failover.c 
b/tests/qtest/virtio-net-failover.c
index 73dfabc272..e263ecd80e 100644
--- a/tests/qtest/virtio-net-failover.c
+++ b/tests/qtest/virtio-net-failover.c
@@ -772,7 +772,7 @@ static void test_migrate_in(gconstpointer opaque)
 check_one_card(qts, true, "standby0", MAC_STANDBY0);
 check_one_card(qts, false, "primary0", MAC_PRIMARY0);
 
-migrate_incoming_qmp(qts, uri, "{}");
+

Re: [QEMU][PATCH v3 5/7] memory: add MemoryRegion map and unmap callbacks

[Edgar E. Iglesias]

On Fri, Mar 1, 2024 at 12:11 AM Stefano Stabellini 
wrote:

> On Tue, 27 Feb 2024, Vikram Garhwal wrote:
> > From: Juergen Gross 
> >
> > In order to support mapping and unmapping guest memory dynamically to
> > and from qemu during address_space_[un]map() operations add the map()
> > and unmap() callbacks to MemoryRegionOps.
> >
> > Those will be used e.g. for Xen grant mappings when performing guest
> > I/Os.
> >
> > Signed-off-by: Juergen Gross 
> > Signed-off-by: Vikram Garhwal 
>
> Reviewed-by: Stefano Stabellini 
>

Reviewed-by: Edgar E. Iglesias 



> ---
> >  include/exec/memory.h | 21 ++
> >  system/physmem.c  | 50 +--
> >  2 files changed, 60 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 8626a355b3..9f7dfe59c7 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -282,6 +282,27 @@ struct MemoryRegionOps {
> >  unsigned size,
> >  MemTxAttrs attrs);
> >
> > +/*
> > + * Dynamically create mapping. @addr is the guest address to map;
> @plen
> > + * is the pointer to the usable length of the buffer.
> > + * @mr contents can be changed in case a new memory region is
> created for
> > + * the mapping.
> > + * Returns the buffer address for accessing the data.
> > + */
> > +void *(*map)(MemoryRegion **mr,
> > + hwaddr addr,
> > + hwaddr *plen,
> > + bool is_write,
> > + MemTxAttrs attrs);
> > +
> > +/* Unmap an area obtained via map() before. */
> > +void (*unmap)(MemoryRegion *mr,
> > +  void *buffer,
> > +  ram_addr_t addr,
> > +  hwaddr len,
> > +  bool is_write,
> > +  hwaddr access_len);
> > +
> >  enum device_endian endianness;
> >  /* Guest-visible constraints: */
> >  struct {
> > diff --git a/system/physmem.c b/system/physmem.c
> > index 949dcb20ba..d989e9fc1f 100644
> > --- a/system/physmem.c
> > +++ b/system/physmem.c
> > @@ -3141,6 +3141,7 @@ void *address_space_map(AddressSpace *as,
> >  hwaddr len = *plen;
> >  hwaddr l, xlat;
> >  MemoryRegion *mr;
> > +void *ptr = NULL;
> >  FlatView *fv;
> >
> >  if (len == 0) {
> > @@ -3174,12 +3175,20 @@ void *address_space_map(AddressSpace *as,
> >  return bounce.buffer;
> >  }
> >
> > -
> >  memory_region_ref(mr);
> > +
> > +if (mr->ops && mr->ops->map) {
> > +ptr = mr->ops->map(, addr, plen, is_write, attrs);
> > +}
> > +
> >  *plen = flatview_extend_translation(fv, addr, len, mr, xlat,
> >  l, is_write, attrs);
> >  fuzz_dma_read_cb(addr, *plen, mr);
> > -return qemu_ram_ptr_length(mr->ram_block, xlat, plen, true);
> > +if (ptr == NULL) {
> > +ptr = qemu_ram_ptr_length(mr->ram_block, xlat, plen, true);
> > +}
> > +
> > +return ptr;
> >  }
> >
> >  /* Unmaps a memory region previously mapped by address_space_map().
> > @@ -3195,11 +3204,16 @@ void address_space_unmap(AddressSpace *as, void
> *buffer, hwaddr len,
> >
> >  mr = memory_region_from_host(buffer, );
> >  assert(mr != NULL);
> > -if (is_write) {
> > -invalidate_and_set_dirty(mr, addr1, access_len);
> > -}
> > -if (xen_enabled()) {
> > -xen_invalidate_map_cache_entry(buffer);
> > +
> > +if (mr->ops && mr->ops->unmap) {
> > +mr->ops->unmap(mr, buffer, addr1, len, is_write,
> access_len);
> > +} else {
> > +if (is_write) {
> > +invalidate_and_set_dirty(mr, addr1, access_len);
> > +}
> > +if (xen_enabled()) {
> > +xen_invalidate_map_cache_entry(buffer);
> > +}
> >  }
> >  memory_region_unref(mr);
> >  return;
> > @@ -3272,10 +3286,18 @@ int64_t
> address_space_cache_init(MemoryRegionCache *cache,
> >   * doing this if we found actual RAM, which behaves the same
> >   * regardless of attributes; so UNSPECIFIED is fine.
> >   */
> > +if (mr->ops && mr->ops->map) {
> > +cache->ptr = mr->ops->map(, addr, , is_write,
> > +  MEMTXATTRS_UNSPECIFIED);
> > +}
> > +
> >  l = flatview_extend_translation(cache->fv, addr, len, mr,
> >  cache->xlat, l, is_write,
> >  MEMTXATTRS_UNSPECIFIED);
> > -cache->ptr = qemu_ram_ptr_length(mr->ram_block, cache->xlat,
> , true);
> > +if (!cache->ptr) {
> > +cache->ptr = qemu_ram_ptr_length(mr->ram_block,
> cache->xlat, ,
> > + true);
> > +}
> >  } else {
> >  cache->ptr = NULL;
> 

[PATCH 0/4] tests/qtest/migration: Add postcopy qtests for introducing 'channels' argument with new QAPI syntax

[Het Gala]

Add postcopy migration qtests with new QAPI syntax, having 'channels' as
the starting argument.
Also, introduce 'channels' to migrate_incoming_qmp function so as to
call migration with the new QAPI syntax from src as well as dest.

Patch 1:

Revert back commit which temporarily introduced 'migrate-incoming' QAPI
with the new 'channels' syntax.

Patch 2-3:
-
Introduce channels arg to migrate_incoming_qmp

Patch 4:
---
Introduce postcopy qtests with new QAPI syntax

Het Gala (4):
  Revert "migration: modify test_multifd_tcp_none() to use new QAPI
syntax"
  tests/qtest/migration: Replace 'migrate-incoming'
qtest_qmp_assert_success with migrate_incoming_qmp
  tests/qtest/migration: Add channels parameter in migrate_incoming_qmp
  tests/qtest/migration: Add postcopy migration qtests to use 'channels'
argument instead of uri

 tests/qtest/migration-helpers.c   | 13 ++--
 tests/qtest/migration-helpers.h   |  4 +--
 tests/qtest/migration-test.c  | 54 +++
 tests/qtest/virtio-net-failover.c |  8 ++---
 4 files changed, 58 insertions(+), 21 deletions(-)

-- 
2.22.3

[PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"

[Het Gala]

This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb

After addition of 'channels' as the starting argument of new QAPI
syntax inside postcopy test, even if the user entered the old QAPI
syntax, test used the new syntax.
It was a temporary patch added to have some presence of the new syntax
since the migration qtest framework lacked any logic for introducing
'channels' argument.

Now that the qtest framework has logic to introduce uri and channel
argument separately, we can remove this temporary change.

Signed-off-by: Het Gala 
---
 tests/qtest/migration-test.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 5d6d8cd634..27a1066ae4 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1297,12 +1297,7 @@ static int migrate_postcopy_prepare(QTestState 
**from_ptr,
 
 migrate_prepare_for_dirty_mem(from);
 qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
- "  'arguments': { "
- "  'channels': [ { 'channel-type': 'main',"
- "  'addr': { 'transport': 'socket',"
- "'type': 'inet',"
- "'host': '127.0.0.1',"
- "'port': '0' } } ] } }");
+ "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
 
 /* Wait for the first serial output from the source */
 wait_for_serial("src_serial");
-- 
2.22.3

[PATCH 2/4] tests/qtest/migration: Replace 'migrate-incoming' qtest_qmp_assert_success with migrate_incoming_qmp

[Het Gala]

Already have a migrate_incoming_qmp helper function to initiate
'migrate-incoming' QMP command with some additional checks.
Replace 'migrate-incoming' qtest_qmp_assert_success command with
calling migrate_incoming_qmp helper function for postcopy qtests.

Signed-off-by: Het Gala 
---
 tests/qtest/migration-test.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 27a1066ae4..f2c27d611c 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1296,8 +1296,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
 migrate_ensure_non_converge(from);
 
 migrate_prepare_for_dirty_mem(from);
-qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
- "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
+migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
 
 /* Wait for the first serial output from the source */
 wait_for_serial("src_serial");
-- 
2.22.3

[PATCH 4/4] tests/qtest/migration: Add postcopy migration qtests to use 'channels' argument instead of uri

[Het Gala]

Add qtests to perform postcopy live migration by having list of
'channels' argument as the starting point instead of uri string.
(Note: length of the list is restricted to 1 for now)

Signed-off-by: Het Gala 
---
 tests/qtest/migration-test.c | 38 ++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index fa8a860811..599018baa0 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1296,13 +1296,17 @@ static int migrate_postcopy_prepare(QTestState 
**from_ptr,
 migrate_ensure_non_converge(from);
 
 migrate_prepare_for_dirty_mem(from);
-migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
+if (args->connect_channels) {
+migrate_incoming_qmp(to, NULL, args->connect_channels, "{}");
+} else {
+migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
+}
 
 /* Wait for the first serial output from the source */
 wait_for_serial("src_serial");
 wait_for_suspend(from, _state);
 
-migrate_qmp(from, to, NULL, NULL, "{}");
+migrate_qmp(from, to, args->connect_uri, args->connect_channels, "{}");
 
 migrate_wait_for_dirty_mem(from, to);
 
@@ -1355,6 +1359,20 @@ static void test_postcopy(void)
 test_postcopy_common();
 }
 
+static void test_postcopy_channels(void)
+{
+MigrateCommon args = {
+.listen_uri = "defer",
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+"  'type': 'inet',"
+"  'host': '127.0.0.1',"
+"  'port': '0' } } ]",
+};
+
+test_postcopy_common();
+}
+
 static void test_postcopy_suspend(void)
 {
 MigrateCommon args = {
@@ -1555,6 +1573,18 @@ static void test_postcopy_recovery(void)
 test_postcopy_recovery_common();
 }
 
+static void test_postcopy_recovery_channels(void)
+{
+MigrateCommon args = {
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+"  'type': 'inet',"
+"  'host': '127.0.0.1',"
+"  'port': '0' } } ]",
+};
+
+test_postcopy_recovery_common();
+}
 static void test_postcopy_recovery_compress(void)
 {
 MigrateCommon args = {
@@ -3585,8 +3615,12 @@ int main(int argc, char **argv)
 
 if (has_uffd) {
 migration_test_add("/migration/postcopy/plain", test_postcopy);
+migration_test_add("/migration/postcopy/channels/plain",
+   test_postcopy_channels);
 migration_test_add("/migration/postcopy/recovery/plain",
test_postcopy_recovery);
+migration_test_add("/migration/postcopy/recovery/channels/plain",
+   test_postcopy_recovery_channels);
 migration_test_add("/migration/postcopy/preempt/plain",
test_postcopy_preempt);
 migration_test_add("/migration/postcopy/preempt/recovery/plain",
-- 
2.22.3

Re: [QEMU][PATCH v3 3/7] softmmu: let qemu_map_ram_ptr() use qemu_ram_ptr_length()

[Edgar E. Iglesias]

On Tue, Feb 27, 2024 at 11:37 PM Vikram Garhwal 
wrote:

> From: Juergen Gross 
>
> qemu_map_ram_ptr() and qemu_ram_ptr_length() share quite some code, so
> modify qemu_ram_ptr_length() a little bit and use it for
> qemu_map_ram_ptr(), too.
>
> Signed-off-by: Juergen Gross 
> Signed-off-by: Vikram Garhwal 
> Reviewed-by: Stefano Stabellini 
>

Reviewed-by: Edgar E. Iglesias 




> ---
>  system/physmem.c | 56 
>  1 file changed, 23 insertions(+), 33 deletions(-)
>
> diff --git a/system/physmem.c b/system/physmem.c
> index 84f3022099..949dcb20ba 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -2163,43 +2163,17 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t
> length)
>  }
>  #endif /* !_WIN32 */
>
> -/* Return a host pointer to ram allocated with qemu_ram_alloc.
> - * This should not be used for general purpose DMA.  Use address_space_map
> - * or address_space_rw instead. For local memory (e.g. video ram) that the
> - * device owns, use memory_region_get_ram_ptr.
> - *
> - * Called within RCU critical section.
> - */
> -void *qemu_map_ram_ptr(RAMBlock *block, ram_addr_t addr)
> -{
> -if (block == NULL) {
> -block = qemu_get_ram_block(addr);
> -addr -= block->offset;
> -}
> -
> -if (xen_enabled() && block->host == NULL) {
> -/* We need to check if the requested address is in the RAM
> - * because we don't want to map the entire memory in QEMU.
> - * In that case just map until the end of the page.
> - */
> -if (block->offset == 0) {
> -return xen_map_cache(addr, 0, 0, false);
> -}
> -
> -block->host = xen_map_cache(block->offset, block->max_length, 1,
> false);
> -}
> -return ramblock_ptr(block, addr);
> -}
> -
> -/* Return a host pointer to guest's ram. Similar to qemu_map_ram_ptr
> - * but takes a size argument.
> +/*
> + * Return a host pointer to guest's ram.
>   *
>   * Called within RCU critical section.
>   */
>  static void *qemu_ram_ptr_length(RAMBlock *block, ram_addr_t addr,
>   hwaddr *size, bool lock)
>  {
> -if (*size == 0) {
> +hwaddr len = 0;
> +
> +if (size && *size == 0) {
>  return NULL;
>  }
>
> @@ -2207,7 +2181,10 @@ static void *qemu_ram_ptr_length(RAMBlock *block,
> ram_addr_t addr,
>  block = qemu_get_ram_block(addr);
>  addr -= block->offset;
>  }
> -*size = MIN(*size, block->max_length - addr);
> +if (size) {
> +*size = MIN(*size, block->max_length - addr);
> +len = *size;
> +}
>
>  if (xen_enabled() && block->host == NULL) {
>  /* We need to check if the requested address is in the RAM
> @@ -2215,7 +2192,7 @@ static void *qemu_ram_ptr_length(RAMBlock *block,
> ram_addr_t addr,
>   * In that case just map the requested area.
>   */
>  if (block->offset == 0) {
> -return xen_map_cache(addr, *size, lock, lock);
> +return xen_map_cache(addr, len, lock, lock);
>  }
>
>  block->host = xen_map_cache(block->offset, block->max_length, 1,
> lock);
> @@ -2224,6 +2201,19 @@ static void *qemu_ram_ptr_length(RAMBlock *block,
> ram_addr_t addr,
>  return ramblock_ptr(block, addr);
>  }
>
> +/*
> + * Return a host pointer to ram allocated with qemu_ram_alloc.
> + * This should not be used for general purpose DMA.  Use address_space_map
> + * or address_space_rw instead. For local memory (e.g. video ram) that the
> + * device owns, use memory_region_get_ram_ptr.
> + *
> + * Called within RCU critical section.
> + */
> +void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
> +{
> +return qemu_ram_ptr_length(ram_block, addr, NULL, false);
> +}
> +
>  /* Return the offset of a hostpointer within a ramblock */
>  ram_addr_t qemu_ram_block_host_offset(RAMBlock *rb, void *host)
>  {
> --
> 2.17.1
>
>
>

Re: [QEMU][PATCH v3 4/7] xen: let xen_ram_addr_from_mapcache() return -1 in case of not found entry

[Edgar E. Iglesias]

On Fri, Mar 1, 2024 at 6:08 PM Alex Bennée  wrote:

> Vikram Garhwal  writes:
>
> > From: Juergen Gross 
> >
> > Today xen_ram_addr_from_mapcache() will either abort() or return 0 in
> > case it can't find a matching entry for a pointer value. Both cases
> > are bad, so change that to return an invalid address instead.
> >
> > Signed-off-by: Juergen Gross 
> > Reviewed-by: Stefano Stabellini 
> > ---
> >  hw/xen/xen-mapcache.c | 11 +++
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
> > index dfc412d138..179b7e95b2 100644
> > --- a/hw/xen/xen-mapcache.c
> > +++ b/hw/xen/xen-mapcache.c
> > @@ -396,13 +396,8 @@ ram_addr_t xen_ram_addr_from_mapcache(void *ptr)
> >  }
> >  }
> >  if (!found) {
> > -trace_xen_ram_addr_from_mapcache_not_found(ptr);
> > -QTAILQ_FOREACH(reventry, >locked_entries, next) {
> > -
> trace_xen_ram_addr_from_mapcache_found(reventry->paddr_index,
> > -   reventry->vaddr_req);
> > -}
>
> If these tracepoints aren't useful they need removing from trace-events.
> However I suspect it would be better to keep them in as they are fairly
> cheap.
>
> Otherwise:
>
> Reviewed-by: Alex Bennée 
>
>
Reviewed-by: Edgar E. Iglesias 

Re: [QEMU][PATCH v3 2/7] xen: add pseudo RAM region for grant mappings

[Edgar E. Iglesias]

On Fri, Mar 1, 2024 at 3:06 PM Alex Bennée  wrote:

> Vikram Garhwal  writes:
>
> > From: Juergen Gross 
> >
> > Add a memory region which can be used to automatically map granted
> > memory. It is starting at 0x8000ULL in order to be able to
> > distinguish it from normal RAM.
>
> Is the Xen memory map for HVM guests documented anywhere? I couldn't
> find anything googling or on the Xen wiki. I'm guessing this is going to
> be shared across all 64 bit HVM arches in Xen?
>
> Anyway:
>
> Reviewed-by: Alex Bennée 
>
>
Reviewed-by: Edgar E. Iglesias 

Re: [QEMU][PATCH v3 6/7] xen: add map and unmap callbacks for grant region

[Edgar E. Iglesias]

On Fri, Mar 1, 2024 at 12:34 AM Stefano Stabellini 
wrote:

> On Tue, 27 Feb 2024, Vikram Garhwal wrote:
> > From: Juergen Gross 
> >
> > Add the callbacks for mapping/unmapping guest memory via grants to the
> > special grant memory region.
> >
> > Signed-off-by: Juergen Gross 
> > Signed-off-by: Vikram Garhwal 
>
> Reviewed-by: Stefano Stabellini 
>
>
Reviewed-by: Edgar E. Iglesias 

Re: [QEMU][PATCH v3 1/7] softmmu: physmem: Split ram_block_add()

[Edgar E. Iglesias]

On Fri, Mar 1, 2024 at 12:35 PM Alex Bennée  wrote:

> Vikram Garhwal  writes:
>
> > Extract ram block list update to a new function ram_block_add_list().
> This is
> > done to support grant mappings which adds a memory region for granted
> memory and
> > updates the ram_block list.
> >
> > Signed-off-by: Juergen Gross 
> > Signed-off-by: Vikram Garhwal 
> > Reviewed-by: Stefano Stabellini 
>
> Reviewed-by: Alex Bennée 
>
>

Reviewed-by: Edgar E. Iglesias 

Re: [QEMU][PATCH v3 7/7] hw: arm: Add grant mapping.

[Edgar E. Iglesias]

On Wed, Mar 6, 2024 at 9:57 PM Vikram Garhwal 
wrote:

> Hi Alex,
> On Fri, Mar 01, 2024 at 05:10:28PM +, Alex Bennée wrote:
> > Vikram Garhwal  writes:
> >
> > > Enable grant ram mapping support for Xenpvh machine on ARM.
> > >
> > > Signed-off-by: Vikram Garhwal 
> > > Reviewed-by: Stefano Stabellini 
> > > ---
> > >  hw/arm/xen_arm.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
> > > index 32776d94df..b5993ef2a6 100644
> > > --- a/hw/arm/xen_arm.c
> > > +++ b/hw/arm/xen_arm.c
> > > @@ -125,6 +125,9 @@ static void xen_init_ram(MachineState *machine)
> > >   GUEST_RAM1_BASE, ram_size[1]);
> > >  memory_region_add_subregion(sysmem, GUEST_RAM1_BASE, _hi);
> > >  }
> > > +
> > > +DPRINTF("init grant ram mapping for XEN\n");
> >
> > I don't think we need the DPRINTF here (there others where recently
> > converted to trace-points although I suspect a memory_region tracepoint
> > would be a better place to capture this).
> May be drop the print? As it's not providing much information anyways.
>

With the DPRINTF dropped:
Reviewed-by: Edgar E. Iglesias 

Re: secure boot & direct kernel load (was: Re: [PATCH] x86/loader: only patch linux kernels)

[Michael S. Tsirkin]

On Wed, Apr 10, 2024 at 12:35:13PM +0200, Gerd Hoffmann wrote:
> On Wed, Apr 10, 2024 at 03:26:29AM -0400, Michael S. Tsirkin wrote:
> > On Wed, Apr 10, 2024 at 09:21:26AM +0200, Gerd Hoffmann wrote:
> > > If the binary loaded via -kernel is *not* a linux kernel (in which
> > > case protocol == 0), do not patch the linux kernel header fields.
> > > 
> > > It's (a) pointless and (b) might break binaries by random patching
> > > and (c) changes the binary hash which in turn breaks secure boot
> > > verification.
> > > 
> > > Background: OVMF happily loads and runs not only linux kernels but
> > > any efi binary via direct kernel boot.
> > > 
> > > Note: Breaking the secure boot verification is a problem for linux
> > > kernels too, but fixed that is left for another day ...
> > 
> > Um we kind of care about Linux ;)
> > 
> > What's the plan?  I suspect we should just add a command line flag
> > to skip patching? And once we do that, it seems safer to just
> > always rely on the flag?
> 
> Well, there are more problems to solve here than just the patching.  So
> lets have a look at the bigger picture before discussion the details ...
> 
> [ Cc'ing Daniel + Cole ]
> 
> Current state of affairs is that OVMF supports two ways to boot a linux
> kernel:
> 
>  (1) Just load it as EFI binary and boot via linux kernel EFI stub,
>  which is the modern way to load a linux kernel (which is why you
>  can boot not only linux kernels but any efi binary).
> 
>  (2) Use the old EFI handover protocol.  Which is the RHEL-6 era way to
>  boot a linux kernel on EFI.
> 
> For method (1) secure boot verification must pass.  For (2) not.  So if
> you try to use direct kernel boot with secure boot enabled OVMF will
> first try (1), which will fail, then go fallback to (2).
> 
> The reason for the failure is not only the patching, but also the fact
> that the linux kernel is typically verified by shim.efi (and the distro
> keys compiled into the binary) instead of the firmware.
> 
> Going though (2) is not ideal for multiple reasons, so we need some
> strategy how we'll go handle direct kernel load with uefi and secure
> boot in a way that (1) works.
> 
> Options I see:
> 
>   (a) Stop using direct kernel boot, let virt-install & other tools
>   create vfat boot media with shim+kernel+initrd instead.
> 
>   (b) Enroll the distro signing keys in the efi variable store, so
>   booting the kernel without shim.efi works.
> 
>   (c) Add support for loading shim to qemu (and ovmf), for example
>   with a new '-shim' command line option which stores shim.efi
>   in some new fw_cfg file.
> 
> (b) + (c) both require a fix for the patching issue.  The options
> I see here are:
> 
>   (A) Move the patching from qemu to the linuxboot option rom.
>   Strictly speaking it belongs there anyway.  It doesn't look
>   that easy though, for qemu it is easier to gather all
>   information needed ...
> 
>   (B) Provide both patched and unpatched setup header, so the
>   guest can choose what it needs.
> 
>   (C) When implementing (c) above we can piggyback on the -shim
>   switch and skip patching in case it is present.
> 
>   (D) Add a flag to skip the patching.
> 
> Comments?  Other/better ideas?
> 
> take care,
>   Gerd

So if you didn't decide whether to do b or c then I guess D is
easiest and covers both cases?

-- 
MST

Re: [PATCH for-9.0] ppc440_pcix: Do not expose a bridge device on PCI bus

[BALATON Zoltan]

On Wed, 10 Apr 2024, Nicholas Piggin wrote:

On Wed Apr 10, 2024 at 9:55 AM AEST, BALATON Zoltan wrote:

Real 460EX SoC apparently does not expose a bridge device and having
it appear on PCI bus confuses an AmigaOS file system driver that uses
this to detect which machine it is running on. Since values written
here by firmware are never read, just ignore these writes and drop the
bridge device.

Signed-off-by: BALATON Zoltan 
---
This is only used by sam460ex and this fixes an issue with AmigaOS on
this machine so I'd like this to be merged for 9.0 please.


Is it a regression? Does it have a fixes: or resolves: tag?

Unless we broke it in this cycle, I would be inclined to wait,
and we can ask to put it in stable.


It's not something that broke in this cycle but since this does not affect 
anything else than sam460ex I think it's OK to change this for 9.0. The 
changes to 440 tlb in this cycle made sam460ex more useful to run AmigaOS 
and this fixes the file system driver on it so it would make 9.0 really 
usable. Otherwise people would have to wait longer until August or install 
a stable update. Since this has low chance to break anything (tested with 
AmogaOS and Linux and MorphOS does not boot due to do_io changes anyway) I 
don't think we have to wait with this.


Regards,
BALATON Zoltan


Thanks,
Nick



 hw/pci-host/ppc440_pcix.c | 14 +-
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/hw/pci-host/ppc440_pcix.c b/hw/pci-host/ppc440_pcix.c
index 1926ae2a27..ba38172989 100644
--- a/hw/pci-host/ppc440_pcix.c
+++ b/hw/pci-host/ppc440_pcix.c
@@ -52,7 +52,6 @@ OBJECT_DECLARE_SIMPLE_TYPE(PPC440PCIXState, PPC440_PCIX_HOST)
 struct PPC440PCIXState {
 PCIHostState parent_obj;

-PCIDevice *dev;
 struct PLBOutMap pom[PPC440_PCIX_NR_POMS];
 struct PLBInMap pim[PPC440_PCIX_NR_PIMS];
 uint32_t sts;
@@ -170,10 +169,6 @@ static void ppc440_pcix_reg_write4(void *opaque, hwaddr 
addr,

 trace_ppc440_pcix_reg_write(addr, val, size);
 switch (addr) {
-case PCI_VENDOR_ID ... PCI_MAX_LAT:
-stl_le_p(s->dev->config + addr, val);
-break;
-
 case PCIX0_POM0LAL:
 s->pom[0].la &= 0xULL;
 s->pom[0].la |= val;
@@ -301,10 +296,6 @@ static uint64_t ppc440_pcix_reg_read4(void *opaque, hwaddr 
addr,
 uint32_t val;

 switch (addr) {
-case PCI_VENDOR_ID ... PCI_MAX_LAT:
-val = ldl_le_p(s->dev->config + addr);
-break;
-
 case PCIX0_POM0LAL:
 val = s->pom[0].la;
 break;
@@ -498,10 +489,7 @@ static void ppc440_pcix_realize(DeviceState *dev, Error 
**errp)
 memory_region_init(>iomem, OBJECT(dev), "pci-io", 64 * KiB);
 h->bus = pci_register_root_bus(dev, NULL, ppc440_pcix_set_irq,
  ppc440_pcix_map_irq, >irq, >busmem, >iomem,
- PCI_DEVFN(0, 0), 1, TYPE_PCI_BUS);
-
-s->dev = pci_create_simple(h->bus, PCI_DEVFN(0, 0),
-   TYPE_PPC4xx_HOST_BRIDGE);
+ PCI_DEVFN(1, 0), 1, TYPE_PCI_BUS);

 memory_region_init(>bm, OBJECT(s), "bm-ppc440-pcix", UINT64_MAX);
 memory_region_add_subregion(>bm, 0x0, >busmem);

secure boot & direct kernel load (was: Re: [PATCH] x86/loader: only patch linux kernels)

[Gerd Hoffmann]

On Wed, Apr 10, 2024 at 03:26:29AM -0400, Michael S. Tsirkin wrote:
> On Wed, Apr 10, 2024 at 09:21:26AM +0200, Gerd Hoffmann wrote:
> > If the binary loaded via -kernel is *not* a linux kernel (in which
> > case protocol == 0), do not patch the linux kernel header fields.
> > 
> > It's (a) pointless and (b) might break binaries by random patching
> > and (c) changes the binary hash which in turn breaks secure boot
> > verification.
> > 
> > Background: OVMF happily loads and runs not only linux kernels but
> > any efi binary via direct kernel boot.
> > 
> > Note: Breaking the secure boot verification is a problem for linux
> > kernels too, but fixed that is left for another day ...
> 
> Um we kind of care about Linux ;)
> 
> What's the plan?  I suspect we should just add a command line flag
> to skip patching? And once we do that, it seems safer to just
> always rely on the flag?

Well, there are more problems to solve here than just the patching.  So
lets have a look at the bigger picture before discussion the details ...

[ Cc'ing Daniel + Cole ]

Current state of affairs is that OVMF supports two ways to boot a linux
kernel:

 (1) Just load it as EFI binary and boot via linux kernel EFI stub,
 which is the modern way to load a linux kernel (which is why you
 can boot not only linux kernels but any efi binary).

 (2) Use the old EFI handover protocol.  Which is the RHEL-6 era way to
 boot a linux kernel on EFI.

For method (1) secure boot verification must pass.  For (2) not.  So if
you try to use direct kernel boot with secure boot enabled OVMF will
first try (1), which will fail, then go fallback to (2).

The reason for the failure is not only the patching, but also the fact
that the linux kernel is typically verified by shim.efi (and the distro
keys compiled into the binary) instead of the firmware.

Going though (2) is not ideal for multiple reasons, so we need some
strategy how we'll go handle direct kernel load with uefi and secure
boot in a way that (1) works.

Options I see:

  (a) Stop using direct kernel boot, let virt-install & other tools
  create vfat boot media with shim+kernel+initrd instead.

  (b) Enroll the distro signing keys in the efi variable store, so
  booting the kernel without shim.efi works.

  (c) Add support for loading shim to qemu (and ovmf), for example
  with a new '-shim' command line option which stores shim.efi
  in some new fw_cfg file.

(b) + (c) both require a fix for the patching issue.  The options
I see here are:

  (A) Move the patching from qemu to the linuxboot option rom.
  Strictly speaking it belongs there anyway.  It doesn't look
  that easy though, for qemu it is easier to gather all
  information needed ...

  (B) Provide both patched and unpatched setup header, so the
  guest can choose what it needs.

  (C) When implementing (c) above we can piggyback on the -shim
  switch and skip patching in case it is present.

  (D) Add a flag to skip the patching.

Comments?  Other/better ideas?

take care,
  Gerd

[RFC 0/2] Identify aliased maps in vdpa SVQ iova_tree

[Eugenio Pérez]

The guest may have overlapped memory regions, where different GPA leads
to the same HVA.  This causes a problem when overlapped regions
(different GPA but same translated HVA) exists in the tree, as looking
them by HVA will return them twice.

To solve this, track GPA in the DMA entry that acs as unique identifiers
to the maps.  When the map needs to be removed, iova tree is able to
find the right one.

Users that does not go to this extra layer of indirection can use the
iova tree as usual, with id = 0.

This was found by Si-Wei Liu , but I'm having a hard
time to reproduce the issue.  This has been tested only without overlapping
maps.  If it works with overlapping maps, it will be intergrated in the main
series.

Comments are welcome.  Thanks!

Eugenio Pérez (2):
  iova_tree: add an id member to DMAMap
  vdpa: identify aliased maps in iova_tree

 hw/virtio/vhost-vdpa.c   | 2 ++
 include/qemu/iova-tree.h | 5 +++--
 util/iova-tree.c | 3 ++-
 3 files changed, 7 insertions(+), 3 deletions(-)

-- 
2.44.0

[RFC 2/2] vdpa: identify aliased maps in iova_tree

[Eugenio Pérez]

The guest may have overlapped memory regions, where different GPA leads
to the same HVA.  This causes a problem when overlapped regions
(different GPA but same translated HVA) exists in the tree, as looking
them by HVA will return them twice.

To solve this, track GPA in the DMA entry that acs as unique identifiers
to the maps.  When the map needs to be removed, iova tree is able to
find the right one.

Users that does not go to this extra layer of indirection can use the
iova tree as usual, with id = 0.

Signed-off-by: Eugenio Pérez 
---
 hw/virtio/vhost-vdpa.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index e827b9175f..90adff597c 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -361,6 +361,7 @@ static void vhost_vdpa_listener_region_add(MemoryListener 
*listener,
 mem_region.translated_addr = (hwaddr)(uintptr_t)vaddr,
 mem_region.size = int128_get64(llsize) - 1,
 mem_region.perm = IOMMU_ACCESS_FLAG(true, section->readonly),
+mem_region.id = iova;
 
 r = vhost_iova_tree_map_alloc(s->iova_tree, _region);
 if (unlikely(r != IOVA_OK)) {
@@ -443,6 +444,7 @@ static void vhost_vdpa_listener_region_del(MemoryListener 
*listener,
 DMAMap mem_region = {
 .translated_addr = (hwaddr)(uintptr_t)vaddr,
 .size = int128_get64(llsize) - 1,
+.id = iova,
 };
 
 result = vhost_iova_tree_find_iova(s->iova_tree, _region);
-- 
2.44.0

[RFC 1/2] iova_tree: add an id member to DMAMap

[Eugenio Pérez]

IOVA tree is also used to track the mappings of virtio-net shadow
virtqueue.  This mappings may not match with the GPA->HVA ones.

This causes a problem when overlapped regions (different GPA but same
translated HVA) exists in the tree, as looking them by HVA will return
them twice.  To solve this, create an id member so we can assign unique
identifiers (GPA) to the maps.

Signed-off-by: Eugenio Pérez 
---
 include/qemu/iova-tree.h | 5 +++--
 util/iova-tree.c | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/qemu/iova-tree.h b/include/qemu/iova-tree.h
index 2a10a7052e..34ee230e7d 100644
--- a/include/qemu/iova-tree.h
+++ b/include/qemu/iova-tree.h
@@ -36,6 +36,7 @@ typedef struct DMAMap {
 hwaddr iova;
 hwaddr translated_addr;
 hwaddr size;/* Inclusive */
+uint64_t id;
 IOMMUAccessFlags perm;
 } QEMU_PACKED DMAMap;
 typedef gboolean (*iova_tree_iterator)(DMAMap *map);
@@ -100,8 +101,8 @@ const DMAMap *iova_tree_find(const IOVATree *tree, const 
DMAMap *map);
  * @map: the mapping to search
  *
  * Search for a mapping in the iova tree that translated_addr overlaps with the
- * mapping range specified.  Only the first found mapping will be
- * returned.
+ * mapping range specified and map->id is equal.  Only the first found
+ * mapping will be returned.
  *
  * Return: DMAMap pointer if found, or NULL if not found.  Note that
  * the returned DMAMap pointer is maintained internally.  User should
diff --git a/util/iova-tree.c b/util/iova-tree.c
index 536789797e..0863e0a3b8 100644
--- a/util/iova-tree.c
+++ b/util/iova-tree.c
@@ -97,7 +97,8 @@ static gboolean iova_tree_find_address_iterator(gpointer key, 
gpointer value,
 
 needle = args->needle;
 if (map->translated_addr + map->size < needle->translated_addr ||
-needle->translated_addr + needle->size < map->translated_addr) {
+needle->translated_addr + needle->size < map->translated_addr ||
+needle->id != map->id) {
 return false;
 }
 
-- 
2.44.0

[PULL 13/16] hw/net/lan9118: Fix overflow in MIL TX FIFO

[Philippe Mathieu-Daudé]

When the MAC Interface Layer (MIL) transmit FIFO is full,
truncate the packet, and raise the Transmitter Error (TXE)
flag.

Broken since model introduction in commit 2a42499017
("LAN9118 emulation").

When using the reproducer from
https://gitlab.com/qemu-project/qemu/-/issues/2267 we get:

  hw/net/lan9118.c:798:17: runtime error:
  index 2048 out of bounds for type 'uint8_t[2048]' (aka 'unsigned char[2048]')
    #0 0x563ec9a057b1 in tx_fifo_push hw/net/lan9118.c:798:43
    #1 0x563ec99fbb28 in lan9118_writel hw/net/lan9118.c:1042:9
    #2 0x563ec99f2de2 in lan9118_16bit_mode_write hw/net/lan9118.c:1205:9
    #3 0x563ecbf78013 in memory_region_write_accessor system/memory.c:497:5
    #4 0x563ecbf776f5 in access_with_adjusted_size system/memory.c:573:18
    #5 0x563ecbf75643 in memory_region_dispatch_write system/memory.c:1521:16
    #6 0x563ecc01bade in flatview_write_continue_step system/physmem.c:2713:18
    #7 0x563ecc01b374 in flatview_write_continue system/physmem.c:2743:19
    #8 0x563ecbff1c9b in flatview_write system/physmem.c:2774:12
    #9 0x563ecbff1768 in address_space_write system/physmem.c:2894:18
...

[*] LAN9118 DS2266B.pdf, Table 5.3.3 "INTERRUPT STATUS REGISTER"

Cc: qemu-sta...@nongnu.org
Reported-by: Will Lester
Reported-by: Chuhong Yuan 
Suggested-by: Peter Maydell 
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2267
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Peter Maydell 
Message-Id: <20240409133801.23503-3-phi...@linaro.org>
---
 hw/net/lan9118.c | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index 8214569a2c..91d81b410b 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -799,8 +799,22 @@ static void tx_fifo_push(lan9118_state *s, uint32_t val)
 /* Documentation is somewhat unclear on the ordering of bytes
in FIFO words.  Empirical results show it to be little-endian.
*/
-/* TODO: FIFO overflow checking.  */
 while (n--) {
+if (s->txp->len == MIL_TXFIFO_SIZE) {
+/*
+ * No more space in the FIFO. The datasheet is not
+ * precise about this case. We choose what is easiest
+ * to model: the packet is truncated, and TXE is raised.
+ *
+ * Note, it could be a fragmented packet, but we currently
+ * do not handle that (see earlier TX_B case).
+ */
+qemu_log_mask(LOG_GUEST_ERROR,
+  "MIL TX FIFO overrun, discarding %u 
byte%s\n",
+  n, n > 1 ? "s" : "");
+s->int_sts |= TXE_INT;
+break;
+}
 s->txp->data[s->txp->len] = val & 0xff;
 s->txp->len++;
 val >>= 8;
-- 
2.41.0

[PULL 01/16] hw/virtio: Introduce virtio_bh_new_guarded() helper

[Philippe Mathieu-Daudé]

Introduce virtio_bh_new_guarded(), similar to qemu_bh_new_guarded()
but using the transport memory guard, instead of the device one
(there can only be one virtio device per virtio bus).

Inspired-by: Gerd Hoffmann 
Reviewed-by: Gerd Hoffmann 
Acked-by: Michael S. Tsirkin 
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Michael S. Tsirkin 
Message-Id: <20240409105537.18308-2-phi...@linaro.org>
---
 include/hw/virtio/virtio.h |  7 +++
 hw/virtio/virtio.c | 10 ++
 2 files changed, 17 insertions(+)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index c8f72850bc..7d5ffdc145 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -22,6 +22,7 @@
 #include "standard-headers/linux/virtio_config.h"
 #include "standard-headers/linux/virtio_ring.h"
 #include "qom/object.h"
+#include "block/aio.h"
 
 /*
  * A guest should never accept this. It implies negotiation is broken
@@ -508,4 +509,10 @@ static inline bool virtio_device_disabled(VirtIODevice 
*vdev)
 bool virtio_legacy_allowed(VirtIODevice *vdev);
 bool virtio_legacy_check_disabled(VirtIODevice *vdev);
 
+QEMUBH *virtio_bh_new_guarded_full(DeviceState *dev,
+   QEMUBHFunc *cb, void *opaque,
+   const char *name);
+#define virtio_bh_new_guarded(dev, cb, opaque) \
+virtio_bh_new_guarded_full((dev), (cb), (opaque), (stringify(cb)))
+
 #endif
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index c5bedca848..871674f9be 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -4145,3 +4145,13 @@ static void virtio_register_types(void)
 }
 
 type_init(virtio_register_types)
+
+QEMUBH *virtio_bh_new_guarded_full(DeviceState *dev,
+   QEMUBHFunc *cb, void *opaque,
+   const char *name)
+{
+DeviceState *transport = qdev_get_parent_bus(dev)->parent;
+
+return qemu_bh_new_full(cb, opaque, name,
+>mem_reentrancy_guard);
+}
-- 
2.41.0

[PULL 08/16] hw/block/nand: Fix out-of-bound access in NAND block buffer

[Philippe Mathieu-Daudé]

nand_command() and nand_getio() don't check @offset points
into the block, nor the available data length (s->iolen) is
not negative.

In order to fix:

- check the offset is in range in nand_blk_load_NAND_PAGE_SIZE(),
- do not set @iolen if blk_load() failed.

Reproducer:

  $ cat << EOF | qemu-system-arm -machine tosa \
 -monitor none -serial none \
 -display none -qtest stdio
  write 0x1111 0x1 0xca
  write 0x1104 0x1 0x47
  write 0x1000ca04 0x1 0xd7
  write 0x1000ca01 0x1 0xe0
  write 0x1000ca04 0x1 0x71
  write 0x1000ca00 0x1 0x50
  write 0x1000ca04 0x1 0xd7
  read 0x1000ca02 0x1
  write 0x1000ca01 0x1 0x10
  EOF

=
==15750==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61f00de0
 at pc 0x560e61557210 bp 0x7ffcfc4a59f0 sp 0x7ffcfc4a59e8
READ of size 1 at 0x61f00de0 thread T0
#0 0x560e6155720f in mem_and hw/block/nand.c:101:20
#1 0x560e6155ac9c in nand_blk_write_512 hw/block/nand.c:663:9
#2 0x560e61544200 in nand_command hw/block/nand.c:293:13
#3 0x560e6153cc83 in nand_setio hw/block/nand.c:520:13
#4 0x560e61a0a69e in tc6393xb_nand_writeb hw/display/tc6393xb.c:380:13
#5 0x560e619f9bf7 in tc6393xb_writeb hw/display/tc6393xb.c:524:9
#6 0x560e647c7d03 in memory_region_write_accessor softmmu/memory.c:492:5
#7 0x560e647c7641 in access_with_adjusted_size softmmu/memory.c:554:18
#8 0x560e647c5f66 in memory_region_dispatch_write softmmu/memory.c:1514:16
#9 0x560e6485409e in flatview_write_continue softmmu/physmem.c:2825:23
#10 0x560e648421eb in flatview_write softmmu/physmem.c:2867:12
#11 0x560e64841ca8 in address_space_write softmmu/physmem.c:2963:18
#12 0x560e61170162 in qemu_writeb tests/qtest/videzzo/videzzo_qemu.c:1080:5
#13 0x560e6116eef7 in dispatch_mmio_write 
tests/qtest/videzzo/videzzo_qemu.c:1227:28

0x61f00de0 is located 0 bytes to the right of 3424-byte region 
[0x61f00080,0x61f00de0)
allocated by thread T0 here:
#0 0x560e611276cf in malloc 
/root/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
#1 0x7f7959a87e98 in g_malloc 
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57e98)
#2 0x560e64b98871 in object_new qom/object.c:749:12
#3 0x560e64b5d1a1 in qdev_new hw/core/qdev.c:153:19
#4 0x560e61547ea5 in nand_init hw/block/nand.c:639:11
#5 0x560e619f8772 in tc6393xb_init hw/display/tc6393xb.c:558:16
#6 0x560e6390bad2 in tosa_init hw/arm/tosa.c:250:12

SUMMARY: AddressSanitizer: heap-buffer-overflow hw/block/nand.c:101:20 in 
mem_and
==15750==ABORTING

Broken since introduction in commit 3e3d5815cb ("NAND Flash memory
emulation and ECC calculation helpers for use by NAND controllers").

Cc: qemu-sta...@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1445
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1446
Reported-by: Qiang Liu 
Reviewed-by: Richard Henderson 
Reviewed-by: Kevin Wolf 
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20240409135944.24997-4-phi...@linaro.org>
---
 hw/block/nand.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index 5a31d78b6b..e2433c25bd 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -255,7 +255,9 @@ static unsigned nand_load_block(NANDFlashState *s, unsigned 
offset)
 {
 unsigned iolen;
 
-s->blk_load(s, s->addr, offset);
+if (!s->blk_load(s, s->addr, offset)) {
+return 0;
+}
 
 iolen = (1 << s->page_shift);
 if (s->gnd) {
@@ -783,6 +785,10 @@ static bool glue(nand_blk_load_, 
NAND_PAGE_SIZE)(NANDFlashState *s,
 return false;
 }
 
+if (offset > NAND_PAGE_SIZE + OOB_SIZE) {
+return false;
+}
+
 if (s->blk) {
 if (s->mem_oob) {
 if (blk_pread(s->blk, SECTOR(addr) << BDRV_SECTOR_BITS,
-- 
2.41.0

[PULL 02/16] hw/display/virtio-gpu: Protect from DMA re-entrancy bugs

[Philippe Mathieu-Daudé]

Replace qemu_bh_new_guarded() by virtio_bh_new_guarded()
so the bus and device use the same guard. Otherwise the
DMA-reentrancy protection can be bypassed:

  $ cat << EOF | qemu-system-i386 -display none -nodefaults \
  -machine q35,accel=qtest \
  -m 512M \
  -device virtio-gpu \
  -qtest stdio
  outl 0xcf8 0x8820
  outl 0xcfc 0xe0004000
  outl 0xcf8 0x8804
  outw 0xcfc 0x06
  write 0xe0004030 0x4 0x024000e0
  write 0xe0004028 0x1 0xff
  write 0xe0004020 0x4 0x9300
  write 0xe000401c 0x1 0x01
  write 0x101 0x1 0x04
  write 0x103 0x1 0x1c
  write 0x9301c8 0x1 0x18
  write 0x105 0x1 0x1c
  write 0x107 0x1 0x1c
  write 0x109 0x1 0x1c
  write 0x10b 0x1 0x00
  write 0x10d 0x1 0x00
  write 0x10f 0x1 0x00
  write 0x111 0x1 0x00
  write 0x113 0x1 0x00
  write 0x115 0x1 0x00
  write 0x117 0x1 0x00
  write 0x119 0x1 0x00
  write 0x11b 0x1 0x00
  write 0x11d 0x1 0x00
  write 0x11f 0x1 0x00
  write 0x121 0x1 0x00
  write 0x123 0x1 0x00
  write 0x125 0x1 0x00
  write 0x127 0x1 0x00
  write 0x129 0x1 0x00
  write 0x12b 0x1 0x00
  write 0x12d 0x1 0x00
  write 0x12f 0x1 0x00
  write 0x131 0x1 0x00
  write 0x133 0x1 0x00
  write 0x135 0x1 0x00
  write 0x137 0x1 0x00
  write 0x139 0x1 0x00
  write 0xe0007003 0x1 0x00
  EOF
  ...
  =
  ==276099==ERROR: AddressSanitizer: heap-use-after-free on address 
0x60d11178
  at pc 0x562cc3b736c7 bp 0x7ffed49dee60 sp 0x7ffed49dee58
  READ of size 8 at 0x60d11178 thread T0
  #0 0x562cc3b736c6 in virtio_gpu_ctrl_response 
hw/display/virtio-gpu.c:180:42
  #1 0x562cc3b7c40b in virtio_gpu_ctrl_response_nodata 
hw/display/virtio-gpu.c:192:5
  #2 0x562cc3b7c40b in virtio_gpu_simple_process_cmd 
hw/display/virtio-gpu.c:1015:13
  #3 0x562cc3b82873 in virtio_gpu_process_cmdq 
hw/display/virtio-gpu.c:1050:9
  #4 0x562cc4a85514 in aio_bh_call util/async.c:169:5
  #5 0x562cc4a85c52 in aio_bh_poll util/async.c:216:13
  #6 0x562cc4a1a79b in aio_dispatch util/aio-posix.c:423:5
  #7 0x562cc4a8a2da in aio_ctx_dispatch util/async.c:358:5
  #8 0x7f36840547a8 in g_main_context_dispatch 
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x547a8)
  #9 0x562cc4a8b753 in glib_pollfds_poll util/main-loop.c:290:9
  #10 0x562cc4a8b753 in os_host_main_loop_wait util/main-loop.c:313:5
  #11 0x562cc4a8b753 in main_loop_wait util/main-loop.c:592:11
  #12 0x562cc3938186 in qemu_main_loop system/runstate.c:782:9
  #13 0x562cc43b7af5 in qemu_default_main system/main.c:37:14
  #14 0x7f3683a6c189 in __libc_start_call_main 
csu/../sysdeps/nptl/libc_start_call_main.h:58:16
  #15 0x7f3683a6c244 in __libc_start_main csu/../csu/libc-start.c:381:3
  #16 0x562cc2a58ac0 in _start (qemu-system-i386+0x231bac0)

  0x60d11178 is located 56 bytes inside of 136-byte region 
[0x60d11140,0x60d111c8)
  freed by thread T0 here:
  #0 0x562cc2adb662 in __interceptor_free (qemu-system-i386+0x239e662)
  #1 0x562cc3b86b21 in virtio_gpu_reset hw/display/virtio-gpu.c:1524:9
  #2 0x562cc416e20e in virtio_reset hw/virtio/virtio.c:2145:9
  #3 0x562cc37c5644 in virtio_pci_reset hw/virtio/virtio-pci.c:2249:5
  #4 0x562cc4233758 in memory_region_write_accessor system/memory.c:497:5
  #5 0x562cc4232eea in access_with_adjusted_size system/memory.c:573:18

  previously allocated by thread T0 here:
  #0 0x562cc2adb90e in malloc (qemu-system-i386+0x239e90e)
  #1 0x7f368405a678 in g_malloc 
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x5a678)
  #2 0x562cc4163ffc in virtqueue_split_pop hw/virtio/virtio.c:1612:12
  #3 0x562cc4163ffc in virtqueue_pop hw/virtio/virtio.c:1783:16
  #4 0x562cc3b91a95 in virtio_gpu_handle_ctrl 
hw/display/virtio-gpu.c:1112:15
  #5 0x562cc4a85514 in aio_bh_call util/async.c:169:5
  #6 0x562cc4a85c52 in aio_bh_poll util/async.c:216:13
  #7 0x562cc4a1a79b in aio_dispatch util/aio-posix.c:423:5

  SUMMARY: AddressSanitizer: heap-use-after-free hw/display/virtio-gpu.c:180:42 
in virtio_gpu_ctrl_response

With this change, the same reproducer triggers:

  qemu-system-i386: warning: Blocked re-entrant IO on MemoryRegion: 
virtio-pci-common-virtio-gpu at addr: 0x6

Fixes: CVE-2024-3446
Cc: qemu-sta...@nongnu.org
Reported-by: Alexander Bulekov 
Reported-by: Yongkang Jia 
Reported-by: Xiao Lei 
Reported-by: Yiming Tao 
Buglink: https://bugs.launchpad.net/qemu/+bug/1888606
Reviewed-by: Gerd Hoffmann 
Acked-by: Michael S. Tsirkin 
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Michael S. Tsirkin 
Message-Id: <20240409105537.18308-3-phi...@linaro.org>
---
 hw/display/virtio-gpu.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 78d5a4f164..ae831b6b3e 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1492,10 

[PULL 16/16] hw/audio/virtio-snd: Remove unused assignment

[Philippe Mathieu-Daudé]

Coverity reported:

  >>> CID 1542933:  Code maintainability issues  (UNUSED_VALUE)
  >>> CID 1542934:  Code maintainability issues  (UNUSED_VALUE)
  >>> Assigning value "NULL" to "stream" here, but that stored
  value is overwritten before it can be used.

Simply remove the unused assignments.

Resolves: Coverity CID 1542933
Resolves: Coverity CID 1542934
Fixes: 731655f87f ("virtio-snd: rewrite invalid tx/rx message handling")
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Manos Pitsidianakis 
Message-Id: <20240410053712.34747-1-phi...@linaro.org>
---
 hw/audio/virtio-snd.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index 90d9a2796e..c80b58bf5d 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -885,7 +885,9 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, 
VirtQueue *vq)
 }
 trace_virtio_snd_handle_tx_xfer();
 
-for (VirtIOSoundPCMStream *stream = NULL;; stream = NULL) {
+for (;;) {
+VirtIOSoundPCMStream *stream;
+
 elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
 if (!elem) {
 break;
@@ -964,7 +966,9 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, 
VirtQueue *vq)
 }
 trace_virtio_snd_handle_rx_xfer();
 
-for (VirtIOSoundPCMStream *stream = NULL;; stream = NULL) {
+for (;;) {
+VirtIOSoundPCMStream *stream;
+
 elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
 if (!elem) {
 break;
-- 
2.41.0

[PULL 12/16] hw/net/lan9118: Replace magic '2048' value by MIL_TXFIFO_SIZE definition

[Philippe Mathieu-Daudé]

The magic 2048 is explained in the LAN9211 datasheet (DS2414A)
in chapter 1.4, "10/100 Ethernet MAC":

  The MAC Interface Layer (MIL), within the MAC, contains a
  2K Byte transmit and a 128 Byte receive FIFO which is separate
  from the TX and RX FIFOs. [...]

Note, the use of the constant in lan9118_receive() reveals that
our implementation is using the same buffer for both tx and rx.

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Peter Maydell 
Message-Id: <20240409133801.23503-2-phi...@linaro.org>
---
 hw/net/lan9118.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index 47ff25b441..8214569a2c 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -150,6 +150,12 @@ do { printf("lan9118: " fmt , ## __VA_ARGS__); } while (0)
 
 #define GPT_TIMER_EN0x2000
 
+/*
+ * The MAC Interface Layer (MIL), within the MAC, contains a 2K Byte transmit
+ * and a 128 Byte receive FIFO which is separate from the TX and RX FIFOs.
+ */
+#define MIL_TXFIFO_SIZE 2048
+
 enum tx_state {
 TX_IDLE,
 TX_B,
@@ -166,7 +172,7 @@ typedef struct {
 int32_t pad;
 int32_t fifo_used;
 int32_t len;
-uint8_t data[2048];
+uint8_t data[MIL_TXFIFO_SIZE];
 } LAN9118Packet;
 
 static const VMStateDescription vmstate_lan9118_packet = {
@@ -182,7 +188,7 @@ static const VMStateDescription vmstate_lan9118_packet = {
 VMSTATE_INT32(pad, LAN9118Packet),
 VMSTATE_INT32(fifo_used, LAN9118Packet),
 VMSTATE_INT32(len, LAN9118Packet),
-VMSTATE_UINT8_ARRAY(data, LAN9118Packet, 2048),
+VMSTATE_UINT8_ARRAY(data, LAN9118Packet, MIL_TXFIFO_SIZE),
 VMSTATE_END_OF_LIST()
 }
 };
@@ -544,7 +550,7 @@ static ssize_t lan9118_receive(NetClientState *nc, const 
uint8_t *buf,
 return -1;
 }
 
-if (size >= 2048 || size < 14) {
+if (size >= MIL_TXFIFO_SIZE || size < 14) {
 return -1;
 }
 
-- 
2.41.0

[PULL 06/16] hw/block/nand: Factor nand_load_iolen() method out

[Philippe Mathieu-Daudé]

Reviewed-by: Richard Henderson 
Reviewed-by: Kevin Wolf 
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20240409135944.24997-2-phi...@linaro.org>
---
 hw/block/nand.c | 35 ++-
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index d1435f2207..f33eb2d552 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -243,9 +243,28 @@ static inline void nand_pushio_byte(NANDFlashState *s, 
uint8_t value)
 }
 }
 
+/*
+ * nand_load_block: Load block containing (s->addr + @offset).
+ * Returns length of data available at @offset in this block.
+ */
+static unsigned nand_load_block(NANDFlashState *s, unsigned offset)
+{
+unsigned iolen;
+
+s->blk_load(s, s->addr, offset);
+
+iolen = (1 << s->page_shift);
+if (s->gnd) {
+iolen += 1 << s->oob_shift;
+}
+assert(offset <= iolen);
+iolen -= offset;
+
+return iolen;
+}
+
 static void nand_command(NANDFlashState *s)
 {
-unsigned int offset;
 switch (s->cmd) {
 case NAND_CMD_READ0:
 s->iolen = 0;
@@ -271,12 +290,7 @@ static void nand_command(NANDFlashState *s)
 case NAND_CMD_NOSERIALREAD2:
 if (!(nand_flash_ids[s->chip_id].options & NAND_SAMSUNG_LP))
 break;
-offset = s->addr & ((1 << s->addr_shift) - 1);
-s->blk_load(s, s->addr, offset);
-if (s->gnd)
-s->iolen = (1 << s->page_shift) - offset;
-else
-s->iolen = (1 << s->page_shift) + (1 << s->oob_shift) - offset;
+s->iolen = nand_load_block(s, s->addr & ((1 << s->addr_shift) - 1));
 break;
 
 case NAND_CMD_RESET:
@@ -597,12 +611,7 @@ uint32_t nand_getio(DeviceState *dev)
 if (!s->iolen && s->cmd == NAND_CMD_READ0) {
 offset = (int) (s->addr & ((1 << s->addr_shift) - 1)) + s->offset;
 s->offset = 0;
-
-s->blk_load(s, s->addr, offset);
-if (s->gnd)
-s->iolen = (1 << s->page_shift) - offset;
-else
-s->iolen = (1 << s->page_shift) + (1 << s->oob_shift) - offset;
+s->iolen = nand_load_block(s, offset);
 }
 
 if (s->ce || s->iolen <= 0) {
-- 
2.41.0

[PULL 15/16] hw/net/net_tx_pkt: Fix overrun in update_sctp_checksum()

[Philippe Mathieu-Daudé]

If a fragmented packet size is too short, do not try to
calculate its checksum.

Reproduced using:

  $ cat << EOF | qemu-system-i386 -display none -nodefaults \
  -machine q35,accel=qtest -m 32M \
  -device igb,netdev=net0 \
  -netdev user,id=net0 \
  -qtest stdio
  outl 0xcf8 0x8810
  outl 0xcfc 0xe000
  outl 0xcf8 0x8804
  outw 0xcfc 0x06
  write 0xe403 0x1 0x02
  writel 0xe0003808 0x
  write 0xe000381a 0x1 0x5b
  write 0xe000381b 0x1 0x00
  EOF
  Assertion failed: (offset == 0), function iov_from_buf_full, file util/iov.c, 
line 39.
  #1 0x5575e81e952a in iov_from_buf_full qemu/util/iov.c:39:5
  #2 0x5575e6500768 in net_tx_pkt_update_sctp_checksum 
qemu/hw/net/net_tx_pkt.c:144:9
  #3 0x5575e659f3e1 in igb_setup_tx_offloads qemu/hw/net/igb_core.c:478:11
  #4 0x5575e659f3e1 in igb_tx_pkt_send qemu/hw/net/igb_core.c:552:10
  #5 0x5575e659f3e1 in igb_process_tx_desc qemu/hw/net/igb_core.c:671:17
  #6 0x5575e659f3e1 in igb_start_xmit qemu/hw/net/igb_core.c:903:9
  #7 0x5575e659f3e1 in igb_set_tdt qemu/hw/net/igb_core.c:2812:5
  #8 0x5575e657d6a4 in igb_core_write qemu/hw/net/igb_core.c:4248:9

Fixes: CVE-2024-3567
Cc: qemu-sta...@nongnu.org
Reported-by: Zheyu Ma 
Fixes: f199b13bc1 ("igb: Implement Tx SCTP CSO")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2273
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Akihiko Odaki 
Acked-by: Jason Wang 
Message-Id: <20240410070459.49112-1-phi...@linaro.org>
---
 hw/net/net_tx_pkt.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
index 2134a18c4c..b7b1de816d 100644
--- a/hw/net/net_tx_pkt.c
+++ b/hw/net/net_tx_pkt.c
@@ -141,6 +141,10 @@ bool net_tx_pkt_update_sctp_checksum(struct NetTxPkt *pkt)
 uint32_t csum = 0;
 struct iovec *pl_start_frag = pkt->vec + NET_TX_PKT_PL_START_FRAG;
 
+if (iov_size(pl_start_frag, pkt->payload_frags) < 8 + sizeof(csum)) {
+return false;
+}
+
 if (iov_from_buf(pl_start_frag, pkt->payload_frags, 8, , 
sizeof(csum)) < sizeof(csum)) {
 return false;
 }
-- 
2.41.0

[PULL 09/16] hw/misc/applesmc: Do not call DeviceReset from DeviceRealize

[Philippe Mathieu-Daudé]

QDev core layer always call DeviceReset() after DeviceRealize(),
no need to do it manually. Remove the extra call.

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Peter Maydell 
Message-Id: <20240408095217.57239-2-phi...@linaro.org>
---
 hw/misc/applesmc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index 72300d0cbc..8e65816da6 100644
--- a/hw/misc/applesmc.c
+++ b/hw/misc/applesmc.c
@@ -342,7 +342,6 @@ static void applesmc_isa_realize(DeviceState *dev, Error 
**errp)
 }
 
 QLIST_INIT(>data_def);
-qdev_applesmc_isa_reset(dev);
 }
 
 static Property applesmc_isa_properties[] = {
-- 
2.41.0

[PULL 05/16] qemu-options: Fix CXL Fixed Memory Window interleave-granularity typo

[Philippe Mathieu-Daudé]

From: Yuquan Wang 

Fix the unit typo of interleave-granularity of CXL Fixed Memory
Window in qemu-option.hx.

Fixes: 03b39fcf64 ("hw/cxl: Make the CFMW a machine parameter.")
Signed-off-by: Yuquan Wang wangyuquan1...@phytium.com.cn
Message-ID: <20240407083539.1488172-2-wangyuquan1...@phytium.com.cn>
[PMD: Reworded]
Signed-off-by: Philippe Mathieu-Daudé 
---
 qemu-options.hx | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 7fd1713fa8..8ce85d4559 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -151,14 +151,14 @@ SRST
 platform and configuration dependent.
 
 ``interleave-granularity=granularity`` sets the granularity of
-interleave. Default 256KiB. Only 256KiB, 512KiB, 1024KiB, 2048KiB
-4096KiB, 8192KiB and 16384KiB granularities supported.
+interleave. Default 256 (bytes). Only 256, 512, 1k, 2k,
+4k, 8k and 16k granularities supported.
 
 Example:
 
 ::
 
--machine 
cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=128G,cxl-fmw.0.interleave-granularity=512k
+-machine 
cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=128G,cxl-fmw.0.interleave-granularity=512
 ERST
 
 DEF("M", HAS_ARG, QEMU_OPTION_M,
-- 
2.41.0

[PULL 14/16] hw/sd/sdhci: Do not update TRNMOD when Command Inhibit (DAT) is set

[Philippe Mathieu-Daudé]

Per "SD Host Controller Standard Specification Version 3.00":

  * 2.2.5 Transfer Mode Register (Offset 00Ch)

Writes to this register shall be ignored when the Command
Inhibit (DAT) in the Present State register is 1.

Do not update the TRNMOD register when Command Inhibit (DAT)
bit is set to avoid the present-status register going out of
sync, leading to malicious guest using DMA mode and overflowing
the FIFO buffer:

  $ cat << EOF | qemu-system-i386 \
 -display none -nographic -nodefaults \
 -machine accel=qtest -m 512M \
 -device sdhci-pci,sd-spec-version=3 \
 -device sd-card,drive=mydrive \
 -drive 
if=none,index=0,file=null-co://,format=raw,id=mydrive \
 -qtest stdio
  outl 0xcf8 0x80001013
  outl 0xcfc 0x91
  outl 0xcf8 0x80001001
  outl 0xcfc 0x0600
  write 0x912c 0x1 0x05
  write 0x9158 0x1 0x16
  write 0x9105 0x1 0x04
  write 0x9128 0x1 0x08
  write 0x16 0x1 0x21
  write 0x19 0x1 0x20
  write 0x910c 0x1 0x01
  write 0x910e 0x1 0x20
  write 0x910f 0x1 0x00
  write 0x910c 0x1 0x00
  write 0x9120 0x1 0x00
  EOF

Stack trace (part):
=
==89993==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x61529900 at pc 0x55d5f885700d bp 0x7ffc1e1e9470 sp 0x7ffc1e1e9468
WRITE of size 1 at 0x61529900 thread T0
#0 0x55d5f885700c in sdhci_write_dataport hw/sd/sdhci.c:564:39
#1 0x55d5f8849150 in sdhci_write hw/sd/sdhci.c:1223:13
#2 0x55d5fa01db63 in memory_region_write_accessor system/memory.c:497:5
#3 0x55d5fa01d245 in access_with_adjusted_size system/memory.c:573:18
#4 0x55d5fa01b1a9 in memory_region_dispatch_write system/memory.c:1521:16
#5 0x55d5fa09f5c9 in flatview_write_continue system/physmem.c:2711:23
#6 0x55d5fa08f78b in flatview_write system/physmem.c:2753:12
#7 0x55d5fa08f258 in address_space_write system/physmem.c:2860:18
...
0x61529900 is located 0 bytes to the right of 512-byte region
[0x61529700,0x61529900) allocated by thread T0 here:
#0 0x55d5f7237b27 in __interceptor_calloc
#1 0x7f9e36dd4c50 in g_malloc0
#2 0x55d5f88672f7 in sdhci_pci_realize hw/sd/sdhci-pci.c:36:5
#3 0x55d5f844b582 in pci_qdev_realize hw/pci/pci.c:2092:9
#4 0x55d5fa2ee74b in device_set_realized hw/core/qdev.c:510:13
#5 0x55d5fa325bfb in property_set_bool qom/object.c:2358:5
#6 0x55d5fa31ea45 in object_property_set qom/object.c:1472:5
#7 0x55d5fa332509 in object_property_set_qobject om/qom-qobject.c:28:10
#8 0x55d5fa31f6ed in object_property_set_bool qom/object.c:1541:15
#9 0x55d5fa2e2948 in qdev_realize hw/core/qdev.c:292:12
#10 0x55d5f8eed3f1 in qdev_device_add_from_qdict 
system/qdev-monitor.c:719:10
#11 0x55d5f8eef7ff in qdev_device_add system/qdev-monitor.c:738:11
#12 0x55d5f8f211f0 in device_init_func system/vl.c:1200:11
#13 0x55d5fad0877d in qemu_opts_foreach util/qemu-option.c:1135:14
#14 0x55d5f8f0df9c in qemu_create_cli_devices system/vl.c:2638:5
#15 0x55d5f8f0db24 in qmp_x_exit_preconfig system/vl.c:2706:5
#16 0x55d5f8f14dc0 in qemu_init system/vl.c:3737:9
...
SUMMARY: AddressSanitizer: heap-buffer-overflow hw/sd/sdhci.c:564:39
in sdhci_write_dataport

Add assertions to ensure the fifo_buffer[] is not overflowed by
malicious accesses to the Buffer Data Port register.

Fixes: CVE-2024-3447
Cc: qemu-sta...@nongnu.org
Fixes: d7dfca0807 ("hw/sdhci: introduce standard SD host controller")
Buglink: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=58813
Reported-by: Alexander Bulekov 
Reported-by: Chuhong Yuan 
Signed-off-by: Peter Maydell 
Message-Id: 
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20240409145524.27913-1-phi...@linaro.org>
---
 hw/sd/sdhci.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index c5e0bc018b..27673e1c70 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -473,6 +473,7 @@ static uint32_t sdhci_read_dataport(SDHCIState *s, unsigned 
size)
 }
 
 for (i = 0; i < size; i++) {
+assert(s->data_count < s->buf_maxsz);
 value |= s->fifo_buffer[s->data_count] << i * 8;
 s->data_count++;
 /* check if we've read all valid data (blksize bytes) from buffer */
@@ -561,6 +562,7 @@ static void sdhci_write_dataport(SDHCIState *s, uint32_t 
value, unsigned size)
 }
 
 for (i = 0; i < size; i++) {
+assert(s->data_count < s->buf_maxsz);
 s->fifo_buffer[s->data_count] = value & 0xFF;
 s->data_count++;
 value >>= 8;
@@ -1208,6 +1210,12 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
 if (!(s->capareg & R_SDHC_CAPAB_SDMA_MASK)) {
 value &= ~SDHC_TRNS_DMA;
 }
+
+/* TRNMOD writes are inhibited while Command Inhibit (DAT) is true */
+if (s->prnsts & 

[PULL 10/16] hw/misc/applesmc: Fix memory leak in reset() handler

[Philippe Mathieu-Daudé]

AppleSMCData is allocated with g_new0() in applesmc_add_key():
release it with g_free().

Leaked since commit 1ddda5cd36 ("AppleSMC device emulation").

Cc: qemu-sta...@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2272
Reported-by: Zheyu Ma 
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Peter Maydell 
Message-Id: <20240408095217.57239-3-phi...@linaro.org>
---
 hw/misc/applesmc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
index 8e65816da6..14e3ef667d 100644
--- a/hw/misc/applesmc.c
+++ b/hw/misc/applesmc.c
@@ -274,6 +274,7 @@ static void qdev_applesmc_isa_reset(DeviceState *dev)
 /* Remove existing entries */
 QLIST_FOREACH_SAFE(d, >data_def, node, next) {
 QLIST_REMOVE(d, node);
+g_free(d);
 }
 s->status = 0x00;
 s->status_1e = 0x00;
-- 
2.41.0

[PULL 07/16] hw/block/nand: Have blk_load() take unsigned offset and return boolean

[Philippe Mathieu-Daudé]

Negative offset is meaningless, use unsigned type.
Return a boolean value indicating success.

Reviewed-by: Richard Henderson 
Reviewed-by: Kevin Wolf 
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20240409135944.24997-3-phi...@linaro.org>
---
 hw/block/nand.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index f33eb2d552..5a31d78b6b 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -84,7 +84,11 @@ struct NANDFlashState {
 
 void (*blk_write)(NANDFlashState *s);
 void (*blk_erase)(NANDFlashState *s);
-void (*blk_load)(NANDFlashState *s, uint64_t addr, int offset);
+/*
+ * Returns %true when block containing (@addr + @offset) is
+ * successfully loaded, otherwise %false.
+ */
+bool (*blk_load)(NANDFlashState *s, uint64_t addr, unsigned offset);
 
 uint32_t ioaddr_vmstate;
 };
@@ -772,11 +776,11 @@ static void glue(nand_blk_erase_, 
NAND_PAGE_SIZE)(NANDFlashState *s)
 }
 }
 
-static void glue(nand_blk_load_, NAND_PAGE_SIZE)(NANDFlashState *s,
-uint64_t addr, int offset)
+static bool glue(nand_blk_load_, NAND_PAGE_SIZE)(NANDFlashState *s,
+ uint64_t addr, unsigned 
offset)
 {
 if (PAGE(addr) >= s->pages) {
-return;
+return false;
 }
 
 if (s->blk) {
@@ -804,6 +808,8 @@ static void glue(nand_blk_load_, 
NAND_PAGE_SIZE)(NANDFlashState *s,
 offset, NAND_PAGE_SIZE + OOB_SIZE - offset);
 s->ioaddr = s->io;
 }
+
+return true;
 }
 
 static void glue(nand_init_, NAND_PAGE_SIZE)(NANDFlashState *s)
-- 
2.41.0

[PULL 04/16] hw/virtio/virtio-crypto: Protect from DMA re-entrancy bugs

[Philippe Mathieu-Daudé]

Replace qemu_bh_new_guarded() by virtio_bh_new_guarded()
so the bus and device use the same guard. Otherwise the
DMA-reentrancy protection can be bypassed.

Fixes: CVE-2024-3446
Cc: qemu-sta...@nongnu.org
Suggested-by: Alexander Bulekov 
Reviewed-by: Gerd Hoffmann 
Acked-by: Michael S. Tsirkin 
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Michael S. Tsirkin 
Message-Id: <20240409105537.18308-5-phi...@linaro.org>
---
 hw/virtio/virtio-crypto.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index fe1313f2ad..bbe8aa4b99 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -1080,8 +1080,8 @@ static void virtio_crypto_device_realize(DeviceState 
*dev, Error **errp)
 vcrypto->vqs[i].dataq =
  virtio_add_queue(vdev, 1024, virtio_crypto_handle_dataq_bh);
 vcrypto->vqs[i].dataq_bh =
- qemu_bh_new_guarded(virtio_crypto_dataq_bh, >vqs[i],
- >mem_reentrancy_guard);
+ virtio_bh_new_guarded(dev, virtio_crypto_dataq_bh,
+   >vqs[i]);
 vcrypto->vqs[i].vcrypto = vcrypto;
 }
 
-- 
2.41.0

[PULL 11/16] backends/cryptodev: Do not abort for invalid session ID

[Philippe Mathieu-Daudé]

Instead of aborting when a session ID is invalid,
return VIRTIO_CRYPTO_INVSESS ("Invalid session id").

Reproduced using:

  $ cat << EOF | qemu-system-i386 -display none \
 -machine q35,accel=qtest -m 512M -nodefaults \
 -object cryptodev-backend-builtin,id=cryptodev0 \
 -device virtio-crypto-pci,id=crypto0,cryptodev=cryptodev0 \
 -qtest stdio
  outl 0xcf8 0x8804
  outw 0xcfc 0x06
  outl 0xcf8 0x8820
  outl 0xcfc 0xe0008000
  write 0x10800e 0x1 0x01
  write 0xe0008016 0x1 0x01
  write 0xe0008020 0x4 0x00801000
  write 0xe0008028 0x4 0x00c01000
  write 0xe000801c 0x1 0x01
  write 0x11 0x1 0x05
  write 0x110001 0x1 0x04
  write 0x108002 0x1 0x11
  write 0x108008 0x1 0x48
  write 0x10800c 0x1 0x01
  write 0x108018 0x1 0x10
  write 0x10801c 0x1 0x02
  write 0x10c002 0x1 0x01
  write 0xe000b005 0x1 0x00
  EOF
  Assertion failed: (session_id < MAX_NUM_SESSIONS && 
builtin->sessions[session_id]),
  function cryptodev_builtin_close_session, file cryptodev-builtin.c, line 430.

Cc: qemu-sta...@nongnu.org
Reported-by: Zheyu Ma 
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2274
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: zhenwei pi 
Message-Id: <20240409094757.9127-1-phi...@linaro.org>
---
 backends/cryptodev-builtin.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c
index 39d0455280..a514bbb310 100644
--- a/backends/cryptodev-builtin.c
+++ b/backends/cryptodev-builtin.c
@@ -427,7 +427,9 @@ static int cryptodev_builtin_close_session(
   CRYPTODEV_BACKEND_BUILTIN(backend);
 CryptoDevBackendBuiltinSession *session;
 
-assert(session_id < MAX_NUM_SESSIONS && builtin->sessions[session_id]);
+if (session_id >= MAX_NUM_SESSIONS || !builtin->sessions[session_id]) {
+return -VIRTIO_CRYPTO_INVSESS;
+}
 
 session = builtin->sessions[session_id];
 if (session->cipher) {
-- 
2.41.0

[PULL 00/16] Misc HW patches for 2024-04-10

[Philippe Mathieu-Daudé]

The following changes since commit 927284d65bce63ab1495d3febe7c7b5b6d563874:

  Merge tag 'edk2-20240409-pull-request' of https://gitlab.com/kraxel/qemu into 
staging (2024-04-09 17:36:40 +0100)

are available in the Git repository at:

  https://github.com/philmd/qemu.git tags/hw-misc-20240410

for you to fetch changes up to dcb0a1ac03d6b5ba6c7fcbe467f0215738006113:

  hw/audio/virtio-snd: Remove unused assignment (2024-04-10 11:07:37 +0200)


Misc HW patch queue

- Fix CXL Fixed Memory Window interleave-granularity typo
- Fix for DMA re-entrancy abuse with VirtIO devices (CVE-2024-3446)
- Fix out-of-bound access in NAND block buffer
- Fix memory leak in AppleSMC reset() handler
- Avoid VirtIO crypto backends abort o invalid session ID
- Fix overflow in LAN9118 MIL TX FIFO
- Fix overflow when abusing SDHCI TRNMOD register (CVE-2024-3447)
- Fix overrun in short fragmented packet SCTP checksum (CVE-2024-3567)
- Remove unused assignment in virtio-snd model (Coverity 1542933 & 1542934)



Philippe Mathieu-Daudé (15):
  hw/virtio: Introduce virtio_bh_new_guarded() helper
  hw/display/virtio-gpu: Protect from DMA re-entrancy bugs
  hw/char/virtio-serial-bus: Protect from DMA re-entrancy bugs
  hw/virtio/virtio-crypto: Protect from DMA re-entrancy bugs
  hw/block/nand: Factor nand_load_iolen() method out
  hw/block/nand: Have blk_load() take unsigned offset and return boolean
  hw/block/nand: Fix out-of-bound access in NAND block buffer
  hw/misc/applesmc: Do not call DeviceReset from DeviceRealize
  hw/misc/applesmc: Fix memory leak in reset() handler
  backends/cryptodev: Do not abort for invalid session ID
  hw/net/lan9118: Replace magic '2048' value by MIL_TXFIFO_SIZE
definition
  hw/net/lan9118: Fix overflow in MIL TX FIFO
  hw/sd/sdhci: Do not update TRNMOD when Command Inhibit (DAT) is set
  hw/net/net_tx_pkt: Fix overrun in update_sctp_checksum()
  hw/audio/virtio-snd: Remove unused assignment

Yuquan Wang (1):
  qemu-options: Fix CXL Fixed Memory Window interleave-granularity typo

 include/hw/virtio/virtio.h   |  7 +
 backends/cryptodev-builtin.c |  4 ++-
 hw/audio/virtio-snd.c|  8 --
 hw/block/nand.c  | 55 +---
 hw/char/virtio-serial-bus.c  |  3 +-
 hw/display/virtio-gpu.c  |  6 ++--
 hw/misc/applesmc.c   |  2 +-
 hw/net/lan9118.c | 28 +++---
 hw/net/net_tx_pkt.c  |  4 +++
 hw/sd/sdhci.c|  8 ++
 hw/virtio/virtio-crypto.c|  4 +--
 hw/virtio/virtio.c   | 10 +++
 qemu-options.hx  |  6 ++--
 13 files changed, 109 insertions(+), 36 deletions(-)

-- 
2.41.0

[PULL 03/16] hw/char/virtio-serial-bus: Protect from DMA re-entrancy bugs

[Philippe Mathieu-Daudé]

Replace qemu_bh_new_guarded() by virtio_bh_new_guarded()
so the bus and device use the same guard. Otherwise the
DMA-reentrancy protection can be bypassed.

Fixes: CVE-2024-3446
Cc: qemu-sta...@nongnu.org
Suggested-by: Alexander Bulekov 
Reviewed-by: Gerd Hoffmann 
Acked-by: Michael S. Tsirkin 
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Michael S. Tsirkin 
Message-Id: <20240409105537.18308-4-phi...@linaro.org>
---
 hw/char/virtio-serial-bus.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 016aba6374..2094d213cd 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -985,8 +985,7 @@ static void virtser_port_device_realize(DeviceState *dev, 
Error **errp)
 return;
 }
 
-port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
-   >mem_reentrancy_guard);
+port->bh = virtio_bh_new_guarded(dev, flush_queued_data_bh, port);
 port->elem = NULL;
 }
 
-- 
2.41.0

Re: [PATCH] hw/audio/virtio-snd: Remove unused assignment

[Philippe Mathieu-Daudé]

On 10/4/24 07:37, Philippe Mathieu-Daudé wrote:

Coverity reported:

   >>> CID 1542933:  Code maintainability issues  (UNUSED_VALUE)
   >>> CID 1542934:  Code maintainability issues  (UNUSED_VALUE)
   >>> Assigning value "NULL" to "stream" here, but that stored
   value is overwritten before it can be used.

Simply remove the unused assignments.

Resolves: Coverity CID 1542933
Resolves: Coverity CID 1542934
Fixes: 731655f87f ("virtio-snd: rewrite invalid tx/rx message handling")
Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/audio/virtio-snd.c | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)


Per IRC:
Reviewed-by: Manos Pitsidianakis 

Thanks, queued.

Re: [PATCH v2 1/1] virtio-pci: Fix the crash that the vector was used after released.

[Jason Wang]

Offline:

On Wed, Apr 10, 2024 at 2:28 PM Cindy Lu  wrote:
>
> On Wed, Apr 10, 2024 at 1:36 PM Jason Wang  wrote:
> >
> > On Wed, Apr 10, 2024 at 1:29 PM Cindy Lu  wrote:
> > >
> > > When the guest triggers vhost_stop and then virtio_reset, the vector will 
> > > the
> > > IRQFD for this vector will be released and change to VIRTIO_NO_VECTOR.
> > > After that, the guest called vhost_net_start,  (at this time, the 
> > > configure
> > > vector is still VIRTIO_NO_VECTOR),  vector 0 still was not "init".
> > > The guest system continued to boot, set the vector back to 0, and then 
> > > met the crash.
> > >
> > > To fix this, we need to call the function 
> > > "kvm_virtio_pci_vector_use_one()"
> > > when the vector changes back from VIRTIO_NO_VECTOR
> > >
> > > (gdb) bt
> > > 0  __pthread_kill_implementation (threadid=, 
> > > signo=signo@entry=6, no_tid=no_tid@entry=0)
> > > at pthread_kill.c:44
> > > 1  0x7fc87148ec53 in __pthread_kill_internal (signo=6, 
> > > threadid=) at pthread_kill.c:78
> > > 2  0x7fc87143e956 in __GI_raise (sig=sig@entry=6) at 
> > > ../sysdeps/posix/raise.c:26
> > > 3  0x7fc8714287f4 in __GI_abort () at abort.c:79
> > > 4  0x7fc87142871b in __assert_fail_base
> > > (fmt=0x7fc8715bbde0 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
> > > assertion=0x5606413efd53 "ret == 0", file=0x5606413ef87d 
> > > "../accel/kvm/kvm-all.c", line=1837, function=) at 
> > > assert.c:92
> > > 5  0x7fc871437536 in __GI___assert_fail
> > > (assertion=0x5606413efd53 "ret == 0", file=0x5606413ef87d 
> > > "../accel/kvm/kvm-all.c", line=1837, function=0x5606413f06f0 
> > > <__PRETTY_FUNCTION__.19> "kvm_irqchip_commit_routes") at assert.c:101
> > > 6  0x560640f884b5 in kvm_irqchip_commit_routes (s=0x560642cae1f0) at 
> > > ../accel/kvm/kvm-all.c:1837
> > > 7  0x560640c98f8e in virtio_pci_one_vector_unmask
> > > (proxy=0x560643c65f00, queue_no=4294967295, vector=0, msg=..., 
> > > n=0x560643c6e4c8)
> > > at ../hw/virtio/virtio-pci.c:1005
> > > 8  0x560640c99201 in virtio_pci_vector_unmask (dev=0x560643c65f00, 
> > > vector=0, msg=...)
> > > at ../hw/virtio/virtio-pci.c:1070
> > > 9  0x560640bc402e in msix_fire_vector_notifier (dev=0x560643c65f00, 
> > > vector=0, is_masked=false)
> > > at ../hw/pci/msix.c:120
> > > 10 0x560640bc40f1 in msix_handle_mask_update (dev=0x560643c65f00, 
> > > vector=0, was_masked=true)
> > > at ../hw/pci/msix.c:140
> > > 11 0x560640bc4503 in msix_table_mmio_write (opaque=0x560643c65f00, 
> > > addr=12, val=0, size=4)
> > > at ../hw/pci/msix.c:231
> > > 12 0x560640f26d83 in memory_region_write_accessor
> > > (mr=0x560643c66540, addr=12, value=0x7fc86b7bc628, size=4, shift=0, 
> > > mask=4294967295, attrs=...)
> > > at ../system/memory.c:497
> > > 13 0x560640f270a6 in access_with_adjusted_size
> > >
> > >  (addr=12, value=0x7fc86b7bc628, size=4, access_size_min=1, 
> > > access_size_max=4, access_fn=0x560640f26c8d 
> > > , mr=0x560643c66540, attrs=...) at 
> > > ../system/memory.c:573
> > > 14 0x560640f2a2b5 in memory_region_dispatch_write (mr=0x560643c66540, 
> > > addr=12, data=0, op=MO_32, attrs=...)
> > > at ../system/memory.c:1521
> > > 15 0x560640f37bac in flatview_write_continue
> > > (fv=0x7fc65805e0b0, addr=4273803276, attrs=..., ptr=0x7fc871e9c028, 
> > > len=4, addr1=12, l=4, mr=0x560643c66540)
> > > at ../system/physmem.c:2714
> > > 16 0x560640f37d0f in flatview_write
> > > (fv=0x7fc65805e0b0, addr=4273803276, attrs=..., buf=0x7fc871e9c028, 
> > > len=4) at ../system/physmem.c:2756
> > > 17 0x560640f380bf in address_space_write
> > > (as=0x560642161ae0 , addr=4273803276, 
> > > attrs=..., buf=0x7fc871e9c028, len=4)
> > > at ../system/physmem.c:2863
> > > 18 0x560640f3812c in address_space_rw
> > > (as=0x560642161ae0 , addr=4273803276, 
> > > attrs=..., buf=0x7fc871e9c028, len=4, is_write=true) at 
> > > ../system/physmem.c:2873
> > > --Type  for more, q to quit, c to continue without paging--
> > > 19 0x560640f8aa55 in kvm_cpu_exec (cpu=0x560642f205e0) at 
> > > ../accel/kvm/kvm-all.c:2915
> > > 20 0x560640f8d731 in kvm_vcpu_thread_fn (arg=0x560642f205e0) at 
> > > ../accel/kvm/kvm-accel-ops.c:51
> > > 21 0x5606411949f4 in qemu_thread_start (args=0x560642f292b0) at 
> > > ../util/qemu-thread-posix.c:541
> > > 22 0x7fc87148cdcd in start_thread (arg=) at 
> > > pthread_create.c:442
> > > 23 0x7fc871512630 in clone3 () at 
> > > ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
> > > (gdb)
> > > Signed-off-by: Cindy Lu 
> > > ---
> > >  hw/virtio/virtio-pci.c | 35 +++
> > >  1 file changed, 35 insertions(+)
> > >
> > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > index 1a7039fb0c..344f4fb844 100644
> > > --- a/hw/virtio/virtio-pci.c
> > > +++ b/hw/virtio/virtio-pci.c
> > > @@ -880,6 +880,7 @@ static int 
> > > kvm_virtio_pci_vector_use_one(VirtIOPCIProxy 

Re: [PATCH-for-9.0? v2] hw/net/net_tx_pkt: Fix overrun in update_sctp_checksum()

[Philippe Mathieu-Daudé]

On 10/4/24 09:35, Mauro Matteo Cascella wrote:

Hi,

On Wed, Apr 10, 2024 at 9:05 AM Philippe Mathieu-Daudé
 wrote:


If a fragmented packet size is too short, do not try to
calculate its checksum.


This was assigned CVE-2024-3567.


Thanks for the quick reaction!


Reproduced using:

   $ cat << EOF | qemu-system-i386 -display none -nodefaults \
   -machine q35,accel=qtest -m 32M \
   -device igb,netdev=net0 \
   -netdev user,id=net0 \
   -qtest stdio
   outl 0xcf8 0x8810
   outl 0xcfc 0xe000
   outl 0xcf8 0x8804
   outw 0xcfc 0x06
   write 0xe403 0x1 0x02
   writel 0xe0003808 0x
   write 0xe000381a 0x1 0x5b
   write 0xe000381b 0x1 0x00
   EOF
   Assertion failed: (offset == 0), function iov_from_buf_full, file 
util/iov.c, line 39.
   #1 0x5575e81e952a in iov_from_buf_full qemu/util/iov.c:39:5
   #2 0x5575e6500768 in net_tx_pkt_update_sctp_checksum 
qemu/hw/net/net_tx_pkt.c:144:9
   #3 0x5575e659f3e1 in igb_setup_tx_offloads qemu/hw/net/igb_core.c:478:11
   #4 0x5575e659f3e1 in igb_tx_pkt_send qemu/hw/net/igb_core.c:552:10
   #5 0x5575e659f3e1 in igb_process_tx_desc qemu/hw/net/igb_core.c:671:17
   #6 0x5575e659f3e1 in igb_start_xmit qemu/hw/net/igb_core.c:903:9
   #7 0x5575e659f3e1 in igb_set_tdt qemu/hw/net/igb_core.c:2812:5
   #8 0x5575e657d6a4 in igb_core_write qemu/hw/net/igb_core.c:4248:9

Cc: qemu-sta...@nongnu.org
Reported-by: Zheyu Ma 
Fixes: f199b13bc1 ("igb: Implement Tx SCTP CSO")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2273
Signed-off-by: Philippe Mathieu-Daudé 
---
Since v1: check at offset 8 (Akihiko)
---
  hw/net/net_tx_pkt.c | 4 
  1 file changed, 4 insertions(+)


Patch queued.

Re: [PATCH-for-9.0? v2] hw/net/net_tx_pkt: Fix overrun in update_sctp_checksum()

[Jason Wang]

On Wed, Apr 10, 2024 at 3:06 PM Akihiko Odaki  wrote:
>
> On 2024/04/10 16:04, Philippe Mathieu-Daudé wrote:
> > If a fragmented packet size is too short, do not try to
> > calculate its checksum.
> >
> > Reproduced using:
> >
> >$ cat << EOF | qemu-system-i386 -display none -nodefaults \
> >-machine q35,accel=qtest -m 32M \
> >-device igb,netdev=net0 \
> >-netdev user,id=net0 \
> >-qtest stdio
> >outl 0xcf8 0x8810
> >outl 0xcfc 0xe000
> >outl 0xcf8 0x8804
> >outw 0xcfc 0x06
> >write 0xe403 0x1 0x02
> >writel 0xe0003808 0x
> >write 0xe000381a 0x1 0x5b
> >write 0xe000381b 0x1 0x00
> >EOF
> >Assertion failed: (offset == 0), function iov_from_buf_full, file 
> > util/iov.c, line 39.
> >#1 0x5575e81e952a in iov_from_buf_full qemu/util/iov.c:39:5
> >#2 0x5575e6500768 in net_tx_pkt_update_sctp_checksum 
> > qemu/hw/net/net_tx_pkt.c:144:9
> >#3 0x5575e659f3e1 in igb_setup_tx_offloads qemu/hw/net/igb_core.c:478:11
> >#4 0x5575e659f3e1 in igb_tx_pkt_send qemu/hw/net/igb_core.c:552:10
> >#5 0x5575e659f3e1 in igb_process_tx_desc qemu/hw/net/igb_core.c:671:17
> >#6 0x5575e659f3e1 in igb_start_xmit qemu/hw/net/igb_core.c:903:9
> >#7 0x5575e659f3e1 in igb_set_tdt qemu/hw/net/igb_core.c:2812:5
> >#8 0x5575e657d6a4 in igb_core_write qemu/hw/net/igb_core.c:4248:9
> >
> > Cc: qemu-sta...@nongnu.org
> > Reported-by: Zheyu Ma 
> > Fixes: f199b13bc1 ("igb: Implement Tx SCTP CSO")
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2273
> > Signed-off-by: Philippe Mathieu-Daudé 
>
> Reviewed-by: Akihiko Odaki 

Fixes: CVE-2024-3567
Acked-by: Jason Wang 

Peter, would you want to pick this for 9.0?

Thanks

>
> > ---
> > Since v1: check at offset 8 (Akihiko)
> > ---
> >   hw/net/net_tx_pkt.c | 4 
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
> > index 2134a18c4c..b7b1de816d 100644
> > --- a/hw/net/net_tx_pkt.c
> > +++ b/hw/net/net_tx_pkt.c
> > @@ -141,6 +141,10 @@ bool net_tx_pkt_update_sctp_checksum(struct NetTxPkt 
> > *pkt)
> >   uint32_t csum = 0;
> >   struct iovec *pl_start_frag = pkt->vec + NET_TX_PKT_PL_START_FRAG;
> >
> > +if (iov_size(pl_start_frag, pkt->payload_frags) < 8 + sizeof(csum)) {
> > +return false;
> > +}
> > +
> >   if (iov_from_buf(pl_start_frag, pkt->payload_frags, 8, , 
> > sizeof(csum)) < sizeof(csum)) {
> >   return false;
> >   }
>

Re: vhost-user-blk reconnect issue

[Yajun Wu]

On 4/2/2024 4:44 PM, Li Feng wrote:

*External email: Use caution opening links or attachments*



Hi,

I tested it today and there is indeed a problem in this scenario.
It seems that the first version of the patch is the best and can 
handle all scenarios.

With this patch, the previously merged patches are no longer useful.


I will revert this patch and submit a new fix. Do you have any comments?

Revert: 
https://lore.kernel.org/all/a68c0148e9bf105f9e83ff5e763b8fcb6f7ba9be.1697644299.git@redhat.com/ 

New: 
https://lore.kernel.org/all/20230804052954.2918915-2-fen...@smartx.com/ 



Looks good to me.

Thanks,

Yajun



Thanks,
Li


2024年4月1日 16:43，Yajun Wu  写道：


On 4/1/2024 4:34 PM, Li Feng wrote:

*External email: Use caution opening links or attachments*


Hi yajun,

I have submitted a patch to fix this problem a few months ago, but 
in the end this solution was not accepted and other solutions

were adopted to fix it.

[PATCH 1/2] vhost-user: fix lost reconnect - Li Feng 

lore.kernel.org 








I think this fix is valid.


This is the merged fix:


[PULL 76/83] vhost-user: fix lost reconnect - Michael S. Tsirkin 

lore.kernel.org 








My tests are with this fix, failed in the two scenarios I mentioned.



Thanks,
Li


2024年4月1日 10:08，Yajun Wu  写道：


On 3/27/2024 6:47 PM, Stefano Garzarella wrote:

External email: Use caution opening links or attachments


Hi Yajun,

On Mon, Mar 25, 2024 at 10:54:13AM +, Yajun Wu wrote:

Hi experts,

With latest QEMU (8.2.90), we find two vhost-user-blk backend 
reconnect

failure scenarios:

Do you know if has it ever worked and so it's a regression, or have we
always had this problem?


I am afraid this commit: "71e076a07d (2022-12-01 02:30:13 -0500) 
hw/virtio: generalise CHR_EVENT_CLOSED handling"  caused both 
failures. Previous hash is good.


I suspect the "if (vhost->vdev)" in vhost_user_async_close_bh is 
the cause, previous code doesn't have this check?




Thanks,
Stefano

1. Disconnect vhost-user-blk backend before guest driver probe 
vblk device, then reconnect backend after guest driver probe 
device. QEMU won't send out any vhost messages to restore backend.
This is because vhost->vdev is NULL before guest driver probe 
vblk device, so vhost_user_blk_disconnect won't be called, 
s->connected is still true. Next vhost_user_blk_connect will 
simply return without doing anything.


2. modprobe -r virtio-blk inside VM, then disconnect backend, 
then reconnect backend, then modprobe virtio-blk. QEMU won't send 
messages in vhost_dev_init.
This is because rmmod will let qemu call vhost_user_blk_stop, 
vhost->vdev also become NULL(in vhost_dev_stop), 
vhost_user_blk_disconnect won't be called. Again s->connected is 
still true, even chr connect is closed.


I think even vhost->vdev is NULL, vhost_user_blk_disconnect 
should be called when chr connect close?

Hope we can have a fix soon.


Thanks,
Yajun

[Stable-8.2.3 69/87] target/riscv: rvv: Remove the dependency of Zvfbfmin to Zfbfmin

[Michael Tokarev]

From: Max Chou 

According to the Zvfbfmin definition in the RISC-V BF16 extensions spec,
the Zvfbfmin extension only requires either the V extension or the
Zve32f extension.

Signed-off-by: Max Chou 
Reviewed-by: Alistair Francis 
Message-ID: <20240321170929.1162507-1-max.c...@sifive.com>
Signed-off-by: Alistair Francis 
(cherry picked from commit c9b07fe14d3525cd3f2fc01f46eeb3d4ed7c3603)
Signed-off-by: Michael Tokarev 

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 8a35683a34..b437180b27 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -417,11 +417,6 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, 
Error **errp)
 return;
 }
 
-if (cpu->cfg.ext_zvfbfmin && !cpu->cfg.ext_zfbfmin) {
-error_setg(errp, "Zvfbfmin extension depends on Zfbfmin extension");
-return;
-}
-
 if (cpu->cfg.ext_zvfbfmin && !cpu->cfg.ext_zve32f) {
 error_setg(errp, "Zvfbfmin extension depends on Zve32f extension");
 return;
-- 
2.39.2

[Stable-8.2.3 36/87] target/i386: use separate MMU indexes for 32-bit accesses

[Michael Tokarev]

From: Paolo Bonzini 

Accesses from a 32-bit environment (32-bit code segment for instruction
accesses, EFER.LMA==0 for processor accesses) have to mask away the
upper 32 bits of the address.  While a bit wasteful, the easiest way
to do so is to use separate MMU indexes.  These days, QEMU anyway is
compiled with a fixed value for NB_MMU_MODES.  Split MMU_USER_IDX,
MMU_KSMAP_IDX and MMU_KNOSMAP_IDX in two.

Signed-off-by: Paolo Bonzini 
(cherry picked from commit 90f641531c782c873a05895f411c05fbbbef3c49)
Signed-off-by: Michael Tokarev 
(Mjt: move changes for x86_cpu_mmu_index() to cpu_mmu_index() due to missing
 v8.2.0-1030-gace0c5fe59 "target/i386: Populate CPUClass.mmu_index")

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index df1f602758..70e6713ba3 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2289,27 +2289,42 @@ uint64_t cpu_get_tsc(CPUX86State *env);
 #define cpu_list x86_cpu_list
 
 /* MMU modes definitions */
-#define MMU_KSMAP_IDX   0
-#define MMU_USER_IDX1
-#define MMU_KNOSMAP_IDX 2
-#define MMU_NESTED_IDX  3
-#define MMU_PHYS_IDX4
+#define MMU_KSMAP64_IDX0
+#define MMU_KSMAP32_IDX1
+#define MMU_USER64_IDX 2
+#define MMU_USER32_IDX 3
+#define MMU_KNOSMAP64_IDX  4
+#define MMU_KNOSMAP32_IDX  5
+#define MMU_PHYS_IDX   6
+#define MMU_NESTED_IDX 7
+
+#ifdef CONFIG_USER_ONLY
+#ifdef TARGET_X86_64
+#define MMU_USER_IDX MMU_USER64_IDX
+#else
+#define MMU_USER_IDX MMU_USER32_IDX
+#endif
+#endif
 
 static inline int cpu_mmu_index(CPUX86State *env, bool ifetch)
 {
-return (env->hflags & HF_CPL_MASK) == 3 ? MMU_USER_IDX :
-(!(env->hflags & HF_SMAP_MASK) || (env->eflags & AC_MASK))
-? MMU_KNOSMAP_IDX : MMU_KSMAP_IDX;
+int mmu_index_32 = (env->hflags & HF_CS64_MASK) ? 1 : 0;
+int mmu_index_base =
+(env->hflags & HF_CPL_MASK) == 3 ? MMU_USER64_IDX :
+!(env->hflags & HF_SMAP_MASK) ? MMU_KNOSMAP64_IDX :
+(env->eflags & AC_MASK) ? MMU_KNOSMAP64_IDX : MMU_KSMAP64_IDX;
+
+return mmu_index_base + mmu_index_32;
 }
 
 static inline bool is_mmu_index_smap(int mmu_index)
 {
-return mmu_index == MMU_KSMAP_IDX;
+return (mmu_index & ~1) == MMU_KSMAP64_IDX;
 }
 
 static inline bool is_mmu_index_user(int mmu_index)
 {
-return mmu_index == MMU_USER_IDX;
+return (mmu_index & ~1) == MMU_USER64_IDX;
 }
 
 static inline bool is_mmu_index_32(int mmu_index)
@@ -2320,9 +2335,12 @@ static inline bool is_mmu_index_32(int mmu_index)
 
 static inline int cpu_mmu_index_kernel(CPUX86State *env)
 {
-return !(env->hflags & HF_SMAP_MASK) ? MMU_KNOSMAP_IDX :
-((env->hflags & HF_CPL_MASK) < 3 && (env->eflags & AC_MASK))
-? MMU_KNOSMAP_IDX : MMU_KSMAP_IDX;
+int mmu_index_32 = (env->hflags & HF_LMA_MASK) ? 1 : 0;
+int mmu_index_base =
+!(env->hflags & HF_SMAP_MASK) ? MMU_KNOSMAP64_IDX :
+((env->hflags & HF_CPL_MASK) < 3 && (env->eflags & AC_MASK)) ? 
MMU_KNOSMAP64_IDX : MMU_KSMAP64_IDX;
+
+return mmu_index_base + mmu_index_32;
 }
 
 #define CC_DST  (env->cc_dst)
diff --git a/target/i386/tcg/sysemu/excp_helper.c 
b/target/i386/tcg/sysemu/excp_helper.c
index 18fde700c1..8f7011d966 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -542,7 +542,8 @@ static bool get_physical_address(CPUX86State *env, vaddr 
addr,
 if (likely(use_stage2)) {
 in.cr3 = env->nested_cr3;
 in.pg_mode = env->nested_pg_mode;
-in.mmu_idx = MMU_USER_IDX;
+in.mmu_idx =
+env->nested_pg_mode & PG_MODE_LMA ? MMU_USER64_IDX : 
MMU_USER32_IDX;
 in.ptw_idx = MMU_PHYS_IDX;
 
 if (!mmu_translate(env, , out, err)) {
-- 
2.39.2

[Stable-8.2.3 87/87] virtio-snd: rewrite invalid tx/rx message handling

[Michael Tokarev]

From: Manos Pitsidianakis 

The current handling of invalid virtqueue elements inside the TX/RX virt
queue handlers is wrong.

They are added in a per-stream invalid queue to be processed after the
handler is done examining each message, but the invalid message might
not be specifying any stream_id; which means it's invalid to add it to
any stream->invalid queue since stream could be NULL at this point.

This commit moves the invalid queue to the VirtIOSound struct which
guarantees there will always be a valid temporary place to store them
inside the tx/rx handlers. The queue will be emptied before the handler
returns, so the queue must be empty at any other point of the device's
lifetime.

Signed-off-by: Manos Pitsidianakis 
Message-Id: 

Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
(cherry picked from commit 731655f87f319fd06f27282c6cafbc2467ac8045)
Signed-off-by: Michael Tokarev 

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index 2d118d6423..256a132ece 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -456,7 +456,6 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, 
uint32_t stream_id)
 stream->s = s;
 qemu_mutex_init(>queue_mutex);
 QSIMPLEQ_INIT(>queue);
-QSIMPLEQ_INIT(>invalid);
 
 /*
  * stream_id >= s->snd_conf.streams was checked before so this is
@@ -611,9 +610,6 @@ static size_t 
virtio_snd_pcm_get_io_msgs_count(VirtIOSoundPCMStream *stream)
 QSIMPLEQ_FOREACH_SAFE(buffer, >queue, entry, next) {
 count += 1;
 }
-QSIMPLEQ_FOREACH_SAFE(buffer, >invalid, entry, next) {
-count += 1;
-}
 }
 return count;
 }
@@ -831,47 +827,36 @@ static void virtio_snd_handle_event(VirtIODevice *vdev, 
VirtQueue *vq)
 trace_virtio_snd_handle_event();
 }
 
+/*
+ * Must only be called if vsnd->invalid is not empty.
+ */
 static inline void empty_invalid_queue(VirtIODevice *vdev, VirtQueue *vq)
 {
 VirtIOSoundPCMBuffer *buffer = NULL;
-VirtIOSoundPCMStream *stream = NULL;
 virtio_snd_pcm_status resp = { 0 };
 VirtIOSound *vsnd = VIRTIO_SND(vdev);
-bool any = false;
 
-for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
-stream = vsnd->pcm->streams[i];
-if (stream) {
-any = false;
-WITH_QEMU_LOCK_GUARD(>queue_mutex) {
-while (!QSIMPLEQ_EMPTY(>invalid)) {
-buffer = QSIMPLEQ_FIRST(>invalid);
-if (buffer->vq != vq) {
-break;
-}
-any = true;
-resp.status = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
-iov_from_buf(buffer->elem->in_sg,
- buffer->elem->in_num,
- 0,
- ,
- sizeof(virtio_snd_pcm_status));
-virtqueue_push(vq,
-   buffer->elem,
-   sizeof(virtio_snd_pcm_status));
-QSIMPLEQ_REMOVE_HEAD(>invalid, entry);
-virtio_snd_pcm_buffer_free(buffer);
-}
-if (any) {
-/*
- * Notify vq about virtio_snd_pcm_status responses.
- * Buffer responses must be notified separately later.
- */
-virtio_notify(vdev, vq);
-}
-}
-}
+g_assert(!QSIMPLEQ_EMPTY(>invalid));
+
+while (!QSIMPLEQ_EMPTY(>invalid)) {
+buffer = QSIMPLEQ_FIRST(>invalid);
+/* If buffer->vq != vq, our logic is fundamentally wrong, so bail out 
*/
+g_assert(buffer->vq == vq);
+
+resp.status = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
+iov_from_buf(buffer->elem->in_sg,
+ buffer->elem->in_num,
+ 0,
+ ,
+ sizeof(virtio_snd_pcm_status));
+virtqueue_push(vq,
+   buffer->elem,
+   sizeof(virtio_snd_pcm_status));
+QSIMPLEQ_REMOVE_HEAD(>invalid, entry);
+virtio_snd_pcm_buffer_free(buffer);
 }
+/* Notify vq about virtio_snd_pcm_status responses. */
+virtio_notify(vdev, vq);
 }
 
 /*
@@ -883,15 +868,14 @@ static inline void empty_invalid_queue(VirtIODevice 
*vdev, VirtQueue *vq)
  */
 static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq)
 {
-VirtIOSound *s = VIRTIO_SND(vdev);
-VirtIOSoundPCMStream *stream = NULL;
+VirtIOSound *vsnd = VIRTIO_SND(vdev);
 VirtIOSoundPCMBuffer *buffer;
 VirtQueueElement *elem;
 size_t msg_sz, size;
 virtio_snd_pcm_xfer hdr;
 uint32_t stream_id;
 /*
- * If any of the I/O messages are invalid, put them in stream->invalid and
+ * If any of the I/O messages are invalid, put them in vsnd->invalid and
  

[Stable-8.2.3 74/87] tcg/optimize: Fix sign_mask for logical right-shift

[Michael Tokarev]

From: Richard Henderson 

The 'sign' computation is attempting to locate the sign bit that has
been repeated, so that we can test if that bit is known zero.  That
computation can be zero if there are no known sign repetitions.

Cc: qemu-sta...@nongnu.org
Fixes: 93a967fbb57 ("tcg/optimize: Propagate sign info for shifting")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2248
Signed-off-by: Richard Henderson 
Reviewed-by: Philippe Mathieu-Daudé 
(cherry picked from commit 2911e9b95f3bb03783ae5ca3e2494dc3b44a9161)
Signed-off-by: Michael Tokarev 

diff --git a/tcg/optimize.c b/tcg/optimize.c
index f2d01654c5..6fcdda68ef 100644
--- a/tcg/optimize.c
+++ b/tcg/optimize.c
@@ -2123,7 +2123,7 @@ static bool fold_shift(OptContext *ctx, TCGOp *op)
  * will not reduced the number of input sign repetitions.
  */
 sign = (s_mask & -s_mask) >> 1;
-if (!(z_mask & sign)) {
+if (sign && !(z_mask & sign)) {
 ctx->s_mask = s_mask;
 }
 break;
diff --git a/tests/tcg/aarch64/Makefile.target 
b/tests/tcg/aarch64/Makefile.target
index ea3e232e65..0efd565f05 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -10,6 +10,7 @@ VPATH += $(AARCH64_SRC)
 
 # Base architecture tests
 AARCH64_TESTS=fcvt pcalign-a64 lse2-fault
+AARCH64_TESTS += test-2248
 
 fcvt: LDFLAGS+=-lm
 
diff --git a/tests/tcg/aarch64/test-2248.c b/tests/tcg/aarch64/test-2248.c
new file mode 100644
index 00..aac2e17836
--- /dev/null
+++ b/tests/tcg/aarch64/test-2248.c
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* See https://gitlab.com/qemu-project/qemu/-/issues/2248 */
+
+#include 
+
+__attribute__((noinline))
+long test(long x, long y, long sh)
+{
+long r;
+asm("cmp   %1, %2\n\t"
+"cset  x12, lt\n\t"
+"and   w11, w12, #0xff\n\t"
+"cmp   w11, #0\n\t"
+"csetm x14, ne\n\t"
+"lsr   x13, x14, %3\n\t"
+"sxtb  %0, w13"
+: "=r"(r)
+: "r"(x), "r"(y), "r"(sh)
+: "x11", "x12", "x13", "x14");
+return r;
+}
+
+int main()
+{
+long r = test(0, 1, 2);
+assert(r == -1);
+return 0;
+}
-- 
2.39.2