Re: [Qemu-devel] [PATCH V3 2/2] arm_gicv3_kvm: kvm_dist_get/put: skip the registers banked by GICR

[Auger Eric <eric.auger@redhat.com>]

Hi Peter,

On 05/24/2018 04:16 PM, Peter Maydell wrote:
> On 24 May 2018 at 14:59, Auger Eric <eric.auger@redhat.com> wrote:
>> Hi,
>>
>> On 05/24/2018 03:14 PM, Peter Maydell wrote:
>>> On 24 May 2018 at 10:04, Auger Eric <eric.auger@redhat.com> wrote:
>>>> Now I am unclear about the semantics of the s->gicd_ipriority & friends.
>>>> With that change, is it supposed to contain only the states of SPIs or
>>>> contain the RAZ states of PPI/SGIs + states of SPIs. The array is
>>>> dimensionned to contain states for PPI/SGI+SPIs, right? In other words,
>>>> shouldn't we also shift field?
>>>
>>> The semantics of the gicd_ipriority and other data structures are
>>> set by the TCG GIC implementation, and include blank space at
>>> the start where the PPI/SGI bits would live. See this comment
>>> from arm_gicv3_common.h:
>>>
>>>  * Each bitmap contains a bit for each interrupt. Although there is
>>>  * space for the PPIs and SGIs, those bits (the first 32) are never
>>>  * used as that state lives in the redistributor. The unused bits are
>>>  * provided purely so that interrupt X's state is always in bit X; this
>>>  * avoids bugs where we forget to subtract GIC_INTERNAL from an
>>>  * interrupt number.
>>
>> If I understand Shannon's code correctly, the space for PPIs/SGIs is
>> currently overwritten by SPI state, hence my comment.
> 
> Only for KVM, not for TCG, and it's the other way round: we
> end up with two lots of PPI/SGI space in the data structure
> by mistake. Let me fish out the comment I made on the v2 of this
> series:
> 
> In the code in master, we have QEMU data structures
> (bitmaps, etc) which have one entry for each of GICV3_MAXIRQ
> irqs. That includes the RAZ/WI unused space for the SPIs/PPIs, so
> for a 1-bit-per-irq bitmap:
>  [0x00000000, irq 32, irq 33, .... ]
> 
> When we fill in the values from KVM into these data structures,
> we start after the unused space, because the for_each_dist_irq_reg()
> macro starts with _irq = GIC_INTERNAL. But we forgot to adjust
> the offset value we use for the KVM access, so we start by
> reading the RAZ/WI values from KVM, and the data structure
> contents end up with:
>  [0x00000000, 0x00000000, irq 32, irq 33, ... ]
> (and the last irqs wouldn't get transferred).
In kvm_dist_get_priority (new code), the offset is where we read and
field is where we write, correct? Offset was shifted so we effectively
read in KVM regs the num_irq-32 SPI states now but don't we start
writing at the beginning of bmp, (ie s->gicd_ipriority), at PPI/SGI
offset? What am I missing?

I don't understand you TCG remark above, sorry.

Thanks

Eric
> 
> With this change to the code we will get the offset right and
> the data structure will be filled as
>  [0x00000000, irq 32, irq 33, .... ]
> For TCG, where we never had this bug, this is how the data
> structure has always looked.
> 
> But for migration from the old version, the data structure
> we receive from the migration source will contain the old
> broken layout of
>  [0x00000000, 0x00000000, irq 32, irq 33, ... ]
> 
> So we need in inbound migration to identify when we need
> to fix this up (by copying the data down to get rid of that
> extra 0x00000000), which is "when KVM is enabled and the source
> is not a version new enough to have fixed this bug".
> 
>> If we stick to the
>> current semantics, can't we just add the last missing 32 SPI states and
>> we don't need the subsection?
> 
> You need a subsection, because that's how you get migration
> compatibility.
> 
> thanks
> -- PMM
>