lkml.org 
[lkml]   [2006]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [Fastboot] [RFC] [PATCH 10/16] x86_64: 64bit PIC ACPI wakeup
Hi!

> > > Fixed the resume problem happening on my second box which supported NX
> > > protection bit. Please find attached the regenerated patch.
> > >
> > > - Killed lots of dead code
> >
> > Cleanup. (a)
> >
> > > - Improve the cpu sanity checks to verify long mode
> > > is enabled when we wake up.
> >
> > Change. (b). I'm not sure if we really need this one. I do not think
> > replacing cpu while suspended is supported operation.
> >
>
> That's fine but it does not harm. Now all the entry path share the
> same sanity check (verify_cpu.S) and I believe it makes code more
> maintanable and more robust. It just makes our checks stronger in
> case somebody really replaces the cpus.

It is probably okay if shared.

> > > - Removed the need for modifying any existing kernel page table.
> >
> > Unrelated change, probably good one. (c).
> >
> > > - Moved wakeup_level4_pgt into the wakeup routine so we can
> > > run the kernel above 4G.
> >
> > The change you really wanted to do in the first place. (d).
> >
> > > - Increased the size of the wakeup routine to 8K.
> >
> > You want bigger stack or what? (e)
> >
>
> I think this is because of wakeup_level4_pgt page tables which are now
> part of trampoline. And these page tables got to be at 4K byte boundary.
> Hence now we need two pages for trampoline instead of one.

Aha, ok. Notice that in the changelog.

> > > - Renamed the variables to use the 64bit register names.
> >
> > Cleanup. (a)
> >
> > > - Lots of misc cleanups to match trampoline.S
> >
> > More cleanups. (a).
> >
> > Can we at least get (a) (b) (c) (d) and (e) separated?
>
> Ok. I will separate the patches.

Thanks!

> > > I don't have a configuration I can test this but it compiles cleanly
> > > and it should work, the code is very similar to the SMP trampoline,
> >
> > I assume you have configuration for test now?
>
> Eric did not have but now I have tested it already on two configurations.
> I think that's good enough. Isn't it?

Should be... if it is in -mm for long enough. Unfortunately very
little people are testing x86-64 on notebooks :-(. I guess I should
force myself to install 64-bit distro on old arima here...

> > > -static pgd_t low_ptr;
> > > -
> > > -static void init_low_mapping(void)
> > > -{
> > > - pgd_t *slot0 = pgd_offset(current->mm, 0UL);
> > > - low_ptr = *slot0;
> > > - set_pgd(slot0, *pgd_offset(current->mm, PAGE_OFFSET));
> > > - WARN_ON(num_online_cpus() != 1);
> > > - local_flush_tlb();
> > > -}
> > > -
> >
> > So you no longer need identity mapping? Is not it specified that when
> > you transition between modes, you should do that while in identity
> > mapping?
> >
>
> I am not sure where these mappings are required at all in first place?
> While going to sleep state? While resuming we are using wake page tables
> and they already got identity mappings so it should not be an issue.

Ok, I guess that's okay.

> > > @@ -228,25 +206,10 @@ wakeup_long64:
> > > .align 64
> > > gdta:
> > > .word 0, 0, 0, 0 # dummy
> > > -
> > > - .word 0, 0, 0, 0 # unused
> > > -
> > > - .word 0xFFFF # 4Gb - (0x100000*0x1000 = 4Gb)
> > > - .word 0 # base address = 0
> > > - .word 0x9B00 # code read/exec. ??? Why I need 0x9B00 (as opposed to 0x9A00 in order for this to work?)
> > > - .word 0x00CF # granularity = 4096, 386
> > > - # (+5th nibble of limit)
> > > -
> > > - .word 0xFFFF # 4Gb - (0x100000*0x1000 = 4Gb)
> > > - .word 0 # base address = 0
> > > - .word 0x9200 # data read/write
> > > - .word 0x00CF # granularity = 4096, 386
> > > - # (+5th nibble of limit)
> > > -# this is 64bit descriptor for code
> > > - .word 0xFFFF
> > > - .word 0
> > > - .word 0x9A00 # code read/exec
> > > - .word 0x00AF # as above, but it is long mode and with D=0
> > > + /* ??? Why I need the accessed bit set in order for this to work? */
> > > + .quad 0x00cf9b000000ffff # __KERNEL32_CS
> > > + .quad 0x00af9b000000ffff # __KERNEL_CS
> > > + .quad 0x00cf93000000ffff # __KERNEL_DS
> >
> > Why this change, why did you change the values in here, and why you
> > did not tell me about it in the changelog?
>
> I think mainly it has been modified to be consistent gdt table across
> the kernel (cpu_gdt_table, trampoline.S and wakeup.S). Now __KERNEL_32_CS
> entry has been moved up to keep the size of gdt small on trampoline. This
> change was done in patch number 7 (cleanup segments).
>
> Seondly, I think it is just change of form from using .word to .quad. More
> compact form.
>
> Thirdly I think it does not harm marking that gdt entry has been accessed.
> Eric can elaborate more on it. Patch 7 also has got details.

Ok. Maybe it would be nice to #include the GDT's, too, so they do not
drift apart? Or at least comment "this has to be kept in sync
with..."?

Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-11-16 22:55    [W:0.098 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site