x86/mm: Adapt MODULES_END based on Fixmap section size

arch/x86/xen/smp.c cpu_initialize_context(unsigned int cpu, struct task_struct *idle) · line 397

Quoted 13 lines · Thomas Garnier

arch/x86/xen/smp.c +1 -1

index 0dee6f59ea82..6399bab936cd 100644
@@ -391,7 +391,7 @@ cpu_initialize_context(unsigned int cpu, struct task_struct *idle)
      if (ctxt == NULL)
              return -ENOMEM;
 
-     gdt = get_cpu_gdt_table(cpu);
+     gdt = get_cpu_gdt_rw(cpu);
 
 #ifdef CONFIG_X86_32
      ctxt->user_regs.fs = __KERNEL_PERCPU;

Which tree are these patches against? The chunk above cannot be applied to current Linus tree but it can be applied against Xen staging tree that has not been pulled yet.

I can apply this manually but the series crashes PV guests so I want to make sure I am using right bits.

-boris

arch/x86/xen/smp.c cpu_initialize_context(unsigned int cpu, struct task_struct *idle) · line 397

Quoted 13 lines · Thomas Garnier

arch/x86/xen/smp.c +1 -1

index 0dee6f59ea82..6399bab936cd 100644
@@ -391,7 +391,7 @@ cpu_initialize_context(unsigned int cpu, struct task_struct *idle)
      if (ctxt == NULL)
              return -ENOMEM;
 
-     gdt = get_cpu_gdt_table(cpu);
+     gdt = get_cpu_gdt_rw(cpu);
 
 #ifdef CONFIG_X86_32
      ctxt->user_regs.fs = __KERNEL_PERCPU;

Which tree are these patches against? The chunk above cannot be applied to current Linus tree but it can be applied against Xen staging tree that has not been pulled yet.

I can apply this manually but the series crashes PV guests so I want to make sure I am using right bits.

-boris

The KVM segment_base function is confusing. This patch replaces integers with appropriate flags, simplify constructs and add comments.

Signed-off-by Thomas Garnier <[email protected]>

---
Based on next-20170213

---
 arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

arch/x86/kvm/vmx.c +0 -0

index 99167f20bc34..edb8326108dd 100644

Quoted 2 lines · Thomas Garnier

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.

Quoted 13 lines

Signed-off-by Thomas Garnier <[email protected]>

---
Based on next-20170213

---
 arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

arch/x86/kvm/vmx.c +0 -0

index 99167f20bc34..edb8326108dd 100644

arch/x86/kvm/vmx.c static unsigned long segment_base(u16 selector) · line 2063

Quoted 2 lines

arch/x86/kvm/vmx.c +0 -0

@@ -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;

arch/x86/kvm/vmx.c static unsigned long segment_base(u16 selector) · line 2079

Quoted 1 line

arch/x86/kvm/vmx.c +1 -0

+               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.

arch/x86/kvm/vmx.c static unsigned long segment_base(u16 selector) · line 2082

Quoted 1 line

arch/x86/kvm/vmx.c +1 -0

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

And this cast goes away too.

Quoted 2 lines · Thomas Garnier

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.

Quoted 13 lines

Signed-off-by Thomas Garnier <[email protected]>

---
Based on next-20170213

---
 arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

arch/x86/kvm/vmx.c +0 -0

index 99167f20bc34..edb8326108dd 100644

arch/x86/kvm/vmx.c static unsigned long segment_base(u16 selector) · line 2063

Quoted 2 lines

arch/x86/kvm/vmx.c +0 -0

@@ -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;

arch/x86/kvm/vmx.c static unsigned long segment_base(u16 selector) · line 2079

Quoted 1 line

arch/x86/kvm/vmx.c +1 -0

+               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.

arch/x86/kvm/vmx.c static unsigned long segment_base(u16 selector) · line 2082

Quoted 1 line

arch/x86/kvm/vmx.c +1 -0

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

And this cast goes away too.

The KVM segment_base function is confusing. This patch replaces integers with appropriate flags, simplify constructs and add comments.

Signed-off-by Thomas Garnier <[email protected]>

---
Based on next-20170213

---
 arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

arch/x86/kvm/vmx.c +0 -0

index 99167f20bc34..edb8326108dd 100644

