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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 22/11/24 11:53, Paolo Bonzini wrote:
> 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. :)

If there are no more vector uses, personally I'd convert qemu_set_irq()
to use an explicit boolean level. If vector need arises then I'd
add it using a new explicit method, i.e. qemu_set_irq_vector(); so
the arguments are obvious when we review qemu_set_irq*() uses.

Otherwise I'll defer to Peter who raised that point first.

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