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 10:13, Jiamei Xie wrote:
> 
> 
> Hi Michal,
> 
>> -----Original Message-----
>> From: Michal Orzel <michal.orzel@xxxxxxx>
>> Sent: Friday, March 3, 2023 3:47 PM
>> To: Jiamei Xie <Jiamei.Xie@xxxxxxx>; Stefano Stabellini
>> <sstabellini@xxxxxxxxxx>
>> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Wei Chen <Wei.Chen@xxxxxxx>;
>> 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
>>
>> 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.
> 
> Got it, thanks!  Another question,  set MEMORY_END=0x50000000 for all test 
> cases or just this test cases ? Currently I run the test case manually 
> without xen gitlab ci enviroment . If set this for all test cases, all test 
> cases should be tested.
You can set it globally as it will be helpful also for static-mem and other 
test cases should not be impacted.
Before pushing any commit related to CI, the maintainer shall check that it 
does not cause any issues
by running the pipeline on his account before committing a patch.

~Michal