Re: [PATCH 0/1 RFC] FUSE Export Coroutine Integration Cover Letter
On Mon, Mar 24, 2025 at 04:05:09PM +0800, saz97 wrote: > This patch series refactors QEMU's FUSE export module to leverage coroutines > for read/write operations, > addressing concurrency limitations and aligning with QEMU's asynchronous I/O > model. The changes > demonstrate measurable performance improvements while simplifying resource > management. > > 1. technology implementation > >according to Stefan suggerstion, i move the processing logic of > read_from_fuse_export into a coroutine for buffer management. >and change the fuse_getattr to call: bdrv_co_get_allocated_file_size(). > > 2. performance summary > >For the coroutine_integration_fuse test, the average results for iodepth=1 > and iodepth=64 are as follows: > --- > Average results for iodepth=1: > Read_IOPS: coroutine_integration_fuse: 4492.88 | origin: 4309.39 | 4.25% > improvement > Write_IOPS: coroutine_integration_fuse: 4500.68 | origin: 4318.68 | 4.21% > improvement > Read_BW: coroutine_integration_fuse: 17971.00 KB/s | origin: 17237.30 > KB/s | 4.26% improvement > Write_BW: coroutine_integration_fuse: 18002.50 KB/s | origin: 17274.30 > KB/s | 4.23% improvement > > --- > Average results for iodepth=64: > Read_IOPS: coroutine_integration_fuse: 5576.93 | origin: 5347.13 | 4.29% > improvement > Write_IOPS: coroutine_integration_fuse: 5569.55 | origin: 5337.33 | 4.33% > improvement > Read_BW: coroutine_integration_fuse: 22311.40 KB/s | origin: 21392.20 > KB/s | 4.31% improvement > Write_BW: coroutine_integration_fuse: 22282.20 KB/s | origin: 21353.20 > KB/s | 4.34% improvement > >Although all metrics show improvements, the gains are concentrated in the > 4.2%–4.3% range, which is lower than expected. Further investigation using > gprof reveals the reasons for this limited improvement. > > 3. Performance Bottlenecks Identified via gprof >After running a fio test with the following command: >fio --ioengine=io_uring --numjobs=1 --runtime=30 --ramp_time=5 \ > --rw=randrw --bs=4k --time_based=1 --name=job1 \ > --filename=/mnt/qemu-fuse --iopath=64 >and analyzing the execution profile using gprof, the following issues were > identified: > >3.1 Increased Overall Execution Time >In the original implementation, fuse_write + blk_pwrite accounted for 8.7% > of total execution time (6.0% + 2.7%). >After refactoring, fuse_write_coroutine + blk_co_pwrite now accounts for > 43.1% (22.9% + 20.2%). >This suggests that coroutine overhead is contributing significantly to > execution time. > >3.2 Increased Read and Write Calls >fuse_write calls increased from 173,400 → 333,232. >fuse_read calls increased from 173,526 → 332,931. >This indicates that the coroutine-based approach is introducing redundant > I/O calls, likely due to unnecessary coroutine switches. > >3.3 Significant Coroutine Overhead >qemu_coroutine_enter is now called 1,572,803 times, compared to ~476,057 > previously. >This frequent coroutine switching introduces unnecessary overhead, > limiting the expected performance improvements. Due to the remaining performance issues, let's leave this contribution task here. Please focus on submitting your Google Summer of Code application at https://summerofcode.withgoogle.com/ by April 8th. Thanks, Stefan > > saz97 (1): > Integration coroutines into fuse export > > block/export/fuse.c | 190 +--- > 1 file changed, 126 insertions(+), 64 deletions(-) > > -- > 2.34.1 > signature.asc Description: PGP signature
Re: [PATCH] vfio: Open code vfio_migration_set_error()
On 24/03/2025 14:33, Cédric Le Goater wrote:
External email: Use caution opening links or attachments
VFIO uses migration_file_set_error() in a couple of places where an
'Error **' parameter is not provided. In MemoryListener handlers :
vfio_listener_region_add
vfio_listener_log_global_stop
vfio_listener_log_sync
and in callback routines for IOMMU notifiers :
vfio_iommu_map_notify
vfio_iommu_map_dirty_notify
Hopefully, one day, we will be able to extend these callbacks with an
'Error **' parameter and avoid setting the global migration error.
Until then, it seems sensible to clearly identify the use cases, which
are limited, and open code vfio_migration_set_error(). One other
benefit is an improved error reporting when migration is running.
While at it, slightly modify error reporting to only report errors
when migration is not active and not always as is currently done.
Cc: Prasad Pandit
Signed-off-by: Cédric Le Goater
---
hw/vfio/common.c | 60 +---
1 file changed, 36 insertions(+), 24 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index
1a0d9290f88c9774a98f65087a36b86922b21a73..a591ce5b97ff41cdc8249e9eeafc8dc347d45fac
100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -149,13 +149,6 @@ bool vfio_viommu_preset(VFIODevice *vbasedev)
return vbasedev->bcontainer->space->as != &address_space_memory;
}
-static void vfio_set_migration_error(int ret)
-{
-if (migration_is_running()) {
-migration_file_set_error(ret, NULL);
-}
-}
Wouldn't it be better to extend vfio_set_migration_error() to take also
Error* instead of duplicating code?
We can rename it to vfio_set_error() if it's not solely related to vfio
migration anymore.
Thanks.
-
bool vfio_device_state_is_running(VFIODevice *vbasedev)
{
VFIOMigration *migration = vbasedev->migration;
@@ -291,9 +284,14 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n,
IOMMUTLBEntry *iotlb)
iova, iova + iotlb->addr_mask);
if (iotlb->target_as != &address_space_memory) {
-error_report("Wrong target AS \"%s\", only system memory is allowed",
- iotlb->target_as->name ? iotlb->target_as->name : "none");
-vfio_set_migration_error(-EINVAL);
+error_setg(&local_err,
+ "Wrong target AS \"%s\", only system memory is allowed",
+ iotlb->target_as->name ? iotlb->target_as->name : "none");
+if (migration_is_running()) {
+migration_file_set_error(-EINVAL, local_err);
+} else {
+error_report_err(local_err);
+}
return;
}
@@ -326,11 +324,16 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n,
IOMMUTLBEntry *iotlb)
ret = vfio_container_dma_unmap(bcontainer, iova,
iotlb->addr_mask + 1, iotlb);
if (ret) {
-error_report("vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
- "0x%"HWADDR_PRIx") = %d (%s)",
- bcontainer, iova,
- iotlb->addr_mask + 1, ret, strerror(-ret));
-vfio_set_migration_error(ret);
+error_setg(&local_err,
+ "vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
+ "0x%"HWADDR_PRIx") = %d (%s)",
+ bcontainer, iova,
+ iotlb->addr_mask + 1, ret, strerror(-ret));
+if (migration_is_running()) {
+migration_file_set_error(ret, local_err);
+} else {
+error_report_err(local_err);
+}
}
}
out:
@@ -1112,8 +1115,11 @@ static void vfio_listener_log_global_stop(MemoryListener
*listener)
if (ret) {
error_prepend(&local_err,
"vfio: Could not stop dirty page tracking - ");
-error_report_err(local_err);
-vfio_set_migration_error(ret);
+if (migration_is_running()) {
+migration_file_set_error(ret, local_err);
+} else {
+error_report_err(local_err);
+}
}
}
@@ -1229,14 +1235,14 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier
*n, IOMMUTLBEntry *iotlb)
trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
if (iotlb->target_as != &address_space_memory) {
-error_report("Wrong target AS \"%s\", only system memory is allowed",
- iotlb->target_as->name ? iotlb->target_as->name : "none");
+error_setg(&local_err,
+ "Wrong target AS \"%s\", only system memory is allowed",
+ iotlb->target_as->name ? iotlb->target_as->name : "none");
goto out;
}
rcu_read_lock();
if (!vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL, &local_err))
{
-error_report_err(local_err);
goto out_unlock;
}
@
Re: [PATCH] docs/devel/build-environment: enhance MSYS2 instructions
On 3/5/25 13:38, Pierrick Bouvier wrote: Signed-off-by: Pierrick Bouvier --- docs/devel/build-environment.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/devel/build-environment.rst b/docs/devel/build-environment.rst index f133ef2e012..661f6ea8504 100644 --- a/docs/devel/build-environment.rst +++ b/docs/devel/build-environment.rst @@ -97,11 +97,11 @@ build QEMU in MSYS2 itself. :: -pacman -S wget +pacman -S wget base-devel git wget https://raw.githubusercontent.com/msys2/MINGW-packages/refs/heads/master/mingw-w64-qemu/PKGBUILD # Some packages may be missing for your environment, installation will still # be done though. -makepkg -s PKGBUILD || true +makepkg --syncdeps --nobuild PKGBUILD || true Build on windows-aarch64 Gentle ping on this trivial change for doc. Thanks, Pierrick
Re: [PATCH v6] hw/misc/vmfwupdate: Introduce hypervisor fw-cfg interface support
On Mon, Mar 24, 2025 at 05:31:30PM +0100, Alexander Graf wrote: > > > > > > What does all this mean for the hypervisor interface ? > > > > That means we'll go scratch the region list idea and depend on igvm > > > > instead. > > > > Which means we are back to the single firmware image. > > > So in this model, how are we passing the hashes of kernel, initrd and > > > cmdline? > > > Are they going to be part of the firmware image as was previously thought? > > Either scratch the idea of using hashes to verify kernel + initrd + > > cmdline and use a secure boot signed UKI instead, or use IGVM firmware > > images and add the kernel hashes page to the igvm. > It's a nice idea to have a firmware IGVM file that contains a signature, so > you can for example have a RHEL provided IGVM that only runs UKIs that are > signed by Red Hat. Yep, that is what should be possible when all this is ready. > - Attestable Bootable Containers. In that case, the command line contains > a hash of the target fs-verity protected partition. Red Hat can not be the > entity signing that UKI. It must be the user. Well, add-ons have been created to address exactly that: Allow the UKI being configured without changing it, so the UKI can be signed by redhat. You'll go add user-signed add-on for the command line. > - Add-ons. How do you provide a "debug add-on" and prevent it to gain any > access to confidential data? Not sure I understand the point you are trying to raise. Add-ons signatures are checked too. > So we need some equivalent of a hash page. Ok. So two opaque blobs, one which is measured into the launch measurement and one which is not? That gives you the option to pass around hashes (or any other data) and leave a trace in the launch measurement should you need that. > That can absolutely integrate more deeply into UEFI and be actual PE > hashes rather than the icky thing the OVMF code does today. How the measured opaque blob is actually used is not something vmfwupdate needs to care about. > But we need to support a way to embed the hash in the ukify.py > generated IGVM on the fly. With add-ons, maybe even multiple ones. I'd prefer not patch the IGVM on the fly at boot time. take care, Gerd
Re: [PATCH v2 09/11] target/ppc: convert gdbstub to new helpers
On 3/24/25 03:21, Alex Bennée wrote: +#ifdef TARGET_BIG_ENDIAN +MemOp end = MO_BE; +#else +MemOp end = MO_LE; +#endif +#endif That's what MO_TE is for. +/* + * Helpers copied from helpers.h just for handling target_ulong values + * from gdbstub's GByteArray based on what the build config is. This + * will need fixing for single-binary. + */ + +#if TARGET_LONG_BITS == 64 +#define ldtul_p(addr) ldq_p(addr) +#define gdb_get_regl_value(m, b, v) gdb_get_reg64_value(m, b, v) +#else +#define ldtul_p(addr) ldl_p(addr) +#define gdb_get_regl_value(m, b, v) gdb_get_reg32_value(m, b, v) +#endif Surely you're not intending to replicate this in any target that can have multiple sizes? This is not an improvement. r~
Re: [PATCH] target/s390x: Fix a typo in s390_cpu_class_init()
On 24/3/25 07:05, Thomas Huth wrote:
On 23/03/2025 16.30, Philippe Mathieu-Daudé wrote:
Fixes: 41868f846d2 ("s390x/cpumodel: "host" and "qemu" as CPU
subclasses")
Signed-off-by: Philippe Mathieu-Daudé
---
target/s390x/cpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index d73142600bf..1f75629ddc2 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -377,7 +377,7 @@ static void s390_cpu_class_init(ObjectClass *oc,
void *data)
resettable_class_set_parent_phases(rc, NULL,
s390_cpu_reset_hold, NULL,
&scc->parent_phases);
- cc->class_by_name = s390_cpu_class_by_name,
+ cc->class_by_name = s390_cpu_class_by_name;
Please add a proper patch description next time. I spent dozens of
seconds to spot the typo in one of the words 'til I realized that it is
the comma at the end ;-)
Sorry I thought it was trivial, but since it got unnoticed during
8 years, maybe not.
Reviewed-by: Thomas Huth
Re: [PATCH-for-10.1 v2 4/7] tcg: Remove use of TCG_GUEST_DEFAULT_MO in tb_gen_code()
On 3/21/25 11:15, Philippe Mathieu-Daudé wrote: Use TCGCPUOps::guest_default_memory_order to set TCGContext::guest_mo. Signed-off-by: Philippe Mathieu-Daudé --- accel/tcg/translate-all.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index fb9f83dbba3..26442e83776 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -349,7 +349,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, tcg_ctx->tlb_dyn_max_bits = CPU_TLB_DYN_MAX_BITS; #endif tcg_ctx->insn_start_words = TARGET_INSN_START_WORDS; -tcg_ctx->guest_mo = TCG_GUEST_DEFAULT_MO; +tcg_ctx->guest_mo = cpu->cc->tcg_ops->guest_default_memory_order; restart_translate: trace_translate_block(tb, pc, tb->tc.ptr); Reviewed-by: Richard Henderson r~
Re: [PATCH v6] hw/misc/vmfwupdate: Introduce hypervisor fw-cfg interface support
On Mon, Mar 24, 2025 at 06:53:59PM +0100, Gerd Hoffman wrote: > On Mon, Mar 24, 2025 at 05:31:30PM +0100, Alexander Graf wrote: > > > > > > > > What does all this mean for the hypervisor interface ? > > > > > > That means we'll go scratch the region list idea and depend on igvm > > > > > instead. > > > > > > Which means we are back to the single firmware image. > > > > So in this model, how are we passing the hashes of kernel, initrd and > > > > cmdline? > > > > Are they going to be part of the firmware image as was previously > > > > thought? > > > > Either scratch the idea of using hashes to verify kernel + initrd + > > > cmdline and use a secure boot signed UKI instead, or use IGVM firmware > > > images and add the kernel hashes page to the igvm. > > > It's a nice idea to have a firmware IGVM file that contains a signature, so > > you can for example have a RHEL provided IGVM that only runs UKIs that are > > signed by Red Hat. > > Yep, that is what should be possible when all this is ready. > > > - Attestable Bootable Containers. In that case, the command line contains > > a hash of the target fs-verity protected partition. Red Hat can not be the > > entity signing that UKI. It must be the user. > > Well, add-ons have been created to address exactly that: Allow the UKI > being configured without changing it, so the UKI can be signed by redhat. > You'll go add user-signed add-on for the command line. > > > - Add-ons. How do you provide a "debug add-on" and prevent it to gain any > > access to confidential data? > > Not sure I understand the point you are trying to raise. Add-ons > signatures are checked too. "Debug add-on" is quite a broad concept - some things might be safe for the vendor to ship as pre-built signed add-ons by default, others things may not be safe & require user opt-in, by signing an add-on locally with their MOK and enrolling with it shim. Also UKIs semi-recently gained support for multiple profiles avoiding the need for addons in some cases[1]. A simple use of this might be two cmdlines - one quiet by default and one with verbose kernel messages. A more advanced use would be one "normal" profile, and one "factory reset" profile With regards, Daniel [1] https://github.com/uapi-group/specifications/blob/main/specs/unified_kernel_image.md#multi-profile-ukis -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH-for-10.1 v2 0/7] cpus: Convert cpu_list definition to CPUClass:list_cpus callback
On 24/3/25 19:59, Pierrick Bouvier wrote: On 3/24/25 11:46, Philippe Mathieu-Daudé wrote: Since v1 (Thomas review comments) - Move s390_set_qemu_cpu_model/s390_cpu_list to "cpu_models.h" - Correct 'target/s390x: Register CPUClass:list_cpus' subject 'cpu_list' might be defined per target, and force code to be built per-target. We can avoid that by introducing a CPUClass callback. This series combined with another which converts CPU_RESOLVING_TYPE to a runtime helper, allows to move most of cpu-target to common. I think we should focus on a more general solution, usable with machines, cpus, and devices, like the interface based approached we talked about. Having an optional callback, implemented on half target, looks like a half baked solution. And it would not be desirable to do the same thing for machines, and devices later. In case CPU_RESOLVING_TYPE is ambiguous (because 64 bit type inherit from 32 bit type, like TYPE_AARCH64_CPU inherits from TYPE_ARM_CPU), it's better to declare another interface, non ambiguous, like: - TYPE_MACHINE_TARGET_AARCH64 - TYPE_MACHINE_TARGET_ARM - TYPE_CPU_TARGET_AARCH64 - TYPE_CPU_TARGET_ARM - TYPE_DEVICE_TARGET_AARCH64 - TYPE_DEVICE_TARGET_ARM I would be in favor to introduce this for all targets, so all of them are implemented in the exact same way, and the pattern is easy to follow. By using this approach and tagging machines/cpus/devices accordingly, there is no need for any callback anywhere, and the listing code can be generic. As a bonus, we can assert all machines/cpus are properly tagged, something impossible to do with the callbacks approach, which opens room for a possible error, if one of the callback is buggy. This is exactly the plan (and what I've in my local branch). But since we need to remove the list_cpus definition (otherwise clash) I went with this surgical cleanup first.
Re: [PATCH v2 04/11] gdbstub: introduce target independent gdb register helper
On 3/24/25 03:21, Alex Bennée wrote:
The current helper.h functions rely on hard coded assumptions about
target endianess to use the tswap macros. We also end up double
swapping a bunch of values if the target can run in multiple endianess
modes. Avoid this by getting the target to pass the endianess and size
via a MemOp and fixing up appropriately.
Reviewed-by: Pierrick Bouvier
Signed-off-by: Alex Bennée
---
v2
- use unsigned consistently
- fix some rouge whitespace
- add typed reg32/64 wrappers
- pass void * to underlying helper to avoid casting
---
include/gdbstub/registers.h | 55 +
gdbstub/gdbstub.c | 23
2 files changed, 78 insertions(+)
create mode 100644 include/gdbstub/registers.h
diff --git a/include/gdbstub/registers.h b/include/gdbstub/registers.h
new file mode 100644
index 00..2220f58efe
--- /dev/null
+++ b/include/gdbstub/registers.h
@@ -0,0 +1,55 @@
+/*
+ * GDB Common Register Helpers
+ *
+ * Copyright (c) 2025 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef GDB_REGISTERS_H
+#define GDB_REGISTERS_H
+
+#include "exec/memop.h"
+
+/**
+ * gdb_get_register_value() - get register value for gdb
+ * mo: size and endian MemOp
+ * buf: GByteArray to store in target order
+ * val: pointer to value in host order
+ *
+ * This replaces the previous legacy read functions with a single
+ * function to handle all sizes. Passing @mo allows the target mode to
+ * be taken into account and avoids using hard coded tswap() macros.
+ *
+ * There are wrapper functions for the common sizes you can use to
+ * keep type checking.
+ *
+ * Returns the number of bytes written to the array.
+ */
+int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
+
+/**
+ * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
+ * mo: size and endian MemOp
+ * buf: GByteArray to store in target order
+ * val: pointer to uint32_t value in host order
+ */
+static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t
*val) {
+g_assert((op & MO_SIZE) == MO_32);
+return gdb_get_register_value(op, buf, val);
+}
+
+/**
+ * gdb_get_reg64_value() - type checked wrapper for gdb_get_register_value()
+ * mo: size and endian MemOp
+ * buf: GByteArray to store in target order
+ * val: pointer to uint32_t value in host order
+ */
+static inline int gdb_get_reg64_value(MemOp op, GByteArray *buf, uint64_t
*val) {
+g_assert((op & MO_SIZE) == MO_64);
+return gdb_get_register_value(op, buf, val);
+}
+
Maybe we could simply expect endian information from MemOp, and add
required size here.
It clutters call sites to have gdb_get_regX_value(MO_X | ...).
We can eventually assert no size was set, so it's not added by mistake
from callers.
+#endif /* GDB_REGISTERS_H */
+
+
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index b6d5e11e03..e799fdc019 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -32,6 +32,7 @@
#include "exec/gdbstub.h"
#include "gdbstub/commands.h"
#include "gdbstub/syscalls.h"
+#include "gdbstub/registers.h"
#ifdef CONFIG_USER_ONLY
#include "accel/tcg/vcpu-state.h"
#include "gdbstub/user.h"
@@ -45,6 +46,7 @@
#include "system/runstate.h"
#include "exec/replay-core.h"
#include "exec/hwaddr.h"
+#include "exec/memop.h"
#include "internals.h"
@@ -551,6 +553,27 @@ static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
return 0;
}
+/*
+ * Target helper function to read value into GByteArray, target
+ * supplies the size and target endianess via the MemOp.
+ */
+int gdb_get_register_value(MemOp op, GByteArray *buf, void *val)
+{
+unsigned bytes = memop_size(op);
+
+if (op & MO_BSWAP) {
+uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
+for (unsigned i = bytes; i > 0; i--) {
+g_byte_array_append(buf, ptr--, 1);
+};
+} else {
+g_byte_array_append(buf, val, bytes);
+}
+
+return bytes;
+}
+
+
static void gdb_register_feature(CPUState *cpu, int base_reg,
gdb_get_reg_cb get_reg, gdb_set_reg_cb
set_reg,
const GDBFeature *feature)
[PATCH-for-10.1 v2 1/7] cpus: Introduce CPUClass::list_cpus() callback
Some targets define cpu_list to a method listing their
CPUs on stdout. In order to make list_cpus() generic,
introduce the CPUClass::list_cpus() callback.
When no callback is registered, list_cpus() defaults
to the cpu_list definition.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
---
include/hw/core/cpu.h | 2 ++
cpu-target.c | 8 +++-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 54570d21aea..b633766ee8f 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -102,6 +102,7 @@ struct SysemuCPUOps;
* CPUClass:
* @class_by_name: Callback to map -cpu command line model name to an
* instantiatable CPU type.
+ * @list_cpus: list available CPU models and flags.
* @parse_features: Callback to parse command line arguments.
* @reset_dump_flags: #CPUDumpFlags to use for reset logging.
* @mmu_index: Callback for choosing softmmu mmu index;
@@ -150,6 +151,7 @@ struct CPUClass {
/*< public >*/
ObjectClass *(*class_by_name)(const char *cpu_model);
+void (*list_cpus)(void);
void (*parse_features)(const char *typename, char *str, Error **errp);
int (*mmu_index)(CPUState *cpu, bool ifetch);
diff --git a/cpu-target.c b/cpu-target.c
index cae77374b38..5947ca31a0a 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -98,7 +98,13 @@ static void cpu_list(void)
void list_cpus(void)
{
-cpu_list();
+CPUClass *cc = CPU_CLASS(object_class_by_name(CPU_RESOLVING_TYPE));
+
+if (cc->list_cpus) {
+cc->list_cpus();
+} else {
+cpu_list();
+}
}
/* enable or disable single step mode. EXCP_DEBUG is returned by the
--
2.47.1
Re: [PATCH-for-10.1 v2 5/7] target/s390x: Declare s390_set_qemu_cpu_model/cpu_list in cpu_models.h
On 3/24/25 11:46, Philippe Mathieu-Daudé wrote: Both s390_cpu_list() and s390_set_qemu_cpu_model() are defined in cpu_models.c, move their declarations in the related "cpu_models.h" header. Use full path to header in s390-virtio-ccw.c file. Suggested-by: Thomas Huth Signed-off-by: Philippe Mathieu-Daudé --- target/s390x/cpu.h | 4 target/s390x/cpu_models.h | 3 +++ hw/s390x/s390-virtio-ccw.c | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 5b7992deda6..8dd1936e3e2 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -900,11 +900,7 @@ static inline uint8_t s390_cpu_get_state(S390CPU *cpu) } -/* cpu_models.c */ -void s390_cpu_list(void); Are you really able to remove this here, without the next patch? r~
Re: [PATCH-for-10.1 v2 0/7] cpus: Convert cpu_list definition to CPUClass:list_cpus callback
On 3/24/25 11:46, Philippe Mathieu-Daudé wrote: Since v1 (Thomas review comments) - Move s390_set_qemu_cpu_model/s390_cpu_list to "cpu_models.h" - Correct 'target/s390x: Register CPUClass:list_cpus' subject 'cpu_list' might be defined per target, and force code to be built per-target. We can avoid that by introducing a CPUClass callback. This series combined with another which converts CPU_RESOLVING_TYPE to a runtime helper, allows to move most of cpu-target to common. I think we should focus on a more general solution, usable with machines, cpus, and devices, like the interface based approached we talked about. Having an optional callback, implemented on half target, looks like a half baked solution. And it would not be desirable to do the same thing for machines, and devices later. In case CPU_RESOLVING_TYPE is ambiguous (because 64 bit type inherit from 32 bit type, like TYPE_AARCH64_CPU inherits from TYPE_ARM_CPU), it's better to declare another interface, non ambiguous, like: - TYPE_MACHINE_TARGET_AARCH64 - TYPE_MACHINE_TARGET_ARM - TYPE_CPU_TARGET_AARCH64 - TYPE_CPU_TARGET_ARM - TYPE_DEVICE_TARGET_AARCH64 - TYPE_DEVICE_TARGET_ARM I would be in favor to introduce this for all targets, so all of them are implemented in the exact same way, and the pattern is easy to follow. By using this approach and tagging machines/cpus/devices accordingly, there is no need for any callback anywhere, and the listing code can be generic. As a bonus, we can assert all machines/cpus are properly tagged, something impossible to do with the callbacks approach, which opens room for a possible error, if one of the callback is buggy.
[PATCH v3 6/7] target/s390x: Register CPUClass:list_cpus
Register s390_cpu_list() as CPUClass:list_cpus callback and remove the cpu_list definition. Signed-off-by: Philippe Mathieu-Daudé --- target/s390x/cpu.h | 3 --- target/s390x/cpu.c | 1 + 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 8dd1936e3e2..1012be35d25 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -900,9 +900,6 @@ static inline uint8_t s390_cpu_get_state(S390CPU *cpu) } -#define cpu_list s390_cpu_list - - /* helper.c */ #define CPU_RESOLVING_TYPE TYPE_S390_CPU diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c index 1f75629ddc2..ac05e82f0ac 100644 --- a/target/s390x/cpu.c +++ b/target/s390x/cpu.c @@ -378,6 +378,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data) &scc->parent_phases); cc->class_by_name = s390_cpu_class_by_name; +cc->list_cpus = s390_cpu_list; cc->mmu_index = s390x_cpu_mmu_index; cc->dump_state = s390_cpu_dump_state; cc->query_cpu_fast = s390_query_cpu_fast; -- 2.47.1
[PATCH v3 7/7] cpus: Remove #ifdef check on cpu_list definition
Since we removed all definitions of cpu_list, the #ifdef
check is always true. Remove it, inlining cpu_list().
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
Reviewed-by: Richard Henderson
---
cpu-target.c | 19 ++-
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/cpu-target.c b/cpu-target.c
index 5947ca31a0a..30598619581 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -71,7 +71,6 @@ const char *parse_cpu_option(const char *cpu_option)
return cpu_type;
}
-#ifndef cpu_list
static void cpu_list_entry(gpointer data, gpointer user_data)
{
CPUClass *cc = CPU_CLASS(OBJECT_CLASS(data));
@@ -85,17 +84,6 @@ static void cpu_list_entry(gpointer data, gpointer user_data)
}
}
-static void cpu_list(void)
-{
-GSList *list;
-
-list = object_class_get_list_sorted(TYPE_CPU, false);
-qemu_printf("Available CPUs:\n");
-g_slist_foreach(list, cpu_list_entry, NULL);
-g_slist_free(list);
-}
-#endif
-
void list_cpus(void)
{
CPUClass *cc = CPU_CLASS(object_class_by_name(CPU_RESOLVING_TYPE));
@@ -103,7 +91,12 @@ void list_cpus(void)
if (cc->list_cpus) {
cc->list_cpus();
} else {
-cpu_list();
+GSList *list;
+
+list = object_class_get_list_sorted(TYPE_CPU, false);
+qemu_printf("Available CPUs:\n");
+g_slist_foreach(list, cpu_list_entry, NULL);
+g_slist_free(list);
}
}
--
2.47.1
[PATCH v3 1/7] cpus: Introduce CPUClass::list_cpus() callback
Some targets define cpu_list to a method listing their
CPUs on stdout. In order to make list_cpus() generic,
introduce the CPUClass::list_cpus() callback.
When no callback is registered, list_cpus() defaults
to the cpu_list definition.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
Reviewed-by: Richard Henderson
---
include/hw/core/cpu.h | 2 ++
cpu-target.c | 8 +++-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 5d11d26556a..ccfcd3eb3a6 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -102,6 +102,7 @@ struct SysemuCPUOps;
* CPUClass:
* @class_by_name: Callback to map -cpu command line model name to an
* instantiatable CPU type.
+ * @list_cpus: list available CPU models and flags.
* @parse_features: Callback to parse command line arguments.
* @reset_dump_flags: #CPUDumpFlags to use for reset logging.
* @mmu_index: Callback for choosing softmmu mmu index;
@@ -150,6 +151,7 @@ struct CPUClass {
/*< public >*/
ObjectClass *(*class_by_name)(const char *cpu_model);
+void (*list_cpus)(void);
void (*parse_features)(const char *typename, char *str, Error **errp);
int (*mmu_index)(CPUState *cpu, bool ifetch);
diff --git a/cpu-target.c b/cpu-target.c
index cae77374b38..5947ca31a0a 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -98,7 +98,13 @@ static void cpu_list(void)
void list_cpus(void)
{
-cpu_list();
+CPUClass *cc = CPU_CLASS(object_class_by_name(CPU_RESOLVING_TYPE));
+
+if (cc->list_cpus) {
+cc->list_cpus();
+} else {
+cpu_list();
+}
}
/* enable or disable single step mode. EXCP_DEBUG is returned by the
--
2.47.1
[PATCH-for-10.1 v2 4/7] target/sparc: Register CPUClass:list_cpus
Register sparc_cpu_list() as CPUClass:list_cpus callback.
Reduce its scope and remove the cpu_list definition.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
---
target/sparc/cpu.h | 3 ---
target/sparc/cpu.c | 3 ++-
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
index 462bcb6c0e6..7c6296ae70e 100644
--- a/target/sparc/cpu.h
+++ b/target/sparc/cpu.h
@@ -594,7 +594,6 @@ G_NORETURN void cpu_raise_exception_ra(CPUSPARCState *,
int, uintptr_t);
/* cpu_init.c */
void cpu_sparc_set_id(CPUSPARCState *env, unsigned int cpu);
-void sparc_cpu_list(void);
/* mmu_helper.c */
bool sparc_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
MMUAccessType access_type, int mmu_idx,
@@ -665,8 +664,6 @@ hwaddr cpu_get_phys_page_nofault(CPUSPARCState *env,
target_ulong addr,
#define CPU_RESOLVING_TYPE TYPE_SPARC_CPU
-#define cpu_list sparc_cpu_list
-
/* MMU modes definitions */
#if defined (TARGET_SPARC64)
#define MMU_USER_IDX 0
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 2ae7173c0bc..6b2288c727d 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -579,7 +579,7 @@ static void print_features(uint32_t features, const char
*prefix)
}
}
-void sparc_cpu_list(void)
+static void sparc_cpu_list(void)
{
unsigned int i;
@@ -1055,6 +1055,7 @@ static void sparc_cpu_class_init(ObjectClass *oc, void
*data)
&scc->parent_phases);
cc->class_by_name = sparc_cpu_class_by_name;
+cc->list_cpus = sparc_cpu_list,
cc->parse_features = sparc_cpu_parse_features;
cc->mmu_index = sparc_cpu_mmu_index;
cc->dump_state = sparc_cpu_dump_state;
--
2.47.1
[PATCH-for-10.1 v2 6/7] target/s390x: Register CPUClass:list_cpus
Register s390_cpu_list() as CPUClass:list_cpus callback and remove the cpu_list definition. Signed-off-by: Philippe Mathieu-Daudé --- target/s390x/cpu.c | 1 + 1 file changed, 1 insertion(+) diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c index 8449bfee5a9..2876f2c4eb3 100644 --- a/target/s390x/cpu.c +++ b/target/s390x/cpu.c @@ -385,6 +385,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data) &scc->parent_phases); cc->class_by_name = s390_cpu_class_by_name; +cc->list_cpus = s390_cpu_list; cc->mmu_index = s390x_cpu_mmu_index; cc->dump_state = s390_cpu_dump_state; cc->query_cpu_fast = s390_query_cpu_fast; -- 2.47.1
[PATCH-for-10.1 v2 7/7] cpus: Remove #ifdef check on cpu_list definition
Since we removed all definitions of cpu_list, the #ifdef
check is always true. Remove it, inlining cpu_list().
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
---
cpu-target.c | 19 ++-
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/cpu-target.c b/cpu-target.c
index 5947ca31a0a..30598619581 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -71,7 +71,6 @@ const char *parse_cpu_option(const char *cpu_option)
return cpu_type;
}
-#ifndef cpu_list
static void cpu_list_entry(gpointer data, gpointer user_data)
{
CPUClass *cc = CPU_CLASS(OBJECT_CLASS(data));
@@ -85,17 +84,6 @@ static void cpu_list_entry(gpointer data, gpointer user_data)
}
}
-static void cpu_list(void)
-{
-GSList *list;
-
-list = object_class_get_list_sorted(TYPE_CPU, false);
-qemu_printf("Available CPUs:\n");
-g_slist_foreach(list, cpu_list_entry, NULL);
-g_slist_free(list);
-}
-#endif
-
void list_cpus(void)
{
CPUClass *cc = CPU_CLASS(object_class_by_name(CPU_RESOLVING_TYPE));
@@ -103,7 +91,12 @@ void list_cpus(void)
if (cc->list_cpus) {
cc->list_cpus();
} else {
-cpu_list();
+GSList *list;
+
+list = object_class_get_list_sorted(TYPE_CPU, false);
+qemu_printf("Available CPUs:\n");
+g_slist_foreach(list, cpu_list_entry, NULL);
+g_slist_free(list);
}
}
--
2.47.1
[PATCH-for-10.1 v2 2/7] target/i386: Register CPUClass:list_cpus
Register x86_cpu_list() as CPUClass:list_cpus callback.
Reduce its scope and remove the cpu_list definition.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
---
target/i386/cpu.h | 3 ---
target/i386/cpu.c | 3 ++-
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 76f24446a55..28011eff0a8 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2367,7 +2367,6 @@ int x86_cpu_gdb_read_register(CPUState *cpu, GByteArray
*buf, int reg);
int x86_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
void x86_cpu_gdb_init(CPUState *cs);
-void x86_cpu_list(void);
int cpu_x86_support_mca_broadcast(CPUX86State *env);
#ifndef CONFIG_USER_ONLY
@@ -2561,8 +2560,6 @@ uint64_t cpu_get_tsc(CPUX86State *env);
#define TARGET_DEFAULT_CPU_TYPE X86_CPU_TYPE_NAME("qemu32")
#endif
-#define cpu_list x86_cpu_list
-
/* MMU modes definitions */
#define MMU_KSMAP64_IDX0
#define MMU_KSMAP32_IDX1
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 1b64ceaaba4..1f502587c96 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6305,7 +6305,7 @@ static void x86_cpu_list_entry(gpointer data, gpointer
user_data)
}
/* list available CPU models and flags */
-void x86_cpu_list(void)
+static void x86_cpu_list(void)
{
int i, j;
GSList *list;
@@ -8924,6 +8924,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc,
void *data)
cc->reset_dump_flags = CPU_DUMP_FPU | CPU_DUMP_CCOP;
cc->class_by_name = x86_cpu_class_by_name;
+cc->list_cpus = x86_cpu_list;
cc->parse_features = x86_cpu_parse_featurestr;
cc->mmu_index = x86_cpu_mmu_index;
cc->dump_state = x86_cpu_dump_state;
--
2.47.1
[PATCH v3 5/7] target/s390x: Declare s390_set_qemu_cpu_model/cpu_list in cpu_models.h
Both s390_cpu_list() and s390_set_qemu_cpu_model() are defined in cpu_models.c, move their declarations in the related "cpu_models.h" header. Use full path to header in s390-virtio-ccw.c file. Suggested-by: Thomas Huth Signed-off-by: Philippe Mathieu-Daudé --- target/s390x/cpu.h | 4 target/s390x/cpu_models.h | 3 +++ hw/s390x/s390-virtio-ccw.c | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 5b7992deda6..8dd1936e3e2 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -900,11 +900,7 @@ static inline uint8_t s390_cpu_get_state(S390CPU *cpu) } -/* cpu_models.c */ -void s390_cpu_list(void); #define cpu_list s390_cpu_list -void s390_set_qemu_cpu_model(uint16_t type, uint8_t gen, uint8_t ec_ga, - const S390FeatInit feat_init); /* helper.c */ diff --git a/target/s390x/cpu_models.h b/target/s390x/cpu_models.h index 71d4bc2dd4a..f701bc0b532 100644 --- a/target/s390x/cpu_models.h +++ b/target/s390x/cpu_models.h @@ -113,6 +113,9 @@ static inline uint64_t s390_cpuid_from_cpu_model(const S390CPUModel *model) } S390CPUDef const *s390_find_cpu_def(uint16_t type, uint8_t gen, uint8_t ec_ga, S390FeatBitmap features); +void s390_set_qemu_cpu_model(uint16_t type, uint8_t gen, uint8_t ec_ga, + const S390FeatInit feat_init); +void s390_cpu_list(void); bool kvm_s390_cpu_models_supported(void); bool kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp); diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 75b32182eb0..4f11c75b625 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -35,7 +35,7 @@ #include "hw/s390x/css-bridge.h" #include "hw/s390x/ap-bridge.h" #include "migration/register.h" -#include "cpu_models.h" +#include "target/s390x/cpu_models.h" #include "hw/nmi.h" #include "hw/qdev-properties.h" #include "hw/s390x/tod.h" -- 2.47.1
RE: [PATCH 3/8] hw/hexagon: Add v68, sa8775-cdsp0 defs
> -Original Message- > From: Brian Cain > Sent: Saturday, March 1, 2025 11:21 AM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 3/8] hw/hexagon: Add v68, sa8775-cdsp0 defs > > From: Brian Cain > > Signed-off-by: Brian Cain Acked-by: Taylor Simpson
Re: [PATCH v2 17/30] exec/target_page: runtime defintion for TARGET_PAGE_BITS_MIN
On 3/22/25 13:55, Richard Henderson wrote: On 3/21/25 17:20, Pierrick Bouvier wrote: On 3/21/25 17:01, Pierrick Bouvier wrote: On 3/21/25 15:19, Richard Henderson wrote: On 3/21/25 13:11, Pierrick Bouvier wrote: On 3/21/25 12:27, Richard Henderson wrote: On 3/21/25 11:09, Pierrick Bouvier wrote: Mmm, ok I guess. Yesterday I would have suggested merging this with page-vary.h, but today I'm actively working on making TARGET_PAGE_BITS_MIN a global constant. When you mention this, do you mean "constant accross all architectures", or a global (const) variable vs having a function call? The first -- constant across all architectures. That's great. Does choosing the min(set_of(TARGET_PAGE_BITS_MIN)) is what we want there, or is the answer more subtle than that? It will be, yes. This isn't as hard as it seems, because there are exactly two targets with TARGET_PAGE_BITS < 12: arm and avr. Because we still support armv4, TARGET_PAGE_BITS_MIN must be <= 10. AVR currently has TARGET_PAGE_BITS == 8, which is a bit of a problem. My first task is to allow avr to choose TARGET_PAGE_BITS_MIN >= 10. Which will leave us with TARGET_PAGE_BITS_MIN == 10. Ok. From what I understand, we make sure tlb flags are stored in an immutable position, within virtual addresses related to guest, by using lower bits belonging to address range inside a given page, since page addresses are aligned on page size, leaving those bits free. bits [0..2) are bswap, watchpoint and check_aligned. bits [TARGET_PAGE_BITS_MIN - 5..TARGET_PAGE_BITS_MIN) are slow, discard_write, mmio, notdirty, and invalid mask. And the compile time check we have is to make sure we don't overlap those sets (would happen in TARGET_PAGE_BITS_MIN <= 7). I wonder why we can't use bits [3..8) everywhere, like it's done for AVR, even for bigger page sizes. I noticed the comment about "address alignment bits", but I'm confused why bits [0..2) can be used, and not upper ones. Are we storing something else in the middle on other archs, or did I miss some piece of the puzzle? After looking better, TLB_SLOW_FLAGS are not part of address, so we don't use bits [0..2). For a given TARGET_PAGE_SIZE, how do we define alignment bits? Alignment bits are the least significant bits that must be 0 in order to enforce a particular alignment. The specific alignment is requested via MO_ALIGN et al as part of the guest memory reference. I think the piece you're missing is the softmmu fast path test in the generated code. We begin by indexing the tlb to find an entry. At that index, the entry may or may not match because (1) we have never looked up the page so the entry is empty, (2) we have looked up a different page that aliases, or (3) the page is present and (3a) correct, or (3b) invalidated, or (3c) some other condition that forces the slow path. The target address and the comparator have several fields: page address [63 ... TARGET_PAGE_BITS] page flags [TARGET_PAGE_BITS - 1 ... TARGET_PAGE_BITS - 5] unused [TARGET_PAGE_BITS - 6 ... align_bits], or empty. alignment [align_bits - 1 ... 0], or empty In the comparator, the unused and alignment bits are always zero; the page flags may be non-zero in order to force the comparison to fail. In the target address, we mask the page flags and unused bits; if the alignment bits of the address are set, then the address is of course unaligned and so the comparison fails. In order for all this work, the alignment field cannot overlap the page flags. The maximum alignment currently used by any guest is 5 bits, for Arm Neon, which means the minimum value for TARGET_PAGE_BITS_MIN is 10. Thanks, I think I can finally understand better what prepare_host_addr is doing, which you mentioned when we talked about that weeks ago. And thus, grasp what is really our fast path for MMU emulation. That's pretty neat by the way, including our heuristic to resize the TLB itself. r~
Re: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a pxb-pcie bus
On 3/24/25 10:56 AM, Eric Auger wrote:
On 3/21/25 1:59 AM, Donald Dutile wrote:
On 3/19/25 2:21 PM, Eric Auger wrote:
Hi Don,
On 3/19/25 5:21 PM, Donald Dutile wrote:
On 3/19/25 5:26 AM, Shameerali Kolothum Thodi wrote:
Hi Don,
Hey!
-Original Message-
From: Donald Dutile
Sent: Tuesday, March 18, 2025 10:12 PM
To: Shameerali Kolothum Thodi
; qemu-...@nongnu.org;
qemu-devel@nongnu.org
Cc: eric.au...@redhat.com; peter.mayd...@linaro.org; j...@nvidia.com;
nicol...@nvidia.com; berra...@redhat.com; nath...@nvidia.com;
mo...@nvidia.com; smost...@google.com; Linuxarm
; Wangzhou (B) ;
jiangkunkun ; Jonathan Cameron
; zhangfei@linaro.org
Subject: Re: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a
pxb-
pcie bus
Shameer,
Hi!
On 3/11/25 10:10 AM, Shameer Kolothum wrote:
User must associate a pxb-pcie root bus to smmuv3-accel
and that is set as the primary-bus for the smmu dev.
Signed-off-by: Shameer Kolothum
---
hw/arm/smmuv3-accel.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
index c327661636..1471b65374 100644
--- a/hw/arm/smmuv3-accel.c
+++ b/hw/arm/smmuv3-accel.c
@@ -9,6 +9,21 @@
#include "qemu/osdep.h"
#include "hw/arm/smmuv3-accel.h"
+#include "hw/pci/pci_bridge.h"
+
+static int smmuv3_accel_pxb_pcie_bus(Object *obj, void *opaque)
+{
+ DeviceState *d = opaque;
+
+ if (object_dynamic_cast(obj, "pxb-pcie-bus")) {
+ PCIBus *bus = PCI_HOST_BRIDGE(obj->parent)->bus;
+ if (d->parent_bus && !strcmp(bus->qbus.name, d->parent_bus-
name)) {
+ object_property_set_link(OBJECT(d), "primary-bus",
OBJECT(bus),
+ &error_abort);
+ }
+ }
+ return 0;
+}
static void smmu_accel_realize(DeviceState *d, Error **errp)
{
@@ -17,6 +32,9 @@ static void smmu_accel_realize(DeviceState *d,
Error
**errp)
SysBusDevice *dev = SYS_BUS_DEVICE(d);
Error *local_err = NULL;
+ object_child_foreach_recursive(object_get_root(),
+ smmuv3_accel_pxb_pcie_bus, d);
+
object_property_set_bool(OBJECT(dev), "accel", true,
&error_abort);
c->parent_realize(d, &local_err);
if (local_err) {
@@ -33,6 +51,7 @@ static void smmuv3_accel_class_init(ObjectClass
*klass, void *data)
device_class_set_parent_realize(dc, smmu_accel_realize,
&c->parent_realize);
dc->hotpluggable = false;
+ dc->bus_type = TYPE_PCIE_BUS;
}
static const TypeInfo smmuv3_accel_type_info = {
I am not seeing the need for a pxb-pcie bus(switch) introduced for
each
'accel'.
Isn't the IORT able to define different SMMUs for different RIDs?
if so,
itsn't that sufficient
to associate (define) an SMMU<->RID association without introducing a
pxb-pcie?
and again, I'm not sure how that improves/enables the device<->SMMU
associativity?
Thanks for taking a look at the series. As discussed elsewhere in
this thread(with
Eric), normally in physical world (or atleast in the most common
cases) SMMUv3
is attached to PCIe Root Complex and if you take a look at the IORT
spec, it describes
association of ID mappings between a RC node and SMMUV3 node.
And if my understanding is correct, in Qemu, only pxb-pcie allows you
to add
extra root complexes even though it is still plugged to
parent(pcie.0). ie, for all
devices downstream it acts as a root complex but still plugged into a
parent pcie.0.
This allows us to add/describe multiple "smmuv3-accel" each
associated with a RC.
I find the qemu statements a bit unclear here as well.
I looked at the hot plug statement(s) in docs/pcie.txt, as I figured
that's where dynamic
IORT changes would be needed as well. There, it says you can hot-add
PCIe devices to RPs,
one has to define/add RP's to the machine model for that plug-in.
Using libvirt, it could auto-add the needed RPs to do dynmaic smmuv3
additions,
I am not sure I understand your statement here. we don't want "dynamic"
SMMUv3 instantiation. SMMUv3 is a platform device which is supposed to
be coldplugged on a pre-existing PCIe hierarchy. The SMMUv3 device is
not something that is meant to be hotplugged or hotunplugged.
To me we hijack the bus= property to provide information about the IORT
IDMAP
Dynamic in the sense that if one adds smmuv3 for multiple devices,
libvirt will dynamically figure out how to instantiate one, two,
three... smmu's
in the machine at cold boot.
If you want a machine to be able to hot-plug a device that would
require another smmu,
than the config, and smmu, would have to be explicilty stated; as is
done today for
hot-plug PCIe if the simple machine that libvirt would make is not
sufficient to
hot-add a PCIe device.
Hum this will need to be discussed with libvirt guys but I am not sure
they will be inclined to support such kind of policy, esp because vIOMMU
is a pretty marginal use case as of now. Th
Re: [PATCH v2 26/30] hw/arm/armv7m: prepare compilation unit to be common
On 3/24/25 14:31, Pierrick Bouvier wrote:
On 3/23/25 12:48, Richard Henderson wrote:
On 3/20/25 15:29, Pierrick Bouvier wrote:
Signed-off-by: Pierrick Bouvier
---
hw/arm/armv7m.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 98a69846119..c367c2dcb99 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -139,8 +139,9 @@ static MemTxResult v7m_sysreg_ns_write(void *opaque, hwaddr
addr,
if (attrs.secure) {
/* S accesses to the alias act like NS accesses to the real region */
attrs.secure = 0;
+ MemOp end = target_words_bigendian() ? MO_BE : MO_LE;
return memory_region_dispatch_write(mr, addr, value,
- size_memop(size) | MO_TE, attrs);
+ size_memop(size) | end, attrs);
target_words_bigendian() is always false for arm system mode.
Just s/TE/LE/.
Good point.
By the way, what's the QEMU rationale behind having Arm big endian user binaries, and not
provide it for softmmu binaries?
For system mode, endianness is set via a combination of CPSR.E, SCTLR.B and SCTLR.EE,
details depending on armv4, armv6, armv7+.
It is IMPLEMENTATION DEFINED how the cpu initiailizes at reset. In olden times, via a
board-level pin (sometimes switched, sometimes soldered). We model the board-level pin
via the "cfgend" cpu property.
In any case, for system mode we expect the guest to do the same thing it would need to do
on real hardware. For user mode, we can't do that, as we're also emulating the OS layer,
which needs to know the endianness to expect from the guest binaries.
If those systems are so rare, why would people need a user mode emulation?
IMO armbe-linux-user is extinct.
Debian never had big-endian support at all. If there was some other distro which had it,
I don't recall which. Otherwise you'd need to bootstrap the entire toolchain, which to me
seems rather beside the point.
r~
Re: [RFC v5 3/7] vhost: Forward descriptors to device via packed SVQ
Hi,
I had a few queries here.
On 3/24/25 7:29 PM, Sahil Siddiq wrote:
Implement the insertion of available buffers in the descriptor area of
packed shadow virtqueues. It takes into account descriptor chains, but
does not consider indirect descriptors.
Enable the packed SVQ to forward the descriptors to the device.
Signed-off-by: Sahil Siddiq
---
Changes from v4 -> v5:
- This was commit #2 in v4. This has been reordered to commit #3
based on review comments.
- vhost-shadow-virtqueue.c:
(vhost_svq_valid_features): Move addition of enums to commit #6
based on review comments.
(vhost_svq_add_packed): Set head_idx to buffer id instead of vring's
index.
(vhost_svq_kick): Split into vhost_svq_kick_split and
vhost_svq_kick_packed.
(vhost_svq_add): Use new vhost_svq_kick_* functions.
hw/virtio/vhost-shadow-virtqueue.c | 117 +++--
1 file changed, 112 insertions(+), 5 deletions(-)
diff --git a/hw/virtio/vhost-shadow-virtqueue.c
b/hw/virtio/vhost-shadow-virtqueue.c
index 4f74ad402a..6e16cd4bdf 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -193,10 +193,83 @@ static void vhost_svq_add_split(VhostShadowVirtqueue *svq,
/* Update the avail index after write the descriptor */
smp_wmb();
avail->idx = cpu_to_le16(svq->shadow_avail_idx);
+}
+
+/**
+ * Write descriptors to SVQ packed vring
+ *
+ * @svq: The shadow virtqueue
+ * @out_sg: The iovec to the guest
+ * @out_num: Outgoing iovec length
+ * @in_sg: The iovec from the guest
+ * @in_num: Incoming iovec length
+ * @sgs: Cache for hwaddr
+ * @head: Saves current free_head
+ */
+static void vhost_svq_add_packed(VhostShadowVirtqueue *svq,
+ const struct iovec *out_sg, size_t out_num,
+ const struct iovec *in_sg, size_t in_num,
+ hwaddr *sgs, unsigned *head)
+{
+uint16_t id, curr, i, head_flags = 0, head_idx;
+size_t num = out_num + in_num;
+unsigned n;
+
+struct vring_packed_desc *descs = svq->vring_packed.vring.desc;
+
+head_idx = svq->vring_packed.next_avail_idx;
Since "svq->vring_packed.next_avail_idx" is part of QEMU internals and not
stored in guest memory, no endianness conversion is required here, right?
+i = head_idx;
+id = svq->free_head;
+curr = id;
+*head = id;
Should head be the buffer id or the idx of the descriptor ring where the
first descriptor of a descriptor chain is inserted?
+/* Write descriptors to SVQ packed vring */
+for (n = 0; n < num; n++) {
+uint16_t flags = cpu_to_le16(svq->vring_packed.avail_used_flags |
+ (n < out_num ? 0 : VRING_DESC_F_WRITE) |
+ (n + 1 == num ? 0 : VRING_DESC_F_NEXT));
+if (i == head_idx) {
+head_flags = flags;
+} else {
+descs[i].flags = flags;
+}
+
+descs[i].addr = cpu_to_le64(sgs[n]);
+descs[i].id = id;
+if (n < out_num) {
+descs[i].len = cpu_to_le32(out_sg[n].iov_len);
+} else {
+descs[i].len = cpu_to_le32(in_sg[n - out_num].iov_len);
+}
+
+curr = cpu_to_le16(svq->desc_next[curr]);
+
+if (++i >= svq->vring_packed.vring.num) {
+i = 0;
+svq->vring_packed.avail_used_flags ^=
+1 << VRING_PACKED_DESC_F_AVAIL |
+1 << VRING_PACKED_DESC_F_USED;
+}
+}
+if (i <= head_idx) {
+svq->vring_packed.avail_wrap_counter ^= 1;
+}
+
+svq->vring_packed.next_avail_idx = i;
+svq->shadow_avail_idx = i;
+svq->free_head = curr;
+
+/*
+ * A driver MUST NOT make the first descriptor in the list
+ * available before all subsequent descriptors comprising
+ * the list are made available.
+ */
+smp_wmb();
+svq->vring_packed.vring.desc[head_idx].flags = head_flags;
}
-static void vhost_svq_kick(VhostShadowVirtqueue *svq)
+static void vhost_svq_kick_split(VhostShadowVirtqueue *svq)
{
bool needs_kick;
@@ -209,7 +282,8 @@ static void vhost_svq_kick(VhostShadowVirtqueue *svq)
if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
uint16_t avail_event = le16_to_cpu(
*(uint16_t *)(&svq->vring.used->ring[svq->vring.num]));
-needs_kick = vring_need_event(avail_event, svq->shadow_avail_idx,
svq->shadow_avail_idx - 1);
+needs_kick = vring_need_event(avail_event, svq->shadow_avail_idx,
+ svq->shadow_avail_idx - 1);
} else {
needs_kick =
!(svq->vring.used->flags &
cpu_to_le16(VRING_USED_F_NO_NOTIFY));
@@ -222,6 +296,30 @@ static void vhost_svq_kick(VhostShadowVirtqueue *svq)
event_notifier_set(&svq->hdev_kick);
}
+static void vhost_svq_kick_packed(VhostShadowVirtqueue *svq)
+{
+bool needs_kick;
+
+/*
+ * We need t
Re: [PATCH v2 2/3] vhost: return failure if stop virtqueue failed in vhost_dev_stop
On Thu, Mar 20, 2025 at 08:21:25PM +0800, Haoqian He wrote:
2025年3月19日 23:11，Stefano Garzarella 写道：
On Fri, Mar 14, 2025 at 06:15:33AM -0400, Haoqian He wrote:
The backend maybe crash when vhost_dev_stop and GET_VRING_BASE
would fail, we can return failure to indicate the connection
with the backend is broken.
Signed-off-by: Haoqian He
---
hw/virtio/vhost.c | 27 +++
include/hw/virtio/vhost.h | 8 +---
2 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 6aa72fd434..c82bbbe4cc 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1368,23 +1368,23 @@ fail_alloc_desc:
return r;
}
-void vhost_virtqueue_stop(struct vhost_dev *dev,
- struct VirtIODevice *vdev,
- struct vhost_virtqueue *vq,
- unsigned idx)
+int vhost_virtqueue_stop(struct vhost_dev *dev,
+ struct VirtIODevice *vdev,
+ struct vhost_virtqueue *vq,
+ unsigned idx)
{
int vhost_vq_index = dev->vhost_ops->vhost_get_vq_index(dev, idx);
struct vhost_vring_state state = {
.index = vhost_vq_index,
};
-int r;
+int r = 0;
if (virtio_queue_get_desc_addr(vdev, idx) == 0) {
/* Don't stop the virtqueue which might have not been started */
-return;
+return 0;
}
-r = dev->vhost_ops->vhost_get_vring_base(dev, &state);
+r |= dev->vhost_ops->vhost_get_vring_base(dev, &state);
We can avoid this and also initialize r to 0.
Here we need to do `vhost_virtqueue_stop` for each vq.
Sorry, my question is what's the point of initializing r to 0 and then
putting it in or here with the result of vhost_get_vring_base?
Can't we leave it as before and initialize it directly here?
if (r < 0) {
VHOST_OPS_DEBUG(r, "vhost VQ %u ring restore failed: %d", idx, r);
/* Connection to the backend is broken, so let's sync internal
@@ -1412,6 +1412,7 @@ void vhost_virtqueue_stop(struct vhost_dev *dev,
0, virtio_queue_get_avail_size(vdev, idx));
vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
0, virtio_queue_get_desc_size(vdev, idx));
+return r;
}
static int vhost_virtqueue_set_busyloop_timeout(struct vhost_dev *dev,
@@ -2136,9 +2137,10 @@ fail_features:
}
/* Host notifiers must be enabled at this point. */
-void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
+int vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
{
int i;
+int rc = 0;
/* should only be called after backend is connected */
assert(hdev->vhost_ops);
@@ -2157,10 +2159,10 @@ void vhost_dev_stop(struct vhost_dev *hdev,
VirtIODevice *vdev, bool vrings)
vhost_dev_set_vring_enable(hdev, false);
}
for (i = 0; i < hdev->nvqs; ++i) {
-vhost_virtqueue_stop(hdev,
- vdev,
- hdev->vqs + i,
- hdev->vq_index + i);
+rc |= vhost_virtqueue_stop(hdev,
+ vdev,
+ hdev->vqs + i,
+ hdev->vq_index + i);
Also other function can fails, should we consider also them?
(e.g. , vhost_dev_set_vring_enable, etc.)
If not, why?
Since we only want to know the return value of callback when the
stopping device
live migration, there is no need to catch the failure of `vhost_dev_start`.
Please add that in the commit message, and maybe also in a comment here.
We can also catch the failure of `vhost_dev_set_vring_enable`, because
`vhost_dev_set_vring_enable` will also fail if qemu lost connection with the
the backend, but I need to test it.
Capturing failures of only some things is a little confusing to me, I
think it needs to be better explained.
Thanks,
Stefano
}
if (hdev->vhost_ops->vhost_reset_status) {
hdev->vhost_ops->vhost_reset_status(hdev);
@@ -2177,6 +2179,7 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice
*vdev, bool vrings)
hdev->started = false;
vdev->vhost_started = false;
hdev->vdev = NULL;
+return rc;
}
int vhost_net_set_backend(struct vhost_dev *hdev,
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index a9469d50bc..fd96ec9c39 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -232,8 +232,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice
*vdev, bool vrings);
* Stop the vhost device. After the device is stopped the notifiers
* can be disabled (@vhost_dev_disable_notifiers) and the device can
* be torn down (@vhost_dev_cleanup).
+ *
+ * Return: 0 on success, != 0 on error when stopping dev.
*/
-void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings);
+int vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings
Re: [RFC PATCH v2 03/20] hw/arm/smmuv3-accel: Add initial infrastructure for smmuv3-accel device
On 3/21/25 1:54 AM, Donald Dutile wrote: > > > On 3/19/25 1:04 PM, Eric Auger wrote: >> >> >> >> On 3/18/25 10:22 PM, Donald Dutile wrote: >>> >>> >>> On 3/18/25 3:13 PM, Nicolin Chen wrote: On Tue, Mar 18, 2025 at 07:31:36PM +0100, Eric Auger wrote: > On 3/17/25 9:19 PM, Nicolin Chen wrote: >> On Mon, Mar 17, 2025 at 04:24:53PM -0300, Jason Gunthorpe wrote: >>> On Mon, Mar 17, 2025 at 12:10:19PM -0700, Nicolin Chen wrote: Another question: how does an emulated device work with a vSMMUv3? I could imagine that all the accel steps would be bypassed since !sdev->idev. Yet, the emulated iotlb should cache its translation so we will need to flush the iotlb, which will increase complexity as the TLBI command dispatching function will need to be aware what ASID is for emulated device and what is for vfio device.. >>> I think you should block it. We already expect different vSMMU's >>> depending on the physical SMMU under the PCI device, it makes sense >>> that a SW VFIO device would have it's own, non-accelerated, vSMMU >>> model in the guest. >> Yea, I agree and it'd be cleaner for an implementation separating >> them. >> >> In my mind, the general idea of "accel=on" is also to keep things >> in a more efficient way: passthrough devices go to HW-accelerated >> vSMMUs (separated PCIE buses), while emulated ones go to a vSMMU- >> bypassed (PCIE0). > Originally a specific SMMU device was needed to opt in for MSI > reserved > region ACPI IORT description which are not needed if you don't > rely on > S1+S2. However if we don't rely on this trick this was not even > needed > with legacy integration > (https://patchwork.kernel.org/project/qemu-devel/cover/20180921081819.9203-1-eric.au...@redhat.com/). > > > > Nevertheless I don't think anything prevents the acceleration granted > device from also working with virtio/vhost devices for instance > unless > you unplug the existing infra. The translation and invalidation just > should use different control paths (explicit translation requests, > invalidation notifications towards vhost, ...). smmuv3_translate() is per sdev, so it's easy. Invalidation is done via commands, which could be tricky: a) Broadcast command b) ASID validation -- we'll need to keep track of a list of ASIDs for vfio device to compare the ASID in each per-ASID command, potentially by trapping all CFGI_CD(_ALL) commands? Note that each vfio device may have multiple ASIDs (for multiple CDs). Either a or b above will have some validation efficiency impact. > Again, what does legitimate to have different qemu devices for the > same > IP? I understand that it simplifies the implementation but I am not > sure > this is a good reason. Nevertheless it worth challenging. What is the > plan for intel iommu? Will we have 2 devices, the legacy device > and one > for nested? Hmm, it seems that there are two different topics: 1. Use one SMMU device model (source code file; "iommu=" string) for both an emulated vSMMU and an HW-accelerated vSMMU. 2. Allow one vSMMU instance to work with both an emulated device and a passthrough device. And I get that you want both 1 and 2. I'm totally okay with 1, yet see no compelling benefit from 2 for the increased complexity in the invalidation routine. And another question about the mixed device attachment. Let's say we have in the host: VFIO passthrough dev0 -> pSMMU0 VFIO passthrough dev1 -> pSMMU1 Should we allow emulated devices to be flexibly plugged? dev0 -> vSMMU0 /* Hard requirement */ dev1 -> vSMMU1 /* Hard requirement */ emu0 -> vSMMU0 /* Soft requirement; can be vSMMU1 also */ emu1 -> vSMMU1 /* Soft requirement; can be vSMMU0 also */ Thanks Nicolin >>> I agree w/Jason & Nicolin: different vSMMUs for pass-through devices >>> than emulated, & vice-versa. >>> Not mixing... because... of the next agreement: >> you need to clarify what you mean by different vSMMUs: are you taking >> about different instances or different qemu device types? > Both. a device needed to use hw-accel feature has to use an smmu that > has that feature; > an emulated device can use such an smmu, but as mentioned in other > threads, > if you start with all emulated in one smmu, if you hot-plug a > (assigned) device, > it needs another smmu that has hw-accel features. > Keeping them split makes it easier at config time, and it may enable > the code to be simpler... > but the other half of my brain wants common code paths with > accel/emulate branches but > a different smmu instance will like simplify the smmu-(accel-)specific > lookups. Yes I think we
Re: [PATCH v2] virtio-net: Fix the interpretation of max_tx_vq
QE tested this patch with virtio-net regression tests, everything works fine.
Tested-by: Lei Yang
On Sat, Mar 22, 2025 at 2:48 PM Akihiko Odaki wrote:
>
> virtio-net uses the max_tx_vq field of struct virtio_net_rss_config to
> determine the number of queue pairs and emits an error message saying
> "Can't get queue_pairs". However, the field tells only about tx.
>
> Examine unclassified_queue and indirection_table to determine the number
> of queues required for rx, and correct the name of field in the error
> message, clarifying its correct semantics.
>
> Fixes: 590790297c0d ("virtio-net: implement RSS configuration command")
> Signed-off-by: Akihiko Odaki
> ---
> Changes in v2:
> - Handled unclassified_queue too.
> - Added a Fixes: tag.
> - Link to v1:
> https://lore.kernel.org/qemu-devel/20250321-vq-v1-1-6d6d285e5...@daynix.com
> ---
> hw/net/virtio-net.c | 26 --
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index de87cfadffe1..afc6b82f13c9 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1450,23 +1450,29 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n,
> err_value = (uint32_t)s;
> goto error;
> }
> -for (i = 0; i < n->rss_data.indirections_len; ++i) {
> -uint16_t val = n->rss_data.indirections_table[i];
> -n->rss_data.indirections_table[i] = virtio_lduw_p(vdev, &val);
> -}
> offset += size_get;
> size_get = sizeof(temp);
> s = iov_to_buf(iov, iov_cnt, offset, &temp, size_get);
> if (s != size_get) {
> -err_msg = "Can't get queue_pairs";
> +err_msg = "Can't get max_tx_vq";
> err_value = (uint32_t)s;
> goto error;
> }
> -queue_pairs = do_rss ? virtio_lduw_p(vdev, &temp.us) :
> n->curr_queue_pairs;
> -if (queue_pairs == 0 || queue_pairs > n->max_queue_pairs) {
> -err_msg = "Invalid number of queue_pairs";
> -err_value = queue_pairs;
> -goto error;
> +if (do_rss) {
> +queue_pairs = MAX(virtio_lduw_p(vdev, &temp.us),
> + n->rss_data.default_queue);
> +for (i = 0; i < n->rss_data.indirections_len; ++i) {
> +uint16_t val = n->rss_data.indirections_table[i];
> +n->rss_data.indirections_table[i] = virtio_lduw_p(vdev, &val);
> +queue_pairs = MAX(queue_pairs,
> n->rss_data.indirections_table[i]);
> +}
> +if (queue_pairs == 0 || queue_pairs > n->max_queue_pairs) {
> +err_msg = "Invalid number of queue_pairs";
> +err_value = queue_pairs;
> +goto error;
> +}
> +} else {
> +queue_pairs = n->curr_queue_pairs;
> }
> if (temp.b > VIRTIO_NET_RSS_MAX_KEY_SIZE) {
> err_msg = "Invalid key size";
>
> ---
> base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32
> change-id: 20250321-vq-87aff4f531bf
>
> Best regards,
> --
> Akihiko Odaki
>
>
Re: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a pxb-pcie bus
On 3/21/25 1:59 AM, Donald Dutile wrote:
>
>
> On 3/19/25 2:21 PM, Eric Auger wrote:
>> Hi Don,
>>
>>
>> On 3/19/25 5:21 PM, Donald Dutile wrote:
>>>
>>>
>>> On 3/19/25 5:26 AM, Shameerali Kolothum Thodi wrote:
Hi Don,
>>> Hey!
>>>
> -Original Message-
> From: Donald Dutile
> Sent: Tuesday, March 18, 2025 10:12 PM
> To: Shameerali Kolothum Thodi
> ; qemu-...@nongnu.org;
> qemu-devel@nongnu.org
> Cc: eric.au...@redhat.com; peter.mayd...@linaro.org; j...@nvidia.com;
> nicol...@nvidia.com; berra...@redhat.com; nath...@nvidia.com;
> mo...@nvidia.com; smost...@google.com; Linuxarm
> ; Wangzhou (B) ;
> jiangkunkun ; Jonathan Cameron
> ; zhangfei@linaro.org
> Subject: Re: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a
> pxb-
> pcie bus
>
> Shameer,
>
> Hi!
>
> On 3/11/25 10:10 AM, Shameer Kolothum wrote:
>> User must associate a pxb-pcie root bus to smmuv3-accel
>> and that is set as the primary-bus for the smmu dev.
>>
>> Signed-off-by: Shameer Kolothum
>
>> ---
>> hw/arm/smmuv3-accel.c | 19 +++
>> 1 file changed, 19 insertions(+)
>>
>> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
>> index c327661636..1471b65374 100644
>> --- a/hw/arm/smmuv3-accel.c
>> +++ b/hw/arm/smmuv3-accel.c
>> @@ -9,6 +9,21 @@
>> #include "qemu/osdep.h"
>>
>> #include "hw/arm/smmuv3-accel.h"
>> +#include "hw/pci/pci_bridge.h"
>> +
>> +static int smmuv3_accel_pxb_pcie_bus(Object *obj, void *opaque)
>> +{
>> + DeviceState *d = opaque;
>> +
>> + if (object_dynamic_cast(obj, "pxb-pcie-bus")) {
>> + PCIBus *bus = PCI_HOST_BRIDGE(obj->parent)->bus;
>> + if (d->parent_bus && !strcmp(bus->qbus.name, d->parent_bus-
>> name)) {
>> + object_property_set_link(OBJECT(d), "primary-bus",
>> OBJECT(bus),
>> + &error_abort);
>> + }
>> + }
>> + return 0;
>> +}
>>
>> static void smmu_accel_realize(DeviceState *d, Error **errp)
>> {
>> @@ -17,6 +32,9 @@ static void smmu_accel_realize(DeviceState *d,
>> Error
> **errp)
>> SysBusDevice *dev = SYS_BUS_DEVICE(d);
>> Error *local_err = NULL;
>>
>> + object_child_foreach_recursive(object_get_root(),
>> + smmuv3_accel_pxb_pcie_bus, d);
>> +
>> object_property_set_bool(OBJECT(dev), "accel", true,
>> &error_abort);
>> c->parent_realize(d, &local_err);
>> if (local_err) {
>> @@ -33,6 +51,7 @@ static void smmuv3_accel_class_init(ObjectClass
> *klass, void *data)
>> device_class_set_parent_realize(dc, smmu_accel_realize,
>> &c->parent_realize);
>> dc->hotpluggable = false;
>> + dc->bus_type = TYPE_PCIE_BUS;
>> }
>>
>> static const TypeInfo smmuv3_accel_type_info = {
>
> I am not seeing the need for a pxb-pcie bus(switch) introduced for
> each
> 'accel'.
> Isn't the IORT able to define different SMMUs for different RIDs?
> if so,
> itsn't that sufficient
> to associate (define) an SMMU<->RID association without introducing a
> pxb-pcie?
> and again, I'm not sure how that improves/enables the device<->SMMU
> associativity?
Thanks for taking a look at the series. As discussed elsewhere in
this thread(with
Eric), normally in physical world (or atleast in the most common
cases) SMMUv3
is attached to PCIe Root Complex and if you take a look at the IORT
spec, it describes
association of ID mappings between a RC node and SMMUV3 node.
And if my understanding is correct, in Qemu, only pxb-pcie allows you
to add
extra root complexes even though it is still plugged to
parent(pcie.0). ie, for all
devices downstream it acts as a root complex but still plugged into a
parent pcie.0.
This allows us to add/describe multiple "smmuv3-accel" each
associated with a RC.
>>> I find the qemu statements a bit unclear here as well.
>>> I looked at the hot plug statement(s) in docs/pcie.txt, as I figured
>>> that's where dynamic
>>> IORT changes would be needed as well. There, it says you can hot-add
>>> PCIe devices to RPs,
>>> one has to define/add RP's to the machine model for that plug-in.
>>>
>>> Using libvirt, it could auto-add the needed RPs to do dynmaic smmuv3
>>> additions,
>> I am not sure I understand your statement here. we don't want "dynamic"
>> SMMUv3 instantiation. SMMUv3 is a platform device which is supposed to
>> be coldplugged on a pre-existing PCIe hierarchy. The SMMUv3 device is
>> not something that is meant to be hotplugged or hotunplugged.
>> To me we hijack the bus= pro
Re: [PULL 00/24] Mostly Rust changes for QEMU 10.0
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/10.0 for any user-visible changes. signature.asc Description: PGP signature
Re: [PATCH v2 1/2] io/block: Refactoring the bdrv_drained_begin() function and implement a timeout mechanism.
On Fri, Mar 21, 2025 at 03:09:16PM +0800, zoudongjie wrote:
> From: Zhu Yangyang
>
> The bdrv_drained_begin() function is a blocking function. In scenarios where
> network storage
> is used and network links fail, it may block for a long time.
> Therefore, we add a timeout parameter to control the duration of the block.
>
> Since bdrv_drained_begin() has been widely adopted, both bdrv_drained_begin()
> and bdrv_drained_begin_timeout() will be retained.
>
> Signed-off-by: Zhu Yangyang
> ---
> block/io.c | 58 +---
> include/block/aio-wait.h | 49 +
> include/block/block-io.h | 22 ++-
> util/aio-wait.c | 5
> 4 files changed, 123 insertions(+), 11 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index 1ba8d1aeea..912b76c4a4 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -255,6 +255,8 @@ typedef struct {
> bool begin;
> bool poll;
> BdrvChild *parent;
> +uint64_t timeout_ns;
> +int ret;
> } BdrvCoDrainData;
>
> /* Returns true if BDRV_POLL_WHILE() should go into a blocking aio_poll() */
> @@ -283,6 +285,10 @@ static bool bdrv_drain_poll_top_level(BlockDriverState
> *bs,
> return bdrv_drain_poll(bs, ignore_parent, false);
> }
>
> +static int bdrv_do_drained_begin_timeout(BlockDriverState *bs,
> + BdrvChild *parent,
> + bool poll,
> + uint64_t timeout_ns);
> static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent,
>bool poll);
> static void bdrv_do_drained_end(BlockDriverState *bs, BdrvChild *parent);
> @@ -296,7 +302,9 @@ static void bdrv_co_drain_bh_cb(void *opaque)
> if (bs) {
> bdrv_dec_in_flight(bs);
> if (data->begin) {
> -bdrv_do_drained_begin(bs, data->parent, data->poll);
> +data->ret = bdrv_do_drained_begin_timeout(bs, data->parent,
> + data->poll,
> + data->timeout_ns);
> } else {
> assert(!data->poll);
> bdrv_do_drained_end(bs, data->parent);
> @@ -310,10 +318,11 @@ static void bdrv_co_drain_bh_cb(void *opaque)
> aio_co_wake(co);
> }
>
> -static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
> -bool begin,
> -BdrvChild *parent,
> -bool poll)
> +static int coroutine_fn bdrv_co_yield_to_drain_timeout(BlockDriverState *bs,
> + bool begin,
> + BdrvChild *parent,
> + bool poll,
> + uint64_t timeout_ns)
> {
> BdrvCoDrainData data;
> Coroutine *self = qemu_coroutine_self();
> @@ -329,6 +338,8 @@ static void coroutine_fn
> bdrv_co_yield_to_drain(BlockDriverState *bs,
> .begin = begin,
> .parent = parent,
> .poll = poll,
> +.timeout_ns = timeout_ns,
> +.ret = 0
> };
>
> if (bs) {
> @@ -342,16 +353,27 @@ static void coroutine_fn
> bdrv_co_yield_to_drain(BlockDriverState *bs,
> /* If we are resumed from some other event (such as an aio completion or
> a
> * timer callback), it is a bug in the caller that should be fixed. */
> assert(data.done);
> +return data.ret;
> }
>
> -static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent,
> - bool poll)
> +static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
> +bool begin,
> +BdrvChild *parent,
> +bool poll)
> +{
> +bdrv_co_yield_to_drain_timeout(bs, begin, parent, poll, 0);
> +}
> +
> +static int bdrv_do_drained_begin_timeout(BlockDriverState *bs,
> + BdrvChild *parent,
> + bool poll,
> + uint64_t timeout_ns)
> {
> IO_OR_GS_CODE();
>
> if (qemu_in_coroutine()) {
> -bdrv_co_yield_to_drain(bs, true, parent, poll);
> -return;
> +return bdrv_co_yield_to_drain_timeout(bs, true, parent, poll,
> + timeout_ns);
> }
>
> GLOBAL_STATE_CODE();
> @@ -375,8 +397,17 @@ static void bdrv_do_drained_begin(BlockDriverState *bs,
> BdrvChild *parent,
> * nodes.
> */
> if (poll) {
> -BDRV_POLL_WHILE(bs, bdrv_drain_poll_top_level(bs, parent));
> +retur
[PATCH-for-10.1 v2 3/7] target/ppc: Register CPUClass:list_cpus
Register ppc_cpu_list() as CPUClass:list_cpus callback.
Reduce its scope and remove the cpu_list definition.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
---
target/ppc/cpu.h | 4
target/ppc/cpu_init.c | 3 ++-
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index efab54a0683..0062579ef3e 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1597,8 +1597,6 @@ void ppc_store_dawrx1(CPUPPCState *env, uint32_t value);
#endif /* !defined(CONFIG_USER_ONLY) */
void ppc_store_msr(CPUPPCState *env, target_ulong value);
-void ppc_cpu_list(void);
-
/* Time-base and decrementer management */
uint64_t cpu_ppc_load_tbl(CPUPPCState *env);
uint32_t cpu_ppc_load_tbu(CPUPPCState *env);
@@ -1660,8 +1658,6 @@ static inline uint64_t ppc_dump_gpr(CPUPPCState *env, int
gprn)
int ppc_dcr_read(ppc_dcr_t *dcr_env, int dcrn, uint32_t *valp);
int ppc_dcr_write(ppc_dcr_t *dcr_env, int dcrn, uint32_t val);
-#define cpu_list ppc_cpu_list
-
/* MMU modes definitions */
#define MMU_USER_IDX 0
static inline int ppc_env_mmu_index(CPUPPCState *env, bool ifetch)
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index ed79cc1a6b7..4d7c0619739 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -7184,7 +7184,7 @@ static void ppc_cpu_list_entry(gpointer data, gpointer
user_data)
g_free(name);
}
-void ppc_cpu_list(void)
+static void ppc_cpu_list(void)
{
GSList *list;
@@ -7528,6 +7528,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void
*data)
&pcc->parent_phases);
cc->class_by_name = ppc_cpu_class_by_name;
+cc->list_cpus = ppc_cpu_list;
cc->mmu_index = ppc_cpu_mmu_index;
cc->dump_state = ppc_cpu_dump_state;
cc->set_pc = ppc_cpu_set_pc;
--
2.47.1
Re: [PATCH v2 00/11] gdbstub: conversion to runtime endianess helpers
On 3/24/25 03:21, Alex Bennée wrote: The aim of this work is to get rid of the endian aware helpers in gdbstub/helpers.h which due to their use of tswap() mean target gdbstubs need to be built multiple times. While this series doesn't actually build each stub once it introduces a new helper - gdb_get_register_value() which takes a MemOp which can describe the current endian state of the system. This will be a lot easier to dynamically feed from a helper function. The most complex example is PPC which has a helper called ppc_maybe_bswap_register() which was doing this. This is still an RFC but I've spun out v2 for further discussion. In v2: - drop uint8_t casting and use void as C intended ;-) C allows that permissively, and it can be convenient when underlying type is really unknown. But in our case, it's only two variants. If really it hurts to replace callers, _Generic is at least a better solution than invoking the void* hammer. - add specific 32/64 bit helpers with type checking an assertion - various tweaks and fixes (see individual commits) There are a few other misc clean-ups I did on the way which might be worth cherry picking for 10.0 but I'll leave that up to maintainers. Alex Bennée (11): include/exec: fix assert in size_memop include/gdbstub: fix include guard in commands.h gdbstub: assert earlier in handle_read_all_regs gdbstub: introduce target independent gdb register helper target/arm: convert 32 bit gdbstub to new helpers target/arm: convert 64 bit gdbstub to new helpers target/ppc: expand comment on FP/VMX/VSX access functions target/ppc: make ppc_maybe_bswap_register static target/ppc: convert gdbstub to new helpers target/microblaze: convert gdbstub to new helper include/gdbstub: add note to helpers.h include/exec/memop.h| 4 +- include/gdbstub/commands.h | 2 +- include/gdbstub/helpers.h | 4 +- include/gdbstub/registers.h | 55 ++ target/ppc/cpu.h| 8 +- gdbstub/gdbstub.c | 25 - target/arm/gdbstub.c| 55 ++ target/arm/gdbstub64.c | 53 ++ target/microblaze/gdbstub.c | 49 - target/ppc/gdbstub.c| 197 10 files changed, 292 insertions(+), 160 deletions(-) create mode 100644 include/gdbstub/registers.h
Re: [PATCH v2 08/11] target/ppc: make ppc_maybe_bswap_register static
On 3/24/25 03:21, Alex Bennée wrote:
It's not used outside of the gdbstub code.
Reviewed-by: Pierrick Bouvier
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Alex Bennée
---
target/ppc/cpu.h | 1 -
target/ppc/gdbstub.c | 2 +-
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 1e833ade04..950bb6e06c 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -3016,7 +3016,6 @@ static inline bool
ppc_interrupts_little_endian(PowerPCCPU *cpu, bool hv)
void dump_mmu(CPUPPCState *env);
-void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len);
void ppc_store_vscr(CPUPPCState *env, uint32_t vscr);
uint32_t ppc_get_vscr(CPUPPCState *env);
void ppc_set_cr(CPUPPCState *env, uint64_t cr);
diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index 3b28d4e21c..c09e93abaf 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -81,7 +81,7 @@ static int ppc_gdb_register_len(int n)
* TARGET_BIG_ENDIAN is always set, and we must check the current
* mode of the chip to see if we're running in little-endian.
*/
-void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len)
+static void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int
len)
{
#ifndef CONFIG_USER_ONLY
if (!FIELD_EX64(env->msr, MSR, LE)) {
Reviewed-by: Pierrick Bouvier
Re: [PATCH v2 10/11] target/microblaze: convert gdbstub to new helper
On 3/24/25 03:21, Alex Bennée wrote:
This is a pretty simple conversion given a single set of registers and
an existing helper to probe endianess.
Signed-off-by: Alex Bennée
---
v2
- use mb_cpu_is_big_endian
- use explicit MO_32 size
- handle differing size of env->ear between user/system
---
target/microblaze/gdbstub.c | 49 +
1 file changed, 22 insertions(+), 27 deletions(-)
diff --git a/target/microblaze/gdbstub.c b/target/microblaze/gdbstub.c
index d493681d38..dbaf7ecb9c 100644
--- a/target/microblaze/gdbstub.c
+++ b/target/microblaze/gdbstub.c
@@ -19,7 +19,7 @@
*/
#include "qemu/osdep.h"
#include "cpu.h"
-#include "gdbstub/helpers.h"
+#include "gdbstub/registers.h"
/*
* GDB expects SREGs in the following order:
@@ -50,62 +50,57 @@ int mb_cpu_gdb_read_register(CPUState *cs, GByteArray
*mem_buf, int n)
{
MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
CPUMBState *env = &cpu->env;
-uint32_t val;
+MemOp mo = mb_cpu_is_big_endian(cs) ? MO_BE : MO_LE;
+uint32_t msr;
switch (n) {
case 1 ... 31:
-val = env->regs[n];
-break;
+return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->regs[n]);
case GDB_PC:
-val = env->pc;
-break;
+return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->pc);
case GDB_MSR:
-val = mb_cpu_read_msr(env);
-break;
+msr = mb_cpu_read_msr(env);
+return gdb_get_reg32_value(mo | MO_32, mem_buf, &msr);
case GDB_EAR:
-val = env->ear;
-break;
+#if TARGET_LONG_BITS == 64
+return gdb_get_reg64_value(mo | MO_64, mem_buf, &env->ear);
+#else
+return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->ear);
+#endif
case GDB_ESR:
-val = env->esr;
-break;
+return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->esr);
case GDB_FSR:
-val = env->fsr;
-break;
+return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->fsr);
case GDB_BTR:
-val = env->btr;
-break;
+return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->btr);
case GDB_PVR0 ... GDB_PVR11:
/* PVR12 is intentionally skipped */
-val = cpu->cfg.pvr_regs[n - GDB_PVR0];
-break;
+return gdb_get_reg32_value(mo | MO_32, mem_buf,
+ &cpu->cfg.pvr_regs[n - GDB_PVR0]);
case GDB_EDR:
-val = env->edr;
-break;
+return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->edr);
default:
/* Other SRegs aren't modeled, so report a value of 0 */
-val = 0;
-break;
+return 0;
}
-return gdb_get_reg32(mem_buf, val);
}
int mb_cpu_gdb_read_stack_protect(CPUState *cs, GByteArray *mem_buf, int n)
{
MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
CPUMBState *env = &cpu->env;
-uint32_t val;
+MemOp mo = TARGET_BIG_ENDIAN ? MO_BEUL : MO_LEUL;
switch (n) {
case GDB_SP_SHL:
-val = env->slr;
+return gdb_get_reg32_value(mo, mem_buf, &env->slr);
break;
case GDB_SP_SHR:
-val = env->shr;
+return gdb_get_reg32_value(mo, mem_buf, &env->shr);
break;
default:
return 0;
}
-return gdb_get_reg32(mem_buf, val);
}
int mb_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
Reviewed-by: Pierrick Bouvier
RE: [PATCH 5/8] hw/hexagon: Modify "Standalone" symbols
> -Original Message- > From: Brian Cain > Sent: Saturday, March 1, 2025 11:21 AM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com; Brian Cain > Subject: [PATCH 5/8] hw/hexagon: Modify "Standalone" symbols > > From: Brian Cain > > These symbols are used by Hexagon Standalone OS to indicate whether the > program should halt and wait for interrupts at startup. For QEMU, we want > these programs to just continue crt0 startup through to the user program's > main(). > > Signed-off-by: Brian Cain Reviewed-by: Taylor Simpson
Re: [PATCH 1/6] ui/dmabuf: extend QemuDmaBuf to support multi-plane
On Mon, Mar 24, 2025 at 10:06 PM Marc-André Lureau
wrote:
>
> Hi
>
> On Mon, Mar 24, 2025 at 5:20 PM Qiang Yu wrote:
> >
> > On Mon, Mar 24, 2025 at 6:04 PM Marc-André Lureau
> > wrote:
> > >
> > > Hi
> > >
> > > On Mon, Mar 24, 2025 at 12:19 PM wrote:
> > > >
> > > > From: Qiang Yu
> > > >
> > > > mesa/radeonsi is going to support explicit midifier which
> > > > may export a multi-plane texture. For example, texture with
> > > > DCC enabled (a compressed format) has two planes, one with
> > > > compressed data, the other with meta data for compression.
> > > >
> > > > Signed-off-by: Qiang Yu
> > > > ---
> > > > hw/display/vhost-user-gpu.c | 6 ++-
> > > > hw/display/virtio-gpu-udmabuf.c | 6 ++-
> > > > hw/vfio/display.c | 7 ++--
> > > > include/ui/dmabuf.h | 20 ++
> > > > ui/dbus-listener.c | 10 ++---
> > > > ui/dmabuf.c | 67 +
> > > > ui/egl-helpers.c| 4 +-
> > > > ui/spice-display.c | 4 +-
> > > > 8 files changed, 76 insertions(+), 48 deletions(-)
> > > >
> > > > diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
> > > > index 2aed6243f6..a7949c7078 100644
> > > > --- a/hw/display/vhost-user-gpu.c
> > > > +++ b/hw/display/vhost-user-gpu.c
> > > > @@ -249,6 +249,8 @@ 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);
> > > > +uint32_t offset = 0;
> > > > +uint32_t stride = m->fd_stride;
> > > > uint64_t modifier = 0;
> > > > QemuDmaBuf *dmabuf;
> > > >
> > > > @@ -282,10 +284,10 @@ vhost_user_gpu_handle_display(VhostUserGPU *g,
> > > > VhostUserGpuMsg *msg)
> > > > }
> > > >
> > > > dmabuf = qemu_dmabuf_new(m->width, m->height,
> > > > - m->fd_stride, 0, 0,
> > > > + &offset, &stride, 0, 0,
> > > > m->fd_width, m->fd_height,
> > > > m->fd_drm_fourcc, modifier,
> > > > - fd, false, m->fd_flags &
> > > > + &fd, 1, false, m->fd_flags &
> > > > VIRTIO_GPU_RESOURCE_FLAG_Y_0_TOP);
> > > >
> > > > dpy_gl_scanout_dmabuf(con, dmabuf);
> > > > diff --git a/hw/display/virtio-gpu-udmabuf.c
> > > > b/hw/display/virtio-gpu-udmabuf.c
> > > > index 85ca23cb32..34fbe05b7a 100644
> > > > --- a/hw/display/virtio-gpu-udmabuf.c
> > > > +++ b/hw/display/virtio-gpu-udmabuf.c
> > > > @@ -176,16 +176,18 @@ static VGPUDMABuf
> > > >struct virtio_gpu_rect *r)
> > > > {
> > > > VGPUDMABuf *dmabuf;
> > > > +uint32_t offset = 0;
> > > >
> > > > if (res->dmabuf_fd < 0) {
> > > > return NULL;
> > > > }
> > > >
> > > > dmabuf = g_new0(VGPUDMABuf, 1);
> > > > -dmabuf->buf = qemu_dmabuf_new(r->width, r->height, fb->stride,
> > > > +dmabuf->buf = qemu_dmabuf_new(r->width, r->height,
> > > > + &offset, &fb->stride,
> > > >r->x, r->y, fb->width, fb->height,
> > > >
> > > > qemu_pixman_to_drm_format(fb->format),
> > > > - 0, res->dmabuf_fd, true, false);
> > > > + 0, &res->dmabuf_fd, 1, true, false);
> > > > dmabuf->scanout_id = scanout_id;
> > > > QTAILQ_INSERT_HEAD(&g->dmabuf.bufs, dmabuf, next);
> > > >
> > > > diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> > > > index ea87830fe0..9d882235fb 100644
> > > > --- a/hw/vfio/display.c
> > > > +++ b/hw/vfio/display.c
> > > > @@ -214,6 +214,7 @@ static VFIODMABuf
> > > > *vfio_display_get_dmabuf(VFIOPCIDevice *vdev,
> > > > struct vfio_device_gfx_plane_info plane;
> > > > VFIODMABuf *dmabuf;
> > > > int fd, ret;
> > > > +uint32_t offset = 0;
> > > >
> > > > memset(&plane, 0, sizeof(plane));
> > > > plane.argsz = sizeof(plane);
> > > > @@ -246,10 +247,10 @@ static VFIODMABuf
> > > > *vfio_display_get_dmabuf(VFIOPCIDevice *vdev,
> > > >
> > > > dmabuf = g_new0(VFIODMABuf, 1);
> > > > dmabuf->dmabuf_id = plane.dmabuf_id;
> > > > -dmabuf->buf = qemu_dmabuf_new(plane.width, plane.height,
> > > > - plane.stride, 0, 0, plane.width,
> > > > +dmabuf->buf = qemu_dmabuf_new(plane.width, plane.height, &offset,
> > > > + &plane.stride, 0, 0, plane.width,
> > > >plane.height, plane.drm_format,
> > > > - plane.drm_format_mod, fd, false,
> > > > false);
> > > > + plane.drm_f
Re: [PATCH v3 6/7] target/s390x: Register CPUClass:list_cpus
On 24/3/25 20:02, Richard Henderson wrote: On 3/24/25 11:58, Philippe Mathieu-Daudé wrote: Register s390_cpu_list() as CPUClass:list_cpus callback and remove the cpu_list definition. Signed-off-by: Philippe Mathieu-Daudé --- target/s390x/cpu.h | 3 --- target/s390x/cpu.c | 1 + 2 files changed, 1 insertion(+), 3 deletions(-) I really think this has to be merged with the previous. I strongly suspect that it either doesn't compile separately, or doesn't function as desired. It does compile, because "cpu.h" includes "cpu_models.h". I don't mind squashing if Thomas is OK. Thanks, Phil.
Re: [PATCH v2 01/11] include/exec: fix assert in size_memop
On 3/24/25 03:21, Alex Bennée wrote:
We can handle larger sized memops now, expand the range of the assert.
Fixes: 4b473e0c60 (tcg: Expand MO_SIZE to 3 bits)
Signed-off-by: Alex Bennée
---
v2
- instead of 128 use 1 << MO_SIZE for future proofing
---
include/exec/memop.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/exec/memop.h b/include/exec/memop.h
index 407a47d82c..6afe50a7d0 100644
--- a/include/exec/memop.h
+++ b/include/exec/memop.h
@@ -162,8 +162,8 @@ static inline unsigned memop_size(MemOp op)
static inline MemOp size_memop(unsigned size)
{
#ifdef CONFIG_DEBUG_TCG
-/* Power of 2 up to 8. */
-assert((size & (size - 1)) == 0 && size >= 1 && size <= 8);
+/* Power of 2 up to 128. */
+assert((size & (size - 1)) == 0 && size >= 1 && size <= (1 << MO_SIZE));
#endif
return (MemOp)ctz32(size);
}
Reviewed-by: Pierrick Bouvier
[PATCH-for-10.1 v2 0/7] cpus: Convert cpu_list definition to CPUClass:list_cpus callback
Since v1 (Thomas review comments) - Move s390_set_qemu_cpu_model/s390_cpu_list to "cpu_models.h" - Correct 'target/s390x: Register CPUClass:list_cpus' subject 'cpu_list' might be defined per target, and force code to be built per-target. We can avoid that by introducing a CPUClass callback. This series combined with another which converts CPU_RESOLVING_TYPE to a runtime helper, allows to move most of cpu-target to common. Based-on: <20250324165356.39540-1-phi...@linaro.org> Philippe Mathieu-Daudé (7): cpus: Introduce CPUClass::list_cpus() callback target/i386: Register CPUClass:list_cpus target/ppc: Register CPUClass:list_cpus target/sparc: Register CPUClass:list_cpus target/s390x: Declare s390_set_qemu_cpu_model/cpu_list in cpu_models.h target/s390x: Register CPUClass:list_cpus cpus: Remove #ifdef check on cpu_list definition include/hw/core/cpu.h | 2 ++ target/i386/cpu.h | 3 --- target/ppc/cpu.h | 4 target/s390x/cpu.h | 4 target/s390x/cpu_models.h | 3 +++ target/sparc/cpu.h | 3 --- cpu-target.c | 25 - hw/s390x/s390-virtio-ccw.c | 2 +- target/i386/cpu.c | 3 ++- target/ppc/cpu_init.c | 3 ++- target/s390x/cpu.c | 1 + target/sparc/cpu.c | 3 ++- 12 files changed, 25 insertions(+), 31 deletions(-) -- 2.47.1
[PATCH-for-10.1 v2 5/7] target/s390x: Declare s390_set_qemu_cpu_model/cpu_list in cpu_models.h
Both s390_cpu_list() and s390_set_qemu_cpu_model() are defined in cpu_models.c, move their declarations in the related "cpu_models.h" header. Use full path to header in s390-virtio-ccw.c file. Suggested-by: Thomas Huth Signed-off-by: Philippe Mathieu-Daudé --- target/s390x/cpu.h | 4 target/s390x/cpu_models.h | 3 +++ hw/s390x/s390-virtio-ccw.c | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 5b7992deda6..8dd1936e3e2 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -900,11 +900,7 @@ static inline uint8_t s390_cpu_get_state(S390CPU *cpu) } -/* cpu_models.c */ -void s390_cpu_list(void); #define cpu_list s390_cpu_list -void s390_set_qemu_cpu_model(uint16_t type, uint8_t gen, uint8_t ec_ga, - const S390FeatInit feat_init); /* helper.c */ diff --git a/target/s390x/cpu_models.h b/target/s390x/cpu_models.h index 71d4bc2dd4a..f701bc0b532 100644 --- a/target/s390x/cpu_models.h +++ b/target/s390x/cpu_models.h @@ -113,6 +113,9 @@ static inline uint64_t s390_cpuid_from_cpu_model(const S390CPUModel *model) } S390CPUDef const *s390_find_cpu_def(uint16_t type, uint8_t gen, uint8_t ec_ga, S390FeatBitmap features); +void s390_set_qemu_cpu_model(uint16_t type, uint8_t gen, uint8_t ec_ga, + const S390FeatInit feat_init); +void s390_cpu_list(void); bool kvm_s390_cpu_models_supported(void); bool kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp); diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 75b32182eb0..4f11c75b625 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -35,7 +35,7 @@ #include "hw/s390x/css-bridge.h" #include "hw/s390x/ap-bridge.h" #include "migration/register.h" -#include "cpu_models.h" +#include "target/s390x/cpu_models.h" #include "hw/nmi.h" #include "hw/qdev-properties.h" #include "hw/s390x/tod.h" -- 2.47.1
Re: [PATCH v2 03/11] gdbstub: assert earlier in handle_read_all_regs
On 3/24/25 03:21, Alex Bennée wrote: When things go wrong we want to assert on the register that failed to be able to figure out what went wrong. Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Alex Bennée --- gdbstub/gdbstub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c index 282e13e163..b6d5e11e03 100644 --- a/gdbstub/gdbstub.c +++ b/gdbstub/gdbstub.c @@ -1328,8 +1328,8 @@ static void handle_read_all_regs(GArray *params, void *user_ctx) len += gdb_read_register(gdbserver_state.g_cpu, gdbserver_state.mem_buf, reg_id); +g_assert(len == gdbserver_state.mem_buf->len); } -g_assert(len == gdbserver_state.mem_buf->len); gdb_memtohex(gdbserver_state.str_buf, gdbserver_state.mem_buf->data, len); gdb_put_strbuf(); Reviewed-by: Pierrick Bouvier
Re: [PATCH v2 02/11] include/gdbstub: fix include guard in commands.h
On 3/24/25 03:21, Alex Bennée wrote: Reviewed-by: Pierrick Bouvier Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Alex Bennée --- include/gdbstub/commands.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/gdbstub/commands.h b/include/gdbstub/commands.h index 40f0514fe9..bff3674872 100644 --- a/include/gdbstub/commands.h +++ b/include/gdbstub/commands.h @@ -1,5 +1,5 @@ #ifndef GDBSTUB_COMMANDS_H -#define GDBSTUB +#define GDBSTUB_COMMANDS_H typedef void (*GdbCmdHandler)(GArray *params, void *user_ctx); Reviewed-by: Pierrick Bouvier
Re: [PATCH v2 07/11] target/ppc: expand comment on FP/VMX/VSX access functions
On 3/24/25 03:21, Alex Bennée wrote: Mainly as an aid to myself getting confused too many bswaps deep into the code. Reviewed-by: Pierrick Bouvier Signed-off-by: Alex Bennée --- target/ppc/cpu.h | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h index efab54a068..1e833ade04 100644 --- a/target/ppc/cpu.h +++ b/target/ppc/cpu.h @@ -2906,7 +2906,12 @@ static inline bool lsw_reg_in_range(int start, int nregs, int rx) (start + nregs > 32 && (rx >= start || rx < start + nregs - 32)); } -/* Accessors for FP, VMX and VSX registers */ +/* + * Access functions for FP, VMX and VSX registers + * + * The register is stored as a 128 bit host endian value so we need to + * take that into account when accessing smaller parts of it. + */ #if HOST_BIG_ENDIAN #define VsrB(i) u8[i] #define VsrSB(i) s8[i] Reviewed-by: Pierrick Bouvier
RE: [PATCH 4/8] hw/hexagon: Add support for cfgbase
> -Original Message- > From: Brian Cain > Sent: Saturday, March 1, 2025 11:21 AM > To: qemu-devel@nongnu.org > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org; > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com; > alex.ben...@linaro.org; quic_mbur...@quicinc.com; > sidn...@quicinc.com > Subject: [PATCH 4/8] hw/hexagon: Add support for cfgbase > > From: Sid Manning > > Signed-off-by: Sid Manning Reviewed-by: Taylor Simpson
Re: [PATCH v2 09/11] target/ppc: convert gdbstub to new helpers
On 3/24/25 13:04, Richard Henderson wrote: On 3/24/25 12:29, Pierrick Bouvier wrote: On 3/24/25 10:39, Richard Henderson wrote: On 3/24/25 03:21, Alex Bennée wrote: + #ifdef TARGET_BIG_ENDIAN + MemOp end = MO_BE; + #else + MemOp end = MO_LE; + #endif +#endif That's what MO_TE is for. +/* + * Helpers copied from helpers.h just for handling target_ulong values + * from gdbstub's GByteArray based on what the build config is. This + * will need fixing for single-binary. + */ + +#if TARGET_LONG_BITS == 64 +#define ldtul_p(addr) ldq_p(addr) +#define gdb_get_regl_value(m, b, v) gdb_get_reg64_value(m, b, v) +#else +#define ldtul_p(addr) ldl_p(addr) +#define gdb_get_regl_value(m, b, v) gdb_get_reg32_value(m, b, v) +#endif Surely you're not intending to replicate this in any target that can have multiple sizes? This is not an improvement. If I'm correct (and from v1), ppc is the only architecture having registers defined with target_long types. With a quick check, mips, riscv, sparc do as well. Right, I should have run the simple grep :) So it's better to keep those macros defined in a separate header (out of helpers.h, like helpers-target.h), so we can already start to cleanup some targets, and do the rest of the work for the ones using target_ulong type for later. r~
Re: [PATCH v8 00/20] Change ghes to use HEST-based offsets and add support for error inject
Hi Michael,
Gentile ping.
Regards,
Mauro
Em Fri, 7 Mar 2025 20:14:29 +0100
Mauro Carvalho Chehab escreveu:
> Hi Michael,
>
> I'm sending v8 to avoid a merge conflict with v7 due to this
> changeset:
>
>611f3bdb20f7 ("hw/acpi/ghes: Make static")
>
> As ghes_record_cper_errors() was written since the beginning
> to be public and used by ghes-cper.c. It ended being meged
> earlier because the error-injection series become too big,
> so it was decided last year to split in two to make easier for
> reviewers and maintainers to discuss.
>
> Anyway, as mentioned on v7, I guess we're ready to merge this
> series, as patches here have been thoughfully reviewed mainly
> by Igor and Jonathan over the last 5-6 months.
>
> The only change from v7 is a minor editorial change at HEST doc
> spec, and the addition of Igor and Jonathan's A-B/R-B.
>
> This series change the way HEST table offsets are calculated,
> making them identical to what an OSPM would do and allowing
> multiple HEST entries without causing migration issues. It open
> space to add HEST support for non-arm architectures, as now
> the number and type of HEST notification entries are not
> hardcoded at ghes.c. Instead, they're passed as a parameter
> from the arch-dependent init code.
>
> With such issue addressed, it adds a new notification type and
> add support to inject errors via a Python script. The script
> itself is at the final patch.
>
> ---
> v8:
> - added a patch to revert recently-added changeset causing a
> conflict with these. All remaining patches are identical.
>
> v7:
> - minor editorial change at the patch updating HEST doc spec
>with the new workflow
>
> v6:
> - some minor nits addressed:
>- use GPA instead of offset;
>- merged two patches;
>- fixed a couple of long line coding style issues;
>- the HEST/DSDT diff inside a patch was changed to avoid troubles
> applying it.
>
> v5:
> - make checkpatch happier;
> - HEST table is now tested;
> - some changes at HEST spec documentation to align with code changes;
> - extra care was taken with regards to git bisectability.
>
> v4:
> - added an extra comment for AcpiGhesState structure;
> - patches reordered;
> - no functional changes, just code shift between the patches in this series.
>
> v3:
> - addressed more nits;
> - hest_add_le now points to the beginning of HEST table;
> - removed HEST from tests/data/acpi;
> - added an extra patch to not use fw_cfg with virt-10.0 for hw_error_le
>
> v2:
> - address some nits;
> - improved ags cleanup patch and removed ags.present field;
> - added some missing le*_to_cpu() calls;
> - update date at copyright for new files to 2024-2025;
> - qmp command changed to: inject-ghes-v2-error ans since updated to 10.0;
> - added HEST and DSDT tables after the changes to make check target happy.
> (two patches: first one whitelisting such tables; second one removing from
>whitelist and updating/adding such tables to tests/data/acpi)
>
> Mauro Carvalho Chehab (20):
> tests/acpi: virt: add an empty HEST file
> tests/qtest/bios-tables-test: extend to also check HEST table
> tests/acpi: virt: update HEST file with its current data
> Revert "hw/acpi/ghes: Make ghes_record_cper_errors() static"
> acpi/ghes: Cleanup the code which gets ghes ged state
> acpi/ghes: prepare to change the way HEST offsets are calculated
> acpi/ghes: add a firmware file with HEST address
> acpi/ghes: Use HEST table offsets when preparing GHES records
> acpi/ghes: don't hard-code the number of sources for HEST table
> acpi/ghes: add a notifier to notify when error data is ready
> acpi/generic_event_device: Update GHES migration to cover hest addr
> acpi/generic_event_device: add logic to detect if HEST addr is
> available
> acpi/generic_event_device: add an APEI error device
> tests/acpi: virt: allow acpi table changes at DSDT and HEST tables
> arm/virt: Wire up a GED error device for ACPI / GHES
> qapi/acpi-hest: add an interface to do generic CPER error injection
> acpi/generic_event_device.c: enable use_hest_addr for QEMU 10.x
> tests/acpi: virt: update HEST and DSDT tables
> docs: hest: add new "etc/acpi_table_hest_addr" and update workflow
> scripts/ghes_inject: add a script to generate GHES error inject
>
> MAINTAINERS | 10 +
> docs/specs/acpi_hest_ghes.rst | 28 +-
> hw/acpi/Kconfig | 5 +
> hw/acpi/aml-build.c | 10 +
> hw/acpi/generic_event_device.c| 44 ++
> hw/acpi/ghes-stub.c | 7 +-
> hw/acpi/ghes.c| 233 --
> hw/acpi/ghes_cper.c | 38 +
> hw/acpi/ghes_cper_stub.c | 19 +
> hw/acpi/meson.build | 2 +
> hw/arm/virt-acpi-build.c | 35 +-
> hw/arm/virt.c
RE: [PATCH 2/2] target/hexagon: Drop `ident` postprocess step
> -Original Message-
> From: Anton Johansson
> Sent: Wednesday, March 12, 2025 2:46 PM
> To: qemu-devel@nongnu.org
> Cc: a...@rev.ng; ltaylorsimp...@gmail.com; brian.c...@oss.qualcomm.com;
> phi...@linaro.org
> Subject: [PATCH 2/2] target/hexagon: Drop `ident` postprocess step
>
> The indent command is not available on a default mac osx setup with xcode
> cli tools installed. While it does make idef-parser generated code nicer
to
> debug, it's not crucial and can be dropped.
>
> Signed-off-by: Anton Johansson
> ---
> target/hexagon/meson.build | 21 ++---
> 1 file changed, 2 insertions(+), 19 deletions(-)
>
> diff --git a/target/hexagon/meson.build b/target/hexagon/meson.build
> index abcf00ca1f..246dc7b241 100644
> --- a/target/hexagon/meson.build
> +++ b/target/hexagon/meson.build
> @@ -323,30 +323,13 @@ if idef_parser_enabled and 'hexagon-linux-user' in
> target_dirs
> command: [idef_parser, '@INPUT@', '@OUTPUT0@', '@OUTPUT1@',
> '@OUTPUT2@']
> )
>
> -indent = find_program('indent', required: false)
> -if indent.found()
> -idef_generated_tcg_c = custom_target(
> -'indent',
> -input: idef_generated_tcg[0],
> -output: 'idef-generated-emitter.indented.c',
> -command: [indent, '-linux', '@INPUT@', '-o', '@OUTPUT@']
> -)
> -else
> -idef_generated_tcg_c = custom_target(
> -'copy',
> -input: idef_generated_tcg[0],
> -output: 'idef-generated-emitter.indented.c',
> -command: ['cp', '@INPUT@', '@OUTPUT@']
> -)
> -endif
> -
I prefer to have the indented version present.
Is the above check/fallback not sufficient on MacOS? It works on a Linux
system where indent is not present.
Thanks,
Taylor
[PATCH v2] hw/i386/amd_iommu: Assign pci-id 0x1419 for the AMD IOMMU device
Currently, the QEMU-emulated AMD IOMMU device use PCI vendor id 0x1022 (AMD) with device id zero (undefined). Eventhough this does not cause any functional issue for AMD IOMMU driver since it normally uses information in the ACPI IVRS table to probe and initialize the device per recommendation in the AMD IOMMU specification, the device id zero causes the Windows Device Manager utility to show the device as an unknown device. Since Windows only recognizes AMD IOMMU device with device id 0x1419 as listed in the machine.inf file, modify the QEMU AMD IOMMU model to use the id 0x1419 to avoid the issue. This advertise the IOMMU as the AMD IOMMU device for Family 15h (Models 10h-1fh). Signed-off-by: Suravee Suthikulpanit --- Changes from v1 (https://lore.kernel.org/all/20250304183747.639382-1-suravee.suthikulpa...@amd.com/) * Use the existing device id 0x1419 instead of the proposed new id. hw/i386/amd_iommu.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c index dda1a5781f..b006ead804 100644 --- a/hw/i386/amd_iommu.c +++ b/hw/i386/amd_iommu.c @@ -1767,6 +1767,7 @@ static void amdvi_pci_class_init(ObjectClass *klass, void *data) PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); k->vendor_id = PCI_VENDOR_ID_AMD; +k->device_id = 0x1419; k->class_id = 0x0806; k->realize = amdvi_pci_realize; -- 2.34.1
Re: [PATCH v7 1/2] hw/loongarch/virt: Fix cpuslot::cpu set at last in virt_cpu_plug()
Markus,
Thanks for your reviewing and guidance.
Regards
Bibo Mao
On 2025/3/24 下午2:05, Markus Armbruster wrote:
Bibo Mao writes:
In function virt_cpu_plug(), Object cpuslot::cpu is set at last
only when there is no any error, otherwise it is problematic that
cpuslot::cpu is set in advance however it returns because of error.
Fixes: ab9935d2991e (hw/loongarch/virt: Implement cpu plug interface)
Signed-off-by: Bibo Mao
---
hw/loongarch/virt.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index e25864214f..504f8755a0 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -973,8 +973,6 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
Error *err = NULL;
-cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
-cpu_slot->cpu = CPU(dev);
if (lvms->ipi) {
hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), dev, &err);
if (err) {
@@ -998,6 +996,8 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
}
}
+cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
+cpu_slot->cpu = CPU(dev);
return;
}
Reviewed-by: Markus Armbruster
Re: [PATCH 2/2] target/hexagon: Drop `ident` postprocess step
On 3/24/2025 8:53 PM, ltaylorsimp...@gmail.com wrote:
-Original Message-
From: Anton Johansson
Sent: Wednesday, March 12, 2025 2:46 PM
To: qemu-devel@nongnu.org
Cc: a...@rev.ng; ltaylorsimp...@gmail.com; brian.c...@oss.qualcomm.com;
phi...@linaro.org
Subject: [PATCH 2/2] target/hexagon: Drop `ident` postprocess step
The indent command is not available on a default mac osx setup with xcode
cli tools installed. While it does make idef-parser generated code nicer
to
debug, it's not crucial and can be dropped.
Signed-off-by: Anton Johansson
---
target/hexagon/meson.build | 21 ++---
1 file changed, 2 insertions(+), 19 deletions(-)
diff --git a/target/hexagon/meson.build b/target/hexagon/meson.build
index abcf00ca1f..246dc7b241 100644
--- a/target/hexagon/meson.build
+++ b/target/hexagon/meson.build
@@ -323,30 +323,13 @@ if idef_parser_enabled and 'hexagon-linux-user' in
target_dirs
command: [idef_parser, '@INPUT@', '@OUTPUT0@', '@OUTPUT1@',
'@OUTPUT2@']
)
-indent = find_program('indent', required: false)
-if indent.found()
-idef_generated_tcg_c = custom_target(
-'indent',
-input: idef_generated_tcg[0],
-output: 'idef-generated-emitter.indented.c',
-command: [indent, '-linux', '@INPUT@', '-o', '@OUTPUT@']
-)
-else
-idef_generated_tcg_c = custom_target(
-'copy',
-input: idef_generated_tcg[0],
-output: 'idef-generated-emitter.indented.c',
-command: ['cp', '@INPUT@', '@OUTPUT@']
-)
-endif
-
I prefer to have the indented version present.
Is the above check/fallback not sufficient on MacOS? It works on a Linux
system where indent is not present.
Aside: could using "clang-format" be another approach? I suppose it's
just exchanging one dependency for another, but maybe xcode comes w/this
tool? Then again, maybe it would be an inconvenient dependency on linux
systems?
Thanks,
Taylor
[PATCH v4 3/8] hw/ppc: Trigger Fadump boot if fadump is registered
According to PAPR:
R1–7.3.30–3. When the platform receives an ibm,os-term RTAS call, or
on a system reset without an ibm,nmi-interlock RTAS call, if the
platform has a dump structure registered through the
ibm,configure-kernel-dump call, the platform must process each
registered kernel dump section as required and, when available,
present the dump structure information to the operating system
through the “ibm,kernel-dump” property, updated with status for each
dump section, until the dump has been invalidated through the
ibm,configure-kernel-dump RTAS call.
If Fadump has been registered, trigger an Fadump boot (memory preserving
boot), if QEMU recieves a 'ibm,os-term' rtas call.
Implementing the fadump boot as:
* pause all vcpus (will need to save registers later)
* preserve memory regions specified by fadump (will be implemented
in future)
* do a memory preserving reboot (GUEST_RESET in QEMU doesn't clear
the memory)
Memory regions registered by fadump will be handled in a later patch.
Note: Preserving memory regions is not implemented yet so on an
"ibm,os-term" call will just trigger a reboot in QEMU if fadump is
registered, and the second kernel will boot as a normal boot (not
fadump boot)
Signed-off-by: Aditya Gupta
---
hw/ppc/spapr_fadump.c | 77 +++
hw/ppc/spapr_rtas.c | 5 +++
include/hw/ppc/spapr_fadump.h | 6 +++
3 files changed, 88 insertions(+)
diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
index 9c7fb9e12b16..fedd2cde9a4f 100644
--- a/hw/ppc/spapr_fadump.c
+++ b/hw/ppc/spapr_fadump.c
@@ -7,6 +7,7 @@
#include "qemu/osdep.h"
#include "qemu/log.h"
#include "hw/ppc/spapr.h"
+#include "system/cpus.h"
/*
* Handle the "FADUMP_CMD_REGISTER" command in 'ibm,configure-kernel-dump'
@@ -121,3 +122,79 @@ uint32_t do_fadump_register(SpaprMachineState *spapr,
target_ulong args)
return RTAS_OUT_SUCCESS;
}
+
+/* Preserve the memory locations registered for fadump */
+static bool fadump_preserve_mem(void)
+{
+/*
+ * TODO: Implement preserving memory regions requested during fadump
+ * registration
+ */
+return false;
+}
+
+/*
+ * Trigger a fadump boot, ie. next boot will be a crashkernel/fadump boot
+ * with fadump dump active.
+ *
+ * This is triggered by ibm,os-term RTAS call, if fadump was registered.
+ *
+ * It preserves the memory and sets 'FADUMP_STATUS_DUMP_TRIGGERED' as
+ * fadump status, which can be used later to add the "ibm,kernel-dump"
+ * device tree node as presence of 'FADUMP_STATUS_DUMP_TRIGGERED' signifies
+ * next boot as fadump boot in our case
+ */
+void trigger_fadump_boot(SpaprMachineState *spapr, target_ulong spapr_retcode)
+{
+FadumpSectionHeader *header = &spapr->registered_fdm.header;
+
+pause_all_vcpus();
+
+/* Preserve the memory locations registered for fadump */
+if (!fadump_preserve_mem()) {
+/* Failed to preserve the registered memory regions */
+rtas_st(spapr_retcode, 0, RTAS_OUT_HW_ERROR);
+
+/* Cause a reboot */
+qemu_system_guest_panicked(NULL);
+return;
+}
+
+/*
+ * Mark next boot as fadump boot
+ *
+ * Note: These is some bit of assumption involved here, as PAPR doesn't
+ * specify any use of the dump status flags, nor does the kernel use it
+ *
+ * But from description in Table 136 in PAPR v2.13, it looks like:
+ * FADUMP_STATUS_DUMP_TRIGGERED
+ * = Dump was triggered by the previous system boot (PAPR says)
+ * = Next boot will be a fadump boot (My perception)
+ *
+ * FADUMP_STATUS_DUMP_PERFORMED
+ * = Dump performed (Set to 0 by caller of the
+ *ibm,configure-kernel-dump call) (PAPR says)
+ * = Firmware has performed the copying/dump of requested regions
+ *(My perception)
+ * = Dump is active for the next boot (My perception)
+ */
+header->dump_status_flag = cpu_to_be16(
+FADUMP_STATUS_DUMP_TRIGGERED | /* Next boot will be fadump boot */
+FADUMP_STATUS_DUMP_PERFORMED/* Dump is active */
+);
+
+/* Reset fadump_registered for next boot */
+spapr->fadump_registered = false;
+spapr->fadump_dump_active = true;
+
+/*
+ * Then do a guest reset
+ *
+ * Requirement:
+ * GUEST_RESET is expected to NOT clear the memory, as is the case when
+ * this is merged
+ */
+qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+
+rtas_st(spapr_retcode, 0, RTAS_OUT_SUCCESS);
+}
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 0454938a01e9..14b954a05109 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -412,6 +412,11 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
target_ulong msgaddr = rtas_ld(args, 0);
char msg[512];
+if (spapr->fadump_registered) {
+/* If fadump boot works, control won't come back here */
+ret
[PATCH v2 06/30] exec/cpu-all: remove exec/page-protection include
Signed-off-by: Pierrick Bouvier --- include/exec/cpu-all.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h index eb029b65552..4a2cac1252d 100644 --- a/include/exec/cpu-all.h +++ b/include/exec/cpu-all.h @@ -19,7 +19,6 @@ #ifndef CPU_ALL_H #define CPU_ALL_H -#include "exec/page-protection.h" #include "exec/cpu-common.h" #include "exec/cpu-interrupt.h" #include "exec/tswap.h" -- 2.39.5
Re: [PATCH v2 11/11] include/gdbstub: add note to helpers.h
On 3/24/25 10:29, Alex Bennée wrote: Philippe Mathieu-Daudé writes: On 24/3/25 11:21, Alex Bennée wrote: We've not yet deprecated but we should steer users away from these helpers if they want to be in a single/heterogeneous binary. Why not deprecate? I guess philosophically do we expect to eventually convert all frontends to the new API or only those that want to be in the single binary? Should I just be more explicit: * * These are all used by the various frontends and have to be host - * aware to ensure things are store in target order. + * aware to ensure things are store in target order. Consider using + * the endian neutral registers.h if you want the architecture to be + * included in an eventual single QEMU binary. * * Copyright (c) 2022 Linaro Ltd * These are all used by the various frontends and have to be host aware to ensure things are store in target order. If the fix is an easy "sed like" with static typing guarantee (so no bug can be introduced), we can probably just do it on all existing targets, and remove this. If it's hard or error prone, then I would say it's ok to use a "one target at a time" approach. New front-ends should not be using these APIs at all. They should be using the endian neutral registers.h as should any architecture that intends to be included in an eventual single QEMU binary.
Re: [PATCH v2 04/11] gdbstub: introduce target independent gdb register helper
On 3/24/25 03:21, Alex Bennée wrote:
The current helper.h functions rely on hard coded assumptions about
target endianess to use the tswap macros. We also end up double
swapping a bunch of values if the target can run in multiple endianess
modes. Avoid this by getting the target to pass the endianess and size
via a MemOp and fixing up appropriately.
Reviewed-by: Pierrick Bouvier
Signed-off-by: Alex Bennée
---
v2
- use unsigned consistently
- fix some rouge whitespace
- add typed reg32/64 wrappers
- pass void * to underlying helper to avoid casting
---
include/gdbstub/registers.h | 55 +
gdbstub/gdbstub.c | 23
2 files changed, 78 insertions(+)
create mode 100644 include/gdbstub/registers.h
diff --git a/include/gdbstub/registers.h b/include/gdbstub/registers.h
new file mode 100644
index 00..2220f58efe
--- /dev/null
+++ b/include/gdbstub/registers.h
@@ -0,0 +1,55 @@
+/*
+ * GDB Common Register Helpers
+ *
+ * Copyright (c) 2025 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef GDB_REGISTERS_H
+#define GDB_REGISTERS_H
+
+#include "exec/memop.h"
+
+/**
+ * gdb_get_register_value() - get register value for gdb
+ * mo: size and endian MemOp
+ * buf: GByteArray to store in target order
+ * val: pointer to value in host order
+ *
+ * This replaces the previous legacy read functions with a single
+ * function to handle all sizes. Passing @mo allows the target mode to
+ * be taken into account and avoids using hard coded tswap() macros.
+ *
+ * There are wrapper functions for the common sizes you can use to
+ * keep type checking.
+ *
+ * Returns the number of bytes written to the array.
+ */
+int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
+
Could it be made static, so it's hidden from the public interface?
You'll need to un-inline gdb_get_reg*_value functions. I doubt it's
performance critical and need to be inline.
+/**
+ * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
+ * mo: size and endian MemOp
+ * buf: GByteArray to store in target order
+ * val: pointer to uint32_t value in host order
+ */
+static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t
*val) {
+g_assert((op & MO_SIZE) == MO_32);
+return gdb_get_register_value(op, buf, val);
+}
+
+/**
+ * gdb_get_reg64_value() - type checked wrapper for gdb_get_register_value()
+ * mo: size and endian MemOp
+ * buf: GByteArray to store in target order
+ * val: pointer to uint32_t value in host order
+ */
+static inline int gdb_get_reg64_value(MemOp op, GByteArray *buf, uint64_t
*val) {
+g_assert((op & MO_SIZE) == MO_64);
+return gdb_get_register_value(op, buf, val);
+}
+
+#endif /* GDB_REGISTERS_H */
+
+
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index b6d5e11e03..e799fdc019 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -32,6 +32,7 @@
#include "exec/gdbstub.h"
#include "gdbstub/commands.h"
#include "gdbstub/syscalls.h"
+#include "gdbstub/registers.h"
#ifdef CONFIG_USER_ONLY
#include "accel/tcg/vcpu-state.h"
#include "gdbstub/user.h"
@@ -45,6 +46,7 @@
#include "system/runstate.h"
#include "exec/replay-core.h"
#include "exec/hwaddr.h"
+#include "exec/memop.h"
#include "internals.h"
@@ -551,6 +553,27 @@ static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
return 0;
}
+/*
+ * Target helper function to read value into GByteArray, target
+ * supplies the size and target endianess via the MemOp.
+ */
+int gdb_get_register_value(MemOp op, GByteArray *buf, void *val)
+{
+unsigned bytes = memop_size(op);
+
+if (op & MO_BSWAP) {
+uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
+for (unsigned i = bytes; i > 0; i--) {
+g_byte_array_append(buf, ptr--, 1);
+};
+} else {
+g_byte_array_append(buf, val, bytes);
+}
+
+return bytes;
+}
+
+
static void gdb_register_feature(CPUState *cpu, int base_reg,
gdb_get_reg_cb get_reg, gdb_set_reg_cb
set_reg,
const GDBFeature *feature)
Re: [PATCH v2 11/11] include/gdbstub: add note to helpers.h
On 3/24/25 10:29, Alex Bennée wrote: Philippe Mathieu-Daudé writes: On 24/3/25 11:21, Alex Bennée wrote: We've not yet deprecated but we should steer users away from these helpers if they want to be in a single/heterogeneous binary. Why not deprecate? I guess philosophically do we expect to eventually convert all frontends to the new API or only those that want to be in the single binary? Should I just be more explicit: * * These are all used by the various frontends and have to be host - * aware to ensure things are store in target order. + * aware to ensure things are store in target order. Consider using + * the endian neutral registers.h if you want the architecture to be + * included in an eventual single QEMU binary. * * Copyright (c) 2022 Linaro Ltd * These are all used by the various frontends and have to be host aware to ensure things are store in target order. If the fix is an easy "sed like" with static typing guarantee (so no bug can be introduced), we can probably just do it on all existing targets, and remove this. If it's hard or error prone, then I would say it's ok to use a "one target at a time" approach. New front-ends should not be using these APIs at all. They should be using the endian neutral registers.h as should any architecture that intends to be included in an eventual single QEMU binary.
Re: [PATCH v2 04/11] gdbstub: introduce target independent gdb register helper
On 3/24/25 03:21, Alex Bennée wrote:
The current helper.h functions rely on hard coded assumptions about
target endianess to use the tswap macros. We also end up double
swapping a bunch of values if the target can run in multiple endianess
modes. Avoid this by getting the target to pass the endianess and size
via a MemOp and fixing up appropriately.
Reviewed-by: Pierrick Bouvier
Signed-off-by: Alex Bennée
---
v2
- use unsigned consistently
- fix some rouge whitespace
- add typed reg32/64 wrappers
- pass void * to underlying helper to avoid casting
---
include/gdbstub/registers.h | 55 +
gdbstub/gdbstub.c | 23
2 files changed, 78 insertions(+)
create mode 100644 include/gdbstub/registers.h
diff --git a/include/gdbstub/registers.h b/include/gdbstub/registers.h
new file mode 100644
index 00..2220f58efe
--- /dev/null
+++ b/include/gdbstub/registers.h
@@ -0,0 +1,55 @@
+/*
+ * GDB Common Register Helpers
+ *
+ * Copyright (c) 2025 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef GDB_REGISTERS_H
+#define GDB_REGISTERS_H
+
+#include "exec/memop.h"
+
+/**
+ * gdb_get_register_value() - get register value for gdb
+ * mo: size and endian MemOp
+ * buf: GByteArray to store in target order
+ * val: pointer to value in host order
+ *
+ * This replaces the previous legacy read functions with a single
+ * function to handle all sizes. Passing @mo allows the target mode to
+ * be taken into account and avoids using hard coded tswap() macros.
+ *
+ * There are wrapper functions for the common sizes you can use to
+ * keep type checking.
+ *
+ * Returns the number of bytes written to the array.
+ */
+int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
+
+/**
+ * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
+ * mo: size and endian MemOp
+ * buf: GByteArray to store in target order
+ * val: pointer to uint32_t value in host order
+ */
+static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t
*val) {
+g_assert((op & MO_SIZE) == MO_32);
+return gdb_get_register_value(op, buf, val);
+}
+
After reading the whole series, I think I am missing the point of
introduce op here.
If what we really want is just the target endianness, why not add the
"if target_words_bigendian()" in the wrapper here?
Are there cases where we want another endianness than target one?
+/**
+ * gdb_get_reg64_value() - type checked wrapper for gdb_get_register_value()
+ * mo: size and endian MemOp
+ * buf: GByteArray to store in target order
+ * val: pointer to uint32_t value in host order
+ */
+static inline int gdb_get_reg64_value(MemOp op, GByteArray *buf, uint64_t
*val) {
+g_assert((op & MO_SIZE) == MO_64);
+return gdb_get_register_value(op, buf, val);
+}
+
+#endif /* GDB_REGISTERS_H */
+
+
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index b6d5e11e03..e799fdc019 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -32,6 +32,7 @@
#include "exec/gdbstub.h"
#include "gdbstub/commands.h"
#include "gdbstub/syscalls.h"
+#include "gdbstub/registers.h"
#ifdef CONFIG_USER_ONLY
#include "accel/tcg/vcpu-state.h"
#include "gdbstub/user.h"
@@ -45,6 +46,7 @@
#include "system/runstate.h"
#include "exec/replay-core.h"
#include "exec/hwaddr.h"
+#include "exec/memop.h"
#include "internals.h"
@@ -551,6 +553,27 @@ static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
return 0;
}
+/*
+ * Target helper function to read value into GByteArray, target
+ * supplies the size and target endianess via the MemOp.
+ */
+int gdb_get_register_value(MemOp op, GByteArray *buf, void *val)
+{
+unsigned bytes = memop_size(op);
+
+if (op & MO_BSWAP) {
+uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
+for (unsigned i = bytes; i > 0; i--) {
+g_byte_array_append(buf, ptr--, 1);
+};
+} else {
+g_byte_array_append(buf, val, bytes);
+}
+
+return bytes;
+}
+
+
static void gdb_register_feature(CPUState *cpu, int base_reg,
gdb_get_reg_cb get_reg, gdb_set_reg_cb
set_reg,
const GDBFeature *feature)
Re: [PATCH v2 09/11] target/ppc: convert gdbstub to new helpers
On 3/24/25 12:29, Pierrick Bouvier wrote: On 3/24/25 10:39, Richard Henderson wrote: On 3/24/25 03:21, Alex Bennée wrote: + #ifdef TARGET_BIG_ENDIAN + MemOp end = MO_BE; + #else + MemOp end = MO_LE; + #endif +#endif That's what MO_TE is for. +/* + * Helpers copied from helpers.h just for handling target_ulong values + * from gdbstub's GByteArray based on what the build config is. This + * will need fixing for single-binary. + */ + +#if TARGET_LONG_BITS == 64 +#define ldtul_p(addr) ldq_p(addr) +#define gdb_get_regl_value(m, b, v) gdb_get_reg64_value(m, b, v) +#else +#define ldtul_p(addr) ldl_p(addr) +#define gdb_get_regl_value(m, b, v) gdb_get_reg32_value(m, b, v) +#endif Surely you're not intending to replicate this in any target that can have multiple sizes? This is not an improvement. If I'm correct (and from v1), ppc is the only architecture having registers defined with target_long types. With a quick check, mips, riscv, sparc do as well. r~
Re: [PATCH v2 05/11] target/arm: convert 32 bit gdbstub to new helpers
On 3/24/25 03:21, Alex Bennée wrote:
For some of the helpers we need a temporary variable to copy from
although we could add some helpers to return pointers into env in
those cases if we wanted to.
Reviewed-by: Pierrick Bouvier
Signed-off-by: Alex Bennée
---
v2
- use new wrappers
- explicit MO_32 usage and reg32/64 helpers
---
target/arm/gdbstub.c | 55 +++-
1 file changed, 34 insertions(+), 21 deletions(-)
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 30068c2262..71d672ace5 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -20,7 +20,7 @@
#include "qemu/osdep.h"
#include "cpu.h"
#include "exec/gdbstub.h"
-#include "gdbstub/helpers.h"
+#include "gdbstub/registers.h"
#include "gdbstub/commands.h"
#include "system/tcg.h"
#include "internals.h"
@@ -33,12 +33,16 @@ typedef struct RegisterSysregFeatureParam {
int n;
} RegisterSysregFeatureParam;
-/* Old gdb always expect FPA registers. Newer (xml-aware) gdb only expect
- whatever the target description contains. Due to a historical mishap
- the FPA registers appear in between core integer regs and the CPSR.
- We hack round this by giving the FPA regs zero size when talking to a
- newer gdb. */
-
+/*
+ * Old gdb always expect FPA registers. Newer (xml-aware) gdb only
+ * expect whatever the target description contains. Due to a
+ * historical mishap the FPA registers appear in between core integer
+ * regs and the CPSR. We hack round this by giving the FPA regs zero
+ * size when talking to a newer gdb.
+ *
+ * While gdb cares about the memory endianess of the target all
+ * registers are passed in little-endian mode.
+ */
int arm_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
{
ARMCPU *cpu = ARM_CPU(cs);
@@ -46,15 +50,17 @@ int arm_cpu_gdb_read_register(CPUState *cs, GByteArray
*mem_buf, int n)
if (n < 16) {
/* Core integer register. */
-return gdb_get_reg32(mem_buf, env->regs[n]);
+return gdb_get_reg32_value(MO_TE | MO_32, mem_buf, &env->regs[n]);
}
if (n == 25) {
/* CPSR, or XPSR for M-profile */
+uint32_t reg;
if (arm_feature(env, ARM_FEATURE_M)) {
-return gdb_get_reg32(mem_buf, xpsr_read(env));
+reg = xpsr_read(env);
} else {
-return gdb_get_reg32(mem_buf, cpsr_read(env));
+reg = cpsr_read(env);
}
+return gdb_get_reg32_value(MO_TE | MO_32, mem_buf, ®);
}
/* Unknown register. */
return 0;
@@ -115,19 +121,21 @@ static int vfp_gdb_get_reg(CPUState *cs, GByteArray *buf,
int reg)
/* VFP data registers are always little-endian. */
if (reg < nregs) {
-return gdb_get_reg64(buf, *aa32_vfp_dreg(env, reg));
+return gdb_get_reg64_value(MO_TE | MO_64, buf, aa32_vfp_dreg(env,
reg));
}
if (arm_feature(env, ARM_FEATURE_NEON)) {
/* Aliases for Q regs. */
nregs += 16;
if (reg < nregs) {
uint64_t *q = aa32_vfp_qreg(env, reg - 32);
-return gdb_get_reg128(buf, q[0], q[1]);
+return gdb_get_reg64_value(MO_TE | MO_64, buf, q);
}
What about upper part of register? s/128/64 seems unintended, and buf
will only contain lower part.
Probably needed to introduce gdb_get_reg128_value helper as well.
}
switch (reg - nregs) {
+uint32_t fpcr;
case 0:
-return gdb_get_reg32(buf, vfp_get_fpscr(env));
+fpcr = vfp_get_fpscr(env);
+return gdb_get_reg32_value(MO_TE | MO_32, buf, &fpcr);
}
return 0;
}
@@ -166,9 +174,11 @@ static int vfp_gdb_get_sysreg(CPUState *cs, GByteArray
*buf, int reg)
switch (reg) {
case 0:
-return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPSID]);
+return gdb_get_reg32_value(MO_TE | MO_32, buf,
+ &env->vfp.xregs[ARM_VFP_FPSID]);
case 1:
-return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPEXC]);
+return gdb_get_reg32_value(MO_TE | MO_32, buf,
+ &env->vfp.xregs[ARM_VFP_FPEXC]);
}
return 0;
}
@@ -196,7 +206,7 @@ static int mve_gdb_get_reg(CPUState *cs, GByteArray *buf,
int reg)
switch (reg) {
case 0:
-return gdb_get_reg32(buf, env->v7m.vpr);
+return gdb_get_reg32_value(MO_TE | MO_32, buf, &env->v7m.vpr);
default:
return 0;
}
@@ -236,9 +246,11 @@ static int arm_gdb_get_sysreg(CPUState *cs, GByteArray
*buf, int reg)
ri = get_arm_cp_reginfo(cpu->cp_regs, key);
if (ri) {
if (cpreg_field_is_64bit(ri)) {
-return gdb_get_reg64(buf, (uint64_t)read_raw_cp_reg(env, ri));
+uint64_t cpreg = read_raw_cp_reg(env, ri);
+return gdb_get_register_value(MO_TEUQ, buf, (uint8_t *) &cpreg);
} else {
-return gdb_get_reg32(buf,
Re: [PULL 0/3] aspeed queue
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/10.0 for any user-visible changes. signature.asc Description: PGP signature
Re: [PATCH v2 24/30] meson: add common hw files
On 3/23/25 12:58, Richard Henderson wrote:
On 3/20/25 15:29, Pierrick Bouvier wrote:
Those files will be compiled once per base architecture ("arm" in this
case), instead of being compiled for every variant/bitness of
architecture.
We make sure to not include target cpu definitions (exec/cpu-defs.h) by
defining header guard directly. This way, a given compilation unit can
access a specific cpu definition, but not access to compile time defines
associated.
Previous commits took care to clean up some headers to not rely on
cpu-defs.h content.
Signed-off-by: Pierrick Bouvier
---
meson.build | 37 -
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index c21974020dd..994d3e5d536 100644
--- a/meson.build
+++ b/meson.build
@@ -3691,6 +3691,7 @@ hw_arch = {}
target_arch = {}
target_system_arch = {}
target_user_arch = {}
+hw_common_arch = {}
# NOTE: the trace/ subdirectory needs the qapi_trace_events variable
# that is filled in by qapi/.
@@ -4089,6 +4090,34 @@ common_all = static_library('common',
implicit_include_directories: false,
dependencies: common_ss.all_dependencies())
+# construct common libraries per base architecture
+hw_common_arch_libs = {}
+foreach target : target_dirs
+ config_target = config_target_mak[target]
+ target_base_arch = config_target['TARGET_BASE_ARCH']
+
+ # check if already generated
+ if target_base_arch in hw_common_arch_libs
+continue
+ endif
+
+ if target_base_arch in hw_common_arch
+target_inc = [include_directories('target' / target_base_arch)]
+src = hw_common_arch[target_base_arch]
+lib = static_library(
+ 'hw_' + target_base_arch,
+ build_by_default: false,
+ sources: src.all_sources() + genh,
+ include_directories: common_user_inc + target_inc,
+ implicit_include_directories: false,
+ # prevent common code to access cpu compile time
+ # definition, but still allow access to cpu.h
+ c_args: ['-DCPU_DEFS_H', '-DCOMPILING_SYSTEM_VS_USER',
'-DCONFIG_SOFTMMU'],
Oof. This really seems like a hack, but it does work,
and I'm not sure what else to suggest.
Yes, it's the best (least-worst in reality) solution I found.
Initially I simply tried to add them to libsystem.
However, it has some problems:
- Impossible to link arch files only for concerned targets (or you need
to add when: [TARGET_X] everywhere, which is not convenient).
- They need specific flags (most notably header guard -DCPU_DEFS_H, to
ensure we don't rely on cpu compile time defines), which is only
achievable through static lib hack already used in our build system. So
another library needs to be declared.
All the rest of the meson-foo looks ok, but a second eye couldn't hurt.
If someone else has a better idea achieving the same result (maybe
Paolo?), I would be happy to implement it.
Acked-by: Richard Henderson
r~
Re: [PATCH] docs/devel/build-environment: enhance MSYS2 instructions
Cc'ing Stefan and Yonggang On 5/3/25 22:38, Pierrick Bouvier wrote: Signed-off-by: Pierrick Bouvier --- docs/devel/build-environment.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/devel/build-environment.rst b/docs/devel/build-environment.rst index f133ef2e012..661f6ea8504 100644 --- a/docs/devel/build-environment.rst +++ b/docs/devel/build-environment.rst @@ -97,11 +97,11 @@ build QEMU in MSYS2 itself. :: -pacman -S wget +pacman -S wget base-devel git wget https://raw.githubusercontent.com/msys2/MINGW-packages/refs/heads/master/mingw-w64-qemu/PKGBUILD # Some packages may be missing for your environment, installation will still # be done though. -makepkg -s PKGBUILD || true +makepkg --syncdeps --nobuild PKGBUILD || true Build on windows-aarch64
Re: [PATCH 03/17] target/avr: Improve decode of LDS, STS
On 3/23/25 10:37, Richard Henderson wrote:
The comment about not being able to define a field with
zero bits is out of date since 94597b6146f3
("decodetree: Allow !function with no input bits").
This fixes the missing load of imm in the disassembler.
Cc: qemu-sta...@nongnu.org
Fixes: 9d8caa67a24 ("target/avr: Add support for disassembling via option '-d
in_asm'")
Signed-off-by: Richard Henderson
---
target/avr/translate.c | 2 --
target/avr/insn.decode | 7 ++-
2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/target/avr/translate.c b/target/avr/translate.c
index 4ab71d8138..e7f8ced9b3 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -1578,7 +1578,6 @@ static bool trans_LDS(DisasContext *ctx, arg_LDS *a)
TCGv Rd = cpu_r[a->rd];
TCGv addr = tcg_temp_new_i32();
TCGv H = cpu_rampD;
-a->imm = next_word(ctx);
tcg_gen_mov_tl(addr, H); /* addr = H:M:L */
tcg_gen_shli_tl(addr, addr, 16);
@@ -1783,7 +1782,6 @@ static bool trans_STS(DisasContext *ctx, arg_STS *a)
TCGv Rd = cpu_r[a->rd];
TCGv addr = tcg_temp_new_i32();
TCGv H = cpu_rampD;
-a->imm = next_word(ctx);
tcg_gen_mov_tl(addr, H); /* addr = H:M:L */
tcg_gen_shli_tl(addr, addr, 16);
diff --git a/target/avr/insn.decode b/target/avr/insn.decode
index 482c23ad0c..cc302249db 100644
--- a/target/avr/insn.decode
+++ b/target/avr/insn.decode
@@ -118,11 +118,8 @@ BRBC 01 ... ... @op_bit_imm
@io_rd_imm . .. . &rd_imm rd=%rd imm=%io_imm
@ldst_d .. . . .. . rd:5 . ... &rd_imm imm=%ldst_d_imm
-# The 16-bit immediate is completely in the next word.
-# Fields cannot be defined with no bits, so we cannot play
-# the same trick and append to a zero-bit value.
-# Defer reading the immediate until trans_{LDS,STS}.
-@ldst_s ... rd:5 imm=0
+%ldst_imm !function=next_word
+@ldst_s ... rd:5 imm=%ldst_imm
MOV 0010 11 . . @op_rd_rr
MOVW 0001 &rd_rr rd=%rd_d rr=%rr_d
Reviewed-by: Pierrick Bouvier
Re: [PATCH 04/17] target/avr: Remove OFFSET_CPU_REGISTERS
On 3/23/25 10:37, Richard Henderson wrote:
This define isn't really used.
Signed-off-by: Richard Henderson
---
target/avr/cpu.h| 2 --
target/avr/helper.c | 3 +--
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/target/avr/cpu.h b/target/avr/cpu.h
index 06f5ae4d1b..84a8f5cc8c 100644
--- a/target/avr/cpu.h
+++ b/target/avr/cpu.h
@@ -60,8 +60,6 @@
#define OFFSET_CODE 0x
/* CPU registers, IO registers, and SRAM */
#define OFFSET_DATA 0x0080
-/* CPU registers specifically, these are mapped at the start of data */
-#define OFFSET_CPU_REGISTERS OFFSET_DATA
/*
* IO registers, including status register, stack pointer, and memory
* mapped peripherals, mapped just after CPU registers
diff --git a/target/avr/helper.c b/target/avr/helper.c
index 3412312ad5..e5bf16c6b7 100644
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -340,8 +340,7 @@ void helper_fullwr(CPUAVRState *env, uint32_t data,
uint32_t addr)
env->fullacc = false;
/* Following logic assumes this: */
-assert(OFFSET_CPU_REGISTERS == OFFSET_DATA);
-assert(OFFSET_IO_REGISTERS == OFFSET_CPU_REGISTERS +
+assert(OFFSET_IO_REGISTERS == OFFSET_DATA +
NUMBER_OF_CPU_REGISTERS);
if (addr < NUMBER_OF_CPU_REGISTERS) {
Reviewed-by: Pierrick Bouvier
Re: [PATCH v4 0/2] hw/i386/amd_iommu: Add migration support
Hi, Any other concerns for this series? Thanks Suravee On 3/4/2025 9:17 PM, Suravee Suthikulpanit wrote: Currently, amd-iommu device does not support migration. This series addresses an issue due hidden AMDVI-PCI device enumeration. Then introduces migratable VMStateDescription, which saves necessary parameters for the device. Changes from v2: (https://lore.kernel.org/all/20250212054450.578449-1-suravee.suthikulpa...@amd.com) * Patch 1: Fix build error * Patch 2: Fix 32-bit build issue. Changes from v2: (https://lore.kernel.org/all/20250206051856.323651-1-suravee.suthikulpa...@amd.com) * Add patch 1/2 Suravee Suthikulpanit (2): hw/i386/amd_iommu: Isolate AMDVI-PCI from amd-iommu device to allow full control over the PCI device creation hw/i386/amd_iommu: Allow migration when explicitly create the AMDVI-PCI device hw/i386/acpi-build.c | 8 ++-- hw/i386/amd_iommu.c | 111 +-- hw/i386/amd_iommu.h | 5 +- 3 files changed, 92 insertions(+), 32 deletions(-)
[RFC PATCH] Hexagon (target/hexagon) analyze all reads before writes
I noticed that analyze_packet is marking the implicit pred reads after
marking all the writes. However, the semantics of the instrucion and
packet are to do all the reads, then do the operation, then do all the
writes.
Here is the old code
static void analyze_packet(DisasContext *ctx)
{
Packet *pkt = ctx->pkt;
ctx->read_after_write = false;
ctx->has_hvx_overlap = false;
for (int i = 0; i < pkt->num_insns; i++) {
Insn *insn = &pkt->insn[i];
ctx->insn = insn;
if (opcode_analyze[insn->opcode]) {
opcode_analyze[insn->opcode](ctx);
}
mark_implicit_reg_writes(ctx);
mark_implicit_pred_writes(ctx);
mark_implicit_pred_reads(ctx);
}
ctx->need_commit = need_commit(ctx);
}
Recall that opcode_analyze[insn->opcode](ctx) will mark all the
explicit reads then all the explicit writes.
To properly handle the semantics, we'll create two new functions
mark_implicit_reads
mark_implicit_writes
Then we change gen_analyze_funcs.py to add a call to the former
after all the explicit reads and a call to the latter after all
the explicit_writes.
The reason this is an RFC patch is I can't find any instructions
where this distinction makes a difference in ctx->need_commit which
determines if the packet commit can be short-circuited. However, this
could change in the future if the architecture introduces an
instruction with an implicit read of a register that is also written
(either implicit or explicit). Then, anlayze_packet would detect
a read-after-write, and the packet would not short-circuit. The
execution would be correct, but the performance would not be optimal.
Signed-off-by: Taylor Simpson
---
target/hexagon/translate.c | 18 +++---
target/hexagon/gen_analyze_funcs.py | 4
2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
index fe7858703c..5271c4e022 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -37,6 +37,10 @@
#include "exec/helper-info.c.inc"
#undef HELPER_H
+/* Forward declarations referenced in analyze_funcs_generated.c.inc */
+static void mark_implicit_reads(DisasContext *ctx);
+static void mark_implicit_writes(DisasContext *ctx);
+
#include "analyze_funcs_generated.c.inc"
typedef void (*AnalyzeInsn)(DisasContext *ctx);
@@ -378,6 +382,17 @@ static void mark_implicit_pred_reads(DisasContext *ctx)
mark_implicit_pred_read(ctx, A_IMPLICIT_READS_P3, 3);
}
+static void mark_implicit_reads(DisasContext *ctx)
+{
+mark_implicit_pred_reads(ctx);
+}
+
+static void mark_implicit_writes(DisasContext *ctx)
+{
+mark_implicit_reg_writes(ctx);
+mark_implicit_pred_writes(ctx);
+}
+
static void analyze_packet(DisasContext *ctx)
{
Packet *pkt = ctx->pkt;
@@ -389,9 +404,6 @@ static void analyze_packet(DisasContext *ctx)
if (opcode_analyze[insn->opcode]) {
opcode_analyze[insn->opcode](ctx);
}
-mark_implicit_reg_writes(ctx);
-mark_implicit_pred_writes(ctx);
-mark_implicit_pred_reads(ctx);
}
ctx->need_commit = need_commit(ctx);
diff --git a/target/hexagon/gen_analyze_funcs.py
b/target/hexagon/gen_analyze_funcs.py
index 3ac7cc2cfe..fdefd5b4b3 100755
--- a/target/hexagon/gen_analyze_funcs.py
+++ b/target/hexagon/gen_analyze_funcs.py
@@ -67,6 +67,8 @@ def gen_analyze_func(f, tag, regs, imms):
if reg.is_read():
reg.analyze_read(f, regno)
+f.write("mark_implicit_reads(ctx);\n")
+
## Analyze the register writes
for regno, register in enumerate(regs):
reg_type, reg_id = register
@@ -74,6 +76,8 @@ def gen_analyze_func(f, tag, regs, imms):
if reg.is_written():
reg.analyze_write(f, tag, regno)
+f.write("mark_implicit_writes(ctx);\n")
+
f.write("}\n\n")
--
2.43.0
Re: [PULL 0/3] aspeed queue
24.03.2025 23:46, Cédric Le Goater wrote:
Is there anything in there worth to pick up for stable series?
you are fast !
I was just about to send final announcements for a bunch of next
stable releases, and noticed another pull request has been merged.. :)
- "aspeed: Fix maximum number of spi controller" is QEMU 10.0 material.
- "hw/intc/aspeed: Fix IRQ handler mask check" was merged in QEMU 9.1
- "hw/misc/aspeed_hace: Fix buffer overflow in has_padding function"
was merged in QEMU 7.1
The last 2 deserve to be backported IMO. They will need some massaging.
The "buffer overflow" fix seems to be okay for 9.2, 8.2 and 7.2.
The "IRQ handler mask check" seems to be this (for 9.2). Does it look sane?
Author: Steven Lee
Date: Thu Mar 20 17:25:43 2025 +0800
hw/intc/aspeed: Fix IRQ handler mask check
Updated the IRQ handler mask check to AND with select variable.
This ensures that the interrupt service routine is correctly triggered
for the interrupts within the same irq group.
For example, both `eth0` and the debug UART are handled in `GICINT132`.
Without this fix, the debug console may hang if the `eth0` ISR is not
handled.
Signed-off-by: Steven Lee
Change-Id: Ic3609eb72218dfd68be6057d78b8953b18828709
Reviewed-by: Cédric Le Goater
Fixes: d831c5fd8682 ("aspeed/intc: Add AST2700 support")
Link:
https://lore.kernel.org/qemu-devel/20250320092543.4040672-2-steven_...@aspeedtech.com
Signed-off-by: Cédric Le Goater
(cherry picked from commit 7b8cbe5162e69ad629c5326bf3c158b81857955d)
(Mjt: update for before v9.2.0-2466-g5824e8bf6beb
"hw/intc/aspeed: Introduce IRQ handler function to reduce code
duplication")
Signed-off-by: Michael Tokarev
diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c
index 126b711b94..495fd2bdfa 100644
--- a/hw/intc/aspeed_intc.c
+++ b/hw/intc/aspeed_intc.c
@@ -92,7 +92,7 @@ static void aspeed_intc_set_irq(void *opaque, int irq, int
level)
trace_aspeed_intc_select(select);
-if (s->mask[irq] || s->regs[status_addr]) {
+if ((s->mask[irq] & select) || (s->regs[status_addr] & select)) {
/*
* a. mask is not 0 means in ISR mode
* sources interrupt routine are executing.
Re: [PATCH 13/17] target/avr: Handle offset_io in avr_cpu_realizefn
On 3/23/25 10:37, Richard Henderson wrote: Signed-off-by: Richard Henderson --- target/avr/cpu.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/target/avr/cpu.c b/target/avr/cpu.c index e4011004b4..538fcbc215 100644 --- a/target/avr/cpu.c +++ b/target/avr/cpu.c @@ -161,12 +161,14 @@ static void avr_cpu_realizefn(DeviceState *dev, Error **errp) memory_region_init_io(&cpu->cpu_reg1, OBJECT(cpu), &avr_cpu_reg1, env, "avr-cpu-reg1", 32); memory_region_add_subregion(get_system_memory(), -OFFSET_DATA, &cpu->cpu_reg1); +OFFSET_DATA + cpu->offset_io, +&cpu->cpu_reg1); memory_region_init_io(&cpu->cpu_reg2, OBJECT(cpu), &avr_cpu_reg2, env, "avr-cpu-reg2", 8); memory_region_add_subregion(get_system_memory(), -OFFSET_DATA + 0x58, &cpu->cpu_reg2); +OFFSET_DATA + cpu->offset_io + 0x58, +&cpu->cpu_reg2); } static void avr_cpu_set_int(void *opaque, int irq, int level) Reviewed-by: Pierrick Bouvier
Re: [PATCH 01/17] hw/core/cpu: Use size_t for memory_rw_debug len argument
On 3/23/25 10:37, Richard Henderson wrote:
Match the prototype of cpu_memory_rw_debug().
Signed-off-by: Richard Henderson
---
include/hw/core/cpu.h | 2 +-
target/sparc/cpu.h| 2 +-
target/sparc/mmu_helper.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 5d11d26556..abd8764e83 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -154,7 +154,7 @@ struct CPUClass {
int (*mmu_index)(CPUState *cpu, bool ifetch);
int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
- uint8_t *buf, int len, bool is_write);
+ uint8_t *buf, size_t len, bool is_write);
void (*dump_state)(CPUState *cpu, FILE *, int flags);
void (*query_cpu_fast)(CPUState *cpu, CpuInfoFast *value);
int64_t (*get_arch_id)(CPUState *cpu);
diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
index 462bcb6c0e..68f8c21e7c 100644
--- a/target/sparc/cpu.h
+++ b/target/sparc/cpu.h
@@ -604,7 +604,7 @@ void dump_mmu(CPUSPARCState *env);
#if !defined(TARGET_SPARC64) && !defined(CONFIG_USER_ONLY)
int sparc_cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
- uint8_t *buf, int len, bool is_write);
+ uint8_t *buf, size_t len, bool is_write);
#endif
/* translate.c */
diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
index 7548d01777..3821cd91ec 100644
--- a/target/sparc/mmu_helper.c
+++ b/target/sparc/mmu_helper.c
@@ -389,7 +389,7 @@ void dump_mmu(CPUSPARCState *env)
* that the sparc ABI is followed.
*/
int sparc_cpu_memory_rw_debug(CPUState *cs, vaddr address,
- uint8_t *buf, int len, bool is_write)
+ uint8_t *buf, size_t len, bool is_write)
{
CPUSPARCState *env = cpu_env(cs);
target_ulong addr = address;
Reviewed-by: Pierrick Bouvier
Re: [PATCH 2/6] ui/egl: require EGL_EXT_image_dma_buf_import_modifiers
On Mon, Mar 24, 2025 at 9:45 PM Marc-André Lureau
wrote:
>
> Hi
>
> On Mon, Mar 24, 2025 at 5:27 PM Qiang Yu wrote:
> >
> > On Mon, Mar 24, 2025 at 6:09 PM Marc-André Lureau
> > wrote:
> > >
> > > Hi
> > >
> > > On Mon, Mar 24, 2025 at 12:19 PM wrote:
> > > >
> > > > From: Qiang Yu
> > > >
> > > > It's used already, just check it explicitly.
> > > >
> > > > Signed-off-by: Qiang Yu
> > > > ---
> > > > ui/egl-helpers.c | 10 ++
> > > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
> > > > index 72a1405782..45b1b0b700 100644
> > > > --- a/ui/egl-helpers.c
> > > > +++ b/ui/egl-helpers.c
> > > > @@ -257,6 +257,11 @@ int egl_rendernode_init(const char *rendernode,
> > > > DisplayGLMode mode)
> > > > error_report("egl: EGL_MESA_image_dma_buf_export not
> > > > supported");
> > > > goto err;
> > > > }
> > > > +if (!epoxy_has_egl_extension(qemu_egl_display,
> > > > +
> > > > "EGL_EXT_image_dma_buf_import_modifiers")) {
> > > > +error_report("egl: EGL_EXT_image_dma_buf_import_modifiers not
> > > > supported");
> > > > +goto err;
> > > > +}
> > > >
> > > > qemu_egl_rn_ctx = qemu_egl_init_ctx();
> > > > if (!qemu_egl_rn_ctx) {
> > > > @@ -308,7 +313,7 @@ void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf)
> > > > EGLImageKHR image = EGL_NO_IMAGE_KHR;
> > > > EGLint attrs[64];
> > > > int i = 0;
> > > > -uint64_t modifier;
> > > > +uint64_t modifier = qemu_dmabuf_get_modifier(dmabuf);
> > > > uint32_t texture = qemu_dmabuf_get_texture(dmabuf);
> > > >
> > > > if (texture != 0) {
> > > > @@ -328,15 +333,12 @@ void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf)
> > > > attrs[i++] = qemu_dmabuf_get_stride(dmabuf)[0];
> > > > attrs[i++] = EGL_DMA_BUF_PLANE0_OFFSET_EXT;
> > > > attrs[i++] = 0;
> > > > -#ifdef EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT
> > >
> > > We should check for that define during meson.build when gbm.found(),
> > > to avoid potential later build errors.
> > >
> > This macro should be in EGL header for many years as this is a 2015 EGL
> > extension. Check the macro in meson.build is not necessary now like you
> > won't check all other EGL macros like EGL_DMA_BUF_PLANE0_OFFSET_EXT.
>
> Imho we should still check for those macros, as they are extensions to egl.
>
Macros like EGL_DMA_BUF_PLANE0_OFFSET_EXT are also from EGL extension
but we don't check them. EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT makes
no difference. And we do need this macro to be always present to make
sure exporter
and importer of the texture to work correctly.
>
> --
> Marc-André Lureau
[PATCH-for-10.1 0/3] migration/cpu: Remove qemu_{get, put}_[s]betl[s] macros
The following macros:
- qemu_put_betl()
- qemu_get_betl()
- qemu_put_betls()
- qemu_get_betls()
- qemu_put_sbetl()
- qemu_get_sbetl()
- qemu_put_sbetls()
- qemu_get_sbetls()
are used twice. Expand tl -> 32/64 and remove them.
Philippe Mathieu-Daudé (3):
target/mips: Inline qemu_get_betls() and qemu_put_betls()
target/sparc: Inline qemu_get_betl() and qemu_put_betl()
migration/cpu: Remove qemu_{get,put}_[s]betl[s] macros
include/migration/cpu.h | 18 --
target/mips/system/machine.c | 12 ++--
target/sparc/machine.c | 14 --
3 files changed, 22 insertions(+), 22 deletions(-)
--
2.47.1
Re: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a pxb-pcie bus
On Mon, Mar 24, 2025 at 03:56:12PM +0100, Eric Auger wrote:
>
>
> On 3/21/25 1:59 AM, Donald Dutile wrote:
> >
> >
> > On 3/19/25 2:21 PM, Eric Auger wrote:
> >> Hi Don,
> >>
> >>
> >> On 3/19/25 5:21 PM, Donald Dutile wrote:
> >>>
> >>>
> >>> On 3/19/25 5:26 AM, Shameerali Kolothum Thodi wrote:
> Hi Don,
>
> >>> Hey!
> >>>
> > -Original Message-
> > From: Donald Dutile
> > Sent: Tuesday, March 18, 2025 10:12 PM
> > To: Shameerali Kolothum Thodi
> > ; qemu-...@nongnu.org;
> > qemu-devel@nongnu.org
> > Cc: eric.au...@redhat.com; peter.mayd...@linaro.org; j...@nvidia.com;
> > nicol...@nvidia.com; berra...@redhat.com; nath...@nvidia.com;
> > mo...@nvidia.com; smost...@google.com; Linuxarm
> > ; Wangzhou (B) ;
> > jiangkunkun ; Jonathan Cameron
> > ; zhangfei@linaro.org
> > Subject: Re: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a
> > pxb-
> > pcie bus
> >
> > Shameer,
> >
> > Hi!
> >
> > On 3/11/25 10:10 AM, Shameer Kolothum wrote:
> >> User must associate a pxb-pcie root bus to smmuv3-accel
> >> and that is set as the primary-bus for the smmu dev.
> >>
> >> Signed-off-by: Shameer Kolothum
> >
> >> ---
> >> hw/arm/smmuv3-accel.c | 19 +++
> >> 1 file changed, 19 insertions(+)
> >>
> >> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
> >> index c327661636..1471b65374 100644
> >> --- a/hw/arm/smmuv3-accel.c
> >> +++ b/hw/arm/smmuv3-accel.c
> >> @@ -9,6 +9,21 @@
> >> #include "qemu/osdep.h"
> >>
> >> #include "hw/arm/smmuv3-accel.h"
> >> +#include "hw/pci/pci_bridge.h"
> >> +
> >> +static int smmuv3_accel_pxb_pcie_bus(Object *obj, void *opaque)
> >> +{
> >> + DeviceState *d = opaque;
> >> +
> >> + if (object_dynamic_cast(obj, "pxb-pcie-bus")) {
> >> + PCIBus *bus = PCI_HOST_BRIDGE(obj->parent)->bus;
> >> + if (d->parent_bus && !strcmp(bus->qbus.name, d->parent_bus-
> >> name)) {
> >> + object_property_set_link(OBJECT(d), "primary-bus",
> >> OBJECT(bus),
> >> + &error_abort);
> >> + }
> >> + }
> >> + return 0;
> >> +}
> >>
> >> static void smmu_accel_realize(DeviceState *d, Error **errp)
> >> {
> >> @@ -17,6 +32,9 @@ static void smmu_accel_realize(DeviceState *d,
> >> Error
> > **errp)
> >> SysBusDevice *dev = SYS_BUS_DEVICE(d);
> >> Error *local_err = NULL;
> >>
> >> + object_child_foreach_recursive(object_get_root(),
> >> + smmuv3_accel_pxb_pcie_bus, d);
> >> +
> >> object_property_set_bool(OBJECT(dev), "accel", true,
> >> &error_abort);
> >> c->parent_realize(d, &local_err);
> >> if (local_err) {
> >> @@ -33,6 +51,7 @@ static void smmuv3_accel_class_init(ObjectClass
> > *klass, void *data)
> >> device_class_set_parent_realize(dc, smmu_accel_realize,
> >> &c->parent_realize);
> >> dc->hotpluggable = false;
> >> + dc->bus_type = TYPE_PCIE_BUS;
> >> }
> >>
> >> static const TypeInfo smmuv3_accel_type_info = {
> >
> > I am not seeing the need for a pxb-pcie bus(switch) introduced for
> > each
> > 'accel'.
> > Isn't the IORT able to define different SMMUs for different RIDs?
> > if so,
> > itsn't that sufficient
> > to associate (define) an SMMU<->RID association without introducing a
> > pxb-pcie?
> > and again, I'm not sure how that improves/enables the device<->SMMU
> > associativity?
>
> Thanks for taking a look at the series. As discussed elsewhere in
> this thread(with
> Eric), normally in physical world (or atleast in the most common
> cases) SMMUv3
> is attached to PCIe Root Complex and if you take a look at the IORT
> spec, it describes
> association of ID mappings between a RC node and SMMUV3 node.
>
> And if my understanding is correct, in Qemu, only pxb-pcie allows you
> to add
> extra root complexes even though it is still plugged to
> parent(pcie.0). ie, for all
> devices downstream it acts as a root complex but still plugged into a
> parent pcie.0.
> This allows us to add/describe multiple "smmuv3-accel" each
> associated with a RC.
>
> >>> I find the qemu statements a bit unclear here as well.
> >>> I looked at the hot plug statement(s) in docs/pcie.txt, as I figured
> >>> that's where dynamic
> >>> IORT changes would be needed as well. There, it says you can hot-add
> >>> PCIe devices to RPs,
> >>> one has to define/add RP's to the machine model for that plug-in.
> >>>
> >>> Using libvirt, it could auto-add the needed RPs to do dynmaic smmuv3
> >>>
Re: [PATCH v6] hw/misc/vmfwupdate: Introduce hypervisor fw-cfg interface support
On 24.03.25 16:48, Gerd Hoffman wrote: On Mon, Mar 24, 2025 at 04:42:28PM +0530, Ani Sinha wrote: On Mon, Mar 24, 2025 at 1:13 PM Gerd Hoffman wrote: Hi, Going ship the distro kernel as igvm image would work too. Will simplify the measurement pre-calculation. Also there is no need to pass around any parameters, everything (how the firmware finds the UKI etc) can be arranged at igvm build time then. Disadvantage: This introduces a completely new boot workflow. Will probably need a new set of cloud images exclusively for the BYOF case. What does all this mean for the hypervisor interface ? That means we'll go scratch the region list idea and depend on igvm instead. Which means we are back to the single firmware image. So in this model, how are we passing the hashes of kernel, initrd and cmdline? Are they going to be part of the firmware image as was previously thought? Either scratch the idea of using hashes to verify kernel + initrd + cmdline and use a secure boot signed UKI instead, or use IGVM firmware images and add the kernel hashes page to the igvm. It's a nice idea to have a firmware IGVM file that contains a signature, so you can for example have a RHEL provided IGVM that only runs UKIs that are signed by Red Hat. Unfortunately, it does not address some of the other interesting use cases: - Attestable Bootable Containers. In that case, the command line contains a hash of the target fs-verity protected partition. Red Hat can not be the entity signing that UKI. It must be the user. - Add-ons. How do you provide a "debug add-on" and prevent it to gain any access to confidential data? So we need some equivalent of a hash page. That can absolutely integrate more deeply into UEFI and be actual PE hashes rather than the icky thing the OVMF code does today. But we need to support a way to embed the hash in the ukify.py generated IGVM on the fly. With add-ons, maybe even multiple ones. Alex
Re: [PATCH-for-10.1 5/6] target/sparc: Register CPUClass:list_cpus
On 24/3/25 10:30, Thomas Huth wrote: On 23/03/2025 23.40, Philippe Mathieu-Daudé wrote: Register sparc_cpu_list() as CPUClass:list_cpus callback and remove the cpu_list definition. Copy-n-paste error in both, subject and patch description: This should be about s390x, not sparc. Oh oops. diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 5b7992deda6..1aac967a0ce 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -902,7 +902,6 @@ static inline uint8_t s390_cpu_get_state(S390CPU *cpu) /* cpu_models.c */ void s390_cpu_list(void); Don't you want to remove the prototype here, too? (and make the function static in the .c file) s390_set_qemu_cpu_model is defined in cpu_models.c, while CPUClass in cpu.c. Thomas
Re: [PATCH v2 01/11] include/exec: fix assert in size_memop
On 24/3/25 11:21, Alex Bennée wrote: We can handle larger sized memops now, expand the range of the assert. Fixes: 4b473e0c60 (tcg: Expand MO_SIZE to 3 bits) Signed-off-by: Alex Bennée --- v2 - instead of 128 use 1 << MO_SIZE for future proofing --- include/exec/memop.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH] vfio: Open code vfio_migration_set_error()
On 24/03/2025 17:25, Cédric Le Goater wrote:
External email: Use caution opening links or attachments
On 3/24/25 16:14, Avihai Horon wrote:
On 24/03/2025 14:33, Cédric Le Goater wrote:
External email: Use caution opening links or attachments
VFIO uses migration_file_set_error() in a couple of places where an
'Error **' parameter is not provided. In MemoryListener handlers :
vfio_listener_region_add
Nit: migration_file_set_error() is not used in vfio_listener_region_add.
vfio_listener_log_global_stop
vfio_listener_log_sync
and in callback routines for IOMMU notifiers :
vfio_iommu_map_notify
vfio_iommu_map_dirty_notify
Hopefully, one day, we will be able to extend these callbacks with an
'Error **' parameter and avoid setting the global migration error.
Until then, it seems sensible to clearly identify the use cases, which
are limited, and open code vfio_migration_set_error(). One other
benefit is an improved error reporting when migration is running.
While at it, slightly modify error reporting to only report errors
when migration is not active and not always as is currently done.
Cc: Prasad Pandit
Signed-off-by: Cédric Le Goater
---
hw/vfio/common.c | 60
+---
1 file changed, 36 insertions(+), 24 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index
1a0d9290f88c9774a98f65087a36b86922b21a73..a591ce5b97ff41cdc8249e9eeafc8dc347d45fac
100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -149,13 +149,6 @@ bool vfio_viommu_preset(VFIODevice *vbasedev)
return vbasedev->bcontainer->space->as != &address_space_memory;
}
-static void vfio_set_migration_error(int ret)
-{
- if (migration_is_running()) {
- migration_file_set_error(ret, NULL);
- }
-}
Wouldn't it be better to extend vfio_set_migration_error() to take
also Error* instead of duplicating code?
We can rename it to vfio_set_error() if it's not solely related to
vfio migration anymore.
IMO, the vfio_set_migration_error() wrapper shadows the use of the
migration routines and their context. I prefer to be explicit about
it, open the code and work on removal. It is possible to add an
'Error **' parameter to log_global_stop handlers and to the IOMMU
notifiers. It just takes time.
I see, fair enough.
Reviewed-by: Avihai Horon
Thanks,
C.
Thanks.
-
bool vfio_device_state_is_running(VFIODevice *vbasedev)
{
VFIOMigration *migration = vbasedev->migration;
@@ -291,9 +284,14 @@ static void vfio_iommu_map_notify(IOMMUNotifier
*n, IOMMUTLBEntry *iotlb)
iova, iova + iotlb->addr_mask);
if (iotlb->target_as != &address_space_memory) {
- error_report("Wrong target AS \"%s\", only system memory is
allowed",
- iotlb->target_as->name ?
iotlb->target_as->name : "none");
- vfio_set_migration_error(-EINVAL);
+ error_setg(&local_err,
+ "Wrong target AS \"%s\", only system memory is
allowed",
+ iotlb->target_as->name ? iotlb->target_as->name
: "none");
+ if (migration_is_running()) {
+ migration_file_set_error(-EINVAL, local_err);
+ } else {
+ error_report_err(local_err);
+ }
return;
}
@@ -326,11 +324,16 @@ static void
vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
ret = vfio_container_dma_unmap(bcontainer, iova,
iotlb->addr_mask + 1, iotlb);
if (ret) {
- error_report("vfio_container_dma_unmap(%p,
0x%"HWADDR_PRIx", "
- "0x%"HWADDR_PRIx") = %d (%s)",
- bcontainer, iova,
- iotlb->addr_mask + 1, ret, strerror(-ret));
- vfio_set_migration_error(ret);
+ error_setg(&local_err,
+ "vfio_container_dma_unmap(%p,
0x%"HWADDR_PRIx", "
+ "0x%"HWADDR_PRIx") = %d (%s)",
+ bcontainer, iova,
+ iotlb->addr_mask + 1, ret, strerror(-ret));
+ if (migration_is_running()) {
+ migration_file_set_error(ret, local_err);
+ } else {
+ error_report_err(local_err);
+ }
}
}
out:
@@ -1112,8 +1115,11 @@ static void
vfio_listener_log_global_stop(MemoryListener *listener)
if (ret) {
error_prepend(&local_err,
"vfio: Could not stop dirty page tracking - ");
- error_report_err(local_err);
- vfio_set_migration_error(ret);
+ if (migration_is_running()) {
+ migration_file_set_error(ret, local_err);
+ } else {
+ error_report_err(local_err);
+ }
}
}
@@ -1229,14 +1235,14 @@ static void
vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
if (
Re: [PATCH 2/6] ui/egl: require EGL_EXT_image_dma_buf_import_modifiers
Hi
On Mon, Mar 24, 2025 at 5:27 PM Qiang Yu wrote:
>
> On Mon, Mar 24, 2025 at 6:09 PM Marc-André Lureau
> wrote:
> >
> > Hi
> >
> > On Mon, Mar 24, 2025 at 12:19 PM wrote:
> > >
> > > From: Qiang Yu
> > >
> > > It's used already, just check it explicitly.
> > >
> > > Signed-off-by: Qiang Yu
> > > ---
> > > ui/egl-helpers.c | 10 ++
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
> > > index 72a1405782..45b1b0b700 100644
> > > --- a/ui/egl-helpers.c
> > > +++ b/ui/egl-helpers.c
> > > @@ -257,6 +257,11 @@ int egl_rendernode_init(const char *rendernode,
> > > DisplayGLMode mode)
> > > error_report("egl: EGL_MESA_image_dma_buf_export not supported");
> > > goto err;
> > > }
> > > +if (!epoxy_has_egl_extension(qemu_egl_display,
> > > +
> > > "EGL_EXT_image_dma_buf_import_modifiers")) {
> > > +error_report("egl: EGL_EXT_image_dma_buf_import_modifiers not
> > > supported");
> > > +goto err;
> > > +}
> > >
> > > qemu_egl_rn_ctx = qemu_egl_init_ctx();
> > > if (!qemu_egl_rn_ctx) {
> > > @@ -308,7 +313,7 @@ void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf)
> > > EGLImageKHR image = EGL_NO_IMAGE_KHR;
> > > EGLint attrs[64];
> > > int i = 0;
> > > -uint64_t modifier;
> > > +uint64_t modifier = qemu_dmabuf_get_modifier(dmabuf);
> > > uint32_t texture = qemu_dmabuf_get_texture(dmabuf);
> > >
> > > if (texture != 0) {
> > > @@ -328,15 +333,12 @@ void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf)
> > > attrs[i++] = qemu_dmabuf_get_stride(dmabuf)[0];
> > > attrs[i++] = EGL_DMA_BUF_PLANE0_OFFSET_EXT;
> > > attrs[i++] = 0;
> > > -#ifdef EGL_DMA_BUF_PLANE0_MODIFIER_LO_EXT
> >
> > We should check for that define during meson.build when gbm.found(),
> > to avoid potential later build errors.
> >
> This macro should be in EGL header for many years as this is a 2015 EGL
> extension. Check the macro in meson.build is not necessary now like you
> won't check all other EGL macros like EGL_DMA_BUF_PLANE0_OFFSET_EXT.
Imho we should still check for those macros, as they are extensions to egl.
--
Marc-André Lureau
Re: [PATCH 6/6] ui/spice: support multi plane dmabuf scanout
On Mon, Mar 24, 2025 at 5:35 PM Qiang Yu wrote:
>
> On Mon, Mar 24, 2025 at 5:30 PM Marc-André Lureau
> wrote:
> >
> > Hi
> >
> > On Mon, Mar 24, 2025 at 12:20 PM wrote:
> > >
> > > From: Qiang Yu
> > >
> > > Signed-off-by: Qiang Yu
> > > ---
> > > meson.build| 2 +-
> > > ui/spice-display.c | 65 +++---
> > > 2 files changed, 34 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/meson.build b/meson.build
> > > index 9d9c11731f..b87704a83b 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -1329,7 +1329,7 @@ if get_option('spice') \
> > > .require(pixman.found(),
> > >error_message: 'cannot enable SPICE if pixman is
> > > not available') \
> > > .allowed()
> > > - spice = dependency('spice-server', version: '>=0.14.0',
> > > + spice = dependency('spice-server', version: '>=0.14.3',
> >
> > I am okay with bumping dependency requirements, but the nicer
> > alternative would be to allow the current version and check the
> > existence of the new API/function instead.
> >
> I'm not familiar with how qemu handle new API dependency, but isn't it much
> convenient to just bump lib version instead of a macro all around? And I can't
> see similar macro in meson.build
Yes it is generally simpler to bump requirements, but as Daniel
hinted, we have policies about supporting older environments.
For your series, I think we could simply have:
if spice.found()
config_host_data.set('HAVE_SPICE_QXL_GL_SCANOUT2',
cc.has_function('spice_qxl_gl_scanout2', dependencies: spice))
endif
>
> >
> > > required: get_option('spice'),
> > > method: 'pkg-config')
> > > endif
> > > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > > index b7016ece6a..46b6d5dfc9 100644
> > > --- a/ui/spice-display.c
> > > +++ b/ui/spice-display.c
> > > @@ -28,6 +28,8 @@
> > >
> > > #include "ui/spice-display.h"
> > >
> > > +#include "standard-headers/drm/drm_fourcc.h"
> > > +
> > > bool spice_opengl;
> > >
> > > int qemu_spice_rect_is_empty(const QXLRect* r)
> > > @@ -884,16 +886,11 @@ static void spice_gl_switch(DisplayChangeListener
> > > *dcl,
> > > if (ssd->ds) {
> > > uint32_t offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES];
> > > int fd[DMABUF_MAX_PLANES], num_planes, fourcc;
> > > +uint64_t modifier;
> > >
> > > surface_gl_create_texture(ssd->gls, ssd->ds);
> > > if (!egl_dmabuf_export_texture(ssd->ds->texture, fd, (EGLint
> > > *)offset,
> > > - (EGLint *)stride, &fourcc,
> > > &num_planes, NULL)) {
> > > -surface_gl_destroy_texture(ssd->gls, ssd->ds);
> > > -return;
> > > -}
> > > -
> > > -if (num_planes > 1) {
> > > -fprintf(stderr, "%s: does not support multi-plane
> > > texture\n", __func__);
> > > + (EGLint *)stride, &fourcc,
> > > &num_planes, &modifier)) {
> > > surface_gl_destroy_texture(ssd->gls, ssd->ds);
> > > return;
> > > }
> > > @@ -904,10 +901,11 @@ static void spice_gl_switch(DisplayChangeListener
> > > *dcl,
> > > fourcc);
> > >
> > > /* note: spice server will close the fd */
> > > -spice_qxl_gl_scanout(&ssd->qxl, fd[0],
> > > - surface_width(ssd->ds),
> > > - surface_height(ssd->ds),
> > > - stride[0], fourcc, false);
> > > +spice_qxl_gl_scanout2(&ssd->qxl, fd,
> > > + surface_width(ssd->ds),
> > > + surface_height(ssd->ds),
> > > + offset, stride, num_planes,
> > > + fourcc, modifier, false);
> > > ssd->have_surface = true;
> > > ssd->have_scanout = false;
> > >
> > > @@ -930,7 +928,8 @@ static void
> > > qemu_spice_gl_scanout_disable(DisplayChangeListener *dcl)
> > > SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> > >
> > > trace_qemu_spice_gl_scanout_disable(ssd->qxl.id);
> > > -spice_qxl_gl_scanout(&ssd->qxl, -1, 0, 0, 0, 0, false);
> > > +spice_qxl_gl_scanout2(&ssd->qxl, NULL, 0, 0, NULL, NULL, 0,
> > > DRM_FORMAT_INVALID,
> > > + DRM_FORMAT_MOD_INVALID, false);
> > > qemu_spice_gl_monitor_config(ssd, 0, 0, 0, 0);
> > > ssd->have_surface = false;
> > > ssd->have_scanout = false;
> > > @@ -948,22 +947,21 @@ static void
> > > qemu_spice_gl_scanout_texture(DisplayChangeListener *dcl,
> > > SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
> > > EGLint offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES], fourcc
> > > = 0;
> > > int fd[DMABUF_MAX_PLANES], num_planes;
> > > +uint64_t modifier;
> > >
> > > assert(tex_id);
Re: [PATCH] tests/functional: Add missing require_netdev('user') statements
On Mon, Mar 24, 2025 at 01:34:50PM +0100, Thomas Huth wrote:
> From: Thomas Huth
>
> A bunch of tests are using "-netdev user" but fail to check
> for the availability of SLIRP in the binary, so these tests
> fail if QEMU has been configured with "--disable-slirp"
> (most of the tests are disabled by default with a decorator,
> that's likely why nobody noticed this problem yet). Add the
> missing self.require_netdev('user') statements to skip the
> tests if SLIRP is not available.
>
> Signed-off-by: Thomas Huth
> ---
> tests/functional/test_aarch64_rme_sbsaref.py | 1 +
> tests/functional/test_aarch64_rme_virt.py| 4 +++-
> tests/functional/test_arm_bpim2u.py | 2 ++
> tests/functional/test_arm_cubieboard.py | 2 ++
> tests/functional/test_arm_orangepi.py| 4
> tests/functional/test_ppc64_hv.py| 3 +++
> tests/functional/test_x86_64_kvm_xen.py | 1 +
> 7 files changed, 16 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 3/3] vhost-user: return failure if backend crash when live migration
On Thu, Mar 20, 2025 at 08:21:30PM +0800, Haoqian He wrote:
2025年3月19日 23:20，Stefano Garzarella 写道：
On Fri, Mar 14, 2025 at 06:15:34AM -0400, Haoqian He wrote:
Live migration should be terminated if the backend crashes before
the migration completes.
Since the vhost device will be stopped when VM is stopped before
the end of the live migration, current implementation if vhost
backend died, vhost device's set_status() will not return failure,
live migration won't perceive the disconnection between qemu and
vhost backend, inflight io would be submitted in migration target
host, leading to IO error.
To fix this issue:
1. Add set_status_ext() which has return value for
VirtioDeviceClass and vhost-user-blk/scsi use the _ext version.
2. In set_status_ext(), return failure if the flag `connected`
is false or vhost_dev_stop return failure, which means qemu lost
connection with backend.
Hence migration_completion() will process failure, terminate
migration and restore VM.
Signed-off-by: Haoqian He
---
hw/block/vhost-user-blk.c | 29 +++
hw/scsi/vhost-scsi-common.c | 13 ++--
hw/scsi/vhost-user-scsi.c | 20 ++
hw/virtio/virtio.c| 20 +-
include/hw/virtio/vhost-scsi-common.h | 2 +-
include/hw/virtio/virtio.h| 1 +
6 files changed, 52 insertions(+), 33 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index ae42327cf8..4865786c54 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -204,7 +204,7 @@ err_host_notifiers:
return ret;
}
-static void vhost_user_blk_stop(VirtIODevice *vdev)
+static int vhost_user_blk_stop(VirtIODevice *vdev)
{
VHostUserBlk *s = VHOST_USER_BLK(vdev);
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
@@ -212,26 +212,26 @@ static void vhost_user_blk_stop(VirtIODevice *vdev)
int ret;
if (!s->started_vu) {
-return;
+return 0;
}
s->started_vu = false;
if (!k->set_guest_notifiers) {
-return;
+return 0;
}
-vhost_dev_stop(&s->dev, vdev, true);
+ret = vhost_dev_stop(&s->dev, vdev, true);
-ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
-if (ret < 0) {
+if (k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false) < 0) {
error_report("vhost guest notifier cleanup failed: %d", ret);
-return;
+return -1;
}
vhost_dev_disable_notifiers(&s->dev, vdev);
+return ret;
}
-static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
+static int vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
{
VHostUserBlk *s = VHOST_USER_BLK(vdev);
bool should_start = virtio_device_should_start(vdev, status);
@@ -239,11 +239,11 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev,
uint8_t status)
int ret;
if (!s->connected) {
-return;
+return -1;
}
if (vhost_dev_is_started(&s->dev) == should_start) {
-return;
+return 0;
}
if (should_start) {
@@ -253,9 +253,12 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev,
uint8_t status)
qemu_chr_fe_disconnect(&s->chardev);
}
} else {
-vhost_user_blk_stop(vdev);
+ret = vhost_user_blk_stop(vdev);
+if (ret < 0) {
+return ret;
+}
}
-
+return 0;
}
static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev,
@@ -597,7 +600,7 @@ static void vhost_user_blk_class_init(ObjectClass *klass,
void *data)
vdc->get_config = vhost_user_blk_update_config;
vdc->set_config = vhost_user_blk_set_config;
vdc->get_features = vhost_user_blk_get_features;
-vdc->set_status = vhost_user_blk_set_status;
+vdc->set_status_ext = vhost_user_blk_set_status;
vdc->reset = vhost_user_blk_reset;
vdc->get_vhost = vhost_user_blk_get_vhost;
}
diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
index 4c8637045d..43525ba46d 100644
--- a/hw/scsi/vhost-scsi-common.c
+++ b/hw/scsi/vhost-scsi-common.c
@@ -101,24 +101,25 @@ err_host_notifiers:
return ret;
}
-void vhost_scsi_common_stop(VHostSCSICommon *vsc)
+int vhost_scsi_common_stop(VHostSCSICommon *vsc)
{
VirtIODevice *vdev = VIRTIO_DEVICE(vsc);
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
int ret = 0;
-vhost_dev_stop(&vsc->dev, vdev, true);
+ret = vhost_dev_stop(&vsc->dev, vdev, true);
if (k->set_guest_notifiers) {
-ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
-if (ret < 0) {
-error_report("vhost guest notifier cleanup failed: %d", ret);
+int r = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
+if (r < 0) {
+error_report("vhost guest notifier cleanup failed: %d", ret);
The variable `ret` in the error_report() seems wrong.
Ohh, thanks, I will fix it late
Re: [PATCH] vfio: Open code vfio_migration_set_error()
On 3/24/25 16:14, Avihai Horon wrote:
On 24/03/2025 14:33, Cédric Le Goater wrote:
External email: Use caution opening links or attachments
VFIO uses migration_file_set_error() in a couple of places where an
'Error **' parameter is not provided. In MemoryListener handlers :
vfio_listener_region_add
vfio_listener_log_global_stop
vfio_listener_log_sync
and in callback routines for IOMMU notifiers :
vfio_iommu_map_notify
vfio_iommu_map_dirty_notify
Hopefully, one day, we will be able to extend these callbacks with an
'Error **' parameter and avoid setting the global migration error.
Until then, it seems sensible to clearly identify the use cases, which
are limited, and open code vfio_migration_set_error(). One other
benefit is an improved error reporting when migration is running.
While at it, slightly modify error reporting to only report errors
when migration is not active and not always as is currently done.
Cc: Prasad Pandit
Signed-off-by: Cédric Le Goater
---
hw/vfio/common.c | 60 +---
1 file changed, 36 insertions(+), 24 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index
1a0d9290f88c9774a98f65087a36b86922b21a73..a591ce5b97ff41cdc8249e9eeafc8dc347d45fac
100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -149,13 +149,6 @@ bool vfio_viommu_preset(VFIODevice *vbasedev)
return vbasedev->bcontainer->space->as != &address_space_memory;
}
-static void vfio_set_migration_error(int ret)
-{
- if (migration_is_running()) {
- migration_file_set_error(ret, NULL);
- }
-}
Wouldn't it be better to extend vfio_set_migration_error() to take also Error*
instead of duplicating code?
We can rename it to vfio_set_error() if it's not solely related to vfio
migration anymore.
IMO, the vfio_set_migration_error() wrapper shadows the use of the
migration routines and their context. I prefer to be explicit about
it, open the code and work on removal. It is possible to add an
'Error **' parameter to log_global_stop handlers and to the IOMMU
notifiers. It just takes time.
Thanks,
C.
Thanks.
-
bool vfio_device_state_is_running(VFIODevice *vbasedev)
{
VFIOMigration *migration = vbasedev->migration;
@@ -291,9 +284,14 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n,
IOMMUTLBEntry *iotlb)
iova, iova + iotlb->addr_mask);
if (iotlb->target_as != &address_space_memory) {
- error_report("Wrong target AS \"%s\", only system memory is allowed",
- iotlb->target_as->name ? iotlb->target_as->name : "none");
- vfio_set_migration_error(-EINVAL);
+ error_setg(&local_err,
+ "Wrong target AS \"%s\", only system memory is allowed",
+ iotlb->target_as->name ? iotlb->target_as->name : "none");
+ if (migration_is_running()) {
+ migration_file_set_error(-EINVAL, local_err);
+ } else {
+ error_report_err(local_err);
+ }
return;
}
@@ -326,11 +324,16 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n,
IOMMUTLBEntry *iotlb)
ret = vfio_container_dma_unmap(bcontainer, iova,
iotlb->addr_mask + 1, iotlb);
if (ret) {
- error_report("vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
- "0x%"HWADDR_PRIx") = %d (%s)",
- bcontainer, iova,
- iotlb->addr_mask + 1, ret, strerror(-ret));
- vfio_set_migration_error(ret);
+ error_setg(&local_err,
+ "vfio_container_dma_unmap(%p, 0x%"HWADDR_PRIx", "
+ "0x%"HWADDR_PRIx") = %d (%s)",
+ bcontainer, iova,
+ iotlb->addr_mask + 1, ret, strerror(-ret));
+ if (migration_is_running()) {
+ migration_file_set_error(ret, local_err);
+ } else {
+ error_report_err(local_err);
+ }
}
}
out:
@@ -1112,8 +1115,11 @@ static void vfio_listener_log_global_stop(MemoryListener
*listener)
if (ret) {
error_prepend(&local_err,
"vfio: Could not stop dirty page tracking - ");
- error_report_err(local_err);
- vfio_set_migration_error(ret);
+ if (migration_is_running()) {
+ migration_file_set_error(ret, local_err);
+ } else {
+ error_report_err(local_err);
+ }
}
}
@@ -1229,14 +1235,14 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier
*n, IOMMUTLBEntry *iotlb)
trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
if (iotlb->target_as != &address_space_memory) {
- error_report("Wrong target AS \"%s\", only system memory is allowed",
- iotlb->target_as->name ? iotlb->target_as->name : "none");
+ error_setg(&local_err,
+
Re: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a pxb-pcie bus
On 3/24/25 2:55 PM, Shameerali Kolothum Thodi wrote: > Hi Eric, > >> -Original Message- >> From: qemu-devel- >> bounces+shameerali.kolothum.thodi=huawei@nongnu.org > devel-bounces+shameerali.kolothum.thodi=huawei@nongnu.org> On >> Behalf Of Eric Auger >> Sent: Monday, March 24, 2025 1:13 PM >> To: Shameerali Kolothum Thodi >> ; Nicolin Chen >> >> Cc: Donald Dutile ; qemu-...@nongnu.org; qemu- >> de...@nongnu.org; peter.mayd...@linaro.org; j...@nvidia.com; >> berra...@redhat.com; nath...@nvidia.com; mo...@nvidia.com; >> smost...@google.com; Linuxarm ; Wangzhou (B) >> ; jiangkunkun ; >> Jonathan Cameron ; >> zhangfei@linaro.org >> Subject: Re: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a pxb- >> pcie bus >> >> Hi Shameer, >> >> On 3/24/25 9:19 AM, Shameerali Kolothum Thodi wrote: -Original Message- From: Nicolin Chen Sent: Thursday, March 20, 2025 5:03 PM To: Shameerali Kolothum Thodi >> Cc: Donald Dutile ; qemu-...@nongnu.org; >> qemu- de...@nongnu.org; eric.au...@redhat.com; peter.mayd...@linaro.org; j...@nvidia.com; berra...@redhat.com; nath...@nvidia.com; mo...@nvidia.com; smost...@google.com; Linuxarm ; Wangzhou (B) ; jiangkunkun ; Jonathan Cameron ; zhangfei@linaro.org Subject: Re: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a >> pxb- pcie bus On Wed, Mar 19, 2025 at 09:26:29AM +, Shameerali Kolothum Thodi wrote: > Having said that, current code only allows pxb-pcie root complexes avoiding > the pcie.0. The idea behind this was, user can use pcie.0 with a non >> accel SMMUv3 > for any emulated devices avoiding the performance bottlenecks we are > discussing for emulated dev+smmuv3-accel cases. But based on the feedback from > Eric and Daniel I will relax that restriction and will allow association >> with pcie.0. Just want a clarification here.. If VM has a passthrough device only: attach it to PCIE.0 <=> vSMMU0 (accel=on) >>> Yes. Basically support accel=on to pcie.0 as well. >> agreed we shall be able to instantiate the accelerated SMMU on pcie.0 too. If VM has an emulated device and a passthrough device: attach the emulated device to PCIE.0 <=> vSMMU bypass (or accel=off?) attach the passthrough device to pxb-pcie <=> vSMMU0 (accel=on) >>> This can be other way around as well: >>> ie, >>> pass-through to pcie.0(accel=on) and emulated to any other pxb-pcie with >> accel = off. >> +1 >>> I think the way bus numbers are allocated in Qemu for pcie.0 and pxb- >> pcie allows >>> us to support this in IORT ID maps. >> One trouble we may get into is possible bus reordering by the guest. I >> don't know the details but I remember that in certain conditions the >> guest can reorder the bus numbers. > Yeah, Guest kernel can re-enumerate PCIe. I will check. > >> Besides what I don't get in the above discussion, related to whether the >> accelerated mode can also sipport emulated devices, is that if you use >> the originally suggested hierarchy (pxb-pcie + root port + VFIO device) >> you eventually get on guest side 2 devices protected by the SMMU >> instance: the root port and the VFIO device. They end up in different >> iommu groups. So there is already a mix of emulated and VFIO device. > True. But I guess the root port associated activity(invalidations etc) will be > very minimal(or nil?) compared to a virtio device. Agreed. I just meant discriminating between devices that can bring trouble and others may require some caution Eric > > Thanks, > Shameer > > >
Re: [PATCH-for-10.1 1/4] target/riscv: Restrict RV128 MTTCG check on system emulation
On 3/21/25 08:59, Philippe Mathieu-Daudé wrote: Multi-threaded TCG only concerns system emulation. That's not really true. User emulation simply has no option to run in a single-threaded context. I really don't think we should allow RV128 in user-mode at all. Certainly not until there's a kernel abi for it. r~
Re: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a pxb-pcie bus
On Mon, Mar 24, 2025 at 02:13:20PM +0100, Eric Auger wrote: > >> If VM has an emulated device and a passthrough device: > >> attach the emulated device to PCIE.0 <=> vSMMU bypass (or accel=off?) > >> attach the passthrough device to pxb-pcie <=> vSMMU0 (accel=on) > > This can be other way around as well: > > ie, > > pass-through to pcie.0(accel=on) and emulated to any other pxb-pcie with > > accel = off. > +1 > > > > I think the way bus numbers are allocated in Qemu for pcie.0 and pxb-pcie > > allows > > us to support this in IORT ID maps. > One trouble we may get into is possible bus reordering by the guest. I > don't know the details but I remember that in certain conditions the > guest can reorder the bus numbers. Hmm, that sounds troublesome. IORT mappings are done using the bus number, which is fixed to a vSMMU. Can we disable that reordering? > Besides what I don't get in the above discussion, related to whether the > accelerated mode can also sipport emulated devices, is that if you use > the originally suggested hierarchy (pxb-pcie + root port + VFIO device) > you eventually get on guest side 2 devices protected by the SMMU > instance: the root port and the VFIO device. They end up in different > iommu groups. So there is already a mix of emulated and VFIO device. Strictly speaking, yes, that's a mix. Maybe we should say emulated endpoints and passthrough endpoints? Thanks Nicolin
Re: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a pxb-pcie bus
On Mon, Mar 24, 2025 at 08:19:27AM +, Shameerali Kolothum Thodi wrote: > > If VM has an emulated device and a passthrough device: > > attach the emulated device to PCIE.0 <=> vSMMU bypass (or accel=off?) > > attach the passthrough device to pxb-pcie <=> vSMMU0 (accel=on) > > This can be other way around as well: > ie, > pass-through to pcie.0(accel=on) and emulated to any other pxb-pcie with > accel = off. > > I think the way bus numbers are allocated in Qemu for pcie.0 and pxb-pcie > allows > us to support this in IORT ID maps. That sounds fine. The reason why I picked pcie.0 for emulated devices, simply for keeping the design of pxb-pcie for multi- vSMMU cases. I think libvirt could still choose to keep it simple, although on the QEMU side we have to keep the flexibility. Thanks Nicolin
RE: [PATCH 17/39] target/hexagon: Implement software interrupt
> -Original Message-
> From: ltaylorsimp...@gmail.com
> Sent: Wednesday, March 19, 2025 4:28 PM
> To: 'Brian Cain' ; qemu-devel@nongnu.org
> Cc: richard.hender...@linaro.org; phi...@linaro.org; Matheus Bernardino
> (QUIC) ; a...@rev.ng; a...@rev.ng; Marco
> Liebel (QUIC) ; alex.ben...@linaro.org; Mark
> Burton (QUIC) ; Sid Manning
> ; Brian Cain ; 'Mike Lambert'
>
> Subject: RE: [PATCH 17/39] target/hexagon: Implement software interrupt
>
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
>
> > -Original Message-
> > From: Brian Cain
> > Sent: Friday, February 28, 2025 11:28 PM
> > To: qemu-devel@nongnu.org
> > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> > phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng;
> a...@rev.ng;
> > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> > alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com;
> > Brian Cain ; Mike Lambert
> > Subject: [PATCH 17/39] target/hexagon: Implement software interrupt
> >
> > From: Brian Cain
> >
> > Co-authored-by: Mike Lambert
> > Signed-off-by: Brian Cain
> > ---
> > target/hexagon/cpu.h | 1 -
> > target/hexagon/hexswi.h| 17 +++
> > target/hexagon/cpu.c | 2 +
> > target/hexagon/hexswi.c| 258
> > +
> > target/hexagon/op_helper.c | 1 +
> > 5 files changed, 278 insertions(+), 1 deletion(-) create mode 100644
> > target/hexagon/hexswi.h create mode 100644 target/hexagon/hexswi.c
> >
> > diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h index
> > dabee310c5..045581d7be 100644
> > --- a/target/hexagon/cpu.h
> > +++ b/target/hexagon/cpu.h
> > @@ -256,5 +256,4 @@ typedef HexagonCPU ArchCPU; void
> > hexagon_translate_init(void); void hexagon_translate_code(CPUState
> > *cs, TranslationBlock *tb,
> > int *max_insns, vaddr pc, void *host_pc);
> > -
>
> Gratuitous change
>
> > #endif /* HEXAGON_CPU_H */
> > diff --git a/target/hexagon/hexswi.h b/target/hexagon/hexswi.h new
> > file mode 100644 index 00..5d232cb06c
> > --- /dev/null
> > +++ b/target/hexagon/hexswi.h
> > @@ -0,0 +1,17 @@
> > +/*
> > + * Copyright(c) 2025 Qualcomm Innovation Center, Inc. All Rights
> Reserved.
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#ifndef HEXSWI_H
> > +#define HEXSWI_H
> > +
> > +
> > +#include "cpu.h"
> > +
> > +void hexagon_cpu_do_interrupt(CPUState *cpu); void
> > +register_trap_exception(CPUHexagonState *env, int type, int imm,
> > + target_ulong PC);
> > +
> > +#endif /* HEXSWI_H */
> > diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c index
> > 89a051b41d..843be8221f 100644
> > --- a/target/hexagon/cpu.c
> > +++ b/target/hexagon/cpu.c
> > @@ -33,6 +33,8 @@
> > #ifndef CONFIG_USER_ONLY
> > #include "sys_macros.h"
> > #include "qemu/main-loop.h"
> > +#include "hex_interrupts.h"
> > +#include "hexswi.h"
>
> Move these added include to a different patch where the contents are
> needed.
>
> > #endif
> >
> > static void hexagon_v66_cpu_init(Object *obj) { } diff --git
> > a/target/hexagon/hexswi.c b/target/hexagon/hexswi.c new file mode
> > 100644 index 00..5fcf9b2be9
> > --- /dev/null
> > +++ b/target/hexagon/hexswi.c
> > @@ -0,0 +1,258 @@
> > +/*
> > + * Copyright(c) 2019-2025 Qualcomm Innovation Center, Inc. All Rights
> > Reserved.
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include "qemu/osdep.h"
> > +#include "cpu.h"
> > +#ifdef CONFIG_USER_ONLY
>
> This file is only included in the system-mode build, so we don't need these
> guards. Several in this file.
>
> > +#include "exec/helper-proto.h"
> > +#include "qemu.h"
> > +#endif
> > +#include "exec/cpu_ldst.h"
> > +#include "exec/exec-all.h"
> > +#include "qemu/log.h"
> > +#include "qemu/main-loop.h"
> > +#include "arch.h"
> > +#include "internal.h"
> > +#include "macros.h"
> > +#include "sys_macros.h"
> > +#include "tcg/tcg-op.h"
> > +#ifndef CONFIG_USER_ONLY
> > +#include "hex_mmu.h"
> > +#include "hexswi.h"
> > +#endif
> > +
> > +#ifndef CONFIG_USER_ONLY
> > +
> > +
> > +static void set_addresses(CPUHexagonState *env, target_ulong
> pc_offset,
> > + target_ulong exception_index)
> > +
> > +{
> > +arch_set_system_reg(env, HEX_SREG_ELR,
> > +arch_get_thread_reg(env, HEX_REG_PC) + pc_offset);
> > +arch_set_thread_reg(env, HEX_REG_PC,
> > +arch_get_system_reg(env, HEX_SREG_EVB) |
> > +(exception_index << 2)); }
> > +
> > +static const char *event_name[] = {
> > +[HEX_EVENT_RESET] = "HEX_EVENT_RESET",
> > +[HEX_EVENT_IMPRECISE] = "HEX_EVENT_IMPRECISE",
> > +[HEX_EVENT_TLB_MISS_X] = "HEX_EVENT_TLB_MISS_X",
> > +[HEX_EVENT_TLB_MISS_RW] = "HEX_EVENT_TLB_MISS_RW",
> > +[HEX_EVENT_TRAP0] = "HEX_EVENT_TRAP0",
>
[RFC 1/3] vdagent: Wrap vdagent_register_to_qemu_clipboard function
From: Hyman Huang
For direct use in the upcoming commit, wrap the vdagent
registry logic as vdagent_register_to_qemu_clipboard.
Meanwhile, add a trace event for vdagent_recv_caps.
Signed-off-by: Hyman Huang
---
ui/trace-events | 1 +
ui/vdagent.c| 23 ---
2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/ui/trace-events b/ui/trace-events
index 3da0d5e280..427a8383cb 100644
--- a/ui/trace-events
+++ b/ui/trace-events
@@ -140,6 +140,7 @@ vdagent_send(const char *name) "msg %s"
vdagent_send_empty_clipboard(void) ""
vdagent_recv_chunk(uint32_t size) "size %d"
vdagent_recv_msg(const char *name, uint32_t size) "msg %s, size %d"
+vdagent_recv_caps(uint32_t caps) "received caps %u"
vdagent_peer_cap(const char *name) "cap %s"
vdagent_cb_grab_selection(const char *name) "selection %s"
vdagent_cb_grab_discard(const char *name, int cur, int recv) "selection %s,
cur:%d recv:%d"
diff --git a/ui/vdagent.c b/ui/vdagent.c
index 724eff972f..7a1cf674d0 100644
--- a/ui/vdagent.c
+++ b/ui/vdagent.c
@@ -694,6 +694,18 @@ static void vdagent_chr_open(Chardev *chr,
*be_opened = true;
}
+static void vdagent_register_to_qemu_clipboard(VDAgentChardev *vd)
+{
+if (vd->cbpeer.notifier.notify == NULL) {
+qemu_clipboard_reset_serial();
+
+vd->cbpeer.name = "vdagent";
+vd->cbpeer.notifier.notify = vdagent_clipboard_notify;
+vd->cbpeer.request = vdagent_clipboard_request;
+qemu_clipboard_peer_register(&vd->cbpeer);
+}
+}
+
static void vdagent_chr_recv_caps(VDAgentChardev *vd, VDAgentMessage *msg)
{
VDAgentAnnounceCapabilities *caps = (void *)msg->data;
@@ -711,6 +723,8 @@ static void vdagent_chr_recv_caps(VDAgentChardev *vd,
VDAgentMessage *msg)
}
vd->caps = caps->caps[0];
+trace_vdagent_recv_caps(vd->caps);
+
if (caps->request) {
vdagent_send_caps(vd, false);
}
@@ -720,13 +734,8 @@ static void vdagent_chr_recv_caps(VDAgentChardev *vd,
VDAgentMessage *msg)
memset(vd->last_serial, 0, sizeof(vd->last_serial));
-if (have_clipboard(vd) && vd->cbpeer.notifier.notify == NULL) {
-qemu_clipboard_reset_serial();
-
-vd->cbpeer.name = "vdagent";
-vd->cbpeer.notifier.notify = vdagent_clipboard_notify;
-vd->cbpeer.request = vdagent_clipboard_request;
-qemu_clipboard_peer_register(&vd->cbpeer);
+if (have_clipboard(vd)) {
+vdagent_register_to_qemu_clipboard(vd);
}
}
--
2.27.0
RE: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a pxb-pcie bus
> -Original Message- > From: Nicolin Chen > Sent: Monday, March 24, 2025 4:02 PM > To: Eric Auger > Cc: Shameerali Kolothum Thodi > ; Donald Dutile > ; qemu-...@nongnu.org; qemu-devel@nongnu.org; > peter.mayd...@linaro.org; j...@nvidia.com; berra...@redhat.com; > nath...@nvidia.com; mo...@nvidia.com; smost...@google.com; Linuxarm > ; Wangzhou (B) ; > jiangkunkun ; Jonathan Cameron > ; zhangfei@linaro.org > Subject: Re: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a pxb- > pcie bus > > On Mon, Mar 24, 2025 at 02:13:20PM +0100, Eric Auger wrote: > > >> If VM has an emulated device and a passthrough device: > > >> attach the emulated device to PCIE.0 <=> vSMMU bypass (or > accel=off?) > > >> attach the passthrough device to pxb-pcie <=> vSMMU0 (accel=on) > > > This can be other way around as well: > > > ie, > > > pass-through to pcie.0(accel=on) and emulated to any other pxb-pcie > with accel = off. > > +1 > > > > > > I think the way bus numbers are allocated in Qemu for pcie.0 and pxb- > pcie allows > > > us to support this in IORT ID maps. > > One trouble we may get into is possible bus reordering by the guest. I > > don't know the details but I remember that in certain conditions the > > guest can reorder the bus numbers. > > Hmm, that sounds troublesome. IORT mappings are done using the bus > number, which is fixed to a vSMMU. Can we disable that reordering? DSM 5# is actually a way to do that. But I don't think we need that as host kernel also will have the same issues with IORT if re enumeration happens. I think the iommu_fwspec mechanism is to take care of this. I need to double check though. Thanks, Shameer
Re: [PATCH-for-10.1 3/3] migration/cpu: Remove qemu_{get,put}_[s]betl[s] macros
Philippe Mathieu-Daudé writes: > The following macros: > > - qemu_put_betl() > - qemu_get_betl() > - qemu_put_betls() > - qemu_get_betls() > - qemu_put_sbetl() > - qemu_get_sbetl() > - qemu_put_sbetls() > - qemu_get_sbetls() > > are not used in the whole code base, remove them. > > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Fabiano Rosas
Re: [PATCH for-10.1 24/32] vfio: Introduce new files for dirty tracking definitions and declarations
On 3/20/25 10:46, John Levon wrote: On Tue, Mar 18, 2025 at 10:54:07AM +0100, Cédric Le Goater wrote: File "common.c" has been emptied of most of its definitions by the previous changes and the only definitions left are related to dirty tracking. Rename it to "dirty-tracking.c" and introduce its associated "dirty-tracking.h" header file for the declarations. Cleanup a little the includes while at it. Signed-off-by: Cédric Le Goater rename from hw/vfio/common.c rename to hw/vfio/dirty-tracking.c index ed2f2ed8839caaf40fabb0cbbcaa1df2c5b70d67..441f9d9a08c06a88dda44ef143dcee5f0a89a900 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/dirty-tracking.c @@ -20,14 +20,10 @@ I think you might want to update the file comment from "generic functions used by VFIO devices". Yes. next respin will have to take a closer look at the top of the files: comments, copyrights, includes. Thanks, C.
Re: [PATCH v6] hw/misc/vmfwupdate: Introduce hypervisor fw-cfg interface support
On Fri, Mar 21, 2025 at 09:22:59AM +0100, Gerd Hoffman wrote: > On Thu, Mar 20, 2025 at 09:34:26AM +0100, Jörg Rödel wrote: > > On Tue, Mar 18, 2025 at 12:11:02PM +0100, Gerd Hoffman wrote: > > > Open questions: > > > > > > - Does the idea to use igvm parameters for the kernel hashes makes > > >sense? Are parameters part of the launch measurement? > > > > Parameters itself are fully measured, their presence is, but not their > > data. This is to keep the same launch measurements across different > > platform configurations. > > > > So for hashes it is best to put some on some measured page and let the > > parameters point to it. > > Had a look at the kernel hashes details this week. > > So, the story is this: It's essentially a private arrangement between > ovmf (the amdsev build variant only) and qemu. The hashes are placed in > a specific page, together with "launch secrets" (that is not the sev-snp > "secrets" page). That page is part of the lanuch measurement. That > effectively makes the kernel + initrd + cmdline part of the launch > measurement too (ovmf verifies the hashes), but without the relatively > slow secure processor hashing kernel + initrd + cmdline, which reduces > the time needed to launch a VM. > > The "launch secret" is intended to hold things like a luks secret to > unlock the root filesystem. OVMF doesn't touch it but reserves the page > and registers a EFI table for it so the linux kernel can find it. > > As far I know these are more experimental bits than something actually > used in production. It's also clearly a pre-UKI design. That IMHO > opens up the question whenever we actually want carry forward with that, > or if we better check out what alternatives we have. We'll have a > signed UKI after all, so going for secure boot and/or measured boot for > the UKI verification looks attractive compared to passing around hashes > for the elements inside the UKI. Although it has been extended to SNP in QEMU, personally I'd consider the QEMU 'kernel hashes' functionality to be an niche feature. >From a virt mgmt POV we know in general that direct kernel boot while useful, is somewhat limited in usage. Most of the time it is saner to have the guest decide what kernel to boot without interaction of the host. So by extension, the kernel hashes feature is also going to be a similarly (or even more) niche use case. The UKI with system extensions and secureboot model gives much greater flexibility for defining policy around valid configurations than using fixed hashes - especially when it comes to kernel command line which has potentially many valid configs. > Not fully sure what to do about the "launch secrets". IIRC the initial > design of this is for sev-es, i.e. pre-snp, so maybe the sev-snp secrets > page can be used instead. I see the spec has 0x60 bytes (offset 0xa0) > reserved for guest os usage. In any case this probably is only needed > as temporary stopgap until we have a complete vTPM implementation for > the svsm. I view the launch secrets feature as a crutch to cope with the HW limitations of SEV/SEV-ES. You have the host initiated attestation dance and after verification can pass data back to the host, which will inject it into the guest. The inability to have a secure TPM in SEV/SEV-ES forces you down this road. It is highly undesirable as a feature going forward because we're better served by having a boot workflow that is common to traditional virt, confidential virt, and bare metal, and TPM delivers on that. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PULL 00/12] ppc-for-10.0-2 queue
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/10.0 for any user-visible changes. signature.asc Description: PGP signature
Re: [PULL 0/8] Error reporting patches for 2025-03-21
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/10.0 for any user-visible changes. signature.asc Description: PGP signature
Re: [PULL 0/6] Uefi 20250321 patches
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/10.0 for any user-visible changes. signature.asc Description: PGP signature
Re: [PATCH] smbios: Fix buffer overrun when using path= option
CC qemu-stable - this needs cherry-picking into all active stable
branches once accepted.
On Mon, Mar 24, 2025 at 09:12:53AM +, Daniel P. Berrangé wrote:
> On Sun, Mar 23, 2025 at 10:35:54PM +0100, Daan De Meyer wrote:
> > We have to make sure the array of bytes read from the path= file
> > is null-terminated, otherwise we run into a buffer overrun later on.
> >
> > Fixes: bb99f4772f54017490e3356ecbb3df25c5d4537f ("hw/smbios: support
> > loading OEM strings values from a file")
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2879
> >
> > Signed-off-by: Daan De Meyer
> > ---
> > hw/smbios/smbios.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> > index 02a09eb9cd..ad4cd6721e 100644
> > --- a/hw/smbios/smbios.c
> > +++ b/hw/smbios/smbios.c
> > @@ -1285,6 +1285,9 @@ static int save_opt_one(void *opaque,
> > g_byte_array_append(data, (guint8 *)buf, ret);
> > }
> >
> > +buf[0] = '\0';
> > +g_byte_array_append(data, (guint8 *)buf, 1);
> > +
> > qemu_close(fd);
> >
> > *opt->dest = g_renew(char *, *opt->dest, (*opt->ndest) + 1);
>
> Reviewed-by: Daniel P. Berrangé
>
> With regards,
> Daniel
> --
> |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
>
>
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PULL 0/3] loongarch-to-apply queue
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/10.0 for any user-visible changes. signature.asc Description: PGP signature
Re: [PULL 0/3] Trivial patches for 2025-03-21
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/10.0 for any user-visible changes. signature.asc Description: PGP signature
Re: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a pxb-pcie bus
Hi Shameer, On 3/24/25 9:19 AM, Shameerali Kolothum Thodi wrote: > >> -Original Message- >> From: Nicolin Chen >> Sent: Thursday, March 20, 2025 5:03 PM >> To: Shameerali Kolothum Thodi >> Cc: Donald Dutile ; qemu-...@nongnu.org; qemu- >> de...@nongnu.org; eric.au...@redhat.com; peter.mayd...@linaro.org; >> j...@nvidia.com; berra...@redhat.com; nath...@nvidia.com; >> mo...@nvidia.com; smost...@google.com; Linuxarm >> ; Wangzhou (B) ; >> jiangkunkun ; Jonathan Cameron >> ; zhangfei@linaro.org >> Subject: Re: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a pxb- >> pcie bus >> >> On Wed, Mar 19, 2025 at 09:26:29AM +, Shameerali Kolothum Thodi >> wrote: >>> Having said that, current code only allows pxb-pcie root complexes >> avoiding >>> the pcie.0. The idea behind this was, user can use pcie.0 with a non accel >> SMMUv3 >>> for any emulated devices avoiding the performance bottlenecks we are >>> discussing for emulated dev+smmuv3-accel cases. But based on the >> feedback from >>> Eric and Daniel I will relax that restriction and will allow association >>> with >> pcie.0. >> >> Just want a clarification here.. >> >> If VM has a passthrough device only: >> attach it to PCIE.0 <=> vSMMU0 (accel=on) > Yes. Basically support accel=on to pcie.0 as well. agreed we shall be able to instantiate the accelerated SMMU on pcie.0 too. > >> If VM has an emulated device and a passthrough device: >> attach the emulated device to PCIE.0 <=> vSMMU bypass (or accel=off?) >> attach the passthrough device to pxb-pcie <=> vSMMU0 (accel=on) > This can be other way around as well: > ie, > pass-through to pcie.0(accel=on) and emulated to any other pxb-pcie with > accel = off. +1 > > I think the way bus numbers are allocated in Qemu for pcie.0 and pxb-pcie > allows > us to support this in IORT ID maps. One trouble we may get into is possible bus reordering by the guest. I don't know the details but I remember that in certain conditions the guest can reorder the bus numbers. Besides what I don't get in the above discussion, related to whether the accelerated mode can also sipport emulated devices, is that if you use the originally suggested hierarchy (pxb-pcie + root port + VFIO device) you eventually get on guest side 2 devices protected by the SMMU instance: the root port and the VFIO device. They end up in different iommu groups. So there is already a mix of emulated and VFIO device. Thanks Eric > > Thanks, > Shameer >
Re: [PATCH 1/6] ui/dmabuf: extend QemuDmaBuf to support multi-plane
On Mon, Mar 24, 2025 at 6:04 PM Marc-André Lureau
wrote:
>
> Hi
>
> On Mon, Mar 24, 2025 at 12:19 PM wrote:
> >
> > From: Qiang Yu
> >
> > mesa/radeonsi is going to support explicit midifier which
> > may export a multi-plane texture. For example, texture with
> > DCC enabled (a compressed format) has two planes, one with
> > compressed data, the other with meta data for compression.
> >
> > Signed-off-by: Qiang Yu
> > ---
> > hw/display/vhost-user-gpu.c | 6 ++-
> > hw/display/virtio-gpu-udmabuf.c | 6 ++-
> > hw/vfio/display.c | 7 ++--
> > include/ui/dmabuf.h | 20 ++
> > ui/dbus-listener.c | 10 ++---
> > ui/dmabuf.c | 67 +
> > ui/egl-helpers.c| 4 +-
> > ui/spice-display.c | 4 +-
> > 8 files changed, 76 insertions(+), 48 deletions(-)
> >
> > diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
> > index 2aed6243f6..a7949c7078 100644
> > --- a/hw/display/vhost-user-gpu.c
> > +++ b/hw/display/vhost-user-gpu.c
> > @@ -249,6 +249,8 @@ 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);
> > +uint32_t offset = 0;
> > +uint32_t stride = m->fd_stride;
> > uint64_t modifier = 0;
> > QemuDmaBuf *dmabuf;
> >
> > @@ -282,10 +284,10 @@ vhost_user_gpu_handle_display(VhostUserGPU *g,
> > VhostUserGpuMsg *msg)
> > }
> >
> > dmabuf = qemu_dmabuf_new(m->width, m->height,
> > - m->fd_stride, 0, 0,
> > + &offset, &stride, 0, 0,
> > m->fd_width, m->fd_height,
> > m->fd_drm_fourcc, modifier,
> > - fd, false, m->fd_flags &
> > + &fd, 1, false, m->fd_flags &
> > VIRTIO_GPU_RESOURCE_FLAG_Y_0_TOP);
> >
> > dpy_gl_scanout_dmabuf(con, dmabuf);
> > diff --git a/hw/display/virtio-gpu-udmabuf.c
> > b/hw/display/virtio-gpu-udmabuf.c
> > index 85ca23cb32..34fbe05b7a 100644
> > --- a/hw/display/virtio-gpu-udmabuf.c
> > +++ b/hw/display/virtio-gpu-udmabuf.c
> > @@ -176,16 +176,18 @@ static VGPUDMABuf
> >struct virtio_gpu_rect *r)
> > {
> > VGPUDMABuf *dmabuf;
> > +uint32_t offset = 0;
> >
> > if (res->dmabuf_fd < 0) {
> > return NULL;
> > }
> >
> > dmabuf = g_new0(VGPUDMABuf, 1);
> > -dmabuf->buf = qemu_dmabuf_new(r->width, r->height, fb->stride,
> > +dmabuf->buf = qemu_dmabuf_new(r->width, r->height,
> > + &offset, &fb->stride,
> >r->x, r->y, fb->width, fb->height,
> >qemu_pixman_to_drm_format(fb->format),
> > - 0, res->dmabuf_fd, true, false);
> > + 0, &res->dmabuf_fd, 1, true, false);
> > dmabuf->scanout_id = scanout_id;
> > QTAILQ_INSERT_HEAD(&g->dmabuf.bufs, dmabuf, next);
> >
> > diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> > index ea87830fe0..9d882235fb 100644
> > --- a/hw/vfio/display.c
> > +++ b/hw/vfio/display.c
> > @@ -214,6 +214,7 @@ static VFIODMABuf
> > *vfio_display_get_dmabuf(VFIOPCIDevice *vdev,
> > struct vfio_device_gfx_plane_info plane;
> > VFIODMABuf *dmabuf;
> > int fd, ret;
> > +uint32_t offset = 0;
> >
> > memset(&plane, 0, sizeof(plane));
> > plane.argsz = sizeof(plane);
> > @@ -246,10 +247,10 @@ static VFIODMABuf
> > *vfio_display_get_dmabuf(VFIOPCIDevice *vdev,
> >
> > dmabuf = g_new0(VFIODMABuf, 1);
> > dmabuf->dmabuf_id = plane.dmabuf_id;
> > -dmabuf->buf = qemu_dmabuf_new(plane.width, plane.height,
> > - plane.stride, 0, 0, plane.width,
> > +dmabuf->buf = qemu_dmabuf_new(plane.width, plane.height, &offset,
> > + &plane.stride, 0, 0, plane.width,
> >plane.height, plane.drm_format,
> > - plane.drm_format_mod, fd, false, false);
> > + plane.drm_format_mod, &fd, 1, false,
> > false);
> >
> > if (plane_type == DRM_PLANE_TYPE_CURSOR) {
> > vfio_display_update_cursor(dmabuf, &plane);
> > diff --git a/include/ui/dmabuf.h b/include/ui/dmabuf.h
> > index dc74ba895a..407fb2274e 100644
> > --- a/include/ui/dmabuf.h
> > +++ b/include/ui/dmabuf.h
> > @@ -10,24 +10,29 @@
> > #ifndef DMABUF_H
> > #define DMABUF_H
> >
> > +#define DMABUF_MAX_PLANES 4
> > +
> > typedef struct QemuDmaBuf QemuDmaBuf;
> >
> > QemuDmaBuf *qemu_dmabuf_new(uint32_t width, uint32_t height,
> > -
