Re: [PATCH edk2-platforms 1/2] Platform, Silicon: use DxeRuntimeDebugLibSerialPort for runtime DXE drivers

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 27 February 2018 at 17:47, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/27/18 18:33, Ard Biesheuvel wrote:
>> On 27 February 2018 at 16:10, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> On 27 February 2018 at 16:09, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> On 02/27/18 15:21, Ard Biesheuvel wrote:
>>>>> On 27 February 2018 at 10:43, Laszlo Ersek <lersek@redhat.com> wrote:
>>>>>> On 02/27/18 10:23, Ard Biesheuvel wrote:
>>>>>>> On 27 February 2018 at 01:50, Guo Heyi <heyi.guo@linaro.org> wrote:
>>>>>>>> Hi Ard,
>>>>>>>>
>>>>>>>> Sorry for the late of seeing this patch. I have one question: why don't we
>>>>>>>> implement a runtime serial port lib, which will switch UART base address in
>>>>>>>> virtual address map change? I think this will be useful when we want to debug
>>>>>>>> runtime driver in OS stage. And if we have a runtime version of SerialPortLib,
>>>>>>>> then we don't need a runtime version of DebugLib which just disable touching
>>>>>>>> serial port.
>>>>>>>>
>>>>>>>
>>>>>>> Well, only if the serial port is not exposed to the OS as well. The
>>>>>>> Linux PL011 driver is especially easy to confuse, and having both the
>>>>>>> firmware and the OS control it at the same time is likely to cause
>>>>>>> problems.
>>>>>>>
>>>>>>> However, I do agree that having the ability to assign a UART to DEBUG
>>>>>>> at runtime is useful, and so I do intend to create a runtime version
>>>>>>> of the PL011 library, in which case DxeRuntimeDebugLibSerialPort can
>>>>>>> be replaced with BaseDebugLibSerialPort for DXE_RUNTIME_DRIVER
>>>>>>> modules.
>>>>>>>
>>>>>>
>>>>>> Converting the PL011 base address from phys to virt can be done in the
>>>>>> library instance, yes (and then every runtime driver module linked
>>>>>> against this library instance will individually convert the address for
>>>>>> its own use). The messier aspect is getting the PL011 base address into
>>>>>> the UEFI memmap, marked as MMIO / RUNTIME, so that the OS assign a
>>>>>> virtual mapping to it in the first place.
>>>>>>
>>>>>> The flash drivers generally do this with AddMemorySpace /
>>>>>> SetMemorySpaceAttributes.
>>>>>>
>>>>>> (Side point: while I agree that those are good APIs to invoke, I think
>>>>>> they should also call AllocateMemorySpace right after; otherwise a
>>>>>> "stray" AllocateMemorySpace elsewhere could allocate a chunk out of the
>>>>>> middle of what the flash driver *thinks* it owns.)
>>>>>>
>>>>>> The question is where this pair (or triplet) of GCD APIs should be called:
>>>>>>
>>>>>> - In a platform DXE driver? Perhaps.
>>>>>>
>>>>>> - In the DebugLib instance constructor / destructor? That could result
>>>>>> in some ugly reference counting -- you might want to keep the PL011 area
>>>>>> registered in GCD as long as *at least one* such runtime driver is loaded.
>>>>>>
>>>>>> This is different from the flash driver because the flash driver is the
>>>>>> sole runtime DXE (or SMM) driver that accesses & owns the flash MMIO
>>>>>> range. With the PL011 register block, that's not the case; all runtime
>>>>>> drivers that produce debug messages own it co-operatively.
>>>>>>
>>>>>
>>>>> Given that this SerialPortLib will be of the DXE_RUNTIME_DRIVER type
>>>>> anyway, couldn't we base it on a generic RuntimeUart protocol that we
>>>>> can depex on in the library, and produce in a separate [singleton]
>>>>> runtime DXE driver, which takes care of the UART initialization as
>>>>> well as the GCD memory space handling?
>>>>
>>>> Yes, that should totally work.
>>>>
>>>>> In fact, we could modify DxeRuntimeDebugLibSerialPort to attach to
>>>>> this protocol if it exists at EndOfDxe, and either do nothing at
>>>>> runtime (as it does currently) or produce UART output via the protocol
>>>>> if it exists.
>>>>
>>>> This is a good idea IMO. What about the following: since you'd have to
>>>> register an event notification function for EndOfDxe, why not register a
>>>> protocol notify directly for the runtime UART protocol?
>>>>
>>
>> What I also like about this approach is that it defers the decision
>> whether to enable the DEBUG UART at OS runtime from compile time to
>> runtime [where 'runtime' is a tad overloaded here]
>>
>> IOW, you can use a variable, PCD or simply do 'load
>> fs0:\UartRuntimeDxe.efi' from the Shell to promote a 'runtime silent'
>> DEBUG build to one that produces DEBUG output while running under the
>> OS.
>>
>> I guess the only problem is that DxeRuntimeDebugLibSerialPort lives in
>> MdePkg, and so any extensions to it involving protocols require an ECR
>> against the PI spec first.
>>
>
> This is a brand new library instance, not many platform DSCs use it just
> yet. Can we simply move it to MdeModulePkg? Do we see any downsides to
> not offering the lib instance in MdePkg?
>

Good point. I think that should be fine