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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.07.18 at 12:18, <luwei.kang@xxxxxxxxx> wrote:
>> > @@ -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.

>> > @@ -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);

Jan



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