xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Wei Liu" <wl@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH] x86/oprof: fix !HVM && !PV32 build
Date: Fri, 23 Apr 2021 12:58:16 +0200	[thread overview]
Message-ID: <135f8bc2-d07c-c531-e498-9f0e15207cca@suse.com> (raw)
In-Reply-To: <98d0b627-de38-f7a0-7487-cbf83864c82f@citrix.com>

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


  reply	other threads:[~2021-04-23 10:58 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-16  8:16 Jan Beulich
2021-04-16 13:41 ` Andrew Cooper
2021-04-16 14:20   ` Jan Beulich
2021-04-23  9:50     ` Roger Pau Monné
2021-04-23 10:51       ` Andrew Cooper
2021-04-23 10:58         ` Jan Beulich [this message]
2021-04-23 11:04           ` Andrew Cooper
2021-04-23 11:08             ` Jan Beulich
2021-04-23 13:50               ` Andrew Cooper

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=135f8bc2-d07c-c531-e498-9f0e15207cca@suse.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=roger.pau@citrix.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [PATCH] x86/oprof: fix '\!'HVM && '\!'PV32 build' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).