linux-kernel - Re: [PATCH v4 6/8] x86: Add hardware prefetch control support for x86

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 18 May 2022 08:43:14 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Kohei Tarumizu <tarumizu.kohei@...itsu.com>
Cc:     catalin.marinas@....com, will@...nel.org, tglx@...utronix.de,
        mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
        x86@...nel.org, hpa@...or.com, rafael@...nel.org,
        mchehab+huawei@...nel.org, eugenis@...gle.com, tony.luck@...el.com,
        pcc@...gle.com, peterz@...radead.org, marcos@...a.pet,
        conor.dooley@...rochip.com, nicolas.ferre@...rochip.com,
        marcan@...can.st, linus.walleij@...aro.org, arnd@...db.de,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v4 6/8] x86: Add hardware prefetch control support for x86

On Wed, May 18, 2022 at 03:30:30PM +0900, Kohei Tarumizu wrote:
> +/*
> + * Returns the cpu number of the cpu_device(/sys/devices/system/cpu/cpuX)
> + * in the ancestor directory of prefetch_control.
> + *
> + * When initializing this driver, it is verified that the cache directory exists
> + * under cpuX device. Therefore, the third level up from prefetch_control is
> + * cpuX device as shown below.
> + *
> + * /sys/devices/system/cpu/cpuX/cache/indexX/prefetch_control
> + */
> +static inline unsigned int pfctl_dev_get_cpu(struct device *pfctl_dev)
> +{
> +	return pfctl_dev->parent->parent->parent->id;

As much fun as it is to see a function like this, that is not ok, and
never guaranteed to keep working, sorry.  Please find the device
properly, never assume a specific driver model topology as they are
guaranteed to break over time and never supposed to be static like this.



> +}
> +
> +static ssize_t
> +pfctl_show(struct device *pfctl_dev, struct device_attribute *attr, char *buf)
> +{
> +	unsigned int cpu = pfctl_dev_get_cpu(pfctl_dev);
> +	struct x86_pfctl_attr *xa;
> +	u64 val;
> +
> +	xa = container_of(attr, struct x86_pfctl_attr, attr);
> +
> +	rdmsrl_on_cpu(cpu, MSR_MISC_FEATURE_CONTROL, &val);
> +	return sysfs_emit(buf, "%d\n", val & xa->mask ? 0 : 1);
> +}
> +
> +struct write_info {
> +	u64 mask;
> +	bool enable;
> +};
> +
> +/*
> + * wrmsrl() in this patch is only done inside of an interrupt-disabled region
> + * to avoid a conflict of write access from other drivers,
> + */
> +static void pfctl_write(void *info)
> +{
> +	struct write_info *winfo = info;
> +	u64 reg;
> +
> +	reg = 0;
> +	rdmsrl(MSR_MISC_FEATURE_CONTROL, reg);
> +
> +	if (winfo->enable)
> +		reg &= ~winfo->mask;
> +	else
> +		reg |= winfo->mask;
> +
> +	wrmsrl(MSR_MISC_FEATURE_CONTROL, reg);
> +}
> +
> +/*
> + * MSR_MISC_FEATURE_CONTROL has "core" scope, so define the lock to avoid a
> + * conflict of write access from different logical processors in the same core.
> + */
> +static DEFINE_MUTEX(pfctl_mutex);
> +
> +static ssize_t
> +pfctl_store(struct device *pfctl_dev, struct device_attribute *attr,
> +	    const char *buf, size_t size)
> +{
> +	unsigned int cpu = pfctl_dev_get_cpu(pfctl_dev);
> +	struct x86_pfctl_attr *xa;
> +	struct write_info info;
> +
> +	xa = container_of(attr, struct x86_pfctl_attr, attr);
> +	info.mask = xa->mask;
> +
> +	if (strtobool(buf, &info.enable) < 0)
> +		return -EINVAL;
> +
> +	mutex_lock(&pfctl_mutex);
> +	smp_call_function_single(cpu, pfctl_write, &info, true);
> +	mutex_unlock(&pfctl_mutex);
> +
> +	return size;
> +}
> +
> +#define PFCTL_ATTR(_name, _level, _bit)					\
> +	struct x86_pfctl_attr attr_l##_level##_##_name = {		\
> +		.attr = __ATTR(_name, 0600, pfctl_show, pfctl_store),	\
> +		.mask = BIT_ULL(_bit), }
> +
> +static PFCTL_ATTR(hardware_prefetcher_enable,			1, 2);
> +static PFCTL_ATTR(hardware_prefetcher_enable,			2, 0);
> +static PFCTL_ATTR(ip_prefetcher_enable,				1, 3);
> +static PFCTL_ATTR(adjacent_cache_line_prefetcher_enable,	2, 1);
> +
> +static struct device_attribute *l1_attrs[] __ro_after_init = {

How do you know attributes are to be marked read only after init?  Do
other parts of the kernel do that?  I don't think the driver core
guarantees that at all.

thanks,

greg k-h