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;
Message-ID: <1bb04108-a250-7cc7-e008-a227cbafdb15@intel.com>
Date:   Mon, 17 Oct 2022 15:39:39 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.1
Subject: Re: [PATCH] x86/fpu: Remove dynamic features from xcomp_bv for
 init_fpstate
Content-Language: en-CA
To:     Dave Hansen <dave.hansen@linux.intel.com>,
        <linux-kernel@vger.kernel.org>
CC:     <x86@kernel.org>, Yuan Yao <yuan.yao@intel.com>,
        Dave Hansen <dave.hansen@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>
References: <20221011222425.866137-1-dave.hansen@linux.intel.com>
From:   "Chang S. Bae" <chang.seok.bae@intel.com>
In-Reply-To: <20221011222425.866137-1-dave.hansen@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?N1RkNEgvNHVYWEVWZTZxcHhRSDg3Z0hoZHFyTGZsMS9kK05raC80bEpTVGhZ?=
 =?utf-8?B?dlNxYmp6ZzRUUHpVRkVkUHJMeSt1eFVDYUR1MVZLb291K000L3AvVlBMa2RL?=
 =?utf-8?B?WGhIbmJycFRMVkRmTjcxNFljYXlmMzc0R2hZeHZXR0VkanQxM2I3ajQya0xI?=
 =?utf-8?B?ZjAxSFRMa08zUHBOUjBkakdMVTFZcnpEU1duSXV4a1dwTGxBWXdPNWFsNUJL?=
 =?utf-8?B?M2x4d0lnNTh4VVEwQjFoRUptQ2JOa1oyNm90amxTN3BCeU9mYUlOYjluOFVJ?=
 =?utf-8?B?dmdjMDAxV3M1aUZ0NTZHQnAyemVYVHZJUXQ1Rjd3emw5N2dqZTNpd1hMbVNp?=
 =?utf-8?B?K0FhUjNSaHVhVUtpa2lPM3greXRJdHA5L2V6ZEthblJEejBMUWNWWnB6Vi9l?=
 =?utf-8?B?ZWtXem53NFNYSVNWdFBzY3pIaGRseGgrM2RIY2ZXUElZQ0tvVjZLL2cwZ2xO?=
 =?utf-8?B?QmVDRXpBTFlFdnFjWkdaMGlER1lLc08yc01mVDhmbjYwUUZFNlZLdmxaRVhQ?=
 =?utf-8?B?RFhiZXh3ekJoLzJRYkZ1ZDEyWkdQOVY2SkJvSktHR25zRVE3TVgrclltRDVq?=
 =?utf-8?B?dFVsazQxa2ZERG1Ea0NleW1JQk44TWxSeVJwOXlsb25ERzZvdUZrUjZ4UUxz?=
 =?utf-8?B?VytjbkhzTkVvOGJYTVEwUE1HQzA3MXo5OWpOdmZJeU9FeHRMM3ZtcTNudTZy?=
 =?utf-8?B?M21HcmxSeXZwL1FHNEhEWlRQRUxmZ3hkSVJTSGQ4VmFrUy9GMG11ODBOclRt?=
 =?utf-8?B?bGQvUG1NY3JIbHpBOUl4clJOR0M4SDJaZ1NEYWlxVTgvSk5DZ0lqVXdjTHNG?=
 =?utf-8?B?elJaNm01UFZBNjZ3RnhjazRwTGFMTFVMVnl6Y0w5NU1zV1h2WEVGdStJNENw?=
 =?utf-8?B?VWFqZHMyazNBRWp1WDJJS3BoY2oyYmVwbnpvZnlROXhVRFhwN2t5NW5OaFFX?=
 =?utf-8?B?RnAyeFNWT3U2WUJsSlNHYlRRQU8rUXZwMU9WRlpNZjVmZzMzZXRMbEVrbzVS?=
 =?utf-8?B?eDRVTVovLzZHMDVoWmRVaExWSDB1YWsyZ09DQkFtWGRTWkhTbVVpMUNsRldr?=
 =?utf-8?B?dU8wYWtzM0xWSkRFZHlvbkdjaEFDMEhDeTFPTEZEdWRLUWRZanIxMDMyWDc1?=
 =?utf-8?B?a1c2MDN1WlJhQ1gzQ3hKQ0Y2NzJ6RGYyQ2tOVGR6TzR3Szd6SzR3ZDBlaVds?=
 =?utf-8?B?M05CZmpBOThjQ0JWa1FpbEpYU1dKZHUydUFsZmh2cGlxWjRpNzRHdEQ5L0or?=
 =?utf-8?B?Y05wdXZUbFIreXU2TmdvOWIvRFFEdDZWcThkc1ladWYxdnQyK1BRREhBbmxM?=
 =?utf-8?B?dXRQOFZhUngvTmpLQTVnTVVML09Bem9IU1dEYTBDWTlCYUhjc2p0a1RKRjQ2?=
 =?utf-8?B?OWNic0F1amlvR0RjSVdpaGYyNjc5UkpmRytkV2FPSjVFcnFudnM3M3pGdkEv?=
 =?utf-8?B?K2tZM0l1MXVUU2E3aEVGVFozbmdKSkt4UGR6Vy81Q24zRmlmcVU1R0xFWFBJ?=
 =?utf-8?B?dzBlMmlpRzRUR2FxRFcvc05EVXdFUmpZaDRsY3JRbU9OSk1iQ0k4Z1E4TWJP?=
 =?utf-8?B?TFlBMWZGYXdLb3ZlY2NXV1ZBbzR4TUJKaVdIR251ZlF4eWtybllLMmwvU013?=
 =?utf-8?B?a2txNDdEalltc0REVXp0ZkZIRmhCcDNCVlNzUUZQY09UKy9ETHVuMTNrTWxk?=
 =?utf-8?B?eWkwYVp3V05FZTN2MDRRcEFLVGlpMWxobmMrVHdJTjZtbHBXbms0eVlqSGlk?=
 =?utf-8?B?OEMyK0p1N2twYkRsOWJVVFNMWFc0VjIyNFNIZVZNTFdRV3VnT0ZiRGlZejRH?=
 =?utf-8?B?endaaFA4SjBROHo0K1EreUhDd05pN1VhckJiTjJ1MGZGZEZyMEh3SzZjZFpS?=
 =?utf-8?B?THJaaVA0L0UrNyt0YmJ1d0orcEJGcWh3bDFpaW1ORnc1eFE1Tis2Y1k4MUQr?=
 =?utf-8?B?RW4xeFJmVXBoTFNXZnhmczdlb1VZVzV6eHJadWFJM0lscmwxdVRPeEFITGFn?=
 =?utf-8?B?SDNhVXQxTUNXWE1TdHFjelJQaE5KWUhvVFJTVmhXU3hWbmlhdXcwRUlqUVp4?=
 =?utf-8?B?WkpKVGJWckRvMTJORVh5ekdiQW9sOW5LZVRkMFo4MTVpd2FUdGZaclo0cFFo?=
 =?utf-8?B?d0xKeWFJN1hMSjVMRmk3dzI0Uzd6eE5Mdlh1OFhRUFN6VGtkMEFiaWJVUno4?=
 =?utf-8?B?N1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1b03a355-7c82-444e-9acf-08dab0907ae0
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4855.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Oct 2022 22:39:41.1540
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: C4mgPMSG2ZFvpPZ/jrSRgTm3ZUEVQ1PdVwgnqrToXpv9oAb9Skg1xOoHcRzuJp8DWMbyPl8nQ7fOrvWZ/NSy+CiprqK/52kU54Tcogd+yv8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB6624
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_HI,SPF_HELO_NONE,SPF_NONE 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: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 10/11/2022 3:24 PM, Dave Hansen wrote:
> 
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 3b28c5b25e12..4d64de34da12 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -526,6 +526,9 @@ static void __fpstate_reset(struct fpstate *fpstate, u64 xfd)
>   	fpstate->xfeatures	= fpu_kernel_cfg.default_features;
>   	fpstate->user_xfeatures	= fpu_user_cfg.default_features;
>   	fpstate->xfd		= xfd;
> +
> +	/* Ensure that xcomp_bv matches ->xfeatures */
> +	xstate_init_xcomp_bv(&fpstate->regs.xsave, fpstate->xfeatures);
>   }

I have some difficulty understanding the problem without this. Maybe I'm 
missing something here:

We have two call sites for this -- (a) one for the guest fpstate 
allocation [1] and (b) the other for the reset [2].

(a) The former has a call chain to init xcomp_bv:
   fpu_alloc_guest_fpstate()->fpstate_init_user()->xstate_init_xcomp_bv()

(b) And the latter picks up the default area:
   void fpstate_reset(struct fpu *fpu)
   {
	/* Set the fpstate pointer to the default fpstate */
	fpu->fpstate = &fpu->__fpstate;
	__fpstate_reset(fpu->fpstate, init_fpstate.xfd);
	...
   }
Then, xcomp_bv looks to be subsequently written by XSAVE* or by copying 
from init_fpstate.

There are three distinct call sites for fpstate_reset():
* fpu_clone() [3]: the child will either copy from init_fpstate or do 
XSAVE* that will update xcomp_bv according to ->xfeatures.
* fpu__init_system_xstate() [4]: When the init task switches away, 
xcomp_bv will be updated by XSAVE*.
* fpu_flush_thread() [5]: xcomp_bv will be copied from init_fpstate via 
fpu_reset_fpregs().

Thanks,
Chang

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/core.c#n229
[2]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/core.c#n535
[3]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/core.c#n567
[4]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/xstate.c#n869
[5]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/core.c#n744