[PATCH] bsps/i386: Separate variable for i8259 IRQs disable due to in progress state.

[Pavel Pisa]

Hello Chris,

On Sunday 25 of September 2016 03:24:06 Chris Johns wrote:
> Hi Pavel,
>
> Thank you for this. What testing has the patch had?

Only QEMU, serial and E100 and VirtIO NIC.
So I agree that testing on the real HW is a must.
We have some test PCs with remote booting and monitoring
at university. But I am not sure if when I get to that.
But I am not sure which IRQ sources could be used there
to flood RTEMS. NIC are special Intel IEEE-1588 equipped
ones so probably unsupported by RTEMS.

So if you have hardware which has been used in past
to debug IRQs then it would be of more value.

> >    */
> > -static rtems_i8259_masks i8259a_cache = 0xFFFB;
> > +static rtems_i8259_masks i8259a_imr_cache = 0xFFFB;
> > +static rtems_i8259_masks i8259a_in_progress = 0;
>
> Should these be volatile to be sure?

The i8259a_cache has not been and because these and i8259
registers accesses from enable/disable are protected
by rtems_interrupt_disable() then they should be full
barrier even in SMP respect.

In the IRQ processing, there is only plain

  __asm__ __volatile__("sti");

which brings me to conclusion now that this is problematic
not only in the respect of these two variables but
all other state. So there should be added at least
compiler barrier even for UP build before sti
and after cli.

It could be more interesting to change variables
to atomic but i8259 registers accesses has to be
serialized and generally any interaction with i8259
is catastrophic from the performance point of view.
Using it in SMP case is allmost impossible so this
whole part should have better, local APIC based
alternative ideally combined with MSI interrupts
for modern systems.

So cli and sti is minimal performance problem there
and volatile alone is unusable.

> > +
> > @@ -365,11 +380,9 @@ void BSP_dispatch_isr(int vector)
> >          * again.
> >          */
> >         if (vector <= BSP_IRQ_MAX_ON_i8259A) {
> > -        if (irq_trigger[vector] == INTR_TRIGGER_LEVEL)
> > -          old_imr |= 1 << vector;
>
> Looking at the interrupt server I see a disable in the trigger call
> which means it should be ok to remove this code. I am wondering if this
> piece of code is an artefact of the comparison of the code in FreeBSD I
> did which always uses an interrupt server and may assume the interrupt
> is masked.

Yes, I have checked that before patch sending too and server
looks to be implemented portable and correctly.

Best wishes,

              Pavel