[Qemu-devel] [PATCH] hw/intc/arm_gicv3_kvm: Check KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS in reset

[Qemu-devel] [PATCH] hw/intc/arm_gicv3_kvm: Check KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS in reset

[Eric Auger]

KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS needs to be checked before
attempting to read ICC_CTLR_EL1; otherwise kernel versions not
exposing this kvm device group will be incompatible with qemu 2.9.

Fixes: 07a5628  ("hw/intc/arm_gicv3_kvm: Reset GICv3 cpu interface registers")
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reported-by: Prakash B <bjsprakash.linux@gmail.com>

---

I understand the ICC_CTLR_EL1 state only is used in the put() function
which is used for migration
---
 hw/intc/arm_gicv3_kvm.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 81f0403..4c3a88e 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -614,12 +614,6 @@ static void arm_gicv3_icc_reset(CPUARMState *env, const ARMCPRegInfo *ri)
     s = c->gic;
     cpu = ARM_CPU(c->cpu);
 
-    /* Initialize to actual HW supported configuration */
-    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
-                      KVM_VGIC_ATTR(ICC_CTLR_EL1, cpu->mp_affinity),
-                      &c->icc_ctlr_el1[GICV3_NS], false);
-
-    c->icc_ctlr_el1[GICV3_S] = c->icc_ctlr_el1[GICV3_NS];
     c->icc_pmr_el1 = 0;
     c->icc_bpr[GICV3_G0] = GIC_MIN_BPR;
     c->icc_bpr[GICV3_G1] = GIC_MIN_BPR;
@@ -628,6 +622,17 @@ static void arm_gicv3_icc_reset(CPUARMState *env, const ARMCPRegInfo *ri)
     c->icc_sre_el1 = 0x7;
     memset(c->icc_apr, 0, sizeof(c->icc_apr));
     memset(c->icc_igrpen, 0, sizeof(c->icc_igrpen));
+
+    if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
+                              KVM_VGIC_ATTR(ICC_CTLR_EL1, cpu->mp_affinity))) {
+        return;
+    }
+    /* Initialize to actual HW supported configuration */
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
+                      KVM_VGIC_ATTR(ICC_CTLR_EL1, cpu->mp_affinity),
+                      &c->icc_ctlr_el1[GICV3_NS], false);
+
+    c->icc_ctlr_el1[GICV3_S] = c->icc_ctlr_el1[GICV3_NS];
 }
 
 static void kvm_arm_gicv3_reset(DeviceState *dev)
-- 
2.5.5

Re: [Qemu-devel] [PATCH] hw/intc/arm_gicv3_kvm: Check KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS in reset

[Auger Eric]

Adding Prakash B in cc too, sorry.

Vijaya, please let me know if I missed something in your original patch.

I tested GICv3 KVM save/restore with v4.11-rc4 and Prakash B use case
with 4.10 kernel.

Thanks

Eric

