Re: [Xen-devel] [PATCH v8 15/19] x86/VPMU: Merge vpmu_rdmsr and vpmu_wrmsr

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.07.14 at 16:37, <boris.ostrovsky@xxxxxxxxxx> wrote:
> @@ -1794,10 +1794,13 @@ static int svm_msr_write_intercept(unsigned int msr, 
> uint64_t msr_content)
>      case MSR_AMD_FAM15H_EVNTSEL3:
>      case MSR_AMD_FAM15H_EVNTSEL4:
>      case MSR_AMD_FAM15H_EVNTSEL5:
> -        if ( vpmu_do_wrmsr(msr, msr_content) )
> +    {
> +        uint64_t msr_val = msr_content;
> +
> +        if ( vpmu_do_msr(msr, &msr_val, VPMU_MSR_WRITE) )
>              goto gpf;
>          break;
> -
> +    }
>      case MSR_IA32_MCx_MISC(4): /* Threshold register */

Please don't drop blank lines between not falling-through case blocks.

> @@ -2240,6 +2240,7 @@ void vmx_vlapic_msr_changed(struct vcpu *v)
>  static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>  {
>      struct vcpu *v = current;
> +    uint64_t msr_val;
>  
>      HVM_DBG_LOG(DBG_LEVEL_1, "ecx=%#x, msr_value=%#"PRIx64, msr, 
> msr_content);
>  
> @@ -2263,7 +2264,8 @@ static int vmx_msr_write_intercept(unsigned int msr, 
> uint64_t msr_content)
>          if ( msr_content & ~supported )
>          {
>              /* Perhaps some other bits are supported in vpmu. */
> -            if ( vpmu_do_wrmsr(msr, msr_content) )
> +            msr_val = msr_content;
> +            if ( vpmu_do_msr(msr, &msr_val, VPMU_MSR_WRITE) )

What do you need "msr_val" for? I.e. why can't you pass
"&msr_content" here?

> --- a/xen/arch/x86/hvm/vpmu.c
> +++ b/xen/arch/x86/hvm/vpmu.c
> @@ -91,7 +91,7 @@ void vpmu_lvtpc_update(uint32_t val)
>          apic_write(APIC_LVTPC, vpmu->hw_lapic_lvtpc);
>  }
>  
> -int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content)
> +int vpmu_do_msr(unsigned int msr, uint64_t *msr_content, uint8_t rw)

It would seem to me that a plain int (or even better enum) would
result in better code than a needlessly fixed-8-bit type.

> @@ -99,13 +99,21 @@ int vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content)
>      if ( !(vpmu_mode & XENPMU_MODE_ON) )
>          return 0;
>  
> -    if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->do_wrmsr )
> +    ASSERT((rw == VPMU_MSR_READ) || (rw == VPMU_MSR_WRITE));
> +
> +    if ( vpmu->arch_vpmu_ops )
>      {
> -        int ret = vpmu->arch_vpmu_ops->do_wrmsr(msr, msr_content);
> +        int ret;
>  
> +        if ( (rw == VPMU_MSR_READ) && vpmu->arch_vpmu_ops->do_rdmsr )
> +            ret = vpmu->arch_vpmu_ops->do_rdmsr(msr, msr_content);
> +        else if ( vpmu->arch_vpmu_ops->do_wrmsr )
> +            ret = vpmu->arch_vpmu_ops->do_wrmsr(msr, *msr_content);

This won't do what you intend if rw == VPMU_MSR_READ but there's
no ->do_rdmsr handler.

I also wonder whether latching vpmu->arch_vpmu_ops into a
local variable wouldn't benefit readability quite a bit.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel