All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] KVM: arm64: Replace KVM_ARM_PMU with HW_PERF_EVENTS
@ 2021-01-04 17:27 ` Marc Zyngier
  0 siblings, 0 replies; 8+ messages in thread
From: Marc Zyngier @ 2021-01-04 17:27 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel; +Cc: kernel-team

KVM_ARM_PMU only existed for the benefit of 32bit ARM hosts,
and makes no sense now that we are 64bit only. Get rid of it.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/Kconfig  | 8 --------
 arch/arm64/kvm/Makefile | 2 +-
 include/kvm/arm_pmu.h   | 2 +-
 3 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index 043756db8f6e..3964acf5451e 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -49,14 +49,6 @@ if KVM
 
 source "virt/kvm/Kconfig"
 
-config KVM_ARM_PMU
-	bool "Virtual Performance Monitoring Unit (PMU) support"
-	depends on HW_PERF_EVENTS
-	default y
-	help
-	  Adds support for a virtual Performance Monitoring Unit (PMU) in
-	  virtual machines.
-
 endif # KVM
 
 endif # VIRTUALIZATION
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index 60fd181df624..13b017284bf9 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -24,4 +24,4 @@ kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o \
 	 vgic/vgic-mmio-v3.o vgic/vgic-kvm-device.o \
 	 vgic/vgic-its.o vgic/vgic-debug.o
 
-kvm-$(CONFIG_KVM_ARM_PMU)  += pmu-emul.o
+kvm-$(CONFIG_HW_PERF_EVENTS)  += pmu-emul.o
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index fc85f50fa0e9..8dcb3e1477bc 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -13,7 +13,7 @@
 #define ARMV8_PMU_CYCLE_IDX		(ARMV8_PMU_MAX_COUNTERS - 1)
 #define ARMV8_PMU_MAX_COUNTER_PAIRS	((ARMV8_PMU_MAX_COUNTERS + 1) >> 1)
 
-#ifdef CONFIG_KVM_ARM_PMU
+#ifdef CONFIG_HW_PERF_EVENTS
 
 struct kvm_pmc {
 	u8 idx;	/* index into the pmu->pmc array */
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH] KVM: arm64: Replace KVM_ARM_PMU with HW_PERF_EVENTS
@ 2021-01-04 17:27 ` Marc Zyngier
  0 siblings, 0 replies; 8+ messages in thread
From: Marc Zyngier @ 2021-01-04 17:27 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel
  Cc: Alexandru Elisei, kernel-team, James Morse, Julien Thierry,
	Suzuki K Poulose

KVM_ARM_PMU only existed for the benefit of 32bit ARM hosts,
and makes no sense now that we are 64bit only. Get rid of it.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/Kconfig  | 8 --------
 arch/arm64/kvm/Makefile | 2 +-
 include/kvm/arm_pmu.h   | 2 +-
 3 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index 043756db8f6e..3964acf5451e 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -49,14 +49,6 @@ if KVM
 
 source "virt/kvm/Kconfig"
 
-config KVM_ARM_PMU
-	bool "Virtual Performance Monitoring Unit (PMU) support"
-	depends on HW_PERF_EVENTS
-	default y
-	help
-	  Adds support for a virtual Performance Monitoring Unit (PMU) in
-	  virtual machines.
-
 endif # KVM
 
 endif # VIRTUALIZATION
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index 60fd181df624..13b017284bf9 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -24,4 +24,4 @@ kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o \
 	 vgic/vgic-mmio-v3.o vgic/vgic-kvm-device.o \
 	 vgic/vgic-its.o vgic/vgic-debug.o
 
-kvm-$(CONFIG_KVM_ARM_PMU)  += pmu-emul.o
+kvm-$(CONFIG_HW_PERF_EVENTS)  += pmu-emul.o
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index fc85f50fa0e9..8dcb3e1477bc 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -13,7 +13,7 @@
 #define ARMV8_PMU_CYCLE_IDX		(ARMV8_PMU_MAX_COUNTERS - 1)
 #define ARMV8_PMU_MAX_COUNTER_PAIRS	((ARMV8_PMU_MAX_COUNTERS + 1) >> 1)
 
