| Message ID | e36ac729b227d728e2b0d1a48cfbbeca4523f1a5.1610935432.git.kai.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM SGX virtualization support | expand |
On Mon, Jan 18, 2021 at 04:28:04PM +1300, Kai Huang wrote: > From: Sean Christopherson <sean.j.christopherson@intel.com> > > Add a helper to extract the fault indicator from an encoded ENCLS return > value. SGX virtualization will also need to detect ENCLS faults. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > Acked-by: Dave Hansen <dave.hansen@intel.com> > Signed-off-by: Kai Huang <kai.huang@intel.com> > --- > arch/x86/kernel/cpu/sgx/encls.h | 14 +++++++++++++- > arch/x86/kernel/cpu/sgx/ioctl.c | 2 +- > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h > index be5c49689980..55919a2b01b0 100644 > --- a/arch/x86/kernel/cpu/sgx/encls.h > +++ b/arch/x86/kernel/cpu/sgx/encls.h > @@ -40,6 +40,18 @@ > } while (0); \ > } > > +/* > + * encls_faulted() - Check if an ENCLS leaf faulted given an error code > + * @ret the return value of an ENCLS leaf function call > + * > + * Return: > + * %true if @ret indicates a fault, %false otherwise Follow here the style of commenting as in ioctl.c, for the return value. It has optimal readability both as text, and also when converted to HTML. See sgx_ioc_enclave_add_pages() for an example. > + */ > +static inline bool encls_faulted(int ret) > +{ > + return ret & ENCLS_FAULT_FLAG; > +} > + > /** > * encls_failed() - Check if an ENCLS function failed > * @ret: the return value of an ENCLS function call > @@ -50,7 +62,7 @@ > */ > static inline bool encls_failed(int ret) > { > - if (ret & ENCLS_FAULT_FLAG) > + if (encls_faulted(ret)) > return ENCLS_TRAPNR(ret) != X86_TRAP_PF; > > return !!ret; > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > index 90a5caf76939..e5977752c7be 100644 > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > @@ -568,7 +568,7 @@ static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct, > } > } > > - if (ret & ENCLS_FAULT_FLAG) { > + if (encls_faulted(ret)) { > if (encls_failed(ret)) > ENCLS_WARN(ret, "EINIT"); > > -- > 2.29.2 > > /Jarkko
On Wed, 20 Jan 2021 14:03:08 +0200 Jarkko Sakkinen wrote: > On Mon, Jan 18, 2021 at 04:28:04PM +1300, Kai Huang wrote: > > From: Sean Christopherson <sean.j.christopherson@intel.com> > > > > Add a helper to extract the fault indicator from an encoded ENCLS return > > value. SGX virtualization will also need to detect ENCLS faults. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > Acked-by: Dave Hansen <dave.hansen@intel.com> > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > --- > > arch/x86/kernel/cpu/sgx/encls.h | 14 +++++++++++++- > > arch/x86/kernel/cpu/sgx/ioctl.c | 2 +- > > 2 files changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h > > index be5c49689980..55919a2b01b0 100644 > > --- a/arch/x86/kernel/cpu/sgx/encls.h > > +++ b/arch/x86/kernel/cpu/sgx/encls.h > > @@ -40,6 +40,18 @@ > > } while (0); \ > > } > > > > +/* > > + * encls_faulted() - Check if an ENCLS leaf faulted given an error code > > + * @ret the return value of an ENCLS leaf function call > > + * > > + * Return: > > + * %true if @ret indicates a fault, %false otherwise > > Follow here the style of commenting as in ioctl.c, for the return value. > It has optimal readability both as text, and also when converted to HTML. > See sgx_ioc_enclave_add_pages() for an example. You mean something like below? Return: - %true: @ret indicates a fault. - %false: @ret indicates no fault.
On Thu, Jan 21, 2021 at 12:43:59PM +1300, Kai Huang wrote: > On Wed, 20 Jan 2021 14:03:08 +0200 Jarkko Sakkinen wrote: > > On Mon, Jan 18, 2021 at 04:28:04PM +1300, Kai Huang wrote: > > > From: Sean Christopherson <sean.j.christopherson@intel.com> > > > > > > Add a helper to extract the fault indicator from an encoded ENCLS return > > > value. SGX virtualization will also need to detect ENCLS faults. > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > Acked-by: Dave Hansen <dave.hansen@intel.com> > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > > --- > > > arch/x86/kernel/cpu/sgx/encls.h | 14 +++++++++++++- > > > arch/x86/kernel/cpu/sgx/ioctl.c | 2 +- > > > 2 files changed, 14 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h > > > index be5c49689980..55919a2b01b0 100644 > > > --- a/arch/x86/kernel/cpu/sgx/encls.h > > > +++ b/arch/x86/kernel/cpu/sgx/encls.h > > > @@ -40,6 +40,18 @@ > > > } while (0); \ > > > } > > > > > > +/* > > > + * encls_faulted() - Check if an ENCLS leaf faulted given an error code > > > + * @ret the return value of an ENCLS leaf function call > > > + * > > > + * Return: > > > + * %true if @ret indicates a fault, %false otherwise > > > > Follow here the style of commenting as in ioctl.c, for the return value. > > It has optimal readability both as text, and also when converted to HTML. > > See sgx_ioc_enclave_add_pages() for an example. > > You mean something like below? > > Return: > - %true: @ret indicates a fault. > - %false: @ret indicates no fault. Yeah, with '\t' indentation. I'd remove also '%'. Also '@ret' is redudant. To put this all together: * Return: * - true: ENCLS leaf faulted. * - false: Otherwise. I tried various ways and this was the best way to document return values that i've found. It's easy to read as plain text, and also has the benefit that return values get nicely lined up in htmldocs. I've been even considering a patch for https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt The only advice given ATM is: "Take a look around the source tree for examples." /Jarkko
On Thu, 21 Jan 2021 03:08:18 +0200 Jarkko Sakkinen wrote: > On Thu, Jan 21, 2021 at 12:43:59PM +1300, Kai Huang wrote: > > On Wed, 20 Jan 2021 14:03:08 +0200 Jarkko Sakkinen wrote: > > > On Mon, Jan 18, 2021 at 04:28:04PM +1300, Kai Huang wrote: > > > > From: Sean Christopherson <sean.j.christopherson@intel.com> > > > > > > > > Add a helper to extract the fault indicator from an encoded ENCLS return > > > > value. SGX virtualization will also need to detect ENCLS faults. > > > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > > Acked-by: Dave Hansen <dave.hansen@intel.com> > > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > > > --- > > > > arch/x86/kernel/cpu/sgx/encls.h | 14 +++++++++++++- > > > > arch/x86/kernel/cpu/sgx/ioctl.c | 2 +- > > > > 2 files changed, 14 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h > > > > index be5c49689980..55919a2b01b0 100644 > > > > --- a/arch/x86/kernel/cpu/sgx/encls.h > > > > +++ b/arch/x86/kernel/cpu/sgx/encls.h > > > > @@ -40,6 +40,18 @@ > > > > } while (0); \ > > > > } > > > > > > > > +/* > > > > + * encls_faulted() - Check if an ENCLS leaf faulted given an error code > > > > + * @ret the return value of an ENCLS leaf function call > > > > + * > > > > + * Return: > > > > + * %true if @ret indicates a fault, %false otherwise > > > > > > Follow here the style of commenting as in ioctl.c, for the return value. > > > It has optimal readability both as text, and also when converted to HTML. > > > See sgx_ioc_enclave_add_pages() for an example. > > > > You mean something like below? > > > > Return: > > - %true: @ret indicates a fault. > > - %false: @ret indicates no fault. > > Yeah, with '\t' indentation. I'd remove also '%'. Also '@ret' is redudant. > > To put this all together: > > * Return: > * - true: ENCLS leaf faulted. > * - false: Otherwise. > > I tried various ways and this was the best way to document return values > that i've found. It's easy to read as plain text, and also has the benefit > that return values get nicely lined up in htmldocs. > > I've been even considering a patch for > > https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt > > The only advice given ATM is: "Take a look around the source tree for > examples." OK. Will change to yours. Thanks for the info. > > /Jarkko
On Thu, Jan 21, 2021 at 02:12:21PM +1300, Kai Huang wrote: > On Thu, 21 Jan 2021 03:08:18 +0200 Jarkko Sakkinen wrote: > > On Thu, Jan 21, 2021 at 12:43:59PM +1300, Kai Huang wrote: > > > On Wed, 20 Jan 2021 14:03:08 +0200 Jarkko Sakkinen wrote: > > > > On Mon, Jan 18, 2021 at 04:28:04PM +1300, Kai Huang wrote: > > > > > From: Sean Christopherson <sean.j.christopherson@intel.com> > > > > > > > > > > Add a helper to extract the fault indicator from an encoded ENCLS return > > > > > value. SGX virtualization will also need to detect ENCLS faults. > > > > > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > > > Acked-by: Dave Hansen <dave.hansen@intel.com> > > > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > > > > --- > > > > > arch/x86/kernel/cpu/sgx/encls.h | 14 +++++++++++++- > > > > > arch/x86/kernel/cpu/sgx/ioctl.c | 2 +- > > > > > 2 files changed, 14 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h > > > > > index be5c49689980..55919a2b01b0 100644 > > > > > --- a/arch/x86/kernel/cpu/sgx/encls.h > > > > > +++ b/arch/x86/kernel/cpu/sgx/encls.h > > > > > @@ -40,6 +40,18 @@ > > > > > } while (0); \ > > > > > } > > > > > > > > > > +/* > > > > > + * encls_faulted() - Check if an ENCLS leaf faulted given an error code > > > > > + * @ret the return value of an ENCLS leaf function call > > > > > + * > > > > > + * Return: > > > > > + * %true if @ret indicates a fault, %false otherwise > > > > > > > > Follow here the style of commenting as in ioctl.c, for the return value. > > > > It has optimal readability both as text, and also when converted to HTML. > > > > See sgx_ioc_enclave_add_pages() for an example. > > > > > > You mean something like below? > > > > > > Return: > > > - %true: @ret indicates a fault. > > > - %false: @ret indicates no fault. > > > > Yeah, with '\t' indentation. I'd remove also '%'. Also '@ret' is redudant. > > > > To put this all together: > > > > * Return: > > * - true: ENCLS leaf faulted. > > * - false: Otherwise. > > > > I tried various ways and this was the best way to document return values > > that i've found. It's easy to read as plain text, and also has the benefit > > that return values get nicely lined up in htmldocs. > > > > I've been even considering a patch for > > > > https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt > > > > The only advice given ATM is: "Take a look around the source tree for > > examples." > > OK. Will change to yours. Thanks for the info. There's also two examples of patterns that *do not* work in that file :-) I think I'll post a patch for that file. /Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h index be5c49689980..55919a2b01b0 100644 --- a/arch/x86/kernel/cpu/sgx/encls.h +++ b/arch/x86/kernel/cpu/sgx/encls.h @@ -40,6 +40,18 @@ } while (0); \ } +/* + * encls_faulted() - Check if an ENCLS leaf faulted given an error code + * @ret the return value of an ENCLS leaf function call + * + * Return: + * %true if @ret indicates a fault, %false otherwise + */ +static inline bool encls_faulted(int ret) +{ + return ret & ENCLS_FAULT_FLAG; +} + /** * encls_failed() - Check if an ENCLS function failed * @ret: the return value of an ENCLS function call @@ -50,7 +62,7 @@ */ static inline bool encls_failed(int ret) { - if (ret & ENCLS_FAULT_FLAG) + if (encls_faulted(ret)) return ENCLS_TRAPNR(ret) != X86_TRAP_PF; return !!ret; diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 90a5caf76939..e5977752c7be 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -568,7 +568,7 @@ static int sgx_encl_init(struct sgx_encl *encl, struct sgx_sigstruct *sigstruct, } } - if (ret & ENCLS_FAULT_FLAG) { + if (encls_faulted(ret)) { if (encls_failed(ret)) ENCLS_WARN(ret, "EINIT");