Re: Commit a9d1fb58 - uefi-sct/SctPkg: allowable NotifyTpl in CreateEvent

Re: Commit a9d1fb58 - uefi-sct/SctPkg: allowable NotifyTpl in CreateEvent

[Heinrich Schuchardt]

On 6/10/21 7:22 PM, Samer El-Haj-Mahmoud wrote:
> + Mike
>
> I agree with Sunny's evaluation. The original EDK2 patch from Jan had good feedback from Mike K.. The exact language from the UEFI spec section 7.1 is:
>
> " Consequently, there is a fourth TPL, TPL_HIGH_LEVEL, designed for use exclusively by the firmware."

Firmware includes drivers. Drivers use CreateEvent().

>
> " This is the highest priority level. It is not interruptible (interrupts are disabled) and is used sparingly by firmware to synchronize operations that need to be accessible from any priority level. For example, it must be possible to signal events while executing at any priority level. Therefore, firmware manipulates the internal event structure while at this priority level."
>
> Given this, I think the following needs to happen:
>
> 1. The SCT commit (https://github.com/tianocore/edk2-test/commit/421a6997ef362c6286c4ef87d21d5367a9d1fb58 ) needs to be reverted
> 2. The SCT change can be re-submitted as a new patch, without adding TPL_HIGH_LEVEL
> 3. The original EDK2 patch on Jan 6 (https://edk2.groups.io/g/devel/topic/79479680#69853) needs to be re-submitted per Mike's feedback. This means not adding the TPL_HIGH_LEVEL support in CreateEvent
>
> In my opinion, (1) needs to be reverted right way, as it is causing SCT failures on many (all?) EDK2 based systems. This needs to happen before the edk2-test stable tag is created

Table 7-3 TPL Restrictions in UEFI spec 2.9 explicitly has:

Event Notification Levels
 > TPL_APPLICATION
<= TPL_HIGH_LEVEL

Nowhere does the specification say that CreateEvent() cannot be called
with with TPL_HIGH_LEVEL.

I can understand why Michael suggests that CreateEvent() should not be
callable at TPL_HIGH_LEVEL. But this requires a change request updating
the specification.

I am missing this step in the action list above.

1) and 2) can be done with a single patch. Just don't revert the line
adding TPL_APPLICATION.

Best regards

Heinrich

