Re: [for-4.17 3/3] automation: Add a new job for testing boot time cpupools on arm64

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Stefano,

On 06/09/2022 01:21, Stefano Stabellini wrote:
> 
> On Mon, 5 Sep 2022, Michal Orzel wrote:
>> Hi Stefano,
>>
>> On 03/09/2022 01:49, Stefano Stabellini wrote:
>>>
>>>
>>> Currently this test fails with:
>>>
>>> + fdtput binaries/virt-gicv2.dtb -p -t s /pl061@9030000 status disabled
>>> + [[ boot-cpupools == \b\o\o\t\-\c\p\u\p\o\o\l\s ]]
>>> ++ fdtget binaries/virt-gicv2.dtb -t x /cpus/cpu@1 phandle
>>> Error at 'phandle': FDT_ERR_NOTFOUND
>> My bad. The qemu version used by CI does not generate phandles for cpus.
>> So the fix is very straightforward and requires putting custom phandle for 
>> cpu@1.
>>
>> diff --git a/automation/scripts/qemu-smoke-arm64.sh 
>> b/automation/scripts/qemu-smoke-arm64.sh
>> index c2184850293c..158d5665d71d 100755
>> --- a/automation/scripts/qemu-smoke-arm64.sh
>> +++ b/automation/scripts/qemu-smoke-arm64.sh
>> @@ -50,8 +50,9 @@ fdtput binaries/virt-gicv2.dtb -p -t s /pl061@9030000 
>> status disabled
>>
>>  if [[ "${test_variant}" == "boot-cpupools" ]]; then
>>      # Create cpupool node and assign it to domU0
>> -    cpu_phandle="$(fdtget binaries/virt-gicv2.dtb -t x /cpus/cpu@1 phandle)"
>> +    cpu_phandle="0xfffffe"
>>      cpupool_phandle="0xffffff"
>> +    fdtput binaries/virt-gicv2.dtb -p -t x /cpus/cpu@1 phandle $cpu_phandle
>>      fdtput binaries/virt-gicv2.dtb -p -t s /chosen/cpupool compatible 
>> xen,cpupool
>>      fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool cpupool-cpus 
>> $cpu_phandle
>>      fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool phandle 
>> $cpupool_phandle
> 
> 
>>> Given my other comment below, I would leave this code as is.
>>>
>>>
>>>> +if [[ "${test_variant}" == "boot-cpupools" ]]; then
>>>> +    # Create cpupool node and assign it to domU0
>>>> +    cpu_phandle="$(fdtget binaries/virt-gicv2.dtb -t x /cpus/cpu@1 
>>>> phandle)"
>>>> +    cpupool_phandle="0xffffff"
>>>> +    fdtput binaries/virt-gicv2.dtb -p -t s /chosen/cpupool compatible 
>>>> xen,cpupool
>>>> +    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool cpupool-cpus 
>>>> $cpu_phandle
>>>> +    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/cpupool phandle 
>>>> $cpupool_phandle
>>>> +    fdtput binaries/virt-gicv2.dtb -p -t x /chosen/domU0 domain-cpupool 
>>>> $cpupool_phandle
>>>> +
>>>> +    # Check if domU0 (id=1) is assigned to Pool-1
>>>> +    passed="${test_variant} test passed"
>>>> +    dom0_check="if xl list -c 1 | grep -q Pool-1; then echo ${passed}; fi"
>>>> +fi
>>>
>>> I would prefer to keep the device tree editing here to a minimum and
>>> instead add boot-cpupool support in ImageBuilder and add CPUPOOL* config
>>> options to the existing config file for ImageBuilder created in this
>>> file below. This way, we keep this test cleaner and we help more the
>>> user by proving a way to set boot-cpupools more easily in general, also
>>> useful outside gitlab-ci.
>>
>> I agree that ImageBuilder is a great tool. However, I would opt for keeping 
>> what I did because of the following:
>> - current release schedule (we could benefit from having a test for 4.17 
>> feature instead of waiting for the corresponding
>>   change to be done in ImageBuilder first and tested),
>> - test is already prepared and requires just a trivial fix,
>> - we should not enforce users willing to add tests to gitlab-ci to always 
>> prepare the ImageBuilder changes first.
>>   ImageBuilder is not meant to support all the features strictly because 
>> some of them require too much
>>   end-user knowledge and digging into device tree (it should stay as simple 
>> as possible),
>> - all in all we need to have a way to modify the dtb and fdtput is certainly 
>> better than sed as it does not
>>   require additional steps for decompilation/compilation and its commands 
>> look more clean than using sed transformation.
>>
>> Let me know what you think.
>>
>> On a side note, I can add boot-time cpupools support in ImageBuilder to my 
>> TODO list so that we can check if this is something
>> ImageBuilder should support. If yes, we can modify this test after the 
>> release.
> 
> 
> Yeah, ImageBuilder doesn't necessarely need to support every feature.
> However, a tool (if not ImageBuilder, Lopper, or a new ImageBuilder
> script) should support CPUPOOLs to enable the user.
> 
> You are right that ImageBuilder is not necessarely tied with gitlab-ci.
> This is especially true once we start doing more interface-level
> testing, such as hypercalls fuzzing with XTF. We are not going to be
> able to use ImageBuilder to trigger every possible device tree boot time
> combination, especially the ones that are invalid. We want to be able to
> test Xen with invalid device tree input as well.
> 
> In addition to interface-level testing, we need user-level testing to
> test features the way we expect a user to use them. This is what
> ImageBuilder is for and that is why it has been used today in gitlab-ci.
> On ARM today we only have user-level testing in gitlab-ci, but I'd love
> to have more interface-level testing, which will surely require more
> device tree manipulations outside of ImageBuilder.
> 
> - user-level tests -> ImageBuilder, common valid configurations
> - interface-level tests -> not ImageBuilder, various valid and invalid
>                            configurations, maybe automatically generated?
>                            Device tree manipulations expected in gitlab-ci.
> 
> 
> In my view, this test belongs to the "user-level test" category, this is
> why I would prefer if it was done using the same tool that we expect the
> user to use. Ideally, it would be ImageBuilder because that is the tool
> that we have used so far (but it could be a new script under
> ImageBuilder or Lopper).
> 
> But I understand deadlines, release schedule, etc., so if you think it
> cannot be done properly using ImageBuilder in 2-3 days, then I would
> take this patch as is, and we can revisit it in the future as you
> suggested. I am OK with that too.

Ok, let me see if I can come up with a clean solution in the ImageBuilder.

~Michal