Re: [Xen-devel] [PATCH RFC v12 03/21] Remove an unnecessary assert from vmx_update_debug_state

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.09.13 at 14:53, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
> On 18/09/13 13:38, Jan Beulich wrote:
>>>>> On 18.09.13 at 12:39, George Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote:
>>> On Mon, Sep 16, 2013 at 10:09 PM, Mukesh Rathor
>>> <mukesh.rathor@xxxxxxxxxx> wrote:
>>>> On Fri, 13 Sep 2013 17:25:03 +0100
>>>> George Dunlap <george.dunlap@xxxxxxxxxxxxx> 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 <george.dunlap@xxxxxxxxxxxxx>
>>>>> CC: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>>>>> CC: Jan Beulich <jan.beulich@xxxxxxxx>
>>>>> CC: Tim Deegan <tim@xxxxxxx>
>>>>> CC: Keir Fraser <keir@xxxxxxx>
>>>>> ---
>>>>>   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.

Perhaps it's then better to indeed call vmx_vmcs_enter() here
(as I think Mukesh also already suggested), which is mostly a
no-op when called redundantly.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel