From: "Alastair D'Silva" <alastair@au1.ibm.com> To: Jonathan Cameron <Jonathan.Cameron@Huawei.com> Cc: "Benjamin Herrenschmidt" <benh@kernel.crashing.org>, "Paul Mackerras" <paulus@samba.org>, "Michael Ellerman" <mpe@ellerman.id.au>, "Frederic Barrat" <fbarrat@linux.ibm.com>, "Andrew Donnellan" <ajd@linux.ibm.com>, "Arnd Bergmann" <arnd@arndb.de>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Andrew Morton" <akpm@linux-foundation.org>, "Mauro Carvalho Chehab" <mchehab+samsung@kernel.org>, "David S. Miller" <davem@davemloft.net>, "Rob Herring" <robh@kernel.org>, "Anton Blanchard" <anton@ozlabs.org>, "Krzysztof Kozlowski" <krzk@kernel.org>, "Mahesh Salgaonkar" <mahesh@linux.vnet.ibm.com>, "Madhavan Srinivasan" <maddy@linux.vnet.ibm.com>, "Cédric Le Goater" <clg@kaod.org>, "Anju T Sudhakar" <anju@linux.vnet.ibm.com>, "Hari Bathini" <hbathini@linux.ibm.com>, "Thomas Gleixner" <tglx@linutronix.de>, "Greg Kurz" <groug@kaod.org>, "Nicholas Piggin" <npiggin@gmail.com>, "Masahiro Yamada" <yamada.masahiro@socionext.com>, "Alexey Kardashevskiy" <aik@ozlabs.ru>, linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-nvdimm@lists.01.org, linux-mm@kvack.org Subject: RE: [PATCH v2 24/27] nvdimm/ocxl: Implement Overwrite Date: Wed, 19 Feb 2020 16:13:43 +1100 Message-ID: <03158c6df708f2e4654d36b6241eaf8a7e438ec4.camel@au1.ibm.com> (raw) In-Reply-To: <20200203151019.0000262f@Huawei.com> On Mon, 2020-02-03 at 15:10 +0000, Jonathan Cameron wrote: > On Tue, 3 Dec 2019 14:46:52 +1100 > Alastair D'Silva <alastair@au1.ibm.com> wrote: > > > From: Alastair D'Silva <alastair@d-silva.org> > > > > The near storage command 'Secure Erase' overwrites all data on the > > media. > > > > This patch hooks it up to the security function 'overwrite'. > > > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org> > > A few things to tidy up in here. > > Thanks, > > Jonathan > > > > --- > > drivers/nvdimm/ocxl/scm.c | 164 > > ++++++++++++++++++++++++++++- > > drivers/nvdimm/ocxl/scm_internal.c | 1 + > > drivers/nvdimm/ocxl/scm_internal.h | 17 +++ > > 3 files changed, 180 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/nvdimm/ocxl/scm.c b/drivers/nvdimm/ocxl/scm.c > > index a81eb5916eb3..8deb7862793c 100644 > > --- a/drivers/nvdimm/ocxl/scm.c > > +++ b/drivers/nvdimm/ocxl/scm.c > > @@ -169,6 +169,86 @@ static int scm_reserve_metadata(struct > > scm_data *scm_data, > > return 0; > > } > > > > +/** > > + * scm_overwrite() - Overwrite all data on the card > > + * @scm_data: The SCM device data > > I would mention in here that this exists with the lock held and > where that is unlocked again. Ok > > > + * Return: 0 on success > > + */ > > +int scm_overwrite(struct scm_data *scm_data) > > +{ > > + int rc; > > + > > + mutex_lock(&scm_data->ns_command.lock); > > + > > + rc = scm_ns_command_request(scm_data, NS_COMMAND_SECURE_ERASE); > > + if (rc) > > Perhaps change that goto label to reflect it is the error path rather > than a shared exit route. > Ok > > + goto out; > > + > > + rc = scm_ns_command_execute(scm_data); > > + if (rc) > > + goto out; > > + > > + scm_data->overwrite_state = SCM_OVERWRITE_BUSY; > > + > > + return 0; > > + > > +out: > > + mutex_unlock(&scm_data->ns_command.lock); > > + return rc; > > +} > > + > > +/** > > + * scm_secop_overwrite() - Overwrite all data on the card > > + * @nvdimm: The nvdimm representation of the SCM device to start > > the overwrite on > > + * @key_data: Unused (no security key implementation) > > + * Return: 0 on success > > + */ > > +static int scm_secop_overwrite(struct nvdimm *nvdimm, > > + const struct nvdimm_key_data *key_data) > > +{ > > + struct scm_data *scm_data = nvdimm_provider_data(nvdimm); > > + > > + return scm_overwrite(scm_data); > > +} > > + > > +/** > > + * scm_secop_query_overwrite() - Get the current overwrite state > > + * @nvdimm: The nvdimm representation of the SCM device to start > > the overwrite on > > + * Return: 0 if successful or idle, -EBUSY if busy, -EFAULT if > > failed > > + */ > > +static int scm_secop_query_overwrite(struct nvdimm *nvdimm) > > +{ > > + struct scm_data *scm_data = nvdimm_provider_data(nvdimm); > > + > > + if (scm_data->overwrite_state == SCM_OVERWRITE_BUSY) > > + return -EBUSY; > > + > > + if (scm_data->overwrite_state == SCM_OVERWRITE_FAILED) > > + return -EFAULT; > > + > > + return 0; > > +} > > + > > +/** > > + * scm_secop_get_flags() - return the security flags for the SCM > > device > > All params need to documented in kernel-doc comments. Ok > > > + */ > > +static unsigned long scm_secop_get_flags(struct nvdimm *nvdimm, > > + enum nvdimm_passphrase_type ptype) > > +{ > > + struct scm_data *scm_data = nvdimm_provider_data(nvdimm); > > + > > + if (scm_data->overwrite_state == SCM_OVERWRITE_BUSY) > > + return BIT(NVDIMM_SECURITY_OVERWRITE); > > + > > + return BIT(NVDIMM_SECURITY_DISABLED); > > +} > > + > > +static const struct nvdimm_security_ops sec_ops = { > > + .get_flags = scm_secop_get_flags, > > + .overwrite = scm_secop_overwrite, > > + .query_overwrite = scm_secop_query_overwrite, > > +}; > > + > > /** > > * scm_register_lpc_mem() - Discover persistent memory on a device > > and register it with the NVDIMM subsystem > > * @scm_data: The SCM device data > > @@ -224,10 +304,10 @@ static int scm_register_lpc_mem(struct > > scm_data *scm_data) > > set_bit(NDD_ALIASING, &nvdimm_flags); > > > > snprintf(serial, sizeof(serial), "%llx", fn_config->serial); > > - nd_mapping_desc.nvdimm = nvdimm_create(scm_data->nvdimm_bus, > > scm_data, > > + nd_mapping_desc.nvdimm = __nvdimm_create(scm_data->nvdimm_bus, > > scm_data, > > scm_dimm_attribute_groups, > > nvdimm_flags, nvdimm_cmd_mask, > > - 0, NULL); > > + 0, NULL, serial, &sec_ops); > > if (!nd_mapping_desc.nvdimm) > > return -ENOMEM; > > > > @@ -1530,6 +1610,83 @@ static void scm_dump_error_log(struct > > scm_data *scm_data) > > kfree(buf); > > } > > > > +static void scm_handle_nscra_doorbell(struct scm_data *scm_data) > > +{ > > + int rc; > > + > > + if (scm_data->ns_command.op_code == NS_COMMAND_SECURE_ERASE) { > > Feels likely that we are going to end up with quite a few blocks like > this as > the driver is extended. Perhaps just start out with a switch > statement and > separate functions that it calls? > At the moment, this is the only near storage command documented on the device, and I don't think there will be any more. > > + u64 success, attempted; > > + > > One is enough here. > It's not, there is a comparison between them later. > > + > > + rc = scm_ns_response(scm_data); > > + if (rc < 0) { > > + scm_data->overwrite_state = > > SCM_OVERWRITE_FAILED; > > If this were a separate function as suggested above, I'd use a goto > to ensure we > unlock in all paths. > > > + mutex_unlock(&scm_data->ns_command.lock); > > + return; > > + } > > + if (rc != STATUS_SUCCESS) > > + scm_warn_status(scm_data, "Unexpected status > > from overwrite", rc); > > + > > + rc = ocxl_global_mmio_read64(scm_data->ocxl_afu, > > + scm_data- > > >ns_command.response_offset + > > + NS_RESPONSE_SECURE_ERASE_A > > CCESSIBLE_SUCCESS, > > + OCXL_HOST_ENDIAN, > > &success); > > + if (rc) { > > + scm_data->overwrite_state = > > SCM_OVERWRITE_FAILED; > > + mutex_unlock(&scm_data->ns_command.lock); > > + return; > > + } > > + > > + rc = ocxl_global_mmio_read64(scm_data->ocxl_afu, > > + scm_data- > > >ns_command.response_offset + > > + NS_RESPONSE_SECURE_ERASE_A > > CCESSIBLE_ATTEMPTED, > > + OCXL_HOST_ENDIAN, > > &attempted); > > + if (rc) { > > + scm_data->overwrite_state = > > SCM_OVERWRITE_FAILED; > > + mutex_unlock(&scm_data->ns_command.lock); > > + return; > > + } > > + > > + scm_data->overwrite_state = SCM_OVERWRITE_SUCCESS; > > + if (success != attempted) > > + scm_data->overwrite_state = > > SCM_OVERWRITE_FAILED; > > + > > + dev_info(&scm_data->dev, > > + "Overwritten %llu/%llu accessible pages", > > success, attempted); > > Do we want to spam the log? Feels like dev_dbg maybe? This only occurs once per overwrite operation. Each overwrite operation is expected to take a non-trivial amount of time. > > > + > > + rc = ocxl_global_mmio_read64(scm_data->ocxl_afu, > > + scm_data- > > >ns_command.response_offset + > > + NS_RESPONSE_SECURE_ERASE_D > > EFECTIVE_SUCCESS, > > + OCXL_HOST_ENDIAN, > > &success); > > + if (rc) { > > + scm_data->overwrite_state = > > SCM_OVERWRITE_FAILED; > > + mutex_unlock(&scm_data->ns_command.lock); > > + return; > > + } > > + > > + rc = ocxl_global_mmio_read64(scm_data->ocxl_afu, > > + scm_data- > > >ns_command.response_offset + > > + NS_RESPONSE_SECURE_ERASE_D > > EFECTIVE_ATTEMPTED, > > + OCXL_HOST_ENDIAN, > > &attempted); > > + if (rc) { > > + scm_data->overwrite_state = > > SCM_OVERWRITE_FAILED; > > + mutex_unlock(&scm_data->ns_command.lock); > > + return; > > + } > > + > > + if (success != attempted) > > + scm_data->overwrite_state = > > SCM_OVERWRITE_FAILED; > > + > > + dev_info(&scm_data->dev, > > + "Overwritten %llu/%llu defective pages", > > success, attempted); > > Again, maybe dev_dbg? > (see above) > > + > > + scm_ns_response_handled(scm_data); > > + > > + mutex_unlock(&scm_data->ns_command.lock); > > + return; > > + } > > +} > > + > > static irqreturn_t scm_imn0_handler(void *private) > > { > > struct scm_data *scm_data = private; > > @@ -1537,6 +1694,9 @@ static irqreturn_t scm_imn0_handler(void > > *private) > > > > (void)scm_chi(scm_data, &chi); > > > > + if (chi & GLOBAL_MMIO_CHI_NSCRA) > > + scm_handle_nscra_doorbell(scm_data); > > + > > if (chi & GLOBAL_MMIO_CHI_ELA) { > > dev_warn(&scm_data->dev, "Error log is available\n"); > > > > diff --git a/drivers/nvdimm/ocxl/scm_internal.c > > b/drivers/nvdimm/ocxl/scm_internal.c > > index 8fc849610eaa..db919a23c69b 100644 > > --- a/drivers/nvdimm/ocxl/scm_internal.c > > +++ b/drivers/nvdimm/ocxl/scm_internal.c > > @@ -173,6 +173,7 @@ int scm_ns_response_handled(const struct > > scm_data *scm_data) > > OCXL_LITTLE_ENDIAN, > > GLOBAL_MMIO_CHI_NSCRA); > > } > > > > + > > Stray blank line.. Sneaky things... > > > void scm_warn_status(const struct scm_data *scm_data, const char > > *message, > > u8 status) > > { > > diff --git a/drivers/nvdimm/ocxl/scm_internal.h > > b/drivers/nvdimm/ocxl/scm_internal.h > > index af19813a7f75..4a29088612a9 100644 > > --- a/drivers/nvdimm/ocxl/scm_internal.h > > +++ b/drivers/nvdimm/ocxl/scm_internal.h > > @@ -70,6 +70,15 @@ > > #define ADMIN_COMMAND_CMD_CAPS 0x08u > > #define ADMIN_COMMAND_MAX 0x08u > > > > +#define NS_COMMAND_SECURE_ERASE 0x20ull > > + > > +#define NS_RESPONSE_SECURE_ERASE_ACCESSIBLE_SUCCESS 0x20 > > +#define NS_RESPONSE_SECURE_ERASE_ACCESSIBLE_ATTEMPTED 0x28 > > +#define NS_RESPONSE_SECURE_ERASE_DEFECTIVE_SUCCESS 0x30 > > +#define NS_RESPONSE_SECURE_ERASE_DEFECTIVE_ATTEMPTED 0x38 > > + > > Lot of blank lines... Whoops > > > + > > + > > #define STATUS_SUCCESS 0x00 > > #define STATUS_MEM_UNAVAILABLE 0x20 > > #define STATUS_BAD_OPCODE 0x50 > > @@ -99,6 +108,13 @@ struct scm_function_0 { > > struct ocxl_fn *ocxl_fn; > > }; > > > > +enum overwrite_state { > > + SCM_OVERWRITE_IDLE = 0, > > + SCM_OVERWRITE_BUSY, > > + SCM_OVERWRITE_SUCCESS, > > + SCM_OVERWRITE_FAILED > > +}; > > + > > struct scm_data { > > struct device dev; > > struct pci_dev *pdev; > > @@ -116,6 +132,7 @@ struct scm_data { > > void *metadata_addr; > > struct command_metadata admin_command; > > struct command_metadata ns_command; > > + enum overwrite_state overwrite_state; > > struct resource scm_res; > > struct nd_region *nd_region; > > struct eventfd_ctx *ev_ctx; -- Alastair D'Silva Open Source Developer Linux Technology Centre, IBM Australia mob: 0423 762 819 _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
next prev parent reply index Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-12-03 3:46 [PATCH v2 00/27] Add support for OpenCAPI SCM devices Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 01/27] memory_hotplug: Add a bounds check to __add_pages Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 02/27] nvdimm: remove prototypes for nonexistent functions Alastair D'Silva 2019-12-03 4:47 ` Andrew Donnellan 2019-12-04 0:10 ` Dan Williams 2020-01-23 21:49 ` Dan Williams 2019-12-03 3:46 ` [PATCH v2 03/27] powerpc: Add OPAL calls for LPC memory alloc/release Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 04/27] mm/memory_hotplug: Allow check_hotplug_memory_addressable to be called from drivers Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 05/27] powerpc: Map & release OpenCAPI LPC memory Alastair D'Silva 2020-01-09 14:41 ` Frederic Barrat 2020-01-21 6:46 ` Andrew Donnellan 2020-01-21 7:11 ` Greg Kurz 2020-02-14 11:09 ` Frederic Barrat 2020-02-18 23:44 ` Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 06/27] ocxl: Tally up the LPC memory on a link & allow it to be mapped Alastair D'Silva 2020-01-09 14:48 ` Frederic Barrat 2020-02-03 12:37 ` Jonathan Cameron 2020-02-19 0:01 ` Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 07/27] ocxl: Add functions to map/unmap LPC memory Alastair D'Silva 2020-01-09 14:49 ` Frederic Barrat 2020-02-03 12:49 ` Jonathan Cameron 2020-02-19 2:39 ` Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 08/27] ocxl: Save the device serial number in ocxl_fn Alastair D'Silva 2020-02-03 12:53 ` Jonathan Cameron 2020-02-19 4:03 ` Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 09/27] ocxl: Free detached contexts in ocxl_context_detach_all() Alastair D'Silva 2020-01-09 14:54 ` Frederic Barrat 2019-12-03 3:46 ` [PATCH v2 10/27] nvdimm: Add driver for OpenCAPI Storage Class Memory Alastair D'Silva 2019-12-03 5:05 ` Alastair D'Silva 2020-02-03 13:20 ` Jonathan Cameron 2020-02-19 4:40 ` Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 11/27] nvdimm/ocxl: Add register addresses & status values to header Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 12/27] nvdimm/ocxl: Read the capability registers & wait for device ready Alastair D'Silva 2020-02-03 13:23 ` Jonathan Cameron 2020-02-19 4:46 ` Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 13/27] nvdimm/ocxl: Add support for Admin commands Alastair D'Silva 2020-02-03 14:18 ` Jonathan Cameron 2020-02-19 5:00 ` Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 14/27] nvdimm/ocxl: Add support for near storage commands Alastair D'Silva 2020-02-03 14:22 ` Jonathan Cameron 2020-02-19 4:54 ` Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 15/27] nvdimm/ocxl: Register a character device for userspace to interact with Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 16/27] nvdimm/ocxl: Implement the Read Error Log command Alastair D'Silva 2019-12-05 3:42 ` Alastair D'Silva 2019-12-05 19:34 ` kbuild test robot 2019-12-03 3:46 ` [PATCH v2 17/27] nvdimm/ocxl: Add controller dump IOCTLs Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 18/27] nvdimm/ocxl: Add an IOCTL to report controller statistics Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 19/27] nvdimm/ocxl: Forward events to userspace Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 20/27] nvdimm/ocxl: Add an IOCTL to request controller health & perf data Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 21/27] nvdimm/ocxl: Support firmware update via sysfs Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 22/27] nvdimm/ocxl: Implement the heartbeat command Alastair D'Silva 2020-02-03 15:11 ` Jonathan Cameron 2020-02-19 5:02 ` Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 23/27] nvdimm/ocxl: Add debug IOCTLs Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 24/27] nvdimm/ocxl: Implement Overwrite Alastair D'Silva 2020-02-03 15:10 ` Jonathan Cameron 2020-02-19 5:13 ` Alastair D'Silva [this message] 2019-12-03 3:46 ` [PATCH v2 25/27] nvdimm/ocxl: Expose SMART data via ndctl Alastair D'Silva 2019-12-16 0:15 ` Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 26/27] powerpc: Enable OpenCAPI Storage Class Memory driver on bare metal Alastair D'Silva 2019-12-03 4:54 ` Andrew Donnellan 2019-12-03 4:57 ` Alastair D'Silva 2019-12-03 3:46 ` [PATCH v2 27/27] MAINTAINERS: Add myself & nvdimm/ocxl to ocxl Alastair D'Silva 2019-12-03 3:50 ` [PATCH v2 00/27] Add support for OpenCAPI SCM devices Matthew Wilcox 2019-12-03 4:01 ` Alastair D'Silva 2019-12-03 12:42 ` Matthew Wilcox 2019-12-04 0:15 ` Dan Williams
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=03158c6df708f2e4654d36b6241eaf8a7e438ec4.camel@au1.ibm.com \ --to=alastair@au1.ibm.com \ --cc=Jonathan.Cameron@Huawei.com \ --cc=aik@ozlabs.ru \ --cc=ajd@linux.ibm.com \ --cc=akpm@linux-foundation.org \ --cc=anju@linux.vnet.ibm.com \ --cc=anton@ozlabs.org \ --cc=arnd@arndb.de \ --cc=benh@kernel.crashing.org \ --cc=clg@kaod.org \ --cc=davem@davemloft.net \ --cc=fbarrat@linux.ibm.com \ --cc=gregkh@linuxfoundation.org \ --cc=groug@kaod.org \ --cc=hbathini@linux.ibm.com \ --cc=krzk@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=linux-nvdimm@lists.01.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=maddy@linux.vnet.ibm.com \ --cc=mahesh@linux.vnet.ibm.com \ --cc=mchehab+samsung@kernel.org \ --cc=mpe@ellerman.id.au \ --cc=npiggin@gmail.com \ --cc=paulus@samba.org \ --cc=robh@kernel.org \ --cc=tglx@linutronix.de \ --cc=yamada.masahiro@socionext.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Linux-NVDIMM Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/linux-nvdimm/0 linux-nvdimm/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 linux-nvdimm linux-nvdimm/ https://lore.kernel.org/linux-nvdimm \ linux-nvdimm@lists.01.org public-inbox-index linux-nvdimm Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.01.lists.linux-nvdimm AGPL code for this site: git clone https://public-inbox.org/public-inbox.git