Re: [PATCH] x86: don't drop guest visible state updates when 64-bit PV guest is in user mode

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 21/01/2014 17:01, Jan Beulich wrote:
>>>> On 21.01.14 at 17:55, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 21/01/2014 15:33, Jan Beulich wrote:
>>> Since 64-bit PV uses separate kernel and user mode page tables, kernel
>>> addresses (as usually provided via VCPUOP_register_runstate_memory_area
>>> and possibly via VCPUOP_register_vcpu_time_memory_area) aren't
>>> necessarily accessible when the respective updating occurs. Add logic
>>> for toggle_guest_mode() to take care of this (if necessary) the next
>>> time the vCPU switches to kernel mode.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -1323,10 +1323,10 @@ static void paravirt_ctxt_switch_to(stru
>>>  }
>>>  
>>>  /* Update per-VCPU guest runstate shared memory area (if registered). */
>>> -static void update_runstate_area(struct vcpu *v)
>>> +bool_t update_runstate_area(const struct vcpu *v)
>> Can you adjust the comment to indicate what the return value means.  The
>> logic is quite opaque, but I believe it is "true if the runstate has
>> been suitably updated".
> Sigh - yes, I could of course. But it looks quite obvious to me.
>
>>> --- a/xen/arch/x86/x86_64/traps.c
>>> +++ b/xen/arch/x86/x86_64/traps.c
>>> @@ -266,6 +266,18 @@ void toggle_guest_mode(struct vcpu *v)
>>>  #else
>>>      write_ptbase(v);
>>>  #endif
>>> +
>>> +    if ( !(v->arch.flags & TF_kernel_mode) )
>>> +        return;
>>> +
>>> +    if ( v->arch.pv_vcpu.need_update_runstate_area &&
>>> +         update_runstate_area(v) )
>>> +        v->arch.pv_vcpu.need_update_runstate_area = 0;
>>> +
>>> +    if ( v->arch.pv_vcpu.pending_system_time.version &&
>>> +         update_secondary_system_time(v,
>>> +                                      &v->arch.pv_vcpu.pending_system_time) )
>>> +        v->arch.pv_vcpu.pending_system_time.version = 0;
>> What would happen now if a guest kernel loads a faulting address for its
>> runstate info (or edits its pagetables so a previously good address now
>> faults)?  It appears as if we could retry writing to the same bad
>> address each time we try to toggle into kernel mode.
>>
>> Given that we know we are running on the correct set of pagetables, I
>> think both of these pending variable can be unconditionally cleared,
>> whether or not update{runstate_area,secondary_system_time} succeed or fail.
> We could, but do you really think there's much harm in us keeping
> this logically consistent? The only entity impacted is going to be
> the "offending" domain, as we're wasting more of its time doing the
> mode switch for it.
>
> Jan
>

I suppose not - an extra pagefault or two for a toggle into kernel mode
is hardly the biggest of overheads.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>