Xen devel - Jul 2009 - [PATCH] switch to a known good/static GDT before kexec

# 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) )
     {

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel