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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

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


As we both wrote above, ImageBuilder doesn't need to support every
feature and every possible way a feature can be used. Only the most
common ones. I don't think we want to support the case where a user
assigns cpus of different type.

I think you are right that these are the interesting options at the
ImageBuilder level:

NUM_CPUPOOLS="<number>"
CPUPOOL[number] = "<list_of_cpus> <scheduler>"
DOMU_CPUPOOL[number] = "CPUPOOL[number]"

The last one could also be:

DOMU_CPUPOOL[number] = "CPUPOOL<number>" # e.g. DOMU_CPUPOOL[1] = "CPUPOOL2"