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 0x01000000
- 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]
-=-=-=-=-=-=-=-=-=-=-=-
