diff mbox

[v3,4/4] KVM: VMX: Simplify segment_base

Message ID 20170214194259.75960-4-thgarnie@google.com
State New, archived
Headers show

Commit Message

Thomas Garnier Feb. 14, 2017, 7:42 p.m. UTC
The KVM segment_base function is confusing. This patch replaces integers
with appropriate flags, simplify constructs and add comments.

Signed-off-by: Thomas Garnier <thgarnie@google.com>
---
Based on next-20170213
---
 arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

Comments

Andy Lutomirski Feb. 15, 2017, 3:57 a.m. UTC | #1
On Tue, Feb 14, 2017 at 11:42 AM, Thomas Garnier <thgarnie@google.com> wrote:
> The KVM segment_base function is confusing. This patch replaces integers
> with appropriate flags, simplify constructs and add comments.

It could pay to put this first in the series, but last is probably fine, too.

>
> Signed-off-by: Thomas Garnier <thgarnie@google.com>
> ---
> Based on next-20170213
> ---
>  arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 99167f20bc34..edb8326108dd 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2062,25 +2062,35 @@ static unsigned long segment_base(u16 selector)
>         struct desc_struct *d;
>         unsigned long table_base;

This should go away IMO.  It should be struct desc_struct *table;

> +               table_base = get_current_gdt_rw_vaddr();

Then this can go away, too, and you can stop having the silly
get_current_gdt_rw_vaddr() function at all.

> +       d = (struct desc_struct *)table_base + (selector >> 3);

And this cast goes away too.
Thomas Garnier Feb. 15, 2017, 3:44 p.m. UTC | #2
On Tue, Feb 14, 2017 at 7:57 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Tue, Feb 14, 2017 at 11:42 AM, Thomas Garnier <thgarnie@google.com> wrote:
>> The KVM segment_base function is confusing. This patch replaces integers
>> with appropriate flags, simplify constructs and add comments.
>
> It could pay to put this first in the series, but last is probably fine, too.
>

:)

>>
>> Signed-off-by: Thomas Garnier <thgarnie@google.com>
>> ---
>> Based on next-20170213
>> ---
>>  arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
>>  1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 99167f20bc34..edb8326108dd 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -2062,25 +2062,35 @@ static unsigned long segment_base(u16 selector)
>>         struct desc_struct *d;
>>         unsigned long table_base;
>
> This should go away IMO.  It should be struct desc_struct *table;
>
>> +               table_base = get_current_gdt_rw_vaddr();
>
> Then this can go away, too, and you can stop having the silly
> get_current_gdt_rw_vaddr() function at all.
>
>> +       d = (struct desc_struct *)table_base + (selector >> 3);
>
> And this cast goes away too.

No problem.
Jim Mattson Feb. 17, 2017, 5:49 p.m. UTC | #3
Can we use the read-only GDT here? When expanding the virtual address
for 64-bit system descriptors, isn't it sufficient to check (d->s == 0
&& d->type != 0)?