-#ifdef CONFIG_KVM_ARM_PMU
+#ifdef CONFIG_HW_PERF_EVENTS
 
 struct kvm_pmc {
 	u8 idx;	/* index into the pmu->pmc array */
-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: arm64: Replace KVM_ARM_PMU with HW_PERF_EVENTS
  2021-01-04 17:27 ` Marc Zyngier
@ 2021-01-05 15:49   ` Alexandru Elisei
  -1 siblings, 0 replies; 8+ messages in thread
From: Alexandru Elisei @ 2021-01-05 15:49 UTC (permalink / raw)
  To: Marc Zyngier, kvmarm, linux-arm-kernel; +Cc: kernel-team

Hi Marc,

On 1/4/21 5:27 PM, Marc Zyngier wrote:
> KVM_ARM_PMU only existed for the benefit of 32bit ARM hosts,
> and makes no sense now that we are 64bit only. Get rid of it.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/Kconfig  | 8 --------
>  arch/arm64/kvm/Makefile | 2 +-
>  include/kvm/arm_pmu.h   | 2 +-
>  3 files changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
> index 043756db8f6e..3964acf5451e 100644
> --- a/arch/arm64/kvm/Kconfig
> +++ b/arch/arm64/kvm/Kconfig
> @@ -49,14 +49,6 @@ if KVM
>  
>  source "virt/kvm/Kconfig"
>  
> -config KVM_ARM_PMU
> -	bool "Virtual Performance Monitoring Unit (PMU) support"
> -	depends on HW_PERF_EVENTS
> -	default y
> -	help
> -	  Adds support for a virtual Performance Monitoring Unit (PMU) in
> -	  virtual machines.
> -
>  endif # KVM
>  
>  endif # VIRTUALIZATION
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index 60fd181df624..13b017284bf9 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -24,4 +24,4 @@ kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o \
>  	 vgic/vgic-mmio-v3.o vgic/vgic-kvm-device.o \
>  	 vgic/vgic-its.o vgic/vgic-debug.o
>  
> -kvm-$(CONFIG_KVM_ARM_PMU)  += pmu-emul.o
> +kvm-$(CONFIG_HW_PERF_EVENTS)  += pmu-emul.o
> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> index fc85f50fa0e9..8dcb3e1477bc 100644
> --- a/include/kvm/arm_pmu.h
> +++ b/include/kvm/arm_pmu.h
> @@ -13,7 +13,7 @@
>  #define ARMV8_PMU_CYCLE_IDX		(ARMV8_PMU_MAX_COUNTERS - 1)
>  #define ARMV8_PMU_MAX_COUNTER_PAIRS	((ARMV8_PMU_MAX_COUNTERS + 1) >> 1)
>  
> -#ifdef CONFIG_KVM_ARM_PMU
> +#ifdef CONFIG_HW_PERF_EVENTS
>  
>  struct kvm_pmc {
>  	u8 idx;	/* index into the pmu->pmc array */

I grep'ed for KVM_ARM_PMU in the Linux sources, this patch takes care of all its
occurrences.

A few things popped into my mind when I saw the patch.

1. Replacing KVM_ARM_PMU with CONFIG_HW_PERF_EVENTS means it's not possible
anymore for the host to have perf support while KVM does not support emulating a
PMU. In this scenario, functions which would have been empty functions if
KVM_ARM_PMU was still around (I only found kvm_pmu_flush_hwstate() and
kvm_pmu_sync_hwstate() on the KVM_RUN path) will now be called and return early
after kvm_vcpu_has_pmu() returns 0. The overhead looks negligible to me, and I
don't think this configuration was common (especially since the default was y).

2. I did a grep for the files that include arm_pmu.h, and all the files were in
arch/arm64. I suppose arm_pmu.h exists in include/kvm instead of
arch/arm64/include/asm because it was shared with KVM/arm when it was still
around, right? Or is there another reason for that?

[1] https://www.spinics.net/lists/kvm-arm/msg44184.html

Thanks,
Alex
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: arm64: Replace KVM_ARM_PMU with HW_PERF_EVENTS
@ 2021-01-05 15:49   ` Alexandru Elisei
  0 siblings, 0 replies; 8+ messages in thread
From: Alexandru Elisei @ 2021-01-05 15:49 UTC (permalink / raw)
  To: Marc Zyngier, kvmarm, linux-arm-kernel
  Cc: kernel-team, James Morse, Julien Thierry, Suzuki K Poulose

Hi Marc,

On 1/4/21 5:27 PM, Marc Zyngier wrote:
> KVM_ARM_PMU only existed for the benefit of 32bit ARM hosts,
> and makes no sense now that we are 64bit only. Get rid of it.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/Kconfig  | 8 --------
>  arch/arm64/kvm/Makefile | 2 +-
>  include/kvm/arm_pmu.h   | 2 +-
>  3 files changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
> index 043756db8f6e..3964acf5451e 100644
> --- a/arch/arm64/kvm/Kconfig
> +++ b/arch/arm64/kvm/Kconfig
> @@ -49,14 +49,6 @@ if KVM
>  
>  source "virt/kvm/Kconfig"
>  
> -config KVM_ARM_PMU
> -	bool "Virtual Performance Monitoring Unit (PMU) support"
> -	depends on HW_PERF_EVENTS
> -	default y
> -	help
> -	  Adds support for a virtual Performance Monitoring Unit (PMU) in
> -	  virtual machines.
> -
>  endif # KVM
>  
>  endif # VIRTUALIZATION
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index 60fd181df624..13b017284bf9 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -24,4 +24,4 @@ kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o \
>  	 vgic/vgic-mmio-v3.o vgic/vgic-kvm-device.o \
>  	 vgic/vgic-its.o vgic/vgic-debug.o
>  
> -kvm-$(CONFIG_KVM_ARM_PMU)  += pmu-emul.o
> +kvm-$(CONFIG_HW_PERF_EVENTS)  += pmu-emul.o
> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> index fc85f50fa0e9..8dcb3e1477bc 100644
> --- a/include/kvm/arm_pmu.h
> +++ b/include/kvm/arm_pmu.h
> @@ -13,7 +13,7 @@
>  #define ARMV8_PMU_CYCLE_IDX		(ARMV8_PMU_MAX_COUNTERS - 1)
>  #define ARMV8_PMU_MAX_COUNTER_PAIRS	((ARMV8_PMU_MAX_COUNTERS + 1) >> 1)
>  
> -#ifdef CONFIG_KVM_ARM_PMU
> +#ifdef CONFIG_HW_PERF_EVENTS
>  
>  struct kvm_pmc {
>  	u8 idx;	/* index into the pmu->pmc array */

I grep'ed for KVM_ARM_PMU in the Linux sources, this patch takes care of all its
occurrences.

A few things popped into my mind when I saw the patch.

1. Replacing KVM_ARM_PMU with CONFIG_HW_PERF_EVENTS means it's not possible
anymore for the host to have perf support while KVM does not support emulating a
PMU. In this scenario, functions which would have been empty functions if
KVM_ARM_PMU was still around (I only found kvm_pmu_flush_hwstate() and
kvm_pmu_sync_hwstate() on the KVM_RUN path) will now be called and return early
after kvm_vcpu_has_pmu() returns 0. The overhead looks negligible to me, and I
don't think this configuration was common (especially since the default was y).

2. I did a grep for the files that include arm_pmu.h, and all the files were in
arch/arm64. I suppose arm_pmu.h exists in include/kvm instead of
arch/arm64/include/asm because it was shared with KVM/arm when it was still
around, right? Or is there another reason for that?

[1] https://www.spinics.net/lists/kvm-arm/msg44184.html

Thanks,
Alex

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: arm64: Replace KVM_ARM_PMU with HW_PERF_EVENTS
  2021-01-05 15:49   ` Alexandru Elisei
@ 2021-01-05 16:25     ` Marc Zyngier
  -1 siblings, 0 replies; 8+ messages in thread
From: Marc Zyngier @ 2021-01-05 16:25 UTC (permalink / raw)
  To: Alexandru Elisei; +Cc: linux-arm-kernel, kernel-team, kvmarm

Hi Alex,

On 2021-01-05 15:49, Alexandru Elisei wrote:
> Hi Marc,
> 
> On 1/4/21 5:27 PM, Marc Zyngier wrote:
>> KVM_ARM_PMU only existed for the benefit of 32bit ARM hosts,
>> and makes no sense now that we are 64bit only. Get rid of it.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/kvm/Kconfig  | 8 --------
>>  arch/arm64/kvm/Makefile | 2 +-
>>  include/kvm/arm_pmu.h   | 2 +-
>>  3 files changed, 2 insertions(+), 10 deletions(-)
>> 
>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
>> index 043756db8f6e..3964acf5451e 100644
>> --- a/arch/arm64/kvm/Kconfig
>> +++ b/arch/arm64/kvm/Kconfig
>> @@ -49,14 +49,6 @@ if KVM
>> 
>>  source "virt/kvm/Kconfig"
>> 
>> -config KVM_ARM_PMU
>> -	bool "Virtual Performance Monitoring Unit (PMU) support"
>> -	depends on HW_PERF_EVENTS
>> -	default y
>> -	help
>> -	  Adds support for a virtual Performance Monitoring Unit (PMU) in
>> -	  virtual machines.
>> -
>>  endif # KVM
>> 
>>  endif # VIRTUALIZATION
>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> index 60fd181df624..13b017284bf9 100644
>> --- a/arch/arm64/kvm/Makefile
>> +++ b/arch/arm64/kvm/Makefile
>> @@ -24,4 +24,4 @@ kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o 
>> $(KVM)/eventfd.o \
>>  	 vgic/vgic-mmio-v3.o vgic/vgic-kvm-device.o \
>>  	 vgic/vgic-its.o vgic/vgic-debug.o
>> 
>> -kvm-$(CONFIG_KVM_ARM_PMU)  += pmu-emul.o
>> +kvm-$(CONFIG_HW_PERF_EVENTS)  += pmu-emul.o
>> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
>> index fc85f50fa0e9..8dcb3e1477bc 100644
>> --- a/include/kvm/arm_pmu.h
>> +++ b/include/kvm/arm_pmu.h
>> @@ -13,7 +13,7 @@
>>  #define ARMV8_PMU_CYCLE_IDX		(ARMV8_PMU_MAX_COUNTERS - 1)
>>  #define ARMV8_PMU_MAX_COUNTER_PAIRS	((ARMV8_PMU_MAX_COUNTERS + 1) >> 
>> 1)
>> 
>> -#ifdef CONFIG_KVM_ARM_PMU
>> +#ifdef CONFIG_HW_PERF_EVENTS
>> 
>>  struct kvm_pmc {
>>  	u8 idx;	/* index into the pmu->pmc array */
> 
> I grep'ed for KVM_ARM_PMU in the Linux sources, this patch takes care 
> of all its
> occurrences.
> 
> A few things popped into my mind when I saw the patch.
> 
> 1. Replacing KVM_ARM_PMU with CONFIG_HW_PERF_EVENTS means it's not 
> possible
> anymore for the host to have perf support while KVM does not support 
> emulating a
> PMU. In this scenario, functions which would have been empty functions 
> if
> KVM_ARM_PMU was still around (I only found kvm_pmu_flush_hwstate() and
> kvm_pmu_sync_hwstate() on the KVM_RUN path) will now be called and 
> return early
> after kvm_vcpu_has_pmu() returns 0. The overhead looks negligible to 
> me, and I
> don't think this configuration was common (especially since the default 
> was y).

I don't think this is either common nor useful. If the kernel supports
the PMU, then finding a PMU enables all the uses of the PMU, including
for guests. And userspace is still in control of what it exposes to
the guest. Yes, it's a tiny more overhead (one extra load) on the
exit/entry path. Should we care? I don't think so.

> 2. I did a grep for the files that include arm_pmu.h, and all the files 
> were in
> arch/arm64. I suppose arm_pmu.h exists in include/kvm instead of
> arch/arm64/include/asm because it was shared with KVM/arm when it was 
> still
> around, right? Or is there another reason for that?

No, that's basically the only reason. That was the easy landing spot
for anything shared (including things like GIC, timers and co).
I'm not sure it is worth the move, TBH...

> [1] https://www.spinics.net/lists/kvm-arm/msg44184.html

Yuo, and that's the reason I posted this patch. I have a couple more
that I'll post by the end of the day.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: arm64: Replace KVM_ARM_PMU with HW_PERF_EVENTS
@ 2021-01-05 16:25     ` Marc Zyngier
  0 siblings, 0 replies; 8+ messages in thread
From: Marc Zyngier @ 2021-01-05 16:25 UTC (permalink / raw)
  To: Alexandru Elisei
  Cc: Suzuki K Poulose, James Morse, linux-arm-kernel, kernel-team,
	kvmarm, Julien Thierry

Hi Alex,

On 2021-01-05 15:49, Alexandru Elisei wrote:
> Hi Marc,
> 
> On 1/4/21 5:27 PM, Marc Zyngier wrote:
>> KVM_ARM_PMU only existed for the benefit of 32bit ARM hosts,
>> and makes no sense now that we are 64bit only. Get rid of it.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/kvm/Kconfig  | 8 --------
>>  arch/arm64/kvm/Makefile | 2 +-
>>  include/kvm/arm_pmu.h   | 2 +-
>>  3 files changed, 2 insertions(+), 10 deletions(-)
>> 
>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
>> index 043756db8f6e..3964acf5451e 100644
>> --- a/arch/arm64/kvm/Kconfig
>> +++ b/arch/arm64/kvm/Kconfig
>> @@ -49,14 +49,6 @@ if KVM
>> 
>>  source "virt/kvm/Kconfig"
>> 
>> -config KVM_ARM_PMU
>> -	bool "Virtual Performance Monitoring Unit (PMU) support"
>> -	depends on HW_PERF_EVENTS
>> -	default y
>> -	help
>> -	  Adds support for a virtual Performance Monitoring Unit (PMU) in
>> -	  virtual machines.
>> -
>>  endif # KVM
>> 
>>  endif # VIRTUALIZATION
>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> index 60fd181df624..13b017284bf9 100644
>> --- a/arch/arm64/kvm/Makefile
>> +++ b/arch/arm64/kvm/Makefile
>> @@ -24,4 +24,4 @@ kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o 
>> $(KVM)/eventfd.o \
>>  	 vgic/vgic-mmio-v3.o vgic/vgic-kvm-device.o \
>>  	 vgic/vgic-its.o vgic/vgic-debug.o
>> 
>> -kvm-$(CONFIG_KVM_ARM_PMU)  += pmu-emul.o
>> +kvm-$(CONFIG_HW_PERF_EVENTS)  += pmu-emul.o
>> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
>> index fc85f50fa0e9..8dcb3e1477bc 100644
>> --- a/include/kvm/arm_pmu.h
>> +++ b/include/kvm/arm_pmu.h
>> @@ -13,7 +13,7 @@
>>  #define ARMV8_PMU_CYCLE_IDX		(ARMV8_PMU_MAX_COUNTERS - 1)
>>  #define ARMV8_PMU_MAX_COUNTER_PAIRS	((ARMV8_PMU_MAX_COUNTERS + 1) >> 
>> 1)
>> 
>> -#ifdef CONFIG_KVM_ARM_PMU
>> +#ifdef CONFIG_HW_PERF_EVENTS
>> 
>>  struct kvm_pmc {
>>  	u8 idx;	/* index into the pmu->pmc array */
> 
> I grep'ed for KVM_ARM_PMU in the Linux sources, this patch takes care 
> of all its
> occurrences.
> 
> A few things popped into my mind when I saw the patch.
> 
> 1. Replacing KVM_ARM_PMU with CONFIG_HW_PERF_EVENTS means it's not 
> possible
> anymore for the host to have perf support while KVM does not support 
> emulating a
> PMU. In this scenario, functions which would have been empty functions 
> if
> KVM_ARM_PMU was still around (I only found kvm_pmu_flush_hwstate() and
> kvm_pmu_sync_hwstate() on the KVM_RUN path) will now be called and 
> return early
> after kvm_vcpu_has_pmu() returns 0. The overhead looks negligible to 
> me, and I
> don't think this configuration was common (especially since the default 
> was y).

I don't think this is either common nor useful. If the kernel supports
the PMU, then finding a PMU enables all the uses of the PMU, including
for guests. And userspace is still in control of what it exposes to
the guest. Yes, it's a tiny more overhead (one extra load) on the
exit/entry path. Should we care? I don't think so.

> 2. I did a grep for the files that include arm_pmu.h, and all the files 
> were in
> arch/arm64. I suppose arm_pmu.h exists in include/kvm instead of
> arch/arm64/include/asm because it was shared with KVM/arm when it was 
> still
> around, right? Or is there another reason for that?

No, that's basically the only reason. That was the easy landing spot
for anything shared (including things like GIC, timers and co).
I'm not sure it is worth the move, TBH...

> [1] https://www.spinics.net/lists/kvm-arm/msg44184.html

Yuo, and that's the reason I posted this patch. I have a couple more
that I'll post by the end of the day.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: arm64: Replace KVM_ARM_PMU with HW_PERF_EVENTS
  2021-01-05 16:25     ` Marc Zyngier
@ 2021-01-05 16:38       ` Alexandru Elisei
  -1 siblings, 0 replies; 8+ messages in thread
From: Alexandru Elisei @ 2021-01-05 16:38 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-arm-kernel, kernel-team, kvmarm

Hi Marc,

On 1/5/21 4:25 PM, Marc Zyngier wrote:
> Hi Alex,
>
> On 2021-01-05 15:49, Alexandru Elisei wrote:
>> Hi Marc,
>>
>> On 1/4/21 5:27 PM, Marc Zyngier wrote:
>>> KVM_ARM_PMU only existed for the benefit of 32bit ARM hosts,
>>> and makes no sense now that we are 64bit only. Get rid of it.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  arch/arm64/kvm/Kconfig  | 8 --------
>>>  arch/arm64/kvm/Makefile | 2 +-
>>>  include/kvm/arm_pmu.h   | 2 +-
>>>  3 files changed, 2 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
>>> index 043756db8f6e..3964acf5451e 100644
>>> --- a/arch/arm64/kvm/Kconfig
>>> +++ b/arch/arm64/kvm/Kconfig
>>> @@ -49,14 +49,6 @@ if KVM
>>>
>>>  source "virt/kvm/Kconfig"
>>>
>>> -config KVM_ARM_PMU
>>> -    bool "Virtual Performance Monitoring Unit (PMU) support"
>>> -    depends on HW_PERF_EVENTS
>>> -    default y
>>> -    help
>>> -      Adds support for a virtual Performance Monitoring Unit (PMU) in
>>> -      virtual machines.
>>> -
>>>  endif # KVM
>>>
>>>  endif # VIRTUALIZATION
>>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>>> index 60fd181df624..13b017284bf9 100644
>>> --- a/arch/arm64/kvm/Makefile
>>> +++ b/arch/arm64/kvm/Makefile
>>> @@ -24,4 +24,4 @@ kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o
>>> $(KVM)/eventfd.o \
>>>       vgic/vgic-mmio-v3.o vgic/vgic-kvm-device.o \
>>>       vgic/vgic-its.o vgic/vgic-debug.o
>>>
>>> -kvm-$(CONFIG_KVM_ARM_PMU)  += pmu-emul.o
>>> +kvm-$(CONFIG_HW_PERF_EVENTS)  += pmu-emul.o
>>> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
>>> index fc85f50fa0e9..8dcb3e1477bc 100644
>>> --- a/include/kvm/arm_pmu.h
>>> +++ b/include/kvm/arm_pmu.h
>>> @@ -13,7 +13,7 @@
>>>  #define ARMV8_PMU_CYCLE_IDX        (ARMV8_PMU_MAX_COUNTERS - 1)
>>>  #define ARMV8_PMU_MAX_COUNTER_PAIRS    ((ARMV8_PMU_MAX_COUNTERS + 1) >> 1)
>>>
>>> -#ifdef CONFIG_KVM_ARM_PMU
>>> +#ifdef CONFIG_HW_PERF_EVENTS
>>>
>>>  struct kvm_pmc {
>>>      u8 idx;    /* index into the pmu->pmc array */
>>
>> I grep'ed for KVM_ARM_PMU in the Linux sources, this patch takes care of all its
>> occurrences.
>>
>> A few things popped into my mind when I saw the patch.
>>
>> 1. Replacing KVM_ARM_PMU with CONFIG_HW_PERF_EVENTS means it's not possible
>> anymore for the host to have perf support while KVM does not support emulating a
>> PMU. In this scenario, functions which would have been empty functions if
>> KVM_ARM_PMU was still around (I only found kvm_pmu_flush_hwstate() and
>> kvm_pmu_sync_hwstate() on the KVM_RUN path) will now be called and return early
>> after kvm_vcpu_has_pmu() returns 0. The overhead looks negligible to me, and I
>> don't think this configuration was common (especially since the default was y).
>
> I don't think this is either common nor useful. If the kernel supports
> the PMU, then finding a PMU enables all the uses of the PMU, including
> for guests. And userspace is still in control of what it exposes to
> the guest. Yes, it's a tiny more overhead (one extra load) on the
> exit/entry path. Should we care? I don't think so.

Yes, I agree:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

>
>> 2. I did a grep for the files that include arm_pmu.h, and all the files were in
>> arch/arm64. I suppose arm_pmu.h exists in include/kvm instead of
>> arch/arm64/include/asm because it was shared with KVM/arm when it was still
>> around, right? Or is there another reason for that?
>
> No, that's basically the only reason. That was the easy landing spot
> for anything shared (including things like GIC, timers and co).
> I'm not sure it is worth the move, TBH...

I feel the same way, I was asking more in the context of new code. I admit to also
having an ulterior motive, as one of the patches I picked up for the SPE series
added a header file in include/kvm and it looked a bit out of place.

Thanks,
Alex
>
>> [1] https://www.spinics.net/lists/kvm-arm/msg44184.html
>
> Yuo, and that's the reason I posted this patch. I have a couple more
> that I'll post by the end of the day.
>
> Thanks,
>
>         M.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: arm64: Replace KVM_ARM_PMU with HW_PERF_EVENTS
@ 2021-01-05 16:38       ` Alexandru Elisei
  0 siblings, 0 replies; 8+ messages in thread
From: Alexandru Elisei @ 2021-01-05 16:38 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Suzuki K Poulose, James Morse, linux-arm-kernel, kernel-team,
	kvmarm, Julien Thierry

Hi Marc,

On 1/5/21 4:25 PM, Marc Zyngier wrote:
> Hi Alex,
>
> On 2021-01-05 15:49, Alexandru Elisei wrote:
>> Hi Marc,
>>
>> On 1/4/21 5:27 PM, Marc Zyngier wrote:
>>> KVM_ARM_PMU only existed for the benefit of 32bit ARM hosts,
>>> and makes no sense now that we are 64bit only. Get rid of it.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  arch/arm64/kvm/Kconfig  | 8 --------
>>>  arch/arm64/kvm/Makefile | 2 +-
>>>  include/kvm/arm_pmu.h   | 2 +-
>>>  3 files changed, 2 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
>>> index 043756db8f6e..3964acf5451e 100644
>>> --- a/arch/arm64/kvm/Kconfig
>>> +++ b/arch/arm64/kvm/Kconfig
>>> @@ -49,14 +49,6 @@ if KVM
>>>
>>>  source "virt/kvm/Kconfig"
>>>
>>> -config KVM_ARM_PMU
>>> -    bool "Virtual Performance Monitoring Unit (PMU) support"
>>> -    depends on HW_PERF_EVENTS
>>> -    default y
>>> -    help
>>> -      Adds support for a virtual Performance Monitoring Unit (PMU) in
>>> -      virtual machines.
>>> -
>>>  endif # KVM
>>>
>>>  endif # VIRTUALIZATION
>>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>>> index 60fd181df624..13b017284bf9 100644
>>> --- a/arch/arm64/kvm/Makefile
>>> +++ b/arch/arm64/kvm/Makefile
>>> @@ -24,4 +24,4 @@ kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o
>>> $(KVM)/eventfd.o \
>>>       vgic/vgic-mmio-v3.o vgic/vgic-kvm-device.o \
>>>       vgic/vgic-its.o vgic/vgic-debug.o
>>>
>>> -kvm-$(CONFIG_KVM_ARM_PMU)  += pmu-emul.o
>>> +kvm-$(CONFIG_HW_PERF_EVENTS)  += pmu-emul.o
>>> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
>>> index fc85f50fa0e9..8dcb3e1477bc 100644
>>> --- a/include/kvm/arm_pmu.h
>>> +++ b/include/kvm/arm_pmu.h
>>> @@ -13,7 +13,7 @@
>>>  #define ARMV8_PMU_CYCLE_IDX        (ARMV8_PMU_MAX_COUNTERS - 1)
>>>  #define ARMV8_PMU_MAX_COUNTER_PAIRS    ((ARMV8_PMU_MAX_COUNTERS + 1) >> 1)
>>>
>>> -#ifdef CONFIG_KVM_ARM_PMU
>>> +#ifdef CONFIG_HW_PERF_EVENTS
>>>
>>>  struct kvm_pmc {
>>>      u8 idx;    /* index into the pmu->pmc array */
>>
>> I grep'ed for KVM_ARM_PMU in the Linux sources, this patch takes care of all its
>> occurrences.
>>
>> A few things popped into my mind when I saw the patch.
>>
>> 1. Replacing KVM_ARM_PMU with CONFIG_HW_PERF_EVENTS means it's not possible
>> anymore for the host to have perf support while KVM does not support emulating a
>> PMU. In this scenario, functions which would have been empty functions if
>> KVM_ARM_PMU was still around (I only found kvm_pmu_flush_hwstate() and
>> kvm_pmu_sync_hwstate() on the KVM_RUN path) will now be called and return early
>> after kvm_vcpu_has_pmu() returns 0. The overhead looks negligible to me, and I
>> don't think this configuration was common (especially since the default was y).
>
> I don't think this is either common nor useful. If the kernel supports
> the PMU, then finding a PMU enables all the uses of the PMU, including
> for guests. And userspace is still in control of what it exposes to
> the guest. Yes, it's a tiny more overhead (one extra load) on the
> exit/entry path. Should we care? I don't think so.

Yes, I agree:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

>
>> 2. I did a grep for the files that include arm_pmu.h, and all the files were in
>> arch/arm64. I suppose arm_pmu.h exists in include/kvm instead of
>> arch/arm64/include/asm because it was shared with KVM/arm when it was still
>> around, right? Or is there another reason for that?
>
> No, that's basically the only reason. That was the easy landing spot
> for anything shared (including things like GIC, timers and co).
> I'm not sure it is worth the move, TBH...

I feel the same way, I was asking more in the context of new code. I admit to also
having an ulterior motive, as one of the patches I picked up for the SPE series
added a header file in include/kvm and it looked a bit out of place.

Thanks,
Alex
>
>> [1] https://www.spinics.net/lists/kvm-arm/msg44184.html
>
> Yuo, and that's the reason I posted this patch. I have a couple more
> that I'll post by the end of the day.
>
> Thanks,
>
>         M.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-01-05 16:40 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-04 17:27 [PATCH] KVM: arm64: Replace KVM_ARM_PMU with HW_PERF_EVENTS Marc Zyngier
2021-01-04 17:27 ` Marc Zyngier
2021-01-05 15:49 ` Alexandru Elisei
2021-01-05 15:49   ` Alexandru Elisei
2021-01-05 16:25   ` Marc Zyngier
2021-01-05 16:25     ` Marc Zyngier
2021-01-05 16:38     ` Alexandru Elisei
2021-01-05 16:38       ` Alexandru Elisei

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.