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

["Michael Brown" <mcb30@ipxe.org>]

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-