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

[Oliver Upton <oupton@google.com>]

Hi Reiji,

On Wed, Mar 23, 2022 at 11:00:25PM -0700, Reiji Watanabe wrote:
> Hi Oliver,
> 
> > > > 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
> 
> What cpufeature knows is that consumers of the validation function
> needs the validation of each field based on ID register schemes that
> are described in Arm ARM (basically lower safe).
> As lower values of PMUVER/DEBUGVER indicates lower level of features
> or fewer level of features, those entries are to provide validation
> based on that.  So, entries in arm64_ftr_bits_kvm_override will be added
> to adjust cpufeture's behavior based on ID register schemes, and KVM may
> or may not use them.
> 
> I need to remove the word "kvm" from variable/function/structure names
> and put more clear comments:)

I'll admit I definitely drilled down on the fact that KVM is the only
actual user of these, and not the fact that it was realigning the fields
with the Arm ARM :)

> > 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.
> 
> It's not impossible but it's a bit tricky (With __arm64_ftr_reg_valid(),
> it might look straight forward, but I don't think that treats FTR_EXACT
> correctly. Please see update_cpu_ftr_reg).
>

Ah right. __arm64_ftr_reg_valid() needs to trust either the value that
comes from the boot CPU, or ->safe_val if the cores are different in the
system. And what does it mean if the caller specified FTR_EXACT?

I'll think on this more if I have any other suggestions.

[...]

> > 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.
> 
> Could you elaborate this a little more?

Urgh. Ignore me, I fixated to heavily on the SAFE_VAL you used for
DEBUGVER, not the fact that it was LOWER_SAFE.

> More specific concern I have about providing the override (with the
> existing arm64_ftr_bits) would be when field values of arm64_ftr_bits
> (i.e. LOWER_SAFE to EXACT) in cpufeature are changed due to kernel's
> implementation reasons, which might affect KVM (may need to pass
> extra override to arm64_ftr_reg_valid).
> But, by having cpufeature provide the validation based on the ID
> register schemes, cpufeature should be changed to provide the same
> validation in that case (i.e. if DFR0.PERFMON is changed from LOWER_SAFE
> to EXACT like AA64DFR0.PMUVER, DFR0.PERFMON should be added in
> arm64_ftr_bits_kvm_override with LOWER_SAFE).
> 
> So, if I go with the option to provide override to cpufeature, IMHO it
> would be preferable for cpufeature to provide the validation based
> on ID schemes instead of with the current need-based policy (, which
> might get changed) for clear separation.

Sounds good. Per your suggestion above, changing the
naming/documentation around what is being added to cpufeature removes
the confusion that it relates to KVM and really is a precise
implementation of the rules in the Arm ARM.

> > > 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
> 
> What does "verbose cpufeature tables" mean ?

Currently KVM implements a sparsely-defined denylist on top of whatever
we get back from read_sanitised_ftr_reg(). We do not have an absolute
upper bound for all fields in the feature registers, so there are times
where unsupported features leak through to the guest much like the SPE
commit I mentioned below.

What I am suggesting is that KVM define an absolute limit on what it
virtualizes for *all* fields, including what is presently RAZ. We have
absolutely no idea whether or not we can virtualize new features that
come in later revisions of the spec. It does mean we will need to
raise those limits from time to time, but would rather do that than
accidentally expose a feature we cannot virtualize.

> > 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.
> 
> Userspace with configurable ID registers would eliminate such problems
> on known systems, but I agree that KVM itself should prevent it.
> It will be inconvenient for some people, but it would be safer in general.

We cannot require userspace to write to these registers to run a guest
given the fact that the present ABI doesn't. Given that fact, KVM is
still responsible for having sane default values for these registers.

If a field that we do not handle implies a feature we do not virtualize
on newer hardware, invariably our guest will see it and likely panic
when it realizes the vCPU is out of spec.

Maybe the feature bits tables is a bit extreme given the fact that it
does define the architected handling of each field. I think the upper
bound on register values I mentioned above would do the trick and avoid
copy/pasting a whole set of structures we don't desperately need.

> > > (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).
> 
> Sure, I will improve that if I continue to keep the current way.
> With the option of having a separate KVM's arm64_ftr_bits,
> the code will be very different, but I will keep that in mind.

arm64_ftr_bits might be a bit extreme in KVM after all, I'll retract
that suggestion in favor of what I said above :)

Thanks for being patient working through all of this with me.

--
Best,
Oliver

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