| Message ID | 1555086498-26691-10-git-send-email-Dave.Martin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: SVE cleanups | expand |
On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote: > A complicated DIV_ROUND_UP() expression is currently written out > explicitly in multiple places in order to specify the size of the > bitmap exchanged with userspace to represent the value of the > KVM_REG_ARM64_SVE_VLS pseudo-register. > > To make this more readable, this patch replaces these with a single > define. > > Since the number of words in a bitmap is just the index of the last > word used + 1, this patch expresses the bound that way instead. > This should make it clearer what is being expressed. > > Since use of DIV_ROUND_UP() was the only reason for including > <linux/kernel.h> in guest.c, this patch removes that #include. > > No functional change. > > Suggested-by: Andrew Jones <drjones@redhat.com> > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > --- > arch/arm64/kvm/guest.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index 73044e3..f025a2f 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -23,7 +23,6 @@ > #include <linux/errno.h> > #include <linux/err.h> > #include <linux/nospec.h> > -#include <linux/kernel.h> > #include <linux/kvm_host.h> > #include <linux/module.h> > #include <linux/stddef.h> > @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64) > #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64) > > +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1) > + > static bool vq_present( > - const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)], > + const u64 (*const vqs)[SVE_VLS_WORDS], > unsigned int vq) > { > return (*vqs)[vq_word(vq)] & vq_mask(vq); > @@ -219,7 +220,7 @@ static bool vq_present( > static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > { > unsigned int max_vq, vq; > - u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)]; > + u64 vqs[SVE_VLS_WORDS]; > > if (!vcpu_has_sve(vcpu)) > return -ENOENT; > @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > { > unsigned int max_vq, vq; > - u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)]; > + u64 vqs[SVE_VLS_WORDS]; > > if (!vcpu_has_sve(vcpu)) > return -ENOENT; > -- > 2.1.4 > This is good, but I wonder if we could define the number of VLS words in the documentation in terms of SVE_VQ_MAX too. Currently it's just the hard coded 8 ("__u64 vector_lengths[8]"). Reviewed-by: Andrew Jones <drjones@redhat.com>
On Mon, Apr 15, 2019 at 05:20:37PM +0200, Andrew Jones wrote: > On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote: > > A complicated DIV_ROUND_UP() expression is currently written out > > explicitly in multiple places in order to specify the size of the > > bitmap exchanged with userspace to represent the value of the > > KVM_REG_ARM64_SVE_VLS pseudo-register. > > > > To make this more readable, this patch replaces these with a single > > define. > > > > Since the number of words in a bitmap is just the index of the last > > word used + 1, this patch expresses the bound that way instead. > > This should make it clearer what is being expressed. > > > > Since use of DIV_ROUND_UP() was the only reason for including > > <linux/kernel.h> in guest.c, this patch removes that #include. > > > > No functional change. > > > > Suggested-by: Andrew Jones <drjones@redhat.com> > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > --- > > arch/arm64/kvm/guest.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > > index 73044e3..f025a2f 100644 > > --- a/arch/arm64/kvm/guest.c > > +++ b/arch/arm64/kvm/guest.c > > @@ -23,7 +23,6 @@ > > #include <linux/errno.h> > > #include <linux/err.h> > > #include <linux/nospec.h> > > -#include <linux/kernel.h> > > #include <linux/kvm_host.h> > > #include <linux/module.h> > > #include <linux/stddef.h> > > @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64) > > #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64) > > > > +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1) > > + > > static bool vq_present( > > - const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)], > > + const u64 (*const vqs)[SVE_VLS_WORDS], > > unsigned int vq) > > { > > return (*vqs)[vq_word(vq)] & vq_mask(vq); > > @@ -219,7 +220,7 @@ static bool vq_present( > > static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > { > > unsigned int max_vq, vq; > > - u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)]; > > + u64 vqs[SVE_VLS_WORDS]; > > > > if (!vcpu_has_sve(vcpu)) > > return -ENOENT; > > @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > { > > unsigned int max_vq, vq; > > - u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)]; > > + u64 vqs[SVE_VLS_WORDS]; > > > > if (!vcpu_has_sve(vcpu)) > > return -ENOENT; > > -- > > 2.1.4 > > > > This is good, but I wonder if we could define the number of VLS words in > the documentation in terms of SVE_VQ_MAX too. Currently it's just the > hard coded 8 ("__u64 vector_lengths[8]"). > > Reviewed-by: Andrew Jones <drjones@redhat.com> I see your point, but SVE_VQ_MAX isn't really part of the KVM API, so I was avoiding it here. [8] is at least impossible to misinterpret, even if it's not the most self-explanatory option. Is that OK? Cheers ---Dave
On Tue, Apr 16, 2019 at 01:41:42PM +0100, Dave Martin wrote: > On Mon, Apr 15, 2019 at 05:20:37PM +0200, Andrew Jones wrote: > > On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote: > > > A complicated DIV_ROUND_UP() expression is currently written out > > > explicitly in multiple places in order to specify the size of the > > > bitmap exchanged with userspace to represent the value of the > > > KVM_REG_ARM64_SVE_VLS pseudo-register. > > > > > > To make this more readable, this patch replaces these with a single > > > define. > > > > > > Since the number of words in a bitmap is just the index of the last > > > word used + 1, this patch expresses the bound that way instead. > > > This should make it clearer what is being expressed. > > > > > > Since use of DIV_ROUND_UP() was the only reason for including > > > <linux/kernel.h> in guest.c, this patch removes that #include. > > > > > > No functional change. > > > > > > Suggested-by: Andrew Jones <drjones@redhat.com> > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > --- > > > arch/arm64/kvm/guest.c | 9 +++++---- > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > > > index 73044e3..f025a2f 100644 > > > --- a/arch/arm64/kvm/guest.c > > > +++ b/arch/arm64/kvm/guest.c > > > @@ -23,7 +23,6 @@ > > > #include <linux/errno.h> > > > #include <linux/err.h> > > > #include <linux/nospec.h> > > > -#include <linux/kernel.h> > > > #include <linux/kvm_host.h> > > > #include <linux/module.h> > > > #include <linux/stddef.h> > > > @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > > #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64) > > > #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64) > > > > > > +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1) > > > + > > > static bool vq_present( > > > - const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)], > > > + const u64 (*const vqs)[SVE_VLS_WORDS], > > > unsigned int vq) > > > { > > > return (*vqs)[vq_word(vq)] & vq_mask(vq); > > > @@ -219,7 +220,7 @@ static bool vq_present( > > > static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > > { > > > unsigned int max_vq, vq; > > > - u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)]; > > > + u64 vqs[SVE_VLS_WORDS]; > > > > > > if (!vcpu_has_sve(vcpu)) > > > return -ENOENT; > > > @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > > static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > > { > > > unsigned int max_vq, vq; > > > - u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)]; > > > + u64 vqs[SVE_VLS_WORDS]; > > > > > > if (!vcpu_has_sve(vcpu)) > > > return -ENOENT; > > > -- > > > 2.1.4 > > > > > > > This is good, but I wonder if we could define the number of VLS words in > > the documentation in terms of SVE_VQ_MAX too. Currently it's just the > > hard coded 8 ("__u64 vector_lengths[8]"). > > > > Reviewed-by: Andrew Jones <drjones@redhat.com> > > I see your point, but SVE_VQ_MAX isn't really part of the KVM API, so I > was avoiding it here. It's not part of the KVM API, but it is uapi (asm/sigcontext.h). I'd prefer we use it than to encourage KVM userspace to scatter 8's around. > > [8] is at least impossible to misinterpret, even if it's not the most > self-explanatory option. It's impossible to misinterpret, but also begs the questions of where it comes from and if it will always be that way forever and ever. Thanks, drew
On Tue, Apr 16, 2019 at 03:00:54PM +0200, Andrew Jones wrote: > On Tue, Apr 16, 2019 at 01:41:42PM +0100, Dave Martin wrote: > > On Mon, Apr 15, 2019 at 05:20:37PM +0200, Andrew Jones wrote: > > > On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote: > > > > A complicated DIV_ROUND_UP() expression is currently written out > > > > explicitly in multiple places in order to specify the size of the > > > > bitmap exchanged with userspace to represent the value of the > > > > KVM_REG_ARM64_SVE_VLS pseudo-register. > > > > > > > > To make this more readable, this patch replaces these with a single > > > > define. > > > > > > > > Since the number of words in a bitmap is just the index of the last > > > > word used + 1, this patch expresses the bound that way instead. > > > > This should make it clearer what is being expressed. > > > > > > > > Since use of DIV_ROUND_UP() was the only reason for including > > > > <linux/kernel.h> in guest.c, this patch removes that #include. > > > > > > > > No functional change. > > > > > > > > Suggested-by: Andrew Jones <drjones@redhat.com> > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > > --- > > > > arch/arm64/kvm/guest.c | 9 +++++---- > > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > > > > index 73044e3..f025a2f 100644 > > > > --- a/arch/arm64/kvm/guest.c > > > > +++ b/arch/arm64/kvm/guest.c > > > > @@ -23,7 +23,6 @@ > > > > #include <linux/errno.h> > > > > #include <linux/err.h> > > > > #include <linux/nospec.h> > > > > -#include <linux/kernel.h> > > > > #include <linux/kvm_host.h> > > > > #include <linux/module.h> > > > > #include <linux/stddef.h> > > > > @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > > > #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64) > > > > #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64) > > > > > > > > +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1) > > > > + > > > > static bool vq_present( > > > > - const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)], > > > > + const u64 (*const vqs)[SVE_VLS_WORDS], > > > > unsigned int vq) > > > > { > > > > return (*vqs)[vq_word(vq)] & vq_mask(vq); > > > > @@ -219,7 +220,7 @@ static bool vq_present( > > > > static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > > > { > > > > unsigned int max_vq, vq; > > > > - u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)]; > > > > + u64 vqs[SVE_VLS_WORDS]; > > > > > > > > if (!vcpu_has_sve(vcpu)) > > > > return -ENOENT; > > > > @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > > > static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > > > { > > > > unsigned int max_vq, vq; > > > > - u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)]; > > > > + u64 vqs[SVE_VLS_WORDS]; > > > > > > > > if (!vcpu_has_sve(vcpu)) > > > > return -ENOENT; > > > > -- > > > > 2.1.4 > > > > > > > > > > This is good, but I wonder if we could define the number of VLS words in > > > the documentation in terms of SVE_VQ_MAX too. Currently it's just the > > > hard coded 8 ("__u64 vector_lengths[8]"). > > > > > > Reviewed-by: Andrew Jones <drjones@redhat.com> > > > > I see your point, but SVE_VQ_MAX isn't really part of the KVM API, so I > > was avoiding it here. > > It's not part of the KVM API, but it is uapi (asm/sigcontext.h). I'd > prefer we use it than to encourage KVM userspace to scatter 8's around. > > > > > [8] is at least impossible to misinterpret, even if it's not the most > > self-explanatory option. > > It's impossible to misinterpret, but also begs the questions of where > it comes from and if it will always be that way forever and ever. It is sized to the maximum that makes sense without a major API redesign, so the expectation is that it will never change (and userspace should assume that it does not). I sympathise though, since this case is less intuitive than most. We could add some dedicated defines for this: arch/arm64/include/uapi/asm/kvm.h: #include <asm/sve_context.h> /* which we already have anyway */ #define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX #define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN /* Vector lengths pseudo-register: */ #define KVM_REG_ARM64_SVE_VLS (KVM_REG_ARM64 | KVM_REG_ARM64_SVE | \ KVM_REG_SIZE_U512 | 0xffff) #define KVM_ARM64_SVE_VLS_WORDS \ ((KVM_ARM64_SVE_VQ_MAX - KVM_ARM64_SVE_VQ_MIN) / 64 + 1) Then document as follows: Documentation/virtual/kvm/api.txt: __u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS]; if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_SVE_VQ_MAX) (vector_lengths[(vq - KVM_ARM64_SVE_VQ_MIN) / 64] >> ((vq - KVM_ARM64_SVE_VQ_MIN) % 64)) & 1) /* Vector length vq * 16 bytes supported */ else /* Vector length vq * 16 bytes not supported */ Does that work for you? Doing things this way avoids people having to include <asm/sigcontext.h> (and the consequent clashes with glibc headers). Cheers ---Dave
On Tue, Apr 16, 2019 at 03:10:55PM +0100, Dave Martin wrote: > On Tue, Apr 16, 2019 at 03:00:54PM +0200, Andrew Jones wrote: > > On Tue, Apr 16, 2019 at 01:41:42PM +0100, Dave Martin wrote: > > > On Mon, Apr 15, 2019 at 05:20:37PM +0200, Andrew Jones wrote: > > > > On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote: > > > > > A complicated DIV_ROUND_UP() expression is currently written out > > > > > explicitly in multiple places in order to specify the size of the > > > > > bitmap exchanged with userspace to represent the value of the > > > > > KVM_REG_ARM64_SVE_VLS pseudo-register. > > > > > > > > > > To make this more readable, this patch replaces these with a single > > > > > define. > > > > > > > > > > Since the number of words in a bitmap is just the index of the last > > > > > word used + 1, this patch expresses the bound that way instead. > > > > > This should make it clearer what is being expressed. > > > > > > > > > > Since use of DIV_ROUND_UP() was the only reason for including > > > > > <linux/kernel.h> in guest.c, this patch removes that #include. > > > > > > > > > > No functional change. > > > > > > > > > > Suggested-by: Andrew Jones <drjones@redhat.com> > > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > > > --- > > > > > arch/arm64/kvm/guest.c | 9 +++++---- > > > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > > > > > index 73044e3..f025a2f 100644 > > > > > --- a/arch/arm64/kvm/guest.c > > > > > +++ b/arch/arm64/kvm/guest.c > > > > > @@ -23,7 +23,6 @@ > > > > > #include <linux/errno.h> > > > > > #include <linux/err.h> > > > > > #include <linux/nospec.h> > > > > > -#include <linux/kernel.h> > > > > > #include <linux/kvm_host.h> > > > > > #include <linux/module.h> > > > > > #include <linux/stddef.h> > > > > > @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > > > > #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64) > > > > > #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64) > > > > > > > > > > +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1) > > > > > + > > > > > static bool vq_present( > > > > > - const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)], > > > > > + const u64 (*const vqs)[SVE_VLS_WORDS], > > > > > unsigned int vq) > > > > > { > > > > > return (*vqs)[vq_word(vq)] & vq_mask(vq); > > > > > @@ -219,7 +220,7 @@ static bool vq_present( > > > > > static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > > > > { > > > > > unsigned int max_vq, vq; > > > > > - u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)]; > > > > > + u64 vqs[SVE_VLS_WORDS]; > > > > > > > > > > if (!vcpu_has_sve(vcpu)) > > > > > return -ENOENT; > > > > > @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > > > > static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > > > > { > > > > > unsigned int max_vq, vq; > > > > > - u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)]; > > > > > + u64 vqs[SVE_VLS_WORDS]; > > > > > > > > > > if (!vcpu_has_sve(vcpu)) > > > > > return -ENOENT; > > > > > -- > > > > > 2.1.4 > > > > > > > > > > > > > This is good, but I wonder if we could define the number of VLS words in > > > > the documentation in terms of SVE_VQ_MAX too. Currently it's just the > > > > hard coded 8 ("__u64 vector_lengths[8]"). > > > > > > > > Reviewed-by: Andrew Jones <drjones@redhat.com> > > > > > > I see your point, but SVE_VQ_MAX isn't really part of the KVM API, so I > > > was avoiding it here. > > > > It's not part of the KVM API, but it is uapi (asm/sigcontext.h). I'd > > prefer we use it than to encourage KVM userspace to scatter 8's around. > > > > > > > > [8] is at least impossible to misinterpret, even if it's not the most > > > self-explanatory option. > > > > It's impossible to misinterpret, but also begs the questions of where > > it comes from and if it will always be that way forever and ever. > > It is sized to the maximum that makes sense without a major API > redesign, so the expectation is that it will never change (and userspace > should assume that it does not). > > I sympathise though, since this case is less intuitive than most. > > We could add some dedicated defines for this: > > arch/arm64/include/uapi/asm/kvm.h: > > #include <asm/sve_context.h> /* which we already have anyway */ > > #define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX > #define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN > > /* Vector lengths pseudo-register: */ > #define KVM_REG_ARM64_SVE_VLS (KVM_REG_ARM64 | KVM_REG_ARM64_SVE | \ > KVM_REG_SIZE_U512 | 0xffff) > #define KVM_ARM64_SVE_VLS_WORDS \ > ((KVM_ARM64_SVE_VQ_MAX - KVM_ARM64_SVE_VQ_MIN) / 64 + 1) > > > Then document as follows: > Documentation/virtual/kvm/api.txt: > > __u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS]; > > if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_SVE_VQ_MAX) > (vector_lengths[(vq - KVM_ARM64_SVE_VQ_MIN) / 64] >> > ((vq - KVM_ARM64_SVE_VQ_MIN) % 64)) & 1) > /* Vector length vq * 16 bytes supported */ > else > /* Vector length vq * 16 bytes not supported */ > > > Does that work for you? > > Doing things this way avoids people having to include <asm/sigcontext.h> > (and the consequent clashes with glibc headers). > I like that. Thanks, drew
On Tue, Apr 16, 2019 at 04:28:31PM +0200, Andrew Jones wrote: > On Tue, Apr 16, 2019 at 03:10:55PM +0100, Dave Martin wrote: > > On Tue, Apr 16, 2019 at 03:00:54PM +0200, Andrew Jones wrote: > > > On Tue, Apr 16, 2019 at 01:41:42PM +0100, Dave Martin wrote: > > > > On Mon, Apr 15, 2019 at 05:20:37PM +0200, Andrew Jones wrote: > > > > > On Fri, Apr 12, 2019 at 05:28:13PM +0100, Dave Martin wrote: > > > > > > A complicated DIV_ROUND_UP() expression is currently written out > > > > > > explicitly in multiple places in order to specify the size of the > > > > > > bitmap exchanged with userspace to represent the value of the > > > > > > KVM_REG_ARM64_SVE_VLS pseudo-register. > > > > > > > > > > > > To make this more readable, this patch replaces these with a single > > > > > > define. > > > > > > > > > > > > Since the number of words in a bitmap is just the index of the last > > > > > > word used + 1, this patch expresses the bound that way instead. > > > > > > This should make it clearer what is being expressed. > > > > > > > > > > > > Since use of DIV_ROUND_UP() was the only reason for including > > > > > > <linux/kernel.h> in guest.c, this patch removes that #include. > > > > > > > > > > > > No functional change. > > > > > > > > > > > > Suggested-by: Andrew Jones <drjones@redhat.com> > > > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > > > > --- > > > > > > arch/arm64/kvm/guest.c | 9 +++++---- > > > > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > > > > > > index 73044e3..f025a2f 100644 > > > > > > --- a/arch/arm64/kvm/guest.c > > > > > > +++ b/arch/arm64/kvm/guest.c > > > > > > @@ -23,7 +23,6 @@ > > > > > > #include <linux/errno.h> > > > > > > #include <linux/err.h> > > > > > > #include <linux/nospec.h> > > > > > > -#include <linux/kernel.h> > > > > > > #include <linux/kvm_host.h> > > > > > > #include <linux/module.h> > > > > > > #include <linux/stddef.h> > > > > > > @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > > > > > #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64) > > > > > > #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64) > > > > > > > > > > > > +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1) > > > > > > + > > > > > > static bool vq_present( > > > > > > - const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)], > > > > > > + const u64 (*const vqs)[SVE_VLS_WORDS], > > > > > > unsigned int vq) > > > > > > { > > > > > > return (*vqs)[vq_word(vq)] & vq_mask(vq); > > > > > > @@ -219,7 +220,7 @@ static bool vq_present( > > > > > > static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > > > > > { > > > > > > unsigned int max_vq, vq; > > > > > > - u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)]; > > > > > > + u64 vqs[SVE_VLS_WORDS]; > > > > > > > > > > > > if (!vcpu_has_sve(vcpu)) > > > > > > return -ENOENT; > > > > > > @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > > > > > static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > > > > > { > > > > > > unsigned int max_vq, vq; > > > > > > - u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)]; > > > > > > + u64 vqs[SVE_VLS_WORDS]; > > > > > > > > > > > > if (!vcpu_has_sve(vcpu)) > > > > > > return -ENOENT; > > > > > > -- > > > > > > 2.1.4 > > > > > > > > > > > > > > > > This is good, but I wonder if we could define the number of VLS words in > > > > > the documentation in terms of SVE_VQ_MAX too. Currently it's just the > > > > > hard coded 8 ("__u64 vector_lengths[8]"). > > > > > > > > > > Reviewed-by: Andrew Jones <drjones@redhat.com> > > > > > > > > I see your point, but SVE_VQ_MAX isn't really part of the KVM API, so I > > > > was avoiding it here. > > > > > > It's not part of the KVM API, but it is uapi (asm/sigcontext.h). I'd > > > prefer we use it than to encourage KVM userspace to scatter 8's around. > > > > > > > > > > > [8] is at least impossible to misinterpret, even if it's not the most > > > > self-explanatory option. > > > > > > It's impossible to misinterpret, but also begs the questions of where > > > it comes from and if it will always be that way forever and ever. > > > > It is sized to the maximum that makes sense without a major API > > redesign, so the expectation is that it will never change (and userspace > > should assume that it does not). > > > > I sympathise though, since this case is less intuitive than most. > > > > We could add some dedicated defines for this: > > > > arch/arm64/include/uapi/asm/kvm.h: > > > > #include <asm/sve_context.h> /* which we already have anyway */ > > > > #define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX > > #define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN > > > > /* Vector lengths pseudo-register: */ > > #define KVM_REG_ARM64_SVE_VLS (KVM_REG_ARM64 | KVM_REG_ARM64_SVE | \ > > KVM_REG_SIZE_U512 | 0xffff) > > #define KVM_ARM64_SVE_VLS_WORDS \ > > ((KVM_ARM64_SVE_VQ_MAX - KVM_ARM64_SVE_VQ_MIN) / 64 + 1) > > > > > > Then document as follows: > > Documentation/virtual/kvm/api.txt: > > > > __u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS]; > > > > if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_SVE_VQ_MAX) > > (vector_lengths[(vq - KVM_ARM64_SVE_VQ_MIN) / 64] >> > > ((vq - KVM_ARM64_SVE_VQ_MIN) % 64)) & 1) > > /* Vector length vq * 16 bytes supported */ > > else > > /* Vector length vq * 16 bytes not supported */ > > > > > > Does that work for you? > > > > Doing things this way avoids people having to include <asm/sigcontext.h> > > (and the consequent clashes with glibc headers). > > > > I like that. OK, I'll do that then. I was worried this was going to end up too cumbersome / too verbose, but it doesn't feel too bad in practice. Cheers ---Dave
On Tue, Apr 16, 2019 at 04:28:31PM +0200, Andrew Jones wrote: > On Tue, Apr 16, 2019 at 03:10:55PM +0100, Dave Martin wrote: [...] > > arch/arm64/include/uapi/asm/kvm.h: > > > > #include <asm/sve_context.h> /* which we already have anyway */ > > > > #define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX > > #define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN > > > > /* Vector lengths pseudo-register: */ > > #define KVM_REG_ARM64_SVE_VLS (KVM_REG_ARM64 | KVM_REG_ARM64_SVE | \ > > KVM_REG_SIZE_U512 | 0xffff) > > #define KVM_ARM64_SVE_VLS_WORDS \ > > ((KVM_ARM64_SVE_VQ_MAX - KVM_ARM64_SVE_VQ_MIN) / 64 + 1) > > > > > > Then document as follows: > > Documentation/virtual/kvm/api.txt: > > > > __u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS]; > > > > if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_SVE_VQ_MAX) > > (vector_lengths[(vq - KVM_ARM64_SVE_VQ_MIN) / 64] >> > > ((vq - KVM_ARM64_SVE_VQ_MIN) % 64)) & 1) > > /* Vector length vq * 16 bytes supported */ > > else > > /* Vector length vq * 16 bytes not supported */ > > > > > > Does that work for you? > > > > Doing things this way avoids people having to include <asm/sigcontext.h> > > (and the consequent clashes with glibc headers). > > > > I like that. OK, are you happy with the following on top of this patch? Cheers ---Dave --8<-- diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt index 68509de..03df379 100644 --- a/Documentation/virtual/kvm/api.txt +++ b/Documentation/virtual/kvm/api.txt @@ -2171,13 +2171,15 @@ and KVM_ARM_VCPU_FINALIZE for more information about this procedure. KVM_REG_ARM64_SVE_VLS is a pseudo-register that allows the set of vector lengths supported by the vcpu to be discovered and configured by userspace. When transferred to or from user memory via KVM_GET_ONE_REG -or KVM_SET_ONE_REG, the value of this register is of type __u64[8], and -encodes the set of vector lengths as follows: +or KVM_SET_ONE_REG, the value of this register is of type +__u64[KVM_ARM64_SVE_VLS_WORDS], and encodes the set of vector lengths as +follows: -__u64 vector_lengths[8]; +__u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS]; if (vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX && - ((vector_lengths[(vq - 1) / 64] >> ((vq - 1) % 64)) & 1)) + ((vector_lengths[(vq - KVM_ARM64_SVE_VQ_MIN) / 64] >> + ((vq - KVM_ARM64_SVE_VQ_MIN) % 64)) & 1)) /* Vector length vq * 16 bytes supported */ else /* Vector length vq * 16 bytes not supported */ diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h index 2a04ef0..edd2db8 100644 --- a/arch/arm64/include/uapi/asm/kvm.h +++ b/arch/arm64/include/uapi/asm/kvm.h @@ -258,9 +258,14 @@ struct kvm_vcpu_events { KVM_REG_SIZE_U256 | \ ((i) & (KVM_ARM64_SVE_MAX_SLICES - 1))) +#define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN +#define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX + /* Vector lengths pseudo-register: */ #define KVM_REG_ARM64_SVE_VLS (KVM_REG_ARM64 | KVM_REG_ARM64_SVE | \ KVM_REG_SIZE_U512 | 0xffff) +#define KVM_ARM64_SVE_VLS_WORDS \ + ((KVM_ARM64_SVE_VQ_MAX - KVM_ARM64_SVE_VQ_MIN) / 64 + 1) /* Device Control API: ARM VGIC */ #define KVM_DEV_ARM_VGIC_GRP_ADDR 0 diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c index f025a2f..5bb909c 100644 --- a/arch/arm64/kvm/guest.c +++ b/arch/arm64/kvm/guest.c @@ -208,10 +208,8 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64) #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64) -#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1) - static bool vq_present( - const u64 (*const vqs)[SVE_VLS_WORDS], + const u64 (*const vqs)[KVM_ARM64_SVE_VLS_WORDS], unsigned int vq) { return (*vqs)[vq_word(vq)] & vq_mask(vq); @@ -220,7 +218,7 @@ static bool vq_present( static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) { unsigned int max_vq, vq; - u64 vqs[SVE_VLS_WORDS]; + u64 vqs[KVM_ARM64_SVE_VLS_WORDS]; if (!vcpu_has_sve(vcpu)) return -ENOENT; @@ -244,7 +242,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) { unsigned int max_vq, vq; - u64 vqs[SVE_VLS_WORDS]; + u64 vqs[KVM_ARM64_SVE_VLS_WORDS]; if (!vcpu_has_sve(vcpu)) return -ENOENT;
On Tue, Apr 16, 2019 at 04:52:28PM +0100, Dave Martin wrote: > On Tue, Apr 16, 2019 at 04:28:31PM +0200, Andrew Jones wrote: > > On Tue, Apr 16, 2019 at 03:10:55PM +0100, Dave Martin wrote: > > [...] > > > > arch/arm64/include/uapi/asm/kvm.h: > > > > > > #include <asm/sve_context.h> /* which we already have anyway */ > > > > > > #define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX > > > #define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN > > > > > > /* Vector lengths pseudo-register: */ > > > #define KVM_REG_ARM64_SVE_VLS (KVM_REG_ARM64 | KVM_REG_ARM64_SVE | \ > > > KVM_REG_SIZE_U512 | 0xffff) > > > #define KVM_ARM64_SVE_VLS_WORDS \ > > > ((KVM_ARM64_SVE_VQ_MAX - KVM_ARM64_SVE_VQ_MIN) / 64 + 1) > > > > > > > > > Then document as follows: > > > Documentation/virtual/kvm/api.txt: > > > > > > __u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS]; > > > > > > if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_SVE_VQ_MAX) > > > (vector_lengths[(vq - KVM_ARM64_SVE_VQ_MIN) / 64] >> > > > ((vq - KVM_ARM64_SVE_VQ_MIN) % 64)) & 1) > > > /* Vector length vq * 16 bytes supported */ > > > else > > > /* Vector length vq * 16 bytes not supported */ > > > > > > > > > Does that work for you? > > > > > > Doing things this way avoids people having to include <asm/sigcontext.h> > > > (and the consequent clashes with glibc headers). > > > > > > > I like that. > > OK, are you happy with the following on top of this patch? > > Cheers > ---Dave > > --8<-- > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > index 68509de..03df379 100644 > --- a/Documentation/virtual/kvm/api.txt > +++ b/Documentation/virtual/kvm/api.txt > @@ -2171,13 +2171,15 @@ and KVM_ARM_VCPU_FINALIZE for more information about this procedure. > KVM_REG_ARM64_SVE_VLS is a pseudo-register that allows the set of vector > lengths supported by the vcpu to be discovered and configured by > userspace. When transferred to or from user memory via KVM_GET_ONE_REG > -or KVM_SET_ONE_REG, the value of this register is of type __u64[8], and > -encodes the set of vector lengths as follows: > +or KVM_SET_ONE_REG, the value of this register is of type > +__u64[KVM_ARM64_SVE_VLS_WORDS], and encodes the set of vector lengths as > +follows: > > -__u64 vector_lengths[8]; > +__u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS]; > > if (vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX && > - ((vector_lengths[(vq - 1) / 64] >> ((vq - 1) % 64)) & 1)) > + ((vector_lengths[(vq - KVM_ARM64_SVE_VQ_MIN) / 64] >> > + ((vq - KVM_ARM64_SVE_VQ_MIN) % 64)) & 1)) > /* Vector length vq * 16 bytes supported */ > else > /* Vector length vq * 16 bytes not supported */ > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h > index 2a04ef0..edd2db8 100644 > --- a/arch/arm64/include/uapi/asm/kvm.h > +++ b/arch/arm64/include/uapi/asm/kvm.h > @@ -258,9 +258,14 @@ struct kvm_vcpu_events { > KVM_REG_SIZE_U256 | \ > ((i) & (KVM_ARM64_SVE_MAX_SLICES - 1))) > > +#define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN > +#define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX > + > /* Vector lengths pseudo-register: */ > #define KVM_REG_ARM64_SVE_VLS (KVM_REG_ARM64 | KVM_REG_ARM64_SVE | \ > KVM_REG_SIZE_U512 | 0xffff) > +#define KVM_ARM64_SVE_VLS_WORDS \ > + ((KVM_ARM64_SVE_VQ_MAX - KVM_ARM64_SVE_VQ_MIN) / 64 + 1) > > /* Device Control API: ARM VGIC */ > #define KVM_DEV_ARM_VGIC_GRP_ADDR 0 > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index f025a2f..5bb909c 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -208,10 +208,8 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64) > #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64) > > -#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1) > - > static bool vq_present( > - const u64 (*const vqs)[SVE_VLS_WORDS], > + const u64 (*const vqs)[KVM_ARM64_SVE_VLS_WORDS], > unsigned int vq) > { > return (*vqs)[vq_word(vq)] & vq_mask(vq); > @@ -220,7 +218,7 @@ static bool vq_present( > static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > { > unsigned int max_vq, vq; > - u64 vqs[SVE_VLS_WORDS]; > + u64 vqs[KVM_ARM64_SVE_VLS_WORDS]; > > if (!vcpu_has_sve(vcpu)) > return -ENOENT; > @@ -244,7 +242,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > { > unsigned int max_vq, vq; > - u64 vqs[SVE_VLS_WORDS]; > + u64 vqs[KVM_ARM64_SVE_VLS_WORDS]; > > if (!vcpu_has_sve(vcpu)) > return -ENOENT; > Yup Reviewed-by: Andrew Jones <drjones@redhat.com> Thanks, drew
On Wed, Apr 17, 2019 at 06:16:56AM +0200, Andrew Jones wrote: > On Tue, Apr 16, 2019 at 04:52:28PM +0100, Dave Martin wrote: > > On Tue, Apr 16, 2019 at 04:28:31PM +0200, Andrew Jones wrote: > > > On Tue, Apr 16, 2019 at 03:10:55PM +0100, Dave Martin wrote: > > > > [...] > > > > > > arch/arm64/include/uapi/asm/kvm.h: > > > > > > > > #include <asm/sve_context.h> /* which we already have anyway */ > > > > > > > > #define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX > > > > #define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN > > > > > > > > /* Vector lengths pseudo-register: */ > > > > #define KVM_REG_ARM64_SVE_VLS (KVM_REG_ARM64 | KVM_REG_ARM64_SVE | \ > > > > KVM_REG_SIZE_U512 | 0xffff) > > > > #define KVM_ARM64_SVE_VLS_WORDS \ > > > > ((KVM_ARM64_SVE_VQ_MAX - KVM_ARM64_SVE_VQ_MIN) / 64 + 1) > > > > > > > > > > > > Then document as follows: > > > > Documentation/virtual/kvm/api.txt: > > > > > > > > __u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS]; > > > > > > > > if (vq >= KVM_ARM64_SVE_VQ_MIN && vq <= KVM_ARM64_SVE_VQ_MAX) > > > > (vector_lengths[(vq - KVM_ARM64_SVE_VQ_MIN) / 64] >> > > > > ((vq - KVM_ARM64_SVE_VQ_MIN) % 64)) & 1) > > > > /* Vector length vq * 16 bytes supported */ > > > > else > > > > /* Vector length vq * 16 bytes not supported */ > > > > > > > > > > > > Does that work for you? > > > > > > > > Doing things this way avoids people having to include <asm/sigcontext.h> > > > > (and the consequent clashes with glibc headers). > > > > > > > > > > I like that. > > > > OK, are you happy with the following on top of this patch? > > > > Cheers > > ---Dave > > > > --8<-- > > > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > > index 68509de..03df379 100644 > > --- a/Documentation/virtual/kvm/api.txt > > +++ b/Documentation/virtual/kvm/api.txt > > @@ -2171,13 +2171,15 @@ and KVM_ARM_VCPU_FINALIZE for more information about this procedure. > > KVM_REG_ARM64_SVE_VLS is a pseudo-register that allows the set of vector > > lengths supported by the vcpu to be discovered and configured by > > userspace. When transferred to or from user memory via KVM_GET_ONE_REG > > -or KVM_SET_ONE_REG, the value of this register is of type __u64[8], and > > -encodes the set of vector lengths as follows: > > +or KVM_SET_ONE_REG, the value of this register is of type > > +__u64[KVM_ARM64_SVE_VLS_WORDS], and encodes the set of vector lengths as > > +follows: > > > > -__u64 vector_lengths[8]; > > +__u64 vector_lengths[KVM_ARM64_SVE_VLS_WORDS]; > > > > if (vq >= SVE_VQ_MIN && vq <= SVE_VQ_MAX && > > - ((vector_lengths[(vq - 1) / 64] >> ((vq - 1) % 64)) & 1)) > > + ((vector_lengths[(vq - KVM_ARM64_SVE_VQ_MIN) / 64] >> > > + ((vq - KVM_ARM64_SVE_VQ_MIN) % 64)) & 1)) > > /* Vector length vq * 16 bytes supported */ > > else > > /* Vector length vq * 16 bytes not supported */ > > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h > > index 2a04ef0..edd2db8 100644 > > --- a/arch/arm64/include/uapi/asm/kvm.h > > +++ b/arch/arm64/include/uapi/asm/kvm.h > > @@ -258,9 +258,14 @@ struct kvm_vcpu_events { > > KVM_REG_SIZE_U256 | \ > > ((i) & (KVM_ARM64_SVE_MAX_SLICES - 1))) > > > > +#define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN > > +#define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX > > + > > /* Vector lengths pseudo-register: */ > > #define KVM_REG_ARM64_SVE_VLS (KVM_REG_ARM64 | KVM_REG_ARM64_SVE | \ > > KVM_REG_SIZE_U512 | 0xffff) > > +#define KVM_ARM64_SVE_VLS_WORDS \ > > + ((KVM_ARM64_SVE_VQ_MAX - KVM_ARM64_SVE_VQ_MIN) / 64 + 1) > > > > /* Device Control API: ARM VGIC */ > > #define KVM_DEV_ARM_VGIC_GRP_ADDR 0 > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > > index f025a2f..5bb909c 100644 > > --- a/arch/arm64/kvm/guest.c > > +++ b/arch/arm64/kvm/guest.c > > @@ -208,10 +208,8 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64) > > #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64) > > > > -#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1) > > - > > static bool vq_present( > > - const u64 (*const vqs)[SVE_VLS_WORDS], > > + const u64 (*const vqs)[KVM_ARM64_SVE_VLS_WORDS], > > unsigned int vq) > > { > > return (*vqs)[vq_word(vq)] & vq_mask(vq); > > @@ -220,7 +218,7 @@ static bool vq_present( > > static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > { > > unsigned int max_vq, vq; > > - u64 vqs[SVE_VLS_WORDS]; > > + u64 vqs[KVM_ARM64_SVE_VLS_WORDS]; > > > > if (!vcpu_has_sve(vcpu)) > > return -ENOENT; > > @@ -244,7 +242,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) > > { > > unsigned int max_vq, vq; > > - u64 vqs[SVE_VLS_WORDS]; > > + u64 vqs[KVM_ARM64_SVE_VLS_WORDS]; > > > > if (!vcpu_has_sve(vcpu)) > > return -ENOENT; > > > > Yup > > Reviewed-by: Andrew Jones <drjones@redhat.com> OK, thanks ---Dave
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c index 73044e3..f025a2f 100644 --- a/arch/arm64/kvm/guest.c +++ b/arch/arm64/kvm/guest.c @@ -23,7 +23,6 @@ #include <linux/errno.h> #include <linux/err.h> #include <linux/nospec.h> -#include <linux/kernel.h> #include <linux/kvm_host.h> #include <linux/module.h> #include <linux/stddef.h> @@ -209,8 +208,10 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64) #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64) +#define SVE_VLS_WORDS (vq_word(SVE_VQ_MAX) + 1) + static bool vq_present( - const u64 (*const vqs)[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)], + const u64 (*const vqs)[SVE_VLS_WORDS], unsigned int vq) { return (*vqs)[vq_word(vq)] & vq_mask(vq); @@ -219,7 +220,7 @@ static bool vq_present( static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) { unsigned int max_vq, vq; - u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)]; + u64 vqs[SVE_VLS_WORDS]; if (!vcpu_has_sve(vcpu)) return -ENOENT; @@ -243,7 +244,7 @@ static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg) { unsigned int max_vq, vq; - u64 vqs[DIV_ROUND_UP(SVE_VQ_MAX - SVE_VQ_MIN + 1, 64)]; + u64 vqs[SVE_VLS_WORDS]; if (!vcpu_has_sve(vcpu)) return -ENOENT;
A complicated DIV_ROUND_UP() expression is currently written out explicitly in multiple places in order to specify the size of the bitmap exchanged with userspace to represent the value of the KVM_REG_ARM64_SVE_VLS pseudo-register. To make this more readable, this patch replaces these with a single define. Since the number of words in a bitmap is just the index of the last word used + 1, this patch expresses the bound that way instead. This should make it clearer what is being expressed. Since use of DIV_ROUND_UP() was the only reason for including <linux/kernel.h> in guest.c, this patch removes that #include. No functional change. Suggested-by: Andrew Jones <drjones@redhat.com> Signed-off-by: Dave Martin <Dave.Martin@arm.com> --- arch/arm64/kvm/guest.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)