[PATCH] x86/rapl: Do not load in a guest

[PATCH] x86/rapl: Do not load in a guest

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

qemu/kvm doesn't support RAPL and RAPL doesn't have a CPUID feature bit
so check whether we're in a guest instead.

Reported-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/perf_event_intel_rapl.c | 3 +++
 drivers/powercap/intel_rapl.c               | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/arch/x86/kernel/cpu/perf_event_intel_rapl.c b/arch/x86/kernel/cpu/perf_event_intel_rapl.c
index ed446bdcbf31..bc60bc1118b4 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_rapl.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_rapl.c
@@ -711,6 +711,9 @@ static int __init rapl_pmu_init(void)
 	struct x86_pmu_quirk *quirk;
 	int i;
 
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
+		return 0;
+
 	/*
 	 * check for Intel processor family 6
 	 */
diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c
index cc97f0869791..297a9b5074e2 100644
--- a/drivers/powercap/intel_rapl.c
+++ b/drivers/powercap/intel_rapl.c
@@ -1511,6 +1511,9 @@ static int __init rapl_init(void)
 	int ret = 0;
 	const struct x86_cpu_id *id;
 
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
+		return -ENODEV;
+
 	id = x86_match_cpu(rapl_ids);
 	if (!id) {
 		pr_err("driver does not support CPU family %d model %d\n",
-- 
2.3.5

Re: [PATCH] x86/rapl: Do not load in a guest

[Jacob Pan]

On Thu,  3 Dec 2015 19:27:02 +0100
Borislav Petkov <bp@alien8.de> wrote:

>  
> +	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
> +		return 0;
> +
or use this?
#define cpu_has_hypervisor	boot_cpu_has(X86_FEATURE_HYPERVISOR)

Re: [PATCH] x86/rapl: Do not load in a guest

[Borislav Petkov]

On Thu, Dec 03, 2015 at 10:38:28AM -0800, Jacob Pan wrote:
> or use this?
> #define cpu_has_hypervisor	boot_cpu_has(X86_FEATURE_HYPERVISOR)

No, those are going away:

https://lkml.kernel.org/r/1448982023-19187-4-git-send-email-bp@alien8.de

Next on my TODO is killing the rest of them.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/rapl: Do not load in a guest

[Jacob Pan]

On Thu, 3 Dec 2015 19:42:41 +0100
Borislav Petkov <bp@alien8.de> wrote:

> No, those are going away:
> 
> https://lkml.kernel.org/r/1448982023-19187-4-git-send-email-bp@alien8.de
> 
> Next on my TODO is killing the rest of them.

Fair enough.
Acked-by: Jacob Pan <jacob.jun.pan@linux.intel.com>

Re: [PATCH] x86/rapl: Do not load in a guest

[Rafael J. Wysocki]

On Thursday, December 03, 2015 10:59:33 AM Jacob Pan wrote:
> On Thu, 3 Dec 2015 19:42:41 +0100
> Borislav Petkov <bp@alien8.de> wrote:
> 
> > No, those are going away:
> > 
> > https://lkml.kernel.org/r/1448982023-19187-4-git-send-email-bp@alien8.de
> > 
> > Next on my TODO is killing the rest of them.
> 
> Fair enough.
> Acked-by: Jacob Pan <jacob.jun.pan@linux.intel.com>

OK, so who's suppose to apply this?

Rafael

Re: [PATCH] x86/rapl: Do not load in a guest

[Borislav Petkov]

On Fri, Dec 04, 2015 at 12:32:12AM +0100, Rafael J. Wysocki wrote:
> OK, so who's suppose to apply this?

It doesn't matter to me. I was planning on routing it through tip next
week if you're fine with it or tip guys could ack it too and you could
pick it up. Preference?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/rapl: Do not load in a guest

[Rafael J. Wysocki]

On Friday, December 04, 2015 12:25:34 AM Borislav Petkov wrote:
> On Fri, Dec 04, 2015 at 12:32:12AM +0100, Rafael J. Wysocki wrote:
> > OK, so who's suppose to apply this?
> 
> It doesn't matter to me. I was planning on routing it through tip next
> week if you're fine with it or tip guys could ack it too and you could
> pick it up. Preference?

Nope.  It can go in via tip as far as I'm concerned.

Re: [PATCH] x86/rapl: Do not load in a guest

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> qemu/kvm doesn't support RAPL and RAPL doesn't have a CPUID feature bit
> so check whether we're in a guest instead.

So when a hypervisor starts supporting RAPL we'll disable the driver erroneously?

Isn't there any better method to detect RAPL support?

So in particular in drivers/powercap/intel_rapl.c there's an enumerated list of 
CPU models, which is used via a x86_match_cpu() call. That's still not ideal (it 
does not work on hypervisors for example), but even better would be to detect RAPL 
support in some other fashion, that does not rely on us statically enumerating CPU 
models that support it.

Thanks,

	Ingo

Re: [PATCH] x86/rapl: Do not load in a guest

[Peter Zijlstra]

On Fri, Dec 04, 2015 at 08:42:06AM +0100, Ingo Molnar wrote:
> 
> * Borislav Petkov <bp@alien8.de> wrote:
> 
> > From: Borislav Petkov <bp@suse.de>
> > 
> > qemu/kvm doesn't support RAPL and RAPL doesn't have a CPUID feature bit
> > so check whether we're in a guest instead.
> 
> So when a hypervisor starts supporting RAPL we'll disable the driver erroneously?
> 
> Isn't there any better method to detect RAPL support?
> 
> So in particular in drivers/powercap/intel_rapl.c there's an enumerated list of 
> CPU models, which is used via a x86_match_cpu() call. That's still not ideal (it 
> does not work on hypervisors for example), but even better would be to detect RAPL 
> support in some other fashion, that does not rely on us statically enumerating CPU 
> models that support it.

