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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 03/15/2008 05:50 PM, Aravind Gopalakrishnan wrote:
> vmce_amd_[rd|wr]msr functions can handle accesses to AMD thresholding
> registers. But due to this statement here:
> switch ( msr & (MSR_IA32_MC0_CTL | 3) )
> we are wrongly masking off top two bits and bit 4 which meant the
> register accesses never made it to vmce_amd_* functions.
>
> We correct this problem by modifying the mask in this patch to allow
> AMD thresholding registers to fall to 'default' case which in turn
> allows vmce_amd_* functions to handle access to the registers.
>
> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>   xen/arch/x86/cpu/mcheck/vmce.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c
> index f6c35db..cb4fd12 100644
> --- 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))

Which MSRs are going to be handled in the non-default cases? 
MSR0000_040[0:3] and MSRC000_040[0:3]? The first four already have 
explicit cases and I think it would be more readable if you explicitly 
created case statements for the latter four and had a simple 'switch(msr)'.

In fact, do MSRC000_040[0:3] even exist?

(You may also want to adjust your clock --- your emails are being sent 
from distant past ;-) )

-boris



>       {
>       case MSR_IA32_MC0_CTL:
>           /* stick all 1's to MCi_CTL */
> @@ -210,7 +210,7 @@ static int bank_mce_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>       int ret = 1;
>       unsigned int bank = (msr - MSR_IA32_MC0_CTL) / 4;
>   
> -    switch ( msr & (MSR_IA32_MC0_CTL | 3) )
> +    switch ( msr & (MSR_IA32_MC0_CTL | (0x3 << 30) | 0x13))
>       {
>       case MSR_IA32_MC0_CTL:
>           /*