Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Zhang, Xiong Y" <xiong.y.zhang@intel.com>
To:     "Christopherson,, Sean" <seanjc@google.com>
CC:     "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "peterz@infradead.org" <peterz@infradead.org>,
        "like.xu.linux@gmail.com" <like.xu.linux@gmail.com>,
        "kan.liang@linux.intel.com" <kan.liang@linux.intel.com>,
        "zhenyuw@linux.intel.com" <zhenyuw@linux.intel.com>,
        "Lv, Zhiyuan" <zhiyuan.lv@intel.com>
Subject: RE: [PATCH 3/4] KVM: VMX/pmu: Enable inactive vLBR event in guest LBR
 MSR emulation
Thread-Topic: [PATCH 3/4] KVM: VMX/pmu: Enable inactive vLBR event in guest
 LBR MSR emulation
Thread-Index: AQHZoEacn/ONl4ZlqEiTKxW2MUPT76+Y5LQAgAImU6A=
Date:   Sun, 25 Jun 2023 06:54:24 +0000
Message-ID: <MW4PR11MB5824653862500CB4F9EE4519BB21A@MW4PR11MB5824.namprd11.prod.outlook.com>
References: <20230616113353.45202-1-xiong.y.zhang@intel.com>
 <20230616113353.45202-4-xiong.y.zhang@intel.com>
 <ZJYCtDN+ITmrgCUs@google.com>
In-Reply-To: <ZJYCtDN+ITmrgCUs@google.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?fqOGAcsP2kaMgeWNl7Ndefh1iY7Wzc3U4OLpjcecYOZx5ZDCbZtkZzFEHWh+?=
 =?us-ascii?Q?pmmGiZBvtV4lsMtTjFhRhJ46QlIpSFHwEjihm/J0JcqHC70P5+Fh9bnDycwO?=
 =?us-ascii?Q?QAGK6M7RVQBAQnr58a5rpOpkRbbbBqOl8tSQ8TjvxSHI5NsjrtubIcVKq2e5?=
 =?us-ascii?Q?y3aTU0iL7bFyX1ZWAgqstqJLOLLhqhWCPb6v34vxdKf0/xLaBbUfU6KfDUUn?=
 =?us-ascii?Q?cIWAFdNDo7iOYBRcuKP+KTcp/crCla+7jaygFpLdKdAeClgUM6JsXl4ne0fm?=
 =?us-ascii?Q?oLswcRXvnQFrR9a/v27N0p2Qrvm6HJbP8XMDo1XI93bmI9yorcmiNSxZs7/+?=
 =?us-ascii?Q?N4IVEkC1ZSnB9mmX1tI+Jbq63b9D8dzAHwHHMBtCpStUQWADiWLUjJSr5BsH?=
 =?us-ascii?Q?SVpBTbn3n4AnoYbOuc7BQrzoU/ezyMucgsv5s2wTIhqQiinQ+eY3IpJ0QNMo?=
 =?us-ascii?Q?NQSbbTLLP2knjx85L9dyaZI4WtErV18fzBoy/KjUvvHhTjUJIhH4d1jhRBS5?=
 =?us-ascii?Q?cNtRPv4McLc7A1sZLKOBCdU1S9sUap5QKahxti2Rz2fCLgPxZca5SiM9UoMI?=
 =?us-ascii?Q?wI3BbFB8oZ6OgXI9CKN6nc5XXIJ/lAwCOPPp1JZtwVLbZULxL355UUyLX/S0?=
 =?us-ascii?Q?BuGsGjecUAn0/4Fbcx6a46H7NV8zK9BBKwGj5hhXQkLwUPrBaDUpOvJDG4wq?=
 =?us-ascii?Q?W85Y1Ym4vfj++0ctLSSKuCQI/rQdxRpXCwacYF9nQmgKAIgEfVPOOJ6k2B+k?=
 =?us-ascii?Q?pL4g/eO1ocuHMzpJpm+cqpxwbrbhgazZZ5mCUTLYRRjOeW2urRjhH4mNXr/B?=
 =?us-ascii?Q?N19dHc4/VdOQGHUe7BkfySWR7DDzlvsim1UIn176MIiOOnEwShKyO7OVdR+J?=
 =?us-ascii?Q?HspfrUncIhy5+4Jy6n/tZ+JoTbCE5YcDFoYeT08YPbxo+wNynXCBGpMI9wEr?=
 =?us-ascii?Q?tO/ko1v3ytuB+9DzC1xV4tk938x/DiCrLWlokARLQX+HaLt/mIn7Ek/sFEQV?=
 =?us-ascii?Q?73dxQFwOzw9pUBXC0ld49KIIOT1BgmRz/sZ8DHmjOOIideKfhnuS9ETERf02?=
 =?us-ascii?Q?w6EQyNkzlNUVLdWSdLq/y7FyEnPo2nW6rduvmerQE+hY9dgn6+WB+rBfpOOy?=
 =?us-ascii?Q?aMydnSbkKFpoClWM+mf8ci1SumEW8dskMqnkwPf9JfSGpZj1AP9Nx5TVC0HR?=
 =?us-ascii?Q?7FIX2H8R1QneT+Y/NQknjO+igDYENwyCpUCgkT8DKnLh+rYpvfv/rzCvrstq?=
 =?us-ascii?Q?W3I5n1Mgh9q/vW4huYqa9+FgeAwUxLSsg402vyRe95oOIE5XJurRt6OX+4my?=
 =?us-ascii?Q?lH0T3p2BrBr+AYzohDtsWop3BMkSnKgta/BK/4pU2HtJRDDpd3NRuRm4o0Gf?=
 =?us-ascii?Q?Z5v7LGIco75jbhZ+dpdLlngZtI2jFljfh5uhigNA+jDP0bRGd1ksN0PYX/VU?=
 =?us-ascii?Q?kwuzUv9WtgJ3QjCiy9RWeRblCxv1pXGnz2bnsusuAfU5eCPoX/8PgL37DqF7?=
 =?us-ascii?Q?dCyTbHuSpgYqlyHrtgyEY6WUIGwM9t3zlV24Sc0JgF76UTLOAR2hB+2BHSPO?=
 =?us-ascii?Q?3bVtZh4Sw275TQKwajzjn8ff8tKsydLZ3vqNHFu/?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: MW4PR11MB5824.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6ba8ca80-108c-48a2-7455-08db754902f2
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Jun 2023 06:54:24.5014
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 3e3r73YPdCFiHdq/rQ6kWunoCZR7zF0jf9HlEYuAmOz3q98chi+/NI37/WpohGvrYnBHRu6xRTt0j0rbBNLQug==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR11MB4528
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_PASS,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> > On Fri, Jun 16, 2023, Xiong Zhang wrote:
> > vLBR event could be inactive in two case:
> > a. host per cpu pinned LBR event occupy LBR when vLBR event is created
> > b. vLBR event is preempted by host per cpu pinned LBR event during vm
> > exit handler.
> > When vLBR event is inactive, guest couldn't access LBR msr, and it is
> > forced into error state and is excluded from schedule by perf scheduler=
.
> > So vLBR event couldn't be active through perf scheduler even if host
> > per cpu pinned LBR event has released LBR, kvm could enable vLBR event
> > proactively, then vLBR event may be active and LBR msr could be
> > passthrough into guest.
> >
> > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
> > ---
> >  arch/x86/kvm/vmx/pmu_intel.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/vmx/pmu_intel.c
> > b/arch/x86/kvm/vmx/pmu_intel.c index 741efe2c497b..5a3ab8c8711b 100644
> > --- a/arch/x86/kvm/vmx/pmu_intel.c
> > +++ b/arch/x86/kvm/vmx/pmu_intel.c
> > @@ -314,7 +314,16 @@ static bool intel_pmu_handle_lbr_msrs_access(struc=
t
> kvm_vcpu *vcpu,
> >  	if (!intel_pmu_is_valid_lbr_msr(vcpu, index))
> >  		return false;
> >
> > -	if (!lbr_desc->event && intel_pmu_create_guest_lbr_event(vcpu) < 0)
> > +	/* vLBR event may be inactive, but physical LBR may be free now.
>=20
> 	/*
> 	 * This is the preferred block comment style.
> 	 */
>=20
> > +	 * but vLBR event is pinned event, once it is inactive state, perf
> > +	 * will force it to error state in merge_sched_in() and exclude it fr=
om
> > +	 * perf schedule, so even if LBR is free now, vLBR event couldn't be
> active
> > +	 * through perf scheduler and vLBR event could be active through
> > +	 * perf_event_enable().
> > +	 */
>=20
> Trimming that down, is this what you mean?
Yes, thanks a lot.
>=20
> 	/*
> 	 * Attempt to re-enable the vLBR event if it was disabled due to
> 	 * contention with host LBR usage, i.e. was put into an error state.
> 	 * Perf doesn't notify KVM if the host stops using LBRs, i.e. KVM needs
> 	 * to manually re-enable the event.
> 	 */
>=20
> Which begs the question, why can't there be a notification of some form t=
hat
> the LBRs are once again available?
This is perf scheduler rule. If pinned event couldn't get resource as resou=
rce limitation, perf will put it into error state and exclude it from perf =
scheduler, even if resource available later, perf won't schedule it again a=
s it is in error state, the only way to reschedule it is to enable it again=
.=20
If non-pinned event couldn't get resource as resource limitation, perf will=
 put it into inactive state, perf will reschedule it automatically once res=
ource is available.
vLBR event is per process pinned event.
>=20
> Assuming that's too difficult for whatever reason, why wait until the gue=
st tries
> to read LBRs?  E.g. why not be more aggressive and try to re-enable vLBRs=
 on
> every VM-Exit.
Yes, it is a good suggestion. Actually vmx_passthrough_lbr_msrs() is called=
 on every
VM-exit, it also check vLBR event state, so I could re-enable vLBR in this =
function.
>=20
> And if we do wait until the guest touches relevant MSRs, shouldn't writes=
 to
> DEBUG_CTL that set DEBUGCTLMSR_LBR also try to re-enable the event?
Only perf know whether vLBR event could be active or not at this moment, if=
 vLBR is active, KVM could read/write DEBUG_CTL[0] with irq disable/enable =
pair in theory, but it is better that kvm don't touch perf hw resource dire=
ctly, as vLBR event is one host LBR event, host may have other LBR event, p=
erf will schedule them according to perf scheduler rule.  If vLBR is inacti=
ve, KVM shouldn't touch DEBUG_CTL MSR totally.
>=20
> Lastly, what guarantees that the MSRs hold guest data?  I assume perf pur=
ges
> the MSRs at some point, but it would be helpful to call that out in the c=
hangelog.
For DEBUG_CTL msr, VMCS has two fields for this:
1. "Save debug controls" in VM-Exit controls
2. "Load debug controls" in VM-Entry controls
For LBR records MSRs, perf will save them at process schedule out and load =
them at process schedule in.
Sure, I will add it into changelog.

> > +	if (lbr_desc->event && (lbr_desc->event->state =3D=3D
> PERF_EVENT_STATE_ERROR))
> > +		perf_event_enable(lbr_desc->event);
> > +	else if (!lbr_desc->event && intel_pmu_create_guest_lbr_event(vcpu)
> > +< 0)
> >  		goto dummy;
> >
> >  	/*
> > --
> > 2.25.1
> >