Re: [Qemu-devel] [PATCH 04/14] hvf: add fields to CPUState and CPUX86State; add definitions

[Stefan Hajnoczi]

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 <address@hidden>
> ---
>  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)