[PATCH 0/3] KVM_VCPU_GET_REG_LIST API

[PATCH 0/3] KVM_VCPU_GET_REG_LIST API

[Rusty Russell]

This is the generic part of the KVM_SET_ONE_REG/KVM_GET_ONE_REG
enhancements which ARM wants, rebased onto kvm/next.

Rusty Russell (3):
  KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.
  KVM: Add KVM_REG_SIZE() helper.
  KVM: Add KVM_VCPU_GET_REG_LIST/KVM_CAP_REG_LIST.

 Documentation/virtual/kvm/api.txt   |   20 ++++++++++++++++++
 arch/powerpc/include/asm/kvm_host.h |    1 +
 arch/powerpc/kvm/book3s_hv.c        |    4 ++--
 arch/powerpc/kvm/book3s_pr.c        |    4 ++--
 arch/powerpc/kvm/booke.c            |    4 ++--
 arch/powerpc/kvm/powerpc.c          |   15 --------------
 arch/s390/include/asm/kvm_host.h    |    1 +
 arch/s390/kvm/kvm-s390.c            |   19 ++----------------
 include/linux/kvm.h                 |   14 +++++++++++++
 include/linux/kvm_host.h            |    9 ++++++++-
 virt/kvm/kvm_main.c                 |   38 +++++++++++++++++++++++++++++++++++
 11 files changed, 90 insertions(+), 39 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.

[Rusty Russell]

Avi has indicated that this is the future.  For now, make it dependent on
KVM_HAVE_ONE_REG (and define that for PPC and S/390).

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
---
 arch/powerpc/include/asm/kvm_host.h |    1 +
 arch/powerpc/kvm/book3s_hv.c        |    4 ++--
 arch/powerpc/kvm/book3s_pr.c        |    4 ++--
 arch/powerpc/kvm/booke.c            |    4 ++--
 arch/powerpc/kvm/powerpc.c          |   15 ---------------
 arch/s390/include/asm/kvm_host.h    |    1 +
 arch/s390/kvm/kvm-s390.c            |   19 ++-----------------
 include/linux/kvm_host.h            |    4 ++++
 virt/kvm/kvm_main.c                 |   18 ++++++++++++++++++
 9 files changed, 32 insertions(+), 38 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index 28e8f5e..a021412 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -50,6 +50,7 @@
 #include <linux/mmu_notifier.h>
 
 #define KVM_ARCH_WANT_MMU_NOTIFIER
+#define KVM_HAVE_ONE_REG
 
 struct kvm;
 extern int kvm_unmap_hva(struct kvm *kvm, unsigned long hva);
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 83e929e..8c711f1 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -535,7 +535,7 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
-int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
+int kvm_arch_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	int r = -EINVAL;
 
@@ -550,7 +550,7 @@ int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
 	return r;
 }
 
-int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
+int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	int r = -EINVAL;
 
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index 05c28f5..add88a9 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -899,7 +899,7 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
-int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
+int kvm_arch_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	int r = -EINVAL;
 
@@ -915,7 +915,7 @@ int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
 	return r;
 }
 
-int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
+int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	int r = -EINVAL;
 
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index d25a097..d239e8e 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -1219,12 +1219,12 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 	return kvmppc_core_set_sregs(vcpu, sregs);
 }
 
-int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
+int kvm_arch_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	return -EINVAL;
 }
 
-int kvm_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
+int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	return -EINVAL;
 }
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 879b14a..21cd47b 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -228,7 +228,6 @@ int kvm_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_PPC_UNSET_IRQ:
 	case KVM_CAP_PPC_IRQ_LEVEL:
 	case KVM_CAP_ENABLE_CAP:
-	case KVM_CAP_ONE_REG:
 		r = 1;
 		break;
 #ifndef CONFIG_KVM_BOOK3S_64_HV
@@ -708,20 +707,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		break;
 	}
 
-	case KVM_SET_ONE_REG:
-	case KVM_GET_ONE_REG:
-	{
-		struct kvm_one_reg reg;
-		r = -EFAULT;
-		if (copy_from_user(&reg, argp, sizeof(reg)))
-			goto out;
-		if (ioctl == KVM_SET_ONE_REG)
-			r = kvm_vcpu_ioctl_set_one_reg(vcpu, &reg);
-		else
-			r = kvm_vcpu_ioctl_get_one_reg(vcpu, &reg);
-		break;
-	}
-
 #if defined(CONFIG_KVM_E500V2) || defined(CONFIG_KVM_E500MC)
 	case KVM_DIRTY_TLB: {
 		struct kvm_dirty_tlb dirty;
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index b784154..9adb19d 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -23,6 +23,7 @@
 #define KVM_MEMORY_SLOTS 32
 /* memory slots that does not exposed to userspace */
 #define KVM_PRIVATE_MEM_SLOTS 4
+#define KVM_HAVE_ONE_REG
 
 struct sca_entry {
 	atomic_t scn;
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index e83df7f..916cf1d 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -139,7 +139,6 @@ int kvm_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_S390_UCONTROL:
 #endif
 	case KVM_CAP_SYNC_REGS:
-	case KVM_CAP_ONE_REG:
 		r = 1;
 		break;
 	case KVM_CAP_NR_VCPUS:
@@ -447,8 +446,7 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-static int kvm_arch_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu,
-					   struct kvm_one_reg *reg)
+int kvm_arch_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	int r = -EINVAL;
 
@@ -476,8 +474,7 @@ static int kvm_arch_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu,
 	return r;
 }
 
-static int kvm_arch_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu,
-					   struct kvm_one_reg *reg)
+int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	int r = -EINVAL;
 
@@ -839,18 +836,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 	case KVM_S390_INITIAL_RESET:
 		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
 		break;
-	case KVM_SET_ONE_REG:
-	case KVM_GET_ONE_REG: {
-		struct kvm_one_reg reg;
-		r = -EFAULT;
-		if (copy_from_user(&reg, argp, sizeof(reg)))
-			break;
-		if (ioctl == KVM_SET_ONE_REG)
-			r = kvm_arch_vcpu_ioctl_set_one_reg(vcpu, &reg);
-		else
-			r = kvm_arch_vcpu_ioctl_get_one_reg(vcpu, &reg);
-		break;
-	}
 #ifdef CONFIG_KVM_S390_UCONTROL
 	case KVM_S390_UCAS_MAP: {
 		struct kvm_s390_ucas_mapping ucasmap;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 9c0b3c3..2277ff8 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -583,6 +583,10 @@ void kvm_arch_hardware_unsetup(void);
 void kvm_arch_check_processor_compat(void *rtn);
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu);
 int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu);
+#ifdef KVM_HAVE_ONE_REG
+int kvm_arch_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+#endif
 
 void kvm_free_physmem(struct kvm *kvm);
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 6425906..169a001 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2068,6 +2068,21 @@ out_free2:
 		r = 0;
 		break;
 	}
+#ifdef KVM_HAVE_ONE_REG
+	case KVM_SET_ONE_REG:
+	case KVM_GET_ONE_REG: {
+		struct kvm_one_reg reg;
+		r = -EFAULT;
+		if (copy_from_user(&reg, argp, sizeof(reg)))
+			goto out;
+		if (ioctl == KVM_SET_ONE_REG)
+			r = kvm_arch_set_reg(vcpu, &reg);
+		else
+			r = kvm_arch_get_reg(vcpu, &reg);
+		break;
+	}
+#endif
+
 	default:
 		r = kvm_arch_vcpu_ioctl(filp, ioctl, arg);
 	}
@@ -2379,6 +2394,9 @@ static long kvm_dev_ioctl_check_extension_generic(long arg)
 #ifdef CONFIG_HAVE_KVM_MSI
 	case KVM_CAP_SIGNAL_MSI:
 #endif
+#ifdef KVM_HAVE_ONE_REG
+	case KVM_CAP_ONE_REG:
+#endif
 		return 1;
 #ifdef KVM_CAP_IRQ_ROUTING
 	case KVM_CAP_IRQ_ROUTING:
-- 
1.7.9.5

[PATCH 2/3] KVM: Add KVM_REG_SIZE() helper.

[Rusty Russell]

