Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <c9401dbf-88a0-404d-a8d3-33f0e712cda3@intel.com>
Date:   Fri, 17 Nov 2023 16:33:27 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 3/9] KVM: x86: Initialize guest cpu_caps based on guest
 CPUID
To:     Sean Christopherson <seanjc@google.com>
CC:     Paolo Bonzini <pbonzini@redhat.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Maxim Levitsky <mlevitsk@redhat.com>
References: <20231110235528.1561679-1-seanjc@google.com>
 <20231110235528.1561679-4-seanjc@google.com>
 <c9f65fc1-ab55-4959-a8ec-390aee51ee3a@intel.com>
 <ZVaXroTZQi1IcTvm@google.com>
Content-Language: en-US
From:   "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <ZVaXroTZQi1IcTvm@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?dzZkTjVBTXU5RlVlTFp2SVAvb21EL0VKb2J4RC9EaHhtOUZod09GRjluTlFZ?=
 =?utf-8?B?VGY5OHFhaWNBalcwYnorTGJ4eEZMdWhQaTlWazgzaDZUUXFwWjBvZ1l3aHRh?=
 =?utf-8?B?NjlRQlVYZHFtMG9lemkzOGw2NG9ONVhnMDlqbGFvc0lpQVhnMXhSdXZ6Nmxz?=
 =?utf-8?B?ZFpPbXNLZUMvVzc5b0tPNFYxWjFjNXdiTmtKYXhNZnE2d1dlSmpvQkhacXVo?=
 =?utf-8?B?NjN5ampBOXJzbCtMNnJEREp6endXRTViSVB4Y0lzcGdpUnFoWkdrSVN0czlL?=
 =?utf-8?B?dVpZUk9QZFQwZi8yNTFGbjQzOTlzRkxqalFZK0pycDBUbDR0Z0xoLy94eS9x?=
 =?utf-8?B?UVg5bEhMcUdMa05jS1N2YlBGWFlET0ozWlkrdnlyK1V2bW11TENZVS9GSTZZ?=
 =?utf-8?B?RXhnaE5CUjhjOEpsTytFbCsvOFBKMkNTVHMvakxpeFBtYkRrWHJHNzRKS3F3?=
 =?utf-8?B?STByNVV5RHA4ZkJObnY4NHZ4U1V4TjhiZktmWXgweXRzcmVwTFFRUXAvM2xW?=
 =?utf-8?B?U1NIRWhLamhaU3RZMUhpSVV1SXZNblR5ZGpMTTllNTBEYStlR0ViYjZyMEZR?=
 =?utf-8?B?THBZK1pIQmhlbUVFMStWa2Y0UDNaczMxRHY1eklNRDBZVUY1S21qdXU1WU9y?=
 =?utf-8?B?bmxNRS81SCtYNVcyYmhtN1ZMaWdwYVlRS0xRN2dSWHVPYVJXZkVUL3d6YVhh?=
 =?utf-8?B?a2NYYXBHSXB3YXZpZlE2SGRNbU5nRlIveVNBZlUvWFJjQkc2UGZNWHZkWTNr?=
 =?utf-8?B?bnlEbmdaTnYwbE1uOVFqM092cjM3Z3Y0SUVmUXk3djVjUENBaW1SR3ZYZFN3?=
 =?utf-8?B?cyszNU8rZ0QzT2ZNaFdSRThCNXFVSzRMRmdUVGJrTmhvM0RNRFowbUtYTjFs?=
 =?utf-8?B?YmFUcmFoYjdNNkpOZTl5WUJJL3YwMGdFWStSa0FmYms4a1lZM0g3cm1VU3Bt?=
 =?utf-8?B?N01pby9vVXpWUWZCclczZ2NpYVZFLzlwb0grK3k5UWMwOUhnUGJUbTNZSEY4?=
 =?utf-8?B?U2FySTVlMS9Sc2FOR05RQ29HQ3FYUTJ5a29zclRvd2twK3o5KzRHWjZJWlBX?=
 =?utf-8?B?SEY5Q1lLYWR6RVhaQzhJRk5XSEx3Um5LQzNrM1pBVVJaeHpGMkVGNzgrMFBw?=
 =?utf-8?B?eHFvMGl2dkM5K1VRSGhwMnh6WVRlODVVRWtiUXhPelloUVd4WWJmaHZBNFZG?=
 =?utf-8?B?VGJvTU1WYXEyRS9HNWdhdGN0UXJWWkJsL0JubVl2akR3WlFoZEc2cnVVSlRq?=
 =?utf-8?B?SE13TUNkRTk1SHBoZTZBbFlRdHl3b0VjUzdMTzNkblpoalhsbjh0SllKR1lG?=
 =?utf-8?B?RmFlb3RUNkhwUjQ1RlBmcGtFcEY3RVFST3RPdkV6UENVS0R5TjZqYWFZSWVI?=
 =?utf-8?B?K0dmdHBMdXp1UTdBWVR3dU9vbzF6SGpPSkVTYzBmeE1FWTE1N0oxcng1aWs5?=
 =?utf-8?B?OVpqVG9aK01DbXphV05JbE9MeEgySGtCZjJ3NFdPeWVmOW15cktTLy85Q2ov?=
 =?utf-8?B?cUdCeHRiMVRFV0ZmQjFidVQ2K1RCS3d0UHdWV1pWWkVibkkyL1NFL1VKSXVh?=
 =?utf-8?B?YkdoMy9zT0NMdTFIZXkxY2VzRWZGcXh3eWpId1dPN09mUmFvL2crdC9yTUFN?=
 =?utf-8?B?d0FMSDZ0V2daZzBXY3F0aWlVY3R4SU5TUytHRmY0dUFIY2IzSDBubjlPUHdw?=
 =?utf-8?B?UjRrd1RCQ3hvVjNnc25UNk40QTZpR1dYTHRVZVFDU1RIelo1LzA1L1NTdDdD?=
 =?utf-8?B?WWhSckxWa0RoNHB1S0x1aDZ2UUNLdGt4U05VNUQwb0RvMW9XRlZTOVVUVHlj?=
 =?utf-8?B?ZUJBZG5lbUsyUStmQTQ1OWZZYjQ1SlIwS0FEdjdHYTY3bFQrdHorZUZpcElj?=
 =?utf-8?B?dFFBbDI1dE40NXBoZy9ld1VPSkZJNnpkWlZKU05Qa2FzMWJLelA3amlqVHZM?=
 =?utf-8?B?U01TSmgwdy9ncWtPNmZOS3c1NTNaeHg2SksrQ21EQ2FmOGdyZE93V0lVaFNh?=
 =?utf-8?B?MTRWTzQ2ZTZYckRxY25FajBleGdLY0tIMEVPL2puSmpxTlFsVDBhczFvRVpJ?=
 =?utf-8?B?STFCamw3YVp4cEduMmFFTlgyRmFuVjVheGkzejRQTkFMc0M4YjREcXZUWU15?=
 =?utf-8?B?cmpwYlc4Q3NjWUxQbHUxL2ZBZmpCK1VUWDNRWVVWZ0FwcXVjTGpURDIySjRl?=
 =?utf-8?B?dUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9293818f-107b-44b1-adcc-08dbe747e35d
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Nov 2023 08:33:35.1356
 (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: dD8aa1eYTGFsJUDHO7GW/ukvEgs5R0tRypan4tBNO+U9mOKE+AU+erpxEbQhAN8/UeTwP1ZQrwHEIJbiSU5KHg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB8398
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE,
	URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.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 (agentk.vger.email [0.0.0.0]); Fri, 17 Nov 2023 00:33:56 -0800 (PST)

On 11/17/2023 6:29 AM, Sean Christopherson wrote:
> On Thu, Nov 16, 2023, Weijiang Yang wrote:
>> On 11/11/2023 7:55 AM, Sean Christopherson wrote:
>>
>> [...]
>>
>>> -static __always_inline void guest_cpu_cap_check_and_set(struct kvm_vcpu *vcpu,
>>> -							unsigned int x86_feature)
>>> +static __always_inline void guest_cpu_cap_clear(struct kvm_vcpu *vcpu,
>>> +						unsigned int x86_feature)
>>>    {
>>> -	if (kvm_cpu_cap_has(x86_feature) && guest_cpuid_has(vcpu, x86_feature))
>>> +	unsigned int x86_leaf = __feature_leaf(x86_feature);
>>> +
>>> +	reverse_cpuid_check(x86_leaf);
>>> +	vcpu->arch.cpu_caps[x86_leaf] &= ~__feature_bit(x86_feature);
>>> +}
>>> +
>>> +static __always_inline void guest_cpu_cap_change(struct kvm_vcpu *vcpu,
>>> +						 unsigned int x86_feature,
>>> +						 bool guest_has_cap)
>>> +{
>>> +	if (guest_has_cap)
>>>    		guest_cpu_cap_set(vcpu, x86_feature);
>>> +	else
>>> +		guest_cpu_cap_clear(vcpu, x86_feature);
>>> +}
>> I don't see any necessity to add 3 functions, i.e., guest_cpu_cap_{set, clear, change}, for
> I want to have equivalents to the cpuid_entry_*() APIs so that we don't end up
> with two different sets of names.  And the clear() API already has a second user.
>
>> guest_cpu_cap update. IMHO one function is enough, e.g,:
> Hrm, I open coded the OR/AND logic in cpuid_entry_change() to try to force CMOV
> instead of Jcc.  That honestly seems like a pointless optimization.  I would
> rather use the helpers, which is less code.
>
>> static __always_inline void guest_cpu_cap_update(struct kvm_vcpu *vcpu,
>>                                                   unsigned int x86_feature,
>>                                                   bool guest_has_cap)
>> {
>>          unsigned int x86_leaf = __feature_leaf(x86_feature);
>>
>> reverse_cpuid_check(x86_leaf);
>>          if (guest_has_cap)
>>                  vcpu->arch.cpu_caps[x86_leaf] |= __feature_bit(x86_feature);
>> else
>>                  vcpu->arch.cpu_caps[x86_leaf] &= ~__feature_bit(x86_feature);
>> }
>>
>>> +
>>> +static __always_inline void guest_cpu_cap_restrict(struct kvm_vcpu *vcpu,
>>> +						   unsigned int x86_feature)
>>> +{
>>> +	if (!kvm_cpu_cap_has(x86_feature))
>>> +		guest_cpu_cap_clear(vcpu, x86_feature);
>>>    }
>> _restrict is not clear to me for what the function actually does -- it
>> conditionally clears guest cap depending on KVM support of the feature.
>>
>> How about renaming it to guest_cpu_cap_sync()?
> "sync" isn't correct because it's not synchronizing with KVM's capabilitiy, e.g.
> the guest capability will remaing unset if the guest CPUID bit is clear but the
> KVM capability is available.
>
> How about constrain()?
I don't know, just feel we already have guest_cpu_cap_{set, clear, change}, here the name cannot exactly match the behavior of the function, maybe guest_cpu_cap_filter()? But just ignore the nit, up to you to decide the name :-)