Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1652529 > unrolled thread

[RFC] KVM: SVM: ignore type when setting segment registers

Back to article view | Back to linux.kernel

Contents

  [RFC] KVM: SVM: ignore type when setting segment registers Gioh Kim <gi-oh.kim@profitbricks.com> - 2017-05-29 15:30 +0200
    Re: [RFC] KVM: SVM: ignore type when setting segment registers Radim Krčmář <rkrcmar@redhat.com> - 2017-05-30 15:00 +0200
      Re: [RFC] KVM: SVM: ignore type when setting segment registers Gioh Kim <gi-oh.kim@profitbricks.com> - 2017-05-30 15:40 +0200
      Re: [RFC] KVM: SVM: ignore type when setting segment registers Matt Mullins <mmullins@mmlx.us> - 2017-05-30 20:20 +0200
        Re: [RFC] KVM: SVM: ignore type when setting segment registers Gi-Oh Kim <gi-oh.kim@profitbricks.com> - 2017-05-31 08:50 +0200

#1652529 — [RFC] KVM: SVM: ignore type when setting segment registers

Current code sets unusable as 1 if present is 1 and type is 0.
In Long mode, type value in segment descriptor is ignored.
So I think type should be ignored when setting the segment registers,
if type means the descriptor type in the segment descriptor.

Is the type field of struct kvm_segment the descriptor type?
If so, why type is checked when setting segment registers?

If the type field is not the descriptor type,
is it ok to set unusable when present is 1?

I'm copying a code as following to show what code I'm asking.

----------------------------- 8< ---------------------------------
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 5f48f62..0133f6f 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1803,7 +1803,7 @@ static void svm_get_segment(struct kvm_vcpu *vcpu,
 	 * AMD's VMCB does not have an explicit unusable field, so emulate it
 	 * for cross vendor migration purposes by "not present"
 	 */
-	var->unusable = !var->present || (var->type == 0);
+	var->unusable = !var->present;
 
 	switch (seg) {
 	case VCPU_SREG_TR:
-- 
2.5.0

[toc] | [next] | [standalone]

#1653212

2017-05-29 15:24+0200, Gioh Kim:
> Current code sets unusable as 1 if present is 1 and type is 0.
> In Long mode, type value in segment descriptor is ignored.
> So I think type should be ignored when setting the segment registers,
> if type means the descriptor type in the segment descriptor.
> 
> Is the type field of struct kvm_segment the descriptor type?

Yes.

> If so, why type is checked when setting segment registers?

No idea.  19bca6ab75d8 ("KVM: SVM: Fix cross vendor migration issue with
unusable bit") also moved the assigment up to initialize it before use
and I think that is enough.

> If the type field is not the descriptor type,
> is it ok to set unusable when present is 1?

Looks like a bug.  type = 0 can be a usable read-only data segment.

> I'm copying a code as following to show what code I'm asking.

Please send it as a patch,

thanks.

> ----------------------------- 8< ---------------------------------
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 5f48f62..0133f6f 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1803,7 +1803,7 @@ static void svm_get_segment(struct kvm_vcpu *vcpu,
>  	 * AMD's VMCB does not have an explicit unusable field, so emulate it
>  	 * for cross vendor migration purposes by "not present"
>  	 */
> -	var->unusable = !var->present || (var->type == 0);
> +	var->unusable = !var->present;
>  
>  	switch (seg) {
>  	case VCPU_SREG_TR:
> -- 
> 2.5.0
> 

[toc] | [prev] | [next] | [standalone]

#1653242

On Tue, May 30, 2017 at 02:54:21PM +0200, Radim Krčmář wrote:
> 2017-05-29 15:24+0200, Gioh Kim:
> > Current code sets unusable as 1 if present is 1 and type is 0.
> > In Long mode, type value in segment descriptor is ignored.
> > So I think type should be ignored when setting the segment registers,
> > if type means the descriptor type in the segment descriptor.
> > 
> > Is the type field of struct kvm_segment the descriptor type?
> 
> Yes.
> 
> > If so, why type is checked when setting segment registers?
> 
> No idea.  19bca6ab75d8 ("KVM: SVM: Fix cross vendor migration issue with
> unusable bit") also moved the assigment up to initialize it before use
> and I think that is enough.
> 
> > If the type field is not the descriptor type,
> > is it ok to set unusable when present is 1?
> 
> Looks like a bug.  type = 0 can be a usable read-only data segment.
> 
> > I'm copying a code as following to show what code I'm asking.
> 
> Please send it as a patch,

Hi Radim,

Thank you for reply.
I sent a patch: https://lkml.org/lkml/2017/5/30/459
I'd appreciate if if you could review it.

-- 
Best regards,
Gi-Oh Kim
TEL: 0176 2697 8962

[toc] | [prev] | [next] | [standalone]

#1653464

On Tue, May 30, 2017 at 02:54:21PM +0200, Radim Krčmář wrote:
> 2017-05-29 15:24+0200, Gioh Kim:
> > If so, why type is checked when setting segment registers?
> 
> No idea.  19bca6ab75d8 ("KVM: SVM: Fix cross vendor migration issue with
> unusable bit") also moved the assigment up to initialize it before use
> and I think that is enough.

Was this perhaps intended to instead check for a zero selector, which is also
an unusable segment?

[toc] | [prev] | [next] | [standalone]

#1653881

On Tue, May 30, 2017 at 8:03 PM, Matt Mullins <mmullins@mmlx.us> wrote:
> On Tue, May 30, 2017 at 02:54:21PM +0200, Radim Krčmář wrote:
>> 2017-05-29 15:24+0200, Gioh Kim:
>> > If so, why type is checked when setting segment registers?
>>
>> No idea.  19bca6ab75d8 ("KVM: SVM: Fix cross vendor migration issue with
>> unusable bit") also moved the assigment up to initialize it before use
>> and I think that is enough.
>
> Was this perhaps intended to instead check for a zero selector, which is also
> an unusable segment?

I think that is what present value is for.


-- 
Best regards,
Gi-Oh Kim
TEL: 0176 2697 8962

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web