[PATCH] mtrr: refine memory type above 4GB on AMD platforms

[Alex Hung]

On 2020-11-17 4:29 a.m., Colin Ian King wrote:
> On 17/11/2020 03:38, Alex Hung wrote:
>> Buglink: https://bugs.launchpad.net/bugs/1901146
>>
>> 1. amd_Tom2ForceMemTypeWB only works from 4GB and top of memory 2 (TOM2)
>> 2. amd_Tom2ForceMemTypeWB can be overridden by MTRR
>> 3. amd_Tom2ForceMemTypeWB overrides MTRR_DEF_TYPE
>>
>> Signed-off-by: Alex Hung <alex.hung at canonical.com>
>> ---
>>  src/bios/mtrr/mtrr.c | 16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/bios/mtrr/mtrr.c b/src/bios/mtrr/mtrr.c
>> index e064f9c4..c9b77941 100644
>> --- a/src/bios/mtrr/mtrr.c
>> +++ b/src/bios/mtrr/mtrr.c
>> @@ -51,8 +51,10 @@ static fwts_cpuinfo_x86 *fwts_cpuinfo;
>>  
>>  #define MTRR_DEF_TYPE_MSR	0x2FF
>>  #define AMD_SYS_CFG_MSR		0xC0010010
>> +#define AMD_TOM2_MSR		0xC001001D
>>  
>>  static	uint64_t mtrr_default;
>> +static	uint64_t amd_tom2_addr;
>>  static	bool amd_Tom2ForceMemTypeWB = false;
>>  
>>  struct mtrr_entry {
>> @@ -195,6 +197,9 @@ static int get_default_mtrr(fwts_framework *fw)
>>  		if (fwts_cpu_readmsr(fw, 0, AMD_SYS_CFG_MSR, &amd_sys_conf) == FWTS_OK)
>>  			if (amd_sys_conf & 0x200000)
>>  				amd_Tom2ForceMemTypeWB = true;
>> +
>> +		if (fwts_cpu_readmsr(fw, 0, AMD_TOM2_MSR, &amd_tom2_addr) != FWTS_OK)
>> +			amd_tom2_addr = 0x100000000;	// this should above 4G
> 
> I'd rather not have // style comments in fwts if that's OK.

Thanks for pointing this out.

> 
>>  	}
>>  
>>  	if (fwts_cpu_readmsr(fw, 0, MTRR_DEF_TYPE_MSR, &mtrr_default) == FWTS_OK) {
>> @@ -229,12 +234,6 @@ static int cache_types(uint64_t start, uint64_t end)
>>  	struct mtrr_entry *entry;
>>  	int type = 0;
>>  
>> -	/* On AMD platforms, Tom2ForceMemTypeWB overwrites other memory types */
>> -	if (amd_Tom2ForceMemTypeWB && start >= 0x100000000) {
>> -		type = WRITE_BACK;
>> -		return type;
>> -	}
>> -
>>  	fwts_list_foreach(item, mtrr_list) {
>>  		entry = fwts_list_data(struct mtrr_entry*, item);
>>  
>> @@ -242,6 +241,11 @@ static int cache_types(uint64_t start, uint64_t end)
>>  			type |= entry->type;
>>  	}
>>  
>> +	/* On AMD platforms, Tom2ForceMemTypeWB overwrites MTRRdefType */
>> +	if (type == 0 && (strstr(fwts_cpuinfo->vendor_id, "AMD") || strstr(fwts_cpuinfo->vendor_id, "Hygon")))
>> +		if (amd_Tom2ForceMemTypeWB && start >= 0x100000000 && start <= amd_tom2_addr)
>> +			return WRITE_BACK;
>> +
> 
> The second if statement could be replaced with a further && clause,
> something like:
> 
> 	if ((type == 0) &&
>             (strstr(fwts_cpuinfo->vendor_id, "AMD") ||
> strstr(fwts_cpuinfo->vendor_id, "Hygon")) &&
>             (amd_Tom2ForceMemTypeWB && (start >= 0x100000000) && (start
> <= amd_tom2_addr)))

I just realized that CPU vendor IDs aren't necessary because
amd_Tom2ForceMemTypeWB will never be set without them in the first
place, and the variable name is also clear about this too.

Therefore, the following condition will be sufficient:

if (type == 0 && amd_Tom2ForceMemTypeWB && start >= 0x100000000 && start
<= amd_tom2_addr)


> 		return WRITE_BACK;
> 
> 
> 
>>  	/*
>>  	 * now to see if there is any part of the range that isn't
>>  	 * covered by an mtrr, since it's UNCACHED if so
>>
> 


-- 
Cheers,
Alex Hung