You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/01/10 07:04:06 UTC
[GitHub] [incubator-nuttx] GUIDINGLI opened a new pull request #5198: xtensa: fix lack of float register save & resotre
GUIDINGLI opened a new pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198
Signed-off-by: ligd <li...@xiaomi.com>
## Summary
xtensa: fix lack of float register save & resotre
## Impact
xtensa/hifi
## Testing
VELA
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#issuecomment-1008591727
@GUIDINGLI please fix the style.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#discussion_r781396898
##########
File path: arch/xtensa/include/xtensa/xtensa_coproc.h
##########
@@ -38,6 +38,7 @@
* Included Files
****************************************************************************/
+#include <assert.h>
Review comment:
Or probably even `assert.h` should be corrected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] GUIDINGLI commented on pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#issuecomment-1008996802
> @GUIDINGLI thanks for the detailed summary. For 2. I have a commit to fix that, I was willing to submit it after finishing another work. I also have a commit to fix `top_of_stack` in `xtensa_createstack`, is this the error you were referring to in 1.?
error is here:
```
- cpstart = (uintptr_t)_CP_ALIGNDOWN(XCHAL_CP0_SA_ALIGN,
- top_of_stack -
- **XCHAL_CP1_SA_ALIGN**);
```
XCHAL_CP1_SA_ALIGN should be XTENSA_CP_SA_SIZE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#issuecomment-1009008724
@GUIDINGLI I understand that it was moved to an array, but when is this structure defined now? Before, we were allocating part of the thread's stack for that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] GUIDINGLI commented on a change in pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on a change in pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#discussion_r781234785
##########
File path: arch/xtensa/include/xtensa/xtensa_coproc.h
##########
@@ -131,19 +131,20 @@
#define XTENSA_CPENABLE 0 /* (2 bytes) coprocessors active for this thread */
#define XTENSA_CPSTORED 2 /* (2 bytes) coprocessors saved for this thread */
-#define XTENSA_CPASA 4 /* (4 bytes) ptr to aligned save area */
+#define XTENSA_CPASA 8 /* (8 bytes) ptr to aligned save area */
/****************************************************************************
* Public Types
****************************************************************************/
#ifndef __ASSEMBLY__
-struct xtensa_cpstate_s
+struct aligned_data(8) xtensa_cpstate_s
{
- uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
- uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
- uint32_t *cpasa; /* (4 bytes) Pointer to aligned save area */
+ uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
+ uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
+ uint16_t reserved[2]; /* keep cpasa aligned 8 */
Review comment:
Yes, with your suggestion, it also worked!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] GUIDINGLI commented on a change in pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on a change in pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#discussion_r781305408
##########
File path: arch/xtensa/include/xtensa/xtensa_coproc.h
##########
@@ -141,9 +141,9 @@
struct xtensa_cpstate_s
{
- uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
- uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
- uint32_t *cpasa; /* (4 bytes) Pointer to aligned save area */
+ uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
+ uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
+ uint8_t cpasa[XTENSA_CP_SA_SIZE] aligned_data(8); /* cp save area */
Review comment:
Ok, thanks and done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#discussion_r781395453
##########
File path: arch/xtensa/include/xtensa/xtensa_coproc.h
##########
@@ -38,6 +38,7 @@
* Included Files
****************************************************************************/
+#include <assert.h>
Review comment:
```suggestion
#ifndef __ASSEMBLY__
# include <assert.h>
#endif
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] GUIDINGLI edited a comment on pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
GUIDINGLI edited a comment on pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#issuecomment-1008996802
> @GUIDINGLI thanks for the detailed summary. For 2. I have a commit to fix that, I was willing to submit it after finishing another work. I also have a commit to fix `top_of_stack` in `xtensa_createstack`, is this the error you were referring to in 1.?
error is here:
```
- cpstart = (uintptr_t)_CP_ALIGNDOWN(XCHAL_CP0_SA_ALIGN,
- top_of_stack -
- XCHAL_CP1_SA_ALIGN);
```
XCHAL_CP1_SA_ALIGN should be XTENSA_CP_SA_SIZE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] GUIDINGLI edited a comment on pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
GUIDINGLI edited a comment on pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#issuecomment-1009022365
> @GUIDINGLI I understand that it was moved to an array, but when is this structure defined now? Before, we were allocating part of the thread's stack for that.
@Ouss4
After this PR, `struct xtensa_cpstate_s cpstate;` will be in `struct xcptcontext` which belong to `struct tcb_s`.
So,
before: cpstate - part of the thread's stack
after: cpstate - part of the `struct tcb_s` in heap
This will simplify the logic and no need to set `cpstate` any more.
138 struct xcptcontext
139 {
140 /* The following function pointer is non-zero if there are pending signals
141 * to be processed.
142 */
143
144 void *sigdeliver; /* Actual type is sig_deliver_t */
145
146 /* These are saved copies of registers used during signal processing.
147 *
148 * REVISIT: Because there is only one copy of these save areas,
149 * only a single signal handler can be active. This precludes
150 * queuing of signal actions. As a result, signals received while
151 * another signal handler is executing will be ignored!
152 */
153
154 uint32_t saved_pc;
155 uint32_t saved_ps;
156
157 /* Register save area */
158
159 uint32_t regs[XCPTCONTEXT_REGS];
160
161 #if XCHAL_CP_NUM > 0
162 /* Co-processor save area */
163
164 struct xtensa_cpstate_s cpstate; ------------------------------------------------------------------------------------- here
165 #endif
166
167 #ifdef CONFIG_LIB_SYSCALL
168 /* The following array holds the return address and the exc_return value
169 * needed to return from each nested system call.
170 */
171
172 uint8_t nsyscalls;
173 struct xcpt_syscall_s syscall[CONFIG_SYS_NNEST];
174 #endif
175 };
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] GUIDINGLI edited a comment on pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
GUIDINGLI edited a comment on pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#issuecomment-1009022365
> @GUIDINGLI I understand that it was moved to an array, but when is this structure defined now? Before, we were allocating part of the thread's stack for that.
@Ouss4
After this PR, `struct xtensa_cpstate_s cpstate;` will be in `struct xcptcontext` which belong to `struct tcb_s`.
So,
before: cpstate - part of the thread's stack
after: cpstate - part of the `struct tcb_s` in heap
This will simplify the logic and no need to set `cpstate` any more.
138 struct xcptcontext
139 {
140 /* The following function pointer is non-zero if there are pending signals
141 * to be processed.
142 */
143
144 void *sigdeliver; /* Actual type is sig_deliver_t */
145
146 /* These are saved copies of registers used during signal processing.
147 *
148 * REVISIT: Because there is only one copy of these save areas,
149 * only a single signal handler can be active. This precludes
150 * queuing of signal actions. As a result, signals received while
151 * another signal handler is executing will be ignored!
152 */
153
154 uint32_t saved_pc;
155 uint32_t saved_ps;
156
157 /* Register save area */
158
159 uint32_t regs[XCPTCONTEXT_REGS];
160
161 #if XCHAL_CP_NUM > 0
162 /* Co-processor save area */
163
164 struct xtensa_cpstate_s cpstate; ---------------------------------- here
165 #endif
166
167 #ifdef CONFIG_LIB_SYSCALL
168 /* The following array holds the return address and the exc_return value
169 * needed to return from each nested system call.
170 */
171
172 uint8_t nsyscalls;
173 struct xcpt_syscall_s syscall[CONFIG_SYS_NNEST];
174 #endif
175 };
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#issuecomment-1009025183
@GUIDINGLI yes, you are right. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#discussion_r781282406
##########
File path: arch/xtensa/include/xtensa/xtensa_coproc.h
##########
@@ -141,9 +141,9 @@
struct xtensa_cpstate_s
{
- uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
- uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
- uint32_t *cpasa; /* (4 bytes) Pointer to aligned save area */
+ uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
+ uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
+ uint8_t cpasa[XTENSA_CP_SA_SIZE] aligned_data(8); /* cp save area */
Review comment:
I think it is good to add `_Static_assert(offsetof(struct xtensa_cpstate_s, cpasa) == XTENSA_CPASA, "CP save area address alignment violation.");` just as an insurance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] GUIDINGLI edited a comment on pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
GUIDINGLI edited a comment on pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#issuecomment-1009022365
> @GUIDINGLI I understand that it was moved to an array, but when is this structure defined now? Before, we were allocating part of the thread's stack for that.
@Ouss4
After this PR, `struct xtensa_cpstate_s cpstate;` will be in `struct xcptcontext` which belong to `struct tcb_s`.
So,
before: cpstate - part of the thread's stack
after: cpstate - part of the `struct tcb_s` in heap
This will simplify the logic and no need to set `cpstate` any more.
···
138 struct xcptcontext
139 {
140 /* The following function pointer is non-zero if there are pending signals
141 * to be processed.
142 */
143
144 void *sigdeliver; /* Actual type is sig_deliver_t */
145
146 /* These are saved copies of registers used during signal processing.
147 *
148 * REVISIT: Because there is only one copy of these save areas,
149 * only a single signal handler can be active. This precludes
150 * queuing of signal actions. As a result, signals received while
151 * another signal handler is executing will be ignored!
152 */
153
154 uint32_t saved_pc;
155 uint32_t saved_ps;
156
157 /* Register save area */
158
159 uint32_t regs[XCPTCONTEXT_REGS];
160
161 #if XCHAL_CP_NUM > 0
162 /* Co-processor save area */
163
164 struct xtensa_cpstate_s cpstate; ------------------------------------------------------------------------------------- here
165 #endif
166
167 #ifdef CONFIG_LIB_SYSCALL
168 /* The following array holds the return address and the exc_return value
169 * needed to return from each nested system call.
170 */
171
172 uint8_t nsyscalls;
173 struct xcpt_syscall_s syscall[CONFIG_SYS_NNEST];
174 #endif
175 };
···
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] GUIDINGLI commented on pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#issuecomment-1009022365
> @GUIDINGLI I understand that it was moved to an array, but when is this structure defined now? Before, we were allocating part of the thread's stack for that.
@Ouss4
After this PR, `struct xtensa_cpstate_s cpstate;` will be in `struct xcptcontext` which belong to `struct tcb_s`.
So,
before: cpstate - part of the thread's stack
after: cpstate - part of the `struct tcb_s` in heap
This will simplify the logic and no need to set `cpstate` any more.
138 struct xcptcontext
139 {
140 /* The following function pointer is non-zero if there are pending signals
141 * to be processed.
142 */
143
144 void *sigdeliver; /* Actual type is sig_deliver_t */
145
146 /* These are saved copies of registers used during signal processing.
147 *
148 * REVISIT: Because there is only one copy of these save areas,
149 * only a single signal handler can be active. This precludes
150 * queuing of signal actions. As a result, signals received while
151 * another signal handler is executing will be ignored!
152 */
153
154 uint32_t saved_pc;
155 uint32_t saved_ps;
156
157 /* Register save area */
158
159 uint32_t regs[XCPTCONTEXT_REGS];
160
161 #if XCHAL_CP_NUM > 0
162 /* Co-processor save area */
163
164 struct xtensa_cpstate_s cpstate;
165 #endif
166
167 #ifdef CONFIG_LIB_SYSCALL
168 /* The following array holds the return address and the exc_return value
169 * needed to return from each nested system call.
170 */
171
172 uint8_t nsyscalls;
173 struct xcpt_syscall_s syscall[CONFIG_SYS_NNEST];
174 #endif
175 };
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 edited a comment on pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
Ouss4 edited a comment on pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#issuecomment-1009011022
BTW, in the previous implementation we were missing the metadata as well, so the size needed to account for that: `#define XTENSA_CP_SIZE (8 + XTENSA_CP_SA_SIZE + XCHAL_TOTAL_SA_ALIGN)`. 8 is for cpenable (2bytes), cpstored(2bytes) and the pointer to the save area (4bytes).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#discussion_r781326498
##########
File path: arch/xtensa/include/xtensa/xtensa_coproc.h
##########
@@ -141,11 +141,14 @@
struct xtensa_cpstate_s
{
- uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
- uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
- uint32_t *cpasa; /* (4 bytes) Pointer to aligned save area */
+ uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
+ uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
+ uint8_t cpasa[XTENSA_CP_SA_SIZE] aligned_data(8); /* cp save area */
};
+_Static_assert(offsetof(struct xtensa_cpstate_s, cpasa) == XTENSA_CPASA,
Review comment:
There is
```
#ifndef __cplusplus
# define static_assert _Static_assert
#endif
```
in `include/assert.h`, so better to use that one
```suggestion
static_assert(offsetof(struct xtensa_cpstate_s, cpasa) == XTENSA_CPASA,
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#discussion_r781284446
##########
File path: arch/xtensa/include/xtensa/xtensa_coproc.h
##########
@@ -131,7 +131,7 @@
#define XTENSA_CPENABLE 0 /* (2 bytes) coprocessors active for this thread */
#define XTENSA_CPSTORED 2 /* (2 bytes) coprocessors saved for this thread */
-#define XTENSA_CPASA 4 /* (4 bytes) ptr to aligned save area */
+#define XTENSA_CPASA 8 /* (8 bytes) ptr to aligned save area */
Review comment:
Why is this 8 bytes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#discussion_r781401751
##########
File path: arch/xtensa/include/xtensa/xtensa_coproc.h
##########
@@ -38,6 +38,7 @@
* Included Files
****************************************************************************/
+#include <assert.h>
Review comment:
Please check if this will help https://github.com/apache/incubator-nuttx/pull/5200
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#issuecomment-1009713228
@Ouss4 could you take a look again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#discussion_r781009681
##########
File path: arch/xtensa/include/xtensa/xtensa_coproc.h
##########
@@ -131,19 +131,20 @@
#define XTENSA_CPENABLE 0 /* (2 bytes) coprocessors active for this thread */
#define XTENSA_CPSTORED 2 /* (2 bytes) coprocessors saved for this thread */
-#define XTENSA_CPASA 4 /* (4 bytes) ptr to aligned save area */
+#define XTENSA_CPASA 8 /* (8 bytes) ptr to aligned save area */
/****************************************************************************
* Public Types
****************************************************************************/
#ifndef __ASSEMBLY__
-struct xtensa_cpstate_s
+struct aligned_data(8) xtensa_cpstate_s
{
- uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
- uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
- uint32_t *cpasa; /* (4 bytes) Pointer to aligned save area */
+ uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
+ uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
+ uint16_t reserved[2]; /* keep cpasa aligned 8 */
+ uint8_t cpasa[XTENSA_CP_SA_SIZE]; /* cp save area */
Review comment:
Why was the save area moved here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] GUIDINGLI commented on a change in pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on a change in pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#discussion_r781299918
##########
File path: arch/xtensa/include/xtensa/xtensa_coproc.h
##########
@@ -131,7 +131,7 @@
#define XTENSA_CPENABLE 0 /* (2 bytes) coprocessors active for this thread */
#define XTENSA_CPSTORED 2 /* (2 bytes) coprocessors saved for this thread */
-#define XTENSA_CPASA 4 /* (4 bytes) ptr to aligned save area */
+#define XTENSA_CPASA 8 /* (8 bytes) ptr to aligned save area */
Review comment:
#define XTENSA_CPENABLE 0 /* (2 bytes) coprocessors active for this thread */
#define XTENSA_CPSTORED 2 /* (2 bytes) coprocessors saved for this thread */
-#define XTENSA_CPASA 4 /* (4 bytes) ptr to aligned save area */
+#define XTENSA_CPASA 8 /* (8 bytes) ptr to aligned save area */
/****************************************************************************
* Public Types
@@ -141,9 +141,9 @@
struct xtensa_cpstate_s
{
- uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
- uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
- uint32_t *cpasa; /* (4 bytes) Pointer to aligned save area */
+ uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
+ uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
+ uint8_t cpasa[XTENSA_CP_SA_SIZE] aligned_data(8); /* cp save area */
};
0 - cpenable
2 - cpstored
8 - cpasa
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 merged pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
Ouss4 merged pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#discussion_r781326498
##########
File path: arch/xtensa/include/xtensa/xtensa_coproc.h
##########
@@ -141,11 +141,14 @@
struct xtensa_cpstate_s
{
- uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
- uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
- uint32_t *cpasa; /* (4 bytes) Pointer to aligned save area */
+ uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
+ uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
+ uint8_t cpasa[XTENSA_CP_SA_SIZE] aligned_data(8); /* cp save area */
};
+_Static_assert(offsetof(struct xtensa_cpstate_s, cpasa) == XTENSA_CPASA,
Review comment:
There is
```
#ifndef __cplusplus
# define static_assert _Static_assert
#endif
```
in `include/assert.h`, so better to use that one
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#discussion_r781479176
##########
File path: arch/xtensa/include/xtensa/xtensa_coproc.h
##########
@@ -38,6 +38,7 @@
* Included Files
****************************************************************************/
+#include <assert.h>
Review comment:
Please rebase PR on top of the latest master branch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] GUIDINGLI commented on pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#issuecomment-1008999432
> @GUIDINGLI when is the coprocessors save area allocated now?
@Ouss4
```
struct xtensa_cpstate_s
{
- uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
- uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
- uint32_t *cpasa; /* (4 bytes) Pointer to aligned save area */
+ uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
+ uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
+ uint8_t cpasa[XTENSA_CP_SA_SIZE] aligned_data(8); /* cp save area */
};
```
use array instead of stack pointer.
So no need set address for `cpasa`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#discussion_r781394648
##########
File path: arch/xtensa/include/xtensa/xtensa_coproc.h
##########
@@ -38,6 +38,7 @@
* Included Files
****************************************************************************/
+#include <assert.h>
Review comment:
probably `<assert.h>` is not ready to be included from `__ASSEMBLY__`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#issuecomment-1009011022
BTW, in the previous implementation we were missing the metadata as well, so the size needed to account for that: `#define XTENSA_CP_SIZE (8 + XTENSA_CP_SA_SIZE + XCHAL_TOTAL_SA_ALIGN)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#discussion_r781327880
##########
File path: tools/nxstyle.c
##########
@@ -182,27 +182,28 @@ static const struct file_section_s g_section_info[] =
static const char *g_white_prefix[] =
{
- "Elf", /* Ref: include/elf.h, include/elf32.h, include/elf64.h */
- "PRId", /* Ref: inttypes.h */
- "PRIi", /* Ref: inttypes.h */
- "PRIo", /* Ref: inttypes.h */
- "PRIu", /* Ref: inttypes.h */
- "PRIx", /* Ref: inttypes.h */
- "SCNd", /* Ref: inttypes.h */
- "SCNi", /* Ref: inttypes.h */
- "SCNo", /* Ref: inttypes.h */
- "SCNu", /* Ref: inttypes.h */
- "SCNx", /* Ref: inttypes.h */
- "SYS_", /* Ref: include/sys/syscall.h */
- "STUB_", /* Ref: syscall/syscall_lookup.h, syscall/sycall_stublookup.c */
- "b8", /* Ref: include/fixedmath.h */
- "b16", /* Ref: include/fixedmath.h */
- "b32", /* Ref: include/fixedmath.h */
- "ub8", /* Ref: include/fixedmath.h */
- "ub16", /* Ref: include/fixedmath.h */
- "ub32", /* Ref: include/fixedmath.h */
- "ASCII_", /* Ref: include/nuttx/ascii.h */
- "XK_", /* Ref: include/input/X11_keysymdef.h */
+ "Elf", /* Ref: include/elf.h, include/elf32.h, include/elf64.h */
+ "PRId", /* Ref: inttypes.h */
+ "PRIi", /* Ref: inttypes.h */
+ "PRIo", /* Ref: inttypes.h */
+ "PRIu", /* Ref: inttypes.h */
+ "PRIx", /* Ref: inttypes.h */
+ "SCNd", /* Ref: inttypes.h */
+ "SCNi", /* Ref: inttypes.h */
+ "SCNo", /* Ref: inttypes.h */
+ "SCNu", /* Ref: inttypes.h */
+ "SCNx", /* Ref: inttypes.h */
+ "SYS_", /* Ref: include/sys/syscall.h */
+ "STUB_", /* Ref: syscall/syscall_lookup.h, syscall/sycall_stublookup.c */
+ "b8", /* Ref: include/fixedmath.h */
+ "b16", /* Ref: include/fixedmath.h */
+ "b32", /* Ref: include/fixedmath.h */
+ "ub8", /* Ref: include/fixedmath.h */
+ "ub16", /* Ref: include/fixedmath.h */
+ "ub32", /* Ref: include/fixedmath.h */
+ "ASCII_", /* Ref: include/nuttx/ascii.h */
+ "XK_", /* Ref: include/input/X11_keysymdef.h */
+ "_Static_assert", /* Ref: arch/xtensa/include/xtensa/xtensa_coproc.h */
Review comment:
Probably this change can be reverted if you use `static_assert` from `assert.h`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#discussion_r781325407
##########
File path: tools/nxstyle.c
##########
@@ -182,27 +182,28 @@ static const struct file_section_s g_section_info[] =
static const char *g_white_prefix[] =
{
- "Elf", /* Ref: include/elf.h, include/elf32.h, include/elf64.h */
- "PRId", /* Ref: inttypes.h */
- "PRIi", /* Ref: inttypes.h */
- "PRIo", /* Ref: inttypes.h */
- "PRIu", /* Ref: inttypes.h */
- "PRIx", /* Ref: inttypes.h */
- "SCNd", /* Ref: inttypes.h */
- "SCNi", /* Ref: inttypes.h */
- "SCNo", /* Ref: inttypes.h */
- "SCNu", /* Ref: inttypes.h */
- "SCNx", /* Ref: inttypes.h */
- "SYS_", /* Ref: include/sys/syscall.h */
- "STUB_", /* Ref: syscall/syscall_lookup.h, syscall/sycall_stublookup.c */
- "b8", /* Ref: include/fixedmath.h */
- "b16", /* Ref: include/fixedmath.h */
- "b32", /* Ref: include/fixedmath.h */
- "ub8", /* Ref: include/fixedmath.h */
- "ub16", /* Ref: include/fixedmath.h */
- "ub32", /* Ref: include/fixedmath.h */
- "ASCII_", /* Ref: include/nuttx/ascii.h */
- "XK_", /* Ref: include/input/X11_keysymdef.h */
+ "Elf", /* Ref: include/elf.h, include/elf32.h, include/elf64.h */
+ "PRId", /* Ref: inttypes.h */
+ "PRIi", /* Ref: inttypes.h */
+ "PRIo", /* Ref: inttypes.h */
+ "PRIu", /* Ref: inttypes.h */
+ "PRIx", /* Ref: inttypes.h */
+ "SCNd", /* Ref: inttypes.h */
+ "SCNi", /* Ref: inttypes.h */
+ "SCNo", /* Ref: inttypes.h */
+ "SCNu", /* Ref: inttypes.h */
+ "SCNx", /* Ref: inttypes.h */
+ "SYS_", /* Ref: include/sys/syscall.h */
+ "STUB_", /* Ref: syscall/syscall_lookup.h, syscall/sycall_stublookup.c */
+ "b8", /* Ref: include/fixedmath.h */
+ "b16", /* Ref: include/fixedmath.h */
+ "b32", /* Ref: include/fixedmath.h */
+ "ub8", /* Ref: include/fixedmath.h */
+ "ub16", /* Ref: include/fixedmath.h */
+ "ub32", /* Ref: include/fixedmath.h */
+ "ASCII_", /* Ref: include/nuttx/ascii.h */
+ "XK_", /* Ref: include/input/X11_keysymdef.h */
+ "_Static_assert", /* Ref: arch/xtensa/include/xtensa/xtensa_coproc.h */
Review comment:
```suggestion
"_Static_assert", /* Ref: arch/xtensa/include/xtensa/xtensa_coproc.h */
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#discussion_r780976470
##########
File path: arch/xtensa/include/xtensa/xtensa_coproc.h
##########
@@ -131,19 +131,20 @@
#define XTENSA_CPENABLE 0 /* (2 bytes) coprocessors active for this thread */
#define XTENSA_CPSTORED 2 /* (2 bytes) coprocessors saved for this thread */
-#define XTENSA_CPASA 4 /* (4 bytes) ptr to aligned save area */
+#define XTENSA_CPASA 8 /* (8 bytes) ptr to aligned save area */
/****************************************************************************
* Public Types
****************************************************************************/
#ifndef __ASSEMBLY__
-struct xtensa_cpstate_s
+struct aligned_data(8) xtensa_cpstate_s
{
- uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
- uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
- uint32_t *cpasa; /* (4 bytes) Pointer to aligned save area */
+ uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
+ uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
+ uint16_t reserved[2]; /* keep cpasa aligned 8 */
Review comment:
Minor: using `uint16_t reserved[2]` is fine, but I would recommend.
```suggestion
uint8_t reserved[4]; /* keep cpasa aligned 8 */
```
Also a question can we do `uint8_t cpasa[XTENSA_CP_SA_SIZE] aligned_data(8); /* cp save area */` will work with `reserved` removed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] GUIDINGLI commented on a change in pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on a change in pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#discussion_r781242859
##########
File path: arch/xtensa/include/xtensa/xtensa_coproc.h
##########
@@ -131,19 +131,20 @@
#define XTENSA_CPENABLE 0 /* (2 bytes) coprocessors active for this thread */
#define XTENSA_CPSTORED 2 /* (2 bytes) coprocessors saved for this thread */
-#define XTENSA_CPASA 4 /* (4 bytes) ptr to aligned save area */
+#define XTENSA_CPASA 8 /* (8 bytes) ptr to aligned save area */
/****************************************************************************
* Public Types
****************************************************************************/
#ifndef __ASSEMBLY__
-struct xtensa_cpstate_s
+struct aligned_data(8) xtensa_cpstate_s
{
- uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
- uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
- uint32_t *cpasa; /* (4 bytes) Pointer to aligned save area */
+ uint16_t cpenable; /* (2 bytes) Co-processors active for this thread */
+ uint16_t cpstored; /* (2 bytes) Co-processors saved for this thread */
+ uint16_t reserved[2]; /* keep cpasa aligned 8 */
+ uint8_t cpasa[XTENSA_CP_SA_SIZE]; /* cp save area */
Review comment:
See the Summary
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#issuecomment-1008966568
@GUIDINGLI thanks for the detailed summary. For 2. I have a commit to fix that, I was willing to submit it after finishing another work. I also have a commit to fix `top_of_stack` in `xtensa_createstack`, is this the error you were referring to in 1.?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] GUIDINGLI edited a comment on pull request #5198: xtensa: fix lack of float register save & resotre
Posted by GitBox <gi...@apache.org>.
GUIDINGLI edited a comment on pull request #5198:
URL: https://github.com/apache/incubator-nuttx/pull/5198#issuecomment-1009022365
> @GUIDINGLI I understand that it was moved to an array, but when is this structure defined now? Before, we were allocating part of the thread's stack for that.
@Ouss4
After this PR, `struct xtensa_cpstate_s cpstate;` will be in `struct xcptcontext` which belong to `struct tcb_s`.
So,
before: cpstate - part of the thread's stack
after: cpstate - part of the `struct tcb_s` in heap
This will simplify the logic and no need to set `cpstate` any more.
```
138 struct xcptcontext
139 {
140 /* The following function pointer is non-zero if there are pending signals
141 * to be processed.
142 */
143
144 void *sigdeliver; /* Actual type is sig_deliver_t */
145
146 /* These are saved copies of registers used during signal processing.
147 *
148 * REVISIT: Because there is only one copy of these save areas,
149 * only a single signal handler can be active. This precludes
150 * queuing of signal actions. As a result, signals received while
151 * another signal handler is executing will be ignored!
152 */
153
154 uint32_t saved_pc;
155 uint32_t saved_ps;
156
157 /* Register save area */
158
159 uint32_t regs[XCPTCONTEXT_REGS];
160
161 #if XCHAL_CP_NUM > 0
162 /* Co-processor save area */
163
164 struct xtensa_cpstate_s cpstate; ------------------------------------------------------------------------------------- here
165 #endif
166
167 #ifdef CONFIG_LIB_SYSCALL
168 /* The following array holds the return address and the exc_return value
169 * needed to return from each nested system call.
170 */
171
172 uint8_t nsyscalls;
173 struct xcpt_syscall_s syscall[CONFIG_SYS_NNEST];
174 #endif
175 };
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org