[PATCH] kvm: always set accessed bit in VMCS segment selectors

[PATCH] kvm: always set accessed bit in VMCS segment selectors

[Andre Przywara]

Intel manual 22.3.1.2 demands that the accessed bit (bit 0 in type field)
must be set when on DS,ES,FS and GS when the selector is usable.
This fixes cross vendor migration from AMD to Intel.

I am not sure what the real purpose of this check is, so I put this
in the VMX path and not in the SVM one. If someone has an explanation
which justifies a move, I am happy to do this.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>
---
 arch/x86/kvm/vmx.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 9b56d21..d19e39c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1723,6 +1723,11 @@ static void vmx_set_segment(struct kvm_vcpu *vcpu,
 		ar = 0xf3;
 	} else
 		ar = vmx_segment_access_rights(var);
+
+	/* 22.3.1.2 demands that the accessed bit must be set on [DEFG]S */
+	if (var->s && (sf->ar_bytes & AR_UNUSABLE_MASK) == 0)
+		ar |= 0x1;
+
 	vmcs_write32(sf->ar_bytes, ar);
 }
 
-- 
1.5.2.2

Re: [PATCH] kvm: always set accessed bit in VMCS segment selectors

[Avi Kivity]

Andre Przywara wrote:
> Intel manual 22.3.1.2 demands that the accessed bit (bit 0 in type field)
> must be set when on DS,ES,FS and GS when the selector is usable.
> This fixes cross vendor migration from AMD to Intel.
>
> I am not sure what the real purpose of this check is, so I put this
> in the VMX path and not in the SVM one. If someone has an explanation
> which justifies a move, I am happy to do this.
>   

If I understand correctly, loading a segment should set the accessed bit 
in the descriptor table (without virtualization there is now way to have 
the accessed bit clear in the segment cache), so it looks like the 
correct fix is to adjust svm (we already have a couple of similar fixes 
there).

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[PATCH] set accessed bit for VMCB segment selectors

[Andre Przywara]

In the segment descriptor _cache_ the accessed bit is always set
(although it can be cleared in the descriptor itself). Since Intel
checks for this condition on a VMENTRY, set this bit in the AMD path
to enable cross vendor migration.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>
---
 arch/x86/kvm/svm.c |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 32dafb7..ffaba66 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -793,20 +793,36 @@ static void svm_get_segment(struct kvm_vcpu *vcpu,
 	var->db = (s->attrib >> SVM_SELECTOR_DB_SHIFT) & 1;
 	var->g = (s->attrib >> SVM_SELECTOR_G_SHIFT) & 1;
 
+	switch (seg) {
+	case VCPU_SREG_CS:
 	/*
 	 * SVM always stores 0 for the 'G' bit in the CS selector in
 	 * the VMCB on a VMEXIT. This hurts cross-vendor migration:
 	 * Intel's VMENTRY has a check on the 'G' bit.
 	 */
-	if (seg == VCPU_SREG_CS)
 		var->g = s->limit > 0xfffff;
-
+		break;
+	case VCPU_SREG_TR:
 	/*
 	 * Work around a bug where the busy flag in the tr selector
 	 * isn't exposed
 	 */
-	if (seg == VCPU_SREG_TR)
 		var->type |= 0x2;
+		break;
+	case VCPU_SREG_DS:
+	case VCPU_SREG_ES:
+	case VCPU_SREG_FS:
+	case VCPU_SREG_GS:
+	/*
+	 * The accessed bit must always be set in the segment
+	 * descriptor cache, although it can be cleared in the
+	 * descriptor, the cached bit always remains at 1. Since
+	 * Intel has a check on this, set it here to support
+	 * cross-vendor migration.
+	 */
+		if (!var->unusable) var->type |= 0x1;
+		break;
+	}
 
 	var->unusable = !var->present;
 }
-- 
1.5.2.2

Re: [PATCH] set accessed bit for VMCB segment selectors

[Amit Shah]

