Received-SPF: pass (google.com: domain of linux-kernel+bounces-150863-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <d401e7a1-44fa-44b6-9c6e-c0abdabd5111@intel.com>
Date: Fri, 19 Apr 2024 11:29:53 +1200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v12 05/14] x86/sgx: Implement basic EPC misc cgroup
 functionality
To: Haitao Huang <haitao.huang@linux.intel.com>, "hpa@zytor.com"
	<hpa@zytor.com>, "tim.c.chen@linux.intel.com" <tim.c.chen@linux.intel.com>,
	"linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>, "x86@kernel.org"
	<x86@kernel.org>, "dave.hansen@linux.intel.com"
	<dave.hansen@linux.intel.com>, "jarkko@kernel.org" <jarkko@kernel.org>,
	"cgroups@vger.kernel.org" <cgroups@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"mkoutny@suse.com" <mkoutny@suse.com>, "tglx@linutronix.de"
	<tglx@linutronix.de>, "Mehta, Sohil" <sohil.mehta@intel.com>, "tj@kernel.org"
	<tj@kernel.org>, "mingo@redhat.com" <mingo@redhat.com>, "bp@alien8.de"
	<bp@alien8.de>
CC: "mikko.ylinen@linux.intel.com" <mikko.ylinen@linux.intel.com>,
	"seanjc@google.com" <seanjc@google.com>, "anakrish@microsoft.com"
	<anakrish@microsoft.com>, "Zhang, Bo" <zhanb@microsoft.com>,
	"kristen@linux.intel.com" <kristen@linux.intel.com>, "yangjie@microsoft.com"
	<yangjie@microsoft.com>, "Li, Zhiquan1" <zhiquan1.li@intel.com>,
	"chrisyan@microsoft.com" <chrisyan@microsoft.com>
