KVM ARM Archive on lore.kernel.org
 help / color / Atom feed
From: Alexandru Elisei <alexandru.elisei@arm.com>
To: Andrew Jones <drjones@redhat.com>, Eric Auger <eric.auger@redhat.com>
Cc: kvm@vger.kernel.org, maz@kernel.org, qemu-devel@nongnu.org,
	qemu-arm@nongnu.org, andre.przywara@arm.com,
	andrew.murray@arm.com, kvmarm@lists.cs.columbia.edu,
	eric.auger.pro@gmail.com
Subject: Re: [kvm-unit-tests PATCH v2 8/9] arm: gic: Provide per-IRQ helper functions
Date: Thu, 5 Mar 2020 11:10:35 +0000
Message-ID: <80417be0-b897-2e09-ad56-05ae6b6a5fd6@arm.com> (raw)
In-Reply-To: <20200305095529.hkdyhghkofquat75@kamzik.brq.redhat.com>

Hi,

I'm going to reiterate the comments I posted on this exact same patch sent by
Andre [1], and add a few new ones.

[1] https://www.spinics.net/lists/arm-kernel/msg767690.html

On 3/5/20 9:55 AM, Andrew Jones wrote:
> On Thu, Jan 30, 2020 at 12:25:09PM +0100, Eric Auger wrote:
>> From: Andre Przywara <andre.przywara@arm.com>
>>
>> A common theme when accessing per-IRQ parameters in the GIC distributor
>> is to set fields of a certain bit width in a range of MMIO registers.
>> Examples are the enabled status (one bit per IRQ), the level/edge
>> configuration (2 bits per IRQ) or the priority (8 bits per IRQ).
>>
>> Add a generic helper function which is able to mask and set the
>> respective number of bits, given the IRQ number and the MMIO offset.
>> Provide wrappers using this function to easily allow configuring an IRQ.
>>
>> For now assume that private IRQ numbers always refer to the current CPU.
>> In a GICv2 accessing the "other" private IRQs is not easily doable (the
>> registers are banked per CPU on the same MMIO address), so we impose the
>> same limitation on GICv3, even though those registers are not banked
>> there anymore.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Missing Eric's s-b.
>
>> ---
>>
>> initialize reg
>> ---
>>  lib/arm/asm/gic-v3.h |  2 +
>>  lib/arm/asm/gic.h    |  9 +++++
>>  lib/arm/gic.c        | 90 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 101 insertions(+)
>>
>> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
>> index 347be2f..4a445a5 100644
>> --- a/lib/arm/asm/gic-v3.h
>> +++ b/lib/arm/asm/gic-v3.h
>> @@ -23,6 +23,8 @@
>>  #define GICD_CTLR_ENABLE_G1A		(1U << 1)
>>  #define GICD_CTLR_ENABLE_G1		(1U << 0)
>>  
>> +#define GICD_IROUTER			0x6000
>> +
>>  /* Re-Distributor registers, offsets from RD_base */
>>  #define GICR_TYPER			0x0008
>>  
>> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
>> index 1fc10a0..21cdb58 100644
>> --- a/lib/arm/asm/gic.h
>> +++ b/lib/arm/asm/gic.h
>> @@ -15,6 +15,7 @@
>>  #define GICD_IIDR			0x0008
>>  #define GICD_IGROUPR			0x0080
>>  #define GICD_ISENABLER			0x0100
>> +#define GICD_ICENABLER			0x0180
>>  #define GICD_ISPENDR			0x0200
>>  #define GICD_ICPENDR			0x0280
>>  #define GICD_ISACTIVER			0x0300
>> @@ -73,5 +74,13 @@ extern void gic_write_eoir(u32 irqstat);
>>  extern void gic_ipi_send_single(int irq, int cpu);
>>  extern void gic_ipi_send_mask(int irq, const cpumask_t *dest);
>>  
>> +void gic_set_irq_bit(int irq, int offset);
>> +void gic_enable_irq(int irq);
>> +void gic_disable_irq(int irq);
>> +void gic_set_irq_priority(int irq, u8 prio);
>> +void gic_set_irq_target(int irq, int cpu);
>> +void gic_set_irq_group(int irq, int group);
>> +int gic_get_irq_group(int irq);
>> +
>>  #endif /* !__ASSEMBLY__ */
>>  #endif /* _ASMARM_GIC_H_ */
>> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
>> index 9430116..aa9cb86 100644
>> --- a/lib/arm/gic.c
>> +++ b/lib/arm/gic.c
>> @@ -146,3 +146,93 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
>>  	assert(gic_common_ops && gic_common_ops->ipi_send_mask);
>>  	gic_common_ops->ipi_send_mask(irq, dest);
>>  }
>> +
>> +enum gic_bit_access {
>> +	ACCESS_READ,
>> +	ACCESS_SET,
>> +	ACCESS_RMW
>> +};
>> +
>> +static u8 gic_masked_irq_bits(int irq, int offset, int bits, u8 value,
>> +			      enum gic_bit_access access)
>> +{
>> +	void *base;
>> +	int split = 32 / bits;
>> +	int shift = (irq % split) * bits;
>> +	u32 reg = 0, mask = ((1U << bits) - 1) << shift;
>> +
>> +	switch (gic_version()) {
>> +	case 2:
>> +		base = gicv2_dist_base();
>> +		break;
>> +	case 3:
>> +		if (irq < 32)
>> +			base = gicv3_sgi_base();
>> +		else
>> +			base = gicv3_dist_base();
>> +		break;
>> +	default:
>> +		return 0;
>> +	}
>> +	base += offset + (irq / split) * 4;
>> +
>> +	switch (access) {
>> +	case ACCESS_READ:
>> +		return (readl(base) & mask) >> shift;
>> +	case ACCESS_SET:
>> +		reg = 0;
>> +		break;
>> +	case ACCESS_RMW:
>> +		reg = readl(base) & ~mask;
>> +		break;
>> +	}
>> +
>> +	writel(reg | ((u32)value << shift), base);
>> +
>> +	return 0;
>> +}

