| Message ID | 1553017938-710-20-git-send-email-Dave.Martin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: SVE guest support | expand |
Hi Dave, On 19/03/2019 17:52, Dave Martin wrote: > This patch includes the SVE register IDs in the list returned by > KVM_GET_REG_LIST, as appropriate. > > On a non-SVE-enabled vcpu, no new IDs are added. > > On an SVE-enabled vcpu, IDs for the FPSIMD V-registers are removed > from the list, since userspace is required to access the Z- > registers instead in order to access the V-register content. For > the variably-sized SVE registers, the appropriate set of slice IDs > are enumerated, depending on the maximum vector length for the > vcpu. > > As it currently stands, the SVE architecture never requires more > than one slice to exist per register, so this patch adds no > explicit support for enumerating multiple slices. The code can be > extended straightforwardly to support this in the future, if > needed. > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Reviewed-by: Julien Thierry <julien.thierry@arm.com> > --- > > Changes since v5: > > (Dropped Julien Thierry's Reviewed-by due to non-trivial rebasing) > > * Move mis-split reword to prevent put_user()s being accidentally the > correct size from KVM: arm64/sve: Add pseudo-register for the guest's > vector lengths. > --- > arch/arm64/kvm/guest.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 56 insertions(+) > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index 736d8cb..585c31e5 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -411,6 +411,56 @@ static int get_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > return copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)) ? -EFAULT : 0; > } > > +static unsigned long num_sve_regs(const struct kvm_vcpu *vcpu) > +{ > + /* Only the first slice ever exists, for now */ > + const unsigned int slices = 1; Nit: Might be worth introducing a macro/inline function for the number of slices supported. This way, the day we need to change that, we only need to look for that identifier. Cheers,
On Wed, Mar 27, 2019 at 09:47:42AM +0000, Julien Thierry wrote: > Hi Dave, > > On 19/03/2019 17:52, Dave Martin wrote: > > This patch includes the SVE register IDs in the list returned by > > KVM_GET_REG_LIST, as appropriate. > > > > On a non-SVE-enabled vcpu, no new IDs are added. > > > > On an SVE-enabled vcpu, IDs for the FPSIMD V-registers are removed > > from the list, since userspace is required to access the Z- > > registers instead in order to access the V-register content. For > > the variably-sized SVE registers, the appropriate set of slice IDs > > are enumerated, depending on the maximum vector length for the > > vcpu. > > > > As it currently stands, the SVE architecture never requires more > > than one slice to exist per register, so this patch adds no > > explicit support for enumerating multiple slices. The code can be > > extended straightforwardly to support this in the future, if > > needed. > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > > Reviewed-by: Julien Thierry <julien.thierry@arm.com> Thanks, although... > > --- > > > > Changes since v5: > > > > (Dropped Julien Thierry's Reviewed-by due to non-trivial rebasing) > > > > * Move mis-split reword to prevent put_user()s being accidentally the > > correct size from KVM: arm64/sve: Add pseudo-register for the guest's > > vector lengths. > > --- > > arch/arm64/kvm/guest.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 56 insertions(+) > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > > index 736d8cb..585c31e5 100644 > > --- a/arch/arm64/kvm/guest.c > > +++ b/arch/arm64/kvm/guest.c > > @@ -411,6 +411,56 @@ static int get_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > return copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)) ? -EFAULT : 0; > > } > > > > +static unsigned long num_sve_regs(const struct kvm_vcpu *vcpu) > > +{ > > + /* Only the first slice ever exists, for now */ > > + const unsigned int slices = 1; > > Nit: Might be worth introducing a macro/inline function for the number > of slices supported. This way, the day we need to change that, we only > need to look for that identifier. ... Reasonable point, but I wanted to avoid inventing anything prematurely, partly because sve_reg_to_region() will need work in order to support multiple slices (though it's not rocket science). I could introduce something like the following: static unsigned int sve_num_slices(const struct kvm_vcpu *vcpu) { unsigned int slice_size = KVM_REG_SIZE(KVM_REG_ARM64_SVE_ZREG(0, 0)); unsigned int slices = DIV_ROUND_UP(vcpu->arch.sve_max_vl, slice_size); /* * For now, the SVE register ioctl access code won't work * properly with multiple register slices. KVM should prevent * configuration of a vcpu with a maximum vector length large * enough to trigger this: */ if (WARN_ON_ONCE(slices > 1)) return 1; return slices; } This may be clearer, but felt a bit like overkill... Thoughts? Cheers ---Dave
On 27/03/2019 10:33, Dave Martin wrote: > On Wed, Mar 27, 2019 at 09:47:42AM +0000, Julien Thierry wrote: >> Hi Dave, >> >> On 19/03/2019 17:52, Dave Martin wrote: >>> This patch includes the SVE register IDs in the list returned by >>> KVM_GET_REG_LIST, as appropriate. >>> >>> On a non-SVE-enabled vcpu, no new IDs are added. >>> >>> On an SVE-enabled vcpu, IDs for the FPSIMD V-registers are removed >>> from the list, since userspace is required to access the Z- >>> registers instead in order to access the V-register content. For >>> the variably-sized SVE registers, the appropriate set of slice IDs >>> are enumerated, depending on the maximum vector length for the >>> vcpu. >>> >>> As it currently stands, the SVE architecture never requires more >>> than one slice to exist per register, so this patch adds no >>> explicit support for enumerating multiple slices. The code can be >>> extended straightforwardly to support this in the future, if >>> needed. >>> >>> Signed-off-by: Dave Martin <Dave.Martin@arm.com> >>> >> >> Reviewed-by: Julien Thierry <julien.thierry@arm.com> > > Thanks, although... > >>> --- >>> >>> Changes since v5: >>> >>> (Dropped Julien Thierry's Reviewed-by due to non-trivial rebasing) >>> >>> * Move mis-split reword to prevent put_user()s being accidentally the >>> correct size from KVM: arm64/sve: Add pseudo-register for the guest's >>> vector lengths. >>> --- >>> arch/arm64/kvm/guest.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 56 insertions(+) >>> >>> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c >>> index 736d8cb..585c31e5 100644 >>> --- a/arch/arm64/kvm/guest.c >>> +++ b/arch/arm64/kvm/guest.c >>> @@ -411,6 +411,56 @@ static int get_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) >>> return copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)) ? -EFAULT : 0; >>> } >>> >>> +static unsigned long num_sve_regs(const struct kvm_vcpu *vcpu) >>> +{ >>> + /* Only the first slice ever exists, for now */ >>> + const unsigned int slices = 1; >> >> Nit: Might be worth introducing a macro/inline function for the number >> of slices supported. This way, the day we need to change that, we only >> need to look for that identifier. > > ... Reasonable point, but I wanted to avoid inventing anything > prematurely, partly because sve_reg_to_region() will need work in order > to support multiple slices (though it's not rocket science). > > I could introduce something like the following: > > static unsigned int sve_num_slices(const struct kvm_vcpu *vcpu) > { > unsigned int slice_size = KVM_REG_SIZE(KVM_REG_ARM64_SVE_ZREG(0, 0)); > unsigned int slices = DIV_ROUND_UP(vcpu->arch.sve_max_vl, slice_size); > > /* > * For now, the SVE register ioctl access code won't work > * properly with multiple register slices. KVM should prevent > * configuration of a vcpu with a maximum vector length large > * enough to trigger this: > */ > if (WARN_ON_ONCE(slices > 1)) > return 1; > > return slices; > } > > This may be clearer, but felt a bit like overkill... > > Thoughts? Seems a bit overkill yes... I was more thinking of a define and the person in charge of adding the slice support would just need to look for references to that define to know (some of) the places that would need rework/review. So, unless someone else thinks it's good to introduce it right now you can ignore that. Thanks,
On Wed, Mar 27, 2019 at 03:21:02PM +0000, Julien Thierry wrote: > > > On 27/03/2019 10:33, Dave Martin wrote: > > On Wed, Mar 27, 2019 at 09:47:42AM +0000, Julien Thierry wrote: > >> Hi Dave, > >> > >> On 19/03/2019 17:52, Dave Martin wrote: [...] > >>> +static unsigned long num_sve_regs(const struct kvm_vcpu *vcpu) > >>> +{ > >>> + /* Only the first slice ever exists, for now */ > >>> + const unsigned int slices = 1; > >> > >> Nit: Might be worth introducing a macro/inline function for the number > >> of slices supported. This way, the day we need to change that, we only > >> need to look for that identifier. > > > > ... Reasonable point, but I wanted to avoid inventing anything > > prematurely, partly because sve_reg_to_region() will need work in order > > to support multiple slices (though it's not rocket science). > > > > I could introduce something like the following: > > > > static unsigned int sve_num_slices(const struct kvm_vcpu *vcpu) > > { > > unsigned int slice_size = KVM_REG_SIZE(KVM_REG_ARM64_SVE_ZREG(0, 0)); > > unsigned int slices = DIV_ROUND_UP(vcpu->arch.sve_max_vl, slice_size); > > > > /* > > * For now, the SVE register ioctl access code won't work > > * properly with multiple register slices. KVM should prevent > > * configuration of a vcpu with a maximum vector length large > > * enough to trigger this: > > */ > > if (WARN_ON_ONCE(slices > 1)) > > return 1; > > > > return slices; > > } > > > > This may be clearer, but felt a bit like overkill... > > > > Thoughts? > > Seems a bit overkill yes... I was more thinking of a define and the > person in charge of adding the slice support would just need to look for > references to that define to know (some of) the places that would need > rework/review. > > So, unless someone else thinks it's good to introduce it right now you > can ignore that. OK, how about the following? This keeps things minimal, but should help future maintainers know that something may need updating here in the future. Cheers ---Dave diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c index ea5219d..086ab05 100644 --- a/arch/arm64/kvm/guest.c +++ b/arch/arm64/kvm/guest.c @@ -289,6 +289,13 @@ static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) #define KVM_SVE_ZREG_SIZE KVM_REG_SIZE(KVM_REG_ARM64_SVE_ZREG(0, 0)) #define KVM_SVE_PREG_SIZE KVM_REG_SIZE(KVM_REG_ARM64_SVE_PREG(0, 0)) +/* + * number of register slices required to cover each whole SVE register on vcpu + * NOTE: If you are tempted to modify this, you must also rework + * sve_reg_to_region() to match: + */ +#define vcpu_sve_slices(vcpu) 1 + /* Bounds of a single SVE register slice within vcpu->arch.sve_state */ struct sve_state_reg_region { unsigned int koffset; /* offset into sve_state in kernel memory */ @@ -505,7 +512,7 @@ static int get_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) static unsigned long num_sve_regs(const struct kvm_vcpu *vcpu) { /* Only the first slice ever exists, for now */ - const unsigned int slices = 1; + const unsigned int slices = vcpu_sve_slices(vcpu); if (!vcpu_has_sve(vcpu)) return 0; @@ -521,7 +528,7 @@ static int copy_sve_reg_indices(const struct kvm_vcpu *vcpu, u64 __user *uindices) { /* Only the first slice ever exists, for now */ - const unsigned int slices = 1; + const unsigned int slices = vcpu_sve_slices(vcpu); u64 reg; unsigned int i, n; int num_regs = 0;
On 28/03/2019 12:27, Dave Martin wrote: > On Wed, Mar 27, 2019 at 03:21:02PM +0000, Julien Thierry wrote: >> >> >> On 27/03/2019 10:33, Dave Martin wrote: >>> On Wed, Mar 27, 2019 at 09:47:42AM +0000, Julien Thierry wrote: >>>> Hi Dave, >>>> >>>> On 19/03/2019 17:52, Dave Martin wrote: > > [...] > >>>>> +static unsigned long num_sve_regs(const struct kvm_vcpu *vcpu) >>>>> +{ >>>>> + /* Only the first slice ever exists, for now */ >>>>> + const unsigned int slices = 1; >>>> >>>> Nit: Might be worth introducing a macro/inline function for the number >>>> of slices supported. This way, the day we need to change that, we only >>>> need to look for that identifier. >>> >>> ... Reasonable point, but I wanted to avoid inventing anything >>> prematurely, partly because sve_reg_to_region() will need work in order >>> to support multiple slices (though it's not rocket science). >>> >>> I could introduce something like the following: >>> >>> static unsigned int sve_num_slices(const struct kvm_vcpu *vcpu) >>> { >>> unsigned int slice_size = KVM_REG_SIZE(KVM_REG_ARM64_SVE_ZREG(0, 0)); >>> unsigned int slices = DIV_ROUND_UP(vcpu->arch.sve_max_vl, slice_size); >>> >>> /* >>> * For now, the SVE register ioctl access code won't work >>> * properly with multiple register slices. KVM should prevent >>> * configuration of a vcpu with a maximum vector length large >>> * enough to trigger this: >>> */ >>> if (WARN_ON_ONCE(slices > 1)) >>> return 1; >>> >>> return slices; >>> } >>> >>> This may be clearer, but felt a bit like overkill... >>> >>> Thoughts? >> >> Seems a bit overkill yes... I was more thinking of a define and the >> person in charge of adding the slice support would just need to look for >> references to that define to know (some of) the places that would need >> rework/review. >> >> So, unless someone else thinks it's good to introduce it right now you >> can ignore that. > > OK, how about the following? This keeps things minimal, but should help > future maintainers know that something may need updating here in the > future. > Yes, I think this looks good. Thanks,
On Thu, Mar 28, 2019 at 02:29:23PM +0000, Julien Thierry wrote: > > > On 28/03/2019 12:27, Dave Martin wrote: > > On Wed, Mar 27, 2019 at 03:21:02PM +0000, Julien Thierry wrote: > >> > >> > >> On 27/03/2019 10:33, Dave Martin wrote: [...] > >>> return slices; > >>> } > >>> > >>> This may be clearer, but felt a bit like overkill... > >>> > >>> Thoughts? > >> > >> Seems a bit overkill yes... I was more thinking of a define and the > >> person in charge of adding the slice support would just need to look for > >> references to that define to know (some of) the places that would need > >> rework/review. > >> > >> So, unless someone else thinks it's good to introduce it right now you > >> can ignore that. > > > > OK, how about the following? This keeps things minimal, but should help > > future maintainers know that something may need updating here in the > > future. > > > > Yes, I think this looks good. OK, are you happy for me to keep your Reviewed-by with that change? Cheers ---Dave
On 28/03/2019 16:48, Dave Martin wrote: > On Thu, Mar 28, 2019 at 02:29:23PM +0000, Julien Thierry wrote: >> >> >> On 28/03/2019 12:27, Dave Martin wrote: >>> On Wed, Mar 27, 2019 at 03:21:02PM +0000, Julien Thierry wrote: >>>> >>>> >>>> On 27/03/2019 10:33, Dave Martin wrote: > > [...] > >>>>> return slices; >>>>> } >>>>> >>>>> This may be clearer, but felt a bit like overkill... >>>>> >>>>> Thoughts? >>>> >>>> Seems a bit overkill yes... I was more thinking of a define and the >>>> person in charge of adding the slice support would just need to look for >>>> references to that define to know (some of) the places that would need >>>> rework/review. >>>> >>>> So, unless someone else thinks it's good to introduce it right now you >>>> can ignore that. >>> >>> OK, how about the following? This keeps things minimal, but should help >>> future maintainers know that something may need updating here in the >>> future. >>> >> >> Yes, I think this looks good. > > OK, are you happy for me to keep your Reviewed-by with that change? > Yes, please do! Thanks,
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c index 736d8cb..585c31e5 100644 --- a/arch/arm64/kvm/guest.c +++ b/arch/arm64/kvm/guest.c @@ -411,6 +411,56 @@ static int get_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) return copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)) ? -EFAULT : 0; } +static unsigned long num_sve_regs(const struct kvm_vcpu *vcpu) +{ + /* Only the first slice ever exists, for now */ + const unsigned int slices = 1; + + if (!vcpu_has_sve(vcpu)) + return 0; + + return slices * (SVE_NUM_PREGS + SVE_NUM_ZREGS + 1 /* FFR */); +} + +static int copy_sve_reg_indices(const struct kvm_vcpu *vcpu, + u64 __user *uindices) +{ + /* Only the first slice ever exists, for now */ + const unsigned int slices = 1; + u64 reg; + unsigned int i, n; + int num_regs = 0; + + if (!vcpu_has_sve(vcpu)) + return 0; + + for (i = 0; i < slices; i++) { + for (n = 0; n < SVE_NUM_ZREGS; n++) { + reg = KVM_REG_ARM64_SVE_ZREG(n, i); + if (put_user(reg, uindices++)) + return -EFAULT; + + num_regs++; + } + + for (n = 0; n < SVE_NUM_PREGS; n++) { + reg = KVM_REG_ARM64_SVE_PREG(n, i); + if (put_user(reg, uindices++)) + return -EFAULT; + + num_regs++; + } + + reg = KVM_REG_ARM64_SVE_FFR(i); + if (put_user(reg, uindices++)) + return -EFAULT; + + num_regs++; + } + + return num_regs; +} + /** * kvm_arm_num_regs - how many registers do we present via KVM_GET_ONE_REG * @@ -421,6 +471,7 @@ unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu) unsigned long res = 0; res += num_core_regs(vcpu); + res += num_sve_regs(vcpu); res += kvm_arm_num_sys_reg_descs(vcpu); res += kvm_arm_get_fw_num_regs(vcpu); res += NUM_TIMER_REGS; @@ -442,6 +493,11 @@ int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices) return ret; uindices += ret; + ret = copy_sve_reg_indices(vcpu, uindices); + if (ret) + return ret; + uindices += ret; + ret = kvm_arm_copy_fw_reg_indices(vcpu, uindices); if (ret) return ret;
This patch includes the SVE register IDs in the list returned by KVM_GET_REG_LIST, as appropriate. On a non-SVE-enabled vcpu, no new IDs are added. On an SVE-enabled vcpu, IDs for the FPSIMD V-registers are removed from the list, since userspace is required to access the Z- registers instead in order to access the V-register content. For the variably-sized SVE registers, the appropriate set of slice IDs are enumerated, depending on the maximum vector length for the vcpu. As it currently stands, the SVE architecture never requires more than one slice to exist per register, so this patch adds no explicit support for enumerating multiple slices. The code can be extended straightforwardly to support this in the future, if needed. Signed-off-by: Dave Martin <Dave.Martin@arm.com> --- Changes since v5: (Dropped Julien Thierry's Reviewed-by due to non-trivial rebasing) * Move mis-split reword to prevent put_user()s being accidentally the correct size from KVM: arm64/sve: Add pseudo-register for the guest's vector lengths. --- arch/arm64/kvm/guest.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)