Re: [PATCH] xen/arm: debug-pl011.inc: Use macros instead of hardcoded values

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 17 Nov 2022, at 09:29, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 17/11/2022 08:34, Michal Orzel wrote:
>> Hi Julien,
>> On 16/11/2022 19:37, Julien Grall wrote:
>>> 
>>> 
>>> Hi Michal,
>>> 
>>> On 16/11/2022 18:05, Michal Orzel wrote:
>>>> On 16/11/2022 16:56, Julien Grall wrote:
>>>>> 
>>>>> 
>>>>> On 16/11/2022 14:45, Michal Orzel wrote:
>>>>>> Hi Julien,
>>>>> 
>>>>> Hi Michal,
>>>>> 
>>>>>>> 
>>>>>>>> and use it in the pl011-debug files (+ there is a question whether we 
>>>>>>>> should define WLEN_7-5 for completeness).
>>>>>>> 
>>>>>>> I would not define WLEN_7-5. That said, I wonder if we really need to
>>>>>>> set the baud rate & co here?
>>>>>>> 
>>>>>>> AFAICT the runtime driver never touch them. The reasoning is the
>>>>>>> firmware is responsible to configure the serial. Therefore, I would
>>>>>>> consider to drop the code (setting UARTCR might still be necessary).
>>>>>> I do not really agree because the current behavior was done on purpose.
>>>>> 
>>>>> EARLY_UART_PL011_BAUD_RATE is only used for very early debugging (this
>>>>> is protected by CONFIG_DEBUG and CONFIG_EXPERT). This is not a
>>>>> production ready code.
>>>> I am fully aware of it. I just found it useful but I understand the global 
>>>> reasoning.
>>>> 
>>>>> 
>>>>>> At the moment early_uart_init is called only if 
>>>>>> EARLY_UART_PL011_BAUD_RATE is set to a value != 0.
>>>>>> This is done in order to have flexibility to either stick to what 
>>>>>> firmware/bootloader configured or to change this
>>>>>> configuration by specifying the EARLY_UART_PL011_BAUD_RATE (useful when 
>>>>>> you do not know what
>>>>>> the firmware configured).
>>>>> The chances are that you want to use the baud rate that was configured
>>>>> by the firmware. Otherwise, you would need to change the configuration
>>>>> of minicom (or whatever you used) to get proper output for the firmware
>>>>> and then Xen.
>>>>> 
>>>>> Furthermore, as I wrote before, the runtime driver doesn't configure the
>>>>> baud rate. This was removed in Xen 4.7 (see commit 2048e17ca9df
>>>>> "drivers/pl011: Don't configure baudrate") because it was buggy and this
>>>>> code is not simple.
>>>>> 
>>>>> So it makes no sense to configure the baud rate when using early printk
>>>>> but not the runtime driver.
>>>> Ok, so we will get rid of EARLY_UART_PL011_BAUD_RATE config and setting 
>>>> the bd
>>>> in the early uart code. Now, what about setting "8n1"? The runtime driver 
>>>> sets them
>>>> as well as the early code. It can also be set to a different value from 
>>>> the firmware
>>>> (unlikely but it can happen I think). In any case, if we decide to do what 
>>>> the runtime driver
>>>> does, I reckon setting LCR_H should be kept in early code.
>>> 
>>> Good question. I think, you would end up with the same issue I mentioned
>>> above if the firmware and Xen have different line control registers
>>> (tools like minicom/screen would ask for it).
>>> 
>>> So I am on the fence here. In one way, it seems pointless keep it. But
>>> on the other hand, Xen has always set it. So I have no data to prove
>>> this will be fine everywhere.
>> If we are relying on the firmware[1] to configure the baud rate, it is not 
>> very wise
>> not to rely on it to configure the LCR. Looking at the other serial drivers 
>> in Xen,
>> we have a real mismatch in what is being configured. Some of the drivers 
>> (omap, imx),
>> apart from setting 8n1 also set the baud rate explicitly to 115200 and 
>> almost all of them
>> do set 8n1. In that case we will not benefit too much from fixing just pl011.
> It is not great that Xen hardcode the baud rate (I can't remember whether 
> there was a reason), but I don't think the consistency is necessary here (see 
> more below).
> 
>> On the other hand, Xen follows the zImage/Image protocols for ARM [2] which 
>> do not
>> state that serial port initializing is something mandatory. This could 
>> indicate that
>> the firmware does not really need to configure the serial.
> 
> The firmware doesn't need to configure the serial and yes in theory Xen 
> should configure the baud rate and parity based on the firmware table.
> 
> However, this is a trade off between complexity and benefits. The patch I 
> mentioned earlier has been removed nearly 6 years ago and I haven't seen 
> anyone reporting any issues.
> 
> Hence why I think for the PL011 it is not worth looking [3] at the baud rate 
> and instead removing it completely in the early PL011 code as well.
> 
> That said, if you feel strongly adding support for baud rate then I will be 
> happy to review the patch.

Just one remark here: In some cases the firmware might use a different console 
than the serial one (for example a graphical one) and Xen could be the first to 
use the serial console.
So there might be cases where if we do not set a default value, the console 
will not be configured at all.

I think the best solution here would be to have a CONFIG setting so that 
someone compiling Xen could choose between “keeping firmware config” or “force 
a config”.

Cheers
Bertrand

> 
>> [1] It is not stated anywhere in our docs.
> 
> Our docs are not perfect. Patches are welcomed for improvement. Although, I 
> think the statement should only be for driver where we don't set the baud 
> rate. For the others, we should leave it as is unless you can prove this is 
> not necessary (we don't want to break existing setup).
> 
>> [2] BTW: our docs/misc/arm/booting contains invalid links to the kernel 
>> docs. I guess
>> this wants to be fixed.
> 
> Patches are welcomed.
> 
> [3] I do have a large list of more critical bugs that I will be happy to 
> share if you are looking for improving Xen.
> 
> -- 
> Julien Grall