diff mbox series

[v7,18/20] cxl: bypass cpu_cache_invalidate_memregion() when in test config

Message ID 166983619332.2734609.2800078343178136915.stgit@djiang5-desk3.ch.intel.com (mailing list archive)
State New, archived
Headers show
Series Introduce security commands for CXL pmem device | expand

Commit Message

Dave Jiang Nov. 30, 2022, 7:23 p.m. UTC
Bypass cpu_cache_invalidate_memregion() and checks when doing testing
using CONFIG_NVDIMM_SECURITY_TEST flag. The bypass allows testing on
QEMU where cpu_cache_has_invalidate_memregion() fails. Usage of
cpu_cache_invalidate_memregion() is not needed for cxl_test security
testing.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/security.c |   35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

Comments

Davidlohr Bueso Nov. 30, 2022, 10:16 p.m. UTC | #1
On Wed, 30 Nov 2022, Dave Jiang wrote:

>Bypass cpu_cache_invalidate_memregion() and checks when doing testing
>using CONFIG_NVDIMM_SECURITY_TEST flag. The bypass allows testing on
>QEMU where cpu_cache_has_invalidate_memregion() fails. Usage of
>cpu_cache_invalidate_memregion() is not needed for cxl_test security
>testing.

We'll also want something similar for the non-pmem specific security
bits by extending these wrappers with CONFIG_CXL_SECURITY_TEST. I
think the current naming is very generic but the functionality is
too tied to pmem. So I would either rename these to 'cxl_pmem...'
or make them more generic by placing them in cxlmem.h and taking the
dev pointer directly as well as the iores.

Thanks,
Davidlohr
Dan Williams Nov. 30, 2022, 11:54 p.m. UTC | #2
Davidlohr Bueso wrote:
> On Wed, 30 Nov 2022, Dave Jiang wrote:
> 
> >Bypass cpu_cache_invalidate_memregion() and checks when doing testing
> >using CONFIG_NVDIMM_SECURITY_TEST flag. The bypass allows testing on
> >QEMU where cpu_cache_has_invalidate_memregion() fails. Usage of
> >cpu_cache_invalidate_memregion() is not needed for cxl_test security
> >testing.
> 
> We'll also want something similar for the non-pmem specific security
> bits

Wait, you expect someone is going to build a device *with* security
commands but *without* pmem?  In the volatile case the device can just
secure erase itself without user intervention every time power is
removed, no need for explicit user action to trigger that. So the
data-at-rest security argument goes away with a pure volatile device,
no?

> think the current naming is very generic but the functionality is
> too tied to pmem. So I would either rename these to 'cxl_pmem...'
> or make them more generic by placing them in cxlmem.h and taking the
> dev pointer directly as well as the iores.

This does remind me that security is just one use case CXL has for
cpu_cache_has_invalidate_memregion(). It also needs to be used for any
HDM decoder changes where the HPA to DPA translation has changed. I
think this means that any user created region needs to flush CPU caches
before that region goes into service. So I like the idea of separate
cxl_pmem_invalidate_memregion() and cxl_region_invalidate_memregion()
with something like:

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 1e61d1bafc0c..430e8e5ba7d9 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1403,6 +1403,8 @@ static int attach_target(struct cxl_region *cxlr, const char *decoder, int pos)
 		goto out;
 	down_read(&cxl_dpa_rwsem);
 	rc = cxl_region_attach(cxlr, to_cxl_endpoint_decoder(dev), pos);
+	if (rc == 0)
+		set_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags);
 	up_read(&cxl_dpa_rwsem);
 	up_write(&cxl_region_rwsem);
 out:
@@ -1958,6 +1960,33 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
 	return rc;
 }
 
+static bool cxl_region_has_invalidate_memregion(struct cxl_region *cxlr)
+{
+	if (!cpu_cache_has_invalidate_memregion()) {
+		if (IS_ENABLED(CONFIG_CXL_REGION_TEST)) {
+			dev_warn_once(
+				&cxlr->dev,
+				"Bypassing cpu_cache_has_invalidate_memregion() check for testing!\n");
+			return true;
+		}
+		return false;
+	}
+
+	return true;
+}
+
+static void cxl_region_invalidate_memregion(struct cxl_region *cxlr)
+{
+	if (IS_ENABLED(CONFIG_CXL_REGION_TEST)) {
+		dev_warn_once(
+			&cxlr->dev,
+			"Bypassing cpu_cache_invalidate_memergion() for testing!\n");
+		return;
+	}
+
+	cpu_cache_invalidate_memregion(IORES_DESC_CXL);
+}
+
 static int cxl_region_probe(struct device *dev)
 {
 	struct cxl_region *cxlr = to_cxl_region(dev);
@@ -1975,12 +2004,22 @@ static int cxl_region_probe(struct device *dev)
 		rc = -ENXIO;
 	}
 
