Re: [Xen-devel] [PATCH 1/2] hvmloader: dynamically determine scratch memory range for tests

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 31/05/17 08:37, Jan Beulich wrote:
> This re-enables tests on configurations where commit 0d6968635c
> ("hvmloader: avoid tests when they would clobber used memory") forced
> them to be skipped.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

I still fail to see the value in retaining these tests.  They only cover
two very specific cases, and the rep_io test will start failing if a
device model ever implements port 0x5f.

>
> --- a/tools/firmware/hvmloader/tests.c
> +++ b/tools/firmware/hvmloader/tests.c
> @@ -29,14 +29,15 @@
>  
>  /*
>   * Memory layout during tests:
> - *  4MB to 8MB is cleared.
> - *  Page directory resides at 4MB.
> - *  2 page table pages reside at 4MB+4kB to 4MB+12kB.
> - *  Pagetables identity-map 0-8MB, except 4kB at va 6MB maps to pa 5MB.
> + *  The 4MB block at test_mem_base is cleared.
> + *  Page directory resides at test_mem_base.
> + *  2 page table pages reside at test_mem_base+4kB to test_mem_base+12kB.
> + *  Pagetables identity-map 0-4MB and test_mem_base-test_mem_base+4MB,
> + *  except 4kB at va test_mem_base+2MB maps to pa test_mem_base+1MB.
>   */
> -#define TEST_MEM_BASE (4ul << 20)
> +static unsigned long test_mem_base;
>  #define TEST_MEM_SIZE (4ul << 20)
> -#define PD_START TEST_MEM_BASE
> +#define PD_START test_mem_base
>  #define PT_START (PD_START + 4096)
>  
>  static void setup_paging(void)
> @@ -45,14 +46,25 @@ static void setup_paging(void)
>      uint32_t *pt = (uint32_t *)PT_START;
>      uint32_t i;
>  
> -    /* Identity map 0-8MB. */
> -    for ( i = 0; i < 2; i++ )
> -        pd[i] = (unsigned long)pt + (i<<12) + 3;
> -    for ( i = 0; i < 2 * 1024; i++ )
> -        pt[i] = (i << 12) + 3;
> +    /* Identity map [0,_end). */
> +    for ( i = 0; i <= (unsigned long)(_end - 1) >> (PAGE_SHIFT + 10); ++i )
> +    {
> +        unsigned int j;
> +
> +        pd[i] = (unsigned long)pt + 3;
> +        for ( j = 0; j < PAGE_SIZE / sizeof(*pt); ++j )
> +            *pt++ = (i << (PAGE_SHIFT + 10)) + (j << PAGE_SHIFT) + 3;
> +    }
> +
> +    /* Identity map TEST_MEM_SIZE @ test_mem_base. */
> +    for ( i = 0; i < (TEST_MEM_SIZE >> (PAGE_SHIFT + 10)); ++i )
> +        pd[i + (test_mem_base >> (PAGE_SHIFT + 10))] =
> +            (unsigned long)pt + (i << PAGE_SHIFT) + 3;
> +    for ( i = 0; i < (TEST_MEM_SIZE >> PAGE_SHIFT); ++i )
> +        *pt++ = test_mem_base + (i << PAGE_SHIFT) + 3;
>  
> -    /* Page at virtual 6MB maps to physical 5MB. */
> -    pt[6u<<8] -= 0x100000u;
> +    /* Page at virtual test_mem_base+2MB maps physical test_mem_base+1MB. */
> +    pt[(long)(-TEST_MEM_SIZE + 0x200000) >> PAGE_SHIFT] -= 0x100000;

This line is very confusing with its negative offset into pt[].  The
logic would be far clearer if pt[] wasn't mutated and stayed pointing at
PT_START, which looks to be easy by not resetting i on each loop.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel