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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hi Julien,

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" ?

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.

> > > FWIW, Linux is doing the same, so I guess this is correct. For
> > > Arm we
> > > also follow the Linux implementation.
> > > 
> > > But I am wondering whether we are just too strict on Arm, RISCv
> > > compiler
> > > offer a different guarantee, or you expect the user to be
> > > responsible
> > > to
> > > prevent the compiler to do harmful optimization.
> > > 
> > > > +/*
> > > > + * panic() isn't available at the moment so an infinite loop
> > > > will
> > > > be
> > > > + * used temporarily.
> > > > + * TODO: change it to panic()
> > > > + */
> > > > +static inline void die(void)
> > > > +{
> > > > +    for( ;; ) wfi();
> > > 
> > > Please move wfi() to a newline.
> > Thanks.
> > 
> > I thought that it is fine to put into one line in this case but
> > I'll
> > move it to a newline. It's fine.
> 
> I am not aware of any place in Xen where we would combine the lines.
> Also, you want a space after 'for'.
> 
> Cheers,
> 

~ Oleksii