Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: Initialise serial port early in StandaloneMmEntryPoint

["Laszlo Ersek" <lersek@redhat.com>]

On 1/10/24 17:13, levi.yun wrote:
>> My personal conclusion in that thread was [1], and correspondingly,
>> commit 5087a0773645 ("ArmVirtPkg/FdtPL011SerialPortLib: initialize
>> implicitly", 2023-10-07). In the end, the only tractable solution was to
>> initialize the serial port (hardware, and library instance) exactly
>> once, in (a) the constructor, or (b) the explicit SerialPortInitialize()
>> call, or (c) any SerialPortLib API, whichever occurred first. (And (a)
>> and (b) can be coalesced, because SerialPortInitialize() can be marked
>> as the constructor for the lib instance.)
>>
>> [1]
>> http://mid.mail-archive.com/542db9e1-cd28-27a2-3a98-5b0c85cd7c79@redhat.com
>>      https://edk2.groups.io/g/devel/message/109235
>>
>> Laszlo
>>
> In my personal thinking, It's better to make new interface like
> 
> RETURN_STATUS
> EFIAPI
> SerialPortInitializeEarly (
> VOID
>   );
> 
> to solve this problem.
> 
> Because, It makes a memory permission fault
> when we call SerialPortInitialize like
> ArmVirtPkg/Library/FdtPL011SerialPortLib
> where try to modify global variable.
> 
> At the _ModuleEntryPoint of StandAloneMmCore which is FIRST entry from TF-A
> All of Image area is mapped as RO+X, so before load StandaloneMmCore,
> We couldn't write global variable.
> 
> the purposes of above interface are:
>     - Initalize serial port to use in early environment only (like
> StandAloneMmCore Entry Point) where couldn't write global variable by
> static information (FixedPcd).
>     - It presume that all setting configured by it will be overwritten
> by SerialPortInitialize.

This is not a scalable solution (assuming I understand your proposal
right). It would require the SerialPortLib *class* (i.e., header) to
grow a new API called SerialPortInitializeEarly(), and then all existent
SerialPortLib *instances* -- and there are too many of them -- would
have to implement that function, even if the new function were empty in
several particular library instances.

If you don't have writeable global variables at the time
SerialPortInitialize() is called, then there are two options:

(a) the common option is to just not use global variables for caching
state, and to perform the serial port init on every API call.

(b) A somewhat nasty / limited, but functional approach could be to
check the page protection covering the global variable. Something like this:

STATIC BOOLEAN  mInitialized;

...

  UINTN    AddressOfGlobal;
  BOOLEAN  GlobalsWriteable;

  if (mInitialized) {
    //
    // we're done, nothing to be done
    //
    return RETURN_SUCCESS;
  }

  //
  // here, perform the serial port initialization
  //
  ...

  //
  // finally:
  //
  AddressOfGlobal = (UINTN)&mInitialized;
  GlobalsWriteable = CheckWritable (AddressOfGlobal);
  if (GlobalsWriteable) {
    mInitialized = TRUE;
  }

This will keep initing the serial port upon every API call until the
global variable becomes writeable, and then the next API call will init
the serial port for one last time, and also prevent further page table
checks.

The CheckWritable() function is an implementation detail. In the DXE
phase, it could be implemented (I think?) with the
GetMemorySpaceDescriptor() DXE service, or perhaps even the
EFI_MEMORY_ATTRIBUTE_PROTOCOL.GetMemoryAttributes() UEFI protocol member
function. In the standalone MM core, CheckWritable() could walk the page
tables explicitly. The idea is, either way, to *predict* whether writing
to "mInitialized" would trap.

Now I think that speculative / out of order execution could actually
trigger the trap *before* GlobalsWriteable is calculated; however, I
think such a trap should be architecturally hidden (i.e., invisible). I
think at worst we could need a compiler barrier (maybe throw in some
"volatile" for GlobalsWriteable and mInitialized), so that the
*compiler* not try to reorder the accesses. But even that sounds like a
stretch.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113535): https://edk2.groups.io/g/devel/message/113535
Mute This Topic: https://groups.io/mt/103540969/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-