Re: [PATCH 2/2] xen: add update indicator to vcpu_runstate_info

[Juergen Gross <jgross@suse.com>]

On 20/05/16 16:49, Jan Beulich wrote:
>>>> On 20.05.16 at 15:22, <JGross@suse.com> wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1925,13 +1925,37 @@ static void paravirt_ctxt_switch_to(struct vcpu *v)
>>  bool_t update_runstate_area(struct vcpu *v)
>>  {
>>      bool_t rc;
>> +    bool_t update_flag;
> 
> I think this variable is superfluous (and causes more register pressure
> in the compiler), since ...
> 
>>      smap_check_policy_t smap_policy;
>> +    void __user *guest_handle = NULL;
> 
> ... you can key off of this being non-NULL or ...
> 
>> +    unsigned off = 0;
> 
> ... this being non-zero in the second if().

Okay. Will change.

> 
>>      if ( guest_handle_is_null(runstate_guest(v)) )
>>          return 1;
>>  
>> +    update_flag = VM_ASSIST(v->domain, runstate_update_flag);
>> +
>>      smap_policy = smap_policy_change(v, SMAP_CHECK_ENABLED);
>>  
>> +    if ( update_flag )
>> +    {
>> +        off = offsetof(struct vcpu_runstate_info, state_entry_time) + 7;
> 
> How come this is outside the following if()? Also sizeof(...) - 1 please
> instead of the literal 7.

I'm using off for the source address in __raw_copy_to_guest(), too.
Regarding sizeof(): okay.

> 
>> --- 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 think this should be UINT64_C(1), as ULL is not a C89 compatible
> suffix, but by requiring uint64_t I think we can imply that along with
> that C99 type the platform also surfaces respective macros.

Okay.


Juergen

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