scripts: Dynamically add driver subsystems to subsystems list

[xodus7]

This change extends the parse_syscalls.py script to scan for a
__subsystem sentinal added to driver api declarations. It thens
generates a list that is passed into gen_kobject_list.py to extend
the subsystems list. This allows subsystems to be declared in the
code instead of a separate python list and provides a mechanism for
defining out-of-tree subsystems.

Signed-off-by: Corey Wharton coreyw7@fb.com

[SebastianBoe]

Hi, can you please explain what the "subsystem list" is. Does it exist already, or is it introduced in this PR?

If it is introduced in this PR please, in detail, explain what it is for.

Also, AFAIK it is possible to define out-of-tree subystems already, if it is not, then please elaborate
on what is preventing us from doing so today.

[xodus7]

@SebastianBoe Certainly, the list exists already inside the gen_kobject_list.py script and is currently defined as:

subsystems = [
    "adc_driver_api",
    "aio_cmp_driver_api",
    "counter_driver_api",
    "crypto_driver_api",
    "dma_driver_api",
    "flash_driver_api",
    "gpio_driver_api",
    "i2c_driver_api",
    "i2s_driver_api",
    "ipm_driver_api",
    "led_driver_api",
    "pinmux_driver_api",
    "pwm_driver_api",
    "entropy_driver_api",
    "sensor_driver_api",
    "spi_driver_api",
    "uart_driver_api",
    "can_driver_api",
    "ptp_clock_driver_api",
    "eeprom_driver_api",
    "wdt_driver_api",

    # Fake 'sample driver' subsystem, used by tests/samples
    "sample_driver_api"
]

While it is possible to define custom drivers and interfaces, these don't get the special handling done in this script (see comments in file header) unless the driver api struct name is added to this list. This change allows that through the __subsystem sentinel value, and if accepted could replace the hard-coded list completely. I held off from changing the current list until I've received feedback on this PR.

[andrewboie]

I like this! I think we should just go ahead and tag all the existing subsystems and remove the hard-coded subsystems list to make everything clean and consistent.

I defer the build system stuff to @SebastianBoe opinion.

[andrewboie]

Hi, can you please explain what the "subsystem list" is. Does it exist already, or is it introduced in this PR?

@SebastianBoe this is for memory protection. All driver APIs take a struct device parameter. But we need to enforce that APIs for driver subsystem X only accept device pointers of subsystem X, this mechanism is to enforce that by checking the type of API struct that device->api points to (at build time).

[SebastianBoe]

I was not aware of this hardcoded list. We do indeed need to support out-of-tree subsystem
declarations.

But we should populate this list through CMake instead of by parsing C code because this is significantly faster, simpler, and less prone to obscure issues (e.g. subsystem declarations using preprocessor constructs).

We don't do this for syscalls because there are hundreds of syscalls, but for a few dozen subsystems this is the better approach.

The way it would work is that we would in CMake populate a list that is stored in a target property. Storing it in a target property means that we can read it out at generation-time, and therefore not have a read-after-write dependency on the code declaring the gen_kobject_list.py command, and the code populating the list.

See 6538c28 for a demonstration on how it can be done.

[andrewboie]

No no no I don't agree at all, I understand doing this in CMake is simpler from a build system perspective, but I think this is far less intuitive to the end user, and splits information between the C code and the build system files.

It is dead simple, from an end user perspective, if they declare a subsystem api, to tag the struct declaration with __subsystem. That is all they need to do, and it is co-located with the API struct declaration. No dual maintenance between separate files, which was the whole point of this patch.

See 6538c28 for a demonstration on how it can be done.

No offense, but yuck. You're just moving the list of subsystems out of the python code and into some CMakelists.txt. What's the point? Might as well leave it in the Python.

We don't do this for syscalls because there are hundreds of syscalls

This has nothing to do with the number of system calls. We need to know the type information for the system call. Moving this to CMake would require that we effectively duplicate the function prototype, and have to update both if it changes. And from an end user perspective, its simple and intuitive to just tag an API that needs to be a system call with __syscall and be done with it. It's co-located with the code. It's simple.

[andrewboie]

@SebastianBoe would an acceptable compromise be to simply have the same script that does the __syscall scanning also look for __subsystem? I think this would eliminate most of the build system changes.

[SebastianBoe]

would an acceptable compromise be to simply have the same script that does the __syscall scanning also look for __subsystem?

That is how it is implemented, no?

[SebastianBoe]

I disagree that it is simpler to regex parse all the C code in the codebase than to maintain a list in CMake.

Note that the point of this PR was to support out-of-tree declarations of subsystems.

There are a myriad of limitations, e.g. the __subsystem keyword must be put at a very specific place in the declaration, and it can not be expanded through a macro.

[xodus7]

@SebastianBoe I acknowledge your proposed solution is simpler in both the size of the change and complexity, and it's not that much different from my first approach. However, I do agree with @andrewboie that using the __subsystem sentinel is clearer from a code perspective, self-documenting, and more end-user friendly. I also think the potential issues you identified, while valid, they are mostly theoretical and unlikely to come up often (ever?). Finally, syscalls and subsystems are intimately related and I think utilizing the same approach for both makes sense.

Ultimately, my team has a need for defining new subsystems without maintaining a separate Zephyr tree, and I'm willing to implement whatever solution the greater community thinks is the best approach.

[andrewboie]

I disagree that it is simpler to regex parse all the C code in the codebase than to maintain a list in CMake.

We're fundamentally not looking at this the same way.

This is simpler for the end user developer experience, which is what I care about, to a very intense degree.
This does make the guts of the build system more complicated, sure. But to be honest, few people other than you care about that. We get it right and it's done. The guts of the build system is already (necessarily) a "here there be dragons" situation.
Doing this in CMake is a NACK from me, sorry.

[SebastianBoe]

I disagree that this is simpler for the end user developer experience. The user
needs to understand the caveats.

When considering what is the simplest for the user I like to look at how much documentation is necessary to read.

Subsystems are appended through CMake to the 'SUBSYSTEMS' property of
the 'zephyr_property_target' target

versus

subsystems are introduced by adding the __subsystem keyword to driver
API struct declarations. The '__subsystem' keyword, the 'struct'
keyword, and the variable name for the struct must appear in order
without any other tokens (such as attributes) interleaved between
them or before them. The preprocessor may not expand any macros for these first three
tokens. The struct must be declared in one of the directories listed
in the CMake variable SYSCALL_INCLUDE_DIRS. This list always
contains ${ZEPHYR_BASE}/include with subdirectories, but will also contain
APPLICATION_SOURCE_DIR when CONFIG_APPLICATION_DEFINED_SYSCALL
is set. Additional paths can be added to the list through the CMake
command line or in CMake code that is run before
${ZEPHYR_BASE}/cmake/app/boilerplate.cmake is run.

That being said, since you are the maintainer of this API, I will
defer to your opinion on what is the simplest for the user.

If we do regex parsing I would like to see some numbers for the
parsing overhead and if more than 10ms, and if it is possible to avoid
this parsing for certain Kconfig'urations (e.g. only applicable when
CONFIG_USERSPACE), then we should make it configurable.

[xodus7]

I wrote some benchmarking code in the parse_syscalls.py script:

Build command:

west build -p always -b stm32f4_disco samples/userspace/prod_consumer/

Build system:

Intel(R) Xeon(R) W-2135 CPU @ 3.70GHz
64GB RAM
Running Fedora 30

Results

processed 443 files
syscall regex min/avg/max/std = 0.000715/0.004955/0.111103/0.007806
subsys regex min/avg/max/std = 0.000477/0.003816/0.074387/0.006123

Times are in milliseconds so the average impact per file is roughly 4 microseconds and the total impact should be around 1.8ms total. Not bad for a technology first defined in the 50s!

The runtime for entire python script:

real	0m0.062s
user	0m0.053s
sys	0m0.008s

I'm guessing much of that is file I/O and python overhead.

[xodus7]

I pushed a commit removing the names in the hard-coded list as mentioned earlier. In the process of creating the change I've found the follow interesting issues. I may need a sanity check to make sure I'm not missing something:

• aio_cmp_driver_api is not found anywhere in the codebase. Is this obsolete?

• espi, kscan, ps2 are missing:

  • espi does not have z_vrfy handlers defined so I believe will fail at runtime in a system with CONFIG_USERSPACE enabled.
  • kscan has z_vrfy handlers but either does not verify device ownership in 2 cases or references Z_SYSCALL_DRIVER_KSCAN in one place which should fail to build.
  • ps2 uses the deprecated handler syntax and calls Z_SYSCALL_DRIVER_PS2 in several places which should fail to build.

[albertofloyd]

@xodus7 Note that the eSPI subsystem so far is only used on MEC15xx boards, which has no CONFIG_USERSPACE not enabled (No MPU). Nevertheless, I'm planning to add espi handlers for v2.3.

[SebastianBoe]

The regex parsing overhead is ok. Thank you for benchmarking.

[jukkar]

LGTM

[xodus7]

@andrewboie Is there anything else I need to do here before final approval?

[andrewboie]

Merge window needs to open up for 2.3 and then we can push this.

[andrewboie]

I pushed a commit removing the names in the hard-coded list as mentioned earlier. In the process of creating the change I've found the follow interesting issues. I may need a sanity check to make sure I'm not missing something:

• aio_cmp_driver_api is not found anywhere in the codebase. Is this obsolete?

• espi, kscan, ps2 are missing:

  • espi does not have z_vrfy handlers defined so I believe will fail at runtime in a system with CONFIG_USERSPACE enabled.
  • kscan has z_vrfy handlers but either does not verify device ownership in 2 cases or references Z_SYSCALL_DRIVER_KSCAN in one place which should fail to build.
  • ps2 uses the deprecated handler syntax and calls Z_SYSCALL_DRIVER_PS2 in several places which should fail to build.

It's probably the case that this stuff isn't being built by tests, we will need to address that.