From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nadav Har'El Subject: Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling Date: Mon, 19 Mar 2012 18:53:06 +0200 Message-ID: <20120319165306.GA20643@fermat.math.technion.ac.il> References: <20120307155846.GA17631@fermat.math.technion.ac.il> <4F578A53.20809@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, Julian Stecklina To: Avi Kivity Return-path: Received: from mailgw13.technion.ac.il ([132.68.225.13]:61790 "EHLO mailgw13.technion.ac.il" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1162218Ab2CSQxN (ORCPT ); Mon, 19 Mar 2012 12:53:13 -0400 Content-Disposition: inline In-Reply-To: <4F578A53.20809@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Hi, in a minute I'll send a new version of the MSR_IA32_FEATURE_CONTROL patch for nested VMX; I just wanted to reply first to your comments so you'll know what to expect: On Wed, Mar 07, 2012, Avi Kivity wrote about "Re: PATCH: nVMX: Better MSR_IA32_FEATURE_CONTROL handling": > On 03/07/2012 05:58 PM, Nadav Har'El wrote: > > + u64 msr_ia32_feature_control; > > }; > > Need to add to the list of exported MSRs so it can be live migrated > (msrs_to_save). Did this. > The variable itself should live in vcpu->arch, even if > some bits are vendor specific. But not this. I understand what you explained about vmx.c being for Intel *hosts*, not about emulating Intel *guests*, but I do think that since none of the bits in this MSR are relevant on AMD hosts (which don't do nested VMX), it isn't useful to support this MSR outside vmx.c. So I left this variable it in vmx->nested. As I noted earlier, svm.c did exactly the same thing (nested.vm_cr_msr), so at least there's symmetry here. > > @@ -1999,7 +2000,7 @@ static int vmx_get_vmx_msr(struct kvm_vc > > > > switch (msr_index) { > > case MSR_IA32_FEATURE_CONTROL: > > - *pdata = 0; > > + *pdata = to_vmx(vcpu)->nested.msr_ia32_feature_control; > > break; > > In a separate patch, please move this outside vmx_get_vmx_msr(). It's > not a vmx msr. Done, but not split into two patches: The patch removes the old case in vmx_get_vmx_msr() (and also removes vmx_set_vmx_msr() entirely) and instead adds the case in vmx_get_msr() and vmx_set_msr(). > > +#define VMXON_NEEDED_FEATURES \ > > + (FEATURE_CONTROL_LOCKED | FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX) > > Use const u64 instead of #define please, it jars my eyes. I would, if Linux coding style allowed to declare variables in the middle of blocks. Unfortunately it doesn't, so I left this #define. I don't think it's that bad. -- Nadav Har'El | Monday, Mar 19 2012, nyh@math.technion.ac.il |----------------------------------------- Phone +972-523-790466, ICQ 13349191 |A conscience does not prevent sin. It http://nadav.harel.org.il |only prevents you from enjoying it.