Re: [edk2-devel] [PATCH v1 07/14] NetworkPkg:: SECURITY PATCH CVE-2023-45237

[Michael Brown]

On 08/05/2024 22:19, Ard Biesheuvel wrote:

I've always found that logic rather bizarre - there is no way the
implementation of the raw protocol can ensure that the caller uses it
correctly, and so enforcing a minimum read size is pointless and
arbitrary. And as you note, it has no basis in the UEFI spec either.

So this should just be removed imo.


For what it's worth, I agree that it should be removed.

iPXE has the following comment:

/** Minimum number of bytes to request from RNG
 *
 * The UEFI spec states (for no apparently good reason) that "When a
 * Deterministic Random Bit Generator (DRBG) is used on the output of
 * a (raw) entropy source, its security level must be at least 256
 * bits."  The EDK2 codebase (mis)interprets this to mean that the
 * call to GetRNG() should fail if given a buffer less than 32 bytes.
 *
 * Incidentally, nothing in the EFI RNG protocol provides any way to
 * report the actual amount of entropy returned by GetRNG().
 */
#define EFIRNG_LEN 32

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118705): https://edk2.groups.io/g/devel/message/118705
Mute This Topic: https://groups.io/mt/105983246/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] NetworkPkg/HttpBootDxe: Correctly uninstall HttpBootCallbackProtocol

[Michael Brown]

On 19/04/2024 11:02, Mike Beaton wrote:

Dear Michael,

I don't know if you had time to answer one follow-up question.

Obviously one thing that someone might want to do is to notify on 
protocol installs and trap installs of this protocol - e.g. so that 
something other than UefiBootManagerLib can manage and monitor HTTP 
boot, but still allowing the original callback to occur, by hooking it. 
Not sure if this counts as 'supported' or not (possibly not...) though I 
think it may count as 'quite likely to happen'. However, one could hook 
in such a way that the uninstall would succeed anyway, assuming that the 
function pointer within the original installed protocol is writeable.


My question is: was the above is roughly what you were thinking of, that 
might cause the assert to fail, or, if not, if you had the time to give 
a very brief sketch of what else it might be (just a plausible, very 
rough example)? Certainly not saying you're wrong, just that it would be 
helpful (to me!) to understand what sort of thing you were thinking of!


I don't have a specific use case in mind for why someone might want to 
have opened this particular protocol in a way that would subsequently 
cause UninstallMultipleProtocolInterfaces() to fail (e.g. opening with 
BY_CHILD_CONTROLLER attributes).  Just that, as a general rule, there 
exists a design flaw in the UEFI specification that means that 
operations that should have been chosen at the design stage to be 
conceptually impossible to fail (such as freeing memory or uninstalling 
protocols) are instead allowed to return a failure status.


This design issue manifests itself as extremely unreliable behaviour on 
the removal or shutdown paths of many UEFI drivers.  For example: many 
drivers will simply deadlock the system if disconnected from their 
underlying controllers (e.g. via the UEFI shell "disconnect" command).


In the case of UninstallMultipleProtocolInterfaces(), the failure mode 
is particularly problematic since the specification dictates that the 
firmware must do the absolutely worst thing possible by *reinstalling* 
any protocol instances that it had managed to uninstall, and 
consequently retriggering driver Start() method calls.  This generally 
leads to chaos and confusion (and use-after-free bugs that could 
probably be fairly easily extended to obtain a Secure Boot exploit).


There's nothing that you really need to do specifically in HttpBootDxe 
to work around this design flaw.  But it's definitely worth removing the 
unjustified ASSERT(), since that ASSERT() may cause a crash in a system 
that could otherwise continue to operate successfully.


Hope that helps,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118048): https://edk2.groups.io/g/devel/message/118048
Mute This Topic: https://groups.io/mt/105368366/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] NetworkPkg/HttpBootDxe: Correctly uninstall HttpBootCallbackProtocol

[Michael Brown]

On 06/04/2024 16:53, Mike Beaton wrote:

The existing uninstall call was passing the wrong handle (parent object,
not the correct child object) and additionally passing the address
of a pointer to the interface to be removed rather than the pointer
itself, so always failed with EFI_NOT_FOUND. After altering these, we
add an ASSERT which confirms that the modified uninstall is succeeding.

Cc: Maciej Rabeda 
Cc: Jiaxin Wu 
Cc: Siyuan Fu 
Signed-off-by: Mike Beaton 
---
  NetworkPkg/HttpBootDxe/HttpBootImpl.c | 21 -
  1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/NetworkPkg/HttpBootDxe/HttpBootImpl.c 
b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
index b4c61925b9..100b721ad4 100644
--- a/NetworkPkg/HttpBootDxe/HttpBootImpl.c
+++ b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
@@ -77,12 +77,23 @@ HttpBootUninstallCallback (
IN HTTP_BOOT_PRIVATE_DATA  *Private
)
  {
+  EFI_STATUS  Status;
+  EFI_HANDLE  ControllerHandle;
+
if (Private->HttpBootCallback == >LoadFileCallback) {
-gBS->UninstallProtocolInterface (
-   Private->Controller,
-   ,
-   >HttpBootCallback
-   );
+if (!Private->UsingIpv6) {
+  ControllerHandle = Private->Ip4Nic->Controller;
+} else {
+  ControllerHandle = Private->Ip6Nic->Controller;
+}
+
+Status = gBS->UninstallProtocolInterface (
+ControllerHandle,
+,
+Private->HttpBootCallback
+);
+ASSERT_EFI_ERROR (Status);


This assertion is not necessarily safe.  Uninstallation of protocol 
interfaces is allowed to fail under the UEFI model.  (This is arguably 
insane, but that's a separate discussion.)  This could therefore 
potentially result in a perfectly valid sequence of events leading to an 
ASSERT() failure.


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117469): https://edk2.groups.io/g/devel/message/117469
Mute This Topic: https://groups.io/mt/105368366/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile

[Michael Brown]

Reviewed-by: Michael Brown 

Thanks,

Michael

On 01/03/2024 02:54, Zhou Jianfeng wrote:

Add volatile qualifier to page table related variable to prevent
compiler from optimizing away the variables which may lead to
unexpected result.

Signed-off-by: Zhou Jianfeng 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
Cc: Pedro Falcato 
Cc: Zhang Di 
Cc: Tan Dun 
Cc: Michael Brown 
---
  .../Library/CpuPageTableLib/CpuPageTableMap.c | 36 +--
  1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c 
b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index c4e46a6d74..0a380a04cb 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -20,17 +20,17 @@
  **/
  VOID
  PageTableLibSetPte4K (
-  IN OUT IA32_PTE_4K *Pte4K,
-  IN UINT64  Offset,
-  IN IA32_MAP_ATTRIBUTE  *Attribute,
-  IN IA32_MAP_ATTRIBUTE  *Mask
+  IN OUT volatile IA32_PTE_4K  *Pte4K,
+  IN UINT64Offset,
+  IN IA32_MAP_ATTRIBUTE*Attribute,
+  IN IA32_MAP_ATTRIBUTE*Mask
)
  {
IA32_PTE_4K  LocalPte4K;

LocalPte4K.Uint64 = Pte4K->Uint64;
if (Mask->Bits.PageTableBaseAddressLow || 
Mask->Bits.PageTableBaseAddressHigh) {
-LocalPte4K.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + 
Offset) | (Pte4K->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);
+LocalPte4K.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + 
Offset) | (LocalPte4K.Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);
}

if (Mask->Bits.Present) {
@@ -94,17 +94,17 @@ PageTableLibSetPte4K (
  **/
  VOID
  PageTableLibSetPleB (
-  IN OUT IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  *PleB,
-  IN UINT64 Offset,
-  IN IA32_MAP_ATTRIBUTE *Attribute,
-  IN IA32_MAP_ATTRIBUTE *Mask
+  IN OUT volatile IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  *PleB,
+  IN UINT64  Offset,
+  IN IA32_MAP_ATTRIBUTE  *Attribute,
+  IN IA32_MAP_ATTRIBUTE  *Mask
)
  {
IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;

LocalPleB.Uint64 = PleB->Uint64;
if (Mask->Bits.PageTableBaseAddressLow || 
Mask->Bits.PageTableBaseAddressHigh) {
-LocalPleB.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + 
Offset) | (PleB->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
+LocalPleB.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + 
Offset) | (LocalPleB.Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
}

LocalPleB.Bits.MustBeOne = 1;
@@ -171,11 +171,11 @@ PageTableLibSetPleB (
  **/
  VOID
  PageTableLibSetPle (
-  IN UINTN   Level,
-  IN OUT IA32_PAGING_ENTRY   *Ple,
-  IN UINT64  Offset,
-  IN IA32_MAP_ATTRIBUTE  *Attribute,
-  IN IA32_MAP_ATTRIBUTE  *Mask
+  IN UINTNLevel,
+  IN OUT volatile IA32_PAGING_ENTRY   *Ple,
+  IN UINT64   Offset,
+  IN IA32_MAP_ATTRIBUTE   *Attribute,
+  IN IA32_MAP_ATTRIBUTE   *Mask
)
  {
if (Level == 1) {
@@ -195,9 +195,9 @@ PageTableLibSetPle (
  **/
  VOID
  PageTableLibSetPnle (
-  IN OUT IA32_PAGE_NON_LEAF_ENTRY  *Pnle,
-  IN IA32_MAP_ATTRIBUTE*Attribute,
-  IN IA32_MAP_ATTRIBUTE*Mask
+  IN OUT volatile IA32_PAGE_NON_LEAF_ENTRY  *Pnle,
+  IN IA32_MAP_ATTRIBUTE *Attribute,
+  IN IA32_MAP_ATTRIBUTE *Mask
)
  {
IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;
--
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116230): https://edk2.groups.io/g/devel/message/116230
Mute This Topic: https://groups.io/mt/104661494/1770615
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [mc...@ipxe.org]
-=-=-=-=-=-=-=-=-=-=-






-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116236): https://edk2.groups.io/g/devel/message/116236
Mute This Topic: https://groups.io/mt/104661494/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

[Michael Brown]

On 01/03/2024 09:33, Paolo Bonzini wrote:

On Fri, Mar 1, 2024 at 10:27 AM Michael Brown  wrote:

It's possible that it doesn't matter.  The new logic will effectively
mean that RestoreTPL() will restore not only the TPL but also the
interrupts-enabled state to whatever existed at the time of the
corresponding RaiseTPL().


Right: that's what my comment says

+  // However, when the handler calls RestoreTPL
+  // before returning, we want to keep interrupts disabled.  This
+  // restores the exact state at the beginning of the handler,
+  // before the call to RaiseTPL(): low TPL and interrupts disabled.

but indeed it applies beyond interrupt handlers. It might even be a bugfix.


Right.  I'm leaning towards treating this as a bugfix: essentially 
tightening up the semantics of RestoreTPL() to mean:


- any callbacks in the range OldTpl < Tpl < gEfiCurrentTpl will be 
dispatched with interrupts unconditionally enabled


- the TPL will be restored to OldTpl

- the interrupt state will be restored to the value it had when the TPL 
was last raised from OldTpl


It feels as though this should be able to be cleanly modelled with a 
single global state array


  BOOLEAN mSavedInterruptState[TPL_HIGH_LEVEL]

(or possibly a bitmask, though using the array avoids having to disable 
interrupts just to write a value).


I still need to think through the subtleties, to make sure it could cope 
with pathological edge cases such as


  OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);

  ...

  gBS->RestoreTPL (OldTpl);
  gBS->RestoreTPL (OldTpl);

or

  OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL - 1);
  gBS->RaiseTPL (TPL_HIGH_LEVEL);

  ..

  gBS->RestoreTPL (OldTpl);

I think that at least one of the above pathological usage patterns would 
break the existing mInterruptedTplMask patches, since they currently 
clear state in RestoreTPL() and so will not correctly handle a duplicate 
call to RestoreTPL().


I'll try to get a patch put together over the weekend.

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116232): https://edk2.groups.io/g/devel/message/116232
Mute This Topic: https://groups.io/mt/104642317/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

[Michael Brown]

On 01/03/2024 08:37, Paolo Bonzini wrote:

So I am starting to see more and more similarities between the two
approaches.  I went a step further with fresh mind, removing the while
loop... and basically reinvented your and Michael's patch. :) The only
difference in the logic is a slightly different handling of
mInterruptedTplMask in CoreRestoreTpl(), which is a bit safer in my
case.

However, my roundabout way of getting to the same patch resulted in
very different comments. Personally, I found the large text at the
head of mInterruptedTplMask a bit too much, and the ones inside the
function too focused on "how" and not "why". Maybe it's my exposure to
NestedInterruptTplLib, but I find that a much smaller text can achieve
the same purpose, by explaining the logic instead of the individual
steps.

My version is attached, feel free to reuse it (either entirely or
partially) for a hypothetical v2. Apologies to you and Mike K for the
confusion!


I much prefer this version of the patch, because the explanations are 
easier to follow and to reason about.



Minor point (applies to both your and Ray's versions):

- The use of gCpu->GetInterruptState() vs CoreSetInterruptState() is 
inconsistent.  It feels as though CoreGetInterruptState() ought to 
exist.  I assume that the PI spec defines whether interrupts are enabled 
or disabled prior to the CPU arch protocol being installed, so it should 
be possible to have a well-defined return value from 
CoreGetInterruptState().



Major point:

There is an implicit assumption that if interrupts are disabled when 
RaiseTPL() is called then we must have been called from an interrupt 
handler.  How sure are we that this assumption is correct?


It's possible that it doesn't matter.  The new logic will effectively 
mean that RestoreTPL() will restore not only the TPL but also the 
interrupts-enabled state to whatever existed at the time of the 
corresponding RaiseTPL().  Maybe this is what we want?  If so, then we 
should probably phrase the comments in these terms instead of in terms 
of being called from an interrupt handler.



Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116227): https://edk2.groups.io/g/devel/message/116227
Mute This Topic: https://groups.io/mt/104642317/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

[Michael Brown]

On 29/02/2024 19:26, Michael D Kinney wrote:

I think one advantage of this new proposal is to prevent
an extra level of nesting and use of stack resources in
that extra level.


I think that's a negligible benefit.  In the scenario as I outlined for 
NestedInterruptTplLib, there is potentially one more interrupt stack 
frame, but in that case the inner handler can only consume a small and 
fixed amount of stack space since it will not call RestoreTPL() (and so 
will not dispatch events, etc).


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116188): https://edk2.groups.io/g/devel/message/116188
Mute This Topic: https://groups.io/mt/104642317/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

[Michael Brown]

On 29/02/2024 19:09, Michael D Kinney wrote:

"When the DXE Foundation is notified that the EFI_CPU_ARCH_PROTOCOL has
been installed, then the full version of the Boot Service RestoreTPL()
can be made available.  When an attempt is made to restore the TPL
level
to level below EFI_TPL_HIGH_LEVEL, then the DXE Foundation should use
the services of the EFI_CPU_ARCH_PROTOCOL to enable interrupts."


I would claim that this spec is perhaps incomplete in this area that
that incomplete description is what allows the window for interrupt
nesting to occur.  This language is correct for UEFI code that calls
Raise/Restore TPL once the CPU Arch Protocol is available.  It does
not cover the required behavior to prevent nesting when processing
a timer interrupt.  This could be considered a gap in the UEFI/PI
spec content.


I think it's important that we don't phrase it as preventing interrupt 
nesting.  The UEFI design *requires* that nested interrupts be allowed 
to happen, since callbacks at TPL_CALLBACK are allowed to wait for 
events at TPL_NOTIFY, and this can't happen without the existence of 
nested interrupts.


The problem is not nested interrupts per se: the problem is the 
potential for unlimited stack consumption.



- How does the proposed patch react to an interrupt occurring
(illegally) at TPL_HIGH_LEVEL (as happens with some versions of
Windows)?  As far as I can tell, it will result in mInterruptedTplMask
having bit 31 set but never cleared.  What impact will this have?


This behavior could potentially break any UEFI code that sets TPL to
TPL_HIGH_LEVEL as a lock, which can then cause any number of
undefined behaviors.  I am curious of you have a way to reproduce
this failure for testing purposed.

I would agree that any proposed change needs to comprehend this
Scenario if it can be reproduced with shipping OS images.


https://bugzilla.redhat.com/show_bug.cgi?id=2189136 was the original bug 
report in which it was discovered that Windows 11 would call 
RaiseTPL(TPL_HIGH_LEVEL) and then enable interrupts using the STI 
instruction.


It would be interesting to hear from anyone at Microsoft as to why this 
happens!



- How does the proposed patch react to potentially mismatched
RaisedTPL()/RestoreTPL() calls (e.g. oldTpl = RaiseTPL(TPL_CALLBACK)
followed by RaiseTPL(TPL_NOTIFY) followed by a single
RestoreTPL(oldTpl))?


The proposed patch only changes behavior when processing a timer
interrupt.  I do not think there would be any changes in behavior
for UEFI code that makes that sequence of calls.


The patch affects all callers of RaiseTPL() and RestoreTPL().  Given 
that it creates a new piece of shared state (mInterruptedTplMask), I'd 
like to see some kind of proof that it can correctly handle an arbitrary 
sequence of calls from unknown third-party code.


For example: consider an interrupt at TPL_APPLICATION with a third-party 
timer interrupt handler that does something like:


  OldTpl = RaiseTPL (TPL_HIGH_LEVEL);

  ... send EOI, call timer tick function, etc ...

  if (OldTpl < TPL_NOTIFY) {
RestoreTPL (TPL_NOTIFY);
... do some weird OEM-specific thing ...
  }

  RestoreTPL ( OldTpl );

This is arguably a valid sequence of calls to RaiseTPL()/RestoreTPL(). 
With the patch as-is, mInterruptedTplMask will have flagged the 
TPL_APPLICATION bit but not the TPL_NOTIFY bit, and so the call to 
RestoreTPL(TPL_NOTIFY) *will* re-enable interrupts, which is against the 
intention of the patch.


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116187): https://edk2.groups.io/g/devel/message/116187
Mute This Topic: https://groups.io/mt/104642317/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

[Michael Brown]

On 29/02/2024 19:04, Paolo Bonzini wrote:

On 2/29/24 14:02, Ray Ni wrote:
In the end, it will lower the TPL to TPL_APPLICATION with interrupt 
enabled.


However, it's possible that another timer interrupt happens just in 
the end

of RestoreTPL() function when TPL is TPL_APPLICATION.


How do non-OVMF platforms solve the issue?  Do they just have the same 
bug as in https://bugzilla.tianocore.org/show_bug.cgi?id=4162 ?


Yes, they have that bug (or one of the bugs mentioned in that long 
discussion, depending on which particular implementation choices have 
been made).


The design of NestedInterruptTplLib is that each nested interrupt must 
increase the TPL, but if I understand correctly there is a hole here:


   //
   // Call RestoreTPL() to allow event notifications to be
   // dispatched.  This will implicitly re-enable interrupts.
   //
   gBS->RestoreTPL (InterruptedTPL);

   //
   // Re-disable interrupts after the call to RestoreTPL() to ensure
   // that we have exclusive access to the shared state.
   //
   DisableInterrupts ();

because gBS->RestoreTPL will unconditionally enable interrupts if 
InterruptedTPL < TPL_HIGH_LEVEL.


There's no hole there.

Yes, interrupts will be temporarily reenabled, but the whole function of 
NestedInterruptTplLib is to safely allow for this window in which 
interrupts have been (annoyingly) enabled by RestoreTPL().


If another interrupt *does* occur within that window, the inner 
interrupt handler will call NestedInterruptRestoreTPL(), which will take 
the code path leading to the "DEFERRAL INVOCATION POINT", and will 
therefore *not* call RestoreTPL() within that stack frame.  The inner 
interrupt returns at TPL_HIGH_LEVEL with interrupts still disabled, and 
execution is therefore guaranteed to immediately reach the "DEFERRAL 
RETURN POINT" in the outer interrupt handler.  The deferred call to 
RestoreTPL() is then safely executed in the context of the outer 
interrupt handler (i.e. with zero increase in stack usage, hence a 
guarantee of no stack overflow).


See the comments in the code for further details - I made them fairly 
extensive.  :)


If possible, the easiest solution would be to merge 
NestedInterruptTplLib into Core DXE.


The question with that approach would be how to cleanly violate the 
abstraction layer that separates the timer interrupt handler (existing 
in a separate DXE driver executable) from the implementation of 
CoreRestoreTplInternal() (existing in core DXE and not exposed via the 
boot services table).


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116185): https://edk2.groups.io/g/devel/message/116185
Mute This Topic: https://groups.io/mt/104642317/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

[Michael Brown]

On 29/02/2024 16:43, Kinney, Michael D wrote:

Hi Michael,

Can you provide a pointer to the UEFI Spec statement this breaks?


II-9.7.1.3 RestoreTPL():

"When the DXE Foundation is notified that the EFI_CPU_ARCH_PROTOCOL has 
been installed, then the full version of the Boot Service RestoreTPL() 
can be made available.  When an attempt is made to restore the TPL level 
to level below EFI_TPL_HIGH_LEVEL, then the DXE Foundation should use 
the services of the EFI_CPU_ARCH_PROTOCOL to enable interrupts."


I suspect this is sufficient to veto the proposed design, though we 
could argue that the loosely worded "should" is technically not "must".



If we still want to proceed with this design, then I have several other 
questions:


- How does the proposed patch react to an interrupt occurring 
(illegally) at TPL_HIGH_LEVEL (as happens with some versions of 
Windows)?  As far as I can tell, it will result in mInterruptedTplMask 
having bit 31 set but never cleared.  What impact will this have?


- How does the proposed patch react to potentially mismatched 
RaisedTPL()/RestoreTPL() calls (e.g. oldTpl = RaiseTPL(TPL_CALLBACK) 
followed by RaiseTPL(TPL_NOTIFY) followed by a single RestoreTPL(oldTpl))?



I believe the proposed patch is attempting to establish a new invariant 
as follows:


Once an interrupt has occured at a given TPL, then that *TPL* is 
conceptually considered to be in an "interrupted" state.  The *only* 
thing that can clear this "interrupted" state from the TPL is to return 
from the interrupt handler.


Note that this conceptual definition does not perfectly align with the 
bit flags in mInterruptedTplMask, since those bits will necessarily be 
set only some time after the interrupt occurs, and will have to be 
cleared before returning from the interrupt.  However, it is the 
conceptual definition that is relevant to the invariant.


The new invariant is that no code may execute at an "interrupted" TPL 
with interrupts enabled.  It is legitimate for code to raise to a higher 
TPL and to enable interrupts while there, and it is legitimate for code 
to execute in an "interrupted" TPL with interrupts disabled, but it is 
not legitimate for any code to reenable interrupts while still at an 
"interrupted" TPL.


It would be good to call out this invariant explicitly, so that authors 
of interrupt handlers are aware of the restrictions.  It would also 
clarify some of the logic (e.g. it provides the reason why interrupts 
must be disabled before lowering gEfiCurrentTpl in CoreRestoreTpl()).


It's also generally easier to reason about a stated invariant than to 
extrapolate from a list of complicated examples.


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116178): https://edk2.groups.io/g/devel/message/116178
Mute This Topic: https://groups.io/mt/104642317/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts

[Michael Brown]

On 29/02/2024 13:02, Ni, Ray wrote:

A ideal solution is to not keep the interrupt disabled when
RestoreTPL(TPL_HIGH -> not TPL_HIGH) is executed in the timer interrupt
context because the interrupt handler will re-enable the interrupt with
arch specific instructions (e.g.: IRET for x86).

The patch introduces mInterruptedTplMask which tells RestoreTPL() if
it's called in the interrupt context and whether it should defer enabling
the interrupt.


NACK.  This breaks the specification-defined behaviour for RestoreTPL().

What guarantees do we have that there is no code anywhere in the world 
that relies upon RestoreTPL() unconditionally re-enabling interrupts.


I also find this code substantially harder to follow than 
NestedInterruptTplLib (which does not break any specified behaviour).


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116168): https://edk2.groups.io/g/devel/message/116168
Mute This Topic: https://groups.io/mt/104642317/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile

[Michael Brown]

On 23/02/2024 15:12, Zhou, Jianfeng wrote:

While it may well cause the compiler to generate less optimised code, there is 
absolutely no way that this volatile declaration on a local stack variable can 
possibly change the outcome of the code.
There can never be any meaningful side-effects from reading or writing a stack 
variable.
I would suggest dropping the volatile on LocalPte4K, since its *only* possible 
impact is to confuse a future reader of the code.


The change is for preventing compiler from optimizing.
As a temporary variable,  LocalPte4K may be replaced by function parameter 
Pte4K.


No, it can't.  If Pte4K is marked as a volatile pointer, then the 
compiler is not allowed to unilaterally decide to treat it as a 
non-volatile pointer.



In this case, code like "LocalPte4K.Bits.Present = Attribute->Bits.Present" may 
lead to unexpected result, as it is not atomic. Assembly code look like:
mov eax, [r8]
and dword [rcx], 0xfffe  // this instruction clear the present bit and 
may leads to unexpected result.
and eax, 0x1
or [rcx], eax


Please test with Pte4K marked as volatile and LocalPte4K marked as 
non-volatile.  If you can still generate assembly code that writes to 
*Pte4K more than once, then that would be a serious compiler bug.



As a separate note, I would also suggest removing the unnecessary second 
read through Pte4K, since once Pte4K is marked as volatile the compiler 
will generate an extra read from that address:


--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -30,7 +30,7 @@ PageTableLibSetPte4K (

   LocalPte4K.Uint64 = Pte4K->Uint64;
   if (Mask->Bits.PageTableBaseAddressLow || 
Mask->Bits.PageTableBaseAddressHigh) {
-LocalPte4K.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS 
(Attribute) + Offset) | (Pte4K->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);
+LocalPte4K.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS 
(Attribute) + Offset) | (LocalPte4K.Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);

   }

   if (Mask->Bits.Present) {

Michael




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115888): https://edk2.groups.io/g/devel/message/115888
Mute This Topic: https://groups.io/mt/104524857/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile

[Michael Brown]

On 22/02/2024 08:41, Zhou Jianfeng wrote:

--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -20,13 +20,13 @@
  **/
  VOID
  PageTableLibSetPte4K (
-  IN OUT IA32_PTE_4K *Pte4K,
-  IN UINT64  Offset,
-  IN IA32_MAP_ATTRIBUTE  *Attribute,
-  IN IA32_MAP_ATTRIBUTE  *Mask
+  IN OUT volatile IA32_PTE_4K  *Pte4K,
+  IN UINT64Offset,
+  IN IA32_MAP_ATTRIBUTE*Attribute,
+  IN IA32_MAP_ATTRIBUTE*Mask
)
  {
-  IA32_PTE_4K  LocalPte4K;
+  volatile IA32_PTE_4K  LocalPte4K;


While it may well cause the compiler to generate less optimised code, 
there is absolutely no way that this volatile declaration on a local 
stack variable can possibly change the outcome of the code.


There can never be any meaningful side-effects from reading or writing a 
stack variable.


I would suggest dropping the volatile on LocalPte4K, since its *only* 
possible impact is to confuse a future reader of the code.



-  IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;
+  volatile IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;


Same comment.


-  IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;
+  volatile IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;


Same comment.

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115883): https://edk2.groups.io/g/devel/message/115883
Mute This Topic: https://groups.io/mt/104524857/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Problem Booting Starlingx On PXE

[Michael Brown]

On 19/02/2024 06:44, Hamit Can Karaca wrote:
I am having problem while booting Starlingx on iPXE on UEFI. It says 
malformed binary and does not allow me to boot afterwards. What should I 
do in BIOS to solve this problem?


I can boot Starlingx on other platforms using OEM BIOS.

Our platform is Intel Xeon Purley server. We are using EDK2 BIOS. I have 
added iPXE.efi to fdf file to enable PXE boot.


Any help or suggestions are appreciated.


From your screenshot: iPXE is starting up with no problems, is 
successfully completing DHCP and downloading the file bootx64.efi as 
directed by your DHCP server.


This downloaded file is the UEFI shim, and the error messages you are 
seeing are coming from shim (not from iPXE):


  https://github.com/rhboot/shim/blob/main/pe.c#L264-L286

It appears that shim does not think that your next-stage binary is 
valid.  Unfortunately shim does not provide any information about what 
this next stage binary is (beyond the message "Fetching Netboot Image"), 
and so there is no further diagnostic information available in your 
screenshot.



Using shim's own netboot support in this way is strongly deprecated. 
Please instead use iPXE to load the kernel, initrd, and (if required) 
shim directly.  Documentation is available at


  https://ipxe.org/cmd/shim

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115587): https://edk2.groups.io/g/devel/message/115587
Mute This Topic: https://groups.io/mt/104442316/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] RFC: Another solution to the nested interrupt issue

[Michael Brown]

On 25/01/2024 15:06, Ni, Ray wrote:

I agree with you about the above PI spec clarifications.
Would you like to write a PI spec ECR?


I am not a UEFI Forum member, so I don't think I have standing to do 
this.  (I also don't have the spare time to do this for free, sorry.)



But I do not think the PI spec version stored in the PI system table needs to
reflect whether a DxeCore implementation follows the clarification.

Since the DxeCore::CoreTimerTick() implementation raises TPL to HIGH in the 
very first version created
in more than 10 years ago, I think it's safe for TimerInterruptHandler() 
assumes CoreTimerTick() will
raise TPL to HIGH so that TimerInterruptHandler() does not need to raise TPL to 
HIGH.
(I agree changing the spec version is the most correct way if we review the 
problem in a very theorical way.)


I don't see any call to RaiseTPL() in the current version of 
CoreTimerTick(), unless this is implicit in the use of CoreAcquireLock()?



I really want to keep the UEFI world simple with the bug fixed.


I definitely agree with this aim!  :)


(The cost is assumption on existing DxeCore::CoreTimerTick().)


For me, the problem is that we can't assume that it's the EDK2 
implementation of CoreTimerTick() that is being used.  It's perfectly 
legitimate for an implementation to not use EDK2's DxeCore, and to 
provide a NotifyFunction that *requires* being called with the TPL 
already raised to TPL_HIGH_LEVEL.


If we're not worried about maintaining conformance to the published 
specifications, then there are several very breaking changes I'd like to 
make, starting with the USB I/O protocols.  :)


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114427): https://edk2.groups.io/g/devel/message/114427
Mute This Topic: https://groups.io/mt/103950154/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] RFC: Another solution to the nested interrupt issue

[Michael Brown]

On 25/01/2024 13:54, Ni, Ray wrote:

I don't disagree with the approach, but it does break the API as per the
UEFI PI specification (version 1.8 section II-12.10), and so this is not
something that can just be dropped in as an EDK2 code change.


You think that the TimerInterruptHandler() doesn't raise/restore TPL
which would violate the PI spec as PI spec says " NotifyFunction ... executes at 
EFI_TPL_HIGH_LEVEL."?

I do not think the PI spec requires TimerInterruptHandler() raises TPL
to HIGH before invoking NotifyFunction. It just means the NotifyFunction
will execute at TPL_HIGH.


If the caller is not supposed to raise TPL to TPL_HIGH_LEVEL before 
calling NotifyFunction, then the statement "This function executes at 
EFI_TPL_HIGH_LEVEL" in the PI specification is meaningless.  There is no 
other possible interpretation besides "the caller must raise TPL to 
TPL_HIGH_LEVEL before calling this function".



If you review HpetTimer driver, it does not raise TPL to HIGH before
invoking NotifyFunction.


That would then be a bug in HpetTimer, which ought to be fixed.  If 
HpetTimer were to be used on a platform where the NotifyFunction 
correctly assumes that it is called at TPL_HIGH_LEVEL and does something 
that would break at a lower level, then this could lead to undefined 
behaviour.



And I think implementing the DxeCore changes as attached does not
prevent the TimerInterruptHandler() from calling raise/restore TPL.


No, but a spec-conforming timer interrupt handler could not take 
advantage of the feature, because it would have to raise to 
TPL_HIGH_LEVEL before calling the NotifyFunction.  (Any raise/restore 
within the NotifyFunction would then have no effect.)



So, with the changes done in DxeCore, a timer driver could either
not raise/restore TPL in TimerInterruptHandler(), or it calls
NestedInterruptTplLib if it wants.


As a pure code change, I do agree that it solves the problem and it's a 
much simpler approach.  However, it is a breaking change to the 
specification and I think it would need be handled as such.


The minimal specification change I can think of that would make this 
possible would be to relax the wording on NotifyFunction in the next 
version of the PI specification to say that


* the NotifyFunction can be called at any TPL level

* the NotifyFunction will raise TPL to TPL_HIGH_LEVEL, restore TPL back 
to the original TPL before returning


* the NotifyFunction may re-enable interrupts during its execution, and 
that the caller must be prepared to be re-entered before NotifyFunction 
returns


* the timer interrupt must have been rearmed before calling NotifyFunction

* the NotifyFunction must guarantee that it never reaches a state in 
which the TPL has been restored to the original level with CPU 
interrupts enabled.


This would be backwards compatible with the existing behaviour.  A 
caller written to the current specification would call NotifyFunction at 
TPL_HIGH_LEVEL and so any RaiseTPL/RestoreTPL done within a 
NotifyFunction complying to the new specification would be a no-op anyway.


A caller written to the new specification would have to check the 
supported version of the PI specification (which I assume is available 
in some system configuration table somewhere) to know that it was safe 
to call NotifyFunction without first raising to TPL_HIGH_LEVEL.


This approach would at least avoid the need for an ARCH2_PROTOCOL 
variant, which is potentially lower impact.


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114407): https://edk2.groups.io/g/devel/message/114407
Mute This Topic: https://groups.io/mt/103950154/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] RFC: Another solution to the nested interrupt issue

[Michael Brown]

On 25/01/2024 07:57, Ni, Ray wrote:
This mail is to bring another approach to solve the stack-overflow due 
to nested interrupts. Michael solved this problem in OVMF through 
NestedInterruptTplLib.


I made a draft patch as attached “DxeCore.diff”. The patch simply to 
avoid the interrupt in enable state when TPL is dropped to the 
interrupted TPL. The interrupt will be enabled later through “IRET”.


I don't disagree with the approach, but it does break the API as per the 
UEFI PI specification (version 1.8 section II-12.10), and so this is not 
something that can just be dropped in as an EDK2 code change.


There are no version number fields or spare parameters (that I can see) 
that could be used within EFI_TIMER_ARCH_PROTOCOL to allow for this 
change of semantics, so this would have to be a breaking change.


I think you'd need to first define an EFI_TIMER_ARCH2_PROTOCOL with 
different semantics for RegisterHandler(), but I'm not a UEFI Forum 
member and I have no idea what the bureaucratic process is for pushing 
through this kind of change.  I suspect we'd end up having to support 
both protocol variants (which means added maintenance burden).


It might be plausible instead to extend EFI_CPU_ARCH_PROTOCOL in a 
backwards-compatible way.  The InterruptType parameter to 
RegisterInterruptHandler() is already handled in a very casual way: the 
UEFI spec defines a limited range of possible types (0-19 for IA32/X64) 
for EFI_EXCEPTION_TYPE, but the code in LocalApicTimerDxe.c already 
treats it as meaning just an IRQ vector number by passing the value 
LOCAL_APIC_TIMER_VECTOR=32.


(Incidentally, the code comments in MdePkg/Include/Protocol/Cpu.h are 
completely wrong - the description of the InterruptType parameter seems 
to have been copied from the description of the State parameter in 
EFI_CPU_GET_INTERRUPT_STATE.)


The extension I'm thinking of could be:

- Clean up the definition of EFI_EXCEPTION_TYPE to clarify that it 
represents a CPU interrupt vector number (if this is how all current 
architectures actually use it).


- Extend the definition of EFI_EXCEPTION_TYPE to include a high bit flag 
that can be ORed on to the exception type, e.g.


  #define EFI_EXCEPTION_TYPE_TPL_HIGH 0x0100

- The RegisterInterruptHandler() implementation could then treat this 
flag as meaning that the handler should be called via a wrapper that 
does RaiseTPL(TPL_HIGH_LEVEL) before calling the handler, and the 
equivalent of your CoreRestoreTplDeferEnableInterrupt() after the 
handler returns, i.e. that the handler is automatically called at 
TPL_HIGH_LEVEL.


This would not make any breaking change to the definitions of 
EFI_TIMER_ARCH_PROTOCOL or EFI_CPU_ARCH_PROTOCOL, and so would not 
require an ARCH2_PROTOCOL variant to be created.


I haven't tried implementing this to see if it's viable, so I've 
probably missed something crucial.


Personally I would go for moving NestedInterruptTplLib to MdeModulePkg 
since this can be done today, whereas anything involving a spec change 
will take a lot more time, but that's not my call.


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114401): https://edk2.groups.io/g/devel/message/114401
Mute This Topic: https://groups.io/mt/103950154/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib

[Michael Brown]

On 23/01/2024 16:55, Laszlo Ersek wrote:

+  ///
+  /// Number of self-tests performed.
+  ///
+  UINTN  SelfTestCount;
  } NESTED_INTERRUPT_STATE;
  
  /**


I suggest that the new field be UINT32. The (exclusive) limit is a
32-bit PCD. Making the counter (potentially) wider than the limit is not
useful, but it's also a bit of a complication for the debug messages
(see below).


Great suggestion, thanks!


+///
+/// Perform a limited number of self-tests on the first few
+/// invocations.
+///
+if ((IsrState->DeferredRestoreTPL == FALSE) &&


This comment applies to several locations in the patch:

BOOLEANs should not be checked using explicit "== TRUE" and "== FALSE"
operators / comparisons; they should only be evaluated in their logical
contexts:


We've had this conversation before :)

  https://edk2.groups.io/g/devel/message/104369

I personally find that the "!" operators get visually lost in the EDK2 
coding style with its very long variable and function names.  That said, 
I'm happy to omit all of the explicit comparisons, but I should then add 
a precursor patch that changes the existing code style first.  Thoughts?



+VOID
+NestedInterruptSelfTest (
+  IN NESTED_INTERRUPT_STATE  *IsrState
+  )
+{
+  UINTN SelfTestCount;
+  UINTN TimeOut;


Did this pass the uncrustify check? I think uncrustify would insist on
inserting two spaces here.

For running uncrustify locally:


You are right, and thank you for reminding me how to run the EDK2 
version of uncrustify.  I've fixed up everything it identified.



+  // This error represents a bug in the platform-specific timer
+  // interrupt handler.
+  //
+  DEBUG ((
+DEBUG_ERROR,
+"Nested interrupt self-test %u/%u failed: no nested interrupt\n",
+SelfTestCount,
+NUMBER_OF_SELF_TESTS
+));
+  ASSERT (FALSE);
+}


I'd prefer something stronger than just ASSERT (FALSE) here, but -- per
previous discussion -- we don't have a generally accepted "panic" API
yet, and CpuDeadLoop() is not suitable for all platforms, so this should do.

With my trivial comments addressed:

Acked-by: Laszlo Ersek 

Comment on the general idea: I much like that the self-test is active on
every boot (without high costs).


Thank you!


Side idea: technically we could merge the first two patches in
separation (pending MdeModulePkg maintainer approval), and then consider
the last three patches as new improvements (possibly needing longer
review). This kind of splitting has both advantages and disadvantages;
the advantage is that the code movement / upstreaming to MdeModulePkg is
not blocked by (somewhat) unrelated discussion. The disadvantages are
that more admin work is needed (more posting, and more PRs), and that
patches in the series that one might consider to belong together will
fly apart in the git history. So I just figured I'd raise the option.


I'm happy to work either way, and shall await instruction.

In the absence of any instruction to the contrary, I'll send out a v4 
tomorrow with everyone's suggestions and tags included, but without the 
above-mentioned precursor patch to remove the boolean comparisons.


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114227): https://edk2.groups.io/g/devel/message/114227
Mute This Topic: https://groups.io/mt/103911608/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 5/5] MdeModulePkg: Extend NestedInterruptTplLib to support Arm CPUs

[Michael Brown]

On 23/01/2024 17:10, Laszlo Ersek wrote:

Other than that, the patch looks great to me; I'd only propose one
(superficial) improvement:

rather than include , scavenge

#ifdef MDE_CPU_ARM
#include 
#elif defined (MDE_CPU_AARCH64)
#include 
#endif

from it.

Reasons:

(a) Those are the headers that directly provide the macros we need, no
need to include the rest of ArmLib.h. (Listing ArmPkg/ArmPkg.dec in the
Packages section of the INF file will make these more direct #include
directives work, too.)

(b) Including  kind of de-synchronizes the #include
directives in the C source from the [LibraryClasses] section in the INF
file. Generally there should be a 1-to-1 match -- we should include the
declarations of variables and functions for exactly those libraries that
we link against. There are two exceptions (that I can think of at once):
when we only want macros from a lib class header, or when we include a
lib class header because we are implementing an instance for that lib
class (i.e., we're providing, not consuming, the definitions for the
header-declared variables and functions). In this case, neither seems to
apply, this is not an ArmLib instance (= implementation), and the macros
we need don't actually come from ArmLib.h.

Acked-by: Laszlo Ersek 


Will do, thanks!

Michael




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114225): https://edk2.groups.io/g/devel/message/114225
Mute This Topic: https://groups.io/mt/103911611/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 3/5] MdeModulePkg: Do nothing on NestedInterruptRestoreTPL(TPL_HIGH_LEVEL)

[Michael Brown]

On 23/01/2024 16:32, Laszlo Ersek wrote:

On 1/23/24 16:31, Michael Brown wrote:

At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI
specification) and so we should never encounter a situation in which
an interrupt occurs at TPL_HIGH_LEVEL.

Restoring TPL to TPL_HIGH_LEVEL is always a no-op.  Return immediately
from NestedInterruptRestoreTPL(TPL_HIGH_LEVEL), so that we do not need
to consider the effect of this possible invariant violation on the
remainder of the logic.


Feels like the handling logic might as well be "panic" here (except edk2
does not have a central panic API that's suitable for all platforms). I
probably missed the previous discussion that led to this patch. Either
way, it seems reasonable.

Acked-by: Laszlo Ersek 


Thank you.  We can't panic because there are some bootloaders (*cough* 
Microsoft *cough*) that illegally call RaiseTPL(TPL_HIGH_LEVEL) and then 
even more illegally enable interrupts via STI.  Gerd tracked this down 
before, which lead to commit


  https://github.com/tianocore/edk2/commit/bee67e0c1

I found another way to trigger a RestoreTPL(TPL_HIGH_LEVEL) while I was 
testing the self-tests by deliberately breaking 
DisableInterruptsOnIret() to fail to disable interrupts.  This also 
induces a situation in which we end up at TPL_HIGH_LEVEL with interrupts 
enabled.


This ended up triggering an assertion (due to the invariant violation) 
in NestedInterruptRestoreTPL() before reaching the point in the 
self-test that would have reported a more meaningful error message.


Adding the bypass is justifiable on its own merits (as per the reasoning 
in the commit), and it avoids needing to add clutter to the complex 
logic in NestedInterruptRestoreTPL() just to ensure that the self-test 
fails on the desired ASSERT().


I decided against trying to explain all that in the commit message, but 
I can have a go if you think it needs to be captured.  :)


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114220): https://edk2.groups.io/g/devel/message/114220
Mute This Topic: https://groups.io/mt/103911606/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 5/5] MdeModulePkg: Extend NestedInterruptTplLib to support Arm CPUs

[Michael Brown]

On 23/01/2024 15:51, Ard Biesheuvel wrote:

On Tue, 23 Jan 2024 at 16:31, Michael Brown  wrote:

Add implementations of DisableInterruptsOnIret() for MDE_CPU_ARM and
MDE_CPU_AARCH64.  In both cases, the saved IRQs-disabled and
FIQs-disabled flags are set in the stored processor status register
(matching the behaviour of DisableInterrupts(), which also sets both
flags).


Thanks for this.

You can drop the FIQ bits, though: anything that can run Tianocore
will have the FIQs routed to the secure world, and all of the higher
level en/disable interrupt code only reasons about the IRQ line.


Thank you.

My commit message was incorrect, sorry: DisableInterrupts() in MdePkg 
does indeed disable only IRQs.  I was accidentally looking at 
ArmDisableInterrupts() in ArmPkg, which disables both IRQs and FIQs.


My (incomplete) understanding of Arm behaviour is that when an interrupt 
is raised, both IRQs and FIQs will be disabled by hardware.  When the 
interrupt handler ends up enabling interrupts during RestoreTPL(), only 
the IRQs will be re-enabled since EnableInterrupts() touches only the 
IRQ bit.  This state can persist for an extended period of time (e.g. 30 
seconds) if a callback at TPL_CALLBACK ends up waiting for further timer 
events.


Q1: Is it a problem to have this situation, with FIQs disabled for an 
extended period of time?


Q2: What happens with FIQs when there is no secure world (e.g. using 
ArmVirtQemu with "-M virt")?


Q3: Would you like me to add in the extra patch that modifies TimerDxe 
to use NestedInterruptTplLib?


Many thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114217): https://edk2.groups.io/g/devel/message/114217
Mute This Topic: https://groups.io/mt/103911611/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 5/5] MdeModulePkg: Extend NestedInterruptTplLib to support Arm CPUs

[Michael Brown]

The only architecture-specific portion of NestedInterruptTplLib is in
Iret.c, which must manipulate the interrupt stack frame such that the
return-from-interrupt instruction will not re-enable interrupts.  The
remaining logic in Tpl.c is architecture-agnostic.

Add implementations of DisableInterruptsOnIret() for MDE_CPU_ARM and
MDE_CPU_AARCH64.  In both cases, the saved IRQs-disabled and
FIQs-disabled flags are set in the stored processor status register
(matching the behaviour of DisableInterrupts(), which also sets both
flags).

Tested by patching ArmPkg's TimerDxe to use NestedInterruptTplLib and
verifying that ArmVirtQemu passes the NestedInterruptTplLib self-tests
for both ARM and AARCH64.

Cc: Ard Biesheuvel 
Signed-off-by: Michael Brown 
---
 MdeModulePkg/MdeModulePkg.dsc  |  2 +-
 .../NestedInterruptTplLib.inf  |  3 +++
 .../Library/NestedInterruptTplLib/Iret.c   | 18 ++
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
index 5b9ddfd26e75..4565b8e1b6e7 100644
--- a/MdeModulePkg/MdeModulePkg.dsc
+++ b/MdeModulePkg/MdeModulePkg.dsc
@@ -462,6 +462,7 @@ [Components.IA32, Components.X64, Components.AARCH64]
 
 [Components.IA32, Components.X64, Components.ARM, Components.AARCH64]
   MdeModulePkg/Library/BrotliCustomDecompressLib/BrotliCustomDecompressLib.inf
+  MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
   MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
   MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
   MdeModulePkg/Core/Dxe/DxeMain.inf {
@@ -526,7 +527,6 @@ [Components.IA32, Components.X64]
   MdeModulePkg/Library/TraceHubDebugSysTLib/BaseTraceHubDebugSysTLib.inf
   MdeModulePkg/Library/TraceHubDebugSysTLib/PeiTraceHubDebugSysTLib.inf
   MdeModulePkg/Library/TraceHubDebugSysTLib/DxeSmmTraceHubDebugSysTLib.inf
-  MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
 
 [Components.X64]
   MdeModulePkg/Universal/CapsulePei/CapsuleX64.inf
diff --git 
a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf 
b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
index e67d899b9446..1501f067d77f 100644
--- a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+++ b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
@@ -27,6 +27,9 @@ [Packages]
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
 
+[Packages.ARM, Packages.AARCH64]
+  ArmPkg/ArmPkg.dec
+
 [LibraryClasses]
   BaseLib
   DebugLib
diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c 
b/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c
index f6b2c51b6cc1..87cb74566730 100644
--- a/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c
+++ b/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c
@@ -9,6 +9,10 @@
 #include 
 #include 
 
+#if defined (MDE_CPU_AARCH64) || defined (MDE_CPU_ARM)
+#include 
+#endif
+
 #include "Iret.h"
 
 /**
@@ -54,6 +58,20 @@ DisableInterruptsOnIret (
   Eflags.Bits.IF  = 0;
   SystemContext.SystemContextIa32->Eflags = Eflags.UintN;
 
+ #elif defined (MDE_CPU_AARCH64)
+
+  //
+  // Set IRQ-disabled and FIQ-disabled flags.
+  //
+  SystemContext.SystemContextAArch64->SPSR |= (SPSR_I | SPSR_F);
+
+ #elif defined (MDE_CPU_ARM)
+
+  //
+  // Set IRQ-disabled and FIQ-disabled flags.
+  //
+  SystemContext.SystemContextArm->CPSR |= (CPSR_IRQ | CPSR_FIQ);
+
  #else
 
   #error "Unsupported CPU"
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114211): https://edk2.groups.io/g/devel/message/114211
Mute This Topic: https://groups.io/mt/103911611/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib

[Michael Brown]

Add the ability to perform self-tests on the first few invocations of
NestedInterruptRestoreTPL(), to verify that:

- the timer interrupt handler correctly rearms the timer interrupt
  before calling NestedInterruptRestoreTPL(), and

- the architecture-specific DisableInterruptsOnIret() implementation
  correctly causes interrupts to be disabled after the IRET or
  equivalent instruction.

Any test failure will be treated as fatal and will halt the system
with an appropriate diagnostic message.

Each test invocation adds approximately one timer tick of delay to the
overall system startup time.

Only one test is performed by default (to avoid unnecessary system
startup delay).  The number of tests performed can be controlled via
PcdNestedInterruptNumberOfSelfTests at build time.

Signed-off-by: Michael Brown 
---
 MdeModulePkg/MdeModulePkg.dec |   4 +
 .../NestedInterruptTplLib.inf |   3 +
 .../Include/Library/NestedInterruptTplLib.h   |   4 +
 .../Library/NestedInterruptTplLib/Tpl.c   | 129 ++
 4 files changed, 140 insertions(+)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index d6fb729af5a7..efd32c197b18 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1142,6 +1142,10 @@ [PcdsFixedAtBuild]
   # @Prompt Enable large address image loading.
   
gEfiMdeModulePkgTokenSpaceGuid.PcdImageLargeAddressLoad|TRUE|BOOLEAN|0x30001059
 
+  ## Number of NestedInterruptTplLib self-tests to perform at startup.
+  # @Prompt Number of NestedInterruptTplLib self-tests.
+  
gEfiMdeModulePkgTokenSpaceGuid.PcdNestedInterruptNumberOfSelfTests|1|UINT32|0x30001060
+
 [PcdsFixedAtBuild, PcdsPatchableInModule]
   ## Dynamic type PCD can be registered callback function for Pcd setting 
action.
   #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of 
callback function
diff --git 
a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf 
b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
index f130d6dcd213..e67d899b9446 100644
--- a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+++ b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
@@ -34,3 +34,6 @@ [LibraryClasses]
 
 [Depex.common.DXE_DRIVER]
   TRUE
+
+[Pcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNestedInterruptNumberOfSelfTests
diff --git a/MdeModulePkg/Include/Library/NestedInterruptTplLib.h 
b/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
index 0ead6e4b346a..7dd934577e99 100644
--- a/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
+++ b/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
@@ -32,6 +32,10 @@ typedef struct {
   /// interrupt handler.
   ///
   BOOLEANDeferredRestoreTPL;
+  ///
+  /// Number of self-tests performed.
+  ///
+  UINTN  SelfTestCount;
 } NESTED_INTERRUPT_STATE;
 
 /**
diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c 
b/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
index 99af553ab189..dfe22331204f 100644
--- a/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
+++ b/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
@@ -17,6 +17,18 @@
 
 #include "Iret.h"
 
+//
+// Number of self-tests to perform.
+//
+#define NUMBER_OF_SELF_TESTS \
+  (FixedPcdGet32 (PcdNestedInterruptNumberOfSelfTests))
+
+STATIC
+VOID
+NestedInterruptSelfTest (
+  IN NESTED_INTERRUPT_STATE  *IsrState
+  );
+
 /**
   Raise the task priority level to TPL_HIGH_LEVEL.
 
@@ -211,6 +223,16 @@ NestedInterruptRestoreTPL (
 //
 DisableInterrupts ();
 
+///
+/// Perform a limited number of self-tests on the first few
+/// invocations.
+///
+if ((IsrState->DeferredRestoreTPL == FALSE) &&
+   (IsrState->SelfTestCount < NUMBER_OF_SELF_TESTS)) {
+  IsrState->SelfTestCount++;
+  NestedInterruptSelfTest (IsrState);
+}
+
 //
 // DEFERRAL RETURN POINT
 //
@@ -248,3 +270,110 @@ NestedInterruptRestoreTPL (
 }
   }
 }
+
+/**
+  Perform internal self-test.
+
+  Induce a delay to force a nested timer interrupt to take place, and
+  verify that the nested interrupt behaves as required.
+
+  @param IsrState  A pointer to the state shared between all
+   invocations of the nested interrupt handler.
+**/
+VOID
+NestedInterruptSelfTest (
+  IN NESTED_INTERRUPT_STATE  *IsrState
+  )
+{
+  UINTN SelfTestCount;
+  UINTN TimeOut;
+
+  //
+  // Record number of this self-test for debug messages.
+  //
+  SelfTestCount = IsrState->SelfTestCount;
+
+  //
+  // Re-enable interrupts and stall for up to one second to induce at
+  // least one more timer interrupt.
+  //
+  // This mimics the effect of an interrupt having occurred precisely
+  // at the end of our call to RestoreTPL(), with interrupts having
+  // been re-enabled by RestoreTPL() and with the interrupt happening
+  // to occur after the TPL has already been lowered 

[edk2-devel] [PATCH v3 3/5] MdeModulePkg: Do nothing on NestedInterruptRestoreTPL(TPL_HIGH_LEVEL)

[Michael Brown]

At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI
specification) and so we should never encounter a situation in which
an interrupt occurs at TPL_HIGH_LEVEL.

Restoring TPL to TPL_HIGH_LEVEL is always a no-op.  Return immediately
from NestedInterruptRestoreTPL(TPL_HIGH_LEVEL), so that we do not need
to consider the effect of this possible invariant violation on the
remainder of the logic.

Signed-off-by: Michael Brown 
---
 MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c 
b/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
index d56c12a44529..99af553ab189 100644
--- a/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
+++ b/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
@@ -99,6 +99,19 @@ NestedInterruptRestoreTPL (
   EFI_TPL  SavedInProgressRestoreTPL;
   BOOLEAN  DeferredRestoreTPL;
 
+  //
+  // At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI
+  // specification) and so we should never encounter a situation in
+  // which InterruptedTPL==TPL_HIGH_LEVEL.
+  //
+  // Restoring TPL to TPL_HIGH_LEVEL is always a no-op.  Return
+  // immediately so that we do not need to consider the effect of this
+  // possible invariant violation in the logic below.
+  //
+  if (InterruptedTPL >= TPL_HIGH_LEVEL) {
+return;
+  }
+
   //
   // If the TPL at which this interrupt occurred is equal to that of
   // the in-progress RestoreTPL() for an outer instance of the same
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114209): https://edk2.groups.io/g/devel/message/114209
Mute This Topic: https://groups.io/mt/103911606/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 2/5] MdeModulePkg: Add missing Iret.h to NestedInterruptTplLib sources list

[Michael Brown]

Allow build system to detect changes to Iret.h.

Suggested-by: Ray Ni 
Reviewed-by: Laszlo Ersek 
Signed-off-by: Michael Brown 
---
 .../Library/NestedInterruptTplLib/NestedInterruptTplLib.inf  | 1 +
 1 file changed, 1 insertion(+)

diff --git 
a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf 
b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
index 1e03e1364e0f..f130d6dcd213 100644
--- a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+++ b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
@@ -20,6 +20,7 @@ [Defines]
 
 [Sources]
   Tpl.c
+  Iret.h
   Iret.c
 
 [Packages]
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114208): https://edk2.groups.io/g/devel/message/114208
Mute This Topic: https://groups.io/mt/103911605/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 0/5] MdeModulePkg: Move NestedInterruptTplLib to MdeModulePkg

[Michael Brown]

NestedInterruptTplLib provides a way for timer interrupt handlers
(which must support nested interrupts) to prevent unbounded stack
consumption.

The underlying issue was first observed in OvmfPkg, since interrupt
storms can arise more easily in virtual machines due to CPU
starvation.  However, careful investigation shows that the unbounded
stack consumption can also occur in physical machines.

Move NestedInterruptTplLib from OvmfPkg to MdeModulePkg so that it can
more easily be consumed by drivers outside of OvmfPkg, adding a
self-test capability and support for Arm CPUs.

Changes since v1:
  - Add missing Iret.h to NestedInterruptTplLib sources list

Changes since v2:
  - Remove obsolete dependency of LocalApicTimerDxe on OvmfPkg
  - Add to MdeModulePkg.dsc for build coverage
  - Add self-tests
  - Add support for Arm CPUs

Michael Brown (5):
  MdeModulePkg: Move NestedInterruptTplLib to MdeModulePkg
  MdeModulePkg: Add missing Iret.h to NestedInterruptTplLib sources list
  MdeModulePkg: Do nothing on NestedInterruptRestoreTPL(TPL_HIGH_LEVEL)
  MdeModulePkg: Add self-tests for NestedInterruptTplLib
  MdeModulePkg: Extend NestedInterruptTplLib to support Arm CPUs

 MdeModulePkg/MdeModulePkg.dec |   8 +
 OvmfPkg/OvmfPkg.dec   |   4 -
 MdeModulePkg/MdeModulePkg.dsc |   1 +
 OvmfPkg/AmdSev/AmdSevX64.dsc  |   2 +-
 OvmfPkg/CloudHv/CloudHvX64.dsc|   2 +-
 OvmfPkg/IntelTdx/IntelTdxX64.dsc  |   2 +-
 OvmfPkg/Microvm/MicrovmX64.dsc|   2 +-
 OvmfPkg/OvmfPkgIa32.dsc   |   2 +-
 OvmfPkg/OvmfPkgIa32X64.dsc|   2 +-
 OvmfPkg/OvmfPkgX64.dsc|   2 +-
 OvmfPkg/OvmfXen.dsc   |   2 +-
 UefiPayloadPkg/UefiPayloadPkg.dsc |   2 +-
 .../NestedInterruptTplLib.inf |   9 +-
 .../LocalApicTimerDxe/LocalApicTimerDxe.inf   |   2 +-
 .../Include/Library/NestedInterruptTplLib.h   |   4 +
 .../Library/NestedInterruptTplLib/Iret.h  |   0
 .../Library/NestedInterruptTplLib/Iret.c  |  18 +++
 .../Library/NestedInterruptTplLib/Tpl.c   | 142 ++
 18 files changed, 191 insertions(+), 15 deletions(-)
 rename {OvmfPkg => 
MdeModulePkg}/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf (78%)
 rename {OvmfPkg => MdeModulePkg}/Include/Library/NestedInterruptTplLib.h (94%)
 rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Iret.h (100%)
 rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Iret.c (72%)
 rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Tpl.c (64%)

-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114206): https://edk2.groups.io/g/devel/message/114206
Mute This Topic: https://groups.io/mt/103911600/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 1/5] MdeModulePkg: Move NestedInterruptTplLib to MdeModulePkg

[Michael Brown]

NestedInterruptTplLib provides a way for timer interrupt handlers
(which must support nested interrupts) to prevent unbounded stack
consumption.

The underlying issue was first observed in OvmfPkg, since interrupt
storms can arise more easily in virtual machines due to CPU
starvation.  However, careful investigation shows that the unbounded
stack consumption can also occur in physical machines.

Move NestedInterruptTplLib from OvmfPkg to MdeModulePkg so that it can
more easily be consumed by drivers outside of OvmfPkg.

Add NestedInterruptTplLib to the MdeModulePkg component build list for
IA32 and X64 only, since those are the only two architectures for
which DisableInterruptsOnIret() is currently supported.

Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Michael D Kinney 
Reviewed-by: Laszlo Ersek 
Signed-off-by: Michael Brown 
---
 MdeModulePkg/MdeModulePkg.dec | 4 
 OvmfPkg/OvmfPkg.dec   | 4 
 MdeModulePkg/MdeModulePkg.dsc | 1 +
 OvmfPkg/AmdSev/AmdSevX64.dsc  | 2 +-
 OvmfPkg/CloudHv/CloudHvX64.dsc| 2 +-
 OvmfPkg/IntelTdx/IntelTdxX64.dsc  | 2 +-
 OvmfPkg/Microvm/MicrovmX64.dsc| 2 +-
 OvmfPkg/OvmfPkgIa32.dsc   | 2 +-
 OvmfPkg/OvmfPkgIa32X64.dsc| 2 +-
 OvmfPkg/OvmfPkgX64.dsc| 2 +-
 OvmfPkg/OvmfXen.dsc   | 2 +-
 UefiPayloadPkg/UefiPayloadPkg.dsc | 2 +-
 .../Library/NestedInterruptTplLib/NestedInterruptTplLib.inf   | 2 +-
 OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf   | 2 +-
 .../Include/Library/NestedInterruptTplLib.h   | 0
 .../Library/NestedInterruptTplLib/Iret.h  | 0
 .../Library/NestedInterruptTplLib/Iret.c  | 0
 {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Tpl.c | 0
 18 files changed, 16 insertions(+), 15 deletions(-)
 rename {OvmfPkg => 
MdeModulePkg}/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf (91%)
 rename {OvmfPkg => MdeModulePkg}/Include/Library/NestedInterruptTplLib.h (100%)
 rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Iret.h (100%)
 rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Iret.c (100%)
 rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Tpl.c (100%)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index a2cd83345f5b..d6fb729af5a7 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -169,6 +169,10 @@ [LibraryClasses]
   #
   ImagePropertiesRecordLib|Include/Library/ImagePropertiesRecordLib.h
 
+  ##  @libraryclass  Handle TPL changes within nested interrupt handlers
+  #
+  NestedInterruptTplLib|Include/Library/NestedInterruptTplLib.h
+
 [Guids]
   ## MdeModule package token space guid
   # Include/Guid/MdeModulePkgTokenSpace.h
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index b44fa039f76c..05d43d5a6861 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -41,10 +41,6 @@ [LibraryClasses]
   #
   MemEncryptTdxLib|Include/Library/MemEncryptTdxLib.h
 
-  ##  @libraryclass  Handle TPL changes within nested interrupt handlers
-  #
-  NestedInterruptTplLib|Include/Library/NestedInterruptTplLib.h
-
   ##  @libraryclass  Save and restore variables using a file
   #
   NvVarsFileLib|Include/Library/NvVarsFileLib.h
diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
index 6b3052ff4614..5b9ddfd26e75 100644
--- a/MdeModulePkg/MdeModulePkg.dsc
+++ b/MdeModulePkg/MdeModulePkg.dsc
@@ -526,6 +526,7 @@ [Components.IA32, Components.X64]
   MdeModulePkg/Library/TraceHubDebugSysTLib/BaseTraceHubDebugSysTLib.inf
   MdeModulePkg/Library/TraceHubDebugSysTLib/PeiTraceHubDebugSysTLib.inf
   MdeModulePkg/Library/TraceHubDebugSysTLib/DxeSmmTraceHubDebugSysTLib.inf
+  MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
 
 [Components.X64]
   MdeModulePkg/Universal/CapsulePei/CapsuleX64.inf
diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index a31a89344a60..80456f878a22 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -354,7 +354,7 @@ [LibraryClasses.common.DXE_DRIVER]
 !endif
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
-  
NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+  
NestedInterruptTplLib|MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
   
QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
 
diff --git a/OvmfPkg/CloudH

Re: [edk2-devel] [PATCH v2 1/2] MdeModulePkg: Move NestedInterruptTplLib to MdeModulePkg

[Michael Brown]

On 22/01/2024 10:36, Laszlo Ersek wrote:

(1) Does LocalApicTimerDxe have any other dependencies on OvmfPkg than
the library class header?

In other words, can't we remove "OvmfPkg/OvmfPkg.dec" from this
[Packages] section, too?

I can't see anything else, so I'd suggest to remove
"OvmfPkg/OvmfPkg.dec", but I could be missing something.


Good catch, thank you.  I have removed it and OvmfPkg still builds 
without errors, so I think it can go.



Therefore, I suggest adding "NestedInterruptTplLib.inf" to
"MdeModulePkg.dsc". AIUI, right now the library instance is x86-only
(with a proposal on-list for the AARCH64 extension). Therefore, the
section in "MdeModulePkg.dsc" to add the INF file to would be:

[Components.IA32, Components.X64]

Within that section, library instances and drivers are apparently not
listed in lexicographical order, so maybe just tack the new line to the
bottom.

For build-testing, run the command

   build \
 -a IA32 -a X64 \
 -b NOOPT \
 -p MdeModulePkg/MdeModulePkg.dsc \
 -t GCC5 \
 -m MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf


Thank you very much!  I have made all of the changes that you suggested 
(in both emails).


Following Ray's concerns about the ability to test this code, I have 
also added a self-test capability.  This deliberately triggers a 
configurable number of delays within NestedInterruptRestoreTPL() to 
force the system to experience a nested interrupt.  The code verifies that:


a) the architecture's implementation of DisableInterruptsOnIret() is 
behaving as specified, and


b) the platform's timer interrupt handler is correctly rearming the 
timer interrupt before calling NestedInterruptRestoreTPL().


I've also added AArch64 and ARM implementations for 
DisableInterruptsOnIret(), which I shall test shortly.


Patch series v3 to follow.

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114198): https://edk2.groups.io/g/devel/message/114198
Mute This Topic: https://groups.io/mt/103852900/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 0/2] Move NestedInterruptTplLib to MdeModulePkg

[Michael Brown]

NestedInterruptTplLib provides a way for timer interrupt handlers
(which must support nested interrupts) to prevent unbounded stack
consumption.

The underlying issue was first observed in OvmfPkg, since interrupt
storms can arise more easily in virtual machines due to CPU
starvation.  However, careful investigation shows that the unbounded
stack consumption can also occur in physical machines.

Move NestedInterruptTplLib from OvmfPkg to MdeModulePkg so that it can
more easily be consumed by drivers outside of OvmfPkg.

Changes since v1:
  - Add missing Iret.h to NestedInterruptTplLib sources list

Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Michael D Kinney 

Michael Brown (2):
  MdeModulePkg: Move NestedInterruptTplLib to MdeModulePkg
  MdeModulePkg: Add missing Iret.h to NestedInterruptTplLib sources list

 MdeModulePkg/MdeModulePkg.dec | 4 
 OvmfPkg/OvmfPkg.dec   | 4 
 OvmfPkg/AmdSev/AmdSevX64.dsc  | 2 +-
 OvmfPkg/CloudHv/CloudHvX64.dsc| 2 +-
 OvmfPkg/IntelTdx/IntelTdxX64.dsc  | 2 +-
 OvmfPkg/Microvm/MicrovmX64.dsc| 2 +-
 OvmfPkg/OvmfPkgIa32.dsc   | 2 +-
 OvmfPkg/OvmfPkgIa32X64.dsc| 2 +-
 OvmfPkg/OvmfPkgX64.dsc| 2 +-
 OvmfPkg/OvmfXen.dsc   | 2 +-
 UefiPayloadPkg/UefiPayloadPkg.dsc | 2 +-
 .../Library/NestedInterruptTplLib/NestedInterruptTplLib.inf   | 3 ++-
 OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf   | 1 +
 .../Include/Library/NestedInterruptTplLib.h   | 0
 .../Library/NestedInterruptTplLib/Iret.h  | 0
 .../Library/NestedInterruptTplLib/Iret.c  | 0
 {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Tpl.c | 0
 17 files changed, 16 insertions(+), 14 deletions(-)
 rename {OvmfPkg => 
MdeModulePkg}/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf (90%)
 rename {OvmfPkg => MdeModulePkg}/Include/Library/NestedInterruptTplLib.h (100%)
 rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Iret.h (100%)
 rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Iret.c (100%)
 rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Tpl.c (100%)

-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114119): https://edk2.groups.io/g/devel/message/114119
Mute This Topic: https://groups.io/mt/103852899/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 2/2] MdeModulePkg: Add missing Iret.h to NestedInterruptTplLib sources list

[Michael Brown]

Suggested-by: Ray Ni 
Signed-off-by: Michael Brown 
---
 .../Library/NestedInterruptTplLib/NestedInterruptTplLib.inf  | 1 +
 1 file changed, 1 insertion(+)

diff --git 
a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf 
b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
index 1e03e1364e0f..f130d6dcd213 100644
--- a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+++ b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
@@ -20,6 +20,7 @@ [Defines]
 
 [Sources]
   Tpl.c
+  Iret.h
   Iret.c
 
 [Packages]
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114121): https://edk2.groups.io/g/devel/message/114121
Mute This Topic: https://groups.io/mt/103852903/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 1/2] MdeModulePkg: Move NestedInterruptTplLib to MdeModulePkg

[Michael Brown]

NestedInterruptTplLib provides a way for timer interrupt handlers
(which must support nested interrupts) to prevent unbounded stack
consumption.

The underlying issue was first observed in OvmfPkg, since interrupt
storms can arise more easily in virtual machines due to CPU
starvation.  However, careful investigation shows that the unbounded
stack consumption can also occur in physical machines.

Move NestedInterruptTplLib from OvmfPkg to MdeModulePkg so that it can
more easily be consumed by drivers outside of OvmfPkg.

Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Michael D Kinney 
Signed-off-by: Michael Brown 
---
 MdeModulePkg/MdeModulePkg.dec | 4 
 OvmfPkg/OvmfPkg.dec   | 4 
 OvmfPkg/AmdSev/AmdSevX64.dsc  | 2 +-
 OvmfPkg/CloudHv/CloudHvX64.dsc| 2 +-
 OvmfPkg/IntelTdx/IntelTdxX64.dsc  | 2 +-
 OvmfPkg/Microvm/MicrovmX64.dsc| 2 +-
 OvmfPkg/OvmfPkgIa32.dsc   | 2 +-
 OvmfPkg/OvmfPkgIa32X64.dsc| 2 +-
 OvmfPkg/OvmfPkgX64.dsc| 2 +-
 OvmfPkg/OvmfXen.dsc   | 2 +-
 UefiPayloadPkg/UefiPayloadPkg.dsc | 2 +-
 .../Library/NestedInterruptTplLib/NestedInterruptTplLib.inf   | 2 +-
 OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf   | 1 +
 .../Include/Library/NestedInterruptTplLib.h   | 0
 .../Library/NestedInterruptTplLib/Iret.h  | 0
 .../Library/NestedInterruptTplLib/Iret.c  | 0
 {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Tpl.c | 0
 17 files changed, 15 insertions(+), 14 deletions(-)
 rename {OvmfPkg => 
MdeModulePkg}/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf (91%)
 rename {OvmfPkg => MdeModulePkg}/Include/Library/NestedInterruptTplLib.h (100%)
 rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Iret.h (100%)
 rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Iret.c (100%)
 rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Tpl.c (100%)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index a2cd83345f5b..d6fb729af5a7 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -169,6 +169,10 @@ [LibraryClasses]
   #
   ImagePropertiesRecordLib|Include/Library/ImagePropertiesRecordLib.h
 
+  ##  @libraryclass  Handle TPL changes within nested interrupt handlers
+  #
+  NestedInterruptTplLib|Include/Library/NestedInterruptTplLib.h
+
 [Guids]
   ## MdeModule package token space guid
   # Include/Guid/MdeModulePkgTokenSpace.h
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index b44fa039f76c..05d43d5a6861 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -41,10 +41,6 @@ [LibraryClasses]
   #
   MemEncryptTdxLib|Include/Library/MemEncryptTdxLib.h
 
-  ##  @libraryclass  Handle TPL changes within nested interrupt handlers
-  #
-  NestedInterruptTplLib|Include/Library/NestedInterruptTplLib.h
-
   ##  @libraryclass  Save and restore variables using a file
   #
   NvVarsFileLib|Include/Library/NvVarsFileLib.h
diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index a31a89344a60..80456f878a22 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -354,7 +354,7 @@ [LibraryClasses.common.DXE_DRIVER]
 !endif
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
-  
NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+  
NestedInterruptTplLib|MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
   
QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
 
diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc b/OvmfPkg/CloudHv/CloudHvX64.dsc
index b522fa10594d..9c6c68ae2c35 100644
--- a/OvmfPkg/CloudHv/CloudHvX64.dsc
+++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
@@ -394,7 +394,7 @@ [LibraryClasses.common.DXE_DRIVER]
 !endif
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
-  
NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+  
NestedInterruptTplLib|MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
   QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
 
diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
index 82e3e41cfc57..5270c59e1279 100644
--- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
+++ b/OvmfPkg/IntelTdx/IntelTdxX64.ds

Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Move NestedInterruptTplLib to MdeModulePkg

[Michael Brown]

On 20/01/2024 07:03, Ni, Ray wrote:

Can you submit another patch to add Iret.h to the INF file [Sources] section?

Without that, incremental build might not work if changes are made in Iret.h.


Good catch, thank you.  I will send a v2.


It doesn't support non-x86 CPU. Will ARM have similar problems?
+Ard,


The underlying issue is not x86-specific, so it would make sense for all 
timer interrupt handlers to make use of NestedInterruptTplLib.


The Arm ARM states in D1.3.2 that when an interrupt is raised, the 
PSTATE.I and PSTATE.F bits are both set to 1.  I believe the following 
code in Iret.c should therefore work for AArch64:


+ #elif defined (MDE_CPU_AARCH64)
+
+  //
+  // Set IRQ-disabled and FIQ-disabled flags.
+  //
+  SystemContext.SystemContextAArch64->SPSR |= (SPSR_I | SPSR_F);
+

(plus whatever .inf changes are needed to pick up the ArmLib headers 
where SPSR_I and SPSR_F are defined).


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114118): https://edk2.groups.io/g/devel/message/114118
Mute This Topic: https://groups.io/mt/103841406/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Michael Brown]

On 19/01/2024 23:44, Ni, Ray wrote:
I still want to see if the RestoreTpl2 that does not enable interrupt is 
added as a protocol, and how simple the lib could be.


RestoreTpl() always has to enable interrupts during its execution, since 
interrupts must be allowed to occur while callbacks are running 
(otherwise the callbacks may break due to the system time freezing).


The only alternative approach I am aware of would be to add a 
RestoreTPLEx() call to EFI_BOOT_SERVICES, with an additional parameter 
EnableInterruptsAtRestoredTpl.


RestoreTPLEx() would then:

1. For each TPL between EfiCurrentTpl and OldTpl:
   a) enable interrupts
   b) dispatch any callbacks registered at this TPL
   c) disable interrupts

2. Re-enable interrupts before returning if 
EnableInterruptsAtRestoredTpl is TRUE.


The implementation of RestoreTPL() would then become just a call to 
RestoreTPLEx(OldTPl, (OldTpl < TPL_HIGH_LEVEL)).


This would require a change to the EFI_BOOT_SERVICES table definition, 
which is something that I don't think has happened in the 18 years since 
the UEFI specification was released.  There's a very good chance that 
such a table change would break something, somewhere.


RestoreTPLEx() could be installed as a protocol instead, but it seems 
very messy to have something so fundamental as TPL management and event 
dispatch handled through an installable (and therefore uninstallable) 
protocol.  Are there any other instances where deep internals of DxeCore 
are exposed in this way?


Lastly: I think the RestoreTPLEx approach ought to work, but I have not 
done any testing on it (and have no intention of trying it, unless Intel 
wants to fund the work).  NestedInterruptTplLib has been quite 
thoroughly tested by now.



The reason is about maintainability.
I can image that one day people would question the Lib implementation if 
some timer event issue appears. If the Lib is easy to understand, the 
suspicion could be avoided.
And if the correctness of the Lib can be proven by a thorough test, that 
will be better. But it seems to me the Lib can only be proven as correct 
with careful code review, like some multi-threaded logic.


It's relatively easy to test with a deliberately broken ISR: that's how 
I tested it during development.


The semi-formal proof is an added bonus.  Testing shows that the 
symptoms have gone away, but the semi-formal proof is what gives 
confidence (to me, at least) that the problem has actually been fixed 
properly.


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114104): https://edk2.groups.io/g/devel/message/114104
Mute This Topic: https://groups.io/mt/103734961/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/1] MdeModulePkg: Move NestedInterruptTplLib to MdeModulePkg

[Michael Brown]

NestedInterruptTplLib provides a way for timer interrupt handlers
(which must support nested interrupts) to prevent unbounded stack
consumption.

The underlying issue was first observed in OvmfPkg, since interrupt
storms can arise more easily in virtual machines due to CPU
starvation.  However, careful investigation shows that the unbounded
stack consumption can also occur in physical machines.

Move NestedInterruptTplLib from OvmfPkg to MdeModulePkg so that it can
more easily be consumed by drivers outside of OvmfPkg.

Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Michael D Kinney 
Signed-off-by: Michael Brown 
---
 MdeModulePkg/MdeModulePkg.dec | 4 
 OvmfPkg/OvmfPkg.dec   | 4 
 OvmfPkg/AmdSev/AmdSevX64.dsc  | 2 +-
 OvmfPkg/CloudHv/CloudHvX64.dsc| 2 +-
 OvmfPkg/IntelTdx/IntelTdxX64.dsc  | 2 +-
 OvmfPkg/Microvm/MicrovmX64.dsc| 2 +-
 OvmfPkg/OvmfPkgIa32.dsc   | 2 +-
 OvmfPkg/OvmfPkgIa32X64.dsc| 2 +-
 OvmfPkg/OvmfPkgX64.dsc| 2 +-
 OvmfPkg/OvmfXen.dsc   | 2 +-
 UefiPayloadPkg/UefiPayloadPkg.dsc | 2 +-
 .../Library/NestedInterruptTplLib/NestedInterruptTplLib.inf   | 2 +-
 OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf   | 1 +
 .../Include/Library/NestedInterruptTplLib.h   | 0
 .../Library/NestedInterruptTplLib/Iret.h  | 0
 .../Library/NestedInterruptTplLib/Iret.c  | 0
 {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Tpl.c | 0
 17 files changed, 15 insertions(+), 14 deletions(-)
 rename {OvmfPkg => 
MdeModulePkg}/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf (91%)
 rename {OvmfPkg => MdeModulePkg}/Include/Library/NestedInterruptTplLib.h (100%)
 rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Iret.h (100%)
 rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Iret.c (100%)
 rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Tpl.c (100%)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index a2cd83345f5b..d6fb729af5a7 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -169,6 +169,10 @@ [LibraryClasses]
   #
   ImagePropertiesRecordLib|Include/Library/ImagePropertiesRecordLib.h
 
+  ##  @libraryclass  Handle TPL changes within nested interrupt handlers
+  #
+  NestedInterruptTplLib|Include/Library/NestedInterruptTplLib.h
+
 [Guids]
   ## MdeModule package token space guid
   # Include/Guid/MdeModulePkgTokenSpace.h
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index b44fa039f76c..05d43d5a6861 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -41,10 +41,6 @@ [LibraryClasses]
   #
   MemEncryptTdxLib|Include/Library/MemEncryptTdxLib.h
 
-  ##  @libraryclass  Handle TPL changes within nested interrupt handlers
-  #
-  NestedInterruptTplLib|Include/Library/NestedInterruptTplLib.h
-
   ##  @libraryclass  Save and restore variables using a file
   #
   NvVarsFileLib|Include/Library/NvVarsFileLib.h
diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index a31a89344a60..80456f878a22 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -354,7 +354,7 @@ [LibraryClasses.common.DXE_DRIVER]
 !endif
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
-  
NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+  
NestedInterruptTplLib|MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
   
QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
 
diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc b/OvmfPkg/CloudHv/CloudHvX64.dsc
index b522fa10594d..9c6c68ae2c35 100644
--- a/OvmfPkg/CloudHv/CloudHvX64.dsc
+++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
@@ -394,7 +394,7 @@ [LibraryClasses.common.DXE_DRIVER]
 !endif
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
-  
NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+  
NestedInterruptTplLib|MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
   QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
 
diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
index 82e3e41cfc57..5270c59e1279 100644
--- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
+++ b/OvmfPkg/IntelTdx/IntelTdxX64.ds

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Michael Brown]

