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

[Jane Malalane <Jane.Malalane@xxxxxxxxxx>]

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.