On 14/07/18 18:05, Christoffer Dall wrote:
> Simply letting IGROUPR be writable from userspace would break
> migration from old kernels to newer kernels, because old kernels
> incorrectly report interrupt groups as group 1. This would not be a big
> problem if userspace wrote GICD_IIDR as read from the kernel, because we
> could detect the incompatibility and return an error to userspace.
> Unfortunately, this is not the case with current userspace
> implementations and simply letting IGROUPR be writable from userspace for
> an emulated GICv2 silently breaks migration and causes the destination
> VM to no longer run after migration.
>
> We now encourage userspace to write the read and expected value of
> GICD_IIDR as the first part of a GIC register restore, and if we observe
> a write to GICD_IIDR we know that userspace has been updated and has had
> a chance to cope with older kernels (VGICv2 IIDR.Revision == 0)
> incorrectly reporting interrupts as group 1, and therefore we now allow
> groups to be user writable.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxx>
> ---
> include/kvm/arm_vgic.h | 3 +++
> virt/kvm/arm/vgic/vgic-mmio-v2.c | 15 ++++++++++++++-
> 2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index c661d0e..c134790 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -221,6 +221,9 @@ struct vgic_dist {
> /* Implementation revision as reported in the GICD_IIDR */
> u32 implementation_rev;
>
> + /* Userspace can write to GICv2 IGROUPR */
> + bool v2_groups_user_writable;
> +
> /* Do injected MSIs require an additional device ID? */
> bool msis_require_devid;
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index b79de42..9b6b758 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -85,6 +85,17 @@ static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
> case GIC_DIST_IIDR:
> if (val != vgic_mmio_read_v2_misc(vcpu, addr, len))
> return -EINVAL;
> +
> + /*
> + * If we observe a write to GICD_IIDR we know that userspace
> + * has been updated and has had a chance to cope with older
> + * kernels (VGICv2 IIDR.Revision == 0) incorrectly reporting
> + * interrupts as group 1, and therefore we now allow gruops to
groups
> + * be user writable. Doing this by default would break
> + * migration from old kernels to new kernels with legacy
> + * userspace.
> + */
> + vcpu->kvm->arch.vgic.v2_groups_user_writable = true;
return 0; ?
I think it'd be safer, just in case we decide to do something bizarre in
vgic_mmio_write_v2_misc...
> }
>
> vgic_mmio_write_v2_misc(vcpu, addr, len, val);
> @@ -95,7 +106,9 @@ static int vgic_mmio_uaccess_write_v2_group(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val)
> {
> - /* Ignore writes from userspace */
> + if (vcpu->kvm->arch.vgic.v2_groups_user_writable)
> + vgic_mmio_write_group(vcpu, addr, len, val);
> +
> return 0;
> }
>
>
Otherwise looks good..
M.
--
Jazz is not dead. It just smells funny...
