question about patch 13252

question about patch 13252

[Lu, Guanqun]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

Hi Jan,

I saw that in patch 13252,
you added such lines in xen/arch/x86/traps.c

+#ifdef CONFIG_COMPAT
+    _set_tssldt_desc(
+        compat_gdt_table + __TSS(n) - FIRST_RESERVED_GDT_ENTRY,
+        (unsigned long)addr,
+        offsetof(struct tss_struct, __cacheline_filler) - 1,
+        11);
+#endif

I have such question about the number 11, 11 is 1010 in binary format,
which means that the busy flat is set.  Then later, load_TR() is called.
load_TR() is a wrapper around instruction 'ltr'.  As I consult SDM2A,
it says that ltr will generate #GP, when the busy flag is set.

So I'm a little puzzled. Can you explain a little why it's not 9 ?
Or am I missing something here?

Thanks.

--
Guanqun

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

RE: question about patch 13252

[Lu, Guanqun]

[-- Attachment #1: Type: text/plain, Size: 861 bytes --]

On Thursday, March 19, 2009 10:08 PM Lu, Guanqun wrote:

> Hi Jan,
> 
> I saw that in patch 13252,
> you added such lines in xen/arch/x86/traps.c
> 
> +#ifdef CONFIG_COMPAT
> +    _set_tssldt_desc(
> +        compat_gdt_table + __TSS(n) - FIRST_RESERVED_GDT_ENTRY,
> +        (unsigned long)addr,
> +        offsetof(struct tss_struct, __cacheline_filler) - 1,
> +        11);
> +#endif
> 
> I have such question about the number 11, 11 is 1010 in binary format,

typo. 1011  ;-)

> which means that the busy flat is set.  Then later, load_TR() is
> called. load_TR() is a wrapper around instruction 'ltr'.  As I
> consult SDM2A, it says that ltr will generate #GP, when the busy flag
> is set. 
> 
> So I'm a little puzzled. Can you explain a little why it's not 9 ?
> Or am I missing something here?
> 
> Thanks.



-- 
Guanqun

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: question about patch 13252

[Keir Fraser]

On 19/03/2009 14:07, "Lu, Guanqun" <guanqun.lu@intel.com> wrote:

> I have such question about the number 11, 11 is 1010 in binary format,

Try again. :D

> which means that the busy flat is set.  Then later, load_TR() is called.
> load_TR() is a wrapper around instruction 'ltr'.  As I consult SDM2A,
> it says that ltr will generate #GP, when the busy flag is set.
> 
> So I'm a little puzzled. Can you explain a little why it's not 9 ?
> Or am I missing something here?

We only execute LTR once at start of day for each CPU. This is done while
running on the non-compat gdt_table. When we switch to compat_gdt_table we
do not do any LTR switch. I suppose Jan sets the busy bit to match what will
be in the non-compat table after we have executed our one-off LTR.

 -- Keir

RE: question about patch 13252

[Lu, Guanqun]

[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]

On Thursday, March 19, 2009 10:23 PM Keir Fraser wrote:

> On 19/03/2009 14:07, "Lu, Guanqun" <guanqun.lu@intel.com> wrote:
> 
>> I have such question about the number 11, 11 is 1010 in binary
>> format, 
> 
> Try again. :D

Typo, it seems my fingers have their own thoughts...

> 
>> which means that the busy flat is set.  Then later, load_TR() is
>> called. load_TR() is a wrapper around instruction 'ltr'.  As I
>> consult SDM2A, it says that ltr will generate #GP, when the busy
>> flag is set. 
>> 
>> So I'm a little puzzled. Can you explain a little why it's not 9 ?
>> Or am I missing something here?
> 
> We only execute LTR once at start of day for each CPU. This is done
> while running on the non-compat gdt_table. When we switch to
> compat_gdt_table we do not do any LTR switch. I suppose Jan sets the
> busy bit to match what will be in the non-compat table after we have
> executed our one-off LTR. 
> 
>  -- Keir

Thanks for your reply.

It makes a little sense...
But the problem is that when we do S3,
we execute load_TR() again (in file arch/x86/acpi/suspend.c),
and this causes the bug.

Do we need to go back to non-compat gdt_table when it resumes, and 
switch to compat_gdt_table again?

-- 
Guanqun

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: question about patch 13252

[Keir Fraser]

On 19/03/2009 14:34, "Lu, Guanqun" <guanqun.lu@intel.com> wrote:

> Thanks for your reply.
> 
> It makes a little sense...
> But the problem is that when we do S3,
> we execute load_TR() again (in file arch/x86/acpi/suspend.c),
> and this causes the bug.
> 
> Do we need to go back to non-compat gdt_table when it resumes, and
> switch to compat_gdt_table again?

Ah, I see. There are a few options, the easiest of which is to leave the B
bit clear in both descriptors. I'm not certain whether that actually matters
for any reason, but I think for our purposes it does not.

If Jan can counter my claim, then you can instead switch back to the
non-compat GDT for the LTR, or you can decide which descriptor to set B in
based on which GDT you're running on, or force the B bit in both descriptors
after the LTR, or... You have a few options. :-)

 -- Keir

Re: question about patch 13252

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 19.03.09 15:49 >>>
>On 19/03/2009 14:34, "Lu, Guanqun" <guanqun.lu@intel.com> wrote:
>
>> Thanks for your reply.
>> 
>> It makes a little sense...
>> But the problem is that when we do S3,
>> we execute load_TR() again (in file arch/x86/acpi/suspend.c),
>> and this causes the bug.
>> 
>> Do we need to go back to non-compat gdt_table when it resumes, and
>> switch to compat_gdt_table again?

There must be code clearing the B bit in the non-compat GDT already,
otherwise loading of the LDTR would always have faulted.

>Ah, I see. There are a few options, the easiest of which is to leave the B
>bit clear in both descriptors. I'm not certain whether that actually matters
>for any reason, but I think for our purposes it does not.

No, that's not an option afaict: On an outgoing TSS (i.e. during a double
fault) the B bit must be set iirc.

>If Jan can counter my claim, then you can instead switch back to the
>non-compat GDT for the LTR, or you can decide which descriptor to set B in
>based on which GDT you're running on, or force the B bit in both descriptors
>after the LTR, or... You have a few options. :-)

Correct. I wonder why you're running on the compat GDT in the first place -
is your Dom0 32-bit? If that's the case, then the clearing of the bit that must
exist somewhere simply should be extended to touch the current GDT
rather than the default one.

Jan

Re: question about patch 13252

[Keir Fraser]

On 19/03/2009 14:58, "Jan Beulich" <jbeulich@novell.com> wrote:

>> If Jan can counter my claim, then you can instead switch back to the
>> non-compat GDT for the LTR, or you can decide which descriptor to set B in
>> based on which GDT you're running on, or force the B bit in both descriptors
>> after the LTR, or... You have a few options. :-)
> 
> Correct. I wonder why you're running on the compat GDT in the first place -
> is your Dom0 32-bit? If that's the case, then the clearing of the bit that
> must
> exist somewhere simply should be extended to touch the current GDT
> rather than the default one.

I fixed this issue as c/s 19400. I just temporarily switch over to the
non-compat GDT to do the LTR. Guanqun: please can you test this?

 -- Keir

RE: question about patch 13252

[Lu, Guanqun]

On Friday, March 20, 2009 1:07 AM Keir Fraser wrote:

> On 19/03/2009 14:58, "Jan Beulich" <jbeulich@novell.com> wrote:
> 
>>> If Jan can counter my claim, then you can instead switch back to the
>>> non-compat GDT for the LTR, or you can decide which descriptor to
>>> set B in based on which GDT you're running on, or force the B bit
>>> in both descriptors after the LTR, or... You have a few options. :-)
>> 
>> Correct. I wonder why you're running on the compat GDT in the first
>> place - is your Dom0 32-bit? If that's the case, then the clearing
>> of the bit that must exist somewhere simply should be extended to
>> touch the current GDT 
>> rather than the default one.
> 
> I fixed this issue as c/s 19400. I just temporarily switch over to the
> non-compat GDT to do the LTR. Guanqun: please can you test this?
> 
>  -- Keir

Hi Keir,

I tested with your patch, and in 32/32 xen/dom0 stack, S3 woks fine.
but with 64/32 xen/dom0 stack, when S3 resumes back, it complains:
	INIT: PANIC: segmentation violation at 0xb7f36405! sleeping for 30 seconds.
and basically the shell is usable, but 'make' always got a seg fault.

The phenomenon is very similar to justing changing the number 11 to 9.

Any idea?

-- 
Guanqun

RE: question about patch 13252

[Lu, Guanqun]

On Friday, March 20, 2009 1:07 AM Keir Fraser wrote:

> On 19/03/2009 14:58, "Jan Beulich" <jbeulich@novell.com> wrote:
> 
>>> If Jan can counter my claim, then you can instead switch back to the
>>> non-compat GDT for the LTR, or you can decide which descriptor to
>>> set B in based on which GDT you're running on, or force the B bit
>>> in both descriptors after the LTR, or... You have a few options. :-)
>> 
>> Correct. I wonder why you're running on the compat GDT in the first
>> place - is your Dom0 32-bit? If that's the case, then the clearing
>> of the bit that must exist somewhere simply should be extended to
>> touch the current GDT 
>> rather than the default one.
> 
> I fixed this issue as c/s 19400. I just temporarily switch over to the
> non-compat GDT to do the LTR. Guanqun: please can you test this?
> 
>  -- Keir

BTW:

the patch 19400 is incomplete:


diff -r e1562a36094e xen/arch/x86/acpi/suspend.c
--- a/xen/arch/x86/acpi/suspend.c   Thu Mar 19 17:04:06 2009 +0000
+++ b/xen/arch/x86/acpi/suspend.c   Fri Mar 20 13:27:17 2009 +0800
@@ -32,6 +32,9 @@ void restore_rest_processor_state(void)
 void restore_rest_processor_state(void)
 {
     struct vcpu *v = current;
+#if !defined(CONFIG_X86_64)
+    struct tss_struct *t = &init_tss[smp_processor_id()];
+#endif

     load_TR();


-- 
Guanqun

Re: question about patch 13252

[Jan Beulich]

>>> "Lu, Guanqun" <guanqun.lu@intel.com> 19.03.09 15:07 >>>
>I saw that in patch 13252,
>you added such lines in xen/arch/x86/traps.c
>
>+#ifdef CONFIG_COMPAT
>+    _set_tssldt_desc(
>+        compat_gdt_table + __TSS(n) - FIRST_RESERVED_GDT_ENTRY,
>+        (unsigned long)addr,
>+        offsetof(struct tss_struct, __cacheline_filler) - 1,
>+        11);
>+#endif
>
>I have such question about the number 11, 11 is 1010 in binary format,
>which means that the busy flat is set.  Then later, load_TR() is called.
>load_TR() is a wrapper around instruction 'ltr'.  As I consult SDM2A,
>it says that ltr will generate #GP, when the busy flag is set.
>
>So I'm a little puzzled. Can you explain a little why it's not 9 ?
>Or am I missing something here?

You're not running on compat_gdt_table when doing the ltr, and when
you switch between GDTs the busy bits must be in sync.

Jan