[Xen-devel] [linux-4.14 test] 133353: regressions - trouble: blocked/broken/fail/pass

[osstest service owner]

flight 133353 linux-4.14 real [real]
http://logs.test-lab.xenproject.org/osstest/logs/133353/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-xl-qemut-debianhvm-amd64-xsmbroken
 test-amd64-amd64-libvirt-xsm broken
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrictbroken
 test-amd64-i386-xl-qemuu-ovmf-amd64 broken
 test-amd64-amd64-i386-pvgrub broken
 test-amd64-i386-xl-qemuu-ws16-amd64 broken
 build-armhf-pvopsbroken
 test-amd64-i386-xl-qemuu-debianhvm-amd64-xsmbroken
 build-armhf-pvops 4 host-install(4)broken REGR. vs. 133261
 test-amd64-amd64-xl  broken  in 133326
 build-armhf  broken  in 133326
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm  broken in 133326
 test-amd64-amd64-libvirt-vhd broken  in 133326
 test-amd64-i386-xl-qemuu-win10-i386   broken in 133326
 test-amd64-amd64-xl-pvhv2-intel   broken in 133326
 test-amd64-amd64-xl-qemut-debianhvm-amd64-xsm broken in 133326
 test-amd64-amd64-xl-qemut-ws16-amd64  broken in 133326
 test-amd64-amd64-xl-qemuu-ws16-amd64  broken in 133326
 test-amd64-amd64-xl-qemut-win7-amd64  broken in 133326
 test-amd64-amd64-xl-qcow2broken  in 133326
 test-amd64-i386-qemuu-rhel6hvm-intel  broken in 133326
 test-amd64-i386-xl-qemut-win10-i386   broken in 133326
 test-amd64-i386-xl-qemut-win7-amd64   broken in 133326
 build-armhf4 host-install(4) broken in 133326 REGR. vs. 133261
 test-amd64-amd64-xl-qemut-debianhvm-amd64-xsm 14 guest-localmigrate fail REGR. 
vs. 133261
 test-amd64-amd64-libvirt-vhd 10 debian-di-installfail REGR. vs. 133261
 test-amd64-amd64-qemuu-nested-intel 17 debian-hvm-install/l1/l2 fail REGR. vs. 
133261
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install 
fail in 133326 REGR. vs. 133261
 test-amd64-i386-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail in 133326 REGR. 
vs. 133261
 test-amd64-amd64-i386-pvgrub 10 debian-di-install fail in 133326 REGR. vs. 
133261

Tests which are failing intermittently (not blocking):
 test-amd64-i386-xl-qemut-win7-amd64 4 host-install(4) broken in 133326 pass in 
133353
 test-amd64-amd64-xl-qemut-ws16-amd64 4 host-install(4) broken in 133326 pass 
in 133353
 test-amd64-amd64-xl-qemuu-ws16-amd64 4 host-install(4) broken in 133326 pass 
in 133353
 test-amd64-amd64-libvirt-vhd 4 host-install(4) broken in 133326 pass in 133353
 test-amd64-amd64-xl  4 host-install(4) broken in 133326 pass in 133353
 test-amd64-i386-xl-qemuu-win10-i386 4 host-install(4) broken in 133326 pass in 
133353
 test-amd64-i386-qemuu-rhel6hvm-intel 4 host-install(4) broken in 133326 pass 
in 133353
 test-amd64-amd64-xl-qemut-debianhvm-amd64-xsm 4 host-install(4) broken in 
133326 pass in 133353
 test-amd64-amd64-xl-pvhv2-intel 4 host-install(4) broken in 133326 pass in 
133353
 test-amd64-amd64-xl-qemut-win7-amd64 4 host-install(4) broken in 133326 pass 
in 133353
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 4 host-install(4) broken in 
133326 pass in 133353
 test-amd64-i386-xl-qemut-win10-i386 4 host-install(4) broken in 133326 pass in 
133353
 test-amd64-amd64-xl-qcow24 host-install(4) broken in 133326 pass in 133353
 test-amd64-i386-xl-qemut-debianhvm-amd64-xsm 4 host-install(4) broken pass in 
133326
 test-amd64-amd64-i386-pvgrub  4 host-install(4)  broken pass in 133326
 test-amd64-i386-xl-qemuu-ws16-amd64  4 host-install(4)   broken pass in 133326
 test-amd64-i386-xl-qemuu-ovmf-amd64  4 host-install(4)   broken pass in 133326
 test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm 4 host-install(4) broken pass in 
133326
 test-amd64-amd64-libvirt-xsm  4 host-install(4)  broken pass in 133326
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 4 host-install(4) broken 
pass in 133326
 test-amd64-i386-xl  20 guest-start/debian.repeat fail in 133326 pass in 133353
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 10 debian-hvm-install 
fail in 133326 pass in 133353
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsm 18 guest-start/debianhvm.repeat 
fail in 133326 pass in 133353
 test-amd64-amd64-amd64-pvgrub 10 debian-di-install fail in 133326 pass in 
133353
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 debian-hvm-install fail 
in 133326 pass in 133353
 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 5 host-ping-check-native 
fail pass in 133326
 test-amd64-i386-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail pass in 
133326
 test-amd64-i386-xl-qemut-debianhvm-amd64 10 debian-hvm-install fail pass in 
133326

Tests which did not succeed, but are not blocking:
 build-armhf-libvirt   1 

[Xen-devel] [xen-unstable test] 133345: regressions - trouble: blocked/broken/pass

[osstest service owner]

flight 133345 xen-unstable real [real]
http://logs.test-lab.xenproject.org/osstest/logs/133345/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-arm64  broken
 build-amd64-prev broken
 build-amd64  broken
 build-amd64-pvopsbroken
 build-arm64-xsm  broken
 test-armhf-armhf-xl-cubietruck broken
 test-armhf-armhf-libvirt broken
 build-i386-pvops broken
 build-i386-pvops  2 hosts-allocate broken REGR. vs. 133300
 build-amd64-pvops 2 hosts-allocate broken REGR. vs. 133300
 build-amd64   2 hosts-allocate broken REGR. vs. 133300
 build-amd64-prev  2 hosts-allocate broken REGR. vs. 133300
 test-amd64-amd64-xl-pvhv2-intel   broken in 133316
 test-amd64-amd64-xl-qemut-win7-amd64  broken in 133316
 test-armhf-armhf-libvirt-raw broken  in 133316
 build-i386-pvops   4 host-install(4) broken in 133316 REGR. vs. 133300
 test-amd64-amd64-xl-qemut-win7-amd64 4 host-install(4) broken in 133316 REGR. 
vs. 133300
 test-amd64-amd64-xl-pvhv2-intel 4 host-install(4) broken in 133316 REGR. vs. 
133300
 test-xtf-amd64-amd64-37 xen-boot   fail in 133316 REGR. vs. 133300
 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install 
fail in 133316 REGR. vs. 133300
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow  1 build-check(1) running
 test-amd64-amd64-qemuu-nested-amd  1 build-check(1)   running
 build-amd64-rumprun   1 build-check(1)   running
 test-amd64-amd64-xl-credit2   1 build-check(1)   running
 test-amd64-amd64-livepatch1 build-check(1)   running
 test-amd64-amd64-xl-credit1   1 build-check(1)   running
 test-amd64-amd64-libvirt-pair  1 build-check(1)   running
 test-amd64-amd64-xl   1 build-check(1)   running
 test-amd64-amd64-xl-qemuu-ovmf-amd64  1 build-check(1)   running
 test-amd64-amd64-xl-qemuu-win7-amd64  1 build-check(1)   running
 test-amd64-amd64-xl-qemut-debianhvm-amd64  1 build-check(1)running
 build-amd64-libvirt   1 build-check(1)   running
 test-amd64-amd64-xl-multivcpu  1 build-check(1)   running
 test-amd64-amd64-xl-shadow1 build-check(1)   running
 test-amd64-amd64-pygrub   1 build-check(1)   running
 build-arm64-libvirt   1 build-check(1)   running
 test-amd64-amd64-xl-qemut-ws16-amd64  1 build-check(1)   running
 test-amd64-amd64-xl-pvshim1 build-check(1)   running
 test-amd64-amd64-xl-qemuu-debianhvm-amd64  1 build-check(1)running
 test-amd64-amd64-xl-qemut-win10-i386  1 build-check(1)   running
 test-amd64-amd64-xl-rtds  1 build-check(1)   running
 test-amd64-amd64-xl-qcow2 1 build-check(1)   running
 test-amd64-amd64-qemuu-nested-intel  1 build-check(1)   running
 test-amd64-amd64-i386-pvgrub  1 build-check(1)   running
 test-amd64-amd64-xl-pvhv2-amd  1 build-check(1)   running
 test-amd64-amd64-amd64-pvgrub  1 build-check(1)   running
 test-amd64-amd64-xl-qemuu-win10-i386  1 build-check(1)   running
 test-amd64-amd64-xl-qemut-win7-amd64  1 build-check(1)   running
 test-amd64-amd64-libvirt-vhd  1 build-check(1)   running
 test-amd64-amd64-pair 1 build-check(1)   running
 test-amd64-amd64-libvirt  1 build-check(1)   running
 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 1 build-check(1) running
 test-amd64-amd64-xl-pvhv2-intel  1 build-check(1)   running
 test-amd64-amd64-migrupgrade  1 build-check(1)   running
 test-amd64-amd64-examine  1 build-check(1)   running
 test-amd64-amd64-rumprun-amd64  1 build-check(1)   running
 test-amd64-amd64-xl-qemuu-ws16-amd64  1 build-check(1)   running

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-libvirt-raw 4 host-install(4) broken in 133316 pass in 133345
 test-armhf-armhf-libvirt  4 host-install(4)  broken pass in 133316
 test-armhf-armhf-xl-cubietruck  4 host-install(4)broken pass in 133316
 test-armhf-armhf-xl-vhd 10 debian-di-install fail in 133316 pass in 133345

Regressions which are regarded as allowable (not blocking):
 build-arm64   2 hosts-allocate broken REGR. vs. 133300
 build-arm64-xsm   2 hosts-allocate broken REGR. vs. 133300

Tests which did not succeed, but are not blocking:
 

[Xen-devel] [linux-4.4 test] 133352: regressions - trouble: blocked/broken/fail/pass

[osstest service owner]

flight 133352 linux-4.4 real [real]
http://logs.test-lab.xenproject.org/osstest/logs/133352/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-armhf-armhf-xl-credit2  broken
 test-armhf-armhf-xl  broken
 test-armhf-armhf-xl-vhd  broken
 build-amd64  broken
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsm   broken
 build-amd64   4 host-install(4)broken REGR. vs. 133285
 test-armhf-armhf-xl-vhd   4 host-install(4)broken REGR. vs. 133285
 test-armhf-armhf-libvirt-raw broken  in 133327
 test-amd64-i386-xl-xsm   broken  in 133327
 test-armhf-armhf-xl-multivcpu broken in 133327
 test-amd64-amd64-xl-qemuu-win10-i386  broken in 133327
 test-amd64-i386-xl-qemuu-win10-i386   broken in 133327
 test-amd64-amd64-xl-qemut-ws16-amd64  broken in 133327
 test-amd64-i386-xl-qemuu-win10-i386 4 host-install(4) broken in 133327 REGR. 
vs. 133285
 test-amd64-amd64-xl-qemuu-win10-i386 4 host-install(4) broken in 133327 REGR. 
vs. 133285
 test-amd64-amd64-examine  5 host-install broken in 133327 REGR. vs. 133285
 test-armhf-armhf-libvirt-raw 10 debian-di-installfail REGR. vs. 133285
 test-amd64-amd64-amd64-pvgrub 8 host-ping-check-xen fail in 133327 REGR. vs. 
133285
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 8 host-ping-check-xen fail in 
133327 REGR. vs. 133285
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsm 10 debian-hvm-install fail in 
133327 REGR. vs. 133285
 test-amd64-amd64-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail in 133327 
REGR. vs. 133285
 test-amd64-amd64-xl-qcow2 10 debian-di-install fail in 133327 REGR. vs. 133285
 test-amd64-amd64-i386-pvgrub 10 debian-di-install fail in 133327 REGR. vs. 
133285
 test-amd64-i386-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail in 133327 REGR. 
vs. 133285
 test-amd64-amd64-xl-qemut-ws16-amd64 16 guest-localmigrate/x10 fail in 133327 
REGR. vs. 133285
 test-amd64-amd64-pygrub   10 debian-di-install fail in 133327 REGR. vs. 133285
 test-armhf-armhf-xl-credit2 16 guest-start/debian.repeat fail in 133327 REGR. 
vs. 133285

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-libvirt-raw 4 host-install(4) broken in 133327 pass in 133352
 test-amd64-i386-xl-xsm   4 host-install(4) broken in 133327 pass in 133352
 test-armhf-armhf-xl-vhd   3 syslog-server  broken in 133327 pass in 133352
 test-armhf-armhf-xl   4 host-install(4)  broken pass in 133327
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsm 4 host-install(4) broken pass in 
133327
 test-armhf-armhf-xl-credit2   4 host-install(4)  broken pass in 133327
 test-armhf-armhf-xl-multivcpu 10 debian-install  fail in 133327 pass in 133352
 test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 10 debian-hvm-install 
fail pass in 133327
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 18 
guest-start/debianhvm.repeat fail pass in 133327

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemut-win10-i386  1 build-check(1) blocked n/a
 test-amd64-amd64-libvirt  1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemut-ws16-amd64  1 build-check(1) blocked n/a
 test-amd64-amd64-qemuu-nested-intel  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemut-win7-amd64  1 build-check(1)  blocked n/a
 test-amd64-amd64-xl-qemut-debianhvm-amd64  1 build-check(1)blocked n/a
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow  1 build-check(1) blocked n/a
 test-amd64-i386-pair  1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-rtds  1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-win10-i386  1 build-check(1)  blocked n/a
 build-amd64-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-raw1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-vhd  1 build-check(1)   blocked  n/a
 test-amd64-amd64-examine  1 build-check(1)   blocked  n/a
 test-amd64-amd64-amd64-pvgrub  1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemut-win10-i386  1 build-check(1)  blocked n/a
 test-amd64-amd64-xl-qemuu-win7-amd64  1 build-check(1) blocked n/a
 test-amd64-i386-qemuu-rhel6hvm-intel  1 build-check(1) blocked n/a
 test-amd64-i386-xl-qemuu-ovmf-amd64  1 build-check(1)  blocked n/a
 test-amd64-amd64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-amd64-i386-freebsd10-i386  1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-pvhv2-amd  1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemut-debianhvm-amd64  1 build-check(1) blocked n/a
 

[Xen-devel] [xen-unstable-smoke test] 133382: tolerable all pass - PUSHED

[osstest service owner]

flight 133382 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/133382/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  e72ecc7615410e5bf1a1c9a4c7772322c16eeb82
baseline version:
 xen  db2af23d15077605f286d8ef86c8f5d9c1b8302a

Last test of basis   133343  2019-02-21 03:00:49 Z1 days
Testing same since   133371  2019-02-22 15:00:34 Z0 days3 attempts


People who touched revisions under test:
  Andrew Cooper 
  Jan Beulich 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-i386 pass
 test-amd64-amd64-libvirt pass



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


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   db2af23d15..e72ecc7615  e72ecc7615410e5bf1a1c9a4c7772322c16eeb82 -> smoke

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

Re: [Xen-devel] [PATCH] x86/hvm: Intercept RDPMC when vPMU is disabled

[Boris Ostrovsky]

On 2/22/19 5:44 PM, Andrew Cooper wrote:
> On 22/02/2019 21:58, Boris Ostrovsky wrote:
>> On 2/22/19 4:13 PM, Andrew Cooper wrote:
>>> vPMU isn't security supported, and in general guests can't access any of the
>>> performance counter MSRs.  However, the RDPMC instruction isn't intercepted,
>>> meaning that guest software can read the instantaneous counter values.
>>>
>>> When vPMU isn't configured, intercept RDPMC and unconditionally fail it as 
>>> if
>>> software has requested a bad counter index (#GP fault).  It is model 
>>> specific
>>> as to which counters are available to begin with, and in levelled scenarios,
>>> this information may not be accurate in the first place.
>>>
>>> This change isn't expected to have any impact on VMs.  Userspace is not
>>> usually given access to RDPMC (Windows appear to completely prohibit it; 
>>> Linux
>>> is restricted to root), and kernels won't be executing RDPMC instructions if
>>> their PMU drivers have failed to start.
>>>
>>> Signed-off-by: Andrew Cooper 
>>> ---
>>> CC: Jan Beulich 
>>> CC: Wei Liu 
>>> CC: Roger Pau Monné 
>>> CC: Jun Nakajima 
>>> CC: Kevin Tian 
>>> CC: Boris Ostrovsky 
>>> CC: Suravee Suthikulpanit 
>>> CC: Brian Woods 
>>> CC: Juergen Gross 
>>>
>>> This should be taken into Xen 4.12 and backported to the stable releases.
>>> While it isn't an XSA itself, it is an information leak (Xen's NMI watchdog 
>>> in
>>> particular) which could be advantagous to an attacker trying to exploit a 
>>> race
>>> condition.
>>>
>>> The only other option is to emulate the reported family and offer back all 
>>> 0's
>>> for the accessable counters.  Obviously this is a non-starter.
>> When VPMU is off MSR reads return zero.
> That behaviour isn't long for this world.
>
>> While it is debatable whether this the right action, shouldn't rdpmc behave 
>> in the same fashion?
> I specifically don't want to propagate the "lets complete with zero"
> behaviour further, because it takes away #GP faults which the guest
> would otherwise get.

The guest should get a #GP on Intel if CPUID is not reporting any
counters but not on AMD where the first 4 counters are architectural.



-boris

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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Andrew Cooper]

On 22/02/2019 23:22, Julien Grall wrote:
> Hi,
>
> On 22/02/2019 22:34, Andrew Cooper wrote:
>> On 22/02/2019 22:11, Julien Grall wrote:
>>> Hi Stefano,
>>>
>>> On 22/02/2019 21:58, Stefano Stabellini wrote:
 On Fri, 22 Feb 2019, Andrew Cooper wrote:
> On 22/02/2019 21:00, Stefano Stabellini wrote:
>> On Fri, 22 Feb 2019, Julien Grall wrote:
>> BTW, I checked the series with -Wswitch-default:
>> -Wswitch-default
>> Warn whenever a switch statement does not have a default case.
>>> Furthermore, using BUG() is a pretty bad idea in switch.
>> It is and not only in the switch. The reason I put BUG is that I 
>> tried
>> to follow
>> the existing "error handling" at those places.
> It is not because BUG() is been used today in some places that we 
> need to
> continue to spread it.
>
>> Use of BUG() itself is another topic which will also need to be
>> addressed
> So we should not add more of them...
 Again, I see this as a dedicated change. So, in the current series I 
 think
 it is
 acceptable to use the existing way of error handling if any at all.
>>> That's not how it works in upstream. If you know some constructs are 
>>> wrong, it
>>> is best to try to address partially the problem directly then having so 
>>> you
>>> reduce the amounts of change afterwards.
>>>
>>> So please try to not introduce more BUG() in the code base.
>> Hi Oleksandr, Julien,
>>
>> Julien's right that we should not introduce any more BUG()s. In fact,
>> each of them makes the code less safe, not more safe! The purpose of
>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>> more (not less!) resilient to failure.
>>
>> So, I think it is a good idea to introduce a default label because it
>> can help us spot unexpected issues. Instead of calling BUG() in the
>> default handler, which is detrimental, we should return an error when
>> possible, or just print a warning.
> domain_crash() is almost always better than BUG().  It is very obvious
> if it gets hit, and wont crash Xen.
 That's a good suggestion.


>> As 16.4 clearly state, even a simple comment would be enough to address
>> the rule. We just need to explain why a default label is not needed.
>> Such as:
>>
>> default:
>> /* unreachable because blah and blah */
> What a simple comment doesn't do is avoid breaking -Wswitch.
 I don't know how to reconcile 16.4 with -Wswitch. One could argue that
 -Wswitch could be a good way to address 16.4, but then we introduce a
 compiler specific requirement. Typically gcc is not the compiler of
 choice for these environments, unfortunately forcing gcc is not an
 option.
>>> Well, you could build with GCC and then build with your custom
>>> compiler... But, GCC is pretty much the only choice for Xen on Arm today
>>> as we don't build with clang and I pretty doubt we can build with compcert.
>> So the suggestion I had was to have an overall CONFIG_MISRA which we can
>> hide some of this nonsense behind, and then
>>
>> #ifdef CONFIG_MISRA
>> #define MISRA_BLE_DEFAULT default:
>> #else
>> #define MISRA_BLE_DEFAULT
>> #endif
> This is pretty disgusting :). But then, it makes the code is bit more 
> obscure. So how that rule is making Xen more safe? Furthermore, one 
> default may not rule them all. So aren't we just adding code to make 
> MISRA happy at the risk of introducing more bug?

Any switch statement where the default isn't empty
(assert/bug/domain_crash/print) should probably be a regular default:

I very much doubt MISRA intended for people to make themselves compliant
by adding "default: break;" everywhere, but lethargy is a powerful
driving force.

An alternative, and substantially more ugly would be to have the else
case be "if ( 0 )" or similar, so you do take out the full next block,
but that requires people to member braces.

Even more ugly would be to take (x) and construct the block manually,
but then you get at minimum a set of brackets where you would expect to
see braces in normal C, and this alone is confusing to read.

>> So when you disable CONFIG_MISRA, your compiler starts being able to
>> help you again.
>>
>> TBH, it would also be nice to hide the SYMBOL nonsense behind, so we can
>> continue doing it the efficient way for ~100% of the time.
> My main concern is we only looked at 2 MISRA rules and we already need 
> some nonsense code to spread everywhere. How much more are we going to 
> get like that?

MISRA scanning is just like Coverity scanning.  The item classes with
the highest seen-count tend to be the least interesting and relevant, as
they tend to be the ones where we are systematically different.

It is the one-off flagged issues which tend to be important.

~Andrew

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 22/02/2019 22:38, Stefano Stabellini wrote:
> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>> On 22/02/2019 22:11, Julien Grall wrote:
>>> Hi Stefano,
>>>
>>> On 22/02/2019 21:58, Stefano Stabellini wrote:
 On Fri, 22 Feb 2019, Andrew Cooper wrote:
> On 22/02/2019 21:00, Stefano Stabellini wrote:
>> On Fri, 22 Feb 2019, Julien Grall wrote:
>> BTW, I checked the series with -Wswitch-default:
>> -Wswitch-default
>> Warn whenever a switch statement does not have a default case.
>>> Furthermore, using BUG() is a pretty bad idea in switch.
>> It is and not only in the switch. The reason I put BUG is that I 
>> tried
>> to follow
>> the existing "error handling" at those places.
> It is not because BUG() is been used today in some places that we 
> need to
> continue to spread it.
>
>> Use of BUG() itself is another topic which will also need to be
>> addressed
> So we should not add more of them...
 Again, I see this as a dedicated change. So, in the current series I 
 think
 it is
 acceptable to use the existing way of error handling if any at all.
>>> That's not how it works in upstream. If you know some constructs are 
>>> wrong, it
>>> is best to try to address partially the problem directly then having so 
>>> you
>>> reduce the amounts of change afterwards.
>>>
>>> So please try to not introduce more BUG() in the code base.
>> Hi Oleksandr, Julien,
>>
>> Julien's right that we should not introduce any more BUG()s. In fact,
>> each of them makes the code less safe, not more safe! The purpose of
>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>> more (not less!) resilient to failure.
>>
>> So, I think it is a good idea to introduce a default label because it
>> can help us spot unexpected issues. Instead of calling BUG() in the
>> default handler, which is detrimental, we should return an error when
>> possible, or just print a warning.
> domain_crash() is almost always better than BUG().  It is very obvious
> if it gets hit, and wont crash Xen.
 That's a good suggestion.


>> As 16.4 clearly state, even a simple comment would be enough to address
>> the rule. We just need to explain why a default label is not needed.
>> Such as:
>>
>> default:
>> /* unreachable because blah and blah */
> What a simple comment doesn't do is avoid breaking -Wswitch.
 I don't know how to reconcile 16.4 with -Wswitch. One could argue that
 -Wswitch could be a good way to address 16.4, but then we introduce a
 compiler specific requirement. Typically gcc is not the compiler of
 choice for these environments, unfortunately forcing gcc is not an
 option.
>>> Well, you could build with GCC and then build with your custom
>>> compiler... But, GCC is pretty much the only choice for Xen on Arm today
>>> as we don't build with clang and I pretty doubt we can build with compcert.
>>
>> So the suggestion I had was to have an overall CONFIG_MISRA which we can
>> hide some of this nonsense behind, and then
>>
>> #ifdef CONFIG_MISRA
>> #define MISRA_BLE_DEFAULT default:
>> #else
>> #define MISRA_BLE_DEFAULT
>> #endif
>>
>> So when you disable CONFIG_MISRA, your compiler starts being able to
>> help you again.
> 
> This is actually a good way to make progress which could make everybody
> happy. (And it wouldn't require a slow back-and-forth with third parties
> to ask difficult questions.)

I can tell you I am not happy with that :). We would make the code more 
obscure. So it raises question on what would be the benefits of adopting 
the rule in Xen.

But maybe the first question is how much we need to adhere to those 
rules. What are the consequences of not following them in Xen Project? I 
know that some upstream project chose to not apply to all the rules.

Cheers,

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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 22/02/2019 22:34, Andrew Cooper wrote:
> On 22/02/2019 22:11, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 22/02/2019 21:58, Stefano Stabellini wrote:
>>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
 On 22/02/2019 21:00, Stefano Stabellini wrote:
> On Fri, 22 Feb 2019, Julien Grall wrote:
> BTW, I checked the series with -Wswitch-default:
> -Wswitch-default
> Warn whenever a switch statement does not have a default case.
>> Furthermore, using BUG() is a pretty bad idea in switch.
> It is and not only in the switch. The reason I put BUG is that I tried
> to follow
> the existing "error handling" at those places.
 It is not because BUG() is been used today in some places that we need 
 to
 continue to spread it.

> Use of BUG() itself is another topic which will also need to be
> addressed
 So we should not add more of them...
>>> Again, I see this as a dedicated change. So, in the current series I 
>>> think
>>> it is
>>> acceptable to use the existing way of error handling if any at all.
>> That's not how it works in upstream. If you know some constructs are 
>> wrong, it
>> is best to try to address partially the problem directly then having so 
>> you
>> reduce the amounts of change afterwards.
>>
>> So please try to not introduce more BUG() in the code base.
> Hi Oleksandr, Julien,
>
> Julien's right that we should not introduce any more BUG()s. In fact,
> each of them makes the code less safe, not more safe! The purpose of
> MISRAC 16.4 is "defensive programming": write the code in a way that is
> more (not less!) resilient to failure.
>
> So, I think it is a good idea to introduce a default label because it
> can help us spot unexpected issues. Instead of calling BUG() in the
> default handler, which is detrimental, we should return an error when
> possible, or just print a warning.
 domain_crash() is almost always better than BUG().  It is very obvious
 if it gets hit, and wont crash Xen.
>>> That's a good suggestion.
>>>
>>>
> As 16.4 clearly state, even a simple comment would be enough to address
> the rule. We just need to explain why a default label is not needed.
> Such as:
>
> default:
> /* unreachable because blah and blah */
 What a simple comment doesn't do is avoid breaking -Wswitch.
>>> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
>>> -Wswitch could be a good way to address 16.4, but then we introduce a
>>> compiler specific requirement. Typically gcc is not the compiler of
>>> choice for these environments, unfortunately forcing gcc is not an
>>> option.
>> Well, you could build with GCC and then build with your custom
>> compiler... But, GCC is pretty much the only choice for Xen on Arm today
>> as we don't build with clang and I pretty doubt we can build with compcert.
> 
> So the suggestion I had was to have an overall CONFIG_MISRA which we can
> hide some of this nonsense behind, and then
> 
> #ifdef CONFIG_MISRA
> #define MISRA_BLE_DEFAULT default:
> #else
> #define MISRA_BLE_DEFAULT
> #endif

