Re: [Xen-devel] [PATCH 1/2] Xen/MCA: bugfix for mca bank clear

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.02.13 at 17:44, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> wrote:
> Xen/MCA: bugfix for mca bank clear
> 
> mcabank_clear() is common code for real h/w mca and s/w simulated mca.
> Under s/w simulated case, getting status via mca_rdmsr may trigger #GP
> if MCx_ADDR/MISC not supported by real h/w.
> 
> This patch fix the bug. It always invalidates intpose for s/w simulated
> mca case, and do check real h/w status ADDRV/MISCV to avoid #GP when
> clear MCx_ADDR/MISC for h/w mca case.
> 
> Reported-by: Ren Yongjie <yongjie.ren@xxxxxxxxx>
> Singed-off-by: Liu Jinsong <jinsong.liu@xxxxxxxxx>
> 
> diff -r e84a79d11d7a xen/arch/x86/cpu/mcheck/mce.c
> --- a/xen/arch/x86/cpu/mcheck/mce.c   Thu Nov 01 01:41:03 2012 +0800
> +++ b/xen/arch/x86/cpu/mcheck/mce.c   Thu Feb 28 08:05:15 2013 +0800
> @@ -138,17 +138,28 @@
>      xfree(banks);
>  }
>  
> +/*
> + * Common code for real h/w mca and s/w simulated mca.
> + * Always invalidate intpose for s/w simulated mca case, and do check
> + * real h/w status ADDRV/MISCV to avoid #GP when clear MCx_ADDR/MISC
> + * for h/w mca case.
> + */
>  static void mcabank_clear(int banknum)
>  {
> -    uint64_t status;
> +    uint64_t hw_status;
>  
> -    status = mca_rdmsr(MSR_IA32_MCx_STATUS(banknum));
> +    /* For s/w simulated mca case */
> +    intpose_inval(smp_processor_id(), MSR_IA32_MCx_ADDR(banknum));
> +    intpose_inval(smp_processor_id(), MSR_IA32_MCx_MISC(banknum));

And no invalidation of MSR_IA32_MCx_STATUS(banknum)?

>  
> -    if (status & MCi_STATUS_ADDRV)
> -        mca_wrmsr(MSR_IA32_MCx_ADDR(banknum), 0x0ULL);
> -    if (status & MCi_STATUS_MISCV)
> -        mca_wrmsr(MSR_IA32_MCx_MISC(banknum), 0x0ULL);
> +    /* For real h/w mca case */
> +    rdmsrl(MSR_IA32_MCx_STATUS(banknum), hw_status);
> +    if (hw_status & MCi_STATUS_ADDRV)
> +        wrmsrl(MSR_IA32_MCx_ADDR(banknum), 0x0ULL);
> +    if (hw_status & MCi_STATUS_MISCV)
> +        wrmsrl(MSR_IA32_MCx_MISC(banknum), 0x0ULL);
>  
> +    /* For both cases */
>      mca_wrmsr(MSR_IA32_MCx_STATUS(banknum), 0x0ULL);

Oh, that happens as a side effect of this one. Not really obvious,
so likely worth a comment.

>  }
>  

But anyway, I don't think that's quite right either: For one, I'm not
sure the ordering of the MSR writes can be freely exchanged
(originally you cleared STATUS first, while now it's done last).

And then I don't see why you would do physical MSR accesses in
the injection case at all. It should really be mca_wrmsr() to take
care of this, it just needs to have a way to know it got called in
the context of an injection. One fundamental question here is
why the invalidation of the interposed data is being done
before the MSR write rather than after. One thing we surely
don't care much about in the injection logic is interference with
a real #MC.

Jan


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