Re: [Qemu-devel] [RFC PATCH v5 2/2] hw/intc/arm_gicv3_kvm: Implement get/put functions

[Vijay Kilari]

On Fri, Oct 7, 2016 at 9:00 PM, Peter Maydell <address@hidden> wrote:
> On 20 September 2016 at 07:55,  <address@hidden> wrote:
>> From: Vijaya Kumar K <address@hidden>
>>
>> This actually implements pre_save and post_load methods for in-kernel
>> vGICv3.
>>
>> Signed-off-by: Pavel Fedin <address@hidden>
>> Signed-off-by: Peter Maydell <address@hidden>
>> Signed-off-by: Vijaya Kumamr K <address@hidden>
>> [PMM:
>>  * use decimal, not 0bnnn
>>  * fixed typo in names of ICC_APR0R_EL1 and ICC_AP1R_EL1
>>  * completely rearranged the get and put functions to read and write
>>    the state in a natural order, rather than mixing distributor and
>>    redistributor state together]
>> [Vijay:
>>  * Update macro KVM_VGIC_ATTR
>>  * Use 32 bit access for gicd and gicr
>>  * GICD_IROUTER, GICD_TYPER, GICR_PROPBASER and GICR_PENDBASER reg
>>    access  are changed from 64-bit to 32-bit access
>>  * s->edge_trigger stores only even bits value of an irq config.
>>    Update translate_edge_trigger() accordingly.
>>  * Add ICC_SRE_EL1 save and restore
>>  * Initialized ICC registers during reset
>
> These sorts of [] changes should go above the sign-off
> of the person who did them, to indicate where in the
> chain they happened. Also, yours is missing the closing ].
>
>> ---
>> ---
>
>> +/* Translate from the in-kernel field for an IRQ value to/from the qemu
>> + * representation. Note that these are only expected to be used for
>> + * SPIs (that is, for interrupts whose state is in the distributor
>> + * rather than the redistributor).
>> + */
>> +typedef void (*vgic_translate_fn)(GICv3State *s, int irq,
>> +                                  uint32_t *field, bool to_kernel);
>> +
>> +static void translate_edge_trigger(GICv3State *s, int irq,
>> +    uint32_t *field, bool to_kernel)
>> +{
>> +    /*
>> +     * s->edge_trigger stores only even bits value of an irq config.
>> +     * Consider only even bits and translate accordingly.
>> +     */
>> +    if (to_kernel) {
>> +        *field = gicv3_gicd_edge_trigger_test(s, irq);
>> +        *field = (*field << 1) & 3;
>> +    } else {
>> +        *field = (*field >> 1) & 1;
>> +        gicv3_gicd_edge_trigger_replace(s, irq, *field);
>> +    }
>> +}
>
> I would prefer that we just open-coded a for-loop for these,
> as then you can use half_shuffle32 and half_unshuffle32 to
> deal with the bits 32 at a time.

You mean to completely drop this translate_fn which is called from
kvm_dist_put/get() and have a direct function to handle edge_trigger?

>
>> +
>> +static void translate_priority(GICv3State *s, int irq,
>> +                               uint32_t *field, bool to_kernel)
>> +{
>> +    if (to_kernel) {
>> +        *field = s->gicd_ipriority[irq];
>> +    } else {
>> +        s->gicd_ipriority[irq] = *field;
>> +    }
>> +}
>
> Similarly, this would be better with open-coded for loops.
> Then we can dump the translate_fn machinery entirely.
>
>> +
>> +static void kvm_arm_gicv3_reset_reg(GICv3State *s)
>> +{
>> +    int ncpu;
>> +
>> +    for (ncpu = 0; ncpu < s->num_cpu; ncpu++) {
>> +        GICv3CPUState *c = &s->cpu[ncpu];
>> +
>> +        /* Initialize to actual HW supported configuration */
>> +        kvm_gicc_access(s, ICC_CTLR_EL1, ncpu,
>> +                        &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));
>> +    }
>
> This shouldn't be in this patch. If we need to fix reset we
> should do it as a separate patch.
>
> Also this isn't the right place, really, because the CPU interface
> registers need to be reset when the CPU is reset, not when
> the GIC device is reset.

To make GIC cpuif registers to reset upon cpu reset,
I propose to add Interface for gicv3_common class and
call this interface from arm_cpu_reset() similar to
ARMLinuxBootIf. This will be more generic way rather
than searching for gicv3 object and reset the registers

>
>>  }
>
>>  static void kvm_arm_gicv3_class_init(ObjectClass *klass, void *data)
>> diff --git a/include/hw/intc/arm_gicv3_common.h 
>> b/include/hw/intc/arm_gicv3_common.h
>> index 341a311..183c7f8 100644
>> --- a/include/hw/intc/arm_gicv3_common.h
>> +++ b/include/hw/intc/arm_gicv3_common.h
>> @@ -166,6 +166,7 @@ struct GICv3CPUState {
>>      uint8_t gicr_ipriorityr[GIC_INTERNAL];
>>
>>      /* CPU interface */
>> +    uint64_t icc_sre_el1;
>
> Where has this come from? If we need to add a new field then it

This was part of review comment from Christoffer to add icc_sre_el1
save and restore

> needs to be in a different patch (and we need to make sure we
OK. I will spin a new patch

> add it to the VMState struct as well). But neither the emulated
> GIC nor the kernel will support writing any bits in this
> register as far as I'm aware, so it's always constant 0x7...

Kernel will not allow write on this. But make a check for SRE bit.
>
> thanks
> -- PMM