Re: [PATCH 1/4] KVM: arm64: vgic: Check that new ITEs could be saved in guest memory

[Ricardo Koller <ricarkol@google.com>]

On Tue, Apr 26, 2022 at 06:34:49PM +0100, Marc Zyngier wrote:
> On Tue, 26 Apr 2022 17:21:07 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > On Tue, Apr 26, 2022 at 05:07:40AM +0100, Marc Zyngier wrote:
> > > On Mon, 25 Apr 2022 19:55:31 +0100,
> > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > 
> > > > A command that adds an entry into an ITS table that is not in guest
> > > > memory should fail, as any command should be treated as if it was
> > > > actually saving entries in guest memory (KVM doesn't until saving).
> > > > Add the corresponding check for the ITT when adding ITEs.
> > > > 
> > > > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > > ---
> > > >  arch/arm64/kvm/vgic/vgic-its.c | 34 ++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 34 insertions(+)
> > > > 
> > > > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > > > index 2e13402be3bd..d7c1a3a01af4 100644
> > > > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > > > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > > > @@ -976,6 +976,37 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> > > >  	return ret;
> > > >  }
> > > >  
> > > > +/*
> > > > + * Check whether an event ID can be stored in the corresponding Interrupt
> > > > + * Translation Table, which starts at device->itt_addr.
> > > > + */
> > > > +static bool vgic_its_check_ite(struct vgic_its *its, struct its_device *device,
> > > > +		u32 event_id)
> > > > +{
> > > > +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> > > > +	int ite_esz = abi->ite_esz;
> > > > +	gpa_t gpa;
> > > > +	gfn_t gfn;
> > > > +	int idx;
> > > > +	bool ret;
> > > > +
> > > > +	/* max table size is: BIT_ULL(device->num_eventid_bits) * ite_esz */
> > > > +	if (event_id >= BIT_ULL(device->num_eventid_bits))
> > > > +		return false;
> > > 
> > > We have already checked this condition, it seems.
> > > 
> > > > +
> > > > +	gpa = device->itt_addr + event_id * ite_esz;
> > > > +	gfn = gpa >> PAGE_SHIFT;
> > > > +
> > > > +	idx = srcu_read_lock(&its->dev->kvm->srcu);
> > > > +	ret = kvm_is_visible_gfn(its->dev->kvm, gfn);
> > > > +	srcu_read_unlock(&its->dev->kvm->srcu, idx);
> > > > +	return ret;
> > > 
> > > Why should we care? If the guest doesn't give us the memory that is
> > > required, that's its problem.
> > 
> > The issue is that if the guest does that, then the pause will fail and
> > we won't be able to migrate the VM. This series objective is to help
> > with failed migrations due to the ITS. This commit tries to do it by
> > avoiding them.
> 
> But the guest is buggy, isn't it? No memory, no cookie! ;-)
> 
> I understand that you want save/restore to be predictable even when
> the guest is too crap for words. I think clarifying this in your
> commit message would help.
> 
> > > The only architectural requirement is
> > > that the EID fits into the device table. There is no guarantee that
> > > the ITS will actually write to the memory.
> > 
> > If I understand it correctly, failing the command in this case would
> > also be architectural (right?). If the ITS tries to write the new
> > entry into memory immediately, then the command would fail. This
> > proposed check is just making this assumption.
> 
> Neither behaviour is architectural (they are both allowed, but none
> is required). Not providing the memory, however, is unpredictable.
> 
> I'm OK with your approach because it makes things consistent (we fail
> early rather than late). But the commit message doesn't really reflect
> that (it sort of hints to it, but not in a clear way).
> 

Sounds good, will do that, thanks.

> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.