[PATCH v3 22/28] arm64/sve: KVM: Prevent guests from using SVE

[Christoffer Dall]

On Wed, Oct 18, 2017 at 04:00:05PM +0100, Dave Martin wrote:
> On Wed, Oct 18, 2017 at 03:23:23PM +0200, Christoffer Dall wrote:
> > On Tue, Oct 17, 2017 at 03:31:42PM +0100, Dave Martin wrote:
> > > On Tue, Oct 17, 2017 at 01:50:24PM +0200, Christoffer Dall wrote:
> > > > On Tue, Oct 10, 2017 at 07:38:39PM +0100, Dave Martin wrote:
> > > > > Until KVM has full SVE support, guests must not be allowed to
> > > > > execute SVE instructions.
> > > > > 
> > > > > This patch enables the necessary traps, and also ensures that the
> > > > > traps are disabled again on exit from the guest so that the host
> > > > > can still use SVE if it wants to.
> > > > > 
> > > > > This patch introduces another instance of
> > > > > __this_cpu_write(fpsimd_last_state, NULL), so this flush operation
> > > > > is abstracted out as a separate helper fpsimd_flush_cpu_state().
> > > > > Other instances are ported appropriately.
> > > > 
> > > > I don't understand this paragraph, beginning from ", so this...".
> > > > 
> > > > 
> > > > From reading the code, what I think is the reason for having to flush
> > > > the SVE state (and mark the host state invalid) is that even though we
> > > > disallow SVE usage in the guest, the guest can use the normal FP state,
> > > > and while we always fully preserve the host state, this could still
> > > > corrupt some additional SVE state not properly preserved for the host.
> > > > Is that correct?
> > > 
> > > Yes, that's right: the guest can't touch the SVE-specific registers
> > > Pn/FFR, but FPSIMD accesses to Vn regs cause the high bits of the
> > > corresponding SVE Zn registers to be clobbered.  In any case, the
> > > FPSIMD restore done by KVM after guest exit is sufficient to clobber
> > > those bits even if the guest didn't do it.
> > > 
> > > This is a band-aid for not making the KVM world switch code properly
> > > SVE-aware yet.
> > > 
> > > Does the following wording sound better:
> > > 
> > > --8<--
> > > 
> > > On guest exit, high bits of the SVE Zn registers may have been
> > > clobbered as a side-effect the execution of FPSIMD instructions in
> > > the guest.  The existing KVM host FPSIMD restore code is not
> > > sufficient to restore these bits, so this patch explicitly marks
> > > the CPU as not containing cached vector state for any task, this
> > > forcing a reload on the next return to userspace.  This is an
> > > interim measure, in advance of adding full SVE awareness to KVM.
> > > 
> > > Because of the duplication of this operation
> > > (__this_cpu_write(fpsimd_last_state, NULL)), it is factored out as
> > 
> > s/it is/is/  (I think)
> > 
> > > a new helper fpsimd_flush_cpu_state() to make the purpose clearer.
> > 
> > Yes, the wording is good and helps a lot.  Thanks for writing that.
> > 
> 
> I think it "it is" is correct, but it's a pretty ghastly sentence...

Ah, I missed the comma, before and read it as __this_cpu_write... is
factored... but that doesn't make any sense.  Sorry, I was just not
paying proper attention.

> 
> I'll split it as:
> 
> This marking of cached vector state in the CPU as invalid is done using
> __this_cpu_write(fpsimd_last_state, NULL) in fpsimd.c.  Due to the
> repeated use of this rather obscure operation, it makes sense to factor
> it out as a separate helper with a clearer name.  This patch factors it
> out as fpsimd_flush_cpu_state().
> 

That's definitely clear.

> > Reviewed-by: Christoffer Dall <christoffer.dall at linaro.org>
> 
> I'll assume I can keep keep your Reviewed-by, since this change is just
> clarification.
> 
> But if you're not happy, please shout!
> 
I'm happy - in most aspects of life - indeed keep my reviewed-by.

Thanks,
-Christoffer