lkml.org 
[lkml]   [2022]   [Mar]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3 5/7] KVM: x86: nSVM: implement nested vGIF
From
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;
> +}
> +

\
 
 \ /
  Last update: 2022-03-09 14:41    [W:0.158 / U:0.332 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site