Re: [Xen-devel] [PATCH v2 2/2] xen: add update indicator to vcpu_runstate_info

[Juergen Gross <jgross@xxxxxxxx>]

On 16/06/16 13:15, Jan Beulich wrote:
>>>> On 15.06.16 at 13:34, <JGross@xxxxxxxx> wrote:
>> In order to support reading another vcpu's mapped vcpu_runstate_info
>> an indicator for an occurring update of the runstate information is
>> needed.
>>
>> Add the possibility to activate setting this indicator in the highest
>> bit of state_entry_time via a vm_assist hypercall. When activated the
>> update indicator will be set before the runstate information is
>> modified in guest memory and it will be reset after modification is
>> done. As state_entry_time is guaranteed to be different after each
>> update the guest can detect any update (either in progress or while
>> reading the runstate data) by comparing state_entry_time before and
>> after reading runstate data: in case the values differ or the update
>> indicator was set the data might be inconsistent and should be reread.
> 
> Neither here nor in the cover letter you mention why this is useful
> to have (and to be honest I did forget what you need this for since
> the time the original discussion happened), yet that's quite relevant
> since for many years we lived happily without it.

I'll add the following in the cover letter:

There has been a report about incorrect vruntime accounting in Linux
guests under Xen. A Linux kernel with CONFIG_PARAVIRT_TIME_ACCOUNTING
set is capable to do correct vruntime accounting, but this would
require the kernel to be able to read the runstate data of other cpus.

> 
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1926,12 +1926,35 @@ bool_t update_runstate_area(struct vcpu *v)
>>  {
>>      bool_t rc;
>>      smap_check_policy_t smap_policy;
>> +    void __user *guest_handle = NULL;
>> +    unsigned off = 0;
>>  
>>      if ( guest_handle_is_null(runstate_guest(v)) )
>>          return 1;
>>  
>>      smap_policy = smap_policy_change(v, SMAP_CHECK_ENABLED);
>>  
>> +    if ( VM_ASSIST(v->domain, runstate_update_flag) )
>> +    {
>> +        off = offsetof(struct vcpu_runstate_info, state_entry_time) +
>> +              sizeof(v->runstate.state_entry_time) - 1;
>> +        if ( has_32bit_shinfo(v->domain) )
>> +        {
>> +            guest_handle = v->runstate_guest.compat.p;
>> +            guest_handle +=
>> +                offsetof(struct compat_vcpu_runstate_info, 
>> state_entry_time) +
>> +                sizeof(v->runstate.state_entry_time) - 1;
> 
> The sizes of the native and compat fields happen to be the same,
> but it would be nice if the right field/type could be used here.

Hmm, this will require some ugly type casting, but it is probably
cleaner.

>> --- a/xen/include/public/vcpu.h
>> +++ b/xen/include/public/vcpu.h
>> @@ -84,6 +84,12 @@ struct vcpu_runstate_info {
>>      /* When was current state entered (system time, ns)? */
>>      uint64_t state_entry_time;
>>      /*
>> +     * Update indicator set in state_entry_time:
>> +     * When activated via VMASST_TYPE_runstate_update_flag, set during
>> +     * updates in guest memory mapped copy of vcpu_runstate_info.
>> +     */
>> +#define XEN_RUNSTATE_UPDATE          (1ULL << 63)
> 
> I continue to be not particularly happy with the C99ism here, but
> I see we have a few other instances of such already in the public
> headers.

Indeed. And UINT64_C() is used nowhere in the hypervisor and defined in
xen/include/crypto/vmac.h


Juergen

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