[09/11] powerpc/64s: Allow control of RFI flush via sysfs

Message ID 20180108165453.26066-9-mpe@ellerman.id.au
State Rejected
Headers show
Series
  • [01/11] powerpc/pseries: Add H_GET_CPU_CHARACTERISTICS flags & wrapper
Related show

Commit Message

Michael Ellerman Jan. 8, 2018, 4:54 p.m.
From: Nicholas Piggin <npiggin@gmail.com>

Expose the state of the RFI flush (enabled/disabled) via sysfs, and
allow it to be enabled/dissabled at runtime.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/setup.h |  2 ++
 arch/powerpc/kernel/sysfs.c | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

Comments

Thomas Gleixner Jan. 8, 2018, 5:20 p.m. | #1
On Tue, 9 Jan 2018, Michael Ellerman wrote:

Sorry, I wasn't aware about your efforts and did not cc you. I've just
queued a more generic sysfs interface for this whole mess:

https://lkml.kernel.org/r/20180107214913.096657732@linutronix.de

It should be simple to extend for write and it would be great if all
affected architectures could share it.

Thanks,

	tglx
Michael Ellerman Jan. 8, 2018, 10:09 p.m. | #2
Thomas Gleixner <tglx@linutronix.de> writes:

> On Tue, 9 Jan 2018, Michael Ellerman wrote:
>
> Sorry, I wasn't aware about your efforts and did not cc you. I've just
> queued a more generic sysfs interface for this whole mess:

No worries.

> https://lkml.kernel.org/r/20180107214913.096657732@linutronix.de
>
> It should be simple to extend for write and it would be great if all
> affected architectures could share it.

As you say this has all been a bit of a mess, and as a result we already
have people running kernels with this patch, so we don't want to remove
the 'rfi_flush' file.

But we will certainly add support on powerpc for the files you have
created, in addition to 'rfi_flush'.

cheers
Jon Masters Jan. 9, 2018, 6:06 a.m. | #3
On 01/08/2018 05:09 PM, Michael Ellerman wrote:
> Thomas Gleixner <tglx@linutronix.de> writes:
> 
>> On Tue, 9 Jan 2018, Michael Ellerman wrote:
>>
>> Sorry, I wasn't aware about your efforts and did not cc you. I've just
>> queued a more generic sysfs interface for this whole mess:
> 
> No worries.
> 
>> https://lkml.kernel.org/r/20180107214913.096657732@linutronix.de
>>
>> It should be simple to extend for write and it would be great if all
>> affected architectures could share it.
> 
> As you say this has all been a bit of a mess

Indeed. All of us wish this went very differently.

I've been testing various versions of these patches since before the
holidays. For those doing backports to older kernels, a note that the
IBM team added OOL (Out Of Line) exception handlers and reworked all of
that code over the years since older kernels (e.g. 3.10) so you might
get problems on those if you enable the debug entry. I've got notes on
how to backport the OOL exceptions to older kernels if anyone cares.

> and as a result we already have people running kernels with this patch,
> so we don't want to remove the 'rfi_flush' file.

Knowing that the IBM team was going to post with this sysfs interface,
our trees contain the rfi_flush file. I mentioned it to some folks on
this end (because we know we don't want to add things in sysfs
generally, debugfs is a good substitute, per Andrea, and I raised this
with him yesterday as a concern in the backport here) but in the end it
seemed reasonable to pull this in because it was what got posted, and as
Michael says, it's gone into other distro kernels beyond just ours.

> But we will certainly add support on powerpc for the files you have
> created, in addition to 'rfi_flush'.

Thanks,

Jon.
Greg KH Jan. 9, 2018, 8:03 a.m. | #4
On Tue, Jan 09, 2018 at 03:54:51AM +1100, Michael Ellerman wrote:
> From: Nicholas Piggin <npiggin@gmail.com>
> 
> Expose the state of the RFI flush (enabled/disabled) via sysfs, and
> allow it to be enabled/dissabled at runtime.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/kernel/setup.h |  2 ++
>  arch/powerpc/kernel/sysfs.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)

