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

[Reinette Chatre]

Hi James,

On 6/7/2022 5:07 AM, James Morse wrote:
> 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!

ok, yes, that is fair. I believe no functional change is intended with this change so
please do highlight any such change(s).

Reinette