Messages in this thread Patch in this message | | | Date | Thu, 13 Aug 2009 08:08:11 -0700 | From | "H. Peter Anvin" <> | Subject | Re: [PATCH 1/1] intel_txt: to fix build errors of CONFIG_ACPI_SLEEP |
| |
On 08/12/2009 11:46 PM, Ingo Molnar wrote: > * Wang, Shane <shane.wang@intel.com> wrote: > >> Signed-off-by: Shane Wang <shane.wang@intel.com> >> >> diff -r 7358cf1b3090 arch/x86/kernel/tboot.c >> --- a/arch/x86/kernel/tboot.c Wed Aug 12 03:04:23 2009 -0700 >> +++ b/arch/x86/kernel/tboot.c Wed Aug 12 18:06:21 2009 -0700 >> @@ -169,6 +169,7 @@ void tboot_create_trampoline(void) >> >> static void set_mac_regions(void) >> { >> +#ifdef CONFIG_ACPI_SLEEP >> tboot->num_mac_regions = 3; >> /* S3 resume code */ >> tboot->mac_regions[0].start = PFN_PHYS(PFN_DOWN(acpi_wakeup_address)); >> @@ -181,6 +182,7 @@ static void set_mac_regions(void) >> tboot->mac_regions[2].start = PFN_PHYS(PFN_DOWN(virt_to_phys(&_text))); >> tboot->mac_regions[2].size = PFN_PHYS(PFN_UP(virt_to_phys(&_end))) - >> PFN_PHYS(PFN_DOWN(virt_to_phys(&_text))); >> +#endif > > These #ifdefs are quite ugly. Why not add a 'depends on ACPI_SLEEP' > rule to the INTEL_TXT Kconfig section?
I *strongly* disagree with that kind of false dependencies. It makes it seem like there is something magic going on, which invites cargo cult programming in the future. I also think those particular #ifdefs are fairly innocuous... it's not like they're hiding flow of control or major chunks of code.
However, I think the actual code to set the sections is ugly as hell, which is probably why the #ifdef sticks in your eyes.
Consider the attached instead patch, which abstracts some of the (way more complex than it should be) open-coded stuff and therefore makes it stick out less.
-hpa
-- H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c index 1ab8012..af16d26 100644 --- a/arch/x86/kernel/tboot.c +++ b/arch/x86/kernel/tboot.c @@ -34,6 +34,7 @@ #include <asm/pgtable.h> #include <asm/pgalloc.h> #include <asm/fixmap.h> +#include <asm/proto.h> #include <asm/setup.h> #include <asm/tboot.h> #include <asm/e820.h> @@ -164,23 +165,36 @@ void tboot_create_trampoline(void) map_base = PFN_DOWN(tboot->tboot_base); map_size = PFN_UP(tboot->tboot_size); if (map_tboot_pages(map_base << PAGE_SHIFT, map_base, map_size)) - panic("tboot: Error mapping tboot pages (mfns) @ 0x%x, 0x%x\n", map_base, map_size); + panic("tboot: Error mapping tboot pages (mfns) @ 0x%x, 0x%x\n", + map_base, map_size); +} + +static void add_mac_region(phys_addr_t start, unsigned long size) +{ + struct tboot_mac_region *mr; + phys_addr_t end = start + size; + + if (start && size) { + mr = &tboot->mac_regions[tboot->num_mac_regions++]; + mr->start = round_down(start, PAGE_SIZE); + mr->size = round_up(end, PAGE_SIZE) - mr->start; + } } static void set_mac_regions(void) { - tboot->num_mac_regions = 3; + tboot->num_mac_regions = 0; + +#ifdef CONFIG_ACPI_SLEEP /* S3 resume code */ - tboot->mac_regions[0].start = PFN_PHYS(PFN_DOWN(acpi_wakeup_address)); - tboot->mac_regions[0].size = PFN_UP(WAKEUP_SIZE) << PAGE_SHIFT; + add_mac_region(acpi_wakeup_address, WAKEUP_SIZE); +#endif +#ifdef CONFIG_X86_TRAMPOLINE /* AP trampoline code */ - tboot->mac_regions[1].start = - PFN_PHYS(PFN_DOWN(virt_to_phys(trampoline_base))); - tboot->mac_regions[1].size = PFN_UP(TRAMPOLINE_SIZE) << PAGE_SHIFT; + add_mac_region(virt_to_phys(trampoline_base), TRAMPOLINE_SIZE); +#endif /* kernel code + data + bss */ - tboot->mac_regions[2].start = PFN_PHYS(PFN_DOWN(virt_to_phys(&_text))); - tboot->mac_regions[2].size = PFN_PHYS(PFN_UP(virt_to_phys(&_end))) - - PFN_PHYS(PFN_DOWN(virt_to_phys(&_text))); + add_mac_region(virt_to_phys(_text), _end - _text); } void tboot_shutdown(u32 shutdown_type) @@ -253,7 +267,9 @@ void tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control) tboot->acpi_sinfo.pm1b_cnt_val = pm1b_control; /* we always use the 32b wakeup vector */ tboot->acpi_sinfo.vector_width = 32; +#ifdef CONFIG_ACPI_SLEEP tboot->acpi_sinfo.kernel_s3_resume_vector = acpi_wakeup_address; +#endif if (sleep_state >= ACPI_S_STATE_COUNT || acpi_shutdown_map[sleep_state] == -1) { | |