Re: [PATCH 2/2] rust: add bindings for interrupt sources

[Paolo Bonzini]

On 11/22/24 11:30, Philippe Mathieu-Daudé wrote:
> On 22/11/24 09:32, Paolo Bonzini wrote:
> > > > +/// Interrupt sources are used by devices to pass changes to a 
> > > > boolean value to
> > > > +/// other devices (typically interrupt or GPIO controllers).  QEMU 
> > > > interrupt
> > > > +/// sources are always active-high.
> > >
> > > So 'always active-high' = true below? (Wondering about pulsation, if the
> > > true -> false transition is always correct).
> >
> > Yeah, I mean that raise uses true (or 1 :)) and lower uses false.
> > an example?
>
> I was thinking of an active-low line where you want to pulse 1 -> 0.
> Just chiming in, not to worry about.

This is not happening at the device level, so I assume that such a line 
would not use raise/lower.  Rather, the board (which is on the interrupt 
sink side) would install a qemu_irq_invert() between the device and the 
interrupt controller or GPIO controller.

> > > Is this deliberate to restrict the Rust binding to boolean? (Maybe you
> > > envision a VectoredInterruptSource implementation for that).
> >
> > No, I simply wasn't aware of that.  I'll adjust; do you have
> > an example?
>
> I am having hard time to find one, in particular because I
> removed one in c264c074d8 ("hw/intc: Remove TYPE_ETRAX_FS_PIC device"):

Ok, then we could put the type as a generic parameter, and use that 
parameter in InterruptSource::set().

pub struct InterruptSource<T = bool> where u32: From<T> {
    inner: BqlCell<*mut IrqState>,

    // this is only needed top ensure that T appears somehow in the
    // struct.  Random Rust type theory stuff. :)
    _marker: PhantomData<fn(&Self, T)>,
}

...

/// Send `level` to the interrupt sink.
pub fn set(&self, level: T) {
    let ptr = self.0.get();
    // SAFETY: the pointer is retrieved under the BQL and remains valid
    // until the BQL is released, which is after qemu_set_irq() is entered.
    unsafe {
        qemu_set_irq(ptr, level.into());
    }
}

and then only implement raise/lower/pulse for InterruptSource<bool>.

This is backwards compatible so we can do it either now, or later when 
needs arises.  You tell me. :)

Paolo

> See Peter's comment in https://lore.kernel.org/qemu-devel/ 
> CAFEAcA9cObnb11cSS_StbSHdP0aB6sDeqSHfjb3-qRBfy7K9Kw@mail.gmail.com/
>
> > > > +/// Interrupt sources can only be triggered under the Big QEMU 
> > > > Lock; they are
> > > > +/// neither `Send` nor `Sync`.
> >
> > Oops, this is incorrect.  BqlCell *is* Send/Sync, but checks the
> > BQL state at run-time.
> >
> > Paolo