Hi,
On 03/07/2020 13:21, Anastasiia Lukianenko wrote:
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_andrushchenko@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.
Well, that's not part of the official port. It would have been nice to at least mention that in somewhere in the series.
- 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
I looked at the usage in Xen and don't really think it would help in any way to get the code SMP ready. Does U-boot will enable Xen features on secondary CPUs? If not, then I would recomment to just drop it.
[...]
+#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.
I think you misunderstood my comment here. The file seems to be common but you include xen.h unconditionnally. Is it really what you want to do?
+/*
- 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.
Ok. I doubt that one will want to use U-boot on PV x86. So I would recommend to drop anything related to CONFIG_PARAVIRT.
+{
- 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)); ...
Right, this would indeed be safer.
[...]
+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?
It really depends on how you plan to use in_callback. If you want to use it in interrupt context to know whether you are dealing with a callback, then you will want a compiler barrier. But...
Or it can be removed completely as there are no currently users of it.
... it would be best to remove if you
- 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?
I am not sure why you are referring to Mini-OS... We are discussing this code in the context of U-boot.
smp_processor_id() leads to think that you want to make your code ready for SMP support. However, on Arm, if smp_processor_id() return another value other than 0 it would be totally broken.
Will you ever need to run this code on other code than CPU0?
[1]
http://xenbits.xenproject.org/gitweb/?p=mini-os.git;a=blob;f=include/x86/os....
+#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.
I don't think I agree with your analysis. vcpu->evtchn_upcall_mask can be modified by the hypervisor, so you want to make sure that vcpu->evtchn_upcall_mask is read *after* we finish to deal with the first round of events. Otherwise you have a risk to delay handling of events.
This likely means a "dmb ishld" + compiler barrier after do_hypercall_callback(). FWIW, in Linux they use virt_rmb().
I think you don't need any barrier before hand thanks to xchg as the atomic built-in should already add a barrier for you (you use __ATOMIC_SEQ_CST). Although, it probably worth to check this is the case.
+#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 channelis 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"?
Well the comment is referring to "hw_resend_irq". I guess this is a function I can't find any code in either Mini-OS and U-boot.
Therefore comment seems to be wrong and needs to be updated.
- 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.
That's fine. I was just asking if it was still worth it.
Cheers,