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

[Jane Malalane <Jane.Malalane@citrix.com>]

On 25/07/2022 21:46, Boris Ostrovsky wrote:
> 
> On 7/25/22 6:03 AM, Jane Malalane wrote:
>> On 18/07/2022 14:59, Boris Ostrovsky wrote:
>>> On 7/18/22 4:56 AM, Andrew Cooper wrote:
>>>> On 15/07/2022 14:10, Boris Ostrovsky wrote:
>>>>> On 7/15/22 5:50 AM, Andrew Cooper wrote:
>>>>>> On 15/07/2022 09:18, Jane Malalane wrote:
>>>>>>> On 14/07/2022 00:27, Boris Ostrovsky wrote:
>>>>>>>>>          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.
>>>>>> And others.
>>>>>>
>>>>>> This is all a giant bodge, but basically a lot of tooling uses the
>>>>>> non-zero-ness of the CALLBACK_VIA param to determine whether the 
>>>>>> VM has
>>>>>> Xen-aware drivers loaded or not.
>>>>>>
>>>>>> The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
>>>>>> reason this doesn't explode everywhere is because the
>>>>>> evtchn_upcall_vector registration takes priority over GSI delivery.
>>>>>>
>>>>>> This is decades of tech debt piled on top of tech debt.
>>>>> Feels like it (setting the callback parameter) is something that the
>>>>> hypervisor should do --- no need to expose guests to this.
>>>> Sensible or not, it is the ABI.
>>>>
>>>> Linux still needs to work (nicely) with older Xen's in the world, 
>>>> and we
>>>> can't just retrofit a change in the hypervisor which says "btw, this 
>>>> ABI
>>>> we've just changed now has a side effect of modifying a field that you
>>>> also logically own".
>>>
>>> The hypercall has been around for a while so I understand ABI concerns
>>> there but XEN_HVM_CPUID_UPCALL_VECTOR was introduced only a month ago.
>>> Why not tie presence of this bit to no longer having to explicitly set
>>> the callback field?
>>>
>> Any other opinions on this?
>>
>> (i.e., calling xen_set_callback_via(1) after
>> HVMOP_set_evtchn_upcall_vector OR not exposing this to guests and
>> instead having Xen call this function (in hvmop_set_evtchn_upcall_vector
>> maybe) and tieing its presense to XEN_HVM_CPUID_UPCALL_VECTOR which was
>> recently added)
> 
> 
> CPUID won't help here, I wasn't thinking clearly.
> 
> 
> Can we wrap the HVMOP_set_evtchn_upcall_vector hypercall in a function 
> that will decide whether or not to also do xen_set_callback_via(1)?
> 
Okay. Will do this in a v2.

Thanks,

Jane.