KVM ARM Archive on lore.kernel.org
 help / color / Atom feed
From: Andrew Jones <drjones@redhat.com>
To: Zenghui Yu <yuzenghui@huawei.com>
Cc: andre.przywara@arm.com, kvmarm@lists.cs.columbia.edu,
	kvm@vger.kernel.org
Subject: Re: [PATCH kvm-unit-tests v2] arm64: timer: Speed up gic-timer-state check
Date: Tue, 11 Feb 2020 16:41:35 +0100
Message-ID: <20200211154135.vxxkpstt4cpoyqsp@kamzik.brq.redhat.com> (raw)
In-Reply-To: <60c6c4c7-1d6b-5b64-adc1-8e96f45332c6@huawei.com>

On Tue, Feb 11, 2020 at 10:50:58PM +0800, Zenghui Yu wrote:
> Hi Drew,
> 
> On 2020/2/11 21:37, Andrew Jones wrote:
> > Let's bail out of the wait loop if we see the expected state
> > to save over six seconds of run time. Make sure we wait a bit
> > before reading the registers and double check again after,
> > though, to somewhat mitigate the chance of seeing the expected
> > state by accident.
> > 
> > We also take this opportunity to push more IRQ state code to
> > the library.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> 
> [...]
> 
> > +
> > +enum gic_irq_state gic_irq_state(int irq)
> 
> This is a *generic* name while this function only deals with PPI.
> Maybe we can use something like gic_ppi_state() instead?  Or you
> will have to take all interrupt types into account in a single
> function, which is not a easy job I think.

Very good point.

> 
> > +{
> > +	enum gic_irq_state state;
> > +	bool pending = false, active = false;
> > +	void *base;
> > +
> > +	assert(gic_common_ops);
> > +
> > +	switch (gic_version()) {
> > +	case 2:
> > +		base = gicv2_dist_base();
> > +		pending = readl(base + GICD_ISPENDR) & (1 << PPI(irq));
> > +		active = readl(base + GICD_ISACTIVER) & (1 << PPI(irq));
> > +		break;
> > +	case 3:
> > +		base = gicv3_sgi_base();
> > +		pending = readl(base + GICR_ISPENDR0) & (1 << PPI(irq));
> > +		active = readl(base + GICR_ISACTIVER0) & (1 << PPI(irq));
> 
> And you may also want to ensure that the 'irq' is valid for PPI().
> Or personally, I'd just use a real PPI number (PPI(info->irq)) as
> the input parameter of this function.

Indeed, if we want to make this a general function we should require
the caller to pass PPI(irq).

> 
> > +		break;
> > +	}
> > +
> > +	if (!active && !pending)
> > +		state = GIC_IRQ_STATE_INACTIVE;
> > +	if (pending)
> > +		state = GIC_IRQ_STATE_PENDING;
> > +	if (active)
> > +		state = GIC_IRQ_STATE_ACTIVE;
> > +	if (active && pending)
> > +		state = GIC_IRQ_STATE_ACTIVE_PENDING;
> > +
> > +	return state;
> > +}
> > 
> 
> Otherwise,
> 
> Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
> Tested-by: Zenghui Yu <yuzenghui@huawei.com>

Thanks, but I guess I should squash in changes to make this function more
general. My GIC/SPI skills are weak, so I'm not sure this is right,
especially because the SPI stuff doesn't yet have a user to validate it.
However, if all reviewers think it's correct, then I'll squash it into
the arm/queue branch. I've added Andre and Eric to help review too.

Thanks,
drew


diff --git a/arm/timer.c b/arm/timer.c
index ae5fdbf54b35..44621b4f2967 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -189,9 +189,9 @@ static bool gic_timer_check_state(struct timer_info *info,
 	/* Wait for up to 1s for the GIC to sample the interrupt. */
 	for (i = 0; i < 10; i++) {
 		mdelay(100);
-		if (gic_irq_state(info->irq) == expected_state) {
+		if (gic_irq_state(PPI(info->irq)) == expected_state) {
 			mdelay(100);
-			if (gic_irq_state(info->irq) == expected_state)
+			if (gic_irq_state(PPI(info->irq)) == expected_state)
 				return true;
 		}
 	}
diff --git a/lib/arm/gic.c b/lib/arm/gic.c
index 0563b31132c8..3924dd1d1ee6 100644
--- a/lib/arm/gic.c
+++ b/lib/arm/gic.c
@@ -150,22 +150,37 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
 enum gic_irq_state gic_irq_state(int irq)
 {
 	enum gic_irq_state state;
-	bool pending = false, active = false;
-	void *base;
+	void *ispendr, *isactiver;
+	bool pending, active;
 
 	assert(gic_common_ops);
 
 	switch (gic_version()) {
 	case 2:
-		base = gicv2_dist_base();
-		pending = readl(base + GICD_ISPENDR) & (1 << PPI(irq));
-		active = readl(base + GICD_ISACTIVER) & (1 << PPI(irq));
+		ispendr = gicv2_dist_base() + GICD_ISPENDR;
+		isactiver = gicv2_dist_base() + GICD_ISACTIVER;
 		break;
 	case 3:
-		base = gicv3_sgi_base();
-		pending = readl(base + GICR_ISPENDR0) & (1 << PPI(irq));
-		active = readl(base + GICR_ISACTIVER0) & (1 << PPI(irq));
+		if (irq < GIC_NR_PRIVATE_IRQS) {
+			ispendr = gicv3_sgi_base() + GICR_ISPENDR0;
+			isactiver = gicv3_sgi_base() + GICR_ISACTIVER0;
+		} else {
+			ispendr = gicv3_dist_base() + GICD_ISPENDR;
+			isactiver = gicv3_dist_base() + GICD_ISACTIVER;
+		}
 		break;
+	default:
+		assert(0);
+	}
+
+	if (irq < GIC_NR_PRIVATE_IRQS) {
+		pending = readl(ispendr) & (1 << irq);
+		active = readl(isactiver) & (1 << irq);
+	} else {
+		int offset = (irq - GIC_FIRST_SPI) / 32;
+		int mask = 1 << ((irq - GIC_FIRST_SPI) % 32);
+		pending = readl(ispendr + offset) & mask;
+		active = readl(isactiver + offset) & mask;
 	}
 
 	if (!active && !pending)

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

  parent reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-11 13:37 Andrew Jones
2020-02-11 13:52 ` Alexandru Elisei
2020-02-11 14:05   ` Andrew Jones
2020-02-11 14:50 ` Zenghui Yu
2020-02-11 15:32   ` Zenghui Yu
2020-02-11 15:49     ` Andrew Jones
2020-02-11 15:41   ` Andrew Jones [this message]
2020-02-12  2:57     ` Zenghui Yu
2020-02-13  9:31       ` Andrew Jones

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=20200211154135.vxxkpstt4cpoyqsp@kamzik.brq.redhat.com \
    --to=drjones@redhat.com \
    --cc=andre.przywara@arm.com \
    --cc=kvm@vger.kernel.org \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=yuzenghui@huawei.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