Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752369AbbFCCQE (ORCPT ); Tue, 2 Jun 2015 22:16:04 -0400 Received: from mga03.intel.com ([134.134.136.65]:51196 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751469AbbFCCPz (ORCPT ); Tue, 2 Jun 2015 22:15:55 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,544,1427785200"; d="scan'208";a="719730582" Message-ID: <556E6270.2060701@linux.intel.com> Date: Wed, 03 Jun 2015 10:12:00 +0800 From: Xiao Guangrong User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Paolo Bonzini CC: gleb@kernel.org, mtosatti@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 07/15] KVM: MTRR: improve kvm_mtrr_get_guest_memory_type References: <1432983566-15773-1-git-send-email-guangrong.xiao@linux.intel.com> <1432983566-15773-8-git-send-email-guangrong.xiao@linux.intel.com> <556C22FC.50505@redhat.com> In-Reply-To: <556C22FC.50505@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5527 Lines: 162 On 06/01/2015 05:16 PM, Paolo Bonzini wrote: > > > On 30/05/2015 12:59, Xiao Guangrong wrote: >> - kvm_mtrr_get_guest_memory_type() only checks one page in MTRRs so that >> it's unnecessary to check to see if the range is partially covered in >> MTRR >> >> - optimize the check of overlap memory type and add some comments to explain >> the precedence >> >> Signed-off-by: Xiao Guangrong >> --- >> arch/x86/kvm/mtrr.c | 89 ++++++++++++++++++++++++++--------------------------- >> 1 file changed, 44 insertions(+), 45 deletions(-) >> >> diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c >> index bc9c6da..d3c06d2 100644 >> --- a/arch/x86/kvm/mtrr.c >> +++ b/arch/x86/kvm/mtrr.c >> @@ -231,24 +231,16 @@ int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) >> return 0; >> } >> >> -/* >> - * The function is based on mtrr_type_lookup() in >> - * arch/x86/kernel/cpu/mtrr/generic.c >> - */ >> -static int get_mtrr_type(struct kvm_mtrr *mtrr_state, >> - u64 start, u64 end) >> +u8 kvm_mtrr_get_guest_memory_type(struct kvm_vcpu *vcpu, gfn_t gfn) >> { >> - u64 base, mask; >> - u8 prev_match, curr_match; >> - int i, num_var_ranges = KVM_NR_VAR_MTRR; >> + struct kvm_mtrr *mtrr_state = &vcpu->arch.mtrr_state; >> + u64 base, mask, start = gfn_to_gpa(gfn); >> + int i, num_var_ranges = KVM_NR_VAR_MTRR, type_mask, type = -1; > > Do not mix initialized and uninitialized variables on the same line > (preexisting, I know, but let's fix it instead of making it worse :)). > Please put each initialized variable on a separate line. Okay. Will follow this style in the future development. > > Also please initialize type_mask here (more on this below). > >> >> /* MTRR is completely disabled, use UC for all of physical memory. */ >> if (!mtrr_state->mtrr_enabled) >> return MTRR_TYPE_UNCACHABLE; >> >> - /* Make end inclusive end, instead of exclusive */ >> - end--; >> - >> /* Look in fixed ranges. Just return the type as per start */ >> if (mtrr_state->fixed_mtrr_enabled && (start < 0x100000)) { >> int idx; >> @@ -273,9 +265,9 @@ static int get_mtrr_type(struct kvm_mtrr *mtrr_state, >> * Look of multiple ranges matching this address and pick type >> * as per MTRR precedence >> */ >> - prev_match = 0xFF; >> + type_mask = (1 << MTRR_TYPE_WRBACK) | (1 << MTRR_TYPE_WRTHROUGH); >> for (i = 0; i < num_var_ranges; ++i) { >> - unsigned short start_state, end_state; >> + int curr_type; >> >> if (!(mtrr_state->var_ranges[i].mask & (1 << 11))) >> continue; >> @@ -283,50 +275,57 @@ static int get_mtrr_type(struct kvm_mtrr *mtrr_state, >> base = mtrr_state->var_ranges[i].base & PAGE_MASK; >> mask = mtrr_state->var_ranges[i].mask & PAGE_MASK; >> >> - start_state = ((start & mask) == (base & mask)); >> - end_state = ((end & mask) == (base & mask)); >> - if (start_state != end_state) >> - return 0xFE; >> - >> if ((start & mask) != (base & mask)) >> continue; >> >> - curr_match = mtrr_state->var_ranges[i].base & 0xff; >> - if (prev_match == 0xFF) { >> - prev_match = curr_match; >> + /* >> + * Please refer to Intel SDM Volume 3: 11.11.4.1 MTRR >> + * Precedences. >> + */ >> + >> + curr_type = mtrr_state->var_ranges[i].base & 0xff; >> + if (type == -1) { >> + type = curr_type; >> continue; >> } >> >> - if (prev_match == MTRR_TYPE_UNCACHABLE || >> - curr_match == MTRR_TYPE_UNCACHABLE) >> + /* >> + * If two or more variable memory ranges match and the >> + * memory types are identical, then that memory type is >> + * used. >> + */ >> + if (type == curr_type) >> + continue; >> + >> + /* >> + * If two or more variable memory ranges match and one of >> + * the memory types is UC, the UC memory type used. >> + */ >> + if (curr_type == MTRR_TYPE_UNCACHABLE) >> return MTRR_TYPE_UNCACHABLE; >> >> - if ((prev_match == MTRR_TYPE_WRBACK && >> - curr_match == MTRR_TYPE_WRTHROUGH) || >> - (prev_match == MTRR_TYPE_WRTHROUGH && >> - curr_match == MTRR_TYPE_WRBACK)) { >> - prev_match = MTRR_TYPE_WRTHROUGH; >> - curr_match = MTRR_TYPE_WRTHROUGH; >> + /* >> + * If two or more variable memory ranges match and the >> + * memory types are WT and WB, the WT memory type is used. >> + */ >> + if (((1 << type) & type_mask) && >> + ((1 << curr_type) & type_mask)) { > > Please inline definition of type_mask in the "if", or rename it to > "wt_wb_mask" and make it const. Or another suggestion below... Okay, it's a better name indeed. > >> + type = MTRR_TYPE_WRTHROUGH; >> + continue; >> } >> >> - if (prev_match != curr_match) >> - return MTRR_TYPE_UNCACHABLE; >> + /* >> + * For overlaps not defined by the above rules, processor >> + * behavior is undefined. >> + */ > > Perhaps just use type = MIN(type, curr_type), which also happens to get > WT vs. WB right? You can also add a > > BUILD_BUG_ON(MTRR_TYPE_WRTHROUGH > MTRR_TYPE_WRBACK); > > to ensure that the WT vs. WB precedence is correct. Only WT and WB are allowed to be overlapped here otherwise is "undefined behavior". For example if the types are MTRR_TYPE_WRPROT and MTRR_TYPE_WRTHROUGH, min() will get MTRR_TYPE_WRTHROUGH rather than "undefined behavior". -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/