kvm: always set accessed bit in VMCS segment selectors
PATCH 1/1 kvm: always set accessed bit in VMCS segment selectors
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.
--- arch/x86/kvm/vmx.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
arch/x86/kvm/vmx.c +5 -0
@@ -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); }
Signature · 1 line
1.5.2.2
Quoted 8 lines · 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.
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).
Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.
PATCH 1/1 set accessed bit for VMCB segment selectors
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.
--- arch/x86/kvm/svm.c | 22 +++++++++++++++++++--- 1 files changed, 19 insertions(+), 3 deletions(-)
arch/x86/kvm/svm.c +19 -3
@@ -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; }
Signature · 1 line
1.5.2.2
Re: [PATCH] set accessed bit for VMCB segment selectors
Quoted 15 lines · 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 <[email protected]>--- arch/x86/kvm/svm.c | 22 +++++++++++++++++++--- 1 files changed, 19 insertions(+), 3 deletions(-)arch/x86/kvm/svm.c +0
-0
arch/x86/kvm/svm.c static void svm_get_segment(struct kvm_vcpu *vcpu, · line 802
Quoted 10 lines
arch/x86/kvm/svm.c +2
-0@@ -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.
Quoted 14 lines
arch/x86/kvm/svm.c +5
-3-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 :
arch/x86/kvm/svm.c static void svm_get_segment(struct kvm_vcpu *vcpu, · line 823
Quoted 10 lines
arch/x86/kvm/svm.c +10
-0+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.
arch/x86/kvm/svm.c static void svm_get_segment(struct kvm_vcpu *vcpu, · line 828
Quoted 5 lines
arch/x86/kvm/svm.c +2
-0+break ; + } var->unusable = !var->present; }
Other than these minor style issues,
PATCH 1/1 kvm: set accessed bit for VMCB segment selectors
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.
--- arch/x86/kvm/svm.c | 41 +++++++++++++++++++++++++++++------------ 1 files changed, 29 insertions(+), 12 deletions(-)
arch/x86/kvm/svm.c +29 -12
@@ -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; }
Signature · 1 line
1.5.2.2