linux-kernel - Re: [PATCH v4 14/19] x86/resctrl: Add the interface to assign ABMC counter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 21 Jun 2024 11:07:15 -0500
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, babu.moger@....com,
 corbet@....net, fenghua.yu@...el.com, tglx@...utronix.de, mingo@...hat.com,
 bp@...en8.de, dave.hansen@...ux.intel.com
Cc: x86@...nel.org, hpa@...or.com, paulmck@...nel.org, rdunlap@...radead.org,
 tj@...nel.org, peterz@...radead.org, yanjiewtw@...il.com,
 kim.phillips@....com, lukas.bulwahn@...il.com, seanjc@...gle.com,
 jmattson@...gle.com, leitao@...ian.org, jpoimboe@...nel.org,
 rick.p.edgecombe@...el.com, kirill.shutemov@...ux.intel.com,
 jithu.joseph@...el.com, kai.huang@...el.com, kan.liang@...ux.intel.com,
 daniel.sneddon@...ux.intel.com, pbonzini@...hat.com, sandipan.das@....com,
 ilpo.jarvinen@...ux.intel.com, peternewman@...gle.com,
 maciej.wieczor-retman@...el.com, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, eranian@...gle.com, james.morse@....com
Subject: Re: [PATCH v4 14/19] x86/resctrl: Add the interface to assign ABMC
 counter

Hi Reinette,

On 6/20/2024 5:50 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/19/24 3:38 PM, Moger, Babu wrote:
>> On 6/13/2024 8:48 PM, Reinette Chatre wrote:
>>> On 5/24/24 5:23 AM, Babu Moger wrote:
> 
>>>> +int resctrl_arch_assign(struct rdt_domain *d, u32 evtid, u32 rmid,
>>>> +            u32 ctr_id, u32 closid, bool enable)
>>>> +{
>>>> +    struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
>>>> +    union l3_qos_abmc_cfg abmc_cfg = { 0 };
>>>> +    struct arch_mbm_state *arch_mbm;
>>>> +
>>>> +    abmc_cfg.split.cfg_en = 1;
>>>> +    abmc_cfg.split.ctr_en = enable ? 1 : 0;
>>>> +    abmc_cfg.split.ctr_id = ctr_id;
>>>> +    abmc_cfg.split.bw_src = rmid;
>>>> +
>>>> +    /*
>>>> +     * Read the event configuration from the domain and pass it as
>>>> +     * bw_type.
>>>> +     */
>>>> +    if (evtid == QOS_L3_MBM_TOTAL_EVENT_ID) {
>>>> +        abmc_cfg.split.bw_type = hw_dom->mbm_total_cfg;
>>>> +        arch_mbm = &hw_dom->arch_mbm_total[rmid];
>>>> +    } else {
>>>> +        abmc_cfg.split.bw_type = hw_dom->mbm_local_cfg;
>>>> +        arch_mbm = &hw_dom->arch_mbm_local[rmid];
>>>> +    }
>>>> +
>>>> +    smp_call_function_any(&d->cpu_mask, rdtgroup_abmc_cfg, 
>>>> &abmc_cfg, 1);
>>>> +
>>>> +    /* Reset the internal counters */
>>>
>>> "internal counters"? This needs a definition ... but since this is not
>>> a new data structure the comment can be more specific about what is done
>>> and why.
>>
>> How about?
>> /* Reset internal mbm event counters. */
> 
> I think it is potentially confusing to introduce a new concept of
> "internal counters". This is just the architectural state helping to
> detect overflows. This commit has no mention of this but I assume that
> the re-configuration of the counter will reset it, which explains
> why it is required to reset the state maintained by software.
> A similar comment to what can be found in mbm_config_write_domain()
> would be useful. This does not have to be so elaborate, maybe just 
> something
> like (please feel free to improve):
> 
>      Configuring a hardware counter resets it. Reset architectural
>      state to match.
> 

Sure. Thanks
- Babu Moger