Re: [Xen-devel] [PATCH v3 07/24] x86/emul: Clean up the naming of the retire union

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: 30 November 2016 13:50
> To: Xen-devel <xen-devel@xxxxxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich
> <JBeulich@xxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Subject: [PATCH v3 07/24] x86/emul: Clean up the naming of the retire union
> 
> Rename byte to raw, as the field being a single byte long is an
> implementation
> detail.  Make the bitfields part of an anonymous struct to remove the .flags
> qualifier.  Change the types of the flags to being booleans, to match their
> use.
> 

Is it legitimate to use a bool in a bitfield? Also, anonymous unions are not 
part of C99 AFAIK... are we now stipulating something more recent?

  Paul

> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
> 
> v3:
>  * New
> ---
>  xen/arch/x86/hvm/emulate.c             |  6 +++---
>  xen/arch/x86/x86_emulate/x86_emulate.c | 10 +++++-----
>  xen/arch/x86/x86_emulate/x86_emulate.h | 10 +++++-----
>  3 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index bc259ec..fe62500 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1791,13 +1791,13 @@ static int _hvm_emulate_one(struct
> hvm_emulate_ctxt *hvmemul_ctxt,
>      new_intr_shadow = hvmemul_ctxt->intr_shadow;
> 
>      /* MOV-SS instruction toggles MOV-SS shadow, else we just clear it. */
> -    if ( hvmemul_ctxt->ctxt.retire.flags.mov_ss )
> +    if ( hvmemul_ctxt->ctxt.retire.mov_ss )
>          new_intr_shadow ^= HVM_INTR_SHADOW_MOV_SS;
>      else
>          new_intr_shadow &= ~HVM_INTR_SHADOW_MOV_SS;
> 
>      /* STI instruction toggles STI shadow, else we just clear it. */
> -    if ( hvmemul_ctxt->ctxt.retire.flags.sti )
> +    if ( hvmemul_ctxt->ctxt.retire.sti )
>          new_intr_shadow ^= HVM_INTR_SHADOW_STI;
>      else
>          new_intr_shadow &= ~HVM_INTR_SHADOW_STI;
> @@ -1808,7 +1808,7 @@ static int _hvm_emulate_one(struct
> hvm_emulate_ctxt *hvmemul_ctxt,
>          hvm_funcs.set_interrupt_shadow(curr, new_intr_shadow);
>      }
> 
> -    if ( hvmemul_ctxt->ctxt.retire.flags.hlt &&
> +    if ( hvmemul_ctxt->ctxt.retire.hlt &&
>           !hvm_local_events_need_delivery(curr) )
>      {
>          hvm_hlt(regs->eflags);
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
> b/xen/arch/x86/x86_emulate/x86_emulate.c
> index 9c28ed4..416812e 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -1905,7 +1905,7 @@ x86_decode(
>      state->eip = ctxt->regs->eip;
> 
>      /* Initialise output state in x86_emulate_ctxt */
> -    ctxt->retire.byte = 0;
> +    ctxt->retire.raw = 0;
> 
>      op_bytes = def_op_bytes = ad_bytes = def_ad_bytes = ctxt-
> >addr_size/8;
>      if ( op_bytes == 8 )
> @@ -2668,7 +2668,7 @@ x86_emulate(
> 
>      case 0x17: /* pop %%ss */
>          src.val = x86_seg_ss;
> -        ctxt->retire.flags.mov_ss = 1;
> +        ctxt->retire.mov_ss = 1;
>          goto pop_seg;
> 
>      case 0x1e: /* push %%ds */
> @@ -2996,7 +2996,7 @@ x86_emulate(
>          if ( (rc = load_seg(seg, src.val, 0, NULL, ctxt, ops)) != 0 )
>              goto done;
>          if ( seg == x86_seg_ss )
> -            ctxt->retire.flags.mov_ss = 1;
> +            ctxt->retire.mov_ss = 1;
>          dst.type = OP_NONE;
>          break;
> 
> @@ -4033,7 +4033,7 @@ x86_emulate(
> 
>      case 0xf4: /* hlt */
>          generate_exception_if(!mode_ring0(), EXC_GP, 0);
> -        ctxt->retire.flags.hlt = 1;
> +        ctxt->retire.hlt = 1;
>          break;
> 
>      case 0xf5: /* cmc */
> @@ -4247,7 +4247,7 @@ x86_emulate(
>          if ( !(_regs.eflags & EFLG_IF) )
>          {
>              _regs.eflags |= EFLG_IF;
> -            ctxt->retire.flags.sti = 1;
> +            ctxt->retire.sti = 1;
>          }
>          break;
> 
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h
> b/xen/arch/x86/x86_emulate/x86_emulate.h
> index b0f0304..ef39601 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -468,12 +468,12 @@ struct x86_emulate_ctxt
> 
>      /* Retirement state, set by the emulator (valid only on X86EMUL_OKAY).
> */
>      union {
> +        uint8_t raw;
>          struct {
> -            uint8_t hlt:1;          /* Instruction HLTed. */
> -            uint8_t mov_ss:1;       /* Instruction sets MOV-SS irq shadow. */
> -            uint8_t sti:1;          /* Instruction sets STI irq shadow. */
> -        } flags;
> -        uint8_t byte;
> +            bool hlt:1;          /* Instruction HLTed. */
> +            bool mov_ss:1;       /* Instruction sets MOV-SS irq shadow. */
> +            bool sti:1;          /* Instruction sets STI irq shadow. */
> +        };
>      } retire;
>  };
> 
> --
> 2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel