[v7,RESEND,5/5] hw/intc/arm_gicv3_kvm: Reset GICv3 cpu interface registers

From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>

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 <Vijaya.Kumar@cavium.com>
---
 hw/intc/arm_gicv3_kvm.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

On 31 January 2017 at 16:23,  <vijay.kilari@gmail.com> wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> 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 <Vijaya.Kumar@cavium.com>
> ---
>  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.

> +        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.

> +
> +/*
> + * 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

On Tue, Feb 7, 2017 at 8:19 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 31 January 2017 at 16:23,  <vijay.kilari@gmail.com> wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>
>> 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 <Vijaya.Kumar@cavium.com>
>> ---
>>  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

On 16 February 2017 at 09:54, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> On Tue, Feb 7, 2017 at 8:19 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> 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

Not clear to me why using ARM_CP_NOP should result in a KVM
call failure -- can you dig further into why that is happening?

thanks
-- PMM

On Thu, Feb 16, 2017 at 3:39 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 16 February 2017 at 09:54, Vijay Kilari <vijay.kilari@gmail.com> wrote:
>> On Tue, Feb 7, 2017 at 8:19 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> 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
>
> Not clear to me why using ARM_CP_NOP should result in a KVM
> call failure -- can you dig further into why that is happening?

if ARM_CP_NOP is set, .reset function is not called and there by KVM fails
when SRE_EL1 is written with 0x0.

>
> thanks
> -- PMM

On 16 February 2017 at 11:28, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> On Thu, Feb 16, 2017 at 3:39 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 16 February 2017 at 09:54, Vijay Kilari <vijay.kilari@gmail.com> wrote:
>>> On Tue, Feb 7, 2017 at 8:19 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> 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
>>
>> Not clear to me why using ARM_CP_NOP should result in a KVM
>> call failure -- can you dig further into why that is happening?
>
> if ARM_CP_NOP is set, .reset function is not called and there by KVM fails
> when SRE_EL1 is written with 0x0.

OK. Use the arm_cp_read_zero/arm_cp_write_ignore functions, and
add a comment that we don't use CP_NOP because that means our
reset function isn't called.

thanks
-- PMM