Re: [Xen-devel] [PATCH 3/5] x86/time: Rework pv_soft_rdtsc() to aid further cleanup

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/02/18 16:04, Roger Pau Monné wrote:
> On Tue, Feb 20, 2018 at 11:58:41AM +0000, Andrew Cooper wrote:
>> Having pv_soft_rdtsc() emulate all parts of an rdtscp is awkward, and gets in
>> the way of some intended cleanup.
>>
>>  * Drop the rdtscp parameter and always make the caller responsible for ecx
>>    updates when appropriate.
>>  * Switch the function from being void, and return the main timestamp in the
>>    return value.
>>
>> The regs parameter is still needed, but only for the stats collection, once
>> again bringing into question their utility.  The parameter can however switch
>> to being const.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>> ---
>>  xen/arch/x86/pv/emul-inv-op.c  |  7 ++++++-
>>  xen/arch/x86/pv/emul-priv-op.c | 12 ++++++++----
>>  xen/arch/x86/time.c            |  8 ++------
>>  xen/include/asm-x86/time.h     |  2 +-
>>  4 files changed, 17 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/x86/pv/emul-inv-op.c b/xen/arch/x86/pv/emul-inv-op.c
>> index f894417..b1916b4 100644
>> --- a/xen/arch/x86/pv/emul-inv-op.c
>> +++ b/xen/arch/x86/pv/emul-inv-op.c
>> @@ -46,6 +46,7 @@ static int emulate_invalid_rdtscp(struct cpu_user_regs 
>> *regs)
>>      char opcode[3];
>>      unsigned long eip, rc;
>>      struct vcpu *v = current;
>> +    struct domain *currd = v->domain;
> const?
>
>>  
>>      eip = regs->rip;
>>      if ( (rc = copy_from_user(opcode, (char *)eip, sizeof(opcode))) != 0 )
>> @@ -56,7 +57,11 @@ static int emulate_invalid_rdtscp(struct cpu_user_regs 
>> *regs)
>>      if ( memcmp(opcode, "\xf\x1\xf9", sizeof(opcode)) )
>>          return 0;
>>      eip += sizeof(opcode);
>> -    pv_soft_rdtsc(v, regs, 1);
>> +
>> +    msr_split(regs, pv_soft_rdtsc(v, regs));
>> +    regs->rcx = ((currd->arch.tsc_mode == TSC_MODE_PVRDTSCP)
>> +                 ? currd->arch.incarnation : 0);
> In the previous patch you use the same expression without any
> parentheses. I think I prefer the version without parentheses. I also
> wonder whether it would make sense to turn this into a macro (seeing
> it being used in the previous patch and also further below).
>
> With those fixed (if applicable):

None of them survive patch 5 in the series.  I'll fix the formatting,
but I'm not going to macro-ise anything.

>
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>
> Thanks, Roger.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel