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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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):

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