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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/12/16 09:37, Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- 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

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