[PATCH RFC 7/7] ARM64: KVM: Add user set handler for id_aa64mmfr0_el1
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
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
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
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