Re: [PATCH 3/3] MdePkg/BaseDebugLibSerialPort: blacklist for use by DXE runtime drivers

[Laszlo Ersek <lersek@redhat.com>]

On 02/20/18 19:02, Kinney, Michael D wrote:
> I do not agree with this change.
> 
> If the PCDs that describe the UART are for a UART
> that is owned by the FW and hidden from the OS, then
> this lib can work well at runtime.

Does that imply that we should do the runtime disablement in the
SerialPortLib instance that underlies BaseDebugLibSerialPort?

I think it is an integral part of the feature (or, well, fix) that the
runtime incompatibility be caught at edk2 platform build time. In other
words, *some* library instance in edk2 should blacklist
DXE_RUNTIME_DRIVER modules (and the counterpart library instance should
be appropriate for DXE_RUNTIME_DRIVER modules only, and handle EBS, to
dynamically disable use of the serial port).

Are you suggesting that we implement this at the PL011 SerialPortLib
instance level?

Thanks!
Laszlo


>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, February 20, 2018 9:00 AM
>> To: Leif Lindholm <leif.lindholm@linaro.org>; Ard
>> Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: edk2-devel@lists.01.org; Ni, Ruiyu
>> <ruiyu.ni@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; Zeng, Star
>> <star.zeng@intel.com>; afish@apple.com
>> Subject: Re: [PATCH 3/3] MdePkg/BaseDebugLibSerialPort:
>> blacklist for use by DXE runtime drivers
>>
>> On 02/20/18 15:22, Leif Lindholm wrote:
>>> On Tue, Feb 20, 2018 at 11:05:24AM +0000, Ard
>> Biesheuvel wrote:
>>>> BaseDebugLibSerialPort is not suitable for use by
>> DXE_RUNTIME_DRIVER
>>>> modules, so blacklist it for use by such modules.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement
>> 1.1
>>>> Signed-off-by: Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>
>>>> ---
>>>>
>> MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerial
>> Port.inf | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git
>> a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSeri
>> alPort.inf
>> b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSeri
>> alPort.inf
>>>> index 823511b22f6b..25da1fb9363a 100644
>>>> ---
>> a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSeri
>> alPort.inf
>>>> +++
>> b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSeri
>> alPort.inf
>>>> @@ -21,7 +21,7 @@ [Defines]
>>>>    FILE_GUID                      = BB83F95F-EDBC-
>> 4884-A520-CD42AF388FAE
>>>>    MODULE_TYPE                    = BASE
>>>>    VERSION_STRING                 = 1.0
>>>> -  LIBRARY_CLASS                  = DebugLib
>>>> +  LIBRARY_CLASS                  = DebugLib|SEC
>> PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_SMM_DRIVER
>> DXE_SAL_DRIVER UEFI_DRIVER UEFI_APPLICATION SMM_CORE
>> MM_STANDALONE MM_CORE_STANDALONE
>>>
>>> Not a comment on this patch as such (which looks
>> sensible to me), but
>>> what you're doing here isn't blacklisting
>> DXE_RUNTIME_DRIVER, but
>>> rather whitelisting every other module type.
>>>
>>> Could we use a ! operator added to the syntax?
>>
>> No, I don't think so, based on the INF file spec.
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> (
>>
>> A future customization would be to give a similar
>> treatment to SMM (or
>> "MM") drivers. While MM has its own set of (identity
>> mapped) page
>> tables, and therefore I'd expect an MMIO serial port to
>> "just work", we
>> still shouldn't mess with the serial port once the OS
>> owns it,
>> regardless of the fact that we're in MM. So, for that,
>> the lib instance
>> meant for MM drivers would have to catch EBS too.
>>
>> Of course, that doesn't work the same way -- the
>> SMM_CORE catches the
>> normal EBS notification, and "forwards" it into the MM
>> protocol database
>> (see SmmExitBootServicesHandler() in
>> "MdeModulePkg/Core/PiSmmCore/PiSmmCore.c"). So the MM
>> lib instance would
>> have to register an MM protocol notify for
>> "gEdkiiSmmExitBootServicesProtocolGuid".
>>
>> But, that's for the future at best.
>>
>> *This* lib instance should remain correct for the
>> SMM_CORE itself, however.
>>
>> )
>>
>> Thanks,
>> Laszlo
>>
>>
>>
>>> /
>>>     Leif
>>>
>>>>    CONSTRUCTOR                    =
>> BaseDebugLibSerialPortConstructor
>>>>
>>>>  #
>>>> --
>>>> 2.11.0
>>>>
>