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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

> +		.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;
>