Messages in this thread |  | | Date | Wed, 03 Jun 2015 10:29:34 +0800 | From | Xiao Guangrong <> | Subject | Re: [PATCH 08/15] KVM: MTRR: introduce fixed_mtrr_segment table |
| |
On 06/01/2015 05:25 PM, Paolo Bonzini wrote: > > > On 30/05/2015 12:59, Xiao Guangrong wrote: >> This table summarizes the information of fixed MTRRs and introduce some APIs >> to abstract its operation which helps us to clean up the code and will be >> used in later patches >> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >> --- >> arch/x86/kvm/mtrr.c | 191 ++++++++++++++++++++++++++++++++++++++-------------- >> 1 file changed, 142 insertions(+), 49 deletions(-) >> >> diff --git a/arch/x86/kvm/mtrr.c b/arch/x86/kvm/mtrr.c >> index d3c06d2..888441e 100644 >> --- a/arch/x86/kvm/mtrr.c >> +++ b/arch/x86/kvm/mtrr.c >> @@ -102,12 +102,126 @@ bool kvm_mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data) >> } >> EXPORT_SYMBOL_GPL(kvm_mtrr_valid); >> >> +struct fixed_mtrr_segment { >> + u64 start; >> + u64 end; >> + >> + /* >> + * unit corresponds to the MSR entry in this segment, the size >> + * of unit is covered in one MSR. >> + */ >> + u64 unit_size; >> + >> + /* a range is covered in one memory cache type. */ >> + u64 range_size; >> + >> + /* the start position in kvm_mtrr.fixed_ranges[]. */ >> + int range_start; >> +}; >> + >> +static struct fixed_mtrr_segment fixed_seg_table[] = { >> + /* MSR_MTRRfix64K_00000, 1 unit. 64K fixed mtrr. */ >> + { >> + .start = 0x0, >> + .end = 0x80000, >> + .unit_size = 0x80000, > > Unit size is always range size * 8.
Good catch, will drop this.
> >> + .range_size = 1ULL << 16, > > Perhaps 64 * 1024 (and so on below) is clearer, because it matches the > name of the MSR?
Yes, it's better.
> >> + .range_start = 0, >> + }, >> + >> + /* >> + * MSR_MTRRfix16K_80000 ... MSR_MTRRfix16K_A0000, 2 units, >> + * 16K fixed mtrr. >> + */ >> + { >> + .start = 0x80000, >> + .end = 0xc0000, >> + .unit_size = (0xc0000 - 0x80000) / 2, >> + .range_size = 1ULL << 14, >> + .range_start = 8, >> + }, >> + >> + /* >> + * MSR_MTRRfix4K_C0000 ... MSR_MTRRfix4K_F8000, 8 units, >> + * 4K fixed mtrr. >> + */ >> + { >> + .start = 0xc0000, >> + .end = 0x100000, >> + .unit_size = (0x100000 - 0xc0000) / 8, >> + .range_size = 1ULL << 12, >> + .range_start = 24, >> + } >> +}; >> + >> +static int fixed_msr_to_range_index(u32 msr) >> +{ >> + int seg, unit; >> + >> + if (!fixed_msr_to_seg_unit(msr, &seg, &unit)) >> + return -1; >> + >> + fixed_mtrr_seg_unit_range_index(seg, unit); > > This looks wrong.
Oops, should be "return fixed_mtrr_seg_unit_range_index(seg, unit);" :(
> >> + return 0; >> +} >> + >> static void update_mtrr(struct kvm_vcpu *vcpu, u32 msr) >> { >> struct kvm_mtrr *mtrr_state = &vcpu->arch.mtrr_state; >> gfn_t start, end, mask; >> int index; >> - bool is_fixed = true; >> >> if (msr == MSR_IA32_CR_PAT || !tdp_enabled || >> !kvm_arch_has_noncoherent_dma(vcpu->kvm)) >> @@ -116,32 +230,19 @@ static void update_mtrr(struct kvm_vcpu *vcpu, u32 msr) >> if (!mtrr_state->mtrr_enabled && msr != MSR_MTRRdefType) >> return; >> >> + if (fixed_msr_to_range(msr, &start, &end)) { >> + if (!mtrr_state->fixed_mtrr_enabled) >> + return; >> + goto do_zap; >> + } >> + >> switch (msr) { > > Please move defType handling in an "if" above "if > (fixed_msr_to_range(msr, &start, &end))". Then you don't need the goto. >
Yes, indeed. Will do it in the next version.
|  |