On Wed, 2022-03-09 at 14:40 +0100, Paolo Bonzini wrote:
> 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.
This is a very good idea - I'll do this in 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 */
Good idea as well.
Thanks for the review!
Best regards,
Maxim Levitsky
>
> 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;
> > +}
> > +
