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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.01.14 at 17:55, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxx>
>>
>> --- 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


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