Re: [PATCH v6 1/2] xen/riscv: introduce early_printk basic stuff

[Julien Grall <julien@xen.org>]

Hi Oleksii,

On 27/01/2023 11:49, Oleksii wrote:
> On Fri, 2023-01-27 at 11:26 +0000, Julien Grall wrote:
>> Hi,
>>
>> On 27/01/2023 11:15, Oleksii Kurochko wrote:
>>> Because printk() relies on a serial driver (like the ns16550
>>> driver)
>>> and drivers require working virtual memory (ioremap()) there is not
>>> print functionality early in Xen boot.
>>>
>>> The patch introduces the basic stuff of early_printk functionality
>>> which will be enough to print 'hello from C environment".
>>>
>>> Originally early_printk.{c,h} was introduced by Bobby Eshleman
>>> (
>>> https://github.com/glg-rv/xen/commit/a3c9916bbdff7ad6030055bbee7e53d
>>> 1aab71384)
>>> but some functionality was changed:
>>> early_printk() function was changed in comparison with the original
>>> as
>>> common isn't being built now so there is no vscnprintf.
>>>
>>> This commit adds early printk implementation built on the putc SBI
>>> call.
>>>
>>> As sbi_console_putchar() is already being planned for deprecation
>>> it is used temporarily now and will be removed or reworked after
>>> real uart will be ready.
>>>
>>> Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com>
>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>> Reviewed-by: Bobby Eshleman <bobby.eshleman@gmail.com>
>>> ---
>>> Changes in V6:
>>>       - Remove __riscv_cmodel_medany check from early_printk.c
>>
>> Why? I know Andrew believed this is wrong, but I replied back with my
>> understanding and saw no discussion afterwards explaining why I am
>> incorrect.
>>
>> I am not a maintainer of the code here, but I don't particularly
>> appreciate comments to be ignored. If there was any discussion on
>> IRC,
>> then please summarize them here.
> Sorry, I have to mentioned that in the description of patch series.

I think this should be part of the section --- in this patch as this 
makes easier to know what changes have been done.

> 
> There is no any specific reason to remove and only one reason I decided
> to remove the check was that the check wasn't present in original
> Alistair/Bobby patches and based on my experiments with that patches
> all worked fine. ( at least with some additional patches from my side I
> was able to run Dom0 with console )

The lines you removed only confirm that the file was built with the 
given model and if it is incorrect then the compilation will fail. There 
are no change in behavior expected past the compilation.

So I am not quite too sure what sort of experiment you did. Did you try 
to change the model used to build Xen?

Now if you (or anyone else) can tell me that there will be no issues if 
the model is changed. Then yes, I will agree that the check is unnecessary.

The alternative is you say that you are happy to accept the risk if the 
model is changed. If I were the maintainer, that would not be something 
I would agree with because "wrong" unwritten assumptions are a pain to 
debug. So I much prefer a "wrong" written assumptions that would tip me 
that the author thought the code would not work otherwise.

This is quite easy to remove the check after the fact if it is not correct.

I am not the maintainer of the code, so if this is what they want then 
so be it. But it needs to be explicitely stated. So far, the reviewed-by 
from Bobby was with the check, so it would imply that he was happy with 
it...

Cheers,

-- 
Julien Grall