On Wed, Mar 27, 2019 at 03:21:02PM +0000, Julien Thierry wrote:
>
>
> On 27/03/2019 10:33, Dave Martin wrote:
> > On Wed, Mar 27, 2019 at 09:47:42AM +0000, Julien Thierry wrote:
> >> Hi Dave,
> >>
> >> On 19/03/2019 17:52, Dave Martin wrote:
[...]
> >>> +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.
OK, how about the following? This keeps things minimal, but should help
future maintainers know that something may need updating here in the
future.
Cheers
---Dave
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index ea5219d..086ab05 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -289,6 +289,13 @@ static int set_sve_vls(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 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 */
@@ -505,7 +512,7 @@ static int get_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
static unsigned long num_sve_regs(const struct kvm_vcpu *vcpu)
{
/* Only the first slice ever exists, for now */
- const unsigned int slices = 1;
+ const unsigned int slices = vcpu_sve_slices(vcpu);
if (!vcpu_has_sve(vcpu))
return 0;
@@ -521,7 +528,7 @@ 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 = 1;
+ const unsigned int slices = vcpu_sve_slices(vcpu);
u64 reg;
unsigned int i, n;
int num_regs = 0;
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
