Re: [PATCH v5 01/12] x86/paravirt: Move halt paravirt calls under CONFIG_PARAVIRT

From: Juergen Gross <jgross@suse.com>
To: Borislav Petkov <bp@alien8.de>,
	Kuppuswamy Sathyanarayanan 
	<sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Andy Lutomirski <luto@kernel.org>, Peter H Anvin <hpa@zytor.com>,
	Dave Hansen <dave.hansen@intel.com>,
	Tony Luck <tony.luck@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Andi Kleen <ak@linux.intel.com>,
	Kirill Shutemov <kirill.shutemov@linux.intel.com>,
	Sean Christopherson <seanjc@google.com>,
	Kuppuswamy Sathyanarayanan <knsathya@kernel.org>,
	x86@kernel.org, linux-kernel@vger.kernel.org,
	Deep Shah <sdeep@vmware.com>,
	"VMware, Inc." <pv-drivers@vmware.com>
Subject: Re: [PATCH v5 01/12] x86/paravirt: Move halt paravirt calls under CONFIG_PARAVIRT
Date: Tue, 17 Aug 2021 14:50:47 +0200	[thread overview]
Message-ID: <057170db-9382-eb40-7bcb-4ec1de4aae62@suse.com> (raw)
In-Reply-To: <YRTLO0eQOEChETId@zn.tnic>

[-- Attachment #1.1.1: Type: text/plain, Size: 6673 bytes --]

On 12.08.21 09:18, Borislav Petkov wrote:
> On Wed, Aug 04, 2021 at 11:13:18AM -0700, Kuppuswamy Sathyanarayanan wrote:
>> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>>
>> CONFIG_PARAVIRT_XXL is mainly defined/used by XEN PV guests. For
>> other VM guest types, features supported under CONFIG_PARAVIRT
>> are self sufficient. CONFIG_PARAVIRT mainly provides support for
>> TLB flush operations and time related operations.
>>
>> For TDX guest as well, paravirt calls under CONFIG_PARVIRT meets
>> most of its requirement except the need of HLT and SAFE_HLT
>> paravirt calls, which is currently defined under
>> COFNIG_PARAVIRT_XXL.
>>
>> Since enabling CONFIG_PARAVIRT_XXL is too bloated for TDX guest
>> like platforms, move HLT and SAFE_HLT paravirt calls under
>> CONFIG_PARAVIRT.
>>
>> Moving HLT and SAFE_HLT paravirt calls are not fatal and should not
>> break any functionality for current users of CONFIG_PARAVIRT.
>>
>> Co-developed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Reviewed-by: Andi Kleen <ak@linux.intel.com>
>> Reviewed-by: Tony Luck <tony.luck@intel.com>
>> ---
> 
> You need to do this before sending your patches:
> 
> ./scripts/get_maintainer.pl /tmp/tdx.01
> Thomas Gleixner <tglx@linutronix.de> (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),commit_signer:1/6=17%)
> Ingo Molnar <mingo@redhat.com> (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT))
> Borislav Petkov <bp@alien8.de> (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),commit_signer:6/6=100%)
> x86@kernel.org (maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT))
> "H. Peter Anvin" <hpa@zytor.com> (reviewer:X86 ARCHITECTURE (32-BIT AND 64-BIT))
> Juergen Gross <jgross@suse.com> (supporter:PARAVIRT_OPS INTERFACE,commit_signer:5/6=83%,authored:5/6=83%,added_lines:15/16=94%,removed_lines:38/39=97%)
> Deep Shah <sdeep@vmware.com> (supporter:PARAVIRT_OPS INTERFACE)
> "VMware, Inc." <pv-drivers@vmware.com> (supporter:PARAVIRT_OPS INTERFACE)
> ...
> 
> and CC also the supporters - I'm pretty sure at least Juergen would like
> to be kept up-to-date on pv changes. I'll CC him and the others now and
> leave in the whole diff but make sure you do that in the future please.
> 
>>   arch/x86/include/asm/irqflags.h       | 40 +++++++++++++++------------
>>   arch/x86/include/asm/paravirt.h       | 20 +++++++-------
>>   arch/x86/include/asm/paravirt_types.h |  3 +-
>>   arch/x86/kernel/paravirt.c            |  4 ++-
>>   4 files changed, 36 insertions(+), 31 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
>> index c5ce9845c999..f3bb33b1715d 100644
>> --- a/arch/x86/include/asm/irqflags.h
>> +++ b/arch/x86/include/asm/irqflags.h
>> @@ -59,6 +59,28 @@ static inline __cpuidle void native_halt(void)
>>   
>>   #endif
>>   
>> +#ifndef CONFIG_PARAVIRT
>> +#ifndef __ASSEMBLY__
>> +/*
>> + * Used in the idle loop; sti takes one instruction cycle
>> + * to complete:
>> + */
>> +static inline __cpuidle void arch_safe_halt(void)
>> +{
>> +	native_safe_halt();
>> +}
>> +
>> +/*
>> + * Used when interrupts are already enabled or to
>> + * shutdown the processor:
>> + */
>> +static inline __cpuidle void halt(void)
>> +{
>> +	native_halt();
>> +}
>> +#endif /* __ASSEMBLY__ */
>> +#endif /* CONFIG_PARAVIRT */
>> +
>>   #ifdef CONFIG_PARAVIRT_XXL
>>   #include <asm/paravirt.h>

