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

[George Dunlap <george.dunlap@eu.citrix.com>]

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