On 19/01/2024 13:14, Ni, Ray wrote:
So, the interrupt re-entrance we want to avoid is “env:NOTIFY”  -> 
“env:NOTIFY”, or “env:CALLBACK” -> “env:CALLBACK”, or “env:APPLICATION” 
-> “env:APPLICATION”. Because it’s endless.


NestedTplInterruptLib was written to avoid it.


Yes, precisely this.


 2. Some questions on NestedInterruptTplLib.

 1. Can we remove DisableInterruptsOnIret()? That means the inner
interrupt handler would returns to the outer world with interrupt
enabled and TPL==HIGH. But I don’t see any issue with that.
Using DisableInterruptsOnIret() allows us to guarantee that absolutely 
nothing happens between the "DEFERRAL INVOCATION POINT" and "DEFERRAL 
RETURN POINT" described in the comments in Tpl.c.


If we don't use DisableInterruptsOnIret() then we lose this guarantee, 
and the situation becomes even more complex than it already is.


I don't personally feel able to reason through all the possible 
circumstances that could arise if an interrupt were to occur between 
"DEFERRAL INVOCATION POINT" and "DEFERRAL RETURN POINT", so I don't feel 
safe removing the use of DisableInterruptsOnIret().


I have a vague memory that I was still experiencing some kind of crashes 
before I added DisableInterruptsOnIret(), but I cannot now remember any 
details, sorry.



 2. If DxeCore can be changed, do you have an easier-to-understand
solution? It really took me 2 days to understand why
NestedInterruptTplLib is written in today’s way.


The ability to change DxeCore doesn't help, unfortunately.

If we could change the prototype of RaiseTPL() and RestoreTPL() to 
include a flag indicating whether or not interrupts should be enabled at 
the point that RestoreTPL() returns, then that would allow for an 
easier-to-understand solution.


This would require making a breaking change to the UEFI specification, 
though, so it's not a viable solution.



I do appreciate that it's difficult to understand the internals of 
NestedInterruptTplLib.  It's fundamentally having to solve a very 
difficult problem within the constraints of the UEFI API.  I think the 
solution that NestedInterruptTplLib provides is as simple as it's 
possible to get, and it does at least have the advantage that all of the 
complexity is hidden inside the library: the caller gets to just change 
two lines:


- OriginalTPL = gBS->RaiseTPL(TPL_HIGH_LEVEL);
+ OriginalTPL = NestedInterruptRaiseTPL();
  ...
- gBS->RestoreTPL(OriginalTPL);
+ NestedInterruptRestoreTPL(OriginalTPL, Context, );


I'll send through a patch to move NestedInterruptTplLib to MdeModulePkg.

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114091): https://edk2.groups.io/g/devel/message/114091
Mute This Topic: https://groups.io/mt/103734961/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Michael Brown]

On 17/01/2024 07:11, Ni, Ray wrote:

The above flow shows endless re-entrance of timer interrupt handler.

But, my question is: above flow only can happen in real platform when the below 
4 steps occupies more time than the timer period (usually 10ms).
 [Timer Interrupt #2]1. RaiseTPL (HIGH) from NOTIFY causing CPU 
interrupt be disabled.
 [Timer Interrupt #2]2. Send APIC EOI (ACK the interrupt received 
so APIC can continue generate interrupts)
 [Timer Interrupt #2]3. Call DxeCore::CoreTimerTick()
 [Timer Interrupt #2]4. RestoreTPL (NOTIFY) from HIGH. No callback 
runs as no callback can be registered at TPL > NOTIFY. In the end of 
RestoreTPL(), CPU interrupt is enabled.

But, in my opinion, it's impossible.


As is thoroughly documented in NestedInterruptRestoreTpl(), the 
potential for unbounded stack consumption arises when an interrupt 
occurs after the point that RestoreTPL() completes dispatching all 
notifications but before the IRET (or equivalent) instruction pops the 
original stack frame.


Since dispatching notifications can take an unbounded amount of time, 
there is absolutely no guarantee that this will be less than 10ms after 
the previous interrupt.  It could easily be 30 seconds later.


The problematic flow is a subtle variation on what you described:

  [IRQ#1] timer interrupt at TPL_APPLICATION
[ISR#1] RaiseTPL from TPL_APPLICATION -> TPL_HIGH_LEVEL
[ISR#1] Send APIC EOI
[ISR#1] Call CoreTimerTick()
[ISR#1] RestoreTPL from TPL_HIGH_LEVEL -> TPL_APPLICATION
  [ISR#1] Callbacks for TPL_NOTIFY are run
  [ISR#1] Callbacks for TPL_CALLBACK are run
  ... these may take several *seconds* to complete, during
  which further interrupts are raised, the details of
  which are not shown here...
  [ISR#1] TPL is now restored to TPL_APPLICATION
  [IRQ#N] timer interrupt at TPL_APPLICATION
[ISR#N] RaiseTPL from TPL_APPLICATION -> TPL_HIGH_LEVEL
... continues ...

The root cause is that the ISR reaches a state in which:

  a) an arbitrary amount of time has elapsed since the triggering
 interrupt (due to unknown callbacks being invoked, which may
 themselves wait for further timer interrupts), and

  b) the TPL has been fully restored back to the TPL at the point
 the triggering interrupt occurred (i.e. TPL_APPLICATION in
 this example), and

  c) the timer interrupt source is enabled, and

  d) CPU interrupts are enabled

At this point, there is nothing preventing another interrupt from 
occurring.  It will occur at TPL_APPLICATION and it will be one stack 
frame deeper than the previous interrupt at TPL_APPLICATION.


Rinse and repeat, and you have unbounded stack consumption.

Hence the requirement for NestedInterruptTplLib, even on physical hardware.

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113947): https://edk2.groups.io/g/devel/message/113947
Mute This Topic: https://groups.io/mt/103734961/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Michael Brown]

On 16/01/2024 14:34, Laszlo Ersek wrote:

On 1/16/24 10:48, Michael Brown wrote:
IOW, my impression is that NestedInterruptTplLib can certainly handle
all scenarios thrown at it, but where it really matters is in the face
of an interrupt storm (not just "normal nesting"), and a storm is
unlikely (or even impossible?) on physical hardware.

... Oh, scratch that. "Interrupt storm" simply means that interrupts are
being delivered at a rate higher than the handler routine can service
them. IOW, the "storm" is not that interrupts are delivered *very
rapidly* in an absoulte sense. If interrupts are delivered at normal
frequency, but the handler is too slow to service *even that rate*, then
that also qualifies as "storm", because the nesting depth will *keep
growing*. It's not really the growth rate that matters; what matter is
the *trend*, i.e., the fact that there *is* growth (the stack gets
deeper and deeper). The stack might not overflow immediately, and if the
handler speeds up (for whatever reason), the stack might recover, but
there is nothing to prevent an overflow.

So, in the end, I think you've convinced me.


:)


I'm happy to send a patch to migrate NestedInterruptTplLib to
MdeModulePkg, so that it can be consumed outside of OvmfPkg.  Shall I do
this?


Sounds like a valid idea to me.

Could be greatly supported by a test case (to be run on the bare metal)
installing a slow handler that *eventually* exhausted the stack, when
not using NestedInterruptTplLib.

(FWIW, IIRC, the UEFI spec warns about this -- it says something like,
"return from TPL_HIGH as soon as you can, otherwise the system will
become unstable".)

Sorry for the wall of text, I find this very difficult to reason about.


I also find it very difficult to reason about, which is why 
NestedInterruptRestoreTpl() has 126 lines of comments providing a 
semi-formal proof of correctness for a mere 15 statements of C code!


In particular, I find it difficult to reason about when it would be safe 
for a platform to *not* use NestedInterruptTplLib.  It's clearly 
empirically difficult to trigger stack underflow via an interrupt 
"storm" on physical hardware, but I'm not convinced it's impossible.


I find it mentally easier to rely on the hard guarantee that 
NestedInterruptTplLib provides: that nested interrupts will continue to 
be delivered but that the number of interrupt-induced stack frames is 
bounded by the (small, finite) number of distinct TPL levels in existence.




While developing NestedInterruptTplLib, I did hack together a test case 
for a slow handler that would deliberately induce an interrupt storm, 
since I needed this to test that my code was working.  When triggered, 
this test would cause the machine to effectively hang due to servicing 
an endless storm of timer interrupts.  Before NestedInterruptTplLib, the 
stack would soon underflow and would typically cause a reboot (or other 
crash).  With NestedInterruptTplLib the machine would continue to 
service interrupts indefinitely.


How might such a test case be included in upstream EDK2?  I'm 
peripherally aware of EDK2 test infrastructure such as UEFI SCT, but 
I've never interacted with it yet.


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113908): https://edk2.groups.io/g/devel/message/113908
Mute This Topic: https://groups.io/mt/103734961/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Michael Brown]

On 16/01/2024 08:47, Gerd Hoffmann wrote:

I think the reason is that the next timer interrupt arriving while the
handler is running still is *much* more likely in virtual machines
because the vCPU does not get 100% of the of the physical CPU time
slice.


The interrupt handler can run for an arbitrary length of time, because 
the call to RestoreTPL() can end up calling an application callback 
which in turn waits on further timer interrupts.


This is a legitimate use case within UEFI, so all timer interrupt 
handlers (not just in virtual machines) need to allow for the 
possibility that nested interrupts will occur.



So on OVMF we will continue to need NestedInterruptTplLib.  I like the
idea to have a Null version of the library, so OVMF does not need its
own version of the driver just because of NestedInterruptTplLib.


I agree that there should not need to be two versions of LocalApicTimerDxe.

I would suggest moving NestedInterruptTplLib from OvmfPkg to MdeModulePkg.

The code is complex, but for the caller the complexity is completely 
hidden behind the calls to NestedInterruptRaiseTPL() and 
NestedInterruptRestoreTPL(), which straightforwardly replace what would 
otherwise be (unsafe) calls to RaiseTPL() and RestoreTPL(), as shown in


https://github.com/tianocore/edk2/commit/a086f4a#diff-1418ec21e24e4ce2f3d621113585e3bea11fbcfa3b77d54046e61be7928e0c92

For a new CPU architecture, the only requirement is to provide a short 
fragment (~5 lines) of code that can clear the interrupts-enabled flag 
in the EFI_SYSTEM_CONTEXT, as shown in 
OvmfPkg/Library/NestedInterruptTplLib/Iret.c.


I'm happy to send a patch to migrate NestedInterruptTplLib to 
MdeModulePkg, so that it can be consumed outside of OvmfPkg.  Shall I do 
this?


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113883): https://edk2.groups.io/g/devel/message/113883
Mute This Topic: https://groups.io/mt/103734961/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Updates to .mailmap needed for Jeff Brasen, Jake Garver, Joey Vagades and Michael Roth?

[Michael Brown]

On 09/01/2024 12:13, Laszlo Ersek wrote:

On 1/9/24 11:45, Ard Biesheuvel wrote:

Note that git am does support a 'From: ' header as the first line of
the commit log, and will use it correctly to supersede the From:
header in the SMTP envelope.


OTOH, that doesn't help in this case, IIUC. When the poster originally
formats and sends the patch, their gitconfig says
user.email=foo...@example.com, and the author meta-datum on the patch
most likely *also* says foo...@example.com. (I.e., they are formatting a
patch they have authored themselves.) Therefore
git-format-patch/git-send-email have no reason to include an explicit
"From:" line at the top of the commit message body. I agree that
"From:", if present, mitigates the issue, but in most cases, I reckon,
it's not present.


Is there a way to configure git to force git-format-patch to 
unconditionally include the inline "From:" header?  I tried:


  git config format.forceInBodyFrom true

which is described in the man page as "may help if the mailing list 
software mangles the sender’s identity", but it seems to have an effect 
only if "--from" is also specified.


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113446): https://edk2.groups.io/g/devel/message/113446
Mute This Topic: https://groups.io/mt/103534194/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Michael Brown]

On 05/01/2024 08:41, Chang, Abner wrote:

We are not aware there is a TlsConnectSession() for TLS handshake using the 
default TLS configuration data and it returns a failure as expected because the 
default TLS configuration is TLS_VERIFY_HOST without certificate installed on 
system.
This happens in HttpInitSession before notifying HttpEventInitSession event,  
so we have to reconfigure TLS config data before TlsConnectSession() function.
As there is an existing HttpEventTlsConnectSession event notified after 
TlsConnectSession(), that makes sense to me to introduce a new HTTP event 
HttpEventTlsConfigured as I mentioned in previous conversation and notify 
callback functions after TlsConfigureSession().
Upper layer HTTP application then listen to HttpEventTlsConfigured event and 
reconfigure TLS configuration data in the callback function.


Sounds good to me.  Thank you for the improvements.  I think this design 
is now ready.


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113312): https://edk2.groups.io/g/devel/message/113312
Mute This Topic: https://groups.io/mt/103368438/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 3/5] NetwokrPkg/HttpDxe: Add HttpEventTlsConfigured HTTP callback event

[Michael Brown]

On 05/01/2024 08:37, abner.ch...@amd.com wrote:

+  ///
+  /// The Status of Event to configure TLS configuration data.
+  /// EventStatus:
+  /// EFI_SUCCESSThe TLS is configured successfully with the 
default value.
+  /// EFI_INVALID_PARAMETER  One or more input parameters to SetSessionData() 
is invalid.
+  /// EFI_NOT_READY  Current TLS session state is NOT 
EfiTlsSessionStateNotStarted.
+  /// EFI_NOT_FOUND  Fail to get 'HttpTlsCipherList' variable.
+  /// Others Other error as indicated.
+  ///
+  HttpEventTlsConfigured,
+


Since this changes the ABI, you may want to also update the protocol 
GUID to prevent strange errors if old and new binaries are used on the 
same system.


Reviewed-by: Michael Brown 

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113311): https://edk2.groups.io/g/devel/message/113311
Mute This Topic: https://groups.io/mt/103539580/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/5] NetwokrPkg/HttpDxe: Consider TLS certificate not found as a success case

[Michael Brown]

On 05/01/2024 08:37, abner.ch...@amd.com wrote:

We still return EFI_SUCCESS to the caller when TlsConfigCertificate
returns error, for the use case the platform doesn't require
certificate for the specific HTTP session. This ensures
HttpInitSession function still initiated and returns EFI_SUCCESS to
the caller. The failure is pushed back to TLS DXE driver if the
HTTP communication actually requires certificate.


Reviewed-by: Michael Brown 

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113310): https://edk2.groups.io/g/devel/message/113310
Mute This Topic: https://groups.io/mt/103539579/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/5] NetwokrPkg/HttpDxe: Refactor TlsCreateChild

[Michael Brown]

On 05/01/2024 08:37, abner.ch...@amd.com wrote:

From: Abner Chang 

- Use HTTP instance as the parameter for TlsCreateChild function.
- Install TLS protocol on the HTTP instance thats create TLS child.


Logic looks good to me, just some minor cosmetic comments.

Commit title has "NetwokrPkg" typo, should be "NetworkPkg".


-  @return  The child handle with opened EFI_TLS_PROTOCOL and 
EFI_TLS_CONFIGURATION_PROTOCOL.
+  @return  EFI_SUCCESSTLS child handle is returned in 
HttpInstance->TlsChildHandle
+  with opened EFI_TLS_PROTOCOL and 
EFI_TLS_CONFIGURATION_PROTOCOL.


Comment refers to TlsChildHandle, which no longer exists after this patch.


-  @return  The child handle with opened EFI_TLS_PROTOCOL and 
EFI_TLS_CONFIGURATION_PROTOCOL.
+  @return  EFI_SUCCESSTLS child handle is returned in 
HttpInstance->TlsChildHandle
+  with opened EFI_TLS_PROTOCOL and 
EFI_TLS_CONFIGURATION_PROTOCOL.


As above.

Reviewed-by: Michael Brown 

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113309): https://edk2.groups.io/g/devel/message/113309
Mute This Topic: https://groups.io/mt/103539578/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Michael Brown]

On 02/01/2024 16:31, Chang, Abner via groups.io wrote:

From: Michael Brown 
- Allow the call to Request() to perform its normal TLS configuration
via TlsConfigureSession(), as though the connection were going to
perform host verification etc as per the platform default policy.  This
configuration should succeed, with no error returned.


This is not correct. The first Request would be failed at 
TlsConfigureCertificate here 
https://github.com/tianocore/edk2/blob/master/NetworkPkg/HttpDxe/HttpsSupport.c#L711


I assume this is because we expect that the TlsCaCertificate variable 
may be empty or non-existent?


Would it make sense to have an EFI_NOT_FOUND from TlsConfigCertificate() 
be ignored, as is already done for TlsConfigCipherList() just above? 
Something like:


  Status = TlsConfigCertificate (HttpInstance);
  if (EFI_ERROR (Status) && (Status != EFI_NOT_FOUND)) {
DEBUG ((DEBUG_ERROR, "TLS Certificate Config Error!\n"));
return Status;
  }

i.e. treat an absent TlsCaCertificate variable as meaning "there are no 
explicit CA certificates", allowing TlsDxe to do whatever it does in 
that situation (which is presumably to fail any attempted certificate 
verifications).


That would eliminate this problem in the RedfishRestExDxe case.


and  SetSessionData to EfiTlsVerifyHost here: 
https://github.com/tianocore/edk2/blob/master/NetworkPkg/HttpDxe/HttpsSupport.c#L679.


I don't understand why this call would be expected to fail.  It's not 
performing any verification at this stage, just recording the hostname 
from the URI for subsequent use in certificate verification.


I would expect this call to succeed and record whatever hostname is 
present in the request from RedfishRestExDxe.  This hostname will 
subsequently be ignored for verification, since the HttpEventInitSession 
callback will set EFI_TLS_VERIFY_NONE.



- In the RedfishRestExDxe callback, check for HttpEventInitSession and
use calls to EFI_TLS_CONFIGURATION_PROTOCOL.SetData() to modify the TLS
configuration to e.g. set EFI_TLS_VERIFY_NONE.

Here is the thing. Even we reconfigure TLS configuration data at 
HttpEventInitSession in RestEx, the EfiHttpRequest still returns fail to the 
caller here: 
https://github.com/tianocore/edk2/blob/master/NetworkPkg/HttpDxe/HttpImpl.c#L599.
 Not to mention the reason of failures may not be caused by 
TlsConfigureSession. There are failures for some other reasons in 
HttpInitSession. Also, what the caller suppose to do when it gets error 
returned? How does caller knows the error is just because the TLS configuration 
failure and it has to reconfigure TLS and retry HttpRequest? The logic doesn’t 
make sense if the caller assumes the failure is caused by TLS configure at 
HttpEventInitSession callback. Actually, having a high layer application to 
reconfigure TLS configuration data because the failure caused by not 
well-considered default TLS config values also doesn’t make sense, right?


I would expect this to be resolved by the above suggestions. 
HttpInitSession() should succeed.  The HttpEventInitSession callback 
should be called with an EFI_SUCCESS status code, and there is no need 
for the caller to retry anything.



To make the callback implementation easier, you may want to extend
HttpNotify() to take HTTP_PROTOCOL *HttpInstance as its first parameter,
and then pass HttpInstance->Handle as an additional parameter to the
callback method, i.e.:

typedef
VOID
(EFIAPI *EDKII_HTTP_CALLBACK)(
IN EDKII_HTTP_CALLBACK_PROTOCOL *This,
IN EFI_HANDLE   HttpHandle,
IN EDKII_HTTP_CALLBACK_EVENTEvent,
IN EFI_STATUS   EventStatus
);

We shouldn’t change the prototype as the callback mechanism may used by OEM/ODM 
platform code which is not part of tianocore. This change breaks backward 
compatible. Honestly, leverage HTTP callback function doesn’t really serve the 
purpose well. This is the HttpDxe design defect as we don’t the used case like 
in-band Redfish communication.


OEM/ODM code should restrict itself to using APIs covered by the UEFI 
specification.  If OEM/ODMs choose to rely on EDK2 private 
implementation details (such as EDKII_HTTP_CALLBACK) then they must be 
prepared to update their code when the private implementation detail 
changes.


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113040): https://edk2.groups.io/g/devel/message/113040
Mute This Topic: https://groups.io/mt/103368438/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Michael Brown]

On 02/01/2024 06:06, Chang, Abner via groups.io wrote:

What do you think about:

- installing TLS on HTTP handle (as you have already implemented)

- using EDKII_HTTP_CALLBACK_PROTOCOL to catch the HttpEventInitSession
and perform whatever calls are needed to SetData() to modify the TLS
configuration?


Leverage HttpNotify is good but still has some problems, as HttpNotify is 
designed to notify callback owner about a specific task was done. In order to 
keep this HttpNotify nature, we can create a callback point at the end of 
TlsCreateChild() with a newly introduced event type says 
HttpEventTlsChildCreated. The reason we have to create this notification before 
TlsConfigureSession() is because this function uses the default configuration 
data to configure TLS. However, it doesn't have to do EfiTlsVerifyHost and 
TlsConfigCertificate if there is nothing to verify.
The problem in configuring  EfiTlsVerifyHost is It always checks verification 
method with EFI_TLS_VERIFY_PEER, while the problem of TlsConfigCertificate is 
it considers platform always can provide the certificate.  Anyway to configure 
TLS after TlsConfigCertificate is to late as the error status already returned 
earlier. Furthermore the design of HttpNotify doesn't provide the output 
information for caller to determine the different code paths.  So with above, 
how can we skip configuring TLS again with the default values in HttpSupport.c 
even platform code already configured it before TlsConfigureSession()?


I may not have been clear enough: I'm suggesting that we allow 
TlsConfigureSession() to perform its normal configuration, and then use 
the HttpEventInitSession callback to modify this configuration (e.g. 
setting EFI_TLS_VERIFY_NONE).


Yes, this would mean that a tiny amount of unnecessary work is done 
(e.g. first setting EFI_TLS_VERIFY_PEER, then later changing it to 
EFI_TLS_VERIFY_NONE) but this is a negligible amount of execution time 
and allows us to keep the code simple.


The HttpEventInitSession callback is guaranteed to be called before the 
calls to HttpGenRequestMessage() and HttpTransmitTcp() (even if running 
at TPL_APPLICATION with network polling taking place) and so is 
guaranteed to be a safe point at which to perform additional TLS 
configuration via SetData().


So, to expand on what I wrote before, I'm suggesting:

- refactor TlsCreateChild() to install the TLS protocols on the HTTP 
handle (as you have already implemented).


- (optional) Remove TlsChildHandle and replace with a boolean flag.

- No further changes required to HttpDxe, as far as I can tell.

- In RedfishRestExDxe, install an EDKII_HTTP_CALLBACK_PROTOCOL before 
calling EFI_HTTP_PROTOCOL.Request().


- Allow the call to Request() to perform its normal TLS configuration 
via TlsConfigureSession(), as though the connection were going to 
perform host verification etc as per the platform default policy.  This 
configuration should succeed, with no error returned.


- In the RedfishRestExDxe callback, check for HttpEventInitSession and 
use calls to EFI_TLS_CONFIGURATION_PROTOCOL.SetData() to modify the TLS 
configuration to e.g. set EFI_TLS_VERIFY_NONE.


To make the callback implementation easier, you may want to extend 
HttpNotify() to take HTTP_PROTOCOL *HttpInstance as its first parameter, 
and then pass HttpInstance->Handle as an additional parameter to the 
callback method, i.e.:


typedef
VOID
(EFIAPI *EDKII_HTTP_CALLBACK)(
  IN EDKII_HTTP_CALLBACK_PROTOCOL *This,
  IN EFI_HANDLE   HttpHandle,
  IN EDKII_HTTP_CALLBACK_EVENTEvent,
  IN EFI_STATUS   EventStatus
  );

VOID
HttpNotify (
  IN  HTTP_PROTOCOL  *HttpInstance,
  IN  EDKII_HTTP_CALLBACK_EVENT  Event,
  IN  EFI_STATUS EventStatus
  )
{
  ...
  HttpCallback->Callback (
HttpCallback,
HttpInstance->Handle,
Event,
EventStatus
);
  ...
}

Since EDKII_HTTP_CALLBACK_PROTOCOL is internal to EDK2 (and not covered 
by the UEFI specification), this change should be possible.  I've 
checked all of the HttpNotify() call sites in the EDK2 repo, and they do 
all have an HttpInstance available to use.


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113033): https://edk2.groups.io/g/devel/message/113033
Mute This Topic: https://groups.io/mt/103368438/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Michael Brown]

On 29/12/2023 15:07, Chang, Abner via groups.io wrote:

