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

[Alistair Francis <alistair23@xxxxxxxxx>]

On Thu, Feb 2, 2023 at 3:34 AM Bobby Eshleman <bobbyeshleman@xxxxxxxxx> wrote:
>
> 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.

Yeah, I agree. It probably is worth leaving the check in for now, even
if it's imperfect. We can always remove it in the future if required.

Alistair

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