Useful helper for getting length of register given id.

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
---
 include/linux/kvm.h |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index d808694..8c3760e 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -744,6 +744,8 @@ struct kvm_dirty_tlb {
 #define KVM_REG_SIZE_U256	0x0050000000000000ULL
 #define KVM_REG_SIZE_U512	0x0060000000000000ULL
 #define KVM_REG_SIZE_U1024	0x0070000000000000ULL
+#define KVM_REG_SIZE(id)						\
+	(1U << (((id) & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT))
 
 struct kvm_one_reg {
 	__u64 id;
-- 
1.7.9.5

[PATCH 3/3] KVM: Add KVM_VCPU_GET_REG_LIST/KVM_CAP_REG_LIST.

[Rusty Russell]

This is a generic interface to find out what you can use
KVM_GET_ONE_REG/KVM_SET_ONE_REG on.  Archs need to define
KVM_HAVE_REG_LIST and then kvm_arch_num_regs() and
kvm_arch_copy_reg_indices() functions.

It's inspired by KVM_GET_MSR_INDEX_LIST, except it's a per-vcpu ioctl,
and uses 64-bit indices.

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
---
 Documentation/virtual/kvm/api.txt |   20 ++++++++++++++++++++
 include/linux/kvm.h               |   12 ++++++++++++
 include/linux/kvm_host.h          |    5 ++++-
 virt/kvm/kvm_main.c               |   20 ++++++++++++++++++++
 4 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index b91bfd4..f30c3d0 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1985,6 +1985,26 @@ the virtualized real-mode area (VRMA) facility, the kernel will
 re-create the VMRA HPTEs on the next KVM_RUN of any vcpu.)
 
 
+4.76 KVM_VCPU_GET_REG_LIST
+
+Capability: KVM_CAP_REG_LIST
+Architectures: all
+Type: vcpu ioctl
+Parameters: struct kvm_reg_list (in/out)
+Returns: 0 on success; -1 on error
+Errors:
+  E2BIG:     the reg index list is too big to fit in the array specified by
+             the user (the number required will be written into n).
+
+struct kvm_reg_list {
+	__u64 n; /* number of registers in reg[] */
+	__u64 reg[0];
+};
+
+This ioctl returns the guest registers that are supported for the
+KVM_GET_ONE_REG/KVM_SET_ONE_REG calls.
+
+
 5. The kvm_run structure
 ------------------------
 
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 8c3760e..e839889 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -625,6 +625,7 @@ struct kvm_ppc_smmu_info {
 #ifdef __KVM_HAVE_READONLY_MEM
 #define KVM_CAP_READONLY_MEM 81
 #endif
+#define KVM_CAP_REG_LIST 82
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -913,6 +914,7 @@ struct kvm_s390_ucas_mapping {
 #define KVM_SET_ONE_REG		  _IOW(KVMIO,  0xac, struct kvm_one_reg)
 /* VM is being stopped by host */
 #define KVM_KVMCLOCK_CTRL	  _IO(KVMIO,   0xad)
+#define KVM_VCPU_GET_REG_LIST	  _IOWR(KVMIO, 0xb0, struct kvm_reg_list)
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 #define KVM_DEV_ASSIGN_PCI_2_3		(1 << 1)
@@ -964,4 +971,9 @@ struct kvm_assigned_msix_entry {
 	__u16 padding[3];
 };
 
+/* For KVM_VCPU_GET_REG_LIST. */
+struct kvm_reg_list {
+	__u64 n; /* number of regs */
+	__u64 reg[0];
+};
 #endif /* __LINUX_KVM_H */
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 2277ff8..d9ee33f 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -587,7 +587,10 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu);
 int kvm_arch_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
 int kvm_arch_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
 #endif
-
+#ifdef KVM_HAVE_REG_LIST
+unsigned long kvm_arch_num_regs(struct kvm_vcpu *vcpu);
+int kvm_arch_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *indices);
+#endif
 void kvm_free_physmem(struct kvm *kvm);
 
 void *kvm_kvzalloc(unsigned long size);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 169a001..453fe93 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2082,6 +2082,23 @@ out_free2:
 		break;
 	}
 #endif
+#ifdef KVM_HAVE_REG_LIST
+	case KVM_VCPU_GET_REG_LIST: {
+		struct kvm_reg_list __user *user_list = argp;
+		struct kvm_reg_list reg_list;
+		unsigned n;
+
+		if (copy_from_user(&reg_list, user_list, sizeof reg_list))
+			return -EFAULT;
+		n = reg_list.n;
+		reg_list.n = kvm_arch_num_regs(vcpu);
+		if (copy_to_user(user_list, &reg_list, sizeof reg_list))
+			return -EFAULT;
+		if (n < reg_list.n)
+			return -E2BIG;
+		return kvm_arch_copy_reg_indices(vcpu, user_list->reg);
+	}
+#endif
 
 	default:
 		r = kvm_arch_vcpu_ioctl(filp, ioctl, arg);
@@ -2397,6 +2414,9 @@ static long kvm_dev_ioctl_check_extension_generic(long arg)
 #ifdef KVM_HAVE_ONE_REG
 	case KVM_CAP_ONE_REG:
 #endif
+#ifdef KVM_HAVE_REG_LIST
+	case KVM_CAP_REG_LIST:
+#endif
 		return 1;
 #ifdef KVM_CAP_IRQ_ROUTING
 	case KVM_CAP_IRQ_ROUTING:
-- 
1.7.9.5

Re: [PATCH 1/3] KVM: Move KVM_SET_ONE_REG/KVM_GET_ONE_REG to generic code.

[Alexander Graf]

On 05.09.2012, at 09:58, Rusty Russell wrote:

> Avi has indicated that this is the future.  For now, make it dependent on
> KVM_HAVE_ONE_REG (and define that for PPC and S/390).
> 
> Signed-off-by: Rusty Russell <rusty.russell@linaro.org>

Acked-by: Alexander Graf <agraf@suse.de>


Alex

Re: [PATCH 2/3] KVM: Add KVM_REG_SIZE() helper.

[Alexander Graf]

On 05.09.2012, at 09:58, Rusty Russell wrote:

> Useful helper for getting length of register given id.
> 
> Signed-off-by: Rusty Russell <rusty.russell@linaro.org>

Acked-by: Alexander Graf <agraf@suse.de>


Alex

> ---
> include/linux/kvm.h |    2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index d808694..8c3760e 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -744,6 +744,8 @@ struct kvm_dirty_tlb {
> #define KVM_REG_SIZE_U256	0x0050000000000000ULL
> #define KVM_REG_SIZE_U512	0x0060000000000000ULL
> #define KVM_REG_SIZE_U1024	0x0070000000000000ULL
> +#define KVM_REG_SIZE(id)						\
> +	(1U << (((id) & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT))
> 
> struct kvm_one_reg {
> 	__u64 id;
> -- 
> 1.7.9.5
> 

Re: [PATCH 3/3] KVM: Add KVM_VCPU_GET_REG_LIST/KVM_CAP_REG_LIST.

[Alexander Graf]

On 05.09.2012, at 09:58, Rusty Russell wrote:

> This is a generic interface to find out what you can use
> KVM_GET_ONE_REG/KVM_SET_ONE_REG on.  Archs need to define
> KVM_HAVE_REG_LIST and then kvm_arch_num_regs() and
> kvm_arch_copy_reg_indices() functions.
> 
> It's inspired by KVM_GET_MSR_INDEX_LIST, except it's a per-vcpu ioctl,
> and uses 64-bit indices.
> 
> Signed-off-by: Rusty Russell <rusty.russell@linaro.org>

Acked-by: Alexander Graf <agraf@suse.de>


Alex

Re: [PATCH 3/3] KVM: Add KVM_VCPU_GET_REG_LIST/KVM_CAP_REG_LIST.

[Marcelo Tosatti]

On Wed, Sep 05, 2012 at 05:28:46PM +0930, Rusty Russell wrote:
> This is a generic interface to find out what you can use
> KVM_GET_ONE_REG/KVM_SET_ONE_REG on.  Archs need to define
> KVM_HAVE_REG_LIST and then kvm_arch_num_regs() and
> kvm_arch_copy_reg_indices() functions.
> 
> It's inspired by KVM_GET_MSR_INDEX_LIST, except it's a per-vcpu ioctl,
> and uses 64-bit indices.
> 
> Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
> ---
>  Documentation/virtual/kvm/api.txt |   20 ++++++++++++++++++++
>  include/linux/kvm.h               |   12 ++++++++++++
>  include/linux/kvm_host.h          |    5 ++++-
>  virt/kvm/kvm_main.c               |   20 ++++++++++++++++++++
>  4 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index b91bfd4..f30c3d0 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1985,6 +1985,26 @@ the virtualized real-mode area (VRMA) facility, the kernel will
>  re-create the VMRA HPTEs on the next KVM_RUN of any vcpu.)
>  
>  
> +4.76 KVM_VCPU_GET_REG_LIST
> +
> +Capability: KVM_CAP_REG_LIST
> +Architectures: all
> +Type: vcpu ioctl
> +Parameters: struct kvm_reg_list (in/out)
> +Returns: 0 on success; -1 on error
> +Errors:
> +  E2BIG:     the reg index list is too big to fit in the array specified by
> +             the user (the number required will be written into n).
> +
> +struct kvm_reg_list {
> +	__u64 n; /* number of registers in reg[] */
> +	__u64 reg[0];
> +};
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 169a001..453fe93 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2082,6 +2082,23 @@ out_free2:
>  		break;
>  	}
>  #endif
> +#ifdef KVM_HAVE_REG_LIST
> +	case KVM_VCPU_GET_REG_LIST: {
> +		struct kvm_reg_list __user *user_list = argp;
> +		struct kvm_reg_list reg_list;
> +		unsigned n;
> +
> +		if (copy_from_user(&reg_list, user_list, sizeof reg_list))
> +			return -EFAULT;
> +		n = reg_list.n;
> +		reg_list.n = kvm_arch_num_regs(vcpu);

The code does not actually support more than 2^32 registers, does it?
Why "__u64 n" ?

Re: [PATCH 3/3] KVM: Add KVM_VCPU_GET_REG_LIST/KVM_CAP_REG_LIST.

[Rusty Russell]

Marcelo Tosatti <mtosatti@redhat.com> writes:
> On Wed, Sep 05, 2012 at 05:28:46PM +0930, Rusty Russell wrote:
...
>> +struct kvm_reg_list {
>> +	__u64 n; /* number of registers in reg[] */
>> +	__u64 reg[0];
>> +};
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 169a001..453fe93 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -2082,6 +2082,23 @@ out_free2:
>>  		break;
>>  	}
>>  #endif
>> +#ifdef KVM_HAVE_REG_LIST
>> +	case KVM_VCPU_GET_REG_LIST: {
>> +		struct kvm_reg_list __user *user_list = argp;
>> +		struct kvm_reg_list reg_list;
>> +		unsigned n;
>> +
>> +		if (copy_from_user(&reg_list, user_list, sizeof reg_list))
>> +			return -EFAULT;
>> +		n = reg_list.n;
>> +		reg_list.n = kvm_arch_num_regs(vcpu);
>
> The code does not actually support more than 2^32 registers, does it?
> Why "__u64 n" ?

Well, the interface is simpler, and the alignment issues vanish.

kvm_arch_num_regs could return a 64-bit number in future if we want to
get completely insane :)

Cheers,
Rusty.

Re: [PATCH 0/3] KVM_VCPU_GET_REG_LIST API

[Avi Kivity]

On 09/05/2012 10:58 AM, Rusty Russell wrote:
> This is the generic part of the KVM_SET_ONE_REG/KVM_GET_ONE_REG
> enhancements which ARM wants, rebased onto kvm/next.

This was stalled for so long it needs rebasing again, sorry.

But otherwise I'm happy to apply.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/3] KVM_VCPU_GET_REG_LIST API

[Rusty Russell]

Avi Kivity <avi@redhat.com> writes:
> On 09/05/2012 10:58 AM, Rusty Russell wrote:
>> This is the generic part of the KVM_SET_ONE_REG/KVM_GET_ONE_REG
>> enhancements which ARM wants, rebased onto kvm/next.
>
> This was stalled for so long it needs rebasing again, sorry.
>
> But otherwise I'm happy to apply.

Ok, will rebase and re-test against kvm-next.

Cheers,
Rusty.

Re: [PATCH 0/3] KVM_VCPU_GET_REG_LIST API

[Rusty Russell]

Rusty Russell <rusty@rustcorp.com.au> writes:
> Avi Kivity <avi@redhat.com> writes:
>> On 09/05/2012 10:58 AM, Rusty Russell wrote:
>>> This is the generic part of the KVM_SET_ONE_REG/KVM_GET_ONE_REG
>>> enhancements which ARM wants, rebased onto kvm/next.
>>
>> This was stalled for so long it needs rebasing again, sorry.
>>
>> But otherwise I'm happy to apply.
>
> Ok, will rebase and re-test against kvm-next.

Wait, what?  kvm/arm isn't in kvm-next?

This will produce a needless clash with that, which is more important
than this cleanup.  I'll rebase this as soon as that is merged.

Christoffer, is there anything I can help with?

Cheers,
Rusty.

Re: [PATCH 0/3] KVM_VCPU_GET_REG_LIST API

[Christoffer Dall]

On Fri, Oct 19, 2012 at 2:19 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Rusty Russell <rusty@rustcorp.com.au> writes:
>> Avi Kivity <avi@redhat.com> writes:
>>> On 09/05/2012 10:58 AM, Rusty Russell wrote:
>>>> This is the generic part of the KVM_SET_ONE_REG/KVM_GET_ONE_REG
>>>> enhancements which ARM wants, rebased onto kvm/next.
>>>
>>> This was stalled for so long it needs rebasing again, sorry.
>>>
>>> But otherwise I'm happy to apply.
>>
>> Ok, will rebase and re-test against kvm-next.
>
> Wait, what?  kvm/arm isn't in kvm-next?
>
> This will produce a needless clash with that, which is more important
> than this cleanup.  I'll rebase this as soon as that is merged.
>
> Christoffer, is there anything I can help with?
>

There are some worries about duplicating functionality on the ARM side
of things.

Specifically there are worries about the instruction decoding for the
mmio instructions. My cycles are unfortunately too limited to change
this right now and I'm also not sure I agree things will turn out
nicer by unifying all decoding into a large complicated space ship,
but it would be great if you could take a look. This discussion seems
to be a good place to start:

https://lists.cs.columbia.edu/pipermail/kvmarm/2012-September/003447.html

Thanks!
-Christoffer

Re: [PATCH 0/3] KVM_VCPU_GET_REG_LIST API

[Rusty Russell]

Christoffer Dall <c.dall@virtualopensystems.com> writes:
> On Fri, Oct 19, 2012 at 2:19 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Wait, what?  kvm/arm isn't in kvm-next?
>> Christoffer, is there anything I can help with?
>
> Specifically there are worries about the instruction decoding for the
> mmio instructions. My cycles are unfortunately too limited to change
> this right now and I'm also not sure I agree things will turn out
> nicer by unifying all decoding into a large complicated space ship,
> but it would be great if you could take a look. This discussion seems
> to be a good place to start:
>
> https://lists.cs.columbia.edu/pipermail/kvmarm/2012-September/003447.html

They're still asking you to boil that ocean??

I could create a struct and do simple decode into it for limited cases
(ie. for kvm).  Will, do you want to see that?

But unifying them all is a much larger task, and only when that's all
done can you judge whether it was worthwhile.  I've spend half an hour
looking and each case is subtly different, and the conversion has to be
incredibly careful not to break them.  And converting opcodes.c is just
ideology; it's great as it is.

I'm interested in the 64-bit ARM kvm, because it would be nice to unify
the two implementations.  But the ABI will be different anyway (64 bit
regs get their own id space even if nothing else changes).

Let's get the current code shipped please, it's only going to bitrot
outside the tree.

Cheers,
Rusty.

Re: [PATCH 0/3] KVM_VCPU_GET_REG_LIST API

[Will Deacon]

On Mon, Oct 22, 2012 at 04:09:06AM +0100, Rusty Russell wrote:
> Christoffer Dall <c.dall@virtualopensystems.com> writes:
> > On Fri, Oct 19, 2012 at 2:19 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> >> Wait, what?  kvm/arm isn't in kvm-next?
> >> Christoffer, is there anything I can help with?
> >
> > Specifically there are worries about the instruction decoding for the
> > mmio instructions. My cycles are unfortunately too limited to change
> > this right now and I'm also not sure I agree things will turn out
> > nicer by unifying all decoding into a large complicated space ship,
> > but it would be great if you could take a look. This discussion seems
> > to be a good place to start:
> >
> > https://lists.cs.columbia.edu/pipermail/kvmarm/2012-September/003447.html
> 
> They're still asking you to boil that ocean??
> 
> I could create a struct and do simple decode into it for limited cases
> (ie. for kvm).  Will, do you want to see that?

Yes, I think that would be great! Basically, I'd like the code to be
reusable so that other subsystems (including uprobes as of last week) can
plug into if it possible. The actual plugging in of those subsystems is
obviously not up to you.

Dave posted an idea here:

  http://lists.infradead.org/pipermail/linux-arm-kernel/2012-October/123464.html

which could form a basic starting block for something like load/store
decoding. It looks like Christoffer has actually done a bunch of this for
v3.

> But unifying them all is a much larger task, and only when that's all
> done can you judge whether it was worthwhile.  I've spend half an hour
> looking and each case is subtly different, and the conversion has to be
> incredibly careful not to break them.  And converting opcodes.c is just
> ideology; it's great as it is.

opcodes.c may be where this stuff ultimately ends up. In the meantime, you
could try moving what you have for v3 into common ARM code so that other
people can try to use it. In fact, if you don't even want to do that, just
put it in its own file under arch/arm/kvm/ so that the interface to
emulate.c doesn't make it too hard to move around in future.

> I'm interested in the 64-bit ARM kvm, because it would be nice to unify
> the two implementations.  But the ABI will be different anyway (64 bit
> regs get their own id space even if nothing else changes).

Assumedly you'll need a wrapper around the 32-bit ABI in order to run 32-bit
guests under a 64-bit kernel, so unification is definitely a good idea.

Will

Re: [PATCH 0/3] KVM_VCPU_GET_REG_LIST API

[Christoffer Dall]

On Mon, Oct 22, 2012 at 1:45 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Mon, Oct 22, 2012 at 04:09:06AM +0100, Rusty Russell wrote:
>> Christoffer Dall <c.dall@virtualopensystems.com> writes:
>> > On Fri, Oct 19, 2012 at 2:19 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> >> Wait, what?  kvm/arm isn't in kvm-next?
>> >> Christoffer, is there anything I can help with?
>> >
>> > Specifically there are worries about the instruction decoding for the
>> > mmio instructions. My cycles are unfortunately too limited to change
>> > this right now and I'm also not sure I agree things will turn out
>> > nicer by unifying all decoding into a large complicated space ship,
>> > but it would be great if you could take a look. This discussion seems
>> > to be a good place to start:
>> >
>> > https://lists.cs.columbia.edu/pipermail/kvmarm/2012-September/003447.html
>>
>> They're still asking you to boil that ocean??
>>
>> I could create a struct and do simple decode into it for limited cases
>> (ie. for kvm).  Will, do you want to see that?
>
> Yes, I think that would be great! Basically, I'd like the code to be
> reusable so that other subsystems (including uprobes as of last week) can
> plug into if it possible. The actual plugging in of those subsystems is
> obviously not up to you.
>
> Dave posted an idea here:
>
>   http://lists.infradead.org/pipermail/linux-arm-kernel/2012-October/123464.html
>
> which could form a basic starting block for something like load/store
> decoding. It looks like Christoffer has actually done a bunch of this for
> v3.
>

The issue is that the decoding assumes that you're only going to
decode instructions that don't carry decode information in the HSR.
For example, we don't do anything special about unprivileged
load/store, because they would have failed in their stage 1
translation and we wouldn't even get to the decoding in KVM. There are
also a number of corner cases such as loading into the PC from an MMIO
address that we don't need to worry about as we simply dismiss it as
being insane guest behavior. Another example is all the checking of
the write-back cases, since we can simply assume that there will be a
write-back in case we're decoding anything.

We also don't decode load/store multiples (although Antonios Motakis
did work up a patch for this some time ago for the ARM mode), since
the user space ABI to communicate the MMIO operations don't support
multiple registers and reworking that on the architecture-generic
level for some theoretical non-existing guest is simply not something
worth the time.

The point is that we cannot just take the code that is there now and
make it available generically within arch/arm without a lot of work,
including coming up with a test framework and verify it, and as Rusty
says, even then we don't know if the whole thing will look so
complicated that we deem it not worth the effort end.

And, to repeat my favorite argument, this can always be changed after
merging KVM. Alternatively, we can remove the mmio encoding completely
from the patch series and rely on your patch for mmio accessors in any
guest kernel, but I think this is a shame for people wanting to try
slightly more exotic things like an older kernel, when we have code
that's working.

Please, please, consider signing off on the current stages of the
patches if nothing else ground breaking pops up.

>> But unifying them all is a much larger task, and only when that's all
>> done can you judge whether it was worthwhile.  I've spend half an hour
>> looking and each case is subtly different, and the conversion has to be
>> incredibly careful not to break them.  And converting opcodes.c is just
>> ideology; it's great as it is.
>
> opcodes.c may be where this stuff ultimately ends up. In the meantime, you
> could try moving what you have for v3 into common ARM code so that other
> people can try to use it. In fact, if you don't even want to do that, just
> put it in its own file under arch/arm/kvm/ so that the interface to
> emulate.c doesn't make it too hard to move around in future.
>
>> I'm interested in the 64-bit ARM kvm, because it would be nice to unify
>> the two implementations.  But the ABI will be different anyway (64 bit
>> regs get their own id space even if nothing else changes).
>
> Assumedly you'll need a wrapper around the 32-bit ABI in order to run 32-bit
> guests under a 64-bit kernel, so unification is definitely a good idea.
>
> Will

RFC: Move kvm's instruction decoding into generic code.

[Rusty Russell]

Compile-tested only, but shows what it looks like (ugly).

I just used the existing code and didn't crack open my ARM books to
determine if there's a better way.  The struct names are pretty vague,
too, as a result.

[PATCH 01/10] kvm: split out instruction structure from decoding method.

[Rusty Russell]

From: Rusty Russell <rusty.russell@linaro.org>

Add a new 'struct arm_insn' to represent the decoded instruction; the
decoding logic belong in a separate structure (arm_decode).

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
---
 arch/arm/kvm/emulate.c |  120 +++++++++++++++++++++++++++---------------------
 1 file changed, 68 insertions(+), 52 deletions(-)

diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
index 30124cb..e4fc12b 100644
--- a/arch/arm/kvm/emulate.c
+++ b/arch/arm/kvm/emulate.c
@@ -288,34 +288,37 @@ out:
 /******************************************************************************
  * Load-Store instruction emulation
  *****************************************************************************/
+enum SRType {
+	SRType_LSL,
+	SRType_LSR,
+	SRType_ASR,
+	SRType_ROR,
+	SRType_RRX
+};
 
-struct arm_instr {
-	/* Instruction decoding */
-	u32 opc;
-	u32 opc_mask;
-
+struct arm_insn {
 	/* Decoding for the register write back */
 	bool register_form;
 	u32 imm;
-	u8 Rm;
 	u8 type;
+	u8 Rt, Rn, Rm;
 	u8 shift_n;
 
 	/* Common decoding */
 	u8 len;
 	bool sign_extend;
 	bool w;
+};
+
+struct arm_decode {
+	/* Instruction decoding */
+	u32 opc;
+	u32 opc_mask;
 
 	bool (*decode)(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
-		       unsigned long instr, struct arm_instr *ai);
-};
+		       unsigned long instr, struct arm_insn *ai);
 
-enum SRType {
-	SRType_LSL,
-	SRType_LSR,
-	SRType_ASR,
-	SRType_ROR,
-	SRType_RRX
+	struct arm_insn template;
 };
 
 /* Modelled after DecodeImmShift() in the ARM ARM */
@@ -383,7 +386,7 @@ u32 shift(u32 value, u8 N, enum SRType type, u8 amount, bool carry_in)
 }
 
 static bool decode_arm_wb(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
-			  unsigned long instr, const struct arm_instr *ai)
+			  unsigned long instr, const struct arm_insn *ai)
 {
 	u8 Rt = (instr >> 12) & 0xf;
 	u8 Rn = (instr >> 16) & 0xf;
@@ -431,7 +434,7 @@ static bool decode_arm_wb(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
 }
 
 static bool decode_arm_ls(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
-			  unsigned long instr, struct arm_instr *ai)
+			  unsigned long instr, struct arm_insn *ai)
 {
 	u8 A = (instr >> 25) & 1;
 
@@ -449,7 +452,7 @@ static bool decode_arm_ls(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
 }
 
 static bool decode_arm_extra(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
-			     unsigned long instr, struct arm_instr *ai)
+			     unsigned long instr, struct arm_insn *ai)
 {
 	mmio->is_write = ai->w;
 	mmio->len = ai->len;
@@ -476,56 +479,69 @@ static bool decode_arm_extra(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
  * loads and stores as their encodings mandate the W bit set and the P bit
  * clear.
  */
-static const struct arm_instr arm_instr[] = {
+static const struct arm_decode arm_decode[] = {
 	/**************** Load/Store Word and Byte **********************/
 	/* Store word with writeback */
-	{ .opc = 0x04000000, .opc_mask = 0x0c500000, .len = 4, .w = true,
-		.sign_extend = false, .decode = decode_arm_ls },
+	{ .opc = 0x04000000, .opc_mask = 0x0c500000,
+	  .decode = decode_arm_ls,
+	  .template =  { .len = 4, .w = true, .sign_extend = false, }, },
 	/* Store byte with writeback */
-	{ .opc = 0x04400000, .opc_mask = 0x0c500000, .len = 1, .w = true,
-		.sign_extend = false, .decode = decode_arm_ls },
+	{ .opc = 0x04400000, .opc_mask = 0x0c500000,
+	  .decode = decode_arm_ls,
+	  .template = {  .len = 1, .w = true, .sign_extend = false, }, },
 	/* Load word with writeback */
-	{ .opc = 0x04100000, .opc_mask = 0x0c500000, .len = 4, .w = false,
-		.sign_extend = false, .decode = decode_arm_ls },
+	{ .opc = 0x04100000, .opc_mask = 0x0c500000,
+	  .decode = decode_arm_ls,
+	  .template = {  .len = 4, .w = false, .sign_extend = false, }, },
 	/* Load byte with writeback */
-	{ .opc = 0x04500000, .opc_mask = 0x0c500000, .len = 1, .w = false,
-		.sign_extend = false, .decode = decode_arm_ls },
+	{ .opc = 0x04500000, .opc_mask = 0x0c500000,
+	  .decode = decode_arm_ls,
+	  .template = { .len = 1, .w = false, .sign_extend = false, }, },
 
 	/*************** Extra load/store instructions ******************/
 
 	/* Store halfword with writeback */
-	{ .opc = 0x000000b0, .opc_mask = 0x0c1000f0, .len = 2, .w = true,
-		.sign_extend = false, .decode = decode_arm_extra },
+	{ .opc = 0x000000b0, .opc_mask = 0x0c1000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 2, .w = true, .sign_extend = false, }, },
 	/* Load halfword with writeback */
-	{ .opc = 0x001000b0, .opc_mask = 0x0c1000f0, .len = 2, .w = false,
-		.sign_extend = false, .decode = decode_arm_extra },
-
+	{ .opc = 0x001000b0, .opc_mask = 0x0c1000f0, 
+	  .decode = decode_arm_extra,
+	  .template = { .len = 2, .w = false, .sign_extend = false, }, },
 	/* Load dual with writeback */
-	{ .opc = 0x000000d0, .opc_mask = 0x0c1000f0, .len = 8, .w = false,
-		.sign_extend = false, .decode = decode_arm_extra },
+	{ .opc = 0x000000d0, .opc_mask = 0x0c1000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 8, .w = false, .sign_extend = false, }, },
 	/* Load signed byte with writeback */
-	{ .opc = 0x001000d0, .opc_mask = 0x0c1000f0, .len = 1, .w = false,
-		.sign_extend = true,  .decode = decode_arm_extra },
+	{ .opc = 0x001000d0, .opc_mask = 0x0c1000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 1, .w = false, .sign_extend = true, }, },
 
 	/* Store dual with writeback */
-	{ .opc = 0x000000f0, .opc_mask = 0x0c1000f0, .len = 8, .w = true,
-		.sign_extend = false, .decode = decode_arm_extra },
+	{ .opc = 0x000000f0, .opc_mask = 0x0c1000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 8, .w = true, .sign_extend = false, }, },
 	/* Load signed halfword with writeback */
