Mark Johnson
2009-Oct-01 16:47 UTC
[Xen-devel] Re: [Xen-changelog] [xen-3.4-testing] tboot: fix tboot memory mapping for 32b
FYI. I''m seeing the same error with this patch that we saw in unstable. tboot.c: In function `tboot_copy_memory'': tboot.c:77: warning: ''map_addr'' might be used uninitialized in this function gmake[5]: *** [tboot.o] Error 1 MRJ On Thu, Oct 1, 2009 at 11:25 AM, Xen patchbot-3.4-testing <patchbot-3.4-testing@lists.xensource.com> wrote:> # HG changeset patch > # User Keir Fraser <keir.fraser@citrix.com> > # Date 1254410050 -3600 > # Node ID efcb14260292a686740cee8239aae0a503097eb8 > # Parent 1764b0cba2e9dcf1684d5c82975302277ba62064 > tboot: fix tboot memory mapping for 32b > > This patch used fixmap to get TXT heap base/size and SINIT base/size > from TXT pub config registers (whose address starts from 0xfed20000), > and get DMAR table copy from TXT heap (whose address may start from > 0x7d520000) for tboot, instead of using map_pages_to_xen(), which will > cause panic on x86_32. > > Signed-off-by: Shane Wang <shane.wang@intel.com> > xen-unstable changeset: 20242:bcb6b95b30b1 > xen-unstable date: Tue Sep 22 08:36:40 2009 +0100 > --- > xen/arch/x86/tboot.c | 94 ++++++++++++++++++++++++------------------- > xen/include/asm-x86/fixmap.h | 1 > 2 files changed, 54 insertions(+), 41 deletions(-) > > diff -r 1764b0cba2e9 -r efcb14260292 xen/arch/x86/tboot.c > --- a/xen/arch/x86/tboot.c Thu Oct 01 16:13:03 2009 +0100 > +++ b/xen/arch/x86/tboot.c Thu Oct 01 16:14:10 2009 +0100 > @@ -70,12 +70,29 @@ typedef struct __packed { > uint32_t vtd_dmars_off; > } sinit_mle_data_t; > > +static void tboot_copy_memory(unsigned char *va, uint32_t size, > + unsigned long pa) > +{ > + uint32_t map_base; > + unsigned long map_addr; > + int i; > + > + map_base = 0; > + for (i = 0; i < size; i++) { > + if ( map_base != PFN_DOWN(pa + i) ) { > + map_base = PFN_DOWN(pa + i); > + set_fixmap(FIX_TBOOT_MAP_ADDRESS, map_base << PAGE_SHIFT); > + map_addr = (unsigned long)fix_to_virt(FIX_TBOOT_MAP_ADDRESS); > + } > + *(va + i) = *(unsigned char *)(map_addr + pa + i > + - (map_base << PAGE_SHIFT)); > + } > +} > + > void __init tboot_probe(void) > { > tboot_shared_t *tboot_shared; > unsigned long p_tboot_shared; > - uint32_t map_base, map_size; > - unsigned long map_addr; > > /* Look for valid page-aligned address for shared page. */ > p_tboot_shared = simple_strtoul(opt_tboot, NULL, 0); > @@ -108,26 +125,17 @@ void __init tboot_probe(void) > /* these will be needed by tboot_protect_mem_regions() and/or > tboot_parse_dmar_table(), so get them now */ > > - map_base = PFN_DOWN(TXT_PUB_CONFIG_REGS_BASE); > - map_size = PFN_UP(NR_TXT_CONFIG_PAGES * PAGE_SIZE); > - map_addr = (unsigned long)__va(map_base << PAGE_SHIFT); > - if ( map_pages_to_xen(map_addr, map_base, map_size, __PAGE_HYPERVISOR) ) > - return; > - > + txt_heap_base = txt_heap_size = sinit_base = sinit_size = 0; > /* TXT Heap */ > - txt_heap_base > - *(uint64_t *)__va(TXT_PUB_CONFIG_REGS_BASE + TXTCR_HEAP_BASE); > - txt_heap_size > - *(uint64_t *)__va(TXT_PUB_CONFIG_REGS_BASE + TXTCR_HEAP_SIZE); > - > + tboot_copy_memory((unsigned char *)&txt_heap_base, sizeof(txt_heap_base), > + TXT_PUB_CONFIG_REGS_BASE + TXTCR_HEAP_BASE); > + tboot_copy_memory((unsigned char *)&txt_heap_size, sizeof(txt_heap_size), > + TXT_PUB_CONFIG_REGS_BASE + TXTCR_HEAP_SIZE); > /* SINIT */ > - sinit_base > - *(uint64_t *)__va(TXT_PUB_CONFIG_REGS_BASE + TXTCR_SINIT_BASE); > - sinit_size > - *(uint64_t *)__va(TXT_PUB_CONFIG_REGS_BASE + TXTCR_SINIT_SIZE); > - > - destroy_xen_mappings((unsigned long)__va(map_base << PAGE_SHIFT), > - (unsigned long)__va((map_base + map_size) << PAGE_SHIFT)); > + tboot_copy_memory((unsigned char *)&sinit_base, sizeof(sinit_base), > + TXT_PUB_CONFIG_REGS_BASE + TXTCR_SINIT_BASE); > + tboot_copy_memory((unsigned char *)&sinit_size, sizeof(sinit_size), > + TXT_PUB_CONFIG_REGS_BASE + TXTCR_SINIT_SIZE); > } > > /* definitions from xen/drivers/passthrough/vtd/iommu.h > @@ -380,11 +388,14 @@ int __init tboot_protect_mem_regions(voi > > int __init tboot_parse_dmar_table(acpi_table_handler dmar_handler) > { > - uint32_t map_base, map_size; > - unsigned long map_vaddr; > - void *heap_ptr; > struct acpi_table_header *dmar_table; > int rc; > + uint64_t size; > + uint32_t dmar_table_length; > + unsigned long pa; > + sinit_mle_data_t sinit_mle_data; > + unsigned char *dmar_table_raw; > + > > if ( !tboot_in_measured_env() ) > return acpi_table_parse(ACPI_SIG_DMAR, dmar_handler); > @@ -396,32 +407,33 @@ int __init tboot_parse_dmar_table(acpi_t > return 1; > > /* map TXT heap into Xen addr space */ > - map_base = PFN_DOWN(txt_heap_base); > - map_size = PFN_UP(txt_heap_size); > - map_vaddr = (unsigned long)__va(map_base << PAGE_SHIFT); > - if ( map_pages_to_xen(map_vaddr, map_base, map_size, __PAGE_HYPERVISOR) ) > - return 1; > > /* walk heap to SinitMleData */ > - heap_ptr = __va(txt_heap_base); > + pa = txt_heap_base; > /* skip BiosData */ > - heap_ptr += *(uint64_t *)heap_ptr; > + tboot_copy_memory((unsigned char *)&size, sizeof(size), pa); > + pa += size; > /* skip OsMleData */ > - heap_ptr += *(uint64_t *)heap_ptr; > + tboot_copy_memory((unsigned char *)&size, sizeof(size), pa); > + pa += size; > /* skip OsSinitData */ > - heap_ptr += *(uint64_t *)heap_ptr; > + tboot_copy_memory((unsigned char *)&size, sizeof(size), pa); > + pa += size; > /* now points to SinitMleDataSize; set to SinitMleData */ > - heap_ptr += sizeof(uint64_t); > + pa += sizeof(uint64_t); > + tboot_copy_memory((unsigned char *)&sinit_mle_data, sizeof(sinit_mle_data), > + pa); > /* get addr of DMAR table */ > - dmar_table = (struct acpi_table_header *)(heap_ptr + > - ((sinit_mle_data_t *)heap_ptr)->vtd_dmars_off - sizeof(uint64_t)); > - > + pa += sinit_mle_data.vtd_dmars_off - sizeof(uint64_t); > + tboot_copy_memory((unsigned char *)&dmar_table_length, > + sizeof(dmar_table_length), > + pa + sizeof(char) * ACPI_NAME_SIZE); > + dmar_table_raw = xmalloc_array(unsigned char, dmar_table_length); > + tboot_copy_memory(dmar_table_raw, dmar_table_length, pa); > + dmar_table = (struct acpi_table_header *)dmar_table_raw; > rc = dmar_handler(dmar_table); > - > - destroy_xen_mappings( > - (unsigned long)__va(map_base << PAGE_SHIFT), > - (unsigned long)__va((map_base + map_size) << PAGE_SHIFT)); > - > + xfree(dmar_table_raw); > + > /* acpi_parse_dmar() zaps APCI DMAR signature in TXT heap table */ > /* but dom0 will read real table, so must zap it there too */ > dmar_table = NULL; > diff -r 1764b0cba2e9 -r efcb14260292 xen/include/asm-x86/fixmap.h > --- a/xen/include/asm-x86/fixmap.h Thu Oct 01 16:13:03 2009 +0100 > +++ b/xen/include/asm-x86/fixmap.h Thu Oct 01 16:14:10 2009 +0100 > @@ -51,6 +51,7 @@ enum fixed_addresses { > FIX_TBOOT_SHARED_BASE, > FIX_MSIX_IO_RESERV_BASE, > FIX_MSIX_IO_RESERV_END = FIX_MSIX_IO_RESERV_BASE + FIX_MSIX_MAX_PAGES -1, > + FIX_TBOOT_MAP_ADDRESS, > __end_of_fixed_addresses > }; > > > _______________________________________________ > Xen-changelog mailing list > Xen-changelog@lists.xensource.com > http://lists.xensource.com/xen-changelog >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2009-Oct-01 17:29 UTC
Re: [Xen-devel] Re: [Xen-changelog] [xen-3.4-testing] tboot: fix tboot memory mapping for 32b
I''ll backport the fix. Thanks. K. On 01/10/2009 17:47, "Mark Johnson" <johnson.nh@gmail.com> wrote:> FYI. I''m seeing the same error with this patch that we saw > in unstable. > > tboot.c: In function `tboot_copy_memory'': > tboot.c:77: warning: ''map_addr'' might be used uninitialized in this function > gmake[5]: *** [tboot.o] Error 1 > > > MRJ > > > On Thu, Oct 1, 2009 at 11:25 AM, Xen patchbot-3.4-testing > <patchbot-3.4-testing@lists.xensource.com> wrote: >> # HG changeset patch >> # User Keir Fraser <keir.fraser@citrix.com> >> # Date 1254410050 -3600 >> # Node ID efcb14260292a686740cee8239aae0a503097eb8 >> # Parent 1764b0cba2e9dcf1684d5c82975302277ba62064 >> tboot: fix tboot memory mapping for 32b >> >> This patch used fixmap to get TXT heap base/size and SINIT base/size >> from TXT pub config registers (whose address starts from 0xfed20000), >> and get DMAR table copy from TXT heap (whose address may start from >> 0x7d520000) for tboot, instead of using map_pages_to_xen(), which will >> cause panic on x86_32. >> >> Signed-off-by: Shane Wang <shane.wang@intel.com> >> xen-unstable changeset: 20242:bcb6b95b30b1 >> xen-unstable date: Tue Sep 22 08:36:40 2009 +0100 >> --- >> xen/arch/x86/tboot.c | 94 >> ++++++++++++++++++++++++------------------- >> xen/include/asm-x86/fixmap.h | 1 >> 2 files changed, 54 insertions(+), 41 deletions(-) >> >> diff -r 1764b0cba2e9 -r efcb14260292 xen/arch/x86/tboot.c >> --- a/xen/arch/x86/tboot.c Thu Oct 01 16:13:03 2009 +0100 >> +++ b/xen/arch/x86/tboot.c Thu Oct 01 16:14:10 2009 +0100 >> @@ -70,12 +70,29 @@ typedef struct __packed { >> uint32_t vtd_dmars_off; >> } sinit_mle_data_t; >> >> +static void tboot_copy_memory(unsigned char *va, uint32_t size, >> + unsigned long pa) >> +{ >> + uint32_t map_base; >> + unsigned long map_addr; >> + int i; >> + >> + map_base = 0; >> + for (i = 0; i < size; i++) { >> + if ( map_base != PFN_DOWN(pa + i) ) { >> + map_base = PFN_DOWN(pa + i); >> + set_fixmap(FIX_TBOOT_MAP_ADDRESS, map_base << PAGE_SHIFT); >> + map_addr = (unsigned long)fix_to_virt(FIX_TBOOT_MAP_ADDRESS); >> + } >> + *(va + i) = *(unsigned char *)(map_addr + pa + i >> + - (map_base << PAGE_SHIFT)); >> + } >> +} >> + >> void __init tboot_probe(void) >> { >> tboot_shared_t *tboot_shared; >> unsigned long p_tboot_shared; >> - uint32_t map_base, map_size; >> - unsigned long map_addr; >> >> /* Look for valid page-aligned address for shared page. */ >> p_tboot_shared = simple_strtoul(opt_tboot, NULL, 0); >> @@ -108,26 +125,17 @@ void __init tboot_probe(void) >> /* these will be needed by tboot_protect_mem_regions() and/or >> tboot_parse_dmar_table(), so get them now */ >> >> - map_base = PFN_DOWN(TXT_PUB_CONFIG_REGS_BASE); >> - map_size = PFN_UP(NR_TXT_CONFIG_PAGES * PAGE_SIZE); >> - map_addr = (unsigned long)__va(map_base << PAGE_SHIFT); >> - if ( map_pages_to_xen(map_addr, map_base, map_size, __PAGE_HYPERVISOR) ) >> - return; >> - >> + txt_heap_base = txt_heap_size = sinit_base = sinit_size = 0; >> /* TXT Heap */ >> - txt_heap_base >> - *(uint64_t *)__va(TXT_PUB_CONFIG_REGS_BASE + TXTCR_HEAP_BASE); >> - txt_heap_size >> - *(uint64_t *)__va(TXT_PUB_CONFIG_REGS_BASE + TXTCR_HEAP_SIZE); >> - >> + tboot_copy_memory((unsigned char *)&txt_heap_base, >> sizeof(txt_heap_base), >> + TXT_PUB_CONFIG_REGS_BASE + TXTCR_HEAP_BASE); >> + tboot_copy_memory((unsigned char *)&txt_heap_size, >> sizeof(txt_heap_size), >> + TXT_PUB_CONFIG_REGS_BASE + TXTCR_HEAP_SIZE); >> /* SINIT */ >> - sinit_base >> - *(uint64_t *)__va(TXT_PUB_CONFIG_REGS_BASE + TXTCR_SINIT_BASE); >> - sinit_size >> - *(uint64_t *)__va(TXT_PUB_CONFIG_REGS_BASE + TXTCR_SINIT_SIZE); >> - >> - destroy_xen_mappings((unsigned long)__va(map_base << PAGE_SHIFT), >> - (unsigned long)__va((map_base + map_size) << >> PAGE_SHIFT)); >> + tboot_copy_memory((unsigned char *)&sinit_base, sizeof(sinit_base), >> + TXT_PUB_CONFIG_REGS_BASE + TXTCR_SINIT_BASE); >> + tboot_copy_memory((unsigned char *)&sinit_size, sizeof(sinit_size), >> + TXT_PUB_CONFIG_REGS_BASE + TXTCR_SINIT_SIZE); >> } >> >> /* definitions from xen/drivers/passthrough/vtd/iommu.h >> @@ -380,11 +388,14 @@ int __init tboot_protect_mem_regions(voi >> >> int __init tboot_parse_dmar_table(acpi_table_handler dmar_handler) >> { >> - uint32_t map_base, map_size; >> - unsigned long map_vaddr; >> - void *heap_ptr; >> struct acpi_table_header *dmar_table; >> int rc; >> + uint64_t size; >> + uint32_t dmar_table_length; >> + unsigned long pa; >> + sinit_mle_data_t sinit_mle_data; >> + unsigned char *dmar_table_raw; >> + >> >> if ( !tboot_in_measured_env() ) >> return acpi_table_parse(ACPI_SIG_DMAR, dmar_handler); >> @@ -396,32 +407,33 @@ int __init tboot_parse_dmar_table(acpi_t >> return 1; >> >> /* map TXT heap into Xen addr space */ >> - map_base = PFN_DOWN(txt_heap_base); >> - map_size = PFN_UP(txt_heap_size); >> - map_vaddr = (unsigned long)__va(map_base << PAGE_SHIFT); >> - if ( map_pages_to_xen(map_vaddr, map_base, map_size, __PAGE_HYPERVISOR) >> ) >> - return 1; >> >> /* walk heap to SinitMleData */ >> - heap_ptr = __va(txt_heap_base); >> + pa = txt_heap_base; >> /* skip BiosData */ >> - heap_ptr += *(uint64_t *)heap_ptr; >> + tboot_copy_memory((unsigned char *)&size, sizeof(size), pa); >> + pa += size; >> /* skip OsMleData */ >> - heap_ptr += *(uint64_t *)heap_ptr; >> + tboot_copy_memory((unsigned char *)&size, sizeof(size), pa); >> + pa += size; >> /* skip OsSinitData */ >> - heap_ptr += *(uint64_t *)heap_ptr; >> + tboot_copy_memory((unsigned char *)&size, sizeof(size), pa); >> + pa += size; >> /* now points to SinitMleDataSize; set to SinitMleData */ >> - heap_ptr += sizeof(uint64_t); >> + pa += sizeof(uint64_t); >> + tboot_copy_memory((unsigned char *)&sinit_mle_data, >> sizeof(sinit_mle_data), >> + pa); >> /* get addr of DMAR table */ >> - dmar_table = (struct acpi_table_header *)(heap_ptr + >> - ((sinit_mle_data_t *)heap_ptr)->vtd_dmars_off - >> sizeof(uint64_t)); >> - >> + pa += sinit_mle_data.vtd_dmars_off - sizeof(uint64_t); >> + tboot_copy_memory((unsigned char *)&dmar_table_length, >> + sizeof(dmar_table_length), >> + pa + sizeof(char) * ACPI_NAME_SIZE); >> + dmar_table_raw = xmalloc_array(unsigned char, dmar_table_length); >> + tboot_copy_memory(dmar_table_raw, dmar_table_length, pa); >> + dmar_table = (struct acpi_table_header *)dmar_table_raw; >> rc = dmar_handler(dmar_table); >> - >> - destroy_xen_mappings( >> - (unsigned long)__va(map_base << PAGE_SHIFT), >> - (unsigned long)__va((map_base + map_size) << PAGE_SHIFT)); >> - >> + xfree(dmar_table_raw); >> + >> /* acpi_parse_dmar() zaps APCI DMAR signature in TXT heap table */ >> /* but dom0 will read real table, so must zap it there too */ >> dmar_table = NULL; >> diff -r 1764b0cba2e9 -r efcb14260292 xen/include/asm-x86/fixmap.h >> --- a/xen/include/asm-x86/fixmap.h Thu Oct 01 16:13:03 2009 +0100 >> +++ b/xen/include/asm-x86/fixmap.h Thu Oct 01 16:14:10 2009 +0100 >> @@ -51,6 +51,7 @@ enum fixed_addresses { >> FIX_TBOOT_SHARED_BASE, >> FIX_MSIX_IO_RESERV_BASE, >> FIX_MSIX_IO_RESERV_END = FIX_MSIX_IO_RESERV_BASE + FIX_MSIX_MAX_PAGES -1, >> + FIX_TBOOT_MAP_ADDRESS, >> __end_of_fixed_addresses >> }; >> >> >> _______________________________________________ >> Xen-changelog mailing list >> Xen-changelog@lists.xensource.com >> http://lists.xensource.com/xen-changelog >> > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel