From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH RFC v12 03/21] Remove an unnecessary assert from vmx_update_debug_state Date: Wed, 18 Sep 2013 13:53:42 +0100 Message-ID: <5239A256.7040800@eu.citrix.com> References: <1379089521-25720-1-git-send-email-george.dunlap@eu.citrix.com> <1379089521-25720-4-git-send-email-george.dunlap@eu.citrix.com> <20130916140908.061d5484@mantra.us.oracle.com> <5239BACB02000078000F455D@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5239BACB02000078000F455D@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: Tim Deegan , Keir Fraser , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 18/09/13 13:38, Jan Beulich wrote: >>>> On 18.09.13 at 12:39, George Dunlap wrote: >> On Mon, Sep 16, 2013 at 10:09 PM, Mukesh Rathor >> wrote: >>> On Fri, 13 Sep 2013 17:25:03 +0100 >>> George Dunlap wrote: >>> >>>> As far as I can tell, there's nothing here that requires v to be >>>> current. vmx_update_exception_bitmap() is updated in other situations >>>> where v!=current. >>>> >>>> Removing this allows the PVH code to call this during vmcs >>>> construction in a later patch, making the code more robust by removing >>>> duplicate code. >>>> >>>> Signed-off-by: George Dunlap >>>> CC: Mukesh Rathor >>>> CC: Jan Beulich >>>> CC: Tim Deegan >>>> CC: Keir Fraser >>>> --- >>>> xen/arch/x86/hvm/vmx/vmx.c | 2 -- >>>> 1 file changed, 2 deletions(-) >>>> >>>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c >>>> index 8ed7026..f02e47a 100644 >>>> --- a/xen/arch/x86/hvm/vmx/vmx.c >>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c >>>> @@ -1041,8 +1041,6 @@ void vmx_update_debug_state(struct vcpu *v) >>>> { >>>> unsigned long mask; >>>> >>>> - ASSERT(v == current); >>>> - >>>> mask = 1u << TRAP_int3; >>>> if ( !cpu_has_monitor_trap_flag ) >>>> mask |= 1u << TRAP_debug; >>> >>> vmx_update_exception_bitmap() writes EXCEPTION_BITMAP of current vmcs, >>> ie of current vcpu. If v != current, you'd be writing the bitmap of >>> wrong vcpu. If the assertion is removed, then the function would need >>> to vmx enter... >> You mean, callers would need to call vmcs_enter before calling it. >> Hmm... is there an assert for that? > This is basically what the assertion you're removing does, albeit > restricting it more than it needs to be. But - do you have a use > case for the function when v != current? Yes -- in patch 8, I add a call to this from xen/arch/x86/hvm/vmx/vmcs.c:construct_vmcs(). As the comment there says, this update normally happens when the guest enables paging. But since PVH guests start out with paging enabled, this never happens so we do it at start-of-day. (That's what Mukesh's code did, but by duplicating the code inside the function, so I followed suit, but called the function itself instead.) At that point vmx_vmcs_enter() has already been called, so the ASSERT is overly restrictive. -George