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

["Huang, Kai" <kai.huang@intel.com>]

> > > >
> > > > +	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;
        }
        ......
}