[PATCH 05/17] xen: Port Xen hypervizor related code from mini-os

[Anastasiia Lukianenko]

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 at 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_29dbcQIUBPA!i0hVwJuV0iEI89D83SJP8zr1mgHfh5o3IS2vytGwgxyJ0kzSiCLqVdtA3crvFm0GUMTNGQU$
> >  
> 
> 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.git;a=blob;f=include*x86*os.h;h=a73b63e5e4e0f4b7fa7ca944739f2c3b8a956833;hb=HEAD*l10__;Ly8j!!GF_29dbcQIUBPA!i0hVwJuV0iEI89D83SJP8zr1mgHfh5o3IS2vytGwgxyJ0kzSiCLqVdtA3crvFm0GI_2BcP0$
> >  
> > 
> > > > +#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
> > > > 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"?
> 
> 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(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,
> 

Regards,
Anastasiia