[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 2/2 V2] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs

On 1/28/2014 5:24 AM, Jan Beulich wrote:
On 27.01.14 at 23:44, Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx> 
--- a/xen/arch/x86/cpu/mcheck/vmce.c
+++ b/xen/arch/x86/cpu/mcheck/vmce.c
@@ -107,7 +107,7 @@ static int bank_mce_rdmsr(const struct vcpu *v, uint32_t 
msr, uint64_t *val)
*val = 0; - switch ( msr & (MSR_IA32_MC0_CTL | 3) )
+    switch ( msr & (MSR_IA32_MC0_CTL | (0x3 << 30) | 0x13))
As one of the other reviewers already said - 0xC0000000 would
be better recognizable here.

As to the 3 -> 0x13 change - I don't think this is conceptually
correct. While at present we emulate only 2 banks, this had
been different in the past and may become different again.
Hence introducing a dis-contiguity after bank 3 is undesirable.

IMHO, including the '0x13' is necessary. The reason is that 0x413, 0xc0000408 and 0xc0000409 together form the set of MC4 thresholding registers. Not including 0x13 in the mask would mean that accesses to 0x413 alone would not be handled. (which would be confusing if someone new
were to look into the mce codebase)

Also, (in response to Boris's comments) - AFAICT, this should not affect Intel codepath.. Intel's vmce_* functions only care about MSR_IA32_MC0_CTL2 = 0x00000280 (if bank0) or 0x00000281 (if bank 1) and, having 0x13 in the mask does not affect the ability of bank_mce_[rd|wr]msr functions to call into vmce_intel*
(I haven't tested this, so if someone could test and let me know, that'd be great.)

I have addressed Christoph's and Boris.O's earlier comments too in the next version of this patch.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.