Message-ID: <e91b9172-8a2e-e299-a84f-1e9331c51cb7@intel.com>
Date:   Tue, 21 Feb 2023 19:05:04 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v3 01/13] x86/fpu/xstate: Avoid getting xstate address of
 init_fpstate if fpstate contains the component
To:     Mingwei Zhang <mizhang@google.com>,
        Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>
CC:     "H. Peter Anvin" <hpa@zytor.com>, <linux-kernel@vger.kernel.org>,
        <kvm@vger.kernel.org>, <linux-kselftest@vger.kernel.org>,
        Jim Mattson <jmattson@google.com>,
        Venkatesh Srinivas <venkateshs@google.com>,
        "Aaron Lewis" <aaronlewis@google.com>,
        Chao Gao <chao.gao@intel.com>
References: <20230221163655.920289-1-mizhang@google.com>
 <20230221163655.920289-2-mizhang@google.com>
Content-Language: en-US
From:   "Chang S. Bae" <chang.seok.bae@intel.com>
In-Reply-To: <20230221163655.920289-2-mizhang@google.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?VXMvdDRTQWpNbXVGb1hyMGdSYmpVRjV2RGY0R0QzaWpGQk8rem5wZjFIY1Av?=
 =?utf-8?B?VUdVdjlLL1ZJK2g1MTIvcTcvRi9FQzBJLzZnVk5STDJOSUttdjF2c0E2a1R1?=
 =?utf-8?B?VVVSODdSVXVST0U5aTN4UUdMMllnQzdyRFgvVWc2UGhadHVkVUk3QTFYa0pS?=
 =?utf-8?B?ZkhLbmJkcTZXTjllTS9VNHRpeHVIVnhTNUx3T2NMNDMwSXJ6MXVlaDBWYks1?=
 =?utf-8?B?YnJZUndkc0c4cmZSTjhZZ3l4ejhvWVhsNmxjT3hMQTNhYjNsQkZNaFZGRUZh?=
 =?utf-8?B?QTQvUWlHSDZORUZXS25teGozb1g5UmJZUjk3WURtZkdtK1RGY2pFdXpQYmND?=
 =?utf-8?B?YU54QndPV0hUTkd3VVg0UExKNVpGT25KVlJhVXRDMllkMW5sVXF1cGhUVDJD?=
 =?utf-8?B?VnlPbEJ3cmZyTGxyS0NrVzdka29RdTk2WkJmRjd1Q2psY2Myd2tZMFlZTjRr?=
 =?utf-8?B?anNrTldXYkFUSk1lRFJjZmhLRnhKdlgwcWF1blJDRUhFR0FxTHpjMWlsM2hY?=
 =?utf-8?B?WU0wR0pvcFBuZ1UxT3YxMXBmcHZ6Nmw4cVYvVVBPVW81UjNiMjhQZ2VXNGhy?=
 =?utf-8?B?SnNBUEE4ODRHQ0srNE5heW16QkZhRjhHZ0lKckhXUnV0RkI3RzRoaER3TUl4?=
 =?utf-8?B?WTV4a2h3amxoZ00yQzhpaGVHZmpEVGY1SlMrcE42WmhiSk05UmRoQkdNc1E1?=
 =?utf-8?B?ak5hZEhKalZveUx5YXNpZjJUamh5eDdEZzA2ZTRKQkFIWEtpSlZpL2V2MExm?=
 =?utf-8?B?Vnlvd3BxWm44cVJmckxXQStoOFBKdE9nS3R5bk9NWUpTMmZQUHpPVktkbzla?=
 =?utf-8?B?S1FIRmRiS2U2TnBmNVZ0OW83VjR0elE4QkJJdVQrNTF0RVhhMUJmdU5TNEIy?=
 =?utf-8?B?Qk1PeFhLRzlCTSt1bWJNenpDRFFLNjhYa1d5a2VsWlQyT1R3aTdtU3dVVUUz?=
 =?utf-8?B?WVJqeU4yeTdnOStNVWxwSzdzZ3Y0SFEyKzduME1MSC9oKzJ6c2JqNWtqNVEw?=
 =?utf-8?B?OVg1amMyTERiS05YTUN0S1RjMTJoeUxCZ1lJUmNoV2d5ZEg1K1FjRW5WVUcz?=
 =?utf-8?B?emlIM1RSRXl4Ris5OUl0ZTE1b1lQN0J1R0FVbUIxOWw3OWxycHEyYXl5T0Ri?=
 =?utf-8?B?TmZHSjdCRHcrUnB4MHVocnVBVmtMcVpPbVd5TXJYdXRxbFZwQlhodi9VeGlM?=
 =?utf-8?B?c0hrMTVjWHhjODNRS3NUajc3Mk8zL3ZkYW15dGNQWTIvdEh1Tnh6OHhFM2pV?=
 =?utf-8?B?aUQvWDhxVkZSWWdvcW9icnRsVm1wOUF0cEU4VzY5VEt3QVNNcEtrOTg3a2hZ?=
 =?utf-8?B?QUtFeEE3YmhvOXVwZXFmRDh4NTlkMjliT21jUkc1YUxQY3IyS0NKeXhYdXlw?=
 =?utf-8?B?TlZPNWJWd1JCZE43d2MzS2xub1RsNlpYU3ZmZU1XTFNsY254QnlDV2djMmZB?=
 =?utf-8?B?VnEvMzBLLzJhZVlQZVh4VGQwdURPdjNjeXdKajlRM3VrQVpLS0p2SlQweEZv?=
 =?utf-8?B?MER2SFRNZlBsMC9lOUJsRks3ZUFRMkRISFhkUGJOVkhrOTc4Q0JFa1Nqa3NM?=
 =?utf-8?B?RXgxdllnY0ZIRTFPU0lXUWUvK3lDbVZTMzhIV2dhMzFKKy9IKzBQdG16WjlB?=
 =?utf-8?B?TzdJTVVlSmg1N0hrbXVmeEJsdXdpRUZmY1ZPeWVBMk5wQm5NTTlBUGFyTC9u?=
 =?utf-8?B?MkVBQk5LeGMySVQwQ3V4bUgvNCt5MUd0dUtUWkRmbk5xNEtDRmU2bFBrTnlh?=
 =?utf-8?B?bjlLYTdVc1N3SnJZVE5qc0pBeWtEWDFqcUFrUDNkRE9yenFKcWl2ME9pN2RR?=
 =?utf-8?B?ZGdlUDZ1d3RGdHBsdE9EZGw0Vmp6ZGNEcnVZQWNKa2RLeUdJWDA5WFVCWTJi?=
 =?utf-8?B?a0FsL2VyUEpERUFBRkd0OWpDamhRS0F3UDdyTnZoTW5ndFVkNjFuY1h4Vjl6?=
 =?utf-8?B?OHdNNFNHUDh1aEZMeWJQVFUxbWoxcXMyREdxTVJsVTJqTHBOM0FINGxmOTc4?=
 =?utf-8?B?YUNJaVJVelFnL1NwdnpzbTFWTHVTWWVVYjBmL1ZkRDBHMVNHQ3F3a0t6OCs4?=
 =?utf-8?B?NzM2TmtiZ2hHWEdFK3FCQmNSdmMxaFRrSkVKK3FWeVJEdjZ6dXhtMmFOZnFJ?=
 =?utf-8?B?VGVjbzFsYVlhckltOHRQQng4RnIzQlpHNGVBZlpUQlZHTE9wMm04MjFIQmh6?=
 =?utf-8?B?eFE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0b9502e1-b147-4e54-bee7-08db148199f4
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4855.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Feb 2023 03:05:07.1378
 (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: gL/h5uy7EKvKgvuHj8kX4XQbLgjXFFe9/7S8Vz07nExL4LtBm2DSHKIqWR/PkthKj+OFlZOHgs7mMVAnXBr0asByTJNuLvaCABgPdjZAnWo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4945
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2/21/2023 8:36 AM, Mingwei Zhang wrote:
> Avoid getting xstate address of init_fpstate if fpstate contains the xstate
> component. Since XTILEDATA (bit 18) was turned off in xinit, when KVM calls
> __raw_xsave_addr(xinit, 18), it triggers a warning as follows.
> 
> __raw_xsave_addr() is an internal function that assume caller does the
> checking, ie., all function arguments should be checked before calling.
> So, instead of removing the WARNING, add checks in
> __copy_xstate_to_uabi_buf().
> 

<snip>

> @@ -1151,10 +1152,11 @@ void __copy_xstate_to_uabi_buf(struct membuf to, struct fpstate *fpstate,
>   			pkru.pkru = pkru_val;
>   			membuf_write(&to, &pkru, sizeof(pkru));
>   		} else {
> -			copy_feature(header.xfeatures & BIT_ULL(i), &to,
> -				     __raw_xsave_addr(xsave, i),
> -				     __raw_xsave_addr(xinit, i),
> -				     xstate_sizes[i]);
> +			xsave_addr = (header.xfeatures & BIT_ULL(i)) ?
> +				__raw_xsave_addr(xsave, i) :
> +				__raw_xsave_addr(xinit, i);
> +
> +			membuf_write(&to, xsave_addr, xstate_sizes[i]);
>   		}
>   		/*
>   		 * Keep track of the last copied state in the non-compacted

So this hunk is under for_each_extended_xfeature(i, mask) -- it skips 
the copy routine if mask[i] == 0; instead, it fills zeros.

We have this [1]:

	if (fpu_state_size_dynamic())
		mask &= (header.xfeatures | xinit->header.xcomp_bv);

If header.xfeatures[18] = 0 then mask[18] = 0 because 
xinit->header.xcomp_bv[18] = 0. Then, it won't hit that code. So, I'm 
confused about the problem that you described here.

Can you elaborate on your test case a bit? Let me try to reproduce the 
issue on my end.

Thanks,
Chang

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/xstate.c#n1134