On 2/5/19 15:43, Jan Beulich wrote: >>>> On 05.02.19 at 15:23, <nmant...@amazon.de> wrote: >> On 1/31/19 17:35, Jan Beulich wrote: >>>>>> On 29.01.19 at 15:43, <nmant...@amazon.de> wrote: >>>> @@ -1942,6 +1942,12 @@ Irrespective of Xen's setting, the feature is >> virtualised for HVM guests to >>>> use. By default, Xen will enable this mitigation on hardware believed to >>>> be >>>> vulnerable to L1TF. >>>> >>>> +On hardware vulnerable to L1TF, the `l1tf-barrier=` option can be used to >>>> force >>>> +or prevent Xen from protecting evaluations inside the hypervisor with a >>>> barrier >>>> +instruction to not load potentially secret information into L1 cache. By >>>> +default, Xen will enable this mitigation on hardware believed to be >>>> vulnerable >>>> +to L1TF. >>> ... and having SMT enabled, since aiui this is a non-issue without. >> In case flushing the L1 cache is not enabled, that is still an issue, >> because the transition guest -> hypervisor -> guest would allow to >> retrieve hypervisor data from the cache still. Do you want me to extend >> the logic to consider L1 cache flushing as well? > Well, I wouldn't be overly concerned of people disabling it from the > command line, but being kind to people without updated microcode > is perhaps a good idea. I will extend the commit message to state that this the CPU flag is set automatically independently of SMT and cache flushing. > >>>> @@ -100,6 +102,7 @@ static int __init parse_spec_ctrl(const char *s) >>>> opt_ibpb = false; >>>> opt_ssbd = false; >>>> opt_l1d_flush = 0; >>>> + opt_l1tf_barrier = 0; >>>> } >>>> else if ( val > 0 ) >>>> rc = -EINVAL; >>> Is this really something we want "spec-ctrl=no-xen" to disable? >>> It would seem to me that this should be restricted to "spec-ctrl=no". >> I have no strong opinion here. If you ask me to move it somewhere else, >> I will do that. I just want to make sure it's disable in case >> speculation mitigations should be disabled. > Unless anyone else voices a different opinion, I'd like to see it > moved as suggested. I will move the change above the disable_common label. >>>> @@ -843,6 +849,14 @@ void __init init_speculation_mitigations(void) >>>> opt_l1d_flush = cpu_has_bug_l1tf && !(caps & >>>> ARCH_CAPS_SKIP_L1DFL); >>>> >>>> /* >>>> + * By default, enable L1TF_VULN on L1TF-vulnerable hardware >>>> + */ >>> This ought to be a single line comment. >> Will fix. >>>> + if ( opt_l1tf_barrier == -1 ) >>>> + opt_l1tf_barrier = cpu_has_bug_l1tf; >>> At the very least opt_smt should be taken into account here. But >>> I guess this setting of the default may need to be deferred >>> further, until the topology of the system is known (there may >>> not be any hyperthreads after all). >> Again, cache flushing also has to be considered. So, I would like to >> keep it like this for now. > With the "for now" aspect properly explained in the description, > I guess that would be fine as a first step. I will extend the commit message accordingly. > >>>> + if ( cpu_has_bug_l1tf && opt_l1tf_barrier > 0) >>>> + setup_force_cpu_cap(X86_FEATURE_SC_L1TF_VULN); >>> Why the left side of the &&? >> IMHO, the CPU flag L1TF should only be set when the CPU is reported to >> be vulnerable, even if the command line wants to enforce mitigations. > What's the command line option good for if it doesn't trigger > patching in of the LFENCEs? Command line options exist, among > other purposes, to aid mitigating flaws in our determination of > what is a vulnerable platform. I will remove the extra conditional and enable patching based on the command line only. > >>>> + /* >>>> * We do not disable HT by default on affected hardware. >>>> * >>>> * Firstly, if the user intends to use exclusively PV, or HVM shadow >>> Furthermore, as per the comment and logic here and below a >>> !HVM configuration ought to be safe too, unless "pv-l1tf=" was >>> used (in which case we defer to the admin anyway), so it's >>> questionable whether the whole logic should be there in the >>> first place in this case. This would then in particular keep all >>> of this out for the PV shim. >> For the PV shim, I could add pv-shim to my check before enabling the CPU >> flag. > But the PV shim is just a special case. I'd like this code to be > compiled out for all !HVM configurations.
The that introduces the evaluate_nospec macro does that already. Based on defined(CONFIG_HVM) lfence patching is disabled there. Do you want me to wrap this command line option into CONFIG_HVM checks as well? Best, Norbert Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich Ust-ID: DE 289 237 879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
