Hi Andrew,
[Sorry for the long delay of resuming this discussion.] I think psa_irq_is_enabled() could be removed. As the peripherals are exclusively owned by Partitions, Partitions should be able to manage the status of the IRQs by themselves. And the psa_irq_disable() could still return the previous status. It (and the psa_irq_enable() as well) should be atomic anyway because the caller would consider the IRQ is disabled or enabled by calling the corresponding API. The APIs must ensure the validness, to do that disabling interrupts entirely might be inevitable. In TF-M, this is done by calling SVC in the APIs to tell SPM to manipulate the interrupt controller. And SVC has higher priority than all interrupts.
Best Regards, Kevin
-----Original Message----- From: TF-M tf-m-bounces@lists.trustedfirmware.org On Behalf Of Andrew Thoelke via TF-M Sent: Monday, February 22, 2021 11:03 PM To: tf-m@lists.trustedfirmware.org; nd nd@arm.com Subject: Re: [TF-M] Arm Firmware Framework for M 1.1 Extensions Alpha specification
I've had some further input on the interrupt management API. The use of patterns such as:
bool irq_enabled = psa_irq_is_enabled(MY_IRQ); // [1] psa_irq_disable(MY_IRQ); // [2] // critical section if (irq_enabled) psa_irq_enable(MY_IRQ);
must be discouraged as this is not guaranteed to be safe in the presence of interrupts that can change the status of MY_IRQ between the query [1] and the disable [2]. In general, it requires whole system analysis to determine that there are no such interrupts, and that this code is 100% reliable.
However, providing the originally proposed API such as psa_irq_disable(), which returns the prior status, does not practically solve the problem. Instead, it just moves the race condition window into the implementation of that API.
The only way in which such an API would be generally safe from the race condition is if the query+disable is atomic with respect to all other interrupts in the system - this either requires disabling interrupts entirely, or having an atomic read+disable capability in the interrupt controller. In systems which worry about such race conditions, disabling all interrupts can be unacceptable.
The recommended approach is to avoid having software that depends on the state of the interrupt, but which does not implicitly know the state of the interrupt. In such a system, there is never a need to query the current interrupt state as on any line of code, the interrupt state is always known at design time.
I am not sure if this suggests that we should: 1. Remove even the psa_irq_is_enabled() function, to prevent developers writing the above code, OR 2. We do not document the above pattern as a way to manage nested critical sections, OR 3. Retain the example above, but explain that this must be coupled with a software design that ensures the stability of the MY_IRQ status between [1] and [2]?
Regards, Andrew
From: TF-M tf-m-bounces@lists.trustedfirmware.org On Behalf Of Ken Liu via TF-M Sent: 22 February 2021 04:49 To: tf-m@lists.trustedfirmware.org Cc: nd nd@arm.com Subject: Re: [TF-M] Arm Firmware Framework for M 1.1 Extensions Alpha specification
As the ‘psa_irq_status_t’ is a implementation-defined value, is it possible let the implementation-define the status encoding?
Then the status and its checking code needs to be define by implementation, too: PSA_IRQ_STATUS_NOCHANGE PSA_IRQ_STATUS_DISABLE PSA_IRQ_STATUS_ENABLE
PSA_IRQ_STAUTS_IS_ENABLED(status)
This would make everything implementation-defined and this affects the headers, and one extra header: psa_impdef.h needs to be created by implementations. With this the ffm based applications just use preprocessors to get status and check them; the enable/disable can be out of ‘true’ and ‘false’ values.
/Ken
From: TF-M mailto:tf-m-bounces@lists.trustedfirmware.org On Behalf Of Kevin Peng via TF-M Sent: Tuesday, January 26, 2021 11:08 AM To: mailto:tf-m@lists.trustedfirmware.org Cc: nd mailto:nd@arm.com Subject: Re: [TF-M] Arm Firmware Framework for M 1.1 Extensions Alpha specification
Hi all,
Per the off-line discussion with Andrew, I’d like to start a wider discussion on the interrupt APIs, specifically the Secure Partition API changes for interrupt control in chapter 6.3.3. There are the following APIs: • uint32_t psa_irq_is_enabled (psa_signal_t irq_signal); This API returns 0 if the interrupt is disabled and 1 if the interrupt is enabled. • psa_irq_status_t psa_irq_disable(psa_signal_t irq_signal); This API returns the status of the interrupt prior to this call with an implementation defined value
Note the return type of the interrupt status is different. The first one is only to tell whether the interrupt is enabled (1) or not (0) – an equivalent to bool type. The second one could be any value to indicate an interrupt status. And that value is intended to be passed to psa_irq_restore to write to the interrupt control register directly. • void psa_irq_restore(psa_signal_t irq_signal, psa_irq_status_t saved_status);
The typical usage: psa_irq_status irq2_state = psa_irq_disable(IRQ2_SIGNAL) ; // manipulate data shared with IRQ2 … psa_irq_restore(IRQ2_SIGNAL, irq2_state);
This is a very efficient design as the 'saved status value' can be the exact value that needs to be written to an interrupt control register to restore the previous state. But TF-M seems to be unable to take that advantage. Because the most common interrupt controller is the NVIC provided by the core. The NVIC takes 1/0 to enable or disable the interrupt and one register for 32 interrupts. The underlying NVIC operation provided by CMSIS is NVIC_Enable/DisableIRQ. So the psa_irq_status_t in TF-M would simply 1 or 0 for a specific interrupt signal. Then the psa_irq_restore could be unnecessary if psa_irq_disable returns uint32_t just like psa_irq_is_enabled: uint32_t irq_status = psa_irq_disable(IRQ); ... // critical section if (irq_status) psa_irq_enable(IRQ);
Any thoughts on the necessity of the psa_irq_restore API?
The draft implementation of the current APIs for easy understanding: https://review.trustedfirmware.org/q/topic:%22psa_interrupt_api%22+(status:o...)
Best Regards, Kevin
-----Original Message----- From: TF-M mailto:tf-m-bounces@lists.trustedfirmware.org On Behalf Of Andrew Thoelke via TF-M Sent: Friday, January 15, 2021 1:25 AM To: mailto:tf-m@lists.trustedfirmware.org; nd mailto:nd@arm.com Subject: [TF-M] Arm Firmware Framework for M 1.1 Extensions Alpha specification
Hi all,
The PSA Firmware Framework for M 1.1 Extensions specification is now published on Arm Developer.
This document introduces a set of updates and extensions to the Arm® Platform Security Architecture Firmware Framework (FF-M) specification, designed to build on the capabilities provided in version 1.0.
This is an initial ALPHA release in order to enable wider review and feedback on the changes proposed to be included in the v1.1 specification. At this quality level, the changes and interfaces defined are not stable enough for product development. When the proposed extensions are sufficiently stable to be classed as Beta, they will be integrated into the FF-M version 1.1 specification.
The 1.1 Extensions document can be downloaded from:
https://developer.arm.com/documentation/aes0039/latest
Both the 1.0 Specification and the 1.1 Extensions document are linked from the main PSA architecture page:
https://developer.arm.com/architectures/security-architectures/platform-secu...
Ken and I have presented a number of times at last year's Tech Forums on the proposals in the specification, most recently I provided a summary of the whole document on 10th December 2020.
If you have any feedback, please provide it to mailto:arm.psa-feedback@arm.com, or discuss the proposals here in the TF-M mailing list.
Regards, Andrew -- TF-M mailing list mailto:TF-M@lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-m -- TF-M mailing list TF-M@lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-m
Hi Kevin,
In light of the concern regarding atomicity and race conditions, is there a real benefit to retaining the behaviour of psa_irq_disable() returning the prior status.
By defining the API this way, it forces every implementation to ensure that the test-and-disable action of this API is atomic with respect to all interrupts in the system (as these could modify the target interrupt state between the test and the disable). Even if TF-M today does this as a result of using an SVC to implement this API, that does not imply that all implementations will need to do that.
If we remove the return value from psa_irq_disable(), this also simplifies the TF-M implementation using the CMSIS NVIC_DisableIRQ() function.
This would leave us with:
void psa_irq_enable(psa_signal_t irq_signal); void psa_irq_disable(psa_signal_t irq_signal);
Is it problematic to provide no mechanism to query the state? Are the following use cases for psa_irq_is_enabled() important enough to include this API:
* Diagnostics (e.g. when debugging) * Initialisation * Recovery from error conditions
The last two of these shouldn't require such an API, as the software should be aware of the peripheral and interrupt state at these points - but the API might make coding these easier?
Are there other important uses for the query API?
Regards, Andrew
-----Original Message----- From: Kevin Peng Kevin.Peng@arm.com Sent: 13 April 2021 05:03 To: Andrew Thoelke Andrew.Thoelke@arm.com; tf-m@lists.trustedfirmware.org Cc: nd nd@arm.com Subject: RE: Arm Firmware Framework for M 1.1 Extensions Alpha specification
Hi Andrew,
[Sorry for the long delay of resuming this discussion.] I think psa_irq_is_enabled() could be removed. As the peripherals are exclusively owned by Partitions, Partitions should be able to manage the status of the IRQs by themselves. And the psa_irq_disable() could still return the previous status. It (and the psa_irq_enable() as well) should be atomic anyway because the caller would consider the IRQ is disabled or enabled by calling the corresponding API. The APIs must ensure the validness, to do that disabling interrupts entirely might be inevitable. In TF-M, this is done by calling SVC in the APIs to tell SPM to manipulate the interrupt controller. And SVC has higher priority than all interrupts.
Best Regards, Kevin
-----Original Message----- From: TF-M tf-m-bounces@lists.trustedfirmware.org On Behalf Of Andrew Thoelke via TF-M Sent: Monday, February 22, 2021 11:03 PM To: tf-m@lists.trustedfirmware.org; nd nd@arm.com Subject: Re: [TF-M] Arm Firmware Framework for M 1.1 Extensions Alpha specification
I've had some further input on the interrupt management API. The use of patterns such as:
bool irq_enabled = psa_irq_is_enabled(MY_IRQ); // [1] psa_irq_disable(MY_IRQ); // [2] // critical section if (irq_enabled) psa_irq_enable(MY_IRQ);
must be discouraged as this is not guaranteed to be safe in the presence of interrupts that can change the status of MY_IRQ between the query [1] and the disable [2]. In general, it requires whole system analysis to determine that there are no such interrupts, and that this code is 100% reliable.
However, providing the originally proposed API such as psa_irq_disable(), which returns the prior status, does not practically solve the problem. Instead, it just moves the race condition window into the implementation of that API.
The only way in which such an API would be generally safe from the race condition is if the query+disable is atomic with respect to all other interrupts in the system - this either requires disabling interrupts entirely, or having an atomic read+disable capability in the interrupt controller. In systems which worry about such race conditions, disabling all interrupts can be unacceptable.
The recommended approach is to avoid having software that depends on the state of the interrupt, but which does not implicitly know the state of the interrupt. In such a system, there is never a need to query the current interrupt state as on any line of code, the interrupt state is always known at design time.
I am not sure if this suggests that we should: 1. Remove even the psa_irq_is_enabled() function, to prevent developers writing the above code, OR 2. We do not document the above pattern as a way to manage nested critical sections, OR 3. Retain the example above, but explain that this must be coupled with a software design that ensures the stability of the MY_IRQ status between [1] and [2]?
Regards, Andrew
From: TF-M tf-m-bounces@lists.trustedfirmware.org On Behalf Of Ken Liu via TF-M Sent: 22 February 2021 04:49 To: tf-m@lists.trustedfirmware.org Cc: nd nd@arm.com Subject: Re: [TF-M] Arm Firmware Framework for M 1.1 Extensions Alpha specification
As the ‘psa_irq_status_t’ is a implementation-defined value, is it possible let the implementation-define the status encoding?
Then the status and its checking code needs to be define by implementation, too: PSA_IRQ_STATUS_NOCHANGE PSA_IRQ_STATUS_DISABLE PSA_IRQ_STATUS_ENABLE
PSA_IRQ_STAUTS_IS_ENABLED(status)
This would make everything implementation-defined and this affects the headers, and one extra header: psa_impdef.h needs to be created by implementations. With this the ffm based applications just use preprocessors to get status and check them; the enable/disable can be out of ‘true’ and ‘false’ values.
/Ken
From: TF-M mailto:tf-m-bounces@lists.trustedfirmware.org On Behalf Of Kevin Peng via TF-M Sent: Tuesday, January 26, 2021 11:08 AM To: mailto:tf-m@lists.trustedfirmware.org Cc: nd mailto:nd@arm.com Subject: Re: [TF-M] Arm Firmware Framework for M 1.1 Extensions Alpha specification
Hi all,
Per the off-line discussion with Andrew, I’d like to start a wider discussion on the interrupt APIs, specifically the Secure Partition API changes for interrupt control in chapter 6.3.3. There are the following APIs: • uint32_t psa_irq_is_enabled (psa_signal_t irq_signal); This API returns 0 if the interrupt is disabled and 1 if the interrupt is enabled. • psa_irq_status_t psa_irq_disable(psa_signal_t irq_signal); This API returns the status of the interrupt prior to this call with an implementation defined value
Note the return type of the interrupt status is different. The first one is only to tell whether the interrupt is enabled (1) or not (0) – an equivalent to bool type. The second one could be any value to indicate an interrupt status. And that value is intended to be passed to psa_irq_restore to write to the interrupt control register directly. • void psa_irq_restore(psa_signal_t irq_signal, psa_irq_status_t saved_status);
The typical usage: psa_irq_status irq2_state = psa_irq_disable(IRQ2_SIGNAL) ; // manipulate data shared with IRQ2 … psa_irq_restore(IRQ2_SIGNAL, irq2_state);
This is a very efficient design as the 'saved status value' can be the exact value that needs to be written to an interrupt control register to restore the previous state. But TF-M seems to be unable to take that advantage. Because the most common interrupt controller is the NVIC provided by the core. The NVIC takes 1/0 to enable or disable the interrupt and one register for 32 interrupts. The underlying NVIC operation provided by CMSIS is NVIC_Enable/DisableIRQ. So the psa_irq_status_t in TF-M would simply 1 or 0 for a specific interrupt signal. Then the psa_irq_restore could be unnecessary if psa_irq_disable returns uint32_t just like psa_irq_is_enabled: uint32_t irq_status = psa_irq_disable(IRQ); ... // critical section if (irq_status) psa_irq_enable(IRQ);
Any thoughts on the necessity of the psa_irq_restore API?
The draft implementation of the current APIs for easy understanding: https://review.trustedfirmware.org/q/topic:%22psa_interrupt_api%22+(status:o...)
Best Regards, Kevin
-----Original Message----- From: TF-M mailto:tf-m-bounces@lists.trustedfirmware.org On Behalf Of Andrew Thoelke via TF-M Sent: Friday, January 15, 2021 1:25 AM To: mailto:tf-m@lists.trustedfirmware.org; nd mailto:nd@arm.com Subject: [TF-M] Arm Firmware Framework for M 1.1 Extensions Alpha specification
Hi all,
The PSA Firmware Framework for M 1.1 Extensions specification is now published on Arm Developer.
This document introduces a set of updates and extensions to the Arm® Platform Security Architecture Firmware Framework (FF-M) specification, designed to build on the capabilities provided in version 1.0.
This is an initial ALPHA release in order to enable wider review and feedback on the changes proposed to be included in the v1.1 specification. At this quality level, the changes and interfaces defined are not stable enough for product development. When the proposed extensions are sufficiently stable to be classed as Beta, they will be integrated into the FF-M version 1.1 specification.
The 1.1 Extensions document can be downloaded from:
https://developer.arm.com/documentation/aes0039/latest
Both the 1.0 Specification and the 1.1 Extensions document are linked from the main PSA architecture page:
https://developer.arm.com/architectures/security-architectures/platform-secu...
Ken and I have presented a number of times at last year's Tech Forums on the proposals in the specification, most recently I provided a summary of the whole document on 10th December 2020.
If you have any feedback, please provide it to mailto:arm.psa-feedback@arm.com, or discuss the proposals here in the TF-M mailing list.
Regards, Andrew -- TF-M mailing list mailto:TF-M@lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-m -- TF-M mailing list TF-M@lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-m
tf-m@lists.trustedfirmware.org
-
Andrew Thoelke -
Kevin Peng