Hi Peter, On 05/24/2018 04:16 PM, Peter Maydell wrote: > 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). 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 >
