Re: [Xen-devel] [PATCH 2/2] x86/HVM: Use fixed TSC value when saving or restoring domain

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.03.14 at 16:01, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 03/31/2014 05:51 AM, Jan Beulich wrote:
>>
>>> --- a/xen/arch/x86/hvm/save.c
>>> +++ b/xen/arch/x86/hvm/save.c
>>> @@ -24,7 +24,7 @@
>>>   #include <asm/hvm/support.h>
>>>   #include <public/hvm/save.h>
>>>   
>>> -void arch_hvm_save(struct domain *d, struct hvm_save_header *hdr)
>>> +void arch_hvm_save(struct domain *dom, struct hvm_save_header *hdr)
>> The change is unmotivated (afaict) and inconsistent with most other
>> code - we conventionally use "d" for struct domain * variables. Please
>> drop the change here and use "d" throughout the rest of the patch,
>> at once resulting in less churn and hence making it easier to review.
> 
> The reason for this change is subsequent
>      rdtscll(dom->arch.chkpt_tsc);
> 
> which will not work as
>      rdtscll(d->arch.chkpt_tsc);
> 
> The alternatives as I see are
> * Declare a temporary variable for use with rdtscll
> * Change rdtscll's definition to use something else instead of variable 
> 'd'. Say, eax and edx (hopefully this won't clash with something else)

Yeah, the macro using "a" and "d" as variables is really bad, and
it's that what should be changed.

>>> index 95b4b91..032eb23 100644
>>> --- a/xen/include/xen/time.h
>>> +++ b/xen/include/xen/time.h
>>> @@ -32,7 +32,8 @@ struct vcpu;
>>>   typedef s64 s_time_t;
>>>   #define PRI_stime PRId64
>>>   
>>> -s_time_t get_s_time(void);
>>> +s_time_t get_s_time_fixed(u64 at_tick);
>>> +#define get_s_time() get_s_time_fixed(0)
>> get_s_time(), through NOW(), has quite many users, so I'm not certain
>> the code bloat resulting from this is desirable. I'd suggest the function
>> to remain such; the compiler will be able to make it a mov+jmp.
> 
> Sorry, not sure I understand what you are asking for.
> 
> There shouldn't be much of code size increase since get_s_time() 
> currently (and get_s_time_fixed() after this patch is applied) are not 
> inlines. The only increase is due to routine itself getting very 
> slightly larger.
> 
> But I suspect you meant something else.

All call sites have to zero %edi with the change in place, and I
was trying to tell you that the number of call sites of this isn't
exactly small due to the function's use via NOW(). Hence I think
you shouldn't penalize the callers and have an explicit out of line
wrapper.

Jan


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