On 28/03/2017 15:58, Eric Auger wrote:
> KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS needs to be checked before
> attempting to read ICC_CTLR_EL1; otherwise kernel versions not
> exposing this kvm device group will be incompatible with qemu 2.9.
> 
> Fixes: 07a5628  ("hw/intc/arm_gicv3_kvm: Reset GICv3 cpu interface registers")
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: Prakash B <bjsprakash.linux@gmail.com>
> 
> ---
> 
> I understand the ICC_CTLR_EL1 state only is used in the put() function
> which is used for migration
> ---
>  hw/intc/arm_gicv3_kvm.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index 81f0403..4c3a88e 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -614,12 +614,6 @@ static void arm_gicv3_icc_reset(CPUARMState *env, const ARMCPRegInfo *ri)
>      s = c->gic;
>      cpu = ARM_CPU(c->cpu);
>  
> -    /* Initialize to actual HW supported configuration */
> -    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
> -                      KVM_VGIC_ATTR(ICC_CTLR_EL1, cpu->mp_affinity),
> -                      &c->icc_ctlr_el1[GICV3_NS], false);
> -
> -    c->icc_ctlr_el1[GICV3_S] = c->icc_ctlr_el1[GICV3_NS];
>      c->icc_pmr_el1 = 0;
>      c->icc_bpr[GICV3_G0] = GIC_MIN_BPR;
>      c->icc_bpr[GICV3_G1] = GIC_MIN_BPR;
> @@ -628,6 +622,17 @@ static void arm_gicv3_icc_reset(CPUARMState *env, const ARMCPRegInfo *ri)
>      c->icc_sre_el1 = 0x7;
>      memset(c->icc_apr, 0, sizeof(c->icc_apr));
>      memset(c->icc_igrpen, 0, sizeof(c->icc_igrpen));
> +
> +    if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
> +                              KVM_VGIC_ATTR(ICC_CTLR_EL1, cpu->mp_affinity))) {
> +        return;
> +    }
> +    /* Initialize to actual HW supported configuration */
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
> +                      KVM_VGIC_ATTR(ICC_CTLR_EL1, cpu->mp_affinity),
> +                      &c->icc_ctlr_el1[GICV3_NS], false);
> +
> +    c->icc_ctlr_el1[GICV3_S] = c->icc_ctlr_el1[GICV3_NS];
>  }
>  
>  static void kvm_arm_gicv3_reset(DeviceState *dev)
> 

Re: [Qemu-devel] [PATCH] hw/intc/arm_gicv3_kvm: Check KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS in reset

[Vijay Kilari]

Hi Eric,

On Tue, Mar 28, 2017 at 7:28 PM, Eric Auger <eric.auger@redhat.com> wrote:
> KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS needs to be checked before
> attempting to read ICC_CTLR_EL1; otherwise kernel versions not
> exposing this kvm device group will be incompatible with qemu 2.9.
>
> Fixes: 07a5628  ("hw/intc/arm_gicv3_kvm: Reset GICv3 cpu interface registers")
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: Prakash B <bjsprakash.linux@gmail.com>
>
> ---
>
> I understand the ICC_CTLR_EL1 state only is used in the put() function
> which is used for migration
> ---
>  hw/intc/arm_gicv3_kvm.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index 81f0403..4c3a88e 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -614,12 +614,6 @@ static void arm_gicv3_icc_reset(CPUARMState *env, const ARMCPRegInfo *ri)
>      s = c->gic;
>      cpu = ARM_CPU(c->cpu);
>
> -    /* Initialize to actual HW supported configuration */
> -    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
> -                      KVM_VGIC_ATTR(ICC_CTLR_EL1, cpu->mp_affinity),
> -                      &c->icc_ctlr_el1[GICV3_NS], false);
> -
> -    c->icc_ctlr_el1[GICV3_S] = c->icc_ctlr_el1[GICV3_NS];
>      c->icc_pmr_el1 = 0;
>      c->icc_bpr[GICV3_G0] = GIC_MIN_BPR;
>      c->icc_bpr[GICV3_G1] = GIC_MIN_BPR;
> @@ -628,6 +622,17 @@ static void arm_gicv3_icc_reset(CPUARMState *env, const ARMCPRegInfo *ri)
>      c->icc_sre_el1 = 0x7;
>      memset(c->icc_apr, 0, sizeof(c->icc_apr));
>      memset(c->icc_igrpen, 0, sizeof(c->icc_igrpen));
> +
> +    if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
> +                              KVM_VGIC_ATTR(ICC_CTLR_EL1, cpu->mp_affinity))) {
> +        return;
> +    }

 Can't we use gicv3 migration blocker provided if it is set before this reset.

> +    /* Initialize to actual HW supported configuration */
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
> +                      KVM_VGIC_ATTR(ICC_CTLR_EL1, cpu->mp_affinity),
> +                      &c->icc_ctlr_el1[GICV3_NS], false);
> +
> +    c->icc_ctlr_el1[GICV3_S] = c->icc_ctlr_el1[GICV3_NS];
>  }
>
>  static void kvm_arm_gicv3_reset(DeviceState *dev)
> --
> 2.5.5
>

Re: [Qemu-devel] [PATCH] hw/intc/arm_gicv3_kvm: Check KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS in reset

