Re: [Xen-devel] [PATCH 02/17] x86emul: fetch all insn bytes during the decode phase

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/09/16 10:55, Jan Beulich wrote:
> > > > On 13.09.16 at 20:44, <andrew.cooper3@xxxxxxxxxx> wrote:
> > On 08/09/16 14:07, Jan Beulich wrote:
> > > @@ -1602,6 +1602,45 @@ struct x86_emulate_state {
> > >   #define _regs (state->regs)
> > >   
> > >   static int
> > > +x86_decode_base(
> > What do you mean by decode_base here?
> The base instruction set (no 0f or alike prefixes). Suggestions for
> a better name welcome.
x86_decode_onebyte() to match the table of opcodes it is further decoding.

> > > @@ -2644,18 +2704,13 @@ x86_emulate(
> > >   
> > >       case 0x9a: /* call (far, absolute) */ {
> > >           struct segment_register reg;
> > > -        uint16_t sel;
> > > -        uint32_t eip;
> > >   
> > > -        generate_exception_if(mode_64bit(), EXC_UD, -1);
> > > +        ASSERT(!mode_64bit());
> > Are we going to strictly require that noone ever hand-crafts a
> > x86_emulate_state and hands it to x86_emulate()?
> Absolutely - that's why its definition does not live in a header.
Ok.

> > I would suggest leaving the generate_exception_if(mode_64bit(), EXC_UD,
> > -1); after the ASSERT() so even if we do end up in a wonky state, we
> > don't try to jump the guest to 0.
> That would look really strange to a reader, I think, and hence I'd
> rather not do this if I can get the patch accepted without.
It is conceptually no different to

default:
ASSERT_UNREACHABLE();
return;

~Andrew

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