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

[Anthony Liguori]

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