Re: [Xen-devel] [PATCH v2 08/10] x86: Introduce a function to check the value of RTIT_CTL

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

> > Any attempt to modify IA32_RTIT_CTL while TraceEn is set will result
> > in a #GP unless the same write also clears TraceEn.
> > Writes to IA32_RTIT_CTL that do not modify any bits will not cause a
> > #GP, even if TraceEn remains set.
> > MSR write that attempts to change bits marked reserved, or utilize
> > encodings marked reserved, will cause a #GP fault.
> 
> May I ask that you also add a similar code comment, perhaps ahead of the 
> function definition?

Get it. Will do that.

> 
> > --- a/xen/arch/x86/cpu/ipt.c
> > +++ b/xen/arch/x86/cpu/ipt.c
> > @@ -114,6 +114,114 @@ static int __init parse_ipt_params(const char *str)
> >      return 0;
> >  }
> >
> > +static int rtit_ctl_check(uint64_t new, uint64_t old)
> 
> It looks as if again you mean the function to return boolean, so please have 
> it have bool return type.

Get it.

> 
> > +{
> > +    const struct cpuid_policy *p = current->domain->arch.cpuid;
> > +    const struct ipt_desc *ipt_desc = current->arch.hvm_vmx.ipt_desc;
> > +    uint64_t rtit_ctl_mask = ~((uint64_t)0);
> 
> Too many parentheses.

Here is a pointless initializer. Will fix it.

> > +     */
> > +    if ( new & rtit_ctl_mask )
> > +        return 1;
> > +
> > +    /*
> > +     * Any attempt to modify IA32_RTIT_CTL while TraceEn is set will
> > +     * result in a #GP unless the same write also clears TraceEn.
> > +     */
> > +    if ( (ipt_desc->ipt_guest.ctl & RTIT_CTL_TRACEEN) &&
> > +        ((ipt_desc->ipt_guest.ctl ^ new) & ~RTIT_CTL_TRACEEN) )
> 
> Why the ^ ? You only need to check whether new has the bit clear.

I mean if change any bits (set or clear, not include RTIT_CTL_TRACEEN) with PT 
is enabled (RTIT_CTL_TRACEEN is set) will inject an #GP.

> Also please use "old" wherever possible, if you already have it passed into 
> the function. This way it'll become obvious that the
> "nothing changed" case is already handled by the very first if().

I think also can remove "old" (decrease a function parameter) and all use 
ipt_desc->ipt_guest.ctl instead.

> 
> > +        return 1;
> > +
> > +    /*
> > +     * WRMSR to IA32_RTIT_CTL that sets TraceEn but clears this bit
> > +     * and FabricEn would cause #GP, if
> > +     * CPUID.(EAX=14H, ECX=0):ECX.SNGLRGNOUT[bit 2] = 0
> > +     */
> > +   if ( (new & RTIT_CTL_TRACEEN) && !(new & RTIT_CTL_TOPA) &&
> > +        !(new & RTIT_CTL_FABRIC_EN) &&
> > +        !ipt_cap(p->ipt.raw, IPT_CAP_single_range_output) )
> > +        return 1;
> > +    /*
> > +     * MTCFreq, CycThresh and PSBFreq encodings check, any MSR write that
> > +     * utilize encodings marked reserved will casue a #GP fault.
> > +     */
> > +    val = ipt_cap(p->ipt.raw, IPT_CAP_mtc_period);
> > +    if ( ipt_cap(p->ipt.raw, IPT_CAP_mtc) &&
> > +                !test_bit((new & RTIT_CTL_MTC_FREQ) >>
> > +                RTIT_CTL_MTC_FREQ_OFFSET, &val) )
> 
> Indentation.
> 
> > @@ -171,6 +279,8 @@ int ipt_do_wrmsr(unsigned int msr, uint64_t msr_content)
> >      switch ( msr )
> >      {
> >      case MSR_IA32_RTIT_CTL:
> > +        if ( rtit_ctl_check(msr_content, ipt_desc->ipt_guest.ctl) )
> > +            return 1;
> >          ipt_desc->ipt_guest.ctl = msr_content;
> >          __vmwrite(GUEST_IA32_RTIT_CTL, msr_content);
> >          break;
> 
> Without this I don't see how the previous patch is complete.

What about merge this patch with patch 7 ?

Thanks,
Luwei Kang

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