[PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Kirill A. Shutemov]

TDX is going to use CONFIG_PARAVIRT_XXL, but kernel fails to compile if
XEN_PV is not enabled:

	ld.lld: error: undefined symbol: xen_iret

It happens because INTERRUPT_RETURN defined to use xen_iret if
CONFIG_PARAVIRT_XXL enabled regardless of CONFIG_XEN_PV.

The issue is not visible in the current kernel because CONFIG_XEN_PV is
the only user of CONFIG_PARAVIRT_XXL and there's no way to enable them
separately.

Rework code to define INTERRUPT_RETURN based on CONFIG_XEN_PV, not
CONFIG_PARAVIRT_XXL.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Deep Shah <sdeep@vmware.com>
Cc: "VMware, Inc." <pv-drivers@vmware.com>
---
 arch/x86/include/asm/irqflags.h | 7 +++++--
 arch/x86/include/asm/paravirt.h | 5 -----
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
index b794b6da3214..3b8ddcb7be76 100644
--- a/arch/x86/include/asm/irqflags.h
+++ b/arch/x86/include/asm/irqflags.h
@@ -118,8 +118,6 @@ static __always_inline unsigned long arch_local_irq_save(void)
 #define SAVE_FLAGS		pushfq; popq %rax
 #endif
 
-#define INTERRUPT_RETURN	jmp native_iret
-
 #endif
 
 #endif /* __ASSEMBLY__ */
@@ -147,8 +145,13 @@ static __always_inline void arch_local_irq_restore(unsigned long flags)
 #ifdef CONFIG_X86_64
 #ifdef CONFIG_XEN_PV
 #define SWAPGS	ALTERNATIVE "swapgs", "", X86_FEATURE_XENPV
+#define INTERRUPT_RETURN						\
+	ANNOTATE_RETPOLINE_SAFE;					\
+	ALTERNATIVE_TERNARY("jmp *paravirt_iret(%rip);",		\
+		X86_FEATURE_XENPV, "jmp xen_iret;", "jmp native_iret;")
 #else
 #define SWAPGS	swapgs
+#define INTERRUPT_RETURN	jmp native_iret
 #endif
 #endif
 #endif /* !__ASSEMBLY__ */
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 8e04268d7c10..865f78635af1 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -745,11 +745,6 @@ extern void default_banner(void);
 #define PARA_SITE(ptype, ops)	_PVSITE(ptype, ops, .quad, 8)
 #define PARA_INDIRECT(addr)	*addr(%rip)
 
-#define INTERRUPT_RETURN						\
-	ANNOTATE_RETPOLINE_SAFE;					\
-	ALTERNATIVE_TERNARY("jmp *paravirt_iret(%rip);",		\
-		X86_FEATURE_XENPV, "jmp xen_iret;", "jmp native_iret;")
-
 #ifdef CONFIG_DEBUG_ENTRY
 .macro PARA_IRQ_save_fl
 	PARA_SITE(PARA_PATCH(PV_IRQ_save_fl),
-- 
2.32.0

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Peter Zijlstra]

On Wed, Nov 17, 2021 at 09:14:39PM +0300, Kirill A. Shutemov wrote:
> TDX is going to use CONFIG_PARAVIRT_XXL

*AARGGHHH*. srlsy? We were trying to cut back on that insanity, not
proliferate it.

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Kirill A. Shutemov]

On Wed, Nov 17, 2021 at 07:35:01PM +0100, Peter Zijlstra wrote:
> On Wed, Nov 17, 2021 at 09:14:39PM +0300, Kirill A. Shutemov wrote:
> > TDX is going to use CONFIG_PARAVIRT_XXL
> 
> *AARGGHHH*. srlsy? We were trying to cut back on that insanity, not
> proliferate it.

It is a way to minimize amount of changes needed for getting TDX
functinal. We will remove the dependency later on.

-- 
 Kirill A. Shutemov

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Sathyanarayanan Kuppuswamy]

On 11/17/21 10:42 AM, Kirill A. Shutemov wrote:
> On Wed, Nov 17, 2021 at 07:35:01PM +0100, Peter Zijlstra wrote:
>> On Wed, Nov 17, 2021 at 09:14:39PM +0300, Kirill A. Shutemov wrote:
>>> TDX is going to use CONFIG_PARAVIRT_XXL
>>
>> *AARGGHHH*. srlsy? We were trying to cut back on that insanity, not
>> proliferate it.
> 
> It is a way to minimize amount of changes needed for getting TDX
> functinal. We will remove the dependency later on.
> 

TDX has a requirement to use HLT paravirt calls (which is currently
listed under PARAVIRT_XXL). Once we submit a patch to move it
under CONFIG_PARAVIRT, we will drop this dependency.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Borislav Petkov]

On Wed, Nov 17, 2021 at 10:46:30AM -0800, Sathyanarayanan Kuppuswamy wrote:
> TDX has a requirement to use HLT paravirt calls (which is currently
> listed under PARAVIRT_XXL). Once we submit a patch to move it
> under CONFIG_PARAVIRT, we will drop this dependency.

You already have this patch in some set:

https://lore.kernel.org/r/20211009053747.1694419-2-sathyanarayanan.kuppuswamy@linux.intel.com

So what's this churn for?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Peter Zijlstra]

On Wed, Nov 17, 2021 at 10:46:30AM -0800, Sathyanarayanan Kuppuswamy wrote:
> TDX has a requirement to use HLT paravirt calls (which is currently
> listed under PARAVIRT_XXL). Once we submit a patch to move it
> under CONFIG_PARAVIRT, we will drop this dependency.

Why do you need to paravirt hlt at all? Can't the TDX thing simply
install a different x86_idle function?

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Dave Hansen]

On 11/17/21 10:48 AM, Borislav Petkov wrote:
> On Wed, Nov 17, 2021 at 10:46:30AM -0800, Sathyanarayanan Kuppuswamy wrote:
>> TDX has a requirement to use HLT paravirt calls (which is currently
>> listed under PARAVIRT_XXL). Once we submit a patch to move it
>> under CONFIG_PARAVIRT, we will drop this dependency.
> You already have this patch in some set:
> 
> https://lore.kernel.org/r/20211009053747.1694419-2-sathyanarayanan.kuppuswamy@linux.intel.com
> 
> So what's this churn for?

The churn is my doing.  It occurred to me that we could toss the
aforementioned patch out of the set by using:

	depends on PARAVIRT_XXL

instead of:

	depends on PARAVIRT

Especially since PARAVIRT_XXL seems pretty common in distro kernels.
But, I didn't realize quite how reviled PARAVIRT_XXL was.

So, first of all, _this_ patch is a proper cleanup.  We should merge it
or something like it either way.

As for TDX, my preference (obviously) is to keep the "depends on
PARAVIRT_XXL", reviled as it may be.  It's the literal truth at this
point: TDX guest support depends on PARAVIRT_XXL functionality.  There
is no shortage of ways to remove that dependency (expand PARAVIRT or
custom idle), which makes me very confident that we are not painting
ourselves into a corner here.

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Sathyanarayanan Kuppuswamy]

On 11/17/21 10:48 AM, Borislav Petkov wrote:
> On Wed, Nov 17, 2021 at 10:46:30AM -0800, Sathyanarayanan Kuppuswamy wrote:
>> TDX has a requirement to use HLT paravirt calls (which is currently
>> listed under PARAVIRT_XXL). Once we submit a patch to move it
>> under CONFIG_PARAVIRT, we will drop this dependency.
> 
> You already have this patch in some set:
> 
> https://lore.kernel.org/r/20211009053747.1694419-2-sathyanarayanan.kuppuswamy@linux.intel.com
> 
> So what's this churn for?

Previously, we have posted ~45+ patches for TDX guest support. But we 
have noticed that submitting too many TDX patches at the same time seems
to complicates the review process.

So to make the review process simpler, we had planned to resubmit only a 
minimum set required to boot the TDX guest to user space (which comes 
around 25 patches).

Since moving HLT PV calls under CONFIG_PARAVIRT is an optimization fix
we did not include it in the minimal set.

If you think otherwise, please let me know. We will drop the use of
PARAVIRT_XXL and just use the above-mentioned patch.


> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Dave Hansen]

> TDX has a requirement to use HLT paravirt calls (which is currently
> listed under PARAVIRT_XXL). Once we submit a patch to move it
> under CONFIG_PARAVIRT, we will drop this dependency.

