| Message ID | 20201113182602.471776-7-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Disabled PMU handling | expand |
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))
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
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
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.
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))
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(-)