On 06/15/2015 11:06 AM, Jan Beulich wrote:
On 10.06.15 at 17:04, <boris.ostrov...@oracle.com> wrote:--- a/xen/arch/x86/hvm/vmx/vpmu_core2.c +++ b/xen/arch/x86/hvm/vmx/vpmu_core2.c @@ -454,36 +454,41 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content, IA32_DEBUGCTLMSR_BTS_OFF_USR; if ( !(msr_content & ~supported) && vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) ) - return 1; + return 0; if ( (msr_content & supported) && !vpmu_is_set(vpmu, VPMU_CPU_HAS_BTS) ) printk(XENLOG_G_WARNING "%pv: Debug Store unsupported on this CPU\n", current); } - return 0; + return -EINVAL; }ASSERT(!supported); + if ( type == MSR_TYPE_COUNTER &&+ (msr_content & + ~((1ull << core2_get_bitwidth_fix_count()) - 1)) ) + /* Writing unsupported bits to a fixed counter */ + return -EINVAL; + core2_vpmu_cxt = vpmu->context; enabled_cntrs = vpmu->priv_context; switch ( msr ) { case MSR_CORE_PERF_GLOBAL_OVF_CTRL: core2_vpmu_cxt->global_status &= ~msr_content; - return 1; + return 0; case MSR_CORE_PERF_GLOBAL_STATUS: gdprintk(XENLOG_INFO, "Can not write readonly MSR: " "MSR_PERF_GLOBAL_STATUS(0x38E)!\n"); - hvm_inject_hw_exception(TRAP_gp_fault, 0); - return 1; + return -EINVAL;Is it intentional that you convert a success to a failure here? If so, this should be mentioned (with reason) in the commit message. If not, this should be adjusted to there's no behavioral change here.
Yes, this is intentional. Until now return value indicated whether access was to a PMU register. This worked for HVM guests since they can do hvm_inject_trap() at any time. For PV guests we are called from emulate_privileged_op() and we need to know whether access was successful or not. This way emulate_privileged_op() will take care of fault injection by returning 0.
-boris _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
