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

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

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.