On Tue, Jan 9, 2018 at 1:16 AM, Wei Wang <wei.w.wang@xxxxxxxxx> wrote: > On 01/09/2018 04:43 PM, Liran Alon wrote: >> >> ----- wei.w.wang@xxxxxxxxx wrote: >> >>> This patch shows an alternative approach to the one posted here: >>> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mail-2Darchive.com_linux-2Dkernel-40vger.kernel.org_msg1580364.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=o_uh0-IfLdLMwf2MaOOQVKtZbPlJ-fDVmglRj277cwk&s=j1oGOoYysZhUuGuuvuwdTU_OO7bv1ysIyvDtAlc6C-c&e= >>> >>> The advantages are >>> 1) Simpler; >>> 2) More reasonable because this is used to fill the hardware security >>> hole, for all the x86 cpus that physically support the two CPUIDs, >>> which means the hole already exists physically. All the VMs should >>> use this feature no matter what CPU model they are using. So, >> >> I'm not sure I 100% agree with this. >> There should be a way for the userspace agent to disable these CPUIDs if >> wanted. >> You don't want to lose the ability to expose a mimic of a real physical >> CPU-model of core2duo that >> doesn't have these CPUIDs. A good solution can be that these features will >> be exposed by default to guests >> if available on hardware but can still be explicitly not-exposed if >> userspace agent wishes so. > > > I think the case we are handling here is different: > It shouldn't be treated as a regular feature (e.g. xsaves) that a user can > choose to use or not. It is a security hole (or say a bug). When we fixed a > bug, we don't give users an option to select to trigger the bug, right? Due to the performance impact on the neighboring hyperthread, one may not want to expose SPEC_CTRL to all VMs. >> The only weird side-effect of this is that live-migration between >> different physical hosts running with >> exact same QEMU cmdline will result in different CPUID values exposed to >> guest. > > > I think live migration itself doesn't do the CPUID check, so adding the QEMU > side hardcode part doesn't help. That is, even using that patch 7/7, I think > it wouldn't make a difference. > This is the same case when we migrate a VM from a skylake physical machine > to an older machine, the difference of supported CPUID won't even generate a > warning when the destination side QEMU gets booted, right? > > > > > >>> exposing >>> the two CPUIDs as long as they are physically supported by the >>> hardware, >>> and this doesn't require the QEMU side hardcode as usual. >>> >>> When the related feature bits are added to the kernel, and we can >>> simply >>> change it to: >>> best->edx |= F(SPEC_CTRL) | F(PRED_CMD); >>> >>> Signed-off-by: Wei Wang <wei.w.wang@xxxxxxxxx> >>> --- >>> arch/x86/kvm/cpuid.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >>> index 0099e10..c33d3d4 100644 >>> --- a/arch/x86/kvm/cpuid.c >>> +++ b/arch/x86/kvm/cpuid.c >>> @@ -70,6 +70,8 @@ u64 kvm_supported_xcr0(void) >>> /* These are scattered features in cpufeatures.h. */ >>> #define KVM_CPUID_BIT_AVX512_4VNNIW 2 >>> #define KVM_CPUID_BIT_AVX512_4FMAPS 3 >>> +#define KVM_CPUID_BIT_SPEC_CTRL 26 >>> +#define KVM_CPUID_BIT_STIBP 27 >>> #define KF(x) bit(KVM_CPUID_BIT_##x) >>> int kvm_update_cpuid(struct kvm_vcpu *vcpu) >>> @@ -109,6 +111,9 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu) >>> } >>> } >>> + if (cpuid_edx(0x7) & (KF(SPEC_CTRL) | KF(STIBP))) >> >> You should put this inside the "if (best) {...}" block above. > > > Right, thanks. > > Best, > Wei
