Re: [PATCH 08/15] KVM: MTRR: introduce fixed_mtrr_segment table

From: Xiao Guangrong
Date: Tue Jun 02 2015 - 22:33:46 EST




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@xxxxxxxxxxxxxxx>
---
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/