Hi Julien, On Wed, 2020-07-01 at 18:46 +0100, Julien Grall wrote: > Title: s/hypervizor/hypervisor/
Thank you for pointing :) I will fix it in the next version. > > On 01/07/2020 17:29, Anastasiia Lukianenko wrote: > > From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com> > > > > Port hypervizor related code from mini-os. Update essential > > Ditto. > > But I would be quite cautious to import code from mini-OS in order > to > support Arm. The port has always been broken and from a look below > needs > to be refined for Arm. We were referencing the code of Mini-OS from [1] by Huang Shijie and Volodymyr Babchuk which is for ARM64, so we hope this part should be ok. [1] https://github.com/zyzii/mini-os.git > > > arch code to support required bit operations, memory barriers etc. > > > > Copyright for the bits ported belong to at least the following > > authors, > > please see related files for details: > > > > Copyright (c) 2002-2003, K A Fraser > > Copyright (c) 2005, Grzegorz Milos, gm...@cam.ac.uk,Intel Research > > Cambridge > > Copyright (c) 2014, Karim Allah Ahmed <karim.allah.ah...@gmail.com> > > > > Signed-off-by: Oleksandr Andrushchenko < > > oleksandr_andrushche...@epam.com> > > Signed-off-by: Anastasiia Lukianenko < > > anastasiia_lukiane...@epam.com> > > --- > > arch/arm/include/asm/xen/system.h | 96 +++++++++++ > > common/board_r.c | 11 ++ > > drivers/Makefile | 1 + > > drivers/xen/Makefile | 5 + > > drivers/xen/hypervisor.c | 277 > > ++++++++++++++++++++++++++++++ > > include/xen.h | 11 ++ > > include/xen/hvm.h | 30 ++++ > > 7 files changed, 431 insertions(+) > > create mode 100644 arch/arm/include/asm/xen/system.h > > create mode 100644 drivers/xen/Makefile > > create mode 100644 drivers/xen/hypervisor.c > > create mode 100644 include/xen.h > > create mode 100644 include/xen/hvm.h > > > > diff --git a/arch/arm/include/asm/xen/system.h > > b/arch/arm/include/asm/xen/system.h > > new file mode 100644 > > index 0000000000..81ab90160e > > --- /dev/null > > +++ b/arch/arm/include/asm/xen/system.h > > @@ -0,0 +1,96 @@ > > +/* > > + * SPDX-License-Identifier: GPL-2.0 > > + * > > + * (C) 2014 Karim Allah Ahmed <karim.allah.ah...@gmail.com> > > + * (C) 2020, EPAM Systems Inc. > > + */ > > +#ifndef _ASM_ARM_XEN_SYSTEM_H > > +#define _ASM_ARM_XEN_SYSTEM_H > > + > > +#include <compiler.h> > > +#include <asm/bitops.h> > > + > > +/* If *ptr == old, then store new there (and return new). > > + * Otherwise, return the old value. > > + * Atomic. > > + */ > > +#define synch_cmpxchg(ptr, old, new) \ > > +({ __typeof__(*ptr) stored = old; \ > > + __atomic_compare_exchange_n(ptr, &stored, new, 0, > > __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST) ? new : old; \ > > +}) > > + > > +/* As test_and_clear_bit, but using __ATOMIC_SEQ_CST */ > > +static inline int synch_test_and_clear_bit(int nr, volatile void > > *addr) > > +{ > > + u8 *byte = ((u8 *)addr) + (nr >> 3); > > + u8 bit = 1 << (nr & 7); > > + u8 orig; > > + > > + orig = __atomic_fetch_and(byte, ~bit, __ATOMIC_SEQ_CST); > > + > > + return (orig & bit) != 0; > > +} > > + > > +/* As test_and_set_bit, but using __ATOMIC_SEQ_CST */ > > +static inline int synch_test_and_set_bit(int nr, volatile void > > *base) > > +{ > > + u8 *byte = ((u8 *)base) + (nr >> 3); > > + u8 bit = 1 << (nr & 7); > > + u8 orig; > > + > > + orig = __atomic_fetch_or(byte, bit, __ATOMIC_SEQ_CST); > > + > > + return (orig & bit) != 0; > > +} > > + > > +/* As set_bit, but using __ATOMIC_SEQ_CST */ > > +static inline void synch_set_bit(int nr, volatile void *addr) > > +{ > > + synch_test_and_set_bit(nr, addr); > > +} > > + > > +/* As clear_bit, but using __ATOMIC_SEQ_CST */ > > +static inline void synch_clear_bit(int nr, volatile void *addr) > > +{ > > + synch_test_and_clear_bit(nr, addr); > > +} > > + > > +/* As test_bit, but with a following memory barrier. */ > > +//static inline int synch_test_bit(int nr, volatile void *addr) > > +static inline int synch_test_bit(int nr, const void *addr) > > +{ > > + int result; > > + > > + result = test_bit(nr, addr); > > + barrier(); > > + return result; > > +} > > I can understand why we implement sync_* helpers as AFAICT the > generic > helpers are not SMP safe. However... > > > + > > +#define xchg(ptr, v) __atomic_exchange_n(ptr, v, > > __ATOMIC_SEQ_CST) > > +#define xchg(ptr, v) __atomic_exchange_n(ptr, v, > > __ATOMIC_SEQ_CST) > > + > > +#define mb() dsb() > > +#define rmb() dsb() > > +#define wmb() dsb() > > +#define __iormb() dmb() > > +#define __iowmb() dmb() > > Why do you need to re-implement the barriers? Indeed, we do not need to do this. I will fix it in the next version. > > > +#define xen_mb() mb() > > +#define xen_rmb() rmb() > > +#define xen_wmb() wmb() > > + > > +#define smp_processor_id() 0 > > Shouldn't this be common? Currently it is only used by Xen and we are not sure if any other entity will use it, but we can put that into arch/arm/include/asm/io.h > > > + > > +#define to_phys(x) ((unsigned long)(x)) > > +#define to_virt(x) ((void *)(x)) > > + > > +#define PFN_UP(x) (unsigned long)(((x) + PAGE_SIZE - 1) > > >> PAGE_SHIFT) > > +#define PFN_DOWN(x) (unsigned long)((x) >> > > PAGE_SHIFT) > > +#define PFN_PHYS(x) ((unsigned long)(x) << > > PAGE_SHIFT) > > +#define PHYS_PFN(x) (unsigned long)((x) >> > > PAGE_SHIFT) > > + > > +#define virt_to_pfn(_virt) (PFN_DOWN(to_phys(_virt))) > > +#define virt_to_mfn(_virt) (PFN_DOWN(to_phys(_virt))) > > +#define mfn_to_virt(_mfn) (to_virt(PFN_PHYS(_mfn))) > > +#define pfn_to_virt(_pfn) (to_virt(PFN_PHYS(_pfn))) > > There is already generic phys <-> virt helpers (see > include/asm-generic/io.h). So why do you need to create a new > version? Indeed, we do not need to do this. I will fix it in the next version. > > > + > > +#endif > > diff --git a/common/board_r.c b/common/board_r.c > > index fa57fa9b69..fd36edb4e5 100644 > > --- a/common/board_r.c > > +++ b/common/board_r.c > > @@ -56,6 +56,7 @@ > > #include <timer.h> > > #include <trace.h> > > #include <watchdog.h> > > +#include <xen.h> > > Do we want to include it for other boards? For now, we do not have a plan and resources to support anything other than what we need. Therefore only ARM64. > > > #ifdef CONFIG_ADDR_MAP > > #include <asm/mmu.h> > > #endif > > @@ -462,6 +463,13 @@ static int initr_mmc(void) > > } > > #endif > > > > +#ifdef CONFIG_XEN > > +static int initr_xen(void) > > +{ > > + xen_init(); > > + return 0; > > +} > > +#endif > > /* > > * Tell if it's OK to load the environment early in boot. > > * > > @@ -769,6 +777,9 @@ static init_fnc_t init_sequence_r[] = { > > #endif > > #ifdef CONFIG_MMC > > initr_mmc, > > +#endif > > +#ifdef CONFIG_XEN > > + initr_xen, > > #endif > > initr_env, > > #ifdef CONFIG_SYS_BOOTPARAMS_LEN > > diff --git a/drivers/Makefile b/drivers/Makefile > > index 94e8c5da17..0dd8891e76 100644 > > --- a/drivers/Makefile > > +++ b/drivers/Makefile > > @@ -28,6 +28,7 @@ obj-$(CONFIG_$(SPL_)REMOTEPROC) += remoteproc/ > > obj-$(CONFIG_$(SPL_TPL_)TPM) += tpm/ > > obj-$(CONFIG_$(SPL_TPL_)ACPI_PMC) += power/acpi_pmc/ > > obj-$(CONFIG_$(SPL_)BOARD) += board/ > > +obj-$(CONFIG_XEN) += xen/ > > > > ifndef CONFIG_TPL_BUILD > > ifdef CONFIG_SPL_BUILD > > diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile > > new file mode 100644 > > index 0000000000..1211bf2386 > > --- /dev/null > > +++ b/drivers/xen/Makefile > > @@ -0,0 +1,5 @@ > > +# SPDX-License-Identifier: GPL-2.0+ > > +# > > +# (C) Copyright 2020 EPAM Systems Inc. > > + > > +obj-y += hypervisor.o > > diff --git a/drivers/xen/hypervisor.c b/drivers/xen/hypervisor.c > > new file mode 100644 > > index 0000000000..5883285142 > > --- /dev/null > > +++ b/drivers/xen/hypervisor.c > > @@ -0,0 +1,277 @@ > > +/***************************************************************** > > ************* > > + * hypervisor.c > > + * > > + * Communication to/from hypervisor. > > + * > > + * Copyright (c) 2002-2003, K A Fraser > > + * Copyright (c) 2005, Grzegorz Milos, gm...@cam.ac.uk,Intel > > Research Cambridge > > + * Copyright (c) 2020, EPAM Systems Inc. > > + * > > + * Permission is hereby granted, free of charge, to any person > > obtaining a copy > > + * of this software and associated documentation files (the > > "Software"), to > > + * deal in the Software without restriction, including without > > limitation the > > + * rights to use, copy, modify, merge, publish, distribute, > > sublicense, and/or > > + * sell copies of the Software, and to permit persons to whom the > > Software is > > + * furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice shall be > > included in > > + * all copies or substantial portions of the Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > > EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > > MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO > > EVENT SHALL THE > > + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES > > OR OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > > ARISING > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR > > OTHER > > + * DEALINGS IN THE SOFTWARE. > > + */ > > +#include <common.h> > > +#include <cpu_func.h> > > +#include <log.h> > > +#include <memalign.h> > > + > > +#include <asm/io.h> > > +#include <asm/armv8/mmu.h> > > +#include <asm/xen/system.h> > > + > > +#include <linux/bug.h> > > + > > +#include <xen/hvm.h> > > +#include <xen/interface/memory.h> > > + > > +#define active_evtchns(cpu, sh, idx) \ > > + ((sh)->evtchn_pending[idx] & \ > > + ~(sh)->evtchn_mask[idx]) > > + > > +int in_callback; > > + > > +/* > > + * Shared page for communicating with the hypervisor. > > + * Events flags go here, for example. > > + */ > > +struct shared_info *HYPERVISOR_shared_info; > > + > > +#ifndef CONFIG_PARAVIRT > > Is there any plan to support this on x86? For now, we do not have a plan and resources to support anything other than what we need. Therefore only ARM64. > > > +static const char *param_name(int op) > > +{ > > +#define PARAM(x)[HVM_PARAM_##x] = #x > > + static const char *const names[] = { > > + PARAM(CALLBACK_IRQ), > > + PARAM(STORE_PFN), > > + PARAM(STORE_EVTCHN), > > + PARAM(PAE_ENABLED), > > + PARAM(IOREQ_PFN), > > + PARAM(TIMER_MODE), > > + PARAM(HPET_ENABLED), > > + PARAM(IDENT_PT), > > + PARAM(ACPI_S_STATE), > > + PARAM(VM86_TSS), > > + PARAM(VPT_ALIGN), > > + PARAM(CONSOLE_PFN), > > + PARAM(CONSOLE_EVTCHN), > > Most of those parameters are never going to be used on Arm. So could > this be clobberred? > > > + }; > > +#undef PARAM > > + > > + if (op >= ARRAY_SIZE(names)) > > + return "unknown"; > > + > > + if (!names[op]) > > + return "reserved"; > > + > > + return names[op]; > > +} > > + > > +int hvm_get_parameter_maintain_dcache(int idx, uint64_t *value) > > I would recommend to add some comments explaining when this function > is > meant to be used and what it is doing in regards of the cache. Thank you for recommendation. I will add comments about this function in the next version. > > > +{ > > + struct xen_hvm_param xhv; > > + int ret; > > I don't think there is a guarantee that your cache is going to be > clean > when writing xhv. So you likely want to add a > invalidate_dcache_range() > before writing it. Thank you for advice. Ah, so we need something like: ... invalidate_dcache_range((unsigned long)&xhv, (unsigned long)&xhv + sizeof(xhv)); xhv.domid = DOMID_SELF; xhv.index = idx; invalidate_dcache_range((unsigned long)&xhv, (unsigned long)&xhv + sizeof(xhv)); ... > > > + > > + xhv.domid = DOMID_SELF; > > + xhv.index = idx; > > + invalidate_dcache_range((unsigned long)&xhv, > > + (unsigned long)&xhv + sizeof(xhv)); > > + > > + ret = HYPERVISOR_hvm_op(HVMOP_get_param, &xhv); > > + if (ret < 0) { > > + pr_err("Cannot get hvm parameter %s (%d): %d!\n", > > + param_name(idx), idx, ret); > > + BUG(); > > + } > > + invalidate_dcache_range((unsigned long)&xhv, > > + (unsigned long)&xhv + sizeof(xhv)); > > + > > + *value = xhv.value; > > + return ret; > > +} > > + > > +int hvm_get_parameter(int idx, uint64_t *value) > > +{ > > + struct xen_hvm_param xhv; > > + int ret; > > + > > + xhv.domid = DOMID_SELF; > > + xhv.index = idx; > > + ret = HYPERVISOR_hvm_op(HVMOP_get_param, &xhv); > > + if (ret < 0) { > > + pr_err("Cannot get hvm parameter %s (%d): %d!\n", > > + param_name(idx), idx, ret); > > + BUG(); > > + } > > + > > + *value = xhv.value; > > + return ret; > > +} > > + > > +int hvm_set_parameter(int idx, uint64_t value) > > +{ > > + struct xen_hvm_param xhv; > > + int ret; > > + > > + xhv.domid = DOMID_SELF; > > + xhv.index = idx; > > + xhv.value = value; > > + ret = HYPERVISOR_hvm_op(HVMOP_set_param, &xhv); > > + > > + if (ret < 0) { > > + pr_err("Cannot get hvm parameter %s (%d): %d!\n", > > + param_name(idx), idx, ret); > > + BUG(); > > + } > > + > > + return ret; > > +} > > + > > +struct shared_info *map_shared_info(void *p) > > +{ > > + struct xen_add_to_physmap xatp; > > + > > + HYPERVISOR_shared_info = (struct shared_info > > *)memalign(PAGE_SIZE, > > + PAGE_SI > > ZE); > > + if (HYPERVISOR_shared_info == NULL) > > + BUG(); > > + > > + xatp.domid = DOMID_SELF; > > + xatp.idx = 0; > > + xatp.space = XENMAPSPACE_shared_info; > > + xatp.gpfn = virt_to_pfn(HYPERVISOR_shared_info); > > + if (HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp) != 0) > > + BUG(); > > + > > + return HYPERVISOR_shared_info; > > +} > > + > > +void unmap_shared_info(void) > > +{ > > + struct xen_remove_from_physmap xrtp; > > + > > + xrtp.domid = DOMID_SELF; > > + xrtp.gpfn = virt_to_pfn(HYPERVISOR_shared_info); > > + if (HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrtp) != > > 0) > > + BUG(); > > +} > > +#endif > > + > > +void do_hypervisor_callback(struct pt_regs *regs) > > +{ > > + unsigned long l1, l2, l1i, l2i; > > + unsigned int port; > > + int cpu = 0; > > + struct shared_info *s = HYPERVISOR_shared_info; > > + struct vcpu_info *vcpu_info = &s->vcpu_info[cpu]; > > + > > + in_callback = 1; > > + > > + vcpu_info->evtchn_upcall_pending = 0; > > + /* NB x86. No need for a barrier here -- XCHG is a barrier on > > x86. */ > > +#if !defined(__i386__) && !defined(__x86_64__) > > + /* Clear master flag /before/ clearing selector flag. */ > > + wmb(); > > +#endif > > + l1 = xchg(&vcpu_info->evtchn_pending_sel, 0); > > + > > + while (l1 != 0) { > > + l1i = __ffs(l1); > > + l1 &= ~(1UL << l1i); > > + > > + while ((l2 = active_evtchns(cpu, s, l1i)) != 0) { > > + l2i = __ffs(l2); > > + l2 &= ~(1UL << l2i); > > + > > + port = (l1i * (sizeof(unsigned long) * 8)) + > > l2i; > > + /* TODO: handle new event: do_event(port, > > regs); */ > > + /* Suppress -Wunused-but-set-variable */ > > + (void)(port); > > + } > > + } > > You likely want a memory barrier here as otherwise in_callback could > be > written/seen before the loop end. > We are not running in a multi-threaded environment, so probably in_callback should be fine as is? Or it can be removed completely as there are no currently users of it. > > + > > + in_callback = 0; > > +} > > + > > +void force_evtchn_callback(void) > > +{ > > +#ifdef XEN_HAVE_PV_UPCALL_MASK > > + int save; > > +#endif > > + struct vcpu_info *vcpu; > > + > > + vcpu = &HYPERVISOR_shared_info->vcpu_info[smp_processor_id()]; > > On Arm, this is only valid for vCPU0. For all the other vCPUs, you > will > want to register a vCPU shared info. > According to Mini-OS this is also expected for x86 [1] as both ARM and x86 are defining smp_processor_id as 0. Do you expect any issue with that? [1] http://xenbits.xenproject.org/gitweb/?p=mini-os.git;a=blob;f=include/x86/os.h;h=a73b63e5e4e0f4b7fa7ca944739f2c3b8a956833;hb=HEAD#l10 > > +#ifdef XEN_HAVE_PV_UPCALL_MASK > > + save = vcpu->evtchn_upcall_mask; > > +#endif > > + > > + while (vcpu->evtchn_upcall_pending) { > > +#ifdef XEN_HAVE_PV_UPCALL_MASK > > + vcpu->evtchn_upcall_mask = 1; > > +#endif > > + barrier(); > > What are you trying to prevent with this barrier? In particular why > would the compiler be an issue but not the processor? This is the original code from Mini-OS and it seems that the barriers are leftovers from some old code. We do not define XEN_HAVE_PV_UPCALL_MASK, so this function can be stripped a lot with barriers removed completely. > > > + do_hypervisor_callback(NULL); > > + barrier(); > > +#ifdef XEN_HAVE_PV_UPCALL_MASK > > + vcpu->evtchn_upcall_mask = save; > > + barrier(); > > Same here. Same as above. > > > +#endif > > + }; > > +} > > + > > +void mask_evtchn(uint32_t port) > > +{ > > + struct shared_info *s = HYPERVISOR_shared_info; > > + synch_set_bit(port, &s->evtchn_mask[0]); > > +} > > + > > +void unmask_evtchn(uint32_t port) > > +{ > > + struct shared_info *s = HYPERVISOR_shared_info; > > + struct vcpu_info *vcpu_info = &s- > > >vcpu_info[smp_processor_id()]; > > + > > + synch_clear_bit(port, &s->evtchn_mask[0]); > > + > > + /* > > + * The following is basically the equivalent of > > 'hw_resend_irq'. Just like > > + * a real IO-APIC we 'lose the interrupt edge' if the channel > > is masked. > > + */ > > This seems to be out-of-context now, you might want to update it. I am not sure I understand it right. Could you please clarify what do you mean under the word "update"? > > > + if (synch_test_bit(port, &s->evtchn_pending[0]) && > > + !synch_test_and_set_bit(port / (sizeof(unsigned long) * 8), > > + &vcpu_info->evtchn_pending_sel)) { > > + vcpu_info->evtchn_upcall_pending = 1; > > +#ifdef XEN_HAVE_PV_UPCALL_MASK > > + if (!vcpu_info->evtchn_upcall_mask) > > +#endif > > + force_evtchn_callback(); > > + } > > +} > > + > > +void clear_evtchn(uint32_t port) > > +{ > > + struct shared_info *s = HYPERVISOR_shared_info; > > + > > + synch_clear_bit(port, &s->evtchn_pending[0]); > > +} > > + > > +void xen_init(void) > > +{ > > + debug("%s\n", __func__); > > Is this a left-over? I think this is a relevant comment for debug purpose. But we do not mind removing it, if it seems superfluous. > > > + > > + map_shared_info(NULL); > > +} > > + > > diff --git a/include/xen.h b/include/xen.h > > new file mode 100644 > > index 0000000000..1d6f74cc92 > > --- /dev/null > > +++ b/include/xen.h > > @@ -0,0 +1,11 @@ > > +/* > > + * SPDX-License-Identifier: GPL-2.0 > > + * > > + * (C) 2020, EPAM Systems Inc. > > + */ > > +#ifndef __XEN_H__ > > +#define __XEN_H__ > > + > > +void xen_init(void); > > + > > +#endif /* __XEN_H__ */ > > diff --git a/include/xen/hvm.h b/include/xen/hvm.h > > new file mode 100644 > > index 0000000000..89de9625ca > > --- /dev/null > > +++ b/include/xen/hvm.h > > @@ -0,0 +1,30 @@ > > +/* > > + * SPDX-License-Identifier: GPL-2.0 > > + * > > + * Simple wrappers around HVM functions > > + * > > + * Copyright (c) 2002-2003, K A Fraser > > + * Copyright (c) 2005, Grzegorz Milos, gm...@cam.ac.uk,Intel > > Research Cambridge > > + * Copyright (c) 2020, EPAM Systems Inc. > > + */ > > +#ifndef XEN_HVM_H__ > > +#define XEN_HVM_H__ > > + > > +#include <asm/xen/hypercall.h> > > +#include <xen/interface/hvm/params.h> > > +#include <xen/interface/xen.h> > > + > > +extern struct shared_info *HYPERVISOR_shared_info; > > + > > +int hvm_get_parameter(int idx, uint64_t *value); > > +int hvm_get_parameter_maintain_dcache(int idx, uint64_t *value); > > +int hvm_set_parameter(int idx, uint64_t value); > > + > > +struct shared_info *map_shared_info(void *p); > > +void unmap_shared_info(void); > > +void do_hypervisor_callback(struct pt_regs *regs); > > +void mask_evtchn(uint32_t port); > > +void unmask_evtchn(uint32_t port); > > +void clear_evtchn(uint32_t port); > > + > > +#endif /* XEN_HVM_H__ */ > > Cheers, > > Regards, Anastasiia