To locate TLS protocol from the HTTP handle and configure TLS configuration 
data at the return from EfiHttpRequest during that short window of non-blocking 
request is not reliable. It also doesn't make sense to ask upper layer 
application to do this when it first time invokes EfiHttpRequest.
I already refactored TlsCreateChild to install TLS protocol on HTTP handle. I 
also implemented the corresponding code in Redfish REST EX to listen the 
installation of TLS protocol and hook the SetSessionData. It works fine on the 
system, however I really don’t like having the upper layer application to do 
this much just for overriding TLS configuration data. The code looked a 
specific implementation to hack the TLS protocol interface. Plus I still have 
to add few code in TlsConfigCertificate to skip configure certificate with 
checking TlsVerifyMethod.
We should sit back to consider introducing a new protocol for upper layer 
application to provide their own TLS configuration data, as the root cause is 
that hard coded TLS configuration data in HttpSupport.c. We shouldn't have the 
code like that and add the burdens to application.

What my thought is as below and maybe more elegant than the patch a sent,
- Still install TLS on HTTP handle, then upper layer application can listen to 
the installation of EFI TLS protocol to find the correct HTTP handle.
- Move TLS_CONFIG_DATA in a public header file.
- Introduce a new protocol called EDKII_HTTP_TLS_CONFIGURATION_DATA
- Upper layer application installs this protocol with their own TLS_CONFIG_DATA.
- TlsConfigureSession locates EDKII_HTTP_TLS_CONFIGURATION_DATA to replace the 
default TLS_CONFIG_DATA.

This way we can remove that hardcoded code and fix the root cause, also the 
upper layer application do not have to take the burden.
What do you think?


Firstly, thank you very much for taking the time to dig through this and 
work towards a cleaner design - I, for one, really appreciate it.


I think we're completely agreed that installing the TLS protocols on the 
HTTP handle is the right thing to do - that seems to be a clear 
improvement over the status quo where there's no introspectable 
relationship between the two handles.


I'm torn on the use of TLS_CONFIG_DATA.  For better or worse, the 
existing and standardised EFI_TLS_CONFIGURATION_PROTOCOL is verb-based, 
using SetData() and GetData() methods.  Adding a noun-based protocol for 
TLS configuration seems to cut across this, with the potential to look 
confusing: a new reader of the code could legitimately wonder why the 
codebase contains two competing solutions to what is essentially the 
same problem.


Given that the verb-based approach of EFI_TLS_CONFIGURATION_PROTOCOL has 
made it as far as being standardised and included in the UEFI 
specification, I think we probably need to accept that this is the 
"correct" way to perform TLS configuration within UEFI code.  The 
problem with HttpsSupport.c then becomes that there is no good 
opportunity for a consumer to call SetData(), since (a) 
EFI_TLS_CONFIGURATION_PROTOCOL comes into existence only halfway through 
the call to EFI_HTTP_PROTOCOL.Request() and (b) the call to 
TlsConfigureSession() will overwrite the configuration anyway.


Is there a way that TlsConfigureSession() could sensibly provide an 
opportunity for the consumer to make calls to SetData(), so that the 
consumer could cleanly override any default configuration?


Looking through the code, TlsConfigureSession() is called only from 
HttpInitSession(), which in turn is called only from EfiHttpRequest(). 
This call is followed immediately by the line:


  HttpNotify (HttpEventInitSession, Status);

which seems to already use an existing EDKII_HTTP_CALLBACK_PROTOCOL to 
notify an arbitrary list of interested consumers that an event has taken 
place (in this case, that a session has just been initialised).


What do you think about:

- installing TLS on HTTP handle (as you have already implemented)

- using EDKII_HTTP_CALLBACK_PROTOCOL to catch the HttpEventInitSession 
and perform whatever calls are needed to SetData() to modify the TLS 
configuration?


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113015): https://edk2.groups.io/g/devel/message/113015
Mute This Topic: https://groups.io/mt/103368438/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/5] NetworkPkg/HttpDxe: Refactor TlsCreateChild function

[Michael Brown]

On 30/12/2023 11:29, abner.ch...@amd.com wrote:

+  @return  EFI_SUCCESSTLS child handle is returned in 
HttpInstance->TlsChildHandle
+  with opened EFI_TLS_PROTOCOL and 
EFI_TLS_CONFIGURATION_PROTOCOL.


All looks good to me, but do we need to retain 
HttpInstance->TlsChildHandle as a separate EFI_HANDLE field?  Now that 
EFI_TLS_PROTOCOL is installed on the same handle, it seems to function 
solely as a flag to indicate that we have already called 
TlsCreateChild(), in which case an EFI_BOOLEAN might be clearer?


With or without the above suggestion, I'm happy to add

Reviewed-by: Michael Brown 

for this patch.

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113014): https://edk2.groups.io/g/devel/message/113014
Mute This Topic: https://groups.io/mt/103430430/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Michael Brown]

On 28/12/2023 23:37, Chang, Abner via groups.io wrote:

As far as I am aware, EfiHttpRequest sets up all of the relevant data
structures but functions as a non-blocking open.  If you reconfigure the
TLS session immediately after return from EfiHttpRequest() then this
reconfiguration should take effect before any network packets have been
transmitted or received.  I have not tested this, though.

If the immediate reconfiguration does not work, then your suggestion of
hooking SetSessionData() sounds like the easiest approach.

I think the non-blocking transfer still sends out the request but just not 
waiting the response there, have to check the implementation.


The code seems to construct the HTTP request and enqueue it, but unless 
it blocks polling on the network somewhere then the most it can do in 
terms of network I/O is to send out the initial TCP SYN.  (Not even 
that, if a DNS lookup is required.)


The implementation could plausibly construct and enqueue the 
ClientHello, in which case it would be too late to modify the cipher 
suite list, but any attempt to verify the hostname definitely can't 
happen until a lot of network I/O has taken place.


Good luck! :)

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112995): https://edk2.groups.io/g/devel/message/112995
Mute This Topic: https://groups.io/mt/103368438/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Michael Brown]

On 28/12/2023 15:04, Chang, Abner via groups.io wrote:

With the TLS protocol installed onto the same handle, I don't think you
then even need to use RegisterProtocolNotify().  On return from
EFI_HTTP_PROTOCOL.Request() you can open the TLS protocol on the handle
and immediately call SetSessionData() to override VerifyMethod etc.


This part I am not sure, as TLS is initiated on the first HttpRequest. 
Reconfigure TLS session on return from HTTP Request function means we have to 
take one time error. I think I will still use RegisterProtocolNotify and 
LocateHandle with ByRegisterNotify to get the newly installed TLS handle, then 
check it with REST EX HTTP handle. Hook the TLS SetSessionData() function 
provided by REST EX, override the value then invoke the original 
SetSessionData(). Something like this.


As far as I am aware, EfiHttpRequest sets up all of the relevant data 
structures but functions as a non-blocking open.  If you reconfigure the 
TLS session immediately after return from EfiHttpRequest() then this 
reconfiguration should take effect before any network packets have been 
transmitted or received.  I have not tested this, though.


If the immediate reconfiguration does not work, then your suggestion of 
hooking SetSessionData() sounds like the easiest approach.



Would you like to refactor HttpSupport.c or let me do that?


Nobody is paying me to work on EDK2, so I'll leave it to you.  :)

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112982): https://edk2.groups.io/g/devel/message/112982
Mute This Topic: https://groups.io/mt/103368438/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Michael Brown]

On 28/12/2023 02:47, Chang, Abner via groups.io wrote:

On 26/12/2023 11:28, Chang, Abner via groups.io wrote:

Platform developer can provide this protoocl to EFI HTTP driver to
configure TLS using TLS conifg data provided by
EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL for the specific HTTP
protocol handle. How to distinguish the correct HTTP protocol
handle for the platform TLS policy is outside the scope of this
change. For Redfish, we will provide this protocol in EFI Redfish
REST EX driver.


This looks messy to me.

Did you try my suggestion of using RegisterProtocolNotify() in order to
register a callback that will be called for any new instances of
EFI_TLS_PROTOCOL?

This would be functionally equivalent to your patch, but with zero lines
of additional code required in HttpDxe.


I think you suggest to hook/replace the EFI_TLS_PROTOCOL for the specific HTTP 
handle?
EFI_TLS_PROTOCOL is installed implicitly when the first time HTTPs request is 
performed. There is no connection between HTTP handle and EFI TLS protocol 
instance besides the HTTP driver internal structure.
Listen to the installation of EFI_TLS_PROTOCOL has no way to distinguish the 
dedicated HTTP handle, for example the HTTP handle created by Redfish REST EX 
driver.
I don’t see the chance to provide the flexibility to TLS config with using 
RegisterProtocolNotify for EFI_TLS_PROTOCO unless we add one line to install 
the same TLS_PTOTOCOL on the given HTTP instance.  Or something I missed?


Having looked through the code in more detail, you are correct that 
there is no connection between the TLS handle and the HTTP handle, since 
TlsCreateChild() in HttpsSupport.c currently chooses to call 
(*TlsSb)->CreateChild() with a NULL handle and does not ever call 
OpenProtocol() with BY_CHILD to set up a parent/child relationship.


I would therefore suggest a mild refactoring of TlsCreateChild() so that 
it installs the TLS protocols directly onto the HTTP handle.  This 
refactoring does not break any APIs, since TlsCreateChild() is purely 
internal to HttpDxe.


Since all other functions in HttpsSupport.c seem to take HttpInstance as 
the first parameter, I would probably change TlsCreateChild() to do the 
same:


EFI_STATUS
EFIAPI
TlsCreateChild (
  IN OUT HTTP_PROTOCOL  *HttpInstance
  )

There is only one call site of TlsCreateChild() (in EfiHttpRequest()) 
and so the most obvious approach would be to move the logic around the 
call site to be inside of TlsCreateChild():


  if (HttpInstance->LocalAddressIsIPv6) {
ImageHandle = HttpInstance->Service->Ip6DriverBindingHandle;
  } else {
ImageHandle = HttpInstance->Service->Ip4DriverBindingHandle;
  }

and then use HttpInstance->TlsSb in place of the current *TlsSb, etc, 
within TlsCreateChild().


The call within TlsCreateChild() to HttpInstance->TlsSb->CreateChild() 
can then pass >Handle as the second parameter, so that the 
EFI_TLS_PROTOCOL is installed onto the HTTP handle.


This refactoring would have the side effect of cleaning up 
TlsCreateChild() to be consistent with other functions in the same file, 
and would allow it to return an EFI_STATUS for more meaningful error 
reporting.


With the TLS protocol installed onto the same handle, I don't think you 
then even need to use RegisterProtocolNotify().  On return from 
EFI_HTTP_PROTOCOL.Request() you can open the TLS protocol on the handle 
and immediately call SetSessionData() to override VerifyMethod etc.


What do you think?

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112980): https://edk2.groups.io/g/devel/message/112980
Mute This Topic: https://groups.io/mt/103368438/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Michael Brown]

On 26/12/2023 11:28, Chang, Abner via groups.io wrote:

For the HTTPS connetion that doesn't require TLS peer verification,
EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL is introduced to platform
developer to provide the TLS configure data that is different than
the default TLS configuration. The use case such as Redfish service
connction which doesn't require the TLS peer verification on the
cetificate, especially to the Redfish service connection through
the in-band network interface.

Platform developer can provide this protoocl to EFI HTTP driver to
configure TLS using TLS conifg data provided by
EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL for the specific HTTP
protocol handle. How to distinguish the correct HTTP protocol
handle for the platform TLS policy is outside the scope of this
change. For Redfish, we will provide this protocol in EFI Redfish
REST EX driver.


This looks messy to me.

Did you try my suggestion of using RegisterProtocolNotify() in order to 
register a callback that will be called for any new instances of 
EFI_TLS_PROTOCOL?


This would be functionally equivalent to your patch, but with zero lines 
of additional code required in HttpDxe.


(My apologies if you did try it and already found a reason why it would 
not work - I have not been able to keep up with all EDK2 list messages.)


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112932): https://edk2.groups.io/g/devel/message/112932
Mute This Topic: https://groups.io/mt/103368438/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdeModulePkg/PciHostBridgeDxe: Add readback after final Cfg-Write

[Michael Brown]

On 03/11/2023 16:03, Kinney, Michael D wrote:

PCI Configuration read/write operations are non-posted, so the PCI
Configuration write operation should complete without the need for
an additional transaction.  If you are seeing an issue, then it may
be in the implementation of the PciLib/PciSegmentLib that is not
guaranteeing this non-posted behavior.


From the PCIe spec:

  "The ECAM converts memory transactions from the host CPU into
   Configuration Requests on the PCI Express fabric. This conversion
   potentially creates ordering problems for the software, because
   writes to memory addresses are typically posted transactions but
   writes to Configuration Space are not posted on the PCI Express
   fabric."

My understanding from the above is that the posted write occurs at the 
level of the CPU performing a memory write to the ECAM window.  By the 
time this write reaches the ECAM and becomes a non-posted write 
transaction within the downstream PCIe fabric, the CPU has already moved 
on and is not waiting for completion of the ECAM memory write.


Reading back from the PCI configuration register will cause the CPU to 
wait until the read from the ECAM window has completed, which cannot 
happen until the corresponding downstream PCI configuration read has 
completed.  Since the CPU will (with appropriate barrier operations) not 
reorder the memory read ahead of the preceding memory write, the overall 
effect is to guarantee that the memory write has reached the ECAM, and 
that the memory write reached the ECAM before the subsequent memory read 
from the same location.


There is an implicit assumption in the above that the ECAM and PCIe 
fabric will themselves not reorder the PCI configuration read ahead of 
the PCI configuration write.  Unfortunately, on a closer reading of the 
spec, this may not be a valid assumption: other parts of the PCIe spec 
state that non-posted transactions (e.g. configuration reads and writes) 
may be freely reordered.


It seems slightly insane that PCIe bridges would be allowed to 
arbitrarily reorder configuration cycles to downstream devices, but 
that's what the spec seems to state.  The upshot seems to be that:


a) software is entirely responsible for ensuring that PCI configuration 
writes via ECAM have completed, and


b) software has no available mechanism for ensuring that PCI 
configuration writes via ECAM have completed.


Ard: any alternative suggestions on ways we can wait for completion, 
given that even reading back from the PCI configuration register is 
apparently not guaranteed?


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110646): https://edk2.groups.io/g/devel/message/110646
Mute This Topic: https://groups.io/mt/102354842/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC] Ordering of Arm PCI ECAM and MMIO operations

[Michael Brown]

On 01/11/2023 20:17, Joe L wrote:

Thanks for the discussion,

Are we saying that DataSynchronizationBarrier () before the return 
from RootBridgeIoPciAccess() is not strong enough to determine that 
the final ECAM write would have completed? According to Learn the 
architecture - ARMv8-A memory systems 
 the 
DSB should block "execution of any further instructions, not just 
loads or stores, until synchronization is complete". To me this means

that for Arm the DSB will stall any subsequent instructions until the
completion for the ECAM write is received by the processor.


I honestly can't tell from the wording there whether or not DSB would
guarantee that the configuration space write has completed all the way 
to the target PCIe device (as opposed to e.g. completing as far as the 
host bridge, or to an intermediate PCIe bridge).


I don't know the answer, and it feels like the kind of messy area where 
adding a barrier will definitely reduce the probability of the issue 
occurring but might not guarantee a fix, which is a bad situation to be 
left in.


Though if an architecture-agnostic solution is desired the readback 
before returning from RootBridgeIoPciAccess() does make sense.


Personally I like having an architecture-agnostic solution, and I'll be
updating the ECAM driver in iPXE to use the readback approach.

If the access spanned multiple DWORDs then should a read from the 
final aligned DWORD in the "buffer" be sufficient?


Quite possibly.  Given that PCIe configuration space accesses are never 
performance-critical, I'm not sure it's worth the optimisation, but I'll 
defer to the people who actually have to implement it in EDK2.


Thanks,

Michael


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110493): https://edk2.groups.io/g/devel/message/110493
Mute This Topic: https://groups.io/mt/102310377/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC] Ordering of Arm PCI ECAM and MMIO operations

[Michael Brown]

On 01/11/2023 12:51, Ard Biesheuvel wrote:

On Wed, 1 Nov 2023 at 13:25, Michael Brown  wrote:

By my reading, the PCIe specification seems to therefore require
something stronger than an ordering guarantee: it requires the ability
for software to make a standalone determination that the write has
*completed*, independent of the existence of any subsequent I/O operations.


indeed, thanks for bringing this up.


The PCIe specification does not mandate that any particular mechanism be
used, but it does require that the processor and/or host bridge provides
*some* mechanism for software to determine that the ECAM write has
completed.

What mechanism does ARM (or the host bridge) provide to determine
completion of an ECAM write?


A MMIO read of the same location should ensure that the MMIO write has
completed by the time the read returns. Not sure whether or not there
are any other requirements (e.g., wrt.the size of the read vs the size
of the write).


That seems to suggest that a logical PCIe configuration space write 
operation using ECAM should probably always comprise:


1. ECAM write
2. ECAM read from the same location (using the same size)

If reads are not allowed to have side effects (e.g. read-clear 
registers) then this seems safe.  The PCIe specification "Configuration 
Register Types" list comprises (in version 3.0, at least):


  HwInit - read-only, no read side effects

  RO - read-only, no read side effects

  RW - read-write, no read side effects

  RW1C - write 1 to clear bits, no read side effects

  ROS - read-only, no read side effects

  RWS - read-write, no read side effects

  RW1CS - write 1 to clear bits, no read side effects

  RsvdP - read-write, no read side effects

  RsvdZ - read-write, no read side effects

So, unless newer versions of the PCIe specification have allowed for the 
existence of configuration register types with read side effects, then 
the approach of always reading back from ECAM seems to be safe for any 
conforming PCIe device.


I would therefore suggest that all ECAM driver implementation functions 
in EDK2 (e.g. PciExpressWrite32(), PciExpressOr32(), 
PciSegmentWrite32(), etc) be updated to add the relevant ECAM read 
following the write operation.


PCI configuration space writes are never fast-path operations (in any 
sane hardware), and so the delay introduced by the read should not be 
significant.


Does this seem like a sensible solution?

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110480): https://edk2.groups.io/g/devel/message/110480
Mute This Topic: https://groups.io/mt/102310377/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC] Ordering of Arm PCI ECAM and MMIO operations

[Michael Brown]

On 01/11/2023 09:56, Ard Biesheuvel wrote:

On Wed, 1 Nov 2023 at 03:09, Pedro Falcato  wrote:

On Wed, Nov 1, 2023 at 12:40 AM Joe L  wrote:

Our CMN TRM showcases an example where ECAM and MMIO are two different regions 
in the HN-I SAM. The implication is that we would expect a DSB between the ECAM 
write and MMIO read. I'm asking our Open Source Software group to confirm that 
standard PCIe software is generally expected to be aware of the need for a 
DSB--but my impression from talking to some of our hardware engineers is that 
that is indeed the expectation.




1) as per the architecture (as interpreted by the ARM architects), a
DSB is required to ensure that the side effects of enabling a MMIO BAR
in the PCI config space are sufficiently observable to memory accesses
to that BAR that appear after the PCI config space access in the
program.


It's possibly worth mentioning what the PCIe specification requires in 
terms of ECAM ordering:


  "As an example, software may wish to configure a device Function’s
   Base Address register by writing to the device using the ECAM,
   and then read a location in the memory-mapped range described by
   this Base Address register. If the software were to issue the
   memory-mapped read before the ECAM write was completed, it would
   be possible for the memory-mapped read to be re-ordered and arrive
   at the device before the Configuration Write Request, thus causing
   unpredictable results.

   To avoid this problem, processor and host bridge implementations must
   ensure that a method exists for the software to determine when the
   write using the ECAM is completed by the completer."

By my reading, the PCIe specification seems to therefore require 
something stronger than an ordering guarantee: it requires the ability 
for software to make a standalone determination that the write has 
*completed*, independent of the existence of any subsequent I/O operations.


As a practical example of when this might be relevant: software could be 
writing to device configuration space to disable bus mastering as part 
of a reset or shutdown sequence, in order to guarantee that the device 
will initiate no further DMA operations and that any DMA buffers 
allocated to the device can be freed and reused.  In this situation, 
there may be no subsequent MMIO read or write to the device, and so 
there is no way to rely upon an ordering guarantee to satisfy the 
requirement.


Any solution involving ordering guarantees can therefore mask the 
problem in some situations, but cannot solve it.


The PCIe specification does not mandate that any particular mechanism be 
used, but it does require that the processor and/or host bridge provides 
*some* mechanism for software to determine that the ECAM write has 
completed.


What mechanism does ARM (or the host bridge) provide to determine 
completion of an ECAM write?


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110477): https://edk2.groups.io/g/devel/message/110477
Mute This Topic: https://groups.io/mt/102310377/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] efi and ext4 and case sensitive file names

[Michael Brown]

On 29/09/2023 10:47, Marvin Häuser wrote:

Maybe when Linux starts adhering the spec for file names (the spec clearly 
defines e.g. BOOTx64.EFI, while at least some distros/images use bootx64.efi), 
this can be discussed. :) Let's not break various GRUB setups...


Seems slightly unfair to blame Linux when EDK2 also apparently gets it 
wrong (in a different way)... :)


//
// EFI File location to boot from on removable media devices
//
...
#define EFI_REMOVABLE_MEDIA_FILE_NAME_X64 L"\\EFI\\BOOT\\BOOTX64.EFI"

Is this something worth fixing in UefiSpec.h?

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109194): https://edk2.groups.io/g/devel/message/109194
Mute This Topic: https://groups.io/mt/101615699/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-libc Patch 1/1] edk2-libc/StdLib: Fix uninitialized global variable

[Michael Brown]

On 25/07/2023 17:07, Jayaprakash, N wrote:

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4506

res_init() is called from different places in sockets library. It depends
on global _res variable containing a state.

The problem is that if __BIND_RES_TEXT macro is not defined, _res is not
initialized. Depending on compiler and build optimization this can fill the
variable with garbage that is later used by res_init().

Fix is trivial - explicitly initialize _res.
  
  struct __res_state _res

-# if defined(__BIND_RES_TEXT)
+#if defined(__BIND_RES_TEXT)
  = { RES_TIMEOUT, }  /* Motorola, et al. */
-# endif
+#endif
+= {0}
+#endif
  ;


NAK.  This is very wrong.

Firstly, your patch introduces unnecessary whitespace changes and so is 
unnecessarily cumbersome to review.


Secondly, the patch creates an invalid "#if ... #endif ... #endif" 
combination.  I suspect that you meant to use "#else" in the middle.


Thirdly, and most importantly, the whole patch is entirely unnecessary. 
As a variable with static storage duration, if no explicit initializer 
is provided then the C language guarantees that the value will be 
initialized to zero anyway.


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107268): https://edk2.groups.io/g/devel/message/107268
Mute This Topic: https://groups.io/mt/100353380/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] OvmfPkg/IoMmuDxe: add locking to IoMmuAllocateBounceBuffer

[Michael Brown]

On 19/07/2023 17:52, Ard Biesheuvel wrote:

On Wed, 19 Jul 2023 at 18:32, Gerd Hoffmann  wrote:

On Wed, Jul 19, 2023 at 04:04:28PM +, Michael Brown wrote:

It looks as though IoMmuFreeBounceBuffer() should also raise to TPL_NOTIFY
while modifying mReservedMemBitmap, since the modification made in
IoMmuFreeBounceBuffer() is not an atomic operation:

   mReservedMemBitmap &= (UINT32)(~MapInfo->ReservedMemBitmap);


I'd expect modern compilers optimize that to a single instruction,


You mean something along the lines of

   andl %reg, mReservedMemBitmap(%rip)

right?


Even with a single orl/andl instruction, the operation is unlocked. 
It's guaranteed atomic against interrupts (since interrupts always occur 
at instruction boundaries) but it's not guaranteed atomic against 
concurrent accesses to the same global variable from other processors.


(I have no idea if the UEFI model allows APs to call into the IOMMU 
protocol or not, so I don't know if this is a real problem.)


On a quick review of the code, there appear to be other points that also 
modify mReservedMemBitmap (IoMmuAllocateCommonBuffer() and 
IoMmuFreeCommonBuffer()).  I'd guess that these also need to raise to 
TPL_NOTIFY, but I'm not familiar with the code so I don't know if 
there's anything that makes this unnecessary.


Sorry not to be more help.

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107060): https://edk2.groups.io/g/devel/message/107060
Mute This Topic: https://groups.io/mt/100233359/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] OvmfPkg/IoMmuDxe: add locking to IoMmuAllocateBounceBuffer

[Michael Brown]

On 19/07/2023 12:33, Gerd Hoffmann wrote:

Searching for an unused bounce buffer in mReservedMemBitmap and
reserving the buffer by flipping the bit is a critical section
which must not be interrupted.  Raise the TPL level to ensure
that.

Without this fix it can happen that IoMmuDxe hands out the same
bounce buffer twice, causing trouble down the road.  Seen happening
in practice with VirtioNetDxe setting up the network interface (and
calling into IoMmuDxe from a polling timer callback) in parallel with
Boot Manager doing some disk I/O.  An ASSERT() in VirtioNet caught
the buffer inconsistency.

Full story with lots of details and discussions is available here:
https://bugzilla.redhat.com/show_bug.cgi?id=2211060

Signed-off-by: Gerd Hoffmann 
---
  OvmfPkg/IoMmuDxe/IoMmuBuffer.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
index c8f6cf4818e8..7f8a0368ab5d 100644
--- a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
+++ b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
@@ -367,7 +367,9 @@ IoMmuAllocateBounceBuffer (
  {
EFI_STATUS  Status;
UINT32  ReservedMemBitmap;
+  EFI_TPL OldTpl;
  
+  OldTpl= gBS->RaiseTPL (TPL_NOTIFY);

ReservedMemBitmap = 0;
Status= InternalAllocateBuffer (
  Type,
@@ -378,6 +380,7 @@ IoMmuAllocateBounceBuffer (
  );
MapInfo->ReservedMemBitmap = ReservedMemBitmap;
mReservedMemBitmap|= ReservedMemBitmap;
+  gBS->RestoreTPL (OldTpl);
  
ASSERT (Status == EFI_SUCCESS);


It looks as though IoMmuFreeBounceBuffer() should also raise to 
TPL_NOTIFY while modifying mReservedMemBitmap, since the modification 
made in IoMmuFreeBounceBuffer() is not an atomic operation:


  mReservedMemBitmap &= (UINT32)(~MapInfo->ReservedMemBitmap);

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107056): https://edk2.groups.io/g/devel/message/107056
Mute This Topic: https://groups.io/mt/100233359/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

[Michael Brown]

On 06/07/2023 15:19, Henz, Patrick wrote:

I agree that XhcGetElapsedTime() would be better off in TimerLib, but I wasn't 
sure how the community would feel about adding to the interface.


My understanding is that the TimerLib API is not prescribed by any 
standards document, and that this change would add a new function 
without changing any existing functions and so would not break any 
existing users of TimerLib.  On that basis, I would be firmly in favour 
of extending TimerLib to include this functionality.


I will defer to actual EDK2 maintainers on this, however.  :)

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106709): https://edk2.groups.io/g/devel/message/106709
Mute This Topic: https://groups.io/mt/99972791/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Use Performance Timer for XHCI Timeouts

