Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: 0meLswPweA+fJ7t6AYDAi2CMYqOuDcup7O26jdFHwoy4rttKfYaT3vspwtfDB8A9DHuc0ojJjJ
 XEjLjfx+2fTQ==
IronPort-SDR: pIShAQU7CxUI9esdy7kkM0IsTpmGk3+oIe95TTbLQTR/KNGLHoaxN7spIMrr806ZJG7Lz5EbbT
 esAeXYf+04Pg==
Date:   Tue, 26 May 2020 20:56:14 -0700
From:   Sean Christopherson <sean.j.christopherson@intel.com>
To:     Borislav Petkov <bp@alien8.de>
Cc:     Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
        linux-kernel@vger.kernel.org, x86@kernel.org,
        linux-sgx@vger.kernel.org, akpm@linux-foundation.org,
        dave.hansen@intel.com, nhorman@redhat.com, npmccallum@redhat.com,
        haitao.huang@intel.com, andriy.shevchenko@linux.intel.com,
        tglx@linutronix.de, kai.svahn@intel.com, josh@joshtriplett.org,
        luto@kernel.org, kai.huang@intel.com, rientjes@google.com,
        cedric.xing@intel.com, puiterwijk@redhat.com,
        Serge Ayoun <serge.ayoun@intel.com>,
        Jethro Beekman <jethro@fortanix.com>
Subject: Re: [PATCH v30 07/20] x86/sgx: Enumerate and track EPC sections
Message-ID: <20200527035613.GH31696@linux.intel.com>
References: <20200515004410.723949-1-jarkko.sakkinen@linux.intel.com>
 <20200515004410.723949-8-jarkko.sakkinen@linux.intel.com>
 <20200525092304.GD25636@zn.tnic>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200525092304.GD25636@zn.tnic>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, May 25, 2020 at 11:23:04AM +0200, Borislav Petkov wrote:
> On Fri, May 15, 2020 at 03:43:57AM +0300, Jarkko Sakkinen wrote:
> > +struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS];
> > +int sgx_nr_epc_sections;
> 
> We have become very averse against global stuff. What is going to use
> those, only sgx code I assume...?

Yes, only SGX code.  The reclaim/swap code needs access to the sections,
and that code is in a different file, reclaim.c.  I don't have a super
strong objection to sucking reclaim.c into main.c, but I'm somewhat
indifferent on code organization as a whole.  Jarkko likely has a stronger
opinion.

> > +static bool __init sgx_page_cache_init(void)
> > +{
> > +	u32 eax, ebx, ecx, edx, type;
> > +	u64 pa, size;
> > +	int i;
> > +
> > +	for (i = 0; i <= ARRAY_SIZE(sgx_epc_sections); i++) {
> > +		cpuid_count(SGX_CPUID, i + SGX_CPUID_FIRST_VARIABLE_SUB_LEAF,
> > +			    &eax, &ebx, &ecx, &edx);
> > +
> > +		type = eax & SGX_CPUID_SUB_LEAF_TYPE_MASK;
> > +		if (type == SGX_CPUID_SUB_LEAF_INVALID)
> > +			break;
> > +
> > +		if (type != SGX_CPUID_SUB_LEAF_EPC_SECTION) {
> > +			pr_err_once("Unknown EPC section type: %u\n", type);
> > +			break;
> > +		}
> > +
> > +		if (i == ARRAY_SIZE(sgx_epc_sections)) {
> > +			pr_warn("No free slot for an EPC section\n");
> > +			break;
> > +		}
> 
> This is also the loop termination: do we really need this warn or can
> the loop simply do "i < ARRAY_SIZE" ?

The warn alerts the user that there will effectively be lost/unused memory,
so IMO it's worth keeping.
 
> If the warn is needed, it can be after the loop too.
> > +
> > +		pa = sgx_calc_section_metric(eax, ebx);
> > +		size = sgx_calc_section_metric(ecx, edx);
> > +
> > +		pr_info("EPC section 0x%llx-0x%llx\n", pa, pa + size - 1);
> 
> I'm assuming that's useful information to issue in dmesg?

Yes, it's effectively the equivalent of dumping the e820 tables.  It might
not be as useful now that the code is stable, but I suspect it will come in
handy for debug/triage down the road.
 
> > diff --git a/arch/x86/kernel/cpu/sgx/reclaim.c b/arch/x86/kernel/cpu/sgx/reclaim.c
> > new file mode 100644
> > index 000000000000..215371588a25
> > --- /dev/null
> > +++ b/arch/x86/kernel/cpu/sgx/reclaim.c
> > @@ -0,0 +1,82 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> > +// Copyright(c) 2016-19 Intel Corporation.
> > +
> > +#include <linux/freezer.h>
> > +#include <linux/highmem.h>
> > +#include <linux/kthread.h>
> > +#include <linux/pagemap.h>
> > +#include <linux/ratelimit.h>
> > +#include <linux/slab.h>
> > +#include <linux/sched/mm.h>
> > +#include <linux/sched/signal.h>
> > +#include "encls.h"
> > +
> > +struct task_struct *ksgxswapd_tsk;
> 
> Same for this one: also shared only among sgx code?

Yes, that one can definitely be buried behind a helper.

> > +/**
> > + * enum sgx_epc_page_desc - bits and masks for an EPC page's descriptor
> > + * %SGX_EPC_SECTION_MASK:	SGX allows to have multiple EPC sections in the
> > + *				physical memory. The existing and near-future
> > + *				hardware defines at most eight sections, hence
> > + *				three bits to hold a section.
> > + */
> > +enum sgx_epc_page_desc {
> > +	SGX_EPC_SECTION_MASK			= GENMASK_ULL(3, 0),
> 
> If that should be three bits, then it should be (2, 0). Because now you
> have 4 bits:

Apparently even pre-school math is hard these days.  I'm pretty sure that's
an ongoing brain fart, I don't think we ever conciously bumped it to 4 bits.

That being said, using 4 bits (or even 5+) isn't necessary a bad thing.  We
aren't tight on bits and the burned memory isn't awful, e.g. <100 bytes per
unused section.  Not having to update the kernel just to handle a system
with more EPC sections would be nice, though we (Intel) should check on our
end to see if nearish-future CPUs are still hard limited to eight sections.

One idea would be to provide a Kconfig a la NR_CPUS or NODES_SHIFT.  I.e.
carve out the bits in sgx_epc_page_desc to allow up to N sections, but let
the user limit the number of sections to recoup the unused memory.

> # arch/x86/kernel/cpu/sgx/sgx.h:56:     return &sgx_epc_sections[page->desc & SGX_EPC_SECTION_MASK];
>         andl    $15, %edx       #, _22
> 		^^^
> 
> > +	/* bits 12-63 are reserved for the physical page address of the page */
> > +};