Re: [PATCH v13 021/113] KVM: TDX: Make pmu_intel.c ignore guest TD case

[Like Xu]

On 28/5/2023 4:26 pm, Isaku Yamahata wrote:
> On Wed, Apr 19, 2023 at 04:21:21PM +0800,
> Like Xu <like.xu.linux@xxxxxxxxx> wrote:
>
> > On 2/4/2023 4:50 pm, Zhi Wang wrote:
> > > Hi Like:
> > >
> > > Would you mind to take a look on this patch? It would be nice to have
> > > a r-b also from you. :)
> > >
> > > On Sun, 12 Mar 2023 10:55:45 -0700
> > > isaku.yamahata@xxxxxxxxx wrote:
> > >
> > > > From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> > > >
> > > > Because TDX KVM doesn't support PMU yet (it's future work of TDX KVM
> > > > support as another patch series) and pmu_intel.c touches vmx specific
> >
> > It would be nice to have pmu support for tdx-guest from the very beginning.
>
> It's supported in the public github repo.
> https://github.com/intel/tdx/tree/kvm-upstream-workaround
> As this patch series has 100+ patches, I don't want to bloat this patch more.

I presume we are talking about 873e2391e729...63761adbf5aa for TD pmu:

A quick glance brought me at least these comments:

(1) how does intel_pmu_save/restore() handle the enabled host LBR/PEBS ?
(2) guest PMI injection may be malicious and could the current guest pmu driver 
handle it ?
(3) how do we handle the case when host counters can be enabled before TDENTER
for debuggable TD and support the case like "perf-kvm for both guest and host" ?

My point is actually, changes to perf/core should be CC to the perf reviewers
as early as possible to prevent key player from killing the direction.

> > If you guys are more open on the tdx development model, I'd like to help on
> > those features.
>
> This mainling list is the place.

Yeah, plus the PUCK and LPC KVM micro-conference.

> > > > structure in vcpu initialization, as workaround add dummy structure to
> > > > struct vcpu_tdx and pmu_intel.c can ignore TDX case.
> >
> > If the target is not to provide a workaround, how about other variants:
> > 	- struct lbr_desc lbr_desc;
> > 	- pebs ds_buffer;
> > ?
> >
> > We also need tdx selftest to verify the unavailability of these features.
> > Also, it would be great to have TDX's "System Profiling Mode" featue back in
> > the specification.

Detailed TD (plus debuggable) PMU selftest would clearly speed up the review 
process.

> I don't think it's productive. Once merging this patch series, we can move on
> to TDX PMU support (or whatever still missing feature) as second (or later)
> step.

I totally agree that TD PMU (plus debuggable) support should not be the first 
part of the
code to be merged in, but the related discussion should be kicked off on day 0.
As a scenario for users using Intel TDX, it's expected to support "System 
Profiling Mode",
which was first introduced in 344425-001US but disappeared since version 003.