[Xen-devel] Fwd: [xen-4.10-testing bisection] complete test-armhf-armhf-xl-arndale

[Xen-devel] Fwd: [xen-4.10-testing bisection] complete test-armhf-armhf-xl-arndale

[Jan Beulich]

>>> On 19.06.19 at 09:06, <osstest-admin@xenproject.org> wrote:
> branch xen-4.10-testing
> xenbranch xen-4.10-testing
> job test-armhf-armhf-xl-arndale
> testid debian-install
> 
> Tree: linux git://xenbits.xen.org/linux-pvops.git
> Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
> Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git
> Tree: qemuu git://xenbits.xen.org/qemu-xen.git
> Tree: xen git://xenbits.xen.org/xen.git
> 
> *** Found and reproduced problem changeset ***
> 
>   Bug is in tree:  xen git://xenbits.xen.org/xen.git
>   Bug introduced:  702c9146c00d65d1e9c5955335ba002505e97e09
>   Bug not present: 52220b5f437a8d03ba108e127e7d717657edf99c
>   Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/138005/ 
> 
> 
>   commit 702c9146c00d65d1e9c5955335ba002505e97e09
>   Author: Julien Grall <julien.grall@arm.com>
>   Date:   Mon Apr 29 15:05:16 2019 +0100
>   
>       xen/arm: Add an isb() before reading CNTPCT_EL0 to prevent re-ordering
>       
>       Per D8.2.1 in ARM DDI 0487C.a, "a read to CNTPCT_EL0 can occur
>       speculatively and out of order relative to other instructions executed
>       on the same PE."
>       
>       Add an instruction barrier to get accurate number of cycles when
>       requested in get_cycles(). For the other users of CNPCT_EL0, replace by
>       a call to get_cycles().
>       
>       This is part of XSA-295.
>       
>       Signed-off-by: Julien Grall <julien.grall@arm.com>
>       Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Just in case you didn't notice this coming in.

Jan

> For bisection revision-tuple graph see:
>    
> http://logs.test-lab.xenproject.org/osstest/results/bisect/xen-4.10-testing/t 
> est-armhf-armhf-xl-arndale.debian-install.html
> Revision IDs in each graph node refer, respectively, to the Trees above.
> 
> ----------------------------------------
> Running cs-bisection-step 
> --graph-out=/home/logs/results/bisect/xen-4.10-testing/test-armhf-armhf-xl-ar
> ndale.debian-install --summary-out=tmp/138005.bisection-summary 
> --basis-template=137381 --blessings=real,real-bisect xen-4.10-testing 
> test-armhf-armhf-xl-arndale debian-install
> Searching for failure / basis pass:
>  137854 fail [host=arndale-metrocentre] / 137381 [host=arndale-bluewater] 
> 137277 [host=arndale-lakeside] 136692 [host=arndale-westfield] 136552 
> [host=arndale-bluewater] 136391 [host=arndale-lakeside] 136232 ok.
> Failure / basis pass flights: 137854 / 136232
> (tree in latest but not in basispass: seabios)
> Tree: linux git://xenbits.xen.org/linux-pvops.git
> Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
> Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git
> Tree: qemuu git://xenbits.xen.org/qemu-xen.git
> Tree: xen git://xenbits.xen.org/xen.git
> Latest e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> fc7d997c35372126823c3b0acf7b67c45cbeea36 
> 8acabec966263f90ad493e4af2642947c0c43d23 
> 6197b859ec77e4950e5ae0202002d6d4dbef143b
> Basis pass e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> f684c3f5eef4be691e137ae64e7d00521ec201de 
> 6ea4cef2bd717045ac0e84b52a5b1b7716feb0c2 
> ab261f5ac491a0a4d65a641fc7da29b810ec0fb2
> Generating revisions with ./adhoc-revtuple-generator  
> git://xenbits.xen.org/linux-pvops.git#e64ac26749dc2c0f390caccd04274608ab31c8c
> f-e64ac26749dc2c0f390caccd04274608ab31c8cf 
> git://xenbits.xen.org/osstest/linux-firmware.git#c530a75c1e6a472b0eb9558310b5
> 18f0dfcd8860-c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> git://xenbits.xen.org/osstest/ovmf.git#f684c3f5eef4be691e137ae64e7d00521ec201
> de-fc7d997c35372126823c3b0acf7b67c45cbeea36 
> git://xenbits.xen.org/qemu-xen.git#6ea4cef2bd717045ac0e84b52a5b1b7716feb0c\
>  2-8acabec966263f90ad493e4af2642947c0c43d23 
> git://xenbits.xen.org/xen.git#ab261f5ac491a0a4d65a641fc7da29b810ec0fb2-6197b8
> 59ec77e4950e5ae0202002d6d4dbef143b
> Loaded 3009 nodes in revision graph
> Searching for test results:
>  136232 pass e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> f684c3f5eef4be691e137ae64e7d00521ec201de 
> 6ea4cef2bd717045ac0e84b52a5b1b7716feb0c2 
> ab261f5ac491a0a4d65a641fc7da29b810ec0fb2
>  136391 [host=arndale-lakeside]
>  136552 [host=arndale-bluewater]
>  136692 [host=arndale-westfield]
>  137277 [host=arndale-lakeside]
>  137381 [host=arndale-bluewater]
>  137727 fail irrelevant
>  137854 fail e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> fc7d997c35372126823c3b0acf7b67c45cbeea36 
> 8acabec966263f90ad493e4af2642947c0c43d23 
> 6197b859ec77e4950e5ae0202002d6d4dbef143b
>  137924 pass e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> f684c3f5eef4be691e137ae64e7d00521ec201de 
> 6ea4cef2bd717045ac0e84b52a5b1b7716feb0c2 
> ab261f5ac491a0a4d65a641fc7da29b810ec0fb2
>  137934 pass e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> e2afc8ab59c3dc6cf368c339a26840f0869a2356 
> 04a43f76e2d73e8387bd3e3bd439ef8c6d69d361 
> ac516e89402b0c8df92b139831727ece5db700e3
>  137926 fail irrelevant
>  137944 fail e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> fc7d997c35372126823c3b0acf7b67c45cbeea36 
> 8acabec966263f90ad493e4af2642947c0c43d23 
> 6197b859ec77e4950e5ae0202002d6d4dbef143b
>  137970 fail e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> fc7d997c35372126823c3b0acf7b67c45cbeea36 
> 8acabec966263f90ad493e4af2642947c0c43d23 
> 7484d020f0e91b70fe3a14030a45369f959c932f
>  137951 pass e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> 49fb605709796488589c6f282522283911a575f6 
> 04a43f76e2d73e8387bd3e3bd439ef8c6d69d361 
> adf037bba1e6af47fef8584c1ad41f424ebda01e
>  137957 pass e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> 1631bb26ae991e530d3c96fe3161ea15144b358e 
> 8acabec966263f90ad493e4af2642947c0c43d23 
> adf037bba1e6af47fef8584c1ad41f424ebda01e
>  138005 fail e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> fc7d997c35372126823c3b0acf7b67c45cbeea36 
> 8acabec966263f90ad493e4af2642947c0c43d23 
> 702c9146c00d65d1e9c5955335ba002505e97e09
>  137983 pass e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> fc7d997c35372126823c3b0acf7b67c45cbeea36 
> 8acabec966263f90ad493e4af2642947c0c43d23 
> c11933bda1eeb6ffc3350720bf4701d37b02211c
>  137988 fail e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> fc7d997c35372126823c3b0acf7b67c45cbeea36 
> 8acabec966263f90ad493e4af2642947c0c43d23 
> 3f10c53b3ef12f770f64b914914b9f13882e9dae
>  137993 pass e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> fc7d997c35372126823c3b0acf7b67c45cbeea36 
> 8acabec966263f90ad493e4af2642947c0c43d23 
> 52220b5f437a8d03ba108e127e7d717657edf99c
>  137996 fail e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> fc7d997c35372126823c3b0acf7b67c45cbeea36 
> 8acabec966263f90ad493e4af2642947c0c43d23 
> 702c9146c00d65d1e9c5955335ba002505e97e09
>  137998 pass e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> fc7d997c35372126823c3b0acf7b67c45cbeea36 
> 8acabec966263f90ad493e4af2642947c0c43d23 
> 52220b5f437a8d03ba108e127e7d717657edf99c
>  138000 fail e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> fc7d997c35372126823c3b0acf7b67c45cbeea36 
> 8acabec966263f90ad493e4af2642947c0c43d23 
> 702c9146c00d65d1e9c5955335ba002505e97e09
>  138003 pass e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> fc7d997c35372126823c3b0acf7b67c45cbeea36 
> 8acabec966263f90ad493e4af2642947c0c43d23 
> 52220b5f437a8d03ba108e127e7d717657edf99c
> Searching for interesting versions
>  Result found: flight 136232 (pass), for basis pass
>  Result found: flight 137854 (fail), for basis failure
>  Repro found: flight 137924 (pass), for basis pass
>  Repro found: flight 137944 (fail), for basis failure
>  0 revisions at e64ac26749dc2c0f390caccd04274608ab31c8cf 
> c530a75c1e6a472b0eb9558310b518f0dfcd8860 
> fc7d997c35372126823c3b0acf7b67c45cbeea36 
> 8acabec966263f90ad493e4af2642947c0c43d23 
> 52220b5f437a8d03ba108e127e7d717657edf99c
> No revisions left to test, checking graph state.
>  Result found: flight 137993 (pass), for last pass
>  Result found: flight 137996 (fail), for first failure
>  Repro found: flight 137998 (pass), for last pass
>  Repro found: flight 138000 (fail), for first failure
>  Repro found: flight 138003 (pass), for last pass
>  Repro found: flight 138005 (fail), for first failure
> 
> *** Found and reproduced problem changeset ***
> 
>   Bug is in tree:  xen git://xenbits.xen.org/xen.git
>   Bug introduced:  702c9146c00d65d1e9c5955335ba002505e97e09
>   Bug not present: 52220b5f437a8d03ba108e127e7d717657edf99c
>   Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/138005/ 
> 
> 
>   commit 702c9146c00d65d1e9c5955335ba002505e97e09
>   Author: Julien Grall <julien.grall@arm.com>
>   Date:   Mon Apr 29 15:05:16 2019 +0100
>   
>       xen/arm: Add an isb() before reading CNTPCT_EL0 to prevent re-ordering
>       
>       Per D8.2.1 in ARM DDI 0487C.a, "a read to CNTPCT_EL0 can occur
>       speculatively and out of order relative to other instructions executed
>       on the same PE."
>       
>       Add an instruction barrier to get accurate number of cycles when
>       requested in get_cycles(). For the other users of CNPCT_EL0, replace 
> by
>       a call to get_cycles().
>       
>       This is part of XSA-295.
>       
>       Signed-off-by: Julien Grall <julien.grall@arm.com>
>       Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> pnmtopng: 203 colors found
> Revision graph left in 
> /home/logs/results/bisect/xen-4.10-testing/test-armhf-armhf-xl-arndale.debian
> -install.{dot,ps,png,html,svg}.
> ----------------------------------------
> 138005: tolerable ALL FAIL
> 
> flight 138005 xen-4.10-testing real-bisect [real]
> http://logs.test-lab.xenproject.org/osstest/logs/138005/ 
> 
> Failures :-/ but no regressions.
> 
> Tests which did not succeed,
> including tests which could not be run:
>  test-armhf-armhf-xl-arndale  10 debian-install          fail baseline 
> untested
> 
> 
> jobs:
>  test-armhf-armhf-xl-arndale                                  fail    
> 
> 
> ------------------------------------------------------------
> sg-report-flight on osstest.test-lab.xenproject.org
> logs: /home/logs/logs
> images: /home/logs/images
> 
> Logs, config files, etc. are available at
>     http://logs.test-lab.xenproject.org/osstest/logs 
> 
> Explanation of these reports, and of osstest in general, is at
>     
> http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master 
>     http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master 
> 
> Test harness code can be found at
>     http://xenbits.xen.org/gitweb?p=osstest.git;a=summary 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org 
> https://lists.xenproject.org/mailman/listinfo/xen-devel 




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Fwd: [xen-4.10-testing bisection] complete test-armhf-armhf-xl-arndale

