Messages in this thread | | | Date | Mon, 25 Jul 2022 16:46:53 -0400 | Subject | Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector | From | Boris Ostrovsky <> |
| |
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)?
-boris
| |