-	{ .opc = 0x001000f0, .opc_mask = 0x0c1000f0, .len = 2, .w = false,
-		.sign_extend = true,  .decode = decode_arm_extra },
+	{ .opc = 0x001000f0, .opc_mask = 0x0c1000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 2, .w = false, .sign_extend = true, }, },
 
 	/* Store halfword unprivileged */
-	{ .opc = 0x002000b0, .opc_mask = 0x0f3000f0, .len = 2, .w = true,
-		.sign_extend = false, .decode = decode_arm_extra },
+	{ .opc = 0x002000b0, .opc_mask = 0x0f3000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 2, .w = true, .sign_extend = false, }, },
 	/* Load halfword unprivileged */
-	{ .opc = 0x003000b0, .opc_mask = 0x0f3000f0, .len = 2, .w = false,
-		.sign_extend = false, .decode = decode_arm_extra },
+	{ .opc = 0x003000b0, .opc_mask = 0x0f3000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 2, .w = false, .sign_extend = false, }, },
 	/* Load signed byte unprivileged */
-	{ .opc = 0x003000d0, .opc_mask = 0x0f3000f0, .len = 1, .w = false,
-		.sign_extend = true , .decode = decode_arm_extra },
+	{ .opc = 0x003000d0, .opc_mask = 0x0f3000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 1, .w = false, .sign_extend = true, }, },
 	/* Load signed halfword unprivileged */
