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/03/18 04:24:49 UTC
[GitHub] [incubator-nuttx] no1wudi opened a new pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
no1wudi opened a new pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775
## Summary
And also fix a issue that MIE should be cleared for a new task/pthread since it was handled by up_irq_enable/up_irq_disable.
## Impact
N/A
## Testing
maix-bit:smp
--
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] masayuki2009 commented on pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072075444
@no1wudi
Please add the following comment in the commit message as well.
```
And also fix a issue that MIE should be cleared for a new task/pthread since it was handled by up_irq_enable/up_irq_disable.
```
--
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 a change in pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#discussion_r829918043
##########
File path: arch/risc-v/include/csr.h
##########
@@ -307,6 +307,14 @@
#define MSTATUS_FS_CLEAN (0x2 << 13)
#define MSTATUS_FS_DIRTY (0x3 << 13)
+/* Mask of preserved bits for mstatus */
+
+#ifdef CONFIG_ARCH_RV32
+#define MSTATUS_WPRI (0xff << 23 | 0x15)
+#else
+#define MSTATUS_WPRI (0x1ffffffUL << 38 | 0x1ffUL << 23 | 0x15UL)
Review comment:
change all UL to ul
##########
File path: arch/risc-v/include/csr.h
##########
@@ -307,6 +307,14 @@
#define MSTATUS_FS_CLEAN (0x2 << 13)
#define MSTATUS_FS_DIRTY (0x3 << 13)
+/* Mask of preserved bits for mstatus */
+
+#ifdef CONFIG_ARCH_RV32
+#define MSTATUS_WPRI (0xff << 23 | 0x15)
+#else
+#define MSTATUS_WPRI (0x1ffffffUL << 38 | 0x1ffUL << 23 | 0x15UL)
Review comment:
@no1wudi not address this comment yet.
--
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] jlaitine commented on pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072204575
--
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 pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072299132
please resolve merge conflicts
--
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] no1wudi commented on pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072026909
--
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 a change in pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#discussion_r830251174
##########
File path: arch/risc-v/include/csr.h
##########
@@ -307,6 +307,14 @@
#define MSTATUS_FS_CLEAN (0x2 << 13)
#define MSTATUS_FS_DIRTY (0x3 << 13)
+/* Mask of preserved bits for mstatus */
+
+#ifdef CONFIG_ARCH_RV32
+#define MSTATUS_WPRI (0xff << 23 | 0x15)
+#else
+#define MSTATUS_WPRI (0x1ffffffUL << 38 | 0x1ffUL << 23 | 0x15UL)
Review comment:
need llu to avoid lose high 32bit value
--
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] jlaitine commented on pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072204575
I think this should be done as follows:
- read the csr
- mask the bits which should be reserved (from ISA spec)
- set all the other bits
Don't preserve any other bits here except the ones which are specified to be preserved
--
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] masayuki2009 commented on a change in pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#discussion_r829847232
##########
File path: arch/risc-v/src/common/riscv_getnewintctx.c
##########
@@ -0,0 +1,71 @@
+/****************************************************************************
+ * arch/risc-v/src/common/riscv_getnewintctx.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <stdint.h>
+#include <stdio.h>
+#include <assert.h>
+#include <debug.h>
+
+#include <nuttx/arch.h>
+#include <arch/irq.h>
+#include <arch/csr.h>
+
+#include "riscv_internal.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: riscv_get_newintctx
+ *
+ * Description:
+ * Return initial mstatus when a task is created.
+ *
+ ****************************************************************************/
+
+uintptr_t riscv_get_newintctx(void)
+{
+ /* Set machine previous privilege mode to machine mode. Reegardless of
+ * how NuttX is configured and of what kind of thread is being started.
+ * That is because all threads, even user-mode threads will start in
+ * kernel trampoline at nxtask_start() or pthread_start().
+ * The thread's privileges will be dropped before transitioning to
+ * user code. Also set machine previous interrupt enable.
+ *
+ * Clear MIE bit since it will be handled by up_irq_enable/up_irq_disable.
Review comment:
I think we should remove this comment
```
* Clear MIE bit since it will be handled by up_irq_enable/up_irq_disable.
```
--
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 #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#discussion_r829980621
##########
File path: arch/risc-v/include/csr.h
##########
@@ -307,6 +307,14 @@
#define MSTATUS_FS_CLEAN (0x2 << 13)
#define MSTATUS_FS_DIRTY (0x3 << 13)
+/* Mask of preserved bits for mstatus */
+
+#ifdef CONFIG_ARCH_RV32
+#define MSTATUS_WPRI (0xff << 23 | 0x15)
+#else
+#define MSTATUS_WPRI (0x1ffffffUL << 38 | 0x1ffUL << 23 | 0x15UL)
Review comment:
maybe even to `llu`
--
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] masayuki2009 commented on a change in pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#discussion_r829848192
##########
File path: arch/risc-v/src/common/riscv_getnewintctx.c
##########
@@ -0,0 +1,71 @@
+/****************************************************************************
+ * arch/risc-v/src/common/riscv_getnewintctx.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <stdint.h>
+#include <stdio.h>
+#include <assert.h>
+#include <debug.h>
+
+#include <nuttx/arch.h>
+#include <arch/irq.h>
+#include <arch/csr.h>
+
+#include "riscv_internal.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: riscv_get_newintctx
+ *
+ * Description:
+ * Return initial mstatus when a task is created.
+ *
+ ****************************************************************************/
+
+uintptr_t riscv_get_newintctx(void)
+{
+ /* Set machine previous privilege mode to machine mode. Reegardless of
+ * how NuttX is configured and of what kind of thread is being started.
+ * That is because all threads, even user-mode threads will start in
+ * kernel trampoline at nxtask_start() or pthread_start().
+ * The thread's privileges will be dropped before transitioning to
+ * user code. Also set machine previous interrupt enable.
+ *
+ * Clear MIE bit since it will be handled by up_irq_enable/up_irq_disable.
Review comment:
And add
```
mask the bits which should be preserved (from ISA spec)
```
--
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] jlaitine edited a comment on pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
jlaitine edited a comment on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072204575
I think this should be done as follows:
- read the csr
- mask the bits which should be preserved (from ISA spec)
- set all the other bits
Don't preserve any other bits here except the ones which are specified to be preserved
--
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 #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#discussion_r830386543
##########
File path: arch/risc-v/include/csr.h
##########
@@ -307,6 +307,14 @@
#define MSTATUS_FS_CLEAN (0x2 << 13)
#define MSTATUS_FS_DIRTY (0x3 << 13)
+/* Mask of preserved bits for mstatus */
+
+#ifdef CONFIG_ARCH_RV32
+#define MSTATUS_WPRI (0xff << 23 | 0x15)
+#else
+#define MSTATUS_WPRI (0x1fffffful << 38 | 0x1fful << 23 | 0x15ul)
Review comment:
```suggestion
#define MSTATUS_WPRI (0x1ffffffllu << 38 | 0x1ffllu << 23 | 0x15)
```
or we can go with
```suggestion
#define MSTATUS_WPRI (UINT64_C(0x1ffffff) << 38 | UINT64_C(0x1ff) << 23 | 0x15)
```
--
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] no1wudi commented on a change in pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#discussion_r829853003
##########
File path: arch/risc-v/src/common/riscv_getnewintctx.c
##########
@@ -0,0 +1,71 @@
+/****************************************************************************
+ * arch/risc-v/src/common/riscv_getnewintctx.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <stdint.h>
+#include <stdio.h>
+#include <assert.h>
+#include <debug.h>
+
+#include <nuttx/arch.h>
+#include <arch/irq.h>
+#include <arch/csr.h>
+
+#include "riscv_internal.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: riscv_get_newintctx
+ *
+ * Description:
+ * Return initial mstatus when a task is created.
+ *
+ ****************************************************************************/
+
+uintptr_t riscv_get_newintctx(void)
+{
+ /* Set machine previous privilege mode to machine mode. Reegardless of
+ * how NuttX is configured and of what kind of thread is being started.
+ * That is because all threads, even user-mode threads will start in
+ * kernel trampoline at nxtask_start() or pthread_start().
+ * The thread's privileges will be dropped before transitioning to
+ * user code. Also set machine previous interrupt enable.
+ *
+ * Clear MIE bit since it will be handled by up_irq_enable/up_irq_disable.
Review comment:
Updated, 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] no1wudi commented on pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072233141
>
> * read the csr
> * mask the bits which should be preserved (from ISA spec)
> * set all the other bits
>
> Don't preserve any other bits here except the ones which are specified to be preserved
Thanks for your suggestions
--
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] jlaitine edited a comment on pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
jlaitine edited a comment on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072204575
I think this should be done as follows:
- read the csr
- mask the bits which should be preserved (from ISA spec)
- set all the other bits
Don't preserve any other bits here except the ones which are specified to be preserved
--
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] no1wudi commented on pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072026909
@masayuki2009 Could you have a try for it ?
BTW, in exception_common we save/restore mstatus by csrr/csrw, but in up_irq_enable/up_irq_disable use the bit operation csrc/csrs, should we use bit operation for these bits while restore mstatus ?
--
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] masayuki2009 edited a comment on pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
masayuki2009 edited a comment on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072066519
@no1wudi
```
====================================================================================
Configuration/Tool: rv32m1-vega/nsh-itcm,CONFIG_RISCV_TOOLCHAIN_GNU_RVGL
------------------------------------------------------------------------------------
Cleaning...
Configuring...
Disabling CONFIG_RISCV_TOOLCHAIN_GNU_RVGL
Enabling CONFIG_RISCV_TOOLCHAIN_GNU_RVGL
Building NuttX...
riscv64-unknown-elf-ld: /github/workspace/sources/nuttx/staging/libarch.a(riscv_initialstate.o): in function `up_initial_state':
/github/workspace/sources/nuttx/arch/risc-v/src/common/riscv_initialstate.c:87: undefined reference to `riscv_get_newintctx'
make[1]: *** [Makefile:145: nuttx] Error 1
make: *** [tools/Unix.mk:509: nuttx] Error 2
```
--
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 pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072114322
@no1wudi please be aware of https://github.com/apache/incubator-nuttx/pull/5776
--
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] masayuki2009 commented on pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072066519
```
====================================================================================
Configuration/Tool: rv32m1-vega/nsh-itcm,CONFIG_RISCV_TOOLCHAIN_GNU_RVGL
------------------------------------------------------------------------------------
Cleaning...
Configuring...
Disabling CONFIG_RISCV_TOOLCHAIN_GNU_RVGL
Enabling CONFIG_RISCV_TOOLCHAIN_GNU_RVGL
Building NuttX...
riscv64-unknown-elf-ld: /github/workspace/sources/nuttx/staging/libarch.a(riscv_initialstate.o): in function `up_initial_state':
/github/workspace/sources/nuttx/arch/risc-v/src/common/riscv_initialstate.c:87: undefined reference to `riscv_get_newintctx'
make[1]: *** [Makefile:145: nuttx] Error 1
make: *** [tools/Unix.mk:509: nuttx] Error 2
```
--
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] masayuki2009 commented on pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072069818
@no1wudi
>Could you have a try for it ?
ostest with maix-bit:smp on QEMU now works.
>BTW, in exception_common we save/restore mstatus by csrr/csrw, but in up_irq_enable/up_irq_disable use the bit operation csrc/csrs, should we use bit operation for these bits while restore mstatus ?
In my understanding, we can not use csrc/csrs in exception_common to save/restore the mstatus.
--
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 #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#discussion_r829980621
##########
File path: arch/risc-v/include/csr.h
##########
@@ -307,6 +307,14 @@
#define MSTATUS_FS_CLEAN (0x2 << 13)
#define MSTATUS_FS_DIRTY (0x3 << 13)
+/* Mask of preserved bits for mstatus */
+
+#ifdef CONFIG_ARCH_RV32
+#define MSTATUS_WPRI (0xff << 23 | 0x15)
+#else
+#define MSTATUS_WPRI (0x1ffffffUL << 38 | 0x1ffUL << 23 | 0x15UL)
Review comment:
maybe even to `llu`
--
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] masayuki2009 edited a comment on pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
masayuki2009 edited a comment on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072066519
--
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 a change in pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#discussion_r830035050
##########
File path: arch/risc-v/include/csr.h
##########
@@ -307,6 +307,14 @@
#define MSTATUS_FS_CLEAN (0x2 << 13)
#define MSTATUS_FS_DIRTY (0x3 << 13)
+/* Mask of preserved bits for mstatus */
+
+#ifdef CONFIG_ARCH_RV32
+#define MSTATUS_WPRI (0xff << 23 | 0x15)
+#else
+#define MSTATUS_WPRI (0x1ffffffUL << 38 | 0x1ffUL << 23 | 0x15UL)
Review comment:
@no1wudi not address this comment yet.
--
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] masayuki2009 commented on a change in pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#discussion_r829847232
##########
File path: arch/risc-v/src/common/riscv_getnewintctx.c
##########
@@ -0,0 +1,71 @@
+/****************************************************************************
+ * arch/risc-v/src/common/riscv_getnewintctx.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <stdint.h>
+#include <stdio.h>
+#include <assert.h>
+#include <debug.h>
+
+#include <nuttx/arch.h>
+#include <arch/irq.h>
+#include <arch/csr.h>
+
+#include "riscv_internal.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: riscv_get_newintctx
+ *
+ * Description:
+ * Return initial mstatus when a task is created.
+ *
+ ****************************************************************************/
+
+uintptr_t riscv_get_newintctx(void)
+{
+ /* Set machine previous privilege mode to machine mode. Reegardless of
+ * how NuttX is configured and of what kind of thread is being started.
+ * That is because all threads, even user-mode threads will start in
+ * kernel trampoline at nxtask_start() or pthread_start().
+ * The thread's privileges will be dropped before transitioning to
+ * user code. Also set machine previous interrupt enable.
+ *
+ * Clear MIE bit since it will be handled by up_irq_enable/up_irq_disable.
Review comment:
I think we should remove this comment
```
* Clear MIE bit since it will be handled by up_irq_enable/up_irq_disable.
```
##########
File path: arch/risc-v/src/common/riscv_getnewintctx.c
##########
@@ -0,0 +1,71 @@
+/****************************************************************************
+ * arch/risc-v/src/common/riscv_getnewintctx.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <stdint.h>
+#include <stdio.h>
+#include <assert.h>
+#include <debug.h>
+
+#include <nuttx/arch.h>
+#include <arch/irq.h>
+#include <arch/csr.h>
+
+#include "riscv_internal.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: riscv_get_newintctx
+ *
+ * Description:
+ * Return initial mstatus when a task is created.
+ *
+ ****************************************************************************/
+
+uintptr_t riscv_get_newintctx(void)
+{
+ /* Set machine previous privilege mode to machine mode. Reegardless of
+ * how NuttX is configured and of what kind of thread is being started.
+ * That is because all threads, even user-mode threads will start in
+ * kernel trampoline at nxtask_start() or pthread_start().
+ * The thread's privileges will be dropped before transitioning to
+ * user code. Also set machine previous interrupt enable.
+ *
+ * Clear MIE bit since it will be handled by up_irq_enable/up_irq_disable.
Review comment:
And add
```
mask the bits which should be preserved (from ISA spec)
```
--
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] jlaitine commented on pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072253271
Tested this on mpfs and now it is working fine, 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 pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072114322
--
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 #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#discussion_r830386543
##########
File path: arch/risc-v/include/csr.h
##########
@@ -307,6 +307,14 @@
#define MSTATUS_FS_CLEAN (0x2 << 13)
#define MSTATUS_FS_DIRTY (0x3 << 13)
+/* Mask of preserved bits for mstatus */
+
+#ifdef CONFIG_ARCH_RV32
+#define MSTATUS_WPRI (0xff << 23 | 0x15)
+#else
+#define MSTATUS_WPRI (0x1fffffful << 38 | 0x1fful << 23 | 0x15ul)
Review comment:
```suggestion
#define MSTATUS_WPRI (0x1ffffffllu << 38 | 0x1ffllu << 23 | 0x15llu)
```
or we can go with
```suggestion
#define MSTATUS_WPRI (UINT64_C(0x1ffffff) << 38 | UINT64_C(0x1ff) << 23 | 0x15)
```
--
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] masayuki2009 edited a comment on pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
masayuki2009 edited a comment on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072066519
@no1wudi
Please fix the following errors.
```
====================================================================================
Configuration/Tool: rv32m1-vega/nsh-itcm,CONFIG_RISCV_TOOLCHAIN_GNU_RVGL
------------------------------------------------------------------------------------
Cleaning...
Configuring...
Disabling CONFIG_RISCV_TOOLCHAIN_GNU_RVGL
Enabling CONFIG_RISCV_TOOLCHAIN_GNU_RVGL
Building NuttX...
riscv64-unknown-elf-ld: /github/workspace/sources/nuttx/staging/libarch.a(riscv_initialstate.o): in function `up_initial_state':
/github/workspace/sources/nuttx/arch/risc-v/src/common/riscv_initialstate.c:87: undefined reference to `riscv_get_newintctx'
make[1]: *** [Makefile:145: nuttx] Error 1
make: *** [tools/Unix.mk:509: nuttx] Error 2
```
--
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] no1wudi commented on a change in pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
no1wudi commented on a change in pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#discussion_r829853003
##########
File path: arch/risc-v/src/common/riscv_getnewintctx.c
##########
@@ -0,0 +1,71 @@
+/****************************************************************************
+ * arch/risc-v/src/common/riscv_getnewintctx.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <stdint.h>
+#include <stdio.h>
+#include <assert.h>
+#include <debug.h>
+
+#include <nuttx/arch.h>
+#include <arch/irq.h>
+#include <arch/csr.h>
+
+#include "riscv_internal.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: riscv_get_newintctx
+ *
+ * Description:
+ * Return initial mstatus when a task is created.
+ *
+ ****************************************************************************/
+
+uintptr_t riscv_get_newintctx(void)
+{
+ /* Set machine previous privilege mode to machine mode. Reegardless of
+ * how NuttX is configured and of what kind of thread is being started.
+ * That is because all threads, even user-mode threads will start in
+ * kernel trampoline at nxtask_start() or pthread_start().
+ * The thread's privileges will be dropped before transitioning to
+ * user code. Also set machine previous interrupt enable.
+ *
+ * Clear MIE bit since it will be handled by up_irq_enable/up_irq_disable.
Review comment:
Updated, 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] masayuki2009 commented on pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072066519
--
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 merged pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775
--
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 a change in pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#discussion_r829918043
##########
File path: arch/risc-v/include/csr.h
##########
@@ -307,6 +307,14 @@
#define MSTATUS_FS_CLEAN (0x2 << 13)
#define MSTATUS_FS_DIRTY (0x3 << 13)
+/* Mask of preserved bits for mstatus */
+
+#ifdef CONFIG_ARCH_RV32
+#define MSTATUS_WPRI (0xff << 23 | 0x15)
+#else
+#define MSTATUS_WPRI (0x1ffffffUL << 38 | 0x1ffUL << 23 | 0x15UL)
Review comment:
change all UL to ul
--
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] no1wudi commented on pull request #5775: arch/risc-v: Merge riscv_getnewintctx into common
Posted by GitBox <gi...@apache.org>.
no1wudi commented on pull request #5775:
URL: https://github.com/apache/incubator-nuttx/pull/5775#issuecomment-1072083359
> In my understanding, we can not use csrc/csrs in exception_common to save/restore the mstatus.
Only few bits like `MSTATUS_FS_INIT / MSTATUS_MPPM / MSTATUS_MPIE` will be used by user tasks, it might be a potential issue that a bit were changed by bit operation for some reason, but it would be override by task's mstatus by context switch.
--
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