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:

quoted

On Tue, Oct 17, 2017 at 03:31:42PM +0100, Dave Martin wrote:

quoted

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)

quoted

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.

quoted

Reviewed-by: Christoffer Dall <redacted>

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