Re: [pvops-dom0] Adding MCA logging support in pv_ops

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 08/06/09 19:32, Ke, Liping wrote:
> Hi, Jeremy and all
>
> This is the new formated patch and I have tried linux patch format check tools on it.
>   

Thanks.  I have a couple of little comments:

    * when referring to Xen hg changesets, use the hex changeset ID
      rather than the number, since the number only makes sense locally
      (ie, your change 902 may not match anyone else's)
    * don't put "dom0" in the driver filename; drivers/xen/mce.c is
      enough.  Same with the various symbols and messages mentioning dom0.
    * don't use "#if define" if #ifdef would do; but try to avoid #ifdef
      anyway.  In particular, this *definitely* shouldn't be duplicating
      code between the #if and #else blocks.  At the very least, you
      should just need one #ifdef to re-add the MCE/MCA flags back into
      the mask.

      +#if !defined(CONFIG_XEN_MCE)
       	cpuid_leaf1_edx_mask =
       		~((1 << X86_FEATURE_MCE)  |  /* disable MCE */
       		  (1 << X86_FEATURE_MCA)  |  /* disable MCA */
       		  (1 << X86_FEATURE_PAT)  |  /* disable PAT */
       		  (1 << X86_FEATURE_ACC));   /* thermal monitoring */
      -
      +#else
      +	cpuid_leaf1_edx_mask =
      +		~((1 << X86_FEATURE_PAT)  |  /* disable PAT */
      +		  (1 << X86_FEATURE_ACC));   /* thermal monitoring */
      +#endif
          


Also, if you include the patch inline it makes it easier to review and
comment on.

Thanks,
    J