+	if (test_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags) &&
+	    !cxl_region_has_invalidate_memregion(cxlr)) {
+		dev_err(&cxlr->dev, "Failed to synchronize CPU cache state\n");
+		rc = -ENXIO;
+	} else if (test_and_clear_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags))
+		cxl_region_invalidate_memregion(cxlr);
+
 	/*
 	 * From this point on any path that changes the region's state away from
 	 * CXL_CONFIG_COMMIT is also responsible for releasing the driver.
 	 */
 	up_read(&cxl_region_rwsem);
 
+	if (rc)
+		return rc;
+
 	switch (cxlr->mode) {
 	case CXL_DECODER_PMEM:
 		return devm_cxl_add_pmem_region(cxlr);
@@ -2008,4 +2047,5 @@ void cxl_region_exit(void)
 }
 
 MODULE_IMPORT_NS(CXL);
+MODULE_IMPORT_NS(DEVMEM);
 MODULE_ALIAS_CXL(CXL_DEVICE_REGION);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 9a212ab3cae4..827b1ad6cae4 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -388,12 +388,19 @@ struct cxl_region_params {
 	int nr_targets;
 };
 
+/*
+ * Flag whether this region needs to have its HPA span synchronized with
+ * CPU cache state at region activation time.
+ */
+#define CXL_REGION_F_INCOHERENT BIT(0)
+
 /**
  * struct cxl_region - CXL region
  * @dev: This region's device
  * @id: This region's id. Id is globally unique across all regions
  * @mode: Endpoint decoder allocation / access mode
  * @type: Endpoint decoder target type
+ * @flags: Region state flags
  * @cxl_nvb: nvdimm bridge for coordinating @cxlr_pmem setup / shutdown
  * @cxlr_pmem: (for pmem regions) cached copy of the nvdimm bridge
  * @params: active + config params for the region
@@ -403,6 +410,7 @@ struct cxl_region {
 	int id;
 	enum cxl_decoder_mode mode;
 	enum cxl_decoder_type type;
+	unsigned long flags;
 	struct cxl_nvdimm_bridge *cxl_nvb;
 	struct cxl_pmem_region *cxlr_pmem;
 	struct cxl_region_params params;
Davidlohr Bueso Dec. 1, 2022, 1:51 a.m. UTC | #3
On Wed, 30 Nov 2022, Dan Williams wrote:

>Davidlohr Bueso wrote:
>> On Wed, 30 Nov 2022, Dave Jiang wrote:
>>
>> >Bypass cpu_cache_invalidate_memregion() and checks when doing testing
>> >using CONFIG_NVDIMM_SECURITY_TEST flag. The bypass allows testing on
>> >QEMU where cpu_cache_has_invalidate_memregion() fails. Usage of
>> >cpu_cache_invalidate_memregion() is not needed for cxl_test security
>> >testing.
>>
>> We'll also want something similar for the non-pmem specific security
>> bits
>
>Wait, you expect someone is going to build a device *with* security
>commands but *without* pmem?  In the volatile case the device can just
>secure erase itself without user intervention every time power is
>removed, no need for explicit user action to trigger that. So the
>data-at-rest security argument goes away with a pure volatile device,
>no?

Well the spec explicitly states that sanitation can be done to volatile
capacity devices, which makes me think the use case for this would not
require rebooting.

Thanks,
Davidlohr
Dan Williams Dec. 1, 2022, 3:05 a.m. UTC | #4
Davidlohr Bueso wrote:
> On Wed, 30 Nov 2022, Dan Williams wrote:
> 
> >Davidlohr Bueso wrote:
> >> On Wed, 30 Nov 2022, Dave Jiang wrote:
> >>
> >> >Bypass cpu_cache_invalidate_memregion() and checks when doing testing
> >> >using CONFIG_NVDIMM_SECURITY_TEST flag. The bypass allows testing on
> >> >QEMU where cpu_cache_has_invalidate_memregion() fails. Usage of
> >> >cpu_cache_invalidate_memregion() is not needed for cxl_test security
> >> >testing.
> >>
> >> We'll also want something similar for the non-pmem specific security
> >> bits
> >
> >Wait, you expect someone is going to build a device *with* security
> >commands but *without* pmem?  In the volatile case the device can just
> >secure erase itself without user intervention every time power is
> >removed, no need for explicit user action to trigger that. So the
> >data-at-rest security argument goes away with a pure volatile device,
> >no?
> 
> Well the spec explicitly states that sanitation can be done to volatile
> capacity devices, which makes me think the use case for this would not
> require rebooting.

It does, but Linux supports memory sharing across multiple tenants today
without secure erase. So I think the specification may have been getting
ahead of itself about how useful that is as a concept, at least in the
near term. In the long term, when memory may be shared outside of the
system I think the kernel will have an interest in santizing that memory
before it leaves the local system control, but for that there's the
"Sanitize on Release" facility for DCD regions.

You are right though, I was under the mistaken impression that CXL
Secure Erase behaved like PMEM [1], and only erased the persistent
media, not the volatile, but the spec says it erases everything user
accessible. That's going to need to make it as violent as CXL Sanitize
from the perspective of what needs to be taken offline before the
operation, similar to PMEM Overwrite. However, the cache management for
Sanitize, Secure Erase, and Unlock can all be unified under the same
cxl_region_invalidate_memregion() proposal.

Basically any time the DPA contents are changed behind the kernel's back
either "in-place" by a security operation, or a "remap" event of
assigning different DPA, any associated regions get marked
CXL_REGION_F_INCOHERENT. Then the region driver can handle cache
management centrally if the given device gets reactivated.

That also has the nice property of deferring wbinvd violence until the
last possible moment where the cache incoherence can actually spill over
into a data corruption scenario.

[1]: https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V3.0.pdf
Jonathan Cameron Dec. 1, 2022, 11:11 a.m. UTC | #5
On Wed, 30 Nov 2022 12:23:13 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Bypass cpu_cache_invalidate_memregion() and checks when doing testing
> using CONFIG_NVDIMM_SECURITY_TEST flag. The bypass allows testing on
> QEMU where cpu_cache_has_invalidate_memregion() fails. Usage of
> cpu_cache_invalidate_memregion() is not needed for cxl_test security
> testing.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Subject to naming discussion resolving, this looks good to me..

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/security.c |   35 ++++++++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c
> index cbd005ceb091..2b5088af8fc4 100644
> --- a/drivers/cxl/security.c
> +++ b/drivers/cxl/security.c
> @@ -111,6 +111,31 @@ static int cxl_pmem_security_freeze(struct nvdimm *nvdimm)
>  	return cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_FREEZE_SECURITY, NULL, 0, NULL, 0);
>  }
>  
> +static bool cxl_has_invalidate_memregion(struct cxl_nvdimm *cxl_nvd)
> +{
> +	if (!cpu_cache_has_invalidate_memregion()) {
> +		if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) {
> +			dev_warn_once(&cxl_nvd->dev,
> +				      "Bypassing cpu_cache_has_invalidate_memregion() check for testing!\n");
> +			return true;
> +		}
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static void cxl_invalidate_memregion(struct cxl_nvdimm *cxl_nvd)
> +{
> +	if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) {
> +		dev_warn_once(&cxl_nvd->dev,
> +			      "Bypassing cpu_cache_invalidate_memergion() for testing!\n");
> +		return;
> +	}
> +
> +	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
> +}
> +
>  static int cxl_pmem_security_unlock(struct nvdimm *nvdimm,
>  				    const struct nvdimm_key_data *key_data)
>  {
> @@ -120,7 +145,7 @@ static int cxl_pmem_security_unlock(struct nvdimm *nvdimm,
>  	u8 pass[NVDIMM_PASSPHRASE_LEN];
>  	int rc;
>  
> -	if (!cpu_cache_has_invalidate_memregion())
> +	if (!cxl_has_invalidate_memregion(cxl_nvd))
>  		return -EINVAL;
>  
>  	memcpy(pass, key_data->data, NVDIMM_PASSPHRASE_LEN);
> @@ -130,7 +155,7 @@ static int cxl_pmem_security_unlock(struct nvdimm *nvdimm,
>  		return rc;
>  
>  	/* DIMM unlocked, invalidate all CPU caches before we read it */
> -	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
> +	cxl_invalidate_memregion(cxl_nvd);
>  	return 0;
>  }
>  
> @@ -144,21 +169,21 @@ static int cxl_pmem_security_passphrase_erase(struct nvdimm *nvdimm,
>  	struct cxl_pass_erase erase;
>  	int rc;
>  
> -	if (!cpu_cache_has_invalidate_memregion())
> +	if (!cxl_has_invalidate_memregion(cxl_nvd))
>  		return -EINVAL;
>  
>  	erase.type = ptype == NVDIMM_MASTER ?
>  		CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER;
>  	memcpy(erase.pass, key->data, NVDIMM_PASSPHRASE_LEN);
>  	/* Flush all cache before we erase mem device */
> -	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
> +	cxl_invalidate_memregion(cxl_nvd);
>  	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE,
>  			       &erase, sizeof(erase), NULL, 0);
>  	if (rc < 0)
>  		return rc;
>  
>  	/* mem device erased, invalidate all CPU caches before data is read */
> -	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
> +	cxl_invalidate_memregion(cxl_nvd);
>  	return 0;
>  }
>  
> 
>
diff mbox series

