Re: [PATCH v6 01/25] KVM: arm64: Introduce a validation function for an ID register

[Oliver Upton <oupton@google.com>]

On Tue, Mar 22, 2022 at 11:06:26PM -0700, Reiji Watanabe wrote:
> Hi Oliver,
> 
> > On Thu, Mar 10, 2022 at 08:47:47PM -0800, Reiji Watanabe wrote:
> > > Introduce arm64_check_features(), which does a basic validity checking
> > > of an ID register value against the register's limit value, which is
> > > generally the host's sanitized value.
> > >
> > > This function will be used by the following patches to check if an ID
> > > register value that userspace tries to set for a guest can be supported
> > > on the host.
> > >
> > > The validation is done using arm64_ftr_bits_kvm, which is created from
> > > arm64_ftr_regs, with some entries overwritten by entries from
> > > arm64_ftr_bits_kvm_override.
> > >
> > > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> >
> > I have some concerns regarding the API between cpufeature and KVM that's
> > being proposed here. It would appear that we are adding some of KVM's
> > implementation details into the cpufeature code. In particular, I see
> > that KVM's limitations on AA64DFR0 are being copied here.
> 
> I assume "KVM's limitation details" you meant is about
> ftr_id_aa64dfr0_kvm.
> Entries in arm64_ftr_bits_kvm_override shouldn't be added based
> on KVM's implementation.  When cpufeature.c doesn't handle lower level
> of (or fewer) features as the "safe" value for fields, the field should
> be added to arm64_ftr_bits_kvm_override.  As PMUVER and DEBUGVER are not
> treated as LOWER_SAFE, they were added in arm64_ftr_bits_kvm_override.

I believe the fact that KVM is more permissive on PMUVER and DEBUGVER
than cpufeature is in fact a detail of KVM, no? read_id_reg() already
implicitly trusts the cpufeature code filtering and applies additional
limitations on top of what we get back. Similarly, there are fields
where KVM is more restrictive than cpufeature (ID_AA64DFR0_PMSVER).

Each of those constraints could theoretically be expressed as an
arm64_ftr_bits structure within KVM.

> Having said that, although ftr_id_aa64dfr0 has PMUVER as signed field,
> I didn't fix that in ftr_id_aa64dfr0_kvm, and the code abused that....
> I should make PMUVER unsigned field, and fix cpufeature.c to validate
> the field based on its special ID scheme for cleaner abstraction.

Good point. AA64DFR0 is an annoying register :)

> (And KVM should skip the cpufeature.c's PMUVER validation using
>  id_reg_desc's ignore_mask and have KVM validate it locally based on
>  the KVM's special requirement)
> 
> 
> > Additionally, I think it would be preferable to expose this in a manner
> > that does not require CONFIG_KVM guards in other parts of the kernel.
> >
> > WDYT about having KVM keep its set of feature overrides and otherwise
> > rely on the kernel's feature tables? I messed around with the idea a
> > bit until I could get something workable (attached). I only compile
> > tested this :)
> 
> Thanks for the proposal!
> But, providing the overrides to arm64_ftr_reg_valid() means that its
> consumer knows implementation details of cpufeture.c (values of entries
> in arm64_ftr_regs[]), which is a similar abstraction problem I want to
> avoid (the default validation by cpufeature.c should be purely based on
> ID schemes even with this option).

It is certainly a matter of where you choose to draw those lines. We already
do bank on the implementation details of cpufeature.c quite heavily, its
just stuffed away behind read_sanitised_ftr_reg(). Now we have KVM bits and
pieces in cpufeature.c and might wind up forcing others to clean up our dirty
laundry in the future.

It also seems to me that if I wanted to raise the permitted DEBUGVER for KVM,
would I have to make a change outside of KVM.

> Another option that I considered earlier was having a full set of
> arm64_ftr_bits in KVM for its validation. At the time, I thought
> statically) having a full set of arm64_ftr_bits in KVM is not good in
> terms of maintenance.  But, considering that again, since most of
> fields are unsigned and lower safe fields, and KVM doesn't necessarily
> have to statically have a full set of arm64_ftr_bits

I think the argument could be made for KVM having its own static +
verbose cpufeature tables. We've already been bitten by scenarios where
cpufeature exposes a feature that we simply do not virtualize in KVM.
That really can become a game of whack-a-mole. commit 96f4f6809bee
("KVM: arm64: Don't advertise FEAT_SPE to guests") is a good example,
and I can really see no end to these sorts of issues without an
overhaul. We'd need to also find a way to leverage the existing
infrasturcture for working out a system-wide safe value, but this time
with KVM's table of registers.

KVM would then need to take a change to expose any new feature that has
no involvement of EL2. Personally, I'd take that over the possibility of
another unhandled feature slipping through and blowing up a guest kernel
when running on newer hardware.

> (dynamically generate during KVM's initialization)

This was another one of my concerns with the current state of this
patch. I found the register table construction at runtime hard to
follow. I think it could be avoided with a helper that has a prescribed
set of rules (caller-provided field definition takes precedence over the
general one).

Sorry it took me a bit to comment on everything, needed to really sit
down and wrap my head around the series.

--
Thanks,
Oliver