From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55979) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c9YA8-0000j1-ME for qemu-devel@nongnu.org; Wed, 23 Nov 2016 09:04:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c9YA4-00054L-JO for qemu-devel@nongnu.org; Wed, 23 Nov 2016 09:04:36 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47108) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c9YA4-00052x-AU for qemu-devel@nongnu.org; Wed, 23 Nov 2016 09:04:32 -0500 From: Auger Eric References: <20161117175752.16475-1-andre.przywara@arm.com> <20161117175752.16475-5-andre.przywara@arm.com> <20161118144153.4cs7lybgvrhjq6uq@kamzik.brq.redhat.com> Message-ID: <3242c90d-dfc0-ff2d-1a01-1d89f0a81844@redhat.com> Date: Wed, 23 Nov 2016 15:04:27 +0100 MIME-Version: 1.0 In-Reply-To: <20161118144153.4cs7lybgvrhjq6uq@kamzik.brq.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [kvm-unit-tests PATCH 4/4] arm/arm64: GICv3: add TYPER test List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andrew Jones , Andre Przywara Cc: Peter Maydell , kvm@vger.kernel.org, Marc Zyngier , qemu-devel@nongnu.org, kvmarm@lists.cs.columbia.edu, Christoffer Dall Hi, On 18/11/2016 15:41, Andrew Jones wrote: > On Thu, Nov 17, 2016 at 05:57:52PM +0000, Andre Przywara wrote: >> Add a simple test for the GICv3 TYPER test, which does only one basic >> check to ensure we have actually enough interrupt IDs if we support >> LPIs. >> Allow a GICv3 guest to do the common MMIO checks as well, where the >> register semantics are shared with a GICv2. >> >> Signed-off-by: Andre Przywara >> --- >> arm/gic.c | 34 +++++++++++++++++++++++++++++++--- >> arm/unittests.cfg | 6 ++++++ >> 2 files changed, 37 insertions(+), 3 deletions(-) >> >> diff --git a/arm/gic.c b/arm/gic.c nit: in the top comments you could add "GICv3 MMIO test" for homogeneity >> index 02b1be1..7de0e47 100644 >> --- a/arm/gic.c >> +++ b/arm/gic.c >> @@ -327,6 +327,30 @@ static bool test_typer_v2(uint32_t reg) >> return true; >> } >> >> +static bool test_typer_v3(uint32_t reg) >> +{ >> + int nr_intids; >> + >> + report("GIC emulation %ssupport%s MBIs", 1, >> + reg & BIT(16) ? "" : "does not ", >> + reg & BIT(16) ? "s" : ""); > > Could just do the test once > > ("...%s...", reg & BIT(16) ? "supports" : "does not support" > >> + report("GIC emulation %ssupport%s LPIs", 1, >> + reg & BIT(17) ? "" : "does not ", >> + reg & BIT(17) ? "s" : ""); >> + report("GIC emulation %ssupport%s Aff3", 1, >> + reg & BIT(24) ? "" : "does not ", >> + reg & BIT(24) ? "s" : ""); >> + >> + nr_intids = BIT(((reg >> 19) & 0x1f) + 1); >> + report("%d interrupt IDs implemented", 1, nr_intids); >> + >> + if (reg & BIT(17)) >> + report("%d LPIs supported", nr_intids > 8192, Maybe I misunderstand the spec but LPIs start at 8192 (>=) and also the spec says that "In an implementation that includes LPIs, at least 8192 LPIs are supported (>= 2x 8192)" Thanks Eric >> + nr_intids > 8192 ? nr_intids - 8192 : 0); > > I'm wondering if we should try to keep the number of report lines > the same host to host. So anywhere we can't do a PASS/FAIL test we > should do a SKIP. Doing that will allow us to cleanly diff test > results between hosts. (I'm not sure I've been doing a good job of > that with the existing tests though...) > >> + >> + return true; > > No need to return a value. > >> +} >> + >> #define BYTE(reg32, byte) (((reg32) >> ((byte) * 8)) & 0xff) >> #define REPLACE_BYTE(reg32, byte, new) (((reg32) & ~(0xff << ((byte) * 8))) |\ >> ((new) << ((byte) * 8))) >> @@ -460,8 +484,9 @@ static int gic_test_mmio(int gic_version) >> idreg = gic_dist_base + 0xfe8; >> break; >> case 0x3: >> - report_abort("GICv3 MMIO tests NYI"); >> - return -1; >> + gic_dist_base = gicv3_dist_base(); >> + idreg = gic_dist_base + 0xffe8; > > No define for this ID reg? > >> + break; >> default: >> report_abort("GIC version %d not supported", gic_version); >> return 0; >> @@ -471,7 +496,10 @@ static int gic_test_mmio(int gic_version) >> nr_irqs = 32 * ((reg & 0x1f) + 1); >> report("number of implemented SPIs: %d", 1, nr_irqs - 32); >> >> - test_typer_v2(reg); >> + if (gic_version == 2) >> + test_typer_v2(reg); >> + else >> + test_typer_v3(reg); > > Maybe we should use a switch here too, preparing for v4 > >> >> report("IIDR: 0x%x", 1, readl(gic_dist_base + GICD_IIDR)); >> >> diff --git a/arm/unittests.cfg b/arm/unittests.cfg >> index 0162e5a..b432346 100644 >> --- a/arm/unittests.cfg >> +++ b/arm/unittests.cfg >> @@ -78,3 +78,9 @@ file = gic.flat >> smp = $MAX_SMP >> extra_params = -machine gic-version=3 -append 'ipi' >> groups = gic >> + >> +[gicv3-mmio] >> +file = gic.flat >> +smp = $MAX_SMP >> +extra_params = -machine gic-version=3 -append 'mmio' >> +groups = gic >> -- >> 2.9.0 >> >> > > Thanks, > drew >