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

["Liu, Jinsong" <jinsong.liu@xxxxxxxxx>]

Jan Beulich wrote:
>>>> On 28.02.13 at 09:05, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>> 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.
> 
> Like this (RFC only, untested so far), based on having gone through
> all call sites of mca_wrmsr():
> 
> --- a/xen/arch/x86/cpu/mcheck/mce.c
> +++ b/xen/arch/x86/cpu/mcheck/mce.c
> @@ -1065,13 +1065,15 @@ static void intpose_add(unsigned int cpu
>      printk("intpose_add: interpose array full - request dropped\n");
>  }
> 
> -void intpose_inval(unsigned int cpu_nr, uint64_t msr)
> +bool_t intpose_inval(unsigned int cpu_nr, uint64_t msr)
>  {
> -    struct intpose_ent *ent;
> +    struct intpose_ent *ent = intpose_lookup(cpu_nr, msr, NULL);
> 
> -    if ((ent = intpose_lookup(cpu_nr, msr, NULL)) != NULL) {
> -        ent->cpu_nr = -1;
> -    }
> +    if ( !ent )
> +        return 0;
> +
> +    ent->cpu_nr = -1;
> +    return 1;
>  }
> 
>  #define IS_MCA_BANKREG(r) \
> --- a/xen/arch/x86/cpu/mcheck/mce.h
> +++ b/xen/arch/x86/cpu/mcheck/mce.h
> @@ -77,7 +77,7 @@ extern void mce_recoverable_register(mce
>  /* Read an MSR, checking for an interposed value first */
>  extern struct intpose_ent *intpose_lookup(unsigned int, uint64_t,
>      uint64_t *);
> -extern void intpose_inval(unsigned int, uint64_t);
> +extern bool_t intpose_inval(unsigned int, uint64_t);
> 
>  static inline uint64_t mca_rdmsr(unsigned int msr)
>  {
> @@ -89,9 +89,9 @@ static inline uint64_t mca_rdmsr(unsigne
> 
>  /* Write an MSR, invalidating any interposed value */
>  #define mca_wrmsr(msr, val) do { \
> -       intpose_inval(smp_processor_id(), msr); \
> -       wrmsrl(msr, val); \
> -} while (0)
> +    if ( !intpose_inval(smp_processor_id(), msr) ) \
> +        wrmsrl(msr, val); \
> +} while ( 0 )
> 
> 
>  /* Utility function to "logout" all architectural MCA telemetry from
> the MCA 

No, it doesn't work, considering mce broadcast to all cpus, while s/w only 
simulate 1 cpu.

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