[Bug 1001111] New: Access to saved registers from Cortex-M ISR

[Bug 1001111] New: Access to saved registers from Cortex-M ISR

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001111

           Summary: Access to saved registers from Cortex-M ISR
           Product: eCos
           Version: CVS
          Platform: All
        OS/Version: Cortex-M
            Status: NEW
          Severity: enhancement
          Priority: normal
         Component: HAL
        AssignedTo: unassigned@bugs.ecos.sourceware.org
        ReportedBy: john@dallaway.org.uk
                CC: ecos-bugs@ecos.sourceware.org
             Class: Advice Request


Created an attachment (id=1072)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1072)
cortexm-intr-state-110107.patch

There is currently no access to the saved interrupt state from an ISR on
Cortex-M. This is required for implementing profiling support and is commonly
provided by passing a HAL_SavedRegisters* parameter into the ISR (eg ARM,
PowerPC).

On Cortex-M, interrupts are delivered via the C function
hal_deliver_interrupt() which is called from the default interrupt VSR. The
attached patch modifies the VSR to pass the interrupt state to
hal_deliver_interrupt() unconditionally. hal_deliver_interrupt() then passes
this parameter on to the ISR.

API impact is confined to code which calls hal_deliver_interrupt() directly -
currently a single instance in an STM32 springboard ISR only.

The patch adds two machine code instructions to an unoptimised build of
hal_deliver_interrupt(). The patch adds three machine code instructions to the
default interrupt VSR in the case where GDB break support is not enabled. These
three instructions could be made conditional on CYGPKG_PROFILE_GPROF, but
access to the saved interrupt state is unconditional on other architectures.

Comments?

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001111] Access to saved registers from Cortex-M ISR

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001111

Nick Garnett <nickg@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |nickg@ecoscentric.com

--- Comment #1 from Nick Garnett <nickg@ecoscentric.com> 2011-01-07 13:39:06 GMT ---
The intention with the code as it was written is that if anything needs access
to the interrupt state then it should enable the existing code that saves a
full and complete CPU state and stores it in hal_saved_interrupt_state; either
by defining the CTRLC support option or by new means. Interested code can then
fetch it from there. That way this code is only included when this
functionality is required. The extra ISR argument is also a bad idea, it has
caused problems in other architectures and my intention was to keep the
Cortex-M architecture clean and not have it.

There is maybe a case for defining a CDL option that controls this piece of
code and have that depend on CTRLC and BREAK support, as well as allowing it to
be enabled by any other package.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001111] Access to saved registers from Cortex-M ISR

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001111

--- Comment #2 from John Dallaway <john@dallaway.org.uk> 2011-01-07 14:35:52 GMT ---
Nick, thanks for your comments.

(In reply to comment #1)

> The intention with the code as it was written is that if anything needs access
> to the interrupt state then it should enable the existing code that saves a
> full and complete CPU state and stores it in hal_saved_interrupt_state; either
> by defining the CTRLC support option or by new means. Interested code can then
> fetch it from there. That way this code is only included when this
> functionality is required.

I looked at hal_saved_interrupt_state, but it is declared only when the GDB
break/Ctrl-C support is enabled. So...

> The extra ISR argument is also a bad idea, it has
> caused problems in other architectures and my intention was to keep the
> Cortex-M architecture clean and not have it.
> 
> There is maybe a case for defining a CDL option that controls this piece of
> code and have that depend on CTRLC and BREAK support, as well as allowing it
> to be enabled by any other package.

I would like to propose a CYGINT_HAL_COMMON_SAVED_INTERRUPT_STATE_REQUIRED CDL
interface which controls the declaration of 'hal_saved_interrupt_state'. The
interface would be implemented by all of the following:

 * CYGDBG_HAL_DEBUG_GDB_BREAK_SUPPORT
 * CYGDBG_HAL_DEBUG_GDB_CTRLC_SUPPORT
 * profiling CDL options within those HALs which make use of
      'hal_saved_interrupt_state' for profiling

Individual architecture HALs could then test for
CYGINT_HAL_COMMON_SAVED_INTERRUPT_STATE_REQUIRED > 0 to determine if it is
necessary to assign a value to 'hal_saved_interrupt_state'.

Other than the common HAL package, HAL packages would only require modification
if/when the use of 'hal_saved_interrupt_state' outside the context of GDB
Ctrl-C/break becomes necessary.

The advantage of a CDL interface over a CDL option in this context is that the
saved interrupt state support is automatically disabled when no-longer
required. eg When profiling was being used but is now disabled.

Again, any comments are welcome. I'll pick this up again next week.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001111] Access to saved registers from Cortex-M ISR

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001111

--- Comment #3 from Nick Garnett <nickg@ecoscentric.com> 2011-01-10 12:10:38 GMT ---
(In reply to comment #2)

> I looked at hal_saved_interrupt_state, but it is declared only when the GDB
> break/Ctrl-C support is enabled. So...

One approach would be for any interested code to define its own instance of
hal_saved_interrupt_state if the break/ctrl-c code is not included. There are
precedents for this. However...

> I would like to propose a CYGINT_HAL_COMMON_SAVED_INTERRUPT_STATE_REQUIRED CDL
> interface which controls the declaration of 'hal_saved_interrupt_state'.

This seems a perfectly sensible solution.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001111] Access to saved registers from Cortex-M ISR

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001111

John Dallaway <john@dallaway.org.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned@bugs.ecos.source |john@dallaway.org.uk
                   |ware.org                    |
         AssignedTo|unassigned@bugs.ecos.source |john@dallaway.org.uk
                   |ware.org                    |

--- Comment #4 from John Dallaway <john@dallaway.org.uk> 2011-01-10 14:27:50 GMT ---
(In reply to comment #3)
> (In reply to comment #2)
> 
> > I looked at hal_saved_interrupt_state, but it is declared only when the GDB
> > break/Ctrl-C support is enabled. So...
> 
> One approach would be for any interested code to define its own instance of
> hal_saved_interrupt_state if the break/ctrl-c code is not included. There are
> precedents for this. However...

Each package which contains such interested code would then need to be aware of
every other package which also contains interested code and there would need to
be some priority mechanism to determine which package defines the symbol.

> > I would like to propose a CYGINT_HAL_COMMON_SAVED_INTERRUPT_STATE_REQUIRED CDL
> > interface which controls the declaration of 'hal_saved_interrupt_state'.
> 
> This seems a perfectly sensible solution.

OK. I'll develop a patch along these lines. Thanks, again, for your input.

--- Comment #5 from John Dallaway <john@dallaway.org.uk> 2011-01-10 14:27:50 GMT ---
(In reply to comment #3)
> (In reply to comment #2)
> 
> > I looked at hal_saved_interrupt_state, but it is declared only when the GDB
> > break/Ctrl-C support is enabled. So...
> 
> One approach would be for any interested code to define its own instance of
> hal_saved_interrupt_state if the break/ctrl-c code is not included. There are
> precedents for this. However...

Each package which contains such interested code would then need to be aware of
every other package which also contains interested code and there would need to
be some priority mechanism to determine which package defines the symbol.

> > I would like to propose a CYGINT_HAL_COMMON_SAVED_INTERRUPT_STATE_REQUIRED CDL
> > interface which controls the declaration of 'hal_saved_interrupt_state'.
> 
> This seems a perfectly sensible solution.

OK. I'll develop a patch along these lines. Thanks, again, for your input.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001111] Access to saved registers from Cortex-M ISR

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001111

John Dallaway <john@dallaway.org.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned@bugs.ecos.source |john@dallaway.org.uk
                   |ware.org                    |
         AssignedTo|unassigned@bugs.ecos.source |john@dallaway.org.uk
                   |ware.org                    |

--- Comment #4 from John Dallaway <john@dallaway.org.uk> 2011-01-10 14:27:50 GMT ---
(In reply to comment #3)
> (In reply to comment #2)
> 
> > I looked at hal_saved_interrupt_state, but it is declared only when the GDB
> > break/Ctrl-C support is enabled. So...
> 
> One approach would be for any interested code to define its own instance of
> hal_saved_interrupt_state if the break/ctrl-c code is not included. There are
> precedents for this. However...

Each package which contains such interested code would then need to be aware of
every other package which also contains interested code and there would need to
be some priority mechanism to determine which package defines the symbol.

> > I would like to propose a CYGINT_HAL_COMMON_SAVED_INTERRUPT_STATE_REQUIRED CDL
> > interface which controls the declaration of 'hal_saved_interrupt_state'.
> 
> This seems a perfectly sensible solution.

OK. I'll develop a patch along these lines. Thanks, again, for your input.

--- Comment #5 from John Dallaway <john@dallaway.org.uk> 2011-01-10 14:27:50 GMT ---
(In reply to comment #3)
> (In reply to comment #2)
> 
> > I looked at hal_saved_interrupt_state, but it is declared only when the GDB
> > break/Ctrl-C support is enabled. So...
> 
> One approach would be for any interested code to define its own instance of
> hal_saved_interrupt_state if the break/ctrl-c code is not included. There are
> precedents for this. However...

Each package which contains such interested code would then need to be aware of
every other package which also contains interested code and there would need to
be some priority mechanism to determine which package defines the symbol.

> > I would like to propose a CYGINT_HAL_COMMON_SAVED_INTERRUPT_STATE_REQUIRED CDL
> > interface which controls the declaration of 'hal_saved_interrupt_state'.
> 
> This seems a perfectly sensible solution.

OK. I'll develop a patch along these lines. Thanks, again, for your input.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001111] Access to saved registers from Cortex-M ISR

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001111

John Dallaway <john@dallaway.org.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |1001112

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001111] Access to saved registers from Cortex-M ISR

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001111

John Dallaway <john@dallaway.org.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |CURRENTRELEASE

--- Comment #8 from John Dallaway <john@dallaway.org.uk> 2011-01-13 18:01:35 GMT ---
Patch checked-in.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001111] Access to saved registers from Cortex-M ISR

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001111

--- Comment #7 from Nick Garnett <nickg@ecoscentric.com> 2011-01-13 17:36:47 GMT ---
John,

It looks OK to me. Check it in!

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001111] Access to saved registers from Cortex-M ISR

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001111

John Dallaway <john@dallaway.org.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1072|0                           |1
        is obsolete|                            |

--- Comment #6 from John Dallaway <john@dallaway.org.uk> 2011-01-13 11:47:29 GMT ---
Created an attachment (id=1074)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1074)
cortexm-intr-state-110113.patch

This patch fixes the reported issue by providing a CDL interface to control the
definition of the hal_saved_interrupt_state symbol used for GDB Ctrl-C break
and certain profiling ISR implementations. This requires no immediate changes
elsewhere.

The patch also modifies the Cortex-M architecture HAL to test
CYGINT_HAL_COMMON_SAVED_INTERRUPT_STATE_REQUIRED in order to determine whether
to save interrupt state. Other architecture HALs may be modified to do likewise
in the future.

Any HAL packages associated with such modified architecture HALs may implement
CYGINT_HAL_COMMON_SAVED_INTERRUPT_STATE_REQUIRED to ensure that interrupt state
will be accessible via hal_saved_interrupt_state.

Any further comments?

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001111] Access to saved registers from Cortex-M ISR

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001111

John Dallaway <john@dallaway.org.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned@bugs.ecos.source |john@dallaway.org.uk
                   |ware.org                    |
         AssignedTo|unassigned@bugs.ecos.source |john@dallaway.org.uk
                   |ware.org                    |

--- Comment #4 from John Dallaway <john@dallaway.org.uk> 2011-01-10 14:27:50 GMT ---
(In reply to comment #3)
> (In reply to comment #2)
> 
> > I looked at hal_saved_interrupt_state, but it is declared only when the GDB
> > break/Ctrl-C support is enabled. So...
> 
> One approach would be for any interested code to define its own instance of
> hal_saved_interrupt_state if the break/ctrl-c code is not included. There are
> precedents for this. However...

Each package which contains such interested code would then need to be aware of
every other package which also contains interested code and there would need to
be some priority mechanism to determine which package defines the symbol.

> > I would like to propose a CYGINT_HAL_COMMON_SAVED_INTERRUPT_STATE_REQUIRED CDL
> > interface which controls the declaration of 'hal_saved_interrupt_state'.
> 
> This seems a perfectly sensible solution.

OK. I'll develop a patch along these lines. Thanks, again, for your input.

--- Comment #5 from John Dallaway <john@dallaway.org.uk> 2011-01-10 14:27:50 GMT ---
(In reply to comment #3)
> (In reply to comment #2)
> 
> > I looked at hal_saved_interrupt_state, but it is declared only when the GDB
> > break/Ctrl-C support is enabled. So...
> 
> One approach would be for any interested code to define its own instance of
> hal_saved_interrupt_state if the break/ctrl-c code is not included. There are
> precedents for this. However...

Each package which contains such interested code would then need to be aware of
every other package which also contains interested code and there would need to
be some priority mechanism to determine which package defines the symbol.

> > I would like to propose a CYGINT_HAL_COMMON_SAVED_INTERRUPT_STATE_REQUIRED CDL
> > interface which controls the declaration of 'hal_saved_interrupt_state'.
> 
> This seems a perfectly sensible solution.

OK. I'll develop a patch along these lines. Thanks, again, for your input.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001111] Access to saved registers from Cortex-M ISR

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001111

John Dallaway <john@dallaway.org.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned@bugs.ecos.source |john@dallaway.org.uk
                   |ware.org                    |
         AssignedTo|unassigned@bugs.ecos.source |john@dallaway.org.uk
                   |ware.org                    |

--- Comment #4 from John Dallaway <john@dallaway.org.uk> 2011-01-10 14:27:50 GMT ---
(In reply to comment #3)
> (In reply to comment #2)
> 
> > I looked at hal_saved_interrupt_state, but it is declared only when the GDB
> > break/Ctrl-C support is enabled. So...
> 
> One approach would be for any interested code to define its own instance of
> hal_saved_interrupt_state if the break/ctrl-c code is not included. There are
> precedents for this. However...

Each package which contains such interested code would then need to be aware of
every other package which also contains interested code and there would need to
be some priority mechanism to determine which package defines the symbol.

> > I would like to propose a CYGINT_HAL_COMMON_SAVED_INTERRUPT_STATE_REQUIRED CDL
> > interface which controls the declaration of 'hal_saved_interrupt_state'.
> 
> This seems a perfectly sensible solution.

OK. I'll develop a patch along these lines. Thanks, again, for your input.

--- Comment #5 from John Dallaway <john@dallaway.org.uk> 2011-01-10 14:27:50 GMT ---
(In reply to comment #3)
> (In reply to comment #2)
> 
> > I looked at hal_saved_interrupt_state, but it is declared only when the GDB
> > break/Ctrl-C support is enabled. So...
> 
> One approach would be for any interested code to define its own instance of
> hal_saved_interrupt_state if the break/ctrl-c code is not included. There are
> precedents for this. However...

Each package which contains such interested code would then need to be aware of
every other package which also contains interested code and there would need to
be some priority mechanism to determine which package defines the symbol.

> > I would like to propose a CYGINT_HAL_COMMON_SAVED_INTERRUPT_STATE_REQUIRED CDL
> > interface which controls the declaration of 'hal_saved_interrupt_state'.
> 
> This seems a perfectly sensible solution.

OK. I'll develop a patch along these lines. Thanks, again, for your input.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001111] Access to saved registers from Cortex-M ISR

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001111

--- Comment #3 from Nick Garnett <nickg@ecoscentric.com> 2011-01-10 12:10:38 GMT ---
(In reply to comment #2)

> I looked at hal_saved_interrupt_state, but it is declared only when the GDB
> break/Ctrl-C support is enabled. So...

One approach would be for any interested code to define its own instance of
hal_saved_interrupt_state if the break/ctrl-c code is not included. There are
precedents for this. However...

> I would like to propose a CYGINT_HAL_COMMON_SAVED_INTERRUPT_STATE_REQUIRED CDL
> interface which controls the declaration of 'hal_saved_interrupt_state'.

This seems a perfectly sensible solution.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001111] Access to saved registers from Cortex-M ISR

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001111

--- Comment #2 from John Dallaway <john@dallaway.org.uk> 2011-01-07 14:35:52 GMT ---
Nick, thanks for your comments.

(In reply to comment #1)

> The intention with the code as it was written is that if anything needs access
> to the interrupt state then it should enable the existing code that saves a
> full and complete CPU state and stores it in hal_saved_interrupt_state; either
> by defining the CTRLC support option or by new means. Interested code can then
> fetch it from there. That way this code is only included when this
> functionality is required.

I looked at hal_saved_interrupt_state, but it is declared only when the GDB
break/Ctrl-C support is enabled. So...

> The extra ISR argument is also a bad idea, it has
> caused problems in other architectures and my intention was to keep the
> Cortex-M architecture clean and not have it.
> 
> There is maybe a case for defining a CDL option that controls this piece of
> code and have that depend on CTRLC and BREAK support, as well as allowing it
> to be enabled by any other package.

I would like to propose a CYGINT_HAL_COMMON_SAVED_INTERRUPT_STATE_REQUIRED CDL
interface which controls the declaration of 'hal_saved_interrupt_state'. The
interface would be implemented by all of the following:

 * CYGDBG_HAL_DEBUG_GDB_BREAK_SUPPORT
 * CYGDBG_HAL_DEBUG_GDB_CTRLC_SUPPORT
 * profiling CDL options within those HALs which make use of
      'hal_saved_interrupt_state' for profiling

Individual architecture HALs could then test for
CYGINT_HAL_COMMON_SAVED_INTERRUPT_STATE_REQUIRED > 0 to determine if it is
necessary to assign a value to 'hal_saved_interrupt_state'.

Other than the common HAL package, HAL packages would only require modification
if/when the use of 'hal_saved_interrupt_state' outside the context of GDB
Ctrl-C/break becomes necessary.

The advantage of a CDL interface over a CDL option in this context is that the
saved interrupt state support is automatically disabled when no-longer
required. eg When profiling was being used but is now disabled.

Again, any comments are welcome. I'll pick this up again next week.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

[Bug 1001111] Access to saved registers from Cortex-M ISR

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001111

Nick Garnett <nickg@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |nickg@ecoscentric.com

--- Comment #1 from Nick Garnett <nickg@ecoscentric.com> 2011-01-07 13:39:06 GMT ---
The intention with the code as it was written is that if anything needs access
to the interrupt state then it should enable the existing code that saves a
full and complete CPU state and stores it in hal_saved_interrupt_state; either
by defining the CTRLC support option or by new means. Interested code can then
fetch it from there. That way this code is only included when this
functionality is required. The extra ISR argument is also a bad idea, it has
caused problems in other architectures and my intention was to keep the
Cortex-M architecture clean and not have it.

There is maybe a case for defining a CDL option that controls this piece of
code and have that depend on CTRLC and BREAK support, as well as allowing it to
be enabled by any other package.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.