* [PULL 01/38] rust: fix --enable-debug-mutex
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 18:45 ` [PULL 02/38] rust: add --check-cfg test to rustc arguments Paolo Bonzini
` (36 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: Bernhard Beschow
--feature is an option for cargo but not for rustc.
Reported-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Bernhard Beschow <shentey@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index ccb20f38c1d..9425ba7100c 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -7,7 +7,7 @@ if rustc.version().version_compare('>=1.77.0')
_qemu_api_cfg += ['--cfg', 'has_offset_of']
endif
if get_option('debug_mutex')
- _qemu_api_cfg += ['--feature', 'debug_cell']
+ _qemu_api_cfg += ['--cfg', 'feature="debug_cell"']
endif
_qemu_api_rs = static_library(
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 02/38] rust: add --check-cfg test to rustc arguments
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
2025-01-10 18:45 ` [PULL 01/38] rust: fix --enable-debug-mutex Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 21:31 ` Kevin Wolf
2025-01-10 18:45 ` [PULL 03/38] rust: qom: add ParentField Paolo Bonzini
` (35 subsequent siblings)
37 siblings, 1 reply; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel
rustc will check that every reachable #[cfg] matches a list of
the expected config names and values. Recent versions of rustc are
also complaining about #[cfg(test)], even if it is basically a standard
part of the language. So, always allow it.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
scripts/rust/rustc_args.py | 2 ++
1 file changed, 2 insertions(+)
diff --git a/scripts/rust/rustc_args.py b/scripts/rust/rustc_args.py
index 5525b3886fa..d79dc6d81f1 100644
--- a/scripts/rust/rustc_args.py
+++ b/scripts/rust/rustc_args.py
@@ -215,6 +215,8 @@ def main() -> None:
if rustc_version >= (1, 80):
if args.lints:
+ print("--check-cfg")
+ print("test")
for cfg in sorted(cargo_toml.check_cfg):
print("--check-cfg")
print(cfg)
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PULL 02/38] rust: add --check-cfg test to rustc arguments
2025-01-10 18:45 ` [PULL 02/38] rust: add --check-cfg test to rustc arguments Paolo Bonzini
@ 2025-01-10 21:31 ` Kevin Wolf
2025-01-12 12:15 ` Paolo Bonzini
0 siblings, 1 reply; 42+ messages in thread
From: Kevin Wolf @ 2025-01-10 21:31 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
Am 10.01.2025 um 19:45 hat Paolo Bonzini geschrieben:
> rustc will check that every reachable #[cfg] matches a list of
> the expected config names and values. Recent versions of rustc are
> also complaining about #[cfg(test)], even if it is basically a standard
> part of the language. So, always allow it.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
error: invalid `--check-cfg` argument: `test`
|
= note: expected `cfg(name, values("value1", "value2", ... "valueN"))`
= note: visit <https://doc.rust-lang.org/nightly/rustc/check-cfg.html> for more details
> scripts/rust/rustc_args.py | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/scripts/rust/rustc_args.py b/scripts/rust/rustc_args.py
> index 5525b3886fa..d79dc6d81f1 100644
> --- a/scripts/rust/rustc_args.py
> +++ b/scripts/rust/rustc_args.py
> @@ -215,6 +215,8 @@ def main() -> None:
>
> if rustc_version >= (1, 80):
> if args.lints:
> + print("--check-cfg")
> + print("test")
Should this be print("cfg(test)")?
> for cfg in sorted(cargo_toml.check_cfg):
> print("--check-cfg")
> print(cfg)
Kevin
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PULL 02/38] rust: add --check-cfg test to rustc arguments
2025-01-10 21:31 ` Kevin Wolf
@ 2025-01-12 12:15 ` Paolo Bonzini
0 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-12 12:15 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel
On 1/10/25 22:31, Kevin Wolf wrote:
> Am 10.01.2025 um 19:45 hat Paolo Bonzini geschrieben:
>> rustc will check that every reachable #[cfg] matches a list of
>> the expected config names and values. Recent versions of rustc are
>> also complaining about #[cfg(test)], even if it is basically a standard
>> part of the language. So, always allow it.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> error: invalid `--check-cfg` argument: `test`
> |
> = note: expected `cfg(name, values("value1", "value2", ... "valueN"))`
> = note: visit <https://doc.rust-lang.org/nightly/rustc/check-cfg.html> for more details
Ouch. :/ That's even in the GitLab pipeline...
I fixed the branch for now and will send the v2 pull request for the
record tomorrow.
Paolo
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PULL 03/38] rust: qom: add ParentField
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
2025-01-10 18:45 ` [PULL 01/38] rust: fix --enable-debug-mutex Paolo Bonzini
2025-01-10 18:45 ` [PULL 02/38] rust: add --check-cfg test to rustc arguments Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 18:45 ` [PULL 04/38] rust: add a utility module for compile-time type checks Paolo Bonzini
` (34 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
Add a type that, together with the C function object_deinit, ensures the
correct drop order for QOM objects relative to their superclasses.
Right now it is not possible to implement the Drop trait for QOM classes
that are defined in Rust, as the drop() function would not be called when
the object goes away; instead what is called is ObjectImpl::INSTANCE_FINALIZE.
It would be nice for INSTANCE_FINALIZE to just drop the object, but this has
a problem: suppose you have
pub struct MySuperclass {
parent: DeviceState,
field: Box<MyData>,
...
}
impl Drop for MySuperclass {
...
}
pub struct MySubclass {
parent: MySuperclass,
...
}
and an instance_finalize implementation that is like
unsafe extern "C" fn drop_object<T: ObjectImpl>(obj: *mut Object) {
unsafe { std::ptr::drop_in_place(obj.cast::<T>()) }
}
When instance_finalize is called for MySubclass, it will walk the struct's
list of fields and call the drop method for MySuperclass. Then, object_deinit
recurses to the superclass and calls the same drop method again. This
will cause double-freeing of the Box<Data>.
What's happening here is that QOM wants to control the drop order of
MySuperclass and MySubclass's fields. To do so, the parent field must
be marked ManuallyDrop<>, which is quite ugly. Instead, add a wrapper
type ParentField<> that is specific to QOM. This hides the implementation
detail of *what* is special about the ParentField, and will also be easy
to check in the #[derive(Object)] macro.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 6 +--
rust/qemu-api/src/qom.rs | 64 +++++++++++++++++++++++++++++---
rust/qemu-api/tests/tests.rs | 4 +-
3 files changed, 63 insertions(+), 11 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 18cc122951d..689202f4550 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -14,7 +14,7 @@
irq::InterruptSource,
prelude::*,
qdev::DeviceImpl,
- qom::ObjectImpl,
+ qom::{ObjectImpl, ParentField},
};
use crate::{
@@ -86,7 +86,7 @@ fn index(&self, idx: u32) -> &Self::Output {
#[derive(Debug, qemu_api_macros::Object, qemu_api_macros::offsets)]
/// PL011 Device Model in QEMU
pub struct PL011State {
- pub parent_obj: SysBusDevice,
+ pub parent_obj: ParentField<SysBusDevice>,
pub iomem: MemoryRegion,
#[doc(alias = "fr")]
pub flags: registers::Flags,
@@ -645,7 +645,7 @@ pub fn post_load(&mut self, _version_id: u32) -> Result<(), ()> {
#[derive(Debug, qemu_api_macros::Object)]
/// PL011 Luminary device model.
pub struct PL011Luminary {
- parent_obj: PL011State,
+ parent_obj: ParentField<PL011State>,
}
impl PL011Luminary {
diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index 7d5fbef1e17..40d17a92e1f 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -55,6 +55,7 @@
use std::{
ffi::CStr,
+ fmt,
ops::{Deref, DerefMut},
os::raw::c_void,
};
@@ -105,6 +106,52 @@ fn as_ref(&self) -> &$parent {
};
}
+/// This is the same as [`ManuallyDrop<T>`](std::mem::ManuallyDrop), though
+/// it hides the standard methods of `ManuallyDrop`.
+///
+/// The first field of an `ObjectType` must be of type `ParentField<T>`.
+/// (Technically, this is only necessary if there is at least one Rust
+/// superclass in the hierarchy). This is to ensure that the parent field is
+/// dropped after the subclass; this drop order is enforced by the C
+/// `object_deinit` function.
+///
+/// # Examples
+///
+/// ```ignore
+/// #[repr(C)]
+/// #[derive(qemu_api_macros::Object)]
+/// pub struct MyDevice {
+/// parent: ParentField<DeviceState>,
+/// ...
+/// }
+/// ```
+#[derive(Debug)]
+#[repr(transparent)]
+pub struct ParentField<T: ObjectType>(std::mem::ManuallyDrop<T>);
+
+impl<T: ObjectType> Deref for ParentField<T> {
+ type Target = T;
+
+ #[inline(always)]
+ fn deref(&self) -> &Self::Target {
+ &self.0
+ }
+}
+
+impl<T: ObjectType> DerefMut for ParentField<T> {
+ #[inline(always)]
+ fn deref_mut(&mut self) -> &mut Self::Target {
+ &mut self.0
+ }
+}
+
+impl<T: fmt::Display + ObjectType> fmt::Display for ParentField<T> {
+ #[inline(always)]
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
+ self.0.fmt(f)
+ }
+}
+
unsafe extern "C" fn rust_instance_init<T: ObjectImpl>(obj: *mut Object) {
// SAFETY: obj is an instance of T, since rust_instance_init<T>
// is called from QOM core as the instance_init function
@@ -151,11 +198,16 @@ fn as_ref(&self) -> &$parent {
///
/// - the struct must be `#[repr(C)]`;
///
-/// - the first field of the struct must be of the instance struct corresponding
-/// to the superclass, which is `ObjectImpl::ParentType`
+/// - the first field of the struct must be of type
+/// [`ParentField<T>`](ParentField), where `T` is the parent type
+/// [`ObjectImpl::ParentType`]
///
-/// - likewise, the first field of the `Class` must be of the class struct
-/// corresponding to the superclass, which is `ObjectImpl::ParentType::Class`.
+/// - the first field of the `Class` must be of the class struct corresponding
+/// to the superclass, which is `ObjectImpl::ParentType::Class`. `ParentField`
+/// is not needed here.
+///
+/// In both cases, having a separate class type is not necessary if the subclass
+/// does not add any field.
pub unsafe trait ObjectType: Sized {
/// The QOM class object corresponding to this struct. This is used
/// to automatically generate a `class_init` method.
@@ -384,8 +436,8 @@ impl<T: ObjectType> ObjectCastMut for &mut T {}
/// Trait a type must implement to be registered with QEMU.
pub trait ObjectImpl: ObjectType + ClassInitImpl<Self::Class> {
- /// The parent of the type. This should match the first field of
- /// the struct that implements `ObjectImpl`:
+ /// The parent of the type. This should match the first field of the
+ /// struct that implements `ObjectImpl`, minus the `ParentField<_>` wrapper.
type ParentType: ObjectType;
/// Whether the object can be instantiated
diff --git a/rust/qemu-api/tests/tests.rs b/rust/qemu-api/tests/tests.rs
index 1d2825b0986..526c3f4f8ea 100644
--- a/rust/qemu-api/tests/tests.rs
+++ b/rust/qemu-api/tests/tests.rs
@@ -15,7 +15,7 @@
declare_properties, define_property,
prelude::*,
qdev::{DeviceImpl, DeviceState, Property},
- qom::ObjectImpl,
+ qom::{ObjectImpl, ParentField},
vmstate::VMStateDescription,
zeroable::Zeroable,
};
@@ -31,7 +31,7 @@
#[repr(C)]
#[derive(qemu_api_macros::Object)]
pub struct DummyState {
- parent: DeviceState,
+ parent: ParentField<DeviceState>,
migrate_clock: bool,
}
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 04/38] rust: add a utility module for compile-time type checks
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (2 preceding siblings ...)
2025-01-10 18:45 ` [PULL 03/38] rust: qom: add ParentField Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 18:45 ` [PULL 05/38] rust: macros: check that #[derive(Object)] requires #[repr(C)] Paolo Bonzini
` (33 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
It is relatively common in the low-level qemu_api code to assert that
a field of a struct has a specific type; for example, it can be used
to ensure that the fields match what the qemu_api and C code expects
for safety.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/meson.build | 1 +
rust/qemu-api/src/assertions.rs | 90 +++++++++++++++++++++++++++++++++
rust/qemu-api/src/lib.rs | 1 +
3 files changed, 92 insertions(+)
create mode 100644 rust/qemu-api/src/assertions.rs
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 9425ba7100c..60944a657de 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -15,6 +15,7 @@ _qemu_api_rs = static_library(
structured_sources(
[
'src/lib.rs',
+ 'src/assertions.rs',
'src/bindings.rs',
'src/bitops.rs',
'src/callbacks.rs',
diff --git a/rust/qemu-api/src/assertions.rs b/rust/qemu-api/src/assertions.rs
new file mode 100644
index 00000000000..6e420469806
--- /dev/null
+++ b/rust/qemu-api/src/assertions.rs
@@ -0,0 +1,90 @@
+// Copyright 2024, Red Hat Inc.
+// Author(s): Paolo Bonzini <pbonzini@redhat.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+//! This module provides macros to check the equality of types and
+//! the type of `struct` fields. This can be useful to ensure that
+//! types match the expectations of C code.
+
+// Based on https://stackoverflow.com/questions/64251852/x/70978292#70978292
+// (stackoverflow answers are released under MIT license).
+
+#[doc(hidden)]
+pub trait EqType {
+ type Itself;
+}
+
+impl<T> EqType for T {
+ type Itself = T;
+}
+
+/// Assert that two types are the same.
+///
+/// # Examples
+///
+/// ```
+/// # use qemu_api::assert_same_type;
+/// # use std::ops::Deref;
+/// assert_same_type!(u32, u32);
+/// assert_same_type!(<Box<u32> as Deref>::Target, u32);
+/// ```
+///
+/// Different types will cause a compile failure
+///
+/// ```compile_fail
+/// # use qemu_api::assert_same_type;
+/// assert_same_type!(&Box<u32>, &u32);
+/// ```
+#[macro_export]
+macro_rules! assert_same_type {
+ ($t1:ty, $t2:ty) => {
+ const _: () = {
+ #[allow(unused)]
+ fn assert_same_type(v: $t1) {
+ fn types_must_be_equal<T, U>(_: T)
+ where
+ T: $crate::assertions::EqType<Itself = U>,
+ {
+ }
+ types_must_be_equal::<_, $t2>(v);
+ }
+ };
+ };
+}
+
+/// Assert that a field of a struct has the given type.
+///
+/// # Examples
+///
+/// ```
+/// # use qemu_api::assert_field_type;
+/// pub struct A {
+/// field1: u32,
+/// }
+///
+/// assert_field_type!(A, field1, u32);
+/// ```
+///
+/// Different types will cause a compile failure
+///
+/// ```compile_fail
+/// # use qemu_api::assert_field_type;
+/// # pub struct A { field1: u32 }
+/// assert_field_type!(A, field1, i32);
+/// ```
+#[macro_export]
+macro_rules! assert_field_type {
+ ($t:ty, $i:tt, $ti:ty) => {
+ const _: () = {
+ #[allow(unused)]
+ fn assert_field_type(v: $t) {
+ fn types_must_be_equal<T, U>(_: T)
+ where
+ T: $crate::assertions::EqType<Itself = U>,
+ {
+ }
+ types_must_be_equal::<_, $ti>(v.$i);
+ }
+ };
+ };
+}
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index 4b43e02c0f9..83c6a987c05 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -12,6 +12,7 @@
#[rustfmt::skip]
pub mod prelude;
+pub mod assertions;
pub mod bitops;
pub mod c_str;
pub mod callbacks;
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 05/38] rust: macros: check that #[derive(Object)] requires #[repr(C)]
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (3 preceding siblings ...)
2025-01-10 18:45 ` [PULL 04/38] rust: add a utility module for compile-time type checks Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 18:45 ` [PULL 06/38] rust: macros: check that the first field of a #[derive(Object)] struct is a ParentField Paolo Bonzini
` (32 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
Convert derive_object to the same pattern of first making a
Result<proc_macro2::TokenStream, CompileError>, and then doing
.unwrap_or_else(Into::into) to support checking the validity of
the input. Add is_c_repr to check that all QOM structs include
a #[repr(C)] attribute.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api-macros/src/lib.rs | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
index 74a8bc7503e..160b283d7fd 100644
--- a/rust/qemu-api-macros/src/lib.rs
+++ b/rust/qemu-api-macros/src/lib.rs
@@ -32,18 +32,23 @@ fn is_c_repr(input: &DeriveInput, msg: &str) -> Result<(), CompileError> {
}
}
-#[proc_macro_derive(Object)]
-pub fn derive_object(input: TokenStream) -> TokenStream {
- let input = parse_macro_input!(input as DeriveInput);
- let name = input.ident;
+fn derive_object_or_error(input: DeriveInput) -> Result<proc_macro2::TokenStream, CompileError> {
+ is_c_repr(&input, "#[derive(Object)]")?;
- let expanded = quote! {
+ let name = &input.ident;
+ Ok(quote! {
::qemu_api::module_init! {
MODULE_INIT_QOM => unsafe {
::qemu_api::bindings::type_register_static(&<#name as ::qemu_api::qom::ObjectImpl>::TYPE_INFO);
}
}
- };
+ })
+}
+
+#[proc_macro_derive(Object)]
+pub fn derive_object(input: TokenStream) -> TokenStream {
+ let input = parse_macro_input!(input as DeriveInput);
+ let expanded = derive_object_or_error(input).unwrap_or_else(Into::into);
TokenStream::from(expanded)
}
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 06/38] rust: macros: check that the first field of a #[derive(Object)] struct is a ParentField
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (4 preceding siblings ...)
2025-01-10 18:45 ` [PULL 05/38] rust: macros: check that #[derive(Object)] requires #[repr(C)] Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 18:45 ` [PULL 07/38] rust: qom: automatically use Drop trait to implement instance_finalize Paolo Bonzini
` (31 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api-macros/src/lib.rs | 46 +++++++++++++++++++--------------
1 file changed, 27 insertions(+), 19 deletions(-)
diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
index 160b283d7fd..0f04cca3841 100644
--- a/rust/qemu-api-macros/src/lib.rs
+++ b/rust/qemu-api-macros/src/lib.rs
@@ -19,6 +19,27 @@ fn from(err: CompileError) -> Self {
}
}
+fn get_fields<'a>(
+ input: &'a DeriveInput,
+ msg: &str,
+) -> Result<&'a Punctuated<Field, Comma>, CompileError> {
+ if let Data::Struct(s) = &input.data {
+ if let Fields::Named(fs) = &s.fields {
+ Ok(&fs.named)
+ } else {
+ Err(CompileError(
+ format!("Named fields required for {}", msg),
+ input.ident.span(),
+ ))
+ }
+ } else {
+ Err(CompileError(
+ format!("Struct required for {}", msg),
+ input.ident.span(),
+ ))
+ }
+}
+
fn is_c_repr(input: &DeriveInput, msg: &str) -> Result<(), CompileError> {
let expected = parse_quote! { #[repr(C)] };
@@ -36,7 +57,12 @@ fn derive_object_or_error(input: DeriveInput) -> Result<proc_macro2::TokenStream
is_c_repr(&input, "#[derive(Object)]")?;
let name = &input.ident;
+ let parent = &get_fields(&input, "#[derive(Object)]")?[0].ident;
+
Ok(quote! {
+ ::qemu_api::assert_field_type!(#name, #parent,
+ ::qemu_api::qom::ParentField<<#name as ::qemu_api::qom::ObjectImpl>::ParentType>);
+
::qemu_api::module_init! {
MODULE_INIT_QOM => unsafe {
::qemu_api::bindings::type_register_static(&<#name as ::qemu_api::qom::ObjectImpl>::TYPE_INFO);
@@ -53,30 +79,12 @@ pub fn derive_object(input: TokenStream) -> TokenStream {
TokenStream::from(expanded)
}
-fn get_fields(input: &DeriveInput) -> Result<&Punctuated<Field, Comma>, CompileError> {
- if let Data::Struct(s) = &input.data {
- if let Fields::Named(fs) = &s.fields {
- Ok(&fs.named)
- } else {
- Err(CompileError(
- "Cannot generate offsets for unnamed fields.".to_string(),
- input.ident.span(),
- ))
- }
- } else {
- Err(CompileError(
- "Cannot generate offsets for union or enum.".to_string(),
- input.ident.span(),
- ))
- }
-}
-
#[rustfmt::skip::macros(quote)]
fn derive_offsets_or_error(input: DeriveInput) -> Result<proc_macro2::TokenStream, CompileError> {
is_c_repr(&input, "#[derive(offsets)]")?;
let name = &input.ident;
- let fields = get_fields(&input)?;
+ let fields = get_fields(&input, "#[derive(offsets)]")?;
let field_names: Vec<&Ident> = fields.iter().map(|f| f.ident.as_ref().unwrap()).collect();
let field_types: Vec<&Type> = fields.iter().map(|f| &f.ty).collect();
let field_vis: Vec<&Visibility> = fields.iter().map(|f| &f.vis).collect();
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 07/38] rust: qom: automatically use Drop trait to implement instance_finalize
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (5 preceding siblings ...)
2025-01-10 18:45 ` [PULL 06/38] rust: macros: check that the first field of a #[derive(Object)] struct is a ParentField Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 18:45 ` [PULL 08/38] rust: qom: move device_id to PL011 class side Paolo Bonzini
` (30 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
Replace the customizable INSTANCE_FINALIZE with a generic function
that drops the Rust object.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api/src/qom.rs | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index 40d17a92e1f..b0332ba2477 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -180,6 +180,16 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
T::class_init(unsafe { &mut *klass.cast::<T::Class>() })
}
+unsafe extern "C" fn drop_object<T: ObjectImpl>(obj: *mut Object) {
+ // SAFETY: obj is an instance of T, since drop_object<T> is called
+ // from the QOM core function object_deinit() as the instance_finalize
+ // function for class T. Note that while object_deinit() will drop the
+ // superclass field separately after this function returns, `T` must
+ // implement the unsafe trait ObjectType; the safety rules for the
+ // trait mandate that the parent field is manually dropped.
+ unsafe { std::ptr::drop_in_place(obj.cast::<T>()) }
+}
+
/// Trait exposed by all structs corresponding to QOM objects.
///
/// # Safety
@@ -442,7 +452,6 @@ pub trait ObjectImpl: ObjectType + ClassInitImpl<Self::Class> {
/// Whether the object can be instantiated
const ABSTRACT: bool = false;
- const INSTANCE_FINALIZE: Option<unsafe extern "C" fn(obj: *mut Object)> = None;
/// Function that is called to initialize an object. The parent class will
/// have already been initialized so the type is only responsible for
@@ -478,7 +487,7 @@ pub trait ObjectImpl: ObjectType + ClassInitImpl<Self::Class> {
None => None,
Some(_) => Some(rust_instance_post_init::<Self>),
},
- instance_finalize: Self::INSTANCE_FINALIZE,
+ instance_finalize: Some(drop_object::<Self>),
abstract_: Self::ABSTRACT,
class_size: core::mem::size_of::<Self::Class>(),
class_init: Some(rust_class_init::<Self>),
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 08/38] rust: qom: move device_id to PL011 class side
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (6 preceding siblings ...)
2025-01-10 18:45 ` [PULL 07/38] rust: qom: automatically use Drop trait to implement instance_finalize Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 18:45 ` [PULL 09/38] rust: pl011: only leave embedded object initialization in instance_init Paolo Bonzini
` (29 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
There is no need to monkeypatch DeviceId::Luminary into the already-initialized
PL011State. Instead, now that we can define a class hierarchy, we can define
PL011Class and make device_id a field in there.
There is also no need anymore to have "Arm" as zero, so change DeviceId into a
wrapper for the array; all it does is provide an Index<hwaddr> implementation
because arrays can only be indexed by usize.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 59 +++++++++++++++-----------------
1 file changed, 28 insertions(+), 31 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 689202f4550..215f94a6e4a 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -5,7 +5,7 @@
use core::ptr::{addr_of_mut, NonNull};
use std::{
ffi::CStr,
- os::raw::{c_int, c_uchar, c_uint, c_void},
+ os::raw::{c_int, c_uint, c_void},
};
use qemu_api::{
@@ -14,7 +14,7 @@
irq::InterruptSource,
prelude::*,
qdev::DeviceImpl,
- qom::{ObjectImpl, ParentField},
+ qom::{ClassInitImpl, ObjectImpl, ParentField},
};
use crate::{
@@ -33,27 +33,20 @@
/// QEMU sourced constant.
pub const PL011_FIFO_DEPTH: u32 = 16;
-#[derive(Clone, Copy, Debug)]
-enum DeviceId {
- #[allow(dead_code)]
- Arm = 0,
- Luminary,
-}
+#[derive(Clone, Copy)]
+struct DeviceId(&'static [u8; 8]);
impl std::ops::Index<hwaddr> for DeviceId {
- type Output = c_uchar;
+ type Output = u8;
fn index(&self, idx: hwaddr) -> &Self::Output {
- match self {
- Self::Arm => &Self::PL011_ID_ARM[idx as usize],
- Self::Luminary => &Self::PL011_ID_LUMINARY[idx as usize],
- }
+ &self.0[idx as usize]
}
}
impl DeviceId {
- const PL011_ID_ARM: [c_uchar; 8] = [0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1];
- const PL011_ID_LUMINARY: [c_uchar; 8] = [0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1];
+ const ARM: Self = Self(&[0x11, 0x10, 0x14, 0x00, 0x0d, 0xf0, 0x05, 0xb1]);
+ const LUMINARY: Self = Self(&[0x11, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1]);
}
// FIFOs use 32-bit indices instead of usize, for compatibility with
@@ -126,17 +119,28 @@ pub struct PL011State {
pub clock: NonNull<Clock>,
#[doc(alias = "migrate_clk")]
pub migrate_clock: bool,
- /// The byte string that identifies the device.
- device_id: DeviceId,
}
qom_isa!(PL011State : SysBusDevice, DeviceState, Object);
+pub struct PL011Class {
+ parent_class: <SysBusDevice as ObjectType>::Class,
+ /// The byte string that identifies the device.
+ device_id: DeviceId,
+}
+
unsafe impl ObjectType for PL011State {
- type Class = <SysBusDevice as ObjectType>::Class;
+ type Class = PL011Class;
const TYPE_NAME: &'static CStr = crate::TYPE_PL011;
}
+impl ClassInitImpl<PL011Class> for PL011State {
+ fn class_init(klass: &mut PL011Class) {
+ klass.device_id = DeviceId::ARM;
+ <Self as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut klass.parent_class);
+ }
+}
+
impl ObjectImpl for PL011State {
type ParentType = SysBusDevice;
@@ -214,7 +218,8 @@ pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> std::ops::ControlFlow<u
let value = match RegisterOffset::try_from(offset) {
Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
- u32::from(self.device_id[(offset - 0xfe0) >> 2])
+ let device_id = self.get_class().device_id;
+ u32::from(device_id[(offset - 0xfe0) >> 2])
}
Err(_) => {
// qemu_log_mask(LOG_GUEST_ERROR, "pl011_read: Bad offset 0x%x\n", (int)offset);
@@ -648,16 +653,10 @@ pub struct PL011Luminary {
parent_obj: ParentField<PL011State>,
}
-impl PL011Luminary {
- /// Initializes a pre-allocated, unitialized instance of `PL011Luminary`.
- ///
- /// # Safety
- ///
- /// We expect the FFI user of this function to pass a valid pointer, that
- /// has the same size as [`PL011Luminary`]. We also expect the device is
- /// readable/writeable from one thread at any time.
- unsafe fn init(&mut self) {
- self.parent_obj.device_id = DeviceId::Luminary;
+impl ClassInitImpl<PL011Class> for PL011Luminary {
+ fn class_init(klass: &mut PL011Class) {
+ klass.device_id = DeviceId::LUMINARY;
+ <Self as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut klass.parent_class);
}
}
@@ -670,8 +669,6 @@ unsafe impl ObjectType for PL011Luminary {
impl ObjectImpl for PL011Luminary {
type ParentType = PL011State;
-
- const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
}
impl DeviceImpl for PL011Luminary {}
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 09/38] rust: pl011: only leave embedded object initialization in instance_init
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (7 preceding siblings ...)
2025-01-10 18:45 ` [PULL 08/38] rust: qom: move device_id to PL011 class side Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 18:45 ` [PULL 10/38] rust: qom: make INSTANCE_POST_INIT take a shared reference Paolo Bonzini
` (28 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
Leave IRQ and MMIO initialization to instance_post_init. In Rust the
two callbacks are more distinct, because only instance_post_init has a
fully initialized object available.
While at it, add a wrapper for sysbus_init_mmio so that accesses to
the SysBusDevice correctly use shared references.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 18 ++++++++++--------
rust/qemu-api/src/sysbus.rs | 12 ++++++++++++
2 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 215f94a6e4a..72a4cea042c 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -145,6 +145,7 @@ impl ObjectImpl for PL011State {
type ParentType = SysBusDevice;
const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
+ const INSTANCE_POST_INIT: Option<fn(&mut Self)> = Some(Self::post_init);
}
impl DeviceImpl for PL011State {
@@ -183,14 +184,6 @@ unsafe fn init(&mut self) {
Self::TYPE_NAME.as_ptr(),
0x1000,
);
-
- let sbd: &mut SysBusDevice = self.upcast_mut();
- sysbus_init_mmio(sbd, addr_of_mut!(self.iomem));
- }
-
- for irq in self.interrupts.iter() {
- let sbd: &SysBusDevice = self.upcast();
- sbd.init_irq(irq);
}
// SAFETY:
@@ -213,6 +206,15 @@ unsafe fn init(&mut self) {
}
}
+ fn post_init(&mut self) {
+ let sbd: &SysBusDevice = self.upcast();
+
+ sbd.init_mmio(&self.iomem);
+ for irq in self.interrupts.iter() {
+ sbd.init_irq(irq);
+ }
+ }
+
pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> std::ops::ControlFlow<u64, u64> {
use RegisterOffset::*;
diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs
index 8193734bde4..b96eaaf25f2 100644
--- a/rust/qemu-api/src/sysbus.rs
+++ b/rust/qemu-api/src/sysbus.rs
@@ -38,6 +38,18 @@ const fn as_mut_ptr(&self) -> *mut SysBusDevice {
addr_of!(*self) as *mut _
}
+ /// Expose a memory region to the board so that it can give it an address
+ /// in guest memory. Note that the ordering of calls to `init_mmio` is
+ /// important, since whoever creates the sysbus device will refer to the
+ /// region with a number that corresponds to the order of calls to
+ /// `init_mmio`.
+ pub fn init_mmio(&self, iomem: &bindings::MemoryRegion) {
+ assert!(bql_locked());
+ unsafe {
+ bindings::sysbus_init_mmio(self.as_mut_ptr(), addr_of!(*iomem) as *mut _);
+ }
+ }
+
/// Expose an interrupt source outside the device as a qdev GPIO output.
/// Note that the ordering of calls to `init_irq` is important, since
/// whoever creates the sysbus device will refer to the interrupts with
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 10/38] rust: qom: make INSTANCE_POST_INIT take a shared reference
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (8 preceding siblings ...)
2025-01-10 18:45 ` [PULL 09/38] rust: pl011: only leave embedded object initialization in instance_init Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 18:45 ` [PULL 11/38] rust: qemu-api-macros: extend error reporting facility to parse errors Paolo Bonzini
` (27 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 4 ++--
rust/qemu-api/src/qom.rs | 8 ++------
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 72a4cea042c..6792d13fb77 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -145,7 +145,7 @@ impl ObjectImpl for PL011State {
type ParentType = SysBusDevice;
const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
- const INSTANCE_POST_INIT: Option<fn(&mut Self)> = Some(Self::post_init);
+ const INSTANCE_POST_INIT: Option<fn(&Self)> = Some(Self::post_init);
}
impl DeviceImpl for PL011State {
@@ -206,7 +206,7 @@ unsafe fn init(&mut self) {
}
}
- fn post_init(&mut self) {
+ fn post_init(&self) {
let sbd: &SysBusDevice = self.upcast();
sbd.init_mmio(&self.iomem);
diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs
index b0332ba2477..97901fb9084 100644
--- a/rust/qemu-api/src/qom.rs
+++ b/rust/qemu-api/src/qom.rs
@@ -163,11 +163,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
// SAFETY: obj is an instance of T, since rust_instance_post_init<T>
// is called from QOM core as the instance_post_init function
// for class T
- //
- // FIXME: it's not really guaranteed that there are no backpointers to
- // obj; it's quite possible that they have been created by instance_init().
- // The receiver should be &self, not &mut self.
- T::INSTANCE_POST_INIT.unwrap()(unsafe { &mut *obj.cast::<T>() })
+ T::INSTANCE_POST_INIT.unwrap()(unsafe { &*obj.cast::<T>() })
}
unsafe extern "C" fn rust_class_init<T: ObjectType + ClassInitImpl<T::Class>>(
@@ -463,7 +459,7 @@ pub trait ObjectImpl: ObjectType + ClassInitImpl<Self::Class> {
/// Function that is called to finish initialization of an object, once
/// `INSTANCE_INIT` functions have been called.
- const INSTANCE_POST_INIT: Option<fn(&mut Self)> = None;
+ const INSTANCE_POST_INIT: Option<fn(&Self)> = None;
/// Called on descendent classes after all parent class initialization
/// has occurred, but before the class itself is initialized. This
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 11/38] rust: qemu-api-macros: extend error reporting facility to parse errors
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (9 preceding siblings ...)
2025-01-10 18:45 ` [PULL 10/38] rust: qom: make INSTANCE_POST_INIT take a shared reference Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 18:45 ` [PULL 12/38] rust: qemu-api-macros: add automatic TryFrom/TryInto derivation Paolo Bonzini
` (26 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
Generalize the CompileError tuple to an enum, that can be either an error
message or a parse error from syn.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/qemu-api-macros/src/lib.rs | 27 ++++++++++-----------------
rust/qemu-api-macros/src/utils.rs | 26 ++++++++++++++++++++++++++
2 files changed, 36 insertions(+), 17 deletions(-)
create mode 100644 rust/qemu-api-macros/src/utils.rs
diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
index 0f04cca3841..539c48df298 100644
--- a/rust/qemu-api-macros/src/lib.rs
+++ b/rust/qemu-api-macros/src/lib.rs
@@ -3,57 +3,50 @@
// SPDX-License-Identifier: GPL-2.0-or-later
use proc_macro::TokenStream;
-use proc_macro2::Span;
-use quote::{quote, quote_spanned};
+use quote::quote;
use syn::{
parse_macro_input, parse_quote, punctuated::Punctuated, token::Comma, Data, DeriveInput, Field,
Fields, Ident, Type, Visibility,
};
-struct CompileError(String, Span);
-
-impl From<CompileError> for proc_macro2::TokenStream {
- fn from(err: CompileError) -> Self {
- let CompileError(msg, span) = err;
- quote_spanned! { span => compile_error!(#msg); }
- }
-}
+mod utils;
+use utils::MacroError;
fn get_fields<'a>(
input: &'a DeriveInput,
msg: &str,
-) -> Result<&'a Punctuated<Field, Comma>, CompileError> {
+) -> Result<&'a Punctuated<Field, Comma>, MacroError> {
if let Data::Struct(s) = &input.data {
if let Fields::Named(fs) = &s.fields {
Ok(&fs.named)
} else {
- Err(CompileError(
+ Err(MacroError::Message(
format!("Named fields required for {}", msg),
input.ident.span(),
))
}
} else {
- Err(CompileError(
+ Err(MacroError::Message(
format!("Struct required for {}", msg),
input.ident.span(),
))
}
}
-fn is_c_repr(input: &DeriveInput, msg: &str) -> Result<(), CompileError> {
+fn is_c_repr(input: &DeriveInput, msg: &str) -> Result<(), MacroError> {
let expected = parse_quote! { #[repr(C)] };
if input.attrs.iter().any(|attr| attr == &expected) {
Ok(())
} else {
- Err(CompileError(
+ Err(MacroError::Message(
format!("#[repr(C)] required for {}", msg),
input.ident.span(),
))
}
}
-fn derive_object_or_error(input: DeriveInput) -> Result<proc_macro2::TokenStream, CompileError> {
+fn derive_object_or_error(input: DeriveInput) -> Result<proc_macro2::TokenStream, MacroError> {
is_c_repr(&input, "#[derive(Object)]")?;
let name = &input.ident;
@@ -80,7 +73,7 @@ pub fn derive_object(input: TokenStream) -> TokenStream {
}
#[rustfmt::skip::macros(quote)]
-fn derive_offsets_or_error(input: DeriveInput) -> Result<proc_macro2::TokenStream, CompileError> {
+fn derive_offsets_or_error(input: DeriveInput) -> Result<proc_macro2::TokenStream, MacroError> {
is_c_repr(&input, "#[derive(offsets)]")?;
let name = &input.ident;
diff --git a/rust/qemu-api-macros/src/utils.rs b/rust/qemu-api-macros/src/utils.rs
new file mode 100644
index 00000000000..02c91aed7f6
--- /dev/null
+++ b/rust/qemu-api-macros/src/utils.rs
@@ -0,0 +1,26 @@
+// Procedural macro utilities.
+// Author(s): Paolo Bonzini <pbonzini@redhat.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+use proc_macro2::Span;
+use quote::quote_spanned;
+
+pub enum MacroError {
+ Message(String, Span),
+ ParseError(syn::Error),
+}
+
+impl From<syn::Error> for MacroError {
+ fn from(err: syn::Error) -> Self {
+ MacroError::ParseError(err)
+ }
+}
+
+impl From<MacroError> for proc_macro2::TokenStream {
+ fn from(err: MacroError) -> Self {
+ match err {
+ MacroError::Message(msg, span) => quote_spanned! { span => compile_error!(#msg); },
+ MacroError::ParseError(err) => err.into_compile_error(),
+ }
+ }
+}
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 12/38] rust: qemu-api-macros: add automatic TryFrom/TryInto derivation
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (10 preceding siblings ...)
2025-01-10 18:45 ` [PULL 11/38] rust: qemu-api-macros: extend error reporting facility to parse errors Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 18:45 ` [PULL 13/38] rust: qdev: expose inherited methods to subclasses of SysBusDevice Paolo Bonzini
` (25 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
This is going to be fairly common. Using a custom procedural macro
provides better error messages and automatically finds the right
type.
Note that this is different from the same-named macro in the
derive_more crate. That one provides conversion from e.g. tuples
to enums with tuple variants, not from integers to enums.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/lib.rs | 28 +------------
rust/qemu-api-macros/src/lib.rs | 74 ++++++++++++++++++++++++++++++++-
2 files changed, 73 insertions(+), 29 deletions(-)
diff --git a/rust/hw/char/pl011/src/lib.rs b/rust/hw/char/pl011/src/lib.rs
index 69064d6929b..0a89d393e0f 100644
--- a/rust/hw/char/pl011/src/lib.rs
+++ b/rust/hw/char/pl011/src/lib.rs
@@ -45,7 +45,7 @@
#[doc(alias = "offset")]
#[allow(non_camel_case_types)]
#[repr(u64)]
-#[derive(Debug)]
+#[derive(Debug, qemu_api_macros::TryInto)]
pub enum RegisterOffset {
/// Data Register
///
@@ -102,32 +102,6 @@ pub enum RegisterOffset {
//Reserved = 0x04C,
}
-impl core::convert::TryFrom<u64> for RegisterOffset {
- type Error = u64;
-
- fn try_from(value: u64) -> Result<Self, Self::Error> {
- macro_rules! case {
- ($($discriminant:ident),*$(,)*) => {
- /* check that matching on all macro arguments compiles, which means we are not
- * missing any enum value; if the type definition ever changes this will stop
- * compiling.
- */
- const fn _assert_exhaustive(val: RegisterOffset) {
- match val {
- $(RegisterOffset::$discriminant => (),)*
- }
- }
-
- match value {
- $(x if x == Self::$discriminant as u64 => Ok(Self::$discriminant),)*
- _ => Err(value),
- }
- }
- }
- case! { DR, RSR, FR, FBRD, ILPR, IBRD, LCR_H, CR, FLS, IMSC, RIS, MIS, ICR, DMACR }
- }
-}
-
pub mod registers {
//! Device registers exposed as typed structs which are backed by arbitrary
//! integer bitmaps. [`Data`], [`Control`], [`LineControl`], etc.
diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
index 539c48df298..7ec218202f4 100644
--- a/rust/qemu-api-macros/src/lib.rs
+++ b/rust/qemu-api-macros/src/lib.rs
@@ -5,8 +5,8 @@
use proc_macro::TokenStream;
use quote::quote;
use syn::{
- parse_macro_input, parse_quote, punctuated::Punctuated, token::Comma, Data, DeriveInput, Field,
- Fields, Ident, Type, Visibility,
+ parse_macro_input, parse_quote, punctuated::Punctuated, spanned::Spanned, token::Comma, Data,
+ DeriveInput, Field, Fields, Ident, Meta, Path, Token, Type, Variant, Visibility,
};
mod utils;
@@ -98,3 +98,73 @@ pub fn derive_offsets(input: TokenStream) -> TokenStream {
TokenStream::from(expanded)
}
+
+#[allow(non_snake_case)]
+fn get_repr_uN(input: &DeriveInput, msg: &str) -> Result<Path, MacroError> {
+ let repr = input.attrs.iter().find(|attr| attr.path().is_ident("repr"));
+ if let Some(repr) = repr {
+ let nested = repr.parse_args_with(Punctuated::<Meta, Token![,]>::parse_terminated)?;
+ for meta in nested {
+ match meta {
+ Meta::Path(path) if path.is_ident("u8") => return Ok(path),
+ Meta::Path(path) if path.is_ident("u16") => return Ok(path),
+ Meta::Path(path) if path.is_ident("u32") => return Ok(path),
+ Meta::Path(path) if path.is_ident("u64") => return Ok(path),
+ _ => {}
+ }
+ }
+ }
+
+ Err(MacroError::Message(
+ format!("#[repr(u8/u16/u32/u64) required for {}", msg),
+ input.ident.span(),
+ ))
+}
+
+fn get_variants(input: &DeriveInput) -> Result<&Punctuated<Variant, Comma>, MacroError> {
+ if let Data::Enum(e) = &input.data {
+ if let Some(v) = e.variants.iter().find(|v| v.fields != Fields::Unit) {
+ return Err(MacroError::Message(
+ "Cannot derive TryInto for enum with non-unit variants.".to_string(),
+ v.fields.span(),
+ ));
+ }
+ Ok(&e.variants)
+ } else {
+ Err(MacroError::Message(
+ "Cannot derive TryInto for union or struct.".to_string(),
+ input.ident.span(),
+ ))
+ }
+}
+
+#[rustfmt::skip::macros(quote)]
+fn derive_tryinto_or_error(input: DeriveInput) -> Result<proc_macro2::TokenStream, MacroError> {
+ let repr = get_repr_uN(&input, "#[derive(TryInto)]")?;
+
+ let name = &input.ident;
+ let variants = get_variants(&input)?;
+ let discriminants: Vec<&Ident> = variants.iter().map(|f| &f.ident).collect();
+
+ Ok(quote! {
+ impl core::convert::TryFrom<#repr> for #name {
+ type Error = #repr;
+
+ fn try_from(value: #repr) -> Result<Self, Self::Error> {
+ #(const #discriminants: #repr = #name::#discriminants as #repr;)*;
+ match value {
+ #(#discriminants => Ok(Self::#discriminants),)*
+ _ => Err(value),
+ }
+ }
+ }
+ })
+}
+
+#[proc_macro_derive(TryInto)]
+pub fn derive_tryinto(input: TokenStream) -> TokenStream {
+ let input = parse_macro_input!(input as DeriveInput);
+ let expanded = derive_tryinto_or_error(input).unwrap_or_else(Into::into);
+
+ TokenStream::from(expanded)
+}
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 13/38] rust: qdev: expose inherited methods to subclasses of SysBusDevice
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (11 preceding siblings ...)
2025-01-10 18:45 ` [PULL 12/38] rust: qemu-api-macros: add automatic TryFrom/TryInto derivation Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 18:45 ` [PULL 14/38] rust: hide warnings for subprojects Paolo Bonzini
` (24 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
The ObjectDeref trait now provides all the magic that is required to fake
inheritance. Replace the "impl SysBusDevice" block of qemu_api::sysbus
with a trait, so that sysbus_init_irq() can be invoked as "self.init_irq()"
without any intermediate upcast.
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
rust/hw/char/pl011/src/device.rs | 6 ++----
rust/qemu-api/src/irq.rs | 3 +--
rust/qemu-api/src/prelude.rs | 2 ++
rust/qemu-api/src/sysbus.rs | 17 +++++++++--------
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 6792d13fb77..994c2fc0593 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -207,11 +207,9 @@ unsafe fn init(&mut self) {
}
fn post_init(&self) {
- let sbd: &SysBusDevice = self.upcast();
-
- sbd.init_mmio(&self.iomem);
+ self.init_mmio(&self.iomem);
for irq in self.interrupts.iter() {
- sbd.init_irq(irq);
+ self.init_irq(irq);
}
}
diff --git a/rust/qemu-api/src/irq.rs b/rust/qemu-api/src/irq.rs
index 6258141bdf0..378e5202951 100644
--- a/rust/qemu-api/src/irq.rs
+++ b/rust/qemu-api/src/irq.rs
@@ -24,8 +24,7 @@
///
/// Interrupts are implemented as a pointer to the interrupt "sink", which has
/// type [`IRQState`]. A device exposes its source as a QOM link property using
-/// a function such as
-/// [`SysBusDevice::init_irq`](crate::sysbus::SysBusDevice::init_irq), and
+/// a function such as [`SysBusDeviceMethods::init_irq`], and
/// initially leaves the pointer to a NULL value, representing an unconnected
/// interrupt. To connect it, whoever creates the device fills the pointer with
/// the sink's `IRQState *`, for example using `sysbus_connect_irq`. Because
diff --git a/rust/qemu-api/src/prelude.rs b/rust/qemu-api/src/prelude.rs
index 6f32deeb2ed..4ea70b9c823 100644
--- a/rust/qemu-api/src/prelude.rs
+++ b/rust/qemu-api/src/prelude.rs
@@ -16,3 +16,5 @@
pub use crate::qom::ObjectType;
pub use crate::qom_isa;
+
+pub use crate::sysbus::SysBusDeviceMethods;
diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs
index b96eaaf25f2..e6762b5c145 100644
--- a/rust/qemu-api/src/sysbus.rs
+++ b/rust/qemu-api/src/sysbus.rs
@@ -32,18 +32,17 @@ fn class_init(sdc: &mut SysBusDeviceClass) {
}
}
-impl SysBusDevice {
- /// Return `self` cast to a mutable pointer, for use in calls to C code.
- const fn as_mut_ptr(&self) -> *mut SysBusDevice {
- addr_of!(*self) as *mut _
- }
-
+/// Trait for methods of [`SysBusDevice`] and its subclasses.
+pub trait SysBusDeviceMethods: ObjectDeref
+where
+ Self::Target: IsA<SysBusDevice>,
+{
/// Expose a memory region to the board so that it can give it an address
/// in guest memory. Note that the ordering of calls to `init_mmio` is
/// important, since whoever creates the sysbus device will refer to the
/// region with a number that corresponds to the order of calls to
/// `init_mmio`.
- pub fn init_mmio(&self, iomem: &bindings::MemoryRegion) {
+ fn init_mmio(&self, iomem: &bindings::MemoryRegion) {
assert!(bql_locked());
unsafe {
bindings::sysbus_init_mmio(self.as_mut_ptr(), addr_of!(*iomem) as *mut _);
@@ -54,10 +53,12 @@ pub fn init_mmio(&self, iomem: &bindings::MemoryRegion) {
/// Note that the ordering of calls to `init_irq` is important, since
/// whoever creates the sysbus device will refer to the interrupts with
/// a number that corresponds to the order of calls to `init_irq`.
- pub fn init_irq(&self, irq: &InterruptSource) {
+ fn init_irq(&self, irq: &InterruptSource) {
assert!(bql_locked());
unsafe {
bindings::sysbus_init_irq(self.as_mut_ptr(), irq.as_ptr());
}
}
}
+
+impl<R: ObjectDeref> SysBusDeviceMethods for R where R::Target: IsA<SysBusDevice> {}
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 14/38] rust: hide warnings for subprojects
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (12 preceding siblings ...)
2025-01-10 18:45 ` [PULL 13/38] rust: qdev: expose inherited methods to subclasses of SysBusDevice Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 18:45 ` [PULL 15/38] qom: remove unused field Paolo Bonzini
` (23 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel
This matches cargo's own usage of "--cap-lints allow" when building
dependencies. The dummy changes to the .wrap files help Meson notice
that the subproject is out of date.
Also remove an unnecessary subprojects/unicode-ident-1-rs/meson.build file.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
subprojects/arbitrary-int-1-rs.wrap | 3 +++
subprojects/bilge-0.2-rs.wrap | 3 +++
subprojects/bilge-impl-0.2-rs.wrap | 3 +++
subprojects/either-1-rs.wrap | 3 +++
subprojects/itertools-0.11-rs.wrap | 3 +++
.../arbitrary-int-1-rs/meson.build | 1 +
.../packagefiles/bilge-0.2-rs/meson.build | 1 +
.../bilge-impl-0.2-rs/meson.build | 1 +
.../packagefiles/either-1-rs/meson.build | 1 +
.../itertools-0.11-rs/meson.build | 1 +
.../proc-macro-error-1-rs/meson.build | 1 +
.../proc-macro-error-attr-1-rs/meson.build | 1 +
.../packagefiles/proc-macro2-1-rs/meson.build | 1 +
.../packagefiles/quote-1-rs/meson.build | 1 +
subprojects/packagefiles/syn-2-rs/meson.build | 1 +
.../unicode-ident-1-rs/meson.build | 1 +
subprojects/proc-macro-error-1-rs.wrap | 3 +++
subprojects/proc-macro-error-attr-1-rs.wrap | 3 +++
subprojects/proc-macro2-1-rs.wrap | 3 +++
subprojects/quote-1-rs.wrap | 3 +++
subprojects/syn-2-rs.wrap | 3 +++
subprojects/unicode-ident-1-rs.wrap | 3 +++
subprojects/unicode-ident-1-rs/meson.build | 20 -------------------
23 files changed, 44 insertions(+), 20 deletions(-)
delete mode 100644 subprojects/unicode-ident-1-rs/meson.build
diff --git a/subprojects/arbitrary-int-1-rs.wrap b/subprojects/arbitrary-int-1-rs.wrap
index e580538a877..a1838b20b0f 100644
--- a/subprojects/arbitrary-int-1-rs.wrap
+++ b/subprojects/arbitrary-int-1-rs.wrap
@@ -5,3 +5,6 @@ source_filename = arbitrary-int-1.2.7.tar.gz
source_hash = c84fc003e338a6f69fbd4f7fe9f92b535ff13e9af8997f3b14b6ddff8b1df46d
#method = cargo
patch_directory = arbitrary-int-1-rs
+
+# bump this version number on every change to meson.build or the patches:
+# v2
diff --git a/subprojects/bilge-0.2-rs.wrap b/subprojects/bilge-0.2-rs.wrap
index 7a4339d2989..900bb1497b9 100644
--- a/subprojects/bilge-0.2-rs.wrap
+++ b/subprojects/bilge-0.2-rs.wrap
@@ -5,3 +5,6 @@ source_filename = bilge-0.2.0.tar.gz
source_hash = dc707ed8ebf81de5cd6c7f48f54b4c8621760926cdf35a57000747c512e67b57
#method = cargo
patch_directory = bilge-0.2-rs
+
+# bump this version number on every change to meson.build or the patches:
+# v2
diff --git a/subprojects/bilge-impl-0.2-rs.wrap b/subprojects/bilge-impl-0.2-rs.wrap
index b24c34a9043..d14c3dc769b 100644
--- a/subprojects/bilge-impl-0.2-rs.wrap
+++ b/subprojects/bilge-impl-0.2-rs.wrap
@@ -6,3 +6,6 @@ source_hash = feb11e002038ad243af39c2068c8a72bcf147acf05025dcdb916fcc000adb2d8
#method = cargo
patch_directory = bilge-impl-0.2-rs
diff_files = bilge-impl-1.63.0.patch
+
+# bump this version number on every change to meson.build or the patches:
+# v2
diff --git a/subprojects/either-1-rs.wrap b/subprojects/either-1-rs.wrap
index 6046712036c..352e11cfee6 100644
--- a/subprojects/either-1-rs.wrap
+++ b/subprojects/either-1-rs.wrap
@@ -5,3 +5,6 @@ source_filename = either-1.12.0.tar.gz
source_hash = 3dca9240753cf90908d7e4aac30f630662b02aebaa1b58a3cadabdb23385b58b
#method = cargo
patch_directory = either-1-rs
+
+# bump this version number on every change to meson.build or the patches:
+# v2
diff --git a/subprojects/itertools-0.11-rs.wrap b/subprojects/itertools-0.11-rs.wrap
index 66b05252cd5..ee12d0053bc 100644
--- a/subprojects/itertools-0.11-rs.wrap
+++ b/subprojects/itertools-0.11-rs.wrap
@@ -5,3 +5,6 @@ source_filename = itertools-0.11.0.tar.gz
source_hash = b1c173a5686ce8bfa551b3563d0c2170bf24ca44da99c7ca4bfdab5418c3fe57
#method = cargo
patch_directory = itertools-0.11-rs
+
+# bump this version number on every change to meson.build or the patches:
+# v2
diff --git a/subprojects/packagefiles/arbitrary-int-1-rs/meson.build b/subprojects/packagefiles/arbitrary-int-1-rs/meson.build
index cff3f62ce74..00733d1faab 100644
--- a/subprojects/packagefiles/arbitrary-int-1-rs/meson.build
+++ b/subprojects/packagefiles/arbitrary-int-1-rs/meson.build
@@ -9,6 +9,7 @@ _arbitrary_int_rs = static_library(
files('src/lib.rs'),
gnu_symbol_visibility: 'hidden',
override_options: ['rust_std=2021', 'build.rust_std=2021'],
+ rust_args: ['--cap-lints', 'allow'],
rust_abi: 'rust',
dependencies: [],
)
diff --git a/subprojects/packagefiles/bilge-0.2-rs/meson.build b/subprojects/packagefiles/bilge-0.2-rs/meson.build
index e69bac91b40..ce13d0fe80f 100644
--- a/subprojects/packagefiles/bilge-0.2-rs/meson.build
+++ b/subprojects/packagefiles/bilge-0.2-rs/meson.build
@@ -17,6 +17,7 @@ lib = static_library(
'src/lib.rs',
override_options : ['rust_std=2021', 'build.rust_std=2021'],
rust_abi : 'rust',
+ rust_args: ['--cap-lints', 'allow'],
dependencies: [
arbitrary_int_dep,
bilge_impl_dep,
diff --git a/subprojects/packagefiles/bilge-impl-0.2-rs/meson.build b/subprojects/packagefiles/bilge-impl-0.2-rs/meson.build
index f8f3486fc08..42b03dcd53c 100644
--- a/subprojects/packagefiles/bilge-impl-0.2-rs/meson.build
+++ b/subprojects/packagefiles/bilge-impl-0.2-rs/meson.build
@@ -25,6 +25,7 @@ _bilge_impl_rs = rust.proc_macro(
files('src/lib.rs'),
override_options: ['rust_std=2021', 'build.rust_std=2021'],
rust_args: [
+ '--cap-lints', 'allow',
'--cfg', 'use_fallback',
'--cfg', 'feature="syn-error"',
'--cfg', 'feature="proc-macro"',
diff --git a/subprojects/packagefiles/either-1-rs/meson.build b/subprojects/packagefiles/either-1-rs/meson.build
index 608e64e31fc..04c96cc5fb3 100644
--- a/subprojects/packagefiles/either-1-rs/meson.build
+++ b/subprojects/packagefiles/either-1-rs/meson.build
@@ -11,6 +11,7 @@ _either_rs = static_library(
override_options: ['rust_std=2018', 'build.rust_std=2018'],
rust_abi: 'rust',
rust_args: [
+ '--cap-lints', 'allow',
'--cfg', 'feature="use_std"',
'--cfg', 'feature="use_alloc"',
],
diff --git a/subprojects/packagefiles/itertools-0.11-rs/meson.build b/subprojects/packagefiles/itertools-0.11-rs/meson.build
index 30982a4ee76..2a3fbe9ee5a 100644
--- a/subprojects/packagefiles/itertools-0.11-rs/meson.build
+++ b/subprojects/packagefiles/itertools-0.11-rs/meson.build
@@ -15,6 +15,7 @@ _itertools_rs = static_library(
override_options: ['rust_std=2018', 'build.rust_std=2018'],
rust_abi: 'rust',
rust_args: [
+ '--cap-lints', 'allow',
'--cfg', 'feature="use_std"',
'--cfg', 'feature="use_alloc"',
],
diff --git a/subprojects/packagefiles/proc-macro-error-1-rs/meson.build b/subprojects/packagefiles/proc-macro-error-1-rs/meson.build
index ae27a696862..10c2741085c 100644
--- a/subprojects/packagefiles/proc-macro-error-1-rs/meson.build
+++ b/subprojects/packagefiles/proc-macro-error-1-rs/meson.build
@@ -20,6 +20,7 @@ _proc_macro_error_rs = static_library(
override_options: ['rust_std=2018', 'build.rust_std=2018'],
rust_abi: 'rust',
rust_args: [
+ '--cap-lints', 'allow',
'--cfg', 'use_fallback',
'--cfg', 'feature="syn-error"',
'--cfg', 'feature="proc-macro"',
diff --git a/subprojects/packagefiles/proc-macro-error-attr-1-rs/meson.build b/subprojects/packagefiles/proc-macro-error-attr-1-rs/meson.build
index 3281b264333..c4c4c5e397c 100644
--- a/subprojects/packagefiles/proc-macro-error-attr-1-rs/meson.build
+++ b/subprojects/packagefiles/proc-macro-error-attr-1-rs/meson.build
@@ -16,6 +16,7 @@ _proc_macro_error_attr_rs = rust.proc_macro(
files('src/lib.rs'),
override_options: ['rust_std=2018', 'build.rust_std=2018'],
rust_args: [
+ '--cap-lints', 'allow',
'--cfg', 'use_fallback',
'--cfg', 'feature="syn-error"',
'--cfg', 'feature="proc-macro"'
diff --git a/subprojects/packagefiles/proc-macro2-1-rs/meson.build b/subprojects/packagefiles/proc-macro2-1-rs/meson.build
index f9c8675ebad..5759df3ecc9 100644
--- a/subprojects/packagefiles/proc-macro2-1-rs/meson.build
+++ b/subprojects/packagefiles/proc-macro2-1-rs/meson.build
@@ -15,6 +15,7 @@ _proc_macro2_rs = static_library(
override_options: ['rust_std=2021', 'build.rust_std=2021'],
rust_abi: 'rust',
rust_args: [
+ '--cap-lints', 'allow',
'--cfg', 'feature="proc-macro"',
'--cfg', 'no_literal_byte_character',
'--cfg', 'no_literal_c_string',
diff --git a/subprojects/packagefiles/quote-1-rs/meson.build b/subprojects/packagefiles/quote-1-rs/meson.build
index 7f7792569b9..bf41fad99bb 100644
--- a/subprojects/packagefiles/quote-1-rs/meson.build
+++ b/subprojects/packagefiles/quote-1-rs/meson.build
@@ -15,6 +15,7 @@ _quote_rs = static_library(
override_options: ['rust_std=2021', 'build.rust_std=2021'],
rust_abi: 'rust',
rust_args: [
+ '--cap-lints', 'allow',
'--cfg', 'feature="proc-macro"',
],
dependencies: [
diff --git a/subprojects/packagefiles/syn-2-rs/meson.build b/subprojects/packagefiles/syn-2-rs/meson.build
index 2c62cf7e1b0..a0094174084 100644
--- a/subprojects/packagefiles/syn-2-rs/meson.build
+++ b/subprojects/packagefiles/syn-2-rs/meson.build
@@ -19,6 +19,7 @@ _syn_rs = static_library(
override_options: ['rust_std=2021', 'build.rust_std=2021'],
rust_abi: 'rust',
rust_args: [
+ '--cap-lints', 'allow',
'--cfg', 'feature="full"',
'--cfg', 'feature="derive"',
'--cfg', 'feature="parsing"',
diff --git a/subprojects/packagefiles/unicode-ident-1-rs/meson.build b/subprojects/packagefiles/unicode-ident-1-rs/meson.build
index 9d76ebbd1ad..11a5dab97df 100644
--- a/subprojects/packagefiles/unicode-ident-1-rs/meson.build
+++ b/subprojects/packagefiles/unicode-ident-1-rs/meson.build
@@ -10,6 +10,7 @@ _unicode_ident_rs = static_library(
gnu_symbol_visibility: 'hidden',
override_options: ['rust_std=2021', 'build.rust_std=2021'],
rust_abi: 'rust',
+ rust_args: ['--cap-lints', 'allow'],
dependencies: [],
native: true,
)
diff --git a/subprojects/proc-macro-error-1-rs.wrap b/subprojects/proc-macro-error-1-rs.wrap
index b7db03b06a0..59f892f7825 100644
--- a/subprojects/proc-macro-error-1-rs.wrap
+++ b/subprojects/proc-macro-error-1-rs.wrap
@@ -5,3 +5,6 @@ source_filename = proc-macro-error-1.0.4.tar.gz
source_hash = da25490ff9892aab3fcf7c36f08cfb902dd3e71ca0f9f9517bea02a73a5ce38c
#method = cargo
patch_directory = proc-macro-error-1-rs
+
+# bump this version number on every change to meson.build or the patches:
+# v2
diff --git a/subprojects/proc-macro-error-attr-1-rs.wrap b/subprojects/proc-macro-error-attr-1-rs.wrap
index d13d8a239ac..5aeb224a103 100644
--- a/subprojects/proc-macro-error-attr-1-rs.wrap
+++ b/subprojects/proc-macro-error-attr-1-rs.wrap
@@ -5,3 +5,6 @@ source_filename = proc-macro-error-attr-1.0.4.tar.gz
source_hash = a1be40180e52ecc98ad80b184934baf3d0d29f979574e439af5a55274b35f869
#method = cargo
patch_directory = proc-macro-error-attr-1-rs
+
+# bump this version number on every change to meson.build or the patches:
+# v2
diff --git a/subprojects/proc-macro2-1-rs.wrap b/subprojects/proc-macro2-1-rs.wrap
index 7053e2c013c..6c9369f0df3 100644
--- a/subprojects/proc-macro2-1-rs.wrap
+++ b/subprojects/proc-macro2-1-rs.wrap
@@ -5,3 +5,6 @@ source_filename = proc-macro2-1.0.84.0.tar.gz
source_hash = ec96c6a92621310b51366f1e28d05ef11489516e93be030060e5fc12024a49d6
#method = cargo
patch_directory = proc-macro2-1-rs
+
+# bump this version number on every change to meson.build or the patches:
+# v2
diff --git a/subprojects/quote-1-rs.wrap b/subprojects/quote-1-rs.wrap
index 6e7ea69049f..8b721dfa00b 100644
--- a/subprojects/quote-1-rs.wrap
+++ b/subprojects/quote-1-rs.wrap
@@ -5,3 +5,6 @@ source_filename = quote-1.0.36.0.tar.gz
source_hash = 0fa76aaf39101c457836aec0ce2316dbdc3ab723cdda1c6bd4e6ad4208acaca7
#method = cargo
patch_directory = quote-1-rs
+
+# bump this version number on every change to meson.build or the patches:
+# v2
diff --git a/subprojects/syn-2-rs.wrap b/subprojects/syn-2-rs.wrap
index 13ffdac3c3b..d79cf750fb4 100644
--- a/subprojects/syn-2-rs.wrap
+++ b/subprojects/syn-2-rs.wrap
@@ -5,3 +5,6 @@ source_filename = syn-2.0.66.0.tar.gz
source_hash = c42f3f41a2de00b01c0aaad383c5a45241efc8b2d1eda5661812fda5f3cdcff5
#method = cargo
patch_directory = syn-2-rs
+
+# bump this version number on every change to meson.build or the patches:
+# v2
diff --git a/subprojects/unicode-ident-1-rs.wrap b/subprojects/unicode-ident-1-rs.wrap
index 4609f96ed97..50988f612e2 100644
--- a/subprojects/unicode-ident-1-rs.wrap
+++ b/subprojects/unicode-ident-1-rs.wrap
@@ -5,3 +5,6 @@ source_filename = unicode-ident-1.0.12.tar.gz
source_hash = 3354b9ac3fae1ff6755cb6db53683adb661634f67557942dea4facebec0fee4b
#method = cargo
patch_directory = unicode-ident-1-rs
+
+# bump this version number on every change to meson.build or the patches:
+# v2
diff --git a/subprojects/unicode-ident-1-rs/meson.build b/subprojects/unicode-ident-1-rs/meson.build
deleted file mode 100644
index 54f23768545..00000000000
--- a/subprojects/unicode-ident-1-rs/meson.build
+++ /dev/null
@@ -1,20 +0,0 @@
-project('unicode-ident-1-rs', 'rust',
- version: '1.0.12',
- license: '(MIT OR Apache-2.0) AND Unicode-DFS-2016',
- default_options: [])
-
-_unicode_ident_rs = static_library(
- 'unicode_ident',
- files('src/lib.rs'),
- gnu_symbol_visibility: 'hidden',
- override_options: ['rust_std=2021', 'build.rust_std=2021'],
- rust_abi: 'rust',
- dependencies: [],
- native: true,
-)
-
-unicode_ident_dep = declare_dependency(
- link_with: _unicode_ident_rs,
-)
-
-meson.override_dependency('unicode-ident-1-rs', unicode_ident_dep, native: true)
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 15/38] qom: remove unused field
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (13 preceding siblings ...)
2025-01-10 18:45 ` [PULL 14/38] rust: hide warnings for subprojects Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 18:45 ` [PULL 16/38] make-release: only leave tarball of wrap-file subprojects Paolo Bonzini
` (22 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel
The "concrete_class" field of InterfaceClass is only ever written, and as far
as I can tell is not particularly useful when debugging either; remove it.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/qom/object.h | 5 ++++-
qom/object.c | 1 -
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/qom/object.h b/include/qom/object.h
index 95d6e064d9b..f28ffea9a64 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -573,12 +573,15 @@ struct InterfaceInfo {
*
* The class for all interfaces. Subclasses of this class should only add
* virtual methods.
+ *
+ * Note that most of the fields of ObjectClass are unused (all except
+ * "type", in fact). They are only present in InterfaceClass to allow
+ * @object_class_dynamic_cast to work with both regular classes and interfaces.
*/
struct InterfaceClass
{
ObjectClass parent_class;
/* private: */
- ObjectClass *concrete_class;
Type interface_type;
};
diff --git a/qom/object.c b/qom/object.c
index b4c52d055d9..e9dfad854bd 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -314,7 +314,6 @@ static void type_initialize_interface(TypeImpl *ti, TypeImpl *interface_type,
g_free((char *)info.name);
new_iface = (InterfaceClass *)iface_impl->class;
- new_iface->concrete_class = ti->class;
new_iface->interface_type = interface_type;
ti->class->interfaces = g_slist_append(ti->class->interfaces, new_iface);
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 16/38] make-release: only leave tarball of wrap-file subprojects
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (14 preceding siblings ...)
2025-01-10 18:45 ` [PULL 15/38] qom: remove unused field Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 18:45 ` [PULL 17/38] target/i386: improve code generation for BT Paolo Bonzini
` (21 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Tokarev
The QEMU source archive is including the sources downloaded from crates.io
in both tarball form (in subprojects/packagecache) and expanded/patched
form (in the subprojects directory). The former is the more authoritative
form, as it has a hash that can be verified in the wrap file and checked
against the download URL, so keep that one only. This works also with
--disable-download; when building QEMU for the first time from the
tarball, Meson will print something like
Using proc-macro2-1-rs source from cache.
for each subproject, and then go on to extract the tarball and apply the
overlay or the patches in subprojects/packagefiles.
Reported-by: Michael Tokarev <mjt@tls.msk.ru>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2719
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
scripts/make-release | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/scripts/make-release b/scripts/make-release
index 8dc939124c4..2885e872109 100755
--- a/scripts/make-release
+++ b/scripts/make-release
@@ -10,6 +10,27 @@
# This work is licensed under the terms of the GNU GPLv2 or later.
# See the COPYING file in the top-level directory.
+function subproject_dir() {
+ if test ! -f "subprojects/$1.wrap"; then
+ error "scripts/archive-source.sh should only process wrap subprojects"
+ fi
+
+ # Print the directory key of the wrap file, defaulting to the
+ # subproject name. The wrap file is in ini format and should
+ # have a single section only. There should be only one section
+ # named "[wrap-*]", which helps keeping the script simple.
+ local dir
+ dir=$(sed -n \
+ -e '/^\[wrap-[a-z][a-z]*\]$/,/^\[/{' \
+ -e '/^directory *= */!b' \
+ -e 's///p' \
+ -e 'q' \
+ -e '}' \
+ "subprojects/$1.wrap")
+
+ echo "${dir:-$1}"
+}
+
if [ $# -ne 2 ]; then
echo "Usage:"
echo " $0 gitrepo version"
@@ -51,5 +72,13 @@ meson subprojects download $SUBPROJECTS
CryptoPkg/Library/OpensslLib/openssl \
MdeModulePkg/Library/BrotliCustomDecompressLib/brotli)
popd
-tar --exclude=.git -cJf ${destination}.tar.xz ${destination}
+
+exclude=(--exclude=.git)
+# include the tarballs in subprojects/packagecache but not their expansion
+for sp in $SUBPROJECTS; do
+ if grep -xqF "[wrap-file]" subprojects/$sp.wrap; then
+ exclude+=(--exclude=subprojects/"$(subproject_dir $sp)")
+ fi
+done
+tar "${exclude[@]}" -cJf ${destination}.tar.xz ${destination}
rm -rf ${destination}
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 17/38] target/i386: improve code generation for BT
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (15 preceding siblings ...)
2025-01-10 18:45 ` [PULL 16/38] make-release: only leave tarball of wrap-file subprojects Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 18:45 ` [PULL 18/38] target/i386: use shr to load high-byte registers into T0/T1 Paolo Bonzini
` (20 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel
Because BT does not write back to the source operand, it can modify it to
ensure that one of the operands of TSTNE is a constant (after either gen_BT
or the optimizer's constant propagation). This produces better and more
optimizable TCG ops. For example, the sequence
movl $0x60013f, %ebx
btl %ecx, %ebx
becomes just
and_i32 tmp1,ecx,$0x1f dead: 1 2 pref=0xffff
shr_i32 tmp0,$0x60013f,tmp1 dead: 1 2 pref=0xffff
and_i32 tmp16,tmp0,$0x1 dead: 1 pref=0xbf80
On s390x, it can use four instructions to isolate bit 0 of 0x60013f >> (ecx & 31):
nilf %r12, 0x1f
lgfi %r11, 0x60013f
srlk %r12, %r11, 0(%r12)
nilf %r12, 1
Previously, it used five instructions to build 1 << (ecx & 31) and compute
TSTEQ, and also needed two more to construct the result of setcond:
nilf %r12, 0x1f
lghi %r11, 1
sllk %r12, %r11, 0(%r12)
lgfi %r9, 0x60013f
nrk %r0, %r12, %r9
lghi %r12, 0
locghilh %r12, 1
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/tcg/emit.c.inc | 36 ++++++++++++++++++++++++++++--------
1 file changed, 28 insertions(+), 8 deletions(-)
diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
index 785ff63f2ac..5c115429350 100644
--- a/target/i386/tcg/emit.c.inc
+++ b/target/i386/tcg/emit.c.inc
@@ -1443,8 +1443,9 @@ static TCGv gen_bt_mask(DisasContext *s, X86DecodedInsn *decode)
return mask;
}
-/* Expects truncated bit index in s->T1, 1 << s->T1 in MASK. */
-static void gen_bt_flags(DisasContext *s, X86DecodedInsn *decode, TCGv src, TCGv mask)
+/* Expects truncated bit index in COUNT, 1 << COUNT in MASK. */
+static void gen_bt_flags(DisasContext *s, X86DecodedInsn *decode, TCGv src,
+ TCGv count, TCGv mask)
{
TCGv cf;
@@ -1467,15 +1468,34 @@ static void gen_bt_flags(DisasContext *s, X86DecodedInsn *decode, TCGv src, TCGv
decode->cc_src = tcg_temp_new();
decode->cc_dst = cpu_cc_dst;
decode->cc_op = CC_OP_SARB + cc_op_size(s->cc_op);
- tcg_gen_shr_tl(decode->cc_src, src, s->T1);
+ tcg_gen_shr_tl(decode->cc_src, src, count);
}
}
static void gen_BT(DisasContext *s, X86DecodedInsn *decode)
{
- TCGv mask = gen_bt_mask(s, decode);
+ TCGv count = s->T1;
+ TCGv mask;
- gen_bt_flags(s, decode, s->T0, mask);
+ /*
+ * Try to ensure that the rhs of the TSTNE condition is a constant (and a
+ * power of two), as that is more readily available on most TCG backends.
+ *
+ * For immediate bit number gen_bt_mask()'s output is already a constant;
+ * for register bit number, shift the source right and check bit 0.
+ */
+ if (decode->e.op2 == X86_TYPE_I) {
+ mask = gen_bt_mask(s, decode);
+ } else {
+ MemOp ot = decode->op[1].ot;
+
+ tcg_gen_andi_tl(s->T1, s->T1, (8 << ot) - 1);
+ tcg_gen_shr_tl(s->T0, s->T0, s->T1);
+
+ count = tcg_constant_tl(0);
+ mask = tcg_constant_tl(1);
+ }
+ gen_bt_flags(s, decode, s->T0, count, mask);
}
static void gen_BTC(DisasContext *s, X86DecodedInsn *decode)
@@ -1491,7 +1511,7 @@ static void gen_BTC(DisasContext *s, X86DecodedInsn *decode)
tcg_gen_xor_tl(s->T0, s->T0, mask);
}
- gen_bt_flags(s, decode, old, mask);
+ gen_bt_flags(s, decode, old, s->T1, mask);
}
static void gen_BTR(DisasContext *s, X86DecodedInsn *decode)
@@ -1509,7 +1529,7 @@ static void gen_BTR(DisasContext *s, X86DecodedInsn *decode)
tcg_gen_andc_tl(s->T0, s->T0, mask);
}
- gen_bt_flags(s, decode, old, mask);
+ gen_bt_flags(s, decode, old, s->T1, mask);
}
static void gen_BTS(DisasContext *s, X86DecodedInsn *decode)
@@ -1525,7 +1545,7 @@ static void gen_BTS(DisasContext *s, X86DecodedInsn *decode)
tcg_gen_or_tl(s->T0, s->T0, mask);
}
- gen_bt_flags(s, decode, old, mask);
+ gen_bt_flags(s, decode, old, s->T1, mask);
}
static void gen_BZHI(DisasContext *s, X86DecodedInsn *decode)
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 18/38] target/i386: use shr to load high-byte registers into T0/T1
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (16 preceding siblings ...)
2025-01-10 18:45 ` [PULL 17/38] target/i386: improve code generation for BT Paolo Bonzini
@ 2025-01-10 18:45 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 19/38] i386/cpu: Mark avx10_version filtered when prefix is NULL Paolo Bonzini
` (19 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:45 UTC (permalink / raw)
To: qemu-devel
Using a sextract or extract operation is only necessary if a
sign or zero extended value is needed. If not, a shift is
enough.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/tcg/translate.c | 2 +-
target/i386/tcg/emit.c.inc | 23 ++++++++++++-----------
2 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 834aea1e594..dbc9d637c4b 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -486,7 +486,7 @@ static inline
void gen_op_mov_v_reg(DisasContext *s, MemOp ot, TCGv t0, int reg)
{
if (ot == MO_8 && byte_reg_is_xH(s, reg)) {
- tcg_gen_extract_tl(t0, cpu_regs[reg - 4], 8, 8);
+ tcg_gen_shri_tl(t0, cpu_regs[reg - 4], 8);
} else {
tcg_gen_mov_tl(t0, cpu_regs[reg]);
}
diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
index 5c115429350..c4cc5f48d83 100644
--- a/target/i386/tcg/emit.c.inc
+++ b/target/i386/tcg/emit.c.inc
@@ -286,24 +286,25 @@ static void gen_load(DisasContext *s, X86DecodedInsn *decode, int opn, TCGv v)
gen_op_ld_v(s, op->ot, v, s->A0);
}
- } else if (op->ot == MO_8 && byte_reg_is_xH(s, op->n)) {
- if (v == s->T0 && decode->e.special == X86_SPECIAL_SExtT0) {
- tcg_gen_sextract_tl(v, cpu_regs[op->n - 4], 8, 8);
- } else {
- tcg_gen_extract_tl(v, cpu_regs[op->n - 4], 8, 8);
- }
-
} else if (op->ot < MO_TL && v == s->T0 &&
(decode->e.special == X86_SPECIAL_SExtT0 ||
decode->e.special == X86_SPECIAL_ZExtT0)) {
- if (decode->e.special == X86_SPECIAL_SExtT0) {
- tcg_gen_ext_tl(v, cpu_regs[op->n], op->ot | MO_SIGN);
+ if (op->ot == MO_8 && byte_reg_is_xH(s, op->n)) {
+ if (decode->e.special == X86_SPECIAL_SExtT0) {
+ tcg_gen_sextract_tl(v, cpu_regs[op->n - 4], 8, 8);
+ } else {
+ tcg_gen_extract_tl(v, cpu_regs[op->n - 4], 8, 8);
+ }
} else {
- tcg_gen_ext_tl(v, cpu_regs[op->n], op->ot);
+ if (decode->e.special == X86_SPECIAL_SExtT0) {
+ tcg_gen_ext_tl(v, cpu_regs[op->n], op->ot | MO_SIGN);
+ } else {
+ tcg_gen_ext_tl(v, cpu_regs[op->n], op->ot);
+ }
}
} else {
- tcg_gen_mov_tl(v, cpu_regs[op->n]);
+ gen_op_mov_v_reg(s, op->ot, v, op->n);
}
break;
case X86_OP_IMM:
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 19/38] i386/cpu: Mark avx10_version filtered when prefix is NULL
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (17 preceding siblings ...)
2025-01-10 18:45 ` [PULL 18/38] target/i386: use shr to load high-byte registers into T0/T1 Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 20/38] target/i386/kvm: Add feature bit definitions for KVM CPUID Paolo Bonzini
` (18 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu, qemu-stable, Tao Su
From: Zhao Liu <zhao1.liu@intel.com>
In x86_cpu_filter_features(), if host doesn't support AVX10, the
configured avx10_version should be marked as filtered regardless of
whether prefix is NULL or not.
Check prefix before warn_report() instead of checking for
have_filtered_features.
Cc: qemu-stable@nongnu.org
Fixes: commit bccfb846fd52 ("target/i386: add AVX10 feature and AVX10 version property")
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Tao Su <tao1.su@linux.intel.com>
Link: https://lore.kernel.org/r/20241106030728.553238-2-zhao1.liu@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/cpu.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 0b639848cd6..579d9bac955 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7719,8 +7719,10 @@ static bool x86_cpu_filter_features(X86CPU *cpu, bool verbose)
env->avx10_version = version;
have_filtered_features = true;
}
- } else if (env->avx10_version && prefix) {
- warn_report("%s: avx10.%d.", prefix, env->avx10_version);
+ } else if (env->avx10_version) {
+ if (prefix) {
+ warn_report("%s: avx10.%d.", prefix, env->avx10_version);
+ }
have_filtered_features = true;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 20/38] target/i386/kvm: Add feature bit definitions for KVM CPUID
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (18 preceding siblings ...)
2025-01-10 18:46 ` [PULL 19/38] i386/cpu: Mark avx10_version filtered when prefix is NULL Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 21/38] target/i386/kvm: Remove local MSR_KVM_WALL_CLOCK and MSR_KVM_SYSTEM_TIME definitions Paolo Bonzini
` (17 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu, Zide Chen
From: Zhao Liu <zhao1.liu@intel.com>
Add feature definitions for KVM_CPUID_FEATURES in CPUID (
CPUID[4000_0001].EAX and CPUID[4000_0001].EDX), to get rid of lots of
offset calculations.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Zide Chen <zide.chen@intel.com>
Link: https://lore.kernel.org/r/20241106030728.553238-3-zhao1.liu@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/cpu.h | 23 +++++++++++++++++++++++
hw/i386/kvm/clock.c | 5 ++---
target/i386/kvm/kvm.c | 28 ++++++++++++++--------------
3 files changed, 39 insertions(+), 17 deletions(-)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index dbd8f1ffc79..f41462d8c19 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -29,6 +29,7 @@
#include "qapi/qapi-types-common.h"
#include "qemu/cpu-float.h"
#include "qemu/timer.h"
+#include "standard-headers/asm-x86/kvm_para.h"
#define XEN_NR_VIRQS 24
@@ -1010,6 +1011,28 @@ uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w);
#define CPUID_8000_0007_EBX_OVERFLOW_RECOV (1U << 0)
#define CPUID_8000_0007_EBX_SUCCOR (1U << 1)
+/* (Old) KVM paravirtualized clocksource */
+#define CPUID_KVM_CLOCK (1U << KVM_FEATURE_CLOCKSOURCE)
+/* (New) KVM specific paravirtualized clocksource */
+#define CPUID_KVM_CLOCK2 (1U << KVM_FEATURE_CLOCKSOURCE2)
+/* KVM asynchronous page fault */
+#define CPUID_KVM_ASYNCPF (1U << KVM_FEATURE_ASYNC_PF)
+/* KVM stolen (when guest vCPU is not running) time accounting */
+#define CPUID_KVM_STEAL_TIME (1U << KVM_FEATURE_STEAL_TIME)
+/* KVM paravirtualized end-of-interrupt signaling */
+#define CPUID_KVM_PV_EOI (1U << KVM_FEATURE_PV_EOI)
+/* KVM paravirtualized spinlocks support */
+#define CPUID_KVM_PV_UNHALT (1U << KVM_FEATURE_PV_UNHALT)
+/* KVM host-side polling on HLT control from the guest */
+#define CPUID_KVM_POLL_CONTROL (1U << KVM_FEATURE_POLL_CONTROL)
+/* KVM interrupt based asynchronous page fault*/
+#define CPUID_KVM_ASYNCPF_INT (1U << KVM_FEATURE_ASYNC_PF_INT)
+/* KVM 'Extended Destination ID' support for external interrupts */
+#define CPUID_KVM_MSI_EXT_DEST_ID (1U << KVM_FEATURE_MSI_EXT_DEST_ID)
+
+/* Hint to KVM that vCPUs expect never preempted for an unlimited time */
+#define CPUID_KVM_HINTS_REALTIME (1U << KVM_HINTS_REALTIME)
+
/* CLZERO instruction */
#define CPUID_8000_0008_EBX_CLZERO (1U << 0)
/* Always save/restore FP error pointers */
diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 63be5088420..17443552e91 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -27,7 +27,6 @@
#include "qapi/error.h"
#include <linux/kvm.h>
-#include "standard-headers/asm-x86/kvm_para.h"
#include "qom/object.h"
#define TYPE_KVM_CLOCK "kvmclock"
@@ -333,8 +332,8 @@ void kvmclock_create(bool create_always)
assert(kvm_enabled());
if (create_always ||
- cpu->env.features[FEAT_KVM] & ((1ULL << KVM_FEATURE_CLOCKSOURCE) |
- (1ULL << KVM_FEATURE_CLOCKSOURCE2))) {
+ cpu->env.features[FEAT_KVM] & (CPUID_KVM_CLOCK |
+ CPUID_KVM_CLOCK2)) {
sysbus_create_simple(TYPE_KVM_CLOCK, -1, NULL);
}
}
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 2f66e63b880..d6fb3bee86d 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -564,13 +564,13 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
* be enabled without the in-kernel irqchip
*/
if (!kvm_irqchip_in_kernel()) {
- ret &= ~(1U << KVM_FEATURE_PV_UNHALT);
+ ret &= ~CPUID_KVM_PV_UNHALT;
}
if (kvm_irqchip_is_split()) {
- ret |= 1U << KVM_FEATURE_MSI_EXT_DEST_ID;
+ ret |= CPUID_KVM_MSI_EXT_DEST_ID;
}
} else if (function == KVM_CPUID_FEATURES && reg == R_EDX) {
- ret |= 1U << KVM_HINTS_REALTIME;
+ ret |= CPUID_KVM_HINTS_REALTIME;
}
if (current_machine->cgs) {
@@ -3978,20 +3978,20 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
kvm_msr_entry_add(cpu, MSR_IA32_TSC, env->tsc);
kvm_msr_entry_add(cpu, MSR_KVM_SYSTEM_TIME, env->system_time_msr);
kvm_msr_entry_add(cpu, MSR_KVM_WALL_CLOCK, env->wall_clock_msr);
- if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_ASYNC_PF_INT)) {
+ if (env->features[FEAT_KVM] & CPUID_KVM_ASYNCPF_INT) {
kvm_msr_entry_add(cpu, MSR_KVM_ASYNC_PF_INT, env->async_pf_int_msr);
}
- if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_ASYNC_PF)) {
+ if (env->features[FEAT_KVM] & CPUID_KVM_ASYNCPF) {
kvm_msr_entry_add(cpu, MSR_KVM_ASYNC_PF_EN, env->async_pf_en_msr);
}
- if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_PV_EOI)) {
+ if (env->features[FEAT_KVM] & CPUID_KVM_PV_EOI) {
kvm_msr_entry_add(cpu, MSR_KVM_PV_EOI_EN, env->pv_eoi_en_msr);
}
- if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_STEAL_TIME)) {
+ if (env->features[FEAT_KVM] & CPUID_KVM_STEAL_TIME) {
kvm_msr_entry_add(cpu, MSR_KVM_STEAL_TIME, env->steal_time_msr);
}
- if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_POLL_CONTROL)) {
+ if (env->features[FEAT_KVM] & CPUID_KVM_POLL_CONTROL) {
kvm_msr_entry_add(cpu, MSR_KVM_POLL_CONTROL, env->poll_control_msr);
}
@@ -4456,19 +4456,19 @@ static int kvm_get_msrs(X86CPU *cpu)
#endif
kvm_msr_entry_add(cpu, MSR_KVM_SYSTEM_TIME, 0);
kvm_msr_entry_add(cpu, MSR_KVM_WALL_CLOCK, 0);
- if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_ASYNC_PF_INT)) {
+ if (env->features[FEAT_KVM] & CPUID_KVM_ASYNCPF_INT) {
kvm_msr_entry_add(cpu, MSR_KVM_ASYNC_PF_INT, 0);
}
- if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_ASYNC_PF)) {
+ if (env->features[FEAT_KVM] & CPUID_KVM_ASYNCPF) {
kvm_msr_entry_add(cpu, MSR_KVM_ASYNC_PF_EN, 0);
}
- if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_PV_EOI)) {
+ if (env->features[FEAT_KVM] & CPUID_KVM_PV_EOI) {
kvm_msr_entry_add(cpu, MSR_KVM_PV_EOI_EN, 0);
}
- if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_STEAL_TIME)) {
+ if (env->features[FEAT_KVM] & CPUID_KVM_STEAL_TIME) {
kvm_msr_entry_add(cpu, MSR_KVM_STEAL_TIME, 0);
}
- if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_POLL_CONTROL)) {
+ if (env->features[FEAT_KVM] & CPUID_KVM_POLL_CONTROL) {
kvm_msr_entry_add(cpu, MSR_KVM_POLL_CONTROL, 1);
}
if (has_architectural_pmu_version > 0) {
@@ -6195,7 +6195,7 @@ uint64_t kvm_swizzle_msi_ext_dest_id(uint64_t address)
return address;
}
env = &X86_CPU(first_cpu)->env;
- if (!(env->features[FEAT_KVM] & (1 << KVM_FEATURE_MSI_EXT_DEST_ID))) {
+ if (!(env->features[FEAT_KVM] & CPUID_KVM_MSI_EXT_DEST_ID)) {
return address;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 21/38] target/i386/kvm: Remove local MSR_KVM_WALL_CLOCK and MSR_KVM_SYSTEM_TIME definitions
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (19 preceding siblings ...)
2025-01-10 18:46 ` [PULL 20/38] target/i386/kvm: Add feature bit definitions for KVM CPUID Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 22/38] target/i386/kvm: Only save/load kvmclock MSRs when kvmclock enabled Paolo Bonzini
` (16 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu, Xiaoyao Li, Zide Chen
From: Zhao Liu <zhao1.liu@intel.com>
These 2 MSRs have been already defined in kvm_para.h (standard-headers/
asm-x86/kvm_para.h).
Remove QEMU local definitions to avoid duplication.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>
Reviewed-by: Zide Chen <zide.chen@intel.com>
Link: https://lore.kernel.org/r/20241106030728.553238-4-zhao1.liu@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/kvm/kvm.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index d6fb3bee86d..7870820a2b6 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -95,9 +95,6 @@
#define KVM_APIC_BUS_CYCLE_NS 1
#define KVM_APIC_BUS_FREQUENCY (1000000000ULL / KVM_APIC_BUS_CYCLE_NS)
-#define MSR_KVM_WALL_CLOCK 0x11
-#define MSR_KVM_SYSTEM_TIME 0x12
-
/* A 4096-byte buffer can hold the 8-byte kvm_msrs header, plus
* 255 kvm_msr_entry structs */
#define MSR_BUF_SIZE 4096
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 22/38] target/i386/kvm: Only save/load kvmclock MSRs when kvmclock enabled
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (20 preceding siblings ...)
2025-01-10 18:46 ` [PULL 21/38] target/i386/kvm: Remove local MSR_KVM_WALL_CLOCK and MSR_KVM_SYSTEM_TIME definitions Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 23/38] target/i386/kvm: Drop workaround for KVM_X86_DISABLE_EXITS_HTL typo Paolo Bonzini
` (15 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu, Zide Chen
From: Zhao Liu <zhao1.liu@intel.com>
MSR_KVM_SYSTEM_TIME and MSR_KVM_WALL_CLOCK are attached with the (old)
kvmclock feature (KVM_FEATURE_CLOCKSOURCE).
So, just save/load them only when kvmclock (KVM_FEATURE_CLOCKSOURCE) is
enabled.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Zide Chen <zide.chen@intel.com>
Link: https://lore.kernel.org/r/20241106030728.553238-5-zhao1.liu@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/kvm/kvm.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 7870820a2b6..7536a3c9fd0 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -3973,8 +3973,10 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
*/
if (level >= KVM_PUT_RESET_STATE) {
kvm_msr_entry_add(cpu, MSR_IA32_TSC, env->tsc);
- kvm_msr_entry_add(cpu, MSR_KVM_SYSTEM_TIME, env->system_time_msr);
- kvm_msr_entry_add(cpu, MSR_KVM_WALL_CLOCK, env->wall_clock_msr);
+ if (env->features[FEAT_KVM] & (CPUID_KVM_CLOCK | CPUID_KVM_CLOCK2)) {
+ kvm_msr_entry_add(cpu, MSR_KVM_SYSTEM_TIME, env->system_time_msr);
+ kvm_msr_entry_add(cpu, MSR_KVM_WALL_CLOCK, env->wall_clock_msr);
+ }
if (env->features[FEAT_KVM] & CPUID_KVM_ASYNCPF_INT) {
kvm_msr_entry_add(cpu, MSR_KVM_ASYNC_PF_INT, env->async_pf_int_msr);
}
@@ -4451,8 +4453,10 @@ static int kvm_get_msrs(X86CPU *cpu)
}
}
#endif
- kvm_msr_entry_add(cpu, MSR_KVM_SYSTEM_TIME, 0);
- kvm_msr_entry_add(cpu, MSR_KVM_WALL_CLOCK, 0);
+ if (env->features[FEAT_KVM] & (CPUID_KVM_CLOCK | CPUID_KVM_CLOCK2)) {
+ kvm_msr_entry_add(cpu, MSR_KVM_SYSTEM_TIME, 0);
+ kvm_msr_entry_add(cpu, MSR_KVM_WALL_CLOCK, 0);
+ }
if (env->features[FEAT_KVM] & CPUID_KVM_ASYNCPF_INT) {
kvm_msr_entry_add(cpu, MSR_KVM_ASYNC_PF_INT, 0);
}
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 23/38] target/i386/kvm: Drop workaround for KVM_X86_DISABLE_EXITS_HTL typo
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (21 preceding siblings ...)
2025-01-10 18:46 ` [PULL 22/38] target/i386/kvm: Only save/load kvmclock MSRs when kvmclock enabled Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 24/38] target/i386/confidential-guest: Fix comment of x86_confidential_guest_kvm_type() Paolo Bonzini
` (14 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu, Zide Chen
From: Zhao Liu <zhao1.liu@intel.com>
The KVM_X86_DISABLE_EXITS_HTL typo has been fixed in commit
77d361b13c19 ("linux-headers: Update to kernel mainline commit
b357bf602").
Drop the related workaround.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Zide Chen <zide.chen@intel.com>
Link: https://lore.kernel.org/r/20241106030728.553238-7-zhao1.liu@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/kvm/kvm.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 7536a3c9fd0..17f23607ed0 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -3100,10 +3100,7 @@ static int kvm_vm_set_tss_addr(KVMState *s, uint64_t tss_base)
static int kvm_vm_enable_disable_exits(KVMState *s)
{
int disable_exits = kvm_check_extension(s, KVM_CAP_X86_DISABLE_EXITS);
-/* Work around for kernel header with a typo. TODO: fix header and drop. */
-#if defined(KVM_X86_DISABLE_EXITS_HTL) && !defined(KVM_X86_DISABLE_EXITS_HLT)
-#define KVM_X86_DISABLE_EXITS_HLT KVM_X86_DISABLE_EXITS_HTL
-#endif
+
if (disable_exits) {
disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT |
KVM_X86_DISABLE_EXITS_HLT |
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 24/38] target/i386/confidential-guest: Fix comment of x86_confidential_guest_kvm_type()
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (22 preceding siblings ...)
2025-01-10 18:46 ` [PULL 23/38] target/i386/kvm: Drop workaround for KVM_X86_DISABLE_EXITS_HTL typo Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 25/38] target/i386/kvm: Clean up return values of MSR filter related functions Paolo Bonzini
` (13 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu, Xiaoyao Li, Pankaj Gupta, Zide Chen
From: Zhao Liu <zhao1.liu@intel.com>
Update the comment to match the X86ConfidentialGuestClass
implementation.
Reported-by: Xiaoyao Li <xiaoyao.li@intel.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>
Reviewed-by: Zide Chen <zide.chen@intel.com>
Link: https://lore.kernel.org/r/20241106030728.553238-8-zhao1.liu@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/confidential-guest.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/i386/confidential-guest.h b/target/i386/confidential-guest.h
index 0afb8317b58..164be7633a2 100644
--- a/target/i386/confidential-guest.h
+++ b/target/i386/confidential-guest.h
@@ -46,7 +46,7 @@ struct X86ConfidentialGuestClass {
/**
* x86_confidential_guest_kvm_type:
*
- * Calls #X86ConfidentialGuestClass.unplug callback of @plug_handler.
+ * Calls #X86ConfidentialGuestClass.kvm_type() callback.
*/
static inline int x86_confidential_guest_kvm_type(X86ConfidentialGuest *cg)
{
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 25/38] target/i386/kvm: Clean up return values of MSR filter related functions
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (23 preceding siblings ...)
2025-01-10 18:46 ` [PULL 24/38] target/i386/confidential-guest: Fix comment of x86_confidential_guest_kvm_type() Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 26/38] target/i386/kvm: Return -1 when kvm_msr_energy_thread_init() fails Paolo Bonzini
` (12 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu, Zide Chen
From: Zhao Liu <zhao1.liu@intel.com>
Before commit 0cc42e63bb54 ("kvm/i386: refactor kvm_arch_init and split
it into smaller functions"), error_report() attempts to print the error
code from kvm_filter_msr(). However, printing error code does not work
due to kvm_filter_msr() returns bool instead int.
0cc42e63bb54 fixed the error by removing error code printing, but this
lost useful error messages. Bring it back by making kvm_filter_msr()
return int.
This also makes the function call chain processing clearer, allowing for
better handling of error result propagation from kvm_filter_msr() to
kvm_arch_init(), preparing for the subsequent cleanup work of error
handling in kvm_arch_init().
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Zide Chen <zide.chen@intel.com>
Link: https://lore.kernel.org/r/20241106030728.553238-9-zhao1.liu@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/kvm/kvm.c | 87 ++++++++++++++++++++++---------------------
1 file changed, 44 insertions(+), 43 deletions(-)
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 17f23607ed0..097a040da30 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -108,8 +108,8 @@ typedef struct {
} KVMMSRHandlers;
static void kvm_init_msrs(X86CPU *cpu);
-static bool kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr,
- QEMUWRMSRHandler *wrmsr);
+static int kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr,
+ QEMUWRMSRHandler *wrmsr);
const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
KVM_CAP_INFO(SET_TSS_ADDR),
@@ -3150,17 +3150,21 @@ static int kvm_vm_enable_notify_vmexit(KVMState *s)
static int kvm_vm_enable_userspace_msr(KVMState *s)
{
- int ret = kvm_vm_enable_cap(s, KVM_CAP_X86_USER_SPACE_MSR, 0,
- KVM_MSR_EXIT_REASON_FILTER);
+ int ret;
+
+ ret = kvm_vm_enable_cap(s, KVM_CAP_X86_USER_SPACE_MSR, 0,
+ KVM_MSR_EXIT_REASON_FILTER);
if (ret < 0) {
error_report("Could not enable user space MSRs: %s",
strerror(-ret));
exit(1);
}
- if (!kvm_filter_msr(s, MSR_CORE_THREAD_COUNT,
- kvm_rdmsr_core_thread_count, NULL)) {
- error_report("Could not install MSR_CORE_THREAD_COUNT handler!");
+ ret = kvm_filter_msr(s, MSR_CORE_THREAD_COUNT,
+ kvm_rdmsr_core_thread_count, NULL);
+ if (ret < 0) {
+ error_report("Could not install MSR_CORE_THREAD_COUNT handler: %s",
+ strerror(-ret));
exit(1);
}
@@ -3169,36 +3173,37 @@ static int kvm_vm_enable_userspace_msr(KVMState *s)
static void kvm_vm_enable_energy_msrs(KVMState *s)
{
- bool r;
+ int ret;
+
if (s->msr_energy.enable == true) {
- r = kvm_filter_msr(s, MSR_RAPL_POWER_UNIT,
- kvm_rdmsr_rapl_power_unit, NULL);
- if (!r) {
- error_report("Could not install MSR_RAPL_POWER_UNIT \
- handler");
+ ret = kvm_filter_msr(s, MSR_RAPL_POWER_UNIT,
+ kvm_rdmsr_rapl_power_unit, NULL);
+ if (ret < 0) {
+ error_report("Could not install MSR_RAPL_POWER_UNIT handler: %s",
+ strerror(-ret));
exit(1);
}
- r = kvm_filter_msr(s, MSR_PKG_POWER_LIMIT,
- kvm_rdmsr_pkg_power_limit, NULL);
- if (!r) {
- error_report("Could not install MSR_PKG_POWER_LIMIT \
- handler");
+ ret = kvm_filter_msr(s, MSR_PKG_POWER_LIMIT,
+ kvm_rdmsr_pkg_power_limit, NULL);
+ if (ret < 0) {
+ error_report("Could not install MSR_PKG_POWER_LIMIT handler: %s",
+ strerror(-ret));
exit(1);
}
- r = kvm_filter_msr(s, MSR_PKG_POWER_INFO,
- kvm_rdmsr_pkg_power_info, NULL);
- if (!r) {
- error_report("Could not install MSR_PKG_POWER_INFO \
- handler");
+ ret = kvm_filter_msr(s, MSR_PKG_POWER_INFO,
+ kvm_rdmsr_pkg_power_info, NULL);
+ if (ret < 0) {
+ error_report("Could not install MSR_PKG_POWER_INFO handler: %s",
+ strerror(-ret));
exit(1);
}
- r = kvm_filter_msr(s, MSR_PKG_ENERGY_STATUS,
- kvm_rdmsr_pkg_energy_status, NULL);
- if (!r) {
- error_report("Could not install MSR_PKG_ENERGY_STATUS \
- handler");
+ ret = kvm_filter_msr(s, MSR_PKG_ENERGY_STATUS,
+ kvm_rdmsr_pkg_energy_status, NULL);
+ if (ret < 0) {
+ error_report("Could not install MSR_PKG_ENERGY_STATUS handler: %s",
+ strerror(-ret));
exit(1);
}
}
@@ -5841,13 +5846,13 @@ void kvm_arch_update_guest_debug(CPUState *cpu, struct kvm_guest_debug *dbg)
}
}
-static bool kvm_install_msr_filters(KVMState *s)
+static int kvm_install_msr_filters(KVMState *s)
{
uint64_t zero = 0;
struct kvm_msr_filter filter = {
.flags = KVM_MSR_FILTER_DEFAULT_ALLOW,
};
- int r, i, j = 0;
+ int i, j = 0;
for (i = 0; i < KVM_MSR_FILTER_MAX_RANGES; i++) {
KVMMSRHandlers *handler = &msr_handlers[i];
@@ -5871,18 +5876,13 @@ static bool kvm_install_msr_filters(KVMState *s)
}
}
- r = kvm_vm_ioctl(s, KVM_X86_SET_MSR_FILTER, &filter);
- if (r) {
- return false;
- }
-
- return true;
+ return kvm_vm_ioctl(s, KVM_X86_SET_MSR_FILTER, &filter);
}
-static bool kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr,
- QEMUWRMSRHandler *wrmsr)
+static int kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr,
+ QEMUWRMSRHandler *wrmsr)
{
- int i;
+ int i, ret;
for (i = 0; i < ARRAY_SIZE(msr_handlers); i++) {
if (!msr_handlers[i].msr) {
@@ -5892,16 +5892,17 @@ static bool kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr,
.wrmsr = wrmsr,
};
- if (!kvm_install_msr_filters(s)) {
+ ret = kvm_install_msr_filters(s);
+ if (ret) {
msr_handlers[i] = (KVMMSRHandlers) { };
- return false;
+ return ret;
}
- return true;
+ return 0;
}
}
- return false;
+ return -EINVAL;
}
static int kvm_handle_rdmsr(X86CPU *cpu, struct kvm_run *run)
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 26/38] target/i386/kvm: Return -1 when kvm_msr_energy_thread_init() fails
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (24 preceding siblings ...)
2025-01-10 18:46 ` [PULL 25/38] target/i386/kvm: Clean up return values of MSR filter related functions Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 27/38] target/i386/kvm: Clean up error handling in kvm_arch_init() Paolo Bonzini
` (11 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu
From: Zhao Liu <zhao1.liu@intel.com>
It is common practice to return a negative value (like -1) to indicate
an error, and other functions in kvm_arch_init() follow this style.
To avoid confusion (sometimes returned -1 indicates failure, and
sometimes -1, in a same function), return -1 when
kvm_msr_energy_thread_init() fails.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Link: https://lore.kernel.org/r/20241106030728.553238-10-zhao1.liu@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/kvm/kvm.c | 29 +++++++++++------------------
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 097a040da30..3624abbb397 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2936,7 +2936,6 @@ static int kvm_msr_energy_thread_init(KVMState *s, MachineState *ms)
{
MachineClass *mc = MACHINE_GET_CLASS(ms);
struct KVMMsrEnergy *r = &s->msr_energy;
- int ret = 0;
/*
* Sanity check
@@ -2946,13 +2945,11 @@ static int kvm_msr_energy_thread_init(KVMState *s, MachineState *ms)
if (!is_host_cpu_intel()) {
error_report("The RAPL feature can only be enabled on hosts "
"with Intel CPU models");
- ret = 1;
- goto out;
+ return -1;
}
if (!is_rapl_enabled()) {
- ret = 1;
- goto out;
+ return -1;
}
/* Retrieve the virtual topology */
@@ -2974,16 +2971,14 @@ static int kvm_msr_energy_thread_init(KVMState *s, MachineState *ms)
r->host_topo.maxcpus = vmsr_get_maxcpus();
if (r->host_topo.maxcpus == 0) {
error_report("host max cpus = 0");
- ret = 1;
- goto out;
+ return -1;
}
/* Max number of packages on the host */
r->host_topo.maxpkgs = vmsr_get_max_physical_package(r->host_topo.maxcpus);
if (r->host_topo.maxpkgs == 0) {
error_report("host max pkgs = 0");
- ret = 1;
- goto out;
+ return -1;
}
/* Allocate memory for each package on the host */
@@ -2995,8 +2990,7 @@ static int kvm_msr_energy_thread_init(KVMState *s, MachineState *ms)
for (int i = 0; i < r->host_topo.maxpkgs; i++) {
if (r->host_topo.pkg_cpu_count[i] == 0) {
error_report("cpu per packages = 0 on package_%d", i);
- ret = 1;
- goto out;
+ return -1;
}
}
@@ -3013,8 +3007,7 @@ static int kvm_msr_energy_thread_init(KVMState *s, MachineState *ms)
if (s->msr_energy.sioc == NULL) {
error_report("vmsr socket opening failed");
- ret = 1;
- goto out;
+ return -1;
}
/* Those MSR values should not change */
@@ -3026,15 +3019,13 @@ static int kvm_msr_energy_thread_init(KVMState *s, MachineState *ms)
s->msr_energy.sioc);
if (r->msr_unit == 0 || r->msr_limit == 0 || r->msr_info == 0) {
error_report("can't read any virtual msr");
- ret = 1;
- goto out;
+ return -1;
}
qemu_thread_create(&r->msr_thr, "kvm-msr",
kvm_msr_energy_thread,
s, QEMU_THREAD_JOINABLE);
-out:
- return ret;
+ return 0;
}
int kvm_arch_get_default_type(MachineState *ms)
@@ -3342,7 +3333,9 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
if (s->msr_energy.enable == true) {
kvm_vm_enable_energy_msrs(s);
- if (kvm_msr_energy_thread_init(s, ms)) {
+
+ ret = kvm_msr_energy_thread_init(s, ms);
+ if (ret < 0) {
error_report("kvm : error RAPL feature requirement not met");
exit(1);
}
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 27/38] target/i386/kvm: Clean up error handling in kvm_arch_init()
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (25 preceding siblings ...)
2025-01-10 18:46 ` [PULL 26/38] target/i386/kvm: Return -1 when kvm_msr_energy_thread_init() fails Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 28/38] target/i386/kvm: Replace ARRAY_SIZE(msr_handlers) with KVM_MSR_FILTER_MAX_RANGES Paolo Bonzini
` (10 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Zhao Liu, Zide Chen
From: Zhao Liu <zhao1.liu@intel.com>
Currently, there're following incorrect error handling cases in
kvm_arch_init():
* Missed to handle failure of kvm_get_supported_feature_msrs().
* Missed to return when kvm_vm_enable_disable_exits() fails.
* MSR filter related cases called exit() directly instead of returning
to kvm_init(). (The caller of kvm_arch_init() - kvm_init() - needs to
know if kvm_arch_init() fails in order to perform cleanup).
Fix the above cases.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Zide Chen <zide.chen@intel.com>
Link: https://lore.kernel.org/r/20241106030728.553238-11-zhao1.liu@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/kvm/kvm.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 3624abbb397..6f424774b33 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -3162,7 +3162,7 @@ static int kvm_vm_enable_userspace_msr(KVMState *s)
return 0;
}
-static void kvm_vm_enable_energy_msrs(KVMState *s)
+static int kvm_vm_enable_energy_msrs(KVMState *s)
{
int ret;
@@ -3172,7 +3172,7 @@ static void kvm_vm_enable_energy_msrs(KVMState *s)
if (ret < 0) {
error_report("Could not install MSR_RAPL_POWER_UNIT handler: %s",
strerror(-ret));
- exit(1);
+ return ret;
}
ret = kvm_filter_msr(s, MSR_PKG_POWER_LIMIT,
@@ -3180,7 +3180,7 @@ static void kvm_vm_enable_energy_msrs(KVMState *s)
if (ret < 0) {
error_report("Could not install MSR_PKG_POWER_LIMIT handler: %s",
strerror(-ret));
- exit(1);
+ return ret;
}
ret = kvm_filter_msr(s, MSR_PKG_POWER_INFO,
@@ -3188,17 +3188,17 @@ static void kvm_vm_enable_energy_msrs(KVMState *s)
if (ret < 0) {
error_report("Could not install MSR_PKG_POWER_INFO handler: %s",
strerror(-ret));
- exit(1);
+ return ret;
}
ret = kvm_filter_msr(s, MSR_PKG_ENERGY_STATUS,
kvm_rdmsr_pkg_energy_status, NULL);
if (ret < 0) {
error_report("Could not install MSR_PKG_ENERGY_STATUS handler: %s",
strerror(-ret));
- exit(1);
+ return ret;
}
}
- return;
+ return 0;
}
int kvm_arch_init(MachineState *ms, KVMState *s)
@@ -3265,7 +3265,10 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
return ret;
}
- kvm_get_supported_feature_msrs(s);
+ ret = kvm_get_supported_feature_msrs(s);
+ if (ret < 0) {
+ return ret;
+ }
uname(&utsname);
lm_capable_kernel = strcmp(utsname.machine, "x86_64") == 0;
@@ -3301,6 +3304,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
if (ret < 0) {
error_report("kvm: guest stopping CPU not supported: %s",
strerror(-ret));
+ return ret;
}
}
@@ -3332,12 +3336,15 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
}
if (s->msr_energy.enable == true) {
- kvm_vm_enable_energy_msrs(s);
+ ret = kvm_vm_enable_energy_msrs(s);
+ if (ret < 0) {
+ return ret;
+ }
ret = kvm_msr_energy_thread_init(s, ms);
if (ret < 0) {
error_report("kvm : error RAPL feature requirement not met");
- exit(1);
+ return ret;
}
}
}
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 28/38] target/i386/kvm: Replace ARRAY_SIZE(msr_handlers) with KVM_MSR_FILTER_MAX_RANGES
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (26 preceding siblings ...)
2025-01-10 18:46 ` [PULL 27/38] target/i386/kvm: Clean up error handling in kvm_arch_init() Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 29/38] i386/cpu: Extract a common fucntion to setup value of MSR_CORE_THREAD_COUNT Paolo Bonzini
` (9 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel
kvm_install_msr_filters() uses KVM_MSR_FILTER_MAX_RANGES as the bound
when traversing msr_handlers[], while other places still compute the
size by ARRAY_SIZE(msr_handlers).
In fact, msr_handlers[] is an array with the fixed size
KVM_MSR_FILTER_MAX_RANGES, and this has to be true because
kvm_install_msr_filters copies from one array to the other.
For code consistency, assert that they match and use
ARRAY_SIZE(msr_handlers) everywehere.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/kvm/kvm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 6f424774b33..1d7214b6a65 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -5854,7 +5854,8 @@ static int kvm_install_msr_filters(KVMState *s)
};
int i, j = 0;
- for (i = 0; i < KVM_MSR_FILTER_MAX_RANGES; i++) {
+ QEMU_BUILD_BUG_ON(ARRAY_SIZE(msr_handlers) != ARRAY_SIZE(filter.ranges));
+ for (i = 0; i < ARRAY_SIZE(msr_handlers); i++) {
KVMMSRHandlers *handler = &msr_handlers[i];
if (handler->msr) {
struct kvm_msr_filter_range *range = &filter.ranges[j++];
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 29/38] i386/cpu: Extract a common fucntion to setup value of MSR_CORE_THREAD_COUNT
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (27 preceding siblings ...)
2025-01-10 18:46 ` [PULL 28/38] target/i386/kvm: Replace ARRAY_SIZE(msr_handlers) with KVM_MSR_FILTER_MAX_RANGES Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 30/38] i386/cpu: Drop the variable smp_cores and smp_threads in x86_cpu_pre_plug() Paolo Bonzini
` (8 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Xiaoyao Li, Zhao Liu
From: Xiaoyao Li <xiaoyao.li@intel.com>
There are duplicated code to setup the value of MSR_CORE_THREAD_COUNT.
Extract a common function for it.
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Link: https://lore.kernel.org/r/20241219110125.1266461-2-xiaoyao.li@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/cpu.h | 2 ++
target/i386/cpu-system.c | 11 +++++++++++
target/i386/hvf/x86_emu.c | 3 +--
target/i386/kvm/kvm.c | 5 +----
target/i386/tcg/system/misc_helper.c | 3 +--
5 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index f41462d8c19..e8c46d877e0 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2413,6 +2413,8 @@ static inline void cpu_x86_load_seg_cache_sipi(X86CPU *cpu,
cs->halted = 0;
}
+uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu);
+
int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
target_ulong *base, unsigned int *limit,
unsigned int *flags);
diff --git a/target/i386/cpu-system.c b/target/i386/cpu-system.c
index 9d007afdab5..eb38cca68ff 100644
--- a/target/i386/cpu-system.c
+++ b/target/i386/cpu-system.c
@@ -309,3 +309,14 @@ void x86_cpu_get_crash_info_qom(Object *obj, Visitor *v,
errp);
qapi_free_GuestPanicInformation(panic_info);
}
+
+uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu)
+{
+ CPUState *cs = CPU(cpu);
+ uint64_t val;
+
+ val = cs->nr_threads * cs->nr_cores; /* thread count, bits 15..0 */
+ val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 31..16 */
+
+ return val;
+}
diff --git a/target/i386/hvf/x86_emu.c b/target/i386/hvf/x86_emu.c
index 015f760acb3..69c61c9c073 100644
--- a/target/i386/hvf/x86_emu.c
+++ b/target/i386/hvf/x86_emu.c
@@ -765,8 +765,7 @@ void simulate_rdmsr(CPUX86State *env)
val = env->mtrr_deftype;
break;
case MSR_CORE_THREAD_COUNT:
- val = cs->nr_threads * cs->nr_cores; /* thread count, bits 15..0 */
- val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 31..16 */
+ val = cpu_x86_get_msr_core_thread_count(cpu);
break;
default:
/* fprintf(stderr, "%s: unknown msr 0x%x\n", __func__, msr); */
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 1d7214b6a65..6c749d4ee81 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2614,10 +2614,7 @@ static bool kvm_rdmsr_core_thread_count(X86CPU *cpu,
uint32_t msr,
uint64_t *val)
{
- CPUState *cs = CPU(cpu);
-
- *val = cs->nr_threads * cs->nr_cores; /* thread count, bits 15..0 */
- *val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 31..16 */
+ *val = cpu_x86_get_msr_core_thread_count(cpu);
return true;
}
diff --git a/target/i386/tcg/system/misc_helper.c b/target/i386/tcg/system/misc_helper.c
index ffed8a3215e..c9c4d42f844 100644
--- a/target/i386/tcg/system/misc_helper.c
+++ b/target/i386/tcg/system/misc_helper.c
@@ -468,8 +468,7 @@ void helper_rdmsr(CPUX86State *env)
val = x86_cpu->ucode_rev;
break;
case MSR_CORE_THREAD_COUNT: {
- CPUState *cs = CPU(x86_cpu);
- val = (cs->nr_threads * cs->nr_cores) | (cs->nr_cores << 16);
+ val = cpu_x86_get_msr_core_thread_count(x86_cpu);
break;
}
case MSR_APIC_START ... MSR_APIC_END: {
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 30/38] i386/cpu: Drop the variable smp_cores and smp_threads in x86_cpu_pre_plug()
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (28 preceding siblings ...)
2025-01-10 18:46 ` [PULL 29/38] i386/cpu: Extract a common fucntion to setup value of MSR_CORE_THREAD_COUNT Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 31/38] i386/cpu: Drop cores_per_pkg in cpu_x86_cpuid() Paolo Bonzini
` (7 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Xiaoyao Li, Zhao Liu
From: Xiaoyao Li <xiaoyao.li@intel.com>
No need to define smp_cores and smp_threads, just using ms->smp.cores
and ms->smp.threads is straightforward. It's also consistent with other
checks of socket/die/module.
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Link: https://lore.kernel.org/r/20241219110125.1266461-3-xiaoyao.li@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/i386/x86-common.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
index a7d46c31059..5b0629f9ad3 100644
--- a/hw/i386/x86-common.c
+++ b/hw/i386/x86-common.c
@@ -248,8 +248,6 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
CPUX86State *env = &cpu->env;
MachineState *ms = MACHINE(hotplug_dev);
X86MachineState *x86ms = X86_MACHINE(hotplug_dev);
- unsigned int smp_cores = ms->smp.cores;
- unsigned int smp_threads = ms->smp.threads;
X86CPUTopoInfo topo_info;
if (!object_dynamic_cast(OBJECT(cpu), ms->cpu_type)) {
@@ -329,17 +327,17 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
if (cpu->core_id < 0) {
error_setg(errp, "CPU core-id is not set");
return;
- } else if (cpu->core_id > (smp_cores - 1)) {
+ } else if (cpu->core_id > (ms->smp.cores - 1)) {
error_setg(errp, "Invalid CPU core-id: %u must be in range 0:%u",
- cpu->core_id, smp_cores - 1);
+ cpu->core_id, ms->smp.cores - 1);
return;
}
if (cpu->thread_id < 0) {
error_setg(errp, "CPU thread-id is not set");
return;
- } else if (cpu->thread_id > (smp_threads - 1)) {
+ } else if (cpu->thread_id > (ms->smp.threads - 1)) {
error_setg(errp, "Invalid CPU thread-id: %u must be in range 0:%u",
- cpu->thread_id, smp_threads - 1);
+ cpu->thread_id, ms->smp.threads - 1);
return;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 31/38] i386/cpu: Drop cores_per_pkg in cpu_x86_cpuid()
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (29 preceding siblings ...)
2025-01-10 18:46 ` [PULL 30/38] i386/cpu: Drop the variable smp_cores and smp_threads in x86_cpu_pre_plug() Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 32/38] i386/topology: Update the comment of x86_apicid_from_topo_ids() Paolo Bonzini
` (6 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Xiaoyao Li, Zhao Liu
From: Xiaoyao Li <xiaoyao.li@intel.com>
Local variable cores_per_pkg is only used to calculate threads_per_pkg.
No need for it. Drop it and open-code it instead.
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Link: https://lore.kernel.org/r/20241219110125.1266461-4-xiaoyao.li@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/cpu.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 579d9bac955..6d9c85576f6 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6499,7 +6499,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
uint32_t limit;
uint32_t signature[3];
X86CPUTopoInfo topo_info;
- uint32_t cores_per_pkg;
uint32_t threads_per_pkg;
topo_info.dies_per_pkg = env->nr_dies;
@@ -6507,9 +6506,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
topo_info.cores_per_module = cs->nr_cores / env->nr_dies / env->nr_modules;
topo_info.threads_per_core = cs->nr_threads;
- cores_per_pkg = topo_info.cores_per_module * topo_info.modules_per_die *
- topo_info.dies_per_pkg;
- threads_per_pkg = cores_per_pkg * topo_info.threads_per_core;
+ threads_per_pkg = topo_info.threads_per_core * topo_info.cores_per_module *
+ topo_info.modules_per_die * topo_info.dies_per_pkg;
/* Calculate & apply limits for different index ranges */
if (index >= 0xC0000000) {
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 32/38] i386/topology: Update the comment of x86_apicid_from_topo_ids()
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (30 preceding siblings ...)
2025-01-10 18:46 ` [PULL 31/38] i386/cpu: Drop cores_per_pkg in cpu_x86_cpuid() Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 33/38] i386/topology: Introduce helpers for various topology info of different level Paolo Bonzini
` (5 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Xiaoyao Li, Zhao Liu
From: Xiaoyao Li <xiaoyao.li@intel.com>
Update the comment of x86_apicid_from_topo_ids() to match the current
implementation,
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Link: https://lore.kernel.org/r/20241219110125.1266461-5-xiaoyao.li@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/hw/i386/topology.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
index b2c8bf2de15..21b65219a5c 100644
--- a/include/hw/i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -121,9 +121,10 @@ static inline unsigned apicid_pkg_offset(X86CPUTopoInfo *topo_info)
}
/*
- * Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
+ * Make APIC ID for the CPU based on topology and IDs of each topology level.
*
- * The caller must make sure core_id < nr_cores and smt_id < nr_threads.
+ * The caller must make sure the ID of each level doesn't exceed the width of
+ * the level.
*/
static inline apic_id_t x86_apicid_from_topo_ids(X86CPUTopoInfo *topo_info,
const X86CPUTopoIDs *topo_ids)
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 33/38] i386/topology: Introduce helpers for various topology info of different level
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (31 preceding siblings ...)
2025-01-10 18:46 ` [PULL 32/38] i386/topology: Update the comment of x86_apicid_from_topo_ids() Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 34/38] i386/cpu: Track a X86CPUTopoInfo directly in CPUX86State Paolo Bonzini
` (4 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Xiaoyao Li
From: Xiaoyao Li <xiaoyao.li@intel.com>
Introduce various helpers for getting the topology info of different
semantics. Using the helper is more self-explanatory.
Besides, the semantic of the helper will stay unchanged even when new
topology is added in the future. At that time, updating the
implementation of the helper without affecting the callers.
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Link: https://lore.kernel.org/r/20241219110125.1266461-6-xiaoyao.li@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/hw/i386/topology.h | 25 +++++++++++++++++++++++++
target/i386/cpu.c | 11 ++++-------
2 files changed, 29 insertions(+), 7 deletions(-)
diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
index 21b65219a5c..f6380f1ed75 100644
--- a/include/hw/i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -203,4 +203,29 @@ static inline bool x86_has_extended_topo(unsigned long *topo_bitmap)
test_bit(CPU_TOPOLOGY_LEVEL_DIE, topo_bitmap);
}
+static inline unsigned x86_module_per_pkg(X86CPUTopoInfo *topo_info)
+{
+ return topo_info->modules_per_die * topo_info->dies_per_pkg;
+}
+
+static inline unsigned x86_cores_per_pkg(X86CPUTopoInfo *topo_info)
+{
+ return topo_info->cores_per_module * x86_module_per_pkg(topo_info);
+}
+
+static inline unsigned x86_threads_per_pkg(X86CPUTopoInfo *topo_info)
+{
+ return topo_info->threads_per_core * x86_cores_per_pkg(topo_info);
+}
+
+static inline unsigned x86_threads_per_module(X86CPUTopoInfo *topo_info)
+{
+ return topo_info->threads_per_core * topo_info->cores_per_module;
+}
+
+static inline unsigned x86_threads_per_die(X86CPUTopoInfo *topo_info)
+{
+ return x86_threads_per_module(topo_info) * topo_info->modules_per_die;
+}
+
#endif /* HW_I386_TOPOLOGY_H */
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 6d9c85576f6..a58c719e90c 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -312,13 +312,11 @@ static uint32_t num_threads_by_topo_level(X86CPUTopoInfo *topo_info,
case CPU_TOPOLOGY_LEVEL_CORE:
return topo_info->threads_per_core;
case CPU_TOPOLOGY_LEVEL_MODULE:
- return topo_info->threads_per_core * topo_info->cores_per_module;
+ return x86_threads_per_module(topo_info);
case CPU_TOPOLOGY_LEVEL_DIE:
- return topo_info->threads_per_core * topo_info->cores_per_module *
- topo_info->modules_per_die;
+ return x86_threads_per_die(topo_info);
case CPU_TOPOLOGY_LEVEL_SOCKET:
- return topo_info->threads_per_core * topo_info->cores_per_module *
- topo_info->modules_per_die * topo_info->dies_per_pkg;
+ return x86_threads_per_pkg(topo_info);
default:
g_assert_not_reached();
}
@@ -6506,8 +6504,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
topo_info.cores_per_module = cs->nr_cores / env->nr_dies / env->nr_modules;
topo_info.threads_per_core = cs->nr_threads;
- threads_per_pkg = topo_info.threads_per_core * topo_info.cores_per_module *
- topo_info.modules_per_die * topo_info.dies_per_pkg;
+ threads_per_pkg = x86_threads_per_pkg(&topo_info);
/* Calculate & apply limits for different index ranges */
if (index >= 0xC0000000) {
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 34/38] i386/cpu: Track a X86CPUTopoInfo directly in CPUX86State
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (32 preceding siblings ...)
2025-01-10 18:46 ` [PULL 33/38] i386/topology: Introduce helpers for various topology info of different level Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 35/38] i386/cpu: Hoist check of CPUID_EXT3_TOPOEXT against threads_per_core Paolo Bonzini
` (3 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Xiaoyao Li
From: Xiaoyao Li <xiaoyao.li@intel.com>
The name of nr_modules/nr_dies are ambiguous and they mislead people.
The purpose of them is to record and form the topology information. So
just maintain a X86CPUTopoInfo member in CPUX86State instead. Then
nr_modules and nr_dies can be dropped.
As the benefit, x86 can switch to use information in
CPUX86State::topo_info and get rid of the nr_cores and nr_threads in
CPUState. This helps remove the dependency on qemu_init_vcpu(), so that
x86 can get and use topology info earlier in x86_cpu_realizefn(); drop
the comment that highlighted the depedency.
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Link: https://lore.kernel.org/r/20241219110125.1266461-7-xiaoyao.li@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/cpu.h | 6 +----
hw/i386/x86-common.c | 12 ++++------
target/i386/cpu-system.c | 6 ++---
target/i386/cpu.c | 51 +++++++++++++++++-----------------------
4 files changed, 30 insertions(+), 45 deletions(-)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index e8c46d877e0..b26e25ba15e 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2068,11 +2068,7 @@ typedef struct CPUArchState {
TPRAccess tpr_access_type;
- /* Number of dies within this CPU package. */
- unsigned nr_dies;
-
- /* Number of modules within one die. */
- unsigned nr_modules;
+ X86CPUTopoInfo topo_info;
/* Bitmap of available CPU topology levels for this CPU. */
DECLARE_BITMAP(avail_cpu_topo, CPU_TOPOLOGY_LEVEL__MAX);
diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
index 5b0629f9ad3..d5a44af2433 100644
--- a/hw/i386/x86-common.c
+++ b/hw/i386/x86-common.c
@@ -248,7 +248,7 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
CPUX86State *env = &cpu->env;
MachineState *ms = MACHINE(hotplug_dev);
X86MachineState *x86ms = X86_MACHINE(hotplug_dev);
- X86CPUTopoInfo topo_info;
+ X86CPUTopoInfo *topo_info = &env->topo_info;
if (!object_dynamic_cast(OBJECT(cpu), ms->cpu_type)) {
error_setg(errp, "Invalid CPU type, expected cpu type: '%s'",
@@ -267,15 +267,13 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
}
}
- init_topo_info(&topo_info, x86ms);
+ init_topo_info(topo_info, x86ms);
if (ms->smp.modules > 1) {
- env->nr_modules = ms->smp.modules;
set_bit(CPU_TOPOLOGY_LEVEL_MODULE, env->avail_cpu_topo);
}
if (ms->smp.dies > 1) {
- env->nr_dies = ms->smp.dies;
set_bit(CPU_TOPOLOGY_LEVEL_DIE, env->avail_cpu_topo);
}
@@ -346,12 +344,12 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
topo_ids.module_id = cpu->module_id;
topo_ids.core_id = cpu->core_id;
topo_ids.smt_id = cpu->thread_id;
- cpu->apic_id = x86_apicid_from_topo_ids(&topo_info, &topo_ids);
+ cpu->apic_id = x86_apicid_from_topo_ids(topo_info, &topo_ids);
}
cpu_slot = x86_find_cpu_slot(MACHINE(x86ms), cpu->apic_id, &idx);
if (!cpu_slot) {
- x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
+ x86_topo_ids_from_apicid(cpu->apic_id, topo_info, &topo_ids);
error_setg(errp,
"Invalid CPU [socket: %u, die: %u, module: %u, core: %u, thread: %u]"
@@ -374,7 +372,7 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
/* TODO: move socket_id/core_id/thread_id checks into x86_cpu_realizefn()
* once -smp refactoring is complete and there will be CPU private
* CPUState::nr_cores and CPUState::nr_threads fields instead of globals */
- x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
+ x86_topo_ids_from_apicid(cpu->apic_id, topo_info, &topo_ids);
if (cpu->socket_id != -1 && cpu->socket_id != topo_ids.pkg_id) {
error_setg(errp, "property socket-id: %u doesn't match set apic-id:"
" 0x%x (socket-id: %u)", cpu->socket_id, cpu->apic_id,
diff --git a/target/i386/cpu-system.c b/target/i386/cpu-system.c
index eb38cca68ff..b56a2821af2 100644
--- a/target/i386/cpu-system.c
+++ b/target/i386/cpu-system.c
@@ -312,11 +312,11 @@ void x86_cpu_get_crash_info_qom(Object *obj, Visitor *v,
uint64_t cpu_x86_get_msr_core_thread_count(X86CPU *cpu)
{
- CPUState *cs = CPU(cpu);
+ CPUX86State *env = &cpu->env;
uint64_t val;
- val = cs->nr_threads * cs->nr_cores; /* thread count, bits 15..0 */
- val |= ((uint32_t)cs->nr_cores << 16); /* core count, bits 31..16 */
+ val = x86_threads_per_pkg(&env->topo_info); /* thread count, bits 15..0 */
+ val |= x86_cores_per_pkg(&env->topo_info) << 16; /* core count, bits 31..16 */
return val;
}
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index a58c719e90c..1797bd8c071 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6496,15 +6496,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
CPUState *cs = env_cpu(env);
uint32_t limit;
uint32_t signature[3];
- X86CPUTopoInfo topo_info;
+ X86CPUTopoInfo *topo_info = &env->topo_info;
uint32_t threads_per_pkg;
- topo_info.dies_per_pkg = env->nr_dies;
- topo_info.modules_per_die = env->nr_modules;
- topo_info.cores_per_module = cs->nr_cores / env->nr_dies / env->nr_modules;
- topo_info.threads_per_core = cs->nr_threads;
-
- threads_per_pkg = x86_threads_per_pkg(&topo_info);
+ threads_per_pkg = x86_threads_per_pkg(topo_info);
/* Calculate & apply limits for different index ranges */
if (index >= 0xC0000000) {
@@ -6581,12 +6576,12 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
*eax &= ~0xFC000000;
- *eax |= max_core_ids_in_package(&topo_info) << 26;
+ *eax |= max_core_ids_in_package(topo_info) << 26;
if (host_vcpus_per_cache > threads_per_pkg) {
*eax &= ~0x3FFC000;
/* Share the cache at package level. */
- *eax |= max_thread_ids_for_cache(&topo_info,
+ *eax |= max_thread_ids_for_cache(topo_info,
CPU_TOPOLOGY_LEVEL_SOCKET) << 14;
}
}
@@ -6598,7 +6593,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
switch (count) {
case 0: /* L1 dcache info */
encode_cache_cpuid4(env->cache_info_cpuid4.l1d_cache,
- &topo_info,
+ topo_info,
eax, ebx, ecx, edx);
if (!cpu->l1_cache_per_core) {
*eax &= ~MAKE_64BIT_MASK(14, 12);
@@ -6606,7 +6601,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
break;
case 1: /* L1 icache info */
encode_cache_cpuid4(env->cache_info_cpuid4.l1i_cache,
- &topo_info,
+ topo_info,
eax, ebx, ecx, edx);
if (!cpu->l1_cache_per_core) {
*eax &= ~MAKE_64BIT_MASK(14, 12);
@@ -6614,13 +6609,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
break;
case 2: /* L2 cache info */
encode_cache_cpuid4(env->cache_info_cpuid4.l2_cache,
- &topo_info,
+ topo_info,
eax, ebx, ecx, edx);
break;
case 3: /* L3 cache info */
if (cpu->enable_l3_cache) {
encode_cache_cpuid4(env->cache_info_cpuid4.l3_cache,
- &topo_info,
+ topo_info,
eax, ebx, ecx, edx);
break;
}
@@ -6703,12 +6698,12 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
switch (count) {
case 0:
- *eax = apicid_core_offset(&topo_info);
- *ebx = topo_info.threads_per_core;
+ *eax = apicid_core_offset(topo_info);
+ *ebx = topo_info->threads_per_core;
*ecx |= CPUID_B_ECX_TOPO_LEVEL_SMT << 8;
break;
case 1:
- *eax = apicid_pkg_offset(&topo_info);
+ *eax = apicid_pkg_offset(topo_info);
*ebx = threads_per_pkg;
*ecx |= CPUID_B_ECX_TOPO_LEVEL_CORE << 8;
break;
@@ -6734,7 +6729,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
break;
}
- encode_topo_cpuid1f(env, count, &topo_info, eax, ebx, ecx, edx);
+ encode_topo_cpuid1f(env, count, topo_info, eax, ebx, ecx, edx);
break;
case 0xD: {
/* Processor Extended State */
@@ -7037,7 +7032,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
* thread ID within a package".
* Bits 7:0 is "The number of threads in the package is NC+1"
*/
- *ecx = (apicid_pkg_offset(&topo_info) << 12) |
+ *ecx = (apicid_pkg_offset(topo_info) << 12) |
(threads_per_pkg - 1);
} else {
*ecx = 0;
@@ -7066,19 +7061,19 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
switch (count) {
case 0: /* L1 dcache info */
encode_cache_cpuid8000001d(env->cache_info_amd.l1d_cache,
- &topo_info, eax, ebx, ecx, edx);
+ topo_info, eax, ebx, ecx, edx);
break;
case 1: /* L1 icache info */
encode_cache_cpuid8000001d(env->cache_info_amd.l1i_cache,
- &topo_info, eax, ebx, ecx, edx);
+ topo_info, eax, ebx, ecx, edx);
break;
case 2: /* L2 cache info */
encode_cache_cpuid8000001d(env->cache_info_amd.l2_cache,
- &topo_info, eax, ebx, ecx, edx);
+ topo_info, eax, ebx, ecx, edx);
break;
case 3: /* L3 cache info */
encode_cache_cpuid8000001d(env->cache_info_amd.l3_cache,
- &topo_info, eax, ebx, ecx, edx);
+ topo_info, eax, ebx, ecx, edx);
break;
default: /* end of info */
*eax = *ebx = *ecx = *edx = 0;
@@ -7090,7 +7085,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
break;
case 0x8000001E:
if (cpu->core_id <= 255) {
- encode_topo_cpuid8000001e(cpu, &topo_info, eax, ebx, ecx, edx);
+ encode_topo_cpuid8000001e(cpu, topo_info, eax, ebx, ecx, edx);
} else {
*eax = 0;
*ebx = 0;
@@ -7997,17 +7992,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
* fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
* based on inputs (sockets,cores,threads), it is still better to give
* users a warning.
- *
- * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise
- * cs->nr_threads hasn't be populated yet and the checking is incorrect.
*/
if (IS_AMD_CPU(env) &&
!(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
- cs->nr_threads > 1) {
+ env->topo_info.threads_per_core > 1) {
warn_report_once("This family of AMD CPU doesn't support "
"hyperthreading(%d). Please configure -smp "
"options properly or try enabling topoext "
- "feature.", cs->nr_threads);
+ "feature.", env->topo_info.threads_per_core);
}
#ifndef CONFIG_USER_ONLY
@@ -8168,8 +8160,7 @@ static void x86_cpu_init_default_topo(X86CPU *cpu)
{
CPUX86State *env = &cpu->env;
- env->nr_modules = 1;
- env->nr_dies = 1;
+ env->topo_info = (X86CPUTopoInfo) {1, 1, 1, 1};
/* thread, core and socket levels are set by default. */
set_bit(CPU_TOPOLOGY_LEVEL_THREAD, env->avail_cpu_topo);
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 35/38] i386/cpu: Hoist check of CPUID_EXT3_TOPOEXT against threads_per_core
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (33 preceding siblings ...)
2025-01-10 18:46 ` [PULL 34/38] i386/cpu: Track a X86CPUTopoInfo directly in CPUX86State Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 36/38] cpu: Remove nr_cores from struct CPUState Paolo Bonzini
` (2 subsequent siblings)
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Xiaoyao Li
From: Xiaoyao Li <xiaoyao.li@intel.com>
Now it changes to use env->topo_info.threads_per_core and doesn't depend
on qemu_init_vcpu() anymore. Put it together with other feature checks
before qemu_init_vcpu()
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Link: https://lore.kernel.org/r/20241219110125.1266461-8-xiaoyao.li@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/cpu.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 1797bd8c071..3f9475b4856 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7883,6 +7883,21 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
*/
cpu->mwait.ecx |= CPUID_MWAIT_EMX | CPUID_MWAIT_IBE;
+ /*
+ * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
+ * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
+ * based on inputs (sockets,cores,threads), it is still better to give
+ * users a warning.
+ */
+ if (IS_AMD_CPU(env) &&
+ !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
+ env->topo_info.threads_per_core > 1) {
+ warn_report_once("This family of AMD CPU doesn't support "
+ "hyperthreading(%d). Please configure -smp "
+ "options properly or try enabling topoext "
+ "feature.", env->topo_info.threads_per_core);
+ }
+
/* For 64bit systems think about the number of physical bits to present.
* ideally this should be the same as the host; anything other than matching
* the host can cause incorrect guest behaviour.
@@ -7987,21 +8002,6 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
x86_cpu_gdb_init(cs);
qemu_init_vcpu(cs);
- /*
- * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
- * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
- * based on inputs (sockets,cores,threads), it is still better to give
- * users a warning.
- */
- if (IS_AMD_CPU(env) &&
- !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
- env->topo_info.threads_per_core > 1) {
- warn_report_once("This family of AMD CPU doesn't support "
- "hyperthreading(%d). Please configure -smp "
- "options properly or try enabling topoext "
- "feature.", env->topo_info.threads_per_core);
- }
-
#ifndef CONFIG_USER_ONLY
x86_cpu_apic_realize(cpu, &local_err);
if (local_err != NULL) {
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 36/38] cpu: Remove nr_cores from struct CPUState
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (34 preceding siblings ...)
2025-01-10 18:46 ` [PULL 35/38] i386/cpu: Hoist check of CPUID_EXT3_TOPOEXT against threads_per_core Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 37/38] i386/cpu: Set up CPUID_HT in x86_cpu_expand_features() instead of cpu_x86_cpuid() Paolo Bonzini
2025-01-10 18:46 ` [PULL 38/38] i386/cpu: Set and track CPUID_EXT3_CMP_LEG in env->features[FEAT_8000_0001_ECX] Paolo Bonzini
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Xiaoyao Li, Zhao Liu
From: Xiaoyao Li <xiaoyao.li@intel.com>
There is no user of it now, remove it.
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
Link: https://lore.kernel.org/r/20241219110125.1266461-9-xiaoyao.li@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/hw/core/cpu.h | 2 --
hw/core/cpu-common.c | 1 -
system/cpus.c | 1 -
3 files changed, 4 deletions(-)
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index c3ca0babcb3..fb397cdfc53 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -407,7 +407,6 @@ struct qemu_work_item;
* Under TCG this value is propagated to @tcg_cflags.
* See TranslationBlock::TCG CF_CLUSTER_MASK.
* @tcg_cflags: Pre-computed cflags for this cpu.
- * @nr_cores: Number of cores within this CPU package.
* @nr_threads: Number of threads within this CPU core.
* @thread: Host thread details, only live once @created is #true
* @sem: WIN32 only semaphore used only for qtest
@@ -466,7 +465,6 @@ struct CPUState {
CPUClass *cc;
/*< public >*/
- int nr_cores;
int nr_threads;
struct QemuThread *thread;
diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 1edc16f65c8..cb79566cc51 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -243,7 +243,6 @@ static void cpu_common_initfn(Object *obj)
cpu->cluster_index = UNASSIGNED_CLUSTER_INDEX;
/* user-mode doesn't have configurable SMP topology */
/* the default value is changed by qemu_init_vcpu() for system-mode */
- cpu->nr_cores = 1;
cpu->nr_threads = 1;
cpu->cflags_next_tb = -1;
diff --git a/system/cpus.c b/system/cpus.c
index 99f83806c16..37e5892c240 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -687,7 +687,6 @@ void qemu_init_vcpu(CPUState *cpu)
{
MachineState *ms = MACHINE(qdev_get_machine());
- cpu->nr_cores = machine_topo_get_cores_per_socket(ms);
cpu->nr_threads = ms->smp.threads;
cpu->stopped = true;
cpu->random_seed = qemu_guest_random_seed_thread_part1();
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 37/38] i386/cpu: Set up CPUID_HT in x86_cpu_expand_features() instead of cpu_x86_cpuid()
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (35 preceding siblings ...)
2025-01-10 18:46 ` [PULL 36/38] cpu: Remove nr_cores from struct CPUState Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
2025-01-10 18:46 ` [PULL 38/38] i386/cpu: Set and track CPUID_EXT3_CMP_LEG in env->features[FEAT_8000_0001_ECX] Paolo Bonzini
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Xiaoyao Li
From: Xiaoyao Li <xiaoyao.li@intel.com>
Currently CPUID_HT is evaluated in cpu_x86_cpuid() each time. It's not a
correct usage of how feature bit is maintained and evaluated. The
expected practice is that features are tracked in env->features[] and
cpu_x86_cpuid() should be the consumer of env->features[].
Track CPUID_HT in env->features[FEAT_1_EDX] instead and evaluate it in
cpu's realizefn().
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Link: https://lore.kernel.org/r/20241219110125.1266461-10-xiaoyao.li@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/cpu.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 3f9475b4856..3f0821c15f2 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6538,7 +6538,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
*edx = env->features[FEAT_1_EDX];
if (threads_per_pkg > 1) {
*ebx |= threads_per_pkg << 16;
- *edx |= CPUID_HT;
}
if (!cpu->enable_pmu) {
*ecx &= ~CPUID_EXT_PDCM;
@@ -7529,6 +7528,10 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
}
}
+ if (x86_threads_per_pkg(&env->topo_info) > 1) {
+ env->features[FEAT_1_EDX] |= CPUID_HT;
+ }
+
for (i = 0; i < ARRAY_SIZE(feature_dependencies); i++) {
FeatureDep *d = &feature_dependencies[i];
if (!(env->features[d->from.index] & d->from.mask)) {
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PULL 38/38] i386/cpu: Set and track CPUID_EXT3_CMP_LEG in env->features[FEAT_8000_0001_ECX]
2025-01-10 18:45 [PULL 00/38] Rust, i386 patches for 2024-01-10 Paolo Bonzini
` (36 preceding siblings ...)
2025-01-10 18:46 ` [PULL 37/38] i386/cpu: Set up CPUID_HT in x86_cpu_expand_features() instead of cpu_x86_cpuid() Paolo Bonzini
@ 2025-01-10 18:46 ` Paolo Bonzini
37 siblings, 0 replies; 42+ messages in thread
From: Paolo Bonzini @ 2025-01-10 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Xiaoyao Li
From: Xiaoyao Li <xiaoyao.li@intel.com>
The correct usage is tracking and maintaining features in env->features[]
instead of manually set it in cpu_x86_cpuid().
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Link: https://lore.kernel.org/r/20241219110125.1266461-11-xiaoyao.li@intel.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/cpu.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 3f0821c15f2..1b9c11022c4 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6953,17 +6953,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
*ecx = env->features[FEAT_8000_0001_ECX];
*edx = env->features[FEAT_8000_0001_EDX];
- /* The Linux kernel checks for the CMPLegacy bit and
- * discards multiple thread information if it is set.
- * So don't set it here for Intel to make Linux guests happy.
- */
- if (threads_per_pkg > 1) {
- if (env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1 ||
- env->cpuid_vendor2 != CPUID_VENDOR_INTEL_2 ||
- env->cpuid_vendor3 != CPUID_VENDOR_INTEL_3) {
- *ecx |= 1 << 1; /* CmpLegacy bit */
- }
- }
if (tcg_enabled() && env->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 &&
!(env->hflags & HF_LMA_MASK)) {
*edx &= ~CPUID_EXT2_SYSCALL;
@@ -7530,6 +7519,15 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
if (x86_threads_per_pkg(&env->topo_info) > 1) {
env->features[FEAT_1_EDX] |= CPUID_HT;
+
+ /*
+ * The Linux kernel checks for the CMPLegacy bit and
+ * discards multiple thread information if it is set.
+ * So don't set it here for Intel to make Linux guests happy.
+ */
+ if (!IS_INTEL_CPU(env)) {
+ env->features[FEAT_8000_0001_ECX] |= CPUID_EXT3_CMP_LEG;
+ }
}
for (i = 0; i < ARRAY_SIZE(feature_dependencies); i++) {
--
2.47.1
^ permalink raw reply related [flat|nested] 42+ messages in thread