linux-kernel - Re: [PATCH] powercap / RAPL : remove dependency on iosf

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 for Android: free password hash cracker in your pocket

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

Date:	Tue, 22 Sep 2015 10:01:49 -0700
From:	Jacob Pan <jacob.jun.pan@...ux.intel.com>
To:	Pengyu Ma <pengyu.ma@...driver.com>
Cc:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	<rafael.j.wysocki@...el.com>, <linux-kernel@...r.kernel.org>,
	"Box, David E" <david.e.box@...el.com>,
	"Anvin, H Peter" <h.peter.anvin@...el.com>
Subject: Re: [PATCH] powercap / RAPL : remove dependency on iosf_mbi

On Tue, 22 Sep 2015 11:11:36 +0800
Pengyu Ma <pengyu.ma@...driver.com> wrote:

> 
> 
> On 09/22/2015 05:36 AM, Jacob Pan wrote:
> > On Mon, 21 Sep 2015 11:48:14 +0800
> > Pengyu Ma <pengyu.ma@...driver.com> wrote:
> >
> >>
> >> On 09/18/2015 11:43 PM, Jacob Pan wrote:
> >>> On Fri, 18 Sep 2015 02:09:55 +0200
> >>> "Rafael J. Wysocki" <rjw@...ysocki.net> wrote:
> >>>
> >>>> On Thursday, September 17, 2015 03:31:41 PM Pengyu Ma wrote:
> >>>>> iosf_mbi is supported on Quark, Braswell, Baytrail and some Atom
> >>>>> SoC, but RAPL is not limited to these SoC, it supports almost
> >>>>> Intel CPUs. Remove this dependece to make RAPL support more
> >>>>> Intel CPUs.
> >>>>>
> >>>>> Please select IOSF_MBI on Atom SoCs.
> >>>>>
> >>> Unlike Quark, I don't think we want to or do differentiate Atom
> >>> from other x86 at compile time. IOSF driver can be compiled as a
> >>> module also, therefore RAPL driver needs this explicit dependency
> >>> at compile time.
> >> As commit had exported iosf_mbi to let user use it.
> >>
> >> commit aa8e4f22ab7773352ba3895597189b8097f2c307
> >> Author: David E. Box <david.e.box@...ux.intel.com>
> >> Date:   Wed Aug 27 14:40:39 2014 -0700
> >>
> >>       x86/iosf: Add Kconfig prompt for IOSF_MBI selection
> >>
> >>
> >> While selecting IOSF_MBI is preferred, it does mean carrying extra
> >> code on non-SoC architectures.
> >>
> >> We can NOT force user to build in iosf_mbi if they want use RAPL on
> >> haswell/broadwell/skylake.
> >> And RAPL can be compiled and worked well on
> >> haswell/broadwell/skylake without IOSF_MBI.
> >> RAPL is really NOT depended on IOSF_MBI.
> >>
> > True for haswell/broadwell/skylake platforms. But if we want binary
> > compatibility for Atom and Core, I can' see how simply removing the
> > dependency would work, unless we have runtime detection of IOSF.
> If you want use iosf_mbi on atom, please select it on generic x86
> config. But not force it depend on another feature that not related
> on it with other boards.
> I don't care how iosf_mbi is added to kernel config, but why should I
> be forced to add it if I want use RAPL?
> It doesn't make any sense.
> 
I understand your concern about wasting code. But let's look at all the
cases of config options here. (without Kconfig dependency as you
suggested)

RAPL\IOSF	Y	M	N
___________________________________________________
 Y		OK      DC*     Warn on Atom**
 M		OK	OK	Warn on Atom
 N		OK	OK	OK
___________________________________________________

Notes:
* DC: don't compile
** Warn on Atom is runtime if I add the following code to RAPL driver,
but this case is ok.

--- a/drivers/powercap/intel_rapl.c
+++ b/drivers/powercap/intel_rapl.c
@@ -982,6 +982,11 @@ static void set_floor_freq_atom(struct rapl_domain
*rd, bool enable) static u32 power_ctrl_orig_val;
        u32 mdata;
 
+       if (!iosf_mbi_available()) {
+               pr_warn("No IOSF MBI access to set floor frequency\n");
+               return;
+       }
+

So the problematic case is when RAPL=Y IOSF=M
Since real IOSF functions are available when
#if IS_ENABLED(CONFIG_IOSF_MBI)
There will be no dummy functions for RAPL to reference in this case.

Since IOSF is a driver, making it a module is a reasonable requirement.
As I mentioned before, I don't think we want to have a CONFIG_ATOM
option for X86.

+David, HPA

Jacob

> Pengyu
> 
> >
> >> Pengyu
> >>>>> Signed-off-by: Pengyu Ma <pengyu.ma@...driver.com>
> >>>> Jacob?
> >>>>
> >>>>> ---
> >>>>>    drivers/powercap/Kconfig | 2 +-
> >>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
> >>>>> index 85727ef..a7c81b5 100644
> >>>>> --- a/drivers/powercap/Kconfig
> >>>>> +++ b/drivers/powercap/Kconfig
> >>>>> @@ -17,7 +17,7 @@ if POWERCAP
> >>>>>    # Client driver configurations go here.
> >>>>>    config INTEL_RAPL
> >>>>>    	tristate "Intel RAPL Support"
> >>>>> -	depends on X86 && IOSF_MBI
> >>>>> +	depends on X86
> >>>>>    	default n
> >>>>>    	---help---
> >>>>>    	  This enables support for the Intel Running Average
> >>>>> Power Limit (RAPL)
> >>>>>
> >>> [Jacob Pan]
> > [Jacob Pan]
> 

[Jacob Pan]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/