>
> Any thought about these suggested next steps?
>
> Thanks,
> --Samer
>
>> -----Original Message-----
>> From: Sunny Wang <Sunny.Wang@arm.com>
>> Sent: Thursday, June 10, 2021 4:26 AM
>> To: Heinrich Schuchardt <xypron.glpk@gmx.de>; Samer El-Haj-Mahmoud
>> <Samer.El-Haj-Mahmoud@arm.com>; Barton Gao
>> <gaojie@byosoft.com.cn>; G Edhaya Chandran
>> <Edhaya.Chandran@arm.com>
>> Cc: Sunny Wang <Sunny.Wang@arm.com>
>> Subject: RE: Commit a9d1fb58
>>
>> Thanks for the information, Heinrich.
>>
>> I think we need to further work on this for Mike's comment with your
>> ongoing edk2 patch.
>>       -
>> https://edk2.groups.io/g/devel/message/69853?p=,,,20,0,0,0::relevance,,M
>> deModulePkg%3A+correct+TPL+level+check+CoreCreateEventEx,20,2,0,794
>> 79680
>> For addressing Mike's comment, we will need to update both of your edk2
>> and edk2-test patches.
>> Therefore, I would suggest NOT picking up your edk2-test commit into the
>> upcoming stable tag/release. What do you guys think?
>>
>> In other words, the main problem is that SCT should not create an event with
>> TPL_HIGH_LEVEL, so we need to revert TPL_HIGH_LEVEL related change in
>> Heinrich's commit https://github.com/tianocore/edk2-
>> test/commit/421a6997ef362c6286c4ef87d21d5367a9d1fb58.
>>
>> By the way, I confirmed this issue on two of my AARCH64 platforms (RPi4 and
>> another one).
>>
>> Best Regards,
>> Sunny Wang
>>
>> -----Original Message-----
>> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> Sent: Thursday, June 10, 2021 1:04 PM
>> To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Barton
>> Gao <gaojie@byosoft.com.cn>; G Edhaya Chandran
>> <Edhaya.Chandran@arm.com>; Sunny Wang <Sunny.Wang@arm.com>
>> Subject: Re: Commit a9d1fb58
>>
>> On 6/10/21 4:10 AM, Samer El-Haj-Mahmoud wrote:
>>> With commit
>>> https://github.com/tianocore/edk2-
>> test/commit/421a6997ef362c6286c4ef87d21d5367a9d1fb58
>>> <https://github.com/tianocore/edk2-
>> test/commit/421a6997ef362c6286c4ef87d21d5367a9d1fb58>
>>> , I am seeing the following new failure on some EDK2 based systems:
>>>
>>> BS.CreateEvent - Create event with all valid event type and supported TPL.
>> --
>>> *FAILURE*
>>>
>>> EF317ADE-8668-456F-BED9-766056672DFF
>>>
>>> /RELEASE_BUILD/edk2-test/uefi-
>> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServices/
>> BlackBoxTest/EventTimerTaskPriorityServicesBBTestCreateEvent.c:520:Statu
>> s - Invalid Parameter, EventType - 0x00000100, NotifyTpl -
>>> 31
>>>
>>> BS.CreateEvent - Create event with all valid event type and supported TPL.
>> --
>>> *FAILURE*
>>>
>>> EF317ADE-8668-456F-BED9-766056672DFF
>>>
>>> /RELEASE_BUILD/edk2-test/uefi-
>> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServices/
>> BlackBoxTest/EventTimerTaskPriorityServicesBBTestCreateEvent.c:520:Statu
>> s - Invalid Parameter, EventType - 0x00000200, NotifyTpl -
>>> 31
>>>
>>> Are we sure this is implemented properly in EDK2? I imagine this code is
>>> common in TianoCore/EDK2, and all systems will experience such failure.
>>>
>>> I confirmed this on one AARCH64 system. I am trying to confirm on
>>> another AARCH64 as well.
>>>
>>> Can anyone confirm on other systems/archs?
>>>
>>> If there is going to be a stable tag/release of edk2-test, we may want
>>> to confirm this change does not cause failures in many systems before we
>>> pick it up.
>>>
>>> Thanks,
>>>
>>> --Samer
>>
>> An EDK II patch is available since 2021-01-06 via
>> https://bugzilla.tianocore.org/show_bug.cgi?id=3058
>> Cf. https://edk2.groups.io/g/devel/message/69851
>>
>> Best regards
>>
>> Heinrich
>>
>
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>

Re: [edk2-devel] Commit a9d1fb58 - uefi-sct/SctPkg: allowable NotifyTpl in CreateEvent

[Andrew Fish]

