Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755128AbeAOJVV (ORCPT + 1 other); Mon, 15 Jan 2018 04:21:21 -0500 Received: from mx1.redhat.com ([209.132.183.28]:50402 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754462AbeAOJVT (ORCPT ); Mon, 15 Jan 2018 04:21:19 -0500 Subject: Re: [PATCH 4/8] kvm: vmx: Set IBPB when running a different VCPU To: "Woodhouse, David" , "jmattson@google.com" , "kernellwp@gmail.com" Cc: "kvm@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "tim.c.chen@linux.intel.com" , "Liguori, Anthony" , "x86@kernel.org" , "liran.alon@oracle.com" , "bp@alien8.de" , "thomas.lendacky@amd.com" , "rkrcmar@redhat.com" References: <20180109120311.27565-1-pbonzini@redhat.com> <20180109120311.27565-5-pbonzini@redhat.com> <1515835795.22302.517.camel@amazon.co.uk> From: Paolo Bonzini Message-ID: Date: Mon, 15 Jan 2018 10:21:08 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <1515835795.22302.517.camel@amazon.co.uk> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Mon, 15 Jan 2018 09:21:19 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On 13/01/2018 10:29, Woodhouse, David wrote: > On Fri, 2018-01-12 at 09:03 -0800, Jim Mattson wrote: >> The point behind the IPBP in vmx_vcpu_load is to prevent one VCPU from >> steering the speculative execution of the next. If the VMCS address is >> recycled, vmx_vcpu_load doesn't realize that the VCPUs are different, >> and so it won't issue the IPBP. > > I don't understand the sequence of events that could lead to this. > > If the VMCS is freed, surely per_cpu(current_vmcs, cpu) has to be > cleared? If the VMCS is freed while it's still *active* on a CPU, > that's a bug, surely? And if that CPU is later offlined and clears the > VMCS, it's going to scribble on freed (and potentially re-used) memory. You're right, svm.c needs it but vmx.c does not (because AMD has no vmclear equivalent). >>> +       if (have_spec_ctrl) >>> +               wrmsrl(MSR_IA32_PRED_CMD, PRED_CMD_IBPB); > > Also, I think the same condition applies to the conditional branches > over the IBPB-frobbing, as it does to setting IBRS. You can eschew the > 'else lfence' only if you put in a comment showing that you've proved > it's safe. Many of the other bits like this are being done with > alternatives, which avoids that concern completely. > > But really, I don't like this series much. Don't say "let's do this > until upstream supports...". Just fix it up properly, and add the > generic X86_FEATURE_IBPB bit and use it. We have *too* many separate > tiny patch sets, and we need to be getting our act together and putting > it all in one. I agree, this series is not meant to be committed. I only posted to get early reviews (and it was very effective for that purpose). Paolo