[Julien Grall]

Hi,

On 6/19/19 8:28 AM, Jan Beulich wrote:
>>>> On 19.06.19 at 09:06, <osstest-admin@xenproject.org> wrote:
>> branch xen-4.10-testing
>> xenbranch xen-4.10-testing
>> job test-armhf-armhf-xl-arndale
>> testid debian-install
>>
>> Tree: linux git://xenbits.xen.org/linux-pvops.git
>> Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
>> Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git
>> Tree: qemuu git://xenbits.xen.org/qemu-xen.git
>> Tree: xen git://xenbits.xen.org/xen.git
>>
>> *** Found and reproduced problem changeset ***
>>
>>    Bug is in tree:  xen git://xenbits.xen.org/xen.git
>>    Bug introduced:  702c9146c00d65d1e9c5955335ba002505e97e09
>>    Bug not present: 52220b5f437a8d03ba108e127e7d717657edf99c
>>    Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/138005/
>>
>>
>>    commit 702c9146c00d65d1e9c5955335ba002505e97e09
>>    Author: Julien Grall <julien.grall@arm.com>
>>    Date:   Mon Apr 29 15:05:16 2019 +0100
>>    
>>        xen/arm: Add an isb() before reading CNTPCT_EL0 to prevent re-ordering
>>        
>>        Per D8.2.1 in ARM DDI 0487C.a, "a read to CNTPCT_EL0 can occur
>>        speculatively and out of order relative to other instructions executed
>>        on the same PE."
>>        
>>        Add an instruction barrier to get accurate number of cycles when
>>        requested in get_cycles(). For the other users of CNPCT_EL0, replace by
>>        a call to get_cycles().
>>        
>>        This is part of XSA-295.
>>        
>>        Signed-off-by: Julien Grall <julien.grall@arm.com>
>>        Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> Just in case you didn't notice this coming in.

