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  wrote:
> On 20 September 2016 at 07:55,   wrote:
>> From: Vijaya Kumar K 
>>
>> This actually implements pre_save and post_load methods for in-kernel
>> vGICv3.
>>
>> Signed-off-by: Pavel Fedin 
>> Signed-off-by: Peter Maydell 
>> Signed-off-by: Vijaya Kumamr K 
>> [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 = >cpu[ncpu];
>> +
>> +/* Initialize to actual HW supported configuration */
>> +kvm_gicc_access(s, ICC_CTLR_EL1, ncpu,
>> +>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 

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

[Peter Maydell]

On 20 September 2016 at 07:55,   wrote:
> From: Vijaya Kumar K 
>
> This actually implements pre_save and post_load methods for in-kernel
> vGICv3.
>
> Signed-off-by: Pavel Fedin 
> Signed-off-by: Peter Maydell 
> Signed-off-by: Vijaya Kumamr K 
> [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.

> +
> +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 = >cpu[ncpu];
> +
> +/* Initialize to actual HW supported configuration */
> +kvm_gicc_access(s, ICC_CTLR_EL1, ncpu,
> +>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.

>  }

>  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
needs to be in a different patch (and we need to make sure we
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...

thanks
-- PMM