This is pretty disgusting :). But then, it makes the code is bit more 
obscure. So how that rule is making Xen more safe? Furthermore, one 
default may not rule them all. So aren't we just adding code to make 
MISRA happy at the risk of introducing more bug?

>
> So when you disable CONFIG_MISRA, your compiler starts being able to
> help you again.
> 
> TBH, it would also be nice to hide the SYMBOL nonsense behind, so we can
> continue doing it the efficient way for ~100% of the time.

My main concern is we only looked at 2 MISRA rules and we already need 
some nonsense code to spread everywhere. How much more are we going to 
get like that?

Cheers,

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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 22/02/2019 22:34, Stefano Stabellini wrote:
> On Fri, 22 Feb 2019, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 22/02/2019 21:58, Stefano Stabellini wrote:
>>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
 On 22/02/2019 21:00, Stefano Stabellini wrote:
> On Fri, 22 Feb 2019, Julien Grall wrote:
> BTW, I checked the series with -Wswitch-default:
> -Wswitch-default
> Warn whenever a switch statement does not have a default case.
>> Furthermore, using BUG() is a pretty bad idea in switch.
> It is and not only in the switch. The reason I put BUG is that I tried
> to follow
> the existing "error handling" at those places.
 It is not because BUG() is been used today in some places that we need 
 to
 continue to spread it.

> Use of BUG() itself is another topic which will also need to be
> addressed
 So we should not add more of them...
>>> Again, I see this as a dedicated change. So, in the current series I 
>>> think
>>> it is
>>> acceptable to use the existing way of error handling if any at all.
>> That's not how it works in upstream. If you know some constructs are 
>> wrong, it
>> is best to try to address partially the problem directly then having so 
>> you
>> reduce the amounts of change afterwards.
>>
>> So please try to not introduce more BUG() in the code base.
> Hi Oleksandr, Julien,
>
> Julien's right that we should not introduce any more BUG()s. In fact,
> each of them makes the code less safe, not more safe! The purpose of
> MISRAC 16.4 is "defensive programming": write the code in a way that is
> more (not less!) resilient to failure.
>
> So, I think it is a good idea to introduce a default label because it
> can help us spot unexpected issues. Instead of calling BUG() in the
> default handler, which is detrimental, we should return an error when
> possible, or just print a warning.

 domain_crash() is almost always better than BUG().  It is very obvious
 if it gets hit, and wont crash Xen.
>>>
>>> That's a good suggestion.
>>>
>>>
> As 16.4 clearly state, even a simple comment would be enough to address
> the rule. We just need to explain why a default label is not needed.
> Such as:
>
> default:
> /* unreachable because blah and blah */

 What a simple comment doesn't do is avoid breaking -Wswitch.
>>>
>>> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
>>> -Wswitch could be a good way to address 16.4, but then we introduce a
>>> compiler specific requirement. Typically gcc is not the compiler of
>>> choice for these environments, unfortunately forcing gcc is not an
>>> option.
>>
>> Well, you could build with GCC and then build with your custom
>> compiler...
> 
> This suggestion is problematic: as an individual interested in MISRA-C
> compliance, I only have the MISRA-C rules in my hands. I don't know how
> to deal with suggestions like this one, that don't comply to the Rules,
> but it tries to address the same issue in a different manner.

Are you suggesting we will have to abide to all the rules even if they 
doesn't make things worst? I was under the impression we don't necessary 
need to follow a rule if we have justification for it.

> 
> I cannot rule out that it wouldn't work, but also I cannot be sure that
> it would work. In short, I have no way to make progress or to find out
> how to move forward. I guess as a contributor I would be forced to go
> back to the MISRAC compliance experts and ask for their opinion. (One
> non-technical issue is who is going to pay them for spending their time
> on this.) But what if they say it is not acceptable for compliance?

I appreciate people might want to use Xen with MISRA C. But I am not 
convinced we should bend to some rules in Xen Project for the sake of 
making MISRA happy. People could carry such patch themselves if they 
care about it.

> 
> This is a great topic to discuss in March and decide what to do in these
> situations.
> 
> 
>> But, GCC is pretty much the only choice for Xen on Arm today
>> as we don't build with clang and I pretty doubt we can build with compcert.
> 
> Obviously, this has to change if we want to make progress on safety
> certifications.

I am curious to know what is plan for this. I mean that if no-one is 
planning to make Xen build with other compilers. Then what would be the 
benefits of this?

Cheers,

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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 22/02/2019 22:34, Stefano Stabellini wrote:
> On Fri, 22 Feb 2019, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 22/02/2019 21:58, Stefano Stabellini wrote:
>>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
 On 22/02/2019 21:00, Stefano Stabellini wrote:
> On Fri, 22 Feb 2019, Julien Grall wrote:
> BTW, I checked the series with -Wswitch-default:
> -Wswitch-default
> Warn whenever a switch statement does not have a default case.
>> Furthermore, using BUG() is a pretty bad idea in switch.
> It is and not only in the switch. The reason I put BUG is that I tried
> to follow
> the existing "error handling" at those places.
 It is not because BUG() is been used today in some places that we need 
 to
 continue to spread it.

> Use of BUG() itself is another topic which will also need to be
> addressed
 So we should not add more of them...
>>> Again, I see this as a dedicated change. So, in the current series I 
>>> think
>>> it is
>>> acceptable to use the existing way of error handling if any at all.
>> That's not how it works in upstream. If you know some constructs are 
>> wrong, it
>> is best to try to address partially the problem directly then having so 
>> you
>> reduce the amounts of change afterwards.
>>
>> So please try to not introduce more BUG() in the code base.
> Hi Oleksandr, Julien,
>
> Julien's right that we should not introduce any more BUG()s. In fact,
> each of them makes the code less safe, not more safe! The purpose of
> MISRAC 16.4 is "defensive programming": write the code in a way that is
> more (not less!) resilient to failure.
>
> So, I think it is a good idea to introduce a default label because it
> can help us spot unexpected issues. Instead of calling BUG() in the
> default handler, which is detrimental, we should return an error when
> possible, or just print a warning.

 domain_crash() is almost always better than BUG().  It is very obvious
 if it gets hit, and wont crash Xen.
>>>
>>> That's a good suggestion.
>>>
>>>
> As 16.4 clearly state, even a simple comment would be enough to address
> the rule. We just need to explain why a default label is not needed.
> Such as:
>
> default:
> /* unreachable because blah and blah */

 What a simple comment doesn't do is avoid breaking -Wswitch.
>>>
>>> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
>>> -Wswitch could be a good way to address 16.4, but then we introduce a
>>> compiler specific requirement. Typically gcc is not the compiler of
>>> choice for these environments, unfortunately forcing gcc is not an
>>> option.
>>
>> Well, you could build with GCC and then build with your custom
>> compiler...
> 
> This suggestion is problematic: as an individual interested in MISRA-C
> compliance, I only have the MISRA-C rules in my hands. I don't know how
> to deal with suggestions like this one, that don't comply to the Rules,
> but it tries to address the same issue in a different manner.

Are you suggesting we will have to abide to all the rules even if they 
doesn't make things worst? I was under the impression we don't necessary 
need to follow a rule if we have justification for it.

> 
> I cannot rule out that it wouldn't work, but also I cannot be sure that
> it would work. In short, I have no way to make progress or to find out
> how to move forward. I guess as a contributor I would be forced to go
> back to the MISRAC compliance experts and ask for their opinion. (One
> non-technical issue is who is going to pay them for spending their time
> on this.) But what if they say it is not acceptable for compliance?
> 
> This is a great topic to discuss in March and decide what to do in these
> situations.
> 
> 
>> But, GCC is pretty much the only choice for Xen on Arm today
>> as we don't build with clang and I pretty doubt we can build with compcert.
> 
> Obviously, this has to change if we want to make progress on safety
> certifications.

I am curious to know what is plan for this. I mean that if no-one is 
planning to make Xen build with other compilers. Then what would be the 
benefits of this?

Cheers,

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

Re: [Xen-devel] [PATCH] x86/hvm: Intercept RDPMC when vPMU is disabled

[Andrew Cooper]

On 22/02/2019 21:58, Boris Ostrovsky wrote:
> On 2/22/19 4:13 PM, Andrew Cooper wrote:
>> vPMU isn't security supported, and in general guests can't access any of the
>> performance counter MSRs.  However, the RDPMC instruction isn't intercepted,
>> meaning that guest software can read the instantaneous counter values.
>>
>> When vPMU isn't configured, intercept RDPMC and unconditionally fail it as if
>> software has requested a bad counter index (#GP fault).  It is model specific
>> as to which counters are available to begin with, and in levelled scenarios,
>> this information may not be accurate in the first place.
>>
>> This change isn't expected to have any impact on VMs.  Userspace is not
>> usually given access to RDPMC (Windows appear to completely prohibit it; 
>> Linux
>> is restricted to root), and kernels won't be executing RDPMC instructions if
>> their PMU drivers have failed to start.
>>
>> Signed-off-by: Andrew Cooper 
>> ---
>> CC: Jan Beulich 
>> CC: Wei Liu 
>> CC: Roger Pau Monné 
>> CC: Jun Nakajima 
>> CC: Kevin Tian 
>> CC: Boris Ostrovsky 
>> CC: Suravee Suthikulpanit 
>> CC: Brian Woods 
>> CC: Juergen Gross 
>>
>> This should be taken into Xen 4.12 and backported to the stable releases.
>> While it isn't an XSA itself, it is an information leak (Xen's NMI watchdog 
>> in
>> particular) which could be advantagous to an attacker trying to exploit a 
>> race
>> condition.
>>
>> The only other option is to emulate the reported family and offer back all 
>> 0's
>> for the accessable counters.  Obviously this is a non-starter.
> When VPMU is off MSR reads return zero.

That behaviour isn't long for this world.

> While it is debatable whether this the right action, shouldn't rdpmc behave 
> in the same fashion?

I specifically don't want to propagate the "lets complete with zero"
behaviour further, because it takes away #GP faults which the guest
would otherwise get.

~Andrew

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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Stefano Stabellini]

On Fri, 22 Feb 2019, Andrew Cooper wrote:
> On 22/02/2019 22:11, Julien Grall wrote:
> > Hi Stefano,
> >
> > On 22/02/2019 21:58, Stefano Stabellini wrote:
> >> On Fri, 22 Feb 2019, Andrew Cooper wrote:
> >>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>  On Fri, 22 Feb 2019, Julien Grall wrote:
>  BTW, I checked the series with -Wswitch-default:
>  -Wswitch-default
>  Warn whenever a switch statement does not have a default case.
> > Furthermore, using BUG() is a pretty bad idea in switch.
>  It is and not only in the switch. The reason I put BUG is that I 
>  tried
>  to follow
>  the existing "error handling" at those places.
> >>> It is not because BUG() is been used today in some places that we 
> >>> need to
> >>> continue to spread it.
> >>>
>  Use of BUG() itself is another topic which will also need to be
>  addressed
> >>> So we should not add more of them...
> >> Again, I see this as a dedicated change. So, in the current series I 
> >> think
> >> it is
> >> acceptable to use the existing way of error handling if any at all.
> > That's not how it works in upstream. If you know some constructs are 
> > wrong, it
> > is best to try to address partially the problem directly then having so 
> > you
> > reduce the amounts of change afterwards.
> >
> > So please try to not introduce more BUG() in the code base.
>  Hi Oleksandr, Julien,
> 
>  Julien's right that we should not introduce any more BUG()s. In fact,
>  each of them makes the code less safe, not more safe! The purpose of
>  MISRAC 16.4 is "defensive programming": write the code in a way that is
>  more (not less!) resilient to failure.
> 
>  So, I think it is a good idea to introduce a default label because it
>  can help us spot unexpected issues. Instead of calling BUG() in the
>  default handler, which is detrimental, we should return an error when
>  possible, or just print a warning.
> >>> domain_crash() is almost always better than BUG().  It is very obvious
> >>> if it gets hit, and wont crash Xen.
> >> That's a good suggestion.
> >>
> >>
>  As 16.4 clearly state, even a simple comment would be enough to address
>  the rule. We just need to explain why a default label is not needed.
>  Such as:
> 
> default:
> /* unreachable because blah and blah */
> >>> What a simple comment doesn't do is avoid breaking -Wswitch.
> >> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
> >> -Wswitch could be a good way to address 16.4, but then we introduce a
> >> compiler specific requirement. Typically gcc is not the compiler of
> >> choice for these environments, unfortunately forcing gcc is not an
> >> option.
> > Well, you could build with GCC and then build with your custom 
> > compiler... But, GCC is pretty much the only choice for Xen on Arm today 
> > as we don't build with clang and I pretty doubt we can build with compcert.
> 
> So the suggestion I had was to have an overall CONFIG_MISRA which we can
> hide some of this nonsense behind, and then
> 
> #ifdef CONFIG_MISRA
> #define MISRA_BLE_DEFAULT default:
> #else
> #define MISRA_BLE_DEFAULT
> #endif
> 
> So when you disable CONFIG_MISRA, your compiler starts being able to
> help you again.

This is actually a good way to make progress which could make everybody
happy. (And it wouldn't require a slow back-and-forth with third parties
to ask difficult questions.)


> TBH, it would also be nice to hide the SYMBOL nonsense behind, so we can
> continue doing it the efficient way for ~100% of the time.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [qemu-mainline test] 133346: regressions - trouble: blocked/broken/fail/pass

[osstest service owner]

flight 133346 qemu-mainline real [real]
http://logs.test-lab.xenproject.org/osstest/logs/133346/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-xl-shadow   broken
 test-amd64-i386-xl-pvshimbroken
 test-amd64-i386-pair broken
 build-arm64-xsm  broken
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow broken
 build-armhf  broken
 build-armhf   2 hosts-allocate broken REGR. vs. 133284
 test-amd64-i386-libvirt-xsm  broken  in 133317
 test-amd64-amd64-pairbroken  in 133317
 test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm  broken in 133317
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsm 10 debian-hvm-install fail REGR. 
vs. 133284
 test-amd64-amd64-libvirt-vhd 10 debian-di-installfail REGR. vs. 133284
 test-amd64-i386-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail REGR. vs. 
133284
 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install 
fail REGR. vs. 133284
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 debian-hvm-install fail 
REGR. vs. 133284
 test-armhf-armhf-xl-arndale   6 xen-installfail in 133317 REGR. vs. 133284
 test-armhf-armhf-xl-vhd   10 debian-di-install fail in 133317 REGR. vs. 133284
 build-armhf-libvirt   1 build-check(1)   running

Tests which are failing intermittently (not blocking):
 test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm 4 host-install(4) broken in 
133317 pass in 133346
 test-amd64-amd64-pair 4 host-install/src_host(4) broken in 133317 pass in 
133346
 test-amd64-i386-libvirt-xsm  4 host-install(4) broken in 133317 pass in 133346
 test-amd64-i386-pair  4 host-install/src_host(4) broken pass in 133317
 test-amd64-i386-xl-pvshim 4 host-install(4)  broken pass in 133317
 test-amd64-amd64-xl-shadow4 host-install(4)  broken pass in 133317
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 4 host-install(4) broken pass 
in 133317
 test-amd64-i386-qemuu-rhel6hvm-intel 12 guest-start/redhat.repeat fail in 
133317 pass in 133346
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 10 debian-hvm-install fail in 
133317 pass in 133346
 test-amd64-amd64-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail in 133317 pass 
in 133346
 test-amd64-amd64-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail pass in 
133317
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 debian-hvm-install fail 
pass in 133317
 test-amd64-i386-xl-xsm7 xen-boot   fail pass in 133317
 test-amd64-amd64-xl-qcow210 debian-di-install  fail pass in 133317
 test-amd64-amd64-pygrub  10 debian-di-install  fail pass in 133317

Regressions which are regarded as allowable (not blocking):
 build-arm64-xsm   2 hosts-allocate broken REGR. vs. 133284

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-xl-rtds  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-raw  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-arndale   1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-credit2   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-vhd   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-credit1   1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-multivcpu  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-cubietruck  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-xsm   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl   1 build-check(1)   blocked  n/a
 build-armhf   3 capture-logs  broken blocked in 133284
 build-arm64-xsm   3 capture-logs  broken blocked in 133284
 test-armhf-armhf-libvirt 14 saverestore-support-check fail in 133317 like 
133284
 test-armhf-armhf-libvirt-raw 13 saverestore-support-check fail in 133317 like 
133284
 test-amd64-i386-xl-pvshim12 guest-start  fail in 133317 never pass
 test-arm64-arm64-libvirt-xsm 13 migrate-support-check fail in 133317 never pass
 test-arm64-arm64-libvirt-xsm 14 saverestore-support-check fail in 133317 never 
pass
 test-arm64-arm64-xl-xsm 13 migrate-support-check fail in 133317 never pass
 test-arm64-arm64-xl-xsm 14 saverestore-support-check fail in 133317 never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail in 133317 never pass
 test-armhf-armhf-xl 13 migrate-support-check fail in 133317 never pass
 test-armhf-armhf-xl 14 saverestore-support-check fail in 133317 never pass
 

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Andrew Cooper]

On 22/02/2019 22:11, Julien Grall wrote:
> Hi Stefano,
>
> On 22/02/2019 21:58, Stefano Stabellini wrote:
>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
 On Fri, 22 Feb 2019, Julien Grall wrote:
 BTW, I checked the series with -Wswitch-default:
 -Wswitch-default
 Warn whenever a switch statement does not have a default case.
> Furthermore, using BUG() is a pretty bad idea in switch.
 It is and not only in the switch. The reason I put BUG is that I tried
 to follow
 the existing "error handling" at those places.
>>> It is not because BUG() is been used today in some places that we need 
>>> to
>>> continue to spread it.
>>>
 Use of BUG() itself is another topic which will also need to be
 addressed
>>> So we should not add more of them...
>> Again, I see this as a dedicated change. So, in the current series I 
>> think
>> it is
>> acceptable to use the existing way of error handling if any at all.
> That's not how it works in upstream. If you know some constructs are 
> wrong, it
> is best to try to address partially the problem directly then having so 
> you
> reduce the amounts of change afterwards.
>
> So please try to not introduce more BUG() in the code base.
 Hi Oleksandr, Julien,

 Julien's right that we should not introduce any more BUG()s. In fact,
 each of them makes the code less safe, not more safe! The purpose of
 MISRAC 16.4 is "defensive programming": write the code in a way that is
 more (not less!) resilient to failure.

 So, I think it is a good idea to introduce a default label because it
 can help us spot unexpected issues. Instead of calling BUG() in the
 default handler, which is detrimental, we should return an error when
 possible, or just print a warning.
>>> domain_crash() is almost always better than BUG().  It is very obvious
>>> if it gets hit, and wont crash Xen.
>> That's a good suggestion.
>>
>>
 As 16.4 clearly state, even a simple comment would be enough to address
 the rule. We just need to explain why a default label is not needed.
 Such as:

default:
/* unreachable because blah and blah */
>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
>> -Wswitch could be a good way to address 16.4, but then we introduce a
>> compiler specific requirement. Typically gcc is not the compiler of
>> choice for these environments, unfortunately forcing gcc is not an
>> option.
> Well, you could build with GCC and then build with your custom 
> compiler... But, GCC is pretty much the only choice for Xen on Arm today 
> as we don't build with clang and I pretty doubt we can build with compcert.

So the suggestion I had was to have an overall CONFIG_MISRA which we can
hide some of this nonsense behind, and then

#ifdef CONFIG_MISRA
#define MISRA_BLE_DEFAULT default:
#else
#define MISRA_BLE_DEFAULT
#endif

So when you disable CONFIG_MISRA, your compiler starts being able to
help you again.

TBH, it would also be nice to hide the SYMBOL nonsense behind, so we can
continue doing it the efficient way for ~100% of the time.

~Andrew

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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Stefano Stabellini]

On Fri, 22 Feb 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 22/02/2019 21:58, Stefano Stabellini wrote:
> > On Fri, 22 Feb 2019, Andrew Cooper wrote:
> >> On 22/02/2019 21:00, Stefano Stabellini wrote:
> >>> On Fri, 22 Feb 2019, Julien Grall wrote:
> >>> BTW, I checked the series with -Wswitch-default:
> >>> -Wswitch-default
> >>> Warn whenever a switch statement does not have a default case.
>  Furthermore, using BUG() is a pretty bad idea in switch.
> >>> It is and not only in the switch. The reason I put BUG is that I tried
> >>> to follow
> >>> the existing "error handling" at those places.
> >> It is not because BUG() is been used today in some places that we need 
> >> to
> >> continue to spread it.
> >>
> >>> Use of BUG() itself is another topic which will also need to be
> >>> addressed
> >> So we should not add more of them...
> > Again, I see this as a dedicated change. So, in the current series I 
> > think
> > it is
> > acceptable to use the existing way of error handling if any at all.
>  That's not how it works in upstream. If you know some constructs are 
>  wrong, it
>  is best to try to address partially the problem directly then having so 
>  you
>  reduce the amounts of change afterwards.
> 
>  So please try to not introduce more BUG() in the code base.
> >>> Hi Oleksandr, Julien,
> >>>
> >>> Julien's right that we should not introduce any more BUG()s. In fact,
> >>> each of them makes the code less safe, not more safe! The purpose of
> >>> MISRAC 16.4 is "defensive programming": write the code in a way that is
> >>> more (not less!) resilient to failure.
> >>>
> >>> So, I think it is a good idea to introduce a default label because it
> >>> can help us spot unexpected issues. Instead of calling BUG() in the
> >>> default handler, which is detrimental, we should return an error when
> >>> possible, or just print a warning.
> >>
> >> domain_crash() is almost always better than BUG().  It is very obvious
> >> if it gets hit, and wont crash Xen.
> > 
> > That's a good suggestion.
> > 
> > 
> >>> As 16.4 clearly state, even a simple comment would be enough to address
> >>> the rule. We just need to explain why a default label is not needed.
> >>> Such as:
> >>>
> >>>default:
> >>>/* unreachable because blah and blah */
> >>
> >> What a simple comment doesn't do is avoid breaking -Wswitch.
> > 
> > I don't know how to reconcile 16.4 with -Wswitch. One could argue that
> > -Wswitch could be a good way to address 16.4, but then we introduce a
> > compiler specific requirement. Typically gcc is not the compiler of
> > choice for these environments, unfortunately forcing gcc is not an
> > option.
> 
> Well, you could build with GCC and then build with your custom 
> compiler... 

This suggestion is problematic: as an individual interested in MISRA-C
compliance, I only have the MISRA-C rules in my hands. I don't know how
to deal with suggestions like this one, that don't comply to the Rules,
but it tries to address the same issue in a different manner.

I cannot rule out that it wouldn't work, but also I cannot be sure that
it would work. In short, I have no way to make progress or to find out
how to move forward. I guess as a contributor I would be forced to go
back to the MISRAC compliance experts and ask for their opinion. (One
non-technical issue is who is going to pay them for spending their time
on this.) But what if they say it is not acceptable for compliance?

This is a great topic to discuss in March and decide what to do in these
situations.


> But, GCC is pretty much the only choice for Xen on Arm today 
> as we don't build with clang and I pretty doubt we can build with compcert.

Obviously, this has to change if we want to make progress on safety
certifications.___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi Stefano,

On 22/02/2019 21:58, Stefano Stabellini wrote:
> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>> BTW, I checked the series with -Wswitch-default:
>>> -Wswitch-default
>>> Warn whenever a switch statement does not have a default case.
 Furthermore, using BUG() is a pretty bad idea in switch.
>>> It is and not only in the switch. The reason I put BUG is that I tried
>>> to follow
>>> the existing "error handling" at those places.
>> It is not because BUG() is been used today in some places that we need to
>> continue to spread it.
>>
>>> Use of BUG() itself is another topic which will also need to be
>>> addressed
>> So we should not add more of them...
> Again, I see this as a dedicated change. So, in the current series I think
> it is
> acceptable to use the existing way of error handling if any at all.
 That's not how it works in upstream. If you know some constructs are 
 wrong, it
 is best to try to address partially the problem directly then having so you
 reduce the amounts of change afterwards.

 So please try to not introduce more BUG() in the code base.
>>> Hi Oleksandr, Julien,
>>>
>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>> each of them makes the code less safe, not more safe! The purpose of
>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>>> more (not less!) resilient to failure.
>>>
>>> So, I think it is a good idea to introduce a default label because it
>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>> default handler, which is detrimental, we should return an error when
>>> possible, or just print a warning.
>>
>> domain_crash() is almost always better than BUG().  It is very obvious
>> if it gets hit, and wont crash Xen.
> 
> That's a good suggestion.
> 
> 
>>> As 16.4 clearly state, even a simple comment would be enough to address
>>> the rule. We just need to explain why a default label is not needed.
>>> Such as:
>>>
>>>default:
>>>/* unreachable because blah and blah */
>>
>> What a simple comment doesn't do is avoid breaking -Wswitch.
> 
> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
> -Wswitch could be a good way to address 16.4, but then we introduce a
> compiler specific requirement. Typically gcc is not the compiler of
> choice for these environments, unfortunately forcing gcc is not an
> option.

Well, you could build with GCC and then build with your custom 
compiler... But, GCC is pretty much the only choice for Xen on Arm today 
as we don't build with clang and I pretty doubt we can build with compcert.

Cheers,

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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

On 22/02/2019 21:00, Stefano Stabellini wrote:
> On Fri, 22 Feb 2019, Julien Grall wrote:
> BTW, I checked the series with -Wswitch-default:
> -Wswitch-default
> Warn whenever a switch statement does not have a default case.
>> Furthermore, using BUG() is a pretty bad idea in switch.
> It is and not only in the switch. The reason I put BUG is that I tried
> to follow
> the existing "error handling" at those places.

 It is not because BUG() is been used today in some places that we need to
 continue to spread it.

> Use of BUG() itself is another topic which will also need to be
> addressed

 So we should not add more of them...
>>> Again, I see this as a dedicated change. So, in the current series I think
>>> it is
>>> acceptable to use the existing way of error handling if any at all.
>>
>> That's not how it works in upstream. If you know some constructs are wrong, 
>> it
>> is best to try to address partially the problem directly then having so you
>> reduce the amounts of change afterwards.
>>
>> So please try to not introduce more BUG() in the code base.
> 
> Hi Oleksandr, Julien,
> 
> Julien's right that we should not introduce any more BUG()s. In fact,
> each of them makes the code less safe, not more safe! The purpose of
> MISRAC 16.4 is "defensive programming": write the code in a way that is
> more (not less!) resilient to failure.
> 
> So, I think it is a good idea to introduce a default label because it
> can help us spot unexpected issues. Instead of calling BUG() in the
> default handler, which is detrimental, we should return an error when
> possible, or just print a warning.

I looked at the first patch and to be honest I can't see how this hence 
our code...

> 
> As 16.4 clearly state, even a simple comment would be enough to address
> the rule. We just need to explain why a default label is not needed.
> Such as:
> 
>default:
>/* unreachable because blah and blah */

... as we already have defensive code. Indeed, in most of the switch we 
deal with potential issue by initializing the variable before the 
switch. If you look at the first patch, a lot of "default: break;" is 
introduced. So what's our benefits? How this code is more defensive than 
what we currently have?

Furthermore, how this is going to help us (thanks to -Wswitch) if an 
enumerate is extended and we miss a case that the compiler don't notice 
anymore?

Cheers,

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

Re: [Xen-devel] [PATCH] x86/hvm: Intercept RDPMC when vPMU is disabled

[Boris Ostrovsky]

On 2/22/19 4:13 PM, Andrew Cooper wrote:
> vPMU isn't security supported, and in general guests can't access any of the
> performance counter MSRs.  However, the RDPMC instruction isn't intercepted,
> meaning that guest software can read the instantaneous counter values.
>
> When vPMU isn't configured, intercept RDPMC and unconditionally fail it as if
> software has requested a bad counter index (#GP fault).  It is model specific
> as to which counters are available to begin with, and in levelled scenarios,
> this information may not be accurate in the first place.
>
> This change isn't expected to have any impact on VMs.  Userspace is not
> usually given access to RDPMC (Windows appear to completely prohibit it; Linux
> is restricted to root), and kernels won't be executing RDPMC instructions if
> their PMU drivers have failed to start.
>
> Signed-off-by: Andrew Cooper 
> ---
> CC: Jan Beulich 
> CC: Wei Liu 
> CC: Roger Pau Monné 
> CC: Jun Nakajima 
> CC: Kevin Tian 
> CC: Boris Ostrovsky 
> CC: Suravee Suthikulpanit 
> CC: Brian Woods 
> CC: Juergen Gross 
>
> This should be taken into Xen 4.12 and backported to the stable releases.
> While it isn't an XSA itself, it is an information leak (Xen's NMI watchdog in
> particular) which could be advantagous to an attacker trying to exploit a race
> condition.
>
> The only other option is to emulate the reported family and offer back all 0's
> for the accessable counters.  Obviously this is a non-starter.


When VPMU is off MSR reads return zero. While it is debatable whether
this the right action, shouldn't rdpmc behave in the same fashion?

-boris



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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Stefano Stabellini]

On Fri, 22 Feb 2019, Andrew Cooper wrote:
> On 22/02/2019 21:00, Stefano Stabellini wrote:
> > On Fri, 22 Feb 2019, Julien Grall wrote:
> > BTW, I checked the series with -Wswitch-default:
> > -Wswitch-default
> > Warn whenever a switch statement does not have a default case.
> >> Furthermore, using BUG() is a pretty bad idea in switch. 
> > It is and not only in the switch. The reason I put BUG is that I tried
> > to follow
> > the existing "error handling" at those places.
>  It is not because BUG() is been used today in some places that we need to
>  continue to spread it.
> 
> > Use of BUG() itself is another topic which will also need to be
> > addressed
>  So we should not add more of them...
> >>> Again, I see this as a dedicated change. So, in the current series I think
> >>> it is
> >>> acceptable to use the existing way of error handling if any at all.
> >> That's not how it works in upstream. If you know some constructs are 
> >> wrong, it
> >> is best to try to address partially the problem directly then having so you
> >> reduce the amounts of change afterwards.
> >>
> >> So please try to not introduce more BUG() in the code base.
> > Hi Oleksandr, Julien,
> >
> > Julien's right that we should not introduce any more BUG()s. In fact,
> > each of them makes the code less safe, not more safe! The purpose of
> > MISRAC 16.4 is "defensive programming": write the code in a way that is
> > more (not less!) resilient to failure.
> >
> > So, I think it is a good idea to introduce a default label because it
> > can help us spot unexpected issues. Instead of calling BUG() in the
> > default handler, which is detrimental, we should return an error when
> > possible, or just print a warning.
> 
> domain_crash() is almost always better than BUG().  It is very obvious
> if it gets hit, and wont crash Xen.

That's a good suggestion.


> > As 16.4 clearly state, even a simple comment would be enough to address
> > the rule. We just need to explain why a default label is not needed.
> > Such as:
> >
> >   default:
> >   /* unreachable because blah and blah */
> 
> What a simple comment doesn't do is avoid breaking -Wswitch.

I don't know how to reconcile 16.4 with -Wswitch. One could argue that
-Wswitch could be a good way to address 16.4, but then we introduce a
compiler specific requirement. Typically gcc is not the compiler of
choice for these environments, unfortunately forcing gcc is not an
option.

But if there was a non-gcc way to do -Wswitch, then yes, that would be
the way to go.


> This requirement is actively hostile towards compilers trying to help
> you spot when you made a mistake and forgot to update one of the $N
> places you needed to.
> 
> In this case, I don't think "Because MISRA demand it" is a good enough
> justification to offset the increased error-prone-ness of the result.
> 
> ~Andrew
> 
> P.S. There is a solution here which could work, but IMO a better use of
> time and energy would be to get MISRA to update their rules to match
> this century, and stop getting in the way of compiler features intended
> to help the programmer avoid bugs.___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [xen-unstable-smoke test] 133375: trouble: broken/pass

[osstest service owner]

flight 133375 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/133375/

Failures and problems with tests :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-libvirt broken

Tests which are failing intermittently (not blocking):
 test-amd64-amd64-libvirt  4 host-install(4)  broken pass in 133371
 test-amd64-amd64-xl-qemuu-debianhvm-i386 10 debian-hvm-install fail in 133371 
pass in 133375

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt13 migrate-support-check fail in 133371 never pass
 test-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  e72ecc7615410e5bf1a1c9a4c7772322c16eeb82
baseline version:
 xen  db2af23d15077605f286d8ef86c8f5d9c1b8302a

Last test of basis   133343  2019-02-21 03:00:49 Z1 days
Testing same since   133371  2019-02-22 15:00:34 Z0 days2 attempts


People who touched revisions under test:
  Andrew Cooper 
  Jan Beulich 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-i386 pass
 test-amd64-amd64-libvirt broken  



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

broken-job test-amd64-amd64-libvirt broken
broken-step test-amd64-amd64-libvirt host-install(4)

Not pushing.


commit e72ecc7615410e5bf1a1c9a4c7772322c16eeb82
Author: Andrew Cooper 
Date:   Thu Jan 17 12:26:17 2019 +

x86/altp2m: Rework #VE enable/disable paths

Split altp2m_vcpu_{enable,disable}_ve() out of the
HVMOP_altp2m_vcpu_{enable,disable}_notify marshalling logic.  A future 
change
is going to need to call altp2m_vcpu_disable_ve() from the domain_kill() 
path.

While at it, clean up the logic in altp2m_vcpu_{initialise,destroy}().
altp2m_vcpu_reset() has no external callers, so fold it into its two
callsites.  This in turn allows for altp2m_vcpu_destroy() to reuse
altp2m_vcpu_disable_ve() rather than opencoding it.

No practical change.

Signed-off-by: Andrew Cooper 
Reviewed-by: Razvan Cojocaru 
Acked-by: Jan Beulich 
Release-acked-by: Juergen Gross 

commit 0dfffe01d5681ede6a50c6b57131320d9f4a3361
Author: Andrew Cooper 
Date:   Wed Feb 20 13:39:20 2019 +

x86: Improve the efficiency of domain_relinquish_resources()

pci_release_devices() takes the global PCI lock.  Once pci_release_devices()
has completed, it will be called redundantly each time paging_teardown() and
vcpu_destroy_pagetables() continue.

This is liable to be millions of times for a reasonably sized guest, and is 
a
serialising bottleneck now that domain_kill() can be run concurrently on
different domains.

Instead of propagating the opencoding of the relinquish state machine, take
the opportunity to clean it up.

Leave a proper set of comments explaining that domain_relinquish_resources()
implements a co-routine.  Introduce a documented PROGRESS() macro to avoid
latent bugs such as the RELMEM_xen case, and make the new PROG_* states
private to domain_relinquish_resources().

Signed-off-by: Andrew Cooper 
Reviewed-by: Wei Liu 
Acked-by: Jan Beulich 
Release-acked-by: Juergen Gross 
(qemu changes not included)

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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Andrew Cooper]

On 22/02/2019 21:00, Stefano Stabellini wrote:
> On Fri, 22 Feb 2019, Julien Grall wrote:
> BTW, I checked the series with -Wswitch-default:
> -Wswitch-default
> Warn whenever a switch statement does not have a default case.
>> Furthermore, using BUG() is a pretty bad idea in switch. 
> It is and not only in the switch. The reason I put BUG is that I tried
> to follow
> the existing "error handling" at those places.
 It is not because BUG() is been used today in some places that we need to
 continue to spread it.

> Use of BUG() itself is another topic which will also need to be
> addressed
 So we should not add more of them...
>>> Again, I see this as a dedicated change. So, in the current series I think
>>> it is
>>> acceptable to use the existing way of error handling if any at all.
>> That's not how it works in upstream. If you know some constructs are wrong, 
>> it
>> is best to try to address partially the problem directly then having so you
>> reduce the amounts of change afterwards.
>>
>> So please try to not introduce more BUG() in the code base.
> Hi Oleksandr, Julien,
>
> Julien's right that we should not introduce any more BUG()s. In fact,
> each of them makes the code less safe, not more safe! The purpose of
> MISRAC 16.4 is "defensive programming": write the code in a way that is
> more (not less!) resilient to failure.
>
> So, I think it is a good idea to introduce a default label because it
> can help us spot unexpected issues. Instead of calling BUG() in the
> default handler, which is detrimental, we should return an error when
> possible, or just print a warning.

domain_crash() is almost always better than BUG().  It is very obvious
if it gets hit, and wont crash Xen.

>
> As 16.4 clearly state, even a simple comment would be enough to address
> the rule. We just need to explain why a default label is not needed.
> Such as:
>
>   default:
>   /* unreachable because blah and blah */

What a simple comment doesn't do is avoid breaking -Wswitch.

This requirement is actively hostile towards compilers trying to help
you spot when you made a mistake and forgot to update one of the $N
places you needed to.

In this case, I don't think "Because MISRA demand it" is a good enough
justification to offset the increased error-prone-ness of the result.

~Andrew

P.S. There is a solution here which could work, but IMO a better use of
time and energy would be to get MISRA to update their rules to match
this century, and stop getting in the way of compiler features intended
to help the programmer avoid bugs.

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

[Xen-devel] [PATCH] x86/hvm: Intercept RDPMC when vPMU is disabled

[Andrew Cooper]

vPMU isn't security supported, and in general guests can't access any of the
performance counter MSRs.  However, the RDPMC instruction isn't intercepted,
meaning that guest software can read the instantaneous counter values.

When vPMU isn't configured, intercept RDPMC and unconditionally fail it as if
software has requested a bad counter index (#GP fault).  It is model specific
as to which counters are available to begin with, and in levelled scenarios,
this information may not be accurate in the first place.

This change isn't expected to have any impact on VMs.  Userspace is not
usually given access to RDPMC (Windows appear to completely prohibit it; Linux
is restricted to root), and kernels won't be executing RDPMC instructions if
their PMU drivers have failed to start.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Wei Liu 
CC: Roger Pau Monné 
CC: Jun Nakajima 
CC: Kevin Tian 
CC: Boris Ostrovsky 
CC: Suravee Suthikulpanit 
CC: Brian Woods 
CC: Juergen Gross 

This should be taken into Xen 4.12 and backported to the stable releases.
While it isn't an XSA itself, it is an information leak (Xen's NMI watchdog in
particular) which could be advantagous to an attacker trying to exploit a race
condition.

The only other option is to emulate the reported family and offer back all 0's
for the accessable counters.  Obviously this is a non-starter.

This patch has had some basic testing in XenServer with no issues identified
with any guests.
---
 xen/arch/x86/hvm/svm/svm.c  | 8 
 xen/arch/x86/hvm/svm/vmcb.c | 3 +++
 xen/arch/x86/hvm/vmx/vmcs.c | 2 ++
 xen/arch/x86/hvm/vmx/vmx.c  | 9 +
 4 files changed, 22 insertions(+)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 2584b90..de20bb2 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2984,6 +2984,14 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
 svm_vmexit_do_rdtsc(regs, exit_reason == VMEXIT_RDTSCP);
 break;
 
+case VMEXIT_RDPMC:
+/*
+ * Interception is only active when VPMU is disabled, and the guest
+ * mustn't be able to read the counters.  Emulate "bad counter index".
+ */
+hvm_inject_hw_exception(TRAP_gp_fault, 0);
+break;
+
 case VMEXIT_MONITOR:
 case VMEXIT_MWAIT:
 hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 9d1c5bf..092c049 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -67,6 +67,9 @@ static int construct_vmcb(struct vcpu *v)
 GENERAL1_INTERCEPT_INVLPGA | GENERAL1_INTERCEPT_IOIO_PROT   |
 GENERAL1_INTERCEPT_MSR_PROT| GENERAL1_INTERCEPT_SHUTDOWN_EVT|
 GENERAL1_INTERCEPT_TASK_SWITCH;
+if ( !vpmu_available(v) )
+vmcb->_general1_intercepts |= GENERAL1_INTERCEPT_RDPMC;
+
 vmcb->_general2_intercepts = 
 GENERAL2_INTERCEPT_VMRUN   | GENERAL2_INTERCEPT_VMMCALL |
 GENERAL2_INTERCEPT_VMLOAD  | GENERAL2_INTERCEPT_VMSAVE  |
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 74f2a08..e860db9 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -994,6 +994,8 @@ static int construct_vmcs(struct vcpu *v)
 __vmwrite(PIN_BASED_VM_EXEC_CONTROL, vmx_pin_based_exec_control);
 
 v->arch.hvm.vmx.exec_control = vmx_cpu_based_exec_control;
+if ( !vpmu_available(v) )
+v->arch.hvm.vmx.exec_control |= CPU_BASED_RDPMC_EXITING;
 if ( d->arch.vtsc && !cpu_has_vmx_tsc_scaling )
 v->arch.hvm.vmx.exec_control |= CPU_BASED_RDTSC_EXITING;
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 24def93..c5e5804 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3956,6 +3956,15 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
 __vmread(EXIT_QUALIFICATION, _qualification);
 vmx_invlpg_intercept(exit_qualification);
 break;
+
+case EXIT_REASON_RDPMC:
+/*
+ * Interception is only active when VPMU is disabled, and the guest
+ * mustn't be able to read the counters.  Emulate "bad counter index".
+ */
+hvm_inject_hw_exception(TRAP_gp_fault, 0);
+break;
+
 case EXIT_REASON_RDTSCP:
 if ( !currd->arch.cpuid->extd.rdtscp )
 {
-- 
2.1.4


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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Stefano Stabellini]

On Fri, 22 Feb 2019, Julien Grall wrote:
> > > > BTW, I checked the series with -Wswitch-default:
> > > > -Wswitch-default
> > > > Warn whenever a switch statement does not have a default case.
> > > > > Furthermore, using BUG() is a pretty bad idea in switch. 
> > > > It is and not only in the switch. The reason I put BUG is that I tried
> > > > to follow
> > > > the existing "error handling" at those places.
> > > 
> > > It is not because BUG() is been used today in some places that we need to
> > > continue to spread it.
> > > 
> > > > Use of BUG() itself is another topic which will also need to be
> > > > addressed
> > > 
> > > So we should not add more of them...
> > Again, I see this as a dedicated change. So, in the current series I think
> > it is
> > acceptable to use the existing way of error handling if any at all.
> 
> That's not how it works in upstream. If you know some constructs are wrong, it
> is best to try to address partially the problem directly then having so you
> reduce the amounts of change afterwards.
> 
> So please try to not introduce more BUG() in the code base.

Hi Oleksandr, Julien,

Julien's right that we should not introduce any more BUG()s. In fact,
each of them makes the code less safe, not more safe! The purpose of
MISRAC 16.4 is "defensive programming": write the code in a way that is
more (not less!) resilient to failure.

So, I think it is a good idea to introduce a default label because it
can help us spot unexpected issues. Instead of calling BUG() in the
default handler, which is detrimental, we should return an error when
possible, or just print a warning.

As 16.4 clearly state, even a simple comment would be enough to address
the rule. We just need to explain why a default label is not needed.
Such as:

  default:
  /* unreachable because blah and blah */


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

Re: [Xen-devel] XEN on R-CAR H3

[Amit Tomer]

Hello,

> Did removing reserved-memory regions together with users work out well
> for you?

Sorry, didn't get chance to work on this today. I would test it and
let you know.

Thanks
-Amit

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

[Xen-devel] [PATCH 5/6] x86/vtd: Drop struct iommu_flush

[Andrew Cooper]

It is unclear why this abstraction exists, but iommu_get_flush() returns
possibly NULL and every user unconditionally dereferences the result.  In
practice, I can't spot a path where iommu is NULL, so I think it is mostly
dead.

Move the two function pointers into struct vtd_iommu (using a flush_ prefix),
and delete iommu_get_flush().  Furthermore, there is no need to pass the IOMMU
pointer to the callbacks via a void pointer, so change the parameter to be
correctly typed as struct vtd_iommu.  Clean up bool_t to bool in surrounding
context.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Paul Durrant 
CC: Kevin Tian 
---
 xen/drivers/passthrough/vtd/iommu.c  | 62 
 xen/drivers/passthrough/vtd/iommu.h  | 24 +-
 xen/drivers/passthrough/vtd/qinval.c | 21 +---
 3 files changed, 44 insertions(+), 63 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c 
b/xen/drivers/passthrough/vtd/iommu.c
index 05dc7ff..7fc6fe0 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -334,11 +334,11 @@ static void iommu_flush_write_buffer(struct vtd_iommu 
*iommu)
 }
 
 /* return value determine if we need a write buffer flush */
-static int __must_check flush_context_reg(void *_iommu, u16 did, u16 source_id,
-  u8 function_mask, u64 type,
-  bool_t flush_non_present_entry)
+static int __must_check flush_context_reg(struct vtd_iommu *iommu, u16 did,
+  u16 source_id, u8 function_mask,
+  u64 type,
+  bool flush_non_present_entry)
 {
-struct vtd_iommu *iommu = _iommu;
 u64 val = 0;
 unsigned long flags;
 
@@ -387,31 +387,28 @@ static int __must_check flush_context_reg(void *_iommu, 
u16 did, u16 source_id,
 }
 
 static int __must_check iommu_flush_context_global(struct vtd_iommu *iommu,
-   bool_t 
flush_non_present_entry)
+   bool 
flush_non_present_entry)
 {
-struct iommu_flush *flush = iommu_get_flush(iommu);
-return flush->context(iommu, 0, 0, 0, DMA_CCMD_GLOBAL_INVL,
- flush_non_present_entry);
+return iommu->flush_context(iommu, 0, 0, 0, DMA_CCMD_GLOBAL_INVL,
+flush_non_present_entry);
 }
 
 static int __must_check iommu_flush_context_device(struct vtd_iommu *iommu,
u16 did, u16 source_id,
u8 function_mask,
-   bool_t 
flush_non_present_entry)
+   bool 
flush_non_present_entry)
 {
-struct iommu_flush *flush = iommu_get_flush(iommu);
-return flush->context(iommu, did, source_id, function_mask,
- DMA_CCMD_DEVICE_INVL,
- flush_non_present_entry);
+return iommu->flush_context(iommu, did, source_id, function_mask,
+DMA_CCMD_DEVICE_INVL, flush_non_present_entry);
 }
 
 /* return value determine if we need a write buffer flush */
