Re: [Xen-devel] [PATCH v2 07/10] x86: Add Intel Processor Trace MSRs read/write emulation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.05.18 at 15:28, <luwei.kang@xxxxxxxxx> wrote:
> @@ -106,6 +114,105 @@ static int __init parse_ipt_params(const char *str)
>      return 0;
>  }
>  
> +int ipt_do_rdmsr(unsigned int msr, uint64_t *msr_content)
> +{
> +    const struct ipt_desc *ipt_desc = current->arch.hvm_vmx.ipt_desc;
> +    const struct cpuid_policy *p = current->domain->arch.cpuid;
> +    unsigned int index;
> +
> +    if ( !ipt_desc )
> +        return 1;
> +
> +    switch ( msr )
> +    {
> +    case MSR_IA32_RTIT_CTL:
> +        *msr_content = ipt_desc->ipt_guest.ctl;
> +        break;
> +    case MSR_IA32_RTIT_STATUS:
> +        *msr_content = ipt_desc->ipt_guest.status;
> +        break;
> +    case MSR_IA32_RTIT_OUTPUT_BASE:
> +        if ( !ipt_cap(p->ipt.raw, IPT_CAP_single_range_output) &&
> +             !ipt_cap(p->ipt.raw, IPT_CAP_topa_output) )
> +            return 1;
> +        *msr_content = ipt_desc->ipt_guest.output_base;
> +        break;
> +    case MSR_IA32_RTIT_OUTPUT_MASK:
> +        if ( !ipt_cap(p->ipt.raw, IPT_CAP_single_range_output) &&
> +             !ipt_cap(p->ipt.raw, IPT_CAP_topa_output) )
> +            return 1;
> +        *msr_content = ipt_desc->ipt_guest.output_mask |
> +                                    RTIT_OUTPUT_MASK_DEFAULT;
> +        break;
> +    case MSR_IA32_RTIT_CR3_MATCH:
> +        if ( !ipt_cap(p->ipt.raw, IPT_CAP_cr3_filter) )
> +            return 1;
> +        *msr_content = ipt_desc->ipt_guest.cr3_match;
> +        break;
> +    default:
> +     index = msr - MSR_IA32_RTIT_ADDR_A(0);

Hard tab. Also throughout both functions' switch() statements please
add blank lines between case blocks.

> +int ipt_do_wrmsr(unsigned int msr, uint64_t msr_content)
> +{
> +    struct ipt_desc *ipt_desc = current->arch.hvm_vmx.ipt_desc;
> +    const struct cpuid_policy *p = current->domain->arch.cpuid;
> +    unsigned int index;
> +
> +    if ( !ipt_desc )
> +        return 1;
> +
> +    switch ( msr )
> +    {
> +    case MSR_IA32_RTIT_CTL:
> +        ipt_desc->ipt_guest.ctl = msr_content;
> +        __vmwrite(GUEST_IA32_RTIT_CTL, msr_content);
> +        break;
> +    case MSR_IA32_RTIT_STATUS:
> +        if ( (ipt_desc->ipt_guest.ctl & RTIT_CTL_TRACEEN) ||
> +             (msr_content & MSR_IA32_RTIT_STATUS_MASK) )
> +            return 1;
> +        ipt_desc->ipt_guest.status = msr_content;
> +        break;
> +    case MSR_IA32_RTIT_OUTPUT_BASE:
> +        if ( (ipt_desc->ipt_guest.ctl & RTIT_CTL_TRACEEN) ||
> +             (msr_content &
> +                 MSR_IA32_RTIT_OUTPUT_BASE_MASK(p->extd.maxphysaddr)) ||

Indentation.

> +             (!ipt_cap(p->ipt.raw, IPT_CAP_single_range_output) &&
> +              !ipt_cap(p->ipt.raw, IPT_CAP_topa_output)) )
> +            return 1;
> +        ipt_desc->ipt_guest.output_base = msr_content;
> +        break;
> +    case MSR_IA32_RTIT_OUTPUT_MASK:
> +        if ( (ipt_desc->ipt_guest.ctl & RTIT_CTL_TRACEEN) ||
> +             (!ipt_cap(p->ipt.raw, IPT_CAP_single_range_output) &&
> +              !ipt_cap(p->ipt.raw, IPT_CAP_topa_output)) )
> +            return 1;
> +        ipt_desc->ipt_guest.output_mask = msr_content |
> +                                RTIT_OUTPUT_MASK_DEFAULT;

Again.

> +        break;
> +    case MSR_IA32_RTIT_CR3_MATCH:
> +        if ( (ipt_desc->ipt_guest.ctl & RTIT_CTL_TRACEEN) ||
> +             !ipt_cap(p->ipt.raw, IPT_CAP_cr3_filter) )
> +            return 1;
> +        ipt_desc->ipt_guest.cr3_match = msr_content;
> +        break;
> +    default:
> +        index = msr - MSR_IA32_RTIT_ADDR_A(0);
> +        if ( index >= ipt_cap(p->ipt.raw, IPT_CAP_addr_range) * 2 )
> +            return 1;
> +        ipt_desc->ipt_guest.addr[index] = msr_content;
> +    }

Please don't omit the "break;" above here (same in the other
function).

> +    return 0;
> +}

Both functions only ever return 0 or 1 - did you mean their return type
to be bool? And the perhaps better use true for success and false for
failure?

> @@ -204,3 +311,4 @@ void ipt_destroy(struct vcpu *v)
>          v->arch.hvm_vmx.ipt_desc = NULL;
>      }
>  }
> +

Stray addition of a blank line.

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2898,6 +2898,15 @@ static int vmx_msr_read_intercept(unsigned int msr, 
> uint64_t *msr_content)
>          if ( vpmu_do_rdmsr(msr, msr_content) )
>              goto gp_fault;
>          break;
> +    case MSR_IA32_RTIT_CTL:
> +    case MSR_IA32_RTIT_STATUS:
> +    case MSR_IA32_RTIT_OUTPUT_BASE:
> +    case MSR_IA32_RTIT_OUTPUT_MASK:
> +    case MSR_IA32_RTIT_CR3_MATCH:
> +    case MSR_IA32_RTIT_ADDR_A(0) ... MSR_IA32_RTIT_ADDR_B(3):

Is the 3 here an architectural limit? Otherwise you want to use a higher
number here and rely on the callee to do the full checking.

Jan


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