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.)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