RAPL isn't enumerated, the best we could do is attempt to write to one
of the writable MSRs and see if that 'works'.

Also, yuck @ powercap/intel_rapl.c for doing rdmsr_on_cpu() +
wrmsr_on_cpu() all over the place.

Re: [PATCH] x86/rapl: Do not load in a guest

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Dec 04, 2015 at 08:42:06AM +0100, Ingo Molnar wrote:
> > 
> > * Borislav Petkov <bp@alien8.de> wrote:
> > 
> > > From: Borislav Petkov <bp@suse.de>
> > > 
> > > qemu/kvm doesn't support RAPL and RAPL doesn't have a CPUID feature bit
> > > so check whether we're in a guest instead.
> > 
> > So when a hypervisor starts supporting RAPL we'll disable the driver erroneously?
> > 
> > Isn't there any better method to detect RAPL support?
> > 
> > So in particular in drivers/powercap/intel_rapl.c there's an enumerated list of 
> > CPU models, which is used via a x86_match_cpu() call. That's still not ideal (it 
> > does not work on hypervisors for example), but even better would be to detect RAPL 
> > support in some other fashion, that does not rely on us statically enumerating CPU 
> > models that support it.
> 
> RAPL isn't enumerated, the best we could do is attempt to write to one
> of the writable MSRs and see if that 'works'.

Hm, bad - writing to MSRs like that is generally dangerous.

So we should at least provide a central 'is RAPL available' call instead of 
spreading multiple X86_FEATURE_HYPERVISOR checks.

Thanks,

	Ingo

Re: [PATCH] x86/rapl: Do not load in a guest

[Borislav Petkov]

+ Paolo.

On Fri, Dec 04, 2015 at 09:28:23AM +0100, Ingo Molnar wrote:
> > > So when a hypervisor starts supporting RAPL we'll disable the driver erroneously?
> > > 
> > > Isn't there any better method to detect RAPL support?
> > > 
> > > So in particular in drivers/powercap/intel_rapl.c there's an enumerated list of 
> > > CPU models, which is used via a x86_match_cpu() call. That's still not ideal (it 
> > > does not work on hypervisors for example), but even better would be to detect RAPL 
> > > support in some other fashion, that does not rely on us statically enumerating CPU 
> > > models that support it.
> > 
> > RAPL isn't enumerated, the best we could do is attempt to write to one
> > of the writable MSRs and see if that 'works'.
> 
> Hm, bad - writing to MSRs like that is generally dangerous.
> 
> So we should at least provide a central 'is RAPL available' call instead of 
> spreading multiple X86_FEATURE_HYPERVISOR checks.

