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 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.
I can already think of the following IB config options that would need to be 
introduced to properly support boot-time cpupools:
CPUPOOL[number] = "<list_of_cpus> <scheduler>" - to create cpupools
NUM_CPUPOOLS = "<number>" - to keep the number of created cpupools
DOMU_CPUPOOL[number] = "CPUPOOL[number]" - to assing domU to one of the created 
cpupools
So we already have 3 new options and the number of required sanity checks I can 
think of is significant.
Even then, we could easily trigger a failure e.g. if user assigns cpus of 
different type and does not pass hmp-unsafe=1.


~Michal