Taking a step back...

The basic requirement here is murky.  Why does TDX need to use these
paravirt hooks in the first place?  Why does TDX have "a requirement to
use HLT paravirt calls"?

If it really is just about idle, perhaps Peter's suggestion warrants
investigation.  But, we need to know the root cause instead of simply
tossing around "requirements".

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Sathyanarayanan Kuppuswamy]

On 11/17/21 11:57 AM, Dave Hansen wrote:
> If it really is just about idle, perhaps Peter's suggestion warrants
> investigation.  But, we need to know the root cause instead of simply
> tossing around "requirements".

It is not only for idle case. We also need to support emulation of
normal halt case (which is used in cases like reboot or
early_fixup_exception(), etc.).

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Borislav Petkov]

On Wed, Nov 17, 2021 at 12:54:56PM -0800, Sathyanarayanan Kuppuswamy wrote:
> It is not only for idle case. We also need to support emulation of
> normal halt case (which is used in cases like reboot or
> early_fixup_exception(), etc.).

This is more of that piecemeal feeding of people asking why. Please sit
down and explain exactly and in detail why you need this. "We need to
support emulation" is not nearly good enough.

I don't think any of the people who replied on this thread *actually*
know *why* PV support is needed.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Sathyanarayanan Kuppuswamy]

On 11/17/21 1:09 PM, Borislav Petkov wrote:
> On Wed, Nov 17, 2021 at 12:54:56PM -0800, Sathyanarayanan Kuppuswamy wrote:
>> It is not only for idle case. We also need to support emulation of
>> normal halt case (which is used in cases like reboot or
>> early_fixup_exception(), etc.).
> 
> This is more of that piecemeal feeding of people asking why. Please sit
> down and explain exactly and in detail why you need this. "We need to
> support emulation" is not nearly good enough.
> 
> I don't think any of the people who replied on this thread *actually*
> know *why* PV support is needed.
> 

We need PV support to handle halt() and safe_halt() cases.

HLT instruction is generally used in cases like reboot, idle and
exception fixup handlers. For the idle case, interrupts will be enabled
(using STI) before the HLT instruction (this is also called
safe_halt()).

In TDX guest, to support HLT instruction, it has to be emulated using
a hypercall (aka TDVMCALL).

We have the following three ways to emulate the HLT instruction:

1. Directly substitute TDVMCALLs in places where we require emulation.
2. Use #VE exception handler to emulate it (In TDX guest, executing HLT
    will lead to #VE exception).
3. Emulate it using pv_ops

Since option#1 is not a scalable approach, it can be ignored. Option #2
is also not preferred because, we cannot differentiate between safe
halt and normal halt use cases in the exception handler. This
differentiation is needed to add STI before the hypercall for safe halt 
use case. That leaves us with using pv_ops, which provides separate
methods to emulate safe and normal halt cases.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Peter Zijlstra]

On Wed, Nov 17, 2021 at 03:04:11PM -0800, Sathyanarayanan Kuppuswamy wrote:

> We need PV support to handle halt() and safe_halt() cases.
> 
> HLT instruction is generally used in cases like reboot, idle and
> exception fixup handlers.

Which exception calls hlt? Because idle and reboot can easily be done.

> In TDX guest, to support HLT instruction, it has to be emulated using
> a hypercall (aka TDVMCALL).
> 
> We have the following three ways to emulate the HLT instruction:
> 
> 1. Directly substitute TDVMCALLs in places where we require emulation.
> 2. Use #VE exception handler to emulate it (In TDX guest, executing HLT
>    will lead to #VE exception).
> 3. Emulate it using pv_ops
> 
> Since option#1 is not a scalable approach, it can be ignored. Option #2
> is also not preferred because, we cannot differentiate between safe
> halt and normal halt use cases in the exception handler. 

Would not regs->flags & IF provide clue? I know STI normally has a
shadow, but wouldn't a trap in that shadow still get the flag straight?
I'm sure there's fun bugs around this, but surely TDX is new and doesn't
have these bugs.

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Dave Hansen]

On 11/17/21 3:04 PM, Sathyanarayanan Kuppuswamy wrote:
> 1. Directly substitute TDVMCALLs in places where we require emulation.
...
> Since option#1 is not a scalable approach,

