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

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > > ---
> > > CC: Jan Beulich <jbeulich@xxxxxxxx>
> > > CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > CC: Teddy Astie <teddy.astie@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>

Attachment: OpenPGP_0x660FA9D102CBCFD0.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature