Re: [Minios-devel] [PATCH v3 12/43] arm64: add the basic helpers for arm64

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 23/04/18 10:44, Huang Shijie wrote:
> On Wed, Apr 18, 2018 at 09:27:12PM +0100, Julien Grall wrote:
> > Hi,
> >
> > Most of my comments from <cc143169-08da-18b2-060f-8823909abe6e@xxxxxxxxxx>
> > has not been addressed. Some of them are probably have been repeated 3-4
> > times.... So please address them.
> sorry, I did reply the old email.  I will address them here..
>
> > On 16/04/2018 07:31, Huang Shijie wrote:
> > > This patch adds the basic helpers in headers for arm64:
> > >      1.) mb/rmb/wmb
> > >      2.) local_irq_disable/local_irq_enable
> > >      3.) local_irq_save/local_irq_restore/local_save_flags
> > >      4.) simplify the __ffs
> > >      5.) add BUG().
> > You also drop arm32 code making that patch quite confusing to read.
> yes. I can remove it to #2 patch.
> > > This patch refers to Chen Baozi's patch:
> > Do you mean "based on" rather than "refers to"?
> okay.
>
> > >       "Initial codes for arm64"
> > > And this patch also refers to FreeBSD code:
> > Ditto
> >
> > >       sys/arm64/include/atomic.h
> > You very likely have to retain the copyright from those files.
> do you mean I should copy the copyright here?
In the new header yes. If you  header will contain other code, then 
clearly write down what was imported.
> I think both the code is FreeBSD license, so I did not do that.
I don't understand what you mean. Code has been written by someone, that 
someone likely put a copyright in the header. So I think you have to 
retain it.
> > > Signed-off-by: Huang Shijie <shijie.huang@xxxxxxx>
> > > ---
> > >   include/arm/arm64/os.h | 28 +++++++++++++++++++
> > >   include/arm/os.h       | 75 ++++++++++++++++----------------------------------
> > >   2 files changed, 52 insertions(+), 51 deletions(-)
> > >   create mode 100644 include/arm/arm64/os.h
> > >
> > > diff --git a/include/arm/arm64/os.h b/include/arm/arm64/os.h
> > > new file mode 100644
> > > index 0000000..3d4aada
> > > --- /dev/null
> > > +++ b/include/arm/arm64/os.h
> > > @@ -0,0 +1,28 @@
> > > +#ifndef _ARM64_OS_H_
> > > +#define _ARM64_OS_H_
> > > +
> > > +#define BUG()           __asm__ __volatile("wfi" ::: "memory")
> > I don't understand that implementation. WFI means wait for interrupt and
> > will continue as soon as an interrupt came up. What you want here is dumping
> > the registers and exit.
> >
> > So most likely you want to use "brk" that will provoke a trap.
> okay, I will use "brk" in next version..
> > > +
> > > +static inline void local_irq_disable(void)
> > > +{
> > > +    __asm__ __volatile__("msr daifset, #2":::"memory");
> > Again, does the __ is really needed? Same for everywhere in that patch.
> > You said you will check it and never came back.
> We (Steve and I) ever checked the GCC document about this.
>
> https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html
>
> “The asm keyword is a GNU extension. When writing code that can be compiled 
> with -ansi and
> the various -std options, use __asm__ instead of asm (see Alternate Keywords).”
Thank you for the answer.

> > > +}
> > > +
> > > +static inline void local_irq_enable(void)
> > > +{
> > > +    __asm__ __volatile__("msr daifclr, #2":::"memory");
> > > +}
> > > +
> > > +#define local_irq_save(x) { \
> > > +    __asm__ __volatile__("mrs %0, daif; msr daifset, #2":"=r"(x)::"memory"); \
> > > +}
> > > +
> > > +#define local_irq_restore(x) { \
> > > +    __asm__ __volatile__("msr daif, %0"::"r"(x):"memory"); \
> > > +}
> > > +
> > > +#define local_save_flags(x) { \
> > > +    __asm__ __volatile__("mrs %0, daif":"=r"(x)::"memory"); \
> > > +}
> > > +
> > > +#endif
> > > diff --git a/include/arm/os.h b/include/arm/os.h
> > > index 6a1cc37..b10557a 100644
> > > --- a/include/arm/os.h
> > > +++ b/include/arm/os.h
> > > @@ -8,41 +8,42 @@
> > >   #include <mini-os/compiler.h>
> > >   #include <mini-os/kernel.h>
> > >   #include <xen/xen.h>
> > > +#include <arm64/os.h>
> > Same remark as for traps.h. You want to #ifdef that.
> do you mean I should add #ifdef here?
Yes.

