Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <de1b148c-45c6-6517-0926-53d1aad8978e@intel.com>
Date:   Wed, 25 Oct 2023 22:49:13 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH v6 06/25] x86/fpu/xstate: Opt-in kernel dynamic bits when
 calculate guest xstate size
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>
CC:     Dave Hansen <dave.hansen@intel.com>, <pbonzini@redhat.com>,
        <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <peterz@infradead.org>, <chao.gao@intel.com>,
        <rick.p.edgecombe@intel.com>, <john.allen@amd.com>
References: <20230914063325.85503-1-weijiang.yang@intel.com>
 <20230914063325.85503-7-weijiang.yang@intel.com>
 <e0db6ffd-5d92-2a1a-bdfb-a190fe1ccd25@intel.com>
 <1347cf03-4598-f923-74e4-a3d193d9d2e9@intel.com>
 <ZTf5wPKXuHBQk0AN@google.com>
From:   "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <ZTf5wPKXuHBQk0AN@google.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aGtrekU5RzY5d0NFdUpNOGpFekhPeHE3UFMvY0gwWmZ2bFBibWMvNnpEeklx?=
 =?utf-8?B?cFJaZ2oweVNjZUZBczdnVVhZMVZjTUJxUUVsWTM2RXdZc0wyQnNsWWxsYWFl?=
 =?utf-8?B?M1VNNFFaVUhzTGV1d3NiaThuR25vY2dISnh3ZHpTQWxISkNEbmlIYk5XOHhE?=
 =?utf-8?B?NUZnd2JVMFFJZUF2a3E2aW4rQjdFN2Q2YmxYVHZlTWtUZ0dseEJsK3Y0djhI?=
 =?utf-8?B?WmtZZzJkd0M5djZDSCtFYm5aeFBDOVM5UTN5L3M2UnJMVU1MSTh6MUpGNC9y?=
 =?utf-8?B?QlZTdmdFMnkwOHJtSnFaRmhPbFlnU3FJbExUQ1k3d3Y4VmpaQmFKeDd4eUx0?=
 =?utf-8?B?dVJpREhTcVEzR1RvTXo0cXd1eUdnZHhOZ2Z6ZkNQdnowcDhSeEJyTjlZcVV1?=
 =?utf-8?B?aGRFVDdlTGZwVGxQWDBPNmdtNm9XSlpmNTdTUjAwWjBuQ2MzSmprTlhLdng1?=
 =?utf-8?B?eDZFN0M1dTljdG1TYzFOQjAyZ1MzY1h2YmVvMmZLZHRBcGJJMDMzWGJndFBC?=
 =?utf-8?B?dldjblViYkYvdEdPSlVWNVZYRlBPdGYvMklkbnI4WGE3QWluOXd0WTU4ZVFE?=
 =?utf-8?B?ZjFnRnplUEpDemJnQ3V6RVNISksxUkE0YjY5UlFUYXljSVduT3ErcFAxUVdp?=
 =?utf-8?B?QUVjd2JHOTNPQ2JQSXN3c1hpQ2xYRkQrcEQrTG91WHVKaUViZ3hmZzNsM1Rx?=
 =?utf-8?B?V2pkSWIvNnNsSlZ1M0Y5RHdLZks3OUF6ZW1NU2pzcmZ4V0xLN1NNOHV4Q0JY?=
 =?utf-8?B?dk9Hb0lTeStaTDJJR2Q5ekhlM0ZFOEpTV0pNb3hTYzNMc1ZMbVlEc055U1h5?=
 =?utf-8?B?SDE1MGpmMkJPeUxYaXdtV1o5dm56dStrV3hZUmVqQ2JkYmh3RFBhVENjenVt?=
 =?utf-8?B?WGtzaGlPMkI3dk00OUF0L0JqSFR3YmM4aE1NRjZKVFNGdmVlT3hlZW1HTFZ4?=
 =?utf-8?B?MmZrZXFqOGQvWDgrOWhsYmJ3czVpcE15QXh2QXhhd1VXRHR6OGdSN2d0WW0v?=
 =?utf-8?B?QURKTHdCMlc5NWJXVzdMaUsvZWlyK2MzWVUyMFR4a2tYWmhwTDBJbi9TMnVI?=
 =?utf-8?B?T2N6TlhxVW9GZnVUSmcyK2JuOTlhU010dHhWclRScElvMWhQUzhYZ2U0ZDJD?=
 =?utf-8?B?Lzh4M01vZlYxS2RlcldSYmJGTE5RVXRjTmFWaWNWMG9kcmcwODkxaTRGbEE0?=
 =?utf-8?B?MGFreXRKVmE0d1BHcmkzeitmMlNzTjJ6aGFnZ1BCS0RqMXNPUWJHMDBxSUJz?=
 =?utf-8?B?Z2VScG1YamJJZjNJR3FiUlV3RWlpQ1N1OEhQM2NzV25oOWxremQvY2lod1I5?=
 =?utf-8?B?emcwUWMxdnBNVmR6ajF4TEpuNjhrRUtlRzBVdHhyRisra0FLbjA2NkFlckdO?=
 =?utf-8?B?STQ0MXEzbDU5anJkQ2Q4ZGJROXppeVprNHNxV0RKQ2RqaG5WbEZyTmFLSG1m?=
 =?utf-8?B?S0U5aTFVbitDbWR5Ynh0ZlpWZFdUNzdySjJKODdJQzYvOGJkUGVTa0hWMEVX?=
 =?utf-8?B?Y3IyVFRENmh1Mit5b0ozVmNqbHZpa0lqNU90OU1yQVVEYkI0Uk1OSkluei90?=
 =?utf-8?B?b24yZXhFbHhxL0xiRW5Cd210VUJZZEZzZzFvNkZCSUJNVi9XMGtiTXAwbnAr?=
 =?utf-8?B?RHdTWmJrazFCcDdFSEVVT1gxTWhMUTZqdHc2ZDJPTzF6cEl3SGI1UFJmbDIz?=
 =?utf-8?B?VmVINFpCVTdvampPMEhCRzgrcmhCaHZiN2IxYUpqTEpZUzNhdGVPTDFHdE5Z?=
 =?utf-8?B?bnpyZHovNEkwNTRFUGZac1NjWXRHbVRTOXBkc3BueWcyUHUxeFh3ZEgwMVlK?=
 =?utf-8?B?WHMydWJreEtFaEQ0Z0h5QUVxWXZoTm9hdlYvVG55WGZMK2FId1FpRzgwRHNF?=
 =?utf-8?B?bXdSS0dnRDVQZENydWowVVA5czI0MFFHYmszOFhVbk9jUjZEODNOYzFYZHR1?=
 =?utf-8?B?UmZaSzRJMm5XL00yU29DdWkrN1cvMGZTTW02RFV3TTR4RUcraG1UdzAxUk11?=
 =?utf-8?B?SEpoaDBPUHFUdjZxQVRBOTRLTTFFRzNVMCttYzBBdE56TjFZUkdTVGtjWTdZ?=
 =?utf-8?B?Tnd4QXQ4UzVyREVaaWxUc3hHSVRZSTMvRXh0d2JMNTdURnR0MVRjYmM5cDM4?=
 =?utf-8?B?VTkxWHpSTVIrZ3UyWWFVaTl3VlQzaGxmTzJSSmlVTHZaVlpJVlcybjZMWEFF?=
 =?utf-8?B?S0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 68e05cb4-d6f2-485c-7ba8-08dbd5699307
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Oct 2023 14:49:21.9461
 (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: LgkxOF7oJOQlDY0b/F2TIJ/owgTVn0krODTXNU7zZyNm2GeYuMy3P0DfGMZRPrMCCsax8GWOoD+sYT9Fk2UB7A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB5794
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.2 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Wed, 25 Oct 2023 07:51:00 -0700 (PDT)

On 10/25/2023 1:07 AM, Sean Christopherson wrote:
> On Fri, Sep 15, 2023, Weijiang Yang wrote:
>> On 9/15/2023 1:40 AM, Dave Hansen wrote:
>>> On 9/13/23 23:33, Yang Weijiang wrote:
>>>> --- a/arch/x86/kernel/fpu/xstate.c
>>>> +++ b/arch/x86/kernel/fpu/xstate.c
>>>> @@ -1636,9 +1636,17 @@ static int __xstate_request_perm(u64 permitted, u64 requested, bool guest)
>>>>    	/* Calculate the resulting kernel state size */
>>>>    	mask = permitted | requested;
>>>> -	/* Take supervisor states into account on the host */
>>>> +	/*
>>>> +	 * Take supervisor states into account on the host. And add
>>>> +	 * kernel dynamic xfeatures to guest since guest kernel may
>>>> +	 * enable corresponding CPU feaures and the xstate registers
>>>> +	 * need to be saved/restored properly.
>>>> +	 */
>>>>    	if (!guest)
>>>>    		mask |= xfeatures_mask_supervisor();
>>>> +	else
>>>> +		mask |= fpu_kernel_dynamic_xfeatures;
> This looks wrong.  Per commit 781c64bfcb73 ("x86/fpu/xstate: Handle supervisor
> states in XSTATE permissions"), mask at this point only contains user features,
> which somewhat unintuitively doesn't include CET_USER (I get that they're MSRs
> and thus supervisor state, it's just the name that's odd).

I think the user-only boundary becomes unclear when fpstate_reset() introduce below line:
fpu->perm.__state_perm          = fpu_kernel_cfg.default_features;

Then in xstate_request_perm(), it re-uses above reset value for __xstate_request_perm(),
so in the latter, the mask is already mixed with supervisor xfeatures.

> IIUC, the "dynamic" features contains CET_KERNEL, whereas xfeatures_mask_supervisor()
> conatins PASID, CET_USER, and CET_KERNEL.  PASID isn't virtualized by KVM, but
> doesn't that mean CET_USER will get dropped/lost if userspace requests AMX/XTILE
> enabling?

Yes, __state_size is correct for guest enabled xfeatures, including CET_USER, and it gets
removed from __state_perm.

IIUC, from current qemu/kernel interaction for guest permission settings, __xstate_request_perm()
is called only _ONCE_ to set AMX/XTILE for every vCPU thread, so the removal of guest supervisor
xfeatures won't hurt guest! ;-/

> The existing code also seems odd, but I might be missing something.  Won't the
> kernel drop PASID if the guest request AMX/XTILE?

Yeah, dropped after the first invocation.

> I'm not at all familiar with
> what PASID state is managed via XSAVE, so I've no idea if that's an actual problem
> or just an oddity.
>
>>>>    	ksize = xstate_calculate_size(mask, compacted);
>>> Heh, you changed the "guest" naming in "fpu_kernel_dynamic_xfeatures"
>>> but didn't change the logic.
>>>
>>> As it's coded at the moment *ALL* "fpu_kernel_dynamic_xfeatures" are
>>> guest xfeatures.  So, they're different in name only.
> ...
>
>>> Would there ever be any reason for KVM to be on a system which supports a
>>> dynamic kernel feature but where it doesn't get enabled for guest use, or
>>> at least shouldn't have the FPU space allocated?
>> I haven't heard of that kind of usage for other features so far, CET
>> supervisor xstate is the only dynamic kernel feature now,  not sure whether
>> other CPU features having supervisor xstate would share the handling logic
>> like CET does one day.
> There are definitely scenarios where CET will not be exposed to KVM guests, but
> I don't see any reason to make the guest FPU space dynamically sized for CET.
> It's what, 40 bytes?

Could it also be xsave/xrstor operation efficiency for non-guest threads?

> I would much prefer to avoid the whole "dynamic" thing and instead make CET
> explicitly guest-only.  E.g. fpu_kernel_guest_only_xfeatures?  Or even better
> if it doesn't cause weirdness elsewhere, a dedicated fpu_guest_cfg.  For me at
> least, a fpu_guest_cfg would make it easier to understand what all is going on.

Agree,  guess non-kernel-generic designs are not very much welcome for kernel...