Re: [Qemu-devel] [PATCH v7 RESEND 5/5] hw/intc/arm_gicv3_kvm: Reset GICv3 cpu interface registers

[Vijay Kilari]

On Tue, Feb 7, 2017 at 8:19 PM, Peter Maydell <address@hidden> wrote:
> On 31 January 2017 at 16:23,  <address@hidden> wrote:
>> From: Vijaya Kumar K <address@hidden>
>>
>> Reset CPU interface registers of GICv3 when CPU is reset.
>> For this, ARMCPRegInfo struct is registered with one ICC
>> register whose resetfn is called when cpu is reset.
>>
>> All the ICC registers are reset under one single register
>> reset function instead of calling resetfn for each ICC
>> register.
>>
>> Signed-off-by: Vijaya Kumar K <address@hidden>
>> ---
>>  hw/intc/arm_gicv3_kvm.c | 69 
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 69 insertions(+)
>>
>> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
>> index f91e0ac..c3f38aa 100644
>> --- a/hw/intc/arm_gicv3_kvm.c
>> +++ b/hw/intc/arm_gicv3_kvm.c
>> @@ -604,6 +604,39 @@ static void kvm_arm_gicv3_get(GICv3State *s)
>>      }
>>  }
>>
>> +static void arm_gicv3_icc_reset(CPUARMState *env, const ARMCPRegInfo *ri)
>> +{
>> +    ARMCPU *cpu;
>> +    GICv3State *s;
>> +    GICv3CPUState *c;
>> +
>> +    c = (GICv3CPUState *)env->gicv3state;
>> +    if (!c || !c->cpu || !c->gic) {
>
> We should assert this kind of thing, not just silently do nothing.
> Or just assume it's true, because if it's not then we'll segfault
> immediately below which is just as clear an indication of where
> the bug is as asserting.

OK
>
>> +        return;
>> +    }
>> +
>> +    s = c->gic;
>> +    if (!s) {
>
> You've already checked this once.
>
>> +        return;
>> +    }
>> +
>> +    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;
>> +    c->icc_bpr[GICV3_G1NS] = GIC_MIN_BPR;
>> +
>> +    c->icc_sre_el1 = 0x7;
>> +    memset(c->icc_apr, 0, sizeof(c->icc_apr));
>> +    memset(c->icc_igrpen, 0, sizeof(c->icc_igrpen));
>> +}
>> +
>>  static void kvm_arm_gicv3_reset(DeviceState *dev)
>>  {
>>      GICv3State *s = ARM_GICV3_COMMON(dev);
>> @@ -621,6 +654,41 @@ static void kvm_arm_gicv3_reset(DeviceState *dev)
>>      kvm_arm_gicv3_put(s);
>>  }
>>
>> +static uint64_t icc_cp_reg_read(CPUARMState *env, const ARMCPRegInfo *ri)
>> +{
>> +    return 0;
>> +}
>> +
>> +static void icc_cp_reg_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> +                          uint64_t value)
>> +{
>> +    return;
>> +}
>
> If you want to do nothing in a read/write function there are
> already arm_cp_read_zero and arm_cp_write_ignore functions for
> this. But using the ARM_CP_NOP flag is better still.

With ARM_CP_NOP qemu fails to boot.
qemu-system-aarch64: KVM_SET_DEVICE_ATTR failed: Invalid argument
Group 6 attr 0x000000000000c665

>
>> +
>> +/*
>> + * CPU interface registers of GIC needs to be reset on CPU reset.
>> + * For the calling arm_gicv3_icc_reset() on CPU reset, we register
>> + * below ARMCPRegInfo. As we reset the whole cpu interface under single
>> + * register reset, we define only one register of CPU interface instead
>> + * of defining all the registers.
>> + */
>> +static const ARMCPRegInfo gicv3_cpuif_reginfo[] = {
>> +    { .name = "ICC_CTLR_EL1", .state = ARM_CP_STATE_BOTH,
>> +      .opc0 = 3, .opc1 = 0, .crn = 12, .crm = 12, .opc2 = 4,
>> +      .type = ARM_CP_NO_RAW,
>> +      .access = PL1_RW,
>> +      .readfn = icc_cp_reg_read,
>> +      .writefn = icc_cp_reg_write,
>> +      /*
>> +       * We hang the whole cpu interface reset routine off here
>> +       * rather than parcelling it out into one little function
>> +       * per register
>> +       */
>> +      .resetfn = arm_gicv3_icc_reset,
>> +    },
>> +    REGINFO_SENTINEL
>> +};
>> +
>>  static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
>>  {
>>      GICv3State *s = KVM_ARM_GICV3(dev);
>> @@ -650,6 +718,7 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, 
>> Error **errp)
>>
>>          /* Store GICv3CPUState in CPUARMState gicv3state pointer */
>>          env->gicv3state = (void *)&s->cpu[i];
>> +        define_arm_cp_regs(cpu, gicv3_cpuif_reginfo);
>>      }
>>
>>      /* Try to create the device via the device control API */
>> --
>> 1.9.1
>>
>
> thanks
> -- PMM