[Michael Brown]

On 05/07/2023 21:15, Henz, Patrick wrote:

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2948

XhciDxe uses the timer functionality provided by the
boot services table to detect timeout conditions. This
breaks the driver's ExitBootServices call back, as
CoreExitBootServices halts the timer before signaling
the ExitBootServices event. If the host controller
fails to halt in the call back, the timeout condition
will never occur and the boot gets stuck in an indefinite
spin loop. Use the free running timer provided by
TimerLib to calculate timeouts, avoiding the potential
hang.


Two points:

1. The XhcGetElapsedTime() function feels like a generally reusable 
abstraction that should exist within TimerLib, rather than being 
open-coded within XhciDxe.


2. Is the performance counter guaranteed to exist and work as expected 
on all platforms and CPU architectures?  (For example: what if the CPU 
does not support a constant TSC?)


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106699): https://edk2.groups.io/g/devel/message/106699
Mute This Topic: https://groups.io/mt/99972791/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/2] OvmfPkg: Relax assertion that interrupts do not occur at TPL_HIGH_LEVEL

[Michael Brown]

On 09/05/2023 14:31, Laszlo Ersek wrote:

On 5/9/23 14:09, Michael Brown wrote:

At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI
specification) and so we should never encounter a situation in which
an interrupt occurs at TPL_HIGH_LEVEL. The specification also
restricts usage of TPL_HIGH_LEVEL to the firmware itself.

However, nothing actually prevents a UEFI application from calling
gBS->RaiseTPL(TPL_HIGH_LEVEL) and then violating the invariant by
enabling interrupts via the STI or equivalent instruction. Some
versions of the Microsoft Windows bootloader are known to do this.

NestedInterruptTplLib maintains the invariant that interrupts are
disabled at TPL_HIGH_LEVEL (even when performing the dark art of
deliberately manipulating the stack so that IRET will return with
interrupts still disabled), but does not itself rely on external code
maintaining this invariant.

Relax the assertion that the interrupted TPL is below TPL_HIGH_LEVEL
to an error message, to allow UEFI applications such as these versions
of the Microsoft Windows bootloader to continue to function.

Ref: https://bugzilla.redhat.com/show_bug.cgi?id=2189136
Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Oliver Steffen 
Cc: Pawel Polawski 
Cc: Jiewen Yao 
Cc: Ard Biesheuvel 
Cc: Jordan Justen 

Michael Brown (2):
   OvmfPkg: Clarify invariants for NestedInterruptTplLib
   OvmfPkg: Relax assertion that interrupts do not occur at
 TPL_HIGH_LEVEL

  OvmfPkg/Library/NestedInterruptTplLib/Tpl.c | 31 +
  1 file changed, 26 insertions(+), 5 deletions(-)



series
Acked-by: Laszlo Ersek 


Thank you!

Gerd: are you happy for your Reviewed-by to stand, since the only 
changes since v1 were to comment wording?  (My apologies for forgetting 
to include a v2 description in the cover letter.)


Thanks,

Michael




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104380): https://edk2.groups.io/g/devel/message/104380
Mute This Topic: https://groups.io/mt/98782100/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 2/2] OvmfPkg: Relax assertion that interrupts do not occur at TPL_HIGH_LEVEL

[Michael Brown]

At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI
specification) and so we should never encounter a situation in which
an interrupt occurs at TPL_HIGH_LEVEL.  The specification also
restricts usage of TPL_HIGH_LEVEL to the firmware itself.

However, nothing actually prevents a UEFI application from calling
gBS->RaiseTPL(TPL_HIGH_LEVEL) and then violating the invariant by
enabling interrupts via the STI or equivalent instruction.  Some
versions of the Microsoft Windows bootloader are known to do this.

NestedInterruptTplLib maintains the invariant that interrupts are
disabled at TPL_HIGH_LEVEL (even when performing the dark art of
deliberately manipulating the stack so that IRET will return with
interrupts still disabled), but does not itself rely on external code
maintaining this invariant.

Relax the assertion that the interrupted TPL is below TPL_HIGH_LEVEL
to an error message, to allow UEFI applications such as these versions
of the Microsoft Windows bootloader to continue to function.

Debugged-by: Gerd Hoffmann 
Debugged-by: Laszlo Ersek 
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=2189136
Signed-off-by: Michael Brown 
---
 OvmfPkg/Library/NestedInterruptTplLib/Tpl.c | 21 ++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c 
b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
index e921a09c5599..d56c12a44529 100644
--- a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
+++ b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
@@ -34,12 +34,27 @@ NestedInterruptRaiseTPL (
 
   //
   // Raise TPL and assert that we were called from within an interrupt
-  // handler (i.e. with TPL below TPL_HIGH_LEVEL but with interrupts
-  // disabled).
+  // handler (i.e. with interrupts already disabled before raising the
+  // TPL).
   //
   ASSERT (GetInterruptState () == FALSE);
   InterruptedTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
-  ASSERT (InterruptedTPL < TPL_HIGH_LEVEL);
+
+  //
+  // At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI
+  // specification) and so we should never encounter a situation in
+  // which InterruptedTPL==TPL_HIGH_LEVEL.  The specification also
+  // restricts usage of TPL_HIGH_LEVEL to the firmware itself.
+  //
+  // However, nothing actually prevents a UEFI application from
+  // invalidly calling gBS->RaiseTPL(TPL_HIGH_LEVEL) and then
+  // violating the invariant by enabling interrupts via the STI or
+  // equivalent instruction.  Some versions of the Microsoft Windows
+  // bootloader are known to do this.
+  //
+  if (InterruptedTPL >= TPL_HIGH_LEVEL) {
+DEBUG ((DEBUG_ERROR, "ERROR: Interrupts enabled at TPL_HIGH_LEVEL!\n"));
+  }
 
   return InterruptedTPL;
 }
-- 
2.39.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104368): https://edk2.groups.io/g/devel/message/104368
Mute This Topic: https://groups.io/mt/98782105/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 1/2] OvmfPkg: Clarify invariants for NestedInterruptTplLib

[Michael Brown]

NestedInterruptTplLib relies on CPU interrupts being disabled to
guarantee exclusive (and hence atomic) access to the shared state in
IsrState.  Nothing in the calling interrupt handler should have
re-enabled interrupts before calling NestedInterruptRestoreTPL(), and
the loop in NestedInterruptRestoreTPL() itself maintains the invariant
that interrupts are disabled at the start of each iteration.

Add assertions to clarify this invariant, and expand the comments
around the calls to RestoreTPL() and DisableInterrupts() to clarify
the expectations around enabling and disabling interrupts.

Signed-off-by: Michael Brown 
---
 OvmfPkg/Library/NestedInterruptTplLib/Tpl.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c 
b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
index e19d98878eb7..e921a09c5599 100644
--- a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
+++ b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
@@ -104,6 +104,7 @@ NestedInterruptRestoreTPL (
   // defer our call to RestoreTPL() to the in-progress outer instance
   // of the same interrupt handler.
   //
+  ASSERT (GetInterruptState () == FALSE);
   if (InterruptedTPL == IsrState->InProgressRestoreTPL) {
 //
 // Trigger outer instance of this interrupt handler to perform the
@@ -153,6 +154,7 @@ NestedInterruptRestoreTPL (
 //
 // Check shared state loop invariants.
 //
+ASSERT (GetInterruptState () == FALSE);
 ASSERT (IsrState->InProgressRestoreTPL < InterruptedTPL);
 ASSERT (IsrState->DeferredRestoreTPL == FALSE);
 
@@ -167,13 +169,17 @@ NestedInterruptRestoreTPL (
 
 //
 // Call RestoreTPL() to allow event notifications to be
-// dispatched.  This will implicitly re-enable interrupts.
+// dispatched.  This will implicitly re-enable interrupts (if
+// InterruptedTPL is below TPL_HIGH_LEVEL), even though we are
+// still inside the interrupt handler.
 //
 gBS->RestoreTPL (InterruptedTPL);
 
 //
 // Re-disable interrupts after the call to RestoreTPL() to ensure
-// that we have exclusive access to the shared state.
+// that we have exclusive access to the shared state.  Interrupts
+// will be re-enabled by the IRET or equivalent instruction when
+// we subsequently return from the interrupt handler.
 //
 DisableInterrupts ();
 
-- 
2.39.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104367): https://edk2.groups.io/g/devel/message/104367
Mute This Topic: https://groups.io/mt/98782101/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 0/2] OvmfPkg: Relax assertion that interrupts do not occur at TPL_HIGH_LEVEL

[Michael Brown]

At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI
specification) and so we should never encounter a situation in which
an interrupt occurs at TPL_HIGH_LEVEL. The specification also
restricts usage of TPL_HIGH_LEVEL to the firmware itself.

However, nothing actually prevents a UEFI application from calling
gBS->RaiseTPL(TPL_HIGH_LEVEL) and then violating the invariant by
enabling interrupts via the STI or equivalent instruction. Some
versions of the Microsoft Windows bootloader are known to do this.

NestedInterruptTplLib maintains the invariant that interrupts are
disabled at TPL_HIGH_LEVEL (even when performing the dark art of
deliberately manipulating the stack so that IRET will return with
interrupts still disabled), but does not itself rely on external code
maintaining this invariant.

Relax the assertion that the interrupted TPL is below TPL_HIGH_LEVEL
to an error message, to allow UEFI applications such as these versions
of the Microsoft Windows bootloader to continue to function.

Ref: https://bugzilla.redhat.com/show_bug.cgi?id=2189136
Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Oliver Steffen 
Cc: Pawel Polawski 
Cc: Jiewen Yao 
Cc: Ard Biesheuvel 
Cc: Jordan Justen 

Michael Brown (2):
  OvmfPkg: Clarify invariants for NestedInterruptTplLib
  OvmfPkg: Relax assertion that interrupts do not occur at
TPL_HIGH_LEVEL

 OvmfPkg/Library/NestedInterruptTplLib/Tpl.c | 31 +
 1 file changed, 26 insertions(+), 5 deletions(-)

-- 
2.39.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104366): https://edk2.groups.io/g/devel/message/104366
Mute This Topic: https://groups.io/mt/98782100/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] OvmfPkg: Relax assertion that interrupts do not occur at TPL_HIGH_LEVEL

[Michael Brown]

On 09/05/2023 09:43, Laszlo Ersek wrote:

I'm not subscribed to the list, so I don't have a copy of patch#1. I've
checked patch#1 at this URL:

https://listman.redhat.com/archives/edk2-devel-archive/2023-May/063591.html

and I'll comment on it using the cover letter:

I really like that patch, with one stylistic exception: in edk2,
explicit FALSE and TRUE comparisons are not desired. So I suggest:

   ASSERT (!GetInterruptState ());

Twice.

In fact, I *think* that if you run uncrustify with the edk2 config on
the patch, then it will rewrite that code.


I built and ran uncrustify with the edk2 config but it did not modify 
the code.  (I did check that it would fix other deliberate errors such 
as extra whitespace, so I don't think this was an error in my setup.)


I will send through v2 with the explicit "== FALSE" still present, for 
consistency with the rest of the code in that file.  (I think I vaguely 
remember someone asking me to add the explicit comparisons when I first 
submitted the code.)  I'm happy for there to be a follow up patch to 
change the coding style to remove all of the explicit boolean 
comparisons, if that is what is wanted.


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104365): https://edk2.groups.io/g/devel/message/104365
Mute This Topic: https://groups.io/mt/98771393/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] OvmfPkg: Relax assertion that interrupts do not occur at TPL_HIGH_LEVEL

[Michael Brown]

On 09/05/2023 09:35, Laszlo Ersek wrote:

Thank you for the patch; I do apologize about splitting hairs. The
debugging was difficult, and you *are* working around a bug here -- but
I'd really like our tone of voice to be positive here, simply because of
the stunningly positive attitude I've experienced from Microsoft.


No problem.  Patch v2 to follow.

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104364): https://edk2.groups.io/g/devel/message/104364
Mute This Topic: https://groups.io/mt/98771399/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 2/2] OvmfPkg: Relax assertion that interrupts do not occur at TPL_HIGH_LEVEL

[Michael Brown]

At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI
specification) and so we should never encounter a situation in which
an interrupt occurs at TPL_HIGH_LEVEL.  The specification also
restricts usage of TPL_HIGH_LEVEL to the firmware itself.

However, nothing prevents a rogue UEFI application from illegally
calling gBS->RaiseTPL(TPL_HIGH_LEVEL) and then deliberately violating
the invariant by enabling interrupts via the STI or equivalent
instruction.  Some versions of the Microsoft Windows bootloader are
known to do this.

NestedInterruptTplLib maintains the invariant that interrupts are
disabled at TPL_HIGH_LEVEL (even when performing the dark art of
deliberately manipulating the stack so that IRET will return with
interrupts still disabled), but does not itself rely on external code
maintaining this invariant.

Relax the assertion that the interrupted TPL is below TPL_HIGH_LEVEL
to an error message, to allow rogue UEFI applications such as the
Microsoft Windows bootloader to continue to function.

Debugged-by: Gerd Hoffmann 
Debugged-by: Laszlo Ersek 
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=2189136
Signed-off-by: Michael Brown 
---
 OvmfPkg/Library/NestedInterruptTplLib/Tpl.c | 21 ++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c 
b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
index e921a09c5599..a91f2d3cb8c7 100644
--- a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
+++ b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
@@ -34,12 +34,27 @@ NestedInterruptRaiseTPL (
 
   //
   // Raise TPL and assert that we were called from within an interrupt
-  // handler (i.e. with TPL below TPL_HIGH_LEVEL but with interrupts
-  // disabled).
+  // handler (i.e. with interrupts already disabled before raising the
+  // TPL).
   //
   ASSERT (GetInterruptState () == FALSE);
   InterruptedTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
-  ASSERT (InterruptedTPL < TPL_HIGH_LEVEL);
+
+  //
+  // At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI
+  // specification) and so we should never encounter a situation in
+  // which InterruptedTPL==TPL_HIGH_LEVEL.  The specification also
+  // restricts usage of TPL_HIGH_LEVEL to the firmware itself.
+  //
+  // However, nothing prevents a rogue UEFI application from illegally
+  // calling gBS->RaiseTPL(TPL_HIGH_LEVEL) and then deliberately
+  // violating the invariant by enabling interrupts via the STI or
+  // equivalent instruction.  Some versions of the Microsoft Windows
+  // bootloader are known to do this.
+  //
+  if (InterruptedTPL >= TPL_HIGH_LEVEL) {
+DEBUG ((DEBUG_ERROR, "Illegal interrupt at TPL_HIGH_LEVEL!\n"));
+  }
 
   return InterruptedTPL;
 }
-- 
2.39.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104292): https://edk2.groups.io/g/devel/message/104292
Mute This Topic: https://groups.io/mt/98771399/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/2] OvmfPkg: Clarify invariants for NestedInterruptTplLib

[Michael Brown]

NestedInterruptTplLib relies on CPU interrupts being disabled to
guarantee exclusive (and hence atomic) access to the shared state in
IsrState.  Nothing in the calling interrupt handler should have
re-enabled interrupts before calling NestedInterruptRestoreTPL(), and
the loop in NestedInterruptRestoreTPL() itself maintains the invariant
that interrupts are disabled at the start of each iteration.

Add assertions to clarify this invariant, and expand the comments
around the calls to RestoreTPL() and DisableInterrupts() to clarify
the expectations around enabling and disabling interrupts.

Signed-off-by: Michael Brown 
---
 OvmfPkg/Library/NestedInterruptTplLib/Tpl.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c 
b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
index e19d98878eb7..e921a09c5599 100644
--- a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
+++ b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
@@ -104,6 +104,7 @@ NestedInterruptRestoreTPL (
   // defer our call to RestoreTPL() to the in-progress outer instance
   // of the same interrupt handler.
   //
+  ASSERT (GetInterruptState () == FALSE);
   if (InterruptedTPL == IsrState->InProgressRestoreTPL) {
 //
 // Trigger outer instance of this interrupt handler to perform the
@@ -153,6 +154,7 @@ NestedInterruptRestoreTPL (
 //
 // Check shared state loop invariants.
 //
+ASSERT (GetInterruptState () == FALSE);
 ASSERT (IsrState->InProgressRestoreTPL < InterruptedTPL);
 ASSERT (IsrState->DeferredRestoreTPL == FALSE);
 
@@ -167,13 +169,17 @@ NestedInterruptRestoreTPL (
 
 //
 // Call RestoreTPL() to allow event notifications to be
-// dispatched.  This will implicitly re-enable interrupts.
+// dispatched.  This will implicitly re-enable interrupts (if
+// InterruptedTPL is below TPL_HIGH_LEVEL), even though we are
+// still inside the interrupt handler.
 //
 gBS->RestoreTPL (InterruptedTPL);
 
 //
 // Re-disable interrupts after the call to RestoreTPL() to ensure
-// that we have exclusive access to the shared state.
+// that we have exclusive access to the shared state.  Interrupts
+// will be re-enabled by the IRET or equivalent instruction when
+// we subsequently return from the interrupt handler.
 //
 DisableInterrupts ();
 
-- 
2.39.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104291): https://edk2.groups.io/g/devel/message/104291
Mute This Topic: https://groups.io/mt/98771397/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 0/2] OvmfPkg: Relax assertion that interrupts do not occur at TPL_HIGH_LEVEL

[Michael Brown]

At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI
specification) and so we should never encounter a situation in which
an interrupt occurs at TPL_HIGH_LEVEL. The specification also
restricts usage of TPL_HIGH_LEVEL to the firmware itself.

However, nothing prevents a rogue UEFI application from illegally
calling gBS->RaiseTPL(TPL_HIGH_LEVEL) and then deliberately violating
the invariant by enabling interrupts via the STI or equivalent
instruction. Some versions of the Microsoft Windows bootloader are
known to do this.

NestedInterruptTplLib maintains the invariant that interrupts are
disabled at TPL_HIGH_LEVEL (even when performing the dark art of
deliberately manipulating the stack so that IRET will return with
interrupts still disabled), but does not itself rely on external code
maintaining this invariant.

Relax the assertion that the interrupted TPL is below TPL_HIGH_LEVEL
to an error message, to allow rogue UEFI applications such as the
Microsoft Windows bootloader to continue to function.

Ref: https://bugzilla.redhat.com/show_bug.cgi?id=2189136
Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Oliver Steffen 
Cc: Pawel Polawski 
Cc: Jiewen Yao 
Cc: Ard Biesheuvel 
Cc: Jordan Justen 

Michael Brown (2):
  OvmfPkg: Clarify invariants for NestedInterruptTplLib
  OvmfPkg: Relax assertion that interrupts do not occur at
TPL_HIGH_LEVEL

 OvmfPkg/Library/NestedInterruptTplLib/Tpl.c | 31 +
 1 file changed, 26 insertions(+), 5 deletions(-)

-- 
2.39.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104290): https://edk2.groups.io/g/devel/message/104290
Mute This Topic: https://groups.io/mt/98771393/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/1] OvmfPkg/NestedInterruptTplLib: replace ASSERT() with a warning logged.

[Michael Brown]

On 05/05/2023 19:56, Laszlo Ersek wrote:

I don't like the patch. For two reasons:

(1) It papers over the actual issue. The problem should be fixed where
it is, if possible.


Agreed, but (as you have shown in 
https://bugzilla.redhat.com/show_bug.cgi?id=2189136) the bug lies in 
Windows code rather than in EDK2 code.  If the goal is to allow these 
buggy Windows builds to still be used with OVMF, then the only option is 
to paper over the issue.  We should do this only if it can be proven 
safe to do so, of course.



(2) With the patch applied, NestedInterruptRaiseTPL() can return
TPL_HIGH_LEVEL (as "InterruptedTPL"). Consequently,
TimerInterruptHandler() [OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.c]
may pass TPL_HIGH_LEVEL back to NestedInterruptRestoreTPL(), as
"InterruptedTPL".

I believe that this in turn may invalidate at least one comment in
NestedInterruptRestoreTPL():

 //
 // Call RestoreTPL() to allow event notifications to be
 // dispatched.  This will implicitly re-enable interrupts.
 //
 gBS->RestoreTPL (InterruptedTPL);

Restoring TPL_HIGH_LEVEL does not re-enable interrupts -- nominally anyways.


I agree that the comment is invalidated, but as far as I can tell the 
logic remains safe.


I will put together a patch to update the comments in 
NestedInterruptTplLib to address the possibility of an interrupt 
occurring (illegally) at TPL_HIGH_LEVEL.



(a) Make LocalApicTimerDxe Xen-specific again. It's only the OVMF Xen
platform that really *needs* NestedInterruptTplLib. (Don't get me wrong:
NestedInterruptTplLib is technically correct in all circumstances, but
in practice it happens to be too strict.)

(b) For the non-Xen OVMF platforms, re-create a LocalApicTimerDxe
variant that effectively has commits a086f4a63bc0 and a24fbd606125
reverted. (We should keep 9bf473da4c1d.) This returns us to
pre-239b50a86370 status -- that is, a timer interrupt handler that (a)
does not try to be smart about nested interrupts, therefore one that is
much simpler, and (b) is more tolerant of the Windows / cdboot.efi spec
violation, (c) is vulnerable to the timer interrupt storm seen on Xen,
but will never run on Xen. (Only the OVMF Xen platform is supposed to be
launched on Xen.)


I'm less keen on this because it reduces the runtime exposure of a very 
complex piece of code, and will effectively cause that code to become 
unmaintained.


It's also satisfying (to me) that NestedInterruptTplLib provides a 
provable upper bound on stack consumption due to interrupts, which can't 
be guaranteed by the simpler pre-239b50a86370 scheme.


Could we defer judgement until after I've fully reasoned through (and 
documented) how NestedInterruptTplLib will work in the presence of 
interrupts occurring at TPL_HIGH_LEVEL?


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104162): https://edk2.groups.io/g/devel/message/104162
Mute This Topic: https://groups.io/mt/98656860/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/1] OvmfPkg/NestedInterruptTplLib: replace ASSERT() with a warning logged.

[Michael Brown]

On 03/05/2023 08:19, Gerd Hoffmann wrote:

OVMF can't guarantee that the ASSERT() doesn't happen.  Misbehaving
EFI applications can trigger this.  So log a warning instead and try
to continue.

Reproducer: Fetch windows 11 22H2 iso image, boot it in qemu with OVMF.

Traced to BootServices->Stall() being called with IPL=TPL_HIGH_LEVEL
and Interrupts /enabled/ while windows is booting.

Cc: Michael Brown 
Cc: Laszlo Ersek 
Signed-off-by: Gerd Hoffmann 
---
  OvmfPkg/Library/NestedInterruptTplLib/Tpl.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c 
b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
index e19d98878eb7..fdd7d15c4ba8 100644
--- a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
+++ b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
@@ -39,7 +39,9 @@ NestedInterruptRaiseTPL (
//
ASSERT (GetInterruptState () == FALSE);
InterruptedTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
-  ASSERT (InterruptedTPL < TPL_HIGH_LEVEL);
+  if (InterruptedTPL >= TPL_HIGH_LEVEL) {
+DEBUG ((DEBUG_WARN, "%a: Called at IPL %d\n", __func__, InterruptedTPL));
+  }
  
return InterruptedTPL;

  }


While https://bugzilla.redhat.com/show_bug.cgi?id=2189136 continues to 
track the underlying Windows bug that leads to this assertion being 
triggered: I suspect that this patch will allow people to boot these 
buggy versions of Windows in OVMF, and I don't think it will make things 
any worse.


I would probably suggest changing DEBUG_WARN to DEBUG_ERROR since this 
represents a serious invariant violation being detected.  With that change:


  Reviewed-by: Michael Brown 

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104134): https://edk2.groups.io/g/devel/message/104134
Mute This Topic: https://groups.io/mt/98656860/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] OvmfPkg/NestedInterruptTplLib: replace ASSERT() with a warning logged.

[Michael Brown]

On 28/04/2023 14:38, Gerd Hoffmann wrote:

I suspect the windows boot loader does something fishy here, but I can't
proof it, I have not yet pinned down the exact location where interrupts
get enabled while running at IPL=TPL_HIGH_LEVEL (which is what I suspect
is happening, but of course this is not the only possible theory how
that ASSERT got triggered).

Not fully sure how to best continue debugging this, I don't think gdb
can set an watchpoint on eflags.if ...


