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

[Jane Malalane <Jane.Malalane@xxxxxxxxxx>]

On 14/07/2022 00:27, Boris Ostrovsky wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open 
> attachments unless you have verified the sender and know the content is 
> safe.
> 
> On 7/11/22 11:22 AM, Jane Malalane wrote:
>> --- a/arch/x86/xen/enlighten_hvm.c
>> +++ b/arch/x86/xen/enlighten_hvm.c
>> @@ -7,6 +7,7 @@
>>   #include <xen/features.h>
>>   #include <xen/events.h>
>> +#include <xen/interface/hvm/hvm_op.h>
>>   #include <xen/interface/memory.h>
>>   #include <asm/apic.h>
>> @@ -30,6 +31,9 @@
>>   static unsigned long shared_info_pfn;
>> +__ro_after_init bool xen_ack_upcall;
>> +EXPORT_SYMBOL_GPL(xen_ack_upcall);
> 
> 
> Shouldn't this be called something like xen_percpu_upcall?
Sure.
> 
> 
>> +
>>   void xen_hvm_init_shared_info(void)
>>   {
>>       struct xen_add_to_physmap xatp;
>> @@ -125,6 +129,9 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_xen_hvm_callback)
>>   {
>>       struct pt_regs *old_regs = set_irq_regs(regs);
>> +    if (xen_ack_upcall)
>> +        ack_APIC_irq();
>> +
>>       inc_irq_stat(irq_hv_callback_count);
>>       xen_hvm_evtchn_do_upcall();
>> @@ -168,6 +175,15 @@ static int xen_cpu_up_prepare_hvm(unsigned int cpu)
>>       if (!xen_have_vector_callback)
>>           return 0;
>> +    if (xen_ack_upcall) {
>> +        xen_hvm_evtchn_upcall_vector_t op = {
>> +            .vector = HYPERVISOR_CALLBACK_VECTOR,
>> +            .vcpu = per_cpu(xen_vcpu_id, cpu),
>> +        };
>> +
>> +        BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector, &op));
> 
> 
> Does this have to be fatal? Can't we just fail bringing this vcpu up?
Yes, will amend.
> 
> 
>> +    }
>> +
>>       if (xen_feature(XENFEAT_hvm_safe_pvclock))
>>           xen_setup_timer(cpu);
>> @@ -211,8 +227,7 @@ static void __init xen_hvm_guest_init(void)
>>       xen_panic_handler_init();
>> -    if (!no_vector_callback && xen_feature(XENFEAT_hvm_callback_vector))
>> -        xen_have_vector_callback = 1;
>> +    xen_have_vector_callback = !no_vector_callback;
> 
> 
> Can we get rid of one of those two variables then?
I'll remove no_vector_callback for less code changes.
> 
> 
>>       xen_hvm_smp_init();
>>       WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm, xen_cpu_dead_hvm));
>> diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
>> index 9d548b0c772f..be66e027ef28 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,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
>>           xen_hvm_init_shared_info();
>>           xen_vcpu_restore();
>>       }
>> -    xen_setup_callback_vector();
>> +    if (xen_ack_upcall) {
>> +        unsigned int cpu;
>> +
>> +        for_each_online_cpu(cpu) {
>> +            xen_hvm_evtchn_upcall_vector_t op = {
>> +                    .vector = HYPERVISOR_CALLBACK_VECTOR,
>> +                    .vcpu = per_cpu(xen_vcpu_id, cpu),
>> +            };
>> +
>> +            BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
>> +                         &op));
>> +            /* Trick toolstack to think we are enlightened. */
>> +            if (!cpu)
>> +                BUG_ON(xen_set_callback_via(1));
> 
> 
> What are you trying to make the toolstack aware of? That we have *a* 
> callback (either global or percpu)?
Yes, specifically for the check in libxl__domain_pvcontrol_available.
> 
> 
> BTW, you can take it out the loop. And maybe @op definition too, except 
> for .vcpu assignment.
> 
> 
>> +        }
>> +    } else {
>> +        xen_setup_callback_vector();
>> +    }
>>       xen_unplug_emulated_devices();
>>   }
> 
> 
> -boris
> 
> 

Thank you,

Jane.