Received-SPF: pass (google.com: domain of linux-kernel+bounces-65963-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 14 Feb 2024 13:06:12 -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: <Zc0rRFLRhLfLshpm@linux.dev>
References: <20240213093250.3960069-1-oliver.upton@linux.dev>
 <20240213094114.3961683-1-oliver.upton@linux.dev>
 <86zfw33qae.wl-maz@kernel.org>
 <Zc0NsFm40nIqTmRf@linux.dev>
 <86v86q4xkf.wl-maz@kernel.org>
 <Zc0orzU-CeKEyx3j@linux.dev>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zc0orzU-CeKEyx3j@linux.dev>

On Wed, Feb 14, 2024 at 12:55:11PM -0800, Oliver Upton wrote:
> On Wed, Feb 14, 2024 at 08:09:52PM +0000, Marc Zyngier wrote:
> > > 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!!!
> > 
> > This is documented Documentation/virt/kvm/devices/arm-vgic-its.rst to
> > some extent, and it is allowed for the guest to crap itself on behalf
> > of userspace if the ordering isn't respected.
> 
> Ah, fair, I missed the documentation here. If we require userspace to
> write CTLR last then we _should_ be fine, but damn is this a tricky set
> of expectations.
> 
> > > 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.
> > 
> > Are we guaranteed that we cannot ever see CWRITER != CREADR at VM
> > dumping time? I'm not convinced that we cannot preempt the vcpu thread
> > at the right spot, specially given that you can have an arbitrary
> > large batch of commands to execute.
> > 
> > Just add a page-fault to the mix, and a signal pending. Pronto, you
> > see a guest exit and you should be able to start dumping things
> > without the ITS having processed much. I haven't tried, but that
> > doesn't seem totally unlikely.
> 
> Well, we would need to run all userspace reads and writes through the
> cmd_lock in this case, which is what we already do for the CREADR
> uaccess hook. To me the 'racy' queue accessors only make sense for guest
> accesses, since the driver is expecting to poll for completion in that
> case.

My proposed invariant cannot be maintained, of course, since userspace
can do whatever it pleases on the cmdq pointers.

> Otherwise we decide the existing rules for restoring the ITS are fine
> and I get to keep my funky driver :)
> 
> -- 
> Thanks,
> Oliver
>