Hi,
On Fri, 2020-07-03 at 14:38 +0100, Julien Grall wrote:
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.
[1] https://urldefense.com/v3/__https://github.com/zyzii/mini-os.git__;!!GF_29db...
Well, that's not part of the official port. It would have been nice to at least mention that in somewhere in the series.
Sure, will mention.
- 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.
Ok, will drop
[...]
+#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.
Ok, will remove
+{
- 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
Ok, will remove.
- 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]
https://urldefense.com/v3/__http://xenbits.xenproject.org/gitweb/?p=mini-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 thechannel 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"?
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.
Thank you for clarification. Ok, will update.
- if (synch_test_bit(port, &s->evtchn_pending[0]) &&
!synch_test_and_set_bit(port / (sizeof(unsignedlong) * 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,
Regards, Anastasiia