On 09/09/12 22:18, Christoffer Dall wrote:
> On Thu, Sep 6, 2012 at 1:16 PM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
>> In order to be able to correctly profile what is happening on the
>> host, we need to be able to identify when we're running on the guest,
>> and log these events differently.
>>
>> Perf offers a simple way to register callbacks into KVM. Mimic what
>> x86 does and enjoy being able to profile your KVM host.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>> ---
>> arch/arm/include/asm/kvm_host.h | 3 ++
>> arch/arm/kvm/Makefile | 2 +-
>> arch/arm/kvm/arm.c | 5 ++-
>> arch/arm/kvm/perf.c | 70 +++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 78 insertions(+), 2 deletions(-)
>> create mode 100644 arch/arm/kvm/perf.c
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 4302007..32299c1 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -207,4 +207,7 @@ static inline int kvm_test_age_hva(struct kvm *kvm, unsigned long hva)
>> struct kvm_vcpu *kvm_arm_get_running_vcpu(void);
>> struct kvm_vcpu __percpu **kvm_get_running_vcpus(void);
>>
>> +int kvm_perf_init(void);
>> +int kvm_perf_teardown(void);
>> +
>> #endif /* __ARM_KVM_HOST_H__ */
>> diff --git a/arch/arm/kvm/Makefile b/arch/arm/kvm/Makefile
>> index 4f27970..73c268a 100644
>> --- a/arch/arm/kvm/Makefile
>> +++ b/arch/arm/kvm/Makefile
>> @@ -18,7 +18,7 @@ obj-$(CONFIG_KVM_ARM_HOST) += init.o interrupts.o exports.o
>>
>> kvm-arm-y += $(addprefix ../../../virt/kvm/, kvm_main.o coalesced_mmio.o)
>>
>> -kvm-arm-y += arm.o guest.o mmu.o emulate.o reset.o coproc.o
>> +kvm-arm-y += arm.o guest.o mmu.o emulate.o reset.o coproc.o perf.o
>>
>> obj-$(CONFIG_KVM) += kvm-arm.o
>> obj-$(CONFIG_KVM_ARM_VGIC) += vgic.o
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 8d8bf72..743bdc8 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -995,7 +995,9 @@ static int init_hyp_mode(void)
>> err = kvm_timer_hyp_init();
>> if (err)
>> goto out_free_mappings;
>> -
>> +
>> + kvm_perf_init();
>> +
>> return 0;
>> out_free_vfp:
>> free_percpu(kvm_host_vfp_state);
>> @@ -1090,6 +1092,7 @@ void kvm_arch_exit(void)
>> free_page(per_cpu(kvm_arm_hyp_stack_page, cpu));
>> per_cpu(kvm_arm_hyp_stack_page, cpu) = 0;
>> }
>> + kvm_perf_teardown();
>> }
>>
>> static int arm_init(void)
>> diff --git a/arch/arm/kvm/perf.c b/arch/arm/kvm/perf.c
>
> seems a bit overkill with a separate file, I would prefer if this was
> just in arm.c
Frankly, I think arm.c contains too much stuff already - ioctls, various
stubs, init code, the whole entry/exit logic. Yet you could argue that
all this is intimately connected.
But the perf code is completely standalone. No interaction with any of
the above. Maybe it is just me being anal about that, but I like to have
separate files for separate features. Keeps the clutter at a minimal
level in my low-bandwidth brain... ;-)
>> new file mode 100644
>> index 0000000..e7c42fd
>> --- /dev/null
>> +++ b/arch/arm/kvm/perf.c
>> @@ -0,0 +1,70 @@
>> +/*
>> + * Based on the x86 implementation.
>> + *
>> + * Copyright (C) 2012 ARM Ltd.
>> + * Author: Marc Zyngier <marc.zyngier@xxxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>> + */
>> +
>> +#include <linux/perf_event.h>
>> +
>> +#include <asm/ptrace.h>
>> +#include <asm/kvm_host.h>
>> +#include <asm/kvm_emulate.h>
>> +
>> +static int kvm_is_in_guest(void)
>> +{
>> + return kvm_arm_get_running_vcpu() != NULL;
>> +}
>> +
>> +static int kvm_is_user_mode(void)
>> +{
>> + struct kvm_vcpu *vcpu;
>> +
>> + vcpu = kvm_arm_get_running_vcpu();
>> +
>> + if (vcpu)
>> + return (*vcpu_cpsr(vcpu) & MODE_MASK) == USR_MODE;
>
> isn't this completely racy if the vcpu is freed from under your?
This is called from an interrupt handler. Nothing can change at that point.
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
