Re: [PATCH v1 3/5] xen/PMU: Initialization code for Xen PMU

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 09/23/2013 09:26 AM, Konrad Rzeszutek Wilk wrote:
> On Tue, Sep 10, 2013 at 11:31:48AM -0400, Boris Ostrovsky wrote:
>> Map shared data structure that will hold CPU registers, VPMU context,
>> VCPU/PCPI IDs of the VCPU interrupted by PMU interrupt. Hypervisor
>> fills this information in its handler and passes it to the guest for
>> further processing.
>>
>> Set up PMU VIRQ.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>   arch/x86/xen/Makefile          |   2 +-
>>   arch/x86/xen/pmu.c             | 122 +++++++++++++++++++++++++++++++++++++++++
>>   arch/x86/xen/pmu.h             |  12 ++++
>>   arch/x86/xen/smp.c             |  31 ++++++++++-
>>   include/xen/interface/xen.h    |   1 +
>>   include/xen/interface/xenpmu.h |  77 ++++++++++++++++++++++++++
>>   6 files changed, 243 insertions(+), 2 deletions(-)
>>   create mode 100644 arch/x86/xen/pmu.c
>>   create mode 100644 arch/x86/xen/pmu.h
>>
>> diff --git a/arch/x86/xen/Makefile b/arch/x86/xen/Makefile
>> index 96ab2c0..b187df5 100644
>> --- a/arch/x86/xen/Makefile
>> +++ b/arch/x86/xen/Makefile
>> @@ -13,7 +13,7 @@ CFLAGS_mmu.o			:= $(nostackp)
>>   obj-y		:= enlighten.o setup.o multicalls.o mmu.o irq.o \
>>   			time.o xen-asm.o xen-asm_$(BITS).o \
>>   			grant-table.o suspend.o platform-pci-unplug.o \
>> -			p2m.o
>> +			p2m.o pmu.o
> Perhaps guard the build of this based on CONFIG_PERF_EVENTS?
>
> That would of course mean you also have to create in xenpmu.h
> static inline empy functions for xen_pmu_finish and xen_pmu_init in case
> CONFIG_PERF_EVENTS is not set.

This is interface header, am I allowed to do those sorts of things there?

Also, *theoretically* other performance-measuring tools can use this 
framework, expect for perf-specific things like callbacks and the 
handler call. So maybe I should put CONFIG_PERF_EVENTS in pmu.c?

>>   
>>   obj-$(CONFIG_EVENT_TRACING) += trace.o
>>   
>> diff --git a/arch/x86/xen/pmu.c b/arch/x86/xen/pmu.c
>> new file mode 100644
>> index 0000000..da061d4
>> --- /dev/null
>> +++ b/arch/x86/xen/pmu.c
>> @@ -0,0 +1,122 @@
>> +#include <linux/types.h>
>> +#include <linux/interrupt.h>
>> +
>> +#include <asm/xen/hypercall.h>
>> +#include <xen/page.h>
>> +#include <xen/interface/xen.h>
>> +#include <xen/interface/vcpu.h>
>> +#include <xen/interface/xenpmu.h>
>> +
>> +#include "xen-ops.h"
>> +#include "pmu.h"
>> +
>> +/* x86_pmu.handle_irq definition */
>> +#include <../kernel/cpu/perf_event.h>
>> +
>> +
>> +/* Shared page between hypervisor and domain */
>> +DEFINE_PER_CPU(struct xenpmu_data *, xenpmu_shared);
>> +
>> +/* perf callbacks*/
>> +int xen_is_in_guest(void)
>> +{
>> +	struct xenpmu_data *xenpmu_data = per_cpu(xenpmu_shared,
>> +						  smp_processor_id());
>> +
>> +	if (!xen_initial_domain() ||
>> +	    xenpmu_data->domain_id > DOMID_SELF || xenpmu_data->domain_id == 0)
>> +		return 0;
>> +
>> +	return 1;
>> +}
>> +
>> +static int xen_is_user_mode(void)
>> +{
>> +	struct xenpmu_data *xenpmu_data = per_cpu(xenpmu_shared,
>> +						  smp_processor_id());
>> +	return ((xenpmu_data->regs.cs & 3) == 3);
>> +}
>> +
>> +static unsigned long xen_get_guest_ip(void)
>> +{
>> +	struct xenpmu_data *xenpmu_data = per_cpu(xenpmu_shared,
>> +						  smp_processor_id());
>> +	return xenpmu_data->regs.eip;
>> +}
>> +
>> +static struct perf_guest_info_callbacks xen_guest_cbs = {
>> +	.is_in_guest            = xen_is_in_guest,
>> +	.is_user_mode           = xen_is_user_mode,
>> +	.get_guest_ip           = xen_get_guest_ip,
>> +};
>> +
>> +/* Convert registers from Xen's format to Linux' */
>> +static void xen_convert_regs(struct cpu_user_regs *xen_regs,
>> +			     struct pt_regs *regs)
>> +{
>> +	regs->ip = xen_regs->eip;
>> +	regs->cs = xen_regs->cs;
>> +}
>> +
>> +irqreturn_t xen_pmu_irq_handler(int irq, void *dev_id)
>> +{
>> +	int ret = IRQ_NONE;
>> +	struct pt_regs regs;
>> +	struct xenpmu_data *xenpmu_data = per_cpu(xenpmu_shared,
>> +						  smp_processor_id());
>> +
>> +	xen_convert_regs(&xenpmu_data->regs, &regs);
>> +	if (x86_pmu.handle_irq(&regs))
>> +		ret = IRQ_HANDLED;
>> +
>> +	return ret;
>> +}
>> +
>> +int xen_pmu_init(int cpu)
>> +{
>> +	int ret = 0;
>> +	struct xenpmu_params xp;
>> +	unsigned long pfn;
>> +	struct xenpmu_data *xenpmu_data;
>> +
>> +	BUILD_BUG_ON(sizeof(struct xenpmu_data) > PAGE_SIZE);
>> +	xenpmu_data = vmalloc(PAGE_SIZE);
>> +	if (!xenpmu_data) {
>> +		ret = -ENOMEM;
>> +		goto fail;
>> +	}
>> +	pfn = vmalloc_to_pfn((char *)xenpmu_data);
>> +
>> +	xp.mfn = pfn_to_mfn(pfn);
>> +	xp.vcpu = cpu;
>> +	xp.version.maj = XENPMU_VER_MAJ;
>> +	xp.version.min = XENPMU_VER_MIN;
>> +	ret = HYPERVISOR_xenpmu_op(XENPMU_init, &xp);
>> +	if (ret)
>> +		goto fail;
>> +
>> +	per_cpu(xenpmu_shared, cpu) = xenpmu_data;
>> +
>> +	if (cpu == 0)
>> +		perf_register_guest_info_callbacks(&xen_guest_cbs);
>> +
>> +	return ret;
>> +
>> +fail:
>> +	vfree(xenpmu_data);
>> +	return ret;
>> +}
>> +
>> +void xen_pmu_finish(int cpu)
>> +{
>> +	struct xenpmu_params xp;
>> +
>> +	xp.vcpu = cpu;
>> +	xp.version.maj = XENPMU_VER_MAJ;
>> +	xp.version.min = XENPMU_VER_MIN;
>> +
>> +	(void)HYPERVISOR_xenpmu_op(XENPMU_finish, &xp);
>> +
>> +	vfree(per_cpu(xenpmu_shared, cpu));
>> +	per_cpu(xenpmu_shared, cpu) = NULL;
> I think you are missing:
>
> perf_unregister_guest_info_callbacks when this is the bootup CPU.

But there is always at least one CPU running, so I think I should keep 
the callbacks. The only reason I have 'if (cpu == 0)' when I register 
the callbacks is because I only want to do this once and when we are 
coming up cpu 0 is always the boot cpu (right?).

>
> And you should probably move this around to be:
>
> 	if (cpu == 0 && num_online_cpus() == 1)
> 		perf_unregister_guest_info_callbacks(..)
> 	per_cpu(xenpmu_shared, cpu) = NULL;
> 	vfree(per_cpu(xenpmu_shared, cpu))

Then I'd be calling vfree(NULL), wouldn't I?

-boris