On Thu, Apr 04, 2019 at 09:06:58AM +0100, Dave Martin wrote:
> On Wed, Apr 03, 2019 at 09:39:43PM +0200, Andrew Jones wrote:
> > On Fri, Mar 29, 2019 at 01:00:37PM +0000, Dave Martin wrote:
> > > This patch adds the necessary support for context switching ZCR_EL1
> > > for each vcpu.
> > > 
> > > ZCR_EL1 is trapped alongside the FPSIMD/SVE registers, so it makes
> > > sense for it to be handled as part of the guest FPSIMD/SVE context
> > > for context switch purposes instead of handling it as a general
> > > system register.  This means that it can be switched in lazily at
> > > the appropriate time.  No effort is made to track host context for
> > > this register, since SVE requires VHE: thus the hosts's value for
> > > this register lives permanently in ZCR_EL2 and does not alias the
> > > guest's value at any time.
> > > 
> > > The Hyp switch and fpsimd context handling code is extended
> > > appropriately.
> > > 
> > > Accessors are added in sys_regs.c to expose the SVE system
> > > registers and ID register fields.  Because these need to be
> > > conditionally visible based on the guest configuration, they are
> > > implemented separately for now rather than by use of the generic
> > > system register helpers.  This may be abstracted better later on
> > > when/if there are more features requiring this model.
> > > 
> > > ID_AA64ZFR0_EL1 is RO-RAZ for MRS/MSR when SVE is disabled for the
> > > guest, but for compatibility with non-SVE aware KVM implementations
> > > the register should not be enumerated at all for KVM_GET_REG_LIST
> > > in this case.  For consistency we also reject ioctl access to the
> > > register.  This ensures that a non-SVE-enabled guest looks the same
> > > to userspace, irrespective of whether the kernel KVM implementation
> > > supports SVE.
> > > 
> > > Signed-off-by: Dave Martin <dave.mar...@arm.com>
> > > Reviewed-by: Julien Thierry <julien.thie...@arm.com>
> > > Tested-by: zhang.lei <zhang....@jp.fujitsu.com>
> > > 
> > > ---
> > > 
> > > Changes since v5:
> > > 
> > >  * Port to the renamed visibility() framework.
> > > 
> > >  * Swap visiblity() helpers so that they appear by the relevant accessor
> > >    functions.
> > > 
> > >  * [Julien Grall] With the visibility() checks, {get,set}_zcr_el1()
> > >    degenerate to doing exactly what the common code does, so drop them.
> > > 
> > >    The ID_AA64ZFR0_EL1 handlers are still needed to provide contitional
> > >    RAZ behaviour.  This could be moved to the common code too, but since
> > >    this is a one-off case I don't do this for now.  We can address this
> > >    later if other regs need to follow the same pattern.
> > > 
> > >  * [Julien Thierry] Reset ZCR_EL1 to a fixed value using reset_val
> > >    instead of using relying on reset_unknown() honouring set bits in val
> > >    as RES0.
> > > 
> > >    Most of the bits in ZCR_EL1 are RES0 anyway, and many implementations
> > >    of SVE will support larger vectors than 128 bits, so 0 seems as good
> > >    a value as any to expose guests that forget to initialise this
> > >    register properly.
> > > ---
> 
> [...]
> 
> > > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> > > index 3563fe6..9d46066 100644
> > > --- a/arch/arm64/kvm/hyp/switch.c
> > > +++ b/arch/arm64/kvm/hyp/switch.c
> 
> [...]
> 
> > > +static int get_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> > > +         const struct sys_reg_desc *rd,
> > > +         const struct kvm_one_reg *reg, void __user *uaddr)
> > > +{
> > > + u64 val;
> > > +
> > > + if (!vcpu_has_sve(vcpu))
> > > +         return -ENOENT;
> > 
> > This shouldn't be necessary. The visibility check in
> > kvm_arm_sys_reg_get_reg already covers it.
> > 
> > > +
> > > + val = guest_id_aa64zfr0_el1(vcpu);
> > > + return reg_to_user(uaddr, &val, reg->id);
> > > +}
> > > +
> > > +static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> > > +         const struct sys_reg_desc *rd,
> > > +         const struct kvm_one_reg *reg, void __user *uaddr)
> > > +{
> > > + const u64 id = sys_reg_to_index(rd);
> > > + int err;
> > > + u64 val;
> > > +
> > > + if (!vcpu_has_sve(vcpu))
> > > +         return -ENOENT;
> > 
> > Also not necessary.
> 
> Hmm, true.  Because the logic is a bit spread out I felt uneasy with
> simply deleting these checks, but if they fire, something has
> definitely gone wrong elsewhere.
> 
> In its current form the code makes it look like it could be legitimate
> to get here with !vcpu_has_sve(vcpu), which is misleading.
> 
> What if we demote these to WARN_ON()?  This isn't a fast path.

A WARN_ON sounds good to me.

Thanks,
drew
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Reply via email to