Patch

diff --git a/drivers/cxl/security.c b/drivers/cxl/security.c
index cbd005ceb091..2b5088af8fc4 100644
--- a/drivers/cxl/security.c
+++ b/drivers/cxl/security.c
@@ -111,6 +111,31 @@  static int cxl_pmem_security_freeze(struct nvdimm *nvdimm)
 	return cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_FREEZE_SECURITY, NULL, 0, NULL, 0);
 }
 
+static bool cxl_has_invalidate_memregion(struct cxl_nvdimm *cxl_nvd)
+{
+	if (!cpu_cache_has_invalidate_memregion()) {
+		if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) {
+			dev_warn_once(&cxl_nvd->dev,
+				      "Bypassing cpu_cache_has_invalidate_memregion() check for testing!\n");
+			return true;
+		}
+		return false;
+	}
+
+	return true;
+}
+
+static void cxl_invalidate_memregion(struct cxl_nvdimm *cxl_nvd)
+{
+	if (IS_ENABLED(CONFIG_NVDIMM_SECURITY_TEST)) {
+		dev_warn_once(&cxl_nvd->dev,
+			      "Bypassing cpu_cache_invalidate_memergion() for testing!\n");
+		return;
+	}
+
+	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
+}
+
 static int cxl_pmem_security_unlock(struct nvdimm *nvdimm,
 				    const struct nvdimm_key_data *key_data)
 {
@@ -120,7 +145,7 @@  static int cxl_pmem_security_unlock(struct nvdimm *nvdimm,
 	u8 pass[NVDIMM_PASSPHRASE_LEN];
 	int rc;
 
-	if (!cpu_cache_has_invalidate_memregion())
+	if (!cxl_has_invalidate_memregion(cxl_nvd))
 		return -EINVAL;
 
 	memcpy(pass, key_data->data, NVDIMM_PASSPHRASE_LEN);
@@ -130,7 +155,7 @@  static int cxl_pmem_security_unlock(struct nvdimm *nvdimm,
 		return rc;
 
 	/* DIMM unlocked, invalidate all CPU caches before we read it */
-	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
+	cxl_invalidate_memregion(cxl_nvd);
 	return 0;
 }
 
@@ -144,21 +169,21 @@  static int cxl_pmem_security_passphrase_erase(struct nvdimm *nvdimm,
 	struct cxl_pass_erase erase;
 	int rc;
 
-	if (!cpu_cache_has_invalidate_memregion())
+	if (!cxl_has_invalidate_memregion(cxl_nvd))
 		return -EINVAL;
 
 	erase.type = ptype == NVDIMM_MASTER ?
 		CXL_PMEM_SEC_PASS_MASTER : CXL_PMEM_SEC_PASS_USER;
 	memcpy(erase.pass, key->data, NVDIMM_PASSPHRASE_LEN);
 	/* Flush all cache before we erase mem device */
-	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
+	cxl_invalidate_memregion(cxl_nvd);
 	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE,
 			       &erase, sizeof(erase), NULL, 0);
 	if (rc < 0)
 		return rc;
 
 	/* mem device erased, invalidate all CPU caches before data is read */
-	cpu_cache_invalidate_memregion(IORES_DESC_PERSISTENT_MEMORY);
+	cxl_invalidate_memregion(cxl_nvd);
 	return 0;
 }