The KVM segment_base function is confusing. This patch replaces integers with appropriate flags, simplify constructs and add comments.

Signed-off-by Thomas Garnier <[email protected]>

---
Based on next-20170213

---
 arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

arch/x86/kvm/vmx.c +18 -9

index 99167f20bc34..edb8326108dd 100644
@@ -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

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)?

Quoted 61 lines

The KVM segment_base function is confusing. This patch replaces integers with appropriate flags, simplify constructs and add comments.

Signed-off-by Thomas Garnier <[email protected]>

---
Based on next-20170213

---
 arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

arch/x86/kvm/vmx.c +18 -9

index 99167f20bc34..edb8326108dd 100644
@@ -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

Quoted 4 lines · Jim Mattson

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).

Quoted 57 lines · Thomas Garnier

Quoted 53 lines

The KVM segment_base function is confusing. This patch replaces integers with appropriate flags, simplify constructs and add comments.

Signed-off-by Thomas Garnier <[email protected]>

---
Based on next-20170213

---
 arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

arch/x86/kvm/vmx.c +16 -8

index 99167f20bc34..edb8326108dd 100644
@@ -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;

Quoted 8 lines

Quoted 8 lines

arch/x86/kvm/vmx.c +2 -1

+               v |= (u64)high32 << 32;
+       }
 #endif
        return v;
 }
--

2.11.0.483.g087da7b7c-goog

--
Thomas

Quoted 10 lines · Thomas Garnier

Quoted 4 lines · Jim Mattson

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.

Quoted 71 lines

Quoted 57 lines · Thomas Garnier

Quoted 53 lines

The KVM segment_base function is confusing. This patch replaces integers with appropriate flags, simplify constructs and add comments.

Signed-off-by Thomas Garnier <[email protected]>

---
Based on next-20170213

---
 arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

arch/x86/kvm/vmx.c +16 -8

index 99167f20bc34..edb8326108dd 100644
@@ -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;

Quoted 8 lines

Quoted 8 lines

arch/x86/kvm/vmx.c +2 -1

+               v |= (u64)high32 << 32;
+       }
 #endif
        return v;
 }
--

2.11.0.483.g087da7b7c-goog

--
Thomas

Quoted 19 lines · Jim Mattson

Quoted 10 lines · Thomas Garnier

Quoted 4 lines · Jim Mattson

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”).

Quoted 4 lines

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.

Quoted 25 lines · Thomas Garnier

Quoted 23 lines · Jim Mattson

Quoted 10 lines · Thomas Garnier

Quoted 4 lines · Jim Mattson

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.

Quoted 2 lines

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.

Quoted 29 lines · Andy Lutomirski

Quoted 27 lines · Thomas Garnier

Quoted 14 lines · Jim Mattson

Quoted 10 lines · Thomas Garnier

Quoted 4 lines · Jim Mattson

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:

Quoted 9 lines

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.

Quoted 2 lines

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

Quoted 3 lines

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.

Paolo, how stable, non-rebasing are the KVM tree commits?

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.

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 2 lines

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, how stable, non-rebasing are the KVM tree commits?

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.

Quoted 3 lines

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.

Quoted 29 lines · Andy Lutomirski

Quoted 27 lines · Thomas Garnier

Quoted 14 lines · Jim Mattson

Quoted 10 lines · Thomas Garnier

Quoted 4 lines · Jim Mattson

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:

Quoted 9 lines

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.

Quoted 2 lines

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

Quoted 25 lines · Thomas Garnier

Quoted 23 lines · Jim Mattson

Quoted 10 lines · Thomas Garnier

Quoted 4 lines · Jim Mattson

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.

Quoted 2 lines

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.

Quoted 19 lines · Jim Mattson

Quoted 10 lines · Thomas Garnier

Quoted 4 lines · Jim Mattson

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”).

Quoted 4 lines

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.

Quoted 10 lines · Thomas Garnier

Quoted 4 lines · Jim Mattson

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.

Quoted 71 lines

Quoted 57 lines · Thomas Garnier

Quoted 53 lines

The KVM segment_base function is confusing. This patch replaces integers with appropriate flags, simplify constructs and add comments.

Signed-off-by Thomas Garnier <[email protected]>

---
Based on next-20170213

---
 arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

arch/x86/kvm/vmx.c +16 -8

index 99167f20bc34..edb8326108dd 100644
@@ -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;

Quoted 8 lines

Quoted 8 lines

arch/x86/kvm/vmx.c +2 -1

+               v |= (u64)high32 << 32;
+       }
 #endif
        return v;
 }
--

2.11.0.483.g087da7b7c-goog

--
Thomas

Quoted 4 lines · Jim Mattson

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).

Quoted 63 lines · Thomas Garnier

Quoted 59 lines

The KVM segment_base function is confusing. This patch replaces integers with appropriate flags, simplify constructs and add comments.

Signed-off-by Thomas Garnier <[email protected]>

---
Based on next-20170213

---
 arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

arch/x86/kvm/vmx.c +18 -9

index 99167f20bc34..edb8326108dd 100644
@@ -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;
 }
--

Quoted 2 lines

Quoted 2 lines

2.11.0.483.g087da7b7c-goog

--
Thomas

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)?

Quoted 61 lines

The KVM segment_base function is confusing. This patch replaces integers with appropriate flags, simplify constructs and add comments.

Signed-off-by Thomas Garnier <[email protected]>

---
Based on next-20170213

---
 arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

arch/x86/kvm/vmx.c +18 -9

index 99167f20bc34..edb8326108dd 100644
@@ -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

The KVM segment_base function is confusing. This patch replaces integers with appropriate flags, simplify constructs and add comments.

Signed-off-by Thomas Garnier <[email protected]>

---
Based on next-20170213

---
 arch/x86/kvm/vmx.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

arch/x86/kvm/vmx.c +18 -9

index 99167f20bc34..edb8326108dd 100644
@@ -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

This patch aligns MODULES_END to the beginning of the Fixmap section. It optimizes the space available for both sections. The address is pre-computed based on the number of pages required by the Fixmap section.

It will allow GDT remapping in the Fixmap section. The current MODULES_END static address does not provide enough space for the kernel to support a large number of processors.

Signed-off-by Thomas Garnier <[email protected]>

---
Based on next-20170213

---
 arch/x86/include/asm/fixmap.h           | 8 ++++++++
 arch/x86/include/asm/pgtable_64_types.h | 3 ---
 arch/x86/kernel/module.c                | 1 +
 arch/x86/mm/dump_pagetables.c           | 1 +
 arch/x86/mm/kasan_init_64.c             | 1 +
 5 files changed, 11 insertions(+), 3 deletions(-)

arch/x86/include/asm/fixmap.h +0 -0

index 8554f960e21b..20231189e0e3 100644

Quoted 25 lines · Thomas Garnier

This patch aligns MODULES_END to the beginning of the Fixmap section. It optimizes the space available for both sections. The address is pre-computed based on the number of pages required by the Fixmap section.

It will allow GDT remapping in the Fixmap section. The current MODULES_END static address does not provide enough space for the kernel to support a large number of processors.

Signed-off-by Thomas Garnier <[email protected]>

---
Based on next-20170213

---
 arch/x86/include/asm/fixmap.h           | 8 ++++++++
 arch/x86/include/asm/pgtable_64_types.h | 3 ---
 arch/x86/kernel/module.c                | 1 +
 arch/x86/mm/dump_pagetables.c           | 1 +
 arch/x86/mm/kasan_init_64.c             | 1 +
 5 files changed, 11 insertions(+), 3 deletions(-)

arch/x86/include/asm/fixmap.h +0 -0

index 8554f960e21b..20231189e0e3 100644

arch/x86/include/asm/fixmap.h enum fixed_addresses { · line 140

Quoted 9 lines

arch/x86/include/asm/fixmap.h +6 -0

@@ -132,6 +132,14 @@ enum fixed_addresses {
 
 extern void reserve_top_address(unsigned long reserve);
 
+/* On 64-bit, the module sections ends with the start of the fixmap */
+#ifdef CONFIG_X86_64
+#define MODULES_VADDR    (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
+#define MODULES_END   __fix_to_virt(__end_of_fixed_addresses + 1)
+#define MODULES_LEN   (MODULES_END - MODULES_VADDR)
+#endif /* CONFIG_X86_64 */

JFYI: so there's another patchset which adds KERNEL_MAPPING_SIZE:

https://lkml.kernel.org/r/[email protected]

and makes it a 1G, i.e., the KASLR default. I guess the above will have to be KERNEL_MAPPING_SIZE then.

And why are you moving those to fixmap.h? What's wrong with including fixmap.h into pgtable_64_types.h so that you can get __end_of_fixed_addresses?

FWIW, I didn't even have to add any includes with my .config, i.e., that builds:

---

arch/x86/include/asm/pgtable_64_types.h +1 -2

index 3a264200c62f..eda7fa856fa9 100644
@@ -67,7 +67,7 @@ typedef struct { pteval_t pte; } pte_t;
 #endif /* CONFIG_RANDOMIZE_MEMORY */
 #define VMALLOC_END    (VMALLOC_START + _AC((VMALLOC_SIZE_TB << 40) - 1, UL))
 #define MODULES_VADDR    (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
-#define MODULES_END      _AC(0xffffffffff000000, UL)
+#define MODULES_END   __fix_to_virt(__end_of_fixed_addresses + 1)
 #define MODULES_LEN   (MODULES_END - MODULES_VADDR)
 #define ESPFIX_PGD_ENTRY _AC(-2, UL)
 #define ESPFIX_BASE_ADDR (ESPFIX_PGD_ENTRY << PGDIR_SHIFT)
---

but I wouldn't be surprised if some strange configuration would need it.

arch/x86/include/asm/pgtable_64_types.h typedef struct { pteval_t pte; } pte_t; · line 70

Quoted 13 lines

arch/x86/include/asm/fixmap.h +0 -0

 #define FIXADDR_SIZE (__end_of_permanent_fixed_addresses << PAGE_SHIFT)
 #define FIXADDR_START                (FIXADDR_TOP - FIXADDR_SIZE)
 

arch/x86/include/asm/pgtable_64_types.h +0 -2

index 3a264200c62f..de8bace10200 100644
@@ -66,9 +66,6 @@ typedef struct { pteval_t pte; } pte_t;
 #define VMEMMAP_START        __VMEMMAP_BASE
 #endif /* CONFIG_RANDOMIZE_MEMORY */
 #define VMALLOC_END  (VMALLOC_START + _AC((VMALLOC_SIZE_TB << 40) - 1, UL))
-#define MODULES_VADDR    (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
-#define MODULES_END      _AC(0xffffffffff000000, UL)

How much of an ABI breakage would that be? See
Documentation/x86/x86_64/mm.txt.

With my .config MODULES_END becomes 0xffffffffff1fe000 and it'll remain dynamic depending on .config. No idea how much in userspace relies on MODULES_END being static 0xffffffffff000000...

Hmm.

--
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --

Why do you think they rely on it being static? The VSYSCALL address is not changed for example.

Quoted 2 lines · Thomas Garnier

Why do you think they rely on it being static? The VSYSCALL address is not changed for example.

I don't know, that's why I'm asking first. Userspace is known to pick different aspects of the kernel and deciding to use them for whatever. And once it relies on that, there's no changing it.

Quoted 2 lines · Thomas Garnier

Why do you think they rely on it being static? The VSYSCALL address is not changed for example.

I don't know, that's why I'm asking first. Userspace is known to pick different aspects of the kernel and deciding to use them for whatever. And once it relies on that, there's no changing it.

Why do you think they rely on it being static? The VSYSCALL address is not changed for example.

Quoted 25 lines · Thomas Garnier

This patch aligns MODULES_END to the beginning of the Fixmap section. It optimizes the space available for both sections. The address is pre-computed based on the number of pages required by the Fixmap section.

It will allow GDT remapping in the Fixmap section. The current MODULES_END static address does not provide enough space for the kernel to support a large number of processors.

Signed-off-by Thomas Garnier <[email protected]>

---
Based on next-20170213

---
 arch/x86/include/asm/fixmap.h           | 8 ++++++++
 arch/x86/include/asm/pgtable_64_types.h | 3 ---
 arch/x86/kernel/module.c                | 1 +
 arch/x86/mm/dump_pagetables.c           | 1 +
 arch/x86/mm/kasan_init_64.c             | 1 +
 5 files changed, 11 insertions(+), 3 deletions(-)

arch/x86/include/asm/fixmap.h +0 -0

index 8554f960e21b..20231189e0e3 100644

arch/x86/include/asm/fixmap.h enum fixed_addresses { · line 140

Quoted 9 lines

arch/x86/include/asm/fixmap.h +6 -0

@@ -132,6 +132,14 @@ enum fixed_addresses {
 
 extern void reserve_top_address(unsigned long reserve);
 
+/* On 64-bit, the module sections ends with the start of the fixmap */
+#ifdef CONFIG_X86_64
+#define MODULES_VADDR    (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
+#define MODULES_END   __fix_to_virt(__end_of_fixed_addresses + 1)
+#define MODULES_LEN   (MODULES_END - MODULES_VADDR)
+#endif /* CONFIG_X86_64 */

JFYI: so there's another patchset which adds KERNEL_MAPPING_SIZE:

https://lkml.kernel.org/r/[email protected]

and makes it a 1G, i.e., the KASLR default. I guess the above will have to be KERNEL_MAPPING_SIZE then.

And why are you moving those to fixmap.h? What's wrong with including fixmap.h into pgtable_64_types.h so that you can get __end_of_fixed_addresses?

FWIW, I didn't even have to add any includes with my .config, i.e., that builds:

---

arch/x86/include/asm/pgtable_64_types.h +1 -2

index 3a264200c62f..eda7fa856fa9 100644
@@ -67,7 +67,7 @@ typedef struct { pteval_t pte; } pte_t;
 #endif /* CONFIG_RANDOMIZE_MEMORY */
 #define VMALLOC_END    (VMALLOC_START + _AC((VMALLOC_SIZE_TB << 40) - 1, UL))
 #define MODULES_VADDR    (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
-#define MODULES_END      _AC(0xffffffffff000000, UL)
+#define MODULES_END   __fix_to_virt(__end_of_fixed_addresses + 1)
 #define MODULES_LEN   (MODULES_END - MODULES_VADDR)
 #define ESPFIX_PGD_ENTRY _AC(-2, UL)
 #define ESPFIX_BASE_ADDR (ESPFIX_PGD_ENTRY << PGDIR_SHIFT)
---

but I wouldn't be surprised if some strange configuration would need it.

arch/x86/include/asm/pgtable_64_types.h typedef struct { pteval_t pte; } pte_t; · line 70

Quoted 13 lines

arch/x86/include/asm/fixmap.h +0 -0

 #define FIXADDR_SIZE (__end_of_permanent_fixed_addresses << PAGE_SHIFT)
 #define FIXADDR_START                (FIXADDR_TOP - FIXADDR_SIZE)
 

arch/x86/include/asm/pgtable_64_types.h +0 -2

index 3a264200c62f..de8bace10200 100644
@@ -66,9 +66,6 @@ typedef struct { pteval_t pte; } pte_t;
 #define VMEMMAP_START        __VMEMMAP_BASE
 #endif /* CONFIG_RANDOMIZE_MEMORY */
 #define VMALLOC_END  (VMALLOC_START + _AC((VMALLOC_SIZE_TB << 40) - 1, UL))
-#define MODULES_VADDR    (__START_KERNEL_map + KERNEL_IMAGE_SIZE)
-#define MODULES_END      _AC(0xffffffffff000000, UL)

How much of an ABI breakage would that be? See
Documentation/x86/x86_64/mm.txt.

With my .config MODULES_END becomes 0xffffffffff1fe000 and it'll remain dynamic depending on .config. No idea how much in userspace relies on MODULES_END being static 0xffffffffff000000...

Hmm.

--
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --

This patch aligns MODULES_END to the beginning of the Fixmap section. It optimizes the space available for both sections. The address is pre-computed based on the number of pages required by the Fixmap section.

It will allow GDT remapping in the Fixmap section. The current MODULES_END static address does not provide enough space for the kernel to support a large number of processors.

Signed-off-by Thomas Garnier <[email protected]>

---
Based on next-20170213

---
 arch/x86/include/asm/fixmap.h           | 8 ++++++++
 arch/x86/include/asm/pgtable_64_types.h | 3 ---
 arch/x86/kernel/module.c                | 1 +
 arch/x86/mm/dump_pagetables.c           | 1 +
 arch/x86/mm/kasan_init_64.c             | 1 +
 5 files changed, 11 insertions(+), 3 deletions(-)

arch/x86/include/asm/fixmap.h +0 -0

index 8554f960e21b..20231189e0e3 100644