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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hi all,

On Wed, 2023-02-01 at 09:06 +0000, Julien Grall wrote:
> 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@xxxxxxx>
> > > wrote:
> > > > On 31/01/2023 11:44, Alistair Francis wrote:
> > > > > On Sat, Jan 28, 2023 at 12:15 AM Oleksii
> > > > > <oleksii.kurochko@xxxxxxxxx> 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.
> 

I'll bring back the check and send the new version of the patch
tomorrow as Bobby&Alistair lean toward it.

Andrew, do you have other thoughts?

> Cheers,
> 
~ Oleksii