From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH V3 3/6] x86/xsaves: enable xsaves/xrstors for hvm guest Date: Fri, 7 Aug 2015 14:04:51 +0100 Message-ID: <55C4ACF3.1080509@citrix.com> References: <1438739842-31658-1-git-send-email-shuai.ruan@linux.intel.com> <1438739842-31658-4-git-send-email-shuai.ruan@linux.intel.com> <55C25348.3000306@citrix.com> <20150807082244.GB2976@shuai.ruan@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150807082244.GB2976@shuai.ruan@linux.intel.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: Shuai Ruan Cc: kevin.tian@intel.com, wei.liu2@citrix.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, jun.nakajima@intel.com, ian.jackson@eu.citrix.com, eddie.dong@intel.com, xen-devel@lists.xen.org, jbeulich@suse.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org On 07/08/15 09:22, Shuai Ruan wrote: > >>> void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, >>> unsigned int *ecx, unsigned int *edx) >>> { >>> @@ -4456,6 +4460,34 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, >>> *ebx = _eax + _ebx; >>> } >>> } >>> + if ( count == 1 ) >>> + { >>> + if ( cpu_has_xsaves ) >>> + { >>> + *ebx = XSTATE_AREA_MIN_SIZE; >>> + if ( v->arch.xcr0 | v->arch.msr_ia32_xss ) >>> + for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ ) >>> + { >>> + if ( !((v->arch.xcr0 | v->arch.msr_ia32_xss) >>> + & (1ULL << sub_leaf)) ) >>> + continue; >>> + domain_cpuid(d, input, sub_leaf, &_eax, &_ebx, &_ecx, >>> + &_edx); >>> + *ebx = *ebx + _eax; >>> + } >>> + } >>> + else >>> + { >>> + *eax &= ~XSAVES; >>> + *ebx = *ecx = *edx = 0; >>> + } >>> + if ( !cpu_has_xgetbv1 ) >>> + *eax &= ~XGETBV1; >>> + if ( !cpu_has_xsavec ) >>> + *eax &= ~XSAVEC; >>> + if ( !cpu_has_xsaveopt ) >>> + *eax &= ~XSAVEOPT; >>> + } >> Urgh - I really need to get domain cpuid fixed in Xen. This is >> currently making a very bad situation a little worse. >> > In patch 4, I expose the xsaves/xsavec/xsaveopt and need to check > whether the hardware supoort it. What's your suggestion about this? Calling into domain_cpuid() in the loop is not useful as nothing will set the subleaves up. As a first pass, reading from xstate_{offsets,sizes} will be better than nothing, as it will at least match reality until the domain is migrated. Longterm, I plan to overhaul the cpuid infrastructure to allow it to properly represent per-core and per-package data, as well as move it into the Xen architectural migration state, to avoid any host specific values leaking into guest state. This however is also a lot of work, which you don't want to dependent on. > >>> static int construct_vmcs(struct vcpu *v) >>> { >>> struct domain *d = v->domain; >>> @@ -1204,6 +1206,9 @@ static int construct_vmcs(struct vcpu *v) >>> __vmwrite(GUEST_PAT, guest_pat); >>> } >>> >>> + if ( cpu_has_vmx_xsaves ) >>> + __vmwrite(XSS_EXIT_BITMAP, VMX_XSS_EXIT_BITMAP); >>> + >>> vmx_vmcs_exit(v); >>> >>> /* PVH: paging mode is updated by arch_set_info_guest(). */ >>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c >>> index d3183a8..64ff63b 100644 >>> --- a/xen/arch/x86/hvm/vmx/vmx.c >>> +++ b/xen/arch/x86/hvm/vmx/vmx.c >>> @@ -2708,6 +2708,16 @@ static int vmx_handle_apic_write(void) >>> return vlapic_apicv_write(current, exit_qualification & 0xfff); >>> } >>> >>> +static void vmx_handle_xsaves(void) >>> +{ >>> + WARN(); >>> +} >>> + >>> +static void vmx_handle_xrstors(void) >>> +{ >>> + WARN(); >>> +} >>> + >> What is these supposed to do? They are not an appropriate handlers. >> > These two handlers do nothing here. Perform xsaves in HVM guest will > not trap in hypersior in this patch (by setting XSS_EXIT_BITMAP zero). > However it may trap in the future. See SDM Volume 3 Section 25.1.3 > for detail information. in which case use domain_crash(). WARN() here will allow a guest to DoS Xen. ~Andrew