[...]

> > >   void arch_fini(void);
> > >   void timer_handler(evtchn_port_t port, struct pt_regs *regs, void *ign);
> > >   extern void *device_tree;
> > > -#define BUG() while(1){asm volatile (".word 0xe7f000f0\n");} /* Undefined 
> > > instruction; will call our fault handler. */
> > > -
> > >   #define smp_processor_id() 0
> > >   #define barrier() __asm__ __volatile__("": : :"memory")
> > >   extern shared_info_t *HYPERVISOR_shared_info;
> > > -// disable interrupts
> > > -static inline void local_irq_disable(void) {
> > > -    __asm__ __volatile__("cpsid i":::"memory");
> > > -}
> > > -
> > > -// enable interrupts
> > > -static inline void local_irq_enable(void) {
> > > -    __asm__ __volatile__("cpsie i":::"memory");
> > > -}
> > > -
> > > -#define local_irq_save(x) { \
> > > -    __asm__ __volatile__("mrs %0, cpsr;cpsid i":"=r"(x)::"memory");    \
> > > -}
> > > -
> > > -#define local_irq_restore(x) {    \
> > > -    __asm__ __volatile__("msr cpsr_c, %0"::"r"(x):"memory");    \
> > > -}
> > > +#define isb()           __asm__ __volatile("isb" ::: "memory")
> > > +
> > > +/*
> > > + * Options for DMB and DSB:
> > > + *     oshld   Outer Shareable, load
> > > + *     oshst   Outer Shareable, store
> > > + *     osh     Outer Shareable, all
> > > + *     nshld   Non-shareable, load
> > > + *     nshst   Non-shareable, store
> > > + *     nsh     Non-shareable, all
> > > + *     ishld   Inner Shareable, load
> > > + *     ishst   Inner Shareable, store
> > > + *     ish     Inner Shareable, all
> > > + *     ld      Full system, load
> > > + *     st      Full system, store
> > > + *     sy      Full system, all
> > > + */
> > > +#define dmb(opt)        __asm__ __volatile("dmb " #opt ::: "memory")
> > > +#define dsb(opt)        __asm__ __volatile("dsb " #opt ::: "memory")
> > > -#define local_save_flags(x)    { \
> > > -    __asm__ __volatile__("mrs %0, cpsr":"=r"(x)::"memory");    \
> > > -}
> > > +#define mb()            dmb(sy) /* Full system memory barrier all */
> > > +#define wmb()           dmb(st) /* Full system memory barrier store */
> > > +#define rmb()           dmb(ld) /* Full system memory barrier load */
> > ld does not exist for arm32. So you want to ifdef that.
> You mean I should add #ifdef here?
Yes.

> > But again, can you explain why you use dmb(...) and not dsb(...) here.
> dsb() is more strict then the dmb().
>
> "This enforces the same ordering as the Data Memory Barrier, but has the 
> additional
> effect of blocking execution of any further instructions, not just loads or 
> stores,
> or both, until synchronization is complete."
This does not answer my question. My point is you are moving to a more 
relaxed barrier. There are a comment on the code you dropped saying: "We 
probably only need 'dmb' here, but we'll start by being paranoid".
I am not against using 'dsb'. My main concern is this patch is altering 
the semantics of the barrier. I haven't looked at the code but I assume 
you looked at all callers checking whether the relaxation is fine, am I 
right? Also, what is the expected semantic from the common code for this 
barrier?
In anycase, such change should be documented in the commit message and 
probably in the code. Because the semantic of *mb() are fairly different 
from what was done on arm32.
Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel