On Tue, 18 Jun 2024 06:31:09 -0500, Huang, Kai <kai.huang@xxxxxxxxx> wrote:
@@ -921,7 +956,8 @@ static int __init sgx_init(void)
if (!sgx_page_cache_init())
return -ENOMEM;
- if (!sgx_page_reclaimer_init()) {
+ if (!sgx_page_reclaimer_init() || !sgx_cgroup_init()) {
+ misc_cg_set_capacity(MISC_CG_RES_SGX_EPC, 0);
ret = -ENOMEM;
goto err_page_cache;
}
This code change is wrong due to two reasons:
1) If sgx_page_reclaimer_init() was successful, but sgx_cgroup_init()
failed, you actually need to 'goto err_kthread' because the ksgxd()
kernel
thread is already created and is running.
2) There are other cases after here that can also result in sgx_init() to
fail completely, e.g., registering sgx_dev_provision mics device. We
need
to reset the capacity to 0 for those cases as well.
AFAICT, you need something like:
diff --git a/arch/x86/kernel/cpu/sgx/main.c
b/arch/x86/kernel/cpu/sgx/main.c
index 27892e57c4ef..46f9c26992a7 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -930,6 +930,10 @@ static int __init sgx_init(void)
if (ret)
goto err_kthread;
+ ret = sgx_cgroup_init();
+ if (ret)
+ goto err_provision;
+
/*
* Always try to initialize the native *and* KVM drivers.
* The KVM driver is less picky than the native one and
@@ -941,10 +945,12 @@ static int __init sgx_init(void)
ret = sgx_drv_init();
if (sgx_vepc_init() && ret)
- goto err_provision;
+ goto err_cgroup;
return 0;
+err_cgroup:
+ /* SGX EPC cgroup cleanup */
err_provision:
misc_deregister(&sgx_dev_provision);
@@ -952,6 +958,8 @@ static int __init sgx_init(void)
kthread_stop(ksgxd_tsk);
err_page_cache:
+ misc_misc_cg_set_capacity(MISC_CG_RES_SGX_EPC, 0);
+
for (i = 0; i < sgx_nr_epc_sections; i++) {
vfree(sgx_epc_sections[i].pages);
memunmap(sgx_epc_sections[i].virt_addr);
I put the sgx_cgroup_init() before sgx_drv_init() and sgx_vepc_init(),
otherwise you will need sgx_drv_cleanup() and sgx_vepc_cleanup()
respectively when sgx_cgroup_init() fails.
Yes, good catch.
This looks a little bit weird too, though:
Calling misc_misc_cg_set_capacity() to reset capacity to 0 is done at end
of sgx_init() error path, because the "set capacity" part is done in
sgx_epc_cache_init().
But logically, both "set capacity" and "reset capacity to 0" should be
SGX
EPC cgroup operation, so it's more reasonable to do "set capacity" in
sgx_cgroup_init() and do "reset to 0" in the
/* SGX EPC cgroup cleanup */
as shown above.
Eventually, you will need to do EPC cgroup cleanup anyway, e.g., to free
the workqueue, so it's odd to have two places to handle EPC cgroup
cleanup.
I understand the reason "set capacity" part is done in
sgx_page_cache_init() now is because in that function you can easily get
the capacity. But the fact is @sgx_numa_nodes also tracks EPC size for
each node, so you can also get the total EPC size from @sgx_numa_node in
sgx_cgroup_init() and set capacity there.
In this case, you can put "reset capacity to 0" and "free workqueue"
together as the "SGX EPC cgroup cleanup", which is way more clear IMHO.
Okay, will expose @sgx_numa_nodes to epc_cgroup.c and do the calculations
in sgx_cgroup_init().
Thanks
Haitao
