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]; > } > } > >
WARNING: multiple messages have this Message-ID (diff)
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]; > } > } > > _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
next prev parent reply other threads:[~2020-12-03 15:00 UTC|newest] Thread overview: 78+ 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 ` 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-11-25 15:51 ` Alexandru Elisei 2020-12-01 16:37 ` Auger Eric 2020-12-01 16:37 ` Auger Eric 2020-12-01 17:37 ` Alexandru Elisei 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-11-25 15:51 ` Alexandru Elisei 2020-12-01 16:37 ` Auger Eric 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-11-25 15:51 ` Alexandru Elisei 2020-12-01 16:37 ` Auger Eric 2020-12-01 16:37 ` Auger Eric 2020-12-02 14:02 ` Alexandru Elisei 2020-12-02 14:02 ` Alexandru Elisei 2020-12-02 14:14 ` Alexandru Elisei 2020-12-02 14:14 ` Alexandru Elisei 2020-12-03 9:41 ` Auger Eric 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-11-25 15:51 ` Alexandru Elisei 2020-12-01 16:48 ` Auger Eric 2020-12-01 16:48 ` Auger Eric 2020-12-02 14:06 ` Alexandru Elisei 2020-12-02 14:06 ` Alexandru Elisei 2020-12-03 13:10 ` Auger Eric 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-11-25 15:51 ` Alexandru Elisei 2020-12-03 13:10 ` Auger Eric 2020-12-03 13:10 ` Auger Eric 2020-12-03 13:21 ` Alexandru Elisei 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-11-25 15:51 ` Alexandru Elisei 2020-12-03 13:10 ` Auger Eric 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-11-25 15:51 ` Alexandru Elisei 2020-12-03 13:21 ` Auger Eric 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-11-25 15:51 ` Alexandru Elisei 2020-12-03 13:39 ` Auger Eric 2020-12-03 13:39 ` Auger Eric 2020-12-10 14:45 ` Alexandru Elisei 2020-12-10 14:45 ` Alexandru Elisei 2020-12-15 13:58 ` Auger Eric 2020-12-15 13:58 ` Auger Eric 2020-12-16 11:40 ` Alexandru Elisei 2020-12-16 11:40 ` Alexandru Elisei 2020-12-16 12:37 ` Auger Eric 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-11-25 15:51 ` Alexandru Elisei 2020-12-03 14:59 ` Auger Eric [this message] 2020-12-03 14:59 ` Auger Eric 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-25 15:51 ` Alexandru Elisei 2020-11-26 9:30 ` Zenghui Yu 2020-11-26 9:30 ` Zenghui Yu 2020-11-27 14:50 ` Alexandru Elisei 2020-11-27 14:50 ` Alexandru Elisei 2020-11-30 13:59 ` Zenghui Yu 2020-11-30 13:59 ` Zenghui Yu 2020-11-30 14:19 ` Alexandru Elisei 2020-11-30 14:19 ` Alexandru Elisei 2020-12-01 15:09 ` Alexandru Elisei 2020-12-01 15:09 ` Alexandru Elisei 2020-11-30 17:48 ` Auger Eric 2020-11-30 17:48 ` Auger Eric 2020-12-03 14:59 ` Auger Eric 2020-12-03 14:59 ` Auger Eric 2020-12-09 10:29 ` Alexandru Elisei 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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.