Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752176AbbFAJ0L (ORCPT ); Mon, 1 Jun 2015 05:26:11 -0400 Received: from mail-wi0-f169.google.com ([209.85.212.169]:34915 "EHLO mail-wi0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751386AbbFAJ0H (ORCPT ); Mon, 1 Jun 2015 05:26:07 -0400 Message-ID: <556C2526.6090901@redhat.com> Date: Mon, 01 Jun 2015 11:25:58 +0200 From: Paolo Bonzini User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Xiao Guangrong CC: gleb@kernel.org, mtosatti@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 08/15] KVM: MTRR: introduce fixed_mtrr_segment table References: <1432983566-15773-1-git-send-email-guangrong.xiao@linux.intel.com> <1432983566-15773-9-git-send-email-guangrong.xiao@linux.intel.com> In-Reply-To: <1432983566-15773-9-git-send-email-guangrong.xiao@linux.intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7202 Lines: 247 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 > --- > 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. > + .range_size = 1ULL << 16, Perhaps 64 * 1024 (and so on below) is clearer, because it matches the name of the MSR? > + .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. > + 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. Paolo > - case MSR_MTRRfix64K_00000: > - start = 0x0; > - end = 0x80000; > - break; > - case MSR_MTRRfix16K_80000: > - start = 0x80000; > - end = 0xa0000; > - break; > - case MSR_MTRRfix16K_A0000: > - start = 0xa0000; > - end = 0xc0000; > - break; > - case MSR_MTRRfix4K_C0000 ... MSR_MTRRfix4K_F8000: > - index = msr - MSR_MTRRfix4K_C0000; > - start = 0xc0000 + index * (32 << 10); > - end = start + (32 << 10); > - break; > case MSR_MTRRdefType: > - is_fixed = false; > start = 0x0; > end = ~0ULL; > break; > default: > /* variable range MTRRs. */ > - is_fixed = false; > index = (msr - 0x200) / 2; > start = mtrr_state->var_ranges[index].base & PAGE_MASK; > mask = mtrr_state->var_ranges[index].mask & PAGE_MASK; > @@ -150,38 +251,33 @@ static void update_mtrr(struct kvm_vcpu *vcpu, u32 msr) > end = ((start & mask) | ~mask) + 1; > } > > - if (is_fixed && !mtrr_state->fixed_mtrr_enabled) > - return; > - > +do_zap: > kvm_zap_gfn_range(vcpu->kvm, gpa_to_gfn(start), gpa_to_gfn(end)); > } > > int kvm_mtrr_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data) > { > - u64 *p = (u64 *)&vcpu->arch.mtrr_state.fixed_ranges; > + int index; > > if (!kvm_mtrr_valid(vcpu, msr, data)) > return 1; > > - if (msr == MSR_MTRRdefType) > + index = fixed_msr_to_range_index(msr); > + if (index >= 0) > + *(u64 *)&vcpu->arch.mtrr_state.fixed_ranges[index] = data; > + else if (msr == MSR_MTRRdefType) > vcpu->arch.mtrr_state.deftype = data; > - else if (msr == MSR_MTRRfix64K_00000) > - p[0] = data; > - else if (msr == MSR_MTRRfix16K_80000 || msr == MSR_MTRRfix16K_A0000) > - p[1 + msr - MSR_MTRRfix16K_80000] = data; > - else if (msr >= MSR_MTRRfix4K_C0000 && msr <= MSR_MTRRfix4K_F8000) > - p[3 + msr - MSR_MTRRfix4K_C0000] = data; > else if (msr == MSR_IA32_CR_PAT) > vcpu->arch.pat = data; > else { /* Variable MTRRs */ > - int idx, is_mtrr_mask; > + int is_mtrr_mask; > > - idx = (msr - 0x200) / 2; > - is_mtrr_mask = msr - 0x200 - 2 * idx; > + index = (msr - 0x200) / 2; > + is_mtrr_mask = msr - 0x200 - 2 * index; > if (!is_mtrr_mask) > - vcpu->arch.mtrr_state.var_ranges[idx].base = data; > + vcpu->arch.mtrr_state.var_ranges[index].base = data; > else > - vcpu->arch.mtrr_state.var_ranges[idx].mask = data; > + vcpu->arch.mtrr_state.var_ranges[index].mask = data; > } > > update_mtrr(vcpu, msr); > @@ -190,7 +286,7 @@ int kvm_mtrr_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data) > > int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) > { > - u64 *p = (u64 *)&vcpu->arch.mtrr_state.fixed_ranges; > + int index; > > /* MSR_MTRRcap is a readonly MSR. */ > if (msr == MSR_MTRRcap) { > @@ -207,25 +303,22 @@ int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) > if (!msr_mtrr_valid(msr)) > return 1; > > - if (msr == MSR_MTRRdefType) > + index = fixed_msr_to_range_index(msr); > + if (index >= 0) > + *pdata = *(u64 *)&vcpu->arch.mtrr_state.fixed_ranges[index]; > + else if (msr == MSR_MTRRdefType) > *pdata = vcpu->arch.mtrr_state.deftype; > - else if (msr == MSR_MTRRfix64K_00000) > - *pdata = p[0]; > - else if (msr == MSR_MTRRfix16K_80000 || msr == MSR_MTRRfix16K_A0000) > - *pdata = p[1 + msr - MSR_MTRRfix16K_80000]; > - else if (msr >= MSR_MTRRfix4K_C0000 && msr <= MSR_MTRRfix4K_F8000) > - *pdata = p[3 + msr - MSR_MTRRfix4K_C0000]; > else if (msr == MSR_IA32_CR_PAT) > *pdata = vcpu->arch.pat; > else { /* Variable MTRRs */ > - int idx, is_mtrr_mask; > + int is_mtrr_mask; > > - idx = (msr - 0x200) / 2; > - is_mtrr_mask = msr - 0x200 - 2 * idx; > + index = (msr - 0x200) / 2; > + is_mtrr_mask = msr - 0x200 - 2 * index; > if (!is_mtrr_mask) > - *pdata = vcpu->arch.mtrr_state.var_ranges[idx].base; > + *pdata = vcpu->arch.mtrr_state.var_ranges[index].base; > else > - *pdata = vcpu->arch.mtrr_state.var_ranges[idx].mask; > + *pdata = vcpu->arch.mtrr_state.var_ranges[index].mask; > } > > return 0; > -- 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/