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: <50e67263-33ba-9921-1bc2-a37b99bc2459@intel.com>
Date:   Tue, 11 Apr 2023 18:21:25 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.11.0
Subject: Re: [RFC PATCH] x86/fpu/xstate: Add more diagnostic information on
 inconsistent xstate sizes
Content-Language: en-US
To:     "Chang S. Bae" <chang.seok.bae@intel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Borislav Petkov <bp@alien8.de>, Ingo Molnar <mingo@redhat.com>
CC:     linux-kernel <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>,
        Chintan M Patel <chintan.m.patel@intel.com>,
        Thiago Macieira <thiago.macieira@intel.com>
References: <20230405183942.734019-1-fenghua.yu@intel.com>
 <113918f9-0e44-3d46-8b48-028277ec26bb@intel.com>
 <c716e021-99ad-2cb7-44c0-ec8393a3440d@intel.com>
 <2af114b9-2737-70e5-f534-e60416b52246@intel.com>
From:   Fenghua Yu <fenghua.yu@intel.com>
In-Reply-To: <2af114b9-2737-70e5-f534-e60416b52246@intel.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?WEthK2EwMnVuNUtGY3pFa3NrU3ZvcUlRT1I5TDNKQWNJeW81ZE5kNVA4eTFv?=
 =?utf-8?B?VGROejN4Q0kvdm9jVVBjV3Z0a2I5VmNJWEF6NjVKYzZBZ2ZnbTl5OWtUWWx2?=
 =?utf-8?B?ZjhGcDd6dmVXSXFRdnZyL1U1SVB2aHlXOUwrYzhPRWczZzVmYXRoL1hJb3VM?=
 =?utf-8?B?WVg3K1UxNDhsSzZzQ0NLWmh6T3VCWmpKOVk0ZDZ4R2RuWERBc3VaK2NtVlRV?=
 =?utf-8?B?VWRKNCtDRWhTN00zTWEzb3pqeGI3WU1JYVluMUlWb3ZzR0lMdXVPKzBIdXMv?=
 =?utf-8?B?U1FieTR6a3YraWRYT2hkTzIwQVVybUZqZ2FiM2Vja2tGRWJkTTl4ZW5jVlY1?=
 =?utf-8?B?SExmeFM0ZnJQRkVPK1VGc2ZBYXZzVXZKb0N1bHlocDZZTnB0Zmk1eGVvZ3BW?=
 =?utf-8?B?ZGV0YW9mVWFJRzRjNEpkejNxZmF0OEl1Y0VaMGx6bTZ4eVV2YWFMSWhmbTA5?=
 =?utf-8?B?SWdvWUNTT1ZhSVhjaUpzVUxlVThwOXpkeGlTbzdHTk1SZWlBVXpFR2JOc3ZD?=
 =?utf-8?B?NiswQjRGSCt3RnRMcGRXT21GU1lqa0VzL1hobjRsZmFUZG9wd1JOQ1dPT2E3?=
 =?utf-8?B?ckxQMnZkdlRQUmc5cGVPMW1hOHJ6OVpsK2E1NWNOQXdKUVUrcENkL0p6dHQ5?=
 =?utf-8?B?NUt4UVZNYXNGaWN6Wk5BWVFNYkY2RllBWEZNNExpN2c5M3NXZDJ0ejZnSjho?=
 =?utf-8?B?L3V5WU5qWXVabEYrS1FidFIyb1BxWHBZQWdPZ3FwZGNiSUZlV0tPcG5laXJQ?=
 =?utf-8?B?emtJQ1J1aTQ4a2NuTEx6eGdkakxLdU50V3JVR2UzK1lLbzFOQzFtS251bDJP?=
 =?utf-8?B?ZXVtYUxCT2RkeXhPWmhDZ1hBUVNwRE9ZclZuN05wMTc0Mm1GY0hqQjZ4c3lP?=
 =?utf-8?B?UXVSb1JJUGl2eGpwR1pXeWs0Nk9CWDhxMTc2U2l4ZXdQano5MFp3L0RzajRF?=
 =?utf-8?B?N0laVGZUWHFRTzNkYWNiRmxvLzBhc253eFNGZzljNjkzVklzcHlhU2tPcHVY?=
 =?utf-8?B?NHN5YnU1RGxaSGwxQlBMUWtIYmxCTGZDUzRocGRUV0NxSEl3UmlTT3RiTnZT?=
 =?utf-8?B?M2RJSzA4MGM4RzExWnJ0SFVBaGIyZDRraUdKUGVzWE9RRW9qdmxMOWdybnZj?=
 =?utf-8?B?ZHdkRzdEYTBJaVJSemgxYkxJNFhqbHQ4M0prb0hESnB3M0JXcHdzMlhqb1Bh?=
 =?utf-8?B?bDR4d1hkUm5EVjJ4VkVOWUduY2U0T1haRUZmeUIxbTlZKzFFZTdveXF3a09Q?=
 =?utf-8?B?SFlhSTJKdWZReklWcVBHZVlXWnJDN0RrUjdnb0FEVjdqV3FZVFdpbmRrR3NP?=
 =?utf-8?B?VkNVdVBuWHVaR2V5YTA2bzZWdGd2anpQUm92V2V3MjRxdlFoT3RkQ09yNGtm?=
 =?utf-8?B?cEtKdTRMSkYrbTJZMzR1LzBXSUNBcGZBd3BKdnFpZnFwM1dmaXlFczcraU9T?=
 =?utf-8?B?T2c4d21hUlhYOUR2TnhZSm5uVTc3bHVwbXVmMG1ub3ZXemd4VEJIdy9kOURT?=
 =?utf-8?B?bU1lMS93ZkhlMHp4UnZ0c0xNSjRXM2htVWxraFpMb1BRWEhrMGdCTFJPaE1K?=
 =?utf-8?B?MCtIb1N0WTVESG80ZzFmTTFyVW4rbzJkRm4rREdDZXNTM3IzbHVWU29sQWVo?=
 =?utf-8?B?RkhJYVVSbWpTTmFFbVlPNW41R1R6KzFXVmQ5WXVlM1lUbWhaS0ZMUlF0aG5u?=
 =?utf-8?B?YmM4SG5TeWgvcEk5VUl5Y2wwUFd2ZXh1dEVIdGl5UVNCTEplcTg2aXFRZWVY?=
 =?utf-8?B?YVdjK3IrNE9mTU1QdW82WmVPblRDM0JvTGJ2SitXUzBVdU02UllUY1hTZkZO?=
 =?utf-8?B?clNjdWlRR04wWElhODRYWFRCNWlsYzcveUxvb0VOdFVBVmk1dDBlYU1MckN1?=
 =?utf-8?B?d0xxUGVueU4xTHpzYzQyQ3ZCdEFuWis0SzFWYWZON2NPMTVXM0VFS0ZWRkRz?=
 =?utf-8?B?M1J3cXRYTThBSFc5YUdqQkxNMG1RSWxpMWIzVmhBQnBYelo5cUlBblVjQmJj?=
 =?utf-8?B?Tm1KblRENEdKdmM0SHZxWC9Qb3l2MXNWZ0VneHQ2Yk1wbXliYzJUa1VZdmtk?=
 =?utf-8?B?V3lCU2xuSnEva3RkY3Y2Qi81K210T2Z5OERrYzhxcTlWbDRqVzFTcWhiUHdw?=
 =?utf-8?Q?zZkV5BfdBT69XhajIHtL9NHe8?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 38c112c0-ea2c-45f1-a73a-08db3af43066
