[PATCH] switch to a known good/static GDT before kexec

[PATCH] switch to a known good/static GDT before kexec

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1246524221 -3600
# Node ID 31002ac6a13caadab7c163d387af558a2b2da7ce
# Parent  8e18dd41c6c7bb0980b29393b275c564cfb96437
switch to a known good/static GDT before kexec

kexec has been failing (at least on 32on64, didn't try others) since
18771:8e18dd41c6c7 "x86: reduce GDT switching". Ensure that we are
using a known good GDT before attempting to switch to compatability mode.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 8e18dd41c6c7 -r 31002ac6a13c xen/arch/x86/machine_kexec.c
--- a/xen/arch/x86/machine_kexec.c	Wed Nov 12 12:01:35 2008 +0000
+++ b/xen/arch/x86/machine_kexec.c	Thu Jul 02 09:43:41 2009 +0100
@@ -115,6 +115,13 @@
 
 void machine_kexec(xen_kexec_image_t *image)
 {
+    struct desc_ptr gdt_desc = {
+        .base = (unsigned long)(boot_cpu_gdt_table - FIRST_RESERVED_GDT_ENTRY),
+        .limit = LAST_RESERVED_GDT_BYTE
+    };
+
+    asm volatile ( "lgdt %0" : : "m" (gdt_desc) );
+
 #ifdef CONFIG_COMPAT
     if ( is_pv_32on64_domain(dom0) )
     {

Re: [PATCH] switch to a known good/static GDT before kexec

[Keir Fraser]

On 02/07/2009 09:45, "Ian Campbell" <Ian.Campbell@eu.citrix.com> wrote:

> switch to a known good/static GDT before kexec
> 
> kexec has been failing (at least on 32on64, didn't try others) since
> 18771:8e18dd41c6c7 "x86: reduce GDT switching". Ensure that we are
> using a known good GDT before attempting to switch to compatability mode.

I thought 18771 still left us always running on a good GDT, as least as far
as Xen's own limited needs are concerned. I'd like this in for 3.4.1 of
course, but I'd also like to understand why the fix is required (and whether
this is the best fix).

 -- Keir

Re: Re: [PATCH] switch to a known good/static GDT before kexec

[Keir Fraser]

On 02/07/2009 10:18, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

>> switch to a known good/static GDT before kexec
>> 
>> kexec has been failing (at least on 32on64, didn't try others) since
>> 18771:8e18dd41c6c7 "x86: reduce GDT switching". Ensure that we are
>> using a known good GDT before attempting to switch to compatability mode.
> 
> I thought 18771 still left us always running on a good GDT, as least as far
> as Xen's own limited needs are concerned. I'd like this in for 3.4.1 of
> course, but I'd also like to understand why the fix is required (and whether
> this is the best fix).

Jan,

Thinking about this raises another question. Since your patch to allow
arbitrary numbers of VCPUs per domain, does the GDT logic in
__context_switch() work any more? What if you have a next vcpu n for which
!need_full_gdt(n) but where its domain does not have a vcpu_id as large as
p->vcpu_id? Won't you end up with a GDTR pointing at a non-existent mapping
('off the end' of n->domain's per-domain mappings)?

 -- Keir

Re: Re: [PATCH] switch to a known good/static GDT before kexec

[Keir Fraser]

On 02/07/2009 11:52, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

> Thinking about this raises another question. Since your patch to allow
> arbitrary numbers of VCPUs per domain, does the GDT logic in
> __context_switch() work any more? What if you have a next vcpu n for which
> !need_full_gdt(n) but where its domain does not have a vcpu_id as large as
> p->vcpu_id? Won't you end up with a GDTR pointing at a non-existent mapping
> ('off the end' of n->domain's per-domain mappings)?

Okay, having read through __context_switch() in detail with Ian Campbell, we
decided the logic there is correct. If you switch to a !need_full_gdt() vcpu
then you always switch back to the default gdt_table, so the per-domain
mappings do not matter. This also explains why kexec now needs to forcibly
lgdt -- when it forcibly switches back to idle_pg_table the per-domain
mappings will not be there and so we fault on next segment reload.

I have one more question then -- when need_full_gdt(n), why do we always
unconditionally rewrite the l1es mapping the reserved gdt entries? Shouldn't
that be done on vcpu creation only and perhaps on compat mode switch (i.e.,
isn't it wasteful to do it in the context switch path)?

 -- Keir

Re: Re: [PATCH] switch to a known good/static GDT beforekexec

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 02.07.09 13:11 >>>
>I have one more question then -- when need_full_gdt(n), why do we always
>unconditionally rewrite the l1es mapping the reserved gdt entries? Shouldn't
>that be done on vcpu creation only and perhaps on compat mode switch (i.e.,
>isn't it wasteful to do it in the context switch path)?

While that's correct, we're talking about a single memory write here (plus
the setup code), which I think is no more wasteful than adding the extra
checking that would be needed to see whether we switch between
compat and !compat (including the !need_full_gdt(p) case, where we
don't know what kind of mapping we currently have).

Jan

Re: Re: [PATCH] switch to a known good/static GDT beforekexec

[Keir Fraser]

On 02/07/2009 12:45, "Jan Beulich" <JBeulich@novell.com> wrote:

>>>> Keir Fraser <keir.fraser@eu.citrix.com> 02.07.09 13:11 >>>
>> I have one more question then -- when need_full_gdt(n), why do we always
>> unconditionally rewrite the l1es mapping the reserved gdt entries? Shouldn't
>> that be done on vcpu creation only and perhaps on compat mode switch (i.e.,
>> isn't it wasteful to do it in the context switch path)?
> 
> While that's correct, we're talking about a single memory write here (plus
> the setup code), which I think is no more wasteful than adding the extra
> checking that would be needed to see whether we switch between
> compat and !compat (including the !need_full_gdt(p) case, where we
> don't know what kind of mapping we currently have).

Hm yes, fair enough.

 -- Keir

Re: Re: [PATCH] switch to a known good/static GDT beforekexec

[Keir Fraser]

On 02/07/2009 13:38, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

>>> I have one more question then -- when need_full_gdt(n), why do we always
>>> unconditionally rewrite the l1es mapping the reserved gdt entries? Shouldn't
>>> that be done on vcpu creation only and perhaps on compat mode switch (i.e.,
>>> isn't it wasteful to do it in the context switch path)?
>> 
>> While that's correct, we're talking about a single memory write here (plus
>> the setup code), which I think is no more wasteful than adding the extra
>> checking that would be needed to see whether we switch between
>> compat and !compat (including the !need_full_gdt(p) case, where we
>> don't know what kind of mapping we currently have).
> 
> Hm yes, fair enough.

Actually no, I'm not sure what extra checking you think you'd need? You'd
create the per-vcpu mapping when a vcpu is created and when it changes
to/from compat mode, and that would be it wouldn't it? You'd only delete
code from __context_switch(); not add different code to it?

 -- Keir

Re: Re: [PATCH] switch to a known good/static GDTbeforekexec

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 02.07.09 21:59 >>>
>On 02/07/2009 13:38, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:
>
>>>> I have one more question then -- when need_full_gdt(n), why do we always
>>>> unconditionally rewrite the l1es mapping the reserved gdt entries? Shouldn't
>>>> that be done on vcpu creation only and perhaps on compat mode switch (i.e.,
>>>> isn't it wasteful to do it in the context switch path)?
>>> 
>>> While that's correct, we're talking about a single memory write here (plus
>>> the setup code), which I think is no more wasteful than adding the extra
>>> checking that would be needed to see whether we switch between
>>> compat and !compat (including the !need_full_gdt(p) case, where we
>>> don't know what kind of mapping we currently have).
>> 
>> Hm yes, fair enough.
>
>Actually no, I'm not sure what extra checking you think you'd need? You'd
>create the per-vcpu mapping when a vcpu is created and when it changes
>to/from compat mode, and that would be it wouldn't it? You'd only delete
>code from __context_switch(); not add different code to it?

While you're right that my explanation wasn't really correct, moving this
back to the vCPU creation/mode-switching functions doesn't work because
the GDT is now per-CPU (and it's that patch where this code got added to
context switch): The installed mapping depends on the *physical* CPU the
code is running on. So the extra dependency that might be added here
would be to check whether the physical CPU the vCPU is running on
changed, but afair v->processor gets set before entering context_switch(),
so we'd have to additionally capture and store the last used value. Would
you think that's worth it?

Jan

Re: Re: [PATCH] switch to a known good/static GDTbeforekexec

[Keir Fraser]

On 03/07/2009 08:15, "Jan Beulich" <JBeulich@novell.com> wrote:

>> Actually no, I'm not sure what extra checking you think you'd need? You'd
>> create the per-vcpu mapping when a vcpu is created and when it changes
>> to/from compat mode, and that would be it wouldn't it? You'd only delete
>> code from __context_switch(); not add different code to it?
> 
> While you're right that my explanation wasn't really correct, moving this
> back to the vCPU creation/mode-switching functions doesn't work because
> the GDT is now per-CPU (and it's that patch where this code got added to
> context switch): The installed mapping depends on the *physical* CPU the
> code is running on. So the extra dependency that might be added here
> would be to check whether the physical CPU the vCPU is running on
> changed, but afair v->processor gets set before entering context_switch(),
> so we'd have to additionally capture and store the last used value. Would
> you think that's worth it?

Ok, now I get it. I think. There's no way to avoid potentially having to
rewrite the PTE on context switch, so may as well always do it.

 -- Keir