From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id F2731C433EF for ; Thu, 17 Feb 2022 15:07:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242281AbiBQPHj (ORCPT ); Thu, 17 Feb 2022 10:07:39 -0500 Received: from mxb-00190b01.gslb.pphosted.com ([23.128.96.19]:47424 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234684AbiBQPHf (ORCPT ); Thu, 17 Feb 2022 10:07:35 -0500 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 892B725B2D6 for ; Thu, 17 Feb 2022 07:07:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1645110439; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=qFSOR5Tcp5Zr84hEbE+k+4Qy9iD42HSTK7sgtEU/6PM=; b=Os8rOc5eqWVn6A2qtwojoQOFZ+O56RtWNTIx5bU1pz9iQjBQLMx61y09mzC7bIw3ax2ruO tM7jfTsEsX4lE/DpCEm/Y/R2+k8DAJntRn0CkYM9ynsPCwdjqjJTiDqiiw0acyx5ut/VMi lEfIwLcSkzNZXd9DUq9SkgFroDOTK7c= Received: from mail-ed1-f71.google.com (mail-ed1-f71.google.com [209.85.208.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-652-LH346JF0MhChqwqVxc3FGA-1; Thu, 17 Feb 2022 10:07:18 -0500 X-MC-Unique: LH346JF0MhChqwqVxc3FGA-1 Received: by mail-ed1-f71.google.com with SMTP id l3-20020a50cbc3000000b0041083c11173so3704339edi.4 for ; Thu, 17 Feb 2022 07:07:18 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=qFSOR5Tcp5Zr84hEbE+k+4Qy9iD42HSTK7sgtEU/6PM=; b=wqD1JNmb4g1SS5e6lq2pSnRht4c7GoVh+Yt5KDYTdVznKCxj3aSwGIQjvb8f0as/jd zvUMuFIxS/7rpdgMOgf7izhDXIrzR63MCSKc+UIciWKuBfB8fH9fA8sv3JiUfWdNHcxy 1AsGzlMrxfFYrA3Xi4wt4kir56aUbFbkgBA9vt2M7qmYm65dH3d8+9m+GqPCOl17kdRb kjzfuiLHT0Yn5NwOJEOECMVUmKO1wbF5KXdl53W4+TqEobe1AvLrbMdMvnONQDuwYAFV DGYWWE7nXjaJCSJcatpzpr89O8JK5DMYzMawCC0XZUnikuXMPiENS3dqFsjEpnAUDj8o Efgg== X-Gm-Message-State: AOAM531gWPoFR1cfMPV9Z9SAOHLxpaK/gpMFiuXH+HSK08E+ObWWB/uF Y5OtgOTALsu9f/cQpxU10g8X30PS6Le15xXxSPA+GpctK+jtNE96GTa/WGF4visPpYtryExf7Jm BhWzhXSF6ZHdnQgmPJu0DfkSK X-Received: by 2002:a17:906:3104:b0:6ce:6b85:ecc9 with SMTP id 4-20020a170906310400b006ce6b85ecc9mr2636476ejx.339.1645110437081; Thu, 17 Feb 2022 07:07:17 -0800 (PST) X-Google-Smtp-Source: ABdhPJzANKo7hcVxixfL7sOwJBypOhLvl1k3r71hqgukS7AJAyKh1nYcS5xz9etrTRJFPRrNrgRSxA== X-Received: by 2002:a17:906:3104:b0:6ce:6b85:ecc9 with SMTP id 4-20020a170906310400b006ce6b85ecc9mr2636451ejx.339.1645110436820; Thu, 17 Feb 2022 07:07:16 -0800 (PST) Received: from ?IPV6:2001:b07:6468:f312:c8dd:75d4:99ab:290a? ([2001:b07:6468:f312:c8dd:75d4:99ab:290a]) by smtp.googlemail.com with ESMTPSA id m4sm1295147ejl.45.2022.02.17.07.07.15 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 17 Feb 2022 07:07:15 -0800 (PST) Message-ID: Date: Thu, 17 Feb 2022 16:07:14 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [PATCH v4 1/2] x86/kvm/fpu: Mask guest fpstate->xfeatures with guest_supported_xcr0 Content-Language: en-US To: David Edmondson , Leonardo Bras Cc: Sean Christopherson , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , x86@kernel.org, "H. Peter Anvin" , "Chang S. Bae" , Andy Lutomirski , David Gilbert , Peter Xu , kvm@vger.kernel.org, linux-kernel@vger.kernel.org References: <20220217053028.96432-1-leobras@redhat.com> <20220217053028.96432-2-leobras@redhat.com> From: Paolo Bonzini In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2/17/22 13:07, David Edmondson wrote: > The single line summary is now out of date - there's no new masking. Thanks for the review, I made the adjustments and pushed to master. Paolo > On Thursday, 2022-02-17 at 02:30:29 -03, Leonardo Bras wrote: > >> During host/guest switch (like in kvm_arch_vcpu_ioctl_run()), the kernel >> swaps the fpu between host/guest contexts, by using fpu_swap_kvm_fpstate(). >> >> When xsave feature is available, the fpu swap is done by: >> - xsave(s) instruction, with guest's fpstate->xfeatures as mask, is used >> to store the current state of the fpu registers to a buffer. >> - xrstor(s) instruction, with (fpu_kernel_cfg.max_features & >> XFEATURE_MASK_FPSTATE) as mask, is used to put the buffer into fpu regs. >> >> For xsave(s) the mask is used to limit what parts of the fpu regs will >> be copied to the buffer. Likewise on xrstor(s), the mask is used to >> limit what parts of the fpu regs will be changed. >> >> The mask for xsave(s), the guest's fpstate->xfeatures, is defined on >> kvm_arch_vcpu_create(), which (in summary) sets it to all features >> supported by the cpu which are enabled on kernel config. >> >> This means that xsave(s) will save to guest buffer all the fpu regs >> contents the cpu has enabled when the guest is paused, even if they >> are not used. >> >> This would not be an issue, if xrstor(s) would also do that. >> >> xrstor(s)'s mask for host/guest swap is basically every valid feature >> contained in kernel config, except XFEATURE_MASK_PKRU. >> Accordingto kernel src, it is instead switched in switch_to() and >> flush_thread(). >> >> Then, the following happens with a host supporting PKRU starts a >> guest that does not support it: >> 1 - Host has XFEATURE_MASK_PKRU set. 1st switch to guest, >> 2 - xsave(s) fpu regs to host fpustate (buffer has XFEATURE_MASK_PKRU) >> 3 - xrstor(s) guest fpustate to fpu regs (fpu regs have XFEATURE_MASK_PKRU) >> 4 - guest runs, then switch back to host, >> 5 - xsave(s) fpu regs to guest fpstate (buffer now have XFEATURE_MASK_PKRU) >> 6 - xrstor(s) host fpstate to fpu regs. >> 7 - kvm_vcpu_ioctl_x86_get_xsave() copy guest fpstate to userspace (with >> XFEATURE_MASK_PKRU, which should not be supported by guest vcpu) >> >> On 5, even though the guest does not support PKRU, it does have the flag >> set on guest fpstate, which is transferred to userspace via vcpu ioctl >> KVM_GET_XSAVE. >> >> This becomes a problem when the user decides on migrating the above guest >> to another machine that does not support PKRU: >> The new host restores guest's fpu regs to as they were before (xrstor(s)), >> but since the new host don't support PKRU, a general-protection exception >> ocurs in xrstor(s) and that crashes the guest. >> >> This can be solved by making the guest's fpstate->user_xfeatures hold >> a copy of guest_supported_xcr0. This way, on 7 the only flags copied to >> userspace will be the ones compatible to guest requirements, and thus >> there will be no issue during migration. >> >> As a bonus, it will also fail if userspace tries to set fpu features >> that are not compatible to the guest configuration. (KVM_SET_XSAVE ioctl) >> >> Also, since kvm_vcpu_after_set_cpuid() now sets fpstate->user_xfeatures, >> there is not need to set it in kvm_check_cpuid(). So, change >> fpstate_realloc() so it does not touch fpstate->user_xfeatures if a >> non-NULL guest_fpu is passed, which is the case when kvm_check_cpuid() >> calls it. >> >> Signed-off-by: Leonardo Bras >> --- >> arch/x86/kernel/fpu/xstate.c | 5 ++++- >> arch/x86/kvm/cpuid.c | 2 ++ >> 2 files changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c >> index 02b3ddaf4f75..7c7824ae7862 100644 >> --- a/arch/x86/kernel/fpu/xstate.c >> +++ b/arch/x86/kernel/fpu/xstate.c >> @@ -1558,7 +1558,10 @@ static int fpstate_realloc(u64 xfeatures, unsigned int ksize, >> fpregs_restore_userregs(); >> >> newfps->xfeatures = curfps->xfeatures | xfeatures; >> - newfps->user_xfeatures = curfps->user_xfeatures | xfeatures; >> + >> + if (!guest_fpu) >> + newfps->user_xfeatures = curfps->user_xfeatures | xfeatures; >> + >> newfps->xfd = curfps->xfd & ~xfeatures; >> >> /* Do the final updates within the locked region */ >> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >> index 494d4d351859..71125291c578 100644 >> --- a/arch/x86/kvm/cpuid.c >> +++ b/arch/x86/kvm/cpuid.c >> @@ -296,6 +296,8 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) >> vcpu->arch.guest_supported_xcr0 = >> cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent); >> >> + vcpu->arch.guest_fpu.fpstate->user_xfeatures = vcpu->arch.guest_supported_xcr0; >> + >> kvm_update_pv_runtime(vcpu); >> >> vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu); > > dme.