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

[Xiao Guangrong]

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/