Received: by 2002:a25:ef43:0:0:0:0:0 with SMTP id w3csp614053ybm; Wed, 27 May 2020 03:52:20 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwqcajk4palvV4VMmgGLFLJp2cOpojLpz+7xBtt1UoSAjcdZWrTV87w9TdqHrPxDxT+lzhi X-Received: by 2002:a17:906:15cb:: with SMTP id l11mr5219438ejd.224.1590576740421; Wed, 27 May 2020 03:52:20 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1590576740; cv=none; d=google.com; s=arc-20160816; b=BFWYEICL42H6Zf+4gQSUsTlATHnpkGdL+qNP8DGfkN3z+/tW+oANUhKU9gNiaOtKKn 30nbkbttxkFql5OTBQ6ger/8IPE8cyW8cR/BlmqKUGKitqOUGGbF+vIhc2d2cMrW4Grn RqhA3fNNbMM0BOTaTGClpcn+K35NsoyOMTYB2jaS6+AxNtKiSnVvaM7Y1wSipHI7BknB mxCRUAnjzU6/9Q1RuMQkqC95JcBfKJc8H/1WqHNZnywS38Qano8yPSB13gvdtIfWtVhg THfrxCUmDmEEfSiwRWOeObgc0XzJfCcXiKKvmzKJ8mGpsZniyxyWpWS41e3q9ZFoxetc uVLA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:organization:from:references:cc:to:subject:ironport-sdr :ironport-sdr; bh=Y/tSoN2djdQCIyZ0Y1GoYcFTw8QGEwzGtUIlQhMt75o=; b=dnFIg3f6W4qDLWeDjxASUgmWbL7Oy17lPXDOSGoOfdgLpsKX9c0Xvmlh13dXXRxuvY nA6X4jrqlgNC4IXgg1v11kC6l+AFoMja3PdtTom+VuW2zrjFtcljLAXXI+51RzVwyTl9 aYaSL4N4ksbvs5Ch9Py96VYj6FRrZ6CNVnPkGu5zjnUmbZBfT9RQEV66p7qaE6k+pAqR 9uJClFRbvJVReEJSE3sX3udKqOwvuTI7W99gTfM50EdHY137FlXoUsrHkjthmdJ47dAN 03KReXVMq6vnZ2rp+FzkSVpqAeM4QqJJApvCHsMMalJznqXjJtIzbhT+pm6Teee+qolb qUxw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id m13si1688573ejj.742.2020.05.27.03.51.56; Wed, 27 May 2020 03:52:20 -0700 (PDT) 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; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2387962AbgE0IRs (ORCPT + 99 others); Wed, 27 May 2020 04:17:48 -0400 Received: from mga11.intel.com ([192.55.52.93]:28919 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2387835AbgE0IRr (ORCPT ); Wed, 27 May 2020 04:17:47 -0400 IronPort-SDR: JtV1HuaAcmnSO8Ef7PKxlMn/MoEsZNASG5BAg287BkCL9nLgFLKL77ygd7CorKGi7cz8tBGcmt 0ZK516F8BSeQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 May 2020 01:17:46 -0700 IronPort-SDR: rNAW23FmNFw0cWIuOenLCsRYoK3DaExJz+QiHFQ5/tn1anWrD5Ndz1/H4gsjY609MUdPgiuTYx C3nVtGHtZtZA== X-IronPort-AV: E=Sophos;i="5.73,440,1583222400"; d="scan'208";a="414123547" Received: from likexu-mobl1.ccr.corp.intel.com (HELO [10.238.4.141]) ([10.238.4.141]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 May 2020 01:17:43 -0700 Subject: Re: [PATCH v11 10/11] KVM: x86/pmu: Check guest LBR availability in case host reclaims them To: like.xu@intel.com, Peter Zijlstra Cc: Paolo Bonzini , linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Sean Christopherson , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , Thomas Gleixner , ak@linux.intel.com, wei.w.wang@intel.com References: <20200514083054.62538-1-like.xu@linux.intel.com> <20200514083054.62538-11-like.xu@linux.intel.com> <20200519111559.GJ279861@hirez.programming.kicks-ass.net> <3a234754-e103-907f-9b06-44b5e7ae12d3@intel.com> <20200519145756.GC317569@hirez.programming.kicks-ass.net> <9577169d-62f4-0750-7054-5e842d5d2296@intel.com> From: Like Xu Organization: Intel OTC Message-ID: <9f6bef69-08bc-2daa-6f12-764e9de7d418@linux.intel.com> Date: Wed, 27 May 2020 16:17:27 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: <9577169d-62f4-0750-7054-5e842d5d2296@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Peter, On 2020/5/20 10:01, Xu, Like wrote: > On 2020/5/19 22:57, Peter Zijlstra wrote: >> On Tue, May 19, 2020 at 09:10:58PM +0800, Xu, Like wrote: >>> On 2020/5/19 19:15, Peter Zijlstra wrote: >>>> On Thu, May 14, 2020 at 04:30:53PM +0800, Like Xu wrote: >>>> >>>>> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c >>>>> index ea4faae56473..db185dca903d 100644 >>>>> --- a/arch/x86/kvm/vmx/pmu_intel.c >>>>> +++ b/arch/x86/kvm/vmx/pmu_intel.c >>>>> @@ -646,6 +646,43 @@ static void intel_pmu_lbr_cleanup(struct kvm_vcpu >>>>> *vcpu) >>>>>            intel_pmu_free_lbr_event(vcpu); >>>>>    } >>>>> +static bool intel_pmu_lbr_is_availabile(struct kvm_vcpu *vcpu) >>>>> +{ >>>>> +    struct kvm_pmu *pmu = vcpu_to_pmu(vcpu); >>>>> + >>>>> +    if (!pmu->lbr_event) >>>>> +        return false; >>>>> + >>>>> +    if (event_is_oncpu(pmu->lbr_event)) { >>>>> +        intel_pmu_intercept_lbr_msrs(vcpu, false); >>>>> +    } else { >>>>> +        intel_pmu_intercept_lbr_msrs(vcpu, true); >>>>> +        return false; >>>>> +    } >>>>> + >>>>> +    return true; >>>>> +} >>>> This is unreadable gunk, what? >>> Abstractly, it is saying "KVM would passthrough the LBR satck MSRs if >>> event_is_oncpu() is true, otherwise cancel the passthrough state if any." >>> >>> I'm using 'event->oncpu != -1' to represent the guest LBR event >>> is scheduled on rather than 'event->state == PERF_EVENT_STATE_ERROR'. >>> >>> For intel_pmu_intercept_lbr_msrs(), false means to passthrough the LBR >>> stack >>> MSRs to the vCPU, and true means to cancel the passthrough state and make >>> LBR MSR accesses trapped by the KVM. >> To me it seems very weird to change state in a function that is supposed >> to just query state. >> >> 'is_available' seems to suggest a simple: return 'lbr_event->state == >> PERF_EVENT_STATE_ACTIVE' or something. > This clarification led me to reconsider the use of a more readable name here. > > Do you accept the check usage of "event->oncpu != -1" instead of > 'event->state == PERF_EVENT_STATE_ERROR' before KVM do passthrough ? >> >>>>> +static void intel_pmu_availability_check(struct kvm_vcpu *vcpu) >>>>> +{ >>>>> +    lockdep_assert_irqs_disabled(); >>>>> + >>>>> +    if (lbr_is_enabled(vcpu) && !intel_pmu_lbr_is_availabile(vcpu) && >>>>> +        (vmcs_read64(GUEST_IA32_DEBUGCTL) & DEBUGCTLMSR_LBR)) >>>>> +        pr_warn_ratelimited("kvm: vcpu-%d: LBR is temporarily >>>>> unavailable.\n", >>>>> +            vcpu->vcpu_id); >>>> More unreadable nonsense; when the events go into ERROR state, it's a >>>> permanent fail, they'll not come back. >>> It's not true.  The guest LBR event with 'ERROR state' or 'oncpu != -1' >>> would be >>> lazy released and re-created in the next time the >>> intel_pmu_create_lbr_event() is >>> called and it's supposed to be re-scheduled and re-do availability_check() >>> as well. >> Where? Also, wth would you need to destroy and re-create an event for >> that? > If the guest does not set the EN_LBR bit and did not touch any LBR-related > registers > in the last time slice, KVM will destroy the guest LBR event in > kvm_pmu_cleanup() > which is called once every time the vCPU thread is scheduled in. > > The re-creation is not directly called after the destruction > but is triggered by the next guest access to the LBR-related registers if any. > > From the time when the guest LBR event enters the "oncpu! = -1" state > to the next re-creation, the guest LBR is not available. After the > re-creation, > the guest LBR is hopefully available and if it's true, the LBR will be > passthrough > and used by the guest normally. > > That's the reason for "LBR is temporarily unavailable" Do you still have any concerns on this issue? > and please let me know if it doesn't make sense to you. > >>>>> @@ -6696,8 +6696,10 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu >>>>> *vcpu) >>>>>        pt_guest_enter(vmx); >>>>> -    if (vcpu_to_pmu(vcpu)->version) >>>>> +    if (vcpu_to_pmu(vcpu)->version) { >>>>>            atomic_switch_perf_msrs(vmx); >>>>> +        kvm_x86_ops.pmu_ops->availability_check(vcpu); >>>>> +    } >>>> AFAICT you just did a call out to the kvm_pmu crud in >>>> atomic_switch_perf_msrs(), why do another call? >>> In fact, availability_check() is only called here for just one time. >>> >>> The callchain looks like: >>> - vmx_vcpu_run() >>>      - kvm_x86_ops.pmu_ops->availability_check(); >>>          - intel_pmu_availability_check() >>>              - intel_pmu_lbr_is_availabile() >>>                  - event_is_oncpu() ... >>> >> What I'm saying is that you just did a pmu_ops indirect call in >> atomic_switch_perf_msrs(), why add another? > Do you mean the indirect call: > - atomic_switch_perf_msrs() >     - perf_guest_get_msrs() >         - x86_pmu.guest_get_msrs() > ? > > The two pmu_ops are quite different: > - the first one in atomic_switch_perf_msrs() is defined in the host side; > - the second one for availability_check() is defined in the KVM side; > > The availability_check() for guest LBR event and MSRs pass-through > operations are definitely KVM context specific. Do you still have any concerns on this issue? If you have more comments on the patchset, please let me know. > > Thanks, > Like Xu >