shim: implement KVM_GET_MSR_INDEX_LIST #54
Conversation
Codecov Report
@@ Coverage Diff @@
## master #54 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 210 210
Lines 6242 6397 +155
Branches 113 123 +10
==========================================
+ Hits 6242 6397 +155
|
enr0n
force-pushed
the
get-msr-index-list
branch
2 times, most recently
from
6a79543
to
bd120da
Compare
shim/linux/src/entry.c
Outdated
| mut_ret = -EINVAL; | ||
| if (platform_copy_from_user(&mut_args, user_args, sizeof(mut_args))) { | ||
| bferror("platform_copy_from_user failed"); | ||
| goto out; |
There was a problem hiding this comment.
you should not jump to out here, and instead just return an error. There is nothing to cleanup
shim/linux/src/entry.c
Outdated
| } | ||
|
|
||
| pmut_mut_user_indices = mut_args.indices; | ||
| mut_args.indices = vzalloc(mut_args.nmsrs * sizeof(*mut_args.indices)); |
There was a problem hiding this comment.
All of this code should be in the handle function, not in the dispatch function. The only code that should be in a dispatch function is copy to/from user and a call to the handle. Otherwise, all of this will have to be written again for Windows.
There was a problem hiding this comment.
Also note that if the handle function has to use platform_copy_from_user and vice versa because the args have user address in them, that is fine.
There was a problem hiding this comment.
Looking at this again, it seems that the only logic that could move from the dispatch function would be vzalloc, because in order to call vfree in the handle function, we would need a copy_to_user call in the handle function also.
It seems to me that moving vzalloc and/or copy_to_user would only (1) complicate unit testing, and (2) make this code less clear.
| @@ -319,7 +363,7 @@ dev_unlocked_ioctl_system( | |||
|
|
|||
| case KVM_GET_MSR_INDEX_LIST: { | |||
| return dispatch_system_kvm_get_msr_index_list( | |||
| (struct kvm_msr_list *)ioctl_args); | |||
| (struct kvm_msr_list __user *)ioctl_args); | |||
There was a problem hiding this comment.
we should be consistent with the use of __user. Either they all use it, or non do.
There was a problem hiding this comment.
I would be in favor of using it everywhere.
| pmut_rdl->reg0 = MV_RDL_FLAG_ALL; | ||
| pmut_rdl->num_entries = ((uint64_t)0); | ||
|
|
||
| platform_expects(MV_INVALID_HANDLE != g_mut_hndl); |
There was a problem hiding this comment.
Why do you check to see if the global handle is valid on every iteration of the loop?
There was a problem hiding this comment.
From other code it seemed like a pattern to check the handle before each hypercall. I take it that is not necessary.
It's compiled out of a release build anyways, right?
| } | ||
|
|
||
| mut_nmsrs = ((uint32_t)0); | ||
| do { |
There was a problem hiding this comment.
do loops are not allowed. Please make sure you read the AUTOSAR and MISRA specs.
There was a problem hiding this comment.
Would you mind pointing out where that rule is defined? I have looked at the docs and the only reference to do ... while loops I can find is about using braces. The documents are quite large, I must be missing it.
There was a problem hiding this comment.
Without a reference to the spec disallowing this, I'm inclined to approve. @rianquinn do you have a reference?
There was a problem hiding this comment.
Due to time pressures, I'm going to merge. I'll be creating an issue and assigning it to @rianquinn to provide reference to the spec (and a fix).
enr0n
force-pushed
the
get-msr-index-list
branch
5 times, most recently
from
c1399c5
to
6e765d0
Compare
enr0n
force-pushed
the
get-msr-index-list
branch
2 times, most recently
from
3e721e5
to
9c0214a
Compare
Implment support in the shim for the KVM_GET_MSR_INDEX_LIST ioctl. Expand functionality in the mv_pp_op_msr_get_supported_list mock to support unit tests, and also add an intergratio test. Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
enr0n
force-pushed
the
get-msr-index-list
branch
from
9c0214a
to
9e94c70
Compare
Implment support in the shim for the KVM_GET_MSR_INDEX_LIST ioctl.
Expand functionality in the mv_pp_op_msr_get_supported_list mock to
support unit tests, and also add an intergratio test.