* [PATCH] x86/sgx: Declare sgx_set_attribute() for !CONFIG_X86_SGX
@ 2021-09-03 6:41 Jarkko Sakkinen
2021-09-03 15:29 ` Sean Christopherson
0 siblings, 1 reply; 6+ messages in thread
From: Jarkko Sakkinen @ 2021-09-03 6:41 UTC (permalink / raw)
To: Jarkko Sakkinen, Dave Hansen, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, x86, H. Peter Anvin, Paolo Bonzini,
Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel
Cc: Tony Luck, linux-sgx, linux-kernel, kvm
Simplify sgx_set_attribute() usage by declaring a fallback
implementation for it rather than requiring to have compilation
flag checks in the call site. The fallback unconditionally returns
-EINVAL.
Refactor the call site in kvm_vm_ioctl_enable_cap() accordingly.
The net result is the same: KVM_CAP_SGX_ATTRIBUTE causes -EINVAL
when kernel is compiled without CONFIG_X86_SGX_KVM.
Cc: Tony Luck <tony.luck@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
arch/x86/include/asm/sgx.h | 8 ++++++++
arch/x86/kvm/x86.c | 2 --
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index 05f3e21f01a7..31ee106c0f4b 100644
--- a/arch/x86/include/asm/sgx.h
+++ b/arch/x86/include/asm/sgx.h
@@ -372,7 +372,15 @@ int sgx_virt_einit(void __user *sigstruct, void __user *token,
void __user *secs, u64 *lepubkeyhash, int *trapnr);
#endif
+#ifdef CONFIG_X86_SGX
int sgx_set_attribute(unsigned long *allowed_attributes,
unsigned int attribute_fd);
+#else
+static inline int sgx_set_attribute(unsigned long *allowed_attributes,
+ unsigned int attribute_fd)
+{
+ return -EINVAL;
+}
+#endif
#endif /* _ASM_X86_SGX_H */
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e5d5c5ed7dd4..a6a27a8f41eb 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5633,7 +5633,6 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
kvm->arch.bus_lock_detection_enabled = true;
r = 0;
break;
-#ifdef CONFIG_X86_SGX_KVM
case KVM_CAP_SGX_ATTRIBUTE: {
unsigned long allowed_attributes = 0;
@@ -5649,7 +5648,6 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
r = -EINVAL;
break;
}
-#endif
case KVM_CAP_VM_COPY_ENC_CONTEXT_FROM:
r = -EINVAL;
if (kvm_x86_ops.vm_copy_enc_context_from)
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] x86/sgx: Declare sgx_set_attribute() for !CONFIG_X86_SGX
2021-09-03 6:41 [PATCH] x86/sgx: Declare sgx_set_attribute() for !CONFIG_X86_SGX Jarkko Sakkinen
@ 2021-09-03 15:29 ` Sean Christopherson
2021-09-03 15:58 ` Jarkko Sakkinen
0 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2021-09-03 15:29 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Dave Hansen, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel, Tony Luck, linux-sgx, linux-kernel,
kvm
On Fri, Sep 03, 2021, Jarkko Sakkinen wrote:
> Simplify sgx_set_attribute() usage by declaring a fallback
> implementation for it rather than requiring to have compilation
> flag checks in the call site. The fallback unconditionally returns
> -EINVAL.
>
> Refactor the call site in kvm_vm_ioctl_enable_cap() accordingly.
> The net result is the same: KVM_CAP_SGX_ATTRIBUTE causes -EINVAL
> when kernel is compiled without CONFIG_X86_SGX_KVM.
Eh, it doesn't really simplify the usage. If anything it makes it more convoluted
because the capability check in kvm_vm_ioctl_check_extension() still needs an
#ifdef, e.g. readers will wonder why the check is conditional but the usage is not.
> Cc: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> arch/x86/include/asm/sgx.h | 8 ++++++++
> arch/x86/kvm/x86.c | 2 --
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
> index 05f3e21f01a7..31ee106c0f4b 100644
> --- a/arch/x86/include/asm/sgx.h
> +++ b/arch/x86/include/asm/sgx.h
> @@ -372,7 +372,15 @@ int sgx_virt_einit(void __user *sigstruct, void __user *token,
> void __user *secs, u64 *lepubkeyhash, int *trapnr);
> #endif
>
> +#ifdef CONFIG_X86_SGX
> int sgx_set_attribute(unsigned long *allowed_attributes,
> unsigned int attribute_fd);
> +#else
> +static inline int sgx_set_attribute(unsigned long *allowed_attributes,
> + unsigned int attribute_fd)
> +{
> + return -EINVAL;
> +}
> +#endif
>
> #endif /* _ASM_X86_SGX_H */
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e5d5c5ed7dd4..a6a27a8f41eb 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5633,7 +5633,6 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> kvm->arch.bus_lock_detection_enabled = true;
> r = 0;
> break;
> -#ifdef CONFIG_X86_SGX_KVM
> case KVM_CAP_SGX_ATTRIBUTE: {
> unsigned long allowed_attributes = 0;
>
> @@ -5649,7 +5648,6 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> r = -EINVAL;
> break;
> }
> -#endif
> case KVM_CAP_VM_COPY_ENC_CONTEXT_FROM:
> r = -EINVAL;
> if (kvm_x86_ops.vm_copy_enc_context_from)
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/sgx: Declare sgx_set_attribute() for !CONFIG_X86_SGX
2021-09-03 15:29 ` Sean Christopherson
@ 2021-09-03 15:58 ` Jarkko Sakkinen
2021-09-03 16:33 ` Jarkko Sakkinen
2021-09-06 8:35 ` Paolo Bonzini
0 siblings, 2 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2021-09-03 15:58 UTC (permalink / raw)
To: Sean Christopherson
Cc: Dave Hansen, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel, Tony Luck, linux-sgx, linux-kernel,
kvm
On Fri, 2021-09-03 at 15:29 +0000, Sean Christopherson wrote:
> On Fri, Sep 03, 2021, Jarkko Sakkinen wrote:
> > Simplify sgx_set_attribute() usage by declaring a fallback
> > implementation for it rather than requiring to have compilation
> > flag checks in the call site. The fallback unconditionally returns
> > -EINVAL.
> >
> > Refactor the call site in kvm_vm_ioctl_enable_cap() accordingly.
> > The net result is the same: KVM_CAP_SGX_ATTRIBUTE causes -EINVAL
> > when kernel is compiled without CONFIG_X86_SGX_KVM.
>
> Eh, it doesn't really simplify the usage. If anything it makes it more convoluted
> because the capability check in kvm_vm_ioctl_check_extension() still needs an
> #ifdef, e.g. readers will wonder why the check is conditional but the usage is not.
It does objectively a bit, since it's one ifdef less.
This is fairly standard practice to do in kernel APIs, used in countless
places, for instance in Tony's patch set to add MCE recovery for SGX. And
it would be nice to share common pattern here how we define API now and
futre.
I also remarked that declaration of "sgx_provisioning_allowed" is not flagged,
which is IMHO even more convolved because without SGX it is spare data.
/Jarkko
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/sgx: Declare sgx_set_attribute() for !CONFIG_X86_SGX
2021-09-03 15:58 ` Jarkko Sakkinen
@ 2021-09-03 16:33 ` Jarkko Sakkinen
2021-09-06 8:35 ` Paolo Bonzini
1 sibling, 0 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2021-09-03 16:33 UTC (permalink / raw)
To: Sean Christopherson
Cc: Dave Hansen, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel, Tony Luck, linux-sgx, linux-kernel,
kvm
On Fri, 2021-09-03 at 18:58 +0300, Jarkko Sakkinen wrote:
> On Fri, 2021-09-03 at 15:29 +0000, Sean Christopherson wrote:
> > On Fri, Sep 03, 2021, Jarkko Sakkinen wrote:
> > > Simplify sgx_set_attribute() usage by declaring a fallback
> > > implementation for it rather than requiring to have compilation
> > > flag checks in the call site. The fallback unconditionally returns
> > > -EINVAL.
> > >
> > > Refactor the call site in kvm_vm_ioctl_enable_cap() accordingly.
> > > The net result is the same: KVM_CAP_SGX_ATTRIBUTE causes -EINVAL
> > > when kernel is compiled without CONFIG_X86_SGX_KVM.
> >
> > Eh, it doesn't really simplify the usage. If anything it makes it more convoluted
> > because the capability check in kvm_vm_ioctl_check_extension() still needs an
> > #ifdef, e.g. readers will wonder why the check is conditional but the usage is not.
>
> It does objectively a bit, since it's one ifdef less.
>
> This is fairly standard practice to do in kernel APIs, used in countless
> places, for instance in Tony's patch set to add MCE recovery for SGX. And
> it would be nice to share common pattern here how we define API now and
> futre.
>
> I also remarked that declaration of "sgx_provisioning_allowed" is not flagged,
> which is IMHO even more convolved because without SGX it is spare data.
This should have had RFC tho (my bad forgot --subject-prefix="PATCH
RFC"), given that this makes less sense alone than within context of
patch set. I get that like this it's not worth of applying even if
it makes sense as a change.
I prefer sending patches, rather than attaching patches to responses,
because:
1. They get a lore.kernel.org link.
2. Can be fluently applied to other patch sets with b4:
https://people.kernel.org/monsieuricon/introducing-4-and-patch-attestation
3. They get a patchwork link.
Attachments are not as nice objects to manage as distinct emails.
/Jarkko
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/sgx: Declare sgx_set_attribute() for !CONFIG_X86_SGX
2021-09-03 15:58 ` Jarkko Sakkinen
2021-09-03 16:33 ` Jarkko Sakkinen
@ 2021-09-06 8:35 ` Paolo Bonzini
2021-09-07 13:37 ` Jarkko Sakkinen
1 sibling, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2021-09-06 8:35 UTC (permalink / raw)
To: Jarkko Sakkinen, Sean Christopherson
Cc: Dave Hansen, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, Tony Luck, linux-sgx, linux-kernel, kvm
On 03/09/21 17:58, Jarkko Sakkinen wrote:
>> Eh, it doesn't really simplify the usage. If anything it makes it more convoluted
>> because the capability check in kvm_vm_ioctl_check_extension() still needs an
>> #ifdef, e.g. readers will wonder why the check is conditional but the usage is not.
> It does objectively a bit, since it's one ifdef less.
But you're effectively replacing #ifdef CONFIG_X86_SGX_KVM with #ifdef
CONFIG_X86_SGX; so the patch is not a no-op as far as KVM is concerned.
So NACK for the KVM parts (yeah I know it's RFC but just to be clearer),
but I agree that adding a stub inline version of the function is
standard practice and we do it a lot in KVM too.
Paolo
> This is fairly standard practice to do in kernel APIs, used in countless
> places, for instance in Tony's patch set to add MCE recovery for SGX. And
> it would be nice to share common pattern here how we define API now and
> futre.
>
> I also remarked that declaration of "sgx_provisioning_allowed" is not flagged,
> which is IMHO even more convolved because without SGX it is spare data.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/sgx: Declare sgx_set_attribute() for !CONFIG_X86_SGX
2021-09-06 8:35 ` Paolo Bonzini
@ 2021-09-07 13:37 ` Jarkko Sakkinen
0 siblings, 0 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2021-09-07 13:37 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson
Cc: Dave Hansen, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, Tony Luck, linux-sgx, linux-kernel, kvm
On Mon, 2021-09-06 at 10:35 +0200, Paolo Bonzini wrote:
> On 03/09/21 17:58, Jarkko Sakkinen wrote:
> > > Eh, it doesn't really simplify the usage. If anything it makes it more convoluted
> > > because the capability check in kvm_vm_ioctl_check_extension() still needs an
> > > #ifdef, e.g. readers will wonder why the check is conditional but the usage is not.
> > It does objectively a bit, since it's one ifdef less.
>
> But you're effectively replacing #ifdef CONFIG_X86_SGX_KVM with #ifdef
> CONFIG_X86_SGX; so the patch is not a no-op as far as KVM is concerned.
>
> So NACK for the KVM parts (yeah I know it's RFC but just to be clearer),
> but I agree that adding a stub inline version of the function is
> standard practice and we do it a lot in KVM too.
OK, this is perfectly fine for me (I care most that we can do this in
SGX side).
/Jarkko
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-07 13:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-03 6:41 [PATCH] x86/sgx: Declare sgx_set_attribute() for !CONFIG_X86_SGX Jarkko Sakkinen
2021-09-03 15:29 ` Sean Christopherson
2021-09-03 15:58 ` Jarkko Sakkinen
2021-09-03 16:33 ` Jarkko Sakkinen
2021-09-06 8:35 ` Paolo Bonzini
2021-09-07 13:37 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).