Re: [PATCH RFC 7/7] ARM64: KVM: Add user set handler for id_aa64mmfr0_el1

[Andrew Jones]

On Mon, Jan 16, 2017 at 05:33:34PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> Check if the configuration is fine.

I don't think we need this patch. Instead, userspace should
get_one_reg first when the target id register needs to be
sanity checked. The value it gets will be the host value at
that point, so it can sanity check itself before calling
set_one_reg.

drew

> 
> Signed-off-by: Shannon Zhao 
> ---
>  arch/arm64/kvm/sys_regs.c | 32 +++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index f613e29..9763b79 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1493,6 +1493,35 @@ static bool access_id_reg(struct kvm_vcpu *vcpu,
>   return true;
>  }
>  
> +static int set_id_aa64mmfr0_el1(struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *rd,
> + const struct kvm_one_reg *reg,
> + void __user *uaddr)
> +{
> + u64 val, id_aa64mmfr0;
> +
> + if (copy_from_user(, uaddr, KVM_REG_SIZE(reg->id)) != 0)
> + return -EFAULT;
> +
> + asm volatile("mrs %0, id_aa64mmfr0_el1\n" : "=r" (id_aa64mmfr0));
> +
> + if ((val & GENMASK(3, 0)) > (id_aa64mmfr0 & GENMASK(3, 0)) ||
> + (val & GENMASK(7, 4)) > (id_aa64mmfr0 & GENMASK(7, 4)) ||
> + (val & GENMASK(11, 8)) > (id_aa64mmfr0 & GENMASK(11, 8)) ||
> + (val & GENMASK(15, 12)) > (id_aa64mmfr0 & GENMASK(15, 12)) ||
> + (val & GENMASK(19, 16)) > (id_aa64mmfr0 & GENMASK(19, 16)) ||
> + (val & GENMASK(23, 20)) > (id_aa64mmfr0 & GENMASK(23, 20)) ||
> + (val & GENMASK(27, 24)) < (id_aa64mmfr0 & GENMASK(27, 24)) ||
> + (val & GENMASK(31, 28)) < (id_aa64mmfr0 & GENMASK(31, 28))) {
> + kvm_err("Wrong memory translation granule size/Physical Address 
> range\n");
> + return -EINVAL;
> + }
> +
> + vcpu_id_sys_reg(vcpu, rd->reg) = val & GENMASK(31, 0);
> +
> + return 0;
> +}
> +
>  static struct sys_reg_desc invariant_sys_regs[] = {
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b), Op2(0b000),
> access_id_reg, get_midr_el1, MIDR_EL1 },
> @@ -1549,7 +1578,8 @@ static struct sys_reg_desc invariant_sys_regs[] = {
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0110), Op2(0b001),
> access_id_reg, get_id_aa64isar1_el1, ID_AA64ISAR1_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0111), Op2(0b000),
> -   access_id_reg, get_id_aa64mmfr0_el1, ID_AA64MMFR0_EL1 },
> +   access_id_reg, get_id_aa64mmfr0_el1, ID_AA64MMFR0_EL1,
> +   0, NULL, set_id_aa64mmfr0_el1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0111), Op2(0b001),
> access_id_reg, get_id_aa64mmfr1_el1, ID_AA64MMFR1_EL1 },
>   { Op0(0b11), Op1(0b001), CRn(0b), CRm(0b), Op2(0b001),
> -- 
> 2.0.4
> 
> 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH RFC 6/7] ARM64: KVM: Support heterogeneous system

[Andrew Jones]

