Skip to content

arch/riscv: add cluster local hartid#12881

Merged
xiaoxiang781216 merged 1 commit into
apache:masterfrom
yf13:qemu
Aug 11, 2024
Merged

arch/riscv: add cluster local hartid#12881
xiaoxiang781216 merged 1 commit into
apache:masterfrom
yf13:qemu

Conversation

@yf13
Copy link
Copy Markdown
Contributor

@yf13 yf13 commented Aug 11, 2024

Summary

arch/riscv: add cluster local hartid

Some RV chips (mpfs, jh7110, qemu-rv etc) have hart clusters and globally numbered mhartids. Clusters with single hart or SMP support can be managed by one NuttX instance. Currently NuttX expects to use cluster-local ids.

This allows us to get local ids by offsetting mhartids with provisioned base hartid for each cluster.

Note that there are chips (e.g. k230) that use cluster-local ids directly, so this is not needed for them.

Impacts

None

Testing

CI checks

@yf13
Copy link
Copy Markdown
Contributor Author

yf13 commented Aug 11, 2024

Got CI error:

Error response from daemon: Head "https://ghcr.io/v2/apache/nuttx/apache-nuttx-ci-linux/manifests/latest": Get "https://ghcr.io/token?account=yf13&scope=repository%3Aapache%2Fnuttx%2Fapache-nuttx-ci-linux%3Apull&service=ghcr.io": context deadline exceeded

Trigger re-check once.

Comment thread arch/risc-v/src/common/riscv_macros.S Outdated
Some multicore RV chips (mpfs, jh7110 etc) have hart clusters
and globally numbered mhartids. Clusters with single hart or
SMP support can be managed by one NuttX instance. Currently
NuttX expects to use cluster-local ids.

This allows us to get local ids by offsetting mhartids with a
base value.

Note that there are chips (e.g. k230) that use cluster-local
ids directly, so this is not needed for them.

Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
@xiaoxiang781216 xiaoxiang781216 merged commit 47b0414 into apache:master Aug 11, 2024
@pussuw
Copy link
Copy Markdown
Contributor

pussuw commented Oct 11, 2024

Hello @yf13

I have a question about this patch: Is the intent of using ARCH_RV_HARTID_BASE in the riscv_mhartid macro to convert physical CPU id to logical CPU id (for SMP mode) ?

If so, a problem arises when the physical hart ID is required, e.g. in interrupt acknowledgments etc. This occurs in platforms where harts are numbered like in MPFS (like you mentioned), from 0...4, where 0 is the monitor core and 1...3 are the application harts. Hart 0 is not usable for SMP and harts 1...3 are.

@yf13
Copy link
Copy Markdown
Contributor Author

yf13 commented Oct 11, 2024

@pussuw, yes that base is used to convert physical mhartid to logical id required by NuttX. It is useful if one NuttX instance only manages a single cluster of harts.

If global ID is needed, then simply ignore it as the default base 0 has no effects.

@pussuw
Copy link
Copy Markdown
Contributor

pussuw commented Oct 11, 2024

Ok, I see the point of ARCH_RV_HARTID_BASE but IMO riscv_mhartid is the wrong place for it, since based on the macro's name I would expect it to return the physical CPU (i.e. Hart) index.

@pussuw
Copy link
Copy Markdown
Contributor

pussuw commented Oct 11, 2024

After this PR, riscv_cpuindex.c / up_cpu_index is now definitely wrong #13886

The physical core index is expected, but logical index is returned. We have missing functionality; translation between physical and logical CPU index.

@yf13
Copy link
Copy Markdown
Contributor Author

yf13 commented Oct 11, 2024

@pussuw, as I have basically no browser access to Github.com, thus I am unaware of that PR until seeing your last comment. I agree with you that if we need physical id, another API shall be introduced.

When running AMP NuttX instances with QEMU, I didn't use such physical hart id API but used QEMU_RPTUN_MASTER_IPI and QQEMU_RPTUN_REMOTE_IPIS to provision resources on the peer nodes.

@pussuw
Copy link
Copy Markdown
Contributor

pussuw commented Oct 11, 2024

I'll create a patch next week which implements your ARCH_RV_HARTID_BASE approach to this_cpu (which means the logical CPU index). I got this issue as I'm implementing SMP for MPFS and it has hart0 that doesn't work for SMP.

@yf13
Copy link
Copy Markdown
Contributor Author

yf13 commented Oct 11, 2024

I'll create a patch next week which implements your ARCH_RV_HARTID_BASE
approach to this_cpu (which means the logical CPU index)

That sounds great. We then have separate APIs for logical and physical CPU ids.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants