Skip to content
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

SMC Capability #24

Merged
merged 1 commit into from
Sep 5, 2023
Merged

SMC Capability #24

merged 1 commit into from
Sep 5, 2023

Conversation

Furao
Copy link
Contributor

@Furao Furao commented Dec 10, 2021

Add a new capability which certain threads can invoke so seL4 can make SMC calls on ARM platforms in EL2 (virtualized mode) on the thread’s behalf.

See https://sel4.atlassian.net/browse/RFC-9

@Furao
Copy link
Contributor Author

Furao commented Jan 18, 2023

From the style checker:

 #ifdef CONFIG_ALLOW_SMC_CALLS
     /* Function pointer to SMC capability list */
-    seL4_CPtr (*vm_get_smc_cap)(seL4_Word smc_call);
+    seL4_CPtr(*vm_get_smc_cap)(seL4_Word smc_call);
 #endif

I could change this to appease CI, but since it is a function pointer, I am not sure why it is telling me to make that change. I see plenty of other function pointers in this format throughout the codebase.

@lsf37
Copy link
Member

lsf37 commented Jan 18, 2023

I could change this to appease CI, but since it is a function pointer, I am not sure why it is telling me to make that change. I see plenty of other function pointers in this format throughout the codebase.

For what it's worth I like your version better, but there is little choice but to follow the style checker, because it will otherwise fail forever. It might well be that it's a bug in the style checker where it treats built-in types differently to declared types in this instance or something like that.

Grepping through the code base, it does seem that the style checker version is used almost everywhere where seL4_CPtr is returned. I could find only one instance that mentions a function pointer and seL4_CPtr with space: seL4_CPtr (*getIRQHandlerEndpoint)(void *cookie, int irq);, which is in refos and has not been under style checker control. All other 11 instances are in the form without space.

So I think we should just change it and comply to the style checkers wishes even if it's a bit strange.

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.

Looks good from my side, but someone with more experience on the library side should have another look. @kent-mcleod or @Indanz maybe?

Copy link

@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 don't have experience using libsel4vm, but it needs to be more generic to be widely useful I think. In its current form it only solves one specific Zynq problem.

libsel4vm/include/sel4vm/guest_vm.h Outdated Show resolved Hide resolved
libsel4vmmplatsupport/src/arch/arm/smc.c Outdated Show resolved Hide resolved
libsel4vmmplatsupport/src/sel4_arch/aarch64/smc.c Outdated Show resolved Hide resolved
libsel4vmmplatsupport/src/sel4_arch/aarch64/smc.c Outdated Show resolved Hide resolved
libsel4vmmplatsupport/src/sel4_arch/aarch64/smc.c Outdated Show resolved Hide resolved
Copy link

@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'm not sure what the split in code should be between libsel4vm and libsel4vmmplatsupport. It seems fine to me as it is.

libsel4vm/arch_include/arm/sel4vm/arch/guest_vm_arch.h Outdated Show resolved Hide resolved
libsel4vm/arch_include/arm/sel4vm/arch/guest_vm_arch.h Outdated Show resolved Hide resolved
libsel4vmmplatsupport/src/arch/arm/smc.c Outdated Show resolved Hide resolved
Copy link

@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.

Looks good enough to be merged now.

Comment on lines +220 to +223
if (!vm_smc_handler) {
ZF_LOGE("Failed to register smc_handler callback: Invalid callback");
return -1;
}
Copy link

Choose a reason for hiding this comment

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

An alternative would be to set it back to the default instead. The people can call this with NULL to unregister their callback.

This required a new callback to be added to the vm_arch struct for
ARM. The old handle_smc function gets registered as the smc handler by
default. The callback allows a user to register a custom smc handler
instead.

This is especially useful to allow SMC forwarding in the user's custom
handler.

Signed-off-by: Robbie VanVossen <[email protected]>
@Furao
Copy link
Contributor Author

Furao commented Sep 5, 2023

@kent-mcleod Can you merge this?

@Indanz Indanz merged commit eb42051 into seL4:master Sep 5, 2023
15 checks passed
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.

3 participants