[PATCH v2 0/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[PATCH v2 0/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[Heyi Guo]

After rebasing to edk2 commit 5e3719a, we found D05 would hang after printing a
lot of "Spurious interrupt" messages. The issue would gone away if we restored
the removal of "enable interrupt source":
  gInterrupt->EnableInterruptSource (gInterrupt, Source);

It can also be fixed if we add a "ISB" after reloading timer compare value, and
we agree that it makes sense to do that.

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>

Heyi Guo (1):
  ArmPkg/TimerDxe: Add ISB for timer compare value reload

 ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +
 1 file changed, 1 insertion(+)

-- 
2.7.4

[PATCH v2 1/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[Heyi Guo]

If timer interrupt is level sensitive, reloading timer compare
register has a side effect of clearing GIC pending status, so a "ISB"
is needed to make sure this instruction is executed before enabling
CPU IRQ, or else we may get spurious timer interrupts.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
---

Notes:
    v2:
    - Use ISB instead of DSB [Marc]
    - Update commit message accordingly.

 ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
index 33d7c922221f..32abee8726a0 100644
--- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
+++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
@@ -337,6 +337,7 @@ TimerInterruptHandler (
 
     // Set next compare value
     ArmGenericTimerSetCompareVal (CompareValue);
+    ArmInstructionSynchronizationBarrier ();
     ArmGenericTimerEnableTimer ();
   }
 
-- 
2.7.4

Re: [PATCH v2 1/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[Marc Zyngier]

On 13/03/18 00:31, Heyi Guo wrote:
> If timer interrupt is level sensitive, reloading timer compare
> register has a side effect of clearing GIC pending status, so a "ISB"
> is needed to make sure this instruction is executed before enabling
> CPU IRQ, or else we may get spurious timer interrupts.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> ---
> 
> Notes:
>     v2:
>     - Use ISB instead of DSB [Marc]
>     - Update commit message accordingly.
> 
>  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> index 33d7c922221f..32abee8726a0 100644
> --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> @@ -337,6 +337,7 @@ TimerInterruptHandler (
>  
>      // Set next compare value
>      ArmGenericTimerSetCompareVal (CompareValue);
> +    ArmInstructionSynchronizationBarrier ();
>      ArmGenericTimerEnableTimer ();
>    }

Sorry for being pedantic here, but it would make more sense if ISB was
placed after the enabling of the timer. Otherwise, you only force the
update of the comparator. But on the other hand, the timer was never
disabled the first place, in which case you'd wonder why you're trying
to enable it again.

So either you leave the ISB here and remove the enable call, or move the
ISB after the enable.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 1/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[Guo Heyi]

On Tue, Mar 13, 2018 at 09:33:33AM +0000, Marc Zyngier wrote:
> On 13/03/18 00:31, Heyi Guo wrote:
> > If timer interrupt is level sensitive, reloading timer compare
> > register has a side effect of clearing GIC pending status, so a "ISB"
> > is needed to make sure this instruction is executed before enabling
> > CPU IRQ, or else we may get spurious timer interrupts.
> > 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> > 
> > Notes:
> >     v2:
> >     - Use ISB instead of DSB [Marc]
> >     - Update commit message accordingly.
> > 
> >  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > index 33d7c922221f..32abee8726a0 100644
> > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > @@ -337,6 +337,7 @@ TimerInterruptHandler (
> >  
> >      // Set next compare value
> >      ArmGenericTimerSetCompareVal (CompareValue);
> > +    ArmInstructionSynchronizationBarrier ();
> >      ArmGenericTimerEnableTimer ();
> >    }
> 
> Sorry for being pedantic here, but it would make more sense if ISB was
> placed after the enabling of the timer. Otherwise, you only force the
> update of the comparator. But on the other hand, the timer was never
> disabled the first place, in which case you'd wonder why you're trying
> to enable it again.
Yes, I also had such question and hesitated at this place :)
> 
> So either you leave the ISB here and remove the enable call, or move the
> ISB after the enable.

If we are going to remove the enable call, is it better to be changed in a
separate patch? It seems not related with adding ISB, though it is only a
one-line change.

Thanks and regards,
Heyi

> 
> Thanks,
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...

Re: [PATCH v2 1/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[Marc Zyngier]

On Wed, 14 Mar 2018 00:25:09 +0000,
Guo Heyi wrote:
> 
> On Tue, Mar 13, 2018 at 09:33:33AM +0000, Marc Zyngier wrote:
> > On 13/03/18 00:31, Heyi Guo wrote:
> > > If timer interrupt is level sensitive, reloading timer compare
> > > register has a side effect of clearing GIC pending status, so a "ISB"
> > > is needed to make sure this instruction is executed before enabling
> > > CPU IRQ, or else we may get spurious timer interrupts.
> > > 
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> > > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
> > > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > > ---
> > > 
> > > Notes:
> > >     v2:
> > >     - Use ISB instead of DSB [Marc]
> > >     - Update commit message accordingly.
> > > 
> > >  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > > index 33d7c922221f..32abee8726a0 100644
> > > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > > @@ -337,6 +337,7 @@ TimerInterruptHandler (
> > >  
> > >      // Set next compare value
> > >      ArmGenericTimerSetCompareVal (CompareValue);
> > > +    ArmInstructionSynchronizationBarrier ();
> > >      ArmGenericTimerEnableTimer ();
> > >    }
> > 
> > Sorry for being pedantic here, but it would make more sense if ISB was
> > placed after the enabling of the timer. Otherwise, you only force the
> > update of the comparator. But on the other hand, the timer was never
> > disabled the first place, in which case you'd wonder why you're trying
> > to enable it again.
> Yes, I also had such question and hesitated at this place :)
> > 
> > So either you leave the ISB here and remove the enable call, or move the
> > ISB after the enable.
> 
> If we are going to remove the enable call, is it better to be changed in a
> separate patch? It seems not related with adding ISB, though it is only a
> one-line change.

I guess a separate patch doesn't hurt, but that's for Ard and Leif to
decide.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH v2 1/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[Ard Biesheuvel]

On 14 March 2018 at 07:45, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On Wed, 14 Mar 2018 00:25:09 +0000,
> Guo Heyi wrote:
>>
>> On Tue, Mar 13, 2018 at 09:33:33AM +0000, Marc Zyngier wrote:
>> > On 13/03/18 00:31, Heyi Guo wrote:
>> > > If timer interrupt is level sensitive, reloading timer compare
>> > > register has a side effect of clearing GIC pending status, so a "ISB"
>> > > is needed to make sure this instruction is executed before enabling
>> > > CPU IRQ, or else we may get spurious timer interrupts.
>> > >
>> > > Contributed-under: TianoCore Contribution Agreement 1.1
>> > > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
>> > > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
>> > > Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > > Cc: Marc Zyngier <marc.zyngier@arm.com>
>> > > ---
>> > >
>> > > Notes:
>> > >     v2:
>> > >     - Use ISB instead of DSB [Marc]
>> > >     - Update commit message accordingly.
>> > >
>> > >  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +
>> > >  1 file changed, 1 insertion(+)
>> > >
>> > > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
>> > > index 33d7c922221f..32abee8726a0 100644
>> > > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
>> > > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
>> > > @@ -337,6 +337,7 @@ TimerInterruptHandler (
>> > >
>> > >      // Set next compare value
>> > >      ArmGenericTimerSetCompareVal (CompareValue);
>> > > +    ArmInstructionSynchronizationBarrier ();
>> > >      ArmGenericTimerEnableTimer ();
>> > >    }
>> >
>> > Sorry for being pedantic here, but it would make more sense if ISB was
>> > placed after the enabling of the timer. Otherwise, you only force the
>> > update of the comparator. But on the other hand, the timer was never
>> > disabled the first place, in which case you'd wonder why you're trying
>> > to enable it again.
>> Yes, I also had such question and hesitated at this place :)
>> >
>> > So either you leave the ISB here and remove the enable call, or move the
>> > ISB after the enable.
>>
>> If we are going to remove the enable call, is it better to be changed in a
>> separate patch? It seems not related with adding ISB, though it is only a
>> one-line change.
>
> I guess a separate patch doesn't hurt, but that's for Ard and Leif to
> decide.
>

Yes, please.

Re: [PATCH v2 1/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[Guo Heyi]

Hi Marc and Ard,

I found the timer re-enable code was added by Ard for special reason:

commit b1a633434ddc5fc28de817debd963f7845fb78c7
Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date:   Thu Sep 18 21:16:47 2014 +0000

    ArmPkg/TimerDxe: add workaround for KVM timer interrupt handling

So this line of code cannot be removed and I will add an ISB after enabling
timer.

Another strange thing is that we got lots of "Spurious GIC interrupt" error
messages when timer enable code was removed, though at last it could boot to EFI
shell. Please let me know if you have any idea about the possible reason of this
issue.

Regards,
Heyi

On Wed, Mar 14, 2018 at 02:50:41PM +0000, Ard Biesheuvel wrote:
> On 14 March 2018 at 07:45, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > On Wed, 14 Mar 2018 00:25:09 +0000,
> > Guo Heyi wrote:
> >>
> >> On Tue, Mar 13, 2018 at 09:33:33AM +0000, Marc Zyngier wrote:
> >> > On 13/03/18 00:31, Heyi Guo wrote:
> >> > > If timer interrupt is level sensitive, reloading timer compare
> >> > > register has a side effect of clearing GIC pending status, so a "ISB"
> >> > > is needed to make sure this instruction is executed before enabling
> >> > > CPU IRQ, or else we may get spurious timer interrupts.
> >> > >
> >> > > Contributed-under: TianoCore Contribution Agreement 1.1
> >> > > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> >> > > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
> >> > > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> >> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> > > Cc: Marc Zyngier <marc.zyngier@arm.com>
> >> > > ---
> >> > >
> >> > > Notes:
> >> > >     v2:
> >> > >     - Use ISB instead of DSB [Marc]
> >> > >     - Update commit message accordingly.
> >> > >
> >> > >  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +
> >> > >  1 file changed, 1 insertion(+)
> >> > >
> >> > > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> >> > > index 33d7c922221f..32abee8726a0 100644
> >> > > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> >> > > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> >> > > @@ -337,6 +337,7 @@ TimerInterruptHandler (
> >> > >
> >> > >      // Set next compare value
> >> > >      ArmGenericTimerSetCompareVal (CompareValue);
> >> > > +    ArmInstructionSynchronizationBarrier ();
> >> > >      ArmGenericTimerEnableTimer ();
> >> > >    }
> >> >
> >> > Sorry for being pedantic here, but it would make more sense if ISB was
> >> > placed after the enabling of the timer. Otherwise, you only force the
> >> > update of the comparator. But on the other hand, the timer was never
> >> > disabled the first place, in which case you'd wonder why you're trying
> >> > to enable it again.
> >> Yes, I also had such question and hesitated at this place :)
> >> >
> >> > So either you leave the ISB here and remove the enable call, or move the
> >> > ISB after the enable.
> >>
> >> If we are going to remove the enable call, is it better to be changed in a
> >> separate patch? It seems not related with adding ISB, though it is only a
> >> one-line change.
> >
> > I guess a separate patch doesn't hurt, but that's for Ard and Leif to
> > decide.
> >
> 
> Yes, please.

Re: [PATCH v2 1/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[Ard Biesheuvel]

On 15 March 2018 at 07:11, Guo Heyi <heyi.guo@linaro.org> wrote:
> Hi Marc and Ard,
>
> I found the timer re-enable code was added by Ard for special reason:
>
> commit b1a633434ddc5fc28de817debd963f7845fb78c7
> Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Date:   Thu Sep 18 21:16:47 2014 +0000
>
>     ArmPkg/TimerDxe: add workaround for KVM timer interrupt handling
>
> So this line of code cannot be removed and I will add an ISB after enabling
> timer.
>

I'm not sure. IIUC, the KVM issue that required this has been fixed
long ago, and I don't want to carry this forever. Marc?

> Another strange thing is that we got lots of "Spurious GIC interrupt" error
> messages when timer enable code was removed, though at last it could boot to EFI
> shell. Please let me know if you have any idea about the possible reason of this
> issue.
>

No idea, sorry.

Re: [PATCH v2 1/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[Marc Zyngier]

On 15/03/18 07:30, Ard Biesheuvel wrote:
> On 15 March 2018 at 07:11, Guo Heyi <heyi.guo@linaro.org> wrote:
>> Hi Marc and Ard,
>>
>> I found the timer re-enable code was added by Ard for special reason:
>>
>> commit b1a633434ddc5fc28de817debd963f7845fb78c7
>> Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Date:   Thu Sep 18 21:16:47 2014 +0000
>>
>>     ArmPkg/TimerDxe: add workaround for KVM timer interrupt handling
>>
>> So this line of code cannot be removed and I will add an ISB after enabling
>> timer.
>>
> 
> I'm not sure. IIUC, the KVM issue that required this has been fixed
> long ago, and I don't want to carry this forever. Marc?

This has been fixed quite a while ago:

commit f120cd6533d21075ab103ae6c225b1697853660d
Author: Marc Zyngier <marc.zyngier@arm.com>
Date:   Mon Jun 23 13:59:13 2014 +0100

    KVM: arm/arm64: timer: Allow the timer to control the active state

    In order to remove the crude hack where we sneak the masked bit
    into the timer's control register, make use of the phys_irq_map
    API control the active state of the interrupt.

    This causes some limited changes to allow for potential error
    propagation.

    Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
    Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

>> Another strange thing is that we got lots of "Spurious GIC interrupt" error
>> messages when timer enable code was removed, though at last it could boot to EFI
>> shell. Please let me know if you have any idea about the possible reason of this
>> issue.
>>
> 
> No idea, sorry.

Me neither. You'd have to dump the control register before and after,
and work out what is being changed exactly.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 1/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[Ard Biesheuvel]

On 15 March 2018 at 09:40, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 15/03/18 07:30, Ard Biesheuvel wrote:
>> On 15 March 2018 at 07:11, Guo Heyi <heyi.guo@linaro.org> wrote:
>>> Hi Marc and Ard,
>>>
>>> I found the timer re-enable code was added by Ard for special reason:
>>>
>>> commit b1a633434ddc5fc28de817debd963f7845fb78c7
>>> Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Date:   Thu Sep 18 21:16:47 2014 +0000
>>>
>>>     ArmPkg/TimerDxe: add workaround for KVM timer interrupt handling
>>>
>>> So this line of code cannot be removed and I will add an ISB after enabling
>>> timer.
>>>
>>
>> I'm not sure. IIUC, the KVM issue that required this has been fixed
>> long ago, and I don't want to carry this forever. Marc?
>
> This has been fixed quite a while ago:
>
> commit f120cd6533d21075ab103ae6c225b1697853660d
> Author: Marc Zyngier <marc.zyngier@arm.com>
> Date:   Mon Jun 23 13:59:13 2014 +0100
>
>     KVM: arm/arm64: timer: Allow the timer to control the active state
>
>     In order to remove the crude hack where we sneak the masked bit
>     into the timer's control register, make use of the phys_irq_map
>     API control the active state of the interrupt.
>
>     This causes some limited changes to allow for potential error
>     propagation.
>
>     Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
>     Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>

Does this mean we can lose this as well?

https://github.com/tianocore/edk2/blob/master/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c#L31


>>> Another strange thing is that we got lots of "Spurious GIC interrupt" error
>>> messages when timer enable code was removed, though at last it could boot to EFI
>>> shell. Please let me know if you have any idea about the possible reason of this
>>> issue.
>>>
>>
>> No idea, sorry.
>
> Me neither. You'd have to dump the control register before and after,
> and work out what is being changed exactly.
>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...

Re: [PATCH v2 1/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[Marc Zyngier]

On 15/03/18 09:52, Ard Biesheuvel wrote:
> On 15 March 2018 at 09:40, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 15/03/18 07:30, Ard Biesheuvel wrote:
>>> On 15 March 2018 at 07:11, Guo Heyi <heyi.guo@linaro.org> wrote:
>>>> Hi Marc and Ard,
>>>>
>>>> I found the timer re-enable code was added by Ard for special reason:
>>>>
>>>> commit b1a633434ddc5fc28de817debd963f7845fb78c7
>>>> Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Date:   Thu Sep 18 21:16:47 2014 +0000
>>>>
>>>>     ArmPkg/TimerDxe: add workaround for KVM timer interrupt handling
>>>>
>>>> So this line of code cannot be removed and I will add an ISB after enabling
>>>> timer.
>>>>
>>>
>>> I'm not sure. IIUC, the KVM issue that required this has been fixed
>>> long ago, and I don't want to carry this forever. Marc?
>>
>> This has been fixed quite a while ago:
>>
>> commit f120cd6533d21075ab103ae6c225b1697853660d
>> Author: Marc Zyngier <marc.zyngier@arm.com>
>> Date:   Mon Jun 23 13:59:13 2014 +0100
>>
>>     KVM: arm/arm64: timer: Allow the timer to control the active state
>>
>>     In order to remove the crude hack where we sneak the masked bit
>>     into the timer's control register, make use of the phys_irq_map
>>     API control the active state of the interrupt.
>>
>>     This causes some limited changes to allow for potential error
>>     propagation.
>>
>>     Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
>>     Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>
> 
> Does this mean we can lose this as well?
> 
> https://github.com/tianocore/edk2/blob/master/ArmPkg/Library/ArmGenericTimerVirtCounterLib/ArmGenericTimerVirtCounterLib.c#L31

Indeed, you should be able to remove this as well.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...