On Wed, Nov 6, 2024 at 5:14 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Wed, 06 Nov 2024 08:30:33 +0000,
> Jing Zhang <jingzhangos@xxxxxxxxxx> wrote:
> >
> > From: Kunkun Jiang <jiangkunkun@xxxxxxxxxx>
> >
> > vgic_its_save_device_tables will traverse its->device_list to
> > save DTE for each device. vgic_its_restore_device_tables will
> > traverse each entry of device table and check if it is valid.
> > Restore if valid.
> >
> > But when MAPD unmaps a device, it does not invalidate the
> > corresponding DTE. In the scenario of continuous saves
> > and restores, there may be a situation where a device's DTE
> > is not saved but is restored. This is unreasonable and may
> > cause restore to fail. This patch clears the corresponding
> > DTE when MAPD unmaps a device.
> >
> > Co-developed-by: Shusen Li <lishusen2@xxxxxxxxxx>
> > Signed-off-by: Shusen Li <lishusen2@xxxxxxxxxx>
> > Signed-off-by: Kunkun Jiang <jiangkunkun@xxxxxxxxxx>
> > Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> > ---
> > arch/arm64/kvm/vgic/vgic-its.c | 14 ++++++++++++--
> > 1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > index 2381bc5ce544..7c57c7c6fbff 100644
> > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > @@ -1140,8 +1140,9 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
> > u8 num_eventid_bits = its_cmd_get_size(its_cmd);
> > gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
> > struct its_device *device;
> > + gpa_t gpa;
> >
> > - if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
> > + if (!vgic_its_check_id(its, its->baser_device_table, device_id, &gpa))
> > return E_ITS_MAPD_DEVICE_OOR;
> >
> > if (valid && num_eventid_bits > VITS_TYPER_IDBITS)
> > @@ -1161,8 +1162,17 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
> > * The spec does not say whether unmapping a not-mapped device
> > * is an error, so we are done in any case.
> > */
> > - if (!valid)
> > + if (!valid) {
> > + struct kvm *kvm = its->dev->kvm;
> > + int dte_esz = vgic_its_get_abi(its)->dte_esz;
> > + u64 val = 0;
> > +
> > + if (KVM_BUG_ON(dte_esz != sizeof(val), kvm))
> > + return -EINVAL;
>
> I find it pretty odd to bug only in that case, and the sprinkling of
> these checks all over the place is horrible. I'm starting to wonder if
> we shouldn't simply wrap vgic_write_guest() and co to do the checking.
>
> > +
> > + vgic_write_guest_lock(kvm, gpa, &val, dte_esz);
>
> I'm thinking of something like:
>
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index ba945ba78cc7d..d8e57aefcd3a5 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -1128,6 +1128,19 @@ static struct its_device *vgic_its_alloc_device(struct vgic_its *its,
> return device;
> }
>
> +
> +#define its_write_entry_lock(i, g, valp, t) \
> + ({ \
> + struct kvm *__k = (i)->dev->kvm; \
> + int __sz = vgic_its_get_abi(i)->t; \
> + int __ret = 0; \
> + if (KVM_BUG_ON(__sz != sizeof(*(valp)), __k)) \
> + __ret = -EINVAL; \
> + else \
> + vgic_write_guest_lock(__k, (g), (valp), __sz); \
> + __ret; \
> + })
> +
> /*
> * MAPD maps or unmaps a device ID to Interrupt Translation Tables (ITTs).
> * Must be called with the its_lock mutex held.
> @@ -1140,8 +1153,9 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
> u8 num_eventid_bits = its_cmd_get_size(its_cmd);
> gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
> struct its_device *device;
> + gpa_t gpa;
>
> - if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
> + if (!vgic_its_check_id(its, its->baser_device_table, device_id, &gpa))
> return E_ITS_MAPD_DEVICE_OOR;
>
> if (valid && num_eventid_bits > VITS_TYPER_IDBITS)
> @@ -1161,8 +1175,10 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
> * The spec does not say whether unmapping a not-mapped device
> * is an error, so we are done in any case.
> */
> - if (!valid)
> - return 0;
> + if (!valid) {
> + u64 val = 0;
> + return its_write_entry_lock(its, gpa, &val, dte_esz);
> + }
>
> device = vgic_its_alloc_device(its, device_id, itt_addr,
> num_eventid_bits);
>
> which can be generalised everywhere (you can even extract the check
> and move it to an out-of-line helper as required).
Sounds good. Will do as you suggested.
Jing
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
