Received-SPF: pass (google.com: domain of linux-kernel+bounces-178149-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <6100e822-378b-422e-8ff8-f41b19785eea@intel.com>
Date: Tue, 14 May 2024 10:44:29 +1200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/4] x86/reboot: Unconditionally define
 cpu_emergency_virt_cb typedef
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>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <ZkI5WApAR6iqCgil@google.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?UXFtaE83eUI1UmxZRjk5ZzJlWG4wK21CeXgwRU1rZXpVMEFCZjQrV0RrVndj?=
 =?utf-8?B?NEhwR0FpYW5FWmNkTGJtVVZlS2VWZXZyQ2Z0TUk2OGtMZmtTeDlHUEkzMHd3?=
 =?utf-8?B?czZwVjEwaHZIcFZ5U1hUUHBKRWJWbklRVVNWTHYwZEl5dGloYUNMQVVuWTJQ?=
 =?utf-8?B?THk4aEJzek1hL1JuV2V2aitlWlFUSmVZUWYzc0VqY1VhYjc0TitZN2Z0TTJi?=
 =?utf-8?B?NW1CKzc5SDRCaDVwQjlWMFEvTkpRVHBiWUhXc0RjMnVLZU51YTFGbWlLWkQ1?=
 =?utf-8?B?WW1YWTVHZ1BzUjE5dEw4M05EdlhmdzQ0ditrV3NsdVJ2WXJJazJ6ZEhFcjhC?=
 =?utf-8?B?L2tCREYydjlsdGpTa3RGcXhzRjE0ZW1nWHFOQXRPeUFLY0ZaY2hPd3VrT3Mv?=
 =?utf-8?B?VlVSMldPQzFxMzJ6emJFM3J6VWtiZDgxSmx0SjlpZTN2VVYveUVVcnptM2pL?=
 =?utf-8?B?R3pEZmJQeHpza3Q4RnJtU1luVFg3WE0yalJMU3dvUC9HYndpcWtmTU10cjBW?=
 =?utf-8?B?TWtwQWV1M1lkT2FxQU1GVU5ybDFxdlBOZ3JhdkdGRXl1K2FHZkRYakNERlRj?=
 =?utf-8?B?YW9Qa1NlT1h1RU9iaG4wQStKQzA3RkZWUFFQbkh0TEU4RkltbG9vd0UyWDVh?=
 =?utf-8?B?NXVrcm5Ra2ZINlVONk13bjVjOHVnWEU3Nzk4UVVxMkpoZW9mdzd3QjN1eDcz?=
 =?utf-8?B?OFJZSEVzQ0djajZvbUpLbk5TWXYzTmdnMEszRjdkc0lnbHgxbVpaaFFabGFL?=
 =?utf-8?B?TXpEY3J5MVJ4dnhRSUs4VUJJR1hEcklLcFRTK1B4eVFDN3phNnF4TFhwNmdG?=
 =?utf-8?B?RldPSGZYQlhMYXBBMHp6d1Ftdk9FekhoS0J0akswOVBDamtSUFV3TmFYZkNw?=
 =?utf-8?B?WEhoNWhnZXBRRWY5TWpyVDlha0JvbkJJN0xCOXVzMW1EUVN0eWQ4Y1l1cVZO?=
 =?utf-8?B?S01oaC9oUWdJaTQ3OExPbmIyVkd1RDA4ZkNtK2x2QVhMK0ZwVlhtRmgxcTd2?=
 =?utf-8?B?SEZGamVwN3AzM2R3eVJVejRaQnplSDcyeWVvNTVqVWUyelJ5Mjk2YnlSSk53?=
 =?utf-8?B?bXBmOE9OR1A1THh6OTdweGdaVk5XZUg2WHE2Zzl5SW9sTHVYaUFOaTM0Wmxa?=
 =?utf-8?B?U0hDVHJxOVkwMi9ESFNwcU1GQS9NWE9LcXc0T0hhRThOTTd0UlJ3d29tRURQ?=
 =?utf-8?B?NExtVXhMczYzdmEyQzlnRDJkV0loOVR5dmZnMTZrUm0rR1B6UkZwZFlUVjZ2?=
 =?utf-8?B?WmZEZmR0dU5PTUVEeTliandHZWlTTFBSYkxpOXhjeHVhcEVjZ0laQ3dycDdt?=
 =?utf-8?B?RTVQdDBmU2NqNnhNYlhlT3BvN2V6ZCs3dHROZnF2eHJzRkJuQ3VYL1pDMFRh?=
 =?utf-8?B?VHRXY2t5NzJ0STZTR1pxM3ZaU1Q2Tk9QaUx2anB3L3FWYUFlTTZ3Vm4vQjM3?=
 =?utf-8?B?cFhDSGw4aTNjMjVTYTZaVE53TUJ0OU04ZkNPL0tRSXhPdkRLM3hsVE1aYW5m?=
 =?utf-8?B?S0crbzA2UVBsVXppaU9pcVFac25IUzdBK0JSa2FTR0NiWnM0Zjg0dUtOQnRx?=
 =?utf-8?B?SUhZUjI3MjdPZmcrL0RFQlk0OWlNM2Z2TFRNVk1MRXJEVHNGbXZYRHJtYzNK?=
 =?utf-8?B?WkJHVFA0cUNOREZ5bGM5OXUwZFdpVG5wN2cwbFZLMDE4aWRHYmRoUHVFNXpC?=
 =?utf-8?B?RWw0anB2Mno4eWpQdHowbXpEYnk5UnVaMjFFQjM2d01jdHJHTVNlbXYwcmxy?=
 =?utf-8?B?dUtmZnBOTHRQYW1pd2hxQUszT0liaExOQThRZ2F3cTdtMmJHaVpKSFJrZ0Yx?=
 =?utf-8?B?cFRGeU1UWmpkQkZ5L1h5eCtDR1RPbzVJRG1SdHp0UmpWd1RpQmxicFd1eDhM?=
 =?utf-8?B?VDkwcmZNMm5jazFxVGNqYkZ0cFRUZWdMQ1l2YnJYOXlKaGNHU3lGblhWWnZQ?=
 =?utf-8?B?UU5MS1BiVnJKeE53OGlXTUJlOXpQeW1hMHJhdzVLcXBMa1NSRzlEellhUFdT?=
 =?utf-8?B?aHN6QmZ0aVJ6d0xjSkptaW1OdEM4MVVDOGQ4VFdtdHZYUmhQVklqSit1b0lk?=
 =?utf-8?B?VE5KTTNrcHc2cjcvQnI3QUdLaUtBOVd0VEVtaktmUFlSQ1daZzRZdzVISTNv?=
 =?utf-8?Q?h4pMLOb2Bf7k6vPAlTH6pN1b0?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3f475ee9-9550-4d5c-fcea-08dc739e4448
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 May 2024 22:44:36.9022
 (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: gDy6iKKhf3hGIqkTeFI2T/R7N0zBSICLJ5aXqRZMFhLnN2/QIA4SgKqum+8Sjm/cBgI7ruGrtpvEgX7Bk1FH/Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7649
X-OriginatorOrg: intel.com


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.


> 
> [1] https://lore.kernel.org/all/20230916003118.2540661-21-seanjc@google.com
> [2] https://lore.kernel.org/all/ZXkVSKULLivrMkBl@google.com
> 
>> And it seems vfio does it in vfio_main.c:
>>
>> 	#if IS_ENABLED(CONFIG_KVM)
>> 	#include <linux/kvm_host.h>
>> 	#endif
>>
>> 	#if IS_ENABLED(CONFIG_KVM)
>> 	void vfio_device_get_kvm_safe(struct vfio_device *device,
>> 			struct kvm *kvm)
>> 	{
>> 		...
>> 	}
>> 	...
>> 	#endif
>>
>>
>> The only remaining weird thing is 'struct kvm *kvm' is still used
>> unconditionally in vfio_main.c, but I think the reason it builds fine with
>> !CONFIG_KVM is because <linux/vfio.h> declares it explicitly:
>>
>> 	struct kvm;
>> 	struct iommufd_ctx;
>> 	...
>>
>> So it seems to me that this patch around 'cpu_emergency_virt_cb' is more
>> like a workaround of existing non-perfect <linux/kvm_host.h> and/or
>> <asm/kvm_host.h>?
>