diff mbox series

[v7,19/27] KVM: arm64: Enumerate SVE register indices for KVM_GET_REG_LIST

Message ID 1553864452-15080-20-git-send-email-Dave.Martin@arm.com (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: SVE guest support | expand

Commit Message

Dave Martin March 29, 2019, 1 p.m. UTC 
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>
Tested-by: zhang.lei <zhang.lei@jp.fujitsu.com>

---

Changes since v6:

 * [Julien Thierry] Add a #define to replace the magic "slices = 1",
   and add a comment explaining to maintainers what needs to happen if
   this is updated in the future.

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 | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

Comments

Andrew Jones April 4, 2019, 2:08 p.m. UTC | #1 
On Fri, Mar 29, 2019 at 01:00:44PM +0000, 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>
> Tested-by: zhang.lei <zhang.lei@jp.fujitsu.com>
> 
> ---
> 
> Changes since v6:
> 
>  * [Julien Thierry] Add a #define to replace the magic "slices = 1",
>    and add a comment explaining to maintainers what needs to happen if
>    this is updated in the future.
> 
> 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 | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 736d8cb..2aa80a5 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -222,6 +222,13 @@ static int set_core_reg(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

s/number/Number/
s/on vcpu//

> + * NOTE: If you are tempted to modify this, you must also to rework

s/to rework/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 */
> @@ -411,6 +418,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 */

I'd move this comment up into the one above vcpu_sve_slices(),
and then nothing needs to change here when more slices come.

> +	const unsigned int slices = vcpu_sve_slices(vcpu);
> +
> +	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 */

Same comment as above.

> +	const unsigned int slices = vcpu_sve_slices(vcpu);
> +	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++;
> +	}

nit: the extra blank lines above the num_regs++'s give the code an odd
     look (to me)

> +
> +	return num_regs;
> +}
> +
>  /**
>   * kvm_arm_num_regs - how many registers do we present via KVM_GET_ONE_REG
>   *
> @@ -421,6 +478,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 +500,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;

I know this if ret vs. if ret < 0 is being addressed already.

> +
>  	ret = kvm_arm_copy_fw_reg_indices(vcpu, uindices);
>  	if (ret)
>  		return ret;
> -- 
> 2.1.4
> 

Thanks,
drew
Dave Martin April 5, 2019, 9:35 a.m. UTC | #2 
On Thu, Apr 04, 2019 at 04:08:32PM +0200, Andrew Jones wrote:
> On Fri, Mar 29, 2019 at 01:00:44PM +0000, 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>
> > Tested-by: zhang.lei <zhang.lei@jp.fujitsu.com>
> > 
> > ---
> > 
> > Changes since v6:
> > 
> >  * [Julien Thierry] Add a #define to replace the magic "slices = 1",
> >    and add a comment explaining to maintainers what needs to happen if
> >    this is updated in the future.
> > 
> > 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 | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> > 
> > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > index 736d8cb..2aa80a5 100644
> > --- a/arch/arm64/kvm/guest.c
> > +++ b/arch/arm64/kvm/guest.c
> > @@ -222,6 +222,13 @@ static int set_core_reg(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
> 
> s/number/Number/

Not a sentence -> no capital letter.

Due to the adjacent note it does look a little odd though.  I'm happy to
change it.

> s/on vcpu//

Agreed, I can drop that.

> > + * NOTE: If you are tempted to modify this, you must also to rework
> 
> s/to rework/rework/

Ack

> > + * 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 */
> > @@ -411,6 +418,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 */
> 
> I'd move this comment up into the one above vcpu_sve_slices(),
> and then nothing needs to change here when more slices come.
> 
> > +	const unsigned int slices = vcpu_sve_slices(vcpu);
> > +
> > +	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 */
> 
> Same comment as above.

Fair point: this was to explain the magic "1" that was previously here,
but the comments are a bit redundant here now: better to move the
comments where the 1 itself went.

> > +	const unsigned int slices = vcpu_sve_slices(vcpu);
> > +	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++;
> > +	}
> 
> nit: the extra blank lines above the num_regs++'s give the code an odd
>      look (to me)

There's no guaranteed fall-through onto the increments: the blank line
was there to highlight the fact that we may jump out using a return
instead.

But I'm happy enough to change it if you have a strong preference or
you feel the code is equally clear without.

