From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v5 06/15] VMX/altp2m: add code to support EPTP switching and #VE. Date: Mon, 20 Jul 2015 07:21:39 +0100 Message-ID: <55ACAF930200007800092D74@mail.emea.novell.com> References: <1436832903-12639-1-git-send-email-edmund.h.white@intel.com> <1436832903-12639-7-git-send-email-edmund.h.white@intel.com> <55A531600200007800090C22@mail.emea.novell.com> <55A797C70200007800091C7B@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ravi Sahita Cc: Tim Deegan , Wei Liu , George Dunlap , Andrew Cooper , Ian Jackson , Edmund H White , "xen-devel@lists.xen.org" , "tlengyel@novetta.com" , Daniel De Graaf List-Id: xen-devel@lists.xenproject.org >>> On 17.07.15 at 23:08, wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >>Sent: Thursday, July 16, 2015 2:39 AM >> >>>>> On 16.07.15 at 11:20, wrote: >>>> From: Jan Beulich [mailto:JBeulich@suse.com] >>>>Sent: Tuesday, July 14, 2015 6:57 AM >>>>>>> On 14.07.15 at 02:14, wrote: >>>>> +static bool_t vmx_vcpu_emulate_ve(struct vcpu *v) { >>>>> + bool_t rc = 0; >>>>> + ve_info_t *veinfo = gfn_x(vcpu_altp2m(v).veinfo_gfn) != >>>>> +INVALID_GFN >>>>? >>>>> + hvm_map_guest_frame_rw(gfn_x(vcpu_altp2m(v).veinfo_gfn), 0) : >>>>> +NULL; >>>>> + >>>>> + if ( !veinfo ) >>>>> + return 0; >>>>> + >>>>> + if ( veinfo->semaphore != 0 ) >>>>> + goto out; >>>>> + >>>>> + rc = 1; >>>>> + >>>>> + veinfo->exit_reason = EXIT_REASON_EPT_VIOLATION; >>>>> + veinfo->semaphore = ~0l; >>>> >>>>Isn't semaphore a 32-bit quantity? >>> >>> Yes. >> >>I.e. the l suffix can and should be dropped. >> > > Ok. > >>>>> + { >>>>> + unsigned long idx; >>>>> + >>>>> + if ( v->arch.hvm_vmx.secondary_exec_control & >>>>> + SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS ) >>>>> + __vmread(EPTP_INDEX, &idx); >>>>> + else >>>>> + { >>>>> + unsigned long eptp; >>>>> + >>>>> + __vmread(EPT_POINTER, &eptp); >>>>> + >>>>> + if ( (idx = p2m_find_altp2m_by_eptp(v->domain, eptp)) == >>>>> + INVALID_ALTP2M ) >>>>> + { >>>>> + gdprintk(XENLOG_ERR, "EPTP not found in alternate p2m >>list\n"); >>>>> + domain_crash(v->domain); >>>>> + } >>>>> + } >>>>> + >>>>> + if ( (uint16_t)idx != vcpu_altp2m(v).p2midx ) >>>> >>>>Is this cast really necessary? >>> >>> Yes - The index is 16-bits, this reflects how the field is specified >>> in the vmcs also. >> >>While "yes" answers the question, the explanation you give suggests that the >>answer may be wrong: Can idx indeed have bits set beyond bit 15? Because if >>it can't, the cast is pointless. >> > > We were just trying to ensure we matched the hardware behavior (I think > there was a message George had posted earlier for SVE that asked for that). > Since hardware considers only a 16 bit field we were doing the same. > >>>>> + { >>>>> + BUG_ON(idx >= MAX_ALTP2M); >>>>> + atomic_dec(&p2m_get_altp2m(v)->active_vcpus); >>>>> + vcpu_altp2m(v).p2midx = (uint16_t)idx; >>>> >>>>This one surely isn't (or else the field type is wrong). >>> >>> Again required. idx can't be uint16_t because __vmread() requires >>> unsigned long*, but the index is 16 bits. >> >>But it's a 16-bit VMCS field that you read it from, and hence the upper 48 > bits >>are necessarily zero. >> >>Just to re-iterate: Casts are necessary in certain places, yes, but I see them >>used pointlessly or even wrongly more often than not. > > Same approach as above - emulating hardware exactly. > Should we add a comment? No, drop the casts. Jan