-	{ .opc = 0x003000d0, .opc_mask = 0x0f3000f0, .len = 2, .w = false,
-		.sign_extend = true , .decode = decode_arm_extra },
+	{ .opc = 0x003000d0, .opc_mask = 0x0f3000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 2, .w = false, .sign_extend = true, }, },
 };
 
 static bool kvm_decode_arm_ls(struct kvm_vcpu *vcpu, unsigned long instr,
@@ -533,11 +549,11 @@ static bool kvm_decode_arm_ls(struct kvm_vcpu *vcpu, unsigned long instr,
 {
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(arm_instr); i++) {
-		const struct arm_instr *ai = &arm_instr[i];
-		if ((instr & ai->opc_mask) == ai->opc) {
-			struct arm_instr ai_copy = *ai;
-			return ai->decode(vcpu, mmio, instr, &ai_copy);
+	for (i = 0; i < ARRAY_SIZE(arm_decode); i++) {
+		const struct arm_decode *d = &arm_decode[i];
+		if ((instr & d->opc_mask) == d->opc) {
+			struct arm_insn ai = d->template;
+			return d->decode(vcpu, mmio, instr, &ai);
 		}
 	}
 	return false;
-- 
1.7.10.4

[PATCH 02/10] kvm: split out instruction decode from emulation.

[Rusty Russell]

From: Rusty Russell <rusty.russell@linaro.org>

This adds more fields which we need to store in the struct arm_insn,
so execute() knows what to do.

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
---
 arch/arm/kvm/emulate.c |   80 ++++++++++++++++++++++++++----------------------
 1 file changed, 43 insertions(+), 37 deletions(-)

diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
index e4fc12b..05b534f 100644
--- a/arch/arm/kvm/emulate.c
+++ b/arch/arm/kvm/emulate.c
@@ -303,11 +303,12 @@ struct arm_insn {
 	u8 type;
 	u8 Rt, Rn, Rm;
 	u8 shift_n;
+	u32 offset_addr;
 
 	/* Common decoding */
 	u8 len;
 	bool sign_extend;
-	bool w;
+	bool w, W, U, P;
 };
 
 struct arm_decode {
@@ -315,7 +316,7 @@ struct arm_decode {
 	u32 opc;
 	u32 opc_mask;
 
-	bool (*decode)(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
+	bool (*decode)(struct kvm_vcpu *vcpu,
 		       unsigned long instr, struct arm_insn *ai);
 
 	struct arm_insn template;
@@ -385,31 +386,24 @@ u32 shift(u32 value, u8 N, enum SRType type, u8 amount, bool carry_in)
 	return value & mask;
 }
 
-static bool decode_arm_wb(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
-			  unsigned long instr, const struct arm_insn *ai)
+static bool decode_arm_wb(struct kvm_vcpu *vcpu, unsigned long instr,
+			  struct arm_insn *ai)
 {
-	u8 Rt = (instr >> 12) & 0xf;
-	u8 Rn = (instr >> 16) & 0xf;
-	u8 W = (instr >> 21) & 1;
-	u8 U = (instr >> 23) & 1;
-	u8 P = (instr >> 24) & 1;
-	u32 base_addr = *vcpu_reg(vcpu, Rn);
-	u32 offset_addr, offset;
+	u32 base_addr, offset;
 
-	/*
-	 * Technically this is allowed in certain circumstances,
-	 * but we don't support it.
-	 */
-	if (Rt == 15 || Rn == 15)
-		return false;
+	ai->Rt = (instr >> 12) & 0xf;
+	ai->Rn = (instr >> 16) & 0xf;
+	ai->W = (instr >> 21) & 1;
+	ai->U = (instr >> 23) & 1;
+	ai->P = (instr >> 24) & 1;
+
+	base_addr = *vcpu_reg(vcpu, ai->Rn);
 
-	if (P && !W) {
+	if (ai->P && !ai->W) {
 		kvm_err("Decoding operation with valid ISV?\n");
 		return false;
 	}
 
-	vcpu->arch.mmio.rd = Rt;
-
 	if (ai->register_form) {
 		/* Register operation */
 		enum SRType s_type;
@@ -425,46 +419,56 @@ static bool decode_arm_wb(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
 	}
 
 	/* Handle Writeback */
-	if (U)
-		offset_addr = base_addr + offset;
+	if (ai->U)
+		ai->offset_addr = base_addr + offset;
 	else
-		offset_addr = base_addr - offset;
-	*vcpu_reg(vcpu, Rn) = offset_addr;
+		ai->offset_addr = base_addr - offset;
 	return true;
 }
 
-static bool decode_arm_ls(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
+static bool decode_arm_ls(struct kvm_vcpu *vcpu,
 			  unsigned long instr, struct arm_insn *ai)
 {
 	u8 A = (instr >> 25) & 1;
 
-	mmio->is_write = ai->w;
-	mmio->len = ai->len;
-	vcpu->arch.mmio.sign_extend = false;
-
 	ai->register_form = A;
 	ai->imm = instr & 0xfff;
 	ai->Rm = instr & 0xf;
 	ai->type = (instr >> 5) & 0x3;
 	ai->shift_n = (instr >> 7) & 0x1f;
 
-	return decode_arm_wb(vcpu, mmio, instr, ai);
+	return decode_arm_wb(vcpu, instr, ai);
 }
 
-static bool decode_arm_extra(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
+static bool decode_arm_extra(struct kvm_vcpu *vcpu,
 			     unsigned long instr, struct arm_insn *ai)
 {
-	mmio->is_write = ai->w;
-	mmio->len = ai->len;
-	vcpu->arch.mmio.sign_extend = ai->sign_extend;
-
 	ai->register_form = !((instr >> 22) & 1);
 	ai->imm = ((instr >> 4) & 0xf0) | (instr & 0xf);
 	ai->Rm = instr & 0xf;
 	ai->type = 0; /* SRType_LSL */
 	ai->shift_n = 0;
 
-	return decode_arm_wb(vcpu, mmio, instr, ai);
+	return decode_arm_wb(vcpu, instr, ai);
+}
+
+static bool execute(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
+		    const struct arm_insn *ai)
+{
+	/*
+	 * Technically this is allowed in certain circumstances,
+	 * but we don't support it.
+	 */
+	if (ai->Rt == 15 || ai->Rn == 15)
+		return false;
+
+	mmio->is_write = ai->w;
+	mmio->len = ai->len;
+	vcpu->arch.mmio.sign_extend = ai->sign_extend;
+
+	vcpu->arch.mmio.rd = ai->Rt;
+	*vcpu_reg(vcpu, ai->Rn) = ai->offset_addr;
+	return true;
 }
 
 /*
@@ -553,7 +557,9 @@ static bool kvm_decode_arm_ls(struct kvm_vcpu *vcpu, unsigned long instr,
 		const struct arm_decode *d = &arm_decode[i];
 		if ((instr & d->opc_mask) == d->opc) {
 			struct arm_insn ai = d->template;
-			return d->decode(vcpu, mmio, instr, &ai);
+			if (!d->decode(vcpu, instr, &ai))
+				return false;
+			return execute(vcpu, mmio, &ai);
 		}
 	}
 	return false;
-- 
1.7.10.4

[PATCH 03/10] kvm: split out instruction decode from emulation (thumb instructions).

[Rusty Russell]

From: Rusty Russell <rusty.russell@linaro.org>

Add thumb info to struct arm_insn, and use that to store decode
information for later handling in execute_thumb().

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
---
 arch/arm/kvm/emulate.c |  120 +++++++++++++++++++++++++++++-------------------
 1 file changed, 74 insertions(+), 46 deletions(-)

diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
index 05b534f..882db33 100644
--- a/arch/arm/kvm/emulate.c
+++ b/arch/arm/kvm/emulate.c
@@ -301,7 +301,7 @@ struct arm_insn {
 	bool register_form;
 	u32 imm;
 	u8 type;
-	u8 Rt, Rn, Rm;
+	u8 Rt, Rn, Rm, Rd;
 	u8 shift_n;
 	u32 offset_addr;
 
@@ -309,6 +309,21 @@ struct arm_insn {
 	u8 len;
 	bool sign_extend;
 	bool w, W, U, P;
+
+	/* Thumb encoding */
+	bool is_thumb, is_thumb32;
+	union {
+		struct {
+			u8 opcode;
+			u8 mask;
+		} t16;
+
+		struct {
+			u8 op1;
+			u8 op2;
+			u8 op2_mask;
+		} t32;
+	};
 };
 
 struct arm_decode {
@@ -565,7 +580,7 @@ static bool kvm_decode_arm_ls(struct kvm_vcpu *vcpu, unsigned long instr,
 	return false;
 }
 
-struct thumb_instr {
+struct thumb_decode {
 	bool is32;
 
 	union {
@@ -581,86 +596,95 @@ struct thumb_instr {
 		} t32;
 	};
 
-	bool (*decode)(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
-		       unsigned long instr, const struct thumb_instr *ti);
+	bool (*decode)(struct kvm_vcpu *vcpu,
+		       unsigned long instr, struct arm_insn *ti);
 };
 
-static bool decode_thumb_wb(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
-			    unsigned long instr)
+static bool decode_thumb_wb(struct kvm_vcpu *vcpu,
+			    unsigned long instr, struct arm_insn *ti)
 {
-	bool P = (instr >> 10) & 1;
-	bool U = (instr >> 9) & 1;
 	u8 imm8 = instr & 0xff;
 	u32 offset_addr = vcpu->arch.hxfar;
-	u8 Rn = (instr >> 16) & 0xf;
-
-	vcpu->arch.mmio.rd = (instr >> 12) & 0xf;
 
-	if (kvm_vcpu_reg_is_pc(vcpu, Rn))
-		return false;
+	ti->P = (instr >> 10) & 1;
+	ti->U = (instr >> 9) & 1;
+	ti->Rn = (instr >> 16) & 0xf;
+	ti->Rd = (instr >> 12) & 0xf;
 
 	/* Handle Writeback */
-	if (!P && U)
-		*vcpu_reg(vcpu, Rn) = offset_addr + imm8;
-	else if (!P && !U)
-		*vcpu_reg(vcpu, Rn) = offset_addr - imm8;
+	if (!ti->P && ti->U)
+		ti->offset_addr = offset_addr + imm8;
+	else if (!ti->P && !ti->U)
+		ti->offset_addr = offset_addr - imm8;
 	return true;
 }
 
-static bool decode_thumb_str(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
-			     unsigned long instr, const struct thumb_instr *ti)
+static bool decode_thumb_str(struct kvm_vcpu *vcpu,
+			     unsigned long instr, struct arm_insn *ti)
 {
 	u8 op1 = (instr >> (16 + 5)) & 0x7;
 	u8 op2 = (instr >> 6) & 0x3f;
 
-	mmio->is_write = true;
-	vcpu->arch.mmio.sign_extend = false;
+	ti->W = true;
+	ti->sign_extend = false;
 
 	switch (op1) {
-	case 0x0: mmio->len = 1; break;
-	case 0x1: mmio->len = 2; break;
-	case 0x2: mmio->len = 4; break;
+	case 0x0: ti->len = 1; break;
+	case 0x1: ti->len = 2; break;
+	case 0x2: ti->len = 4; break;
 	default:
 		  return false; /* Only register write-back versions! */
 	}
 
 	if ((op2 & 0x24) == 0x24) {
 		/* STRB (immediate, thumb, W=1) */
-		return decode_thumb_wb(vcpu, mmio, instr);
+		return decode_thumb_wb(vcpu, instr, ti);
 	}
 
 	return false;
 }
 
-static bool decode_thumb_ldr(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
-			     unsigned long instr, const struct thumb_instr *ti)
+static bool decode_thumb_ldr(struct kvm_vcpu *vcpu,
+			     unsigned long instr, struct arm_insn *ti)
 {
 	u8 op1 = (instr >> (16 + 7)) & 0x3;
 	u8 op2 = (instr >> 6) & 0x3f;
 
-	mmio->is_write = false;
+	ti->W = false;
 
 	switch (ti->t32.op2 & 0x7) {
-	case 0x1: mmio->len = 1; break;
-	case 0x3: mmio->len = 2; break;
-	case 0x5: mmio->len = 4; break;
+	case 0x1: ti->len = 1; break;
+	case 0x3: ti->len = 2; break;
+	case 0x5: ti->len = 4; break;
 	}
 
 	if (op1 == 0x0)
-		vcpu->arch.mmio.sign_extend = false;
+		ti->sign_extend = false;
 	else if (op1 == 0x2 && (ti->t32.op2 & 0x7) != 0x5)
-		vcpu->arch.mmio.sign_extend = true;
+		ti->sign_extend = true;
 	else
 		return false; /* Only register write-back versions! */
 
 	if ((op2 & 0x24) == 0x24) {
 		/* LDR{S}X (immediate, thumb, W=1) */
-		return decode_thumb_wb(vcpu, mmio, instr);
+		return decode_thumb_wb(vcpu, instr, ti);
 	}
 
 	return false;
 }
 
+static bool execute_thumb(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
+			  const struct arm_insn *ti)
+{
+	if (kvm_vcpu_reg_is_pc(vcpu, ti->Rn))
+		return false;
+
+	if (!ti->P)
+		*vcpu_reg(vcpu, ti->Rn) = ti->offset_addr;
+	vcpu->arch.mmio.sign_extend = ti->sign_extend;
+	return true;
+}
+
 /*
  * We only support instruction decoding for valid reasonable MMIO operations
  * where trapping them do not provide sufficient information in the HSR (no
@@ -673,7 +697,7 @@ static bool decode_thumb_ldr(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
  *  - any load/store dual
  *  - anything with the PC as the dest register
  */
-static const struct thumb_instr thumb_instr[] = {
+static const struct thumb_decode thumb_decode[] = {
 	/**************** 32-bit Thumb instructions **********************/
 	/* Store single data item:	Op1 == 11, Op2 == 000xxx0 */
 	{ .is32 = true,  .t32 = { 3, 0x00, 0x71}, decode_thumb_str	},
@@ -686,15 +710,17 @@ static const struct thumb_instr thumb_instr[] = {
 };
 
 
-
 static bool kvm_decode_thumb_ls(struct kvm_vcpu *vcpu, unsigned long instr,
 				struct kvm_exit_mmio *mmio)
 {
-	bool is32 = is_wide_instruction(instr);
-	bool is16 = !is32;
-	struct thumb_instr tinstr; /* re-use to pass on already decoded info */
+	struct arm_insn tinstr; /* re-use to pass on already decoded info */
+	bool is16;
 	int i;
 
+	tinstr.is_thumb = true;
+	tinstr.is_thumb32 = is_wide_instruction(instr);
+
+	is16 = !tinstr.is_thumb32;
 	if (is16) {
 		tinstr.t16.opcode = (instr >> 10) & 0x3f;
 	} else {
@@ -702,22 +728,24 @@ static bool kvm_decode_thumb_ls(struct kvm_vcpu *vcpu, unsigned long instr,
 		tinstr.t32.op2 = (instr >> (16 + 4)) & 0x7f;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(thumb_instr); i++) {
-		const struct thumb_instr *ti = &thumb_instr[i];
-		if (ti->is32 != is32)
+	for (i = 0; i < ARRAY_SIZE(thumb_decode); i++) {
+		const struct thumb_decode *td = &thumb_decode[i];
+		if (td->is32 != tinstr.is_thumb32)
 			continue;
 
 		if (is16) {
-			if ((tinstr.t16.opcode & ti->t16.mask) != ti->t16.opcode)
+			if ((tinstr.t16.opcode & td->t16.mask) != td->t16.opcode)
 				continue;
 		} else {
-			if (ti->t32.op1 != tinstr.t32.op1)
+			if (td->t32.op1 != tinstr.t32.op1)
 				continue;
-			if ((ti->t32.op2_mask & tinstr.t32.op2) != ti->t32.op2)
+			if ((td->t32.op2_mask & tinstr.t32.op2) != td->t32.op2)
 				continue;
 		}
 
-		return ti->decode(vcpu, mmio, instr, &tinstr);
+		if (!td->decode(vcpu, instr, &tinstr))
+			return false;
+		return execute_thumb(vcpu, mmio, &tinstr);
 	}
 
 	return false;
-- 
1.7.10.4

[PATCH 04/10] kvm: completely separate decoding from execution.

[Rusty Russell]

From: Rusty Russell <rusty.russell@linaro.org>

Now we simply call kvm_decode_ls(), and if it succeeds, execute() to
handle the instruction.

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
---
 arch/arm/kvm/emulate.c |  131 ++++++++++++++++++++++++++----------------------
 1 file changed, 70 insertions(+), 61 deletions(-)

diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
index 882db33..6ebf0ff 100644
--- a/arch/arm/kvm/emulate.c
+++ b/arch/arm/kvm/emulate.c
@@ -467,25 +467,6 @@ static bool decode_arm_extra(struct kvm_vcpu *vcpu,
 	return decode_arm_wb(vcpu, instr, ai);
 }
 
-static bool execute(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
-		    const struct arm_insn *ai)
-{
-	/*
-	 * Technically this is allowed in certain circumstances,
-	 * but we don't support it.
-	 */
-	if (ai->Rt == 15 || ai->Rn == 15)
-		return false;
-
-	mmio->is_write = ai->w;
-	mmio->len = ai->len;
-	vcpu->arch.mmio.sign_extend = ai->sign_extend;
-
-	vcpu->arch.mmio.rd = ai->Rt;
-	*vcpu_reg(vcpu, ai->Rn) = ai->offset_addr;
-	return true;
-}
-
 /*
  * The encodings in this table assumes that a fault was generated where the
  * ISV field in the HSR was clear, and the decoding information was invalid,
@@ -563,18 +544,16 @@ static const struct arm_decode arm_decode[] = {
 	  .template = { .len = 2, .w = false, .sign_extend = true, }, },
 };
 
-static bool kvm_decode_arm_ls(struct kvm_vcpu *vcpu, unsigned long instr,
-			      struct kvm_exit_mmio *mmio)
+static bool kvm_decode_arm_ls(struct kvm_vcpu *vcpu,
+			      u32 instr, struct arm_insn *ai)
 {
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(arm_decode); i++) {
 		const struct arm_decode *d = &arm_decode[i];
 		if ((instr & d->opc_mask) == d->opc) {
-			struct arm_insn ai = d->template;
-			if (!d->decode(vcpu, instr, &ai))
-				return false;
-			return execute(vcpu, mmio, &ai);
+			*ai = d->template;
+			return d->decode(vcpu, instr, ai);
 		}
 	}
 	return false;
@@ -673,18 +652,6 @@ static bool decode_thumb_ldr(struct kvm_vcpu *vcpu,
 	return false;
 }
 
-static bool execute_thumb(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
-			  const struct arm_insn *ti)
-{
-	if (kvm_vcpu_reg_is_pc(vcpu, ti->Rn))
-		return false;
-
-	if (!ti->P)
-		*vcpu_reg(vcpu, ti->Rn) = ti->offset_addr;
-	vcpu->arch.mmio.sign_extend = ti->sign_extend;
-	return true;
-}
-
 /*
  * We only support instruction decoding for valid reasonable MMIO operations
  * where trapping them do not provide sufficient information in the HSR (no
@@ -710,47 +677,86 @@ static const struct thumb_decode thumb_decode[] = {
 };
 
 
-static bool kvm_decode_thumb_ls(struct kvm_vcpu *vcpu, unsigned long instr,
-				struct kvm_exit_mmio *mmio)
+static bool kvm_decode_thumb_ls(struct kvm_vcpu *vcpu,
+				u32 instr, struct arm_insn *ti)
 {
-	struct arm_insn tinstr; /* re-use to pass on already decoded info */
 	bool is16;
 	int i;
 
-	tinstr.is_thumb = true;
-	tinstr.is_thumb32 = is_wide_instruction(instr);
+	ti->is_thumb = true;
+	ti->is_thumb32 = is_wide_instruction(instr);
 
-	is16 = !tinstr.is_thumb32;
+	is16 = !ti->is_thumb32;
 	if (is16) {
-		tinstr.t16.opcode = (instr >> 10) & 0x3f;
+		ti->t16.opcode = (instr >> 10) & 0x3f;
 	} else {
-		tinstr.t32.op1 = (instr >> (16 + 11)) & 0x3;
-		tinstr.t32.op2 = (instr >> (16 + 4)) & 0x7f;
+		ti->t32.op1 = (instr >> (16 + 11)) & 0x3;
+		ti->t32.op2 = (instr >> (16 + 4)) & 0x7f;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(thumb_decode); i++) {
 		const struct thumb_decode *td = &thumb_decode[i];
-		if (td->is32 != tinstr.is_thumb32)
+		if (td->is32 != ti->is_thumb32)
 			continue;
 
 		if (is16) {
-			if ((tinstr.t16.opcode & td->t16.mask) != td->t16.opcode)
+			if ((ti->t16.opcode & td->t16.mask) != td->t16.opcode)
 				continue;
 		} else {
-			if (td->t32.op1 != tinstr.t32.op1)
+			if (td->t32.op1 != ti->t32.op1)
 				continue;
-			if ((td->t32.op2_mask & tinstr.t32.op2) != td->t32.op2)
+			if ((td->t32.op2_mask & ti->t32.op2) != td->t32.op2)
 				continue;
 		}
 
-		if (!td->decode(vcpu, instr, &tinstr))
-			return false;
-		return execute_thumb(vcpu, mmio, &tinstr);
+		return td->decode(vcpu, instr, ti);
 	}
 
 	return false;
 }
 
+static int kvm_decode_ls(struct kvm_vcpu *vcpu, u32 instr, u32 psr,
+			 struct arm_insn *ai)
+{
+	bool is_thumb = !!(psr & PSR_T_BIT);
+
+	if (!is_thumb && !kvm_decode_arm_ls(vcpu, instr, ai))
+		return -ENOENT;
+	else if (is_thumb && !kvm_decode_thumb_ls(vcpu, instr, ai))
+		return -ENOENT;
+
+	return 0;
+}
+
+static bool execute(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
+		    const struct arm_insn *ai)
+{
+	if (ai->is_thumb) {
+		if (kvm_vcpu_reg_is_pc(vcpu, ai->Rn))
+			return false;
+
+		if (!ai->P)
+			*vcpu_reg(vcpu, ai->Rn) = ai->offset_addr;
+		vcpu->arch.mmio.sign_extend = ai->sign_extend;
+		return true;
+	}
+
+	/*
+	 * Technically this is allowed in certain circumstances,
+	 * but we don't support it.
+	 */
+	if (ai->Rt == 15 || ai->Rn == 15)
+		return false;
+
+	mmio->is_write = ai->w;
+	mmio->len = ai->len;
+	vcpu->arch.mmio.sign_extend = ai->sign_extend;
+
+	vcpu->arch.mmio.rd = ai->Rt;
+	*vcpu_reg(vcpu, ai->Rn) = ai->offset_addr;
+	return true;
+}
+
 /**
  * kvm_emulate_mmio_ls - emulates load/store instructions made to I/O memory
  * @vcpu:	The vcpu pointer
@@ -770,8 +776,9 @@ static bool kvm_decode_thumb_ls(struct kvm_vcpu *vcpu, unsigned long instr,
 int kvm_emulate_mmio_ls(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 			struct kvm_exit_mmio *mmio)
 {
-	bool is_thumb;
+	bool is_thumb = !!(*vcpu_cpsr(vcpu) & PSR_T_BIT);
 	unsigned long instr = 0;
+	struct arm_insn insn;
 
 	trace_kvm_mmio_emulate(*vcpu_pc(vcpu), instr, *vcpu_cpsr(vcpu));
 
@@ -780,17 +787,19 @@ int kvm_emulate_mmio_ls(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 		return 1;
 
 	mmio->phys_addr = fault_ipa;
-	is_thumb = !!(*vcpu_cpsr(vcpu) & PSR_T_BIT);
-	if (!is_thumb && !kvm_decode_arm_ls(vcpu, instr, mmio)) {
-		kvm_debug("Unable to decode inst: %#08lx (cpsr: %#08x (T=0)"
+
+	if (kvm_decode_ls(vcpu, instr, *vcpu_cpsr(vcpu), &insn) != 0) {
+		kvm_debug("Unable to decode inst: %#08lx (cpsr: %#08x (T=%i)"
 			  "pc: %#08x)\n",
-			  instr, *vcpu_cpsr(vcpu), *vcpu_pc(vcpu));
+			  instr, *vcpu_cpsr(vcpu), is_thumb, *vcpu_pc(vcpu));
 		kvm_inject_dabt(vcpu, vcpu->arch.hxfar);
 		return 1;
-	} else if (is_thumb && !kvm_decode_thumb_ls(vcpu, instr, mmio)) {
-		kvm_debug("Unable to decode inst: %#08lx (cpsr: %#08x (T=1)"
+	}
+
+	if (!execute(vcpu, mmio, &insn)) {
+		kvm_debug("Unable to execute inst: %#08lx (cpsr: %#08x (T=%i)"
 			  "pc: %#08x)\n",
-			  instr, *vcpu_cpsr(vcpu), *vcpu_pc(vcpu));
+			  instr, *vcpu_cpsr(vcpu), is_thumb, *vcpu_pc(vcpu));
 		kvm_inject_dabt(vcpu, vcpu->arch.hxfar);
 		return 1;
 	}
-- 
1.7.10.4

[PATCH 05/10] kvm: move instruction copying inside kvm_decode()

[Rusty Russell]

From: Rusty Russell <rusty.russell@linaro.org>

Now kvm_decode_ls (renamed to kvm_decode) does the copying; we
generalize copy_current_insn() to copy_from_guest(), though it's used
the same way.

This means stopping the kvm threads twice for a wide thumb instruction,
but we've already declared this a v. slow path.

In order for the caller to print out what instruction we couldn't
decode, we add an 'u32 instr' field to struct arm_insn, and always put
the raw instruction in that.

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
---
 arch/arm/kvm/emulate.c |   95 ++++++++++++++++++++++++++----------------------
 1 file changed, 52 insertions(+), 43 deletions(-)

diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
index 6ebf0ff..a6af2a5 100644
--- a/arch/arm/kvm/emulate.c
+++ b/arch/arm/kvm/emulate.c
@@ -231,13 +231,14 @@ static void do_nothing(void *info)
  * Fortunately this is so rare (we don't usually need the instruction), we
  * can go very slowly and noone will mind.
  */
-static bool copy_current_insn(struct kvm_vcpu *vcpu, unsigned long *instr)
+static int copy_from_guest(struct kvm_vcpu *vcpu,
+			   void *dest,
+			   unsigned long gva,
+			   size_t len)
 {
 	int i;
 	bool ret;
 	struct kvm_vcpu *v;
-	bool is_thumb;
-	size_t instr_len;
 
 	/* Don't cross with IPIs in kvm_main.c */
 	spin_lock(&vcpu->kvm->mmu_lock);
@@ -256,33 +257,16 @@ static bool copy_current_insn(struct kvm_vcpu *vcpu, unsigned long *instr)
 			smp_call_function_single(v->cpu, do_nothing, NULL, 1);
 	}
 
-
-	is_thumb = !!(*vcpu_cpsr(vcpu) & PSR_T_BIT);
-	instr_len = (is_thumb) ? 2 : 4;
-
-	BUG_ON(!is_thumb && *vcpu_pc(vcpu) & 0x3);
-
 	/* Now guest isn't running, we can va->pa map and copy atomically. */
-	ret = copy_from_guest_va(vcpu, instr, *vcpu_pc(vcpu), instr_len,
-				 vcpu_mode_priv(vcpu));
-	if (!ret)
-		goto out;
-
-	/* A 32-bit thumb2 instruction can actually go over a page boundary! */
-	if (is_thumb && is_wide_instruction(*instr)) {
-		*instr = *instr << 16;
-		ret = copy_from_guest_va(vcpu, instr, *vcpu_pc(vcpu) + 2, 2,
-					 vcpu_mode_priv(vcpu));
-	}
+	ret = copy_from_guest_va(vcpu, dest, gva, len, vcpu_mode_priv(vcpu));
 
-out:
 	/* Release them all. */
 	kvm_for_each_vcpu(i, v, vcpu->kvm)
 		v->arch.pause = false;
 
 	spin_unlock(&vcpu->kvm->mmu_lock);
 
-	return ret;
+	return ret ? 0 : -EFAULT;
 }
 
 /******************************************************************************
@@ -297,6 +281,9 @@ enum SRType {
 };
 
 struct arm_insn {
+	/* Encoded instruction. */
+	u32 instr;
+
 	/* Decoding for the register write back */
 	bool register_form;
 	u32 imm;
@@ -545,14 +532,16 @@ static const struct arm_decode arm_decode[] = {
 };
 
 static bool kvm_decode_arm_ls(struct kvm_vcpu *vcpu,
-			      u32 instr, struct arm_insn *ai)
+			      unsigned long instr, struct arm_insn *ai)
 {
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(arm_decode); i++) {
 		const struct arm_decode *d = &arm_decode[i];
 		if ((instr & d->opc_mask) == d->opc) {
-			*ai = d->template;
+			ai->len = d->template.len;
+			ai->w = d->template.w;
+			ai->sign_extend = d->template.sign_extend;
 			return d->decode(vcpu, instr, ai);
 		}
 	}
@@ -678,7 +667,7 @@ static const struct thumb_decode thumb_decode[] = {
 
 
 static bool kvm_decode_thumb_ls(struct kvm_vcpu *vcpu,
-				u32 instr, struct arm_insn *ti)
+				unsigned long instr, struct arm_insn *ti)
 {
 	bool is16;
 	int i;
@@ -715,14 +704,38 @@ static bool kvm_decode_thumb_ls(struct kvm_vcpu *vcpu,
 	return false;
 }
 
-static int kvm_decode_ls(struct kvm_vcpu *vcpu, u32 instr, u32 psr,
-			 struct arm_insn *ai)
+static int kvm_decode(struct kvm_vcpu *vcpu, unsigned long pc, u32 psr,
+		      struct arm_insn *ai)
 {
 	bool is_thumb = !!(psr & PSR_T_BIT);
+	unsigned int instr_len = is_thumb ? 2 : 4;
+	int err;
+
+	BUG_ON(!is_thumb && (pc & 0x3));
 
-	if (!is_thumb && !kvm_decode_arm_ls(vcpu, instr, ai))
+	/* Zero out high bits for thumb case, and so it's set on error. */
+	ai->instr = 0;
+
+	/* Now guest isn't running, we can va->pa map and copy atomically. */
+	err = copy_from_guest(vcpu, &ai->instr, pc, instr_len);
+	if (err)
+		return err;
+
+	/*
+	 * Is it a 32 bit thumb instruction?  Can't get it all in 1 go, since
+	 * it can actually go over a page boundary.
+	 */
+	if (is_thumb && is_wide_instruction(ai->instr)) {
+		ai->instr = ai->instr << 16;
+		err = copy_from_guest(vcpu, &ai->instr,
+				      pc + instr_len, instr_len);
+		if (err)
+			return err;
+	}
+
+	if (!is_thumb && !kvm_decode_arm_ls(vcpu, ai->instr, ai))
 		return -ENOENT;
-	else if (is_thumb && !kvm_decode_thumb_ls(vcpu, instr, ai))
+	else if (is_thumb && !kvm_decode_thumb_ls(vcpu, ai->instr, ai))
 		return -ENOENT;
 
 	return 0;
@@ -777,29 +790,25 @@ int kvm_emulate_mmio_ls(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 			struct kvm_exit_mmio *mmio)
 {
 	bool is_thumb = !!(*vcpu_cpsr(vcpu) & PSR_T_BIT);
-	unsigned long instr = 0;
 	struct arm_insn insn;
-
-	trace_kvm_mmio_emulate(*vcpu_pc(vcpu), instr, *vcpu_cpsr(vcpu));
-
-	/* If it fails (SMP race?), we reenter guest for it to retry. */
-	if (!copy_current_insn(vcpu, &instr))
-		return 1;
+	int err;
 
 	mmio->phys_addr = fault_ipa;
 
-	if (kvm_decode_ls(vcpu, instr, *vcpu_cpsr(vcpu), &insn) != 0) {
-		kvm_debug("Unable to decode inst: %#08lx (cpsr: %#08x (T=%i)"
-			  "pc: %#08x)\n",
-			  instr, *vcpu_cpsr(vcpu), is_thumb, *vcpu_pc(vcpu));
+	err = kvm_decode(vcpu, *vcpu_pc(vcpu), *vcpu_cpsr(vcpu), &insn);
+	if (err) {
+		kvm_debug("Unable to decode inst: %#08x (cpsr: %#08x (T=%i)"
+			  "pc: %#08x) - %i\n",
+			  insn.instr, *vcpu_cpsr(vcpu), is_thumb,
+			  *vcpu_pc(vcpu), err);
 		kvm_inject_dabt(vcpu, vcpu->arch.hxfar);
 		return 1;
 	}
 
 	if (!execute(vcpu, mmio, &insn)) {
-		kvm_debug("Unable to execute inst: %#08lx (cpsr: %#08x (T=%i)"
+		kvm_debug("Unable to execute inst: %#08x (cpsr: %#08x (T=%i)"
 			  "pc: %#08x)\n",
-			  instr, *vcpu_cpsr(vcpu), is_thumb, *vcpu_pc(vcpu));
+			  insn.instr, *vcpu_cpsr(vcpu), is_thumb, *vcpu_pc(vcpu));
 		kvm_inject_dabt(vcpu, vcpu->arch.hxfar);
 		return 1;
 	}
@@ -808,7 +817,7 @@ int kvm_emulate_mmio_ls(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	 * The MMIO instruction is emulated and should not be re-executed
 	 * in the guest.
 	 */
-	kvm_skip_instr(vcpu, is_wide_instruction(instr));
+	kvm_skip_instr(vcpu, insn.is_thumb32);
 	return 0;
 }
 
-- 
1.7.10.4

[PATCH 06/10] kvm: cleanup use of instr.

[Rusty Russell]

From: Rusty Russell <rusty.russell@linaro.org>

Now we put instr inside struct arm_insn, we don't need to hand it
internally.

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
---
 arch/arm/kvm/emulate.c |  100 ++++++++++++++++++++++--------------------------
 1 file changed, 45 insertions(+), 55 deletions(-)

diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
index a6af2a5..d619ad8 100644
--- a/arch/arm/kvm/emulate.c
+++ b/arch/arm/kvm/emulate.c
@@ -318,8 +318,7 @@ struct arm_decode {
 	u32 opc;
 	u32 opc_mask;
 
-	bool (*decode)(struct kvm_vcpu *vcpu,
-		       unsigned long instr, struct arm_insn *ai);
+	bool (*decode)(struct kvm_vcpu *vcpu, struct arm_insn *ai);
 
 	struct arm_insn template;
 };
@@ -388,16 +387,15 @@ u32 shift(u32 value, u8 N, enum SRType type, u8 amount, bool carry_in)
 	return value & mask;
 }
 
-static bool decode_arm_wb(struct kvm_vcpu *vcpu, unsigned long instr,
-			  struct arm_insn *ai)
+static bool decode_arm_wb(struct kvm_vcpu *vcpu, struct arm_insn *ai)
 {
 	u32 base_addr, offset;
 
-	ai->Rt = (instr >> 12) & 0xf;
-	ai->Rn = (instr >> 16) & 0xf;
-	ai->W = (instr >> 21) & 1;
-	ai->U = (instr >> 23) & 1;
-	ai->P = (instr >> 24) & 1;
+	ai->Rt = (ai->instr >> 12) & 0xf;
+	ai->Rn = (ai->instr >> 16) & 0xf;
+	ai->W = (ai->instr >> 21) & 1;
+	ai->U = (ai->instr >> 23) & 1;
+	ai->P = (ai->instr >> 24) & 1;
 
 	base_addr = *vcpu_reg(vcpu, ai->Rn);
 
@@ -428,30 +426,28 @@ static bool decode_arm_wb(struct kvm_vcpu *vcpu, unsigned long instr,
 	return true;
 }
 
-static bool decode_arm_ls(struct kvm_vcpu *vcpu,
-			  unsigned long instr, struct arm_insn *ai)
+static bool decode_arm_ls(struct kvm_vcpu *vcpu, struct arm_insn *ai)
 {
-	u8 A = (instr >> 25) & 1;
+	u8 A = (ai->instr >> 25) & 1;
 
 	ai->register_form = A;
-	ai->imm = instr & 0xfff;
-	ai->Rm = instr & 0xf;
-	ai->type = (instr >> 5) & 0x3;
-	ai->shift_n = (instr >> 7) & 0x1f;
+	ai->imm = ai->instr & 0xfff;
+	ai->Rm = ai->instr & 0xf;
+	ai->type = (ai->instr >> 5) & 0x3;
+	ai->shift_n = (ai->instr >> 7) & 0x1f;
 
-	return decode_arm_wb(vcpu, instr, ai);
+	return decode_arm_wb(vcpu, ai);
 }
 
-static bool decode_arm_extra(struct kvm_vcpu *vcpu,
-			     unsigned long instr, struct arm_insn *ai)
+static bool decode_arm_extra(struct kvm_vcpu *vcpu, struct arm_insn *ai)
 {
-	ai->register_form = !((instr >> 22) & 1);
-	ai->imm = ((instr >> 4) & 0xf0) | (instr & 0xf);
-	ai->Rm = instr & 0xf;
+	ai->register_form = !((ai->instr >> 22) & 1);
+	ai->imm = ((ai->instr >> 4) & 0xf0) | (ai->instr & 0xf);
+	ai->Rm = ai->instr & 0xf;
 	ai->type = 0; /* SRType_LSL */
 	ai->shift_n = 0;
 
-	return decode_arm_wb(vcpu, instr, ai);
+	return decode_arm_wb(vcpu, ai);
 }
 
 /*
@@ -531,18 +527,17 @@ static const struct arm_decode arm_decode[] = {
 	  .template = { .len = 2, .w = false, .sign_extend = true, }, },
 };
 
-static bool kvm_decode_arm_ls(struct kvm_vcpu *vcpu,
-			      unsigned long instr, struct arm_insn *ai)
+static bool kvm_decode_arm_ls(struct kvm_vcpu *vcpu, struct arm_insn *ai)
 {
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(arm_decode); i++) {
 		const struct arm_decode *d = &arm_decode[i];
-		if ((instr & d->opc_mask) == d->opc) {
+		if ((ai->instr & d->opc_mask) == d->opc) {
 			ai->len = d->template.len;
 			ai->w = d->template.w;
 			ai->sign_extend = d->template.sign_extend;
-			return d->decode(vcpu, instr, ai);
+			return d->decode(vcpu, ai);
 		}
 	}
 	return false;
@@ -564,20 +559,18 @@ struct thumb_decode {
 		} t32;
 	};
 
-	bool (*decode)(struct kvm_vcpu *vcpu,
-		       unsigned long instr, struct arm_insn *ti);
+	bool (*decode)(struct kvm_vcpu *vcpu, struct arm_insn *ti);
 };
 
-static bool decode_thumb_wb(struct kvm_vcpu *vcpu,
-			    unsigned long instr, struct arm_insn *ti)
+static bool decode_thumb_wb(struct kvm_vcpu *vcpu, struct arm_insn *ti)
 {
-	u8 imm8 = instr & 0xff;
+	u8 imm8 = ti->instr & 0xff;
 	u32 offset_addr = vcpu->arch.hxfar;
 
-	ti->P = (instr >> 10) & 1;
-	ti->U = (instr >> 9) & 1;
-	ti->Rn = (instr >> 16) & 0xf;
-	ti->Rd = (instr >> 12) & 0xf;
+	ti->P = (ti->instr >> 10) & 1;
+	ti->U = (ti->instr >> 9) & 1;
+	ti->Rn = (ti->instr >> 16) & 0xf;
+	ti->Rd = (ti->instr >> 12) & 0xf;
 
 	/* Handle Writeback */
 	if (!ti->P && ti->U)
@@ -587,11 +580,10 @@ static bool decode_thumb_wb(struct kvm_vcpu *vcpu,
 	return true;
 }
 
-static bool decode_thumb_str(struct kvm_vcpu *vcpu,
-			     unsigned long instr, struct arm_insn *ti)
+static bool decode_thumb_str(struct kvm_vcpu *vcpu, struct arm_insn *ti)
 {
-	u8 op1 = (instr >> (16 + 5)) & 0x7;
-	u8 op2 = (instr >> 6) & 0x3f;
+	u8 op1 = (ti->instr >> (16 + 5)) & 0x7;
+	u8 op2 = (ti->instr >> 6) & 0x3f;
 
 	ti->W = true;
 	ti->sign_extend = false;
@@ -606,17 +598,16 @@ static bool decode_thumb_str(struct kvm_vcpu *vcpu,
 
 	if ((op2 & 0x24) == 0x24) {
 		/* STRB (immediate, thumb, W=1) */
-		return decode_thumb_wb(vcpu, instr, ti);
+		return decode_thumb_wb(vcpu, ti);
 	}
 
 	return false;
 }
 
-static bool decode_thumb_ldr(struct kvm_vcpu *vcpu,
-			     unsigned long instr, struct arm_insn *ti)
+static bool decode_thumb_ldr(struct kvm_vcpu *vcpu, struct arm_insn *ti)
 {
-	u8 op1 = (instr >> (16 + 7)) & 0x3;
-	u8 op2 = (instr >> 6) & 0x3f;
+	u8 op1 = (ti->instr >> (16 + 7)) & 0x3;
+	u8 op2 = (ti->instr >> 6) & 0x3f;
 
 	ti->W = false;
 
@@ -635,7 +626,7 @@ static bool decode_thumb_ldr(struct kvm_vcpu *vcpu,
 
 	if ((op2 & 0x24) == 0x24) {
 		/* LDR{S}X (immediate, thumb, W=1) */
-		return decode_thumb_wb(vcpu, instr, ti);
+		return decode_thumb_wb(vcpu, ti);
 	}
 
 	return false;
@@ -666,21 +657,20 @@ static const struct thumb_decode thumb_decode[] = {
 };
 
 
-static bool kvm_decode_thumb_ls(struct kvm_vcpu *vcpu,
-				unsigned long instr, struct arm_insn *ti)
+static bool kvm_decode_thumb_ls(struct kvm_vcpu *vcpu, struct arm_insn *ti)
 {
 	bool is16;
 	int i;
 
 	ti->is_thumb = true;
-	ti->is_thumb32 = is_wide_instruction(instr);
+	ti->is_thumb32 = is_wide_instruction(ti->instr);
 
 	is16 = !ti->is_thumb32;
 	if (is16) {
-		ti->t16.opcode = (instr >> 10) & 0x3f;
+		ti->t16.opcode = (ti->instr >> 10) & 0x3f;
 	} else {
-		ti->t32.op1 = (instr >> (16 + 11)) & 0x3;
-		ti->t32.op2 = (instr >> (16 + 4)) & 0x7f;
+		ti->t32.op1 = (ti->instr >> (16 + 11)) & 0x3;
+		ti->t32.op2 = (ti->instr >> (16 + 4)) & 0x7f;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(thumb_decode); i++) {
@@ -698,7 +688,7 @@ static bool kvm_decode_thumb_ls(struct kvm_vcpu *vcpu,
 				continue;
 		}
 
-		return td->decode(vcpu, instr, ti);
+		return td->decode(vcpu, ti);
 	}
 
 	return false;
@@ -733,9 +723,9 @@ static int kvm_decode(struct kvm_vcpu *vcpu, unsigned long pc, u32 psr,
 			return err;
 	}
 
-	if (!is_thumb && !kvm_decode_arm_ls(vcpu, ai->instr, ai))
+	if (!is_thumb && !kvm_decode_arm_ls(vcpu, ai))
 		return -ENOENT;
-	else if (is_thumb && !kvm_decode_thumb_ls(vcpu, ai->instr, ai))
+	else if (is_thumb && !kvm_decode_thumb_ls(vcpu, ai))
 		return -ENOENT;
 
 	return 0;
-- 
1.7.10.4

[PATCH 07/10] kvm: clean up use of is_wide_instruction()

[Rusty Russell]

From: Rusty Russell <rusty.russell@linaro.org>

We determine whether we're in thumb mode and whether it's a wide instruction
when we copy it in, so just initialize is_thumb and is_thumb32 there.

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
---
 arch/arm/kvm/emulate.c |   44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
index d619ad8..ca24828 100644
--- a/arch/arm/kvm/emulate.c
+++ b/arch/arm/kvm/emulate.c
@@ -662,9 +662,6 @@ static bool kvm_decode_thumb_ls(struct kvm_vcpu *vcpu, struct arm_insn *ti)
 	bool is16;
 	int i;
 
-	ti->is_thumb = true;
-	ti->is_thumb32 = is_wide_instruction(ti->instr);
-
 	is16 = !ti->is_thumb32;
 	if (is16) {
 		ti->t16.opcode = (ti->instr >> 10) & 0x3f;
@@ -697,11 +694,13 @@ static bool kvm_decode_thumb_ls(struct kvm_vcpu *vcpu, struct arm_insn *ti)
 static int kvm_decode(struct kvm_vcpu *vcpu, unsigned long pc, u32 psr,
 		      struct arm_insn *ai)
 {
-	bool is_thumb = !!(psr & PSR_T_BIT);
-	unsigned int instr_len = is_thumb ? 2 : 4;
 	int err;
+	unsigned int instr_len;
+
+	ai->is_thumb = psr & PSR_T_BIT;
+	instr_len = ai->is_thumb ? 2 : 4;
 
-	BUG_ON(!is_thumb && (pc & 0x3));
+	BUG_ON(!ai->is_thumb && (pc & 0x3));
 
 	/* Zero out high bits for thumb case, and so it's set on error. */
 	ai->instr = 0;
@@ -711,24 +710,25 @@ static int kvm_decode(struct kvm_vcpu *vcpu, unsigned long pc, u32 psr,
 	if (err)
 		return err;
 
-	/*
-	 * Is it a 32 bit thumb instruction?  Can't get it all in 1 go, since
-	 * it can actually go over a page boundary.
-	 */
-	if (is_thumb && is_wide_instruction(ai->instr)) {
-		ai->instr = ai->instr << 16;
-		err = copy_from_guest(vcpu, &ai->instr,
-				      pc + instr_len, instr_len);
-		if (err)
-			return err;
+	if (ai->is_thumb) {
+		/*
+		 * Is it a 32 bit thumb instruction?  Can't get it all
+		 * in 1 go, since it can actually go over a page
+		 * boundary.
+		 */
+		ai->is_thumb32 = is_wide_instruction(ai->instr);
+
+		if (ai->is_thumb32) {
+			ai->instr = ai->instr << 16;
+			err = copy_from_guest(vcpu, &ai->instr,
+					      pc + instr_len, instr_len);
+			if (err)
+				return err;
+		}
+		return kvm_decode_thumb_ls(vcpu, ai);
 	}
 
-	if (!is_thumb && !kvm_decode_arm_ls(vcpu, ai))
-		return -ENOENT;
-	else if (is_thumb && !kvm_decode_thumb_ls(vcpu, ai))
-		return -ENOENT;
-
-	return 0;
+	return kvm_decode_arm_ls(vcpu, ai);
 }
 
 static bool execute(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
-- 
1.7.10.4

[PATCH 08/10] kvm: avoid using vcpu_cpsr() by passing down PSR.

[Rusty Russell]

From: Rusty Russell <rusty.russell@linaro.org>

We need the C bit, so hand down the psr through the callchain.

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
---
 arch/arm/kvm/emulate.c |   22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
index ca24828..5ac4cf7 100644
--- a/arch/arm/kvm/emulate.c
+++ b/arch/arm/kvm/emulate.c
@@ -318,7 +318,7 @@ struct arm_decode {
 	u32 opc;
 	u32 opc_mask;
 
-	bool (*decode)(struct kvm_vcpu *vcpu, struct arm_insn *ai);
+	bool (*decode)(struct kvm_vcpu *vcpu, struct arm_insn *ai, u32 psr);
 
 	struct arm_insn template;
 };
@@ -387,7 +387,7 @@ u32 shift(u32 value, u8 N, enum SRType type, u8 amount, bool carry_in)
 	return value & mask;
 }
 
-static bool decode_arm_wb(struct kvm_vcpu *vcpu, struct arm_insn *ai)
+static bool decode_arm_wb(struct kvm_vcpu *vcpu, struct arm_insn *ai, u32 psr)
 {
 	u32 base_addr, offset;
 
@@ -408,7 +408,7 @@ static bool decode_arm_wb(struct kvm_vcpu *vcpu, struct arm_insn *ai)
 		/* Register operation */
 		enum SRType s_type;
 		u8 shift_n;
-		bool c_bit = *vcpu_cpsr(vcpu) & PSR_C_BIT;
+		bool c_bit = psr & PSR_C_BIT;
 		u32 s_reg = *vcpu_reg(vcpu, ai->Rm);
 
 		s_type = decode_imm_shift(ai->type, ai->shift_n, &shift_n);
@@ -426,7 +426,7 @@ static bool decode_arm_wb(struct kvm_vcpu *vcpu, struct arm_insn *ai)
 	return true;
 }
 
-static bool decode_arm_ls(struct kvm_vcpu *vcpu, struct arm_insn *ai)
+static bool decode_arm_ls(struct kvm_vcpu *vcpu, struct arm_insn *ai, u32 psr)
 {
 	u8 A = (ai->instr >> 25) & 1;
 
@@ -436,10 +436,11 @@ static bool decode_arm_ls(struct kvm_vcpu *vcpu, struct arm_insn *ai)
 	ai->type = (ai->instr >> 5) & 0x3;
 	ai->shift_n = (ai->instr >> 7) & 0x1f;
 
-	return decode_arm_wb(vcpu, ai);
+	return decode_arm_wb(vcpu, ai, psr);
 }
 
-static bool decode_arm_extra(struct kvm_vcpu *vcpu, struct arm_insn *ai)
+static bool decode_arm_extra(struct kvm_vcpu *vcpu, struct arm_insn *ai,
+			     u32 psr)
 {
 	ai->register_form = !((ai->instr >> 22) & 1);
 	ai->imm = ((ai->instr >> 4) & 0xf0) | (ai->instr & 0xf);
@@ -447,7 +448,7 @@ static bool decode_arm_extra(struct kvm_vcpu *vcpu, struct arm_insn *ai)
 	ai->type = 0; /* SRType_LSL */
 	ai->shift_n = 0;
 
-	return decode_arm_wb(vcpu, ai);
+	return decode_arm_wb(vcpu, ai, psr);
 }
 
 /*
@@ -527,7 +528,8 @@ static const struct arm_decode arm_decode[] = {
 	  .template = { .len = 2, .w = false, .sign_extend = true, }, },
 };
 
-static bool kvm_decode_arm_ls(struct kvm_vcpu *vcpu, struct arm_insn *ai)
+static bool kvm_decode_arm_ls(struct kvm_vcpu *vcpu, struct arm_insn *ai,
+			      u32 psr)
 {
 	int i;
 
@@ -537,7 +539,7 @@ static bool kvm_decode_arm_ls(struct kvm_vcpu *vcpu, struct arm_insn *ai)
 			ai->len = d->template.len;
 			ai->w = d->template.w;
 			ai->sign_extend = d->template.sign_extend;
-			return d->decode(vcpu, ai);
+			return d->decode(vcpu, ai, psr);
 		}
 	}
 	return false;
@@ -728,7 +730,7 @@ static int kvm_decode(struct kvm_vcpu *vcpu, unsigned long pc, u32 psr,
 		return kvm_decode_thumb_ls(vcpu, ai);
 	}
 
-	return kvm_decode_arm_ls(vcpu, ai);
+	return kvm_decode_arm_ls(vcpu, ai, psr);
 }
 
 static bool execute(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
-- 
1.7.10.4

[PATCH 09/10] kvm: avoid reference vcpu->arch.hxfar by making thumb offset_addr relative

[Rusty Russell]

From: Rusty Russell <rusty.russell@linaro.org>

For generic code we won't know the hdfar, so make the offset_addr relative
in thumb mode.

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
---
 arch/arm/kvm/emulate.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
index 5ac4cf7..c0014e1 100644
--- a/arch/arm/kvm/emulate.c
+++ b/arch/arm/kvm/emulate.c
@@ -567,18 +567,17 @@ struct thumb_decode {
 static bool decode_thumb_wb(struct kvm_vcpu *vcpu, struct arm_insn *ti)
 {
 	u8 imm8 = ti->instr & 0xff;
-	u32 offset_addr = vcpu->arch.hxfar;
 
 	ti->P = (ti->instr >> 10) & 1;
 	ti->U = (ti->instr >> 9) & 1;
 	ti->Rn = (ti->instr >> 16) & 0xf;
 	ti->Rd = (ti->instr >> 12) & 0xf;
 
-	/* Handle Writeback */
+	/* Handle Writeback: offset_addr relative to fault address. */
 	if (!ti->P && ti->U)
-		ti->offset_addr = offset_addr + imm8;
+		ti->offset_addr = imm8;
 	else if (!ti->P && !ti->U)
-		ti->offset_addr = offset_addr - imm8;
+		ti->offset_addr = -imm8;
 	return true;
 }
 
@@ -740,8 +739,10 @@ static bool execute(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
 		if (kvm_vcpu_reg_is_pc(vcpu, ai->Rn))
 			return false;
 
-		if (!ai->P)
-			*vcpu_reg(vcpu, ai->Rn) = ai->offset_addr;
+		if (!ai->P) {
+			*vcpu_reg(vcpu, ai->Rn)
+				= vcpu->arch.hxfar + ai->offset_addr;
+		}
 		vcpu->arch.mmio.sign_extend = ai->sign_extend;
 		return true;
 	}
-- 
1.7.10.4

[PATCH 10/10] opcode: move generic instruction decode out of KVM.

[Rusty Russell]

From: Rusty Russell <rusty.russell@linaro.org>

We add some callbacks and use them instead of using struct kvm_vcpu or
vcpu_reg() directly.  We also remove the kvm_ prefixes, and call the
top-level function decode_insn() instead of kvm_decode().

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
---
 arch/arm/include/asm/opcodes.h |   38 ++++
 arch/arm/kernel/opcodes.c      |  438 +++++++++++++++++++++++++++++++++++++
 arch/arm/kvm/emulate.c         |  473 +---------------------------------------
 3 files changed, 484 insertions(+), 465 deletions(-)

diff --git a/arch/arm/include/asm/opcodes.h b/arch/arm/include/asm/opcodes.h
index 74e211a..7ceb7fb 100644
--- a/arch/arm/include/asm/opcodes.h
+++ b/arch/arm/include/asm/opcodes.h
@@ -11,6 +11,44 @@
 
 #ifndef __ASSEMBLY__
 extern asmlinkage unsigned int arm_check_condition(u32 opcode, u32 psr);
+
+struct arm_insn {
+	/* Encoded instruction. */
+	u32 instr;
+
+	/* Decoding for the register write back */
+	bool register_form;
+	u32 imm;
+	u8 type;
+	u8 Rt, Rn, Rm, Rd;
+	u8 shift_n;
+	u32 offset_addr;
+
+	/* Common decoding */
+	u8 len;
+	bool sign_extend;
+	bool w, W, U, P;
+
+	/* Thumb encoding */
+	bool is_thumb, is_thumb32;
+	union {
+		struct {
+			u8 opcode;
+			u8 mask;
+		} t16;
+
+		struct {
+			u8 op1;
+			u8 op2;
+			u8 op2_mask;
+		} t32;
+	};
+};
+
+int decode_insn(struct arm_insn *ai, unsigned long pc, u32 psr,
+		int (*copy_from)(void *dst, unsigned long addr, size_t, void *),
+		u32 (*get_regval)(int reg, void *),
+		void *priv);
 #endif
 
 #define ARM_OPCODE_CONDTEST_FAIL   0
diff --git a/arch/arm/kernel/opcodes.c b/arch/arm/kernel/opcodes.c
index f8179c6..b6c7940 100644
--- a/arch/arm/kernel/opcodes.c
+++ b/arch/arm/kernel/opcodes.c
@@ -70,3 +70,441 @@ asmlinkage unsigned int arm_check_condition(u32 opcode, u32 psr)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(arm_check_condition);
+
+/******************************************************************************
+ * Load-Store instruction emulation
+ *****************************************************************************/
+enum SRType {
+	SRType_LSL,
+	SRType_LSR,
+	SRType_ASR,
+	SRType_ROR,
+	SRType_RRX
+};
+
+struct arm_decode {
+	/* Instruction decoding */
+	u32 opc;
+	u32 opc_mask;
+
+	bool (*decode)(struct arm_insn *ai, u32 psr,
+		       u32 (*get_regval)(int reg, void *priv), void *priv);
+
+	struct arm_insn template;
+};
+
+/* Modelled after DecodeImmShift() in the ARM ARM */
+enum SRType decode_imm_shift(u8 type, u8 imm5, u8 *amount)
+{
+	switch (type) {
+	case 0x0:
+		*amount = imm5;
+		return SRType_LSL;
+	case 0x1:
+		*amount = (imm5 == 0) ? 32 : imm5;
+		return SRType_LSR;
+	case 0x2:
+		*amount = (imm5 == 0) ? 32 : imm5;
+		return SRType_ASR; 
+	case 0x3:
+		if (imm5 == 0) {
+			*amount = 1;
+			return SRType_RRX;
+		} else {
+			*amount = imm5;
+			return SRType_ROR;
+		}
+	}
+
+	return SRType_LSL;
+}
+
+/* Modelled after Shift() in the ARM ARM */
+u32 shift(u32 value, u8 N, enum SRType type, u8 amount, bool carry_in)
+{
+	u32 mask = (1 << N) - 1;
+	s32 svalue = (s32)value;
+
+	BUG_ON(N > 32);
+	BUG_ON(type == SRType_RRX && amount != 1);
+	BUG_ON(amount > N);
+
+	if (amount == 0)
+		return value;
+
+	switch (type) {
+	case SRType_LSL:
+		value <<= amount;
+		break;
+	case SRType_LSR:
+		 value >>= amount;
+		break;
+	case SRType_ASR:
+		if (value & (1 << (N - 1)))
+			svalue |= ((-1UL) << N);
+		value = svalue >> amount;
+		break;
+	case SRType_ROR:
+		value = (value >> amount) | (value << (N - amount));
+		break;
+	case SRType_RRX: {
+		u32 C = (carry_in) ? 1 : 0;
+		value = (value >> 1) | (C << (N - 1));
+		break;
+	}
+	}
+
+	return value & mask;
+}
+
+static bool decode_arm_wb(struct arm_insn *ai, u32 psr,
+			  u32 (*get_regval)(int reg, void *priv), void *priv)
+{
+	u32 base_addr, offset;
+
+	ai->Rt = (ai->instr >> 12) & 0xf;
+	ai->Rn = (ai->instr >> 16) & 0xf;
+	ai->W = (ai->instr >> 21) & 1;
+	ai->U = (ai->instr >> 23) & 1;
+	ai->P = (ai->instr >> 24) & 1;
+
+	base_addr = get_regval(ai->Rn, priv);
+
+	if (ai->P && !ai->W) {
+		pr_err("Decoding operation with valid ISV?\n");
+		return false;
+	}
+
+	if (ai->register_form) {
+		/* Register operation */
+		enum SRType s_type;
+		u8 shift_n;
+		bool c_bit = psr & PSR_C_BIT;
+		u32 s_reg = get_regval(ai->Rm, priv);
+
+		s_type = decode_imm_shift(ai->type, ai->shift_n, &shift_n);
+		offset = shift(s_reg, 5, s_type, shift_n, c_bit);
+	} else {
+		/* Immediate operation */
+		offset = ai->imm;
+	}
+
+	/* Handle Writeback */
+	if (ai->U)
+		ai->offset_addr = base_addr + offset;
+	else
+		ai->offset_addr = base_addr - offset;
+	return true;
+}
+
+static bool decode_arm_ls(struct arm_insn *ai, u32 psr,
+			  u32 (*get_regval)(int reg, void *priv), void *priv)
+{
+	u8 A = (ai->instr >> 25) & 1;
+
+	ai->register_form = A;
+	ai->imm = ai->instr & 0xfff;
+	ai->Rm = ai->instr & 0xf;
+	ai->type = (ai->instr >> 5) & 0x3;
+	ai->shift_n = (ai->instr >> 7) & 0x1f;
+
+	return decode_arm_wb(ai, psr, get_regval, priv);
+}
+
+static bool decode_arm_extra(struct arm_insn *ai, u32 psr,
+			     u32 (*get_regval)(int reg, void *priv), void *priv)
+{
+	ai->register_form = !((ai->instr >> 22) & 1);
+	ai->imm = ((ai->instr >> 4) & 0xf0) | (ai->instr & 0xf);
+	ai->Rm = ai->instr & 0xf;
+	ai->type = 0; /* SRType_LSL */
+	ai->shift_n = 0;
+
+	return decode_arm_wb(ai, psr, get_regval, priv);
+}
+
+/*
+ * The encodings in this table assumes that a fault was generated where the
+ * ISV field in the HSR was clear, and the decoding information was invalid,
+ * which means that a register write-back occurred, the PC was used as the
+ * destination or a load/store multiple operation was used. Since the latter
+ * two cases are crazy for MMIO on the guest side, we simply inject a fault
+ * when this happens and support the common case.
+ *
+ * We treat unpriviledged loads and stores of words and bytes like all other
+ * loads and stores as their encodings mandate the W bit set and the P bit
+ * clear.
+ */
+static const struct arm_decode arm_decode[] = {
+	/**************** Load/Store Word and Byte **********************/
+	/* Store word with writeback */
+	{ .opc = 0x04000000, .opc_mask = 0x0c500000,
+	  .decode = decode_arm_ls,
+	  .template =  { .len = 4, .w = true, .sign_extend = false, }, },
+	/* Store byte with writeback */
+	{ .opc = 0x04400000, .opc_mask = 0x0c500000,
+	  .decode = decode_arm_ls,
+	  .template = {  .len = 1, .w = true, .sign_extend = false, }, },
+	/* Load word with writeback */
+	{ .opc = 0x04100000, .opc_mask = 0x0c500000,
+	  .decode = decode_arm_ls,
+	  .template = {  .len = 4, .w = false, .sign_extend = false, }, },
+	/* Load byte with writeback */
+	{ .opc = 0x04500000, .opc_mask = 0x0c500000,
+	  .decode = decode_arm_ls,
+	  .template = { .len = 1, .w = false, .sign_extend = false, }, },
+
+	/*************** Extra load/store instructions ******************/
+
+	/* Store halfword with writeback */
+	{ .opc = 0x000000b0, .opc_mask = 0x0c1000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 2, .w = true, .sign_extend = false, }, },
+	/* Load halfword with writeback */
+	{ .opc = 0x001000b0, .opc_mask = 0x0c1000f0, 
+	  .decode = decode_arm_extra,
+	  .template = { .len = 2, .w = false, .sign_extend = false, }, },
+	/* Load dual with writeback */
+	{ .opc = 0x000000d0, .opc_mask = 0x0c1000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 8, .w = false, .sign_extend = false, }, },
+	/* Load signed byte with writeback */
+	{ .opc = 0x001000d0, .opc_mask = 0x0c1000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 1, .w = false, .sign_extend = true, }, },
+
+	/* Store dual with writeback */
+	{ .opc = 0x000000f0, .opc_mask = 0x0c1000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 8, .w = true, .sign_extend = false, }, },
+	/* Load signed halfword with writeback */
+	{ .opc = 0x001000f0, .opc_mask = 0x0c1000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 2, .w = false, .sign_extend = true, }, },
+
+	/* Store halfword unprivileged */
+	{ .opc = 0x002000b0, .opc_mask = 0x0f3000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 2, .w = true, .sign_extend = false, }, },
+	/* Load halfword unprivileged */
+	{ .opc = 0x003000b0, .opc_mask = 0x0f3000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 2, .w = false, .sign_extend = false, }, },
+	/* Load signed byte unprivileged */
+	{ .opc = 0x003000d0, .opc_mask = 0x0f3000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 1, .w = false, .sign_extend = true, }, },
+	/* Load signed halfword unprivileged */
+	{ .opc = 0x003000d0, .opc_mask = 0x0f3000f0,
+	  .decode = decode_arm_extra,
+	  .template = { .len = 2, .w = false, .sign_extend = true, }, },
+};
+
+static bool decode_arm(struct arm_insn *ai, u32 psr,
+		       u32 (*get_regval)(int reg, void *), void *priv)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(arm_decode); i++) {
+		const struct arm_decode *d = &arm_decode[i];
+		if ((ai->instr & d->opc_mask) == d->opc) {
+			ai->len = d->template.len;
+			ai->w = d->template.w;
+			ai->sign_extend = d->template.sign_extend;
+			return d->decode(ai, psr, get_regval, priv);
+		}
+	}
+	return false;
+}
+
+struct thumb_decode {
+	bool is32;
+
+	union {
+		struct {
+			u8 opcode;
+			u8 mask;
+		} t16;
+
+		struct {
+			u8 op1;
+			u8 op2;
+			u8 op2_mask;
+		} t32;
+	};
+
+	bool (*decode)(struct arm_insn *ti);
+};
+
+static bool decode_thumb_wb(struct arm_insn *ti)
+{
+	u8 imm8 = ti->instr & 0xff;
+
+	ti->P = (ti->instr >> 10) & 1;
+	ti->U = (ti->instr >> 9) & 1;
+	ti->Rn = (ti->instr >> 16) & 0xf;
+	ti->Rd = (ti->instr >> 12) & 0xf;
+
+	/* Handle Writeback: offset_addr relative to fault address. */
+	if (!ti->P && ti->U)
+		ti->offset_addr = imm8;
+	else if (!ti->P && !ti->U)
+		ti->offset_addr = -imm8;
+	return true;
+}
+
+static bool decode_thumb_str(struct arm_insn *ti)
+{
+	u8 op1 = (ti->instr >> (16 + 5)) & 0x7;
+	u8 op2 = (ti->instr >> 6) & 0x3f;
+
+	ti->W = true;
+	ti->sign_extend = false;
+
+	switch (op1) {
+	case 0x0: ti->len = 1; break;
+	case 0x1: ti->len = 2; break;
+	case 0x2: ti->len = 4; break;
+	default:
+		  return false; /* Only register write-back versions! */
+	}
+
+	if ((op2 & 0x24) == 0x24) {
+		/* STRB (immediate, thumb, W=1) */
+		return decode_thumb_wb(ti);
+	}
+
+	return false;
+}
+
+static bool decode_thumb_ldr(struct arm_insn *ti)
+{
+	u8 op1 = (ti->instr >> (16 + 7)) & 0x3;
+	u8 op2 = (ti->instr >> 6) & 0x3f;
+
+	ti->W = false;
+
+	switch (ti->t32.op2 & 0x7) {
+	case 0x1: ti->len = 1; break;
+	case 0x3: ti->len = 2; break;
+	case 0x5: ti->len = 4; break;
+	}
+
+	if (op1 == 0x0)
+		ti->sign_extend = false;
+	else if (op1 == 0x2 && (ti->t32.op2 & 0x7) != 0x5)
+		ti->sign_extend = true;
+	else
+		return false; /* Only register write-back versions! */
+
+	if ((op2 & 0x24) == 0x24) {
+		/* LDR{S}X (immediate, thumb, W=1) */
+		return decode_thumb_wb(ti);
+	}
+
+	return false;
+}
+
+/*
+ * We only support instruction decoding for valid reasonable MMIO operations
+ * where trapping them do not provide sufficient information in the HSR (no
+ * 16-bit Thumb instructions provide register writeback that we care about).
+ *
+ * The following instruciton types are NOT supported for MMIO operations
+ * despite the HSR not containing decode info:
+ *  - any Load/Store multiple
+ *  - any load/store exclusive
+ *  - any load/store dual
+ *  - anything with the PC as the dest register
+ */
+static const struct thumb_decode thumb_decode[] = {
+	/**************** 32-bit Thumb instructions **********************/
+	/* Store single data item:	Op1 == 11, Op2 == 000xxx0 */
+	{ .is32 = true,  .t32 = { 3, 0x00, 0x71}, decode_thumb_str	},
+	/* Load byte:			Op1 == 11, Op2 == 00xx001 */
+	{ .is32 = true,  .t32 = { 3, 0x01, 0x67}, decode_thumb_ldr	},
+	/* Load halfword:		Op1 == 11, Op2 == 00xx011 */
+	{ .is32 = true,  .t32 = { 3, 0x03, 0x67}, decode_thumb_ldr	},
+	/* Load word:			Op1 == 11, Op2 == 00xx101 */
+	{ .is32 = true,  .t32 = { 3, 0x05, 0x67}, decode_thumb_ldr	},
+};
+
+
+static bool decode_thumb(struct arm_insn *ti)
+{
+	bool is16;
+	int i;
+
+	is16 = !ti->is_thumb32;
+	if (is16) {
+		ti->t16.opcode = (ti->instr >> 10) & 0x3f;
+	} else {
+		ti->t32.op1 = (ti->instr >> (16 + 11)) & 0x3;
+		ti->t32.op2 = (ti->instr >> (16 + 4)) & 0x7f;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(thumb_decode); i++) {
+		const struct thumb_decode *td = &thumb_decode[i];
+		if (td->is32 != ti->is_thumb32)
+			continue;
+
+		if (is16) {
+			if ((ti->t16.opcode & td->t16.mask) != td->t16.opcode)
+				continue;
+		} else {
+			if (td->t32.op1 != ti->t32.op1)
+				continue;
+			if ((td->t32.op2_mask & ti->t32.op2) != td->t32.op2)
+				continue;
+		}
+
+		return td->decode(ti);
+	}
+
+	return false;
+}
+
+int decode_insn(struct arm_insn *ai, unsigned long pc, u32 psr,
+		int (*copy_from)(void *dst, unsigned long addr, size_t, void *),
+		u32 (*get_regval)(int reg, void *),
+		void *priv)
+{
+	int err;
+	unsigned int instr_len;
+
+	memset(&ai, 0, sizeof(ai));
+
+	ai->is_thumb = psr & PSR_T_BIT;
+	instr_len = ai->is_thumb ? 2 : 4;
+
+	BUG_ON(!ai->is_thumb && (pc & 0x3));
+
+	/* Zero out high bits for thumb case, and so it's set on error. */
+	ai->instr = 0;
+
+	/* Now guest isn't running, we can va->pa map and copy atomically. */
+	err = copy_from(&ai->instr, pc, instr_len, priv);
+	if (err)
+		return err;
+
+	if (ai->is_thumb) {
+		/*
+		 * Is it a 32 bit thumb instruction?  Can't get it all
+		 * in 1 go, since it can actually go over a page
+		 * boundary.
+		 */
+		ai->is_thumb32 = is_wide_instruction(ai->instr);
+
+		if (ai->is_thumb32) {
+			ai->instr = ai->instr << 16;
+			err = copy_from(&ai->instr, pc + instr_len, instr_len,
+					priv);
+			if (err)
+				return err;
+		}
+		return decode_thumb(ai);
+	}
+
+	return decode_arm(ai, psr, get_regval, priv);
+}
+EXPORT_SYMBOL_GPL(decode_insn);
diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
index c0014e1..eeebd33 100644
--- a/arch/arm/kvm/emulate.c
+++ b/arch/arm/kvm/emulate.c
@@ -20,6 +20,7 @@
 #include <linux/kvm_host.h>
 #include <asm/kvm_arm.h>
 #include <asm/kvm_emulate.h>
+#include <asm/opcodes.h>
 #include <trace/events/kvm.h>
 
 #include "trace.h"
@@ -231,11 +232,10 @@ static void do_nothing(void *info)
  * Fortunately this is so rare (we don't usually need the instruction), we
  * can go very slowly and noone will mind.
  */
-static int copy_from_guest(struct kvm_vcpu *vcpu,
-			   void *dest,
-			   unsigned long gva,
-			   size_t len)
+static int copy_from_guest(void *dest, unsigned long gva, size_t len,
+			   void *_vcpu)
 {
+	struct kvm_vcpu *vcpu = _vcpu;
 	int i;
 	bool ret;
 	struct kvm_vcpu *v;
@@ -269,467 +269,9 @@ static int copy_from_guest(struct kvm_vcpu *vcpu,
 	return ret ? 0 : -EFAULT;
 }
 
-/******************************************************************************
- * Load-Store instruction emulation
- *****************************************************************************/
-enum SRType {
-	SRType_LSL,
-	SRType_LSR,
-	SRType_ASR,
-	SRType_ROR,
-	SRType_RRX
-};
-
-struct arm_insn {
-	/* Encoded instruction. */
-	u32 instr;
-
-	/* Decoding for the register write back */
-	bool register_form;
-	u32 imm;
-	u8 type;
-	u8 Rt, Rn, Rm, Rd;
-	u8 shift_n;
-	u32 offset_addr;
-
-	/* Common decoding */
-	u8 len;
-	bool sign_extend;
-	bool w, W, U, P;
-
-	/* Thumb encoding */
-	bool is_thumb, is_thumb32;
-	union {
-		struct {
-			u8 opcode;
-			u8 mask;
-		} t16;
-
-		struct {
-			u8 op1;
-			u8 op2;
-			u8 op2_mask;
-		} t32;
-	};
-};
-
-struct arm_decode {
-	/* Instruction decoding */
-	u32 opc;
-	u32 opc_mask;
-
-	bool (*decode)(struct kvm_vcpu *vcpu, struct arm_insn *ai, u32 psr);
-
-	struct arm_insn template;
-};
-
-/* Modelled after DecodeImmShift() in the ARM ARM */
-enum SRType decode_imm_shift(u8 type, u8 imm5, u8 *amount)
-{
-	switch (type) {
-	case 0x0:
-		*amount = imm5;
-		return SRType_LSL;
-	case 0x1:
-		*amount = (imm5 == 0) ? 32 : imm5;
-		return SRType_LSR;
-	case 0x2:
-		*amount = (imm5 == 0) ? 32 : imm5;
-		return SRType_ASR; 
-	case 0x3:
-		if (imm5 == 0) {
-			*amount = 1;
-			return SRType_RRX;
-		} else {
-			*amount = imm5;
-			return SRType_ROR;
-		}
-	}
-
-	return SRType_LSL;
-}
-
-/* Modelled after Shift() in the ARM ARM */
-u32 shift(u32 value, u8 N, enum SRType type, u8 amount, bool carry_in)
+static u32 get_guest_reg(int reg, void *_vcpu)
 {
-	u32 mask = (1 << N) - 1;
-	s32 svalue = (s32)value;
-
-	BUG_ON(N > 32);
-	BUG_ON(type == SRType_RRX && amount != 1);
-	BUG_ON(amount > N);
-
-	if (amount == 0)
-		return value;
-
-	switch (type) {
-	case SRType_LSL:
-		value <<= amount;
-		break;
-	case SRType_LSR:
-		 value >>= amount;
-		break;
-	case SRType_ASR:
-		if (value & (1 << (N - 1)))
-			svalue |= ((-1UL) << N);
-		value = svalue >> amount;
-		break;
-	case SRType_ROR:
-		value = (value >> amount) | (value << (N - amount));
-		break;
-	case SRType_RRX: {
-		u32 C = (carry_in) ? 1 : 0;
-		value = (value >> 1) | (C << (N - 1));
-		break;
-	}
-	}
-
-	return value & mask;
-}
-
-static bool decode_arm_wb(struct kvm_vcpu *vcpu, struct arm_insn *ai, u32 psr)
-{
-	u32 base_addr, offset;
-
-	ai->Rt = (ai->instr >> 12) & 0xf;
-	ai->Rn = (ai->instr >> 16) & 0xf;
-	ai->W = (ai->instr >> 21) & 1;
-	ai->U = (ai->instr >> 23) & 1;
-	ai->P = (ai->instr >> 24) & 1;
-
-	base_addr = *vcpu_reg(vcpu, ai->Rn);
-
-	if (ai->P && !ai->W) {
-		kvm_err("Decoding operation with valid ISV?\n");
-		return false;
-	}
-
-	if (ai->register_form) {
-		/* Register operation */
-		enum SRType s_type;
-		u8 shift_n;
-		bool c_bit = psr & PSR_C_BIT;
-		u32 s_reg = *vcpu_reg(vcpu, ai->Rm);
-
-		s_type = decode_imm_shift(ai->type, ai->shift_n, &shift_n);
-		offset = shift(s_reg, 5, s_type, shift_n, c_bit);
-	} else {
-		/* Immediate operation */
-		offset = ai->imm;
-	}
-
-	/* Handle Writeback */
-	if (ai->U)
-		ai->offset_addr = base_addr + offset;
-	else
-		ai->offset_addr = base_addr - offset;
-	return true;
-}
-
-static bool decode_arm_ls(struct kvm_vcpu *vcpu, struct arm_insn *ai, u32 psr)
-{
-	u8 A = (ai->instr >> 25) & 1;
-
-	ai->register_form = A;
-	ai->imm = ai->instr & 0xfff;
-	ai->Rm = ai->instr & 0xf;
-	ai->type = (ai->instr >> 5) & 0x3;
-	ai->shift_n = (ai->instr >> 7) & 0x1f;
-
-	return decode_arm_wb(vcpu, ai, psr);
-}
-
-static bool decode_arm_extra(struct kvm_vcpu *vcpu, struct arm_insn *ai,
-			     u32 psr)
-{
-	ai->register_form = !((ai->instr >> 22) & 1);
-	ai->imm = ((ai->instr >> 4) & 0xf0) | (ai->instr & 0xf);
-	ai->Rm = ai->instr & 0xf;
-	ai->type = 0; /* SRType_LSL */
-	ai->shift_n = 0;
-
-	return decode_arm_wb(vcpu, ai, psr);
-}
-
-/*
- * The encodings in this table assumes that a fault was generated where the
- * ISV field in the HSR was clear, and the decoding information was invalid,
- * which means that a register write-back occurred, the PC was used as the
- * destination or a load/store multiple operation was used. Since the latter
- * two cases are crazy for MMIO on the guest side, we simply inject a fault
- * when this happens and support the common case.
- *
- * We treat unpriviledged loads and stores of words and bytes like all other
- * loads and stores as their encodings mandate the W bit set and the P bit
- * clear.
- */
-static const struct arm_decode arm_decode[] = {
-	/**************** Load/Store Word and Byte **********************/
-	/* Store word with writeback */
-	{ .opc = 0x04000000, .opc_mask = 0x0c500000,
-	  .decode = decode_arm_ls,
-	  .template =  { .len = 4, .w = true, .sign_extend = false, }, },
-	/* Store byte with writeback */
-	{ .opc = 0x04400000, .opc_mask = 0x0c500000,
-	  .decode = decode_arm_ls,
-	  .template = {  .len = 1, .w = true, .sign_extend = false, }, },
-	/* Load word with writeback */
-	{ .opc = 0x04100000, .opc_mask = 0x0c500000,
-	  .decode = decode_arm_ls,
-	  .template = {  .len = 4, .w = false, .sign_extend = false, }, },
-	/* Load byte with writeback */
-	{ .opc = 0x04500000, .opc_mask = 0x0c500000,
-	  .decode = decode_arm_ls,
-	  .template = { .len = 1, .w = false, .sign_extend = false, }, },
-
-	/*************** Extra load/store instructions ******************/
-
-	/* Store halfword with writeback */
-	{ .opc = 0x000000b0, .opc_mask = 0x0c1000f0,
-	  .decode = decode_arm_extra,
-	  .template = { .len = 2, .w = true, .sign_extend = false, }, },
-	/* Load halfword with writeback */
-	{ .opc = 0x001000b0, .opc_mask = 0x0c1000f0, 
-	  .decode = decode_arm_extra,
-	  .template = { .len = 2, .w = false, .sign_extend = false, }, },
-	/* Load dual with writeback */
-	{ .opc = 0x000000d0, .opc_mask = 0x0c1000f0,
-	  .decode = decode_arm_extra,
-	  .template = { .len = 8, .w = false, .sign_extend = false, }, },
-	/* Load signed byte with writeback */
-	{ .opc = 0x001000d0, .opc_mask = 0x0c1000f0,
-	  .decode = decode_arm_extra,
-	  .template = { .len = 1, .w = false, .sign_extend = true, }, },
-
-	/* Store dual with writeback */
-	{ .opc = 0x000000f0, .opc_mask = 0x0c1000f0,
-	  .decode = decode_arm_extra,
-	  .template = { .len = 8, .w = true, .sign_extend = false, }, },
-	/* Load signed halfword with writeback */
-	{ .opc = 0x001000f0, .opc_mask = 0x0c1000f0,
-	  .decode = decode_arm_extra,
-	  .template = { .len = 2, .w = false, .sign_extend = true, }, },
-
-	/* Store halfword unprivileged */
-	{ .opc = 0x002000b0, .opc_mask = 0x0f3000f0,
-	  .decode = decode_arm_extra,
-	  .template = { .len = 2, .w = true, .sign_extend = false, }, },
-	/* Load halfword unprivileged */
-	{ .opc = 0x003000b0, .opc_mask = 0x0f3000f0,
-	  .decode = decode_arm_extra,
-	  .template = { .len = 2, .w = false, .sign_extend = false, }, },
-	/* Load signed byte unprivileged */
-	{ .opc = 0x003000d0, .opc_mask = 0x0f3000f0,
-	  .decode = decode_arm_extra,
-	  .template = { .len = 1, .w = false, .sign_extend = true, }, },
-	/* Load signed halfword unprivileged */
-	{ .opc = 0x003000d0, .opc_mask = 0x0f3000f0,
-	  .decode = decode_arm_extra,
-	  .template = { .len = 2, .w = false, .sign_extend = true, }, },
-};
-
-static bool kvm_decode_arm_ls(struct kvm_vcpu *vcpu, struct arm_insn *ai,
-			      u32 psr)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(arm_decode); i++) {
-		const struct arm_decode *d = &arm_decode[i];
-		if ((ai->instr & d->opc_mask) == d->opc) {
-			ai->len = d->template.len;
-			ai->w = d->template.w;
-			ai->sign_extend = d->template.sign_extend;
-			return d->decode(vcpu, ai, psr);
-		}
-	}
-	return false;
-}
-
-struct thumb_decode {
-	bool is32;
-
-	union {
-		struct {
-			u8 opcode;
-			u8 mask;
-		} t16;
-
-		struct {
-			u8 op1;
-			u8 op2;
-			u8 op2_mask;
-		} t32;
-	};
-
-	bool (*decode)(struct kvm_vcpu *vcpu, struct arm_insn *ti);
-};
-
-static bool decode_thumb_wb(struct kvm_vcpu *vcpu, struct arm_insn *ti)
-{
-	u8 imm8 = ti->instr & 0xff;
-
-	ti->P = (ti->instr >> 10) & 1;
-	ti->U = (ti->instr >> 9) & 1;
-	ti->Rn = (ti->instr >> 16) & 0xf;
-	ti->Rd = (ti->instr >> 12) & 0xf;
-
-	/* Handle Writeback: offset_addr relative to fault address. */
-	if (!ti->P && ti->U)
-		ti->offset_addr = imm8;
-	else if (!ti->P && !ti->U)
-		ti->offset_addr = -imm8;
-	return true;
-}
-
-static bool decode_thumb_str(struct kvm_vcpu *vcpu, struct arm_insn *ti)
-{
-	u8 op1 = (ti->instr >> (16 + 5)) & 0x7;
-	u8 op2 = (ti->instr >> 6) & 0x3f;
-
-	ti->W = true;
-	ti->sign_extend = false;
-
-	switch (op1) {
-	case 0x0: ti->len = 1; break;
-	case 0x1: ti->len = 2; break;
-	case 0x2: ti->len = 4; break;
-	default:
-		  return false; /* Only register write-back versions! */
-	}
-
-	if ((op2 & 0x24) == 0x24) {
-		/* STRB (immediate, thumb, W=1) */
-		return decode_thumb_wb(vcpu, ti);
-	}
-
-	return false;
-}
-
-static bool decode_thumb_ldr(struct kvm_vcpu *vcpu, struct arm_insn *ti)
-{
-	u8 op1 = (ti->instr >> (16 + 7)) & 0x3;
-	u8 op2 = (ti->instr >> 6) & 0x3f;
-
-	ti->W = false;
-
-	switch (ti->t32.op2 & 0x7) {
-	case 0x1: ti->len = 1; break;
-	case 0x3: ti->len = 2; break;
-	case 0x5: ti->len = 4; break;
-	}
-
-	if (op1 == 0x0)
-		ti->sign_extend = false;
-	else if (op1 == 0x2 && (ti->t32.op2 & 0x7) != 0x5)
-		ti->sign_extend = true;
-	else
-		return false; /* Only register write-back versions! */
-
-	if ((op2 & 0x24) == 0x24) {
-		/* LDR{S}X (immediate, thumb, W=1) */
-		return decode_thumb_wb(vcpu, ti);
-	}
-
-	return false;
-}
-
-/*
- * We only support instruction decoding for valid reasonable MMIO operations
- * where trapping them do not provide sufficient information in the HSR (no
- * 16-bit Thumb instructions provide register writeback that we care about).
- *
- * The following instruciton types are NOT supported for MMIO operations
- * despite the HSR not containing decode info:
- *  - any Load/Store multiple
- *  - any load/store exclusive
- *  - any load/store dual
- *  - anything with the PC as the dest register
- */
-static const struct thumb_decode thumb_decode[] = {
-	/**************** 32-bit Thumb instructions **********************/
-	/* Store single data item:	Op1 == 11, Op2 == 000xxx0 */
-	{ .is32 = true,  .t32 = { 3, 0x00, 0x71}, decode_thumb_str	},
-	/* Load byte:			Op1 == 11, Op2 == 00xx001 */
-	{ .is32 = true,  .t32 = { 3, 0x01, 0x67}, decode_thumb_ldr	},
-	/* Load halfword:		Op1 == 11, Op2 == 00xx011 */
-	{ .is32 = true,  .t32 = { 3, 0x03, 0x67}, decode_thumb_ldr	},
-	/* Load word:			Op1 == 11, Op2 == 00xx101 */
-	{ .is32 = true,  .t32 = { 3, 0x05, 0x67}, decode_thumb_ldr	},
-};
-
-
-static bool kvm_decode_thumb_ls(struct kvm_vcpu *vcpu, struct arm_insn *ti)
-{
-	bool is16;
-	int i;
-
-	is16 = !ti->is_thumb32;
-	if (is16) {
-		ti->t16.opcode = (ti->instr >> 10) & 0x3f;
-	} else {
-		ti->t32.op1 = (ti->instr >> (16 + 11)) & 0x3;
-		ti->t32.op2 = (ti->instr >> (16 + 4)) & 0x7f;
-	}
-
-	for (i = 0; i < ARRAY_SIZE(thumb_decode); i++) {
-		const struct thumb_decode *td = &thumb_decode[i];
-		if (td->is32 != ti->is_thumb32)
-			continue;
-
-		if (is16) {
-			if ((ti->t16.opcode & td->t16.mask) != td->t16.opcode)
-				continue;
-		} else {
-			if (td->t32.op1 != ti->t32.op1)
-				continue;
-			if ((td->t32.op2_mask & ti->t32.op2) != td->t32.op2)
-				continue;
-		}
-
-		return td->decode(vcpu, ti);
-	}
-
-	return false;
-}
-
-static int kvm_decode(struct kvm_vcpu *vcpu, unsigned long pc, u32 psr,
-		      struct arm_insn *ai)
-{
-	int err;
-	unsigned int instr_len;
-
-	ai->is_thumb = psr & PSR_T_BIT;
-	instr_len = ai->is_thumb ? 2 : 4;
-
-	BUG_ON(!ai->is_thumb && (pc & 0x3));
-
-	/* Zero out high bits for thumb case, and so it's set on error. */
-	ai->instr = 0;
-
-	/* Now guest isn't running, we can va->pa map and copy atomically. */
-	err = copy_from_guest(vcpu, &ai->instr, pc, instr_len);
-	if (err)
-		return err;
-
-	if (ai->is_thumb) {
-		/*
-		 * Is it a 32 bit thumb instruction?  Can't get it all
-		 * in 1 go, since it can actually go over a page
-		 * boundary.
-		 */
-		ai->is_thumb32 = is_wide_instruction(ai->instr);
-
-		if (ai->is_thumb32) {
-			ai->instr = ai->instr << 16;
-			err = copy_from_guest(vcpu, &ai->instr,
-					      pc + instr_len, instr_len);
-			if (err)
-				return err;
-		}
-		return kvm_decode_thumb_ls(vcpu, ai);
-	}
-
-	return kvm_decode_arm_ls(vcpu, ai, psr);
+	return *vcpu_reg(_vcpu, reg);
 }
 
 static bool execute(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio,
@@ -788,7 +330,8 @@ int kvm_emulate_mmio_ls(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 
 	mmio->phys_addr = fault_ipa;
 
-	err = kvm_decode(vcpu, *vcpu_pc(vcpu), *vcpu_cpsr(vcpu), &insn);
+	err = decode_insn(&insn, *vcpu_pc(vcpu), *vcpu_cpsr(vcpu),
+			  copy_from_guest, get_guest_reg, vcpu);
 	if (err) {
 		kvm_debug("Unable to decode inst: %#08x (cpsr: %#08x (T=%i)"
 			  "pc: %#08x) - %i\n",
-- 
1.7.10.4

Re: RFC: Move kvm's instruction decoding into generic code.

[Dave Martin]

On Wed, Oct 24, 2012 at 09:55:13PM +1030, Rusty Russell wrote:
> Compile-tested only, but shows what it looks like (ugly).
> 
> I just used the existing code and didn't crack open my ARM books to
> determine if there's a better way.  The struct names are pretty vague,
> too, as a result.
> 

Thanks (to you and Christoffer) for having a stab at this.

I'll try to respond to these patches, but I have a recorsive panic
going on here so it may take a bit of time...

Cheers
---Dave