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  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 
> Reported-by: Prakash B 
>
> ---
>
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 

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  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 
>> Reported-by: Prakash B 
>>
>> ---
>>
>> 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),
>> -  >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),
>> +  >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

[Vijay Kilari]

Hi Eric,

On Tue, Mar 28, 2017 at 7:28 PM, 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 
> Reported-by: Prakash B 
>
> ---
>
> 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),
> -  >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),
> +  >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 
> Reported-by: Prakash B 
> 
> ---
> 
> 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),
> -  >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),
> +  >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)
> 

[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 
Reported-by: Prakash B 

---

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),
-  >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),
+  >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