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

[Julien Grall]

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://github.com/zyzii/mini-os.git

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.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.

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.

> 
>>
>>> +	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,

-- 
Julien Grall