Received: by 2002:a05:6358:3188:b0:123:57c1:9b43 with SMTP id q8csp1124831rwd; Wed, 31 May 2023 09:45:31 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ7/XcDcz/PjSys6n7mEwOKxHuhzfc+BMP5TtceyyAUcNFa97LF6fdxCgEFx7t67etEr2ZUD X-Received: by 2002:a05:6a20:3d85:b0:101:2160:ff8f with SMTP id s5-20020a056a203d8500b001012160ff8fmr5595363pzi.11.1685551530879; Wed, 31 May 2023 09:45:30 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1685551530; cv=none; d=google.com; s=arc-20160816; b=U6zbYBOD3GJ39m553k3qiq5EiuwQM6dvGZ2suWmms1OJdIL5IdISs2aOUIV4apfBu2 ZGHP8XLY36BEPl9ERo4TamWCp5tpIUPcKK7YfJfVAggAzHbW2MvzdsIPx0s/GG2XUaqu j2o757iJUpLUjAn9LLUx8GOoi25ubvCz9F81wluUztcuYbX23E3QG2gxwGFgNvw+cRPa 9Ev+FEiySeN2HauI3K1O5r29Ubj2CJQpHXsGw6JSsIASqTovJZqPr98Q+1qI7W3qRm1G 9m3P7J1HpqjdOEPRhWdp3V4Q6cE4kaWgnRJDuhM8pcPAAxH3O9hYycONE6BGITcADxAr nHUQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:cc:to:from:subject :message-id:references:mime-version:in-reply-to:date:dkim-signature; bh=4oMPwuBewXzucRhAy2k91jt6FR1Q3W1PQo5GZNd6lqM=; b=EqYxapO3v8VGBJpu9bNBop88+xKgXr61IrhKZbn8aBdA7izJERguxLy5J9HioMZaY2 /Au4ploufIstdujWlzwk0L3OU3Ys8WvXWg3vYnzZULduquwJvMeJi1deGcZ8MLEcDZB4 FjwbMk9jpHGIFzI4TE5yHFemgf25c2i7u4aATEjfxsXs2Y+mQnpSkYYu2ufyuix/TEIt bGUSunnDUKFgMzWrV6NruB48tCcUWWnPxZPlj73LryiUbT7+VMQbzFlAoCz16oQk+KKv pV8YUpwclu7E2Wife7DxEfR12wbntOlsOtSbpm5UHecQySTKRmsvhSiM1UiM4Aacb+6/ Cdkg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20221208 header.b=ryeCZ321; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id x29-20020aa7957d000000b0064d6c74a8bbsi3827281pfq.98.2023.05.31.09.45.15; Wed, 31 May 2023 09:45:30 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20221208 header.b=ryeCZ321; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229623AbjEaQbB (ORCPT + 99 others); Wed, 31 May 2023 12:31:01 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54962 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229573AbjEaQa7 (ORCPT ); Wed, 31 May 2023 12:30:59 -0400 Received: from mail-yw1-x1149.google.com (mail-yw1-x1149.google.com [IPv6:2607:f8b0:4864:20::1149]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 07207E2 for ; Wed, 31 May 2023 09:30:58 -0700 (PDT) Received: by mail-yw1-x1149.google.com with SMTP id 00721157ae682-556011695d1so105876807b3.1 for ; Wed, 31 May 2023 09:30:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1685550657; x=1688142657; h=content-transfer-encoding:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:from:to:cc:subject:date:message-id :reply-to; bh=4oMPwuBewXzucRhAy2k91jt6FR1Q3W1PQo5GZNd6lqM=; b=ryeCZ321lBNpiYoS40S71CMHh8oGv0krJKIZpmIGsRIe+xGkwcAHYnzd9/aT6BnPjG gaiFm7JRSiNpBjcX+wc1xbIiR6Nv5swrk3nawefNF3b/5dbY6OGYqJ+F2BxfxTYUyd6v D6mf8CgTUcPhO15w70n9pgY36xb5L2RSXnfymapJLr3enxB9Cwr7TCYP9QAM3Wx4enBT cmDWCxlbMpEaj0zNJ5C992qVuWpncecoglFbvm+Hj4HDMi8KYlgoqpBM2QKVwxk6ui15 qATljkSrWZcTGytOWYTwM9y7jUIFK9JzCGI4og+UAoZP4VUJmQk25c/1HVxPYepTTTIR FV/w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685550657; x=1688142657; h=content-transfer-encoding:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to; bh=4oMPwuBewXzucRhAy2k91jt6FR1Q3W1PQo5GZNd6lqM=; b=Lml7RvQhcCSSuEHq/D3b7pa+jFY1QsAuizmzKhIHMRQ61H88ip76BddZxn/dFwas8Q OtpqGggMLyOMfRNDLMjP/2auHMJ5MUadKdjHRTWYJ/w4+7U6iCFeWR3QmfD9hfecUZf0 ix23dYgxyflI/OGmS2XIsD3CCiztf1sYKM5TdjvwtXqCDmedNk8H/Ar7HXU2gbU1dqO+ tGnE4/p7w+roubgAt8SMvhne/pTiPFFGV4elDzlo6rR+F30U0DWcLtvgEa8+UWGcZ9to HROXIKbVYKfuOJdGvN7C7Rp6vg4JjYqtWjooXY8z+GOl8I+1hUoQvfH4n9MsvswZqXgr WiGQ== X-Gm-Message-State: AC+VfDxYpD+bY/2TigtH3cDUVOXEzXz59A7eizZUsC9z7ijXPEKvCx50 r2ke3bdwvVWJW+YhJ4MFfdlOxMpMpl0= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a81:b70c:0:b0:561:8e86:9818 with SMTP id v12-20020a81b70c000000b005618e869818mr3513805ywh.7.1685550657211; Wed, 31 May 2023 09:30:57 -0700 (PDT) Date: Wed, 31 May 2023 09:30:55 -0700 In-Reply-To: Mime-Version: 1.0 References: <20230530200152.18961-1-jon@nutanix.com> <2a6502e3-ba87-0355-af09-825e8467b81f@intel.com> Message-ID: Subject: Re: [PATCH] x86/fpu/xstate: clear XSAVE features if DISABLED_MASK set From: Sean Christopherson To: Jon Kohler Cc: Dave Hansen , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , "x86@kernel.org" , "H. Peter Anvin" , "Chang S. Bae" , Kyle Huey , "neelnatu@google.com" , Andrew Cooper , "linux-kernel@vger.kernel.org" , "kvm@vger.kernel.org" , Paolo Bonzini Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 31, 2023, Jon Kohler wrote: >=20 > > On May 30, 2023, at 6:22 PM, Dave Hansen wrote: > >=20 > > On 5/30/23 13:01, Jon Kohler wrote: > > Is that the only problem? kvm_load_guest_xsave_state() seems to have > > some #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS code and I can't > > imagine that KVM guests can even use PKRU if this code is compiled out. ... > >> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate= .c > >> index 0bab497c9436..211ef82b53e3 100644 > >> --- a/arch/x86/kernel/fpu/xstate.c > >> +++ b/arch/x86/kernel/fpu/xstate.c > >> @@ -798,7 +798,8 @@ void __init fpu__init_system_xstate(unsigned int l= egacy_size) > >> unsigned short cid =3D xsave_cpuid_features[i]; > >>=20 > >> /* Careful: X86_FEATURE_FPU is 0! */ > >> - if ((i !=3D XFEATURE_FP && !cid) || !boot_cpu_has(cid)) > >> + if ((i !=3D XFEATURE_FP && !cid) || !boot_cpu_has(cid) || > >> + DISABLED_MASK_BIT_SET(cid)) > >> fpu_kernel_cfg.max_features &=3D ~BIT_ULL(i); > >> } > >=20 > > I _think_ I'd rather this just be cpu_feature_enabled(cid) rather than > > using DISABLED_MASK_BIT_SET() directly. +1, xstate.c uses cpu_feature_enabled() all over the place, and IMO effecti= vely open coding cpu_feature_enabled() yields less intuitive code. And on the KVM side, we can and should replace the #ifdef with cpu_feature_= enabled() (I'll post a patch), as modern compilers are clever enough to completely op= timize out the code when CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS=3Dn. At that poi= nt, using cpu_feature_enabled() in both KVM and xstate.c will provide a nice bit of s= ymmetry. Caveat #1: cpu_feature_enabled() has a flaw that's relevant to this code: i= n the unlikely scenario that the compiler doesn't resolve "cid" to a compile-time constant value, cpu_feature_enabled() won't query DISABLED_MASK_BIT_SET(). = I don't see any other use of cpu_feature_enabled() without a hardcoded X86_FEATURE_= *, and the below compiles with my config, so I think/hope we can just require a co= mpile-time constant when using cpu_feature_enabled(). diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufe= ature.h index ce0c8f7d3218..886200fbf8d9 100644 --- a/arch/x86/include/asm/cpufeature.h +++ b/arch/x86/include/asm/cpufeature.h @@ -141,8 +141,11 @@ extern const char * const x86_bug_flags[NBUGINTS*32]; * supporting a possible guest feature where host support for it * is not relevant. */ -#define cpu_feature_enabled(bit) \ - (__builtin_constant_p(bit) && DISABLED_MASK_BIT_SET(bit) ? 0 : stat= ic_cpu_has(bit)) +#define cpu_feature_enabled(bit) \ +({ \ + BUILD_BUG_ON(!__builtin_constant_p(bit)); \ + DISABLED_MASK_BIT_SET(bit) ? 0 : static_cpu_has(bit); \ +}) =20 #define boot_cpu_has(bit) cpu_has(&boot_cpu_data, bit) =20 Caveat #2: Using cpu_feature_enabled() could subtly break KVM, as KVM adver= tises support for features based on boot_cpu_data. E.g. if a feature were disabl= ed by Kconfig but present in hardware, KVM would allow the guest to use the featu= re without properly context switching the data. PKU isn't problematic because= KVM explicitly gates PKU on boot_cpu_has(X86_FEATURE_OSPKE), but KVM learned th= at lesson the hard way (see commit c469268cd523, "KVM: x86: block guest protec= tion keys unless the host has them enabled"). Exposing a feature that's disable= d in the host isn't completely absurd, e.g. KVM already effectively does this fo= r MPX. The only reason using cpu_feature_enabled() wouldn't be problematic for MPX= is because there's no longer a Kconfig for MPX. I'm totally ok gating xfeature bits on cpu_feature_enabled(), but there sho= uld be a prep patch for KVM to clear features bits in kvm_cpu_caps if the correspo= nding XCR0/XSS bit is not set in the host. If KVM ever wants to expose an xstate= feature (other than MPX) that's disabled in the host, then we can revisit fpu__init_system_xstate(). But we need to ensure the "failure" mode is tha= t KVM doesn't advertise the feature, as opposed to advertising a feature with= out without context switching its data. > > But, I guess this probably also isn't a big deal for _most_ people. An= y > > sane distro kernel will just set CONFIG_X86_INTEL_MEMORY_PROTECTION_KEY= S > > since it's pretty widespread on modern CPUs and works across Intel and > > AMD now. >=20 > Ack, I=E2=80=99m using PKU as the key example here, but looking forward t= his is more of a > correctness thing than anything else. If for any reason, any xsave featur= e is disabled > In the way that PKU is disabled, it will slip thru the cracks. I'd be careful about billing this as a correctness thing. See above.