Re: [PATCH v3 5/7] KVM: x86: nSVM: implement nested vGIF

[Paolo Bonzini]

The patch is good but I think it's possibly to rewrite some parts in an 
easier way.

On 3/1/22 15:36, Maxim Levitsky wrote:
> +	if (svm->vgif_enabled && (svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK))
> +		int_ctl_vmcb12_bits |= (V_GIF_MASK | V_GIF_ENABLE_MASK);
> +	else
> +		int_ctl_vmcb01_bits |= (V_GIF_MASK | V_GIF_ENABLE_MASK);

To remember for later: svm->vmcb's V_GIF_ENABLE_MASK is always the same 
as vgif:

- if it comes from vmcb12, it must be 1 (and then vgif is also 1)

- if it comes from vmcb01, it must be equal to vgif (because 
V_GIF_ENABLE_MASK is set in init_vmcb and never touched again).

>   
> +static bool nested_vgif_enabled(struct vcpu_svm *svm)
> +{
> +	if (!is_guest_mode(&svm->vcpu) || !svm->vgif_enabled)
> +		return false;
> +	return svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK;
> +}
> +
>   static inline bool vgif_enabled(struct vcpu_svm *svm)
>   {
> -	return !!(svm->vmcb->control.int_ctl & V_GIF_ENABLE_MASK);
> +	struct vmcb *vmcb = nested_vgif_enabled(svm) ? svm->vmcb01.ptr : svm->vmcb;
> +
> +	return !!(vmcb->control.int_ctl & V_GIF_ENABLE_MASK);
>   }
>   

Slight simplification:

- before the patch, vgif_enabled() is just "vgif", because 
V_GIF_ENABLE_MASK is set in init_vmcb and copied to vmcb02

- after the patch, vgif_enabled() is also just "vgif".  Outside guest 
mode the same reasoning applies.  If L2 has enabled vGIF,  vmcb01's 
V_GIF_ENABLE is equal to vgif per the previous bullet.  If L2 has not 
enabled vGIF, vmcb02's V_GIF_ENABLE uses svm->vmcb's int_ctl field which 
is always the same as vgif (see remark above).

You can make this simplification a separate patch.

>  static inline void enable_gif(struct vcpu_svm *svm)
>  {
> +	struct vmcb *vmcb = nested_vgif_enabled(svm) ? svm->vmcb01.ptr : svm->vmcb;
> +
>  	if (vgif_enabled(svm))
> -		svm->vmcb->control.int_ctl |= V_GIF_MASK;
> +		vmcb->control.int_ctl |= V_GIF_MASK;
>  	else
>  		svm->vcpu.arch.hflags |= HF_GIF_MASK;
>  }
>  
>  static inline void disable_gif(struct vcpu_svm *svm)
>  {
> +	struct vmcb *vmcb = nested_vgif_enabled(svm) ? svm->vmcb01.ptr : svm->vmcb;
> +
>  	if (vgif_enabled(svm))
> -		svm->vmcb->control.int_ctl &= ~V_GIF_MASK;
> +		vmcb->control.int_ctl &= ~V_GIF_MASK;
>  	else
>  		svm->vcpu.arch.hflags &= ~HF_GIF_MASK;
> +
>  }

Looks good.  For a little optimization however you could write

static inline struct vmcb *get_vgif_vmcb(struct vcpu_svm *svm)
{
	if (!vgif)
		return NULL;
	if (!is_guest_mode(&svm->vcpu)
		return svm->vmcb01.ptr;
	if ((svm->vgif_enabled && (svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK))
		return svm->vmcb01.ptr;
	else
		return svm->nested.vmcb02.ptr;
}

and then

	struct vmcb *vmcb = get_vgif_vmcb(svm);
	if (vmcb)
		/* use vmcb->control.int_ctl */
	else
		/* use hflags */

Paolo

>  
> +static bool nested_vgif_enabled(struct vcpu_svm *svm)
> +{
> +	if (!is_guest_mode(&svm->vcpu) || !svm->vgif_enabled)
> +		return false;
> +	return svm->nested.ctl.int_ctl & V_GIF_ENABLE_MASK;
> +}
> +