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 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.

> 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.

> 
> And if you feel that there is a strong need to have this, surely you
> can reuse some of the vgic_its_check_id() code.
> 

Sure, will do that.

> > +}
> > +
> > +/*
> > + * Adds a new collection into the ITS collection table.
> > + * Returns 0 on success, and a negative error value for generic errors.
> 
> Not only. A positive error can also be returned for an out of range
> condition.
> 

This is coming from a subsequent commit. It made it here by mistake
(most likely when I was reordering the commits). Will put it where it
belongs, sorry for that.

Thanks for the review,
Ricardo

> > + */
> >  static int vgic_its_alloc_collection(struct vgic_its *its,
> >  				     struct its_collection **colp,
> >  				     u32 coll_id)
> > @@ -1076,6 +1107,9 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
> >  	if (find_ite(its, device_id, event_id))
> >  		return 0;
> >  
> > +	if (!vgic_its_check_ite(its, device, event_id))
> > +		return E_ITS_MAPTI_ID_OOR;
> > +
> >  	collection = find_collection(its, coll_id);
> >  	if (!collection) {
> >  		int ret = vgic_its_alloc_collection(its, &collection, coll_id);
> > -- 
> > 2.36.0.rc2.479.g8af0fa9b8e-goog
> > 
> > 
> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.