Well, looks like someone dropped the ball at the CPUID registrar. Other
features have more than one CPUID bit allocated to them, this one
doesn't have a single one.

And since there's no CPUID bit, I don't see any other way to detect the
RAPL presence. Poking at MSRs is a bad idea.

I wonder if we could go and allocate a bit in the kvm-emulated CPUID
leafs which says whether RAPL is supported or not.

Then we can go and check for that leaf on baremetal - if it is not
there, we do the vendor + fms check and if it is there, we know we're in
a guest and whether the guest supports it or not.

Dunno.

On the one hand, it looks like a bit too much to me.

On the other, it could be useful for other future feature checks where
we want baremetal and kvm to be synchronized wrt features and a single
method to be used by the kernel for checking features presence works
both on baremetal and virt.

Just a thought, anyway...

hpa, thoughts?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/rapl: Do not load in a guest

[Paolo Bonzini]

On 04/12/2015 11:19, Borislav Petkov wrote:
> + Paolo.
> 
> On Fri, Dec 04, 2015 at 09:28:23AM +0100, Ingo Molnar wrote:
>>>> So when a hypervisor starts supporting RAPL we'll disable the driver erroneously?
>>>>
>>>> Isn't there any better method to detect RAPL support?
>>>>
>>>> So in particular in drivers/powercap/intel_rapl.c there's an enumerated list of 
>>>> CPU models, which is used via a x86_match_cpu() call. That's still not ideal (it 
>>>> does not work on hypervisors for example), but even better would be to detect RAPL 
>>>> support in some other fashion, that does not rely on us statically enumerating CPU 
>>>> models that support it.
>>>
>>> RAPL isn't enumerated, the best we could do is attempt to write to one
>>> of the writable MSRs and see if that 'works'.
>>
>> Hm, bad - writing to MSRs like that is generally dangerous.
>>
>> So we should at least provide a central 'is RAPL available' call instead of 
>> spreading multiple X86_FEATURE_HYPERVISOR checks.
> 
> Well, looks like someone dropped the ball at the CPUID registrar.

Yup, this is an issue with RAPL.

> And since there's no CPUID bit, I don't see any other way to detect the
> RAPL presence. Poking at MSRs is a bad idea.
> 
> I wonder if we could go and allocate a bit in the kvm-emulated CPUID
> leafs which says whether RAPL is supported or not.

No, please don't.  Why do you need a wrmsr instead of a rdmsr?  If
there's no RAPL domains, the device doesn't load.  On hypervisors,
reading random MSRs is generally safe.

Paolo

> Then we can go and check for that leaf on baremetal - if it is not
> there, we do the vendor + fms check and if it is there, we know we're in
> a guest and whether the guest supports it or not.
> 
> Dunno.
> 
> On the one hand, it looks like a bit too much to me.
> 
> On the other, it could be useful for other future feature checks where
> we want baremetal and kvm to be synchronized wrt features and a single
> method to be used by the kernel for checking features presence works
> both on baremetal and virt.
> 
> Just a thought, anyway...
> 
> hpa, thoughts?
> 

Re: [PATCH] x86/rapl: Do not load in a guest

[Borislav Petkov]

On Fri, Dec 04, 2015 at 11:41:03AM +0100, Paolo Bonzini wrote:
> No, please don't.  Why do you need a wrmsr instead of a rdmsr?  If
> there's no RAPL domains, the device doesn't load.  On hypervisors,
> reading random MSRs is generally safe.

Well, we could not do anything, sure, that's an option too. It would
only be the annoying error message. Which is

	pr_err("no valid rapl domains found in package %d\n", rp->id);

I guess we can tone that down as apparently it is not an error to
not have valid rapl domains anymore. Maybe kill it altogether:
rapl_detect_topology() will propagate the error and the driver won't
load...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/rapl: Do not load in a guest

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Dec 04, 2015 at 11:41:03AM +0100, Paolo Bonzini wrote:
> > No, please don't.  Why do you need a wrmsr instead of a rdmsr?  If
> > there's no RAPL domains, the device doesn't load.  On hypervisors,
> > reading random MSRs is generally safe.
> 
> Well, we could not do anything, sure, that's an option too. It would
> only be the annoying error message. Which is
> 
> 	pr_err("no valid rapl domains found in package %d\n", rp->id);
> 
> I guess we can tone that down as apparently it is not an error to
> not have valid rapl domains anymore. Maybe kill it altogether:
> rapl_detect_topology() will propagate the error and the driver won't
> load...

