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

[Dan Williams <dan.j.williams@intel.com>]

Davidlohr Bueso wrote:
> 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?

Sure, makes sense.

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

The driver handles this in nd_region_activate(), but hey, look at that,
nd_region_invalidate_memregion() is too early. I.e. it's before this check:

                if (test_bit(NDD_SECURITY_OVERWRITE, &nvdimm->flags)) {
                        nvdimm_bus_unlock(&nd_region->dev);
                        return -EBUSY;
                }

...which means that the cache could be invalidated too early while the
overwrite is still happening. Will move the cache invalidate below that
check. Thanks for poking at it!

Folded the following:

diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index c73e3b1fd0a6..83dbf398ea84 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -67,8 +67,10 @@ static int nd_region_invalidate_memregion(struct nd_region *nd_region)
                struct nd_mapping *nd_mapping = &nd_region->mapping[i];
                struct nvdimm *nvdimm = nd_mapping->nvdimm;
 
-               if (test_bit(NDD_INCOHERENT, &nvdimm->flags))
+               if (test_bit(NDD_INCOHERENT, &nvdimm->flags)) {
                        incoherent++;
+                       break;
+               }
        }
 
        if (!incoherent)
@@ -106,10 +108,6 @@ int nd_region_activate(struct nd_region *nd_region)
        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];
@@ -129,6 +127,9 @@ int nd_region_activate(struct nd_region *nd_region)
        }
        nvdimm_bus_unlock(&nd_region->dev);
 
+       rc = nd_region_invalidate_memregion(nd_region);
+       if (rc)
+               return rc;
 
        ndrd = devm_kzalloc(dev, sizeof(*ndrd) + flush_data_size, GFP_KERNEL);
        if (!ndrd)