| Message ID | 1479915538-15282-3-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: 23 November 2016 15:39 > To: Xen-devel <xen-devel@lists.xen.org> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich > <JBeulich@suse.com>; Tim (Xen.org) <tim@xen.org>; George Dunlap > <George.Dunlap@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com> > Subject: [PATCH 02/15] x86/emul: Simplfy emulation state setup > > The current code to set up emulation state is ad-hoc and error prone. > > * Consistently zero all emulation state structures. > * Avoid explicitly initialising some state to 0. > * Explicitly identify all input and output state in x86_emulate_ctxt. This > involves rearanging some fields. > * Have x86_decode() explicitly initalise all output state at its start. > > In addition, move the calculation of hvmemul_ctxt->ctxt.swint_emulate > from > _hvm_emulate_one() to hvm_emulate_init_once(), as it doesn't need > recalculating for each instruction. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Tim Deegan <tim@xen.org> > CC: George Dunlap <george.dunlap@eu.citrix.com> > CC: Paul Durrant <paul.durrant@citrix.com> > --- > xen/arch/x86/hvm/emulate.c | 28 +++++++++++++++------------- > xen/arch/x86/mm.c | 3 ++- > xen/arch/x86/mm/shadow/common.c | 4 ++-- > xen/arch/x86/x86_emulate/x86_emulate.c | 2 ++ > xen/arch/x86/x86_emulate/x86_emulate.h | 22 +++++++++++++++------- > 5 files changed, 36 insertions(+), 23 deletions(-) > > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c > index 3ab0e8e..f24e211 100644 > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -1769,13 +1769,6 @@ static int _hvm_emulate_one(struct > hvm_emulate_ctxt *hvmemul_ctxt, > > vio->mmio_retry = 0; > > - if ( cpu_has_vmx ) > - hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_none; > - else if ( cpu_has_svm_nrips ) > - hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_icebp; > - else > - hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_all; > - > rc = x86_emulate(&hvmemul_ctxt->ctxt, ops); > > if ( rc == X86EMUL_OKAY && vio->mmio_retry ) > @@ -1946,14 +1939,23 @@ void hvm_emulate_init_once( > struct hvm_emulate_ctxt *hvmemul_ctxt, > struct cpu_user_regs *regs) > { > - hvmemul_ctxt->intr_shadow = > hvm_funcs.get_interrupt_shadow(current); > - hvmemul_ctxt->ctxt.regs = regs; > - hvmemul_ctxt->ctxt.force_writeback = 1; > - hvmemul_ctxt->seg_reg_accessed = 0; > - hvmemul_ctxt->seg_reg_dirty = 0; > - hvmemul_ctxt->set_context = 0; > + struct vcpu *curr = current; > + > + memset(hvmemul_ctxt, 0, sizeof(*hvmemul_ctxt)); > + > + hvmemul_ctxt->intr_shadow = hvm_funcs.get_interrupt_shadow(curr); > hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt); > hvmemul_get_seg_reg(x86_seg_ss, hvmemul_ctxt); > + > + hvmemul_ctxt->ctxt.regs = regs; > + hvmemul_ctxt->ctxt.force_writeback = true; > + > + if ( cpu_has_vmx ) > + hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_none; > + else if ( cpu_has_svm_nrips ) > + hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_icebp; > + else > + hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_all; > } > > void hvm_emulate_init_per_insn( > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > index 03dcd71..9901f6f 100644 > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -5363,8 +5363,9 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned > long addr, > goto bail; > } > > + memset(&ptwr_ctxt, 0, sizeof(ptwr_ctxt)); > + > ptwr_ctxt.ctxt.regs = regs; > - ptwr_ctxt.ctxt.force_writeback = 0; > ptwr_ctxt.ctxt.addr_size = ptwr_ctxt.ctxt.sp_size = > is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG; > ptwr_ctxt.ctxt.swint_emulate = x86_swint_emulate_none; > diff --git a/xen/arch/x86/mm/shadow/common.c > b/xen/arch/x86/mm/shadow/common.c > index ced2313..6f6668d 100644 > --- a/xen/arch/x86/mm/shadow/common.c > +++ b/xen/arch/x86/mm/shadow/common.c > @@ -385,8 +385,9 @@ const struct x86_emulate_ops > *shadow_init_emulation( > struct vcpu *v = current; > unsigned long addr; > > + memset(sh_ctxt, 0, sizeof(*sh_ctxt)); > + > sh_ctxt->ctxt.regs = regs; > - sh_ctxt->ctxt.force_writeback = 0; > sh_ctxt->ctxt.swint_emulate = x86_swint_emulate_none; > > if ( is_pv_vcpu(v) ) > @@ -396,7 +397,6 @@ const struct x86_emulate_ops > *shadow_init_emulation( > } > > /* Segment cache initialisation. Primed with CS. */ > - sh_ctxt->valid_seg_regs = 0; > creg = hvm_get_seg_reg(x86_seg_cs, sh_ctxt); > > /* Work out the emulation mode. */ > diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c > b/xen/arch/x86/x86_emulate/x86_emulate.c > index 04f0dac..c5d9664 100644 > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -1904,6 +1904,8 @@ x86_decode( > state->regs = ctxt->regs; > state->eip = ctxt->regs->eip; > > + /* Initialise output state in x86_emulate_ctxt */ > + ctxt->opcode = ~0u; > ctxt->retire.byte = 0; In the commit message you state that x86_decode() will "explicitly initalise all output state at its start". This doesn't seem to be all the output state. In fact you appear to be removing some initialization. > > op_bytes = def_op_bytes = ad_bytes = def_ad_bytes = ctxt- > >addr_size/8; > diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h > b/xen/arch/x86/x86_emulate/x86_emulate.h > index 993c576..93b268e 100644 > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -412,6 +412,10 @@ struct cpu_user_regs; > > struct x86_emulate_ctxt > { > + /* > + * Input state: > + */ > + > /* Register state before/after emulation. */ > struct cpu_user_regs *regs; > > @@ -421,14 +425,21 @@ struct x86_emulate_ctxt > /* Stack pointer width in bits (16, 32 or 64). */ > unsigned int sp_size; > > - /* Canonical opcode (see below). */ > - unsigned int opcode; > - > /* Software event injection support. */ > enum x86_swint_emulation swint_emulate; > > /* Set this if writes may have side effects. */ > - uint8_t force_writeback; > + bool force_writeback; Is this type change intentional? I assume it is, but you didn't call it out. Paul > + > + /* Caller data that can be used by x86_emulate_ops' routines. */ > + void *data; > + > + /* > + * Output state: > + */ > + > + /* Canonical opcode (see below). */ > + unsigned int opcode; > > /* Retirement state, set by the emulator (valid only on X86EMUL_OKAY). > */ > union { > @@ -439,9 +450,6 @@ struct x86_emulate_ctxt > } flags; > uint8_t byte; > } retire; > - > - /* Caller data that can be used by x86_emulate_ops' routines. */ > - void *data; > }; > > /* > -- > 2.1.4
On 23/11/16 15:58, Paul Durrant wrote: >> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c >> b/xen/arch/x86/x86_emulate/x86_emulate.c >> index 04f0dac..c5d9664 100644 >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -1904,6 +1904,8 @@ x86_decode( >> state->regs = ctxt->regs; >> state->eip = ctxt->regs->eip; >> >> + /* Initialise output state in x86_emulate_ctxt */ >> + ctxt->opcode = ~0u; >> ctxt->retire.byte = 0; > In the commit message you state that x86_decode() will "explicitly initalise all output state at its start". This doesn't seem to be all the output state. In fact you appear to be removing some initialization. There are only two fields of output state, as delineated by the extra comments in x86_emulate_ctxt. Most of x86_emulate_ctxt is input state. > >> op_bytes = def_op_bytes = ad_bytes = def_ad_bytes = ctxt- >>> addr_size/8; >> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h >> b/xen/arch/x86/x86_emulate/x86_emulate.h >> index 993c576..93b268e 100644 >> --- a/xen/arch/x86/x86_emulate/x86_emulate.h >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h >> @@ -412,6 +412,10 @@ struct cpu_user_regs; >> >> struct x86_emulate_ctxt >> { >> + /* >> + * Input state: >> + */ >> + >> /* Register state before/after emulation. */ >> struct cpu_user_regs *regs; >> >> @@ -421,14 +425,21 @@ struct x86_emulate_ctxt >> /* Stack pointer width in bits (16, 32 or 64). */ >> unsigned int sp_size; >> >> - /* Canonical opcode (see below). */ >> - unsigned int opcode; >> - >> /* Software event injection support. */ >> enum x86_swint_emulation swint_emulate; >> >> /* Set this if writes may have side effects. */ >> - uint8_t force_writeback; >> + bool force_writeback; > Is this type change intentional? I assume it is, but you didn't call it out. Yes. I thought I had it in the commit message, but will update for v2. ~Andrew > > Paul > >> + >> + /* Caller data that can be used by x86_emulate_ops' routines. */ >> + void *data; >> + >> + /* >> + * Output state: >> + */ >> + >> + /* Canonical opcode (see below). */ >> + unsigned int opcode; >> >> /* Retirement state, set by the emulator (valid only on X86EMUL_OKAY). >> */ >> union { >> @@ -439,9 +450,6 @@ struct x86_emulate_ctxt >> } flags; >> uint8_t byte; >> } retire; >> - >> - /* Caller data that can be used by x86_emulate_ops' routines. */ >> - void *data; >> }; >> >> /* >> -- >> 2.1.4
> -----Original Message----- > From: Andrew Cooper > Sent: 23 November 2016 16:01 > To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen- > devel@lists.xen.org> > Cc: Jan Beulich <JBeulich@suse.com>; Tim (Xen.org) <tim@xen.org>; George > Dunlap <George.Dunlap@citrix.com> > Subject: Re: [PATCH 02/15] x86/emul: Simplfy emulation state setup > > On 23/11/16 15:58, Paul Durrant wrote: > >> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c > >> b/xen/arch/x86/x86_emulate/x86_emulate.c > >> index 04f0dac..c5d9664 100644 > >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c > >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > >> @@ -1904,6 +1904,8 @@ x86_decode( > >> state->regs = ctxt->regs; > >> state->eip = ctxt->regs->eip; > >> > >> + /* Initialise output state in x86_emulate_ctxt */ > >> + ctxt->opcode = ~0u; > >> ctxt->retire.byte = 0; > > In the commit message you state that x86_decode() will "explicitly initalise > all output state at its start". This doesn't seem to be all the output state. In > fact you appear to be removing some initialization. > > There are only two fields of output state, as delineated by the extra > comments in x86_emulate_ctxt. Most of x86_emulate_ctxt is input state. D'oh, yes. Sorry, got confused by the field movements... my eyes were seeing '+' as '-' for some reason. Paul > > > > >> op_bytes = def_op_bytes = ad_bytes = def_ad_bytes = ctxt- > >>> addr_size/8; > >> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h > >> b/xen/arch/x86/x86_emulate/x86_emulate.h > >> index 993c576..93b268e 100644 > >> --- a/xen/arch/x86/x86_emulate/x86_emulate.h > >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > >> @@ -412,6 +412,10 @@ struct cpu_user_regs; > >> > >> struct x86_emulate_ctxt > >> { > >> + /* > >> + * Input state: > >> + */ > >> + > >> /* Register state before/after emulation. */ > >> struct cpu_user_regs *regs; > >> > >> @@ -421,14 +425,21 @@ struct x86_emulate_ctxt > >> /* Stack pointer width in bits (16, 32 or 64). */ > >> unsigned int sp_size; > >> > >> - /* Canonical opcode (see below). */ > >> - unsigned int opcode; > >> - > >> /* Software event injection support. */ > >> enum x86_swint_emulation swint_emulate; > >> > >> /* Set this if writes may have side effects. */ > >> - uint8_t force_writeback; > >> + bool force_writeback; > > Is this type change intentional? I assume it is, but you didn't call it out. > > Yes. I thought I had it in the commit message, but will update for v2. > > ~Andrew > > > > > Paul > > > >> + > >> + /* Caller data that can be used by x86_emulate_ops' routines. */ > >> + void *data; > >> + > >> + /* > >> + * Output state: > >> + */ > >> + > >> + /* Canonical opcode (see below). */ > >> + unsigned int opcode; > >> > >> /* Retirement state, set by the emulator (valid only on > X86EMUL_OKAY). > >> */ > >> union { > >> @@ -439,9 +450,6 @@ struct x86_emulate_ctxt > >> } flags; > >> uint8_t byte; > >> } retire; > >> - > >> - /* Caller data that can be used by x86_emulate_ops' routines. */ > >> - void *data; > >> }; > >> > >> /* > >> -- > >> 2.1.4
At 15:38 +0000 on 23 Nov (1479915525), Andrew Cooper wrote: > The current code to set up emulation state is ad-hoc and error prone. > > * Consistently zero all emulation state structures. > * Avoid explicitly initialising some state to 0. > * Explicitly identify all input and output state in x86_emulate_ctxt. This > involves rearanging some fields. > * Have x86_decode() explicitly initalise all output state at its start. > > In addition, move the calculation of hvmemul_ctxt->ctxt.swint_emulate from > _hvm_emulate_one() to hvm_emulate_init_once(), as it doesn't need > recalculating for each instruction. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Shadow code changes Acked-by: Tim Deegan <tim@xen.org>
>>> On 23.11.16 at 16:38, <andrew.cooper3@citrix.com> wrote: > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -5363,8 +5363,9 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned long addr, > goto bail; > } > > + memset(&ptwr_ctxt, 0, sizeof(ptwr_ctxt)); > + > ptwr_ctxt.ctxt.regs = regs; > - ptwr_ctxt.ctxt.force_writeback = 0; > ptwr_ctxt.ctxt.addr_size = ptwr_ctxt.ctxt.sp_size = > is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG; > ptwr_ctxt.ctxt.swint_emulate = x86_swint_emulate_none; Mind using an explicit initializer instead, moving everything there that's already known at function start? > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -1904,6 +1904,8 @@ x86_decode( > state->regs = ctxt->regs; > state->eip = ctxt->regs->eip; > > + /* Initialise output state in x86_emulate_ctxt */ > + ctxt->opcode = ~0u; I have to admit that I don't see the point of this. > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -412,6 +412,10 @@ struct cpu_user_regs; > > struct x86_emulate_ctxt > { > + /* > + * Input state: > + */ > + > /* Register state before/after emulation. */ > struct cpu_user_regs *regs; > > @@ -421,14 +425,21 @@ struct x86_emulate_ctxt > /* Stack pointer width in bits (16, 32 or 64). */ > unsigned int sp_size; > > - /* Canonical opcode (see below). */ > - unsigned int opcode; > - > /* Software event injection support. */ > enum x86_swint_emulation swint_emulate; > > /* Set this if writes may have side effects. */ > - uint8_t force_writeback; > + bool force_writeback; > + > + /* Caller data that can be used by x86_emulate_ops' routines. */ > + void *data; > + > + /* > + * Output state: > + */ > + > + /* Canonical opcode (see below). */ > + unsigned int opcode; > > /* Retirement state, set by the emulator (valid only on X86EMUL_OKAY). */ > union { Hmm, this separation looks to be correct for the current state of the emulator, but I don't think it is architecturally so (and hence it would become wrong in the course of us completing it): Both addr_size and sp_size are not necessarily inputs only. Also keeping regs in the input only group is slightly misleading - the pointer itself surely is input only, but the pointed to data isn't. So I'd suggest to have three groups: input, input/output, output, even if for your purpose here you only want to tell apart fields which need to be initialized before calling x86_emulate() / x86_decode() (the first two groups) from those which don't (the last group). Jan
On 24/11/16 13:44, Jan Beulich wrote: >>>> On 23.11.16 at 16:38, <andrew.cooper3@citrix.com> wrote: >> --- a/xen/arch/x86/mm.c >> +++ b/xen/arch/x86/mm.c >> @@ -5363,8 +5363,9 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned long addr, >> goto bail; >> } >> >> + memset(&ptwr_ctxt, 0, sizeof(ptwr_ctxt)); >> + >> ptwr_ctxt.ctxt.regs = regs; >> - ptwr_ctxt.ctxt.force_writeback = 0; >> ptwr_ctxt.ctxt.addr_size = ptwr_ctxt.ctxt.sp_size = >> is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG; >> ptwr_ctxt.ctxt.swint_emulate = x86_swint_emulate_none; > Mind using an explicit initializer instead, moving everything there that's > already known at function start? Done. > >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -1904,6 +1904,8 @@ x86_decode( >> state->regs = ctxt->regs; >> state->eip = ctxt->regs->eip; >> >> + /* Initialise output state in x86_emulate_ctxt */ >> + ctxt->opcode = ~0u; > I have to admit that I don't see the point of this. There are early exit paths which leave it uninitialised. An alternative would be explicitly document that it is only valid for X86EMUL_OKAY? > >> --- a/xen/arch/x86/x86_emulate/x86_emulate.h >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h >> @@ -412,6 +412,10 @@ struct cpu_user_regs; >> >> struct x86_emulate_ctxt >> { >> + /* >> + * Input state: >> + */ >> + >> /* Register state before/after emulation. */ >> struct cpu_user_regs *regs; >> >> @@ -421,14 +425,21 @@ struct x86_emulate_ctxt >> /* Stack pointer width in bits (16, 32 or 64). */ >> unsigned int sp_size; >> >> - /* Canonical opcode (see below). */ >> - unsigned int opcode; >> - >> /* Software event injection support. */ >> enum x86_swint_emulation swint_emulate; >> >> /* Set this if writes may have side effects. */ >> - uint8_t force_writeback; >> + bool force_writeback; >> + >> + /* Caller data that can be used by x86_emulate_ops' routines. */ >> + void *data; >> + >> + /* >> + * Output state: >> + */ >> + >> + /* Canonical opcode (see below). */ >> + unsigned int opcode; >> >> /* Retirement state, set by the emulator (valid only on X86EMUL_OKAY). */ >> union { > Hmm, this separation looks to be correct for the current state of the > emulator, but I don't think it is architecturally so (and hence it would > become wrong in the course of us completing it): Both addr_size and > sp_size are not necessarily inputs only. Also keeping regs in the > input only group is slightly misleading - the pointer itself surely is input > only, but the pointed to data isn't. > > So I'd suggest to have three groups: input, input/output, output, > even if for your purpose here you only want to tell apart fields which > need to be initialized before calling x86_emulate() / x86_decode() > (the first two groups) from those which don't (the last group). Right - proposed net result looks like: struct x86_emulate_ctxt { /* * Input-only state: */ /* Software event injection support. */ enum x86_swint_emulation swint_emulate; /* Set this if writes may have side effects. */ bool force_writeback; /* Caller data that can be used by x86_emulate_ops' routines. */ void *data; /* * Input/output state: */ /* Register state before/after emulation. */ struct cpu_user_regs *regs; /* Default address size in current execution mode (16, 32, or 64). */ unsigned int addr_size; /* Stack pointer width in bits (16, 32 or 64). */ unsigned int sp_size; /* * Output-only state: */ /* Canonical opcode (see below) (valid only on X86EMUL_OKAY). */ unsigned int opcode; /* Retirement state, set by the emulator (valid only on X86EMUL_OKAY). */ union { 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; } retire; }; If that is ok? ~Andrew
>>> On 24.11.16 at 14:59, <andrew.cooper3@citrix.com> wrote: > On 24/11/16 13:44, Jan Beulich wrote: >>>>> On 23.11.16 at 16:38, <andrew.cooper3@citrix.com> wrote: >>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >>> @@ -1904,6 +1904,8 @@ x86_decode( >>> state->regs = ctxt->regs; >>> state->eip = ctxt->regs->eip; >>> >>> + /* Initialise output state in x86_emulate_ctxt */ >>> + ctxt->opcode = ~0u; >> I have to admit that I don't see the point of this. > > There are early exit paths which leave it uninitialised. An alternative > would be explicitly document that it is only valid for X86EMUL_OKAY? Well, to me that goes without saying, but I'm fine with it being added. >> So I'd suggest to have three groups: input, input/output, output, >> even if for your purpose here you only want to tell apart fields which >> need to be initialized before calling x86_emulate() / x86_decode() >> (the first two groups) from those which don't (the last group). > > Right - proposed net result looks like: > > struct x86_emulate_ctxt > { > /* > * Input-only state: > */ > > /* Software event injection support. */ > enum x86_swint_emulation swint_emulate; > > /* Set this if writes may have side effects. */ > bool force_writeback; > > /* Caller data that can be used by x86_emulate_ops' routines. */ > void *data; > > /* > * Input/output state: > */ > > /* Register state before/after emulation. */ > struct cpu_user_regs *regs; > > /* Default address size in current execution mode (16, 32, or 64). */ > unsigned int addr_size; > > /* Stack pointer width in bits (16, 32 or 64). */ > unsigned int sp_size; > > /* > * Output-only state: > */ > > /* Canonical opcode (see below) (valid only on X86EMUL_OKAY). */ > unsigned int opcode; > > /* Retirement state, set by the emulator (valid only on > X86EMUL_OKAY). */ > union { > 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; > } retire; > }; > > If that is ok? Looks good, thanks. With that 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 3ab0e8e..f24e211 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -1769,13 +1769,6 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt, vio->mmio_retry = 0; - if ( cpu_has_vmx ) - hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_none; - else if ( cpu_has_svm_nrips ) - hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_icebp; - else - hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_all; - rc = x86_emulate(&hvmemul_ctxt->ctxt, ops); if ( rc == X86EMUL_OKAY && vio->mmio_retry ) @@ -1946,14 +1939,23 @@ void hvm_emulate_init_once( struct hvm_emulate_ctxt *hvmemul_ctxt, struct cpu_user_regs *regs) { - hvmemul_ctxt->intr_shadow = hvm_funcs.get_interrupt_shadow(current); - hvmemul_ctxt->ctxt.regs = regs; - hvmemul_ctxt->ctxt.force_writeback = 1; - hvmemul_ctxt->seg_reg_accessed = 0; - hvmemul_ctxt->seg_reg_dirty = 0; - hvmemul_ctxt->set_context = 0; + struct vcpu *curr = current; + + memset(hvmemul_ctxt, 0, sizeof(*hvmemul_ctxt)); + + hvmemul_ctxt->intr_shadow = hvm_funcs.get_interrupt_shadow(curr); hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt); hvmemul_get_seg_reg(x86_seg_ss, hvmemul_ctxt); + + hvmemul_ctxt->ctxt.regs = regs; + hvmemul_ctxt->ctxt.force_writeback = true; + + if ( cpu_has_vmx ) + hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_none; + else if ( cpu_has_svm_nrips ) + hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_icebp; + else + hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_all; } void hvm_emulate_init_per_insn( diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 03dcd71..9901f6f 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -5363,8 +5363,9 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned long addr, goto bail; } + memset(&ptwr_ctxt, 0, sizeof(ptwr_ctxt)); + ptwr_ctxt.ctxt.regs = regs; - ptwr_ctxt.ctxt.force_writeback = 0; ptwr_ctxt.ctxt.addr_size = ptwr_ctxt.ctxt.sp_size = is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG; ptwr_ctxt.ctxt.swint_emulate = x86_swint_emulate_none; diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c index ced2313..6f6668d 100644 --- a/xen/arch/x86/mm/shadow/common.c +++ b/xen/arch/x86/mm/shadow/common.c @@ -385,8 +385,9 @@ const struct x86_emulate_ops *shadow_init_emulation( struct vcpu *v = current; unsigned long addr; + memset(sh_ctxt, 0, sizeof(*sh_ctxt)); + sh_ctxt->ctxt.regs = regs; - sh_ctxt->ctxt.force_writeback = 0; sh_ctxt->ctxt.swint_emulate = x86_swint_emulate_none; if ( is_pv_vcpu(v) ) @@ -396,7 +397,6 @@ const struct x86_emulate_ops *shadow_init_emulation( } /* Segment cache initialisation. Primed with CS. */ - sh_ctxt->valid_seg_regs = 0; creg = hvm_get_seg_reg(x86_seg_cs, sh_ctxt); /* Work out the emulation mode. */ diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index 04f0dac..c5d9664 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -1904,6 +1904,8 @@ x86_decode( state->regs = ctxt->regs; state->eip = ctxt->regs->eip; + /* Initialise output state in x86_emulate_ctxt */ + ctxt->opcode = ~0u; ctxt->retire.byte = 0; op_bytes = def_op_bytes = ad_bytes = def_ad_bytes = ctxt->addr_size/8; diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h index 993c576..93b268e 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.h +++ b/xen/arch/x86/x86_emulate/x86_emulate.h @@ -412,6 +412,10 @@ struct cpu_user_regs; struct x86_emulate_ctxt { + /* + * Input state: + */ + /* Register state before/after emulation. */ struct cpu_user_regs *regs; @@ -421,14 +425,21 @@ struct x86_emulate_ctxt /* Stack pointer width in bits (16, 32 or 64). */ unsigned int sp_size; - /* Canonical opcode (see below). */ - unsigned int opcode; - /* Software event injection support. */ enum x86_swint_emulation swint_emulate; /* Set this if writes may have side effects. */ - uint8_t force_writeback; + bool force_writeback; + + /* Caller data that can be used by x86_emulate_ops' routines. */ + void *data; + + /* + * Output state: + */ + + /* Canonical opcode (see below). */ + unsigned int opcode; /* Retirement state, set by the emulator (valid only on X86EMUL_OKAY). */ union { @@ -439,9 +450,6 @@ struct x86_emulate_ctxt } flags; uint8_t byte; } retire; - - /* Caller data that can be used by x86_emulate_ops' routines. */ - void *data; }; /*
The current code to set up emulation state is ad-hoc and error prone. * Consistently zero all emulation state structures. * Avoid explicitly initialising some state to 0. * Explicitly identify all input and output state in x86_emulate_ctxt. This involves rearanging some fields. * Have x86_decode() explicitly initalise all output state at its start. In addition, move the calculation of hvmemul_ctxt->ctxt.swint_emulate from _hvm_emulate_one() to hvm_emulate_init_once(), as it doesn't need recalculating for each instruction. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Tim Deegan <tim@xen.org> CC: George Dunlap <george.dunlap@eu.citrix.com> CC: Paul Durrant <paul.durrant@citrix.com> --- xen/arch/x86/hvm/emulate.c | 28 +++++++++++++++------------- xen/arch/x86/mm.c | 3 ++- xen/arch/x86/mm/shadow/common.c | 4 ++-- xen/arch/x86/x86_emulate/x86_emulate.c | 2 ++ xen/arch/x86/x86_emulate/x86_emulate.h | 22 +++++++++++++++------- 5 files changed, 36 insertions(+), 23 deletions(-)