Patchwork [09/14] KVM: arm64/sve: Simplify KVM_REG_ARM64_SVE_VLS array sizing

login
register
mail settings
Submitter Dave Martin
Date April 12, 2019, 4:28 p.m.
Message ID <1555086498-26691-10-git-send-email-Dave.Martin@arm.com>
Download mbox | patch
Permalink /patch/772025/
State New
Headers show

Comments

Dave Martin - April 12, 2019, 4:28 p.m.
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(-)
Andrew Jones - April 15, 2019, 3:20 p.m.
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>
Dave Martin - April 16, 2019, 12:41 p.m.
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
Andrew Jones - April 16, 2019, 1 p.m.
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
Dave Martin - April 16, 2019, 2:10 p.m.
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
Andrew Jones - April 16, 2019, 2:28 p.m.
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
Dave Martin - April 16, 2019, 3:36 p.m.
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

Patch

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;