KVM: x86/pmu: Add '.exclude_hv = 1' for guest perf_event

PATCH In review Like Xu 4 participants 9 messages Aug 12, 2020

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Like Xu

author

2020-08-12 05:09 UTC #0 lore raw

PATCH 1/1 KVM: x86/pmu: Add '.exclude_hv = 1' for guest perf_event

To emulate PMC counter for guest, KVM would create an event on the host with 'exclude_guest=0, exclude_hv=0' which simply makes no sense and is utterly broken.

To keep perf semantics consistent, any event created by pmc_reprogram_counter() should both set exclude_hv and exclude_host in the KVM context.

Message-ID: <[email protected]>

Signed-off-by Like Xu <[email protected]>

---
 arch/x86/kvm/pmu.c | 1 +
 1 file changed, 1 insertion(+)

arch/x86/kvm/pmu.c +1 -0

index 67741d2a0308..6a30763a10d7 100644
@@ -108,6 +108,7 @@ static void pmc_reprogram_counter(struct kvm_pmc *pmc, u32 type,
 		.exclude_host = 1,
 		.exclude_user = exclude_user,
 		.exclude_kernel = exclude_kernel,
+		.exclude_hv = 1,
 		.config = config,
 	};

Signature · 1 line

2.21.3

Paolo Bonzini

maintainer

2020-08-12 10:25 UTC #1 lore raw

Quoted 20 lines · Like Xu

To emulate PMC counter for guest, KVM would create an event on the host with 'exclude_guest=0, exclude_hv=0' which simply makes no sense and is utterly broken.
To keep perf semantics consistent, any event created by pmc_reprogram_counter() should both set exclude_hv and exclude_host in the KVM context.
Message-ID: <[email protected]>
Signed-off-by Like Xu <[email protected]>
---
 arch/x86/kvm/pmu.c | 1 +
 1 file changed, 1 insertion(+)
arch/x86/kvm/pmu.c +0 -0
index 67741d2a0308..6a30763a10d7 100644