On Mon, Jan 16, 2017 at 05:33:33PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> When initializing KVM, check whether physical hardware is a
> heterogeneous system through the MIDR values. If so, force userspace to
> set the KVM_ARM_VCPU_CROSS feature bit. Otherwise, it should fail to
> initialize VCPUs.
> 
> Signed-off-by: Shannon Zhao 
> ---
>  arch/arm/kvm/arm.c   | 26 ++
>  include/uapi/linux/kvm.h |  1 +
>  2 files changed, 27 insertions(+)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index bdceb19..21ec070 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -46,6 +46,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #ifdef REQUIRES_VIRT
>  __asm__(".arch_extension virt");
> @@ -65,6 +66,7 @@ static unsigned int kvm_vmid_bits __read_mostly;
>  static DEFINE_SPINLOCK(kvm_vmid_lock);
>  
>  static bool vgic_present;
> +static bool heterogeneous_system;
>  
>  static DEFINE_PER_CPU(unsigned char, kvm_arm_hardware_enabled);
>  
> @@ -210,6 +212,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long 
> ext)
>   case KVM_CAP_ARM_CROSS_VCPU:
>   r = 1;
>   break;
> + case KVM_CAP_ARM_HETEROGENEOUS:
> + r = heterogeneous_system;
> + break;

What's this for? When/why would usespace check it?

>   case KVM_CAP_COALESCED_MMIO:
>   r = KVM_COALESCED_MMIO_PAGE_OFFSET;
>   break;
> @@ -812,6 +817,12 @@ static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
>   int phys_target = kvm_target_cpu();
>   bool cross_vcpu = kvm_vcpu_has_feature_cross_cpu(init);
>  
> + if (heterogeneous_system && !cross_vcpu) {
> + kvm_err("%s:Host is a heterogeneous system, set 
> KVM_ARM_VCPU_CROSS bit\n",
> + __func__);
> + return -EINVAL;
> + }

Instead of forcing userspace to set a bit, why not just confirm the
target selected will work? E.g. if only generic works on a heterogeneous
system then just 

 if (heterogeneous_system && init->target != GENERIC)
return -EINVAL

should work

