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

[Dave Martin <Dave.Martin@arm.com>]

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