Re: [Xen-devel] [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD
On Tue, Feb 20, 2018 at 05:09:24PM -0500, Boris Ostrovsky wrote: > That's possibly because you needed an SVM maintainer ACK. > > I think Jan was waiting for decision on how to present the ASSERT. From > the 3 options I slightly more prefer > > ASSERT(nestedhvm_enabled(v->domain) || !(v->arch.hvm_vcpu.guest_efer & > EFER_SVME)); > > (which wasn't the first choice for either of you ;-)) > > -boris I'll change it and send out a new version then. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD
On 02/20/2018 05:00 PM, Brian Woods wrote: > I've seen patch 1 and 3 are in but this one isn't. Any status on it? > That's possibly because you needed an SVM maintainer ACK. I think Jan was waiting for decision on how to present the ASSERT. From the 3 options I slightly more prefer ASSERT(nestedhvm_enabled(v->domain) || !(v->arch.hvm_vcpu.guest_efer & EFER_SVME)); (which wasn't the first choice for either of you ;-)) -boris ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD
I've seen patch 1 and 3 are in but this one isn't. Any status on it? -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD
On Thu, Feb 08, 2018 at 02:45:31AM -0700, Jan Beulich wrote: > I'm afraid I continue to be confused: A function with this name should > imo, as said earlier, live in nestedsvm.c. However ... I'll move it to nestedsvm.c then. > ... this indicates that the function does something even for the > non-nested case. In particular ... It makes sure that SVME isn't set when nested isn't enabled. If SVME is set it does certain checks to enable features if enabled. Else it makes sure nested features are turned off. The reason for this is that, with VGIF/VVMLOAD, they can still work even with SVME not being set. This sets it where they get intercepted to Xen so that Xen can correctly emulate what should be happening. If SVME isn't set then nested guest shouldn't be able to do VGIF/VVMLOAD. > ... 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 It only checks two things (two if statements) in the VMCB per EFER update. Suppose you add an if to check if it's a nested guest and then run the nested_features func. You're only saving a total of one if and going in and out a function but you add a small divergence in the codepath. If this was a long computer/IO intense function, I'd completely agree but this is two very simple checks. I'll change it though, since it'll be easier than going back and forth about a minor detail. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD
>>> On 07.02.18 at 22:06, 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/3] x86/svm: add EFER SVME support for VGIF/VLOAD
If Andy wants to use his version of this, that's fine also. This is just a newer version based on Jan's input. v1 -> v2 Got rid of "== X"s Added an assert and got rid of a check in an if -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
