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:

quoted

On Tue, Oct 10, 2017 at 07:38:39PM +0100, Dave Martin wrote:

quoted

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.

-->8--

quoted

quoted

As a side effect of this refactoring, a this_cpu_write() in
fpsimd_cpu_pm_notifier() is changed to __this_cpu_write().  This
should be fine, since cpu_pm_enter() is supposed to be called only
with interrupts disabled.

Otherwise the patch itself looks good to me.

Thanks, let me know about the above wording change though.

Yes, the wording is good and helps a lot.  Thanks for writing that.

Reviewed-by: Christoffer Dall <redacted>