On Mon, Jan 16, 2017 at 05:33:30PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
>
> 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 <shannon.zhao@xxxxxxxxxx>
> ---
> 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 <linux/kvm_host.h>
>
> 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(0b0000), CRm(0b0000), 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, ¶ms))
> + if (!index_to_params(reg->id, ¶ms))
> return -ENOENT;
>
> r = find_reg(¶ms, invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs));
> if (!r)
> return -ENOENT;
>
> - return reg_to_user(uaddr, &r->val, id);
> + if (r->get_user)
> + return (r->get_user)(vcpu, r, reg, uaddr);
> +
> + return reg_to_user(uaddr, &vcpu_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, ¶ms))
> + if (!index_to_params(reg->id, ¶ms))
> return -ENOENT;
> r = find_reg(¶ms, invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs));
> if (!r)
> return -ENOENT;
>
> - err = reg_from_user(&val, 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 (r->set_user)(vcpu, r, reg, uaddr);
>
> - return 0;
> + return reg_from_user(&vcpu_id_sys_reg(vcpu, r->reg), uaddr, reg->id);
> }
>
> static bool is_valid_cache(u32 val)
> @@ -2086,7 +2085,7 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
>
> r = index_to_sys_reg_desc(vcpu, reg->id);
> if (!r)
> - return get_invariant_sys_reg(reg->id, uaddr);
> + return get_invariant_sys_reg(vcpu, reg);
>
> if (r->get_user)
> return (r->get_user)(vcpu, r, reg, uaddr);
> @@ -2107,7 +2106,7 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
>
> r = index_to_sys_reg_desc(vcpu, reg->id);
> if (!r)
> - return set_invariant_sys_reg(reg->id, uaddr);
> + return set_invariant_sys_reg(vcpu, reg);
>
> if (r->set_user)
> return (r->set_user)(vcpu, r, reg, uaddr);
> @@ -2252,7 +2251,6 @@ static int check_sysreg_table(const struct sys_reg_desc *table, unsigned int n)
> void kvm_sys_reg_table_init(void)
> {
> unsigned int i;
> - struct sys_reg_desc clidr;
>
> /* Make sure tables are unique and in order. */
> BUG_ON(check_sysreg_table(sys_reg_descs, ARRAY_SIZE(sys_reg_descs)));
> @@ -2262,10 +2260,6 @@ void kvm_sys_reg_table_init(void)
> BUG_ON(check_sysreg_table(cp15_64_regs, ARRAY_SIZE(cp15_64_regs)));
> BUG_ON(check_sysreg_table(invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs)));
>
> - /* We abuse the reset function to overwrite the table itself. */
> - for (i = 0; i < ARRAY_SIZE(invariant_sys_regs); i++)
> - invariant_sys_regs[i].reset(NULL, &invariant_sys_regs[i]);
> -
> /*
> * CLIDR format is awkward, so clean it up. See ARM B4.1.20:
> *
> @@ -2276,8 +2270,7 @@ void kvm_sys_reg_table_init(void)
> * value of 0b000, the values of Ctype4 to Ctype7 must be
> * ignored.
> */
> - get_clidr_el1(NULL, &clidr); /* Ugly... */
> - cache_levels = clidr.val;
> + cache_levels = read_sysreg(clidr_el1);
Hmm... I think this ugly code was here to ensure cache_levels was
derived from the guest view of clidr_el1. With this series that
may no longer be the case, unless we can and do enforce it. We need
Marc to advise here.
> for (i = 0; i < 7; i++)
> if (((cache_levels >> (i*3)) & 7) == 0)
> break;
> @@ -2310,3 +2303,10 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
> if (vcpu_sys_reg(vcpu, num) == 0x4242424242424242)
> panic("Didn't reset vcpu_sys_reg(%zi)", num);
> }
> +
> +void kvm_reset_id_sys_regs(struct kvm_vcpu *vcpu)
> +{
> + unsigned int i;
> + for (i = 0; i < ARRAY_SIZE(invariant_sys_regs); i++)
> + invariant_sys_regs[i].reset(vcpu, &invariant_sys_regs[i]);
This function should be dropped and the calls to it replaced with
reset_sys_reg_descs(vcpu, invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs))
or whatever the table gets renamed to, now that it's not invariant. Or
all this just goes away if we move the registers into the sys_reg
table...
> +}
> --
> 2.0.4
>
>
Thanks,
drew
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
