Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Blue Swirl]

On Sat, Sep 3, 2011 at 8:07 PM, Anthony Liguori <address@hidden> wrote:
> On 09/01/2011 12:58 AM, Avi Kivity wrote:
>>
>> On 08/31/2011 07:59 PM, Blue Swirl wrote:
>>>
>>> >
>>> > That makes it impossible to migrate level-triggered irq lines. Or at
>>> least,
>>> > the receiver has to remember the state, instead of (or in addition
>>> to) the
>>> > sender.
>>>
>>> Both ends probably need to remember the state. That should work
>>> without any multiphase restores and transient suppressors.
>>
>> State should always correspond to real hardware state - a flip flop or
>> capacitor. Input is not state, it is input.
>>
>>> It might be also possible to introduce stateful signal lines which
>>> save and restore their state, then the receiving end could check what
>>> is the current level. However, if you consider that the devices may be
>>> restored in random order, if the IRQ line device happens to be
>>> restored later, the receiver would still get wrong information. Adding
>>> priorities could solve this, but I think stateless IRQs are the only
>>> sane way.
>>
>> I agree that irqs should be stateless, since they don't have any memory
>> associated.
>
> In Real Life, you can tie a single bit multiple registers together with
> boolean logic to form an output pin.  This is essentially computed state.
>  If we wanted to model a stateless pin, we would need to do something like:
>
> struct Serial {
>   uint8_t thr;
>   uint8_t lsr;
> };
>
> static bool serial_get_irq(Serial *s) {
>   return (s->thr & THRE) | (s->lsr & LSRE);
> }
>
> static void serial_write(Serial *s, uint64_t addr, uint8_t value)
> {
>   switch (addr) {
>   case THR:
>      bool old_irq = serial_get_irq(s);
>      s->thr = value;
>      if (!old_irq && serial_get_irq(s)) {
>          notify_edge_change(s);
>      }
>   ...
> }
>
> static void serial_init(Serial *s)
> {
>    register_pin(s, serial_get_irq);
> }
>
> Obviously, this is pretty sucky.  This is what we do today but we don't have
> a way to query irq value which is wrong.  You could fix that by adding the
> get function but that's not terribly fun.  A better way:
>
> struct Serial {
>    Pin irq;
>    uint8_t thr;
>    uint8_t lsr;
> };
>
> static void serial_update_irq(Serial *s)
> {
>   pin_set_level(&s->irq, (s->thr & THRE) | (s->lsr & LSRE));
> }
>
> static void serial_write(Serial *s) {
>   switch (addr) {
>   case THR:
>      s->thr = value;
>      serial_update_irq(s);
>   ...
> }
>
> This results in much nicer code.  The edge handling can be done in generic
> code which will make things more robust overall.

I'm sorry but I don't see a huge difference, could you elaborate?

Maybe if the internal state of Pin is magically shared between the
endpoint devices (like typedef bool *Pin) and the devices somehow
still save Pin state as part of their own despite the duplication,
this could work. Restoring Pins first and then devices is a sort of
priority scheme.