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

[Julien Grall <julien@xen.org>]

Hi Andrew,

On 01/02/2023 00:21, Andrew Cooper wrote:
> On 31/01/2023 11:17 pm, Alistair Francis wrote:
>> On Tue, Jan 31, 2023 at 10:03 PM Julien Grall <julien@xen.org> wrote:
>>> On 31/01/2023 11:44, Alistair Francis wrote:
>>>> On Sat, Jan 28, 2023 at 12:15 AM Oleksii <oleksii.kurochko@gmail.com> wrote:
>>>>
>>>   From my understanding, on RISC-V, the use of PC-relative address is
>>> only guaranteed with medany. So if you were going to change the cmodel
>>> (Andrew suggested you would), then early_*() may end up to be broken.
>>>
>>> This check serve as a documentation of the assumption and also help the
>>> developer any change in the model and take the appropriate action to
>>> remediate it.
>>>
>>>> I think this is safe to remove.
>>> Based on what I wrote above, do you still think this is safe?
>> With that in mind it's probably worth leaving in then. Maybe the
>> comment should be updated to make it explicit why we want this check
>> (I find the current comment not very helpful).
> 
> The presence of this check pre-supposes that Xen will always relocate
> itself, and this simply not true.  XIP for example typically does not
> 
> Furthermore it's not checking the property wanted.  The way C is
> compiled has no bearing on what relocation head.S uses to call it.

I think we are still cross-talking each other because this is not my 
point. I am not sure how to explain differently...

This check is not about how head.S call early_*() but making sure the C 
function can be executed with the MMU off. In which case, you will 
likely have VA != PA.

In theory head.S could apply some relocations before hand, but it may be 
too complicated to do it before calling early_*().

> 
> It is a given that we compile the hypervisor with a consistent code
> model, meaning that the properly actually making the check do what is
> wanted is also the property that means it is not needed in the first place.

See above.

> 
> This check is literally not worth the bytes it's taking up on disk, and
> not worth the added compiler time, nor the wasted time of whoever comes
> to support something like XIP in the future finds it to be in the way.
> Xen as a whole will really genuinely function as intended in models
> other than medany, and it demonstrates a misunderstanding of the topic
> to put in such a check to begin with.

Then enlight me :). So far it looks more like you are not understanding 
my point: I am talking about C function call with MMU off (e.g. VA != 
PA) and you are talking about Xen as a whole.

I guess the only way to really know is to implement a different model. 
At which point there are three possible outcome:
   1) We had the compiler check, it fired and the developper will take 
action to fix it (if needed).
   2) We have no compiler check, the developper knew what to do it and 
fixed it.
   3) We have no compiler check, the developper where not aware of the 
problem and we will waste time.

Even if you disagree with the check, then I think 1 is still the best 
because it would explain our assumption. Yes it may waste a few minutes 
to the developer switching the model. But that probably nothing compare 
to the time you could waste if you don't notice it.

Anyway, Alistair has now all the information to take an informative 
decision.

Cheers,

-- 
Julien Grall