From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52998) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZqcRM-0001lU-Nj for qemu-devel@nongnu.org; Mon, 26 Oct 2015 03:43:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZqcRH-0006Nv-MW for qemu-devel@nongnu.org; Mon, 26 Oct 2015 03:43:36 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:22628) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZqcRH-0006M0-Ej for qemu-devel@nongnu.org; Mon, 26 Oct 2015 03:43:31 -0400 Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout4.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0NWT00GWZHGFU350@mailout4.w1.samsung.com> for qemu-devel@nongnu.org; Mon, 26 Oct 2015 07:43:27 +0000 (GMT) From: Pavel Fedin References: <5dbabd5337ae85feccb64057cc21e03a1e34132c.1445522263.git.p.fedin@samsung.com> In-reply-to: Date: Mon, 26 Oct 2015 10:43:25 +0300 Message-id: <00f501d10fc1$ffd3a420$ff7aec60$@samsung.com> MIME-version: 1.0 Content-type: text/plain; charset=UTF-8 Content-transfer-encoding: quoted-printable Content-language: ru Subject: Re: [Qemu-devel] [RFC PATCH v3 1/4] hw/intc/arm_gicv3_common: Add state information List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: 'Peter Maydell' Cc: 'Diana Craciun' , 'Shlomo Pongratz' , 'Shlomo Pongratz' , 'QEMU Developers' , 'Vijay Kilari' Hello! I skipped many of comments because they are straightforward to address, = decided to discuss only important ones. > So we're going to (potentially) emulate: > * non-system-register config > * non-affinity-routing config >=20 > ? OK, but are we really sure we want to do that? Legacy config > is deprecated and it's really going to complicate the model... I remember that you told me that we are going to emulate full-blown = GICv3, with HYP support and will all legacy stuff. You told me about = this while merging the initial GICv3 series, so we reserved MMIO space = for legacy CPU interface. So, i was pretty confident that we are going = to do this over time, aren't we? > (Are we starting out with the non-legacy config that forces > system-regs and affinity-routing to always-on?) Yes, we are, but i also remember that you told me that migration data = format, once implemented, is set in stone, so we should think very well. = (*) So far, i added also the following legacy stuff: 1. GICC_CTLR This is currently used to store GRPEN bits. I thought that having = dedicated bool's for them is too much, they are just single bits, and = they have to be mirrored in GICC_CTLR, once implemented. So i just = squashed them in there from the beginning. Additionally, some of its = bits, like FIQBypDisGrpX and IRQBypDisGrpX, do not have analogs in = system registers. So, if we even implement them, we'll have to store = them in dedicated place, and now we already have this place. 2. GIC_SPENDSGIR (**) Actually this is not used by in-kernel vGIC, but this is necessary for = SW emulation. Because, as far as i can understand Shlomo's code, for = software emulation we need to track down which source CPUs are sending = SGIs, even if we don't emulate legacy interface. Because, for example, = if two CPUs send an SGI to another CPU at the same time, the destination = CPU should actually get two interrupts. So, we have to track down whose = interrupts are already delivered and whose are not yet. And Shlomo's = code uses a bitmask for that, which i put in GICv3SGISource. 3. GICD_ITARGETSR Ok, this is actually not used yet, but again, this does not have any = analog in system register interface, so once we have legacy mode, we = have to store it somewhere. So i decided to reserve it too, because of = (*). > > + /* Workaround! > > + * Linux (drivers/irqchip/irq-gic-v3.c) is enabling only = group one, > > + * in gic_cpu_sys_reg_init it calls gic_write_grpen1(1); > > + * but it doesn't conigure any interrupt to be in group = one. > > + * The same for SPIs below > > + */ >=20 > Is this a bug in Linux, or is it just expecting that firmware = configures > all interrupts into group 1 for it? (ie do we need some variation on > commit 8ff41f3995ad2d for gicv3 ?) To tell the truth i don't know, i left original Shlomo's comment. I'm not sure it=E2=80=99s really a bug, i think Linux relies on the = firmware. All boards i know have some kind of trustzone code, even = minimal one, and i believe it's its responsibility to set these things = up. > > +uint32_t gicv3_get_igrpen0(GICv3State *s, int cpuindex) > > +{ > > + GICv3CPUState *c =3D &s->cpu[cpuindex]; > > + > > + return extract32(c->legacy_ctlr, GICC_CTLR_EN_GRP0_SHIFT, 1); > > +} >=20 > My gut feeling is that if we alias legacy and system register > state, then we should do it by having the 'master' copy be > the system register format. Explained above. I could have bool grpen[2], but this would be two = 32-bit variables, not used for anything else. And legacy_ctlr is a = single 32-bit variable, which is potentially more functional, and = provides better backwards compatibility for live migration data format. > > +struct GICv3SGISource { > > + /* For each SGI on the target CPU, we store bit mask > > + * indicating which source CPUs have made this SGI > > + * pending on the target CPU. These correspond to > > + * the bytes in the GIC_SPENDSGIR* registers as > > + * read by the target CPU. > > + */ > > + unsigned long *pending; > > + int32_t size; /* Bitmap size for migration */ > > +}; >=20 > This doesn't look right. There is one GICD_SPENDSGIR* register set > for each CPU, but each CPU has only 8 pending bits per SGI. > (That's because this is only relevant for legacy operation > with affinity-routing disabled, and the limitations on > legacy operation apply.) So you don't need a huge bitmap here. > I would default to modelling this as uint32_t spendsgir[4] > unless there's a good reason to do something else. This is about (**). Or do you want to tell that GICv3 with affinity = routing enabled simply doesn't care about source CPUs, and if several = CPUs trigger the same SGI for the same destination, the destination gets = only one SGI? Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia