Re: [Intel-gfx] [PATCH 03/11] drm/i915: Allow the sysadmin to override security mitigations

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Sun, Jan 10, 2021 at 03:03:56PM +0000, Chris Wilson wrote:
> The clear-residuals mitigation is a relatively heavy hammer and under some
> circumstances the user may wish to forgo the context isolation in order
> to meet some performance requirement. Introduce a generic module
> parameter to allow selectively enabling/disabling different mitigations.
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/1858

I'm afraid this will have the same faith as the rc6 and the validation impact :/

> Fixes: 47f8253d2b89 ("drm/i915/gen7: Clear all EU/L3 residual contexts")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: stable@vger.kernel.org # v5.7
> ---
>  drivers/gpu/drm/i915/Makefile                 |   1 +
>  .../gpu/drm/i915/gt/intel_ring_submission.c   |   4 +-
>  drivers/gpu/drm/i915/i915_mitigations.c       | 148 ++++++++++++++++++
>  drivers/gpu/drm/i915/i915_mitigations.h       |  13 ++
>  4 files changed, 165 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_mitigations.c
>  create mode 100644 drivers/gpu/drm/i915/i915_mitigations.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 4074d8cb0d6e..48f82c354611 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -38,6 +38,7 @@ i915-y += i915_drv.o \
>  	  i915_config.o \
>  	  i915_irq.o \
>  	  i915_getparam.o \
> +	  i915_mitigations.o \
>  	  i915_params.o \
>  	  i915_pci.o \
>  	  i915_scatterlist.o \
> diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> index 724d56c9583d..657afd8ebc14 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> @@ -32,6 +32,7 @@
>  #include "gen6_ppgtt.h"
>  #include "gen7_renderclear.h"
>  #include "i915_drv.h"
> +#include "i915_mitigations.h"
>  #include "intel_breadcrumbs.h"
>  #include "intel_context.h"
>  #include "intel_gt.h"
> @@ -918,7 +919,8 @@ static int switch_context(struct i915_request *rq)
>  	GEM_BUG_ON(HAS_EXECLISTS(engine->i915));
>  
>  	if (engine->wa_ctx.vma && ce != engine->kernel_context) {
> -		if (engine->wa_ctx.vma->private != ce) {
> +		if (engine->wa_ctx.vma->private != ce &&
> +		    i915_mitigate_clear_residuals()) {
>  			ret = clear_residuals(rq);
>  			if (ret)
>  				return ret;
> diff --git a/drivers/gpu/drm/i915/i915_mitigations.c b/drivers/gpu/drm/i915/i915_mitigations.c
> new file mode 100644
> index 000000000000..8d5637cfa734
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_mitigations.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2021 Intel Corporation
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/moduleparam.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +
> +#include "i915_drv.h"
> +#include "i915_mitigations.h"
> +
> +static unsigned long mitigations = ~0UL;
> +
> +enum {
> +	CLEAR_RESIDUALS = 0,

specially worse if this list grows...

> +};
> +
> +static const char * const names[] = {
> +	[CLEAR_RESIDUALS] = "residuals",
> +};
> +
> +bool i915_mitigate_clear_residuals(void)
> +{
> +	return READ_ONCE(mitigations) & BIT(CLEAR_RESIDUALS);
> +}
> +
> +static int mitigations_set(const char *val, const struct kernel_param *kp)
> +{
> +	unsigned long new = ~0UL;
> +	char *str, *sep, *tok;
> +	bool first = true;
> +	int err = 0;
> +
> +	BUILD_BUG_ON(ARRAY_SIZE(names) >= BITS_PER_TYPE(mitigations));
> +
> +	str = kstrdup(val, GFP_KERNEL);
> +	if (!str)
> +		return -ENOMEM;
> +
> +	for (sep = str; (tok = strsep(&sep, ","));) {
> +		bool enable = true;
> +		int i;
> +
> +		/* Be tolerant of leading/trailing whitespace */
> +		tok = strim(tok);
> +
> +		if (first) {
> +			first = false;
> +
> +			if (!strcmp(tok, "auto")) {
> +				new = ~0UL;
> +				continue;
> +			}
> +
> +			new = 0;
> +			if (!strcmp(tok, "off"))
> +				continue;
> +		}
> +
> +		if (*tok == '!') {
> +			enable = !enable;
> +			tok++;
> +		}
> +
> +		if (!strncmp(tok, "no", 2)) {
> +			enable = !enable;
> +			tok += 2;
> +		}
> +
> +		if (*tok == '\0')
> +			continue;
> +
> +		for (i = 0; i < ARRAY_SIZE(names); i++) {
> +			if (!strcmp(tok, names[i])) {
> +				if (enable)
> +					new |= BIT(i);
> +				else
> +					new &= ~BIT(i);
> +				break;
> +			}
> +		}
> +		if (i == ARRAY_SIZE(names)) {
> +			pr_err("Bad %s.mitigations=%s, '%s' is unknown\n",
> +			       DRIVER_NAME, val, tok);
> +			err = -EINVAL;
> +			break;
> +		}
> +	}
> +	kfree(str);
> +	if (err)
> +		return err;
> +
> +	WRITE_ONCE(mitigations, new);
> +	return 0;
> +}
> +
> +static int mitigations_get(char *buffer, const struct kernel_param *kp)
> +{
> +	unsigned long local = READ_ONCE(mitigations);
> +	int count, i;
> +	bool enable;
> +
> +	if (!local)
> +		return scnprintf(buffer, PAGE_SIZE, "%s\n", "off");
> +
> +	if (local & BIT(BITS_PER_LONG - 1)) {
> +		count = scnprintf(buffer, PAGE_SIZE, "%s,", "auto");
> +		enable = false;
> +	} else {
> +		enable = true;
> +		count = 0;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(names); i++) {
> +		if ((local & BIT(i)) != enable)
> +			continue;
> +
> +		count += scnprintf(buffer + count, PAGE_SIZE - count,
> +				   "%s%s,", enable ? "" : "!", names[i]);
> +	}
> +
> +	buffer[count - 1] = '\n';
> +	return count;
> +}
> +
> +static const struct kernel_param_ops ops = {
> +	.set = mitigations_set,
> +	.get = mitigations_get,
> +};
> +
> +module_param_cb_unsafe(mitigations, &ops, NULL, 0600);
> +MODULE_PARM_DESC(mitigations,
> +"Selectively enable security mitigations for all Intel® GPUs in the system.\n"
> +"\n"
> +"  auto -- enables all mitigations required for the platform [default]\n"
> +"  off  -- disables all mitigations\n"
> +"\n"
> +"Individual mitigations can be enabled by passing a comma-separated string,\n"
> +"e.g. mitigations=residuals to enable only clearing residuals or\n"
> +"mitigations=auto,noresiduals to disable only the clear residual mitigation.\n"
> +"Either '!' or 'no' may be used to switch from enabling the mitigation to\n"
> +"disabling it.\n"

but I liked this structure to at least stop the growing of the params...

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +"\n"
> +"Active mitigations for Ivybridge, Baytrail, Haswell:\n"
> +"  residuals -- clear all thread-local registers between contexts"
> +);
> diff --git a/drivers/gpu/drm/i915/i915_mitigations.h b/drivers/gpu/drm/i915/i915_mitigations.h
> new file mode 100644
> index 000000000000..1359d8135287
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_mitigations.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2021 Intel Corporation
> + */
> +
> +#ifndef __I915_MITIGATIONS_H__
> +#define __I915_MITIGATIONS_H__
> +
> +#include <linux/types.h>
> +
> +bool i915_mitigate_clear_residuals(void);
> +
> +#endif /* __I915_MITIGATIONS_H__ */
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx