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

[Peter Maydell]

On 26 October 2015 at 07:43, Pavel Fedin <address@hidden> wrote:
>  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?

Yeah, you're right -- we should at least leave ourselves the room
to do that.

>> (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.

You'll see from a comment I made in a later patch that we can have
optional migration subsections, so we can have the data which is
only needed in legacy configs be in its own subsection which won't
break migration for non-legacy setups. It's the non-legacy used by
KVM format we really need to get right from the start.

> (*) 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.

The only reason not to want to do this is that it means we have
legacy state in the not-legacy parts of the migration struct...

> 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.

I haven't looked into the details but this suggests to me that the
emulation code is doing something wrong. The GICv3 registers should
expose (one way or another) all the state in hardware, and a sw
emulation version should not need any extra state in order to behave
correctly.

>> > +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?

There is no hidden state in the GIC. If a non-legacy GICv3 has
no register state to store information about source CPUs, then
it cannot care about source CPUs. (For instance, note that the
OS is not passed info about SGI source CPUs in the INTID bits
[12:10] on reads of GICC_IAR if affinity routing is enabled.)
One of the GICv2-to-GICv3+affinity changes is that in GICv3
SGIs were banked by both source and destination processor;
in GICv3+affinity-routing, they are banked only by destination
processor.

thanks
-- PMM