remoteproc_allocate_id not treating notifyid as 32-bits

[tammyleino]

notifyid is 32 bits in the resource table and must be uint32_t in code
Signed-off-by: Tammy Leino tammy_leino@mentor.com

[arnopo]

Please fix compilation warnings:
https://github.com/OpenAMP/open-amp/runs/7505888819?check_suite_focus=true#step:4:1128

[tammyleino]

@arnopo I am unable to find any warnings related to the code change. Can you kindly show me where the error is being reported? - Nevermind, I downloaded the logs and grepped - I see now. Thanks!

[tammyleino]

@arnopo Resolving the warnings will require changing data types in the functions in libmetal's utilities.h.

I think we need to have a discussion about this, because changing this will have an effect on all OS's libmetal layers. Maybe we should rethink the way handle_vdev_rsc uses remoteproc_allocate_id instead of making such drastic changes to libmetal bitmaps.

[arnopo]

@arnopo Resolving the warnings will require changing data types in the functions in libmetal's utilities.h.

I think we need to have a discussion about this, because changing this will have an effect on all OS's libmetal layers. Maybe we should rethink the way handle_vdev_rsc uses remoteproc_allocate_id instead of making such drastic changes to libmetal bitmaps.

That's right! We could keep the bitmap as an unsigned long (4 bytes for 32-bit architecture, 8 bytes for 64-bit architecture). The important point is to make sure that the allocated ID does not exceed 32 bits.
I guess changing the end = METAL_BITS_PER_ULONG; could be enough to prevent overflow.

[tammyleino]

@arnopo So do you recommend handle_vdev_rsc handle setting the input parameter end to METAL_BITS_PER_ULONG if it is RSC_NOTIFY_ID_ANY instead of remoteproc_allocate_id doing it?

[arnopo]

@arnopo So do you recommend handle_vdev_rsc handle setting the input parameter end to METAL_BITS_PER_ULONG if it is RSC_NOTIFY_ID_ANY instead of remoteproc_allocate_id doing it?

my proposal is to keep rproc->bitmap as an unsigned long and then set the end value to

- 		end = METAL_BITS_PER_ULONG;
+		end = CHAR_BIT * sizeof(uint32_t);

With that we should ensure that the notifyid is in the expected range

[tammyleino]

@arnopo I was thinking of changing the way handle_vdev_rsc uses remoteproc_allocate_id instead; otherwise, we would end up passing end = 0xffffffff + 1 into remoteproc_allocate_id in the case of RSC_NOTIFY_ID_ANY in the resource table on 64-bit platforms.

I suggest instead of passing notifyid + 1 in the case of RSC_NOTIFY_ID_ANY, to pass 0 or pass RSC_NOTIFY_ID_ANY and change remoteproc_allocate_id to check for end == RSC_NOTIFY_ID_ANY instead of end == 0.

[arnopo]

@arnopo I was thinking of changing the way handle_vdev_rsc uses remoteproc_allocate_id instead; otherwise, we would end up passing end = 0xffffffff + 1 into remoteproc_allocate_id in the case of RSC_NOTIFY_ID_ANY in the resource table on 64-bit platforms.

In the resource table, the notifyid is in uint32_t format whatever the processor architecture.
RSC_NOTIFY_ID_ANY should probably also be defined as an integer not as a long to avoid issue in 64-bit architecture.

- #define RSC_NOTIFY_ID_ANY 0xFFFFFFFFUL
+ #define RSC_NOTIFY_ID_ANY 0xFFFFFFFFU

I suggest instead of passing notifyid + 1 in the case of RSC_NOTIFY_ID_ANY, to pass 0 or pass RSC_NOTIFY_ID_ANY and change remoteproc_allocate_id to check for end == RSC_NOTIFY_ID_ANY instead of end == 0.

Passing RSC_NOTIFY_ID_ANY for both start and end seems a good Idea

[tammyleino]

@arnopo It seems github has reached some limit with Azure so my Zephyr build has failed, but can you please review the updated fix and let me know your thoughts.

[arnopo]

The code LGTM.
The commit message contain details to explain the reason of the commit. The main reason is that this fix a potential overflow of the notify_id in case of 64-bit architecture. This information is missing

[arnopo]

@arnopo It seems github has reached some limit with Azure so my Zephyr build has failed, but can you please review the updated fix and let me know your thoughts.

Yes this is the second time I see the error, re-running the github action solves the issue.

[tammyleino]

@arnopo Thank you for the input - please review updated commit message at your convenience.

[edmooring]

Looks good to go.