References: <20240416032011.58578-1-haitao.huang@linux.intel.com>
 <20240416032011.58578-6-haitao.huang@linux.intel.com>
 <a5e009636c5144622e0a910a459cd9d05976715e.camel@intel.com>
 <op.2mf3ykfswjvjmi@hhuan26-mobl.amr.corp.intel.com>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <op.2mf3ykfswjvjmi@hhuan26-mobl.amr.corp.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?LzZoNE1XTGZPWEg1c1l2cGlMTjlnY0dxUTN0dEZyQk1pUjRqZzYwT09leTFB?=
 =?utf-8?B?N2dxUHo1Q2Y1dS9YSkgwYm9QVzZJenBCcis3SCtQNktVV25MaGRleEZiQzh3?=
 =?utf-8?B?NWJSTjkzcjhnMmV2T1ArNytUZFpsMHZzd3U2N0ZkRExNb3Y4bHlkcWNDTDZB?=
 =?utf-8?B?aFJ6TE51VmVJRUxIbDZpZ1NsSkp6UnNqTzVFbmZETFRTejZjaFFKOWFvb3hH?=
 =?utf-8?B?UjIxb20ycUxybWw3bHEvNEpZUmR4U0hjMGJ2WUZEdTNpUC9CY1lpa0ZTSHY4?=
 =?utf-8?B?L3VJNWozQkpZUnpkaFZxTGFJU25tYmk1QXRkTURXUGtlNUl6OTlvT2RsMzI3?=
 =?utf-8?B?TTNseFhJL2xjcE41TDFxZG4xSkN3SWFaMTZsL1N6MmFZZ1NlVWVVUlR2VFg2?=
 =?utf-8?B?T1oyUmhoQUVvY3BTWDcvRnJCOElBQkgxNExjZ29vMEJHdUJvcDZDY3JRSXVN?=
 =?utf-8?B?OFZlclR5MytTTmc1SytiemRkMHcyOWhDNVd2cVN5WG9nZTlpeFlBWDFicysy?=
 =?utf-8?B?YzEwRFprRHJFcTBHd0VmaTFKOWhsL2FBRGdjbjRpTUdLNWF0N3VpOVZXY21Q?=
 =?utf-8?B?Vmxkb0F6c3d3TmlEVEtiQXhrT3NhZkFkVDRBQTlhQXk1Ri9aS3ZpTXg3TWpU?=
 =?utf-8?B?TlA1RE9TYjMrOWhCSHVQdlJTamtKZ09USUlaREpjMStTV3J4cGRuaE1aZlFO?=
 =?utf-8?B?WmlxNHdRaVhlSUtiK1hSWUxGZVU3UDRnTG5WUnFsSStPZEtHMVdWeVNFTjBK?=
 =?utf-8?B?cy9mVjNHeFVOZTNld1I2SGdhRFp5Qy8zNEc1WEo5ZWF2SDVLUGU2VnV5M3U3?=
 =?utf-8?B?dHN0RytOUXFNZWZzOUNEOWFTQ1IrSE5kNXF5OGlZc3Rqc014aGNCekRlb1Fn?=
 =?utf-8?B?dEJZRnRGOVFZNnVFYjFBU2hkQm9oUkhsdUVreDYvdkhjZHkvZFFUQTFHZFVD?=
 =?utf-8?B?SElNdHEzS3gvc3FrbEp2dWxnZ3VodzgvY3BTNm5yZTdRWFdnaC90WXZpalVG?=
 =?utf-8?B?NFdSN2g4TFp3QW1tdmZrSVp5ZSs4cDArYndEVFEzb0p3QUtkOU9yaEwwRVFY?=
 =?utf-8?B?dnN4aWo4Zm9USCtpZE1OemN1TVQyRkxDQlBHakE2cmxBenk4dWxqRThyRmhO?=
 =?utf-8?B?Z3dBSnlUMXh2NmVNc29EYjQzcHoxRWMwMlRpNTJ3NWVqZHJ4VVNlbUd1NjJw?=
 =?utf-8?B?bEVEbXJGenBPTmR3UW9JcnQ2cGFsUGZvSkNWNUZwQ0RlZDM1alhxa0JOODh5?=
 =?utf-8?B?dE5VKzBkTWE2OWhHRnJTbHcrOFEyNld0cExmdmlBcFNONzl1Y1c5ektQaWk0?=
 =?utf-8?B?end2Z0RkajRXbTVYbytNelAzWlJvalpqQWt1aWY0dDF6Qm4zdk9vY0xlTGdH?=
 =?utf-8?B?OUM3czRNSVZBY1J3bVJFWVU2anpaWmxQRWNDT1NHWmRaU0l4aStWV05US01Q?=
 =?utf-8?B?NDd2VllwZlpna1hJajVYQjBWRGV3VHovcytIOVphS0RLR0tuTVlpbXlja3B5?=
 =?utf-8?B?M0R6SXBQK3ZuazRyVFRHd3FKQkJPcUlaa3ZPcHlzcjNtWFBBWEpOVy84NVg3?=
 =?utf-8?B?QXlDdXhTWWRvSEZGMjhSWUwrK3VvNG9KUnJmZ0lkbWVJd0RRM2dRNGhpaXd5?=
 =?utf-8?B?RmJHZ1RhM2JiRDJjemRVT0pLRVdTNGtWbVFENzZmTk1DdEVrSTRyZHNPcG0r?=
 =?utf-8?B?OXRFUEJaZVBJWHRWdEVibEJlcWVJelFzUlFGWk9nVXdGK09DU3RzbllGbGU5?=
 =?utf-8?B?Wm40TUlmWDdISVVJK1NmS05vU3FqZEU3eXhodEk5aE1kb2lIVjZweDVvWEM1?=
 =?utf-8?B?dXZnZmVaRlh3cXQydjg4ODlXY1FCc1lXdi9Mb0JmUzM0S3FRL1BGMFlQejRj?=
 =?utf-8?B?VE96cWUvb2I2Vk91Q2M5bUJLZ2x1K21RSFptcWhaQ0tYSXRGQ1pXQTlDRzA1?=
 =?utf-8?B?dWRDZHJnUFFLUWs0S0twbDd2bzBPTmU5MHNCWXpVNUxacklnVVNsWWJVRW9B?=
 =?utf-8?B?Nml2TTk1eU1JQjNzUGxVbGZ2cCsxRnd1R0JtUW56aGJKRCtUQnV4UnJLTHNJ?=
 =?utf-8?B?aVpScGpvR0pveFE5cUZWTUpIT2J3dHJGc1dEV0c5NlBJYkROcEczUU1ieG5q?=
 =?utf-8?Q?BvRAdzE6K++3a45l4bP/GLRu/?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d40007af-4ec2-4f23-6ced-08dc5fff81c2
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Apr 2024 23:30:17.9212
 (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: 9bOoIfDFT0shuCeyvZn8QSBhwt44rhL17oRkY7iInKL/6stN+gcgt0Hmb6Bco/7ecESA1jUNGnyrB529xGiOkg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB5028
X-OriginatorOrg: intel.com


> Was requested by Jarkko:
> https://lore.kernel.org/lkml/CYU504RLY7QU.QZY9LWC076NX@suppilovahvero/#t
> 
>> [...]

Ah I missed that.  No problem to me.

>>
>>>
>>> --- /dev/null
>>> +++ b/arch/x86/kernel/cpu/sgx/epc_cgroup.h
>>> @@ -0,0 +1,72 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +#ifndef _SGX_EPC_CGROUP_H_
>>> +#define _SGX_EPC_CGROUP_H_
>>> +
>>> +#include <asm/sgx.h>
>>
>> I don't see why you need <asm/sgx.h> here.  Also, ...
>>
>>> +#include <linux/cgroup.h>
>>> +#include <linux/misc_cgroup.h>
>>> +
>>> +#include "sgx.h"
>>
>> ... "sgx.h" already includes <asm/sgx.h>
>>
>> [...]
>>
> right
> 
>>>
>>> +static inline struct sgx_cgroup *sgx_get_current_cg(void)
>>> +{
>>> +    /* get_current_misc_cg() never returns NULL when Kconfig enabled */
>>> +    return sgx_cgroup_from_misc_cg(get_current_misc_cg());
>>> +}
>>
>> I spent some time looking into this.  And yes if I was reading code
>> correctly the get_current_misc_cg() should never return NULL when Kconfig
>> is on.
>>
>> I typed my analysis below in [*].  And it would be helpful if any cgroup
>> expert can have a second eye on this.
>>
>> [...]
>>
> Thanks for checking this and I did similar and agree with the 
> conclusion. I think this is confirmed also by Michal's description AFAICT:
> "
> The current implementation creates root css object (see cgroup_init(),
> cgroup_ssid_enabled() check is after cgroup_init_subsys()).
> I.e. it will look like all tasks are members of root cgroup wrt given
> controller permanently and controller attribute files won't exist."

After looking I believe we can even disable MISC cgroup at runtime for a 
particular cgroup (haven't actually verified on real machine, though):

  # echo "-misc" > /sys/fs/cgroup/my_group/cgroup.subtree_control

And if you look at the MISC cgroup core code, many functions actually 
handle a NULL css, e.g., misc_cg_try_charge():

	int misc_cg_try_charge(enum misc_res_type type,
				struct misc_cg *cg, u64 amount)
	{
		...

         	if (!(valid_type(type) && cg &&
				READ_ONCE(misc_res_capacity[type])))
                 	return -EINVAL;

		...
	}

That's why I am still a little bit worried about this.  And it's better 
to have cgroup expert(s) to confirm here.

Btw, AMD SEV doesn't need to worry because it doesn't dereference @css 
but just pass it to MISC cgroup core functions like 
misc_cg_try_charge().  But for SGX, we actually dereference it directly.