On Fri, Jan 09, 2009 at 10:27:35PM +0100, Andre Przywara wrote:
> In the segment descriptor _cache_ the accessed bit is always set
> (although it can be cleared in the descriptor itself). Since Intel
> checks for this condition on a VMENTRY, set this bit in the AMD path
> to enable cross vendor migration.
> 
> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
> ---
>  arch/x86/kvm/svm.c |   22 +++++++++++++++++++---
>  1 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 32dafb7..ffaba66 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -793,20 +793,36 @@ static void svm_get_segment(struct kvm_vcpu *vcpu,
>  	var->db = (s->attrib >> SVM_SELECTOR_DB_SHIFT) & 1;
>  	var->g = (s->attrib >> SVM_SELECTOR_G_SHIFT) & 1;
>  
> +	switch (seg) {
> +	case VCPU_SREG_CS:
>  	/*
>  	 * SVM always stores 0 for the 'G' bit in the CS selector in
>  	 * the VMCB on a VMEXIT. This hurts cross-vendor migration:
>  	 * Intel's VMENTRY has a check on the 'G' bit.
>  	 */

This comment will need to be indented.

> -	if (seg == VCPU_SREG_CS)
>  		var->g = s->limit > 0xfffff;
> -
> +		break;
> +	case VCPU_SREG_TR:
>  	/*
>  	 * Work around a bug where the busy flag in the tr selector
>  	 * isn't exposed
>  	 */
> -	if (seg == VCPU_SREG_TR)
>  		var->type |= 0x2;
> +		break;
> +	case VCPU_SREG_DS:
> +	case VCPU_SREG_ES:
> +	case VCPU_SREG_FS:
> +	case VCPU_SREG_GS:
> +	/*
> +	 * The accessed bit must always be set in the segment
> +	 * descriptor cache, although it can be cleared in the
> +	 * descriptor, the cached bit always remains at 1. Since
> +	 * Intel has a check on this, set it here to support
> +	 * cross-vendor migration.
> +	 */
> +		if (!var->unusable) var->type |= 0x1;

A line break is missing after the if check.

> +		break;
> +	}
>  
>  	var->unusable = !var->present;
>  }

Other than these minor style issues,

Acked-By: Amit Shah <amit.shah@redhat.com>

[PATCH] kvm: set accessed bit for VMCB segment selectors

[Andre Przywara]

In the segment descriptor _cache_ the accessed bit is always set
(although it can be cleared in the descriptor itself). Since Intel
checks for this condition on a VMENTRY, set this bit in the AMD path
to enable cross vendor migration.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>
Acked-By: Amit Shah <amit.shah@redhat.com>
---
 arch/x86/kvm/svm.c |   41 +++++++++++++++++++++++++++++------------
 1 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 14e517e..41ba356 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -793,20 +793,37 @@ static void svm_get_segment(struct kvm_vcpu *vcpu,
 	var->db = (s->attrib >> SVM_SELECTOR_DB_SHIFT) & 1;
 	var->g = (s->attrib >> SVM_SELECTOR_G_SHIFT) & 1;
 
-	/*
-	 * SVM always stores 0 for the 'G' bit in the CS selector in
-	 * the VMCB on a VMEXIT. This hurts cross-vendor migration:
-	 * Intel's VMENTRY has a check on the 'G' bit.
-	 */
-	if (seg == VCPU_SREG_CS)
+	switch (seg) {
+	case VCPU_SREG_CS:
+		/*
+		 * SVM always stores 0 for the 'G' bit in the CS selector in
+		 * the VMCB on a VMEXIT. This hurts cross-vendor migration:
+		 * Intel's VMENTRY has a check on the 'G' bit.
+		 */
 		var->g = s->limit > 0xfffff;
-
-	/*
-	 * Work around a bug where the busy flag in the tr selector
-	 * isn't exposed
-	 */
-	if (seg == VCPU_SREG_TR)
+		break;
+	case VCPU_SREG_TR:
+		/*
+		 * Work around a bug where the busy flag in the tr selector
+		 * isn't exposed
+		 */
 		var->type |= 0x2;
+		break;
+	case VCPU_SREG_DS:
+	case VCPU_SREG_ES:
+	case VCPU_SREG_FS:
+	case VCPU_SREG_GS:
+		/*
+		 * The accessed bit must always be set in the segment
+		 * descriptor cache, although it can be cleared in the
+		 * descriptor, the cached bit always remains at 1. Since
+		 * Intel has a check on this, set it here to support
+		 * cross-vendor migration.
+		 */
+		if (!var->unusable)
+			var->type |= 0x1;
+		break;
+	}
 
 	var->unusable = !var->present;
 }
-- 
1.5.2.2