On 4/16/2020 5:22 AM, Thomas Gleixner wrote:
Sean,
Sean Christopherson <sean.j.christopherson@xxxxxxxxx> writes:
On Wed, Apr 15, 2020 at 07:43:22PM +0200, Thomas Gleixner wrote:
Xiaoyao Li <xiaoyao.li@xxxxxxxxx> writes:
+static inline void vmx_update_sld(struct kvm_vcpu *vcpu, bool on)
+{
+ /*
+ * Toggle SLD if the guest wants it enabled but its been disabled for
+ * the userspace VMM, and vice versa. Note, TIF_SLD is true if SLD has
+ * been turned off. Yes, it's a terrible name.
Instead of writing that useless blurb you could have written a patch
which changes TIF_SLD to TIF_SLD_OFF to make it clear.
Hah, that's my comment, though I must admit I didn't fully intend for the
editorial at the end to get submitted upstream.
Anyways, I _did_ point out that TIF_SLD is a terrible name[1][2], and my
feedback got ignored/overlooked. I'd be more than happy to write a
patch, I didn't do so because I assumed that people wanted TIF_SLD as the name for
whatever reason.
I somehow missed that in the maze of mails regarding this stuff. I've
already written a patch to rename it to TIF_SLD_DISABLED which is pretty
self explaining. But see below.
[...]
@@ -1188,6 +1217,10 @@ void vmx_prepare_switch_to_guest(struct kvm_vcpu *vcpu)
#endif
vmx_set_host_fs_gs(host_state, fs_sel, gs_sel, fs_base, gs_base);
+
+ vmx->host_sld_on = !test_thread_flag(TIF_SLD);
This inverted storage is non-intuitive. What's wrong with simply
reflecting the TIF_SLD state?
So that the guest/host tracking use the same polairy, and IMO it makes
the restoration code more intuitive, e.g.:
vmx_update_sld(&vmx->vcpu, vmx->host_sld_on);
vs
vmx_update_sld(&vmx->vcpu, !vmx->host_tif_sld);
I.e. the inversion needs to happen somewhere.
Correct, but we can make it consistently use the 'disabled' convention.
I briefly thought about renaming the flag to TIF_SLD_ENABLED, set it by
default and update the 5 places where it is used. But that's
inconsistent as well simply because it does not make any sense to set
that flag when detection is not available or disabled on the command
line.
Assuming you'll pick TIF_SLD_DISABLED, I guess we need to set this flag
by default for the case SLD is no available or disabled on the command,
for consistency?
