Re: [Xen-devel] [PATCH] x86emul: support {RD,WR}{F,G}SBASE

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/12/16 13:18, Jan Beulich wrote:
>>>> On 14.12.16 at 13:36, <andrew.cooper3@xxxxxxxxxx> 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

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