From: Auger Eric <eric.auger@redhat.com>
To: Alexandru Elisei <alexandru.elisei@arm.com>,
kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
drjones@redhat.com
Cc: andre.przywara@arm.com
Subject: Re: [kvm-unit-tests PATCH 09/10] arm/arm64: gic: Make check_acked() more generic
Date: Thu, 3 Dec 2020 15:59:19 +0100 [thread overview]
Message-ID: <d187299c-de94-bb1b-496a-0d552a3ae601@redhat.com> (raw)
In-Reply-To: <20201125155113.192079-10-alexandru.elisei@arm.com>
Hi,
On 11/25/20 4:51 PM, Alexandru Elisei wrote:
> Testing that an interrupt is received as expected is done in three places:
> in check_ipi_sender(), check_irqnr() and check_acked(). check_irqnr()
> compares the interrupt ID with IPI_IRQ and records a failure in bad_irq,
> and check_ipi_sender() compares the sender with IPI_SENDER and writes to
> bad_sender when they don't match.
>
> Let's move all the checks to check_acked() by renaming
> bad_sender->irq_sender and bad_irq->irq_number and changing their semantics
> so they record the interrupt sender, respectively the irq number.
> check_acked() now takes two new parameters: the expected interrupt number
> and sender.
>
> This has two distinct advantages:
>
> 1. check_acked() and ipi_handler() can now be used for interrupts other
> than IPIs.
> 2. Correctness checks are consolidated in one function.
>
> CC: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
> ---
> arm/gic.c | 68 +++++++++++++++++++++++++++----------------------------
> 1 file changed, 33 insertions(+), 35 deletions(-)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index dcdab7d5f39a..da7b42da5449 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -35,7 +35,7 @@ struct gic {
>
> static struct gic *gic;
> static int acked[NR_CPUS], spurious[NR_CPUS];
> -static int bad_sender[NR_CPUS], bad_irq[NR_CPUS];
> +static int irq_sender[NR_CPUS], irq_number[NR_CPUS];
> static cpumask_t ready;
>
> static void nr_cpu_check(int nr)
> @@ -57,8 +57,8 @@ static void stats_reset(void)
>
> for (i = 0; i < nr_cpus; ++i) {
> acked[i] = 0;
> - bad_sender[i] = -1;
> - bad_irq[i] = -1;
> + irq_sender[i] = -1;
> + irq_number[i] = -1;
> }
> }
>
> @@ -92,9 +92,10 @@ static void wait_for_interrupts(cpumask_t *mask)
> report_info("interrupts timed-out (5s)");
> }
>
> -static bool check_acked(cpumask_t *mask)
> +static bool check_acked(cpumask_t *mask, int sender, int irqnum)
> {
> int missing = 0, extra = 0, unexpected = 0;
> + bool has_gicv2 = (gic_version() == 2);
> bool pass = true;
> int cpu;
>
> @@ -108,17 +109,19 @@ static bool check_acked(cpumask_t *mask)
> if (acked[cpu])
> ++unexpected;
> }
> + if (!acked[cpu])
> + continue;
> smp_rmb(); /* pairs with smp_wmb in ipi_handler */
>
> - if (bad_sender[cpu] != -1) {
> + if (has_gicv2 && irq_sender[cpu] != sender) {
> report_info("cpu%d received IPI from wrong sender %d",
> - cpu, bad_sender[cpu]);
> + cpu, irq_sender[cpu]);
> pass = false;
> }
>
> - if (bad_irq[cpu] != -1) {
> + if (irq_number[cpu] != irqnum) {
> report_info("cpu%d received wrong irq %d",
> - cpu, bad_irq[cpu]);
> + cpu, irq_number[cpu]);
> pass = false;
> }
> }
> @@ -143,26 +146,18 @@ static void check_spurious(void)
> }
> }
>
> -static void check_ipi_sender(u32 irqstat, int sender)
> +static int gic_get_sender(int irqstat)
> {
> - if (gic_version() == 2) {
> - int src = (irqstat >> 10) & 7;
> -
> - if (src != sender)
> - bad_sender[smp_processor_id()] = src;
> - }
> -}
> -
> -static void check_irqnr(u32 irqnr)
> -{
> - if (irqnr != IPI_IRQ)
> - bad_irq[smp_processor_id()] = irqnr;
> + if (gic_version() == 2)
> + return (irqstat >> 10) & 7;
> + return -1;
> }
>
> static void ipi_handler(struct pt_regs *regs __unused)
> {
> u32 irqstat = gic_read_iar();
> u32 irqnr = gic_iar_irqnr(irqstat);
> + int this_cpu = smp_processor_id();
>
> if (irqnr != GICC_INT_SPURIOUS) {
> gic_write_eoir(irqstat);
> @@ -173,12 +168,12 @@ static void ipi_handler(struct pt_regs *regs __unused)
> */
> if (gic_version() == 2)
> smp_rmb();
> - check_ipi_sender(irqstat, IPI_SENDER);
> - check_irqnr(irqnr);
> + irq_sender[this_cpu] = gic_get_sender(irqstat);
> + irq_number[this_cpu] = irqnr;
> smp_wmb(); /* pairs with smp_rmb in check_acked */
> - ++acked[smp_processor_id()];
> + ++acked[this_cpu];
> } else {
> - ++spurious[smp_processor_id()];
> + ++spurious[this_cpu];
> }
>
> /* Wait for writes to acked/spurious to complete */
> @@ -311,40 +306,42 @@ static void gicv3_ipi_send_broadcast(void)
>
> static void ipi_test_self(void)
> {
> + int this_cpu = smp_processor_id();
> cpumask_t mask;
>
> report_prefix_push("self");
> stats_reset();
> cpumask_clear(&mask);
> - cpumask_set_cpu(smp_processor_id(), &mask);
> + cpumask_set_cpu(this_cpu, &mask);
> gic->ipi.send_self();
> wait_for_interrupts(&mask);
> - report(check_acked(&mask), "Interrupts received");
> + report(check_acked(&mask, this_cpu, IPI_IRQ), "Interrupts received");
> report_prefix_pop();
> }
>
> static void ipi_test_smp(void)
> {
> + int this_cpu = smp_processor_id();
> cpumask_t mask;
> int i;
>
> report_prefix_push("target-list");
> stats_reset();
> cpumask_copy(&mask, &cpu_present_mask);
> - for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
> + for (i = this_cpu & 1; i < nr_cpus; i += 2)
> cpumask_clear_cpu(i, &mask);
> gic_ipi_send_mask(IPI_IRQ, &mask);
> wait_for_interrupts(&mask);
> - report(check_acked(&mask), "Interrupts received");
> + report(check_acked(&mask, this_cpu, IPI_IRQ), "Interrupts received");
> report_prefix_pop();
>
> report_prefix_push("broadcast");
> stats_reset();
> cpumask_copy(&mask, &cpu_present_mask);
> - cpumask_clear_cpu(smp_processor_id(), &mask);
> + cpumask_clear_cpu(this_cpu, &mask);
> gic->ipi.send_broadcast();
> wait_for_interrupts(&mask);
> - report(check_acked(&mask), "Interrupts received");
> + report(check_acked(&mask, this_cpu, IPI_IRQ), "Interrupts received");
> report_prefix_pop();
> }
>
> @@ -393,6 +390,7 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
> {
> u32 irqstat = gic_read_iar();
> u32 irqnr = gic_iar_irqnr(irqstat);
> + int this_cpu = smp_processor_id();
>
> if (irqnr != GICC_INT_SPURIOUS) {
> void *base;
> @@ -405,11 +403,11 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>
> writel(val, base + GICD_ICACTIVER);
>
> - check_ipi_sender(irqstat, smp_processor_id());
> - check_irqnr(irqnr);
> - ++acked[smp_processor_id()];
> + irq_sender[this_cpu] = gic_get_sender(irqstat);
> + irq_number[this_cpu] = irqnr;
> + ++acked[this_cpu];
> } else {
> - ++spurious[smp_processor_id()];
> + ++spurious[this_cpu];
> }
> }
>
>
next prev parent reply other threads:[~2020-12-03 15:00 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-25 15:51 [kvm-unit-tests PATCH 00/10] GIC fixes and improvements Alexandru Elisei
2020-11-25 15:51 ` [kvm-unit-tests PATCH 01/10] lib: arm/arm64: gicv3: Add missing barrier when sending IPIs Alexandru Elisei
2020-12-01 16:37 ` Auger Eric
2020-12-01 17:37 ` Alexandru Elisei
2020-11-25 15:51 ` [kvm-unit-tests PATCH 02/10] lib: arm/arm64: gicv2: " Alexandru Elisei
2020-12-01 16:37 ` Auger Eric
2020-11-25 15:51 ` [kvm-unit-tests PATCH 03/10] arm/arm64: gic: Remove memory synchronization from ipi_clear_active_handler() Alexandru Elisei
2020-12-01 16:37 ` Auger Eric
2020-12-02 14:02 ` Alexandru Elisei
2020-12-02 14:14 ` Alexandru Elisei
2020-12-03 9:41 ` Auger Eric
2020-11-25 15:51 ` [kvm-unit-tests PATCH 04/10] arm/arm64: gic: Remove unnecessary synchronization with stats_reset() Alexandru Elisei
2020-12-01 16:48 ` Auger Eric
2020-12-02 14:06 ` Alexandru Elisei
2020-12-03 13:10 ` Auger Eric
2020-11-25 15:51 ` [kvm-unit-tests PATCH 05/10] arm/arm64: gic: Use correct memory ordering for the IPI test Alexandru Elisei
2020-12-03 13:10 ` Auger Eric
2020-12-03 13:21 ` Alexandru Elisei
2020-11-25 15:51 ` [kvm-unit-tests PATCH 06/10] arm/arm64: gic: Check spurious and bad_sender in the active test Alexandru Elisei
2020-12-03 13:10 ` Auger Eric
2020-11-25 15:51 ` [kvm-unit-tests PATCH 07/10] arm/arm64: gic: Wait for writes to acked or spurious to complete Alexandru Elisei
2020-12-03 13:21 ` Auger Eric
2020-11-25 15:51 ` [kvm-unit-tests PATCH 08/10] arm/arm64: gic: Split check_acked() into two functions Alexandru Elisei
2020-12-03 13:39 ` Auger Eric
2020-12-10 14:45 ` Alexandru Elisei
2020-12-15 13:58 ` Auger Eric
2020-12-16 11:40 ` Alexandru Elisei
2020-12-16 12:37 ` Auger Eric
2020-11-25 15:51 ` [kvm-unit-tests PATCH 09/10] arm/arm64: gic: Make check_acked() more generic Alexandru Elisei
2020-12-03 14:59 ` Auger Eric [this message]
2020-11-25 15:51 ` [kvm-unit-tests PATCH 10/10] arm64: gic: Use IPI test checking for the LPI tests Alexandru Elisei
2020-11-26 9:30 ` Zenghui Yu
2020-11-27 14:50 ` Alexandru Elisei
2020-11-30 13:59 ` Zenghui Yu
2020-11-30 14:19 ` Alexandru Elisei
2020-12-01 15:09 ` Alexandru Elisei
2020-11-30 17:48 ` Auger Eric
2020-12-03 14:59 ` Auger Eric
2020-12-09 10:29 ` Alexandru Elisei
Reply instructions:
You may reply publicly 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=d187299c-de94-bb1b-496a-0d552a3ae601@redhat.com \
--to=eric.auger@redhat.com \
--cc=alexandru.elisei@arm.com \
--cc=andre.przywara@arm.com \
--cc=drjones@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).