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