Why is this not "scalable"?  Just eyeballing the problem, here's my
laptop's kernel:

$ objdump  -d vmlinux | grep 'hlt ' | awk -F: '{print $1}' |  while read
addr; do  addr2line -e vmlinux $addr; done
arch/x86/include/asm/irqflags.h:51
arch/x86/include/asm/irqflags.h:51
arch/x86/include/asm/irqflags.h:57
arch/x86/include/asm/irqflags.h:57
arch/x86/include/asm/irqflags.h:57
arch/x86/include/asm/irqflags.h:57
arch/x86/include/asm/irqflags.h:51
arch/x86/include/asm/irqflags.h:51
arch/x86/include/asm/irqflags.h:51
arch/x86/kernel/fpu/init.c:84
arch/x86/include/asm/irqflags.h:57

There are a grand total of 3 sites from which a hlt originates.  With
only 11 possible instances and 3 sites to patch, I'm not sure anything
here needs to be "scalable".

I'd suspect half these sites aren't even reachable on a TDX system.

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Sathyanarayanan Kuppuswamy]

On 11/17/21 3:23 PM, Peter Zijlstra wrote:
> On Wed, Nov 17, 2021 at 03:04:11PM -0800, Sathyanarayanan Kuppuswamy wrote:
> 
>> We need PV support to handle halt() and safe_halt() cases.
>>
>> HLT instruction is generally used in cases like reboot, idle and
>> exception fixup handlers.
> 
> Which exception calls hlt? Because idle and reboot can easily be done.

It is called in early_fixup_exception().

> 
>> In TDX guest, to support HLT instruction, it has to be emulated using
>> a hypercall (aka TDVMCALL).
>>
>> We have the following three ways to emulate the HLT instruction:
>>
>> 1. Directly substitute TDVMCALLs in places where we require emulation.
>> 2. Use #VE exception handler to emulate it (In TDX guest, executing HLT
>>     will lead to #VE exception).
>> 3. Emulate it using pv_ops
>>
>> Since option#1 is not a scalable approach, it can be ignored. Option #2
>> is also not preferred because, we cannot differentiate between safe
>> halt and normal halt use cases in the exception handler.
> 
> Would not regs->flags & IF provide clue? I know STI normally has a
> shadow, but wouldn't a trap in that shadow still get the flag straight?
> I'm sure there's fun bugs around this, but surely TDX is new and doesn't
> have these bugs.

We have attempted this approach, but it failed some performance tests.

Yes, if we use option # 2, for safe_halt() use case, STI will leave the
interrupts in the desired state. But, between the STI instruction and
the actual emulation of the HLT instruction, interrupts will be left in
the enabled state. So any interrupt that happen in that window will
delay the HLT operation for a long time.

With above consideration, we thought PV ops is error free and a simpler
solution.

> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Sathyanarayanan Kuppuswamy]

On 11/17/21 3:33 PM, Dave Hansen wrote:
> On 11/17/21 3:04 PM, Sathyanarayanan Kuppuswamy wrote:
>> 1. Directly substitute TDVMCALLs in places where we require emulation.
> ...
>> Since option#1 is not a scalable approach,
> 
> Why is this not "scalable"?  Just eyeballing the problem, here's my
> laptop's kernel:

I meant it not scalable because, for any new use case of HLT
instruction, it would need substitution again. We cannot always keep
tracking its uses in the future, right?

Also, If we make any changes to the core emulation code (like arguments
change), we will have to re-touch these use cases to handle it.

If this approach is taken to add some performance benefit, then it
makes sense.  But for our case, I think it is simpler to use PV ops.



> 
> $ objdump  -d vmlinux | grep 'hlt ' | awk -F: '{print $1}' |  while read
> addr; do  addr2line -e vmlinux $addr; done
> arch/x86/include/asm/irqflags.h:51
> arch/x86/include/asm/irqflags.h:51
> arch/x86/include/asm/irqflags.h:57
> arch/x86/include/asm/irqflags.h:57
> arch/x86/include/asm/irqflags.h:57
> arch/x86/include/asm/irqflags.h:57
> arch/x86/include/asm/irqflags.h:51
> arch/x86/include/asm/irqflags.h:51
> arch/x86/include/asm/irqflags.h:51
> arch/x86/kernel/fpu/init.c:84
> arch/x86/include/asm/irqflags.h:57
> 
> There are a grand total of 3 sites from which a hlt originates.  With
> only 11 possible instances and 3 sites to patch, I'm not sure anything
> here needs to be "scalable".
> 
> I'd suspect half these sites aren't even reachable on a TDX system.
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Juergen Gross]

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

On 17.11.21 19:14, Kirill A. Shutemov wrote:
> TDX is going to use CONFIG_PARAVIRT_XXL, but kernel fails to compile if
> XEN_PV is not enabled:
> 
> 	ld.lld: error: undefined symbol: xen_iret
> 
> It happens because INTERRUPT_RETURN defined to use xen_iret if
> CONFIG_PARAVIRT_XXL enabled regardless of CONFIG_XEN_PV.
> 
> The issue is not visible in the current kernel because CONFIG_XEN_PV is
> the only user of CONFIG_PARAVIRT_XXL and there's no way to enable them
> separately.
> 
> Rework code to define INTERRUPT_RETURN based on CONFIG_XEN_PV, not
> CONFIG_PARAVIRT_XXL.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Deep Shah <sdeep@vmware.com>
> Cc: "VMware, Inc." <pv-drivers@vmware.com>

I agree with the patch, so:

Reviewed-by: Juergen Gross <jgross@suse.com>

Nevertheless I believe that TDX should not switch to use PARAVIRT_XXL.


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 --]

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Borislav Petkov]

On Wed, Nov 17, 2021 at 05:26:15PM -0800, Sathyanarayanan Kuppuswamy wrote:
> I meant it not scalable because, for any new use case of HLT
> instruction, it would need substitution again. We cannot always keep
> tracking its uses in the future, right?

I don't understand what the whole hoopla is about:

	alternative_call(halt, tdx_halt, X86_FEATURE_TDX_GUEST, ...)

No PV needed.

> Also, If we make any changes to the core emulation code (like arguments
> change), we will have to re-touch these use cases to handle it.

We do change the core code all the time to accomodate new stuff. You
should be seeing that constantly with the rate of change happening...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Lai Jiangshan]

On Thu, Nov 18, 2021 at 7:14 AM Kirill A. Shutemov <kirill@shutemov.name> wrote:
>
> TDX is going to use CONFIG_PARAVIRT_XXL, but kernel fails to compile if
> XEN_PV is not enabled:
>
>         ld.lld: error: undefined symbol: xen_iret
>
> It happens because INTERRUPT_RETURN defined to use xen_iret if
> CONFIG_PARAVIRT_XXL enabled regardless of CONFIG_XEN_PV.
>
> The issue is not visible in the current kernel because CONFIG_XEN_PV is
> the only user of CONFIG_PARAVIRT_XXL and there's no way to enable them
> separately.
>
> Rework code to define INTERRUPT_RETURN based on CONFIG_XEN_PV, not
> CONFIG_PARAVIRT_XXL.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Deep Shah <sdeep@vmware.com>
> Cc: "VMware, Inc." <pv-drivers@vmware.com>
> ---
>  arch/x86/include/asm/irqflags.h | 7 +++++--
>  arch/x86/include/asm/paravirt.h | 5 -----
>  2 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
> index b794b6da3214..3b8ddcb7be76 100644
> --- a/arch/x86/include/asm/irqflags.h
> +++ b/arch/x86/include/asm/irqflags.h
> @@ -118,8 +118,6 @@ static __always_inline unsigned long arch_local_irq_save(void)
>  #define SAVE_FLAGS             pushfq; popq %rax
>  #endif
>
> -#define INTERRUPT_RETURN       jmp native_iret
> -
>  #endif
>
>  #endif /* __ASSEMBLY__ */
> @@ -147,8 +145,13 @@ static __always_inline void arch_local_irq_restore(unsigned long flags)
>  #ifdef CONFIG_X86_64
>  #ifdef CONFIG_XEN_PV
>  #define SWAPGS ALTERNATIVE "swapgs", "", X86_FEATURE_XENPV
> +#define INTERRUPT_RETURN                                               \
> +       ANNOTATE_RETPOLINE_SAFE;                                        \
> +       ALTERNATIVE_TERNARY("jmp *paravirt_iret(%rip);",                \
> +               X86_FEATURE_XENPV, "jmp xen_iret;", "jmp native_iret;")

