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

[Blue Swirl]

On Sat, Sep 3, 2011 at 9:41 PM, Anthony Liguori <address@hidden> wrote:
> On 09/03/2011 04:10 PM, Blue Swirl wrote:
>>
>> 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?
>
> The main difference is whether the Pin is capable of determine if there was
> a level change on its own.  It can only do this is if knows the current
> level which implies that its holding state.
>
>>
>> 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,
>
> I'm somewhat confused by what you mean here.

Pretty similar to what you propose below except the magical sharing.

> If you have two devices that have a connection, one has an output pin and
> one has an input pin.  This would look like this:
>
> struct Serial {
>   Pin irq; // output pin
> };
>
> struct PIIX3 {
>   Pin *in[16]; // input pins
> };
>
> As part of connecting devices, you'd basically do:
>
> PIIX3 piix3;
> Serial serial;
>
> piix3.in[4] = &serial.irq;
>
> serial.irq setting it pin level doesn't do anything to piix3.  piix3 has to
> explicitly read the pin state for its behavior to influence anything.
>
> Here's the flow with taking migration into account:
>
> 1) PIIX3 maintains some type of state, performs action (A) whenever in[3]
> changes its state based on an edge change notifier.
>
> 2) During migration, PIIX3 has its state saved as does Serial.  Pin is part
> of Serial so it also has its state saved.
>
> 3) During restore, PIIX3 has its state restored, as does Serial, and Pin.
>  Action (A) is not invoked because notifiers are not fired when a device is
> not realized.  Restore happens before a device is realized.
>
> So the scenario you're concerned about doesn't happen and it doesn't require
> anything funky.

OK, this should work.

>> this could work. Restoring Pins first and then devices is a sort of
>> priority scheme.
>
> There is no priority.  But devices have an explicit realize event and in
> general, shouldn't react to other devices until realize happens.  You need
> this behavior to support construction properly.
>
> Regards,
>
> Anthony Liguori
>