All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: "Liang, Kan" <kan.liang@intel.com>
Cc: "andi@firstfloor.org" <andi@firstfloor.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>
Subject: Re: [PATCH V4 1/2] perf ignore LBR and extra_regs.
Date: Thu, 10 Jul 2014 11:02:04 +0200	[thread overview]
Message-ID: <20140710090204.GU3935@laptop> (raw)
In-Reply-To: <37D7C6CF3E00A74B8858931C1DB2F077014D361B@SHSMSX103.ccr.corp.intel.com>

/me reminds you of 78 char text wrap.

On Wed, Jul 09, 2014 at 07:32:09PM +0000, Liang, Kan wrote:
> > Sure; but what I meant was, check_msr() is broken when ran on such a
> > kernel. You need to fix check_msr() to return failure on these 'ignored'
> > MSRs, after all they don't function as expected, they're effectively broken.
> 
> The function is designed to check if the MSRs can be safely accessed
> (no #GP). It cannot guarantee the correctness of the MSRs.  If KVM
> applied patch 2 and guest applied patch 1, from the guest's
> perspective, the MSRs can be accessed (no #GP triggered). So return
> true is expected. It should not be a broken. 

You're not understanding. I know you wrote that function to do that. I'm
saying that's wrong.

Look at check_hw_exists() it explicitly checks for fake MSRs and reports
them broken.

These fake MSRs _ARE_ broken, they do not behave as expected. Not
crashing is not the right consideration here, we're interested in higher
order correct behaviour.

> The only unexpected
> thing for guest is that the counting/sampling result for LBR/extra reg
> is always 0. But the patch is a short term fix to stop things from
> crashing. I think it should be acceptable.

Patch 2 is fine, patch 1, in particular your check_msr() routine is not.

      reply	other threads:[~2014-07-10  9:02 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-07-08 16:49 [PATCH V4 1/2] perf ignore LBR and extra_regs kan.liang
2014-07-08 16:49 ` [PATCH V4 2/2] kvm: " kan.liang
2014-07-09  8:32 ` [PATCH V4 1/2] perf " Peter Zijlstra
2014-07-09  9:14 ` Peter Zijlstra
2014-07-09  9:19 ` Peter Zijlstra
2014-07-09  9:27 ` Peter Zijlstra
2014-07-09 14:04   ` Liang, Kan
2014-07-09 14:16 ` Peter Zijlstra
2014-07-09 14:32   ` Liang, Kan
2014-07-09 14:58     ` Peter Zijlstra
2014-07-09 15:43       ` Liang, Kan
2014-07-09 16:41         ` Peter Zijlstra
2014-07-09 19:32           ` Liang, Kan
2014-07-10  9:02             ` Peter Zijlstra [this message]

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=20140710090204.GU3935@laptop \
    --to=peterz@infradead.org \
    --cc=andi@firstfloor.org \
    --cc=kan.liang@intel.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.