On 2/18/25 10:07, Philippe Mathieu-Daudé wrote:
On 18/2/25 09:53, Paolo Bonzini wrote:
On 2/18/25 08:37, Zhao Liu wrote:
"addr & 0x18" ignores invalid address, so that the trace in default
branch (trace_hpet_ram_{read|write}_invalid()) doesn't work.
Mask addr by "0x1f & ~4", in which 0x1f means to get the complete TN
registers access and ~4 means to keep any invalid address offset.
I think this is less readable.
The reason to use !4 in the Rust code is because the initial AND is done
in a separate function, timer_and_addr().
Having a quick look at the model without looking at the specs:
include/hw/timer/hpet.h:20:#define HPET_LEN 0x400
hw/timer/hpet.c:439:static uint64_t hpet_ram_read(...,
hw/timer/hpet.c-441-{
hw/timer/hpet.c-448- /*address range of all TN regs*/
hw/timer/hpet.c-449- if (addr >= 0x100 && addr <= 0x3ff) {
hw/timer/hpet.c-450- uint8_t timer_id = (addr - 0x100) / 0x20;
...
hw/timer/hpet.c-469- } else {
hw/timer/hpet.c-470- switch (addr & ~4) {
...
hw/timer/hpet.c-488- }
hw/timer/hpet.c-489- }
hw/timer/hpet.c-490- return 0;
hw/timer/hpet.c-491-}
hw/timer/hpet.c:699: memory_region_init_io(&s->iomem, obj,
&hpet_ram_ops, s,
"hpet", HPET_LEN);
I suppose we want to register multiple timers of I/O size 0x20 at 0x100,
and the I/O size of 0x20 at 0x000 is a generic control region.
Maybe split hpet_ram_ops in 2 (hpet_cfg_ops and hpet_tmr_ops), mapping
the first one once at 0x000 and the other 24 times at 0x100-0x3ff?
You would have to come up with a way to get the index though. It seems
to be adding churn for no particular reason.
I'd rather look into how to make decoding code *easy* without making
everything MemoryRegions. As I explained yesterday, while I'm not yet
sure that Rust is going to stay in QEMU, I'd like to have as many
examples as possible to help tilting the balance one way or the other.
And indeed in the Rust version of HPET, timer_and_addr() could be
extended to something like this:
// Start with the same "enum for registers" pattern that PL011 uses:
#[derive(qemu_api_macros::TryInto)]
#[repr(u64)]
enum TimerRegister {
CFG = 0,
CMP = 8,
ROUTE = 16,
}
#[derive(qemu_api_macros::TryInto)]
#[repr(u64)]
enum GlobalRegister {
CAP = 0,
CFG = 0x10,
INT_STATUS = 0x20,
COUNTER = 0xF0,
}
// Go one step further and define types for all possible outcomes:
#[derive(Copy)]
enum HPETRegister {
Timer(&BqlRefCell<HPETTimer>, TimerRegister),
Global(GlobalRegister),
Unknown(hwaddr),
}
struct HPETAddrDecode {
u32 shift,
u32 len,
HPETRegister reg,
}
fn decode(&self, addr: hwaddr, size: u32) -> HPETAddrDecode {
let shift = ((addr & 4) * 8) as u32;
let len = std::cmp::min(size * 8, 64 - shift);
addr &= !4;
let reg = if (0x100..=0x3ff).contains(&addr) {
let timer_id: usize = ((addr - 0x100) / 0x20) as usize;
TimerRegister::try_from(addr)
.map(|reg| HPETRegister::Timer(&self.timers[timer_id], reg))
} else {
GlobalRegister::try_from(addr)
.map(HPETRegister::Global)
}
// reg is now a Result<HPETRegister, hwaddr>
// convert the Err case into HPETRegister as well
let reg = reg.unwrap_or_else(HPETRegister::Unknown);
HPETAddrDecode { shift, len, reg }
}
(untested). The read and write functions then can do something like
let val = match decoded.reg {
Timer(timer, reg) => timer.borrow_mut().read(decoded),
Global(GlobalRegister::CAP) => self.capability.get(),
Global(GlobalRegister::CFG) => self.config.get(),
...
}
val >> decoded.shift
and for write:
match decoded.reg {
Timer(timer, reg) => timer.borrow_mut().write(decoded, value),
Global(GlobalRegister::CAP) => {}, // read-only
Global(GlobalRegister::CFG) => self.set_cfg_reg(decoded, value),
...
}
The above could be a scheme that new devices could copy. Overall I
think it would be shorter code than what is there in rust/hw/timer/hpet
(which is IMO already better than C, mind!).
The honest question for people with less experience is whether this is
readable at all; whether seeing it helps you learn the language or
discourages you.
Paolo
No clue what is between 0x020-0x0ff.
My 2 cents looking at QDev modelling to avoid these address
manipulations.
Regards,
Phil.
