From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37091) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c5DcI-0001o1-G1 for qemu-devel@nongnu.org; Fri, 11 Nov 2016 10:19:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c5DcF-0006uJ-9f for qemu-devel@nongnu.org; Fri, 11 Nov 2016 10:19:46 -0500 References: <1478798481-25030-1-git-send-email-drjones@redhat.com> <1478798481-25030-11-git-send-email-drjones@redhat.com> <874m3f8621.fsf@linaro.org> <20161110203713.4idzgs5u2xm7ms4o@hawk.localdomain> <871syi8hbg.fsf@linaro.org> <20161111135421.5oxdkfqynkgipgom@hawk.localdomain> <87twbe6pah.fsf@linaro.org> From: Andre Przywara Message-ID: Date: Fri, 11 Nov 2016 15:20:15 +0000 MIME-Version: 1.0 In-Reply-To: <87twbe6pah.fsf@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [kvm-unit-tests PATCH v5 10/11] arm/arm64: gicv3: add an IPI test List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Alex_Benn=c3=a9e?= , Andrew Jones Cc: peter.maydell@linaro.org, kvm@vger.kernel.org, marc.zyngier@arm.com, qemu-devel@nongnu.org, eric.auger@redhat.com, qemu-arm@nongnu.org, pbonzini@redhat.com, kvmarm@lists.cs.columbia.edu, christoffer.dall@linaro.org Hi, On 11/11/16 14:53, Alex Benn=C3=A9e wrote: >=20 > Andrew Jones writes: >=20 >> On Fri, Nov 11, 2016 at 10:02:59AM +0000, Alex Benn=C3=A9e wrote: >>> >>> Andrew Jones writes: >>> >>>> On Thu, Nov 10, 2016 at 07:53:58PM +0000, Alex Benn=C3=A9e wrote: >>>> [...] >>>>>> +struct gic gicv2 =3D { >>>>>> + .ipi =3D { >>>>>> + .enable =3D gicv2_enable_defaults, >>>>>> + .send_self =3D gicv2_ipi_send_self, >>>>>> + .send_tlist =3D gicv2_ipi_send_tlist, >>>>>> + .send_broadcast =3D gicv2_ipi_send_broadcast, >>>>>> + }, >>>>>> + .read_iar =3D gicv2_read_iar, >>>>>> + .irqnr =3D gicv2_irqnr, >>>>>> + .write_eoi =3D gicv2_write_eoi, >>>>>> +}; >>>>>> + >>>>>> +struct gic gicv3 =3D { >>>>>> + .ipi =3D { >>>>>> + .enable =3D gicv3_enable_defaults, >>>>>> + .send_self =3D gicv3_ipi_send_self, >>>>>> + .send_tlist =3D gicv3_ipi_send_tlist, >>>>>> + .send_broadcast =3D gicv3_ipi_send_broadcast, >>>>>> + }, >>>>>> + .read_iar =3D gicv3_read_iar, >>>>>> + .irqnr =3D gicv3_irqnr, >>>>>> + .write_eoi =3D gicv3_write_eoir, >>>>>> +}; >>>>>> + >>>>> >>>>> So I was re-basing my kvm-unit-tests against your GIC rework and fo= und >>>>> myself copy and pasting a bunch of this into my tests that fire IRQ= s. >>>>> That makes me think the abstraction should be in the library code s= o >>>>> other tests can fiddle with sending IRQs. >>>>> >>>>> What do you think? >>>>> >>>> >>>> I guess you mean moving the above two structs and their correspondin= g >>>> functions (all which aren't already common) to lib/arm/ ? Or do you >>>> just mean the one non-trivial function gicv3_ipi_send_tlist? I think >>>> agree with gicv3_ipi_send_tlist getting shared, but the others are >>>> mostly one-liners, so I'm not sure. I guess I'd have to see how you'= re >>>> using them first. >>> >>> So it looked like there were some functions in the common code for on= e >>> GIC which had local test defined functions for the other. They should= at >>> least be consistent. >> >> gicv3_read_iar and gicv3_write_eoir being common already is a product = of >> being sysreg wrappers, allowing for both arm32 and arm64 to use functi= ons >> of the same names, not because I wanted gicv3 to be inconsistent with >> gicv2 (which uses MMIO and thus doesn't need wrappers) >> >>> >>> For my use case I could do with a common: >>> >>> gic_enable >> >> OK, I can extend gic_init() to initialize a 'struct gic_common_ops' th= at >> includes an enable -> *_enable_defaults(void), ipi_send(int cpu), >> read_iar(void), iar_irqnr(u32 iar), and write_eoi(u32 irqstat). And al= so >> provide the wrappers gic_enable, gic_ipi_send(cpu), ... >> >>> gic_send_spi(cpu, irq) >> >> I'll let you add this one to the new common ops struct :-) >> >>> gic_irq_ack() which returns the iar. >> >> This one will be called read_iar. >> >> Would that work for you, Alex? >=20 > Sounds good to me :-) >=20 >> >> Andre, >> >> Would this also satisfy your needs for more common code? TBH I haven't look deeply enough to give an educated answer. I just wanted to +1 Alex for the general direction. So I guess it's OK. ;-) I guess we may need some refactoring later anyway, so any missing pieces can be added/refactored later once we exactly know what we need. Cheers, Andre.