In the absence of any better ideas, I'd be tempted to run QEMU via TCG 
instead of KVM, and hack the STI instruction definition to check the 
location in guest memory where gEfiCurrentTpl gets stored in your test 
build.  (It's brute force debugging, but it should find the culprit.)



This workaround looks wrong to me: it will cause the subsequent call to
NestedInterruptRestoreTPL() to drop the TPL back down to TPL_HIGH_LEVEL-1,
which is lower than it would have been when the interrupt occurred.  The
completed hardware interrupt would then result in an overall change of TPL,
which cannot be correct.


The idea was to set InterruptedTPL to the highest level which is allowed
to run with interrupts enabled and continue running with interrupts
enabled, hoping that things get back into normal once the next timer
interrupt arrives.

Which -- now that I'm thinking about it again -- is clearly bogus, we
are in the timer interrupt so IRQs have already been disabled at that
point, so the fixup idea doesn't make much sense.

So just leave InterruptedTPL alone and hope for the best?


I think so.  CoreRestoreTpl(TPL_HIGH_LEVEL) should be a no-op, so it 
would be reasonable to expect that 
NestedInterruptRestoreTPL(TPL_HIGH_LEVEL, ...) should also be a no-op.


From a quick check of the implementation, I'm pretty sure this will be 
the case.  However, the logic is already (necessarily) pretty complex 
and so I am not 100% sure of this.  The reasoning behind the logic in 
NestedInterruptTplLib relies on certain axioms and invariants (e.g. that 
there are a finite number of distinct TPLs, and that there are certain 
places within the code that further interrupts provably cannot occur), 
and so it gets very difficult to reason about when one of those 
invariants is violated, as it seems to be in this situation.


If it seems to work, then I think it would be plausible to replace the

  ASSERT (InterruptedTPL < TPL_HIGH_LEVEL);

with a warning message, and to return with InterruptedTPL unmodified. 
But I'd prefer to understand how this invariant violation arises in the 
first place.


Good luck!

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#103780): https://edk2.groups.io/g/devel/message/103780
Mute This Topic: https://groups.io/mt/98554842/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] OvmfPkg/NestedInterruptTplLib: replace ASSERT() with a warning logged.

[Michael Brown]

On 28/04/2023 10:10, Gerd Hoffmann wrote:

OVMF can't guarantee that the ASSERT() doesn't happen.  Misbehaving
EFI applications can trigger this.  So log a warning instead and try
to continue.

Reproducer: Fetch windows 11 22H2 iso image, boot it in qemu with OVMF.

Traced to BootServices->Stall() being called with IPL=TPL_HIGH_LEVEL
and Interrupts /enabled/ while windows is booting.


Do we know how the system ended up in a state of being at TPL_HIGH_LEVEL 
with interrupts enabled?  This ought not to be possible: the code in 
EDK2 will (as far as I can tell) always maintain the invariant that 
interrupts are disabled while at TPL_HIGH_LEVEL.



--- a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
+++ b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
@@ -39,7 +39,15 @@ NestedInterruptRaiseTPL (
//
ASSERT (GetInterruptState () == FALSE);
InterruptedTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
-  ASSERT (InterruptedTPL < TPL_HIGH_LEVEL);
+  if (InterruptedTPL >= TPL_HIGH_LEVEL) {
+DEBUG ((
+  DEBUG_WARN,
+  "%a: Called at IPL %d, trying to fixup and continue...\n",
+  __func__,
+  InterruptedTPL
+  ));
+InterruptedTPL = TPL_HIGH_LEVEL - 1;
+  }


This workaround looks wrong to me: it will cause the subsequent call to 
NestedInterruptRestoreTPL() to drop the TPL back down to 
TPL_HIGH_LEVEL-1, which is lower than it would have been when the 
interrupt occurred.  The completed hardware interrupt would then result 
in an overall change of TPL, which cannot be correct.


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#103775): https://edk2.groups.io/g/devel/message/103775
Mute This Topic: https://groups.io/mt/98554842/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] On integrating LoongArch EDK2 firmware into QEMU build process

[Michael Brown]

On 03/04/2023 11:13, Chao Li wrote:
This problem is because the gcc-12 does not yet to support the option 
'mno-explicit-reloc', this option is used to open the new reloaction 
type for LoongArch, this new feature is very important for LoongArch, 
because it can reduce the binary size and improve code execution 
efficiency, so we turn it on when submitting the code to the EDK2 repo.


Is it possible to produce a _functional_ LoongArch64 EDK2 binary without 
this option, even if the resulting binary is less efficient?


(I'm the person who updated Fedora's binutils and cross-gcc packages to 
ensure that LoongArch64 was supported in Fedora 38, and this 
-mno-explicit-relocs issue is also currently blocking me from merging 
LoongArch64 support into iPXE.)


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102381): https://edk2.groups.io/g/devel/message/102381
Mute This Topic: https://groups.io/mt/98030924/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] NetworkPkg/HttpDxe: provide function to disable TLS host verify

[Michael Brown]

On 07/03/2023 08:21, Nickle Wang via groups.io wrote:

I got an idea to handle this issue.

EFI_HTTP_SERVICE_BINDING_PROTOCOL is defined in UEFI specification for 
caller to create HTTP protocol on child instance. How about I propose a 
new service binding protocol called 
EFI_HTTP_*NO_TLS_HOST_VERIFY*_SERVICE_BINDING_PROTOCOL, and the 
EFI_HTTP_PROTOCOL created by this service binding protocol will not do 
TLS host verify during HTTPS communication.


When caller like to disable host verify on HTTPS communication, caller 
use this service binding protocol to create special HTTP instance. For 
other case, caller use regular EFI_HTTP_SERVICE_BINDING_PROTOCOL to get 
normal EFI_HTTP_PROTOCOL instance.


That seems very hacky, and does not help to address the general problem 
of being able to more flexibly configure HTTP connections.


From a quick look through the UEFI spec, it looks as though 
EFI_TLS_PROTOCOL.SetSessionData() should already allow you to set 
EfiTlsVerifyMethod with a value of EFI_TLS_VERIFY_NONE.


The implementation of HttpDxe makes it very messy to gain access to the 
EFI_TLS_PROTOCOL instance, since it will be created only when 
EFI_HTTP_PROTOCOL.Request() is called.  I think you may have to use 
gBS->RegisterProtocolNotify() in order to intercept the point at which 
EFI_TLS_PROTOCOL is installed.  In your notification event callback, you 
would then check to see if the handle is a child of the 
EFI_HTTP_PROTOCOL handle and, if so, call 
EFI_TLS_PROTOCOL.SetSessionData() to disable host verification.


You would need to be using a newly created EFI_HTTP_PROTOCOL instance, 
so that you could be sure that there was no existing EFI_TLS_PROTOCOL 
instance already in place.


I haven't tested any of the above, but it looks as though it should work 
and allow you to disable host verification for a single 
EFI_HTTP_PROTOCOL instance, without any specification changes.


Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100798): https://edk2.groups.io/g/devel/message/100798
Mute This Topic: https://groups.io/mt/96669380/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-staging/RiscV64QemuVirt PATCH V8 00/19] Add support for RISC-V virt machine

[Michael Brown]

On 17/02/2023 04:27, Sunil V L wrote:

On Thu, Feb 16, 2023 at 03:45:49PM -0700, dann frazier wrote:

   Thanks for your work getting this merged! In the above wiki, it
notes that GCC 12+ is not supported. Is that still accurate? If so,
can you clarify what is blocking that?


Please see https://bugzilla.tianocore.org/show_bug.cgi?id=4061.

My attempt to fix this issue
(https://edk2.groups.io/g/devel/message/93831) was not accepted due to
the concerns that it can cause weird issues in CI.

So, we are left with either support gcc <12 or gcc >=12. We can mandate
gcc 12 itself for RISC-V, but that change need to be done hand in hand
with CI tests moving to use gcc 12. Otherwise, it will break CI.


Is there an alternative (and presumably less ideal) way to force an 
instruction cache invalidation?  For example, does a global TSO "fence" 
instruction as used in RiscVInvalidateDataCacheAsm() also invalidate the 
instruction cache?


If so, then a viable solution would be:

--- a/MdePkg/Library/BaseLib/RiscV64/FlushCache.S
+++ b/MdePkg/Library/BaseLib/RiscV64/FlushCache.S
@@ -15,3 +15,7 @@ ASM_GLOBAL ASM_PFX(RiscVInvalidateDataCacheAsm)
 ASM_PFX(RiscVInvalidateInstCacheAsm):
-fence.i
+#ifdef __riscv_zifencei
+   fence.i
+#else
+   fence
+#endif
 ret


This would also permit EDK2 to be used on implementations that genuinely 
do not provide the fence.i instruction.


Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100311): https://edk2.groups.io/g/devel/message/100311
Mute This Topic: https://groups.io/mt/96874977/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/3] UsbNetworkPkg/UsbRndis: Add USB RNDIS devices support

[Michael Brown]

On 15/02/2023 05:36, RichardHo [何明忠] via groups.io wrote:
On 15/02/2023 05:36, RichardHo [何明忠] via groups.io wrote:
On 15/02/2023 05:36, RichardHo [何明忠] via groups.io wrote:
On 15/02/2023 05:36, RichardHo [何明忠] via groups.io wrote:
On 15/02/2023 05:36, RichardHo [何明忠] via groups.io wrote:

+  if (Nic->RateLimitingEnable == TRUE) {
+Status = gBS->CreateEvent (
+EVT_TIMER | EVT_NOTIFY_SIGNAL,
+TPL_CALLBACK,
+UndiRateLimiterCallback,
+Nic,
+>RateLimiter
+);
+if (EFI_ERROR (Status)) {
+  goto ErrorCreateEvent;
+}
+
+Status = gBS->SetTimer (
+Nic->RateLimiter,
+TimerPeriodic,
+PcdGet32 (RateLimitingFactor) * 1
+);
+if (EFI_ERROR (Status)) {
+  goto ErrorSetTimer;
+}
+  }
+
+  if (Nic->UsbEth->UsbEthUndi.UsbEthUndiStart != NULL) {
+Status = Nic->UsbEth->UsbEthUndi.UsbEthUndiStart (Cdb, Nic);
+if (EFI_ERROR (Status)) {
+  goto ErrorUndiStart;
+}
+  }
+
+  Nic->State = PXE_STATFLAGS_GET_STATE_STARTED;
+  Cdb->StatFlags = PXE_STATFLAGS_COMMAND_COMPLETE;
+  Cdb->StatCode  = PXE_STATCODE_SUCCESS;
+  return;
+
+ErrorUndiStart:
+  gBS->SetTimer (>RateLimiter, TimerCancel, 0);
+ErrorSetTimer:
+  gBS->CloseEvent (>RateLimiter);
+ErrorCreateEvent:
+  Cdb->StatFlags = PXE_STATFLAGS_COMMAND_FAILED;
+  Cdb->StatCode  = PXE_STATCODE_DEVICE_FAILURE;
+}


Thank you for incorporating the polling rate limiting feature.

There is a bug in the above code: if RateLimitingEnable==FALSE and then 
an error occurs in UsbEthUndiStart(), the error handling code will call 
SetTimer() and CloseEvent() on an event that was never created.


The simplest fix is to move the conditional check for 
RateLimitingEnable==FALSE so that the event is always created even if it 
will not be used:


  Status = gBS->CreateEvent (
  EVT_TIMER | EVT_NOTIFY_SIGNAL,
  TPL_CALLBACK,
  UndiRateLimiterCallback,
  Nic,
  >RateLimiter
  );
  if (EFI_ERROR (Status)) {
goto ErrorCreateEvent;
  }

  if (Nic->RateLimitingEnable == TRUE) {
Status = gBS->SetTimer (
Nic->RateLimiter,
TimerPeriodic,
PcdGet32 (RateLimitingFactor) * 1
);
if (EFI_ERROR (Status)) {
  goto ErrorSetTimer;
}
  }


+  if (Nic->RateLimitingEnable == TRUE) {
+gBS->SetTimer (>RateLimiter, TimerCancel, 0);
+gBS->CloseEvent (>RateLimiter);
+  }


... and this conditional check may then also be removed.


+  if ((Nic->RateLimitingCreditCount == 0) && (Nic->RateLimitingEnable == 
TRUE)) {
+return PXE_STATCODE_NO_DATA;
+  }
+
+  Status = Nic->UsbEth->UsbEthReceive (Cdb, Nic->UsbEth, (VOID *)BulkInData, 
);
+  if (EFI_ERROR (Status)) {
+Nic->ReceiveStatus = 0;
+if (Nic->RateLimitingEnable == TRUE) {
+  OriginalTpl = gBS->RaiseTPL (TPL_NOTIFY);
+  if (Nic->RateLimitingCreditCount != 0) {
+Nic->RateLimitingCreditCount--;
+  }
+
+  gBS->RestoreTPL (OriginalTpl);
+}


The check for RateLimitingCreditCount!=0 is redundant: if 
RateLimitingCreditCount is zero then you cannot reach that point in the 
code anyway.



+[PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
+  ## Support rate limiting
+  gUsbNetworkPkgTokenSpaceGuid.EnableRateLimiting|FALSE|BOOLEAN|0x00010001


I would suggest that the default value should be TRUE.

The downside of setting the default value to TRUE is that the overall 
throughput will be slower (as reported by you).


The downside of setting the default value to FALSE is that the system 
will lock up completely (as reported by me and others).


Setting to TRUE by default is therefore less harmful overall.

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100234): https://edk2.groups.io/g/devel/message/100234
Mute This Topic: https://groups.io/mt/96977783/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts

[Michael Brown]

On 14/02/2023 13:01, Gerd Hoffmann wrote:

On Mon, Feb 13, 2023 at 04:15:30PM +, Michael Brown wrote:

On 13/02/2023 15:48, Michael Kubacki wrote:

@@ -1608,9 +1610,12 @@ ParseAndAddExistingSmbiosTable (
   //
   // Make sure not to access memory beyond SmbiosEnd
   //
-if ((Smbios.Raw + sizeof (SMBIOS_STRUCTURE) > SmbiosEnd.Raw) ||
-(Smbios.Raw + sizeof (SMBIOS_STRUCTURE) < Smbios.Raw))
-{
+Status = SafeUintnAdd ((UINTN)Smbios.Raw, sizeof (SMBIOS_STRUCTURE), 
);
+if (EFI_ERROR (Status)) {
+  return EFI_INVALID_PARAMETER;
+}
+
+if ((SafeIntResult > (UINTN)SmbiosEnd.Raw) || (SafeIntResult < 
(UINTN)Smbios.Raw)) {
 return EFI_INVALID_PARAMETER;
   }


Could this not be expressed much more cleanly as just:

   if ((UINTN)(SmbiosEnd.Raw - Smbios.Raw) < sizeof (SMBIOS_STRUCTURE)) {
 return EFI_INVALID_PARAMETER;
   }

without the need to hide a basic arithmetic operation behind an ugly wrapper
function and drag in an external library?


Well, the advantage of using SafeIntLib is that (a) it is obvious even
to untrained code readers what is going on here, and (b) it is hard to
get it wrong.

When looking at the quality some of the patches being posted to the list
have I'd say that is important to consider even if you personally have
no problems avoiding integer overflows without the help of SafeIntLib.


Fair enough.  But in that case it should be used in a way that makes it 
clear what it's actually doing.  Specifically, the check for


  if (... || (SafeIntResult < (UINTN)Smbios.Raw)) {
...
  }

then becomes meaningless, since the whole point of SafeUintnAdd() is 
that it cannot result in this kind of unsigned integer wraparound.  The 
code as modified by this patch is *harder* to understand, because the 
reader has to dig through to figure out why this check still exists, 
look at the implementation of SafeUintnAdd() to see what is meant by 
"safe" in this context, and finally come to the conclusion that the 
remaining underflow check is redundant code that should have been removed.


The reader is also going to be confused by the fact that the code as 
modified by this patch then contains two separate methods for checking 
for integer overflows, since the patch does not similarly update the 
very similar code found almost immediately afterwards:


  if ((Smbios.Raw + Smbios.Hdr->Length + 2 * sizeof (UINT8) > 
SmbiosEnd.Raw) ||
(Smbios.Raw + Smbios.Hdr->Length + 2 * sizeof (UINT8) < 
Smbios.Raw))

{
  return EFI_INVALID_PARAMETER;
}


Lastly: the actual operation being made safe here is "check that buffer 
contains at least this much data remaining".  This is most obviously 
done by calculating the remaining buffer space (i.e. 
(UINTN)(SmbiosEnd.Raw - Smbios.Raw)) and comparing against that.  That 
logic is clear, simple, and obviously correct at first glance.  Using 
the SafeUintnAdd() call does something that *is* mathematically 
equivalent to this check, but where the logic is much harder to parse.



I'd prefer it if the code were updated to avoid SafeUintnAdd() 
altogether.  But if not, then at a minimum the redundant check should be 
removed, and the calculation involving Smbios.Hdr->Length should also be 
updated to use SafeUintnAdd().


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100177): https://edk2.groups.io/g/devel/message/100177
Mute This Topic: https://groups.io/mt/96938294/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts

[Michael Brown]

On 13/02/2023 15:48, Michael Kubacki wrote:

@@ -1608,9 +1610,12 @@ ParseAndAddExistingSmbiosTable (
  //
  // Make sure not to access memory beyond SmbiosEnd
  //
-if ((Smbios.Raw + sizeof (SMBIOS_STRUCTURE) > SmbiosEnd.Raw) ||
-(Smbios.Raw + sizeof (SMBIOS_STRUCTURE) < Smbios.Raw))
-{
+Status = SafeUintnAdd ((UINTN)Smbios.Raw, sizeof (SMBIOS_STRUCTURE), 
);
+if (EFI_ERROR (Status)) {
+  return EFI_INVALID_PARAMETER;
+}
+
+if ((SafeIntResult > (UINTN)SmbiosEnd.Raw) || (SafeIntResult < 
(UINTN)Smbios.Raw)) {
return EFI_INVALID_PARAMETER;
  }


Could this not be expressed much more cleanly as just:

  if ((UINTN)(SmbiosEnd.Raw - Smbios.Raw) < sizeof (SMBIOS_STRUCTURE)) {
return EFI_INVALID_PARAMETER;
  }

without the need to hide a basic arithmetic operation behind an ugly 
wrapper function and drag in an external library?


The almost-identical check performed in the code immediately below (that 
takes Smbios.Hdr->Length into account) should also be updated to e.g.:


  if ((UINTN)(SmbiosEnd.Raw - Smbios.Raw) < (Smbios.Hdr->Length + 2)) {
...
  }

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100116): https://edk2.groups.io/g/devel/message/100116
Mute This Topic: https://groups.io/mt/96938294/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-staging/RiscV64QemuVirt PATCH V7 05/20] UefiCpuPkg: Add CpuTimerDxe module

[Michael Brown]

On 09/02/2023 10:28, Sunil V L wrote:

+  gBS->RestoreTPL (OriginalTPL);
+  RiscVEnableTimerInterrupt (); // enable SMode timer int
+}


This design looks as though it does not support nested timer interrupts.
The call to RestoreTPL() may invoke callbacks that may themselves include
delay loops that wait upon further timer interrupts.  With the above code,
those timer interrupts will never arrive since the timer interrupt is
disabled at the point that you call RestoreTPL().

This will break device drivers such as those for USB network devices that
rely on nested timer interrupts.


Thanks a lot for this feedback and background. We are aware of few issues
in this module. Currently, it is mostly porting what exists today in
edk2-platforms repo. We want to add all these additional fixes after
this basic thing is merged. That way we will have git history instead of
combining all fixes single commit. Andrei has a patch ready and waiting
for this to get merged. We can either combine this with his patch or
create one more.

Would that strategy be fine with you?


Sure, as long as someone other than me is keeping track of the need to 
fix this bug.  My work here is done.  :)


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99881): https://edk2.groups.io/g/devel/message/99881
Mute This Topic: https://groups.io/mt/96593498/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-staging/RiscV64QemuVirt PATCH V7 05/20] UefiCpuPkg: Add CpuTimerDxe module

[Michael Brown]

+/**
+  Timer Interrupt Handler.
+
+  @param InterruptTypeThe type of interrupt that occured
+  @param SystemContextA pointer to the system context when the interrupt 
occured
+**/
+VOID
+EFIAPI
+TimerInterruptHandler (
+  IN EFI_EXCEPTION_TYPE  InterruptType,
+  IN EFI_SYSTEM_CONTEXT  SystemContext
+  )
+{
+  EFI_TPL  OriginalTPL;
+  UINT64   RiscvTimer;
+
+  OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
+  if (mTimerNotifyFunction != NULL) {
+mTimerNotifyFunction (mTimerPeriod);
+  }
+
+  RiscVDisableTimerInterrupt (); // Disable SMode timer int
+  RiscVClearPendingTimerInterrupt ();
+  if (mTimerPeriod == 0) {
+gBS->RestoreTPL (OriginalTPL);
+RiscVDisableTimerInterrupt (); // Disable SMode timer int
+return;
+  }
+
+  RiscvTimer   = RiscVReadTimer ();
+  SbiSetTimer (RiscvTimer += mTimerPeriod);
+  gBS->RestoreTPL (OriginalTPL);
+  RiscVEnableTimerInterrupt (); // enable SMode timer int
+}


This design looks as though it does not support nested timer interrupts. 
 The call to RestoreTPL() may invoke callbacks that may themselves 
include delay loops that wait upon further timer interrupts.  With the 
above code, those timer interrupts will never arrive since the timer 
interrupt is disabled at the point that you call RestoreTPL().


This will break device drivers such as those for USB network devices 
that rely on nested timer interrupts.



The easiest fix is to use NestedTimerInterruptLib and change the code as 
follows:


Add at the start of the function:

  STATIC NESTED_INTERRUPT_STATE NestedInterruptState;

Change:

  OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);

to

  OriginalTPL = NestedInterruptRaiseTPL ();

Change:

  gBS->RestoreTPL (OriginalTPL);
  RiscVEnableTimerInterrupt (); // enable SMode timer int

to

  RiscVEnableTimerInterrupt (); // enable SMode timer int
  NestedInterruptRestoreTPL (OriginalTpl, SystemContext, 
);


Note that the timer interrupt is then correctly re-enabled before the 
call to NestedInterruptRestoreTPL().  NestedInterruptTplLib takes care 
of the mess required behind the scenes to make this all provably safe.



You'll also need to extend Library/NestedInterruptTplLib/Iret.c to 
support MDE_CPU_RISCV64: it should be fairly obvious what's needed there.



See commit https://github.com/tianocore/edk2/commit/a086f4a63b for the 
equivalent fix being applied to OVMF.



You can find the background leading up to the creation of 
NestedInterruptTplLib at:


  https://bugzilla.tianocore.org/show_bug.cgi?id=4162
  https://github.com/tianocore/edk2/commit/9bf473da4c
  https://github.com/tianocore/edk2/commit/a24fbd6061
  https://github.com/tianocore/edk2/commit/a086f4a63b

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99868): https://edk2.groups.io/g/devel/message/99868
Mute This Topic: https://groups.io/mt/96593498/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Graphic Output on qemu

[Michael Brown]

On 08/02/2023 09:55, Alireza Banejad wrote:
As my previous email, I mentioned that I am able to find the protocol 
since the LocateProtocol returns 0 (EFI_SUCCESS) (using the exact code 
you wrote) . But when I want to open it either with OpenProtocol or 
HandleProtocol I get a RETURN_UNSUPPORTED. Are you implying that by 
calling LocateProtocol() the protocol would also be opened?


LocateProtocol() gives you back a pointer to the protocol instance. 
There is no need for a separate "open" action: you can just immediately 
use the protocol instance that LocateProtocol() returns to you.


Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99792): https://edk2.groups.io/g/devel/message/99792
Mute This Topic: https://groups.io/mt/96810830/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Graphic Output on qemu

[Michael Brown]

On 08/02/2023 08:03, Alireza Banejad wrote:

Below is how I use the HandleProtocol function:

   Status = gBS->HandleProtocol (
                   gST->ConsoleOutHandle,
                   ,
                   (VOID **)
                   );


You are querying only the ConsoleOutHandle, which is not necessarily 
where the GOP will be installed.


Use LocateProtocol() to find the protocol instance instead:


  Status = gBS->LocateProtocol(, NULL,
   (VOID **));

HTH,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99790): https://edk2.groups.io/g/devel/message/99790
Mute This Topic: https://groups.io/mt/96810830/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/3] UsbNetworkPkg/UsbRndis: Add USB RNDIS devices support

[Michael Brown]

On 07/02/2023 06:21, Tinh Nguyen via groups.io wrote:

From: Tinh Nguyen 
Date: Tue, 7 Feb 2023 12:43:17 +0700
Subject: [PATCH] UsbNetworkPkg: Support rate limitting

Signed-off-by: Tinh Nguyen 


Thank you for extending my patch to add the PCD support.  The overall 
patch appears still to be substantially my code: could you please credit 
it as such?



+  if (Nic->RateLimitCredit < PcdGet32 (RateLimitingResumeTime)) {
+    Nic->RateLimitCredit++;
+  }


Is PcdGet32() guaranteed to be a compile-time constant?  If not, then 
it's probably a good idea to read it once upon initialisation, rather 
than once per timer interrupt handler invocation.



+  if ((Nic->RateLimitCredit == 0) && (PcdGetBool (EnableRateLimiting))) {
+    return PXE_STATCODE_NO_DATA;
+  }


Again: unless PcdGetBool() is guaranteed to be a compile-time constant, 
then it's probably best to avoid reading it on every call to Receive().


(Sorry, I've never been clear on how the PCD mechanism works, so I'm not 
sure whether or not there is any runtime overhead from reading them.)


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99722): https://edk2.groups.io/g/devel/message/99722
Mute This Topic: https://groups.io/mt/96740897/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] UsbNetworkPkg/UsbRndis: Add USB RNDIS devices support

[Michael Brown]

On 05/02/2023 08:04, Tinh Nguyen via groups.io wrote:

On 1/12/2023 3:36 PM, Richard Ho (何明忠) wrote:

We add this patch in my X86 platform and use the NCM device to test IPV4 PXE 
boot from 1330MB ISO file.

No this patch: 35 sec to download 1330M ISO file
Add this patch: 181  sec to download 1330M ISO file

The patch will increase boot time in my X86 platform.


Sorry for the late response to this thread.  My platform cannot utilize
this driver without this fix, whether we can support a PCD to
enable/disable this rate limiting?


Same for me: without the rate limiting in place, the download speed 
can't even be measured since the whole platform locks up as soon as the 
driver binds to the USB device.


Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99645): https://edk2.groups.io/g/devel/message/99645
Mute This Topic: https://groups.io/mt/95531719/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH v2 7/7] ArmVirtPkg: Implement BTI for runtime regions

[Michael Brown]

On 03/02/2023 12:55, Ard Biesheuvel wrote:

Question: as a producer of externally loaded UEFI binaries (e.g.
ipxe.efi): what would I need to do to take advantage of BTI?

I'm assuming:

- enable -mbranch-protection=bti in my builds (easy)

- wait for PE/COFF specification change and then update my produced
images to include whatever flag gets decided upon.

Is that correct?


First of all, in case you missed this, the series in question only
covers runtime DXE drivers, i.e., the code that persists after
ExitBootServices() and gets mapped by the OS and called to access the
variable store. So iPXE should not be affected at all by these
changes.


I was not paying close attention to this patch series and had missed 
that detail: thank you for clarifying.



So to answer your question: yes.


Thank you!

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99581): https://edk2.groups.io/g/devel/message/99581
Mute This Topic: https://groups.io/mt/96721191/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH v2 7/7] ArmVirtPkg: Implement BTI for runtime regions

[Michael Brown]

On 03/02/2023 12:10, Ard Biesheuvel wrote:

+[BuildOptions]
+!if $(RUNTIME_BTI_ENABLE) == TRUE
+  GCC:*_*_AARCH64_CC_FLAGS = -mbranch-protection=bti
+!endif


