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

From: Julien Grall <julien@xen.org>
To: Alistair Francis <alistair23@gmail.com>,
	Oleksii <oleksii.kurochko@gmail.com>
Cc: xen-devel@lists.xenproject.org,
	Bob Eshleman <bobbyeshleman@gmail.com>,
	Alistair Francis <alistair.francis@wdc.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Gianluca Guida <gianluca@rivosinc.com>,
	Connor Davis <connojdavis@gmail.com>,
	Bobby Eshleman <bobby.eshleman@gmail.com>
Subject: Re: [PATCH v7 1/2] xen/riscv: introduce early_printk basic stuff
Date: Tue, 31 Jan 2023 12:03:36 +0000	[thread overview]
Message-ID: <2f6a3b17-4e41-fe9a-1713-4942b3bd3585@xen.org> (raw)
In-Reply-To: <CAKmqyKMGiDiPRZBekdKan=+YduSmkB2DoWo5btrtVQ8nS3KMAg@mail.gmail.com>

On 31/01/2023 11:44, Alistair Francis wrote:
> On Sat, Jan 28, 2023 at 12:15 AM Oleksii <oleksii.kurochko@gmail.com> 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.

 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?

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@xen.org/
>> [2]
>> https://lore.kernel.org/xen-devel/0ec33871-96fa-bd9f-eb1b-eb37d3d7c982@xen.org/

[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/riscv/mm/init.c

-- 
Julien Grall