Re: [Xen-devel] [PATCH v2 05/10] x86: Implement Intel Processor Trace context switch

["Kang, Luwei" <luwei.kang@xxxxxxxxx>]

> >> > @@ -40,3 +42,102 @@ static int __init parse_ipt_params(const char
> >> > +static inline void ipt_save_msr(struct ipt_ctx *ctx, unsigned int
> >> > +addr_range) {
> >> > +    unsigned int i;
> >> > +
> >> > +    rdmsrl(MSR_IA32_RTIT_STATUS, ctx->status);
> >> > +    rdmsrl(MSR_IA32_RTIT_OUTPUT_BASE, ctx->output_base);
> >> > +    rdmsrl(MSR_IA32_RTIT_OUTPUT_MASK, ctx->output_mask);
> >> > +    rdmsrl(MSR_IA32_RTIT_CR3_MATCH, ctx->cr3_match);
> >> > +    for ( i = 0; i < addr_range; i++ )
> >> > +    {
> >> > +        rdmsrl(MSR_IA32_RTIT_ADDR_A(i), ctx->addr[i * 2]);
> >> > +        rdmsrl(MSR_IA32_RTIT_ADDR_B(i), ctx->addr[i * 2 + 1]);
> >> > +    }
> >> > +}
> >>
> >> So you save/restore them not at context switch, but at VM entry/exit
> >> time. This means the title is misleading. But it raises efficiency
> >> questions:
> >> Is it really necessary to do it this often? In patch 7 you handle
> >> reads and writes to the MSRs, but you don't disable the MSR
> >> intercepts (and judging from their titles no other patch is a candidate 
> >> where you might do that). If all writes are seen by Xen, why
> would you need to read all the MSRs here, when the majority is - afaict - not 
> modified by hardware?
> >
> > when PT in disabled in guest (guest have capability to enable PT but
> > RTIT_CTL.EN is 0), all the PT msrs read/write are intercepted and we
> > don't need to save or restore during vm-exit/entry. When PT is enabled
> > in guest, we need to save or restore the guest stat when vm-exit/entry.
> 
> Why for MSRs which don't get changed by hardware?
> 
> > What about add a flag to log the value of MSRs' changes so that we
> > don't need save/restore the MSRs when guest not change these values?
> 
> I'm afraid it's not clear to me what "log the value" is supposed to mean here.

I mean add a new flag to mark if the value of Intel PT MSRs is changed by 
guest. If guest don't have any change that we don't need to save/restore the 
guest PT MSRs value to real hardware when VM-exit/entry.

> 
> >> > @@ -466,11 +467,16 @@ static int vmx_vcpu_initialise(struct vcpu *v)
> >> >      if ( v->vcpu_id == 0 )
> >> >          v->arch.user_regs.rax = 1;
> >> >
> >> > +    rc = ipt_initialize(v);
> >> > +    if ( rc )
> >> > +        dprintk(XENLOG_ERR, "%pv: Failed to init Intel Processor
> >> > + Trace.\n", v);
> >>
> >> For such a message to be helpful, please also log rc. And no full stop in 
> >> log messages please (again with very few exceptions).
> >
> > Not full understand here. What is the " no full stop in log messages " mean?
> 
> "full stop" is the final period in a sentence. I.e. you want
> 
>         dprintk(XENLOG_ERR, "%pv: Failed to init Intel Processor Trace\n", v);

Change like this ?
dprintk(XENLOG_ERR, "%pv: Failed to init Intel Processor Trace: err=%d.\n", v, 
rc);

Thanks,
Luwei Kang

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