-static int __must_check flush_iotlb_reg(void *_iommu, u16 did, u64 addr,
+static int __must_check flush_iotlb_reg(struct vtd_iommu *iommu, u16 did,
+u64 addr,
 unsigned int size_order, u64 type,
-bool_t flush_non_present_entry,
-bool_t flush_dev_iotlb)
+bool flush_non_present_entry,
+bool flush_dev_iotlb)
 {
-struct vtd_iommu *iommu = _iommu;
 int tlb_offset = ecap_iotlb_offset(iommu->ecap);
 u64 val = 0;
 unsigned long flags;
@@ -474,17 +471,16 @@ static int __must_check flush_iotlb_reg(void *_iommu, u16 
did, u64 addr,
 }
 
 static int __must_check iommu_flush_iotlb_global(struct vtd_iommu *iommu,
- bool_t 
flush_non_present_entry,
- bool_t flush_dev_iotlb)
+ bool flush_non_present_entry,
+ bool flush_dev_iotlb)
 {
-struct iommu_flush *flush = iommu_get_flush(iommu);
 int status;
 
 /* apply platform specific errata workarounds */
 vtd_ops_preamble_quirk(iommu);
 
-status = flush->iotlb(iommu, 0, 0, 0, DMA_TLB_GLOBAL_FLUSH,
-flush_non_present_entry, flush_dev_iotlb);
+status = iommu->flush_iotlb(iommu, 0, 0, 0, DMA_TLB_GLOBAL_FLUSH,
+flush_non_present_entry, flush_dev_iotlb);
 
 /* undo 

Re: [Xen-devel] XEN on R-CAR H3

[Oleksandr]

Hi Amit


Likely, it is because you left device nodes (mmngr,adsp,etc) which had 
links to reserved-memory regions ...




Did removing reserved-memory regions together with users work out well 
for you?



--
Regards,

Oleksandr Tyshchenko


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

[Xen-devel] [PATCH 3/6] x86/vtd: Drop struct qi_ctrl

[Andrew Cooper]

It is unclear why this abstraction exists, but iommu_qi_ctrl() returns
possibly NULL and every user unconditionally dereferences the result.  In
practice, I can't spot a path where iommu is NULL, so I think it is mostly
dead.

Move the sole member into struct vtd_iommu, and delete iommu_qi_ctrl().

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Paul Durrant 
CC: Kevin Tian 
---
 xen/drivers/passthrough/vtd/iommu.h  | 13 +++
 xen/drivers/passthrough/vtd/qinval.c | 42 +++-
 2 files changed, 20 insertions(+), 35 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.h 
b/xen/drivers/passthrough/vtd/iommu.h
index 556b3d6..12b531c 100644
--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -506,10 +506,6 @@ extern struct list_head acpi_drhd_units;
 extern struct list_head acpi_rmrr_units;
 extern struct list_head acpi_ioapic_units;
 
-struct qi_ctrl {
-u64 qinval_maddr;  /* queue invalidation page machine address */
-};
-
 struct ir_ctrl {
 u64 iremap_maddr;/* interrupt remap table machine address */
 int iremap_num;  /* total num of used interrupt remap entry */
@@ -527,7 +523,6 @@ struct iommu_flush {
 };
 
 struct intel_iommu {
-struct qi_ctrl qi_ctrl;
 struct ir_ctrl ir_ctrl;
 struct iommu_flush flush;
 struct acpi_drhd_unit *drhd;
@@ -545,16 +540,14 @@ struct vtd_iommu {
 u64 root_maddr; /* root entry machine address */
 struct msi_desc msi;
 struct intel_iommu *intel;
+
+uint64_t qinval_maddr;   /* queue invalidation page machine address */
+
 struct list_head ats_devices;
 unsigned long *domid_bitmap;  /* domain id bitmap */
 u16 *domid_map;   /* domain id mapping array */
 };
 
-static inline struct qi_ctrl *iommu_qi_ctrl(struct vtd_iommu *iommu)
-{
-return iommu ? >intel->qi_ctrl : NULL;
-}
-
 static inline struct ir_ctrl *iommu_ir_ctrl(struct vtd_iommu *iommu)
 {
 return iommu ? >intel->ir_ctrl : NULL;
diff --git a/xen/drivers/passthrough/vtd/qinval.c 
b/xen/drivers/passthrough/vtd/qinval.c
index 3ddb9b6..f6fcee5 100644
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -84,7 +84,7 @@ static int __must_check queue_invalidate_context_sync(struct 
vtd_iommu *iommu,
 
 spin_lock_irqsave(>register_lock, flags);
 index = qinval_next_index(iommu);
-entry_base = iommu_qi_ctrl(iommu)->qinval_maddr +
+entry_base = iommu->qinval_maddr +
  ((index >> QINVAL_ENTRY_ORDER) << PAGE_SHIFT);
 qinval_entries = map_vtd_domain_page(entry_base);
 qinval_entry = _entries[index % (1 << QINVAL_ENTRY_ORDER)];
@@ -118,7 +118,7 @@ static int __must_check queue_invalidate_iotlb_sync(struct 
vtd_iommu *iommu,
 
 spin_lock_irqsave(>register_lock, flags);
 index = qinval_next_index(iommu);
-entry_base = iommu_qi_ctrl(iommu)->qinval_maddr +
+entry_base = iommu->qinval_maddr +
  ((index >> QINVAL_ENTRY_ORDER) << PAGE_SHIFT);
 qinval_entries = map_vtd_domain_page(entry_base);
 qinval_entry = _entries[index % (1 << QINVAL_ENTRY_ORDER)];
@@ -155,7 +155,7 @@ static int __must_check queue_invalidate_wait(struct 
vtd_iommu *iommu,
 
 spin_lock_irqsave(>register_lock, flags);
 index = qinval_next_index(iommu);
-entry_base = iommu_qi_ctrl(iommu)->qinval_maddr +
+entry_base = iommu->qinval_maddr +
  ((index >> QINVAL_ENTRY_ORDER) << PAGE_SHIFT);
 qinval_entries = map_vtd_domain_page(entry_base);
 qinval_entry = _entries[index % (1 << QINVAL_ENTRY_ORDER)];
@@ -201,9 +201,7 @@ static int __must_check queue_invalidate_wait(struct 
vtd_iommu *iommu,
 
 static int __must_check invalidate_sync(struct vtd_iommu *iommu)
 {
-struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
-
-ASSERT(qi_ctrl->qinval_maddr);
+ASSERT(iommu->qinval_maddr);
 
 return queue_invalidate_wait(iommu, 0, 1, 1, 0);
 }
@@ -211,10 +209,9 @@ static int __must_check invalidate_sync(struct vtd_iommu 
*iommu)
 static int __must_check dev_invalidate_sync(struct vtd_iommu *iommu,
 struct pci_dev *pdev, u16 did)
 {
-struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
 int rc;
 
-ASSERT(qi_ctrl->qinval_maddr);
+ASSERT(iommu->qinval_maddr);
 rc = queue_invalidate_wait(iommu, 0, 1, 1, 1);
 if ( rc == -ETIMEDOUT )
 {
@@ -248,7 +245,7 @@ int qinval_device_iotlb_sync(struct vtd_iommu *iommu, 
struct pci_dev *pdev,
 ASSERT(pdev);
 spin_lock_irqsave(>register_lock, flags);
 index = qinval_next_index(iommu);
-entry_base = iommu_qi_ctrl(iommu)->qinval_maddr +
+entry_base = iommu->qinval_maddr +
  ((index >> QINVAL_ENTRY_ORDER) << PAGE_SHIFT);
 qinval_entries = map_vtd_domain_page(entry_base);
 qinval_entry = _entries[index % (1 << QINVAL_ENTRY_ORDER)];
@@ -282,7 +279,7 @@ static int __must_check queue_invalidate_iec_sync(struct 

[Xen-devel] [PATCH 6/6] x86/vtd: Drop struct intel_iommu

[Andrew Cooper]

The sole remaining member of struct intel_iommu is the drhd backpointer.  Move
this into struct vtd_iommu, replacing the the 'intel' pointer.

This removes one dynamic memory allocation per IOMMU on the system.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Paul Durrant 
CC: Kevin Tian 
---
 xen/drivers/passthrough/vtd/iommu.c  | 33 +
 xen/drivers/passthrough/vtd/iommu.h  |  6 +-
 xen/drivers/passthrough/vtd/quirks.c |  9 +++--
 xen/drivers/passthrough/vtd/utils.c  |  2 +-
 4 files changed, 10 insertions(+), 40 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c 
b/xen/drivers/passthrough/vtd/iommu.c
index 7fc6fe0..01e2574 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -139,22 +139,6 @@ static int context_get_domain_id(struct context_entry 
*context,
 return domid;
 }
 
-static struct intel_iommu *__init alloc_intel_iommu(void)
-{
-struct intel_iommu *intel;
-
-intel = xzalloc(struct intel_iommu);
-if ( intel == NULL )
-return NULL;
-
-return intel;
-}
-
-static void __init free_intel_iommu(struct intel_iommu *intel)
-{
-xfree(intel);
-}
-
 static int iommus_incoherent;
 static void __iommu_flush_cache(void *addr, unsigned int size)
 {
@@ -869,7 +853,7 @@ static int iommu_page_fault_do_one(struct vtd_iommu *iommu, 
int type,
 {
 const char *reason, *kind;
 enum faulttype fault_type;
-u16 seg = iommu->intel->drhd->segment;
+u16 seg = iommu->drhd->segment;
 
 reason = iommu_get_fault_reason(fault_reason, _type);
 switch ( fault_type )
@@ -982,7 +966,7 @@ static void __do_iommu_page_fault(struct vtd_iommu *iommu)
 iommu_page_fault_do_one(iommu, type, fault_reason,
 source_id, guest_addr);
 
-pci_check_disable_device(iommu->intel->drhd->segment,
+pci_check_disable_device(iommu->drhd->segment,
  PCI_BUS(source_id), PCI_DEVFN2(source_id));
 
 fault_index++;
@@ -1180,13 +1164,7 @@ int __init iommu_alloc(struct acpi_drhd_unit *drhd)
 INIT_LIST_HEAD(>ats_devices);
 spin_lock_init(>iremap_lock);
 
-iommu->intel = alloc_intel_iommu();
-if ( iommu->intel == NULL )
-{
-xfree(iommu);
-return -ENOMEM;
-}
-iommu->intel->drhd = drhd;
+iommu->drhd = drhd;
 drhd->iommu = iommu;
 
 if ( !(iommu->root_maddr = alloc_pgtable_maddr(drhd, 1)) )
@@ -1279,7 +1257,6 @@ void __init iommu_free(struct acpi_drhd_unit *drhd)
 xfree(iommu->domid_bitmap);
 xfree(iommu->domid_map);
 
-free_intel_iommu(iommu->intel);
 if ( iommu->msi.irq >= 0 )
 destroy_irq(iommu->msi.irq);
 xfree(iommu);
@@ -1329,7 +1306,7 @@ int domain_context_mapping_one(
 struct domain_iommu *hd = dom_iommu(domain);
 struct context_entry *context, *context_entries;
 u64 maddr, pgd_maddr;
-u16 seg = iommu->intel->drhd->segment;
+u16 seg = iommu->drhd->segment;
 int agaw, rc, ret;
 bool_t flush_dev_iotlb;
 
@@ -1618,7 +1595,7 @@ int domain_context_unmap_one(
 spin_unlock(>lock);
 unmap_vtd_domain_page(context_entries);
 
-if ( !iommu->intel->drhd->segment && !rc )
+if ( !iommu->drhd->segment && !rc )
 rc = me_wifi_quirk(domain, bus, devfn, UNMAP_ME_PHANTOM_FUNC);
 
 return rc;
diff --git a/xen/drivers/passthrough/vtd/iommu.h 
b/xen/drivers/passthrough/vtd/iommu.h
index a8cffba..808dfcd 100644
--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -506,10 +506,6 @@ extern struct list_head acpi_drhd_units;
 extern struct list_head acpi_rmrr_units;
 extern struct list_head acpi_ioapic_units;
 
-struct intel_iommu {
-struct acpi_drhd_unit *drhd;
-};
-
 struct vtd_iommu {
 struct list_head list;
 void __iomem *reg; /* Pointer to hardware regs, virtual addr */
@@ -521,7 +517,7 @@ struct vtd_iommu {
 spinlock_t register_lock; /* protect iommu register handling */
 u64 root_maddr; /* root entry machine address */
 struct msi_desc msi;
-struct intel_iommu *intel;
+struct acpi_drhd_unit *drhd;
 
 uint64_t qinval_maddr;   /* queue invalidation page machine address */
 
diff --git a/xen/drivers/passthrough/vtd/quirks.c 
b/xen/drivers/passthrough/vtd/quirks.c
index 79209f3..f3a617f 100644
--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -139,8 +139,7 @@ static void __init map_igd_reg(void)
  */
 static int cantiga_vtd_ops_preamble(struct vtd_iommu *iommu)
 {
-struct intel_iommu *intel = iommu->intel;
-struct acpi_drhd_unit *drhd = intel ? intel->drhd : NULL;
+struct acpi_drhd_unit *drhd = iommu->drhd;
 
 if ( !is_igd_drhd(drhd) || !is_cantiga_b3 )
 return 0;
@@ -174,8 +173,7 @@ static int cantiga_vtd_ops_preamble(struct vtd_iommu *iommu)
  */
 static void snb_vtd_ops_preamble(struct vtd_iommu *iommu)
 {
-struct intel_iommu *intel = iommu->intel;
-struct 

[Xen-devel] [PATCH 2/6] x86/vtd: Rename struct iommu to vtd_iommu

[Andrew Cooper]

VT-d's local struct iommu is an overly-generic name, for a structure which in
practice maps 1-to-1 with the real IOMMUs in the system.

Additionally, address style issues on impacted lines.  This is mostly
positioning of * for pointers and unnecessay casts with void pointers.

No functional change.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Paul Durrant 
CC: Jun Nakajima 
CC: Kevin Tian 
---
 xen/drivers/passthrough/vtd/dmar.c |  6 +--
 xen/drivers/passthrough/vtd/dmar.h |  4 +-
 xen/drivers/passthrough/vtd/extern.h   | 36 -
 xen/drivers/passthrough/vtd/intremap.c | 26 ++--
 xen/drivers/passthrough/vtd/iommu.c| 74 +-
 xen/drivers/passthrough/vtd/iommu.h|  8 ++--
 xen/drivers/passthrough/vtd/qinval.c   | 34 
 xen/drivers/passthrough/vtd/quirks.c   | 10 ++---
 xen/drivers/passthrough/vtd/utils.c|  8 ++--
 xen/drivers/passthrough/vtd/x86/ats.c  |  6 +--
 10 files changed, 106 insertions(+), 106 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/dmar.c 
b/xen/drivers/passthrough/vtd/dmar.c
index 81afa54..ce1b8ce 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -137,7 +137,7 @@ struct acpi_drhd_unit * ioapic_to_drhd(unsigned int apic_id)
 return NULL;
 }
 
-struct acpi_drhd_unit * iommu_to_drhd(struct iommu *iommu)
+struct acpi_drhd_unit *iommu_to_drhd(struct vtd_iommu *iommu)
 {
 struct acpi_drhd_unit *drhd;
 
@@ -151,7 +151,7 @@ struct acpi_drhd_unit * iommu_to_drhd(struct iommu *iommu)
 return NULL;
 }
 
-struct iommu * ioapic_to_iommu(unsigned int apic_id)
+struct vtd_iommu *ioapic_to_iommu(unsigned int apic_id)
 {
 struct acpi_drhd_unit *drhd;
 
@@ -182,7 +182,7 @@ struct acpi_drhd_unit *hpet_to_drhd(unsigned int hpet_id)
 return NULL;
 }
 
-struct iommu *hpet_to_iommu(unsigned int hpet_id)
+struct vtd_iommu *hpet_to_iommu(unsigned int hpet_id)
 {
 struct acpi_drhd_unit *drhd = hpet_to_drhd(hpet_id);
 
diff --git a/xen/drivers/passthrough/vtd/dmar.h 
b/xen/drivers/passthrough/vtd/dmar.h
index 95bb132..1a9c965 100644
--- a/xen/drivers/passthrough/vtd/dmar.h
+++ b/xen/drivers/passthrough/vtd/dmar.h
@@ -63,7 +63,7 @@ struct acpi_drhd_unit {
 u64address; /* register base address of the unit */
 u16segment;
 u8 include_all:1;
-struct iommu *iommu;
+struct vtd_iommu *iommu;
 struct list_head ioapic_list;
 struct list_head hpet_list;
 };
@@ -128,7 +128,7 @@ do {\
 } while (0)
 
 int vtd_hw_check(void);
-void disable_pmr(struct iommu *iommu);
+void disable_pmr(struct vtd_iommu *iommu);
 int is_igd_drhd(struct acpi_drhd_unit *drhd);
 
 #endif /* _DMAR_H_ */
diff --git a/xen/drivers/passthrough/vtd/extern.h 
b/xen/drivers/passthrough/vtd/extern.h
index 16eada9..cfef9a3 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -30,38 +30,38 @@ extern bool_t rwbf_quirk;
 extern const struct iommu_ops intel_iommu_ops;
 
 void print_iommu_regs(struct acpi_drhd_unit *drhd);
-void print_vtd_entries(struct iommu *iommu, int bus, int devfn, u64 gmfn);
+void print_vtd_entries(struct vtd_iommu *iommu, int bus, int devfn, u64 gmfn);
 keyhandler_fn_t vtd_dump_iommu_info;
 
-int enable_qinval(struct iommu *iommu);
-void disable_qinval(struct iommu *iommu);
-int enable_intremap(struct iommu *iommu, int eim);
-void disable_intremap(struct iommu *iommu);
+int enable_qinval(struct vtd_iommu *iommu);
+void disable_qinval(struct vtd_iommu *iommu);
+int enable_intremap(struct vtd_iommu *iommu, bool eim);
+void disable_intremap(struct vtd_iommu *iommu);
 
 void iommu_flush_cache_entry(void *addr, unsigned int size);
 void iommu_flush_cache_page(void *addr, unsigned long npages);
 int iommu_alloc(struct acpi_drhd_unit *drhd);
 void iommu_free(struct acpi_drhd_unit *drhd);
 
-int iommu_flush_iec_global(struct iommu *iommu);
-int iommu_flush_iec_index(struct iommu *iommu, u8 im, u16 iidx);
-void clear_fault_bits(struct iommu *iommu);
+int iommu_flush_iec_global(struct vtd_iommu *iommu);
+int iommu_flush_iec_index(struct vtd_iommu *iommu, u8 im, u16 iidx);
+void clear_fault_bits(struct vtd_iommu *iommu);
 
-struct iommu * ioapic_to_iommu(unsigned int apic_id);
-struct iommu * hpet_to_iommu(unsigned int hpet_id);
+struct vtd_iommu *ioapic_to_iommu(unsigned int apic_id);
+struct vtd_iommu *hpet_to_iommu(unsigned int hpet_id);
 struct acpi_drhd_unit * ioapic_to_drhd(unsigned int apic_id);
 struct acpi_drhd_unit * hpet_to_drhd(unsigned int hpet_id);
-struct acpi_drhd_unit * iommu_to_drhd(struct iommu *iommu);
+struct acpi_drhd_unit *iommu_to_drhd(struct vtd_iommu *iommu);
 struct acpi_rhsa_unit * drhd_to_rhsa(struct acpi_drhd_unit *drhd);
 
-struct acpi_drhd_unit * find_ats_dev_drhd(struct iommu *iommu);
+struct acpi_drhd_unit *find_ats_dev_drhd(struct vtd_iommu *iommu);
 
 int ats_device(const struct pci_dev *, const struct 

[Xen-devel] [PATCH 4/6] x86/vtd: Drop struct ir_ctrl

[Andrew Cooper]

It is unclear why this abstraction exists, but iommu_ir_ctrl() returns
possibly NULL and every user unconditionally dereferences the result.  In
practice, I can't spot a path where iommu is NULL, so I think it is mostly
dead.

Move the fields into struct vtd_iommu, and delete iommu_ir_ctrl().

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Paul Durrant 
CC: Kevin Tian 
---
 xen/drivers/passthrough/vtd/intremap.c | 90 --
 xen/drivers/passthrough/vtd/iommu.c|  3 +-
 xen/drivers/passthrough/vtd/iommu.h| 16 ++
 xen/drivers/passthrough/vtd/utils.c| 13 +++--
 4 files changed, 52 insertions(+), 70 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/intremap.c 
b/xen/drivers/passthrough/vtd/intremap.c
index 30b8f90..ce32bb1 100644
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -179,7 +179,7 @@ bool_t __init iommu_supports_eim(void)
 static void update_irte(struct vtd_iommu *iommu, struct iremap_entry *entry,
 const struct iremap_entry *new_ire, bool atomic)
 {
-ASSERT(spin_is_locked(_ir_ctrl(iommu)->iremap_lock));
+ASSERT(spin_is_locked(>iremap_lock));
 
 if ( cpu_has_cx16 )
 {
@@ -220,14 +220,13 @@ static void update_irte(struct vtd_iommu *iommu, struct 
iremap_entry *entry,
 static void free_remap_entry(struct vtd_iommu *iommu, int index)
 {
 struct iremap_entry *iremap_entry = NULL, *iremap_entries, new_ire = { };
-struct ir_ctrl *ir_ctrl = iommu_ir_ctrl(iommu);
 
 if ( index < 0 || index > IREMAP_ENTRY_NR - 1 )
 return;
 
-ASSERT( spin_is_locked(_ctrl->iremap_lock) );
+ASSERT(spin_is_locked(>iremap_lock));
 
-GET_IREMAP_ENTRY(ir_ctrl->iremap_maddr, index,
+GET_IREMAP_ENTRY(iommu->iremap_maddr, index,
  iremap_entries, iremap_entry);
 
 update_irte(iommu, iremap_entry, _ire, false);
@@ -235,7 +234,7 @@ static void free_remap_entry(struct vtd_iommu *iommu, int 
index)
 iommu_flush_iec_index(iommu, 0, index);
 
 unmap_vtd_domain_page(iremap_entries);
-ir_ctrl->iremap_num--;
+iommu->iremap_num--;
 }
 
 /*
@@ -245,10 +244,9 @@ static void free_remap_entry(struct vtd_iommu *iommu, int 
index)
 static unsigned int alloc_remap_entry(struct vtd_iommu *iommu, unsigned int nr)
 {
 struct iremap_entry *iremap_entries = NULL;
-struct ir_ctrl *ir_ctrl = iommu_ir_ctrl(iommu);
 unsigned int i, found;
 
-ASSERT( spin_is_locked(_ctrl->iremap_lock) );
+ASSERT(spin_is_locked(>iremap_lock));
 
 for ( found = i = 0; i < IREMAP_ENTRY_NR; i++ )
 {
@@ -259,7 +257,7 @@ static unsigned int alloc_remap_entry(struct vtd_iommu 
*iommu, unsigned int nr)
 if ( iremap_entries )
 unmap_vtd_domain_page(iremap_entries);
 
-GET_IREMAP_ENTRY(ir_ctrl->iremap_maddr, i,
+GET_IREMAP_ENTRY(iommu->iremap_maddr, i,
  iremap_entries, p);
 }
 else
@@ -274,8 +272,9 @@ static unsigned int alloc_remap_entry(struct vtd_iommu 
*iommu, unsigned int nr)
 if ( iremap_entries )
 unmap_vtd_domain_page(iremap_entries);
 
-if ( i < IREMAP_ENTRY_NR ) 
-ir_ctrl->iremap_num += nr;
+if ( i < IREMAP_ENTRY_NR )
+iommu->iremap_num += nr;
+
 return i;
 }
 
@@ -284,7 +283,6 @@ static int remap_entry_to_ioapic_rte(
 {
 struct iremap_entry *iremap_entry = NULL, *iremap_entries;
 unsigned long flags;
-struct ir_ctrl *ir_ctrl = iommu_ir_ctrl(iommu);
 
 if ( index < 0 || index > IREMAP_ENTRY_NR - 1 )
 {
@@ -294,9 +292,9 @@ static int remap_entry_to_ioapic_rte(
 return -EFAULT;
 }
 
-spin_lock_irqsave(_ctrl->iremap_lock, flags);
+spin_lock_irqsave(>iremap_lock, flags);
 
-GET_IREMAP_ENTRY(ir_ctrl->iremap_maddr, index,
+GET_IREMAP_ENTRY(iommu->iremap_maddr, index,
  iremap_entries, iremap_entry);
 
 if ( iremap_entry->val == 0 )
@@ -305,7 +303,7 @@ static int remap_entry_to_ioapic_rte(
 "IO-APIC index (%d) has an empty entry\n",
 index);
 unmap_vtd_domain_page(iremap_entries);
-spin_unlock_irqrestore(_ctrl->iremap_lock, flags);
+spin_unlock_irqrestore(>iremap_lock, flags);
 return -EFAULT;
 }
 
@@ -318,7 +316,8 @@ static int remap_entry_to_ioapic_rte(
 old_rte->dest.logical.logical_dest = iremap_entry->remap.dst >> 8;
 
 unmap_vtd_domain_page(iremap_entries);
-spin_unlock_irqrestore(_ctrl->iremap_lock, flags);
+spin_unlock_irqrestore(>iremap_lock, flags);
+
 return 0;
 }
 
@@ -332,11 +331,10 @@ static int ioapic_rte_to_remap_entry(struct vtd_iommu 
*iommu,
 struct IO_xAPIC_route_entry new_rte;
 int index;
 unsigned long flags;
-struct ir_ctrl *ir_ctrl = iommu_ir_ctrl(iommu);
 bool init = false;
 
 remap_rte = (struct IO_APIC_route_remap_entry *) old_rte;
-spin_lock_irqsave(_ctrl->iremap_lock, flags);
+

[Xen-devel] [PATCH 0/6] x86/vtd: Removal of unnecessary abstractions

[Andrew Cooper]

Patch 1 of this series was XSA-283 before people pointed out that I'd got my
maths wrong.  The rest of the series was the work I was doing at the time, to
try and clean up the IOMMU code.

This series comes with a net bloat-o-meter reduction of -536, a reduction in
code volume, runtime memory usage, and has no practical change in behaviour.

Andrew Cooper (6):
  x86/vtd: Don't include control register state in the table pointers
  x86/vtd: Rename struct iommu to vtd_iommu
  x86/vtd: Drop struct qi_ctrl
  x86/vtd: Drop struct ir_ctrl
  x86/vtd: Drop struct iommu_flush
  x86/vtd: Drop struct intel_iommu

 xen/drivers/passthrough/vtd/dmar.c |   6 +-
 xen/drivers/passthrough/vtd/dmar.h |   4 +-
 xen/drivers/passthrough/vtd/extern.h   |  36 +++
 xen/drivers/passthrough/vtd/intremap.c | 127 -
 xen/drivers/passthrough/vtd/iommu.c| 168 ++---
 xen/drivers/passthrough/vtd/iommu.h|  61 
 xen/drivers/passthrough/vtd/qinval.c   |  99 +--
 xen/drivers/passthrough/vtd/quirks.c   |  19 ++--
 xen/drivers/passthrough/vtd/utils.c|  28 +++---
 xen/drivers/passthrough/vtd/x86/ats.c  |   6 +-
 10 files changed, 237 insertions(+), 317 deletions(-)

-- 
2.1.4


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

[Xen-devel] [PATCH 1/6] x86/vtd: Don't include control register state in the table pointers

[Andrew Cooper]

iremap_maddr and qinval_maddr point to the base of a block of contiguous RAM,
allocated by the driver, holding the Interrupt Remapping table, and the Queued
Invalidation ring.

Despite their name, they are actually the values of the hardware register,
including control metadata in the lower 12 bits.  While uses of these fields
do appear to correctly shift out the metadata, this is very subtle behaviour
and confusing to follow.

Nothing uses the metadata, so make the fields actually point at the base of
the relevant tables.

Signed-off-by: Andrew Cooper 
Reviewed-by: Jan Beulich 
---
CC: Kevin Tian 

This was XSA-283 before people pointed out that I'd got my maths wrong, and
there wasn't actually a vulnerability with x2apic mode.  Either way, the
current code is downright dangerous.
---
 xen/drivers/passthrough/vtd/intremap.c | 13 +++--
 xen/drivers/passthrough/vtd/qinval.c   |  8 
 xen/drivers/passthrough/vtd/utils.c|  5 +++--
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/intremap.c 
b/xen/drivers/passthrough/vtd/intremap.c
index 838268d..1d19856 100644
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -802,14 +802,15 @@ int enable_intremap(struct iommu *iommu, int eim)
 ir_ctrl->iremap_num = 0;
 }
 
-/* set extended interrupt mode bit */
-ir_ctrl->iremap_maddr |= eim ? IRTA_EIME : 0;
-
 spin_lock_irqsave(>register_lock, flags);
 
-/* set size of the interrupt remapping table */
-ir_ctrl->iremap_maddr |= IRTA_REG_TABLE_SIZE;
-dmar_writeq(iommu->reg, DMAR_IRTA_REG, ir_ctrl->iremap_maddr);
+/*
+ * Set size of the interrupt remapping table and optionally Extended
+ * Interrupt Mode.
+ */
+dmar_writeq(iommu->reg, DMAR_IRTA_REG,
+ir_ctrl->iremap_maddr | IRTA_REG_TABLE_SIZE |
+(eim ? IRTA_EIME : 0));
 
 /* set SIRTP */
 gcmd = dmar_readl(iommu->reg, DMAR_GSTS_REG);
diff --git a/xen/drivers/passthrough/vtd/qinval.c 
b/xen/drivers/passthrough/vtd/qinval.c
index e95dc54..01447cf 100644
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -428,6 +428,8 @@ int enable_qinval(struct iommu *iommu)
 flush->context = flush_context_qi;
 flush->iotlb = flush_iotlb_qi;
 
+spin_lock_irqsave(>register_lock, flags);
+
 /* Setup Invalidation Queue Address(IQA) register with the
  * address of the page we just allocated.  QS field at
  * bits[2:0] to indicate size of queue is one 4KB page.
@@ -435,10 +437,8 @@ int enable_qinval(struct iommu *iommu)
  * registers are automatically reset to 0 with write
  * to IQA register.
  */
-qi_ctrl->qinval_maddr |= QINVAL_PAGE_ORDER;
-
-spin_lock_irqsave(>register_lock, flags);
-dmar_writeq(iommu->reg, DMAR_IQA_REG, qi_ctrl->qinval_maddr);
+dmar_writeq(iommu->reg, DMAR_IQA_REG,
+qi_ctrl->qinval_maddr | QINVAL_PAGE_ORDER);
 
 dmar_writeq(iommu->reg, DMAR_IQT_REG, 0);
 
diff --git a/xen/drivers/passthrough/vtd/utils.c 
b/xen/drivers/passthrough/vtd/utils.c
index 85e0f41..94a6e4e 100644
--- a/xen/drivers/passthrough/vtd/utils.c
+++ b/xen/drivers/passthrough/vtd/utils.c
@@ -204,8 +204,9 @@ void vtd_dump_iommu_info(unsigned char key)
 if ( status & DMA_GSTS_IRES )
 {
 /* Dump interrupt remapping table. */
-u64 iremap_maddr = dmar_readq(iommu->reg, DMAR_IRTA_REG);
-int nr_entry = 1 << ((iremap_maddr & 0xF) + 1);
+uint64_t irta = dmar_readq(iommu->reg, DMAR_IRTA_REG);
+uint64_t iremap_maddr = irta & PAGE_MASK;
+unsigned int nr_entry = 1 << ((irta & 0xF) + 1);
 struct iremap_entry *iremap_entries = NULL;
 int print_cnt = 0;
 
-- 
2.1.4


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

[Xen-devel] [freebsd-master test] 133365: all pass - PUSHED

[osstest service owner]

flight 133365 freebsd-master real [real]
http://logs.test-lab.xenproject.org/osstest/logs/133365/

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 freebsd  559f0dfc7a5f8f6a3ba157087820ce5e93c21486
baseline version:
 freebsd  fa1581bf5c27168a1678fb252001ffcbc00f3b31

Last test of basis   133325  2019-02-20 09:19:24 Z2 days
Testing same since   133365  2019-02-22 09:19:08 Z0 days1 attempts


People who touched revisions under test:
  andrew 
  avg 
  bde 
  cem 
  dab 
  dim 
  emaste 
  ganbold 
  jkim 
  kib 
  markj 
  mav 
  mmacy 
  sef 
  tsoome 
  tuexen 

jobs:
 build-amd64-freebsd-againpass
 build-amd64-freebsd  pass
 build-amd64-xen-freebsd  pass



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


Pushing revision :

To xenbits.xen.org:/home/xen/git/freebsd.git
   fa1581bf5c2..559f0dfc7a5  559f0dfc7a5f8f6a3ba157087820ce5e93c21486 -> 
tested/master

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

[Xen-devel] [xen-4.9-testing test] 133342: regressions - trouble: blocked/broken/fail/pass

[osstest service owner]

flight 133342 xen-4.9-testing real [real]
http://logs.test-lab.xenproject.org/osstest/logs/133342/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-armhf-armhf-xl-cubietruck broken
 build-amd64  broken
 build-arm64-xsm  broken
 build-i386-pvops broken
 build-i386   broken
 build-amd64-pvopsbroken
 build-i386-xsm   broken
 build-amd64-prev broken
 build-arm64-pvopsbroken
 build-arm64  broken
 build-i386-pvops  2 hosts-allocate broken REGR. vs. 132889
 build-i386-xsm2 hosts-allocate broken REGR. vs. 132889
 build-amd64-prev  2 hosts-allocate broken REGR. vs. 132889
 build-amd64   2 hosts-allocate broken REGR. vs. 132889
 build-i3862 hosts-allocate broken REGR. vs. 132889
 build-amd64-pvops 2 hosts-allocate broken REGR. vs. 132889
 build-arm644 host-install(4) broken in 133252 REGR. vs. 132889
 build-arm64-pvops  4 host-install(4) broken in 133252 REGR. vs. 132889
 build-arm64-xsm4 host-install(4) broken in 133252 REGR. vs. 132889
 test-xtf-amd64-amd64-2   broken  in 133314
 test-amd64-amd64-xl-qemut-debianhvm-amd64 broken in 133314
 test-amd64-amd64-xl-xsm  broken  in 133314
 test-amd64-amd64-i386-pvgrub broken  in 133314
 test-amd64-amd64-xl  broken  in 133314
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow   broken in 133314
 test-amd64-amd64-xl-qemut-debianhvm-amd64-xsm broken in 133314
 test-amd64-amd64-xl-qemuu-win10-i386  broken in 133314
 build-armhf-pvopsbroken  in 133314
 test-amd64-i386-xl-qemuu-ovmf-amd64   broken in 133314
 test-amd64-amd64-pygrub  broken  in 133314
 test-amd64-i386-xl-qemut-win10-i386   broken in 133314
 build-armhf-pvops  4 host-install(4) broken in 133314 REGR. vs. 132889
 test-amd64-i386-libvirt-pair 22 guest-migrate/src_host/dst_host fail in 133252 
REGR. vs. 132889
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop fail in 133252 REGR. vs. 
132889
 build-i386-pvops  6 kernel-build   fail in 133295 REGR. vs. 132889
 test-amd64-i386-freebsd10-i386  1 build-check(1)   running
 test-amd64-amd64-xl-qemuu-ovmf-amd64  1 build-check(1)   running
 build-amd64-libvirt   1 build-check(1)   running
 test-amd64-amd64-xl-credit2   1 build-check(1)   running
 test-amd64-amd64-xl   1 build-check(1)   running
 test-amd64-i386-xl-qemuu-ovmf-amd64  1 build-check(1)   running
 test-amd64-i386-xl-raw1 build-check(1)   running
 test-amd64-amd64-xl-rtds  1 build-check(1)   running
 test-amd64-amd64-rumprun-amd64  1 build-check(1)   running
 test-amd64-i386-libvirt   1 build-check(1)   running
 test-amd64-i386-xl-qemut-win7-amd64  1 build-check(1)   running
 build-arm64-libvirt   1 build-check(1)   running
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow  1 build-check(1)  running
 test-amd64-amd64-xl-qemuu-win10-i386  1 build-check(1)   running
 test-amd64-i386-xl-qemut-win10-i386  1 build-check(1)   running
 test-amd64-i386-xl-qemuu-win7-amd64  1 build-check(1)   running
 test-amd64-amd64-qemuu-nested-intel  1 build-check(1)   running
 test-amd64-i386-rumprun-i386  1 build-check(1)   running
 test-amd64-i386-xl-qemuu-ws16-amd64  1 build-check(1)   running
 test-amd64-amd64-xl-qemuu-debianhvm-amd64  1 build-check(1)running
 test-amd64-amd64-xl-qemut-win10-i386  1 build-check(1)   running
 test-amd64-amd64-xl-qemuu-win7-amd64  1 build-check(1)   running
 test-amd64-i386-xl-shadow 1 build-check(1)   running
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow  1 build-check(1) running
 build-i386-rumprun1 build-check(1)   running
 test-amd64-amd64-qemuu-nested-amd  1 build-check(1)   running
 test-amd64-i386-qemuu-rhel6hvm-amd  1 build-check(1)   running
 test-amd64-amd64-xl-qemut-win7-amd64  1 build-check(1)   running
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   running
 test-amd64-i386-xl-qemuu-debianhvm-amd64  1 build-check(1) running
 build-i386-libvirt1 build-check(1)   running
 test-amd64-i386-qemut-rhel6hvm-amd  

Re: [Xen-devel] [RFC PATCH 1/4] cert:arch/arm: Add missing default labels to switch statements

[Julien Grall]

Hi Oleksandr,

On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

It is required by MISRA [1] that every switch statement has a default
label as a measure of defensive programming technique.

The changes in this patch are to match MISRA C:2012: Rule 16.4
requirements.

[1] https://www.misra.org.uk/

Signed-off-by: Oleksandr Andrushchenko 
---
  xen/arch/arm/decode.c |  3 +++
  xen/arch/arm/domain.c | 10 ++
  xen/arch/arm/guest_walk.c |  2 ++
  xen/arch/arm/mm.c |  3 +++
  xen/arch/arm/p2m.c|  7 +++
  xen/arch/arm/traps.c  |  6 ++
  xen/arch/arm/vsmc.c   |  9 +
  7 files changed, 40 insertions(+)

diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
index 8b1e15d11892..1ed37696d678 100644
--- a/xen/arch/arm/decode.c
+++ b/xen/arch/arm/decode.c
@@ -112,6 +112,9 @@ static int decode_thumb(register_t pc, struct hsr_dabt 
*dabt)
  case 3: /* Signed byte */
  update_dabt(dabt, reg, 0, true);
  break;
+default:
+ASSERT_UNREACHABLE();
+goto bad_thumb;


Here the switch can only have 4 value (see opB & 0x3). They are handled 
correctly, so not only it does not make much sense to me and it adds more 
confusion to it.



  }
  
  break;

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 6dc633ed5048..ecb43736a7c3 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -439,6 +439,11 @@ unsigned long hypercall_create_continuation(
  case 3: regs->x3 = arg; break;
  case 4: regs->x4 = arg; break;
  case 5: regs->x5 = arg; break;
+/*
+ * arm_abi Hypercall Calling Convention:


s/arm_abi/ARM ABI/


+ * A hypercall can take up to 5 arguments.
+ */
+default: BUG(); break;


This should be an ASSERT_UNREACHABLE here.


  }
  }
  
@@ -462,6 +467,11 @@ unsigned long hypercall_create_continuation(

  case 3: regs->r3 = arg; break;
  case 4: regs->r4 = arg; break;
  case 5: regs->r5 = arg; break;
+/*
+ * arm_abi Hypercall Calling Convention:


Ditto.

+ * A hypercall can take up to 5 arguments.
+ */
+default: BUG(); break;


Ditto.


  }
  }
  
diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c

index 7db7a7321b20..8c4be32b7ef8 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -101,6 +101,8 @@ static bool guest_walk_sd(const struct vcpu *v,
  
  switch ( pte.walk.dt )

  {
+default:
+/* Fall through. */


Similar to the first switch, we cover all the values here. So what does it 
really bring us?



  case L1DESC_INVALID:
  return false;
  
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c

index 01ae2068..ba5bf5b2b3ba 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1112,6 +1112,9 @@ static void set_pte_flags_on_range(const char *p, 
unsigned long l, enum mg mg)
  pte.pt.xn = 0;
  pte.pt.ro = 1;
  break;
+default:
+pte.pt.valid = 0;
+break;


This one, ...


  }
  write_pte(xen_xenmap + i, pte);
  }
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index c38bd7e16e26..1e12dc0fd482 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -540,6 +540,10 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, 
p2m_access_t a)
  case p2m_max_real_type:
  BUG();
  break;
+
+default:
+BUG();
+break;


... this one and...


  }
  
  /* Then restrict with access permissions */

@@ -574,6 +578,9 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, 
p2m_access_t a)
  e->p2m.read = e->p2m.write = 0;
  e->p2m.xn = 1;
  break;
+default:
+BUG();
+break;


... this one is going to make much harder to extend the enum. TBH, I don't see 
the issue of initializing before the switch with default invalid value and don't 
add the default here.



  }
  }
  
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c

index 8741aa1d59ce..42e1bd54d31f 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1306,6 +1306,10 @@ int do_bug_frame(const struct cpu_user_regs *regs, 
vaddr_t pc)
  show_execution_state(regs);
  panic("Assertion '%s' failed at %s%s:%d\n",
predicate, prefix, filename, lineno);
+break;
+
+default:
+break;


As all the other case panic or return an error, you can move "return -EINVAL" 
here.


  }
  
  return -EINVAL;

@@ -1972,6 +1976,8 @@ static void do_trap_stage2_abort_guest(struct 
cpu_user_regs *regs,
  advance_pc(regs, hsr);
  return;
   

[Xen-devel] [xen-unstable-smoke test] 133371: regressions - FAIL

[osstest service owner]

flight 133371 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/133371/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-xl-qemuu-debianhvm-i386 10 debian-hvm-install fail REGR. vs. 
133343

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  e72ecc7615410e5bf1a1c9a4c7772322c16eeb82
baseline version:
 xen  db2af23d15077605f286d8ef86c8f5d9c1b8302a

Last test of basis   133343  2019-02-21 03:00:49 Z1 days
Testing same since   133371  2019-02-22 15:00:34 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Jan Beulich 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-i386 fail
 test-amd64-amd64-libvirt pass



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


Not pushing.


commit e72ecc7615410e5bf1a1c9a4c7772322c16eeb82
Author: Andrew Cooper 
Date:   Thu Jan 17 12:26:17 2019 +

x86/altp2m: Rework #VE enable/disable paths

Split altp2m_vcpu_{enable,disable}_ve() out of the
HVMOP_altp2m_vcpu_{enable,disable}_notify marshalling logic.  A future 
change
is going to need to call altp2m_vcpu_disable_ve() from the domain_kill() 
path.

While at it, clean up the logic in altp2m_vcpu_{initialise,destroy}().
altp2m_vcpu_reset() has no external callers, so fold it into its two
callsites.  This in turn allows for altp2m_vcpu_destroy() to reuse
altp2m_vcpu_disable_ve() rather than opencoding it.

No practical change.

Signed-off-by: Andrew Cooper 
Reviewed-by: Razvan Cojocaru 
Acked-by: Jan Beulich 
Release-acked-by: Juergen Gross 

commit 0dfffe01d5681ede6a50c6b57131320d9f4a3361
Author: Andrew Cooper 
Date:   Wed Feb 20 13:39:20 2019 +

x86: Improve the efficiency of domain_relinquish_resources()

pci_release_devices() takes the global PCI lock.  Once pci_release_devices()
has completed, it will be called redundantly each time paging_teardown() and
vcpu_destroy_pagetables() continue.

This is liable to be millions of times for a reasonably sized guest, and is 
a
serialising bottleneck now that domain_kill() can be run concurrently on
different domains.

Instead of propagating the opencoding of the relinquish state machine, take
the opportunity to clean it up.

Leave a proper set of comments explaining that domain_relinquish_resources()
implements a co-routine.  Introduce a documented PROGRESS() macro to avoid
latent bugs such as the RELMEM_xen case, and make the new PROG_* states
private to domain_relinquish_resources().

Signed-off-by: Andrew Cooper 
Reviewed-by: Wei Liu 
Acked-by: Jan Beulich 
Release-acked-by: Juergen Gross 
(qemu changes not included)

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

[Xen-devel] Xen Security Advisory 283 v2 - Withdrawn Xen Security Advisory number

[Xen . org security team]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

 Xen Security Advisory XSA-283
   version 2

  Withdrawn Xen Security Advisory number

SUMMARY
===

The advisory XSA-283 has been withdrawn.

This is because, on further analysis, we have determined that the
advisory was issued in error: there is no security issue.

UPDATES IN VERSION 2


Advisory withdrawn.

DESCRIPTION
===

XSA-283 stated:

VT-d: Incorrect accesses into the Interrupt Remapping table

   A VT-d IOMMU has several tables in main RAM, which are configured by the
   driver when it starts.  The tables are required to be aligned on a 4k
   boundary, and the control registers in the IOMMU which point to them use
   the bottom 12 bits for additional metadata.

   Unfortunately, Xen's VT-d driver includes this metadata in its base
   pointer to the table, resulting in incorrect calculations when indexing
   into the table.

Upon closer inspection, due to the particular way the calculations are
implemented, the "metadata" components end up being eliminated without
affecting the final result.

IMPLICATIONS


XSA-283 does not describe any security or functional issue.

The previously declared embargo for XSA-283 is vacated.
Anyone who has information relating to XSA-283 may publish it.

NB: there are other advisories are with the same embargo date.
Those advisories stand, and their embargoes REMAIN IN FORCE.

STATUS OF THE PATCHES
=

The patch previously published under embargo in XSA-283 is not
necessary.  However, it is harmless; indeed it improves code clarity
and is likely to be included in future Xen releases in some form.

In the interests of transparency, the patch is attached:

$ sha256sum xsa283*
97069456b91064450b6da1e9834f0ab91270f3b93962ca66f2eb9315cf133055  
xsa283.patch-withdrawn
$

There is no need to apply this patch.
If you have already applied it, there is no need to revert it.

CREDITS
===

Thanks to Pawel Wieczorkiewicz and Uwe Dannowski, both of Amazon, for
pointing out that there was no actual security issue.
-BEGIN PGP SIGNATURE-

iQFABAEBCAAqFiEEI+MiLBRfRHX6gGCng/4UyVfoK9kFAlxwNH8MHHBncEB4ZW4u
b3JnAAoJEIP+FMlX6CvZ9TkH/iGiPQgUhfvBOQamhBAbeCJ4877+lM+HSln3UiUy
hBvsA6mQCOsNKS2qUXQ8txE2w459V6DYbsmqFPRXLAaF7B+QMK6zPfICxwbCkyii
24qoITatBKvPpEhqzoM6VvkjpuUOi9+n41d/JVcyE53yAuA4R+bR9c36cz1j+j8J
Sd1Betvb5C51V6VQXjL/2zVb/v/fz5tuutIDC+jc7J1eHi7rN31TqizvuF19DQUu
YvSyUjfX2tSlzSp2oJ/uG1wZrAd0Ah+scViSZd6FUsCZyCiHsU02kG0zKfhXCsQ2
+3UkI+WylK2n664uUJAtvvYBkpnGejg224jqasrzGhjZASI=
=+3TC
-END PGP SIGNATURE-


xsa283.patch-withdrawn
Description: Binary data
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V2 3/3] xen/arm: Add SCIFA UART support for early printk

[Oleksandr]

Hi, Julien


Your solution below require to overwrite EARLY_PRINTK_INC and not 
very easy to extend of other version (e.g scifb). As I suggested 
earlier, we can introduce an option the same way REG_SHIFT exist for 
8250. The definition of CONFIG_EARLY_PRINTK is:


CONFIG_EARLY_PRINTK=,,

 would be the version. Nothing for SCIF and A for SCFIA.

Then in Rules.mk, you would have something like:

ifneq ($(word 3,$(EARLY_PRINTK_CFG)),)
CFLAGS-y += -DCONFIG_EARLY_PRINTK_VERSION_$(word 3, $(EARLY_PRINTK_CFG)
else
CFLAGS-y += -DCONFIG_EARLY_PRINTK_VERSION_NONE
endif

debug-scif.inc would then contain:

#ifdef CONFIG_EARLY_PRINTK_VERSION_A
#define foo
#define bar
#elifdef CONFIG_EARLY_PRINTK_VERSION_NONE
#define foo
#define bar
#endif

The CONFIG_EARLY_PRINTK_VERSION_NONE is here to help catching new 
addition. If someone if using a different version, it would not compile.
Also, the code in Rules.mk is generic enough to extend for other 
version (e.g scifb).


Does it make sense?


Absolutely. Thank you



Idea works as expected.


Just a quick question.

Shall I do this by a single patch or this should be spitted in two 
patches: "reworking current code" followed by "adding SCIFA"?








Cheers,


--
Regards,

Oleksandr Tyshchenko


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

Re: [Xen-devel] [PATCH RFC 00/39] x86/KVM: Xen HVM guest support

[Paolo Bonzini]

On 21/02/19 12:45, Joao Martins wrote:
> On 2/20/19 9:09 PM, Paolo Bonzini wrote:
>> On 20/02/19 21:15, Joao Martins wrote:
>>>  2. PV Driver support (patches 17 - 39)
>>>
>>>  We start by redirecting hypercalls from the backend to routines
>>>  which emulate the behaviour that PV backends expect i.e. grant
>>>  table and interdomain events. Next, we add support for late
>>>  initialization of xenbus, followed by implementing
>>>  frontend/backend communication mechanisms (i.e. grant tables and
>>>  interdomain event channels). Finally, introduce xen-shim.ko,
>>>  which will setup a limited Xen environment. This uses the added
>>>  functionality of Xen specific shared memory (grant tables) and
>>>  notifications (event channels).
>>
>> I am a bit worried by the last patches, they seem really brittle and
>> prone to breakage.  I don't know Xen well enough to understand if the
>> lack of support for GNTMAP_host_map is fixable, but if not, you have to
>> define a completely different hypercall.
>>
> I guess Ankur already answered this; so just to stack this on top of his 
> comment.
> 
> The xen_shim_domain() is only meant to handle the case where the backend
> has/can-have full access to guest memory [i.e. netback and blkback would work
> with similar assumptions as vhost?]. For the normal case, where a backend *in 
> a
> guest* maps and unmaps other guest memory, this is not applicable and these
> changes don't affect that case.
> 
> IOW, the PV backend here sits on the hypervisor, and the hypercalls aren't
> actual hypercalls but rather invoking shim_hypercall(). The call chain would 
> go
> more or less like:
> 
> 
>  gnttab_map_refs(map_ops, pages)
>HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref,...)
>  shim_hypercall()
>shim_hcall_gntmap()
> 
> Our reasoning was that given we are already in KVM, why mapping a page if the
> user (i.e. the kernel PV backend) is himself? The lack of GNTMAP_host_map is 
> how
> the shim determines its user doesn't want to map the page. Also, there's 
> another
> issue where PV backends always need a struct page to reference the device
> inflight data as Ankur pointed out.

Ultimately it's up to the Xen people.  It does make their API uglier,
especially the in/out change for the parameter.  If you can at least
avoid that, it would alleviate my concerns quite a bit.

Paolo

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

Re: [Xen-devel] [PATCH v5 3/5] p2m: change write_p2m_entry to return an error code

[Jan Beulich]

>>> On 21.02.19 at 17:50,  wrote:
> @@ -202,13 +204,14 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
>  new_entry = l1e_from_mfn(mfn, P2M_BASE_FLAGS | _PAGE_RW);
>  
>  p2m_add_iommu_flags(_entry, level, 
> IOMMUF_readable|IOMMUF_writable);
> -p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
> +rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
> +if ( rc )
> +ASSERT_UNREACHABLE();

Why not simply ASSERT(!rc)? Also further down then.

> --- a/xen/arch/x86/mm/shadow/none.c
> +++ b/xen/arch/x86/mm/shadow/none.c
> @@ -60,11 +60,12 @@ static void _update_paging_modes(struct vcpu *v)
>  ASSERT_UNREACHABLE();
>  }
>  
> -static void _write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn,
> - l1_pgentry_t *p, l1_pgentry_t new,
> - unsigned int level)
> +static int _write_p2m_entry(struct p2m_domain *p2m, unsigned long gfn,
> +l1_pgentry_t *p, l1_pgentry_t new,
> +unsigned int level)
>  {
>  ASSERT_UNREACHABLE();
> +return -ENOSYS;

-EOPNOTSUPP or basically anything else, but not -ENOSYS please.

Jan



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

[Xen-devel] [PATCH] x86/cpuid: add missing PCLMULQDQ dependency

[Jan Beulich]

Since we can't seem to be able to settle our discussion for the wider
adjustment previously posted, let's at least add the missing dependency
for 4.12. I'm not convinced though that attaching it to SSE is correct.

Signed-off-by: Jan Beulich 

--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -197,7 +197,7 @@ def crunch_numbers(state):
 # %XMM support, without specific inter-dependencies.  Additionally
 # AMD has a special mis-alignment sub-mode.
 SSE: [SSE2, SSE3, SSSE3, SSE4A, MISALIGNSSE,
-  AESNI, SHA],
+  AESNI, PCLMULQDQ, SHA],
 
 # SSE2 was re-specified as core instructions for 64bit.
 SSE2: [LM],



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

[Xen-devel] [linux-linus test] 133341: regressions - trouble: blocked/broken/fail/pass

[osstest service owner]

flight 133341 linux-linus real [real]
http://logs.test-lab.xenproject.org/osstest/logs/133341/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-amd64-rumprun  broken
 test-amd64-i386-xl-raw   broken
 build-amd64-pvopsbroken
 test-armhf-armhf-xl-cubietruck broken
 test-amd64-i386-libvirt-pair broken
 test-amd64-i386-pair broken
 test-amd64-i386-qemut-rhel6hvm-amd broken
 build-amd64-rumprun   2 hosts-allocate broken REGR. vs. 132911
 build-amd64-pvops 2 hosts-allocate broken REGR. vs. 132911
 test-amd64-i386-qemut-rhel6hvm-amd  4 host-install(4)  broken REGR. vs. 132911
 test-amd64-i386-pair5 host-install/dst_host(5) broken REGR. vs. 132911
 test-amd64-i386-xl-raw4 host-install(4)broken REGR. vs. 132911
 test-amd64-i386-libvirt-pair 4 host-install/src_host(4) broken REGR. vs. 132911
 test-armhf-armhf-xl-cubietruck  4 host-install(4)  broken REGR. vs. 132911
 build-arm64-xsm   6 xen-buildfail REGR. vs. 132911
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 10 debian-hvm-install fail 
REGR. vs. 132911
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 debian-hvm-install fail 
REGR. vs. 132911
 test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm 10 debian-hvm-install fail REGR. 
vs. 132911
 test-amd64-i386-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail REGR. vs. 132911
 test-amd64-i386-xl-qemut-debianhvm-amd64-xsm 10 debian-hvm-install fail REGR. 
vs. 132911
 test-armhf-armhf-libvirt-raw 10 debian-di-installfail REGR. vs. 132911
 test-amd64-amd64-xl-pvshim1 build-check(1)   running
 test-amd64-amd64-xl   1 build-check(1)   running
 test-amd64-amd64-xl-qcow2 1 build-check(1)   running
 test-amd64-amd64-xl-pvhv2-intel  1 build-check(1)   running
 test-amd64-amd64-xl-shadow1 build-check(1)   running
 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 1 build-check(1) running
 test-amd64-amd64-pygrub   1 build-check(1)   running
 test-amd64-amd64-xl-qemut-ws16-amd64  1 build-check(1)   running
 test-amd64-amd64-rumprun-amd64  1 build-check(1)   running
 test-amd64-amd64-xl-qemuu-ovmf-amd64  1 build-check(1)   running
 test-amd64-amd64-xl-multivcpu  1 build-check(1)   running
 test-amd64-amd64-libvirt-pair  1 build-check(1)   running
 test-amd64-amd64-qemuu-nested-intel  1 build-check(1)   running
 test-amd64-amd64-xl-qemuu-win10-i386  1 build-check(1)   running
 test-amd64-amd64-pair 1 build-check(1)   running
 test-amd64-amd64-xl-credit1   1 build-check(1)   running
 test-amd64-amd64-xl-rtds  1 build-check(1)   running
 test-amd64-amd64-examine  1 build-check(1)   running
 test-amd64-amd64-xl-pvhv2-amd  1 build-check(1)   running
 test-amd64-amd64-amd64-pvgrub  1 build-check(1)   running
 test-amd64-amd64-xl-qemut-win7-amd64  1 build-check(1)   running
 test-amd64-amd64-libvirt  1 build-check(1)   running
 test-amd64-amd64-xl-qemut-debianhvm-amd64  1 build-check(1)running
 test-amd64-amd64-xl-qemuu-ws16-amd64  1 build-check(1)   running
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow  1 build-check(1) running
 test-amd64-amd64-xl-qemuu-win7-amd64  1 build-check(1)   running
 test-amd64-amd64-libvirt-vhd  1 build-check(1)   running
 test-amd64-amd64-i386-pvgrub  1 build-check(1)   running
 test-amd64-amd64-qemuu-nested-amd  1 build-check(1)   running
 test-amd64-amd64-xl-qemut-win10-i386  1 build-check(1)   running
 test-amd64-amd64-xl-credit2   1 build-check(1)   running
 test-amd64-amd64-xl-qemuu-debianhvm-amd64  1 build-check(1)running
 build-arm64-xsm   3 syslog-serverrunning

Tests which did not succeed, but are not blocking:
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 1 build-check(1) blocked 
n/a
 test-amd64-amd64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-xsm   1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a
 test-amd64-amd64-xl-qemut-debianhvm-amd64-xsm  1 build-check(1)blocked n/a
 test-arm64-arm64-xl-xsm   1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsm  1 build-check(1)blocked n/a
 build-amd64-rumprun   3 capture-logs  broken blocked in 132911
 build-amd64-pvops 3 

Re: [Xen-devel] Organising a workshop to solve safety certification related questions (March 25/26, Cambridge, UK, Citrix)

[Lars Kurth]

Hi everyone,
I made some progress on the agenda: see 
https://docs.google.com/document/d/1aKjxDLkEnPZ_0gHgAv4xy9iPv6hVBkIC_wiA0rZzRms/edit
 

  
There are still a few gaps that need filling: feedback, additional suggestions, 
etc. are very welcome
Lars

> On 15 Feb 2019, at 18:38, Lars Kurth  wrote:
> 
> Hi all,
> 
> apologies this took a while. 5 weeks to go to the event!
> I created 
> https://wiki.xenproject.org/wiki/Developer_Meeting/March2019_-_Safety_Certification
>  
> 
>  which contains information about the venue, hotels and travel
> 
> I also added a registration form: please fill this out if you want to attend 
> (even if just remotely): see 
> https://wiki.xenproject.org/wiki/Developer_Meeting/March2019_-_Safety_Certification#Registration_.28both_in_person_and_for_remote_participation.29
>  
> 
> 
> I started to put in some thoughts with regards to the agenda: I will be 
> looking for volunteers to help with some of the content. I am working with 
> EPAM on some of these, but others are welcome to join.
> A scratchpad of ideas which will eventually become an agenda are here: 
> https://docs.google.com/document/d/1aKjxDLkEnPZ_0gHgAv4xy9iPv6hVBkIC_wiA0rZzRms/edit
>  
> 
>  
> 
> If you need a visa invitation letter, there is a 3 working day turnaround. 
> Please contact me via my Citrix email address as outlined in the Wiki page if 
> you need an invitation letter
> 
> Best Regards
> Lars
> 
> 
>> On 4 Feb 2019, at 20:25, Lars Kurth > > wrote:
>> 
>> Hi all,
>> 
>> from my perspective we have enough momentum to move forward, albeit some 
>> prospective attendees are still confirming their travel plans. I can 
>> accommodate a maximum if 15, but possibly, a few more. With this in Mind, 
>> please start booking flights.
>> 
>> Location:
>> The Citrix office in Milton, just outside of Cambridge
>> Citrix Systems:
>> 101 Cambridge Science Park Rd, Milton
>> Cambridge CB4 0FY
>> UK
>> 
>> Timing
>> The event will be held on Monday March 25, and ends on the 26th. I expect 
>> days to go from 9:00 to 17:00 and some beverages and food will be provided
>> EPAM will host an evening event on the 25th
>> 
>> Agenda
>> With regards the agenda, I will work selected community members on it.
>> The agenda is yours, so please prepare and be specific about the technical, 
>> community, process and maybe financial problems we have to solve.
>> 
>> Remote Participation
>> I will still need to test this and provide more feedback
>> 
>> Registration
>> I will set up an ad-hoc google doc
>> 
>> Getting To Cambridge/Accommodation
>> London Stansted is the easiest airport to fly to: there is a direct train 
>> that goes frequently and take 30-40 minutes
>> You may have to use London Heathrow you come from the US, China or Japan. In 
>> that case, you can take a fixed rate taxi: see 
>> http://www.expressairporttransport.co.uk/Taxi-Cambridge-To-Heathrow-Airport 
>> 
>> 
>> The key question you will have to decide upon is whether you stay in the 
>> town centre, which is stunning, but it may take 30 mins to the Citrix 
>> office, or whether you stay close tp the office. I will provide more info in 
>> due time
>> 
>> Regards
>> Lars 
>> 
>>> On 23 Jan 2019, at 10:16, Lars Kurth >> > wrote:
>>> 
>>> Hi all,
>>> 
>>> it looks as if March 25/26 in Frankfurt or Cambridge is the best option. 
>>> For Matt, this would mean that he can only attend the first day, but I 
>>> believe this would be OK. Maybe Robin can attend the second day, instead of 
>>> Matt. Before we finalise the dates, I will need to secure the meeting 
>>> space. I will be able to do this in the next few days and will send an 
>>> update as soon as this is done.
>>> 
>>> Note that we had a few people on this list which have replied to me 
>>> privately. Please let me know privately or publicly whether March 25/26 
>>> would be suitable for you. We can in parallel work on the agenda.
>>>  
>>> Best Regards
>>> Lars
>>> 
 On 16 Jan 2019, at 13:09, Lars Kurth >>> > wrote:
 
 
 
> On 16 Jan 2019, at 12:16, George Dunlap  > wrote:
> 
> On 1/8/19 5:59 PM, Lars Kurth wrote:
>> What I need is 
>> - Raise your hands if you are interested 
>> - Let me know of date / location restrictions
>> - We could try and so some of this via video conference: would you be 
>> able to attend if we did open the meeting up to some 

Re: [Xen-devel] rootfs about xen on FVP-Base-ReVC-2xAEMv8A

[Julien Grall]

On 22/02/2019 05:57, 敏 wrote:


hello


Hello,

now ，I am trying to run domain0（xen on FVP-Base-ReVC-2xAEMv8A）but there is a 
issue about rootfs ，

kernel panic  VFS：ubable to mount root fs on unknown-block
the filesystem image is xenial-server-cloudimg-arm64-uefi1.img
I can not resolve this issue ，can you give me some opinion？


Can you provide a bit more details such as the FVP command line, kernel command 
line and the full log?


Best regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH SpectreV1+L1TF v7 9/9] common/grant_table: block speculative out-of-bound accesses

[Jan Beulich]

>>> On 21.02.19 at 09:16,  wrote:
> @@ -226,10 +228,18 @@ nr_maptrack_frames(struct grant_table *t)
>  static grant_entry_header_t *
>  shared_entry_header(struct grant_table *t, grant_ref_t ref)
>  {
> -if ( t->gt_version == 1 )
> +switch ( t->gt_version )
> +{
> +case 1:
> +/* Make sure we return a value independently of speculative 
> execution */
> +block_speculation();
>  return (grant_entry_header_t*)_entry_v1(t, ref);
> -else
> +case 2:
> +/* Make sure we return a value independently of speculative 
> execution */
> +block_speculation();
>  return _entry_v2(t, ref).hdr;
> +}
> +return NULL;
>  }

I'm not happy with the comment wording, as to me it reads ambiguously
at best. How about "Make sure the value returned is independent of
speculative execution"? I'm of course open to even better suggestions,
in particular by native speakers.

Also please add a blank line
- between the individual case blocks,
- before the final (main) return statement.

Plus please add ASSERT_UNREACHABLE() ahead of the NULL return.

> @@ -963,9 +979,15 @@ map_grant_ref(
>  PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref %#x for d%d\n",
>   op->ref, rgt->domain->domain_id);
>  
> +/* Make sure the above bound check cannot be bypassed speculatively */
> +block_speculation();
> +
>  act = active_entry_acquire(rgt, op->ref);
>  shah = shared_entry_header(rgt, op->ref);

So shared_entry_header() now has a fence before consuming op->ref.
Is there anything wrong with swapping these two and dropping the
extra fence you add above, like you do in acquire_grant_for_copy()? All
this would seem to require is adding block_speculation() also onto the
"return NULL" path of shared_entry_header() (where it shouldn't hurt
at all).

> @@ -2946,6 +2981,7 @@ 
> gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
>  grant_entry_v1_t reserved_entries[GNTTAB_NR_RESERVED_ENTRIES];
>  int res;
>  unsigned int i;
> +unsigned int gt_nr_grant_entries;

Rather then lengthening the name by adding a disambiguating prefix,
how about shortening it to "nr" or "nr_ents" (also elsewhere)? Also
please add onto the line declaring i instead of adding yet another line
with the same type.

> @@ -2969,7 +3005,8 @@ 
> gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
>   * are allowed to be in use (xenstore/xenconsole keeps them mapped).
>   * (You need to change the version number for e.g. kexec.)
>   */
> -for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < nr_grant_entries(gt); i++ )
> +gt_nr_grant_entries = nr_grant_entries(gt);
> +for ( i = GNTTAB_NR_RESERVED_ENTRIES; i < gt_nr_grant_entries; i++ )

This then also calls for a 3rd block_speculation() in nr_grant_entries(),
I think.

Jan



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

Re: [Xen-devel] [PATCH SpectreV1+L1TF v7 8/9] x86/hvm: add nospec to hvmop param

[Jan Beulich]

>>> On 21.02.19 at 09:16,  wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4109,6 +4109,13 @@ static int hvmop_set_param(
>  if ( a.index >= HVM_NR_PARAMS )
>  return -EINVAL;
>  
> +/*
> + * Make sure the guest controlled value a.index is bounded even during
> + * speculative execution.
> + */
> +a.index = array_index_nospec(a.index, HVM_NR_PARAMS);
> +block_speculation();
> +
>  d = rcu_lock_domain_by_any_id(a.domid);
>  if ( d == NULL )
>  return -ESRCH;
> @@ -4375,6 +4382,13 @@ static int hvmop_get_param(
>  if ( a.index >= HVM_NR_PARAMS )
>  return -EINVAL;
>  
> +/*
> + * Make sure the guest controlled value a.index is bounded even during
> + * speculative execution.
> + */
> +a.index = array_index_nospec(a.index, HVM_NR_PARAMS);
> +block_speculation();

Please can the comments briefly explain the otherwise apparently
pointless redundancy of both constructs?

Jan



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

Re: [Xen-devel] Fwd: xen: credit2: credit2 can’t reach the throughput as expected

[Dario Faggioli]

On Mon, 2019-02-18 at 14:04 +, zheng chuan wrote:
> Hi, Dario
>
Hi,

> [sorry for the html email format, resend by text.]
> 
Thanks! :-)

> > On Fri, 2019-02-15 at 06:15 +, zheng chuan wrote:
> > > 
> > Now, can I ask you a favour? Can you rerun with:
> > 
> > sched_credit2_migrate_resist=0
> > 
> > added to Xen's boot command line?
> > 
> Unfortunately, sched_credit2_migrate_resist=0 seems do not work :(
> It still around 60% and 120% for guest_1 and guest_2 with
> ratelimiting of 1ms, respectively
> 
Ok, I was just curios. Thanks for doing the test.

> > But can we actually try to measure latency as well? Because it
> > looks to me that
> > we're discussing while having only half of the picture available.
> > 
> Sure, but due to my lack of knowledge, does xen have sched_latency
> measurement
> tools like perf sched_latency for CFS ? I will give it try to do it
> if I get it.
> 
No, we don't have anything like that. When speaking of measuring
latencies, I was mainly thinking to logging some timestamps from your
app.

Anyway, I just wanted to let you know that I'm looking into this, but
it's a few days that I don't feel very well, that's why I wasn't
replying to emails.

I'll get back to it when I'm better.

> What I am tried is rude and empirical, which is shown as bellow:
> 
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 9a3e71f..b781ebe 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -1642,13 +1642,13 @@ static void reset_credit(const struct
> scheduler *ops, int cpu, s_time_t now,
> if ( snext->credit < -CSCHED2_CREDIT_INIT )
> m += (-snext->credit) / CSCHED2_CREDIT_INIT;
> 
> - list_for_each( iter, >svc )
> + list_for_each( iter, >runq )
> {
>
Ok, I see. Well, if we do something like this, we'll have to figure out
what happens when one of the vcpu which didn't get the credit reset,
wakes up, and how that influence the algorithm.

I was thinking more to have csched2_runtime() return something
"clever", but I have no precise idea yet.

Regards,
Dario
-- 
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/


signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 3/4] x86/vmx: Fix security issue when a guest balloons out the #VE info page

[Andrew Cooper]

On 22/02/2019 12:24, Jan Beulich wrote:
 On 21.02.19 at 21:18,  wrote:
>> The logic in altp2m_vcpu_{en,dis}able_ve() and vmx_vcpu_update_vmfunc_ve() is
>> dangerous.  After #VE has been set up, the guest can balloon out and free the
>> nominated GFN, after which the processor may write to it.  Also, the unlocked
>> GFN query means the MFN is stale by the time it is used.  Alternatively, a
>> guest can race two disable calls to cause one VMCS to still reference the
>> nominated GFN after the tracking information was dropped.
>>
>> Rework the logic from scratch to make it safe.
>>
>> Hold an extra page reference on the underlying frame, to account for the
>> VMCS's reference.  This means that if the GFN gets ballooned out, it isn't
>> freed back to Xen until #VE is disabled, and the VMCS no longer refers to the
>> page.
>>
>> A consequence of this is that altp2m_vcpu_disable_ve() needs to be called
>> during the domain_kill() path, to drop the reference for domains which shut
>> down with #VE still enabled.
>>
>> For domains using altp2m, we expect a single enable call and no disable for
>> the remaining lifetime of the domain.  However, to avoid problems with
>> concurrent calls, use cmpxchg() to locklessly maintain safety.
>>
>> This doesn't have an XSA because altp2m is not yet a security-supported
>> feature.
>>
>> Signed-off-by: Andrew Cooper 
>> Release-acked-by: Juergen Gross 
> Reviewed-by: Jan Beulich 
>
> I would appreciate though if you could add half a sentence clarifying
> that and why you decided against obtaining a writable type-ref.
>
> Also - I take it that altp2m and migration don't work together?
> Otherwise wouldn't you need to mark the page dirty in more
> cases?

Noone ever considers migration, and it definitely doesn't work as a
self-contained action.  For one, nothing preserves the already
established mem_access / alternatives information.

For general logdirty, you will recall that Razvan spent a long time
fixing that in Xen 4.12.

The #VE-info page itself can in principle be tracked with Page
Modification Logging, but PML isn't available because EPT A/D is
mutually exclusive with introspection in current processors.  Enabling
EPT A/D causes all guest pagetable accesses to be treated as writes even
when they aren't setting guest A/D bits, meaning that if you try and
write protect a pagetable, you livelock taking EPT_VIOLATIONs when
trying to perform a TLB Fill.

However, what happens in practice is that you can't safely migrate VM's
with an attached vmevent ring as part of the p2m (as the guest kernel
can interfere), and you might notice that there is ongoing effort to use
resource mapping instead.  IIRC Introspection detaches in the post-pause
phase of migrate, and (by design) is capable of re-initialising all
protections from first principles at the destination (so individual
introspection agents on each server don't need to communicate state).

So overall, migration does work at least in the altp2m_external case,
but only due to a fair amount of heavy lifting in the toolstack

~Andrew

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

[Xen-devel] [ovmf test] 133354: regressions - FAIL

[osstest service owner]

flight 133354 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/133354/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail REGR. vs. 133291

version targeted for testing:
 ovmf 08b4ad6283c362fff412baa95fb49dc5dc7ccb3c
baseline version:
 ovmf c417c1b33d06ef6ae96adb373201a5a3c3b38772

Last test of basis   133291  2019-02-18 01:41:15 Z4 days
Failing since133305  2019-02-19 00:41:21 Z3 days4 attempts
Testing same since   133354  2019-02-22 01:56:14 Z0 days1 attempts


People who touched revisions under test:
  Albecki Mateusz 
  Albecki, Mateusz 
  Anthony PERARD 
  Ard Biesheuvel 
  Bob Feng 
  Chasel Chiu 
  Chasel, Chiu 
  Chen A Chen 
  Dandan Bi 
  Edgar Handal 
  Fan, ZhijuX 
  Feng, Bob C 
  Gonzalez Del Cueto, Rodrigo 
  Hao Wu 
  Jeff Brasen 
  Jian J Wang 
  Jiaxin Wu 
  Jordan Justen 
  Julien Grall 
  Laszlo Ersek 
  Liming Gao 
  Max Knutsen 
  Ray Ni 
  Rodrigo Gonzalez del Cueto 
  Ruiyu Ni 
  Sami Mujawar 
  Shenglei Zhang 
  Star Zeng 
  Wu Jiaxin 
  Zhichao Gao 
  Zhiju.Fan 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 pass
 test-amd64-i386-xl-qemuu-ovmf-amd64  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


Not pushing.

(No revision log; it would be 1603 lines long.)

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

[Xen-devel] [libvirt test] 133347: regressions - trouble: blocked/broken/fail/pass

[osstest service owner]

flight 133347 libvirt real [real]
http://logs.test-lab.xenproject.org/osstest/logs/133347/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-armhf  broken
 build-arm64  broken
 build-armhf   2 hosts-allocate broken REGR. vs. 133272
 test-amd64-amd64-libvirt  8 host-ping-check-xen  fail REGR. vs. 133272
 build-i386-libvirt6 libvirt-buildfail REGR. vs. 133272
 build-armhf-libvirt   1 build-check(1)   running
 build-arm64-libvirt   1 build-check(1)   running

Regressions which are regarded as allowable (not blocking):
 build-arm64   2 hosts-allocate broken REGR. vs. 133272

Tests which did not succeed, but are not blocking:
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a
 test-amd64-i386-libvirt-pair  1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-xsm   1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-raw  1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt   1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-qcow2  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt  1 build-check(1)   blocked  n/a
 build-armhf   3 capture-logs  broken blocked in 133272
 build-arm64   3 capture-logs  broken blocked in 133272
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   never pass

version targeted for testing:
 libvirt  27eb32466807f1f46e0b72a2270c0863300542d2
baseline version:
 libvirt  0624ac3fa846b3e2a8e70e4cc2fd8477710cd76d

Last test of basis   133272  2019-02-15 22:58:38 Z6 days
Failing since133306  2019-02-19 04:19:06 Z3 days3 attempts
Testing same since   133347  2019-02-21 11:31:20 Z1 days1 attempts


People who touched revisions under test:
  Andrea Bolognani 
  Chris Venteicher 
  Daniel P. Berrangé 
  Eric Blake 
  Jiri Denemark 
  John Ferlan 
  Ján Tomko 
  Marc Hartmayer 
  Michal Privoznik 
  Nikolay Shirokovskiy 
  Peter Krempa 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-arm64  broken  
 build-armhf  broken  
 build-i386   pass
 build-amd64-libvirt  pass
 build-arm64-libvirt  blocked 
 build-armhf-libvirt  blocked 
 build-i386-libvirt   fail
 build-amd64-pvopspass
 build-arm64-pvopspass
 build-armhf-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm   pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmblocked 
 test-amd64-amd64-libvirt-xsm pass
 test-arm64-arm64-libvirt-xsm blocked 
 test-amd64-i386-libvirt-xsm  blocked 
 test-amd64-amd64-libvirt fail
 test-arm64-arm64-libvirt blocked 
 test-armhf-armhf-libvirt blocked 
 test-amd64-i386-libvirt  blocked 
 test-amd64-amd64-libvirt-pairpass
 test-amd64-i386-libvirt-pair blocked 
 test-arm64-arm64-libvirt-qcow2   blocked 
 test-armhf-armhf-libvirt-raw blocked 
 test-amd64-amd64-libvirt-vhd pass



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

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Julien Grall]

Hi,

On 22/02/2019 12:38, Oleksandr Andrushchenko wrote:

On 2/20/19 10:46 PM, Julien Grall wrote:
Discussing with my team, a solution that came up would be to introduce one 
atomic field per event to record the number of event received. I will explore 
that solution tomorrow.

How will this help if events have some payload?


What payload? The event channel does not carry any payload. It only notify you 
that something happen. Then this is up to the user to decide what to you with it.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

On 22/02/2019 11:53, Andrii Anisov wrote:

Hello Julien,


Hi,


On 22.02.19 13:30, Julien Grall wrote:
While review tend to be very thorough, it is sometimes hard to spot when we 
miss a case. This is where -Wswitch comes into place to spot missing how.


How the BUG/ASSERT_UNREACHABLE solution is going to help us here?
I understand that hitting -Wswitch requires less efforts, the compilation only. 
Otherwise build run, even fulfilling some conditions are needed to hit missed 
enum index.


But how those `switch()` without `default:` label are protected against the 
value outside the enum?


You really don't need a default case for handling such issues. You can construct 
code in a way to initialize the variables with default values.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] Reducing or removing direct map from xen (was Re: Ongoing/future speculative mitigation work)

[Jan Beulich]

>>> On 22.02.19 at 14:24,  wrote:
 On 22.02.19 at 14:19,  wrote:
> > I have only realised this today: essentially we will end up implementing
> > xmalloc with vmalloc, which at the moment depends on xmalloc to allocate
> > the array of mfns.
> 
> Which (potential locking issues aside) is not a problem, as the size of
> the MFN array will reduce logarithmically, until it eventually is no larger
> than a page anymore.

Err, not logarithmically, but you get the point.

Jan



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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 22/02/2019 12:01, Oleksandr Andrushchenko wrote:

On 2/22/19 1:27 PM, Julien Grall wrote:

Hi Oleksandr,

On 22/02/2019 11:13, Oleksandr Andrushchenko wrote:

On 2/22/19 1:05 PM, Julien Grall wrote:

Hi,

On 22/02/2019 10:27, Andrew Cooper wrote:

On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

Hello, everybody!

We at EPAM Systems would like to present first series of patches targeting 
Xen

on ARM Functional Safety certification (ISO61508 based): implementation of
MISRA [1] C:2012 Rule 16.4 which requires that every switch statement has a
default label as a measure of defensive programming technique.


Hang on - what?

Can someone attempt to justify why actively breaking -Wswitch is going
to result in safer/better code?


I was about to ask the same. There are quite a few cases where this series 
is going to make more difficult extending enum.



Well, I am not sure I can truly defend MISRA requirements here, but I'll try
to express my view on that.

Let us have a look at gcc's options [1]: so, for what you are saying:
"-Wswitch
Warn whenever a switch statement has an index of enumerated type and lacks a 
case for one or more of the named codes of that enumeration. (The presence of 
a default label prevents this warning.) case labels outside the enumeration 
range also provoke warnings when this option is used (even if there is a 
default label). This warning is enabled by -Wall."


So, if we do not have all the cases covered then this compiler's switch will 
fire
a warning (error). What if we have an integer as a switch's index, not an 
enumeration,
so then there is no easy way to handle all the cases and we have to provide 
*default* statement.


You have a point for the integer switch.



What if with time enumeration changes, what if some of the case statements 
get removed and so on.


In that case, the compiler will throw an error (Xen is built with -Werror). So 
for enumeration the compiler will help us to spot the missing places.


If you add a default case, you remove us a good way to check we actually add 
the new element everywhere.
The answer is that what happens if we by any reason either by a mistake or any 
other mean
have a build which doesn't have -Wswitch or -Wall. I mean that everything 
changes and having

"default" in the source does guarantee the handling as it was intended to be.


If you consider that and ...
[...]



But what happens if you miss default handling and because of bugs in the code 
you do not

handle "compile time unexpected values"?
that. Then when do you put a limit between theoretical and real issue? For 
instance, what is the default case is introduced an unintended behavior?




BTW, I checked the series with -Wswitch-default:
-Wswitch-default
Warn whenever a switch statement does not have a default case.
Furthermore, using BUG() is a pretty bad idea in switch. 

It is and not only in the switch. The reason I put BUG is that I tried to follow
the existing "error handling" at those places.


It is not because BUG() is been used today in some places that we need to 
continue to spread it.



Use of BUG() itself is another topic which will also need to be addressed


So we should not add more of them...

Again, I see this as a dedicated change. So, in the current series I think it is
acceptable to use the existing way of error handling if any at all.


That's not how it works in upstream. If you know some constructs are wrong, it 
is best to try to address partially the problem directly then having so you 
reduce the amounts of change afterwards.


So please try to not introduce more BUG() in the code base.

Cheers,

--
Julien Grall

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

Re: [Xen-devel] Reducing or removing direct map from xen (was Re: Ongoing/future speculative mitigation work)

[Jan Beulich]

>>> On 22.02.19 at 14:19,  wrote:
> I have only realised this today: essentially we will end up implementing
> xmalloc with vmalloc, which at the moment depends on xmalloc to allocate
> the array of mfns.

Which (potential locking issues aside) is not a problem, as the size of
the MFN array will reduce logarithmically, until it eventually is no larger
than a page anymore.

Jan



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

Re: [Xen-devel] [PATCH v2] iommu: leave IOMMU enabled by default during kexec crash transition

[Jan Beulich]

>>> On 21.02.19 at 23:08,  wrote:
> It's unsafe to disable IOMMU on a live system which is the case
> if we're crashing since remapping hardware doesn't usually know what
> to do with ongoing bus transactions and frequently raises NMI/MCE/SMI,
> etc. (depends on the firmware configuration) to signal these abnormalities.
> This, in turn, doesn't play well with kexec transition process as there is
> no handling available at the moment for this kind of events resulting
> in failures to enter the kernel.
> 
> Modern Linux kernels taught to copy all the necessary DMAR/IR tables
> following kexec from the previous kernel (Xen in our case) - so it's
> currently normal to keep IOMMU enabled. It might require minor changes to
> kdump command line that enables IOMMU drivers (e.g. intel_iommu=on /
> intremap=on) but recent kernels don't require any additional changes for
> the transition to be transparent.
> 
> A fallback option is still left for compatibility with ancient crash
> kernels which didn't like to have IOMMU active under their feet on boot.
> 
> Signed-off-by: Igor Druzhinin 

Based on the subsequent discussion
Acked-by: Jan Beulich 

Jan



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

Re: [Xen-devel] [PATCH SpectreV1+L1TF v7 5/9] is_control_domain: block speculation

[Jan Beulich]

>>> On 21.02.19 at 09:16,  wrote:
> Checks of domain properties, such as is_hardware_domain or is_hvm_domain,
> might be bypassed by speculatively executing these instructions. A reason
> for bypassing these checks is that these macros access the domain
> structure via a pointer, and check a certain field. Since this memory
> access is slow, the CPU assumes a returned value and continues the
> execution.
> 
> In case an is_control_domain check is bypassed, for example during a
> hypercall, data that should only be accessible by the control domain could
> be loaded into the cache.
> 
> This is part of the speculative hardening effort.
> 
> Signed-off-by: Norbert Manthey 

Acked-by: Jan Beulich 



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

Re: [Xen-devel] [PATCH SpectreV1+L1TF v7 6/9] is_hvm/pv_domain: block speculation

[Jan Beulich]

>>> On 21.02.19 at 09:16,  wrote:
> When checking for being an hvm domain, or PV domain, we have to make
> sure that speculation cannot bypass that check, and eventually access
> data that should not end up in cache for the current domain type.
> 
> This is part of the speculative hardening effort.
> 
> Signed-off-by: Norbert Manthey 

Acked-by: Jan Beulich 



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

Re: [Xen-devel] [PATCH v2 00/14] Add support for Hygon Dhyana Family 18h processor

[Wei Liu]

On Fri, Feb 22, 2019 at 10:30:17AM +0800, Pu Wen wrote:
> On 2019/2/22 0:38, Wei Liu wrote:
> > I think the version should have been v5?
> 
> Aha. This is the second revision of the patch series. So why should it
> have been v5?

I have seen several previous postings from last year. But I just
realised they were for Linux, not Xen.

Sorry for the noise.

Wei.

> 
> -- 
> Regards,
> Pu Wen

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

Re: [Xen-devel] Reducing or removing direct map from xen (was Re: Ongoing/future speculative mitigation work)

[Wei Liu]

On Fri, Feb 22, 2019 at 05:47:13AM -0700, Jan Beulich wrote:
> >>> On 22.02.19 at 13:11,  wrote:
> > On Fri, Feb 22, 2019 at 05:06:03AM -0700, Jan Beulich wrote:
> >> >>> On 22.02.19 at 12:50,  wrote:
> >> > On Fri, Feb 22, 2019 at 04:48:09AM -0700, Jan Beulich wrote:
> >> >> >>> On 20.02.19 at 18:08,  wrote:
> >> >> > On Wed, Feb 20, 2019 at 01:09:56PM +, Wei Liu wrote:
> >> >> > [...]
> >> >> >> I think under-allocate-then-map looks plausible. xmalloc will need
> >> >> >> to allocate pages, put them into an array and call __vmap on that 
> >> >> >> array
> >> >> >> directly.
> >> >> > 
> >> >> > The biggest issue with this approach is that we now need an array of
> >> >> > 1UL< >> >> > x86
> >> >> > this is going to be (1UL<<20)*8 bytes long. This is not feasible.
> >> >> 
> >> >> Are we really calling xmalloc() with any number nearly this big?
> >> > 
> >> > In practice, I don't think so. What do you think is a sensible limit?
> >> 
> >> I'm afraid you won't like the answer: Whatever the biggest chunk is
> >> we currently allocate anywhere. Perhaps, e.g. if there's a single big
> >> "violator", changing some code to reduce the upper bound might be
> >> desirable.
> >> 
> >> In general there shouldn't be any going beyond one page once we've
> >> completed booting. Several years back I think I had managed to
> >> replace most (all?) higher order xmalloc()-s. So another option might
> >> be to allow up to MAX_ORDER by way of some init-only mechanism,
> >> and later allow only up to single page chunks.
> >> 
> > 
> > Think about it, if you have done the work to remove high order
> > allocations, removing this optimisation is the easiest thing to do and
> > wouldn't make things worse, isn't it?
> 
> Not sure I understand what exactly you want to remove. Allocating

Remove the code that returns those 17 pages.

> 32 pages when you need 17 is wasteful, and hence I'd prefer if we
> could continue to make actual use of the remaining 15. That's
> independent of whether the allocation occurs at boot or run time.
> 

Sure. But in my opinion there will only be one such wastage in the life
time of system then opting for simpler code is a far better approach
(with appropriate checks in place). On the other hand, if not returning
pages results in wasting almost half of each allocation, we will need to
think of a clever way. Your reply made me think of the former.

I have only realised this today: essentially we will end up implementing
xmalloc with vmalloc, which at the moment depends on xmalloc to allocate
the array of mfns.

Wei.

> Jan
> 
> 

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

Re: [Xen-devel] [PATCH SpectreV1+L1TF v7 4/9] nospec: introduce evaluate_nospec

[Jan Beulich]

>>> On 21.02.19 at 09:16,  wrote:
> --- /dev/null
> +++ b/xen/include/asm-x86/nospec.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. */
> +
> +#ifndef _ASM_X86_NOSPEC_H
> +#define _ASM_X86_NOSPEC_H
> +
> +#include 
> +
> +/* Allow to insert a read memory barrier into conditionals */
> +static always_inline bool barrier_nospec_true(void)
> +{
> +#ifdef CONFIG_HVM
> +alternative("", "lfence", X86_FEATURE_SC_L1TF_VULN);
> +#endif
> +return true;
> +}
> +
> +/* Allow to protect evaluation of conditionasl with respect to speculation */
> +#ifdef CONFIG_HVM
> +#define evaluate_nospec(condition) \
> +((condition) ? barrier_nospec_true() : !barrier_nospec_true())
> +#else
> +#define evaluate_nospec(condition) (condition)
> +#endif
> +
> +/* Allow to block speculative execution in generic code */
> +#define block_speculation() ((void)barrier_nospec_true())

Seeing Julien's request for switching to inline functions on the Arm side,
is there anything preventing these two to become inline functions too?

Jan



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

Re: [Xen-devel] [PATCH SpectreV1+L1TF v7 3/9] spec: add l1tf-barrier

[Jan Beulich]

>>> On 21.02.19 at 09:16,  wrote:
> To control the runtime behavior on L1TF vulnerable platforms better, the
> command line option l1tf-barrier is introduced. This option controls
> whether on vulnerable x86 platforms the lfence instruction is used to
> prevent speculative execution from bypassing the evaluation of
> conditionals that are protected with the evaluate_nospec macro.
> 
> By now, Xen is capable of identifying L1TF vulnerable hardware. However,
> this information cannot be used for alternative patching, as a CPU feature
> is required. To control alternative patching with the command line option,
> a new x86 feature "X86_FEATURE_SC_L1TF_VULN" is introduced. This feature
> is used to patch the lfence instruction into the arch_barrier_nospec_true
> function. The feature is enabled only if L1TF vulnerable hardware is
> detected and the command line option does not prevent using this feature.
> 
> The status of hyperthreading is considered when automatically enabling
> adding the lfence instruction. Since platforms without hyperthreading can
> still be vulnerable to L1TF in case the L1 cache is not flushed properly,
> the additional lfence instructions are patched in if either hyperthreading
> is enabled, or L1 cache flushing is missing.
> 
> This is part of the speculative hardening effort.
> 
> Signed-off-by: Norbert Manthey 

Reviewed-by: Jan Beulich 
with one remark:

> @@ -842,6 +849,12 @@ void __init init_speculation_mitigations(void)
>  else if ( opt_l1d_flush == -1 )
>  opt_l1d_flush = cpu_has_bug_l1tf && !(caps & ARCH_CAPS_SKIP_L1DFL);
>  
> +/* By default, enable L1TF_VULN on L1TF-vulnerable hardware */
> +if ( opt_l1tf_barrier == -1 )
> +opt_l1tf_barrier = cpu_has_bug_l1tf && (opt_smt != 0 || 
> opt_l1d_flush == 0);

We commonly omit "!= 0" and use ! instead of "== 0". If I end
up committing this, I may take the liberty of changing these.

Jan



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

Re: [Xen-devel] [PATCH] iommu: leave IOMMU enabled by default during kexec crash transition

[Igor Druzhinin]

On 22/02/2019 12:51, Jan Beulich wrote:
 On 22.02.19 at 13:40,  wrote:
>> There are several reasons why it's better:
>> a) kernel is able to perform device reset properly as it has bus
>> specific code that does this. There is even a comment in the code
>> mentioning that at the moment it disables the translation bus-specific
>> reset is finished and it's safer (as devices likely stopped DMA at this
>> point) to do it now.
>> b) kernel has the drivers that do per-driver-specific reset of the
>> devices that do not work well with bus-specific reset. It's simply
>> impossible to implement that in Xen
>> c) even if a device is uncooperative and keeps sending bus transactions,
>> an error event that I mentioned earlier will be properly handled as we
>> have facilities for it in the kernel at that point
> 
> Okay, c) is a convincing argument. a) and b) are partly only: Iirc
> crash kernels don't load unnecessary drivers, so a babbling device
> may be left untouched unless generic kernel code can reset or
> otherwise silence it.

Crash kernel loads whatever drivers are necessary to save crash dumps:
if it needs to load RAID controller drivers which is still sending DMAs
it will do it - therefore it will try to reset the device.

In order to avoid scenario in (c) if the device is untouched it's still
unsafe to disable translation while any of the devices on a bus haven't
been properly reset. So the kernel will try it's best to avoid (c) and
will reset all the devices.

I want to also mention that I've tested this particular change in our
lab on about 500 of machines of different ages and classes. It
definitely makes the crash path more reliable - before this change
entering the crash dump failed in ~25-30% of the cases while with this
change reliability is close to 98-99%.

Igor

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

Re: [Xen-devel] [PATCH SpectreV1+L1TF v7 2/9] x86/vioapic: block speculative out-of-bound accesses

[Jan Beulich]

>>> On 21.02.19 at 09:16,  wrote:
> When interacting with io apic, a guest can specify values that are used
> as index to structures, and whose values are not compared against
> upper bounds to prevent speculative out-of-bound accesses. This change
> prevents these speculative accesses.
> 
> Furthermore, variables are initialized and the compiler is asked to not
> optimized these initializations, as the uninitialized variables might be
> used in a speculative out-of-bound access. Out of the four initialized
> variables, two are potentially problematic, namely ones in the functions
> vioapic_irq_positive_edge and vioapic_get_trigger_mode.
> 
> As the two problematic variables are both used in the common function
> gsi_vioapic, the mitigation is implemented there. As the access pattern
> of the currently non-guest-controlled functions might change in the
> future as well, the other variables are initialized as well.
> 
> This is part of the speculative hardening effort.
> 
> Signed-off-by: Norbert Manthey 

Reviewed-by: Jan Beulich 



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

Re: [Xen-devel] [PATCH SpectreV1+L1TF v7 1/9] xen/evtchn: block speculative out-of-bound accesses

[Jan Beulich]

>>> On 21.02.19 at 09:16,  wrote:
> @@ -813,6 +817,7 @@ int set_global_virq_handler(struct domain *d, uint32_t 
> virq)
>  
>  if (virq >= NR_VIRQS)
>  return -EINVAL;
> +
>  if (!virq_is_global(virq))
>  return -EINVAL;
>  

Stray (but benign) change. Easy enough to take out while committing.
Without this:
Reviewed-by: Jan Beulich 

Btw, it would have been nice if you had also dropped the somewhat
misleading SpectreV1 from the subject line tags of the series.

Jan



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

Re: [Xen-devel] [PATCH SpectreV1+L1TF v7 7/9] common/memory: block speculative out-of-bound accesses

[Jan Beulich]

>>> On 21.02.19 at 09:16,  wrote:
> The get_page_from_gfn method returns a pointer to a page that belongs
> to a gfn. Before returning the pointer, the gfn is checked for being
> valid. Under speculation, these checks can be bypassed, so that
> the function get_page is still executed partially. Consequently, the
> function page_get_owner_and_reference might be executed partially as
> well. In this function, the computed pointer is accessed, resulting in
> a speculative out-of-bound address load. As the gfn can be controlled by
> a guest, this access is problematic.
> 
> To mitigate the root cause, an lfence instruction is added via the
> evaluate_nospec macro. To make the protection generic, we do not
> introduce the lfence instruction for this single check, but add it to
> the mfn_valid function. This way, other potentially problematic accesses
> are protected as well.
> 
> This is part of the speculative hardening effort.
> 
> Signed-off-by: Norbert Manthey 

Acked-by: Jan Beulich 



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

Re: [Xen-devel] [PATCH] iommu: leave IOMMU enabled by default during kexec crash transition

[Jan Beulich]

>>> On 22.02.19 at 13:40,  wrote:
> On 22/02/2019 09:52, Jan Beulich wrote:
> On 20.02.19 at 19:19,  wrote:
>>> On 20/02/2019 08:48, Jan Beulich wrote:

 Some entity needs to decide whether to add the respective command
 line option to the crash kernel's command line. It should be this same
 entity to tell Xen whether to keep the IOMMU enabled while invoking
 the crash kernel. I am merely guessing that this entity is the kexec
 tool.

>>>
>>> I was just double checking and it seems (assuming the device reset
>>> correctly in the crash kernel) everything seem to work even without
>>> enabling intel_iommu in the command line - newer kernels handle this
>>> case by explicitly disabling translation in any case: they expect it to
>>> be enabled by the previous kernel.
>> 
>> So if they disable translation, how is them doing so any better than us
>> doing so, other than theirs occurring slightly later on the time scale and
>> hence there being better chances of in-flight (at the time of the crash)
>> DMA having completed meanwhile?
>> 
> 
> There are several reasons why it's better:
> a) kernel is able to perform device reset properly as it has bus
> specific code that does this. There is even a comment in the code
> mentioning that at the moment it disables the translation bus-specific
> reset is finished and it's safer (as devices likely stopped DMA at this
> point) to do it now.
> b) kernel has the drivers that do per-driver-specific reset of the
> devices that do not work well with bus-specific reset. It's simply
> impossible to implement that in Xen
> c) even if a device is uncooperative and keeps sending bus transactions,
> an error event that I mentioned earlier will be properly handled as we
> have facilities for it in the kernel at that point

Okay, c) is a convincing argument. a) and b) are partly only: Iirc
crash kernels don't load unnecessary drivers, so a babbling device
may be left untouched unless generic kernel code can reset or
otherwise silence it.

Jan



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

Re: [Xen-devel] [PATCH] x86/shadow: don't use map_domain_page_global() on paths that may not fail

[Jan Beulich]

>>> On 22.02.19 at 13:33,  wrote:
> At 08:15 -0700 on 20 Feb (1550650529), Jan Beulich wrote:
>> The assumption (according to one comment) and hope (according to
>> another) that map_domain_page_global() can't fail are both wrong on
>> large enough systems. Do away with the guest_vtable field altogether,
>> and establish / tear down the desired mapping as necessary.
>> 
>> The alternatives, discarded as being undesirable, would have been to
>> either crash the guest in sh_update_cr3() when the mapping fails, or to
>> bubble up an error indicator, which upper layers would have a hard time
>> to deal with (other than again by crashing the guest).
>> 
>> Signed-off-by: Jan Beulich 
> 
> I follow your argument, so Acked-by: Tim Deegan 

Thanks.

> I would expect this to have a measurable cost on page fault times (on
> configurations where global map isn't just a directmap).  It would be
> good to know if that's the case.

I have absolutely no doubt that this will cost performance on huge
systems (and debug builds). That's basically why I've explicitly listed
the (discarded) options that would have had less overhead.

Jan



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

Re: [Xen-devel] Reducing or removing direct map from xen (was Re: Ongoing/future speculative mitigation work)

[Jan Beulich]

>>> On 22.02.19 at 13:11,  wrote:
> On Fri, Feb 22, 2019 at 05:06:03AM -0700, Jan Beulich wrote:
>> >>> On 22.02.19 at 12:50,  wrote:
>> > On Fri, Feb 22, 2019 at 04:48:09AM -0700, Jan Beulich wrote:
>> >> >>> On 20.02.19 at 18:08,  wrote:
>> >> > On Wed, Feb 20, 2019 at 01:09:56PM +, Wei Liu wrote:
>> >> > [...]
>> >> >> I think under-allocate-then-map looks plausible. xmalloc will need
>> >> >> to allocate pages, put them into an array and call __vmap on that array
>> >> >> directly.
>> >> > 
>> >> > The biggest issue with this approach is that we now need an array of
>> >> > 1UL<> >> > this is going to be (1UL<<20)*8 bytes long. This is not feasible.
>> >> 
>> >> Are we really calling xmalloc() with any number nearly this big?
>> > 
>> > In practice, I don't think so. What do you think is a sensible limit?
>> 
>> I'm afraid you won't like the answer: Whatever the biggest chunk is
>> we currently allocate anywhere. Perhaps, e.g. if there's a single big
>> "violator", changing some code to reduce the upper bound might be
>> desirable.
>> 
>> In general there shouldn't be any going beyond one page once we've
>> completed booting. Several years back I think I had managed to
>> replace most (all?) higher order xmalloc()-s. So another option might
>> be to allow up to MAX_ORDER by way of some init-only mechanism,
>> and later allow only up to single page chunks.
>> 
> 
> Think about it, if you have done the work to remove high order
> allocations, removing this optimisation is the easiest thing to do and
> wouldn't make things worse, isn't it?

Not sure I understand what exactly you want to remove. Allocating
32 pages when you need 17 is wasteful, and hence I'd prefer if we
could continue to make actual use of the remaining 15. That's
independent of whether the allocation occurs at boot or run time.

Jan



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

Re: [Xen-devel] [PATCH v2] iommu: leave IOMMU enabled by default during kexec crash transition

[Igor Druzhinin]

On 22/02/2019 12:34, Jan Beulich wrote:
 On 21.02.19 at 23:08,  wrote:
>> Modern Linux kernels taught to copy all the necessary DMAR/IR tables
>> following kexec from the previous kernel (Xen in our case) - so it's
>> currently normal to keep IOMMU enabled. It might require minor changes to
>> kdump command line that enables IOMMU drivers (e.g. intel_iommu=on /
>> intremap=on) but recent kernels don't require any additional changes for
>> the transition to be transparent.
> 
> In your reply on the v1 thread didn't you say Linux disables translation?

I didn't rule out a possibility that there were versions of Linux that
did it in a wrong order or didn't do it at all as current versions do.

Igor

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

Re: [Xen-devel] [PATCH] iommu: leave IOMMU enabled by default during kexec crash transition

[Igor Druzhinin]

On 22/02/2019 09:52, Jan Beulich wrote:
 On 20.02.19 at 19:19,  wrote:
>> On 20/02/2019 08:48, Jan Beulich wrote:
>>>
>>> Some entity needs to decide whether to add the respective command
>>> line option to the crash kernel's command line. It should be this same
>>> entity to tell Xen whether to keep the IOMMU enabled while invoking
>>> the crash kernel. I am merely guessing that this entity is the kexec
>>> tool.
>>>
>>
>> I was just double checking and it seems (assuming the device reset
>> correctly in the crash kernel) everything seem to work even without
>> enabling intel_iommu in the command line - newer kernels handle this
>> case by explicitly disabling translation in any case: they expect it to
>> be enabled by the previous kernel.
> 
> So if they disable translation, how is them doing so any better than us
> doing so, other than theirs occurring slightly later on the time scale and
> hence there being better chances of in-flight (at the time of the crash)
> DMA having completed meanwhile?
> 

There are several reasons why it's better:
a) kernel is able to perform device reset properly as it has bus
specific code that does this. There is even a comment in the code
mentioning that at the moment it disables the translation bus-specific
reset is finished and it's safer (as devices likely stopped DMA at this
point) to do it now.
b) kernel has the drivers that do per-driver-specific reset of the
devices that do not work well with bus-specific reset. It's simply
impossible to implement that in Xen
c) even if a device is uncooperative and keeps sending bus transactions,
an error event that I mentioned earlier will be properly handled as we
have facilities for it in the kernel at that point

Igor

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

Re: [Xen-devel] [PATCH for-4.12] x86: Improve the efficiency of domain_relinquish_resources()

[Jan Beulich]

>>> On 21.02.19 at 14:31,  wrote:
> On Thu, Feb 21, 2019 at 12:22:13PM +, Andrew Cooper wrote:
>> pci_release_devices() takes the global PCI lock.  Once pci_release_devices()
>> has completed, it will be called redundantly each time paging_teardown() and
>> vcpu_destroy_pagetables() continue.
>> 
>> This is liable to be millions of times for a reasonably sized guest, and is a
>> serialising bottleneck now that domain_kill() can be run concurrently on
>> different domains.
>> 
>> Instead of propagating the opencoding of the relinquish state machine, take
>> the opportunity to clean it up.
>> 
>> Leave a proper set of comments explaining that domain_relinquish_resources()
>> implements a co-routine.  Introduce a documented PROGRESS() macro to avoid
>> latent bugs such as the RELMEM_xen case, and make the new PROG_* states
>> private to domain_relinquish_resources().
>> 
>> Signed-off-by: Andrew Cooper 
> 
> Reviewed-by: Wei Liu 

Acked-by: Jan Beulich 



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

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Oleksandr Andrushchenko]

On 2/20/19 10:46 PM, Julien Grall wrote:

(+ Andrew and Jan for feedback on the event channel interrupt)

Hi Boris,

Thank you for the your feedback.

On 2/20/19 8:04 PM, Boris Ostrovsky wrote:

On 2/20/19 1:05 PM, Julien Grall wrote:

Hi,

On 20/02/2019 17:07, Boris Ostrovsky wrote:

On 2/20/19 9:15 AM, Julien Grall wrote:

Hi Boris,

Thank you for your answer.

On 20/02/2019 00:02, Boris Ostrovsky wrote:

On Tue, Feb 19, 2019 at 05:31:10PM +, Julien Grall wrote:

Hi all,

I have been looking at using Linux RT in Dom0. Once the guest is
started,
the console is ending to have a lot of warning (see trace below).

After some investigation, this is because the irq handler will now
be threaded.
I can reproduce the same error with the vanilla Linux when passing
the option
'threadirqs' on the command line (the trace below is from 5.0.0-rc7
that has
not RT support).

FWIW, the interrupt for port 6 is used to for the guest to
communicate with
xenstore.

   From my understanding, this is happening because the interrupt
handler is now
run in a thread. So we can have the following happening.

  Interrupt context    | Interrupt thread
   |
  receive interrupt port 6 |
  clear the evtchn port    |
  set IRQF_RUNTHREAD    |
  kick interrupt thread    |
   |    clear IRQF_RUNTHREAD
   |    call evtchn_interrupt
  receive interrupt port 6 |
  clear the evtchn port    |
  set IRQF_RUNTHREAD   |
  kick interrupt thread    |
   |    disable interrupt port 6
   | evtchn->enabled = false
   |    []
   |
   |    *** Handling the second
