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

Thu, 10 Jun 2021 11:06:36 -0700 

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 <[email protected]>
Sent: Thursday, June 10, 2021 4:26 AM
To: Heinrich Schuchardt <[email protected]>; Samer El-Haj-Mahmoud
<[email protected]>; Barton Gao
<[email protected]>; G Edhaya Chandran
<[email protected]>
Cc: Sunny Wang <[email protected]>
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 <[email protected]>
Sent: Thursday, June 10, 2021 1:04 PM
To: Samer El-Haj-Mahmoud <[email protected]>; Barton
Gao <[email protected]>; G Edhaya Chandran
<[email protected]>; Sunny Wang <[email protected]>
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.





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76337): https://edk2.groups.io/g/devel/message/76337
Mute This Topic: https://groups.io/mt/83451169/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-

Reply via email to