Received-SPF: pass (google.com: domain of linux-kernel+bounces-149391-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <2fc900b8-4028-4fc2-a83e-a70ef8d7b37a@oracle.com>
Date: Wed, 17 Apr 2024 21:16:17 -0400
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/2] KVM: x86: Only set APICV_INHIBIT_REASON_ABSENT if
 APICv is enabled
To: Sean Christopherson <seanjc@google.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, linux-kernel@vger.kernel.org,
        joao.m.martins@oracle.com, boris.ostrovsky@oracle.com,
        suravee.suthikulpanit@amd.com, mlevitsk@redhat.com
References: <20240417200849.971433-1-alejandro.j.jimenez@oracle.com>
 <20240417200849.971433-2-alejandro.j.jimenez@oracle.com>
 <ZiBPHVKKnQPYK7Xy@google.com>
Content-Language: en-US
From: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
In-Reply-To: <ZiBPHVKKnQPYK7Xy@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?aXh3VjVYWlNSNjNhRjQ3bi93SjdyYlB5MThDb0tDUS9oQWRKaGJGeS9FcWtI?=
 =?utf-8?B?NC9sR1MzWWd5U1pya2R1TERiZkRudElvOFczTHRsU0hxMzdoODBHRFMzbE0z?=
 =?utf-8?B?eTM3cDRDU2hLQXFVcm82aVdZcnZ0Y1Z0NHlLWnNMK2RSYzJzR21BVWs0Y3JT?=
 =?utf-8?B?UTZDRlE1bGhoMXJsczRsVENuVzRhbmVtcndCcGNHTWdiYWdNT2llSnMwRUxN?=
 =?utf-8?B?MW0rWW9lUXhHSDlIZDk5YmZwT20wT3ZTZTNkYTBiZGRRVGp3YURQZnpmcUJW?=
 =?utf-8?B?dGFTYjdsbTRNWGo1cTgyMG05Vk5MbGw4T1VHUlA5bFVoV3lZV1ZVTDVhN1dH?=
 =?utf-8?B?YXpScU1PL1FjeDM3cGl1SC8yc21MTGZXZTgvdVU1d0tTL0VPKyswVlJiVkh0?=
 =?utf-8?B?bGxieGlXbEhROS9YVGhQWnRabzdxdGNGTWlTRUNybEplNTFmYllham4xdCtG?=
 =?utf-8?B?ck1ySVRvMTE5MXJ2Wkk2Z0hvWjMrMVoweE92VmYxWUNiczhiTmZoQ2VJcXlQ?=
 =?utf-8?B?TFNxVjVndHZ5R2srR05LUk5aM2poTDFVN05rTVF5bkJJTFBpb0p1Wm56Ym40?=
 =?utf-8?B?bzVYYVN1bDhrVktSV2tSN01BY3FJOXZFTGE2TjEwRHU0dTZ6Z2hLa0xGSDUw?=
 =?utf-8?B?bWl1MHR1WlhRN3U0ZTYxaU5SQ005emlXR1NzSmczWEswYjhpUDBnZUUzNG50?=
 =?utf-8?B?RHYwMDNicUNtaTY1R1I0ZFVXL3FnYjNKSlFQUFZrLzcybFpsejlENGtMZ3lI?=
 =?utf-8?B?Ly9QazFrWHlTeFlKTnlPMmlWTW1WTUxOSUlJckVNNWZZTEtvUVhMeVB6eFJy?=
 =?utf-8?B?dk5EVUVyRUl0TnlPVkRaZklHVERmNUlzbmVMZERLZDFLMzF6V1pQZWlQN0M3?=
 =?utf-8?B?c1dHVEVTeCtoa1N3MW12Yld0ejVvYmFnM3VKdnhDNTdDQ1RlRytDYys5ci8w?=
 =?utf-8?B?SFBOYTRpb0pSS2xuT09zem9rdXN3SUFtSmxjc1JtNFk4YkRrMi9jSVRpSmZy?=
 =?utf-8?B?T1dselIwTS9mNlBuWTdtOFoyc2xYeXRMM0ZvVThNMDYvOHc3UlJmYzlGWTVS?=
 =?utf-8?B?UUdyU3FydUl0aGdZTHZJM3c5RXhob1ZqbmZpaGowSzVTL0hpVjE2dFl3Z0VC?=
 =?utf-8?B?YUZnOFI4WGNjU1RTbnlCZjI5R0RUSXZoaWV6RWlwTjBEUDFIOXk2QzZNKzNS?=
 =?utf-8?B?U0djMXVzNmd6SzR1M3ZvaExHclJJb2M1RXN1YldRSm52SmNIMTJnbG4yY1Yy?=
 =?utf-8?B?K1A2MjAwN1JoTDcxcDBEcjlMSmtNTkVGbVFENkg5Y2UvS2ZjQjk2bkdGbW9C?=
 =?utf-8?B?Mlplc1dYT3VaSUxOdDJOYjVsL3FmNTNxajlpMFpDNVNLTmRnQzFKTDlRakFy?=
 =?utf-8?B?Y3psR3dNQlplYitEZERucVRQWXA5UDJKSUZnUERxS1hiUUlLSGt2cU02V1Jx?=
 =?utf-8?B?S2pQRDQ0c2xZV3JKamg2eHhLRkx5M2VvT05qUTQzNlNveDRENVdCeCswVGNl?=
 =?utf-8?B?VEFIYzYzTW5IbWxNaVdSV3kzZTNJN3R4Z0g3ZmQ4VFZGSjd4UXRzOWdQVmJY?=
 =?utf-8?B?OUhkMm1jRmFtcXhOaGNqYkg4RXdXQWtVL0lUQ09MUk5GT05ONlIrVlhZemE0?=
 =?utf-8?B?WXpmVlpJbi9SZVJ5dmxick1teHNha3U4aXJTbWxrQjluQ2pYMlRVUjZGeGoy?=
 =?utf-8?B?YXA1VVROWWVHaHVXdURrclc1dEQvK2lqYmhOb2p6dVdqb0RVTC9LUnZUT0Rj?=
 =?utf-8?B?NVRGVHZYRVhxajRVc1E5TkI2N016WlN0alNTTXFYMW55NnhnbThKQWhCcllk?=
 =?utf-8?B?N3pPWTFKaTJyUzNWTy9tVmc2MTN4R01JMXg5ejFndWxsSGxEb0tQOWl1TWF3?=
 =?utf-8?B?MmhuUXFCS05KdHF1RnNpVFFhcUY4eDNPeXo3UXpVaEZhM0poTHBkSnNES3dW?=
 =?utf-8?B?VEY0aDNDNEpxemZQUktoaDNncWcxQkg5QUVxckZPbkRzUTFVZzcwcE5aY1BU?=
 =?utf-8?B?QXIvRVdzSE5EZWxlRnpPOC80WnZnNUZHVFRaSzM1N1UzVDZUWUw3eVVndGNx?=
 =?utf-8?B?OHR4aXJnQzFYckJad3p0MVFxOWorc281ajBzUS9xMTNOUVhlMHQ4ZHY5WVRH?=
 =?utf-8?B?cHlVNDA0aDVlUFllbldaYmloRmkwcTRmMlAzUEJGZXJpemxITTJLM1p5bWJJ?=
 =?utf-8?B?a3c9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	ZcecbMet6jydCVnC7I5d/RieEk36maK6g/sOnPOVB7crO9cCwWQW1Iic7IahIVhvV6fA5XZM+kzNCOGCl5ChxnS4k+/YPhXDar3t0sm3RiJDmn6sGVwkRng4zjgZNeAyOmvABmJLng7OjJyktBb4UYnDDwIoi3e/y+VFjb2VbiteJkCMHygM70GJqeY2AKPYu3IsZTvPciZLkgehIJnXlEuMuEK2F02CLgPRABqAZEMho/nchAGp7mlpoFKNVKKLqdXskuf8NODWYLTqzYzmmcNdxLD3tjpFEmglJmACuGmpWTkGbPVBeECbzREdxXw8Cj5/v67xwOvN5CFU6w96VlDkftChSGT7D5dVi36CS4BVXrz+/xZVABwPqXqHO3Y8HNeAQiSXrDWIC7m6yMaGcnYwxBAhKhRqaciqvoVLwGDo2ZZtvGiDOTO0vMiIIjmRgGJLFZDrl2JWoGqLhwuAqwrePoTPWKIGy7bTh1NGMrp3rSvy2vhMcKA+2IvyKvk07egKRr23wb2ZdPR5Wodwhjdj5camzjR9o4Bq9a1rgWlxT1V50T7DLHWhBt71tP4rUfmrrpYF4xCk1nBlYYlnW0M2tSD5sNbJpCO+qiARCHk=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d11c28e0-e89d-45aa-62a6-08dc5f45283e
