If L1 executes VMRUN with the GIF set and it fails consistency checks,
Xen will inject a VMEXIT and fail the assert checking the GIF is cleared.

Instead, clear the GIF when injecting a VMEXIT to match what hardware
does.

Fixes: 9a779e4fc161 ("Implement SVM specific part for Nested Virtualization")
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/hvm/svm/nestedsvm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index ef6fa5d23b67..f89b087a1155 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -733,9 +733,9 @@ nsvm_vcpu_vmexit_inject(struct vcpu *v, struct cpu_user_regs *regs,
     struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
 
     if ( vmcb->_vintr.fields.vgif_enable )
-        ASSERT(vmcb->_vintr.fields.vgif == 0);
+        vmcb->_vintr.fields.vgif = 0;
     else
-        ASSERT(svm->ns_gif == 0);
+        nestedsvm_vcpu_clgi(v);
 
     ns_vmcb = nv->nv_vvmcx;
 
-- 
2.53.0

On 23/04/2026 5:11 pm, Ross Lagerwall wrote:
> If L1 executes VMRUN with the GIF set and it fails consistency checks,
> Xen will inject a VMEXIT and fail the assert checking the GIF is cleared.
>
> Instead, clear the GIF when injecting a VMEXIT to match what hardware
> does.
>
> Fixes: 9a779e4fc161 ("Implement SVM specific part for Nested Virtualization")
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  xen/arch/x86/hvm/svm/nestedsvm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
> index ef6fa5d23b67..f89b087a1155 100644
> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -733,9 +733,9 @@ nsvm_vcpu_vmexit_inject(struct vcpu *v, struct cpu_user_regs *regs,
>      struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
>  
>      if ( vmcb->_vintr.fields.vgif_enable )
> -        ASSERT(vmcb->_vintr.fields.vgif == 0);
> +        vmcb->_vintr.fields.vgif = 0;
>      else
> -        ASSERT(svm->ns_gif == 0);
> +        nestedsvm_vcpu_clgi(v);
>  
>      ns_vmcb = nv->nv_vvmcx;
>  

I agree this is a bug.

It is common for GIF to be clear when executing VMRUN but it is not
required.  The pseudocode says that VMRUN simply sets GIF, and #VMEXIT
simply clears GIF.

But, looking at nestedsvm_vcpu_clgi(), it's not the only bug here. 
nestedsvm_vcpu_{clgi,stgi}() have additional calls to
local_event_delivery_{disable,enable}().

vgif_enable is strictly an optimisation that allows hardware to manage
the vGIF bit, so at an absolute minimum this ought to be:

    if ( ->vgif_enable )
        ->vgif = 0;
    else
        ->ns_gif = 0;
    local_event_delivery_disable();

to have balanced semantics.  Except that messing with event channels
can't be done when hardware is handling the vgif bit.

In fact, I think it's plain buggy to be touching event channels in
response to GIF changes.  In PV guests  vcpu_info->evtchn_upcall_mask is
the singular mask bit, but this is not true in HVM guests.  Even with
direct vector injection, HVM guests are still subject to normal rules
about INTR delivery, and that CLI blocks them.  This should naturally
extend to GIF for nested-SVM too.

As far as testing goes, perhaps we want to start with something even
more simple.  For real self-IPIs, and self-evtchns (for all reasonable
evtchn delivery configurations), check that delivery is blocked by IF=0
|| GIF=0.

~Andrew

Le 23/04/2026 à 18:13, Ross Lagerwall a écrit :
> If L1 executes VMRUN with the GIF set and it fails consistency checks,
> Xen will inject a VMEXIT and fail the assert checking the GIF is cleared.
> 
> Instead, clear the GIF when injecting a VMEXIT to match what hardware
> does.
> 
> Fixes: 9a779e4fc161 ("Implement SVM specific part for Nested Virtualization")
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>   xen/arch/x86/hvm/svm/nestedsvm.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
> index ef6fa5d23b67..f89b087a1155 100644
> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -733,9 +733,9 @@ nsvm_vcpu_vmexit_inject(struct vcpu *v, struct cpu_user_regs *regs,
>       struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
>   
>       if ( vmcb->_vintr.fields.vgif_enable )
> -        ASSERT(vmcb->_vintr.fields.vgif == 0);
> +        vmcb->_vintr.fields.vgif = 0;
>       else
> -        ASSERT(svm->ns_gif == 0);
> +        nestedsvm_vcpu_clgi(v);
>   
>       ns_vmcb = nv->nv_vvmcx;
>   

Looks good to me, though I think we are here looking to make a "guest 
CLGI" (clear GIF), so the vGIF specific logic should be collapsed into 
nestedsvm_vcpu_clgi() instead of having it as the non-vgif-support case.
(as IIUC, vGIF is a hardware accelration for nested GIF handling ?)

(also making me notice that svm_vmexit_do_{stgi,clgi}() seems to lack 
vGIF specific logic)

Teddy


-- 
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

On 4/29/26 5:54 PM, Teddy Astie wrote:
> Le 23/04/2026 à 18:13, Ross Lagerwall a écrit :
>> If L1 executes VMRUN with the GIF set and it fails consistency checks,
>> Xen will inject a VMEXIT and fail the assert checking the GIF is cleared.
>>
>> Instead, clear the GIF when injecting a VMEXIT to match what hardware
>> does.
>>
>> Fixes: 9a779e4fc161 ("Implement SVM specific part for Nested Virtualization")
>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>> ---
>>    xen/arch/x86/hvm/svm/nestedsvm.c | 4 ++--
>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
>> index ef6fa5d23b67..f89b087a1155 100644
>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>> @@ -733,9 +733,9 @@ nsvm_vcpu_vmexit_inject(struct vcpu *v, struct cpu_user_regs *regs,
>>        struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
>>    
>>        if ( vmcb->_vintr.fields.vgif_enable )
>> -        ASSERT(vmcb->_vintr.fields.vgif == 0);
>> +        vmcb->_vintr.fields.vgif = 0;
>>        else
>> -        ASSERT(svm->ns_gif == 0);
>> +        nestedsvm_vcpu_clgi(v);
>>    
>>        ns_vmcb = nv->nv_vvmcx;
>>    
> 
> Looks good to me, though I think we are here looking to make a "guest
> CLGI" (clear GIF), so the vGIF specific logic should be collapsed into
> nestedsvm_vcpu_clgi() instead of having it as the non-vgif-support case.
> (as IIUC, vGIF is a hardware accelration for nested GIF handling ?)

Not 100% sure I follow your point here but v2 of this series removes
nestedsvm_vcpu_clgi() entirely. Does that address your concern?

> 
> (also making me notice that svm_vmexit_do_{stgi,clgi}() seems to lack
> vGIF specific logic)
> 

Isn't that by design? If vGIF is enabled the hardware should handle
STGI/CLGI without a VMEXIT.

Ross

Le 07/05/2026 à 15:03, Ross Lagerwall a écrit :
> On 4/29/26 5:54 PM, Teddy Astie wrote:
>> Le 23/04/2026 à 18:13, Ross Lagerwall a écrit :
>>> If L1 executes VMRUN with the GIF set and it fails consistency checks,
>>> Xen will inject a VMEXIT and fail the assert checking the GIF is 
>>> cleared.
>>>
>>> Instead, clear the GIF when injecting a VMEXIT to match what hardware
>>> does.
>>>
>>> Fixes: 9a779e4fc161 ("Implement SVM specific part for Nested 
>>> Virtualization")
>>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>>> ---
>>>    xen/arch/x86/hvm/svm/nestedsvm.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/ 
>>> nestedsvm.c
>>> index ef6fa5d23b67..f89b087a1155 100644
>>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>>> @@ -733,9 +733,9 @@ nsvm_vcpu_vmexit_inject(struct vcpu *v, struct 
>>> cpu_user_regs *regs,
>>>        struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
>>>        if ( vmcb->_vintr.fields.vgif_enable )
>>> -        ASSERT(vmcb->_vintr.fields.vgif == 0);
>>> +        vmcb->_vintr.fields.vgif = 0;
>>>        else
>>> -        ASSERT(svm->ns_gif == 0);
>>> +        nestedsvm_vcpu_clgi(v);
>>>        ns_vmcb = nv->nv_vvmcx;
>>
>> Looks good to me, though I think we are here looking to make a "guest
>> CLGI" (clear GIF), so the vGIF specific logic should be collapsed into
>> nestedsvm_vcpu_clgi() instead of having it as the non-vgif-support case.
>> (as IIUC, vGIF is a hardware accelration for nested GIF handling ?)
> 
> Not 100% sure I follow your point here but v2 of this series removes
> nestedsvm_vcpu_clgi() entirely. Does that address your concern?
> 
>>
>> (also making me notice that svm_vmexit_do_{stgi,clgi}() seems to lack
>> vGIF specific logic)
>>
> 
> Isn't that by design? If vGIF is enabled the hardware should handle
> STGI/CLGI without a VMEXIT.
> 

I think at some point in the future, we may need a 
`nestedsvm_vcpu_{stgi,clgi}()` function, or something in that regard; 
e.g for emulating this instruction outside of a SVM intercept path (x86 
emulator bits ?). As I think there are cases where the instruction is 
not intercepted but still needs to be emulated.

Such function would need to know whether or not we use vGIF.

> Ross
> 

Teddy

On 11/05/2026 3:35 pm, Teddy Astie wrote:
> Le 07/05/2026 à 15:03, Ross Lagerwall a écrit :
>> On 4/29/26 5:54 PM, Teddy Astie wrote:
>>> Le 23/04/2026 à 18:13, Ross Lagerwall a écrit :
>>>> If L1 executes VMRUN with the GIF set and it fails consistency checks,
>>>> Xen will inject a VMEXIT and fail the assert checking the GIF is
>>>> cleared.
>>>>
>>>> Instead, clear the GIF when injecting a VMEXIT to match what hardware
>>>> does.
>>>>
>>>> Fixes: 9a779e4fc161 ("Implement SVM specific part for Nested
>>>> Virtualization")
>>>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>>>> ---
>>>>    xen/arch/x86/hvm/svm/nestedsvm.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c
>>>> b/xen/arch/x86/hvm/svm/ nestedsvm.c
>>>> index ef6fa5d23b67..f89b087a1155 100644
>>>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>>>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>>>> @@ -733,9 +733,9 @@ nsvm_vcpu_vmexit_inject(struct vcpu *v, struct
>>>> cpu_user_regs *regs,
>>>>        struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
>>>>        if ( vmcb->_vintr.fields.vgif_enable )
>>>> -        ASSERT(vmcb->_vintr.fields.vgif == 0);
>>>> +        vmcb->_vintr.fields.vgif = 0;
>>>>        else
>>>> -        ASSERT(svm->ns_gif == 0);
>>>> +        nestedsvm_vcpu_clgi(v);
>>>>        ns_vmcb = nv->nv_vvmcx;
>>>
>>> Looks good to me, though I think we are here looking to make a "guest
>>> CLGI" (clear GIF), so the vGIF specific logic should be collapsed into
>>> nestedsvm_vcpu_clgi() instead of having it as the non-vgif-support
>>> case.
>>> (as IIUC, vGIF is a hardware accelration for nested GIF handling ?)
>>
>> Not 100% sure I follow your point here but v2 of this series removes
>> nestedsvm_vcpu_clgi() entirely. Does that address your concern?
>>
>>>
>>> (also making me notice that svm_vmexit_do_{stgi,clgi}() seems to lack
>>> vGIF specific logic)
>>>
>>
>> Isn't that by design? If vGIF is enabled the hardware should handle
>> STGI/CLGI without a VMEXIT.
>>
>
> I think at some point in the future, we may need a
> `nestedsvm_vcpu_{stgi,clgi}()` function, or something in that regard;
> e.g for emulating this instruction outside of a SVM intercept path
> (x86 emulator bits ?). As I think there are cases where the
> instruction is not intercepted but still needs to be emulated.
>
> Such function would need to know whether or not we use vGIF.

Yes we will (eventually) want emulation of STGI/CLGI.  It will
specifically not reintroduce nestedsvm_vcpu_{stgi,clgi}().

With introspection, we do get arbitrary instructions running through the
emulator.  This is why we take care to ensure that the emulator can cope
before a feature gets enabled generally.

For normal cases, we either have vGIF active and everything is handled
without VMExit, or we don't have vGIF active and everything hits the
intercept; this is not a split based on hardware support - it includes
both L0 and L1's configuration.

In fact, vGIF is a feature where on older hardware it's actually faster
for L0 to emulate it for L1, because a virtual VMExit/Entry and running
the L1 vmexit handler is much slower than L0 just emulating it and
re-entering L2 as if nothing had happened.  But that's an optimisation
for a later day.

~Andrew