Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: L9sJ7igl1phoiIEXAuQ9xJl3YrFkr5VLBPwSCegmCvDwewQjFDw4MBYaP7f9zeWRHhi9PKl6Gi
 leJ92X76RCfQ==
IronPort-SDR: s4znUWJU2XXel6e0JhbKBdQEeigoyLFbRp7i1UPfMAnK5HT36ADEossmBe48hJB8pmJGbEO6H8
 JDsDlUuZuNdA==
Date:   Tue, 30 Mar 2021 12:25:58 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Andy Lutomirski <luto@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>
Cc:     Dan Williams <dan.j.williams@intel.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
        linux-kselftest@vger.kernel.org
Subject: Re: [PATCH V4 07/10] x86/pks: Preserve the PKRS MSR on context switch
Message-ID: <20210330192558.GD3014244@iweiny-DESK2.sc.intel.com>
References: <20210322053020.2287058-1-ira.weiny@intel.com>
 <20210322053020.2287058-8-ira.weiny@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210322053020.2287058-8-ira.weiny@intel.com>
User-Agent: Mutt/1.11.1 (2018-12-01)
Precedence: bulk

[snip]

<self review>

The test bot reported build errors on i386 yesterday.  Not sure why they were
not caught before...

Anyway that caused me to look at this patch again and I've found a couple of
issues noted below.  Combined with Sean's review I'll be re-spinning a new v5.

> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 546d6ecf0a35..c15a049bf6ac 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -765,6 +765,7 @@
>  
>  #define MSR_IA32_TSC_DEADLINE		0x000006E0
>  
> +#define MSR_IA32_PKRS			0x000006E1

This belongs in patch 5 where it is 'used'.  Note that nothing is really used
until the final test patch...  But in review this define does not make any
sense here...

>  
>  #define MSR_TSX_FORCE_ABORT		0x0000010F
>  
> diff --git a/arch/x86/include/asm/pkeys_common.h b/arch/x86/include/asm/pkeys_common.h
> index 0681522974ba..6917f1a27479 100644
> --- a/arch/x86/include/asm/pkeys_common.h
> +++ b/arch/x86/include/asm/pkeys_common.h
> @@ -17,4 +17,18 @@
>  #define PKR_AD_KEY(pkey)     (PKR_AD_BIT << PKR_PKEY_SHIFT(pkey))
>  #define PKR_WD_KEY(pkey)     (PKR_WD_BIT << PKR_PKEY_SHIFT(pkey))
>  
> +/*
> + * Define a default PKRS value for each task.
> + *
> + * Key 0 has no restriction.  All other keys are set to the most restrictive
> + * value which is access disabled (AD=1).
> + *
> + * NOTE: This needs to be a macro to be used as part of the INIT_THREAD macro.
> + */
> +#define INIT_PKRS_VALUE (PKR_AD_KEY(1) | PKR_AD_KEY(2) | PKR_AD_KEY(3) | \
> +			 PKR_AD_KEY(4) | PKR_AD_KEY(5) | PKR_AD_KEY(6) | \
> +			 PKR_AD_KEY(7) | PKR_AD_KEY(8) | PKR_AD_KEY(9) | \
> +			 PKR_AD_KEY(10) | PKR_AD_KEY(11) | PKR_AD_KEY(12) | \
> +			 PKR_AD_KEY(13) | PKR_AD_KEY(14) | PKR_AD_KEY(15))

The same is true for this macro.  While it is used in this patch it is used
first in patch 5.  So it should be there.

I'm letting 0-day crank on these changes but there should be a v5 out very
soon.

Sorry for the noise,
Ira