[PATCH 0/8] KVM: arm64: Disabled PMU handling

[PATCH 0/8] KVM: arm64: Disabled PMU handling

[Marc Zyngier]

It recently dawned on me that the way we handle PMU traps when the PMU
is disabled is plain wrong. We consider that handling the registers as
RAZ/WI is a fine thing to do, while the ARMv8 ARM is pretty clear that
that's not OK and that such registers should UNDEF when FEAT_PMUv3
doesn't exist. I went all the way back to the first public version of
the spec, and it turns out we were *always* wrong.

It probably comes from the fact that we used not to trap the ID
registers, and thus were unable to advertise the lack of PMU, but
that's hardly an excuse. So let's fix the damned thing.

This series adds an extra check in the helpers that check for the
validity of the PMU access (most of the registers have to checked
against some enable flags and/or the accessing exception level), and
rids us of the RAZ/WI behaviour.

This enables us to make additional cleanups, to the point where we can
remove the PMU "ready" state that always had very bizarre semantics.
All in all, a negative diffstat, and spec compliant behaviours. What's
not to like?

I've run a few guests with and without PMUs as well as KUT, and
nothing caught fire. The patches are on top of kvmarm/queue.

Marc Zyngier (8):
  KVM: arm64: Add kvm_vcpu_has_pmu() helper
  KVM: arm64: Set ID_AA64DFR0_EL1.PMUVer to 0 when no PMU support
  KVM: arm64: Refuse illegal KVM_ARM_VCPU_PMU_V3 at reset time
  KVM: arm64: Inject UNDEF on PMU access when no PMU configured
  KVM: arm64: Remove PMU RAZ/WI handling
  KVM: arm64: Remove dead PMU sysreg decoding code
  KVM: arm64: Gate kvm_pmu_update_state() on the PMU feature
  KVM: arm64: Get rid of the PMU ready state

 arch/arm64/include/asm/kvm_host.h |  3 ++
 arch/arm64/kvm/pmu-emul.c         | 11 +++----
 arch/arm64/kvm/reset.c            |  4 +++
 arch/arm64/kvm/sys_regs.c         | 51 ++++++++-----------------------
 include/kvm/arm_pmu.h             |  3 --
 5 files changed, 24 insertions(+), 48 deletions(-)

-- 
2.28.0


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

[PATCH 1/8] KVM: arm64: Add kvm_vcpu_has_pmu() helper

[Marc Zyngier]

There are a number of places where we check for the KVM_ARM_VCPU_PMU_V3
feature. Wrap this check into a new kvm_vcpu_has_pmu(), and use
it at the existing locations.

No functional change.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h | 3 +++
 arch/arm64/kvm/pmu-emul.c         | 8 +++-----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 709f892f7a14..8c681d621a82 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -731,4 +731,7 @@ bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
 #define kvm_arm_vcpu_sve_finalized(vcpu) \
 	((vcpu)->arch.flags & KVM_ARM64_VCPU_SVE_FINALIZED)
 
+#define kvm_vcpu_has_pmu(vcpu)					\
+	(test_bit(KVM_ARM_VCPU_PMU_V3, (vcpu)->arch.features))
+
 #endif /* __ARM64_KVM_HOST_H__ */
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 2ed5ef8f274b..e7e3b4629864 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -913,8 +913,7 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
 
 int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 {
-	if (!kvm_arm_support_pmu_v3() ||
-	    !test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
+	if (!kvm_arm_support_pmu_v3() || !kvm_vcpu_has_pmu(vcpu))
 		return -ENODEV;
 
 	if (vcpu->arch.pmu.created)
@@ -1015,7 +1014,7 @@ int kvm_arm_pmu_v3_get_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 		if (!irqchip_in_kernel(vcpu->kvm))
 			return -EINVAL;
 
-		if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
+		if (!kvm_vcpu_has_pmu(vcpu))
 			return -ENODEV;
 
 		if (!kvm_arm_pmu_irq_initialized(vcpu))
@@ -1035,8 +1034,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 	case KVM_ARM_VCPU_PMU_V3_IRQ:
 	case KVM_ARM_VCPU_PMU_V3_INIT:
 	case KVM_ARM_VCPU_PMU_V3_FILTER:
-		if (kvm_arm_support_pmu_v3() &&
-		    test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
+		if (kvm_arm_support_pmu_v3() && kvm_vcpu_has_pmu(vcpu))
 			return 0;
 	}
 
-- 
2.28.0


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

[PATCH 2/8] KVM: arm64: Set ID_AA64DFR0_EL1.PMUVer to 0 when no PMU support

[Marc Zyngier]

We always expose the HW view of PMU in ID_AA64FDR0_EL1.PMUver,
even when the PMU feature is disabled, while the architecture
says that FEAT_PMUv3 not being implemented should result in this
field being zero.

Let's follow the architecture's guidance.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index d2e1d745f067..6629cfde2838 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1070,10 +1070,15 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 			 (0xfUL << ID_AA64ISAR1_GPA_SHIFT) |
 			 (0xfUL << ID_AA64ISAR1_GPI_SHIFT));
 	} else if (id == SYS_ID_AA64DFR0_EL1) {
+		u64 cap = 0;
+
 		/* Limit guests to PMUv3 for ARMv8.1 */
+		if (kvm_vcpu_has_pmu(vcpu))
+			cap = ID_AA64DFR0_PMUVER_8_1;
+
 		val = cpuid_feature_cap_perfmon_field(val,
 						ID_AA64DFR0_PMUVER_SHIFT,
-						ID_AA64DFR0_PMUVER_8_1);
+						cap);
 	} else if (id == SYS_ID_DFR0_EL1) {
 		/* Limit guests to PMUv3 for ARMv8.1 */
 		val = cpuid_feature_cap_perfmon_field(val,
-- 
2.28.0


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

[PATCH 3/8] KVM: arm64: Refuse illegal KVM_ARM_VCPU_PMU_V3 at reset time

[Marc Zyngier]

We accept to configure a PMU when a vcpu is created, even if the
HW (or the host) doesn't support it. This results in failures
when attributes get set, which is a bit odd as we should have
failed the vcpu creation the first place.

Move the check to the point where we check the vcpu feature set,
and fail early if we cannot support a PMU. This further simplifies
the attribute handling.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/pmu-emul.c | 4 ++--
 arch/arm64/kvm/reset.c    | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index e7e3b4629864..200f2a0d8d17 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -913,7 +913,7 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
 
 int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 {
-	if (!kvm_arm_support_pmu_v3() || !kvm_vcpu_has_pmu(vcpu))
+	if (!kvm_vcpu_has_pmu(vcpu))
 		return -ENODEV;
 
 	if (vcpu->arch.pmu.created)
@@ -1034,7 +1034,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 	case KVM_ARM_VCPU_PMU_V3_IRQ:
 	case KVM_ARM_VCPU_PMU_V3_INIT:
 	case KVM_ARM_VCPU_PMU_V3_FILTER:
-		if (kvm_arm_support_pmu_v3() && kvm_vcpu_has_pmu(vcpu))
+		if (kvm_vcpu_has_pmu(vcpu))
 			return 0;
 	}
 
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index 74ce92a4988c..3e772ea4e066 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -285,6 +285,10 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 			pstate = VCPU_RESET_PSTATE_EL1;
 		}
 
+		if (kvm_vcpu_has_pmu(vcpu) && !kvm_arm_support_pmu_v3()) {
+			ret = -EINVAL;
+			goto out;
+		}
 		break;
 	}
 
