Re: [PATCH 04/11] rust/qemu-api: Add wrappers to run futures in QEMU
On 2/11/25 22:43, Kevin Wolf wrote: +/// Use QEMU's event loops to run a Rust [`Future`] to completion and return its result. +/// +/// This function must be called in coroutine context. If the future isn't ready yet, it yields. +pub fn qemu_co_run_future(future: F) -> F::Output { +let waker = Arc::new(RunFutureWaker { +co: unsafe { bindings::qemu_coroutine_self() }, +}) +.into(); into what? :) Maybe you can add the type to the "let" for clarity. +let mut cx = Context::from_waker(&waker); + +let mut pinned_future = std::pin::pin!(future); +loop { +match pinned_future.as_mut().poll(&mut cx) { +Poll::Ready(res) => return res, Alternatively, "break res" (matter of taste). +Poll::Pending => unsafe { +bindings::qemu_coroutine_yield(); +}, +} +} +} +/// Wrapper around [`qemu_co_run_future`] that can be called from C. +/// +/// # Safety +/// +/// `future` must be a valid pointer to an owned `F` (it will be freed in this function). `output` +/// must be a valid pointer representing a mutable reference to an `F::Output` where the result can +/// be stored. +unsafe extern "C" fn rust_co_run_future( +future: *mut bindings::RustBoxedFuture, +output: *mut c_void, +) { +let future = unsafe { Box::from_raw(future.cast::()) }; +let output = output.cast::(); +let ret = qemu_co_run_future(*future); +unsafe { +*output = ret; This should use output.write(ret), to ensure that the output is written without dropping the previous value. Also, would qemu_co_run_future() and qemu_run_future() become methods on an Executor later? Maybe it make sense to have already something like pub trait QemuExecutor { fn run_until(future: F) -> F::Output; } pub struct Executor; pub struct CoExecutor; and pass an executor to Rust functions (&Executor for no_coroutine_fn, &CoExecutor for coroutine_fn, &dyn QemuExecutor for mixed). Or would that be premature in your opinion? Paolo
Re: [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust
On Tue, Feb 11, 2025 at 10:43:27PM +0100, Kevin Wolf wrote: > This adds a separate block driver for the bochs image format called > 'bochs-rs' so that for the moment both the C implementation and the Rust > implementation can be present in the same build. The intention is to > remove the C implementation eventually and rename this one into 'bochs'. > This can only happen once Rust can be a hard build dependency for QEMU. > > Signed-off-by: Kevin Wolf > --- > rust/block/Cargo.toml| 2 +- > rust/block/src/bochs.rs | 296 +++ > rust/block/src/driver.rs | 5 - > rust/block/src/lib.rs| 1 + > 4 files changed, 298 insertions(+), 6 deletions(-) > create mode 100644 rust/block/src/bochs.rs > > diff --git a/rust/block/Cargo.toml b/rust/block/Cargo.toml > index 70ee02f429..1c06f3a00c 100644 > --- a/rust/block/Cargo.toml > +++ b/rust/block/Cargo.toml > @@ -3,7 +3,7 @@ name = "block" > version = "0.1.0" > edition = "2021" > authors = ["Kevin Wolf "] > -license = "GPL-2.0-or-later" > +license = "GPL-2.0-or-later AND MIT" > readme = "README.md" > description = "Block backends for QEMU" > repository = "https://gitlab.com/qemu-project/qemu/"; > diff --git a/rust/block/src/bochs.rs b/rust/block/src/bochs.rs > new file mode 100644 > index 00..388ac5ef03 > --- /dev/null > +++ b/rust/block/src/bochs.rs > @@ -0,0 +1,296 @@ > +// SPDX-License-Identifier: MIT Why MIT instead of our normal GPL-2.0-or-later. Using Rust conversion to eliminate GPL usage for permissive licenses like MIT is not something I'd like to see us doing. 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 28/42] qapi/parser: prohibit untagged sections between tagged sections
John Snow writes: > This is being done primarily to ensure consistency between the source > documents and the final, rendered HTML output. Because > member/feature/returns sections will always appear in a visually grouped > element in the HTML output, prohibiting free paragraphs between those > sections ensures ordering consistency between source and the final > render. > > Additionally, prohibiting such "middle" text paragraphs allows us to > classify all plain text sections as either "intro" or "detail" > sections, because these sections must either appear before structured > elements ("intro") or afterwards ("detail"). > > This keeps the inlining algorithm simpler with fewer "splice" points > when inlining multiple documentation blocks. Mention the two "middle" paragraphs you have to eliminate in this patch? > > Signed-off-by: John Snow > --- > qapi/net.json | 4 ++-- > qapi/qom.json | 4 ++-- > scripts/qapi/parser.py | 16 > tests/qapi-schema/doc-good.json | 4 ++-- > tests/qapi-schema/doc-good.out | 4 ++-- > tests/qapi-schema/doc-good.txt | 8 > 6 files changed, 28 insertions(+), 12 deletions(-) > > diff --git a/qapi/net.json b/qapi/net.json > index 2739a2f4233..49bc7de64e9 100644 > --- a/qapi/net.json > +++ b/qapi/net.json > @@ -655,13 +655,13 @@ > # this to zero disables this function. This member is mutually > # exclusive with @reconnect. (default: 0) (Since: 9.2) > # > -# Only SocketAddress types 'unix', 'inet' and 'fd' are supported. > -# > # Features: > # > # @deprecated: Member @reconnect is deprecated. Use @reconnect-ms > # instead. > # > +# Only SocketAddress types 'unix', 'inet' and 'fd' are supported. > +# > # Since: 7.2 > ## > { 'struct': 'NetdevStreamOptions', The text moved applies to member @addr. You're moving it even farther away from @addr. Move it into @addr instead? Could be done as a separate cleanup patch to keep this one as simple as possible; matter of taste. The same text is in NetdevDgramOptions below, where it applies to both @remote and @local. It just happens to follow @remote and @local immediately, because there are no other members and no features. Hmm. Ideally, we'd have a way to put such notes next to the stuff they apply to without having to rely on happy accidents like "no features". Alternatively, have a way to link stuff and note. Footnotes? Food for thought, not demand. > diff --git a/qapi/qom.json b/qapi/qom.json > index 28ce24cd8d0..11277d1f84c 100644 > --- a/qapi/qom.json > +++ b/qapi/qom.json > @@ -195,12 +195,12 @@ > # > # @typename: the type name of an object > # > +# Returns: a list of ObjectPropertyInfo describing object properties > +# > # .. note:: Objects can create properties at runtime, for example to > #describe links between different devices and/or objects. These > #properties are not included in the output of this command. > # > -# Returns: a list of ObjectPropertyInfo describing object properties > -# > # Since: 2.12 > ## This move is fine. Placing notes at the end is more common already. > { 'command': 'qom-list-properties', > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > index b2f77ffdd7a..c5d2b950a82 100644 > --- a/scripts/qapi/parser.py > +++ b/scripts/qapi/parser.py > @@ -500,6 +500,20 @@ def get_doc(self) -> 'QAPIDoc': > self.accept(False) > line = self.get_doc_line() > have_tagged = False > +no_more_tags = False > + > +def _tag_check(what: str) -> None: > +if what in ('TODO', 'Since'): > +return > + > +if no_more_tags: > +raise QAPIParseError( > +self, > +f"{what!r} section cannot appear after free " > +"paragraphs that follow other tagged sections. " > +"Move this section upwards with the preceding " > +"tagged sections." > +) Why !r conversion? Error messages should be a single, short phrase, no punctuation at the end. Sometimes a helpful hint is desirable. Here's one in expr.py: raise QAPISemError( info, "%s has unknown key%s %s\nValid keys are %s." % (source, 's' if len(unknown) > 1 else '', pprint(unknown), pprint(allowed))) Needs a negative test case. Aside: we should probably convert most string interpolation to f-strings en masse at some point. > > while line is not None: > # Blank lines > @@ -513,6 +527,7 @@ def get_doc(self) -> 'QAPIDoc': > if doc.features: > raise QAPIParseError( > self, "duplicated 'Features:' line") > +_tag_check("Features") > self.accept(False) > li
Re: [PULL 04/32] hw/timer/xilinx_timer: Make device endianness configurable
On 12/2/25 09:27, Thomas Huth wrote: On 10/02/2025 21.41, Philippe Mathieu-Daudé wrote: Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness for each machine using the device. Reviewed-by: Richard Henderson Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20250206131052.30207-5-phi...@linaro.org> --- hw/microblaze/petalogix_ml605_mmu.c | 1 + hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 + hw/ppc/virtex_ml507.c | 1 + hw/timer/xilinx_timer.c | 35 +++- 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/ petalogix_ml605_mmu.c index cf3b9574db3..bbda70aa93b 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -127,6 +127,7 @@ petalogix_ml605_init(MachineState *machine) /* 2 timers at irq 2 @ 100 Mhz. */ dev = qdev_new("xlnx.xps-timer"); + qdev_prop_set_bit(dev, "little-endian", true); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 100 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/ petalogix_s3adsp1800_mmu.c index fbf52ba8f2f..9d4316b4036 100644 --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c @@ -114,6 +114,7 @@ petalogix_s3adsp1800_init(MachineState *machine) /* 2 timers at irq 2 @ 62 Mhz. */ dev = qdev_new("xlnx.xps-timer"); + qdev_prop_set_bit(dev, "little-endian", !TARGET_BIG_ENDIAN); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 62 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c index 23238119273..f87c221d076 100644 --- a/hw/ppc/virtex_ml507.c +++ b/hw/ppc/virtex_ml507.c @@ -230,6 +230,7 @@ static void virtex_init(MachineState *machine) /* 2 timers at irq 2 @ 62 Mhz. */ dev = qdev_new("xlnx.xps-timer"); + qdev_prop_set_bit(dev, "little-endian", false); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 62 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); Hi, with this patch applied, the ppc_virtex_ml507 functional test is now failing for me ... could you please double-check whether "make check- functional-ppc" still works for you? Thanks, not this patch problem, but patch #2 misses: -- >8 -- diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c index 23238119273..723f62c904b 100644 --- a/hw/ppc/virtex_ml507.c +++ b/hw/ppc/virtex_ml507.c @@ -219,2 +219,3 @@ static void virtex_init(MachineState *machine) dev = qdev_new("xlnx.xps-intc"); +qdev_prop_set_bit(dev, "little-endian", false); qdev_prop_set_uint32(dev, "kind-of-intr", 0); --- Why is my CI green?
Re: [PATCH 29/42] qapi: Add "Details:" disambiguation marker
John Snow writes: > This clarifies sections that are mistaken by the parser as "intro" > sections to be "details" sections instead. Impact on output? See notes inline. > > Signed-off-by: John Snow > --- > qapi/machine.json | 2 ++ > qapi/migration.json| 4 > qapi/qom.json | 4 > qapi/yank.json | 2 ++ > scripts/qapi/parser.py | 8 > 5 files changed, 20 insertions(+) > > diff --git a/qapi/machine.json b/qapi/machine.json > index a6b8795b09e..3c1b397f6cc 100644 > --- a/qapi/machine.json > +++ b/qapi/machine.json > @@ -1301,6 +1301,8 @@ > # Return the amount of initially allocated and present hotpluggable > # (if enabled) memory in bytes. > # > +# Details: > +# > # .. qmp-example:: > # > # -> { "execute": "query-memory-size-summary" } Output unchanged in my testing. Same for the other hunks unless otherwise noted. > diff --git a/qapi/migration.json b/qapi/migration.json > index 43babd1df41..9070a91e655 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -1920,6 +1920,8 @@ > # > # Xen uses this command to notify replication to trigger a checkpoint. > # > +# Details: > +# > # .. qmp-example:: > # > # -> { "execute": "xen-colo-do-checkpoint" } > @@ -1993,6 +1995,8 @@ > # > # Pause a migration. Currently it only supports postcopy. > # > +# Details: > +# > # .. qmp-example:: > # > # -> { "execute": "migrate-pause" } > diff --git a/qapi/qom.json b/qapi/qom.json > index 11277d1f84c..5d285ef9239 100644 > --- a/qapi/qom.json > +++ b/qapi/qom.json > @@ -729,6 +729,8 @@ > # > # Properties for memory-backend-shm objects. > # > +# Details: > +# > # This memory backend supports only shared memory, which is the > # default. > # The paragraphs moves from above to below the auto-generated member documentation, like this: @@ -25908,13 +25908,13 @@ If Properties for memory-backend-shm objects. -This memory backend supports only shared memory, which is the default. - Members ~~~ The members of "MemoryBackendProperties" +This memory backend supports only shared memory, which is the default. + Since ~ This is sphinx-build -b text. I don't understand why there is no blank line between "The members of ... " and the moved paragraph. > @@ -744,6 +746,8 @@ > # > # Properties for memory-backend-epc objects. > # > +# Details: > +# > # The @merge boolean option is false by default with epc > # > # The @dump boolean option is false by default with epc Likewise. > diff --git a/qapi/yank.json b/qapi/yank.json > index 30f46c97c98..4d36d21e76a 100644 > --- a/qapi/yank.json > +++ b/qapi/yank.json > @@ -104,6 +104,8 @@ > # > # Returns: list of @YankInstance > # > +# Details: > +# > # .. qmp-example:: > # > # -> { "execute": "query-yank" } > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > index c5d2b950a82..5890a13b5ba 100644 > --- a/scripts/qapi/parser.py > +++ b/scripts/qapi/parser.py > @@ -544,6 +544,14 @@ def _tag_check(what: str) -> None: > raise QAPIParseError( > self, 'feature descriptions expected') > have_tagged = True > +elif line == 'Details:': > +_tag_check("Details") > +self.accept(False) > +line = self.get_doc_line() > +while line == '': > +self.accept(False) > +line = self.get_doc_line() > +have_tagged = True > elif match := self._match_at_name_colon(line): > # description > if have_tagged:
[PATCH] rust: add module to convert -errno to io::Error
This is an initial, minimal part of the chardev bindings. It is a common convention in QEMU to return a positive value in case of success, and a negated errno value in case of error. Unfortunately, using errno portably in Rust is a bit complicated; on Unix the errno values are supported natively by io::Error, but on Windows they are not and one would have to use the libc crate. Provide a small module that interprets QEMU's convention and does a best-effort translation to io::Error on Windows. Signed-off-by: Paolo Bonzini --- rust/qemu-api/meson.build | 1 + rust/qemu-api/src/assertions.rs | 28 ++ rust/qemu-api/src/errno.rs | 161 rust/qemu-api/src/lib.rs| 1 + rust/qemu-api/src/prelude.rs| 2 + 5 files changed, 193 insertions(+) create mode 100644 rust/qemu-api/src/errno.rs diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build index 26b8500e7d8..607c3010d1d 100644 --- a/rust/qemu-api/meson.build +++ b/rust/qemu-api/meson.build @@ -22,6 +22,7 @@ _qemu_api_rs = static_library( 'src/cell.rs', 'src/chardev.rs', 'src/c_str.rs', + 'src/errno.rs', 'src/irq.rs', 'src/memory.rs', 'src/module.rs', diff --git a/rust/qemu-api/src/assertions.rs b/rust/qemu-api/src/assertions.rs index fa1a18de6fe..104dec39774 100644 --- a/rust/qemu-api/src/assertions.rs +++ b/rust/qemu-api/src/assertions.rs @@ -92,3 +92,31 @@ fn types_must_be_equal(_: T) }; }; } + +/// Assert that an expression matches a pattern. This can also be +/// useful to compare enums that do not implement `Eq`. +/// +/// # Examples +/// +/// ``` +/// # use qemu_api::assert_match; +/// // JoinHandle does not implement `Eq`, therefore the result +/// // does not either. +/// let result: Result, u32> = Err(42); +/// assert_match!(result, Err(42)); +/// ``` +#[macro_export] +macro_rules! assert_match { +($a:expr, $b:pat) => { +assert!( +match $a { +$b => true, +_ => false, +}, +"{} = {:?} does not match {}", +stringify!($a), +$a, +stringify!($b) +); +}; +} diff --git a/rust/qemu-api/src/errno.rs b/rust/qemu-api/src/errno.rs new file mode 100644 index 000..c697f9bef05 --- /dev/null +++ b/rust/qemu-api/src/errno.rs @@ -0,0 +1,161 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +//! Module to portably convert `errno` into [`io::Error`], and the +//! return value of functions returning `-errno` into [`io::Result`]. + +use std::io; +#[cfg(windows)] +use std::io::ErrorKind; + +/// An `errno` value that can be converted into an [`io::Error`] +pub struct Errno(u16); + +// On Unix, from_raw_os_error takes an errno value and OS errors +// are printed using strerror. On Windows however it takes a +// GetLastError() value; therefore we need to convert errno values +// into io::Error by hand. For simplicity use ErrorKind and use +// the standard library's simple-minded mapping of ErrorKind to Error +// (`impl From for io::Error`). +// +// Since this is just for Windows, do not bother with using the libc +// crate or generating the constants from C. Just list here the +// constants that map to stable error kinds. +#[cfg(windows)] +mod libc { +pub const EPERM: u16 = 1; +pub const ENOENT: u16 = 2; +pub const EINTR: u16 = 4; +pub const EAGAIN: u16 = 11; +pub const ENOMEM: u16 = 12; +pub const EACCES: u16 = 13; +pub const EEXIST: u16 = 17; +pub const EINVAL: u16 = 22; +pub const EPIPE: u16 = 32; +pub const EADDRINUSE: u16 = 100; +pub const EADDRNOTAVAIL: u16 = 101; +pub const ECONNABORTED: u16 = 106; +pub const ECONNREFUSED: u16 = 107; +pub const ECONNRESET: u16 = 108; +pub const ENOTCONN: u16 = 126; +pub const ENOTSUP: u16 = 129; +pub const ETIMEDOUT: u16 = 138; +pub const EWOULDBLOCK: u16 = 140; +} + +impl From for io::Error { +#[cfg(unix)] +fn from(value: Errno) -> io::Error { +let Errno(errno) = value; +io::Error::from_raw_os_error(errno.into()) +} + +#[cfg(windows)] +fn from(value: Errno) -> io::Error { +let Errno(errno) = value; +let error_kind = match errno { +libc::EPERM | libc::EACCES => ErrorKind::PermissionDenied, +libc::ENOENT => ErrorKind::NotFound, +libc::EINTR => ErrorKind::Interrupted, +// Note that on Windows we know these two are distinct. In general, +// it would not be possible to use "|". +libc::EAGAIN | libc::EWOULDBLOCK => ErrorKind::WouldBlock, +libc::ENOMEM => ErrorKind::OutOfMemory, +libc::EEXIST => ErrorKind::AlreadyExists, +libc::EINVAL => ErrorKind::InvalidInput, +libc::EPIPE => ErrorKind::BrokenPipe, +libc::EADDRINUSE => ErrorKind::AddrInUse, +libc::EADDRNOTAVAIL => ErrorKind::AddrNotAvailable, +
Re: [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust
On Wed, Feb 12, 2025 at 09:14:57AM +, Daniel P. Berrangé wrote: > On Tue, Feb 11, 2025 at 10:43:27PM +0100, Kevin Wolf wrote: > > This adds a separate block driver for the bochs image format called > > 'bochs-rs' so that for the moment both the C implementation and the Rust > > implementation can be present in the same build. The intention is to > > remove the C implementation eventually and rename this one into 'bochs'. > > This can only happen once Rust can be a hard build dependency for QEMU. > > > > Signed-off-by: Kevin Wolf > > --- > > rust/block/Cargo.toml| 2 +- > > rust/block/src/bochs.rs | 296 +++ > > rust/block/src/driver.rs | 5 - > > rust/block/src/lib.rs| 1 + > > 4 files changed, 298 insertions(+), 6 deletions(-) > > create mode 100644 rust/block/src/bochs.rs > > > > diff --git a/rust/block/Cargo.toml b/rust/block/Cargo.toml > > index 70ee02f429..1c06f3a00c 100644 > > --- a/rust/block/Cargo.toml > > +++ b/rust/block/Cargo.toml > > @@ -3,7 +3,7 @@ name = "block" > > version = "0.1.0" > > edition = "2021" > > authors = ["Kevin Wolf "] > > -license = "GPL-2.0-or-later" > > +license = "GPL-2.0-or-later AND MIT" > > readme = "README.md" > > description = "Block backends for QEMU" > > repository = "https://gitlab.com/qemu-project/qemu/"; > > diff --git a/rust/block/src/bochs.rs b/rust/block/src/bochs.rs > > new file mode 100644 > > index 00..388ac5ef03 > > --- /dev/null > > +++ b/rust/block/src/bochs.rs > > @@ -0,0 +1,296 @@ > > +// SPDX-License-Identifier: MIT > > Why MIT instead of our normal GPL-2.0-or-later. > > Using Rust conversion to eliminate GPL usage for permissive licenses > like MIT is not something I'd like to see us doing. My bad. I should have noticed that the original bochs.c was also MIT, so I presume you're considering this Rust impl to be a derived work. 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 v4 26/33] vfio/migration: Multifd device state transfer support - receive init/cleanup
On 1/30/25 11:08, Maciej S. Szmigiero wrote: From: "Maciej S. Szmigiero" Add support for VFIOMultifd data structure that will contain most of the receive-side data together with its init/cleanup methods. Signed-off-by: Maciej S. Szmigiero --- hw/vfio/migration.c | 52 +-- include/hw/vfio/vfio-common.h | 5 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 3211041939c6..bcdf204d5cf4 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -300,6 +300,9 @@ typedef struct VFIOStateBuffer { size_t len; } VFIOStateBuffer; +typedef struct VFIOMultifd { +} VFIOMultifd; + static void vfio_state_buffer_clear(gpointer data) { VFIOStateBuffer *lb = data; @@ -398,6 +401,18 @@ static int vfio_load_device_config_state(QEMUFile *f, void *opaque) return qemu_file_get_error(f); } +static VFIOMultifd *vfio_multifd_new(void) +{ +VFIOMultifd *multifd = g_new(VFIOMultifd, 1); + +return multifd; +} + +static void vfio_multifd_free(VFIOMultifd *multifd) +{ +g_free(multifd); +} + static void vfio_migration_cleanup(VFIODevice *vbasedev) { VFIOMigration *migration = vbasedev->migration; @@ -785,14 +800,47 @@ static void vfio_save_state(QEMUFile *f, void *opaque) static int vfio_load_setup(QEMUFile *f, void *opaque, Error **errp) { VFIODevice *vbasedev = opaque; +VFIOMigration *migration = vbasedev->migration; +int ret; + +/* + * Make a copy of this setting at the start in case it is changed + * mid-migration. + */ +if (vbasedev->migration_multifd_transfer == ON_OFF_AUTO_AUTO) { +migration->multifd_transfer = vfio_multifd_transfer_supported(); Attribute "migration->multifd_transfer" is not necessary. It can be replaced by a small inline helper testing pointer migration->multifd and this routine can use a local variable instead. I don't think the '_transfer' suffix adds much to the understanding. +} else { +migration->multifd_transfer = +vbasedev->migration_multifd_transfer == ON_OFF_AUTO_ON; +} + +if (migration->multifd_transfer && !vfio_multifd_transfer_supported()) { +error_setg(errp, + "%s: Multifd device transfer requested but unsupported in the current config", + vbasedev->name); +return -EINVAL; +} The above checks are also introduced in vfio_save_setup(). Please implement a common routine vfio_multifd_is_enabled() or some other name. +ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING, + migration->device_state, errp); +if (ret) { +return ret; +} + +if (migration->multifd_transfer) { +assert(!migration->multifd); +migration->multifd = vfio_multifd_new(); +} -return vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING, -vbasedev->migration->device_state, errp); +return 0; } static int vfio_load_cleanup(void *opaque) { VFIODevice *vbasedev = opaque; +VFIOMigration *migration = vbasedev->migration; + +g_clear_pointer(&migration->multifd, vfio_multifd_free); please add a vfio_multifd_cleanup() routine. vfio_migration_cleanup(vbasedev); trace_vfio_load_cleanup(vbasedev->name); diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index 153d03745dc7..c0c9c0b1b263 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -61,6 +61,8 @@ typedef struct VFIORegion { uint8_t nr; /* cache the region number for debug */ } VFIORegion; +typedef struct VFIOMultifd VFIOMultifd; + typedef struct VFIOMigration { struct VFIODevice *vbasedev; VMChangeStateEntry *vm_state; @@ -72,6 +74,8 @@ typedef struct VFIOMigration { uint64_t mig_flags; uint64_t precopy_init_size; uint64_t precopy_dirty_size; +bool multifd_transfer; +VFIOMultifd *multifd; bool initial_data_sent; bool event_save_iterate_started; @@ -133,6 +137,7 @@ typedef struct VFIODevice { bool no_mmap; bool ram_block_discard_allowed; OnOffAuto enable_migration; +OnOffAuto migration_multifd_transfer; This property should be added at the end of the series, with documentation, and used in the vfio_multifd_some_name() routine I mentioned above. Thanks, C. OnOffAuto migration_load_config_after_iter; bool migration_events; VFIODeviceOps *ops;
[PATCH 5/5] target/i386: Mark WHPX APIC region as little-endian
This device is only used by the x86 targets, which are only built as little-endian. Therefore the DEVICE_NATIVE_ENDIAN definition expand to DEVICE_LITTLE_ENDIAN (besides, the DEVICE_BIG_ENDIAN case isn't tested). Simplify directly using DEVICE_LITTLE_ENDIAN. Signed-off-by: Philippe Mathieu-Daudé --- target/i386/whpx/whpx-apic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/i386/whpx/whpx-apic.c b/target/i386/whpx/whpx-apic.c index 4245ab68a27..630a9616d71 100644 --- a/target/i386/whpx/whpx-apic.c +++ b/target/i386/whpx/whpx-apic.c @@ -231,7 +231,7 @@ static void whpx_apic_mem_write(void *opaque, hwaddr addr, static const MemoryRegionOps whpx_apic_io_ops = { .read = whpx_apic_mem_read, .write = whpx_apic_mem_write, -.endianness = DEVICE_NATIVE_ENDIAN, +.endianness = DEVICE_LITTLE_ENDIAN, }; static void whpx_apic_reset(APICCommonState *s) -- 2.47.1
[PATCH 4/5] hw/pci-host: Mark versatile regions as little-endian
This device is only used by the ARM targets, which are only built as little-endian. Therefore the DEVICE_NATIVE_ENDIAN definition expand to DEVICE_LITTLE_ENDIAN (besides, the DEVICE_BIG_ENDIAN case isn't tested). Simplify directly using DEVICE_LITTLE_ENDIAN. Signed-off-by: Philippe Mathieu-Daudé --- hw/pci-host/versatile.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c index c3fbf4cbf94..33a8ceb3b54 100644 --- a/hw/pci-host/versatile.c +++ b/hw/pci-host/versatile.c @@ -246,7 +246,7 @@ static uint64_t pci_vpb_reg_read(void *opaque, hwaddr addr, static const MemoryRegionOps pci_vpb_reg_ops = { .read = pci_vpb_reg_read, .write = pci_vpb_reg_write, -.endianness = DEVICE_NATIVE_ENDIAN, +.endianness = DEVICE_LITTLE_ENDIAN, .valid = { .min_access_size = 4, .max_access_size = 4, @@ -312,7 +312,7 @@ static uint64_t pci_vpb_config_read(void *opaque, hwaddr addr, static const MemoryRegionOps pci_vpb_config_ops = { .read = pci_vpb_config_read, .write = pci_vpb_config_write, -.endianness = DEVICE_NATIVE_ENDIAN, +.endianness = DEVICE_LITTLE_ENDIAN, }; static int pci_vpb_map_irq(PCIDevice *d, int irq_num) -- 2.47.1
[PATCH 0/5] hw: More DEVICE_ [NATIVE -> LITTLE] _ENDIAN conversions
When devices are only built for little-endian targets, DEVICE_NATIVE_ENDIAN expand to DEVICE_LITTLE_ENDIAN. Replace by the latter to simplify. Philippe Mathieu-Daudé (5): hw/arm: Mark Allwinner Technology devices as little-endian hw/mips: Mark Boston machine devices as little-endian hw/mips: Mark Loonson3 Virt machine devices as little-endian hw/pci-host: Mark versatile regions as little-endian target/i386: Mark WHPX APIC region as little-endian hw/arm/allwinner-a10.c | 2 +- hw/arm/allwinner-h3.c | 8 hw/arm/allwinner-r40.c | 2 +- hw/i2c/allwinner-i2c.c | 2 +- hw/intc/allwinner-a10-pic.c| 2 +- hw/mips/boston.c | 6 +++--- hw/mips/loongson3_virt.c | 4 ++-- hw/misc/allwinner-a10-ccm.c| 2 +- hw/misc/allwinner-a10-dramc.c | 2 +- hw/misc/allwinner-cpucfg.c | 2 +- hw/misc/allwinner-h3-ccu.c | 2 +- hw/misc/allwinner-h3-dramc.c | 6 +++--- hw/misc/allwinner-h3-sysctrl.c | 2 +- hw/misc/allwinner-r40-ccu.c| 2 +- hw/misc/allwinner-r40-dramc.c | 10 +- hw/misc/allwinner-sid.c| 2 +- hw/misc/allwinner-sramc.c | 2 +- hw/net/allwinner-sun8i-emac.c | 2 +- hw/net/allwinner_emac.c| 2 +- hw/pci-host/versatile.c| 4 ++-- hw/rtc/allwinner-rtc.c | 2 +- hw/sd/allwinner-sdhost.c | 2 +- hw/ssi/allwinner-a10-spi.c | 2 +- hw/timer/allwinner-a10-pit.c | 2 +- hw/watchdog/allwinner-wdt.c| 2 +- target/i386/whpx/whpx-apic.c | 2 +- 26 files changed, 39 insertions(+), 39 deletions(-) -- 2.47.1
[PATCH 3/5] hw/mips: Mark Loonson3 Virt machine devices as little-endian
The Loonson3 Virt machine is only built as little-endian. Therefore the DEVICE_NATIVE_ENDIAN definition expand to DEVICE_LITTLE_ENDIAN (besides, the DEVICE_BIG_ENDIAN case isn't tested). Simplify directly using DEVICE_LITTLE_ENDIAN. Signed-off-by: Philippe Mathieu-Daudé --- hw/mips/loongson3_virt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c index 831fddb1bd7..db1cc513147 100644 --- a/hw/mips/loongson3_virt.c +++ b/hw/mips/loongson3_virt.c @@ -144,7 +144,7 @@ static void loongson3_pm_write(void *opaque, hwaddr addr, static const MemoryRegionOps loongson3_pm_ops = { .read = loongson3_pm_read, .write = loongson3_pm_write, -.endianness = DEVICE_NATIVE_ENDIAN, +.endianness = DEVICE_LITTLE_ENDIAN, .valid = { .min_access_size = 1, .max_access_size = 1 @@ -560,7 +560,7 @@ static void mips_loongson3_virt_init(MachineState *machine) serial_mm_init(address_space_mem, virt_memmap[VIRT_UART].base, 0, qdev_get_gpio_in(liointc, UART_IRQ), 115200, serial_hd(0), - DEVICE_NATIVE_ENDIAN); + DEVICE_LITTLE_ENDIAN); sysbus_create_simple("goldfish_rtc", virt_memmap[VIRT_RTC].base, qdev_get_gpio_in(liointc, RTC_IRQ)); -- 2.47.1
[PATCH 2/5] hw/mips: Mark Boston machine devices as little-endian
The Boston machine is only built as little-endian. Therefore the DEVICE_NATIVE_ENDIAN definition expand to DEVICE_LITTLE_ENDIAN (besides, the DEVICE_BIG_ENDIAN case isn't tested). Simplify directly using DEVICE_LITTLE_ENDIAN. Signed-off-by: Philippe Mathieu-Daudé --- hw/mips/boston.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/mips/boston.c b/hw/mips/boston.c index 364c328032a..4690b254dda 100644 --- a/hw/mips/boston.c +++ b/hw/mips/boston.c @@ -220,7 +220,7 @@ static void boston_lcd_write(void *opaque, hwaddr addr, static const MemoryRegionOps boston_lcd_ops = { .read = boston_lcd_read, .write = boston_lcd_write, -.endianness = DEVICE_NATIVE_ENDIAN, +.endianness = DEVICE_LITTLE_ENDIAN, }; static uint64_t boston_platreg_read(void *opaque, hwaddr addr, @@ -299,7 +299,7 @@ static void boston_platreg_write(void *opaque, hwaddr addr, static const MemoryRegionOps boston_platreg_ops = { .read = boston_platreg_read, .write = boston_platreg_write, -.endianness = DEVICE_NATIVE_ENDIAN, +.endianness = DEVICE_LITTLE_ENDIAN, }; static void mips_boston_instance_init(Object *obj) @@ -758,7 +758,7 @@ static void boston_mach_init(MachineState *machine) s->uart = serial_mm_init(sys_mem, boston_memmap[BOSTON_UART].base, 2, get_cps_irq(&s->cps, 3), 1000, - serial_hd(0), DEVICE_NATIVE_ENDIAN); + serial_hd(0), DEVICE_LITTLE_ENDIAN); lcd = g_new(MemoryRegion, 1); memory_region_init_io(lcd, NULL, &boston_lcd_ops, s, "boston-lcd", 0x8); -- 2.47.1
Re: [PATCH v6 11/11] tests/functional: Have microblaze tests inherit common parent class
On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: Have the MicroblazeMachine class being common to both MicroblazeBigEndianMachine and MicroblazeLittleEndianMachine classes. Move the xmaton and ballerina tests to the parent class. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Thomas Huth Message-Id: <20250206131052.30207-16-phi...@linaro.org> --- .../functional/test_microblaze_s3adsp1800.py | 24 +++ .../test_microblazeel_s3adsp1800.py | 30 ++- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/tests/functional/test_microblaze_s3adsp1800.py b/tests/functional/test_microblaze_s3adsp1800.py index c4226f49cf3..650416e0c09 100755 --- a/tests/functional/test_microblaze_s3adsp1800.py +++ b/tests/functional/test_microblaze_s3adsp1800.py @@ -7,6 +7,7 @@ # This work is licensed under the terms of the GNU GPL, version 2 or # later. See the COPYING file in the top-level directory. +from qemu_test import exec_command_and_wait_for_pattern from qemu_test import QemuSystemTest, Asset from qemu_test import wait_for_console_pattern @@ -20,6 +21,11 @@ class MicroblazeMachine(QemuSystemTest): 'day17.tar.xz'), '3ba7439dfbea7af4876662c97f8e1f0cdad9231fc166e4861d17042489270057') +ASSET_IMAGE_LE = Asset( +('https://qemu-advcal.gitlab.io/qac-best-of-multiarch/download/' + 'day05.tar.xz'), +'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22') + def do_ballerina_be_test(self, machine): self.set_machine(machine) self.archive_extract(self.ASSET_IMAGE_BE) @@ -34,6 +40,24 @@ def do_ballerina_be_test(self, machine): # message, that's why we don't test for a later string here. This # needs some investigation by a microblaze wizard one day... +def do_xmaton_le_test(self, machine): +self.require_netdev('user') +self.set_machine(machine) +self.archive_extract(self.ASSET_IMAGE_LE) +self.vm.set_console() +self.vm.add_args('-kernel', self.scratch_file('day13', 'xmaton.bin')) +tftproot = self.scratch_file('day13') +self.vm.add_args('-nic', f'user,tftp={tftproot}') +self.vm.launch() +wait_for_console_pattern(self, 'QEMU Advent Calendar 2023') +wait_for_console_pattern(self, 'buildroot login:') +exec_command_and_wait_for_pattern(self, 'root', '#') +exec_command_and_wait_for_pattern(self, +'tftp -g -r xmaton.png 10.0.2.2 ; md5sum xmaton.png', +'821cd3cab8efd16ad6ee5acc3642a8ea') + +class MicroblazeBigEndianMachine(MicroblazeMachine): Add this here 'til the problem with the precaching is fixed: ASSET_IMAGE_BE = MicroblazeMachine.ASSET_IMAGE_BE ? def test_microblaze_s3adsp1800_legacy_be(self): self.do_ballerina_be_test('petalogix-s3adsp1800') diff --git a/tests/functional/test_microblazeel_s3adsp1800.py b/tests/functional/test_microblazeel_s3adsp1800.py index d50b98342d7..56645bd0bb2 100755 --- a/tests/functional/test_microblazeel_s3adsp1800.py +++ b/tests/functional/test_microblazeel_s3adsp1800.py @@ -7,35 +7,11 @@ # This work is licensed under the terms of the GNU GPL, version 2 or # later. See the COPYING file in the top-level directory. -from qemu_test import exec_command_and_wait_for_pattern -from qemu_test import QemuSystemTest, Asset -from qemu_test import wait_for_console_pattern +from qemu_test import QemuSystemTest +from test_microblaze_s3adsp1800 import MicroblazeMachine -class MicroblazeelMachine(QemuSystemTest): - -timeout = 90 - -ASSET_IMAGE_LE = Asset( -('https://qemu-advcal.gitlab.io/qac-best-of-multiarch/download/' - 'day05.tar.xz'), -'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22') - -def do_xmaton_le_test(self, machine): -self.require_netdev('user') -self.set_machine(machine) -self.archive_extract(self.ASSET_IMAGE_LE) -self.vm.set_console() -self.vm.add_args('-kernel', self.scratch_file('day13', 'xmaton.bin')) -tftproot = self.scratch_file('day13') -self.vm.add_args('-nic', f'user,tftp={tftproot}') -self.vm.launch() -wait_for_console_pattern(self, 'QEMU Advent Calendar 2023') -wait_for_console_pattern(self, 'buildroot login:') -exec_command_and_wait_for_pattern(self, 'root', '#') -exec_command_and_wait_for_pattern(self, -'tftp -g -r xmaton.png 10.0.2.2 ; md5sum xmaton.png', -'821cd3cab8efd16ad6ee5acc3642a8ea') +class MicroblazeLittleEndianMachine(MicroblazeMachine): And add this here: ASSET_IMAGE_LE = MicroblazeMachine.ASSET_IMAGE_LE ? Thomas def test_microblaze_s3adsp1800_legacy_le(self): self.do_xmaton_le_test('petalogix-s3adsp1800')
[PATCH 1/5] hw/arm: Mark Allwinner Technology devices as little-endian
These devices are only used by the ARM targets, which are only built as little-endian. Therefore the DEVICE_NATIVE_ENDIAN definition expand to DEVICE_LITTLE_ENDIAN (besides, the DEVICE_BIG_ENDIAN case isn't tested). Simplify directly using DEVICE_LITTLE_ENDIAN. Signed-off-by: Philippe Mathieu-Daudé --- hw/arm/allwinner-a10.c | 2 +- hw/arm/allwinner-h3.c | 8 hw/arm/allwinner-r40.c | 2 +- hw/i2c/allwinner-i2c.c | 2 +- hw/intc/allwinner-a10-pic.c| 2 +- hw/misc/allwinner-a10-ccm.c| 2 +- hw/misc/allwinner-a10-dramc.c | 2 +- hw/misc/allwinner-cpucfg.c | 2 +- hw/misc/allwinner-h3-ccu.c | 2 +- hw/misc/allwinner-h3-dramc.c | 6 +++--- hw/misc/allwinner-h3-sysctrl.c | 2 +- hw/misc/allwinner-r40-ccu.c| 2 +- hw/misc/allwinner-r40-dramc.c | 10 +- hw/misc/allwinner-sid.c| 2 +- hw/misc/allwinner-sramc.c | 2 +- hw/net/allwinner-sun8i-emac.c | 2 +- hw/net/allwinner_emac.c| 2 +- hw/rtc/allwinner-rtc.c | 2 +- hw/sd/allwinner-sdhost.c | 2 +- hw/ssi/allwinner-a10-spi.c | 2 +- hw/timer/allwinner-a10-pit.c | 2 +- hw/watchdog/allwinner-wdt.c| 2 +- 22 files changed, 31 insertions(+), 31 deletions(-) diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c index a829913f1b5..f1b399759a1 100644 --- a/hw/arm/allwinner-a10.c +++ b/hw/arm/allwinner-a10.c @@ -158,7 +158,7 @@ static void aw_a10_realize(DeviceState *dev, Error **errp) /* FIXME use a qdev chardev prop instead of serial_hd() */ serial_mm_init(get_system_memory(), AW_A10_UART0_REG_BASE, 2, qdev_get_gpio_in(dev, 1), - 115200, serial_hd(0), DEVICE_NATIVE_ENDIAN); + 115200, serial_hd(0), DEVICE_LITTLE_ENDIAN); for (size_t i = 0; i < AW_A10_NUM_USB; i++) { g_autofree char *bus = g_strdup_printf("usb-bus.%zu", i); diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c index 2efced3f66a..1b1afa4fb6f 100644 --- a/hw/arm/allwinner-h3.c +++ b/hw/arm/allwinner-h3.c @@ -408,19 +408,19 @@ static void allwinner_h3_realize(DeviceState *dev, Error **errp) /* UART0. For future clocktree API: All UARTS are connected to APB2_CLK. */ serial_mm_init(get_system_memory(), s->memmap[AW_H3_DEV_UART0], 2, qdev_get_gpio_in(DEVICE(&s->gic), AW_H3_GIC_SPI_UART0), - 115200, serial_hd(0), DEVICE_NATIVE_ENDIAN); + 115200, serial_hd(0), DEVICE_LITTLE_ENDIAN); /* UART1 */ serial_mm_init(get_system_memory(), s->memmap[AW_H3_DEV_UART1], 2, qdev_get_gpio_in(DEVICE(&s->gic), AW_H3_GIC_SPI_UART1), - 115200, serial_hd(1), DEVICE_NATIVE_ENDIAN); + 115200, serial_hd(1), DEVICE_LITTLE_ENDIAN); /* UART2 */ serial_mm_init(get_system_memory(), s->memmap[AW_H3_DEV_UART2], 2, qdev_get_gpio_in(DEVICE(&s->gic), AW_H3_GIC_SPI_UART2), - 115200, serial_hd(2), DEVICE_NATIVE_ENDIAN); + 115200, serial_hd(2), DEVICE_LITTLE_ENDIAN); /* UART3 */ serial_mm_init(get_system_memory(), s->memmap[AW_H3_DEV_UART3], 2, qdev_get_gpio_in(DEVICE(&s->gic), AW_H3_GIC_SPI_UART3), - 115200, serial_hd(3), DEVICE_NATIVE_ENDIAN); + 115200, serial_hd(3), DEVICE_LITTLE_ENDIAN); /* DRAMC */ sysbus_realize(SYS_BUS_DEVICE(&s->dramc), &error_fatal); diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c index 47b3180f0ec..cef6e4d18c2 100644 --- a/hw/arm/allwinner-r40.c +++ b/hw/arm/allwinner-r40.c @@ -492,7 +492,7 @@ static void allwinner_r40_realize(DeviceState *dev, Error **errp) serial_mm_init(get_system_memory(), addr, 2, qdev_get_gpio_in(DEVICE(&s->gic), uart_irqs[i]), - 115200, serial_hd(i), DEVICE_NATIVE_ENDIAN); + 115200, serial_hd(i), DEVICE_LITTLE_ENDIAN); } /* I2C */ diff --git a/hw/i2c/allwinner-i2c.c b/hw/i2c/allwinner-i2c.c index 16f1d6d40e7..66d6431c508 100644 --- a/hw/i2c/allwinner-i2c.c +++ b/hw/i2c/allwinner-i2c.c @@ -407,7 +407,7 @@ static const MemoryRegionOps allwinner_i2c_ops = { .write = allwinner_i2c_write, .valid.min_access_size = 1, .valid.max_access_size = 4, -.endianness = DEVICE_NATIVE_ENDIAN, +.endianness = DEVICE_LITTLE_ENDIAN, }; static const VMStateDescription allwinner_i2c_vmstate = { diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c index c0f30092cd6..93a604f7a04 100644 --- a/hw/intc/allwinner-a10-pic.c +++ b/hw/intc/allwinner-a10-pic.c @@ -135,7 +135,7 @@ static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value, static const MemoryRegionOps aw_a10_pic_ops = { .read = aw_a10_pic_read, .write = aw_a10_pic_write, -.endianness = DEVICE_NATIVE_ENDIAN, +.endianness = DEVICE_LITTLE_ENDIAN, }; static const VMStateDescripti
Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum
On 12/2/25 12:37, Thomas Huth wrote: On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. Endianness can be BIG, LITTLE or unspecified (default). Signed-off-by: Philippe Mathieu-Daudé --- qapi/common.json | 16 include/hw/qdev-properties-system.h | 7 +++ hw/core/qdev-properties-system.c | 11 +++ 3 files changed, 34 insertions(+) diff --git a/qapi/common.json b/qapi/common.json index 6ffc7a37890..217feaaf683 100644 --- a/qapi/common.json +++ b/qapi/common.json @@ -212,3 +212,19 @@ ## { 'struct': 'HumanReadableText', 'data': { 'human-readable-text': 'str' } } + +## +# @EndianMode: +# +# An enumeration of three options: little, big, and unspecified +# +# @little: Little endianness +# +# @big: Big endianness +# +# @unspecified: Endianness not specified +# +# Since: 10.0 +## +{ 'enum': 'EndianMode', + 'data': [ 'little', 'big', 'unspecified' ] } Should 'unspecified' come first? ... so that it gets the value 0, i.e. when someone forgets to properly initialize a related variable, the chances are higher that it ends up as "unspecified" than as "little" ? Hmm but then in this series the dual-endianness regions are defined as: +static const MemoryRegionOps pic_ops[2] = { +[0 ... 1] = { +.read = pic_read, +.write = pic_write, +.endianness = DEVICE_BIG_ENDIAN, +.impl = { +.min_access_size = 4, +.max_access_size = 4, +}, +.valid = { +/* + * All XPS INTC registers are accessed through the PLB interface. + * The base address for these registers is provided by the + * configuration parameter, C_BASEADDR. Each register is 32 bits + * although some bits may be unused and is accessed on a 4-byte + * boundary offset from the base address. + */ +.min_access_size = 4, +.max_access_size = 4, +}, }, -.valid = { -.min_access_size = 4, -.max_access_size = 4 -} +[ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN, +[ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN, }; We could declare the array using the confusing __MAX definition (at the price of wasting the ENDIAN_MODE_UNSPECIFIED entry) as: static const MemoryRegionOps pic_ops[ENDIAN_MODE__MAX - 1] { } WDYT? Apart from that: Reviewed-by: Thomas Huth Thanks!
Re: [PATCH v6 02/11] hw/intc/xilinx_intc: Make device endianness configurable
On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness for each machine using the device. Signed-off-by: Philippe Mathieu-Daudé --- hw/intc/xilinx_intc.c| 60 ++-- hw/microblaze/petalogix_ml605_mmu.c | 1 + hw/microblaze/petalogix_s3adsp1800_mmu.c | 3 ++ hw/ppc/virtex_ml507.c| 1 + hw/riscv/microblaze-v-generic.c | 1 + 5 files changed, 52 insertions(+), 14 deletions(-) diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c index 6930f83907a..523402b688c 100644 --- a/hw/intc/xilinx_intc.c +++ b/hw/intc/xilinx_intc.c @@ -3,6 +3,9 @@ * * Copyright (c) 2009 Edgar E. Iglesias. * + * https://docs.amd.com/v/u/en-US/xps_intc + * DS572: LogiCORE IP XPS Interrupt Controller (v2.01a) + * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal * in the Software without restriction, including without limitation the rights @@ -23,10 +26,12 @@ */ #include "qemu/osdep.h" +#include "qapi/error.h" #include "hw/sysbus.h" #include "qemu/module.h" #include "hw/irq.h" #include "hw/qdev-properties.h" +#include "hw/qdev-properties-system.h" #include "qom/object.h" #define D(x) @@ -49,6 +54,7 @@ struct XpsIntc { SysBusDevice parent_obj; +EndianMode model_endianness; MemoryRegion mmio; qemu_irq parent_irq; @@ -140,18 +146,29 @@ static void pic_write(void *opaque, hwaddr addr, update_irq(p); } -static const MemoryRegionOps pic_ops = { -.read = pic_read, -.write = pic_write, -.endianness = DEVICE_NATIVE_ENDIAN, -.impl = { -.min_access_size = 4, -.max_access_size = 4, +static const MemoryRegionOps pic_ops[2] = { +[0 ... 1] = { Hmm, ok, so here we have now an assumption that ENDIAN_MODE_BIG and ENDIAN_MODE_LITTLE match 0 and 1, which would not work anymore when using 0 as unspecified... a little bit ugly ... so maybe instead of changing pic_ops into an array +.read = pic_read, +.write = pic_write, +.endianness = DEVICE_BIG_ENDIAN, +.impl = { +.min_access_size = 4, +.max_access_size = 4, +}, +.valid = { +/* + * All XPS INTC registers are accessed through the PLB interface. + * The base address for these registers is provided by the + * configuration parameter, C_BASEADDR. Each register is 32 bits + * although some bits may be unused and is accessed on a 4-byte + * boundary offset from the base address. + */ +.min_access_size = 4, +.max_access_size = 4, +}, }, -.valid = { -.min_access_size = 4, -.max_access_size = 4 -} +[ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN, +[ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN, }; static void irq_handler(void *opaque, int irq, int level) @@ -174,13 +191,27 @@ static void xilinx_intc_init(Object *obj) qdev_init_gpio_in(DEVICE(obj), irq_handler, 32); sysbus_init_irq(SYS_BUS_DEVICE(obj), &p->parent_irq); - -memory_region_init_io(&p->mmio, obj, &pic_ops, p, "xlnx.xps-intc", - R_MAX * 4); sysbus_init_mmio(SYS_BUS_DEVICE(obj), &p->mmio); } +static void xilinx_intc_realize(DeviceState *dev, Error **errp) +{ +XpsIntc *p = XILINX_INTC(dev); + +if (p->model_endianness == ENDIAN_MODE_UNSPECIFIED) { +error_setg(errp, TYPE_XILINX_INTC " property 'endianness'" + " must be set to 'big' or 'little'"); +return; +} ... would it be possible to patch in the right value for pic_ops.endianness here instead? Thomas +memory_region_init_io(&p->mmio, OBJECT(dev), + &pic_ops[p->model_endianness], + p, "xlnx.xps-intc", + R_MAX * 4); +} + static const Property xilinx_intc_properties[] = { +DEFINE_PROP_ENDIAN_NODEFAULT("endianness", XpsIntc, model_endianness), DEFINE_PROP_UINT32("kind-of-intr", XpsIntc, c_kind_of_intr, 0), }; @@ -188,6 +219,7 @@ static void xilinx_intc_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); +dc->realize = xilinx_intc_realize; device_class_set_props(dc, xilinx_intc_properties); } diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c index 8b44be75a22..55398cc67d1 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -111,6 +111,7 @@ petalogix_ml605_init(MachineState *machine)
Re: [PATCH v6 07/11] tests/functional: Avoid using www.qemu-advent-calendar.org URL
On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: Avoid fetching assets from www.qemu-advent-calendar.org website, prefer fetching microblaze assets from GitLab servers. Suggested-by: Thomas Huth Signed-off-by: Philippe Mathieu-Daudé --- tests/functional/test_microblazeel_s3adsp1800.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/functional/test_microblazeel_s3adsp1800.py b/tests/functional/test_microblazeel_s3adsp1800.py index c382afe6bfa..5bf94d88dd8 100755 --- a/tests/functional/test_microblazeel_s3adsp1800.py +++ b/tests/functional/test_microblazeel_s3adsp1800.py @@ -18,7 +18,8 @@ class MicroblazeelMachine(QemuSystemTest): timeout = 90 ASSET_IMAGE = Asset( -('http://www.qemu-advent-calendar.org/2023/download/day13.tar.gz'), +('https://qemu-advcal.gitlab.io/qac-best-of-multiarch/download/' + 'day05.tar.xz'), 'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22') You'd likely need to adjust the sha256 sum as well since I repacked with xz instead of gz ... but according to Eldon, the original download should be working again, so I'd suggest to drop this patch for now. Thomas
Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum
On 12/2/25 12:43, Philippe Mathieu-Daudé wrote: On 12/2/25 12:37, Thomas Huth wrote: On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. Endianness can be BIG, LITTLE or unspecified (default). Signed-off-by: Philippe Mathieu-Daudé --- qapi/common.json | 16 include/hw/qdev-properties-system.h | 7 +++ hw/core/qdev-properties-system.c | 11 +++ 3 files changed, 34 insertions(+) diff --git a/qapi/common.json b/qapi/common.json index 6ffc7a37890..217feaaf683 100644 --- a/qapi/common.json +++ b/qapi/common.json @@ -212,3 +212,19 @@ ## { 'struct': 'HumanReadableText', 'data': { 'human-readable-text': 'str' } } + +## +# @EndianMode: +# +# An enumeration of three options: little, big, and unspecified +# +# @little: Little endianness +# +# @big: Big endianness +# +# @unspecified: Endianness not specified +# +# Since: 10.0 +## +{ 'enum': 'EndianMode', + 'data': [ 'little', 'big', 'unspecified' ] } Should 'unspecified' come first? ... so that it gets the value 0, i.e. when someone forgets to properly initialize a related variable, the chances are higher that it ends up as "unspecified" than as "little" ? BTW I'm not sure QAPI guaranty enums are following an order (at least, as in C, I wouldn't rely on that assumption).
Re: [PATCH v6 11/11] tests/functional: Have microblaze tests inherit common parent class
On 12/2/25 12:46, Thomas Huth wrote: On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: Have the MicroblazeMachine class being common to both MicroblazeBigEndianMachine and MicroblazeLittleEndianMachine classes. Move the xmaton and ballerina tests to the parent class. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Thomas Huth Message-Id: <20250206131052.30207-16-phi...@linaro.org> --- .../functional/test_microblaze_s3adsp1800.py | 24 +++ .../test_microblazeel_s3adsp1800.py | 30 ++- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/tests/functional/test_microblaze_s3adsp1800.py b/tests/ functional/test_microblaze_s3adsp1800.py index c4226f49cf3..650416e0c09 100755 --- a/tests/functional/test_microblaze_s3adsp1800.py +++ b/tests/functional/test_microblaze_s3adsp1800.py @@ -7,6 +7,7 @@ # This work is licensed under the terms of the GNU GPL, version 2 or # later. See the COPYING file in the top-level directory. +from qemu_test import exec_command_and_wait_for_pattern from qemu_test import QemuSystemTest, Asset from qemu_test import wait_for_console_pattern @@ -20,6 +21,11 @@ class MicroblazeMachine(QemuSystemTest): 'day17.tar.xz'), '3ba7439dfbea7af4876662c97f8e1f0cdad9231fc166e4861d17042489270057') + ASSET_IMAGE_LE = Asset( + ('https://qemu-advcal.gitlab.io/qac-best-of-multiarch/download/' + 'day05.tar.xz'), + 'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22') + def do_ballerina_be_test(self, machine): self.set_machine(machine) self.archive_extract(self.ASSET_IMAGE_BE) @@ -34,6 +40,24 @@ def do_ballerina_be_test(self, machine): # message, that's why we don't test for a later string here. This # needs some investigation by a microblaze wizard one day... + def do_xmaton_le_test(self, machine): + self.require_netdev('user') + self.set_machine(machine) + self.archive_extract(self.ASSET_IMAGE_LE) + self.vm.set_console() + self.vm.add_args('-kernel', self.scratch_file('day13', 'xmaton.bin')) + tftproot = self.scratch_file('day13') + self.vm.add_args('-nic', f'user,tftp={tftproot}') + self.vm.launch() + wait_for_console_pattern(self, 'QEMU Advent Calendar 2023') + wait_for_console_pattern(self, 'buildroot login:') + exec_command_and_wait_for_pattern(self, 'root', '#') + exec_command_and_wait_for_pattern(self, + 'tftp -g -r xmaton.png 10.0.2.2 ; md5sum xmaton.png', + '821cd3cab8efd16ad6ee5acc3642a8ea') + +class MicroblazeBigEndianMachine(MicroblazeMachine): Add this here 'til the problem with the precaching is fixed: ASSET_IMAGE_BE = MicroblazeMachine.ASSET_IMAGE_BE ? Actually the cache works, I mis-interpreted the network issue. I'll update Daniel on the other thread.
Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum
On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. Endianness can be BIG, LITTLE or unspecified (default). Signed-off-by: Philippe Mathieu-Daudé --- qapi/common.json| 16 include/hw/qdev-properties-system.h | 7 +++ hw/core/qdev-properties-system.c| 11 +++ 3 files changed, 34 insertions(+) diff --git a/qapi/common.json b/qapi/common.json index 6ffc7a37890..217feaaf683 100644 --- a/qapi/common.json +++ b/qapi/common.json @@ -212,3 +212,19 @@ ## { 'struct': 'HumanReadableText', 'data': { 'human-readable-text': 'str' } } + +## +# @EndianMode: +# +# An enumeration of three options: little, big, and unspecified +# +# @little: Little endianness +# +# @big: Big endianness +# +# @unspecified: Endianness not specified +# +# Since: 10.0 +## +{ 'enum': 'EndianMode', + 'data': [ 'little', 'big', 'unspecified' ] } Should 'unspecified' come first? ... so that it gets the value 0, i.e. when someone forgets to properly initialize a related variable, the chances are higher that it ends up as "unspecified" than as "little" ? Apart from that: Reviewed-by: Thomas Huth
Re: [PULL 04/32] hw/timer/xilinx_timer: Make device endianness configurable
On 12/2/25 11:24, Thomas Huth wrote: On 12/02/2025 10.19, Philippe Mathieu-Daudé wrote: On 12/2/25 09:27, Thomas Huth wrote: On 10/02/2025 21.41, Philippe Mathieu-Daudé wrote: Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness for each machine using the device. Reviewed-by: Richard Henderson Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20250206131052.30207-5-phi...@linaro.org> --- hw/microblaze/petalogix_ml605_mmu.c | 1 + hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 + hw/ppc/virtex_ml507.c | 1 + hw/timer/xilinx_timer.c | 35 ++ +- 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/ petalogix_ml605_mmu.c index cf3b9574db3..bbda70aa93b 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -127,6 +127,7 @@ petalogix_ml605_init(MachineState *machine) /* 2 timers at irq 2 @ 100 Mhz. */ dev = qdev_new("xlnx.xps-timer"); + qdev_prop_set_bit(dev, "little-endian", true); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 100 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/ microblaze/ petalogix_s3adsp1800_mmu.c index fbf52ba8f2f..9d4316b4036 100644 --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c @@ -114,6 +114,7 @@ petalogix_s3adsp1800_init(MachineState *machine) /* 2 timers at irq 2 @ 62 Mhz. */ dev = qdev_new("xlnx.xps-timer"); + qdev_prop_set_bit(dev, "little-endian", !TARGET_BIG_ENDIAN); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 62 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c index 23238119273..f87c221d076 100644 --- a/hw/ppc/virtex_ml507.c +++ b/hw/ppc/virtex_ml507.c @@ -230,6 +230,7 @@ static void virtex_init(MachineState *machine) /* 2 timers at irq 2 @ 62 Mhz. */ dev = qdev_new("xlnx.xps-timer"); + qdev_prop_set_bit(dev, "little-endian", false); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 62 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); Hi, with this patch applied, the ppc_virtex_ml507 functional test is now failing for me ... could you please double-check whether "make check- functional-ppc" still works for you? Thanks, not this patch problem, but patch #2 misses: -- >8 -- diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c index 23238119273..723f62c904b 100644 --- a/hw/ppc/virtex_ml507.c +++ b/hw/ppc/virtex_ml507.c @@ -219,2 +219,3 @@ static void virtex_init(MachineState *machine) dev = qdev_new("xlnx.xps-intc"); + qdev_prop_set_bit(dev, "little-endian", false); qdev_prop_set_uint32(dev, "kind-of-intr", 0); --- Why is my CI green? Looking at https://gitlab.com/philmd/qemu/-/pipelines/1664238124 it seems like you did not start the functional test jobs? Doh, while doing my PR process I forgot even using push-ci-now these need to be triggered manually :S Thomas
Re: [PATCH v6 07/11] tests/functional: Avoid using www.qemu-advent-calendar.org URL
On 12/2/25 12:49, Thomas Huth wrote: On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: Avoid fetching assets from www.qemu-advent-calendar.org website, prefer fetching microblaze assets from GitLab servers. Suggested-by: Thomas Huth Signed-off-by: Philippe Mathieu-Daudé --- tests/functional/test_microblazeel_s3adsp1800.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/functional/test_microblazeel_s3adsp1800.py b/tests/ functional/test_microblazeel_s3adsp1800.py index c382afe6bfa..5bf94d88dd8 100755 --- a/tests/functional/test_microblazeel_s3adsp1800.py +++ b/tests/functional/test_microblazeel_s3adsp1800.py @@ -18,7 +18,8 @@ class MicroblazeelMachine(QemuSystemTest): timeout = 90 ASSET_IMAGE = Asset( - ('http://www.qemu-advent-calendar.org/2023/download/ day13.tar.gz'), + ('https://qemu-advcal.gitlab.io/qac-best-of-multiarch/download/' + 'day05.tar.xz'), 'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22') You'd likely need to adjust the sha256 sum as well since I repacked with xz instead of gz ... but according to Eldon, the original download should be working again, so I'd suggest to drop this patch for now. Indeed, I forgot to flush my cache before running my tests, and now figured it out: 9d031aa55fe988fddffab26932552c53dc9adeb75f30d04ece8abce02b226179 day05.tar.xz
Re: [PULL 00/32] Misc HW patches for 2025-02-10
On 12/2/25 10:10, Daniel P. Berrangé wrote: On Tue, Feb 11, 2025 at 07:53:32PM +0100, Philippe Mathieu-Daudé wrote: On 11/2/25 19:48, Philippe Mathieu-Daudé wrote: On 11/2/25 19:26, Stefan Hajnoczi wrote: On Mon, Feb 10, 2025 at 3:43 PM Philippe Mathieu-Daudé wrote: The following changes since commit 54e91d1523b412b4cff7cb36c898fa9dc133e886: Merge tag 'pull-qapi-2025-02-10-v2' of https://repo.or.cz/qemu/ armbru into staging (2025-02-10 10:47:31 -0500) are available in the Git repository at: https://github.com/philmd/qemu.git tags/hw-misc-20250210 for you to fetch changes up to 1078a376932cc1d1c501ee3643fef329da6a189a: hw/net/smc91c111: Ignore attempt to pop from empty RX fifo (2025-02-10 21:30:44 +0100) Misc HW patches - Use qemu_hexdump_line() in TPM backend (Philippe) - Make various Xilinx devices endianness configurable (Philippe) - Remove magic number in APIC (Phil) - Disable thread-level cache topology (Zhao) - Xen QOM style cleanups (Bernhard) - Introduce TYPE_DYNAMIC_SYS_BUS_DEVICE (Philippe) - Invert logic of machine no_sdcard flag (Philippe) - Housekeeping in MicroBlaze functional tests (Philippe) Please take a look at this CI failure: https://gitlab.com/qemu-project/qemu/-/jobs/9106591368 Hmm I can not reproduce locally this error: Exception: Asset cache is invalid and downloads disabled OK, I could reproduce by blowing my cache away. The problem seems in the "tests/functional: Have microblaze tests inherit common parent class" patch, which does: -class MicroblazeelMachine(QemuSystemTest): +class MicroblazeLittleEndianMachine(MicroblazeMachine): I presume, since MicroblazeLittleEndianMachine is no more a direct child of QemuSystemTest, then the ASSET_IMAGE_* aren't automatically downloaded. Yes, my code assumes all assets are on the leaf test classes. I'll look at a fix since it is easy enough to check parent classes too. Actually your code works fine :) I mis-interpreted the network issue: DEBUG:qemu-test:Extract /Users/philmd/.cache/qemu/download/b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22 format=Nonesub_dir=None member=None INFO:qemu-test:Downloading http://www.qemu-advent-calendar.org/2023/download/day13.tar.gz to /Users/philmd/.cache/qemu/download/b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22... DEBUG:qemu-test:Unable to set xattr on /Users/philmd/.cache/qemu/download/b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22.download: module 'os' has no attribute 'setxattr' INFO:qemu-test:Cached http://www.qemu-advent-calendar.org/2023/download/day13.tar.gz at /Users/philmd/.cache/qemu/download/b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22
Re: [PATCH v3 09/23] hw/uefi: add var-service-core.c
On Wed, Feb 12, 2025 at 12:30:20PM +0100, Alexander Graf wrote: > > On 12.02.25 11:24, Gerd Hoffmann wrote: > > > > Why do you use confidential computing in the first place if you trust > > the host with your EFI variables? I'd rather see something simliar > > running under guest control, in svsm context. > > That depends heavily on your threat model. You can use a host provided > variable store to gain variable persistence for things like boot variables > and then have an ephemeral SVSM based TPM that you use to measure the loaded > payloads. A malicious host can already replace your root volume, so > extending the threat to variables is not the end of the world. If you go depend on measured boot instead of secure boot then yes, this might be a workable model. Should be doable with a small svsm driver which forwards requests to the host via svsm-controlled bounce buffer (where the svsm has control over page properties). > > The firmware knows all this very well though. The OS passes a mapping > > table to the firmware, efi runtime drivers can subscribe to mapping > > updates and can use RT->ConvertPointer to translate addresses from > > physical to virtual. > > > > The edk2 code (https://github.com/tianocore/edk2/pull/10695) does > > exactly that. > > > > I see your driver does that too, so in theory it should work just fine. > > I'm wondering what exactly the problem with macOS is? > > You get to know the new virtual address, but ConvertPointer never tells you > what the new *physical* address is. That means you have no idea where to DMA > from once you're in virtual land. Yes. Knowing both physical and virtual address works only for memory you allocated yourself before ExitBootServices. So you can't pass on pointers from the OS, you have to copy the data to a buffer where you know the physical address instead. Yes, some overhead. Should still be much faster than going to pio transfer mode ... > > It would be nice to have nitro support merged upstream, > > especially with BYOF coming. > > Yes. Or converge on this protocol instead to simplify the firmware > implementation Yes, that works too and would reduce your stack of unmerged patches a bit. take care, Gerd
Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum
On Wed, Feb 12, 2025 at 01:02:18PM +0100, Philippe Mathieu-Daudé wrote: > On 12/2/25 12:43, Philippe Mathieu-Daudé wrote: > > On 12/2/25 12:37, Thomas Huth wrote: > > > On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: > > > > Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. > > > > Endianness can be BIG, LITTLE or unspecified (default). > > > > > > > > Signed-off-by: Philippe Mathieu-Daudé > > > > --- > > > > qapi/common.json | 16 > > > > include/hw/qdev-properties-system.h | 7 +++ > > > > hw/core/qdev-properties-system.c | 11 +++ > > > > 3 files changed, 34 insertions(+) > > > > > > > > diff --git a/qapi/common.json b/qapi/common.json > > > > index 6ffc7a37890..217feaaf683 100644 > > > > --- a/qapi/common.json > > > > +++ b/qapi/common.json > > > > @@ -212,3 +212,19 @@ > > > > ## > > > > { 'struct': 'HumanReadableText', > > > > 'data': { 'human-readable-text': 'str' } } > > > > + > > > > +## > > > > +# @EndianMode: > > > > +# > > > > +# An enumeration of three options: little, big, and unspecified > > > > +# > > > > +# @little: Little endianness > > > > +# > > > > +# @big: Big endianness > > > > +# > > > > +# @unspecified: Endianness not specified > > > > +# > > > > +# Since: 10.0 > > > > +## > > > > +{ 'enum': 'EndianMode', > > > > + 'data': [ 'little', 'big', 'unspecified' ] } > > > > > > Should 'unspecified' come first? ... so that it gets the value 0, > > > i.e. when someone forgets to properly initialize a related variable, > > > the chances are higher that it ends up as "unspecified" than as > > > "little" ? > > BTW I'm not sure QAPI guaranty enums are following an order > (at least, as in C, I wouldn't rely on that assumption). If we don't document a guaranteed order IMHO we should, mostly just for the sake for guaranteeing exactly what will be the 0 value . It is pretty common to want a particular enum constant to be special default for the 0 value. It allows enums to be retrofitted into existing code, with confidence that any code forgetting to initialize a variable/field will get the special default. Missed initialization is relatively common as a C bug, and we use -ftrivial-auto-var-init=zero to give well defined (usually safe) semantics. 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 v7] target/riscv: Add support to access ctrsource, ctrtarget, ctrdata regs.
CTR entries are accessed using ctrsource, ctrtarget and ctrdata registers using smcsrind/sscsrind extension. This commits extends the csrind extension to support CTR registers. ctrsource is accessible through xireg CSR, ctrtarget is accessible through xireg1 and ctrdata is accessible through xireg2 CSR. CTR supports maximum depth of 256 entries which are accessed using xiselect range 0x200 to 0x2ff. This commits also adds properties to enable CTR extension. CTR can be enabled using smctr=true and ssctr=true now. Signed-off-by: Rajnesh Kanwal Acked-by: Alistair Francis --- This series enables Control Transfer Records extension support on riscv platform. This extension is similar to Arch LBR in x86 and BRBE in ARM. The Extension has been ratified and this series is based on v1.0 [0] CTR extension depends on both the implementation of S-mode and Sscsrind extension v1.0.0 [1]. CTR access ctrsource, ctrtartget and ctrdata CSRs using sscsrind extension. The series is based on Smcdeleg/Ssccfg counter delegation extension [2] patches [3]. CTR itself doesn't depend on counter delegation support. This rebase is basically to include the Smcsrind patches. Here is the link to a quick start guide [4] to setup and run a basic perf demo on Linux to use CTR Ext. Qemu patches can be found here: https://github.com/rajnesh-kanwal/qemu/tree/b4/ctr_upstream_v7 Opensbi patch can be found here: https://github.com/rajnesh-kanwal/opensbi/tree/ctr_upstream_v2 Linux kernel patches can be found here: https://github.com/rajnesh-kanwal/linux/tree/b4/ctr_upstream_v2 [0]: https://github.com/riscv/riscv-control-transfer-records/releases/tag/v1.0 [1]: https://github.com/riscvarchive/riscv-indirect-csr-access/releases/tag/v1.0.0 [2]: https://github.com/riscvarchive/riscv-smcdeleg-ssccfg/releases/tag/v1.0.0 [3]: https://lore.kernel.org/qemu-riscv/20241203-counter_delegation-v4-0-c12a89bae...@rivosinc.com/ [4]: https://github.com/rajnesh-kanwal/linux/wiki/Running-CTR-basic-demo-on-QEMU-RISC%E2%80%90V-Virt-machine --- Changes in v7: v7: Rebased on latest riscv-to-apply.next. Given 6 out of 7 patches are already in riscv-to-apply.next, this version only contains the last patch which failed to apply. v6: Rebased on latest riscv-to-apply.for-upstream. - https://lore.kernel.org/qemu-devel/20250205-b4-ctr_upstream_v6-v6-0-439d8e06c...@rivosinc.com v5: Improvements based on Richard Henderson's feedback. - Fixed code gen logic to use gen_update_pc() instead of tcg_constant_tl(). - Some function renaming. - Rebased onto v4 of counter delegation series. - https://lore.kernel.org/qemu-riscv/20241205-b4-ctr_upstream_v3-v5-0-60b993aa5...@rivosinc.com/ v4: Improvements based on Richard Henderson's feedback. - Refactored CTR related code generation to move more code into translation side and avoid unnecessary code execution in generated code. - Added missing code in machine.c to migrate the new state. - https://lore.kernel.org/r/20241204-b4-ctr_upstream_v3-v4-0-d3ce6bef9...@rivosinc.com v3: Improvements based on Jason Chien and Frank Chang's feedback. - Created single set of MACROs for CTR CSRs in cpu_bit.h - Some fixes in riscv_ctr_add_entry. - Return zero for vs/sireg4-6 for CTR 0x200 to 0x2ff range. - Improved extension dependency check. - Fixed invalid ctrctl csr selection bug in riscv_ctr_freeze. - Added implied rules for Smctr and Ssctr. - Added missing SMSTATEEN0_CTR bit in mstateen0 and hstateen0 write ops. - Some more cosmetic changes. - https://lore.kernel.org/qemu-riscv/20241104-b4-ctr_upstream_v3-v3-0-32fd3c482...@rivosinc.com/ v2: Lots of improvements based on Jason Chien's feedback including: - Added CTR recording for cm.jalt, cm.jt, cm.popret, cm.popretz. - Fixed and added more CTR extension enable checks. - Fixed CTR CSR predicate functions. - Fixed external trap xTE bit checks. - One fix in freeze function for VS-mode. - Lots of minor code improvements. - Added checks in sctrclr instruction helper. - https://lore.kernel.org/qemu-riscv/20240619152708.135991-1-rkan...@rivosinc.com/ v1: - https://lore.kernel.org/qemu-riscv/20240529160950.132754-1-rkan...@rivosinc.com/ --- target/riscv/cpu.c | 26 +++- target/riscv/csr.c | 150 - target/riscv/tcg/tcg-cpu.c | 11 3 files changed, 185 insertions(+), 2 deletions(-) diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 8264c81e889424dfd491cec0ef95eeffc8fcc5b6..522d6584e4c3be7070e5a59f70f5948be8196a77 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -216,6 +216,8 @@ const RISCVIsaExtData isa_edata_arr[] = { ISA_EXT_DATA_ENTRY(ssu64xl, PRIV_VERSION_1_12_0, has_priv_1_12), ISA_EXT_DATA_ENTRY(supm, PRIV_VERSION_1_13_0, ext_supm), ISA_EXT_DATA_ENTRY(svade, PRIV_VERSION_1_11_0, ext_svade), +ISA_EXT_DATA_ENTRY(smctr, PRIV_VERSION_1_12_0, ext_smctr), +ISA_EXT_DATA_ENTRY(ssctr, PRIV_VERSION_1_12_0,
Re: [PULL 04/32] hw/timer/xilinx_timer: Make device endianness configurable
On 12/02/2025 10.19, Philippe Mathieu-Daudé wrote: On 12/2/25 09:27, Thomas Huth wrote: On 10/02/2025 21.41, Philippe Mathieu-Daudé wrote: Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness for each machine using the device. Reviewed-by: Richard Henderson Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20250206131052.30207-5-phi...@linaro.org> --- hw/microblaze/petalogix_ml605_mmu.c | 1 + hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 + hw/ppc/virtex_ml507.c | 1 + hw/timer/xilinx_timer.c | 35 +++- 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/ petalogix_ml605_mmu.c index cf3b9574db3..bbda70aa93b 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -127,6 +127,7 @@ petalogix_ml605_init(MachineState *machine) /* 2 timers at irq 2 @ 100 Mhz. */ dev = qdev_new("xlnx.xps-timer"); + qdev_prop_set_bit(dev, "little-endian", true); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 100 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/ petalogix_s3adsp1800_mmu.c index fbf52ba8f2f..9d4316b4036 100644 --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c @@ -114,6 +114,7 @@ petalogix_s3adsp1800_init(MachineState *machine) /* 2 timers at irq 2 @ 62 Mhz. */ dev = qdev_new("xlnx.xps-timer"); + qdev_prop_set_bit(dev, "little-endian", !TARGET_BIG_ENDIAN); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 62 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c index 23238119273..f87c221d076 100644 --- a/hw/ppc/virtex_ml507.c +++ b/hw/ppc/virtex_ml507.c @@ -230,6 +230,7 @@ static void virtex_init(MachineState *machine) /* 2 timers at irq 2 @ 62 Mhz. */ dev = qdev_new("xlnx.xps-timer"); + qdev_prop_set_bit(dev, "little-endian", false); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 62 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); Hi, with this patch applied, the ppc_virtex_ml507 functional test is now failing for me ... could you please double-check whether "make check- functional-ppc" still works for you? Thanks, not this patch problem, but patch #2 misses: -- >8 -- diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c index 23238119273..723f62c904b 100644 --- a/hw/ppc/virtex_ml507.c +++ b/hw/ppc/virtex_ml507.c @@ -219,2 +219,3 @@ static void virtex_init(MachineState *machine) dev = qdev_new("xlnx.xps-intc"); + qdev_prop_set_bit(dev, "little-endian", false); qdev_prop_set_uint32(dev, "kind-of-intr", 0); --- Why is my CI green? Looking at https://gitlab.com/philmd/qemu/-/pipelines/1664238124 it seems like you did not start the functional test jobs? Thomas
Re: [PULL 04/32] hw/timer/xilinx_timer: Make device endianness configurable
On 12/2/25 10:19, Philippe Mathieu-Daudé wrote: On 12/2/25 09:27, Thomas Huth wrote: On 10/02/2025 21.41, Philippe Mathieu-Daudé wrote: Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness for each machine using the device. Reviewed-by: Richard Henderson Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20250206131052.30207-5-phi...@linaro.org> --- hw/microblaze/petalogix_ml605_mmu.c | 1 + hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 + hw/ppc/virtex_ml507.c | 1 + hw/timer/xilinx_timer.c | 35 +++- 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/ petalogix_ml605_mmu.c index cf3b9574db3..bbda70aa93b 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -127,6 +127,7 @@ petalogix_ml605_init(MachineState *machine) /* 2 timers at irq 2 @ 100 Mhz. */ dev = qdev_new("xlnx.xps-timer"); + qdev_prop_set_bit(dev, "little-endian", true); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 100 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/ microblaze/ petalogix_s3adsp1800_mmu.c index fbf52ba8f2f..9d4316b4036 100644 --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c @@ -114,6 +114,7 @@ petalogix_s3adsp1800_init(MachineState *machine) /* 2 timers at irq 2 @ 62 Mhz. */ dev = qdev_new("xlnx.xps-timer"); + qdev_prop_set_bit(dev, "little-endian", !TARGET_BIG_ENDIAN); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 62 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c index 23238119273..f87c221d076 100644 --- a/hw/ppc/virtex_ml507.c +++ b/hw/ppc/virtex_ml507.c @@ -230,6 +230,7 @@ static void virtex_init(MachineState *machine) /* 2 timers at irq 2 @ 62 Mhz. */ dev = qdev_new("xlnx.xps-timer"); + qdev_prop_set_bit(dev, "little-endian", false); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 62 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); Hi, with this patch applied, the ppc_virtex_ml507 functional test is now failing for me ... could you please double-check whether "make check- functional-ppc" still works for you? Thanks, not this patch problem, but patch #2 misses: -- >8 -- diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c index 23238119273..723f62c904b 100644 --- a/hw/ppc/virtex_ml507.c +++ b/hw/ppc/virtex_ml507.c @@ -219,2 +219,3 @@ static void virtex_init(MachineState *machine) dev = qdev_new("xlnx.xps-intc"); + qdev_prop_set_bit(dev, "little-endian", false); It looks to me like another "default value problem", where using a tri-state with default=unset would have catched. "disas/dis-asm.h" defines: enum bfd_endian { BFD_ENDIAN_BIG, BFD_ENDIAN_LITTLE, BFD_ENDIAN_UNKNOWN }; Maybe endianness is common enough than we can add a QAPI enum, in machine-common.json or even common.json? Then I'd use qdev_prop_set_enum() here. qdev_prop_set_uint32(dev, "kind-of-intr", 0); --- Why is my CI green?
[PATCH] block/rbd: Do not use BDS's AioContext
RBD schedules the request completion code (qemu_rbd_finish_bh()) to run in the BDS's AioContext. The intent seems to be to run it in the same context that the original request coroutine ran in, i.e. the thread on whose stack the RBDTask object exists (see qemu_rbd_start_co()). However, with multiqueue, that thread is not necessarily the same as the BDS's AioContext. Instead, we need to remember the actual AioContext and schedule the completion BH there. Buglink: https://issues.redhat.com/browse/RHEL-67115 Reported-by: Junyao Zhao Signed-off-by: Hanna Czenczek --- I think I could also drop RBDTask.ctx and just use `qemu_coroutine_get_aio_context(RBDTask.co)` instead, but this is the version of the patch that was tested and confirmed to fix the issue (I don't have a local reproducer), so I thought I'll post this first. --- block/rbd.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/block/rbd.c b/block/rbd.c index af984fb7db..9d4e0817e0 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -102,7 +102,7 @@ typedef struct BDRVRBDState { } BDRVRBDState; typedef struct RBDTask { -BlockDriverState *bs; +AioContext *ctx; Coroutine *co; bool complete; int64_t ret; @@ -1269,8 +1269,7 @@ static void qemu_rbd_completion_cb(rbd_completion_t c, RBDTask *task) { task->ret = rbd_aio_get_return_value(c); rbd_aio_release(c); -aio_bh_schedule_oneshot(bdrv_get_aio_context(task->bs), -qemu_rbd_finish_bh, task); +aio_bh_schedule_oneshot(task->ctx, qemu_rbd_finish_bh, task); } static int coroutine_fn qemu_rbd_start_co(BlockDriverState *bs, @@ -1281,7 +1280,10 @@ static int coroutine_fn qemu_rbd_start_co(BlockDriverState *bs, RBDAIOCmd cmd) { BDRVRBDState *s = bs->opaque; -RBDTask task = { .bs = bs, .co = qemu_coroutine_self() }; +RBDTask task = { +.ctx = qemu_get_current_aio_context(), +.co = qemu_coroutine_self(), +}; rbd_completion_t c; int r; -- 2.48.1
Re: [PULL 00/32] Misc HW patches for 2025-02-10
On Tue, Feb 11, 2025 at 07:53:32PM +0100, Philippe Mathieu-Daudé wrote: > On 11/2/25 19:48, Philippe Mathieu-Daudé wrote: > > On 11/2/25 19:26, Stefan Hajnoczi wrote: > > > On Mon, Feb 10, 2025 at 3:43 PM Philippe Mathieu-Daudé > > > wrote: > > > > > > > > The following changes since commit > > > > 54e91d1523b412b4cff7cb36c898fa9dc133e886: > > > > > > > > Merge tag 'pull-qapi-2025-02-10-v2' of > > > > https://repo.or.cz/qemu/ armbru into staging (2025-02-10 > > > > 10:47:31 -0500) > > > > > > > > are available in the Git repository at: > > > > > > > > https://github.com/philmd/qemu.git tags/hw-misc-20250210 > > > > > > > > for you to fetch changes up to 1078a376932cc1d1c501ee3643fef329da6a189a: > > > > > > > > hw/net/smc91c111: Ignore attempt to pop from empty RX fifo > > > > (2025-02-10 21:30:44 +0100) > > > > > > > > > > > > Misc HW patches > > > > > > > > - Use qemu_hexdump_line() in TPM backend (Philippe) > > > > - Make various Xilinx devices endianness configurable (Philippe) > > > > - Remove magic number in APIC (Phil) > > > > - Disable thread-level cache topology (Zhao) > > > > - Xen QOM style cleanups (Bernhard) > > > > - Introduce TYPE_DYNAMIC_SYS_BUS_DEVICE (Philippe) > > > > - Invert logic of machine no_sdcard flag (Philippe) > > > > - Housekeeping in MicroBlaze functional tests (Philippe) > > > > > > Please take a look at this CI failure: > > > https://gitlab.com/qemu-project/qemu/-/jobs/9106591368 > > > > Hmm I can not reproduce locally this error: > > > > Exception: Asset cache is invalid and downloads disabled > > OK, I could reproduce by blowing my cache away. > > The problem seems in the "tests/functional: Have microblaze tests > inherit common parent class" patch, which does: > > -class MicroblazeelMachine(QemuSystemTest): > +class MicroblazeLittleEndianMachine(MicroblazeMachine): > > I presume, since MicroblazeLittleEndianMachine is no more a direct > child of QemuSystemTest, then the ASSET_IMAGE_* aren't automatically > downloaded. Yes, my code assumes all assets are on the leaf test classes. I'll look at a fix since it is easy enough to check parent classes too. 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 v3 09/23] hw/uefi: add var-service-core.c
Hi, > > +/* read header */ > > +dma_memory_read(&address_space_memory, dma, > > +uv->buffer, sizeof(*mhdr), > > +MEMTXATTRS_UNSPECIFIED); > > Depending on DMA sounds appealing at first, but can fall apart in corner > cases. I know of 2 cases where DMA failed for me in the EC2 equivalent of > this: > > 1) SEV-SNP. If you want the hypervisor to implement UEFI variable services > for you, the buffer region must always be in shared state. Ensuring that > during boot time is tricky but doable. At runtime you no longer really have > control over the sharability of pages. With SEV-SNP I don't see the point in using this. Why do you use confidential computing in the first place if you trust the host with your EFI variables? I'd rather see something simliar running under guest control, in svsm context. > 2) Mac OS X. MacOS is the only OS I'm aware of that really makes use of > relocation. They move your physical pages to random locations, give you a > non-1:1 mapping to that and once you're in real OS land, you have no more > knowledge at all about the physical location of anything. On the host side you have no insight into this indeed. The firmware knows all this very well though. The OS passes a mapping table to the firmware, efi runtime drivers can subscribe to mapping updates and can use RT->ConvertPointer to translate addresses from physical to virtual. The edk2 code (https://github.com/tianocore/edk2/pull/10695) does exactly that. I see your driver does that too, so in theory it should work just fine. I'm wondering what exactly the problem with macOS is? > Also, I'm surprised you cut the variable service off at the SMM boundary > instead of the RTS callback boundary. Why is that cleaner/better than > implementing variables completely in QEMU? Well, the variable service /is/ completely in qemu. See patch #6 which implements getvariable & friends. edk2 serializes the variable calls into a buffer and sends them over to the SMM side (or to qemu with the patches). I didn't feel like inventing a new serialization protocol if we already have a proven one in the edk2 code base. Also it is possible to send over more than just the variable call. There is a variable policy protocol implementation (patch #8), and we also get some events forwarded. More can easily be added should the need for that arise. > It would be nice to agree on a single external variable store implementation > :). It would be nice to have nitro support merged upstream, especially with BYOF coming. take care, Gerd
[PATCH v7 03/10] hw/net/xilinx_ethlite: Make device endianness configurable
Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness on the single machine using the device. Signed-off-by: Philippe Mathieu-Daudé --- hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 + hw/net/xilinx_ethlite.c | 29 +++- hw/riscv/microblaze-v-generic.c | 1 + 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c index 15cabe11777..d419dc49a25 100644 --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c @@ -123,6 +123,7 @@ petalogix_s3adsp1800_init(MachineState *machine) sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq[TIMER_IRQ]); dev = qdev_new("xlnx.xps-ethernetlite"); +qdev_prop_set_enum(dev, "endianness", endianness); qemu_configure_nic_device(dev, true, NULL); qdev_prop_set_uint32(dev, "tx-ping-pong", 0); qdev_prop_set_uint32(dev, "rx-ping-pong", 0); diff --git a/hw/net/xilinx_ethlite.c b/hw/net/xilinx_ethlite.c index 14bf2b2e17a..15d9b95aa80 100644 --- a/hw/net/xilinx_ethlite.c +++ b/hw/net/xilinx_ethlite.c @@ -34,6 +34,7 @@ #include "hw/sysbus.h" #include "hw/irq.h" #include "hw/qdev-properties.h" +#include "hw/qdev-properties-system.h" #include "hw/misc/unimp.h" #include "net/net.h" #include "trace.h" @@ -85,6 +86,7 @@ struct XlnxXpsEthLite { SysBusDevice parent_obj; +EndianMode model_endianness; MemoryRegion container; qemu_irq irq; NICState *nic; @@ -183,10 +185,10 @@ static void port_tx_write(void *opaque, hwaddr addr, uint64_t value, } } -static const MemoryRegionOps eth_porttx_ops = { +static const MemoryRegionOps eth_porttx_ops[2] = { +[0 ... 1] = { .read = port_tx_read, .write = port_tx_write, -.endianness = DEVICE_NATIVE_ENDIAN, .impl = { .min_access_size = 4, .max_access_size = 4, @@ -195,6 +197,9 @@ static const MemoryRegionOps eth_porttx_ops = { .min_access_size = 4, .max_access_size = 4, }, +}, +[0].endianness = DEVICE_LITTLE_ENDIAN, +[1].endianness = DEVICE_BIG_ENDIAN, }; static uint64_t port_rx_read(void *opaque, hwaddr addr, unsigned int size) @@ -232,10 +237,10 @@ static void port_rx_write(void *opaque, hwaddr addr, uint64_t value, } } -static const MemoryRegionOps eth_portrx_ops = { +static const MemoryRegionOps eth_portrx_ops[2] = { +[0 ... 1] = { .read = port_rx_read, .write = port_rx_write, -.endianness = DEVICE_NATIVE_ENDIAN, .impl = { .min_access_size = 4, .max_access_size = 4, @@ -244,6 +249,9 @@ static const MemoryRegionOps eth_portrx_ops = { .min_access_size = 4, .max_access_size = 4, }, +}, +[0].endianness = DEVICE_LITTLE_ENDIAN, +[1].endianness = DEVICE_BIG_ENDIAN, }; static bool eth_can_rx(NetClientState *nc) @@ -300,6 +308,14 @@ static NetClientInfo net_xilinx_ethlite_info = { static void xilinx_ethlite_realize(DeviceState *dev, Error **errp) { XlnxXpsEthLite *s = XILINX_ETHLITE(dev); +unsigned ops_index; + +if (s->model_endianness == ENDIAN_MODE_UNSPECIFIED) { +error_setg(errp, TYPE_XILINX_ETHLITE " property 'endianness'" + " must be set to 'big' or 'little'"); +return; +} +ops_index = s->model_endianness == ENDIAN_MODE_BIG ? 1 : 0; memory_region_init(&s->container, OBJECT(dev), "xlnx.xps-ethernetlite", 0x2000); @@ -328,7 +344,7 @@ static void xilinx_ethlite_realize(DeviceState *dev, Error **errp) BUFSZ_MAX, &error_abort); memory_region_add_subregion(&s->container, 0x0800 * i, &s->port[i].txbuf); memory_region_init_io(&s->port[i].txio, OBJECT(dev), - ð_porttx_ops, s, + ð_porttx_ops[ops_index], s, i ? "ethlite.tx[1]io" : "ethlite.tx[0]io", 4 * TX_MAX); memory_region_add_subregion(&s->container, i ? A_TX_BASE1 : A_TX_BASE0, @@ -340,7 +356,7 @@ static void xilinx_ethlite_realize(DeviceState *dev, Error **errp) memory_region_add_subregion(&s->container, 0x1000 + 0x0800 * i, &s->port[i].rxbuf); memory_region_init_io(&s->port[i].rxio, OBJECT(dev), - ð_portrx_ops, s, + ð_portrx_ops[ops_index], s, i ? "ethlite.rx[1]io" : "ethlite.rx[0]io", 4 * RX_MAX); memory_region_add_subregion(&s->container, i ? A_RX_BASE1 : A_RX_BASE0, @@ -363,6 +379,7 @@ static void xilinx_ethlite_in
[PATCH v7 07/10] tests/functional: Explicit endianness of microblaze assets
The archive used in test_microblaze_s3adsp1800.py (testing a big-endian target) contains a big-endian kernel. Rename using the _BE suffix. Similarly, the archive in test_microblazeel_s3adsp1800 (testing a little-endian target) contains a little-endian kernel. Rename using _LE suffix. These changes will help when adding cross-endian kernel tests. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Richard Henderson Reviewed-by: Thomas Huth Message-Id: <20250206131052.30207-13-phi...@linaro.org> --- tests/functional/test_microblaze_s3adsp1800.py | 6 +++--- tests/functional/test_microblazeel_s3adsp1800.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/functional/test_microblaze_s3adsp1800.py b/tests/functional/test_microblaze_s3adsp1800.py index 2c4464bd05a..fac364b1ea9 100755 --- a/tests/functional/test_microblaze_s3adsp1800.py +++ b/tests/functional/test_microblaze_s3adsp1800.py @@ -15,14 +15,14 @@ class MicroblazeMachine(QemuSystemTest): timeout = 90 -ASSET_IMAGE = Asset( +ASSET_IMAGE_BE = Asset( ('https://qemu-advcal.gitlab.io/qac-best-of-multiarch/download/' 'day17.tar.xz'), '3ba7439dfbea7af4876662c97f8e1f0cdad9231fc166e4861d17042489270057') -def test_microblaze_s3adsp1800(self): +def test_microblaze_s3adsp1800_be(self): self.set_machine('petalogix-s3adsp1800') -self.archive_extract(self.ASSET_IMAGE) +self.archive_extract(self.ASSET_IMAGE_BE) self.vm.set_console() self.vm.add_args('-kernel', self.scratch_file('day17', 'ballerina.bin')) diff --git a/tests/functional/test_microblazeel_s3adsp1800.py b/tests/functional/test_microblazeel_s3adsp1800.py index c382afe6bfa..5d353dba5d2 100755 --- a/tests/functional/test_microblazeel_s3adsp1800.py +++ b/tests/functional/test_microblazeel_s3adsp1800.py @@ -17,14 +17,14 @@ class MicroblazeelMachine(QemuSystemTest): timeout = 90 -ASSET_IMAGE = Asset( +ASSET_IMAGE_LE = Asset( ('http://www.qemu-advent-calendar.org/2023/download/day13.tar.gz'), 'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22') -def test_microblazeel_s3adsp1800(self): +def test_microblazeel_s3adsp1800_le(self): self.require_netdev('user') self.set_machine('petalogix-s3adsp1800') -self.archive_extract(self.ASSET_IMAGE) +self.archive_extract(self.ASSET_IMAGE_LE) self.vm.set_console() self.vm.add_args('-kernel', self.scratch_file('day13', 'xmaton.bin')) tftproot = self.scratch_file('day13') -- 2.47.1
[PATCH v7 09/10] tests/functional: Remove sleep() kludges from microblaze tests
Commit f0ec14c78c4 ("tests/avocado: Fix console data loss") fixed QEMUMachine's problem with console, we don't need to use the sleep() kludges. Suggested-by: Thomas Huth Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Thomas Huth Reviewed-by: Richard Henderson Message-Id: <20250206131052.30207-15-phi...@linaro.org> --- tests/functional/test_microblazeel_s3adsp1800.py | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/functional/test_microblazeel_s3adsp1800.py b/tests/functional/test_microblazeel_s3adsp1800.py index 715ef3f79ac..60aab4a45e8 100755 --- a/tests/functional/test_microblazeel_s3adsp1800.py +++ b/tests/functional/test_microblazeel_s3adsp1800.py @@ -7,8 +7,7 @@ # This work is licensed under the terms of the GNU GPL, version 2 or # later. See the COPYING file in the top-level directory. -import time -from qemu_test import exec_command, exec_command_and_wait_for_pattern +from qemu_test import exec_command_and_wait_for_pattern from qemu_test import QemuSystemTest, Asset from qemu_test import wait_for_console_pattern @@ -31,9 +30,8 @@ def do_xmaton_le_test(self, machine): self.vm.add_args('-nic', f'user,tftp={tftproot}') self.vm.launch() wait_for_console_pattern(self, 'QEMU Advent Calendar 2023') -time.sleep(0.1) -exec_command(self, 'root') -time.sleep(0.1) +wait_for_console_pattern(self, 'buildroot login:') +exec_command_and_wait_for_pattern(self, 'root', '#') exec_command_and_wait_for_pattern(self, 'tftp -g -r xmaton.png 10.0.2.2 ; md5sum xmaton.png', '821cd3cab8efd16ad6ee5acc3642a8ea') -- 2.47.1
[PATCH v7 02/10] hw/intc/xilinx_intc: Make device endianness configurable
Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness for each machine using the device. Signed-off-by: Philippe Mathieu-Daudé --- hw/intc/xilinx_intc.c| 59 ++-- hw/microblaze/petalogix_ml605_mmu.c | 1 + hw/microblaze/petalogix_s3adsp1800_mmu.c | 3 ++ hw/ppc/virtex_ml507.c| 1 + hw/riscv/microblaze-v-generic.c | 1 + 5 files changed, 51 insertions(+), 14 deletions(-) diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c index 6930f83907a..ab1c4a32221 100644 --- a/hw/intc/xilinx_intc.c +++ b/hw/intc/xilinx_intc.c @@ -3,6 +3,9 @@ * * Copyright (c) 2009 Edgar E. Iglesias. * + * https://docs.amd.com/v/u/en-US/xps_intc + * DS572: LogiCORE IP XPS Interrupt Controller (v2.01a) + * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal * in the Software without restriction, including without limitation the rights @@ -23,10 +26,12 @@ */ #include "qemu/osdep.h" +#include "qapi/error.h" #include "hw/sysbus.h" #include "qemu/module.h" #include "hw/irq.h" #include "hw/qdev-properties.h" +#include "hw/qdev-properties-system.h" #include "qom/object.h" #define D(x) @@ -49,6 +54,7 @@ struct XpsIntc { SysBusDevice parent_obj; +EndianMode model_endianness; MemoryRegion mmio; qemu_irq parent_irq; @@ -140,18 +146,28 @@ static void pic_write(void *opaque, hwaddr addr, update_irq(p); } -static const MemoryRegionOps pic_ops = { -.read = pic_read, -.write = pic_write, -.endianness = DEVICE_NATIVE_ENDIAN, -.impl = { -.min_access_size = 4, -.max_access_size = 4, +static const MemoryRegionOps pic_ops[2] = { +[0 ... 1] = { +.read = pic_read, +.write = pic_write, +.impl = { +.min_access_size = 4, +.max_access_size = 4, +}, +.valid = { +/* + * All XPS INTC registers are accessed through the PLB interface. + * The base address for these registers is provided by the + * configuration parameter, C_BASEADDR. Each register is 32 bits + * although some bits may be unused and is accessed on a 4-byte + * boundary offset from the base address. + */ +.min_access_size = 4, +.max_access_size = 4, +}, }, -.valid = { -.min_access_size = 4, -.max_access_size = 4 -} +[0].endianness = DEVICE_LITTLE_ENDIAN, +[1].endianness = DEVICE_BIG_ENDIAN, }; static void irq_handler(void *opaque, int irq, int level) @@ -174,13 +190,27 @@ static void xilinx_intc_init(Object *obj) qdev_init_gpio_in(DEVICE(obj), irq_handler, 32); sysbus_init_irq(SYS_BUS_DEVICE(obj), &p->parent_irq); - -memory_region_init_io(&p->mmio, obj, &pic_ops, p, "xlnx.xps-intc", - R_MAX * 4); sysbus_init_mmio(SYS_BUS_DEVICE(obj), &p->mmio); } +static void xilinx_intc_realize(DeviceState *dev, Error **errp) +{ +XpsIntc *p = XILINX_INTC(dev); + +if (p->model_endianness == ENDIAN_MODE_UNSPECIFIED) { +error_setg(errp, TYPE_XILINX_INTC " property 'endianness'" + " must be set to 'big' or 'little'"); +return; +} + +memory_region_init_io(&p->mmio, OBJECT(dev), + &pic_ops[p->model_endianness == ENDIAN_MODE_BIG], + p, "xlnx.xps-intc", + R_MAX * 4); +} + static const Property xilinx_intc_properties[] = { +DEFINE_PROP_ENDIAN_NODEFAULT("endianness", XpsIntc, model_endianness), DEFINE_PROP_UINT32("kind-of-intr", XpsIntc, c_kind_of_intr, 0), }; @@ -188,6 +218,7 @@ static void xilinx_intc_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); +dc->realize = xilinx_intc_realize; device_class_set_props(dc, xilinx_intc_properties); } diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c index 8b44be75a22..55398cc67d1 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -111,6 +111,7 @@ petalogix_ml605_init(MachineState *machine) dev = qdev_new("xlnx.xps-intc"); +qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE); qdev_prop_set_uint32(dev, "kind-of-intr", 1 << TIMER_IRQ); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, INTC_BASEADDR); diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c index 2c0d8c34cd2..15cabe11777 100644 --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c +++ b/hw/microblaze/petalogix_s3adsp1
[PATCH v7 10/10] tests/functional: Have microblaze tests inherit common parent class
Have the MicroblazeMachine class being common to both MicroblazeBigEndianMachine and MicroblazeLittleEndianMachine classes. Move the xmaton and ballerina tests to the parent class. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Thomas Huth Message-Id: <20250206131052.30207-16-phi...@linaro.org> --- .../functional/test_microblaze_s3adsp1800.py | 23 +++ .../test_microblazeel_s3adsp1800.py | 29 ++- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/tests/functional/test_microblaze_s3adsp1800.py b/tests/functional/test_microblaze_s3adsp1800.py index c4226f49cf3..177c8a685bc 100755 --- a/tests/functional/test_microblaze_s3adsp1800.py +++ b/tests/functional/test_microblaze_s3adsp1800.py @@ -7,6 +7,7 @@ # This work is licensed under the terms of the GNU GPL, version 2 or # later. See the COPYING file in the top-level directory. +from qemu_test import exec_command_and_wait_for_pattern from qemu_test import QemuSystemTest, Asset from qemu_test import wait_for_console_pattern @@ -20,6 +21,10 @@ class MicroblazeMachine(QemuSystemTest): 'day17.tar.xz'), '3ba7439dfbea7af4876662c97f8e1f0cdad9231fc166e4861d17042489270057') +ASSET_IMAGE_LE = Asset( +('http://www.qemu-advent-calendar.org/2023/download/day13.tar.gz'), +'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22') + def do_ballerina_be_test(self, machine): self.set_machine(machine) self.archive_extract(self.ASSET_IMAGE_BE) @@ -34,6 +39,24 @@ def do_ballerina_be_test(self, machine): # message, that's why we don't test for a later string here. This # needs some investigation by a microblaze wizard one day... +def do_xmaton_le_test(self, machine): +self.require_netdev('user') +self.set_machine(machine) +self.archive_extract(self.ASSET_IMAGE_LE) +self.vm.set_console() +self.vm.add_args('-kernel', self.scratch_file('day13', 'xmaton.bin')) +tftproot = self.scratch_file('day13') +self.vm.add_args('-nic', f'user,tftp={tftproot}') +self.vm.launch() +wait_for_console_pattern(self, 'QEMU Advent Calendar 2023') +wait_for_console_pattern(self, 'buildroot login:') +exec_command_and_wait_for_pattern(self, 'root', '#') +exec_command_and_wait_for_pattern(self, +'tftp -g -r xmaton.png 10.0.2.2 ; md5sum xmaton.png', +'821cd3cab8efd16ad6ee5acc3642a8ea') + +class MicroblazeBigEndianMachine(MicroblazeMachine): + def test_microblaze_s3adsp1800_legacy_be(self): self.do_ballerina_be_test('petalogix-s3adsp1800') diff --git a/tests/functional/test_microblazeel_s3adsp1800.py b/tests/functional/test_microblazeel_s3adsp1800.py index 60aab4a45e8..56645bd0bb2 100755 --- a/tests/functional/test_microblazeel_s3adsp1800.py +++ b/tests/functional/test_microblazeel_s3adsp1800.py @@ -7,34 +7,11 @@ # This work is licensed under the terms of the GNU GPL, version 2 or # later. See the COPYING file in the top-level directory. -from qemu_test import exec_command_and_wait_for_pattern -from qemu_test import QemuSystemTest, Asset -from qemu_test import wait_for_console_pattern +from qemu_test import QemuSystemTest +from test_microblaze_s3adsp1800 import MicroblazeMachine -class MicroblazeelMachine(QemuSystemTest): - -timeout = 90 - -ASSET_IMAGE_LE = Asset( -('http://www.qemu-advent-calendar.org/2023/download/day13.tar.gz'), -'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22') - -def do_xmaton_le_test(self, machine): -self.require_netdev('user') -self.set_machine(machine) -self.archive_extract(self.ASSET_IMAGE_LE) -self.vm.set_console() -self.vm.add_args('-kernel', self.scratch_file('day13', 'xmaton.bin')) -tftproot = self.scratch_file('day13') -self.vm.add_args('-nic', f'user,tftp={tftproot}') -self.vm.launch() -wait_for_console_pattern(self, 'QEMU Advent Calendar 2023') -wait_for_console_pattern(self, 'buildroot login:') -exec_command_and_wait_for_pattern(self, 'root', '#') -exec_command_and_wait_for_pattern(self, -'tftp -g -r xmaton.png 10.0.2.2 ; md5sum xmaton.png', -'821cd3cab8efd16ad6ee5acc3642a8ea') +class MicroblazeLittleEndianMachine(MicroblazeMachine): def test_microblaze_s3adsp1800_legacy_le(self): self.do_xmaton_le_test('petalogix-s3adsp1800') -- 2.47.1
[PATCH v7 01/10] hw/qdev-properties-system: Introduce EndianMode QAPI enum
Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. Endianness can be BIG, LITTLE or unspecified (default). Reviewed-by: Thomas Huth Signed-off-by: Philippe Mathieu-Daudé --- qapi/common.json| 16 include/hw/qdev-properties-system.h | 7 +++ hw/core/qdev-properties-system.c| 11 +++ 3 files changed, 34 insertions(+) diff --git a/qapi/common.json b/qapi/common.json index 6ffc7a37890..33d8df19f67 100644 --- a/qapi/common.json +++ b/qapi/common.json @@ -212,3 +212,19 @@ ## { 'struct': 'HumanReadableText', 'data': { 'human-readable-text': 'str' } } + +## +# @EndianMode: +# +# An enumeration of three options: little, big, and unspecified +# +# @unspecified: Endianness not specified +# +# @little: Little endianness +# +# @big: Big endianness +# +# Since: 10.0 +## +{ 'enum': 'EndianMode', + 'data': [ 'unspecified', 'little', 'big' ] } diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h index 7ec37f6316c..ead4dfc2f02 100644 --- a/include/hw/qdev-properties-system.h +++ b/include/hw/qdev-properties-system.h @@ -30,6 +30,7 @@ extern const PropertyInfo qdev_prop_pcie_link_speed; extern const PropertyInfo qdev_prop_pcie_link_width; extern const PropertyInfo qdev_prop_cpus390entitlement; extern const PropertyInfo qdev_prop_iothread_vq_mapping_list; +extern const PropertyInfo qdev_prop_endian_mode; #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d) \ DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t) @@ -97,4 +98,10 @@ extern const PropertyInfo qdev_prop_iothread_vq_mapping_list; DEFINE_PROP(_name, _state, _field, qdev_prop_iothread_vq_mapping_list, \ IOThreadVirtQueueMappingList *) +#define DEFINE_PROP_ENDIAN(_name, _state, _field, _default) \ +DEFINE_PROP_UNSIGNED(_name, _state, _field, _default, \ + qdev_prop_endian_mode, EndianMode) +#define DEFINE_PROP_ENDIAN_NODEFAULT(_name, _state, _field) \ +DEFINE_PROP_ENDIAN(_name, _state, _field, ENDIAN_MODE_UNSPECIFIED) + #endif diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index a96675beb0d..89f954f569e 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -1283,3 +1283,14 @@ const PropertyInfo qdev_prop_iothread_vq_mapping_list = { .set = set_iothread_vq_mapping_list, .release = release_iothread_vq_mapping_list, }; + +/* --- Endian modes */ + +const PropertyInfo qdev_prop_endian_mode = { +.name = "EndianMode", +.description = "Endian mode, big/little/unspecified", +.enum_table = &EndianMode_lookup, +.get = qdev_propinfo_get_enum, +.set = qdev_propinfo_set_enum, +.set_default_value = qdev_propinfo_set_default_value_enum, +}; -- 2.47.1
[PATCH v7 00/10] hw/microblaze: Allow running cross-endian vCPUs
Since v6: - Simplify MemoryRegionOps indexing (Thomas) Since v5: - Introduce QAPI EndianMode - Update RISCV machine while rebasing - Fixed INTC use on PPC (Thomas) - Dropped patch adding more machines (Daniel) Since v4 & v3: - Addressed Thomas review comments Since v2: - Addressed Richard's review comments Since v1: - Make device endianness configurable (Edgar) - Convert more Xilinx devices - Avoid preprocessor #if (Richard) - Add R-b tags Philippe Mathieu-Daudé (10): hw/qdev-properties-system: Introduce EndianMode QAPI enum hw/intc/xilinx_intc: Make device endianness configurable hw/net/xilinx_ethlite: Make device endianness configurable hw/timer/xilinx_timer: Make device endianness configurable hw/char/xilinx_uartlite: Make device endianness configurable hw/ssi/xilinx_spi: Make device endianness configurable tests/functional: Explicit endianness of microblaze assets tests/functional: Allow microblaze tests to take a machine name argument tests/functional: Remove sleep() kludges from microblaze tests tests/functional: Have microblaze tests inherit common parent class qapi/common.json | 16 + include/hw/qdev-properties-system.h | 7 +++ hw/char/xilinx_uartlite.c | 34 +++ hw/core/qdev-properties-system.c | 11 hw/intc/xilinx_intc.c | 59 ++- hw/microblaze/petalogix_ml605_mmu.c | 3 + hw/microblaze/petalogix_s3adsp1800_mmu.c | 6 ++ hw/net/xilinx_ethlite.c | 29 +++-- hw/ppc/virtex_ml507.c | 2 + hw/riscv/microblaze-v-generic.c | 5 ++ hw/ssi/xilinx_spi.c | 32 +++--- hw/timer/xilinx_timer.c | 43 ++ .../functional/test_microblaze_s3adsp1800.py | 34 +-- .../test_microblazeel_s3adsp1800.py | 32 ++ 14 files changed, 229 insertions(+), 84 deletions(-) -- 2.47.1
[PATCH v7 04/10] hw/timer/xilinx_timer: Make device endianness configurable
Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness for each machine using the device. Signed-off-by: Philippe Mathieu-Daudé --- hw/microblaze/petalogix_ml605_mmu.c | 1 + hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 + hw/ppc/virtex_ml507.c| 1 + hw/riscv/microblaze-v-generic.c | 2 ++ hw/timer/xilinx_timer.c | 43 +--- 5 files changed, 35 insertions(+), 13 deletions(-) diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c index 55398cc67d1..490640e9428 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -127,6 +127,7 @@ petalogix_ml605_init(MachineState *machine) /* 2 timers at irq 2 @ 100 Mhz. */ dev = qdev_new("xlnx.xps-timer"); +qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 100 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c index d419dc49a25..caaea222a8c 100644 --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c @@ -116,6 +116,7 @@ petalogix_s3adsp1800_init(MachineState *machine) /* 2 timers at irq 2 @ 62 Mhz. */ dev = qdev_new("xlnx.xps-timer"); +qdev_prop_set_enum(dev, "endianness", endianness); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 62 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c index df8f9644829..a01354d991d 100644 --- a/hw/ppc/virtex_ml507.c +++ b/hw/ppc/virtex_ml507.c @@ -231,6 +231,7 @@ static void virtex_init(MachineState *machine) /* 2 timers at irq 2 @ 62 Mhz. */ dev = qdev_new("xlnx.xps-timer"); +qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_BIG); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 62 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/riscv/microblaze-v-generic.c b/hw/riscv/microblaze-v-generic.c index a21fdfbe6db..3c79f5733b2 100644 --- a/hw/riscv/microblaze-v-generic.c +++ b/hw/riscv/microblaze-v-generic.c @@ -104,6 +104,7 @@ static void mb_v_generic_init(MachineState *machine) /* 2 timers at irq 0 @ 100 Mhz. */ dev = qdev_new("xlnx.xps-timer"); +qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 1); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); @@ -112,6 +113,7 @@ static void mb_v_generic_init(MachineState *machine) /* 2 timers at irq 3 @ 100 Mhz. */ dev = qdev_new("xlnx.xps-timer"); +qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 1); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c index 6595cf5f517..4620528f985 100644 --- a/hw/timer/xilinx_timer.c +++ b/hw/timer/xilinx_timer.c @@ -3,6 +3,9 @@ * * Copyright (c) 2009 Edgar E. Iglesias. * + * DS573: https://docs.amd.com/v/u/en-US/xps_timer + * LogiCORE IP XPS Timer/Counter (v1.02a) + * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal * in the Software without restriction, including without limitation the rights @@ -23,10 +26,12 @@ */ #include "qemu/osdep.h" +#include "qapi/error.h" #include "hw/sysbus.h" #include "hw/irq.h" #include "hw/ptimer.h" #include "hw/qdev-properties.h" +#include "hw/qdev-properties-system.h" #include "qemu/log.h" #include "qemu/module.h" #include "qom/object.h" @@ -69,6 +74,7 @@ struct XpsTimerState { SysBusDevice parent_obj; +EndianMode model_endianness; MemoryRegion mmio; qemu_irq irq; uint8_t one_timer_only; @@ -189,18 +195,21 @@ timer_write(void *opaque, hwaddr addr, timer_update_irq(t); } -static const MemoryRegionOps timer_ops = { -.read = timer_read, -.write = timer_write, -.endianness = DEVICE_NATIVE_ENDIAN, -.impl = { -.min_access_size = 4, -.max_access_size = 4, +static const MemoryRegionOps timer_ops[2] = { +[0 ... 1] = { +.read = timer_read, +.write = timer_write, +.impl = { +.min_access_size = 4, +.max_access_size = 4, +}, +.valid = { +
[PATCH v7 06/10] hw/ssi/xilinx_spi: Make device endianness configurable
Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness on the single machine using the device. Signed-off-by: Philippe Mathieu-Daudé --- hw/microblaze/petalogix_ml605_mmu.c | 1 + hw/ssi/xilinx_spi.c | 32 + 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c index 490640e9428..b34edf13796 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -175,6 +175,7 @@ petalogix_ml605_init(MachineState *machine) SSIBus *spi; dev = qdev_new("xlnx.xps-spi"); +qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE); qdev_prop_set_uint8(dev, "num-ss-bits", NUM_SPI_FLASHES); busdev = SYS_BUS_DEVICE(dev); sysbus_realize_and_unref(busdev, &error_fatal); diff --git a/hw/ssi/xilinx_spi.c b/hw/ssi/xilinx_spi.c index fd1ff12eb1d..be5baa6b350 100644 --- a/hw/ssi/xilinx_spi.c +++ b/hw/ssi/xilinx_spi.c @@ -25,6 +25,7 @@ */ #include "qemu/osdep.h" +#include "qapi/error.h" #include "hw/sysbus.h" #include "migration/vmstate.h" #include "qemu/module.h" @@ -32,6 +33,7 @@ #include "hw/irq.h" #include "hw/qdev-properties.h" +#include "hw/qdev-properties-system.h" #include "hw/ssi/ssi.h" #include "qom/object.h" @@ -83,6 +85,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(XilinxSPI, XILINX_SPI) struct XilinxSPI { SysBusDevice parent_obj; +EndianMode model_endianness; MemoryRegion mmio; qemu_irq irq; @@ -313,14 +316,17 @@ done: xlx_spi_update_irq(s); } -static const MemoryRegionOps spi_ops = { -.read = spi_read, -.write = spi_write, -.endianness = DEVICE_NATIVE_ENDIAN, -.valid = { -.min_access_size = 4, -.max_access_size = 4 -} +static const MemoryRegionOps spi_ops[2] = { +[0 ... 1] = { +.read = spi_read, +.write = spi_write, +.valid = { +.min_access_size = 4, +.max_access_size = 4, +}, +}, +[0].endianness = DEVICE_LITTLE_ENDIAN, +[1].endianness = DEVICE_BIG_ENDIAN, }; static void xilinx_spi_realize(DeviceState *dev, Error **errp) @@ -329,6 +335,12 @@ static void xilinx_spi_realize(DeviceState *dev, Error **errp) XilinxSPI *s = XILINX_SPI(dev); int i; +if (s->model_endianness == ENDIAN_MODE_UNSPECIFIED) { +error_setg(errp, TYPE_XILINX_SPI " property 'endianness'" + " must be set to 'big' or 'little'"); +return; +} + DB_PRINT("\n"); s->spi = ssi_create_bus(dev, "spi"); @@ -339,7 +351,8 @@ static void xilinx_spi_realize(DeviceState *dev, Error **errp) sysbus_init_irq(sbd, &s->cs_lines[i]); } -memory_region_init_io(&s->mmio, OBJECT(s), &spi_ops, s, +memory_region_init_io(&s->mmio, OBJECT(s), + &spi_ops[s->model_endianness == ENDIAN_MODE_BIG], s, "xilinx-spi", R_MAX * 4); sysbus_init_mmio(sbd, &s->mmio); @@ -362,6 +375,7 @@ static const VMStateDescription vmstate_xilinx_spi = { }; static const Property xilinx_spi_properties[] = { +DEFINE_PROP_ENDIAN_NODEFAULT("endianness", XilinxSPI, model_endianness), DEFINE_PROP_UINT8("num-ss-bits", XilinxSPI, num_cs, 1), }; -- 2.47.1
[PATCH v7 08/10] tests/functional: Allow microblaze tests to take a machine name argument
Make microblaze tests a bit more generic. Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Thomas Huth Message-Id: <20250206131052.30207-14-phi...@linaro.org> --- tests/functional/test_microblaze_s3adsp1800.py | 7 +-- tests/functional/test_microblazeel_s3adsp1800.py | 7 +-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/functional/test_microblaze_s3adsp1800.py b/tests/functional/test_microblaze_s3adsp1800.py index fac364b1ea9..c4226f49cf3 100755 --- a/tests/functional/test_microblaze_s3adsp1800.py +++ b/tests/functional/test_microblaze_s3adsp1800.py @@ -20,8 +20,8 @@ class MicroblazeMachine(QemuSystemTest): 'day17.tar.xz'), '3ba7439dfbea7af4876662c97f8e1f0cdad9231fc166e4861d17042489270057') -def test_microblaze_s3adsp1800_be(self): -self.set_machine('petalogix-s3adsp1800') +def do_ballerina_be_test(self, machine): +self.set_machine(machine) self.archive_extract(self.ASSET_IMAGE_BE) self.vm.set_console() self.vm.add_args('-kernel', @@ -34,5 +34,8 @@ def test_microblaze_s3adsp1800_be(self): # message, that's why we don't test for a later string here. This # needs some investigation by a microblaze wizard one day... +def test_microblaze_s3adsp1800_legacy_be(self): +self.do_ballerina_be_test('petalogix-s3adsp1800') + if __name__ == '__main__': QemuSystemTest.main() diff --git a/tests/functional/test_microblazeel_s3adsp1800.py b/tests/functional/test_microblazeel_s3adsp1800.py index 5d353dba5d2..715ef3f79ac 100755 --- a/tests/functional/test_microblazeel_s3adsp1800.py +++ b/tests/functional/test_microblazeel_s3adsp1800.py @@ -21,9 +21,9 @@ class MicroblazeelMachine(QemuSystemTest): ('http://www.qemu-advent-calendar.org/2023/download/day13.tar.gz'), 'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22') -def test_microblazeel_s3adsp1800_le(self): +def do_xmaton_le_test(self, machine): self.require_netdev('user') -self.set_machine('petalogix-s3adsp1800') +self.set_machine(machine) self.archive_extract(self.ASSET_IMAGE_LE) self.vm.set_console() self.vm.add_args('-kernel', self.scratch_file('day13', 'xmaton.bin')) @@ -38,5 +38,8 @@ def test_microblazeel_s3adsp1800_le(self): 'tftp -g -r xmaton.png 10.0.2.2 ; md5sum xmaton.png', '821cd3cab8efd16ad6ee5acc3642a8ea') +def test_microblaze_s3adsp1800_legacy_le(self): +self.do_xmaton_le_test('petalogix-s3adsp1800') + if __name__ == '__main__': QemuSystemTest.main() -- 2.47.1
Re: [PATCH v7 02/10] hw/intc/xilinx_intc: Make device endianness configurable
On 12/2/25 13:36, Philippe Mathieu-Daudé wrote: Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Grr, please read: Add the "endianness" property to select the device endianness. This property is unspecified by default, and machines need to make it explicit. (same in following patches). Set the proper endianness for each machine using the device. Signed-off-by: Philippe Mathieu-Daudé --- hw/intc/xilinx_intc.c| 59 ++-- hw/microblaze/petalogix_ml605_mmu.c | 1 + hw/microblaze/petalogix_s3adsp1800_mmu.c | 3 ++ hw/ppc/virtex_ml507.c| 1 + hw/riscv/microblaze-v-generic.c | 1 + 5 files changed, 51 insertions(+), 14 deletions(-) diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c index 6930f83907a..ab1c4a32221 100644 --- a/hw/intc/xilinx_intc.c +++ b/hw/intc/xilinx_intc.c @@ -3,6 +3,9 @@ * * Copyright (c) 2009 Edgar E. Iglesias. * + * https://docs.amd.com/v/u/en-US/xps_intc + * DS572: LogiCORE IP XPS Interrupt Controller (v2.01a) + * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal * in the Software without restriction, including without limitation the rights @@ -23,10 +26,12 @@ */ #include "qemu/osdep.h" +#include "qapi/error.h" #include "hw/sysbus.h" #include "qemu/module.h" #include "hw/irq.h" #include "hw/qdev-properties.h" +#include "hw/qdev-properties-system.h" #include "qom/object.h" #define D(x) @@ -49,6 +54,7 @@ struct XpsIntc { SysBusDevice parent_obj; +EndianMode model_endianness; MemoryRegion mmio; qemu_irq parent_irq; @@ -140,18 +146,28 @@ static void pic_write(void *opaque, hwaddr addr, update_irq(p); } -static const MemoryRegionOps pic_ops = { -.read = pic_read, -.write = pic_write, -.endianness = DEVICE_NATIVE_ENDIAN, -.impl = { -.min_access_size = 4, -.max_access_size = 4, +static const MemoryRegionOps pic_ops[2] = { +[0 ... 1] = { +.read = pic_read, +.write = pic_write, +.impl = { +.min_access_size = 4, +.max_access_size = 4, +}, +.valid = { +/* + * All XPS INTC registers are accessed through the PLB interface. + * The base address for these registers is provided by the + * configuration parameter, C_BASEADDR. Each register is 32 bits + * although some bits may be unused and is accessed on a 4-byte + * boundary offset from the base address. + */ +.min_access_size = 4, +.max_access_size = 4, +}, }, -.valid = { -.min_access_size = 4, -.max_access_size = 4 -} +[0].endianness = DEVICE_LITTLE_ENDIAN, +[1].endianness = DEVICE_BIG_ENDIAN, }; static void irq_handler(void *opaque, int irq, int level) @@ -174,13 +190,27 @@ static void xilinx_intc_init(Object *obj) qdev_init_gpio_in(DEVICE(obj), irq_handler, 32); sysbus_init_irq(SYS_BUS_DEVICE(obj), &p->parent_irq); - -memory_region_init_io(&p->mmio, obj, &pic_ops, p, "xlnx.xps-intc", - R_MAX * 4); sysbus_init_mmio(SYS_BUS_DEVICE(obj), &p->mmio); } +static void xilinx_intc_realize(DeviceState *dev, Error **errp) +{ +XpsIntc *p = XILINX_INTC(dev); + +if (p->model_endianness == ENDIAN_MODE_UNSPECIFIED) { +error_setg(errp, TYPE_XILINX_INTC " property 'endianness'" + " must be set to 'big' or 'little'"); +return; +} + +memory_region_init_io(&p->mmio, OBJECT(dev), + &pic_ops[p->model_endianness == ENDIAN_MODE_BIG], + p, "xlnx.xps-intc", + R_MAX * 4); +} + static const Property xilinx_intc_properties[] = { +DEFINE_PROP_ENDIAN_NODEFAULT("endianness", XpsIntc, model_endianness), DEFINE_PROP_UINT32("kind-of-intr", XpsIntc, c_kind_of_intr, 0), }; @@ -188,6 +218,7 @@ static void xilinx_intc_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); +dc->realize = xilinx_intc_realize; device_class_set_props(dc, xilinx_intc_properties); } diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c index 8b44be75a22..55398cc67d1 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -111,6 +111,7 @@ petalogix_ml605_init(MachineState *machine) dev = qdev_new("xlnx.xps-intc"); +qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE); qdev_prop_set_uint32(dev, "kind-of-intr", 1 << TIMER_IRQ); sysbus_realize_and_unr
Re: [PATCH 04/11] rust/qemu-api: Add wrappers to run futures in QEMU
Am 12.02.2025 um 10:28 hat Paolo Bonzini geschrieben: > On 2/11/25 22:43, Kevin Wolf wrote: > > +/// Use QEMU's event loops to run a Rust [`Future`] to completion and > > return its result. > > +/// > > +/// This function must be called in coroutine context. If the future isn't > > ready yet, it yields. > > +pub fn qemu_co_run_future(future: F) -> F::Output { > > +let waker = Arc::new(RunFutureWaker { > > +co: unsafe { bindings::qemu_coroutine_self() }, > > +}) > > +.into(); > > into what? :) Maybe you can add the type to the "let" for clarity. Into Waker. Do we have any idea yet how explicit we want to be with type annotations that aren't strictly necessary? I didn't think of making it explicit here because the only thing that is done with it is building a Context from it, so it seemed obvious enough. If you think that being explicit is better, then Waker::from() might be better than adding a type name to let and keeping .into(). > > +let mut cx = Context::from_waker(&waker); > > + > > +let mut pinned_future = std::pin::pin!(future); > > +loop { > > +match pinned_future.as_mut().poll(&mut cx) { > > +Poll::Ready(res) => return res, > > Alternatively, "break res" (matter of taste). > > > +Poll::Pending => unsafe { > > +bindings::qemu_coroutine_yield(); > > +}, > > +} > > +} > > +} > > +/// Wrapper around [`qemu_co_run_future`] that can be called from C. > > +/// > > +/// # Safety > > +/// > > +/// `future` must be a valid pointer to an owned `F` (it will be freed in > > this function). `output` > > +/// must be a valid pointer representing a mutable reference to an > > `F::Output` where the result can > > +/// be stored. > > +unsafe extern "C" fn rust_co_run_future( > > +future: *mut bindings::RustBoxedFuture, > > +output: *mut c_void, > > +) { > > +let future = unsafe { Box::from_raw(future.cast::()) }; > > +let output = output.cast::(); > > +let ret = qemu_co_run_future(*future); > > +unsafe { > > +*output = ret; > > This should use output.write(ret), to ensure that the output is written > without dropping the previous value. Oops, thanks. I need to learn that unsafe Rust still isn't C. :-) > Also, would qemu_co_run_future() and qemu_run_future() become methods on an > Executor later? Maybe it make sense to have already something like > > pub trait QemuExecutor { > fn run_until(future: F) -> F::Output; > } > > pub struct Executor; > pub struct CoExecutor; > > and pass an executor to Rust functions (&Executor for no_coroutine_fn, > &CoExecutor for coroutine_fn, &dyn QemuExecutor for mixed). Or would that > be premature in your opinion? If we could get bindgen to actually do that for the C interface, then this could be interesting, though for most functions it also would remain unused boilerplate. If we have to get the executor manually on the Rust side for each function, that's probably the same function that will want to execute the future - in which case it just can directly call the correct function. The long term idea should be anyway that C coroutines disappear from the path and we integrate an executor into the QEMU main loop. But as long as the block layer core is written in C and uses coroutines and we want Rust futures to be able to call into coroutine_fns, that's still a long way to go. Kevin
[PATCH v7 05/10] hw/char/xilinx_uartlite: Make device endianness configurable
Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness on the single machine using the device. Signed-off-by: Philippe Mathieu-Daudé --- hw/char/xilinx_uartlite.c| 34 hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 + hw/riscv/microblaze-v-generic.c | 1 + 3 files changed, 25 insertions(+), 11 deletions(-) diff --git a/hw/char/xilinx_uartlite.c b/hw/char/xilinx_uartlite.c index 56955e0d74a..4037c937eeb 100644 --- a/hw/char/xilinx_uartlite.c +++ b/hw/char/xilinx_uartlite.c @@ -24,6 +24,7 @@ #include "qemu/osdep.h" #include "qemu/log.h" +#include "qapi/error.h" #include "hw/char/xilinx_uartlite.h" #include "hw/irq.h" #include "hw/qdev-properties.h" @@ -57,6 +58,7 @@ struct XilinxUARTLite { SysBusDevice parent_obj; +EndianMode model_endianness; MemoryRegion mmio; CharBackend chr; qemu_irq irq; @@ -166,17 +168,21 @@ uart_write(void *opaque, hwaddr addr, uart_update_irq(s); } -static const MemoryRegionOps uart_ops = { -.read = uart_read, -.write = uart_write, -.endianness = DEVICE_NATIVE_ENDIAN, -.valid = { -.min_access_size = 1, -.max_access_size = 4 -} +static const MemoryRegionOps uart_ops[2] = { +[0 ... 1] = { +.read = uart_read, +.write = uart_write, +.valid = { +.min_access_size = 1, +.max_access_size = 4, +}, +}, +[0].endianness = DEVICE_LITTLE_ENDIAN, +[1].endianness = DEVICE_BIG_ENDIAN, }; static const Property xilinx_uartlite_properties[] = { +DEFINE_PROP_ENDIAN_NODEFAULT("endianness", XilinxUARTLite, model_endianness), DEFINE_PROP_CHR("chardev", XilinxUARTLite, chr), }; @@ -214,6 +220,15 @@ static void xilinx_uartlite_realize(DeviceState *dev, Error **errp) { XilinxUARTLite *s = XILINX_UARTLITE(dev); +if (s->model_endianness == ENDIAN_MODE_UNSPECIFIED) { +error_setg(errp, TYPE_XILINX_UARTLITE " property 'endianness'" + " must be set to 'big' or 'little'"); +return; +} + +memory_region_init_io(&s->mmio, OBJECT(dev), + &uart_ops[s->model_endianness == ENDIAN_MODE_BIG], + s, "xlnx.xps-uartlite", R_MAX * 4); qemu_chr_fe_set_handlers(&s->chr, uart_can_rx, uart_rx, uart_event, NULL, s, NULL, true); } @@ -223,9 +238,6 @@ static void xilinx_uartlite_init(Object *obj) XilinxUARTLite *s = XILINX_UARTLITE(obj); sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq); - -memory_region_init_io(&s->mmio, obj, &uart_ops, s, - "xlnx.xps-uartlite", R_MAX * 4); sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio); } diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c index caaea222a8c..bdba2006b72 100644 --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c @@ -109,6 +109,7 @@ petalogix_s3adsp1800_init(MachineState *machine) } dev = qdev_new(TYPE_XILINX_UARTLITE); +qdev_prop_set_enum(dev, "endianness", endianness); qdev_prop_set_chr(dev, "chardev", serial_hd(0)); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, UARTLITE_BASEADDR); diff --git a/hw/riscv/microblaze-v-generic.c b/hw/riscv/microblaze-v-generic.c index 3c79f5733b2..d8e67906d26 100644 --- a/hw/riscv/microblaze-v-generic.c +++ b/hw/riscv/microblaze-v-generic.c @@ -92,6 +92,7 @@ static void mb_v_generic_init(MachineState *machine) /* Uartlite */ dev = qdev_new(TYPE_XILINX_UARTLITE); +qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE); qdev_prop_set_chr(dev, "chardev", serial_hd(0)); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, UARTLITE_BASEADDR); -- 2.47.1
Re: [PATCH v7 02/10] hw/intc/xilinx_intc: Make device endianness configurable
On 12/02/2025 13.36, Philippe Mathieu-Daudé wrote: Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness for each machine using the device. Signed-off-by: Philippe Mathieu-Daudé --- ... diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c index 8b44be75a22..55398cc67d1 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -111,6 +111,7 @@ petalogix_ml605_init(MachineState *machine) dev = qdev_new("xlnx.xps-intc"); +qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE); Do we still need a TARGET_BIG_ENDIAN ?: check here, too? ... the petalogix_ml605_machine_init() code still contains it, though big endian is marked as deprecated and untested ... Anyway, assuming that nobody uses this in big endian anymore: Reviewed-by: Thomas Huth
Re: [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust
Am 12.02.2025 um 10:41 hat Daniel P. Berrangé geschrieben: > On Wed, Feb 12, 2025 at 09:14:57AM +, Daniel P. Berrangé wrote: > > On Tue, Feb 11, 2025 at 10:43:27PM +0100, Kevin Wolf wrote: > > > This adds a separate block driver for the bochs image format called > > > 'bochs-rs' so that for the moment both the C implementation and the Rust > > > implementation can be present in the same build. The intention is to > > > remove the C implementation eventually and rename this one into 'bochs'. > > > This can only happen once Rust can be a hard build dependency for QEMU. > > > > > > Signed-off-by: Kevin Wolf > > > --- > > > rust/block/Cargo.toml| 2 +- > > > rust/block/src/bochs.rs | 296 +++ > > > rust/block/src/driver.rs | 5 - > > > rust/block/src/lib.rs| 1 + > > > 4 files changed, 298 insertions(+), 6 deletions(-) > > > create mode 100644 rust/block/src/bochs.rs > > > > > > diff --git a/rust/block/Cargo.toml b/rust/block/Cargo.toml > > > index 70ee02f429..1c06f3a00c 100644 > > > --- a/rust/block/Cargo.toml > > > +++ b/rust/block/Cargo.toml > > > @@ -3,7 +3,7 @@ name = "block" > > > version = "0.1.0" > > > edition = "2021" > > > authors = ["Kevin Wolf "] > > > -license = "GPL-2.0-or-later" > > > +license = "GPL-2.0-or-later AND MIT" > > > readme = "README.md" > > > description = "Block backends for QEMU" > > > repository = "https://gitlab.com/qemu-project/qemu/"; > > > diff --git a/rust/block/src/bochs.rs b/rust/block/src/bochs.rs > > > new file mode 100644 > > > index 00..388ac5ef03 > > > --- /dev/null > > > +++ b/rust/block/src/bochs.rs > > > @@ -0,0 +1,296 @@ > > > +// SPDX-License-Identifier: MIT > > > > Why MIT instead of our normal GPL-2.0-or-later. > > > > Using Rust conversion to eliminate GPL usage for permissive licenses > > like MIT is not something I'd like to see us doing. > > My bad. I should have noticed that the original bochs.c was also MIT, > so I presume you're considering this Rust impl to be a derived work. Yes, this is essentially a translation of the existing C bochs driver, which is also why I kept the original copyright notice and the full license text. Kevin
Re: [PATCH 2/3] hw/cxl/cxl-mailbox-utils.c: Added support for Clear Log (Opcode 0403h)
On 04/02/25 10:53AM, Jonathan Cameron wrote: On Mon, 3 Feb 2025 11:29:49 +0530 Arpit Kumar wrote: Add some description of what is being added here. Key issue in here is that clearing the CEL doesn't make sense. It is a description of what the device supports, there is no state to clear in it. To add this command you need to pick a different log. Jonathan Thanks for the review Jonathan, will modify the code accordingly in V2 patch. Signed-off-by: Arpit Kumar Reviewed-by: Alok Rathore Reviewed-by: Krishna Kanth Reddy --- hw/cxl/cxl-mailbox-utils.c | 36 1 file changed, 36 insertions(+) diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c index 3d66a425a9..5fd7f850c4 100644 --- a/hw/cxl/cxl-mailbox-utils.c +++ b/hw/cxl/cxl-mailbox-utils.c @@ -77,6 +77,7 @@ enum { #define GET_SUPPORTED 0x0 #define GET_LOG 0x1 #define GET_LOG_CAPABILITIES 0x2 +#define CLEAR_LOG 0x3 FEATURES= 0x05, #define GET_SUPPORTED 0x0 #define GET_FEATURE 0x1 @@ -1115,6 +1116,39 @@ static CXLRetCode cmd_logs_get_log_capabilities(const struct cxl_cmd *cmd, return CXL_MBOX_SUCCESS; } +/* CXL r3.1 Section 8.2.9.5.4: Clear Log (Opcode 0403h) */ +static CXLRetCode cmd_logs_clear_log(const struct cxl_cmd *cmd, + uint8_t *payload_in, + size_t len_in, + uint8_t *payload_out, + size_t *len_out, + CXLCCI *cci) +{ +int32_t cap_id; +struct { +QemuUUID uuid; +} QEMU_PACKED QEMU_ALIGNED(8) * clear_log = (void *)payload_in; + +cap_id = valid_log_check(&clear_log->uuid, cci); +if (cap_id == -1) { +return CXL_MBOX_INVALID_LOG; +} Follow on from previous patch, if this returns the cap pointer, the following code wont have to index the array and should end up simpler. okay + +if (cci->supported_log_cap[cap_id].param_flags.clear_log_supported) { I would flip this. if (!(cap->param_flags & PARAM_FLAG_CLEAR_LOG_SUPPORTED)) { return CXL_MBOX_UNSUPPORTED; } +switch (cap_id) { +case CEL: So if we return the cap as suggested, it will have to reference what it is or provide a callback (which might be cleaner as this grows). However, what does clearly the command effects log mean? This makes no sense. So if you want to implement clear_log you need to implement a different log to clear. okay +memset(cci->cel_log, 0, (1 << 16) * sizeof(struct cel_log)); +cci->cel_size = 0; +break; +default: +return CXL_MBOX_UNSUPPORTED; +} +} else { +return CXL_MBOX_UNSUPPORTED; +} +return CXL_MBOX_SUCCESS; +} + /* CXL r3.1 section 8.2.9.6: Features */ /* * Get Supported Features output payload @@ -2882,6 +2916,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = { [LOGS][GET_LOG] = { "LOGS_GET_LOG", cmd_logs_get_log, 0x18, 0 }, [LOGS][GET_LOG_CAPABILITIES] = { "LOGS_GET_LOG_CAPABILITIES", cmd_logs_get_log_capabilities, 0x10, 0 }, +[LOGS][CLEAR_LOG] = { "LOGS_CLEAR_LOG", cmd_logs_clear_log, 0x10, + CXL_MBOX_IMMEDIATE_LOG_CHANGE}, [FEATURES][GET_SUPPORTED] = { "FEATURES_GET_SUPPORTED", cmd_features_get_supported, 0x8, 0 }, [FEATURES][GET_FEATURE] = { "FEATURES_GET_FEATURE",
Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum
On 12/2/25 13:56, BALATON Zoltan wrote: On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote: On 12/2/25 12:37, Thomas Huth wrote: On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. Endianness can be BIG, LITTLE or unspecified (default). Signed-off-by: Philippe Mathieu-Daudé --- qapi/common.json | 16 include/hw/qdev-properties-system.h | 7 +++ hw/core/qdev-properties-system.c | 11 +++ 3 files changed, 34 insertions(+) diff --git a/qapi/common.json b/qapi/common.json index 6ffc7a37890..217feaaf683 100644 --- a/qapi/common.json +++ b/qapi/common.json @@ -212,3 +212,19 @@ ## { 'struct': 'HumanReadableText', 'data': { 'human-readable-text': 'str' } } + +## +# @EndianMode: +# +# An enumeration of three options: little, big, and unspecified +# +# @little: Little endianness +# +# @big: Big endianness +# +# @unspecified: Endianness not specified +# +# Since: 10.0 +## +{ 'enum': 'EndianMode', + 'data': [ 'little', 'big', 'unspecified' ] } Should 'unspecified' come first? ... so that it gets the value 0, i.e. when someone forgets to properly initialize a related variable, the chances are higher that it ends up as "unspecified" than as "little" ? Hmm but then in this series the dual-endianness regions are defined as: +static const MemoryRegionOps pic_ops[2] = { + [0 ... 1] = { This is already confusing as you'd have to know that 0 and 1 here means ENDIAN_MODE_LITTLE and ENDIAN_MODE_BIG (using those constants here as well might be clearer). It's easy to miss what this does so maybe repeating the definitions for each case would be longer but less confusing and then it does not matter what the values are. Or what uses the ops.endianness now should look at the property introduced by this patch to avoid having to propagate it like below and drop the ops.endianness? Or it should move to the memory region and set when the ops are assigned? I'm not understanding well what you ask, but maybe the answer is in v7 :)
Re: [PATCH v3 09/23] hw/uefi: add var-service-core.c
On 12.02.25 13:28, Gerd Hoffmann wrote: On Wed, Feb 12, 2025 at 12:30:20PM +0100, Alexander Graf wrote: On 12.02.25 11:24, Gerd Hoffmann wrote: Why do you use confidential computing in the first place if you trust the host with your EFI variables? I'd rather see something simliar running under guest control, in svsm context. That depends heavily on your threat model. You can use a host provided variable store to gain variable persistence for things like boot variables and then have an ephemeral SVSM based TPM that you use to measure the loaded payloads. A malicious host can already replace your root volume, so extending the threat to variables is not the end of the world. If you go depend on measured boot instead of secure boot then yes, this might be a workable model. Should be doable with a small svsm driver which forwards requests to the host via svsm-controlled bounce buffer (where the svsm has control over page properties). In a BYOF world it's even useful without SVSM at all, because the launch digest performs measurement for you, but you still want to find your boot variables. The firmware knows all this very well though. The OS passes a mapping table to the firmware, efi runtime drivers can subscribe to mapping updates and can use RT->ConvertPointer to translate addresses from physical to virtual. The edk2 code (https://github.com/tianocore/edk2/pull/10695) does exactly that. I see your driver does that too, so in theory it should work just fine. I'm wondering what exactly the problem with macOS is? You get to know the new virtual address, but ConvertPointer never tells you what the new *physical* address is. That means you have no idea where to DMA from once you're in virtual land. Yes. Knowing both physical and virtual address works only for memory you allocated yourself before ExitBootServices. So you can't pass on pointers from the OS, you have to copy the data to a buffer where you know the physical address instead. Yes, some overhead. Should still be much faster than going to pio transfer mode ... MacOS takes over the full physical address map past ExitBootServices: Your code no longer has VA access to random code and it literally memcpy()'s all preserved (virtual available) code and data to different physical addresses. You simply have nothing that is all of 1) RAM (mapped as cacheable on ARM), 2) known VA 3) known PA. So we really really need a fallback mechanism that works without DMA :). Alex
Re: [PATCH v6 02/11] hw/intc/xilinx_intc: Make device endianness configurable
On 12/2/25 12:42, Thomas Huth wrote: On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness for each machine using the device. Signed-off-by: Philippe Mathieu-Daudé --- hw/intc/xilinx_intc.c | 60 ++-- hw/microblaze/petalogix_ml605_mmu.c | 1 + hw/microblaze/petalogix_s3adsp1800_mmu.c | 3 ++ hw/ppc/virtex_ml507.c | 1 + hw/riscv/microblaze-v-generic.c | 1 + 5 files changed, 52 insertions(+), 14 deletions(-) diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c index 6930f83907a..523402b688c 100644 --- a/hw/intc/xilinx_intc.c +++ b/hw/intc/xilinx_intc.c @@ -3,6 +3,9 @@ * * Copyright (c) 2009 Edgar E. Iglesias. * + * https://docs.amd.com/v/u/en-US/xps_intc + * DS572: LogiCORE IP XPS Interrupt Controller (v2.01a) + * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal * in the Software without restriction, including without limitation the rights @@ -23,10 +26,12 @@ */ #include "qemu/osdep.h" +#include "qapi/error.h" #include "hw/sysbus.h" #include "qemu/module.h" #include "hw/irq.h" #include "hw/qdev-properties.h" +#include "hw/qdev-properties-system.h" #include "qom/object.h" #define D(x) @@ -49,6 +54,7 @@ struct XpsIntc { SysBusDevice parent_obj; + EndianMode model_endianness; MemoryRegion mmio; qemu_irq parent_irq; @@ -140,18 +146,29 @@ static void pic_write(void *opaque, hwaddr addr, update_irq(p); } -static const MemoryRegionOps pic_ops = { - .read = pic_read, - .write = pic_write, - .endianness = DEVICE_NATIVE_ENDIAN, - .impl = { - .min_access_size = 4, - .max_access_size = 4, +static const MemoryRegionOps pic_ops[2] = { + [0 ... 1] = { Hmm, ok, so here we have now an assumption that ENDIAN_MODE_BIG and ENDIAN_MODE_LITTLE match 0 and 1, which would not work anymore when using 0 as unspecified... a little bit ugly ... so maybe instead of changing pic_ops into an array + .read = pic_read, + .write = pic_write, + .endianness = DEVICE_BIG_ENDIAN, + .impl = { + .min_access_size = 4, + .max_access_size = 4, + }, + .valid = { + /* + * All XPS INTC registers are accessed through the PLB interface. + * The base address for these registers is provided by the + * configuration parameter, C_BASEADDR. Each register is 32 bits + * although some bits may be unused and is accessed on a 4-byte + * boundary offset from the base address. + */ + .min_access_size = 4, + .max_access_size = 4, + }, }, - .valid = { - .min_access_size = 4, - .max_access_size = 4 - } + [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN, + [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN, }; static void irq_handler(void *opaque, int irq, int level) @@ -174,13 +191,27 @@ static void xilinx_intc_init(Object *obj) qdev_init_gpio_in(DEVICE(obj), irq_handler, 32); sysbus_init_irq(SYS_BUS_DEVICE(obj), &p->parent_irq); - - memory_region_init_io(&p->mmio, obj, &pic_ops, p, "xlnx.xps-intc", - R_MAX * 4); sysbus_init_mmio(SYS_BUS_DEVICE(obj), &p->mmio); } +static void xilinx_intc_realize(DeviceState *dev, Error **errp) +{ + XpsIntc *p = XILINX_INTC(dev); + + if (p->model_endianness == ENDIAN_MODE_UNSPECIFIED) { + error_setg(errp, TYPE_XILINX_INTC " property 'endianness'" + " must be set to 'big' or 'little'"); + return; + } ... would it be possible to patch in the right value for pic_ops.endianness here instead? Ah, clever than my reply on the previous patch. I'll give it a try, thanks! Thomas
Re: [PATCH 1/3] hw/cxl/cxl-mailbox-utils.c: Added support for Get Log Capabilities (Opcode 0402h)
On 04/02/25 10:28AM, Jonathan Cameron wrote: On Mon, 3 Feb 2025 11:29:48 +0530 Arpit Kumar wrote: Please add some descriptive teext here. Sure, will append here in V2 patch. Signed-off-by: Arpit Kumar Reviewed-by: Alok Rathore Reviewed-by: Krishna Kanth Reddy Hi Arpit, Whilst it is good to do internal reviews, I'd prefer to see feedback on the mailing list if possible. Hard for a maintainer to tell what a RB tag given before posting means unfortunately so in most cases preference is to not add RB tags based on internal review. If your colleagues have greatly affected the code, a Co-developed-by: and additional sign off may be a good way to reflect that. Great to have you working on these features. Some comments inline. Main ones are around naming (always tricky!) and suggestion to handle the arrays of log capabilities as static const pointers. Thanks, Jonathan Thanks for the review Jonathan, will make changes accordingly in the next iteration-V2 of the patch. --- hw/cxl/cxl-mailbox-utils.c | 55 include/hw/cxl/cxl_device.h | 31 include/hw/cxl/cxl_mailbox.h | 5 3 files changed, 91 insertions(+) diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c index 9c7ea5bc35..3d66a425a9 100644 --- a/hw/cxl/cxl-mailbox-utils.c +++ b/hw/cxl/cxl-mailbox-utils.c @@ -76,6 +76,7 @@ enum { LOGS= 0x04, #define GET_SUPPORTED 0x0 #define GET_LOG 0x1 +#define GET_LOG_CAPABILITIES 0x2 FEATURES= 0x05, #define GET_SUPPORTED 0x0 #define GET_FEATURE 0x1 @@ -1075,6 +1076,45 @@ static CXLRetCode cmd_logs_get_log(const struct cxl_cmd *cmd, return CXL_MBOX_SUCCESS; } +static int32_t valid_log_check(QemuUUID *uuid, CXLCCI *cci) Perhaps find_log_index() or something like that? I would return &ccx->supported_log_cap[i] / NULL if not found as then can avoid long reference into array below. okay, got it. +{ +int32_t id = -1; +for (int i = CEL; i < MAX_LOG_TYPE; i++) { +if (qemu_uuid_is_equal(uuid, +&cci->supported_log_cap[i].uuid)) { +id = i; +break; +} +} +return id; +} + +/* CXL r3.1 Section 8.2.9.5.3: Get Log Capabilities (Opcode 0402h) */ For new documentation please use latest spec. This is a bit different to many other spec where the request is the earliest one. That is due to the consortium only making the latest version available (currently r3.2) okay +static CXLRetCode cmd_logs_get_log_capabilities(const struct cxl_cmd *cmd, +uint8_t *payload_in, +size_t len_in, +uint8_t *payload_out, +size_t *len_out, +CXLCCI *cci) +{ +int32_t cap_id; +struct { +QemuUUID uuid; +} QEMU_PACKED QEMU_ALIGNED(8) * get_log_capabilities_in = (void *)payload_in; + +CXLParamFlags *get_log_capabilities_out = (void *)payload_out; + +cap_id = valid_log_check(&get_log_capabilities_in->uuid, cci); CXLLogCapabilities *cap; cap = find_log_cap(&get_log_capabilities_in->uuid, cci); if (!cap) { return CXL_MBOX_INVALID_LOG); } mempcy(get_log_capabilities_out, cap->param_flags.pflags, sizeof(cap->param_flags.pflags)); ... got it. +if (cap_id == -1) { +return CXL_MBOX_INVALID_LOG; +} + +memcpy(get_log_capabilities_out, &cci->supported_log_cap[cap_id].param_flags.pflags, + sizeof(CXLParamFlags)); +*len_out = sizeof(*get_log_capabilities_out); +return CXL_MBOX_SUCCESS; +} + /* CXL r3.1 section 8.2.9.6: Features */ /* * Get Supported Features output payload @@ -2840,6 +2880,8 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = { [LOGS][GET_SUPPORTED] = { "LOGS_GET_SUPPORTED", cmd_logs_get_supported, 0, 0 }, [LOGS][GET_LOG] = { "LOGS_GET_LOG", cmd_logs_get_log, 0x18, 0 }, +[LOGS][GET_LOG_CAPABILITIES] = { "LOGS_GET_LOG_CAPABILITIES", + cmd_logs_get_log_capabilities, 0x10, 0 }, [FEATURES][GET_SUPPORTED] = { "FEATURES_GET_SUPPORTED", cmd_features_get_supported, 0x8, 0 }, [FEATURES][GET_FEATURE] = { "FEATURES_GET_FEATURE", @@ -3084,10 +3126,23 @@ static void cxl_rebuild_cel(CXLCCI *cci) } } +static const struct CXLLogCapabilities cxl_get_log_cap[MAX_LOG_TYPE] = { +[CEL] = {.param_flags.pflags = CXL_LOG_CAP_CLEAR | CXL_LOG_CAP_POPULATE, + .uuid = cel_uuid}, +}; + +static void cxl_init_get_log(CXLCCI *cci) +{ +for (int set = CEL; set < MAX_LOG_TYPE; set++) { +cci->supported_log_cap[set] = cxl_get_log_cap[set]; As below. Can we just use a static const pointer in cci? static const pointer in cci gives
Re: [PATCH 3/3] hw/cxl/cxl-mailbox-utils.c: Added support for Populate Log (Opcode 0404h) as background operation
On 04/02/25 10:58AM, Jonathan Cameron wrote: On Mon, 3 Feb 2025 11:29:50 +0530 Arpit Kumar wrote: Signed-off-by: Arpit Kumar Reviewed-by: Alok Rathore Reviewed-by: Krishna Kanth Reddy Likewise, the CEL isn't a dynamic thing. Asking to populate it makes little sense so the log capability should always say this is not supported. I suspect you had this running with a different log and flipped to CEL for purposes of up streaming? Anyhow, choose something else. Maybe component state dump or ECS log? Thanks for the review Jonathan, will update the same in V2 patch and return unsupported for CEL. --- hw/cxl/cxl-mailbox-utils.c | 95 + include/hw/cxl/cxl_device.h | 2 + 2 files changed, 97 insertions(+) diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c index 5fd7f850c4..115c2dc66b 100644 --- a/hw/cxl/cxl-mailbox-utils.c +++ b/hw/cxl/cxl-mailbox-utils.c @@ -78,6 +78,7 @@ enum { #define GET_LOG 0x1 #define GET_LOG_CAPABILITIES 0x2 #define CLEAR_LOG 0x3 +#define POPULATE_LOG 0x4 FEATURES= 0x05, #define GET_SUPPORTED 0x0 #define GET_FEATURE 0x1 @@ -1149,6 +1150,94 @@ static CXLRetCode cmd_logs_clear_log(const struct cxl_cmd *cmd, return CXL_MBOX_SUCCESS; } +static void cxl_rebuild_cel(CXLCCI *cci); + +static int get_populate_duration(uint32_t total_mem) +{ +int secs = 0; + +if (total_mem <= 512) { +secs = 4; +} else if (total_mem <= 1024) { +secs = 8; +} else if (total_mem <= 2 * 1024) { +secs = 15; +} else if (total_mem <= 4 * 1024) { +secs = 30; +} else if (total_mem <= 8 * 1024) { +secs = 60; +} else if (total_mem <= 16 * 1024) { +secs = 2 * 60; +} else if (total_mem <= 32 * 1024) { +secs = 4 * 60; +} else if (total_mem <= 64 * 1024) { +secs = 8 * 60; +} else if (total_mem <= 128 * 1024) { +secs = 15 * 60; +} else if (total_mem <= 256 * 1024) { +secs = 30 * 60; +} else if (total_mem <= 512 * 1024) { +secs = 60 * 60; +} else if (total_mem <= 1024 * 1024) { +secs = 120 * 60; +} else { +secs = 240 * 60; /* max 4 hrs */ +} + +return secs; +} + +/* CXL r3.1 Section 8.2.9.5.5: Populate log (Opcode 0404h) */ +static CXLRetCode cmd_logs_populate_log(const struct cxl_cmd *cmd, +uint8_t *payload_in, +size_t len_in, +uint8_t *payload_out, +size_t *len_out, +CXLCCI *cci) +{ +int32_t cap_id; +uint32_t total_mem = 0; +int secs = 0; +struct { +QemuUUID uuid; +} QEMU_PACKED QEMU_ALIGNED(8) * populate_log = (void *)payload_in; + +cap_id = valid_log_check(&populate_log->uuid, cci); +if (cap_id == -1) { +return CXL_MBOX_INVALID_LOG; +} + +if (cci->supported_log_cap[cap_id].param_flags.populate_log_supported) { +switch (cap_id) { +case CEL: Similar to before, a callback to fill the log inside the cap entry is probably going to be the most extensible way to do this rather than using an ID and a switch statement that gets steadily more complex and doesn't easily allow for different device emulation to make different choices on what to fill with (e.g. faking component state dump for a type 3 vs a type 2 device - once supported in qemu). okay +cci->log = CEL; +total_mem = (1 << 16) * sizeof(struct cel_log); +secs = get_populate_duration(total_mem >> 20); Why would the CEL be based on memory size? CEL is not dependent on memory size. It is representing CEL buffer size. total_mem variable is misleading in this case. will take care in next iteration. +goto start_bg; +break; +default: +return CXL_MBOX_UNSUPPORTED; +} +} +return CXL_MBOX_UNSUPPORTED; + +start_bg: +cci->bg.runtime = secs * 1000UL; +*len_out = 0; +return CXL_MBOX_BG_STARTED; +} + +static void __do_populate(CXLCCI *cci) +{ +switch (cci->log) { +case CEL: +cxl_rebuild_cel(cci); +break; +default: +break; +} +} + /* CXL r3.1 section 8.2.9.6: Features */ /* * Get Supported Features output payload @@ -2918,6 +3007,9 @@ static const struct cxl_cmd cxl_cmd_set[256][256] = { cmd_logs_get_log_capabilities, 0x10, 0 }, [LOGS][CLEAR_LOG] = { "LOGS_CLEAR_LOG", cmd_logs_clear_log, 0x10, CXL_MBOX_IMMEDIATE_LOG_CHANGE}, +[LOGS][POPULATE_LOG] = { "LOGS_POPULATE_LOG", cmd_logs_populate_log, 0x10, + (CXL_MBOX_IMMEDIATE_LOG_CHANGE | + CXL_MBOX_BACKGROUND_OPERATION)}, [FEATURES][GET_SUPPORTED] = { "FEATURES_GET_SUPPORTED
Re: [PATCH v7 04/10] hw/timer/xilinx_timer: Make device endianness configurable
On 12/02/2025 13.36, Philippe Mathieu-Daudé wrote: Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness for each machine using the device. Signed-off-by: Philippe Mathieu-Daudé --- hw/microblaze/petalogix_ml605_mmu.c | 1 + hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 + hw/ppc/virtex_ml507.c| 1 + hw/riscv/microblaze-v-generic.c | 2 ++ hw/timer/xilinx_timer.c | 43 +--- 5 files changed, 35 insertions(+), 13 deletions(-) diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c index 55398cc67d1..490640e9428 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -127,6 +127,7 @@ petalogix_ml605_init(MachineState *machine) /* 2 timers at irq 2 @ 100 Mhz. */ dev = qdev_new("xlnx.xps-timer"); +qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 100 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); Same question as in the second patch: Do we still need to take care of TARGET_BIG_ENDIAN here? Anyway, Reviewed-by: Thomas Huth
Re: [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust
On Wed, Feb 12, 2025 at 01:58:15PM +0100, Kevin Wolf wrote: > Am 12.02.2025 um 10:41 hat Daniel P. Berrangé geschrieben: > > On Wed, Feb 12, 2025 at 09:14:57AM +, Daniel P. Berrangé wrote: > > > On Tue, Feb 11, 2025 at 10:43:27PM +0100, Kevin Wolf wrote: > > > > This adds a separate block driver for the bochs image format called > > > > 'bochs-rs' so that for the moment both the C implementation and the Rust > > > > implementation can be present in the same build. The intention is to > > > > remove the C implementation eventually and rename this one into 'bochs'. > > > > This can only happen once Rust can be a hard build dependency for QEMU. > > > > > > > > Signed-off-by: Kevin Wolf > > > > --- > > > > rust/block/Cargo.toml| 2 +- > > > > rust/block/src/bochs.rs | 296 +++ > > > > rust/block/src/driver.rs | 5 - > > > > rust/block/src/lib.rs| 1 + > > > > 4 files changed, 298 insertions(+), 6 deletions(-) > > > > create mode 100644 rust/block/src/bochs.rs > > > > > > > > diff --git a/rust/block/Cargo.toml b/rust/block/Cargo.toml > > > > index 70ee02f429..1c06f3a00c 100644 > > > > --- a/rust/block/Cargo.toml > > > > +++ b/rust/block/Cargo.toml > > > > @@ -3,7 +3,7 @@ name = "block" > > > > version = "0.1.0" > > > > edition = "2021" > > > > authors = ["Kevin Wolf "] > > > > -license = "GPL-2.0-or-later" > > > > +license = "GPL-2.0-or-later AND MIT" > > > > readme = "README.md" > > > > description = "Block backends for QEMU" > > > > repository = "https://gitlab.com/qemu-project/qemu/"; > > > > diff --git a/rust/block/src/bochs.rs b/rust/block/src/bochs.rs > > > > new file mode 100644 > > > > index 00..388ac5ef03 > > > > --- /dev/null > > > > +++ b/rust/block/src/bochs.rs > > > > @@ -0,0 +1,296 @@ > > > > +// SPDX-License-Identifier: MIT > > > > > > Why MIT instead of our normal GPL-2.0-or-later. > > > > > > Using Rust conversion to eliminate GPL usage for permissive licenses > > > like MIT is not something I'd like to see us doing. > > > > My bad. I should have noticed that the original bochs.c was also MIT, > > so I presume you're considering this Rust impl to be a derived work. > > Yes, this is essentially a translation of the existing C bochs driver, > which is also why I kept the original copyright notice and the full > license text. Agreed, I think including both is the right thing in this particular case, especially given that we don't include the MIT text at all in a LICENSE file at the top level. 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 v7 06/10] hw/ssi/xilinx_spi: Make device endianness configurable
On 12/02/2025 13.36, Philippe Mathieu-Daudé wrote: Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness on the single machine using the device. Signed-off-by: Philippe Mathieu-Daudé --- hw/microblaze/petalogix_ml605_mmu.c | 1 + hw/ssi/xilinx_spi.c | 32 + 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c index 490640e9428..b34edf13796 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -175,6 +175,7 @@ petalogix_ml605_init(MachineState *machine) SSIBus *spi; dev = qdev_new("xlnx.xps-spi"); +qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE); qdev_prop_set_uint8(dev, "num-ss-bits", NUM_SPI_FLASHES); busdev = SYS_BUS_DEVICE(dev); sysbus_realize_and_unref(busdev, &error_fatal); TARGET_BIG_ENDIAN required again? Anyway, Reviewed-by: Thomas Huth
Re: [PATCH v7 05/10] hw/char/xilinx_uartlite: Make device endianness configurable
On 12/02/2025 13.36, Philippe Mathieu-Daudé wrote: Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness on the single machine using the device. With the patch description fixed (as you mentioned in patch 02): Reviewed-by: Thomas Huth
Re: [PATCH] block/rbd: Do not use BDS's AioContext
Am 12.02.2025 um 10:32 hat Hanna Czenczek geschrieben: > RBD schedules the request completion code (qemu_rbd_finish_bh()) to run > in the BDS's AioContext. The intent seems to be to run it in the same > context that the original request coroutine ran in, i.e. the thread on > whose stack the RBDTask object exists (see qemu_rbd_start_co()). > > However, with multiqueue, that thread is not necessarily the same as the > BDS's AioContext. Instead, we need to remember the actual AioContext > and schedule the completion BH there. > > Buglink: https://issues.redhat.com/browse/RHEL-67115 Please add a short summary of what actually happens to the commit message. I had to check the link to remember what the symptoms are. > Reported-by: Junyao Zhao > Signed-off-by: Hanna Czenczek > --- > I think I could also drop RBDTask.ctx and just use > `qemu_coroutine_get_aio_context(RBDTask.co)` instead, but this is the > version of the patch that was tested and confirmed to fix the issue (I > don't have a local reproducer), so I thought I'll post this first. Did you figure out why it even makes a difference in which thread qemu_rbd_finish_bh() runs? For context: static void qemu_rbd_finish_bh(void *opaque) { RBDTask *task = opaque; task->complete = true; aio_co_wake(task->co); } This looks as if it should be working in any thread, except maybe for a missing barrier after updating task->complete - but I think the failure mode for that would be a hang in qemu_rbd_start_co(). > block/rbd.c | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/block/rbd.c b/block/rbd.c > index af984fb7db..9d4e0817e0 100644 > --- a/block/rbd.c > +++ b/block/rbd.c > @@ -102,7 +102,7 @@ typedef struct BDRVRBDState { > } BDRVRBDState; > > typedef struct RBDTask { > -BlockDriverState *bs; > +AioContext *ctx; > Coroutine *co; > bool complete; > int64_t ret; > @@ -1269,8 +1269,7 @@ static void qemu_rbd_completion_cb(rbd_completion_t c, > RBDTask *task) > { > task->ret = rbd_aio_get_return_value(c); > rbd_aio_release(c); > -aio_bh_schedule_oneshot(bdrv_get_aio_context(task->bs), > -qemu_rbd_finish_bh, task); > +aio_bh_schedule_oneshot(task->ctx, qemu_rbd_finish_bh, task); > } > > static int coroutine_fn qemu_rbd_start_co(BlockDriverState *bs, > @@ -1281,7 +1280,10 @@ static int coroutine_fn > qemu_rbd_start_co(BlockDriverState *bs, >RBDAIOCmd cmd) > { > BDRVRBDState *s = bs->opaque; > -RBDTask task = { .bs = bs, .co = qemu_coroutine_self() }; > +RBDTask task = { > +.ctx = qemu_get_current_aio_context(), > +.co = qemu_coroutine_self(), > +}; > rbd_completion_t c; > int r; Nothing wrong I can see about the change, but I don't understand why it fixes the problem. Kevin
Re: [PATCH v5 3/5] migration: enable multifd and postcopy together
Hi, On Tue, 11 Feb 2025 at 20:50, Peter Xu wrote: > > * Yes. AFAIU, tls/file channels don't send magic values. > Please double check whether TLS will send magics. AFAICT, they should. === * ... Also tls live migration already does * tls handshake while initializing main channel so with tls this * issue is not possible. */ if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) { } else if (mis->from_src_file && (!strcmp(ioc->name, "migration-tls-incoming") || !strcmp(ioc->name, "migration-file-incoming"))) { channel = CH_MULTIFD; } === * From the comment and condition above, both 'tls' and 'file' channels are not peekable, ie. no magic value to peek. The 'migration-file-incoming' check also helps to cover the migrate_mapped_ram() case IIUC. > No. We need to figure out a way to do that properly, and that's exactly > what I mentioned as one of the core changes we need in this series, which > is still missing. We may or may not need an ACK message. Please think > about it. * First we tried to call 'multifd_send_shutdown()' to close multifd channels before calling postcopy_start(). That's the best case scenario wherein multifd channels are closed before postcopy starts. So that there's no confusion and/or jumbling of different data packets. It did not work, as qemu would crash during multifd_shutdown(). * Second is we push/flush all multifd pages before calling postcopy_start() and let the multifd channels exist/stay till the migration ends, after that they are duly shutdown. It is working well so far, passing all migration tests too. * Third, if we want to confirm that multifd pages are received on the destination before calling postcopy_start(), then the best way is for the destination to send an acknowledgement to the source side that it has received and processed all multifd pages and nothing is pending on the multifd channels. * Another could be to define a multifd_recv_flush() function, which could process and clear the receive side multifd queue, so that no multifd pages are pending there. Not sure how best to do this yet. Also considering it lacks proper communication and synchronisation between source and destination sides, it does not seem like the best solution. * Do you have any other option/approach in mind? > Please consider the case where multifd recv threads may get scheduled out > on dest host during precopy phase, not getting chance to be scheduled until > postcopy already started running on dst, then the recv thread can stumble > upon a page that was sent during precopy. As long as that can be always > avoided, I think we should be good. * TBH, this sounds like a remote corner case. * I'm testing the revised patch series and will send it shortly. Thank you. --- - Prasad
Re: [PULL 0/7] Functional tests and Gitlab-CI patches
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/10.0 for any user-visible changes. signature.asc Description: PGP signature
Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum
On 12/2/25 14:53, Philippe Mathieu-Daudé wrote: On 12/2/25 13:56, BALATON Zoltan wrote: On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote: On 12/2/25 12:37, Thomas Huth wrote: On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. Endianness can be BIG, LITTLE or unspecified (default). Signed-off-by: Philippe Mathieu-Daudé --- qapi/common.json | 16 include/hw/qdev-properties-system.h | 7 +++ hw/core/qdev-properties-system.c | 11 +++ 3 files changed, 34 insertions(+) diff --git a/qapi/common.json b/qapi/common.json index 6ffc7a37890..217feaaf683 100644 --- a/qapi/common.json +++ b/qapi/common.json @@ -212,3 +212,19 @@ ## { 'struct': 'HumanReadableText', 'data': { 'human-readable-text': 'str' } } + +## +# @EndianMode: +# +# An enumeration of three options: little, big, and unspecified +# +# @little: Little endianness +# +# @big: Big endianness +# +# @unspecified: Endianness not specified +# +# Since: 10.0 +## +{ 'enum': 'EndianMode', + 'data': [ 'little', 'big', 'unspecified' ] } Should 'unspecified' come first? ... so that it gets the value 0, i.e. when someone forgets to properly initialize a related variable, the chances are higher that it ends up as "unspecified" than as "little" ? Hmm but then in this series the dual-endianness regions are defined as: +static const MemoryRegionOps pic_ops[2] = { + [0 ... 1] = { This is already confusing as you'd have to know that 0 and 1 here means ENDIAN_MODE_LITTLE and ENDIAN_MODE_BIG (using those constants here as well might be clearer). It's easy to miss what this does so At this point 0 / 1 only mean "from the index #0 included to the index #1 included", 0 being the first one and 1 the last one. maybe repeating the definitions for each case would be longer but less confusing and then it does not matter what the values are. This is what I tried to do with: +[ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN, +[ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN, }; but in v7 we are back of picking an arbitrary value. Or what uses the ops.endianness now should look at the property introduced by this patch to avoid having to propagate it like below and drop the ops.endianness? Or it should move to the memory region and set when the ops are assigned? I'm not understanding well what you ask, but maybe the answer is in v7 :)
Re: [PULL 02/14] os: add an ability to lock memory on_fault
On Tue, Feb 11, 2025 at 5:52 PM Peter Xu wrote: > > From: Daniil Tatianin > > This will be used in the following commits to make it possible to only > lock memory on fault instead of right away. Hi Peter and Daniil, Please take a look at this CI failure: https://gitlab.com/qemu-project/qemu/-/jobs/9117106042#L3603 Thanks, Stefan > > Reviewed-by: Vladimir Sementsov-Ogievskiy > Reviewed-by: Peter Xu > Signed-off-by: Daniil Tatianin > Link: > https://lore.kernel.org/r/20250123131944.391886-2-d-tatia...@yandex-team.ru > Signed-off-by: Peter Xu > --- > include/system/os-posix.h | 2 +- > include/system/os-win32.h | 3 ++- > migration/postcopy-ram.c | 2 +- > os-posix.c| 10 -- > system/vl.c | 2 +- > 5 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/include/system/os-posix.h b/include/system/os-posix.h > index b881ac6c6f..ce5b3bccf8 100644 > --- a/include/system/os-posix.h > +++ b/include/system/os-posix.h > @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id); > void os_set_chroot(const char *path); > void os_setup_limits(void); > void os_setup_post(void); > -int os_mlock(void); > +int os_mlock(bool on_fault); > > /** > * qemu_alloc_stack: > diff --git a/include/system/os-win32.h b/include/system/os-win32.h > index b82a5d3ad9..cd61d69e10 100644 > --- a/include/system/os-win32.h > +++ b/include/system/os-win32.h > @@ -123,8 +123,9 @@ static inline bool is_daemonized(void) > return false; > } > > -static inline int os_mlock(void) > +static inline int os_mlock(bool on_fault) > { > +(void)on_fault; > return -ENOSYS; > } > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c > index 6a6da6ba7f..fc4d8a10df 100644 > --- a/migration/postcopy-ram.c > +++ b/migration/postcopy-ram.c > @@ -652,7 +652,7 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState > *mis) > } > > if (enable_mlock) { > -if (os_mlock() < 0) { > +if (os_mlock(false) < 0) { > error_report("mlock: %s", strerror(errno)); > /* > * It doesn't feel right to fail at this point, we have a valid > diff --git a/os-posix.c b/os-posix.c > index 9cce55ff2f..48afb2990d 100644 > --- a/os-posix.c > +++ b/os-posix.c > @@ -327,18 +327,24 @@ void os_set_line_buffering(void) > setvbuf(stdout, NULL, _IOLBF, 0); > } > > -int os_mlock(void) > +int os_mlock(bool on_fault) > { > #ifdef HAVE_MLOCKALL > int ret = 0; > +int flags = MCL_CURRENT | MCL_FUTURE; > > -ret = mlockall(MCL_CURRENT | MCL_FUTURE); > +if (on_fault) { > +flags |= MCL_ONFAULT; > +} > + > +ret = mlockall(flags); > if (ret < 0) { > error_report("mlockall: %s", strerror(errno)); > } > > return ret; > #else > +(void)on_fault; > return -ENOSYS; > #endif > } > diff --git a/system/vl.c b/system/vl.c > index 9c6942c6cf..e94fc7ea35 100644 > --- a/system/vl.c > +++ b/system/vl.c > @@ -797,7 +797,7 @@ static QemuOptsList qemu_run_with_opts = { > static void realtime_init(void) > { > if (enable_mlock) { > -if (os_mlock() < 0) { > +if (os_mlock(false) < 0) { > error_report("locking memory failed"); > exit(1); > } > -- > 2.47.0 > >
Re: [PATCH v5 1/4] os: add an ability to lock memory on_fault
Hi Daniil, On 12/2/25 15:39, Daniil Tatianin wrote: This will be used in the following commits to make it possible to only lock memory on fault instead of right away. Signed-off-by: Daniil Tatianin --- include/system/os-posix.h | 2 +- include/system/os-win32.h | 3 ++- meson.build | 6 ++ migration/postcopy-ram.c | 2 +- os-posix.c| 14 -- system/vl.c | 2 +- 6 files changed, 23 insertions(+), 6 deletions(-) diff --git a/include/system/os-posix.h b/include/system/os-posix.h index b881ac6c6f..ce5b3bccf8 100644 --- a/include/system/os-posix.h +++ b/include/system/os-posix.h @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id); void os_set_chroot(const char *path); void os_setup_limits(void); void os_setup_post(void); -int os_mlock(void); +int os_mlock(bool on_fault); If we need to support more flag, is your plan to add more arguments? Otherwise, why not use a 'int flags' argument, and have the callers pass MCL_ONFAULT?
Re: [PATCH v5 1/4] os: add an ability to lock memory on_fault
On 12.02.25 17:39, Daniil Tatianin wrote: This will be used in the following commits to make it possible to only lock memory on fault instead of right away. Signed-off-by: Daniil Tatianin Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [PATCH] vdpa: Allow vDPA to work on big-endian machine
On 2/12/2025 07:38, Eugenio Perez Martin wrote: On Tue, Feb 11, 2025 at 5:20 PM Konstantin Shkolnyy wrote: Add .set_vnet_le() function that always returns success, assuming that vDPA h/w always implements LE data format. Otherwise, QEMU disables vDPA and outputs the message: "backend does not support LE vnet headers; falling back on userspace virtio" Signed-off-by: Konstantin Shkolnyy I guess this patch should be merged after https://lists.nongnu.org/archive/html/qemu-devel/2025-02/msg02290.html ? Actually, it is better to make it a series of patches, isn't it? It doesn't matter if it's before or after. The only (coincidental) connection between these 2 patches is that both are needed for QEMU to enable vDPA on a big-endian machine.
Re: [PATCH v5 1/4] os: add an ability to lock memory on_fault
On 2/12/25 5:48 PM, Philippe Mathieu-Daudé wrote: Hi Daniil, On 12/2/25 15:39, Daniil Tatianin wrote: This will be used in the following commits to make it possible to only lock memory on fault instead of right away. Signed-off-by: Daniil Tatianin --- include/system/os-posix.h | 2 +- include/system/os-win32.h | 3 ++- meson.build | 6 ++ migration/postcopy-ram.c | 2 +- os-posix.c | 14 -- system/vl.c | 2 +- 6 files changed, 23 insertions(+), 6 deletions(-) diff --git a/include/system/os-posix.h b/include/system/os-posix.h index b881ac6c6f..ce5b3bccf8 100644 --- a/include/system/os-posix.h +++ b/include/system/os-posix.h @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id); void os_set_chroot(const char *path); void os_setup_limits(void); void os_setup_post(void); -int os_mlock(void); +int os_mlock(bool on_fault); If we need to support more flag, is your plan to add more arguments? Otherwise, why not use a 'int flags' argument, and have the callers pass MCL_ONFAULT? Hi! I chose this approach because MCL_ONFAULT is a POSIX/linux-specific flag, and this function is called in places that are platform-agnostic, thus a bool flag seemed more fitting.
Re: [PATCH] vdpa: Allow vDPA to work on big-endian machine
On 11/2/25 17:19, Konstantin Shkolnyy wrote: Add .set_vnet_le() function that always returns success, assuming that vDPA h/w always implements LE data format. Otherwise, QEMU disables vDPA and outputs the message: "backend does not support LE vnet headers; falling back on userspace virtio" Signed-off-by: Konstantin Shkolnyy --- net/vhost-vdpa.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index 231b45246c..7219aa2eee 100644 --- a/net/vhost-vdpa.c +++ b/net/vhost-vdpa.c @@ -270,6 +270,11 @@ static bool vhost_vdpa_has_ufo(NetClientState *nc) } +static int vhost_vdpa_set_vnet_le(NetClientState *nc, bool is_le) +{ +return 0; +} + static bool vhost_vdpa_check_peer_type(NetClientState *nc, ObjectClass *oc, Error **errp) { @@ -437,6 +442,7 @@ static NetClientInfo net_vhost_vdpa_info = { .cleanup = vhost_vdpa_cleanup, .has_vnet_hdr = vhost_vdpa_has_vnet_hdr, .has_ufo = vhost_vdpa_has_ufo, +.set_vnet_le = vhost_vdpa_set_vnet_le, Dubious mismatch with set_vnet_be handler. .check_peer_type = vhost_vdpa_check_peer_type, .set_steering_ebpf = vhost_vdpa_set_steering_ebpf, };
Re: [PATCH v5 1/4] os: add an ability to lock memory on_fault
On 12/2/25 15:51, Daniil Tatianin wrote: On 2/12/25 5:48 PM, Philippe Mathieu-Daudé wrote: Hi Daniil, On 12/2/25 15:39, Daniil Tatianin wrote: This will be used in the following commits to make it possible to only lock memory on fault instead of right away. Signed-off-by: Daniil Tatianin --- include/system/os-posix.h | 2 +- include/system/os-win32.h | 3 ++- meson.build | 6 ++ migration/postcopy-ram.c | 2 +- os-posix.c | 14 -- system/vl.c | 2 +- 6 files changed, 23 insertions(+), 6 deletions(-) diff --git a/include/system/os-posix.h b/include/system/os-posix.h index b881ac6c6f..ce5b3bccf8 100644 --- a/include/system/os-posix.h +++ b/include/system/os-posix.h @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id); void os_set_chroot(const char *path); void os_setup_limits(void); void os_setup_post(void); -int os_mlock(void); +int os_mlock(bool on_fault); If we need to support more flag, is your plan to add more arguments? Otherwise, why not use a 'int flags' argument, and have the callers pass MCL_ONFAULT? Hi! I chose this approach because MCL_ONFAULT is a POSIX/linux-specific flag, and this function is called in places that are platform-agnostic, thus a bool flag seemed more fitting. OK then.
Re: [PATCH 3/8] hw/arm/realview: Explicit number of GIC external IRQs
On 30/1/25 19:36, Peter Maydell wrote: On Thu, 30 Jan 2025 at 18:25, Philippe Mathieu-Daudé wrote: When not specified, Cortex-A9MP configures its GIC with 64 external IRQs (see commit a32134aad89 "arm:make the number of GIC interrupts configurable"). Add the GIC_EXT_IRQS definition (with a comment) to make that explicit. Except explicitly setting a property value to its same implicit value, there is no logical change intended. Signed-off-by: Philippe Mathieu-Daudé --- hw/arm/realview.c | 17 +++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/hw/arm/realview.c b/hw/arm/realview.c index 9900a98f3b8..4a62c83506b 100644 --- a/hw/arm/realview.c +++ b/hw/arm/realview.c @@ -35,6 +35,14 @@ #define SMP_BOOT_ADDR 0xe000 #define SMP_BOOTREG_ADDR 0x1030 +/* + * The Cortex-A9MP may have anything from 0 to 224 external interrupt + * IRQ lines (with another 32 internal). We default to 64+32, which + * is the number provided by the Cortex-A9MP test chip in the + * Realview PBX-A9 and Versatile Express A9 development boards. + */ On the other hand, this really *is* the Realview PBX-A9 development board. So we can just say that that's the right number (and the vexpress is irrelevant, and the range of settings the CPU itself can have isn't very important either). (PS: there's no verb "to explicit" in English (ignoring some obscure obsolete ones); French "expliciter" => English "to make explicit"; or you in the case of the subject line here, "specify explicitly" is probably most natural.) TIL, thanks :)
Re: [PATCH 03/11] rust: Add some block layer bindings
Am 12.02.2025 um 10:29 hat Paolo Bonzini geschrieben: > On 2/11/25 22:43, Kevin Wolf wrote: > > Signed-off-by: Kevin Wolf > > --- > > rust/wrapper.h| 4 > > meson.build | 1 + > > rust/qemu-api/src/zeroable.rs | 5 +++-- > > 3 files changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/rust/wrapper.h b/rust/wrapper.h > > index 41be87adcf..c3e1e6f9cf 100644 > > --- a/rust/wrapper.h > > +++ b/rust/wrapper.h > > @@ -53,3 +53,7 @@ typedef enum memory_order { > > #include "chardev/char-fe.h" > > #include "qapi/error.h" > > #include "chardev/char-serial.h" > > +#include "block/block.h" > > +#include "block/block_int.h" > > +#include "block/qdict.h" > > +#include "qapi/qapi-visit-block-core.h" > > diff --git a/meson.build b/meson.build > > index 30aae6b3c3..154195bc80 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -4045,6 +4045,7 @@ if have_rust > > '--with-derive-default', > > '--no-layout-tests', > > '--no-prepend-enum-name', > > +'--allowlist-item', 'EINVAL|EIO', > > I've got some errno bindings that I wrote for chardev, I'll send them > shortly. Yes, we definitely need some proper bindings there. I'm already tired of writing things like this: return -(bindings::EINVAL as std::os::raw::c_int) Or even: return e .raw_os_error() .unwrap_or(-(bindings::EIO as std::os::raw::c_int)) Which actually already shows that your errno binding patch does the opposite direction of what I needed in this series. My problem is when I need to return an int to C, and I either have an io::Result or I just want to directly return an errno value. So we'll have to add that part to your errno module, too. Kevin
Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum
On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote: On 12/2/25 12:37, Thomas Huth wrote: On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. Endianness can be BIG, LITTLE or unspecified (default). Signed-off-by: Philippe Mathieu-Daudé --- qapi/common.json | 16 include/hw/qdev-properties-system.h | 7 +++ hw/core/qdev-properties-system.c | 11 +++ 3 files changed, 34 insertions(+) diff --git a/qapi/common.json b/qapi/common.json index 6ffc7a37890..217feaaf683 100644 --- a/qapi/common.json +++ b/qapi/common.json @@ -212,3 +212,19 @@ ## { 'struct': 'HumanReadableText', 'data': { 'human-readable-text': 'str' } } + +## +# @EndianMode: +# +# An enumeration of three options: little, big, and unspecified +# +# @little: Little endianness +# +# @big: Big endianness +# +# @unspecified: Endianness not specified +# +# Since: 10.0 +## +{ 'enum': 'EndianMode', + 'data': [ 'little', 'big', 'unspecified' ] } Should 'unspecified' come first? ... so that it gets the value 0, i.e. when someone forgets to properly initialize a related variable, the chances are higher that it ends up as "unspecified" than as "little" ? Hmm but then in this series the dual-endianness regions are defined as: +static const MemoryRegionOps pic_ops[2] = { +[0 ... 1] = { This is already confusing as you'd have to know that 0 and 1 here means ENDIAN_MODE_LITTLE and ENDIAN_MODE_BIG (using those constants here as well might be clearer). It's easy to miss what this does so maybe repeating the definitions for each case would be longer but less confusing and then it does not matter what the values are. Or what uses the ops.endianness now should look at the property introduced by this patch to avoid having to propagate it like below and drop the ops.endianness? Or it should move to the memory region and set when the ops are assigned? Regards, BALATON Zoltan +.read = pic_read, +.write = pic_write, +.endianness = DEVICE_BIG_ENDIAN, +.impl = { +.min_access_size = 4, +.max_access_size = 4, +}, +.valid = { +/* + * All XPS INTC registers are accessed through the PLB interface. + * The base address for these registers is provided by the + * configuration parameter, C_BASEADDR. Each register is 32 bits + * although some bits may be unused and is accessed on a 4-byte + * boundary offset from the base address. + */ +.min_access_size = 4, +.max_access_size = 4, +}, }, -.valid = { -.min_access_size = 4, -.max_access_size = 4 -} +[ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN, +[ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN, }; We could declare the array using the confusing __MAX definition (at the price of wasting the ENDIAN_MODE_UNSPECIFIED entry) as: static const MemoryRegionOps pic_ops[ENDIAN_MODE__MAX - 1] { } WDYT? Apart from that: Reviewed-by: Thomas Huth Thanks!
Re: [PATCH 04/11] rust/qemu-api: Add wrappers to run futures in QEMU
On Wed, Feb 12, 2025 at 1:47 PM Kevin Wolf wrote: > > > +pub fn qemu_co_run_future(future: F) -> F::Output { > > > +let waker = Arc::new(RunFutureWaker { > > > +co: unsafe { bindings::qemu_coroutine_self() }, > > > +}) > > > +.into(); > > > > into what? :) Maybe you can add the type to the "let" for clarity. > > Into Waker. Do we have any idea yet how explicit we want to be with type > annotations that aren't strictly necessary? I didn't think of making it > explicit here because the only thing that is done with it is building a > Context from it, so it seemed obvious enough. > > If you think that being explicit is better, then Waker::from() might > be better than adding a type name to let and keeping .into(). I think this case threw me off because From> is a bit more generic than your usual From implementation. Maybe it's obvious to anyone who has used futures in Rust (I haven't). I agree, something like let waker = Waker::from(waker); let mut cx = Context::from_waker(&waker); could be the best of both worlds. > > Also, would qemu_co_run_future() and qemu_run_future() become methods on an > > Executor later? Maybe it make sense to have already something like > > > > pub trait QemuExecutor { > > fn run_until(future: F) -> F::Output; > > } > > > > pub struct Executor; > > pub struct CoExecutor; > > > > and pass an executor to Rust functions (&Executor for no_coroutine_fn, > > &CoExecutor for coroutine_fn, &dyn QemuExecutor for mixed). Or would that > > be premature in your opinion? > > If we could get bindgen to actually do that for the C interface, then > this could be interesting, though for most functions it also would > remain unused boilerplate. If we have to get the executor manually on > the Rust side for each function, that's probably the same function that > will want to execute the future - in which case it just can directly > call the correct function. The idea was that you don't call the correct function but the *only* function :) i.e. exec.run_until(), and it will do the right thing for coroutine vs. no coroutine. But yeah, there would be boilerplate to pass it all the way down so maybe it is not a great idea. I liked the concept that you just *couldn't* get _co_ wrong... but perhaps it is not necessary once more of "BlockDriver::open" is lifted into bdrv_open. Paolo > The long term idea should be anyway that C coroutines disappear from the > path and we integrate an executor into the QEMU main loop. But as long > as the block layer core is written in C and uses coroutines and we want > Rust futures to be able to call into coroutine_fns, that's still a long > way to go.
Re: [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust
Am 12.02.2025 um 08:45 hat Philippe Mathieu-Daudé geschrieben: > On 11/2/25 22:43, Kevin Wolf wrote: > > This adds a separate block driver for the bochs image format called > > 'bochs-rs' so that for the moment both the C implementation and the Rust > > implementation can be present in the same build. The intention is to > > remove the C implementation eventually and rename this one into 'bochs'. > > This can only happen once Rust can be a hard build dependency for QEMU. > > > > Signed-off-by: Kevin Wolf > > --- > > rust/block/Cargo.toml| 2 +- > > rust/block/src/bochs.rs | 296 +++ > > rust/block/src/driver.rs | 5 - > > rust/block/src/lib.rs| 1 + > > 4 files changed, 298 insertions(+), 6 deletions(-) > > create mode 100644 rust/block/src/bochs.rs > > > > diff --git a/rust/block/src/bochs.rs b/rust/block/src/bochs.rs > > new file mode 100644 > > index 00..388ac5ef03 > > --- /dev/null > > +++ b/rust/block/src/bochs.rs > > @@ -0,0 +1,296 @@ > > +// SPDX-License-Identifier: MIT > > +/* > > + * Block driver for the various disk image formats used by Bochs > > + * Currently only for "growing" type in read-only mode > > + * > > + * Copyright (c) 2005 Alex Beregszaszi > > + * Copyright (c) 2024 Red Hat > > + * > > + * Authors: > > + * Alex Beregszaszi > > + * Kevin Wolf > > + * > > + * Permission is hereby granted, free of charge, to any person obtaining a > > copy > > + * of this software and associated documentation files (the "Software"), > > to deal > > + * in the Software without restriction, including without limitation the > > rights > > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or > > sell > > + * copies of the Software, and to permit persons to whom the Software is > > + * furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice shall be included > > in > > + * all copies or substantial portions of the Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS > > OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > > OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > FROM, > > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > > IN > > + * THE SOFTWARE. > > + */ > > As an addition, we don't have to add the full license boilerplate IMO... IANAL, but the license says "The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.", so keeping it feels like the safe option. Kevin
Re: [PATCH v7 03/10] hw/net/xilinx_ethlite: Make device endianness configurable
On 12/02/2025 13.36, Philippe Mathieu-Daudé wrote: Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness on the single machine using the device. Signed-off-by: Philippe Mathieu-Daudé --- Reviewed-by: Thomas Huth
Re: [PATCH v4 27/33] vfio/migration: Multifd device state transfer support - received buffers queuing
On 1/30/25 11:08, Maciej S. Szmigiero wrote: From: "Maciej S. Szmigiero" The multifd received data needs to be reassembled since device state packets sent via different multifd channels can arrive out-of-order. Therefore, each VFIO device state packet carries a header indicating its position in the stream. The raw device state data is saved into a VFIOStateBuffer for later in-order loading into the device. The last such VFIO device state packet should have VFIO_DEVICE_STATE_CONFIG_STATE flag set and carry the device config state. Signed-off-by: Maciej S. Szmigiero --- hw/vfio/migration.c | 116 ++ hw/vfio/pci.c | 2 + hw/vfio/trace-events | 1 + include/hw/vfio/vfio-common.h | 1 + 4 files changed, 120 insertions(+) diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index bcdf204d5cf4..0c0caec1bd64 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -301,6 +301,12 @@ typedef struct VFIOStateBuffer { } VFIOStateBuffer; typedef struct VFIOMultifd { +VFIOStateBuffers load_bufs; +QemuCond load_bufs_buffer_ready_cond; +QemuMutex load_bufs_mutex; /* Lock order: this lock -> BQL */ +uint32_t load_buf_idx; +uint32_t load_buf_idx_last; +uint32_t load_buf_queued_pending_buffers; } VFIOMultifd; static void vfio_state_buffer_clear(gpointer data) @@ -346,6 +352,103 @@ static VFIOStateBuffer *vfio_state_buffers_at(VFIOStateBuffers *bufs, guint idx) return &g_array_index(bufs->array, VFIOStateBuffer, idx); } Each routine executed from a migration thread should have a preliminary comment saying from which context it is called: migration or VFIO +static bool vfio_load_state_buffer_insert(VFIODevice *vbasedev, + VFIODeviceStatePacket *packet, + size_t packet_total_size, + Error **errp) +{ +VFIOMigration *migration = vbasedev->migration; +VFIOMultifd *multifd = migration->multifd; +VFIOStateBuffer *lb; + +vfio_state_buffers_assert_init(&multifd->load_bufs); +if (packet->idx >= vfio_state_buffers_size_get(&multifd->load_bufs)) { +vfio_state_buffers_size_set(&multifd->load_bufs, packet->idx + 1); +} + +lb = vfio_state_buffers_at(&multifd->load_bufs, packet->idx); +if (lb->is_present) { +error_setg(errp, "state buffer %" PRIu32 " already filled", + packet->idx); +return false; +} + +assert(packet->idx >= multifd->load_buf_idx); + +multifd->load_buf_queued_pending_buffers++; +if (multifd->load_buf_queued_pending_buffers > +vbasedev->migration_max_queued_buffers) { +error_setg(errp, + "queuing state buffer %" PRIu32 " would exceed the max of %" PRIu64, + packet->idx, vbasedev->migration_max_queued_buffers); +return false; +} AFAICT, attributes multifd->load_buf_queued_pending_buffers and vbasedev->migration_max_queued_buffers are not strictly necessary. They allow to count buffers and check an arbitrary limit, which is UINT64_MAX today. It makes me wonder how useful they are. Please introduce them in a separate patch at the end of the series, adding documentation on the "x-migration-max-queued-buffers" property and also general documentation on why and how to use it. + +lb->data = g_memdup2(&packet->data, packet_total_size - sizeof(*packet)); +lb->len = packet_total_size - sizeof(*packet); +lb->is_present = true; + +return true; +} + +static bool vfio_load_state_buffer(void *opaque, char *data, size_t data_size, + Error **errp) +{ +VFIODevice *vbasedev = opaque; +VFIOMigration *migration = vbasedev->migration; +VFIOMultifd *multifd = migration->multifd; +VFIODeviceStatePacket *packet = (VFIODeviceStatePacket *)data; + +/* + * Holding BQL here would violate the lock order and can cause + * a deadlock once we attempt to lock load_bufs_mutex below. + */ +assert(!bql_locked()); + +if (!migration->multifd_transfer) { +error_setg(errp, + "got device state packet but not doing multifd transfer"); +return false; +} + +assert(multifd); + +if (data_size < sizeof(*packet)) { +error_setg(errp, "packet too short at %zu (min is %zu)", + data_size, sizeof(*packet)); +return false; +} + +if (packet->version != 0) { Please add a define for version, even if 0. +error_setg(errp, "packet has unknown version %" PRIu32, + packet->version); +return false; +} + +if (packet->idx == UINT32_MAX) { +error_setg(errp, "packet has too high idx %" PRIu32, + packet->idx); I don't think printing out packet->idx is useful here. +return false; +} + +trace_vfio_load_sta
Re: [PATCH 03/11] rust: Add some block layer bindings
On Wed, Feb 12, 2025 at 2:13 PM Kevin Wolf wrote: > Yes, we definitely need some proper bindings there. I'm already tired of > writing things like this: > > return -(bindings::EINVAL as std::os::raw::c_int) > > Or even: > > return e > .raw_os_error() > .unwrap_or(-(bindings::EIO as std::os::raw::c_int)) This by the way seemed incorrect to me as it should be return -(e .raw_os_error() .unwrap_or(bindings::EIO as std::os::raw::c_int)) (leaving aside that raw_os_error does not work on Windows)... But then I noticed that read_raw() also does not negate, which causes the error to print incorrectly... > Which actually already shows that your errno binding patch does the > opposite direction of what I needed in this series. ... so my patch already helps a bit: you can still change if ret < 0 { Err(Error::from_raw_os_error(ret)) } else { Ok(()) } to errno::into_io_result(ret)?; Ok(()) and avoid the positive/negative confusion. Anyhow, I guess the first one wouldn't be much better: return errno::into_negative_errno(ErrorKind::InvalidInput); whereas the second could be return errno::into_negative_errno(e); But then the first is already a special case; it only happens where your bindings are not trivial thanks to the introduction of the Mapping type. Paolo > My problem is when I > need to return an int to C, and I either have an io::Result or I just > want to directly return an errno value. So we'll have to add that part > to your errno module, too.
Re: [PATCH 10/11] bochs-rs: Add bochs block driver reimplementation in Rust
On 12/2/25 13:59, Kevin Wolf wrote: Am 12.02.2025 um 08:45 hat Philippe Mathieu-Daudé geschrieben: On 11/2/25 22:43, Kevin Wolf wrote: This adds a separate block driver for the bochs image format called 'bochs-rs' so that for the moment both the C implementation and the Rust implementation can be present in the same build. The intention is to remove the C implementation eventually and rename this one into 'bochs'. This can only happen once Rust can be a hard build dependency for QEMU. Signed-off-by: Kevin Wolf --- rust/block/Cargo.toml| 2 +- rust/block/src/bochs.rs | 296 +++ rust/block/src/driver.rs | 5 - rust/block/src/lib.rs| 1 + 4 files changed, 298 insertions(+), 6 deletions(-) create mode 100644 rust/block/src/bochs.rs diff --git a/rust/block/src/bochs.rs b/rust/block/src/bochs.rs new file mode 100644 index 00..388ac5ef03 --- /dev/null +++ b/rust/block/src/bochs.rs @@ -0,0 +1,296 @@ +// SPDX-License-Identifier: MIT +/* + * Block driver for the various disk image formats used by Bochs + * Currently only for "growing" type in read-only mode + * + * Copyright (c) 2005 Alex Beregszaszi + * Copyright (c) 2024 Red Hat + * + * Authors: + * Alex Beregszaszi + * Kevin Wolf + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ As an addition, we don't have to add the full license boilerplate IMO... IANAL, but the license says "The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.", so keeping it feels like the safe option. OK (and as Daniel clarified, you consider your work a derivative).
Re: [PATCH] vdpa: Allow vDPA to work on big-endian machine
On Tue, Feb 11, 2025 at 5:20 PM Konstantin Shkolnyy wrote: > > Add .set_vnet_le() function that always returns success, assuming that > vDPA h/w always implements LE data format. Otherwise, QEMU disables vDPA and > outputs the message: > "backend does not support LE vnet headers; falling back on userspace virtio" > > Signed-off-by: Konstantin Shkolnyy I guess this patch should be merged after https://lists.nongnu.org/archive/html/qemu-devel/2025-02/msg02290.html ? Actually, it is better to make it a series of patches, isn't it? > --- > net/vhost-vdpa.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > index 231b45246c..7219aa2eee 100644 > --- a/net/vhost-vdpa.c > +++ b/net/vhost-vdpa.c > @@ -270,6 +270,11 @@ static bool vhost_vdpa_has_ufo(NetClientState *nc) > > } > > +static int vhost_vdpa_set_vnet_le(NetClientState *nc, bool is_le) > +{ > +return 0; > +} > + > static bool vhost_vdpa_check_peer_type(NetClientState *nc, ObjectClass *oc, > Error **errp) > { > @@ -437,6 +442,7 @@ static NetClientInfo net_vhost_vdpa_info = { > .cleanup = vhost_vdpa_cleanup, > .has_vnet_hdr = vhost_vdpa_has_vnet_hdr, > .has_ufo = vhost_vdpa_has_ufo, > +.set_vnet_le = vhost_vdpa_set_vnet_le, > .check_peer_type = vhost_vdpa_check_peer_type, > .set_steering_ebpf = vhost_vdpa_set_steering_ebpf, > }; > -- > 2.34.1 >
Re: [PULL 02/14] os: add an ability to lock memory on_fault
On 2/12/25 5:13 PM, Stefan Hajnoczi wrote: On Tue, Feb 11, 2025 at 5:52 PM Peter Xu wrote: From: Daniil Tatianin This will be used in the following commits to make it possible to only lock memory on fault instead of right away. Hi Peter and Daniil, Please take a look at this CI failure: https://gitlab.com/qemu-project/qemu/-/jobs/9117106042#L3603 Thanks, Stefan Looks like MCL_ONFAULT is linux-only, I guess I'll have to have add something similar to HAVE_MLOCKALL and put this under an ifdef. Thank you Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Peter Xu Signed-off-by: Daniil Tatianin Link: https://lore.kernel.org/r/20250123131944.391886-2-d-tatia...@yandex-team.ru Signed-off-by: Peter Xu --- include/system/os-posix.h | 2 +- include/system/os-win32.h | 3 ++- migration/postcopy-ram.c | 2 +- os-posix.c| 10 -- system/vl.c | 2 +- 5 files changed, 13 insertions(+), 6 deletions(-) diff --git a/include/system/os-posix.h b/include/system/os-posix.h index b881ac6c6f..ce5b3bccf8 100644 --- a/include/system/os-posix.h +++ b/include/system/os-posix.h @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id); void os_set_chroot(const char *path); void os_setup_limits(void); void os_setup_post(void); -int os_mlock(void); +int os_mlock(bool on_fault); /** * qemu_alloc_stack: diff --git a/include/system/os-win32.h b/include/system/os-win32.h index b82a5d3ad9..cd61d69e10 100644 --- a/include/system/os-win32.h +++ b/include/system/os-win32.h @@ -123,8 +123,9 @@ static inline bool is_daemonized(void) return false; } -static inline int os_mlock(void) +static inline int os_mlock(bool on_fault) { +(void)on_fault; return -ENOSYS; } diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index 6a6da6ba7f..fc4d8a10df 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -652,7 +652,7 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis) } if (enable_mlock) { -if (os_mlock() < 0) { +if (os_mlock(false) < 0) { error_report("mlock: %s", strerror(errno)); /* * It doesn't feel right to fail at this point, we have a valid diff --git a/os-posix.c b/os-posix.c index 9cce55ff2f..48afb2990d 100644 --- a/os-posix.c +++ b/os-posix.c @@ -327,18 +327,24 @@ void os_set_line_buffering(void) setvbuf(stdout, NULL, _IOLBF, 0); } -int os_mlock(void) +int os_mlock(bool on_fault) { #ifdef HAVE_MLOCKALL int ret = 0; +int flags = MCL_CURRENT | MCL_FUTURE; -ret = mlockall(MCL_CURRENT | MCL_FUTURE); +if (on_fault) { +flags |= MCL_ONFAULT; +} + +ret = mlockall(flags); if (ret < 0) { error_report("mlockall: %s", strerror(errno)); } return ret; #else +(void)on_fault; return -ENOSYS; #endif } diff --git a/system/vl.c b/system/vl.c index 9c6942c6cf..e94fc7ea35 100644 --- a/system/vl.c +++ b/system/vl.c @@ -797,7 +797,7 @@ static QemuOptsList qemu_run_with_opts = { static void realtime_init(void) { if (enable_mlock) { -if (os_mlock() < 0) { +if (os_mlock(false) < 0) { error_report("locking memory failed"); exit(1); } -- 2.47.0
Re: [PATCH] block/rbd: Do not use BDS's AioContext
On 12.02.25 14:26, Kevin Wolf wrote: Am 12.02.2025 um 10:32 hat Hanna Czenczek geschrieben: RBD schedules the request completion code (qemu_rbd_finish_bh()) to run in the BDS's AioContext. The intent seems to be to run it in the same context that the original request coroutine ran in, i.e. the thread on whose stack the RBDTask object exists (see qemu_rbd_start_co()). However, with multiqueue, that thread is not necessarily the same as the BDS's AioContext. Instead, we need to remember the actual AioContext and schedule the completion BH there. Buglink:https://issues.redhat.com/browse/RHEL-67115 Please add a short summary of what actually happens to the commit message. I had to check the link to remember what the symptoms are. Sure. The problem is, I don’t know exactly what’s going wrong (looked like a coroutine being rescheduled after it was already done), and I don’t know how this fixes it. Reported-by: Junyao Zhao Signed-off-by: Hanna Czenczek --- I think I could also drop RBDTask.ctx and just use `qemu_coroutine_get_aio_context(RBDTask.co)` instead, but this is the version of the patch that was tested and confirmed to fix the issue (I don't have a local reproducer), so I thought I'll post this first. Did you figure out why it even makes a difference in which thread qemu_rbd_finish_bh() runs? For context: static void qemu_rbd_finish_bh(void *opaque) { RBDTask *task = opaque; task->complete = true; aio_co_wake(task->co); } This looks as if it should be working in any thread, except maybe for a missing barrier after updating task->complete - but I think the failure mode for that would be a hang in qemu_rbd_start_co(). Yes, I thought the same thing. All I could imagine was that maybe reading task->co returns the wrong result, but given how long ago that must have been set, it seems quite unlikely (to say the least). In addition, qemu_rbd_completion_cb() already reads the object from a different thread, and that seems to work fine. Really, all I know is that the notion of a BDS’s AioContext no longer makes sense in a multiqueue I/O path, so this should be scheduled in the I/O’s AioContext (just conceptually speaking), and that this seems to fix the bug. block/rbd.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/block/rbd.c b/block/rbd.c index af984fb7db..9d4e0817e0 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -102,7 +102,7 @@ typedef struct BDRVRBDState { } BDRVRBDState; typedef struct RBDTask { -BlockDriverState *bs; +AioContext *ctx; Coroutine *co; bool complete; int64_t ret; @@ -1269,8 +1269,7 @@ static void qemu_rbd_completion_cb(rbd_completion_t c, RBDTask *task) { task->ret = rbd_aio_get_return_value(c); rbd_aio_release(c); -aio_bh_schedule_oneshot(bdrv_get_aio_context(task->bs), -qemu_rbd_finish_bh, task); +aio_bh_schedule_oneshot(task->ctx, qemu_rbd_finish_bh, task); } static int coroutine_fn qemu_rbd_start_co(BlockDriverState *bs, @@ -1281,7 +1280,10 @@ static int coroutine_fn qemu_rbd_start_co(BlockDriverState *bs, RBDAIOCmd cmd) { BDRVRBDState *s = bs->opaque; -RBDTask task = { .bs = bs, .co = qemu_coroutine_self() }; +RBDTask task = { +.ctx = qemu_get_current_aio_context(), +.co = qemu_coroutine_self(), +}; rbd_completion_t c; int r; Nothing wrong I can see about the change, but I don't understand why it fixes the problem. Me neither. But if this patch had been part of one of the original multiqueue series (without pointing out the linked bug), would there have been any argument against it? Indeed it is a problem that I don’t understand what’s happening. But even more honestly, I’ll have to admit I can’t ever claim to understand what’s happening in a multi-threaded asynchronous C environment; even more so when the reproducer is installing Windows on RBD. Hanna
[PATCH] qapi: merge common parts of NbdServerOptions and nbd-server-start data
Instead of comment "Keep this type consistent with the nbd-server-start arguments", we can simply merge these things. Signed-off-by: Vladimir Sementsov-Ogievskiy --- No problem for me to rebase on top of [PATCH 0/2] nbd: Allow debugging tuning of handshake limit if it goes earlier. Also, not that order of nbd-server-start is changed. I think the order could not be a contract of JSON interface. We could instead of making common base structure go another way and define two structures with same data=NbdServerOptionsCommon, and different bases (will have to define additional base strucutres with SocketAddress and SocketAddressLegacy fields). But it would look a bit unusual in QAPI. blockdev-nbd.c | 4 +-- qapi/block-export.json | 57 -- 2 files changed, 29 insertions(+), 32 deletions(-) diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 9e61fbaf2b..b0b8993a1b 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -215,10 +215,10 @@ void nbd_server_start_options(NbdServerOptions *arg, Error **errp) arg->max_connections, errp); } -void qmp_nbd_server_start(SocketAddressLegacy *addr, - const char *tls_creds, +void qmp_nbd_server_start(const char *tls_creds, const char *tls_authz, bool has_max_connections, uint32_t max_connections, + SocketAddressLegacy *addr, Error **errp) { SocketAddress *addr_flat = socket_address_flatten(addr); diff --git a/qapi/block-export.json b/qapi/block-export.json index 117b05d13c..5eb94213db 100644 --- a/qapi/block-export.json +++ b/qapi/block-export.json @@ -9,13 +9,7 @@ { 'include': 'block-core.json' } ## -# @NbdServerOptions: -# -# Keep this type consistent with the nbd-server-start arguments. The -# only intended difference is using SocketAddress instead of -# SocketAddressLegacy. -# -# @addr: Address on which to listen. +# @NbdServerOptionsBase: # # @tls-creds: ID of the TLS credentials object (since 2.6). # @@ -30,14 +24,35 @@ # server from advertising multiple client support (since 5.2; # default: 100) # -# Since: 4.2 +# Since: 10.0 ## -{ 'struct': 'NbdServerOptions', - 'data': { 'addr': 'SocketAddress', -'*tls-creds': 'str', +{ 'struct': 'NbdServerOptionsBase', + 'data': { '*tls-creds': 'str', '*tls-authz': 'str', '*max-connections': 'uint32' } } +## +# @NbdServerOptions: +# +# @addr: Address on which to listen. +# +# Since: 10.0 +## +{ 'struct': 'NbdServerOptions', + 'base': 'NbdServerOptionsBase', + 'data': { 'addr': 'SocketAddress' } } + +## +# @NbdServerOptionsLegacy: +# +# @addr: Address on which to listen. +# +# Since: 10.0 +## +{ 'struct': 'NbdServerOptionsLegacy', + 'base': 'NbdServerOptionsBase', + 'data': { 'addr': 'SocketAddressLegacy' } } + ## # @nbd-server-start: # @@ -50,31 +65,13 @@ # intended difference is using SocketAddressLegacy instead of # SocketAddress. # -# @addr: Address on which to listen. -# -# @tls-creds: ID of the TLS credentials object (since 2.6). -# -# @tls-authz: ID of the QAuthZ authorization object used to validate -# the client's x509 distinguished name. This object is is only -# resolved at time of use, so can be deleted and recreated on the -# fly while the NBD server is active. If missing, it will default -# to denying access (since 4.0). -# -# @max-connections: The maximum number of connections to allow at the -# same time, 0 for unlimited. Setting this to 1 also stops the -# server from advertising multiple client support (since 5.2; -# default: 100). -# # Errors: # - if the server is already running # # Since: 1.3 ## { 'command': 'nbd-server-start', - 'data': { 'addr': 'SocketAddressLegacy', -'*tls-creds': 'str', -'*tls-authz': 'str', -'*max-connections': 'uint32' }, + 'data': 'NbdServerOptionsLegacy', 'allow-preconfig': true } ## -- 2.48.1
Re: [PATCH 2/2] nbd/server: Allow users to adjust handshake limit in QMP
On 11.02.25 00:46, Eric Blake wrote: On Thu, Feb 06, 2025 at 10:20:09AM +0300, Vladimir Sementsov-Ogievskiy wrote: --- qapi/block-export.json | 10 ++ include/block/nbd.h| 6 +++--- [..] @@ -52,6 +57,10 @@ # # @addr: Address on which to listen. # +# @handshake-max-secs: Time limit, in seconds, at which a client that +# has not completed the negotiation handshake will be disconnected, +# or 0 for no limit (since 10.0; default: 10). +# Hmm. [not about the series], shouldn't we finally deprecate older interface? By older interface, you are asking about the QMP command 'nbd-server-start' as compared to struct NbdServerOptions. But the struct is not directly present in any QMP commands; rather, it only appears to be used by qemu-storage-daemon as one of its command line options that needs to set up an NBD server with a JSON-like syntax that has less nesting than QMP nbd-server-start. blockdev-nbd.c has two functions [nbd_server_start_options(NbdServerOPtions *arg...) and qmp_nbd_server_start(args...)] that both unpack their slightly different forms and pass them as parameters to nbd_server_start() that is then agnostic to whether the older QMP command or newer q-s-d CLI option was specified. It looks like libvirt is still using QMP nbd-server-start. If we were to start the deprecation process for qemu 10.0, what would the new command look like? What would everyone be required to use by qemu 10.2? Oh you are right, I was inattentive. So, probably thing to be deprecated is actually SocketAddressLegacy, which is the only difference. And why just not move common part of the structures to common base? I'll send a patch with a try. -- Best regards, Vladimir
Re: [PATCH v5 3/5] migration: enable multifd and postcopy together
On Wed, Feb 12, 2025 at 06:57:48PM +0530, Prasad Pandit wrote: > Hi, > > On Tue, 11 Feb 2025 at 20:50, Peter Xu wrote: > > > * Yes. AFAIU, tls/file channels don't send magic values. > > Please double check whether TLS will send magics. AFAICT, they should. > === > * ... Also tls live migration already does > * tls handshake while initializing main channel so with tls this > * issue is not possible. > */ > if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) { > } else if (mis->from_src_file >&& (!strcmp(ioc->name, "migration-tls-incoming") > || !strcmp(ioc->name, "migration-file-incoming"))) { > channel = CH_MULTIFD; > } > === > * From the comment and condition above, both 'tls' and 'file' channels > are not peekable, ie. no magic value to peek. The > 'migration-file-incoming' check also helps to cover the > migrate_mapped_ram() case IIUC. I think it's not because TLS channels don't send magic, but TLS channels are not prone to ordering issues. In general, I'm not convinced we need to check names of iochannels. > > > No. We need to figure out a way to do that properly, and that's exactly > > what I mentioned as one of the core changes we need in this series, which > > is still missing. We may or may not need an ACK message. Please think > > about it. > > * First we tried to call 'multifd_send_shutdown()' to close multifd > channels before calling postcopy_start(). That's the best case > scenario wherein multifd channels are closed before postcopy starts. > So that there's no confusion and/or jumbling of different data > packets. It did not work, as qemu would crash during > multifd_shutdown(). Have we debugged the crash? I'm not saying we should go with this, but crash isn't a reason to not choose a design. > > * Second is we push/flush all multifd pages before calling > postcopy_start() and let the multifd channels exist/stay till the > migration ends, after that they are duly shutdown. It is working well > so far, passing all migration tests too. > > * Third, if we want to confirm that multifd pages are received on the > destination before calling postcopy_start(), then the best way is for > the destination to send an acknowledgement to the source side that it > has received and processed all multifd pages and nothing is pending on > the multifd channels. > > * Another could be to define a multifd_recv_flush() function, which > could process and clear the receive side multifd queue, so that no > multifd pages are pending there. Not sure how best to do this yet. > Also considering it lacks proper communication and synchronisation > between source and destination sides, it does not seem like the best > solution. > > * Do you have any other option/approach in mind? > > > Please consider the case where multifd recv threads may get scheduled out > > on dest host during precopy phase, not getting chance to be scheduled until > > postcopy already started running on dst, then the recv thread can stumble > > upon a page that was sent during precopy. As long as that can be always > > avoided, I think we should be good. > > * TBH, this sounds like a remote corner case. No this is not. As I mentioned tons of times.. copying page in socket buffers directly into guest page during vcpus running / postcopy is a very hard to debug issue. If it's possible to happen, the design is flawed. > > * I'm testing the revised patch series and will send it shortly. I don't think passing the unit tests prove the series is correct and should be merged. We need to understand how it work, or we can't merge it. I feel very frustrated multiple times that you seem to want to ignore what I comment. I don't know why you rush to repost things. After a 2nd thought, I think maybe multifd flush and sync could work on src side indeed, because when flush and sync there'll be a message (MULTIFD_FLUSH) on main channel and that should order against the rest postcopy messages that will also be sent on the main channel (if we do multifd flush before all the postcopy processes). Then it should guarantee when postcopy starts on dest, dest multifd recv threads flushed all messages, and no multifd message will arrive anymore. But again we should guard it with an assert() in recv threads if you want to postpone recycling of multifd threads, just to double check no outliers we overlooked. When proposing the patches you need to justify the design first before anything. Please evaluate above, these things are critical to appear in either code comments or commit messages. Please digest everything before reposting. Thanks, -- Peter Xu
[PATCH v5 0/4] overcommit: introduce mem-lock-onfault
Currently, passing mem-lock=on to QEMU causes memory usage to grow by huge amounts: no memlock: $ ./qemu-system-x86_64 -overcommit mem-lock=off $ ps -p $(pidof ./qemu-system-x86_64) -o rss= 45652 $ ./qemu-system-x86_64 -overcommit mem-lock=off -enable-kvm $ ps -p $(pidof ./qemu-system-x86_64) -o rss= 39756 memlock: $ ./qemu-system-x86_64 -overcommit mem-lock=on $ ps -p $(pidof ./qemu-system-x86_64) -o rss= 1309876 $ ./qemu-system-x86_64 -overcommit mem-lock=on -enable-kvm $ ps -p $(pidof ./qemu-system-x86_64) -o rss= 259956 This is caused by the fact that mlockall(2) automatically write-faults every existing and future anonymous mappings in the process right away. One of the reasons to enable mem-lock is to protect a QEMU process' pages from being compacted and migrated by kcompactd (which does so by messing with a live process page tables causing thousands of TLB flush IPIs per second) basically stealing all guest time while it's active. mem-lock=on helps against this (given compact_unevictable_allowed is 0), but the memory overhead it introduces is an undesirable side effect, which we can completely avoid by passing MCL_ONFAULT to mlockall, which is what this series allows to do with a new option for mem-lock called on-fault. memlock-onfault: $ ./qemu-system-x86_64 -overcommit mem-lock=on-fault $ ps -p $(pidof ./qemu-system-x86_64) -o rss= 54004 $ ./qemu-system-x86_64 -overcommit mem-lock=on-fault -enable-kvm $ ps -p $(pidof ./qemu-system-x86_64) -o rss= 47772 You may notice the memory usage is still slightly higher, in this case by a few megabytes over the mem-lock=off case. I was able to trace this down to a bug in the linux kernel with MCL_ONFAULT not being honored for the early process heap (with brk(2) etc.) so it is still write-faulted in this case, but it's still way less than it was with just the mem-lock=on. Changes since v1: - Don't make a separate mem-lock-onfault, add an on-fault option to mem-lock instead Changes since v2: - Move overcommit option parsing out of line - Make enable_mlock an enum instead Changes since v3: - Rebase to latest master due to the recent sysemu -> system renames Changes since v4: - Fix compile errors under FreeBSD and MacOS Daniil Tatianin (4): os: add an ability to lock memory on_fault system/vl: extract overcommit option parsing into a helper system: introduce a new MlockState enum overcommit: introduce mem-lock=on-fault hw/virtio/virtio-mem.c| 2 +- include/system/os-posix.h | 2 +- include/system/os-win32.h | 3 ++- include/system/system.h | 12 - meson.build | 6 + migration/postcopy-ram.c | 4 +-- os-posix.c| 14 +-- qemu-options.hx | 14 +++ system/globals.c | 12 - system/vl.c | 52 +++ 10 files changed, 97 insertions(+), 24 deletions(-) -- 2.34.1
[PATCH v5 2/4] system/vl: extract overcommit option parsing into a helper
This will be extended in the future commits, let's move it out of line right away so that it's easier to read. Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Peter Xu Signed-off-by: Daniil Tatianin --- system/vl.c | 21 ++--- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/system/vl.c b/system/vl.c index e94fc7ea35..72a40985f5 100644 --- a/system/vl.c +++ b/system/vl.c @@ -1875,6 +1875,19 @@ static void object_option_parse(const char *str) visit_free(v); } +static void overcommit_parse(const char *str) +{ +QemuOpts *opts; + +opts = qemu_opts_parse_noisily(qemu_find_opts("overcommit"), + str, false); +if (!opts) { +exit(1); +} +enable_mlock = qemu_opt_get_bool(opts, "mem-lock", enable_mlock); +enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", enable_cpu_pm); +} + /* * Very early object creation, before the sandbox options have been activated. */ @@ -3575,13 +3588,7 @@ void qemu_init(int argc, char **argv) object_option_parse(optarg); break; case QEMU_OPTION_overcommit: -opts = qemu_opts_parse_noisily(qemu_find_opts("overcommit"), - optarg, false); -if (!opts) { -exit(1); -} -enable_mlock = qemu_opt_get_bool(opts, "mem-lock", enable_mlock); -enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", enable_cpu_pm); +overcommit_parse(optarg); break; case QEMU_OPTION_compat: { -- 2.34.1
[PATCH v5 1/4] os: add an ability to lock memory on_fault
This will be used in the following commits to make it possible to only lock memory on fault instead of right away. Signed-off-by: Daniil Tatianin --- include/system/os-posix.h | 2 +- include/system/os-win32.h | 3 ++- meson.build | 6 ++ migration/postcopy-ram.c | 2 +- os-posix.c| 14 -- system/vl.c | 2 +- 6 files changed, 23 insertions(+), 6 deletions(-) diff --git a/include/system/os-posix.h b/include/system/os-posix.h index b881ac6c6f..ce5b3bccf8 100644 --- a/include/system/os-posix.h +++ b/include/system/os-posix.h @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id); void os_set_chroot(const char *path); void os_setup_limits(void); void os_setup_post(void); -int os_mlock(void); +int os_mlock(bool on_fault); /** * qemu_alloc_stack: diff --git a/include/system/os-win32.h b/include/system/os-win32.h index b82a5d3ad9..cd61d69e10 100644 --- a/include/system/os-win32.h +++ b/include/system/os-win32.h @@ -123,8 +123,9 @@ static inline bool is_daemonized(void) return false; } -static inline int os_mlock(void) +static inline int os_mlock(bool on_fault) { +(void)on_fault; return -ENOSYS; } diff --git a/meson.build b/meson.build index 18cf9e2913..59953cbe6b 100644 --- a/meson.build +++ b/meson.build @@ -2885,6 +2885,12 @@ config_host_data.set('HAVE_MLOCKALL', cc.links(gnu_source_prefix + ''' return mlockall(MCL_FUTURE); }''')) +config_host_data.set('HAVE_MLOCK_ONFAULT', cc.links(gnu_source_prefix + ''' + #include + int main(void) { + return mlockall(MCL_FUTURE | MCL_ONFAULT); + }''')) + have_l2tpv3 = false if get_option('l2tpv3').allowed() and have_system have_l2tpv3 = cc.has_type('struct mmsghdr', diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index 6a6da6ba7f..fc4d8a10df 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -652,7 +652,7 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis) } if (enable_mlock) { -if (os_mlock() < 0) { +if (os_mlock(false) < 0) { error_report("mlock: %s", strerror(errno)); /* * It doesn't feel right to fail at this point, we have a valid diff --git a/os-posix.c b/os-posix.c index 9cce55ff2f..17b144c0a2 100644 --- a/os-posix.c +++ b/os-posix.c @@ -327,18 +327,28 @@ void os_set_line_buffering(void) setvbuf(stdout, NULL, _IOLBF, 0); } -int os_mlock(void) +int os_mlock(bool on_fault) { #ifdef HAVE_MLOCKALL int ret = 0; +int flags = MCL_CURRENT | MCL_FUTURE; -ret = mlockall(MCL_CURRENT | MCL_FUTURE); +#ifdef HAVE_MLOCK_ONFAULT +if (on_fault) { +flags |= MCL_ONFAULT; +} +#else +(void)on_fault; +#endif + +ret = mlockall(flags); if (ret < 0) { error_report("mlockall: %s", strerror(errno)); } return ret; #else +(void)on_fault; return -ENOSYS; #endif } diff --git a/system/vl.c b/system/vl.c index 9c6942c6cf..e94fc7ea35 100644 --- a/system/vl.c +++ b/system/vl.c @@ -797,7 +797,7 @@ static QemuOptsList qemu_run_with_opts = { static void realtime_init(void) { if (enable_mlock) { -if (os_mlock() < 0) { +if (os_mlock(false) < 0) { error_report("locking memory failed"); exit(1); } -- 2.34.1
[PATCH v5 3/4] system: introduce a new MlockState enum
Replace the boolean value enable_mlock with an enum and add a helper to decide whether we should be calling os_mlock. This is a stepping stone towards introducing a new mlock mode, which will be the third possible state of this enum. Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Peter Xu Signed-off-by: Daniil Tatianin --- hw/virtio/virtio-mem.c | 2 +- include/system/system.h | 10 +- migration/postcopy-ram.c | 2 +- system/globals.c | 7 ++- system/vl.c | 9 +++-- 5 files changed, 24 insertions(+), 6 deletions(-) diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c index b1a003736b..7b140add76 100644 --- a/hw/virtio/virtio-mem.c +++ b/hw/virtio/virtio-mem.c @@ -991,7 +991,7 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp) return; } -if (enable_mlock) { +if (should_mlock(mlock_state)) { error_setg(errp, "Incompatible with mlock"); return; } diff --git a/include/system/system.h b/include/system/system.h index 0cbb43ec30..dc7628357a 100644 --- a/include/system/system.h +++ b/include/system/system.h @@ -44,10 +44,18 @@ extern int display_opengl; extern const char *keyboard_layout; extern int old_param; extern uint8_t *boot_splash_filedata; -extern bool enable_mlock; extern bool enable_cpu_pm; extern QEMUClockType rtc_clock; +typedef enum { +MLOCK_OFF = 0, +MLOCK_ON, +} MlockState; + +bool should_mlock(MlockState); + +extern MlockState mlock_state; + #define MAX_OPTION_ROMS 16 typedef struct QEMUOptionRom { const char *name; diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index fc4d8a10df..04068ee039 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -651,7 +651,7 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis) mis->have_fault_thread = false; } -if (enable_mlock) { +if (should_mlock(mlock_state)) { if (os_mlock(false) < 0) { error_report("mlock: %s", strerror(errno)); /* diff --git a/system/globals.c b/system/globals.c index 4867c93ca6..adeff38348 100644 --- a/system/globals.c +++ b/system/globals.c @@ -31,10 +31,15 @@ #include "system/cpus.h" #include "system/system.h" +bool should_mlock(MlockState state) +{ +return state == MLOCK_ON; +} + enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB; int display_opengl; const char* keyboard_layout; -bool enable_mlock; +MlockState mlock_state; bool enable_cpu_pm; int autostart = 1; int vga_interface_type = VGA_NONE; diff --git a/system/vl.c b/system/vl.c index 72a40985f5..2895824c1a 100644 --- a/system/vl.c +++ b/system/vl.c @@ -796,7 +796,7 @@ static QemuOptsList qemu_run_with_opts = { static void realtime_init(void) { -if (enable_mlock) { +if (should_mlock(mlock_state)) { if (os_mlock(false) < 0) { error_report("locking memory failed"); exit(1); @@ -1878,13 +1878,18 @@ static void object_option_parse(const char *str) static void overcommit_parse(const char *str) { QemuOpts *opts; +bool enable_mlock; opts = qemu_opts_parse_noisily(qemu_find_opts("overcommit"), str, false); if (!opts) { exit(1); } -enable_mlock = qemu_opt_get_bool(opts, "mem-lock", enable_mlock); + +enable_mlock = qemu_opt_get_bool(opts, "mem-lock", + should_mlock(mlock_state)); +mlock_state = enable_mlock ? MLOCK_ON : MLOCK_OFF; + enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", enable_cpu_pm); } -- 2.34.1
[PATCH v5 4/4] overcommit: introduce mem-lock=on-fault
Locking the memory without MCL_ONFAULT instantly prefaults any mmaped anonymous memory with a write-fault, which introduces a lot of extra overhead in terms of memory usage when all you want to do is to prevent kcompactd from migrating and compacting QEMU pages. Add an option to only lock pages lazily as they're faulted by the process by using MCL_ONFAULT if asked. Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Peter Xu Signed-off-by: Daniil Tatianin --- include/system/system.h | 2 ++ migration/postcopy-ram.c | 2 +- qemu-options.hx | 14 +- system/globals.c | 7 ++- system/vl.c | 34 +++--- 5 files changed, 45 insertions(+), 14 deletions(-) diff --git a/include/system/system.h b/include/system/system.h index dc7628357a..a7effe7dfd 100644 --- a/include/system/system.h +++ b/include/system/system.h @@ -50,9 +50,11 @@ extern QEMUClockType rtc_clock; typedef enum { MLOCK_OFF = 0, MLOCK_ON, +MLOCK_ON_FAULT, } MlockState; bool should_mlock(MlockState); +bool is_mlock_on_fault(MlockState); extern MlockState mlock_state; diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index 04068ee039..5d3edfcfec 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -652,7 +652,7 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis) } if (should_mlock(mlock_state)) { -if (os_mlock(false) < 0) { +if (os_mlock(is_mlock_on_fault(mlock_state)) < 0) { error_report("mlock: %s", strerror(errno)); /* * It doesn't feel right to fail at this point, we have a valid diff --git a/qemu-options.hx b/qemu-options.hx index 1b26ad53bd..61270e3206 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -4632,21 +4632,25 @@ SRST ERST DEF("overcommit", HAS_ARG, QEMU_OPTION_overcommit, -"-overcommit [mem-lock=on|off][cpu-pm=on|off]\n" +"-overcommit [mem-lock=on|off|on-fault][cpu-pm=on|off]\n" "run qemu with overcommit hints\n" -"mem-lock=on|off controls memory lock support (default: off)\n" +"mem-lock=on|off|on-fault controls memory lock support (default: off)\n" "cpu-pm=on|off controls cpu power management (default: off)\n", QEMU_ARCH_ALL) SRST -``-overcommit mem-lock=on|off`` +``-overcommit mem-lock=on|off|on-fault`` \ ``-overcommit cpu-pm=on|off`` Run qemu with hints about host resource overcommit. The default is to assume that host overcommits all resources. Locking qemu and guest memory can be enabled via ``mem-lock=on`` -(disabled by default). This works when host memory is not -overcommitted and reduces the worst-case latency for guest. +or ``mem-lock=on-fault`` (disabled by default). This works when +host memory is not overcommitted and reduces the worst-case latency for +guest. The on-fault option is better for reducing the memory footprint +since it makes allocations lazy, but the pages still get locked in place +once faulted by the guest or QEMU. Note that the two options are mutually +exclusive. Guest ability to manage power state of host cpus (increasing latency for other processes on the same host cpu, but decreasing latency for diff --git a/system/globals.c b/system/globals.c index adeff38348..316623bd20 100644 --- a/system/globals.c +++ b/system/globals.c @@ -33,7 +33,12 @@ bool should_mlock(MlockState state) { -return state == MLOCK_ON; +return state == MLOCK_ON || state == MLOCK_ON_FAULT; +} + +bool is_mlock_on_fault(MlockState state) +{ +return state == MLOCK_ON_FAULT; } enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB; diff --git a/system/vl.c b/system/vl.c index 2895824c1a..3c0fa2ff64 100644 --- a/system/vl.c +++ b/system/vl.c @@ -351,7 +351,7 @@ static QemuOptsList qemu_overcommit_opts = { .desc = { { .name = "mem-lock", -.type = QEMU_OPT_BOOL, +.type = QEMU_OPT_STRING, }, { .name = "cpu-pm", @@ -797,7 +797,7 @@ static QemuOptsList qemu_run_with_opts = { static void realtime_init(void) { if (should_mlock(mlock_state)) { -if (os_mlock(false) < 0) { +if (os_mlock(is_mlock_on_fault(mlock_state)) < 0) { error_report("locking memory failed"); exit(1); } @@ -1878,7 +1878,7 @@ static void object_option_parse(const char *str) static void overcommit_parse(const char *str) { QemuOpts *opts; -bool enable_mlock; +const char *mem_lock_opt; opts = qemu_opts_parse_noisily(qemu_find_opts("overcommit"), str, false); @@ -1886,11 +1886,31 @@ static void overcommit_parse(const char *str) exit(1); } -enable_mlock = qemu_opt_get_bool(opts, "mem-lock", - should_mlock(mlock_state
[PULL 0/1] Tracing patches
The following changes since commit f9edf32ea2e18a56de5d92f57e9d10565c822367: Merge tag 'pull-request-2025-02-11' of https://gitlab.com/thuth/qemu into staging (2025-02-11 13:27:32 -0500) are available in the Git repository at: https://gitlab.com/stefanha/qemu.git tags/tracing-pull-request for you to fetch changes up to 9976be3911a2d0503f026ae37c17077273bf30ee: scripts: improve error from qemu-trace-stap on missing 'stap' (2025-02-12 10:03:18 -0500) Pull request Daniel P. Berrangé (1): scripts: improve error from qemu-trace-stap on missing 'stap' scripts/qemu-trace-stap | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) -- 2.48.1
Re: [PATCH v3 09/23] hw/uefi: add var-service-core.c
Hi, > > Yes. Knowing both physical and virtual address works only for memory > > you allocated yourself before ExitBootServices. So you can't pass on > > pointers from the OS, you have to copy the data to a buffer where you > > know the physical address instead. Yes, some overhead. Should still > > be much faster than going to pio transfer mode ... > > MacOS takes over the full physical address map past ExitBootServices: Your > code no longer has VA access to random code That is totally fine. EFI drivers must register everything they need as runtime memory. Anything else can be unmapped by the OS when calling EFI services. > and it literally memcpy()'s all preserved (virtual available) code and > data to different physical addresses. Uhm. I have my doubts this copying behavior is blessed by the UEFI spec. > You simply have nothing that is all of 1) RAM (mapped as cacheable on > ARM), 2) known VA 3) known PA. Bummer. > So we really really need a fallback mechanism that works without DMA > :). On arm it should be relatively simple to move the buffer to device memory. Just place one more region on the platform bus, advertise address + size via device tree, done. Not sure how to do that best on x86 though. Find 64k unused address space over ioapic? Do we have enough free space there? And how future-proof would that be? take care, Gerd
[PULL 1/1] scripts: improve error from qemu-trace-stap on missing 'stap'
From: Daniel P. Berrangé If the 'stap' binary is missing in $PATH, a huge trace is thrown $ qemu-trace-stap list /usr/bin/qemu-system-x86_64 Traceback (most recent call last): File "/usr/bin/qemu-trace-stap", line 169, in main() File "/usr/bin/qemu-trace-stap", line 165, in main args.func(args) File "/usr/bin/qemu-trace-stap", line 83, in cmd_run subprocess.call(stapargs) File "/usr/lib64/python3.12/subprocess.py", line 389, in call with Popen(*popenargs, **kwargs) as p: ^^^ File "/usr/lib64/python3.12/subprocess.py", line 1026, in {}init{} self._execute_child(args, executable, preexec_fn, close_fds, File "/usr/lib64/python3.12/subprocess.py", line 1955, in _execute_child raise child_exception_type(errno_num, err_msg, err_filename) FileNotFoundError: [Errno 2] No such file or directory: 'stap' With this change the user now gets $ qemu-trace-stap list /usr/bin/qemu-system-x86_64 Unable to find 'stap' in $PATH Signed-off-by: Daniel P. Berrangé Reviewed-by: Philippe Mathieu-Daudé Message-id: 20241206114524.164-1-berra...@redhat.com Signed-off-by: Stefan Hajnoczi --- scripts/qemu-trace-stap | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/scripts/qemu-trace-stap b/scripts/qemu-trace-stap index eb6e951ff2..e983460ee7 100755 --- a/scripts/qemu-trace-stap +++ b/scripts/qemu-trace-stap @@ -56,6 +56,7 @@ def tapset_dir(binary): def cmd_run(args): +stap = which("stap") prefix = probe_prefix(args.binary) tapsets = tapset_dir(args.binary) @@ -76,7 +77,7 @@ def cmd_run(args): # We request an 8MB buffer, since the stap default 1MB buffer # can be easily overflowed by frequently firing QEMU traces -stapargs = ["stap", "-s", "8", "-I", tapsets ] +stapargs = [stap, "-s", "8", "-I", tapsets ] if args.pid is not None: stapargs.extend(["-x", args.pid]) stapargs.extend(["-e", script]) @@ -84,6 +85,7 @@ def cmd_run(args): def cmd_list(args): +stap = which("stap") tapsets = tapset_dir(args.binary) if args.verbose: @@ -96,7 +98,7 @@ def cmd_list(args): if verbose: print("Listing probes with name '%s'" % script) -proc = subprocess.Popen(["stap", "-I", tapsets, "-l", script], +proc = subprocess.Popen([stap, "-I", tapsets, "-l", script], stdout=subprocess.PIPE, universal_newlines=True) out, err = proc.communicate() -- 2.48.1
Re: [PATCH v3 5/7] memory: pass MemTxAttrs to memory_access_is_direct()
On Mon, Feb 10, 2025 at 09:46:46AM +0100, David Hildenbrand wrote: > We want to pass another flag that will be stored in MemTxAttrs. So pass > MemTxAttrs directly. > > Reviewed-by: Peter Xu > Reviewed-by: Philippe Mathieu-Daudé > Signed-off-by: David Hildenbrand > --- > hw/core/loader.c | 2 +- > hw/remote/vfio-user-obj.c | 2 +- > include/exec/memory.h | 5 +++-- > system/memory_ldst.c.inc | 18 +- > system/physmem.c | 12 ++-- > 5 files changed, 20 insertions(+), 19 deletions(-) This breaks mac builds.. I'll squash: diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m index aa1455b629..1554f3b801 100644 --- a/hw/display/apple-gfx.m +++ b/hw/display/apple-gfx.m @@ -137,7 +137,8 @@ static void apple_gfx_destroy_task(AppleGFXState *s, PGTask_t *task) MEMTXATTRS_UNSPECIFIED); if (!ram_region || ram_region_length < length || -!memory_access_is_direct(ram_region, !read_only)) { +!memory_access_is_direct(ram_region, !read_only, +MEMTXATTRS_UNSPECIFIED)) { return NULL; } -- Peter Xu
Re: [PATCH v5 1/4] os: add an ability to lock memory on_fault
On 2/12/25 6:23 PM, Peter Xu wrote: On Wed, Feb 12, 2025 at 05:39:17PM +0300, Daniil Tatianin wrote: This will be used in the following commits to make it possible to only lock memory on fault instead of right away. Signed-off-by: Daniil Tatianin --- include/system/os-posix.h | 2 +- include/system/os-win32.h | 3 ++- meson.build | 6 ++ migration/postcopy-ram.c | 2 +- os-posix.c| 14 -- system/vl.c | 2 +- 6 files changed, 23 insertions(+), 6 deletions(-) diff --git a/include/system/os-posix.h b/include/system/os-posix.h index b881ac6c6f..ce5b3bccf8 100644 --- a/include/system/os-posix.h +++ b/include/system/os-posix.h @@ -53,7 +53,7 @@ bool os_set_runas(const char *user_id); void os_set_chroot(const char *path); void os_setup_limits(void); void os_setup_post(void); -int os_mlock(void); +int os_mlock(bool on_fault); /** * qemu_alloc_stack: diff --git a/include/system/os-win32.h b/include/system/os-win32.h index b82a5d3ad9..cd61d69e10 100644 --- a/include/system/os-win32.h +++ b/include/system/os-win32.h @@ -123,8 +123,9 @@ static inline bool is_daemonized(void) return false; } -static inline int os_mlock(void) +static inline int os_mlock(bool on_fault) { +(void)on_fault; return -ENOSYS; } diff --git a/meson.build b/meson.build index 18cf9e2913..59953cbe6b 100644 --- a/meson.build +++ b/meson.build @@ -2885,6 +2885,12 @@ config_host_data.set('HAVE_MLOCKALL', cc.links(gnu_source_prefix + ''' return mlockall(MCL_FUTURE); }''')) +config_host_data.set('HAVE_MLOCK_ONFAULT', cc.links(gnu_source_prefix + ''' + #include + int main(void) { + return mlockall(MCL_FUTURE | MCL_ONFAULT); + }''')) + have_l2tpv3 = false if get_option('l2tpv3').allowed() and have_system have_l2tpv3 = cc.has_type('struct mmsghdr', diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index 6a6da6ba7f..fc4d8a10df 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -652,7 +652,7 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis) } if (enable_mlock) { -if (os_mlock() < 0) { +if (os_mlock(false) < 0) { error_report("mlock: %s", strerror(errno)); /* * It doesn't feel right to fail at this point, we have a valid diff --git a/os-posix.c b/os-posix.c index 9cce55ff2f..17b144c0a2 100644 --- a/os-posix.c +++ b/os-posix.c @@ -327,18 +327,28 @@ void os_set_line_buffering(void) setvbuf(stdout, NULL, _IOLBF, 0); } -int os_mlock(void) +int os_mlock(bool on_fault) { #ifdef HAVE_MLOCKALL int ret = 0; +int flags = MCL_CURRENT | MCL_FUTURE; -ret = mlockall(MCL_CURRENT | MCL_FUTURE); +#ifdef HAVE_MLOCK_ONFAULT +if (on_fault) { +flags |= MCL_ONFAULT; +} +#else +(void)on_fault; +#endif IIUC we should fail this when not supported. Would you mind I squash this in? Sure, go ahead. Thanks! diff --git a/os-posix.c b/os-posix.c index 17b144c0a2..52925c23d3 100644 --- a/os-posix.c +++ b/os-posix.c @@ -333,13 +333,14 @@ int os_mlock(bool on_fault) int ret = 0; int flags = MCL_CURRENT | MCL_FUTURE; -#ifdef HAVE_MLOCK_ONFAULT if (on_fault) { +#ifdef HAVE_MLOCK_ONFAULT flags |= MCL_ONFAULT; -} #else -(void)on_fault; +error_report("mlockall: on_fault not supported"); +return -EINVAL; #endif +} ret = mlockall(flags); if (ret < 0) { + +ret = mlockall(flags); if (ret < 0) { error_report("mlockall: %s", strerror(errno)); } return ret; #else +(void)on_fault; return -ENOSYS; #endif } diff --git a/system/vl.c b/system/vl.c index 9c6942c6cf..e94fc7ea35 100644 --- a/system/vl.c +++ b/system/vl.c @@ -797,7 +797,7 @@ static QemuOptsList qemu_run_with_opts = { static void realtime_init(void) { if (enable_mlock) { -if (os_mlock() < 0) { +if (os_mlock(false) < 0) { error_report("locking memory failed"); exit(1); } -- 2.34.1
Re: [PULL 04/32] hw/timer/xilinx_timer: Make device endianness configurable
On 12/2/25 10:34, Philippe Mathieu-Daudé wrote: On 12/2/25 10:19, Philippe Mathieu-Daudé wrote: On 12/2/25 09:27, Thomas Huth wrote: On 10/02/2025 21.41, Philippe Mathieu-Daudé wrote: Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness for each machine using the device. Reviewed-by: Richard Henderson Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20250206131052.30207-5-phi...@linaro.org> --- hw/microblaze/petalogix_ml605_mmu.c | 1 + hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 + hw/ppc/virtex_ml507.c | 1 + hw/timer/xilinx_timer.c | 35 ++ +- 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/ petalogix_ml605_mmu.c index cf3b9574db3..bbda70aa93b 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -127,6 +127,7 @@ petalogix_ml605_init(MachineState *machine) /* 2 timers at irq 2 @ 100 Mhz. */ dev = qdev_new("xlnx.xps-timer"); + qdev_prop_set_bit(dev, "little-endian", true); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 100 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/ microblaze/ petalogix_s3adsp1800_mmu.c index fbf52ba8f2f..9d4316b4036 100644 --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c @@ -114,6 +114,7 @@ petalogix_s3adsp1800_init(MachineState *machine) /* 2 timers at irq 2 @ 62 Mhz. */ dev = qdev_new("xlnx.xps-timer"); + qdev_prop_set_bit(dev, "little-endian", !TARGET_BIG_ENDIAN); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 62 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c index 23238119273..f87c221d076 100644 --- a/hw/ppc/virtex_ml507.c +++ b/hw/ppc/virtex_ml507.c @@ -230,6 +230,7 @@ static void virtex_init(MachineState *machine) /* 2 timers at irq 2 @ 62 Mhz. */ dev = qdev_new("xlnx.xps-timer"); + qdev_prop_set_bit(dev, "little-endian", false); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 62 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); Hi, with this patch applied, the ppc_virtex_ml507 functional test is now failing for me ... could you please double-check whether "make check- functional-ppc" still works for you? Thanks, not this patch problem, but patch #2 misses: -- >8 -- diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c index 23238119273..723f62c904b 100644 --- a/hw/ppc/virtex_ml507.c +++ b/hw/ppc/virtex_ml507.c @@ -219,2 +219,3 @@ static void virtex_init(MachineState *machine) dev = qdev_new("xlnx.xps-intc"); + qdev_prop_set_bit(dev, "little-endian", false); It looks to me like another "default value problem", where using a tri-state with default=unset would have catched. "disas/dis-asm.h" defines: enum bfd_endian { BFD_ENDIAN_BIG, BFD_ENDIAN_LITTLE, BFD_ENDIAN_UNKNOWN }; Maybe endianness is common enough than we can add a QAPI enum, in machine-common.json or even common.json? Then I'd use qdev_prop_set_enum() here. I'll post a series doing that.
Re: [PATCH 03/11] rust: Add some block layer bindings
On 2/11/25 22:43, Kevin Wolf wrote: Signed-off-by: Kevin Wolf --- rust/wrapper.h| 4 meson.build | 1 + rust/qemu-api/src/zeroable.rs | 5 +++-- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/rust/wrapper.h b/rust/wrapper.h index 41be87adcf..c3e1e6f9cf 100644 --- a/rust/wrapper.h +++ b/rust/wrapper.h @@ -53,3 +53,7 @@ typedef enum memory_order { #include "chardev/char-fe.h" #include "qapi/error.h" #include "chardev/char-serial.h" +#include "block/block.h" +#include "block/block_int.h" +#include "block/qdict.h" +#include "qapi/qapi-visit-block-core.h" diff --git a/meson.build b/meson.build index 30aae6b3c3..154195bc80 100644 --- a/meson.build +++ b/meson.build @@ -4045,6 +4045,7 @@ if have_rust '--with-derive-default', '--no-layout-tests', '--no-prepend-enum-name', +'--allowlist-item', 'EINVAL|EIO', I've got some errno bindings that I wrote for chardev, I'll send them shortly. Paolo '--allowlist-file', meson.project_source_root() + '/include/.*', '--allowlist-file', meson.project_source_root() + '/.*', '--allowlist-file', meson.project_build_root() + '/.*' diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs index b454e9e05e..60af681293 100644 --- a/rust/qemu-api/src/zeroable.rs +++ b/rust/qemu-api/src/zeroable.rs @@ -56,7 +56,6 @@ pub unsafe trait Zeroable: Default { /// ## Differences with `core::mem::zeroed` /// /// `const_zero` zeroes padding bits, while `core::mem::zeroed` doesn't -#[allow(unused)] macro_rules! const_zero { // This macro to produce a type-generic zero constant is taken from the // const_zero crate (v0.1.1): @@ -78,7 +77,6 @@ union TypeAsBytes { } /// A wrapper to implement the `Zeroable` trait through the `const_zero` macro. -#[allow(unused)] macro_rules! impl_zeroable { ($type:ty) => { unsafe impl Zeroable for $type { @@ -110,3 +108,6 @@ fn default() -> Self { impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_1); #[cfg(feature = "system")] impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_2); + +impl_zeroable!(crate::bindings::BlockDriver); +impl_zeroable!(crate::bindings::BlockDriver__bindgen_ty_1);
Re: [PATCH] cryptodev/vhost: allocate CryptoDevBackendVhost using g_mem0()
Acked-by: zhenwei pi On 2/11/25 21:55, Stefano Garzarella wrote: The function `vhost_dev_init()` expects the `struct vhost_dev` (passed as a parameter) to be fully initialized. This is important because some parts of the code check whether `vhost_dev->config_ops` is NULL to determine if it has been set (e.g. later via `vhost_dev_set_config_notifier`). To ensure this initialization, it’s better to allocate the entire `CryptoDevBackendVhost` structure (which includes `vhost_dev`) using `g_mem0()`, following the same approach used for other vhost devices, such as in `vhost_net_init()`. Fixes: 042cea274c ("cryptodev: add vhost-user as a new cryptodev backend") Cc: qemu-sta...@nongnu.org Reported-by: mylu...@m.fudan.edu.cn Signed-off-by: Stefano Garzarella --- backends/cryptodev-vhost.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c index 8718c97326..943680a23a 100644 --- a/backends/cryptodev-vhost.c +++ b/backends/cryptodev-vhost.c @@ -53,7 +53,7 @@ cryptodev_vhost_init( CryptoDevBackendVhost *crypto; Error *local_err = NULL; -crypto = g_new(CryptoDevBackendVhost, 1); +crypto = g_new0(CryptoDevBackendVhost, 1); crypto->dev.max_queues = 1; crypto->dev.nvqs = 1; crypto->dev.vqs = crypto->vqs;
Re: [PATCH] scripts: improve error from qemu-trace-stap on missing 'stap'
Hi Stefan, Are you ok with queuing this patch ? On Fri, Dec 06, 2024 at 11:45:24AM +, Daniel P. Berrangé wrote: > If the 'stap' binary is missing in $PATH, a huge trace is thrown > > $ qemu-trace-stap list /usr/bin/qemu-system-x86_64 > Traceback (most recent call last): > File "/usr/bin/qemu-trace-stap", line 169, in > main() > File "/usr/bin/qemu-trace-stap", line 165, in main > args.func(args) > File "/usr/bin/qemu-trace-stap", line 83, in cmd_run > subprocess.call(stapargs) > File "/usr/lib64/python3.12/subprocess.py", line 389, in call > with Popen(*popenargs, **kwargs) as p: > ^^^ > File "/usr/lib64/python3.12/subprocess.py", line 1026, in {}init{} > self._execute_child(args, executable, preexec_fn, close_fds, > File "/usr/lib64/python3.12/subprocess.py", line 1955, in _execute_child > raise child_exception_type(errno_num, err_msg, err_filename) > FileNotFoundError: [Errno 2] No such file or directory: 'stap' > > With this change the user now gets > > $ qemu-trace-stap list /usr/bin/qemu-system-x86_64 > Unable to find 'stap' in $PATH > > Signed-off-by: Daniel P. Berrangé > --- > scripts/qemu-trace-stap | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/scripts/qemu-trace-stap b/scripts/qemu-trace-stap > index eb6e951ff2..e983460ee7 100755 > --- a/scripts/qemu-trace-stap > +++ b/scripts/qemu-trace-stap > @@ -56,6 +56,7 @@ def tapset_dir(binary): > > > def cmd_run(args): > +stap = which("stap") > prefix = probe_prefix(args.binary) > tapsets = tapset_dir(args.binary) > > @@ -76,7 +77,7 @@ def cmd_run(args): > > # We request an 8MB buffer, since the stap default 1MB buffer > # can be easily overflowed by frequently firing QEMU traces > -stapargs = ["stap", "-s", "8", "-I", tapsets ] > +stapargs = [stap, "-s", "8", "-I", tapsets ] > if args.pid is not None: > stapargs.extend(["-x", args.pid]) > stapargs.extend(["-e", script]) > @@ -84,6 +85,7 @@ def cmd_run(args): > > > def cmd_list(args): > +stap = which("stap") > tapsets = tapset_dir(args.binary) > > if args.verbose: > @@ -96,7 +98,7 @@ def cmd_list(args): > > if verbose: > print("Listing probes with name '%s'" % script) > -proc = subprocess.Popen(["stap", "-I", tapsets, "-l", script], > +proc = subprocess.Popen([stap, "-I", tapsets, "-l", script], > stdout=subprocess.PIPE, > universal_newlines=True) > out, err = proc.communicate() > -- > 2.46.0 > 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 01/11] rust: Build separate qemu_api_tools and qemu_api_system
On 2/11/25 22:43, Kevin Wolf wrote: The existing qemu_api library can't be linked into tools because it contains a few bindings for things that only exist in the system emulator. This adds a new "system" feature to the qemu_api crate that enables the system emulator parts in it, and build the crate twice: qemu_api_tools is the core library that can be linked into tools, and qemu_api_system is the full one for the system emulator. As discussed on IRC, the issue here is ClassInitImpl<>, which has to be defined in the same crate for qemu_api::qom and qemu_api::qdev. Right now, the block layer has no use for QOM, but later it will (for secret management, for example), so splitting QOM into a separate crate does not work long term. I'll try to figure out an alternative way to do the class_init bindings. Paolo Unfortunately, since library names have to be unique in meson, this means that every user of the library now needs to specify a rust_dependency_map to make either qemu_api_tools or qemu_api_system show up as the qemu_api crate in Rust. Signed-off-by: Kevin Wolf --- rust/wrapper-system.h | 44 + rust/wrapper.h | 9 - meson.build| 11 +- rust/hw/char/pl011/meson.build | 3 +- rust/meson.build | 11 +++--- rust/qemu-api/Cargo.toml | 1 + rust/qemu-api/build.rs | 10 - rust/qemu-api/meson.build | 70 ++ rust/qemu-api/src/bindings.rs | 16 ++-- rust/qemu-api/src/lib.rs | 4 ++ rust/qemu-api/src/prelude.rs | 2 + rust/qemu-api/src/zeroable.rs | 10 + 12 files changed, 143 insertions(+), 48 deletions(-) create mode 100644 rust/wrapper-system.h diff --git a/rust/wrapper-system.h b/rust/wrapper-system.h new file mode 100644 index 00..fc6c571e6d --- /dev/null +++ b/rust/wrapper-system.h @@ -0,0 +1,44 @@ +/* + * QEMU System Emulator + * + * Copyright (c) 2024 Linaro Ltd. + * + * Authors: Manos Pitsidianakis + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + + +/* + * This header file is meant to be used as input to the `bindgen` application + * in order to generate C FFI compatible Rust bindings. + */ + +/* The system emulator has all of the bindings tools have */ +#include "wrapper.h" + +#include "system/system.h" +#include "hw/sysbus.h" +#include "exec/memory.h" +#include "hw/clock.h" +#include "hw/qdev-clock.h" +#include "hw/qdev-properties.h" +#include "hw/qdev-properties-system.h" +#include "hw/irq.h" +#include "migration/vmstate.h" diff --git a/rust/wrapper.h b/rust/wrapper.h index a9bc67af0d..41be87adcf 100644 --- a/rust/wrapper.h +++ b/rust/wrapper.h @@ -50,15 +50,6 @@ typedef enum memory_order { #include "qemu/osdep.h" #include "qemu/module.h" #include "qemu-io.h" -#include "system/system.h" -#include "hw/sysbus.h" -#include "exec/memory.h" #include "chardev/char-fe.h" -#include "hw/clock.h" -#include "hw/qdev-clock.h" -#include "hw/qdev-properties.h" -#include "hw/qdev-properties-system.h" -#include "hw/irq.h" #include "qapi/error.h" -#include "migration/vmstate.h" #include "chardev/char-serial.h" diff --git a/meson.build b/meson.build index 18cf9e2913..1f26801b69 100644 --- a/meson.build +++ b/meson.build @@ -4094,10 +4094,17 @@ if have_rust # this case you must pass the path to `clang` and `libclang` to your build # command invocation using the environment variables CLANG_PATH and # LIBCLANG_PATH - bindings_rs = rust.bindgen( + bindings_rs_tools = rust.bindgen( input: 'rust/wrapper.h', +output: 'bindings_tools.inc.rs', +include_directories: include_directories('.', 'include'), +bindgen_version: ['>=0.60.0'], +args: bindgen_args, +) + bindings_rs_system = rust.bindgen( +input: 'rust/wrapper-system.h', dependencies: common_ss.all_dependencies(), -output: 'bindings.inc.rs', +output: 'bindings_system.inc.
[PATCH v6 04/11] hw/timer/xilinx_timer: Make device endianness configurable
Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN. Add the "little-endian" property to select the device endianness, defaulting to little endian. Set the proper endianness for each machine using the device. Signed-off-by: Philippe Mathieu-Daudé --- hw/microblaze/petalogix_ml605_mmu.c | 1 + hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 + hw/ppc/virtex_ml507.c| 1 + hw/riscv/microblaze-v-generic.c | 2 ++ hw/timer/xilinx_timer.c | 43 +--- 5 files changed, 35 insertions(+), 13 deletions(-) diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c index 55398cc67d1..490640e9428 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -127,6 +127,7 @@ petalogix_ml605_init(MachineState *machine) /* 2 timers at irq 2 @ 100 Mhz. */ dev = qdev_new("xlnx.xps-timer"); +qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 100 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c index d419dc49a25..caaea222a8c 100644 --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c @@ -116,6 +116,7 @@ petalogix_s3adsp1800_init(MachineState *machine) /* 2 timers at irq 2 @ 62 Mhz. */ dev = qdev_new("xlnx.xps-timer"); +qdev_prop_set_enum(dev, "endianness", endianness); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 62 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c index df8f9644829..a01354d991d 100644 --- a/hw/ppc/virtex_ml507.c +++ b/hw/ppc/virtex_ml507.c @@ -231,6 +231,7 @@ static void virtex_init(MachineState *machine) /* 2 timers at irq 2 @ 62 Mhz. */ dev = qdev_new("xlnx.xps-timer"); +qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_BIG); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 62 * 100); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/riscv/microblaze-v-generic.c b/hw/riscv/microblaze-v-generic.c index a21fdfbe6db..3c79f5733b2 100644 --- a/hw/riscv/microblaze-v-generic.c +++ b/hw/riscv/microblaze-v-generic.c @@ -104,6 +104,7 @@ static void mb_v_generic_init(MachineState *machine) /* 2 timers at irq 0 @ 100 Mhz. */ dev = qdev_new("xlnx.xps-timer"); +qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 1); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); @@ -112,6 +113,7 @@ static void mb_v_generic_init(MachineState *machine) /* 2 timers at irq 3 @ 100 Mhz. */ dev = qdev_new("xlnx.xps-timer"); +qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE); qdev_prop_set_uint32(dev, "one-timer-only", 0); qdev_prop_set_uint32(dev, "clock-frequency", 1); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c index 6595cf5f517..8cd44fd6925 100644 --- a/hw/timer/xilinx_timer.c +++ b/hw/timer/xilinx_timer.c @@ -3,6 +3,9 @@ * * Copyright (c) 2009 Edgar E. Iglesias. * + * DS573: https://docs.amd.com/v/u/en-US/xps_timer + * LogiCORE IP XPS Timer/Counter (v1.02a) + * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal * in the Software without restriction, including without limitation the rights @@ -23,10 +26,12 @@ */ #include "qemu/osdep.h" +#include "qapi/error.h" #include "hw/sysbus.h" #include "hw/irq.h" #include "hw/ptimer.h" #include "hw/qdev-properties.h" +#include "hw/qdev-properties-system.h" #include "qemu/log.h" #include "qemu/module.h" #include "qom/object.h" @@ -69,6 +74,7 @@ struct XpsTimerState { SysBusDevice parent_obj; +EndianMode model_endianness; MemoryRegion mmio; qemu_irq irq; uint8_t one_timer_only; @@ -189,18 +195,21 @@ timer_write(void *opaque, hwaddr addr, timer_update_irq(t); } -static const MemoryRegionOps timer_ops = { -.read = timer_read, -.write = timer_write, -.endianness = DEVICE_NATIVE_ENDIAN, -.impl = { -.min_access_size = 4, -.max_access_size = 4, +static const MemoryRegionOps timer_ops[2] = { +[0 ... 1] = { +.read = timer_read, +.write = timer_write, +.impl = { +.min_access_size = 4, +.max_access_size = 4, +}, +.valid = { +
[PATCH v6 00/11] hw/microblaze: Allow running cross-endian vCPUs
Since v5: - Introduce QAPI EndianMode - Update RISCV machine while rebasing - Fixed INTC use on PPC (Thomas) - Dropped patch adding more machines (Daniel) Since v4 & v3: - Addressed Thomas review comments Since v2: - Addressed Richard's review comments Since v1: - Make device endianness configurable (Edgar) - Convert more Xilinx devices - Avoid preprocessor #if (Richard) - Add R-b tags Philippe Mathieu-Daudé (11): hw/qdev-properties-system: Introduce EndianMode QAPI enum hw/intc/xilinx_intc: Make device endianness configurable hw/net/xilinx_ethlite: Make device endianness configurable hw/timer/xilinx_timer: Make device endianness configurable hw/char/xilinx_uartlite: Make device endianness configurable hw/ssi/xilinx_spi: Make device endianness configurable tests/functional: Avoid using www.qemu-advent-calendar.org URL tests/functional: Explicit endianness of microblaze assets tests/functional: Allow microblaze tests to take a machine name argument tests/functional: Remove sleep() kludges from microblaze tests tests/functional: Have microblaze tests inherit common parent class qapi/common.json | 16 + include/hw/qdev-properties-system.h | 7 +++ hw/char/xilinx_uartlite.c | 34 +++ hw/core/qdev-properties-system.c | 11 hw/intc/xilinx_intc.c | 60 ++- hw/microblaze/petalogix_ml605_mmu.c | 3 + hw/microblaze/petalogix_s3adsp1800_mmu.c | 6 ++ hw/net/xilinx_ethlite.c | 27 +++-- hw/ppc/virtex_ml507.c | 2 + hw/riscv/microblaze-v-generic.c | 5 ++ hw/ssi/xilinx_spi.c | 32 +++--- hw/timer/xilinx_timer.c | 43 + .../functional/test_microblaze_s3adsp1800.py | 35 +-- .../test_microblazeel_s3adsp1800.py | 32 ++ 14 files changed, 229 insertions(+), 84 deletions(-) -- 2.47.1