Re: [Qemu-devel] [PATCH] KVM: arm/arm64: Add VGICv3 save/restore API documentation

[Pavel Fedin]

 Hello!

 Since we are discussing qemu here, i removed kernel mailing lists and added 
qemu-devel.

> A quick look at your patch suggests you still have data
> structures like
> 
> +struct gicv3_irq_state {
> +    /* The enable bits are only banked for per-cpu interrupts.  */
> +    unsigned long *enabled;
> +    unsigned long *pending;
> +    unsigned long *active;
> +    unsigned long *level;
> +    unsigned long *group;
> +    bool edge_trigger; /* true: edge-triggered, false: level-triggered  */
> +    uint32_t mask_size; /* Size of bitfields in long's, for migration */
> +};
> 
> I think it's probably going to be better to have an array
> of redistributor structures (one per CPU)

 Already done. See struct GICv3CPUState - this is per-CPU data. During realize, 
i allocate an array of these structures and put the pointer into s->cpu.
 GICv3CPUState holds both redistributor and CPU interface data, for simplicity, 
because they come in pairs.

> and then keep
> the state that in hardware is in each redistributor inside
> those sub-structures.

 I can do it easily, but here i left the original layout by Shlomo Pongratz, 
because i thought that it will be easier for his SW emulation code to handle 
it. I remember that he performs some bitwise logic on these bitmasks based on 
CPU masks in order to determine what to do on which CPUs, so i thought that it 
will be better to leave them as they are. These bitfields have one bit per CPU, 
just SPI bits are always set/cleared altogether (i have set_all_cpus() and 
clear_all_cpus() inlines for this purpose).
 I had even better idea of splitting up gicv3_irq_state, and storing only 
per-CPU IRQs in the GICv3CPUState, and SPIs in GICv3State. In this case 
bitfields would be of a fixed size, and one bit would represent one interrupt. 
This would decrease memory usage, because we would not have to duplicate SPI 
bits for every CPU. But will this be good for SW emulation?
 OTOH, i have rechecked current SW emulation code, and i see that it uses 
macros like GIC_TEST_xxx, and cpu mask is always (1 << ncpu). So, can we safely 
replace mask with just CPU number in these macros? It would solve the problem. 
Shlomo, your word?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia