Re: [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

___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

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

[Chris Johns]

On 25/09/2016 11:28, Joel Sherrill wrote:

We have an old PC of mine at the office which is pretty reliable at
generating the spurious interrupt. I won't be there this week though.


Same. I have gear here but lacking time.



Is there still a print when one occurs?



No, there is a counter somewhere and I think a print out but I would 
need to check.


Even with the fixes I have made there is a possibility a spurious 
interrupt can occur and any print out will effect the timing of all 
interrupts which is something we cannot have in production. We need to 
handle it and recover as fast as possible.


Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

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

[Joel Sherrill]

We have an old PC of mine at the office which is pretty reliable at
generating the spurious interrupt. I won't be there this week though.

Is there still a print when one occurs?

On Sep 24, 2016 8:24 PM, "Chris Johns"  wrote:

> Hi Pavel,
>
> Thank you for this. What testing has the patch had?
>
> It needs to be tested on real hardware and hardware which showed the
> spurious interrupt issue. The issue appears around the interrupt enable
> call and my guess is it relates to some type of a race condition in the PIC
> between the signals and some piece of register timing. I timed the spurious
> interrupt always appearing usecs after the enable.
>
> I am currently unable to test this patch and I am not sure when I can. I
> will do my best next week.
>
> On 25/09/2016 10:48, Pavel Pisa wrote:
>
>> From: Pavel Pisa 
>>
>> The global state of enabled and disabled interrupts has to hold
>> interrupts really disabled by drivers and system. If the state is
>> combined with interrupts temporarily disabled because they are
>> processed at given time then it is impossible to maintain state
>> by interrupt handlers in drivers.
>> ---
>>   c/src/lib/libbsp/i386/shared/irq/irq.c | 51
>> +-
>>   1 file changed, 32 insertions(+), 19 deletions(-)
>>
>> diff --git a/c/src/lib/libbsp/i386/shared/irq/irq.c
>> b/c/src/lib/libbsp/i386/shared/irq/irq.c
>> index 0511257..8316fe6 100644
>> --- a/c/src/lib/libbsp/i386/shared/irq/irq.c
>> +++ b/c/src/lib/libbsp/i386/shared/irq/irq.c
>> @@ -51,7 +51,22 @@ static enum intr_trigger irq_trigger[BSP_IRQ_LINES_NUMB
>> ER];
>>* while upper bits are interrupt on the slave PIC.
>>* This cache is initialized in ldseg.s
>>*/
>> -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?
>
> +
>> +static inline
>> +void BSP_i8259a_irq_update_master_imr( void )
>> +{
>> +  rtems_i8259_masks mask = i8259a_in_progress | i8259a_imr_cache;
>> +  outport_byte( PIC_MASTER_IMR_IO_PORT, mask & 0xff );
>> +}
>> +
>> +static inline
>> +void BSP_i8259a_irq_update_slave_imr( void )
>> +{
>> +  rtems_i8259_masks mask = i8259a_in_progress | i8259a_imr_cache;
>> +  outport_byte( PIC_SLAVE_IMR_IO_PORT, ( mask >> 8 ) & 0xff );
>> +}
>>
>>   /*
>>* Print the stats.
>> @@ -107,7 +122,7 @@ static inline uint8_t 
>> BSP_i8259a_irq_in_service_reg(uint32_t
>> ioport)
>>   /*--
>> ---+
>>   | Function:  BSP_irq_disable_at_i8259a
>>   |  Description: Mask IRQ line in appropriate PIC chip.
>> -| Global Variables: i8259a_cache
>> +| Global Variables: i8259a_imr_cache, i8259a_in_progress
>>   |Arguments: vector_offset - number of IRQ line to mask.
>>   |  Returns: 0 is OK.
>>   +---
>> ---*/
>> @@ -119,15 +134,15 @@ static int BSP_irq_disable_at_i8259a(const
>> rtems_irq_number irqLine)
>> rtems_interrupt_disable(level);
>>
>> mask = 1 << irqLine;
>> -  i8259a_cache |= mask;
>> +  i8259a_imr_cache |= mask;
>>
>> if (irqLine < 8)
>> {
>> -outport_byte(PIC_MASTER_IMR_IO_PORT, i8259a_cache & 0xff);
>> +BSP_i8259a_irq_update_master_imr();
>> }
>> else
>> {
>> -outport_byte(PIC_SLAVE_IMR_IO_PORT, (i8259a_cache >> 8) & 0xff);
>> +BSP_i8259a_irq_update_slave_imr();
>> }
>>
>> rtems_interrupt_enable(level);
>> @@ -138,7 +153,7 @@ static int BSP_irq_disable_at_i8259a(const
>> rtems_irq_number irqLine)
>>   /*--
>> ---+
>>   | Function:  BSP_irq_enable_at_i8259a
>>   |  Description: Unmask IRQ line in appropriate PIC chip.
>> -| Global Variables: i8259a_cache
>> +| Global Variables: i8259a_imr_cache, i8259a_in_progress
>>   |Arguments: irqLine - number of IRQ line to mask.
>>   |  Returns: Nothing.
>>   +---
>> ---*/
>> @@ -152,19 +167,19 @@ static int BSP_irq_enable_at_i8259a(const
>> rtems_irq_number irqLine)
>> rtems_interrupt_disable(level);
>>
>> mask = 1 << irqLine;
>> -  i8259a_cache &= ~mask;
>> +  i8259a_imr_cache &= ~mask;
>>
>> if (irqLine < 8)
>> {
>>   isr = BSP_i8259a_irq_in_service_reg(PIC_MASTER_COMMAND_IO_PORT);
>>   irr = BSP_i8259a_irq_int_request_reg(PIC_MASTER_COMMAND_IO_PORT);
>> -outport_byte(PIC_MASTER_IMR_IO_PORT, i8259a_cache & 0xff);
>> +BSP_i8259a_irq_update_master_imr();
>> }
>> else
>> {
>>   isr = BSP_i8259a_irq_in_service_reg(PIC_SLAVE_COMMAND_IO_PORT);
>>   irr = BSP_i8259a_irq_int_request_reg(PIC_SLAVE_COMMAND_IO_PORT);
>> -outport_byte(PIC_SLAVE_IMR_IO_PORT, (i8259a_cache >> 8) & 0xff);
>> +BSP_i8259a_irq_update_slave_imr();
>> }
>>
>> 

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

[Chris Johns]

Hi Pavel,

Thank you for this. What testing has the patch had?

It needs to be tested on real hardware and hardware which showed the 
spurious interrupt issue. The issue appears around the interrupt enable 
call and my guess is it relates to some type of a race condition in the 
PIC between the signals and some piece of register timing. I timed the 
spurious interrupt always appearing usecs after the enable.


I am currently unable to test this patch and I am not sure when I can. I 
will do my best next week.


On 25/09/2016 10:48, Pavel Pisa wrote:

From: Pavel Pisa 

The global state of enabled and disabled interrupts has to hold
interrupts really disabled by drivers and system. If the state is
combined with interrupts temporarily disabled because they are
processed at given time then it is impossible to maintain state
by interrupt handlers in drivers.
---
  c/src/lib/libbsp/i386/shared/irq/irq.c | 51 +-
  1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/c/src/lib/libbsp/i386/shared/irq/irq.c 
b/c/src/lib/libbsp/i386/shared/irq/irq.c
index 0511257..8316fe6 100644
--- a/c/src/lib/libbsp/i386/shared/irq/irq.c
+++ b/c/src/lib/libbsp/i386/shared/irq/irq.c
@@ -51,7 +51,22 @@ static enum intr_trigger irq_trigger[BSP_IRQ_LINES_NUMBER];
   * while upper bits are interrupt on the slave PIC.
   * This cache is initialized in ldseg.s
   */
-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?


+
+static inline
+void BSP_i8259a_irq_update_master_imr( void )
+{
+  rtems_i8259_masks mask = i8259a_in_progress | i8259a_imr_cache;
+  outport_byte( PIC_MASTER_IMR_IO_PORT, mask & 0xff );
+}
+
+static inline
+void BSP_i8259a_irq_update_slave_imr( void )
+{
+  rtems_i8259_masks mask = i8259a_in_progress | i8259a_imr_cache;
+  outport_byte( PIC_SLAVE_IMR_IO_PORT, ( mask >> 8 ) & 0xff );
+}

  /*
   * Print the stats.
@@ -107,7 +122,7 @@ static inline uint8_t 
BSP_i8259a_irq_in_service_reg(uint32_t ioport)
  /*-+
  | Function:  BSP_irq_disable_at_i8259a
  |  Description: Mask IRQ line in appropriate PIC chip.
-| Global Variables: i8259a_cache
+| Global Variables: i8259a_imr_cache, i8259a_in_progress
  |Arguments: vector_offset - number of IRQ line to mask.
  |  Returns: 0 is OK.
  +--*/
@@ -119,15 +134,15 @@ static int BSP_irq_disable_at_i8259a(const 
rtems_irq_number irqLine)
rtems_interrupt_disable(level);

mask = 1 << irqLine;
-  i8259a_cache |= mask;
+  i8259a_imr_cache |= mask;

if (irqLine < 8)
{
-outport_byte(PIC_MASTER_IMR_IO_PORT, i8259a_cache & 0xff);
+BSP_i8259a_irq_update_master_imr();
}
else
{
-outport_byte(PIC_SLAVE_IMR_IO_PORT, (i8259a_cache >> 8) & 0xff);
+BSP_i8259a_irq_update_slave_imr();
}

rtems_interrupt_enable(level);
@@ -138,7 +153,7 @@ static int BSP_irq_disable_at_i8259a(const rtems_irq_number 
irqLine)
  /*-+
  | Function:  BSP_irq_enable_at_i8259a
  |  Description: Unmask IRQ line in appropriate PIC chip.
-| Global Variables: i8259a_cache
+| Global Variables: i8259a_imr_cache, i8259a_in_progress
  |Arguments: irqLine - number of IRQ line to mask.
  |  Returns: Nothing.
  +--*/
@@ -152,19 +167,19 @@ static int BSP_irq_enable_at_i8259a(const 
rtems_irq_number irqLine)
rtems_interrupt_disable(level);

mask = 1 << irqLine;
-  i8259a_cache &= ~mask;
+  i8259a_imr_cache &= ~mask;

if (irqLine < 8)
{
  isr = BSP_i8259a_irq_in_service_reg(PIC_MASTER_COMMAND_IO_PORT);
  irr = BSP_i8259a_irq_int_request_reg(PIC_MASTER_COMMAND_IO_PORT);
-outport_byte(PIC_MASTER_IMR_IO_PORT, i8259a_cache & 0xff);
+BSP_i8259a_irq_update_master_imr();
}
else
{
  isr = BSP_i8259a_irq_in_service_reg(PIC_SLAVE_COMMAND_IO_PORT);
  irr = BSP_i8259a_irq_int_request_reg(PIC_SLAVE_COMMAND_IO_PORT);
-outport_byte(PIC_SLAVE_IMR_IO_PORT, (i8259a_cache >> 8) & 0xff);
+BSP_i8259a_irq_update_slave_imr();
}

if (((isr ^ irr) & mask) != 0)
@@ -298,7 +313,7 @@ void BSP_dispatch_isr(int vector);

  void BSP_dispatch_isr(int vector)
  {
-  uint16_t old_imr = 0;
+  rtems_i8259_masks in_progress_save = 0;

if (vector < BSP_IRQ_VECTOR_NUMBER) {
  /*
@@ -329,10 +344,10 @@ void BSP_dispatch_isr(int vector)
 * vector clear.
 */
if (vector <= BSP_IRQ_MAX_ON_i8259A) {
-old_imr = i8259a_cache;
-i8259a_cache |= irq_mask_or_tbl[vector];
-outport_byte(PIC_MASTER_IMR_IO_PORT, i8259a_cache & 0xff);
-outport_byte(PIC_SLAVE_IMR_IO_PORT, 

Re: VirtIO based network driver works well after i8259 masks maintenance correction

[Pavel Pisa]

Hello Jin-Hyun Kim, Hyun-Wook Jin  and others,

I have returned back to VirtIO based network driver
after I have make PCI based E100 card work by woraround
which re-enables IRQ in driver worker daemon.

In meanwhile, I have included alternative networking setup
with E100 to RTEMS QEMU Wiki page

https://devel.rtems.org/wiki/Developer/Simulators/QEMU#Inteli82557be100PCICardEmulation

I have cleaned a VirtIO driver a little but including
IRQ workaround would be really complicated in this case.
Interrupt source signalling is cleaned by inport_byte
from corresponding VIRTIO_PCI_ISR. But if interrupt
is disabled on controller level by i8259 then reenable
would to be moved to rtems_bsdnet worker thread.
But that would violate all ideas about separation
of layers and encapsulation. Not by one, but over three
levels. So I have corrected (at least according my feeling)
i8259 IRQ processing.

The VirtIO networking seems to work great with this fix.
There is updated VirtIO support on branch "devel-virtio"
of my GitHub repository

  https://github.com/ppisa/rtems/tree/devel-virtio

I would like express big thanks to authors that they
have contributed port.

The last time there has not been reached final decision
if the code should be included in RTEMS or if there is
plug on old networking stack.

I think that it works well and can be used before
there is other alternative location for the code.

As for the code, I would be happy if it can be moved
out of i386 BSP directory or at least that part which
is independent if there is some i386 specific code.
But it uses generic IRQ now, generic PCI so it
should be usable with all other architectures.

There is even another reason why to included virtio
support. It can be used (if extended) even for providing
disk device to the virtualized environment and this
part probably stays in core RTEMS codebase even if
libbsd is used for networking.

Best wishes,

   Pavel

___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

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

[Pavel Pisa]

From: Pavel Pisa 

The global state of enabled and disabled interrupts has to hold
interrupts really disabled by drivers and system. If the state is
combined with interrupts temporarily disabled because they are
processed at given time then it is impossible to maintain state
by interrupt handlers in drivers.
---
 c/src/lib/libbsp/i386/shared/irq/irq.c | 51 +-
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/c/src/lib/libbsp/i386/shared/irq/irq.c 
b/c/src/lib/libbsp/i386/shared/irq/irq.c
index 0511257..8316fe6 100644
--- a/c/src/lib/libbsp/i386/shared/irq/irq.c
+++ b/c/src/lib/libbsp/i386/shared/irq/irq.c
@@ -51,7 +51,22 @@ static enum intr_trigger irq_trigger[BSP_IRQ_LINES_NUMBER];
  * while upper bits are interrupt on the slave PIC.
  * This cache is initialized in ldseg.s
  */
-static rtems_i8259_masks i8259a_cache = 0xFFFB;
+static rtems_i8259_masks i8259a_imr_cache = 0xFFFB;
+static rtems_i8259_masks i8259a_in_progress = 0;
+
+static inline
+void BSP_i8259a_irq_update_master_imr( void )
+{
+  rtems_i8259_masks mask = i8259a_in_progress | i8259a_imr_cache;
+  outport_byte( PIC_MASTER_IMR_IO_PORT, mask & 0xff );
+}
+
+static inline
+void BSP_i8259a_irq_update_slave_imr( void )
+{
+  rtems_i8259_masks mask = i8259a_in_progress | i8259a_imr_cache;
+  outport_byte( PIC_SLAVE_IMR_IO_PORT, ( mask >> 8 ) & 0xff );
+}
 
 /*
  * Print the stats.
@@ -107,7 +122,7 @@ static inline uint8_t 
BSP_i8259a_irq_in_service_reg(uint32_t ioport)
 /*-+
 | Function:  BSP_irq_disable_at_i8259a
 |  Description: Mask IRQ line in appropriate PIC chip.
-| Global Variables: i8259a_cache
+| Global Variables: i8259a_imr_cache, i8259a_in_progress
 |Arguments: vector_offset - number of IRQ line to mask.
 |  Returns: 0 is OK.
 +--*/
@@ -119,15 +134,15 @@ static int BSP_irq_disable_at_i8259a(const 
rtems_irq_number irqLine)
   rtems_interrupt_disable(level);
 
   mask = 1 << irqLine;
-  i8259a_cache |= mask;
+  i8259a_imr_cache |= mask;
 
   if (irqLine < 8)
   {
-outport_byte(PIC_MASTER_IMR_IO_PORT, i8259a_cache & 0xff);
+BSP_i8259a_irq_update_master_imr();
   }
   else
   {
-outport_byte(PIC_SLAVE_IMR_IO_PORT, (i8259a_cache >> 8) & 0xff);
+BSP_i8259a_irq_update_slave_imr();
   }
 
   rtems_interrupt_enable(level);
@@ -138,7 +153,7 @@ static int BSP_irq_disable_at_i8259a(const rtems_irq_number 
irqLine)
 /*-+
 | Function:  BSP_irq_enable_at_i8259a
 |  Description: Unmask IRQ line in appropriate PIC chip.
-| Global Variables: i8259a_cache
+| Global Variables: i8259a_imr_cache, i8259a_in_progress
 |Arguments: irqLine - number of IRQ line to mask.
 |  Returns: Nothing.
 +--*/
@@ -152,19 +167,19 @@ static int BSP_irq_enable_at_i8259a(const 
rtems_irq_number irqLine)
   rtems_interrupt_disable(level);
 
   mask = 1 << irqLine;
-  i8259a_cache &= ~mask;
+  i8259a_imr_cache &= ~mask;
 
   if (irqLine < 8)
   {
 isr = BSP_i8259a_irq_in_service_reg(PIC_MASTER_COMMAND_IO_PORT);
 irr = BSP_i8259a_irq_int_request_reg(PIC_MASTER_COMMAND_IO_PORT);
-outport_byte(PIC_MASTER_IMR_IO_PORT, i8259a_cache & 0xff);
+BSP_i8259a_irq_update_master_imr();
   }
   else
   {
 isr = BSP_i8259a_irq_in_service_reg(PIC_SLAVE_COMMAND_IO_PORT);
 irr = BSP_i8259a_irq_int_request_reg(PIC_SLAVE_COMMAND_IO_PORT);
-outport_byte(PIC_SLAVE_IMR_IO_PORT, (i8259a_cache >> 8) & 0xff);
+BSP_i8259a_irq_update_slave_imr();
   }
 
   if (((isr ^ irr) & mask) != 0)
@@ -298,7 +313,7 @@ void BSP_dispatch_isr(int vector);
 
 void BSP_dispatch_isr(int vector)
 {
-  uint16_t old_imr = 0;
+  rtems_i8259_masks in_progress_save = 0;
 
   if (vector < BSP_IRQ_VECTOR_NUMBER) {
 /*
@@ -329,10 +344,10 @@ void BSP_dispatch_isr(int vector)
* vector clear.
*/
   if (vector <= BSP_IRQ_MAX_ON_i8259A) {
-old_imr = i8259a_cache;
-i8259a_cache |= irq_mask_or_tbl[vector];
-outport_byte(PIC_MASTER_IMR_IO_PORT, i8259a_cache & 0xff);
-outport_byte(PIC_SLAVE_IMR_IO_PORT, (i8259a_cache >> 8) & 0xff);
+in_progress_save = i8259a_in_progress;
+i8259a_in_progress |= irq_mask_or_tbl[vector];
+BSP_i8259a_irq_update_master_imr();
+BSP_i8259a_irq_update_slave_imr();
   }
 
   /*
@@ -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;
-i8259a_cache = old_imr;
-outport_byte(PIC_MASTER_IMR_IO_PORT, i8259a_cache & 0xff);
-outport_byte(PIC_SLAVE_IMR_IO_PORT, (i8259a_cache >> 8) & 0xff);
+