X-MS-Exchange-CrossTenant-AuthSource: IA1PR11MB6097.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Apr 2023 01:21:06.3966
 (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: kn4AjMli41mSFtv+x8mIQaF4HVJ2UHkhHt+ebmmIdCSIkv+FP18jYJMpRnWc7lqrJE7RTEddGhW4bA/TQuTxEg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB6785
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-6.6 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_MED,SPF_HELO_NONE,SPF_NONE,URIBL_BLOCKED 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

Hi, Chang,

On 4/11/23 09:29, Chang S. Bae wrote:
> On 4/10/2023 1:43 PM, Fenghua Yu wrote:
>> On 4/7/23 11:22, Chang S. Bae wrote:
>>> On 4/5/2023 11:39 AM, Fenghua Yu wrote:
>>>>
>>>> diff --git a/arch/x86/kernel/fpu/xstate.c 
>>>> b/arch/x86/kernel/fpu/xstate.c
>>>> index 0bab497c9436..5f27fcdc6c90 100644
>>>> --- a/arch/x86/kernel/fpu/xstate.c
>>>> +++ b/arch/x86/kernel/fpu/xstate.c
>>>> @@ -602,8 +602,37 @@ static bool __init 
>>>> paranoid_xstate_size_valid(unsigned int kernel_size)
>>>>           }
>>>>       }
>>>>       size = xstate_calculate_size(fpu_kernel_cfg.max_features, 
>>>> compacted);
>>>> -    XSTATE_WARN_ON(size != kernel_size,
>>>> -               "size %u != kernel_size %u\n", size, kernel_size);
>>>> +    if (size != kernel_size) {
>>>> +        u64 xcr0, ia32_xss;
>>>> +
>>>> +        XSTATE_WARN_ON(1, "size %u != kernel_size %u\n",
>>>> +                   size, kernel_size);
>>>> +
>>>> +        /* Show more information to help diagnose the size issue. */
>>>> +        pr_info("x86/fpu: max_features=0x%llx\n",
>>>> +            fpu_kernel_cfg.max_features);
>>>> +        print_xstate_offset_size();
>>>> +        pr_info("x86/fpu: total size: %u bytes\n", size);
>>>> +        xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
>>>> +        if (compacted) {
>>>> +            rdmsrl(MSR_IA32_XSS, ia32_xss);
>>>
>>> This shouldn't be directly read here because of the LBR state component.
>>>
>>> See the function comment:
>>>
>>>   * Independent XSAVE features allocate their own buffers and are not
>>>   * covered by these checks. Only the size of the buffer for task->fpu
>>>   * is checked here.
>>>
>>> But, isn't that max_features bitmask pretty much about it?
>>
>> How about getting IA32_XSS from xfeatures_mask_supervisor()? That's 
>> how to get kernel_size by setting IA32_XSS without independent 
>> features in get_xsave_compacted_size()
> I think what it tests here is comparing the sizes between the kernel 
> code and microcode calculations on the same input, which is the 
> max_features bitmask.
> 
> We know that the kernel code calculates the size based on it and also 
> takes it to write down there -- XCR0 and IA32_XSS. Then, showing that 
> bitmask looks to be enough I thought, no?

First of all, max_features is shown already.

Kernel_size from CPUID.0xd.0x1:EBX takes XCR0 | IA32_XSS as input. 
Platform may take wrong XCR0 or IA32_XSS and get wrong kernel_size. The 
purpose of this patch is to provide more debug info to help debug 
platform/kernel issue. So instead of a whole max_features, xgetbv() to 
get XCR0 and xfeatures_mask_supervisor() to get IA32_XSS provides more 
debug info in case platform may have issue in XCR0 or IA32_XSS.

In other words, splitting max_features into XCR0 and IA32_XSS and 
showing them individually provide more useful debug info than one single 
max_features value.

Does it make sense?

> 
> I still expect some acknowledgment of what is coded here for the kernel 
> calculation details.

The kernel calculation is shown in
+        print_xstate_offset_size();
+        pr_info("x86/fpu: total size: %u bytes\n", size);

Isn't that detailed enough to show offset and size of each xstate and 
sum of sizes?

After that,
+	pr_info("x86/fpu: kernel_size from CPUID.0xd.0x%x:EBX: %u bytes\n",
+               compacted ? 1 : 0, kernel_size);
shows how kernel_size is calculated from CPUID?

Using the above debug info, a real platform CPUID issue is shown clearly.

What other details are needed?

Thanks.

-Fenghua