[PATCH v2] x86/svm: Intercept CR0 writes selectively

[PATCH v2] x86/svm: Intercept CR0 writes selectively

[Ross Lagerwall]

Since 3356d685dbda ("x86/svm: Remove lazy FPU support"), Xen does not
need to track when the TS or MP bits change so opt to intercept CR0
writes selectively. Aside from potentially reducing a few VMEXITs, this
fixes a nested virt bug where L1 intercepts CR0_SEL_WRITE and L0
intercepts CR0_WRITE. The hardware prioritizes CR0_WRITE and so L1 never
sees any CR0 writes.

Since CR0 may now change behind Xen's back, sync it on VMEXIT so that
the emulator sees the correct value.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---

In v2:
* Keep case VMEXIT_CR0_WRITE for consistency with other not-intercepted
  CRx VMEXITs.
* Tweak commit message, comments, and formatting.

 xen/arch/x86/hvm/svm/svm.c  |  5 ++++-
 xen/arch/x86/hvm/svm/vmcb.c | 22 +++++++++++++---------
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 5f5d903d872d..ddc35e31506e 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1640,7 +1640,8 @@ static void svm_vmexit_do_cr_access(
 {
     int gp, cr, dir, rc;
 
-    cr = vmcb->exitcode - VMEXIT_CR0_READ;
+    cr = (vmcb->exitcode == VMEXIT_CR0_SEL_WRITE)
+         ? 16 : (vmcb->exitcode - VMEXIT_CR0_READ);
     dir = (cr > 15);
     cr &= 0xf;
     gp = vmcb->ei.mov_cr.gpr;
@@ -2517,6 +2518,7 @@ void asmlinkage svm_vmexit_handler(void)
     hvm_sanitize_regs_fields(
         regs, !(vmcb_get_efer(vmcb) & EFER_LMA) || !(vmcb->cs.l));
 
+    v->arch.hvm.guest_cr[0] = vmcb_get_cr0(vmcb);
     v->arch.hvm.guest_cr[2] = vmcb_get_cr2(vmcb);
     if ( paging_mode_hap(v->domain) )
         v->arch.hvm.guest_cr[3] = v->arch.hvm.hw_cr[3] = vmcb_get_cr3(vmcb);
@@ -2883,6 +2885,7 @@ void asmlinkage svm_vmexit_handler(void)
 
     case VMEXIT_CR0_READ ... VMEXIT_CR15_READ:
     case VMEXIT_CR0_WRITE ... VMEXIT_CR15_WRITE:
+    case VMEXIT_CR0_SEL_WRITE:
         if ( cpu_has_svm_decode && vmcb->ei.mov_cr.mov_insn )
             svm_vmexit_do_cr_access(vmcb, regs);
         else if ( !hvm_emulate_one_insn(x86_insn_is_cr_access, "CR access") )
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 975a1eaef806..f7e86c68b521 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -50,13 +50,13 @@ static int construct_vmcb(struct vcpu *v)
     struct vmcb_struct *vmcb = svm->vmcb;
 
     vmcb->_general1_intercepts =
-        GENERAL1_INTERCEPT_INTR        | GENERAL1_INTERCEPT_NMI         |
-        GENERAL1_INTERCEPT_SMI         | GENERAL1_INTERCEPT_INIT        |
-        GENERAL1_INTERCEPT_CPUID       | GENERAL1_INTERCEPT_INVD        |
-        GENERAL1_INTERCEPT_HLT         | GENERAL1_INTERCEPT_INVLPG      |
-        GENERAL1_INTERCEPT_INVLPGA     | GENERAL1_INTERCEPT_IOIO_PROT   |
-        GENERAL1_INTERCEPT_MSR_PROT    | GENERAL1_INTERCEPT_SHUTDOWN_EVT|
-        GENERAL1_INTERCEPT_TASK_SWITCH;
+        GENERAL1_INTERCEPT_INTR          | GENERAL1_INTERCEPT_NMI           |
+        GENERAL1_INTERCEPT_SMI           | GENERAL1_INTERCEPT_INIT          |
+        GENERAL1_INTERCEPT_CR0_SEL_WRITE | GENERAL1_INTERCEPT_CPUID         |
+        GENERAL1_INTERCEPT_INVD          | GENERAL1_INTERCEPT_HLT           |
+        GENERAL1_INTERCEPT_INVLPG        | GENERAL1_INTERCEPT_INVLPGA       |
+        GENERAL1_INTERCEPT_IOIO_PROT     | GENERAL1_INTERCEPT_MSR_PROT      |
+        GENERAL1_INTERCEPT_TASK_SWITCH   | GENERAL1_INTERCEPT_SHUTDOWN_EVT;
     vmcb->_general2_intercepts =
         GENERAL2_INTERCEPT_VMRUN       | GENERAL2_INTERCEPT_VMMCALL     |
         GENERAL2_INTERCEPT_VMLOAD      | GENERAL2_INTERCEPT_VMSAVE      |
@@ -76,8 +76,12 @@ static int construct_vmcb(struct vcpu *v)
     /* Intercept all debug-register writes. */
     vmcb->_dr_intercepts = ~0u;
 
-    /* Intercept all control-register accesses except for CR2 and CR8. */
-    vmcb->_cr_intercepts = ~(CR_INTERCEPT_CR2_READ |
+    /*
+     * Intercept all control-register accesses except for CR0 writes (use
+     * selective write instead), and CR2 and CR8 reads/writes.
+     */
+    vmcb->_cr_intercepts = ~(CR_INTERCEPT_CR0_WRITE |
+                             CR_INTERCEPT_CR2_READ |
                              CR_INTERCEPT_CR2_WRITE |
                              CR_INTERCEPT_CR8_READ |
                              CR_INTERCEPT_CR8_WRITE);
-- 
2.53.0

Re: [PATCH v2] x86/svm: Intercept CR0 writes selectively

[Jan Beulich]

On 04.09.2026 14:23, Ross Lagerwall wrote:
> Since 3356d685dbda ("x86/svm: Remove lazy FPU support"), Xen does not
> need to track when the TS or MP bits change so opt to intercept CR0
> writes selectively. Aside from potentially reducing a few VMEXITs, this
> fixes a nested virt bug where L1 intercepts CR0_SEL_WRITE and L0
> intercepts CR0_WRITE. The hardware prioritizes CR0_WRITE and so L1 never
> sees any CR0 writes.
> 
> Since CR0 may now change behind Xen's back, sync it on VMEXIT so that
> the emulator sees the correct value.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with two remarks (which I may take the liberty of carrying out while
committing):

> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -50,13 +50,13 @@ static int construct_vmcb(struct vcpu *v)
>      struct vmcb_struct *vmcb = svm->vmcb;
>  
>      vmcb->_general1_intercepts =
> -        GENERAL1_INTERCEPT_INTR        | GENERAL1_INTERCEPT_NMI         |
> -        GENERAL1_INTERCEPT_SMI         | GENERAL1_INTERCEPT_INIT        |
> -        GENERAL1_INTERCEPT_CPUID       | GENERAL1_INTERCEPT_INVD        |
> -        GENERAL1_INTERCEPT_HLT         | GENERAL1_INTERCEPT_INVLPG      |
> -        GENERAL1_INTERCEPT_INVLPGA     | GENERAL1_INTERCEPT_IOIO_PROT   |
> -        GENERAL1_INTERCEPT_MSR_PROT    | GENERAL1_INTERCEPT_SHUTDOWN_EVT|
> -        GENERAL1_INTERCEPT_TASK_SWITCH;
> +        GENERAL1_INTERCEPT_INTR          | GENERAL1_INTERCEPT_NMI           |
> +        GENERAL1_INTERCEPT_SMI           | GENERAL1_INTERCEPT_INIT          |
> +        GENERAL1_INTERCEPT_CR0_SEL_WRITE | GENERAL1_INTERCEPT_CPUID         |
> +        GENERAL1_INTERCEPT_INVD          | GENERAL1_INTERCEPT_HLT           |
> +        GENERAL1_INTERCEPT_INVLPG        | GENERAL1_INTERCEPT_INVLPGA       |
> +        GENERAL1_INTERCEPT_IOIO_PROT     | GENERAL1_INTERCEPT_MSR_PROT      |
> +        GENERAL1_INTERCEPT_TASK_SWITCH   | GENERAL1_INTERCEPT_SHUTDOWN_EVT;

I think it would be nice to avoid moving the |-s out, to keep ...

>      vmcb->_general2_intercepts =
>          GENERAL2_INTERCEPT_VMRUN       | GENERAL2_INTERCEPT_VMMCALL     |
>          GENERAL2_INTERCEPT_VMLOAD      | GENERAL2_INTERCEPT_VMSAVE      |

... aligning with the ones here.

> @@ -76,8 +76,12 @@ static int construct_vmcb(struct vcpu *v)
>      /* Intercept all debug-register writes. */
>      vmcb->_dr_intercepts = ~0u;
>  
> -    /* Intercept all control-register accesses except for CR2 and CR8. */
> -    vmcb->_cr_intercepts = ~(CR_INTERCEPT_CR2_READ |
> +    /*
> +     * Intercept all control-register accesses except for CR0 writes (use
> +     * selective write instead), and CR2 and CR8 reads/writes.
> +     */

Imo slightly more precise as "... (using selective write intercept instead) ..."

Jan

> +    vmcb->_cr_intercepts = ~(CR_INTERCEPT_CR0_WRITE |
> +                             CR_INTERCEPT_CR2_READ |
>                               CR_INTERCEPT_CR2_WRITE |
>                               CR_INTERCEPT_CR8_READ |
>                               CR_INTERCEPT_CR8_WRITE);

Re: [PATCH v2] x86/svm: Intercept CR0 writes selectively

[Jan Beulich]

On 04.09.2026 14:23, Ross Lagerwall wrote:
> @@ -2517,6 +2518,7 @@ void asmlinkage svm_vmexit_handler(void)
>      hvm_sanitize_regs_fields(
>          regs, !(vmcb_get_efer(vmcb) & EFER_LMA) || !(vmcb->cs.l));
>  
> +    v->arch.hvm.guest_cr[0] = vmcb_get_cr0(vmcb);
>      v->arch.hvm.guest_cr[2] = vmcb_get_cr2(vmcb);
>      if ( paging_mode_hap(v->domain) )
>          v->arch.hvm.guest_cr[3] = v->arch.hvm.hw_cr[3] = vmcb_get_cr3(vmcb);

gitlab-CI says no to this patch [1], and I think it's the change above which
gets in the way of running guests in shadow mode (as is the case for the two
qemu-smoke-x86_64-*-pvh tests). svm_update_guest_cr() has

        value = v->arch.hvm.guest_cr[0];
        if ( paging_mode_shadow(v->domain) )
            value |= X86_CR0_PG | X86_CR0_WP;
        vmcb_set_cr0(vmcb, value);

i.e. upon reading back the guest's CR0 value we would wrongly record it
having PG (and WP) enabled, at which point shadow code would try to use the
still-zero CR3 as page table base. I think values need merging here, with
only TS and MP taken from what vmcb_get_cr0() returns.

The zen2-* failure in [1] is, I think, unrelated.

Jan

[1] https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/2828369624

Re: [PATCH v2] x86/svm: Intercept CR0 writes selectively

[Andrew Cooper]

On 08/09/2026 9:44 am, Jan Beulich wrote:
> On 04.09.2026 14:23, Ross Lagerwall wrote:
>> @@ -2517,6 +2518,7 @@ void asmlinkage svm_vmexit_handler(void)
>>      hvm_sanitize_regs_fields(
>>          regs, !(vmcb_get_efer(vmcb) & EFER_LMA) || !(vmcb->cs.l));
>>  
>> +    v->arch.hvm.guest_cr[0] = vmcb_get_cr0(vmcb);
>>      v->arch.hvm.guest_cr[2] = vmcb_get_cr2(vmcb);
>>      if ( paging_mode_hap(v->domain) )
>>          v->arch.hvm.guest_cr[3] = v->arch.hvm.hw_cr[3] = vmcb_get_cr3(vmcb);
> gitlab-CI says no to this patch [1], and I think it's the change above which
> gets in the way of running guests in shadow mode (as is the case for the two
> qemu-smoke-x86_64-*-pvh tests). svm_update_guest_cr() has
>
>         value = v->arch.hvm.guest_cr[0];
>         if ( paging_mode_shadow(v->domain) )
>             value |= X86_CR0_PG | X86_CR0_WP;
>         vmcb_set_cr0(vmcb, value);
>
> i.e. upon reading back the guest's CR0 value we would wrongly record it
> having PG (and WP) enabled, at which point shadow code would try to use the
> still-zero CR3 as page table base. I think values need merging here, with
> only TS and MP taken from what vmcb_get_cr0() returns.

Yes this is broken for Shadow.

Shadow shouldn't be trying to use CR0_WRITE_SEL.  What's in the VMCB is
unrelated to the guest's choice of value, and reverse engineering it is
fragile.

Use full CR0_WRITE for Shadow guests, and use CR0_WRITE_SEL for HAP only.

~Andrew

Re: [PATCH v2] x86/svm: Intercept CR0 writes selectively

[Ross Lagerwall]

On 9/8/26 2:57 PM, Andrew Cooper wrote:
> On 08/09/2026 9:44 am, Jan Beulich wrote:
>> On 04.09.2026 14:23, Ross Lagerwall wrote:
>>> @@ -2517,6 +2518,7 @@ void asmlinkage svm_vmexit_handler(void)
>>>       hvm_sanitize_regs_fields(
>>>           regs, !(vmcb_get_efer(vmcb) & EFER_LMA) || !(vmcb->cs.l));
>>>   
>>> +    v->arch.hvm.guest_cr[0] = vmcb_get_cr0(vmcb);
>>>       v->arch.hvm.guest_cr[2] = vmcb_get_cr2(vmcb);
>>>       if ( paging_mode_hap(v->domain) )
>>>           v->arch.hvm.guest_cr[3] = v->arch.hvm.hw_cr[3] = vmcb_get_cr3(vmcb);
>> gitlab-CI says no to this patch [1], and I think it's the change above which
>> gets in the way of running guests in shadow mode (as is the case for the two
>> qemu-smoke-x86_64-*-pvh tests). svm_update_guest_cr() has
>>
>>          value = v->arch.hvm.guest_cr[0];
>>          if ( paging_mode_shadow(v->domain) )
>>              value |= X86_CR0_PG | X86_CR0_WP;
>>          vmcb_set_cr0(vmcb, value);
>>
>> i.e. upon reading back the guest's CR0 value we would wrongly record it
>> having PG (and WP) enabled, at which point shadow code would try to use the
>> still-zero CR3 as page table base. I think values need merging here, with
>> only TS and MP taken from what vmcb_get_cr0() returns.
> 
> Yes this is broken for Shadow.
> 
> Shadow shouldn't be trying to use CR0_WRITE_SEL.  What's in the VMCB is
> unrelated to the guest's choice of value, and reverse engineering it is
> fragile.
> 
> Use full CR0_WRITE for Shadow guests, and use CR0_WRITE_SEL for HAP only.

I did try Jan's suggestion which fixes the initial problem but crashes when
dom0 brings up its second vCPU.

If keeping Shadow guests using full CR0_WRITE is acceptable, I'll update the
patch to do that since there are surely better things to do than spending time
making Shadow improvements.

Ross