[PATCH] x86/nSVM: Save L2's CR4 on #VMEXIT, not Xen's

[PATCH] x86/nSVM: Save L2's CR4 on #VMEXIT, not Xen's

[Lin Liu]

Xen leaks the host's CR4 bits to L1.

nsvm_vmcb_prepare4vmrun() constructs the shadow VMCB's CR4 via
hvm_set_cr4(), and svm_update_guest_cr() ORs in HVM_CR4_HOST_MASK.
nsvm_vmcb_prepare4vmexit() then copies CR4 back out of the shadow VMCB
instead of the value kept in v->arch.hvm.guest_cr[4], so L1 reads back
Xen's bits - under HAP, CR4.MCE.

Fixes: 9a779e4fc161 ("Implement SVM specific part for Nested Virtualization")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Lin Liu <lin.liu01@citrix.com>
---
 xen/arch/x86/hvm/svm/nestedsvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index a8b15d6eae..8d99b0affc 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -1082,7 +1082,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
     ns_vmcb->_efer = n2vmcb->_efer;
 
     /* CRn */
-    ns_vmcb->_cr4 = n2vmcb->_cr4;
+    ns_vmcb->_cr4 = v->arch.hvm.guest_cr[4];
     ns_vmcb->_cr0 = n2vmcb->_cr0;
 
     /* DRn */
-- 
2.52.0

Re: [PATCH] x86/nSVM: Save L2's CR4 on #VMEXIT, not Xen's

[Ross Lagerwall]

On 9/23/26 10:51 AM, Lin Liu wrote:
> Xen leaks the host's CR4 bits to L1.
> 
> nsvm_vmcb_prepare4vmrun() constructs the shadow VMCB's CR4 via
> hvm_set_cr4(), and svm_update_guest_cr() ORs in HVM_CR4_HOST_MASK.
> nsvm_vmcb_prepare4vmexit() then copies CR4 back out of the shadow VMCB
> instead of the value kept in v->arch.hvm.guest_cr[4], so L1 reads back
> Xen's bits - under HAP, CR4.MCE.
> 
> Fixes: 9a779e4fc161 ("Implement SVM specific part for Nested Virtualization")
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Lin Liu <lin.liu01@citrix.com>
> ---
>   xen/arch/x86/hvm/svm/nestedsvm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
> index a8b15d6eae..8d99b0affc 100644
> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -1082,7 +1082,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>       ns_vmcb->_efer = n2vmcb->_efer;
>   
>       /* CRn */
> -    ns_vmcb->_cr4 = n2vmcb->_cr4;
> +    ns_vmcb->_cr4 = v->arch.hvm.guest_cr[4];
>       ns_vmcb->_cr0 = n2vmcb->_cr0;
>   
>       /* DRn */

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

Did you consider addressing similar issues with the other state copied from
n2vmcb as well? i.e. I think something similar would apply to CR0, EFER, etc.

As an aside, it is confusing that the same CR value also appears in
v->arch.hvm.nvcpu.guest_cr[4]. The SVM code doesn't seem to use it and I
haven't checked why the VMX code needs it. Perhaps something to clean up in
future.

Ross

Re: [PATCH] x86/nSVM: Save L2's CR4 on #VMEXIT, not Xen's

[Lin Liu]

Yeah, I think this is a good idea.
I do think of other similar fields as well, but prefer the one-liner patch
so it can be merged easly.  I will follow up the proposal soon in new patches.
Thanks for the nice help.

Lin

Re: [PATCH] x86/nSVM: Save L2's CR4 on #VMEXIT, not Xen's

[Jan Beulich]

On 23.09.2026 13:16, Ross Lagerwall wrote:
> On 9/23/26 10:51 AM, Lin Liu wrote:
>> Xen leaks the host's CR4 bits to L1.
>>
>> nsvm_vmcb_prepare4vmrun() constructs the shadow VMCB's CR4 via
>> hvm_set_cr4(), and svm_update_guest_cr() ORs in HVM_CR4_HOST_MASK.
>> nsvm_vmcb_prepare4vmexit() then copies CR4 back out of the shadow VMCB
>> instead of the value kept in v->arch.hvm.guest_cr[4], so L1 reads back
>> Xen's bits - under HAP, CR4.MCE.
>>
>> Fixes: 9a779e4fc161 ("Implement SVM specific part for Nested Virtualization")
>> Assisted-by: Claude:claude-opus-5
>> Signed-off-by: Lin Liu <lin.liu01@citrix.com>
>> ---
>>   xen/arch/x86/hvm/svm/nestedsvm.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
>> index a8b15d6eae..8d99b0affc 100644
>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>> @@ -1082,7 +1082,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>       ns_vmcb->_efer = n2vmcb->_efer;
>>   
>>       /* CRn */
>> -    ns_vmcb->_cr4 = n2vmcb->_cr4;
>> +    ns_vmcb->_cr4 = v->arch.hvm.guest_cr[4];
>>       ns_vmcb->_cr0 = n2vmcb->_cr0;
>>   
>>       /* DRn */
> 
> Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> Did you consider addressing similar issues with the other state copied from
> n2vmcb as well? i.e. I think something similar would apply to CR0, EFER, etc.

But (assuming the above code change is indeed correct) wouldn't we better deal
with CR0 then right away, rather that leaving things even visually inconsistent?

Jan

Re: [PATCH] x86/nSVM: Save L2's CR4 on #VMEXIT, not Xen's

[Jan Beulich]

On 23.09.2026 11:51, Lin Liu wrote:
> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -1082,7 +1082,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>      ns_vmcb->_efer = n2vmcb->_efer;
>  
>      /* CRn */
> -    ns_vmcb->_cr4 = n2vmcb->_cr4;
> +    ns_vmcb->_cr4 = v->arch.hvm.guest_cr[4];
>      ns_vmcb->_cr0 = n2vmcb->_cr0;

In addition to mirroring the change to CR0 and EFER, doesn't CR2 also
need handling the same way? Effectively the inverse direction of anything
respective that nsvm_vmcb_prepare4vmrun() does?

Jan

Re: [PATCH] x86/nSVM: Save L2's CR4 on #VMEXIT, not Xen's

[Ross Lagerwall]

On 9/24/26 4:01 PM, Jan Beulich wrote:
> On 23.09.2026 13:16, Ross Lagerwall wrote:
>> On 9/23/26 10:51 AM, Lin Liu wrote:
>>> Xen leaks the host's CR4 bits to L1.
>>>
>>> nsvm_vmcb_prepare4vmrun() constructs the shadow VMCB's CR4 via
>>> hvm_set_cr4(), and svm_update_guest_cr() ORs in HVM_CR4_HOST_MASK.
>>> nsvm_vmcb_prepare4vmexit() then copies CR4 back out of the shadow VMCB
>>> instead of the value kept in v->arch.hvm.guest_cr[4], so L1 reads back
>>> Xen's bits - under HAP, CR4.MCE.
>>>
>>> Fixes: 9a779e4fc161 ("Implement SVM specific part for Nested Virtualization")
>>> Assisted-by: Claude:claude-opus-5
>>> Signed-off-by: Lin Liu <lin.liu01@citrix.com>
>>> ---
>>>    xen/arch/x86/hvm/svm/nestedsvm.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
>>> index a8b15d6eae..8d99b0affc 100644
>>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>>> @@ -1082,7 +1082,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>>        ns_vmcb->_efer = n2vmcb->_efer;
>>>    
>>>        /* CRn */
>>> -    ns_vmcb->_cr4 = n2vmcb->_cr4;
>>> +    ns_vmcb->_cr4 = v->arch.hvm.guest_cr[4];
>>>        ns_vmcb->_cr0 = n2vmcb->_cr0;
>>>    
>>>        /* DRn */
>>
>> Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>>
>> Did you consider addressing similar issues with the other state copied from
>> n2vmcb as well? i.e. I think something similar would apply to CR0, EFER, etc.
> 
> But (assuming the above code change is indeed correct) wouldn't we better deal
> with CR0 then right away, rather that leaving things even visually inconsistent?

That's up to the maintainers to decide, though given the state of the Nested
SVM code at the moment IMO it is fine to take valid improvements and make some
forward progress even if they don't address all the related issues at once.

Ross

Re: [PATCH] x86/nSVM: Save L2's CR4 on #VMEXIT, not Xen's

[Jan Beulich]

On 24.09.2026 17:50, Ross Lagerwall wrote:
> On 9/24/26 4:01 PM, Jan Beulich wrote:
>> On 23.09.2026 13:16, Ross Lagerwall wrote:
>>> On 9/23/26 10:51 AM, Lin Liu wrote:
>>>> Xen leaks the host's CR4 bits to L1.
>>>>
>>>> nsvm_vmcb_prepare4vmrun() constructs the shadow VMCB's CR4 via
>>>> hvm_set_cr4(), and svm_update_guest_cr() ORs in HVM_CR4_HOST_MASK.
>>>> nsvm_vmcb_prepare4vmexit() then copies CR4 back out of the shadow VMCB
>>>> instead of the value kept in v->arch.hvm.guest_cr[4], so L1 reads back
>>>> Xen's bits - under HAP, CR4.MCE.
>>>>
>>>> Fixes: 9a779e4fc161 ("Implement SVM specific part for Nested Virtualization")
>>>> Assisted-by: Claude:claude-opus-5
>>>> Signed-off-by: Lin Liu <lin.liu01@citrix.com>
>>>> ---
>>>>    xen/arch/x86/hvm/svm/nestedsvm.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
>>>> index a8b15d6eae..8d99b0affc 100644
>>>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>>>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>>>> @@ -1082,7 +1082,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>>>        ns_vmcb->_efer = n2vmcb->_efer;
>>>>    
>>>>        /* CRn */
>>>> -    ns_vmcb->_cr4 = n2vmcb->_cr4;
>>>> +    ns_vmcb->_cr4 = v->arch.hvm.guest_cr[4];
>>>>        ns_vmcb->_cr0 = n2vmcb->_cr0;
>>>>    
>>>>        /* DRn */
>>>
>>> Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>>>
>>> Did you consider addressing similar issues with the other state copied from
>>> n2vmcb as well? i.e. I think something similar would apply to CR0, EFER, etc.
>>
>> But (assuming the above code change is indeed correct) wouldn't we better deal
>> with CR0 then right away, rather that leaving things even visually inconsistent?
> 
> That's up to the maintainers to decide, though given the state of the Nested
> SVM code at the moment IMO it is fine to take valid improvements and make some
> forward progress even if they don't address all the related issues at once.

Yet moving code into more inconsistent shape isn't a very good step, when things
are meant to be truly improved.

Jan

Re: [PATCH] x86/nSVM: Save L2's CR4 on #VMEXIT, not Xen's

[Lin Liu]

>> On 23.09.2026 11:51, Lin Liu wrote:
>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>> @@ -1082,7 +1082,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>      ns_vmcb->_efer = n2vmcb->_efer;
>>  
>>      /* CRn */
>> -    ns_vmcb->_cr4 = n2vmcb->_cr4;
>> +    ns_vmcb->_cr4 = v->arch.hvm.guest_cr[4];
>>      ns_vmcb->_cr0 = n2vmcb->_cr0;
>
>In addition to mirroring the change to CR0 and EFER, doesn't CR2 also
>need handling the same way? Effectively the inverse direction of anything
>respective that nsvm_vmcb_prepare4vmrun() does?
>
>Jan

CR2 is different, it store the page fault address on #PF.
If #PF happens, xen as hardware emulator, should write back the CR2 as part of
guest state area. so copy from n2vmcb is the right behavior.

I will raise v2 to update all other CRs and EFER.

Re: [PATCH] x86/nSVM: Save L2's CR4 on #VMEXIT, not Xen's

[Jan Beulich]

On 28.09.2026 11:26, Lin Liu wrote:
>>> On 23.09.2026 11:51, Lin Liu wrote:
>>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>>> @@ -1082,7 +1082,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>>      ns_vmcb->_efer = n2vmcb->_efer;
>>>  
>>>      /* CRn */
>>> -    ns_vmcb->_cr4 = n2vmcb->_cr4;
>>> +    ns_vmcb->_cr4 = v->arch.hvm.guest_cr[4];
>>>      ns_vmcb->_cr0 = n2vmcb->_cr0;
>>
>> In addition to mirroring the change to CR0 and EFER, doesn't CR2 also
>> need handling the same way? Effectively the inverse direction of anything
>> respective that nsvm_vmcb_prepare4vmrun() does?
> 
> CR2 is different, it store the page fault address on #PF.
> If #PF happens, xen as hardware emulator, should write back the CR2 as part of
> guest state area. so copy from n2vmcb is the right behavior.

But besides by #PF, CR2 may also be changed by the guest writing to it.

Jan