X-MS-Exchange-CrossTenant-AuthSource: DS7PR10MB5280.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Apr 2024 01:16:21.4373
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: dEASSdmcVKre6Z8t6J7DK1omdUMgOEL4lBHqt5a42Ih3IMQ1ZvX5FAE43VI+N0HCnNdNAX4TDb0xUHT2Fn2GUVDkRPsmdkUlPMn+LwZbyJ0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR10MB6307
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-04-17_20,2024-04-17_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 spamscore=0
 mlxlogscore=999 phishscore=0 mlxscore=0 malwarescore=0 bulkscore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2404010000 definitions=main-2404180007
X-Proofpoint-ORIG-GUID: t0GEhRXGW5ZYaLKKYF2jrRDjODyZGXaj
X-Proofpoint-GUID: t0GEhRXGW5ZYaLKKYF2jrRDjODyZGXaj


On 4/17/24 18:37, Sean Christopherson wrote:
> On Wed, Apr 17, 2024, Alejandro Jimenez wrote:
>> Use the APICv enablement status to determine if APICV_INHIBIT_REASON_ABSENT
>> needs to be set, instead of unconditionally setting the reason during
>> initialization.
>>
>> Specifically, in cases where AVIC is disabled via module parameter or lack
>> of hardware support, unconditionally setting an inhibit reason due to the
>> absence of an in-kernel local APIC can lead to a scenario where the reason
>> incorrectly remains set after a local APIC has been created by either
>> KVM_CREATE_IRQCHIP or the enabling of KVM_CAP_IRQCHIP_SPLIT. This is
>> because the helpers in charge of removing the inhibit return early if
>> enable_apicv is not true, and therefore the bit remains set.
>>
>> This leads to confusion as to the cause why APICv is not active, since an
>> incorrect reason will be reported by tracepoints and/or a debugging tool
>> that examines the currently set inhibit reasons.
>>
>> Fixes: ef8b4b720368 ("KVM: ensure APICv is considered inactive if there is no APIC")
>> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>> ---
>>   arch/x86/kvm/x86.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 26288ca05364..eadd88fabadc 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -9999,7 +9999,20 @@ static void kvm_apicv_init(struct kvm *kvm)
>>   
>>   	init_rwsem(&kvm->arch.apicv_update_lock);
>>   
>> -	set_or_clear_apicv_inhibit(inhibits, APICV_INHIBIT_REASON_ABSENT, true);
>> +	/*
>> +	 * Unconditionally inhibiting APICv due to the absence of in-kernel
>> +	 * local APIC can lead to a scenario where APICV_INHIBIT_REASON_ABSENT
>> +	 * remains set in the apicv_inhibit_reasons after a local APIC has been
>> +	 * created by either KVM_CREATE_IRQCHIP or the enabling of
>> +	 * KVM_CAP_IRQCHIP_SPLIT.
>> +	 * Hardware support and module parameters governing APICv enablement
>> +	 * have already been evaluated and the initial status is available in
>> +	 * enable_apicv, so it can be used here to determine if an inhibit needs
>> +	 * to be set.
>> +	 */
> 
> Eh, this is good changelog material, but I don't think it's not necessary for
> a comment.  Readers of this code really should be able to deduce that enable_apicv
> can't be toggled on, i.e. DISABLE can't go away.

ACK, I'll remove the comment block.

> 
>> +	if (enable_apicv)
>> +		set_or_clear_apicv_inhibit(inhibits,
>> +					   APICV_INHIBIT_REASON_ABSENT, true);
>>   
>>   	if (!enable_apicv)
>>   		set_or_clear_apicv_inhibit(inhibits,
> 
> This can more concisely be:
> 
> 	enum kvm_apicv_inhibit reason = enable_apicv ? APICV_INHIBIT_REASON_ABSENT :
> 						       APICV_INHIBIT_REASON_DISABLE;
> 
> 	set_or_clear_apicv_inhibit(&kvm->arch.apicv_inhibit_reasons, reason, true);
> 
> 	init_rwsem(&kvm->arch.apicv_update_lock);
> 
> which I think also helps the documentation side, e.g. it's shows the VM starts
> with either ABSENT *or* DISABLE.
>

I initially had combined the checks (using a less elegant if/else), but didn't want
to convey that these two inhibits were mutually exclusive. But as you point out
that is exactly what REASON_DISABLE is with respect to all the other inhibits.

I'll send v2 with the changes.

Thank you,
Alejandro


>> -- 
>> 2.39.3
>>