arch/x86/kvm/pmu.c static void pmc_reprogram_counter(struct kvm_pmc *pmc, u32 type, · line 114

Quoted 7 lines

arch/x86/kvm/pmu.c +1 -0

@@ -108,6 +108,7 @@ static void pmc_reprogram_counter(struct kvm_pmc *pmc, u32 type,
 		.exclude_host = 1,
 		.exclude_user = exclude_user,
 		.exclude_kernel = exclude_kernel,
+		.exclude_hv = 1,
 		.config = config,
 	};

x86 does not have a hypervisor privilege level, so it never uses exclude_hv; exclude_host already excludes all root mode activity for both ring0 and ring3.

Paolo

[email protected]

2020-08-12 11:11 UTC #2 lore raw

Quoted 15 lines · Paolo Bonzini

Quoted 14 lines · Like Xu
To emulate PMC counter for guest, KVM would create an event on the host with 'exclude_guest=0, exclude_hv=0' which simply makes no sense and is utterly broken.
To keep perf semantics consistent, any event created by pmc_reprogram_counter() should both set exclude_hv and exclude_host in the KVM context.
Message-ID: <[email protected]>
Signed-off-by Like Xu <[email protected]>
---
 arch/x86/kvm/pmu.c | 1 +
 1 file changed, 1 insertion(+)

Quoted 15 lines

arch/x86/kvm/pmu.c static void pmc_reprogram_counter(struct kvm_pmc *pmc, u32 type, · line 114
Quoted 13 lines
arch/x86/kvm/pmu.c +1 -0
index 67741d2a0308..6a30763a10d7 100644
@@ -108,6 +108,7 @@ static void pmc_reprogram_counter(struct kvm_pmc *pmc, u32 type,
 		.exclude_host = 1,
 		.exclude_user = exclude_user,
 		.exclude_kernel = exclude_kernel,
+		.exclude_hv = 1,
 		.config = config,
 	};
 
x86 does not have a hypervisor privilege level, so it never uses

Arguably it does when Xen, but I don't think we support that, so *phew*.

Quoted 2 lines

exclude_hv; exclude_host already excludes all root mode activity for both ring0 and ring3.

Right, but we want to tighten the permission checks and not excluding_hv is just sloppy.

That said; the exclude_host / exclude_guest thing is a giant trainwreck and I'm really not sure what to do about it.

The thing is, we very much do not want to allow unpriv user to be able to create: exclude_host=1, exclude_guest=0 counters (they currently can).

So we really want to add:

	if ((!exclude_host || !exclude_guest || !exclude_hv) && !perf_allow_kernel())
		return -EACCESS;

But the problem is, they were added late, so lots of userspace will not be setting those fields (or might not have even known about them), so we got to somehow deal with:

	exclude_host == exclude_guest == 0

And because of that, we now also have genius code like (intel_set_masks, amd_core_hw_config has even 'funnier' code):

	if (event->attr.exclude_host)
		__set_bit(idx, (unsigned long *)&cpuc->intel_ctrl_guest_mask);
	if (event->attr.exclude_guest)
		__set_bit(idx, (unsigned long *)&cpuc->intel_ctrl_host_mask);

Which is just confusing and bad, that really should've been:

	if (!event->attr.exclude_host)
		__set_bit(idx, (unsigned long *)&cpuc->intel_ctrl_host_mask);
	if (!event->attr.exclude_guest)
		__set_bit(idx, (unsigned long *)&cpuc->intel_ctrl_guest_mask);

:-(

Also, exclude_host is really poorly defined:

https://lkml.kernel.org/r/[email protected]

"Suppose we have nested virt:

	  L0-hv
	  |
	  G0/L1-hv
	     |
	     G1

And we're running in G0, then:

- 'exclude_hv' would exclude L0 events
- 'exclude_host' would ... exclude L1-hv events?
- 'exclude_guest' would ... exclude G1 events?

Then the next question is, if G0 is a host, does the L1-hv run in G0 userspace or G0 kernel space?

I was assuming G0 userspace would not include anything L1 (kvm is a kernel module after all), but what do I know."

The way it is implemented, you basically have to always set exclude_host=0, even if there is no virt at all and you want to measure your own userspace thing -- which is just weird.

Meanwhile ARM64 couldn't quite figure out what it was all supposed to be either and also implemented something -- and i've not tried to understand what exactly, but hopefully compatible enough that we're not in an even worse corner.

So on the one hand we're now leaking all sorts due to lack of permission checks, on the other hand we can't fix it because we're not allowed to break userspace :-(

I suppose the 'best' option at this point is something like:

	/*
	 * comment that explains the trainwreck.
	 */
	if (!exclude_host && !exclude_guest)
		exclude_guest = 1;

	if ((!exclude_hv || !exclude_guest) && !perf_allow_kernel())
		return -EPERM;

But that takes away the possibility of actually having:
'exclude_host=0, exclude_guest=0' to create an event that measures both, which also sucks.

Paolo Bonzini

maintainer

2020-08-12 11:33 UTC #3 lore raw

Quoted 3 lines · On 12/08/20 13:11, [email protected] wrote:

Quoted 1 line
x86 does not have a hypervisor privilege level, so it never uses
Arguably it does when Xen, but I don't think we support that, so *phew*.

Yeah, I suppose you could imagine having paravirtualized perf counters where the Xen privileged domain could ask Xen to run perf counters on itself.

Quoted 5 lines

Quoted 2 lines
exclude_hv; exclude_host already excludes all root mode activity for both ring0 and ring3.
Right, but we want to tighten the permission checks and not excluding_hv is just sloppy.

I would just document that it's ignored as it doesn't make sense. ARM64 does that too, for new processors where the kernel is not itself split between supervisor and hypervisor privilege levels.

There are people that are trying to run Linux-based firmware and have SMM handlers as part of the kernel. Perhaps they could use exclude_hv to exclude the SMM handlers from perf (if including them is possible at all).

Quoted 3 lines

The thing is, we very much do not want to allow unpriv user to be able to create: exclude_host=1, exclude_guest=0 counters (they currently can).

That would be the case of an unprivileged user that wants to measure performance of its guests. It's a scenario that makes a lot of sense, are you worried about side channels? Can perf-events on guests leak more about the host than perf-events on a random userspace program?

Quoted 12 lines

Also, exclude_host is really poorly defined:
https://lkml.kernel.org/r/[email protected]
"Suppose we have nested virt:
	  L0-hv
	  |
	  G0/L1-hv
	     |
	     G1

Quoted 5 lines

And we're running in G0, then:
- 'exclude_hv' would exclude L0 events
- 'exclude_host' would ... exclude L1-hv events?
- 'exclude_guest' would ... exclude G1 events?

From the point of view of G0, L0 *does not exist at all*. You just cannot see L0 events if you're running in G0.

exclude_host/exclude_guest are the right definition.

Quoted 2 lines

Then the next question is, if G0 is a host, does the L1-hv run in G0 userspace or G0 kernel space?

It's mostly kernel, but sometimes you're interested in events from QEMU or whoever else has opened /dev/kvm. In that case you care about G0 userspace too.

Quoted 3 lines

The way it is implemented, you basically have to always set exclude_host=0, even if there is no virt at all and you want to measure your own userspace thing -- which is just weird.

I understand regretting having exclude_guest that way; include_guest (defaulting to 0!) would have made more sense. But defaulting to exclude_host==0 makes sense: if there is no virt at all, memset(0) does the right thing so it does not seem weird to me.

Quoted 11 lines

I suppose the 'best' option at this point is something like:

	/*
	 * comment that explains the trainwreck.
	 */
	if (!exclude_host && !exclude_guest)
		exclude_guest = 1;

	if ((!exclude_hv || !exclude_guest) && !perf_allow_kernel())
		return -EPERM;

Quoted 3 lines

But that takes away the possibility of actually having:
'exclude_host=0, exclude_guest=0' to create an event that measures both, which also sucks.

In fact both of the above "if"s suck. :(

Paolo

Xu, Like

2020-08-12 12:56 UTC #4 lore raw

Quoted 11 lines · Paolo Bonzini

Quoted 2 lines · On 12/08/20 13:11, [email protected] wrote:
Quoted 1 line
x86 does not have a hypervisor privilege level, so it never uses
Arguably it does when Xen, but I don't think we support that, so *phew*.
Yeah, I suppose you could imagine having paravirtualized perf counters where the Xen privileged domain could ask Xen to run perf counters on itself.
Quoted 4 lines
Quoted 2 lines
exclude_hv; exclude_host already excludes all root mode activity for both ring0 and ring3.
Right, but we want to tighten the permission checks and not excluding_hv is just sloppy.

Quoted 8 lines

I would just document that it's ignored as it doesn't make sense. ARM64 does that too, for new processors where the kernel is not itself split between supervisor and hypervisor privilege levels.
There are people that are trying to run Linux-based firmware and have SMM handlers as part of the kernel. Perhaps they could use exclude_hv to exclude the SMM handlers from perf (if including them is possible at all).

Hi Paolo,

My proposal is to define:
the "hypervisor privilege levels" events in the KVM/x86 context as all the host kernel events plus /dev/kvm user space events.

If we add ".exclude_hv = 1" in the pmc_reprogram_counter(), do you see any side effect to cover the above usages?

The fact that exclude_hv has never been used in x86 does help the generic perf code to handle permission checks in a more concise way.

Thanks,
Like Xu

Quoted 62 lines

Quoted 3 lines
The thing is, we very much do not want to allow unpriv user to be able to create: exclude_host=1, exclude_guest=0 counters (they currently can).
That would be the case of an unprivileged user that wants to measure performance of its guests. It's a scenario that makes a lot of sense, are you worried about side channels? Can perf-events on guests leak more about the host than perf-events on a random userspace program?
Quoted 17 lines
Also, exclude_host is really poorly defined:
   https://lkml.kernel.org/r/[email protected]

   "Suppose we have nested virt:

	  L0-hv
	  |
	  G0/L1-hv
	     |
	     G1

   And we're running in G0, then:

   - 'exclude_hv' would exclude L0 events
   - 'exclude_host' would ... exclude L1-hv events?
   - 'exclude_guest' would ... exclude G1 events?
From the point of view of G0, L0 *does not exist at all*. You just
cannot see L0 events if you're running in G0.
exclude_host/exclude_guest are the right definition.
Quoted 2 lines
Then the next question is, if G0 is a host, does the L1-hv run in G0 userspace or G0 kernel space?
It's mostly kernel, but sometimes you're interested in events from QEMU or whoever else has opened /dev/kvm. In that case you care about G0 userspace too.
Quoted 3 lines
The way it is implemented, you basically have to always set exclude_host=0, even if there is no virt at all and you want to measure your own userspace thing -- which is just weird.
I understand regretting having exclude_guest that way; include_guest (defaulting to 0!) would have made more sense. But defaulting to exclude_host==0 makes sense: if there is no virt at all, memset(0) does the right thing so it does not seem weird to me.
Quoted 11 lines
I suppose the 'best' option at this point is something like:
	/*
	 * comment that explains the trainwreck.
	 */
	if (!exclude_host && !exclude_guest)
		exclude_guest = 1;

	if ((!exclude_hv || !exclude_guest) && !perf_allow_kernel())
		return -EPERM;
Quoted 3 lines
But that takes away the possibility of actually having:
'exclude_host=0, exclude_guest=0' to create an event that measures both, which also sucks.
In fact both of the above "if"s suck. :(
Paolo

Paolo Bonzini

maintainer

2020-08-12 13:04 UTC #5 lore raw

Quoted 4 lines · Like

My proposal is to define:
the "hypervisor privilege levels" events in the KVM/x86 context as all the host kernel events plus /dev/kvm user space events.

What are "/dev/kvm user space events"? In any case, this patch should be included only in the series that adds exclude_hv support in arch/x86.

Paolo

Quoted 5 lines

If we add ".exclude_hv = 1" in the pmc_reprogram_counter(), do you see any side effect to cover the above usages?
The fact that exclude_hv has never been used in x86 does help the generic perf code to handle permission checks in a more concise way.

Xu, Like

2020-08-12 13:14 UTC #6 lore raw

Quoted 6 lines · Paolo Bonzini

Quoted 3 lines · Like
My proposal is to define:
the "hypervisor privilege levels" events in the KVM/x86 context as all the host kernel events plus /dev/kvm user space events.
What are "/dev/kvm user space events"? In any case, this patch should be included only in the series that adds exclude_hv support in arch/x86.

The exclude_kernel events from the QEMU or whoever else has opened /dev/kvm.

Do you see any (patches) gap if we map
the exclude_host events into exclude_hv events naturally ?

Thanks,
Like Xu

Quoted 7 lines

Paolo
Quoted 5 lines
If we add ".exclude_hv = 1" in the pmc_reprogram_counter(), do you see any side effect to cover the above usages?
The fact that exclude_hv has never been used in x86 does help the generic perf code to handle permission checks in a more concise way.

[email protected]

2020-08-12 13:32 UTC #7 lore raw

In reply to Paolo Bonzini

Quoted 7 lines · Paolo Bonzini

Quoted 2 lines · On 12/08/20 13:11, [email protected] wrote:
Right, but we want to tighten the permission checks and not excluding_hv is just sloppy.
I would just document that it's ignored as it doesn't make sense. ARM64 does that too, for new processors where the kernel is not itself split between supervisor and hypervisor privilege levels.

This isn't about x86, I want these checks in generic code. We have the flag, it needs checking.

unpriv users have no busniess getting anything from a possible hv.

Quoted 8 lines

Quoted 3 lines
The thing is, we very much do not want to allow unpriv user to be able to create: exclude_host=1, exclude_guest=0 counters (they currently can).
That would be the case of an unprivileged user that wants to measure performance of its guests. It's a scenario that makes a lot of sense, are you worried about side channels? Can perf-events on guests leak more about the host than perf-events on a random userspace program?

An unpriv user can run guests?

Quoted 12 lines

Quoted 12 lines
Also, exclude_host is really poorly defined:
https://lkml.kernel.org/r/[email protected]
"Suppose we have nested virt:
	  L0-hv
	  |
	  G0/L1-hv
	     |
	     G1

Quoted 8 lines

Quoted 5 lines
And we're running in G0, then:
- 'exclude_hv' would exclude L0 events
- 'exclude_host' would ... exclude L1-hv events?
- 'exclude_guest' would ... exclude G1 events?
From the point of view of G0, L0 *does not exist at all*. You just cannot see L0 events if you're running in G0.

On x86, probably, in general, I'm not at all sure, we have that exclude_hv flag after all.

Quoted 1 line

exclude_host/exclude_guest are the right definition.

For what? I still think exclude_host is absolute shit. If you set it, you'll not get anything even without virt.

Run a native linux kernel, no kvm loaded, create a counter with exclude_host=1 and you'll get nothing, that's just really confusing IMO. There is no host, so excluding it should not affect anything.

Quoted 6 lines

Quoted 2 lines
Then the next question is, if G0 is a host, does the L1-hv run in G0 userspace or G0 kernel space?
It's mostly kernel, but sometimes you're interested in events from QEMU or whoever else has opened /dev/kvm. In that case you care about G0 userspace too.

I really don't think userspace helpers should be consideed part of the host, but whatever.

Quoted 8 lines

Quoted 3 lines
The way it is implemented, you basically have to always set exclude_host=0, even if there is no virt at all and you want to measure your own userspace thing -- which is just weird.
I understand regretting having exclude_guest that way; include_guest (defaulting to 0!) would have made more sense. But defaulting to exclude_host==0 makes sense: if there is no virt at all, memset(0) does the right thing so it does not seem weird to me.

Sure, but having exclude_host affect anything outside of kvm is still dodgy as heck.

Quoted 8 lines

Quoted 8 lines
I suppose the 'best' option at this point is something like:
	/*
	 * comment that explains the trainwreck.
	 */
	if (!exclude_host && !exclude_guest)
		exclude_guest = 1;

Quoted 8 lines

Quoted 6 lines
	if ((!exclude_hv || !exclude_guest) && !perf_allow_kernel())
		return -EPERM;
But that takes away the possibility of actually having:
'exclude_host=0, exclude_guest=0' to create an event that measures both, which also sucks.
In fact both of the above "if"s suck. :(

If, as you seem to imply above, that unpriv users can create guests, then maybe so, but if I look at /dev/kvm it seems to have 0660 permissions and thus really requires privileges.

Paolo Bonzini

maintainer

2020-08-12 13:51 UTC #8 lore raw

Quoted 4 lines · On 12/08/20 15:31, [email protected] wrote:

This isn't about x86, I want these checks in generic code. We have the flag, it needs checking.
unpriv users have no busniess getting anything from a possible hv.

Ah ok if it's generic that sounds good.

Quoted 4 lines

Quoted 2 lines
That would be the case of an unprivileged user that wants to measure performance of its guests.
An unpriv user can run guests?

Sure, on most distros /dev/kvm is either 0666 or 0660, usually with group kvm if it's 0660. To run a guest you might have to be in group kvm, but it does not require either root or CAP_SOMETHING.

Quoted 18 lines

Quoted 18 lines
Quoted 17 lines
Also, exclude_host is really poorly defined:
https://lkml.kernel.org/r/[email protected]
"Suppose we have nested virt:
	  L0-hv
	  |
	  G0/L1-hv
	     |
	     G1
And we're running in G0, then:
- 'exclude_hv' would exclude L0 events
- 'exclude_host' would ... exclude L1-hv events?
- 'exclude_guest' would ... exclude G1 events?

Quoted 5 lines

Quoted 2 lines
From the point of view of G0, L0 *does not exist at all*. You just cannot see L0 events if you're running in G0.
On x86, probably, in general, I'm not at all sure, we have that exclude_hv flag after all.

No, and you can quote me on that: exclude_hv is *not* about excluding the hypervisor from a guest. It's about excluding the part of _your_ kernel which runs in a "more privileged" level (EL2 on ARM, HV on POWER).

Quoted 3 lines

Quoted 1 line
exclude_host/exclude_guest are the right definition.
For what?

I meant in the nested virt case you drew above.

Quoted 2 lines

I still think exclude_host is absolute shit. If you set it, you'll not get anything even without virt.

If you dislike the name you can change it to only_guest. Anybody who does not do virt just leaves it zero and is happy. Anybody who does not do virt and sets it gets what they expect (or deserve). But this definition is the same as exclude_host, and it's the correct one.

Quoted 3 lines

If, as you seem to imply above, that unpriv users can create guests, then maybe so, but if I look at /dev/kvm it seems to have 0660 permissions and thus really requires privileges.

Since you can be non-root and you don't need any capability either, it doesn't require what the kernel considers to be privilege.

Paolo