I saw it this morning. But I fail to understand how the instruction 
barrier will result in a timeout during the guest installation...

The more this only doesn't seem to appear on staging-4.12.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Fwd: [xen-4.10-testing bisection] complete test-armhf-armhf-xl-arndale

[Julien Grall]

Answering to myself.

On 19/06/2019 10:02, Julien Grall wrote:
> Hi,
> 
> On 6/19/19 8:28 AM, Jan Beulich wrote:
>>>>> On 19.06.19 at 09:06, <osstest-admin@xenproject.org> wrote:
>>> branch xen-4.10-testing
>>> xenbranch xen-4.10-testing
>>> job test-armhf-armhf-xl-arndale
>>> testid debian-install
>>>
>>> Tree: linux git://xenbits.xen.org/linux-pvops.git
>>> Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
>>> Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git
>>> Tree: qemuu git://xenbits.xen.org/qemu-xen.git
>>> Tree: xen git://xenbits.xen.org/xen.git
>>>
>>> *** Found and reproduced problem changeset ***
>>>
>>>    Bug is in tree:  xen git://xenbits.xen.org/xen.git
>>>    Bug introduced:  702c9146c00d65d1e9c5955335ba002505e97e09
>>>    Bug not present: 52220b5f437a8d03ba108e127e7d717657edf99c
>>>    Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/138005/
>>>
>>>
>>>    commit 702c9146c00d65d1e9c5955335ba002505e97e09
>>>    Author: Julien Grall <julien.grall@arm.com>
>>>    Date:   Mon Apr 29 15:05:16 2019 +0100
>>>        xen/arm: Add an isb() before reading CNTPCT_EL0 to prevent re-ordering
>>>        Per D8.2.1 in ARM DDI 0487C.a, "a read to CNTPCT_EL0 can occur
>>>        speculatively and out of order relative to other instructions executed
>>>        on the same PE."
>>>        Add an instruction barrier to get accurate number of cycles when
>>>        requested in get_cycles(). For the other users of CNPCT_EL0, replace by
>>>        a call to get_cycles().
>>>        This is part of XSA-295.
>>>        Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>        Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>> Just in case you didn't notice this coming in.
> 
> I saw it this morning. But I fail to understand how the instruction barrier will 
> result in a timeout during the guest installation...

One thing to keep in mind here is isb() will also carry a compiler barrier. So 
the resulting binary may be different as the compiler may re-order the load.

One possibility is there are a missing data barrier in other part of the code. 
The isb() would expose it.

> 
> The more this only doesn't seem to appear on staging-4.12.

@Stefano, as we know staging-4.12 is working, one way to debug this is to try to 
reproduce in different commit between staging-4.12 with the isb() patch applied.

Hopefully this should test us where the bug was fixed.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Fwd: [xen-4.10-testing bisection] complete test-armhf-armhf-xl-arndale

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3822 bytes --]