You forgot a Documentation/ABI/ update for a new sysfs file :(

> diff --git a/arch/powerpc/kernel/setup.h b/arch/powerpc/kernel/setup.h
> index 21c18071d9d5..493b03b0a966 100644
> --- a/arch/powerpc/kernel/setup.h
> +++ b/arch/powerpc/kernel/setup.h
> @@ -61,4 +61,6 @@ void kvm_cma_reserve(void);
>  static inline void kvm_cma_reserve(void) { };
>  #endif
>  
> +extern bool rfi_flush;
> +
>  #endif /* __ARCH_POWERPC_KERNEL_SETUP_H */
> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
> index b8d4a1dac39f..8c19d014cffc 100644
> --- a/arch/powerpc/kernel/sysfs.c
> +++ b/arch/powerpc/kernel/sysfs.c
> @@ -20,6 +20,7 @@
>  #include <asm/firmware.h>
>  
>  #include "cacheinfo.h"
> +#include "setup.h"
>  
>  #ifdef CONFIG_PPC64
>  #include <asm/paca.h>
> @@ -496,6 +497,43 @@ static DEVICE_ATTR(spurr, 0400, show_spurr, NULL);
>  static DEVICE_ATTR(purr, 0400, show_purr, store_purr);
>  static DEVICE_ATTR(pir, 0400, show_pir, NULL);
>  
> +#ifdef CONFIG_PPC_BOOK3S_64
> +static ssize_t show_rfi_flush(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%d\n", rfi_flush ? 1 : 0);
> +}
> +
> +static ssize_t __used store_rfi_flush(struct device *dev,
> +		struct device_attribute *attr, const char *buf,
> +		size_t count)
> +{
> +	int val;
> +	int ret = 0;
> +
> +	ret = sscanf(buf, "%d", &val);
> +	if (ret != 1)
> +		return -EINVAL;
> +
> +	if (val == 1)
> +		rfi_flush_enable(true);
> +	else if (val == 0)
> +		rfi_flush_enable(false);
> +	else
> +		return -EINVAL;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(rfi_flush, 0600,
> +		show_rfi_flush, store_rfi_flush);

DEVICE_ATTR_RW()?  And why 0600?  That's odd.

> +
> +static void sysfs_create_rfi_flush(void)
> +{
> +	device_create_file(cpu_subsys.dev_root, &dev_attr_rfi_flush);

No error checking?

And as Thomas said, why not just use the generic infrastructure he
created instead?  That way there is some form of unity here for the same
exact issue.

At least he documented the api :)

thanks,

greg k-h
Greg Kroah-Hartman Jan. 9, 2018, 8:05 a.m. | #5
On Tue, Jan 09, 2018 at 01:06:23AM -0500, Jon Masters wrote:
> Knowing that the IBM team was going to post with this sysfs interface,
> our trees contain the rfi_flush file. I mentioned it to some folks on
> this end (because we know we don't want to add things in sysfs
> generally, debugfs is a good substitute, per Andrea, and I raised this
> with him yesterday as a concern in the backport here) but in the end it
> seemed reasonable to pull this in because it was what got posted, and as
> Michael says, it's gone into other distro kernels beyond just ours.

What distro kernels end up enabling does not really reflect on what we
end up doing in mainline.  The api for this should NOT be arch-specific
if at all possible, that way lies madness.  Do you want to write
userspace tools to handle the 60+ different arch implementations?

Don't let the fragmentation problems of the period in which no one was
allowed to talk to each other, result in a unchangable mess, that would
be insane.

thanks,

greg k-h
Jon Masters Jan. 9, 2018, 8:11 a.m. | #6
On 01/09/2018 03:05 AM, Greg KH wrote:
> On Tue, Jan 09, 2018 at 01:06:23AM -0500, Jon Masters wrote:
>> Knowing that the IBM team was going to post with this sysfs interface,
>> our trees contain the rfi_flush file. I mentioned it to some folks on
>> this end (because we know we don't want to add things in sysfs
>> generally, debugfs is a good substitute, per Andrea, and I raised this
>> with him yesterday as a concern in the backport here) but in the end it
>> seemed reasonable to pull this in because it was what got posted, and as
>> Michael says, it's gone into other distro kernels beyond just ours.
> 
> What distro kernels end up enabling does not really reflect on what we
> end up doing in mainline.  The api for this should NOT be arch-specific
> if at all possible, that way lies madness.  Do you want to write
> userspace tools to handle the 60+ different arch implementations?
> 
> Don't let the fragmentation problems of the period in which no one was
> allowed to talk to each other, result in a unchangable mess, that would
> be insane.

Totally fine :) Just saying we tried to do reasonable things with what
we had. Whatever happens upstream in the end is, of course, what we'll
make sure fits into updates that go into the likes of RHEL.

Jon.

Patch

diff --git a/arch/powerpc/kernel/setup.h b/arch/powerpc/kernel/setup.h
index 21c18071d9d5..493b03b0a966 100644
--- a/arch/powerpc/kernel/setup.h
+++ b/arch/powerpc/kernel/setup.h
@@ -61,4 +61,6 @@  void kvm_cma_reserve(void);
 static inline void kvm_cma_reserve(void) { };
 #endif
 
+extern bool rfi_flush;
+
 #endif /* __ARCH_POWERPC_KERNEL_SETUP_H */
diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index b8d4a1dac39f..8c19d014cffc 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -20,6 +20,7 @@ 
 #include <asm/firmware.h>
 
 #include "cacheinfo.h"
+#include "setup.h"
 
 #ifdef CONFIG_PPC64
 #include <asm/paca.h>
@@ -496,6 +497,43 @@  static DEVICE_ATTR(spurr, 0400, show_spurr, NULL);
 static DEVICE_ATTR(purr, 0400, show_purr, store_purr);
 static DEVICE_ATTR(pir, 0400, show_pir, NULL);
 
+#ifdef CONFIG_PPC_BOOK3S_64
+static ssize_t show_rfi_flush(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", rfi_flush ? 1 : 0);
+}
+
+static ssize_t __used store_rfi_flush(struct device *dev,
+		struct device_attribute *attr, const char *buf,
+		size_t count)
+{
+	int val;
+	int ret = 0;
+
+	ret = sscanf(buf, "%d", &val);
+	if (ret != 1)
+		return -EINVAL;
+
+	if (val == 1)
+		rfi_flush_enable(true);
+	else if (val == 0)
+		rfi_flush_enable(false);
+	else
+		return -EINVAL;
+
+	return count;
+}
+
+static DEVICE_ATTR(rfi_flush, 0600,
+		show_rfi_flush, store_rfi_flush);
+
+static void sysfs_create_rfi_flush(void)
+{
+	device_create_file(cpu_subsys.dev_root, &dev_attr_rfi_flush);
+}
+#endif /* CONFIG_PPC_BOOK3S_64 */
+
 /*
  * This is the system wide DSCR register default value. Any
  * change to this default value through the sysfs interface
@@ -1047,6 +1085,9 @@  static int __init topology_init(void)
 	WARN_ON(r < 0);
 #ifdef CONFIG_PPC64
 	sysfs_create_dscr_default();
+#ifdef CONFIG_PPC_BOOK3S
+	sysfs_create_rfi_flush();
+#endif
 #endif /* CONFIG_PPC64 */
 
 	return 0;