On Tue, Feb 14, 2017 at 11:42 AM, Thomas Garnier <thgarnie@google.com> wrote:
> The KVM segment_base function is confusing. This patch replaces integers
> with appropriate flags, simplify constructs and add comments.
>
> Signed-off-by: Thomas Garnier <thgarnie@google.com>
> ---
> Based on next-20170213
> ---
>  arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 99167f20bc34..edb8326108dd 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2062,25 +2062,35 @@ static unsigned long segment_base(u16 selector)
>         struct desc_struct *d;
>         unsigned long table_base;
>         unsigned long v;
> +       u32 high32;
>
> -       if (!(selector & ~3))
> +       if (!(selector & ~SEGMENT_RPL_MASK))
>                 return 0;
>
> -       table_base = get_current_gdt_rw_vaddr();
> -
> -       if (selector & 4) {           /* from ldt */
> +       /* LDT selector */
> +       if ((selector & SEGMENT_TI_MASK) == SEGMENT_LDT) {
>                 u16 ldt_selector = kvm_read_ldt();
>
> -               if (!(ldt_selector & ~3))
> +               if (!(ldt_selector & ~SEGMENT_RPL_MASK))
>                         return 0;
>
>                 table_base = segment_base(ldt_selector);
> +       } else {
> +               table_base = get_current_gdt_rw_vaddr();
>         }
> -       d = (struct desc_struct *)(table_base + (selector & ~7));
> +
> +       d = (struct desc_struct *)table_base + (selector >> 3);
>         v = get_desc_base(d);
>  #ifdef CONFIG_X86_64
> -       if (d->s == 0 && (d->type == 2 || d->type == 9 || d->type == 11))
> -               v |= ((unsigned long)((struct ldttss_desc64 *)d)->base3) << 32;
> +       /*
> +        * Extend the virtual address if we have a system descriptor entry for
> +        * LDT or TSS (available or busy).
> +        */
> +       if (d->s == 0 && (d->type == DESC_LDT || d->type == DESC_TSS ||
> +                         d->type == 11/*Busy TSS */)) {
> +               high32 = ((struct ldttss_desc64 *)d)->base3;
> +               v |= (u64)high32 << 32;
> +       }
>  #endif
>         return v;
>  }
> --
> 2.11.0.483.g087da7b7c-goog
>
Thomas Garnier Feb. 17, 2017, 8:11 p.m. UTC | #4
On Fri, Feb 17, 2017 at 9:49 AM, Jim Mattson <jmattson@google.com> wrote:
>
> Can we use the read-only GDT here? When expanding the virtual address
> for 64-bit system descriptors, isn't it sufficient to check (d->s == 0
> && d->type != 0)?

We can use the readonly GDT but I think doesn't matter one or the
other here. We have to check specific types for LDT or TSS, other
values describe other entries (cf Intel volume 3, 3.5) (for example 14
& 15 on 64-bits are for trap & interrupt gates).

>
>
> On Tue, Feb 14, 2017 at 11:42 AM, Thomas Garnier <thgarnie@google.com> wrote:
> > The KVM segment_base function is confusing. This patch replaces integers
> > with appropriate flags, simplify constructs and add comments.
> >
> > Signed-off-by: Thomas Garnier <thgarnie@google.com>
> > ---
> > Based on next-20170213
> > ---
> >  arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
> >  1 file changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 99167f20bc34..edb8326108dd 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -2062,25 +2062,35 @@ static unsigned long segment_base(u16 selector)
> >         struct desc_struct *d;
> >         unsigned long table_base;
> >         unsigned long v;
> > +       u32 high32;
> >
> > -       if (!(selector & ~3))
> > +       if (!(selector & ~SEGMENT_RPL_MASK))
> >                 return 0;
> >
> > -       table_base = get_current_gdt_rw_vaddr();
> > -
> > -       if (selector & 4) {           /* from ldt */
> > +       /* LDT selector */
> > +       if ((selector & SEGMENT_TI_MASK) == SEGMENT_LDT) {
> >                 u16 ldt_selector = kvm_read_ldt();
> >
> > -               if (!(ldt_selector & ~3))
> > +               if (!(ldt_selector & ~SEGMENT_RPL_MASK))
> >                         return 0;
> >
> >                 table_base = segment_base(ldt_selector);
> > +       } else {
> > +               table_base = get_current_gdt_rw_vaddr();
> >         }
> > -       d = (struct desc_struct *)(table_base + (selector & ~7));
> > +
> > +       d = (struct desc_struct *)table_base + (selector >> 3);
> >         v = get_desc_base(d);
> >  #ifdef CONFIG_X86_64
> > -       if (d->s == 0 && (d->type == 2 || d->type == 9 || d->type == 11))
> > -               v |= ((unsigned long)((struct ldttss_desc64 *)d)->base3) << 32;
> > +       /*
> > +        * Extend the virtual address if we have a system descriptor entry for
> > +        * LDT or TSS (available or busy).
> > +        */
> > +       if (d->s == 0 && (d->type == DESC_LDT || d->type == DESC_TSS ||
> > +                         d->type == 11/*Busy TSS */)) {
> > +               high32 = ((struct ldttss_desc64 *)d)->base3;
> > +               v |= (u64)high32 << 32;
> > +       }
> >  #endif
> >         return v;
> >  }
> > --
> > 2.11.0.483.g087da7b7c-goog
> >
Jim Mattson Feb. 17, 2017, 9 p.m. UTC | #5
On Fri, Feb 17, 2017 at 12:11 PM, Thomas Garnier <thgarnie@google.com> wrote:
> On Fri, Feb 17, 2017 at 9:49 AM, Jim Mattson <jmattson@google.com> wrote:
>>
>> Can we use the read-only GDT here? When expanding the virtual address
>> for 64-bit system descriptors, isn't it sufficient to check (d->s == 0
>> && d->type != 0)?
>
> We can use the readonly GDT but I think doesn't matter one or the
> other here. We have to check specific types for LDT or TSS, other
> values describe other entries (cf Intel volume 3, 3.5) (for example 14
> & 15 on 64-bits are for trap & interrupt gates).

