Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
On 2024/4/12 14:41, Jason Wang wrote: > On Fri, Apr 12, 2024 at 2:05 PM Chen, Jiqian wrote: >> >> On 2024/4/7 19:49, Michael S. Tsirkin wrote: > I will set the default value of No_Soft_Reset bit to true in next version > according to your opinion. > About the compatibility of old machine types, which types should I > consider? Does the same as x-pcie-pm-init(hw_compat_2_8)? > Forgive me for not knowing much about compatibility. "x" means no compatibility at all, please drop the "x" prefix. And it looks more safe to start as "false" by default. Thanks >>> >>> >>> Not sure I agree. External flags are for when users want to tweak them. >>> When would users want it to be off? >>> What is done here is I feel sane, just add machine compat machinery >>> to change to off for old machine types. >> Do you know which old machines should I consider to compatible with? >> Or which guys should I add to "CC" and can get answer from them? >> I have less knowledge about compatibility. > > If you make it off by default, you don't need otherwise, it's one > release before. Ok, thanks. In next version, I will follow the result of discussion and remove "x", while adding some comments in codes. > > Thanks > >> >>> >> >> -- >> Best regards, >> Jiqian Chen. > -- Best regards, Jiqian Chen.
Re: [RFC 0/2] Identify aliased maps in vdpa SVQ iova_tree
On Wed, Apr 10, 2024 at 6:03 PM Eugenio Pérez wrote: > > 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. I think I don't understand if there's any side effect for shadow virtqueue? Thanks > > 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 >
Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
On Fri, Apr 12, 2024 at 1:59 PM Chen, Jiqian wrote:
>
> On 2024/4/7 11:20, Jason Wang wrote:
> > On Tue, Apr 2, 2024 at 11:03 AM Chen, Jiqian wrote:
> >>
> >> On 2024/3/29 18:44, Michael S. Tsirkin wrote:
> >>> On Fri, Mar 29, 2024 at 03:20:59PM +0800, Jason Wang wrote:
> On Fri, Mar 29, 2024 at 3:07 PM Chen, Jiqian wrote:
> >
> > On 2024/3/28 20:36, Michael S. Tsirkin wrote:
> > +}
> > +
> > static void virtio_pci_bus_reset_hold(Object *obj)
> > {
> > PCIDevice *dev = PCI_DEVICE(obj);
> > DeviceState *qdev = DEVICE(obj);
> >
> > +if (virtio_pci_no_soft_reset(dev)) {
> > +return;
> > +}
> > +
> > virtio_pci_reset(qdev);
> >
> > if (pci_is_express(dev)) {
> > @@ -2484,6 +2511,8 @@ static Property virtio_pci_properties[] = {
> > VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
> > DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
> > VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
> > +DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy,
> > flags,
> > +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, false),
>
> Why does it come with an x prefix?
>
> > DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
> > VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
> > DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
>
> I am a bit confused about this part.
> Do you want to make this software controllable?
> >>> Yes, because even the real hardware, this bit is not always set.
>
> We are talking about emulated devices here.
>
> >>
> >> So which virtio devices should and which should not set this bit?
> > This depends on the scenario the virtio-device is used, if we want to
> > trigger an internal soft reset for the virtio-device during S3, this
> > bit shouldn't be set.
>
> If the device doesn't need reset, why bother the driver for this?
>
> Btw, no_soft_reset is insufficient for some cases, there's a proposal
> for the virtio-spec. I think we need to wait until it is done.
> >>>
> >>> That seems orthogonal or did I miss something?
> >> Yes, I looked the detail of the proposal, I also think they are unrelated.
> >
> > The point is the proposal said
> >
> > """
> > Without a mechanism to
> > suspend/resume virtio devices when the driver is suspended/resumed in
> > the early phase of suspend/late phase of resume, there is a window where
> > interrupts can be lost.
> > """
> >
> > It looks safe to enable it with the suspend bit. Or if you think it's
> > wrong, please comment on the virtio spec patch.
> If I understand the proposal correctly.
> Only need to check the SUSPEND bit when virtio_pci_bus_reset_hold is called.
> It seems the proposal won't block this patch to upstream.
> In next version, I will add comments to note the SUSPEND bit that need to be
> considered once it is accepted.
>
> >
> >> I will set the default value of No_Soft_Reset bit to true in next version
> >> according to your opinion.
> >> About the compatibility of old machine types, which types should I
> >> consider? Does the same as x-pcie-pm-init(hw_compat_2_8)?
> >> Forgive me for not knowing much about compatibility.
> >
> > "x" means no compatibility at all, please drop the "x" prefix. And it
> Thanks to explain.
> So it seems the prefix "x" of "x-pcie-pm-init" is also wrong? Because it
> considered the hw_compat_2_8. Also "x-pcie-flr-init".
Probably but too late to fix.
> Back to No_Soft_Reset, do you know which old machines should I consider to
> compatible with?
Replied in another mail.
Thanks
>
> > looks more safe to start as "false" by default.
> >
> > Thanks
> >
> >>>
> > In my use case on my environment, I don't want to reset virtio-gpu
> > during S3,
> > because once the display resources are destroyed, there are not enough
> > information to re-create them, so this bit should be set.
> > Making this bit software controllable is convenient for users to take
> > their own choices.
>
> Thanks
>
> >
> >>
> Or should this be set to true by default and then
> changed to false for old machine types?
> >>> How can I do so?
> >>> Do you mean set this to true by default, and if old machine types
> >>> don't need this bit, they can pass false config to qemu when running
> >>> qemu?
> >>
> >> No, you would use compat machinery. See how is x-pcie-flr-init handled.
> >>
> >>
> >
> > --
> > Best regards,
> > Jiqian Chen.
> >>>
> >>
> >> --
> >> Best regards,
> >> Jiqian Chen.
> >
>
> --
> Best regards,
> Jiqian Chen.
Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
On Fri, Apr 12, 2024 at 2:05 PM Chen, Jiqian wrote: > > On 2024/4/7 19:49, Michael S. Tsirkin wrote: > >>> I will set the default value of No_Soft_Reset bit to true in next version > >>> according to your opinion. > >>> About the compatibility of old machine types, which types should I > >>> consider? Does the same as x-pcie-pm-init(hw_compat_2_8)? > >>> Forgive me for not knowing much about compatibility. > >> > >> "x" means no compatibility at all, please drop the "x" prefix. And it > >> looks more safe to start as "false" by default. > >> > >> Thanks > > > > > > Not sure I agree. External flags are for when users want to tweak them. > > When would users want it to be off? > > What is done here is I feel sane, just add machine compat machinery > > to change to off for old machine types. > Do you know which old machines should I consider to compatible with? > Or which guys should I add to "CC" and can get answer from them? > I have less knowledge about compatibility. If you make it off by default, you don't need otherwise, it's one release before. Thanks > > > > > -- > Best regards, > Jiqian Chen.
Re: [PATCH v6] virtio-pci: Fix the crash that the vector was used after released.
Hi All
I apologize for bothering you again
I send the new patch is because I found that the function
kvm_virtio_pci_vector_use_one/kvm_virtio_pci_vector_release_one
can only change the vector that already set to the device.
ret = virtio_pci_get_notifier(proxy, queue_no, &n, &vector);
if (ret < 0) {
return;
}
...
So I move the setting vector into the function
virtio_pci_set_and_change_vector()
the other part are the same .
the sanity test is passed and the qemu qtest is also passed
Thanks
Cindy
On Fri, Apr 12, 2024 at 2:28 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)
>
> Fixes: f9a09c
[PATCH v6] virtio-pci: Fix the crash that the vector was used after released.
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)
Fixes: f9a09ca3ea ("vhost: add support for configure interrupt")
Cc: qemu-sta...@nongnu.org
Signed-off-by: Cindy Lu
---
hw/virtio/virtio-pci.c | 43 --
1 file changed, 41 insertions(+), 2 deletions(-)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 1a7039fb0c..f83ec92990 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1423,6 +1423,36 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
return offset;
}
+static void virtio_pci_set_and_change_vector(VirtIODevice *vdev,
+ VirtIOPCIProxy *proxy,
+ int queue_no, uint16_t old_vector,
+
[PATCH 1/2] hw: Fix problem with the A*MPCORE switches in the Kconfig files
A9MPCORE, ARM11MPCORE and A15MPCORE are defined twice, once in hw/cpu/Kconfig and once in hw/arm/Kconfig. This is only possible by accident, since hw/cpu/Kconfig is never included from hw/Kconfig. Fix it by declaring the switches only in hw/cpu/Kconfig (since the related files reside in the hw/cpu/ folder) and by making sure that the file hw/cpu/Kconfig is now properly included from hw/Kconfig. Signed-off-by: Thomas Huth --- hw/Kconfig | 1 + hw/arm/Kconfig | 15 --- hw/cpu/Kconfig | 12 +--- 3 files changed, 10 insertions(+), 18 deletions(-) diff --git a/hw/Kconfig b/hw/Kconfig index 2c00936c28..9567cc475d 100644 --- a/hw/Kconfig +++ b/hw/Kconfig @@ -48,6 +48,7 @@ source watchdog/Kconfig # arch Kconfig source arm/Kconfig +source cpu/Kconfig source alpha/Kconfig source avr/Kconfig source cris/Kconfig diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig index 893a7bff66..d97015c45c 100644 --- a/hw/arm/Kconfig +++ b/hw/arm/Kconfig @@ -678,21 +678,6 @@ config ZAURUS select NAND select ECC -config A9MPCORE -bool -select A9_GTIMER -select A9SCU # snoop control unit -select ARM_GIC -select ARM_MPTIMER - -config A15MPCORE -bool -select ARM_GIC - -config ARM11MPCORE -bool -select ARM11SCU - config ARMSSE bool select ARM_V7M diff --git a/hw/cpu/Kconfig b/hw/cpu/Kconfig index 1767d028ac..f776e884cd 100644 --- a/hw/cpu/Kconfig +++ b/hw/cpu/Kconfig @@ -1,8 +1,14 @@ -config ARM11MPCORE -bool - config A9MPCORE bool +select A9_GTIMER +select A9SCU # snoop control unit +select ARM_GIC +select ARM_MPTIMER config A15MPCORE bool +select ARM_GIC + +config ARM11MPCORE +bool +select ARM11SCU -- 2.44.0
[PATCH 2/2] hw: Add a Kconfig switch for the TYPE_CPU_CLUSTER device
The cpu-cluster device is only needed for some few arm and riscv
machines. Let's avoid compiling and linking it if it is not really
necessary.
Signed-off-by: Thomas Huth
---
hw/arm/Kconfig | 3 +++
hw/cpu/Kconfig | 3 +++
hw/cpu/meson.build | 2 +-
hw/riscv/Kconfig | 2 ++
4 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index d97015c45c..5d4015b75a 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -485,6 +485,7 @@ config XLNX_ZYNQMP_ARM
select AHCI
select ARM_GIC
select CADENCE
+select CPU_CLUSTER
select DDC
select DPCD
select SDHCI
@@ -503,6 +504,7 @@ config XLNX_VERSAL
default y
depends on TCG && AARCH64
select ARM_GIC
+select CPU_CLUSTER
select PL011
select CADENCE
select VIRTIO_MMIO
@@ -688,6 +690,7 @@ config ARMSSE
select CMSDK_APB_DUALTIMER
select CMSDK_APB_UART
select CMSDK_APB_WATCHDOG
+select CPU_CLUSTER
select IOTKIT_SECCTL
select IOTKIT_SYSCTL
select IOTKIT_SYSINFO
diff --git a/hw/cpu/Kconfig b/hw/cpu/Kconfig
index f776e884cd..baff478e1b 100644
--- a/hw/cpu/Kconfig
+++ b/hw/cpu/Kconfig
@@ -12,3 +12,6 @@ config A15MPCORE
config ARM11MPCORE
bool
select ARM11SCU
+
+config CPU_CLUSTER
+bool
diff --git a/hw/cpu/meson.build b/hw/cpu/meson.build
index 38cdcfbe57..43a34c4c6e 100644
--- a/hw/cpu/meson.build
+++ b/hw/cpu/meson.build
@@ -1,4 +1,4 @@
-system_ss.add(files('core.c', 'cluster.c'))
+system_ss.add(when: 'CONFIG_CPU_CLUSTER', if_true: files('core.c',
'cluster.c'))
system_ss.add(when: 'CONFIG_ARM11MPCORE', if_true: files('arm11mpcore.c'))
system_ss.add(when: 'CONFIG_REALVIEW', if_true: files('realview_mpcore.c'))
diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
index 5d644eb7b1..fc72ef0379 100644
--- a/hw/riscv/Kconfig
+++ b/hw/riscv/Kconfig
@@ -9,6 +9,7 @@ config IBEX
config MICROCHIP_PFSOC
bool
select CADENCE_SDHCI
+select CPU_CLUSTER
select MCHP_PFSOC_DMC
select MCHP_PFSOC_IOSCB
select MCHP_PFSOC_MMUART
@@ -68,6 +69,7 @@ config SIFIVE_E
config SIFIVE_U
bool
select CADENCE
+select CPU_CLUSTER
select RISCV_ACLINT
select SIFIVE_GPIO
select SIFIVE_PDMA
--
2.44.0
[PATCH 0/2] Improvements for switches in hw/cpu/Kconfig
First patch fixes the problem that the file hw/cpu/Kconfig is currently ignored and the switches there are duplicated in hw/arm/. The second patch introduces a proper config switch for the cpu-cluster device. Thomas Huth (2): hw: Fix problem with the A*MPCORE switches in the Kconfig files hw: Add a Kconfig switch for the TYPE_CPU_CLUSTER device hw/Kconfig | 1 + hw/arm/Kconfig | 18 +++--- hw/cpu/Kconfig | 15 --- hw/cpu/meson.build | 2 +- hw/riscv/Kconfig | 2 ++ 5 files changed, 19 insertions(+), 19 deletions(-) -- 2.44.0
Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
On 2024/4/8 12:56, Jason Wang wrote: I will set the default value of No_Soft_Reset bit to true in next version according to your opinion. About the compatibility of old machine types, which types should I consider? Does the same as x-pcie-pm-init(hw_compat_2_8)? Forgive me for not knowing much about compatibility. >>> >>> "x" means no compatibility at all, please drop the "x" prefix. And it >>> looks more safe to start as "false" by default. >>> >>> Thanks >> >> >> Not sure I agree. External flags are for when users want to tweak them. >> When would users want it to be off? > > If I understand the suspending status proposal correctly, there are at > least 1 device that is not safe. And this series does not mention > which device it has tested. I only tested virtio-gpu with my patch, I will mention this in "commit message" in next version. > > It means if we enable it unconditionally, guests may break. Make sense, will keep " No_Soft_Reset bit " false by default. And add some comments in the codes to describe what you considered. > > Thanks > >> What is done here is I feel sane, just add machine compat machinery >> to change to off for old machine types. -- Best regards, Jiqian Chen.
Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
On 2024/4/7 19:49, Michael S. Tsirkin wrote: >>> I will set the default value of No_Soft_Reset bit to true in next version >>> according to your opinion. >>> About the compatibility of old machine types, which types should I >>> consider? Does the same as x-pcie-pm-init(hw_compat_2_8)? >>> Forgive me for not knowing much about compatibility. >> >> "x" means no compatibility at all, please drop the "x" prefix. And it >> looks more safe to start as "false" by default. >> >> Thanks > > > Not sure I agree. External flags are for when users want to tweak them. > When would users want it to be off? > What is done here is I feel sane, just add machine compat machinery > to change to off for old machine types. Do you know which old machines should I consider to compatible with? Or which guys should I add to "CC" and can get answer from them? I have less knowledge about compatibility. > -- Best regards, Jiqian Chen.
Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit
On 2024/4/7 11:20, Jason Wang wrote:
> On Tue, Apr 2, 2024 at 11:03 AM Chen, Jiqian wrote:
>>
>> On 2024/3/29 18:44, Michael S. Tsirkin wrote:
>>> On Fri, Mar 29, 2024 at 03:20:59PM +0800, Jason Wang wrote:
On Fri, Mar 29, 2024 at 3:07 PM Chen, Jiqian wrote:
>
> On 2024/3/28 20:36, Michael S. Tsirkin wrote:
> +}
> +
> static void virtio_pci_bus_reset_hold(Object *obj)
> {
> PCIDevice *dev = PCI_DEVICE(obj);
> DeviceState *qdev = DEVICE(obj);
>
> +if (virtio_pci_no_soft_reset(dev)) {
> +return;
> +}
> +
> virtio_pci_reset(qdev);
>
> if (pci_is_express(dev)) {
> @@ -2484,6 +2511,8 @@ static Property virtio_pci_properties[] = {
> VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
> DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
> VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
> +DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags,
> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, false),
Why does it come with an x prefix?
> DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
> VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
> DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
I am a bit confused about this part.
Do you want to make this software controllable?
>>> Yes, because even the real hardware, this bit is not always set.
We are talking about emulated devices here.
>>
>> So which virtio devices should and which should not set this bit?
> This depends on the scenario the virtio-device is used, if we want to
> trigger an internal soft reset for the virtio-device during S3, this bit
> shouldn't be set.
If the device doesn't need reset, why bother the driver for this?
Btw, no_soft_reset is insufficient for some cases, there's a proposal
for the virtio-spec. I think we need to wait until it is done.
>>>
>>> That seems orthogonal or did I miss something?
>> Yes, I looked the detail of the proposal, I also think they are unrelated.
>
> The point is the proposal said
>
> """
> Without a mechanism to
> suspend/resume virtio devices when the driver is suspended/resumed in
> the early phase of suspend/late phase of resume, there is a window where
> interrupts can be lost.
> """
>
> It looks safe to enable it with the suspend bit. Or if you think it's
> wrong, please comment on the virtio spec patch.
If I understand the proposal correctly.
Only need to check the SUSPEND bit when virtio_pci_bus_reset_hold is called.
It seems the proposal won't block this patch to upstream.
In next version, I will add comments to note the SUSPEND bit that need to be
considered once it is accepted.
>
>> I will set the default value of No_Soft_Reset bit to true in next version
>> according to your opinion.
>> About the compatibility of old machine types, which types should I consider?
>> Does the same as x-pcie-pm-init(hw_compat_2_8)?
>> Forgive me for not knowing much about compatibility.
>
> "x" means no compatibility at all, please drop the "x" prefix. And it
Thanks to explain.
So it seems the prefix "x" of "x-pcie-pm-init" is also wrong? Because it
considered the hw_compat_2_8. Also "x-pcie-flr-init".
Back to No_Soft_Reset, do you know which old machines should I consider to
compatible with?
> looks more safe to start as "false" by default.
>
> Thanks
>
>>>
> In my use case on my environment, I don't want to reset virtio-gpu during
> S3,
> because once the display resources are destroyed, there are not enough
> information to re-create them, so this bit should be set.
> Making this bit software controllable is convenient for users to take
> their own choices.
Thanks
>
>>
Or should this be set to true by default and then
changed to false for old machine types?
>>> How can I do so?
>>> Do you mean set this to true by default, and if old machine types don't
>>> need this bit, they can pass false config to qemu when running qemu?
>>
>> No, you would use compat machinery. See how is x-pcie-flr-init handled.
>>
>>
>
> --
> Best regards,
> Jiqian Chen.
>>>
>>
>> --
>> Best regards,
>> Jiqian Chen.
>
--
Best regards,
Jiqian Chen.
Re: [PATCH] target/sparc: Use GET_ASI_CODE for ASI_KERNELTXT and ASI_USERTXT
On Thu, Apr 11, 2024, 10:15 PM Richard Henderson <
richard.hender...@linaro.org> wrote:
> Reads are done with execute access. It is not clear whether writes
> are legal at all -- for now, leave helper_st_asi unchanged, so that
> we continue to raise an mmu fault.
>
> This generalizes the exiting code for ASI_KERNELTXT to be usable for
> ASI_USERTXT as well, by passing down the MemOpIdx to use.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2281
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2059
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1609
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1166
> Signed-off-by: Richard Henderson
> ---
> target/sparc/helper.h | 3 ++
> target/sparc/ldst_helper.c | 65 ++
> target/sparc/translate.c | 49 ++--
> 3 files changed, 95 insertions(+), 22 deletions(-)
>
> diff --git a/target/sparc/helper.h b/target/sparc/helper.h
> index e55fad5b8c..b8087d0d2b 100644
> --- a/target/sparc/helper.h
> +++ b/target/sparc/helper.h
> @@ -32,6 +32,9 @@ DEF_HELPER_FLAGS_3(udiv, TCG_CALL_NO_WG, i64, env, tl,
> tl)
> DEF_HELPER_FLAGS_3(sdiv, TCG_CALL_NO_WG, i64, env, tl, tl)
> DEF_HELPER_3(taddcctv, tl, env, tl, tl)
> DEF_HELPER_3(tsubcctv, tl, env, tl, tl)
> +#if !defined(CONFIG_USER_ONLY) && !defined(TARGET_SPARC64)
> +DEF_HELPER_FLAGS_3(ld_code, TCG_CALL_NO_WG, i64, env, tl, i32)
> +#endif
> #if !defined(CONFIG_USER_ONLY) || defined(TARGET_SPARC64)
> DEF_HELPER_FLAGS_4(ld_asi, TCG_CALL_NO_WG, i64, env, tl, int, i32)
> DEF_HELPER_FLAGS_5(st_asi, TCG_CALL_NO_WG, void, env, tl, i64, int, i32)
> diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c
> index e581bb42ac..2846a86cc4 100644
> --- a/target/sparc/ldst_helper.c
> +++ b/target/sparc/ldst_helper.c
> @@ -585,7 +585,6 @@ uint64_t helper_ld_asi(CPUSPARCState *env,
> target_ulong addr,
> #if defined(DEBUG_MXCC) || defined(DEBUG_ASI)
> uint32_t last_addr = addr;
> #endif
> -MemOpIdx oi;
>
> do_check_align(env, addr, size - 1, GETPC());
> switch (asi) {
> @@ -684,24 +683,6 @@ uint64_t helper_ld_asi(CPUSPARCState *env,
> target_ulong addr,
> case ASI_M_DIAGS: /* Turbosparc DTLB Diagnostic */
> case ASI_M_IODIAG: /* Turbosparc IOTLB Diagnostic */
> break;
> -case ASI_KERNELTXT: /* Supervisor code access */
> -oi = make_memop_idx(memop, cpu_mmu_index(env_cpu(env), true));
> -switch (size) {
> -case 1:
> -ret = cpu_ldb_code_mmu(env, addr, oi, GETPC());
> -break;
> -case 2:
> -ret = cpu_ldw_code_mmu(env, addr, oi, GETPC());
> -break;
> -default:
> -case 4:
> -ret = cpu_ldl_code_mmu(env, addr, oi, GETPC());
> -break;
> -case 8:
> -ret = cpu_ldq_code_mmu(env, addr, oi, GETPC());
> -break;
> -}
> -break;
> case ASI_M_TXTC_TAG: /* SparcStation 5 I-cache tag */
> case ASI_M_TXTC_DATA: /* SparcStation 5 I-cache data */
> case ASI_M_DATAC_TAG: /* SparcStation 5 D-cache tag */
> @@ -779,7 +760,6 @@ uint64_t helper_ld_asi(CPUSPARCState *env,
> target_ulong addr,
> case 0x4c: /* SuperSPARC MMU Breakpoint Action */
> ret = env->mmubpaction;
> break;
> -case ASI_USERTXT: /* User code access, XXX */
> default:
> sparc_raise_mmu_fault(cs, addr, false, false, asi, size, GETPC());
> ret = 0;
> @@ -787,6 +767,8 @@ uint64_t helper_ld_asi(CPUSPARCState *env,
> target_ulong addr,
>
> case ASI_USERDATA: /* User data access */
> case ASI_KERNELDATA: /* Supervisor data access */
> +case ASI_USERTXT: /* User code access */
> +case ASI_KERNELTXT: /* Supervisor code access */
> case ASI_P: /* Implicit primary context data access (v9 only?) */
> case ASI_M_BYPASS:/* MMU passthrough */
> case ASI_LEON_BYPASS: /* LEON MMU passthrough */
> @@ -1161,6 +1143,49 @@ void helper_st_asi(CPUSPARCState *env, target_ulong
> addr, uint64_t val,
> #endif
> }
>
> +uint64_t helper_ld_code(CPUSPARCState *env, target_ulong addr, uint32_t
> oi)
> +{
> +MemOp mop = get_memop(oi);
> +uintptr_t ra = GETPC();
> +uint64_t ret;
> +
> +switch (mop & MO_SIZE) {
> +case MO_8:
> +ret = cpu_ldb_code_mmu(env, addr, oi, ra);
> +if (mop & MO_SIGN) {
> +ret = (int8_t)ret;
> +}
> +break;
> +case MO_16:
> +ret = cpu_ldw_code_mmu(env, addr, oi, ra);
> +if ((mop & MO_BSWAP) != MO_TE) {
> +ret = bswap16(ret);
> +}
> +if (mop & MO_SIGN) {
> +ret = (int16_t)ret;
> +}
> +break;
> +case MO_32:
> +ret = cpu_ldl_code_mmu(env, addr, oi, ra);
> +if ((mop & MO_BSWAP) != MO_TE) {
> +ret = bswap32(ret);
> +}
> +if (mop & MO_SIGN) {
> +ret = (int32_t)ret;
> +}
>
Re: [PATCH v7 00/10] Support blob memory and venus on qemu
On Thu, Apr 11, 2024 at 06:19:52PM +0800, Dmitry Osipenko wrote: > Hello, > > This series enables Vulkan Venus context support on virtio-gpu. > Upstreaming of Venus to Qemu was originally started by Antonio Caggiano, > later Huang Rui continued the effort. I'm now taking it over because > Rui will be busy for awhile and he asked me to do so. > Thanks Dmitry! :-) Please go ahead and it's important implementation which is not only on venus but also on virtio native context. Best Regards, Ray
[PATCH v5 3/3] ui/console: Introduce dpy_gl_qemu_dmabuf_new() and free() helpers
From: Dongwon Kim
This commit introduces utility functions for the creation and deallocation
of QemuDmaBuf instances. Additionally, it updates all relevant sections
of the codebase to utilize these new utility functions.
Suggested-by: Marc-André Lureau
Cc: Philippe Mathieu-Daudé
Cc: Vivek Kasireddy
Signed-off-by: Dongwon Kim
---
include/hw/vfio/vfio-common.h | 2 +-
include/hw/virtio/virtio-gpu.h | 4 ++--
include/ui/console.h| 8 +++-
hw/display/vhost-user-gpu.c | 32 +--
hw/display/virtio-gpu-udmabuf.c | 24 +--
hw/vfio/display.c | 26 -
ui/console.c| 34 +
ui/dbus-listener.c | 28 ---
8 files changed, 95 insertions(+), 63 deletions(-)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index b9da6c08ef..d66e27db02 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -148,7 +148,7 @@ typedef struct VFIOGroup {
} VFIOGroup;
typedef struct VFIODMABuf {
-QemuDmaBuf buf;
+QemuDmaBuf *buf;
uint32_t pos_x, pos_y, pos_updates;
uint32_t hot_x, hot_y, hot_updates;
int dmabuf_id;
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index ed44cdad6b..56d6e821bf 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -169,7 +169,7 @@ struct VirtIOGPUBaseClass {
DEFINE_PROP_UINT32("yres", _state, _conf.yres, 800)
typedef struct VGPUDMABuf {
-QemuDmaBuf buf;
+QemuDmaBuf *buf;
uint32_t scanout_id;
QTAILQ_ENTRY(VGPUDMABuf) next;
} VGPUDMABuf;
@@ -238,7 +238,7 @@ struct VhostUserGPU {
VhostUserBackend *vhost;
int vhost_gpu_fd; /* closed by the chardev */
CharBackend vhost_chr;
-QemuDmaBuf dmabuf[VIRTIO_GPU_MAX_SCANOUTS];
+QemuDmaBuf *dmabuf[VIRTIO_GPU_MAX_SCANOUTS];
bool backend_blocked;
};
diff --git a/include/ui/console.h b/include/ui/console.h
index 3d9d8b9fce..6d7c03b7c5 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -358,7 +358,13 @@ void dpy_gl_cursor_dmabuf(QemuConsole *con, QemuDmaBuf
*dmabuf,
bool have_hot, uint32_t hot_x, uint32_t hot_y);
void dpy_gl_cursor_position(QemuConsole *con,
uint32_t pos_x, uint32_t pos_y);
-
+QemuDmaBuf *dpy_gl_qemu_dmabuf_new(uint32_t width, uint32_t height,
+ uint32_t stride, uint32_t x,
+ uint32_t y, uint32_t backing_width,
+ uint32_t backing_height, uint32_t fourcc,
+ uint64_t modifier, uint32_t dmabuf_fd,
+ bool allow_fences, bool y0_top);
+void dpy_gl_qemu_dmabuf_free(QemuDmaBuf *dmabuf);
int32_t dpy_gl_qemu_dmabuf_get_fd(QemuDmaBuf *dmabuf);
uint32_t dpy_gl_qemu_dmabuf_get_width(QemuDmaBuf *dmabuf);
uint32_t dpy_gl_qemu_dmabuf_get_height(QemuDmaBuf *dmabuf);
diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 87dcfbca10..4d8461e94a 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -250,6 +250,7 @@ vhost_user_gpu_handle_display(VhostUserGPU *g,
VhostUserGpuMsg *msg)
VhostUserGpuDMABUFScanout *m = &msg->payload.dmabuf_scanout;
int fd = qemu_chr_fe_get_msgfd(&g->vhost_chr);
int old_fd;
+uint64_t modifier = 0;
QemuDmaBuf *dmabuf;
if (m->scanout_id >= g->parent_obj.conf.max_outputs) {
@@ -262,31 +263,34 @@ vhost_user_gpu_handle_display(VhostUserGPU *g,
VhostUserGpuMsg *msg)
g->parent_obj.enable = 1;
con = g->parent_obj.scanout[m->scanout_id].con;
-dmabuf = &g->dmabuf[m->scanout_id];
-old_fd = dpy_gl_qemu_dmabuf_get_fd(dmabuf);
-if (old_fd >= 0) {
-close(old_fd);
-dmabuf->fd = -1;
+dmabuf = g->dmabuf[m->scanout_id];
+if (dmabuf) {
+old_fd = dpy_gl_qemu_dmabuf_get_fd(dmabuf);
+if (old_fd >= 0) {
+close(old_fd);
+dpy_gl_qemu_dmabuf_set_fd(dmabuf, -1);
+}
}
dpy_gl_release_dmabuf(con, dmabuf);
+g_clear_pointer(&dmabuf, dpy_gl_qemu_dmabuf_free);
if (fd == -1) {
dpy_gl_scanout_disable(con);
break;
}
-*dmabuf = (QemuDmaBuf) {
-.fd = fd,
-.width = m->fd_width,
-.height = m->fd_height,
-.stride = m->fd_stride,
-.fourcc = m->fd_drm_fourcc,
-.y0_top = m->fd_flags & VIRTIO_GPU_RESOURCE_FLAG_Y_0_TOP,
-};
+
if (msg->request == VHOST_USER_GPU_DMABUF_SCANOUT2) {
VhostUserGpuDMABUFScanout2 *m2 = &msg->payload.dmabuf_scanout2;
-dmabuf->modifier = m2->modifier;
+modifier = m2->modifier;
}
+dm
[PATCH v5 2/3] ui/console: Introduce dpy_gl_qemu_dmabuf_set_..() helpers
From: Dongwon Kim
To enhance security in accessing the QemuDmaBuf struct, new helper
functions for setting specific fields within the struct were introduced.
And all occurrences where these fields were previously set directly
have been updated to utilize these helper functions.
Suggested-by: Marc-André Lureau
Cc: Philippe Mathieu-Daudé
Cc: Vivek Kasireddy
Signed-off-by: Dongwon Kim
---
include/ui/console.h | 5 +
ui/console.c | 30 ++
ui/egl-helpers.c | 16 +---
ui/gtk-egl.c | 4 ++--
ui/gtk-gl-area.c | 4 ++--
ui/gtk.c | 2 +-
6 files changed, 49 insertions(+), 12 deletions(-)
diff --git a/include/ui/console.h b/include/ui/console.h
index 6292943a82..3d9d8b9fce 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -375,6 +375,11 @@ void *dpy_gl_qemu_dmabuf_get_sync(QemuDmaBuf *dmabuf);
int32_t dpy_gl_qemu_dmabuf_get_fence_fd(QemuDmaBuf *dmabuf);
bool dpy_gl_qemu_dmabuf_get_allow_fences(QemuDmaBuf *dmabuf);
bool dpy_gl_qemu_dmabuf_get_draw_submitted(QemuDmaBuf *dmabuf);
+void dpy_gl_qemu_dmabuf_set_texture(QemuDmaBuf *dmabuf, uint32_t texture);
+void dpy_gl_qemu_dmabuf_set_fence_fd(QemuDmaBuf *dmabuf, int32_t fence_fd);
+void dpy_gl_qemu_dmabuf_set_sync(QemuDmaBuf *dmabuf, void *sync);
+void dpy_gl_qemu_dmabuf_set_draw_submitted(QemuDmaBuf *dmabuf, bool
draw_submitted);
+void dpy_gl_qemu_dmabuf_set_fd(QemuDmaBuf *dmabuf, int32_t fd);
void dpy_gl_release_dmabuf(QemuConsole *con,
QemuDmaBuf *dmabuf);
void dpy_gl_update(QemuConsole *con,
diff --git a/ui/console.c b/ui/console.c
index 5d5635f783..d4ca9e6e0f 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1244,6 +1244,36 @@ bool dpy_gl_qemu_dmabuf_get_draw_submitted(QemuDmaBuf
*dmabuf)
return dmabuf->draw_submitted;
}
+void dpy_gl_qemu_dmabuf_set_texture(QemuDmaBuf *dmabuf, uint32_t texture)
+{
+assert(dmabuf != NULL);
+dmabuf->texture = texture;
+}
+
+void dpy_gl_qemu_dmabuf_set_fence_fd(QemuDmaBuf *dmabuf, int32_t fence_fd)
+{
+assert(dmabuf != NULL);
+dmabuf->fence_fd = fence_fd;
+}
+
+void dpy_gl_qemu_dmabuf_set_sync(QemuDmaBuf *dmabuf, void *sync)
+{
+assert(dmabuf != NULL);
+dmabuf->sync = sync;
+}
+
+void dpy_gl_qemu_dmabuf_set_draw_submitted(QemuDmaBuf *dmabuf, bool
draw_submitted)
+{
+assert(dmabuf != NULL);
+dmabuf->draw_submitted = draw_submitted;
+}
+
+void dpy_gl_qemu_dmabuf_set_fd(QemuDmaBuf *dmabuf, int32_t fd)
+{
+assert(dmabuf != NULL);
+dmabuf->fd = fd;
+}
+
void dpy_gl_release_dmabuf(QemuConsole *con,
QemuDmaBuf *dmabuf)
{
diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
index 86d64c68ce..c71a2878c2 100644
--- a/ui/egl-helpers.c
+++ b/ui/egl-helpers.c
@@ -348,8 +348,8 @@ void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf)
return;
}
-glGenTextures(1, &dmabuf->texture);
-texture = dpy_gl_qemu_dmabuf_get_texture(dmabuf);
+glGenTextures(1, &texture);
+dpy_gl_qemu_dmabuf_set_texture(dmabuf, texture);
glBindTexture(GL_TEXTURE_2D, texture);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
@@ -368,7 +368,7 @@ void egl_dmabuf_release_texture(QemuDmaBuf *dmabuf)
}
glDeleteTextures(1, &texture);
-dmabuf->texture = 0;
+dpy_gl_qemu_dmabuf_set_texture(dmabuf, 0);
}
void egl_dmabuf_create_sync(QemuDmaBuf *dmabuf)
@@ -382,7 +382,7 @@ void egl_dmabuf_create_sync(QemuDmaBuf *dmabuf)
sync = eglCreateSyncKHR(qemu_egl_display,
EGL_SYNC_NATIVE_FENCE_ANDROID, NULL);
if (sync != EGL_NO_SYNC_KHR) {
-dmabuf->sync = sync;
+dpy_gl_qemu_dmabuf_set_sync(dmabuf, sync);
}
}
}
@@ -390,12 +390,14 @@ void egl_dmabuf_create_sync(QemuDmaBuf *dmabuf)
void egl_dmabuf_create_fence(QemuDmaBuf *dmabuf)
{
void *sync = dpy_gl_qemu_dmabuf_get_sync(dmabuf);
+int fence_fd;
if (sync) {
-dmabuf->fence_fd = eglDupNativeFenceFDANDROID(qemu_egl_display,
- sync);
+fence_fd = eglDupNativeFenceFDANDROID(qemu_egl_display,
+ sync);
+dpy_gl_qemu_dmabuf_set_fence_fd(dmabuf, fence_fd);
eglDestroySyncKHR(qemu_egl_display, sync);
-dmabuf->sync = NULL;
+dpy_gl_qemu_dmabuf_set_sync(dmabuf, NULL);
}
}
diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index c9469af9ed..7494a34d7c 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -87,7 +87,7 @@ void gd_egl_draw(VirtualConsole *vc)
if (!dpy_gl_qemu_dmabuf_get_draw_submitted(dmabuf)) {
return;
} else {
-dmabuf->draw_submitted = false;
+dpy_gl_qemu_dmabuf_set_draw_submitted(dmabuf, false);
}
}
#endif
@@ -381,7 +381,7 @@ void gd_egl_flush(DisplayChangeList
[PATCH v5 1/3] ui/console: Introduce dpy_gl_qemu_dmabuf_get_..() helpers
From: Dongwon Kim
This commit introduces dpy_gl_qemu_dmabuf_get_... helpers to extract
specific fields from the QemuDmaBuf struct. It also updates all instances
where fields within the QemuDmaBuf struct are directly accessed, replacing
them with calls to these new helper functions.
Suggested-by: Marc-André Lureau
Cc: Philippe Mathieu-Daudé
Cc: Vivek Kasireddy
Signed-off-by: Dongwon Kim
---
include/ui/console.h| 17 +
hw/display/vhost-user-gpu.c | 6 +-
hw/display/virtio-gpu-udmabuf.c | 7 +-
hw/vfio/display.c | 15 +++--
ui/console.c| 116 +++-
ui/dbus-console.c | 9 ++-
ui/dbus-listener.c | 43 +++-
ui/egl-headless.c | 23 +--
ui/egl-helpers.c| 47 +++--
ui/gtk-egl.c| 48 -
ui/gtk-gl-area.c| 37 ++
ui/gtk.c| 6 +-
ui/spice-display.c | 50 --
13 files changed, 316 insertions(+), 108 deletions(-)
diff --git a/include/ui/console.h b/include/ui/console.h
index 0bc7a00ac0..6292943a82 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -358,6 +358,23 @@ void dpy_gl_cursor_dmabuf(QemuConsole *con, QemuDmaBuf
*dmabuf,
bool have_hot, uint32_t hot_x, uint32_t hot_y);
void dpy_gl_cursor_position(QemuConsole *con,
uint32_t pos_x, uint32_t pos_y);
+
+int32_t dpy_gl_qemu_dmabuf_get_fd(QemuDmaBuf *dmabuf);
+uint32_t dpy_gl_qemu_dmabuf_get_width(QemuDmaBuf *dmabuf);
+uint32_t dpy_gl_qemu_dmabuf_get_height(QemuDmaBuf *dmabuf);
+uint32_t dpy_gl_qemu_dmabuf_get_stride(QemuDmaBuf *dmabuf);
+uint32_t dpy_gl_qemu_dmabuf_get_fourcc(QemuDmaBuf *dmabuf);
+uint64_t dpy_gl_qemu_dmabuf_get_modifier(QemuDmaBuf *dmabuf);
+uint32_t dpy_gl_qemu_dmabuf_get_texture(QemuDmaBuf *dmabuf);
+uint32_t dpy_gl_qemu_dmabuf_get_x(QemuDmaBuf *dmabuf);
+uint32_t dpy_gl_qemu_dmabuf_get_y(QemuDmaBuf *dmabuf);
+uint32_t dpy_gl_qemu_dmabuf_get_backing_width(QemuDmaBuf *dmabuf);
+uint32_t dpy_gl_qemu_dmabuf_get_backing_height(QemuDmaBuf *dmabuf);
+bool dpy_gl_qemu_dmabuf_get_y0_top(QemuDmaBuf *dmabuf);
+void *dpy_gl_qemu_dmabuf_get_sync(QemuDmaBuf *dmabuf);
+int32_t dpy_gl_qemu_dmabuf_get_fence_fd(QemuDmaBuf *dmabuf);
+bool dpy_gl_qemu_dmabuf_get_allow_fences(QemuDmaBuf *dmabuf);
+bool dpy_gl_qemu_dmabuf_get_draw_submitted(QemuDmaBuf *dmabuf);
void dpy_gl_release_dmabuf(QemuConsole *con,
QemuDmaBuf *dmabuf);
void dpy_gl_update(QemuConsole *con,
diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 709c8a02a1..87dcfbca10 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -249,6 +249,7 @@ vhost_user_gpu_handle_display(VhostUserGPU *g,
VhostUserGpuMsg *msg)
case VHOST_USER_GPU_DMABUF_SCANOUT: {
VhostUserGpuDMABUFScanout *m = &msg->payload.dmabuf_scanout;
int fd = qemu_chr_fe_get_msgfd(&g->vhost_chr);
+int old_fd;
QemuDmaBuf *dmabuf;
if (m->scanout_id >= g->parent_obj.conf.max_outputs) {
@@ -262,8 +263,9 @@ vhost_user_gpu_handle_display(VhostUserGPU *g,
VhostUserGpuMsg *msg)
g->parent_obj.enable = 1;
con = g->parent_obj.scanout[m->scanout_id].con;
dmabuf = &g->dmabuf[m->scanout_id];
-if (dmabuf->fd >= 0) {
-close(dmabuf->fd);
+old_fd = dpy_gl_qemu_dmabuf_get_fd(dmabuf);
+if (old_fd >= 0) {
+close(old_fd);
dmabuf->fd = -1;
}
dpy_gl_release_dmabuf(con, dmabuf);
diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
index d51184d658..e3f358b575 100644
--- a/hw/display/virtio-gpu-udmabuf.c
+++ b/hw/display/virtio-gpu-udmabuf.c
@@ -206,6 +206,7 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,
{
struct virtio_gpu_scanout *scanout = &g->parent_obj.scanout[scanout_id];
VGPUDMABuf *new_primary, *old_primary = NULL;
+uint32_t width, height;
new_primary = virtio_gpu_create_dmabuf(g, scanout_id, res, fb, r);
if (!new_primary) {
@@ -216,10 +217,10 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,
old_primary = g->dmabuf.primary[scanout_id];
}
+width = dpy_gl_qemu_dmabuf_get_width(&new_primary->buf);
+height = dpy_gl_qemu_dmabuf_get_height(&new_primary->buf);
g->dmabuf.primary[scanout_id] = new_primary;
-qemu_console_resize(scanout->con,
-new_primary->buf.width,
-new_primary->buf.height);
+qemu_console_resize(scanout->con, width, height);
dpy_gl_scanout_dmabuf(scanout->con, &new_primary->buf);
if (old_primary) {
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index 1aa440c663..f9c39cbd51 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -259,9 +259,13 @@ static VFIODMABuf *vfio_display_get_dmabuf(VFIOPCIDevice
*vdev,
static void vfio
Re: [PATCH v5] virtio-pci: Fix the crash that the vector was used after released.
On Thu, Apr 11, 2024 at 4:03 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)
>
> Fixes: f9a09ca3ea ("vhost: add support for configure interrupt")
> Cc: qemu-sta...@nongnu.org
>
> Signed-off-by: Cindy Lu
Acked-by: Jason Wang
Thanks
> ---
> hw/virtio/virtio-pci.c | 29 +
> 1 file changed, 29 insertions(+)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 1a7039fb0c..4f2663b27d 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1423,6 +1423,24 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy
> *proxy,
>
> return offset
Re: [PATCH] target/sparc: resolve ASI_USERTXT correctly
On 4/11/24 18:15, M Bazz wrote:
On Thu, Apr 11, 2024, 5:55 PM Richard Henderson
wrote:
On 4/11/24 14:29, M Bazz wrote:
fixes a longstanding bug which causes a "Nonparity Synchronous Error"
kernel panic while using a debugger on Solaris / SunOS systems. The panic
would occur on the first attempt to single-step the process.
The problem stems from an lda instruction on ASI_USERTXT (8). This asi
was not being resolved correctly by resolve_asi().
Further details can be found in #2281
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2281
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2059
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1609
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1166
Signed-off-by: M Bazz
---
target/sparc/translate.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 319934d9bd..1596005e22 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -3,6 +3,7 @@
Copyright (C) 2003 Thomas M. Ogrisegg
Copyright (C) 2003-2005 Fabrice Bellard
+ Copyright (C) 2024 M Bazz
This library is free software; you can redistribute it and/or
modify it under the terms of the GNU Lesser General Public
@@ -1159,6 +1160,7 @@ static DisasASI resolve_asi(DisasContext *dc, int asi,
MemOp memop)
|| (asi == ASI_USERDATA
&& (dc->def->features & CPU_FEATURE_CASA))) {
switch (asi) {
+case ASI_USERTXT:/* User text access */
case ASI_USERDATA: /* User data access */
mem_idx = MMU_USER_IDX;
type = GET_ASI_DIRECT;
I don't believe this is correct, because it operates against the page's "read"
permissions
instead of "execute" permissions.
r~
Hi Richard,
Thanks for your guidance. It set me in the right direction. Now I
think I've got the right spot.
function `helper_ld_asi` has a block to help load ASI_KERNELTXT, but the
ASI_USERTXT case defaults to sparc_raise_mmu_fault(); I believe this
is the true culprit
source reference:
https://gitlab.com/qemu-project/qemu/-/blob/master/target/sparc/ldst_helper.c?ref_type=heads#L687
The code for the ASI_KERNELTXT seems generic enough to also use for
ASI_USERTXT verbatim.
See v2 patch below. I've done a `make test` -- all passing (3 skips).
OS boots, and the
debuggers are working without issue. What do you think?
Once we arrive at the right solution, I'll finalize the patch.
-bazz
diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c
index e581bb42ac..4f87e44a93 100644
--- a/target/sparc/ldst_helper.c
+++ b/target/sparc/ldst_helper.c
@@ -684,6 +684,7 @@ uint64_t helper_ld_asi(CPUSPARCState *env,
target_ulong addr,
case ASI_M_DIAGS: /* Turbosparc DTLB Diagnostic */
case ASI_M_IODIAG: /* Turbosparc IOTLB Diagnostic */
break;
+case ASI_USERTXT: /* User code access */
case ASI_KERNELTXT: /* Supervisor code access */
oi = make_memop_idx(memop, cpu_mmu_index(env_cpu(env), true));
No, this also does not work, because it uses the wrong permissions (kernel instead of
user). I have just sent a patch to fix both problems.
r~
[PATCH] target/sparc: Use GET_ASI_CODE for ASI_KERNELTXT and ASI_USERTXT
Reads are done with execute access. It is not clear whether writes
are legal at all -- for now, leave helper_st_asi unchanged, so that
we continue to raise an mmu fault.
This generalizes the exiting code for ASI_KERNELTXT to be usable for
ASI_USERTXT as well, by passing down the MemOpIdx to use.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2281
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2059
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1609
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1166
Signed-off-by: Richard Henderson
---
target/sparc/helper.h | 3 ++
target/sparc/ldst_helper.c | 65 ++
target/sparc/translate.c | 49 ++--
3 files changed, 95 insertions(+), 22 deletions(-)
diff --git a/target/sparc/helper.h b/target/sparc/helper.h
index e55fad5b8c..b8087d0d2b 100644
--- a/target/sparc/helper.h
+++ b/target/sparc/helper.h
@@ -32,6 +32,9 @@ DEF_HELPER_FLAGS_3(udiv, TCG_CALL_NO_WG, i64, env, tl, tl)
DEF_HELPER_FLAGS_3(sdiv, TCG_CALL_NO_WG, i64, env, tl, tl)
DEF_HELPER_3(taddcctv, tl, env, tl, tl)
DEF_HELPER_3(tsubcctv, tl, env, tl, tl)
+#if !defined(CONFIG_USER_ONLY) && !defined(TARGET_SPARC64)
+DEF_HELPER_FLAGS_3(ld_code, TCG_CALL_NO_WG, i64, env, tl, i32)
+#endif
#if !defined(CONFIG_USER_ONLY) || defined(TARGET_SPARC64)
DEF_HELPER_FLAGS_4(ld_asi, TCG_CALL_NO_WG, i64, env, tl, int, i32)
DEF_HELPER_FLAGS_5(st_asi, TCG_CALL_NO_WG, void, env, tl, i64, int, i32)
diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c
index e581bb42ac..2846a86cc4 100644
--- a/target/sparc/ldst_helper.c
+++ b/target/sparc/ldst_helper.c
@@ -585,7 +585,6 @@ uint64_t helper_ld_asi(CPUSPARCState *env, target_ulong
addr,
#if defined(DEBUG_MXCC) || defined(DEBUG_ASI)
uint32_t last_addr = addr;
#endif
-MemOpIdx oi;
do_check_align(env, addr, size - 1, GETPC());
switch (asi) {
@@ -684,24 +683,6 @@ uint64_t helper_ld_asi(CPUSPARCState *env, target_ulong
addr,
case ASI_M_DIAGS: /* Turbosparc DTLB Diagnostic */
case ASI_M_IODIAG: /* Turbosparc IOTLB Diagnostic */
break;
-case ASI_KERNELTXT: /* Supervisor code access */
-oi = make_memop_idx(memop, cpu_mmu_index(env_cpu(env), true));
-switch (size) {
-case 1:
-ret = cpu_ldb_code_mmu(env, addr, oi, GETPC());
-break;
-case 2:
-ret = cpu_ldw_code_mmu(env, addr, oi, GETPC());
-break;
-default:
-case 4:
-ret = cpu_ldl_code_mmu(env, addr, oi, GETPC());
-break;
-case 8:
-ret = cpu_ldq_code_mmu(env, addr, oi, GETPC());
-break;
-}
-break;
case ASI_M_TXTC_TAG: /* SparcStation 5 I-cache tag */
case ASI_M_TXTC_DATA: /* SparcStation 5 I-cache data */
case ASI_M_DATAC_TAG: /* SparcStation 5 D-cache tag */
@@ -779,7 +760,6 @@ uint64_t helper_ld_asi(CPUSPARCState *env, target_ulong
addr,
case 0x4c: /* SuperSPARC MMU Breakpoint Action */
ret = env->mmubpaction;
break;
-case ASI_USERTXT: /* User code access, XXX */
default:
sparc_raise_mmu_fault(cs, addr, false, false, asi, size, GETPC());
ret = 0;
@@ -787,6 +767,8 @@ uint64_t helper_ld_asi(CPUSPARCState *env, target_ulong
addr,
case ASI_USERDATA: /* User data access */
case ASI_KERNELDATA: /* Supervisor data access */
+case ASI_USERTXT: /* User code access */
+case ASI_KERNELTXT: /* Supervisor code access */
case ASI_P: /* Implicit primary context data access (v9 only?) */
case ASI_M_BYPASS:/* MMU passthrough */
case ASI_LEON_BYPASS: /* LEON MMU passthrough */
@@ -1161,6 +1143,49 @@ void helper_st_asi(CPUSPARCState *env, target_ulong
addr, uint64_t val,
#endif
}
+uint64_t helper_ld_code(CPUSPARCState *env, target_ulong addr, uint32_t oi)
+{
+MemOp mop = get_memop(oi);
+uintptr_t ra = GETPC();
+uint64_t ret;
+
+switch (mop & MO_SIZE) {
+case MO_8:
+ret = cpu_ldb_code_mmu(env, addr, oi, ra);
+if (mop & MO_SIGN) {
+ret = (int8_t)ret;
+}
+break;
+case MO_16:
+ret = cpu_ldw_code_mmu(env, addr, oi, ra);
+if ((mop & MO_BSWAP) != MO_TE) {
+ret = bswap16(ret);
+}
+if (mop & MO_SIGN) {
+ret = (int16_t)ret;
+}
+break;
+case MO_32:
+ret = cpu_ldl_code_mmu(env, addr, oi, ra);
+if ((mop & MO_BSWAP) != MO_TE) {
+ret = bswap32(ret);
+}
+if (mop & MO_SIGN) {
+ret = (int32_t)ret;
+}
+break;
+case MO_64:
+ret = cpu_ldq_code_mmu(env, addr, oi, ra);
+if ((mop & MO_BSWAP) != MO_TE) {
+ret = bswap64(ret);
+}
+break;
+default:
+g_assert_not_reached();
+}
+return ret;
+}
+
#endif /* CONFIG_USER_ONLY */
#else /* TARGET_SPARC64 */
diff --
Re: [PATCH] target/sparc: resolve ASI_USERTXT correctly
On Thu, Apr 11, 2024, 5:55 PM Richard Henderson
wrote:
>
> On 4/11/24 14:29, M Bazz wrote:
> > fixes a longstanding bug which causes a "Nonparity Synchronous Error"
> > kernel panic while using a debugger on Solaris / SunOS systems. The panic
> > would occur on the first attempt to single-step the process.
> >
> > The problem stems from an lda instruction on ASI_USERTXT (8). This asi
> > was not being resolved correctly by resolve_asi().
> >
> > Further details can be found in #2281
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2281
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2059
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1609
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1166
> >
> > Signed-off-by: M Bazz
> > ---
> > target/sparc/translate.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/target/sparc/translate.c b/target/sparc/translate.c
> > index 319934d9bd..1596005e22 100644
> > --- a/target/sparc/translate.c
> > +++ b/target/sparc/translate.c
> > @@ -3,6 +3,7 @@
> >
> > Copyright (C) 2003 Thomas M. Ogrisegg
> > Copyright (C) 2003-2005 Fabrice Bellard
> > + Copyright (C) 2024 M Bazz
> >
> > This library is free software; you can redistribute it and/or
> > modify it under the terms of the GNU Lesser General Public
> > @@ -1159,6 +1160,7 @@ static DisasASI resolve_asi(DisasContext *dc, int
> > asi, MemOp memop)
> > || (asi == ASI_USERDATA
> > && (dc->def->features & CPU_FEATURE_CASA))) {
> > switch (asi) {
> > +case ASI_USERTXT:/* User text access */
> > case ASI_USERDATA: /* User data access */
> > mem_idx = MMU_USER_IDX;
> > type = GET_ASI_DIRECT;
>
> I don't believe this is correct, because it operates against the page's
> "read" permissions
> instead of "execute" permissions.
>
>
> r~
Hi Richard,
Thanks for your guidance. It set me in the right direction. Now I
think I've got the right spot.
function `helper_ld_asi` has a block to help load ASI_KERNELTXT, but the
ASI_USERTXT case defaults to sparc_raise_mmu_fault(); I believe this
is the true culprit
source reference:
https://gitlab.com/qemu-project/qemu/-/blob/master/target/sparc/ldst_helper.c?ref_type=heads#L687
The code for the ASI_KERNELTXT seems generic enough to also use for
ASI_USERTXT verbatim.
See v2 patch below. I've done a `make test` -- all passing (3 skips).
OS boots, and the
debuggers are working without issue. What do you think?
Once we arrive at the right solution, I'll finalize the patch.
-bazz
diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c
index e581bb42ac..4f87e44a93 100644
--- a/target/sparc/ldst_helper.c
+++ b/target/sparc/ldst_helper.c
@@ -684,6 +684,7 @@ uint64_t helper_ld_asi(CPUSPARCState *env,
target_ulong addr,
case ASI_M_DIAGS: /* Turbosparc DTLB Diagnostic */
case ASI_M_IODIAG: /* Turbosparc IOTLB Diagnostic */
break;
+case ASI_USERTXT: /* User code access */
case ASI_KERNELTXT: /* Supervisor code access */
oi = make_memop_idx(memop, cpu_mmu_index(env_cpu(env), true));
switch (size) {
@@ -779,7 +780,6 @@ uint64_t helper_ld_asi(CPUSPARCState *env,
target_ulong addr,
case 0x4c: /* SuperSPARC MMU Breakpoint Action */
ret = env->mmubpaction;
break;
-case ASI_USERTXT: /* User code access, XXX */
default:
sparc_raise_mmu_fault(cs, addr, false, false, asi, size, GETPC());
ret = 0;
Re: wiki.qemu.org account access request
On Thu, Apr 11, 2024 at 5:06 PM Zack Buhman wrote: > > I noticed the recent SH4 patches are included in the 9.0.0-rc3 release. > > Is it appropriate that I request a wiki.qemu.org account so that I may > document these changes in https://wiki.qemu.org/ChangeLog/9.0 in a manner > that is consistent with how the changes to other CPUs have been documented so > far? > > If so, I indeed desire such an account. I've created an account ZackBuhman for you and sent the password off-list. -- Thanks. -- Max
wiki.qemu.org account access request
I noticed the recent SH4 patches are included in the 9.0.0-rc3 release. Is it appropriate that I request a wiki.qemu.org account so that I may document these changes in https://wiki.qemu.org/ChangeLog/9.0 in a manner that is consistent with how the changes to other CPUs have been documented so far? If so, I indeed desire such an account.
Re: [PATCH 56/60] linux-user/x86_64: Handle the vsyscall page in open_self_maps_{2, 4}
On 3/2/24 02:06, Richard Henderson wrote:
This is the only case in which we expect to have no host memory backing
for a guest memory page, because in general linux user processes cannot
map any pages in the top half of the 64-bit address space.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2170
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Richard Henderson
This change fixes a real bug reported against debian qemu package,
https://bugs.debian.org/1068740 .
It looks like it should be picked up for stable-8.2. And it looks
like I missed this one when it's been applied to master - usually
I take a look at all stuff going to master and pick fixes from there,
but not this time. Is there anything else from this series which
needs to go to stable too?
Thanks,
/mjt
linux-user/syscall.c | 16
1 file changed, 16 insertions(+)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e384e14248..bc8c06522f 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7994,6 +7994,10 @@ static void open_self_maps_4(const struct
open_self_maps_data *d,
path = "[heap]";
} else if (start == info->vdso) {
path = "[vdso]";
+#ifdef TARGET_X86_64
+} else if (start == TARGET_VSYSCALL_PAGE) {
+path = "[vsyscall]";
+#endif
}
/* Except null device (MAP_ANON), adjust offset for this fragment. */
@@ -8082,6 +8086,18 @@ static int open_self_maps_2(void *opaque, target_ulong
guest_start,
uintptr_t host_start = (uintptr_t)g2h_untagged(guest_start);
uintptr_t host_last = (uintptr_t)g2h_untagged(guest_end - 1);
+#ifdef TARGET_X86_64
+/*
+ * Because of the extremely high position of the page within the guest
+ * virtual address space, this is not backed by host memory at all.
+ * Therefore the loop below would fail. This is the only instance
+ * of not having host backing memory.
+ */
+if (guest_start == TARGET_VSYSCALL_PAGE) {
+return open_self_maps_3(opaque, guest_start, guest_end, flags);
+}
+#endif
+
while (1) {
IntervalTreeNode *n =
interval_tree_iter_first(d->host_maps, host_start, host_start);
Re: [PATCH 2/2] target/m68k: Remove sprintf() calls
On 4/11/24 14:39, Philippe Mathieu-Daudé wrote: sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1, resulting in painful developper experience. Since they are very few register names, use const arrays instead of trying to be clever generating the names. This silences: [2/8] Compiling C object libqemu-m68k-softmmu.fa.p/target_m68k_translate.c.o target/m68k/translate.c:92:9: 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(p, "D%d", i); ^ sprintf(p, "A%d", i); ^ sprintf(p, "ACC%d", i); ^ Signed-off-by: Philippe Mathieu-Daudé --- target/m68k/translate.c | 27 +++ 1 file changed, 15 insertions(+), 12 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH 1/2] disas/m68k: Replace sprintf() by snprintf()
On 4/11/24 14:39, Philippe Mathieu-Daudé wrote:
@@ -974,7 +974,7 @@ print_base (int regno, bfd_vma disp, disassemble_info *info)
else
(*info->fprintf_func) (info->stream, "%s@(", reg_names[regno]);
- sprintf_vma (buf, disp);
+ snprintf(buf, sizeof(buf), "%0" PRIx64, disp);
(*info->fprintf_func) (info->stream, "%s", buf);
}
}
@@ -1069,7 +1069,7 @@ print_indexed (int basereg,
(*info->fprintf_func) (info->stream, ",%s", buf);
buf[0] = '\0';
}
- sprintf_vma (vmabuf, outer_disp);
+ snprintf(vmabuf, sizeof(vmabuf), "%0" PRIx64, outer_disp);
(*info->fprintf_func) (info->stream, ")@(%s", vmabuf);
if (buf[0] != '\0')
(*info->fprintf_func) (info->stream, ",%s", buf);
In both cases, there's no need for the sprintf at all.
Fold everything into the adjacent fprintf.
r~
Re: [PATCH] target/sparc: resolve ASI_USERTXT correctly
On 4/11/24 14:29, M Bazz wrote:
fixes a longstanding bug which causes a "Nonparity Synchronous Error"
kernel panic while using a debugger on Solaris / SunOS systems. The panic
would occur on the first attempt to single-step the process.
The problem stems from an lda instruction on ASI_USERTXT (8). This asi
was not being resolved correctly by resolve_asi().
Further details can be found in #2281
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2281
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2059
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1609
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1166
Signed-off-by: M Bazz
---
target/sparc/translate.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 319934d9bd..1596005e22 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -3,6 +3,7 @@
Copyright (C) 2003 Thomas M. Ogrisegg
Copyright (C) 2003-2005 Fabrice Bellard
+ Copyright (C) 2024 M Bazz
This library is free software; you can redistribute it and/or
modify it under the terms of the GNU Lesser General Public
@@ -1159,6 +1160,7 @@ static DisasASI resolve_asi(DisasContext *dc, int asi,
MemOp memop)
|| (asi == ASI_USERDATA
&& (dc->def->features & CPU_FEATURE_CASA))) {
switch (asi) {
+case ASI_USERTXT:/* User text access */
case ASI_USERDATA: /* User data access */
mem_idx = MMU_USER_IDX;
type = GET_ASI_DIRECT;
I don't believe this is correct, because it operates against the page's "read" permissions
instead of "execute" permissions.
r~
Re: [PATCH 9/9] target/i386: Replace sprintf() by snprintf()
On 4/11/24 03:43, Philippe Mathieu-Daudé wrote:
sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience. Use snprintf() instead.
Signed-off-by: Philippe Mathieu-Daudé
---
target/i386/kvm/kvm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index e68cbe9293..a46d1426bf 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -5335,7 +5335,8 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run
*run)
case KVM_EXIT_NOTIFY:
ctx_invalid = !!(run->notify.flags & KVM_NOTIFY_CONTEXT_INVALID);
state = KVM_STATE(current_accel());
-sprintf(str, "Encounter a notify exit with %svalid context in"
+snprintf(str, sizeof(str),
+ "Encounter a notify exit with %svalid context in"
" guest. There can be possible misbehaves in guest."
" Please have a look.", ctx_invalid ? "in" : "");
if (ctx_invalid ||
In the larger context,
if (ctx_invalid ||
state->notify_vmexit == NOTIFY_VMEXIT_OPTION_INTERNAL_ERROR) {
warn_report("KVM internal error: %s", str);
ret = -1;
} else {
warn_report_once("KVM: %s", str);
ret = 0;
}
so there's really no need to sprintf into a buffer at all -- just pass it all to
warn_report_*.
The English text could use some improvement as well. :-)
r~
[PATCH 2/2] target/m68k: Remove sprintf() calls
sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.
Since they are very few register names, use const arrays instead
of trying to be clever generating the names. This silences:
[2/8] Compiling C object libqemu-m68k-softmmu.fa.p/target_m68k_translate.c.o
target/m68k/translate.c:92:9: 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(p, "D%d", i);
^
sprintf(p, "A%d", i);
^
sprintf(p, "ACC%d", i);
^
Signed-off-by: Philippe Mathieu-Daudé
---
target/m68k/translate.c | 27 +++
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 8a194f2f21..d0561c18fe 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -48,7 +48,6 @@
static TCGv_i32 cpu_halted;
static TCGv_i32 cpu_exception_index;
-static char cpu_reg_names[2 * 8 * 3 + 5 * 4];
static TCGv cpu_dregs[8];
static TCGv cpu_aregs[8];
static TCGv_i64 cpu_macc[4];
@@ -66,7 +65,15 @@ static TCGv store_dummy;
void m68k_tcg_init(void)
{
-char *p;
+static const char dreg_names[8][3] = {
+"D0", "D1", "D2", "D3", "D4", "D5", "D6", "D7"
+};
+static const char areg_names[8][3] = {
+"A0", "A1", "A2", "A3", "A4", "A5", "A6", "A7"
+};
+static const char macc_names[4][5] = {
+"ACC0", "ACC1", "ACC2", "ACC3"
+};
int i;
#define DEFO32(name, offset) \
@@ -87,22 +94,18 @@ void m68k_tcg_init(void)
offsetof(CPUState,
exception_index),
"EXCEPTION");
-p = cpu_reg_names;
for (i = 0; i < 8; i++) {
-sprintf(p, "D%d", i);
cpu_dregs[i] = tcg_global_mem_new(tcg_env,
- offsetof(CPUM68KState, dregs[i]), p);
-p += 3;
-sprintf(p, "A%d", i);
+ offsetof(CPUM68KState, dregs[i]),
+ dreg_names[i]);
cpu_aregs[i] = tcg_global_mem_new(tcg_env,
- offsetof(CPUM68KState, aregs[i]), p);
-p += 3;
+ offsetof(CPUM68KState, aregs[i]),
+ areg_names[i]);
}
for (i = 0; i < 4; i++) {
-sprintf(p, "ACC%d", i);
cpu_macc[i] = tcg_global_mem_new_i64(tcg_env,
- offsetof(CPUM68KState, macc[i]), p);
-p += 5;
+ offsetof(CPUM68KState, macc[i]),
+ macc_names[i]);
}
NULL_QREG = tcg_global_mem_new(tcg_env, -4, "NULL");
--
2.41.0
[PATCH 1/2] disas/m68k: Replace sprintf() by snprintf()
sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.
Inline sprintf_vma() and use snprintf() instead of sprintf(),
silencing the following warning:
[38/244] Compiling C object libcommon.fa.p/disas_m68k.c.o
disas/m68k.c:977:7: 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_vma (buf, disp);
^
Signed-off-by: Philippe Mathieu-Daudé
---
include/disas/dis-asm.h | 2 --
disas/m68k.c| 4 ++--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/include/disas/dis-asm.h b/include/disas/dis-asm.h
index b26867b641..1d8a4ce9a1 100644
--- a/include/disas/dis-asm.h
+++ b/include/disas/dis-asm.h
@@ -15,8 +15,6 @@ typedef void *PTR;
typedef uint64_t bfd_vma;
typedef int64_t bfd_signed_vma;
typedef uint8_t bfd_byte;
-#define sprintf_vma(s,x) sprintf (s, "%0" PRIx64, x)
-#define snprintf_vma(s,ss,x) snprintf (s, ss, "%0" PRIx64, x)
#define BFD64
diff --git a/disas/m68k.c b/disas/m68k.c
index 800b4145ac..e8e61c7a4e 100644
--- a/disas/m68k.c
+++ b/disas/m68k.c
@@ -974,7 +974,7 @@ print_base (int regno, bfd_vma disp, disassemble_info *info)
else
(*info->fprintf_func) (info->stream, "%s@(", reg_names[regno]);
- sprintf_vma (buf, disp);
+ snprintf(buf, sizeof(buf), "%0" PRIx64, disp);
(*info->fprintf_func) (info->stream, "%s", buf);
}
}
@@ -1069,7 +1069,7 @@ print_indexed (int basereg,
(*info->fprintf_func) (info->stream, ",%s", buf);
buf[0] = '\0';
}
- sprintf_vma (vmabuf, outer_disp);
+ snprintf(vmabuf, sizeof(vmabuf), "%0" PRIx64, outer_disp);
(*info->fprintf_func) (info->stream, ")@(%s", vmabuf);
if (buf[0] != '\0')
(*info->fprintf_func) (info->stream, ",%s", buf);
--
2.41.0
[PATCH 0/2] m68k: Remove sprintf() calls due to macOS deprecation
Continuation of: https://lore.kernel.org/qemu-devel/20240411101550.99392-1-phi...@linaro.org/ Philippe Mathieu-Daudé (2): disas/m68k: Replace sprintf() by snprintf() target/m68k: Remove sprintf() calls include/disas/dis-asm.h | 2 -- disas/m68k.c| 4 ++-- target/m68k/translate.c | 27 +++ 3 files changed, 17 insertions(+), 16 deletions(-) -- 2.41.0
[PATCH] target/sparc: resolve ASI_USERTXT correctly
fixes a longstanding bug which causes a "Nonparity Synchronous Error"
kernel panic while using a debugger on Solaris / SunOS systems. The panic
would occur on the first attempt to single-step the process.
The problem stems from an lda instruction on ASI_USERTXT (8). This asi
was not being resolved correctly by resolve_asi().
Further details can be found in #2281
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2281
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2059
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1609
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1166
Signed-off-by: M Bazz
---
target/sparc/translate.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 319934d9bd..1596005e22 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -3,6 +3,7 @@
Copyright (C) 2003 Thomas M. Ogrisegg
Copyright (C) 2003-2005 Fabrice Bellard
+ Copyright (C) 2024 M Bazz
This library is free software; you can redistribute it and/or
modify it under the terms of the GNU Lesser General Public
@@ -1159,6 +1160,7 @@ static DisasASI resolve_asi(DisasContext *dc, int asi,
MemOp memop)
|| (asi == ASI_USERDATA
&& (dc->def->features & CPU_FEATURE_CASA))) {
switch (asi) {
+case ASI_USERTXT:/* User text access */
case ASI_USERDATA: /* User data access */
mem_idx = MMU_USER_IDX;
type = GET_ASI_DIRECT;
--
2.43.0
Re: [PATCH 8/9] target/arm: Replace sprintf() by snprintf()
On 4/11/24 03:43, Philippe Mathieu-Daudé wrote: sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1, resulting in painful developper experience. Use snprintf() instead. Signed-off-by: Philippe Mathieu-Daudé --- target/arm/cpu64.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH 7/9] hw/riscv/virt: Replace sprintf() by snprintf()
On 4/11/24 03:43, Philippe Mathieu-Daudé wrote: sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1, resulting in painful developper experience. Use snprintf() instead. Signed-off-by: Philippe Mathieu-Daudé --- hw/riscv/virt.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index d171e74f7b..b3fede1207 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -1619,7 +1619,7 @@ static char *virt_get_aia_guests(Object *obj, Error **errp) RISCVVirtState *s = RISCV_VIRT_MACHINE(obj); char val[32]; -sprintf(val, "%d", s->aia_guests); +snprintf(val, sizeof(val), "%d", s->aia_guests); return g_strdup(val); } g_strdup_printf. @@ -1785,7 +1785,8 @@ static void virt_machine_class_init(ObjectClass *oc, void *data) object_class_property_add_str(oc, "aia-guests", virt_get_aia_guests, virt_set_aia_guests); -sprintf(str, "Set number of guest MMIO pages for AIA IMSIC. Valid value " +snprintf(str, sizeof(str), + "Set number of guest MMIO pages for AIA IMSIC. Valid value " "should be between 0 and %d.", VIRT_IRQCHIP_MAX_GUESTS); object_class_property_set_description(oc, "aia-guests", str); object_class_property_add(oc, "acpi", "OnOffAuto", Ok. r~
Re: [PATCH 6/9] hw/net/rocker: Replace sprintf() by snprintf()
On 11/4/24 13:30, Peter Maydell wrote:
On Thu, 11 Apr 2024 at 11:47, Philippe Mathieu-Daudé wrote:
sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience. Use snprintf() instead.
("developer")
Signed-off-by: Philippe Mathieu-Daudé
---
hw/net/rocker/rocker.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
switch (offset) {
case ROCKER_DMA_DESC_ADDR_OFFSET:
-sprintf(buf, "Ring[%s] ADDR", ring_name);
+snprintf(buf, sizeofbuf), "Ring[%s] ADDR", ring_name);
Something seems to have gone wrong here. Shouldn't this have
failed to compile ?
This code is guarded by DEBUG_ROCKER, which is why I didn't noticed :)
Indeed when enabling:
../../hw/net/rocker/rocker.c:930:65: error: extraneous ')' before ';'
snprintf(buf, sizeofbuf), "Ring[%s] ADDR", ring_name);
^
Re: [PATCH 3/9] disas/riscv: Replace sprintf() by snprintf()
On 4/11/24 03:43, Philippe Mathieu-Daudé wrote: sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1, resulting in painful developper experience. Use snprintf() instead. Signed-off-by: Philippe Mathieu-Daudé --- disas/riscv.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) Ug. Repeated strlen+strncat? All of format_inst should be rewritten, probably using GString. r~
Re: [PATCH 2/9] disas/microblaze: Replace sprintf() by snprintf()
On 4/11/24 03:43, Philippe Mathieu-Daudé wrote:
sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience. Use snprintf() instead.
Signed-off-by: Philippe Mathieu-Daudé
---
disas/microblaze.c | 25 -
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/disas/microblaze.c b/disas/microblaze.c
index 0b89b9c4fa..49a4c0fd40 100644
--- a/disas/microblaze.c
+++ b/disas/microblaze.c
@@ -600,7 +600,8 @@ static char *
get_field (long instr, long mask, unsigned short low)
{
char tmpstr[25];
- sprintf(tmpstr, "%s%d", register_prefix, (int)((instr & mask) >> low));
+ snprintf(tmpstr, sizeof(tmpstr), "%s%d", register_prefix,
+ (int)((instr & mask) >> low));
return(strdup(tmpstr));
Any s*printf followed by allocation should use g_strdup_printf to do it in one
step.
r~
Re: [PATCH 1/9] disas/m68k: Replace sprintf() by snprintf()
On 4/11/24 03:43, Philippe Mathieu-Daudé wrote: sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1, resulting in painful developper experience. Use snprintf() instead. Signed-off-by: Philippe Mathieu-Daudé --- disas/m68k.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Richard Henderson r~
Re: [PATCH v2 10/13] hw/scsi/scsi-disk: Use qemu_hexdump_line() to avoid sprintf()
On 4/11/24 03:15, 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:
[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);
This is adding ": " as an unnecessary prefix, because it's added by
qemu_hexdump_line.
I think having qemu_hexdump_line as a primitive is good, but probably the offset argument
should be dropped and printed by the two callers that need it (mostly qemu_hexdump).
r~
QEMU Community Call Agenda Items (April 16th, 2024)
Hi, The KVM/QEMU community call is at: https://meet.jit.si/kvmcallmeeting @ 16/4/2024 14:00 UTC Are there any agenda items for the sync-up? Alex maintains the invite on our Linaro project calendar here: https://calendar.google.com/calendar/event?action=TEMPLATE&tmeid=MWd2dWI5NDM1bzdocnJlbTBhMHJhbG5sNWlfMjAyNDAyMjBUMTQwMDAwWiBjX2s1cDJscGd2YnB0ZGlya3U1c2kwMWJsbW5rQGc&tmsrc=c_k5p2lpgvbptdirku5si01blmnk%40group.calendar.google.com&scp=ALL If you want to be added to the invite list let him know and you can get spammed by your calendar app as well 😉 Regards, Phil.
Re: [PATCH v2 09/13] util/hexdump: Replace sprintf() by g_string_append_printf()
On 4/11/24 13:43, Philippe Mathieu-Daudé wrote:
On 11/4/24 12:15, Philippe Mathieu-Daudé wrote:
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]);
I find using g_string_append_printf() simpler than checking snprintf()
return value, and don't expect this function to be in hot path, but if
preferred I can try to not use the GString API.
GString api is pretty good.
Reviewed-by: Richard Henderson
r~
Re: [PATCH v2 08/13] util/hexdump: Have qemu_hexdump_line() return heap allocated buffer
On 4/11/24 03:15, Philippe Mathieu-Daudé wrote:
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);
}
}
Not especially efficient, re-allocating for each line.
How about
GString *qemu_hexdump_line(GString *str, buf, offset, len, ascii)
{
if (str) {
g_string_truncate(str, 0);
} else {
str = g_string_sized_new(QEMU_HEXDUMP_LINE_LEN);
}
...
return str;
}
void qemu_hexdump(FILE *fp, ...)
{
g_autoptr(GString) str = g_string_sized_new(QEMU_HEXDUMP_LINE_LEN);
for (...) {
qemu_hexdump_line(str, ...);
fprintf(fp, "%s: %s\n", prefix, str->str);
}
}
So that we reuse the one allocation across the whole loop.
r~
Re: [PATCH v2 09/13] util/hexdump: Replace sprintf() by g_string_append_printf()
On 11/4/24 12:15, Philippe Mathieu-Daudé wrote:
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]);
I find using g_string_append_printf() simpler than checking snprintf()
return value, and don't expect this function to be in hot path, but if
preferred I can try to not use the GString API.
} else {
-line += sprintf(line, " ");
+g_string_append(gs, " ");
}
}
Re: [PATCH v2 07/13] util/hexdump: Rename @offset argument in qemu_hexdump_line()
On 4/11/24 03:15, Philippe Mathieu-Daudé wrote: @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(-) Reviewed-by: Richard Henderson r~
Re: [PATCH v2 04/13] hw/mips/malta: Add re-usable rng_seed_hex_new() method
On 4/11/24 13:07, Richard Henderson wrote:
On 4/11/24 03:15, Philippe Mathieu-Daudé wrote:
Extract common code from reinitialize_rng_seed() and
load_kernel() to rng_seed_hex_new().
Signed-off-by: Philippe Mathieu-Daudé
---
hw/mips/malta.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index af74008c82..9fc6a7d313 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 *rng_seed_hex_new(void)
{
- char *rng_seed_hex = opaque;
uint8_t rng_seed[32];
+ char rng_seed_hex[sizeof(rng_seed) * 2 + 1];
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]);
}
+
+ return g_strdup(rng_seed_hex);
+}
+
+static void reinitialize_rng_seed(void *opaque)
+{
+ g_autofree char *rng_seed_hex = rng_seed_hex_new();
+
+ strcpy(opaque, rng_seed_hex);
}
Though it isn't deprecated, strcpy isn't really any safer than sprintf.
We don't need to be copying text around quite as much as this.
How about:
#define RNG_SEED_SIZE 32
static void rng_seed_hex_new(char buf[2 * RNG_SEED_SIZE + 1])
{
static const char hex = "0123456789abcdef";
uint8_t rng_seed[RNG_SEED_SIZE];
qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
for (int i = 0; i < RNG_SEED_SIZE; ++i) {
buf[i * 2 + 0] = hex[rng_seed[i] / 16];
buf[i * 2 + 1] = hex[rng_seed[i] % 16];
Hmm. Maybe a
static inline char hexdump_nibble(unsigned val)
{
return (val < 10 ? '0' : 'a') + val;
}
static inline void hexdump_byte(char *out, uint8_t byte)
{
out[0] = hexdump_nibble(byte >> 4);
out[1] = hexdump_nibble(byte & 15);
}
in "qemu/cutils.h", for use elsewhere including util/hexdump.c.
r~
Re: [PATCH for-9.0] meson.build: Disable -fzero-call-used-regs on OpenBSD
On 4/11/2024 8:12 AM, Thomas Huth wrote: On 11/04/2024 14.08, Thomas Huth wrote: QEMU currently does not work on OpenBSD since the -fzero-call-used-regs That should be "OpenBSD 7.5" ... older versions are fine since they are using an older version of Clang that does not have -fzero-call-used-regs yet, I think. About the compiler version that is correct. Between 7.4 and 7.5 we upgraded from Clang 13 to 16. -fzero-call-used-regs was added with the 15 release. https://github.com/llvm/llvm-project/commit/deaf22bc0e306bc44c70d2503e9364b5ed312c49 Retguard is also used to mitigate ROP exploits and is enabled by default. https://www.openbsd.org/papers/asiabsdcon2019-rop-paper.pdf
Re: [PATCH v2 06/13] system/qtest: Replace sprintf() by g_string_append_printf()
On 4/11/24 03:15, Philippe Mathieu-Daudé wrote: 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(&enc[i * 2], "%02x", data[i]); ^ 1 warning generated. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Thomas Huth --- system/qtest.c | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH v2 04/13] hw/mips/malta: Add re-usable rng_seed_hex_new() method
On 4/11/24 03:15, Philippe Mathieu-Daudé wrote:
Extract common code from reinitialize_rng_seed() and
load_kernel() to rng_seed_hex_new().
Signed-off-by: Philippe Mathieu-Daudé
---
hw/mips/malta.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index af74008c82..9fc6a7d313 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 *rng_seed_hex_new(void)
{
-char *rng_seed_hex = opaque;
uint8_t rng_seed[32];
+char rng_seed_hex[sizeof(rng_seed) * 2 + 1];
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]);
}
+
+return g_strdup(rng_seed_hex);
+}
+
+static void reinitialize_rng_seed(void *opaque)
+{
+g_autofree char *rng_seed_hex = rng_seed_hex_new();
+
+strcpy(opaque, rng_seed_hex);
}
Though it isn't deprecated, strcpy isn't really any safer than sprintf.
We don't need to be copying text around quite as much as this.
How about:
#define RNG_SEED_SIZE 32
static void rng_seed_hex_new(char buf[2 * RNG_SEED_SIZE + 1])
{
static const char hex = "0123456789abcdef";
uint8_t rng_seed[RNG_SEED_SIZE];
qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
for (int i = 0; i < RNG_SEED_SIZE; ++i) {
buf[i * 2 + 0] = hex[rng_seed[i] / 16];
buf[i * 2 + 1] = hex[rng_seed[i] % 16];
}
buf[RNG_SEED_SIZE * 2] = '\0';
}
static void reinitialize_rng_seed(void *opaque)
{
rng_seed_hex_new(opaque);
}
with little change in load_kernel.
r~
Re: [PATCH v2 03/13] hw/ppc/spapr: Replace sprintf() by snprintf()
On 4/11/24 03:15, Philippe Mathieu-Daudé wrote: sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1, resulting in painful developper experience. Replace sprintf() by snprintf() 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Richard Henderson r~
Re: [PATCH v2 02/13] hw/vfio/pci: Replace sprintf() by snprintf()
On 4/11/24 03:15, Philippe Mathieu-Daudé wrote: sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1, resulting in painful developper experience. Use snprintf() instead. Signed-off-by: Philippe Mathieu-Daudé --- hw/vfio/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Richard Henderson r~
Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"
Peter Xu writes: > On Thu, Apr 11, 2024 at 11:31:08PM +0530, Het Gala wrote: >> I just wanted to highlight couple of pointers: >> 1. though we are using 'channels' in the precopy tests for 'migrate' QAPI, >> we >> use the old uri for 'migrate-incoming' QAPI. >> 2. We do not cover other 'channels' abi, only have tcp path tested. >> >> So, the TO-DOs could be: >> 1. Omit the 4th patch here, which introduced postcopy qtests with 'channels' >> interface OR have 'channels' interface with other than tcp transport >> (file, exec, vsock, etc) so as to cover different code paths. >> 2. Extend channels interface to migrate-incoming QAPI for precopy qtests > > You can see whether Fabiano has anything to say, but what you proposed > looks good to me. Ok, so what about we convert some of the 'plain' tests into channels to cover all transports? - tcp: test_multifd_tcp_none (this one we already did) - file: test_precopy_file - unix: test_precopy_unix_plain - exec: test_analyze_script - fd: test_migrate_precopy_fd_socket Those^, plus the validate_uri that's already in next should cover everything. We don't need to do this at once, by the way. Moreover: - leave all test strings untouched to preserve bisecting; - let's not bother adding "channels" and "uri" to the test string anymore. The channels API should be taken for granted at this point, I don't expect we start hitting bugs that will require us to run either foo/uri/plain or foo/channels/plain, so there's not much point in making the distinction.
Re: [PATCH v2 01/13] ui/console-vc: Replace sprintf() by snprintf()
On 4/11/24 03:15, Philippe Mathieu-Daudé wrote: sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1, resulting in painful developper experience. Replace sprintf() by snprintf() 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Richard Henderson r~
[PATCH v2] ppc440_pcix: Do not expose a bridge device on PCI bus
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.
Signed-off-by: BALATON Zoltan
---
Here's another version that keeps the values and only drops the device
so it's even less likely it could break anything, in case this can be
accepted for 9.0.
hw/pci-host/ppc440_pcix.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/hw/pci-host/ppc440_pcix.c b/hw/pci-host/ppc440_pcix.c
index 1926ae2a27..ef212d99aa 100644
--- a/hw/pci-host/ppc440_pcix.c
+++ b/hw/pci-host/ppc440_pcix.c
@@ -52,7 +52,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PPC440PCIXState, PPC440_PCIX_HOST)
struct PPC440PCIXState {
PCIHostState parent_obj;
-PCIDevice *dev;
+uint8_t config[PCI_CONFIG_SPACE_SIZE];
struct PLBOutMap pom[PPC440_PCIX_NR_POMS];
struct PLBInMap pim[PPC440_PCIX_NR_PIMS];
uint32_t sts;
@@ -171,7 +171,7 @@ 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);
+stl_le_p(s->config + addr, val);
break;
case PCIX0_POM0LAL:
@@ -302,7 +302,7 @@ static uint64_t ppc440_pcix_reg_read4(void *opaque, hwaddr
addr,
switch (addr) {
case PCI_VENDOR_ID ... PCI_MAX_LAT:
-val = ldl_le_p(s->dev->config + addr);
+val = ldl_le_p(s->config + addr);
break;
case PCIX0_POM0LAL:
@@ -498,10 +498,7 @@ static void ppc440_pcix_realize(DeviceState *dev, Error
**errp)
memory_region_init(&s->iomem, OBJECT(dev), "pci-io", 64 * KiB);
h->bus = pci_register_root_bus(dev, NULL, ppc440_pcix_set_irq,
ppc440_pcix_map_irq, &s->irq, &s->busmem, &s->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(&s->bm, OBJECT(s), "bm-ppc440-pcix", UINT64_MAX);
memory_region_add_subregion(&s->bm, 0x0, &s->busmem);
--
2.30.9
[ANNOUNCE] QEMU 9.0.0-rc3 is now available
Hello, On behalf of the QEMU Team, I'd like to announce the availability of the fourth release candidate for the QEMU 9.0 release. This release is meant for testing purposes and should not be used in a production environment. http://download.qemu.org/qemu-9.0.0-rc3.tar.xz http://download.qemu.org/qemu-9.0.0-rc3.tar.xz.sig http://download.qemu.org/qemu-9.0.0-rc3.tar.bz2 http://download.qemu.org/qemu-9.0.0-rc3.tar.bz2.sig A note from the maintainer: With luck this will be our last rc, and we'll be able to make the final 9.0 release on April 16th. However there were quite a lot of late-arriving changes in this rc cycle, so it's possible we might need to make a few further fixes that need us to make an rc4. In that case we'll release the rc4 on the 16th, and the final release on the 23rd. You can help improve the quality of the QEMU 9.0 release by testing this release and reporting bugs using our GitLab issue tracker: https://gitlab.com/qemu-project/qemu/-/milestones/11 The release plan, as well a documented known issues for release candidates, are available at: http://wiki.qemu.org/Planning/9.0 Please add entries to the ChangeLog for the 9.0 release below: http://wiki.qemu.org/ChangeLog/9.0 Thank you to everyone involved! Changes since rc2: 02e16ab9f4: Update version for v9.0.0-rc3 release (Peter Maydell) dcb0a1ac03: hw/audio/virtio-snd: Remove unused assignment (Philippe Mathieu-Daudé) 83ddb3dbba: hw/net/net_tx_pkt: Fix overrun in update_sctp_checksum() (Philippe Mathieu-Daudé) 9e4b27ca6b: hw/sd/sdhci: Do not update TRNMOD when Command Inhibit (DAT) is set (Philippe Mathieu-Daudé) ad766d603f: hw/net/lan9118: Fix overflow in MIL TX FIFO (Philippe Mathieu-Daudé) a45223467e: hw/net/lan9118: Replace magic '2048' value by MIL_TXFIFO_SIZE definition (Philippe Mathieu-Daudé) eaf2bd2953: backends/cryptodev: Do not abort for invalid session ID (Philippe Mathieu-Daudé) fc09ff2979: hw/misc/applesmc: Fix memory leak in reset() handler (Philippe Mathieu-Daudé) 5c338216f5: hw/misc/applesmc: Do not call DeviceReset from DeviceRealize (Philippe Mathieu-Daudé) d39fdfff34: hw/block/nand: Fix out-of-bound access in NAND block buffer (Philippe Mathieu-Daudé) 2e3e09b368: hw/block/nand: Have blk_load() take unsigned offset and return boolean (Philippe Mathieu-Daudé) 7a86544f28: hw/block/nand: Factor nand_load_iolen() method out (Philippe Mathieu-Daudé) aa88f99c87: qemu-options: Fix CXL Fixed Memory Window interleave-granularity typo (Yuquan Wang) f4729ec39a: hw/virtio/virtio-crypto: Protect from DMA re-entrancy bugs (Philippe Mathieu-Daudé) b4295bff25: hw/char/virtio-serial-bus: Protect from DMA re-entrancy bugs (Philippe Mathieu-Daudé) ba28e0ff4d: hw/display/virtio-gpu: Protect from DMA re-entrancy bugs (Philippe Mathieu-Daudé) ec0504b989: hw/virtio: Introduce virtio_bh_new_guarded() helper (Philippe Mathieu-Daudé) 143bcc1d59: linux-user: Preserve unswapped siginfo_t for strace (Richard Henderson) dcd092a063: accel/tcg: Improve can_do_io management (Richard Henderson) b338970f8c: target/s390x: Use insn_start from DisasContextBase (Richard Henderson) 401aa608d8: target/riscv: Use insn_start from DisasContextBase (Richard Henderson) e231345027: target/microblaze: Use insn_start from DisasContextBase (Richard Henderson) 8df1ba49d7: target/i386: Preserve DisasContextBase.insn_start across rewind (Richard Henderson) 24638bd17d: target/hppa: Use insn_start from DisasContextBase (Richard Henderson) 4642250e3c: target/arm: Use insn_start from DisasContextBase (Richard Henderson) e7face702a: accel/tcg: Add insn_start to DisasContextBase (Richard Henderson) 07843f75fd: tcg: Add TCGContext.emit_before_op (Richard Henderson) 5888357942: target/m68k: Map FPU exceptions to FPSR register (Keith Packard) b754cb2dcd: target/sh4: add missing CHECK_NOT_DELAY_SLOT (Zack Buhman) 7227c0cd50: target/sh4: Fix mac.w with saturation enabled (Zack Buhman) c97e8977dc: target/sh4: Fix mac.l with saturation enabled (Zack Buhman) 7d95db5e78: target/sh4: Merge mach and macl into a union (Richard Henderson) b0f2f2976b: target/sh4: mac.w: memory accesses are 16-bit words (Zack Buhman) 26d937237f: target/hppa: Fix IIAOQ, IIASQ for pa2.0 (Richard Henderson) 2ee80bce4f: linux-user: replace calloc() with g_new0() (Nguyen Dinh Phi) f0907ff4ca: linux-user: Fix waitid return of siginfo_t and rusage (Richard Henderson) e25fe886b8: tcg/optimize: Do not attempt to constant fold neg_vec (Richard Henderson) e3404e01c7: edk2: rebuild binaries with correct version information (Gerd Hoffmann) 2c4eb439dc: edk2/seabios: use common extra version (Gerd Hoffmann) 6494a08d10: edk2: commit version info (Gerd Hoffmann) 6539c73dcc: edk2: get version + date from git submodule (Gerd Hoffmann) e104a9: qdev-monitor: fix error message in find_device_state() (Vladimir Sementsov-Ogievskiy) f67d296b6e: vhost-user-blk: simplify and fix vhost_user_blk_handle_config_change (Vladimir Sementsov-Ogievskiy) 6ae72f609a: vdpa-dev: Fi
Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"
On Thu, Apr 11, 2024 at 11:31:08PM +0530, Het Gala wrote: > I just wanted to highlight couple of pointers: > 1. though we are using 'channels' in the precopy tests for 'migrate' QAPI, > we > use the old uri for 'migrate-incoming' QAPI. > 2. We do not cover other 'channels' abi, only have tcp path tested. > > So, the TO-DOs could be: > 1. Omit the 4th patch here, which introduced postcopy qtests with 'channels' > interface OR have 'channels' interface with other than tcp transport > (file, exec, vsock, etc) so as to cover different code paths. > 2. Extend channels interface to migrate-incoming QAPI for precopy qtests You can see whether Fabiano has anything to say, but what you proposed looks good to me. Thanks! -- Peter Xu
Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"
On 11/04/24 7:56 pm, Peter Xu wrote: !---| CAUTION: External Email |---! On Thu, Apr 11, 2024 at 07:45:21PM +0530, Het Gala wrote: On 10/04/24 8:23 pm, Peter Xu wrote: !---| CAUTION: External Email |---! 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. Do we want to add other transports too (vsock, exec, rdma) with the new interface ? I believe we have tests for fd based migration Het, What I meant is we used to do white box testing for migration, trying to cover all the code paths would suffice for us in that case. It means maybe we don't need the postcopy test to cover the channels interface as long as precopy has covered that and also if that covered all the "channels" abi then we should be safe. What I worry is we keep extending the test matrix but we're actually testing the same code paths. Then the test runs slower each time, we burn more cpus for each CI kick, but without a real beneift. Yeah, I understood your concern Peter. Yes, since precopy and postcopy tests cover the same code path, adding 'channels' postcopy qtests does not make much sense after already having introduced in precopy qtests. I just wanted to highlight couple of pointers: 1. though we are using 'channels' in the precopy tests for 'migrate' QAPI, we use the old uri for 'migrate-incoming' QAPI. 2. We do not cover other 'channels' abi, only have tcp path tested. So, the TO-DOs could be: 1. Omit the 4th patch here, which introduced postcopy qtests with 'channels' interface OR have 'channels' interface with other than tcp transport (file, exec, vsock, etc) so as to cover different code paths. 2. Extend channels interface to migrate-incoming QAPI for precopy qtests Regards, Het Gala
Re: [PATCH for-9.1 v1 0/3] Add SEV/SEV-ES machine compat options for KVM_SEV_INIT2
On Wed, Apr 10, 2024 at 1:08 AM Michael Roth wrote: > > These patches are also available at: > > https://github.com/amdese/qemu/commits/sev-init-legacy-v1 > > and are based on top Paolo's qemu-coco-queue branch containing the > following patches: A more complete version of patch 2 was already on the list, so I queued 1 and 3 to qemu-coco-queue. Thanks! Paolo > > [PATCH for-9.1 00/26] x86, kvm: common confidential computing subset > https://lore.kernel.org/all/20240322181116.1228416-1-pbonz...@redhat.com/T/ > > Overview > > > With the following patches applied from qemu-coco-queue: > > https://lore.kernel.org/all/2024031914.1014247-1-pbonz...@redhat.com/ > > QEMU version 9.1+ will begin automatically making use of the new > KVM_SEV_INIT2 API for initializing SEV and SEV-ES (and eventually, SEV-SNP) > guests verses the older KVM_SEV_INIT/KVM_SEV_ES_INIT interfaces. > > However, the older interfaces would silently avoid sync'ing FPU/XSAVE state > set by QEMU to each vCPU's VMSA prior to encryption. With KVM_SEV_INIT2, > this state will now be synced into the VMSA, resulting in measurements > changes and, theoretically, behaviorial changes, though the latter are > unlikely to be seen in practice. The specific VMSA changes are documented > in the section below for reference. > > This series implements machine compatibility options for SEV/SEV-ES so that > only VMs created with QEMU 9.1+ will make use of KVM_SEV_INIT2 so that VMSA > differences can be accounted for beforehand, and older machine types will > continue using the older interfaces to avoid unexpected measurement > changes. > > Specific VMSA changes > - > > With KVM_SEV_INIT2, rather than 0, QEMU/KVM will instead begin setting the > following fields in the VMSA before measurement/encryption: > > VMSA byte offset [1032:1033] = 80 1f (MXCSR, Multimedia Control Status > Register) > VMSA byte offset [1040:1041] = 7f 03 (FCW, FPU/x86 Control Word) > > Setting FCW (FPU/x86 Control Word) to 0x37f is consistent with 11.5.7 of > APM Volume 2. MXCSR reset state is not defined for XSAVE, but QEMU's 0x1f80 > value is consistent with machine reset state documented in APM Volume 2 > 4.2.2. As such, it is reasonable to begin including these in the VMSA > measurement calculations. > > NOTE: section 11.5.7 also documents that FTW should be all 1's, whereas > QEMU currently sets all zeroes. Should that be changed as part of > this, or are there other reasons for setting 0? > > Thanks, > > Mike > > > Michael Roth (3): > i386/sev: Add 'legacy-vm-type' parameter for SEV guest objects > hw/i386: Add 9.1 machine types for i440fx/q35 > hw/i386/sev: Use legacy SEV VM types for older machine types > > hw/i386/pc.c | 5 + > hw/i386/pc_piix.c| 13 - > hw/i386/pc_q35.c | 12 +++- > include/hw/i386/pc.h | 3 +++ > qapi/qom.json| 11 ++- > target/i386/sev.c| 19 ++- > 6 files changed, 59 insertions(+), 4 deletions(-) > > >
Re: [PATCH v4 02/10] hw/core: create Resettable QOM interface
On 11/4/24 15:43, Peter Maydell wrote:
On Wed, 21 Aug 2019 at 17:34, Damien Hedde wrote:
This commit defines an interface allowing multi-phase reset. This aims
to solve a problem of the actual single-phase reset (built in
DeviceClass and BusClass): reset behavior is dependent on the order
in which reset handlers are called. In particular doing external
side-effect (like setting an qemu_irq) is problematic because receiving
object may not be reset yet.
So, I wanted to drag up this ancient patch to ask a couple
of Resettable questions, because I'm working on adding a
new ResetType (the equivalent of SHUTDOWN_CAUSE_SNAPSHOT_LOAD).
+/**
+ * ResetType:
+ * Types of reset.
+ *
+ * + Cold: reset resulting from a power cycle of the object.
+ *
+ * TODO: Support has to be added to handle more types. In particular,
+ * ResetState structure needs to be expanded.
+ */
Does anybody remember what this TODO comment is about? What
in particular would need to be in the ResetState struct
to allow another type to be added?
IIRC this comes from this discussion:
https://lore.kernel.org/qemu-devel/7c193b33-8188-2cda-cbf2-fb5452544...@greensocs.com/
Updated in this patch (see after '---' description):
https://lore.kernel.org/qemu-devel/20191018150630.31099-9-damien.he...@greensocs.com/
+typedef enum ResetType {
+RESET_TYPE_COLD,
+} ResetType;
+typedef void (*ResettableInitPhase)(Object *obj, ResetType type);
+typedef void (*ResettableHoldPhase)(Object *obj);
+typedef void (*ResettableExitPhase)(Object *obj);
Was there a reason why we only pass the ResetType to the init
phase method, and not also to the hold and exit phases ?
Given that many devices don't need to implement init, it
seems awkward to require them to do so just to stash the
ResetType somewhere so they can look at it in the hold
or exit phase, so I was thinking about adding the argument
to the other two phase methods.
You are right, the type should be propagated to to all phase
handlers.
thanks
-- PMM
Re: [PATCH] target/riscv: fix instructions count handling in icount mode
On Thu, Apr 11, 2024 at 4:34 AM Clément Léger wrote:
>
> When icount is enabled, rather than returning the virtual CPU time, we
> should return the instruction count itself. Add an instructions bool
> parameter to get_ticks() to correctly return icount_get_raw() when
> icount_enabled() == 1 and instruction count is queried. This will modify
> the existing behavior which was returning an instructions count close to
> the number of cycles (CPI ~= 1).
>
> Signed-off-by: Clément Léger
>
> ---
> target/riscv/csr.c | 29 -
> 1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 726096444f..5f1dcee102 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -762,14 +762,17 @@ static RISCVException write_vcsr(CPURISCVState *env,
> int csrno,
> }
>
> /* User Timers and Counters */
> -static target_ulong get_ticks(bool shift)
> +static target_ulong get_ticks(bool shift, bool instructions)
> {
> int64_t val;
> target_ulong result;
>
> #if !defined(CONFIG_USER_ONLY)
> if (icount_enabled()) {
> -val = icount_get();
> +if (instructions)
> +val = icount_get_raw();
> +else
> +val = icount_get();
> } else {
> val = cpu_get_host_ticks();
> }
> @@ -804,14 +807,14 @@ static RISCVException read_timeh(CPURISCVState *env,
> int csrno,
> static RISCVException read_hpmcounter(CPURISCVState *env, int csrno,
>target_ulong *val)
> {
> -*val = get_ticks(false);
> +*val = get_ticks(false, (csrno == CSR_INSTRET));
> return RISCV_EXCP_NONE;
> }
>
> static RISCVException read_hpmcounterh(CPURISCVState *env, int csrno,
> target_ulong *val)
> {
> -*val = get_ticks(true);
> +*val = get_ticks(true, (csrno == CSR_INSTRETH));
> return RISCV_EXCP_NONE;
> }
>
> @@ -875,11 +878,11 @@ static RISCVException write_mhpmcounter(CPURISCVState
> *env, int csrno,
> int ctr_idx = csrno - CSR_MCYCLE;
> PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> uint64_t mhpmctr_val = val;
> +bool instr = riscv_pmu_ctr_monitor_instructions(env, ctr_idx);
>
> counter->mhpmcounter_val = val;
> -if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
> -riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
> -counter->mhpmcounter_prev = get_ticks(false);
> +if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) || instr) {
> +counter->mhpmcounter_prev = get_ticks(false, instr);
> if (ctr_idx > 2) {
> if (riscv_cpu_mxl(env) == MXL_RV32) {
> mhpmctr_val = mhpmctr_val |
> @@ -902,12 +905,12 @@ static RISCVException write_mhpmcounterh(CPURISCVState
> *env, int csrno,
> PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
> uint64_t mhpmctr_val = counter->mhpmcounter_val;
> uint64_t mhpmctrh_val = val;
> +bool instr = riscv_pmu_ctr_monitor_instructions(env, ctr_idx);
>
> counter->mhpmcounterh_val = val;
> mhpmctr_val = mhpmctr_val | (mhpmctrh_val << 32);
> -if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
> -riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
> -counter->mhpmcounterh_prev = get_ticks(true);
> +if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) || instr) {
> +counter->mhpmcounterh_prev = get_ticks(true, instr);
> if (ctr_idx > 2) {
> riscv_pmu_setup_timer(env, mhpmctr_val, ctr_idx);
> }
> @@ -926,6 +929,7 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState
> *env, target_ulong *val,
> counter->mhpmcounter_prev;
> target_ulong ctr_val = upper_half ? counter->mhpmcounterh_val :
> counter->mhpmcounter_val;
> +bool instr = riscv_pmu_ctr_monitor_instructions(env, ctr_idx);
>
> if (get_field(env->mcountinhibit, BIT(ctr_idx))) {
> /*
> @@ -946,9 +950,8 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState
> *env, target_ulong *val,
> * The kernel computes the perf delta by subtracting the current value
> from
> * the value it initialized previously (ctr_val).
> */
> -if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
> -riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
> -*val = get_ticks(upper_half) - ctr_prev + ctr_val;
> +if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) || instr) {
> +*val = get_ticks(upper_half, instr) - ctr_prev + ctr_val;
> } else {
> *val = ctr_val;
> }
> --
> 2.43.0
>
Reviewed-by: Atish Patra
Re: [PATCH] Makefile: fix use of -j without an argument
On 11/4/24 17:38, Matheus Tavares Bernardino wrote: Hi, Philippe On Thu, 11 Apr 2024 17:29:58 +0200 =?UTF-8?Q?Philippe_Mathieu-Daud=C3=A9?= wrote: Hi Matheus, On 11/4/24 17:09, Matheus Tavares Bernardino wrote: Our Makefile massages the given make arguments to invoke ninja accordingly. One key difference is that ninja will parallelize by default, whereas make only does so with -j or -j. The make man page says that "if the -j option is given without an argument, make will not limit the number of jobs that can run simultaneously". We use to support that by replacing -j with "" (empty string) when calling ninja, so that it would do its auto-parallelization based on the number of CPU cores. This was accidentally broken at d1ce2cc95b (Makefile: preserve --jobserver-auth argument when calling ninja, 2024-04-02), causing `make -j` to fail: $ make -j V=1 /usr/bin/ninja -v -j -d keepdepfile all | cat make -C contrib/plugins/ V="1" TARGET_DIR="contrib/plugins/" all ninja: fatal: invalid -j parameter make: *** [Makefile:161: run-ninja] Error Let's fix that and indent the touched code for better readability. Signed-off-by: Matheus Tavares Bernardino --- Makefile | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 183756018f..d299c14dab 100644 --- a/Makefile +++ b/Makefile @@ -142,8 +142,12 @@ MAKE.k = $(findstring k,$(firstword $(filter-out --%,$(MAKEFLAGS MAKE.q = $(findstring q,$(firstword $(filter-out --%,$(MAKEFLAGS MAKE.nq = $(if $(word 2, $(MAKE.n) $(MAKE.q)),nq) NINJAFLAGS = $(if $V,-v) $(if $(MAKE.n), -n) $(if $(MAKE.k), -k0) \ -$(or $(filter -l% -j%, $(MAKEFLAGS)), $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \ --d keepdepfile +$(if $(filter -j, $(MAKEFLAGS)) \ +,, \ +$(or \ + $(filter -l% -j%, $(MAKEFLAGS)), \ + $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \ +) -d keepdepfile ninja-cmd-goals = $(or $(MAKECMDGOALS), all) ninja-cmd-goals += $(foreach g, $(MAKECMDGOALS), $(.ninja-goals.$g)) Apparently Martin sent the same patch (although not as nicely indented) and Paolo queued it: https://lore.kernel.org/qemu-devel/20240402081738.1051560-1-mar...@geanix.com/ Actually, this patch is a follow-up to that one, fixing a feature that was accidentally broken. Oops sorry I missed that, I was not expecting this patch to be merged in 9.0 :/
Re: [PATCH for-9.1 09/19] target/i386: move 60-BF opcodes to new decoder
On Thu, Apr 11, 2024 at 5:05 PM Zhao Liu wrote: > > On Tue, Apr 09, 2024 at 06:43:13PM +0200, Paolo Bonzini wrote: > > Date: Tue, 9 Apr 2024 18:43:13 +0200 > > From: Paolo Bonzini > > Subject: [PATCH for-9.1 09/19] target/i386: move 60-BF opcodes to new > > decoder > > X-Mailer: git-send-email 2.44.0 > > > > Compared to the old decoder, the main differences in translation > > are for the little-used ARPL instruction. IMUL is adjusted a bit > > to share more code to produce flags, but is otherwise very similar. > > > > Signed-off-by: Paolo Bonzini > > --- > > target/i386/tcg/decode-new.h | 2 + > > target/i386/tcg/translate.c | 9 +- > > target/i386/tcg/decode-new.c.inc | 171 + > > target/i386/tcg/emit.c.inc | 317 +++ > > 4 files changed, 497 insertions(+), 2 deletions(-) > > HMM, I met Guest boot failure on this patch because of ata unrecognized. > I haven't located the exact error yet, so let me post my log first. > If there are other means I can use to dig further, I'd be happy to try > that too. > > # Command (boot a ubuntu Guest via TCG) > > ./qemu/build/qemu-system-x86_64 \ > -smp 1 \ > -name ubuntu -m 4G \ > -cpu max -accel tcg \ > -hda ../img_qemu/test.qcow2 -nographic \ > -kernel ../img_qemu/kernel/vmlinuz-6.4.0-rc6+ \ > -initrd ../img_qemu/kernel/initrd.img-6.4.0-rc6+ \ > -append "root=/dev/sda ro console=ttyS0" \ > -qmp unix:/tmp/qmp-sock,server=on,wait=off I did run a bunch of boot tests but I'll check this one too. Thanks! Paolo
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
> 1) Either a CI test covering at least the major RDMA paths, or at least > periodically tests for each QEMU release will be needed. We use a batch of regression test cases for the stack, which covers the test for QEMU. I did such test for most of the QEMU releases planned as candidates for rollout. The migration test needs a pair of (either physical or virtual) servers with InfiniBand network, which makes it difficult to do on a single server. The nested VM could be a possible approach, for which we may need virtual InfiniBand network. Is SoftRoCE [1] a choice? I will try it and let you know. [1] https://enterprise-support.nvidia.com/s/article/howto-configure-soft-roce Thanks and best regards! On Thu, Apr 11, 2024 at 4:20 PM Peter Xu wrote: > > On Wed, Apr 10, 2024 at 09:49:15AM -0400, Peter Xu wrote: > > 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. > > Some people got dropped on the cc list for unknown reason, I'm adding them > back (Fabiano, Peter Maydell, Phil). Let's make sure nobody is dropped by > accident. > > I'll try to summarize what is still missing, and I think these will be > greatly helpful if we don't want to deprecate rdma migration: > > 1) Either a CI test covering at least the major RDMA paths, or at least > periodically tests for each QEMU release will be needed. > > 2) Some performance tests between modern RDMA and NIC devices are > welcomed. The current knowledge is modern NIC can work similarly to > RDMA in performance, then it's debatable why we still maintain so much > rdma specific code. > > 3) No need to be soild patchsets for this one, but some plan to improve > RDMA migration code so that it is not almost isolated from the rest > protocols. > > 4) Someone to look after this code for real. > > For 2) and 3) more info is here: > > https://lore.kernel.org/r/ZhWa0YeAb9ySVKD1@x1n > > Here 4) can be the most important as Markus pointed out. We just didn't > get there yet on the discussions, but maybe Markus is right that we should > talk that first. > > Thanks, > > -- > Peter Xu >
Re: [PATCH] Makefile: fix use of -j without an argument
Hi, Philippe On Thu, 11 Apr 2024 17:29:58 +0200 =?UTF-8?Q?Philippe_Mathieu-Daud=C3=A9?= wrote: > > Hi Matheus, > > On 11/4/24 17:09, Matheus Tavares Bernardino wrote: > > Our Makefile massages the given make arguments to invoke ninja > > accordingly. One key difference is that ninja will parallelize by > > default, whereas make only does so with -j or -j. The make man page > > says that "if the -j option is given without an argument, make will not > > limit the number of jobs that can run simultaneously". We use to support > > that by replacing -j with "" (empty string) when calling ninja, so that > > it would do its auto-parallelization based on the number of CPU cores. > > > > This was accidentally broken at d1ce2cc95b (Makefile: preserve > > --jobserver-auth argument when calling ninja, 2024-04-02), > > causing `make -j` to fail: > > > > $ make -j V=1 > >/usr/bin/ninja -v -j -d keepdepfile all | cat > >make -C contrib/plugins/ V="1" TARGET_DIR="contrib/plugins/" all > >ninja: fatal: invalid -j parameter > >make: *** [Makefile:161: run-ninja] Error > > > > Let's fix that and indent the touched code for better readability. > > > > Signed-off-by: Matheus Tavares Bernardino > > --- > > Makefile | 8 ++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index 183756018f..d299c14dab 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -142,8 +142,12 @@ MAKE.k = $(findstring k,$(firstword $(filter-out > > --%,$(MAKEFLAGS > > MAKE.q = $(findstring q,$(firstword $(filter-out --%,$(MAKEFLAGS > > MAKE.nq = $(if $(word 2, $(MAKE.n) $(MAKE.q)),nq) > > NINJAFLAGS = $(if $V,-v) $(if $(MAKE.n), -n) $(if $(MAKE.k), -k0) \ > > -$(or $(filter -l% -j%, $(MAKEFLAGS)), $(if $(filter > > --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \ > > --d keepdepfile > > +$(if $(filter -j, $(MAKEFLAGS)) \ > > +,, \ > > +$(or \ > > + $(filter -l% -j%, $(MAKEFLAGS)), \ > > + $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \ > > +) -d keepdepfile > > ninja-cmd-goals = $(or $(MAKECMDGOALS), all) > > ninja-cmd-goals += $(foreach g, $(MAKECMDGOALS), $(.ninja-goals.$g)) > > > > Apparently Martin sent the same patch (although not as nicely > indented) and Paolo queued it: > https://lore.kernel.org/qemu-devel/20240402081738.1051560-1-mar...@geanix.com/ Actually, this patch is a follow-up to that one, fixing a feature that was accidentally broken.
Re: [PATCH] Makefile: fix use of -j without an argument
Hi Matheus, On 11/4/24 17:09, Matheus Tavares Bernardino wrote: Our Makefile massages the given make arguments to invoke ninja accordingly. One key difference is that ninja will parallelize by default, whereas make only does so with -j or -j. The make man page says that "if the -j option is given without an argument, make will not limit the number of jobs that can run simultaneously". We use to support that by replacing -j with "" (empty string) when calling ninja, so that it would do its auto-parallelization based on the number of CPU cores. This was accidentally broken at d1ce2cc95b (Makefile: preserve --jobserver-auth argument when calling ninja, 2024-04-02), causing `make -j` to fail: $ make -j V=1 /usr/bin/ninja -v -j -d keepdepfile all | cat make -C contrib/plugins/ V="1" TARGET_DIR="contrib/plugins/" all ninja: fatal: invalid -j parameter make: *** [Makefile:161: run-ninja] Error Let's fix that and indent the touched code for better readability. Signed-off-by: Matheus Tavares Bernardino --- Makefile | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 183756018f..d299c14dab 100644 --- a/Makefile +++ b/Makefile @@ -142,8 +142,12 @@ MAKE.k = $(findstring k,$(firstword $(filter-out --%,$(MAKEFLAGS MAKE.q = $(findstring q,$(firstword $(filter-out --%,$(MAKEFLAGS MAKE.nq = $(if $(word 2, $(MAKE.n) $(MAKE.q)),nq) NINJAFLAGS = $(if $V,-v) $(if $(MAKE.n), -n) $(if $(MAKE.k), -k0) \ -$(or $(filter -l% -j%, $(MAKEFLAGS)), $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \ --d keepdepfile +$(if $(filter -j, $(MAKEFLAGS)) \ +,, \ +$(or \ + $(filter -l% -j%, $(MAKEFLAGS)), \ + $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \ +) -d keepdepfile ninja-cmd-goals = $(or $(MAKECMDGOALS), all) ninja-cmd-goals += $(foreach g, $(MAKECMDGOALS), $(.ninja-goals.$g)) Apparently Martin sent the same patch (although not as nicely indented) and Paolo queued it: https://lore.kernel.org/qemu-devel/20240402081738.1051560-1-mar...@geanix.com/
[PATCH] Makefile: fix use of -j without an argument
Our Makefile massages the given make arguments to invoke ninja accordingly. One key difference is that ninja will parallelize by default, whereas make only does so with -j or -j. The make man page says that "if the -j option is given without an argument, make will not limit the number of jobs that can run simultaneously". We use to support that by replacing -j with "" (empty string) when calling ninja, so that it would do its auto-parallelization based on the number of CPU cores. This was accidentally broken at d1ce2cc95b (Makefile: preserve --jobserver-auth argument when calling ninja, 2024-04-02), causing `make -j` to fail: $ make -j V=1 /usr/bin/ninja -v -j -d keepdepfile all | cat make -C contrib/plugins/ V="1" TARGET_DIR="contrib/plugins/" all ninja: fatal: invalid -j parameter make: *** [Makefile:161: run-ninja] Error Let's fix that and indent the touched code for better readability. Signed-off-by: Matheus Tavares Bernardino --- Makefile | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 183756018f..d299c14dab 100644 --- a/Makefile +++ b/Makefile @@ -142,8 +142,12 @@ MAKE.k = $(findstring k,$(firstword $(filter-out --%,$(MAKEFLAGS MAKE.q = $(findstring q,$(firstword $(filter-out --%,$(MAKEFLAGS MAKE.nq = $(if $(word 2, $(MAKE.n) $(MAKE.q)),nq) NINJAFLAGS = $(if $V,-v) $(if $(MAKE.n), -n) $(if $(MAKE.k), -k0) \ -$(or $(filter -l% -j%, $(MAKEFLAGS)), $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \ --d keepdepfile +$(if $(filter -j, $(MAKEFLAGS)) \ +,, \ +$(or \ + $(filter -l% -j%, $(MAKEFLAGS)), \ + $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \ +) -d keepdepfile ninja-cmd-goals = $(or $(MAKECMDGOALS), all) ninja-cmd-goals += $(foreach g, $(MAKECMDGOALS), $(.ninja-goals.$g)) -- 2.37.2
Re: [PATCH for-9.1 09/19] target/i386: move 60-BF opcodes to new decoder
On Tue, Apr 09, 2024 at 06:43:13PM +0200, Paolo Bonzini wrote: > Date: Tue, 9 Apr 2024 18:43:13 +0200 > From: Paolo Bonzini > Subject: [PATCH for-9.1 09/19] target/i386: move 60-BF opcodes to new > decoder > X-Mailer: git-send-email 2.44.0 > > Compared to the old decoder, the main differences in translation > are for the little-used ARPL instruction. IMUL is adjusted a bit > to share more code to produce flags, but is otherwise very similar. > > Signed-off-by: Paolo Bonzini > --- > target/i386/tcg/decode-new.h | 2 + > target/i386/tcg/translate.c | 9 +- > target/i386/tcg/decode-new.c.inc | 171 + > target/i386/tcg/emit.c.inc | 317 +++ > 4 files changed, 497 insertions(+), 2 deletions(-) HMM, I met Guest boot failure on this patch because of ata unrecognized. I haven't located the exact error yet, so let me post my log first. If there are other means I can use to dig further, I'd be happy to try that too. # Command (boot a ubuntu Guest via TCG) ./qemu/build/qemu-system-x86_64 \ -smp 1 \ -name ubuntu -m 4G \ -cpu max -accel tcg \ -hda ../img_qemu/test.qcow2 -nographic \ -kernel ../img_qemu/kernel/vmlinuz-6.4.0-rc6+ \ -initrd ../img_qemu/kernel/initrd.img-6.4.0-rc6+ \ -append "root=/dev/sda ro console=ttyS0" \ -qmp unix:/tmp/qmp-sock,server=on,wait=off # Guest log ... [1.567002] scsi host0: ata_piix [1.570945] scsi host1: ata_piix [1.571446] ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xc040 irq 14 [1.571592] ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xc048 irq 15 [1.577016] tun: Universal TUN/TAP device driver, 1.6 [1.579437] PPP generic driver version 2.4.2 [1.583207] VFIO - User Level meta-driver version: 0.3 [1.585787] i8042: PNP: PS/2 Controller [PNP0303:KBD,PNP0f13:MOU] at 0x60,0x64 irq 1,12 [1.593520] serio: i8042 KBD port at 0x60,0x64 irq 1 [1.593847] serio: i8042 AUX port at 0x60,0x64 irq 12 [1.599632] mousedev: PS/2 mouse device common for all mice [1.602405] input: AT Translated Set 2 keyboard as /devices/platform/i8042/serio0/input/input1 [1.604135] rtc_cmos 00:05: RTC can wake from S4 [1.611641] rtc_cmos 00:05: registered as rtc0 [1.612271] rtc_cmos 00:05: setting system clock to 2024-04-11T14:59:56 UTC (1712847596) [1.613380] rtc_cmos 00:05: alarms up to one day, y3k, 242 bytes nvram, hpet irqs [1.613669] i2c_dev: i2c /dev entries driver [1.614302] device-mapper: core: CONFIG_IMA_DISABLE_HTABLE is disabled. Duplicate IMA measurements will not be recorded in the IMA log. [1.614687] device-mapper: uevent: version 1.0.3 [1.619437] device-mapper: ioctl: 4.48.0-ioctl (2023-03-01) initialised: dm-de...@redhat.com [1.620134] platform eisa.0: Probing EISA bus 0 [1.620388] platform eisa.0: EISA: Cannot allocate resource for mainboard [1.620608] platform eisa.0: Cannot allocate resource for EISA slot 1 [1.620749] platform eisa.0: Cannot allocate resource for EISA slot 2 [1.621045] platform eisa.0: Cannot allocate resource for EISA slot 3 [1.621178] platform eisa.0: Cannot allocate resource for EISA slot 4 [1.621291] platform eisa.0: Cannot allocate resource for EISA slot 5 [1.621400] platform eisa.0: Cannot allocate resource for EISA slot 6 [1.621510] platform eisa.0: Cannot allocate resource for EISA slot 7 [1.621621] platform eisa.0: Cannot allocate resource for EISA slot 8 [1.621745] platform eisa.0: EISA: Detected 0 cards [1.621873] amd_pstate: driver load is disabled, boot with specific mode to enable this [1.622309] ledtrig-cpu: registered to indicate activity on CPUs [1.628983] drop_monitor: Initializing network drop monitor service [1.735074] ata1: found unknown device (class 0) [1.748551] ata2: found unknown device (class 0) [1.763380] NET: Registered PF_INET6 protocol family [1.771260] ata1.00: failed to IDENTIFY (I/O error, err_mask=0x2) [2.330777] tsc: Refined TSC clocksource calibration: 2903.979 MHz [2.331349] clocksource: tsc: mask: 0x max_cycles: 0x29dbf1ca37e, max_idle_ns: 440795233019 ns [2.332227] clocksource: Switched to clocksource tsc [5.375200] Freeing initrd memory: 72840K [5.526057] Segment Routing with IPv6 [5.526392] In-situ OAM (IOAM) with IPv6 [5.527164] NET: Registered PF_PACKET protocol family [5.527709] Key type dns_resolver registered [5.530256] IPI shorthand broadcast: enabled [5.545746] sched_clock: Marking stable (5512061965, 30528485)->(5548999115, -6408665) [5.548071] registered taskstats version 1 [5.550142] Loading compiled-in X.509 certificates [5.563141] Key type .fscrypt registered [5.563275] Key type fscrypt-provisioning registered [5.594426] Key type encrypted registered [5.594760] AppArmor: AppArmor sha1 policy hashing enabled [5.595336] ima: No TPM chip found, activating TPM-bypass! [5.595621] Loading compiled-in module X.509 ce
Re: [PATCH for-9.0] meson.build: Disable -fzero-call-used-regs on OpenBSD
On 11/4/24 14:08, Thomas Huth wrote:
QEMU currently does not work on OpenBSD since the -fzero-call-used-regs
option that we added to meson.build recently does not work with the
"retguard" extension from OpenBSD's Clang. Thus let's disable the
-fzero-call-used-regs here until there's a better solution available.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2278
Signed-off-by: Thomas Huth
---
Note: Given that we're close to the release, I think the host_os check
is the best we can do ... the problem does not seem to trigger in all
functions, only if certain registers are used by the compiler, so a
more sophisticated check here seems to be too fragile to me right now.
meson.build | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index c9c3217ba4..91a0aa64c6 100644
--- a/meson.build
+++ b/meson.build
@@ -562,7 +562,11 @@ hardening_flags = [
#
# NB: Clang 17 is broken and SEGVs
# https://github.com/llvm/llvm-project/issues/75168
-if cc.compiles('extern struct { void (*cb)(void); } s; void f(void) { s.cb();
}',
+#
+# NB2: This clashes with the "retguard" extension of OpenBSD's Clang
+# https://gitlab.com/qemu-project/qemu/-/issues/2278
+if host_os != 'openbsd' and \
+ cc.compiles('extern struct { void (*cb)(void); } s; void f(void) { s.cb();
}',
name: '-fzero-call-used-regs=used-gpr',
args: ['-O2', '-fzero-call-used-regs=used-gpr'])
hardening_flags += '-fzero-call-used-regs=used-gpr'
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH] linux-user/flatload.c: Remove unused bFLT shared-library and ZFLAT code
On 11/4/24 13:53, Peter Maydell wrote:
Ever since the bFLT format support was added in 2006, there has been
a chunk of code in the file guarded by CONFIG_BINFMT_SHARED_FLAT
which is supposedly for shared library support. This is not enabled
and it's not possible to enable it, because if you do you'll run into
the "#error needs checking" in the calc_reloc() function.
Similarly, CONFIG_BINFMT_ZFLAT exists but can't be enabled because of
an "#error code needs checking" in load_flat_file().
This code is obviously unfinished and has never been used; nobody in
the intervening 18 years has complained about this or fixed it, so
just delete the dead code. If anybody ever wants the feature they
can always pull it out of git, or (perhaps better) write it from
scratch based on the current Linux bFLT loader rather than the one of
18 years ago.
Signed-off-by: Peter Maydell
---
linux-user/flat.h | 5 +-
linux-user/flatload.c | 293 ++
2 files changed, 11 insertions(+), 287 deletions(-)
@@ -268,40 +115,7 @@ calc_reloc(abi_ulong r, struct lib_info *p, int curid, int
internalp)
abi_ulong text_len;
abi_ulong start_code;
-#ifdef CONFIG_BINFMT_SHARED_FLAT
-#error needs checking
-if (r == 0)
-id = curid;/* Relocs of 0 are always self referring */
-else {
-id = (r >> 24) & 0xff; /* Find ID for this reloc */
-r &= 0x00ff; /* Trim ID off here */
-}
-if (id >= MAX_SHARED_LIBS) {
-fprintf(stderr, "BINFMT_FLAT: reference 0x%x to shared library %d\n",
-(unsigned) r, id);
-goto failed;
-}
-if (curid != id) {
-if (internalp) {
-fprintf(stderr, "BINFMT_FLAT: reloc address 0x%x not "
-"in same module (%d != %d)\n",
-(unsigned) r, curid, id);
-goto failed;
-} else if (!p[id].loaded && is_error(load_flat_shared_library(id, p)))
{
-fprintf(stderr, "BINFMT_FLAT: failed to load library %d\n", id);
-goto failed;
-}
-/* Check versioning information (i.e. time stamps) */
-if (p[id].build_date && p[curid].build_date
-&& p[curid].build_date < p[id].build_date) {
-fprintf(stderr, "BINFMT_FLAT: library %d is younger than %d\n",
-id, curid);
-goto failed;
-}
-}
-#else
id = 0;
I note 'curid' is not used, and 'id' is always 0, because
#defineMAX_SHARED_LIBS (1)
Having:
struct lib_info libinfo[MAX_SHARED_LIBS];
Maybe we can remove MAX_SHARED_LIBS entirely to simplify
further?
Reviewed-by: Philippe Mathieu-Daudé
-#endif
start_brk = p[id].start_brk;
start_data = p[id].start_data;
@@ -425,12 +239,10 @@ static int load_flat_file(struct linux_binprm * bprm,
if (rev == OLD_FLAT_VERSION && flat_old_ram_flag(flags))
flags = FLAT_FLAG_RAM;
-#ifndef CONFIG_BINFMT_ZFLAT
if (flags & (FLAT_FLAG_GZIP|FLAT_FLAG_GZDATA)) {
-fprintf(stderr, "Support for ZFLAT executables is not enabled\n");
+fprintf(stderr, "ZFLAT executables are not supported\n");
return -ENOEXEC;
}
-#endif
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
Hi Peter, On Tue, Apr 9, 2024 at 9:47 PM Peter Xu wrote: > > On Tue, Apr 09, 2024 at 09:32:46AM +0200, Jinpu Wang wrote: > > Hi Peter, > > > > On Mon, Apr 8, 2024 at 6:18 PM Peter Xu wrote: > > > > > > On Mon, Apr 08, 2024 at 04:07:20PM +0200, Jinpu Wang wrote: > > > > Hi Peter, > > > > > > Jinpu, > > > > > > Thanks for joining the discussion. > > > > > > > > > > > On Tue, Apr 2, 2024 at 11:24 PM Peter Xu wrote: > > > > > > > > > > On Mon, Apr 01, 2024 at 11:26:25PM +0200, Yu Zhang wrote: > > > > > > Hello Peter und Zhjian, > > > > > > > > > > > > Thank you so much for letting me know about this. I'm also a bit > > > > > > surprised at > > > > > > the plan for deprecating the RDMA migration subsystem. > > > > > > > > > > It's not too late, since it looks like we do have users not yet > > > > > notified > > > > > from this, we'll redo the deprecation procedure even if it'll be the > > > > > final > > > > > plan, and it'll be 2 releases after this. > > > > > > > > > > > > > > > > > > IMHO it's more important to know whether there are still users > > > > > > > and whether > > > > > > > they would still like to see it around. > > > > > > > > > > > > > I admit RDMA migration was lack of testing(unit/CI test), which > > > > > > > led to the a few > > > > > > > obvious bugs being noticed too late. > > > > > > > > > > > > Yes, we are a user of this subsystem. I was unaware of the lack of > > > > > > test coverage > > > > > > for this part. As soon as 8.2 was released, I saw that many of the > > > > > > migration test > > > > > > cases failed and came to realize that there might be a bug between > > > > > > 8.1 > > > > > > and 8.2, but > > > > > > was unable to confirm and report it quickly to you. > > > > > > > > > > > > The maintenance of this part could be too costly or difficult from > > > > > > your point of view. > > > > > > > > > > It may or may not be too costly, it's just that we need real users of > > > > > RDMA > > > > > taking some care of it. Having it broken easily for >1 releases > > > > > definitely > > > > > is a sign of lack of users. It is an implication to the community > > > > > that we > > > > > should consider dropping some features so that we can get the best > > > > > use of > > > > > the community resources for the things that may have a broader > > > > > audience. > > > > > > > > > > One thing majorly missing is a RDMA tester to guard all the merges to > > > > > not > > > > > break RDMA paths, hopefully in CI. That should not rely on RDMA > > > > > hardwares > > > > > but just to sanity check the migration+rdma code running all fine. > > > > > RDMA > > > > > taught us the lesson so we're requesting CI coverage for all other new > > > > > features that will be merged at least for migration subsystem, so > > > > > that we > > > > > plan to not merge anything that is not covered by CI unless extremely > > > > > necessary in the future. > > > > > > > > > > For sure CI is not the only missing part, but I'd say we should start > > > > > with > > > > > it, then someone should also take care of the code even if only in > > > > > maintenance mode (no new feature to add on top). > > > > > > > > > > > > > > > > > My concern is, this plan will forces a few QEMU users (not sure how > > > > > > many) like us > > > > > > either to stick to the RDMA migration by using an increasingly older > > > > > > version of QEMU, > > > > > > or to abandon the currently used RDMA migration. > > > > > > > > > > RDMA doesn't get new features anyway, if there's specific use case > > > > > for RDMA > > > > > migrations, would it work if such a scenario uses the old binary? Is > > > > > it > > > > > possible to switch to the TCP protocol with some good NICs? > > > > We have used rdma migration with HCA from Nvidia for years, our > > > > experience is RDMA migration works better than tcp (over ipoib). > > > > > > Please bare with me, as I know little on rdma stuff. > > > > > > I'm actually pretty confused (and since a long time ago..) on why we need > > > to operation with rdma contexts when ipoib seems to provide all the tcp > > > layers. I meant, can it work with the current "tcp:" protocol with ipoib > > > even if there's rdma/ib hardwares underneath? Is it because of > > > performance > > > improvements so that we must use a separate path comparing to generic > > > "tcp:" protocol here? > > using rdma protocol with ib verbs , we can leverage the full benefit of > > RDMA by > > talking directly to NIC which bypasses the kernel overhead, less cpu > > utilization and better performance. > > > > While IPoIB is more for compatibility to applications using tcp, but > > can't get full benefit of RDMA. When you have mix generation of IB > > devices, there are performance issue on IPoIB, we've seen 40G HCA can > > only reach 2 Gb/s on IPoIB, but with raw RDMA can reach full line > > speed. > > > > I just run a simple iperf3 test via ipoib and ib_send_bw on same hosts: > > > > iperf 3.9 > > Linu
COLO state?
Hi COLO maintainers, Would you please take a look at this issue? https://gitlab.com/qemu-project/qemu/-/issues/2277 The reporter claims it affects from 9.0-rc2 all the way back to QEMU 7.2. I don't have any kind of setup for COLO, so it will take me a while to be able to verify this. Could you also provide clarification on what is the state of COLO these days? Are any of you looking at it? Do we know of active users of the feature? Also, is the MAINTAINERS file reflecting the actual maintenance state according to you? COLO Framework M: Hailiang Zhang S: Maintained F: migration/colo* F: include/migration/colo.h F: include/migration/failover.h F: docs/COLO-FT.txt COLO Proxy M: Zhang Chen M: Li Zhijian S: Supported F: docs/colo-proxy.txt F: net/colo* F: net/filter-rewriter.c F: net/filter-mirror.c F: tests/qtest/test-filter* Thanks
Re: [PATCH for-9.1 10/19] target/i386: generalize gen_movl_seg_T0
Hi Paolo,
On Tue, Apr 09, 2024 at 06:43:14PM +0200, Paolo Bonzini wrote:
> Date: Tue, 9 Apr 2024 18:43:14 +0200
> From: Paolo Bonzini
> Subject: [PATCH for-9.1 10/19] target/i386: generalize gen_movl_seg_T0
> X-Mailer: git-send-email 2.44.0
>
> In the new decoder it is sometimes easier to put the segment
> in T1 instead of T0, usually because another operand was loaded
> by common code in T0. Genrealize gen_movl_seg_T0 to allow
> using any source.
>
> Signed-off-by: Paolo Bonzini
> ---
> target/i386/tcg/translate.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> index de1ccb6ea7f..8a34e50c452 100644
> --- a/target/i386/tcg/translate.c
> +++ b/target/i386/tcg/translate.c
> @@ -2531,12 +2531,12 @@ static void gen_op_movl_seg_real(DisasContext *s,
> X86Seg seg_reg, TCGv seg)
> tcg_gen_shli_tl(cpu_seg_base[seg_reg], selector, 4);
> }
>
> -/* move T0 to seg_reg and compute if the CPU state may change. Never
> +/* move SRC to seg_reg and compute if the CPU state may change. Never
> call this function with seg_reg == R_CS */
> -static void gen_movl_seg_T0(DisasContext *s, X86Seg seg_reg)
> +static void gen_movl_seg(DisasContext *s, X86Seg seg_reg, TCGv src)
> {
> if (PE(s) && !VM86(s)) {
> -tcg_gen_trunc_tl_i32(s->tmp2_i32, s->T0);
> +tcg_gen_trunc_tl_i32(s->tmp2_i32, src);
> gen_helper_load_seg(tcg_env, tcg_constant_i32(seg_reg), s->tmp2_i32);
> /* abort translation because the addseg value may change or
> because ss32 may change. For R_SS, translation must always
This patch missed to include another gen_movl_seg_T0() use in emit.c.inc,
which was cleaned up later in patch 11. We could move that cleanup into
this patch to avoid compiling failures.
-Zhao
Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"
On Thu, Apr 11, 2024 at 07:45:21PM +0530, Het Gala wrote: > > On 10/04/24 8:23 pm, Peter Xu wrote: > > !---| > >CAUTION: External Email > > > > |---! > > > > 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. > Do we want to add other transports too (vsock, exec, rdma) with the new > interface ? > I believe we have tests for fd based migration Het, What I meant is we used to do white box testing for migration, trying to cover all the code paths would suffice for us in that case. It means maybe we don't need the postcopy test to cover the channels interface as long as precopy has covered that and also if that covered all the "channels" abi then we should be safe. What I worry is we keep extending the test matrix but we're actually testing the same code paths. Then the test runs slower each time, we burn more cpus for each CI kick, but without a real beneift. Thanks, -- Peter Xu
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
On Wed, Apr 10, 2024 at 09:49:15AM -0400, Peter Xu wrote: > 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. Some people got dropped on the cc list for unknown reason, I'm adding them back (Fabiano, Peter Maydell, Phil). Let's make sure nobody is dropped by accident. I'll try to summarize what is still missing, and I think these will be greatly helpful if we don't want to deprecate rdma migration: 1) Either a CI test covering at least the major RDMA paths, or at least periodically tests for each QEMU release will be needed. 2) Some performance tests between modern RDMA and NIC devices are welcomed. The current knowledge is modern NIC can work similarly to RDMA in performance, then it's debatable why we still maintain so much rdma specific code. 3) No need to be soild patchsets for this one, but some plan to improve RDMA migration code so that it is not almost isolated from the rest protocols. 4) Someone to look after this code for real. For 2) and 3) more info is here: https://lore.kernel.org/r/ZhWa0YeAb9ySVKD1@x1n Here 4) can be the most important as Markus pointed out. We just didn't get there yet on the discussions, but maybe Markus is right that we should talk that first. Thanks, -- Peter Xu
Re: [PATCH v6 0/7] Resolve issues with booting distros on x86
On Thu, 04 Jan 2024 08:10:35 -0700, Simon Glass wrote: > This little series reprises the EFI-video fix, fixes a USB problem and > enables a boot script for coreboot. > > It also moves to truetype fonts for coreboot and qemu-x86, since the > menus look much better and there are no strong size constraints. > > With these changes it is possible to boot a Linux distro automatically > with U-Boot on x86, including when U-Boot is the second-stage > bootloader. > > [...] Applied to u-boot/master, thanks! -- Tom
Re: [PATCH for-9.1 09/19] target/i386: move 60-BF opcodes to new decoder
Hi Paolo,
I just did some tests,
> +[0x98] = X86_OP_ENTRY1(CBW,0,v), /* rAX */
> +[0x99] = X86_OP_ENTRY3(CWD,2,v, 0,v, None, None), /* rDX, rAX */
> +[0x9A] = X86_OP_ENTRYrr(CALLF, I_unsigned,p, I_unsigned,w, chk(i64)),
X86_TYPE_I_unsigned is defined in patch 11, so the related changes
should be move into this patch to avoid compiling failures:
--- a/target/i386/tcg/decode-new.c.inc
+++ b/target/i386/tcg/decode-new.c.inc
@@ -1642,6 +1642,11 @@ static bool decode_op(DisasContext *s, CPUX86State *env,
X86DecodedInsn *decode,
decode->immediate = op->imm = insn_get_signed(env, s, op->ot);
break;
+case X86_TYPE_I_unsigned: /* Immediate */
+op->unit = X86_OP_IMM;
+decode->immediate = op->imm = insn_get(env, s, op->ot);
+break;
+
case X86_TYPE_L: /* The upper 4 bits of the immediate select a 128-bit
register */
op->n = insn_get(env, s, op->ot) >> 4;
break;
diff --git a/target/i386/tcg/decode-new.h b/target/i386/tcg/decode-new.h
index ca99a620ce94..790ad5e1d006 100644
--- a/target/i386/tcg/decode-new.h
+++ b/target/i386/tcg/decode-new.h
@@ -48,6 +48,7 @@ typedef enum X86OpType {
/* Custom */
X86_TYPE_WM, /* modrm byte selects an XMM/YMM memory operand */
+X86_TYPE_I_unsigned, /* Immediate, zero-extended */
X86_TYPE_2op, /* 2-operand RMW instruction */
X86_TYPE_LoBits, /* encoded in bits 0-2 of the operand + REX.B */
X86_TYPE_0, /* Hard-coded GPRs (RAX..RDI) */
-Zhao
Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"
On 10/04/24 8:23 pm, Peter Xu wrote: !---| CAUTION: External Email |---! 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. Do we want to add other transports too (vsock, exec, rdma) with the new interface ? I believe we have tests for fd based migration Regards, Het Gala
Re: [PATCH v4 02/10] hw/core: create Resettable QOM interface
On Wed, 21 Aug 2019 at 17:34, Damien Hedde wrote:
>
> This commit defines an interface allowing multi-phase reset. This aims
> to solve a problem of the actual single-phase reset (built in
> DeviceClass and BusClass): reset behavior is dependent on the order
> in which reset handlers are called. In particular doing external
> side-effect (like setting an qemu_irq) is problematic because receiving
> object may not be reset yet.
So, I wanted to drag up this ancient patch to ask a couple
of Resettable questions, because I'm working on adding a
new ResetType (the equivalent of SHUTDOWN_CAUSE_SNAPSHOT_LOAD).
> +/**
> + * ResetType:
> + * Types of reset.
> + *
> + * + Cold: reset resulting from a power cycle of the object.
> + *
> + * TODO: Support has to be added to handle more types. In particular,
> + * ResetState structure needs to be expanded.
> + */
Does anybody remember what this TODO comment is about? What
in particular would need to be in the ResetState struct
to allow another type to be added?
> +typedef enum ResetType {
> +RESET_TYPE_COLD,
> +} ResetType;
> +typedef void (*ResettableInitPhase)(Object *obj, ResetType type);
> +typedef void (*ResettableHoldPhase)(Object *obj);
> +typedef void (*ResettableExitPhase)(Object *obj);
Was there a reason why we only pass the ResetType to the init
phase method, and not also to the hold and exit phases ?
Given that many devices don't need to implement init, it
seems awkward to require them to do so just to stash the
ResetType somewhere so they can look at it in the hold
or exit phase, so I was thinking about adding the argument
to the other two phase methods.
thanks
-- PMM
Re: [PATCH 4/4] tests/qtest/migration: Add postcopy migration qtests to use 'channels' argument instead of uri
On 10/04/24 6:45 pm, Fabiano Rosas wrote:
!---|
CAUTION: External Email
|---!
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.
I think, migrate_qmp has an unique advantage here. Please correct me If
my understanding
is not correct.
1. test_migrate_start starts the qemu cmd line with either --incoming
tcp:127.0.0.1:0
or "defer". If tcp uri is provided, then migrate_incoming_qmp is not
necessary
2. If "defer" is passed, then only migrate_incoming_qmp is required with
tcp uri string
3. migrate_qmp can get the uri anyway either from
test_migrate_start -> defer + migrate_incoming_qmp ->
tcp:127.0.0.1:0
test_migrate_start -> tcp:127.0.0.1:0
with the help of migrate_get_connect_uri() with the correct
migration port.
Even if we always pass NULL, we are okay, But this is just for tcp
transport, and
not unix
We can't have the unique situation for migrate_incoming_qmp, hence
avoiding conditional
earlier seemed to be necessary for me. But, with the hack suggested by
you in previous
patch, will prevent us from using conditional if else
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
wait_for_suspend(from, &src_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(&args);
}
+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(&args);
+}
+
static void test_postcopy_suspend(void)
{
MigrateCommon args = {
@@ -1555,6 +1573,18 @@ static void test_postcopy_recovery(void)
test_postcopy_recovery_common(&args);
}
+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(&args);
+}
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",
Regards,
Het Gala
Re: [PATCH v7 09/10] virtio-gpu: Support Venus capset
Hi, On 4/11/24 15:52, Antonio Caggiano wrote: > Hi Dmitry, > > I have a new version of this patch which you might want to include in > this series. > Please, you can find it below. > > I hope it would also solve the issue raised by Pierre-Eric in v6. AFAICS, this patch should be relevant only once DRM context will be supported, otherwise there is no problem to fix for now. Virgl1/2 always available if Venus available, isn't it? -- Best regards, Dmitry
Re: [PATCH 2/9] disas/microblaze: Replace sprintf() by snprintf()
On Thu, Apr 11, 2024 at 12:43 PM Philippe Mathieu-Daudé
wrote:
>
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> resulting in painful developper experience. Use snprintf() instead.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Edgar E. Iglesias
> ---
> disas/microblaze.c | 25 -
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/disas/microblaze.c b/disas/microblaze.c
> index 0b89b9c4fa..49a4c0fd40 100644
> --- a/disas/microblaze.c
> +++ b/disas/microblaze.c
> @@ -600,7 +600,8 @@ static char *
> get_field (long instr, long mask, unsigned short low)
> {
>char tmpstr[25];
> - sprintf(tmpstr, "%s%d", register_prefix, (int)((instr & mask) >> low));
> + snprintf(tmpstr, sizeof(tmpstr), "%s%d", register_prefix,
> + (int)((instr & mask) >> low));
>return(strdup(tmpstr));
> }
>
> @@ -608,7 +609,8 @@ static char *
> get_field_imm (long instr)
> {
>char tmpstr[25];
> - sprintf(tmpstr, "%d", (short)((instr & IMM_MASK) >> IMM_LOW));
> + snprintf(tmpstr, sizeof(tmpstr), "%d",
> + (short)((instr & IMM_MASK) >> IMM_LOW));
>return(strdup(tmpstr));
> }
>
> @@ -616,7 +618,8 @@ static char *
> get_field_imm5 (long instr)
> {
>char tmpstr[25];
> - sprintf(tmpstr, "%d", (short)((instr & IMM5_MASK) >> IMM_LOW));
> + snprintf(tmpstr, sizeof(tmpstr), "%d",
> + (short)((instr & IMM5_MASK) >> IMM_LOW));
>return(strdup(tmpstr));
> }
>
> @@ -624,7 +627,8 @@ static char *
> get_field_rfsl (long instr)
> {
>char tmpstr[25];
> - sprintf(tmpstr, "%s%d", fsl_register_prefix, (short)((instr & RFSL_MASK)
> >> IMM_LOW));
> + snprintf(tmpstr, sizeof(tmpstr), "%s%d", fsl_register_prefix,
> + (short)((instr & RFSL_MASK) >> IMM_LOW));
>return(strdup(tmpstr));
> }
>
> @@ -632,7 +636,8 @@ static char *
> get_field_imm15 (long instr)
> {
>char tmpstr[25];
> - sprintf(tmpstr, "%d", (short)((instr & IMM15_MASK) >> IMM_LOW));
> + snprintf(tmpstr, sizeof(tmpstr), "%d",
> + (short)((instr & IMM15_MASK) >> IMM_LOW));
>return(strdup(tmpstr));
> }
>
> @@ -641,7 +646,8 @@ static char *
> get_field_unsigned_imm (long instr)
> {
>char tmpstr[25];
> - sprintf(tmpstr, "%d", (int)((instr & IMM_MASK) >> IMM_LOW));
> + snprintf(tmpstr, sizeof(tmpstr), "%d",
> + (int)((instr & IMM_MASK) >> IMM_LOW));
>return(strdup(tmpstr));
> }
> #endif
> @@ -653,7 +659,8 @@ get_field_unsigned_imm (long instr)
>{
>char tmpstr[25];
>
> - sprintf(tmpstr, "%s%s", register_prefix, (((instr & IMM_MASK) >> IMM_LOW)
> & REG_MSR_MASK) == 0 ? "pc" : "msr");
> + snprintf(tmpstr, sizeof(tmpstr), "%s%s", register_prefix,
> + (((instr & IMM_MASK) >> IMM_LOW) & REG_MSR_MASK) == 0 ? "pc" :
> "msr");
>
>return(strdup(tmpstr));
>}
> @@ -709,7 +716,7 @@ get_field_special(long instr, const struct op_code_struct
> *op)
> default :
> {
> if ( instr & IMM_MASK) >> IMM_LOW) ^ op->immval_mask) & 0xE000)
> == REG_PVR_MASK) {
> - sprintf(tmpstr, "%s%u", pvr_register_prefix,
> + snprintf(tmpstr, sizeof(tmpstr), "%s%u", pvr_register_prefix,
> (unsigned short)(((instr & IMM_MASK) >> IMM_LOW) ^
>op->immval_mask) ^ REG_PVR_MASK);
> return(strdup(tmpstr));
> @@ -720,7 +727,7 @@ get_field_special(long instr, const struct op_code_struct
> *op)
> break;
> }
>
> - sprintf(tmpstr, "%s%s", register_prefix, spr);
> + snprintf(tmpstr, sizeof(tmpstr), "%s%s", register_prefix, spr);
> return(strdup(tmpstr));
> }
>
> --
> 2.41.0
>
Re: [PATCH v7 09/10] virtio-gpu: Support Venus capset
Hi Dmitry,
I have a new version of this patch which you might want to include in
this series.
Please, you can find it below.
I hope it would also solve the issue raised by Pierre-Eric in v6.
Cheers,
Antonio
---
virtio-gpu: Support Venus capset
While querying the number of capsets, map each index to the relative
capset ID, then reuse this mapping when querying for the capset info.
This is a flexible approach which allows to add support for new capsets
in the future more easily.
Then add support for the Venus capset, which enables Vulkan support through
the Venus Vulkan driver for virtio-gpu.
Signed-off-by: Antonio Caggiano
---
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 9f34d0e661..0e9e4ebcbb 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -371,17 +371,13 @@ static void virgl_cmd_get_capset_info(VirtIOGPU *g,
{
struct virtio_gpu_get_capset_info info;
struct virtio_gpu_resp_capset_info resp;
+VirtIOGPUGL* gl = VIRTIO_GPU_GL(g);
VIRTIO_GPU_FILL_CMD(info);
memset(&resp, 0, sizeof(resp));
-if (info.capset_index == 0) {
-resp.capset_id = VIRTIO_GPU_CAPSET_VIRGL;
-virgl_renderer_get_cap_set(resp.capset_id,
- &resp.capset_max_version,
- &resp.capset_max_size);
-} else if (info.capset_index == 1) {
-resp.capset_id = VIRTIO_GPU_CAPSET_VIRGL2;
+if (info.capset_index < VIRTIO_GPU_MAX_CAPSETS) {
+resp.capset_id = gl->capset_ids[info.capset_index];
virgl_renderer_get_cap_set(resp.capset_id,
&resp.capset_max_version,
&resp.capset_max_size);
@@ -658,10 +654,28 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
{
-uint32_t capset2_max_ver, capset2_max_size;
+uint32_t capset_max_ver, capset_max_size;
+uint32_t capset_count = 0;
+VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
+
+gl->capset_ids[capset_count] = VIRTIO_GPU_CAPSET_VIRGL;
+capset_count++;
+
virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VIRGL2,
- &capset2_max_ver,
- &capset2_max_size);
+ &capset_max_ver,
+ &capset_max_size);
+if (capset_max_ver) {
+gl->capset_ids[capset_count] = VIRTIO_GPU_CAPSET_VIRGL2;
+capset_count++;
+}
+
+virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VENUS,
+ &capset_max_ver,
+ &capset_max_size);
+if (capset_max_size) {
+gl->capset_ids[capset_count] = VIRTIO_GPU_CAPSET_VENUS;
+capset_count++;
+}
-return capset2_max_ver ? 2 : 1;
+return capset_count;
}
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index ed44cdad6b..c0e7cae42e 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -225,11 +225,15 @@ struct VirtIOGPUClass {
Error **errp);
};
+#define VIRTIO_GPU_MAX_CAPSETS 8
+
struct VirtIOGPUGL {
struct VirtIOGPU parent_obj;
bool renderer_inited;
bool renderer_reset;
+
+int capset_ids[VIRTIO_GPU_MAX_CAPSETS];
};
struct VhostUserGPU {
On 11/04/2024 12:20, Dmitry Osipenko wrote:
From: Antonio Caggiano
Add support for the Venus capset, which enables Vulkan support through
the Venus Vulkan driver for virtio-gpu.
Signed-off-by: Antonio Caggiano
Signed-off-by: Huang Rui
Signed-off-by: Dmitry Osipenko
---
hw/display/virtio-gpu-virgl.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index e01ab8295d4d..0d8f00c7939a 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -517,6 +517,11 @@ static void virgl_cmd_get_capset_info(VirtIOGPU *g,
virgl_renderer_get_cap_set(resp.capset_id,
&resp.capset_max_version,
&resp.capset_max_size);
+} else if (info.capset_index == 2) {
+resp.capset_id = VIRTIO_GPU_CAPSET_VENUS;
+virgl_renderer_get_cap_set(resp.capset_id,
+ &resp.capset_max_version,
+ &resp.capset_max_size);
} else {
resp.capset_max_version = 0;
resp.capset_max_size = 0;
@@ -1067,10 +1072,18 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
{
-uint32_t capset2_max_ver, capset2_max_size;
+uint32_t capset2_max_ver, capset2_max_size, num_capsets;
+num_capsets = 1;
+
virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VIRGL2,
- &capset2_max_ver,
- &capset2_max_size);
+ &caps
Re: [PATCH 3/4] tests/qtest/migration: Add channels parameter in migrate_incoming_qmp
On 10/04/24 6:44 pm, Fabiano Rosas wrote:
!---|
CAUTION: External Email
|---!
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, &error_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.
Yes, this hack would silently solve, avoiding the above check.
IMO, it is good to have like to like QAPIs while running a qtest.
If the qtest uses the new syntax then, both 'migrate' and 'migrate-incoming'
QAPIs should use the new syntax. Even though implementation wise, it makes
no difference (qtests run successfully) but it would be good to ensure,
we have
grammatical correctness.
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, &pair[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,
Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"
On 10/04/24 6:34 pm, Fabiano Rosas wrote: !---| CAUTION: External Email |---! 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? Yes, I had little to no experience on how to introduce 'channels' as a new argument in the migration QAPIs back then. IIRC, while trying to merge the series (migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration), I was adviced to just modify one of the qtest with the new QAPI syntax rather than writing a new qtest altogether. So, I just renamed the old syntax with the new one. 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. Thanks for the support for building that framework. Today we can independently call channel or uri easily for 'migrate' QAPI Reviewed-by: Fabiano Rosas Regards, Het Gala
[PATCH v5 1/3] qio: add support for SO_PEERCRED for socket channel
The function qio_channel_get_peercred() returns a pointer to the
credentials of the peer process connected to this socket.
This credentials structure is defined in as follows:
struct ucred {
pid_t pid;/* Process ID of the sending process */
uid_t uid;/* User ID of the sending process */
gid_t gid;/* Group ID of the sending process */
};
The use of this function is possible only for connected AF_UNIX stream
sockets and for AF_UNIX stream and datagram socket pairs.
On platform other than Linux, the function return 0.
Signed-off-by: Anthony Harivel
---
include/io/channel.h | 21 +
io/channel-socket.c | 28
io/channel.c | 13 +
3 files changed, 62 insertions(+)
diff --git a/include/io/channel.h b/include/io/channel.h
index 7986c49c713a..bdf0bca92ae2 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -160,6 +160,9 @@ struct QIOChannelClass {
void *opaque);
int (*io_flush)(QIOChannel *ioc,
Error **errp);
+int (*io_peerpid)(QIOChannel *ioc,
+ unsigned int *pid,
+ Error **errp);
};
/* General I/O handling functions */
@@ -981,4 +984,22 @@ int coroutine_mixed_fn
qio_channel_writev_full_all(QIOChannel *ioc,
int qio_channel_flush(QIOChannel *ioc,
Error **errp);
+/**
+ * qio_channel_get_peercred:
+ * @ioc: the channel object
+ * @pid: pointer to pid
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Returns the pid of the peer process connected to this socket.
+ *
+ * The use of this function is possible only for connected
+ * AF_UNIX stream sockets and for AF_UNIX stream and datagram
+ * socket pairs on Linux.
+ * Return -1 on error with pid -1 for the non-Linux OS.
+ *
+ */
+int qio_channel_get_peerpid(QIOChannel *ioc,
+ unsigned int *pid,
+ Error **errp);
+
#endif /* QIO_CHANNEL_H */
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 3a899b060858..608bcf066ecd 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -841,6 +841,33 @@ qio_channel_socket_set_cork(QIOChannel *ioc,
socket_set_cork(sioc->fd, v);
}
+static int
+qio_channel_socket_get_peerpid(QIOChannel *ioc,
+ unsigned int *pid,
+ Error **errp)
+{
+#ifdef CONFIG_LINUX
+QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
+Error *err = NULL;
+socklen_t len = sizeof(struct ucred);
+
+struct ucred cred;
+if (getsockopt(sioc->fd,
+ SOL_SOCKET, SO_PEERCRED,
+ &cred, &len) == -1) {
+error_setg_errno(&err, errno, "Unable to get peer credentials");
+error_propagate(errp, err);
+*pid = -1;
+return -1;
+}
+*pid = (unsigned int)cred.pid;
+return 0;
+#else
+error_setg(errp, "Unsupported feature");
+*pid = -1;
+return -1;
+#endif
+}
static int
qio_channel_socket_close(QIOChannel *ioc,
@@ -938,6 +965,7 @@ static void qio_channel_socket_class_init(ObjectClass
*klass,
#ifdef QEMU_MSG_ZEROCOPY
ioc_klass->io_flush = qio_channel_socket_flush;
#endif
+ioc_klass->io_peerpid = qio_channel_socket_get_peerpid;
}
static const TypeInfo qio_channel_socket_info = {
diff --git a/io/channel.c b/io/channel.c
index a1f12f8e9096..e3f17c24a00f 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -548,6 +548,19 @@ void qio_channel_set_cork(QIOChannel *ioc,
}
}
+int qio_channel_get_peerpid(QIOChannel *ioc,
+ unsigned int *pid,
+ Error **errp)
+{
+QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc);
+
+if (!klass->io_peerpid) {
+error_setg(errp, "Channel does not support peer pid");
+return -1;
+}
+klass->io_peerpid(ioc, pid, errp);
+return 0;
+}
off_t qio_channel_io_seek(QIOChannel *ioc,
off_t offset,
--
2.44.0
[PATCH v5 3/3] Add support for RAPL MSRs in KVM/Qemu
Starting with the "Sandy Bridge" generation, Intel CPUs provide a RAPL
interface (Running Average Power Limit) for advertising the accumulated
energy consumption of various power domains (e.g. CPU packages, DRAM,
etc.).
The consumption is reported via MSRs (model specific registers) like
MSR_PKG_ENERGY_STATUS for the CPU package power domain. These MSRs are
64 bits registers that represent the accumulated energy consumption in
micro Joules. They are updated by microcode every ~1ms.
For now, KVM always returns 0 when the guest requests the value of
these MSRs. Use the KVM MSR filtering mechanism to allow QEMU handle
these MSRs dynamically in userspace.
To limit the amount of system calls for every MSR call, create a new
thread in QEMU that updates the "virtual" MSR values asynchronously.
Each vCPU has its own vMSR to reflect the independence of vCPUs. The
thread updates the vMSR values with the ratio of energy consumed of
the whole physical CPU package the vCPU thread runs on and the
thread's utime and stime values.
All other non-vCPU threads are also taken into account. Their energy
consumption is evenly distributed among all vCPUs threads running on
the same physical CPU package.
To overcome the problem that reading the RAPL MSR requires priviliged
access, a socket communication between QEMU and the qemu-vmsr-helper is
mandatory. You can specified the socket path in the parameter.
This feature is activated with -accel kvm,rapl=true,path=/path/sock.sock
Actual limitation:
- Works only on Intel host CPU because AMD CPUs are using different MSR
adresses.
- Only the Package Power-Plane (MSR_PKG_ENERGY_STATUS) is reported at
the moment.
Signed-off-by: Anthony Harivel
---
accel/kvm/kvm-all.c | 27 +++
docs/specs/index.rst | 1 +
docs/specs/rapl-msr.rst | 155
include/sysemu/kvm.h | 2 +
include/sysemu/kvm_int.h | 32 +++
target/i386/cpu.h | 8 +
target/i386/kvm/kvm-cpu.c | 9 +
target/i386/kvm/kvm.c | 428 ++
target/i386/kvm/meson.build | 1 +
target/i386/kvm/vmsr_energy.c | 335 ++
target/i386/kvm/vmsr_energy.h | 99
11 files changed, 1097 insertions(+)
create mode 100644 docs/specs/rapl-msr.rst
create mode 100644 target/i386/kvm/vmsr_energy.c
create mode 100644 target/i386/kvm/vmsr_energy.h
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index a8cecd040ebc..7649f226767a 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -3613,6 +3613,21 @@ static void kvm_set_device(Object *obj,
s->device = g_strdup(value);
}
+static void kvm_set_kvm_rapl(Object *obj, bool value, Error **errp)
+{
+KVMState *s = KVM_STATE(obj);
+s->msr_energy.enable = value;
+}
+
+static void kvm_set_kvm_rapl_socket_path(Object *obj,
+ const char *str,
+ Error **errp)
+{
+KVMState *s = KVM_STATE(obj);
+g_free(s->msr_energy.socket_path);
+s->msr_energy.socket_path = g_strdup(str);
+}
+
static void kvm_accel_instance_init(Object *obj)
{
KVMState *s = KVM_STATE(obj);
@@ -3632,6 +3647,7 @@ static void kvm_accel_instance_init(Object *obj)
s->xen_gnttab_max_frames = 64;
s->xen_evtchn_max_pirq = 256;
s->device = NULL;
+s->msr_energy.enable = false;
}
/**
@@ -3676,6 +3692,17 @@ static void kvm_accel_class_init(ObjectClass *oc, void
*data)
object_class_property_set_description(oc, "device",
"Path to the device node to use (default: /dev/kvm)");
+object_class_property_add_bool(oc, "rapl",
+ NULL,
+ kvm_set_kvm_rapl);
+object_class_property_set_description(oc, "rapl",
+"Allow energy related MSRs for RAPL interface in Guest");
+
+object_class_property_add_str(oc, "rapl-helper-socket", NULL,
+ kvm_set_kvm_rapl_socket_path);
+object_class_property_set_description(oc, "rapl-helper-socket",
+"Socket Path for comminucating with the Virtual MSR helper daemon");
+
kvm_arch_accel_class_init(oc);
}
diff --git a/docs/specs/index.rst b/docs/specs/index.rst
index 1484e3e76077..e738ea7d102f 100644
--- a/docs/specs/index.rst
+++ b/docs/specs/index.rst
@@ -33,3 +33,4 @@ guest hardware that is specific to QEMU.
virt-ctlr
vmcoreinfo
vmgenid
+ rapl-msr
diff --git a/docs/specs/rapl-msr.rst b/docs/specs/rapl-msr.rst
new file mode 100644
index ..1202ee89bee0
--- /dev/null
+++ b/docs/specs/rapl-msr.rst
@@ -0,0 +1,155 @@
+
+RAPL MSR support
+
+
+The RAPL interface (Running Average Power Limit) is advertising the accumulated
+energy consumption of various power domains (e.g. CPU packages, DRAM, etc.).
+
+The consumption is reported via MSRs (model specific registers) like
+MSR_PKG_ENERGY_STATUS for the CPU package power domain. These MSRs ar
[PATCH v5 0/3] Add support for the RAPL MSRs series
Dear maintainers, First of all, thank you very much for your review of my patch [1]. In this version (v5), I have attempted to address all the problems addressed by Daniel during the last review. I've been more careful with all the remarks made. However, one question remains unanswered pointing the issue with the location of "/var/local/run/qemu-vmsr-helper.sock", created by compute_default_paths(). QEMU is not allowed to reach the socket here. Thank you again for your continued guidance. v4 -> v5 - correct qio_channel_get_peerpid: return pid = -1 in case of error - Vmsr_helper: compile only for x86 - Vmsr_helper: use qio_channel_read/write_all - Vmsr_helper: abandon user/group - Vmsr_energy.c: correct all error_report - Vmsr thread: compute default socket path only once - Vmsr thread: open socket only once - Pass relevant QEMU CI v3 -> v4 - Correct memory leaks with AddressSanitizer - Add sanity check for QEMU and qemu-vmsr-helper for checking if host is INTEL and if RAPL is activated. - Rename poor variables naming for easier comprehension - Move code that checks Host before creating the VMSR thread - Get rid of libnuma: create function that read sysfs for reading the Host topology instead v2 -> v3 - Move all memory allocations from Clib to Glib - Compile on *BSD (working on Linux only) - No more limitation on the virtual package: each vCPU that belongs to the same virtual package is giving the same results like expected on a real CPU. This has been tested topology like: -smp 4,sockets=2 -smp 16,sockets=4,cores=2,threads=2 v1 -> v2 - To overcome the CVE-2020-8694 a socket communication is created to a priviliged helper - Add the priviliged helper (qemu-vmsr-helper) - Add SO_PEERCRED in qio channel socket RFC -> v1 - - Add vmsr_* in front of all vmsr specific function - Change malloc()/calloc()... with all glib equivalent - Pre-allocate all dynamic memories when possible - Add a Documentation of implementation, limitation and usage Best regards, Anthony [1]: https://lists.gnu.org/archive/html/qemu-devel/2024-03/msg04417.html Anthony Harivel (3): qio: add support for SO_PEERCRED for socket channel tools: build qemu-vmsr-helper Add support for RAPL MSRs in KVM/Qemu accel/kvm/kvm-all.c | 27 ++ contrib/systemd/qemu-vmsr-helper.service | 15 + contrib/systemd/qemu-vmsr-helper.socket | 9 + docs/specs/index.rst | 1 + docs/specs/rapl-msr.rst | 155 +++ docs/tools/index.rst | 1 + docs/tools/qemu-vmsr-helper.rst | 89 include/io/channel.h | 21 + include/sysemu/kvm.h | 2 + include/sysemu/kvm_int.h | 32 ++ io/channel-socket.c | 28 ++ io/channel.c | 13 + meson.build | 7 + target/i386/cpu.h| 8 + target/i386/kvm/kvm-cpu.c| 9 + target/i386/kvm/kvm.c| 428 ++ target/i386/kvm/meson.build | 1 + target/i386/kvm/vmsr_energy.c| 335 ++ target/i386/kvm/vmsr_energy.h| 99 + tools/i386/qemu-vmsr-helper.c| 529 +++ tools/i386/rapl-msr-index.h | 28 ++ 21 files changed, 1837 insertions(+) create mode 100644 contrib/systemd/qemu-vmsr-helper.service create mode 100644 contrib/systemd/qemu-vmsr-helper.socket create mode 100644 docs/specs/rapl-msr.rst create mode 100644 docs/tools/qemu-vmsr-helper.rst create mode 100644 target/i386/kvm/vmsr_energy.c create mode 100644 target/i386/kvm/vmsr_energy.h create mode 100644 tools/i386/qemu-vmsr-helper.c create mode 100644 tools/i386/rapl-msr-index.h -- 2.44.0
[PATCH v5 2/3] tools: build qemu-vmsr-helper
Introduce a privileged helper to access RAPL MSR. The privileged helper tool, qemu-vmsr-helper, is designed to provide virtual machines with the ability to read specific RAPL (Running Average Power Limit) MSRs without requiring CAP_SYS_RAWIO privileges or relying on external, out-of-tree patches. The helper tool leverages Unix permissions and SO_PEERCRED socket options to enforce access control, ensuring that only processes explicitly requesting read access via readmsr() from a valid Thread ID can access these MSRs. The list of RAPL MSRs that are allowed to be read by the helper tool is defined in rapl-msr-index.h. This list corresponds to the RAPL MSRs that will be supported in the next commit titled "Add support for RAPL MSRs in KVM/QEMU." The tool is intentionally designed to run on the Linux x86 platform. This initial implementation is tailored for Intel CPUs but can be extended to support AMD CPUs in the future. Signed-off-by: Anthony Harivel --- contrib/systemd/qemu-vmsr-helper.service | 15 + contrib/systemd/qemu-vmsr-helper.socket | 9 + docs/tools/index.rst | 1 + docs/tools/qemu-vmsr-helper.rst | 89 meson.build | 7 + tools/i386/qemu-vmsr-helper.c| 529 +++ tools/i386/rapl-msr-index.h | 28 ++ 7 files changed, 678 insertions(+) create mode 100644 contrib/systemd/qemu-vmsr-helper.service create mode 100644 contrib/systemd/qemu-vmsr-helper.socket create mode 100644 docs/tools/qemu-vmsr-helper.rst create mode 100644 tools/i386/qemu-vmsr-helper.c create mode 100644 tools/i386/rapl-msr-index.h diff --git a/contrib/systemd/qemu-vmsr-helper.service b/contrib/systemd/qemu-vmsr-helper.service new file mode 100644 index ..8fd397bf79a9 --- /dev/null +++ b/contrib/systemd/qemu-vmsr-helper.service @@ -0,0 +1,15 @@ +[Unit] +Description=Virtual RAPL MSR Daemon for QEMU + +[Service] +WorkingDirectory=/tmp +Type=simple +ExecStart=/usr/bin/qemu-vmsr-helper +PrivateTmp=yes +ProtectSystem=strict +ReadWritePaths=/var/run +RestrictAddressFamilies=AF_UNIX +Restart=always +RestartSec=0 + +[Install] diff --git a/contrib/systemd/qemu-vmsr-helper.socket b/contrib/systemd/qemu-vmsr-helper.socket new file mode 100644 index ..183e8304d6e2 --- /dev/null +++ b/contrib/systemd/qemu-vmsr-helper.socket @@ -0,0 +1,9 @@ +[Unit] +Description=Virtual RAPL MSR helper for QEMU + +[Socket] +ListenStream=/run/qemu-vmsr-helper.sock +SocketMode=0600 + +[Install] +WantedBy=multi-user.target diff --git a/docs/tools/index.rst b/docs/tools/index.rst index 8e65ce0dfc7b..33ad438e86f6 100644 --- a/docs/tools/index.rst +++ b/docs/tools/index.rst @@ -16,3 +16,4 @@ command line utilities and other standalone programs. qemu-pr-helper qemu-trace-stap virtfs-proxy-helper + qemu-vmsr-helper diff --git a/docs/tools/qemu-vmsr-helper.rst b/docs/tools/qemu-vmsr-helper.rst new file mode 100644 index ..6ec87b49d962 --- /dev/null +++ b/docs/tools/qemu-vmsr-helper.rst @@ -0,0 +1,89 @@ +== +QEMU virtual RAPL MSR helper +== + +Synopsis + + +**qemu-vmsr-helper** [*OPTION*] + +Description +--- + +Implements the virtual RAPL MSR helper for QEMU. + +Accessing the RAPL (Running Average Power Limit) MSR enables the RAPL powercap +driver to advertise and monitor the power consumption or accumulated energy +consumption of different power domains, such as CPU packages, DRAM, and other +components when available. + +However those register are accesible under priviliged access (CAP_SYS_RAWIO). +QEMU can use an external helper to access those priviliged register. + +:program:`qemu-vmsr-helper` is that external helper; it creates a listener +socket which will accept incoming connections for communication with QEMU. + +If you want to run VMs in a setup like this, this helper should be started as a +system service, and you should read the QEMU manual section on "RAPL MSR +support" to find out how to configure QEMU to connect to the socket created by +:program:`qemu-vmsr-helper`. + +After connecting to the socket, :program:`qemu-vmsr-helper` can +optionally drop root privileges, except for those capabilities that +are needed for its operation. + +:program:`qemu-vmsr-helper` can also use the systemd socket activation +protocol. In this case, the systemd socket unit should specify a +Unix stream socket, like this:: + +[Socket] +ListenStream=/var/run/qemu-vmsr-helper.sock + +Options +--- + +.. program:: qemu-vmsr-helper + +.. option:: -d, --daemon + + run in the background (and create a PID file) + +.. option:: -q, --quiet + + decrease verbosity + +.. option:: -v, --verbose + + increase verbosity + +.. option:: -f, --pidfile=PATH + + PID file when running as a daemon. By default the PID file + is created in the system runtime state directory, for example + :file:`/var/run/qemu-vmsr-helper.pid`. + +.. opti
Re: [PATCH for-9.0] meson.build: Disable -fzero-call-used-regs on OpenBSD
On 11/04/2024 14.08, Thomas Huth wrote:
QEMU currently does not work on OpenBSD since the -fzero-call-used-regs
That should be "OpenBSD 7.5" ... older versions are fine since they are
using an older version of Clang that does not have -fzero-call-used-regs
yet, I think.
Thomas
option that we added to meson.build recently does not work with the
"retguard" extension from OpenBSD's Clang. Thus let's disable the
-fzero-call-used-regs here until there's a better solution available.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2278
Signed-off-by: Thomas Huth
---
Note: Given that we're close to the release, I think the host_os check
is the best we can do ... the problem does not seem to trigger in all
functions, only if certain registers are used by the compiler, so a
more sophisticated check here seems to be too fragile to me right now.
meson.build | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index c9c3217ba4..91a0aa64c6 100644
--- a/meson.build
+++ b/meson.build
@@ -562,7 +562,11 @@ hardening_flags = [
#
# NB: Clang 17 is broken and SEGVs
# https://github.com/llvm/llvm-project/issues/75168
-if cc.compiles('extern struct { void (*cb)(void); } s; void f(void) { s.cb();
}',
+#
+# NB2: This clashes with the "retguard" extension of OpenBSD's Clang
+# https://gitlab.com/qemu-project/qemu/-/issues/2278
+if host_os != 'openbsd' and \
+ cc.compiles('extern struct { void (*cb)(void); } s; void f(void) { s.cb();
}',
name: '-fzero-call-used-regs=used-gpr',
args: ['-O2', '-fzero-call-used-regs=used-gpr'])
hardening_flags += '-fzero-call-used-regs=used-gpr'
[PATCH for-9.0] meson.build: Disable -fzero-call-used-regs on OpenBSD
QEMU currently does not work on OpenBSD since the -fzero-call-used-regs
option that we added to meson.build recently does not work with the
"retguard" extension from OpenBSD's Clang. Thus let's disable the
-fzero-call-used-regs here until there's a better solution available.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2278
Signed-off-by: Thomas Huth
---
Note: Given that we're close to the release, I think the host_os check
is the best we can do ... the problem does not seem to trigger in all
functions, only if certain registers are used by the compiler, so a
more sophisticated check here seems to be too fragile to me right now.
meson.build | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index c9c3217ba4..91a0aa64c6 100644
--- a/meson.build
+++ b/meson.build
@@ -562,7 +562,11 @@ hardening_flags = [
#
# NB: Clang 17 is broken and SEGVs
# https://github.com/llvm/llvm-project/issues/75168
-if cc.compiles('extern struct { void (*cb)(void); } s; void f(void) { s.cb();
}',
+#
+# NB2: This clashes with the "retguard" extension of OpenBSD's Clang
+# https://gitlab.com/qemu-project/qemu/-/issues/2278
+if host_os != 'openbsd' and \
+ cc.compiles('extern struct { void (*cb)(void); } s; void f(void) { s.cb();
}',
name: '-fzero-call-used-regs=used-gpr',
args: ['-O2', '-fzero-call-used-regs=used-gpr'])
hardening_flags += '-fzero-call-used-regs=used-gpr'
--
2.44.0
Re: [PATCH 1/9] disas/m68k: Replace sprintf() by snprintf()
On Thu, 11 Apr 2024 at 11:44, Philippe Mathieu-Daudé wrote: > > sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1, > resulting in painful developper experience. Use snprintf() instead. > > Signed-off-by: Philippe Mathieu-Daudé > --- > disas/m68k.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: Peter Maydell thanks -- PMM
Re: [PATCH 3/9] disas/riscv: Replace sprintf() by snprintf()
On Thu, 11 Apr 2024 at 11:44, Philippe Mathieu-Daudé wrote: > > sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1, > resulting in painful developper experience. Use snprintf() instead. > > Signed-off-by: Philippe Mathieu-Daudé > --- Reviewed-by: Peter Maydell thanks -- PMM
Re: [PATCH 9/9] target/i386: Replace sprintf() by snprintf()
On Thu, 11 Apr 2024 at 11:44, Philippe Mathieu-Daudé wrote: > > sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1, > resulting in painful developper experience. Use snprintf() instead. > > Signed-off-by: Philippe Mathieu-Daudé > --- > target/i386/kvm/kvm.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c > index e68cbe9293..a46d1426bf 100644 > --- a/target/i386/kvm/kvm.c > +++ b/target/i386/kvm/kvm.c > @@ -5335,7 +5335,8 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run > *run) > case KVM_EXIT_NOTIFY: > ctx_invalid = !!(run->notify.flags & KVM_NOTIFY_CONTEXT_INVALID); > state = KVM_STATE(current_accel()); > -sprintf(str, "Encounter a notify exit with %svalid context in" > +snprintf(str, sizeof(str), > + "Encounter a notify exit with %svalid context in" > " guest. There can be possible misbehaves in guest." > " Please have a look.", ctx_invalid ? "in" : ""); > if (ctx_invalid || > -- This is a case where I think we would be better off with g_strdup_printf(): * the buffer declaration is a long way away from its use * the string is long and it's not trivial to confirm that it will fit in the buffer * it's quite plausible somebody will come along later to clean up the wording of the error message and not notice they need to enlarge the buffer * it's only for printing a warning, so it's not going to be in a hot codepath thanks -- PMM
[PATCH] linux-user/flatload.c: Remove unused bFLT shared-library and ZFLAT code
Ever since the bFLT format support was added in 2006, there has been
a chunk of code in the file guarded by CONFIG_BINFMT_SHARED_FLAT
which is supposedly for shared library support. This is not enabled
and it's not possible to enable it, because if you do you'll run into
the "#error needs checking" in the calc_reloc() function.
Similarly, CONFIG_BINFMT_ZFLAT exists but can't be enabled because of
an "#error code needs checking" in load_flat_file().
This code is obviously unfinished and has never been used; nobody in
the intervening 18 years has complained about this or fixed it, so
just delete the dead code. If anybody ever wants the feature they
can always pull it out of git, or (perhaps better) write it from
scratch based on the current Linux bFLT loader rather than the one of
18 years ago.
Signed-off-by: Peter Maydell
---
linux-user/flat.h | 5 +-
linux-user/flatload.c | 293 ++
2 files changed, 11 insertions(+), 287 deletions(-)
diff --git a/linux-user/flat.h b/linux-user/flat.h
index ed518e2013b..e374b73e268 100644
--- a/linux-user/flat.h
+++ b/linux-user/flat.h
@@ -12,11 +12,8 @@
#defineFLAT_VERSION0x0004L
-#ifdef CONFIG_BINFMT_SHARED_FLAT
-#defineMAX_SHARED_LIBS (4)
-#else
+/* QEMU doesn't support bflt shared libraries */
#defineMAX_SHARED_LIBS (1)
-#endif
/*
* To make everything easier to port and manage cross platform
diff --git a/linux-user/flatload.c b/linux-user/flatload.c
index 5b62aa0a2be..04d8138d12e 100644
--- a/linux-user/flatload.c
+++ b/linux-user/flatload.c
@@ -29,8 +29,6 @@
* JAN/99 -- coded full program relocation (g...@snapgear.com)
*/
-/* ??? ZFLAT and shared library support is currently disabled. */
-
//
#include "qemu/osdep.h"
@@ -64,10 +62,6 @@ struct lib_info {
short loaded; /* Has this library been loaded? */
};
-#ifdef CONFIG_BINFMT_SHARED_FLAT
-static int load_flat_shared_library(int id, struct lib_info *p);
-#endif
-
struct linux_binprm;
//
@@ -108,153 +102,6 @@ static int target_pread(int fd, abi_ulong ptr, abi_ulong
len,
unlock_user(buf, ptr, len);
return ret;
}
-//
-
-#ifdef CONFIG_BINFMT_ZFLAT
-
-#include
-
-#define LBUFSIZE 4000
-
-/* gzip flag byte */
-#define ASCII_FLAG 0x01 /* bit 0 set: file probably ASCII text */
-#define CONTINUATION 0x02 /* bit 1 set: continuation of multi-part gzip file */
-#define EXTRA_FIELD 0x04 /* bit 2 set: extra field present */
-#define ORIG_NAME0x08 /* bit 3 set: original file name present */
-#define COMMENT 0x10 /* bit 4 set: file comment present */
-#define ENCRYPTED0x20 /* bit 5 set: file is encrypted */
-#define RESERVED 0xC0 /* bit 6,7: reserved */
-
-static int decompress_exec(
- struct linux_binprm *bprm,
- unsigned long offset,
- char *dst,
- long len,
- int fd)
-{
- unsigned char *buf;
- z_stream strm;
- loff_t fpos;
- int ret, retval;
-
- DBG_FLT("decompress_exec(offset=%x,buf=%x,len=%x)\n",(int)offset,
(int)dst, (int)len);
-
- memset(&strm, 0, sizeof(strm));
- strm.workspace = kmalloc(zlib_inflate_workspacesize(), GFP_KERNEL);
- if (strm.workspace == NULL) {
- DBG_FLT("binfmt_flat: no memory for decompress workspace\n");
- return -ENOMEM;
- }
- buf = kmalloc(LBUFSIZE, GFP_KERNEL);
- if (buf == NULL) {
- DBG_FLT("binfmt_flat: no memory for read buffer\n");
- retval = -ENOMEM;
- goto out_free;
- }
-
- /* Read in first chunk of data and parse gzip header. */
- fpos = offset;
- ret = bprm->file->f_op->read(bprm->file, buf, LBUFSIZE, &fpos);
-
- strm.next_in = buf;
- strm.avail_in = ret;
- strm.total_in = 0;
-
- retval = -ENOEXEC;
-
- /* Check minimum size -- gzip header */
- if (ret < 10) {
- DBG_FLT("binfmt_flat: file too small?\n");
- goto out_free_buf;
- }
-
- /* Check gzip magic number */
- if ((buf[0] != 037) || ((buf[1] != 0213) && (buf[1] != 0236))) {
- DBG_FLT("binfmt_flat: unknown compression magic?\n");
- goto out_free_buf;
- }
-
- /* Check gzip method */
- if (buf[2] != 8) {
- DBG_FLT("binfmt_flat: unknown compression method?\n");
- goto out_free_buf;
- }
- /* Check gzip flags */
- if ((buf[3] & ENCRYPTED) || (buf[3] & CONTINUATION) ||
- (buf[3] & RESERVED)) {
- DBG_FLT("binfmt_flat: unknown flags?\n");
- goto out_free_buf;
- }
-
- ret = 10;
- if (buf[3] & EXTR
Re: [PATCH 4/9] linux-user/flatload: Replace sprintf() by snprintf()
On Thu, 11 Apr 2024 at 11:44, Philippe Mathieu-Daudé wrote:
>
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> resulting in painful developper experience. Use snprintf() instead.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> linux-user/flatload.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/linux-user/flatload.c b/linux-user/flatload.c
> index 5b62aa0a2b..b0f04af4b6 100644
> --- a/linux-user/flatload.c
> +++ b/linux-user/flatload.c
> @@ -692,7 +692,7 @@ static int load_flat_shared_library(int id, struct
> lib_info *libs)
> char buf[16];
>
> /* Create the file name */
> - sprintf(buf, "/lib/lib%d.so", id);
> +snprintf(buf, sizeofbuf), "/lib/lib%d.so", id);
Indentation has gone wrong, and another missing '('.
Looking at that, I notice that this function is actually
dead code -- it's in code that is only compiled and called
if CONFIG_BINFMT_SHARED_FLAT is defined, and if you try to
define that you will hit the "#error needs checking" in the
calc_reloc() function. It seems to have been like this since
the flatload support was added in 2006.
So I think we should remove the dead code -- I'll send a patch.
thanks
-- PMM
[PATCH] linux-headers: change the annotation of VFIO_IOMMU_SPAPR_REGISTER_MEMORY in vfio.h
The ioctl(VFIO_IOMMU_MAP_DMA/VFIO_IOMMU_UNMAP_DMA) won't be called
in SPAPR machine, which is replaced by VFIO_IOMMU_SPAPR_TCE_CREATE/
VFIO_IOMMU_SPAPR_TCE_REMOVE, so change the description.
Signed-off-by: JianChunfu
---
linux-headers/linux/vfio.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index b4be37b22..20e8e6ae8 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -1764,7 +1764,7 @@ struct vfio_eeh_pe_op {
*
* Registers user space memory where DMA is allowed. It pins
* user pages and does the locked memory accounting so
- * subsequent VFIO_IOMMU_MAP_DMA/VFIO_IOMMU_UNMAP_DMA calls
+ * subsequent VFIO_IOMMU_SPAPR_TCE_CREATE/VFIO_IOMMU_SPAPR_TCE_REMOVE calls
* get faster.
*/
struct vfio_iommu_spapr_register_memory {
--
2.27.0
[PATCH] target/riscv: fix instructions count handling in icount mode
When icount is enabled, rather than returning the virtual CPU time, we
should return the instruction count itself. Add an instructions bool
parameter to get_ticks() to correctly return icount_get_raw() when
icount_enabled() == 1 and instruction count is queried. This will modify
the existing behavior which was returning an instructions count close to
the number of cycles (CPI ~= 1).
Signed-off-by: Clément Léger
---
target/riscv/csr.c | 29 -
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 726096444f..5f1dcee102 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -762,14 +762,17 @@ static RISCVException write_vcsr(CPURISCVState *env, int
csrno,
}
/* User Timers and Counters */
-static target_ulong get_ticks(bool shift)
+static target_ulong get_ticks(bool shift, bool instructions)
{
int64_t val;
target_ulong result;
#if !defined(CONFIG_USER_ONLY)
if (icount_enabled()) {
-val = icount_get();
+if (instructions)
+val = icount_get_raw();
+else
+val = icount_get();
} else {
val = cpu_get_host_ticks();
}
@@ -804,14 +807,14 @@ static RISCVException read_timeh(CPURISCVState *env, int
csrno,
static RISCVException read_hpmcounter(CPURISCVState *env, int csrno,
target_ulong *val)
{
-*val = get_ticks(false);
+*val = get_ticks(false, (csrno == CSR_INSTRET));
return RISCV_EXCP_NONE;
}
static RISCVException read_hpmcounterh(CPURISCVState *env, int csrno,
target_ulong *val)
{
-*val = get_ticks(true);
+*val = get_ticks(true, (csrno == CSR_INSTRETH));
return RISCV_EXCP_NONE;
}
@@ -875,11 +878,11 @@ static RISCVException write_mhpmcounter(CPURISCVState
*env, int csrno,
int ctr_idx = csrno - CSR_MCYCLE;
PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
uint64_t mhpmctr_val = val;
+bool instr = riscv_pmu_ctr_monitor_instructions(env, ctr_idx);
counter->mhpmcounter_val = val;
-if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
-riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
-counter->mhpmcounter_prev = get_ticks(false);
+if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) || instr) {
+counter->mhpmcounter_prev = get_ticks(false, instr);
if (ctr_idx > 2) {
if (riscv_cpu_mxl(env) == MXL_RV32) {
mhpmctr_val = mhpmctr_val |
@@ -902,12 +905,12 @@ static RISCVException write_mhpmcounterh(CPURISCVState
*env, int csrno,
PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
uint64_t mhpmctr_val = counter->mhpmcounter_val;
uint64_t mhpmctrh_val = val;
+bool instr = riscv_pmu_ctr_monitor_instructions(env, ctr_idx);
counter->mhpmcounterh_val = val;
mhpmctr_val = mhpmctr_val | (mhpmctrh_val << 32);
-if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
-riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
-counter->mhpmcounterh_prev = get_ticks(true);
+if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) || instr) {
+counter->mhpmcounterh_prev = get_ticks(true, instr);
if (ctr_idx > 2) {
riscv_pmu_setup_timer(env, mhpmctr_val, ctr_idx);
}
@@ -926,6 +929,7 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState
*env, target_ulong *val,
counter->mhpmcounter_prev;
target_ulong ctr_val = upper_half ? counter->mhpmcounterh_val :
counter->mhpmcounter_val;
+bool instr = riscv_pmu_ctr_monitor_instructions(env, ctr_idx);
if (get_field(env->mcountinhibit, BIT(ctr_idx))) {
/*
@@ -946,9 +950,8 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState
*env, target_ulong *val,
* The kernel computes the perf delta by subtracting the current value from
* the value it initialized previously (ctr_val).
*/
-if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
-riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
-*val = get_ticks(upper_half) - ctr_prev + ctr_val;
+if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) || instr) {
+*val = get_ticks(upper_half, instr) - ctr_prev + ctr_val;
} else {
*val = ctr_val;
}
--
2.43.0
Re: [PATCH 8/9] target/arm: Replace sprintf() by snprintf()
On Thu, 11 Apr 2024 at 11:44, Philippe Mathieu-Daudé wrote:
>
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> resulting in painful developper experience. Use snprintf() instead.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> target/arm/cpu64.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 985b1efe16..f0f4fe6714 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -433,7 +433,7 @@ void aarch64_add_sve_properties(Object *obj)
>
> for (vq = 1; vq <= ARM_MAX_VQ; ++vq) {
> char name[8];
> -sprintf(name, "sve%d", vq * 128);
> +snprintf(name, sizeof(name), "sve%d", vq * 128);
> object_property_add(obj, name, "bool", cpu_arm_get_vq,
> cpu_arm_set_vq, NULL, &cpu->sve_vq);
> }
> @@ -458,7 +458,7 @@ void aarch64_add_sme_properties(Object *obj)
>
> for (vq = 1; vq <= ARM_MAX_VQ; vq <<= 1) {
> char name[8];
> -sprintf(name, "sme%d", vq * 128);
> +snprintf(name, sizeof(name), "sme%d", vq * 128);
> object_property_add(obj, name, "bool", cpu_arm_get_vq,
> cpu_arm_set_vq, NULL, &cpu->sme_vq);
> }
Reviewed-by: Peter Maydell
thanks
-- PMM
Re: [PATCH v9 13/20] virtio-net: Return an error when vhost cannot enable RSS
On Mon, Apr 8, 2024 at 4:31 AM Akihiko Odaki wrote:
>
> On 2024/04/08 6:46, Yuri Benditovich wrote:
> > On Wed, Apr 3, 2024 at 2:11 PM Akihiko Odaki
> > wrote:
> >>
> >> vhost requires eBPF for RSS. When eBPF is not available, virtio-net
> >> implicitly disables RSS even if the user explicitly requests it. Return
> >> an error instead of implicitly disabling RSS if RSS is requested but not
> >> available.
> >>
> >> Signed-off-by: Akihiko Odaki
> >> ---
> >> hw/net/virtio-net.c | 97
> >> ++---
> >> 1 file changed, 48 insertions(+), 49 deletions(-)
> >>
> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index 61b49e335dea..3d53eba88cfc 100644
> >> --- a/hw/net/virtio-net.c
> >> +++ b/hw/net/virtio-net.c
> >> @@ -793,9 +793,6 @@ static uint64_t virtio_net_get_features(VirtIODevice
> >> *vdev, uint64_t features,
> >> return features;
> >> }
> >>
> >> -if (!ebpf_rss_is_loaded(&n->ebpf_rss)) {
> >> -virtio_clear_feature(&features, VIRTIO_NET_F_RSS);
> >> -}
> >> features = vhost_net_get_features(get_vhost_net(nc->peer), features);
> >> vdev->backend_features = features;
> >>
> >> @@ -3591,6 +3588,50 @@ static bool
> >> failover_hide_primary_device(DeviceListener *listener,
> >> return qatomic_read(&n->failover_primary_hidden);
> >> }
> >>
> >> +static void virtio_net_device_unrealize(DeviceState *dev)
> >> +{
> >> +VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >> +VirtIONet *n = VIRTIO_NET(dev);
> >> +int i, max_queue_pairs;
> >> +
> >> +if (virtio_has_feature(n->host_features, VIRTIO_NET_F_RSS)) {
> >> +virtio_net_unload_ebpf(n);
> >> +}
> >> +
> >> +/* This will stop vhost backend if appropriate. */
> >> +virtio_net_set_status(vdev, 0);
> >> +
> >> +g_free(n->netclient_name);
> >> +n->netclient_name = NULL;
> >> +g_free(n->netclient_type);
> >> +n->netclient_type = NULL;
> >> +
> >> +g_free(n->mac_table.macs);
> >> +g_free(n->vlans);
> >> +
> >> +if (n->failover) {
> >> +qobject_unref(n->primary_opts);
> >> +device_listener_unregister(&n->primary_listener);
> >> +migration_remove_notifier(&n->migration_state);
> >> +} else {
> >> +assert(n->primary_opts == NULL);
> >> +}
> >> +
> >> +max_queue_pairs = n->multiqueue ? n->max_queue_pairs : 1;
> >> +for (i = 0; i < max_queue_pairs; i++) {
> >> +virtio_net_del_queue(n, i);
> >> +}
> >> +/* delete also control vq */
> >> +virtio_del_queue(vdev, max_queue_pairs * 2);
> >> +qemu_announce_timer_del(&n->announce_timer, false);
> >> +g_free(n->vqs);
> >> +qemu_del_nic(n->nic);
> >> +virtio_net_rsc_cleanup(n);
> >> +g_free(n->rss_data.indirections_table);
> >> +net_rx_pkt_uninit(n->rx_pkt);
> >> +virtio_cleanup(vdev);
> >> +}
> >> +
> >> static void virtio_net_device_realize(DeviceState *dev, Error **errp)
> >> {
> >> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >> @@ -3760,53 +3801,11 @@ static void virtio_net_device_realize(DeviceState
> >> *dev, Error **errp)
> >>
> >> net_rx_pkt_init(&n->rx_pkt);
> >>
> >> -if (virtio_has_feature(n->host_features, VIRTIO_NET_F_RSS)) {
> >> -virtio_net_load_ebpf(n);
> >> -}
> >> -}
> >> -
> >> -static void virtio_net_device_unrealize(DeviceState *dev)
> >> -{
> >> -VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >> -VirtIONet *n = VIRTIO_NET(dev);
> >> -int i, max_queue_pairs;
> >> -
> >> -if (virtio_has_feature(n->host_features, VIRTIO_NET_F_RSS)) {
> >> -virtio_net_unload_ebpf(n);
> >> +if (virtio_has_feature(n->host_features, VIRTIO_NET_F_RSS) &&
> >
> > I disagree with this change of qemu behavior.
> > From my point of view:
> > - this is not a major problem and it should not be a reason to stop VM
> > execution
> > - it is enough to disable the RSS feature and continue working. Depending on
> >other qemu parameters (number of queues, number of cpus) this might be
> > just
> >suboptimal. might be a minor problem and might be not a problem at all
>
I think the basic example is when the kernel doesn't support ebpf
loading (either old kernel or kernel that, for some security reason,
disabled the capability). In this case, we will get a behavior change.
Best regards,
Yan.
>
> The reasoning is that we shouldn't disable what the user explicitly
> requested. c.f.,
> https://lore.kernel.org/all/20231102091717-mutt-send-email-...@kernel.org/
>
> > - this change defines rss as _only_ feature whose absence breaks the VM
> > start,
> >_all_ other features are dropped silently and only rss is not. Why??
>
> I'm following what QEMU does in the other places rather than what it
> does just in virtio-net. I have pointed out virtio-gpu raises errors in
> such a situation. c.f.,
> https://lore.kernel.org/all/8880b6f9-f556-46f7-a191-eeec0fe20...@daynix.com
>
> > - the series has a title 'Fixes and impr
Re: [PATCH 6/9] hw/net/rocker: Replace sprintf() by snprintf()
On Thu, 11 Apr 2024 at 11:47, Philippe Mathieu-Daudé wrote:
>
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> resulting in painful developper experience. Use snprintf() instead.
("developer")
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/net/rocker/rocker.c | 24
> 1 file changed, 12 insertions(+), 12 deletions(-)
> switch (offset) {
> case ROCKER_DMA_DESC_ADDR_OFFSET:
> -sprintf(buf, "Ring[%s] ADDR", ring_name);
> +snprintf(buf, sizeofbuf), "Ring[%s] ADDR", ring_name);
Something seems to have gone wrong here. Shouldn't this have
failed to compile ?
thanks
-- PMM
Re: [PATCH for-9.1 09/19] target/i386: move 60-BF opcodes to new decoder
On Thu, Apr 11, 2024 at 9:47 AM Richard Henderson wrote: > > +case MO_32: > > +#ifdef TARGET_X86_64 > > +/* > > + * This could also use the same algorithm as MO_16. It produces > > fewer > > + * TCG ops and better code if flags are needed, but it requires a > > 64-bit > > + * multiply even if they are not (and thus the high part of the > > multiply > > + * is dead). > > + */ > > Is 64-bit multiply ever slower these days? > My intuition says "slow" multiply is at least a decade out of date. I was thinking more about TCG_TARGET_REG_BITS == 32. > > +tcg_gen_negsetcondi_i32(TCG_COND_LT, s->tmp2_i32, s->tmp2_i32, 0); > > This seems like something the optimizer should handle, but doesn't. I wanted to avoid using TARGET_LONG_BITS - 1, but it's not a problem to use extract. I've changed it. At least the ppc and x86 backends do support it and convert it to SAR, so I didn't notice in my test that it was the backend doing it and not the optimizer! Paolo
