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 X-Spam-Level: X-Spam-Status: No, score=-14.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2EC85C10F14 for ; Wed, 2 Oct 2019 18:54:41 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0854021A4C for ; Wed, 2 Oct 2019 18:54:41 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="Aa7u5mRO" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729086AbfJBSyk (ORCPT ); Wed, 2 Oct 2019 14:54:40 -0400 Received: from mail-io1-f68.google.com ([209.85.166.68]:38470 "EHLO mail-io1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728946AbfJBSyj (ORCPT ); Wed, 2 Oct 2019 14:54:39 -0400 Received: by mail-io1-f68.google.com with SMTP id u8so59397968iom.5 for ; Wed, 02 Oct 2019 11:54:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=3r5iSDLGZ8r+gXNiaBNLJJqk1fe5WwzEJBwQCe2vUck=; b=Aa7u5mROY4zwTVOvdlxTPP3QwR2ijNoJH5ND5RovBs+s2OzhvsmGt5KxyaUEjSdHSC W9YvwlknPtpfuQ9B3bGNnXbVYl9sWxZh15Pa6dKQgVm5v41CnC/ltU93piXMvDWVQvrP OPvRvmIGu+dHYh3aISWuf/zKrN5ggBkAseDdEy3JYg/aAydf46hqIUovk9TnLDZvKNcA a+uXvcKAjOAiiDPPeBmh3y5v3lyT5JgZc3qUssO/YMkMCcjYuuWWleNiiTWVEJlhQai3 PUp++kJyAKCOMYL9mCsxNXxgtG3Vn2Hs17Gby/M5oO6ArPZr1WlkjH9HQ5PA/DTAHFzr 6gug== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=3r5iSDLGZ8r+gXNiaBNLJJqk1fe5WwzEJBwQCe2vUck=; b=c50m3S5dbCbA4jjKbuzC9SxW276sRzuF+K1n25FiieBn2AxcLRiIb2y2xB5ozPeYFy Yh3UxNCGNXNovGXJ2h1Bp8hpX5H3WYBgyIar/aZS9BmJHMM90u4AO4MA8op6AJ211+JS oUbPo5Azhn5obYoLVtAjp7Mel5lizQzjizzScJWjzpUhQNPI8NlMWDaS6b+PTh6jXehQ cWEhzpQQ5VyjgGz3xh+AVNQSJSGUsYSOGpHzJIh1tk/RH0RHQ5WfBnPSZ2p3yAz7UWB2 Fnt2MDyLB3f93MhzvjjqPQHcAdLfEWGkY4GJ+yjnvZTSpUGFZqeYzgagnmW1TpemL/yw Iz0A== X-Gm-Message-State: APjAAAX6K68+rPf9qjAcSXHZXnIn3LfXlB2L2ImCzqQV9BBjrNa2rY/M I0EWpxWh4aX/o4V3PD8fQz2xpmCl+cRfEWl+IlxjzA== X-Google-Smtp-Source: APXvYqxGU7SB9vUY0cnrSpSxwIk1DQWFrlBUnyKTd4pXrWzuO00cefZQd4xbUvKX+JT3ealXOPiNXIb4qxPV2/+Q7mM= X-Received: by 2002:a92:8e4f:: with SMTP id k15mr6063919ilh.108.1570042477811; Wed, 02 Oct 2019 11:54:37 -0700 (PDT) MIME-Version: 1.0 References: <20190927021927.23057-1-weijiang.yang@intel.com> <20190927021927.23057-5-weijiang.yang@intel.com> In-Reply-To: <20190927021927.23057-5-weijiang.yang@intel.com> From: Jim Mattson Date: Wed, 2 Oct 2019 11:54:26 -0700 Message-ID: Subject: Re: [PATCH v7 4/7] KVM: VMX: Load Guest CET via VMCS when CET is enabled in Guest To: Yang Weijiang Cc: kvm list , LKML , Paolo Bonzini , Sean Christopherson , "Michael S. Tsirkin" , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Sep 26, 2019 at 7:17 PM Yang Weijiang wrote: > > "Load Guest CET state" bit controls whether Guest CET states > will be loaded at Guest entry. Before doing that, KVM needs > to check if CPU CET feature is enabled on host and available > to Guest. > > Note: SHSTK and IBT features share one control MSR: > MSR_IA32_{U,S}_CET, which means it's difficult to hide > one feature from another in the case of SHSTK != IBT, > after discussed in community, it's agreed to allow Guest > control two features independently as it won't introduce > security hole. > > Co-developed-by: Zhang Yi Z > Signed-off-by: Zhang Yi Z > Signed-off-by: Yang Weijiang > --- > arch/x86/kvm/vmx/vmx.c | 35 +++++++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index f720baa7a9ba..ba1a83d11e69 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -44,6 +44,7 @@ > #include > #include > #include > +#include > > #include "capabilities.h" > #include "cpuid.h" > @@ -2918,6 +2919,37 @@ void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > vmcs_writel(GUEST_CR3, guest_cr3); > } > > +static int set_cet_bit(struct kvm_vcpu *vcpu, unsigned long cr4) Nit: This function does not appear to set CR4.CET, as the name would imply. > +{ > + struct vcpu_vmx *vmx = to_vmx(vcpu); > + const u64 cet_bits = XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL; > + bool cet_xss = vmx_xsaves_supported() && > + (kvm_supported_xss() & cet_bits); > + bool cet_cpuid = guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) || > + guest_cpuid_has(vcpu, X86_FEATURE_IBT); > + bool cet_on = !!(cr4 & X86_CR4_CET); > + > + if (cet_on && vmx->nested.vmxon) > + return 1; This constraint doesn't appear to be architected. Also, this prevents enabling CR4.CET when in VMX operation, but what about the other way around (i.e. entering VMX operation with CR4.CET enabled)? > + if (cet_on && !cpu_x86_cet_enabled()) > + return 1; This seems odd. Why is kernel support for (SHSTK or IBT) required for the guest to use (SHSTK or IBT)? If there's a constraint, shouldn't it be 1:1? (i.e. kernel support for SHSTK is required for the guest to use SHSTK and kernel support for IBT is required for the guest to use IBT?) Either way, enforcement of this constraint seems late, here, when the guest is trying to set CR4 to a value that, per the guest CPUID information, should be legal. Shouldn't this constraint be applied when setting the guest CPUID information, disallowing the enumeration of SHSTK and/or IBT support on a platform where these features are unavailable or disabled in the kernel? > + if (cet_on && !cet_xss) > + return 1; Again, this constraint seems like it's being applied too late. Returning 1 here will result in the guest's MOV-to-CR4 raising #GP, even though there is no architected reason for it to do so. > + if (cet_on && !cet_cpuid) > + return 1; What about the constraint that CR4.CET can't be set when CR0.WP is clear? (And the reverse needs to be handled in vmx_set_cr0). > + if (cet_on) > + vmcs_set_bits(VM_ENTRY_CONTROLS, > + VM_ENTRY_LOAD_GUEST_CET_STATE); Have we ensured that this VM-entry control is supported on the platform? > + else > + vmcs_clear_bits(VM_ENTRY_CONTROLS, > + VM_ENTRY_LOAD_GUEST_CET_STATE); > + return 0; > +} > + > int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) > { > struct vcpu_vmx *vmx = to_vmx(vcpu); > @@ -2958,6 +2990,9 @@ int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) > return 1; > } > > + if (set_cet_bit(vcpu, cr4)) > + return 1; > + > if (vmx->nested.vmxon && !nested_cr4_valid(vcpu, cr4)) > return 1; > > -- > 2.17.2 >