It is part of what CONFIG_PARAVIRT_XXL was designed for to enable
pv-aware INTERRUPT_RETURN.

I would prefer xen_iret is defined as a weak symbol unconditionally.
Like:

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index e38a4cf795d9..c0953f1b4559 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -635,6 +635,7 @@ SYM_INNER_LABEL_ALIGN(native_iret, SYM_L_GLOBAL)
        jnz     native_irq_return_ldt
 #endif

+SYM_INNER_LABEL(xen_iret, SYM_L_WEAK) /* placeholder */
 SYM_INNER_LABEL(native_irq_return_iret, SYM_L_GLOBAL)
        /*
         * This may fault.  Non-paranoid faults on return to userspace are

It will work when !CONFIG_XEN_PV

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Kirill A. Shutemov]

On Fri, Nov 19, 2021 at 03:51:44PM +0800, Lai Jiangshan wrote:
> On Thu, Nov 18, 2021 at 7:14 AM Kirill A. Shutemov <kirill@shutemov.name> wrote:
> >
> > TDX is going to use CONFIG_PARAVIRT_XXL, but kernel fails to compile if
> > XEN_PV is not enabled:
> >
> >         ld.lld: error: undefined symbol: xen_iret
> >
> > It happens because INTERRUPT_RETURN defined to use xen_iret if
> > CONFIG_PARAVIRT_XXL enabled regardless of CONFIG_XEN_PV.
> >
> > The issue is not visible in the current kernel because CONFIG_XEN_PV is
> > the only user of CONFIG_PARAVIRT_XXL and there's no way to enable them
> > separately.
> >
> > Rework code to define INTERRUPT_RETURN based on CONFIG_XEN_PV, not
> > CONFIG_PARAVIRT_XXL.
> >
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Cc: Juergen Gross <jgross@suse.com>
> > Cc: Deep Shah <sdeep@vmware.com>
> > Cc: "VMware, Inc." <pv-drivers@vmware.com>
> > ---
> >  arch/x86/include/asm/irqflags.h | 7 +++++--
> >  arch/x86/include/asm/paravirt.h | 5 -----
> >  2 files changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
> > index b794b6da3214..3b8ddcb7be76 100644
> > --- a/arch/x86/include/asm/irqflags.h
> > +++ b/arch/x86/include/asm/irqflags.h
> > @@ -118,8 +118,6 @@ static __always_inline unsigned long arch_local_irq_save(void)
> >  #define SAVE_FLAGS             pushfq; popq %rax
> >  #endif
> >
> > -#define INTERRUPT_RETURN       jmp native_iret
> > -
> >  #endif
> >
> >  #endif /* __ASSEMBLY__ */
> > @@ -147,8 +145,13 @@ static __always_inline void arch_local_irq_restore(unsigned long flags)
> >  #ifdef CONFIG_X86_64
> >  #ifdef CONFIG_XEN_PV
> >  #define SWAPGS ALTERNATIVE "swapgs", "", X86_FEATURE_XENPV
> > +#define INTERRUPT_RETURN                                               \
> > +       ANNOTATE_RETPOLINE_SAFE;                                        \
> > +       ALTERNATIVE_TERNARY("jmp *paravirt_iret(%rip);",                \
> > +               X86_FEATURE_XENPV, "jmp xen_iret;", "jmp native_iret;")
> 
> It is part of what CONFIG_PARAVIRT_XXL was designed for to enable
> pv-aware INTERRUPT_RETURN.

That's very vague statement.

Could you elaborate on what is wrong with proposed fix?