Did you test this with CONFIG_PARAVIRT enabled and CONFIG_PARAVIRT_XXL
disabled?

I'm asking because in this case I don't see where halt() and
arch_safe_halt() would be defined in case someone is including
asm/irqflags.h and not asm/paravirt.h.

>>   #else
>> @@ -80,24 +102,6 @@ static __always_inline void arch_local_irq_enable(void)
>>   	native_irq_enable();
>>   }
>>   
>> -/*
>> - * Used in the idle loop; sti takes one instruction cycle
>> - * to complete:
>> - */
>> -static inline __cpuidle void arch_safe_halt(void)
>> -{
>> -	native_safe_halt();
>> -}
>> -
>> -/*
>> - * Used when interrupts are already enabled or to
>> - * shutdown the processor:
>> - */
>> -static inline __cpuidle void halt(void)
>> -{
>> -	native_halt();
>> -}
>> -
>>   /*
>>    * For spinlocks, etc:
>>    */
>> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
>> index da3a1ac82be5..d323a626c7a8 100644
>> --- a/arch/x86/include/asm/paravirt.h
>> +++ b/arch/x86/include/asm/paravirt.h
>> @@ -97,6 +97,16 @@ static inline void paravirt_arch_exit_mmap(struct mm_struct *mm)
>>   	PVOP_VCALL1(mmu.exit_mmap, mm);
>>   }
>>   
>> +static inline void arch_safe_halt(void)
>> +{
>> +	PVOP_VCALL0(irq.safe_halt);
>> +}
>> +
>> +static inline void halt(void)
>> +{
>> +	PVOP_VCALL0(irq.halt);
>> +}
>> +
>>   #ifdef CONFIG_PARAVIRT_XXL
>>   static inline void load_sp0(unsigned long sp0)
>>   {
>> @@ -162,16 +172,6 @@ static inline void __write_cr4(unsigned long x)
>>   	PVOP_VCALL1(cpu.write_cr4, x);
>>   }
>>   
>> -static inline void arch_safe_halt(void)
>> -{
>> -	PVOP_VCALL0(irq.safe_halt);
>> -}
>> -
>> -static inline void halt(void)
>> -{
>> -	PVOP_VCALL0(irq.halt);
>> -}
>> -
>>   static inline void wbinvd(void)
>>   {
>>   	PVOP_ALT_VCALL0(cpu.wbinvd, "wbinvd", ALT_NOT(X86_FEATURE_XENPV));
>> diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
>> index d9d6b0203ec4..40082847f314 100644
>> --- a/arch/x86/include/asm/paravirt_types.h
>> +++ b/arch/x86/include/asm/paravirt_types.h
>> @@ -150,10 +150,9 @@ struct pv_irq_ops {
>>   	struct paravirt_callee_save save_fl;
>>   	struct paravirt_callee_save irq_disable;
>>   	struct paravirt_callee_save irq_enable;
>> -
>> +#endif
>>   	void (*safe_halt)(void);
>>   	void (*halt)(void);
>> -#endif
>>   } __no_randomize_layout;
>>   
>>   struct pv_mmu_ops {
>> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
>> index 04cafc057bed..124e0f6c5d1c 100644
>> --- a/arch/x86/kernel/paravirt.c
>> +++ b/arch/x86/kernel/paravirt.c
>> @@ -283,9 +283,11 @@ struct paravirt_patch_template pv_ops = {
>>   	.irq.save_fl		= __PV_IS_CALLEE_SAVE(native_save_fl),
>>   	.irq.irq_disable	= __PV_IS_CALLEE_SAVE(native_irq_disable),
>>   	.irq.irq_enable		= __PV_IS_CALLEE_SAVE(native_irq_enable),
>> +#endif /* CONFIG_PARAVIRT_XXL */
>> +
>> +	/* Irq HLT ops. */
> 
> What's that comment for?

I agree, please drop it.

Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]