* [PATCH] vvmx: fixes after CR4 trapping optimizations
@ 2018-03-01 16:19 Roger Pau Monne
2018-03-02 14:29 ` Sergey Dyasli
0 siblings, 1 reply; 3+ messages in thread
From: Roger Pau Monne @ 2018-03-01 16:19 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, Kevin Tian, Jan Beulich, Jun Nakajima,
Roger Pau Monne
Commit 406817 doesn't update nested VMX code in order to take into
account L1 CR4 host mask when nested guest (L2) writes to CR4, and
thus the mask written to CR4_GUEST_HOST_MASK is likely not as
restrictive as it should be.
Also the VVMCS GUEST_CR4 value should be updated to match the
underlying value when syncing the VVMCS state.
Fixes: 406817 ("vmx/hap: optimize CR4 trapping")
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
I've manually tested and AFAICT this fixes the osstest failure
detected in 120076 ("test-amd64-amd64-qemuu-nested-intel").
---
xen/arch/x86/hvm/vmx/vmx.c | 4 ++++
xen/arch/x86/hvm/vmx/vvmx.c | 13 ++++++++++++-
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 5cee364b0c..18d8ce2303 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1617,6 +1617,10 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr,
v->arch.hvm_vmx.cr4_host_mask |=
~v->domain->arch.monitor.write_ctrlreg_mask[VM_EVENT_X86_CR4];
+ if ( nestedhvm_vcpu_in_guestmode(v) )
+ /* Add the nested host mask to get the more restrictive one. */
+ v->arch.hvm_vmx.cr4_host_mask |= get_vvmcs(v,
+ CR4_GUEST_HOST_MASK);
}
__vmwrite(CR4_GUEST_HOST_MASK, v->arch.hvm_vmx.cr4_host_mask);
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 8176736e8f..2baf707eec 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1101,7 +1101,8 @@ static void load_shadow_guest_state(struct vcpu *v)
(get_vvmcs(v, CR4_READ_SHADOW) & cr_gh_mask);
__vmwrite(CR4_READ_SHADOW, cr_read_shadow);
/* Add the nested host mask to the one set by vmx_update_guest_cr. */
- __vmwrite(CR4_GUEST_HOST_MASK, cr_gh_mask | v->arch.hvm_vmx.cr4_host_mask);
+ v->arch.hvm_vmx.cr4_host_mask |= cr_gh_mask;
+ __vmwrite(CR4_GUEST_HOST_MASK, v->arch.hvm_vmx.cr4_host_mask);
/* TODO: CR3 target control */
}
@@ -1232,6 +1233,16 @@ static void sync_vvmcs_guest_state(struct vcpu *v, struct cpu_user_regs *regs)
/* CR3 sync if exec doesn't want cr3 load exiting: i.e. nested EPT */
if ( !(__n2_exec_control(v) & CPU_BASED_CR3_LOAD_EXITING) )
shadow_to_vvmcs(v, GUEST_CR3);
+
+ if ( v->arch.hvm_vmx.cr4_host_mask != ~0UL )
+ {
+ /* Only need to update nested GUEST_CR4 if not all bits are trapped. */
+ unsigned long nested_cr4_mask = get_vvmcs(v, CR4_GUEST_HOST_MASK);
+
+ set_vvmcs(v, GUEST_CR4,
+ (get_vvmcs(v, GUEST_CR4) & nested_cr4_mask) |
+ (v->arch.hvm_vcpu.guest_cr[4] & ~nested_cr4_mask));
+ }
}
static void sync_vvmcs_ro(struct vcpu *v)
--
2.16.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] vvmx: fixes after CR4 trapping optimizations
2018-03-01 16:19 [PATCH] vvmx: fixes after CR4 trapping optimizations Roger Pau Monne
@ 2018-03-02 14:29 ` Sergey Dyasli
2018-03-02 16:14 ` Roger Pau Monné
0 siblings, 1 reply; 3+ messages in thread
From: Sergey Dyasli @ 2018-03-02 14:29 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel@lists.xenproject.org
Cc: Sergey Dyasli, Kevin Tian, jun.nakajima@intel.com,
jbeulich@suse.com, Andrew Cooper
On Thu, 2018-03-01 at 16:19 +0000, Roger Pau Monne wrote:
> Commit 406817 doesn't update nested VMX code in order to take into
> account L1 CR4 host mask when nested guest (L2) writes to CR4, and
> thus the mask written to CR4_GUEST_HOST_MASK is likely not as
> restrictive as it should be.
>
> Also the VVMCS GUEST_CR4 value should be updated to match the
> underlying value when syncing the VVMCS state.
>
> Fixes: 406817 ("vmx/hap: optimize CR4 trapping")
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> I've manually tested and AFAICT this fixes the osstest failure
> detected in 120076 ("test-amd64-amd64-qemuu-nested-intel").
> ---
> xen/arch/x86/hvm/vmx/vmx.c | 4 ++++
> xen/arch/x86/hvm/vmx/vvmx.c | 13 ++++++++++++-
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 5cee364b0c..18d8ce2303 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1617,6 +1617,10 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr,
> v->arch.hvm_vmx.cr4_host_mask |=
> ~v->domain->arch.monitor.write_ctrlreg_mask[VM_EVENT_X86_CR4];
>
> + if ( nestedhvm_vcpu_in_guestmode(v) )
> + /* Add the nested host mask to get the more restrictive one. */
> + v->arch.hvm_vmx.cr4_host_mask |= get_vvmcs(v,
> + CR4_GUEST_HOST_MASK);
> }
> __vmwrite(CR4_GUEST_HOST_MASK, v->arch.hvm_vmx.cr4_host_mask);
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 8176736e8f..2baf707eec 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1101,7 +1101,8 @@ static void load_shadow_guest_state(struct vcpu *v)
> (get_vvmcs(v, CR4_READ_SHADOW) & cr_gh_mask);
> __vmwrite(CR4_READ_SHADOW, cr_read_shadow);
> /* Add the nested host mask to the one set by vmx_update_guest_cr. */
> - __vmwrite(CR4_GUEST_HOST_MASK, cr_gh_mask | v->arch.hvm_vmx.cr4_host_mask);
> + v->arch.hvm_vmx.cr4_host_mask |= cr_gh_mask;
> + __vmwrite(CR4_GUEST_HOST_MASK, v->arch.hvm_vmx.cr4_host_mask);
>
> /* TODO: CR3 target control */
> }
> @@ -1232,6 +1233,16 @@ static void sync_vvmcs_guest_state(struct vcpu *v, struct cpu_user_regs *regs)
> /* CR3 sync if exec doesn't want cr3 load exiting: i.e. nested EPT */
> if ( !(__n2_exec_control(v) & CPU_BASED_CR3_LOAD_EXITING) )
> shadow_to_vvmcs(v, GUEST_CR3);
> +
> + if ( v->arch.hvm_vmx.cr4_host_mask != ~0UL )
> + {
> + /* Only need to update nested GUEST_CR4 if not all bits are trapped. */
> + unsigned long nested_cr4_mask = get_vvmcs(v, CR4_GUEST_HOST_MASK);
> +
> + set_vvmcs(v, GUEST_CR4,
> + (get_vvmcs(v, GUEST_CR4) & nested_cr4_mask) |
> + (v->arch.hvm_vcpu.guest_cr[4] & ~nested_cr4_mask));
Why reading the old GUEST_CR4 is needed here? Can the new value be set
directly from guest_cr[4]?
> + }
> }
>
> static void sync_vvmcs_ro(struct vcpu *v)
--
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] vvmx: fixes after CR4 trapping optimizations
2018-03-02 14:29 ` Sergey Dyasli
@ 2018-03-02 16:14 ` Roger Pau Monné
0 siblings, 0 replies; 3+ messages in thread
From: Roger Pau Monné @ 2018-03-02 16:14 UTC (permalink / raw)
To: Sergey Dyasli
Cc: xen-devel@lists.xenproject.org, Kevin Tian,
jun.nakajima@intel.com, jbeulich@suse.com, Andrew Cooper
On Fri, Mar 02, 2018 at 02:29:54PM +0000, Sergey Dyasli wrote:
> On Thu, 2018-03-01 at 16:19 +0000, Roger Pau Monne wrote:
> > Commit 406817 doesn't update nested VMX code in order to take into
> > account L1 CR4 host mask when nested guest (L2) writes to CR4, and
> > thus the mask written to CR4_GUEST_HOST_MASK is likely not as
> > restrictive as it should be.
> >
> > Also the VVMCS GUEST_CR4 value should be updated to match the
> > underlying value when syncing the VVMCS state.
> >
> > Fixes: 406817 ("vmx/hap: optimize CR4 trapping")
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Cc: Jun Nakajima <jun.nakajima@intel.com>
> > Cc: Kevin Tian <kevin.tian@intel.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > I've manually tested and AFAICT this fixes the osstest failure
> > detected in 120076 ("test-amd64-amd64-qemuu-nested-intel").
> > ---
> > xen/arch/x86/hvm/vmx/vmx.c | 4 ++++
> > xen/arch/x86/hvm/vmx/vvmx.c | 13 ++++++++++++-
> > 2 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> > index 5cee364b0c..18d8ce2303 100644
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -1617,6 +1617,10 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr,
> > v->arch.hvm_vmx.cr4_host_mask |=
> > ~v->domain->arch.monitor.write_ctrlreg_mask[VM_EVENT_X86_CR4];
> >
> > + if ( nestedhvm_vcpu_in_guestmode(v) )
> > + /* Add the nested host mask to get the more restrictive one. */
> > + v->arch.hvm_vmx.cr4_host_mask |= get_vvmcs(v,
> > + CR4_GUEST_HOST_MASK);
> > }
> > __vmwrite(CR4_GUEST_HOST_MASK, v->arch.hvm_vmx.cr4_host_mask);
> >
> > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> > index 8176736e8f..2baf707eec 100644
> > --- a/xen/arch/x86/hvm/vmx/vvmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> > @@ -1101,7 +1101,8 @@ static void load_shadow_guest_state(struct vcpu *v)
> > (get_vvmcs(v, CR4_READ_SHADOW) & cr_gh_mask);
> > __vmwrite(CR4_READ_SHADOW, cr_read_shadow);
> > /* Add the nested host mask to the one set by vmx_update_guest_cr. */
> > - __vmwrite(CR4_GUEST_HOST_MASK, cr_gh_mask | v->arch.hvm_vmx.cr4_host_mask);
> > + v->arch.hvm_vmx.cr4_host_mask |= cr_gh_mask;
> > + __vmwrite(CR4_GUEST_HOST_MASK, v->arch.hvm_vmx.cr4_host_mask);
> >
> > /* TODO: CR3 target control */
> > }
> > @@ -1232,6 +1233,16 @@ static void sync_vvmcs_guest_state(struct vcpu *v, struct cpu_user_regs *regs)
> > /* CR3 sync if exec doesn't want cr3 load exiting: i.e. nested EPT */
> > if ( !(__n2_exec_control(v) & CPU_BASED_CR3_LOAD_EXITING) )
> > shadow_to_vvmcs(v, GUEST_CR3);
> > +
> > + if ( v->arch.hvm_vmx.cr4_host_mask != ~0UL )
> > + {
> > + /* Only need to update nested GUEST_CR4 if not all bits are trapped. */
> > + unsigned long nested_cr4_mask = get_vvmcs(v, CR4_GUEST_HOST_MASK);
> > +
> > + set_vvmcs(v, GUEST_CR4,
> > + (get_vvmcs(v, GUEST_CR4) & nested_cr4_mask) |
> > + (v->arch.hvm_vcpu.guest_cr[4] & ~nested_cr4_mask));
>
> Why reading the old GUEST_CR4 is needed here? Can the new value be set
> directly from guest_cr[4]?
Yes, I think so. The nested GUEST_CR4 value is the value the L1
hypervisor thinks is written to the hardware CR4 (while the L2 guest
is running) and guest_cr[4] contains the value of the CR4 register as
seen by the L1 hypervisor.
There's no way CR4 L1 tapped bits can leak into guest_cr[4] because
cr4_host_mask is always equally or more restrictive than the nested
CR4_GUEST_HOST_MASK. Let me send a new version.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-03-02 16:15 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-01 16:19 [PATCH] vvmx: fixes after CR4 trapping optimizations Roger Pau Monne
2018-03-02 14:29 ` Sergey Dyasli
2018-03-02 16:14 ` Roger Pau Monné
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.