[PATCH] x86emul: support {RD,WR}{F,G}SBASE

[PATCH] x86emul: support {RD,WR}{F,G}SBASE

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1989 bytes --]

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




[-- Attachment #2: x86emul-fsgsbase.patch --]
[-- Type: text/plain, Size: 2021 bytes --]

x86emul: support {RD,WR}{F,G}SBASE

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

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

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

[Andrew Cooper]

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

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

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

[Jan Beulich]

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


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

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

[Andrew Cooper]

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

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

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

[Jan Beulich]

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


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

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

[Andrew Cooper]

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

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

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

[Jan Beulich]

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


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

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

[Andrew Cooper]

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

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