> On Jun 10, 2021, at 11:05 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> 
> On 6/10/21 7:22 PM, Samer El-Haj-Mahmoud wrote:
>> + Mike
>> 
>> I agree with Sunny's evaluation. The original EDK2 patch from Jan had good feedback from Mike K.. The exact language from the UEFI spec section 7.1 is:
>> 
>> " Consequently, there is a fourth TPL, TPL_HIGH_LEVEL, designed for use exclusively by the firmware."
> 
> Firmware includes drivers. Drivers use CreateEvent().
> 
>> 
>> " This is the highest priority level. It is not interruptible (interrupts are disabled) and is used sparingly by firmware to synchronize operations that need to be accessible from any priority level. For example, it must be possible to signal events while executing at any priority level. Therefore, firmware manipulates the internal event structure while at this priority level."
>> 
>> Given this, I think the following needs to happen:
>> 
>> 1. The SCT commit (https://github.com/tianocore/edk2-test/commit/421a6997ef362c6286c4ef87d21d5367a9d1fb58 ) needs to be reverted
>> 2. The SCT change can be re-submitted as a new patch, without adding TPL_HIGH_LEVEL
>> 3. The original EDK2 patch on Jan 6 (https://edk2.groups.io/g/devel/topic/79479680#69853) needs to be re-submitted per Mike's feedback. This means not adding the TPL_HIGH_LEVEL support in CreateEvent
>> 
>> In my opinion, (1) needs to be reverted right way, as it is causing SCT failures on many (all?) EDK2 based systems. This needs to happen before the edk2-test stable tag is created
> 
> Table 7-3 TPL Restrictions in UEFI spec 2.9 explicitly has:
> 
> Event Notification Levels
> > TPL_APPLICATION
> <= TPL_HIGH_LEVEL
> 
> Nowhere does the specification say that CreateEvent() cannot be called
> with with TPL_HIGH_LEVEL.
> 
> I can understand why Michael suggests that CreateEvent() should not be
> callable at TPL_HIGH_LEVEL. But this requires a change request updating
> the specification.
> 

This is the other bit of the spec that might be relevant….

7.1 Event, Timer, and Task Priority Services
...
Execution in the boot services environment occurs at different task priority levels, or TPLs. The boot services environment exposes only three of these levels to UEFI applications and drivers:
• TPL_APPLICATION, the lowest priority level
• TPL_CALLBACK, an intermediate priority level
• TPL_NOTIFY, the highest priority level

Thanks,

Andrew Fish

> I am missing this step in the action list above.
> 
> 1) and 2) can be done with a single patch. Just don't revert the line
> adding TPL_APPLICATION.
> 
> Best regards
> 
> Heinrich
> 
>> 
>> Any thought about these suggested next steps?
>> 
>> Thanks,
>> --Samer
>> 
>>> -----Original Message-----
>>> From: Sunny Wang <Sunny.Wang@arm.com>
>>> Sent: Thursday, June 10, 2021 4:26 AM
>>> To: Heinrich Schuchardt <xypron.glpk@gmx.de>; Samer El-Haj-Mahmoud
>>> <Samer.El-Haj-Mahmoud@arm.com>; Barton Gao
>>> <gaojie@byosoft.com.cn>; G Edhaya Chandran
>>> <Edhaya.Chandran@arm.com>
>>> Cc: Sunny Wang <Sunny.Wang@arm.com>
>>> Subject: RE: Commit a9d1fb58
>>> 
>>> Thanks for the information, Heinrich.
>>> 
>>> I think we need to further work on this for Mike's comment with your
>>> ongoing edk2 patch.
>>>      -
>>> https://edk2.groups.io/g/devel/message/69853?p=,,,20,0,0,0::relevance,,M
>>> deModulePkg%3A+correct+TPL+level+check+CoreCreateEventEx,20,2,0,794
>>> 79680
>>> For addressing Mike's comment, we will need to update both of your edk2
>>> and edk2-test patches.
>>> Therefore, I would suggest NOT picking up your edk2-test commit into the
>>> upcoming stable tag/release. What do you guys think?
>>> 
>>> In other words, the main problem is that SCT should not create an event with
>>> TPL_HIGH_LEVEL, so we need to revert TPL_HIGH_LEVEL related change in
>>> Heinrich's commit https://github.com/tianocore/edk2-
>>> test/commit/421a6997ef362c6286c4ef87d21d5367a9d1fb58.
>>> 
>>> By the way, I confirmed this issue on two of my AARCH64 platforms (RPi4 and
>>> another one).
>>> 
>>> Best Regards,
>>> Sunny Wang
>>> 
>>> -----Original Message-----
>>> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> Sent: Thursday, June 10, 2021 1:04 PM
>>> To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Barton
>>> Gao <gaojie@byosoft.com.cn>; G Edhaya Chandran
>>> <Edhaya.Chandran@arm.com>; Sunny Wang <Sunny.Wang@arm.com>
>>> Subject: Re: Commit a9d1fb58
>>> 
>>> On 6/10/21 4:10 AM, Samer El-Haj-Mahmoud wrote:
>>>> With commit
>>>> https://github.com/tianocore/edk2-
>>> test/commit/421a6997ef362c6286c4ef87d21d5367a9d1fb58
>>>> <https://github.com/tianocore/edk2-
>>> test/commit/421a6997ef362c6286c4ef87d21d5367a9d1fb58>
>>>> , I am seeing the following new failure on some EDK2 based systems:
>>>> 
>>>> BS.CreateEvent - Create event with all valid event type and supported TPL.
>>> --
>>>> *FAILURE*
>>>> 
>>>> EF317ADE-8668-456F-BED9-766056672DFF
>>>> 
>>>> /RELEASE_BUILD/edk2-test/uefi-
>>> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServices/
>>> BlackBoxTest/EventTimerTaskPriorityServicesBBTestCreateEvent.c:520:Statu
>>> s - Invalid Parameter, EventType - 0x00000100, NotifyTpl -
>>>> 31
>>>> 
>>>> BS.CreateEvent - Create event with all valid event type and supported TPL.
>>> --
>>>> *FAILURE*
>>>> 
>>>> EF317ADE-8668-456F-BED9-766056672DFF
>>>> 
>>>> /RELEASE_BUILD/edk2-test/uefi-
>>> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServices/
>>> BlackBoxTest/EventTimerTaskPriorityServicesBBTestCreateEvent.c:520:Statu
>>> s - Invalid Parameter, EventType - 0x00000200, NotifyTpl -
>>>> 31
>>>> 
>>>> Are we sure this is implemented properly in EDK2? I imagine this code is
>>>> common in TianoCore/EDK2, and all systems will experience such failure.
>>>> 
>>>> I confirmed this on one AARCH64 system. I am trying to confirm on
>>>> another AARCH64 as well.
>>>> 
>>>> Can anyone confirm on other systems/archs?
>>>> 
>>>> If there is going to be a stable tag/release of edk2-test, we may want
>>>> to confirm this change does not cause failures in many systems before we
>>>> pick it up.
>>>> 
>>>> Thanks,
>>>> 
>>>> --Samer
>>> 
>>> An EDK II patch is available since 2021-01-06 via
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=3058
>>> Cf. https://edk2.groups.io/g/devel/message/69851
>>> 
>>> Best regards
>>> 
>>> Heinrich
>>> 
>> 
>> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>> 
> 
> 
> 
> 
> 
> 