This function looks a bit out of place:
- the function name has a verb in the past tense ('masked'), which makes me think
it should return a bool, but the function actually performs an access to a GIC
register.
- the return value is an u8, but it returns an u32 on a read, because readl
returns an u32.
- the semantics of the function and the return value change based on the access
parameter; worse yet, the return value on a write is completely ignored by the
callers and the value parameter is ignored on reads.

And a few new niggles:
- the accessor functions are hard to read: git_masked_irq_bits(irq, offset, 1, 1, ACCESS_SET). What the 1's represent is not obvious without reading the gic_masked_irq_bits functions.
- Andrew introduced a function to check the IRQ state at the GIC level [2]. The function is clear and nice to read because it has no indirection. How about we follow this pattern for all the accessors below and we remove the calls to gic_masked_irq_bits entirely? 

[2] https://www.spinics.net/lists/kvm/msg207073.html

Thanks,
Alex

>> +
>> +void gic_set_irq_bit(int irq, int offset)
>> +{
>> +	gic_masked_irq_bits(irq, offset, 1, 1, ACCESS_SET);
>> +}
>> +
>> +void gic_enable_irq(int irq)
>> +{
>> +	gic_set_irq_bit(irq, GICD_ISENABLER);
>> +}
>> +
>> +void gic_disable_irq(int irq)
>> +{
>> +	gic_set_irq_bit(irq, GICD_ICENABLER);
>> +}
>> +
>> +void gic_set_irq_priority(int irq, u8 prio)
>> +{
>> +	gic_masked_irq_bits(irq, GICD_IPRIORITYR, 8, prio, ACCESS_RMW);
>> +}
>> +
>> +void gic_set_irq_target(int irq, int cpu)
>> +{
>> +	if (irq < 32)
>> +		return;
>> +
>> +	if (gic_version() == 2) {
>> +		gic_masked_irq_bits(irq, GICD_ITARGETSR, 8, 1U << cpu,
>> +				    ACCESS_RMW);
>> +
>> +		return;
>> +	}
>> +
>> +	writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
>> +}
>> +
>> +void gic_set_irq_group(int irq, int group)
>> +{
>> +	gic_masked_irq_bits(irq, GICD_IGROUPR, 1, group, ACCESS_RMW);
>> +}
>> +
>> +int gic_get_irq_group(int irq)
>> +{
>> +	return gic_masked_irq_bits(irq, GICD_IGROUPR, 1, 0, ACCESS_READ);
>> +}
>> -- 
>> 2.20.1
>>
>>
> Looks good to me.
>
> Thanks,
> drew
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

  reply index

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-30 11:25 [kvm-unit-tests PATCH v2 0/9] KVM: arm64: PMUv3 Event Counter Tests Eric Auger
2020-01-30 11:25 ` [kvm-unit-tests PATCH v2 1/9] arm64: Provide read/write_sysreg_s Eric Auger
2020-01-30 11:25 ` [kvm-unit-tests PATCH v2 2/9] arm: pmu: Let pmu tests take a sub-test parameter Eric Auger
2020-03-04 18:01   ` Andre Przywara
2020-03-05  8:44   ` Andrew Jones
2020-01-30 11:25 ` [kvm-unit-tests PATCH v2 3/9] arm: pmu: Add a pmu struct Eric Auger
2020-03-04 18:02   ` Andre Przywara
2020-03-04 18:21     ` Auger Eric
2020-03-05  8:53   ` Andrew Jones
2020-01-30 11:25 ` [kvm-unit-tests PATCH v2 4/9] arm: pmu: Check Required Event Support Eric Auger
2020-02-11 15:33   ` Peter Maydell
2020-02-11 18:08     ` Auger Eric
2020-02-11 16:28   ` Peter Maydell
2020-02-11 18:32     ` Auger Eric
2020-03-04 18:02   ` Andre Przywara
2020-03-05  9:04   ` Andrew Jones
2020-01-30 11:25 ` [kvm-unit-tests PATCH v2 5/9] arm: pmu: Basic event counter Tests Eric Auger
2020-02-11 16:27   ` Peter Maydell
2020-02-11 18:31     ` Auger Eric
2020-03-04 18:03   ` Andre Przywara
2020-03-05  9:33   ` Andrew Jones
2020-03-12 11:19     ` Auger Eric
2020-03-05  9:42   ` Andrew Jones
2020-03-12 11:16     ` Auger Eric
2020-01-30 11:25 ` [kvm-unit-tests PATCH v2 6/9] arm: pmu: Test chained counter Eric Auger
2020-02-11 16:24   ` Peter Maydell
2020-02-11 18:30     ` Auger Eric
2020-03-05  9:37   ` Andrew Jones
2020-01-30 11:25 ` [kvm-unit-tests PATCH v2 7/9] arm: pmu: test 32-bit <-> 64-bit transitions Eric Auger
2020-03-05  9:50   ` Andrew Jones
2020-01-30 11:25 ` [kvm-unit-tests PATCH v2 8/9] arm: gic: Provide per-IRQ helper functions Eric Auger
2020-03-05  9:55   ` Andrew Jones
2020-03-05 11:10     ` Alexandru Elisei [this message]
2020-01-30 11:25 ` [kvm-unit-tests PATCH v2 9/9] arm: pmu: Test overflow interrupts Eric Auger
2020-03-05 10:17   ` Andrew Jones
2020-02-11 15:42 ` [kvm-unit-tests PATCH v2 0/9] KVM: arm64: PMUv3 Event Counter Tests Peter Maydell
2020-02-11 16:07   ` Andrew Jones
2020-02-11 18:23     ` Auger Eric

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=80417be0-b897-2e09-ad56-05ae6b6a5fd6@arm.com \
    --to=alexandru.elisei@arm.com \
    --cc=andre.przywara@arm.com \
    --cc=andrew.murray@arm.com \
    --cc=drjones@redhat.com \
    --cc=eric.auger.pro@gmail.com \
    --cc=eric.auger@redhat.com \
    --cc=kvm@vger.kernel.org \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=maz@kernel.org \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /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