core: interrupts: add interrupt_release api for reset properties #6907

ChenRunyang · 2024-06-21T02:35:54Z

This patch introduces a interrupt_release API to reset the properties of an interrupt to its previous settings. This functionality is essential for scenarios where a specific interrupt needs to be dynamically set to either Group 1 Secure (G1S) or Group 1 Non-Secure (G1NS) at different times.

etienne-lms

I've not fully dug into the implication of this feature. That said, here are some comments. Also, I wonder it interrupt_release() would be a better label, rather than interrupt_reset().

etienne-lms · 2024-06-21T08:07:36Z

core/include/kernel/interrupt.h

+ * @itr_num	Interrupt number to reset
+ */
+static inline void interrupt_reset(struct itr_chip *chip, size_t itr_num)
+{


assert(chip->ops->reset)

etienne-lms · 2024-06-21T08:13:15Z

core/drivers/gic.c

+{
+	size_t idx = it / NUM_INTS_PER_REG;
+	uint32_t mask = 1 << (it % NUM_INTS_PER_REG);
+


must check that it >= GIC_SPI_BASE

For consistency, it think this function should also ensures the the interrupt was previously disabled. I think this means git_it_reset() needs a return code.

Checked, if it < GIC_SPI_BASE, it will panic

The API only reset the properties of interrupt, It is not necessary that interrupts have been disabled before. Functionally, the two are not coupled.

I'm not sure it's that simple (I'm happy to be proven wrong though). What if the interrupt is active?

etienne-lms · 2024-06-21T08:18:36Z

core/drivers/gic.c

@@ -996,6 +1024,26 @@ static void gic_op_raise_sgi(struct itr_chip *chip, size_t it,
 	gic_it_raise_sgi(gd, it, cpu_mask, ns);
 }

+static void gic_it_reset(struct gic_data *gd, size_t it)


this whole function gic_it_reset() seems supported only when CFG_ARM_GICV3 is enabled.

It also works when CFG_ARM_GICV3 is not enabled, The GIC version affects whether IGROUPMODR is configured.

etienne-lms · 2024-06-21T08:19:40Z

core/include/kernel/interrupt.h

+/*
+ * interrupt_reset() - Reset the property for an interrupt
+ * @chip	Interrupt controller
+ * @itr_num	Interrupt number to reset


This is supported only for SPIs, could you add this information in this inline description comment?

ChenRunyang · 2024-06-24T02:58:35Z

I've not fully dug into the implication of this feature. That said, here are some comments. Also, I wonder it interrupt_release() would be a better label, rather than interrupt_reset().

Replaced the label from interrupt_reset to interrupt_release.

jenswi-linaro

I wonder if this doesn't boil to changing the group assignment of the interrupt id. As long as the interrupt is disabled we could leave it to the previous owner to restore the other properties.

jenswi-linaro · 2024-06-24T09:40:31Z

core/drivers/gic.c

+{
+	struct gic_data *gd = &gic_data;
+
+	gd->pre_prio = calloc(gd->max_it + 1, sizeof(*gd->pre_prio));


This will only be needed in very specific configurations, but like this, it will impact all configurations. How about keeping this information outside the GIC driver and letting interrupt_release() take the needed parameters to restore the state?

Add the gic_spi_get_prop API to get the interrupt properties, and the interrupt_release need to specify the interrupt attributes as parameters.

jenswi-linaro · 2024-06-24T09:45:59Z

core/drivers/gic.c

+	size_t idx = it / NUM_INTS_PER_REG;
+	uint32_t mask = 1 << (it % NUM_INTS_PER_REG);
+
+	/* Assigned to group0 */


What about group1 interrupts?

The API is used for change group0 or goup1 secure interrupts to group1 interrupts, so if group1 nonsecure interrupts call this API, it will be asserted

GICv3 Group 0 interrupts belong to EL3 so I'm not sure we should do anything with those.

ChenRunyang · 2024-06-25T09:49:26Z

I wonder if this doesn't boil to changing the group assignment of the interrupt id. As long as the interrupt is disabled we could leave it to the previous owner to restore the other properties.

Yes, it will change the group assignment of the interrupt id.
Updated the gic_op_release API to accept additional properties as parameters. These settings will be restored to their previous state by the original owner.

jenswi-linaro · 2024-06-25T14:13:03Z

core/drivers/gic.c

+	size_t idx = it / NUM_INTS_PER_REG;
+	uint32_t mask = 1 << (it % NUM_INTS_PER_REG);
+
+	/* Assigned to group0 */


GICv3 Group 0 interrupts belong to EL3 so I'm not sure we should do anything with those.

jenswi-linaro · 2024-06-25T14:17:09Z

core/drivers/gic.c

+	/* Assigned to group0 */
+	assert(!(io_read32(gd->gicd_base + GICD_IGROUPR(idx)) & mask));
+
+	gic_it_set_cpu_mask(gd, it, cpu_mask);


Once the interrupt has been changed to non-secure, the normal can configure it to their liking. How about clearing cpu_mask and setting prio to 0xff to avoid leaking information or unintended state?

It's OK to clear the cpu_mask, but is it possible to set the priority to the Linux kernel's default priority setting of 0xa0?And the current behavior of Kernel setting prio is undefined.

Sure, that makes sense.

jenswi-linaro · 2024-06-25T14:18:42Z

core/include/kernel/interrupt.h

+ * @prio	Interrupt priority
+ * @cpu_mask	Mask of the CPUs targeted by the interrupt
+ */
+static inline void interrupt_release(struct itr_chip *chip, size_t itr_num,


I know that you have already changed the name of this function once, but wouldn't interrupt_release_to_ns() be more clear?

OK, I will use interrupt_release_to_ns instead of interrupt_release.

jenswi-linaro · 2024-06-25T14:19:10Z

core/include/kernel/interrupt.h

+ * @cpu_mask	Mask of the CPUs targeted by the interrupt
+ */
+static inline void interrupt_release(struct itr_chip *chip, size_t itr_num,
+				     uint8_t prio, uint8_t cpu_mask)


The normal world should be able to configure this itself.

jenswi-linaro · 2024-06-25T14:26:16Z

core/drivers/gic.c

@@ -858,6 +888,20 @@ void gic_dump_state(void)
 	}
 }

+void gic_spi_get_prop(size_t it, uint8_t *cpu_mask, uint8_t *prio)


Why is this function needed?

I want to get property before requesting the IRQ so that it can be used later in the irq_release flow. I will remove this function if the cpu_mask is cleared and the priority set to default value.

jenswi-linaro · 2024-06-25T15:16:40Z

core/drivers/gic.c

+{
+	size_t idx = it / NUM_INTS_PER_REG;
+	uint32_t mask = 1 << (it % NUM_INTS_PER_REG);
+


I'm not sure it's that simple (I'm happy to be proven wrong though). What if the interrupt is active?

anilreddy3 · 2024-06-28T10:26:38Z

core/include/kernel/interrupt.h

@@ -72,6 +72,8 @@ struct itr_ops {
 	void (*raise_pi)(struct itr_chip *chip, size_t it);
 	void (*raise_sgi)(struct itr_chip *chip, size_t it,
 			  uint32_t cpu_mask);
+	void (*release)(struct itr_chip *chip, size_t it,


release() is doing other way of add(). so, would it be much clear if they are next to each other?

Thanks for your comment, It's sorted them in descending order by the first letter

jenswi-linaro · 2024-07-08T08:48:30Z

By the way, rebasing the patches on the latest should take care of the failing test case.

jenswi-linaro · 2024-07-11T06:24:57Z

Please use rebase instead of merge, we don't accept merges in pull requests.

jenswi-linaro

With my comment addressed, please apply:
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro · 2024-07-16T10:20:49Z

core/drivers/gic.c

+{
+	struct gic_data *gd = &gic_data;
+	size_t idx = it / NUM_INTS_PER_REG;
+	uint32_t mask = 1 << (it % NUM_INTS_PER_REG);


Please use the BIT32() macro instead. I know we're doing a bit of both in this file, but using BIT32() is the preferred way.

etienne-lms · 2024-07-17T11:33:04Z

core/drivers/gic.c

+
+	assert(it < gd->max_it && it >= GIC_SPI_BASE);
+	/* Assert it's already disabled */
+	assert(!gic_it_is_enabled(gd, it));


I wonder if an assertion is enough here. I would rather suggest to return an error code, what do you think?

Most errors here either cause an assert or a panic. I don't see how this error is a special case.
What should a caller do on this error that he shouldn't have done already?
This is a static programming error, not a dynamic error like malloc() failing.

etienne-lms · 2024-07-17T11:36:43Z

core/drivers/gic.c

+	/* Clear pending status */
+	io_write32(gd->gicd_base + GICD_ICPENDR(idx), mask);
+	/* Assign it to NS Group1 */
+	io_setbits32(gd->gicd_base + GICD_IGROUPR(idx), mask);


I think this instruction and the one below should be protected by a lock.

I agree, since we're writing to a value register, not a set or clear bits register.
This function is in contrast to void gic_init_donate_sgi_to_ns() used post-boot when more than one core might be active in the secure world.

etienne-lms · 2024-07-18T16:40:42Z

core/include/drivers/gic.h

@@ -56,4 +58,7 @@ void gic_init_per_cpu(void);

 /* Print GIC state to console */
 void gic_dump_state(void);
+
+/* Release the property for a shared peripheral interrupt */


Explicitly say that the interrupt is re-assigned to the non-secure interrupt group and its priority reset to GIC_SPI_PRI_NS_EL1.
State also that the interrupt shall be disabled when this function is called.

of an interrupt This patch introduces gic_spi_release_to_ns API to release an interrupt to Non secure settings. This functionality is essential for scenarios where a specific interrupt needs to be dynamically set to either Group 1 Secure (G1S) or Group 1 Non-Secure (G1NS) at different times. Signed-off-by: Runyang Chen <runyang.chen@mediatek.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

etienne-lms · 2024-08-29T08:32:56Z

core/drivers/gic.c

+	uint32_t mask = BIT32(it % NUM_INTS_PER_REG);
+
+	if (it < gd->max_it && it >= GIC_SPI_BASE)
+		return TEE_ERROR_OVERFLOW;


I suggest TEE_ERROR_BAD_PARAMETERS instead.

etienne-lms · 2024-08-29T08:32:58Z

core/drivers/gic.c

@@ -858,6 +860,36 @@ void gic_dump_state(void)
 	}
 }

+int gic_spi_release_to_ns(size_t it)


s/int/TEE_Result/

github-actions · 2024-09-29T01:59:50Z

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

etienne-lms · 2024-10-03T08:10:26Z

@ChenRunyang, do you plan to update this P-R?

github-actions · 2024-11-03T01:59:47Z

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

ChenRunyang · 2024-11-06T08:15:51Z

@ChenRunyang, do you plan to update this P-R?

Sorry for late, I've been on vacation recently, I will modify the patch according to your suggestions.

etienne-lms suggested changes Jun 21, 2024

View reviewed changes

ChenRunyang force-pushed the master branch from 1375512 to 6675631 Compare June 24, 2024 02:43

ChenRunyang changed the title ~~core: interrupts: add interrupt_reset api for reset properties~~ core: interrupts: add interrupt_release api for reset properties Jun 24, 2024

ChenRunyang force-pushed the master branch from 6675631 to 4bd0414 Compare June 24, 2024 03:02

jenswi-linaro reviewed Jun 24, 2024

View reviewed changes

ChenRunyang force-pushed the master branch from 4bd0414 to ebc658f Compare June 25, 2024 09:35

jenswi-linaro reviewed Jun 25, 2024

View reviewed changes

anilreddy3 reviewed Jun 28, 2024

View reviewed changes

ChenRunyang force-pushed the master branch 4 times, most recently from 6d5dbeb to 0e58e2d Compare July 11, 2024 08:59

jenswi-linaro reviewed Jul 16, 2024

View reviewed changes

etienne-lms reviewed Jul 18, 2024

View reviewed changes

ChenRunyang force-pushed the master branch from 0e58e2d to e056abd Compare July 30, 2024 01:59

ChenRunyang force-pushed the master branch from e056abd to 4276430 Compare August 6, 2024 03:15

etienne-lms reviewed Aug 29, 2024

View reviewed changes

github-actions bot added the Stale label Sep 29, 2024

github-actions bot removed the Stale label Oct 4, 2024

github-actions bot added the Stale label Nov 3, 2024

github-actions bot removed the Stale label Nov 7, 2024

core: interrupts: add interrupt_release api for reset properties #6907

Are you sure you want to change the base?

core: interrupts: add interrupt_release api for reset properties #6907

Conversation

ChenRunyang commented Jun 21, 2024 • edited Loading

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChenRunyang commented Jun 24, 2024

jenswi-linaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChenRunyang Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChenRunyang commented Jun 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Jul 8, 2024

jenswi-linaro commented Jul 11, 2024

jenswi-linaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 29, 2024

etienne-lms commented Oct 3, 2024

github-actions bot commented Nov 3, 2024

ChenRunyang commented Nov 6, 2024

ChenRunyang commented Jun 21, 2024 •

edited

Loading

ChenRunyang Jun 25, 2024 •

edited

Loading