| Message ID | 20250321123938.802763-2-elena.reshetova@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable automatic SVN updates for SGX enclaves | expand |
On Fri, Mar 21, 2025 at 02:34:40PM +0200, Elena Reshetova wrote: > In order to successfully execute ENCLS[EUPDATESVN], EPC must be empty. > SGX already has a variable sgx_nr_free_pages that tracks free > EPC pages. Add a new variable, sgx_nr_total_pages, that will keep > track of total number of EPC pages. It will be used in subsequent > patch to change the sgx_nr_free_pages into sgx_nr_used_pages and > allow an easy check for an empty EPC. First off, remove "in subsequent patch". What does "change sgx_nr_free_pages into sgx_nr_used_pages" mean? > > Note: The serialization for sgx_nr_total_pages is not needed because > the variable is only updated during the initialization and there's no > concurrent access. > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> > --- > arch/x86/kernel/cpu/sgx/main.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > index 8ce352fc72ac..d5df67dab247 100644 > --- a/arch/x86/kernel/cpu/sgx/main.c > +++ b/arch/x86/kernel/cpu/sgx/main.c > @@ -33,6 +33,7 @@ static LIST_HEAD(sgx_active_page_list); > static DEFINE_SPINLOCK(sgx_reclaimer_lock); > > static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0); > +static unsigned long sgx_nr_total_pages; > > /* Nodes with one or more EPC sections. */ > static nodemask_t sgx_numa_mask; > @@ -648,6 +649,8 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size, > list_add_tail(§ion->pages[i].list, &sgx_dirty_page_list); > } > > + sgx_nr_total_pages += nr_pages; > + > return true; > } > > -- > 2.45.2 > BR, Jarkko
> On Fri, Mar 21, 2025 at 02:34:40PM +0200, Elena Reshetova wrote: > > In order to successfully execute ENCLS[EUPDATESVN], EPC must be empty. > > SGX already has a variable sgx_nr_free_pages that tracks free > > EPC pages. Add a new variable, sgx_nr_total_pages, that will keep > > track of total number of EPC pages. It will be used in subsequent > > patch to change the sgx_nr_free_pages into sgx_nr_used_pages and > > allow an easy check for an empty EPC. > > First off, remove "in subsequent patch". Ok > > What does "change sgx_nr_free_pages into sgx_nr_used_pages" mean? As you can see from patch 2/4, I had to turn around the meaning of the existing sgx_nr_free_pages atomic counter not to count the # of free pages in EPC, but to count the # of used EPC pages (hence the change of name to sgx_nr_used_pages). The reason for doing this is only apparent in patch 4/4 because by having a counter sgx_nr_used_pages incremented in the atomic_long_inc_not_zero, there is a fast path that avoids taking any locks in cases when the EPC page is not the first one to be created (most of cases). I originally created a version with just using sgx_nr_free_pages, but could not avoided taking a lock in each case and it did look much less pretty than this version. The credit for the idea btw goes to Kirill who kindly reviewed my patches before. Best Regards, Elena.
On Mon, Mar 24, 2025 at 12:12:41PM +0000, Reshetova, Elena wrote: > > On Fri, Mar 21, 2025 at 02:34:40PM +0200, Elena Reshetova wrote: > > > In order to successfully execute ENCLS[EUPDATESVN], EPC must be empty. > > > SGX already has a variable sgx_nr_free_pages that tracks free > > > EPC pages. Add a new variable, sgx_nr_total_pages, that will keep > > > track of total number of EPC pages. It will be used in subsequent > > > patch to change the sgx_nr_free_pages into sgx_nr_used_pages and > > > allow an easy check for an empty EPC. > > > > First off, remove "in subsequent patch". > > Ok > > > > > What does "change sgx_nr_free_pages into sgx_nr_used_pages" mean? > > As you can see from patch 2/4, I had to turn around the meaning of the > existing sgx_nr_free_pages atomic counter not to count the # of free pages > in EPC, but to count the # of used EPC pages (hence the change of name > to sgx_nr_used_pages). The reason for doing this is only apparent in patch Why you *absolutely* need to invert the meaning and cannot make this work by any means otherwise? I doubt highly doubt this could not be done other way around. BR, Jarkko
> On Mon, Mar 24, 2025 at 12:12:41PM +0000, Reshetova, Elena wrote: > > > On Fri, Mar 21, 2025 at 02:34:40PM +0200, Elena Reshetova wrote: > > > > In order to successfully execute ENCLS[EUPDATESVN], EPC must be > empty. > > > > SGX already has a variable sgx_nr_free_pages that tracks free > > > > EPC pages. Add a new variable, sgx_nr_total_pages, that will keep > > > > track of total number of EPC pages. It will be used in subsequent > > > > patch to change the sgx_nr_free_pages into sgx_nr_used_pages and > > > > allow an easy check for an empty EPC. > > > > > > First off, remove "in subsequent patch". > > > > Ok > > > > > > > > What does "change sgx_nr_free_pages into sgx_nr_used_pages" mean? > > > > As you can see from patch 2/4, I had to turn around the meaning of the > > existing sgx_nr_free_pages atomic counter not to count the # of free pages > > in EPC, but to count the # of used EPC pages (hence the change of name > > to sgx_nr_used_pages). The reason for doing this is only apparent in patch > > Why you *absolutely* need to invert the meaning and cannot make > this work by any means otherwise? > > I doubt highly doubt this could not be done other way around. I can make it work. The point that this way is much better and no damage to existing logic is done. The sgx_nr_free_pages counter that is used only for page reclaiming and checked in a single piece of code. To give you an idea the previous iteration of the code looked like below. First, I had to define a new unconditional spinlock to protect the EPC page allocation: diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index c8a2542140a1..4f445c28929b 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -31,6 +31,7 @@ static DEFINE_XARRAY(sgx_epc_address_space); */ static LIST_HEAD(sgx_active_page_list); static DEFINE_SPINLOCK(sgx_reclaimer_lock); +static DEFINE_SPINLOCK(sgx_allocate_epc_page_lock); static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0); static unsigned long sgx_nr_total_pages; @@ -457,7 +458,10 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid) page->flags = 0; spin_unlock(&node->lock); + + spin_lock(&sgx_allocate_epc_page_lock); atomic_long_dec(&sgx_nr_free_pages); + spin_unlock(&sgx_allocate_epc_page_lock); return page; } And then also take spinlock every time eupdatesvn attempts to run: int sgx_updatesvn(void) +{ + int ret; + int retry = 10; + + spin_lock(&sgx_allocate_epc_page_lock); + + if (atomic_long_read(&sgx_nr_free_pages) != sgx_nr_total_pages) { + spin_unlock(&sgx_allocate_epc_page_lock); + return SGX_EPC_NOT_READY; + } + + do { + ret = __eupdatesvn(); + if (ret != SGX_INSUFFICIENT_ENTROPY) + break; + + } while (--retry); + + spin_unlock(&sgx_allocate_epc_page_lock); Which was called from each enclave create ioctl: @@ -163,6 +163,11 @@ static long sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg) if (copy_from_user(&create_arg, arg, sizeof(create_arg))) return -EFAULT; + /* Unless running in a VM, execute EUPDATESVN if instruction is avalible */ + if ((cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN) && + !boot_cpu_has(X86_FEATURE_HYPERVISOR)) + sgx_updatesvn(); + secs = kmalloc(PAGE_SIZE, GFP_KERNEL); if (!secs) return -ENOMEM; Would you agree that this way it is much worse even code/logic-wise even without benchmarks? Best Regards, Elena.
oN Thu, Mar 27, 2025 at 03:29:53PM +0000, Reshetova, Elena wrote: > > > On Mon, Mar 24, 2025 at 12:12:41PM +0000, Reshetova, Elena wrote: > > > > On Fri, Mar 21, 2025 at 02:34:40PM +0200, Elena Reshetova wrote: > > > > > In order to successfully execute ENCLS[EUPDATESVN], EPC must be > > empty. > > > > > SGX already has a variable sgx_nr_free_pages that tracks free > > > > > EPC pages. Add a new variable, sgx_nr_total_pages, that will keep > > > > > track of total number of EPC pages. It will be used in subsequent > > > > > patch to change the sgx_nr_free_pages into sgx_nr_used_pages and > > > > > allow an easy check for an empty EPC. > > > > > > > > First off, remove "in subsequent patch". > > > > > > Ok > > > > > > > > > > > What does "change sgx_nr_free_pages into sgx_nr_used_pages" mean? > > > > > > As you can see from patch 2/4, I had to turn around the meaning of the > > > existing sgx_nr_free_pages atomic counter not to count the # of free pages > > > in EPC, but to count the # of used EPC pages (hence the change of name > > > to sgx_nr_used_pages). The reason for doing this is only apparent in patch > > > > Why you *absolutely* need to invert the meaning and cannot make > > this work by any means otherwise? > > > > I doubt highly doubt this could not be done other way around. > > I can make it work. The point that this way is much better and no damage to > existing logic is done. The sgx_nr_free_pages counter that is used only for page reclaiming > and checked in a single piece of code. > To give you an idea the previous iteration of the code looked like below. > First, I had to define a new unconditional spinlock to protect the EPC page allocation: > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > index c8a2542140a1..4f445c28929b 100644 > --- a/arch/x86/kernel/cpu/sgx/main.c > +++ b/arch/x86/kernel/cpu/sgx/main.c > @@ -31,6 +31,7 @@ static DEFINE_XARRAY(sgx_epc_address_space); > */ > static LIST_HEAD(sgx_active_page_list); > static DEFINE_SPINLOCK(sgx_reclaimer_lock); > +static DEFINE_SPINLOCK(sgx_allocate_epc_page_lock); > > static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0); > static unsigned long sgx_nr_total_pages; > @@ -457,7 +458,10 @@ static struct sgx_epc_page *__sgx_alloc_epc_page_from_node(int nid) > page->flags = 0; > > spin_unlock(&node->lock); > + > + spin_lock(&sgx_allocate_epc_page_lock); > atomic_long_dec(&sgx_nr_free_pages); > + spin_unlock(&sgx_allocate_epc_page_lock); > > return page; > } > > And then also take spinlock every time eupdatesvn attempts to run: > > int sgx_updatesvn(void) > +{ > + int ret; > + int retry = 10; Reverse xmas tree order. > + > + spin_lock(&sgx_allocate_epc_page_lock); You could use guard for this. https://elixir.bootlin.com/linux/v6.13.7/source/include/linux/cleanup.h > + > + if (atomic_long_read(&sgx_nr_free_pages) != sgx_nr_total_pages) { > + spin_unlock(&sgx_allocate_epc_page_lock); > + return SGX_EPC_NOT_READY; Don't use uarch error codes. > + } > + > + do { > + ret = __eupdatesvn(); > + if (ret != SGX_INSUFFICIENT_ENTROPY) > + break; > + > + } while (--retry); > + > + spin_unlock(&sgx_allocate_epc_page_lock); > > Which was called from each enclave create ioctl: > > @@ -163,6 +163,11 @@ static long sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg) > if (copy_from_user(&create_arg, arg, sizeof(create_arg))) > return -EFAULT; > > + /* Unless running in a VM, execute EUPDATESVN if instruction is avalible */ > + if ((cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN) && > + !boot_cpu_has(X86_FEATURE_HYPERVISOR)) > + sgx_updatesvn(); > + > secs = kmalloc(PAGE_SIZE, GFP_KERNEL); > if (!secs) > return -ENOMEM; > > Would you agree that this way it is much worse even code/logic-wise even without benchmarks? Yes but obviously I cannot promise that I'll accept this as it is until I see the final version Also you probably should use mutex given the loop where we cannot temporarily exit the lock (like e.g. in keyrings gc we can). > > Best Regards, > Elena. BR, Jarkko
> oN Thu, Mar 27, 2025 at 03:29:53PM +0000, Reshetova, Elena wrote: > > > > > On Mon, Mar 24, 2025 at 12:12:41PM +0000, Reshetova, Elena wrote: > > > > > On Fri, Mar 21, 2025 at 02:34:40PM +0200, Elena Reshetova wrote: > > > > > > In order to successfully execute ENCLS[EUPDATESVN], EPC must be > > > empty. > > > > > > SGX already has a variable sgx_nr_free_pages that tracks free > > > > > > EPC pages. Add a new variable, sgx_nr_total_pages, that will keep > > > > > > track of total number of EPC pages. It will be used in subsequent > > > > > > patch to change the sgx_nr_free_pages into sgx_nr_used_pages and > > > > > > allow an easy check for an empty EPC. > > > > > > > > > > First off, remove "in subsequent patch". > > > > > > > > Ok > > > > > > > > > > > > > > What does "change sgx_nr_free_pages into sgx_nr_used_pages" > mean? > > > > > > > > As you can see from patch 2/4, I had to turn around the meaning of the > > > > existing sgx_nr_free_pages atomic counter not to count the # of free > pages > > > > in EPC, but to count the # of used EPC pages (hence the change of name > > > > to sgx_nr_used_pages). The reason for doing this is only apparent in > patch > > > > > > Why you *absolutely* need to invert the meaning and cannot make > > > this work by any means otherwise? > > > > > > I doubt highly doubt this could not be done other way around. > > > > I can make it work. The point that this way is much better and no damage to > > existing logic is done. The sgx_nr_free_pages counter that is used only for > page reclaiming > > and checked in a single piece of code. > > To give you an idea the previous iteration of the code looked like below. > > First, I had to define a new unconditional spinlock to protect the EPC page > allocation: > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c > b/arch/x86/kernel/cpu/sgx/main.c > > index c8a2542140a1..4f445c28929b 100644 > > --- a/arch/x86/kernel/cpu/sgx/main.c > > +++ b/arch/x86/kernel/cpu/sgx/main.c > > @@ -31,6 +31,7 @@ static DEFINE_XARRAY(sgx_epc_address_space); > > */ > > static LIST_HEAD(sgx_active_page_list); > > static DEFINE_SPINLOCK(sgx_reclaimer_lock); > > +static DEFINE_SPINLOCK(sgx_allocate_epc_page_lock); > > > > > > > static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0); > > static unsigned long sgx_nr_total_pages; > > @@ -457,7 +458,10 @@ static struct sgx_epc_page > *__sgx_alloc_epc_page_from_node(int nid) > > page->flags = 0; > > > > spin_unlock(&node->lock); > > + > > + spin_lock(&sgx_allocate_epc_page_lock); > > atomic_long_dec(&sgx_nr_free_pages); > > + spin_unlock(&sgx_allocate_epc_page_lock); > > > > return page; > > } > > > > And then also take spinlock every time eupdatesvn attempts to run: > > > > int sgx_updatesvn(void) > > +{ > > + int ret; > > + int retry = 10; > > Reverse xmas tree order. > > > + > > + spin_lock(&sgx_allocate_epc_page_lock); > > You could use guard for this. > > https://elixir.bootlin.com/linux/v6.13.7/source/include/linux/cleanup.h > > > + > > + if (atomic_long_read(&sgx_nr_free_pages) != sgx_nr_total_pages) { > > + spin_unlock(&sgx_allocate_epc_page_lock); > > + return SGX_EPC_NOT_READY; > > Don't use uarch error codes. Sure, thanks, I can fix all of the above, this was just to give an idea how the other version of the code would look like. > > > + } > > + > > + do { > > + ret = __eupdatesvn(); > > + if (ret != SGX_INSUFFICIENT_ENTROPY) > > + break; > > + > > + } while (--retry); > > + > > + spin_unlock(&sgx_allocate_epc_page_lock); > > > > Which was called from each enclave create ioctl: > > > > @@ -163,6 +163,11 @@ static long sgx_ioc_enclave_create(struct sgx_encl > *encl, void __user *arg) > > if (copy_from_user(&create_arg, arg, sizeof(create_arg))) > > return -EFAULT; > > > > + /* Unless running in a VM, execute EUPDATESVN if instruction is avalible */ > > + if ((cpuid_eax(SGX_CPUID) & SGX_CPUID_EUPDATESVN) && > > + !boot_cpu_has(X86_FEATURE_HYPERVISOR)) > > + sgx_updatesvn(); > > + > > secs = kmalloc(PAGE_SIZE, GFP_KERNEL); > > if (!secs) > > return -ENOMEM; > > > > Would you agree that this way it is much worse even code/logic-wise even > without benchmarks? > > Yes but obviously I cannot promise that I'll accept this as it is > until I see the final version Are you saying you prefer *this version with spinlock* vs. simpler version that utilizes the fact that sgx_nr_free_pages is changed into tracking of number of used pages? > > Also you probably should use mutex given the loop where we cannot > temporarily exit the lock (like e.g. in keyrings gc we can). Not sure I understand this, could you please elaborate why do I need an additional mutex here? Or are you suggesting switching spinlock to mutex? Best Regards, Elena.
On Fri, Mar 28, 2025 at 08:07:24AM +0000, Reshetova, Elena wrote: > > Yes but obviously I cannot promise that I'll accept this as it is > > until I see the final version > > Are you saying you prefer *this version with spinlock* vs. > simpler version that utilizes the fact that sgx_nr_free_pages is changed > into tracking of number of used pages? I don't know really what I do prefer. Maybe +1 version would make sense where you keep with the approach you've chosen (used pages) and better rationalize why it is mandatory, and why free pages would be worse? > > > > > Also you probably should use mutex given the loop where we cannot > > temporarily exit the lock (like e.g. in keyrings gc we can). > > Not sure I understand this, could you please elaborate why do I need an > additional mutex here? Or are you suggesting switching spinlock to mutex? In your code example you had a loop inside spinlock, which was based on a return code of an opcode, i.e. potentially infinite loop. I'd like to remind you that the hardware I have is NUC7 from 2018 so you really have to nail how things will work semantically as I can only think these things only in theoretical level ;-) [1] > > Best Regards, > Elena. > [1] https://social.kernel.org/notice/AsUbsYH0T4bTcUSdUW BR, Jarkko
On Fri, Mar 28, 2025 at 10:42:18AM +0200, Jarkko Sakkinen wrote: > In your code example you had a loop inside spinlock, which was based on > a return code of an opcode, i.e. potentially infinite loop. > > I'd like to remind you that the hardware I have is NUC7 from 2018 so > you really have to nail how things will work semantically as I can > only think these things only in theoretical level ;-) [1] That said, I do execute these in NUC7 but is getting a bit old.. Cheapest hardware I've heard is Xeon E-2334 but even that with case etc. is like nearing 2k in price. BR, Jarkko
> On Fri, Mar 28, 2025 at 08:07:24AM +0000, Reshetova, Elena wrote: > > > Yes but obviously I cannot promise that I'll accept this as it is > > > until I see the final version > > > > Are you saying you prefer *this version with spinlock* vs. > > simpler version that utilizes the fact that sgx_nr_free_pages is changed > > into tracking of number of used pages? > > I don't know really what I do prefer. > > Maybe +1 version would make sense where you keep with the approach > you've chosen (used pages) and better rationalize why it is mandatory, > and why free pages would be worse? Sure, let me send out v2 with the old approach, all suggestions and fixes taken into account and better reasoning. > > > > > > > > > Also you probably should use mutex given the loop where we cannot > > > temporarily exit the lock (like e.g. in keyrings gc we can). > > > > Not sure I understand this, could you please elaborate why do I need an > > additional mutex here? Or are you suggesting switching spinlock to mutex? > > In your code example you had a loop inside spinlock, which was based on > a return code of an opcode, i.e. potentially infinite loop. Oh, this is a misunderstanding due to limited snippet posting. The loop was bounded also by "retry" condition in while with the max number of retires being 10. It only exists earlier if there is success. > > I'd like to remind you that the hardware I have is NUC7 from 2018 so > you really have to nail how things will work semantically as I can > only think these things only in theoretical level ;-) [1] Sure, I understand. Best Regards, Elena. > > > > > > Best Regards, > > Elena. > > > > [1] https://social.kernel.org/notice/AsUbsYH0T4bTcUSdUW > > BR, Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index 8ce352fc72ac..d5df67dab247 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -33,6 +33,7 @@ static LIST_HEAD(sgx_active_page_list); static DEFINE_SPINLOCK(sgx_reclaimer_lock); static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0); +static unsigned long sgx_nr_total_pages; /* Nodes with one or more EPC sections. */ static nodemask_t sgx_numa_mask; @@ -648,6 +649,8 @@ static bool __init sgx_setup_epc_section(u64 phys_addr, u64 size, list_add_tail(§ion->pages[i].list, &sgx_dirty_page_list); } + sgx_nr_total_pages += nr_pages; + return true; }
In order to successfully execute ENCLS[EUPDATESVN], EPC must be empty. SGX already has a variable sgx_nr_free_pages that tracks free EPC pages. Add a new variable, sgx_nr_total_pages, that will keep track of total number of EPC pages. It will be used in subsequent patch to change the sgx_nr_free_pages into sgx_nr_used_pages and allow an easy check for an empty EPC. Note: The serialization for sgx_nr_total_pages is not needed because the variable is only updated during the initialization and there's no concurrent access. Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> --- arch/x86/kernel/cpu/sgx/main.c | 3 +++ 1 file changed, 3 insertions(+)