Re: [PATCH 22/22] rust/hpet: Enable lockless IO

[Zhao Liu <zhao1.liu@intel.com>]

> >   * 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