Re: [PATCH 1/2] automation: arm64: Create a test job for testing static heap on qemu

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Jiamei,

On 03/03/2023 07:49, Jiamei Xie wrote:
> 
> 
> Hi Stefano,
> 
>> -----Original Message-----
>> From: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Sent: Friday, March 3, 2023 9:51 AM
>> To: Jiamei Xie <Jiamei.Xie@xxxxxxx>
>> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Wei Chen <Wei.Chen@xxxxxxx>;
>> sstabellini@xxxxxxxxxx; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
>> Doug Goldstein <cardoe@xxxxxxxxxx>
>> Subject: Re: [PATCH 1/2] automation: arm64: Create a test job for testing
>> static heap on qemu
>>
>> On Thu, 2 Mar 2023, jiamei.xie wrote:
>>> From: Jiamei Xie <jiamei.xie@xxxxxxx>
>>>
>>> Create a new test job, called qemu-smoke-dom0less-arm64-gcc-staticheap.
>>>
>>> Add property "xen,static-heap" under /chosen node to enable static-heap.
>>> If the domU can start successfully with static-heap enabled, then this
>>> test pass.
>>>
>>> Signed-off-by: Jiamei Xie <jiamei.xie@xxxxxxx>
>>
>> Hi Jiamei, thanks for the patch!
>>
>>
>>> ---
>>>  automation/gitlab-ci/test.yaml                 | 16 ++++++++++++++++
>>>  .../scripts/qemu-smoke-dom0less-arm64.sh       | 18
>> ++++++++++++++++++
>>>  2 files changed, 34 insertions(+)
>>>
>>> diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
>>> index 1c5f400b68..5a9b88477a 100644
>>> --- a/automation/gitlab-ci/test.yaml
>>> +++ b/automation/gitlab-ci/test.yaml
>>> @@ -133,6 +133,22 @@ qemu-smoke-dom0less-arm64-gcc-debug-
>> staticmem:
>>>      - *arm64-test-needs
>>>      - alpine-3.12-gcc-debug-arm64-staticmem
>>>
>>> +qemu-smoke-dom0less-arm64-gcc-staticheap:
>>> + extends: .qemu-arm64
>>> + script:
>>> +   - ./automation/scripts/qemu-smoke-dom0less-arm64.sh static-heap
>> 2>&1 | tee ${LOGFILE}
>>> + needs:
>>> +   - *arm64-test-needs
>>> +   - alpine-3.12-gcc-arm64
>>> +
>>> +qemu-smoke-dom0less-arm64-gcc-debug-staticheap:
>>> + extends: .qemu-arm64
>>> + script:
>>> +   - ./automation/scripts/qemu-smoke-dom0less-arm64.sh static-heap
>> 2>&1 | tee ${LOGFILE}
>>> + needs:
>>> +   - *arm64-test-needs
>>> +   - alpine-3.12-gcc-debug-arm64
>>> +
>>>  qemu-smoke-dom0less-arm64-gcc-boot-cpupools:
>>>    extends: .qemu-arm64
>>>    script:
>>> diff --git a/automation/scripts/qemu-smoke-dom0less-arm64.sh
>> b/automation/scripts/qemu-smoke-dom0less-arm64.sh
>>> index 182a4b6c18..4e73857199 100755
>>> --- a/automation/scripts/qemu-smoke-dom0less-arm64.sh
>>> +++ b/automation/scripts/qemu-smoke-dom0less-arm64.sh
>>> @@ -27,6 +27,11 @@ fi
>>>  "
>>>  fi
>>>
>>> +if [[ "${test_variant}" == "static-heap" ]]; then
>>> +    passed="${test_variant} test passed"
>>> +    domU_check="echo \"${passed}\""
>>> +fi
>>> +
>>>  if [[ "${test_variant}" == "boot-cpupools" ]]; then
>>>      # Check if domU0 (id=1) is assigned to Pool-1 with null scheduler
>>>      passed="${test_variant} test passed"
>>> @@ -128,6 +133,19 @@ if [[ "${test_variant}" == "static-mem" ]]; then
>>>      echo -e "\nDOMU_STATIC_MEM[0]=\"${domu_base} ${domu_size}\"" >>
>> binaries/config
>>>  fi
>>>
>>> +if [[ "${test_variant}" == "static-heap" ]]; then
>>> +    # ImageBuilder uses the config file to create the uboot script. 
>>> Devicetree
>>> +    # will be set via the generated uboot script.
>>> +    # The valid memory range is 0x40000000 to 0x80000000 as defined
>> before.
>>> +    # ImageBuillder sets the kernel and ramdisk range based on the file 
>>> size.
>>> +    # It will use the memory range between 0x45600000 to 0x47AED1E8, so
>> set
>>> +    # memory range between 0x50000000 and 0x80000000 as static heap.
>>
>> I think this is OK. One suggestion to make things more reliable would be
>> to change MEMORY_END to be 0x50000000 so that you can be sure that
>> ImageBuilder won't go over the limit. You could do it just for this
>> test, which would be safer, but to be honest you could limit MEMORY_END
>> to 0x50000000 for all tests in qemu-smoke-dom0less-arm64.sh because it
>> shouldn't really cause any problems.
>>
> [Jiamei Xie]
> Thanks for your comments. I am a little confused about " to change MEMORY_END 
> to be 0x50000000".
>  I set 0STATIC_HEAP="0x50000000 0x30000000" where is the start address. Why 
> change MEMORY_END
>  to be 0x50000000?
Let me answer to that question so that you do not need to wait another day for 
Stefano.
ImageBuilder uses MEMORY_START and MEMORY_END from the cfg file as a range in 
which it can instruct
u-boot where to place the images. It is safer to set MEMORY_END to 0x50000000 
rather than to 0xC0000000
because you will be sure that no image will be placed in a region that you 
defined as static heap.

~Michal