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

2017-01-16 Thread Shannon Zhao 
From: Shannon Zhao 

Check if the configuration is fine.

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(&val, 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 7/7] ARM64: KVM: Add user set handler for id_aa64mmfr0_el1

2017-01-28 Thread 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(&val, 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 7/7] ARM64: KVM: Add user set handler for id_aa64mmfr0_el1

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

This commit message really needs some love and attention.

> 
> 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(&val, uaddr, KVM_REG_SIZE(reg->id)) != 0)
> + return -EFAULT;
> +
> + asm volatile("mrs %0, id_aa64mmfr0_el1\n" : "=r" (id_aa64mmfr0));

Doesn't the kernel have an abstraction for this already or a cached
value?

> +
> + 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;
> + }

This really needs some explanation as to what it's checking and what the
logic is.

> +
> + 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
> 
> 

Thanks,
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

2017-03-09 Thread Mark Rutland 
On Thu, Mar 09, 2017 at 04:52:18AM -0800, Christoffer Dall wrote:
> On Mon, Jan 16, 2017 at 05:33:34PM +0800, Shannon Zhao wrote:
> > From: Shannon Zhao 
> > 
> > Check if the configuration is fine.
> 
> This commit message really needs some love and attention.
> 
> > 
> > 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(&val, uaddr, KVM_REG_SIZE(reg->id)) != 0)
> > +   return -EFAULT;
> > +
> > +   asm volatile("mrs %0, id_aa64mmfr0_el1\n" : "=r" (id_aa64mmfr0));
> 
> Doesn't the kernel have an abstraction for this already or a cached
> value?

Certainly we shouldn't be using a raw mrs instruction. We have
read_sysreg() or read_cpuid() for that.

The cpufeature code has a cached, system-wide safe value cached for each
system register. The cpuid_feature_extract_field() helper uses that.

> > +   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))) {

Please use mnemonics. For example, we have ID_AA64MMFR0_TGRAN*_SHIFT
defined in .

We also have extraction helpers, see
cpuid_feature_extract_unsigned_field(), as used in
id_aa64mmfr0_mixed_endian_el0().

Thanks,
Mark.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm