Messages in this thread | | | Date | Wed, 9 Mar 2022 14:40:52 +0100 | Subject | Re: [PATCH v3 5/7] KVM: x86: nSVM: implement nested vGIF | From | 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; > +} > +
| |