[Auger Eric]

Hi Vijay,

On 28/03/2017 17:08, Vijay Kilari wrote:
> Hi Eric,
> 
> On Tue, Mar 28, 2017 at 7:28 PM, Eric Auger <eric.auger@redhat.com> wrote:
>> KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS needs to be checked before
>> attempting to read ICC_CTLR_EL1; otherwise kernel versions not
>> exposing this kvm device group will be incompatible with qemu 2.9.
>>
>> Fixes: 07a5628  ("hw/intc/arm_gicv3_kvm: Reset GICv3 cpu interface registers")
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reported-by: Prakash B <bjsprakash.linux@gmail.com>
>>
>> ---
>>
>> I understand the ICC_CTLR_EL1 state only is used in the put() function
>> which is used for migration
>> ---
>>  hw/intc/arm_gicv3_kvm.c | 17 +++++++++++------
>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
>> index 81f0403..4c3a88e 100644
>> --- a/hw/intc/arm_gicv3_kvm.c
>> +++ b/hw/intc/arm_gicv3_kvm.c
>> @@ -614,12 +614,6 @@ static void arm_gicv3_icc_reset(CPUARMState *env, const ARMCPRegInfo *ri)
>>      s = c->gic;
>>      cpu = ARM_CPU(c->cpu);
>>
>> -    /* Initialize to actual HW supported configuration */
>> -    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
>> -                      KVM_VGIC_ATTR(ICC_CTLR_EL1, cpu->mp_affinity),
>> -                      &c->icc_ctlr_el1[GICV3_NS], false);
>> -
>> -    c->icc_ctlr_el1[GICV3_S] = c->icc_ctlr_el1[GICV3_NS];
>>      c->icc_pmr_el1 = 0;
>>      c->icc_bpr[GICV3_G0] = GIC_MIN_BPR;
>>      c->icc_bpr[GICV3_G1] = GIC_MIN_BPR;
>> @@ -628,6 +622,17 @@ static void arm_gicv3_icc_reset(CPUARMState *env, const ARMCPRegInfo *ri)
>>      c->icc_sre_el1 = 0x7;
>>      memset(c->icc_apr, 0, sizeof(c->icc_apr));
>>      memset(c->icc_igrpen, 0, sizeof(c->icc_igrpen));
>> +
>> +    if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
>> +                              KVM_VGIC_ATTR(ICC_CTLR_EL1, cpu->mp_affinity))) {
>> +        return;
>> +    }
> 
>  Can't we use gicv3 migration blocker provided if it is set before this reset.
yes we could. I hesitated but eventually found the relationship between
the blocker and this kvm device group access was maybe not obvious.

As preferred.

Thanks

Eric
> 
>> +    /* Initialize to actual HW supported configuration */
>> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
>> +                      KVM_VGIC_ATTR(ICC_CTLR_EL1, cpu->mp_affinity),
>> +                      &c->icc_ctlr_el1[GICV3_NS], false);
>> +
>> +    c->icc_ctlr_el1[GICV3_S] = c->icc_ctlr_el1[GICV3_NS];
>>  }
>>
>>  static void kvm_arm_gicv3_reset(DeviceState *dev)
>> --
>> 2.5.5
>>
> 

Re: [Qemu-devel] [PATCH] hw/intc/arm_gicv3_kvm: Check KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS in reset

[Prakash B]

On Tue, Mar 28, 2017 at 7:28 PM, Eric Auger <eric.auger@redhat.com> wrote:
> KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS needs to be checked before
> attempting to read ICC_CTLR_EL1; otherwise kernel versions not
> exposing this kvm device group will be incompatible with qemu 2.9.
>
> Fixes: 07a5628  ("hw/intc/arm_gicv3_kvm: Reset GICv3 cpu interface registers")
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: Prakash B <bjsprakash.linux@gmail.com>
>
> ---
>
Thank Eric,

Verified compatibility for host kernel versions 4.4, 4.10 and
4.11-rc3, guest booted fine with these.

Tested-by: Prakash, Brahmajyosyula <Brahmajyosyula.Prakash@cavium.com>