-- 
2.28.0


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

Re: [PATCH 3/8] KVM: arm64: Refuse illegal KVM_ARM_VCPU_PMU_V3 at reset time

[Alexandru Elisei]

Hi Marc,

On 11/13/20 6:25 PM, Marc Zyngier wrote:
> We accept to configure a PMU when a vcpu is created, even if the
> HW (or the host) doesn't support it. This results in failures
> when attributes get set, which is a bit odd as we should have
> failed the vcpu creation the first place.
>
> Move the check to the point where we check the vcpu feature set,
> and fail early if we cannot support a PMU. This further simplifies
> the attribute handling.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/pmu-emul.c | 4 ++--
>  arch/arm64/kvm/reset.c    | 4 ++++
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index e7e3b4629864..200f2a0d8d17 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -913,7 +913,7 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
>  
>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  {
> -	if (!kvm_arm_support_pmu_v3() || !kvm_vcpu_has_pmu(vcpu))
> +	if (!kvm_vcpu_has_pmu(vcpu))
>  		return -ENODEV;
>  
>  	if (vcpu->arch.pmu.created)
> @@ -1034,7 +1034,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  	case KVM_ARM_VCPU_PMU_V3_IRQ:
>  	case KVM_ARM_VCPU_PMU_V3_INIT:
>  	case KVM_ARM_VCPU_PMU_V3_FILTER:
> -		if (kvm_arm_support_pmu_v3() && kvm_vcpu_has_pmu(vcpu))
> +		if (kvm_vcpu_has_pmu(vcpu))
>  			return 0;
>  	}
>  
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index 74ce92a4988c..3e772ea4e066 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -285,6 +285,10 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  			pstate = VCPU_RESET_PSTATE_EL1;
>  		}
>  
> +		if (kvm_vcpu_has_pmu(vcpu) && !kvm_arm_support_pmu_v3()) {
> +			ret = -EINVAL;
> +			goto out;
> +		}

This looks correct, but right at the beginning of the function, before this
non-preemptible section, we do kvm_pmu_vcpu_reset(), which is wrong for several
reasons:

- we don't check if the feature flag is set
- we don't check if the hardware supports a PMU
- kvm_pmu_vcpu_reset() relies on __vcpu_sys_reg(vcpu, PMCR_EL0), which is set in
kvm_reset_sys_regs() below when the VCPU is initialized.

This looks to me like a separate issue, I have a patch locally to fix it by moving
kvm_pmu_vcpu_reset() after the non-preemptible section, but it's fine by me if you
want to fold a fix into this patch.

Thanks,

Alex

>  		break;
>  	}
>  

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

Re: [PATCH 3/8] KVM: arm64: Refuse illegal KVM_ARM_VCPU_PMU_V3 at reset time

[Marc Zyngier]

Hi Alex,

