> > * BqlCell/BqlRefCell access.
> >
> > Except InterruptSource, HPETState has other BqlCell and BqlRefCell:
> > hpet_offset (BqlCell<u64>), rtc_irq_level (BqlCell<u32>) and timers
> > ([BqlRefCell<HPETTimer>; HPET_MAX_TIMERS]).
> >
> > Their data may change during runtime, so the atomic context is
> > required.
>
> I have already mentioned HPETTimer in the other email, but I would also move
> hpet_offset to HPETRegisters if possible. It doesn't seem hard.
Yeah, it can.
> And as an aside, I wonder if you really need to pass MutexGuard and not &mut
> HPETRegisters. Once you don't have BQL dependencies, you can just remove
> the assert!(bql::is_locked()) without switching to MutexGuard<>.
The main reason for using MutexGuard at present is to explicitly
indicate that it is protected by a Mutex. Because I considered that
get_mut() in the timer handler could bypass the lock(). But get_mut
depends on the unsafe code `unsafe { t.state.as_mut() }` which always
needs careful check and review.
So yes, we can use &mut HPETRegisters directly.
> In the meanwhile, even if they are not perfect (especially due to
> migration), I think touching patches 1-19 further is too messy, so I'll
> rebase on top of Stefan's tracing patches and push them to rust-next. Let's
> start from there and I'll take a look tomorrow maybe on how to fix
> migration. Migratable<HPETTimer> looks like a powerful tool for that.
Thank you!
> Then the new problem is that we have to figure out a way to handle IRQs.
> They are also messy for PL011 compared to the C version, and that will make
> it possible to enable lockless IO.
>
> The crazy idea that just came to mind, is a Latched<u32> that is something
> like an (AtomicU32, BqlCell<u32>) tuple. Then we set the individual bits
> outside the BQL and update IRQs at the end of the MMIO in a
> bql::with_guard() block.
This is an interesting idea and sounds like a "RCU" (write-copy-update)?
HMM, what does u32 mean, irq number? I understand the bql::with_guard()
is after Muext locking, i.e., after writing registers.
At that point, we need to know which irq should be operated (this is the
u32 but we also have pit_enabled), and what operation should we do now.
I'm not sure whether a tuple is enough... because there may be multiple
IRQ operations during Mutex locking:
fn set_cfg_reg(&self, regs: &mut MutexGuard<HPETRegisters>, shift: u32, len:
u32, val: u64) {
...
// 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) {
bql::with_guard(|| {
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) {
bql::with_guard(|| {
// 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);
});
}
}
So do we need a lockless queue to store IrqOps during Mutex locking?
pub enum HPETIrqOp {
Lower(usize), // usize is index in HPETState::irqs[]
Pulse(usize),
Raise(usize),
Set(usize, bool),
PitSet(bool), // HPETState::pit_enabled
}
Another point I'm considerring is: the IRQ ops is cached in MMIO Mutex,
while its execution occurs in the MMIO BQL. If a timer handler (which
acquires BQL and then Mutex) is present between MMIO Mutex and MMIO BQL,
and also performs an IRQ op, this seems possible a "reordering" issue
for IRQ ops. Is this ok?
I guess it's ok, since even hardware may also can't guarantee that
register operation and irq operation is atomic...
Then with your idea, this could fix deadlock I mentioned in patch 21 and
we don't need the fix to unlock bql in timer handler anymore...
BTW, but, shouldn't C HPET also lock the mutex in the timer handler?
> Maybe if you have some time you can prototype that
> for PL011 (even without generics, you could just do LatchedU32 for a start)?
I guess you mean HPET? PL011 is also Ok but it hasn't reached the
lockless stage yet.
Thanks,
Zhao
