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@xxxxxxx>
[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@xxxxxxxxxx>
> ---
> 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))
