* [PATCH 01/10] i386/fw_cfg: move hpet_cfg definition to hpet.c
2025-01-25 12:51 [PATCH 00/10] rust: Add HPET timer device Zhao Liu
@ 2025-01-25 12:51 ` Zhao Liu
2025-01-25 12:51 ` [PATCH 02/10] rust/qdev: add the macro to define bit property Zhao Liu
` (8 subsequent siblings)
9 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-01-25 12:51 UTC (permalink / raw)
To: Paolo Bonzini, Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé
Cc: qemu-devel, qemu-rust, Zhao Liu
HPET device needs to access and update hpet_cfg variable, but now it is
defined in hw/i386/fw_cfg.c and Rust code can't access it.
Move hpet_cfg definition to hpet.c (and rename it to hpet_fw_cfg). This
allows Rust HPET device implements its own global hpet_fw_cfg variable,
and will further reduce the use of unsafe C code access and calls in the
Rust HPET implementation.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
hw/i386/fw_cfg.c | 4 +---
hw/timer/hpet.c | 14 ++++++++------
include/hw/timer/hpet.h | 2 +-
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
index d2cb08715a21..546de63123e6 100644
--- a/hw/i386/fw_cfg.c
+++ b/hw/i386/fw_cfg.c
@@ -26,8 +26,6 @@
#include CONFIG_DEVICES
#include "target/i386/cpu.h"
-struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
-
const char *fw_cfg_arch_key_name(uint16_t key)
{
static const struct {
@@ -153,7 +151,7 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
PCMachineState *pcms =
(PCMachineState *)object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE);
if (pcms && pcms->hpet_enabled) {
- fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg));
+ fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_fw_cfg, sizeof(hpet_fw_cfg));
}
#endif
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 92b6d509edda..f2e2d83a3f67 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -40,6 +40,8 @@
#include "qom/object.h"
#include "trace.h"
+struct hpet_fw_config hpet_fw_cfg = {.count = UINT8_MAX};
+
#define HPET_MSI_SUPPORT 0
OBJECT_DECLARE_SIMPLE_TYPE(HPETState, HPET)
@@ -655,8 +657,8 @@ static void hpet_reset(DeviceState *d)
s->hpet_counter = 0ULL;
s->hpet_offset = 0ULL;
s->config = 0ULL;
- hpet_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
- hpet_cfg.hpet[s->hpet_id].address = sbd->mmio[0].addr;
+ hpet_fw_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
+ hpet_fw_cfg.hpet[s->hpet_id].address = sbd->mmio[0].addr;
/* to document that the RTC lowers its output on reset as well */
s->rtc_irq_level = 0;
@@ -698,17 +700,17 @@ static void hpet_realize(DeviceState *dev, Error **errp)
if (!s->intcap) {
warn_report("Hpet's intcap not initialized");
}
- if (hpet_cfg.count == UINT8_MAX) {
+ if (hpet_fw_cfg.count == UINT8_MAX) {
/* first instance */
- hpet_cfg.count = 0;
+ hpet_fw_cfg.count = 0;
}
- if (hpet_cfg.count == 8) {
+ if (hpet_fw_cfg.count == 8) {
error_setg(errp, "Only 8 instances of HPET is allowed");
return;
}
- s->hpet_id = hpet_cfg.count++;
+ s->hpet_id = hpet_fw_cfg.count++;
for (i = 0; i < HPET_NUM_IRQ_ROUTES; i++) {
sysbus_init_irq(sbd, &s->irqs[i]);
diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h
index 71e8c62453d1..c2656f7f0bef 100644
--- a/include/hw/timer/hpet.h
+++ b/include/hw/timer/hpet.h
@@ -73,7 +73,7 @@ struct hpet_fw_config
struct hpet_fw_entry hpet[8];
} QEMU_PACKED;
-extern struct hpet_fw_config hpet_cfg;
+extern struct hpet_fw_config hpet_fw_cfg;
#define TYPE_HPET "hpet"
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH 02/10] rust/qdev: add the macro to define bit property
2025-01-25 12:51 [PATCH 00/10] rust: Add HPET timer device Zhao Liu
2025-01-25 12:51 ` [PATCH 01/10] i386/fw_cfg: move hpet_cfg definition to hpet.c Zhao Liu
@ 2025-01-25 12:51 ` Zhao Liu
2025-01-25 12:51 ` [PATCH 03/10] rust/irq: Add a helper to convert [InterruptSource] to [*mut IRQState] Zhao Liu
` (7 subsequent siblings)
9 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-01-25 12:51 UTC (permalink / raw)
To: Paolo Bonzini, Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé
Cc: qemu-devel, qemu-rust, Zhao Liu
HPET device (Rust device) needs to define the bit type property.
Add a variant of define_property macro to define bit type property.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
rust/qemu-api/src/qdev.rs | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index b348f2f4ce71..32740c873604 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -166,6 +166,18 @@ fn class_init(dc: &mut DeviceClass) {
#[macro_export]
macro_rules! define_property {
+ ($name:expr, $state:ty, $field:ident, $prop:expr, $type:ty, bit = $bitnr:expr, default = $defval:expr$(,)*) => {
+ $crate::bindings::Property {
+ // use associated function syntax for type checking
+ name: ::std::ffi::CStr::as_ptr($name),
+ info: $prop,
+ offset: $crate::offset_of!($state, $field) as isize,
+ bitnr: $bitnr,
+ set_default: true,
+ defval: $crate::bindings::Property__bindgen_ty_1 { u: $defval as u64 },
+ ..$crate::zeroable::Zeroable::ZERO
+ }
+ };
($name:expr, $state:ty, $field:ident, $prop:expr, $type:ty, default = $defval:expr$(,)*) => {
$crate::bindings::Property {
// use associated function syntax for type checking
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH 03/10] rust/irq: Add a helper to convert [InterruptSource] to [*mut IRQState]
2025-01-25 12:51 [PATCH 00/10] rust: Add HPET timer device Zhao Liu
2025-01-25 12:51 ` [PATCH 01/10] i386/fw_cfg: move hpet_cfg definition to hpet.c Zhao Liu
2025-01-25 12:51 ` [PATCH 02/10] rust/qdev: add the macro to define bit property Zhao Liu
@ 2025-01-25 12:51 ` Zhao Liu
2025-01-29 10:51 ` Paolo Bonzini
2025-01-25 12:51 ` [PATCH 04/10] rust: add bindings for gpio_{in|out} initialization Zhao Liu
` (6 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Zhao Liu @ 2025-01-25 12:51 UTC (permalink / raw)
To: Paolo Bonzini, Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé
Cc: qemu-devel, qemu-rust, Zhao Liu
This is useful to hanlde InterruptSource slice and pass it to C
bindings.
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC:
* New commit.
---
rust/qemu-api/src/irq.rs | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/rust/qemu-api/src/irq.rs b/rust/qemu-api/src/irq.rs
index 638545c3a649..7dca3b9ee5a8 100644
--- a/rust/qemu-api/src/irq.rs
+++ b/rust/qemu-api/src/irq.rs
@@ -5,12 +5,10 @@
//! Bindings for interrupt sources
use core::ptr;
-use std::{marker::PhantomData, os::raw::c_int};
+use std::{marker::PhantomData, os::raw::c_int, slice};
-use crate::{
- bindings::{qemu_set_irq, IRQState},
- prelude::*,
-};
+pub(crate) use crate::bindings::IRQState;
+use crate::{bindings::qemu_set_irq, prelude::*};
/// Interrupt sources are used by devices to pass changes to a value (typically
/// a boolean). The interrupt sink is usually an interrupt controller or
@@ -81,6 +79,16 @@ pub fn set(&self, level: T) {
pub(crate) const fn as_ptr(&self) -> *mut *mut IRQState {
self.cell.as_ptr()
}
+
+ #[allow(dead_code)]
+ pub(crate) fn as_slice_of_qemu_irq(slice: &[Self]) -> &[*mut IRQState] {
+ unsafe {
+ // SAFETY: InterruptSource has the same memory layout as *mut IRQState.
+ // Additionally, the slice parameter itself, as a reference to a slice type,
+ // can be converted into a valid pointer and has a valid length.
+ slice::from_raw_parts(slice.as_ptr().cast::<*mut IRQState>(), slice.len())
+ }
+ }
}
impl Default for InterruptSource {
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 03/10] rust/irq: Add a helper to convert [InterruptSource] to [*mut IRQState]
2025-01-25 12:51 ` [PATCH 03/10] rust/irq: Add a helper to convert [InterruptSource] to [*mut IRQState] Zhao Liu
@ 2025-01-29 10:51 ` Paolo Bonzini
2025-02-07 7:10 ` Zhao Liu
0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-01-29 10:51 UTC (permalink / raw)
To: Zhao Liu
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé, qemu-devel, qemu-rust, Zhao Liu
On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
>
> This is useful to hanlde InterruptSource slice and pass it to C
> bindings.
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
This may be a bad suggestion, after all. clippy complains that you're
casting &[*mut IRQState] to mutable (https://rust-lang.github.io/rust-
clippy/master/#as_ptr_cast_mut).
I can think of two solutions:
1) add #[allow(clippy::as_ptr_cast_mut)] and explain with a comment
// Casting to *mut *mut IRQState is valid, because
// the source slice `pins` uses interior mutability.
2) drop as_slice_of_qemu_irq() and replace it with something like
pub(crate) const fn slice_as_ptr(slice: &[Self]) -> *mut *mut IRQState {
slice[0].as_ptr()
}
You choose.
Paolo
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 03/10] rust/irq: Add a helper to convert [InterruptSource] to [*mut IRQState]
2025-01-29 10:51 ` Paolo Bonzini
@ 2025-02-07 7:10 ` Zhao Liu
2025-02-07 7:44 ` Zhao Liu
0 siblings, 1 reply; 34+ messages in thread
From: Zhao Liu @ 2025-02-07 7:10 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé, qemu-devel, qemu-rust
On Wed, Jan 29, 2025 at 11:51:02AM +0100, Paolo Bonzini wrote:
> Date: Wed, 29 Jan 2025 11:51:02 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 03/10] rust/irq: Add a helper to convert
> [InterruptSource] to [*mut IRQState]
>
> On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> >
> > This is useful to hanlde InterruptSource slice and pass it to C
> > bindings.
> >
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>
> This may be a bad suggestion, after all. clippy complains that you're
> casting &[*mut IRQState] to mutable (https://rust-lang.github.io/rust-
> clippy/master/#as_ptr_cast_mut).
>
> I can think of two solutions:
>
> 1) add #[allow(clippy::as_ptr_cast_mut)] and explain with a comment
>
> // Casting to *mut *mut IRQState is valid, because
> // the source slice `pins` uses interior mutability.
>
> 2) drop as_slice_of_qemu_irq() and replace it with something like
>
> pub(crate) const fn slice_as_ptr(slice: &[Self]) -> *mut *mut IRQState {
> slice[0].as_ptr()
> }
>
> You choose.
>
:) I choose the second way, which goes back to the initial codes.
Though an empty slice w/o any check would cause a runtime error like:
"thread '<unnamed>' panicked at 'index out of bounds: the len is 0 but the index is 0'"
Adding an assert to ensure a non-empty slice is better.
Thanks,
Zhao
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 03/10] rust/irq: Add a helper to convert [InterruptSource] to [*mut IRQState]
2025-02-07 7:10 ` Zhao Liu
@ 2025-02-07 7:44 ` Zhao Liu
2025-02-07 9:57 ` Paolo Bonzini
0 siblings, 1 reply; 34+ messages in thread
From: Zhao Liu @ 2025-02-07 7:44 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Manos Pitsidianakis, Junjie Mao, Alex Benn�e,
Philippe Mathieu-Daud�, Richard Henderson, Peter Maydell,
Daniel P . Berrang�, qemu-devel, qemu-rust
On Fri, Feb 07, 2025 at 03:10:53PM +0800, Zhao Liu wrote:
> Date: Fri, 7 Feb 2025 15:10:53 +0800
> From: Zhao Liu <zhao1.liu@intel.com>
> Subject: Re: [PATCH 03/10] rust/irq: Add a helper to convert
> [InterruptSource] to [*mut IRQState]
>
> On Wed, Jan 29, 2025 at 11:51:02AM +0100, Paolo Bonzini wrote:
> > Date: Wed, 29 Jan 2025 11:51:02 +0100
> > From: Paolo Bonzini <pbonzini@redhat.com>
> > Subject: Re: [PATCH 03/10] rust/irq: Add a helper to convert
> > [InterruptSource] to [*mut IRQState]
> >
> > On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> > >
> > > This is useful to hanlde InterruptSource slice and pass it to C
> > > bindings.
> > >
> > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> >
> > This may be a bad suggestion, after all. clippy complains that you're
> > casting &[*mut IRQState] to mutable (https://rust-lang.github.io/rust-
> > clippy/master/#as_ptr_cast_mut).
> >
> > I can think of two solutions:
> >
> > 1) add #[allow(clippy::as_ptr_cast_mut)] and explain with a comment
> >
> > // Casting to *mut *mut IRQState is valid, because
> > // the source slice `pins` uses interior mutability.
> >
I agree it's the best to use interior mutability.
Just to confirm, I check with `cargo +nightly clippy` but it doesn't
complain about this case. Should I switch to another version of clippy
when I do such check? (currently I'm using v0.1.63 clippy as well, to
match rustc.)
Thanks,
Zhao
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 03/10] rust/irq: Add a helper to convert [InterruptSource] to [*mut IRQState]
2025-02-07 7:44 ` Zhao Liu
@ 2025-02-07 9:57 ` Paolo Bonzini
2025-02-08 11:14 ` Zhao Liu
0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-07 9:57 UTC (permalink / raw)
To: Zhao Liu
Cc: Manos Pitsidianakis, Junjie Mao, Alex Benn�e,
Philippe Mathieu-Daud�, Richard Henderson, Peter Maydell,
Daniel P . Berrang�, qemu-devel, qemu-rust
[-- Attachment #1: Type: text/plain, Size: 707 bytes --]
Il ven 7 feb 2025, 08:25 Zhao Liu <zhao1.liu@intel.com> ha scritto:
> Just to confirm, I check with `cargo +nightly clippy` but it doesn't
> complain about this case. Should I switch to another version of clippy
> when I do such check? (currently I'm using v0.1.63 clippy as well, to
> match rustc.)
>
I don't remember exactly how I noticed it—I am pretty sure it broke in CI
though. Maybe the change to add rust_version hid it.
To answer your question, generally the idea is that we use the latest
version of the developer tools (cargo, rustfmt, clippy). In particular old
versions of cargo don't support retrieving clippy settings from Cargo.toml.
Paolo
> Thanks,
> Zhao
>
>
[-- Attachment #2: Type: text/html, Size: 1417 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 03/10] rust/irq: Add a helper to convert [InterruptSource] to [*mut IRQState]
2025-02-07 9:57 ` Paolo Bonzini
@ 2025-02-08 11:14 ` Zhao Liu
2025-02-08 11:39 ` Paolo Bonzini
0 siblings, 1 reply; 34+ messages in thread
From: Zhao Liu @ 2025-02-08 11:14 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Manos Pitsidianakis, Junjie Mao, Alex Benn�e,
Philippe Mathieu-Daud�, Richard Henderson, Peter Maydell,
Daniel P . Berrang�, qemu-devel, qemu-rust
On Fri, Feb 07, 2025 at 10:57:11AM +0100, Paolo Bonzini wrote:
> Date: Fri, 7 Feb 2025 10:57:11 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 03/10] rust/irq: Add a helper to convert
> [InterruptSource] to [*mut IRQState]
>
> Il ven 7 feb 2025, 08:25 Zhao Liu <zhao1.liu@intel.com> ha scritto:
>
> > Just to confirm, I check with `cargo +nightly clippy` but it doesn't
> > complain about this case. Should I switch to another version of clippy
> > when I do such check? (currently I'm using v0.1.63 clippy as well, to
> > match rustc.)
> >
>
> I don't remember exactly how I noticed it—I am pretty sure it broke in CI
> though. Maybe the change to add rust_version hid it.
>
> To answer your question, generally the idea is that we use the latest
> version of the developer tools (cargo, rustfmt, clippy). In particular old
> versions of cargo don't support retrieving clippy settings from Cargo.toml.
This one inspired me. I'm thinking that even though we deleted the
README of pl011, a gneral guide or doc section (at somewhere, e.g.,
docs/devel/style.rst or docs/devel/submitting-a-patch.rst) would be
helpful.
At least, that could remind contributor to check patches with latest
toolchain instead of letting the maintainer's CI smoking fail.
Thanks,
Zhao
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 03/10] rust/irq: Add a helper to convert [InterruptSource] to [*mut IRQState]
2025-02-08 11:14 ` Zhao Liu
@ 2025-02-08 11:39 ` Paolo Bonzini
2025-02-08 18:10 ` Zhao Liu
0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-08 11:39 UTC (permalink / raw)
To: Zhao Liu
Cc: Manos Pitsidianakis, Junjie Mao, Alex Benn�e,
Philippe Mathieu-Daud�, Richard Henderson, Peter Maydell,
Daniel P . Berrang�, qemu-devel, qemu-rust
[-- Attachment #1: Type: text/plain, Size: 1605 bytes --]
Il sab 8 feb 2025, 11:55 Zhao Liu <zhao1.liu@intel.com> ha scritto:
> On Fri, Feb 07, 2025 at 10:57:11AM +0100, Paolo Bonzini wrote:
> > Date: Fri, 7 Feb 2025 10:57:11 +0100
> > From: Paolo Bonzini <pbonzini@redhat.com>
> > Subject: Re: [PATCH 03/10] rust/irq: Add a helper to convert
> > [InterruptSource] to [*mut IRQState]
> >
> > Il ven 7 feb 2025, 08:25 Zhao Liu <zhao1.liu@intel.com> ha scritto:
> >
> > > Just to confirm, I check with `cargo +nightly clippy` but it doesn't
> > > complain about this case. Should I switch to another version of clippy
> > > when I do such check? (currently I'm using v0.1.63 clippy as well, to
> > > match rustc.)
> > >
> >
> > I don't remember exactly how I noticed it—I am pretty sure it broke in CI
> > though. Maybe the change to add rust_version hid it.
> >
> > To answer your question, generally the idea is that we use the latest
> > version of the developer tools (cargo, rustfmt, clippy). In particular
> old
> > versions of cargo don't support retrieving clippy settings from
> Cargo.toml.
>
> This one inspired me. I'm thinking that even though we deleted the
> README of pl011, a gneral guide or doc section (at somewhere, e.g.,
> docs/devel/style.rst or docs/devel/submitting-a-patch.rst) would be
> helpful.
>
> At least, that could remind contributor to check patches with latest
> toolchain instead of letting the maintainer's CI smoking fail.
>
I have sent a docs/devel/rust.rst sometime last week, it will be in the
next pull request and then you can send a patch on top.
Paolo
Thanks,
> Zhao
>
>
[-- Attachment #2: Type: text/html, Size: 2558 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 03/10] rust/irq: Add a helper to convert [InterruptSource] to [*mut IRQState]
2025-02-08 11:39 ` Paolo Bonzini
@ 2025-02-08 18:10 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-08 18:10 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Manos Pitsidianakis, Junjie Mao, Alex Benn�e,
Philippe Mathieu-Daud�, Richard Henderson, Peter Maydell,
Daniel P . Berrang�, qemu-devel, qemu-rust
> I have sent a docs/devel/rust.rst sometime last week, it will be in the
> next pull request and then you can send a patch on top.
>
Nice, I'll also have a look at that patch
And I plan to post v2 tomorrow... as I'll leave at next week from Tuesday.
Try to catch your pull request train if possible. :-)
Thanks,
Zhao
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 04/10] rust: add bindings for gpio_{in|out} initialization
2025-01-25 12:51 [PATCH 00/10] rust: Add HPET timer device Zhao Liu
` (2 preceding siblings ...)
2025-01-25 12:51 ` [PATCH 03/10] rust/irq: Add a helper to convert [InterruptSource] to [*mut IRQState] Zhao Liu
@ 2025-01-25 12:51 ` Zhao Liu
2025-01-29 10:59 ` Paolo Bonzini
2025-01-25 12:51 ` [PATCH 05/10] rust: add bindings for memattrs Zhao Liu
` (5 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Zhao Liu @ 2025-01-25 12:51 UTC (permalink / raw)
To: Paolo Bonzini, Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé
Cc: qemu-devel, qemu-rust, Zhao Liu
Wrap qdev_init_gpio_{in|out} as methods in DeviceMethods. And for
qdev_init_gpio_in, based on FnCall, it can support idiomatic Rust
callback without the need for C style wrapper.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC:
* Use FnCall to support gpio in callback.
* Place gpio_{in|out} in DeviceMethods.
* Accept &[InterruptSource] as the parameter of gpio_out.
---
rust/qemu-api/src/qdev.rs | 37 +++++++++++++++++++++++++++++++++++--
1 file changed, 35 insertions(+), 2 deletions(-)
diff --git a/rust/qemu-api/src/qdev.rs b/rust/qemu-api/src/qdev.rs
index 32740c873604..96ca8b8aa9ad 100644
--- a/rust/qemu-api/src/qdev.rs
+++ b/rust/qemu-api/src/qdev.rs
@@ -6,16 +6,17 @@
use std::{
ffi::{CStr, CString},
- os::raw::c_void,
+ os::raw::{c_int, c_void},
ptr::NonNull,
};
pub use bindings::{Clock, ClockEvent, DeviceClass, DeviceState, Property, ResetType};
use crate::{
- bindings::{self, Error, ResettableClass},
+ bindings::{self, qdev_init_gpio_in, qdev_init_gpio_out, Error, ResettableClass},
callbacks::FnCall,
cell::bql_locked,
+ irq::{IRQState, InterruptSource},
prelude::*,
qom::{ClassInitImpl, ObjectClass, ObjectImpl, Owned},
vmstate::VMStateDescription,
@@ -278,6 +279,38 @@ fn do_init_clock_in(
// IsA<DeviceState> bound.
do_init_clock_in(unsafe { self.as_mut_ptr() }, name, cb, events)
}
+
+ fn init_gpio_in<F: for<'a> FnCall<(&'a Self::Target, u32, u32)>>(&self, num_lines: u32, _f: F) {
+ unsafe extern "C" fn rust_irq_handler<T, F: for<'a> FnCall<(&'a T, u32, u32)>>(
+ opaque: *mut c_void,
+ line: c_int,
+ level: c_int,
+ ) {
+ // SAFETY: the opaque was passed as a reference to `T`
+ F::call((unsafe { &*(opaque.cast::<T>()) }, line as u32, level as u32))
+ }
+
+ let gpio_in_cb: unsafe extern "C" fn(*mut c_void, c_int, c_int) =
+ rust_irq_handler::<Self::Target, F>;
+
+ unsafe {
+ qdev_init_gpio_in(
+ self.as_mut_ptr::<DeviceState>(),
+ Some(gpio_in_cb),
+ num_lines as c_int,
+ );
+ }
+ }
+
+ fn init_gpio_out(&self, pins: &[InterruptSource]) {
+ unsafe {
+ qdev_init_gpio_out(
+ self.as_mut_ptr::<DeviceState>(),
+ InterruptSource::as_slice_of_qemu_irq(pins).as_ptr() as *mut *mut IRQState,
+ pins.len() as c_int,
+ );
+ }
+ }
}
impl<R: ObjectDeref> DeviceMethods for R where R::Target: IsA<DeviceState> {}
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 04/10] rust: add bindings for gpio_{in|out} initialization
2025-01-25 12:51 ` [PATCH 04/10] rust: add bindings for gpio_{in|out} initialization Zhao Liu
@ 2025-01-29 10:59 ` Paolo Bonzini
2025-02-07 8:43 ` Zhao Liu
0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-01-29 10:59 UTC (permalink / raw)
To: Zhao Liu
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé, qemu-devel, qemu-rust, Zhao Liu
On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> + fn init_gpio_in<F: for<'a> FnCall<(&'a Self::Target, u32, u32)>>(&self, num_lines: u32, _f: F) {
> + unsafe extern "C" fn rust_irq_handler<T, F: for<'a> FnCall<(&'a T, u32, u32)>>(
> + opaque: *mut c_void,
> + line: c_int,
> + level: c_int,
> + ) {
> + // SAFETY: the opaque was passed as a reference to `T`
> + F::call((unsafe { &*(opaque.cast::<T>()) }, line as u32, level as u32))
> + }
> +
> + let gpio_in_cb: unsafe extern "C" fn(*mut c_void, c_int, c_int) =
> + rust_irq_handler::<Self::Target, F>;
Please add "let _: () = F::ASSERT_IS_SOME;", which is added by the
qdev_init_clock_in() patch.
Paolo
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 04/10] rust: add bindings for gpio_{in|out} initialization
2025-01-29 10:59 ` Paolo Bonzini
@ 2025-02-07 8:43 ` Zhao Liu
2025-02-07 9:54 ` Paolo Bonzini
0 siblings, 1 reply; 34+ messages in thread
From: Zhao Liu @ 2025-02-07 8:43 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé, qemu-devel, qemu-rust
On Wed, Jan 29, 2025 at 11:59:04AM +0100, Paolo Bonzini wrote:
> Date: Wed, 29 Jan 2025 11:59:04 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 04/10] rust: add bindings for gpio_{in|out}
> initialization
>
>
>
> On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> > + fn init_gpio_in<F: for<'a> FnCall<(&'a Self::Target, u32, u32)>>(&self, num_lines: u32, _f: F) {
> > + unsafe extern "C" fn rust_irq_handler<T, F: for<'a> FnCall<(&'a T, u32, u32)>>(
> > + opaque: *mut c_void,
> > + line: c_int,
> > + level: c_int,
> > + ) {
> > + // SAFETY: the opaque was passed as a reference to `T`
> > + F::call((unsafe { &*(opaque.cast::<T>()) }, line as u32, level as u32))
> > + }
> > +
> > + let gpio_in_cb: unsafe extern "C" fn(*mut c_void, c_int, c_int) =
> > + rust_irq_handler::<Self::Target, F>;
>
> Please add "let _: () = F::ASSERT_IS_SOME;", which is added by the
> qdev_init_clock_in() patch.
>
Okay.
I would add `assert!(F::is_some());` at the beginning of init_gpio_in().
There's a difference with origianl C version:
In C side, qdev_get_gpio_in() family could accept a NULL handler, but
there's no such case in current QEMU:
* qdev_get_gpio_in
* qdev_init_gpio_in_named
* qdev_init_gpio_in_named_with_opaque
And from code logic view, creating an input GPIO line but doing nothing
on input, sounds also unusual.
So, for simplicity, in the Rust version I make the handler non-optional.
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 04/10] rust: add bindings for gpio_{in|out} initialization
2025-02-07 8:43 ` Zhao Liu
@ 2025-02-07 9:54 ` Paolo Bonzini
2025-02-08 11:16 ` Zhao Liu
0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-07 9:54 UTC (permalink / raw)
To: Zhao Liu
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé, qemu-devel, qemu-rust
[-- Attachment #1: Type: text/plain, Size: 808 bytes --]
Il ven 7 feb 2025, 09:24 Zhao Liu <zhao1.liu@intel.com> ha scritto:
> > Please add "let _: () = F::ASSERT_IS_SOME;", which is added by the
> > qdev_init_clock_in() patch.
> >
>
> Okay.
>
> I would add `assert!(F::is_some());` at the beginning of init_gpio_in().
>
Use the "let" so that it's caught at compile time.
There's a difference with origianl C version:
>
> In C side, qdev_get_gpio_in() family could accept a NULL handler, but
> there's no such case in current QEMU:
>
> * qdev_get_gpio_in
> * qdev_init_gpio_in_named
> * qdev_init_gpio_in_named_with_opaque
>
> And from code logic view, creating an input GPIO line but doing nothing
> on input, sounds also unusual.
>
Wouldn't it then crash in qemu_set_irq?
Paolo
So, for simplicity, in the Rust version I make the handler non-optional.
>
>
>
[-- Attachment #2: Type: text/html, Size: 1845 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 04/10] rust: add bindings for gpio_{in|out} initialization
2025-02-07 9:54 ` Paolo Bonzini
@ 2025-02-08 11:16 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-08 11:16 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé, qemu-devel, qemu-rust
> Use the "let" so that it's caught at compile time.
Thanks! Fixed.
> There's a difference with origianl C version:
> >
> > In C side, qdev_get_gpio_in() family could accept a NULL handler, but
> > there's no such case in current QEMU:
> >
> > * qdev_get_gpio_in
> > * qdev_init_gpio_in_named
> > * qdev_init_gpio_in_named_with_opaque
> >
> > And from code logic view, creating an input GPIO line but doing nothing
> > on input, sounds also unusual.
> >
>
> Wouldn't it then crash in qemu_set_irq?
>
Yes! Thank you for the education.
Regards,
Zhao
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 05/10] rust: add bindings for memattrs
2025-01-25 12:51 [PATCH 00/10] rust: Add HPET timer device Zhao Liu
` (3 preceding siblings ...)
2025-01-25 12:51 ` [PATCH 04/10] rust: add bindings for gpio_{in|out} initialization Zhao Liu
@ 2025-01-25 12:51 ` Zhao Liu
2025-01-25 12:51 ` [PATCH 06/10] rust: add bindings for timer Zhao Liu
` (4 subsequent siblings)
9 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-01-25 12:51 UTC (permalink / raw)
To: Paolo Bonzini, Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé
Cc: qemu-devel, qemu-rust, Zhao Liu
The MemTxAttrs structure contains bitfield members, and bindgen is
unable to generate an equivalent macro definition for
MEMTXATTRS_UNSPECIFIED.
Therefore, manually define a global constant variable
MEMTXATTRS_UNSPECIFIED to support calls from Rust code.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC:
* With a boolean type unspecified field, no need to add once_cell.
* Merge memattrs binding to memory.rs.
---
rust/qemu-api/src/memory.rs | 16 ++++++++++++++--
rust/qemu-api/src/zeroable.rs | 1 +
rust/wrapper.h | 1 +
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/rust/qemu-api/src/memory.rs b/rust/qemu-api/src/memory.rs
index 963d689c27d4..fff92508c68f 100644
--- a/rust/qemu-api/src/memory.rs
+++ b/rust/qemu-api/src/memory.rs
@@ -2,7 +2,7 @@
// Author(s): Paolo Bonzini <pbonzini@redhat.com>
// SPDX-License-Identifier: GPL-2.0-or-later
-//! Bindings for `MemoryRegion` and `MemoryRegionOps`
+//! Bindings for `MemoryRegion`, `MemoryRegionOps` and `MemTxAttrs`
use std::{
ffi::{CStr, CString},
@@ -11,7 +11,7 @@
ptr::addr_of,
};
-pub use bindings::hwaddr;
+pub use bindings::{hwaddr, MemTxAttrs};
use crate::{
bindings::{self, device_endian, memory_region_init_io},
@@ -189,3 +189,15 @@ unsafe impl ObjectType for MemoryRegion {
unsafe { CStr::from_bytes_with_nul_unchecked(bindings::TYPE_MEMORY_REGION) };
}
qom_isa!(MemoryRegion: Object);
+
+/// A special `MemTxAttrs` constant, used to indicate that no memary
+/// attributes are specified.
+///
+/// Bus masters which don't specify any attributes will get this,
+/// which has all attribute bits clear except the topmost one
+/// (so that we can distinguish "all attributes deliberately clear"
+/// from "didn't specify" if necessary).
+pub const MEMTXATTRS_UNSPECIFIED: MemTxAttrs = MemTxAttrs {
+ unspecified: true,
+ ..Zeroable::ZERO
+};
diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs
index 75742b50d4e3..9f009606b1ab 100644
--- a/rust/qemu-api/src/zeroable.rs
+++ b/rust/qemu-api/src/zeroable.rs
@@ -101,3 +101,4 @@ fn default() -> Self {
impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_1);
impl_zeroable!(crate::bindings::MemoryRegionOps__bindgen_ty_2);
impl_zeroable!(crate::bindings::MemoryRegionOps);
+impl_zeroable!(crate::bindings::MemTxAttrs);
diff --git a/rust/wrapper.h b/rust/wrapper.h
index a9bc67af0d5f..54839ce0f510 100644
--- a/rust/wrapper.h
+++ b/rust/wrapper.h
@@ -62,3 +62,4 @@ typedef enum memory_order {
#include "qapi/error.h"
#include "migration/vmstate.h"
#include "chardev/char-serial.h"
+#include "exec/memattrs.h"
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH 06/10] rust: add bindings for timer
2025-01-25 12:51 [PATCH 00/10] rust: Add HPET timer device Zhao Liu
` (4 preceding siblings ...)
2025-01-25 12:51 ` [PATCH 05/10] rust: add bindings for memattrs Zhao Liu
@ 2025-01-25 12:51 ` Zhao Liu
2025-01-29 10:58 ` Paolo Bonzini
2025-01-25 12:51 ` [PATCH 07/10] rust/timer/hpet: define hpet_cfg Zhao Liu
` (3 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Zhao Liu @ 2025-01-25 12:51 UTC (permalink / raw)
To: Paolo Bonzini, Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé
Cc: qemu-devel, qemu-rust, Zhao Liu
Add timer bindings to help handle idiomatic Rust callbacks.
Additionally, wrap QEMUClockType in ClockType binding to avoid unsafe
calls in device code.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC:
* Use FnCall to support timer callback.
* Only add timer_init_full() binding. timer_new() is unnecessary since
device (HPET) could create and allocate QEMUTimer.
* Implement Drop for QEMUTimer.
* Add ClockType binding.
---
meson.build | 7 +++
rust/qemu-api/meson.build | 1 +
rust/qemu-api/src/lib.rs | 1 +
rust/qemu-api/src/timer.rs | 92 ++++++++++++++++++++++++++++++++++++++
rust/wrapper.h | 1 +
5 files changed, 102 insertions(+)
create mode 100644 rust/qemu-api/src/timer.rs
diff --git a/meson.build b/meson.build
index da91b47be48b..1ec88770a60a 100644
--- a/meson.build
+++ b/meson.build
@@ -4079,6 +4079,13 @@ if have_rust
foreach enum : c_bitfields
bindgen_args += ['--bitfield-enum', enum]
endforeach
+ c_nocopy = [
+ 'QEMUTimer',
+ ]
+ # Used to customize Drop trait
+ foreach struct : c_nocopy
+ bindgen_args += ['--no-copy', struct]
+ endforeach
# TODO: Remove this comment when the clang/libclang mismatch issue is solved.
#
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 80eafc7f6bd8..caed2b233991 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -29,6 +29,7 @@ _qemu_api_rs = static_library(
'src/qdev.rs',
'src/qom.rs',
'src/sysbus.rs',
+ 'src/timer.rs',
'src/vmstate.rs',
'src/zeroable.rs',
],
diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
index 8cc095b13f6f..88825b69cff8 100644
--- a/rust/qemu-api/src/lib.rs
+++ b/rust/qemu-api/src/lib.rs
@@ -24,6 +24,7 @@
pub mod qdev;
pub mod qom;
pub mod sysbus;
+pub mod timer;
pub mod vmstate;
pub mod zeroable;
diff --git a/rust/qemu-api/src/timer.rs b/rust/qemu-api/src/timer.rs
new file mode 100644
index 000000000000..a1c7c7b4d9e6
--- /dev/null
+++ b/rust/qemu-api/src/timer.rs
@@ -0,0 +1,92 @@
+// Copyright (C) 2024 Intel Corporation.
+// Author(s): Zhao Liu <zhai1.liu@intel.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+use std::os::raw::{c_int, c_void};
+
+pub use bindings::QEMUTimer;
+
+use crate::{
+ bindings::{
+ self, qemu_clock_get_ns, timer_del, timer_init_full, timer_mod, QEMUClockType,
+ QEMUTimerListGroup,
+ },
+ callbacks::FnCall,
+};
+
+impl QEMUTimer {
+ pub fn new() -> Self {
+ Default::default()
+ }
+
+ pub fn timer_init_full<'timer, 'opaque: 'timer, T, F>(
+ &'timer mut self,
+ timer_list_group: Option<&QEMUTimerListGroup>,
+ clk_type: QEMUClockType,
+ scale: u32,
+ attributes: u32,
+ _f: F,
+ opaque: &'opaque T,
+ ) where
+ F: for<'a> FnCall<(&'a T,)>,
+ {
+ /// timer expiration callback
+ unsafe extern "C" fn rust_timer_handler<T, F: for<'a> FnCall<(&'a T,)>>(
+ opaque: *mut c_void,
+ ) {
+ // SAFETY: the opaque was passed as a reference to `T`.
+ F::call((unsafe { &*(opaque.cast::<T>()) },))
+ }
+
+ let timer_cb: unsafe extern "C" fn(*mut c_void) = rust_timer_handler::<T, F>;
+
+ // SAFETY: the opaque outlives the timer
+ unsafe {
+ timer_init_full(
+ self,
+ if let Some(g) = timer_list_group {
+ g as *const QEMUTimerListGroup as *mut QEMUTimerListGroup
+ } else {
+ ::core::ptr::null_mut()
+ },
+ clk_type,
+ scale as c_int,
+ attributes as c_int,
+ Some(timer_cb),
+ (opaque as *const T).cast::<c_void>() as *mut c_void,
+ )
+ }
+ }
+
+ pub fn timer_mod(&mut self, expire_time: u64) {
+ unsafe { timer_mod(self as *mut QEMUTimer, expire_time as i64) }
+ }
+}
+
+impl Drop for QEMUTimer {
+ fn drop(&mut self) {
+ unsafe { timer_del(self as *mut QEMUTimer) }
+ }
+}
+
+pub fn qemu_clock_get_virtual_ns() -> u64 {
+ // SAFETY:
+ // Valid parameter.
+ (unsafe { qemu_clock_get_ns(QEMUClockType::QEMU_CLOCK_VIRTUAL) }) as u64
+}
+
+pub struct ClockType {
+ pub id: QEMUClockType,
+}
+
+impl ClockType {
+ pub fn get_ns(&self) -> u64 {
+ // SAFETY: cannot be created outside this module, therefore id
+ // is valid
+ (unsafe { qemu_clock_get_ns(self.id) }) as u64
+ }
+}
+
+pub const CLOCK_VIRTUAL: ClockType = ClockType {
+ id: QEMUClockType::QEMU_CLOCK_VIRTUAL,
+};
diff --git a/rust/wrapper.h b/rust/wrapper.h
index 54839ce0f510..a35bfbd1760d 100644
--- a/rust/wrapper.h
+++ b/rust/wrapper.h
@@ -63,3 +63,4 @@ typedef enum memory_order {
#include "migration/vmstate.h"
#include "chardev/char-serial.h"
#include "exec/memattrs.h"
+#include "qemu/timer.h"
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 06/10] rust: add bindings for timer
2025-01-25 12:51 ` [PATCH 06/10] rust: add bindings for timer Zhao Liu
@ 2025-01-29 10:58 ` Paolo Bonzini
2025-02-07 13:33 ` Zhao Liu
0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-01-29 10:58 UTC (permalink / raw)
To: Zhao Liu
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé, qemu-devel, qemu-rust, Zhao Liu
On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> + c_nocopy = [
> + 'QEMUTimer',
> + ]
> + # Used to customize Drop trait
> + foreach struct : c_nocopy
> + bindgen_args += ['--no-copy', struct]
> + endforeach
Nice.
> +pub use bindings::QEMUTimer;
> +
> +use crate::{
> + bindings::{
> + self, qemu_clock_get_ns, timer_del, timer_init_full, timer_mod, QEMUClockType,
> + QEMUTimerListGroup,
> + },
> + callbacks::FnCall,
> +};
> +
> +impl QEMUTimer {
> + pub fn new() -> Self {
> + Default::default()
> + }
> +
> + pub fn timer_init_full<'timer, 'opaque: 'timer, T, F>(
General question - should the names:
- include the "timer" part, matching QEMU C code, or exclude it to avoid
repetition? I would say remove it, but I'm open to suggestions and other
opinions
- include the "QEMU" part? I'd say remove it, similar to ClockType below
that is:
-pub use bindings::QEMUTimer;
+pub use bindings::QEMUTimer as Timer;
+pub use bindings::QEMUTimerList as TimerList;
+pub use bindings::QEMUTimerListGroup as TimerListGroup;
> + &'timer mut self,
> + timer_list_group: Option<&QEMUTimerListGroup>,
> + clk_type: QEMUClockType,
Please take a ClockType instead.
> + scale: u32,
> + attributes: u32,
> + _f: F,
> + opaque: &'opaque T,
> + ) where
> + F: for<'a> FnCall<(&'a T,)>,
> + {
> + /// timer expiration callback
> + unsafe extern "C" fn rust_timer_handler<T, F: for<'a> FnCall<(&'a T,)>>(
> + opaque: *mut c_void,
> + ) {
> + // SAFETY: the opaque was passed as a reference to `T`.
> + F::call((unsafe { &*(opaque.cast::<T>()) },))
> + }
Please add "let _: () = F::ASSERT_IS_SOME;", which is added by the
qdev_init_clock_in() patch.
> + let timer_cb: unsafe extern "C" fn(*mut c_void) = rust_timer_handler::<T, F>;
> +
> + // SAFETY: the opaque outlives the timer
> + unsafe {
> + timer_init_full(
> + self,
> + if let Some(g) = timer_list_group {
> + g as *const QEMUTimerListGroup as *mut QEMUTimerListGroup
> + } else {
> + ::core::ptr::null_mut()
> + },
> + clk_type,
> + scale as c_int,
> + attributes as c_int,
> + Some(timer_cb),
> + (opaque as *const T).cast::<c_void>() as *mut c_void,
> + )
> + }
> + }
> +
> + pub fn timer_mod(&mut self, expire_time: u64) {
> + unsafe { timer_mod(self as *mut QEMUTimer, expire_time as i64) }
> + }
This can take &self, because timers are thread-safe:
pub fn timer_mod(&self, expire_time: u64) {
unsafe { timer_mod(self.as_mut_ptr(), expire_time as i64) }
}
const fn as_mut_ptr(&self) -> *mut Self {
self as *const QEMUTimer as *mut _
}
> +}
> +
> +impl Drop for QEMUTimer {
> + fn drop(&mut self) {
> + unsafe { timer_del(self as *mut QEMUTimer) }
> + }
timer_del() can be useful even outside Drop, so
pub fn timer_del(&self) {
unsafe { timer_del(self.as_mut_ptr()) }
}
and just use self.timer_del() here.
> +}
> +
> +pub fn qemu_clock_get_virtual_ns() -> u64 {
> + // SAFETY:
> + // Valid parameter.
> + (unsafe { qemu_clock_get_ns(QEMUClockType::QEMU_CLOCK_VIRTUAL) }) as u64
> +}
Not needed.
> +pub struct ClockType {
> + pub id: QEMUClockType,
> +}
The field does not have to be "pub" (maybe "pub(self)", I'm not sure
offhand).
Paolo
> +impl ClockType {
> + pub fn get_ns(&self) -> u64 {
> + // SAFETY: cannot be created outside this module, therefore id
> + // is valid
> + (unsafe { qemu_clock_get_ns(self.id) }) as u64
> + }
> +}
> +
> +pub const CLOCK_VIRTUAL: ClockType = ClockType {
> + id: QEMUClockType::QEMU_CLOCK_VIRTUAL,
> +};
> diff --git a/rust/wrapper.h b/rust/wrapper.h
> index 54839ce0f510..a35bfbd1760d 100644
> --- a/rust/wrapper.h
> +++ b/rust/wrapper.h
> @@ -63,3 +63,4 @@ typedef enum memory_order {
> #include "migration/vmstate.h"
> #include "chardev/char-serial.h"
> #include "exec/memattrs.h"
> +#include "qemu/timer.h"
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 06/10] rust: add bindings for timer
2025-01-29 10:58 ` Paolo Bonzini
@ 2025-02-07 13:33 ` Zhao Liu
2025-02-07 14:55 ` Paolo Bonzini
0 siblings, 1 reply; 34+ messages in thread
From: Zhao Liu @ 2025-02-07 13:33 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé, qemu-devel, qemu-rust
> > +pub use bindings::QEMUTimer;
> > +
> > +use crate::{
> > + bindings::{
> > + self, qemu_clock_get_ns, timer_del, timer_init_full, timer_mod, QEMUClockType,
> > + QEMUTimerListGroup,
> > + },
> > + callbacks::FnCall,
> > +};
> > +
> > +impl QEMUTimer {
> > + pub fn new() -> Self {
> > + Default::default()
> > + }
> > +
> > + pub fn timer_init_full<'timer, 'opaque: 'timer, T, F>(
>
> General question - should the names:
>
> - include the "timer" part, matching QEMU C code, or exclude it to avoid
> repetition? I would say remove it,
I agree and I would name it "init()" instead of "init_full()".
> but I'm open to suggestions and other
> opinions
>
> - include the "QEMU" part? I'd say remove it, similar to ClockType below
> that is:
>
> -pub use bindings::QEMUTimer;
> +pub use bindings::QEMUTimer as Timer;
> +pub use bindings::QEMUTimerList as TimerList;
> +pub use bindings::QEMUTimerListGroup as TimerListGroup;
I notice you've picked another way for IRQState, so I could follow that
like:
pub type Timer = bindings::QEMUTimer;
This style make it easy to add doc (timer binding currently lacks
doc, but I will add it as much as possible).
Another option may be to wrap QEMUTimer as what MemoryRegionOps did, but
timer has only 1 callback so I think it's not necessary.
> > + &'timer mut self,
> > + timer_list_group: Option<&QEMUTimerListGroup>,
> > + clk_type: QEMUClockType,
>
> Please take a ClockType instead.
Sure.
> > + scale: u32,
> > + attributes: u32,
> > + _f: F,
> > + opaque: &'opaque T,
> > + ) where
> > + F: for<'a> FnCall<(&'a T,)>,
> > + {
> > + /// timer expiration callback
> > + unsafe extern "C" fn rust_timer_handler<T, F: for<'a> FnCall<(&'a T,)>>(
> > + opaque: *mut c_void,
> > + ) {
> > + // SAFETY: the opaque was passed as a reference to `T`.
> > + F::call((unsafe { &*(opaque.cast::<T>()) },))
> > + }
>
> Please add "let _: () = F::ASSERT_IS_SOME;", which is added by the
> qdev_init_clock_in() patch.
Sure. Added it to the beginning of this function.
> > + let timer_cb: unsafe extern "C" fn(*mut c_void) = rust_timer_handler::<T, F>;
> > +
> > + // SAFETY: the opaque outlives the timer
> > + unsafe {
> > + timer_init_full(
> > + self,
> > + if let Some(g) = timer_list_group {
> > + g as *const QEMUTimerListGroup as *mut QEMUTimerListGroup
> > + } else {
> > + ::core::ptr::null_mut()
> > + },
> > + clk_type,
> > + scale as c_int,
> > + attributes as c_int,
> > + Some(timer_cb),
> > + (opaque as *const T).cast::<c_void>() as *mut c_void,
> > + )
> > + }
> > + }
> > +
> > + pub fn timer_mod(&mut self, expire_time: u64) {
> > + unsafe { timer_mod(self as *mut QEMUTimer, expire_time as i64) }
> > + }
>
> This can take &self, because timers are thread-safe:
>
> pub fn timer_mod(&self, expire_time: u64) {
> unsafe { timer_mod(self.as_mut_ptr(), expire_time as i64) }
> }
timer_mod means "modify a timer", so I'd rename this method to "modify"
> const fn as_mut_ptr(&self) -> *mut Self {
> self as *const QEMUTimer as *mut _
> }
Thanks!
> > +}
> > +
> > +impl Drop for QEMUTimer {
> > + fn drop(&mut self) {
> > + unsafe { timer_del(self as *mut QEMUTimer) }
> > + }
>
> timer_del() can be useful even outside Drop, so
>
> pub fn timer_del(&self) {
> unsafe { timer_del(self.as_mut_ptr()) }
> }
>
> and just use self.timer_del() here.
OK, will also rename "timer_del" to "delete".
> > +}
> > +
> > +pub fn qemu_clock_get_virtual_ns() -> u64 {
> > + // SAFETY:
> > + // Valid parameter.
> > + (unsafe { qemu_clock_get_ns(QEMUClockType::QEMU_CLOCK_VIRTUAL) }) as u64
> > +}
>
> Not needed.
Yes!
> > +pub struct ClockType {
> > + pub id: QEMUClockType,
> > +}
>
> The field does not have to be "pub" (maybe "pub(self)", I'm not sure
> offhand).
>
You're right! Making it private is enough, since I should also make
timer_init_full accept ClockType instead of QEMUClockType.
Thanks,
Zhao
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 06/10] rust: add bindings for timer
2025-02-07 13:33 ` Zhao Liu
@ 2025-02-07 14:55 ` Paolo Bonzini
2025-02-08 11:08 ` Zhao Liu
0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-07 14:55 UTC (permalink / raw)
To: Zhao Liu
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P. Berrangé, qemu-devel, qemu-rust
On 2/7/25 14:33, Zhao Liu wrote:
>>> +pub use bindings::QEMUTimer;
>>> +
>>> +use crate::{
>>> + bindings::{
>>> + self, qemu_clock_get_ns, timer_del, timer_init_full, timer_mod, QEMUClockType,
>>> + QEMUTimerListGroup,
>>> + },
>>> + callbacks::FnCall,
>>> +};
>>> +
>>> +impl QEMUTimer {
>>> + pub fn new() -> Self {
>>> + Default::default()
>>> + }
>>> +
>>> + pub fn timer_init_full<'timer, 'opaque: 'timer, T, F>(
>>
>> General question - should the names:
>>
>> - include the "timer" part, matching QEMU C code, or exclude it to avoid
>> repetition? I would say remove it,
>
> I agree and I would name it "init()" instead of "init_full()".
Please keep init_full(); init() would be a version without some of the
arguments (e.g. the TimerListGroup, or the attributes).
> I notice you've picked another way for IRQState, so I could follow that
> like:
>
> pub type Timer = bindings::QEMUTimer;
>
> This style make it easy to add doc (timer binding currently lacks
> doc, but I will add it as much as possible).
Good point.
> Another option may be to wrap QEMUTimer as what MemoryRegionOps did, but
> timer has only 1 callback so I think it's not necessary.
Yes, and we actually should do it sooner or later to add a PhantomPinned
field, because timers can't move in memory! But no need to do it now.
>>> + scale: u32,
While at it, can you add constants for the scale, i.e.
pub const NS: u32 = bindings::SCALE_NS;
pub const US: u32 = bindings::SCALE_US;
pub const MS: u32 = bindings::SCALE_MS;
? Using Timer::NS is clear enough and removes the need to import from
"bindings". At least in theory, bindings should not even have to be
"pub" (or at least that's where the code should move towards).
>>> + pub fn timer_mod(&mut self, expire_time: u64) {
>>> + unsafe { timer_mod(self as *mut QEMUTimer, expire_time as i64) }
>>> + }
>>
>> This can take &self, because timers are thread-safe:
>>
>> pub fn timer_mod(&self, expire_time: u64) {
>> unsafe { timer_mod(self.as_mut_ptr(), expire_time as i64) }
>> }
>
> timer_mod means "modify a timer", so I'd rename this method to "modify"
Yeah, changing mod/del to modify/delete is fine!
Paolo
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 06/10] rust: add bindings for timer
2025-02-07 14:55 ` Paolo Bonzini
@ 2025-02-08 11:08 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-08 11:08 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P. Berrangé, qemu-devel, qemu-rust
> Please keep init_full(); init() would be a version without some of the
> arguments (e.g. the TimerListGroup, or the attributes).
Done.
...
> > > > + scale: u32,
>
> While at it, can you add constants for the scale, i.e.
>
> pub const NS: u32 = bindings::SCALE_NS;
> pub const US: u32 = bindings::SCALE_US;
> pub const MS: u32 = bindings::SCALE_MS;
>
> ? Using Timer::NS is clear enough and removes the need to import from
> "bindings". At least in theory, bindings should not even have to be "pub"
> (or at least that's where the code should move towards).
Great! I realized this case, too.
Thanks,
Zhao
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 07/10] rust/timer/hpet: define hpet_cfg
2025-01-25 12:51 [PATCH 00/10] rust: Add HPET timer device Zhao Liu
` (5 preceding siblings ...)
2025-01-25 12:51 ` [PATCH 06/10] rust: add bindings for timer Zhao Liu
@ 2025-01-25 12:51 ` Zhao Liu
2025-01-29 10:58 ` Paolo Bonzini
2025-01-25 12:51 ` [PATCH 08/10] rust/timer/hpet: add basic HPET timer and HPETState Zhao Liu
` (2 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Zhao Liu @ 2025-01-25 12:51 UTC (permalink / raw)
To: Paolo Bonzini, Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé
Cc: qemu-devel, qemu-rust, Zhao Liu
Define HPETFwEntry structure with the same memory layout as
hpet_fw_entry in C.
Further, define the global hpet_cfg variable in Rust which is the
same as the C version. This hpet_cfg variable in Rust will replace
the C version one and allows both Rust code and C code to access it.
The Rust version of hpet_cfg is self-contained, avoiding unsafe
access to C code.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC:
* hpet_fw_cfg doesn't need a BqlRefCell.
---
rust/Cargo.lock | 8 +++
rust/Cargo.toml | 1 +
rust/hw/meson.build | 1 +
rust/hw/timer/hpet/Cargo.toml | 14 +++++
rust/hw/timer/hpet/meson.build | 18 +++++++
rust/hw/timer/hpet/src/fw_cfg.rs | 87 ++++++++++++++++++++++++++++++++
rust/hw/timer/hpet/src/lib.rs | 13 +++++
rust/hw/timer/meson.build | 1 +
rust/qemu-api/src/zeroable.rs | 6 ++-
9 files changed, 147 insertions(+), 2 deletions(-)
create mode 100644 rust/hw/timer/hpet/Cargo.toml
create mode 100644 rust/hw/timer/hpet/meson.build
create mode 100644 rust/hw/timer/hpet/src/fw_cfg.rs
create mode 100644 rust/hw/timer/hpet/src/lib.rs
create mode 100644 rust/hw/timer/meson.build
diff --git a/rust/Cargo.lock b/rust/Cargo.lock
index c0c6069247a8..79e142723b83 100644
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -37,6 +37,14 @@ version = "1.12.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "3dca9240753cf90908d7e4aac30f630662b02aebaa1b58a3cadabdb23385b58b"
+[[package]]
+name = "hpet"
+version = "0.1.0"
+dependencies = [
+ "qemu_api",
+ "qemu_api_macros",
+]
+
[[package]]
name = "itertools"
version = "0.11.0"
diff --git a/rust/Cargo.toml b/rust/Cargo.toml
index 5b6b6ca43825..9265d30f46a2 100644
--- a/rust/Cargo.toml
+++ b/rust/Cargo.toml
@@ -4,6 +4,7 @@ members = [
"qemu-api-macros",
"qemu-api",
"hw/char/pl011",
+ "hw/timer/hpet",
]
[workspace.lints.rust]
diff --git a/rust/hw/meson.build b/rust/hw/meson.build
index 860196645e71..9749d4adfc96 100644
--- a/rust/hw/meson.build
+++ b/rust/hw/meson.build
@@ -1 +1,2 @@
subdir('char')
+subdir('timer')
diff --git a/rust/hw/timer/hpet/Cargo.toml b/rust/hw/timer/hpet/Cargo.toml
new file mode 100644
index 000000000000..db2ef4642b4f
--- /dev/null
+++ b/rust/hw/timer/hpet/Cargo.toml
@@ -0,0 +1,14 @@
+[package]
+name = "hpet"
+version = "0.1.0"
+edition = "2021"
+authors = ["Zhao Liu <zhao1.liu@intel.com>"]
+license = "GPL-2.0-or-later"
+description = "IA-PC High Precision Event Timer emulation in Rust"
+
+[lib]
+crate-type = ["staticlib"]
+
+[dependencies]
+qemu_api = { path = "../../../qemu-api" }
+qemu_api_macros = { path = "../../../qemu-api-macros" }
diff --git a/rust/hw/timer/hpet/meson.build b/rust/hw/timer/hpet/meson.build
new file mode 100644
index 000000000000..c2d7c0532ca4
--- /dev/null
+++ b/rust/hw/timer/hpet/meson.build
@@ -0,0 +1,18 @@
+_libhpet_rs = static_library(
+ 'hpet',
+ files('src/lib.rs'),
+ override_options: ['rust_std=2021', 'build.rust_std=2021'],
+ rust_abi: 'rust',
+ dependencies: [
+ qemu_api,
+ qemu_api_macros,
+ ],
+)
+
+rust_devices_ss.add(when: 'CONFIG_X_HPET_RUST', if_true: [declare_dependency(
+ link_whole: [_libhpet_rs],
+ # Putting proc macro crates in `dependencies` is necessary for Meson to find
+ # them when compiling the root per-target static rust lib.
+ dependencies: [qemu_api_macros],
+ variables: {'crate': 'hpet'},
+)])
diff --git a/rust/hw/timer/hpet/src/fw_cfg.rs b/rust/hw/timer/hpet/src/fw_cfg.rs
new file mode 100644
index 000000000000..2f72bf854a66
--- /dev/null
+++ b/rust/hw/timer/hpet/src/fw_cfg.rs
@@ -0,0 +1,87 @@
+// Copyright (C) 2024 Intel Corporation.
+// Author(s): Zhao Liu <zhai1.liu@intel.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#![allow(dead_code)]
+
+use qemu_api::{cell::bql_locked, impl_zeroable, zeroable::Zeroable};
+
+// Each HPETState represents a Event Timer Block. The v1 spec supports
+// up to 8 blocks. QEMU only uses 1 block (in PC machine).
+const HPET_MAX_NUM_EVENT_TIMER_BLOCK: usize = 8;
+
+#[repr(C, packed)]
+#[derive(Copy, Clone, Default)]
+pub struct HPETFwEntry {
+ pub event_timer_block_id: u32,
+ pub address: u64,
+ pub min_tick: u16,
+ pub page_prot: u8,
+}
+
+unsafe impl Zeroable for HPETFwEntry {
+ const ZERO: Self = Self {
+ event_timer_block_id: 0,
+ address: 0,
+ min_tick: 0,
+ page_prot: 0,
+ };
+}
+
+#[repr(C, packed)]
+#[derive(Copy, Clone, Default)]
+pub struct HPETFwConfig {
+ pub count: u8,
+ pub hpet: [HPETFwEntry; HPET_MAX_NUM_EVENT_TIMER_BLOCK],
+}
+
+impl_zeroable!(HPETFwConfig);
+
+#[allow(non_upper_case_globals)]
+#[no_mangle]
+pub static mut hpet_fw_cfg: HPETFwConfig = HPETFwConfig {
+ count: u8::MAX,
+ ..Zeroable::ZERO
+};
+
+impl HPETFwConfig {
+ pub(crate) fn assign_hpet_id() -> usize {
+ assert!(bql_locked());
+ // SAFETY: all accesses go through these methods, which guarantee
+ // that the accesses are protected by the BQL.
+ let fw_cfg = unsafe { &mut hpet_fw_cfg };
+
+ if fw_cfg.count == u8::MAX {
+ // first instance
+ fw_cfg.count = 0;
+ }
+
+ if fw_cfg.count == 8 {
+ // TODO: Add error binding: error_setg()
+ panic!("Only 8 instances of HPET is allowed");
+ }
+
+ let id: usize = fw_cfg.count.into();
+ fw_cfg.count += 1;
+ id
+ }
+
+ pub(crate) fn update_hpet_cfg(
+ hpet_id: usize,
+ event_timer_block_id: Option<u32>,
+ address: Option<u64>,
+ ) {
+ assert!(bql_locked());
+ // SAFETY: all accesses go through these methods, which guarantee
+ // that the accesses are protected by the BQL.
+ let fw_cfg = unsafe { &mut hpet_fw_cfg };
+
+ if let Some(e) = event_timer_block_id {
+ fw_cfg.hpet[hpet_id].event_timer_block_id = e;
+ }
+
+ if let Some(a) = address {
+ fw_cfg.hpet[hpet_id].address = a;
+ }
+ }
+}
diff --git a/rust/hw/timer/hpet/src/lib.rs b/rust/hw/timer/hpet/src/lib.rs
new file mode 100644
index 000000000000..c2b9c64d0bfe
--- /dev/null
+++ b/rust/hw/timer/hpet/src/lib.rs
@@ -0,0 +1,13 @@
+// Copyright (C) 2024 Intel Corporation.
+// Author(s): Zhao Liu <zhai1.liu@intel.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+//
+// HPET QEMU Device Model
+//
+// This library implements a device model for the IA-PC HPET (High
+// Precision Event Timers) device in QEMU.
+//
+
+#![deny(unsafe_op_in_unsafe_fn)]
+
+pub mod fw_cfg;
diff --git a/rust/hw/timer/meson.build b/rust/hw/timer/meson.build
new file mode 100644
index 000000000000..22a84f15536b
--- /dev/null
+++ b/rust/hw/timer/meson.build
@@ -0,0 +1 @@
+subdir('hpet')
diff --git a/rust/qemu-api/src/zeroable.rs b/rust/qemu-api/src/zeroable.rs
index 9f009606b1ab..cd424e6ea050 100644
--- a/rust/qemu-api/src/zeroable.rs
+++ b/rust/qemu-api/src/zeroable.rs
@@ -56,6 +56,7 @@ pub unsafe trait Zeroable: Default {
/// ## Differences with `core::mem::zeroed`
///
/// `const_zero` zeroes padding bits, while `core::mem::zeroed` doesn't
+#[macro_export]
macro_rules! const_zero {
// This macro to produce a type-generic zero constant is taken from the
// const_zero crate (v0.1.1):
@@ -77,10 +78,11 @@ union TypeAsBytes {
}
/// A wrapper to implement the `Zeroable` trait through the `const_zero` macro.
+#[macro_export]
macro_rules! impl_zeroable {
($type:ty) => {
- unsafe impl Zeroable for $type {
- const ZERO: Self = unsafe { const_zero!($type) };
+ unsafe impl $crate::zeroable::Zeroable for $type {
+ const ZERO: Self = unsafe { $crate::const_zero!($type) };
}
};
}
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 07/10] rust/timer/hpet: define hpet_cfg
2025-01-25 12:51 ` [PATCH 07/10] rust/timer/hpet: define hpet_cfg Zhao Liu
@ 2025-01-29 10:58 ` Paolo Bonzini
2025-02-07 14:30 ` Zhao Liu
0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-01-29 10:58 UTC (permalink / raw)
To: Zhao Liu
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé, qemu-devel, qemu-rust, Zhao Liu
On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> @@ -5,6 +5,7 @@ edition = "2021"
> authors = ["Zhao Liu <zhao1.liu@intel.com>"]
> license = "GPL-2.0-or-later"
> description = "IA-PC High Precision Event Timer emulation in Rust"
Please add
rust-version = "1.63.0"
here.
> + // SAFETY: all accesses go through these methods, which guarantee
> + // that the accesses are protected by the BQL.
> + let fw_cfg = unsafe { &mut hpet_fw_cfg };
Clippy complains about references to static mut; use
let mut fw_cfg = unsafe { *addr_of_mut!(hpet_fw_cfg) };
to make it happy; same below in update_hpet_cfg().
Paolo
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 07/10] rust/timer/hpet: define hpet_cfg
2025-01-29 10:58 ` Paolo Bonzini
@ 2025-02-07 14:30 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-07 14:30 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé, qemu-devel, qemu-rust
On Wed, Jan 29, 2025 at 11:58:46AM +0100, Paolo Bonzini wrote:
> Date: Wed, 29 Jan 2025 11:58:46 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 07/10] rust/timer/hpet: define hpet_cfg
>
>
>
> On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> > @@ -5,6 +5,7 @@ edition = "2021"
> > authors = ["Zhao Liu <zhao1.liu@intel.com>"]
> > license = "GPL-2.0-or-later"
> > description = "IA-PC High Precision Event Timer emulation in Rust"
>
> Please add
>
> rust-version = "1.63.0"
>
> here.
Done.
> > + // SAFETY: all accesses go through these methods, which guarantee
> > + // that the accesses are protected by the BQL.
> > + let fw_cfg = unsafe { &mut hpet_fw_cfg };
>
> Clippy complains about references to static mut; use
>
> let mut fw_cfg = unsafe { *addr_of_mut!(hpet_fw_cfg) };
>
> to make it happy; same below in update_hpet_cfg().
>
Fixed. Thanks!!
Regards,
Zhao
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 08/10] rust/timer/hpet: add basic HPET timer and HPETState
2025-01-25 12:51 [PATCH 00/10] rust: Add HPET timer device Zhao Liu
` (6 preceding siblings ...)
2025-01-25 12:51 ` [PATCH 07/10] rust/timer/hpet: define hpet_cfg Zhao Liu
@ 2025-01-25 12:51 ` Zhao Liu
2025-01-29 10:57 ` Paolo Bonzini
2025-01-25 12:51 ` [PATCH 09/10] rust/timer/hpet: add qom and qdev APIs support Zhao Liu
2025-01-25 12:51 ` [PATCH 10/10] i386: enable rust hpet for pc when rust is enabled Zhao Liu
9 siblings, 1 reply; 34+ messages in thread
From: Zhao Liu @ 2025-01-25 12:51 UTC (permalink / raw)
To: Paolo Bonzini, Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé
Cc: qemu-devel, qemu-rust, Zhao Liu
Add the HPETTimer and HPETState (HPET timer block), along with their
basic methods and register definitions.
This is in preparation for supporting the QAPI interfaces.
Note, wrap all items in HPETState that may be changed in the callback
called by C code into the BqlCell/BqlRefCell.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC:
* Don't wrap HPETState.flags in a BqlCell.
* Consolidate inmutable &self and QOM casting.
* Prefer timer iterator over loop.
* Use BqlRefCell<HPETTimer> directly to store timers in HPETState.
---
rust/hw/timer/hpet/src/hpet.rs | 633 +++++++++++++++++++++++++++++++++
rust/hw/timer/hpet/src/lib.rs | 1 +
rust/wrapper.h | 1 +
3 files changed, 635 insertions(+)
create mode 100644 rust/hw/timer/hpet/src/hpet.rs
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
new file mode 100644
index 000000000000..0884a2ac73c4
--- /dev/null
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -0,0 +1,633 @@
+// Copyright (C) 2024 Intel Corporation.
+// Author(s): Zhao Liu <zhai1.liu@intel.com>
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#![allow(dead_code)]
+
+use core::ptr::{null_mut, NonNull};
+
+use qemu_api::{
+ bindings::{address_space_memory, address_space_stl_le, MemoryRegion, SCALE_NS},
+ cell::{BqlCell, BqlRefCell},
+ irq::InterruptSource,
+ memory::MEMTXATTRS_UNSPECIFIED,
+ prelude::*,
+ qom::ParentField,
+ sysbus::SysBusDevice,
+ timer::{QEMUTimer, CLOCK_VIRTUAL},
+};
+
+// Register space for each timer block. (HPET_BASE isn't defined here.)
+const HPET_REG_SPACE_LEN: u64 = 0x400; // 1024 bytes
+
+const HPET_MIN_TIMERS: usize = 3; // Minimum recommended hardware implementation.
+const HPET_MAX_TIMERS: usize = 32; // Maximum timers in each timer block.
+
+// Flags that HPETState.flags supports.
+const HPET_FLAG_MSI_SUPPORT_SHIFT: usize = 0;
+
+const HPET_NUM_IRQ_ROUTES: usize = 32;
+const HPET_LEGACY_PIT_INT: u32 = 0; // HPET_LEGACY_RTC_INT isn't defined here.
+const RTC_ISA_IRQ: usize = 8;
+
+const HPET_CLK_PERIOD: u64 = 10; // 10 ns
+const FS_PER_NS: u64 = 1000000; // 1000000 femtoseconds == 1 ns
+
+// General Capabilities and ID Register
+const HPET_CAP_REG: u64 = 0x000;
+// Revision ID (bits 0:7)
+const HPET_CAP_REV_ID_VALUE: u64 = 0x1; // Revision 1 is implemented (refer to v1.0a spec).
+const HPET_CAP_REV_ID_SHIFT: usize = 0;
+// Number of Timers (bits 8:12)
+const HPET_CAP_NUM_TIM_SHIFT: usize = 8;
+// Counter Size (bit 13)
+const HPET_CAP_COUNT_SIZE_CAP_SHIFT: usize = 13;
+// LegacyReplacement Route Capable (bit 15)
+const HPET_CAP_LEG_RT_CAP_SHIFT: usize = 15;
+// Vendor ID (bits 16:31)
+const HPET_CAP_VENDER_ID_VALUE: u64 = 0x8086;
+const HPET_CAP_VENDER_ID_SHIFT: usize = 16;
+// Main Counter Tick Period (bits 32:63)
+const HPET_CAP_CNT_CLK_PERIOD_SHIFT: usize = 32;
+
+// General Configuration Register
+const HPET_CFG_REG: u64 = 0x010;
+// Overall Enable (bit 0)
+const HPET_CFG_ENABLE_SHIFT: usize = 0;
+// LegacyReplacement Route (bit 1)
+const HPET_CFG_LEG_RT_SHIFT: usize = 1;
+// Other bits are reserved.
+const HPET_CFG_WRITE_MASK: u64 = 0x003;
+
+// General Interrupt Status Register
+const HPET_INT_STATUS_REG: u64 = 0x020;
+
+// Main Counter Value Register
+const HPET_COUNTER_REG: u64 = 0x0f0;
+
+// Timer N Configuration and Capability Register (masked by 0x18)
+const HPET_TN_CFG_REG: u64 = 0x000;
+// bit 0, 7, and bits 16:31 are reserved.
+// bit 4, 5, 15, and bits 32:64 are read-only.
+const HPET_TN_CFG_WRITE_MASK: u64 = 0x7f4e;
+// Timer N Interrupt Type (bit 1)
+const HPET_TN_CFG_INT_TYPE_SHIFT: usize = 1;
+// Timer N Interrupt Enable (bit 2)
+const HPET_TN_CFG_INT_ENABLE_SHIFT: usize = 2;
+// Timer N Type (Periodic enabled or not, bit 3)
+const HPET_TN_CFG_PERIODIC_SHIFT: usize = 3;
+// Timer N Periodic Interrupt Capable (support Periodic or not, bit 4)
+const HPET_TN_CFG_PERIODIC_CAP_SHIFT: usize = 4;
+// Timer N Size (timer size is 64-bits or 32 bits, bit 5)
+const HPET_TN_CFG_SIZE_CAP_SHIFT: usize = 5;
+// Timer N Value Set (bit 6)
+const HPET_TN_CFG_SETVAL_SHIFT: usize = 6;
+// Timer N 32-bit Mode (bit 8)
+const HPET_TN_CFG_32BIT_SHIFT: usize = 8;
+// Timer N Interrupt Rout (bits 9:13)
+const HPET_TN_CFG_INT_ROUTE_MASK: u64 = 0x3e00;
+const HPET_TN_CFG_INT_ROUTE_SHIFT: usize = 9;
+// Timer N FSB Interrupt Enable (bit 14)
+const HPET_TN_CFG_FSB_ENABLE_SHIFT: usize = 14;
+// Timer N FSB Interrupt Delivery (bit 15)
+const HPET_TN_CFG_FSB_CAP_SHIFT: usize = 15;
+// Timer N Interrupt Routing Capability (bits 32:63)
+const HPET_TN_CFG_INT_ROUTE_CAP_SHIFT: usize = 32;
+
+// Timer N Comparator Value Register (masked by 0x18)
+const HPET_TN_CMP_REG: u64 = 0x008;
+
+// Timer N FSB Interrupt Route Register (masked by 0x18)
+const HPET_TN_FSB_ROUTE_REG: u64 = 0x010;
+
+fn hpet_next_wrap(cur_tick: u64) -> u64 {
+ (cur_tick | 0xffffffff) + 1
+}
+
+fn hpet_time_after(a: u64, b: u64) -> bool {
+ ((b - a) as i64) < 0
+}
+
+fn ticks_to_ns(value: u64) -> u64 {
+ value * HPET_CLK_PERIOD
+}
+
+fn ns_to_ticks(value: u64) -> u64 {
+ value / HPET_CLK_PERIOD
+}
+
+// Avoid touching the bits that cannot be written.
+fn hpet_fixup_reg(new: u64, old: u64, mask: u64) -> u64 {
+ (new & mask) | (old & !mask)
+}
+
+fn activating_bit(old: u64, new: u64, shift: usize) -> bool {
+ let mask: u64 = 1 << shift;
+ (old & mask == 0) && (new & mask != 0)
+}
+
+fn deactivating_bit(old: u64, new: u64, shift: usize) -> bool {
+ let mask: u64 = 1 << shift;
+ (old & mask != 0) && (new & mask == 0)
+}
+
+fn timer_handler(timer_cell: &BqlRefCell<HPETTimer>) {
+ timer_cell.borrow_mut().callback()
+}
+
+/// HPET Timer Abstraction
+#[repr(C)]
+#[derive(Debug, Default, qemu_api_macros::offsets)]
+pub struct HPETTimer {
+ /// timer N index within the timer block (HPETState)
+ #[doc(alias = "tn")]
+ index: usize,
+ qemu_timer: Option<Box<QEMUTimer>>,
+ /// timer block abstraction containing this timer
+ state: Option<NonNull<HPETState>>,
+
+ /// Memory-mapped, software visible timer registers
+
+ /// Timer N Configuration and Capability Register
+ config: u64,
+ /// Timer N Comparator Value Register
+ cmp: u64,
+ /// Timer N FSB Interrupt Route Register
+ fsb: u64,
+
+ /// Hidden register state
+
+ /// comparator (extended to counter width)
+ cmp64: u64,
+ /// Last value written to comparator
+ period: u64,
+ /// timer pop will indicate wrap for one-shot 32-bit
+ /// mode. Next pop will be actual timer expiration.
+ wrap_flag: u8,
+ /// last value armed, to avoid timer storms
+ last: u64,
+}
+
+impl HPETTimer {
+ fn init(&mut self, index: usize, state_ptr: *mut HPETState) -> &mut Self {
+ *self = HPETTimer::default();
+ self.index = index;
+ self.state = NonNull::new(state_ptr);
+ self
+ }
+
+ fn init_timer_with_state(&mut self) {
+ self.qemu_timer = Some(Box::new({
+ let mut t = QEMUTimer::new();
+ t.timer_init_full(
+ None,
+ CLOCK_VIRTUAL.id,
+ SCALE_NS,
+ 0,
+ timer_handler,
+ &self.get_state().timers[self.index],
+ );
+ t
+ }));
+ }
+
+ fn get_state(&self) -> &HPETState {
+ // SAFETY:
+ // the pointer is convertible to a reference
+ unsafe { self.state.unwrap().as_ref() }
+ }
+
+ fn is_int_active(&self) -> bool {
+ self.get_state().is_timer_int_active(self.index)
+ }
+
+ fn is_fsb_route_enabled(&self) -> bool {
+ self.config & 1 << HPET_TN_CFG_FSB_ENABLE_SHIFT != 0
+ }
+
+ fn is_periodic(&self) -> bool {
+ self.config & 1 << HPET_TN_CFG_PERIODIC_SHIFT != 0
+ }
+
+ fn is_int_enabled(&self) -> bool {
+ self.config & 1 << HPET_TN_CFG_INT_ENABLE_SHIFT != 0
+ }
+
+ fn is_32bit_mod(&self) -> bool {
+ self.config & 1 << HPET_TN_CFG_32BIT_SHIFT != 0
+ }
+
+ fn is_valset_enabled(&self) -> bool {
+ self.config & 1 << HPET_TN_CFG_SETVAL_SHIFT != 0
+ }
+
+ fn clear_valset(&mut self) {
+ self.config &= !(1 << HPET_TN_CFG_SETVAL_SHIFT);
+ }
+
+ /// True if timer interrupt is level triggered; otherwise, edge triggered.
+ fn is_int_level_triggered(&self) -> bool {
+ self.config & 1 << HPET_TN_CFG_INT_TYPE_SHIFT != 0
+ }
+
+ /// calculate next value of the general counter that matches the
+ /// target (either entirely, or the low 32-bit only depending on
+ /// the timer mode).
+ fn calculate_cmp64(&self, cur_tick: u64, target: u64) -> u64 {
+ if self.is_32bit_mod() {
+ let mut result: u64 = cur_tick.deposit(0, 32, target);
+ if result < cur_tick {
+ result += 0x100000000;
+ }
+ result
+ } else {
+ target
+ }
+ }
+
+ fn get_individual_route(&self) -> usize {
+ ((self.config & HPET_TN_CFG_INT_ROUTE_MASK) >> HPET_TN_CFG_INT_ROUTE_SHIFT) as usize
+ }
+
+ fn get_int_route(&self) -> usize {
+ if self.index <= 1 && self.get_state().is_legacy_mode() {
+ // If LegacyReplacement Route bit is set, HPET specification requires
+ // timer0 be routed to IRQ0 in NON-APIC or IRQ2 in the I/O APIC,
+ // timer1 be routed to IRQ8 in NON-APIC or IRQ8 in the I/O APIC.
+ //
+ // If the LegacyReplacement Route bit is set, the individual routing
+ // bits for timers 0 and 1 (APIC or FSB) will have no impact.
+ //
+ // FIXME: Consider I/O APIC case.
+ if self.index == 0 {
+ 0
+ } else {
+ RTC_ISA_IRQ
+ }
+ } else {
+ // (If the LegacyReplacement Route bit is set) Timer 2-n will be
+ // routed as per the routing in the timer n config registers.
+ // ...
+ // If the LegacyReplacement Route bit is not set, the individual
+ // routing bits for each of the timers are used.
+ self.get_individual_route()
+ }
+ }
+
+ fn set_irq(&mut self, set: bool) {
+ let route = self.get_int_route();
+
+ if set && self.is_int_enabled() && self.get_state().is_hpet_enabled() {
+ if self.is_fsb_route_enabled() {
+ // SAFETY:
+ // the parameters are valid.
+ unsafe {
+ address_space_stl_le(
+ &mut address_space_memory,
+ self.fsb >> 32, // Timer N FSB int addr
+ self.fsb as u32, // Timer N FSB int value, truncate!
+ MEMTXATTRS_UNSPECIFIED,
+ null_mut(),
+ );
+ }
+ } else if self.is_int_level_triggered() {
+ self.get_state().irqs[route].raise();
+ } else {
+ self.get_state().irqs[route].pulse();
+ }
+ } else if !self.is_fsb_route_enabled() {
+ self.get_state().irqs[route].lower();
+ }
+ }
+
+ fn update_irq(&mut self, set: bool) {
+ // If Timer N Interrupt Enable bit is 0, "the timer will
+ // still operate and generate appropriate status bits, but
+ // will not cause an interrupt"
+ self.get_state()
+ .update_int_status(self.index as u32, set && self.is_int_level_triggered());
+ self.set_irq(set);
+ }
+
+ fn arm_timer(&mut self, tick: u64) {
+ let mut ns = self.get_state().get_ns(tick);
+
+ // Clamp period to reasonable min value (1 us)
+ if self.is_periodic() && ns - self.last < 1000 {
+ ns = self.last + 1000;
+ }
+
+ self.last = ns;
+ if let Some(timer) = self.qemu_timer.as_mut() {
+ timer.timer_mod(self.last);
+ }
+ }
+
+ fn set_timer(&mut self) {
+ let cur_tick: u64 = self.get_state().get_ticks();
+
+ self.wrap_flag = 0;
+ self.cmp64 = self.calculate_cmp64(cur_tick, self.cmp);
+ if self.is_32bit_mod() {
+ // HPET spec says in one-shot 32-bit mode, generate an interrupt when
+ // counter wraps in addition to an interrupt with comparator match.
+ if !self.is_periodic() && self.cmp64 > hpet_next_wrap(cur_tick) {
+ self.wrap_flag = 1;
+ self.arm_timer(hpet_next_wrap(cur_tick));
+ return;
+ }
+ }
+ self.arm_timer(self.cmp64);
+ }
+
+ fn del_timer(&mut self) {
+ if let Some(timer) = self.qemu_timer.take() {
+ std::mem::drop(timer);
+ }
+
+ if self.is_int_active() {
+ // For level-triggered interrupt, this leaves interrupt status
+ // register set but lowers irq.
+ self.update_irq(true);
+ }
+ }
+
+ // Configuration and Capability Register
+ fn set_tn_cfg_reg(&mut self, shift: u32, len: u32, val: u64) {
+ // TODO: Add trace point - trace_hpet_ram_write_tn_cfg(addr & 4)
+ let old_val: u64 = self.config;
+ let mut new_val: u64 = old_val.deposit(shift, len, val);
+ new_val = hpet_fixup_reg(new_val, old_val, HPET_TN_CFG_WRITE_MASK);
+
+ // Switch level-type interrupt to edge-type.
+ if deactivating_bit(old_val, new_val, HPET_TN_CFG_INT_TYPE_SHIFT) {
+ // Do this before changing timer.config; otherwise, if
+ // HPET_TN_FSB is set, update_irq will not lower the qemu_irq.
+ self.update_irq(false);
+ }
+
+ self.config = new_val;
+
+ if activating_bit(old_val, new_val, HPET_TN_CFG_INT_ENABLE_SHIFT) && self.is_int_active() {
+ self.update_irq(true);
+ }
+
+ if self.is_32bit_mod() {
+ self.cmp = self.cmp as u32 as u64; // truncate!
+ self.period = self.period as u32 as u64; // truncate!
+ }
+
+ if self.get_state().is_hpet_enabled() {
+ self.set_timer();
+ }
+ }
+
+ /// Comparator Value Register
+ fn set_tn_cmp_reg(&mut self, shift: u32, len: u32, val: u64) {
+ let mut length = len;
+ let mut value = val;
+
+ // TODO: Add trace point - trace_hpet_ram_write_tn_cmp(addr & 4)
+ if self.is_32bit_mod() {
+ // High 32-bits are zero, leave them untouched.
+ if shift != 0 {
+ // TODO: Add trace point - trace_hpet_ram_write_invalid_tn_cmp()
+ return;
+ }
+ length = 64;
+ value = value as u32 as u64; // truncate!
+ }
+
+ if !self.is_periodic() || self.is_valset_enabled() {
+ self.cmp = self.cmp.deposit(shift, length, value);
+ }
+
+ if self.is_periodic() {
+ self.period = self.period.deposit(shift, length, value);
+ }
+
+ self.clear_valset();
+ if self.get_state().is_hpet_enabled() {
+ self.set_timer();
+ }
+ }
+
+ /// FSB Interrupt Route Register
+ fn set_tn_fsb_route_reg(&mut self, shift: u32, len: u32, val: u64) {
+ self.fsb = self.fsb.deposit(shift, len, val);
+ }
+
+ fn reset(&mut self) {
+ self.del_timer();
+ self.cmp = u64::MAX; // Comparator Match Registers reset to all 1's.
+ self.config = 1 << HPET_TN_CFG_PERIODIC_CAP_SHIFT | 1 << HPET_TN_CFG_SIZE_CAP_SHIFT;
+ if self.get_state().has_msi_flag() {
+ self.config |= 1 << HPET_TN_CFG_FSB_CAP_SHIFT;
+ }
+ // advertise availability of ioapic int
+ self.config |= (self.get_state().int_route_cap as u64) << HPET_TN_CFG_INT_ROUTE_CAP_SHIFT;
+ self.period = 0;
+ self.wrap_flag = 0;
+ }
+
+ /// timer expiration callback
+ fn callback(&mut self) {
+ let period: u64 = self.period;
+ let cur_tick: u64 = self.get_state().get_ticks();
+
+ if self.is_periodic() && period != 0 {
+ while hpet_time_after(cur_tick, self.cmp64) {
+ self.cmp64 += period;
+ }
+ if self.is_32bit_mod() {
+ self.cmp = self.cmp64 as u32 as u64;
+ } else {
+ self.cmp = self.cmp64;
+ }
+ self.arm_timer(self.cmp64);
+ } else if self.wrap_flag != 0 {
+ self.wrap_flag = 0;
+ self.arm_timer(self.cmp64);
+ }
+ self.update_irq(true);
+ }
+}
+
+/// HPET Event Timer Block Abstraction
+/// Note: Wrap all items that may be changed in the callback called by C
+/// into the BqlCell/BqlRefCell.
+#[repr(C)]
+#[derive(qemu_api_macros::offsets)]
+pub struct HPETState {
+ parent_obj: ParentField<SysBusDevice>,
+ iomem: MemoryRegion,
+
+ /// HPET block Registers: Memory-mapped, software visible registers
+
+ /// General Capabilities and ID Register
+ capability: BqlCell<u64>,
+ /// General Configuration Register
+ config: BqlCell<u64>,
+ /// General Interrupt Status Register
+ #[doc(alias = "isr")]
+ int_status: BqlCell<u64>,
+ /// Main Counter Value Register
+ #[doc(alias = "hpet_counter")]
+ counter: BqlCell<u64>,
+
+ /// Internal state
+
+ /// Capabilities that QEMU HPET supports.
+ /// bit 0: MSI (or FSB) support.
+ flags: u32,
+
+ /// Offset of main counter relative to qemu clock.
+ hpet_offset: BqlCell<u64>,
+ hpet_offset_saved: bool,
+
+ irqs: [InterruptSource; HPET_NUM_IRQ_ROUTES],
+ rtc_irq_level: BqlCell<u32>,
+ pit_enabled: InterruptSource,
+
+ /// Interrupt Routing Capability.
+ /// This field indicates to which interrupts in the I/O (x) APIC
+ /// the timers' interrupt can be routed, and is encoded in the
+ /// bits 32:64 of timer N's config register:
+ #[doc(alias = "intcap")]
+ int_route_cap: u32,
+
+ /// HPET timer array managed by this timer block.
+ #[doc(alias = "timer")]
+ timers: [BqlRefCell<HPETTimer>; HPET_MAX_TIMERS],
+ num_timers: BqlCell<usize>,
+
+ /// Instance id (HPET timer block ID).
+ hpet_id: BqlCell<usize>,
+}
+
+impl HPETState {
+ fn has_msi_flag(&self) -> bool {
+ self.flags & 1 << HPET_FLAG_MSI_SUPPORT_SHIFT != 0
+ }
+
+ fn is_legacy_mode(&self) -> bool {
+ self.config.get() & 1 << HPET_CFG_LEG_RT_SHIFT != 0
+ }
+
+ fn is_hpet_enabled(&self) -> bool {
+ self.config.get() & 1 << HPET_CFG_ENABLE_SHIFT != 0
+ }
+
+ fn is_timer_int_active(&self, index: usize) -> bool {
+ self.int_status.get() & 1 << index != 0
+ }
+
+ fn get_ticks(&self) -> u64 {
+ ns_to_ticks(CLOCK_VIRTUAL.get_ns() + self.hpet_offset.get())
+ }
+
+ fn get_ns(&self, tick: u64) -> u64 {
+ ticks_to_ns(tick) - self.hpet_offset.get()
+ }
+
+ fn handle_legacy_irq(&self, irq: u32, level: u32) {
+ if irq == HPET_LEGACY_PIT_INT {
+ if !self.is_legacy_mode() {
+ self.irqs[0].set(level != 0);
+ }
+ } else {
+ self.rtc_irq_level.set(level);
+ if !self.is_legacy_mode() {
+ self.irqs[RTC_ISA_IRQ].set(level != 0);
+ }
+ }
+ }
+
+ fn init_timer(&self) {
+ let raw_ptr: *mut HPETState = self as *const HPETState as *mut HPETState;
+
+ for (index, timer) in self.timers.iter().enumerate() {
+ timer
+ .borrow_mut()
+ .init(index, raw_ptr)
+ .init_timer_with_state();
+ }
+ }
+
+ fn update_int_status(&self, index: u32, level: bool) {
+ self.int_status
+ .set(self.int_status.get().deposit(index, 1, level as u64));
+ }
+
+ /// General Configuration Register
+ fn set_cfg_reg(&self, shift: u32, len: u32, val: u64) {
+ let old_val = self.config.get();
+ let mut new_val = old_val.deposit(shift, len, val);
+
+ new_val = hpet_fixup_reg(new_val, old_val, HPET_CFG_WRITE_MASK);
+ self.config.set(new_val);
+
+ if activating_bit(old_val, new_val, HPET_CFG_ENABLE_SHIFT) {
+ // Enable main counter and interrupt generation.
+ self.hpet_offset
+ .set(ticks_to_ns(self.counter.get()) - CLOCK_VIRTUAL.get_ns());
+
+ for timer in self.timers.iter().take(self.num_timers.get()) {
+ let mut t = timer.borrow_mut();
+
+ if t.is_int_enabled() && t.is_int_active() {
+ t.update_irq(true);
+ }
+ t.set_timer();
+ }
+ } else if deactivating_bit(old_val, new_val, HPET_CFG_ENABLE_SHIFT) {
+ // Halt main counter and disable interrupt generation.
+ self.counter.set(self.get_ticks());
+
+ for timer in self.timers.iter().take(self.num_timers.get()) {
+ timer.borrow_mut().del_timer();
+ }
+ }
+
+ // i8254 and RTC output pins are disabled when HPET is in legacy mode
+ if activating_bit(old_val, new_val, HPET_CFG_LEG_RT_SHIFT) {
+ self.pit_enabled.set(false);
+ self.irqs[0].lower();
+ self.irqs[RTC_ISA_IRQ].lower();
+ } else if deactivating_bit(old_val, new_val, HPET_CFG_LEG_RT_SHIFT) {
+ // TODO: Add irq binding: qemu_irq_lower(s->irqs[0])
+ self.irqs[0].lower();
+ self.pit_enabled.set(true);
+ self.irqs[RTC_ISA_IRQ].set(self.rtc_irq_level.get() != 0);
+ }
+ }
+
+ /// General Interrupt Status Register: Read/Write Clear
+ fn set_int_status_reg(&self, shift: u32, _len: u32, val: u64) {
+ let new_val = val << shift;
+ let cleared = new_val & self.int_status.get();
+
+ for (index, timer) in self.timers.iter().take(self.num_timers.get()).enumerate() {
+ if cleared & 1 << index != 0 {
+ timer.borrow_mut().update_irq(false);
+ }
+ }
+ }
+
+ /// Main Counter Value Register
+ fn set_counter_reg(&self, shift: u32, len: u32, val: u64) {
+ if self.is_hpet_enabled() {
+ // TODO: Add trace point -
+ // trace_hpet_ram_write_counter_write_while_enabled()
+ //
+ // HPET spec says that writes to this register should only be
+ // done while the counter is halted. So this is an undefined
+ // behavior. There's no need to forbid it, but when HPET is
+ // enabled, the changed counter value will not affect the
+ // tick count (i.e., the previously calculated offset will
+ // not be changed as well).
+ }
+ self.counter
+ .set(self.counter.get().deposit(shift, len, val));
+ }
+}
diff --git a/rust/hw/timer/hpet/src/lib.rs b/rust/hw/timer/hpet/src/lib.rs
index c2b9c64d0bfe..027f7f83349a 100644
--- a/rust/hw/timer/hpet/src/lib.rs
+++ b/rust/hw/timer/hpet/src/lib.rs
@@ -11,3 +11,4 @@
#![deny(unsafe_op_in_unsafe_fn)]
pub mod fw_cfg;
+pub mod hpet;
diff --git a/rust/wrapper.h b/rust/wrapper.h
index a35bfbd1760d..d927ad6799da 100644
--- a/rust/wrapper.h
+++ b/rust/wrapper.h
@@ -64,3 +64,4 @@ typedef enum memory_order {
#include "chardev/char-serial.h"
#include "exec/memattrs.h"
#include "qemu/timer.h"
+#include "exec/address-spaces.h"
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 08/10] rust/timer/hpet: add basic HPET timer and HPETState
2025-01-25 12:51 ` [PATCH 08/10] rust/timer/hpet: add basic HPET timer and HPETState Zhao Liu
@ 2025-01-29 10:57 ` Paolo Bonzini
2025-02-08 8:19 ` Zhao Liu
0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-01-29 10:57 UTC (permalink / raw)
To: Zhao Liu
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé, qemu-devel, qemu-rust, Zhao Liu
On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> +// Register space for each timer block. (HPET_BASE isn't defined here.)
> +const HPET_REG_SPACE_LEN: u64 = 0x400; // 1024 bytes
Use doc comments "///"...
> +// Timer N FSB Interrupt Route Register (masked by 0x18)
> +const HPET_TN_FSB_ROUTE_REG: u64 = 0x010;
... all the way to here.
> +/// HPET Timer Abstraction
> +#[repr(C)]
> +#[derive(Debug, Default, qemu_api_macros::offsets)]
> +pub struct HPETTimer {
> + /// timer N index within the timer block (HPETState)
> + #[doc(alias = "tn")]
> + index: usize,
> + qemu_timer: Option<Box<QEMUTimer>>,
> + /// timer block abstraction containing this timer
> + state: Option<NonNull<HPETState>>,
> +
> + /// Memory-mapped, software visible timer registers
These "headings" cannot be doc comments, because they would be attached
to the field after. Same below:
- /// Hidden register state
+ // Hidden register state
- /// HPET block Registers: Memory-mapped, software visible registers
+ // HPET block Registers: Memory-mapped, software visible registers
- /// Internal state
+ // Internal state
> + fn get_state(&self) -> &HPETState {
> + // SAFETY:
> + // the pointer is convertible to a reference
> + unsafe { self.state.unwrap().as_ref() }
> + }
Note for the future: I looked briefly into why a NonNull<> is needed
and the reasons are two. First, the offset_of! replacement does not
support lifetime parameters. Second, it's messy to pass a &HPETState
to the &HPETTimer, because the HPETTimer still has to be written into
the HPETState. This may be worth revisiting when QEMU starts using
pinned-init is added, but for now is self-contained.
> + if let Some(timer) = self.qemu_timer.as_mut() {
> + timer.timer_mod(self.last);
> + }
I think this can be just "timer.unwrap().timer_mod(self.last)" (not
sure if it needs an as_ref() too).
Paolo
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 08/10] rust/timer/hpet: add basic HPET timer and HPETState
2025-01-29 10:57 ` Paolo Bonzini
@ 2025-02-08 8:19 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-08 8:19 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé, qemu-devel, qemu-rust
On Wed, Jan 29, 2025 at 11:57:18AM +0100, Paolo Bonzini wrote:
> Date: Wed, 29 Jan 2025 11:57:18 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 08/10] rust/timer/hpet: add basic HPET timer and
> HPETState
>
>
>
> On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> > +// Register space for each timer block. (HPET_BASE isn't defined here.)
> > +const HPET_REG_SPACE_LEN: u64 = 0x400; // 1024 bytes
>
> Use doc comments "///"...
OK,
> > +// Timer N FSB Interrupt Route Register (masked by 0x18)
> > +const HPET_TN_FSB_ROUTE_REG: u64 = 0x010;
>
> ... all the way to here.
Done.
> > +/// HPET Timer Abstraction
> > +#[repr(C)]
> > +#[derive(Debug, Default, qemu_api_macros::offsets)]
> > +pub struct HPETTimer {
> > + /// timer N index within the timer block (HPETState)
> > + #[doc(alias = "tn")]
> > + index: usize,
> > + qemu_timer: Option<Box<QEMUTimer>>,
> > + /// timer block abstraction containing this timer
> > + state: Option<NonNull<HPETState>>,
> > +
> > + /// Memory-mapped, software visible timer registers
>
> These "headings" cannot be doc comments, because they would be attached
> to the field after. Same below:
>
> - /// Hidden register state
> + // Hidden register state
>
> - /// HPET block Registers: Memory-mapped, software visible registers
> + // HPET block Registers: Memory-mapped, software visible registers
>
> - /// Internal state
> + // Internal state
Fixed. Thanks!
> > + fn get_state(&self) -> &HPETState {
> > + // SAFETY:
> > + // the pointer is convertible to a reference
> > + unsafe { self.state.unwrap().as_ref() }
> > + }
>
> Note for the future: I looked briefly into why a NonNull<> is needed
> and the reasons are two. First, the offset_of! replacement does not
> support lifetime parameters. Second, it's messy to pass a &HPETState
> to the &HPETTimer, because the HPETTimer still has to be written into
> the HPETState.
Yes, and the second use would be a common case in practice. So maybe we
need some wrapper on this, to provide someting like HPETTimer::get_state(),
which returns a reference of the parent structure.
In addition, for underlying library, NonNull<> is nice thinks to its low
overhead. But for device implementation, I'm not sure if it's the good
choice. Rust uses RefCell<Weak<>> (for QEMU, it could be BqlRefCell<Weak<>>)
to handle this cycle reference case.
As I mentioned in RFC,
The device instance is not created in Rust side, and there's
only init() method to initialize created device instance. This
way, it's hard to be compatible with the pattern of creating
weak references (at least I failed).
To address this issue, the method `from` or `from_raw` seems necessary,
like `Owned::from`, but it's tricky... So I don't have a good idea to
replace NonNull<> yet!
> This may be worth revisiting when QEMU starts using
> pinned-init is added, but for now is self-contained.
>
> > + if let Some(timer) = self.qemu_timer.as_mut() {
> > + timer.timer_mod(self.last);
> > + }
>
> I think this can be just "timer.unwrap().timer_mod(self.last)" (not
> sure if it needs an as_ref() too).
>
Yes, it needs as_ref(). A direct unwrap() consumes the Some() since
inter type of qemu_timer (Box<Timer>) is non-`Copy`.
Another example is
fn get_state(&self) -> &HPETState {
// SAFETY:
// the pointer is convertible to a reference
unsafe { self.state.unwrap().as_ref() }
}
Because the state (Option<NonNull<HPETState>>) has `Copy` so that this
case doesn't need as_ref before unwrap.
Thanks,
Zhao
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 09/10] rust/timer/hpet: add qom and qdev APIs support
2025-01-25 12:51 [PATCH 00/10] rust: Add HPET timer device Zhao Liu
` (7 preceding siblings ...)
2025-01-25 12:51 ` [PATCH 08/10] rust/timer/hpet: add basic HPET timer and HPETState Zhao Liu
@ 2025-01-25 12:51 ` Zhao Liu
2025-01-29 10:58 ` Paolo Bonzini
2025-01-25 12:51 ` [PATCH 10/10] i386: enable rust hpet for pc when rust is enabled Zhao Liu
9 siblings, 1 reply; 34+ messages in thread
From: Zhao Liu @ 2025-01-25 12:51 UTC (permalink / raw)
To: Paolo Bonzini, Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé
Cc: qemu-devel, qemu-rust, Zhao Liu
Implement QOM & QAPI support for HPET device.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC:
* Merge qdev.rs to hpet.rs.
* Apply memory and Resettable bindings.
* Consolidate inmutable &self and QOM casting.
* Prefer timer iterator over loop.
* Move init_mmio() and init_irq() to post_init().
---
rust/hw/timer/hpet/src/fw_cfg.rs | 2 -
rust/hw/timer/hpet/src/hpet.rs | 283 ++++++++++++++++++++++++++++++-
rust/hw/timer/hpet/src/lib.rs | 4 +
3 files changed, 279 insertions(+), 10 deletions(-)
diff --git a/rust/hw/timer/hpet/src/fw_cfg.rs b/rust/hw/timer/hpet/src/fw_cfg.rs
index 2f72bf854a66..5223629576a1 100644
--- a/rust/hw/timer/hpet/src/fw_cfg.rs
+++ b/rust/hw/timer/hpet/src/fw_cfg.rs
@@ -2,8 +2,6 @@
// Author(s): Zhao Liu <zhai1.liu@intel.com>
// SPDX-License-Identifier: GPL-2.0-or-later
-#![allow(dead_code)]
-
use qemu_api::{cell::bql_locked, impl_zeroable, zeroable::Zeroable};
// Each HPETState represents a Event Timer Block. The v1 spec supports
diff --git a/rust/hw/timer/hpet/src/hpet.rs b/rust/hw/timer/hpet/src/hpet.rs
index 0884a2ac73c4..7a717dbcfdd0 100644
--- a/rust/hw/timer/hpet/src/hpet.rs
+++ b/rust/hw/timer/hpet/src/hpet.rs
@@ -2,21 +2,30 @@
// Author(s): Zhao Liu <zhai1.liu@intel.com>
// SPDX-License-Identifier: GPL-2.0-or-later
-#![allow(dead_code)]
-
-use core::ptr::{null_mut, NonNull};
+use core::ptr::{addr_of_mut, null_mut, NonNull};
+use std::{ffi::CStr, slice::from_ref};
use qemu_api::{
- bindings::{address_space_memory, address_space_stl_le, MemoryRegion, SCALE_NS},
+ bindings::{
+ address_space_memory, address_space_stl_le, qdev_prop_bit, qdev_prop_bool,
+ qdev_prop_uint32, qdev_prop_uint8, SCALE_NS,
+ },
+ c_str,
cell::{BqlCell, BqlRefCell},
irq::InterruptSource,
- memory::MEMTXATTRS_UNSPECIFIED,
+ memory::{
+ hwaddr, MemoryRegion, MemoryRegionOps, MemoryRegionOpsBuilder, MEMTXATTRS_UNSPECIFIED,
+ },
prelude::*,
- qom::ParentField,
- sysbus::SysBusDevice,
+ qdev::{DeviceImpl, DeviceMethods, DeviceState, Property, ResetType, ResettablePhasesImpl},
+ qom::{ClassInitImpl, ObjectImpl, ObjectType, ParentField},
+ qom_isa,
+ sysbus::{SysBusDevice, SysBusDeviceClass},
timer::{QEMUTimer, CLOCK_VIRTUAL},
};
+use crate::fw_cfg::HPETFwConfig;
+
// Register space for each timer block. (HPET_BASE isn't defined here.)
const HPET_REG_SPACE_LEN: u64 = 0x400; // 1024 bytes
@@ -451,13 +460,43 @@ fn callback(&mut self) {
}
self.update_irq(true);
}
+
+ fn read(&mut self, addr: hwaddr, _size: u32) -> u64 {
+ let shift: u64 = (addr & 4) * 8;
+
+ match addr {
+ HPET_TN_CFG_REG => self.config >> shift, // including interrupt capabilities
+ HPET_TN_CMP_REG => self.cmp >> shift, // comparator register
+ HPET_TN_FSB_ROUTE_REG => self.fsb >> shift,
+ _ => {
+ // TODO: Add trace point - trace_hpet_ram_read_invalid()
+ // Reserved.
+ 0
+ }
+ }
+ }
+
+ fn write(&mut self, addr: hwaddr, value: u64, size: u32) {
+ let shift = ((addr & 4) * 8) as u32;
+ let len = std::cmp::min(size * 8, 64 - shift);
+
+ match addr {
+ HPET_TN_CFG_REG => self.set_tn_cfg_reg(shift, len, value),
+ HPET_TN_CMP_REG => self.set_tn_cmp_reg(shift, len, value),
+ HPET_TN_FSB_ROUTE_REG => self.set_tn_fsb_route_reg(shift, len, value),
+ _ => {
+ // TODO: Add trace point - trace_hpet_ram_write_invalid()
+ // Reserved.
+ }
+ }
+ }
}
/// HPET Event Timer Block Abstraction
/// Note: Wrap all items that may be changed in the callback called by C
/// into the BqlCell/BqlRefCell.
#[repr(C)]
-#[derive(qemu_api_macros::offsets)]
+#[derive(qemu_api_macros::Object, qemu_api_macros::offsets)]
pub struct HPETState {
parent_obj: ParentField<SysBusDevice>,
iomem: MemoryRegion,
@@ -630,4 +669,232 @@ fn set_counter_reg(&self, shift: u32, len: u32, val: u64) {
self.counter
.set(self.counter.get().deposit(shift, len, val));
}
+
+ unsafe fn init(&mut self) {
+ static HPET_RAM_OPS: MemoryRegionOps<HPETState> =
+ MemoryRegionOpsBuilder::<HPETState>::new()
+ .read(&HPETState::read)
+ .write(&HPETState::write)
+ .native_endian()
+ .valid_sizes(4, 8)
+ .impl_sizes(4, 8)
+ .build();
+
+ // SAFETY:
+ // self and self.iomem are guaranteed to be valid at this point since callers
+ // must make sure the `self` reference is valid.
+ MemoryRegion::init_io(
+ unsafe { &mut *addr_of_mut!(self.iomem) },
+ addr_of_mut!(*self),
+ &HPET_RAM_OPS,
+ "hpet",
+ HPET_REG_SPACE_LEN,
+ );
+ }
+
+ fn post_init(&self) {
+ self.init_mmio(&self.iomem);
+ for irq in self.irqs.iter() {
+ self.init_irq(irq);
+ }
+ }
+
+ fn realize(&self) {
+ if self.int_route_cap == 0 {
+ // TODO: Add error binding: warn_report()
+ println!("Hpet's hpet-intcap property not initialized");
+ }
+
+ self.hpet_id.set(HPETFwConfig::assign_hpet_id());
+
+ if self.num_timers.get() < HPET_MIN_TIMERS {
+ self.num_timers.set(HPET_MIN_TIMERS);
+ } else if self.num_timers.get() > HPET_MAX_TIMERS {
+ self.num_timers.set(HPET_MAX_TIMERS);
+ }
+
+ self.init_timer();
+ // 64-bit General Capabilities and ID Register; LegacyReplacementRoute.
+ self.capability.set(
+ HPET_CAP_REV_ID_VALUE << HPET_CAP_REV_ID_SHIFT |
+ 1 << HPET_CAP_COUNT_SIZE_CAP_SHIFT |
+ 1 << HPET_CAP_LEG_RT_CAP_SHIFT |
+ HPET_CAP_VENDER_ID_VALUE << HPET_CAP_VENDER_ID_SHIFT |
+ ((self.num_timers.get() - 1) as u64) << HPET_CAP_NUM_TIM_SHIFT | // indicate the last timer
+ (HPET_CLK_PERIOD * FS_PER_NS) << HPET_CAP_CNT_CLK_PERIOD_SHIFT, // 10 ns
+ );
+
+ self.init_gpio_in(2, HPETState::handle_legacy_irq);
+ self.init_gpio_out(from_ref(&self.pit_enabled));
+ }
+
+ fn reset_hold(&self, _type: ResetType) {
+ let sbd = self.upcast::<SysBusDevice>();
+
+ for timer in self.timers.iter().take(self.num_timers.get()) {
+ timer.borrow_mut().reset();
+ }
+
+ self.counter.set(0);
+ self.config.set(0);
+ self.pit_enabled.set(true);
+ self.hpet_offset.set(0);
+
+ HPETFwConfig::update_hpet_cfg(
+ self.hpet_id.get(),
+ Some(self.capability.get() as u32),
+ Some((*sbd).mmio[0].addr),
+ );
+
+ // to document that the RTC lowers its output on reset as well
+ self.rtc_irq_level.set(0);
+ }
+
+ fn timer_and_addr(&self, addr: hwaddr) -> Option<(&BqlRefCell<HPETTimer>, hwaddr)> {
+ let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
+
+ // TODO: Add trace point - trace_hpet_ram_[read|write]_timer_id(timer_id)
+ if timer_id > self.num_timers.get() {
+ // TODO: Add trace point - trace_hpet_timer_id_out_of_range(timer_id)
+ None
+ } else {
+ Some((&self.timers[timer_id], addr & 0x18))
+ }
+ }
+
+ fn read(&self, addr: hwaddr, size: u32) -> u64 {
+ let shift: u64 = (addr & 4) * 8;
+
+ // address range of all TN regs
+ // TODO: Add trace point - trace_hpet_ram_read(addr)
+ if (0x100..=0x3ff).contains(&addr) {
+ match self.timer_and_addr(addr) {
+ None => 0, // Reserved,
+ Some((timer, addr)) => timer.borrow_mut().read(addr, size),
+ }
+ } else {
+ match addr & !4 {
+ HPET_CAP_REG => self.capability.get() >> shift, /* including HPET_PERIOD 0x004 */
+ // (CNT_CLK_PERIOD field)
+ HPET_CFG_REG => self.config.get() >> shift,
+ HPET_COUNTER_REG => {
+ let cur_tick: u64 = if self.is_hpet_enabled() {
+ self.get_ticks()
+ } else {
+ self.counter.get()
+ };
+
+ // TODO: Add trace point - trace_hpet_ram_read_reading_counter(addr & 4,
+ // cur_tick)
+ cur_tick >> shift
+ }
+ HPET_INT_STATUS_REG => self.int_status.get() >> shift,
+ _ => {
+ // TODO: Add trace point- trace_hpet_ram_read_invalid()
+ // Reserved.
+ 0
+ }
+ }
+ }
+ }
+
+ fn write(&self, addr: hwaddr, value: u64, size: u32) {
+ let shift = ((addr & 4) * 8) as u32;
+ let len = std::cmp::min(size * 8, 64 - shift);
+
+ // TODO: Add trace point - trace_hpet_ram_write(addr, value)
+ if (0x100..=0x3ff).contains(&addr) {
+ match self.timer_and_addr(addr) {
+ None => return, // Reserved.
+ Some((timer, addr)) => timer.borrow_mut().write(addr, value, size),
+ }
+ } else {
+ match addr & !0x4 {
+ HPET_CAP_REG => {} // General Capabilities and ID Register: Read Only
+ HPET_CFG_REG => self.set_cfg_reg(shift, len, value),
+ HPET_INT_STATUS_REG => self.set_int_status_reg(shift, len, value),
+ HPET_COUNTER_REG => self.set_counter_reg(shift, len, value),
+ _ => {
+ // TODO: Add trace point - trace_hpet_ram_write_invalid()
+ // Reserved.
+ }
+ }
+ }
+ }
+}
+
+qom_isa!(HPETState: SysBusDevice, DeviceState, Object);
+
+// TODO: add OBJECT_DECLARE_SIMPLE_TYPE.
+#[repr(C)]
+pub struct HPETClass {
+ parent_class: <SysBusDevice as ObjectType>::Class,
+}
+
+unsafe impl ObjectType for HPETState {
+ type Class = HPETClass;
+ const TYPE_NAME: &'static CStr = crate::TYPE_HPET;
+}
+
+impl ClassInitImpl<HPETClass> for HPETState {
+ fn class_init(klass: &mut HPETClass) {
+ <Self as ClassInitImpl<SysBusDeviceClass>>::class_init(&mut klass.parent_class);
+ }
+}
+
+impl ObjectImpl for HPETState {
+ type ParentType = SysBusDevice;
+
+ const INSTANCE_INIT: Option<unsafe fn(&mut Self)> = Some(Self::init);
+ const INSTANCE_POST_INIT: Option<fn(&Self)> = Some(Self::post_init);
+}
+
+// TODO: Make these properties user-configurable!
+qemu_api::declare_properties! {
+ HPET_PROPERTIES,
+ qemu_api::define_property!(
+ c_str!("timers"),
+ HPETState,
+ num_timers,
+ unsafe { &qdev_prop_uint8 },
+ u8,
+ default = HPET_MIN_TIMERS
+ ),
+ qemu_api::define_property!(
+ c_str!("msi"),
+ HPETState,
+ flags,
+ unsafe { &qdev_prop_bit },
+ u32,
+ bit = HPET_FLAG_MSI_SUPPORT_SHIFT as u8,
+ default = false,
+ ),
+ qemu_api::define_property!(
+ c_str!("hpet-intcap"),
+ HPETState,
+ int_route_cap,
+ unsafe { &qdev_prop_uint32 },
+ u32,
+ default = 0
+ ),
+ qemu_api::define_property!(
+ c_str!("hpet-offset-saved"),
+ HPETState,
+ hpet_offset_saved,
+ unsafe { &qdev_prop_bool },
+ bool,
+ default = true
+ ),
+}
+
+impl DeviceImpl for HPETState {
+ fn properties() -> &'static [Property] {
+ &HPET_PROPERTIES
+ }
+
+ const REALIZE: Option<fn(&Self)> = Some(Self::realize);
+}
+
+impl ResettablePhasesImpl for HPETState {
+ const HOLD: Option<fn(&Self, ResetType)> = Some(Self::reset_hold);
}
diff --git a/rust/hw/timer/hpet/src/lib.rs b/rust/hw/timer/hpet/src/lib.rs
index 027f7f83349a..25251112a86d 100644
--- a/rust/hw/timer/hpet/src/lib.rs
+++ b/rust/hw/timer/hpet/src/lib.rs
@@ -10,5 +10,9 @@
#![deny(unsafe_op_in_unsafe_fn)]
+use qemu_api::c_str;
+
pub mod fw_cfg;
pub mod hpet;
+
+pub const TYPE_HPET: &::std::ffi::CStr = c_str!("hpet");
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH 09/10] rust/timer/hpet: add qom and qdev APIs support
2025-01-25 12:51 ` [PATCH 09/10] rust/timer/hpet: add qom and qdev APIs support Zhao Liu
@ 2025-01-29 10:58 ` Paolo Bonzini
2025-02-08 10:55 ` Zhao Liu
0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-01-29 10:58 UTC (permalink / raw)
To: Zhao Liu
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé, qemu-devel, qemu-rust, Zhao Liu
On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> fn read(&mut self, addr: hwaddr, _size: u32) -> u64 {
This can be &self.
> let shift: u64 = (addr & 4) * 8;
>
> + match addr {
> + HPET_TN_CFG_REG => self.config >> shift, // including interrupt capabilities
This needs to be "match addr & !4".
> + HPET_TN_CMP_REG => self.cmp >> shift, // comparator register
> + HPET_TN_FSB_ROUTE_REG => self.fsb >> shift,
> + _ => {
> + // TODO: Add trace point - trace_hpet_ram_read_invalid()
> + // Reserved.
> + 0
> + }
> + }
> + }
> +
> + fn write(&mut self, addr: hwaddr, value: u64, size: u32) {
> + let shift = ((addr & 4) * 8) as u32;
> + let len = std::cmp::min(size * 8, 64 - shift);
> +
> + match addr {
> + HPET_TN_CFG_REG => self.set_tn_cfg_reg(shift, len, value),
Likewise.
> + fn write(&self, addr: hwaddr, value: u64, size: u32) {
> + let shift = ((addr & 4) * 8) as u32;
> + let len = std::cmp::min(size * 8, 64 - shift);
> +
> + // TODO: Add trace point - trace_hpet_ram_write(addr, value)
> + if (0x100..=0x3ff).contains(&addr
) {
> + match self.timer_and_addr(addr) {
> + None => return, // Reserved.
Clippy complains about an unnecessary return, just replace it with "()".
> + Some((timer, addr)) => timer.borrow_mut().write(addr, value, size),
> + }
> +
> + fn reset_hold(&self, _type: ResetType) {
> + let sbd = self.upcast::<SysBusDevice>();
> +
> + for timer in self.timers.iter().take(self.num_timers.get()) {
> + timer.borrow_mut().reset();
> + }
> +
> + self.counter.set(0);
> + self.config.set(0);
> + self.pit_enabled.set(true);
> + self.hpet_offset.set(0);
> +
> + HPETFwConfig::update_hpet_cfg(
> + self.hpet_id.get(),
> + Some(self.capability.get() as u32),
> + Some((*sbd).mmio[0].addr),
> + );
This can be simply sbd.mmio[0].addr, without the (*...).
Also, you can change update_hpet_cfg to take arguments without the
Option<> around them.
> +// TODO: add OBJECT_DECLARE_SIMPLE_TYPE.
> +#[repr(C)]
> +pub struct HPETClass {
> + parent_class: <SysBusDevice as ObjectType>::Class,
> +}
> +
> +unsafe impl ObjectType for HPETState {
> + type Class = HPETClass;
> + const TYPE_NAME: &'static CStr = crate::TYPE_HPET;
> +}
No need for HPETClass (and for ClassInitImpl<HPETClass>), just do
unsafe impl ObjectType for HPETState {
type Class = <SysBusDevice as ObjectType>::Class;
const TYPE_NAME: &'static CStr = crate::TYPE_HPET;
}
which is indeed more similar to OBJECT_DECLARE_SIMPLE_TYPE().
Paolo
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 09/10] rust/timer/hpet: add qom and qdev APIs support
2025-01-29 10:58 ` Paolo Bonzini
@ 2025-02-08 10:55 ` Zhao Liu
2025-02-08 11:41 ` Paolo Bonzini
0 siblings, 1 reply; 34+ messages in thread
From: Zhao Liu @ 2025-02-08 10:55 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé, qemu-devel, qemu-rust
On Wed, Jan 29, 2025 at 11:58:14AM +0100, Paolo Bonzini wrote:
> Date: Wed, 29 Jan 2025 11:58:14 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 09/10] rust/timer/hpet: add qom and qdev APIs support
>
>
>
> On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> > fn read(&mut self, addr: hwaddr, _size: u32) -> u64 {
>
> This can be &self.
Done.
> > let shift: u64 = (addr & 4) * 8;
> >
> > + match addr {
> > + HPET_TN_CFG_REG => self.config >> shift, // including interrupt capabilities
>
> This needs to be "match addr & !4".
I understand it's not necessary:
In timer_and_addr(), I've masked the address with 0x18.
fn timer_and_addr(&self, addr: hwaddr) -> Option<(&BqlRefCell<HPETTimer>, hwaddr)> {
let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
if timer_id > self.num_timers.get() {
None
} else {
Some((&self.timers[timer_id], addr & 0x18))
}
}
And in HPETState::read(), I pass the masked address to
HPETTimer::read():
fn read(&self, addr: hwaddr, size: u32) -> u64 {
let shift: u64 = (addr & 4) * 8;
if (0x100..=0x3ff).contains(&addr) {
match self.timer_and_addr(addr) {
None => 0, // Reserved,
Some((timer, addr)) => timer.borrow_mut().read(addr, size),
}
}
Ah, the `addr` variable in Some() is misleading, and I should use
anothor name like `tn_addr`.
0x18 is a subset of !0x4, so the bitwise & operation with !0x4 can be
omitted (I understand that this is why you also omitted this in the C
side in the commit c2366567378dd :-) ).
But `match addr & !4` is also reasonable, which explicitly emphasize
`& !4` makes the code clearer. I would do this.
> > + HPET_TN_CMP_REG => self.cmp >> shift, // comparator register
> > + HPET_TN_FSB_ROUTE_REG => self.fsb >> shift,
> > + _ => {
> > + // TODO: Add trace point - trace_hpet_ram_read_invalid()
> > + // Reserved.
> > + 0
> > + }
> > + }
> > + }
> > +
> > + fn write(&mut self, addr: hwaddr, value: u64, size: u32) {
> > + let shift = ((addr & 4) * 8) as u32;
> > + let len = std::cmp::min(size * 8, 64 - shift);
> > +
> > + match addr {
> > + HPET_TN_CFG_REG => self.set_tn_cfg_reg(shift, len, value),
>
> Likewise.
Done. Thanks!
> > + fn write(&self, addr: hwaddr, value: u64, size: u32) {
> > + let shift = ((addr & 4) * 8) as u32;
> > + let len = std::cmp::min(size * 8, 64 - shift);
> > +
> > + // TODO: Add trace point - trace_hpet_ram_write(addr, value)
> > + if (0x100..=0x3ff).contains(&addr
> ) {
> > + match self.timer_and_addr(addr) {
> > + None => return, // Reserved.
>
> Clippy complains about an unnecessary return, just replace it with "()".
Fixed.
> > + Some((timer, addr)) => timer.borrow_mut().write(addr, value, size),
> > + }
>
> > +
> > + fn reset_hold(&self, _type: ResetType) {
> > + let sbd = self.upcast::<SysBusDevice>();
> > +
> > + for timer in self.timers.iter().take(self.num_timers.get()) {
> > + timer.borrow_mut().reset();
> > + }
> > +
> > + self.counter.set(0);
> > + self.config.set(0);
> > + self.pit_enabled.set(true);
> > + self.hpet_offset.set(0);
> > +
> > + HPETFwConfig::update_hpet_cfg(
> > + self.hpet_id.get(),
> > + Some(self.capability.get() as u32),
> > + Some((*sbd).mmio[0].addr),
> > + );
>
> This can be simply sbd.mmio[0].addr, without the (*...).
>
> Also, you can change update_hpet_cfg to take arguments without the Option<>
> around them.
Good idea! Done.
> > +// TODO: add OBJECT_DECLARE_SIMPLE_TYPE.
> > +#[repr(C)]
> > +pub struct HPETClass {
> > + parent_class: <SysBusDevice as ObjectType>::Class,
> > +}
> > +
> > +unsafe impl ObjectType for HPETState {
> > + type Class = HPETClass;
> > + const TYPE_NAME: &'static CStr = crate::TYPE_HPET;
> > +}
>
> No need for HPETClass (and for ClassInitImpl<HPETClass>), just do
>
> unsafe impl ObjectType for HPETState {
Pls let me add a comment here:
// No need for HPETClass. Just like OBJECT_DECLARE_SIMPLE_TYPE in C.
Then this can be "grep", as a reference.
> type Class = <SysBusDevice as ObjectType>::Class;
> const TYPE_NAME: &'static CStr = crate::TYPE_HPET;
> }
>
> which is indeed more similar to OBJECT_DECLARE_SIMPLE_TYPE().
Awesome! Thanks.
Regards,
Zhao
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 09/10] rust/timer/hpet: add qom and qdev APIs support
2025-02-08 10:55 ` Zhao Liu
@ 2025-02-08 11:41 ` Paolo Bonzini
2025-02-08 18:06 ` Zhao Liu
0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2025-02-08 11:41 UTC (permalink / raw)
To: Zhao Liu
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé, qemu-devel, qemu-rust
[-- Attachment #1: Type: text/plain, Size: 1743 bytes --]
Il sab 8 feb 2025, 11:36 Zhao Liu <zhao1.liu@intel.com> ha scritto:
> On Wed, Jan 29, 2025 at 11:58:14AM +0100, Paolo Bonzini wrote:
> > Date: Wed, 29 Jan 2025 11:58:14 +0100
> > From: Paolo Bonzini <pbonzini@redhat.com>
> > Subject: Re: [PATCH 09/10] rust/timer/hpet: add qom and qdev APIs support
> >
> >
> >
> > On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> > > fn read(&mut self, addr: hwaddr, _size: u32) -> u64 {
> >
> > This can be &self.
>
> Done.
>
> > > let shift: u64 = (addr & 4) * 8;
> > >
> > > + match addr {
> > > + HPET_TN_CFG_REG => self.config >> shift, // including
> interrupt capabilities
> >
> > This needs to be "match addr & !4".
>
> I understand it's not necessary:
>
> In timer_and_addr(), I've masked the address with 0x18.
>
> fn timer_and_addr(&self, addr: hwaddr) ->
> Option<(&BqlRefCell<HPETTimer>, hwaddr)> {
> let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
>
> if timer_id > self.num_timers.get() {
> None
> } else {
> Some((&self.timers[timer_id], addr & 0x18))
>
Ah, this should be 0x1C (or perhaps 0x1F). Otherwise there is a bug in
accessing the high 32 bits of a 64-bit register; shift will always be 0 in
HPETTimer::read and write.
// No need for HPETClass. Just like OBJECT_DECLARE_SIMPLE_TYPE in C.
>
> Then this can be "grep", as a reference.
>
Sure.
Thanks,
Paolo
> > type Class = <SysBusDevice as ObjectType>::Class;
> > const TYPE_NAME: &'static CStr = crate::TYPE_HPET;
> > }
> >
> > which is indeed more similar to OBJECT_DECLARE_SIMPLE_TYPE().
>
> Awesome! Thanks.
>
> Regards,
> Zhao
>
>
[-- Attachment #2: Type: text/html, Size: 3191 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH 09/10] rust/timer/hpet: add qom and qdev APIs support
2025-02-08 11:41 ` Paolo Bonzini
@ 2025-02-08 18:06 ` Zhao Liu
0 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-02-08 18:06 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé, qemu-devel, qemu-rust
> > > This needs to be "match addr & !4".
> >
> > I understand it's not necessary:
> >
> > In timer_and_addr(), I've masked the address with 0x18.
> >
> > fn timer_and_addr(&self, addr: hwaddr) ->
> > Option<(&BqlRefCell<HPETTimer>, hwaddr)> {
> > let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
> >
> > if timer_id > self.num_timers.get() {
> > None
> > } else {
> > Some((&self.timers[timer_id], addr & 0x18))
> >
>
> Ah, this should be 0x1C (or perhaps 0x1F). Otherwise there is a bug in
> accessing the high 32 bits of a 64-bit register; shift will always be 0 in
> HPETTimer::read and write.
Yes, you're right. Then we should use 0x1F so that invalid access could
detected (or loged in future) and ignored.
Based on the similar reason, C side also need to use "addr & (0x1f | ~4)"
instead of 0x18 to catch invalid access. If I'm right, I can submit a
fix.
Thanks,
Zhao
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 10/10] i386: enable rust hpet for pc when rust is enabled
2025-01-25 12:51 [PATCH 00/10] rust: Add HPET timer device Zhao Liu
` (8 preceding siblings ...)
2025-01-25 12:51 ` [PATCH 09/10] rust/timer/hpet: add qom and qdev APIs support Zhao Liu
@ 2025-01-25 12:51 ` Zhao Liu
9 siblings, 0 replies; 34+ messages in thread
From: Zhao Liu @ 2025-01-25 12:51 UTC (permalink / raw)
To: Paolo Bonzini, Manos Pitsidianakis, Junjie Mao, Alex Bennée,
Philippe Mathieu-Daudé, Richard Henderson, Peter Maydell,
Daniel P . Berrangé
Cc: qemu-devel, qemu-rust, Zhao Liu
Add HPET configuration in PC's Kconfig options, and select HPET device
(Rust version) if Rust is supported.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC:
* Fix rust hpet not being optional.
* Fix bios-tables-test missing rust hpet.
---
configs/devices/i386-softmmu/default.mak | 1 +
hw/i386/fw_cfg.c | 2 +-
hw/i386/pc.c | 2 +-
hw/timer/Kconfig | 6 +++++-
rust/hw/Kconfig | 1 +
rust/hw/timer/Kconfig | 2 ++
tests/qtest/meson.build | 3 ++-
7 files changed, 13 insertions(+), 4 deletions(-)
create mode 100644 rust/hw/timer/Kconfig
diff --git a/configs/devices/i386-softmmu/default.mak b/configs/devices/i386-softmmu/default.mak
index 4faf2f0315e2..9ef343cace06 100644
--- a/configs/devices/i386-softmmu/default.mak
+++ b/configs/devices/i386-softmmu/default.mak
@@ -6,6 +6,7 @@
#CONFIG_APPLESMC=n
#CONFIG_FDC=n
#CONFIG_HPET=n
+#CONFIG_X_HPET_RUST=n
#CONFIG_HYPERV=n
#CONFIG_ISA_DEBUG=n
#CONFIG_ISA_IPMI_BT=n
diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
index 546de63123e6..a7d69a49edbf 100644
--- a/hw/i386/fw_cfg.c
+++ b/hw/i386/fw_cfg.c
@@ -147,7 +147,7 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
#endif
fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, 1);
-#ifdef CONFIG_HPET
+#if defined(CONFIG_HPET) || defined(CONFIG_X_HPET_RUST)
PCMachineState *pcms =
(PCMachineState *)object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE);
if (pcms && pcms->hpet_enabled) {
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b46975c8a4db..04554817e021 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1701,7 +1701,7 @@ static void pc_machine_initfn(Object *obj)
pcms->sata_enabled = true;
pcms->i8042_enabled = true;
pcms->max_fw_size = 8 * MiB;
-#ifdef CONFIG_HPET
+#if defined(CONFIG_HPET) || defined(CONFIG_X_HPET_RUST)
pcms->hpet_enabled = true;
#endif
pcms->fd_bootchk = true;
diff --git a/hw/timer/Kconfig b/hw/timer/Kconfig
index c96fd5d97ae8..c051597180f4 100644
--- a/hw/timer/Kconfig
+++ b/hw/timer/Kconfig
@@ -11,7 +11,11 @@ config A9_GTIMER
config HPET
bool
- default y if PC
+ default y if PC && !HAVE_RUST
+
+config X_HPET_RUST
+ bool
+ default y if PC && HAVE_RUST
config I8254
bool
diff --git a/rust/hw/Kconfig b/rust/hw/Kconfig
index 4d934f30afe1..36f92ec02874 100644
--- a/rust/hw/Kconfig
+++ b/rust/hw/Kconfig
@@ -1,2 +1,3 @@
# devices Kconfig
source char/Kconfig
+source timer/Kconfig
diff --git a/rust/hw/timer/Kconfig b/rust/hw/timer/Kconfig
new file mode 100644
index 000000000000..afd980335037
--- /dev/null
+++ b/rust/hw/timer/Kconfig
@@ -0,0 +1,2 @@
+config X_HPET_RUST
+ bool
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 94b28e5a5347..0268c657cc8f 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -102,7 +102,8 @@ qtests_i386 = \
config_all_devices.has_key('CONFIG_VIRTIO_PCI') and \
slirp.found() ? ['virtio-net-failover'] : []) + \
(unpack_edk2_blobs and \
- config_all_devices.has_key('CONFIG_HPET') and \
+ (config_all_devices.has_key('CONFIG_HPET') or \
+ config_all_devices.has_key('CONFIG_X_HPET_RUST')) and \
config_all_devices.has_key('CONFIG_PARALLEL') ? ['bios-tables-test'] : []) + \
qtests_pci + \
qtests_cxl + \
--
2.34.1
^ permalink raw reply related [flat|nested] 34+ messages in thread