[PATCH v3 2/4] KVM: arm64: vgic-its: Clear DTE when MAPD unmaps a device

[Jing Zhang]

On Wed, Nov 6, 2024 at 5:14 AM Marc Zyngier <maz at kernel.org> wrote:
>
> On Wed, 06 Nov 2024 08:30:33 +0000,
> Jing Zhang <jingzhangos at google.com> wrote:
> >
> > From: Kunkun Jiang <jiangkunkun at huawei.com>
> >
> > 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 at huawei.com>
> > Signed-off-by: Shusen Li <lishusen2 at huawei.com>
> > Signed-off-by: Kunkun Jiang <jiangkunkun at huawei.com>
> > Signed-off-by: Jing Zhang <jingzhangos at google.com>
> > ---
> >  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.