| Message ID | 1430389490-24602-12-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 30, 2015 at 3:24 AM, <guangrong.xiao@linux.intel.com> wrote: > From: Xiao Guangrong <guangrong.xiao@linux.intel.com> > > There are some bugs in current get_mtrr_type(); > 1: bit 2 of mtrr_state->enabled is corresponding bit 11 of IA32_MTRR_DEF_TYPE bit 1, not bit 2. (code is correct though) > MSR which completely control MTRR's enablement that means other bits are > ignored if it is cleared > > 2: the fixed MTRR ranges are controlled by bit 1 of mtrr_state->enabled (bit 10 bit 0, not bit 1. (code is correct though) > of IA32_MTRR_DEF_TYPE) > > 3: if MTRR is disabled, UC is applied to all of physical memory rather than > mtrr_state->def_type kvm_get_guest_memory_type defaults to MTRR_TYPE_WRBACK, not mtrr_state->def_type, when get_mtrr_type returns 0xFF. > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- > arch/x86/kvm/mmu.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index d43867c..ea3e3e4 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -2393,19 +2393,20 @@ EXPORT_SYMBOL_GPL(kvm_mmu_unprotect_page); > static int get_mtrr_type(struct mtrr_state_type *mtrr_state, > u64 start, u64 end) > { > - int i; > u64 base, mask; > u8 prev_match, curr_match; > - int num_var_ranges = KVM_NR_VAR_MTRR; > + int i, num_var_ranges = KVM_NR_VAR_MTRR; > > - if (!mtrr_state->enabled) > - return 0xFF; > + /* MTRR is completely disabled, use UC for all of physical memory. */ > + if (!(mtrr_state->enabled & 0x2)) > + 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->have_fixed && (start < 0x100000)) { > + if (mtrr_state->have_fixed && (mtrr_state->enabled & 0x1) && > + (start < 0x100000)) { > int idx; > > if (start < 0x80000) { > @@ -2428,9 +2429,6 @@ static int get_mtrr_type(struct mtrr_state_type *mtrr_state, > * Look of multiple ranges matching this address and pick type > * as per MTRR precedence > */ > - if (!(mtrr_state->enabled & 2)) > - return mtrr_state->def_type; > - > prev_match = 0xFF; > for (i = 0; i < num_var_ranges; ++i) { > unsigned short start_state, end_state; > -- > 1.9.3 > > -- > 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/ -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/07/2015 05:42 AM, David Matlack wrote: > On Thu, Apr 30, 2015 at 3:24 AM, <guangrong.xiao@linux.intel.com> wrote: >> From: Xiao Guangrong <guangrong.xiao@linux.intel.com> >> >> There are some bugs in current get_mtrr_type(); >> 1: bit 2 of mtrr_state->enabled is corresponding bit 11 of IA32_MTRR_DEF_TYPE > > bit 1, not bit 2. (code is correct though) Oh, i counted the bit from 1, my fault. :( > >> MSR which completely control MTRR's enablement that means other bits are >> ignored if it is cleared >> >> 2: the fixed MTRR ranges are controlled by bit 1 of mtrr_state->enabled (bit 10 > > bit 0, not bit 1. (code is correct though) Ditto. Will update the changelog in v2. Thank you for pointing it out. > >> of IA32_MTRR_DEF_TYPE) >> >> 3: if MTRR is disabled, UC is applied to all of physical memory rather than >> mtrr_state->def_type > > kvm_get_guest_memory_type defaults to MTRR_TYPE_WRBACK, not > mtrr_state->def_type, when get_mtrr_type returns 0xFF. > Yeah, that confused me. Based on the comment of vmx_get_mt_mask(): * a. VT-d without snooping control feature: can't guarantee the * result, try to trust guest. we need to completely follow guest's MTRR under this case. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index d43867c..ea3e3e4 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -2393,19 +2393,20 @@ EXPORT_SYMBOL_GPL(kvm_mmu_unprotect_page); static int get_mtrr_type(struct mtrr_state_type *mtrr_state, u64 start, u64 end) { - int i; u64 base, mask; u8 prev_match, curr_match; - int num_var_ranges = KVM_NR_VAR_MTRR; + int i, num_var_ranges = KVM_NR_VAR_MTRR; - if (!mtrr_state->enabled) - return 0xFF; + /* MTRR is completely disabled, use UC for all of physical memory. */ + if (!(mtrr_state->enabled & 0x2)) + 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->have_fixed && (start < 0x100000)) { + if (mtrr_state->have_fixed && (mtrr_state->enabled & 0x1) && + (start < 0x100000)) { int idx; if (start < 0x80000) { @@ -2428,9 +2429,6 @@ static int get_mtrr_type(struct mtrr_state_type *mtrr_state, * Look of multiple ranges matching this address and pick type * as per MTRR precedence */ - if (!(mtrr_state->enabled & 2)) - return mtrr_state->def_type; - prev_match = 0xFF; for (i = 0; i < num_var_ranges; ++i) { unsigned short start_state, end_state;