On Thu, Feb 23, 2012 at 10:32 PM, Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote:
> On Thu, 23 Feb 2012 02:32:26 -0500, Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx> wrote:
>> From: Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
>>
>> Targets KVM support for Cortex A-15 processors.
>>
>> Contains no real functionality but all the framework components,
>> make files, header files and some tracing functionality.
>>
>> “Nothing to see here. Move along, move along..."
>>
>> Most functionality is in arch/arm/kvm/* or arch/arm/include/asm/kvm_*.h.
>>
>> Signed-off-by: Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
>> ---
>> arch/arm/Kconfig | 2
>> arch/arm/Makefile | 1
>> arch/arm/include/asm/kvm.h | 72 ++++++++++
>> arch/arm/include/asm/kvm_asm.h | 28 ++++
>> arch/arm/include/asm/kvm_emulate.h | 92 +++++++++++++
>> arch/arm/include/asm/kvm_host.h | 116 ++++++++++++++++
>> arch/arm/include/asm/kvm_para.h | 9 +
>> arch/arm/include/asm/unified.h | 12 ++
>> arch/arm/kvm/Kconfig | 45 ++++++
>> arch/arm/kvm/Makefile | 17 ++
>> arch/arm/kvm/arm.c | 256 ++++++++++++++++++++++++++++++++++++
>> arch/arm/kvm/emulate.c | 121 +++++++++++++++++
>> arch/arm/kvm/exports.c | 16 ++
>> arch/arm/kvm/guest.c | 148 +++++++++++++++++++++
>> arch/arm/kvm/init.S | 17 ++
>> arch/arm/kvm/interrupts.S | 17 ++
>> arch/arm/kvm/mmu.c | 15 ++
>> arch/arm/kvm/trace.h | 52 +++++++
>> arch/arm/mm/Kconfig | 8 +
>> 19 files changed, 1044 insertions(+), 0 deletions(-)
>> create mode 100644 arch/arm/include/asm/kvm.h
>> create mode 100644 arch/arm/include/asm/kvm_asm.h
>> create mode 100644 arch/arm/include/asm/kvm_emulate.h
>> create mode 100644 arch/arm/include/asm/kvm_host.h
>> create mode 100644 arch/arm/include/asm/kvm_para.h
>> create mode 100644 arch/arm/kvm/Kconfig
>> create mode 100644 arch/arm/kvm/Makefile
>> create mode 100644 arch/arm/kvm/arm.c
>> create mode 100644 arch/arm/kvm/emulate.c
>> create mode 100644 arch/arm/kvm/exports.c
>> create mode 100644 arch/arm/kvm/guest.c
>> create mode 100644 arch/arm/kvm/init.S
>> create mode 100644 arch/arm/kvm/interrupts.S
>> create mode 100644 arch/arm/kvm/mmu.c
>> create mode 100644 arch/arm/kvm/trace.h
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index e12bc34..81aa08f 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -2263,3 +2263,5 @@ source "security/Kconfig"
>> source "crypto/Kconfig"
>>
>> source "lib/Kconfig"
>> +
>> +source "arch/arm/kvm/Kconfig"
>> diff --git a/arch/arm/Makefile b/arch/arm/Makefile
>> index 40319d9..eca44e0 100644
>> --- a/arch/arm/Makefile
>> +++ b/arch/arm/Makefile
>> @@ -253,6 +253,7 @@ core-$(CONFIG_VFP) += arch/arm/vfp/
>>
>> # If we have a machine-specific directory, then include it in the build.
>> core-y += arch/arm/kernel/ arch/arm/mm/ arch/arm/common/
>> +core-y += arch/arm/kvm/
>> core-y += $(machdirs) $(platdirs)
>>
>> drivers-$(CONFIG_OPROFILE) += arch/arm/oprofile/
>> diff --git a/arch/arm/include/asm/kvm.h b/arch/arm/include/asm/kvm.h
>> new file mode 100644
>> index 0000000..544cb2a
>> --- /dev/null
>> +++ b/arch/arm/include/asm/kvm.h
>> @@ -0,0 +1,72 @@
>> +/*
>> + * 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, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
>> + *
>> + */
>> +
>> +#ifndef __ARM_KVM_H__
>> +#define __ARM_KVM_H__
>> +
>> +#include <asm/types.h>
>> +
>> +#define __KVM_HAVE_GUEST_DEBUG
>> +
>> +/*
>> + * Modes used for short-hand mode determinition in the world-switch code and
>> + * in emulation code.
>> + *
>> + * Note: These indices do NOT correspond to the value of the CPSR mode bits!
>> + */
>> +enum vcpu_modes {
>
> Nitpick: s/vcpu_modes/vcpu_mode/ ?
>
> ...
>> +static inline unsigned char vcpu_mode(struct kvm_vcpu *vcpu)
>
> static inline enum vcpu_mode vcpu_mode(struct kvm_vcpu *vcpu)...
>
>> +{
>> + u8 modes_table[16] = {
>> + MODE_USR, /* 0x0 */
>> + MODE_FIQ, /* 0x1 */
>> + MODE_IRQ, /* 0x2 */
>> + MODE_SVC, /* 0x3 */
>> + 0xf, 0xf, 0xf,
>> + MODE_ABT, /* 0x7 */
>> + 0xf, 0xf, 0xf,
>> + MODE_UND, /* 0xb */
>> + 0xf, 0xf, 0xf,
>> + MODE_SYS}; /* 0xf */
>> +
>> + BUG_ON(modes_table[vcpu->arch.regs.cpsr & 0xf] == 0xf);
>> + return modes_table[vcpu->arch.regs.cpsr & 0xf];
>> +}
>
> Like much of your code, I find this far too readable. How about:
>
> enum vcpu_mode {
> MODE_USR,
> MODE_FIQ,
> MODE_IRQ,
> MODE_SVC,
> MODE_ABT,
> MODE_UND,
> MODE_SYS
> };
>
> static inline enum vcpu_mode vcpu_mode(const struct kvm_vcpu *vcpu)
> {
> u8 mode = vcpu->arch.regs.cpsr & 0xf;
> return (mode & 3) + (mode >> 2);
> }
>
>
>> +/*
>> + * Return the SPSR for the specified mode of the virtual CPU.
>> + */
>> +static inline u32 *kvm_vcpu_spsr(struct kvm_vcpu *vcpu, u32 mode)
>
> s/u32/enum vcpu_mode/
>
>> +{
>> + switch (mode) {
>> + case MODE_SVC:
>> + return &vcpu->arch.regs.svc_regs[2];
>> + case MODE_ABT:
>> + return &vcpu->arch.regs.svc_regs[2];
>> + case MODE_UND:
>> + return &vcpu->arch.regs.svc_regs[2];
>> + case MODE_IRQ:
>> + return &vcpu->arch.regs.svc_regs[2];
>
> This reminds me: shouldn't these be:
>
> case MODE_ABT:
> return &vcpu->arch.regs.abt_regs[2];
> case MODE_UND:
> return &vcpu->arch.regs.und_regs[2];
> case MODE_IRQ:
> return &vcpu->arch.regs.irq_regs[2];
>
> ?
>
yes, definitely should. thanks.
>> +static inline u32 *vcpu_cpsr(struct kvm_vcpu *vcpu)
>> +{
>> + return &vcpu->arch.regs.cpsr;
>> +}
>
> In general, do you want accessors like this to all regs? I'd like to
> know, because injecting an undefined insn trap in the guest needs to
> access the pc...
>
hmm, not really, but I kind of like it for simplicity and it goes nice
with kvm_vcpu_spsr, which should be called vcpu_spsr in the same light
of things by the way. I would have no problem having a vcpu_pc()
accessor as well, that wraps around vcpu_reg.
>> +struct kvm_vcpu;
>> +u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
>
> s/u32/enum vcpu_mode/
>
>> +config KVM
>> + tristate "Kernel-based Virtual Machine (KVM) support"
>> + select PREEMPT_NOTIFIERS
>> + select ANON_INODES
>> + select KVM_ARM_HOST
>> + select KVM_MMIO
>> + depends on CPU_V7 && ARM_VIRT_EXT
>> + ---help---
>> + Support hosting virtualized guest machines. You will also
>> + need to select one or more of the processor modules below.
>> +
>> + This module provides access to the hardware capabilities through
>> + a character device node named /dev/kvm.
>> +
>> + If unsure, say N.
>> +
>> +config KVM_ARM_HOST
>> + bool "KVM host support for ARM cpus."
>> + depends on KVM
>> + depends on MMU
>> + depends on CPU_V7 && ARM_VIRT_EXT
>> + ---help---
>> + Provides host support for ARM processors.
>
> I don't think KVM should select KVM_ARM_HOST (I assume there'll be a
> KVM_ARM_GUEST one day). Otherwise remove the prompt after 'bool' in
> KVM_ARM_HOST.
>
I see. I removed the select.
>> +/*
>> + * Return a pointer to the register number valid in the specified mode of
>> + * the virtual CPU.
>> + */
>> +u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode)
>> +{
>> + BUG_ON(reg_num > 15);
>> + BUG_ON(mode > MODE_SYS);
>> +
>> + return (u32 *)((void *)&vcpu->arch + vcpu_reg_offsets[mode][reg_num]);
>> +}
>
> Hmm.... I tried to neaten this (untested). Not sure if it's a win, your
> call:
>
I like the macros and getting rid of the horrible casts. I am not sure
about the union in the struct though as people might have to grep for
both access names if they wish to see where something is modified and
such - and since it serves only that single access point. How about
creating that array in kvm_vcpu_reg function instead? I'll take your
newer patch and modify it and see where that takes me.
Thanks for reworking this.
> commit 7f99177bac6173968925f7329333e5bdf752be67
> Author: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
> Date: Fri Feb 24 13:37:44 2012 +1030
>
> ARM: KVM: neaten offset calculations.
>
> If we do this the C-correct way, we use fewer casts and it's a bit clearer.
>
> Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
>
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 73aece6..6f5b08f 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -84,7 +84,11 @@ enum cp15_regs {
> };
>
> struct kvm_vcpu_arch {
> - struct kvm_vcpu_regs regs;
> + /* We sometimes access these as an array for simplicity. */
> + union {
> + struct kvm_vcpu_regs regs;
> + u32 reg_array[sizeof(struct kvm_vcpu_regs) / sizeof(u32)];
> + };
>
> /* System control coprocessor (cp15) */
> u32 cp15[nr_cp15_regs];
> diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
> index 4f5f2de..108db38 100644
> --- a/arch/arm/kvm/emulate.c
> +++ b/arch/arm/kvm/emulate.c
> @@ -22,8 +22,10 @@
>
> #include "trace.h"
>
> -#define USR_REG_OFFSET(_reg) \
> - offsetof(struct kvm_vcpu_arch, regs.usr_regs[_reg])
> +#define REG_OFFSET(_reg) \
> + (offsetof(struct kvm_vcpu_regs, _reg) / sizeof(u32))
> +
> +#define USR_REG_OFFSET(_num) REG_OFFSET(usr_regs[_num])
>
> static const unsigned long vcpu_reg_offsets[MODE_SYS + 1][16] = {
> /* FIQ Registers */
> @@ -31,14 +33,14 @@ static const unsigned long vcpu_reg_offsets[MODE_SYS + 1][16] = {
> USR_REG_OFFSET(0), USR_REG_OFFSET(1), USR_REG_OFFSET(2),
> USR_REG_OFFSET(3), USR_REG_OFFSET(4), USR_REG_OFFSET(5),
> USR_REG_OFFSET(6), USR_REG_OFFSET(7),
> - offsetof(struct kvm_vcpu_arch, regs.fiq_regs[1]), /* r8 */
> - offsetof(struct kvm_vcpu_arch, regs.fiq_regs[1]), /* r9 */
> - offsetof(struct kvm_vcpu_arch, regs.fiq_regs[2]), /* r10 */
> - offsetof(struct kvm_vcpu_arch, regs.fiq_regs[3]), /* r11 */
> - offsetof(struct kvm_vcpu_arch, regs.fiq_regs[4]), /* r12 */
> - offsetof(struct kvm_vcpu_arch, regs.fiq_regs[5]), /* r13 */
> - offsetof(struct kvm_vcpu_arch, regs.fiq_regs[6]), /* r14 */
> - offsetof(struct kvm_vcpu_arch, regs.pc) /* r15 */
> + REG_OFFSET(fiq_regs[1]), /* r8 */
> + REG_OFFSET(fiq_regs[1]), /* r9 */
> + REG_OFFSET(fiq_regs[2]), /* r10 */
> + REG_OFFSET(fiq_regs[3]), /* r11 */
> + REG_OFFSET(fiq_regs[4]), /* r12 */
> + REG_OFFSET(fiq_regs[5]), /* r13 */
> + REG_OFFSET(fiq_regs[6]), /* r14 */
> + REG_OFFSET(pc) /* r15 */
> },
>
> /* IRQ Registers */
> @@ -48,9 +50,9 @@ static const unsigned long vcpu_reg_offsets[MODE_SYS + 1][16] = {
> USR_REG_OFFSET(6), USR_REG_OFFSET(7), USR_REG_OFFSET(8),
> USR_REG_OFFSET(9), USR_REG_OFFSET(10), USR_REG_OFFSET(11),
> USR_REG_OFFSET(12),
> - offsetof(struct kvm_vcpu_arch, regs.irq_regs[0]), /* r13 */
> - offsetof(struct kvm_vcpu_arch, regs.irq_regs[1]), /* r14 */
> - offsetof(struct kvm_vcpu_arch, regs.pc) /* r15 */
> + REG_OFFSET(irq_regs[0]), /* r13 */
> + REG_OFFSET(irq_regs[1]), /* r14 */
> + REG_OFFSET(pc) /* r15 */
> },
>
> /* SVC Registers */
> @@ -60,9 +62,9 @@ static const unsigned long vcpu_reg_offsets[MODE_SYS + 1][16] = {
> USR_REG_OFFSET(6), USR_REG_OFFSET(7), USR_REG_OFFSET(8),
> USR_REG_OFFSET(9), USR_REG_OFFSET(10), USR_REG_OFFSET(11),
> USR_REG_OFFSET(12),
> - offsetof(struct kvm_vcpu_arch, regs.svc_regs[0]), /* r13 */
> - offsetof(struct kvm_vcpu_arch, regs.svc_regs[1]), /* r14 */
> - offsetof(struct kvm_vcpu_arch, regs.pc) /* r15 */
> + REG_OFFSET(svc_regs[0]), /* r13 */
> + REG_OFFSET(svc_regs[1]), /* r14 */
> + REG_OFFSET(pc) /* r15 */
> },
>
> /* ABT Registers */
> @@ -72,9 +74,9 @@ static const unsigned long vcpu_reg_offsets[MODE_SYS + 1][16] = {
> USR_REG_OFFSET(6), USR_REG_OFFSET(7), USR_REG_OFFSET(8),
> USR_REG_OFFSET(9), USR_REG_OFFSET(10), USR_REG_OFFSET(11),
> USR_REG_OFFSET(12),
> - offsetof(struct kvm_vcpu_arch, regs.abt_regs[0]), /* r13 */
> - offsetof(struct kvm_vcpu_arch, regs.abt_regs[1]), /* r14 */
> - offsetof(struct kvm_vcpu_arch, regs.pc) /* r15 */
> + REG_OFFSET(abt_regs[0]), /* r13 */
> + REG_OFFSET(abt_regs[1]), /* r14 */
> + REG_OFFSET(pc) /* r15 */
> },
>
> /* UND Registers */
> @@ -84,9 +86,9 @@ static const unsigned long vcpu_reg_offsets[MODE_SYS + 1][16] = {
> USR_REG_OFFSET(6), USR_REG_OFFSET(7), USR_REG_OFFSET(8),
> USR_REG_OFFSET(9), USR_REG_OFFSET(10), USR_REG_OFFSET(11),
> USR_REG_OFFSET(12),
> - offsetof(struct kvm_vcpu_arch, regs.und_regs[0]), /* r13 */
> - offsetof(struct kvm_vcpu_arch, regs.und_regs[1]), /* r14 */
> - offsetof(struct kvm_vcpu_arch, regs.pc) /* r15 */
> + REG_OFFSET(und_regs[0]), /* r13 */
> + REG_OFFSET(und_regs[1]), /* r14 */
> + REG_OFFSET(pc) /* r15 */
> },
>
> /* USR Registers */
> @@ -96,9 +98,9 @@ static const unsigned long vcpu_reg_offsets[MODE_SYS + 1][16] = {
> USR_REG_OFFSET(6), USR_REG_OFFSET(7), USR_REG_OFFSET(8),
> USR_REG_OFFSET(9), USR_REG_OFFSET(10), USR_REG_OFFSET(11),
> USR_REG_OFFSET(12),
> - offsetof(struct kvm_vcpu_arch, regs.usr_regs[13]), /* r13 */
> - offsetof(struct kvm_vcpu_arch, regs.usr_regs[14]), /* r14 */
> - offsetof(struct kvm_vcpu_arch, regs.pc) /* r15 */
> + REG_OFFSET(usr_regs[13]), /* r13 */
> + REG_OFFSET(usr_regs[14]), /* r14 */
> + REG_OFFSET(pc) /* r15 */
> },
>
> /* SYS Registers */
> @@ -108,9 +110,9 @@ static const unsigned long vcpu_reg_offsets[MODE_SYS + 1][16] = {
> USR_REG_OFFSET(6), USR_REG_OFFSET(7), USR_REG_OFFSET(8),
> USR_REG_OFFSET(9), USR_REG_OFFSET(10), USR_REG_OFFSET(11),
> USR_REG_OFFSET(12),
> - offsetof(struct kvm_vcpu_arch, regs.usr_regs[13]), /* r13 */
> - offsetof(struct kvm_vcpu_arch, regs.usr_regs[14]), /* r14 */
> - offsetof(struct kvm_vcpu_arch, regs.pc) /* r15 */
> + REG_OFFSET(usr_regs[13]), /* r13 */
> + REG_OFFSET(usr_regs[14]), /* r14 */
> + REG_OFFSET(pc) /* r15 */
> },
> };
>
> @@ -123,7 +125,7 @@ u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode)
> BUG_ON(reg_num > 15);
> BUG_ON(mode > MODE_SYS);
>
> - return (u32 *)((void *)&vcpu->arch + vcpu_reg_offsets[mode][reg_num]);
> + return &vcpu->arch.reg_array[vcpu_reg_offsets[mode][reg_num]];
> }
>
> /******************************************************************************
>
>> +config ARM_VIRT_EXT
>> + bool "Support for ARM Virtualization Extensions"
>> + depends on ARM_LPAE
>> + help
>> + Say Y if you have an ARMv7 processor supporting the ARM hardware
>> + Virtualization extensions. KVM depends on this feature and will
>> + not run without it being selected.
>> +
>> config ARCH_PHYS_ADDR_T_64BIT
>> def_bool ARM_LPAE
>
> If you say Y here, you won't boot on a machine which hasn't got VIRT,
> right? That's changing, of course, with Dave's boot changes, but worth
> mentioning either way I think.
>
definitely. comment added.
-Christoffer
--
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