On Wed, 19 Jun 2019, Julien Grall wrote:
> > On 6/19/19 8:28 AM, Jan Beulich wrote:
> > > > > > On 19.06.19 at 09:06, <osstest-admin@xenproject.org> wrote:
> > > > branch xen-4.10-testing
> > > > xenbranch xen-4.10-testing
> > > > job test-armhf-armhf-xl-arndale
> > > > testid debian-install
> > > > 
> > > > Tree: linux git://xenbits.xen.org/linux-pvops.git
> > > > Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
> > > > Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git
> > > > Tree: qemuu git://xenbits.xen.org/qemu-xen.git
> > > > Tree: xen git://xenbits.xen.org/xen.git
> > > > 
> > > > *** Found and reproduced problem changeset ***
> > > > 
> > > >    Bug is in tree:  xen git://xenbits.xen.org/xen.git
> > > >    Bug introduced:  702c9146c00d65d1e9c5955335ba002505e97e09
> > > >    Bug not present: 52220b5f437a8d03ba108e127e7d717657edf99c
> > > >    Last fail repro:
> > > > http://logs.test-lab.xenproject.org/osstest/logs/138005/
> > > > 
> > > > 
> > > >    commit 702c9146c00d65d1e9c5955335ba002505e97e09
> > > >    Author: Julien Grall <julien.grall@arm.com>
> > > >    Date:   Mon Apr 29 15:05:16 2019 +0100
> > > >        xen/arm: Add an isb() before reading CNTPCT_EL0 to prevent
> > > > re-ordering
> > > >        Per D8.2.1 in ARM DDI 0487C.a, "a read to CNTPCT_EL0 can occur
> > > >        speculatively and out of order relative to other instructions
> > > > executed
> > > >        on the same PE."
> > > >        Add an instruction barrier to get accurate number of cycles when
> > > >        requested in get_cycles(). For the other users of CNPCT_EL0,
> > > > replace by
> > > >        a call to get_cycles().
> > > >        This is part of XSA-295.
> > > >        Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > >        Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> > > 
> > > Just in case you didn't notice this coming in.
> > 
> > I saw it this morning. But I fail to understand how the instruction barrier
> > will result in a timeout during the guest installation...
> 
> One thing to keep in mind here is isb() will also carry a compiler barrier. So
> the resulting binary may be different as the compiler may re-order the load.
> 
> One possibility is there are a missing data barrier in other part of the code.
> The isb() would expose it.
> 
> > 
> > The more this only doesn't seem to appear on staging-4.12.
> 
> @Stefano, as we know staging-4.12 is working, one way to debug this is to try
> to reproduce in different commit between staging-4.12 with the isb() patch
> applied.
> 
> Hopefully this should test us where the bug was fixed.