> +
>   if (!cross_vcpu && init->target != phys_target)
>   return -EINVAL;
>  
> @@ -1397,6 +1408,11 @@ static void check_kvm_target_cpu(void *ret)
>   *(int *)ret = kvm_target_cpu();
>  }
>  
> +static void get_physical_cpu_midr(void *midr)
> +{
> + *(u32 *)midr = read_cpuid_id();
> +}
> +
>  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr)
>  {
>   struct kvm_vcpu *vcpu;
> @@ -1417,6 +1433,7 @@ int kvm_arch_init(void *opaque)
>  {
>   int err;
>   int ret, cpu;
> + u32 current_midr, midr;
>  
>   if (!is_hyp_mode_available()) {
>   kvm_err("HYP mode not available\n");
> @@ -1431,6 +1448,15 @@ int kvm_arch_init(void *opaque)
>   }
>   }
>  
> + current_midr = read_cpuid_id();
> + for_each_online_cpu(cpu) {
> + smp_call_function_single(cpu, get_physical_cpu_midr, , 1);
> + if (current_midr != midr) {
> + heterogeneous_system = true;
> + break;
> + }
> + }

Is there no core kernel API that provides this?

> +
>   err = init_common_resources();
>   if (err)
>   return err;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 46115a2..cc2b63d 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -872,6 +872,7 @@ struct kvm_ppc_smmu_info {
>  #define KVM_CAP_MSI_DEVID 131
>  #define KVM_CAP_PPC_HTM 132
>  #define KVM_CAP_ARM_CROSS_VCPU 133
> +#define KVM_CAP_ARM_HETEROGENEOUS 134
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> -- 
> 2.0.4
> 
>

drew 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH RFC 5/7] ARM64: KVM: Support cross type vCPU

[Andrew Jones]

On Mon, Jan 16, 2017 at 05:33:32PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> Add a capability to tell userspace that KVM supports cross type vCPU.
> Add a cpu feature for userspace to set when it doesn't use host type
> vCPU and kvm_vcpu_preferred_target return the host MIDR register value
> so that userspace can check whether its requested vCPU type macthes the
> one of physical CPU and if so, KVM will not trap ID registers even
> though userspace doesn't specify -cpu host.
> Guest accesses MIDR through VPIDR_EL2 so we save/restore it no matter
> it's a cross type vCPU.
> 
> Signed-off-by: Shannon Zhao 
> ---
>  arch/arm/kvm/arm.c   | 10 --
>  arch/arm64/include/asm/kvm_emulate.h |  3 +++
>  arch/arm64/include/asm/kvm_host.h|  3 ++-
>  arch/arm64/include/uapi/asm/kvm.h|  1 +
>  arch/arm64/kvm/guest.c   | 17 -
>  arch/arm64/kvm/hyp/sysreg-sr.c   |  2 ++
>  include/uapi/linux/kvm.h |  1 +
>  7 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 1167678..bdceb19 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -207,6 +207,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long 
> ext)
>   case KVM_CAP_ARM_PSCI_0_2:
>   case KVM_CAP_READONLY_MEM:
>   case KVM_CAP_MP_STATE:
> + case KVM_CAP_ARM_CROSS_VCPU:
>   r = 1;
>   break;
>   case KVM_CAP_COALESCED_MMIO:
> @@ -809,8 +810,9 @@ static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
>  {
>   unsigned int i;
>   int phys_target = kvm_target_cpu();
> + bool cross_vcpu = kvm_vcpu_has_feature_cross_cpu(init);
>  
> - if (init->target != phys_target)
> + if (!cross_vcpu && init->target != phys_target)
>   return -EINVAL;

I'm not sure we need the vcpu feature bit. I think qemu should be
allowed to try any target (if using -cpu host it will try the
kvm preferred target). kvm should check that the input target is
a known target and that it is compatible with the phys_target,
otherwise -EINVAL.

>  
>   /*
> @@ -839,7 +841,11 @@ static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
>   set_bit(i, vcpu->arch.features);
>   }
>  
> - vcpu->arch.target = phys_target;
> + if (!cross_vcpu)
> + vcpu->arch.target = phys_target;
> + else
> + /* Use generic ARMv8 target for cross type vcpu. */
> + vcpu->arch.target = KVM_ARM_TARGET_GENERIC_V8;

We want to be able to select a specific target type. So, if
init->target is approved by kvm's checking, then this should
just be 
 vcpu->arch.target = init->target

If, for starters, we only want to support preferred and generic,
then kvm's check is easy
 init->target == phys_target || init->target == KVM_ARM_TARGET_GENERIC_V8

>  
>   /* Now we know what it is, we can reset it. */
>   return kvm_reset_vcpu(vcpu);
> diff --git a/arch/arm64/include/asm/kvm_emulate.h 
> b/arch/arm64/include/asm/kvm_emulate.h
> index f5ea0ba..bca7d3a 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -49,6 +49,9 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
>   vcpu->arch.hcr_el2 |= HCR_E2H;
>   if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
>   vcpu->arch.hcr_el2 &= ~HCR_RW;
> + if (test_bit(KVM_ARM_VCPU_CROSS, vcpu->arch.features))
> + /* TODO: Set HCR_TID2 and trap cache registers */
> + vcpu->arch.hcr_el2 |= HCR_TID3 | HCR_TID1 | HCR_TID0;

We could optimize this a bit. For each set_one_reg of an ID register
we can check if it matches the host. If it doesn't then we flag that
we'll need that register's group trap enabled (but not the other two
groups). Here we'll then only enable traps for the necessary groups.
Maybe there's little chance that we won't always need all three
though...

>  }
>  
>  static inline unsigned long vcpu_get_hcr(struct kvm_vcpu *vcpu)
> diff --git a/arch/arm64/include/asm/kvm_host.h 
> b/arch/arm64/include/asm/kvm_host.h
> index 6034f92..d0073d7 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -41,10 +41,11 @@
>  
>  #define KVM_MAX_VCPUS VGIC_V3_MAX_CPUS
>  
> -#define KVM_VCPU_MAX_FEATURES 4
> +#define KVM_VCPU_MAX_FEATURES 5
>  
>  #define KVM_REQ_VCPU_EXIT8
>  
> +bool kvm_vcpu_has_feature_cross_cpu(const struct kvm_vcpu_init *init);
>  int __attribute_const__ kvm_target_cpu(void);
>  int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
>  int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext);
> diff --git a/arch/arm64/include/uapi/asm/kvm.h 
> b/arch/arm64/include/uapi/asm/kvm.h
> index 3051f86..7ba7117 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -97,6 +97,7 @@ struct kvm_regs {
>  #define KVM_ARM_VCPU_EL1_32BIT  

Re: [PATCH RFC 4/7] ARM64: KVM: emulate accessing ID registers

[Andrew Jones]

On Mon, Jan 16, 2017 at 05:33:31PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> Signed-off-by: Shannon Zhao 
> ---
>  arch/arm64/kvm/sys_regs.c | 83 
> ---
>  1 file changed, 50 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 7c5fa03..f613e29 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1480,71 +1480,84 @@ FUNCTION_INVARIANT(id_aa64mmfr1_el1)
>  FUNCTION_INVARIANT(clidr_el1)
>  FUNCTION_INVARIANT(aidr_el1)
>  
> +static bool access_id_reg(struct kvm_vcpu *vcpu,
> +   struct sys_reg_params *p,
> +   const struct sys_reg_desc *r)
> +{
> + if (p->is_write) {
> + vcpu_id_sys_reg(vcpu, r->reg) = p->regval;

Hmm, most/all id registers are write-ignore, right? If there
are some that are not, then they should get their own handler.

> + } else {
> + p->regval = vcpu_id_sys_reg(vcpu, r->reg);
> + }
> +
> + return true;
> +}
> +
>  static struct sys_reg_desc invariant_sys_regs[] = {
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b), Op2(0b000),
> -   NULL, get_midr_el1, MIDR_EL1 },
> +   access_id_reg, get_midr_el1, MIDR_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b), Op2(0b110),
> -   NULL, get_revidr_el1, REVIDR_EL1 },
> +   access_id_reg, get_revidr_el1, REVIDR_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0001), Op2(0b000),
> -   NULL, get_id_pfr0_el1, ID_PFR0_EL1 },
> +   access_id_reg, get_id_pfr0_el1, ID_PFR0_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0001), Op2(0b001),
> -   NULL, get_id_pfr1_el1, ID_PFR1_EL1 },
> +   access_id_reg, get_id_pfr1_el1, ID_PFR1_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0001), Op2(0b010),
> -   NULL, get_id_dfr0_el1, ID_DFR0_EL1 },
> +   access_id_reg, get_id_dfr0_el1, ID_DFR0_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0001), Op2(0b011),
> -   NULL, get_id_afr0_el1, ID_AFR0_EL1 },
> +   access_id_reg, get_id_afr0_el1, ID_AFR0_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0001), Op2(0b100),
> -   NULL, get_id_mmfr0_el1, ID_MMFR0_EL1 },
> +   access_id_reg, get_id_mmfr0_el1, ID_MMFR0_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0001), Op2(0b101),
> -   NULL, get_id_mmfr1_el1, ID_MMFR1_EL1 },
> +   access_id_reg, get_id_mmfr1_el1, ID_MMFR1_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0001), Op2(0b110),
> -   NULL, get_id_mmfr2_el1, ID_MMFR2_EL1 },
> +   access_id_reg, get_id_mmfr2_el1, ID_MMFR2_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0001), Op2(0b111),
> -   NULL, get_id_mmfr3_el1, ID_MMFR3_EL1 },
> +   access_id_reg, get_id_mmfr3_el1, ID_MMFR3_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0010), Op2(0b000),
> -   NULL, get_id_isar0_el1, ID_ISAR0_EL1 },
> +   access_id_reg, get_id_isar0_el1, ID_ISAR0_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0010), Op2(0b001),
> -   NULL, get_id_isar1_el1, ID_ISAR1_EL1 },
> +   access_id_reg, get_id_isar1_el1, ID_ISAR1_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0010), Op2(0b010),
> -   NULL, get_id_isar2_el1, ID_ISAR2_EL1 },
> +   access_id_reg, get_id_isar2_el1, ID_ISAR2_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0010), Op2(0b011),
> -   NULL, get_id_isar3_el1, ID_ISAR3_EL1 },
> +   access_id_reg, get_id_isar3_el1, ID_ISAR3_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0010), Op2(0b100),
> -   NULL, get_id_isar4_el1, ID_ISAR4_EL1 },
> +   access_id_reg, get_id_isar4_el1, ID_ISAR4_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0010), Op2(0b101),
> -   NULL, get_id_isar5_el1, ID_ISAR5_EL1 },
> +   access_id_reg, get_id_isar5_el1, ID_ISAR5_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0011), Op2(0b000),
> -   NULL, get_mvfr0_el1, MVFR0_EL1 },
> +   access_id_reg, get_mvfr0_el1, MVFR0_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0011), Op2(0b001),
> -   NULL, get_mvfr1_el1, MVFR1_EL1 },
> +   access_id_reg, get_mvfr1_el1, MVFR1_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0011), Op2(0b010),
> -   NULL, get_mvfr2_el1, MVFR2_EL1 },
> +   access_id_reg, get_mvfr2_el1, MVFR2_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0100), Op2(0b000),
> -   NULL, get_id_aa64pfr0_el1, ID_AA64PFR0_EL1 },
> +   access_id_reg, get_id_aa64pfr0_el1, ID_AA64PFR0_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0100), Op2(0b001),
> -   NULL, get_id_aa64pfr1_el1, ID_AA64PFR1_EL1 },
> +   access_id_reg, get_id_aa64pfr1_el1, ID_AA64PFR1_EL1 },
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0101), Op2(0b000),
> -   NULL, 