interrupt ***
   |    clear IRQF_RUNTHREAD
   |    call evtchn_interrupt
   |    WARN(...)

I am not entirely sure how to fix this. I have two solutions in 
mind:


1) Prevent the interrupt handler to be threaded. We would also
need to
switch from spin_lock to raw_spin_lock as the former may sleep on
RT-Linux.

2) Remove the warning


I think access to evtchn->enabled is racy so (with or without the
warning) we can't use it reliably.


Thinking about it, it would not be the only issue. The ring is sized
to contain only one instance of the same event. So if you receive
twice the event, you may overflow the ring.


Hm... That's another argument in favor of "unthreading" the handler.


I first thought it would be possible to unthread it. However,
wake_up_interruptible is using a spin_lock. On RT spin_lock can sleep,
so this cannot be used in an interrupt context.

So I think "unthreading" the handler is not an option here.


That sounds like a different problem. I.e. there are two issues:
* threaded interrupts don't work properly (races, ring overflow)
* evtchn_interrupt() (threaded or not) has spin_lock(), which is not
going to work for RT


I am afraid that's not correct, you can use spin_lock() in threaded 
interrupt handler.



The first can be fixed by using non-threaded handlers.


The two are somewhat related, if you use a non-threaded handler then 
you are not going to help the RT case.


In general, the unthreaded solution should be used in the last resort.









Another alternative could be to queue the irq if !evtchn->enabled 
and

handle it in evtchn_write() (which is where irq is supposed to be
re-enabled).

What do you mean by queue? Is it queueing in the ring?



No, I was thinking about having a new structure for deferred 
interrupts.


Hmmm, I am not entirely sure what would be the structure here. Could
you expand your thinking?


Some sort of a FIFO that stores {irq, data} tuple. It could obviously be
implemented as a ring but not necessarily as Xen shared ring (if that's
what you were referring to).


The underlying question is what happen if you miss an interrupt. Is it 
going to be ok? If no, then we have to record everything and can't 
kill/send an error to the user app because it is not its fault.


This means a FIFO would not be a viable. How do you size it? Static 
(i.e pre-defined) size is not going to be possible because you don't 
know how many interrupt you are going to receive before the thread 
handler runs. You can't make it grow dynamically as it make become 
quite big for the same reason.


Discussing with my team, a solution that came up would be to introduce 
one atomic field per event to record the number of event received. I 
will explore that solution tomorrow.

How will this help if events have some payload?


Cheers,




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

Re: [Xen-devel] [PATCH v2] iommu: leave IOMMU enabled by default during kexec crash transition

[Jan Beulich]

>>> On 21.02.19 at 23:08,  wrote:
> Modern Linux kernels taught to copy all the necessary DMAR/IR tables
> following kexec from the previous kernel (Xen in our case) - so it's
> currently normal to keep IOMMU enabled. It might require minor changes to
> kdump command line that enables IOMMU drivers (e.g. intel_iommu=on /
> intremap=on) but recent kernels don't require any additional changes for
> the transition to be transparent.

In your reply on the v1 thread didn't you say Linux disables translation?

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -35,6 +35,7 @@ bool_t __read_mostly iommu_igfx = 1;
>  bool_t __read_mostly iommu_snoop = 1;
>  bool_t __read_mostly iommu_qinval = 1;
>  bool_t __read_mostly iommu_intremap = 1;
> +bool_t __read_mostly iommu_crash_disable;
>  
>  static bool __hwdom_initdata iommu_hwdom_none;
>  bool __hwdom_initdata iommu_hwdom_strict;

While I can see that all upper context still uses bool_t, lower context
already suggests to better use bool. An easy to make adjustment
while committing if no other need arises for a v3.

Jan



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

Re: [Xen-devel] [PATCH] x86/shadow: don't use map_domain_page_global() on paths that may not fail

[Tim Deegan]

At 08:15 -0700 on 20 Feb (1550650529), Jan Beulich wrote:
> The assumption (according to one comment) and hope (according to
> another) that map_domain_page_global() can't fail are both wrong on
> large enough systems. Do away with the guest_vtable field altogether,
> and establish / tear down the desired mapping as necessary.
> 
> The alternatives, discarded as being undesirable, would have been to
> either crash the guest in sh_update_cr3() when the mapping fails, or to
> bubble up an error indicator, which upper layers would have a hard time
> to deal with (other than again by crashing the guest).
> 
> Signed-off-by: Jan Beulich 

I follow your argument, so Acked-by: Tim Deegan 

I would expect this to have a measurable cost on page fault times (on
configurations where global map isn't just a directmap).  It would be
good to know if that's the case.

Cheers,

Tim.


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

Re: [Xen-devel] [PATCH v2 3/4] x86/vmx: Fix security issue when a guest balloons out the #VE info page

[Jan Beulich]

>>> On 21.02.19 at 21:18,  wrote:
> The logic in altp2m_vcpu_{en,dis}able_ve() and vmx_vcpu_update_vmfunc_ve() is
> dangerous.  After #VE has been set up, the guest can balloon out and free the
> nominated GFN, after which the processor may write to it.  Also, the unlocked
> GFN query means the MFN is stale by the time it is used.  Alternatively, a
> guest can race two disable calls to cause one VMCS to still reference the
> nominated GFN after the tracking information was dropped.
> 
> Rework the logic from scratch to make it safe.
> 
> Hold an extra page reference on the underlying frame, to account for the
> VMCS's reference.  This means that if the GFN gets ballooned out, it isn't
> freed back to Xen until #VE is disabled, and the VMCS no longer refers to the
> page.
> 
> A consequence of this is that altp2m_vcpu_disable_ve() needs to be called
> during the domain_kill() path, to drop the reference for domains which shut
> down with #VE still enabled.
> 
> For domains using altp2m, we expect a single enable call and no disable for
> the remaining lifetime of the domain.  However, to avoid problems with
> concurrent calls, use cmpxchg() to locklessly maintain safety.
> 
> This doesn't have an XSA because altp2m is not yet a security-supported
> feature.
> 
> Signed-off-by: Andrew Cooper 
> Release-acked-by: Juergen Gross 

Reviewed-by: Jan Beulich 

I would appreciate though if you could add half a sentence clarifying
that and why you decided against obtaining a writable type-ref.

Also - I take it that altp2m and migration don't work together?
Otherwise wouldn't you need to mark the page dirty in more
cases?

Jan



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

Re: [Xen-devel] Reducing or removing direct map from xen (was Re: Ongoing/future speculative mitigation work)

[Wei Liu]

On Fri, Feb 22, 2019 at 05:06:03AM -0700, Jan Beulich wrote:
> >>> On 22.02.19 at 12:50,  wrote:
> > On Fri, Feb 22, 2019 at 04:48:09AM -0700, Jan Beulich wrote:
> >> >>> On 20.02.19 at 18:08,  wrote:
> >> > On Wed, Feb 20, 2019 at 01:09:56PM +, Wei Liu wrote:
> >> > [...]
> >> >> I think under-allocate-then-map looks plausible. xmalloc will need
> >> >> to allocate pages, put them into an array and call __vmap on that array
> >> >> directly.
> >> > 
> >> > The biggest issue with this approach is that we now need an array of
> >> > 1UL< >> > this is going to be (1UL<<20)*8 bytes long. This is not feasible.
> >> 
> >> Are we really calling xmalloc() with any number nearly this big?
> > 
> > In practice, I don't think so. What do you think is a sensible limit?
> 
> I'm afraid you won't like the answer: Whatever the biggest chunk is
> we currently allocate anywhere. Perhaps, e.g. if there's a single big
> "violator", changing some code to reduce the upper bound might be
> desirable.
> 
> In general there shouldn't be any going beyond one page once we've
> completed booting. Several years back I think I had managed to
> replace most (all?) higher order xmalloc()-s. So another option might
> be to allow up to MAX_ORDER by way of some init-only mechanism,
> and later allow only up to single page chunks.
> 

Think about it, if you have done the work to remove high order
allocations, removing this optimisation is the easiest thing to do and
wouldn't make things worse, isn't it?

Wei.

> Jan
> 
> 

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

Re: [Xen-devel] Reducing or removing direct map from xen (was Re: Ongoing/future speculative mitigation work)

[Jan Beulich]

>>> On 22.02.19 at 12:50,  wrote:
> On Fri, Feb 22, 2019 at 04:48:09AM -0700, Jan Beulich wrote:
>> >>> On 20.02.19 at 18:08,  wrote:
>> > On Wed, Feb 20, 2019 at 01:09:56PM +, Wei Liu wrote:
>> > [...]
>> >> I think under-allocate-then-map looks plausible. xmalloc will need
>> >> to allocate pages, put them into an array and call __vmap on that array
>> >> directly.
>> > 
>> > The biggest issue with this approach is that we now need an array of
>> > 1UL<> > this is going to be (1UL<<20)*8 bytes long. This is not feasible.
>> 
>> Are we really calling xmalloc() with any number nearly this big?
> 
> In practice, I don't think so. What do you think is a sensible limit?

I'm afraid you won't like the answer: Whatever the biggest chunk is
we currently allocate anywhere. Perhaps, e.g. if there's a single big
"violator", changing some code to reduce the upper bound might be
desirable.

In general there shouldn't be any going beyond one page once we've
completed booting. Several years back I think I had managed to
replace most (all?) higher order xmalloc()-s. So another option might
be to allow up to MAX_ORDER by way of some init-only mechanism,
and later allow only up to single page chunks.

Jan



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

Re: [Xen-devel] [PATCH] tools: add link path flag for local build to pkg-config files

[Juergen Gross]

On 22/02/2019 12:52, Wei Liu wrote:
> On Thu, Feb 21, 2019 at 06:36:13PM +0100, Juergen Gross wrote:
>> The qemu build process is requiring the link path of Xen libraries
>> to be specified both with -L and -Wl,-rpath-link. Add the -L flag
>> to the local pkg-config files.
>>
>> At the same time let the pkg-config files depend on the Makefile
>> creating them, too.
>>
>> Signed-off-by: Juergen Gross 
> 
> Juergen, do you want this in 4.12?

Hmm, why? Does it repair any build error in 4.12? I don't think so. It
will help for further development (e.g. removing the explicit include
paths and libraries added to the qemu configure call), but this won't
happen in 4.12.


Juergen

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

Re: [Xen-devel] [PATCH] tools: add link path flag for local build to pkg-config files

[Wei Liu]

On Fri, Feb 22, 2019 at 01:02:51PM +0100, Juergen Gross wrote:
> On 22/02/2019 12:52, Wei Liu wrote:
> > On Thu, Feb 21, 2019 at 06:36:13PM +0100, Juergen Gross wrote:
> >> The qemu build process is requiring the link path of Xen libraries
> >> to be specified both with -L and -Wl,-rpath-link. Add the -L flag
> >> to the local pkg-config files.
> >>
> >> At the same time let the pkg-config files depend on the Makefile
> >> creating them, too.
> >>
> >> Signed-off-by: Juergen Gross 
> > 
> > Juergen, do you want this in 4.12?
> 
> Hmm, why? Does it repair any build error in 4.12? I don't think so. It
> will help for further development (e.g. removing the explicit include
> paths and libraries added to the qemu configure call), but this won't
> happen in 4.12.

Thanks for confirming.

Wei.

> 
> 
> Juergen

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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/22/19 1:27 PM, Julien Grall wrote:

Hi Oleksandr,

On 22/02/2019 11:13, Oleksandr Andrushchenko wrote:

On 2/22/19 1:05 PM, Julien Grall wrote:

Hi,

On 22/02/2019 10:27, Andrew Cooper wrote:

On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

Hello, everybody!

We at EPAM Systems would like to present first series of patches 
targeting Xen
on ARM Functional Safety certification (ISO61508 based): 
implementation of
MISRA [1] C:2012 Rule 16.4 which requires that every switch 
statement has a

default label as a measure of defensive programming technique.


Hang on - what?

Can someone attempt to justify why actively breaking -Wswitch is going
to result in safer/better code?


I was about to ask the same. There are quite a few cases where this 
series is going to make more difficult extending enum.


Well, I am not sure I can truly defend MISRA requirements here, but 
I'll try

to express my view on that.

Let us have a look at gcc's options [1]: so, for what you are saying:
"-Wswitch
Warn whenever a switch statement has an index of enumerated type and 
lacks a case for one or more of the named codes of that enumeration. 
(The presence of a default label prevents this warning.) case labels 
outside the enumeration range also provoke warnings when this option 
is used (even if there is a default label). This warning is enabled 
by -Wall."


So, if we do not have all the cases covered then this compiler's 
switch will fire
a warning (error). What if we have an integer as a switch's index, 
not an enumeration,
so then there is no easy way to handle all the cases and we have to 
provide *default* statement.


You have a point for the integer switch.



What if with time enumeration changes, what if some of the case 
statements get removed and so on.


In that case, the compiler will throw an error (Xen is built with 
-Werror). So for enumeration the compiler will help us to spot the 
missing places.


If you add a default case, you remove us a good way to check we 
actually add the new element everywhere.
The answer is that what happens if we by any reason either by a mistake 
or any other mean
have a build which doesn't have -Wswitch or -Wall. I mean that 
everything changes and having
"default" in the source does guarantee the handling as it was intended 
to be.




All these, as per my understanding, lead to a defensive programming 
approach, e.g. do not rely on what
is here at the moment, but think that everything can change any time 
soon.
It does not mean this is always the best solution. If we take the 
example of the enumeration, now you can't easily detect at compilation 
time you missed a case. This could potentially lead to painful 
debugging session to understand what is missing.


But what happens if you miss default handling and because of bugs in the 
code you do not

handle "compile time unexpected values"?


BTW, I checked the series with -Wswitch-default:
-Wswitch-default
Warn whenever a switch statement does not have a default case.
Furthermore, using BUG() is a pretty bad idea in switch. 
It is and not only in the switch. The reason I put BUG is that I 
tried to follow

the existing "error handling" at those places.


It is not because BUG() is been used today in some places that we need 
to continue to spread it.


Use of BUG() itself is another topic which will also need to be 
addressed


So we should not add more of them...
Again, I see this as a dedicated change. So, in the current series I 
think it is

acceptable to use the existing way of error handling if any at all.


Cheers,




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

Re: [Xen-devel] [PATCH] tools: add link path flag for local build to pkg-config files

[Wei Liu]

On Thu, Feb 21, 2019 at 06:36:13PM +0100, Juergen Gross wrote:
> The qemu build process is requiring the link path of Xen libraries
> to be specified both with -L and -Wl,-rpath-link. Add the -L flag
> to the local pkg-config files.
> 
> At the same time let the pkg-config files depend on the Makefile
> creating them, too.
> 
> Signed-off-by: Juergen Gross 

Juergen, do you want this in 4.12?

Wei.

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

Re: [Xen-devel] Reducing or removing direct map from xen (was Re: Ongoing/future speculative mitigation work)

[Wei Liu]

On Fri, Feb 22, 2019 at 04:48:09AM -0700, Jan Beulich wrote:
> >>> On 20.02.19 at 18:08,  wrote:
> > On Wed, Feb 20, 2019 at 01:09:56PM +, Wei Liu wrote:
> > [...]
> >> I think under-allocate-then-map looks plausible. xmalloc will need
> >> to allocate pages, put them into an array and call __vmap on that array
> >> directly.
> > 
> > The biggest issue with this approach is that we now need an array of
> > 1UL< > this is going to be (1UL<<20)*8 bytes long. This is not feasible.
> 
> Are we really calling xmalloc() with any number nearly this big?

In practice, I don't think so. What do you think is a sensible limit?

Wei.

> 
> Jan
> 
> 

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

Re: [Xen-devel] [PATCH v2] tools/xentop: Display '-' when stats are not available.

[Wei Liu]

On Fri, Feb 22, 2019 at 11:48:06AM +, Ronan Abhamon wrote:
> From: Pritha Srivastava 
> 
> Displaying 0 is misleading.
> 
> Signed-off-by: Pritha Srivastava 
> Signed-off-by: Ronan Abhamon 

Acked-by: Wei Liu 

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

[Xen-devel] [PATCH v2] tools/xentop: Display '-' when stats are not available.

[Ronan Abhamon]

From: Pritha Srivastava 

Displaying 0 is misleading.

Signed-off-by: Pritha Srivastava 
Signed-off-by: Ronan Abhamon 
---
 tools/xenstat/libxenstat/src/xenstat.c   |   6 +
 tools/xenstat/libxenstat/src/xenstat.h   |   5 +
 tools/xenstat/libxenstat/src/xenstat_linux.c |  47 +++---
 tools/xenstat/libxenstat/src/xenstat_priv.h  |   1 +
 tools/xenstat/xentop/xentop.c| 161 ++-
 5 files changed, 158 insertions(+), 62 deletions(-)

diff --git a/tools/xenstat/libxenstat/src/xenstat.c 
b/tools/xenstat/libxenstat/src/xenstat.c
index fbe44f3c56..8b856b32a3 100644
--- a/tools/xenstat/libxenstat/src/xenstat.c
+++ b/tools/xenstat/libxenstat/src/xenstat.c
@@ -729,6 +729,12 @@ unsigned long long xenstat_vbd_wr_sects(xenstat_vbd * vbd)
return vbd->wr_sects;
 }
 
