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

From: Andrew Cooper <amc96@srcf.net>
To: Alistair Francis <alistair23@gmail.com>
Cc: Oleksii <oleksii.kurochko@gmail.com>,
	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>,
	Julien Grall <julien@xen.org>
Subject: Re: [PATCH v7 1/2] xen/riscv: introduce early_printk basic stuff
Date: Wed, 1 Feb 2023 00:21:46 +0000	[thread overview]
Message-ID: <9d5841b2-5d0b-390f-6e95-cf492e99e5a2@srcf.net> (raw)
In-Reply-To: <CAKmqyKNwH4-D3dKGQEsW_Zup4OT32C1RwaA7_Sey4fo_jOzFcA@mail.gmail.com>

On 31/01/2023 11:17 pm, Alistair Francis wrote:
> On Tue, Jan 31, 2023 at 10:03 PM Julien Grall <julien@xen.org> wrote:
>> On 31/01/2023 11:44, Alistair Francis wrote:
>>> On Sat, Jan 28, 2023 at 12:15 AM Oleksii <oleksii.kurochko@gmail.com> 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.

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.

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.

~Andrew