RE: [RFC PATCH v5 06/26] x86/cpu/intel: Allow SGX virtualization without Launch Control support

Huang, Kai
2021-02-16T10:25:33Z

> > > >
> > > > + enable_vmx = cpu_has(c, X86_FEATURE_VMX) &&
> > > > + IS_ENABLED(CONFIG_KVM_INTEL);
> > >
> > > It's less than 100 characters:
> >
> > Just carious, shouldn't be 80 characters to wrap a new line, instead of 100?
>
> Try with checkpatch.pl.

Checkpatch.pl has default value 100, but it can be overwritten. I found below document explicitly said 80 should be the length:

https://www.kernel.org/doc/html/v5.11-rc7/process/coding-style.html

2) Breaking long lines and strings

Coding style is all about readability and maintainability using commonly available tools.

The preferred limit on the length of a single line is 80 columns.

Statements longer than 80 columns should be broken into sensible chunks, unless exceeding 80 columns significantly increases readability and does not hide information.

[...]

> > > > update_sgx:
> > > > - if (!(msr & FEAT_CTL_SGX_ENABLED) ||
> > > > - !(msr & FEAT_CTL_SGX_LC_ENABLED) || !enable_sgx) {
> > > > - if (enable_sgx)
> > > > - pr_err_once("SGX disabled by BIOS\n");
> > > > + if (!(msr & FEAT_CTL_SGX_ENABLED)) {
> > > > + if (enable_sgx_kvm || enable_sgx_driver)
> > > > + pr_err_once("SGX disabled by BIOS.\n");
> > > > clear_cpu_cap(c, X86_FEATURE_SGX);
> > >
> > > Empty line before return statement.
> >
> > It's just two statements inside the if() {} statement. Putting a new line here is
> too sparse IMHO.
> >
> > I'd like to hear more.
>
> This was a common review comment in original SGX series, so I'm sticking to the
> pattern.

Well if you insist, I can do that.

But I am not that convinced. In fact, I also believe that in most cases, having empty line before 'return' is good practice, for instance, when 'return' is the very last statement in the function.

I am also glad to do it if it is a x86 patch convention that we even need to put a new empty line when there are only very few statements inside if() {}. However it seems it is not the case.

For example, I just did a search in SGX driver code, and below examples all DONOT have empty line before return (and I don't think I captured them all):

sgx/driver.c:

static int sgx_release(struct inode *inode, struct file *file)
{
......
kref_put(&encl->refcount, sgx_encl_release);
return 0;
}

sgx/ioctl.c:

static struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl)
{
......
va_page->epc_page = sgx_alloc_va_page();
if (IS_ERR(va_page->epc_page)) {
err = ERR_CAST(va_page->epc_page);
kfree(va_page);
return err;
}

WARN_ON_ONCE(encl->page_cnt % SGX_VA_SLOT_COUNT);
}
encl->page_cnt++;
return va_page;
}

static long sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg)
{
......
kfree(secs);
return ret;
}

static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
unsigned long offset, struct sgx_secinfo *secinfo,
unsigned long flags)
{
......
epc_page = sgx_alloc_epc_page(encl_page, true);
if (IS_ERR(epc_page)) {
kfree(encl_page);
return PTR_ERR(epc_page);
}
......
sgx_mark_page_reclaimable(encl_page->epc_page);
mutex_unlock(&encl->lock);
mmap_read_unlock(current->mm);
return ret;

......
}

And in cpu/feat_ctl.c:

void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
{
......
if (rdmsrl_safe(MSR_IA32_FEAT_CTL, &msr)) {
clear_cpu_cap(c, X86_FEATURE_VMX);
clear_sgx_caps();
return;
}
......
}