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

[Anthony Liguori <anthony@codemonkey.ws>]

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.

Regards,

Anthony Liguori