Re: [PATCH v2] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
From: Jane Malalane
Date: Wed Jul 27 2022 - 11:43:38 EST
On 27/07/2022 13:32, Julien Grall wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open
> attachments unless you have verified the sender and know the content is
> safe.
>
> 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>
> | ^~~~~~~~~~~~~~~~~
Thanks, will place it inside the #ifdef CONFIG_X86.
>
>> #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.
I can surround those 4 callback/upcall functions with an ##ifdef.>>
+ 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.
In Xen it is surrounded with an #ifdef. I am happy to do so here too,
unless there is any opposition.
Thank you,
Jane.