Skip to content

Conversation

@midnightveil
Copy link
Contributor

@midnightveil midnightveil commented May 29, 2025

These were taken from the sel4test Sel4testHaveTimer config option.

Do not merge until #195 has been merged (as it will break sel4test for more platforms) ok now

@Indanz
Copy link
Contributor

Indanz commented May 29, 2025

Moving the knowledge from sel4test to util_libs makes a lot of sense. But please don't use negated terms, use something like LIB_PLAT_SUPPORT_HAVE_LTIMER (it's fine to still use the blacklist, as that makes it easier to add a new timer without yet another subtle failure case).

It would be nice if we could add a define to each timer.h driver file to set a HAVE_TIMER define instead of hard-coding it in CMake, but it looks like the code structure won't allow it. That would automatically be up-to-date with the code. Oh well.

midnightveil added a commit to midnightveil/seL4_util_libs that referenced this pull request May 30, 2025
As suggested by Indan to be clearer to understand.
seL4#196 (comment)

Signed-off-by: julia <git.ts@trainwit.ch>
midnightveil added a commit to midnightveil/seL4_util_libs that referenced this pull request May 30, 2025
As suggested by Indan to be clearer to understand.
seL4#196 (comment)

Signed-off-by: julia <git.ts@trainwit.ch>
@midnightveil midnightveil marked this pull request as draft May 30, 2025 02:30
midnightveil added a commit to midnightveil/sel4bench that referenced this pull request May 30, 2025
As suggested by Indan to be clearer to understand.
seL4/util_libs#196 (comment)

Signed-off-by: julia <git.ts@trainwit.ch>
midnightveil added a commit to midnightveil/sel4bench that referenced this pull request May 30, 2025
As suggested by Indan to be clearer to understand.
seL4/util_libs#196 (comment)

Signed-off-by: julia <git.ts@trainwit.ch>
midnightveil added a commit to midnightveil/seL4_util_libs that referenced this pull request May 30, 2025
As suggested by Indan to be clearer to understand.
seL4#196 (comment)

Signed-off-by: julia <git.ts@trainwit.ch>
midnightveil added a commit to midnightveil/seL4_util_libs that referenced this pull request May 30, 2025
As suggested by Indan to be clearer to understand.
seL4#196 (comment)

Signed-off-by: julia <git.ts@trainwit.ch>
midnightveil added a commit to midnightveil/seL4_util_libs that referenced this pull request May 30, 2025
As suggested by Indan to be clearer to understand.
seL4#196 (comment)

Signed-off-by: julia <git.ts@trainwit.ch>
midnightveil added a commit to midnightveil/seL4_util_libs that referenced this pull request May 30, 2025
As suggested by Indan to be clearer to understand.
seL4#196 (comment)

Signed-off-by: julia <git.ts@trainwit.ch>
Indanz pushed a commit that referenced this pull request May 30, 2025
As suggested by Indan to be clearer to understand.
#196 (comment)

Signed-off-by: julia <git.ts@trainwit.ch>
Indanz pushed a commit to seL4/seL4_libs that referenced this pull request May 30, 2025
As suggested by Indan to be clearer to understand.
seL4/util_libs#196 (comment)

Signed-off-by: julia <git.ts@trainwit.ch>
Indanz pushed a commit to midnightveil/seL4_util_libs that referenced this pull request May 30, 2025
As suggested by Indan to be clearer to understand.
seL4#196 (comment)

Signed-off-by: julia <git.ts@trainwit.ch>
Indanz pushed a commit to midnightveil/seL4_util_libs that referenced this pull request May 30, 2025
As suggested by Indan to be clearer to understand.
seL4#196 (comment)

Signed-off-by: julia <git.ts@trainwit.ch>
Indanz pushed a commit to seL4/sel4bench that referenced this pull request May 30, 2025
As suggested by Indan to be clearer to understand.
seL4/util_libs#196 (comment)

Signed-off-by: julia <git.ts@trainwit.ch>
@Indanz Indanz marked this pull request as ready for review May 30, 2025 21:51
@Indanz Indanz force-pushed the sel4test-ltimer-consistency branch from 39360b5 to d85d4f1 Compare May 30, 2025 21:54
Clang didn't have `__attribute__((error(msg)))` until version 14,
but we support older versions of Clang. Both GCC and clang support
`__has_attribute(error)`; GCC since v5 (2015) and Clang seemingly
always.

This fixes builds that were failing due to 'unknown attribute error'.
We are not relying on the presence of this attribute for correct
output, but purely as a debug message which fails before the linker.
If we don't support the message failing at link-time is OK too.

Signed-off-by: julia <git.ts@trainwit.ch>
These were taken from the sel4test Sel4testHaveTimer config option.

Signed-off-by: julia <git.ts@trainwit.ch>
@midnightveil midnightveil force-pushed the sel4test-ltimer-consistency branch from d85d4f1 to 23cd7ba Compare May 31, 2025 02:25
@Indanz
Copy link
Contributor

Indanz commented May 31, 2025

To be fully portable you would need an if defined(__has_attribute), except if you rely on C23. At this point I'd get rid of the error thing altogether and just fall back to linker errors personally, but oh well, bit late for that.

@Indanz Indanz merged commit f8ac370 into seL4:master May 31, 2025
24 checks passed
OR KernelPlatformRocketchip
OR KernelPlatformRocketchipZCU102
OR KernelPlatformCheshire
OR (SIMULATION AND (KernelArchRiscV OR KernelArchARM))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@midnightveil for some reason this line breaks the camkes timer component build. For all Arm platforms we're getting:

  /github/workspace/projects/global-components/components/TimeServer/src/time_server.c: In function ‘post_init’:
  /github/workspace/projects/global-components/components/TimeServer/src/time_server.c:230:13: error: call to ‘ltimer_default_init’ declared with attribute error: no ltimer support for this platform
    230 |     error = ltimer_default_init(&ltimer, io_ops, time_server_ltimer_handle, NULL);
        |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

(e.g. https://github.com/seL4/camkes-tool/actions/runs/15923333523/job/45177979627)

If I remove this line, everything builds and works fine. Either SIMULATION does not mean what it says or it is on for those builds even though they are hardware images. It does seem to work if I explicitly provide -DSIMULATION=OFF on the command line.

Are we actually sure that it is true for all Arm platforms that qemu does not provide an ltimer device? I thought that some of them have fairly extensive device simulation (but maybe not timer, I have not checked).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SIMULATE does just seem to be on for those builds. It is actually generating a simulate script (which then says the platform is not supported for simulation).

So the right fix seems to be to make sure SIMULATE is not on for those builds, but it makes me wonder if we're braking other builds without intending to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so it does intentionally switch on SIMULATION by default in easy-settings.cmake

set(SIMULATION ON CACHE BOOL "Try and use simulable features")

So I'm not entirely sure how to proceed. Do we actually need to guard SIMULATION here in util_libs, or is it just to fail early?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was copied from sel4test: https://github.com/seL4/sel4test/pull/142/files

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What the issue is probably that it was previously an

if (qemu arm virt)
    if (timers) enable
    else disable
else (check this bunch of other conditions)
    disable

Now it is

if (qemu arm virt OR these other conditions)
   disable

Copy link
Contributor Author

@midnightveil midnightveil Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
OR (SIMULATION AND (KernelArchRiscV OR KernelArchARM))
OR ((NOT KernelPlatformQEMUArmVirt) AND SIMULATION AND (KernelArchRiscV OR KernelArchARM))

I think would be equivalent?

EDIT: Actually, no, it's still different. I'm going to go back to the original double if I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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