Re: [PATCH v4 15/21] x86/resctrl: Abstract __rmid_read()

[James Morse <james.morse@arm.com>]

Hi Reinette,

On 17/05/2022 22:23, Reinette Chatre wrote:
> On 4/12/2022 5:44 AM, James Morse wrote:
> 
>> @@ -180,14 +180,24 @@ static u64 __rmid_read(u32 rmid, enum resctrl_event_id eventid)
>>  	 * are error bits.
>>  	 */
>>  	wrmsr(MSR_IA32_QM_EVTSEL, eventid, rmid);
>> -	rdmsrl(MSR_IA32_QM_CTR, val);
>> +	rdmsrl(MSR_IA32_QM_CTR, msr_val);
>>  
>> -	return val;
>> +	if (msr_val & RMID_VAL_ERROR)
>> +		return -EIO;
>> +	if (msr_val & RMID_VAL_UNAVAIL)
>> +		return -EINVAL;
>> +
>> +	*val = msr_val;
>> +
>> +	return 0;
>>  }
>>  
> 
> In above EIO is used to represent RMID_VAL_ERROR ...
> 
>> @@ -343,7 +355,7 @@ static u64 __mon_event_count(u32 rmid, struct rmid_read *rr)
>>  		 * Code would never reach here because an invalid
>>  		 * event id would fail the __rmid_read.

(I'll fix this comment)

>>  		 */
>> -		return RMID_VAL_ERROR;
>> +		return -EINVAL;
>>  	}
>>  
>>  	if (rr->first) {
> 
> I understand it can be seen as a symbolic change but could
> RMID_VAL_ERROR consistently be associated with the same error?

This one isn't really RMID_VAL_ERROR - it was never read from the hardware, this was an
invalid argument supplied by the caller.

You can only hit this if resctrl_arch_rmid_read() doesn't read RMID_VAL_ERROR from the
hardware, because the hardware supports the event, but its an invalid argument as far as
this code is concerned.

I'd prefer to avoid EIO as the error was not reported from hardware - its only reachable
if the hardware does support the event!


Thanks,

James