Messages in this thread |  | | Date | Wed, 03 Jun 2015 10:12:00 +0800 | From | Xiao Guangrong <> | Subject | Re: [PATCH 07/15] KVM: MTRR: improve kvm_mtrr_get_guest_memory_type |
| |
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 <guangrong.xiao@linux.intel.com> >> --- >> 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".
|  |