+/* Returns error while getting stats (1 if error happened, 0 otherwise) */
+bool xenstat_vbd_error(xenstat_vbd * vbd)
+{
+   return vbd->error;
+}
+
 /*
  * Tmem functions
  */
diff --git a/tools/xenstat/libxenstat/src/xenstat.h 
b/tools/xenstat/libxenstat/src/xenstat.h
index 47ec60e14d..9f5053dd92 100644
--- a/tools/xenstat/libxenstat/src/xenstat.h
+++ b/tools/xenstat/libxenstat/src/xenstat.h
@@ -20,6 +20,8 @@
 #ifndef XENSTAT_H
 #define XENSTAT_H
 
+#include 
+
 /* Opaque handles */
 typedef struct xenstat_handle xenstat_handle;
 typedef struct xenstat_domain xenstat_domain;
@@ -193,6 +195,9 @@ unsigned long long xenstat_vbd_wr_reqs(xenstat_vbd * vbd);
 unsigned long long xenstat_vbd_rd_sects(xenstat_vbd * vbd);
 unsigned long long xenstat_vbd_wr_sects(xenstat_vbd * vbd);
 
+/* Returns error while getting stats (1 if error happened, 0 otherwise) */
+bool xenstat_vbd_error(xenstat_vbd * vbd);
+
 /*
  * Tmem functions - extract tmem information
  */
diff --git a/tools/xenstat/libxenstat/src/xenstat_linux.c 
b/tools/xenstat/libxenstat/src/xenstat_linux.c
index 7cdd3bf91f..9421ca43c8 100644
--- a/tools/xenstat/libxenstat/src/xenstat_linux.c
+++ b/tools/xenstat/libxenstat/src/xenstat_linux.c
@@ -436,13 +436,15 @@ int xenstat_collect_vbds(xenstat_node * node)
ret = sscanf(dp->d_name, "%3s-%u-%u", buf, , );
if (ret != 3)
continue;
+   if (!(strstr(buf, "vbd")) && !(strstr(buf, "tap")))
+   continue;
 
if (strcmp(buf,"vbd") == 0)
vbd.back_type = 1;
else if (strcmp(buf,"tap") == 0)
vbd.back_type = 2;
else
-   continue;
+   vbd.back_type = 0;
 
domain = xenstat_node_domain(node, domid);
if (domain == NULL) {
@@ -453,36 +455,29 @@ int xenstat_collect_vbds(xenstat_node * node)
continue;
}
 
-   if((read_attributes_vbd(dp->d_name, "statistics/oo_req", buf, 
256)<=0)
-  || ((ret = sscanf(buf, "%llu", _reqs)) != 1))
-   {
-   continue;
-   }
-
-   if((read_attributes_vbd(dp->d_name, "statistics/rd_req", buf, 
256)<=0)
-  || ((ret = sscanf(buf, "%llu", _reqs)) != 1))
+   if (vbd.back_type == 1 || vbd.back_type == 2)
{
-   continue;
-   }
 
-   if((read_attributes_vbd(dp->d_name, "statistics/wr_req", buf, 
256)<=0)
-  || ((ret = sscanf(buf, "%llu", _reqs)) != 1))
-   {
-   continue;
-   }
-
-   if((read_attributes_vbd(dp->d_name, "statistics/rd_sect", buf, 
256)<=0)
-  || ((ret = sscanf(buf, "%llu", _sects)) != 1))
-   {
-   continue;
+   vbd.error = 0;
+
+   if ((read_attributes_vbd(dp->d_name, 
"statistics/oo_req", buf, 256)<=0) ||
+   ((ret = sscanf(buf, "%llu", _reqs)) != 
1) ||
+   (read_attributes_vbd(dp->d_name, 
"statistics/rd_req", buf, 256)<=0) ||
+   ((ret = sscanf(buf, "%llu", _reqs)) != 
1) ||
+   (read_attributes_vbd(dp->d_name, 
"statistics/wr_req", buf, 256)<=0) ||
+   ((ret = sscanf(buf, "%llu", _reqs)) != 
1) ||
+   (read_attributes_vbd(dp->d_name, 
"statistics/rd_sect", buf, 256)<=0) ||
+   ((ret = sscanf(buf, "%llu", _sects)) != 
1) ||
+   (read_attributes_vbd(dp->d_name, 
"statistics/wr_sect", buf, 256)<=0) ||
+   ((ret = sscanf(buf, "%llu", _sects)) != 
1))
+   {
+   vbd.error = 1;
+   }
}
-
-   if((read_attributes_vbd(dp->d_name, "statistics/wr_sect", buf, 
256)<=0)
-  || ((ret = sscanf(buf, "%llu", _sects)) != 1))
+   

Re: [Xen-devel] Reducing or removing direct map from xen (was Re: Ongoing/future speculative mitigation work)

[Jan Beulich]

>>> On 20.02.19 at 18:08,  wrote:
> On Wed, Feb 20, 2019 at 01:09:56PM +, Wei Liu wrote:
> [...]
>> I think under-allocate-then-map looks plausible. xmalloc will need
>> to allocate pages, put them into an array and call __vmap on that array
>> directly.
> 
> The biggest issue with this approach is that we now need an array of
> 1UL< this is going to be (1UL<<20)*8 bytes long. This is not feasible.

Are we really calling xmalloc() with any number nearly this big?

Jan



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

Re: [Xen-devel] xen/evtchn and forced threaded irq

[Jan Beulich]

>>> On 20.02.19 at 23:03,  wrote:
> On 2/20/19 9:46 PM, Boris Ostrovsky wrote:
>> On 2/20/19 3:46 PM, Julien Grall wrote:
>>> On 2/20/19 8:04 PM, Boris Ostrovsky wrote:
 On 2/20/19 1:05 PM, Julien Grall wrote:
 Some sort of a FIFO that stores {irq, data} tuple. It could obviously be
 implemented as a ring but not necessarily as Xen shared ring (if that's
 what you were referring to).

I'm sort of lost here with the mixed references to "interrupts" and
"events". Interrupts can be shared (and have a per-instance
(irq,data) tuple), but there should be at most one interrupt
underlying the event channel systems, shouldn't there? Event
channels otoh can't be shared, and hence there's only one
"data" item to be associated with each channel, at which point a
plain counter ought to do.

>>> The underlying question is what happen if you miss an interrupt. Is it
>>> going to be ok?
>> 
>> This I am not sure about. I thought yes since we are signaling the
>> process only once.
> 
> I have CCed Andrew and Jan to see if they can help here.

What meaning of "miss" do you use here? As long as is only means
delay (i.e. miss an instance, but get notified as soon as feasible
even if there is not further event coming from the source) - that
would be okay, I think. But if you truly mean "miss" (i.e. event lost
altogether, with silence resulting if there's no further event), then
no, this would not be tolerable.

Jan



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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

On 22/02/2019 11:30, Julien Grall wrote:



On 22/02/2019 11:21, Andrii Anisov wrote:

On 22.02.19 12:27, Andrew Cooper wrote:

On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

Hello, everybody!

We at EPAM Systems would like to present first series of patches targeting Xen
on ARM Functional Safety certification (ISO61508 based): implementation of
MISRA [1] C:2012 Rule 16.4 which requires that every switch statement has a
default label as a measure of defensive programming technique.


Hang on - what?

Can someone attempt to justify why actively breaking -Wswitch is going
to result in safer/better code?


I would express my vision of that MISRA rule requirement:
It requires handling (in any meaning) all possible incoming values explicitly.
Switching through enum still could be guarded by BUG/ASSERT_UNREACHABLE under 
default label. Though at runtime, not compilation time.


While review tend to be very thorough, it is sometimes hard to spot when we miss 
a case. This is where -Wswitch comes into place to spot missing how.


s/how/case/



How the BUG/ASSERT_UNREACHABLE solution is going to help us here?

Cheers,



--
Julien Grall

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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

On 22/02/2019 11:21, Andrii Anisov wrote:

On 22.02.19 12:27, Andrew Cooper wrote:

On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

Hello, everybody!

We at EPAM Systems would like to present first series of patches targeting Xen
on ARM Functional Safety certification (ISO61508 based): implementation of
MISRA [1] C:2012 Rule 16.4 which requires that every switch statement has a
default label as a measure of defensive programming technique.


Hang on - what?

Can someone attempt to justify why actively breaking -Wswitch is going
to result in safer/better code?


I would express my vision of that MISRA rule requirement:
It requires handling (in any meaning) all possible incoming values explicitly.
Switching through enum still could be guarded by BUG/ASSERT_UNREACHABLE under 
default label. Though at runtime, not compilation time.


While review tend to be very thorough, it is sometimes hard to spot when we miss 
a case. This is where -Wswitch comes into place to spot missing how.


How the BUG/ASSERT_UNREACHABLE solution is going to help us here?

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi Oleksandr,

On 22/02/2019 11:13, Oleksandr Andrushchenko wrote:

On 2/22/19 1:05 PM, Julien Grall wrote:

Hi,

On 22/02/2019 10:27, Andrew Cooper wrote:

On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

Hello, everybody!

We at EPAM Systems would like to present first series of patches targeting Xen
on ARM Functional Safety certification (ISO61508 based): implementation of
MISRA [1] C:2012 Rule 16.4 which requires that every switch statement has a
default label as a measure of defensive programming technique.


Hang on - what?

Can someone attempt to justify why actively breaking -Wswitch is going
to result in safer/better code?


I was about to ask the same. There are quite a few cases where this series is 
going to make more difficult extending enum.



Well, I am not sure I can truly defend MISRA requirements here, but I'll try
to express my view on that.

Let us have a look at gcc's options [1]: so, for what you are saying:
"-Wswitch
Warn whenever a switch statement has an index of enumerated type and lacks a 
case for one or more of the named codes of that enumeration. (The presence of a 
default label prevents this warning.) case labels outside the enumeration range 
also provoke warnings when this option is used (even if there is a default 
label). This warning is enabled by -Wall."


So, if we do not have all the cases covered then this compiler's switch will 
fire
a warning (error). What if we have an integer as a switch's index, not an 
enumeration,
so then there is no easy way to handle all the cases and we have to provide 
*default* statement.


You have a point for the integer switch.



What if with time enumeration changes, what if some of the case statements get 
removed and so on.


In that case, the compiler will throw an error (Xen is built with -Werror). So 
for enumeration the compiler will help us to spot the missing places.


If you add a default case, you remove us a good way to check we actually add the 
new element everywhere.




All these, as per my understanding, lead to a defensive programming approach, 
e.g. do not rely on what

is here at the moment, but think that everything can change any time soon.
It does not mean this is always the best solution. If we take the example of the 
enumeration, now you can't easily detect at compilation time you missed a case. 
This could potentially lead to painful debugging session to understand what is 
missing.




BTW, I checked the series with -Wswitch-default:
-Wswitch-default
Warn whenever a switch statement does not have a default case.
Furthermore, using BUG() is a pretty bad idea in switch. 

It is and not only in the switch. The reason I put BUG is that I tried to follow
the existing "error handling" at those places.


It is not because BUG() is been used today in some places that we need to 
continue to spread it.



Use of BUG() itself is another topic which will also need to be addressed


So we should not add more of them...

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Andrii Anisov]

Hello Andrew,

On 22.02.19 12:27, Andrew Cooper wrote:

On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

Hello, everybody!

We at EPAM Systems would like to present first series of patches targeting Xen
on ARM Functional Safety certification (ISO61508 based): implementation of
MISRA [1] C:2012 Rule 16.4 which requires that every switch statement has a
default label as a measure of defensive programming technique.


Hang on - what?

Can someone attempt to justify why actively breaking -Wswitch is going
to result in safer/better code?


I would express my vision of that MISRA rule requirement:
It requires handling (in any meaning) all possible incoming values explicitly.
Switching through enum still could be guarded by BUG/ASSERT_UNREACHABLE under 
default label. Though at runtime, not compilation time.

--
Sincerely,
Andrii Anisov.

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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Andrii Anisov]

Hello Andrew,

On 22.02.19 12:27, Andrew Cooper wrote:

Can someone attempt to justify why actively breaking -Wswitch is going
to result in safer/better code?

I would express my vision of that MISRA rule requirement:
It requires handling (in any meaning) all possible incoming values explicitly.
Switching through enum still could be guarded by BUG/ASSERT_UNREACHABLE under 
default label. Though at runtime, not compilation time.

--
Sincerely,
Andrii Anisov.

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

Re: [Xen-devel] About Porting Virtio to the XEN

[Julien Grall]

On 22/02/2019 01:37, chengyan wrote:

Dear Wei:


Hello,



  Now,  I only make a demo in x86 platform and it is just a try.

  Not sure that whether it is successful using Virtio tech in the 
XEN project.


Not all virtio drivers has a Xen counterpart. So it depends what you are looking 
at for your demo.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/22/19 1:05 PM, Julien Grall wrote:

Hi,

On 22/02/2019 10:27, Andrew Cooper wrote:

On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

Hello, everybody!

We at EPAM Systems would like to present first series of patches 
targeting Xen
on ARM Functional Safety certification (ISO61508 based): 
implementation of
MISRA [1] C:2012 Rule 16.4 which requires that every switch 
statement has a

default label as a measure of defensive programming technique.


Hang on - what?

Can someone attempt to justify why actively breaking -Wswitch is going
to result in safer/better code?


I was about to ask the same. There are quite a few cases where this 
series is going to make more difficult extending enum.



Well, I am not sure I can truly defend MISRA requirements here, but I'll try
to express my view on that.

Let us have a look at gcc's options [1]: so, for what you are saying:
"-Wswitch
Warn whenever a switch statement has an index of enumerated type and 
lacks a case for one or more of the named codes of that enumeration. 
(The presence of a default label prevents this warning.) case labels 
outside the enumeration range also provoke warnings when this option is 
used (even if there is a default label). This warning is enabled by -Wall."


So, if we do not have all the cases covered then this compiler's switch 
will fire
a warning (error). What if we have an integer as a switch's index, not 
an enumeration,
so then there is no easy way to handle all the cases and we have to 
provide *default* statement.


What if with time enumeration changes, what if some of the case 
statements get removed and so on.


All these, as per my understanding, lead to a defensive programming 
approach, e.g. do not rely on what

is here at the moment, but think that everything can change any time soon.

BTW, I checked the series with -Wswitch-default:
-Wswitch-default
Warn whenever a switch statement does not have a default case.
Furthermore, using BUG() is a pretty bad idea in switch. 
It is and not only in the switch. The reason I put BUG is that I tried 
to follow

the existing "error handling" at those places.
Use of BUG() itself is another topic which will also need to be addressed
A guest would be able to crash the whole platform if there was a 
coding mistake. Instead we should use ASSERT_UNREACHABLE() and provide 
proper fallback whenever it is possible.
Exactly, as I said we have to get rid of BUG() and friends and handle 
errors instead


Cheers,


Thank you,
Oleksandr

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

Re: [Xen-devel] [PATCH v4.1 4/6] xen/x86: Allow stubdom access to irq created for msi.

[Jan Beulich]

>>> On 22.02.19 at 11:42,  wrote:
> Jan do you know whether pci_remove_device is supposed to be used
> against devices assigned to a domain different than the hardware
> domain?

No, I don't think it ought to be used on any other devices. I guess
the omission of the check goes back to assuming sane Dom0
behavior, properly de-assigning devices before hot-unplugging
them, and hence before invoking the respective physdev-ops.

Jan



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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 22/02/2019 10:27, Andrew Cooper wrote:

On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

Hello, everybody!

We at EPAM Systems would like to present first series of patches targeting Xen
on ARM Functional Safety certification (ISO61508 based): implementation of
MISRA [1] C:2012 Rule 16.4 which requires that every switch statement has a
default label as a measure of defensive programming technique.


Hang on - what?

Can someone attempt to justify why actively breaking -Wswitch is going
to result in safer/better code?


I was about to ask the same. There are quite a few cases where this series is 
going to make more difficult extending enum.


Furthermore, using BUG() is a pretty bad idea in switch. A guest would be able 
to crash the whole platform if there was a coding mistake. Instead we should use 
ASSERT_UNREACHABLE() and provide proper fallback whenever it is possible.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH] tools/xentop: Display '-' when stats are not available.

[Wei Liu]

On Fri, Feb 22, 2019 at 10:47:24AM +, Ronan Abhamon wrote:
> Le 21/02/2019 à 16:32, Wei Liu a écrit :
> 
> > On Wed, Feb 20, 2019 at 04:19:25PM +, Ronan Abhamon wrote:
> > > From: Pritha Srivastava 
> > > 
> > > Displaying 0 is misleading.
> > > 
> > > Signed-off-by: Pritha Srivastava 
> > > Signed-off-by: Ronan Abhamon 
> > > ---
> > >   tools/xenstat/libxenstat/src/xenstat.c   |   6 +
> > >   tools/xenstat/libxenstat/src/xenstat.h   |   3 +
> > >   tools/xenstat/libxenstat/src/xenstat_linux.c |  47 +++---
> > >   tools/xenstat/libxenstat/src/xenstat_priv.h  |   1 +
> > >   tools/xenstat/xentop/xentop.c| 159 +++
> > >   5 files changed, 158 insertions(+), 58 deletions(-)
> > > 
> > > diff --git a/tools/xenstat/libxenstat/src/xenstat.c 
> > > b/tools/xenstat/libxenstat/src/xenstat.c
> > > index fbe44f3c56..8fa12d7bc0 100644
> > > --- a/tools/xenstat/libxenstat/src/xenstat.c
> > > +++ b/tools/xenstat/libxenstat/src/xenstat.c
> > > @@ -729,6 +729,12 @@ unsigned long long xenstat_vbd_wr_sects(xenstat_vbd 
> > > * vbd)
> > >   return vbd->wr_sects;
> > >   }
> > > +/* Returns error while getting stats (1 if error happened, 0 otherwise) 
> > > */
> > > +unsigned int xenstat_vbd_error(xenstat_vbd * vbd)
> > > +{
> > > + return vbd->error;
> > > +}
> > > +
> > You can do away with this function by accessing vbd->error directly in
> > code.
> 
> Thank you for your feedback, but xenstat_vbd is a private struct of
> libxenstat.
> 
> Correct me if I am wrong but I can't include this file in the xentop folder.

Oh, sorry. I misread.

Yes then you need to keep this function. But I would like you to change
the return type to bool if possible.

Wei.

> 
> 
> Ronan Abhamon
> 

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

Re: [Xen-devel] [PATCH] tools/xentop: Display '-' when stats are not available.

[Ronan Abhamon]

Le 21/02/2019 à 16:32, Wei Liu a écrit :


On Wed, Feb 20, 2019 at 04:19:25PM +, Ronan Abhamon wrote:

From: Pritha Srivastava 

Displaying 0 is misleading.

Signed-off-by: Pritha Srivastava 
Signed-off-by: Ronan Abhamon 
---
  tools/xenstat/libxenstat/src/xenstat.c   |   6 +
  tools/xenstat/libxenstat/src/xenstat.h   |   3 +
  tools/xenstat/libxenstat/src/xenstat_linux.c |  47 +++---
  tools/xenstat/libxenstat/src/xenstat_priv.h  |   1 +
  tools/xenstat/xentop/xentop.c| 159 +++
  5 files changed, 158 insertions(+), 58 deletions(-)

diff --git a/tools/xenstat/libxenstat/src/xenstat.c 
b/tools/xenstat/libxenstat/src/xenstat.c
index fbe44f3c56..8fa12d7bc0 100644
--- a/tools/xenstat/libxenstat/src/xenstat.c
+++ b/tools/xenstat/libxenstat/src/xenstat.c
@@ -729,6 +729,12 @@ unsigned long long xenstat_vbd_wr_sects(xenstat_vbd * vbd)
return vbd->wr_sects;
  }
  
+/* Returns error while getting stats (1 if error happened, 0 otherwise) */

+unsigned int xenstat_vbd_error(xenstat_vbd * vbd)
+{
+   return vbd->error;
+}
+

You can do away with this function by accessing vbd->error directly in
code.


Thank you for your feedback, but xenstat_vbd is a private struct of 
libxenstat.


Correct me if I am wrong but I can't include this file in the xentop 
folder.



Ronan Abhamon


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

Re: [Xen-devel] [PATCH v4.1 4/6] xen/x86: Allow stubdom access to irq created for msi.

[Roger Pau Monné]

On Thu, Feb 21, 2019 at 06:40:40PM +0100, Marek Marczykowski-Górecki wrote:
> On Thu, Feb 21, 2019 at 05:47:51PM +0100, Roger Pau Monné wrote:
> > On Fri, Feb 08, 2019 at 11:17:05AM +0100, Marek Marczykowski-Górecki wrote:
> > >  return -EINVAL;
> > >  }
> > >  
> > > diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> > > index 8b44d6ce0b..d41b32b2f4 100644
> > > --- a/xen/arch/x86/irq.c
> > > +++ b/xen/arch/x86/irq.c
> > > @@ -157,7 +157,7 @@ int __init bind_irq_vector(int irq, int vector, const 
> > > cpumask_t *cpu_mask)
> > >  /*
> > >   * Dynamic irq allocate and deallocation for MSI
> > >   */
> > > -int create_irq(nodeid_t node)
> > > +int create_irq(nodeid_t node, struct domain *dm_domain)
> > 
> > Using plain 'd' would be fine for me here, same below for
> > destroy_irq.
> 
> I think it may be misleading, as the parameter is not about domain that
> will handle that IRQ, but what domain will get access to it.

Right, but dom0 will also end up using this function for it's own irqs
(irqs allocated and mapped to dom0), in which case naming this
parameter dm_domain is misleading. IMO it's better to just name it
'd', which is the domain that gets permissions over the created irq.

> > >  {
> > >  int irq, ret;
> > >  struct irq_desc *desc;
> > > @@ -190,19 +190,19 @@ int create_irq(nodeid_t node)
> > >  desc->arch.used = IRQ_UNUSED;
> > >  irq = ret;
> > >  }
> > > -else if ( hardware_domain )
> > > +else if ( dm_domain )
> > 
> > Can you guarantee that dm_domain is always current->domain?
> 
> No, in some cases it will be hardware_domain.

Right, but in that case current->domain == hardware_domain I guess?

> 
> > I think you need to assert for this, or else take a reference to
> > dm_domain (get_domain) before accessing it's fields, or else you risk
> > the domain being destroyed while modifying it's fields.
> 
> Can hardware_domain be destroyed without panicking Xen? If so,
> get_domain would be indeed needed.

What about other callers that are not the hardware_domain? You need to
make sure those domains are not destroyed while {create/destroy}_irq
is changing the permissions.

If you can guarantee that dm_domain == current->domain then you are
safe, if not you need to get a reference before modifying any fields
of the domain struct.

So IMO you either need to add an assert or a get_domain depending on
the answer to the question above.

> > >  break;
> > >  }
> > >  }
> > > diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> > > index babc4147c4..66026e3ca5 100644
> > > --- a/xen/arch/x86/msi.c
> > > +++ b/xen/arch/x86/msi.c
> > > @@ -633,7 +633,7 @@ int __setup_msi_irq(struct irq_desc *desc, struct 
> > > msi_desc *msidesc,
> > >  return ret;
> > >  }
> > >  
> > > -int msi_free_irq(struct msi_desc *entry)
> > > +int msi_free_irq(struct msi_desc *entry, struct domain *dm_domain)
> > >  {
> > >  unsigned int nr = entry->msi_attrib.type != PCI_CAP_ID_MSIX
> > >? entry->msi.nvec : 1;
> > > @@ -641,7 +641,7 @@ int msi_free_irq(struct msi_desc *entry)
> > >  while ( nr-- )
> > >  {
> > >  if ( entry[nr].irq >= 0 )
> > > -destroy_irq(entry[nr].irq);
> > > +destroy_irq(entry[nr].irq, dm_domain);
> > >  
> > >  /* Free the unused IRTE if intr remap enabled */
> > >  if ( iommu_intremap )
> > > @@ -1280,7 +1280,7 @@ static void msi_free_irqs(struct pci_dev* dev)
> > >  list_for_each_entry_safe( entry, tmp, >msi_list, list )
> > >  {
> > >  pci_disable_msi(entry);
> > > -msi_free_irq(entry);
> > > +msi_free_irq(entry, hardware_domain);
> > 
> > This likely requires some comment to clarify why is the hardcoding of
> > hardware_domain correct here. AFAICT this will be called by
> > pci_remove_device, which I assume assures that the device has been
> > deassigned from any domain before attempting to remove it, hence it
> > can only have irqs assigned to the hardware_domain if any?
> 
> That's also my understanding, but I'm not 100% sure about that.
> See comment just bellow the commit message.

I would add an assert that pdev->domain == hardware_domain just to be
sure.

And then in pci_remove_device I think Xen should also check that
pdev->domain == hardware_domain or else refuse to remove the device.

Jan do you know whether pci_remove_device is supposed to be used
against devices assigned to a domain different than the hardware
domain?

Thanks, Roger.

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

Re: [Xen-devel] [RFC PATCH 0/4] Add missing default labels to switch statements

[Andrew Cooper]

On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
>
> Hello, everybody!
>
> We at EPAM Systems would like to present first series of patches targeting Xen
> on ARM Functional Safety certification (ISO61508 based): implementation of
> MISRA [1] C:2012 Rule 16.4 which requires that every switch statement has a
> default label as a measure of defensive programming technique.

Hang on - what?

Can someone attempt to justify why actively breaking -Wswitch is going
to result in safer/better code?

~Andrew

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