Re: [PATCH 2/9] KVM: Expose a version 2 architectural PMU to a guests

[Gleb Natapov <gleb@redhat.com>]

On Wed, Nov 02, 2011 at 12:01:51PM +0200, Avi Kivity wrote:
> On 11/01/2011 02:30 PM, Gleb Natapov wrote:
> > > > +
> > > > +/* mapping between fixed pmc index and arch_events array */
> > > > +int fixed_pmc_events[] = {1, 0, 2};
> > > > +
> > > > +static bool pmc_is_gp(struct kvm_pmc *pmc)
> > > > +{
> > > > +	return pmc->type == KVM_PMC_GP;
> > > > +}
> > > > +
> > > > +static inline u64 pmc_bitmask(struct kvm_pmc *pmc)
> > > > +{
> > > > +	struct kvm_pmu *pmu = &pmc->vcpu->arch.pmu;
> > > > +
> > > > +	return pmc_is_gp(pmc) ? pmu->gp_counter_bitmask :
> > > > +		pmu->fixed_counter_bitmask;
> > > > +}
> > > 
> > > Nicer to just push the bitmask (or bitwidth) into the counter itself.
> > > 
> > Hmm, is it really nicer to replicate the same information 35 times?
> 
> If it were 35 times, you could do pmu->type->bitmask.  But it's just 5
> or 6 times.
> 
It is 35. Perf defines X86_PMC_MAX_GENERIC to be 32 and
X86_PMC_MAX_FIXED to be 3. I can do pmu->type->bitmask if you think it
is better.

> > > > +
> > > > +static void kvm_perf_overflow_intr(struct perf_event *perf_event,
> > > > +		struct perf_sample_data *data, struct pt_regs *regs)
> > > > +{
> > > > +	struct kvm_pmc *pmc = perf_event->overflow_handler_context;
> > > > +	struct kvm_pmu *pmu = &pmc->vcpu->arch.pmu;
> > > > +	if (!__test_and_set_bit(pmc_to_global_idx(pmc),
> > > > +				(unsigned long *)&pmu->reprogram_pmi)) {
> > > > +		kvm_perf_overflow(perf_event, data, regs);
> > > > +		kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
> > > > +	}
> > > > +}
> > > 
> > > Is it safe to use the __ versions here?
> > >
> > It supposed to run in an NMI context on the same CPU that just ran
> > the vcpu so simultaneous access to the same variable from different
> > CPUs shouldn't be possible. But if your scenario below can happen then
> > that assumption may not hold. The question is if PMI delivery can be
> > so skewed as to be delivered long after vmexit (which switches perf msr
> > values btw).
> 
> The compiler/runtime is allowed to implement __test_and_set_bit() as
> multiple instructions, no? Do we have any similar sequences outside nmi
> context?
> 
Yes we do. On handling PMU event during guest entry and during event
reprogramming. On x86 __ version is different from non __ version only
by lock prefix. It would be pity to use locked functions here though. We
need local_ functions for bitops.

> > > Do we need to follow kvm_make_request() with kvm_vcpu_kick()?  If there
> > > is a skew between the overflow and the host PMI, the guest might have
> > > executed a HLT.
> > Is kvm_vcpu_kick() safe for NMI context?
> 
> No.  There is irq_work_queue() for that.  Would be good to avoid it if
> we know that it's safe to (for example if we have PF_VCPU set).
> 
Checking PF_VCPU will not tell us that vcpu is going to reenter guest
mode again.

> > > 
> > > > +
> > > > +static void reprogram_fixed_counter(struct kvm_pmc *pmc, u8 en_pmi, int idx)
> > > > +{
> > > > +	unsigned en = en_pmi & 0x3;
> > > > +	bool pmi = en_pmi & 0x8;
> > > > +
> > > > +	stop_counter(pmc);
> > > > +
> > > > +	if (!en || !pmc_enabled(pmc))
> > > > +		return;
> > > > +
> > > > +	reprogram_counter(pmc, PERF_TYPE_HARDWARE,
> > > > +			arch_events[fixed_pmc_events[idx]].event_type,
> > > > +			!(en & 0x2), /* exclude user */
> > > > +			!(en & 0x1), /* exclude kernel */
> > > > +			pmi);
> > > 
> > > Are there no #defines for those constants?
> > > 
> > Nope. perf_event_intel.c open codes them too.
> 
> Okay.
> 
> > > 
> > > The user can cause this to be very small (even zero).  Can this cause an
> > > NMI storm?
> > > 
> > If user will set it to zero then attr.sample_period will always be 0 and
> > perf will think that the event is non sampling and will use max_period
> > instead. For a small value greater than zero how is it different from
> > userspace creating an event with sample_period of 1?
> 
> I don't know.  Does the kernel survive it?
> 
Need to test, but I do not see anything in the kernel that prevent userspace
from setting it to any value.

--
			Gleb.