From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeremy Fitzhardinge Subject: Re: [pvops-dom0] Adding MCA logging support in pv_ops Date: Fri, 07 Aug 2009 11:07:35 -0700 Message-ID: <4A7C6D67.7090202@goop.org> References: <4A7174D3020000780000D350@vpn.id2.novell.com> <4A71D4F8.70804@goop.org> <4A7A0DF7.5040706@goop.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: "Ke, Liping" Cc: "Kleen, Andi" , Christoph Egger , "xen-devel@lists.xensource.com" , Keir Fraser , Jan Beulich List-Id: xen-devel@lists.xenproject.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