lkml.org 
[lkml]   [2009]   [Aug]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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) {
\
 
 \ /
  Last update: 2009-08-13 17:41    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site