Ian, I noticed the bisector hasn't managed to pinpoint a commit on
staging-4.11 yet. Obviously, we suspect it is the same patch ("xen/arm:
Add an isb() before reading CNTPCT_EL0 to prevent re-ordering") causing
the issue, but it would be really good to confirm.

Could you please schedule a bisector run on staging-4.11, ideally on
Arndale hardware (the same used for the bisection on 4.10 here)?


After we confirm that 4.11 is suffering from the same issue, we could
try to identify which commit "fixed" the problem between 4.11 and 4.12,
as 4.12 passed the tests yesterday. I am getting a bit ahead of myself
here, but I would love if we could use the bisector to spot the "fix"
somehow, maybe preparing a special branch for the bisector. The special
branch could start from the common root between staging-4.11 and
staging-4.12, which is tag 4.11.0-rc6, apply the patch that breaks, then
apply the other 4.12 commits, one of them we suspect is the fix we
need:

4.11.0-rc6 | CNTPCT_EL0 patch | other 4.12 commits
 ^               ^
 |               broken patch
 current common root between staging-4.12 and staging-4.11

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Fwd: [xen-4.10-testing bisection] complete test-armhf-armhf-xl-arndale

[Julien Grall]

Hi,

On 6/19/19 5:56 PM, Stefano Stabellini wrote:
> On Wed, 19 Jun 2019, Julien Grall wrote:
>>> On 6/19/19 8:28 AM, Jan Beulich wrote:
>>>>>>> On 19.06.19 at 09:06, <osstest-admin@xenproject.org> wrote:
>>>>> branch xen-4.10-testing
>>>>> xenbranch xen-4.10-testing
>>>>> job test-armhf-armhf-xl-arndale
>>>>> testid debian-install
>>>>>
>>>>> Tree: linux git://xenbits.xen.org/linux-pvops.git
>>>>> Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
>>>>> Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git
>>>>> Tree: qemuu git://xenbits.xen.org/qemu-xen.git
>>>>> Tree: xen git://xenbits.xen.org/xen.git
>>>>>
>>>>> *** Found and reproduced problem changeset ***
>>>>>
>>>>>     Bug is in tree:  xen git://xenbits.xen.org/xen.git
>>>>>     Bug introduced:  702c9146c00d65d1e9c5955335ba002505e97e09
>>>>>     Bug not present: 52220b5f437a8d03ba108e127e7d717657edf99c
>>>>>     Last fail repro:
>>>>> http://logs.test-lab.xenproject.org/osstest/logs/138005/
>>>>>
>>>>>
>>>>>     commit 702c9146c00d65d1e9c5955335ba002505e97e09
>>>>>     Author: Julien Grall <julien.grall@arm.com>
>>>>>     Date:   Mon Apr 29 15:05:16 2019 +0100
>>>>>         xen/arm: Add an isb() before reading CNTPCT_EL0 to prevent
>>>>> re-ordering
>>>>>         Per D8.2.1 in ARM DDI 0487C.a, "a read to CNTPCT_EL0 can occur
>>>>>         speculatively and out of order relative to other instructions
>>>>> executed
>>>>>         on the same PE."
>>>>>         Add an instruction barrier to get accurate number of cycles when
>>>>>         requested in get_cycles(). For the other users of CNPCT_EL0,
>>>>> replace by
>>>>>         a call to get_cycles().
>>>>>         This is part of XSA-295.
>>>>>         Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>>         Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>
>>>> Just in case you didn't notice this coming in.
>>>
>>> I saw it this morning. But I fail to understand how the instruction barrier
>>> will result in a timeout during the guest installation...
>>
>> One thing to keep in mind here is isb() will also carry a compiler barrier. So
>> the resulting binary may be different as the compiler may re-order the load.
>>
>> One possibility is there are a missing data barrier in other part of the code.
>> The isb() would expose it.
>>
>>>
>>> The more this only doesn't seem to appear on staging-4.12.
>>
>> @Stefano, as we know staging-4.12 is working, one way to debug this is to try
>> to reproduce in different commit between staging-4.12 with the isb() patch
>> applied.
>>
>> Hopefully this should test us where the bug was fixed.
> 
> Ian, I noticed the bisector hasn't managed to pinpoint a commit on
> staging-4.11 yet. Obviously, we suspect it is the same patch ("xen/arm:
> Add an isb() before reading CNTPCT_EL0 to prevent re-ordering") causing
> the issue, but it would be really good to confirm.
> 
> Could you please schedule a bisector run on staging-4.11, ideally on
> Arndale hardware (the same used for the bisection on 4.10 here)?
> 
> 
> After we confirm that 4.11 is suffering from the same issue, we could
> try to identify which commit "fixed" the problem between 4.11 and 4.12,
> as 4.12 passed the tests yesterday. I am getting a bit ahead of myself
> here, but I would love if we could use the bisector to spot the "fix"
> somehow, maybe preparing a special branch for the bisector. The special
> branch could start from the common root between staging-4.11 and
> staging-4.12, which is tag 4.11.0-rc6, apply the patch that breaks, then
> apply the other 4.12 commits, one of them we suspect is the fix we
> need:
> 
> 4.11.0-rc6 | CNTPCT_EL0 patch | other 4.12 commits
>   ^               ^
>   |               broken patch
>   current common root between staging-4.12 and staging-4.11
> 

Actually I may have found the error. I feel quite ashamed I didn't spot 
this during review and when the bisector fingered it.

staging-4.11 and staging.4.12 didn't have get_cycles implemented (i.e it 
returned 0). During the backport, get_cycles() got suddenly implemented 
(aside the isb()) so it now returns the number of cycles.

However, before commit da3d55ae67 "console: avoid printing no or null 
time stamps", cycles_t (return type of get_cycles) was unsigned long. On 
arm32 unsigned long is 32-bit and therefore does not cover the full 
counter (64-bit). So the number of cycles will be truncated leading to 
all sort of timing issue.

The correct fix is to switch cycles_t from unsigned long to uint64_t. I 
think we would need this to be backported for xen 4.9 and 4.8 as well.

I will send a fix.

This is the second instance where backport gone wrong in this XSA. 
Stefano, when you have a clash during backport, can you make sure to 
investigate the problem and also write in the commit message what you 
changed?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Fwd: [xen-4.10-testing bisection] complete test-armhf-armhf-xl-arndale

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 5789 bytes --]

On Thu, 20 Jun 2019, Julien Grall wrote:
> On 6/19/19 5:56 PM, Stefano Stabellini wrote:
> > On Wed, 19 Jun 2019, Julien Grall wrote:
> > > > On 6/19/19 8:28 AM, Jan Beulich wrote:
> > > > > > > > On 19.06.19 at 09:06, <osstest-admin@xenproject.org> wrote:
> > > > > > branch xen-4.10-testing
> > > > > > xenbranch xen-4.10-testing
> > > > > > job test-armhf-armhf-xl-arndale
> > > > > > testid debian-install
> > > > > > 
> > > > > > Tree: linux git://xenbits.xen.org/linux-pvops.git
> > > > > > Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
> > > > > > Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git
> > > > > > Tree: qemuu git://xenbits.xen.org/qemu-xen.git
> > > > > > Tree: xen git://xenbits.xen.org/xen.git
> > > > > > 
> > > > > > *** Found and reproduced problem changeset ***
> > > > > > 
> > > > > >     Bug is in tree:  xen git://xenbits.xen.org/xen.git
> > > > > >     Bug introduced:  702c9146c00d65d1e9c5955335ba002505e97e09
> > > > > >     Bug not present: 52220b5f437a8d03ba108e127e7d717657edf99c
> > > > > >     Last fail repro:
> > > > > > http://logs.test-lab.xenproject.org/osstest/logs/138005/
> > > > > > 
> > > > > > 
> > > > > >     commit 702c9146c00d65d1e9c5955335ba002505e97e09
> > > > > >     Author: Julien Grall <julien.grall@arm.com>
> > > > > >     Date:   Mon Apr 29 15:05:16 2019 +0100
> > > > > >         xen/arm: Add an isb() before reading CNTPCT_EL0 to prevent
> > > > > > re-ordering
> > > > > >         Per D8.2.1 in ARM DDI 0487C.a, "a read to CNTPCT_EL0 can
> > > > > > occur
> > > > > >         speculatively and out of order relative to other
> > > > > > instructions
> > > > > > executed
> > > > > >         on the same PE."
> > > > > >         Add an instruction barrier to get accurate number of cycles
> > > > > > when
> > > > > >         requested in get_cycles(). For the other users of CNPCT_EL0,
> > > > > > replace by
> > > > > >         a call to get_cycles().
> > > > > >         This is part of XSA-295.
> > > > > >         Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > > > >         Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> > > > > 
> > > > > Just in case you didn't notice this coming in.
> > > > 
> > > > I saw it this morning. But I fail to understand how the instruction
> > > > barrier
> > > > will result in a timeout during the guest installation...
> > > 
> > > One thing to keep in mind here is isb() will also carry a compiler
> > > barrier. So
> > > the resulting binary may be different as the compiler may re-order the
> > > load.
> > > 
> > > One possibility is there are a missing data barrier in other part of the
> > > code.
> > > The isb() would expose it.
> > > 
> > > > 
> > > > The more this only doesn't seem to appear on staging-4.12.
> > > 
> > > @Stefano, as we know staging-4.12 is working, one way to debug this is to
> > > try
> > > to reproduce in different commit between staging-4.12 with the isb() patch
> > > applied.
> > > 
> > > Hopefully this should test us where the bug was fixed.
> > 
> > Ian, I noticed the bisector hasn't managed to pinpoint a commit on
> > staging-4.11 yet. Obviously, we suspect it is the same patch ("xen/arm:
> > Add an isb() before reading CNTPCT_EL0 to prevent re-ordering") causing
> > the issue, but it would be really good to confirm.
> > 
> > Could you please schedule a bisector run on staging-4.11, ideally on
> > Arndale hardware (the same used for the bisection on 4.10 here)?
> > 
> > 
> > After we confirm that 4.11 is suffering from the same issue, we could
> > try to identify which commit "fixed" the problem between 4.11 and 4.12,
> > as 4.12 passed the tests yesterday. I am getting a bit ahead of myself
> > here, but I would love if we could use the bisector to spot the "fix"
> > somehow, maybe preparing a special branch for the bisector. The special
> > branch could start from the common root between staging-4.11 and
> > staging-4.12, which is tag 4.11.0-rc6, apply the patch that breaks, then
> > apply the other 4.12 commits, one of them we suspect is the fix we
> > need:
> > 
> > 4.11.0-rc6 | CNTPCT_EL0 patch | other 4.12 commits
> >   ^               ^
> >   |               broken patch
> >   current common root between staging-4.12 and staging-4.11
> > 
> 
> Actually I may have found the error. I feel quite ashamed I didn't spot this
> during review and when the bisector fingered it.
> 
> staging-4.11 and staging.4.12 didn't have get_cycles implemented (i.e it
> returned 0). During the backport, get_cycles() got suddenly implemented (aside
> the isb()) so it now returns the number of cycles.
> 
> However, before commit da3d55ae67 "console: avoid printing no or null time
> stamps", cycles_t (return type of get_cycles) was unsigned long. On arm32
> unsigned long is 32-bit and therefore does not cover the full counter
> (64-bit). So the number of cycles will be truncated leading to all sort of
> timing issue.
> 
> The correct fix is to switch cycles_t from unsigned long to uint64_t. I think
> we would need this to be backported for xen 4.9 and 4.8 as well.
> 
> I will send a fix.

Thank you, I am glad it got resolved quicker than expected.


> This is the second instance where backport gone wrong in this XSA. Stefano,
> when you have a clash during backport, can you make sure to investigate the
> problem and also write in the commit message what you changed?

Investigation is not bulletproof, in this case the issue slipped through
several reviews by multiple people. But one thing we can do is to adopt
the Linux common practice of writing the changes during backports and
merges between square brackets, such as:

  [fix blah blah blah]

It would make it a bit easier to spot mistakes.

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Fwd: [xen-4.10-testing bisection] complete test-armhf-armhf-xl-arndale

[Jan Beulich]

>>> On 20.06.19 at 19:24, <julien.grall@arm.com> wrote:
> Actually I may have found the error. I feel quite ashamed I didn't spot 
> this during review and when the bisector fingered it.
> 
> staging-4.11 and staging.4.12 didn't have get_cycles implemented (i.e it 
> returned 0). During the backport, get_cycles() got suddenly implemented 
> (aside the isb()) so it now returns the number of cycles.

Stefano, how can this be a valid backport under the given title? The
(imo) only correct way of backporting that hunk would have been to
simply drop it, adding isb() instead of the switch to call the function
in the two other places.

To both of you: How certain are you that the subsequent type
change is really all that's needed, and that the sudden change in
behavior of get_cycles() won't have other undue side effects?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Fwd: [xen-4.10-testing bisection] complete test-armhf-armhf-xl-arndale

[Julien Grall]

Hi Jan,

On 21/06/2019 09:58, Jan Beulich wrote:
>>>> On 20.06.19 at 19:24, <julien.grall@arm.com> wrote:
>> Actually I may have found the error. I feel quite ashamed I didn't spot
>> this during review and when the bisector fingered it.
>>
>> staging-4.11 and staging.4.12 didn't have get_cycles implemented (i.e it
>> returned 0). During the backport, get_cycles() got suddenly implemented
>> (aside the isb()) so it now returns the number of cycles.
> 
> Stefano, how can this be a valid backport under the given title? The
> (imo) only correct way of backporting that hunk would have been to
> simply drop it, adding isb() instead of the switch to call the function
> in the two other places.
> 
> To both of you: How certain are you that the subsequent type
> change is really all that's needed, and that the sudden change in
> behavior of get_cycles() won't have other undue side effects?

Here the current callers:

42sh> ack get_cycles

include/asm-arm/time.h
12:static inline cycles_t get_cycles (void)

include/asm-x86/time.h
29:static inline cycles_t get_cycles(void)

arch/arm/time.c
152:    boot_count = get_cycles();
193:    uint64_t ticks = get_cycles() - boot_count;

arch/x86/irq.c
898:        tsc_in = tb_init_done ? get_cycles() : 0;
900:        TRACE_3D(TRC_HW_IRQ_HANDLED, irq, tsc_in, get_cycles());
921:        tsc_in = tb_init_done ? get_cycles() : 0;
923:        TRACE_3D(TRC_HW_IRQ_HANDLED, irq, tsc_in, get_cycles());

common/tmem.c
275:    pgp->timestamp = get_cycles();
389:    life = get_cycles() - pgp->timestamp;

common/keyhandler.c
389:    per_cpu(read_cycles_time, cpu) = get_cycles();
412:    per_cpu(read_cycles_time, cpu) = get_cycles();

common/trace.c
595:        u64 tsc = (u64)get_cycles();
778:            this_cpu(lost_records_first_tsc)=(u64)get_cycles();

The two callers we care the most are in arch/arm/time.c. They were added with 
the patch backported and I know this is fine because the value before was 64-bit 
as well. So there are no change except for the isb() here.

I am not sure anyone ever tested/used tmem.c on Arm (and it is dropped on recent 
release). So that's not going to make much difference.

common/keyhandler.c, this is mostly for debugging.

common/trace.c does not matter as tracing does not work on Arm before 4.12.

So I am pretty confident that this change will not provoke more side effects 
than an isb() directly in the code.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel