On Thu, 23 Feb 2012 02:32:26 -0500, Christoffer Dall 
<c.d...@virtualopensystems.com> wrote:
> From: Christoffer Dall <c.d...@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.d...@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:

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 <ru...@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 <ru...@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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to