In l4vcpu_irq_restore function don't use l4vcpu_irq_disable(vcpu)

[Ildar]

2012/10/17 Philipp Eppelt <philipp.eppelt at mailbox.tu-dresden.de>:
>
>
> On 10/16/2012 02:04 PM, Ildar Ismagilov wrote:
>>
>> In function l4vcpu_irq_restore on l4/pkg/libvcpu/include/vcpu.h file:
>>
>> L4_CV L4_INLINE
>> void
>> l4vcpu_irq_restore(l4_vcpu_state_t *vcpu, l4vcpu_irq_state_t s,
>>                     l4_utcb_t *utcb,
>>                     l4vcpu_event_hndl_t do_event_work_cb,
>>                     l4vcpu_setup_ipc_t setup_ipc) L4_NOTHROW
>> {
>>    if (s & L4_VCPU_F_IRQ)
>>      l4vcpu_irq_enable(vcpu, utcb, do_event_work_cb, setup_ipc);
>> }
>>
>> why don't call  l4vcpu_irq_disable(vcpu) if "s" variable don't include
>> L4_VCPU_F_IRQ flag?
>>
>> possible should be so:
>>
>> L4_CV L4_INLINE
>> void
>> l4vcpu_irq_restore(l4_vcpu_state_t *vcpu, l4vcpu_irq_state_t s,
>>                     l4_utcb_t *utcb,
>>                     l4vcpu_event_hndl_t do_event_work_cb,
>>                     l4vcpu_setup_ipc_t setup_ipc) L4_NOTHROW
>> {
>>    if (s & L4_VCPU_F_IRQ)
>>      l4vcpu_irq_enable(vcpu, utcb, do_event_work_cb, setup_ipc);
>>    else
>>      l4vcpu_irq_disable(vcpu);
>> }
>>
>
> Hi Ildar,
>
> your way of writing it is also valid, but that would be an unnecesseary
> call. Here is why:
> You can only get a (valid) state to restore by a call to
> l4vcpu_irq_disable_save, so IRQs in your vcpu are disabled.
>
> When you now call l4vcpu_irq_restore and you want to restore a state with
> disabled IRQs, you don't need to change anything. If you want to restore a
> state with enabled IRQs, then IRQs get enabled.
>
>
> Regards,
> Philipp
>
> _______________________________________________
> l4-hackers mailing list
> l4-hackers at os.inf.tu-dresden.de
> http://os.inf.tu-dresden.de/mailman/listinfo/l4-hackers

found bug in genode block driver for L4Linux. Below you can find
description of the bug in a pseudo code:

unsigned long flag1;
unsigned long flag2;

flag1 = l4vcpu_irq_disable_save(...);
// irq is DISABLED
flag2 = l4vcpu_irq_disable_save(...);
// irq is DISABLED
...
l4vcpu_irq_enable(...);
// irq is ENABLED
...
// BUG: not called l4vcpu_irq_disable function!
l4vcpu_irq_restore(flag2);
// irq is ENABLED but should be DISABLED!!!
l4vcpu_irq_restore(flag1);

Should two function l4vcpu_irq_disable_save and l4vcpu_irq_restore
guarantee equal irq state before l4vcpu_irq_disable_save and after
l4vcpu_irq_restore functions?
Maybe an assert in l4vcpu_irq_restore function for bug checking should
be added? Or maybe debug print and l4vcpu_irq_disable(vcpu) function
call should be added?

-- 
Best regards
Ildar