> 
> > +
> > +	return num_regs;
> > +}
> > +
> >  /**
> >   * kvm_arm_num_regs - how many registers do we present via KVM_GET_ONE_REG
> >   *
> > @@ -421,6 +478,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 +500,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;
> 
> I know this if ret vs. if ret < 0 is being addressed already.

Yes, Marc's patch in kvmarm/next should fix that.

Cheers
---Dave
Andrew Jones April 5, 2019, 9:45 a.m. UTC | #3 
On Fri, Apr 05, 2019 at 10:35:45AM +0100, Dave Martin wrote:
> On Thu, Apr 04, 2019 at 04:08:32PM +0200, Andrew Jones wrote:
> > On Fri, Mar 29, 2019 at 01:00:44PM +0000, 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>
> > > Tested-by: zhang.lei <zhang.lei@jp.fujitsu.com>
> > > 
> > > ---
> > > 
> > > Changes since v6:
> > > 
> > >  * [Julien Thierry] Add a #define to replace the magic "slices = 1",
> > >    and add a comment explaining to maintainers what needs to happen if
> > >    this is updated in the future.
> > > 
> > > 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 | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 63 insertions(+)
> > > 
> > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > > index 736d8cb..2aa80a5 100644
> > > --- a/arch/arm64/kvm/guest.c
> > > +++ b/arch/arm64/kvm/guest.c
> > > @@ -222,6 +222,13 @@ static int set_core_reg(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
> > 
> > s/number/Number/
> 
> Not a sentence -> no capital letter.
> 
> Due to the adjacent note it does look a little odd though.  I'm happy to
> change it.
> 
> > s/on vcpu//
> 
> Agreed, I can drop that.
> 
> > > + * NOTE: If you are tempted to modify this, you must also to rework
> > 
> > s/to rework/rework/
> 
> Ack
> 
> > > + * 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 */
> > > @@ -411,6 +418,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 */
> > 
> > I'd move this comment up into the one above vcpu_sve_slices(),
> > and then nothing needs to change here when more slices come.
> > 
> > > +	const unsigned int slices = vcpu_sve_slices(vcpu);
> > > +
> > > +	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 */
> > 
> > Same comment as above.
> 
> Fair point: this was to explain the magic "1" that was previously here,
> but the comments are a bit redundant here now: better to move the
> comments where the 1 itself went.
> 
> > > +	const unsigned int slices = vcpu_sve_slices(vcpu);
> > > +	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++;
> > > +	}
> > 
> > nit: the extra blank lines above the num_regs++'s give the code an odd
> >      look (to me)
> 
> There's no guaranteed fall-through onto the increments: the blank line
> was there to highlight the fact that we may jump out using a return
> instead.
> 
> But I'm happy enough to change it if you have a strong preference or
> you feel the code is equally clear without.

It's just a nit, so I don't have a strong preference :)

> 
> > 
> > > +
> > > +	return num_regs;
> > > +}
> > > +
> > >  /**
> > >   * kvm_arm_num_regs - how many registers do we present via KVM_GET_ONE_REG
> > >   *
> > > @@ -421,6 +478,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 +500,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;
> > 
> > I know this if ret vs. if ret < 0 is being addressed already.
> 
> Yes, Marc's patch in kvmarm/next should fix that.
> 
> Cheers
> ---Dave

Thanks,
drew
Dave Martin April 5, 2019, 11:11 a.m. UTC | #4 
On Fri, Apr 05, 2019 at 11:45:56AM +0200, Andrew Jones wrote:
> On Fri, Apr 05, 2019 at 10:35:45AM +0100, Dave Martin wrote:
> > On Thu, Apr 04, 2019 at 04:08:32PM +0200, Andrew Jones wrote:
> > > On Fri, Mar 29, 2019 at 01:00:44PM +0000, 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>
> > > > Tested-by: zhang.lei <zhang.lei@jp.fujitsu.com>
> > > > 
> > > > ---

[...]

> > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c

[...]

> > > > +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 = vcpu_sve_slices(vcpu);
> > > > +	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++;
> > > > +	}
> > > 
> > > nit: the extra blank lines above the num_regs++'s give the code an odd
> > >      look (to me)
> > 
> > There's no guaranteed fall-through onto the increments: the blank line
> > was there to highlight the fact that we may jump out using a return
> > instead.
> > 
> > But I'm happy enough to change it if you have a strong preference or
> > you feel the code is equally clear without.
> 
> It's just a nit, so I don't have a strong preference :)

OK, well given that you mentioned it and there are other tweaks to make
on top of this patch anyway, I'll go ahead and make the change.

This saves a few lines, if nothing else.

Cheers
---Dave
diff mbox series

Patch

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 736d8cb..2aa80a5 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -222,6 +222,13 @@  static int set_core_reg(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 to 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 */
@@ -411,6 +418,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 = vcpu_sve_slices(vcpu);
+
+	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 = vcpu_sve_slices(vcpu);
+	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 +478,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 +500,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;