On Thu, Apr 04, 2019 at 06:25:39PM +0200, Andrew Jones wrote:
> On Thu, Apr 04, 2019 at 03:50:56PM +0100, Dave Martin wrote:
> > On Thu, Apr 04, 2019 at 03:57:06PM +0200, Andrew Jones wrote:
> > > On Fri, Mar 29, 2019 at 01:00:43PM +0000, Dave Martin wrote:
> > > > This patch adds the following registers for access via the
> > > > KVM_{GET,SET}_ONE_REG interface:
> > > >
> > > > * KVM_REG_ARM64_SVE_ZREG(n, i) (n = 0..31) (in 2048-bit slices)
> > > > * KVM_REG_ARM64_SVE_PREG(n, i) (n = 0..15) (in 256-bit slices)
> > > > * KVM_REG_ARM64_SVE_FFR(i) (in 256-bit slices)
> > > >
> > > > In order to adapt gracefully to future architectural extensions,
> > > > the registers are logically divided up into slices as noted above:
> > > > the i parameter denotes the slice index.
> > > >
> > > > This allows us to reserve space in the ABI for future expansion of
> > > > these registers. However, as of today the architecture does not
> > > > permit registers to be larger than a single slice, so no code is
> > > > needed in the kernel to expose additional slices, for now. The
> > > > code can be extended later as needed to expose them up to a maximum
> > > > of 32 slices (as carved out in the architecture itself) if they
> > > > really exist someday.
> > > >
> > > > The registers are only visible for vcpus that have SVE enabled.
> > > > They are not enumerated by KVM_GET_REG_LIST on vcpus that do not
> > > > have SVE.
> > > >
> > > > Accesses to the FPSIMD registers via KVM_REG_ARM_CORE is not
> > > > allowed for SVE-enabled vcpus: SVE-aware userspace can use the
> > > > KVM_REG_ARM64_SVE_ZREG() interface instead to access the same
> > > > register state. This avoids some complex and pointless emulation
> > > > in the kernel to convert between the two views of these aliased
> > > > registers.
> > > >
> > > > Signed-off-by: Dave Martin <Dave.Martin@xxxxxxx>
> > > > Reviewed-by: Julien Thierry <julien.thierry@xxxxxxx>
> > > > Tested-by: zhang.lei <zhang.lei@xxxxxxxxxxxxxx>
[...]
> > > > +#define KVM_REG_ARM64_SVE_FFR(i) KVM_REG_ARM64_SVE_PREG(16, i)
> > >
> > > Since this is user api and a user may want to construct their own register
> > > indices, then shouldn't we instead provide KVM_REG_ARM64_SVE_FFR_BASE as
> > >
> > > #define KVM_REG_ARM64_SVE_FFR_BASE KVM_REG_ARM64_SVE_PREG_BASE | (16 << 5)
> >
> > Can do, or just
> >
> > #define KVM_REG_ARM64_SVE_FFR_BASE KVM_REG_ARM64_SVE_PREG(0, 0)
>
> I don't see how this would work for an FFR base.
Err yes, scratch that. But I'm happy to have it, however defined.
[...]
> > > > +/* Get sanitised bounds for user/kernel SVE register copy */
> > > > +static int sve_reg_to_region(struct sve_state_reg_region *region,
> > > > + struct kvm_vcpu *vcpu,
> > > > + const struct kvm_one_reg *reg)
> > > > +{
[...]
> > > > + sve_state_size = vcpu_sve_state_size(vcpu);
> > > > + if (!sve_state_size)
> > > > + return -EINVAL;
> > > > +
> > > > + region->koffset = array_index_nospec(reqoffset, sve_state_size);
> > > > + region->klen = min(maxlen, reqlen);
> > > > + region->upad = reqlen - region->klen;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int get_sve_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > +{
> > > > + struct sve_state_reg_region region;
> > > > + char __user *uptr = (char __user *)reg->addr;
> > > > +
> > > > + if (!vcpu_has_sve(vcpu) || sve_reg_to_region(®ion, vcpu, reg))
> > > > + return -ENOENT;
> > >
> > > sve_reg_to_region() can return EINVAL, but here it would get changed to
> > > ENOENT.
> >
> > Hmm, I'd say the affected code in sve_reg_to_region() should really be
> > a WARN_ON(): we're not supposed to hit it because we can't get here
> > until the vcpu is finalized. It's really just a defensive check before
> > dividing by some potentially invalid value. In such a case, it's
> > reasonable to have that EINVAL show through to userspace.
>
> Adding the WARN_ON is a good idea. The thing is that the EINVAL is *not*
> going to show through to userspace. ENOENT will. Which might be fine,
> but if so, then it would clear things up to just return ENOENT in
> sve_reg_to_region() as well.
I meant that we can propagate the actual return value back.
It might be better just to merge the vcpu_has_sve() check into sve_reg_to_region(), and simply have
int ret;
ret = sve_reg_to_region(...);
if (ret)
return ret;
here.
Currently we return -ENOENT for a non-SVE-enabled vcpu, even if the reg
ID is complete garbage. It would probably be useful to tidy that up at
the same time: -EINVAL would probably be more appropriate for such
cases.
[...]
> > > > int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
> > > > {
> > > > return -EINVAL;
> > > > @@ -346,12 +461,12 @@ int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > if ((reg->id & ~KVM_REG_SIZE_MASK) >> 32 != KVM_REG_ARM64 >> 32)
> > > > return -EINVAL;
> > > >
> > > > - /* Register group 16 means we want a core register. */
> > > > - if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
> > > > - return get_core_reg(vcpu, reg);
> > > > -
> > > > - if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_FW)
> > > > - return kvm_arm_get_fw_reg(vcpu, reg);
> > > > + switch (reg->id & KVM_REG_ARM_COPROC_MASK) {
> > > > + case KVM_REG_ARM_CORE: return get_core_reg(vcpu, reg);
> > > > + case KVM_REG_ARM_FW: return kvm_arm_get_fw_reg(vcpu, reg);
> > > > + case KVM_REG_ARM64_SVE: return get_sve_reg(vcpu, reg);
> > > > + default: break; /* fall through */
> > >
> > > This case has a 'break', so it's not a 'fall through'. Do we require
> > > default cases even when they're unused? If not, why have it?
> >
> > My reason for having that was to highlight that we fall through to the
> > code following the switch only in this case, because the other cases
> > all consist of return statements.
>
> I think it's pretty clear from the 'case,return' pattern what's going on
> and the default case isn't needed at all. And since the fall through
> comment is typically used to document why there is not a break, then
> having both looks weird.
Sure, I'm more than happy to remove the redundant default case if you
feel its presence is confusing rather than helpful.
I didn't want it to look like the switch() was supposed to be exhaustive,
but the presence of code after it _should_ make that obvious.
> >
> > > > + }
> > > >
> > > > if (is_timer_reg(reg->id))
> > > > return get_timer_reg(vcpu, reg);
> > > > @@ -365,12 +480,12 @@ int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > if ((reg->id & ~KVM_REG_SIZE_MASK) >> 32 != KVM_REG_ARM64 >> 32)
> > > > return -EINVAL;
> > > >
> > > > - /* Register group 16 means we set a core register. */
> > > > - if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_CORE)
> > > > - return set_core_reg(vcpu, reg);
> > > > -
> > > > - if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_FW)
> > > > - return kvm_arm_set_fw_reg(vcpu, reg);
> > > > + switch (reg->id & KVM_REG_ARM_COPROC_MASK) {
> > > > + case KVM_REG_ARM_CORE: return set_core_reg(vcpu, reg);
> > > > + case KVM_REG_ARM_FW: return kvm_arm_set_fw_reg(vcpu, reg);
> > > > + case KVM_REG_ARM64_SVE: return set_sve_reg(vcpu, reg);
> > > > + default: break; /* fall through */
> > >
> > > Same as above.
> >
> > I could move the trailing code into the default case, but that felt a
> > bit ugly.
> >
> > Thoughts?
>
> I'd remove the default case :)
OK
Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
