Re: [PATCH] KVM/arm64: Don't emulate a PMU for 32-bit guests if feature not set

From: Oliver Upton <oupton@google.com>
To: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: maz@kernel.org, kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] KVM/arm64: Don't emulate a PMU for 32-bit guests if feature not set
Date: Wed, 27 Apr 2022 00:57:57 -0700	[thread overview]
Message-ID: <CAOQ_Qsj47JkEn-sOhKCAKyZnetsKqpWvBALnfAzyMfMy=eqOHg@mail.gmail.com> (raw)
In-Reply-To: <Yme02Tw3WdbXBAR1@monolith.localdoman>

Hi Alex,

On Tue, Apr 26, 2022 at 2:01 AM Alexandru Elisei
<alexandru.elisei@arm.com> wrote:
> > > The odroid-c4 is somewhat of a special case, because Linux doesn't probe
> > > the PMU. But the above errors can easily be reproduced on any hardware,
> > > with or without a PMU driver, as long as userspace doesn't set the PMU
> > > feature.
> >
> > This note has me wondering if we could do more negative testing with
> > kvm-unit-tests just by selectively turning on/off features, with the
> > expectation that tests either skip or pass.
>
> I'm not sure that that can be accomplished right now. kvm-unit-tests
> supports only qemu as an automated test runner, and qemu enables the PMU by
> default. I don't know if it can be disabled, it would be nice if it could.
> I stumbled upon this by mistake, when I ran kvmtool without enabling the
> PMU (the default in kvmtool is to not have it enabled).
>
> If it is possible to disable PMU emulation from the qemu's command line,
> then it should be as simple as writing a test that expects all PMU register
> accesses to trigger an undefined exception (and adding a new test
> definition).

You can disable the PMU with QEMU by specifying pmu=off in the -cpu
argument, among other things.

> If it is not possible to do this with qemu, then we would have to wait
> until kvm-unit-tests can use kvmtool as the test runner. I have an RFC sent
> for that [1], I need to get back to it.
>
> Another option would be to have this as a kselftest, although I don't know
> how easy it is to register an exception handler in a kselftest. The test
> could be further expanded to other registers gated by a VCPU feature being
> set.

We definitely have the plumbing for exception handlers in selftests,
aarch64/debug-exceptions.c is an example. My thought was more general
+ rather lazy. For any combination of CPU features, expect that
kvm-unit-tests should either pass or skip. If they fail or blow up the
host then probably a good indicator of a KVM bug.

> [1] https://lore.kernel.org/kvmarm/20210702163122.96110-1-alexandru.elisei@arm.com/

Thanks for the link, I'll have a peek.

> >
> > > Work around the fact that KVM advertises a PMU even when the VCPU feature
> > > is not set by gating all PMU emulation on the feature. The guest can still
> > > access the registers without KVM injecting an undefined exception.
> >
> > We're going to need something similar even after KVM conditionally
> > advertises the PMU.
> >
> > WDYT about wiring up sys_reg_desc::visibility for the AArch32 PMU
> > registers? For now just treat them as REG_RAZ (probably extend this to
> > imply WI too) then promote to REG_HIDDEN in a later patch.
>
> I was thinking you can simply use .visibility = pmu_visibility, like it's
> done with the PMU_SYS_REG macro:

Right -- I completely agree this is where we should be when AArch32
feature registers are trapped.

Seems to me all the AArch32 feature register trap logic should come
later on as there's a nonzero chance I introduced a bug :) Shall we
stop the bleeding w/ your originally proposed patch? Doesn't seem any
more objectionable than what we're already doing.

[...]

> I've renamed AA32_ZEROHIGH -> AA32_DIRECT. Feel free to use the snippet as
> you see fit (or not at all).

To avoid shamelessly plagiarizing: may I package up what you have
below as a commit coming from you?

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