On Sun, Aug 27, 2017 at 08:56:44PM -0500, Sergio Andres Gomez Del Real wrote:
> This commit adds some fields specific to hvf in CPUState and
> CPUX86State. It also adds some handy #defines.
>
> Signed-off-by: Sergio Andres Gomez Del Real <sergio.g.delr...@gmail.com>
> ---
> include/qom/cpu.h | 8 ++++++++
> target/i386/cpu.h | 23 +++++++++++++++++++++++
> 2 files changed, 31 insertions(+)
>
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 25eefea7ab..c46eb61240 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -407,6 +407,14 @@ struct CPUState {
> * unnecessary flushes.
> */
> uint16_t pending_tlb_flush;
> +
> + // HVF
Please see ./CODING_STYLE "7. Comment style":
We use traditional C-style /* */ comments and avoid // comments.
> + bool hvf_vcpu_dirty;
> + uint64_t hvf_fd; // fd of vcpu created by HVF
File descriptors are ints, why is this uint64_t?
This also raises an issue: none of these fields are used in the patch.
This makes it hard to review the code. I don't know whether the
definitions are correct without seeing the code that uses them.
Please structure the patch series so that patches are self-contained,
logical changes that can be reviewed in isolation. These field
definitions should be made in the same patch uses the fields.
> + // Supporting data structures for VMCS capabilities
> + // and x86 emulation state
> + struct hvf_vcpu_caps* hvf_caps;
> + struct hvf_x86_state* hvf_x86;
Coding style:
1. Please use typedef struct {...} TypeName.
2. Please use TypeName *ptr.
> };
>
> QTAILQ_HEAD(CPUTailQ, CPUState);
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 051867399b..7d90f08b98 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -82,15 +82,19 @@
> #define R_GS 5
>
> /* segment descriptor fields */
> +#define DESC_G_SHIFT 23
> #define DESC_G_MASK (1 << 23)
> #define DESC_B_SHIFT 22
> #define DESC_B_MASK (1 << DESC_B_SHIFT)
> #define DESC_L_SHIFT 21 /* x86_64 only : 64 bit code segment */
> #define DESC_L_MASK (1 << DESC_L_SHIFT)
> +#define DESC_AVL_SHIFT 20
> #define DESC_AVL_MASK (1 << 20)
> +#define DESC_P_SHIFT 15
> #define DESC_P_MASK (1 << 15)
> #define DESC_DPL_SHIFT 13
> #define DESC_DPL_MASK (3 << DESC_DPL_SHIFT)
> +#define DESC_S_SHIFT 12
> #define DESC_S_MASK (1 << 12)
Please reuse the new constants to avoid duplication (just like existing
code for DESC_B_MASK does):
#define DESC_S_SHIFT 12
#define DESC_S_MASK (1 << DESC_S_SHIFT)
