Re: [PATCH v2 07/14] xen/riscv: introduce exception context

From: Julien Grall <julien@xen.org>
To: Oleksii <oleksii.kurochko@gmail.com>, xen-devel@lists.xenproject.org
Cc: Jan Beulich <jbeulich@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Gianluca Guida <gianluca@rivosinc.com>,
	Bob Eshleman <bobbyeshleman@gmail.com>,
	Alistair Francis <alistair.francis@wdc.com>,
	Connor Davis <connojdavis@gmail.com>,
	Bobby Eshleman <bobby.eshleman@gmail.com>
Subject: Re: [PATCH v2 07/14] xen/riscv: introduce exception context
Date: Tue, 31 Jan 2023 12:39:43 +0000	[thread overview]
Message-ID: <a3e2a1d3-0e64-82af-53d0-8b25cd1b7580@xen.org> (raw)
In-Reply-To: <72a7bdfc72161144df741e3a39f628874d88debd.camel@gmail.com>

On 31/01/2023 12:24, Oleksii wrote:
> Hi Julien,

Hi Oleksii,

> On Mon, 2023-01-30 at 22:11 +0000, Julien Grall wrote:
>> Hi,
>>
>> On 30/01/2023 11:40, Oleksii wrote:
>>> On Fri, 2023-01-27 at 14:54 +0000, Julien Grall wrote:
>>>> Hi Oleksii,
>>>>
>>>> On 27/01/2023 13:59, Oleksii Kurochko wrote:
>>>>> +static inline void wfi(void)
>>>>> +{
>>>>> +    __asm__ __volatile__ ("wfi");
>>>>
>>>> I have starred at this line for a while and I am not quite too
>>>> sure
>>>> to
>>>> understand why we don't need to clobber the memory like we do on
>>>> Arm.
>>>>
>>> I don't have an answer. The code was based on Linux so...
>>
>> Hmmm ok. It would probably wise to understand how code imported from
>> Linux work so we don't end up introducing bug when calling such
>> function.
>>
>>   From your current use in this patch, I don't expect any issue. That
>> may
>> chance for the others use.
>>
> Could you please share with me a link or explain what kind of problems
> may occur in case when we don't clobber the memory in the others use
> case during usage of "wfi" ?

I don't have a link and that's why I was asking the question here.

The concern I have is the following:

1)
    wfi();
    val = *addr;

2)
    *addr = val;
    wfi();

Is the compiler allowed to re-order the sequence so '*addr' will be read 
before 'wfi' or (for the second case) write after 'wfi'?

At the moment, I believe this is why we have the 'memory' clobber on 
Arm. But then I couldn't find any documentation implying that the 
compiler cannot do the re-ordering.

> 
> As I understand the reason for clobber the memory is to cause GCC to
> not keep memory values cached in registers across the
> assembler	instruction and not optimize stores/load to the memory.
> But current one instruction isn't expected to work with the memory so
> it should be safe enough to stall current hart ( CPU ) until an
> interrupt might need servicing.
> 
> Anyway we can change the code to:
>      __asm__ __volatile__ ("wfi" ::: "memory")
> In order to be sure that no problems will arise in the future.

As I wrote earlier, so far, I didn't suggest to change any code. I am 
simply trying to understand how this is meant to work.

One action may be that we can remove the memory clobber on Arm.

Cheers,

-- 
Julien Grall