On Tue, Apr 02, 2019 at 03:41:56AM +0100, Marc Zyngier wrote:
> On Fri, 29 Mar 2019 13:00:41 +0000,
> Dave Martin <[email protected]> wrote:
> >
> > In preparation for adding logic to filter out some KVM_REG_ARM_CORE
> > registers from the KVM_GET_REG_LIST output, this patch factors out
> > the core register enumeration into a separate function and rebuilds
> > num_core_regs() on top of it.
> >
> > This may be a little more expensive (depending on how good a job
> > the compiler does of specialising the code), but KVM_GET_REG_LIST
> > is not a hot path.
> >
> > This will make it easier to consolidate ID filtering code in one
> > place.
> >
> > No functional change.
> >
> > Signed-off-by: Dave Martin <[email protected]>
> > Reviewed-by: Julien Thierry <[email protected]>
> > Tested-by: zhang.lei <[email protected]>
> >
> > ---
> >
> > Changes since v5:
> >
> > * New patch.
> >
> > This reimplements part of the separately-posted patch "KVM: arm64:
> > Factor out KVM_GET_REG_LIST core register enumeration", minus aspects
> > that potentially break the ABI.
> >
> > As a result, the opportunity to truly consolidate all the ID reg
> > filtering in one place is deliberately left on the floor, for now.
> > This will be addressed in a separate series later on.
> > ---
> > arch/arm64/kvm/guest.c | 33 +++++++++++++++++++++++++--------
> > 1 file changed, 25 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > index 3e38eb2..a391a61 100644
> > --- a/arch/arm64/kvm/guest.c
> > +++ b/arch/arm64/kvm/guest.c
[...]
> > @@ -276,15 +296,12 @@ unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu)
> > */
> > int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> > {
> > - unsigned int i;
> > - const u64 core_reg = KVM_REG_ARM64 | KVM_REG_SIZE_U64 |
> > KVM_REG_ARM_CORE;
> > int ret;
> >
> > - for (i = 0; i < sizeof(struct kvm_regs) / sizeof(__u32); i++) {
> > - if (put_user(core_reg | i, uindices))
> > - return -EFAULT;
> > - uindices++;
> > - }
> > + ret = kvm_arm_copy_core_reg_indices(uindices);
> > + if (ret)
> > + return ret;
> > + uindices += ret;
>
> Interesting snippet. Given that most implementations have at least one
> register, this can hardly work. Please do test things with QEMU, and
> not only kvmtool which obviously doesn't exercise this path.
My bad: this used to work to do the right thing, but I broke it when
splitting up [1] for v6 to avoid the dependency.
kvm_arm_copy_core_reg_indices() used to take &uindices and update it
directly, returning 0 on success instead of the number of registers.
But this seemed less consistent with the way the other functions are
called.
> For the sake of getting -next back to a vaguely usable state, I've now
> queued the following patch on top.
>
> M.
>
> From 832401c8912680ee56dc5cb6ab101266b3db416a Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <[email protected]>
> Date: Tue, 2 Apr 2019 03:28:39 +0100
> Subject: [PATCH] arm64: KVM: Fix system register enumeration
>
> The introduction of the SVE registers to userspace started with a
> refactoring of the way we expose any register via the ONE_REG
> interface.
>
> Unfortunately, this change doesn't exactly behave as expected
> if the number of registers is non-zero and consider everything
> to be an error. The visible result is that QEMU barfs very early
> when creating vcpus.
>
> Make sure we only exit early in case there is an actual error, rather
> than a positive number of registers...
>
> be25bbb392fa ("KVM: arm64: Factor out core register ID enumeration")
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> arch/arm64/kvm/guest.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 086ab0508d69..4f7b26bbf671 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -604,22 +604,22 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64
> __user *uindices)
> int ret;
>
> ret = copy_core_reg_indices(vcpu, uindices);
> - if (ret)
> + if (ret < 0)
> return ret;
> uindices += ret;
>
> ret = copy_sve_reg_indices(vcpu, uindices);
> - if (ret)
> + if (ret < 0)
> return ret;
> uindices += ret;
^ Ack
> ret = kvm_arm_copy_fw_reg_indices(vcpu, uindices);
> - if (ret)
> + if (ret < 0)
> return ret;
> uindices += kvm_arm_get_fw_num_regs(vcpu);
>
> ret = copy_timer_indices(vcpu, uindices);
> - if (ret)
> + if (ret < 0)
> return ret;
> uindices += NUM_TIMER_REGS;
For these two, the interface is not really the same. These don't
return the number of registers, so return 0 on success. "< 0" here
could be a trap for the future, though the risk looks low.
I can have a go at some rework on top to make this more consistent,
but I'd rather not muddy the water further for the moment.
Any view on that?
Cheers
---Dave
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
