1. Patchew
  2. EDK2
  3. [edk2] [PATCH 0/3] Create UART DebugLib implementation for runtime drivers
  4. View patch

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

Ard Biesheuvel posted 3 patches 6 years, 10 months ago
There is a newer version of this series
[edk2] [PATCH 3/3] MdePkg/BaseDebugLibSerialPort: blacklist for use by DXE runtime drivers
Posted by Ard Biesheuvel 6 years, 10 months ago 
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/BaseDebugLibSerialPort.inf | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
index 823511b22f6b..25da1fb9363a 100644
--- a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
+++ b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.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
   CONSTRUCTOR                    = BaseDebugLibSerialPortConstructor
 
 #
-- 
2.11.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 3/3] MdePkg/BaseDebugLibSerialPort: blacklist for use by DXE runtime drivers
Posted by Leif Lindholm 6 years, 10 months ago 
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/BaseDebugLibSerialPort.inf | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> index 823511b22f6b..25da1fb9363a 100644
> --- a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> +++ b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.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?

/
    Leif

>    CONSTRUCTOR                    = BaseDebugLibSerialPortConstructor
>  
>  #
> -- 
> 2.11.0
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 3/3] MdePkg/BaseDebugLibSerialPort: blacklist for use by DXE runtime drivers
Posted by Laszlo Ersek 6 years, 10 months ago 
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/BaseDebugLibSerialPort.inf | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
>> index 823511b22f6b..25da1fb9363a 100644
>> --- a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
>> +++ b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.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
>>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 3/3] MdePkg/BaseDebugLibSerialPort: blacklist for use by DXE runtime drivers
Posted by Kinney, Michael D 6 years, 10 months ago 
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.

Mike

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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 3/3] MdePkg/BaseDebugLibSerialPort: blacklist for use by DXE runtime drivers
Posted by Laszlo Ersek 6 years, 10 months ago 
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
>>>>
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.