xen/arch/x86/hvm/svm/svm.c | 5 ++++- xen/arch/x86/hvm/svm/vmcb.c | 22 +++++++++++++--------- 2 files changed, 17 insertions(+), 10 deletions(-)
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.0On 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
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
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
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);
© 2016 - 2026 Red Hat, Inc.