KVM ARM Archive on lore.kernel.org
 help / color / Atom feed
From: Zenghui Yu <yuzenghui@huawei.com>
To: Andrew Jones <drjones@redhat.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: Wed, 12 Feb 2020 10:57:09 +0800
Message-ID: <a8876667-934d-f617-682d-c488e7276d38@huawei.com> (raw)
In-Reply-To: <20200211154135.vxxkpstt4cpoyqsp@kamzik.brq.redhat.com>

Hi Drew,

On 2020/2/11 23:41, Andrew Jones wrote:
> 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.

(I guess the PL031 can be another new user.)

> 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);

We can also assert that only interrupts with ID smaller than 1020
will be handled.

>   
>   	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);

No need to minus GIC_FIRST_SPI.  And therefore these two cases
can actually be merged.

> +		pending = readl(ispendr + offset) & mask;
> +		active = readl(isactiver + offset) & mask;
>   	}
>   
>   	if (!active && !pending)

Otherwise this looks good enough (to me) for now, and let's wait
other reviewers to comment.  I've used the following diff to give
the pl031 test a go (roughly written, not dig into PL031 so much),
it just works fine :)


Thanks,
Zenghui

diff --git a/arm/pl031.c b/arm/pl031.c
index 86035fa..2d4160f 100644
--- a/arm/pl031.c
+++ b/arm/pl031.c
@@ -118,11 +118,12 @@ static int check_rtc_freq(void)
  	return 0;
  }

-static bool gic_irq_pending(void)
+static bool gic_pl031_pending(void)
  {
-	uint32_t offset = (pl031_irq / 32) * 4;
+	enum gic_irq_state state = gic_irq_state(pl031_irq);

-	return readl(gic_ispendr + offset) & (1 << (pl031_irq & 31));
+	return state == GIC_IRQ_STATE_PENDING ||
+		state == GIC_IRQ_STATE_ACTIVE_PENDING;
  }

  static void gic_irq_unmask(void)

[...]
/* replace all gic_irq_pending() with gic_pl031_pending() */
[...]

diff --git a/lib/arm/gic.c b/lib/arm/gic.c
index 3924dd1..34d77e3 100644
--- a/lib/arm/gic.c
+++ b/lib/arm/gic.c
@@ -152,6 +152,7 @@ enum gic_irq_state gic_irq_state(int irq)
  	enum gic_irq_state state;
  	void *ispendr, *isactiver;
  	bool pending, active;
+	int offset, mask;

  	assert(gic_common_ops);

@@ -173,15 +174,10 @@ enum gic_irq_state gic_irq_state(int irq)
  		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;
-	}
+	offset = (irq / 32) * 4;
+	mask = 1 << (irq % 32);
+	pending = readl(ispendr + offset) & mask;
+	active = readl(isactiver + offset) & mask;

  	if (!active && !pending)
  		state = GIC_IRQ_STATE_INACTIVE;

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

  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
2020-02-12  2:57     ` Zenghui Yu [this message]
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=a8876667-934d-f617-682d-c488e7276d38@huawei.com \
    --to=yuzenghui@huawei.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

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