[v3] x86/emul: Adjust handling of CR8_LEGACY

[PATCH v3] x86/emul: Adjust handling of CR8_LEGACY

Posted by Andrew Cooper 1 week, 3 days ago

The APM description of the AltMovCR8 feature bit is:

  "LOCK MOV CR0 means MOV CR8"

Adjust the decode logic to behave like this.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Teddy Astie <teddy.astie@vates.tech>

v3:
 * Change yet again.  A contact with information on the matter confirmed that
   it is a special case for Reg = 0, and not a general modifier to all Reg
   values.
---
 xen/arch/x86/x86_emulate/decode.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/x86_emulate/decode.c b/xen/arch/x86/x86_emulate/decode.c
index 2c13356c4d17..57f6baffb066 100644
--- a/xen/arch/x86/x86_emulate/decode.c
+++ b/xen/arch/x86/x86_emulate/decode.c
@@ -780,12 +780,12 @@ decode_twobyte(struct x86_emulate_state *s,
         break;
 
     case 0x20: case 0x22: /* mov to/from cr */
-        if ( s->lock_prefix && vcpu_has_cr8_legacy() )
+        if ( s->lock_prefix && vcpu_has_cr8_legacy() && s->modrm_reg == 0 )
         {
-            s->modrm_reg += 8;
+            s->modrm_reg = 8;
             s->lock_prefix = false;
         }
-        /* fall through */
+        fallthrough;
     case 0x21: case 0x23: /* mov to/from dr */
         ASSERT(s->ea.type == OP_REG); /* Early operand adjustment ensures this. */
         generate_exception_if(s->lock_prefix, X86_EXC_UD);
-- 
2.39.5

Re: [PATCH v3] x86/emul: Adjust handling of CR8_LEGACY

Posted by Teddy Astie 1 week, 2 days ago

Le 07/07/2026 à 21:49, Andrew Cooper a écrit :
> The APM description of the AltMovCR8 feature bit is:
> 
>    "LOCK MOV CR0 means MOV CR8"
> 
> Adjust the decode logic to behave like this.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Teddy Astie <teddy.astie@vates.tech>
> 
> v3:
>   * Change yet again.  A contact with information on the matter confirmed that
>     it is a special case for Reg = 0, and not a general modifier to all Reg
>     values.
> ---
>   xen/arch/x86/x86_emulate/decode.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/x86_emulate/decode.c b/xen/arch/x86/x86_emulate/decode.c
> index 2c13356c4d17..57f6baffb066 100644
> --- a/xen/arch/x86/x86_emulate/decode.c
> +++ b/xen/arch/x86/x86_emulate/decode.c
> @@ -780,12 +780,12 @@ decode_twobyte(struct x86_emulate_state *s,
>           break;
>   
>       case 0x20: case 0x22: /* mov to/from cr */
> -        if ( s->lock_prefix && vcpu_has_cr8_legacy() )
> +        if ( s->lock_prefix && vcpu_has_cr8_legacy() && s->modrm_reg == 0 )
>           {
> -            s->modrm_reg += 8;
> +            s->modrm_reg = 8;
>               s->lock_prefix = false;
>           }
> -        /* fall through */
> +        fallthrough;
>       case 0x21: case 0x23: /* mov to/from dr */
>           ASSERT(s->ea.type == OP_REG); /* Early operand adjustment ensures this. */
>           generate_exception_if(s->lock_prefix, X86_EXC_UD);

I'm fine with it; though we probably want to make sure we always 
advertise the AltMovCr8 feature bit as APM suggest users to always check 
for this before relying on this behavior.

Interestingly, I see this feature bit set on Intel hardware.
Though, it's a reserved bit in SDM, and lock mov cr0 is defined as being 
#UD.

Which CPU model introduced this feature bit ? And does Intel behavior 
matches AMD's spec ?

Teddy

Re: [PATCH v3] x86/emul: Adjust handling of CR8_LEGACY

Posted by Andrew Cooper 1 week, 2 days ago

On 08/07/2026 10:29 am, Teddy Astie wrote:
> Le 07/07/2026 à 21:49, Andrew Cooper a écrit :
>> The APM description of the AltMovCR8 feature bit is:
>>
>>    "LOCK MOV CR0 means MOV CR8"
>>
>> Adjust the decode logic to behave like this.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <jbeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Teddy Astie <teddy.astie@vates.tech>
>>
>> v3:
>>   * Change yet again.  A contact with information on the matter
>> confirmed that
>>     it is a special case for Reg = 0, and not a general modifier to
>> all Reg
>>     values.
>> ---
>>   xen/arch/x86/x86_emulate/decode.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/x86/x86_emulate/decode.c
>> b/xen/arch/x86/x86_emulate/decode.c
>> index 2c13356c4d17..57f6baffb066 100644
>> --- a/xen/arch/x86/x86_emulate/decode.c
>> +++ b/xen/arch/x86/x86_emulate/decode.c
>> @@ -780,12 +780,12 @@ decode_twobyte(struct x86_emulate_state *s,
>>           break;
>>         case 0x20: case 0x22: /* mov to/from cr */
>> -        if ( s->lock_prefix && vcpu_has_cr8_legacy() )
>> +        if ( s->lock_prefix && vcpu_has_cr8_legacy() && s->modrm_reg
>> == 0 )
>>           {
>> -            s->modrm_reg += 8;
>> +            s->modrm_reg = 8;
>>               s->lock_prefix = false;
>>           }
>> -        /* fall through */
>> +        fallthrough;
>>       case 0x21: case 0x23: /* mov to/from dr */
>>           ASSERT(s->ea.type == OP_REG); /* Early operand adjustment
>> ensures this. */
>>           generate_exception_if(s->lock_prefix, X86_EXC_UD);
>
> I'm fine with it; though we probably want to make sure we always
> advertise the AltMovCr8 feature bit as APM suggest users to always
> check for this before relying on this behavior.
>
> Interestingly, I see this feature bit set on Intel hardware.

Are you mixing up the basic and extended feature leaves?  Intel does not
have this behaviour and doesn't (to my knowledge) advertise the CPUID bit.

> Though, it's a reserved bit in SDM, and lock mov cr0 is defined as
> being #UD.
>
> Which CPU model introduced this feature bit ? And does Intel behavior
> matches AMD's spec ?

That was in the v2 commit message, but I took it out here.

AMD K8 Rev F.  Prior K8 revisions didn't have it.

~Andrew

Re: [PATCH v3] x86/emul: Adjust handling of CR8_LEGACY

Posted by Teddy Astie 1 week, 2 days ago

Le 08/07/2026 à 11:57, Andrew Cooper a écrit :
> On 08/07/2026 10:29 am, Teddy Astie wrote:
>> Le 07/07/2026 à 21:49, Andrew Cooper a écrit :
>>> The APM description of the AltMovCR8 feature bit is:
>>>
>>>     "LOCK MOV CR0 means MOV CR8"
>>>
>>> Adjust the decode logic to behave like this.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <jbeulich@suse.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Teddy Astie <teddy.astie@vates.tech>
>>>
>>> v3:
>>>    * Change yet again.  A contact with information on the matter
>>> confirmed that
>>>      it is a special case for Reg = 0, and not a general modifier to
>>> all Reg
>>>      values.
>>> ---
>>>    xen/arch/x86/x86_emulate/decode.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/x86_emulate/decode.c
>>> b/xen/arch/x86/x86_emulate/decode.c
>>> index 2c13356c4d17..57f6baffb066 100644
>>> --- a/xen/arch/x86/x86_emulate/decode.c
>>> +++ b/xen/arch/x86/x86_emulate/decode.c
>>> @@ -780,12 +780,12 @@ decode_twobyte(struct x86_emulate_state *s,
>>>            break;
>>>          case 0x20: case 0x22: /* mov to/from cr */
>>> -        if ( s->lock_prefix && vcpu_has_cr8_legacy() )
>>> +        if ( s->lock_prefix && vcpu_has_cr8_legacy() && s->modrm_reg
>>> == 0 )
>>>            {
>>> -            s->modrm_reg += 8;
>>> +            s->modrm_reg = 8;
>>>                s->lock_prefix = false;
>>>            }
>>> -        /* fall through */
>>> +        fallthrough;
>>>        case 0x21: case 0x23: /* mov to/from dr */
>>>            ASSERT(s->ea.type == OP_REG); /* Early operand adjustment
>>> ensures this. */
>>>            generate_exception_if(s->lock_prefix, X86_EXC_UD);
>>
>> I'm fine with it; though we probably want to make sure we always
>> advertise the AltMovCr8 feature bit as APM suggest users to always
>> check for this before relying on this behavior.
>>
>> Interestingly, I see this feature bit set on Intel hardware.
> 
> Are you mixing up the basic and extended feature leaves?  Intel does not
> have this behaviour and doesn't (to my knowledge) advertise the CPUID bit.
> 

I was reading as "decimal" instead of "hexadecimal", hence things got 
shifted a bit. Looks like I wasn't fully woken up this morning.

>> Though, it's a reserved bit in SDM, and lock mov cr0 is defined as
>> being #UD.
>>
>> Which CPU model introduced this feature bit ? And does Intel behavior
>> matches AMD's spec ?
> 
> That was in the v2 commit message, but I took it out here.
> 
> AMD K8 Rev F.  Prior K8 revisions didn't have it.
> 

Ok thanks.

> ~Andrew


That looks good to me. As a follow-up patch, it might be worth renaming 
cr8_legacy into alt_mov_cr8 to better match APM terminology.

Reviewed-by: Teddy Astie <teddy.astie@vates.tech>

Re: [PATCH v3] x86/emul: Adjust handling of CR8_LEGACY

Posted by Jan Beulich 1 week, 2 days ago

On 08.07.2026 11:29, Teddy Astie wrote:
> Interestingly, I see this feature bit set on Intel hardware.
> Though, it's a reserved bit in SDM, and lock mov cr0 is defined as being 
> #UD.

On what particular piece of Intel hardware do you see the bit? I just looked
over all of the Intel systems I have locally available, and none of them has
the bit set. (The neighboring bit, ABM, is set on many of them.)

Jan

Re: [PATCH v3] x86/emul: Adjust handling of CR8_LEGACY

Posted by Teddy Astie 1 week, 2 days ago

Le 08/07/2026 à 11:41, Jan Beulich a écrit :
> On 08.07.2026 11:29, Teddy Astie wrote:
>> Interestingly, I see this feature bit set on Intel hardware.
>> Though, it's a reserved bit in SDM, and lock mov cr0 is defined as being
>> #UD.
> 
> On what particular piece of Intel hardware do you see the bit? I just looked
> over all of the Intel systems I have locally available, and none of them has
> the bit set. (The neighboring bit, ABM, is set on many of them.)
> 

Ah right, I was reading the thing wrongly; indeed, bit 4 is cleared on 
Intel.

> Jan

Teddy

Re: [PATCH v3] x86/emul: Adjust handling of CR8_LEGACY

Posted by Jan Beulich 1 week, 2 days ago

On 08.07.2026 11:29, Teddy Astie wrote:
> Le 07/07/2026 à 21:49, Andrew Cooper a écrit :
>> --- a/xen/arch/x86/x86_emulate/decode.c
>> +++ b/xen/arch/x86/x86_emulate/decode.c
>> @@ -780,12 +780,12 @@ decode_twobyte(struct x86_emulate_state *s,
>>           break;
>>   
>>       case 0x20: case 0x22: /* mov to/from cr */
>> -        if ( s->lock_prefix && vcpu_has_cr8_legacy() )
>> +        if ( s->lock_prefix && vcpu_has_cr8_legacy() && s->modrm_reg == 0 )
>>           {
>> -            s->modrm_reg += 8;
>> +            s->modrm_reg = 8;
>>               s->lock_prefix = false;
>>           }
>> -        /* fall through */
>> +        fallthrough;
>>       case 0x21: case 0x23: /* mov to/from dr */
>>           ASSERT(s->ea.type == OP_REG); /* Early operand adjustment ensures this. */
>>           generate_exception_if(s->lock_prefix, X86_EXC_UD);
> 
> I'm fine with it; though we probably want to make sure we always 
> advertise the AltMovCr8 feature bit as APM suggest users to always check 
> for this before relying on this behavior.

Why would we set the flag when hardware doesn't have it? Note the
vcpu_has_cr8_legacy() check there - we only engage this behavior if
the guest sees the feature as available.

Jan

Re: [PATCH v3] x86/emul: Adjust handling of CR8_LEGACY

Posted by Teddy Astie 1 week, 2 days ago

Le 08/07/2026 à 11:37, Jan Beulich a écrit :
> On 08.07.2026 11:29, Teddy Astie wrote:
>> Le 07/07/2026 à 21:49, Andrew Cooper a écrit :
>>> --- a/xen/arch/x86/x86_emulate/decode.c
>>> +++ b/xen/arch/x86/x86_emulate/decode.c
>>> @@ -780,12 +780,12 @@ decode_twobyte(struct x86_emulate_state *s,
>>>            break;
>>>    
>>>        case 0x20: case 0x22: /* mov to/from cr */
>>> -        if ( s->lock_prefix && vcpu_has_cr8_legacy() )
>>> +        if ( s->lock_prefix && vcpu_has_cr8_legacy() && s->modrm_reg == 0 )
>>>            {
>>> -            s->modrm_reg += 8;
>>> +            s->modrm_reg = 8;
>>>                s->lock_prefix = false;
>>>            }
>>> -        /* fall through */
>>> +        fallthrough;
>>>        case 0x21: case 0x23: /* mov to/from dr */
>>>            ASSERT(s->ea.type == OP_REG); /* Early operand adjustment ensures this. */
>>>            generate_exception_if(s->lock_prefix, X86_EXC_UD);
>>
>> I'm fine with it; though we probably want to make sure we always
>> advertise the AltMovCr8 feature bit as APM suggest users to always check
>> for this before relying on this behavior.
> 
> Why would we set the flag when hardware doesn't have it? Note the
> vcpu_has_cr8_legacy() check there - we only engage this behavior if
> the guest sees the feature as available.
> 

Hmm, ok, though this bit is named as "AltMovCr8" in July 2025 APM; but 
we name it "cr8_legacy". Has the name of the feature bit changed in APM 
over time ?

> Jan

Teddy

Re: [PATCH v3] x86/emul: Adjust handling of CR8_LEGACY

Posted by Jan Beulich 1 week, 2 days ago

On 08.07.2026 11:59, Teddy Astie wrote:
> Le 08/07/2026 à 11:37, Jan Beulich a écrit :
>> On 08.07.2026 11:29, Teddy Astie wrote:
>>> Le 07/07/2026 à 21:49, Andrew Cooper a écrit :
>>>> --- a/xen/arch/x86/x86_emulate/decode.c
>>>> +++ b/xen/arch/x86/x86_emulate/decode.c
>>>> @@ -780,12 +780,12 @@ decode_twobyte(struct x86_emulate_state *s,
>>>>            break;
>>>>    
>>>>        case 0x20: case 0x22: /* mov to/from cr */
>>>> -        if ( s->lock_prefix && vcpu_has_cr8_legacy() )
>>>> +        if ( s->lock_prefix && vcpu_has_cr8_legacy() && s->modrm_reg == 0 )
>>>>            {
>>>> -            s->modrm_reg += 8;
>>>> +            s->modrm_reg = 8;
>>>>                s->lock_prefix = false;
>>>>            }
>>>> -        /* fall through */
>>>> +        fallthrough;
>>>>        case 0x21: case 0x23: /* mov to/from dr */
>>>>            ASSERT(s->ea.type == OP_REG); /* Early operand adjustment ensures this. */
>>>>            generate_exception_if(s->lock_prefix, X86_EXC_UD);
>>>
>>> I'm fine with it; though we probably want to make sure we always
>>> advertise the AltMovCr8 feature bit as APM suggest users to always check
>>> for this before relying on this behavior.
>>
>> Why would we set the flag when hardware doesn't have it? Note the
>> vcpu_has_cr8_legacy() check there - we only engage this behavior if
>> the guest sees the feature as available.
> 
> Hmm, ok, though this bit is named as "AltMovCr8" in July 2025 APM; but 
> we name it "cr8_legacy". Has the name of the feature bit changed in APM 
> over time ?

I don't know. What I do know is that imo AltMovCr8 isn't the greatest of
all possible names. (Neither is cr8_legacy, first and foremost as it
alludes to this being a compat-mode-only thing when it isn't.)

Jan

Re: [PATCH v3] x86/emul: Adjust handling of CR8_LEGACY

Posted by Andrew Cooper 1 week, 2 days ago

On 08/07/2026 11:24 am, Jan Beulich wrote:
> On 08.07.2026 11:59, Teddy Astie wrote:
>> Le 08/07/2026 à 11:37, Jan Beulich a écrit :
>>> On 08.07.2026 11:29, Teddy Astie wrote:
>>>> Le 07/07/2026 à 21:49, Andrew Cooper a écrit :
>>>>> --- a/xen/arch/x86/x86_emulate/decode.c
>>>>> +++ b/xen/arch/x86/x86_emulate/decode.c
>>>>> @@ -780,12 +780,12 @@ decode_twobyte(struct x86_emulate_state *s,
>>>>>            break;
>>>>>    
>>>>>        case 0x20: case 0x22: /* mov to/from cr */
>>>>> -        if ( s->lock_prefix && vcpu_has_cr8_legacy() )
>>>>> +        if ( s->lock_prefix && vcpu_has_cr8_legacy() && s->modrm_reg == 0 )
>>>>>            {
>>>>> -            s->modrm_reg += 8;
>>>>> +            s->modrm_reg = 8;
>>>>>                s->lock_prefix = false;
>>>>>            }
>>>>> -        /* fall through */
>>>>> +        fallthrough;
>>>>>        case 0x21: case 0x23: /* mov to/from dr */
>>>>>            ASSERT(s->ea.type == OP_REG); /* Early operand adjustment ensures this. */
>>>>>            generate_exception_if(s->lock_prefix, X86_EXC_UD);
>>>> I'm fine with it; though we probably want to make sure we always
>>>> advertise the AltMovCr8 feature bit as APM suggest users to always check
>>>> for this before relying on this behavior.
>>> Why would we set the flag when hardware doesn't have it? Note the
>>> vcpu_has_cr8_legacy() check there - we only engage this behavior if
>>> the guest sees the feature as available.
>> Hmm, ok, though this bit is named as "AltMovCr8" in July 2025 APM; but 
>> we name it "cr8_legacy". Has the name of the feature bit changed in APM 
>> over time ?
> I don't know. What I do know is that imo AltMovCr8 isn't the greatest of
> all possible names. (Neither is cr8_legacy, first and foremost as it
> alludes to this being a compat-mode-only thing when it isn't.)

We inherited the name CR8_LEGACY from Linux.

commit c458f45d2eddcafed054651363ffb5c830e5df5b
Author: Keir Fraser <keir@xen.org>
Date:   Sat Jan 8 10:48:09 2011
    libxc: Update AMD CPU feature flags 0x80000001:ECX for Xen tools
    
    This patch syncs-up AMD CPU feature flags 0x80000001:ECX in libxc with
    the latest Linux kernel.
    
    Signed-off-by: Wei Huang <wei.huang2@amd.com>


renamed from ALTMOVCR to CR8_LEGACY

~Andrew

Re: [PATCH v3] x86/emul: Adjust handling of CR8_LEGACY

Posted by Jan Beulich 1 week, 2 days ago

On 07.07.2026 21:49, Andrew Cooper wrote:
> The APM description of the AltMovCR8 feature bit is:
> 
>   "LOCK MOV CR0 means MOV CR8"
> 
> Adjust the decode logic to behave like this.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
on the basis that ...

> v3:
>  * Change yet again.  A contact with information on the matter confirmed that
>    it is a special case for Reg = 0, and not a general modifier to all Reg
>    values.

... we simply have to trust this source, short of this being properly described
in the APM.

Jan