Re: [PATCH 4/5] nvdimm/region: Move cache management to the region driver

[Davidlohr Bueso <dave@stgolabs.net>]

On Thu, 01 Dec 2022, Dan Williams wrote:

>Now that cpu_cache_invalidate_memregion() is generically available, use
>it to centralize CPU cache management in the nvdimm region driver.
>
>This trades off removing redundant per-dimm CPU cache flushing with an
>opportunistic flush on every region disable event to cover the case of
>sensitive dirty data in the cache being written back to media after a
>secure erase / overwrite event.

Very nifty.

>Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

with a few notes below.

>+static int nd_region_invalidate_memregion(struct nd_region *nd_region)
>+{
>+	int i, incoherent = 0;
>+
>+	for (i = 0; i < nd_region->ndr_mappings; i++) {
>+		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
>+		struct nvdimm *nvdimm = nd_mapping->nvdimm;
>+
>+		if (test_bit(NDD_INCOHERENT, &nvdimm->flags))
>+			incoherent++;

No need to compute the rest, just break out here?

>+	}
>+
>+	if (!incoherent)
>+		return 0;
>+
>+	if (!cpu_cache_has_invalidate_memregion()) {
>+		if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) {
>+			dev_warn(
>+				&nd_region->dev,
>+				"Bypassing cpu_cache_invalidate_memergion() for testing!\n");
>+			goto out;
>+		} else {
>+			dev_err(&nd_region->dev,
>+				"Failed to synchronize CPU cache state\n");
>+			return -ENXIO;
>+		}
>+	}
>+
>+	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
>+out:
>+	for (i = 0; i < nd_region->ndr_mappings; i++) {
>+		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
>+		struct nvdimm *nvdimm = nd_mapping->nvdimm;
>+
>+		clear_bit(NDD_INCOHERENT, &nvdimm->flags);
>+	}
>+
>+	return 0;
>+}
>+
> int nd_region_activate(struct nd_region *nd_region)
> {
>-	int i, j, num_flush = 0;
>+	int i, j, rc, num_flush = 0;
>	struct nd_region_data *ndrd;
>	struct device *dev = &nd_region->dev;
>	size_t flush_data_size = sizeof(void *);
>
>+	rc = nd_region_invalidate_memregion(nd_region);
>+	if (rc)
>+		return rc;
>+
>	nvdimm_bus_lock(&nd_region->dev);
>	for (i = 0; i < nd_region->ndr_mappings; i++) {
>		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
>@@ -85,6 +129,7 @@ int nd_region_activate(struct nd_region *nd_region)
>	}
>	nvdimm_bus_unlock(&nd_region->dev);
>
>+
>	ndrd = devm_kzalloc(dev, sizeof(*ndrd) + flush_data_size, GFP_KERNEL);
>	if (!ndrd)
>		return -ENOMEM;
>@@ -1222,3 +1267,5 @@ int nd_region_conflict(struct nd_region *nd_region, resource_size_t start,
>
>	return device_for_each_child(&nvdimm_bus->dev, &ctx, region_conflict);
> }
>+
>+MODULE_IMPORT_NS(DEVMEM);
>diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
>index 6814339b3dab..a03e3c45f297 100644
>--- a/drivers/nvdimm/security.c
>+++ b/drivers/nvdimm/security.c
>@@ -208,6 +208,8 @@ static int __nvdimm_security_unlock(struct nvdimm *nvdimm)
>	rc = nvdimm->sec.ops->unlock(nvdimm, data);
>	dev_dbg(dev, "key: %d unlock: %s\n", key_serial(key),
>			rc == 0 ? "success" : "fail");
>+	if (rc == 0)
>+		set_bit(NDD_INCOHERENT, &nvdimm->flags);
>
>	nvdimm_put_key(key);
>	nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
>@@ -374,6 +376,8 @@ static int security_erase(struct nvdimm *nvdimm, unsigned int keyid,
>		return -ENOKEY;
>
>	rc = nvdimm->sec.ops->erase(nvdimm, data, pass_type);
>+	if (rc == 0)
>+		set_bit(NDD_INCOHERENT, &nvdimm->flags);
>	dev_dbg(dev, "key: %d erase%s: %s\n", key_serial(key),
>			pass_type == NVDIMM_MASTER ? "(master)" : "(user)",
>			rc == 0 ? "success" : "fail");
>@@ -408,6 +412,8 @@ static int security_overwrite(struct nvdimm *nvdimm, unsigned int keyid)
>		return -ENOKEY;
>
>	rc = nvdimm->sec.ops->overwrite(nvdimm, data);
>+	if (rc == 0)
>+		set_bit(NDD_INCOHERENT, &nvdimm->flags);

Are you relying on hw preventing an incoming region_activate() while the overwrite
operation is in progress to ensure that the flags are stable throughout the whole
op? Currently query-overwrite also provides the flushing guarantees for when the
command is actually complete (at least from a user pov).

Thanks,
Davidlohr