On Tue, May 03, 2022 at 07:14:24PM +0200, Eric Auger wrote:
> Hi Ricardo,
>
> On 4/27/22 20:48, Ricardo Koller wrote:
> > Try to improve the predictability of ITS save/restores by failing
> > commands that would lead to failed saves. More specifically, fail any
> > command that adds an entry into an ITS table that is not in guest
> > memory, which would otherwise lead to a failed ITS save ioctl. There
> > are already checks for collection and device entries, but not for
> > ITEs. Add the corresponding check for the ITT when adding ITEs.
> >
> > Signed-off-by: Ricardo Koller <ricarkol@xxxxxxxxxx>
> > ---
> > arch/arm64/kvm/vgic/vgic-its.c | 51 ++++++++++++++++++++++++----------
> > 1 file changed, 37 insertions(+), 14 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > index 2e13402be3bd..e14790750958 100644
> > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > @@ -894,6 +894,18 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
> > return update_affinity(ite->irq, vcpu);
> > }
> >
> > +static bool __is_visible_gfn_locked(struct vgic_its *its, gpa_t gpa)
> > +{
> > + gfn_t gfn = gpa >> PAGE_SHIFT;
> > + int idx;
> > + bool ret;
> > +
> > + 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;
> > +}
> > +
> > /*
> > * Check whether an ID can be stored into the corresponding guest table.
> > * For a direct table this is pretty easy, but gets a bit nasty for
> > @@ -908,9 +920,7 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> > u64 indirect_ptr, type = GITS_BASER_TYPE(baser);
> > phys_addr_t base = GITS_BASER_ADDR_48_to_52(baser);
> > int esz = GITS_BASER_ENTRY_SIZE(baser);
> > - int index, idx;
> > - gfn_t gfn;
> > - bool ret;
> > + int index;
> >
> > switch (type) {
> > case GITS_BASER_TYPE_DEVICE:
> > @@ -933,12 +943,11 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> > return false;
> >
> > addr = base + id * esz;
> > - gfn = addr >> PAGE_SHIFT;
> >
> > if (eaddr)
> > *eaddr = addr;
> >
> > - goto out;
> > + return __is_visible_gfn_locked(its, addr);
> > }
> >
> > /* calculate and check the index into the 1st level */
> > @@ -964,16 +973,30 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> > /* Find the address of the actual entry */
> > index = id % (SZ_64K / esz);
> > indirect_ptr += index * esz;
> > - gfn = indirect_ptr >> PAGE_SHIFT;
> >
> > if (eaddr)
> > *eaddr = indirect_ptr;
> >
> > -out:
> > - 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;
> > + return __is_visible_gfn_locked(its, indirect_ptr);
> > +}
> > +
> > +/*
> > + * 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_event_id(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;
> > +
> > + /* max table size is: BIT_ULL(device->num_eventid_bits) * ite_esz */
> > + if (event_id >= BIT_ULL(device->num_eventid_bits))
> > + return false;
> > +
> > + gpa = device->itt_addr + event_id * ite_esz;
> > + return __is_visible_gfn_locked(its, gpa);
> > }
> >
> > static int vgic_its_alloc_collection(struct vgic_its *its,
> > @@ -1061,9 +1084,6 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
> > if (!device)
> > return E_ITS_MAPTI_UNMAPPED_DEVICE;
> >
> > - if (event_id >= BIT_ULL(device->num_eventid_bits))
> > - return E_ITS_MAPTI_ID_OOR;
> I would put
> if (!vgic_its_check_event_id(its, device, event_id))
> return E_ITS_MAPTI_ID_OOR;
> here instead of after since if the evend_id not correct, no use to look
> the ite for instance.
Thanks Eric. Will fix in v2.
> > -
> > if (its_cmd_get_command(its_cmd) == GITS_CMD_MAPTI)
> > lpi_nr = its_cmd_get_physical_id(its_cmd);
> > else
> > @@ -1076,6 +1096,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_event_id(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);
> Besides look good to me
> Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx>
>
> Eric
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
