| Message ID | 1480513841-7565-8-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] > Sent: 30 November 2016 13:50 > To: Xen-devel <xen-devel@lists.xen.org> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich > <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com> > 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@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Paul Durrant <paul.durrant@citrix.com> > > 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
On 30/11/16 13:58, Paul Durrant wrote: >> -----Original Message----- >> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] >> Sent: 30 November 2016 13:50 >> To: Xen-devel <xen-devel@lists.xen.org> >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich >> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com> >> 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? Yes. Why wouldn't it be? > Also, anonymous unions are not part of C99 AFAIK... are we now stipulating something more recent? We used gnu99 for as long as I can remember, and we have other examples of this pattern already in Xen. ~Andrew
> -----Original Message----- > From: Andrew Cooper > Sent: 30 November 2016 14:02 > To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen- > devel@lists.xen.org> > Cc: Jan Beulich <JBeulich@suse.com> > Subject: Re: [PATCH v3 07/24] x86/emul: Clean up the naming of the retire > union > > On 30/11/16 13:58, Paul Durrant wrote: > >> -----Original Message----- > >> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] > >> Sent: 30 November 2016 13:50 > >> To: Xen-devel <xen-devel@lists.xen.org> > >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich > >> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com> > >> 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? > > Yes. Why wouldn't it be? > They always used to be restricted to int or unsigned int. Looks like this was relaxed in C99. > > Also, anonymous unions are not part of C99 AFAIK... are we now stipulating > something more recent? > > We used gnu99 for as long as I can remember, and we have other examples > of this pattern already in Xen. > If there's precedent then that's fine. Reviewed-by: Paul Durrant <paul.durrant@citrix.com> > ~Andrew
>>> On 30.11.16 at 15:05, <Paul.Durrant@citrix.com> wrote: >> From: Andrew Cooper >> Sent: 30 November 2016 14:02 >> On 30/11/16 13:58, Paul Durrant wrote: >> > Also, anonymous unions are not part of C99 AFAIK... are we now stipulating >> something more recent? >> >> We used gnu99 for as long as I can remember, and we have other examples >> of this pattern already in Xen. >> > > If there's precedent then that's fine. Tighter rules only apply for the public headers. Jan
>>> On 30.11.16 at 14:50, <andrew.cooper3@citrix.com> wrote: > 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. With that you should then also use true/false in assignments to these fields. This taken care of, > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
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; };
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. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Paul Durrant <paul.durrant@citrix.com> 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(-)