KVM ARM Archive on lore.kernel.org
 help / color / Atom feed
From: Alexandru Elisei <alexandru.elisei@arm.com>
To: Andre Przywara <andre.przywara@arm.com>,
	Andrew Jones <drjones@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Cc: Marc Zyngier <maz@kernel.org>,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org
Subject: Re: [kvm-unit-tests PATCH 06/17] arm: gic: Add simple shared IRQ test
Date: Tue, 12 Nov 2019 13:54:22 +0000
Message-ID: <de22edec-e562-426d-4d87-dd188d6d75de@arm.com> (raw)
In-Reply-To: <20191108144240.204202-7-andre.przywara@arm.com>

Hi,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> So far we were testing the GIC's MMIO interface and IPI delivery.
> Add a simple test to trigger a shared IRQ (SPI), using the ISPENDR
> register in the (emulated) GIC distributor.
> This tests configuration of an IRQ (target CPU) and whether it can be
> properly enabled or disabled.
>
> This is a bit more sophisticated than actually needed at this time,
> but paves the way for extending this to FIQs in the future.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c         | 79 +++++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg | 12 +++++++
>  2 files changed, 91 insertions(+)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index c909668..3be76cb 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -546,6 +546,81 @@ static void gic_test_mmio(void)
>  		test_targets(nr_irqs);
>  }
>  
> +static void gic_spi_trigger(int irq)
> +{
> +	gic_set_irq_bit(irq, GICD_ISPENDR);
> +}
> +
> +static void spi_configure_irq(int irq, int cpu)

The function name is slightly misleading - it might be interpreted as configure
irq to target cpu. How about spi_enable_irq?

