Re: [Qemu-arm] [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 <address@hidden> 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 <address@hidden>
>> Reported-by: Prakash B <address@hidden>
>>
>> ---
>>
>> 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
>>
>