Messages in this thread | | | From | "Rafael J. Wysocki" <> | Subject | Re: [rft] s2ram wakeup moves to .c, could fix few machines | Date | Fri, 8 Feb 2008 00:06:53 +0100 |
| |
On Thursday, 7 of February 2008, H. Peter Anvin wrote: > Rafael J. Wysocki wrote: > > Index: linux-2.6/arch/x86/kernel/acpi/realmode/wakeup.S > > =================================================================== > > --- /dev/null > > +++ linux-2.6/arch/x86/kernel/acpi/realmode/wakeup.S > > @@ -0,0 +1,122 @@ > > +/* > > + * ACPI wakeup real mode startup stub > > + */ > > +#include <asm/segment.h> > > +#include <asm/msr-index.h> > > +#include <asm/page_64.h> > > +#include <asm/pgtable_64.h> > > + > > + .code16 > > + .section ".header", "a" > > + > > +/* This should match the structure in wakeup.h */ > > + .globl wakeup_header > > +wakeup_header: > > +video_mode: .short 0 /* Video mode number */ > > +pmode_return: .byte 0x66, 0xea /* ljmpl */ > > + .long 0 /* offset goes here */ > > + .short __KERNEL_CS > > Missing a .short pad here... Pavel fixed that at some point, I thought.
Hm, the struct in wakeup.h doesn't contain it too. Why exactly is it necessary?
[--snip--] > > } > > - memcpy((void *)acpi_wakeup_address, &wakeup_start, > > - &wakeup_end - &wakeup_start); > > - acpi_copy_wakeup_routine(acpi_wakeup_address); > > + memcpy((void *)acpi_realmode, &wakeup_code_start, 4*PAGE_SIZE); > > Using a PAGE_SIZE multiplier here isn't a good thing...
Yes, I'll fix that in one of the next iterations.
> > > + header = (struct wakeup_header *)(acpi_realmode + 0x3f00); > > ... especially not with magic constants like this.
Yeah. Pavel, what's at 0x3f00, btw?
> If you're putting the "header" at the end, then you should also replace > the end_signature stuff since that's, then, redundant.
In fact, I'd prefer to remove .signature from the header and use the end_signature only, so that I can use struct wakeup_header for addressing the header fields in the assembly too.
> Furthermore, by doing so, you're also padding the binary out to its maximum > length, so you might as well just remove the .bss-clearing stuff.
Do you mean placing the header at the end will fill the area between it and the code with zeros, so the .bss clearing is not necessary?
> It's not really clear to me why to do this what way... > > > + if (header->signature != 0x51ee1111) { > > + printk(KERN_ERR "wakeup header does not match\n"); > > + return -EINVAL; > > + } > > + > > + header->video_mode = saved_video_mode; > > + > > +#ifndef CONFIG_64BIT > > + store_gdt(&header->pmode_gdt); > > + > > + header->pmode_efer_low = nx_enabled; > > + if (header->pmode_efer_low & 1) { > > + /* This is strange, why not save efer, always? */ > > + rdmsr(MSR_EFER, header->pmode_efer_low, > > + header->pmode_efer_high); > > + } > > Yes, why not save EFER every time?
Well, I think we can do that.
Rafael
| |