Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 11.1 \(3445.4.7\))
Subject: Re: [PATCH RFC] KVM: x86: tell guests if the exposed SMT topology is
 trustworthy
From:   Liran Alon <liran.alon@oracle.com>
In-Reply-To: <20191105161737.21395-1-vkuznets@redhat.com>
Date:   Tue, 5 Nov 2019 19:17:48 +0200
Cc:     kvm list <kvm@vger.kernel.org>, x86@kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        Jim Mattson <jmattson@google.com>,
        linux-kernel@vger.kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <83B55424-13A9-4395-98E8-466FFF4C698E@oracle.com>
References: <20191105161737.21395-1-vkuznets@redhat.com>
To:     Vitaly Kuznetsov <vkuznets@redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On 5 Nov 2019, at 18:17, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>=20
> 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=E2=80=99s also important for guest to know if it=E2=80=99s SMT =
topology is trustworthy for various optimisation algorithms.
E.g. Should it attempt to run tasks that share memory on same NUMA node?

>=20
> 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.

>=20
> 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.

>=20
> 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=E2=80=99t think KVM_FEATURE_TRUSTWORTHY_SMT should indicate to =
guest whether it=E2=80=99s vCPU shares a CPU core with another guest.
It should only expose to guest the fact that he can rely on it=E2=80=99s =
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=E2=80=99t guarantee to him that one of it=E2=80=99s vCPU shares a =
CPU core with another guest vCPU.
I don=E2=80=99t think we should have a CPUID bit that expose this =
information to guest.

>=20
> 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=E2=80=A6 I=E2=80=99m 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=E2=80=99t 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 =E2=80=9CNoNonArchitecturalCoreSharing=E2=80=9D.
=E2=80=9CNoNonArchitecturalCoreSharing=E2=80=9D guarantees to guest that =
vCPUs of guest won=E2=80=99t 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=E2=80=99s internal =
scheduling decisions.

-Liran

>=20
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> 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(-)
>=20
> 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.
>=20
> +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
>=20
> Where ``flag`` here is defined as below:
>=20
> -=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D =3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> -flag               value        meaning
> -=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D =3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> -KVM_HINTS_REALTIME 0            guest checks this feature bit to
> -                                determine that vCPUs are never
> -                                preempted for an unlimited time
> -                                allowing optimizations
> -=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D =3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D
> +flag                              value       meaning
> +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D
> +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.
> +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D
> 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
>=20
> #define KVM_HINTS_REALTIME      0
> +#define KVM_HINTS_TRUSTWORTHY_SMT	1
>=20
> /* 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);
>=20
> 		if (sched_info_on())
> 			entry->eax |=3D (1 << KVM_FEATURE_STEAL_TIME);
> @@ -720,6 +721,10 @@ static inline int __do_cpuid_func(struct =
kvm_cpuid_entry2 *entry, u32 function,
> 		entry->ebx =3D 0;
> 		entry->ecx =3D 0;
> 		entry->edx =3D 0;
> +
> +		if (!cpu_smt_possible())
> +			entry->edx |=3D (1 << =
KVM_HINTS_TRUSTWORTHY_SMT);
> +
> 		break;
> 	case 0x80000000:
> 		entry->eax =3D min(entry->eax, 0x8000001f);
> --=20
> 2.20.1
>=20