According to volume 3 of the SDM, section 3.5.2:

The following system descriptors expand to 16 bytes:
— Call gate descriptors (see Section 5.8.3.1, “IA-32e Mode Call Gates”)
— IDT gate descriptors (see Section 6.14.1, “64-Bit Mode IDT”)
— LDT and TSS descriptors (see Section 7.2.3, “TSS Descriptor in 64-bit mode”).

All legal system descriptor types (except for 0: Upper 8 bytes of an 16-byte
descriptor) should get the high 32 bits of the base address from the next 8-byte
descriptor.

>
>>
>>
>> On Tue, Feb 14, 2017 at 11:42 AM, Thomas Garnier <thgarnie@google.com> wrote:
>> > The KVM segment_base function is confusing. This patch replaces integers
>> > with appropriate flags, simplify constructs and add comments.
>> >
>> > Signed-off-by: Thomas Garnier <thgarnie@google.com>
>> > ---
>> > Based on next-20170213
>> > ---
>> >  arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
>> >  1 file changed, 18 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> > index 99167f20bc34..edb8326108dd 100644
>> > --- a/arch/x86/kvm/vmx.c
>> > +++ b/arch/x86/kvm/vmx.c
>> > @@ -2062,25 +2062,35 @@ static unsigned long segment_base(u16 selector)
>> >         struct desc_struct *d;
>> >         unsigned long table_base;
>> >         unsigned long v;
>> > +       u32 high32;
>> >
>> > -       if (!(selector & ~3))
>> > +       if (!(selector & ~SEGMENT_RPL_MASK))
>> >                 return 0;
>> >
>> > -       table_base = get_current_gdt_rw_vaddr();
>> > -
>> > -       if (selector & 4) {           /* from ldt */
>> > +       /* LDT selector */
>> > +       if ((selector & SEGMENT_TI_MASK) == SEGMENT_LDT) {
>> >                 u16 ldt_selector = kvm_read_ldt();
>> >
>> > -               if (!(ldt_selector & ~3))
>> > +               if (!(ldt_selector & ~SEGMENT_RPL_MASK))
>> >                         return 0;
>> >
>> >                 table_base = segment_base(ldt_selector);
>> > +       } else {
>> > +               table_base = get_current_gdt_rw_vaddr();
>> >         }
>> > -       d = (struct desc_struct *)(table_base + (selector & ~7));
>> > +
>> > +       d = (struct desc_struct *)table_base + (selector >> 3);
>> >         v = get_desc_base(d);
>> >  #ifdef CONFIG_X86_64
>> > -       if (d->s == 0 && (d->type == 2 || d->type == 9 || d->type == 11))
>> > -               v |= ((unsigned long)((struct ldttss_desc64 *)d)->base3) << 32;
>> > +       /*
>> > +        * Extend the virtual address if we have a system descriptor entry for
>> > +        * LDT or TSS (available or busy).
>> > +        */
>> > +       if (d->s == 0 && (d->type == DESC_LDT || d->type == DESC_TSS ||
>> > +                         d->type == 11/*Busy TSS */)) {
>> > +               high32 = ((struct ldttss_desc64 *)d)->base3;
>> > +               v |= (u64)high32 << 32;
>> > +       }
>> >  #endif
>> >         return v;
>> >  }
>> > --
>> > 2.11.0.483.g087da7b7c-goog
>> >
>
>
>
>
> --
> Thomas
Thomas Garnier Feb. 17, 2017, 10:01 p.m. UTC | #6
On Fri, Feb 17, 2017 at 1:00 PM, Jim Mattson <jmattson@google.com> wrote:
> On Fri, Feb 17, 2017 at 12:11 PM, Thomas Garnier <thgarnie@google.com> wrote:
>> On Fri, Feb 17, 2017 at 9:49 AM, Jim Mattson <jmattson@google.com> wrote:
>>>
>>> Can we use the read-only GDT here? When expanding the virtual address
>>> for 64-bit system descriptors, isn't it sufficient to check (d->s == 0
>>> && d->type != 0)?
>>
>> We can use the readonly GDT but I think doesn't matter one or the
>> other here. We have to check specific types for LDT or TSS, other
>> values describe other entries (cf Intel volume 3, 3.5) (for example 14
>> & 15 on 64-bits are for trap & interrupt gates).
>
> According to volume 3 of the SDM, section 3.5.2:
>
> The following system descriptors expand to 16 bytes:
> — Call gate descriptors (see Section 5.8.3.1, “IA-32e Mode Call Gates”)
> — IDT gate descriptors (see Section 6.14.1, “64-Bit Mode IDT”)
> — LDT and TSS descriptors (see Section 7.2.3, “TSS Descriptor in 64-bit mode”).
>
> All legal system descriptor types (except for 0: Upper 8 bytes of an 16-byte
> descriptor) should get the high 32 bits of the base address from the next 8-byte
> descriptor.
>

Ok, then I will test an updated version next week.

>>
>>>
>>>
>>> On Tue, Feb 14, 2017 at 11:42 AM, Thomas Garnier <thgarnie@google.com> wrote:
>>> > The KVM segment_base function is confusing. This patch replaces integers
>>> > with appropriate flags, simplify constructs and add comments.
>>> >
>>> > Signed-off-by: Thomas Garnier <thgarnie@google.com>
>>> > ---
>>> > Based on next-20170213
>>> > ---
>>> >  arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
>>> >  1 file changed, 18 insertions(+), 8 deletions(-)
>>> >
>>> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> > index 99167f20bc34..edb8326108dd 100644
>>> > --- a/arch/x86/kvm/vmx.c
>>> > +++ b/arch/x86/kvm/vmx.c
>>> > @@ -2062,25 +2062,35 @@ static unsigned long segment_base(u16 selector)
>>> >         struct desc_struct *d;
>>> >         unsigned long table_base;
>>> >         unsigned long v;
>>> > +       u32 high32;
>>> >
>>> > -       if (!(selector & ~3))
>>> > +       if (!(selector & ~SEGMENT_RPL_MASK))
>>> >                 return 0;
>>> >
>>> > -       table_base = get_current_gdt_rw_vaddr();
>>> > -
>>> > -       if (selector & 4) {           /* from ldt */
>>> > +       /* LDT selector */
>>> > +       if ((selector & SEGMENT_TI_MASK) == SEGMENT_LDT) {
>>> >                 u16 ldt_selector = kvm_read_ldt();
>>> >
>>> > -               if (!(ldt_selector & ~3))
>>> > +               if (!(ldt_selector & ~SEGMENT_RPL_MASK))
>>> >                         return 0;
>>> >
>>> >                 table_base = segment_base(ldt_selector);
>>> > +       } else {
>>> > +               table_base = get_current_gdt_rw_vaddr();
>>> >         }
>>> > -       d = (struct desc_struct *)(table_base + (selector & ~7));
>>> > +
>>> > +       d = (struct desc_struct *)table_base + (selector >> 3);
>>> >         v = get_desc_base(d);
>>> >  #ifdef CONFIG_X86_64
>>> > -       if (d->s == 0 && (d->type == 2 || d->type == 9 || d->type == 11))
>>> > -               v |= ((unsigned long)((struct ldttss_desc64 *)d)->base3) << 32;
>>> > +       /*
>>> > +        * Extend the virtual address if we have a system descriptor entry for
>>> > +        * LDT or TSS (available or busy).
>>> > +        */
>>> > +       if (d->s == 0 && (d->type == DESC_LDT || d->type == DESC_TSS ||
>>> > +                         d->type == 11/*Busy TSS */)) {
>>> > +               high32 = ((struct ldttss_desc64 *)d)->base3;
>>> > +               v |= (u64)high32 << 32;
>>> > +       }
>>> >  #endif
>>> >         return v;
>>> >  }
>>> > --
>>> > 2.11.0.483.g087da7b7c-goog
>>> >
>>
>>
>>
>>
>> --
>> Thomas
Andy Lutomirski Feb. 20, 2017, 4:56 p.m. UTC | #7
On Fri, Feb 17, 2017 at 2:01 PM, Thomas Garnier <thgarnie@google.com> wrote:
> On Fri, Feb 17, 2017 at 1:00 PM, Jim Mattson <jmattson@google.com> wrote:
>> On Fri, Feb 17, 2017 at 12:11 PM, Thomas Garnier <thgarnie@google.com> wrote:
>>> On Fri, Feb 17, 2017 at 9:49 AM, Jim Mattson <jmattson@google.com> wrote:
>>>>
>>>> Can we use the read-only GDT here? When expanding the virtual address
>>>> for 64-bit system descriptors, isn't it sufficient to check (d->s == 0
>>>> && d->type != 0)?
>>>
>>> We can use the readonly GDT but I think doesn't matter one or the
>>> other here. We have to check specific types for LDT or TSS, other
>>> values describe other entries (cf Intel volume 3, 3.5) (for example 14
>>> & 15 on 64-bits are for trap & interrupt gates).
>>
>> According to volume 3 of the SDM, section 3.5.2:
>>
>> The following system descriptors expand to 16 bytes:
>> — Call gate descriptors (see Section 5.8.3.1, “IA-32e Mode Call Gates”)
>> — IDT gate descriptors (see Section 6.14.1, “64-Bit Mode IDT”)
>> — LDT and TSS descriptors (see Section 7.2.3, “TSS Descriptor in 64-bit mode”).
>>
>> All legal system descriptor types (except for 0: Upper 8 bytes of an 16-byte
>> descriptor) should get the high 32 bits of the base address from the next 8-byte
>> descriptor.
>>
>
> Ok, then I will test an updated version next week.
>

I'm going to send out some preliminary patches that just get rid of
this problem entirely.
Thomas Garnier Feb. 20, 2017, 5:28 p.m. UTC | #8
On Mon, Feb 20, 2017 at 8:56 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Fri, Feb 17, 2017 at 2:01 PM, Thomas Garnier <thgarnie@google.com> wrote:
>> On Fri, Feb 17, 2017 at 1:00 PM, Jim Mattson <jmattson@google.com> wrote:
>>> On Fri, Feb 17, 2017 at 12:11 PM, Thomas Garnier <thgarnie@google.com> wrote:
>>>> On Fri, Feb 17, 2017 at 9:49 AM, Jim Mattson <jmattson@google.com> wrote:
>>>>>
>>>>> Can we use the read-only GDT here? When expanding the virtual address
>>>>> for 64-bit system descriptors, isn't it sufficient to check (d->s == 0
>>>>> && d->type != 0)?
>>>>
>>>> We can use the readonly GDT but I think doesn't matter one or the
>>>> other here. We have to check specific types for LDT or TSS, other
>>>> values describe other entries (cf Intel volume 3, 3.5) (for example 14
>>>> & 15 on 64-bits are for trap & interrupt gates).
>>>
>>> According to volume 3 of the SDM, section 3.5.2:
>>>
>>> The following system descriptors expand to 16 bytes:
>>> — Call gate descriptors (see Section 5.8.3.1, “IA-32e Mode Call Gates”)
>>> — IDT gate descriptors (see Section 6.14.1, “64-Bit Mode IDT”)
>>> — LDT and TSS descriptors (see Section 7.2.3, “TSS Descriptor in 64-bit mode”).
>>>
>>> All legal system descriptor types (except for 0: Upper 8 bytes of an 16-byte
>>> descriptor) should get the high 32 bits of the base address from the next 8-byte
>>> descriptor.
>>>
>>
>> Ok, then I will test an updated version next week.
>>
>
> I'm going to send out some preliminary patches that just get rid of
> this problem entirely.

Okay, I guess I will have to wait for it to be integrated to
linux-next then. Or would you rather to it after this patch set is
added?
Thomas Garnier Feb. 20, 2017, 5:39 p.m. UTC | #9
On Mon, Feb 20, 2017 at 9:28 AM, Thomas Garnier <thgarnie@google.com> wrote:
> On Mon, Feb 20, 2017 at 8:56 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Fri, Feb 17, 2017 at 2:01 PM, Thomas Garnier <thgarnie@google.com> wrote:
>>> On Fri, Feb 17, 2017 at 1:00 PM, Jim Mattson <jmattson@google.com> wrote:
>>>> On Fri, Feb 17, 2017 at 12:11 PM, Thomas Garnier <thgarnie@google.com> wrote:
>>>>> On Fri, Feb 17, 2017 at 9:49 AM, Jim Mattson <jmattson@google.com> wrote:
>>>>>>
>>>>>> Can we use the read-only GDT here? When expanding the virtual address
>>>>>> for 64-bit system descriptors, isn't it sufficient to check (d->s == 0
>>>>>> && d->type != 0)?
>>>>>
>>>>> We can use the readonly GDT but I think doesn't matter one or the
>>>>> other here. We have to check specific types for LDT or TSS, other
>>>>> values describe other entries (cf Intel volume 3, 3.5) (for example 14
>>>>> & 15 on 64-bits are for trap & interrupt gates).
>>>>
>>>> According to volume 3 of the SDM, section 3.5.2:
>>>>
>>>> The following system descriptors expand to 16 bytes:
>>>> — Call gate descriptors (see Section 5.8.3.1, “IA-32e Mode Call Gates”)
>>>> — IDT gate descriptors (see Section 6.14.1, “64-Bit Mode IDT”)
>>>> — LDT and TSS descriptors (see Section 7.2.3, “TSS Descriptor in 64-bit mode”).
>>>>
>>>> All legal system descriptor types (except for 0: Upper 8 bytes of an 16-byte
>>>> descriptor) should get the high 32 bits of the base address from the next 8-byte
>>>> descriptor.
>>>>
>>>
>>> Ok, then I will test an updated version next week.
>>>
>>
>> I'm going to send out some preliminary patches that just get rid of
>> this problem entirely.
>
> Okay, I guess I will have to wait for it to be integrated to
> linux-next then. Or would you rather to it after this patch set is
> added?
>

Read your summary for the patchset of KVM cleanup, I will wait for it
to reach linux-next to rebase and send the new iteration.

Thanks for working on the clean-up.

> --
> Thomas
Ingo Molnar Feb. 21, 2017, 8:03 a.m. UTC | #10
* Thomas Garnier <thgarnie@google.com> wrote:

> > Okay, I guess I will have to wait for it to be integrated to
> > linux-next then. Or would you rather to it after this patch set is
> > added?
> 
> Read your summary for the patchset of KVM cleanup, I will wait for it to reach 
> linux-next to rebase and send the new iteration.

Note that to be able to apply your readonly-GDT series to the x86 tree I'll need a 
stable SHA1 to base it on.

Paolo, how stable, non-rebasing are the KVM tree commits? Once Andy's patches hit 
the KVM tree I could pull from you and order it so that I'd send it to Linus only 
after you sent your bits upstream. That would save us 3 months of patch 
propagation latency.

Or should we keep Andy's KVM patches together with the GDT patches? Either 
workflow works for me - it's your call as these are predominantly KVM changes.

Thanks,

	Ingo
Paolo Bonzini Feb. 21, 2017, 10:28 a.m. UTC | #11
> Paolo, how stable, non-rebasing are the KVM tree commits?

Whatever ends in linux-next is stable.  I have a separate rebasing branch,
but it's not part of linux-next by design.

> Or should we keep Andy's KVM patches together with the GDT patches? Either
> workflow works for me - it's your call as these are predominantly KVM
> changes.

I'll delay my pull request to Linus a couple days so that I can test
Andy's 6 patches. Then you can just base your branch on Linus's tree.

Paolo
Ingo Molnar Feb. 22, 2017, 6:34 a.m. UTC | #12
* Paolo Bonzini <pbonzini@redhat.com> wrote:

> > Paolo, how stable, non-rebasing are the KVM tree commits?
> 
> Whatever ends in linux-next is stable.  I have a separate rebasing branch,
> but it's not part of linux-next by design.

Ok, that's nice!

> > Or should we keep Andy's KVM patches together with the GDT patches? Either 
> > workflow works for me - it's your call as these are predominantly KVM changes.
> 
> I'll delay my pull request to Linus a couple days so that I can test Andy's 6 
> patches. Then you can just base your branch on Linus's tree.

Fantastic, thank you!

	Ingo
diff mbox

Patch

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 99167f20bc34..edb8326108dd 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2062,25 +2062,35 @@  static unsigned long segment_base(u16 selector)
 	struct desc_struct *d;
 	unsigned long table_base;
 	unsigned long v;
+	u32 high32;
 
-	if (!(selector & ~3))
+	if (!(selector & ~SEGMENT_RPL_MASK))
 		return 0;
 
-	table_base = get_current_gdt_rw_vaddr();
-
-	if (selector & 4) {           /* from ldt */
+	/* LDT selector */
+	if ((selector & SEGMENT_TI_MASK) == SEGMENT_LDT) {
 		u16 ldt_selector = kvm_read_ldt();
 
-		if (!(ldt_selector & ~3))
+		if (!(ldt_selector & ~SEGMENT_RPL_MASK))
 			return 0;
 
 		table_base = segment_base(ldt_selector);
+	} else {
+		table_base = get_current_gdt_rw_vaddr();
 	}
-	d = (struct desc_struct *)(table_base + (selector & ~7));
+
+	d = (struct desc_struct *)table_base + (selector >> 3);
 	v = get_desc_base(d);
 #ifdef CONFIG_X86_64
-       if (d->s == 0 && (d->type == 2 || d->type == 9 || d->type == 11))
-               v |= ((unsigned long)((struct ldttss_desc64 *)d)->base3) << 32;
+	/*
+	 * Extend the virtual address if we have a system descriptor entry for
+	 * LDT or TSS (available or busy).
+	 */
+	if (d->s == 0 && (d->type == DESC_LDT || d->type == DESC_TSS ||
+			  d->type == 11/*Busy TSS */)) {
+		high32 = ((struct ldttss_desc64 *)d)->base3;
+		v |= (u64)high32 << 32;
+	}
 #endif
 	return v;
 }