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

[Alistair Francis <alistair23@xxxxxxxxx>]

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:
> >>
> >> Hi Alistair, Bobby and community,
> >>
> >> I would like to ask your help with the following check:
> >> +/*
> >> + * early_*() can be called from head.S with MMU-off.
> >> + *
> >> + * The following requiremets should be honoured for early_*() to
> >> + * work correctly:
> >> + *    It should use PC-relative addressing for accessing symbols.
> >> + *    To achieve that GCC cmodel=medany should be used.
> >> + */
> >> +#ifndef __riscv_cmodel_medany
> >> +#error "early_*() can be called from head.S with MMU-off"
> >> +#endif
> >
> > I have never seen a check like this before.
>
> The check is in the Linux code, see [3].
>
> > I don't really understand
> > what it's looking for, if the linker is unable to call early_*() I
> > would expect it to throw an error. I'm not sure what this is adding.
>
> When the MMU is off during early boot, you want any C function called to
> use PC-relative address rather than absolute address. This is because
> the physical address may not match the virtual address.

Ah!

I forgot that Xen would be compiled for virtual addresses, I have
spent too much time running on systems without an MMU recently.

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

Alistair

>
> Cheers,
>
> >> Please take a look at the following messages and help me to decide if
> >> the check mentioned above should be in early_printk.c or not:
> >> [1]
> >> https://lore.kernel.org/xen-devel/599792fa-b08c-0b1e-10c1-0451519d9e4a@xxxxxxx/
> >> [2]
> >> https://lore.kernel.org/xen-devel/0ec33871-96fa-bd9f-eb1b-eb37d3d7c982@xxxxxxx/
>
> [3]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/riscv/mm/init.c
>
>
> --
> Julien Grall