On Fri, Oct 7, 2016 at 9:00 PM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 20 September 2016 at 07:55, <vijay.kil...@gmail.com> wrote:
>> From: Vijaya Kumar K <vijaya.ku...@cavium.com>
>>
>> This actually implements pre_save and post_load methods for in-kernel
>> vGICv3.
>>
>> Signed-off-by: Pavel Fedin <p.fe...@samsung.com>
>> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
>> Signed-off-by: Vijaya Kumamr K <vijaya.ku...@cavium.com>
>> [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
