linux-kernel - Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 25 Jul 2022 10:03:39 +0000
From:   Jane Malalane <Jane.Malalane@...rix.com>
To:     Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Andrew Cooper <Andrew.Cooper3@...rix.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Xen-devel <xen-devel@...ts.xenproject.org>
CC:     Juergen Gross <jgross@...e.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>,
        Maximilian Heyne <mheyne@...zon.de>,
        Jan Beulich <jbeulich@...e.com>,
        Colin Ian King <colin.king@...el.com>
Subject: Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector

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)

Thank you,

Jane.