From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755321AbbCPVZ3 (ORCPT ); Mon, 16 Mar 2015 17:25:29 -0400 Received: from g9t5008.houston.hp.com ([15.240.92.66]:53741 "EHLO g9t5008.houston.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753905AbbCPVZ0 convert rfc822-to-8bit (ORCPT ); Mon, 16 Mar 2015 17:25:26 -0400 From: "Kani, Toshimitsu" To: Ingo Molnar CC: "akpm@linux-foundation.org" , "hpa@zytor.com" , "tglx@linutronix.de" , "mingo@redhat.com" , "linux-mm@kvack.org" , "x86@kernel.org" , "linux-kernel@vger.kernel.org" , "dave.hansen@intel.com" , "Elliott, Robert (Server Storage)" , "pebolle@tiscali.nl" Subject: Re: [PATCH v3 4/5] mtrr, x86: Clean up mtrr_type_lookup() Thread-Topic: [PATCH v3 4/5] mtrr, x86: Clean up mtrr_type_lookup() Thread-Index: AQHQXdWCoVOX6Xpa1EWJUWezQICeWp0ewkWAgADhKM0= Date: Mon, 16 Mar 2015 21:24:12 +0000 Message-ID: References: <1426282421-25385-1-git-send-email-toshi.kani@hp.com> <1426282421-25385-5-git-send-email-toshi.kani@hp.com>,<20150316075821.GA16062@gmail.com> In-Reply-To: <20150316075821.GA16062@gmail.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > On Mar 16, 2015, at 3:58 AM, Ingo Molnar wrote: > > > * Toshi Kani wrote: > >> MTRRs contain fixed and variable entries. mtrr_type_lookup() >> may repeatedly call __mtrr_type_lookup() to handle a request >> that overlaps with variable entries. However, >> __mtrr_type_lookup() also handles the fixed entries, which >> do not have to be repeated. Therefore, this patch creates >> separate functions, mtrr_type_lookup_fixed() and >> mtrr_type_lookup_variable(), to handle the fixed and variable >> ranges respectively. >> >> The patch also updates the function headers to clarify the >> return values and output argument. It updates comments to >> clarify that the repeating is necessary to handle overlaps >> with the default type, since overlaps with multiple entries >> alone can be handled without such repeating. >> >> There is no functional change in this patch. > > Nice cleanup! > > I also suggest adding a small table to the comments before the > function, that lists the fixed purpose MTRRs and their address ranges > - to make it more obvious what the magic hexadecimal constants within > the code are doing. Yes, I will add a table to describe the fixed entries. >> +static u8 mtrr_type_lookup_fixed(u64 start, u64 end) >> +{ >> + int idx; >> + >> + if (start >= 0x100000) >> + return 0xFF; > > Btw., as a separate cleanup patch, we should probably also change > '0xFF' (which is sometimes written as 0xff) to be some sufficiently > named constant, and explain its usage somewhere? Sounds good. I will add a separate patch to do so. >> + if (!(mtrr_state.have_fixed) || >> + !(mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED)) > > Btw., can MTRR_STATE_MTRR_FIXED_ENABLED ever be set in > mtrr_state.enabled, without mtrr_state.have_fixed being set? Yes, I believe the arch allows the fixed entries disabled while MTRRs are enabled. I expect the most of systems implement the fixed entries, though. > AFAICS get_mtrr_state() will only ever fill in mtrr_state with fixed > MTRRs if mtrr_state.have_fixed != 0 - but I might be mis-reading the > (rather convoluted) flow of code ... I will check the code next week. Thanks, -Toshi