On 27/03/2019 10:33, Dave Martin wrote:

On Wed, Mar 27, 2019 at 09:47:42AM +0000, Julien Thierry wrote:

quoted

Hi Dave,

On 19/03/2019 17:52, Dave Martin wrote:

quoted

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 <redacted>

Thanks, although...

quoted

quoted

---

Changes since v5:

(Dropped Julien Thierry's Reviewed-by due to non-trivial rebasing)

 * Move mis-split reword to prevent put_user()s being accidentally the
   correct size from KVM: arm64/sve: Add pseudo-register for the guest's
   vector lengths.
---
 arch/arm64/kvm/guest.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 736d8cb..585c31e5 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c

@@ -411,6 +411,56 @@ static int get_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	return copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)) ? -EFAULT : 0;
 }
 
+static unsigned long num_sve_regs(const struct kvm_vcpu *vcpu)
+{
+	/* Only the first slice ever exists, for now */
+	const unsigned int slices = 1;

Nit: Might be worth introducing a macro/inline function for the number
of slices supported. This way, the day we need to change that, we only
need to look for that identifier.

... Reasonable point, but I wanted to avoid inventing anything
prematurely, partly because sve_reg_to_region() will need work in order
to support multiple slices (though it's not rocket science).

I could introduce something like the following:

static unsigned int sve_num_slices(const struct kvm_vcpu *vcpu)
{
	unsigned int slice_size = KVM_REG_SIZE(KVM_REG_ARM64_SVE_ZREG(0, 0));
	unsigned int slices = DIV_ROUND_UP(vcpu->arch.sve_max_vl, slice_size);

	/*
	 * For now, the SVE register ioctl access code won't work
	 * properly with multiple register slices.  KVM should prevent
	 * configuration of a vcpu with a maximum vector length large
	 * enough to trigger this:
	 */
	if (WARN_ON_ONCE(slices > 1))
		return 1;

	return slices;
}

This may be clearer, but felt a bit like overkill...

Thoughts?

Seems a bit overkill yes... I was more thinking of a define and the
person in charge of adding the slice support would just need to look for
references to that define to know (some of) the places that would need
rework/review.

So, unless someone else thinks it's good to introduce it right now you
can ignore that.

Thanks,

-- 
Julien Thierry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel