On 24 May 2018 at 15:40, Auger Eric <eric.au...@redhat.com> wrote: > Hi Peter, > > On 05/24/2018 04:16 PM, Peter Maydell wrote: >> 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?
Oops, yes, you're right. My explanation applies to the various other bitmaps, where we are accessing the fields in the data structure using gic_bmp_ptr32(bmp, irq), but not to gicd_ipriority[], which we are directly accessing starting with the first word, not by indexing via bmp[irq]. So we need to handle these two cases differently. You're correct that for gicd_ipriority[], the code in master reads and writes to that data structure as: [0, 0, ..., 0, irq 32, irq 33, ..., 0, 0, ... 0] so all the values are in the right place but we: (a) unnecessarily read/write zeroes for the PPI/SGI fields (b) fail to transfer the last 32 interrupts We can fix the gicd_ipriority[] case simply by adding bmp = GIC_INTERNAL; before the assignment to 'field' in both kvm_dist_get_priority() and kvm_dist_put_priority(). This doesn't affect migration compatibility. We should do this separately from fixing the other bitmaps, because it's simpler. > I don't understand you TCG remark above, sorry. You can migrate a TCG VM as well as a KVM one. The TCG GICv3 doesn't use any of this code in hw/intc/arm_gicv3_kvm.c, so it doesn't have any of these bugs. So any fixups for the KVM bugs so we get migration correct in the "buggy VM" -> "not buggy VM" situation should not misidentify a TCG VM as "buggy". PS: I have a feeling that kvm_dist_get/set_priority have an endianness problem -- the 'reg' we read from the kernel will be a 32-bit value with the priority byte for the lowest-numbered IRQ in its least significant byte, but if the host is big-endian we'll write that to the wrong offset in the gicd_priority[] array... thanks -- PMM
