Re: [PATCH v6 02/12] ARM: KVM: Initial skeleton to compile KVM support

Christoffer Dall
2012-02-24T04:43:15Z

On Thu, Feb 23, 2012 at 10:32 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> On Thu, 23 Feb 2012 02:32:26 -0500, Christoffer Dall <c.dall@virtualopensystems.com> wrote:
>> From: Christoffer Dall <c.dall@virtualopensystems.com>
>>
>> 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@virtualopensystems.com>
>> ---
>>  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:
>

"far too readable" - what does this mean?

> 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];
>
> ?
>
>> +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...
>
>> +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.
>
>> +/*
>> + * 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:
>
> commit 7f99177bac6173968925f7329333e5bdf752be67
> Author: Rusty Russell <rusty@rustcorp.com.au>
> 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@rustcorp.com.au>
>
> 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.
>
> Thanks,
> Rusty.