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@xxxxxxxxxx> > > 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 _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
