| Message ID | 20180220110524.9050-4-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Create UART DebugLib implementation for runtime drivers | expand |
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
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
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
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
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 #
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(-) -- 2.11.0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel