Use new VIRTIO_{DRIVER,DEVICE}_SUPPORT compiler define to improve code readability

[glneo]

Currently compiler defines are defined when support for driver or device is the only support being built. This is a negative define, it surrounds the code to not be built and we use ifndef. This is confusing. It also leaves ifndefs all throughout the code-base. Instead, define a macro that is set to 1 if support is enabled. Use this inline in if statements where possible. Any sane compiler will optimize away the code in the branch when support is not enabled just the same as when using the preprocessor so we keep the same binary size.

[arnopo]

Hello @glneo

Your proposal seems making code more readable. Nevertheless I have few concerns

• can we ensure that all compilers will do the optimisation (gcc, armcc, ewarm,...). If not I would prefer to keep #ifdef
• VIRTIO_DEVICE_ONLY VIRTIO_DRIVER_ONLY are currently used in projects They have to be tagged as deprecated and supported 2 years ( as currently done for VIRTIO_MASTER_ONLY)
  • applying your patch i'm facing warning in zephyr

    west build -b stm32mp157c_dk2 samples/subsys/ipc/openamp_rsc_table
    [...]
    /local/home/frq07267/views/zephyrproject/modules/lib/open-amp/open-amp/lib/remoteproc/remoteproc_virtio.c:332:17: warning: implicit declaration of function 'rproc_virtio_negotiate_features'; did you mean 'virtio_negotiate_features'? [-Wimplicit-function-declaration]
    332 |                 rproc_virtio_negotiate_features(vdev, dfeatures);
    |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |                 virtio_negotiate_features
    [...]

[glneo]

Hi @arnopo,

1. All compilers I have found make this trivial dead code elimination, even on the lowest optimization level setting (-O0). If some compiler exists that cannot do that elimination then that compiler is probably generating extremely large and optimized code all over and this micro-optimization for size wasn't helping them much anyway.

2. I can somewhat understand keeping WITH_VIRTIO_MASTER around for a bit longer as it was a user facing setting, so it might have been used in some builder script. But VIRTIO_DRIVER_ONLY is an internal-only flag generated by the external CMake setting WITH_VIRTIO_DRIVER. It is not even included in a configuration header, it is simply injected into the compiler at build time. Nothing outside of internal source files could have made use of this flag.

Unless what you are saying is that you have changes in a downstream fork of this project that makes use of this flag. In which case I'd like to know the policy on that. For instance the Linux kernel is very clear that downstream forks need not be considered when making changes to internal code. Otherwise how could one make any change without risking breaking some fork that they have no visibility into?

3. Fixed.

[arnopo]

Hi @arnopo,

1. All compilers I have found make this trivial dead code elimination, even on the lowest optimization level setting (-O0). If some compiler exists that cannot do that elimination then that compiler is probably generating extremely large and optimized code all over and this micro-optimization for size wasn't helping them much anyway.

Having more optimized code for device mode is required for device role as coprocessor can be limited in term of memories. keeping #if have the advantage to guaranty that all compilers do the job, but also make the code more clear that the use of a define, that mask the optimization. So I would more in favour in keep #if

2. I can somewhat understand keeping WITH_VIRTIO_MASTER around for a bit longer as it was a user facing setting, so it might have been used in some builder script. But VIRTIO_DRIVER_ONLY is an internal-only flag generated by the external CMake setting WITH_VIRTIO_DRIVER. It is not even included in a configuration header, it is simply injected into the compiler at build time. Nothing outside of internal source files could have made use of this flag.

That true for project based on cmake but not for project that use the code without cmake pre-build step. This is the case for instance for some ST projects.

Unless what you are saying is that you have changes in a downstream fork of this project that makes use of this flag. In which case I'd like to know the policy on that. For instance the Linux kernel is very clear that downstream forks need not be considered when making changes to internal code. Otherwise how could one make any change without risking breaking some fork that they have no visibility into?

You can refer to https://www.openampproject.org/governance/ for the rule we expect to apply on OpenAMP. As mentioned above these preprocess defines have to be considered as API for some projects that are not cmake based.

In this PR following the deprecated rule seems quite simple, so I don't see a a reason to make an exception. You can take inspiration from VIRTIO_MASTER_ONLY to do it.

[glneo]

You can refer to https://www.openampproject.org/governance/ for the rule we expect to apply on OpenAMP. As mentioned above these preprocess defines have to be considered as API for some projects that are not cmake based.

What projects? This project is cmake based. It builds a library that can be used by other projects that may not be cmake based, but that doesn't change this project. Unless you are saying you are directly building/copy/pasting the source from this repo into the build of another project. The API must be defined as the function prototypes in include/openamp/open_amp.h. The code itself, nor the build system, can be considered API. If they were, then nothing here could ever be changed again, every line of code would have to be considered an API for some downstream project that might make some use of it.

In this PR following the deprecated rule seems quite simple, so I don't see a a reason to make an exception. You can take inspiration from VIRTIO_MASTER_ONLY to do it.

I'll make the change to keep this PR moving along, but as stated above, I do not agree with the reasoning. Might be worth chatting out what constitutes the "API" in the next meeting.

[glneo]

Having more optimized code for device mode is required for device role as coprocessor can be limited in term of memories. keeping #if have the advantage to guaranty that all compilers do the job, but also make the code more clear that the use of a define, that mask the optimization. So I would more in favour in keep #if

If the issue is that the define is not obvious or masks that the optimization is intended, we could use that IS_ENABLED() macro suggested above, then the lines become:

if (IS_ENABLED(VIRTIO_DRIVER_SUPPORT) && vdev->role == VIRTIO_DEV_DRIVER)

Which I think makes it very clear now that this is a preprocessor level check. It also matches what U-Boot is now doing to remove its #ifdef messiness.

[arnopo]

You can refer to https://www.openampproject.org/governance/ for the rule we expect to apply on OpenAMP. As mentioned above these preprocess defines have to be considered as API for some projects that are not cmake based.

What projects? This project is cmake based. It builds a library that can be used by other projects that may not be cmake based, but that doesn't change this project. Unless you are saying you are directly building/copy/pasting the source from this repo into the build of another project. The API must be defined as the function prototypes in include/openamp/open_amp.h. The code itself, nor the build system, can be considered API. If they were, then nothing here could ever be changed again, every line of code would have to be considered an API for some downstream project that might make some use of it.

CMake can be used to configure a project, but it does not necessarily impose building a library. Some project environment does not support cmake build project, in such case you have to include open-amp source code.

The objective here is to not break compatibility with some existing legacy projects, when possible. We don't know all the users. In such an environment, the work you are doing to rationalize the APIs is very important because it will help with maintainability. We just need to move forward step by step.

In this PR following the deprecated rule seems quite simple, so I don't see a a reason to make an exception. You can take inspiration from VIRTIO_MASTER_ONLY to do it.

I'll make the change to keep this PR moving along, but as stated above, I do not agree with the reasoning. Might be worth chatting out what constitutes the "API" in the next meeting.

That's a good Idea, let's put this on the agenda for next meeting.

[glneo]

Also, the long line checkpatch warnings will be fixed in the next PR.

[arnopo]

Also, the long line checkpatch warnings will be fixed in the next PR.

Please fix that is in this PR, checkpatch success is requested before we merge the PR

[glneo]

Also, the long line checkpatch warnings will be fixed in the next PR.

Please fix that is in this PR, checkpatch success is requested before we merge the PR

Fair enough, I've added one extra patch to this series which I was going to send as part of a different PR, but it fixes the long line warnings and hopefully is trivial enough.