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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 04.07.18 at 10:48, <luwei.kang@intel.com> 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.
> 
> 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?

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

Jan



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