xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.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:04:34 +0100	[thread overview]
Message-ID: <e386f7eb-8ae8-2f18-b9e8-30a319354ec0@citrix.com> (raw)
In-Reply-To: <135f8bc2-d07c-c531-e498-9f0e15207cca@suse.com>

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

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


  reply	other threads:[~2021-04-23 11:05 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
2021-04-23 11:04           ` Andrew Cooper [this message]
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:

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

  git send-email \
    --in-reply-to=e386f7eb-8ae8-2f18-b9e8-30a319354ec0@citrix.com \
    --to=andrew.cooper3@citrix.com \
    --cc=jbeulich@suse.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' \


* 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).