Re: [PATCH v2] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector

[Julien Grall]

Hi Jane,

On 26/07/2022 13:56, Jane Malalane wrote:
> diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
> index 9d548b0c772f..0c4f7554b7cc 100644
> --- a/arch/x86/xen/suspend_hvm.c
> +++ b/arch/x86/xen/suspend_hvm.c
> @@ -5,6 +5,7 @@
>   #include <xen/hvm.h>
>   #include <xen/features.h>
>   #include <xen/interface/features.h>
> +#include <xen/events.h>
>   
>   #include "xen-ops.h"
>   
> @@ -14,6 +15,13 @@ void xen_hvm_post_suspend(int suspend_cancelled)
>   		xen_hvm_init_shared_info();
>   		xen_vcpu_restore();
>   	}
> -	xen_setup_callback_vector();
> +	if (xen_percpu_upcall) {
> +		unsigned int cpu;
> +
> +		for_each_online_cpu(cpu)
> +			BUG_ON(xen_set_upcall_vector(cpu));
> +	} else {
> +		xen_setup_callback_vector();
> +	}
>   	xen_unplug_emulated_devices();
>   }
> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> index 46d9295d9a6e..2ad93595d03a 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -48,6 +48,7 @@
>   #include <asm/xen/pci.h>
>   #endif
>   #include <asm/sync_bitops.h>
> +#include <asm/xen/cpuid.h>

This include doesn't exist on Arm and will result to a build failure:

linux/drivers/xen/events/events_base.c:51:10: fatal error: 
asm/xen/cpuid.h: No such file or directory
   51 | #include <asm/xen/cpuid.h>
      |          ^~~~~~~~~~~~~~~~~


>   #include <asm/xen/hypercall.h>
>   #include <asm/xen/hypervisor.h>
>   #include <xen/page.h>
> @@ -2195,11 +2196,48 @@ void xen_setup_callback_vector(void)
>   		callback_via = HVM_CALLBACK_VECTOR(HYPERVISOR_CALLBACK_VECTOR);
>   		if (xen_set_callback_via(callback_via)) {
>   			pr_err("Request for Xen HVM callback vector failed\n");
> -			xen_have_vector_callback = 0;
> +			xen_have_vector_callback = false;
>   		}
>   	}
>   }
>   
> +/* Setup per-vCPU vector-type callbacks and trick toolstack to think
> + * we are enlightened. If this setup is unavailable, fallback to the
> + * global vector-type callback. */
> +static __init void xen_init_setup_upcall_vector(void)
> +{
> +	unsigned int cpu = 0;
> +
> +	if (!xen_have_vector_callback)
> +		return;
> +
> +	if ((cpuid_eax(xen_cpuid_base() + 4) & XEN_HVM_CPUID_UPCALL_VECTOR) &&
> +	    !xen_set_upcall_vector(cpu) && !xen_set_callback_via(1))

xen_cpuid_base() is an x86-ism. This is going to build because 
CONFIG_XEN_PVHVM is only set for x86. However, I think this is quite 
fragile.

You are also using more variable defined only on x86. So it feels to me 
that these functions should be implemented in x86 code.

> +		xen_percpu_upcall = true;
> +	else if (xen_feature(XENFEAT_hvm_callback_vector))
> +		xen_setup_callback_vector();
> +	else
> +		xen_have_vector_callback = false;
> +}
> +
> +int xen_set_upcall_vector(unsigned int cpu)
> +{
> +	int rc;
> +	xen_hvm_evtchn_upcall_vector_t op = {
> +		.vector = HYPERVISOR_CALLBACK_VECTOR,
> +		.vcpu = per_cpu(xen_vcpu_id, cpu),
> +	};
> +
> +	rc = HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector, &op);
> +	if (rc)
> +		return rc;
> +
> +	if (!cpu)
> +		rc = xen_set_callback_via(1);
> +
> +	return rc;
> +}
> +
>   static __init void xen_alloc_callback_vector(void)
>   {
>   	if (!xen_have_vector_callback)
> @@ -2210,6 +2248,8 @@ static __init void xen_alloc_callback_vector(void)
>   }
>   #else
>   void xen_setup_callback_vector(void) {}
> +static inline void xen_init_setup_upcall_vector(void) {}
> +int xen_set_upcall_vector(unsigned int cpu) {}
>   static inline void xen_alloc_callback_vector(void) {}
>   #endif
>   
> @@ -2271,10 +2311,9 @@ void __init xen_init_IRQ(void)
>   		if (xen_initial_domain())
>   			pci_xen_initial_domain();
>   	}
> -	if (xen_feature(XENFEAT_hvm_callback_vector)) {
> -		xen_setup_callback_vector();
> -		xen_alloc_callback_vector();
> -	}
> +	xen_init_setup_upcall_vector();
> +	xen_alloc_callback_vector();
> +
>   
>   	if (xen_hvm_domain()) {
>   		native_init_IRQ();
> diff --git a/include/xen/hvm.h b/include/xen/hvm.h
> index b7fd7fc9ad41..8da7a6747058 100644
> --- a/include/xen/hvm.h
> +++ b/include/xen/hvm.h
> @@ -60,4 +60,6 @@ static inline int hvm_get_parameter(int idx, uint64_t *value)
>   
>   void xen_setup_callback_vector(void);
>   
> +int xen_set_upcall_vector(unsigned int cpu);
> +
>   #endif /* XEN_HVM_H__ */
> diff --git a/include/xen/interface/hvm/hvm_op.h b/include/xen/interface/hvm/hvm_op.h
> index f3097e79bb03..e714d8b6ef89 100644
> --- a/include/xen/interface/hvm/hvm_op.h
> +++ b/include/xen/interface/hvm/hvm_op.h
> @@ -46,4 +46,19 @@ struct xen_hvm_get_mem_type {
>   };
>   DEFINE_GUEST_HANDLE_STRUCT(xen_hvm_get_mem_type);
>   
> +/*
> + * HVMOP_set_evtchn_upcall_vector: Set a <vector> that should be used for event
> + *                                 channel upcalls on the specified <vcpu>. If set,
> + *                                 this vector will be used in preference to the
> + *                                 domain global callback via (see
> + *                                 HVM_PARAM_CALLBACK_IRQ).
> + */

Technically this hypercall is x86 specific. IOW, it would be possible 
for another architecture to define 23 for something different.

If it is not possible (or desired) to surround with an #ifdef, then I 
think we should at least be mentioned it in the comment.

Cheers,

--
Julien Grall