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.
> 
> Okay, in which case back to the original question: Without disabling the 
> intercepts, you know what the guest wrote last. Why read
> the MSR then?

Oh, understand. I re-check these MSRs in spec and Only IA32_RTIT_STATUS is an 
exception. It can be changed by hardware.
Bit[3:0] are write ignore. 
Bit[5:4] : these bit are set by hardware and once it set only software can 
clear it.
Bit[48:32]: write by processor and can clear or modify this filed at any time 
when PT is enabled.
So, I think this register (IA32_RTIT_STATUS) is needed read after vm-exit. 
Other MSRs may don't need.
Bit[5:4] should be cleared in any way before vm-entry because only software can 
clear it.


> 
> >> >> > @@ -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);
> 
> Excuse me - I've told you to omit the full stop, and there it is again.
> Apart from that, yes, this is one option. A slightly short one we use here 
> and there is
> 
> dprintk(XENLOG_ERR, "%pv: Failed to init Intel Processor Trace (%d)\n", v, 
> rc);

I Just understand. "Full stop" is the  "." at the end of the sentence. We don't 
need that. :)

Thanks,
Luwei Kang

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