On 24 May 2018 at 14:59, Auger Eric <eric.au...@redhat.com> wrote: > Hi, > > On 05/24/2018 03:14 PM, Peter Maydell wrote: >> On 24 May 2018 at 10:04, Auger Eric <eric.au...@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). 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