> +{
> +	gic_set_irq_target(irq, cpu);
> +	gic_set_irq_priority(irq, 0xa0);

It might be worth adding a comment here stating why you have chosen this priority
over priority 0.

> +	gic_enable_irq(irq);
> +}
> +
> +#define IRQ_STAT_NONE		0
> +#define IRQ_STAT_IRQ		1
> +#define IRQ_STAT_TYPE_MASK	0x3
> +#define IRQ_STAT_NO_CLEAR	4
> +
> +/*
> + * Wait for an SPI to fire (or not) on a certain CPU.
> + * Clears the pending bit if requested afterwards.
> + */
> +static void trigger_and_check_spi(const char *test_name,
> +				  unsigned int irq_stat,
> +				  int cpu)
> +{
> +	cpumask_t cpumask;
> +
> +	stats_reset();
> +	gic_spi_trigger(SPI_IRQ);
> +	cpumask_clear(&cpumask);
> +	switch (irq_stat & IRQ_STAT_TYPE_MASK) {
> +	case IRQ_STAT_NONE:
> +		break;
> +	case IRQ_STAT_IRQ:
> +		cpumask_set_cpu(cpu, &cpumask);
> +		break;
> +	}
> +
> +	check_acked(test_name, &cpumask);
> +
> +	/* Clean up pending bit in case this IRQ wasn't taken. */
> +	if (!(irq_stat & IRQ_STAT_NO_CLEAR))
> +		gic_set_irq_bit(SPI_IRQ, GICD_ICPENDR);

There's only one call site which sets this flag. How about you remove the flag
define and the above two lines of code and replace them with one line of code at
the call site? And if you do that, you can turn the flags into proper enums.

> +}
> +
> +static void spi_test_single(void)
> +{
> +	cpumask_t cpumask;
> +	int cpu = smp_processor_id();
> +
> +	spi_configure_irq(SPI_IRQ, cpu);
> +
> +	trigger_and_check_spi("SPI triggered by CPU write", IRQ_STAT_IRQ, cpu);
> +
> +	gic_disable_irq(SPI_IRQ);
> +	trigger_and_check_spi("disabled SPI does not fire",
> +			      IRQ_STAT_NONE | IRQ_STAT_NO_CLEAR, cpu);
> +
> +	stats_reset();
> +	cpumask_clear(&cpumask);
> +	cpumask_set_cpu(cpu, &cpumask);
> +	gic_enable_irq(SPI_IRQ);
> +	check_acked("now enabled SPI fires", &cpumask);
> +}
> +
> +static void spi_send(void)
> +{
> +	irqs_enable();
> +
> +	spi_test_single();

I suggest your rename this to spi_test_self, to match ipi_test_self.

> +
> +	check_spurious();
> +	exit(report_summary());
> +}
> +
>  int main(int argc, char **argv)
>  {
>  	if (!gic_init()) {
> @@ -577,6 +652,10 @@ int main(int argc, char **argv)
>  		report_prefix_push(argv[1]);
>  		gic_test_mmio();
>  		report_prefix_pop();
> +	} else if (strcmp(argv[1], "irq") == 0) {
> +		report_prefix_push(argv[1]);
> +		spi_send();
> +		report_prefix_pop();
>  	} else {
>  		report_abort("Unknown subtest '%s'", argv[1]);
>  	}
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index 12ac142..7a78275 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -92,6 +92,18 @@ smp = $MAX_SMP
>  extra_params = -machine gic-version=3 -append 'ipi'
>  groups = gic
>  
> +[gicv2-spi]
> +file = gic.flat
> +smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
> +extra_params = -machine gic-version=2 -append 'irq'

I think 'spi' as a command line parameter makes more sense for the gic{v2,v3}-spi
tests.

Thanks,
Alex
> +groups = gic
> +
> +[gicv3-spi]
> +file = gic.flat
> +smp = $MAX_SMP
> +extra_params = -machine gic-version=3 -append 'irq'
> +groups = gic
> +
>  [gicv2-max-mmio]
>  file = gic.flat
>  smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

  reply index

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-08 14:42 [kvm-unit-tests PATCH 00/17] arm: gic: Test SPIs and interrupt groups Andre Przywara
2019-11-08 14:42 ` [kvm-unit-tests PATCH 01/17] arm: gic: Enable GIC MMIO tests for GICv3 as well Andre Przywara
2019-11-08 17:28   ` Alexandru Elisei
2019-11-12 12:49   ` Auger Eric
2019-11-08 14:42 ` [kvm-unit-tests PATCH 02/17] arm: gic: Generalise function names Andre Przywara
2019-11-12 11:11   ` Alexandru Elisei
2019-11-12 12:49   ` Auger Eric
2019-11-08 14:42 ` [kvm-unit-tests PATCH 03/17] arm: gic: Provide per-IRQ helper functions Andre Przywara
2019-11-12 12:51   ` Alexandru Elisei
2019-11-12 15:53     ` Auger Eric
2019-11-12 16:53       ` Alexandru Elisei
2019-11-12 13:49   ` Auger Eric
2019-11-08 14:42 ` [kvm-unit-tests PATCH 04/17] arm: gic: Support no IRQs test case Andre Przywara
2019-11-12 13:26   ` Alexandru Elisei
2019-11-12 21:14     ` Auger Eric
2019-11-08 14:42 ` [kvm-unit-tests PATCH 05/17] arm: gic: Prepare IRQ handler for handling SPIs Andre Przywara
2019-11-12 13:36   ` Alexandru Elisei
2019-11-12 20:56   ` Auger Eric
2019-11-08 14:42 ` [kvm-unit-tests PATCH 06/17] arm: gic: Add simple shared IRQ test Andre Przywara
2019-11-12 13:54   ` Alexandru Elisei [this message]
2019-11-08 14:42 ` [kvm-unit-tests PATCH 07/17] arm: gic: Extend check_acked() to allow silent call Andre Przywara
2019-11-12 15:23   ` Alexandru Elisei
2019-11-14 12:32     ` Andrew Jones
2019-11-15 11:32       ` Alexandru Elisei
2019-11-08 14:42 ` [kvm-unit-tests PATCH 08/17] arm: gic: Add simple SPI MP test Andre Przywara
2019-11-12 15:41   ` Alexandru Elisei
2019-11-08 14:42 ` [kvm-unit-tests PATCH 09/17] arm: gic: Add test for flipping GICD_CTLR.DS Andre Przywara
2019-11-12 16:42   ` Alexandru Elisei
2019-11-14 13:39     ` Vladimir Murzin
2019-11-14 14:17       ` Andre Przywara
2019-11-14 14:50         ` Vladimir Murzin
2019-11-14 15:21           ` Alexandru Elisei
2019-11-14 15:27             ` Peter Maydell
2019-11-14 15:47               ` Alexandru Elisei
2019-11-14 15:56                 ` Peter Maydell
2019-11-08 14:42 ` [kvm-unit-tests PATCH 10/17] arm: gic: Check for writable IGROUPR registers Andre Przywara
2019-11-12 16:51   ` Alexandru Elisei
2019-11-08 14:42 ` [kvm-unit-tests PATCH 11/17] arm: gic: Check for validity of both group enable bits Andre Przywara
2019-11-12 16:58   ` Alexandru Elisei
2019-11-08 14:42 ` [kvm-unit-tests PATCH 12/17] arm: gic: Change gic_read_iar() to take group parameter Andre Przywara
2019-11-12 17:19   ` Alexandru Elisei
2019-11-14 12:50     ` Andrew Jones
2019-11-08 14:42 ` [kvm-unit-tests PATCH 13/17] arm: gic: Change write_eoir() " Andre Przywara
2019-11-08 14:42 ` [kvm-unit-tests PATCH 14/17] arm: gic: Prepare for receiving GIC group 0 interrupts via FIQs Andre Przywara
2019-11-12 17:30   ` Alexandru Elisei
2019-11-08 14:42 ` [kvm-unit-tests PATCH 15/17] arm: gic: Provide FIQ handler Andre Przywara
2019-11-13 10:14   ` Alexandru Elisei
2019-11-08 14:42 ` [kvm-unit-tests PATCH 16/17] arm: gic: Prepare interrupt statistics for both groups Andre Przywara
2019-11-13 10:44   ` Alexandru Elisei
2019-11-08 14:42 ` [kvm-unit-tests PATCH 17/17] arm: gic: Test Group0 SPIs Andre Przywara
2019-11-13 11:26   ` Alexandru Elisei

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=de22edec-e562-426d-4d87-dd188d6d75de@arm.com \
    --to=alexandru.elisei@arm.com \
    --cc=andre.przywara@arm.com \
    --cc=drjones@redhat.com \
    --cc=kvm@vger.kernel.org \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=maz@kernel.org \
    --cc=pbonzini@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

KVM ARM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvmarm/0 kvmarm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvmarm kvmarm/ https://lore.kernel.org/kvmarm \
		kvmarm@lists.cs.columbia.edu
	public-inbox-index kvmarm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/edu.columbia.cs.lists.kvmarm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git