Received-SPF: pass (google.com: domain of linux-kernel+bounces-179227-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <1dbb09c5-35c7-49b9-8c6f-24b532511f0b@intel.com>
Date: Wed, 15 May 2024 10:41:07 +1200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/4] x86/reboot: Unconditionally define
 cpu_emergency_virt_cb typedef
From: "Huang, Kai" <kai.huang@intel.com>
To: Sean Christopherson <seanjc@google.com>
CC: "pbonzini@redhat.com" <pbonzini@redhat.com>, "kvm@vger.kernel.org"
	<kvm@vger.kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>
References: <20240425233951.3344485-1-seanjc@google.com>
 <20240425233951.3344485-2-seanjc@google.com>
 <5dfc9eb860a587d1864371874bbf267fa0aa7922.camel@intel.com>
 <ZkI5WApAR6iqCgil@google.com>
 <6100e822-378b-422e-8ff8-f41b19785eea@intel.com>
Content-Language: en-US
In-Reply-To: <6100e822-378b-422e-8ff8-f41b19785eea@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UmNNc1RBZTc3amlSWVNIZUk0ZVJhMThjdTd5bmhwN3FGNGM2SFF6dU1kMzBp?=
 =?utf-8?B?NEtZMzBPT1NTWFBLQW1rcDFpTkdZa2JVSzJUWnIzZHlFV1RCU0Rlcy9BTWFS?=
 =?utf-8?B?dExMR1RQNHpXZUZWdWgrWGtSOHorTkltWTBWZ2ptY3hQalZrZmlablpwbVAw?=
 =?utf-8?B?UHdYRGdzNlBTaWkzYktwU2p6cEFnQkhsWlk1OWpBQnJxc2lTeGZtSDNDWnFB?=
 =?utf-8?B?OWhaUHlUZmh5dk5LQUEzczJOQk1Sbk4yNTQxczRKUmpxMGNxelFsNHZTczVK?=
 =?utf-8?B?WEl3Y3UweUpTVjNMZlpqaFA0dHR4ODgrR0I3U0VqZXlweGc3cUlVMk81NEJk?=
 =?utf-8?B?RzdYUm9qWElNa3piMk5sTTNZOEpWV3lZTzNVK3BycXlhMVpsekgzY2h2eWoy?=
 =?utf-8?B?VnV5UHpXMXhIQkFhczZDL00ySnAvWnVPc2Q0TzhUbXNYemN4bGRkbU9la21a?=
 =?utf-8?B?RVpSR0U3K3hEbzZsYXBhK0dZQ25NbXJCTUFKODZldXVMakFrc2xoN05OT2Zu?=
 =?utf-8?B?dzdTMFFlaUpLSi9YSGxoakJnMEI3d2VGbVpUZE1xd1RGeC85cDZKMTd5b3Zp?=
 =?utf-8?B?V1pLTlVMMU1JbUVEditKZzMzMmZTTFh0L0c0KzdWM1BRdlZQVU91U2d6MFd2?=
 =?utf-8?B?MWJCdHJCeVkzdmdoUHg4dWllU0JBZU5TRnIybU9zTlJBNWZtZUhxWEE4WVNj?=
 =?utf-8?B?VWpHNUlPdTd1ejFxY1JaVUZ2NVF5OG9vNXNacXpiY2I3WGx2ekFneXFDdEsr?=
 =?utf-8?B?VTdORFhQVHViOEN2VkdsWll1VktvTFc3cytwekJpRVJSNWpDakt4UE5HbGUr?=
 =?utf-8?B?MGNFemhlMVJqcVNaSjVvNTJvU3lGL2lTdHFlNHJYZmI4RDlHaFpYd2hvQUtY?=
 =?utf-8?B?cVdTMHhGc1phMmdMalRSU05Gczg4bEgrN1FBVDJSeUtrSU1HSGsrazBTaFRZ?=
 =?utf-8?B?Y05RUjVWMVo1dFZiL0VjRVBpWDJHZWVYeGcybS9mSThCNlllSDA2SlVMY2po?=
 =?utf-8?B?bDF1NVhNdElGRVJHMVNwOUNNdklzaEFwTmovdStiVmxZaHBqc3g1Mm80RlUy?=
 =?utf-8?B?RDFVTzBXeWd0SnhrbElVVEpXUThRTEtXMDRXcGxkU0tKamcvdUVHMjZmQzN2?=
 =?utf-8?B?cFEvTjVpS0xsUVc0MU54aUExNFN2UzJSV2V3OXVESDVIWWJ1ZnZjN0Z3Ri82?=
 =?utf-8?B?TGlZd0FMWndNalZaS1ZCME5SUzRYNmhqckE5bEpVS2FOWmxORDZUSEN3T0I1?=
 =?utf-8?B?S3NuQlhVbHlNMjV6UTEzcVo5eEJEWUl4cUI4WThSdFh5TnI5T1lQbHNSNEw2?=
 =?utf-8?B?ZDZDOXJKZ0lsMHhEalRpS0RIeVdmY2l2dUUwQmgzWjJPZU9jeWtuTTN2QnlE?=
 =?utf-8?B?YVdmSnRld2Q1eUlOSHZVQWVtWDZTL0drenFYNko4aDd2RVhIODF4eWl1L0pK?=
 =?utf-8?B?UkJRWjJhV3NZWmVTUUhiWm5qbkErOEc4NVMyS1poTnI3R3dsUWZXa0lEQzB0?=
 =?utf-8?B?aWpWZ2x1azNTRWJwWGZqVHhBaWFlVnBKblBYKzFyUzJFeTQ3N2poTDNQeFdy?=
 =?utf-8?B?b0x4T3BwWkJIS1o1blAyWXNiTldIbGNYQlFGSjc4YUgrWVM2elpzNFEyUE04?=
 =?utf-8?B?MEx1VDRNWXV3aHBRNk9QUjlOSzdOTWhLQlZybC9MUDN1YWh0Sk05Mm9YVE84?=
 =?utf-8?B?N2ZQRURmNzZtTzRtalFhbXpjTFJHMzc2U1RjaDc0Wkl5UFY0MEFscnVtbHBK?=
 =?utf-8?B?ekx2YmFmVGhFd0lGNEhuV2h3cFRUeXdSckhpTHBaT1RPTkw1SEFDUGdENDBF?=
 =?utf-8?B?YjBUVi8rUGFDRkJ0VU90OUhYeDhEQXBPdmZqNWVRTWZJQ2R0Y1NPQm5pUkZV?=
 =?utf-8?B?TzVpeFg1TDB1UmhiNDRVOGtYcXlnQlFtaDdBcWZhUzlOeGdnc2phcHhtY3hR?=
 =?utf-8?B?NlI5ek1PblVLNkRyN24rNlJkYTdKdWhtUkFBaTZMNHhNc1N1MmFZM09WTGVr?=
 =?utf-8?B?WTNSNllDaXh5WnlMUW5kK2R1QUpFMGF5VXpOSU5nUDhDNVhZdmo3RFlrb1Zi?=
 =?utf-8?B?UVJwSUI2YW0rdFBid0xjSWw5cHc0UHhPWUdJWGY1YjY3cUFlSGYzcHRzVDhF?=
 =?utf-8?Q?lpK6yerROyAp2qOMmFib6k7mk?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 84ebba5a-2b9e-497f-389d-08dc7466f5b9
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 May 2024 22:41:13.9352
 (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: u33p1PNVBbLPYtGogRohtiut8OlsURN695cVqGHwCh5ajojmhEd6lT2M4iPZhnFNTIt1av8tz99bNgWIdCQ6RA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4949
X-OriginatorOrg: intel.com


On 14/05/2024 10:44 am, Huang, Kai wrote:
> 
> 
> On 14/05/2024 4:01 am, Sean Christopherson wrote:
>> On Mon, May 13, 2024, Kai Huang wrote:
>>> On Thu, 2024-04-25 at 16:39 -0700, Sean Christopherson wrote:
>>>> Define cpu_emergency_virt_cb even if the kernel is being built 
>>>> without KVM
>>>> support so that KVM can reference the typedef in asm/kvm_host.h without
>>>> needing yet more #ifdefs.
>>>>
>>>> No functional change intended.
>>>>
>>>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>>>> ---
>>>>   arch/x86/include/asm/reboot.h | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/include/asm/reboot.h 
>>>> b/arch/x86/include/asm/reboot.h
>>>> index 6536873f8fc0..d0ef2a678d66 100644
>>>> --- a/arch/x86/include/asm/reboot.h
>>>> +++ b/arch/x86/include/asm/reboot.h
>>>> @@ -25,8 +25,8 @@ void __noreturn machine_real_restart(unsigned int 
>>>> type);
>>>>   #define MRR_BIOS    0
>>>>   #define MRR_APM        1
>>>> -#if IS_ENABLED(CONFIG_KVM_INTEL) || IS_ENABLED(CONFIG_KVM_AMD)
>>>>   typedef void (cpu_emergency_virt_cb)(void);
>>>> +#if IS_ENABLED(CONFIG_KVM_INTEL) || IS_ENABLED(CONFIG_KVM_AMD)
>>>>   void cpu_emergency_register_virt_callback(cpu_emergency_virt_cb 
>>>> *callback);
>>>>   void cpu_emergency_unregister_virt_callback(cpu_emergency_virt_cb 
>>>> *callback);
>>>>   void cpu_emergency_disable_virtualization(void);
>>>
>>> It looks a little it weird.  If other file wants to include
>>> <asm/kvm_host.h> (directly or via <linux/kvm_host.h>) unconditionally 
>>> then
>>> in general I think <asm/kvm_host.h> or <linux/kvm_host.h> should
>>> have something like:
>>>
>>>     #ifdef CONFIG_KVM
>>>
>>>     void func(void);
>>>     ...
>>>
>>>     #else
>>>
>>>     static inline void func(void) {}
>>>
>>>     #endif
>>>
>>> But it seems neither <asm/kvm_host.h> nor <linux/kvm_host.h> has this
>>> pattern.
>>>
>>> I tried to build with !CONFIG_KVM with patch 2 in this series, and I got
>>> below error:
>>
>> Well, yeah.
>>
>>> In file included from ./include/linux/kvm_host.h:45,
>>>                   from arch/x86/events/intel/core.c:17:
>>> ./arch/x86/include/asm/kvm_host.h:1617:9: error: unknown type name
>>> ‘cpu_emergency_virt_cb’
>>>   1617 |         cpu_emergency_virt_cb *emergency_disable;
>>>        |         ^~~~~~~~~~~~~~~~~~~~~
>>>
>>>
>>> Looking at the code, it seems it is because intel_guest_get_msrs() needs
>>> 'struct kvm_pmu' (e.g., it accesses the members of 'struct 
>>> kvm_pmu').  But
>>> it doesn't look the relevant code should be compiled when !CONFIG_KVM.
>>>
>>> So looks a better way is to explicitly use #ifdef CONFIG_KVM around the
>>> relevant code in the arch/x86/events/intel/core.c?
>>
>> Eh, there's no right or wrong way to handle code that is conditionally 
>> compiled.
>> There are always tradeoffs and pros/cons, e.g. the number of #ifdefs, 
>> the amount
>> of effective code validation for all configs, readability, etc.
>>
>> E.g. if there is only one user of a function that conditionally 
>> exists, then
>> having the caller handle the situation might be cleaner.  But if there 
>> are
>> multiple callers, then providing a stub is usually preferable.
> 
> Yeah.
> 
>>
>> IMO, the real problem is that perf pokes into KVM _at all_.  Same for 
>> VFIO.
>> The perf usage is especially egregious, as there is zero reason perf 
>> should need
>> KVM internals[1].  VFIO requires a bit more effort, but I'm fairly 
>> confident that
>> Jason's file-based approach[2] will yield clean, robust code that 
>> minimizes the
>> number of #ifdefs required.
>>
>> I'm planning/hoping to get back to that series in the next few weeks.  
>> As for
>> this small series, I prefer to unconditionally define the typedef, as 
>> it requires
>> no additional #ifdefs, and there are no meaningful downsides to 
>> letting the
>> typedef exist for all kernel builds.
> 
> Seems the final target is to remove those <linux/kvm_host.h> users, or I 
> think a safe-once-for-all solution is to provide the stubs in 
> <linux/kvm_host.h> with:
> 
>      #ifdef CONFIG_KVM
>      ...
>      #else
>      #endif
> 
> In either way, my concerns is it seems modifying the <asm/reboot.h> is a 
> temporary workaround.  And when we reach the final solution I suppose we 
> will need to revert it back to the current way?
> 
> If so, how about manually add a temporary typedef in <asm/kvm_host.h> 
> for now?
> 
>      #ifndef CONFIG_KVM
>      typedef void (cpu_emergency_virt_cb)(void);
>      #endif
> 
> Yes it's ugly, but it's KVM self-contained, and can be removed when ready.
> 
> Anyway, just my 2 cents.
> 

A second thought:

How about we just make all emergency virtualization disable code 
unconditional but not guided by CONFIG_KVM_INTEL || CONFIG_KVM_AMD, 
i.e., revert commit

    261cd5ed934e ("x86/reboot: Expose VMCS crash hooks if and only if 
KVM_{INTEL,AMD} is enabled")

It makes sense anyway from the perspective that it allows the 
out-of-tree kernel module hypervisor to use this mechanism w/o needing 
to have the kernel built with KVM enabled in Kconfig.  Otherwise, 
strictly speaking, IIUC, the kernel won't be able to support out-of-tree 
module hypervisor as there's no other way the module can intercept 
emergency reboot.

This approach avoids the weirdness of the unconditional define for only 
cpu_emergency_virt_cb.