1. Patchew
  2. Xen
  3. View series

[PATCH] x86/oprof: fix !HVM && !PV32 build

Jan Beulich posted 1 patch 3 years ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/694c9c98-1197-3378-cc43-235e2609c1dd@suse.com
[PATCH] x86/oprof: fix !HVM && !PV32 build
Posted by Jan Beulich 3 years ago 
clang, at the very least, doesn't like unused inline functions, unless
their definitions live in a header.

Fixes: d23d792478 ("x86: avoid building COMPAT code when !HVM && !PV32")
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/oprofile/backtrace.c
+++ b/xen/arch/x86/oprofile/backtrace.c
@@ -43,6 +43,7 @@ dump_hypervisor_backtrace(struct vcpu *v
     return head->ebp;
 }
 
+#ifdef CONFIG_COMPAT
 static inline int is_32bit_vcpu(struct vcpu *vcpu)
 {
     if (is_hvm_vcpu(vcpu))
@@ -50,6 +51,7 @@ static inline int is_32bit_vcpu(struct v
     else
         return is_pv_32bit_vcpu(vcpu);
 }
+#endif
 
 static struct frame_head *
 dump_guest_backtrace(struct vcpu *vcpu, const struct frame_head *head,
Re: [PATCH] x86/oprof: fix !HVM && !PV32 build
Posted by Andrew Cooper 3 years ago 
On 16/04/2021 09:16, Jan Beulich wrote:
> clang, at the very least, doesn't like unused inline functions, unless
> their definitions live in a header.
>
> Fixes: d23d792478 ("x86: avoid building COMPAT code when !HVM && !PV32")
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I agree this will fix the build.  However, looking at the code, I'm not
sure the original CONFIG_COMPAT was correct.  In particular, ...

>
> --- a/xen/arch/x86/oprofile/backtrace.c
> +++ b/xen/arch/x86/oprofile/backtrace.c
> @@ -43,6 +43,7 @@ dump_hypervisor_backtrace(struct vcpu *v
>      return head->ebp;
>  }
>  
> +#ifdef CONFIG_COMPAT
>  static inline int is_32bit_vcpu(struct vcpu *vcpu)
>  {
>      if (is_hvm_vcpu(vcpu))

... this chunk of logic demonstrates that what oprofile is doing isn't
related to the Xen ABI in the slightest.

I think OProfile is misusing the guest handle infrastructure, and
shouldn't be using it for this task.

~Andrew

> @@ -50,6 +51,7 @@ static inline int is_32bit_vcpu(struct v
>      else
>          return is_pv_32bit_vcpu(vcpu);
>  }
> +#endif
Re: [PATCH] x86/oprof: fix !HVM && !PV32 build
Posted by Jan Beulich 3 years ago 
On 16.04.2021 15:41, Andrew Cooper wrote:
> On 16/04/2021 09:16, Jan Beulich wrote:
>> clang, at the very least, doesn't like unused inline functions, unless
>> their definitions live in a header.
>>
>> Fixes: d23d792478 ("x86: avoid building COMPAT code when !HVM && !PV32")
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I agree this will fix the build.  However, looking at the code, I'm not
> sure the original CONFIG_COMPAT was correct.  In particular, ...
> 
>>
>> --- a/xen/arch/x86/oprofile/backtrace.c
>> +++ b/xen/arch/x86/oprofile/backtrace.c
>> @@ -43,6 +43,7 @@ dump_hypervisor_backtrace(struct vcpu *v
>>      return head->ebp;
>>  }
>>  
>> +#ifdef CONFIG_COMPAT
>>  static inline int is_32bit_vcpu(struct vcpu *vcpu)
>>  {
>>      if (is_hvm_vcpu(vcpu))
> 
> ... this chunk of logic demonstrates that what oprofile is doing isn't
> related to the Xen ABI in the slightest.
> 
> I think OProfile is misusing the guest handle infrastructure, and
> shouldn't be using it for this task.

I'm afraid I consider this something for another day. Both the
original #ifdef and the one getting added here are merely
measures to get things to build.

Jan
Re: [PATCH] x86/oprof: fix !HVM && !PV32 build
Posted by Roger Pau Monné 3 years ago 
On Fri, Apr 16, 2021 at 04:20:59PM +0200, Jan Beulich wrote:
> On 16.04.2021 15:41, Andrew Cooper wrote:
> > On 16/04/2021 09:16, Jan Beulich wrote:
> >> clang, at the very least, doesn't like unused inline functions, unless
> >> their definitions live in a header.
> >>
> >> Fixes: d23d792478 ("x86: avoid building COMPAT code when !HVM && !PV32")
> >> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > I agree this will fix the build.  However, looking at the code, I'm not
> > sure the original CONFIG_COMPAT was correct.  In particular, ...
> > 
> >>
> >> --- a/xen/arch/x86/oprofile/backtrace.c
> >> +++ b/xen/arch/x86/oprofile/backtrace.c
> >> @@ -43,6 +43,7 @@ dump_hypervisor_backtrace(struct vcpu *v
> >>      return head->ebp;
> >>  }
> >>  
> >> +#ifdef CONFIG_COMPAT
> >>  static inline int is_32bit_vcpu(struct vcpu *vcpu)
> >>  {
> >>      if (is_hvm_vcpu(vcpu))
> > 
> > ... this chunk of logic demonstrates that what oprofile is doing isn't
> > related to the Xen ABI in the slightest.
> > 
> > I think OProfile is misusing the guest handle infrastructure, and
> > shouldn't be using it for this task.
> 
> I'm afraid I consider this something for another day. Both the
> original #ifdef and the one getting added here are merely
> measures to get things to build.

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Without entering on the debate whether CONFIG_COMPAT is the correct
conditional to use it's not making the issue any worse, and it will
allow to unblock the build. We can discuss about the CONFIG_COMPAT
stuff later.

Thanks, Roger.
Re: [PATCH] x86/oprof: fix !HVM && !PV32 build
Posted by Andrew Cooper 3 years ago 
On 23/04/2021 10:50, Roger Pau Monné wrote:
> On Fri, Apr 16, 2021 at 04:20:59PM +0200, Jan Beulich wrote:
>> On 16.04.2021 15:41, Andrew Cooper wrote:
>>> On 16/04/2021 09:16, Jan Beulich wrote:
>>>> clang, at the very least, doesn't like unused inline functions, unless
>>>> their definitions live in a header.
>>>>
>>>> Fixes: d23d792478 ("x86: avoid building COMPAT code when !HVM && !PV32")
>>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> I agree this will fix the build.  However, looking at the code, I'm not
>>> sure the original CONFIG_COMPAT was correct.  In particular, ...
>>>
>>>> --- a/xen/arch/x86/oprofile/backtrace.c
>>>> +++ b/xen/arch/x86/oprofile/backtrace.c
>>>> @@ -43,6 +43,7 @@ dump_hypervisor_backtrace(struct vcpu *v
>>>>      return head->ebp;
>>>>  }
>>>>  
>>>> +#ifdef CONFIG_COMPAT
>>>>  static inline int is_32bit_vcpu(struct vcpu *vcpu)
>>>>  {
>>>>      if (is_hvm_vcpu(vcpu))
>>> ... this chunk of logic demonstrates that what oprofile is doing isn't
>>> related to the Xen ABI in the slightest.
>>>
>>> I think OProfile is misusing the guest handle infrastructure, and
>>> shouldn't be using it for this task.
>> I'm afraid I consider this something for another day. Both the
>> original #ifdef and the one getting added here are merely
>> measures to get things to build.
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Without entering on the debate whether CONFIG_COMPAT is the correct
> conditional to use it's not making the issue any worse, and it will
> allow to unblock the build. We can discuss about the CONFIG_COMPAT
> stuff later.

I disagree.  Fixing this less effort than the time wasted arguing about
fixing it.

But if you are going to insist on not fixing it, and putting in a patch
like this, then at a minimum, it needs to include a TODO comment stating
that the use of CONFIG_COMPAT is bogus and needs fixing.

~Andrew
Re: [PATCH] x86/oprof: fix !HVM && !PV32 build
Posted by Jan Beulich 3 years ago 
On 23.04.2021 12:51, Andrew Cooper wrote:
> On 23/04/2021 10:50, Roger Pau Monné wrote:
>> On Fri, Apr 16, 2021 at 04:20:59PM +0200, Jan Beulich wrote:
>>> On 16.04.2021 15:41, Andrew Cooper wrote:
>>>> On 16/04/2021 09:16, Jan Beulich wrote:
>>>>> clang, at the very least, doesn't like unused inline functions, unless
>>>>> their definitions live in a header.
>>>>>
>>>>> Fixes: d23d792478 ("x86: avoid building COMPAT code when !HVM && !PV32")
>>>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> I agree this will fix the build.  However, looking at the code, I'm not
>>>> sure the original CONFIG_COMPAT was correct.  In particular, ...
>>>>
>>>>> --- a/xen/arch/x86/oprofile/backtrace.c
>>>>> +++ b/xen/arch/x86/oprofile/backtrace.c
>>>>> @@ -43,6 +43,7 @@ dump_hypervisor_backtrace(struct vcpu *v
>>>>>      return head->ebp;
>>>>>  }
>>>>>  
>>>>> +#ifdef CONFIG_COMPAT
>>>>>  static inline int is_32bit_vcpu(struct vcpu *vcpu)
>>>>>  {
>>>>>      if (is_hvm_vcpu(vcpu))
>>>> ... this chunk of logic demonstrates that what oprofile is doing isn't
>>>> related to the Xen ABI in the slightest.
>>>>
>>>> I think OProfile is misusing the guest handle infrastructure, and
>>>> shouldn't be using it for this task.
>>> I'm afraid I consider this something for another day. Both the
>>> original #ifdef and the one getting added here are merely
>>> measures to get things to build.
>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Without entering on the debate whether CONFIG_COMPAT is the correct
>> conditional to use it's not making the issue any worse, and it will
>> allow to unblock the build. We can discuss about the CONFIG_COMPAT
>> stuff later.
> 
> I disagree.  Fixing this less effort than the time wasted arguing about
> fixing it.
> 
> But if you are going to insist on not fixing it, and putting in a patch
> like this, then at a minimum, it needs to include a TODO comment stating
> that the use of CONFIG_COMPAT is bogus and needs fixing.

I disagree: It is (for now) just you saying this is bogus. The (ab)use
of the handle infrastructure was there before. You could have sent a
fix long ago, therefore, if you were thinking this needs fixing. I can
see that you have good intentions, but orthogonal issues shouldn't be
used to block necessary adjustments (and this applies to other pending
build fixes as well).

Jan
Re: [PATCH] x86/oprof: fix !HVM && !PV32 build
Posted by Andrew Cooper 3 years ago 
On 23/04/2021 11:58, Jan Beulich wrote:
> On 23.04.2021 12:51, Andrew Cooper wrote:
>> On 23/04/2021 10:50, Roger Pau Monné wrote:
>>> On Fri, Apr 16, 2021 at 04:20:59PM +0200, Jan Beulich wrote:
>>>> On 16.04.2021 15:41, Andrew Cooper wrote:
>>>>> On 16/04/2021 09:16, Jan Beulich wrote:
>>>>>> clang, at the very least, doesn't like unused inline functions, unless
>>>>>> their definitions live in a header.
>>>>>>
>>>>>> Fixes: d23d792478 ("x86: avoid building COMPAT code when !HVM && !PV32")
>>>>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> I agree this will fix the build.  However, looking at the code, I'm not
>>>>> sure the original CONFIG_COMPAT was correct.  In particular, ...
>>>>>
>>>>>> --- a/xen/arch/x86/oprofile/backtrace.c
>>>>>> +++ b/xen/arch/x86/oprofile/backtrace.c
>>>>>> @@ -43,6 +43,7 @@ dump_hypervisor_backtrace(struct vcpu *v
>>>>>>      return head->ebp;
>>>>>>  }
>>>>>>  
>>>>>> +#ifdef CONFIG_COMPAT
>>>>>>  static inline int is_32bit_vcpu(struct vcpu *vcpu)
>>>>>>  {
>>>>>>      if (is_hvm_vcpu(vcpu))
>>>>> ... this chunk of logic demonstrates that what oprofile is doing isn't
>>>>> related to the Xen ABI in the slightest.
>>>>>
>>>>> I think OProfile is misusing the guest handle infrastructure, and
>>>>> shouldn't be using it for this task.
>>>> I'm afraid I consider this something for another day. Both the
>>>> original #ifdef and the one getting added here are merely
>>>> measures to get things to build.
>>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>>
>>> Without entering on the debate whether CONFIG_COMPAT is the correct
>>> conditional to use it's not making the issue any worse, and it will
>>> allow to unblock the build. We can discuss about the CONFIG_COMPAT
>>> stuff later.
>> I disagree.  Fixing this less effort than the time wasted arguing about
>> fixing it.
>>
>> But if you are going to insist on not fixing it, and putting in a patch
>> like this, then at a minimum, it needs to include a TODO comment stating
>> that the use of CONFIG_COMPAT is bogus and needs fixing.
> I disagree: It is (for now) just you saying this is bogus. The (ab)use
> of the handle infrastructure was there before. You could have sent a
> fix long ago, therefore, if you were thinking this needs fixing.

I only know it needed fixing because you didn't build test your change
in CI.  Don't make it out to be my fault I didn't spot this 6 months ago.

> I can
> see that you have good intentions, but orthogonal issues shouldn't be
> used to block necessary adjustments (and this applies to other pending
> build fixes as well).

You genuinely regressed things for 32bit HVM guests, with the
CONFIG_COMPAT change.

The code may have been using inappropriate interfaces to perform its job
before, but its actually broken now.

~Andrew
Re: [PATCH] x86/oprof: fix !HVM && !PV32 build
Posted by Jan Beulich 3 years ago 
On 23.04.2021 13:04, Andrew Cooper wrote:
> On 23/04/2021 11:58, Jan Beulich wrote:
>> On 23.04.2021 12:51, Andrew Cooper wrote:
>>> On 23/04/2021 10:50, Roger Pau Monné wrote:
>>>> On Fri, Apr 16, 2021 at 04:20:59PM +0200, Jan Beulich wrote:
>>>>> On 16.04.2021 15:41, Andrew Cooper wrote:
>>>>>> On 16/04/2021 09:16, Jan Beulich wrote:
>>>>>>> clang, at the very least, doesn't like unused inline functions, unless
>>>>>>> their definitions live in a header.
>>>>>>>
>>>>>>> Fixes: d23d792478 ("x86: avoid building COMPAT code when !HVM && !PV32")
>>>>>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>> I agree this will fix the build.  However, looking at the code, I'm not
>>>>>> sure the original CONFIG_COMPAT was correct.  In particular, ...
>>>>>>
>>>>>>> --- a/xen/arch/x86/oprofile/backtrace.c
>>>>>>> +++ b/xen/arch/x86/oprofile/backtrace.c
>>>>>>> @@ -43,6 +43,7 @@ dump_hypervisor_backtrace(struct vcpu *v
>>>>>>>      return head->ebp;
>>>>>>>  }
>>>>>>>  
>>>>>>> +#ifdef CONFIG_COMPAT
>>>>>>>  static inline int is_32bit_vcpu(struct vcpu *vcpu)
>>>>>>>  {
>>>>>>>      if (is_hvm_vcpu(vcpu))
>>>>>> ... this chunk of logic demonstrates that what oprofile is doing isn't
>>>>>> related to the Xen ABI in the slightest.
>>>>>>
>>>>>> I think OProfile is misusing the guest handle infrastructure, and
>>>>>> shouldn't be using it for this task.
>>>>> I'm afraid I consider this something for another day. Both the
>>>>> original #ifdef and the one getting added here are merely
>>>>> measures to get things to build.
>>>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>
>>>> Without entering on the debate whether CONFIG_COMPAT is the correct
>>>> conditional to use it's not making the issue any worse, and it will
>>>> allow to unblock the build. We can discuss about the CONFIG_COMPAT
>>>> stuff later.
>>> I disagree.  Fixing this less effort than the time wasted arguing about
>>> fixing it.
>>>
>>> But if you are going to insist on not fixing it, and putting in a patch
>>> like this, then at a minimum, it needs to include a TODO comment stating
>>> that the use of CONFIG_COMPAT is bogus and needs fixing.
>> I disagree: It is (for now) just you saying this is bogus. The (ab)use
>> of the handle infrastructure was there before. You could have sent a
>> fix long ago, therefore, if you were thinking this needs fixing.
> 
> I only know it needed fixing because you didn't build test your change
> in CI.  Don't make it out to be my fault I didn't spot this 6 months ago.
> 
>> I can
>> see that you have good intentions, but orthogonal issues shouldn't be
>> used to block necessary adjustments (and this applies to other pending
>> build fixes as well).
> 
> You genuinely regressed things for 32bit HVM guests, with the
> CONFIG_COMPAT change.
> 
> The code may have been using inappropriate interfaces to perform its job
> before, but its actually broken now.

In which way? COMPAT gets selected by both PV32 and HVM.

Jan
Re: [PATCH] x86/oprof: fix !HVM && !PV32 build
Posted by Andrew Cooper 3 years ago 
On 23/04/2021 12:08, Jan Beulich wrote:
> On 23.04.2021 13:04, Andrew Cooper wrote:
>> On 23/04/2021 11:58, Jan Beulich wrote:
>>> On 23.04.2021 12:51, Andrew Cooper wrote:
>>>> On 23/04/2021 10:50, Roger Pau Monné wrote:
>>>>> On Fri, Apr 16, 2021 at 04:20:59PM +0200, Jan Beulich wrote:
>>>>>> On 16.04.2021 15:41, Andrew Cooper wrote:
>>>>>>> On 16/04/2021 09:16, Jan Beulich wrote:
>>>>>>>> clang, at the very least, doesn't like unused inline functions, unless
>>>>>>>> their definitions live in a header.
>>>>>>>>
>>>>>>>> Fixes: d23d792478 ("x86: avoid building COMPAT code when !HVM && !PV32")
>>>>>>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>> I agree this will fix the build.  However, looking at the code, I'm not
>>>>>>> sure the original CONFIG_COMPAT was correct.  In particular, ...
>>>>>>>
>>>>>>>> --- a/xen/arch/x86/oprofile/backtrace.c
>>>>>>>> +++ b/xen/arch/x86/oprofile/backtrace.c
>>>>>>>> @@ -43,6 +43,7 @@ dump_hypervisor_backtrace(struct vcpu *v
>>>>>>>>      return head->ebp;
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> +#ifdef CONFIG_COMPAT
>>>>>>>>  static inline int is_32bit_vcpu(struct vcpu *vcpu)
>>>>>>>>  {
>>>>>>>>      if (is_hvm_vcpu(vcpu))
>>>>>>> ... this chunk of logic demonstrates that what oprofile is doing isn't
>>>>>>> related to the Xen ABI in the slightest.
>>>>>>>
>>>>>>> I think OProfile is misusing the guest handle infrastructure, and
>>>>>>> shouldn't be using it for this task.
>>>>>> I'm afraid I consider this something for another day. Both the
>>>>>> original #ifdef and the one getting added here are merely
>>>>>> measures to get things to build.
>>>>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>>
>>>>> Without entering on the debate whether CONFIG_COMPAT is the correct
>>>>> conditional to use it's not making the issue any worse, and it will
>>>>> allow to unblock the build. We can discuss about the CONFIG_COMPAT
>>>>> stuff later.
>>>> I disagree.  Fixing this less effort than the time wasted arguing about
>>>> fixing it.
>>>>
>>>> But if you are going to insist on not fixing it, and putting in a patch
>>>> like this, then at a minimum, it needs to include a TODO comment stating
>>>> that the use of CONFIG_COMPAT is bogus and needs fixing.
>>> I disagree: It is (for now) just you saying this is bogus. The (ab)use
>>> of the handle infrastructure was there before. You could have sent a
>>> fix long ago, therefore, if you were thinking this needs fixing.
>> I only know it needed fixing because you didn't build test your change
>> in CI.  Don't make it out to be my fault I didn't spot this 6 months ago.
>>
>>> I can
>>> see that you have good intentions, but orthogonal issues shouldn't be
>>> used to block necessary adjustments (and this applies to other pending
>>> build fixes as well).
>> You genuinely regressed things for 32bit HVM guests, with the
>> CONFIG_COMPAT change.
>>
>> The code may have been using inappropriate interfaces to perform its job
>> before, but its actually broken now.
> In which way? COMPAT gets selected by both PV32 and HVM.

Hmm ok - with the select in place, I accept that it is only a problem in
principle.

~Andrew

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.