Re: [RFC PATCH v3 14/27] x86/sgx: Add helpers to expose ECREATE and EINIT to KVM

Kai Huang
2021-02-02T20:36:23Z

On Tue, 2 Feb 2021 19:20:54 +0200 Jarkko Sakkinen wrote:
> On Mon, Feb 01, 2021 at 01:17:44PM +1300, Kai Huang wrote:
> > On Sat, 30 Jan 2021 16:51:41 +0200 Jarkko Sakkinen wrote:
> > > On Tue, Jan 26, 2021 at 10:31:06PM +1300, Kai Huang wrote:
> > > > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > > >
> > > > The bare-metal kernel must intercept ECREATE to be able to impose policies
> > > > on guests. When it does this, the bare-metal kernel runs ECREATE against
> > > > the userspace mapping of the virtualized EPC.
> > >
> > > I guess Andy's earlier comment applies here, i.e. SGX driver?
> >
> > Sure.
> >
> > [...]
> >
> > > > + }
> > > > +
> > > > + if (encls_faulted(ret)) {
> > > > + *trapnr = ENCLS_TRAPNR(ret);
>
> Also here is an empty line needed.

I honestly don't like putting new line here, since it is just two lines of
code. Adding new line is too sparse I think.

>
> > > > + return -EFAULT;
> > > > + }
> > >
> > > Empty line here before return. Applies also to sgx_virt_ecreate().
> >
> > Yes I can remove, but I am just carious: isn't "having empty line before return"
> > a good coding-style? Do you have any reference to the guideline?
>
> In the initial SGX patch set, this was the review feedback that I got
> from Boris, so I would presume it is tip tree convention. Also, looking
> at a random selection of files under arch/x86, it is commonly done this
> way.

I'll add a new line here. Sorry I misunderstood your original comment.

>
> >
> > >
> > > > + return ret;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(sgx_virt_einit);
> > > > --
> > > > 2.29.2
> > >
> > > Great work. I think this patch sets is shaping up.
> > >
> > > /Jarkko
> > > >
> > > >
> >
>
> /Jarkko