Re: [Xen-devel] [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.02.18 at 22:06, <brian.woods@xxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -601,6 +601,75 @@ void svm_update_guest_cr(struct vcpu *v, unsigned int cr)
>      }
>  }
>  
> +/*
> + * This runs on EFER change to see if nested features need to either be
> + * turned off or on.
> + */
> +static void svm_nested_features_on_efer_update(struct vcpu *v)

I'm afraid I continue to be confused: A function with this name should
imo, as said earlier, live in nestedsvm.c. However ...

> +{
> +    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> +    struct nestedsvm *svm = &vcpu_nestedsvm(v);
> +    u32 general2_intercepts;
> +    vintr_t vintr;
> +
> +    if ( !nestedhvm_enabled(v->domain) )
> +        ASSERT(!(v->arch.hvm_vcpu.guest_efer & EFER_SVME));

... this indicates that the function does something even for the
non-nested case. In particular ...

> +    /*
> +     * Need state for transfering the nested gif status so only write on
> +     * the hvm_vcpu EFER.SVME changing.
> +     */
> +    if ( v->arch.hvm_vcpu.guest_efer & EFER_SVME )
> +    {
> +        if ( !vmcb->virt_ext.fields.vloadsave_enable &&
> +             paging_mode_hap(v->domain) &&
> +             cpu_has_svm_vloadsave )
> +        {
> +            vmcb->virt_ext.fields.vloadsave_enable = 1;
> +            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
> +            general2_intercepts &= ~(GENERAL2_INTERCEPT_VMLOAD |
> +                                     GENERAL2_INTERCEPT_VMSAVE);
> +            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
> +        }
> +
> +        if ( !vmcb->_vintr.fields.vgif_enable &&
> +             cpu_has_svm_vgif )
> +        {
> +            vintr = vmcb_get_vintr(vmcb);
> +            vintr.fields.vgif = svm->ns_gif;
> +            vintr.fields.vgif_enable = 1;
> +            vmcb_set_vintr(vmcb, vintr);
> +            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
> +            general2_intercepts &= ~(GENERAL2_INTERCEPT_STGI |
> +                                     GENERAL2_INTERCEPT_CLGI);
> +            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
> +        }
> +    }
> +    else
> +    {
> +        if ( vmcb->virt_ext.fields.vloadsave_enable )
> +        {
> +            vmcb->virt_ext.fields.vloadsave_enable = 0;
> +            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
> +            general2_intercepts |= (GENERAL2_INTERCEPT_VMLOAD |
> +                                    GENERAL2_INTERCEPT_VMSAVE);
> +            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
> +        }
> +
> +        if ( vmcb->_vintr.fields.vgif_enable )
> +        {
> +            vintr = vmcb_get_vintr(vmcb);
> +            svm->ns_gif = vintr.fields.vgif;
> +            vintr.fields.vgif_enable = 0;
> +            vmcb_set_vintr(vmcb, vintr);
> +            general2_intercepts  = vmcb_get_general2_intercepts(vmcb);
> +            general2_intercepts |= (GENERAL2_INTERCEPT_STGI |
> +                                    GENERAL2_INTERCEPT_CLGI);
> +            vmcb_set_general2_intercepts(vmcb, general2_intercepts);
> +        }
> +    }

... this entire else block. Is it necessary to do this in the non-nested
case? IOW - do these settings ever change there (I would have
thought that the two *_enable fields checked by the two if()s should
never be true for nested-disabled guests)? Otherwise, as also said
before, the caller should call here only when
nestedhvm_enabled(v->domain), and the function would better
move.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel