| Message ID | 585121010200007800129030@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 14/12/16 09:37, Jan Beulich wrote: > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -432,6 +432,7 @@ typedef union { > #define CR4_OSFXSR (1<<9) > #define CR4_OSXMMEXCPT (1<<10) > #define CR4_UMIP (1<<11) > +#define CR4_FSGSBASE (1<<16) > #define CR4_OSXSAVE (1<<18) > > /* EFLAGS bit definitions. */ > @@ -5205,6 +5206,44 @@ x86_emulate( > } > break; > > + case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */ > + { > + unsigned long cr4; > + > + fail_if(modrm_mod != 3); This should surely be an explicit #UD? The only issue is that we don't yet implement Grp15/F3 instructions with memory operands (as there are none yet defined)? > + generate_exception_if((modrm_reg & 4) || !mode_64bit(), EXC_UD); > + fail_if(!ops->read_cr); > + if ( (rc = ops->read_cr(4, &cr4, ctxt)) != X86EMUL_OKAY ) > + goto done; > + generate_exception_if(!(cr4 & CR4_FSGSBASE), EXC_UD); > + fail_if(!ops->read_segment); seg = modrm_reg & 1 ? x86_seg_gs : x86_seg_fs would avoid the repetition later for write_segment(). > + if ( (rc = ops->read_segment(modrm_reg & 1 ? x86_seg_gs : x86_seg_fs, > + &sreg, ctxt)) != X86EMUL_OKAY ) > + goto done; > + dst.reg = decode_register(modrm_rm, &_regs, 0); > + if ( !(modrm_reg & 2) ) > + { > + /* rd{f,g}sbase */ > + dst.type = OP_REG; > + dst.bytes = (op_bytes == 8) ? 8 : 4; > + dst.val = sreg.base; > + break; > + } > + /* wr{f,g}sbase */ > + if ( op_bytes == 8 ) > + { > + sreg.base = *dst.reg; > + generate_exception_if(!is_canonical_address(sreg.base), EXC_GP, 0); > + } > + else > + sreg.base = (uint32_t)*dst.reg; > + fail_if(!ops->write_segment); > + if ( (rc = ops->write_segment(modrm_reg & 1 ? x86_seg_gs : x86_seg_fs, > + &sreg, ctxt)) != X86EMUL_OKAY ) > + goto done; Can I talk you into using a switch statement for this? I know it would only have two or four cases, it but read path exiting midway through took me a while to spot even though I was expecting to find it. ~Andrew
>>> On 14.12.16 at 13:36, <andrew.cooper3@citrix.com> wrote: > On 14/12/16 09:37, Jan Beulich wrote: >> @@ -5205,6 +5206,44 @@ x86_emulate( >> } >> break; >> >> + case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */ >> + { >> + unsigned long cr4; >> + >> + fail_if(modrm_mod != 3); > > This should surely be an explicit #UD? The only issue is that we don't > yet implement Grp15/F3 instructions with memory operands (as there are > none yet defined)? If there weren't any, I probably would have used #UD here. But there are - ptwrite is even in the normal SDM already (but it looks to be missing from the opcode map). >> + generate_exception_if((modrm_reg & 4) || !mode_64bit(), EXC_UD); >> + fail_if(!ops->read_cr); >> + if ( (rc = ops->read_cr(4, &cr4, ctxt)) != X86EMUL_OKAY ) >> + goto done; >> + generate_exception_if(!(cr4 & CR4_FSGSBASE), EXC_UD); >> + fail_if(!ops->read_segment); > > seg = modrm_reg & 1 ? x86_seg_gs : x86_seg_fs > > would avoid the repetition later for write_segment(). Will do. >> + if ( (rc = ops->read_segment(modrm_reg & 1 ? x86_seg_gs : x86_seg_fs, >> + &sreg, ctxt)) != X86EMUL_OKAY ) >> + goto done; >> + dst.reg = decode_register(modrm_rm, &_regs, 0); >> + if ( !(modrm_reg & 2) ) >> + { >> + /* rd{f,g}sbase */ >> + dst.type = OP_REG; >> + dst.bytes = (op_bytes == 8) ? 8 : 4; >> + dst.val = sreg.base; >> + break; >> + } >> + /* wr{f,g}sbase */ >> + if ( op_bytes == 8 ) >> + { >> + sreg.base = *dst.reg; >> + generate_exception_if(!is_canonical_address(sreg.base), EXC_GP, 0); >> + } >> + else >> + sreg.base = (uint32_t)*dst.reg; >> + fail_if(!ops->write_segment); >> + if ( (rc = ops->write_segment(modrm_reg & 1 ? x86_seg_gs : x86_seg_fs, >> + &sreg, ctxt)) != X86EMUL_OKAY ) >> + goto done; > > Can I talk you into using a switch statement for this? I know it would > only have two or four cases, it but read path exiting midway through > took me a while to spot even though I was expecting to find it. I was specifically trying to avoid another nested switch here. Would it be sufficient if I just made the write path the "else" to that "if"? Jan
On 14/12/16 13:18, Jan Beulich wrote: >>>> On 14.12.16 at 13:36, <andrew.cooper3@citrix.com> wrote: >> On 14/12/16 09:37, Jan Beulich wrote: >>> @@ -5205,6 +5206,44 @@ x86_emulate( >>> } >>> break; >>> >>> + case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */ >>> + { >>> + unsigned long cr4; >>> + >>> + fail_if(modrm_mod != 3); >> This should surely be an explicit #UD? The only issue is that we don't >> yet implement Grp15/F3 instructions with memory operands (as there are >> none yet defined)? > If there weren't any, I probably would have used #UD here. But > there are - ptwrite is even in the normal SDM already (but it looks > to be missing from the opcode map). I find that the opcode maps are consistently out of date. However, I don't understand why you have chosen to avoid the #UD. #UD is the appropriate action for an opcode which isn't implemented. > >>> + if ( (rc = ops->read_segment(modrm_reg & 1 ? x86_seg_gs : x86_seg_fs, >>> + &sreg, ctxt)) != X86EMUL_OKAY ) >>> + goto done; >>> + dst.reg = decode_register(modrm_rm, &_regs, 0); >>> + if ( !(modrm_reg & 2) ) >>> + { >>> + /* rd{f,g}sbase */ >>> + dst.type = OP_REG; >>> + dst.bytes = (op_bytes == 8) ? 8 : 4; >>> + dst.val = sreg.base; >>> + break; >>> + } >>> + /* wr{f,g}sbase */ >>> + if ( op_bytes == 8 ) >>> + { >>> + sreg.base = *dst.reg; >>> + generate_exception_if(!is_canonical_address(sreg.base), EXC_GP, 0); >>> + } >>> + else >>> + sreg.base = (uint32_t)*dst.reg; >>> + fail_if(!ops->write_segment); >>> + if ( (rc = ops->write_segment(modrm_reg & 1 ? x86_seg_gs : x86_seg_fs, >>> + &sreg, ctxt)) != X86EMUL_OKAY ) >>> + goto done; >> Can I talk you into using a switch statement for this? I know it would >> only have two or four cases, it but read path exiting midway through >> took me a while to spot even though I was expecting to find it. > I was specifically trying to avoid another nested switch here. Would > it be sufficient if I just made the write path the "else" to that "if"? Yeah - that is also ok. ~Andrew
>>> On 14.12.16 at 14:28, <andrew.cooper3@citrix.com> wrote: > On 14/12/16 13:18, Jan Beulich wrote: >>>>> On 14.12.16 at 13:36, <andrew.cooper3@citrix.com> wrote: >>> On 14/12/16 09:37, Jan Beulich wrote: >>>> @@ -5205,6 +5206,44 @@ x86_emulate( >>>> } >>>> break; >>>> >>>> + case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */ >>>> + { >>>> + unsigned long cr4; >>>> + >>>> + fail_if(modrm_mod != 3); >>> This should surely be an explicit #UD? The only issue is that we don't >>> yet implement Grp15/F3 instructions with memory operands (as there are >>> none yet defined)? >> If there weren't any, I probably would have used #UD here. But >> there are - ptwrite is even in the normal SDM already (but it looks >> to be missing from the opcode map). > > I find that the opcode maps are consistently out of date. > > However, I don't understand why you have chosen to avoid the #UD. #UD > is the appropriate action for an opcode which isn't implemented. I don't think we should raise #UD knowingly for the wrong reason. Hence my plan was to go through all fail_if()-s once we are at a point where we consider the emulator complete enough, but keep using fail_if() vs #UD to distinguish cases where we know of gaps vs an encoding being undefined in up-to-date docs. While I guess we don't always match this model at present, that was at least what I have been trying to follow in all my recent work. Jan
On 14/12/16 13:39, Jan Beulich wrote: >>>> On 14.12.16 at 14:28, <andrew.cooper3@citrix.com> wrote: >> On 14/12/16 13:18, Jan Beulich wrote: >>>>>> On 14.12.16 at 13:36, <andrew.cooper3@citrix.com> wrote: >>>> On 14/12/16 09:37, Jan Beulich wrote: >>>>> @@ -5205,6 +5206,44 @@ x86_emulate( >>>>> } >>>>> break; >>>>> >>>>> + case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */ >>>>> + { >>>>> + unsigned long cr4; >>>>> + >>>>> + fail_if(modrm_mod != 3); >>>> This should surely be an explicit #UD? The only issue is that we don't >>>> yet implement Grp15/F3 instructions with memory operands (as there are >>>> none yet defined)? >>> If there weren't any, I probably would have used #UD here. But >>> there are - ptwrite is even in the normal SDM already (but it looks >>> to be missing from the opcode map). >> I find that the opcode maps are consistently out of date. >> >> However, I don't understand why you have chosen to avoid the #UD. #UD >> is the appropriate action for an opcode which isn't implemented. > I don't think we should raise #UD knowingly for the wrong reason. > Hence my plan was to go through all fail_if()-s once we are at a > point where we consider the emulator complete enough, but keep > using fail_if() vs #UD to distinguish cases where we know of gaps > vs an encoding being undefined in up-to-date docs. While I guess > we don't always match this model at present, that was at least > what I have been trying to follow in all my recent work. Hmm. I had considered at one point to have X86EMUL_NOT_IMPLEMENTED which was separate from X86EMUL_UNHANDLEABLE, as they are subtly different. NOT_IMPLEMENTED means "everything else was fine, but I don't know how to do that", whereas UNHANDLEABLE is "something went wrong trying to do that", and is typically used for missing hooks or problems in lower levels. ~Andrew
>>> On 14.12.16 at 14:47, <andrew.cooper3@citrix.com> wrote: > On 14/12/16 13:39, Jan Beulich wrote: >>>>> On 14.12.16 at 14:28, <andrew.cooper3@citrix.com> wrote: >>> On 14/12/16 13:18, Jan Beulich wrote: >>>>>>> On 14.12.16 at 13:36, <andrew.cooper3@citrix.com> wrote: >>>>> On 14/12/16 09:37, Jan Beulich wrote: >>>>>> @@ -5205,6 +5206,44 @@ x86_emulate( >>>>>> } >>>>>> break; >>>>>> >>>>>> + case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */ >>>>>> + { >>>>>> + unsigned long cr4; >>>>>> + >>>>>> + fail_if(modrm_mod != 3); >>>>> This should surely be an explicit #UD? The only issue is that we don't >>>>> yet implement Grp15/F3 instructions with memory operands (as there are >>>>> none yet defined)? >>>> If there weren't any, I probably would have used #UD here. But >>>> there are - ptwrite is even in the normal SDM already (but it looks >>>> to be missing from the opcode map). >>> I find that the opcode maps are consistently out of date. >>> >>> However, I don't understand why you have chosen to avoid the #UD. #UD >>> is the appropriate action for an opcode which isn't implemented. >> I don't think we should raise #UD knowingly for the wrong reason. >> Hence my plan was to go through all fail_if()-s once we are at a >> point where we consider the emulator complete enough, but keep >> using fail_if() vs #UD to distinguish cases where we know of gaps >> vs an encoding being undefined in up-to-date docs. While I guess >> we don't always match this model at present, that was at least >> what I have been trying to follow in all my recent work. > > Hmm. > > I had considered at one point to have X86EMUL_NOT_IMPLEMENTED which was > separate from X86EMUL_UNHANDLEABLE, as they are subtly different. > NOT_IMPLEMENTED means "everything else was fine, but I don't know how to > do that", whereas UNHANDLEABLE is "something went wrong trying to do > that", and is typically used for missing hooks or problems in lower levels. Fine with me, preferably when we're closer to covering most of the opcode space. Bottom line here though is that unless you insist I'd prefer to keep the fail_if() as being more in line with what we do elsewhere. Jan
On 14/12/16 14:49, Jan Beulich wrote: >>>> On 14.12.16 at 14:47, <andrew.cooper3@citrix.com> wrote: >> On 14/12/16 13:39, Jan Beulich wrote: >>>>>> On 14.12.16 at 14:28, <andrew.cooper3@citrix.com> wrote: >>>> On 14/12/16 13:18, Jan Beulich wrote: >>>>>>>> On 14.12.16 at 13:36, <andrew.cooper3@citrix.com> wrote: >>>>>> On 14/12/16 09:37, Jan Beulich wrote: >>>>>>> @@ -5205,6 +5206,44 @@ x86_emulate( >>>>>>> } >>>>>>> break; >>>>>>> >>>>>>> + case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */ >>>>>>> + { >>>>>>> + unsigned long cr4; >>>>>>> + >>>>>>> + fail_if(modrm_mod != 3); >>>>>> This should surely be an explicit #UD? The only issue is that we don't >>>>>> yet implement Grp15/F3 instructions with memory operands (as there are >>>>>> none yet defined)? >>>>> If there weren't any, I probably would have used #UD here. But >>>>> there are - ptwrite is even in the normal SDM already (but it looks >>>>> to be missing from the opcode map). >>>> I find that the opcode maps are consistently out of date. >>>> >>>> However, I don't understand why you have chosen to avoid the #UD. #UD >>>> is the appropriate action for an opcode which isn't implemented. >>> I don't think we should raise #UD knowingly for the wrong reason. >>> Hence my plan was to go through all fail_if()-s once we are at a >>> point where we consider the emulator complete enough, but keep >>> using fail_if() vs #UD to distinguish cases where we know of gaps >>> vs an encoding being undefined in up-to-date docs. While I guess >>> we don't always match this model at present, that was at least >>> what I have been trying to follow in all my recent work. >> Hmm. >> >> I had considered at one point to have X86EMUL_NOT_IMPLEMENTED which was >> separate from X86EMUL_UNHANDLEABLE, as they are subtly different. >> NOT_IMPLEMENTED means "everything else was fine, but I don't know how to >> do that", whereas UNHANDLEABLE is "something went wrong trying to do >> that", and is typically used for missing hooks or problems in lower levels. > Fine with me, preferably when we're closer to covering most of the > opcode space. Bottom line here though is that unless you insist I'd > prefer to keep the fail_if() as being more in line with what we do > elsewhere. Fine for now. ~Andrew
--- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -432,6 +432,7 @@ typedef union { #define CR4_OSFXSR (1<<9) #define CR4_OSXMMEXCPT (1<<10) #define CR4_UMIP (1<<11) +#define CR4_FSGSBASE (1<<16) #define CR4_OSXSAVE (1<<18) /* EFLAGS bit definitions. */ @@ -5205,6 +5206,44 @@ x86_emulate( } break; + case X86EMUL_OPC_F3(0x0f, 0xae): /* Grp15 */ + { + unsigned long cr4; + + fail_if(modrm_mod != 3); + generate_exception_if((modrm_reg & 4) || !mode_64bit(), EXC_UD); + fail_if(!ops->read_cr); + if ( (rc = ops->read_cr(4, &cr4, ctxt)) != X86EMUL_OKAY ) + goto done; + generate_exception_if(!(cr4 & CR4_FSGSBASE), EXC_UD); + fail_if(!ops->read_segment); + if ( (rc = ops->read_segment(modrm_reg & 1 ? x86_seg_gs : x86_seg_fs, + &sreg, ctxt)) != X86EMUL_OKAY ) + goto done; + dst.reg = decode_register(modrm_rm, &_regs, 0); + if ( !(modrm_reg & 2) ) + { + /* rd{f,g}sbase */ + dst.type = OP_REG; + dst.bytes = (op_bytes == 8) ? 8 : 4; + dst.val = sreg.base; + break; + } + /* wr{f,g}sbase */ + if ( op_bytes == 8 ) + { + sreg.base = *dst.reg; + generate_exception_if(!is_canonical_address(sreg.base), EXC_GP, 0); + } + else + sreg.base = (uint32_t)*dst.reg; + fail_if(!ops->write_segment); + if ( (rc = ops->write_segment(modrm_reg & 1 ? x86_seg_gs : x86_seg_fs, + &sreg, ctxt)) != X86EMUL_OKAY ) + goto done; + break; + } + case X86EMUL_OPC(0x0f, 0xaf): /* imul */ emulate_2op_SrcV_srcmem("imul", src, dst, _regs.eflags); break;