Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a89ed748-7e30-bf94-a8b6-6e9a63991657@intel.com>
Date:   Fri, 26 Aug 2022 10:34:17 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.2.0
Subject: Re: [PATCH v3 08/10] x86/resctrl: Add the sysfs interface to read the
 event configuration
Content-Language: en-US
To:     <babu.moger@amd.com>, <fenghua.yu@intel.com>, <tglx@linutronix.de>,
        <mingo@redhat.com>, <bp@alien8.de>
CC:     <eranian@google.com>, <dave.hansen@linux.intel.com>,
        <x86@kernel.org>, <hpa@zytor.com>, <corbet@lwn.net>,
        <linux-kernel@vger.kernel.org>, <linux-doc@vger.kernel.org>,
        <bagasdotme@gmail.com>
References: <166117559756.6695.16047463526634290701.stgit@bmoger-ubuntu>
 <166117586269.6695.16560984025494646306.stgit@bmoger-ubuntu>
 <654433c3-2980-41a0-503f-f888d60310e8@intel.com>
 <d6f5affa-1f7d-7944-70a3-6d2c99863fad@amd.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <d6f5affa-1f7d-7944-70a3-6d2c99863fad@amd.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RW1mdkpWTFE1d1M0M3RLVGpBL2pLeDNTZEFGeW5uS3d4TXYzL2srSVp4cGpQ?=
 =?utf-8?B?SndGUVpyMWZsbHUrZUIzVVhCeXBZbXNnelkrcGJkcVV1cHorT2V5OFJSWElY?=
 =?utf-8?B?cHk0bW9zUDdZdE1yQTNmb2FnVUN2Y3JIby9XRlk3L3dlbWxGY0cyenhKT0tn?=
 =?utf-8?B?Vk4ySDNqN21XMlFBMFYvaU1YczM3WTJReXUrMFBIbFBOM05YRUthbldwK3hj?=
 =?utf-8?B?Mm0vUEJFb1h6d3FDT0xHRXpQT1NNcWI1TnNMMDhvNlBzWEdhMThGZ3pBNlU4?=
 =?utf-8?B?TUx2dWtHeU8wNUFhZDdJcU1lUFMyL0krVFYyRERvNThUUnIzN0NPRFlvdnpv?=
 =?utf-8?B?THcxWXVXRVRwWndwRXdZMkVWUzFDSzhTZVBvMzJkSi9nLzZGUDdRWVc3bUJ5?=
 =?utf-8?B?OEdKcVE3WkhwZytLS1VjY3Z0WXlxVitIMStod2E4R0VKVWJNNDFHakRyd1pM?=
 =?utf-8?B?V3J6T01lTzNOdnRBNFhkRjdqakN2NVhmbGlsejVmNVZxMmVvVUF4MWlraXNG?=
 =?utf-8?B?UTBkOGNOOE02RGFjenV0ZFAvYTE3Q3c5bmpuckdGdUcwTG45bGpNdGRrSGk1?=
 =?utf-8?B?RTBGamJRMHM0Ni9yeGhKRlJIaHdjbzhmRXU2SkNwT3djc2RLYUdXVUp2Nlkv?=
 =?utf-8?B?S0ptQXUyS3ZrWmdTVDZ2WHVWOGs4OUhUalIwTGpOeGlyYmJlWE03SkRZTlN1?=
 =?utf-8?B?TTRWUzJFL2JNTmsvN1F4bzdJWTNjV0sxc3BJNU54NXR2bmFmZDhHbytLWU8v?=
 =?utf-8?B?WWFHYUVWaDVEbytkdWRYODhWeG53YVVPcTh2SDFhOHRlb0xuczlVVTVlUG84?=
 =?utf-8?B?Wjk5NWRmRWtOWWlQY2VlbitTQ1dYVGptMnBTZS9FNW9xeGMySGc1NElEcHkz?=
 =?utf-8?B?WGhvbWw3TjJjSHFyTmJrQllZR0k2d01jcTFwczFzY3RGeHF2Q3VkKzQvdVQ4?=
 =?utf-8?B?NHR3ZE1Kay91WGFrcml6ditxL2JFV0RNVVZoTC9ibEFJYzM1M1oxV3kzVjFB?=
 =?utf-8?B?TzBRRnBVWTRDYzJPVURWYSt4dDJvN2wySWxmSFBpV2JQeGhtVlowb3pxR2Nv?=
 =?utf-8?B?L2FRc2ZnTElDUytLWkRqV21oMW9ZNmhDTU5BL0xpbEZPdnArVCtwNDNEampx?=
 =?utf-8?B?WDNoazN0L29OL0NSRmo2Q2RndFJIb1ByQkMyQ3o1cGduUzRoUVV2b1R2czFP?=
 =?utf-8?B?ZlVNVkpzK3ppdHVWcGo3VzRGRmQzTklDWWxUd0crZENTWlAxSVpwbWtCaCtI?=
 =?utf-8?B?ZmhVVFZvQmNTVHNKV1VWSmxCekJGM05YTFJWNzUydnkyZ2hRa3pySXJid0tC?=
 =?utf-8?B?WklzV0Q0SmpyNUhOVFNHYkdNWUVLOW5OZ1VRQk8vV0I5T0NpcHRXQjZSd2pU?=
 =?utf-8?B?M0ttNXluaDZCNUNxZFY3SEdUNVZBd1hlbEphN2JZNnVjNjVOdFVzUkVueHNT?=
 =?utf-8?B?ZUdMcnBsdk9OeUhtVkNxYlZtaTBLejNYbmVubSs2SlcwSWFhc05jYWJESCtq?=
 =?utf-8?B?dDJnbWxzalB2M1lXemM2dzV4d3dZU2RmQzFydklxUlpadElOT0ZsSzVReXdS?=
 =?utf-8?B?bHRzUHFBNjZFYjB0WWdDdllWbW9CMVNwVWpxUEFYUnB2Tm5qa0NnZytVYVU1?=
 =?utf-8?B?Ky9FdlZtYmx1ckRlVVpZKzNDellKbjhQKzhTWWpLbjk2Yjg3V2doQkMyVkw4?=
 =?utf-8?B?VmhxaUFkc0FsY1BHcGVyTVZSNXQrempuUjMwUEM3MFRBQ2VXS2F6SkpUd2Np?=
 =?utf-8?B?T2svSzBML011czZsN0Jyb0pMNUt0SzJBM2MyUS9WQW1lRTdyb3k4R3BYTCtw?=
 =?utf-8?B?U0VXTk5iTENvTUltcWNaS080VTJZNVBqZXBEWDcyTlpUVWZuTWtTU0hTTUk0?=
 =?utf-8?B?STZmT2RHdTVDa0o1RUJ4TG5zRGZZY3ZJS0JlemM1V042VlFXZFhWclgvT3M3?=
 =?utf-8?B?NExTcGtEWHVURktwbzBncFdLQ1JpdGllVmp6eVlRUnlOK1BIa2xJVlhDOHBW?=
 =?utf-8?B?Q215SHlheFZTa3YxcVQzWHk3RnR1Szc5RnJxdHQrbjM3Y09admgvQzVvOXBu?=
 =?utf-8?B?Z2NyVUJQVVZtTFBUQjQveHZpZ1AyMXVpeS95UUd4UEFZTk1kK1lTNUJGeHJH?=
 =?utf-8?B?YkJKMVJDR05wMnhwTmt6bG9tSU8rejdPS3dURnZtanh2T1VYLzg3b0pkWUdr?=
 =?utf-8?B?Ync9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c12355d8-5e2f-44e5-77fe-08da87893530
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Aug 2022 17:34:20.0547
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: N5wixF7P8+FTvqJPx3ax7MI0cEgO8koDbXCvv/TqaJRb7TK8R23tOjzksrefvDohPmfm90KaG1b4jJ24SFv3/kwcbIhV42aKurCe7RYfpvU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR11MB1612
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Babu,

On 8/26/2022 9:49 AM, Moger, Babu wrote:
> On 8/24/22 16:16, Reinette Chatre wrote:
>> On 8/22/2022 6:44 AM, Babu Moger wrote:

...

>>> +#define READS_TO_REMOTE_S_MEM		BIT(5)
>>> +
>>> +/* Dirty Victims to All Types of Memory */
>>> +#define  DIRTY_VICTIS_TO_ALL_MEM	BIT(6)
>> Is this intended to be "DIRTY_VICTIMS_TO_ALL_MEM" ?
> Yes. that is what spec says.

You did notice the typo, right?

>>> +	}
>>> +
>>> +	rdmsr(MSR_IA32_EVT_CFG_BASE + msr_index, md->u.mon_config, h);
>>> +}
>>> +
>>> +void mondata_config_read(struct rdt_domain *d, union mon_data_bits *md)
>>> +{
>>> +	smp_call_function_any(&d->cpu_mask, mon_event_config_read, md, 1);
>>> +}
>>> +
>>> +int rdtgroup_mondata_config_show(struct seq_file *m, void *arg)
>>> +{
>>> +	struct kernfs_open_file *of = m->private;
>>> +	struct rdt_hw_resource *hw_res;
>>> +	u32 resid, evtid, domid;
>>> +	struct rdtgroup *rdtgrp;
>>> +	struct rdt_resource *r;
>>> +	union mon_data_bits md;
>>> +	struct rdt_domain *d;
>>> +	int ret = 0;
>>> +
>>> +	rdtgrp = rdtgroup_kn_lock_live(of->kn);
>>> +	if (!rdtgrp) {
>>> +		ret = -ENOENT;
>>> +		goto out;
>>> +	}
>>> +
>>> +	md.priv = of->kn->priv;
>>> +	resid = md.u.rid;
>>> +	domid = md.u.domid;
>>> +	evtid = md.u.evtid;
>>> +
>>> +	hw_res = &rdt_resources_all[resid];
>>> +	r = &hw_res->r_resctrl;
>>> +
>>> +	d = rdt_find_domain(r, domid, NULL);
>>> +	if (IS_ERR_OR_NULL(d)) {
>>> +		ret = -ENOENT;
>>> +		goto out;
>>> +	}
>>> +
>>> +	mondata_config_read(d, &md);
>>> +
>>> +	seq_printf(m, "0x%x\n", md.u.mon_config);
>> Looking at this patch and the next, the sysfs files are introduced to read
>> from and write to the configuration register. From what I can tell the
>> data is never used internally (what did I miss?). Why is the value of the
>> configuration register stored? 
> 
> You didn't miss anything. We don't need to store it.  But we need it as
> part of mon_data_bits structure because, it need to be passed to
> mon_event_config_read and rdtgroup_mondata_config_write.

These functions are introduced here ... so it is only needed because
the demand is created here also. This can be changed, no? 

> 
> In these functions we need evtid and also config value (mon_config).
> 

I see no need to pass evtid so deep - it can be checked right in
rdtgroup_mondata_config_show() and then an appropriate wrapper
can be called to just return the config value. Even if had to also
pass evtid through many layers you could create a temporary structure
to do so and not unnecessarily  increase the size of a long lived
system structure to satisfy this temporary need.

Reinette