-
Notifications
You must be signed in to change notification settings - Fork 10
RPMB and GPT Listeners - Merge Request #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| # Install the shared library | ||
| install(TARGETS gptservice | ||
| LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} | ||
| ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we installing ARCHIVE and RUNTIME when this is just a shared library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I installed these to compile and test as independent package (as an experiment). I have removed it.
| target_link_libraries(gptservice | ||
| PRIVATE minkadaptor | ||
| PRIVATE z | ||
| PRIVATE uuid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you attempted a standalone build of this package with your changes? Since libz and libuuid aren't available inside this repo, my suspicion is unless you explicitly try to find them first with find_package() they should give a build-time error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have compiled standalone build, and tested. however, the libraries are already compiled and haven't faced issues in compilation. Fixed it in latest change.
| ) | ||
|
|
||
| # Install header files | ||
| install(FILES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think a listener library is supposed to install and export headers. The only client for a listener is qtee_supplicant, which only calls init() and deinit() functions exported by the library. There is also no need for headers because the listener and qtee_supplicant are always separately built, with not build-time dependency on each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you pushing a prebuilt binary here? This is not allowed. Build all your test clients by adding their sources here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
| print_guid(req.guid); | ||
| printf("\n"); | ||
|
|
||
| /* Simulate response (in real implementation, this would come from TEE) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the proper way to ensure testing for this listener library. This is just a test for the sake of adding a test, I cannot allow it. Please create a Trusted Application which makes a call into the GPT listener and push the binary for that Trusted Application in this repository, so that developers with access to a QC board can actually validate that the GPT listener in this repository is being properly invoked by QTEE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed, it was tested internally. It is not required now.
|
|
||
| target_compile_definitions(gptservice | ||
| PRIVATE | ||
| -DGPT_LISTENER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you are using this flag in the GPT listener, it is unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| ) | ||
|
|
||
| # GPT Test Client (optional) | ||
| add_executable(gpt_test_client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't add a new test client for GPT, add your test case to smcinvoke_client itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
listeners/libgptservice/README.md
Outdated
| ### Dependencies | ||
| - zlib (for CRC32 calculations) | ||
| - uuid (for GUID operations) | ||
| - Linux block device support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of block device is expected here? Please provide more information. For example, for QCOMTEE we need the /dev/tee0 node, accessed by the QCOMTEE library. We have provided information that the TEE subsystem exposes this node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added block device details.
|
Please ensure that the name of the Pull Request is a concise title which highlights the individual commits in the Pull Request. Just saying "New listeners" makes it difficult to find the Pull Request later which added support for GPT and RPMB listener. |
bf4a3e1 to
02fdb20
Compare
harshaldev27
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need a separate commit for this. Please rebase your branch ontop of minkipc tip.
| @@ -0,0 +1,87 @@ | |||
| // Copyright (c) 2024 Qualcomm Technologies, Inc. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright update please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
| @@ -0,0 +1,68 @@ | |||
| // Copyright (c) 2024 Qualcomm Technologies, Inc. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright update required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, will remove copyright.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remove, update it to yearless as in gpt_msg.h
| #define MSGE(fmt, ...) GPT_LOG_ERROR(fmt, ##__VA_ARGS__) | ||
| #define MSGD(fmt, ...) GPT_LOG_DEBUG(fmt, ##__VA_ARGS__) | ||
|
|
||
| /* Fixed. Don't increase the size of TZ_CM_MAX_NAME_LEN */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. I see many listeners are using these macros, maybe we should think about moving them to a separate common file eventually. For now it is good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, logging format can be same and we can modify/use for all listeners.
harshaldev27
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the right way @quic-bhaskarv . Please take your original 2 commits for RPMB and GPT. Rebase your fork onto tip of minkipc repo and reapply those two commits. Then force push to your minkipc fork. Creating new 'rebase' and 'merge' commits in this PR is not the way forward.
5c2694b to
da25817
Compare
harshaldev27
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to understand how are you testing this listener? Are you using some TA? Could you provide steps you are using as well as a small log snippet of success case?
|
|
||
| /* GPT constants and definitions from original service */ | ||
| #define GPT_LSTNR_VERSION_1 1 | ||
| #define GPT_LSTNR_VERSION_2 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two macros seem a bit redundant, why not just #define GPT_LSTNR_VERSION 2 on line 46?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to support 2 version, please check the code in smci_gpt_handle_init_req for details.
| } gpt_init_info_t; | ||
|
|
||
| struct gpt_stats { | ||
| int init_done; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid adding unnecessary spaces between member type and name. Just int init_done is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for below as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is for readability purpose, removed the tabs.
| @@ -0,0 +1,68 @@ | |||
| // Copyright (c) 2024 Qualcomm Technologies, Inc. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remove, update it to yearless as in gpt_msg.h
| } __attribute__ ((packed)); | ||
|
|
||
| struct disk { | ||
| char dev_name[FNAME_SZ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid unnecessary spaces in struct members.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| if (dmaBufLen >= sizeof(tz_gpt_rw_res_t)) { | ||
| smci_gpt_handle_rw_req(req, rsp); | ||
| } else { | ||
| MSGE("Invalid input."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's print something more concrete like, "Response buffer length is too small!" to that we know whether this is a transport issue or a use-case issue.
|
|
||
| MSGD("GPT_SERVICE Dispatch ends! "); | ||
|
|
||
| free(req); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an unnecessary allocation let's try to avoid it unless you have a very valid reason.
| init_resp.ctx_id = -1; | ||
| init_resp.status = GPT_LSTNR_VER_NOT_SUPPORTED; | ||
|
|
||
| memmove(rsp, (void*)&init_resp, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this doesn't require wrap around.
| /* | ||
| * Main dispatch function for GPT service - matches original exactly | ||
| */ | ||
| int smci_dispatch(void *dmabuff, size_t dmaBufLen) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should avoid mixing snake case and camelCase variable names. I prefer you use snake_case dma_buf_len since the rest of this file does that.
| } | ||
| } | ||
|
|
||
| static void smci_gpt_handle_rw_req(void *req, void *rsp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we benefit from providing a brief comment explaining these functions? Other listeners are doing it.
listeners/librpmbservice/rpmb.c
Outdated
|
|
||
| static struct rpmb_wake_lock | ||
| { | ||
| int lock_fd; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid unnecessary space between datatype and variable name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
This change introduces GPT listener service to support secure GPT requests from QTEE. Change-Id: I6d6b5f5c2701735253e72d6284ce41234142ddbb Signed-off-by: Bhaskar Valaboju <quic_bhaskarv@quicinc.com>
This change introduces RPMB listener service to support secure RPMB requests from QTEE. Change-Id: Ic46489cc366956a946b7e02512cd8f5432af0133 Signed-off-by: Bhaskar Valaboju <quic_bhaskarv@quicinc.com>
7691f18 to
667875c
Compare
This change implements RPMB and GPT listeners, please review.