Re: [PATCH 4/7] x86/resctrl: Implement SDCIAE enable/disable

["Moger, Babu" <bmoger@amd.com>]

Hi Reinette,

On 9/19/2024 10:34 AM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/18/24 9:26 AM, Moger, Babu wrote:
>> On 9/13/24 15:46, Reinette Chatre wrote:
>>> On 8/16/24 9:16 AM, Babu Moger wrote:
>>>> SDCIAE feature can be enabled by setting bit 1 in MSR L3_QOS_EXT_CFG.
>>>> When the state of SDCIAE is changed, it must be changed to the updated
>>>> value on all logical processors in the QOS Domain. By default, the SDCIAE
>>>> feature is disabled.
>>>>
>>>> Introduce arch handlers to detect and enable/disable the feature.
>>>>
>>>> The SDCIAE feature details are available in APM listed below [1].
>>>> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
>>>> Publication # 24593 Revision 3.41 section 19.4.7 L3 Smart Data Cache
>>>> Injection Allocation Enforcement (SDCIAE)
>>>>
>>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>>>> ---
>>>>    arch/x86/include/asm/msr-index.h       |  1 +
>>>>    arch/x86/kernel/cpu/resctrl/internal.h | 12 +++++
>>>>    arch/x86/kernel/cpu/resctrl/rdtgroup.c | 61 ++++++++++++++++++++++++++
>>>>    3 files changed, 74 insertions(+)
>>>>
>>>> diff --git a/arch/x86/include/asm/msr-index.h
>>>> b/arch/x86/include/asm/msr-index.h
>>>> index 82c6a4d350e0..c78afed3c21f 100644
>>>> --- a/arch/x86/include/asm/msr-index.h
>>>> +++ b/arch/x86/include/asm/msr-index.h
>>>> @@ -1181,6 +1181,7 @@
>>>>    /* - AMD: */
>>>>    #define MSR_IA32_MBA_BW_BASE        0xc0000200
>>>>    #define MSR_IA32_SMBA_BW_BASE        0xc0000280
>>>> +#define MSR_IA32_L3_QOS_EXT_CFG        0xc00003ff
>>>>    #define MSR_IA32_EVT_CFG_BASE        0xc0000400
>>>>      /* MSR_IA32_VMX_MISC bits */
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>>>> b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> index 955999aecfca..ceb0e8e1ed76 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> @@ -56,6 +56,9 @@
>>>>    /* Max event bits supported */
>>>>    #define MAX_EVT_CONFIG_BITS        GENMASK(6, 0)
>>>>    +/* Setting bit 1 in L3_QOS_EXT_CFG enables the SDCIAE feature. */
>>>> +#define SDCIAE_ENABLE_BIT        1
>>>> +
>>>>    /**
>>>>     * cpumask_any_housekeeping() - Choose any CPU in @mask, preferring
>>>> those that
>>>>     *                    aren't marked nohz_full
>>>> @@ -477,6 +480,7 @@ struct rdt_parse_data {
>>>>     * @mbm_cfg_mask:    Bandwidth sources that can be tracked when Bandwidth
>>>>     *            Monitoring Event Configuration (BMEC) is supported.
>>>>     * @cdp_enabled:    CDP state of this resource
>>>> + * @sdciae_enabled:    SDCIAE feature is enabled
>>>>     *
>>>>     * Members of this structure are either private to the architecture
>>>>     * e.g. mbm_width, or accessed via helpers that provide abstraction. e.g.
>>>> @@ -491,6 +495,7 @@ struct rdt_hw_resource {
>>>>        unsigned int        mbm_width;
>>>>        unsigned int        mbm_cfg_mask;
>>>>        bool            cdp_enabled;
>>>> +    bool            sdciae_enabled;
>>>>    };
>>>>      static inline struct rdt_hw_resource *resctrl_to_arch_res(struct
>>>> rdt_resource *r)
>>>> @@ -536,6 +541,13 @@ int resctrl_arch_set_cdp_enabled(enum
>>>> resctrl_res_level l, bool enable);
>>>>      void arch_mon_domain_online(struct rdt_resource *r, struct
>>>> rdt_mon_domain *d);
>>>>    +static inline bool resctrl_arch_get_sdciae_enabled(enum
>>>> resctrl_res_level l)
>>>> +{
>>>> +    return rdt_resources_all[l].sdciae_enabled;
>>>> +}
>>>> +
>>>> +int resctrl_arch_set_sdciae_enabled(enum resctrl_res_level l, bool
>>>> enable);
>>>> +
>>>>    /*
>>>>     * To return the common struct rdt_resource, which is contained in struct
>>>>     * rdt_hw_resource, walk the resctrl member of struct rdt_hw_resource.
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> index d7163b764c62..c62d6183bfe4 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> @@ -1789,6 +1789,67 @@ static ssize_t
>>>> mbm_local_bytes_config_write(struct kernfs_open_file *of,
>>>>        return ret ?: nbytes;
>>>>    }
>>>>    +static void resctrl_sdciae_msrwrite(void *arg)
>>>> +{
>>>> +    bool *enable = arg;
>>>> +
>>>> +    if (*enable)
>>>> +        msr_set_bit(MSR_IA32_L3_QOS_EXT_CFG, SDCIAE_ENABLE_BIT);
>>>> +    else
>>>> +        msr_clear_bit(MSR_IA32_L3_QOS_EXT_CFG, SDCIAE_ENABLE_BIT);
>>>> +}
>>>
>>> (hmmm ... so there is an effort to make the rest of the code not be
>>> resource specific ... but then the lowest level has L3 MSR hardcoded)
>>
>> Not very clear on this.
> 
> The flow starts with functions called with L3 resource as parameter
> while the final function hardcodes programming of L3 resource
> specific MSR making an L3 resource the only supported parameter to begin with.

Oh ok. Got it.

> 
>>
>> I can separate the patch into two, one arch specific and the other FS
>> specific.
>>
>>>
>>>> +
>>>> +static int resctrl_sdciae_setup(enum resctrl_res_level l, bool enable)
>>>> +{
>>>> +    struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
>>>> +    struct rdt_ctrl_domain *d;
>>>> +
>>>> +    /* Update  L3_QOS_EXT_CFG MSR on all the CPUs in all domains*/
>>>
>>> (please note some space issues above)
>>
>> Sure.
>>
>>>
>>>> +    list_for_each_entry(d, &r->ctrl_domains, hdr.list)
>>>> +        on_each_cpu_mask(&d->hdr.cpu_mask, resctrl_sdciae_msrwrite,
>>>> &enable, 1);
>>>> +
>>>> +    return 0;
>>>
>>> It seems that this will be inside the arch specific code while
>>> resctrl_arch_set_sdciae_enabled() will be called by resctrl fs code. It is
>>> thus not clear why above returns an int, thus forcing callers to do
>>> error code handling, when it will always just return 0.
>>
>> Yes. It is returning 0 right now. Keeps the options open for other arch's
>> report error.  Looks like we heading to make this generic feature.
> 
> This is arch specific code though ... other archs will not call this function,
> other archs call (to be renamed) resctrl_arch_set_sdciae_enabled(). As I
> am looking at the ABMC work also this can benefit from using appropriate
> namespaces ... only use "resctrl_" prefix for fs code and then this should
> be more clear.

Sure. I can make code inside the arch to return void.

> 
>>>> +}
>>>> +
>>>> +static int resctrl_sdciae_enable(enum resctrl_res_level l)
>>>> +{
>>>> +    struct rdt_hw_resource *hw_res = &rdt_resources_all[l];
>>>> +    int ret = 0;
>>>> +
>>>> +    if (!hw_res->sdciae_enabled) {
>>>> +        ret = resctrl_sdciae_setup(l, true);
>>>> +        if (!ret)
>>>> +            hw_res->sdciae_enabled = true;
>>>> +    }
>>>> +
>>>> +    return ret;
>>>
>>> Same here ... this will always return 0, no?
>>
>> Yes. it is returns 0 always on AMD. Keeps the options open for other
>> arch's report error.
> 
> This is the arch specific (note the access to struct rdt_hw_resource) AMD callback.
> The function where the return code should be maintained in support of other archs is
> resctrl_arch_set_sdciae_enabled().

Sure. Got it.
- Babu Moger