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

[Julien Grall <julien@xxxxxxx>]

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