On Tue, Nov 5, 2019 at 9:32 AM Liran Alon <liran.alon@xxxxxxxxxx> wrote: > > > > > On 5 Nov 2019, at 19:17, Liran Alon <liran.alon@xxxxxxxxxx> wrote: > > > > > > > >> On 5 Nov 2019, at 18:17, Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote: > >> > >> Virtualized guests may pick a different strategy to mitigate hardware > >> vulnerabilities when it comes to hyper-threading: disable SMT completely, > >> use core scheduling, or, for example, opt in for STIBP. Making the > >> decision, however, requires an extra bit of information which is currently > >> missing: does the topology the guest see match hardware or if it is 'fake' > >> and two vCPUs which look like different cores from guest's perspective can > >> actually be scheduled on the same physical core. Disabling SMT or doing > >> core scheduling only makes sense when the topology is trustworthy. > > > > This is not only related to vulnerability mitigations. > > It’s also important for guest to know if it’s SMT topology is trustworthy for various optimisation algorithms. > > E.g. Should it attempt to run tasks that share memory on same NUMA node? > > > >> > >> Add two feature bits to KVM: KVM_FEATURE_TRUSTWORTHY_SMT with the meaning > >> that KVM_HINTS_TRUSTWORTHY_SMT bit answers the question if the exposed SMT > >> topology is actually trustworthy. It would, of course, be possible to get > >> away with a single bit (e.g. 'KVM_FEATURE_FAKE_SMT') and not lose backwards > >> compatibility but the current approach looks more straightforward. > > > > Agree. > > > >> > >> There were some offline discussions on whether this new feature bit should > >> be complemented with a 're-enlightenment' mechanism for live migration (so > >> it can change in guest's lifetime) but it doesn't seem to be very > >> practical: what a sane guest is supposed to do if it's told that SMT > >> topology is about to become fake other than kill itself? Also, it seems to > >> make little sense to do e.g. CPU pinning on the source but not on the > >> destination. > > > > Agree. > > > >> > >> There is also one additional piece of the information missing. A VM can be > >> sharing physical cores with other VMs (or other userspace tasks on the > >> host) so does KVM_FEATURE_TRUSTWORTHY_SMT imply that it's not the case or > >> not? It is unclear if this changes anything and can probably be left out > >> of scope (just don't do that). > > > > I don’t think KVM_FEATURE_TRUSTWORTHY_SMT should indicate to guest whether it’s vCPU shares a CPU core with another guest. > > It should only expose to guest the fact that he can rely on it’s virtual SMT topology. i.e. That there is a relation between virtual SMT topology > > to which physical logical processors run which vCPUs. > > > > Guest have nothing to do with the fact that he is now aware host doesn’t guarantee to him that one of it’s vCPU shares a CPU core with another guest vCPU. > > I don’t think we should have a CPUID bit that expose this information to guest. > > > >> > >> Similar to the already existent 'NoNonArchitecturalCoreSharing' Hyper-V > >> enlightenment, the default value of KVM_HINTS_TRUSTWORTHY_SMT is set to > >> !cpu_smt_possible(). KVM userspace is thus supposed to pass it to guest's > >> CPUIDs in case it is '1' (meaning no SMT on the host at all) or do some > >> extra work (like CPU pinning and exposing the correct topology) before > >> passing '1' to the guest. > > > > Hmm… I’m not sure this is correct. > > For example, it is possible to expose in virtual SMT topology that guest have 2 vCPUs running on single NUMA node, > > while in reality each vCPU task can be scheduled to run on different NUMA nodes. Therefore, making virtual SMT topology not trustworthy. > > i.e. Disabling SMT on host doesn’t mean that virtual SMT topology is reliable. > > > > I think this CPUID bit should just be set from userspace when admin have guaranteed to guest that it have set vCPU task affinity properly. > > Without KVM attempting to set this bit by itself. > > > > Note that we defined above KVM_HINTS_TRUSTWORTHY_SMT bit differently than “NoNonArchitecturalCoreSharing”. > > “NoNonArchitecturalCoreSharing” guarantees to guest that vCPUs of guest won’t share a physical CPU core unless they are defined as virtual SMT siblings. > > In contrast, KVM_HINTS_TRUSTWORTHY_SMT bit attempts to state that virtual SMT topology is a subset of how vCPUs are scheduled on physical SMT topology. > > i.e. It seems that Hyper-V bit is indeed only attempting to provide guest information related to security mitigations. While newly proposed KVM bit attempts to also > > assist guest to determine how to perform it’s internal scheduling decisions. > > > > -Liran > > Oh I later saw below that you defined KVM_HINTS_TRUSTWORTHY_SMT indeed as Microsoft defined “NoNonArchitecturalCoreSharing”. > If you plan to go with this direction, than I suggest renaming to similar name as Hyper-V. > But I think having a general vSMT topology is trustworthy is also useful. > Maybe we should have separate bits for each. And perhaps a bit each for "vCCX topology is trustworthy" and "vNUMA topology is trustworthy"? > -Liran > > > > >> > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> > >> --- > >> Documentation/virt/kvm/cpuid.rst | 27 +++++++++++++++++++-------- > >> arch/x86/include/uapi/asm/kvm_para.h | 2 ++ > >> arch/x86/kvm/cpuid.c | 7 ++++++- > >> 3 files changed, 27 insertions(+), 9 deletions(-) > >> > >> diff --git a/Documentation/virt/kvm/cpuid.rst b/Documentation/virt/kvm/cpuid.rst > >> index 01b081f6e7ea..64b94103fc90 100644 > >> --- a/Documentation/virt/kvm/cpuid.rst > >> +++ b/Documentation/virt/kvm/cpuid.rst > >> @@ -86,6 +86,10 @@ KVM_FEATURE_PV_SCHED_YIELD 13 guest checks this feature bit > >> before using paravirtualized > >> sched yield. > >> > >> +KVM_FEATURE_TRUSTWORTHY_SMT 14 set when host supports 'SMT > >> + topology is trustworthy' hint > >> + (KVM_HINTS_TRUSTWORTHY_SMT). > >> + > >> KVM_FEATURE_CLOCSOURCE_STABLE_BIT 24 host will warn if no guest-side > >> per-cpu warps are expeced in > >> kvmclock > >> @@ -97,11 +101,18 @@ KVM_FEATURE_CLOCSOURCE_STABLE_BIT 24 host will warn if no guest-side > >> > >> Where ``flag`` here is defined as below: > >> > >> -================== ============ ================================= > >> -flag value meaning > >> -================== ============ ================================= > >> -KVM_HINTS_REALTIME 0 guest checks this feature bit to > >> - determine that vCPUs are never > >> - preempted for an unlimited time > >> - allowing optimizations > >> -================== ============ ================================= > >> +================================= =========== ================================= > >> +flag value meaning > >> +================================= =========== ================================= > >> +KVM_HINTS_REALTIME 0 guest checks this feature bit to > >> + determine that vCPUs are never > >> + preempted for an unlimited time > >> + allowing optimizations > >> + > >> +KVM_HINTS_TRUSTWORTHY_SMT 1 the bit is set when the exposed > >> + SMT topology is trustworthy, this > >> + means that two guest vCPUs will > >> + never share a physical core > >> + unless they are exposed as SMT > >> + threads. > >> +================================= =========== ================================= > >> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h > >> index 2a8e0b6b9805..183239d5dfad 100644 > >> --- a/arch/x86/include/uapi/asm/kvm_para.h > >> +++ b/arch/x86/include/uapi/asm/kvm_para.h > >> @@ -31,8 +31,10 @@ > >> #define KVM_FEATURE_PV_SEND_IPI 11 > >> #define KVM_FEATURE_POLL_CONTROL 12 > >> #define KVM_FEATURE_PV_SCHED_YIELD 13 > >> +#define KVM_FEATURE_TRUSTWORTHY_SMT 14 > >> > >> #define KVM_HINTS_REALTIME 0 > >> +#define KVM_HINTS_TRUSTWORTHY_SMT 1 > >> > >> /* The last 8 bits are used to indicate how to interpret the flags field > >> * in pvclock structure. If no bits are set, all flags are ignored. > >> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > >> index f68c0c753c38..dab527a7081f 100644 > >> --- a/arch/x86/kvm/cpuid.c > >> +++ b/arch/x86/kvm/cpuid.c > >> @@ -712,7 +712,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function, > >> (1 << KVM_FEATURE_ASYNC_PF_VMEXIT) | > >> (1 << KVM_FEATURE_PV_SEND_IPI) | > >> (1 << KVM_FEATURE_POLL_CONTROL) | > >> - (1 << KVM_FEATURE_PV_SCHED_YIELD); > >> + (1 << KVM_FEATURE_PV_SCHED_YIELD) | > >> + (1 << KVM_FEATURE_TRUSTWORTHY_SMT); > >> > >> if (sched_info_on()) > >> entry->eax |= (1 << KVM_FEATURE_STEAL_TIME); > >> @@ -720,6 +721,10 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function, > >> entry->ebx = 0; > >> entry->ecx = 0; > >> entry->edx = 0; > >> + > >> + if (!cpu_smt_possible()) > >> + entry->edx |= (1 << KVM_HINTS_TRUSTWORTHY_SMT); > >> + > >> break; > >> case 0x80000000: > >> entry->eax = min(entry->eax, 0x8000001f); > >> -- > >> 2.20.1 > >> > > >
