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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.07.17 at 18:20, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

Feel free to submit a patch to remove it; I won't object, but I also
won't submit such a patch myself (in the near future at least). But
please let me know if you intend to, as then I won't need to look
into taking care of ...

>> --- 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.

... your comment here.

Jan


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