Re: [PATCH] Issue #2294 | Machine microvm doesn't run under Xen accel for x86_64
On Tue, 28 May 2024 13:23, Will Gyda wrote: Issue #2294: Machine microvm doesn't run under Xen accel for qemu-system-x86_64. Solution: microvm is now not build if only Xen is available. Signed-off-by: Will Gyda I suggest rewording the commit title to something like "i386: remove microvm from default build" And adding a commit message that explains that the microvm does not work on Xen, hence if only Xen is available it should not be built. Also, you can add a Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2294 line before your Signed-off-by. See https://www.qemu.org/docs/master/devel/submitting-a-patch.html But, seeing the issue itself, it's about the microvm being stuck under Xen. So the commit that resolves this would either make it non-stuck or make it impossible to start the vm to begin with. --- configs/devices/i386-softmmu/default.mak | 2 +- hw/i386/Kconfig | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/configs/devices/i386-softmmu/default.mak b/configs/devices/i386-softmmu/default.mak index 598c6646df..6a73aee7dd 100644 --- a/configs/devices/i386-softmmu/default.mak +++ b/configs/devices/i386-softmmu/default.mak @@ -29,4 +29,4 @@ CONFIG_ISAPC=y CONFIG_I440FX=y CONFIG_Q35=y -CONFIG_MICROVM=y +#CONFIG_MICROVM=n Better remove this altogether since it's not a default anymore. diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig index a6ee052f9a..f8ec8ebd7a 100644 --- a/hw/i386/Kconfig +++ b/hw/i386/Kconfig @@ -108,6 +108,8 @@ config Q35 config MICROVM bool +default y +depends on KVM || WHPX || NVMM || HVF What about TCG? Will it be available if we only build with tcg? select SERIAL_ISA # for serial_hds_isa_init() select ISA_BUS select APIC -- 2.25.1
Re: [PATCH] Issue #2294 | Machine microvm doesn't run under Xen accel for x86_64
On Wed, 29 May 2024 at 11:25, Vilhelm Gyda wrote: > > @philmd commented on gitlab: Discussed with @epilys on IRC, only Xen > machines (xenpv/xenfv) configure Xen so can run under it. > > But if we want to make it work under Xen. Any ideas how to move in > that direction? We'd have to specify what "works under Xen" means; xen as a type 1 hypervisor? I am trying to think if it makes sense, Xen machines in qemu already provide PV devices analogously to what microvm promises to support. What would be the use case for a "hypervisor agnostic" microvm machine? > > On Wed, May 29, 2024 at 12:37 PM Paolo Bonzini wrote: > > > > On 5/28/24 12:23, Will Gyda wrote: > > > Issue #2294: Machine microvm doesn't run under Xen accel for > > > qemu-system-x86_64. > > > Solution: microvm is now not build if only Xen is available. > > > > This does not fix the issue that microvm does not start with a Xen > > accelerator. I think it would be better to try and make it work instead. > > > > Paolo > > > > > Signed-off-by: Will Gyda > > > > > > --- > > > configs/devices/i386-softmmu/default.mak | 2 +- > > > hw/i386/Kconfig | 2 ++ > > > 2 files changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/configs/devices/i386-softmmu/default.mak > > > b/configs/devices/i386-softmmu/default.mak > > > index 598c6646df..6a73aee7dd 100644 > > > --- a/configs/devices/i386-softmmu/default.mak > > > +++ b/configs/devices/i386-softmmu/default.mak > > > @@ -29,4 +29,4 @@ > > > CONFIG_ISAPC=y > > > CONFIG_I440FX=y > > > CONFIG_Q35=y > > > -CONFIG_MICROVM=y > > > +#CONFIG_MICROVM=n > > > diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig > > > index a6ee052f9a..f8ec8ebd7a 100644 > > > --- a/hw/i386/Kconfig > > > +++ b/hw/i386/Kconfig > > > @@ -108,6 +108,8 @@ config Q35 > > > > > > config MICROVM > > > bool > > > +default y > > > +depends on KVM || WHPX || NVMM || HVF > > > select SERIAL_ISA # for serial_hds_isa_init() > > > select ISA_BUS > > > select APIC > > > -- Manos Pitsidianakis Emulation and Virtualization Engineer at Linaro Ltd
Re: [PATCH v2 1/4] MAINTAINERS: drop audio maintainership
On Tue, 28 May 2024 at 11:39, Gerd Hoffmann wrote: > > Remove myself from audio (both devices and backend) entries. > Flip status to "Orphan" for entries which have nobody else listed. > Signed-off-by: Gerd Hoffmann > --- > MAINTAINERS | 30 ++ > 1 file changed, 10 insertions(+), 20 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 448dc951c509..58e44885ce94 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1913,8 +1913,7 @@ F: include/hw/xtensa/mx_pic.h > Devices > --- > Overall Audio frontends > -M: Gerd Hoffmann > -S: Odd Fixes > +S: Orphan > F: hw/audio/ > F: include/hw/audio/ > F: tests/qtest/ac97-test.c > @@ -2389,8 +2388,8 @@ F: hw/virtio/virtio-mem-pci.c > F: include/hw/virtio/virtio-mem.h > > virtio-snd > -M: Gerd Hoffmann > -R: Manos Pitsidianakis > +M: Manos Pitsidianakis > +R: Matias Ezequiel Vara Larsen > S: Supported > F: hw/audio/virtio-snd.c > F: hw/audio/virtio-snd-pci.c While extra reviewers are always helpful, someone like Volker would make sense, not someone without any contributions: $ git log --format="%an" hw/audio/virtio-snd.c hw/audio/virtio-snd-pci.c include/hw/audio/virtio-snd.h docs/system/devices/virtio-snd.rst | sort -u Manos Pitsidianakis Michael Tokarev Philippe Mathieu-Daudé Richard Henderson Stefan Hajnoczi Volker Rümelin Zheyu Ma I'd suggest leaving adding reviewers here for a different patch submission. Otherwise: Reviewed-by: Manos Pitsidianakis > @@ -2768,7 +2767,6 @@ F: include/hw/hyperv/hv-balloon.h > Subsystems > -- > Overall Audio backends > -M: Gerd Hoffmann > M: Marc-André Lureau > S: Odd Fixes > F: audio/ > @@ -2784,13 +2782,11 @@ X: audio/spiceaudio.c > F: qapi/audio.json > > ALSA Audio backend > -M: Gerd Hoffmann > R: Christian Schoenebeck > -S: Odd Fixes > +S: Orphan > F: audio/alsaaudio.c > > Core Audio framework backend > -M: Gerd Hoffmann > M: Philippe Mathieu-Daudé > R: Christian Schoenebeck > R: Akihiko Odaki > @@ -2798,36 +2794,30 @@ S: Odd Fixes > F: audio/coreaudio.c > > DSound Audio backend > -M: Gerd Hoffmann > -S: Odd Fixes > +S: Orphan > F: audio/dsound* > > JACK Audio Connection Kit backend > -M: Gerd Hoffmann > R: Christian Schoenebeck > -S: Odd Fixes > +S: Orphan > F: audio/jackaudio.c > > Open Sound System (OSS) Audio backend > -M: Gerd Hoffmann > -S: Odd Fixes > +S: Orphan > F: audio/ossaudio.c > > PulseAudio backend > -M: Gerd Hoffmann > -S: Odd Fixes > +S: Orphan > F: audio/paaudio.c > > SDL Audio backend > -M: Gerd Hoffmann > -R: Thomas Huth > +M: Thomas Huth > S: Odd Fixes > F: audio/sdlaudio.c > > Sndio Audio backend > -M: Gerd Hoffmann > R: Alexandre Ratchov > -S: Odd Fixes > +S: Orphan > F: audio/sndioaudio.c > > Block layer core > -- > 2.45.1 >
Re: [PATCH v6 8/8] hw/arm: xen: Enable use of grant mappings
On Thu, 16 May 2024 18:48, "Edgar E. Iglesias" wrote: From: "Edgar E. Iglesias" Signed-off-by: Edgar E. Iglesias Reviewed-by: Stefano Stabellini --- hw/arm/xen_arm.c | 5 + 1 file changed, 5 insertions(+) diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c index 15fa7dfa84..6fad829ede 100644 --- a/hw/arm/xen_arm.c +++ b/hw/arm/xen_arm.c @@ -125,6 +125,11 @@ static void xen_init_ram(MachineState *machine) GUEST_RAM1_BASE, ram_size[1]); memory_region_add_subregion(sysmem, GUEST_RAM1_BASE, _hi); } + +/* Setup support for grants. */ +memory_region_init_ram(_grants, NULL, "xen.grants", block_len, + _fatal); +memory_region_add_subregion(sysmem, XEN_GRANT_ADDR_OFF, _grants); } void arch_handle_ioreq(XenIOState *state, ioreq_t *req) -- 2.40.1 Reviewed-by: Manos Pitsidianakis
Re: [PATCH v6 7/8] xen: mapcache: Add support for grant mappings
On Thu, 16 May 2024 18:48, "Edgar E. Iglesias" wrote:
From: "Edgar E. Iglesias"
Add a second mapcache for grant mappings. The mapcache for
grants needs to work with XC_PAGE_SIZE granularity since
we can't map larger ranges than what has been granted to us.
Like with foreign mappings (xen_memory), machines using grants
are expected to initialize the xen_grants MR and map it
into their address-map accordingly.
Signed-off-by: Edgar E. Iglesias
Reviewed-by: Stefano Stabellini
---
hw/xen/xen-hvm-common.c | 12 ++-
hw/xen/xen-mapcache.c | 163 ++--
include/hw/xen/xen-hvm-common.h | 3 +
include/sysemu/xen.h| 7 ++
4 files changed, 152 insertions(+), 33 deletions(-)
diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
index a0a0252da0..b8ace1c368 100644
--- a/hw/xen/xen-hvm-common.c
+++ b/hw/xen/xen-hvm-common.c
@@ -10,12 +10,18 @@
#include "hw/boards.h"
#include "hw/xen/arch_hvm.h"
-MemoryRegion xen_memory;
+MemoryRegion xen_memory, xen_grants;
-/* Check for xen memory. */
+/* Check for any kind of xen memory, foreign mappings or grants. */
bool xen_mr_is_memory(MemoryRegion *mr)
{
-return mr == _memory;
+return mr == _memory || mr == _grants;
+}
+
+/* Check specifically for grants. */
+bool xen_mr_is_grants(MemoryRegion *mr)
+{
+return mr == _grants;
}
void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr,
diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
index a07c47b0b1..1cbc2aeaa9 100644
--- a/hw/xen/xen-mapcache.c
+++ b/hw/xen/xen-mapcache.c
@@ -14,6 +14,7 @@
#include
+#include "hw/xen/xen-hvm-common.h"
#include "hw/xen/xen_native.h"
#include "qemu/bitmap.h"
@@ -21,6 +22,8 @@
#include "sysemu/xen-mapcache.h"
#include "trace.h"
+#include
+#include
#if HOST_LONG_BITS == 32
# define MCACHE_MAX_SIZE (1UL<<31) /* 2GB Cap */
@@ -41,6 +44,7 @@ typedef struct MapCacheEntry {
unsigned long *valid_mapping;
uint32_t lock;
#define XEN_MAPCACHE_ENTRY_DUMMY (1 << 0)
+#define XEN_MAPCACHE_ENTRY_GRANT (1 << 1)
Might we get more entry kinds in the future? (for example foreign maps).
Maybe this could be an enum.
uint8_t flags;
hwaddr size;
struct MapCacheEntry *next;
@@ -71,6 +75,8 @@ typedef struct MapCache {
} MapCache;
static MapCache *mapcache;
+static MapCache *mapcache_grants;
+static xengnttab_handle *xen_region_gnttabdev;
static inline void mapcache_lock(MapCache *mc)
{
@@ -131,6 +137,12 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void
*opaque)
unsigned long max_mcache_size;
unsigned int bucket_shift;
+xen_region_gnttabdev = xengnttab_open(NULL, 0);
+if (xen_region_gnttabdev == NULL) {
+error_report("mapcache: Failed to open gnttab device");
+exit(EXIT_FAILURE);
+}
+
if (HOST_LONG_BITS == 32) {
bucket_shift = 16;
} else {
@@ -159,6 +171,15 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void
*opaque)
mapcache = xen_map_cache_init_single(f, opaque,
bucket_shift,
max_mcache_size);
+
+/*
+ * Grant mappings must use XC_PAGE_SIZE granularity since we can't
+ * map anything beyond the number of pages granted to us.
+ */
+mapcache_grants = xen_map_cache_init_single(f, opaque,
+XC_PAGE_SHIFT,
+max_mcache_size);
+
setrlimit(RLIMIT_AS, _as);
}
@@ -168,17 +189,24 @@ static void xen_remap_bucket(MapCache *mc,
hwaddr size,
hwaddr address_index,
bool dummy,
+ bool grant,
+ bool is_write,
ram_addr_t ram_offset)
{
uint8_t *vaddr_base;
-xen_pfn_t *pfns;
+uint32_t *refs = NULL;
+xen_pfn_t *pfns = NULL;
int *err;
You should use g_autofree to perform automatic cleanup on function exit
instead of manually freeing, since the allocations should only live
within the function call.
unsigned int i;
hwaddr nb_pfn = size >> XC_PAGE_SHIFT;
trace_xen_remap_bucket(address_index);
-pfns = g_new0(xen_pfn_t, nb_pfn);
+if (grant) {
+refs = g_new0(uint32_t, nb_pfn);
+} else {
+pfns = g_new0(xen_pfn_t, nb_pfn);
+}
err = g_new0(int, nb_pfn);
if (entry->vaddr_base != NULL) {
@@ -207,21 +235,51 @@ static void xen_remap_bucket(MapCache *mc,
g_free(entry->valid_mapping);
entry->valid_mapping = NULL;
-for (i = 0; i < nb_pfn; i++) {
-pfns[i] = (address_index << (mc->bucket_shift - XC_PAGE_SHIFT)) + i;
+if (grant) {
+hwaddr grant_base = address_index - (ram_offset >> XC_PAGE_SHIFT);
+
+for (i = 0; i < nb_pfn; i++) {
+refs[i] = grant_base + i;
+}
+} else {
+for (i = 0; i < nb_pfn; i++) {
+
Re: [PATCH 2/4] MAINTAINERS: drop usb maintainership
On Thu, 16 May 2024 15:03, Gerd Hoffmann wrote: Remove myself from usb entries. Flip status to "Orphan" for entries which have nobody else listed. Signed-off-by: Gerd Hoffmann --- MAINTAINERS | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 7f52e2912fc3..d81376f84746 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2140,8 +2140,7 @@ F: tests/qtest/fuzz-sdcard-test.c F: tests/qtest/sdhci-test.c USB -M: Gerd Hoffmann -S: Odd Fixes +S: Orphan F: hw/usb/* F: stubs/usb-dev-stub.c F: tests/qtest/usb-*-test.c @@ -2150,7 +2149,6 @@ F: include/hw/usb.h F: include/hw/usb/ USB (serial adapter) -R: Gerd Hoffmann M: Samuel Thibault S: Maintained F: hw/usb/dev-serial.c -- 2.45.0 Reviewed-by: Manos Pitsidianakis
Re: [PATCH 3/4] MAINTAINERS: drop virtio-gpu maintainership
On Thu, 16 May 2024 15:03, Gerd Hoffmann wrote: Remove myself from virtio-gpu entries. Flip status to "Orphan" for entries which have nobody else listed. Signed-off-by: Gerd Hoffmann --- MAINTAINERS | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index d81376f84746..4d9f4fd09823 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2572,8 +2572,7 @@ F: hw/display/ramfb*.c F: include/hw/display/ramfb.h virtio-gpu -M: Gerd Hoffmann -S: Odd Fixes +S: Orphan F: hw/display/virtio-gpu* F: hw/display/virtio-vga.* F: include/hw/virtio/virtio-gpu.h @@ -2595,7 +2594,6 @@ F: include/hw/virtio/virtio-blk-common.h vhost-user-gpu M: Marc-André Lureau -R: Gerd Hoffmann S: Maintained F: docs/interop/vhost-user-gpu.rst F: contrib/vhost-user-gpu -- 2.45.0 Reviewed-by: Manos Pitsidianakis
Re: [PATCH 4/4] MAINTAINERS: drop spice+ui maintainership
On Thu, 16 May 2024 15:03, Gerd Hoffmann wrote: Remove myself from spice and ui entries. Flip status to "Orphan" for entries which have nobody else listed. Signed-off-by: Gerd Hoffmann --- MAINTAINERS | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 4d9f4fd09823..d5b6a1c76abb 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3042,8 +3042,7 @@ F: stubs/memory_device.c F: docs/nvdimm.txt SPICE -M: Gerd Hoffmann -S: Odd Fixes +S: Orphan F: include/ui/qemu-spice.h F: include/ui/spice-display.h F: ui/spice-*.c @@ -3053,7 +3052,6 @@ F: qapi/ui.json F: docs/spice-port-fqdn.txt Graphics -M: Gerd Hoffmann M: Marc-André Lureau S: Odd Fixes F: ui/ -- 2.45.0 Reviewed-by: Manos Pitsidianakis
Re: [PATCH v12 13/13] virtio-gpu: Support Venus context
On Mon, 20 May 2024 00:27, Dmitry Osipenko
wrote:
From: Antonio Caggiano
Request Venus when initializing VirGL and if venus=true flag is set for
virtio-gpu-gl device.
Signed-off-by: Antonio Caggiano
Signed-off-by: Huang Rui
Signed-off-by: Dmitry Osipenko
---
hw/display/virtio-gpu-gl.c | 2 ++
hw/display/virtio-gpu-virgl.c | 22 ++
hw/display/virtio-gpu.c| 13 +
include/hw/virtio/virtio-gpu.h | 3 +++
meson.build| 1 +
5 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index b8f395be8d2d..2078e74050bb 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -148,6 +148,8 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev,
Error **errp)
static Property virtio_gpu_gl_properties[] = {
DEFINE_PROP_BIT("stats", VirtIOGPU, parent_obj.conf.flags,
VIRTIO_GPU_FLAG_STATS_ENABLED, false),
+DEFINE_PROP_BIT("venus", VirtIOGPU, parent_obj.conf.flags,
+VIRTIO_GPU_FLAG_VENUS_ENABLED, false),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 70e2d28ba966..2e9862dd186a 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -1130,6 +1130,11 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
flags |= VIRGL_RENDERER_D3D11_SHARE_TEXTURE;
}
#endif
+#ifdef VIRGL_RENDERER_VENUS
+if (virtio_gpu_venus_enabled(g->parent_obj.conf)) {
+flags |= VIRGL_RENDERER_VENUS | VIRGL_RENDERER_RENDER_SERVER;
+}
+#endif
ret = virgl_renderer_init(g, flags, _gpu_3d_cbs);
if (ret != 0) {
@@ -1161,7 +1166,7 @@ static void virtio_gpu_virgl_add_capset(GArray
*capset_ids, uint32_t capset_id)
GArray *virtio_gpu_virgl_get_capsets(VirtIOGPU *g)
{
-uint32_t capset2_max_ver, capset2_max_size;
+uint32_t capset_max_ver, capset_max_size;
GArray *capset_ids;
capset_ids = g_array_new(false, false, sizeof(uint32_t));
@@ -1170,12 +1175,21 @@ GArray *virtio_gpu_virgl_get_capsets(VirtIOGPU *g)
virtio_gpu_virgl_add_capset(capset_ids, VIRTIO_GPU_CAPSET_VIRGL);
virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VIRGL2,
- _max_ver,
- _max_size);
-if (capset2_max_ver) {
+ _max_ver,
+ _max_size);
+if (capset_max_ver) {
virtio_gpu_virgl_add_capset(capset_ids, VIRTIO_GPU_CAPSET_VIRGL2);
}
+if (virtio_gpu_venus_enabled(g->parent_obj.conf)) {
+virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VENUS,
+ _max_ver,
+ _max_size);
+if (capset_max_size) {
+virtio_gpu_virgl_add_capset(capset_ids, VIRTIO_GPU_CAPSET_VENUS);
+}
+}
+
return capset_ids;
}
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 052ab493a00b..0518bb858e88 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1491,6 +1491,19 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error
**errp)
#endif
}
+if (virtio_gpu_venus_enabled(g->parent_obj.conf)) {
+#ifdef HAVE_VIRGL_VENUS
+if (!virtio_gpu_blob_enabled(g->parent_obj.conf) ||
+!virtio_gpu_hostmem_enabled(g->parent_obj.conf)) {
+error_setg(errp, "venus requires enabled blob and hostmem
options");
+return;
+}
+#else
+error_setg(errp, "old virglrenderer, venus unsupported");
+return;
+#endif
+}
+
if (!virtio_gpu_base_device_realize(qdev,
virtio_gpu_handle_ctrl_cb,
virtio_gpu_handle_cursor_cb,
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 7e1fee836802..ec5d7517f141 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -99,6 +99,7 @@ enum virtio_gpu_base_conf_flags {
VIRTIO_GPU_FLAG_BLOB_ENABLED,
VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED,
VIRTIO_GPU_FLAG_RUTABAGA_ENABLED,
+VIRTIO_GPU_FLAG_VENUS_ENABLED,
};
#define virtio_gpu_virgl_enabled(_cfg) \
@@ -117,6 +118,8 @@ enum virtio_gpu_base_conf_flags {
(_cfg.flags & (1 << VIRTIO_GPU_FLAG_RUTABAGA_ENABLED))
#define virtio_gpu_hostmem_enabled(_cfg) \
(_cfg.hostmem > 0)
+#define virtio_gpu_venus_enabled(_cfg) \
+(_cfg.flags & (1 << VIRTIO_GPU_FLAG_VENUS_ENABLED))
Can we have both venus and rutabaga enabled on the same virtio-gpu
device? How would that work? It seems to me they should be mutually
exclusive.
struct virtio_gpu_base_conf {
uint32_t max_outputs;
diff --git a/meson.build b/meson.build
index 503a7736eda0..5a2b7b660c67 100644
--- a/meson.build
+++ b/meson.build
@@ -2305,6 +2305,7 @@ if virgl.version().version_compare('>=1.0.0')
config_host_data.set('HAVE_VIRGL_D3D_INFO_EXT', 1)
Re: [PATCH v12 12/13] virtio-gpu: Register capsets dynamically
On Mon, 20 May 2024 00:27, Dmitry Osipenko
wrote:
From: Pierre-Eric Pelloux-Prayer
virtio_gpu_virgl_get_num_capsets will return "num_capsets", but we can't
assume that capset_index 1 is always VIRGL2 once we'll support more capsets,
like Venus and DRM capsets. Register capsets dynamically to avoid that problem.
Signed-off-by: Pierre-Eric Pelloux-Prayer
Signed-off-by: Dmitry Osipenko
---
hw/display/virtio-gpu-gl.c | 6 --
hw/display/virtio-gpu-virgl.c | 33 +
include/hw/virtio/virtio-gpu.h | 4 +++-
3 files changed, 28 insertions(+), 15 deletions(-)
diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index 4d0a10070ab3..b8f395be8d2d 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -135,8 +135,8 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev,
Error **errp)
}
g->parent_obj.conf.flags |= (1 << VIRTIO_GPU_FLAG_VIRGL_ENABLED);
-VIRTIO_GPU_BASE(g)->virtio_config.num_capsets =
-virtio_gpu_virgl_get_num_capsets(g);
+g->capset_ids = virtio_gpu_virgl_get_capsets(g);
+VIRTIO_GPU_BASE(g)->virtio_config.num_capsets = g->capset_ids->len;
#ifdef HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS
g->parent_obj.conf.flags |= 1 << VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED;
@@ -159,6 +159,8 @@ static void virtio_gpu_gl_device_unrealize(DeviceState
*qdev)
if (gl->renderer_inited) {
virtio_gpu_virgl_deinit(g);
}
+
+g_array_unref(g->capset_ids);
}
static void virtio_gpu_gl_class_init(ObjectClass *klass, void *data)
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index a41c4f8e1cef..70e2d28ba966 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -623,19 +623,13 @@ static void virgl_cmd_get_capset_info(VirtIOGPU *g,
VIRTIO_GPU_FILL_CMD(info);
memset(, 0, sizeof(resp));
-if (info.capset_index == 0) {
-resp.capset_id = VIRTIO_GPU_CAPSET_VIRGL;
-virgl_renderer_get_cap_set(resp.capset_id,
- _max_version,
- _max_size);
-} else if (info.capset_index == 1) {
-resp.capset_id = VIRTIO_GPU_CAPSET_VIRGL2;
+
+if (info.capset_index < g->capset_ids->len) {
+resp.capset_id = g_array_index(g->capset_ids, uint32_t,
+ info.capset_index);
virgl_renderer_get_cap_set(resp.capset_id,
_max_version,
_max_size);
-} else {
-resp.capset_max_version = 0;
-resp.capset_max_size = 0;
}
resp.hdr.type = VIRTIO_GPU_RESP_OK_CAPSET_INFO;
virtio_gpu_ctrl_response(g, cmd, , sizeof(resp));
@@ -1160,14 +1154,29 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
return 0;
}
-int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
+static void virtio_gpu_virgl_add_capset(GArray *capset_ids, uint32_t capset_id)
+{
+g_array_append_val(capset_ids, capset_id);
+}
+
+GArray *virtio_gpu_virgl_get_capsets(VirtIOGPU *g)
{
uint32_t capset2_max_ver, capset2_max_size;
+GArray *capset_ids;
+
+capset_ids = g_array_new(false, false, sizeof(uint32_t));
+
+/* VIRGL is always supported. */
+virtio_gpu_virgl_add_capset(capset_ids, VIRTIO_GPU_CAPSET_VIRGL);
+
virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VIRGL2,
_max_ver,
_max_size);
+if (capset2_max_ver) {
+virtio_gpu_virgl_add_capset(capset_ids, VIRTIO_GPU_CAPSET_VIRGL2);
+}
-return capset2_max_ver ? 2 : 1;
+return capset_ids;
}
void virtio_gpu_virgl_deinit(VirtIOGPU *g)
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index aea559cdacc5..7e1fee836802 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -208,6 +208,8 @@ struct VirtIOGPU {
QTAILQ_HEAD(, VGPUDMABuf) bufs;
VGPUDMABuf *primary[VIRTIO_GPU_MAX_SCANOUTS];
} dmabuf;
+
+GArray *capset_ids;
};
struct VirtIOGPUClass {
@@ -347,6 +349,6 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g);
void virtio_gpu_virgl_reset(VirtIOGPU *g);
int virtio_gpu_virgl_init(VirtIOGPU *g);
void virtio_gpu_virgl_deinit(VirtIOGPU *g);
-int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g);
+GArray *virtio_gpu_virgl_get_capsets(VirtIOGPU *g);
#endif
--
2.44.0
Reviewed-by: Manos Pitsidianakis
Re: [PATCH 1/4] MAINTAINERS: drop audio maintainership
On Wed, 22 May 2024 at 15:54, Thomas Huth wrote: > > On 16/05/2024 14.03, Gerd Hoffmann wrote: > > Remove myself from audio (both devices and backend) entries. > > Flip status to "Orphan" for entries which have nobody else listed. > > > > Signed-off-by: Gerd Hoffmann > > --- > > MAINTAINERS | 19 --- > > 1 file changed, 4 insertions(+), 15 deletions(-) > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 1b79767d6196..7f52e2912fc3 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > ... > > @@ -2388,7 +2387,6 @@ F: hw/virtio/virtio-mem-pci.c > > F: include/hw/virtio/virtio-mem.h > > > > virtio-snd > > -M: Gerd Hoffmann > > R: Manos Pitsidianakis > > S: Supported > > I think the status should be downgraded to Orphan or at least Odd-fixes, > unless Manos wants to upgrade from "R:" to "M:" ? That's fine with me. > > ALSA Audio backend > > -M: Gerd Hoffmann > > R: Christian Schoenebeck > > S: Odd Fixes > > F: audio/alsaaudio.c > > I'd also suggest that Christian either upgrade from R: to M: or that we > change the status to Orphan If Christian is not available I volunteer to be a Reviewer (but not M:) since I have some familiarity with alsaaudio.c This way even if Orphan it will have more reviewers available. > > > JACK Audio Connection Kit backend > > -M: Gerd Hoffmann > > R: Christian Schoenebeck > > S: Odd Fixes > > F: audio/jackaudio.c > > dito > > > SDL Audio backend > > -M: Gerd Hoffmann > > R: Thomas Huth > > I'm fine if you update my entry from R: to M: here. > > > S: Odd Fixes > > F: audio/sdlaudio.c > > > > Sndio Audio backend > > -M: Gerd Hoffmann > > R: Alexandre Ratchov > > S: Odd Fixes > > F: audio/sndioaudio.c > > Same again, I'd suggest to either set to Orphan or upgrade the R: entry? > > Thomas >
Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one
On Thu, 25 Apr 2024 at 13:24, Michael S. Tsirkin wrote:
>
> On Thu, Apr 25, 2024 at 01:04:31PM +0300, Manos Pitsidianakis wrote:
> > On Thu, 25 Apr 2024 at 10:49, Mark Cave-Ayland
> > wrote:
> > >
> > > On 25/04/2024 07:30, Manos Pitsidianakis wrote:
> > >
> > > > On Wed, 24 Apr 2024 at 13:31, Mark Cave-Ayland
> > > > wrote:
> > > >>
> > > >> On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:
> > > >>
> > > >>> On 23/4/24 11:18, Manos Pitsidianakis wrote:
> > > >>>> On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
> > > >>>> wrote:
> > > >>>>>
> > > >>>>> On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin
> > > >>>>> wrote:
> > > >>>>>>
> > > >>>>>> On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé
> > > >>>>>> wrote:
> > > >>>>>>> On 22/4/24 23:02, Michael S. Tsirkin wrote:
> > > >>>>>>>> On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé
> > > >>>>>>>> wrote:
> > > >>>>>>>>> Since VirtIO devices can change endianness at runtime,
> > > >>>>>>>>> we need to use the device endianness, not the target
> > > >>>>>>>>> one.
> > > >>>>>>>>>
> > > >>>>>>>>> Cc: qemu-sta...@nongnu.org
> > > >>>>>>>>> Fixes: eb9ad377bb ("virtio-sound: handle control messages and
> > > >>>>>>>>> streams")
> > > >>>>>>>>> Signed-off-by: Philippe Mathieu-Daudé
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>> This is all completely bogus. Virtio SND is from Virtio 1.0 only.
> > > >>>>>>>> It is unconditionally little endian.
> > > >>>>>
> > > >>>>>
> > > >>>>> This part of the code is for PCM frames (raw bytes), not virtio spec
> > > >>>>> fields (which indeed must be LE in modern VIRTIO).
> > > >>>>
> > > >>>> Thought a little more about it. We should keep the target's
> > > >>>> endianness
> > > >>>> here, if it's mutable then we should query the machine the device is
> > > >>>> attached to somehow. the virtio device should never change endianness
> > > >>>> like Michael says since it's not legacy.
> > > >>>
> > > >>> Grr. So as Richard suggested, this need to be pass as a device
> > > >>> property then.
> > > >>> (https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09...@linaro.org/)
> > > >>
> > > >> It feels to me that the endianness is something that should be
> > > >> negotiated as part of
> > > >> the frame format, since the endianness of the audio hardware can be
> > > >> different from
> > > >> that of the CPU (think PReP machines where it was common that a big
> > > >> endian CPU is
> > > >> driving little endian hardware as found on x86).
> > > >
> > > > But that is the job of the hardware drivers, isn't it? Here we are
> > > > taking frames passed from the guest to its virtio driver in the format
> > > > specified in the target cpu's endianness and QEMU as the device passes
> > > > it to host ALSA/Pipewire/etc which in turn passes it to the actual
> > > > audio hardware driver..
> > >
> > > The problem is that the notion of target CPU endian is not fixed. For
> > > example the
> > > PowerPC CPU starts off in big-endian mode, but these days most systems
> > > will switch
> > > the CPU to little-endian mode on startup to run ppc64le. There's also the
> > > ILE bit
> > > which can be configured so that a big-endian PowerPC CPU can dynamically
> > > switch to
> > > little-endian mode when processing an interrupt, so you could potentially
> > > end up with
> > > either depending upon the current mode of the CPU.
> > >
> > > These are the kinds of issues that led to the later virtio specifications
> > > simply
> > > using little-endian for everything, since then there is zero ambiguity
> > > over what
> > > endian is required for the virtio configuration space accesses.
> > >
> > > It feels to me that assuming a target CPU endian is fixed for the PCM
> > > frame formats
> > > is simply repeating the mistakes of the past - and even the fact that we
> > > are
> > > discussing this within this thread suggests that at a very minimum the
> > > virtio-snd
> > > specification needs to be updated to clarify the byte ordering of the PCM
> > > frame formats.
> > >
> > >
> > > ATB,
> > >
> > > Mark.
> > >
> >
> >
> > Agreed, I think we are saying approximately the same thing here.
> >
> > We need a mechanism to retrieve the vCPUs endianness and a way to
> > notify subscribed devices when it changes.
>
> I don't think I agree, it's not the same thing.
> Guest should just convert and send data in LE format.
> Host should then convert from LE format.
> Target endian-ness does not come into it.
That's not in the VIRTIO 1.2 spec. We are talking about supporting
things as they currently stand, not as they could have been.
Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one
On Thu, 25 Apr 2024 at 10:49, Mark Cave-Ayland
wrote:
>
> On 25/04/2024 07:30, Manos Pitsidianakis wrote:
>
> > On Wed, 24 Apr 2024 at 13:31, Mark Cave-Ayland
> > wrote:
> >>
> >> On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:
> >>
> >>> On 23/4/24 11:18, Manos Pitsidianakis wrote:
> >>>> On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
> >>>> wrote:
> >>>>>
> >>>>> On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin
> >>>>> wrote:
> >>>>>>
> >>>>>> On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
> >>>>>>> On 22/4/24 23:02, Michael S. Tsirkin wrote:
> >>>>>>>> On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé
> >>>>>>>> wrote:
> >>>>>>>>> Since VirtIO devices can change endianness at runtime,
> >>>>>>>>> we need to use the device endianness, not the target
> >>>>>>>>> one.
> >>>>>>>>>
> >>>>>>>>> Cc: qemu-sta...@nongnu.org
> >>>>>>>>> Fixes: eb9ad377bb ("virtio-sound: handle control messages and
> >>>>>>>>> streams")
> >>>>>>>>> Signed-off-by: Philippe Mathieu-Daudé
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> This is all completely bogus. Virtio SND is from Virtio 1.0 only.
> >>>>>>>> It is unconditionally little endian.
> >>>>>
> >>>>>
> >>>>> This part of the code is for PCM frames (raw bytes), not virtio spec
> >>>>> fields (which indeed must be LE in modern VIRTIO).
> >>>>
> >>>> Thought a little more about it. We should keep the target's endianness
> >>>> here, if it's mutable then we should query the machine the device is
> >>>> attached to somehow. the virtio device should never change endianness
> >>>> like Michael says since it's not legacy.
> >>>
> >>> Grr. So as Richard suggested, this need to be pass as a device
> >>> property then.
> >>> (https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09...@linaro.org/)
> >>
> >> It feels to me that the endianness is something that should be negotiated
> >> as part of
> >> the frame format, since the endianness of the audio hardware can be
> >> different from
> >> that of the CPU (think PReP machines where it was common that a big endian
> >> CPU is
> >> driving little endian hardware as found on x86).
> >
> > But that is the job of the hardware drivers, isn't it? Here we are
> > taking frames passed from the guest to its virtio driver in the format
> > specified in the target cpu's endianness and QEMU as the device passes
> > it to host ALSA/Pipewire/etc which in turn passes it to the actual
> > audio hardware driver..
>
> The problem is that the notion of target CPU endian is not fixed. For example
> the
> PowerPC CPU starts off in big-endian mode, but these days most systems will
> switch
> the CPU to little-endian mode on startup to run ppc64le. There's also the ILE
> bit
> which can be configured so that a big-endian PowerPC CPU can dynamically
> switch to
> little-endian mode when processing an interrupt, so you could potentially end
> up with
> either depending upon the current mode of the CPU.
>
> These are the kinds of issues that led to the later virtio specifications
> simply
> using little-endian for everything, since then there is zero ambiguity over
> what
> endian is required for the virtio configuration space accesses.
>
> It feels to me that assuming a target CPU endian is fixed for the PCM frame
> formats
> is simply repeating the mistakes of the past - and even the fact that we are
> discussing this within this thread suggests that at a very minimum the
> virtio-snd
> specification needs to be updated to clarify the byte ordering of the PCM
> frame formats.
>
>
> ATB,
>
> Mark.
>
Agreed, I think we are saying approximately the same thing here.
We need a mechanism to retrieve the vCPUs endianness and a way to
notify subscribed devices when it changes.
I think that then, since the virtio device is mostly certain of the
correct target endianness and completely certain of the host
endianness, it can perform the necessary conversions.
I don't recall seeing a restriction on the byte ordering of PCM
formats other than the CPU order; except for the ones which have
explicit endianness in their definitions. Please correct me if I am
wrong!
A straightforward solution would be to set an endianness change notify
callback in DeviceClass. What do you think?
--
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd
Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one
On Wed, 24 Apr 2024 at 13:31, Mark Cave-Ayland
wrote:
>
> On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:
>
> > On 23/4/24 11:18, Manos Pitsidianakis wrote:
> >> On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
> >> wrote:
> >>>
> >>> On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin wrote:
> >>>>
> >>>> On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
> >>>>> On 22/4/24 23:02, Michael S. Tsirkin wrote:
> >>>>>> On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
> >>>>>>> Since VirtIO devices can change endianness at runtime,
> >>>>>>> we need to use the device endianness, not the target
> >>>>>>> one.
> >>>>>>>
> >>>>>>> Cc: qemu-sta...@nongnu.org
> >>>>>>> Fixes: eb9ad377bb ("virtio-sound: handle control messages and
> >>>>>>> streams")
> >>>>>>> Signed-off-by: Philippe Mathieu-Daudé
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> This is all completely bogus. Virtio SND is from Virtio 1.0 only.
> >>>>>> It is unconditionally little endian.
> >>>
> >>>
> >>> This part of the code is for PCM frames (raw bytes), not virtio spec
> >>> fields (which indeed must be LE in modern VIRTIO).
> >>
> >> Thought a little more about it. We should keep the target's endianness
> >> here, if it's mutable then we should query the machine the device is
> >> attached to somehow. the virtio device should never change endianness
> >> like Michael says since it's not legacy.
> >
> > Grr. So as Richard suggested, this need to be pass as a device
> > property then.
> > (https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09...@linaro.org/)
>
> It feels to me that the endianness is something that should be negotiated as
> part of
> the frame format, since the endianness of the audio hardware can be different
> from
> that of the CPU (think PReP machines where it was common that a big endian
> CPU is
> driving little endian hardware as found on x86).
But that is the job of the hardware drivers, isn't it? Here we are
taking frames passed from the guest to its virtio driver in the format
specified in the target cpu's endianness and QEMU as the device passes
it to host ALSA/Pipewire/etc which in turn passes it to the actual
audio hardware driver..
> My feeling is that either the VIRTIO_SND_PCM_FMT_* constants should be
> extended to
> have both _LE and _BE variants, or all frame formats are defined to always be
> little
> endian.
>
>
> ATB,
>
> Mark.
>
Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one
On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
wrote:
>
> On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin wrote:
> >
> > On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
> > > On 22/4/24 23:02, Michael S. Tsirkin wrote:
> > > > On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
> > > > > Since VirtIO devices can change endianness at runtime,
> > > > > we need to use the device endianness, not the target
> > > > > one.
> > > > >
> > > > > Cc: qemu-sta...@nongnu.org
> > > > > Fixes: eb9ad377bb ("virtio-sound: handle control messages and
> > > > > streams")
> > > > > Signed-off-by: Philippe Mathieu-Daudé
> > > >
> > > >
> > > >
> > > > This is all completely bogus. Virtio SND is from Virtio 1.0 only.
> > > > It is unconditionally little endian.
>
>
> This part of the code is for PCM frames (raw bytes), not virtio spec
> fields (which indeed must be LE in modern VIRTIO).
Thought a little more about it. We should keep the target's endianness
here, if it's mutable then we should query the machine the device is
attached to somehow. the virtio device should never change endianness
like Michael says since it's not legacy.
Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one
On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin wrote:
>
> On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
> > On 22/4/24 23:02, Michael S. Tsirkin wrote:
> > > On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
> > > > Since VirtIO devices can change endianness at runtime,
> > > > we need to use the device endianness, not the target
> > > > one.
> > > >
> > > > Cc: qemu-sta...@nongnu.org
> > > > Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
> > > > Signed-off-by: Philippe Mathieu-Daudé
> > >
> > >
> > >
> > > This is all completely bogus. Virtio SND is from Virtio 1.0 only.
> > > It is unconditionally little endian.
This part of the code is for PCM frames (raw bytes), not virtio spec
fields (which indeed must be LE in modern VIRTIO).
Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one
On Mon, 22 Apr 2024 17:20, Philippe Mathieu-Daudé
wrote:
Since VirtIO devices can change endianness at runtime,
we need to use the device endianness, not the target
one.
Cc: qemu-sta...@nongnu.org
Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
Signed-off-by: Philippe Mathieu-Daudé
---
Reviewed-by: Manos Pitsidianakis
Thanks for the explanation on v2 btw. virtio_is_big_endian()'s doc
should probably reflect it's not just about legacy devices (virtio sound
isn't legacy) but about target originating data streams too
Re: [PATCH v2] hw/audio/virtio-snd: Use device endianness instead of target one
On Mon, 22 Apr 2024 16:13, Philippe Mathieu-Daudé wrote:
Since VirtIO devices can change endianness at runtime,
we need to use the device endianness, not the target
one.
Hey Philippe, can you clarify what do you mean by they can change
endianness at runtime?
The target's one is used because that's the one it will be using to do
I/O with its kernel's audio interface.
Cc: qemu-sta...@nongnu.org
Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
Signed-off-by: Philippe Mathieu-Daudé
---
hw/audio/virtio-snd.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index c80b58bf5d..82c5647660 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -395,13 +395,15 @@ static uint32_t virtio_snd_get_qemu_freq(uint32_t rate)
* Get QEMU Audiosystem compatible audsettings from virtio based pcm stream
* params.
*/
-static void virtio_snd_get_qemu_audsettings(audsettings *as,
+static void virtio_snd_get_qemu_audsettings(VirtIOSound *s, audsettings *as,
virtio_snd_pcm_set_params *params)
{
+VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
as->nchannels = MIN(AUDIO_MAX_CHANNELS, params->channels);
as->fmt = virtio_snd_get_qemu_format(params->format);
as->freq = virtio_snd_get_qemu_freq(params->rate);
-as->endianness = target_words_bigendian() ? 1 : 0;
+as->endianness = virtio_is_big_endian(vdev) ? 1 : 0;
}
/*
@@ -464,7 +466,7 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s,
uint32_t stream_id)
s->pcm->streams[stream_id] = stream;
}
-virtio_snd_get_qemu_audsettings(, params);
+virtio_snd_get_qemu_audsettings(s, , params);
stream->info.direction = stream_id < s->snd_conf.streams / 2 +
(s->snd_conf.streams & 1) ? VIRTIO_SND_D_OUTPUT : VIRTIO_SND_D_INPUT;
stream->info.hdr.hda_fn_nid = VIRTIO_SOUND_HDA_FN_NID;
--
2.41.0
[PATCH v1 3/4] virtio-snd: factor card removal out of unrealize()
Extract audio card removal logic out of the device unrealize callback so
that it can be re-used in follow up commits.
Signed-off-by: Manos Pitsidianakis
---
hw/audio/virtio-snd.c | 20 ++--
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index 82dd320ebe..a9cfaea046 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -1343,15 +1343,11 @@ static inline void
virtio_snd_pcm_flush(VirtIOSoundPCMStream *stream)
}
}
-static void virtio_snd_unrealize(DeviceState *dev)
+/* Remove audio card and cleanup streams. */
+static void virtio_snd_unsetup(VirtIOSound *vsnd)
{
-VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-VirtIOSound *vsnd = VIRTIO_SND(dev);
VirtIOSoundPCMStream *stream;
-qemu_del_vm_change_state_handler(vsnd->vmstate);
-trace_virtio_snd_unrealize(vsnd);
-
if (vsnd->pcm) {
if (vsnd->pcm->streams) {
for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
@@ -1370,6 +1366,18 @@ static void virtio_snd_unrealize(DeviceState *dev)
vsnd->pcm = NULL;
}
AUD_remove_card(>card);
+}
+
+static void virtio_snd_unrealize(DeviceState *dev)
+{
+VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+VirtIOSound *vsnd = VIRTIO_SND(dev);
+
+qemu_del_vm_change_state_handler(vsnd->vmstate);
+trace_virtio_snd_unrealize(vsnd);
+
+virtio_snd_unsetup(vsnd);
+
qemu_mutex_destroy(>cmdq_mutex);
virtio_delete_queue(vsnd->queues[VIRTIO_SND_VQ_CONTROL]);
virtio_delete_queue(vsnd->queues[VIRTIO_SND_VQ_EVENT]);
--
γαῖα πυρί μιχθήτω
[PATCH v1 2/4] virtio-snd: factor card setup out of realize func
Extract audio card setup logic out of the device realize callback so
that it can be re-used in follow up commits.
Signed-off-by: Manos Pitsidianakis
---
hw/audio/virtio-snd.c | 72 ---
1 file changed, 41 insertions(+), 31 deletions(-)
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index 7ca9ed251c..82dd320ebe 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -1073,27 +1073,21 @@ virtio_snd_is_config_valid(virtio_snd_config snd_conf,
Error **errp)
return true;
}
-static void virtio_snd_realize(DeviceState *dev, Error **errp)
+/* Registers card and sets up streams according to configuration. */
+static bool virtio_snd_setup(VirtIOSound *vsnd, Error **errp)
{
ERRP_GUARD();
-VirtIOSound *vsnd = VIRTIO_SND(dev);
-VirtIODevice *vdev = VIRTIO_DEVICE(dev);
virtio_snd_pcm_set_params default_params = { 0 };
uint32_t status;
-trace_virtio_snd_realize(vsnd);
-
if (!virtio_snd_is_config_valid(vsnd->snd_conf, errp)) {
-return;
+return false;
}
if (!AUD_register_card("virtio-sound", >card, errp)) {
-return;
+return false;
}
-vsnd->vmstate =
-qemu_add_vm_change_state_handler(virtio_snd_vm_state_change, vsnd);
-
vsnd->pcm = g_new0(VirtIOSoundPCM, 1);
vsnd->pcm->snd = vsnd;
vsnd->pcm->streams =
@@ -1101,9 +1095,6 @@ static void virtio_snd_realize(DeviceState *dev, Error
**errp)
vsnd->pcm->pcm_params =
g_new0(virtio_snd_pcm_set_params, vsnd->snd_conf.streams);
-virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config));
-virtio_add_feature(>features, VIRTIO_F_VERSION_1);
-
/* set default params for all streams */
default_params.features = 0;
default_params.buffer_bytes = cpu_to_le32(8192);
@@ -,6 +1102,41 @@ static void virtio_snd_realize(DeviceState *dev, Error
**errp)
default_params.channels = 2;
default_params.format = VIRTIO_SND_PCM_FMT_S16;
default_params.rate = VIRTIO_SND_PCM_RATE_48000;
+
+for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
+status = virtio_snd_set_pcm_params(vsnd, i, _params);
+if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
+error_setg(errp,
+ "Can't initialize stream params, device responded with
%s.",
+ print_code(status));
+return false;
+}
+status = virtio_snd_pcm_prepare(vsnd, i);
+if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
+error_setg(errp,
+ "Can't prepare streams, device responded with %s.",
+ print_code(status));
+return false;
+}
+}
+
+return true;
+}
+
+static void virtio_snd_realize(DeviceState *dev, Error **errp)
+{
+ERRP_GUARD();
+VirtIOSound *vsnd = VIRTIO_SND(dev);
+VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+
+trace_virtio_snd_realize(vsnd);
+
+vsnd->vmstate =
+qemu_add_vm_change_state_handler(virtio_snd_vm_state_change, vsnd);
+
+virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config));
+virtio_add_feature(>features, VIRTIO_F_VERSION_1);
+
vsnd->queues[VIRTIO_SND_VQ_CONTROL] =
virtio_add_queue(vdev, 64, virtio_snd_handle_ctrl);
vsnd->queues[VIRTIO_SND_VQ_EVENT] =
@@ -1123,26 +1149,10 @@ static void virtio_snd_realize(DeviceState *dev, Error
**errp)
QTAILQ_INIT(>cmdq);
QSIMPLEQ_INIT(>invalid);
-for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
-status = virtio_snd_set_pcm_params(vsnd, i, _params);
-if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
-error_setg(errp,
- "Can't initialize stream params, device responded with
%s.",
- print_code(status));
-goto error_cleanup;
-}
-status = virtio_snd_pcm_prepare(vsnd, i);
-if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
-error_setg(errp,
- "Can't prepare streams, device responded with %s.",
- print_code(status));
-goto error_cleanup;
-}
+if (virtio_snd_setup(vsnd, errp)) {
+return;
}
-return;
-
-error_cleanup:
virtio_snd_unrealize(dev);
}
--
γαῖα πυρί μιχθήτω
[PATCH v1 4/4] virtio_snd_set_config: validate and re-setup card
Validate new configuration values and re-setup audio card.
Changing the number of streams via virtio_snd_set_config() did not
re-configure the audio card, leaving it in an invalid state. This can be
demonstrated by this heap buffer overflow:
```shell
cat << EOF | qemu-system-x86_64 -display none \
-machine accel=qtest -m 512M -machine q35 -device \
virtio-sound,audiodev=my_audiodev,streams=2 -audiodev \
alsa,id=my_audiodev -qtest stdio
outl 0xcf8 0x80001804
outw 0xcfc 0x06
outl 0xcf8 0x80001820
outl 0xcfc 0xe0008000
write 0xe0008016 0x1 0x03
write 0xe0008020 0x4 0x00901000
write 0xe0008028 0x4 0x00a01000
write 0xe000801c 0x1 0x01
write 0xe000a004 0x1 0x40
write 0x10c000 0x1 0x02
write 0x109001 0x1 0xc0
write 0x109002 0x1 0x10
write 0x109008 0x1 0x04
write 0x10a002 0x1 0x01
write 0xe000b00d 0x1 0x00
EOF
```
When built with `--enable-sanitizers`, QEMU prints this error:
ERROR: AddressSanitizer: heap-buffer-overflow [..snip..] in
virtio_snd_handle_rx_xfer().
Closes #2296.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2296
Reported-by: Zheyu Ma
Suggested-by: Zheyu Ma
Signed-off-by: Manos Pitsidianakis
---
hw/audio/virtio-snd.c | 35 +--
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index a9cfaea046..d47af2ed69 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -36,7 +36,11 @@ static void virtio_snd_pcm_out_cb(void *data, int available);
static void virtio_snd_process_cmdq(VirtIOSound *s);
static void virtio_snd_pcm_flush(VirtIOSoundPCMStream *stream);
static void virtio_snd_pcm_in_cb(void *data, int available);
+static bool virtio_snd_setup(VirtIOSound *vsnd, Error **errp);
+static void virtio_snd_unsetup(VirtIOSound *vsnd);
static void virtio_snd_unrealize(DeviceState *dev);
+static bool virtio_snd_is_config_valid(virtio_snd_config snd_conf,
+ Error **errp);
static uint32_t supported_formats = BIT(VIRTIO_SND_PCM_FMT_S8)
| BIT(VIRTIO_SND_PCM_FMT_U8)
@@ -111,23 +115,26 @@ static void
virtio_snd_set_config(VirtIODevice *vdev, const uint8_t *config)
{
VirtIOSound *s = VIRTIO_SND(vdev);
-const virtio_snd_config *sndconfig =
-(const virtio_snd_config *)config;
+virtio_snd_config new_value =
+*(const virtio_snd_config *)config;
+le32_to_cpus(_value.jacks);
+le32_to_cpus(_value.streams);
+le32_to_cpus(_value.chmaps);
- trace_virtio_snd_set_config(vdev,
- s->snd_conf.jacks,
- sndconfig->jacks,
- s->snd_conf.streams,
- sndconfig->streams,
- s->snd_conf.chmaps,
- sndconfig->chmaps);
-
-memcpy(>snd_conf, sndconfig, sizeof(virtio_snd_config));
-le32_to_cpus(>snd_conf.jacks);
-le32_to_cpus(>snd_conf.streams);
-le32_to_cpus(>snd_conf.chmaps);
+trace_virtio_snd_set_config(vdev,
+s->snd_conf.jacks,
+new_value.jacks,
+s->snd_conf.streams,
+new_value.streams,
+s->snd_conf.chmaps,
+new_value.chmaps);
+if (virtio_snd_is_config_valid(new_value, _warn)) {
+virtio_snd_unsetup(s);
+s->snd_conf = new_value;
+virtio_snd_setup(s, _fatal);
+}
}
static void
--
γαῖα πυρί μιχθήτω
[PATCH v1 1/4] virtio-snd: add virtio_snd_is_config_valid()
Factor out virtio_snd_config value validation in a separate function, in
order to re-use it in follow up commits.
Signed-off-by: Manos Pitsidianakis
---
hw/audio/virtio-snd.c | 47 ++-
1 file changed, 29 insertions(+), 18 deletions(-)
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index c80b58bf5d..7ca9ed251c 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -1045,6 +1045,34 @@ virtio_snd_vm_state_change(void *opaque, bool running,
}
}
+static bool
+virtio_snd_is_config_valid(virtio_snd_config snd_conf, Error **errp)
+{
+if (snd_conf.jacks > 8) {
+error_setg(errp,
+ "Invalid number of jacks: %"PRIu32
+ ": maximum value is 8", snd_conf.jacks);
+return false;
+}
+if (snd_conf.streams < 1 || snd_conf.streams > 64) {
+error_setg(errp,
+ "Invalid number of streams: %"PRIu32
+ ": minimum value is 1, maximum value is 64",
+ snd_conf.streams);
+return false;
+}
+
+if (snd_conf.chmaps > VIRTIO_SND_CHMAP_MAX_SIZE) {
+error_setg(errp,
+ "Invalid number of channel maps: %"PRIu32
+ ": VIRTIO v1.2 sets the maximum value at %"PRIu32,
+ snd_conf.chmaps, VIRTIO_SND_CHMAP_MAX_SIZE);
+return false;
+}
+
+return true;
+}
+
static void virtio_snd_realize(DeviceState *dev, Error **errp)
{
ERRP_GUARD();
@@ -1055,24 +1083,7 @@ static void virtio_snd_realize(DeviceState *dev, Error
**errp)
trace_virtio_snd_realize(vsnd);
-/* check number of jacks and streams */
-if (vsnd->snd_conf.jacks > 8) {
-error_setg(errp,
- "Invalid number of jacks: %"PRIu32,
- vsnd->snd_conf.jacks);
-return;
-}
-if (vsnd->snd_conf.streams < 1 || vsnd->snd_conf.streams > 10) {
-error_setg(errp,
- "Invalid number of streams: %"PRIu32,
-vsnd->snd_conf.streams);
-return;
-}
-
-if (vsnd->snd_conf.chmaps > VIRTIO_SND_CHMAP_MAX_SIZE) {
-error_setg(errp,
- "Invalid number of channel maps: %"PRIu32,
- vsnd->snd_conf.chmaps);
+if (!virtio_snd_is_config_valid(vsnd->snd_conf, errp)) {
return;
}
--
γαῖα πυρί μιχθήτω
[PATCH v1 0/4] virtio_snd_set_config: Fix #2296
Changing the number of streams via virtio_snd_set_config() did not re-configure the audio card, leaving it in an invalid state. Reported in https://gitlab.com/qemu-project/qemu/-/issues/2296 Manos Pitsidianakis (4): virtio-snd: add virtio_snd_is_config_valid() virtio-snd: factor card setup out of realize func virtio-snd: factor card removal out of unrealize() virtio_snd_set_config: validate and re-setup card hw/audio/virtio-snd.c | 174 +- 1 file changed, 105 insertions(+), 69 deletions(-) base-commit: 62dbe54c24dbf77051bafe1039c31ddc8f37602d -- γαῖα πυρί μιχθήτω
[PATCH] docs/devel: fix minor typo in submitting-a-patch.rst
s/Resolved:/Resolves:/ Cc: qemu-triv...@nongnu.org Signed-off-by: Manos Pitsidianakis --- docs/devel/submitting-a-patch.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst index c641d948f1..83e9092b8c 100644 --- a/docs/devel/submitting-a-patch.rst +++ b/docs/devel/submitting-a-patch.rst @@ -177,7 +177,7 @@ add an additional line with "Fixes: If your patch fixes a bug in the gitlab bug tracker, please add a line with "Resolves: " to the commit message, too. Gitlab can -close bugs automatically once commits with the "Resolved:" keyword get +close bugs automatically once commits with the "Resolves:" keyword get merged into the master branch of the project. And if your patch addresses a bug in another public bug tracker, you can also use a line with "Buglink: " for reference here, too. base-commit: 62dbe54c24dbf77051bafe1039c31ddc8f37602d -- γαῖα πυρί μιχθήτω
Re: [PATCH v1 0/1] virtio-snd: fix invalid tx/rx message handling logic
On Fri, 5 Apr 2024 at 14:07, Michael S. Tsirkin wrote: > > On Fri, Apr 05, 2024 at 01:54:46PM +0300, Manos Pitsidianakis wrote: > > ping > > confused at this point. > Do you mind sending a patchset with everything in the correct order? > Tag it PATCH repost so people know nothing changed. > Thanks! Might not have access to my desktop for a while, so until then let me know if a resend is still necessary. The patches are two, in this order: 1. <20240322110827.568412-1-zheyum...@gmail.com> 2. Thanks, -- Manos Pitsidianakis Emulation and Virtualization Engineer at Linaro Ltd
Re: [PATCH v1 0/1] virtio-snd: fix invalid tx/rx message handling logic
ping On Sun, 24 Mar 2024 12:04, Manos Pitsidianakis wrote: This is a logic fix for the error handling in the TX/RX virt queue handlers. A potential invalid address dereference was reported and fixed by Zheyu Ma in <20240322110827.568412-1-zheyum...@gmail.com>. This patch moves the invalid message storage from the stream structs to the virtio device struct to 1. make such bug impossible 2. reply to invalid messages again with VIRTIO_SND_S_BAD_MSG, which was not possible before. Patch based on master base-commit: 853546f8128476eefb701d4a55b2781bb3a46faa with the following patch applied: Subject: [PATCH v2] virtio-snd: Enhance error handling for invalid transfers From: Zheyu Ma Date: Fri, 22 Mar 2024 12:08:27 +0100 Message-Id: <20240322110827.568412-1-zheyum...@gmail.com> Manos Pitsidianakis (1): virtio-snd: rewrite invalid tx/rx message handling include/hw/audio/virtio-snd.h | 16 +++- hw/audio/virtio-snd.c | 137 +++--- 2 files changed, 77 insertions(+), 76 deletions(-) base-commit: 853546f8128476eefb701d4a55b2781bb3a46faa prerequisite-patch-id: 8209301569bd30ba806d06b3452a2f3156503a7a -- γαῖα πυρί μιχθήτω
Re: [PATCH v2] virtio-snd: Enhance error handling for invalid transfers
ping
On Fri, 22 Mar 2024 13:08, Zheyu Ma wrote:
This patch improves error handling in virtio_snd_handle_tx_xfer()
and virtio_snd_handle_rx_xfer() in the VirtIO sound driver. Previously,
'goto' statements were used for error paths, leading to unnecessary
processing and potential null pointer dereferences. Now, 'continue' is
used to skip the rest of the current loop iteration for errors such as
message size discrepancies or null streams, reducing crash risks.
ASAN log illustrating the issue addressed:
ERROR: AddressSanitizer: SEGV on unknown address 0x00b4
#0 0x57cea39967b8 in qemu_mutex_lock_impl qemu/util/qemu-thread-posix.c:92:5
#1 0x57cea128c462 in qemu_mutex_lock qemu/include/qemu/thread.h:122:5
#2 0x57cea128d72f in qemu_lockable_lock qemu/include/qemu/lockable.h:95:5
#3 0x57cea128c294 in qemu_lockable_auto_lock
qemu/include/qemu/lockable.h:105:5
#4 0x57cea1285eb2 in virtio_snd_handle_rx_xfer
qemu/hw/audio/virtio-snd.c:1026:9
#5 0x57cea2caebbc in virtio_queue_notify_vq qemu/hw/virtio/virtio.c:2268:9
#6 0x57cea2cae412 in virtio_queue_host_notifier_read
qemu/hw/virtio/virtio.c:3671:9
#7 0x57cea39822f1 in aio_dispatch_handler qemu/util/aio-posix.c:372:9
#8 0x57cea3979385 in aio_dispatch_handlers qemu/util/aio-posix.c:414:20
#9 0x57cea3978eb1 in aio_dispatch qemu/util/aio-posix.c:424:5
#10 0x57cea3a1eede in aio_ctx_dispatch qemu/util/async.c:360:5
Signed-off-by: Zheyu Ma
Reviewed-by: Manos Pitsidianakis
---
Changes in v2:
- Applied similar error handling logic to virtio_snd_handle_rx_xfer()
for consistency.
---
hw/audio/virtio-snd.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index e604d8f30c..30493f06a8 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -913,13 +913,13 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev,
VirtQueue *vq)
,
sizeof(virtio_snd_pcm_xfer));
if (msg_sz != sizeof(virtio_snd_pcm_xfer)) {
-goto tx_err;
+continue;
}
stream_id = le32_to_cpu(hdr.stream_id);
if (stream_id >= s->snd_conf.streams
|| s->pcm->streams[stream_id] == NULL) {
-goto tx_err;
+continue;
}
stream = s->pcm->streams[stream_id];
@@ -995,13 +995,13 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev,
VirtQueue *vq)
,
sizeof(virtio_snd_pcm_xfer));
if (msg_sz != sizeof(virtio_snd_pcm_xfer)) {
-goto rx_err;
+continue;
}
stream_id = le32_to_cpu(hdr.stream_id);
if (stream_id >= s->snd_conf.streams
|| !s->pcm->streams[stream_id]) {
-goto rx_err;
+continue;
}
stream = s->pcm->streams[stream_id];
--
2.34.1
[PATCH v1 1/1] virtio-snd: rewrite invalid tx/rx message handling
The current handling of invalid virtqueue elements inside the TX/RX virt
queue handlers is wrong.
They are added in a per-stream invalid queue to be processed after the
handler is done examining each message, but the invalid message might
not be specifying any stream_id; which means it's invalid to add it to
any stream->invalid queue since stream could be NULL at this point.
This commit moves the invalid queue to the VirtIOSound struct which
guarantees there will always be a valid temporary place to store them
inside the tx/rx handlers. The queue will be emptied before the handler
returns, so the queue must be empty at any other point of the device's
lifetime.
Signed-off-by: Manos Pitsidianakis
---
include/hw/audio/virtio-snd.h | 16 +++-
hw/audio/virtio-snd.c | 137 +++---
2 files changed, 77 insertions(+), 76 deletions(-)
diff --git a/include/hw/audio/virtio-snd.h b/include/hw/audio/virtio-snd.h
index 3d79181364..8dafedb276 100644
--- a/include/hw/audio/virtio-snd.h
+++ b/include/hw/audio/virtio-snd.h
@@ -151,7 +151,6 @@ struct VirtIOSoundPCMStream {
QemuMutex queue_mutex;
bool active;
QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) queue;
-QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) invalid;
};
/*
@@ -223,6 +222,21 @@ struct VirtIOSound {
QemuMutex cmdq_mutex;
QTAILQ_HEAD(, virtio_snd_ctrl_command) cmdq;
bool processing_cmdq;
+/*
+ * Convenience queue to keep track of invalid tx/rx queue messages inside
+ * the tx/rx callbacks.
+ *
+ * In the callbacks as a first step we are emptying the virtqueue to handle
+ * each message and we cannot add an invalid message back to the queue: we
+ * would re-process it in subsequent loop iterations.
+ *
+ * Instead, we add them to this queue and after finishing examining every
+ * virtqueue element, we inform the guest for each invalid message.
+ *
+ * This queue must be empty at all times except for inside the tx/rx
+ * callbacks.
+ */
+QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) invalid;
};
struct virtio_snd_ctrl_command {
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index 30493f06a8..90d9a2796e 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -456,7 +456,6 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s,
uint32_t stream_id)
stream->s = s;
qemu_mutex_init(>queue_mutex);
QSIMPLEQ_INIT(>queue);
-QSIMPLEQ_INIT(>invalid);
/*
* stream_id >= s->snd_conf.streams was checked before so this is
@@ -611,9 +610,6 @@ static size_t
virtio_snd_pcm_get_io_msgs_count(VirtIOSoundPCMStream *stream)
QSIMPLEQ_FOREACH_SAFE(buffer, >queue, entry, next) {
count += 1;
}
-QSIMPLEQ_FOREACH_SAFE(buffer, >invalid, entry, next) {
-count += 1;
-}
}
return count;
}
@@ -831,47 +827,36 @@ static void virtio_snd_handle_event(VirtIODevice *vdev,
VirtQueue *vq)
trace_virtio_snd_handle_event();
}
+/*
+ * Must only be called if vsnd->invalid is not empty.
+ */
static inline void empty_invalid_queue(VirtIODevice *vdev, VirtQueue *vq)
{
VirtIOSoundPCMBuffer *buffer = NULL;
-VirtIOSoundPCMStream *stream = NULL;
virtio_snd_pcm_status resp = { 0 };
VirtIOSound *vsnd = VIRTIO_SND(vdev);
-bool any = false;
-for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
-stream = vsnd->pcm->streams[i];
-if (stream) {
-any = false;
-WITH_QEMU_LOCK_GUARD(>queue_mutex) {
-while (!QSIMPLEQ_EMPTY(>invalid)) {
-buffer = QSIMPLEQ_FIRST(>invalid);
-if (buffer->vq != vq) {
-break;
-}
-any = true;
-resp.status = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
-iov_from_buf(buffer->elem->in_sg,
- buffer->elem->in_num,
- 0,
- ,
- sizeof(virtio_snd_pcm_status));
-virtqueue_push(vq,
- buffer->elem,
- sizeof(virtio_snd_pcm_status));
-QSIMPLEQ_REMOVE_HEAD(>invalid, entry);
-virtio_snd_pcm_buffer_free(buffer);
-}
-if (any) {
-/*
- * Notify vq about virtio_snd_pcm_status responses.
- * Buffer responses must be notified separately later.
- */
-virtio_notify(vdev, vq);
-}
-}
-}
+g_assert(!QSIMPLEQ_EMPTY(>invalid));
+
+while (!QSIMPLEQ_EMPTY(>invalid)) {
+buffer = QSIMPLEQ_FIRST(>invalid);
+/* If buffer->
[PATCH v1 0/1] virtio-snd: fix invalid tx/rx message handling logic
This is a logic fix for the error handling in the TX/RX virt queue handlers. A potential invalid address dereference was reported and fixed by Zheyu Ma in <20240322110827.568412-1-zheyum...@gmail.com>. This patch moves the invalid message storage from the stream structs to the virtio device struct to 1. make such bug impossible 2. reply to invalid messages again with VIRTIO_SND_S_BAD_MSG, which was not possible before. Patch based on master base-commit: 853546f8128476eefb701d4a55b2781bb3a46faa with the following patch applied: Subject: [PATCH v2] virtio-snd: Enhance error handling for invalid transfers From: Zheyu Ma Date: Fri, 22 Mar 2024 12:08:27 +0100 Message-Id: <20240322110827.568412-1-zheyum...@gmail.com> Manos Pitsidianakis (1): virtio-snd: rewrite invalid tx/rx message handling include/hw/audio/virtio-snd.h | 16 +++- hw/audio/virtio-snd.c | 137 +++--- 2 files changed, 77 insertions(+), 76 deletions(-) base-commit: 853546f8128476eefb701d4a55b2781bb3a46faa prerequisite-patch-id: 8209301569bd30ba806d06b3452a2f3156503a7a -- γαῖα πυρί μιχθήτω
Re: [PATCH] virtio-snd: Skip invalid message sizes and null streams
Hello Ma,
On Thu, 21 Mar 2024 23:42, Zheyu Ma wrote:
This update changes how virtio_snd_handle_tx_xfer handles message size
discrepancies and null streams. Instead of using error handling paths
which led to unnecessary processing and potential null pointer dereferences,
the function now continues to the next loop iteration.
ASAN log illustrating the issue addressed:
ERROR: AddressSanitizer: SEGV on unknown address 0x00b4 (pc
0x57cea39967b8 bp 0x7ffce84d51b0 sp 0x7ffce84d5160 T0)
#0 0x57cea39967b8 in qemu_mutex_lock_impl qemu/util/qemu-thread-posix.c:92:5
#1 0x57cea128c462 in qemu_mutex_lock qemu/include/qemu/thread.h:122:5
#2 0x57cea128d72f in qemu_lockable_lock qemu/include/qemu/lockable.h:95:5
#3 0x57cea128c294 in qemu_lockable_auto_lock
qemu/include/qemu/lockable.h:105:5
#4 0x57cea1285eb2 in virtio_snd_handle_rx_xfer
qemu/hw/audio/virtio-snd.c:1026:9
#5 0x57cea2caebbc in virtio_queue_notify_vq qemu/hw/virtio/virtio.c:2268:9
#6 0x57cea2cae412 in virtio_queue_host_notifier_read
qemu/hw/virtio/virtio.c:3671:9
#7 0x57cea39822f1 in aio_dispatch_handler qemu/util/aio-posix.c:372:9
#8 0x57cea3979385 in aio_dispatch_handlers qemu/util/aio-posix.c:414:20
#9 0x57cea3978eb1 in aio_dispatch qemu/util/aio-posix.c:424:5
#10 0x57cea3a1eede in aio_ctx_dispatch qemu/util/async.c:360:5
Signed-off-by: Zheyu Ma
---
hw/audio/virtio-snd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index e604d8f30c..d9e9f980f7 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -913,13 +913,13 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev,
VirtQueue *vq)
,
sizeof(virtio_snd_pcm_xfer));
if (msg_sz != sizeof(virtio_snd_pcm_xfer)) {
-goto tx_err;
+continue;
}
stream_id = le32_to_cpu(hdr.stream_id);
if (stream_id >= s->snd_conf.streams
|| s->pcm->streams[stream_id] == NULL) {
-goto tx_err;
+continue;
}
stream = s->pcm->streams[stream_id];
--
2.34.1
While the bug is valid I think the fix is insufficient, but not because
it is wrong. The invalid elements are leaked and the guest never gets a
BAD_MSG response. The problem is in the error handling logic; I think
the invalid queue should be moved to the device struct since it's not
stream specific.
Cc'ing qemu-stable because this bug is present in current versions.
Please make the same changes to virtio_snd_handle_rx_xfer() as well and
send a v2, cc'ing qemu-stable. With those changes you can add:
Reviewed-by: Manos Pitsidianakis
I will prepare a patch fixing the invalid element handling logic for
when this fix is accepted.
Thanks,
Manos
Re: [PATCH v2] gitlab: aggressively avoid extra GIT data
On Tue, 12 Mar 2024 at 19:09, Alex Bennée wrote: > > This avoids fetching blobs and tree references for branches we are not > going to worry about. Also skip tag references which are similarly not > useful and keep the default --prune. This keeps the .git data to > around 100M rather than the ~400M even a shallow clone takes. > > So we can check the savings we also run a quick du while setting up > the build. > > We also have to have special settings of GIT_FETCH_EXTRA_FLAGS for the > Windows build, the migration legacy test and the custom runners. In > the case of the custom runners we also move the free floating variable > to the runner template. > > Signed-off-by: Alex Bennée > > --- > v2 > - make custom runners follow the legacy options > --- Reviewed-by: Manos Pitsidianakis
Re: [PATCH-for-9.0 9/9] hw/xen/hvm: Inline xen_arch_set_memory()
On Tue, 14 Nov 2023 18:31, Philippe Mathieu-Daudé wrote:
xen_arch_set_memory() is not arch-specific anymore. Being
called once, inline it in xen_set_memory().
Signed-off-by: Philippe Mathieu-Daudé
---
include/hw/xen/xen-hvm-common.h | 3 -
hw/xen/xen-hvm-common.c | 104
2 files changed, 51 insertions(+), 56 deletions(-)
diff --git a/include/hw/xen/xen-hvm-common.h b/include/hw/xen/xen-hvm-common.h
index 536712dc83..a1b8a2783b 100644
--- a/include/hw/xen/xen-hvm-common.h
+++ b/include/hw/xen/xen-hvm-common.h
@@ -99,8 +99,5 @@ void cpu_ioreq_pio(ioreq_t *req);
void xen_read_physmap(XenIOState *state);
void xen_arch_handle_ioreq(XenIOState *state, ioreq_t *req);
-void xen_arch_set_memory(XenIOState *state,
- MemoryRegionSection *section,
- bool add);
#endif /* HW_XEN_HVM_COMMON_H */
diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
index 50ce45effc..789c6b4b7a 100644
--- a/hw/xen/xen-hvm-common.c
+++ b/hw/xen/xen-hvm-common.c
@@ -426,50 +426,6 @@ void qmp_xen_set_global_dirty_log(bool enable, Error
**errp)
}
}
-void xen_arch_set_memory(XenIOState *state, MemoryRegionSection *section,
- bool add)
-{
-unsigned target_page_bits = qemu_target_page_bits();
-int page_size = qemu_target_page_size();
-int page_mask = -page_size;
-hwaddr start_addr = section->offset_within_address_space;
-ram_addr_t size = int128_get64(section->size);
-bool log_dirty = memory_region_is_logging(section->mr, DIRTY_MEMORY_VGA);
-hvmmem_type_t mem_type;
-
-if (!memory_region_is_ram(section->mr)) {
-return;
-}
-
-if (log_dirty != add) {
-return;
-}
-
-trace_xen_client_set_memory(start_addr, size, log_dirty);
-
-start_addr &= page_mask;
-size = ROUND_UP(size, page_size);
-
-if (add) {
-if (!memory_region_is_rom(section->mr)) {
-xen_add_to_physmap(state, start_addr, size,
- section->mr, section->offset_within_region);
-} else {
-mem_type = HVMMEM_ram_ro;
-if (xen_set_mem_type(xen_domid, mem_type,
- start_addr >> target_page_bits,
- size >> target_page_bits)) {
-DPRINTF("xen_set_mem_type error, addr: "HWADDR_FMT_plx"\n",
-start_addr);
-}
-}
-} else {
-if (xen_remove_from_physmap(state, start_addr, size) < 0) {
-DPRINTF("physmapping does not exist at "HWADDR_FMT_plx"\n",
start_addr);
-}
-}
-}
-
void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr,
Error **errp)
{
@@ -512,20 +468,62 @@ static void xen_set_memory(struct MemoryListener
*listener,
bool add)
{
XenIOState *state = container_of(listener, XenIOState, memory_listener);
+unsigned target_page_bits = qemu_target_page_bits();
+int page_size = qemu_target_page_size();
+int page_mask = -page_size;
+hwaddr start_addr;
+ram_addr_t size;
+bool log_dirty;
+hvmmem_type_t mem_type;
+
if (section->mr == _memory) {
return;
-} else {
-if (add) {
-xen_map_memory_section(xen_domid, state->ioservid,
- section);
-} else {
-xen_unmap_memory_section(xen_domid, state->ioservid,
- section);
-}
}
-xen_arch_set_memory(state, section, add);
+if (add) {
+xen_map_memory_section(xen_domid, state->ioservid,
+ section);
+} else {
+xen_unmap_memory_section(xen_domid, state->ioservid,
+ section);
+}
+
+if (!memory_region_is_ram(section->mr)) {
+return;
+}
+
+log_dirty = memory_region_is_logging(section->mr, DIRTY_MEMORY_VGA);
+
+if (log_dirty != add) {
+return;
+}
+
+start_addr = section->offset_within_address_space;
+size = int128_get64(section->size);
+trace_xen_client_set_memory(start_addr, size, log_dirty);
+
+start_addr &= page_mask;
+size = ROUND_UP(size, page_size);
+
+if (add) {
+if (!memory_region_is_rom(section->mr)) {
+xen_add_to_physmap(state, start_addr, size,
+ section->mr, section->offset_within_region);
+} else {
+mem_type = HVMMEM_ram_ro;
+if (xen_set_mem_type(xen_domid, mem_type,
+ start_addr >> target_page_bits,
+ size >> target_page_bits)) {
+DPRINTF("xen_set_mem_type error, addr: "HWADDR_FMT_plx"\n",
+start_addr);
+}
+}
+} else {
+if (xen_remove_from_physmap(state, start_addr, size) < 0) {
+DPRINTF("physmapping does not exist at
Re: [RFC PATCH-for-9.0 8/9] hw/xen/hvm: Merge xen-hvm-common.c files
On Tue, 14 Nov 2023 18:31, Philippe Mathieu-Daudé wrote:
hw/i386/xen/xen-hvm-common.c content is target agnostic,
and should be common to all targets. Merge both files.
Remove the now unnecessary xen_register_framebuffer() stub.
ARM targets now inherit the common xen_memory_listener.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/arm/xen_arm.c | 24 --
hw/i386/xen/xen-hvm-common.c | 473 ---
hw/xen/xen-hvm-common.c | 458 +
stubs/xen-hw-stub.c | 4 -
hw/i386/xen/meson.build | 1 -
5 files changed, 458 insertions(+), 502 deletions(-)
delete mode 100644 hw/i386/xen/xen-hvm-common.c
diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
index 39dcd74d07..0ead84c9e1 100644
--- a/hw/arm/xen_arm.c
+++ b/hw/arm/xen_arm.c
@@ -38,17 +38,6 @@
#define TYPE_XEN_ARM MACHINE_TYPE_NAME("xenpvh")
OBJECT_DECLARE_SIMPLE_TYPE(XenArmState, XEN_ARM)
-const MemoryListener xen_memory_listener = {
-.region_add = xen_region_add,
-.region_del = xen_region_del,
-.log_start = NULL,
-.log_stop = NULL,
-.log_sync = NULL,
-.log_global_start = NULL,
-.log_global_stop = NULL,
-.priority = MEMORY_LISTENER_PRIORITY_ACCEL,
-};
-
struct XenArmState {
/*< private >*/
MachineState parent;
@@ -136,19 +125,6 @@ void xen_arch_handle_ioreq(XenIOState *state, ioreq_t *req)
return;
}
-void xen_arch_set_memory(XenIOState *state, MemoryRegionSection *section,
- bool add)
-{
-}
-
-void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length)
-{
-}
-
-void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
-{
-}
I think this is not correct, Xen on arm does not do migration and does
not handle memory (the hardware does). We should keep the stubs here.
And not merge the physmapping/dirty mapping of i386 to the common code.
.. which I guess means xen_memory_listener cannot be shared either.
Unless it's non-const and i386 code sets the log_* callback fields on
init.
-
#ifdef CONFIG_TPM
static void xen_enable_tpm(XenArmState *xam)
{
diff --git a/hw/i386/xen/xen-hvm-common.c b/hw/i386/xen/xen-hvm-common.c
deleted file mode 100644
index e8ef0e0c94..00
--- a/hw/i386/xen/xen-hvm-common.c
+++ /dev/null
@@ -1,473 +0,0 @@
-/*
- * Copyright (C) 2010 Citrix Ltd.
- *
- * This work is licensed under the terms of the GNU GPL, version 2. See
- * the COPYING file in the top-level directory.
- *
- * Contributions after 2012-01-13 are licensed under the terms of the
- * GNU GPL, version 2 or (at your option) any later version.
- */
-
-#include "qemu/osdep.h"
-#include "qemu/range.h"
-#include "qapi/qapi-commands-migration.h"
-#include "exec/target_page.h"
-#include "hw/xen/xen-hvm-common.h"
-#include "trace.h"
-
-static MemoryRegion *framebuffer;
-static bool xen_in_migration;
-
-static QLIST_HEAD(, XenPhysmap) xen_physmap;
-static const XenPhysmap *log_for_dirtybit;
-/* Buffer used by xen_sync_dirty_bitmap */
-static unsigned long *dirty_bitmap;
-
-static XenPhysmap *get_physmapping(hwaddr start_addr, ram_addr_t size,
- int page_mask)
-{
-XenPhysmap *physmap = NULL;
-
-start_addr &= -page_mask;
-
-QLIST_FOREACH(physmap, _physmap, list) {
-if (range_covers_byte(physmap->start_addr, physmap->size, start_addr))
{
-return physmap;
-}
-}
-return NULL;
-}
-
-static hwaddr xen_phys_offset_to_gaddr(hwaddr phys_offset, ram_addr_t size,
- int page_mask)
-{
-hwaddr addr = phys_offset & -page_mask;
-XenPhysmap *physmap = NULL;
-
-QLIST_FOREACH(physmap, _physmap, list) {
-if (range_covers_byte(physmap->phys_offset, physmap->size, addr)) {
-return physmap->start_addr + (phys_offset - physmap->phys_offset);
-}
-}
-
-return phys_offset;
-}
-
-#ifdef XEN_COMPAT_PHYSMAP
-static int xen_save_physmap(XenIOState *state, XenPhysmap *physmap)
-{
-char path[80], value[17];
-
-snprintf(path, sizeof(path),
-"/local/domain/0/device-model/%d/physmap/%"PRIx64"/start_addr",
-xen_domid, (uint64_t)physmap->phys_offset);
-snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)physmap->start_addr);
-if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
-return -1;
-}
-snprintf(path, sizeof(path),
-"/local/domain/0/device-model/%d/physmap/%"PRIx64"/size",
-xen_domid, (uint64_t)physmap->phys_offset);
-snprintf(value, sizeof(value), "%"PRIx64, (uint64_t)physmap->size);
-if (!xs_write(state->xenstore, 0, path, value, strlen(value))) {
-return -1;
-}
-if (physmap->name) {
-snprintf(path, sizeof(path),
-"/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
-xen_domid, (uint64_t)physmap->phys_offset);
-if (!xs_write(state->xenstore, 0, path,
- physmap->name,
Re: [PATCH-for-9.0 7/9] hw/xen/hvm: Extract common code to xen-hvm-common.c
-free(value);
-
-snprintf(path, sizeof(path),
-"/local/domain/0/device-model/%d/physmap/%s/name",
-xen_domid, entries[i]);
-physmap->name = xs_read(state->xenstore, 0, path, );
-
-QLIST_INSERT_HEAD(_physmap, physmap, list);
-}
-free(entries);
-}
-#else
-void xen_read_physmap(XenIOState *state)
-{
-QLIST_INIT(_physmap);
-}
-#endif
-
static void xen_wakeup_notifier(Notifier *notifier, void *data)
{
xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
@@ -635,91 +261,6 @@ err:
exit(1);
}
-void xen_register_framebuffer(MemoryRegion *mr)
-{
-framebuffer = mr;
-}
-
-void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length)
-{
-unsigned target_page_bits = qemu_target_page_bits();
-int page_size = qemu_target_page_size();
-int page_mask = -page_size;
-
-if (unlikely(xen_in_migration)) {
-int rc;
-ram_addr_t start_pfn, nb_pages;
-
-start = xen_phys_offset_to_gaddr(start, length, page_mask);
-
-if (length == 0) {
-length = page_size;
-}
-start_pfn = start >> target_page_bits;
-nb_pages = ((start + length + page_size - 1) >> target_page_bits)
-- start_pfn;
-rc = xen_modified_memory(xen_domid, start_pfn, nb_pages);
-if (rc) {
-fprintf(stderr,
-"%s failed for "RAM_ADDR_FMT" ("RAM_ADDR_FMT"): %i, %s\n",
-__func__, start, nb_pages, errno, strerror(errno));
-}
-}
-}
-
-void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
-{
-if (enable) {
-memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION);
-} else {
-memory_global_dirty_log_stop(GLOBAL_DIRTY_MIGRATION);
-}
-}
-
-void xen_arch_set_memory(XenIOState *state, MemoryRegionSection *section,
- bool add)
-{
-unsigned target_page_bits = qemu_target_page_bits();
-int page_size = qemu_target_page_size();
-int page_mask = -page_size;
-hwaddr start_addr = section->offset_within_address_space;
-ram_addr_t size = int128_get64(section->size);
-bool log_dirty = memory_region_is_logging(section->mr, DIRTY_MEMORY_VGA);
-hvmmem_type_t mem_type;
-
-if (!memory_region_is_ram(section->mr)) {
-return;
-}
-
-if (log_dirty != add) {
-return;
-}
-
-trace_xen_client_set_memory(start_addr, size, log_dirty);
-
-start_addr &= page_mask;
-size = ROUND_UP(size, page_size);
-
-if (add) {
-if (!memory_region_is_rom(section->mr)) {
-xen_add_to_physmap(state, start_addr, size,
- section->mr, section->offset_within_region);
-} else {
-mem_type = HVMMEM_ram_ro;
-if (xen_set_mem_type(xen_domid, mem_type,
- start_addr >> target_page_bits,
- size >> target_page_bits)) {
-DPRINTF("xen_set_mem_type error, addr: "HWADDR_FMT_plx"\n",
-start_addr);
-}
-}
-} else {
-if (xen_remove_from_physmap(state, start_addr, size) < 0) {
-DPRINTF("physmapping does not exist at "HWADDR_FMT_plx"\n",
start_addr);
-}
-}
-}
-
void xen_arch_handle_ioreq(XenIOState *state, ioreq_t *req)
{
switch (req->type) {
diff --git a/hw/i386/xen/meson.build b/hw/i386/xen/meson.build
index 3f0df8bc07..d38759cfe4 100644
--- a/hw/i386/xen/meson.build
+++ b/hw/i386/xen/meson.build
@@ -1,6 +1,7 @@
i386_ss.add(when: 'CONFIG_XEN', if_true: files(
'xen_apic.c',
'xen_pvdevice.c',
+ 'xen-hvm-common.c',
))
i386_ss.add(when: ['CONFIG_XEN', xen], if_true: files(
'xen-hvm.c',
--
2.41.0
Reviewed-by: Manos Pitsidianakis
Re: [RFC PATCH-for-9.0 6/9] hw/xen/hvm: Initialize xen_physmap QLIST in xen_read_physmap()
On Tue, 14 Nov 2023 18:31, Philippe Mathieu-Daudé wrote:
xen_read_physmap() is the first function requiring
xen_physmap QLIST being initialized. Move the init
call there.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/i386/xen/xen-hvm.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 789779d02c..3b9c31c1c8 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -532,6 +532,8 @@ void xen_read_physmap(XenIOState *state)
char path[80], *value = NULL;
char **entries = NULL;
+QLIST_INIT(_physmap);
+
snprintf(path, sizeof(path),
"/local/domain/0/device-model/%d/physmap", xen_domid);
entries = xs_directory(state->xenstore, 0, path, );
@@ -575,6 +577,7 @@ void xen_read_physmap(XenIOState *state)
#else
void xen_read_physmap(XenIOState *state)
{
+QLIST_INIT(_physmap);
}
#endif
@@ -595,7 +598,6 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion
**ram_memory)
xen_register_ioreq(state, max_cpus, _memory_listener);
-QLIST_INIT(_physmap);
xen_read_physmap(state);
suspend.notify = xen_suspend_notifier;
--
2.41.0
Reviewed-by: Manos Pitsidianakis
Re: [PATCH-for-9.0 5/9] hw/xen/hvm: Expose xen_read_physmap() prototype
On Tue, 14 Nov 2023 18:31, Philippe Mathieu-Daudé wrote:
In a pair of commit we are going to call xen_read_physmap()
out of hw/i386/xen/xen-hvm.c.
Signed-off-by: Philippe Mathieu-Daudé
---
include/hw/xen/xen-hvm-common.h | 1 +
hw/i386/xen/xen-hvm.c | 4 ++--
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/hw/xen/xen-hvm-common.h b/include/hw/xen/xen-hvm-common.h
index 0fed15ed04..536712dc83 100644
--- a/include/hw/xen/xen-hvm-common.h
+++ b/include/hw/xen/xen-hvm-common.h
@@ -97,6 +97,7 @@ void xen_register_ioreq(XenIOState *state, unsigned int
max_cpus,
void cpu_ioreq_pio(ioreq_t *req);
+void xen_read_physmap(XenIOState *state);
void xen_arch_handle_ioreq(XenIOState *state, ioreq_t *req);
void xen_arch_set_memory(XenIOState *state,
MemoryRegionSection *section,
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index a65a96f0de..789779d02c 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -525,7 +525,7 @@ static void handle_vmport_ioreq(XenIOState *state, ioreq_t
*req)
}
#ifdef XEN_COMPAT_PHYSMAP
-static void xen_read_physmap(XenIOState *state)
+void xen_read_physmap(XenIOState *state)
{
XenPhysmap *physmap = NULL;
unsigned int len, num, i;
@@ -573,7 +573,7 @@ static void xen_read_physmap(XenIOState *state)
free(entries);
}
#else
-static void xen_read_physmap(XenIOState *state)
+void xen_read_physmap(XenIOState *state)
{
}
#endif
--
2.41.0
Reviewed-by: Manos Pitsidianakis
Re: [PATCH-for-9.0 4/9] hw/xen/hvm: Expose xen_memory_listener declaration
On Tue, 14 Nov 2023 18:31, Philippe Mathieu-Daudé wrote:
There can only be a single xen_memory_listener definition
in a qemu-system binary.
Signed-off-by: Philippe Mathieu-Daudé
---
include/hw/xen/xen-hvm-common.h | 1 +
hw/arm/xen_arm.c| 2 +-
hw/i386/xen/xen-hvm.c | 2 +-
3 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/hw/xen/xen-hvm-common.h b/include/hw/xen/xen-hvm-common.h
index 83ed16f425..0fed15ed04 100644
--- a/include/hw/xen/xen-hvm-common.h
+++ b/include/hw/xen/xen-hvm-common.h
@@ -18,6 +18,7 @@
extern MemoryRegion xen_memory;
extern MemoryListener xen_io_listener;
extern DeviceListener xen_device_listener;
+extern const MemoryListener xen_memory_listener;
//#define DEBUG_XEN_HVM
diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
index b478d74ea0..39dcd74d07 100644
--- a/hw/arm/xen_arm.c
+++ b/hw/arm/xen_arm.c
@@ -38,7 +38,7 @@
#define TYPE_XEN_ARM MACHINE_TYPE_NAME("xenpvh")
OBJECT_DECLARE_SIMPLE_TYPE(XenArmState, XEN_ARM)
-static const MemoryListener xen_memory_listener = {
+const MemoryListener xen_memory_listener = {
.region_add = xen_region_add,
.region_del = xen_region_del,
.log_start = NULL,
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index b64204ea94..a65a96f0de 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -469,7 +469,7 @@ static void xen_log_global_stop(MemoryListener *listener)
xen_in_migration = false;
}
-static const MemoryListener xen_memory_listener = {
+const MemoryListener xen_memory_listener = {
.name = "xen-memory",
.region_add = xen_region_add,
.region_del = xen_region_del,
--
2.41.0
Reviewed-by: Manos Pitsidianakis
Re: [PATCH-for-9.0 3/9] hw/xen/hvm: Get target page size at runtime
void xen_sync_dirty_bitmap(XenIOState *state,
j = ctzl(map);
map &= ~(1ul << j);
memory_region_set_dirty(framebuffer,
-(i * width + j) * TARGET_PAGE_SIZE,
-TARGET_PAGE_SIZE);
+(i * width + j) * page_size, page_size);
};
}
}
@@ -631,17 +640,21 @@ void xen_register_framebuffer(MemoryRegion *mr)
void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length)
{
+unsigned target_page_bits = qemu_target_page_bits();
+int page_size = qemu_target_page_size();
+int page_mask = -page_size;
+
if (unlikely(xen_in_migration)) {
int rc;
ram_addr_t start_pfn, nb_pages;
-start = xen_phys_offset_to_gaddr(start, length, TARGET_PAGE_MASK);
+start = xen_phys_offset_to_gaddr(start, length, page_mask);
if (length == 0) {
-length = TARGET_PAGE_SIZE;
+length = page_size;
}
-start_pfn = start >> TARGET_PAGE_BITS;
-nb_pages = ((start + length + TARGET_PAGE_SIZE - 1) >>
TARGET_PAGE_BITS)
+start_pfn = start >> target_page_bits;
+nb_pages = ((start + length + page_size - 1) >> target_page_bits)
- start_pfn;
rc = xen_modified_memory(xen_domid, start_pfn, nb_pages);
if (rc) {
@@ -664,6 +677,9 @@ void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
void xen_arch_set_memory(XenIOState *state, MemoryRegionSection *section,
bool add)
{
+unsigned target_page_bits = qemu_target_page_bits();
+int page_size = qemu_target_page_size();
+int page_mask = -page_size;
hwaddr start_addr = section->offset_within_address_space;
ram_addr_t size = int128_get64(section->size);
bool log_dirty = memory_region_is_logging(section->mr, DIRTY_MEMORY_VGA);
@@ -679,8 +695,8 @@ void xen_arch_set_memory(XenIOState *state,
MemoryRegionSection *section,
trace_xen_client_set_memory(start_addr, size, log_dirty);
-start_addr &= TARGET_PAGE_MASK;
-size = ROUND_UP(size, TARGET_PAGE_SIZE);
+start_addr &= page_mask;
+size = ROUND_UP(size, page_size);
if (add) {
if (!memory_region_is_rom(section->mr)) {
@@ -689,8 +705,8 @@ void xen_arch_set_memory(XenIOState *state,
MemoryRegionSection *section,
} else {
mem_type = HVMMEM_ram_ro;
if (xen_set_mem_type(xen_domid, mem_type,
- start_addr >> TARGET_PAGE_BITS,
- size >> TARGET_PAGE_BITS)) {
+ start_addr >> target_page_bits,
+ size >> target_page_bits)) {
DPRINTF("xen_set_mem_type error, addr: "HWADDR_FMT_plx"\n",
start_addr);
}
--
2.41.0
Reviewed-by: Manos Pitsidianakis
Re: [PATCH-for-9.0 2/9] hw/xen/hvm: Propagate page_mask to a pair of functions
On Tue, 14 Nov 2023 18:31, Philippe Mathieu-Daudé wrote:
We are going to replace TARGET_PAGE_MASK by a
runtime variable. In order to reduce code duplication,
propagate TARGET_PAGE_MASK to get_physmapping() and
xen_phys_offset_to_gaddr().
Signed-off-by: Philippe Mathieu-Daudé
---
hw/i386/xen/xen-hvm.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 8aa6a1ec3b..3b10425986 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -174,11 +174,12 @@ static void xen_ram_init(PCMachineState *pcms,
}
}
-static XenPhysmap *get_physmapping(hwaddr start_addr, ram_addr_t size)
+static XenPhysmap *get_physmapping(hwaddr start_addr, ram_addr_t size,
+ int page_mask)
{
XenPhysmap *physmap = NULL;
-start_addr &= TARGET_PAGE_MASK;
+start_addr &= page_mask;
QLIST_FOREACH(physmap, _physmap, list) {
if (range_covers_byte(physmap->start_addr, physmap->size, start_addr)) {
@@ -188,9 +189,10 @@ static XenPhysmap *get_physmapping(hwaddr start_addr,
ram_addr_t size)
return NULL;
}
-static hwaddr xen_phys_offset_to_gaddr(hwaddr phys_offset, ram_addr_t size)
+static hwaddr xen_phys_offset_to_gaddr(hwaddr phys_offset, ram_addr_t size,
+ int page_mask)
{
-hwaddr addr = phys_offset & TARGET_PAGE_MASK;
+hwaddr addr = phys_offset & page_mask;
XenPhysmap *physmap = NULL;
QLIST_FOREACH(physmap, _physmap, list) {
@@ -252,7 +254,7 @@ static int xen_add_to_physmap(XenIOState *state,
hwaddr phys_offset = memory_region_get_ram_addr(mr);
const char *mr_name;
-if (get_physmapping(start_addr, size)) {
+if (get_physmapping(start_addr, size, TARGET_PAGE_MASK)) {
return 0;
}
if (size <= 0) {
@@ -325,7 +327,7 @@ static int xen_remove_from_physmap(XenIOState *state,
XenPhysmap *physmap = NULL;
hwaddr phys_offset = 0;
-physmap = get_physmapping(start_addr, size);
+physmap = get_physmapping(start_addr, size, TARGET_PAGE_MASK);
if (physmap == NULL) {
return -1;
}
@@ -373,7 +375,7 @@ static void xen_sync_dirty_bitmap(XenIOState *state,
int rc, i, j;
const XenPhysmap *physmap = NULL;
-physmap = get_physmapping(start_addr, size);
+physmap = get_physmapping(start_addr, size, TARGET_PAGE_MASK);
if (physmap == NULL) {
/* not handled */
return;
@@ -633,7 +635,7 @@ void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t
length)
int rc;
ram_addr_t start_pfn, nb_pages;
-start = xen_phys_offset_to_gaddr(start, length);
+start = xen_phys_offset_to_gaddr(start, length, TARGET_PAGE_MASK);
if (length == 0) {
length = TARGET_PAGE_SIZE;
--
2.41.0
Reviewed-by: Manos Pitsidianakis
Re: [PATCH-for-9.0 1/9] hw/xen/hvm: Inline TARGET_PAGE_ALIGN() macro
On Tue, 14 Nov 2023 18:31, Philippe Mathieu-Daudé wrote:
Use TARGET_PAGE_SIZE to calculate TARGET_PAGE_ALIGN.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/i386/xen/xen-hvm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index f1c30d1384..8aa6a1ec3b 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -678,7 +678,7 @@ void xen_arch_set_memory(XenIOState *state,
MemoryRegionSection *section,
trace_xen_client_set_memory(start_addr, size, log_dirty);
start_addr &= TARGET_PAGE_MASK;
-size = TARGET_PAGE_ALIGN(size);
+size = ROUND_UP(size, TARGET_PAGE_SIZE);
if (add) {
if (!memory_region_is_rom(section->mr)) {
--
2.41.0
Reviewed-by: Manos Pitsidianakis
Re: [QEMU][PATCH v3 0/7] Xen: support grant mappings.
Hello Vikram, Series doesn't apply on master. Can you rebase and also provide a base-commit with --base= when you use git-format-patch? This will help git rebase if there are any conflicts found locally. Thanks, On Wed, 28 Feb 2024 00:34, Vikram Garhwal wrote: Hi, This patch series add support for grant mappings as a pseudo RAM region for Xen. Enabling grant mappings patches(first 6) are written by Juergen in 2021. QEMU Virtio device provides an emulated backends for Virtio frontned devices in Xen. Please set "iommu_platform=on" option when invoking QEMU. As this will set VIRTIO_F_ACCESS_PLATFORM feature which will be used by virtio frontend in Xen to know whether backend supports grants or not. Changelog: v2->v3: Drop patch 1/7. This was done because device unplug is an x86-only case. Add missing qemu_mutex_unlock() before return. v1->v2: Split patch 2/7 to keep phymem.c changes in a separate. In patch "xen: add map and unmap callbacks for grant" add check for total allowed grant < XEN_MAX_VIRTIO_GRANTS. Fix formatting issues and re-based with master latest. Regards, Vikram Juergen Gross (5): xen: add pseudo RAM region for grant mappings softmmu: let qemu_map_ram_ptr() use qemu_ram_ptr_length() xen: let xen_ram_addr_from_mapcache() return -1 in case of not found entry memory: add MemoryRegion map and unmap callbacks xen: add map and unmap callbacks for grant region Vikram Garhwal (2): softmmu: physmem: Split ram_block_add() hw: arm: Add grant mapping. hw/arm/xen_arm.c| 3 + hw/i386/xen/xen-hvm.c | 3 + hw/xen/xen-hvm-common.c | 4 +- hw/xen/xen-mapcache.c | 214 ++-- include/exec/memory.h | 21 include/exec/ram_addr.h | 1 + include/hw/xen/xen-hvm-common.h | 2 + include/hw/xen/xen_pvdev.h | 3 + include/sysemu/xen-mapcache.h | 3 + system/physmem.c| 179 +++--- 10 files changed, 351 insertions(+), 82 deletions(-) -- 2.17.1
Re: [PATCH v1 01/21] docs: correct typos
On Tue, 20 Feb 2024 12:36, "Michael S. Tsirkin" wrote: On Tue, Feb 20, 2024 at 10:52:08AM +0200, Manos Pitsidianakis wrote: Correct typos automatically found with the `typos` tool <https://crates.io/crates/typos> Signed-off-by: Manos Pitsidianakis Acked-by: Michael S. Tsirkin --- docs/devel/ci-jobs.rst.inc | 2 +- docs/devel/docs.rst | 2 +- docs/devel/testing.rst | 2 +- docs/interop/prl-xml.txt| 2 +- docs/interop/vhost-user.rst | 2 +- docs/system/devices/canokey.rst | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/devel/ci-jobs.rst.inc b/docs/devel/ci-jobs.rst.inc index 4c39cdb2d9..6678b4f4ef 100644 --- a/docs/devel/ci-jobs.rst.inc +++ b/docs/devel/ci-jobs.rst.inc @@ -147,7 +147,7 @@ Set this variable to 1 to create the pipelines, but leave all the jobs to be manually started from the UI Set this variable to 2 to create the pipelines and run all -the jobs immediately, as was historicaly behaviour +the jobs immediately, as was historically behaviour as long as we do this let's fix grammar too? as was historically the behaviour After the fact, I think it should be "as was historical behaviour". I will re-spin with only this change, and keep the Acks/RoBs if that is okay with everyone. Thanks, QEMU_CI_AVOCADO_TESTING ~~~ diff --git a/docs/devel/docs.rst b/docs/devel/docs.rst index 50ff0d67f8..a7768b5311 100644 --- a/docs/devel/docs.rst +++ b/docs/devel/docs.rst @@ -21,7 +21,7 @@ are processed in two ways: The syntax of these ``.hx`` files is simple. It is broadly an alternation of C code put into the C output and rST format text -put into the documention. A few special directives are recognised; +put into the documentation. A few special directives are recognised; these are all-caps and must be at the beginning of the line. ``HXCOMM`` is the comment marker. The line, including any arbitrary diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index bd132306c1..aa96eacec5 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -728,7 +728,7 @@ For example to setup the HPPA ports builds of Debian:: EXECUTABLE=(pwd)/qemu-hppa V=1 The ``DEB_`` variables are substitutions used by -``debian-boostrap.pre`` which is called to do the initial debootstrap +``debian-bootstrap.pre`` which is called to do the initial debootstrap of the rootfs before it is copied into the container. The second stage is run as part of the build. The final image will be tagged as ``qemu/debian-sid-hppa``. diff --git a/docs/interop/prl-xml.txt b/docs/interop/prl-xml.txt index 7031f8752c..cf9b3fba26 100644 --- a/docs/interop/prl-xml.txt +++ b/docs/interop/prl-xml.txt @@ -122,7 +122,7 @@ Each Image element has following child elements: * Type - image type of the element. It can be: "Plain" for raw files. "Compressed" for expanding disks. -* File - path to image file. Path can be relative to DiskDecriptor.xml or +* File - path to image file. Path can be relative to DiskDescriptor.xml or absolute. == Snapshots element == diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index ad6e142f23..d1ed39dfa0 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -989,7 +989,7 @@ When reconnecting: #. If ``d.flags`` is not equal to the calculated flags value (means back-end has submitted the buffer to guest driver before crash, so - it has to commit the in-progres update), set ``old_free_head``, + it has to commit the in-progress update), set ``old_free_head``, ``old_used_idx``, ``old_used_wrap_counter`` to ``free_head``, ``used_idx``, ``used_wrap_counter`` diff --git a/docs/system/devices/canokey.rst b/docs/system/devices/canokey.rst index cfa6186e48..7f3664963f 100644 --- a/docs/system/devices/canokey.rst +++ b/docs/system/devices/canokey.rst @@ -14,7 +14,7 @@ CanoKey [1]_ is an open-source secure key with supports of All these platform-independent features are in canokey-core [3]_. For different platforms, CanoKey has different implementations, -including both hardware implementions and virtual cards: +including both hardware implementations and virtual cards: * CanoKey STM32 [4]_ * CanoKey Pigeon [5]_ -- γαῖα πυρί μιχθήτω
Re: [PATCH v4 4/5] hw/virtio: cleanup shared resources
On Mon, 19 Feb 2024 16:34, Albert Esteve wrote:
Ensure that we cleanup all virtio shared
resources when the vhost devices is cleaned
up (after a hot unplug, or a crash).
To do so, we add a new function to the virtio_dmabuf
API called `virtio_dmabuf_vhost_cleanup`, which
loop through the table and removes all
resources owned by the vhost device parameter.
Also, add a test to verify that the new
function in the API behaves as expected.
Signed-off-by: Albert Esteve
Acked-by: Stefan Hajnoczi
---
Reviewed-by: Manos Pitsidianakis
hw/display/virtio-dmabuf.c| 21
hw/virtio/vhost.c | 3 +++
include/hw/virtio/virtio-dmabuf.h | 10 ++
tests/unit/test-virtio-dmabuf.c | 33 +++
4 files changed, 67 insertions(+)
diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
index 961094a561..703b5bd979 100644
--- a/hw/display/virtio-dmabuf.c
+++ b/hw/display/virtio-dmabuf.c
@@ -141,6 +141,27 @@ SharedObjectType virtio_object_type(const QemuUUID *uuid)
return vso->type;
}
+static bool virtio_dmabuf_resource_is_owned(gpointer key,
+gpointer value,
+gpointer dev)
+{
+VirtioSharedObject *vso;
+
+vso = (VirtioSharedObject *) value;
+return vso->type == TYPE_VHOST_DEV && vso->value.dev == dev;
+}
+
+int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev)
+{
+int num_removed;
+
+WITH_QEMU_LOCK_GUARD() {
+num_removed = g_hash_table_foreach_remove(
+resource_uuids, (GHRFunc) virtio_dmabuf_resource_is_owned, dev);
+}
+return num_removed;
+}
+
void virtio_free_resources(void)
{
WITH_QEMU_LOCK_GUARD() {
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 2c9ac79468..c5622eac14 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -16,6 +16,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "hw/virtio/vhost.h"
+#include "hw/virtio/virtio-dmabuf.h"
#include "qemu/atomic.h"
#include "qemu/range.h"
#include "qemu/error-report.h"
@@ -1599,6 +1600,8 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
migrate_del_blocker(>migration_blocker);
g_free(hdev->mem);
g_free(hdev->mem_sections);
+/* free virtio shared objects */
+virtio_dmabuf_vhost_cleanup(hdev);
if (hdev->vhost_ops) {
hdev->vhost_ops->vhost_backend_cleanup(hdev);
}
diff --git a/include/hw/virtio/virtio-dmabuf.h
b/include/hw/virtio/virtio-dmabuf.h
index 627d84dce9..950cd24967 100644
--- a/include/hw/virtio/virtio-dmabuf.h
+++ b/include/hw/virtio/virtio-dmabuf.h
@@ -119,6 +119,16 @@ struct vhost_dev *virtio_lookup_vhost_device(const
QemuUUID *uuid);
*/
SharedObjectType virtio_object_type(const QemuUUID *uuid);
+/**
+ * virtio_dmabuf_vhost_cleanup() - Destroys all entries of the shared
+ * resources lookup table that are owned by the vhost backend
+ * @dev: the pointer to the vhost device that owns the entries. Data is owned
+ * by the called of the function.
+ *
+ * Return: the number of resource entries removed.
+ */
+int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev);
+
/**
* virtio_free_resources() - Destroys all keys and values of the shared
* resources lookup table, and frees them
diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabuf.c
index 20213455ee..e5cf7ee19f 100644
--- a/tests/unit/test-virtio-dmabuf.c
+++ b/tests/unit/test-virtio-dmabuf.c
@@ -107,6 +107,38 @@ static void test_add_invalid_resource(void)
}
}
+static void test_cleanup_res(void)
+{
+QemuUUID uuids[20], uuid_alt;
+struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
+struct vhost_dev *dev_alt = g_new0(struct vhost_dev, 1);
+int i, num_removed;
+
+for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
+qemu_uuid_generate([i]);
+virtio_add_vhost_device([i], dev);
+/* vhost device is found */
+g_assert(virtio_lookup_vhost_device([i]) != NULL);
+}
+qemu_uuid_generate(_alt);
+virtio_add_vhost_device(_alt, dev_alt);
+/* vhost device is found */
+g_assert(virtio_lookup_vhost_device(_alt) != NULL);
+/* cleanup all dev resources */
+num_removed = virtio_dmabuf_vhost_cleanup(dev);
+g_assert_cmpint(num_removed, ==, ARRAY_SIZE(uuids));
+for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
+/* None of the dev resources is found after free'd */
+g_assert_cmpint(virtio_lookup_dmabuf([i]), ==, -1);
+}
+/* uuid_alt is still in the hash table */
+g_assert(virtio_lookup_vhost_device(_alt) != NULL);
+
+virtio_free_resources();
+g_free(dev);
+g_free(dev_alt);
+}
+
static void test_free_resources(void)
{
QemuUUID uuids[20];
@@ -136,6 +168,7 @@ int main(int argc, char **argv)
test_remove_invalid_resource);
g_test_add_func("/virtio-dmabu
Re: [PATCH v4 3/5] hw/virtio: change dmabuf mutex to QemuMutex
Hello Albert,
This is a point of confusion for me; Volker recently pointed out in a
patch for virtio-snd that all its code runs under the BQL. Is this code
ever called without BQL, for example do the backend read/write functions
from vhost-user.c run without the BQL?
On Mon, 19 Feb 2024 16:34, Albert Esteve wrote:
Change GMutex by QemuMutex to be able to use
lock contexts with `WITH_QEMU_LOCK_GUARD`.
As the lock needs to be initialised and there
is no central point for initialisation, add
an init public function and call it from
virtio.c, each time a new backend structure
is initialised.
Signed-off-by: Albert Esteve
---
hw/display/virtio-dmabuf.c| 55 +--
hw/virtio/virtio.c| 3 ++
include/hw/virtio/virtio-dmabuf.h | 5 +++
tests/unit/test-virtio-dmabuf.c | 5 +++
4 files changed, 43 insertions(+), 25 deletions(-)
diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
index 497cb6fa7c..961094a561 100644
--- a/hw/display/virtio-dmabuf.c
+++ b/hw/display/virtio-dmabuf.c
@@ -11,11 +11,12 @@
*/
#include "qemu/osdep.h"
+#include "include/qemu/lockable.h"
#include "hw/virtio/virtio-dmabuf.h"
-static GMutex lock;
+static QemuMutex lock;
static GHashTable *resource_uuids;
/*
@@ -27,23 +28,27 @@ static int uuid_equal_func(const void *lhv, const void *rhv)
return qemu_uuid_is_equal(lhv, rhv);
}
+void virtio_dmabuf_init(void) {
+qemu_mutex_init();
+}
+
static bool virtio_add_resource(QemuUUID *uuid, VirtioSharedObject *value)
{
bool result = true;
-g_mutex_lock();
-if (resource_uuids == NULL) {
-resource_uuids = g_hash_table_new_full(qemu_uuid_hash,
- uuid_equal_func,
- NULL,
- g_free);
-}
-if (g_hash_table_lookup(resource_uuids, uuid) == NULL) {
-g_hash_table_insert(resource_uuids, uuid, value);
-} else {
-result = false;
+WITH_QEMU_LOCK_GUARD() {
+if (resource_uuids == NULL) {
+resource_uuids = g_hash_table_new_full(qemu_uuid_hash,
+uuid_equal_func,
+NULL,
+g_free);
+}
+if (g_hash_table_lookup(resource_uuids, uuid) == NULL) {
+g_hash_table_insert(resource_uuids, uuid, value);
+} else {
+result = false;
+}
}
-g_mutex_unlock();
return result;
}
@@ -87,9 +92,9 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev
*dev)
bool virtio_remove_resource(const QemuUUID *uuid)
{
bool result;
-g_mutex_lock();
-result = g_hash_table_remove(resource_uuids, uuid);
-g_mutex_unlock();
+WITH_QEMU_LOCK_GUARD() {
+result = g_hash_table_remove(resource_uuids, uuid);
+}
return result;
}
@@ -98,11 +103,11 @@ static VirtioSharedObject *get_shared_object(const
QemuUUID *uuid)
{
gpointer lookup_res = NULL;
-g_mutex_lock();
-if (resource_uuids != NULL) {
-lookup_res = g_hash_table_lookup(resource_uuids, uuid);
+WITH_QEMU_LOCK_GUARD() {
+if (resource_uuids != NULL) {
+lookup_res = g_hash_table_lookup(resource_uuids, uuid);
+}
}
-g_mutex_unlock();
return (VirtioSharedObject *) lookup_res;
}
@@ -138,9 +143,9 @@ SharedObjectType virtio_object_type(const QemuUUID *uuid)
void virtio_free_resources(void)
{
-g_mutex_lock();
-g_hash_table_destroy(resource_uuids);
-/* Reference count shall be 0 after the implicit unref on destroy */
-resource_uuids = NULL;
-g_mutex_unlock();
+WITH_QEMU_LOCK_GUARD() {
+g_hash_table_destroy(resource_uuids);
+/* Reference count shall be 0 after the implicit unref on destroy */
+resource_uuids = NULL;
+}
}
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index d229755eae..88189e7178 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -29,6 +29,7 @@
#include "hw/virtio/virtio-bus.h"
#include "hw/qdev-properties.h"
#include "hw/virtio/virtio-access.h"
+#include "hw/virtio/virtio-dmabuf.h"
#include "sysemu/dma.h"
#include "sysemu/runstate.h"
#include "virtio-qmp.h"
@@ -3221,6 +3222,8 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id,
size_t config_size)
int i;
int nvectors = k->query_nvectors ? k->query_nvectors(qbus->parent) : 0;
+// Ensure virtio dmabuf table is initialised.
+virtio_dmabuf_init();
if (nvectors) {
vdev->vector_queues =
g_malloc0(sizeof(*vdev->vector_queues) * nvectors);
diff --git a/include/hw/virtio/virtio-dmabuf.h
b/include/hw/virtio/virtio-dmabuf.h
index 891a43162d..627d84dce9 100644
--- a/include/hw/virtio/virtio-dmabuf.h
+++ b/include/hw/virtio/virtio-dmabuf.h
@@ -50,6 +50,11 @@ typedef struct VirtioSharedObject {
} value;
} VirtioSharedObject;
+/**
+ *
Re: [PATCH v4 2/5] hw/virtio: document SharedObject structures
On Mon, 19 Feb 2024 16:34, Albert Esteve wrote:
Change VirtioSharedObject value type from
a generic pointer to a union storing the different
supported underlying types, which makes naming
less confusing.
With the update, use the chance to add kdoc
to both the SharedObjectType enum and
VirtioSharedObject struct.
Signed-off-by: Albert Esteve
---
hw/display/virtio-dmabuf.c| 8
include/hw/virtio/virtio-dmabuf.h | 25 -
2 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
index 3dba4577ca..497cb6fa7c 100644
--- a/hw/display/virtio-dmabuf.c
+++ b/hw/display/virtio-dmabuf.c
@@ -57,7 +57,7 @@ bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
}
vso = g_new(VirtioSharedObject, 1);
vso->type = TYPE_DMABUF;
-vso->value = GINT_TO_POINTER(udmabuf_fd);
+vso->value.udma_buf = udmabuf_fd;
result = virtio_add_resource(uuid, vso);
if (!result) {
g_free(vso);
@@ -75,7 +75,7 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev
*dev)
}
vso = g_new(VirtioSharedObject, 1);
vso->type = TYPE_VHOST_DEV;
-vso->value = dev;
+vso->value.dev = dev;
result = virtio_add_resource(uuid, vso);
if (!result) {
g_free(vso);
@@ -114,7 +114,7 @@ int virtio_lookup_dmabuf(const QemuUUID *uuid)
return -1;
}
assert(vso->type == TYPE_DMABUF);
-return GPOINTER_TO_INT(vso->value);
+return vso->value.udma_buf;
}
struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid)
@@ -124,7 +124,7 @@ struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID
*uuid)
return NULL;
}
assert(vso->type == TYPE_VHOST_DEV);
-return (struct vhost_dev *) vso->value;
+return (struct vhost_dev *) vso->value.dev;
Is the casting still required?
}
SharedObjectType virtio_object_type(const QemuUUID *uuid)
diff --git a/include/hw/virtio/virtio-dmabuf.h
b/include/hw/virtio/virtio-dmabuf.h
index 627c3b6db7..891a43162d 100644
--- a/include/hw/virtio/virtio-dmabuf.h
+++ b/include/hw/virtio/virtio-dmabuf.h
@@ -16,15 +16,38 @@
#include "qemu/uuid.h"
#include "vhost.h"
+/**
+ * SharedObjectType:
+ *
+ * Identifies the type of the underlying type that the current lookup
+ * table entry is holding.
+ *
+ * TYPE_INVALID: Invalid entry
+ * TYPE_DMABUF: Entry is a dmabuf file descriptor that can be directly
+ * shared with the requestor
+ * TYPE_VHOST_DEV: Entry is a pointer to a vhost device that is holding
+ * the shared object.
nit:
+ * TYPE_INVALID: Invalid entry.
+ * TYPE_DMABUF:Entry is a dmabuf file descriptor that can be
+ * directly shared with the requestor.
+ * TYPE_VHOST_DEV: Entry is a pointer to a vhost device that is holding
+ * the shared object.
+ */
typedef enum SharedObjectType {
TYPE_INVALID = 0,
TYPE_DMABUF,
TYPE_VHOST_DEV,
} SharedObjectType;
+/**
+ * VirtioSharedObject:
+ * @type: Shared object type identifier
+ * @value: Union containing to the underlying type
+ *
+ * The VirtioSharedObject object provides a way to distinguish,
+ * store, and handle the different types supported by the lookup table.
+ */
typedef struct VirtioSharedObject {
SharedObjectType type;
-gpointer value;
+union {
+struct vhost_dev *dev; /* TYPE_VHOST_DEV */
+int udma_buf; /* TYPE_DMABUF */
+} value;
} VirtioSharedObject;
/**
--
2.43.1
[PATCH v1 14/21] pc-bios/README: correct typos
Correct typos automatically found with the `typos` tool
<https://crates.io/crates/typos>
Signed-off-by: Manos Pitsidianakis
---
pc-bios/README | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pc-bios/README b/pc-bios/README
index 4189bb28cc..b8a0210d24 100644
--- a/pc-bios/README
+++ b/pc-bios/README
@@ -67,7 +67,7 @@
and enable the use of well-known bootloaders such as U-Boot.
OpenSBI is distributed under the terms of the BSD 2-clause license
("Simplified BSD License" or "FreeBSD License", SPDX: BSD-2-Clause). OpenSBI
- source code also contains code reused from other projects desribed here:
+ source code also contains code reused from other projects described here:
https://github.com/riscv/opensbi/blob/master/ThirdPartyNotices.md.
- npcm7xx_bootrom.bin is a simplified, free (Apache 2.0) boot ROM for Nuvoton
--
γαῖα πυρί μιχθήτω
[PATCH v1 09/21] include/exec/plugin-gen.h: correct typos
Correct typos automatically found with the `typos` tool <https://crates.io/crates/typos> Signed-off-by: Manos Pitsidianakis --- include/exec/plugin-gen.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/exec/plugin-gen.h b/include/exec/plugin-gen.h index c4552b5061..b18edd6ab4 100644 --- a/include/exec/plugin-gen.h +++ b/include/exec/plugin-gen.h @@ -19,7 +19,7 @@ struct DisasContextBase; #ifdef CONFIG_PLUGIN bool plugin_gen_tb_start(CPUState *cpu, const struct DisasContextBase *db, - bool supress); + bool suppress); void plugin_gen_tb_end(CPUState *cpu, size_t num_insns); void plugin_gen_insn_start(CPUState *cpu, const struct DisasContextBase *db); void plugin_gen_insn_end(void); -- γαῖα πυρί μιχθήτω
[PATCH v1 10/21] hw/arm/omap.h: correct typos
Correct typos automatically found with the `typos` tool <https://crates.io/crates/typos> Signed-off-by: Manos Pitsidianakis --- include/hw/arm/omap.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h index 067e9419f7..2f59220c0e 100644 --- a/include/hw/arm/omap.h +++ b/include/hw/arm/omap.h @@ -1008,7 +1008,7 @@ void omap_mpu_wakeup(void *opaque, int irq, int req); __func__, paddr) /* OMAP-specific Linux bootloader tags for the ATAG_BOARD area - (Board-specifc tags are not here) */ + (Board-specific tags are not here) */ #define OMAP_TAG_CLOCK 0x4f01 #define OMAP_TAG_MMC 0x4f02 #define OMAP_TAG_SERIAL_CONSOLE0x4f03 -- γαῖα πυρί μιχθήτω
[PATCH v1 17/21] ci/gitlab-pipeline-status: correct typos
Correct typos automatically found with the `typos` tool
<https://crates.io/crates/typos>
Signed-off-by: Manos Pitsidianakis
---
scripts/ci/gitlab-pipeline-status | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/ci/gitlab-pipeline-status
b/scripts/ci/gitlab-pipeline-status
index e3343b0510..39f3c22c66 100755
--- a/scripts/ci/gitlab-pipeline-status
+++ b/scripts/ci/gitlab-pipeline-status
@@ -131,7 +131,7 @@ def create_parser():
'checks of the pipeline status. Defaults '
'to %(default)s'))
parser.add_argument('-w', '--wait', action='store_true', default=False,
-help=('Wether to wait, instead of checking only once '
+help=('Whether to wait, instead of checking only once '
'the status of a pipeline'))
parser.add_argument('-p', '--project-id', type=int, default=11167699,
help=('The GitLab project ID. Defaults to the project '
--
γαῖα πυρί μιχθήτω
[PATCH v1 12/21] hw/net/npcm_gmac.h: correct typos
Correct typos automatically found with the `typos` tool
<https://crates.io/crates/typos>
Signed-off-by: Manos Pitsidianakis
---
include/hw/net/npcm_gmac.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/hw/net/npcm_gmac.h b/include/hw/net/npcm_gmac.h
index f2d9f08ec1..6340ffe92c 100644
--- a/include/hw/net/npcm_gmac.h
+++ b/include/hw/net/npcm_gmac.h
@@ -81,7 +81,7 @@ struct NPCMGMACRxDesc {
/* Disable Interrupt on Completion */
#define RX_DESC_RDES1_DIS_INTR_COMP_MASK BIT(31)
-/* Recieve end of ring */
+/* Receive end of ring */
#define RX_DESC_RDES1_RC_END_RING_MASK BIT(25)
/* Second Address Chained */
#define RX_DESC_RDES1_SEC_ADDR_CHND_MASK BIT(24)
@@ -213,7 +213,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(NPCMGMACState, NPCM_GMAC)
#define NPCM_DMA_STATUS_FBI BIT(13)
/* Early transmit Interrupt */
#define NPCM_DMA_STATUS_ETI BIT(10)
-/* Receive Watchdog Timout */
+/* Receive Watchdog Timeout */
#define NPCM_DMA_STATUS_RWT BIT(9)
/* Receive Process Stopped */
#define NPCM_DMA_STATUS_RPS BIT(8)
--
γαῖα πυρί μιχθήτω
[PATCH v1 11/21] hw/cxl/cxl_device.h: correct typos
Correct typos automatically found with the `typos` tool <https://crates.io/crates/typos> Signed-off-by: Manos Pitsidianakis --- include/hw/cxl/cxl_device.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h index d8e184c4ba..279b276bda 100644 --- a/include/hw/cxl/cxl_device.h +++ b/include/hw/cxl/cxl_device.h @@ -268,7 +268,7 @@ void cxl_event_set_status(CXLDeviceState *cxl_dstate, CXLEventLogType log_type, /* * Helper macro to initialize capability headers for CXL devices. * - * In CXL r3.1 Section 8.2.8.2: CXL Device Capablity Header Register, this is + * In CXL r3.1 Section 8.2.8.2: CXL Device Capability Header Register, this is * listed as a 128b register, but in CXL r3.1 Section 8.2.8: CXL Device Register * Interface, it says: * > No registers defined in Section 8.2.8 are larger than 64-bits wide so that @@ -276,7 +276,7 @@ void cxl_event_set_status(CXLDeviceState *cxl_dstate, CXLEventLogType log_type, * > followed, the behavior is undefined. * * > To illustrate how the fields fit together, the layouts ... are shown as - * > wider than a 64 bit register. Implemenations are expected to use any size + * > wider than a 64 bit register. Implementations are expected to use any size * > accesses for this information up to 64 bits without lost of functionality * * Here we've chosen to make it 4 dwords. -- γαῖα πυρί μιχθήτω
[PATCH v1 20/21] s390x: correct typos
Correct typos automatically found with the `typos` tool <https://crates.io/crates/typos> Signed-off-by: Manos Pitsidianakis --- target/s390x/cpu_features_def.h.inc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/s390x/cpu_features_def.h.inc b/target/s390x/cpu_features_def.h.inc index e68da9b8ff..c53ac13352 100644 --- a/target/s390x/cpu_features_def.h.inc +++ b/target/s390x/cpu_features_def.h.inc @@ -142,7 +142,7 @@ DEF_FEAT(SIE_CEI, "cei", SCLP_CPU, 43, "SIE: Conditional-external-interception f /* * Features exposed via no feature bit (but e.g., instruction sensing) - * -> the feature bit number is irrelavant + * -> the feature bit number is irrelevant */ DEF_FEAT(DAT_ENH_2, "dateh2", MISC, 0, "DAT-enhancement facility 2") DEF_FEAT(CMM, "cmm", MISC, 0, "Collaborative-memory-management facility") -- γαῖα πυρί μιχθήτω
[PATCH v1 03/21] Xen headers: correct typos
Correct typos automatically found with the `typos` tool
<https://crates.io/crates/typos>
Signed-off-by: Manos Pitsidianakis
---
include/hw/xen/interface/arch-x86/xen-x86_64.h | 2 +-
include/hw/xen/interface/arch-x86/xen.h| 2 +-
include/hw/xen/interface/event_channel.h | 2 +-
include/hw/xen/interface/grant_table.h | 2 +-
include/hw/xen/interface/hvm/hvm_op.h | 2 +-
include/hw/xen/interface/io/blkif.h| 4 ++--
include/hw/xen/interface/io/fbif.h | 2 +-
include/hw/xen/interface/io/kbdif.h| 2 +-
include/hw/xen/interface/io/ring.h | 2 +-
include/hw/xen/interface/memory.h | 2 +-
include/hw/xen/interface/physdev.h | 4 ++--
include/hw/xen/interface/xen.h | 4 ++--
12 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/include/hw/xen/interface/arch-x86/xen-x86_64.h
b/include/hw/xen/interface/arch-x86/xen-x86_64.h
index 5d9035ed22..3e94eeb0bf 100644
--- a/include/hw/xen/interface/arch-x86/xen-x86_64.h
+++ b/include/hw/xen/interface/arch-x86/xen-x86_64.h
@@ -89,7 +89,7 @@
* RING1 -> RING3 kernel mode.
* RING2 -> RING3 kernel mode.
* RING3 -> RING3 user mode.
- * However RING0 indicates that the guest kernel should return to iteself
+ * However RING0 indicates that the guest kernel should return to itself
* directly with
* orb $3,1*8(%rsp)
* iretq
diff --git a/include/hw/xen/interface/arch-x86/xen.h
b/include/hw/xen/interface/arch-x86/xen.h
index c0f4551247..323bd06a63 100644
--- a/include/hw/xen/interface/arch-x86/xen.h
+++ b/include/hw/xen/interface/arch-x86/xen.h
@@ -156,7 +156,7 @@ typedef uint64_t tsc_timestamp_t; /* RDTSC timestamp */
* information in this structure is updated, the fields read include: fpu_ctxt
* (if VGCT_I387_VALID is set), flags, user_regs and debugreg[*].
*
- * Note: VCPUOP_initialise for HVM guests is non-symetric with
+ * Note: VCPUOP_initialise for HVM guests is non-symmetric with
* DOMCTL_setvcpucontext, and uses struct vcpu_hvm_context from hvm/hvm_vcpu.h
*/
struct vcpu_guest_context {
diff --git a/include/hw/xen/interface/event_channel.h
b/include/hw/xen/interface/event_channel.h
index 0d91a1c4af..d446863230 100644
--- a/include/hw/xen/interface/event_channel.h
+++ b/include/hw/xen/interface/event_channel.h
@@ -302,7 +302,7 @@ typedef struct evtchn_set_priority evtchn_set_priority_t;
* ` enum neg_errnoval
* ` HYPERVISOR_event_channel_op_compat(struct evtchn_op *op)
* `
- * Superceded by new event_channel_op() hypercall since 0x00030202.
+ * Superseded by new event_channel_op() hypercall since 0x00030202.
*/
struct evtchn_op {
uint32_t cmd; /* enum event_channel_op */
diff --git a/include/hw/xen/interface/grant_table.h
b/include/hw/xen/interface/grant_table.h
index 1dfa17a6d0..7652e8bf81 100644
--- a/include/hw/xen/interface/grant_table.h
+++ b/include/hw/xen/interface/grant_table.h
@@ -607,7 +607,7 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t);
/*
* GNTMAP_contains_pte subflag:
* 0 => This map request contains a host virtual address.
- * 1 => This map request contains the machine addess of the PTE to update.
+ * 1 => This map request contains the machine address of the PTE to update.
*/
#define _GNTMAP_contains_pte(4)
#define GNTMAP_contains_pte (1<<_GNTMAP_contains_pte)
diff --git a/include/hw/xen/interface/hvm/hvm_op.h
b/include/hw/xen/interface/hvm/hvm_op.h
index e22adf0319..3defe1c108 100644
--- a/include/hw/xen/interface/hvm/hvm_op.h
+++ b/include/hw/xen/interface/hvm/hvm_op.h
@@ -337,7 +337,7 @@ struct xen_hvm_altp2m_op {
#define HVMOP_altp2m_vcpu_disable_notify 13
/* Get the active vcpu p2m index */
#define HVMOP_altp2m_get_p2m_idx 14
-/* Set the "Supress #VE" bit for a range of pages */
+/* Set the "Suppress #VE" bit for a range of pages */
#define HVMOP_altp2m_set_suppress_ve_multi 15
/* Set visibility for a given altp2m view */
#define HVMOP_altp2m_set_visibility 16
diff --git a/include/hw/xen/interface/io/blkif.h
b/include/hw/xen/interface/io/blkif.h
index 22f1eef0c0..4356956975 100644
--- a/include/hw/xen/interface/io/blkif.h
+++ b/include/hw/xen/interface/io/blkif.h
@@ -42,7 +42,7 @@
* All data in the XenStore is stored as strings. Nodes specifying numeric
* values are encoded in decimal. Integer value ranges listed below are
* expressed as fixed sized integer types capable of storing the conversion
- * of a properly formated node string, without loss of information.
+ * of a properly formatted node string, without loss of information.
*
* Any specified default value is in effect if the corresponding XenBus node
* is not present in the XenStore.
@@ -406,7 +406,7 @@
* further requests may reuse these grants and require write permissions.
* (9) Linux implementation doesn't have a limit on the maximum number of
* grants that can be persisten
[PATCH v1 02/21] tests: correct typos
Correct typos automatically found with the `typos` tool
<https://crates.io/crates/typos>
Signed-off-by: Manos Pitsidianakis
---
tests/avocado/acpi-bits/bits-tests/smbios.py2 | 2 +-
tests/avocado/mem-addr-space-check.py | 6 +++---
tests/avocado/reverse_debugging.py| 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/tests/avocado/acpi-bits/bits-tests/smbios.py2
b/tests/avocado/acpi-bits/bits-tests/smbios.py2
index fc623de072..5868a7137a 100644
--- a/tests/avocado/acpi-bits/bits-tests/smbios.py2
+++ b/tests/avocado/acpi-bits/bits-tests/smbios.py2
@@ -1060,7 +1060,7 @@ class EventLogDescriptor(unpack.Struct):
0x16: 'Log Area Reset/Cleared',
0x17: 'System boot',
xrange(0x18, 0x7F): 'Unused, available for assignment',
-xrange(0x80, 0xFE): 'Availalbe for system- and OEM-specific
assignments',
+xrange(0x80, 0xFE): 'Available for system- and OEM-specific
assignments',
0xFF: 'End of log'
}
yield 'log_type', u.unpack_one('B'), unpack.format_table("{}",
_event_log_type_descriptors)
diff --git a/tests/avocado/mem-addr-space-check.py
b/tests/avocado/mem-addr-space-check.py
index 363c3f12a6..af019969c0 100644
--- a/tests/avocado/mem-addr-space-check.py
+++ b/tests/avocado/mem-addr-space-check.py
@@ -165,7 +165,7 @@ def test_phybits_low_tcg_q35_70_amd(self):
For q35-7.0 machines, "above 4G" memory starts are 4G.
pci64_hole size is 32 GiB. Since TCG_PHYS_ADDR_BITS is defined to
be 40, TCG emulated CPUs have maximum of 1 TiB (1024 GiB) of
-directly addressible memory.
+directly addressable memory.
Hence, maxmem value at most can be
1024 GiB - 4 GiB - 1 GiB per slot for alignment - 32 GiB + 0.5 GiB
which is equal to 987.5 GiB. Setting the value to 988 GiB should
@@ -190,7 +190,7 @@ def test_phybits_low_tcg_q35_71_amd(self):
AMD_HT_START is defined to be at 1012 GiB. So for q35 machines
version > 7.0 and AMD cpus, instead of 1024 GiB limit for 40 bit
processor address space, it has to be 1012 GiB , that is 12 GiB
-less than the case above in order to accomodate HT hole.
+less than the case above in order to accommodate HT hole.
Make sure QEMU fails when maxmem size is 976 GiB (12 GiB less
than 988 GiB).
"""
@@ -297,7 +297,7 @@ def test_phybits_ok_tcg_q35_71_amd_41bits(self):
:avocado: tags=arch:x86_64
AMD processor with 41 bits. Max cpu hw address = 2 TiB.
-Same as above but by setting maxram beween 976 GiB and 992 Gib,
+Same as above but by setting maxram between 976 GiB and 992 Gib,
QEMU should start fine.
"""
self.vm.add_args('-S', '-cpu', 'EPYC-v4,phys-bits=41',
diff --git a/tests/avocado/reverse_debugging.py
b/tests/avocado/reverse_debugging.py
index 4cce5a5598..92855a02a5 100644
--- a/tests/avocado/reverse_debugging.py
+++ b/tests/avocado/reverse_debugging.py
@@ -191,7 +191,7 @@ def reverse_debugging(self, shift=7, args=None):
self.check_pc(g, steps[-1])
logger.info('successfully reached %x' % steps[-1])
-logger.info('exitting gdb and qemu')
+logger.info('exiting gdb and qemu')
vm.shutdown()
class ReverseDebugging_X86_64(ReverseDebugging):
--
γαῖα πυρί μιχθήτω
[PATCH v1 04/21] accel/tcg: correct typos
Correct typos automatically found with the `typos` tool <https://crates.io/crates/typos> Signed-off-by: Manos Pitsidianakis --- accel/tcg/ldst_atomicity.c.inc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accel/tcg/ldst_atomicity.c.inc b/accel/tcg/ldst_atomicity.c.inc index 33a04dec52..97dae70d53 100644 --- a/accel/tcg/ldst_atomicity.c.inc +++ b/accel/tcg/ldst_atomicity.c.inc @@ -76,7 +76,7 @@ static int required_atomicity(CPUState *cpu, uintptr_t p, MemOp memop) /* * Examine the alignment of p to determine if there are subobjects * that must be aligned. Note that we only really need ctz4() -- - * any more sigificant bits are discarded by the immediately + * any more significant bits are discarded by the immediately * following comparison. */ tmp = ctz32(p); -- γαῖα πυρί μιχθήτω
[PATCH v1 21/21] target/sparc: correct typos
Correct typos automatically found with the `typos` tool <https://crates.io/crates/typos> Signed-off-by: Manos Pitsidianakis --- target/sparc/asi.h | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/target/sparc/asi.h b/target/sparc/asi.h index 3270ed0c7f..a66829674b 100644 --- a/target/sparc/asi.h +++ b/target/sparc/asi.h @@ -145,14 +145,14 @@ * and later ASIs. */ #define ASI_REAL0x14 /* Real address, cacheable */ -#define ASI_PHYS_USE_EC0x14 /* PADDR, E-cachable */ -#define ASI_REAL_IO 0x15 /* Real address, non-cachable */ +#define ASI_PHYS_USE_EC0x14 /* PADDR, E-cacheable */ +#define ASI_REAL_IO 0x15 /* Real address, non-cacheable */ #define ASI_PHYS_BYPASS_EC_E 0x15 /* PADDR, E-bit*/ #define ASI_BLK_AIUP_4V0x16 /* (4V) Prim, user, block ld/st */ #define ASI_BLK_AIUS_4V0x17 /* (4V) Sec, user, block ld/st */ #define ASI_REAL_L 0x1c /* Real address, cacheable, LE */ -#define ASI_PHYS_USE_EC_L 0x1c /* PADDR, E-cachable, little endian*/ -#define ASI_REAL_IO_L 0x1d /* Real address, non-cachable, LE */ +#define ASI_PHYS_USE_EC_L 0x1c /* PADDR, E-cacheable, little endian*/ +#define ASI_REAL_IO_L 0x1d /* Real address, non-cacheable, LE */ #define ASI_PHYS_BYPASS_EC_E_L 0x1d /* PADDR, E-bit, little endian */ #define ASI_BLK_AIUP_L_4V 0x1e /* (4V) Prim, user, block, l-endian*/ #define ASI_BLK_AIUS_L_4V 0x1f /* (4V) Sec, user, block, l-endian */ -- γαῖα πυρί μιχθήτω
[PATCH v1 07/21] sh4: correct typos
Correct typos automatically found with the `typos` tool <https://crates.io/crates/typos> Signed-off-by: Manos Pitsidianakis --- hw/sh4/sh7750_regs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/sh4/sh7750_regs.h b/hw/sh4/sh7750_regs.h index edb5d18f00..946ad7b3aa 100644 --- a/hw/sh4/sh7750_regs.h +++ b/hw/sh4/sh7750_regs.h @@ -172,7 +172,7 @@ /* - * Exeption-related registers + * Exception-related registers */ /* Immediate data for TRAPA instruction - TRA */ -- γαῖα πυρί μιχθήτω
[PATCH v1 15/21] qapi/ui: correct typos
Correct typos automatically found with the `typos` tool <https://crates.io/crates/typos> Signed-off-by: Manos Pitsidianakis --- qapi/ui.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qapi/ui.json b/qapi/ui.json index b6d7e142b7..1448eaca73 100644 --- a/qapi/ui.json +++ b/qapi/ui.json @@ -63,7 +63,7 @@ ## # @SetPasswordOptionsVnc: # -# Options for set_password specific to the VNC procotol. +# Options for set_password specific to the VNC protocol. # # @display: The id of the display where the password should be # changed. Defaults to the first. @@ -125,7 +125,7 @@ ## # @ExpirePasswordOptionsVnc: # -# Options for expire_password specific to the VNC procotol. +# Options for expire_password specific to the VNC protocol. # # @display: The id of the display where the expiration should be # changed. Defaults to the first. -- γαῖα πυρί μιχθήτω
[PATCH v1 01/21] docs: correct typos
Correct typos automatically found with the `typos` tool <https://crates.io/crates/typos> Signed-off-by: Manos Pitsidianakis --- docs/devel/ci-jobs.rst.inc | 2 +- docs/devel/docs.rst | 2 +- docs/devel/testing.rst | 2 +- docs/interop/prl-xml.txt| 2 +- docs/interop/vhost-user.rst | 2 +- docs/system/devices/canokey.rst | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/devel/ci-jobs.rst.inc b/docs/devel/ci-jobs.rst.inc index 4c39cdb2d9..6678b4f4ef 100644 --- a/docs/devel/ci-jobs.rst.inc +++ b/docs/devel/ci-jobs.rst.inc @@ -147,7 +147,7 @@ Set this variable to 1 to create the pipelines, but leave all the jobs to be manually started from the UI Set this variable to 2 to create the pipelines and run all -the jobs immediately, as was historicaly behaviour +the jobs immediately, as was historically behaviour QEMU_CI_AVOCADO_TESTING ~~~ diff --git a/docs/devel/docs.rst b/docs/devel/docs.rst index 50ff0d67f8..a7768b5311 100644 --- a/docs/devel/docs.rst +++ b/docs/devel/docs.rst @@ -21,7 +21,7 @@ are processed in two ways: The syntax of these ``.hx`` files is simple. It is broadly an alternation of C code put into the C output and rST format text -put into the documention. A few special directives are recognised; +put into the documentation. A few special directives are recognised; these are all-caps and must be at the beginning of the line. ``HXCOMM`` is the comment marker. The line, including any arbitrary diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index bd132306c1..aa96eacec5 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -728,7 +728,7 @@ For example to setup the HPPA ports builds of Debian:: EXECUTABLE=(pwd)/qemu-hppa V=1 The ``DEB_`` variables are substitutions used by -``debian-boostrap.pre`` which is called to do the initial debootstrap +``debian-bootstrap.pre`` which is called to do the initial debootstrap of the rootfs before it is copied into the container. The second stage is run as part of the build. The final image will be tagged as ``qemu/debian-sid-hppa``. diff --git a/docs/interop/prl-xml.txt b/docs/interop/prl-xml.txt index 7031f8752c..cf9b3fba26 100644 --- a/docs/interop/prl-xml.txt +++ b/docs/interop/prl-xml.txt @@ -122,7 +122,7 @@ Each Image element has following child elements: * Type - image type of the element. It can be: "Plain" for raw files. "Compressed" for expanding disks. -* File - path to image file. Path can be relative to DiskDecriptor.xml or +* File - path to image file. Path can be relative to DiskDescriptor.xml or absolute. == Snapshots element == diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index ad6e142f23..d1ed39dfa0 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -989,7 +989,7 @@ When reconnecting: #. If ``d.flags`` is not equal to the calculated flags value (means back-end has submitted the buffer to guest driver before crash, so - it has to commit the in-progres update), set ``old_free_head``, + it has to commit the in-progress update), set ``old_free_head``, ``old_used_idx``, ``old_used_wrap_counter`` to ``free_head``, ``used_idx``, ``used_wrap_counter`` diff --git a/docs/system/devices/canokey.rst b/docs/system/devices/canokey.rst index cfa6186e48..7f3664963f 100644 --- a/docs/system/devices/canokey.rst +++ b/docs/system/devices/canokey.rst @@ -14,7 +14,7 @@ CanoKey [1]_ is an open-source secure key with supports of All these platform-independent features are in canokey-core [3]_. For different platforms, CanoKey has different implementations, -including both hardware implementions and virtual cards: +including both hardware implementations and virtual cards: * CanoKey STM32 [4]_ * CanoKey Pigeon [5]_ -- γαῖα πυρί μιχθήτω
[PATCH v1 19/21] m68k: correct typos
Correct typos automatically found with the `typos` tool <https://crates.io/crates/typos> Signed-off-by: Manos Pitsidianakis --- target/m68k/cpu.h | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h index aca4aa610b..57e3b6d3ce 100644 --- a/target/m68k/cpu.h +++ b/target/m68k/cpu.h @@ -478,10 +478,11 @@ void do_m68k_semihosting(CPUM68KState *env, int nr); * The 68000 family is defined in six main CPU classes, the 680[012346]0. * Generally each successive CPU adds enhanced data/stack/instructions. * However, some features are only common to one, or a few classes. - * The features covers those subsets of instructons. + * The features covers those subsets of instructions. * - * CPU32/32+ are basically 680010 compatible with some 68020 class instructons, - * and some additional CPU32 instructions. Mostly Supervisor state differences. + * CPU32/32+ are basically 680010 compatible with some 68020 class + * instructions, and some additional CPU32 instructions. Mostly Supervisor + * state differences. * * The ColdFire core ISA is a RISC-style reduction of the 68000 series cpu. * There are 4 ColdFire core ISA revisions: A, A+, B and C. -- γαῖα πυρί μιχθήτω
[PATCH v1 13/21] hw/riscv/virt.h: correct typos
Correct typos automatically found with the `typos` tool <https://crates.io/crates/typos> Signed-off-by: Manos Pitsidianakis --- include/hw/riscv/virt.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h index f89790fd58..3db839160f 100644 --- a/include/hw/riscv/virt.h +++ b/include/hw/riscv/virt.h @@ -144,13 +144,13 @@ uint32_t imsic_num_bits(uint32_t count); #define VIRT_IMSIC_GROUP_MAX_SIZE (1U << IMSIC_MMIO_GROUP_MIN_SHIFT) #if VIRT_IMSIC_GROUP_MAX_SIZE < \ IMSIC_GROUP_SIZE(VIRT_CPUS_MAX_BITS, VIRT_IRQCHIP_MAX_GUESTS_BITS) -#error "Can't accomodate single IMSIC group in address space" +#error "Can't accommodate single IMSIC group in address space" #endif #define VIRT_IMSIC_MAX_SIZE(VIRT_SOCKETS_MAX * \ VIRT_IMSIC_GROUP_MAX_SIZE) #if 0x400 < VIRT_IMSIC_MAX_SIZE -#error "Can't accomodate all IMSIC groups in address space" +#error "Can't accommodate all IMSIC groups in address space" #endif #endif -- γαῖα πυρί μιχθήτω
[PATCH v1 06/21] ppc: correct typos
Correct typos automatically found with the `typos` tool
<https://crates.io/crates/typos>
Signed-off-by: Manos Pitsidianakis
---
include/hw/ppc/ppc4xx.h | 2 +-
hw/ppc/ppc405.h | 2 +-
target/ppc/translate/vmx-impl.c.inc | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h
index ea7740239b..c4ecb1652f 100644
--- a/include/hw/ppc/ppc4xx.h
+++ b/include/hw/ppc/ppc4xx.h
@@ -75,7 +75,7 @@ struct Ppc4xxMalState {
uint8_t rxcnum;
};
-/* Peripheral local bus arbitrer */
+/* Peripheral local bus arbiter */
#define TYPE_PPC4xx_PLB "ppc4xx-plb"
OBJECT_DECLARE_SIMPLE_TYPE(Ppc4xxPlbState, PPC4xx_PLB);
struct Ppc4xxPlbState {
diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
index 9a4312691e..a39f0caea1 100644
--- a/hw/ppc/ppc405.h
+++ b/hw/ppc/ppc405.h
@@ -41,7 +41,7 @@ struct Ppc405PobState {
uint32_t besr1;
};
-/* OPB arbitrer */
+/* OPB arbiter */
#define TYPE_PPC405_OPBA "ppc405-opba"
OBJECT_DECLARE_SIMPLE_TYPE(Ppc405OpbaState, PPC405_OPBA);
struct Ppc405OpbaState {
diff --git a/target/ppc/translate/vmx-impl.c.inc
b/target/ppc/translate/vmx-impl.c.inc
index 4b91c3489d..b56e615c24 100644
--- a/target/ppc/translate/vmx-impl.c.inc
+++ b/target/ppc/translate/vmx-impl.c.inc
@@ -1183,7 +1183,7 @@ static void glue(gen_, name)(DisasContext *ctx)
\
/*
* Support for Altivec instructions that use bit 31 (Rc) as an opcode
- * bit but also use bit 21 as an actual Rc bit. In general, thse pairs
+ * bit but also use bit 21 as an actual Rc bit. In general, these pairs
* come from different versions of the ISA, so we must also support a
* pair of flags for each instruction.
*/
--
γαῖα πυρί μιχθήτω
[PATCH v1 16/21] qemu-options.hx: correct typos
Correct typos automatically found with the `typos` tool <https://crates.io/crates/typos> Signed-off-by: Manos Pitsidianakis --- qemu-options.hx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qemu-options.hx b/qemu-options.hx index 8547254dbf..9be1e5817c 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -2468,7 +2468,7 @@ SRST ``to=L`` With this option, QEMU will try next available VNC displays, -until the number L, if the origianlly defined "-vnc display" is +until the number L, if the originally defined "-vnc display" is not available, e.g. port 5900+display is already used by another application. By default, to=0. @@ -5511,7 +5511,7 @@ SRST -object filter-redirector,netdev=hn0,id=redire0,queue=rx,indev=compare_out -object filter-redirector,netdev=hn0,id=redire1,queue=rx,outdev=compare0 -object iothread,id=iothread1 --object colo-compare,id=comp0,primary_in=compare0-0,secondary_in=compare1,outdev=compare_out0,notify_dev=nofity_way,iothread=iothread1 +-object colo-compare,id=comp0,primary_in=compare0-0,secondary_in=compare1,outdev=compare_out0,notify_dev=notify_way,iothread=iothread1 secondary: -netdev tap,id=hn0,vhost=off -- γαῖα πυρί μιχθήτω
[PATCH v1 08/21] include/exec/memory.h: correct typos
Correct typos automatically found with the `typos` tool <https://crates.io/crates/typos> Signed-off-by: Manos Pitsidianakis --- include/exec/memory.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/exec/memory.h b/include/exec/memory.h index 177be23db7..8626a355b3 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -3146,7 +3146,7 @@ int ram_block_discard_require(bool state); /* * See ram_block_discard_require(): only inhibit technologies that disable - * uncoordinated discarding of pages in RAM blocks, allowing co-existance with + * uncoordinated discarding of pages in RAM blocks, allowing co-existence with * technologies that only inhibit uncoordinated discards (via the * RamDiscardManager). */ -- γαῖα πυρί μιχθήτω
[PATCH v1 18/21] hexagon: correct typos
Correct typos automatically found with the `typos` tool <https://crates.io/crates/typos> Signed-off-by: Manos Pitsidianakis --- target/hexagon/idef-parser/macros.inc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/hexagon/idef-parser/macros.inc b/target/hexagon/idef-parser/macros.inc index 7478d4db17..94975d9583 100644 --- a/target/hexagon/idef-parser/macros.inc +++ b/target/hexagon/idef-parser/macros.inc @@ -127,5 +127,5 @@ /* Include fHIDE macros which hide type declarations */ #define fHIDE(A) A -/* Purge non-relavant parts */ +/* Purge non-relevant parts */ #define fBRANCH_SPECULATE_STALL(A, B, C, D, E) -- γαῖα πυρί μιχθήτω
[PATCH v1 00/21] Trivial tree wide typo fixes
Correct typos automatically found with the `typos` tool <https://crates.io/crates/typos> Manos Pitsidianakis (21): docs: correct typos tests: correct typos Xen headers: correct typos accel/tcg: correct typos loongson3: correct typos ppc: correct typos sh4: correct typos include/exec/memory.h: correct typos include/exec/plugin-gen.h: correct typos hw/arm/omap.h: correct typos hw/cxl/cxl_device.h: correct typos hw/net/npcm_gmac.h: correct typos hw/riscv/virt.h: correct typos pc-bios/README: correct typos qapi/ui: correct typos qemu-options.hx: correct typos ci/gitlab-pipeline-status: correct typos hexagon: correct typos m68k: correct typos s390x: correct typos target/sparc: correct typos include/exec/memory.h | 2 +- include/exec/plugin-gen.h | 2 +- include/hw/arm/omap.h | 2 +- include/hw/cxl/cxl_device.h| 4 ++-- include/hw/net/npcm_gmac.h | 4 ++-- include/hw/ppc/ppc4xx.h| 2 +- include/hw/riscv/virt.h| 4 ++-- include/hw/xen/interface/arch-x86/xen-x86_64.h | 2 +- include/hw/xen/interface/arch-x86/xen.h| 2 +- include/hw/xen/interface/event_channel.h | 2 +- include/hw/xen/interface/grant_table.h | 2 +- include/hw/xen/interface/hvm/hvm_op.h | 2 +- include/hw/xen/interface/io/blkif.h| 4 ++-- include/hw/xen/interface/io/fbif.h | 2 +- include/hw/xen/interface/io/kbdif.h| 2 +- include/hw/xen/interface/io/ring.h | 2 +- include/hw/xen/interface/memory.h | 2 +- include/hw/xen/interface/physdev.h | 4 ++-- include/hw/xen/interface/xen.h | 4 ++-- accel/tcg/ldst_atomicity.c.inc | 2 +- docs/devel/ci-jobs.rst.inc | 2 +- docs/devel/docs.rst| 2 +- docs/devel/testing.rst | 2 +- docs/interop/prl-xml.txt | 2 +- docs/interop/vhost-user.rst| 2 +- docs/system/devices/canokey.rst| 2 +- hw/mips/loongson3_bootp.h | 4 ++-- hw/ppc/ppc405.h| 2 +- hw/sh4/sh7750_regs.h | 2 +- pc-bios/README | 2 +- qapi/ui.json | 4 ++-- qemu-options.hx| 4 ++-- scripts/ci/gitlab-pipeline-status | 2 +- target/hexagon/idef-parser/macros.inc | 2 +- target/m68k/cpu.h | 7 --- target/ppc/translate/vmx-impl.c.inc| 2 +- target/s390x/cpu_features_def.h.inc| 2 +- target/sparc/asi.h | 8 tests/avocado/acpi-bits/bits-tests/smbios.py2 | 2 +- tests/avocado/mem-addr-space-check.py | 6 +++--- tests/avocado/reverse_debugging.py | 2 +- 41 files changed, 58 insertions(+), 57 deletions(-) base-commit: da96ad4a6a2ef26c83b15fa95e7fceef5147269c -- γαῖα πυρί μιχθήτω
[PATCH v1 05/21] loongson3: correct typos
Correct typos automatically found with the `typos` tool
<https://crates.io/crates/typos>
Signed-off-by: Manos Pitsidianakis
---
hw/mips/loongson3_bootp.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/mips/loongson3_bootp.h b/hw/mips/loongson3_bootp.h
index d525ab745a..1b0dd3b591 100644
--- a/hw/mips/loongson3_bootp.h
+++ b/hw/mips/loongson3_bootp.h
@@ -25,7 +25,7 @@
struct efi_memory_map_loongson {
uint16_t vers; /* version of efi_memory_map */
uint32_t nr_map; /* number of memory_maps */
-uint32_t mem_freq; /* memory frequence */
+uint32_t mem_freq; /* memory frequency */
struct mem_map {
uint32_t node_id;/* node_id which memory attached to */
uint32_t mem_type; /* system memory, pci memory, pci io, etc. */
@@ -156,7 +156,7 @@ struct board_devices {
struct loongson_special_attribute {
uint16_t vers; /* version of this special */
-char special_name[64]; /* special_atribute_name */
+char special_name[64]; /* special_attribute_name */
uint32_t loongson_special_type; /* type of special device */
/* for each device's resource */
struct resource_loongson resource[MAX_RESOURCE_NUMBER];
--
γαῖα πυρί μιχθήτω
Re: [PATCH v2 07/11] hw/audio/virtio-sound: add stream state variable
Hello Volker,
On Sun, 18 Feb 2024 at 10:34, Volker Rümelin wrote:
>
> So far, only rudimentary checks have been made to ensure that
> the guest only performs state transitions permitted in
> virtio-v1.2-csd01 5.14.6.6.1 PCM Command Lifecycle.
5.14.6.6.1 is non-normative in virtio v1.2. You can even see it's not
in device requirements. The state transitions were thus not checked on
purpose.
There is nothing in the standard that describes error conditions
pertaining to the "PCM lifecycle" state machine. It really should, but
it doesn't, unfortunately.
> Add a state
> variable per audio stream and check all state transitions.
>
> Because only permitted state transitions are possible, only one
> copy of the audio stream parameters is required and these do not
> need to be initialised with default values.
>
> The state variable will also make it easier to restore the audio
> stream after migration.
I suggest you instead add the state tracking but only use it for the
post_load code for migration.
> Signed-off-by: Volker Rümelin
> ---
> hw/audio/virtio-snd.c | 213 ++
> include/hw/audio/virtio-snd.h | 20 +---
> 2 files changed, 111 insertions(+), 122 deletions(-)
>
> diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
> index 435ce26430..bbbdd01aa9 100644
> --- a/hw/audio/virtio-snd.c
> +++ b/hw/audio/virtio-snd.c
> @@ -31,11 +31,30 @@
> #define VIRTIO_SOUND_CHMAP_DEFAULT 0
> #define VIRTIO_SOUND_HDA_FN_NID 0
>
> +#define VSND_PCMSTREAM_STATE_F_PARAMS_SET 0x1
> +#define VSND_PCMSTREAM_STATE_F_PREPARED0x2
> +#define VSND_PCMSTREAM_STATE_F_ACTIVE 0x4
> +
> +#define VSND_PCMSTREAM_STATE_UNINITIALIZED 0
> +#define VSND_PCMSTREAM_STATE_PARAMS_SET(1 \
> + |
> VSND_PCMSTREAM_STATE_F_PARAMS_SET)
> +#define VSND_PCMSTREAM_STATE_PREPARED (2 \
> + |
> VSND_PCMSTREAM_STATE_F_PARAMS_SET \
> + | VSND_PCMSTREAM_STATE_F_PREPARED)
> +#define VSND_PCMSTREAM_STATE_STARTED (4 \
> + |
> VSND_PCMSTREAM_STATE_F_PARAMS_SET \
> + | VSND_PCMSTREAM_STATE_F_PREPARED
> \
> + | VSND_PCMSTREAM_STATE_F_ACTIVE)
> +#define VSND_PCMSTREAM_STATE_STOPPED (6 \
> + |
> VSND_PCMSTREAM_STATE_F_PARAMS_SET \
> + | VSND_PCMSTREAM_STATE_F_PREPARED)
> +#define VSND_PCMSTREAM_STATE_RELEASED (7 \
> + |
> VSND_PCMSTREAM_STATE_F_PARAMS_SET)
> +
> static void virtio_snd_pcm_out_cb(void *data, int available);
> static void virtio_snd_process_cmdq(VirtIOSound *s);
> static void virtio_snd_pcm_flush(VirtIOSoundPCMStream *stream);
> static void virtio_snd_pcm_in_cb(void *data, int available);
> -static void virtio_snd_unrealize(DeviceState *dev);
>
> static uint32_t supported_formats = BIT(VIRTIO_SND_PCM_FMT_S8)
>| BIT(VIRTIO_SND_PCM_FMT_U8)
> @@ -153,8 +172,8 @@ virtio_snd_ctrl_cmd_free(virtio_snd_ctrl_command *cmd)
> static VirtIOSoundPCMStream *virtio_snd_pcm_get_stream(VirtIOSound *s,
> uint32_t stream_id)
> {
> -return stream_id >= s->snd_conf.streams ? NULL :
> -s->pcm->streams[stream_id];
> +return stream_id >= s->snd_conf.streams ? NULL
> +: >streams[stream_id];
> }
>
> /*
> @@ -167,7 +186,7 @@ static virtio_snd_pcm_set_params
> *virtio_snd_pcm_get_params(VirtIOSound *s,
> uint32_t
> stream_id)
> {
> return stream_id >= s->snd_conf.streams ? NULL
> -: >pcm->pcm_params[stream_id];
> +: >streams[stream_id].params;
> }
>
> /*
> @@ -253,11 +272,10 @@ static void virtio_snd_handle_pcm_info(VirtIOSound *s,
>
> /*
> * Set the given stream params.
> - * Called by both virtio_snd_handle_pcm_set_params and during device
> - * initialization.
> * Returns the response status code. (VIRTIO_SND_S_*).
> *
> * @s: VirtIOSound device
> + * @stream_id: stream id
> * @params: The PCM params as defined in the virtio specification
> */
> static
> @@ -265,9 +283,10 @@ uint32_t virtio_snd_set_pcm_params(VirtIOSound *s,
> uint32_t stream_id,
> virtio_snd_pcm_set_params *params)
> {
> +VirtIOSoundPCMStream *stream;
> virtio_snd_pcm_set_params *st_params;
>
> -if (stream_id >= s->snd_conf.streams || s->pcm->pcm_params == NULL) {
> +if (stream_id >= s->snd_conf.streams) {
> /*
> * TODO: do we need to set DEVICE_NEEDS_RESET?
> */
> @@ -275,7 +294,17 @@ uint32_t virtio_snd_set_pcm_params(VirtIOSound *s,
> return cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
> }
Re: [PATCH v2 02/11] hw/audio/virtio-sound: fix segmentation fault in tx/rx xfer handler
Hello Volker, thanks for working on this,
On Sun, 18 Feb 2024 at 10:33, Volker Rümelin wrote:
>
> A malicious guest may trigger a segmentation fault in the tx/rx xfer
> handlers. On handler entry the stream variable is initialized with
> NULL. If the first element of the virtio queue has an invalid size
> or an invalid stream id, the error handling code dereferences the
> stream variable NULL pointer.
Why not just add a bounds check and a null check instead?
>
> Don't try to handle the invalid virtio queue element with a stream
> queue. Instead, push the invalid queue element back to the guest
> immediately.
IIRC this will result in an infinite loop, because the code is
emptying the vq until virtqueue_pop returns NULL.
So if you add the invalid message back, the vq will never be empty.
Eventually you will loop over all invalid messages forever.
(Please correct me if I'm wrong of course!)
>
> Signed-off-by: Volker Rümelin
> ---
> hw/audio/virtio-snd.c | 100 ++
> include/hw/audio/virtio-snd.h | 1 -
> 2 files changed, 29 insertions(+), 72 deletions(-)
>
> diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
> index e604d8f30c..b87653daf4 100644
> --- a/hw/audio/virtio-snd.c
> +++ b/hw/audio/virtio-snd.c
> @@ -456,7 +456,6 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s,
> uint32_t stream_id)
> stream->s = s;
> qemu_mutex_init(>queue_mutex);
> QSIMPLEQ_INIT(>queue);
> -QSIMPLEQ_INIT(>invalid);
>
> /*
> * stream_id >= s->snd_conf.streams was checked before so this is
> @@ -611,9 +610,6 @@ static size_t
> virtio_snd_pcm_get_io_msgs_count(VirtIOSoundPCMStream *stream)
> QSIMPLEQ_FOREACH_SAFE(buffer, >queue, entry, next) {
> count += 1;
> }
> -QSIMPLEQ_FOREACH_SAFE(buffer, >invalid, entry, next) {
> -count += 1;
> -}
> }
> return count;
> }
> @@ -831,47 +827,19 @@ static void virtio_snd_handle_event(VirtIODevice *vdev,
> VirtQueue *vq)
> trace_virtio_snd_handle_event();
> }
>
> -static inline void empty_invalid_queue(VirtIODevice *vdev, VirtQueue *vq)
> +static void push_bad_msg_resp(VirtQueue *vq, VirtQueueElement *elem)
> {
> -VirtIOSoundPCMBuffer *buffer = NULL;
> -VirtIOSoundPCMStream *stream = NULL;
> virtio_snd_pcm_status resp = { 0 };
> -VirtIOSound *vsnd = VIRTIO_SND(vdev);
> -bool any = false;
> -
> -for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
> -stream = vsnd->pcm->streams[i];
> -if (stream) {
> -any = false;
> -WITH_QEMU_LOCK_GUARD(>queue_mutex) {
> -while (!QSIMPLEQ_EMPTY(>invalid)) {
> -buffer = QSIMPLEQ_FIRST(>invalid);
> -if (buffer->vq != vq) {
> -break;
> -}
> -any = true;
> -resp.status = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
> -iov_from_buf(buffer->elem->in_sg,
> - buffer->elem->in_num,
> - 0,
> - ,
> - sizeof(virtio_snd_pcm_status));
> -virtqueue_push(vq,
> - buffer->elem,
> - sizeof(virtio_snd_pcm_status));
> -QSIMPLEQ_REMOVE_HEAD(>invalid, entry);
> -virtio_snd_pcm_buffer_free(buffer);
> -}
> -if (any) {
> -/*
> - * Notify vq about virtio_snd_pcm_status responses.
> - * Buffer responses must be notified separately later.
> - */
> -virtio_notify(vdev, vq);
> -}
> -}
> -}
> -}
> +size_t msg_sz;
> +
> +resp.status = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
> +msg_sz = iov_from_buf(elem->in_sg,
> + elem->in_num,
> + 0,
> + ,
> + sizeof(virtio_snd_pcm_status));
> +virtqueue_push(vq, elem, msg_sz);
> +g_free(elem);
> }
>
> /*
> @@ -890,11 +858,7 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice
> *vdev, VirtQueue *vq)
> size_t msg_sz, size;
> virtio_snd_pcm_xfer hdr;
> uint32_t stream_id;
> -/*
> - * If any of the I/O messages are invalid, put them in stream->invalid
> and
> - * return them after the for loop.
> - */
> -bool must_empty_invalid_queue = false;
> +bool notify = false;
>
> if (!virtio_queue_ready(vq)) {
> return;
> @@ -942,17 +906,16 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice
> *vdev, VirtQueue *vq)
> continue;
>
> tx_err:
> -WITH_QEMU_LOCK_GUARD(>queue_mutex) {
> -must_empty_invalid_queue = true;
> -buffer =
[PATCH] Print tool binary names in ./configure --help
configure --help currently outputs the following line for the tools
option:
-->8---
░░tcg░TCG░support░░
tools build support utilities that come with QEMU
░░tpm░TPM░support░░
░░u2f░U2F░emulation░support
---8<--
Which does not convey information if you don't already know what these
utilities are going to be.
This commit uses script/meson-buildoptions.py to parse the hard-coded
test binary names in meson.build and update the --help output to include
their names, like as follows:
-->8---
░░tcg░TCG░support░░
tools build utility tool binaries like qemu-edid, qemu-img,
qemu-io, qemu-nbd, qemu-bridge-helper, qemu-pr-helper
░░tpm░TPM░support░░
░░u2f░U2F░emulation░support
---8<--
Since it uses the meson.build AST to find those values, only hard-coded
binary names are selected and the description is non-exhaustive.
Signed-off-by: Manos Pitsidianakis
---
Makefile | 8 +--
meson_options.txt | 2 +-
scripts/meson-buildoptions.py | 43 ---
scripts/meson-buildoptions.sh | 3 ++-
4 files changed, 49 insertions(+), 7 deletions(-)
diff --git a/Makefile b/Makefile
index 8f36990335..79ab594c4b 100644
--- a/Makefile
+++ b/Makefile
@@ -128,8 +128,12 @@ Makefile.mtest: build.ninja scripts/mtest2make.py
.PHONY: update-buildoptions
all update-buildoptions: $(SRC_PATH)/scripts/meson-buildoptions.sh
$(SRC_PATH)/scripts/meson-buildoptions.sh: $(SRC_PATH)/meson_options.txt
- $(MESON) introspect --buildoptions $(SRC_PATH)/meson.build | $(PYTHON) \
- scripts/meson-buildoptions.py > $@.tmp && mv $@.tmp $@
+ { printf '{"buildoptions":'; \
+ $(MESON) introspect --buildoptions $(SRC_PATH)/meson.build 2>
>(grep -v "Unable to evaluate subdir(\[\])" >&2) \
+ && printf ',"ast":' \
+ && $(MESON) introspect --ast $(SRC_PATH)/meson.build 2> >(grep
-v "Unable to evaluate subdir(\[\])" >&2) \
+ && printf "}" ; } \
+ | $(PYTHON) scripts/meson-buildoptions.py > $@.tmp && mv $@.tmp
$@
endif
# 4. Rules to bridge to other makefiles
diff --git a/meson_options.txt b/meson_options.txt
index 0a99a059ec..53a8b6b3e2 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -58,7 +58,7 @@ option('guest_agent', type : 'feature', value : 'auto',
option('guest_agent_msi', type : 'feature', value : 'auto',
description: 'Build MSI package for the QEMU Guest Agent')
option('tools', type : 'feature', value : 'auto',
- description: 'build support utilities that come with QEMU')
+ description: 'build utility tool binaries')
option('qga_vss', type : 'feature', value: 'auto',
description: 'build QGA VSS support (broken with MinGW)')
diff --git a/scripts/meson-buildoptions.py b/scripts/meson-buildoptions.py
index 4814a8ff61..4abdfc1d05 100644
--- a/scripts/meson-buildoptions.py
+++ b/scripts/meson-buildoptions.py
@@ -24,6 +24,7 @@
import textwrap
import shlex
import sys
+from collections import deque
# Options with nonstandard names (e.g. --with/--without) or OS-dependent
# defaults. Try not to add any.
@@ -182,7 +183,7 @@ def cli_metavar(opt):
return "CHOICE"
-def print_help(options):
+def print_help(options, tools: list[str]):
print("meson_options_help() {")
feature_opts = []
for opt in sorted(options, key=cli_help_key):
@@ -212,6 +213,8 @@ def print_help(options):
sh_print()
for opt in sorted(feature_opts, key=cli_option):
key = cli_option(opt)
+if key == "tools":
+opt["description"] += " like " + ", ".join(tools)
help_line(key, opt, 18, False)
print("}")
@@ -242,7 +245,41 @@ def print_parse(options):
print("}")
-options = load_options(json.load(sys.stdin))
+# Returns hard-coded executables from meson.build AST
+def tool_executables(d: dict) -> list[str]:
+def is_executable_fn_call(i: dict) -> bool:
+if not (
+"name" in i
+and i["name"] == "executable"
+and "node" in i
+and i["node"] == "FunctionNode"
+and "positional" in i[&q
Re: [PATCH] i386: xen: fix compilation --without-default-devices
On Fri, 09 Feb 2024 23:59, Paolo Bonzini wrote: The xenpv machine type requires XEN_BUS, so select it. Signed-off-by: Paolo Bonzini --- accel/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/accel/Kconfig b/accel/Kconfig index a30cf2eb483..794e0d18d21 100644 --- a/accel/Kconfig +++ b/accel/Kconfig @@ -16,3 +16,4 @@ config KVM config XEN bool select FSDEV_9P if VIRTFS +select XEN_BUS -- 2.43.0 Reviewed-by: Manos Pitsidianakis
Re: [PATCH] pcie: Support PCIe Gen5/Gen6 link speeds
On Thu, 15 Feb 2024 03:23, Lukas Stockner wrote:
This patch extends the PCIe link speed option so that slots can be
configured as supporting 32GT/s (Gen5) or 64GT/s (Gen5) speeds.
This is as simple as setting the appropriate bit in LnkCap2 and
the appropriate value in LnkCap and LnkCtl2.
Signed-off-by: Lukas Stockner
---
hw/core/qdev-properties-system.c | 16 ++--
hw/pci/pcie.c| 8
include/hw/pci/pcie_regs.h | 2 ++
qapi/common.json | 6 +-
4 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 1a396521d5..106a31c233 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -941,7 +941,7 @@ const PropertyInfo qdev_prop_off_auto_pcibar = {
.set_default_value = qdev_propinfo_set_default_value_enum,
};
-/* --- PCIELinkSpeed 2_5/5/8/16 -- */
+/* --- PCIELinkSpeed 2_5/5/8/16/32/64 -- */
static void get_prop_pcielinkspeed(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
@@ -963,6 +963,12 @@ static void get_prop_pcielinkspeed(Object *obj, Visitor
*v, const char *name,
case QEMU_PCI_EXP_LNK_16GT:
speed = PCIE_LINK_SPEED_16;
break;
+case QEMU_PCI_EXP_LNK_32GT:
+speed = PCIE_LINK_SPEED_32;
+break;
+case QEMU_PCI_EXP_LNK_64GT:
+speed = PCIE_LINK_SPEED_64;
+break;
default:
/* Unreachable */
abort();
@@ -996,6 +1002,12 @@ static void set_prop_pcielinkspeed(Object *obj, Visitor
*v, const char *name,
case PCIE_LINK_SPEED_16:
*p = QEMU_PCI_EXP_LNK_16GT;
break;
+case PCIE_LINK_SPEED_32:
+*p = QEMU_PCI_EXP_LNK_32GT;
+break;
+case PCIE_LINK_SPEED_64:
+*p = QEMU_PCI_EXP_LNK_64GT;
+break;
default:
/* Unreachable */
abort();
@@ -1004,7 +1016,7 @@ static void set_prop_pcielinkspeed(Object *obj, Visitor
*v, const char *name,
const PropertyInfo qdev_prop_pcie_link_speed = {
.name = "PCIELinkSpeed",
-.description = "2_5/5/8/16",
+.description = "2_5/5/8/16/32/64",
.enum_table = _lookup,
.get = get_prop_pcielinkspeed,
.set = set_prop_pcielinkspeed,
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 6db0cf69cd..0b4817e144 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -153,6 +153,14 @@ static void pcie_cap_fill_slot_lnk(PCIDevice *dev)
pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP2,
PCI_EXP_LNKCAP2_SLS_16_0GB);
}
+if (s->speed > QEMU_PCI_EXP_LNK_16GT) {
+pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP2,
+ PCI_EXP_LNKCAP2_SLS_32_0GB);
+}
+if (s->speed > QEMU_PCI_EXP_LNK_32GT) {
+pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP2,
+ PCI_EXP_LNKCAP2_SLS_64_0GB);
+}
}
}
diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
index 4972106c42..9d3b6868dc 100644
--- a/include/hw/pci/pcie_regs.h
+++ b/include/hw/pci/pcie_regs.h
@@ -39,6 +39,8 @@ typedef enum PCIExpLinkSpeed {
QEMU_PCI_EXP_LNK_5GT,
QEMU_PCI_EXP_LNK_8GT,
QEMU_PCI_EXP_LNK_16GT,
+QEMU_PCI_EXP_LNK_32GT,
+QEMU_PCI_EXP_LNK_64GT,
} PCIExpLinkSpeed;
#define QEMU_PCI_EXP_LNKCAP_MLS(speed) (speed)
diff --git a/qapi/common.json b/qapi/common.json
index f1bb841951..867a9ad9b0 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -107,10 +107,14 @@
#
# @16: 16.0GT/s
#
+# @32: 32.0GT/s
+#
+# @64: 64.0GT/s
+#
# Since: 4.0
##
{ 'enum': 'PCIELinkSpeed',
- 'data': [ '2_5', '5', '8', '16' ] }
+ 'data': [ '2_5', '5', '8', '16', '32', '64' ] }
##
# @PCIELinkWidth:
--
2.43.1
Reviewed-by: Manos Pitsidianakis
Re: [PATCH v3 1/1] target: Add system emulation aiming to target any architecture
u-param.h
new file mode 100644
index 00..42e38ae991
--- /dev/null
+++ b/target/any/cpu-param.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef ANY_CPU_PARAM_H
+#define ANY_CPU_PARAM_H
+
+#define TARGET_LONG_BITS 64
+
+#define TARGET_PHYS_ADDR_SPACE_BITS 64 /* MAX(targets) */
+#define TARGET_VIRT_ADDR_SPACE_BITS 64 /* MAX(targets) */
+
+#define TARGET_PAGE_BITS_VARY
+#define TARGET_PAGE_BITS_MIN 10 /* MIN(targets)=ARMv5/ARMv6, ignoring AVR */
+
+#endif
Nit:
-#endif
+#endif /* ANY_CPU_PARAM_H */
(And for the other #endifs following)
diff --git a/target/any/cpu-qom.h b/target/any/cpu-qom.h
new file mode 100644
index 00..18d6a85de9
--- /dev/null
+++ b/target/any/cpu-qom.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef QEMU_DUMMY_CPU_QOM_H
+#define QEMU_DUMMY_CPU_QOM_H
+
+#include "hw/core/cpu.h"
+#include "qom/object.h"
+
+#define TYPE_DUMMY_CPU "dummy-cpu"
+
+OBJECT_DECLARE_CPU_TYPE(DUMMYCPU, CPUClass, DUMMY_CPU)
+
+#endif
diff --git a/target/any/cpu.h b/target/any/cpu.h
new file mode 100644
index 00..e8abb8891f
--- /dev/null
+++ b/target/any/cpu.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef TARGET_ANY_CPU_H
+#define TARGET_ANY_CPU_H
+
+#include "cpu-qom.h"
+#include "exec/cpu-defs.h"
+
+#define DUMMY_CPU_TYPE_SUFFIX "-" TYPE_DUMMY_CPU
+#define DUMMY_CPU_TYPE_NAME(name) (name DUMMY_CPU_TYPE_SUFFIX)
+#define CPU_RESOLVING_TYPE TYPE_DUMMY_CPU
+
+struct CPUArchState {
+/* nothing here */
+};
+
+struct ArchCPU {
+CPUState parent_obj;
+
+/* Properties */
?
+CPUArchState env;
+};
+
+#include "exec/cpu-all.h" /* FIXME remove once exec/ headers cleaned */
+
+#endif
diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index f56df59c94..208625d4d5 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -729,3 +729,23 @@ pages:
- public
variables:
QEMU_JOB_PUBLISH: 1
+
+build-system-any:
+ extends:
+- .native_build_job_template
+ needs:
+- job: amd64-alpine-container
+ variables:
+IMAGE: alpine
+TARGETS: any-softmmu
+MAKE_CHECK_ARGS: check-qtest
+CONFIGURE_ARGS:
+ --disable-tools
+ --disable-hvf
+ --disable-kvm
+ --disable-nvmm
+ --disable-tcg
+ --disable-whpx
+ --disable-xen
+ --with-default-devices
+ --enable-qom-cast-debug
diff --git a/hw/any/meson.build b/hw/any/meson.build
new file mode 100644
index 00..60e1567e53
--- /dev/null
+++ b/hw/any/meson.build
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+any_ss = ss.source_set()
+
+hw_arch += {'any': any_ss}
diff --git a/hw/meson.build b/hw/meson.build
index 463d702683..644938 100644
--- a/hw/meson.build
+++ b/hw/meson.build
@@ -47,6 +47,7 @@ subdir('xenpv')
subdir('fsi')
subdir('alpha')
+subdir('any')
subdir('arm')
subdir('avr')
subdir('cris')
diff --git a/target/Kconfig b/target/Kconfig
index 83da0bd293..09109c4884 100644
--- a/target/Kconfig
+++ b/target/Kconfig
@@ -1,4 +1,5 @@
source alpha/Kconfig
+source any/Kconfig
source arm/Kconfig
source avr/Kconfig
source cris/Kconfig
diff --git a/target/any/Kconfig b/target/any/Kconfig
new file mode 100644
index 00..8840d70e55
--- /dev/null
+++ b/target/any/Kconfig
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+config ANY
+bool
diff --git a/target/any/meson.build b/target/any/meson.build
new file mode 100644
index 00..4f5422d3a3
--- /dev/null
+++ b/target/any/meson.build
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+any_ss = ss.source_set()
+any_system_ss = ss.source_set()
+
+target_arch += {'any': any_ss}
+target_system_arch += {'any': any_system_ss}
diff --git a/target/meson.build b/target/meson.build
index dee2ac47e0..c75b91e1b9 100644
--- a/target/meson.build
+++ b/target/meson.build
@@ -1,4 +1,5 @@
subdir('alpha')
+subdir('any')
subdir('arm')
subdir('avr')
subdir('cris')
--
2.41.0
LGTM in general overall. In case this wasn't discussed already, would it
be a good idea to name the target x-any if it ends up in a stable
release?
Regardless of my inlined style comments:
Reviewed-by: Manos Pitsidianakis
[PATCH] system/physmem: remove redundant arg reassignment
Arguments `ram_block` are reassigned to local declarations `block`
without further use. Remove re-assignment to reduce noise.
Signed-off-by: Manos Pitsidianakis
---
system/physmem.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/system/physmem.c b/system/physmem.c
index 5e66d9ae36..d4c3bfac65 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -2154,10 +2154,8 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
*
* Called within RCU critical section.
*/
-void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
+void *qemu_map_ram_ptr(RAMBlock *block, ram_addr_t addr)
{
-RAMBlock *block = ram_block;
-
if (block == NULL) {
block = qemu_get_ram_block(addr);
addr -= block->offset;
@@ -2182,10 +2180,9 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t
addr)
*
* Called within RCU critical section.
*/
-static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
+static void *qemu_ram_ptr_length(RAMBlock *block, ram_addr_t addr,
hwaddr *size, bool lock)
{
-RAMBlock *block = ram_block;
if (*size == 0) {
return NULL;
}
base-commit: 5767815218efd3cbfd409505ed824d5f356044ae
--
γαῖα πυρί μιχθήτω
Re: [RFC/INCOMPLETE PATCH 0/8] Attempt to make qemu-img options consistent and --help working
Hello Michael, Such changes are long overdue. However given the complexity of commands and arguments, maybe it'd be a good idea to write a code generator for the command line interface, This way you could also generate --help outputs, manpages, shell completions just from the command line spec we'd use to generate the argv parsing routines. On Wed, 7 Feb 2024 at 19:58, Michael Tokarev wrote: > > This is an incomplete first attempt only, there's a lot left to do. > > All the options in qemu-img is a complete mess, - no, inconsistent or > incomplete syntax in documentation, many undocumented options, option > names are used inconsistently and differently for different commands, > no long options exists for many short options, --help output is a huge > mess by its own, and the way how it all is generated is another story. > docs/tools/qemu-img.rst with qemu-img-opts.hx is yet another. > > I hoped to fix just an option or two, but it ended up in a large task, > and I need some help and discussion, hence the RFC. > > The idea is: > > - create more or less consistent set of options between different >subcommands > - provide long options which can be used without figuring out which >-T/-t, -f|-F|-O etc to use for which of the two images given > - have qemu-img --help provide just a list of subcommands > - have qemu-img COMMAND --help to describe just this subcommand > - get rid of qemu-img-opts.hx, instead finish documentation in >qemu-img.rst based on the actual options implemented in >qemu-img.c. > > I started converting subcommands one by one, providing long options > and --help output. And immediately faced some questions which needs > wider discussion. > > o We have --image-opts and --target-image-opts. Do we really need both? >In my view, --image-opts should be sort of global, turning *all* >filenames on this command line into complete image specifications, >there's no need to have separate image-opts and --target-image-opts. >Don't know what to do wrt compatibility though. It shouldn't be made >this way from the beginning. As a possible solution, introduce a new >option and deprecate current set. > > o For conversion (convert, dd, etc), we've source and destination, >so it's easy to distinguish using long options, like --source-format >--target-cache etc (-t/-T -f/-F is a mess here already). What to >do with compare? --format1 --format2 is ugly, maybe --a-format and >--b-format? Maybe we can get off with --source (a) and --target (b) >instead of filename1 & filename2? >(--cache in this context is global for both). > > o qemu-img convert. It's the most messy one, and it is not really >documented (nor in qemu-img.rst , eg there's nothing in there about >FILENAME2, -B is difficult to understand, etc). >At the very least, I'd say the options should be > --source-format, --source-cache etc > --target-format, --target-options > --target-image-opts - this shold go IMHO > > o check and commit - inconsistent cache flags? >In convert, cache is backwards (source/target)? > > At first, I tried to have more or less common option descriptions, > using various parameters, variables or #defines, but in different > commands the same options has slightly different wording, and in > some option names are different, so it looks like it's best to > have complete text in each subcommand. > > > Michael Tokarev (8): > qemu-img: pass current cmdname into command handlers > qemu-img: refresh options/--help for "create" subcommand > qemu-img: factor out parse_output_format() and use it in the code >(this one has been sent in a separate patch, here it is just for > completness) > qemu-img: refresh options/--help for "check" command > qemu-img: simplify --repair error message > qemu-img: refresh options/--help for "commit" command > qemu-img: refresh options/--help for "compare" command > qemu-img: refresh options/--help for "convert" command > > qemu-img.c | 352 ++--- > 1 file changed, 226 insertions(+), 126 deletions(-) > > -- > 2.39.2 > > -- Manos Pitsidianakis Emulation and Virtualization Engineer at Linaro Ltd
Re: [PATCH 1/5] virtio-blk: enforce iothread-vq-mapping validation
IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
- AioContext **vq_aio_context, uint16_t num_queues)
+/**
+ * apply_iothread_vq_mapping:
+ * @iothread_vq_mapping_list: The mapping of virtqueues to IOThreads.
+ * @vq_aio_context: The array of AioContext pointers to fill in.
+ * @num_queues: The length of @vq_aio_context.
+ * @errp: If an error occurs, a pointer to the area to store the error.
+ *
+ * Fill in the AioContext for each virtqueue in the @vq_aio_context array given
+ * the iothread-vq-mapping parameter in @iothread_vq_mapping_list.
+ *
+ * Returns: %true on success, %false on failure.
+ **/
+static bool apply_iothread_vq_mapping(
+IOThreadVirtQueueMappingList *iothread_vq_mapping_list,
+AioContext **vq_aio_context,
+uint16_t num_queues,
+Error **errp)
{
IOThreadVirtQueueMappingList *node;
size_t num_iothreads = 0;
size_t cur_iothread = 0;
+if (!validate_iothread_vq_mapping_list(iothread_vq_mapping_list,
+ num_queues, errp)) {
+return false;
+}
+
for (node = iothread_vq_mapping_list; node; node = node->next) {
num_iothreads++;
}
@@ -1638,6 +1656,7 @@ apply_vq_mapping(IOThreadVirtQueueMappingList
*iothread_vq_mapping_list,
/* Explicit vq:IOThread assignment */
for (vq = node->value->vqs; vq; vq = vq->next) {
+assert(vq->value < num_queues);
vq_aio_context[vq->value] = ctx;
}
} else {
@@ -1650,6 +1669,8 @@ apply_vq_mapping(IOThreadVirtQueueMappingList
*iothread_vq_mapping_list,
cur_iothread++;
}
+
+return true;
}
/* Context: BQL held */
@@ -1660,6 +1681,14 @@ static bool virtio_blk_vq_aio_context_init(VirtIOBlock
*s, Error **errp)
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+if (conf->iothread && conf->iothread_vq_mapping_list) {
+if (conf->iothread) {
+error_setg(errp, "iothread and iothread-vq-mapping properties "
+ "cannot be set at the same time");
+return false;
+}
+}
+
if (conf->iothread || conf->iothread_vq_mapping_list) {
if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
error_setg(errp,
@@ -1685,8 +1714,14 @@ static bool virtio_blk_vq_aio_context_init(VirtIOBlock
*s, Error **errp)
s->vq_aio_context = g_new(AioContext *, conf->num_queues);
if (conf->iothread_vq_mapping_list) {
-apply_vq_mapping(conf->iothread_vq_mapping_list, s->vq_aio_context,
- conf->num_queues);
+if (!apply_iothread_vq_mapping(conf->iothread_vq_mapping_list,
+ s->vq_aio_context,
+ conf->num_queues,
+ errp)) {
+g_free(s->vq_aio_context);
+s->vq_aio_context = NULL;
+return false;
+}
} else if (conf->iothread) {
AioContext *ctx = iothread_get_aio_context(conf->iothread);
for (unsigned i = 0; i < conf->num_queues; i++) {
@@ -1996,19 +2031,6 @@ static void virtio_blk_device_realize(DeviceState *dev,
Error **errp)
return;
}
-if (conf->iothread_vq_mapping_list) {
-if (conf->iothread) {
-error_setg(errp, "iothread and iothread-vq-mapping properties "
- "cannot be set at the same time");
-return;
-}
-
-if (!validate_iothread_vq_mapping_list(conf->iothread_vq_mapping_list,
- conf->num_queues, errp)) {
-return;
-}
-}
-
s->config_size = virtio_get_config_size(_blk_cfg_size_params,
s->host_features);
virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
--
2.43.0
virtio_block_ops and methods are moved around without changes in the
diff, is that on purpose? If no the patch and history would be less
noisy.
Regardless:
Reviewed-by: Manos Pitsidianakis
Re: [PATCH 5/5] monitor: use aio_co_reschedule_self()
On Mon, 05 Feb 2024 19:26, Stefan Hajnoczi wrote:
The aio_co_reschedule_self() API is designed to avoid the race
condition between scheduling the coroutine in another AioContext and
yielding.
The QMP dispatch code uses the open-coded version that appears
susceptible to the race condition at first glance:
aio_co_schedule(qemu_get_aio_context(), qemu_coroutine_self());
qemu_coroutine_yield();
The code is actually safe because the iohandler and qemu_aio_context
AioContext run under the Big QEMU Lock. Nevertheless, set a good example
and use aio_co_reschedule_self() so it's obvious that there is no race.
Suggested-by: Hanna Reitz
Signed-off-by: Stefan Hajnoczi
---
qapi/qmp-dispatch.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 176b549473..f3488afeef 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -212,8 +212,7 @@ QDict *coroutine_mixed_fn qmp_dispatch(const QmpCommandList
*cmds, QObject *requ
* executing the command handler so that it can make progress if it
* involves an AIO_WAIT_WHILE().
*/
-aio_co_schedule(qemu_get_aio_context(), qemu_coroutine_self());
-qemu_coroutine_yield();
+aio_co_reschedule_self(qemu_get_aio_context());
}
monitor_set_cur(qemu_coroutine_self(), cur_mon);
@@ -227,9 +226,7 @@ QDict *coroutine_mixed_fn qmp_dispatch(const QmpCommandList
*cmds, QObject *requ
* Move back to iohandler_ctx so that nested event loops for
* qemu_aio_context don't start new monitor commands.
*/
-aio_co_schedule(iohandler_get_aio_context(),
-qemu_coroutine_self());
-qemu_coroutine_yield();
+aio_co_reschedule_self(iohandler_get_aio_context());
}
} else {
/*
--
2.43.0
Reviewed-by: Manos Pitsidianakis
Re: [PATCH 2/5] virtio-blk: clarify that there is at least 1 virtqueue
On Mon, 05 Feb 2024 19:26, Stefan Hajnoczi wrote:
It is not possible to instantiate a virtio-blk device with 0 virtqueues.
The following check is located in ->realize():
if (!conf->num_queues) {
error_setg(errp, "num-queues property must be larger than 0");
return;
}
Later on we access s->vq_aio_context[0] under the assumption that there
is as least one virtqueue. Hanna Czenczek noted that
it would help to show that the array index is already valid.
Add an assertion to document that s->vq_aio_context[0] is always
safe...and catch future code changes that break this assumption.
Suggested-by: Hanna Czenczek
Signed-off-by: Stefan Hajnoczi
---
hw/block/virtio-blk.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e8b37fd5f4..a0735a9bca 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1825,6 +1825,7 @@ static int virtio_blk_start_ioeventfd(VirtIODevice *vdev)
* Try to change the AioContext so that block jobs and other operations can
* co-locate their activity in the same AioContext. If it fails, nevermind.
*/
+assert(nvqs > 0); /* enforced during ->realize() */
r = blk_set_aio_context(s->conf.conf.blk, s->vq_aio_context[0],
_err);
if (r < 0) {
--
2.43.0
Reviewed-by: Manos Pitsidianakis
Re: [PATCH 3/5] virtio-blk: add vq_rq[] bounds check in virtio_blk_dma_restart_cb()
On Mon, 05 Feb 2024 19:26, Stefan Hajnoczi wrote:
Hanna Czenczek noted that the array index in
virtio_blk_dma_restart_cb() is not bounds-checked:
g_autofree VirtIOBlockReq **vq_rq = g_new0(VirtIOBlockReq *, num_queues);
...
while (rq) {
VirtIOBlockReq *next = rq->next;
uint16_t idx = virtio_get_queue_index(rq->vq);
rq->next = vq_rq[idx];
^^
The code is correct because both rq->vq and vq_rq[] depend on
num_queues, but this is indirect and not 100% obvious. Add an assertion.
This sentence could be useful as an inline comment too.
Suggested-by: Hanna Czenczek
Signed-off-by: Stefan Hajnoczi
---
hw/block/virtio-blk.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index a0735a9bca..f3193f4b75 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1209,6 +1209,7 @@ static void virtio_blk_dma_restart_cb(void *opaque, bool
running,
VirtIOBlockReq *next = rq->next;
uint16_t idx = virtio_get_queue_index(rq->vq);
+assert(idx < num_queues);
rq->next = vq_rq[idx];
vq_rq[idx] = rq;
rq = next;
--
2.43.0
Reviewed-by: Manos Pitsidianakis
Re: [PATCH 4/5] virtio-blk: declare VirtIOBlock::rq with a type
On Mon, 05 Feb 2024 19:26, Stefan Hajnoczi wrote:
The VirtIOBlock::rq field has had the type void * since its introduction
in commit 869a5c6df19a ("Stop VM on error in virtio-blk. (Gleb
Natapov)").
Perhaps this was done to avoid the forward declaration of
VirtIOBlockReq.
Hanna Czenczek pointed out the missing type. Specify
the actual type because there is no need to use void * here.
Suggested-by: Hanna Czenczek
Signed-off-by: Stefan Hajnoczi
---
include/hw/virtio/virtio-blk.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 833a9a344f..5c14110c4b 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -55,7 +55,7 @@ struct VirtIOBlock {
VirtIODevice parent_obj;
BlockBackend *blk;
QemuMutex rq_lock;
-void *rq; /* protected by rq_lock */
+struct VirtIOBlockReq *rq; /* protected by rq_lock */
VirtIOBlkConf conf;
unsigned short sector_mask;
bool original_wce;
--
2.43.0
Reviewed-by: Manos Pitsidianakis
Re: Help understanding allocation and mapping flow of virtio-gpu 3D resource blobs
Good morning Alex, Just one observation, On Sun, 04 Feb 2024 13:06, Alex Bennée wrote: Hi, I'm trying to get an understanding of the blob allocation and mapping flow for virtio-gpu for Vulkan and Rutabaga. Having gotten all the various libraries setup I'm still seeing failures when running a TCG guest (buildroot + latest glm, mesa, vkmark) with: ./qemu-system-aarch64 \ -M virt -cpu cortex-a76 \ -m 8192 \ -object memory-backend-memfd,id=mem,size=8G,share=on \ -serial mon:stdio \ -kernel ~/lsrc/linux.git/builds/arm64.initramfs/arch/arm64/boot/Image \ -append "console=ttyAMA0" \ -device virtio-gpu-gl,context_init=true,blob=true,hostmem=4G \ -display sdl,gl=on -d guest_errors,trace:virtio_gpu_cmd_res\*,trace:virtio_gpu_virgl_process_command -D debug.log which shows up as detected in dmesg but not to vulkaninfo: [0.644879] virtio-pci :00:01.0: enabling device ( -> 0003) [0.648643] virtio-pci :00:02.0: enabling device ( -> 0002) [0.672391] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled [0.678071] Serial: AMBA driver [0.682122] [drm] pci: virtio-gpu-pci detected at :00:02.0 [0.683249] [drm] Host memory window: 0x80 +0x1 [0.683420] [drm] features: +virgl +edid +resource_blob +host_visible [0.683470] [drm] features: +context_init [0.695695] [drm] number of scanouts: 1 [0.695837] [drm] number of cap sets: 3 [0.716173] [drm] cap set 0: id 1, max-version 1, max-size 308 [0.716499] [drm] cap set 1: id 2, max-version 2, max-size 1384 [0.716686] [drm] cap set 2: id 4, max-version 0, max-size 160 [0.726001] [drm] Initialized virtio_gpu 0.1.0 0 for :00:02.0 on minor 0 virgl_resource_create: err=0, res=2 virgl_renderer_resource_attach_iov: 0x55b843c17a80/2 virgl_resource_attach_iov: pipe_resource: 0x55b8434da8f0 vrend_pipe_resource_attach_iov: 0x43 ... # vulkaninfo --summary WARNING: [Loader Message] Code 0 : terminator_CreateInstance: Failed to CreateInstance in ICD 1. Skipping ICD. a common problem I have when testing different mesa builds is not declaring the intended driver each time. I could be getting errors like yours but if I set the VK_ICD_FILENAMES env var to the correct driver manifest (the installed icd.d/virtio-*.json file from my mesa build) the device is properly recognized. Might be unrelated to this error, but still it helps to define it explicitly each time. error: XDG_RUNTIME_DIR is invalid or not set in the environment. == VULKANINFO == Vulkan Instance Version: 1.3.262 Instance Extensions: count = 12 --- VK_EXT_debug_report: extension revision 10 VK_EXT_debug_utils : extension revision 2 VK_KHR_device_group_creation : extension revision 1 VK_KHR_external_fence_capabilities : extension revision 1 VK_KHR_external_memory_capabilities: extension revision 1 VK_KHR_external_semaphore_capabilities : extension revision 1 VK_KHR_get_physical_device_properties2 : extension revision 2 VK_KHR_get_surface_capabilities2 : extension revision 1 VK_KHR_portability_enumeration : extension revision 1 VK_KHR_surface : extension revision 25 VK_KHR_surface_protected_capabilities : extension revision 1 VK_LUNARG_direct_driver_loading: extension revision 1 Instance Layers: Devices: GPU0: apiVersion = 1.3.267 driverVersion = 0.0.1 vendorID = 0x10005 deviceID = 0x deviceType = PHYSICAL_DEVICE_TYPE_CPU deviceName = llvmpipe (LLVM 15.0.3, 128 bits) driverID = DRIVER_ID_MESA_LLVMPIPE driverName = llvmpipe driverInfo = Mesa 23.3.2 (LLVM 15.0.3) conformanceVersion = 1.3.1.1 deviceUUID = 6d657361-3233-2e33-2e32- driverUUID = 6c6c766d-7069-7065--4944 With an older and more hacked up set of the blob patches I can get vulkaninfo to work but I see multiple GPUs and vkmark falls over when mapping stuff: # vulkaninfo --summary render_state_create_resource: res_id = 5 vkr_context_add_resource: res_id = 5 vkr_context_import_resource_internal: res_id = 5 virgl_resource_create: err=0, res=5 render_state_create_resource: res_id = 6 vkr_context_add_resource: res_id = 6 vkr_context_import_resource_internal: res_id = 6 virgl_resource_create: err=0, res=6 error: XDG_RUNTIME_DIR is invalid or not set in the environment. == VULKANINFO == Vulkan Instance Version: 1.3.262 Instance Extensions: count = 12 --- VK_EXT_debug_report: extension revision 10 VK_EXT_debug_utils :
[PATCH v3 3/3] virtio-gpu-rutabaga.c: override resource_destroy method
When the Rutabaga GPU device frees resources, it calls
rutabaga_resource_unref for that resource_id. However, when the generic
VirtIOGPU functions destroys resources, it only removes the
virtio_gpu_simple_resource from the device's VirtIOGPU->reslist list.
The rutabaga resource associated with that resource_id is then leaked.
This commit overrides the resource_destroy class method introduced in
the previous commit to fix this.
Signed-off-by: Manos Pitsidianakis
---
hw/display/virtio-gpu-rutabaga.c | 47
1 file changed, 35 insertions(+), 12 deletions(-)
diff --git a/hw/display/virtio-gpu-rutabaga.c b/hw/display/virtio-gpu-rutabaga.c
index 9e67f9bd51..17bf701a21 100644
--- a/hw/display/virtio-gpu-rutabaga.c
+++ b/hw/display/virtio-gpu-rutabaga.c
@@ -148,14 +148,38 @@ rutabaga_cmd_create_resource_3d(VirtIOGPU *g,
}
static void
+virtio_gpu_rutabaga_resource_unref(VirtIOGPU *g,
+ struct virtio_gpu_simple_resource *res,
+ Error **errp)
+{
+int32_t result;
+VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g);
+
+result = rutabaga_resource_unref(vr->rutabaga, res->resource_id);
+if (result) {
+error_setg_errno(errp,
+(int)result,
+"%s: rutabaga_resource_unref returned %"PRIi32
+" for resource_id = %"PRIu32, __func__, result,
+res->resource_id);
+}
+
+if (res->image) {
+pixman_image_unref(res->image);
+}
+
+QTAILQ_REMOVE(>reslist, res, next);
+g_free(res);
+}
+
+static void
rutabaga_cmd_resource_unref(VirtIOGPU *g,
struct virtio_gpu_ctrl_command *cmd)
{
-int32_t result;
+int32_t result = 0;
struct virtio_gpu_simple_resource *res;
struct virtio_gpu_resource_unref unref;
-
-VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g);
+Error *local_err = NULL;
VIRTIO_GPU_FILL_CMD(unref);
@@ -164,15 +188,14 @@ rutabaga_cmd_resource_unref(VirtIOGPU *g,
res = virtio_gpu_find_resource(g, unref.resource_id);
CHECK(res, cmd);
-result = rutabaga_resource_unref(vr->rutabaga, unref.resource_id);
-CHECK(!result, cmd);
-
-if (res->image) {
-pixman_image_unref(res->image);
+virtio_gpu_rutabaga_resource_unref(g, res, _err);
+if (local_err) {
+error_report_err(local_err);
+/* local_err was freed, do not reuse it. */
+local_err = NULL;
+result = 1;
}
-
-QTAILQ_REMOVE(>reslist, res, next);
-g_free(res);
+CHECK(!result, cmd);
}
static void
@@ -1099,7 +1122,7 @@ static void virtio_gpu_rutabaga_class_init(ObjectClass
*klass, void *data)
vgc->handle_ctrl = virtio_gpu_rutabaga_handle_ctrl;
vgc->process_cmd = virtio_gpu_rutabaga_process_cmd;
vgc->update_cursor_data = virtio_gpu_rutabaga_update_cursor;
-
+vgc->resource_destroy = virtio_gpu_rutabaga_resource_unref;
vdc->realize = virtio_gpu_rutabaga_realize;
device_class_set_props(dc, virtio_gpu_rutabaga_properties);
}
--
γαῖα πυρί μιχθήτω
[PATCH v3 2/3] virtio-gpu.c: add resource_destroy class method
When destroying/unrefing resources, devices such as virtio-gpu-rutabaga
need to do their own bookkeeping (free rutabaga resources that are
associated with the virtio_gpu_simple_resource).
This commit adds a class method so that virtio-gpu-rutabaga can override
it in the next commit.
Reviewed-by: Marc-André Lureau
Signed-off-by: Manos Pitsidianakis
---
include/hw/virtio/virtio-gpu.h | 3 +++
hw/display/virtio-gpu.c| 25 ++---
2 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 584ba2ed73..b28e7ef0d2 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -219,6 +219,9 @@ struct VirtIOGPUClass {
void (*update_cursor_data)(VirtIOGPU *g,
struct virtio_gpu_scanout *s,
uint32_t resource_id);
+void (*resource_destroy)(VirtIOGPU *g,
+ struct virtio_gpu_simple_resource *res,
+ Error **errp);
};
struct VirtIOGPUGL {
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 2b73ae585b..1c1ee230b3 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -402,7 +402,8 @@ static void virtio_gpu_disable_scanout(VirtIOGPU *g, int
scanout_id)
}
static void virtio_gpu_resource_destroy(VirtIOGPU *g,
-struct virtio_gpu_simple_resource *res)
+struct virtio_gpu_simple_resource *res,
+Error **errp)
{
int i;
@@ -438,7 +439,11 @@ static void virtio_gpu_resource_unref(VirtIOGPU *g,
cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
return;
}
-virtio_gpu_resource_destroy(g, res);
+/*
+ * virtio_gpu_resource_destroy does not set any errors, so pass a NULL errp
+ * to ignore them.
+ */
+virtio_gpu_resource_destroy(g, res, NULL);
}
static void virtio_gpu_transfer_to_host_2d(VirtIOGPU *g,
@@ -1488,11 +1493,24 @@ static void virtio_gpu_device_unrealize(DeviceState
*qdev)
static void virtio_gpu_reset_bh(void *opaque)
{
VirtIOGPU *g = VIRTIO_GPU(opaque);
+VirtIOGPUClass *vgc = VIRTIO_GPU_GET_CLASS(g);
struct virtio_gpu_simple_resource *res, *tmp;
+uint32_t resource_id;
+Error *local_err = NULL;
int i = 0;
QTAILQ_FOREACH_SAFE(res, >reslist, next, tmp) {
-virtio_gpu_resource_destroy(g, res);
+resource_id = res->resource_id;
+vgc->resource_destroy(g, res, _err);
+if (local_err) {
+error_append_hint(_err, "%s: %s resource_destroy"
+ "for resource_id = %"PRIu32" failed.\n",
+ __func__, object_get_typename(OBJECT(g)),
+ resource_id);
+/* error_report_err frees the error object for us */
+error_report_err(local_err);
+local_err = NULL;
+}
}
for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
@@ -1632,6 +1650,7 @@ static void virtio_gpu_class_init(ObjectClass *klass,
void *data)
vgc->handle_ctrl = virtio_gpu_handle_ctrl;
vgc->process_cmd = virtio_gpu_simple_process_cmd;
vgc->update_cursor_data = virtio_gpu_update_cursor_data;
+vgc->resource_destroy = virtio_gpu_resource_destroy;
vgbc->gl_flushed = virtio_gpu_handle_gl_flushed;
vdc->realize = virtio_gpu_device_realize;
--
γαῖα πυρί μιχθήτω
[PATCH v3 1/3] hw/display/virtio-gpu.c: use reset_bh class method
While the VirtioGPU type has a reset_bh field to specify a reset
callback, it's never used. virtio_gpu_reset() calls the general
virtio_gpu_reset_bh() function for all devices that inherit from
VirtioGPU.
While no devices override reset_bh at the moment, a device reset might
require special logic for implementations in the future.
Reviewed-by: Marc-André Lureau
Signed-off-by: Manos Pitsidianakis
---
hw/display/virtio-gpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index f8a675eb30..2b73ae585b 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1515,7 +1515,7 @@ void virtio_gpu_reset(VirtIODevice *vdev)
qemu_cond_wait_bql(>reset_cond);
}
} else {
-virtio_gpu_reset_bh(g);
+aio_bh_call(g->reset_bh);
}
while (!QTAILQ_EMPTY(>cmdq)) {
--
γαῖα πυρί μιχθήτω
[PATCH v3 0/3] Fix resource freeing bugs in virtio-gpu-rutabaga
While testing the rutabaga gpu device, we noticed that if the device is
reset, it stops working and complains about missing resource ids. A
quick investigation discovered that the generic VirtIOGPU implementation
frees all resources, but for Rutabaga, they are tied with rutabaga
objects that need to be destroyed as well.
This series adds a resource_destroy class method that the Rutabaga
device can override and do its own bookkeeping.
v2 -> v3 differences:
- use error_setg_errno in virtio-gpu-rutabaga.c
resource_destroy method. (Thanks Marc-André
Lureau !)
v1 -> v2 differences:
- addressed review comments re: using the Error API for the
resource_destroy method.
Manos Pitsidianakis (3):
hw/display/virtio-gpu.c: use reset_bh class method
virtio-gpu.c: add resource_destroy class method
virtio-gpu-rutabaga.c: override resource_destroy method
include/hw/virtio/virtio-gpu.h | 3 ++
hw/display/virtio-gpu-rutabaga.c | 47
hw/display/virtio-gpu.c | 27 +++---
3 files changed, 61 insertions(+), 16 deletions(-)
Range-diff against v2:
1: 5893fb45d1 = 1: 87fb4fa72c hw/display/virtio-gpu.c: use reset_bh class
method
2: 78b15e8f7e ! 2: b0a86630c4 virtio-gpu.c: add resource_destroy class method
@@ Commit message
This commit adds a class method so that virtio-gpu-rutabaga can
override
it in the next commit.
+Reviewed-by: Marc-André Lureau
Signed-off-by: Manos Pitsidianakis
## include/hw/virtio/virtio-gpu.h ##
3: 926db899be ! 3: e3778e44c9 virtio-gpu-rutabaga.c: override
resource_destroy method
@@ hw/display/virtio-gpu-rutabaga.c:
rutabaga_cmd_create_resource_3d(VirtIOGPU *g,
+ Error **errp)
+{
+int32_t result;
-+const char *strerror = NULL;
+VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g);
+
+result = rutabaga_resource_unref(vr->rutabaga, res->resource_id);
+if (result) {
-+error_setg(errp, "%s: rutabaga_resource_unref returned %"PRIi32
-+ " for resource_id = %"PRIu32, __func__, result,
-+ res->resource_id);
-+strerror = strerrorname_np((int)result);
-+if (strerror != NULL) {
-+error_append_hint(errp, "%s: %s\n",
-+ strerror, strerrordesc_np((int)result) ? :
"");
-+}
++error_setg_errno(errp,
++(int)result,
++"%s: rutabaga_resource_unref returned %"PRIi32
++" for resource_id = %"PRIu32, __func__, result,
++res->resource_id);
+}
+
+if (res->image) {
base-commit: 11be70677c70fdccd452a3233653949b79e97908
--
γαῖα πυρί μιχθήτω
Re: [PATCH v2 3/3] virtio-gpu-rutabaga.c: override resource_destroy method
On Tue, 30 Jan 2024 at 15:10, Marc-André Lureau
wrote:
>
> Hi
>
> On Tue, Jan 30, 2024 at 5:01 PM Manos Pitsidianakis
> wrote:
> >
> > On Tue, 30 Jan 2024 at 14:50, Marc-André Lureau
> > wrote:
> > >
> > > Hi
> > >
> > > On Mon, Jan 29, 2024 at 7:46 PM Manos Pitsidianakis
> > > wrote:
> > > >
> > > > When the Rutabaga GPU device frees resources, it calls
> > > > rutabaga_resource_unref for that resource_id. However, when the generic
> > > > VirtIOGPU functions destroys resources, it only removes the
> > > > virtio_gpu_simple_resource from the device's VirtIOGPU->reslist list.
> > > > The rutabaga resource associated with that resource_id is then leaked.
> > > >
> > > > This commit overrides the resource_destroy class method introduced in
> > > > the previous commit to fix this.
> > > >
> > > > Signed-off-by: Manos Pitsidianakis
> > > > ---
> > > > hw/display/virtio-gpu-rutabaga.c | 51
> > > > 1 file changed, 39 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/hw/display/virtio-gpu-rutabaga.c
> > > > b/hw/display/virtio-gpu-rutabaga.c
> > > > index 9e67f9bd51..6ac0776005 100644
> > > > --- a/hw/display/virtio-gpu-rutabaga.c
> > > > +++ b/hw/display/virtio-gpu-rutabaga.c
> > > > @@ -148,14 +148,42 @@ rutabaga_cmd_create_resource_3d(VirtIOGPU *g,
> > > > }
> > > >
> > > > static void
> > > > +virtio_gpu_rutabaga_resource_unref(VirtIOGPU *g,
> > > > + struct virtio_gpu_simple_resource
> > > > *res,
> > > > + Error **errp)
> > > > +{
> > > > +int32_t result;
> > > > +const char *strerror = NULL;
> > > > +VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g);
> > > > +
> > > > +result = rutabaga_resource_unref(vr->rutabaga, res->resource_id);
> > > > +if (result) {
> > > > +error_setg(errp, "%s: rutabaga_resource_unref returned %"PRIi32
> > > > + " for resource_id = %"PRIu32, __func__, result,
> > > > + res->resource_id);
> > > > +strerror = strerrorname_np((int)result);
> > > > +if (strerror != NULL) {
> > > > +error_append_hint(errp, "%s: %s\n",
> > > > + strerror, strerrordesc_np((int)result) ?
> > > > : "");
> > > > +}
> > > > +}
> > >
> > > There is error_setg_errno()
> >
> > I did not use it on purpose because I was not certain if rutabaga_gfx
> > starts using other error numbers in the future. I don't like my
> > approach but I don't like assuming it's an errno either to be
> > honest... Which one seems better to you?
> >
>
> In that case, don't use strerrordesc_np() either :)
strerrordesc_np will be valid if strerrorname_np() does not return NULL.
> I think we can assume they will keep using errno though, unless they
> clearly communicate this and break API, which seems unlikely to me.
I will use error_setg_errno then, thanks!
Manos
> > Thanks,
> > Manos
> >
> > > > +
> > > > +if (res->image) {
> > > > +pixman_image_unref(res->image);
> > > > +}
> > > > +
> > > > +QTAILQ_REMOVE(>reslist, res, next);
> > > > +g_free(res);
> > > > +}
> > > > +
> > > > +static void
> > > > rutabaga_cmd_resource_unref(VirtIOGPU *g,
> > > > struct virtio_gpu_ctrl_command *cmd)
> > > > {
> > > > -int32_t result;
> > > > +int32_t result = 0;
> > > > struct virtio_gpu_simple_resource *res;
> > > > struct virtio_gpu_resource_unref unref;
> > > > -
> > > > -VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g);
> > > > +Error *local_err = NULL;
> > > >
> > > > VIRTIO_GPU_FILL_CMD(unref);
> > > >
> > > > @@ -164,15 +192,14 @@ rutabaga_cmd_resource_unref(VirtIOGPU *g,
> > > > res = virtio_gpu_find_resource(g, unref.resource_id);
> > > > CHECK(res, cmd);
> > > >
> > > > -result = rutabaga_resource
Re: [PATCH v2 3/3] virtio-gpu-rutabaga.c: override resource_destroy method
On Tue, 30 Jan 2024 at 14:50, Marc-André Lureau
wrote:
>
> Hi
>
> On Mon, Jan 29, 2024 at 7:46 PM Manos Pitsidianakis
> wrote:
> >
> > When the Rutabaga GPU device frees resources, it calls
> > rutabaga_resource_unref for that resource_id. However, when the generic
> > VirtIOGPU functions destroys resources, it only removes the
> > virtio_gpu_simple_resource from the device's VirtIOGPU->reslist list.
> > The rutabaga resource associated with that resource_id is then leaked.
> >
> > This commit overrides the resource_destroy class method introduced in
> > the previous commit to fix this.
> >
> > Signed-off-by: Manos Pitsidianakis
> > ---
> > hw/display/virtio-gpu-rutabaga.c | 51
> > 1 file changed, 39 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu-rutabaga.c
> > b/hw/display/virtio-gpu-rutabaga.c
> > index 9e67f9bd51..6ac0776005 100644
> > --- a/hw/display/virtio-gpu-rutabaga.c
> > +++ b/hw/display/virtio-gpu-rutabaga.c
> > @@ -148,14 +148,42 @@ rutabaga_cmd_create_resource_3d(VirtIOGPU *g,
> > }
> >
> > static void
> > +virtio_gpu_rutabaga_resource_unref(VirtIOGPU *g,
> > + struct virtio_gpu_simple_resource *res,
> > + Error **errp)
> > +{
> > +int32_t result;
> > +const char *strerror = NULL;
> > +VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g);
> > +
> > +result = rutabaga_resource_unref(vr->rutabaga, res->resource_id);
> > +if (result) {
> > +error_setg(errp, "%s: rutabaga_resource_unref returned %"PRIi32
> > + " for resource_id = %"PRIu32, __func__, result,
> > + res->resource_id);
> > +strerror = strerrorname_np((int)result);
> > +if (strerror != NULL) {
> > +error_append_hint(errp, "%s: %s\n",
> > + strerror, strerrordesc_np((int)result) ? :
> > "");
> > +}
> > +}
>
> There is error_setg_errno()
I did not use it on purpose because I was not certain if rutabaga_gfx
starts using other error numbers in the future. I don't like my
approach but I don't like assuming it's an errno either to be
honest... Which one seems better to you?
Thanks,
Manos
> > +
> > +if (res->image) {
> > +pixman_image_unref(res->image);
> > +}
> > +
> > +QTAILQ_REMOVE(>reslist, res, next);
> > +g_free(res);
> > +}
> > +
> > +static void
> > rutabaga_cmd_resource_unref(VirtIOGPU *g,
> > struct virtio_gpu_ctrl_command *cmd)
> > {
> > -int32_t result;
> > +int32_t result = 0;
> > struct virtio_gpu_simple_resource *res;
> > struct virtio_gpu_resource_unref unref;
> > -
> > -VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g);
> > +Error *local_err = NULL;
> >
> > VIRTIO_GPU_FILL_CMD(unref);
> >
> > @@ -164,15 +192,14 @@ rutabaga_cmd_resource_unref(VirtIOGPU *g,
> > res = virtio_gpu_find_resource(g, unref.resource_id);
> > CHECK(res, cmd);
> >
> > -result = rutabaga_resource_unref(vr->rutabaga, unref.resource_id);
> > -CHECK(!result, cmd);
> > -
> > -if (res->image) {
> > -pixman_image_unref(res->image);
> > +virtio_gpu_rutabaga_resource_unref(g, res, _err);
> > +if (local_err) {
> > +error_report_err(local_err);
> > +/* local_err was freed, do not reuse it. */
> > +local_err = NULL;
> > +result = 1;
> > }
> > -
> > -QTAILQ_REMOVE(>reslist, res, next);
> > -g_free(res);
> > +CHECK(!result, cmd);
> > }
> >
> > static void
> > @@ -1099,7 +1126,7 @@ static void
> > virtio_gpu_rutabaga_class_init(ObjectClass *klass, void *data)
> > vgc->handle_ctrl = virtio_gpu_rutabaga_handle_ctrl;
> > vgc->process_cmd = virtio_gpu_rutabaga_process_cmd;
> > vgc->update_cursor_data = virtio_gpu_rutabaga_update_cursor;
> > -
> > +vgc->resource_destroy = virtio_gpu_rutabaga_resource_unref;
> > vdc->realize = virtio_gpu_rutabaga_realize;
> > device_class_set_props(dc, virtio_gpu_rutabaga_properties);
> > }
> > --
> > γαῖα πυρί μιχθήτω
> >
>
>
> --
> Marc-André Lureau
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
On Tue, 30 Jan 2024 at 12:57, Peter Maydell wrote: > > On Tue, 30 Jan 2024 at 10:51, Manos Pitsidianakis > wrote: > > > > On Tue, 30 Jan 2024 at 12:42, Peter Maydell > > wrote: > > > > > > On Tue, 30 Jan 2024 at 10:39, Manos Pitsidianakis > > > wrote: > > > > > > > > On Tue, 30 Jan 2024 at 12:34, Peter Maydell > > > > wrote: > > > > > > > > > > On Tue, 30 Jan 2024 at 10:11, Manos Pitsidianakis > > > > > wrote: > > > > > > > > > > > > Check if a file argument is a cover letter patch produced by > > > > > > git-format-patch --cover-letter; It is initialized with subject > > > > > > suffix " > > > > > > *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they > > > > > > exist, warn the user. > > > > > > > > > > FWIW, as far as I can see from my email archive, this particular > > > > > mistake has been made by contributors to qemu-devel perhaps > > > > > half a dozen times at most in the last decade... > > > > > > > > > > thanks > > > > > -- PMM > > > > > > > > Peter, searching for `b:"BLURB HERE"` in lore.kernel.org yields about > > > > 170 results including these patches. > > > > > > > > https://lore.kernel.org/qemu-devel/?q=b%3A%22BLURB+HERE%22 > > > > > > Yes, there's a few more 'blurb here' results than 'subject here' > > > results, but they're almost always just where the submitter did > > > provide a proper blurb but then forgot to delete the 'BLURB HERE' > > > line, rather than where there's no blurb at all. > > > > Though you said half a dozen times at most. > > Yes, because I was counting 'subject here'. > > My question here is really "how much do we care about having > checkpatch point out this error?". > > thanks > -- PMM I do, because it gives some peace of mind. But I do not care so much that I'd want to continue this conversation further. -- Manos Pitsidianakis Emulation and Virtualization Engineer at Linaro Ltd
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
On Tue, 30 Jan 2024 at 12:42, Peter Maydell wrote: > > On Tue, 30 Jan 2024 at 10:39, Manos Pitsidianakis > wrote: > > > > On Tue, 30 Jan 2024 at 12:34, Peter Maydell > > wrote: > > > > > > On Tue, 30 Jan 2024 at 10:11, Manos Pitsidianakis > > > wrote: > > > > > > > > Check if a file argument is a cover letter patch produced by > > > > git-format-patch --cover-letter; It is initialized with subject suffix " > > > > *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they > > > > exist, warn the user. > > > > > > FWIW, as far as I can see from my email archive, this particular > > > mistake has been made by contributors to qemu-devel perhaps > > > half a dozen times at most in the last decade... > > > > > > thanks > > > -- PMM > > > > Peter, searching for `b:"BLURB HERE"` in lore.kernel.org yields about > > 170 results including these patches. > > > > https://lore.kernel.org/qemu-devel/?q=b%3A%22BLURB+HERE%22 > > Yes, there's a few more 'blurb here' results than 'subject here' > results, but they're almost always just where the submitter did > provide a proper blurb but then forgot to delete the 'BLURB HERE' > line, rather than where there's no blurb at all. Though you said half a dozen times at most. In general the only comments so far are examples of "moving the goalposts" fallacy, where the argument changes each time and the discussion changes topic every time. https://en.wikipedia.org/wiki/Moving_the_goalposts I know it's not anyone's intention in this case, but I'd like to remind everyone that this can be perceived negatively by contributors and demotivate them from contributing to QEMU at all. Let's keep the discussion constructive instead of dismissive. I say this in a completely friendly manner, no negativity intended. Manos
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
On Tue, 30 Jan 2024 at 12:34, Peter Maydell wrote: > > On Tue, 30 Jan 2024 at 10:11, Manos Pitsidianakis > wrote: > > > > Check if a file argument is a cover letter patch produced by > > git-format-patch --cover-letter; It is initialized with subject suffix " > > *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they > > exist, warn the user. > > FWIW, as far as I can see from my email archive, this particular > mistake has been made by contributors to qemu-devel perhaps > half a dozen times at most in the last decade... > > thanks > -- PMM Peter, searching for `b:"BLURB HERE"` in lore.kernel.org yields about 170 results including these patches. https://lore.kernel.org/qemu-devel/?q=b%3A%22BLURB+HERE%22
Re: [PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
On Tue, 30 Jan 2024 12:15, "Daniel P. Berrangé" wrote:
On Tue, Jan 30, 2024 at 12:11:07PM +0200, Manos Pitsidianakis wrote:
Check if a file argument is a cover letter patch produced by
git-format-patch --cover-letter; It is initialized with subject suffix "
*** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
exist, warn the user.
Signed-off-by: Manos Pitsidianakis
---
Range-diff against v1:
1: 64b7ec2287 ! 1: 9bf816eb4c scripts/checkpatch.pl: check for placeholders
in cover letter patches
@@ scripts/checkpatch.pl: sub process {
+# --cover-letter; It is initialized with subject suffix
+# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
+ if ($in_header_lines &&
-+ $rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
-+WARN("Patch appears to be a cover letter with uninitialized
subject" .
-+ " '*** SUBJECT HERE ***'\n$hereline\n");
++ $rawline =~ /^Subject:.+[*]{3} SUBJECT HERE
[*]{3}\s*$/) {
++ WARN("Patch appears to be a cover letter with " .
++ "uninitialized subject '*** SUBJECT
HERE ***'\n$hereline\n");
+ }
+
+ if ($rawline =~ /^[*]{3} BLURB HERE [*]{3}\s*$/) {
-+WARN("Patch appears to be a cover letter with leftover placeholder
" .
-+ "text '*** BLURB HERE ***'\n$hereline\n");
++ WARN("Patch appears to be a cover letter with " .
++ "leftover placeholder text '***
BLURB HERE ***'\n$hereline\n");
+ }
+
if ($in_commit_log && $non_utf8_charset && $realfile =~ /^$/ &&
scripts/checkpatch.pl | 14 ++
1 file changed, 14 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7026895074..9a8d49f1d8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1650,6 +1650,20 @@ sub process {
$non_utf8_charset = 1;
}
+# Check if this is a cover letter patch produced by git-format-patch
+# --cover-letter; It is initialized with subject suffix
+# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
+ if ($in_header_lines &&
+ $rawline =~ /^Subject:.+[*]{3} SUBJECT HERE
[*]{3}\s*$/) {
This continuation line is now hugely over-indented - it should
be aligned just after the '('
It is not, it just uses tabs. Like line 2693 in current master:
https://gitlab.com/qemu-project/qemu/-/blob/11be70677c70fdccd452a3233653949b79e97908/scripts/checkpatch.pl#L2693
I will quote the **QEMU Coding Style** again on whitespace:
Whitespace
Of course, the most important aspect in any coding style is whitespace.
Crusty old coders who have trouble spotting the glasses on their noses
can tell the difference between a tab and eight spaces from a distance
of approximately fifteen parsecs. Many a flamewar has been fought and
lost on this issue.
QEMU indents are four spaces. Tabs are never used, except in Makefiles
where they have been irreversibly coded into the syntax. Spaces of
course are superior to tabs because:
You have just one way to specify whitespace, not two. Ambiguity breeds
mistakes.
The confusion surrounding ‘use tabs to indent, spaces to justify’ is gone.
Tab indents push your code to the right, making your screen seriously
unbalanced.
Tabs will be rendered incorrectly on editors who are misconfigured not to
use tab stops of eight positions.
Tabs are rendered badly in patches, causing off-by-one errors in almost
every line.
It is the QEMU coding style.
I think it's better if we leave this discussion here, and accept v1
which is consistent with the coding style, or this one which is
consistent with the inconsistency of the tabs and spaces mix of the
checkpatch.pl source code as a compromise, if it is deemed important.
Thanks,
Manos
+ WARN("Patch appears to be a cover letter with " .
+ "uninitialized subject '*** SUBJECT
HERE ***'\n$hereline\n");
As is this
+ }
+
+ if ($rawline =~ /^[*]{3} BLURB HERE [*]{3}\s*$/) {
+ WARN("Patch appears to be a cover letter with " .
+ "leftover placeholder text '***
BLURB HERE ***'\n$hereline\n");
And this.
+ }
+
if ($in_commit_log && $non_utf8_charset && $realfile =~ /^$/ &&
$rawline =~ /$NON_ASCII_UTF8/) {
WARN("8-b
Re: [PATCH v1] scripts/checkpatch.pl: check for placeholders in cover letter patches
On Tue, 30 Jan 2024 11:59, "Daniel P. Berrangé" wrote:
On Tue, Jan 30, 2024 at 11:54:51AM +0200, Manos Pitsidianakis wrote:
On Tue, 30 Jan 2024 11:47, "Daniel P. Berrangé" wrote:
> On Tue, Jan 30, 2024 at 10:51:58AM +0200, Manos Pitsidianakis wrote:
> > On Tue, 30 Jan 2024 at 10:12, Daniel P. Berrangé
wrote:
> > >
> > > On Tue, Jan 30, 2024 at 09:56:15AM +0200, Manos Pitsidianakis wrote:
> > > > Check if a file argument is a cover letter patch produced by
> > > > git-format-patch --cover-letter; It is initialized with subject suffix "
> > > > *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
> > > > exist, warn the user.
> > > >
> > > > Signed-off-by: Manos Pitsidianakis
> > > > ---
> > > > scripts/checkpatch.pl | 14 ++
> > > > 1 file changed, 14 insertions(+)
> > > >
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > > index 7026895074..34f12c9848 100755
> > > > --- a/scripts/checkpatch.pl
> > > > +++ b/scripts/checkpatch.pl
> > > > @@ -1650,6 +1650,20 @@ sub process {
> > > > $non_utf8_charset = 1;
> > > > }
> > > >
> > > > +# Check if this is a cover letter patch produced by git-format-patch
> > > > +# --cover-letter; It is initialized with subject suffix
> > > > +# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
> > > > + if ($in_header_lines &&
> > > > + $rawline =~ /^Subject:.+[*]{3} SUBJECT HERE
[*]{3}\s*$/) {
> > > > +WARN("Patch appears to be a cover letter with uninitialized
subject" .
> > > > + " '*** SUBJECT HERE ***'\n$hereline\n");
> > > > + }
> > > > +
> > > > + if ($rawline =~ /^[*]{3} BLURB HERE [*]{3}\s*$/) {
> > > > +WARN("Patch appears to be a cover letter with leftover placeholder
" .
> > > > + "text '*** BLURB HERE ***'\n$hereline\n");
> > > > + }
> > >
> > > Indentation here is totally off
> >
> > It only seems that way because the pre-existing lines use tabs, while
> > I used spaces, according to the QEMU Coding style:
>
> It is more important to be consistent within a single function.
>
> Regardless of that though, the indent is still broken because the body
> of the 'if' condition is indented /less/ than the 'if' condition itself.
Well not really, that's because my editor replaced the tabs when quoting
your e-mail.
The under-indentation of the WARN statement has nothing to do with
reply quoting, it is visible in the initial patch you submitted:
https://lists.nongnu.org/archive/html/qemu-devel/2024-01/msg06216.html
The html in that link has expanded tabs to 8 spaces :) And QEMU coding
style says an indentation level is 4 spaces.
This is one of the reasons tabs should not be used for indentation at
all. It shows up different in different contexts.
Anyway, it's not very important in any case. Besides, I have superseded
this patch with a v2.
Thanks,
Manos
[PATCH v2] scripts/checkpatch.pl: check for placeholders in cover letter patches
Check if a file argument is a cover letter patch produced by
git-format-patch --cover-letter; It is initialized with subject suffix "
*** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
exist, warn the user.
Signed-off-by: Manos Pitsidianakis
---
Range-diff against v1:
1: 64b7ec2287 ! 1: 9bf816eb4c scripts/checkpatch.pl: check for placeholders
in cover letter patches
@@ scripts/checkpatch.pl: sub process {
+# --cover-letter; It is initialized with subject suffix
+# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
+ if ($in_header_lines &&
-+ $rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
-+WARN("Patch appears to be a cover letter with uninitialized
subject" .
-+ " '*** SUBJECT HERE ***'\n$hereline\n");
++ $rawline =~ /^Subject:.+[*]{3} SUBJECT HERE
[*]{3}\s*$/) {
++ WARN("Patch appears to be a cover letter with " .
++ "uninitialized subject '***
SUBJECT HERE ***'\n$hereline\n");
+ }
+
+ if ($rawline =~ /^[*]{3} BLURB HERE [*]{3}\s*$/) {
-+WARN("Patch appears to be a cover letter with leftover
placeholder " .
-+ "text '*** BLURB HERE ***'\n$hereline\n");
++ WARN("Patch appears to be a cover letter with " .
++ "leftover placeholder text '***
BLURB HERE ***'\n$hereline\n");
+ }
+
if ($in_commit_log && $non_utf8_charset && $realfile =~ /^$/ &&
scripts/checkpatch.pl | 14 ++
1 file changed, 14 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7026895074..9a8d49f1d8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1650,6 +1650,20 @@ sub process {
$non_utf8_charset = 1;
}
+# Check if this is a cover letter patch produced by git-format-patch
+# --cover-letter; It is initialized with subject suffix
+# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
+ if ($in_header_lines &&
+ $rawline =~ /^Subject:.+[*]{3} SUBJECT HERE
[*]{3}\s*$/) {
+ WARN("Patch appears to be a cover letter with " .
+ "uninitialized subject '***
SUBJECT HERE ***'\n$hereline\n");
+ }
+
+ if ($rawline =~ /^[*]{3} BLURB HERE [*]{3}\s*$/) {
+ WARN("Patch appears to be a cover letter with " .
+ "leftover placeholder text '***
BLURB HERE ***'\n$hereline\n");
+ }
+
if ($in_commit_log && $non_utf8_charset && $realfile =~ /^$/ &&
$rawline =~ /$NON_ASCII_UTF8/) {
WARN("8-bit UTF-8 used in possible commit log\n" .
$herecurr);
base-commit: 11be70677c70fdccd452a3233653949b79e97908
--
γαῖα πυρί μιχθήτω
Re: [PATCH v1] scripts/checkpatch.pl: check for placeholders in cover letter patches
On Tue, 30 Jan 2024 11:47, "Daniel P. Berrangé" wrote:
On Tue, Jan 30, 2024 at 10:51:58AM +0200, Manos Pitsidianakis wrote:
On Tue, 30 Jan 2024 at 10:12, Daniel P. Berrangé wrote:
>
> On Tue, Jan 30, 2024 at 09:56:15AM +0200, Manos Pitsidianakis wrote:
> > Check if a file argument is a cover letter patch produced by
> > git-format-patch --cover-letter; It is initialized with subject suffix "
> > *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
> > exist, warn the user.
> >
> > Signed-off-by: Manos Pitsidianakis
> > ---
> > scripts/checkpatch.pl | 14 ++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 7026895074..34f12c9848 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -1650,6 +1650,20 @@ sub process {
> > $non_utf8_charset = 1;
> > }
> >
> > +# Check if this is a cover letter patch produced by git-format-patch
> > +# --cover-letter; It is initialized with subject suffix
> > +# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
> > + if ($in_header_lines &&
> > + $rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
> > +WARN("Patch appears to be a cover letter with uninitialized
subject" .
> > + " '*** SUBJECT HERE ***'\n$hereline\n");
> > + }
> > +
> > + if ($rawline =~ /^[*]{3} BLURB HERE [*]{3}\s*$/) {
> > +WARN("Patch appears to be a cover letter with leftover placeholder
" .
> > + "text '*** BLURB HERE ***'\n$hereline\n");
> > + }
>
> Indentation here is totally off
It only seems that way because the pre-existing lines use tabs, while
I used spaces, according to the QEMU Coding style:
It is more important to be consistent within a single function.
Regardless of that though, the indent is still broken because the body
of the 'if' condition is indented /less/ than the 'if' condition itself.
Well not really, that's because my editor replaced the tabs when quoting
your e-mail.
Anyway, I will respin a v2 with just tabs for indentation right away.
Thanks!
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] hw/hyperv: Include missing headers
On Mon, 29 Jan 2024 19:00, Philippe Mathieu-Daudé wrote:
Include missing headers in order to avoid when refactoring
unrelated headers:
hw/hyperv/hyperv.c:33:18: error: field ‘msg_page_mr’ has incomplete type
33 | MemoryRegion msg_page_mr;
| ^~~
hw/hyperv/hyperv.c: In function ‘synic_update’:
hw/hyperv/hyperv.c:64:13: error: implicit declaration of function
‘memory_region_del_subregion’ [-Werror=implicit-function-declaration]
64 | memory_region_del_subregion(get_system_memory(),
| ^~~
hw/hyperv/hyperv.c: In function ‘hyperv_hcall_signal_event’:
hw/hyperv/hyperv.c:683:17: error: implicit declaration of function ‘ldq_phys’;
did you mean ‘ldub_phys’? [-Werror=implicit-function-declaration]
683 | param = ldq_phys(_space_memory, addr);
| ^~~~
| ldub_phys
hw/hyperv/hyperv.c:683:17: error: nested extern declaration of ‘ldq_phys’
[-Werror=nested-externs]
hw/hyperv/hyperv.c: In function ‘hyperv_hcall_retreive_dbg_data’:
hw/hyperv/hyperv.c:792:24: error: ‘TARGET_PAGE_SIZE’ undeclared (first use in
this function); did you mean ‘TARGET_PAGE_BITS’?
792 | msg.u.recv.count = TARGET_PAGE_SIZE - sizeof(*debug_data_out);
|^~~~
|TARGET_PAGE_BITS
hw/hyperv/hyperv.c: In function ‘hyperv_syndbg_send’:
hw/hyperv/hyperv.c:885:16: error: ‘HV_SYNDBG_STATUS_INVALID’ undeclared (first
use in this function)
885 | return HV_SYNDBG_STATUS_INVALID;
|^~~~
Signed-off-by: Philippe Mathieu-Daudé
---
BTW who maintains this code?
$ ./scripts/get_maintainer.pl -f hw/hyperv/hyperv.c
get_maintainer.pl: No maintainers found, printing recent contributors.
get_maintainer.pl: Do not blindly cc: them on patches! Use common sense.
---
hw/hyperv/hyperv.c | 4
1 file changed, 4 insertions(+)
diff --git a/hw/hyperv/hyperv.c b/hw/hyperv/hyperv.c
index 57b402b956..6c4a18dd0e 100644
--- a/hw/hyperv/hyperv.c
+++ b/hw/hyperv/hyperv.c
@@ -12,6 +12,7 @@
#include "qemu/module.h"
#include "qapi/error.h"
#include "exec/address-spaces.h"
+#include "exec/memory.h"
#include "sysemu/kvm.h"
#include "qemu/bitops.h"
#include "qemu/error-report.h"
@@ -21,6 +22,9 @@
#include "qemu/rcu_queue.h"
#include "hw/hyperv/hyperv.h"
#include "qom/object.h"
+#include "target/i386/kvm/hyperv-proto.h"
+#include "target/i386/cpu.h"
+#include "exec/cpu-all.h"
struct SynICState {
DeviceState parent_obj;
--
2.41.0
Reviewed-by: Manos Pitsidianakis
Re: [PATCH v1] scripts/checkpatch.pl: check for placeholders in cover letter patches
On Tue, 30 Jan 2024 at 10:12, Daniel P. Berrangé wrote:
>
> On Tue, Jan 30, 2024 at 09:56:15AM +0200, Manos Pitsidianakis wrote:
> > Check if a file argument is a cover letter patch produced by
> > git-format-patch --cover-letter; It is initialized with subject suffix "
> > *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
> > exist, warn the user.
> >
> > Signed-off-by: Manos Pitsidianakis
> > ---
> > scripts/checkpatch.pl | 14 ++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 7026895074..34f12c9848 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -1650,6 +1650,20 @@ sub process {
> > $non_utf8_charset = 1;
> > }
> >
> > +# Check if this is a cover letter patch produced by git-format-patch
> > +# --cover-letter; It is initialized with subject suffix
> > +# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
> > + if ($in_header_lines &&
> > + $rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
> > +WARN("Patch appears to be a cover letter with uninitialized
> > subject" .
> > + " '*** SUBJECT HERE ***'\n$hereline\n");
> > + }
> > +
> > + if ($rawline =~ /^[*]{3} BLURB HERE [*]{3}\s*$/) {
> > +WARN("Patch appears to be a cover letter with leftover placeholder
> > " .
> > + "text '*** BLURB HERE ***'\n$hereline\n");
> > + }
>
> Indentation here is totally off
It only seems that way because the pre-existing lines use tabs, while
I used spaces, according to the QEMU Coding style:
> QEMU indents are four spaces. Tabs are never used, except in Makefiles where
> they have been irreversibly coded into the syntax.
> > +
> > if ($in_commit_log && $non_utf8_charset && $realfile =~ /^$/
> > &&
> > $rawline =~ /$NON_ASCII_UTF8/) {
> > WARN("8-bit UTF-8 used in possible commit log\n" .
> > $herecurr);
> >
> > base-commit: 11be70677c70fdccd452a3233653949b79e97908
> > --
> > γαῖα πυρί μιχθήτω
> >
>
> 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 :|
>
[PATCH v1] scripts/checkpatch.pl: check for placeholders in cover letter patches
Check if a file argument is a cover letter patch produced by
git-format-patch --cover-letter; It is initialized with subject suffix "
*** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***". If they
exist, warn the user.
Signed-off-by: Manos Pitsidianakis
---
scripts/checkpatch.pl | 14 ++
1 file changed, 14 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7026895074..34f12c9848 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1650,6 +1650,20 @@ sub process {
$non_utf8_charset = 1;
}
+# Check if this is a cover letter patch produced by git-format-patch
+# --cover-letter; It is initialized with subject suffix
+# " *** SUBJECT HERE ***" and body prefix " *** BLURB HERE ***"
+ if ($in_header_lines &&
+ $rawline =~ /^Subject:.+[*]{3} SUBJECT HERE [*]{3}\s*$/) {
+WARN("Patch appears to be a cover letter with uninitialized subject" .
+ " '*** SUBJECT HERE ***'\n$hereline\n");
+ }
+
+ if ($rawline =~ /^[*]{3} BLURB HERE [*]{3}\s*$/) {
+WARN("Patch appears to be a cover letter with leftover placeholder " .
+ "text '*** BLURB HERE ***'\n$hereline\n");
+ }
+
if ($in_commit_log && $non_utf8_charset && $realfile =~ /^$/ &&
$rawline =~ /$NON_ASCII_UTF8/) {
WARN("8-bit UTF-8 used in possible commit log\n" .
$herecurr);
base-commit: 11be70677c70fdccd452a3233653949b79e97908
--
γαῖα πυρί μιχθήτω
[PATCH v3 2/2] hw/block/block.c: improve confusing blk_check_size_and_read_all() error
In cases where a device tries to read more bytes than the block device
contains, the error is vague: "device requires X bytes, block backend
provides Y bytes".
This patch changes the errors of this function to include the block
backend name, the device id and device type name where appropriate.
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Manos Pitsidianakis
---
include/hw/block/block.h | 4 ++--
hw/block/block.c | 25 +++--
hw/block/m25p80.c| 3 ++-
hw/block/pflash_cfi01.c | 4 ++--
hw/block/pflash_cfi02.c | 2 +-
5 files changed, 22 insertions(+), 16 deletions(-)
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 15fff66435..de3946a5f1 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -88,8 +88,8 @@ static inline unsigned int get_physical_block_exp(BlockConf
*conf)
/* Backend access helpers */
-bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
- Error **errp);
+bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
+ void *buf, hwaddr size, Error **errp);
/* Configuration helpers */
diff --git a/hw/block/block.c b/hw/block/block.c
index 9f52ee6e72..ec4a675490 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -54,29 +54,30 @@ static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr
size, void *buf)
* BDRV_REQUEST_MAX_BYTES.
* On success, return true.
* On failure, store an error through @errp and return false.
- * Note that the error messages do not identify the block backend.
- * TODO Since callers don't either, this can result in confusing
- * errors.
+ *
* This function not intended for actual block devices, which read on
* demand. It's for things like memory devices that (ab)use a block
* backend to provide persistence.
*/
-bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
- Error **errp)
+bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
+ void *buf, hwaddr size, Error **errp)
{
int64_t blk_len;
int ret;
+g_autofree char *dev_id = NULL;
blk_len = blk_getlength(blk);
if (blk_len < 0) {
error_setg_errno(errp, -blk_len,
- "can't get size of block backend");
+ "can't get size of %s block backend", blk_name(blk));
return false;
}
if (blk_len != size) {
-error_setg(errp, "device requires %" HWADDR_PRIu " bytes, "
- "block backend provides %" PRIu64 " bytes",
- size, blk_len);
+dev_id = qdev_get_human_name(dev);
+error_setg(errp, "%s device '%s' requires %" HWADDR_PRIu
+ " bytes, %s block backend provides %" PRIu64 " bytes",
+ object_get_typename(OBJECT(dev)), dev_id, size,
+ blk_name(blk), blk_len);
return false;
}
@@ -89,7 +90,11 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void
*buf, hwaddr size,
assert(size <= BDRV_REQUEST_MAX_BYTES);
ret = blk_pread_nonzeroes(blk, size, buf);
if (ret < 0) {
-error_setg_errno(errp, -ret, "can't read block backend");
+dev_id = qdev_get_human_name(dev);
+error_setg_errno(errp, -ret, "can't read %s block backend"
+ " for %s device '%s'",
+ blk_name(blk), object_get_typename(OBJECT(dev)),
+ dev_id);
return false;
}
return true;
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 26ce895628..0a12030a3a 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1617,7 +1617,8 @@ static void m25p80_realize(SSIPeripheral *ss, Error
**errp)
trace_m25p80_binding(s);
s->storage = blk_blockalign(s->blk, s->size);
-if (!blk_check_size_and_read_all(s->blk, s->storage, s->size, errp)) {
+if (!blk_check_size_and_read_all(s->blk, DEVICE(s),
+ s->storage, s->size, errp)) {
return;
}
} else {
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index f956f8bcf7..1bda8424b9 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -848,8 +848,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error
**errp)
}
if (pfl->blk) {
-if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len,
- errp)) {
+if (!blk_check_size_and_read_all(pfl->blk, dev, pfl->storage,
+ total_len, errp)) {
vmstate_unregister_ram(>mem, DEVICE(pfl));
return;
[PATCH v3 1/2] hw/core/qdev.c: add qdev_get_human_name()
Add a simple method to return some kind of human readable identifier for
use in error messages.
Reviewed-by: Stefan Hajnoczi
Signed-off-by: Manos Pitsidianakis
---
include/hw/qdev-core.h | 14 ++
hw/core/qdev.c | 8
2 files changed, 22 insertions(+)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 151d968238..66338f479f 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -993,6 +993,20 @@ const char *qdev_fw_name(DeviceState *dev);
void qdev_assert_realized_properly(void);
Object *qdev_get_machine(void);
+/**
+ * qdev_get_human_name() - Return a human-readable name for a device
+ * @dev: The device. Must be a valid and non-NULL pointer.
+ *
+ * .. note::
+ *This function is intended for user friendly error messages.
+ *
+ * Returns: A newly allocated string containing the device id if not null,
+ * else the object canonical path.
+ *
+ * Use g_free() to free it.
+ */
+char *qdev_get_human_name(DeviceState *dev);
+
/* FIXME: make this a link<> */
bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 43d863b0c5..c68d0f7c51 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -879,6 +879,14 @@ Object *qdev_get_machine(void)
return dev;
}
+char *qdev_get_human_name(DeviceState *dev)
+{
+g_assert(dev != NULL);
+
+return dev->id ?
+ g_strdup(dev->id) : object_get_canonical_path(OBJECT(dev));
+}
+
static MachineInitPhase machine_phase;
bool phase_check(MachineInitPhase phase)
--
γαῖα πυρί μιχθήτω
[PATCH v3 0/2] hw/block/block.c: improve confusing error
In cases where a device tries to read more bytes than the block device contains with the blk_check_size_and_read_all() function, the error is vague: "device requires X bytes, block backend provides Y bytes". This patch changes the errors of this function to include the block backend name, the device id and device type name where appropriate. Version 3: - Changed phrasing "%s device with id='%s'" to "%s device '%s'" since second parameter might be either device id or device path. (thanks Stefan Hajnoczi ) Version 2: - Assert dev is not NULL on qdev_get_human_name (thanks Phil Mathieu-Daudé ) Manos Pitsidianakis (2): hw/core/qdev.c: add qdev_get_human_name() hw/block/block.c: improve confusing blk_check_size_and_read_all() error include/hw/block/block.h | 4 ++-- include/hw/qdev-core.h | 14 ++ hw/block/block.c | 25 +++-- hw/block/m25p80.c| 3 ++- hw/block/pflash_cfi01.c | 4 ++-- hw/block/pflash_cfi02.c | 2 +- hw/core/qdev.c | 8 7 files changed, 44 insertions(+), 16 deletions(-) Range-diff against v2: 1: 5fb5879708 ! 1: 8b566bfced hw/core/qdev.c: add qdev_get_human_name() @@ Commit message Add a simple method to return some kind of human readable identifier for use in error messages. +Reviewed-by: Stefan Hajnoczi Signed-off-by: Manos Pitsidianakis ## include/hw/qdev-core.h ## 2: 8e7eb17fbd ! 2: 7260eadff2 hw/block/block.c: improve confusing blk_check_size_and_read_all() error @@ hw/block/block.c: static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr size, - "block backend provides %" PRIu64 " bytes", - size, blk_len); +dev_id = qdev_get_human_name(dev); -+error_setg(errp, "%s device with id='%s' requires %" HWADDR_PRIu ++error_setg(errp, "%s device '%s' requires %" HWADDR_PRIu + " bytes, %s block backend provides %" PRIu64 " bytes", + object_get_typename(OBJECT(dev)), dev_id, size, + blk_name(blk), blk_len); @@ hw/block/block.c: bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, -error_setg_errno(errp, -ret, "can't read block backend"); +dev_id = qdev_get_human_name(dev); +error_setg_errno(errp, -ret, "can't read %s block backend" -+ "for %s device with id='%s'", ++ " for %s device '%s'", + blk_name(blk), object_get_typename(OBJECT(dev)), + dev_id); return false; base-commit: 11be70677c70fdccd452a3233653949b79e97908 -- γαῖα πυρί μιχθήτω
