- add missing brackets for 'if/else' statements based on MISRA-C
requirements
v1 -> v2:
* add brackets for each conditions in 'if' statements to improve
the readability
* modify 'ptdev_init' to make the logic clearer
Tracked-On: #861
Signed-off-by: Shiqing Gao <shiqing.gao@intel.com>
1.Function return type inconsistent
2.cast on a constant value
V1->V2 add () to return type
V2->V3 keep the sbuf_get and sbuf_put return code
Tracked-On: #861
Signed-off-by: Huihuang Shi <huihuang.shi@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
Expression should be boolean immediate before 'if','while' key-words.
V1->V2 add () to bool expression
Tracked-On: #861
Signed-off-by: Huihuang Shi <huihuang.shi@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
struct seg_desc_vmcs is actually for segment descriptor vmcs fields.
Change its name to vmcs_seg_field
Tracked-On: #1231
Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
1. The wrong operand size is assigned in instruction decode phase
if the operand size is 1 byte.
According to the SDM, the bit 0(w bit) of opcode should be checked
first to detect whether the operand size is 1 byte. Then, check
whether there is prefix to overwrite the default operand size.
The original instruction decode doesn't care about the operand
size. But do opsize fixup during instruction emulation phase.
With ACRN we need operand size packed to ioreq and send to DM
after instruction decode.
2. We should always touch the GPA by following opsize to avoid side
effect (especially when GPA is for a MMIO).
Tracked-On: #1337
Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
This patch fixes the following issue pointed by Xiangyang and Junjie.
There are some macro arguments acting as formal parameter names.
Drop such arguments since they make no difference to the expanded
implementation and they might confuse some developers.
Here is an example.
'ptr' is dropped in this patch, which is acting as a formal parameter
name and make no difference to the expanded implementation.
-#define build_atomic_load(name, size, type, ptr) \
+#define build_atomic_load(name, size, type) \
static inline type name(const volatile type *ptr) \
{ \
type ret; \
asm volatile("mov" size " %1,%0" \
: "=r" (ret) \
: "m" (*ptr) \
: "cc", "memory"); \
return ret; \
}
Some minor coding style fixes are also included in this patch.
- use TAB for the alignment rather than mixing TAB with space
- fix some typo in the comments
Tracked-On: #861
Signed-off-by: Shiqing Gao <shiqing.gao@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
MISRAC checks whether a variable is assigned a value not used in
all branches of a program. Var value which is unused on all paths
can be removed with a consequent improvement in the readability
and efficiency of the code. This patch is used to fix these
violations.
Tracked-On: #861
Signed-off-by: Junjun Shan <junjun.shan@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
MISRAC has requirement about implict conversion: actual to formal
param. This patch is used to fix part of these violations.
1.Add a new structure seg_desc_vmcs to hold the VMCS field address of
segment selector to clean up seg_desc structure.
2.Add the definition of maximum MSI entry and the relevant judgement.
3.The violations in shell.c, logmsg.c will be fixed in other series of
patches with modification of function snprintf(), vsnprintf() and other
related usages.
v1->v2:
*Move the definition of struct seg_desc_vmcs from instr_emul.h to
instr_emul.c.
*Modify the formal parameter type in function definition from uint8_t
to char instead of using cast.
*Drop the const declaration for char data in formal parameter.
v2->v3:
*update the data missing conversion.
*change type of internal parameter len to avoid casting in npklog.c.
*change the conversion from signed char to unsigned int in
uart16550_getc() to solve sign-extension.
Tracked-On: #861
Signed-off-by: Junjun Shan <junjun.shan@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
The vie->opcode should be updated when decode_two_byte_opcode(),
otherwise for two bytes opcode emulate(movzx/movsx) will fail.
Signed-off-by: Qi Yadong <yadong.qi@intel.com>
MISRAC has requirements about literal value requires a U suffix and
signed/unsigned conversion with cast. This patch is used to solve
these violations.
v1->v2
*Drop the cast of sz from uint32_t to int32_t, the signed/unsigned
violation of nchars will be solved by other patch together with
printf/sprintf/console/vuart/uart code.
*Delete the unnecessary L suffix of shifting operand.
Tracked-On: #861
Signed-off-by: Junjun Shan <junjun.shan@intel.com>
Reviewed by: Junjie Mao <junjie.mao@intel.com>
The current movs emulation has issues:
1. it use gva to get/put data.
2. it only support src and dst operand are memory which does not
apply to our case (one of them should be mmio and triggers
EPT voilation).
This patch fix the issue by:
1. convert the address from gva to hva before access it.
2. handle mmio emulation.
Also fix the issue introduced by previous instruction reshuffle
patchset:
1. the desc validation should be only applied to none-64bit mode.
2. gva2gpa should be given correct guest virtual address.
Specailly for movs, we cache the dst gpa if the check during
movs decoding success. And use it directly during movs
emulation.
Tracked-On: #1207
Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
Tested-by: Qi Yadong <yadog.qi@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
- convert function like macros to inline functions based on MISRA-C
requirement
- remove some unused and duplicated macros
Tracked-On: #861
Signed-off-by: Shiqing Gao <shiqing.gao@intel.com>
Reviewed-by: Junjie Mao <junjie.mao@intel.com>
We have two same log message in decode_instruction. It's a little bit confusing.
So, this patch refine the log message to make it more explicit.
BTW, we refine one message in create_vcpu.
Tracked-On: #1136
Signed-off-by: Kaige Fu <kaige.fu@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
For the instructions other than MOVS, one operand is register
and another one is memory which trigger EPT voilation. In this
case, there is one possibility that EPT voilation happens before
guest fault:
the fault is triggered by related guest PTE access bit
voilation (like write to a gva with R/W bit cleared in PTE).
So we do this kind of check and inject exception to guest
accordingly during instruction decoding phase.
Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
Unlike the other instructions we emulated, MOVS has two operands
both are memory.
So we need to check whether the operand is valid GVA. With VMX
enabled, the src operand is always checked first by VMX. Which
means if src operand is not valid GVA, it will trigger fault
in guest before trigger EPT. So we don't need to check src
operand. Only need to check dst operand here.
Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
Drop the get_gla function and add
- get_gva_di_si_nocheck
get gva from ES:DI and DS(other segment):SI without
check faulure case
- get_gva_di_si_check
get gva from ES:DI and DS(other segment):SI with failure
case checked
TODO:
- Save dst_gpa and src_gpa for instruction emulation phase
use.
Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
We will do check only during instruction decode phase.
vie_calculate_gla will be called both in instruction emulation
phase, we move the check out of vie_calculate_gla.
Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
According to SDM vol3 25.1.1
With VMX enabled, following exception will be handled by guest
without trigger VM exit:
- faults based on privilege level
- general protection due to relevent segment being unusable
- general protection due to offset beyond limit of relevent segment
- alignment check exception
ACRN always assume VMX is enabled. So we don't need to these check
in instruction emulation. But we need to do page fault related check.
Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
If rm show there is no SIB following rm field, we should get
base_register info from rm.
Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
removed some unnecessary variables and functions.
v1-->v2:
Replace div-by-zero with an inline ASM code
Signed-off-by: Mingqiang Chi <mingqiang.chi@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
The bracket is required when the level of precedence of
the operators is less than 13. Add the bracket to logical
conjunctions. The commit applys the rule to the files under
hypervisor/arch/x86/guest/*
Signed-off-by: Yang, Yu-chu <yu-chu.yang@intel.com>
Reviewed-by: Junjie Mao <junjie.mao@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
MISRAC requires that a switch statement shall contain a default clause.
This patch add the default clause and some comments for the ones
violated the rule.
Signed-off-by: Shiqing Gao <shiqing.gao@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
It's not reasonable to use push/pop against mmio. So we remove
the push/pop emulation.
Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
Reviewed-by: Jason Chen CJ <jason.cj.chen@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
We inject invalid opcode if instruction decode fails.
We don't support many instruction. If new type guest hit
the invalid opcode and it's necessary to emulate that
instruction, we could add new instruction then.
Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
Reviewed-by: Jason Chen CJ <jason.cj.chen@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
When doing copy_to/from_gva, it's possible the guest no page
happens on none-first page. In this case, we need get correct
fault address from gva2gpa.
Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
Reviewed-by: Jason Chen CJ <jason.cj.chen@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
Acked-by: Eddie Dong <Eddie.dong@intel.com>
This patch add some comments after the default and before the break
in the switch statement based on MISRA-C requirement.
Signed-off-by: Shiqing Gao <shiqing.gao@intel.com>
Reviewed-by: Junjie Mao <junjie.mao@intel.com>
vm_update_register calls vm_get/set_register to update register and vm_update_rflags
calls vm_update_register to update RFLAGS.
We have make vm_get/set_register as non-failed function in previous patch.
So, this patch make the vm_update_register/rflags as void.
Signed-off-by: Kaige Fu <kaige.fu@intel.com>
Reviewed-by: Eddie Dong <eddie.dong@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
vie_read/write_bytereg call vm_get/set_register to get/set byteregs.
We have make vm_get/set_register as non-failed function in previous patch.
So, this patch make the vie_read/write_bytereg as non-failed function too.
Signed-off-by: Kaige Fu <kaige.fu@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
Originally, vm_set/get_register return -EINVAL when "vcpu == NULL" or reg is invalid.
But, we don't check the return value actually and there is no chance we get an
null-vcpu and invalid reg in current implementation.
This patch add pre-assumptions about valid parameters before the function and make
them as non-failed functions.
- static uint64_t vm_get_register(struct vcpu *vcpu, enum cpu_reg_name reg)
- static void vm_set_register(struct vcpu *vcpu, enum cpu_reg_name reg, uint64_t val)
Signed-off-by: Kaige Fu <kaige.fu@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
Originally, vm_get_seg_desc return -EINVAL when "vcpu == NULL" or seg is invalid.
But, we don't check the return value actually and there is no chance we get an
null-vcpu and invalid seg in current implementation.
This patch adds pre-assumptions and makes the function as void.
Signed-off-by: Kaige Fu <kaige.fu@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
This patch just removes some dead codes related to Instruction Emulation.
Signed-off-by: Kaige Fu <kaige.fu@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
Originally, there is cross-references between instr_emul.h and instr_emul_wrapper.h.
User must include both of them when calling instruction emulation functions. This
will raise up some confusion and inconvenience.
So we rearrange the logic of instruction emulation code as following:
- External API -- defined in instr_emul.h
* decode_instruction(struct vcpu *vcpu)
* emulate_instruction(struct vcpu *vcpu)
- Make all other functions as static in instr_emul.c
- Remove instr_emul_wrapper.c/h
No functional change.
Signed-off-by: Kaige Fu <kaige.fu@intel.com>
Reviewed-by: Eddie Dong <eddie.dong@intel.com>
Reviewed-by: Yin Fengwei <fengwei.yin@intel.com>
ASSERT is too strict for HV when error happens during emulating instruction.
This patch remove all ASSERT and return a negative error code when failing to
emulate instruction.
Originally, getcc will return -EINVAL when opsize are not one of (1, 2, 4, 8).
But theoretically, opsize in current implementation can only be one of (1, 2, 4, 8).
So, we will always get valid "cc".
This patch add a pre-assumption and make sure that getcc always return valid value.
For the current code, #GP will be injected to guest if something goes wrong with
instruction emulation.
Signed-off-by: Kaige Fu <kaige.fu@intel.com>
Reviewed-by: Yin Fengwei <fengwei.yin@intel.com>
There are so many __unused prefix spaning the emulate_xxx functions. This patch
unify the parameters of emulate_xxx to avoid it.
- All emulate_xxx functions are defined as emulate_xxx(struct vcpu *vcpu, instr_emul_vie *vie)
or emulate_xxx(struct vcpu *vcpu, instr_emul_vie *vie, struct vm_guest_paging *paging).
- Move mmio_read/write to instr_emul.c and call them directly.
Signed-off-by: Kaige Fu <kaige.fu@intel.com>
Reviewed-by: Yin Fengwei <fengwei.yin@intel.com>
there will be 3 types of vcpu runtime contexts:
- runtime contexts always saved/restored during VM exit/entry, which
include general registers rax/rcx/rdx/rbx/rbp/rsi/rdi/r8~r15, cr2 and
msr for spectre control (ia32_spec_ctrl)
- runtime contexts on-demand cached/updated during VM exit/entry, which
include frequently used registers rsp, rip, efer, rflags, cr0 and cr4
- runtime contexts always read/write from/to VMCS, which include left
registers not in above
this patch add get/set register APIs for vcpu runtime contexts, and unified
the save/restore method for them according to above description.
v3:
- update vcpu_get/set_cr0/4 as unified interface to get/set guest cr0/cr4,
use on-demand cache for reading, but always write to VMCS for writing.
v2:
- use reg_cached/reg_updated for on-demand runtime contexts
- always read/write cr3 from/to VMCS
Signed-off-by: Jason Chen CJ <jason.cj.chen@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
Fix the parameter name mismatch between API declaration and definition.
v2 -> v3:
* Fix two more violations which are missed in previous report.
shell_puts and console_write
v1 -> v2:
* Replace 'ret_desc' with 'desc'
Signed-off-by: Shiqing Gao <shiqing.gao@intel.com>
Reviewed-by: Junjie Mao <junjie.mao@intel.com>
one/two_byte_opcodes is indexed by op_byte. So vie_op->op_byte is unnecessary.
This patch remove it and add a new variable opcode to instr_emul_vie.
Signed-off-by: Kaige Fu <kaige.fu@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
There are size2mask array to convert addsize to corresponding mask and function
vie_size2mask doing the same thing except validation check of addrsize.
Theoretically, addrsize can only be one of (1, 2, 4, 8). So, the check of addrsize
is unnecessary.
This patch remove vie_size2mask and use size2maks directly.
Signed-off-by: Kaige Fu <kaige.fu@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
We have vm_set/get_register here. There is no need to wrap the function with
vie_read_register.
This patch remove it.
Signed-off-by: Kaige Fu <kaige.fu@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
Reviewed-by: Yin Fengwei <fengwei.yin@intel.com>
In order to comply with MISRA C rules, renamed vairables
and function names starting with "_".
The major changes invloves mostly static function
names, as they are being called inside the same file
by a wrapper function.
Signed-off-by: Arindam Roy <arindam.roy@intel.com>
In the C99 standard, the order of evaluation associated with
multiple #, multiple ## or a mix of # and ## preprocessor
operator is unspecified. For this case, gcc 7.3.0 manual
does not specify related implementation. So it is unsafe
to use multiple # or ## in a macro.
BTW, there are some macros with one or more "##" which are
not used by hypervisor.
Update relate codes to avoid using multiple # or ## in a macro;
Remove unused macros with one or more "##";
Remove "struct __hack;" at the end of GETCC since it is useless.
Note:
'##' operator usage constraints: A ## preprocessing token shall
not occur at the beginning or at the end of a replacement list
for either form of macro definition.
V1--V2:
Update relate codes to avoid using multiple # or ## in a macro.
V2-->V3:
Remove unused macros with one or more "##";
Remove "struct __hack;" at the end of GETCC since it is useless.
Signed-off-by: Xiangyang Wu <xiangyang.wu@linux.intel.com>
Reviewed-by: Junjie Mao <junjie.mao@intel.com>
For data structure types "enum vpic_wire_mode, struct stack_canary",
its name is identical with variable name in the same scope.
This MISRA C violation is detected by static analysis tool.
For variables "segment_override, pde_index", its name is identical
with function name. This MISRA C violation is detected.
Naming convention rule:Variable name can be shortened from
its data structure type name.
The following udpates are made:
enum vpic_wire_mode vpic_wire_mode-->enum vpic_wire_mode wire_mode
struct stack_canary stack_canary-->struct stack_canary stk_canary
uint8_t segment_override:1 --> uint8_t seg_override:1
uint32_t pde_index--> uint32_t pde_idx
V1-->V2:
Remove update "enum cpu_state cpu_state-->enum cpu_state state"
and "enum irqstate irqstate-->enum irq_ops_mode ops_mode", other
patch will cover it.
V2-->V3:
Update "uint32_t pde_index--> uint32_t pde_idx".
Signed-off-by: Xiangyang Wu <xiangyang.wu@linux.intel.com>
For data struct type struct vie, emul_ctxt, its name
is identical with variable name in the same scope.
This MISRA C violation is detected by static analysis
tool.
According to naming convention rule: If the data structure
type is used by only one module and its name meaning is
simplistic, its name needs prefix shorten module name.
Follow the same rule, data structure name "vie_op" needs
to be renamed;
The following updates are made in this patch:
struct vie-->struct instr_emul_vie
struct vie_op-->struct instr_emul_vie_op
struct emul_ctxt-->struct instr_emul_ctxt
Signed-off-by: Xiangyang Wu <xiangyang.wu@linux.intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
In the function scope,the parameter should not be
changed as Misra required.
V1->V2 recover some violations because of ldra's false positive.
V2->V3 sync local variable' type to parameter's type with the prefix of const.
Signed-off-by: Huihuang Shi <huihuang.shi@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
The general-purpose register layout is identical to instructio emulation context.
So no need to do the mapping.
Signed-off-by: Binbin Wu <binbin.wu@intel.com>
Reviewed-by: Eddie Dong <eddie.dong@intel.com>
In the hypervisor, there are many casts from
an void pointer to integer pointer, then from
integer pointer to structure pointer.
These pointer castings are detected by static analysis
tool. All pointer casts are violations, There are
some duplicated pointer cast. This will make deviation
analysis complex.
BTW, there are one useless pointer casting and one
wrong pointer casting in the hypervisor.
Remvoe duplicated pointer casts to make deviation analysis
simple;
Remove one useless pointer casting;
Update one wrong pointer casting.
Note: There are many void type pointer casts, non-void type
pointer is casted to void type pointer, char type pointer casts,
non-char type pointer is casted to char type pointer. These pointer
casting is need by the memory management module, IO moudle etc.
Deviation analysis will be made and recoded in the analysis report.
V1-->V2:
Fix mixing pointer and array voilation.
V2-->V3:
Remvoe pointer casting from integer pointer into
non-void/non-char pointer directly;
Remove redundant type conversion.
Signed-off-by: Xiangyang Wu <xiangyang.wu@intel.com>
Reviewed-by: Junjie Mao <junjie.mao@intel.com>
Added brackets for expression to make it easy to understand and
reduce the mistake of precedence. The rule is applied to the
mixed same level of prevedence opeartors, high level presedence
operators and logical expression.
Signed-off-by: Yang, Yu-chu <yu-chu.yang@intel.com>
Acked-by: Anthony Xu <anthony.xu@intel.com>
Clean up most reported integral-type-related violations still existing under
arch/x86/guest/. The remaining reports that are not trivial to suppress will be
explained in separate documents.
Also move acpi_info outside acrn_common.h as the structure is no longer shared
with DM.
v1 -> v2:
* Move struct acpi_info to bsp_extern.h
Signed-off-by: Junjie Mao <junjie.mao@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
MISRA C requires that a 'if' statement followed by one or more 'else if'
statement shall be terminated by an 'else' statement which contains either
side-effect or a comment, to ensure that conditions are considered
exhaustively.
Note that a simple 'if' statement is not required to be terminated by 'else'.
This patch fixes such violations by either refactoring the code or add the
'else' statement with either a comment (describing why this case can be skipped)
or logging the event. It may not be satisfactory for the release version where
logging is no-op, but properly handling these non-trivial cases is out of the
scope of this patch.
v1 -> v2:
* Fix unintended semantic changes in add_(msix|intx)_remapping and
io_instr_vmexit_handler.
* Simplify boolean checks in vpic_ocw2.
* Rephrase the comment in strtol_deci.
Signed-off-by: Junjie Mao <junjie.mao@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
In the current hypervisor, there are many members of CPU_reg_name
used to check range and useless register names.
Define some CPU_REG_XX_FIRST and CPU_REG_XX_LAST MACROs to
make range checking clear;
Remove useless register names CPU_REG_XX_LAST in CPU_reg_name;
Update the related caller.
V1-->V2:
Update a mistake, replace second CPU_REG_SEG_FIRST
with CPU_REG_SEG_LAST in ASSERT.
V2-->V3:
Add '()' for bool expression in ASSERT.
Signed-off-by: Xiangyang Wu <xiangyang.wu@intel.com>
Acked-by: Eddie Dong <eddie.dong@intel.com>
