From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: RE: [PATCH 1/9] Add cpu idle pwr mgmt to xen Date: Wed, 30 Apr 2008 08:22:07 +0100 Message-ID: <48183A3F.76E4.0078.0@novell.com> References: <094BCE01AFBE9646AF220B0B3F367AAB02FE2C6E@pdsmsx413.ccr.corp.intel.com> <094BCE01AFBE9646AF220B0B3F367AAB02FE2EC4@pdsmsx413.ccr.corp.intel.com> <4815B40A.76E4.0078.0@novell.com> <094BCE01AFBE9646AF220B0B3F367AAB03028932@pdsmsx413.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <094BCE01AFBE9646AF220B0B3F367AAB03028932@pdsmsx413.ccr.corp.intel.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Gang Wei Cc: xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org >>> "Wei, Gang" 30.04.08 05:27 >>> >Revising done according to Jan's comments. Resend. Thanks. Unfortunately you now use a static (but not per-CPU) variable - while I understand that it is expected that the call is done just once, I don't think this is a good thing to do. Further, xen_processor_csd_t seems to not need translation, so you could simply add a check for the type to xen/include/xlat.lst and copy the handle rather than what it points to. This would reduce size constraints on the xlat area and also simplify the code. As another suggestion - could you use uint32_t for the bitfield declarations, making it more obvious that the remaining bits in the 32-bit quantity are reserved? Alternatively, could you use an explicit padding field after the flags member of struct xen_processor_power? Also, I think there's error checking missing on copy_from_guest* throughout the patch. And I think I saw non-C89 constructs (loop variables declared inside for() statements). Jan