Question: as a producer of externally loaded UEFI binaries (e.g. 
ipxe.efi): what would I need to do to take advantage of BTI?


I'm assuming:

- enable -mbranch-protection=bti in my builds (easy)

- wait for PE/COFF specification change and then update my produced 
images to include whatever flag gets decided upon.


Is that correct?

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99579): https://edk2.groups.io/g/devel/message/99579
Mute This Topic: https://groups.io/mt/96721191/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] NetworkPkg/HttpDxe: provide function to disable TLS host verify

[Michael Brown]

On 01/02/2023 11:06, Nickle Wang via groups.io wrote:
Thanks for catching this. To prevent the change to data structure, would 
you suggest me to create new interface in EFI_HTTP_PROTOCOL and disable 
TLS host verify?


Adding an interface to EFI_HTTP_PROTOCOL would also break the ABI by 
changing the layout of a data structure defined in the UEFI 
specification, and so can't be done.


I took a quick look through Http.h and I can't immediately see any way 
you can convey the information you want without making a breaking 
change.  There are no flags fields (that could be extended with extra 
flags in the same memory slot), no structure version number fields (that 
could allow structures to be extended, subject to a version number 
check), and no general-purpose "additional information" extension 
mechanism besides the one for passing arbitrary HTTP headers.


I suspect you'll need to either make a new protocol (lots of work, very 
ugly) or find some sideband mechanism you can use to work around the 
problem, like a PCD to globally enable/disable host verification.


It may be worth waiting for one of the HttpDxe maintainers to offer an 
opinion on this, since I am totally unfamiliar with this part of the 
codebase.


Sorry,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99397): https://edk2.groups.io/g/devel/message/99397
Mute This Topic: https://groups.io/mt/96669380/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] NetworkPkg/HttpDxe: provide function to disable TLS host verify

[Michael Brown]

On 01/02/2023 03:46, Nickle Wang via groups.io wrote:

diff --git a/MdePkg/Include/Protocol/Http.h b/MdePkg/Include/Protocol/Http.h
index 28e6221593..21a782eaac 100644
--- a/MdePkg/Include/Protocol/Http.h
+++ b/MdePkg/Include/Protocol/Http.h
@@ -6,6 +6,7 @@
  
Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.

(C) Copyright 2015-2017 Hewlett Packard Enterprise Development LP
+  Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
  
@par Revision Reference:

@@ -161,6 +162,10 @@ typedef struct {
/// this instance will use EFI_DNS6_PROTOCOL and EFI_TCP6_PROTOCOL.
///
BOOLEAN LocalAddressIsIPv6;
+  ///
+  /// Verify server certificate during HTTPS handshake.
+  ///
+  BOOLEAN HostCertificateVerifyDisabled;
  
union {

  ///


This change would break the ABI by changing the layout of a data 
structure defined in the UEFI specification.


Even worse, it does so by inserting a field into the middle of a 
structure: an ABI mismatch would result in one side attempting to 
dereference the BOOLEAN value as a pointer.


Nacked-by: Michael Brown 

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99394): https://edk2.groups.io/g/devel/message/99394
Mute This Topic: https://groups.io/mt/96669380/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] arm64 support for stuart

[Michael Brown]

On 25/01/2023 16:55, Gerd Hoffmann wrote:

iasl is a different matter, as we need it to build for arm64 as well.
iasl is already available in the arm64 distros, so as I see it, there
are 3 options here:
- build iasl for Linux/arm64 and add it to the nuget repo
- allow a fallback to system-wide iasl (how?)


Just use the system-wide tools is the best option IMHO.  The packages
are available in Fedora (other distros should be have them too), on both
x86_64 and aarch64, we only need to add them to the CI container image.
So why bother adding nuget builds?


I totally agree with this.  Life is much easier when everything just 
uses the existing system-wide tools.


Deliberately imposing a restriction that CI uses only system-wide tools 
also helps to prevent a project slipping down into the abyss of obscure 
dependencies and multi-GB custom SDK container images.


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99035): https://edk2.groups.io/g/devel/message/99035
Mute This Topic: https://groups.io/mt/96521061/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 0/2] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[Michael Brown]

On 19/01/2023 11:01, Laszlo Ersek wrote:

PlatformCpuCountBugCheck: Present=0 Possible=1
PlatformCpuCountBugCheck: Broken CPU hotplug register block found. Update QEMU 
to version 8+, or
PlatformCpuCountBugCheck: to a stable release with commit dab30fbef389 
backported. Refer to
PlatformCpuCountBugCheck: <https://bugzilla.tianocore.org/show_bug.cgi?id=4250>.
PlatformCpuCountBugCheck: Consequences of the QEMU bug may include, but are not 
limited to:
PlatformCpuCountBugCheck: - all firmware logic, dependent on the CPU hotplug 
register block,
PlatformCpuCountBugCheck:   being confused, for example, 
multiprocessing-related logic;
PlatformCpuCountBugCheck: - guest OS data loss, including filesystem 
corruption, due to crash or
PlatformCpuCountBugCheck:   hang during ACPI S3 resume;
PlatformCpuCountBugCheck: - SMM privilege escalation, by a malicious guest OS 
or 3rd partty UEFI
PlatformCpuCountBugCheck:   agent, against the platform firmware.
PlatformCpuCountBugCheck: These symptoms need not necessarily be limited to the 
QEMU user
PlatformCpuCountBugCheck: attempting to hot(un)plug a CPU.
PlatformCpuCountBugCheck: The firmware will now stop (hang) deliberately, in 
order to prevent the
PlatformCpuCountBugCheck: above symptoms.
PlatformCpuCountBugCheck: You can forcibly override the hang, *at your own 
risk*, with the
PlatformCpuCountBugCheck: following *experimental* QEMU command line option:
PlatformCpuCountBugCheck:   -fw_cfg 
name=opt/org.tianocore/X-Cpuhp-Bugcheck-Override,string=yes
PlatformCpuCountBugCheck: Please only report such bugs that you can reproduce 
*without* the
PlatformCpuCountBugCheck: override.


Laszlo: thank you for taking the time to deal with this so thoroughly 
and to see it through to its conclusion.


For what it's worth:

Hugely-appreciated-by: Michael Brown 

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98892): https://edk2.groups.io/g/devel/message/98892
Mute This Topic: https://groups.io/mt/96374972/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[Michael Brown]

On 12/01/2023 17:58, Laszlo Ersek wrote:
The case is that both QEMU and edk2 check for each other's supported 
features. It's a complex interwoven feature set with security

impact, which is exactly why we added feature negotiation at every
step -- effectively mutual negotiation wherever necessary. I cannot
claim I remember every part of it, and playing tricks around feature
negotiation with SMM impact makes me *extremely uncomfortable*. I
absolutely don't want to author an OVMF patch, briefly before I
disappear again (for good!), that "looks good" now, and then becomes
a horrible SMM CVE in a year or two. I want to go for "obviously no
bug", rather than "no obvious bug".


I'm definitely not sufficiently familiar with all of the QEMU and OVMF
historical quirks to safely author or review a patch to cover all of 
this, so I will very definitely defer to your judgement on this.


On 12/01/2023 18:22, Laszlo Ersek wrote:

There's got to be a limit to how far we try to compensate for broken
(virtual) hardware. :( The right thing to do is to wait for the QEMU
patch to reach as many as possible stable branches, let the distros
pick up the new stable releases, and then merge the hardliner hang.


I concur.

Thanks for considering the suggestion!  :)

Michael


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98393): https://edk2.groups.io/g/devel/message/98393
Mute This Topic: https://groups.io/mt/96218818/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[Michael Brown]

On 12/01/2023 13:22, Laszlo Ersek wrote:

Detect the issue in PlatformMaxCpuCountInitialization(), and
print an error message and *hang* if the issue is present.


Would this mean that OVMF would refuse to start with all current
distro versions of qemu (when not using KVM), or am I
misunderstanding?


Your understanding is correct.


I apologise in advance if this is a stupid question, but: given that we
can detect the issue (as per this patch), and given also:


On 12/01/2023 08:28, Laszlo Ersek wrote:
In QEMU v5.1.0, the CPU hotplug register block misbehaves: the 
negotiation protocol is (effectively) broken such that it

suggests that switching from the legacy interface to the modern
interface works, but in reality the switch never happens.
...would it work to detect the issue and treat it as "modern interface 
is not supported: continue to use the legacy interface"?  IOW, to treat 
"Present=0" as indicating that the modern interface is not supported.


Thanks,

Michael


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98379): https://edk2.groups.io/g/devel/message/98379
Mute This Topic: https://groups.io/mt/96218818/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: catch QEMU's CPU hotplug reg block regression

[Michael Brown]

On 12/01/2023 08:28, Laszlo Ersek wrote:

In QEMU v5.1.0, the CPU hotplug register block misbehaves: the negotiation
protocol is (effectively) broken such that it suggests that switching from
the legacy interface to the modern interface works, but in reality the
switch never happens. The symptom has been witnessed when using TCG
acceleration; KVM seems to mask the issue. The issue persists with the
following (latest) stable QEMU releases: v5.2.0, v6.2.0, v7.2.0. Currently
there is no stable release that addresses the problem.

The QEMU bug confuses the Present and Possible counting in function
PlatformMaxCpuCountInitialization(), in
"OvmfPkg/Library/PlatformInitLib/Platform.c". OVMF ends up with Present=0
Possible=1. This in turn further confuses MpInitLib in UefiCpuPkg (hence
firmware-time multiprocessing will be broken). Worse, CPU hot(un)plug with
SMI will be summarily broken in OvmfPkg/CpuHotplugSmm, which (considering
the privilege level of SMM) is not that great.

Detect the issue in PlatformMaxCpuCountInitialization(), and print an
error message and *hang* if the issue is present.


Would this mean that OVMF would refuse to start with all current distro 
versions of qemu (when not using KVM), or am I misunderstanding?


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98349): https://edk2.groups.io/g/devel/message/98349
Mute This Topic: https://groups.io/mt/96218818/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] UsbNetworkPkg/UsbRndis: Add USB RNDIS devices support

[Michael Brown]

On 11/01/2023 07:34, Tinh Nguyen via groups.io wrote:
I have changed the Metronome driver from EmbeddedPkg to MdeModulePkg, 
but the performance is still very slow.


Could you please try using the patch from 
https://edk2.groups.io/g/devel/message/98256 to see if this fixes the 
problem for you?


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98289): https://edk2.groups.io/g/devel/message/98289
Mute This Topic: https://groups.io/mt/95531719/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] UsbNetworkPkg/UsbRndis: Add USB RNDIS devices support

[Michael Brown]

On 08/12/2022 03:44, RichardHo [何明忠] via groups.io wrote:

+case PXE_OPFLAGS_RECEIVE_FILTER_DISABLE:
+  if (Cdb->CPBsize != PXE_CPBSIZE_NOT_USED) {
+Cdb->StatFlags = PXE_STATFLAGS_COMMAND_FAILED;
+Cdb->StatCode  = PXE_STATCODE_INVALID_CDB;
+  }
+
+  Nic->CanReceive = TRUE;
+  break;


This seems to be the only point in the entire driver that ever sets 
CanReceive = TRUE.


The result of this is that the device will be unable to receive unless 
at least one attempt has been made to *disable* the receive filters. 
This seems unlikely to be the intended behaviour.


It so happens that the combination of MnpDxe and SnpDxe usually *does* 
make an attempt to disable the receive filters, and so the bug is masked 
in normal operation.


I have added a workaround for this bug to iPXE:

  https://github.com/ipxe/ipxe/commit/ab1954638

I would suggest fixing your UndiReceiveFilter() function so that the 
workaround is not necessary.  I cannot follow the logic behind the 
CanReceive flag, so I am unable to suggest a patch, sorry.


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98257): https://edk2.groups.io/g/devel/message/98257
Mute This Topic: https://groups.io/mt/95531719/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] UsbNetworkPkg/UsbRndis: Add USB RNDIS devices support

[Michael Brown]

On 09/01/2023 23:50, Michael Brown wrote:
Coincidentally, I have just implemented a fix for this.  It works by 
implementing a simple credit-based rate limiter.  Calls to 
UsbEthReceive() are limited to 10 per second while the receive datapath 
is idle.  No limiting takes place while the receive datapath is active.


Richard Ho: please consider using this rate limiting approach (or 
something very similar).


I have updated the patch to run the timer callback at TPL_NOTIFY.  This 
seems to be required to avoid a long delay when the timer is disabled. 
(I have not investigated the root cause of this delay, but it is 100% 
reproducible.)


With this updated patch, I get almost no noticeable slowdown of the 
system and I am still able to receive data at around 100Mbps (which is 
about as good as it's likely to get, given the fundamental design flaws 
in EFI_USB_IO_PROTOCOL).


Updated patch:



diff --git a/UsbNetworkPkg/Include/Protocol/EdkIIUsbEthernetProtocol.h 
b/UsbNetworkPkg/Include/Protocol/EdkIIUsbEthernetProtocol.h

index df6ebc64ef..d0e2048114 100644
--- a/UsbNetworkPkg/Include/Protocol/EdkIIUsbEthernetProtocol.h
+++ b/UsbNetworkPkg/Include/Protocol/EdkIIUsbEthernetProtocol.h
@@ -160,6 +160,8 @@ typedef struct {
   UINT8  CurrentNodeAddress[PXE_MAC_LENGTH];
   UINT8  BroadcastNodeAddress[PXE_MAC_LENGTH];
   EFI_USB_DEVICE_REQUEST Request;
+  EFI_EVENT  RateLimiter;
+  UINTN  RateLimitCredit;
 } NIC_DATA;

 #define NIC_DATA_SIGNATURE  SIGNATURE_32('n', 'i', 'c', 'd')
diff --git a/UsbNetworkPkg/NetworkCommon/DriverBinding.h 
b/UsbNetworkPkg/NetworkCommon/DriverBinding.h

index 946727ffc9..ae1d3c201e 100644
--- a/UsbNetworkPkg/NetworkCommon/DriverBinding.h
+++ b/UsbNetworkPkg/NetworkCommon/DriverBinding.h
@@ -25,6 +25,8 @@
 #define RX_BUFFER_COUNT  32
 #define TX_BUFFER_COUNT  32
 #define MEMORY_REQUIRE   0
+#define RATE_LIMITER_INTERVAL100  // 10Hz
+#define RATE_LIMITER_BURST   10

 #define UNDI_DEV_SIGNATURE  SIGNATURE_32('u','n','d','i')
 #define UNDI_DEV_FROM_THIS(a)  CR(a, NIC_DEVICE, NiiProtocol, 
UNDI_DEV_SIGNATURE)
diff --git a/UsbNetworkPkg/NetworkCommon/PxeFunction.c 
b/UsbNetworkPkg/NetworkCommon/PxeFunction.c

index fd53a215a4..e02402dc57 100644
--- a/UsbNetworkPkg/NetworkCommon/PxeFunction.c
+++ b/UsbNetworkPkg/NetworkCommon/PxeFunction.c
@@ -29,6 +29,21 @@ API_FUNC  gUndiApiTable[] = {
   UndiReceive
 };

+STATIC
+VOID
+EFIAPI
+UndiRateLimiterCallback (
+  IN  EFI_EVENT Event,
+  IN  VOID  *Context
+  )
+{
+  NIC_DATA *Nic = Context;
+
+  if (Nic->RateLimitCredit < RATE_LIMITER_BURST) {
+Nic->RateLimitCredit++;
+  }
+}
+
 /**
   This command is used to determine the operational state of the UNDI.

@@ -100,9 +115,6 @@ UndiStart (
 Cdb->StatFlags = PXE_STATFLAGS_COMMAND_FAILED;
 Cdb->StatCode  = PXE_STATCODE_INVALID_CDB;
 return;
-  } else {
-Cdb->StatFlags = PXE_STATFLAGS_COMMAND_COMPLETE;
-Cdb->StatCode  = PXE_STATCODE_SUCCESS;
   }

   if (Nic->State != PXE_STATFLAGS_GET_STATE_STOPPED) {
@@ -120,14 +132,46 @@ UndiStart (
   Nic->PxeStart.UnMap_Mem = 0;
   Nic->PxeStart.Sync_Mem  = Cpb->Sync_Mem;
   Nic->PxeStart.Unique_ID = Cpb->Unique_ID;
-  Nic->State  = PXE_STATFLAGS_GET_STATE_STARTED;
+
+  Status = gBS->CreateEvent (
+EVT_TIMER | EVT_NOTIFY_SIGNAL,
+TPL_NOTIFY,
+UndiRateLimiterCallback,
+Nic,
+>RateLimiter
+);
+  if (EFI_ERROR (Status)) {
+goto ErrorCreateEvent;
+  }
+
+  Status = gBS->SetTimer (
+Nic->RateLimiter,
+TimerPeriodic,
+RATE_LIMITER_INTERVAL
+);
+  if (EFI_ERROR (Status)) {
+goto ErrorSetTimer;
+  }

   if (Nic->UsbEth->UsbEthUndi.UsbEthUndiStart != NULL) {
 Status = Nic->UsbEth->UsbEthUndi.UsbEthUndiStart (Cdb, Nic);
 if (EFI_ERROR (Status)) {
-  Cdb->StatFlags = PXE_STATFLAGS_COMMAND_FAILED;
+  goto ErrorUndiStart;
 }
   }
+
+  Nic->State = PXE_STATFLAGS_GET_STATE_STARTED;
+  Cdb->StatFlags = PXE_STATFLAGS_COMMAND_COMPLETE;
+  Cdb->StatCode  = PXE_STATCODE_SUCCESS;
+  return;
+
+ ErrorUndiStart:
+  gBS->SetTimer (>RateLimiter, TimerCancel, 0);
+ ErrorSetTimer:
+  gBS->CloseEvent (>RateLimiter);
+ ErrorCreateEvent:
+  Cdb->StatFlags = PXE_STATFLAGS_COMMAND_FAILED;
+  Cdb->StatCode  = PXE_STATCODE_DEVICE_FAILURE;
 }

 /**
@@ -183,6 +227,10 @@ UndiStop (
   Nic->PxeStart.Sync_Mem  = 0;
   Nic->State  = PXE_STATFLAGS_GET_STATE_STOPPED;

+  gBS->SetTimer (>RateLimiter, TimerCancel, 0);
+
+  gBS->CloseEvent (>RateLimiter);
+
   if (Nic->UsbEth->UsbEthUndi.UsbEthUndiStop != NULL) {
 Status = Nic->UsbEth->UsbEthUndi.UsbEthUndiStop (Cdb, Nic);
 if (EFI_ERROR (Status)) {
@@ -1483,6 +1531,7 @@ Receive (
   )
 {
   EFI_STATUS   Status;
+  EFI_TPL

Re: [edk2-devel] [PATCH 1/3] UsbNetworkPkg/UsbRndis: Add USB RNDIS devices support

[Michael Brown]

On 08/01/2023 10:41, tinhnguyen via groups.io wrote:
The root cause is that we spend a long time waiting for USB 
UsbBulkTransfer to complete, but if there is no data to communicate

-> it will always time out.


Coincidentally, I have just implemented a fix for this.  It works by 
implementing a simple credit-based rate limiter.  Calls to 
UsbEthReceive() are limited to 10 per second while the receive datapath 
is idle.  No limiting takes place while the receive datapath is active.


I have tried to match the existing code style (i.e. zero explanatory 
comments).


Richard Ho: please consider using this rate limiting approach (or 
something very similar).


Patch inline below:

diff --git a/UsbNetworkPkg/Include/Protocol/EdkIIUsbEthernetProtocol.h 
b/UsbNetworkPkg/Include/Protocol/EdkIIUsbEthernetProtocol.h

index df6ebc64ef..d0e2048114 100644
--- a/UsbNetworkPkg/Include/Protocol/EdkIIUsbEthernetProtocol.h
+++ b/UsbNetworkPkg/Include/Protocol/EdkIIUsbEthernetProtocol.h
@@ -160,6 +160,8 @@ typedef struct {
   UINT8  CurrentNodeAddress[PXE_MAC_LENGTH];
   UINT8  BroadcastNodeAddress[PXE_MAC_LENGTH];
   EFI_USB_DEVICE_REQUEST Request;
+  EFI_EVENT  RateLimiter;
+  UINTN  RateLimitCredit;
 } NIC_DATA;

 #define NIC_DATA_SIGNATURE  SIGNATURE_32('n', 'i', 'c', 'd')
diff --git a/UsbNetworkPkg/NetworkCommon/DriverBinding.h 
b/UsbNetworkPkg/NetworkCommon/DriverBinding.h

index 946727ffc9..ae1d3c201e 100644
--- a/UsbNetworkPkg/NetworkCommon/DriverBinding.h
+++ b/UsbNetworkPkg/NetworkCommon/DriverBinding.h
@@ -25,6 +25,8 @@
 #define RX_BUFFER_COUNT  32
 #define TX_BUFFER_COUNT  32
 #define MEMORY_REQUIRE   0
+#define RATE_LIMITER_INTERVAL100  // 10Hz
+#define RATE_LIMITER_BURST   10

 #define UNDI_DEV_SIGNATURE  SIGNATURE_32('u','n','d','i')
 #define UNDI_DEV_FROM_THIS(a)  CR(a, NIC_DEVICE, NiiProtocol, 
UNDI_DEV_SIGNATURE)
diff --git a/UsbNetworkPkg/NetworkCommon/PxeFunction.c 
b/UsbNetworkPkg/NetworkCommon/PxeFunction.c

index fd53a215a4..2a9b4f6111 100644
--- a/UsbNetworkPkg/NetworkCommon/PxeFunction.c
+++ b/UsbNetworkPkg/NetworkCommon/PxeFunction.c
@@ -29,6 +29,21 @@ API_FUNC  gUndiApiTable[] = {
   UndiReceive
 };

+STATIC
+VOID
+EFIAPI
+UndiRateLimiterCallback (
+  IN  EFI_EVENT Event,
+  IN  VOID  *Context
+  )
+{
+  NIC_DATA *Nic = Context;
+
+  if (Nic->RateLimitCredit < RATE_LIMITER_BURST) {
+Nic->RateLimitCredit++;
+  }
+}
+
 /**
   This command is used to determine the operational state of the UNDI.

@@ -100,9 +115,6 @@ UndiStart (
 Cdb->StatFlags = PXE_STATFLAGS_COMMAND_FAILED;
 Cdb->StatCode  = PXE_STATCODE_INVALID_CDB;
 return;
-  } else {
-Cdb->StatFlags = PXE_STATFLAGS_COMMAND_COMPLETE;
-Cdb->StatCode  = PXE_STATCODE_SUCCESS;
   }

   if (Nic->State != PXE_STATFLAGS_GET_STATE_STOPPED) {
@@ -120,14 +132,46 @@ UndiStart (
   Nic->PxeStart.UnMap_Mem = 0;
   Nic->PxeStart.Sync_Mem  = Cpb->Sync_Mem;
   Nic->PxeStart.Unique_ID = Cpb->Unique_ID;
-  Nic->State  = PXE_STATFLAGS_GET_STATE_STARTED;
+
+  Status = gBS->CreateEvent (
+EVT_TIMER | EVT_NOTIFY_SIGNAL,
+TPL_CALLBACK,
+UndiRateLimiterCallback,
+Nic,
+>RateLimiter
+);
+  if (EFI_ERROR (Status)) {
+goto ErrorCreateEvent;
+  }
+
+  Status = gBS->SetTimer (
+Nic->RateLimiter,
+TimerPeriodic,
+RATE_LIMITER_INTERVAL
+);
+  if (EFI_ERROR (Status)) {
+goto ErrorSetTimer;
+  }

   if (Nic->UsbEth->UsbEthUndi.UsbEthUndiStart != NULL) {
 Status = Nic->UsbEth->UsbEthUndi.UsbEthUndiStart (Cdb, Nic);
 if (EFI_ERROR (Status)) {
-  Cdb->StatFlags = PXE_STATFLAGS_COMMAND_FAILED;
+  goto ErrorUndiStart;
 }
   }
+
+  Nic->State = PXE_STATFLAGS_GET_STATE_STARTED;
+  Cdb->StatFlags = PXE_STATFLAGS_COMMAND_COMPLETE;
+  Cdb->StatCode  = PXE_STATCODE_SUCCESS;
+  return;
+
+ ErrorUndiStart:
+  gBS->SetTimer (>RateLimiter, TimerCancel, 0);
+ ErrorSetTimer:
+  gBS->CloseEvent (>RateLimiter);
+ ErrorCreateEvent:
+  Cdb->StatFlags = PXE_STATFLAGS_COMMAND_FAILED;
+  Cdb->StatCode  = PXE_STATCODE_DEVICE_FAILURE;
 }

 /**
@@ -183,6 +227,10 @@ UndiStop (
   Nic->PxeStart.Sync_Mem  = 0;
   Nic->State  = PXE_STATFLAGS_GET_STATE_STOPPED;

+  gBS->SetTimer (>RateLimiter, TimerCancel, 0);
+
+  gBS->CloseEvent (>RateLimiter);
+
   if (Nic->UsbEth->UsbEthUndi.UsbEthUndiStop != NULL) {
 Status = Nic->UsbEth->UsbEthUndi.UsbEthUndiStop (Cdb, Nic);
 if (EFI_ERROR (Status)) {
@@ -1506,8 +1554,13 @@ Receive (
   }

   while (1) {
+if (Nic->RateLimitCredit == 0) {
+  break;
+}
+
 Status = Nic->UsbEth->UsbEthReceive (Cdb, Nic->UsbEth, (VOID 
*)BulkInData, );

 if (EFI_ERROR (Status)) {
+  Nic->RateLimitCredit--;
   break;
 }


Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98206): 

Re: [edk2-devel] [PATCH 2/3] OvmfPkg: Add library to handle TPL from within nested interrupt handlers

[Michael Brown]

On 09/12/2022 15:22, Michael Brown wrote:

On 09/12/2022 15:02, Gerd Hoffmann wrote:
Add the Nested Interrupt TPL Library (NestedInterruptTplLib) to 
provide helper functions that can be used by nested interrupt

handlers in place of RaiseTPL()/RestoreTPL().






The idea looks sane to me and I couldn't spot any logic errors in
this.


Great! :)


Ping.  Is there anything else that needs to be done before this gets 
merged?  It would be good to have the regression bug fixed.


Thanks,

Michael


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97627): https://edk2.groups.io/g/devel/message/97627
Mute This Topic: https://groups.io/mt/95557698/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-