From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH RFC v13 06/20] pvh: vmx-specific changes Date: Thu, 7 Nov 2013 14:14:36 +0000 Message-ID: <527BA04C.4070407@eu.citrix.com> References: <1379955000-11050-1-git-send-email-george.dunlap@eu.citrix.com> <1379955000-11050-7-git-send-email-george.dunlap@eu.citrix.com> <52446EE202000078000F70AD@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VeQM9-0007px-MM for xen-devel@lists.xenproject.org; Thu, 07 Nov 2013 14:14:45 +0000 In-Reply-To: <52446EE202000078000F70AD@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Keir Fraser , Tim Deegan List-Id: xen-devel@lists.xenproject.org On 26/09/13 16:29, Jan Beulich wrote: >>>> On 23.09.13 at 18:49, George Dunlap wrote: >> Changes: >> * Enforce HAP mode for now >> * Disable exits related to virtual interrupts or emulated APICs >> * Disable changing paging mode >> - "unrestricted guest" (i.e., real mode for EPT) disabled >> - write guest EFER disabled >> * Start in 64-bit mode >> * Force TSC mode to be "none" >> * Paging mode update to happen in arch_set_info_guest >> >> Signed-off-by: George Dunlap >> Signed-off-by: Mukesh Rathor >> --- >> v13: >> - Fix up default cr0 settings >> - Get rid of some unnecessary PVH-related changes >> - Return EOPNOTSUPP instead of ENOSYS if hardware features are not present >> - Remove an unnecessary variable from pvh_check_requirements >> CC: Jan Beulich >> CC: Tim Deegan >> CC: Keir Fraser >> --- >> xen/arch/x86/hvm/vmx/vmcs.c | 130 >> +++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 126 insertions(+), 4 deletions(-) >> >> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >> index cf54d18..53fccdf 100644 >> --- a/xen/arch/x86/hvm/vmx/vmcs.c >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >> @@ -828,6 +828,60 @@ void virtual_vmcs_vmwrite(void *vvmcs, u32 >> vmcs_encoding, u64 val) >> virtual_vmcs_exit(vvmcs); >> } >> >> +static int pvh_check_requirements(struct vcpu *v) >> +{ >> + u64 required; >> + >> + /* Check for required hardware features */ >> + if ( !cpu_has_vmx_ept ) >> + { >> + printk(XENLOG_G_INFO "PVH: CPU does not have EPT support\n"); >> + return -EOPNOTSUPP; >> + } >> + if ( !cpu_has_vmx_pat ) >> + { >> + printk(XENLOG_G_INFO "PVH: CPU does not have PAT support\n"); >> + return -EOPNOTSUPP; >> + } >> + if ( !cpu_has_vmx_msr_bitmap ) >> + { >> + printk(XENLOG_G_INFO "PVH: CPU does not have msr bitmap\n"); >> + return -EOPNOTSUPP; >> + } >> + if ( !cpu_has_vmx_secondary_exec_control ) >> + { >> + printk(XENLOG_G_INFO "CPU Secondary exec is required to run PVH\n"); >> + return -EOPNOTSUPP; >> + } > Up to here the checks are VMX specific, and hence belong in a VMX > specific file, ... > >> + required = X86_CR4_PAE | X86_CR4_VMXE | X86_CR4_OSFXSR; >> + if ( (real_cr4_to_pv_guest_cr4(mmu_cr4_features) & required) != required ) >> + { >> + printk(XENLOG_G_INFO "PVH: required CR4 features not available:%lx\n", >> + required); >> + return -EOPNOTSUPP; >> + } >> + >> + /* Check for required configuration options */ >> + if ( !paging_mode_hap(v->domain) ) >> + { >> + printk(XENLOG_G_INFO "HAP is required for PVH guest.\n"); >> + return -EINVAL; >> + } >> + /* >> + * If rdtsc exiting is turned on and it goes thru emulate_privileged_op, >> + * then pv_vcpu.ctrlreg must be added to the pvh struct. >> + */ >> + if ( v->domain->arch.vtsc ) >> + { >> + printk(XENLOG_G_INFO >> + "At present PVH only supports the default timer mode\n"); >> + return -EINVAL; >> + } > ... but all of these are pretty generic (apart from the X86_CR4_VMXE > in the CR4 mask checked above, but I wonder whether that > shouldn't be checked much earlier - for HVM guests no such check > exists here afaik). The hap check can be removed and checked in hvm_domain_initialise(), and the vtsc one moved to tsc_set_info(). Is it really necessary to check these bits in cr4? Or is this more or less an ASSERT() to make sure people haven't accidentally disabled these in the real->guest cr4 conversion? -George