Skip to content

Conversation

@midnightveil
Copy link
Contributor

The if statements were not strictly equivalent and broke the camkes timer component builds:

https://github.com/seL4/util_libs/pull/196/files#r2178857087

Ref: seL4/sel4test#142

The if statements were not strictly equivalent and broke the
camkes timer component builds:

https://github.com/seL4/util_libs/pull/196/files#r2178857087

Ref: seL4/sel4test#142

Signed-off-by: julia <git.ts@trainwit.ch>
@midnightveil midnightveil force-pushed the sel4test-ltimer-consistency branch from 1161aad to 973c62c Compare July 2, 2025 03:06
Copy link
Member

@lsf37 lsf37 left a comment

Choose a reason for hiding this comment

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

It looks to me like this change is semantically equivalent to what was there before.

If SIMULATION is set, it'll still set LibPlatSupportHaveTimer to OFF, and the CAmkES build does have SIMULATION set by default, i.e. we will see it also for platforms like tx2, imx8mm, etc.

I'm tending towards changing the camkes default setting, tbh. It doesn't really make sense to me to have simulation settings on for hardware images.

It is possible that we had a policy that setting simulation to "on" shouldn't break things on hardware, but I don't remember anything in that direction. Maybe @kent-mcleod remembers?

Copy link
Contributor

@Indanz Indanz left a comment

Choose a reason for hiding this comment

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

I think it would be better to remove the SIMULATION part of the check than shuffling this bit around like this.

if(
(KernelPlatformQEMUArmVirt AND NOT (KernelArmExportPCNTUser AND KernelArmExportPTMRUser))
OR KernelPlatformRocketchip
if(KernelPlatformQEMUArmVirt)
Copy link
Member

@axel-h axel-h Jul 2, 2025

Choose a reason for hiding this comment

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

As a general question, why are we explicitly checking for KernelPlatformQEMUArmVirt here actually, can't this be made KernelArchARM to make this generic?

Below in line 124 there is this, which make we wonder why we don't check KernelPlatformQuartz64/KernelPlatformIMX93 here then also

if(KernelPlatformQEMUArmVirt OR KernelPlatformQuartz64 OR KernelPlatformIMX93)
    if(KernelArmExportPCNTUser AND KernelArmExportPTMRUser)
        list(APPEND deps src/arch/arm/generic_ltimer.c)
    endif()
endif()

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.

4 participants