Hi Oliver,
On Mon, Apr 4, 2022 at 4:19 PM Oliver Upton <oupton@xxxxxxxxxx> wrote:
>
> On Mon, Apr 04, 2022 at 05:28:33AM +0000, Oliver Upton wrote:
> > Hi Reiji,
> >
> > On Sun, Apr 03, 2022 at 08:57:47PM -0700, Reiji Watanabe wrote:
> > > > +int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
> > > > +{
> > > > + int Rt = kvm_vcpu_sys_get_rt(vcpu);
> > > > + u32 esr = kvm_vcpu_get_esr(vcpu);
> > > > + struct sys_reg_params params;
> > > > + int ret;
> > > > +
> > > > + /* UNDEF on any unhandled register or an attempted write */
> > > > + if (!kvm_esr_cp10_id_to_sys64(esr, ¶ms) || params.is_write) {
> > > > + kvm_inject_undefined(vcpu);
> > >
> > > Nit: For debugging, it might be more useful to use unhandled_cp_access()
> > > (, which needs to be changed to support ESR_ELx_EC_CP10_ID though)
> > > rather than directly calling kvm_inject_undefined().
> >
> > A very worthy nit, you spotted my laziness in shunting straight to
> > kvm_inject_undefined() :)
> >
> > Thinking about this a bit more deeply, this code should be dead. The
> > only time either of these conditions would happen is on a broken
> > implementation. Probably should still handle it gracefully in case the
> > CP10 handling in KVM becomes (or is in my own patch!) busted.
>
> Actually, on second thought: any objections to leaving this as-is?
> kvm_esr_cp10_id_to_sys64() spits out sys_reg_params that point at the
> MRS alias for the VMRS register. Even if that call succeeds, the params
> that get printed out by unhandled_cp_access() do not match the actual
> register the guest was accessing. And if the call fails, ->Op2 is
> uninitialized.
I understand a few additional changes are needed for this.
Or simply use WARN_ON_ONCE ? (since this cannot be created by
the guest but by a KVM or CPU problem)
I'm fine with the current code as-is though:)
Thanks,
Reiji
