Re: [PATCH V2 3/5] KVM: x86/vPMU: Create vPMU interface for VMX and SVM

["Radim Krčmář" <rkrcmar@redhat.com>]

2015-04-09 16:08-0500, Wei Huang:
> <snip>
> >>>> @@ -4918,13 +4919,13 @@ static int emulator_set_msr(struct x86_emulate_ctxt *ctxt,
> >>>>  static int emulator_check_pmc(struct x86_emulate_ctxt *ctxt,
> >>>>  			      u32 pmc)
> >>>>  {
> >>>> -	return kvm_pmu_check_pmc(emul_to_vcpu(ctxt), pmc);
> >>>> +	return kvm_pmu_check_msr_idx(emul_to_vcpu(ctxt), pmc);
> >>>
> >>> (Why not pmc?)
> >> See "Design Note" in pmu.c for a better explanation. I tried to use msr as
> >> real x86 MSR; and msr_idx refers to MSR offset.
> > 
> > I skipped the comment as I thought it was there before, sorry ...
> > 
> > I wouldn't call it MSR index, MSR is just a related interface for PMC,
> > and MSR indices don't even have simple mapping to RDPMC ones.
> > We are indexing PMC without MSR, so index/pmc_idx/pmc seems better.
> > 
> I can fix the name of this function (maybe back to kvm_pmu_check_pmc(),
> let me think about it). In the meanwhile, do you have any comments on
> the following names? They will impact the rest code:
> 
> * msr: MSR for x86

Ok, it's the MSR identifier.

> * msr_idx: offset of MSR registers (used by rdpmc)

Same argument as for the function name.

I haven't been thinking much about glb_idx before, so msr_idx could be
named rdpmc_idx;  well, that name also has problems (too to close to
rdpmc function) and I'm fine with anything that doesn't tie it to MSR.

> * glb_idx: a unified index for both GP and fixed counters (should we
> rename it to idx instead?)

This one is the "real" pmc index, which we then have in pmc->idx, so it
could be pmc_idx ... glb_idx is good, we have to differentiate it from
the index in used in rdpmc as well.

> Are they confusing to you?

Yes, but I'm confused most of the time :)

>                            Maybe I should move "Design Note" to commit
> message instead of real code?

The comment couldn't get outdated that way.
(I'm ambivalent, there might be a benefit in having code comments.)