> I would prefer xen_iret is defined as a weak symbol unconditionally.
> Like:
> 
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index e38a4cf795d9..c0953f1b4559 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -635,6 +635,7 @@ SYM_INNER_LABEL_ALIGN(native_iret, SYM_L_GLOBAL)
>         jnz     native_irq_return_ldt
>  #endif
> 
> +SYM_INNER_LABEL(xen_iret, SYM_L_WEAK) /* placeholder */
>  SYM_INNER_LABEL(native_irq_return_iret, SYM_L_GLOBAL)
>         /*
>          * This may fault.  Non-paranoid faults on return to userspace are
> 
> It will work when !CONFIG_XEN_PV

It pollutes namespace for no particular reason. I don't see it justified.

-- 
 Kirill A. Shutemov

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Kirill A. Shutemov]

On Thu, Nov 18, 2021 at 07:23:51AM +0100, Juergen Gross wrote:
> On 17.11.21 19:14, Kirill A. Shutemov wrote:
> > TDX is going to use CONFIG_PARAVIRT_XXL, but kernel fails to compile if
> > XEN_PV is not enabled:
> > 
> > 	ld.lld: error: undefined symbol: xen_iret
> > 
> > It happens because INTERRUPT_RETURN defined to use xen_iret if
> > CONFIG_PARAVIRT_XXL enabled regardless of CONFIG_XEN_PV.
> > 
> > The issue is not visible in the current kernel because CONFIG_XEN_PV is
> > the only user of CONFIG_PARAVIRT_XXL and there's no way to enable them
> > separately.
> > 
> > Rework code to define INTERRUPT_RETURN based on CONFIG_XEN_PV, not
> > CONFIG_PARAVIRT_XXL.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Cc: Juergen Gross <jgross@suse.com>
> > Cc: Deep Shah <sdeep@vmware.com>
> > Cc: "VMware, Inc." <pv-drivers@vmware.com>
> 
> I agree with the patch, so:
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>
> 
> Nevertheless I believe that TDX should not switch to use PARAVIRT_XXL.

Do I need to resend without TDX mention?

-- 
 Kirill A. Shutemov

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Juergen Gross]

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

On 19.11.21 11:20, Kirill A. Shutemov wrote:
> On Fri, Nov 19, 2021 at 03:51:44PM +0800, Lai Jiangshan wrote:
>> On Thu, Nov 18, 2021 at 7:14 AM Kirill A. Shutemov <kirill@shutemov.name> wrote:
>>>
>>> TDX is going to use CONFIG_PARAVIRT_XXL, but kernel fails to compile if
>>> XEN_PV is not enabled:
>>>
>>>          ld.lld: error: undefined symbol: xen_iret
>>>
>>> It happens because INTERRUPT_RETURN defined to use xen_iret if
>>> CONFIG_PARAVIRT_XXL enabled regardless of CONFIG_XEN_PV.
>>>
>>> The issue is not visible in the current kernel because CONFIG_XEN_PV is
>>> the only user of CONFIG_PARAVIRT_XXL and there's no way to enable them
>>> separately.
>>>
>>> Rework code to define INTERRUPT_RETURN based on CONFIG_XEN_PV, not
>>> CONFIG_PARAVIRT_XXL.
>>>
>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>> Cc: Juergen Gross <jgross@suse.com>
>>> Cc: Deep Shah <sdeep@vmware.com>
>>> Cc: "VMware, Inc." <pv-drivers@vmware.com>
>>> ---
>>>   arch/x86/include/asm/irqflags.h | 7 +++++--
>>>   arch/x86/include/asm/paravirt.h | 5 -----
>>>   2 files changed, 5 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
>>> index b794b6da3214..3b8ddcb7be76 100644
>>> --- a/arch/x86/include/asm/irqflags.h
>>> +++ b/arch/x86/include/asm/irqflags.h
>>> @@ -118,8 +118,6 @@ static __always_inline unsigned long arch_local_irq_save(void)
>>>   #define SAVE_FLAGS             pushfq; popq %rax
>>>   #endif
>>>
>>> -#define INTERRUPT_RETURN       jmp native_iret
>>> -
>>>   #endif
>>>
>>>   #endif /* __ASSEMBLY__ */
>>> @@ -147,8 +145,13 @@ static __always_inline void arch_local_irq_restore(unsigned long flags)
>>>   #ifdef CONFIG_X86_64
>>>   #ifdef CONFIG_XEN_PV
>>>   #define SWAPGS ALTERNATIVE "swapgs", "", X86_FEATURE_XENPV
>>> +#define INTERRUPT_RETURN                                               \
>>> +       ANNOTATE_RETPOLINE_SAFE;                                        \
>>> +       ALTERNATIVE_TERNARY("jmp *paravirt_iret(%rip);",                \
>>> +               X86_FEATURE_XENPV, "jmp xen_iret;", "jmp native_iret;")
>>
>> It is part of what CONFIG_PARAVIRT_XXL was designed for to enable
>> pv-aware INTERRUPT_RETURN.
> 
> That's very vague statement.
> 
> Could you elaborate on what is wrong with proposed fix?
> 
>> I would prefer xen_iret is defined as a weak symbol unconditionally.
>> Like:
>>
>> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
>> index e38a4cf795d9..c0953f1b4559 100644
>> --- a/arch/x86/entry/entry_64.S
>> +++ b/arch/x86/entry/entry_64.S
>> @@ -635,6 +635,7 @@ SYM_INNER_LABEL_ALIGN(native_iret, SYM_L_GLOBAL)
>>          jnz     native_irq_return_ldt
>>   #endif
>>
>> +SYM_INNER_LABEL(xen_iret, SYM_L_WEAK) /* placeholder */
>>   SYM_INNER_LABEL(native_irq_return_iret, SYM_L_GLOBAL)
>>          /*
>>           * This may fault.  Non-paranoid faults on return to userspace are
>>
>> It will work when !CONFIG_XEN_PV
> 
> It pollutes namespace for no particular reason. I don't see it justified.
> 

I agree. IMO weak symbols should be avoided if easily possible, and the
patch here shows that it IS possible without any problem.


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 --]

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Lai Jiangshan]

On Fri, Nov 19, 2021 at 6:20 PM Kirill A. Shutemov <kirill@shutemov.name> wrote:

> > It is part of what CONFIG_PARAVIRT_XXL was designed for to enable
> > pv-aware INTERRUPT_RETURN.
>
> That's very vague statement.
>
> Could you elaborate on what is wrong with proposed fix?
>

Although CONFIG_PARAVIRT_XXL is enabled only when XEN, but they are
separated configs.  There is no wrong with the patch, but it is not
justified to narrow the scope of INTERRUPT_RETURN.

How about:

diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index cebec95a7124..4459f853f77b 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -746,10 +746,16 @@ extern void default_banner(void);
 #define PARA_SITE(ptype, ops)  _PVSITE(ptype, ops, .quad, 8)
 #define PARA_INDIRECT(addr)    *addr(%rip)

+#ifdef CONFIG_XEN_PV
+#define XEN_IRET        "jmp xen_iret;"
+#else
+#define XEN_IRET        ""
+#endif
+
 #define INTERRUPT_RETURN                                               \
        ANNOTATE_RETPOLINE_SAFE;                                        \
        ALTERNATIVE_TERNARY("jmp *paravirt_iret(%rip);",                \
-               X86_FEATURE_XENPV, "jmp xen_iret;", "jmp native_iret;")
+               X86_FEATURE_XENPV, XEN_IRET, "jmp native_iret;")

 #ifdef CONFIG_DEBUG_ENTRY
 .macro PARA_IRQ_save_fl

And in the patch, INTERRUPT_RETURN is moved at the definition
of SWAPGS, but SWAPGS is going to be removed:

https://lore.kernel.org/lkml/20211026143436.19071-2-jiangshanlai@gmail.com/

Re: [PATCH] x86/paravirt: Fix build PARAVIRT_XXL=y without XEN_PV

[Juergen Gross]

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

On 20.11.21 02:23, Lai Jiangshan wrote:
> On Fri, Nov 19, 2021 at 6:20 PM Kirill A. Shutemov <kirill@shutemov.name> wrote:
> 
>>> It is part of what CONFIG_PARAVIRT_XXL was designed for to enable
>>> pv-aware INTERRUPT_RETURN.
>>
>> That's very vague statement.
>>
>> Could you elaborate on what is wrong with proposed fix?
>>
> 
> Although CONFIG_PARAVIRT_XXL is enabled only when XEN, but they are
> separated configs.  There is no wrong with the patch, but it is not
> justified to narrow the scope of INTERRUPT_RETURN.

xen_iret is defined only with CONFIG_XEN_PV, so using that in
INTERRUPT_RETURN only in case CONFIG_XEN_PV is defined is absolutely
the right thing to do.

In case another user of INTERRUPT_RETURN would come up, it would
need to be changed again anyway.


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 --]