Received-SPF: pass (google.com: domain of linux-kernel+bounces-65854-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 14 Feb 2024 11:00:00 -0800
From: Oliver Upton <oliver.upton@linux.dev>
To: Marc Zyngier <maz@kernel.org>
Cc: kvmarm@lists.linux.dev, kvm@vger.kernel.org,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 19/23] KVM: selftests: Add a minimal library for
 interacting with an ITS
Message-ID: <Zc0NsFm40nIqTmRf@linux.dev>
References: <20240213093250.3960069-1-oliver.upton@linux.dev>
 <20240213094114.3961683-1-oliver.upton@linux.dev>
 <86zfw33qae.wl-maz@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <86zfw33qae.wl-maz@kernel.org>

On Wed, Feb 14, 2024 at 05:32:25PM +0000, Marc Zyngier wrote:
> On Tue, 13 Feb 2024 09:41:14 +0000,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > A prerequisite of testing LPI injection performance is of course
> > instantiating an ITS for the guest. Add a small library for creating an
> > ITS and interacting with it *from userspace*.
> > 
> > Yep, you read that right. KVM unintentionally allows userspace to send
> > commands to the virtual ITS via the command queue. Besides adding test
> > coverage for an elusive UAPI, interacting with the ITS in userspace
> > simplifies the handling of commands that need to allocate memory, like a
> > MAPD command with an ITT.
> 
> I don't mean to derail the party, but I really think we should plug
> this hole. Either that, or we make it an official interface for state
> restore. And don't we all love to have multiple interfaces to do the
> same thing?

Ok, I've thought about it a bit more and I'm fully convinced we need to
shut the door on this stupidity.

We expect CREADR == CWRITER at the time userspace saves the ITS
registers, but we have a *hideous* ordering issue on the restore path.

If the order of restore from userspace is CBASER, CWRITER, CREADR then
we **wind up replaying the entire command queue**. While insane, I'm
pretty sure it is legal for the guest to write garbage after the read
pointer has moved past a particular command index.

Fsck!!!

So, how about we do this:

 - Provide a uaccess hook for CWRITER that changes the write-pointer
   without processing any commands

 - Assert an invariant that at any time CWRITER or CREADR are read from
   userspace that CREADR == CWRITER. Fail the ioctl and scream if that
   isn't the case, so that way we never need to worry about processing
   'in-flight' commands at the destination.

-- 
Thanks,
Oliver