Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

From: Oliver Upton <oupton@google.com>
To: Reiji Watanabe <reijiw@google.com>
Cc: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
	Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Peter Shier <pshier@google.com>,
	Ricardo Koller <ricarkol@google.com>
Subject: Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler
Date: Mon, 4 Apr 2022 23:19:25 +0000	[thread overview]
Message-ID: <Ykt8/Q5LLpZdgLu5@google.com> (raw)
In-Reply-To: <YkqCAcPCnqYofspa@google.com>

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, &params) || 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.

Sorry for backtracking here.

--
Thanks,
Oliver

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