So given than nothing really tells us in a clear way whether RAPL is supported or 
not on that kernel, it might be better to just centralize the 'detect RAPL' 
function, and print "x86/rapl: Feature detected" on bootup. That function can also 
install a synthetic CPUID bit, which all other code could use in a clean fashion.

Since it will be an __init function, there's not much of an overhead argument 
against it.

This way it becomes part of the CPUID infrastructure - and eventually it might 
even grow a real CPUID bit in future CPU models.

and we'll have a lot less RAPL detection muck all around. Win-win.

Thanks,

	Ingo

Re: [PATCH] x86/rapl: Do not load in a guest

[Jacob Pan]

On Fri, 4 Dec 2015 12:53:35 +0100
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Borislav Petkov <bp@alien8.de> wrote:
> 
> > On Fri, Dec 04, 2015 at 11:41:03AM +0100, Paolo Bonzini wrote:
> > > No, please don't.  Why do you need a wrmsr instead of a rdmsr?  If
> > > there's no RAPL domains, the device doesn't load.  On hypervisors,
> > > reading random MSRs is generally safe.
> > 
> > Well, we could not do anything, sure, that's an option too. It would
> > only be the annoying error message. Which is
> > 
> > 	pr_err("no valid rapl domains found in package %d\n",
> > rp->id);
> > 
> > I guess we can tone that down as apparently it is not an error to
> > not have valid rapl domains anymore. Maybe kill it altogether:
> > rapl_detect_topology() will propagate the error and the driver won't
> > load...
> 
Since RAPL is not architectural, consistency of hw support needs lots
of improvement at the least. This error message is valid in other than
VM. Domain detection error already propagated.

> So given than nothing really tells us in a clear way whether RAPL is
> supported or not on that kernel, it might be better to just
> centralize the 'detect RAPL' function, and print "x86/rapl: Feature
> detected" on bootup. That function can also install a synthetic CPUID
> bit, which all other code could use in a clean fashion.
> 
> Since it will be an __init function, there's not much of an overhead
> argument against it.
> 
This is good for the first level RAPL detection. The only way is to
base detection on known CPU models.

But I still think hypervisor check is sufficient. I don't there will
ever be a use case for VM to control platform level power. A disaster
for sure.

> This way it becomes part of the CPUID infrastructure - and eventually
> it might even grow a real CPUID bit in future CPU models.
> 
> and we'll have a lot less RAPL detection muck all around. Win-win.
> 
True, RAPL is not architectural today but it is supported by all Intel
CPUs since SNB.
> Thanks,
> 
> 	Ingo

Re: [PATCH] x86/rapl: Do not load in a guest

[Paolo Bonzini]

On 04/12/2015 18:46, Jacob Pan wrote:
> But I still think hypervisor check is sufficient. I don't there will
> ever be a use case for VM to control platform level power. A disaster
> for sure.

There isn't just usual VMs.  There are partitioning hypervisors for example.

Paolo

Re: [PATCH] x86/rapl: Do not load in a guest

[Borislav Petkov]

On Fri, Dec 04, 2015 at 09:46:42AM -0800, Jacob Pan wrote:
> Since RAPL is not architectural, consistency of hw support needs lots
> of improvement at the least.

It should have received a CPUID bit, no matter if it is architectural or
not.

> This error message is valid in other than VM. Domain detection error
> already propagated.

Huh, didn't I say that?

> This is good for the first level RAPL detection. The only way is to
> base detection on known CPU models.

...and yet Hannes sees it on a minnowboard. CCed.

The CPU model-based detection is ugly and does not always work, as
you see.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/rapl: Do not load in a guest

[Jacob Pan]

On Fri, 4 Dec 2015 19:04:26 +0100
Borislav Petkov <bp@alien8.de> wrote:

> > This is good for the first level RAPL detection. The only way is to
> > base detection on known CPU models.  
> 
> ...and yet Hannes sees it on a minnowboard. CCed.
> 
> The CPU model-based detection is ugly and does not always work, as
> you see.
CPU model detection is the first level checking. The error is about no
valid domains (e.g. counters not working). So the error on minnowboard
board could be a real problem if you expect to use RAPL.

