Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 8 May 2020 15:09:31 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Like Xu <like.xu@linux.intel.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, wei.w.wang@intel.com,
        ak@linux.intel.com
Subject: Re: [PATCH v10 08/11] KVM: x86/pmu: Add LBR feature emulation via
 guest LBR event
Message-ID: <20200508130931.GE5298@hirez.programming.kicks-ass.net>
References: <20200423081412.164863-1-like.xu@linux.intel.com>
 <20200423081412.164863-9-like.xu@linux.intel.com>
 <20200424121626.GB20730@hirez.programming.kicks-ass.net>
 <87abf620-d292-d997-c9be-9a5d2544f3fa@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <87abf620-d292-d997-c9be-9a5d2544f3fa@linux.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Apr 27, 2020 at 11:16:40AM +0800, Like Xu wrote:
> On 2020/4/24 20:16, Peter Zijlstra wrote:

> > And I suppose that is why you need that horrible:
> > needs_guest_lbr_without_counter() thing to begin with.
> 
> Do you suggest to use event->attr.config check to replace
> "needs_branch_stack(event) && is_kernel_event(event) &&
> event->attr.exclude_host" check for guest LBR event ?

That's what the BTS thing does.

> > Please allocate yourself an event from the pseudo event range:
> > event==0x00. Currently we only have umask==3 for Fixed2 and umask==4
> > for Fixed3, given you claim 58, which is effectively Fixed25,
> > umask==0x1a might be appropriate.
> 
> OK, I assume that adding one more field ".config = 0x1a00" is
> efficient enough for perf_event_attr to allocate guest LBR events.

Uh what? The code is already setting .config. You just have to change it
do another value.

> > Also, I suppose we need to claim 0x0000 as an error, so that other
> > people won't try this again.
> 
> Does the following fix address your concern on this ?
> 
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 2405926e2dba..32d2a3f8c51f 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -498,6 +498,9 @@ int x86_pmu_max_precise(void)
> 
>  int x86_pmu_hw_config(struct perf_event *event)
>  {
> +       if (!unlikely(event->attr.config & X86_ARCH_EVENT_MASK))
> +               return -EINVAL;
> +
>         if (event->attr.precise_ip) {
>                 int precise = x86_pmu_max_precise();

That wouldn't work right for AMD. But yes, something like that.

> > Also, what happens if you fail programming due to a conflicting cpu
> > event? That pinned doesn't guarantee you'll get the event, it just means
> > you'll error instead of getting RR.
> > 
> > I didn't find any code checking the event state.
> > 
> 
> Error instead of RR is expected.
> 
> If the KVM fails programming due to a conflicting cpu event
> the LBR registers will not be passthrough to the guest,
> and KVM would return zero for any guest LBR records accesses
> until the next attempt to program the guest LBR event.
> 
> Every time before cpu enters the non-root mode where irq is
> disabled, the "event-> oncpu! =-1" check will be applied.
> (more details in the comment around intel_pmu_availability_check())
> 
> The guests administer is supposed to know the result of guest
> LBR records is inaccurate if someone is using LBR to record
> guest or hypervisor on the host side.
> 
> Is this acceptable to you？
> 
> If there is anything needs to be improved, please let me know.

It might be nice to emit a pr_warn() or something on the host when this
happens. Then at least the host admin can know he wrecked things for
which guest.