Hi Babu,
On 5/27/25 11:40 AM, Moger, Babu wrote:
> On 5/27/25 12:54, Reinette Chatre wrote:
>> On 5/27/25 10:23 AM, Moger, Babu wrote:
>>> On 5/22/25 15:51, Reinette Chatre wrote:
>>>> On 5/15/25 3:51 PM, Babu Moger wrote:
>>
>>>>> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
>>>>> index a2fbea0be535..2f54831e04e5 100644
>>>>> --- a/arch/x86/kernel/cpu/cpuid-deps.c
>>>>> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
>>>>> @@ -71,6 +71,8 @@ static const struct cpuid_dep cpuid_deps[] = {
>>>>> { X86_FEATURE_CQM_MBM_LOCAL, X86_FEATURE_CQM_LLC },
>>>>> { X86_FEATURE_BMEC, X86_FEATURE_CQM_MBM_TOTAL },
>>>>> { X86_FEATURE_BMEC, X86_FEATURE_CQM_MBM_LOCAL },
>>>>> + { X86_FEATURE_ABMC, X86_FEATURE_CQM_MBM_TOTAL },
>>>>> + { X86_FEATURE_ABMC, X86_FEATURE_CQM_MBM_LOCAL },
>>>>
>>>> Is this dependency still accurate now that the implementation switched to the
>>>> "extended event ID" variant of ABMC that no longer uses the event IDs associated
>>>> with X86_FEATURE_CQM_MBM_TOTAL and X86_FEATURE_CQM_MBM_LOCAL?
>>>
>>> That's a good question. Unfortunately, we may need to retain this
>>> dependency for now, as a significant portion of the code relies on
>>> functions like resctrl_is_mbm_event(), resctrl_is_mbm_enabled(),
>>> resctrl_arch_is_mbm_total_enabled(), and others.
>>>
>>
>> Avoiding needing to change code is not a valid reason.
>>
>> I think that without this dependency the code will
>> still rely on "functions like resctrl_is_mbm_event(),
>> resctrl_is_mbm_enabled(), resctrl_arch_is_mbm_total_enabled(),
>> and others." though.
>>
>> The core shift is to stop thinking about QOS_L3_MBM_TOTAL_EVENT_ID
>> to mean the same as X86_FEATURE_CQM_MBM_TOTAL, similarly to stop
>> thinking about QOS_L3_MBM_LOCAL_EVENT_ID to mean the same as
>> X86_FEATURE_CQM_MBM_LOCAL.
>
> oh. ok.
>
>>
>> I expected that for backwards compatibility ABMC will start by
>> enabling QOS_L3_MBM_TOTAL_EVENT_ID and QOS_L3_MBM_LOCAL_EVENT_ID
>> as part of its initialization, configuring them with the current
>> defaults for which memory transactions are expected to be monitored
>> by each. With these events enabled the existing flows using, for
>> example, resctrl_is_mbm_event(), will continue to work as expected, no?
>
> Yes. It will work as it uses event id.
>>
>> This would require more familiarity with L3 monitoring enumeration
>> on AMD since it will still be required to determine the number of
>> RMIDs etc. but if ABMC does not actually depend on these CQM features
>> then the current enumeration would need to be re-worked anyway.
>
> Are you suggesting to remove the dependency and rework ABMC enumeration in
> get_rdt_mon_resources()?
>
If you have an alternative proposal that would accurately reflect the ABMC
and existing L3 MON features then we can surely consider it.
Reinette
