From: Rusty Russell <rusty@rustcorp.com.au>
To: Christoffer Dall <c.dall@virtualopensystems.com>,
android-virt@lists.cs.columbia.edu, kvm@vger.kernel.org,
Dave Martin <dave.martin@linaro.org>
Cc: tech@virtualopensystems.com
Subject: Re: [PATCH v6 02/12] ARM: KVM: Initial skeleton to compile KVM support
Date: Fri, 24 Feb 2012 14:02:07 +1030 [thread overview]
Message-ID: <87fwe052ko.fsf@rustcorp.com.au> (raw)
In-Reply-To: <20120223073226.3266.42978.stgit@ubuntu>
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:
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.
next prev parent reply other threads:[~2012-02-24 4:08 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-02-23 7:32 [PATCH v6 00/12] KVM/ARM Implementation Christoffer Dall
2012-02-23 7:32 ` [PATCH v6 01/12] KVM: Introduce __KVM_HAVE_IRQ_LINE Christoffer Dall
2012-02-23 7:32 ` [PATCH v6 02/12] ARM: KVM: Initial skeleton to compile KVM support Christoffer Dall
2012-02-24 3:32 ` Rusty Russell [this message]
2012-02-24 4:43 ` Christoffer Dall
2012-02-25 3:50 ` Rusty Russell
2012-02-25 15:20 ` Christoffer Dall
2012-03-11 21:41 ` Christoffer Dall
2012-02-23 7:32 ` [PATCH v6 03/12] ARM: KVM: Hypervisor identity mapping Christoffer Dall
2012-02-24 3:33 ` Rusty Russell
2012-02-23 7:32 ` [PATCH v6 04/12] ARM: KVM: Hypervisor inititalization Christoffer Dall
2012-02-24 4:00 ` Rusty Russell
2012-03-11 22:24 ` Christoffer Dall
2012-03-13 3:20 ` Rusty Russell
2012-03-05 1:12 ` Rusty Russell
2012-03-05 2:13 ` Christoffer Dall
2012-02-23 7:32 ` [PATCH v6 05/12] ARM: KVM: Memory virtualization setup Christoffer Dall
2012-02-23 7:32 ` [PATCH v6 06/12] ARM: KVM: Inject IRQs and FIQs from userspace Christoffer Dall
2012-02-23 7:32 ` [PATCH v6 07/12] ARM: KVM: World-switch implementation Christoffer Dall
2012-02-23 7:33 ` [PATCH v6 08/12] ARM: KVM: Emulation framework and CP15 emulation Christoffer Dall
2012-02-23 7:33 ` [PATCH v6 09/12] ARM: KVM: Handle guest faults in KVM Christoffer Dall
2012-02-23 7:33 ` [PATCH v6 10/12] ARM: KVM: Handle I/O aborts Christoffer Dall
2012-02-23 7:33 ` [PATCH v6 11/12] ARM: KVM: Guest wait-for-interrupts (WFI) support Christoffer Dall
2012-02-23 7:33 ` [PATCH v6 12/12] ARM: KVM: Handle CP15 CR9 accesses for L2CTLR emulation Christoffer Dall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87fwe052ko.fsf@rustcorp.com.au \
--to=rusty@rustcorp.com.au \
--cc=android-virt@lists.cs.columbia.edu \
--cc=c.dall@virtualopensystems.com \
--cc=dave.martin@linaro.org \
--cc=kvm@vger.kernel.org \
--cc=tech@virtualopensystems.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.