| Message ID | 20241106083035.2813799-3-jingzhangos@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Some fixes about vgic-its | expand |
On Wed, 06 Nov 2024 08:30:33 +0000, Jing Zhang <jingzhangos@google.com> wrote: > > From: Kunkun Jiang <jiangkunkun@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@huawei.com> > Signed-off-by: Shusen Li <lishusen2@huawei.com> > Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com> > Signed-off-by: Jing Zhang <jingzhangos@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). M.
On Wed, Nov 6, 2024 at 5:14 AM Marc Zyngier <maz@kernel.org> wrote: > > On Wed, 06 Nov 2024 08:30:33 +0000, > Jing Zhang <jingzhangos@google.com> wrote: > > > > From: Kunkun Jiang <jiangkunkun@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@huawei.com> > > Signed-off-by: Shusen Li <lishusen2@huawei.com> > > Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com> > > Signed-off-by: Jing Zhang <jingzhangos@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.
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; + + vgic_write_guest_lock(kvm, gpa, &val, dte_esz); return 0; + } device = vgic_its_alloc_device(its, device_id, itt_addr, num_eventid_bits);