From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35840) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fLrQW-0000yc-4M for qemu-devel@nongnu.org; Thu, 24 May 2018 10:41:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fLrQS-00028N-15 for qemu-devel@nongnu.org; Thu, 24 May 2018 10:41:12 -0400 References: <1527047633-12368-1-git-send-email-zhaoshenglong@huawei.com> <1527047633-12368-2-git-send-email-zhaoshenglong@huawei.com> <10801e6c-5028-add6-b082-22c5dc9758ca@redhat.com> From: Auger Eric Message-ID: <38aee779-1baf-ab96-7489-0f34bda2f8e6@redhat.com> Date: Thu, 24 May 2018 16:40:58 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V3 2/2] arm_gicv3_kvm: kvm_dist_get/put: skip the registers banked by GICR List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Shannon Zhao , qemu-arm , QEMU Developers , Shannon Zhao Hi Peter, On 05/24/2018 04:16 PM, Peter Maydell wrote: > On 24 May 2018 at 14:59, Auger Eric wrote: >> Hi, >> >> On 05/24/2018 03:14 PM, Peter Maydell wrote: >>> On 24 May 2018 at 10:04, Auger Eric 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 >