Re: [PATCH RFC 3/7] ARM64: KVM: Reset ID registers when creating the VCPUs

[Andrew Jones]

On Mon, Jan 16, 2017 at 05:33:30PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> Reset ID registers when creating the VCPUs and store the values per
> VCPU. Also modify the get_invariant_sys_reg and set_invariant_sys_reg
> to get/set the ID register from vcpu context.

The patch does more than that. It also prepares the table to be
used with get/set_one_reg. The name 'invariant' is less and less
fitting, and should probably be changed before this patch
to 'id', or we should just integrate these ID registers into
the sys_reg table. I still haven't seen the motivation for
not doing that yet.

> 
> Signed-off-by: Shannon Zhao 
> ---
>  arch/arm64/include/asm/kvm_coproc.h |  1 +
>  arch/arm64/kvm/guest.c  |  1 +
>  arch/arm64/kvm/sys_regs.c   | 58 
> ++---
>  3 files changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_coproc.h 
> b/arch/arm64/include/asm/kvm_coproc.h
> index 0b52377..0801b66 100644
> --- a/arch/arm64/include/asm/kvm_coproc.h
> +++ b/arch/arm64/include/asm/kvm_coproc.h
> @@ -24,6 +24,7 @@
>  #include 
>  
>  void kvm_reset_sys_regs(struct kvm_vcpu *vcpu);
> +void kvm_reset_id_sys_regs(struct kvm_vcpu *vcpu);
>  
>  struct kvm_sys_reg_table {
>   const struct sys_reg_desc *table;
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index b37446a..92abe2b 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -48,6 +48,7 @@ struct kvm_stats_debugfs_item debugfs_entries[] = {
>  
>  int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
>  {
> + kvm_reset_id_sys_regs(vcpu);

This call should go in kvm_reset_vcpu

>   return 0;
>  }
>  
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index bf71eb4..7c5fa03 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1440,11 +1440,11 @@ static const struct sys_reg_desc cp15_64_regs[] = {
>   * the guest, or a future kvm may trap them.
>   */
>  
> -#define FUNCTION_INVARIANT(reg)  
> \
> - static void get_##reg(struct kvm_vcpu *v,   \
> -   const struct sys_reg_desc *r) \
> +#define FUNCTION_INVARIANT(register) \
> + static void get_##register(struct kvm_vcpu *v,  \
> +const struct sys_reg_desc *r)\
>   {   \
> - ((struct sys_reg_desc *)r)->val = read_sysreg(reg); \
> + vcpu_id_sys_reg(v, r->reg) = read_sysreg(register); \
>   }
>  
>  FUNCTION_INVARIANT(midr_el1)
> @@ -1480,7 +1480,6 @@ FUNCTION_INVARIANT(id_aa64mmfr1_el1)
>  FUNCTION_INVARIANT(clidr_el1)
>  FUNCTION_INVARIANT(aidr_el1)
>  
> -/* ->val is filled in by kvm_sys_reg_table_init() */
>  static struct sys_reg_desc invariant_sys_regs[] = {
>   { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b), Op2(0b000),
> NULL, get_midr_el1, MIDR_EL1 },
> @@ -1952,43 +1951,43 @@ static int reg_to_user(void __user *uaddr, const u64 
> *val, u64 id)
>   return 0;
>  }
>  
> -static int get_invariant_sys_reg(u64 id, void __user *uaddr)
> +static int get_invariant_sys_reg(struct kvm_vcpu *vcpu,
> +  const struct kvm_one_reg *reg)
>  {
>   struct sys_reg_params params;
>   const struct sys_reg_desc *r;
> + void __user *uaddr = (void __user *)(unsigned long)reg->addr;
>  
> - if (!index_to_params(id, ))
> + if (!index_to_params(reg->id, ))
>   return -ENOENT;
>  
>   r = find_reg(, invariant_sys_regs, 
> ARRAY_SIZE(invariant_sys_regs));
>   if (!r)
>   return -ENOENT;
>  
> - return reg_to_user(uaddr, >val, id);
> + if (r->get_user)
> + return (r->get_user)(vcpu, r, reg, uaddr);
> +
> + return reg_to_user(uaddr, _id_sys_reg(vcpu, r->reg), reg->id);
>  }
>  
> -static int set_invariant_sys_reg(u64 id, void __user *uaddr)
> +static int set_invariant_sys_reg(struct kvm_vcpu *vcpu,
> +  const struct kvm_one_reg *reg)
>  {
>   struct sys_reg_params params;
>   const struct sys_reg_desc *r;
> - int err;
> - u64 val = 0; /* Make sure high bits are 0 for 32-bit regs */
> + void __user *uaddr = (void __user *)(unsigned long)reg->addr;
>  
> - if (!index_to_params(id, ))
> + if (!index_to_params(reg->id, ))
>   return -ENOENT;
>   r = find_reg(, invariant_sys_regs, 
> ARRAY_SIZE(invariant_sys_regs));
>   if (!r)
>   return -ENOENT;
>  
> - err = reg_from_user(, uaddr, id);
> - if (err)
> - return err;
> -
> - /* This is what we mean by invariant: you can't change it. */
> - if (r->val != val)
> - return -EINVAL;
> + if (r->set_user)
> + return 

Re: [PATCH RFC 2/7] ARM64: KVM: Add reset handlers for all ID registers

[Andrew Jones]

On Mon, Jan 16, 2017 at 05:33:29PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> Move invariant_sys_regs before emulate_sys_reg so that it can be used
> later.

This patch says it's adding reset handlers, but it's not, because the
handlers were already there. This patch is adding register indices,
which, at this point, appear to still be unused. The patch also adds new
registers to the table without mentioning that in the commit message.
The new registers should be added with a separate patch first, providing
justification in its commit message. The code movement called out in the
commit message is fine, but yet to serve a purpose, so I guess I'll just
have to stay tuned.

drew

> 
> Signed-off-by: Shannon Zhao 
> ---
>  arch/arm64/kvm/sys_regs.c | 193 
> --
>  1 file changed, 116 insertions(+), 77 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 87e7e66..bf71eb4 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1432,6 +1432,122 @@ static const struct sys_reg_desc cp15_64_regs[] = {
>   { Op1( 1), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, c2_TTBR1 },
>  };
>  
> +/*
> + * These are the invariant sys_reg registers: we let the guest see the
> + * host versions of these, so they're part of the guest state.
> + *
> + * A future CPU may provide a mechanism to present different values to
> + * the guest, or a future kvm may trap them.
> + */
> +
> +#define FUNCTION_INVARIANT(reg)  
> \
> + static void get_##reg(struct kvm_vcpu *v,   \
> +   const struct sys_reg_desc *r) \
> + {   \
> + ((struct sys_reg_desc *)r)->val = read_sysreg(reg); \
> + }
> +
> +FUNCTION_INVARIANT(midr_el1)
> +FUNCTION_INVARIANT(ctr_el0)
> +FUNCTION_INVARIANT(revidr_el1)
> +FUNCTION_INVARIANT(id_pfr0_el1)
> +FUNCTION_INVARIANT(id_pfr1_el1)
> +FUNCTION_INVARIANT(id_dfr0_el1)
> +FUNCTION_INVARIANT(id_afr0_el1)
> +FUNCTION_INVARIANT(id_mmfr0_el1)
> +FUNCTION_INVARIANT(id_mmfr1_el1)
> +FUNCTION_INVARIANT(id_mmfr2_el1)
> +FUNCTION_INVARIANT(id_mmfr3_el1)
> +FUNCTION_INVARIANT(id_isar0_el1)
> +FUNCTION_INVARIANT(id_isar1_el1)
> +FUNCTION_INVARIANT(id_isar2_el1)
> +FUNCTION_INVARIANT(id_isar3_el1)
> +FUNCTION_INVARIANT(id_isar4_el1)
> +FUNCTION_INVARIANT(id_isar5_el1)
> +FUNCTION_INVARIANT(mvfr0_el1)
> +FUNCTION_INVARIANT(mvfr1_el1)
> +FUNCTION_INVARIANT(mvfr2_el1)
> +FUNCTION_INVARIANT(id_aa64pfr0_el1)
> +FUNCTION_INVARIANT(id_aa64pfr1_el1)
> +FUNCTION_INVARIANT(id_aa64dfr0_el1)
> +FUNCTION_INVARIANT(id_aa64dfr1_el1)
> +FUNCTION_INVARIANT(id_aa64afr0_el1)
> +FUNCTION_INVARIANT(id_aa64afr1_el1)
> +FUNCTION_INVARIANT(id_aa64isar0_el1)
> +FUNCTION_INVARIANT(id_aa64isar1_el1)
> +FUNCTION_INVARIANT(id_aa64mmfr0_el1)
> +FUNCTION_INVARIANT(id_aa64mmfr1_el1)
> +FUNCTION_INVARIANT(clidr_el1)
> +FUNCTION_INVARIANT(aidr_el1)
> +
> +/* ->val is filled in by kvm_sys_reg_table_init() */
> +static struct sys_reg_desc invariant_sys_regs[] = {
> + { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b), Op2(0b000),
> +   NULL, get_midr_el1, MIDR_EL1 },
> + { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b), Op2(0b110),
> +   NULL, get_revidr_el1, REVIDR_EL1 },
> + { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0001), Op2(0b000),
> +   NULL, get_id_pfr0_el1, ID_PFR0_EL1 },
> + { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0001), Op2(0b001),
> +   NULL, get_id_pfr1_el1, ID_PFR1_EL1 },
> + { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0001), Op2(0b010),
> +   NULL, get_id_dfr0_el1, ID_DFR0_EL1 },
> + { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0001), Op2(0b011),
> +   NULL, get_id_afr0_el1, ID_AFR0_EL1 },
> + { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0001), Op2(0b100),
> +   NULL, get_id_mmfr0_el1, ID_MMFR0_EL1 },
> + { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0001), Op2(0b101),
> +   NULL, get_id_mmfr1_el1, ID_MMFR1_EL1 },
> + { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0001), Op2(0b110),
> +   NULL, get_id_mmfr2_el1, ID_MMFR2_EL1 },
> + { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0001), Op2(0b111),
> +   NULL, get_id_mmfr3_el1, ID_MMFR3_EL1 },
> + { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0010), Op2(0b000),
> +   NULL, get_id_isar0_el1, ID_ISAR0_EL1 },
> + { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0010), Op2(0b001),
> +   NULL, get_id_isar1_el1, ID_ISAR1_EL1 },
> + { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0010), Op2(0b010),
> +   NULL, get_id_isar2_el1, ID_ISAR2_EL1 },
> + { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0010), Op2(0b011),
> +   NULL, get_id_isar3_el1, ID_ISAR3_EL1 },
> + { Op0(0b11), Op1(0b000), CRn(0b), CRm(0b0010), Op2(0b100),
> +   NULL, get_id_isar4_el1, 

Re: [PATCH RFC 1/7] ARM64: KVM: Add the definition of ID registers

[Andrew Jones]

On Mon, Jan 16, 2017 at 05:33:28PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> Add a new memeber in kvm_cpu_context to save the ID registers value.

Currently all the sysregs that need to be save/restored are in a single
array, sys_regs. This commit message needs to provide the rationale for
introducing the id_sys_regs array. If there is no good reason, then the
ID registers should be integrated with the rest. Also, what about the
ARMv7/AArch32 equivalent registers?

Thanks,
drew

> 
> Signed-off-by: Shannon Zhao 
> ---
>  arch/arm64/include/asm/kvm_host.h | 46 
> +++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h 
> b/arch/arm64/include/asm/kvm_host.h
> index e505038..6034f92 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -187,12 +187,57 @@ enum vcpu_sysreg {
>  
>  #define NR_COPRO_REGS(NR_SYS_REGS * 2)
>  
> +enum id_vcpu_sysreg {
> + MIDR_EL1,
> + /* ID group 1 registers */
> + REVIDR_EL1,
> + AIDR_EL1,
> +
> + /* ID group 2 registers */
> + CTR_EL0,
> + CCSIDR_EL1,
> + CLIDR_EL1,
> +
> + /* ID group 3 registers */
> + ID_PFR0_EL1,
> + ID_PFR1_EL1,
> + ID_DFR0_EL1,
> + ID_AFR0_EL1,
> + ID_MMFR0_EL1,
> + ID_MMFR1_EL1,
> + ID_MMFR2_EL1,
> + ID_MMFR3_EL1,
> + ID_ISAR0_EL1,
> + ID_ISAR1_EL1,
> + ID_ISAR2_EL1,
> + ID_ISAR3_EL1,
> + ID_ISAR4_EL1,
> + ID_ISAR5_EL1,
> + MVFR0_EL1,
> + MVFR1_EL1,
> + MVFR2_EL1,
> + ID_AA64PFR0_EL1,
> + ID_AA64PFR1_EL1,
> + ID_AA64DFR0_EL1,
> + ID_AA64DFR1_EL1,
> + ID_AA64ISAR0_EL1,
> + ID_AA64ISAR1_EL1,
> + ID_AA64MMFR0_EL1,
> + ID_AA64MMFR1_EL1,
> + ID_AA64AFR0_EL1,
> + ID_AA64AFR1_EL1,
> + ID_MMFR4_EL1,
> +
> + NR_ID_SYS_REGS
> +};
> +
>  struct kvm_cpu_context {
>   struct kvm_regs gp_regs;
>   union {
>   u64 sys_regs[NR_SYS_REGS];
>   u32 copro[NR_COPRO_REGS];
>   };
> + u64 id_sys_regs[NR_ID_SYS_REGS];
>  };
>  
>  typedef struct kvm_cpu_context kvm_cpu_context_t;
> @@ -277,6 +322,7 @@ struct kvm_vcpu_arch {
>  
>  #define vcpu_gp_regs(v)  (&(v)->arch.ctxt.gp_regs)
>  #define vcpu_sys_reg(v,r)((v)->arch.ctxt.sys_regs[(r)])
> +#define vcpu_id_sys_reg(v,r) ((v)->arch.ctxt.id_sys_regs[(r)])
>  /*
>   * CP14 and CP15 live in the same array, as they are backed by the
>   * same system registers.
> -- 
> 2.0.4
> 
> 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm