[Xen-devel] Re: [Xen-changelog] [xen-3.4-testing] tboot: fix tboot memory mapping for 32b

[Mark Johnson <johnson.nh@xxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxx> wrote:
> # HG changeset patch
> # User Keir Fraser <keir.fraser@xxxxxxxxxx>
> # 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@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-changelog
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel