RE: [PATCH v5 09/12] nfit/libnvdimm: add support for issue secure erase DSM to Intel nvdimm

From: "Elliott, Robert (Persistent Memory)" <elliott@hpe.com>
To: Dave Jiang <dave.jiang@intel.com>,
	"Williams, Dan J" <dan.j.williams@intel.com>
Cc: "dhowells@redhat.com" <dhowells@redhat.com>,
	"Schofield, Alison  <alison.schofield@intel.com>,
	keyrings@vger.kernel.org" <keyrings@vger.kernel.org>,
	"keescook@chromium.org" <keescook@chromium.org>,
	"linux-nvdimm@lists.01.org" <linux-nvdimm@lists.01.org>
Subject: RE: [PATCH v5 09/12] nfit/libnvdimm: add support for issue secure erase DSM to Intel nvdimm
Date: Thu, 19 Jul 2018 01:43:17 +0000	[thread overview]
Message-ID: <AT5PR8401MB1169AAD6B37FAA0004A2D334AB520@AT5PR8401MB1169.NAMPRD84.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <f9873585-2f9a-6f15-0783-7f784b777ce4@intel.com>

> -----Original Message-----
> From: Dave Jiang <dave.jiang@intel.com>
> Sent: Wednesday, July 18, 2018 12:41 PM
> To: Elliott, Robert (Persistent Memory) <elliott@hpe.com>; Williams,
> Dan J <dan.j.williams@intel.com>
> Cc: dhowells@redhat.com; Schofield, Alison
> <alison.schofield@intel.com>; keyrings@vger.kernel.org;
> keescook@chromium.org; linux-nvdimm@lists.01.org
> Subject: Re: [PATCH v5 09/12] nfit/libnvdimm: add support for issue
> secure erase DSM to Intel nvdimm
> 
> 
> 
> On 07/18/2018 10:27 AM, Elliott, Robert (Persistent Memory) wrote:
> >
> >
> >> -----Original Message-----
> >> From: Linux-nvdimm [mailto:linux-nvdimm-bounces@lists.01.org] On
> Behalf Of Dave Jiang
> >> Sent: Tuesday, July 17, 2018 3:55 PM
> >> Subject: [PATCH v5 09/12] nfit/libnvdimm: add support for issue
> secure erase DSM to Intel nvdimm
> > ...
> >  +static int intel_dimm_security_erase(struct nvdimm_bus
> *nvdimm_bus,
> >> +struct nvdimm *nvdimm, struct nvdimm_key_data *nkey)
> > ...
> >> +/* DIMM unlocked, invalidate all CPU caches before we read it */
> >> +wbinvd_on_all_cpus();
> >
> > For this function, that comment should use "erased" rather than
> > "unlocked".
> >
> > For both this function and intel_dimm_security_unlock() in patch
> 04/12,
> > could the driver do a loop of clflushopts on one CPU via
> > clflush_cache_range() rather than run wbinvd on all CPUs?
> 
> The loop should work, but wbinvd is going to be less overall impact
> to the performance for really huge ranges. Also, unlock should happen
> only once and during NVDIMM initialization. So wbinvd should be ok.

Unlike unlock, secure erase could be requested at any time.

wbinvd must run on every physical core on every physical CPU, while
clflushopt flushes everything from just one CPU core.

wbinvd adds huge interrupt latencies, generating complaints like these:
	https://patchwork.kernel.org/patch/37090/
	https://lists.xenproject.org/archives/html/xen-devel/2011-09/msg00675.html

Also, there's no need to disrupt cache content for other addresses;
only the data at the addresses just erased or unlocked is a concern.
clflushopt avoids disrupting other threads.

Related topic: a flush is also necessary before sending the secure erase or
unlock command.  Otherwise, there could be dirty write data that gets
written by the concluding flush (overwriting the now-unlocked or just-erased
data).  For unlock during boot, you might assume that no writes having
occurred yet, but that isn't true for secure erase on demand.  Flushing
before both commands is safest.

---
Robert Elliott, HPE Persistent Memory

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm