|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [PATCH v3 12/43] arm64: add the basic helpers for arm64
On Mon, Apr 23, 2018 at 12:00:34PM +0100, Julien Grall wrote:
> 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.
Okay. I will check 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?
You are right, it should be dsb here.
(Steve ever pointed out it, but after too many rebasing, I found I missed the
correct code..)
I should not copy the FreeBSD code which is wrong here(may it is right in
FreeBSD).
For example, the wmb() is used in the GIC code, which should be dsb, not
dmb().
I will fix it in the next version.
Thanks
Huang Shijie
> 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |