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;
+}
+
