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

[Bobby Eshleman <bobbyeshleman@xxxxxxxxx>]

On Wed, Feb 01, 2023 at 09:06:21AM +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.
> 

I tend to favor the check because outcome #1 is so desirable, and I do
think that checking for medany is sufficient for the bulk of future
work. But that said, I do see Andrew's point now.

If Xen were to a) not relocate itself, b) be executed under the 2GB
range, c) be compiled under medlow, and d) the MMU is off or on but Xen
is identity mapped, then C functions should still be safe to call in
early boot. Checking medany does protect developers from accidental bad
configuration now, but it is a somewhat imperfect proxy.

One alternative that may be more long term is for the self relocation
code to be Kconfig-able and to require/force medany. Anyone wanting to
develop something like XIP could deselect relocation, which would allow
them to use medlow if they wanted/needed. The early C functions would
still be callable under both in this case. The help strings could offer
explanation too.

Thanks,
Bobby

> 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
>