| Message ID | 1506524205-20763-2-git-send-email-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/3] hw/intc/arm_gicv3_its: Don't abort on table save/restore | expand |
On 27 September 2017 at 15:56, Eric Auger <eric.auger@redhat.com> wrote: > The ITS is not properly reset at the moment. It is possible the > GITS_BASER<n>.valid is set and the in-kernel ITS caches are not > empty (list of devices, collections, LPIs) while data structures > in guest RAM are invalid/inconsistent. > > For instance, this happens after a guest shutdown -r now or a > system reset, if we save the state before the guest re-writes > the ITS registers or map devices, the table save ioctl may > produce a QEMU abort. > > Until there is a proper reset implemented, let's unplug the > consistency error checking. > > The reset issue will be fixed in subsequent patches. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Reported-by: wanghaibin <wanghaibin.wang@huawei.com> When in particular does this cause an abort -- when we're trying to save the state in these edge cases, or when we're trying to restore it? What does the kernel do -- is it just rejecting the attempt, or might it actually have done bad things to guest memory ? thanks -- PMM
Hi Peter, On 12/10/2017 13:54, Peter Maydell wrote: > On 27 September 2017 at 15:56, Eric Auger <eric.auger@redhat.com> wrote: >> The ITS is not properly reset at the moment. It is possible the >> GITS_BASER<n>.valid is set and the in-kernel ITS caches are not >> empty (list of devices, collections, LPIs) while data structures >> in guest RAM are invalid/inconsistent. >> >> For instance, this happens after a guest shutdown -r now or a >> system reset, if we save the state before the guest re-writes >> the ITS registers or map devices, the table save ioctl may >> produce a QEMU abort. >> >> Until there is a proper reset implemented, let's unplug the >> consistency error checking. >> >> The reset issue will be fixed in subsequent patches. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Reported-by: wanghaibin <wanghaibin.wang@huawei.com> > > When in particular does this cause an abort -- when we're > trying to save the state in these edge cases, or when we're > trying to restore it? Both. After a guest reset, device GITS_BASER<n> may point to an L1 device table that is not valid anymore. In that case device table save IOTCL returned -EINVAL and we aborted. On restore we had a bug in case all data in the table is invalid. In that case as well we currently abort. What does the kernel do -- is it just > rejecting the attempt, or might it actually have done bad > things to guest memory ? On Save, in case the GITS_BASER points to an invalid linear device tree table but the page still corresponds to a visible gfn window, this memory could be overwritten by the kernel. Thanks Eric > > thanks > -- PMM >
diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c index 39903d5..120b86d 100644 --- a/hw/intc/arm_gicv3_its_kvm.c +++ b/hw/intc/arm_gicv3_its_kvm.c @@ -64,20 +64,16 @@ static void vm_change_state_handler(void *opaque, int running, { GICv3ITSState *s = (GICv3ITSState *)opaque; Error *err = NULL; - int ret; if (running) { return; } - ret = kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL, - KVM_DEV_ARM_ITS_SAVE_TABLES, NULL, true, &err); + kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL, + KVM_DEV_ARM_ITS_SAVE_TABLES, NULL, true, &err); if (err) { error_report_err(err); } - if (ret < 0 && ret != -EFAULT) { - abort(); - } } static void kvm_arm_its_realize(DeviceState *dev, Error **errp) @@ -157,6 +153,7 @@ static void kvm_arm_its_pre_save(GICv3ITSState *s) */ static void kvm_arm_its_post_load(GICv3ITSState *s) { + Error *err = NULL; int i; if (!s->iidr) { @@ -188,7 +185,11 @@ static void kvm_arm_its_post_load(GICv3ITSState *s) kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL, KVM_DEV_ARM_ITS_RESTORE_TABLES, NULL, true, - &error_abort); + &err); + + if (err) { + error_report_err(err); + } kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS, GITS_CTLR, &s->ctlr, true, &error_abort);
The ITS is not properly reset at the moment. It is possible the GITS_BASER<n>.valid is set and the in-kernel ITS caches are not empty (list of devices, collections, LPIs) while data structures in guest RAM are invalid/inconsistent. For instance, this happens after a guest shutdown -r now or a system reset, if we save the state before the guest re-writes the ITS registers or map devices, the table save ioctl may produce a QEMU abort. Until there is a proper reset implemented, let's unplug the consistency error checking. The reset issue will be fixed in subsequent patches. Signed-off-by: Eric Auger <eric.auger@redhat.com> Reported-by: wanghaibin <wanghaibin.wang@huawei.com> --- this patch would deserve being cc'ed stable (2.10) --- hw/intc/arm_gicv3_its_kvm.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)