Re: [Qemu-devel] [RFC PATCH v3 1/4] hw/intc/arm_gicv3_common: Add state information

[Pavel Fedin]

 Hello!

 I skipped many of comments because they are straightforward to address, 
decided to discuss only important ones.

> So we're going to (potentially) emulate:
>  * non-system-register config
>  * non-affinity-routing config
> 
> ? OK, but are we really sure we want to do that? Legacy config
> is deprecated and it's really going to complicate the model...

 I remember that you told me that we are going to emulate full-blown GICv3, 
with HYP support and will all legacy stuff. You told me about this while 
merging the initial GICv3 series, so we reserved MMIO space for legacy CPU 
interface. So, i was pretty confident that we are going to do this over time, 
aren't we?

> (Are we starting out with the non-legacy config that forces
> system-regs and affinity-routing to always-on?)

 Yes, we are, but i also remember that you told me that migration data format, 
once implemented, is set in stone, so we should think very well. (*) So far, i 
added also the following legacy stuff:

1. GICC_CTLR

 This is currently used to store GRPEN bits. I thought that having dedicated 
bool's for them is too much, they are just single bits, and they have to be 
mirrored in GICC_CTLR, once implemented. So i just squashed them in there from 
the beginning. Additionally, some of its bits, like FIQBypDisGrpX and 
IRQBypDisGrpX, do not have analogs in system registers. So, if we even 
implement them, we'll have to store them in dedicated place, and now we already 
have this place.

2. GIC_SPENDSGIR (**)

 Actually this is not used by in-kernel vGIC, but this is necessary for SW 
emulation. Because, as far as i can understand Shlomo's code, for software 
emulation we need to track down which source CPUs are sending SGIs, even if we 
don't emulate legacy interface. Because, for example, if two CPUs send an SGI 
to another CPU at the same time, the destination CPU should actually get two 
interrupts. So, we have to track down whose interrupts are already delivered 
and whose are not yet. And Shlomo's code uses a bitmask for that, which i put 
in GICv3SGISource.

3. GICD_ITARGETSR

 Ok, this is actually not used yet, but again, this does not have any analog in 
system register interface, so once we have legacy mode, we have to store it 
somewhere. So i decided to reserve it too, because of (*).

> > +        /* Workaround!
> > +         * Linux (drivers/irqchip/irq-gic-v3.c) is enabling only group one,
> > +         * in gic_cpu_sys_reg_init it calls gic_write_grpen1(1);
> > +         * but it doesn't conigure any interrupt to be in group one.
> > +         * The same for SPIs below
> > +         */
> 
> Is this a bug in Linux, or is it just expecting that firmware configures
> all interrupts into group 1 for it? (ie do we need some variation on
> commit 8ff41f3995ad2d for gicv3 ?)

 To tell the truth i don't know, i left original Shlomo's comment.
 I'm not sure it’s really a bug, i think Linux relies on the firmware. All 
boards i know have some kind of trustzone code, even minimal one, and i believe 
it's its responsibility to set these things up.

> > +uint32_t gicv3_get_igrpen0(GICv3State *s, int cpuindex)
> > +{
> > +    GICv3CPUState *c = &s->cpu[cpuindex];
> > +
> > +    return extract32(c->legacy_ctlr, GICC_CTLR_EN_GRP0_SHIFT, 1);
> > +}
> 
> My gut feeling is that if we alias legacy and system register
> state, then we should do it by having the 'master' copy be
> the system register format.

 Explained above. I could have bool grpen[2], but this would be two 32-bit 
variables, not used for anything else. And legacy_ctlr is a single 32-bit 
variable, which is potentially more functional, and provides better backwards 
compatibility for live migration data format.

> > +struct GICv3SGISource {
> > +    /* For each SGI on the target CPU, we store bit mask
> > +     * indicating which source CPUs have made this SGI
> > +     * pending on the target CPU. These correspond to
> > +     * the bytes in the GIC_SPENDSGIR* registers as
> > +     * read by the target CPU.
> > +     */
> > +    unsigned long *pending;
> > +    int32_t size; /* Bitmap size for migration */
> > +};
> 
> This doesn't look right. There is one GICD_SPENDSGIR* register set
> for each CPU, but each CPU has only 8 pending bits per SGI.
> (That's because this is only relevant for legacy operation
> with affinity-routing disabled, and the limitations on
> legacy operation apply.) So you don't need a huge bitmap here.
> I would default to modelling this as uint32_t spendsgir[4]
> unless there's a good reason to do something else.

 This is about (**). Or do you want to tell that GICv3 with affinity routing 
enabled simply doesn't care about source CPUs, and if several CPUs trigger the 
same SGI for the same destination, the destination gets only one SGI?

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