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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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).

>> +        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


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