| Message ID | 1553864452-15080-17-git-send-email-Dave.Martin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: SVE guest support | expand |
On Fri, 29 Mar 2019 13:00:41 +0000, Dave Martin <Dave.Martin@arm.com> 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 <Dave.Martin@arm.com> > Reviewed-by: Julien Thierry <julien.thierry@arm.com> > Tested-by: zhang.lei <zhang.lei@jp.fujitsu.com> > > --- > > 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 > @@ -23,6 +23,7 @@ > #include <linux/err.h> > #include <linux/kvm_host.h> > #include <linux/module.h> > +#include <linux/stddef.h> > #include <linux/string.h> > #include <linux/vmalloc.h> > #include <linux/fs.h> > @@ -194,9 +195,28 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs) > return -EINVAL; > } > > +static int kvm_arm_copy_core_reg_indices(u64 __user *uindices) > +{ > + unsigned int i; > + int n = 0; > + const u64 core_reg = KVM_REG_ARM64 | KVM_REG_SIZE_U64 | KVM_REG_ARM_CORE; > + > + for (i = 0; i < sizeof(struct kvm_regs) / sizeof(__u32); i++) { > + if (uindices) { > + if (put_user(core_reg | i, uindices)) > + return -EFAULT; > + uindices++; > + } > + > + n++; > + } > + > + return n; > +} > + > static unsigned long num_core_regs(void) > { > - return sizeof(struct kvm_regs) / sizeof(__u32); > + return kvm_arm_copy_core_reg_indices(NULL); > } > > /** > @@ -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. 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 <marc.zyngier@arm.com> 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 <marc.zyngier@arm.com> --- 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; 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;
On Tue, Apr 02, 2019 at 03:41:56AM +0100, Marc Zyngier wrote: > On Fri, 29 Mar 2019 13:00:41 +0000, > Dave Martin <Dave.Martin@arm.com> 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 <Dave.Martin@arm.com> > > Reviewed-by: Julien Thierry <julien.thierry@arm.com> > > Tested-by: zhang.lei <zhang.lei@jp.fujitsu.com> > > > > --- > > > > 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 <marc.zyngier@arm.com> > 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 <marc.zyngier@arm.com> > --- > 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
On Tue, 02 Apr 2019 09:59:19 +0100, Dave Martin <Dave.Martin@arm.com> wrote: > > On Tue, Apr 02, 2019 at 03:41:56AM +0100, Marc Zyngier wrote: > > On Fri, 29 Mar 2019 13:00:41 +0000, > > Dave Martin <Dave.Martin@arm.com> 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 <Dave.Martin@arm.com> > > > Reviewed-by: Julien Thierry <julien.thierry@arm.com> > > > Tested-by: zhang.lei <zhang.lei@jp.fujitsu.com> > > > > > > --- > > > > > > 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 <marc.zyngier@arm.com> > > 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 <marc.zyngier@arm.com> > > --- > > 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 realised that, but since they all return a negative value on error, I went for a similar error handling. > 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? I don't think there is any urgency on that front. For now, I'd like to make sure things do not regress. Once we're sure we have something stable, we'll be able to make it shiny. Thanks, M.
On Tue, Apr 02, 2019 at 10:32:34AM +0100, Marc Zyngier wrote: > On Tue, 02 Apr 2019 09:59:19 +0100, > Dave Martin <Dave.Martin@arm.com> wrote: > > > > On Tue, Apr 02, 2019 at 03:41:56AM +0100, Marc Zyngier wrote: [...] > > > 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) [...] > > > 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 realised that, but since they all return a negative value on error, > I went for a similar error handling. Sure, this is more potential future maintenance thing -- and anyway not a big deal. I agree that your changes look like they work. > > 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? > > I don't think there is any urgency on that front. For now, I'd like to > make sure things do not regress. Once we're sure we have something > stable, we'll be able to make it shiny. Agreed. I won't worry about this for now, but we can discuss it later. Cheers ---Dave IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
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 @@ -23,6 +23,7 @@ #include <linux/err.h> #include <linux/kvm_host.h> #include <linux/module.h> +#include <linux/stddef.h> #include <linux/string.h> #include <linux/vmalloc.h> #include <linux/fs.h> @@ -194,9 +195,28 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs) return -EINVAL; } +static int kvm_arm_copy_core_reg_indices(u64 __user *uindices) +{ + unsigned int i; + int n = 0; + const u64 core_reg = KVM_REG_ARM64 | KVM_REG_SIZE_U64 | KVM_REG_ARM_CORE; + + for (i = 0; i < sizeof(struct kvm_regs) / sizeof(__u32); i++) { + if (uindices) { + if (put_user(core_reg | i, uindices)) + return -EFAULT; + uindices++; + } + + n++; + } + + return n; +} + static unsigned long num_core_regs(void) { - return sizeof(struct kvm_regs) / sizeof(__u32); + return kvm_arm_copy_core_reg_indices(NULL); } /** @@ -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; ret = kvm_arm_copy_fw_reg_indices(vcpu, uindices); if (ret)