From 122748c83d2a331b43ea17efd78c4117a362f3f2 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 19 Dec 2024 16:37:20 +0100 Subject: [PATCH 01/38] rust: fix --enable-debug-mutex --feature is an option for cargo but not for rustc. Reported-by: Bernhard Beschow Reviewed-by: Bernhard Beschow Signed-off-by: Paolo Bonzini --- 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 ccb20f38c1..9425ba7100 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( From 1d03e9771e05685e11bbd3cc8cdd072c02cf580d Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 7 Jan 2025 16:52:25 +0100 Subject: [PATCH 02/38] rust: add --check-cfg test to rustc arguments 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 --- 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 5525b3886f..2633157df2 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("cfg(test)") for cfg in sorted(cargo_toml.check_cfg): print("--check-cfg") print(cfg) From ca0d60a6ad777ab617cbc4e6f328eaff60617b3f Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 11 Dec 2024 11:38:20 +0100 Subject: [PATCH 03/38] rust: qom: add ParentField 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, ... } impl Drop for MySuperclass { ... } pub struct MySubclass { parent: MySuperclass, ... } and an instance_finalize implementation that is like unsafe extern "C" fn drop_object(obj: *mut Object) { unsafe { std::ptr::drop_in_place(obj.cast::()) } } 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. 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 Signed-off-by: Paolo Bonzini --- 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 18cc122951..689202f455 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -14,7 +14,7 @@ use qemu_api::{ irq::InterruptSource, prelude::*, qdev::DeviceImpl, - qom::ObjectImpl, + qom::{ObjectImpl, ParentField}, }; use crate::{ @@ -86,7 +86,7 @@ impl std::ops::Index for Fifo { #[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, pub iomem: MemoryRegion, #[doc(alias = "fr")] pub flags: registers::Flags, @@ -645,7 +645,7 @@ pub unsafe extern "C" fn pl011_create( #[derive(Debug, qemu_api_macros::Object)] /// PL011 Luminary device model. pub struct PL011Luminary { - parent_obj: PL011State, + parent_obj: ParentField, } impl PL011Luminary { diff --git a/rust/qemu-api/src/qom.rs b/rust/qemu-api/src/qom.rs index 7d5fbef1e1..40d17a92e1 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 @@ macro_rules! qom_isa { }; } +/// This is the same as [`ManuallyDrop`](std::mem::ManuallyDrop), though +/// it hides the standard methods of `ManuallyDrop`. +/// +/// The first field of an `ObjectType` must be of type `ParentField`. +/// (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, +/// ... +/// } +/// ``` +#[derive(Debug)] +#[repr(transparent)] +pub struct ParentField(std::mem::ManuallyDrop); + +impl Deref for ParentField { + type Target = T; + + #[inline(always)] + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl DerefMut for ParentField { + #[inline(always)] + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +impl fmt::Display for ParentField { + #[inline(always)] + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> { + self.0.fmt(f) + } +} + unsafe extern "C" fn rust_instance_init(obj: *mut Object) { // SAFETY: obj is an instance of T, since rust_instance_init // is called from QOM core as the instance_init function @@ -151,11 +198,16 @@ unsafe extern "C" fn rust_class_init>( /// /// - 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`](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 ObjectCastMut for &mut T {} /// Trait a type must implement to be registered with QEMU. pub trait ObjectImpl: ObjectType + ClassInitImpl { - /// 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 1d2825b098..526c3f4f8e 100644 --- a/rust/qemu-api/tests/tests.rs +++ b/rust/qemu-api/tests/tests.rs @@ -15,7 +15,7 @@ use qemu_api::{ declare_properties, define_property, prelude::*, qdev::{DeviceImpl, DeviceState, Property}, - qom::ObjectImpl, + qom::{ObjectImpl, ParentField}, vmstate::VMStateDescription, zeroable::Zeroable, }; @@ -31,7 +31,7 @@ pub static VMSTATE: VMStateDescription = VMStateDescription { #[repr(C)] #[derive(qemu_api_macros::Object)] pub struct DummyState { - parent: DeviceState, + parent: ParentField, migrate_clock: bool, } From 7f65d4e58b67d6e5ee05c9381a50ef1eba3a5a1e Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 11 Dec 2024 12:18:06 +0100 Subject: [PATCH 04/38] rust: add a utility module for compile-time type checks 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 Signed-off-by: Paolo Bonzini --- 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 9425ba7100..60944a657d 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 0000000000..6e42046980 --- /dev/null +++ b/rust/qemu-api/src/assertions.rs @@ -0,0 +1,90 @@ +// Copyright 2024, Red Hat Inc. +// Author(s): Paolo Bonzini +// 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 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!( as Deref>::Target, u32); +/// ``` +/// +/// Different types will cause a compile failure +/// +/// ```compile_fail +/// # use qemu_api::assert_same_type; +/// assert_same_type!(&Box, &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) + where + T: $crate::assertions::EqType, + { + } + 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) + where + T: $crate::assertions::EqType, + { + } + types_must_be_equal::<_, $ti>(v.$i); + } + }; + }; +} diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs index 4b43e02c0f..83c6a987c0 100644 --- a/rust/qemu-api/src/lib.rs +++ b/rust/qemu-api/src/lib.rs @@ -12,6 +12,7 @@ pub mod bindings; #[rustfmt::skip] pub mod prelude; +pub mod assertions; pub mod bitops; pub mod c_str; pub mod callbacks; From 20f0b8e98b4851bfd52bbb4edd4b602d08b9b817 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 24 Oct 2024 11:57:02 +0200 Subject: [PATCH 05/38] rust: macros: check that #[derive(Object)] requires #[repr(C)] Convert derive_object to the same pattern of first making a Result, 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 Signed-off-by: Paolo Bonzini --- 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 74a8bc7503..160b283d7f 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 { + 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) } From e3ff5a17aa9fc2420197403e69876db48e390ee4 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 11 Dec 2024 11:48:44 +0100 Subject: [PATCH 06/38] rust: macros: check that the first field of a #[derive(Object)] struct is a ParentField Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- 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 160b283d7f..0f04cca384 100644 --- a/rust/qemu-api-macros/src/lib.rs +++ b/rust/qemu-api-macros/src/lib.rs @@ -19,6 +19,27 @@ impl From for proc_macro2::TokenStream { } } +fn get_fields<'a>( + input: &'a DeriveInput, + msg: &str, +) -> Result<&'a Punctuated, 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::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, 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 { 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(); From 33aa660575f7174752a7308229e5fc108921c2a6 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 11 Dec 2024 10:33:31 +0100 Subject: [PATCH 07/38] rust: qom: automatically use Drop trait to implement instance_finalize Replace the customizable INSTANCE_FINALIZE with a generic function that drops the Rust object. Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- 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 40d17a92e1..b0332ba247 100644 --- a/rust/qemu-api/src/qom.rs +++ b/rust/qemu-api/src/qom.rs @@ -180,6 +180,16 @@ unsafe extern "C" fn rust_class_init>( T::class_init(unsafe { &mut *klass.cast::() }) } +unsafe extern "C" fn drop_object(obj: *mut Object) { + // SAFETY: obj is an instance of T, since drop_object 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::()) } +} + /// Trait exposed by all structs corresponding to QOM objects. /// /// # Safety @@ -442,7 +452,6 @@ pub trait ObjectImpl: ObjectType + ClassInitImpl { /// Whether the object can be instantiated const ABSTRACT: bool = false; - const INSTANCE_FINALIZE: Option = 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 { None => None, Some(_) => Some(rust_instance_post_init::), }, - instance_finalize: Self::INSTANCE_FINALIZE, + instance_finalize: Some(drop_object::), abstract_: Self::ABSTRACT, class_size: core::mem::size_of::(), class_init: Some(rust_class_init::), From d9434f29ca83e114fe02ed24c8ad2ccfa7ac3fe9 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 29 Nov 2024 11:38:59 +0100 Subject: [PATCH 08/38] rust: qom: move device_id to PL011 class side 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 implementation because arrays can only be indexed by usize. Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- 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 689202f455..215f94a6e4 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 @@ use qemu_api::{ irq::InterruptSource, prelude::*, qdev::DeviceImpl, - qom::{ObjectImpl, ParentField}, + qom::{ClassInitImpl, ObjectImpl, ParentField}, }; use crate::{ @@ -33,27 +33,20 @@ const FBRD_MASK: u32 = 0x3f; /// 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 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, #[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: ::Class, + /// The byte string that identifies the device. + device_id: DeviceId, +} + unsafe impl ObjectType for PL011State { - type Class = ::Class; + type Class = PL011Class; const TYPE_NAME: &'static CStr = crate::TYPE_PL011; } +impl ClassInitImpl for PL011State { + fn class_init(klass: &mut PL011Class) { + klass.device_id = DeviceId::ARM; + >::class_init(&mut klass.parent_class); + } +} + impl ObjectImpl for PL011State { type ParentType = SysBusDevice; @@ -214,7 +218,8 @@ impl PL011State { 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, } -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 for PL011Luminary { + fn class_init(klass: &mut PL011Class) { + klass.device_id = DeviceId::LUMINARY; + >::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 = Some(Self::init); } impl DeviceImpl for PL011Luminary {} From af68b41d403b81b18de07ebab0ca4c1025c94bf7 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 2 Dec 2024 13:16:19 +0100 Subject: [PATCH 09/38] rust: pl011: only leave embedded object initialization in instance_init 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 Signed-off-by: Paolo Bonzini --- 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 215f94a6e4..72a4cea042 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 = Some(Self::init); + const INSTANCE_POST_INIT: Option = Some(Self::post_init); } impl DeviceImpl for PL011State { @@ -183,14 +184,6 @@ impl PL011State { 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 @@ impl PL011State { } } + 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 { use RegisterOffset::*; diff --git a/rust/qemu-api/src/sysbus.rs b/rust/qemu-api/src/sysbus.rs index 8193734bde..b96eaaf25f 100644 --- a/rust/qemu-api/src/sysbus.rs +++ b/rust/qemu-api/src/sysbus.rs @@ -38,6 +38,18 @@ impl 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 From 22a18f0a98a1ca5d0cd8fff1d81cc74a269083de Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 29 Nov 2024 08:48:07 +0100 Subject: [PATCH 10/38] rust: qom: make INSTANCE_POST_INIT take a shared reference Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- 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 72a4cea042..6792d13fb7 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 = Some(Self::init); - const INSTANCE_POST_INIT: Option = Some(Self::post_init); + const INSTANCE_POST_INIT: Option = Some(Self::post_init); } impl DeviceImpl for PL011State { @@ -206,7 +206,7 @@ impl PL011State { } } - 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 b0332ba247..97901fb908 100644 --- a/rust/qemu-api/src/qom.rs +++ b/rust/qemu-api/src/qom.rs @@ -163,11 +163,7 @@ unsafe extern "C" fn rust_instance_post_init(obj: *mut Object) { // SAFETY: obj is an instance of T, since rust_instance_post_init // 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::INSTANCE_POST_INIT.unwrap()(unsafe { &*obj.cast::() }) } unsafe extern "C" fn rust_class_init>( @@ -463,7 +459,7 @@ pub trait ObjectImpl: ObjectType + ClassInitImpl { /// Function that is called to finish initialization of an object, once /// `INSTANCE_INIT` functions have been called. - const INSTANCE_POST_INIT: Option = None; + const INSTANCE_POST_INIT: Option = None; /// Called on descendent classes after all parent class initialization /// has occurred, but before the class itself is initialized. This From a3b620fff73ec762f2a77a077eb8389dddab4833 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 4 Dec 2024 08:57:27 +0100 Subject: [PATCH 11/38] rust: qemu-api-macros: extend error reporting facility to parse errors Generalize the CompileError tuple to an enum, that can be either an error message or a parse error from syn. Reviewed-by: Zhao Liu Signed-off-by: Paolo Bonzini --- 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 0f04cca384..539c48df29 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 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, CompileError> { +) -> Result<&'a Punctuated, 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 { +fn derive_object_or_error(input: DeriveInput) -> Result { 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 { +fn derive_offsets_or_error(input: DeriveInput) -> Result { 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 0000000000..02c91aed7f --- /dev/null +++ b/rust/qemu-api-macros/src/utils.rs @@ -0,0 +1,26 @@ +// Procedural macro utilities. +// Author(s): Paolo Bonzini +// 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 for MacroError { + fn from(err: syn::Error) -> Self { + MacroError::ParseError(err) + } +} + +impl From 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(), + } + } +} From 809c703a60240125eec16ec134f60793134b4f61 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Wed, 4 Dec 2024 08:58:46 +0100 Subject: [PATCH 12/38] rust: qemu-api-macros: add automatic TryFrom/TryInto derivation 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 Signed-off-by: Paolo Bonzini --- 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 69064d6929..0a89d393e0 100644 --- a/rust/hw/char/pl011/src/lib.rs +++ b/rust/hw/char/pl011/src/lib.rs @@ -45,7 +45,7 @@ pub const TYPE_PL011_LUMINARY: &::std::ffi::CStr = c_str!("pl011_luminary"); #[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 for RegisterOffset { - type Error = u64; - - fn try_from(value: u64) -> Result { - 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 539c48df29..7ec218202f 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 { + let repr = input.attrs.iter().find(|attr| attr.path().is_ident("repr")); + if let Some(repr) = repr { + let nested = repr.parse_args_with(Punctuated::::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, 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 { + 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 { + #(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) +} From 559a779c6aa309853474240b01fcc2beff1f04ca Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 29 Nov 2024 10:46:44 +0100 Subject: [PATCH 13/38] rust: qdev: expose inherited methods to subclasses of SysBusDevice 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 Signed-off-by: Paolo Bonzini --- 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 6792d13fb7..994c2fc059 100644 --- a/rust/hw/char/pl011/src/device.rs +++ b/rust/hw/char/pl011/src/device.rs @@ -207,11 +207,9 @@ impl PL011State { } 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 6258141bdf..378e520295 100644 --- a/rust/qemu-api/src/irq.rs +++ b/rust/qemu-api/src/irq.rs @@ -24,8 +24,7 @@ use crate::{ /// /// 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 6f32deeb2e..4ea70b9c82 100644 --- a/rust/qemu-api/src/prelude.rs +++ b/rust/qemu-api/src/prelude.rs @@ -16,3 +16,5 @@ pub use crate::qom::ObjectMethods; 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 b96eaaf25f..e6762b5c14 100644 --- a/rust/qemu-api/src/sysbus.rs +++ b/rust/qemu-api/src/sysbus.rs @@ -32,18 +32,17 @@ where } } -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, +{ /// 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 @@ impl SysBusDevice { /// 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 SysBusDeviceMethods for R where R::Target: IsA {} From d2c12785be06da708fe1c8e4e81d7134f4f3c56a Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 20 Dec 2024 13:10:03 +0100 Subject: [PATCH 14/38] rust: hide warnings for subprojects 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 --- 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 e580538a87..a1838b20b0 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 7a4339d298..900bb1497b 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 b24c34a904..d14c3dc769 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 6046712036..352e11cfee 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 66b05252cd..ee12d0053b 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 cff3f62ce7..00733d1faa 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 e69bac91b4..ce13d0fe80 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 f8f3486fc0..42b03dcd53 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 608e64e31f..04c96cc5fb 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 30982a4ee7..2a3fbe9ee5 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 ae27a69686..10c2741085 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 3281b26433..c4c4c5e397 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 f9c8675eba..5759df3ecc 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 7f7792569b..bf41fad99b 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 2c62cf7e1b..a009417408 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 9d76ebbd1a..11a5dab97d 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 b7db03b06a..59f892f782 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 d13d8a239a..5aeb224a10 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 7053e2c013..6c9369f0df 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 6e7ea69049..8b721dfa00 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 13ffdac3c3..d79cf750fb 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 4609f96ed9..50988f612e 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 54f2376854..0000000000 --- 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) From b7bd800eba69bab75b8c296ad788df8df8947aae Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 7 Jan 2025 11:42:49 +0100 Subject: [PATCH 15/38] qom: remove unused field 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 --- 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 95d6e064d9..f28ffea9a6 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 b4c52d055d..e9dfad854b 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); From be27b5149c86f81531f8fc609baf3480fc4d9ca0 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 19 Dec 2024 11:24:13 +0100 Subject: [PATCH 16/38] make-release: only leave tarball of wrap-file subprojects 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 Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2719 Signed-off-by: Paolo Bonzini --- scripts/make-release | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/scripts/make-release b/scripts/make-release index 8dc939124c..2885e87210 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} From 88716ae79f89bd6510f0c9e182a73ad40d1ff531 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 17 Oct 2024 12:10:39 +0200 Subject: [PATCH 17/38] target/i386: improve code generation for BT 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 --- 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 785ff63f2a..5c11542935 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) From ef682b08a0b52f4e6d9d790e26291f146e05734a Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 21 Nov 2024 13:01:45 +0100 Subject: [PATCH 18/38] target/i386: use shr to load high-byte registers into T0/T1 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 --- target/i386/tcg/emit.c.inc | 23 ++++++++++++----------- target/i386/tcg/translate.c | 2 +- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc index 5c11542935..c4cc5f48d8 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: diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c index 834aea1e59..dbc9d637c4 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]); } From cf4c263551886964c5d58bd7b675b13fd497b402 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Wed, 6 Nov 2024 11:07:18 +0800 Subject: [PATCH 19/38] i386/cpu: Mark avx10_version filtered when prefix is NULL 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 Reviewed-by: Tao Su Link: https://lore.kernel.org/r/20241106030728.553238-2-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- 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 0b639848cd..579d9bac95 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; } From cee1f341ce0c803ee26deed0a68fa1985391d517 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Wed, 6 Nov 2024 11:07:19 +0800 Subject: [PATCH 20/38] target/i386/kvm: Add feature bit definitions for KVM CPUID 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 Reviewed-by: Zide Chen Link: https://lore.kernel.org/r/20241106030728.553238-3-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- hw/i386/kvm/clock.c | 5 ++--- target/i386/cpu.h | 23 +++++++++++++++++++++++ target/i386/kvm/kvm.c | 28 ++++++++++++++-------------- 3 files changed, 39 insertions(+), 17 deletions(-) diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c index 63be508842..17443552e9 100644 --- a/hw/i386/kvm/clock.c +++ b/hw/i386/kvm/clock.c @@ -27,7 +27,6 @@ #include "qapi/error.h" #include -#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/cpu.h b/target/i386/cpu.h index dbd8f1ffc7..f41462d8c1 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/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index 2f66e63b88..d6fb3bee86 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; } From f5bec7652ddff7e10a0c14aa123352275c720e48 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Wed, 6 Nov 2024 11:07:20 +0800 Subject: [PATCH 21/38] target/i386/kvm: Remove local MSR_KVM_WALL_CLOCK and MSR_KVM_SYSTEM_TIME definitions 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 Reviewed-by: Xiaoyao Li Reviewed-by: Zide Chen Link: https://lore.kernel.org/r/20241106030728.553238-4-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- 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 d6fb3bee86..7870820a2b 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 From 86e032bb7b5ac5a319cef914aa779825fe39a2e2 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Wed, 6 Nov 2024 11:07:21 +0800 Subject: [PATCH 22/38] target/i386/kvm: Only save/load kvmclock MSRs when kvmclock enabled 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 Reviewed-by: Zide Chen Link: https://lore.kernel.org/r/20241106030728.553238-5-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- 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 7870820a2b..7536a3c9fd 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); } From 5dabc87b51030fc08b76b2401a5566f0b1463b59 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Wed, 6 Nov 2024 11:07:23 +0800 Subject: [PATCH 23/38] target/i386/kvm: Drop workaround for KVM_X86_DISABLE_EXITS_HTL typo 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 Reviewed-by: Zide Chen Link: https://lore.kernel.org/r/20241106030728.553238-7-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- 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 7536a3c9fd..17f23607ed 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 | From 26824f9cac69979586b30410ffac8bca4157909e Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Wed, 6 Nov 2024 11:07:24 +0800 Subject: [PATCH 24/38] target/i386/confidential-guest: Fix comment of x86_confidential_guest_kvm_type() Update the comment to match the X86ConfidentialGuestClass implementation. Reported-by: Xiaoyao Li Signed-off-by: Zhao Liu Reviewed-by: Pankaj Gupta Reviewed-by: Zide Chen Link: https://lore.kernel.org/r/20241106030728.553238-8-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- 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 0afb8317b5..164be7633a 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) { From fb81c9cfdd9fc37687a36f41ca07ab0e8a6d9899 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Wed, 6 Nov 2024 11:07:25 +0800 Subject: [PATCH 25/38] target/i386/kvm: Clean up return values of MSR filter related functions 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 Reviewed-by: Zide Chen Link: https://lore.kernel.org/r/20241106030728.553238-9-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- 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 17f23607ed..097a040da3 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) From d7f895cb62f3a709e7b9aeeab19678811deda973 Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Wed, 6 Nov 2024 11:07:26 +0800 Subject: [PATCH 26/38] target/i386/kvm: Return -1 when kvm_msr_energy_thread_init() fails 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 Link: https://lore.kernel.org/r/20241106030728.553238-10-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- 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 097a040da3..3624abbb39 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); } From d2401a6eae8f8fbd8e569c8b0638f0cbc80ec88e Mon Sep 17 00:00:00 2001 From: Zhao Liu Date: Wed, 6 Nov 2024 11:07:27 +0800 Subject: [PATCH 27/38] target/i386/kvm: Clean up error handling in kvm_arch_init() 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 Reviewed-by: Zide Chen Link: https://lore.kernel.org/r/20241106030728.553238-11-zhao1.liu@intel.com Signed-off-by: Paolo Bonzini --- 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 3624abbb39..6f424774b3 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; } } } From d662b66da4bef9272144f7b79715aad90cdbc33e Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 24 Dec 2024 16:59:12 +0100 Subject: [PATCH 28/38] target/i386/kvm: Replace ARRAY_SIZE(msr_handlers) with KVM_MSR_FILTER_MAX_RANGES 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 --- 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 6f424774b3..1d7214b6a6 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++]; From d3bb5d0d4f5d4ad7dc6c02ea5fea51ca2f946593 Mon Sep 17 00:00:00 2001 From: Xiaoyao Li Date: Thu, 19 Dec 2024 06:01:16 -0500 Subject: [PATCH 29/38] i386/cpu: Extract a common fucntion to setup value of MSR_CORE_THREAD_COUNT There are duplicated code to setup the value of MSR_CORE_THREAD_COUNT. Extract a common function for it. Signed-off-by: Xiaoyao Li Reviewed-by: Zhao Liu Link: https://lore.kernel.org/r/20241219110125.1266461-2-xiaoyao.li@intel.com Signed-off-by: Paolo Bonzini --- target/i386/cpu-system.c | 11 +++++++++++ target/i386/cpu.h | 2 ++ 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-system.c b/target/i386/cpu-system.c index 9d007afdab..eb38cca68f 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/cpu.h b/target/i386/cpu.h index f41462d8c1..e8c46d877e 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/hvf/x86_emu.c b/target/i386/hvf/x86_emu.c index 015f760acb..69c61c9c07 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 1d7214b6a6..6c749d4ee8 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 ffed8a3215..c9c4d42f84 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: { From 81bd60625fc23cb8d4d0e682dcc4223d5e1ead84 Mon Sep 17 00:00:00 2001 From: Xiaoyao Li Date: Thu, 19 Dec 2024 06:01:17 -0500 Subject: [PATCH 30/38] i386/cpu: Drop the variable smp_cores and smp_threads in x86_cpu_pre_plug() 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 Reviewed-by: Zhao Liu Link: https://lore.kernel.org/r/20241219110125.1266461-3-xiaoyao.li@intel.com Signed-off-by: Paolo Bonzini --- 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 a7d46c3105..5b0629f9ad 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; } From 00ec7be67c3981b486293aa8e0aef9534f229c5e Mon Sep 17 00:00:00 2001 From: Xiaoyao Li Date: Thu, 19 Dec 2024 06:01:18 -0500 Subject: [PATCH 31/38] i386/cpu: Drop cores_per_pkg in cpu_x86_cpuid() 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 Reviewed-by: Zhao Liu Link: https://lore.kernel.org/r/20241219110125.1266461-4-xiaoyao.li@intel.com Signed-off-by: Paolo Bonzini --- 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 579d9bac95..6d9c85576f 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) { From 8f78378de70fc79fdc7e1318496bd91ddd22df49 Mon Sep 17 00:00:00 2001 From: Xiaoyao Li Date: Thu, 19 Dec 2024 06:01:19 -0500 Subject: [PATCH 32/38] i386/topology: Update the comment of x86_apicid_from_topo_ids() Update the comment of x86_apicid_from_topo_ids() to match the current implementation, Signed-off-by: Xiaoyao Li Reviewed-by: Zhao Liu Link: https://lore.kernel.org/r/20241219110125.1266461-5-xiaoyao.li@intel.com Signed-off-by: Paolo Bonzini --- 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 b2c8bf2de1..21b65219a5 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) From e60cbeec190d349682bf97cf55446e8ae260b11a Mon Sep 17 00:00:00 2001 From: Xiaoyao Li Date: Thu, 19 Dec 2024 06:01:20 -0500 Subject: [PATCH 33/38] i386/topology: Introduce helpers for various topology info of different level 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 Link: https://lore.kernel.org/r/20241219110125.1266461-6-xiaoyao.li@intel.com Signed-off-by: Paolo Bonzini --- 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 21b65219a5..f6380f1ed7 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 6d9c85576f..a58c719e90 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) { From 84b71a131c1bc84c36fafb63271080ecf9f2ff7a Mon Sep 17 00:00:00 2001 From: Xiaoyao Li Date: Thu, 19 Dec 2024 06:01:21 -0500 Subject: [PATCH 34/38] i386/cpu: Track a X86CPUTopoInfo directly in CPUX86State 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 Link: https://lore.kernel.org/r/20241219110125.1266461-7-xiaoyao.li@intel.com Signed-off-by: Paolo Bonzini --- hw/i386/x86-common.c | 12 ++++------ target/i386/cpu-system.c | 6 ++--- target/i386/cpu.c | 51 +++++++++++++++++----------------------- target/i386/cpu.h | 6 +---- 4 files changed, 30 insertions(+), 45 deletions(-) diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c index 5b0629f9ad..d5a44af243 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 eb38cca68f..b56a2821af 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 a58c719e90..1797bd8c07 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); diff --git a/target/i386/cpu.h b/target/i386/cpu.h index e8c46d877e..b26e25ba15 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); From 473d79b56a1645be90b890f9623b27acd0afba49 Mon Sep 17 00:00:00 2001 From: Xiaoyao Li Date: Thu, 19 Dec 2024 06:01:22 -0500 Subject: [PATCH 35/38] i386/cpu: Hoist check of CPUID_EXT3_TOPOEXT against threads_per_core 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 Link: https://lore.kernel.org/r/20241219110125.1266461-8-xiaoyao.li@intel.com Signed-off-by: Paolo Bonzini --- 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 1797bd8c07..3f9475b485 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) { From 6e090ffe0d188e1f09d4efcd10d82158f92abfbb Mon Sep 17 00:00:00 2001 From: Xiaoyao Li Date: Thu, 19 Dec 2024 06:01:23 -0500 Subject: [PATCH 36/38] cpu: Remove nr_cores from struct CPUState There is no user of it now, remove it. Signed-off-by: Xiaoyao Li Reviewed-by: Zhao Liu Link: https://lore.kernel.org/r/20241219110125.1266461-9-xiaoyao.li@intel.com Signed-off-by: Paolo Bonzini --- hw/core/cpu-common.c | 1 - include/hw/core/cpu.h | 2 -- system/cpus.c | 1 - 3 files changed, 4 deletions(-) diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c index 1edc16f65c..cb79566cc5 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/include/hw/core/cpu.h b/include/hw/core/cpu.h index c3ca0babcb..fb397cdfc5 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/system/cpus.c b/system/cpus.c index 99f83806c1..37e5892c24 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(); From c6bd2dd634208ca717b6dc010064fe34d1359080 Mon Sep 17 00:00:00 2001 From: Xiaoyao Li Date: Thu, 19 Dec 2024 06:01:24 -0500 Subject: [PATCH 37/38] i386/cpu: Set up CPUID_HT in x86_cpu_expand_features() instead of cpu_x86_cpuid() 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 Link: https://lore.kernel.org/r/20241219110125.1266461-10-xiaoyao.li@intel.com Signed-off-by: Paolo Bonzini --- 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 3f9475b485..3f0821c15f 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)) { From 99a637a86f55c8486b06c698656befdf012eec4d Mon Sep 17 00:00:00 2001 From: Xiaoyao Li Date: Thu, 19 Dec 2024 06:01:25 -0500 Subject: [PATCH 38/38] i386/cpu: Set and track CPUID_EXT3_CMP_LEG in env->features[FEAT_8000_0001_ECX] 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 Link: https://lore.kernel.org/r/20241219110125.1266461-11-xiaoyao.li@intel.com Signed-off-by: Paolo Bonzini --- 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 3f0821c15f..1b9c11022c 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++) {