| Message ID | 20210903064156.387979-1-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Declare sgx_set_attribute() for !CONFIG_X86_SGX | expand |
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 >
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
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
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.
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
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)
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(-)