On 2020-11-26 14:59, Alexandru Elisei wrote:
> Hi Marc,
> 
> On 11/13/20 6:25 PM, Marc Zyngier wrote:
>> We accept to configure a PMU when a vcpu is created, even if the
>> HW (or the host) doesn't support it. This results in failures
>> when attributes get set, which is a bit odd as we should have
>> failed the vcpu creation the first place.
>> 
>> Move the check to the point where we check the vcpu feature set,
>> and fail early if we cannot support a PMU. This further simplifies
>> the attribute handling.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/kvm/pmu-emul.c | 4 ++--
>>  arch/arm64/kvm/reset.c    | 4 ++++
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>> index e7e3b4629864..200f2a0d8d17 100644
>> --- a/arch/arm64/kvm/pmu-emul.c
>> +++ b/arch/arm64/kvm/pmu-emul.c
>> @@ -913,7 +913,7 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int 
>> irq)
>> 
>>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct 
>> kvm_device_attr *attr)
>>  {
>> -	if (!kvm_arm_support_pmu_v3() || !kvm_vcpu_has_pmu(vcpu))
>> +	if (!kvm_vcpu_has_pmu(vcpu))
>>  		return -ENODEV;
>> 
>>  	if (vcpu->arch.pmu.created)
>> @@ -1034,7 +1034,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu 
>> *vcpu, struct kvm_device_attr *attr)
>>  	case KVM_ARM_VCPU_PMU_V3_IRQ:
>>  	case KVM_ARM_VCPU_PMU_V3_INIT:
>>  	case KVM_ARM_VCPU_PMU_V3_FILTER:
>> -		if (kvm_arm_support_pmu_v3() && kvm_vcpu_has_pmu(vcpu))
>> +		if (kvm_vcpu_has_pmu(vcpu))
>>  			return 0;
>>  	}
>> 
>> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
>> index 74ce92a4988c..3e772ea4e066 100644
>> --- a/arch/arm64/kvm/reset.c
>> +++ b/arch/arm64/kvm/reset.c
>> @@ -285,6 +285,10 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>>  			pstate = VCPU_RESET_PSTATE_EL1;
>>  		}
>> 
>> +		if (kvm_vcpu_has_pmu(vcpu) && !kvm_arm_support_pmu_v3()) {
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
> 
> This looks correct, but right at the beginning of the function, before 
> this
> non-preemptible section, we do kvm_pmu_vcpu_reset(), which is wrong for 
> several
> reasons:
> 
> - we don't check if the feature flag is set
> - we don't check if the hardware supports a PMU
> - kvm_pmu_vcpu_reset() relies on __vcpu_sys_reg(vcpu, PMCR_EL0), which 
> is set in
> kvm_reset_sys_regs() below when the VCPU is initialized.

I'm not sure it actually matters. Here's my rational:

- PMU support not compiled in: no problem!
- PMU support compiled in, but no HW PMU: we just reset some state to 0, 
no harm done
- HW PMU, but no KVM PMU for this vcpu: same thing
- HW PMU, and KVM PMU: we do the right thing!

Am I missing anything?

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

Re: [PATCH 3/8] KVM: arm64: Refuse illegal KVM_ARM_VCPU_PMU_V3 at reset time

[Alexandru Elisei]

Hi Marc,

On 11/26/20 3:25 PM, Marc Zyngier wrote:
> Hi Alex,
>
> On 2020-11-26 14:59, Alexandru Elisei wrote:
>> Hi Marc,
>>
>> On 11/13/20 6:25 PM, Marc Zyngier wrote:
>>> We accept to configure a PMU when a vcpu is created, even if the
>>> HW (or the host) doesn't support it. This results in failures
>>> when attributes get set, which is a bit odd as we should have
>>> failed the vcpu creation the first place.
>>>
>>> Move the check to the point where we check the vcpu feature set,
>>> and fail early if we cannot support a PMU. This further simplifies
>>> the attribute handling.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  arch/arm64/kvm/pmu-emul.c | 4 ++--
>>>  arch/arm64/kvm/reset.c    | 4 ++++
>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>>> index e7e3b4629864..200f2a0d8d17 100644
>>> --- a/arch/arm64/kvm/pmu-emul.c
>>> +++ b/arch/arm64/kvm/pmu-emul.c
>>> @@ -913,7 +913,7 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
>>>
>>>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>>>  {
>>> -    if (!kvm_arm_support_pmu_v3() || !kvm_vcpu_has_pmu(vcpu))
>>> +    if (!kvm_vcpu_has_pmu(vcpu))
>>>          return -ENODEV;
>>>
>>>      if (vcpu->arch.pmu.created)
>>> @@ -1034,7 +1034,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu,
>>> struct kvm_device_attr *attr)
>>>      case KVM_ARM_VCPU_PMU_V3_IRQ:
>>>      case KVM_ARM_VCPU_PMU_V3_INIT:
>>>      case KVM_ARM_VCPU_PMU_V3_FILTER:
>>> -        if (kvm_arm_support_pmu_v3() && kvm_vcpu_has_pmu(vcpu))
>>> +        if (kvm_vcpu_has_pmu(vcpu))
>>>              return 0;
>>>      }
>>>
>>> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
>>> index 74ce92a4988c..3e772ea4e066 100644
>>> --- a/arch/arm64/kvm/reset.c
>>> +++ b/arch/arm64/kvm/reset.c
>>> @@ -285,6 +285,10 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>>>              pstate = VCPU_RESET_PSTATE_EL1;
>>>          }
>>>
>>> +        if (kvm_vcpu_has_pmu(vcpu) && !kvm_arm_support_pmu_v3()) {
>>> +            ret = -EINVAL;
>>> +            goto out;
>>> +        }
>>
>> This looks correct, but right at the beginning of the function, before this
>> non-preemptible section, we do kvm_pmu_vcpu_reset(), which is wrong for several
>> reasons:
>>
>> - we don't check if the feature flag is set
>> - we don't check if the hardware supports a PMU
>> - kvm_pmu_vcpu_reset() relies on __vcpu_sys_reg(vcpu, PMCR_EL0), which is set in
>> kvm_reset_sys_regs() below when the VCPU is initialized.
>
> I'm not sure it actually matters. Here's my rational:
>
> - PMU support not compiled in: no problem!
> - PMU support compiled in, but no HW PMU: we just reset some state to 0, no harm
> done
> - HW PMU, but no KVM PMU for this vcpu: same thing
> - HW PMU, and KVM PMU: we do the right thing!
>
> Am I missing anything?

I don't think so, it also looks harmless to me. When it's called on the VCPU init
path, there will be no perf_events, so that part will be skipped. On the reset
path, PMCR_EL0.N will have been initialized so we end up with the correct number
of counters. In both cases vcpu->arch.pmu.chained will be zero'ed

But I find it strange to reset the PMU before doing any checks and before setting
the VCPU register value it reads.

I am thinking that even though at the moment it's harmless, in the future the
function might change and I don't think whoever modifies it will expect the
function to be called like this. But I guess if we're vigilant enough we can
prevent that hypothetical situation from happening.

Thanks,
Alex

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

[PATCH 4/8] KVM: arm64: Inject UNDEF on PMU access when no PMU configured

[Marc Zyngier]

The ARMv8 architecture says that in the absence of FEAT_PMUv3,
all the PMU-related register generate an UNDEF. Let's make
sure that all our PMU handers catch this case by hooking into
check_pmu_access_disabled(), and add checks in a couple of
other places.

Note that we still cannot deliver an exception into the guest
as the offending cases are already caught by the RAZ/WI handling.
But this puts us one step away to architectural compliance.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 6629cfde2838..b098d667bb42 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -609,8 +609,9 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 static bool check_pmu_access_disabled(struct kvm_vcpu *vcpu, u64 flags)
 {
 	u64 reg = __vcpu_sys_reg(vcpu, PMUSERENR_EL0);
-	bool enabled = (reg & flags) || vcpu_mode_priv(vcpu);
+	bool enabled = kvm_vcpu_has_pmu(vcpu);
 
+	enabled &= (reg & flags) || vcpu_mode_priv(vcpu);
 	if (!enabled)
 		kvm_inject_undefined(vcpu);
 
@@ -857,10 +858,8 @@ static bool access_pminten(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	if (!kvm_arm_pmu_v3_ready(vcpu))
 		return trap_raz_wi(vcpu, p, r);
 
-	if (!vcpu_mode_priv(vcpu)) {
-		kvm_inject_undefined(vcpu);
+	if (check_pmu_access_disabled(vcpu, 0))
 		return false;
-	}
 
 	if (p->is_write) {
 		u64 val = p->regval & mask;
@@ -928,6 +927,11 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	if (!kvm_arm_pmu_v3_ready(vcpu))
 		return trap_raz_wi(vcpu, p, r);
 
+	if (!kvm_vcpu_has_pmu(vcpu)) {
+		kvm_inject_undefined(vcpu);
+		return false;
+	}
+
 	if (p->is_write) {
 		if (!vcpu_mode_priv(vcpu)) {
 			kvm_inject_undefined(vcpu);
-- 
2.28.0


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

[PATCH 5/8] KVM: arm64: Remove PMU RAZ/WI handling

[Marc Zyngier]

There is no RAZ/WI handling allowed for the PMU registers in the
ARMv8 architecture. Nobody can remember how we cam to the conclusion
that we could do this, but the ARMv8 ARM is pretty clear that we cannot.

Remove the RAZ/WI handling of the PMU system registers when it is
not configured.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index b098d667bb42..3bd4cc40536b 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -643,9 +643,6 @@ static bool access_pmcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 {
 	u64 val;
 
-	if (!kvm_arm_pmu_v3_ready(vcpu))
-		return trap_raz_wi(vcpu, p, r);
-
 	if (pmu_access_el0_disabled(vcpu))
 		return false;
 
@@ -672,9 +669,6 @@ static bool access_pmcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 static bool access_pmselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 			  const struct sys_reg_desc *r)
 {
-	if (!kvm_arm_pmu_v3_ready(vcpu))
-		return trap_raz_wi(vcpu, p, r);
-
 	if (pmu_access_event_counter_el0_disabled(vcpu))
 		return false;
 
@@ -693,9 +687,6 @@ static bool access_pmceid(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 {
 	u64 pmceid;
 
-	if (!kvm_arm_pmu_v3_ready(vcpu))
-		return trap_raz_wi(vcpu, p, r);
-
 	BUG_ON(p->is_write);
 
 	if (pmu_access_el0_disabled(vcpu))
@@ -728,9 +719,6 @@ static bool access_pmu_evcntr(struct kvm_vcpu *vcpu,
 {
 	u64 idx;
 
-	if (!kvm_arm_pmu_v3_ready(vcpu))
-		return trap_raz_wi(vcpu, p, r);
-
 	if (r->CRn == 9 && r->CRm == 13) {
 		if (r->Op2 == 2) {
 			/* PMXEVCNTR_EL0 */
@@ -784,9 +772,6 @@ static bool access_pmu_evtyper(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 {
 	u64 idx, reg;
 
-	if (!kvm_arm_pmu_v3_ready(vcpu))
-		return trap_raz_wi(vcpu, p, r);
-
 	if (pmu_access_el0_disabled(vcpu))
 		return false;
 
@@ -824,9 +809,6 @@ static bool access_pmcnten(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 {
 	u64 val, mask;
 
-	if (!kvm_arm_pmu_v3_ready(vcpu))
-		return trap_raz_wi(vcpu, p, r);
-
 	if (pmu_access_el0_disabled(vcpu))
 		return false;
 
@@ -855,9 +837,6 @@ static bool access_pminten(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 {
 	u64 mask = kvm_pmu_valid_counter_mask(vcpu);
 
-	if (!kvm_arm_pmu_v3_ready(vcpu))
-		return trap_raz_wi(vcpu, p, r);
-
 	if (check_pmu_access_disabled(vcpu, 0))
 		return false;
 
@@ -882,9 +861,6 @@ static bool access_pmovs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 {
 	u64 mask = kvm_pmu_valid_counter_mask(vcpu);
 
-	if (!kvm_arm_pmu_v3_ready(vcpu))
-		return trap_raz_wi(vcpu, p, r);
-
 	if (pmu_access_el0_disabled(vcpu))
 		return false;
 
@@ -907,9 +883,6 @@ static bool access_pmswinc(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 {
 	u64 mask;
 
-	if (!kvm_arm_pmu_v3_ready(vcpu))
-		return trap_raz_wi(vcpu, p, r);
-
 	if (!p->is_write)
 		return read_from_write_only(vcpu, p, r);
 
@@ -924,9 +897,6 @@ static bool access_pmswinc(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 			     const struct sys_reg_desc *r)
 {
-	if (!kvm_arm_pmu_v3_ready(vcpu))
-		return trap_raz_wi(vcpu, p, r);
-
 	if (!kvm_vcpu_has_pmu(vcpu)) {
 		kvm_inject_undefined(vcpu);
 		return false;
-- 
2.28.0


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

Re: [PATCH 5/8] KVM: arm64: Remove PMU RAZ/WI handling

[Alexandru Elisei]

Hi Marc,

This patch looks correct to me, I checked in the Arm ARM DDI 0487F.b and indeed
all accesses to the PMU registers are UNDEFINED if the PMU is not present.

I checked all the accessors and now all the PMU registers that KVM emulates will
inject an undefined exception if the VCPU feature isn't set. There's one register
that we don't emulate, PMMIR_EL1, I suppose that's because it's part of PMU
ARMv8.4 and KVM advertises ARMv8.1; if the guest tries to access it, it will get
an undefined exception and KVM will print a warning in emulate_sys_reg().

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

On 11/13/20 6:25 PM, Marc Zyngier wrote:
> There is no RAZ/WI handling allowed for the PMU registers in the
> ARMv8 architecture. Nobody can remember how we cam to the conclusion
> that we could do this, but the ARMv8 ARM is pretty clear that we cannot.
>
> Remove the RAZ/WI handling of the PMU system registers when it is
> not configured.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/sys_regs.c | 30 ------------------------------
>  1 file changed, 30 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index b098d667bb42..3bd4cc40536b 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -643,9 +643,6 @@ static bool access_pmcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  {
>  	u64 val;
>  
> -	if (!kvm_arm_pmu_v3_ready(vcpu))
> -		return trap_raz_wi(vcpu, p, r);
> -
>  	if (pmu_access_el0_disabled(vcpu))
>  		return false;
>  
> @@ -672,9 +669,6 @@ static bool access_pmcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  static bool access_pmselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  			  const struct sys_reg_desc *r)
>  {
> -	if (!kvm_arm_pmu_v3_ready(vcpu))
> -		return trap_raz_wi(vcpu, p, r);
> -
>  	if (pmu_access_event_counter_el0_disabled(vcpu))
>  		return false;
>  
> @@ -693,9 +687,6 @@ static bool access_pmceid(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  {
>  	u64 pmceid;
>  
> -	if (!kvm_arm_pmu_v3_ready(vcpu))
> -		return trap_raz_wi(vcpu, p, r);
> -
>  	BUG_ON(p->is_write);
>  
>  	if (pmu_access_el0_disabled(vcpu))
> @@ -728,9 +719,6 @@ static bool access_pmu_evcntr(struct kvm_vcpu *vcpu,
>  {
>  	u64 idx;
>  
> -	if (!kvm_arm_pmu_v3_ready(vcpu))
> -		return trap_raz_wi(vcpu, p, r);
> -
>  	if (r->CRn == 9 && r->CRm == 13) {
>  		if (r->Op2 == 2) {
>  			/* PMXEVCNTR_EL0 */
> @@ -784,9 +772,6 @@ static bool access_pmu_evtyper(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  {
>  	u64 idx, reg;
>  
> -	if (!kvm_arm_pmu_v3_ready(vcpu))
> -		return trap_raz_wi(vcpu, p, r);
> -
>  	if (pmu_access_el0_disabled(vcpu))
>  		return false;
>  
> @@ -824,9 +809,6 @@ static bool access_pmcnten(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  {
>  	u64 val, mask;
>  
> -	if (!kvm_arm_pmu_v3_ready(vcpu))
> -		return trap_raz_wi(vcpu, p, r);
> -
>  	if (pmu_access_el0_disabled(vcpu))
>  		return false;
>  
> @@ -855,9 +837,6 @@ static bool access_pminten(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  {
>  	u64 mask = kvm_pmu_valid_counter_mask(vcpu);
>  
> -	if (!kvm_arm_pmu_v3_ready(vcpu))
> -		return trap_raz_wi(vcpu, p, r);
> -
>  	if (check_pmu_access_disabled(vcpu, 0))
>  		return false;
>  
> @@ -882,9 +861,6 @@ static bool access_pmovs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  {
>  	u64 mask = kvm_pmu_valid_counter_mask(vcpu);
>  
> -	if (!kvm_arm_pmu_v3_ready(vcpu))
> -		return trap_raz_wi(vcpu, p, r);
> -
>  	if (pmu_access_el0_disabled(vcpu))
>  		return false;
>  
> @@ -907,9 +883,6 @@ static bool access_pmswinc(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  {
>  	u64 mask;
>  
> -	if (!kvm_arm_pmu_v3_ready(vcpu))
> -		return trap_raz_wi(vcpu, p, r);
> -
>  	if (!p->is_write)
>  		return read_from_write_only(vcpu, p, r);
>  
> @@ -924,9 +897,6 @@ static bool access_pmswinc(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  			     const struct sys_reg_desc *r)
>  {
> -	if (!kvm_arm_pmu_v3_ready(vcpu))
> -		return trap_raz_wi(vcpu, p, r);
> -
>  	if (!kvm_vcpu_has_pmu(vcpu)) {
>  		kvm_inject_undefined(vcpu);
>  		return false;

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

Re: [PATCH 5/8] KVM: arm64: Remove PMU RAZ/WI handling

[Marc Zyngier]

On 2020-11-26 15:06, Alexandru Elisei wrote:
> Hi Marc,
> 
> This patch looks correct to me, I checked in the Arm ARM DDI 0487F.b 
> and indeed
> all accesses to the PMU registers are UNDEFINED if the PMU is not 
> present.
> 
> I checked all the accessors and now all the PMU registers that KVM 
> emulates will
> inject an undefined exception if the VCPU feature isn't set. There's
> one register
> that we don't emulate, PMMIR_EL1, I suppose that's because it's part of 
> PMU
> ARMv8.4 and KVM advertises ARMv8.1; if the guest tries to access it, it 
> will get
> an undefined exception and KVM will print a warning in 
> emulate_sys_reg().

Funny that. I wrote a patch for that a long while ago, and obviously
never did anything with it [1]... Actually, the whole series was 
silently
dropped. I guess I had other things to think about at the time!

Let me pick that up again.

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

Thanks!

         M.

[1] 
https://lore.kernel.org/kvmarm/20200216185324.32596-6-maz@kernel.org/
-- 
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

[PATCH 6/8] KVM: arm64: Remove dead PMU sysreg decoding code

[Marc Zyngier]

The handling of traps in access_pmu_evcntr() has a couple of
omminous "else return false;" statements that don't make any sense:
the decoding tree coverse all the registers that trap to this handler,
and returning false implies that we change PC, which we don't.

Get rid of what is evidently dead code.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 3bd4cc40536b..f878d71484d8 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -733,8 +733,6 @@ static bool access_pmu_evcntr(struct kvm_vcpu *vcpu,
 				return false;
 
 			idx = ARMV8_PMU_CYCLE_IDX;
-		} else {
-			return false;
 		}
 	} else if (r->CRn == 0 && r->CRm == 9) {
 		/* PMCCNTR */
@@ -748,8 +746,6 @@ static bool access_pmu_evcntr(struct kvm_vcpu *vcpu,
 			return false;
 
 		idx = ((r->CRm & 3) << 3) | (r->Op2 & 7);
-	} else {
-		return false;
 	}
 
 	if (!pmu_counter_idx_valid(vcpu, idx))
-- 
2.28.0


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

Re: [PATCH 6/8] KVM: arm64: Remove dead PMU sysreg decoding code

[Alexandru Elisei]

Hi Marc,

I checked and indeed the remaining cases cover all registers that use this accessor.

However, I'm a bit torn here. The warning that I got when trying to run a guest
with the PMU feature flag set, but not initialized (reported at [1]) was also not
supposed to ever be reached:

static u32 kvm_pmu_event_mask(struct kvm *kvm)
{
    switch (kvm->arch.pmuver) {
    case 1:            /* ARMv8.0 */
        return GENMASK(9, 0);
    case 4:            /* ARMv8.1 */
    case 5:            /* ARMv8.4 */
    case 6:            /* ARMv8.5 */
        return GENMASK(15, 0);
    default:        /* Shouldn't be here, just for sanity */
        WARN_ONCE(1, "Unknown PMU version %d\n", kvm->arch.pmuver);
        return 0;
    }
}

I realize it's not exactly the same thing and I'll leave it up to you if you want
to add a warning for the cases that should never happen. I'm fine either way:

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

[1] https://www.spinics.net/lists/arm-kernel/msg857927.html

On 11/13/20 6:26 PM, Marc Zyngier wrote:
> The handling of traps in access_pmu_evcntr() has a couple of
> omminous "else return false;" statements that don't make any sense:
> the decoding tree coverse all the registers that trap to this handler,
> and returning false implies that we change PC, which we don't.
>
> Get rid of what is evidently dead code.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/sys_regs.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 3bd4cc40536b..f878d71484d8 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -733,8 +733,6 @@ static bool access_pmu_evcntr(struct kvm_vcpu *vcpu,
>  				return false;
>  
>  			idx = ARMV8_PMU_CYCLE_IDX;
> -		} else {
> -			return false;
>  		}
>  	} else if (r->CRn == 0 && r->CRm == 9) {
>  		/* PMCCNTR */
> @@ -748,8 +746,6 @@ static bool access_pmu_evcntr(struct kvm_vcpu *vcpu,
>  			return false;
>  
>  		idx = ((r->CRm & 3) << 3) | (r->Op2 & 7);
> -	} else {
> -		return false;
>  	}
>  
>  	if (!pmu_counter_idx_valid(vcpu, idx))

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

Re: [PATCH 6/8] KVM: arm64: Remove dead PMU sysreg decoding code

[Marc Zyngier]

Hi Alex,

On 2020-11-26 15:18, Alexandru Elisei wrote:
> Hi Marc,
> 
> I checked and indeed the remaining cases cover all registers that use
> this accessor.
> 
> However, I'm a bit torn here. The warning that I got when trying to run 
> a guest
> with the PMU feature flag set, but not initialized (reported at [1])
> was also not
> supposed to ever be reached:
> 
> static u32 kvm_pmu_event_mask(struct kvm *kvm)
> {
>     switch (kvm->arch.pmuver) {
>     case 1:            /* ARMv8.0 */
>         return GENMASK(9, 0);
>     case 4:            /* ARMv8.1 */
>     case 5:            /* ARMv8.4 */
>     case 6:            /* ARMv8.5 */
>         return GENMASK(15, 0);
>     default:        /* Shouldn't be here, just for sanity */
>         WARN_ONCE(1, "Unknown PMU version %d\n", kvm->arch.pmuver);
>         return 0;
>     }
> }
> 
> I realize it's not exactly the same thing and I'll leave it up to you
> if you want
> to add a warning for the cases that should never happen. I'm fine 
> either way:

I already have queued such a warning[1]. It turns out that LLVM warns
idx can be left uninitialized, and shouts. Let me know if that works
for you.

Thanks,

         M.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=kvm-arm64/pmu-undef&id=af7eff70eaf8f28179334f5aeabb70a306242c83
-- 
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

Re: [PATCH 6/8] KVM: arm64: Remove dead PMU sysreg decoding code

[Alexandru Elisei]

Hi Marc,

On 11/26/20 3:34 PM, Marc Zyngier wrote:
> Hi Alex,
>
> On 2020-11-26 15:18, Alexandru Elisei wrote:
>> Hi Marc,
>>
>> I checked and indeed the remaining cases cover all registers that use
>> this accessor.
>>
>> However, I'm a bit torn here. The warning that I got when trying to run a guest
>> with the PMU feature flag set, but not initialized (reported at [1])
>> was also not
>> supposed to ever be reached:
>>
>> static u32 kvm_pmu_event_mask(struct kvm *kvm)
>> {
>>     switch (kvm->arch.pmuver) {
>>     case 1:            /* ARMv8.0 */
>>         return GENMASK(9, 0);
>>     case 4:            /* ARMv8.1 */
>>     case 5:            /* ARMv8.4 */
>>     case 6:            /* ARMv8.5 */
>>         return GENMASK(15, 0);
>>     default:        /* Shouldn't be here, just for sanity */
>>         WARN_ONCE(1, "Unknown PMU version %d\n", kvm->arch.pmuver);
>>         return 0;
>>     }
>> }
>>
>> I realize it's not exactly the same thing and I'll leave it up to you
>> if you want
>> to add a warning for the cases that should never happen. I'm fine either way:
>
> I already have queued such a warning[1]. It turns out that LLVM warns
> idx can be left uninitialized, and shouts. Let me know if that works
> for you.

Looks good to me, unsigned long is 64 bits and instructions are 32 bits, so we'll
never run into a situation where a valid encoding is ~0UL.

You can add my Reviewed-by to this patch (and to the one at [1] if it's still
possible).

Thanks,

Alex

>
> Thanks,
>
>         M.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=kvm-arm64/pmu-undef&id=af7eff70eaf8f28179334f5aeabb70a306242c83

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

Re: [PATCH 6/8] KVM: arm64: Remove dead PMU sysreg decoding code

[Marc Zyngier]

On 2020-11-26 15:54, Alexandru Elisei wrote:
> Hi Marc,
> 
> On 11/26/20 3:34 PM, Marc Zyngier wrote:
>> Hi Alex,
>> 
>> On 2020-11-26 15:18, Alexandru Elisei wrote:
>>> Hi Marc,
>>> 
>>> I checked and indeed the remaining cases cover all registers that use
>>> this accessor.
>>> 
>>> However, I'm a bit torn here. The warning that I got when trying to 
>>> run a guest
>>> with the PMU feature flag set, but not initialized (reported at [1])
>>> was also not
>>> supposed to ever be reached:
>>> 
>>> static u32 kvm_pmu_event_mask(struct kvm *kvm)
>>> {
>>>     switch (kvm->arch.pmuver) {
>>>     case 1:            /* ARMv8.0 */
>>>         return GENMASK(9, 0);
>>>     case 4:            /* ARMv8.1 */
>>>     case 5:            /* ARMv8.4 */
>>>     case 6:            /* ARMv8.5 */
>>>         return GENMASK(15, 0);
>>>     default:        /* Shouldn't be here, just for sanity */
>>>         WARN_ONCE(1, "Unknown PMU version %d\n", kvm->arch.pmuver);
>>>         return 0;
>>>     }
>>> }
>>> 
>>> I realize it's not exactly the same thing and I'll leave it up to you
>>> if you want
>>> to add a warning for the cases that should never happen. I'm fine 
>>> either way:
>> 
>> I already have queued such a warning[1]. It turns out that LLVM warns
>> idx can be left uninitialized, and shouts. Let me know if that works
>> for you.
> 
> Looks good to me, unsigned long is 64 bits and instructions are 32
> bits, so we'll never run into a situation where a valid encoding is 
> ~0UL.
> 
> You can add my Reviewed-by to this patch (and to the one at [1] if it's 
> still
> possible).

It's a fixup, so it will get folded into the original patch.

Thanks for spending time reviewing (and fixing) this!

       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

[PATCH 7/8] KVM: arm64: Gate kvm_pmu_update_state() on the PMU feature

[Marc Zyngier]

We currently gate the update of the PMU state on the PMU being "ready".
The "ready" state is only set to true when the first vcpu run is
successful, and if it isn't, we never reach the update code.

So the "ready" state is never the right thing to check for, and it
should instead be the presence of the PMU feature, which makes
a bit more sense.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/pmu-emul.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 200f2a0d8d17..8806fdc85e8a 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -384,7 +384,7 @@ static void kvm_pmu_update_state(struct kvm_vcpu *vcpu)
 	struct kvm_pmu *pmu = &vcpu->arch.pmu;
 	bool overflow;
 
-	if (!kvm_arm_pmu_v3_ready(vcpu))
+	if (!kvm_vcpu_has_pmu(vcpu))
 		return;
 
 	overflow = !!kvm_pmu_overflow_status(vcpu);
-- 
2.28.0


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

[PATCH 8/8] KVM: arm64: Get rid of the PMU ready state

[Marc Zyngier]

The PMU ready state has no user left. Goodbye.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/pmu-emul.c | 1 -
 include/kvm/arm_pmu.h     | 3 ---
 2 files changed, 4 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 8806fdc85e8a..643cf819f3c0 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -851,7 +851,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
 	}
 
 	kvm_pmu_vcpu_reset(vcpu);
-	vcpu->arch.pmu.ready = true;
 
 	return 0;
 }
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 1d94acd0bc85..fc85f50fa0e9 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -24,13 +24,11 @@ struct kvm_pmu {
 	int irq_num;
 	struct kvm_pmc pmc[ARMV8_PMU_MAX_COUNTERS];
 	DECLARE_BITMAP(chained, ARMV8_PMU_MAX_COUNTER_PAIRS);
-	bool ready;
 	bool created;
 	bool irq_level;
 	struct irq_work overflow_work;
 };
 
-#define kvm_arm_pmu_v3_ready(v)		((v)->arch.pmu.ready)
 #define kvm_arm_pmu_irq_initialized(v)	((v)->arch.pmu.irq_num >= VGIC_NR_SGIS)
 u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx);
 void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val);
@@ -61,7 +59,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu);
 struct kvm_pmu {
 };
 
-#define kvm_arm_pmu_v3_ready(v)		(false)
 #define kvm_arm_pmu_irq_initialized(v)	(false)
 static inline u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu,
 					    u64 select_idx)
-- 
2.28.0


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

Re: [PATCH 0/8] KVM: arm64: Disabled PMU handling

[Alexandru Elisei]

Hi Marc,

I believe there is something missing from this series.

The original behaviour, which this series changes, was not to do register
emulation and PMU state update if the PMU wasn't ready, where vcpu->arch.pmu.ready
was set to true if the PMU was initialized properly in kvm_vcpu_first_run_init()
-> kvm_arm_pmu_v3_enable().

The series changes PMU emulation such that register emulation and pmu state update
is gated only on the VCPU feature being set. This means that now userspace can set
the VCPU feature, don't do any initialization, and run a guest which can access
PMU registers. Also kvm_pmu_update_state() will now be called before each VM
entry. I'm not exactly sure what happens if we call kvm_vgic_inject_irq() for an
irq_num = 0 and not owned by the PMU (the owner is set KVM_ARM_VCPU_PMU_V3_INIT ->
kvm_arm_pmu_v3_init()), but I don't think that's allowed.

I was also able to trigger this warning with a modified version of kvmtool:

[  118.972174] ------------[ cut here ]------------
[  118.974212] Unknown PMU version 0
[  118.977622] WARNING: CPU: 0 PID: 238 at arch/arm64/kvm/pmu-emul.c:33
kvm_pmu_event_mask.isra.0+0x6c/0x74
[  118.987271] Modules linked in:
[  118.990414] CPU: 0 PID: 238 Comm: kvm-vcpu-0 Not tainted
5.10.0-rc4-00008-gc4cd5186fc2a #37
[  118.999006] Hardware name: Globalscale Marvell ESPRESSOBin Board (DT)
[  119.005641] pstate: 40000005 (nZcv daif -PAN -UAO -TCO BTYPE=--)
[  119.011825] pc : kvm_pmu_event_mask.isra.0+0x6c/0x74
[  119.016929] lr : kvm_pmu_event_mask.isra.0+0x6c/0x74
[  119.022034] sp : ffff80001274ba40
[  119.025438] x29: ffff80001274ba40 x28: ffff0000091c46a0
[  119.030903] x27: 0000000000000000 x26: ffff800011e16ee0
[  119.036368] x25: ffff800011e166c8 x24: ffff000006e00000
[  119.041834] x23: 0000000000000000 x22: ffff80001274bb20
[  119.047300] x21: 0000000000000000 x20: 000000000000001f
[  119.052765] x19: 0000000000000000 x18: 0000000000000030
[  119.058231] x17: 0000000000000000 x16: 0000000000000000
[  119.063697] x15: ffff0000022caf30 x14: ffffffffffffffff
[  119.069163] x13: ffff800011b72718 x12: 0000000000000456
[  119.074627] x11: 0000000000000172 x10: ffff800011bca718
[  119.080094] x9 : 00000000fffff000 x8 : ffff800011b72718
[  119.085559] x7 : ffff800011bca718 x6 : 0000000000000000
[  119.091025] x5 : 0000000000000000 x4 : ffff00003ddbe900
[  119.096491] x3 : ffff00003ddc57f0 x2 : ffff00003ddbe900
[  119.101956] x1 : 34d0d4b3321b9100 x0 : 0000000000000000
[  119.107422] Call trace:
[  119.109935]  kvm_pmu_event_mask.isra.0+0x6c/0x74
[  119.114684]  kvm_pmu_set_counter_event_type+0x2c/0x80
[  119.119882]  access_pmu_evtyper+0x128/0x180
[  119.124181]  perform_access+0x34/0xb0
[  119.127942]  kvm_handle_sys_reg+0xc8/0x160
[  119.132156]  handle_exit+0x6c/0x1f0
[  119.135738]  kvm_arch_vcpu_ioctl_run+0x1e8/0x73c
[  119.140488]  kvm_vcpu_ioctl+0x23c/0x544
[  119.144433]  __arm64_sys_ioctl+0xa8/0xf0
[  119.148464]  el0_svc_common.constprop.0+0x78/0x1a0
[  119.153390]  do_el0_svc+0x24/0x90
[  119.156796]  el0_sync_handler+0x254/0x260
[  119.160915]  el0_sync+0x174/0x180
[  119.164319] ---[ end trace c0c2e6f299d58823 ]---

I removed all KVM_ARM_VCPU_PMU_V3_CTRL ioctl calls from kvmtool's pmu emulation,
and I started the pmu test from kvm-unit-tests:

$ ./lkvm-pmu run -c1 -m64 -f arm/pmu.flat --pmu -p cycle-counter

The reason for the warning is that the correct value for kvm->arch.pmuver is set
in kvm_arm_pmu_v3_set_attr(), which is not called anymore.

This diff seems to solve the issue:

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 643cf819f3c0..150b9cb0f741 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -825,9 +825,12 @@ bool kvm_arm_support_pmu_v3(void)
 
 int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
 {
-       if (!vcpu->arch.pmu.created)
+       if (!kvm_vcpu_has_pmu(vcpu))
                return 0;
 
+       if (!vcpu->arch.pmu.created)
+               return -ENOEXEC;
+
        /*
         * A valid interrupt configuration for the PMU is either to have a
         * properly configured interrupt number and using an in-kernel

If you agree with the fix, I can send a proper patch. vcpu->arch.pmu.created is
set in kvm_arm_pmu_v3_init(), which checks if the interrupt ID has been set. I
chose to return -ENOEXEC  because that's what KVM_RUN returns if the vcpu isn't
initialized in kvm_arch_vcpu_ioctl_run().

Thanks,

Alex

On 11/13/20 6:25 PM, Marc Zyngier wrote:
> It recently dawned on me that the way we handle PMU traps when the PMU
> is disabled is plain wrong. We consider that handling the registers as
> RAZ/WI is a fine thing to do, while the ARMv8 ARM is pretty clear that
> that's not OK and that such registers should UNDEF when FEAT_PMUv3
> doesn't exist. I went all the way back to the first public version of
> the spec, and it turns out we were *always* wrong.
>
> It probably comes from the fact that we used not to trap the ID
> registers, and thus were unable to advertise the lack of PMU, but
> that's hardly an excuse. So let's fix the damned thing.
>
> This series adds an extra check in the helpers that check for the
> validity of the PMU access (most of the registers have to checked
> against some enable flags and/or the accessing exception level), and
> rids us of the RAZ/WI behaviour.
>
> This enables us to make additional cleanups, to the point where we can
> remove the PMU "ready" state that always had very bizarre semantics.
> All in all, a negative diffstat, and spec compliant behaviours. What's
> not to like?
>
> I've run a few guests with and without PMUs as well as KUT, and
> nothing caught fire. The patches are on top of kvmarm/queue.
>
> Marc Zyngier (8):
>   KVM: arm64: Add kvm_vcpu_has_pmu() helper
>   KVM: arm64: Set ID_AA64DFR0_EL1.PMUVer to 0 when no PMU support
>   KVM: arm64: Refuse illegal KVM_ARM_VCPU_PMU_V3 at reset time
>   KVM: arm64: Inject UNDEF on PMU access when no PMU configured
>   KVM: arm64: Remove PMU RAZ/WI handling
>   KVM: arm64: Remove dead PMU sysreg decoding code
>   KVM: arm64: Gate kvm_pmu_update_state() on the PMU feature
>   KVM: arm64: Get rid of the PMU ready state
>
>  arch/arm64/include/asm/kvm_host.h |  3 ++
>  arch/arm64/kvm/pmu-emul.c         | 11 +++----
>  arch/arm64/kvm/reset.c            |  4 +++
>  arch/arm64/kvm/sys_regs.c         | 51 ++++++++-----------------------
>  include/kvm/arm_pmu.h             |  3 --
>  5 files changed, 24 insertions(+), 48 deletions(-)
>

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

Re: [PATCH 0/8] KVM: arm64: Disabled PMU handling

[Marc Zyngier]

On Tue, 24 Nov 2020 17:28:30 +0000,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi Marc,
> 
> I believe there is something missing from this series.
> 
> The original behaviour, which this series changes, was not to do
> register emulation and PMU state update if the PMU wasn't ready,
> where vcpu->arch.pmu.ready was set to true if the PMU was
> initialized properly in kvm_vcpu_first_run_init() ->
> kvm_arm_pmu_v3_enable().
> 
> The series changes PMU emulation such that register emulation and
> pmu state update is gated only on the VCPU feature being set. This
> means that now userspace can set the VCPU feature, don't do any
> initialization, and run a guest which can access PMU registers. Also
> kvm_pmu_update_state() will now be called before each VM entry. I'm
> not exactly sure what happens if we call kvm_vgic_inject_irq() for
> an irq_num = 0 and not owned by the PMU (the owner is set
> KVM_ARM_VCPU_PMU_V3_INIT -> kvm_arm_pmu_v3_init()), but I don't
> think that's allowed.

That's a very good point. I dropped the "ready" state a bit
carelessly, and nothing guards a half baked PMU anymore.

> 
> I was also able to trigger this warning with a modified version of kvmtool:

[ugly warning]

> I removed all KVM_ARM_VCPU_PMU_V3_CTRL ioctl calls from kvmtool's
> pmu emulation, and I started the pmu test from kvm-unit-tests:
> 
> $ ./lkvm-pmu run -c1 -m64 -f arm/pmu.flat --pmu -p cycle-counter
> 
> The reason for the warning is that the correct value for
> kvm->arch.pmuver is set in kvm_arm_pmu_v3_set_attr(), which is not
> called anymore.
> 
> This diff seems to solve the issue:
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 643cf819f3c0..150b9cb0f741 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -825,9 +825,12 @@ bool kvm_arm_support_pmu_v3(void)
>  
>  int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>  {
> -       if (!vcpu->arch.pmu.created)
> +       if (!kvm_vcpu_has_pmu(vcpu))
>                 return 0;
>  
> +       if (!vcpu->arch.pmu.created)
> +               return -ENOEXEC;
> +
>         /*
>          * A valid interrupt configuration for the PMU is either to have a
>          * properly configured interrupt number and using an in-kernel
> 
> If you agree with the fix, I can send a proper patch.
> vcpu->arch.pmu.created is set in kvm_arm_pmu_v3_init(), which checks
> if the interrupt ID has been set. I chose to return -ENOEXEC 
> because that's what KVM_RUN returns if the vcpu isn't initialized in
> kvm_arch_vcpu_ioctl_run().

Yes, this seems reasonable. The first run will fail, as for an
uninitialised vcpu.

Whist you're doing that, can you please document the ENOEXEC return
value? We only document EINTR so far.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

Re: [PATCH 0/8] KVM: arm64: Disabled PMU handling

[Alexandru Elisei]

Hi Marc,

On 11/13/20 6:25 PM, Marc Zyngier wrote:
> It recently dawned on me that the way we handle PMU traps when the PMU
> is disabled is plain wrong. We consider that handling the registers as
> RAZ/WI is a fine thing to do, while the ARMv8 ARM is pretty clear that
> that's not OK and that such registers should UNDEF when FEAT_PMUv3
> doesn't exist. I went all the way back to the first public version of
> the spec, and it turns out we were *always* wrong.
>
> It probably comes from the fact that we used not to trap the ID
> registers, and thus were unable to advertise the lack of PMU, but
> that's hardly an excuse. So let's fix the damned thing.
>
> This series adds an extra check in the helpers that check for the
> validity of the PMU access (most of the registers have to checked
> against some enable flags and/or the accessing exception level), and
> rids us of the RAZ/WI behaviour.
>
> This enables us to make additional cleanups, to the point where we can
> remove the PMU "ready" state that always had very bizarre semantics.
> All in all, a negative diffstat, and spec compliant behaviours. What's
> not to like?
>
> I've run a few guests with and without PMUs as well as KUT, and
> nothing caught fire. The patches are on top of kvmarm/queue.
Everything looks fine to me. You can add my Reviewed-by tag the patches I haven't
commented on separately.

Thanks,

Alex

>
> Marc Zyngier (8):
>   KVM: arm64: Add kvm_vcpu_has_pmu() helper
>   KVM: arm64: Set ID_AA64DFR0_EL1.PMUVer to 0 when no PMU support
>   KVM: arm64: Refuse illegal KVM_ARM_VCPU_PMU_V3 at reset time
>   KVM: arm64: Inject UNDEF on PMU access when no PMU configured
>   KVM: arm64: Remove PMU RAZ/WI handling
>   KVM: arm64: Remove dead PMU sysreg decoding code
>   KVM: arm64: Gate kvm_pmu_update_state() on the PMU feature
>   KVM: arm64: Get rid of the PMU ready state
>
>  arch/arm64/include/asm/kvm_host.h |  3 ++
>  arch/arm64/kvm/pmu-emul.c         | 11 +++----
>  arch/arm64/kvm/reset.c            |  4 +++
>  arch/arm64/kvm/sys_regs.c         | 51 ++++++++-----------------------
>  include/kvm/arm_pmu.h             |  3 --
>  5 files changed, 24 insertions(+), 48 deletions(-)
>

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