Jacob

Re: [PATCH] x86/rapl: Do not load in a guest

[Borislav Petkov]

On Fri, Dec 04, 2015 at 10:16:10AM -0800, Jacob Pan wrote:
> CPU model detection is the first level checking.

And in the case of RAPL, the only checking you can do. This is why it
should've had a CPUID bit.

> The error is about no valid domains (e.g. counters not working). So
> the error on minnowboard board could be a real problem if you expect
> to use RAPL.

Right, and if you need to disable it there, you would need to add a
quirk table looking at DMI strings or so. A CPUID bit might've been a
bit better if BIOS update would clear it on those boards. Then, sw won't
even try to load there.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/rapl: Do not load in a guest

[Jacob Pan]

On Fri, 4 Dec 2015 19:28:07 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Dec 04, 2015 at 10:16:10AM -0800, Jacob Pan wrote:
> > CPU model detection is the first level checking.  
> 
> And in the case of RAPL, the only checking you can do. This is why it
> should've had a CPUID bit.
> 
I am 200% with you, all I can say is we are working on it. Look at the
pain in the driver for dealing with various quirks.

> > The error is about no valid domains (e.g. counters not working). So
> > the error on minnowboard board could be a real problem if you expect
> > to use RAPL.  
> 
> Right, and if you need to disable it there, you would need to add a
> quirk table looking at DMI strings or so. A CPUID bit might've been a
> bit better if BIOS update would clear it on those boards. Then, sw
> won't even try to load there.
who is gonna collect all the DMI strings? I don't think this is
scalable.

Re: [PATCH] x86/rapl: Do not load in a guest

[Borislav Petkov]

On Fri, Dec 04, 2015 at 10:37:31AM -0800, Jacob Pan wrote:
> who is gonna collect all the DMI strings? I don't think this is
> scalable.

Let's hope only a small number of platforms is affected.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86/rapl: Do not load in a guest

[Jacob Pan]

On Fri, 4 Dec 2015 09:22:56 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> Also, yuck @ powercap/intel_rapl.c for doing rdmsr_on_cpu() +
> wrmsr_on_cpu() all over the place.
Can you please be more specific? is the concern related to the
overhead of IPI? I am doing these calls based on MSR CPU scope and
consider the fact that access is less frequent.

Thanks,

Jacob

Re: [PATCH] x86/rapl: Do not load in a guest

[Peter Zijlstra]

On Fri, Dec 04, 2015 at 09:51:02AM -0800, Jacob Pan wrote:
> On Fri, 4 Dec 2015 09:22:56 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > Also, yuck @ powercap/intel_rapl.c for doing rdmsr_on_cpu() +
> > wrmsr_on_cpu() all over the place.
> Can you please be more specific? is the concern related to the
> overhead of IPI? I am doing these calls based on MSR CPU scope and
> consider the fact that access is less frequent.

Yeah, its just offensive to do an IPI to read a value, then twiddle a
few bits on the value and then IPI again to store the value.

I know its low freq, and that MSR access is slow, but *groan*.

Re: [PATCH] x86/rapl: Do not load in a guest

[H. Peter Anvin]

On December 4, 2015 2:14:46 PM PST, Peter Zijlstra <peterz@infradead.org> wrote:
>On Fri, Dec 04, 2015 at 09:51:02AM -0800, Jacob Pan wrote:
>> On Fri, 4 Dec 2015 09:22:56 +0100
>> Peter Zijlstra <peterz@infradead.org> wrote:
>> 
>> > Also, yuck @ powercap/intel_rapl.c for doing rdmsr_on_cpu() +
>> > wrmsr_on_cpu() all over the place.
>> Can you please be more specific? is the concern related to the
>> overhead of IPI? I am doing these calls based on MSR CPU scope and
>> consider the fact that access is less frequent.
>
>Yeah, its just offensive to do an IPI to read a value, then twiddle a
>few bits on the value and then IPI again to store the value.
>
>I know its low freq, and that MSR access is slow, but *groan*.

Yes, for that it would be better to invoke a common routine to do all the accesses on the target CPU.  MSR accesses may be slow, but IPIs are way slower.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.