arch/x86/kvm/vmx/common.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
is_td() and is_td_vcpu() run in no instrumentation, so use __always_inline
to replace inline.
[1]
vmlinux.o: error: objtool: vmx_handle_nmi+0x47:
call to is_td_vcpu.isra.0() leaves .noinstr.text section
Fixes: 7172c753c26a ("KVM: VMX: Move common fields of struct vcpu_{vmx,tdx} to a struct")
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
V1 -> V2: using __always_inline to replace noinstr
arch/x86/kvm/vmx/common.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/vmx/common.h b/arch/x86/kvm/vmx/common.h
index 8f46a06e2c44..a0c5e8781c33 100644
--- a/arch/x86/kvm/vmx/common.h
+++ b/arch/x86/kvm/vmx/common.h
@@ -71,8 +71,8 @@ static __always_inline bool is_td_vcpu(struct kvm_vcpu *vcpu)
#else
-static inline bool is_td(struct kvm *kvm) { return false; }
-static inline bool is_td_vcpu(struct kvm_vcpu *vcpu) { return false; }
+static __always_inline bool is_td(struct kvm *kvm) { return false; }
+static __always_inline bool is_td_vcpu(struct kvm_vcpu *vcpu) { return false; }
#endif
--
2.43.0On Tue, 2025-05-27 at 16:44 +0800, Edward Adam Davis wrote:
> is_td() and is_td_vcpu() run in no instrumentation, so use __always_inline
> to replace inline.
>
> [1]
> vmlinux.o: error: objtool: vmx_handle_nmi+0x47:
> call to is_td_vcpu.isra.0() leaves .noinstr.text section
>
> Fixes: 7172c753c26a ("KVM: VMX: Move common fields of struct vcpu_{vmx,tdx} to a struct")
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
> V1 -> V2: using __always_inline to replace noinstr
Argh, for some reason the original report was sent just to Paolo and so I didn't
see this until now:
https://lore.kernel.org/oe-kbuild-all/202505071640.fUgzT6SF-lkp@intel.com/
You (or Paolo) might want to add that link for [1]. Fix looks good.
Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
On Tue, 2025-05-27 at 17:53 +0000, Edgecombe, Rick P wrote:
> On Tue, 2025-05-27 at 16:44 +0800, Edward Adam Davis wrote:
> > is_td() and is_td_vcpu() run in no instrumentation, so use __always_inline
> > to replace inline.
> >
> > [1]
> > vmlinux.o: error: objtool: vmx_handle_nmi+0x47:
> > call to is_td_vcpu.isra.0() leaves .noinstr.text section
> >
> > Fixes: 7172c753c26a ("KVM: VMX: Move common fields of struct vcpu_{vmx,tdx} to a struct")
> > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > ---
> > V1 -> V2: using __always_inline to replace noinstr
>
> Argh, for some reason the original report was sent just to Paolo and so I didn't
> see this until now:
> https://lore.kernel.org/oe-kbuild-all/202505071640.fUgzT6SF-lkp@intel.com/
>
> You (or Paolo) might want to add that link for [1]. Fix looks good.
>
> Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Also,
Reviewed-by: Kai Huang <kai.huang@intel.com>
On 5/28/2025 5:48 AM, Huang, Kai wrote:
> On Tue, 2025-05-27 at 17:53 +0000, Edgecombe, Rick P wrote:
>> On Tue, 2025-05-27 at 16:44 +0800, Edward Adam Davis wrote:
>>> is_td() and is_td_vcpu() run in no instrumentation, so use __always_inline
>>> to replace inline.
>>>
>>> [1]
>>> vmlinux.o: error: objtool: vmx_handle_nmi+0x47:
>>> call to is_td_vcpu.isra.0() leaves .noinstr.text section
>>>
>>> Fixes: 7172c753c26a ("KVM: VMX: Move common fields of struct vcpu_{vmx,tdx} to a struct")
>>> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
>>> ---
>>> V1 -> V2: using __always_inline to replace noinstr
>> Argh, for some reason the original report was sent just to Paolo and so I didn't
>> see this until now:
>> https://lore.kernel.org/oe-kbuild-all/202505071640.fUgzT6SF-lkp@intel.com/
>>
>> You (or Paolo) might want to add that link for [1]. Fix looks good.
>>
>> Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Also,
>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
Also,
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
On Tue, May 27, 2025 at 04:44:37PM +0800, Edward Adam Davis wrote:
> is_td() and is_td_vcpu() run in no instrumentation, so use __always_inline
> to replace inline.
>
> [1]
> vmlinux.o: error: objtool: vmx_handle_nmi+0x47:
> call to is_td_vcpu.isra.0() leaves .noinstr.text section
>
> Fixes: 7172c753c26a ("KVM: VMX: Move common fields of struct vcpu_{vmx,tdx} to a struct")
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
> V1 -> V2: using __always_inline to replace noinstr
>
> arch/x86/kvm/vmx/common.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/common.h b/arch/x86/kvm/vmx/common.h
> index 8f46a06e2c44..a0c5e8781c33 100644
> --- a/arch/x86/kvm/vmx/common.h
> +++ b/arch/x86/kvm/vmx/common.h
> @@ -71,8 +71,8 @@ static __always_inline bool is_td_vcpu(struct kvm_vcpu *vcpu)
>
> #else
>
> -static inline bool is_td(struct kvm *kvm) { return false; }
> -static inline bool is_td_vcpu(struct kvm_vcpu *vcpu) { return false; }
> +static __always_inline bool is_td(struct kvm *kvm) { return false; }
> +static __always_inline bool is_td_vcpu(struct kvm_vcpu *vcpu) { return false; }
>
> #endif
Right; this is the 'right' fix. Although the better fix would be for the
compiler to not be stupid :-)
On Tue, 2025-05-27 at 13:07 +0200, Peter Zijlstra wrote:
> On Tue, May 27, 2025 at 04:44:37PM +0800, Edward Adam Davis wrote:
> > is_td() and is_td_vcpu() run in no instrumentation, so use __always_inline
> > to replace inline.
> >
> > [1]
> > vmlinux.o: error: objtool: vmx_handle_nmi+0x47:
> > call to is_td_vcpu.isra.0() leaves .noinstr.text section
> >
> > Fixes: 7172c753c26a ("KVM: VMX: Move common fields of struct vcpu_{vmx,tdx} to a struct")
> > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > ---
> > V1 -> V2: using __always_inline to replace noinstr
> >
> > arch/x86/kvm/vmx/common.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/common.h b/arch/x86/kvm/vmx/common.h
> > index 8f46a06e2c44..a0c5e8781c33 100644
> > --- a/arch/x86/kvm/vmx/common.h
> > +++ b/arch/x86/kvm/vmx/common.h
> > @@ -71,8 +71,8 @@ static __always_inline bool is_td_vcpu(struct kvm_vcpu *vcpu)
> >
> > #else
> >
> > -static inline bool is_td(struct kvm *kvm) { return false; }
> > -static inline bool is_td_vcpu(struct kvm_vcpu *vcpu) { return false; }
> > +static __always_inline bool is_td(struct kvm *kvm) { return false; }
> > +static __always_inline bool is_td_vcpu(struct kvm_vcpu *vcpu) { return false; }
> >
> > #endif
>
> Right; this is the 'right' fix. Although the better fix would be for the
> compiler to not be stupid :-)
Hi Peter,
Just out of curiosity, I have a related question.
I just learned there's a 'flatten' attribute ('__flatten' in linux kernel)
supported by both gcc and clang. IIUC it forces all function calls inside one
function to be inlined if that function is annotated with this attribute.
However, it seems gcc and clang handles "recursive inlining" differently. gcc
seems supports recursive inlining with flatten, but clang seems not.
This is the gcc doc [1] says, which explicitly tells recursive inlining is
supported IIUC:
flatten
Generally, inlining into a function is limited. For a function marked with
this attribute, every call inside this function is inlined including the calls
such inlining introduces to the function (but not recursive calls to the
function itself), if possible.
And this is the clang doc [2] says, which doesn't say about recursive inlining:
flatten
The flatten attribute causes calls within the attributed function to be
inlined unless it is impossible to do so, for example if the body of the
callee is unavailable or if the callee has the noinline attribute.
Also, one "AI Overview" provided by google also says below:
Compiler Behavior:
While GCC supports recursive inlining with flatten, other compilers like
Clang might only perform a single level of inlining.
Just wondering whether you can happen to confirm this?
That also being said, if the __flatten could always be "recursive inlining", it
seems to me that __flatten would be a better annotation when we want some
function to be noinstr. But if it's behaviour is compiler dependent, it seems
it's not a good idea to use it.
What's your opinion on this?
[1]: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html
[2]: https://clang.llvm.org/docs/AttributeReference.html
On Tue, May 27, 2025 at 12:34:07PM +0000, Huang, Kai wrote:
> On Tue, 2025-05-27 at 13:07 +0200, Peter Zijlstra wrote:
> > On Tue, May 27, 2025 at 04:44:37PM +0800, Edward Adam Davis wrote:
> > > is_td() and is_td_vcpu() run in no instrumentation, so use __always_inline
> > > to replace inline.
> > >
> > > [1]
> > > vmlinux.o: error: objtool: vmx_handle_nmi+0x47:
> > > call to is_td_vcpu.isra.0() leaves .noinstr.text section
> > >
> > > Fixes: 7172c753c26a ("KVM: VMX: Move common fields of struct vcpu_{vmx,tdx} to a struct")
> > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > ---
> > > V1 -> V2: using __always_inline to replace noinstr
> > >
> > > arch/x86/kvm/vmx/common.h | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/vmx/common.h b/arch/x86/kvm/vmx/common.h
> > > index 8f46a06e2c44..a0c5e8781c33 100644
> > > --- a/arch/x86/kvm/vmx/common.h
> > > +++ b/arch/x86/kvm/vmx/common.h
> > > @@ -71,8 +71,8 @@ static __always_inline bool is_td_vcpu(struct kvm_vcpu *vcpu)
> > >
> > > #else
> > >
> > > -static inline bool is_td(struct kvm *kvm) { return false; }
> > > -static inline bool is_td_vcpu(struct kvm_vcpu *vcpu) { return false; }
> > > +static __always_inline bool is_td(struct kvm *kvm) { return false; }
> > > +static __always_inline bool is_td_vcpu(struct kvm_vcpu *vcpu) { return false; }
> > >
> > > #endif
> >
> > Right; this is the 'right' fix. Although the better fix would be for the
> > compiler to not be stupid :-)
FWIW, the thing that typically happens is that the compiler first
inserts instrumentation (think *SAN) into the trivial stub function and
then figures its too big to inline.
> Hi Peter,
>
> Just out of curiosity, I have a related question.
>
> I just learned there's a 'flatten' attribute ('__flatten' in linux kernel)
> supported by both gcc and clang. IIUC it forces all function calls inside one
> function to be inlined if that function is annotated with this attribute.
>
> However, it seems gcc and clang handles "recursive inlining" differently. gcc
> seems supports recursive inlining with flatten, but clang seems not.
>
> This is the gcc doc [1] says, which explicitly tells recursive inlining is
> supported IIUC:
>
> flatten
>
> Generally, inlining into a function is limited. For a function marked with
> this attribute, every call inside this function is inlined including the calls
> such inlining introduces to the function (but not recursive calls to the
> function itself), if possible.
>
> And this is the clang doc [2] says, which doesn't say about recursive inlining:
>
> flatten
>
> The flatten attribute causes calls within the attributed function to be
> inlined unless it is impossible to do so, for example if the body of the
> callee is unavailable or if the callee has the noinline attribute.
>
> Also, one "AI Overview" provided by google also says below:
>
> Compiler Behavior:
> While GCC supports recursive inlining with flatten, other compilers like
> Clang might only perform a single level of inlining.
>
> Just wondering whether you can happen to confirm this?
>
> That also being said, if the __flatten could always be "recursive inlining", it
> seems to me that __flatten would be a better annotation when we want some
> function to be noinstr. But if it's behaviour is compiler dependent, it seems
> it's not a good idea to use it.
>
> What's your opinion on this?
I am somewhat conflicted on this; using __flatten, while convenient,
would take away the immediate insight into what gets pulled in. Having
to explicitly mark functions with __always_inline is somewhat
inconvenient, but at least you don't pull in stuff by accident.
On Tue, 2025-05-27 at 15:47 +0200, Peter Zijlstra wrote:
> On Tue, May 27, 2025 at 12:34:07PM +0000, Huang, Kai wrote:
> > On Tue, 2025-05-27 at 13:07 +0200, Peter Zijlstra wrote:
> > > On Tue, May 27, 2025 at 04:44:37PM +0800, Edward Adam Davis wrote:
> > > > is_td() and is_td_vcpu() run in no instrumentation, so use __always_inline
> > > > to replace inline.
> > > >
> > > > [1]
> > > > vmlinux.o: error: objtool: vmx_handle_nmi+0x47:
> > > > call to is_td_vcpu.isra.0() leaves .noinstr.text section
> > > >
> > > > Fixes: 7172c753c26a ("KVM: VMX: Move common fields of struct vcpu_{vmx,tdx} to a struct")
> > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > > ---
> > > > V1 -> V2: using __always_inline to replace noinstr
> > > >
> > > > arch/x86/kvm/vmx/common.h | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/x86/kvm/vmx/common.h b/arch/x86/kvm/vmx/common.h
> > > > index 8f46a06e2c44..a0c5e8781c33 100644
> > > > --- a/arch/x86/kvm/vmx/common.h
> > > > +++ b/arch/x86/kvm/vmx/common.h
> > > > @@ -71,8 +71,8 @@ static __always_inline bool is_td_vcpu(struct kvm_vcpu *vcpu)
> > > >
> > > > #else
> > > >
> > > > -static inline bool is_td(struct kvm *kvm) { return false; }
> > > > -static inline bool is_td_vcpu(struct kvm_vcpu *vcpu) { return false; }
> > > > +static __always_inline bool is_td(struct kvm *kvm) { return false; }
> > > > +static __always_inline bool is_td_vcpu(struct kvm_vcpu *vcpu) { return false; }
> > > >
> > > > #endif
> > >
> > > Right; this is the 'right' fix. Although the better fix would be for the
> > > compiler to not be stupid :-)
>
> FWIW, the thing that typically happens is that the compiler first
> inserts instrumentation (think *SAN) into the trivial stub function and
> then figures its too big to inline.
This is helpful. Thanks!
>
> > Hi Peter,
> >
> > Just out of curiosity, I have a related question.
> >
> > I just learned there's a 'flatten' attribute ('__flatten' in linux kernel)
> > supported by both gcc and clang. IIUC it forces all function calls inside one
> > function to be inlined if that function is annotated with this attribute.
> >
> > However, it seems gcc and clang handles "recursive inlining" differently. gcc
> > seems supports recursive inlining with flatten, but clang seems not.
> >
> > This is the gcc doc [1] says, which explicitly tells recursive inlining is
> > supported IIUC:
> >
> > flatten
> >
> > Generally, inlining into a function is limited. For a function marked with
> > this attribute, every call inside this function is inlined including the calls
> > such inlining introduces to the function (but not recursive calls to the
> > function itself), if possible.
> >
> > And this is the clang doc [2] says, which doesn't say about recursive inlining:
> >
> > flatten
> >
> > The flatten attribute causes calls within the attributed function to be
> > inlined unless it is impossible to do so, for example if the body of the
> > callee is unavailable or if the callee has the noinline attribute.
> >
> > Also, one "AI Overview" provided by google also says below:
> >
> > Compiler Behavior:
> > While GCC supports recursive inlining with flatten, other compilers like
> > Clang might only perform a single level of inlining.
> >
> > Just wondering whether you can happen to confirm this?
> >
> > That also being said, if the __flatten could always be "recursive inlining", it
> > seems to me that __flatten would be a better annotation when we want some
> > function to be noinstr. But if it's behaviour is compiler dependent, it seems
> > it's not a good idea to use it.
> >
> > What's your opinion on this?
>
> I am somewhat conflicted on this; using __flatten, while convenient,
> would take away the immediate insight into what gets pulled in. Having
> to explicitly mark functions with __always_inline is somewhat
> inconvenient, but at least you don't pull in stuff by accident.
Yeah, thanks anyway for the insight.
© 2016 - 2025 Red Hat, Inc.