Re: [edk2-devel] Commit a9d1fb58 - uefi-sct/SctPkg: allowable NotifyTpl in CreateEvent

[Sunny Wang]

I just sent a patch for this.  https://edk2.groups.io/g/devel/message/76368
Please help review the patch so that we can pick this up in SCT stable release.

Best Regards,
Sunny Wang

-----Original Message-----
From: Andrew Fish <afish@apple.com>
Sent: Friday, June 11, 2021 2:50 AM
To: edk2-devel-groups-io <devel@edk2.groups.io>; xypron.glpk@gmx.de
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Sunny Wang <Sunny.Wang@arm.com>; Barton Gao <gaojie@byosoft.com.cn>; G Edhaya Chandran <Edhaya.Chandran@arm.com>; Mike Kinney <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] Commit a9d1fb58 - uefi-sct/SctPkg: allowable NotifyTpl in CreateEvent



> On Jun 10, 2021, at 11:05 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 6/10/21 7:22 PM, Samer El-Haj-Mahmoud wrote:
>> + Mike
>>
>> I agree with Sunny's evaluation. The original EDK2 patch from Jan had good feedback from Mike K.. The exact language from the UEFI spec section 7.1 is:
>>
>> " Consequently, there is a fourth TPL, TPL_HIGH_LEVEL, designed for use exclusively by the firmware."
>
> Firmware includes drivers. Drivers use CreateEvent().
>
>>
>> " This is the highest priority level. It is not interruptible (interrupts are disabled) and is used sparingly by firmware to synchronize operations that need to be accessible from any priority level. For example, it must be possible to signal events while executing at any priority level. Therefore, firmware manipulates the internal event structure while at this priority level."
>>
>> Given this, I think the following needs to happen:
>>
>> 1. The SCT commit (https://github.com/tianocore/edk2-test/commit/421a6997ef362c6286c4ef87d21d5367a9d1fb58 ) needs to be reverted
>> 2. The SCT change can be re-submitted as a new patch, without adding TPL_HIGH_LEVEL
>> 3. The original EDK2 patch on Jan 6 (https://edk2.groups.io/g/devel/topic/79479680#69853) needs to be re-submitted per Mike's feedback. This means not adding the TPL_HIGH_LEVEL support in CreateEvent
>>
>> In my opinion, (1) needs to be reverted right way, as it is causing SCT failures on many (all?) EDK2 based systems. This needs to happen before the edk2-test stable tag is created
>
> Table 7-3 TPL Restrictions in UEFI spec 2.9 explicitly has:
>
> Event Notification Levels
> > TPL_APPLICATION
> <= TPL_HIGH_LEVEL
>
> Nowhere does the specification say that CreateEvent() cannot be called
> with with TPL_HIGH_LEVEL.
>
> I can understand why Michael suggests that CreateEvent() should not be
> callable at TPL_HIGH_LEVEL. But this requires a change request updating
> the specification.
>

This is the other bit of the spec that might be relevant….

7.1 Event, Timer, and Task Priority Services
...
Execution in the boot services environment occurs at different task priority levels, or TPLs. The boot services environment exposes only three of these levels to UEFI applications and drivers:
• TPL_APPLICATION, the lowest priority level
• TPL_CALLBACK, an intermediate priority level
• TPL_NOTIFY, the highest priority level

Thanks,

Andrew Fish

> I am missing this step in the action list above.
>
> 1) and 2) can be done with a single patch. Just don't revert the line
> adding TPL_APPLICATION.
>
> Best regards
>
> Heinrich
>
>>
>> Any thought about these suggested next steps?
>>
>> Thanks,
>> --Samer
>>
>>> -----Original Message-----
>>> From: Sunny Wang <Sunny.Wang@arm.com>
>>> Sent: Thursday, June 10, 2021 4:26 AM
>>> To: Heinrich Schuchardt <xypron.glpk@gmx.de>; Samer El-Haj-Mahmoud
>>> <Samer.El-Haj-Mahmoud@arm.com>; Barton Gao
>>> <gaojie@byosoft.com.cn>; G Edhaya Chandran
>>> <Edhaya.Chandran@arm.com>
>>> Cc: Sunny Wang <Sunny.Wang@arm.com>
>>> Subject: RE: Commit a9d1fb58
>>>
>>> Thanks for the information, Heinrich.
>>>
>>> I think we need to further work on this for Mike's comment with your
>>> ongoing edk2 patch.
>>>      -
>>> https://edk2.groups.io/g/devel/message/69853?p=,,,20,0,0,0::relevance,,M
>>> deModulePkg%3A+correct+TPL+level+check+CoreCreateEventEx,20,2,0,794
>>> 79680
>>> For addressing Mike's comment, we will need to update both of your edk2
>>> and edk2-test patches.
>>> Therefore, I would suggest NOT picking up your edk2-test commit into the
>>> upcoming stable tag/release. What do you guys think?
>>>
>>> In other words, the main problem is that SCT should not create an event with
>>> TPL_HIGH_LEVEL, so we need to revert TPL_HIGH_LEVEL related change in
>>> Heinrich's commit https://github.com/tianocore/edk2-
>>> test/commit/421a6997ef362c6286c4ef87d21d5367a9d1fb58.
>>>
>>> By the way, I confirmed this issue on two of my AARCH64 platforms (RPi4 and
>>> another one).
>>>
>>> Best Regards,
>>> Sunny Wang
>>>
>>> -----Original Message-----
>>> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> Sent: Thursday, June 10, 2021 1:04 PM
>>> To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Barton
>>> Gao <gaojie@byosoft.com.cn>; G Edhaya Chandran
>>> <Edhaya.Chandran@arm.com>; Sunny Wang <Sunny.Wang@arm.com>
>>> Subject: Re: Commit a9d1fb58
>>>
>>> On 6/10/21 4:10 AM, Samer El-Haj-Mahmoud wrote:
>>>> With commit
>>>> https://github.com/tianocore/edk2-
>>> test/commit/421a6997ef362c6286c4ef87d21d5367a9d1fb58
>>>> <https://github.com/tianocore/edk2-
>>> test/commit/421a6997ef362c6286c4ef87d21d5367a9d1fb58>
>>>> , I am seeing the following new failure on some EDK2 based systems:
>>>>
>>>> BS.CreateEvent - Create event with all valid event type and supported TPL.
>>> --
>>>> *FAILURE*
>>>>
>>>> EF317ADE-8668-456F-BED9-766056672DFF
>>>>
>>>> /RELEASE_BUILD/edk2-test/uefi-
>>> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServices/
>>> BlackBoxTest/EventTimerTaskPriorityServicesBBTestCreateEvent.c:520:Statu
>>> s - Invalid Parameter, EventType - 0x00000100, NotifyTpl -
>>>> 31
>>>>
>>>> BS.CreateEvent - Create event with all valid event type and supported TPL.
>>> --
>>>> *FAILURE*
>>>>
>>>> EF317ADE-8668-456F-BED9-766056672DFF
>>>>
>>>> /RELEASE_BUILD/edk2-test/uefi-
>>> sct/SctPkg/TestCase/UEFI/EFI/BootServices/EventTimerTaskPriorityServices/
>>> BlackBoxTest/EventTimerTaskPriorityServicesBBTestCreateEvent.c:520:Statu
>>> s - Invalid Parameter, EventType - 0x00000200, NotifyTpl -
>>>> 31
>>>>
>>>> Are we sure this is implemented properly in EDK2? I imagine this code is
>>>> common in TianoCore/EDK2, and all systems will experience such failure.
>>>>
>>>> I confirmed this on one AARCH64 system. I am trying to confirm on
>>>> another AARCH64 as well.
>>>>
>>>> Can anyone confirm on other systems/archs?
>>>>
>>>> If there is going to be a stable tag/release of edk2-test, we may want
>>>> to confirm this change does not cause failures in many systems before we
>>>> pick it up.
>>>>
>>>> Thanks,
>>>>
>>>> --Samer
>>>
>>> An EDK II patch is available since 2021-01-06 via
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=3058
>>> Cf. https://edk2.groups.io/g/devel/message/69851
>>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>
>> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>>
>
>
>
> 
>
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.