[Xen-devel] [seabios test] 100898: tolerable FAIL - PUSHED
flight 100898 seabios real [real] http://logs.test-lab.xenproject.org/osstest/logs/100898/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 100665 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stopfail like 100665 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2 fail never pass version targeted for testing: seabios 642db1905ab133007d7427b6758a2103fb09a19a baseline version: seabios f0cdc36d2f2424f6b40438f7ee7cc502c0eff4df Last test of basis 100665 2016-08-30 20:14:04 Z 13 days Testing same since 100898 2016-09-12 14:45:02 Z0 days1 attempts People who touched revisions under test: Kevin O'Connorjobs: 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-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsmpass test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm pass test-amd64-amd64-qemuu-nested-amdfail test-amd64-i386-qemuu-rhel6hvm-amd pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-i386-xl-qemuu-debianhvm-amd64 pass test-amd64-amd64-xl-qemuu-win7-amd64 fail test-amd64-i386-xl-qemuu-win7-amd64 fail test-amd64-amd64-qemuu-nested-intel pass test-amd64-i386-qemuu-rhel6hvm-intel pass test-amd64-i386-xl-qemuu-winxpsp3-vcpus1 pass test-amd64-amd64-xl-qemuu-winxpsp3 pass test-amd64-i386-xl-qemuu-winxpsp3pass 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 : + branch=seabios + revision=642db1905ab133007d7427b6758a2103fb09a19a + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x '!=' x/home/osstest/repos/lock ']' ++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock ++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push seabios 642db1905ab133007d7427b6758a2103fb09a19a + branch=seabios + revision=642db1905ab133007d7427b6758a2103fb09a19a + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']' + . ./cri-common ++ . ./cri-getconfig ++ umask 002 + select_xenbranch + case "$branch" in + tree=seabios + xenbranch=xen-unstable + '[' xseabios = xlinux ']' + linuxbranch= + '[' x = x ']' + qemuubranch=qemu-upstream-unstable + select_prevxenbranch ++ ./cri-getprevxenbranch xen-unstable + prevxenbranch=xen-4.7-testing + '['
[Xen-devel] [xen-4.5-testing test] 100897: regressions - FAIL
flight 100897 xen-4.5-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/100897/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-xl-multivcpu 15 guest-start/debian.repeat fail REGR. vs. 100828 test-amd64-amd64-xl-qemuu-win7-amd64 15 guest-localmigrate/x10 fail REGR. vs. 100828 Regressions which are regarded as allowable (not blocking): test-amd64-amd64-xl-rtds 6 xen-boot fail like 100828 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stopfail like 100828 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 100828 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail like 100828 test-armhf-armhf-xl-rtds 15 guest-start/debian.repeatfail like 100828 Tests which did not succeed, but are not blocking: test-amd64-i386-rumprun-i386 1 build-check(1) blocked n/a test-amd64-amd64-rumprun-amd64 1 build-check(1) blocked n/a build-i386-rumprun5 rumprun-buildfail never pass build-amd64-rumprun 5 rumprun-buildfail never pass test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt 14 guest-saverestorefail never pass test-armhf-armhf-libvirt-qcow2 10 guest-start fail never pass test-armhf-armhf-xl-arndale 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 12 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 10 guest-start fail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass test-amd64-amd64-xl-pvh-amd 11 guest-start fail never pass test-armhf-armhf-xl-vhd 10 guest-start fail never pass test-amd64-amd64-xl-pvh-intel 11 guest-start fail never pass test-armhf-armhf-xl-cubietruck 12 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-amd64-i386-libvirt 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2 fail never pass test-armhf-armhf-xl-rtds 12 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 saverestore-support-checkfail never pass version targeted for testing: xen 9edce7c42e6c2e8dd19788cab688cb46f779a9ec baseline version: xen 433ebca120e8750eb8085745ccac703e47358e6f Last test of basis 100828 2016-09-08 21:42:35 Z4 days Testing same since 100897 2016-09-12 14:42:24 Z0 days1 attempts People who touched revisions under test: "Rockosov, Dmitry"Andrew Cooper Jan Beulich Tim Deegan jobs: build-amd64-xtf pass build-amd64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-prev pass build-i386-prev pass build-amd64-pvopspass build-armhf-pvopspass build-i386-pvops pass build-amd64-rumprun fail build-i386-rumprun fail test-xtf-amd64-amd64-1 pass test-xtf-amd64-amd64-2 pass test-xtf-amd64-amd64-3 pass test-xtf-amd64-amd64-4 pass test-xtf-amd64-amd64-5 pass test-amd64-amd64-xl
[Xen-devel] [xen-4.6-testing test] 100895: tolerable FAIL - PUSHED
flight 100895 xen-4.6-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/100895/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-rtds 15 guest-start/debian.repeat fail blocked in 100835 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 100835 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail like 100835 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stopfail like 100835 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stopfail like 100835 Tests which did not succeed, but are not blocking: test-amd64-amd64-rumprun-amd64 1 build-check(1) blocked n/a test-amd64-i386-rumprun-i386 1 build-check(1) blocked n/a build-amd64-rumprun 5 rumprun-buildfail never pass build-i386-rumprun5 rumprun-buildfail never pass test-armhf-armhf-xl-credit2 12 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 12 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 saverestore-support-checkfail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl-xsm 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 guest-saverestorefail never pass test-armhf-armhf-xl-arndale 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt 14 guest-saverestorefail never pass test-armhf-armhf-xl-cubietruck 12 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 13 guest-saverestorefail never pass test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-amd64-i386-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-amd64-xl-pvh-intel 11 guest-start fail never pass test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2 fail never pass test-amd64-amd64-xl-pvh-amd 11 guest-start fail never pass version targeted for testing: xen 6b5bb502a93bcb7617ea1f3f5a8712f2b9f33d90 baseline version: xen 26352b6344ce5d5a2ee78e56ae631e156fbdce7f Last test of basis 100835 2016-09-09 03:01:10 Z3 days Testing same since 100895 2016-09-12 14:15:38 Z0 days1 attempts People who touched revisions under test: "Rockosov, Dmitry"Andrew Cooper Jan Beulich Tim Deegan jobs: build-amd64-xsm pass build-armhf-xsm pass build-i386-xsm pass build-amd64-xtf pass build-amd64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass
[Xen-devel] [xen-4.7-testing test] 100896: regressions - FAIL
flight 100896 xen-4.7-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/100896/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-xl 9 debian-install fail REGR. vs. 100830 build-amd64 5 xen-buildfail REGR. vs. 100830 build-amd64-xsm 5 xen-buildfail REGR. vs. 100830 Tests which did not succeed, but are not blocking: build-amd64-rumprun 1 build-check(1) blocked n/a test-xtf-amd64-amd64-11 build-check(1) blocked n/a test-amd64-amd64-migrupgrade 1 build-check(1) blocked n/a test-amd64-amd64-pair 1 build-check(1) blocked n/a test-xtf-amd64-amd64-21 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-xl-qemut-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-i386-xl-qemut-debianhvm-amd64 1 build-check(1) blocked n/a test-amd64-amd64-pygrub 1 build-check(1) blocked n/a test-amd64-amd64-amd64-pvgrub 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-i386-xl1 build-check(1) blocked n/a test-amd64-i386-qemuu-rhel6hvm-intel 1 build-check(1) blocked n/a test-amd64-i386-freebsd10-i386 1 build-check(1) blocked n/a test-amd64-i386-xl-qemut-winxpsp3 1 build-check(1) blocked n/a test-amd64-i386-qemut-rhel6hvm-intel 1 build-check(1) blocked n/a test-amd64-i386-xl-xsm1 build-check(1) blocked n/a test-amd64-i386-qemuu-rhel6hvm-amd 1 build-check(1) blocked n/a test-amd64-i386-xl-qemut-winxpsp3-vcpus1 1 build-check(1) blocked n/a test-amd64-i386-pair 1 build-check(1) blocked n/a test-amd64-i386-qemut-rhel6hvm-amd 1 build-check(1) blocked n/a test-amd64-i386-xl-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-migrupgrade 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-winxpsp3-vcpus1 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-winxpsp3 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-debianhvm-amd64 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-i386-xl-raw1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-win7-amd64 1 build-check(1) blocked n/a test-amd64-i386-rumprun-i386 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-winxpsp3 1 build-check(1) blocked n/a test-amd64-i386-xl-qemut-win7-amd64 1 build-check(1) blocked n/a test-amd64-i386-freebsd10-amd64 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-win7-amd64 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-debianhvm-amd64 1 build-check(1)blocked n/a test-amd64-amd64-qemuu-nested-intel 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-xl-pvh-amd 1 build-check(1) blocked n/a test-amd64-amd64-rumprun-amd64 1 build-check(1) blocked n/a test-xtf-amd64-amd64-31 build-check(1) blocked n/a test-amd64-amd64-xl-pvh-intel 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemut-debianhvm-amd64-xsm 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-multivcpu 1 build-check(1) blocked n/a test-amd64-amd64-xl-qcow2 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemut-win7-amd64 1 build-check(1) blocked n/a test-amd64-amd64-xl-credit2 1 build-check(1) blocked n/a test-amd64-amd64-i386-pvgrub 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemut-winxpsp3 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked
[Xen-devel] [ovmf baseline-only test] 67700: all pass
This run is configured for baseline tests only. flight 67700 ovmf real [real] http://osstest.xs.citrite.net/~osstest/testlogs/logs/67700/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 8c0b0b34f7875571ee9d3a2a1a28484cef36d969 baseline version: ovmf 5654835bd1381dfad9483b767e55db7caaeec907 Last test of basis67699 2016-09-12 12:47:30 Z0 days Testing same since67700 2016-09-12 19:48:41 Z0 days1 attempts People who touched revisions under test: Laszlo ErsekThomas Huth 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 pass sg-report-flight on osstest.xs.citrite.net logs: /home/osstest/logs images: /home/osstest/images Logs, config files, etc. are available at http://osstest.xs.citrite.net/~osstest/testlogs/logs Test harness code can be found at http://xenbits.xensource.com/gitweb?p=osstest.git;a=summary Push not applicable. commit 8c0b0b34f7875571ee9d3a2a1a28484cef36d969 Author: Thomas Huth Date: Fri Sep 9 22:32:15 2016 +0200 OvmfPkg: Fix typing errors Correct some typos (discovered with the codespell utility) Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Thomas Huth Reviewed-by: Laszlo Ersek Cc: Jordan Justen Cc: Thomas Huth Reviewed-by: Jordan Justen commit 3e92a99747aa7027c4b375a75e23e9f2dcc5293b Author: Laszlo Ersek Date: Fri Sep 9 23:32:05 2016 +0200 OvmfPkg: convert C files with LF line terminators to CRLF Run "unix2dos" on the affected files. "git show -b" produces no diff for this patch. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek Cc: Jordan Justen Cc: Thomas Huth Reviewed-by: Jordan Justen Reviewed-by: Thomas Huth commit 3e079d0198b1462d3af3a7883ac20c87d0a25bb6 Author: Laszlo Ersek Date: Fri Sep 9 23:24:54 2016 +0200 OvmfPkg/IndustryStandard: make "Xen/grant_table.h" pure ASCII The header file includes the UTF-8 encoding (0xE2 0x80 0x99) of the U+2019 (RIGHT SINGLE QUOTATION MARK) code point. Replace it with a simple apostrophe (U+0027, ASCII 0x27). Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek Cc: Jordan Justen Cc: Thomas Huth Reviewed-by: Jordan Justen Reviewed-by: Thomas Huth ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable test] 100893: regressions - FAIL
flight 100893 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/100893/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-xl-credit2 16 guest-start.2fail REGR. vs. 100887 Regressions which are regarded as allowable (not blocking): test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail like 100887 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 100887 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stopfail like 100887 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stopfail like 100887 test-amd64-amd64-xl-rtds 9 debian-install fail like 100887 Tests which did not succeed, but are not blocking: test-amd64-amd64-rumprun-amd64 1 build-check(1) blocked n/a test-amd64-i386-rumprun-i386 1 build-check(1) blocked n/a build-i386-rumprun5 rumprun-buildfail never pass build-amd64-rumprun 5 rumprun-buildfail never pass test-armhf-armhf-xl-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 saverestore-support-checkfail never pass test-armhf-armhf-xl-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl-xsm 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 12 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 12 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 12 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 guest-saverestorefail never pass test-armhf-armhf-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt 14 guest-saverestorefail never pass test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-i386-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 13 guest-saverestorefail never pass test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-amd64-xl-pvh-intel 11 guest-start fail never pass test-amd64-amd64-xl-pvh-amd 11 guest-start fail never pass test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2 fail never pass version targeted for testing: xen d45fae589b8d8b6d36c211dcc46d767dda730b61 baseline version: xen a3fe74e4345e66ddb7aa514395260a5e5f8b0cdc Last test of basis 100887 2016-09-12 01:59:02 Z0 days Testing same since 100893 2016-09-12 12:44:56 Z0 days1 attempts People who touched revisions under test: Andrew CooperJan Beulich Juergen Gross Tim Deegan Wei Liu jobs: build-amd64-xsm pass build-armhf-xsm pass build-i386-xsm pass build-amd64-xtf pass build-amd64 pass build-armhf pass build-i386 pass build-amd64-libvirt
Re: [Xen-devel] Xen 4.8 Development Update
On Wed, Aug 31, 2016 at 09:30:46AM +0100, Wei Liu wrote: [...] > * Boot Xen on EFI platforms using GRUB2 (multiboot2 protocol) > - Daniel Kiper I have just sent v6. Daniel ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v6 14/15] x86/boot: rename sym_phys() to sym_offs()
This way macro name better describes its function. There is no functional change. Suggested-by: Jan BeulichSigned-off-by: Daniel Kiper --- xen/arch/x86/boot/head.S | 40 xen/arch/x86/boot/trampoline.S |2 +- xen/arch/x86/boot/wakeup.S |4 ++-- xen/arch/x86/boot/x86_64.S | 18 +- 4 files changed, 32 insertions(+), 32 deletions(-) diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index d747f4a..bb2163a 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -12,9 +12,9 @@ .text .code32 -#define sym_phys(sym) ((sym) - __XEN_VIRT_START) -#define sym_esi(sym) sym_phys(sym)(%esi) -#define sym_fs(sym) %fs:sym_phys(sym) +#define sym_offs(sym) ((sym) - __XEN_VIRT_START) +#define sym_esi(sym) sym_offs(sym)(%esi) +#define sym_fs(sym) %fs:sym_offs(sym) #define BOOT_CS320x0008 #define BOOT_CS640x0010 @@ -97,7 +97,7 @@ multiboot2_header_start: /* EFI64 entry point. */ mb2ht_init MB2_HT(ENTRY_ADDRESS_EFI64), MB2_HT(OPTIONAL), \ - sym_phys(__efi64_start) + sym_offs(__efi64_start) /* Multiboot2 header end tag. */ mb2ht_init MB2_HT(END), MB2_HT(REQUIRED) @@ -110,11 +110,11 @@ multiboot2_header_start: gdt_boot_descr: .word 7*8-1 gdt_boot_base: -.long sym_phys(trampoline_gdt) +.long sym_offs(trampoline_gdt) .long 0 /* Needed for 64-bit lgdt */ cs32_switch_addr: -.long sym_phys(cs32_switch) +.long sym_offs(cs32_switch) .word BOOT_CS32 .align 4 @@ -131,23 +131,23 @@ vga_text_buffer: .section .init.text, "ax", @progbits bad_cpu: -add $sym_phys(.Lbad_cpu_msg),%esi # Error message +add $sym_offs(.Lbad_cpu_msg),%esi # Error message jmp .Lget_vtb not_multiboot: -add $sym_phys(.Lbad_ldr_msg),%esi # Error message +add $sym_offs(.Lbad_ldr_msg),%esi # Error message jmp .Lget_vtb .Lmb2_no_st: -add $sym_phys(.Lbad_ldr_nst),%esi # Error message +add $sym_offs(.Lbad_ldr_nst),%esi # Error message jmp .Lget_vtb .Lmb2_no_ih: -add $sym_phys(.Lbad_ldr_nih),%esi # Error message +add $sym_offs(.Lbad_ldr_nih),%esi # Error message jmp .Lget_vtb .Lmb2_no_bs: -add $sym_phys(.Lbad_ldr_nbs),%esi # Error message +add $sym_offs(.Lbad_ldr_nbs),%esi # Error message xor %edi,%edi # No VGA text buffer jmp .Lsend_chr .Lmb2_efi_ia_32: -add $sym_phys(.Lbad_efi_msg),%esi # Error message +add $sym_offs(.Lbad_efi_msg),%esi # Error message xor %edi,%edi # No VGA text buffer jmp .Lsend_chr .Lget_vtb: @@ -350,7 +350,7 @@ __start: cli /* Load default Xen image load base address. */ -mov $sym_phys(__image_base__),%esi +mov $sym_offs(__image_base__),%esi /* Bootloaders may set multiboot{1,2}.mem_lower to a nonzero value. */ xor %edx,%edx @@ -499,8 +499,8 @@ trampoline_setup: jnz 1f /* Initialize BSS (no nasty surprises!). */ -mov $sym_phys(__bss_start),%edi -mov $sym_phys(__bss_end),%ecx +mov $sym_offs(__bss_start),%edi +mov $sym_offs(__bss_end),%ecx push%fs pop %es sub %edi,%ecx @@ -574,22 +574,22 @@ trampoline_setup: /* Apply relocations to bootstrap trampoline. */ mov sym_fs(trampoline_phys),%edx -mov $sym_phys(__trampoline_rel_start),%edi +mov $sym_offs(__trampoline_rel_start),%edi 1: mov %fs:(%edi),%eax add %edx,%fs:(%edi,%eax) add $4,%edi -cmp $sym_phys(__trampoline_rel_stop),%edi +cmp $sym_offs(__trampoline_rel_stop),%edi jb 1b /* Patch in the trampoline segment. */ shr $4,%edx -mov $sym_phys(__trampoline_seg_start),%edi +mov $sym_offs(__trampoline_seg_start),%edi 1: mov %fs:(%edi),%eax mov %dx,%fs:(%edi,%eax) add $4,%edi -cmp $sym_phys(__trampoline_seg_stop),%edi +cmp $sym_offs(__trampoline_seg_stop),%edi jb 1b /* Do not parse command line on EFI platform here. */ @@ -615,7 +615,7 @@ trampoline_setup: push%eax /* Copy bootstrap trampoline to low memory, below 1MB. */ -mov $sym_phys(trampoline_start),%esi +mov $sym_offs(trampoline_start),%esi mov $((trampoline_end - trampoline_start) / 4),%ecx rep movsl %fs:(%esi),%es:(%edi) diff --git
[Xen-devel] [PATCH v6 08/15] x86/efi: create new early memory allocator
There is a problem with place_string() which is used as early memory allocator. It gets memory chunks starting from start symbol and goes down. Sadly this does not work when Xen is loaded using multiboot2 protocol because then the start lives on 1 MiB address and we should not allocate a memory from below of it. So, I tried to use mem_lower address calculated by GRUB2. However, this solution works only on some machines. There are machines in the wild (e.g. Dell PowerEdge R820) which uses first ~640 KiB for boot services code or data... :-((( Hence, we need new memory allocator for Xen EFI boot code which is quite simple and generic and could be used by place_string() and efi_arch_allocate_mmap_buffer(). I think about following solutions: 1) We could use native EFI allocation functions (e.g. AllocatePool() or AllocatePages()) to get memory chunk. However, later (somewhere in __start_xen()) we must copy its contents to safe place or reserve it in e820 memory map and map it in Xen virtual address space. This means that the code referring to Xen command line, loaded modules and EFI memory map, mostly in __start_xen(), will be further complicated and diverge from legacy BIOS cases. Additionally, both former things have to be placed below 4 GiB because their addresses are stored in multiboot_info_t structure which has 32-bit relevant members. 2) We may allocate memory area statically somewhere in Xen code which could be used as memory pool for early dynamic allocations. Looks quite simple. Additionally, it would not depend on EFI at all and could be used on legacy BIOS platforms if we need it. However, we must carefully choose size of this pool. We do not want increase Xen binary size too much and waste too much memory but also we must fit at least memory map on x86 EFI platforms. As I saw on small machine, e.g. IBM System x3550 M2 with 8 GiB RAM, memory map may contain more than 200 entries. Every entry on x86-64 platform is 40 bytes in size. So, it means that we need more than 8 KiB for EFI memory map only. Additionally, if we use this memory pool for Xen and modules command line storage (it would be used when xen.efi is executed as EFI application) then we should add, I think, about 1 KiB. In this case, to be on safe side, we should assume at least 64 KiB pool for early memory allocations. Which is about 4 times of our earlier calculations. However, during discussion on Xen-devel Jan Beulich suggested that just in case we should use 1 MiB memory pool like it is in original place_string() implementation. So, let's use 1 MiB as it was proposed. If we think that we should not waste unallocated memory in the pool on running system then we can mark this region as __initdata and move all required data to dynamically allocated places somewhere in __start_xen(). 2a) We could put memory pool into .bss.page_aligned section. Then allocate memory chunks starting from the lowest address. After init phase we can free unused portion of the memory pool as in case of .init.text or .init.data sections. This way we do not need to allocate any space in image file and freeing of unused area in the memory pool is very simple. Now #2a solution is implemented because it is quite simple and requires limited number of changes, especially in __start_xen(). Signed-off-by: Daniel Kiper--- v6 - suggestions/fixes: - optimize ebmalloc allocator, - move ebmalloc machinery to xen/common/efi/boot.c (suggested by Jan Beulich), - enforce PAGE_SIZE ebmalloc_mem alignment (suggested by Jan Beulich), - ebmalloc() must allocate properly aligned memory regions (suggested by Jan Beulich), - printk() should use XENLOG_INFO (suggested by Jan Beulich). v4 - suggestions/fixes: - move from #2 solution to #2a solution, - improve commit message. --- xen/arch/x86/efi/efi-boot.h | 11 +++ xen/arch/x86/efi/stub.c |2 ++ xen/arch/x86/setup.c|5 +++-- xen/common/efi/boot.c | 35 ++- xen/include/xen/efi.h |1 + 5 files changed, 43 insertions(+), 11 deletions(-) diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 10985721..42cc5f8 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -117,7 +117,7 @@ static void __init relocate_trampoline(unsigned long phys) static void __init place_string(u32 *addr, const char *s) { -static char *__initdata alloc = start; +char *alloc = NULL; if ( s && *s ) { @@ -125,7 +125,7 @@ static void __init place_string(u32 *addr, const char *s) const char *old = (char *)(long)*addr; size_t len2 = *addr ? strlen(old) + 1 : 0; -alloc -= len1 + len2; +alloc = ebmalloc(len1 + len2); /* * Insert new string before already existing one. This is needed *
[Xen-devel] [PATCH v6 12/15] x86/setup: use XEN_IMG_OFFSET instead of...
..calculating its value during runtime. Signed-off-by: Daniel Kiper--- xen/arch/x86/setup.c |4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index f19af6e..ca1a97a 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -901,7 +901,6 @@ void __init noreturn __start_xen(unsigned long mbi_p) l4_pgentry_t *pl4e; l3_pgentry_t *pl3e; l2_pgentry_t *pl2e; -uint64_t load_start; int i, j, k; /* Select relocation address. */ @@ -915,9 +914,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) * with a barrier(). After this we must *not* modify static/global * data until after we have switched to the relocated pagetables! */ -load_start = (unsigned long)_start - XEN_VIRT_START; barrier(); -move_memory(e + load_start, load_start, _end - _start, 1); +move_memory(e + XEN_IMG_OFFSET, XEN_IMG_OFFSET, _end - _start, 1); /* Walk initial pagetables, relocating page directory entries. */ pl4e = __va(__pa(idle_pg_table)); -- 1.7.10.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v6 13/15] x86: make Xen early boot code relocatable
Every multiboot protocol (regardless of version) compatible image must specify its load address (in ELF or multiboot header). Multiboot protocol compatible loader have to load image at specified address. However, there is no guarantee that the requested memory region (in case of Xen it starts at 2 MiB and ends at ~5 MiB) where image should be loaded initially is a RAM and it is free (legacy BIOS platforms are merciful for Xen but I found at least one EFI platform on which Xen load address conflicts with EFI boot services; it is Dell PowerEdge R820 with latest firmware). To cope with that problem we must make Xen early boot code relocatable and help boot loader to relocate image in proper way by suggesting, not requesting specific load addresses as it is right now, allowed address ranges. This patch does former. It does not add multiboot2 protocol interface which is done in "x86: add multiboot2 protocol support for relocatable images" patch. This patch changes following things: - %esi and %r15d registers are used as a storage for Xen image load base address (%r15d shortly because %rsi is used for EFI SystemTable address in 64-bit code); both registers are (%esi is mostly) unused in early boot code and preserved during C functions calls (%esi in 32-bit code and %r15d in 64-bit code), - %fs is used as base for Xen data relative addressing in 32-bit code if it is possible; %esi is used for that thing during error printing because it is not always possible to properly and efficiently initialize %fs. Signed-off-by: Daniel Kiper--- v6 - suggestions/fixes: - leave static mapping of first 16 MiB in l2_identmap as is (suggested by Jan Beulich), - use xen_phys_start instead of xen_img_load_base_addr (suggested by Daniel Kiper and Jan Beulich), - simplify BOOT_FS segment descriptor base address initialization (suggested by Jan Beulich), - fix BOOT_FS segment limit (suggested by Jan Beulich), - do not rename sym_phys in this patch (suggested by Jan Beulich), - rename esi_offset/fs_offset to sym_esi/sym_fs respectively (suggested by Jan Beulich), - use add instead of lea in assembly error printing code (suggested by Jan Beulich), - improve comments (suggested by Jan Beulich), - improve commit message (suggested by Jan Beulich), - various minor cleanups and fixes (suggested by Jan Beulich). v4 - suggestions/fixes: - do not relocate Xen image if boot loader did work for us (suggested by Andrew Cooper and Jan Beulich), - initialize xen_img_load_base_addr in EFI boot code too, - properly initialize trampoline_xen_phys_start, - calculate Xen image load base address in x86_64 code ourselves, (suggested by Jan Beulich), - change how and when Xen image base address is printed, - use %fs instead of %esi for relative addressing (suggested by Andrew Cooper and Jan Beulich), - create esi_offset and fs_offset() macros in assembly, - calculate mkelf32 argument automatically, - optimize and cleanup code, - improve comments, - improve commit message. v3 - suggestions/fixes: - improve segment registers initialization (suggested by Jan Beulich), - simplify Xen image load base address calculation (suggested by Jan Beulich), - use %esi and %r15d instead of %ebp to store Xen image load base address, - use %esi instead of %fs for relative addressing; this way we get shorter and simpler code, - rename some variables and constants (suggested by Jan Beulich), - improve comments (suggested by Konrad Rzeszutek Wilk), - improve commit message (suggested by Jan Beulich). --- xen/arch/x86/boot/head.S | 163 + xen/arch/x86/boot/trampoline.S|5 ++ xen/arch/x86/boot/x86_64.S| 26 +++--- xen/arch/x86/setup.c | 14 ++-- xen/arch/x86/x86_64/asm-offsets.c |3 + xen/include/asm-x86/page.h|2 +- 6 files changed, 157 insertions(+), 56 deletions(-) diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 12bbd8e..d747f4a 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -13,12 +13,15 @@ .code32 #define sym_phys(sym) ((sym) - __XEN_VIRT_START) +#define sym_esi(sym) sym_phys(sym)(%esi) +#define sym_fs(sym) %fs:sym_phys(sym) #define BOOT_CS320x0008 #define BOOT_CS640x0010 #define BOOT_DS 0x0018 #define BOOT_PSEUDORM_CS 0x0020 #define BOOT_PSEUDORM_DS 0x0028 +#define BOOT_FS 0x0030 #define MB2_HT(name) (MULTIBOOT2_HEADER_TAG_##name) #define MB2_TT(name) (MULTIBOOT2_TAG_TYPE_##name) @@ -105,7 +108,8 @@ multiboot2_header_start: .word 0 gdt_boot_descr: -.word 6*8-1 +.word 7*8-1 +gdt_boot_base: .long sym_phys(trampoline_gdt) .long 0
[Xen-devel] [PATCH v6 15/15] x86: add multiboot2 protocol support for relocatable images
Add multiboot2 protocol support for relocatable images. Only GRUB2 with "multiboot2: Add support for relocatable images" patch understands that feature. Older multiboot protocol (regardless of version) compatible loaders ignore it and everything works as usual. Signed-off-by: Daniel Kiper--- v4 - suggestions/fixes: - do not get Xen image load base address from multiboot2 information in x86_64 code (suggested by Jan Beulich), - improve label names (suggested by Jan Beulich), - improve comments, (suggested by Jan Beulich). v3 - suggestions/fixes: - use %esi and %r15d instead of %ebp to store Xen image load base address, - rename some types and constants, - reformat xen/include/xen/multiboot2.h (suggested by Konrad Rzeszutek Wilk), - improve comments, - improve commit message (suggested by Konrad Rzeszutek Wilk). --- xen/arch/x86/boot/head.S | 19 ++- xen/arch/x86/x86_64/asm-offsets.c |1 + xen/include/xen/multiboot2.h | 13 + 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index bb2163a..9761d30 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -82,6 +82,13 @@ multiboot2_header_start: /* Align modules at page boundry. */ mb2ht_init MB2_HT(MODULE_ALIGN), MB2_HT(REQUIRED) +/* Load address preference. */ +mb2ht_init MB2_HT(RELOCATABLE), MB2_HT(OPTIONAL), \ + sym_offs(start), /* Min load address. */ \ + 0x, /* The end of image max load address (4 GiB - 1). */ \ + 0x20, /* Load address alignment (2 MiB). */ \ + MULTIBOOT2_LOAD_PREFERENCE_HIGH + /* Console flags tag. */ mb2ht_init MB2_HT(CONSOLE_FLAGS), MB2_HT(OPTIONAL), \ MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED @@ -382,10 +389,19 @@ __start: cmp %edi,MB2_fixed_total_size(%ebx) jbe trampoline_bios_setup +/* Get Xen image load base address from Multiboot2 information. */ +cmpl$MULTIBOOT2_TAG_TYPE_LOAD_BASE_ADDR,MB2_tag_type(%ecx) +jne 1f + +mov MB2_load_base_addr(%ecx),%esi +sub $XEN_IMG_OFFSET,%esi +jmp 9f + +1: /* Get mem_lower from Multiboot2 information. */ cmpl$MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,MB2_tag_type(%ecx) cmove MB2_mem_lower(%ecx),%edx -je trampoline_bios_setup +je 9f /* EFI IA-32 platforms are not supported. */ cmpl$MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx) @@ -399,6 +415,7 @@ __start: cmpl$MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%ecx) je trampoline_bios_setup +9: /* Go to next Multiboot2 information tag. */ add MB2_tag_size(%ecx),%ecx add $(MULTIBOOT2_TAG_ALIGN-1),%ecx diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c index 5d7a8e5..beac5ca 100644 --- a/xen/arch/x86/x86_64/asm-offsets.c +++ b/xen/arch/x86/x86_64/asm-offsets.c @@ -174,6 +174,7 @@ void __dummy__(void) OFFSET(MB2_fixed_total_size, multiboot2_fixed_t, total_size); OFFSET(MB2_tag_type, multiboot2_tag_t, type); OFFSET(MB2_tag_size, multiboot2_tag_t, size); +OFFSET(MB2_load_base_addr, multiboot2_tag_load_base_addr_t, load_base_addr); OFFSET(MB2_mem_lower, multiboot2_tag_basic_meminfo_t, mem_lower); OFFSET(MB2_efi64_st, multiboot2_tag_efi64_t, pointer); OFFSET(MB2_efi64_ih, multiboot2_tag_efi64_ih_t, pointer); diff --git a/xen/include/xen/multiboot2.h b/xen/include/xen/multiboot2.h index 8dd5800..feb4297 100644 --- a/xen/include/xen/multiboot2.h +++ b/xen/include/xen/multiboot2.h @@ -59,11 +59,17 @@ #define MULTIBOOT2_HEADER_TAG_EFI_BS 7 #define MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS_EFI32 8 #define MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS_EFI64 9 +#define MULTIBOOT2_HEADER_TAG_RELOCATABLE 10 /* Header tag flags. */ #define MULTIBOOT2_HEADER_TAG_REQUIRED 0 #define MULTIBOOT2_HEADER_TAG_OPTIONAL 1 +/* Where image should be loaded (suggestion not requirement). */ +#define MULTIBOOT2_LOAD_PREFERENCE_NONE0 +#define MULTIBOOT2_LOAD_PREFERENCE_LOW 1 +#define MULTIBOOT2_LOAD_PREFERENCE_HIGH2 + /* Header console tag console_flags. */ #define MULTIBOOT2_CONSOLE_FLAGS_CONSOLE_REQUIRED 1 #define MULTIBOOT2_CONSOLE_FLAGS_EGA_TEXT_SUPPORTED2 @@ -90,6 +96,7 @@ #define MULTIBOOT2_TAG_TYPE_EFI_BS 18 #define MULTIBOOT2_TAG_TYPE_EFI32_IH 19 #define MULTIBOOT2_TAG_TYPE_EFI64_IH 20 +#define MULTIBOOT2_TAG_TYPE_LOAD_BASE_ADDR 21 /* Multiboot 2 tag alignment. */ #define MULTIBOOT2_TAG_ALIGN
[Xen-devel] [PATCH v6 09/15] x86: add multiboot2 protocol support for EFI platforms
This way Xen can be loaded on EFI platforms using GRUB2 and other boot loaders which support multiboot2 protocol. Signed-off-by: Daniel Kiper--- v6 - suggestions/fixes: - improve label names in assembly error printing code (suggested by Jan Beulich), - improve comments (suggested by Jan Beulich), - various minor cleanups and fixes (suggested by Jan Beulich). v4 - suggestions/fixes: - remove redundant BSS alignment, - update BSS alignment check, - use __set_bit() instead of set_bit() if possible (suggested by Jan Beulich), - call efi_arch_cpu() from efi_multiboot2() even if the same work is done later in other place right now (suggested by Jan Beulich), - xen/arch/x86/efi/stub.c:efi_multiboot2() fail properly on EFI platforms, - do not read data beyond the end of multiboot2 information in xen/arch/x86/boot/head.S (suggested by Jan Beulich), - use 32-bit registers in x86_64 code if possible (suggested by Jan Beulich), - multiboot2 information address is 64-bit in x86_64 code, so, treat it is as is (suggested by Jan Beulich), - use cmovcc if possible, - leave only one space between rep and stosq (suggested by Jan Beulich), - improve error handling, - improve early error messages, (suggested by Jan Beulich), - improve early error messages printing code, - improve label names (suggested by Jan Beulich), - improve comments (suggested by Jan Beulich), - various minor cleanups. v3 - suggestions/fixes: - take into account alignment when skipping multiboot2 fixed part (suggested by Konrad Rzeszutek Wilk), - improve segment registers initialization (suggested by Jan Beulich), - improve comments (suggested by Jan Beulich and Konrad Rzeszutek Wilk), - improve commit message (suggested by Jan Beulich). v2 - suggestions/fixes: - generate multiboot2 header using macros (suggested by Jan Beulich), - switch CPU to x86_32 mode before jumping to 32-bit code (suggested by Andrew Cooper), - reduce code changes to increase patch readability (suggested by Jan Beulich), - improve comments (suggested by Jan Beulich), - ignore MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO tag on EFI platform and find on my own multiboot2.mem_lower value, - stop execution if EFI platform is detected in legacy BIOS path. --- xen/arch/x86/boot/head.S | 255 ++--- xen/arch/x86/efi/efi-boot.h | 49 ++- xen/arch/x86/efi/stub.c | 37 ++ xen/arch/x86/x86_64/asm-offsets.c |2 + xen/arch/x86/xen.lds.S|4 +- xen/common/efi/boot.c | 11 ++ 6 files changed, 336 insertions(+), 22 deletions(-) diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 383ff79..00fa47d 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -89,6 +89,13 @@ multiboot2_header_start: 0, /* Number of the lines - no preference. */ \ 0 /* Number of bits per pixel - no preference. */ +/* Inhibit bootloader from calling ExitBootServices(). */ +mb2ht_init MB2_HT(EFI_BS), MB2_HT(OPTIONAL) + +/* EFI64 entry point. */ +mb2ht_init MB2_HT(ENTRY_ADDRESS_EFI64), MB2_HT(OPTIONAL), \ + sym_phys(__efi64_start) + /* Multiboot2 header end tag. */ mb2ht_init MB2_HT(END), MB2_HT(REQUIRED) .Lmultiboot2_header_end: @@ -100,20 +107,49 @@ multiboot2_header_start: gdt_boot_descr: .word 6*8-1 .long sym_phys(trampoline_gdt) +.long 0 /* Needed for 64-bit lgdt */ + +cs32_switch_addr: +.long sym_phys(cs32_switch) +.word BOOT_CS32 + +.align 4 +vga_text_buffer: +.long 0xb8000 .Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!" .Lbad_ldr_msg: .asciz "ERR: Not a Multiboot bootloader!" +.Lbad_ldr_nbs: .asciz "ERR: Bootloader shutdown EFI x64 boot services!" +.Lbad_ldr_nst: .asciz "ERR: EFI SystemTable is not provided by bootloader!" +.Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!" +.Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!" .section .init.text, "ax", @progbits bad_cpu: mov $(sym_phys(.Lbad_cpu_msg)),%esi # Error message -jmp print_err +jmp .Lget_vtb not_multiboot: mov $(sym_phys(.Lbad_ldr_msg)),%esi # Error message -print_err: -mov $0xB8000,%edi # VGA framebuffer -1: mov (%esi),%bl +jmp .Lget_vtb +.Lmb2_no_st: +mov $(sym_phys(.Lbad_ldr_nst)),%esi # Error message +jmp .Lget_vtb +.Lmb2_no_ih: +mov $(sym_phys(.Lbad_ldr_nih)),%esi # Error message +jmp .Lget_vtb +.Lmb2_no_bs: +mov $(sym_phys(.Lbad_ldr_nbs)),%esi # Error message +xor %edi,%edi
[Xen-devel] [PATCH v6 10/15] x86/boot: implement early command line parser in C
Current early command line parser implementation in assembler is very difficult to change to relocatable stuff using segment registers. This requires a lot of changes in very weird and fragile code. So, reimplement this functionality in C. This way code will be relocatable out of the box (without playing with segment registers) and much easier to maintain. Additionally, put all common cmdline.c and reloc.c definitions into defs.h header. This way we do not duplicate needlessly some stuff. And finally remove unused xen/include/asm-x86/config.h header from reloc.c dependencies. Suggested-by: Andrew CooperSigned-off-by: Daniel Kiper --- v6 - suggestions/fixes: - put common cmdline.c and reloc.c definitions into defs.h header (suggested by Jan Beulich), - use xen/include/xen/stdbool.h and bool type from it instead of own defined bool_t (suggested by Jan Beulich), - define delim_chars as constant (suggested by Jan Beulich), - properly align trampoline.S:early_boot_opts struct (suggested by Jan Beulich), - fix overflow check in strtoui() (suggested by Jan Beulich), - remove unused xen/include/asm-x86/config.h header from reloc.c dependencies, - improve commit message. v4 - suggestions/fixes: - move to stdcall calling convention (suggested by Jan Beulich), - define bool_t and use it properly (suggested by Jan Beulich), - put list of delimiter chars into static const char[] (suggested by Jan Beulich), - use strlen() instead of strlen_opt() (suggested by Jan Beulich), - change strtoi() to strtoui() and optimize it a bit (suggested by Jan Beulich), - define strchr() and use it in strtoui() (suggested by Jan Beulich), - optimize vga_parse() (suggested by Jan Beulich), - move !cmdline check from assembly to C (suggested by Jan Beulich), - remove my name from copyright (Oracle requirement) (suggested by Konrad Rzeszutek Wilk). v3 - suggestions/fixes: - optimize some code (suggested by Jan Beulich), - put VESA data into early_boot_opts_t members (suggested by Jan Beulich), - rename some functions and variables (suggested by Jan Beulich), - move around video.h include in xen/arch/x86/boot/trampoline.S (suggested by Jan Beulich), - fix coding style (suggested by Jan Beulich), - fix build with older GCC (suggested by Konrad Rzeszutek Wilk), - remove redundant comments (suggested by Jan Beulich), - add some comments - improve commit message (suggested by Jan Beulich). --- .gitignore |5 +- xen/arch/x86/Makefile |2 +- xen/arch/x86/boot/Makefile | 11 +- xen/arch/x86/boot/build32.mk |2 + xen/arch/x86/boot/cmdline.S| 367 xen/arch/x86/boot/cmdline.c| 339 + xen/arch/x86/boot/defs.h | 52 ++ xen/arch/x86/boot/edd.S|3 - xen/arch/x86/boot/head.S |8 + xen/arch/x86/boot/reloc.c | 13 +- xen/arch/x86/boot/trampoline.S | 14 ++ xen/arch/x86/boot/video.S |7 - 12 files changed, 429 insertions(+), 394 deletions(-) delete mode 100644 xen/arch/x86/boot/cmdline.S create mode 100644 xen/arch/x86/boot/cmdline.c create mode 100644 xen/arch/x86/boot/defs.h diff --git a/.gitignore b/.gitignore index cc64fc9..9aa47a1 100644 --- a/.gitignore +++ b/.gitignore @@ -247,9 +247,10 @@ xen/arch/arm/xen.lds xen/arch/x86/asm-offsets.s xen/arch/x86/boot/mkelf32 xen/arch/x86/xen.lds +xen/arch/x86/boot/cmdline.S xen/arch/x86/boot/reloc.S -xen/arch/x86/boot/reloc.bin -xen/arch/x86/boot/reloc.lnk +xen/arch/x86/boot/*.bin +xen/arch/x86/boot/*.lnk xen/arch/x86/efi.lds xen/arch/x86/efi/check.efi xen/arch/x86/efi/disabled diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index 2766bff..d8fe0f1 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -224,6 +224,6 @@ clean:: rm -f asm-offsets.s *.lds boot/*.o boot/*~ boot/core boot/mkelf32 rm -f $(BASEDIR)/.xen-syms.[0-9]* boot/.*.d rm -f $(BASEDIR)/.xen.efi.[0-9]* efi/*.efi efi/disabled efi/mkreloc - rm -f boot/reloc.S boot/reloc.lnk boot/reloc.bin + rm -f boot/cmdline.S boot/reloc.S boot/*.lnk boot/*.bin rm -f note.o $(MAKE) -f $(BASEDIR)/Rules.mk -C test clean diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile index 06893d8..4a1bc45 100644 --- a/xen/arch/x86/boot/Makefile +++ b/xen/arch/x86/boot/Makefile @@ -1,9 +1,16 @@ obj-bin-y += head.o -RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h $(BASEDIR)/include/xen/multiboot.h \ +DEFS_H_DEPS = $(BASEDIR)/include/xen/stdbool.h + +CMDLINE_DEPS = defs.h $(DEFS_H_DEPS) video.h + +RELOC_DEPS = defs.h $(DEFS_H_DEPS) $(BASEDIR)/include/xen/multiboot.h \ $(BASEDIR)/include/xen/multiboot2.h
[Xen-devel] [PATCH v6 06/15] x86: allow EFI reboot method neither on EFI platforms...
..nor EFI platforms with runtime services enabled. Suggested-by: Jan BeulichSigned-off-by: Daniel Kiper --- v6 - suggestions/fixes: - move this commit behind "efi: create efi_enabled()" commit (suggested by Jan Beulich). v5 - suggestions/fixes: - fix build error (suggested by Jan Beulich), - improve commit message. --- xen/arch/x86/shutdown.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c index 54c2c79..b429fd0 100644 --- a/xen/arch/x86/shutdown.c +++ b/xen/arch/x86/shutdown.c @@ -80,6 +80,9 @@ static void __init set_reboot_type(char *str) break; str++; } + +if ( reboot_type == BOOT_EFI && !efi_enabled(EFI_RS) ) +reboot_type = BOOT_INVALID; } custom_param("reboot", set_reboot_type); -- 1.7.10.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v6 04/15] x86: add multiboot2 protocol support
Add multiboot2 protocol support. Alter min memory limit handling as we now may not find it from either multiboot (v1) or multiboot2. This way we are laying the foundation for EFI + GRUB2 + Xen development. Signed-off-by: Daniel Kiper--- v6 - suggestions/fixes: - properly index multiboot2_tag_mmap_t.entries[] (suggested by Jan Beulich), - do not index mbi_out_mods[] beyond its end (suggested by Andrew Cooper), - reduce number of casts (suggested by Andrew Cooper and Jan Beulich), - add braces to increase code readability (suggested by Andrew Cooper). v5 - suggestions/fixes: - check multiboot2_tag_mmap_t.entry_size before multiboot2_tag_mmap_t.entries[] use (suggested by Jan Beulich), - properly index multiboot2_tag_mmap_t.entries[] (suggested by Jan Beulich), - use "type name[]" instad of "type name[0]" in xen/include/xen/multiboot2.h (suggested by Jan Beulich), - remove unneeded comment (suggested by Jan Beulich). v4 - suggestions/fixes: - avoid assembly usage in xen/arch/x86/boot/reloc.c, - fix boundary check issue and optimize for() loops in mbi2_mbi(), - move to stdcall calling convention, - remove unneeded typeof() from ALIGN_UP() macro (suggested by Jan Beulich), - add and use NULL definition in xen/arch/x86/boot/reloc.c (suggested by Jan Beulich), - do not read data beyond the end of multiboot2 information in xen/arch/x86/boot/head.S (suggested by Jan Beulich), - add :req to some .macro arguments (suggested by Jan Beulich), - use cmovcc if possible, - add .L to multiboot2_header_end label (suggested by Jan Beulich), - add .L to multiboot2_proto label (suggested by Jan Beulich), - improve label names (suggested by Jan Beulich). v3 - suggestions/fixes: - reorder reloc() arguments (suggested by Jan Beulich), - remove .L from multiboot2 header labels (suggested by Andrew Cooper, Jan Beulich and Konrad Rzeszutek Wilk), - take into account alignment when skipping multiboot2 fixed part (suggested by Konrad Rzeszutek Wilk), - create modules data if modules count != 0 (suggested by Jan Beulich), - improve macros (suggested by Jan Beulich), - reduce number of casts (suggested by Jan Beulich), - use const if possible (suggested by Jan Beulich), - drop static and __used__ attribute from reloc() (suggested by Jan Beulich), - remove isolated/stray __packed attribute from multiboot2_memory_map_t type definition (suggested by Jan Beulich), - reformat xen/include/xen/multiboot2.h (suggested by Konrad Rzeszutek Wilk), - improve comments (suggested by Konrad Rzeszutek Wilk), - remove hard tabs (suggested by Jan Beulich and Konrad Rzeszutek Wilk). v2 - suggestions/fixes: - generate multiboot2 header using macros (suggested by Jan Beulich), - improve comments (suggested by Jan Beulich), - simplify assembly in xen/arch/x86/boot/head.S (suggested by Jan Beulich), - do not include include/xen/compiler.h in xen/arch/x86/boot/reloc.c (suggested by Jan Beulich), - do not read data beyond the end of multiboot2 information (suggested by Jan Beulich). v2 - not fixed yet: - dynamic dependency generation for xen/arch/x86/boot/reloc.S; this requires more work; I am not sure that it pays because potential patch requires more changes than addition of just multiboot2.h to Makefile (suggested by Jan Beulich), - isolated/stray __packed attribute usage for multiboot2_memory_map_t (suggested by Jan Beulich). --- xen/arch/x86/boot/Makefile|3 +- xen/arch/x86/boot/head.S | 107 ++- xen/arch/x86/boot/reloc.c | 148 ++-- xen/arch/x86/x86_64/asm-offsets.c |9 ++ xen/include/xen/multiboot2.h | 169 + 5 files changed, 426 insertions(+), 10 deletions(-) create mode 100644 xen/include/xen/multiboot2.h diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile index 5fdb5ae..06893d8 100644 --- a/xen/arch/x86/boot/Makefile +++ b/xen/arch/x86/boot/Makefile @@ -1,6 +1,7 @@ obj-bin-y += head.o -RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h $(BASEDIR)/include/xen/multiboot.h +RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h $(BASEDIR)/include/xen/multiboot.h \ +$(BASEDIR)/include/xen/multiboot2.h head.o: reloc.S diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 6f2c668..383ff79 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -1,5 +1,6 @@ #include #include +#include #include #include #include @@ -19,6 +20,28 @@ #define BOOT_PSEUDORM_CS 0x0020 #define BOOT_PSEUDORM_DS 0x0028 +#define MB2_HT(name) (MULTIBOOT2_HEADER_TAG_##name) +#define MB2_TT(name)
[Xen-devel] [PATCH v6 07/15] efi: build xen.gz with EFI code
Build xen.gz with EFI code. We need this to support multiboot2 protocol on EFI platforms. If we wish to load non-ELF file using multiboot (v1) or multiboot2 then it must contain "linear" (or "flat") representation of code and data. This is requirement of both boot protocols. Currently, PE file contains many sections which are not "linear" (one after another without any holes) or even do not have representation in a file (e.g. BSS). From EFI point of view everything is OK and works. However, this file layout cannot be properly interpreted by multiboot protocols family. In theory there is a chance that we could build proper PE file (from multiboot protocols POV) using current build system. However, it means that xen.efi further diverge from Xen ELF file (in terms of contents and build method). On the other hand ELF has all needed properties. So, it means that this is good starting point for further development. Additionally, I think that this is also good starting point for further xen.efi code and build optimizations. It looks that there is a chance that finally we can generate xen.efi directly from Xen ELF using just simple objcopy or other tool. This way we will have one Xen binary which can be loaded by three boot protocols: EFI native loader, multiboot (v1) and multiboot2. Signed-off-by: Daniel KiperAcked-by: Jan Beulich --- v6 - suggestions/fixes: - improve efi_enabled() checks in efi_runtime_call() (suggested by Jan Beulich). v5 - suggestions/fixes: - properly calculate efi symbol address in xen/arch/x86/xen.lds.S (I hope that this change does not invalidate Jan's ACK). v4 - suggestions/fixes: - functions should return -ENOSYS instead of -EOPNOTSUPP if EFI runtime services are not available (suggested by Jan Beulich), - remove stale bits from xen/arch/x86/Makefile (suggested by Jan Beulich). v3 - suggestions/fixes: - check for EFI platform in EFI code (suggested by Jan Beulich), - fix Makefiles (suggested by Jan Beulich), - improve commit message (suggested by Jan Beulich). v2 - suggestions/fixes: - build EFI code only if it is supported in a given build environment (suggested by Jan Beulich). --- xen/arch/x86/Makefile |2 +- xen/arch/x86/efi/Makefile | 12 xen/arch/x86/xen.lds.S|4 ++-- xen/common/efi/boot.c |3 +++ xen/common/efi/runtime.c |9 + 5 files changed, 19 insertions(+), 11 deletions(-) diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index a4fe740..2766bff 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -223,7 +223,7 @@ efi/mkreloc: efi/mkreloc.c clean:: rm -f asm-offsets.s *.lds boot/*.o boot/*~ boot/core boot/mkelf32 rm -f $(BASEDIR)/.xen-syms.[0-9]* boot/.*.d - rm -f $(BASEDIR)/.xen.efi.[0-9]* efi/*.o efi/.*.d efi/*.efi efi/disabled efi/mkreloc + rm -f $(BASEDIR)/.xen.efi.[0-9]* efi/*.efi efi/disabled efi/mkreloc rm -f boot/reloc.S boot/reloc.lnk boot/reloc.bin rm -f note.o $(MAKE) -f $(BASEDIR)/Rules.mk -C test clean diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile index ad3fdf7..442f3fc 100644 --- a/xen/arch/x86/efi/Makefile +++ b/xen/arch/x86/efi/Makefile @@ -1,18 +1,14 @@ CFLAGS += -fshort-wchar -obj-y += stub.o - -create = test -e $(1) || touch -t 19990101 $(1) - efi := y$(shell rm -f disabled) efi := $(if $(efi),$(shell $(CC) $(filter-out $(CFLAGS-y) .%.d,$(CFLAGS)) -c check.c 2>disabled && echo y)) efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi check.o 2>disabled && echo y)) -efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call create,boot.init.o); $(call create,runtime.o))) - -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o buildid.o +efi := $(if $(efi),$(shell rm disabled)y) %.o: %.ihex $(OBJCOPY) -I ihex -O binary $< $@ -stub.o: $(extra-y) +obj-y := stub.o +obj-$(efi) := boot.init.o compat.o relocs-dummy.o runtime.o +extra-$(efi) += buildid.o nogcov-$(efi) += stub.o diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index d903c31..57f5368 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -271,10 +271,10 @@ SECTIONS .pad : { . = ALIGN(MB(16)); } :text -#else - efi = .; #endif + efi = DEFINED(efi) ? efi : .; + /* Sections to be discarded */ /DISCARD/ : { *(.exit.text) diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 56544dc..1ef5d0b 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -1251,6 +1251,9 @@ void __init efi_init_memory(void) } *extra, *extra_head = NULL; #endif +if ( !efi_enabled(EFI_BOOT) ) +return; + printk(XENLOG_INFO "EFI memory map:%s\n", map_bs ? " (mapping BootServices)" : ""); for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size ) diff --git a/xen/common/efi/runtime.c
[Xen-devel] [PATCH v6 03/15] x86/boot/reloc: rename some variables and rearrange code a bit
Replace mbi with mbi_out and mbi_old with mbi_in and rearrange code a bit to make it more readable. Additionally, this way multiboot (v1) protocol implementation and future multiboot2 protocol implementation will use the same variable naming convention. Signed-off-by: Daniel KiperAcked-by: Jan Beulich --- v4 - suggestions/fixes: - move to stdcall calling convention. v3 - suggestions/fixes: - improve commit message (suggested by Konrad Rzeszutek Wilk). v2 - suggestions/fixes: - extract this change from main mutliboot2 protocol implementation (suggested by Jan Beulich). --- xen/arch/x86/boot/reloc.c | 39 --- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c index 9053e2c..ea8cb37 100644 --- a/xen/arch/x86/boot/reloc.c +++ b/xen/arch/x86/boot/reloc.c @@ -65,45 +65,46 @@ static u32 copy_string(u32 src) return copy_mem(src, p - src + 1); } -multiboot_info_t __stdcall *reloc(u32 mbi_old, u32 trampoline) +multiboot_info_t __stdcall *reloc(u32 mbi_in, u32 trampoline) { -multiboot_info_t *mbi; int i; +multiboot_info_t *mbi_out; alloc = trampoline; -mbi = _p(copy_mem(mbi_old, sizeof(*mbi))); +mbi_out = _p(copy_mem(mbi_in, sizeof(*mbi_out))); -if ( mbi->flags & MBI_CMDLINE ) -mbi->cmdline = copy_string(mbi->cmdline); +if ( mbi_out->flags & MBI_CMDLINE ) +mbi_out->cmdline = copy_string(mbi_out->cmdline); -if ( mbi->flags & MBI_MODULES ) +if ( mbi_out->flags & MBI_MODULES ) { module_t *mods; -mbi->mods_addr = copy_mem(mbi->mods_addr, mbi->mods_count * sizeof(module_t)); +mbi_out->mods_addr = copy_mem(mbi_out->mods_addr, + mbi_out->mods_count * sizeof(module_t)); -mods = _p(mbi->mods_addr); +mods = _p(mbi_out->mods_addr); -for ( i = 0; i < mbi->mods_count; i++ ) +for ( i = 0; i < mbi_out->mods_count; i++ ) { if ( mods[i].string ) mods[i].string = copy_string(mods[i].string); } } -if ( mbi->flags & MBI_MEMMAP ) -mbi->mmap_addr = copy_mem(mbi->mmap_addr, mbi->mmap_length); +if ( mbi_out->flags & MBI_MEMMAP ) +mbi_out->mmap_addr = copy_mem(mbi_out->mmap_addr, mbi_out->mmap_length); -if ( mbi->flags & MBI_LOADERNAME ) -mbi->boot_loader_name = copy_string(mbi->boot_loader_name); +if ( mbi_out->flags & MBI_LOADERNAME ) +mbi_out->boot_loader_name = copy_string(mbi_out->boot_loader_name); /* Mask features we don't understand or don't relocate. */ -mbi->flags &= (MBI_MEMLIMITS | - MBI_CMDLINE | - MBI_MODULES | - MBI_MEMMAP | - MBI_LOADERNAME); +mbi_out->flags &= (MBI_MEMLIMITS | + MBI_CMDLINE | + MBI_MODULES | + MBI_MEMMAP | + MBI_LOADERNAME); -return mbi; +return mbi_out; } -- 1.7.10.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v6 02/15] x86/boot/reloc: create generic alloc and copy functions
Create generic alloc and copy functions. We need separate tools for memory allocation and copy to provide multiboot2 protocol support. Signed-off-by: Daniel Kiper--- v6 - suggestions/fixes: - reduce number of casts (suggested by Andrew Cooper and Jan Beulich). v4 - suggestions/fixes: - avoid assembly usage. v3 - suggestions/fixes: - use "g" constraint instead of "r" for alloc_mem() bytes argument (suggested by Jan Beulich). v2 - suggestions/fixes: - generalize new functions names (suggested by Jan Beulich), - reduce number of casts (suggested by Jan Beulich). --- xen/arch/x86/boot/reloc.c | 53 +++-- 1 file changed, 32 insertions(+), 21 deletions(-) diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c index 28c6cea..9053e2c 100644 --- a/xen/arch/x86/boot/reloc.c +++ b/xen/arch/x86/boot/reloc.c @@ -30,62 +30,73 @@ typedef unsigned int u32; #define ALIGN_UP(arg, align) \ (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) +#define _p(val)((void *)(unsigned long)(val)) + static u32 alloc; -static void *reloc_mbi_struct(void *old, unsigned int bytes) +static u32 alloc_mem(u32 bytes) { -void *new; +return alloc -= ALIGN_UP(bytes, 16); +} -alloc -= ALIGN_UP(bytes, 16); -new = (void *)alloc; +static u32 copy_mem(u32 src, u32 bytes) +{ +u32 dst, dst_ret; + +dst = alloc_mem(bytes); +dst_ret = dst; while ( bytes-- ) -*(char *)new++ = *(char *)old++; +*(char *)dst++ = *(char *)src++; -return (void *)alloc; +return dst_ret; } -static char *reloc_mbi_string(char *old) +static u32 copy_string(u32 src) { -char *p; -for ( p = old; *p != '\0'; p++ ) +u32 p; + +if ( !src ) +return 0; + +for ( p = src; *(char *)p != '\0'; p++ ) continue; -return reloc_mbi_struct(old, p - old + 1); + +return copy_mem(src, p - src + 1); } -multiboot_info_t __stdcall *reloc(multiboot_info_t *mbi_old, u32 trampoline) +multiboot_info_t __stdcall *reloc(u32 mbi_old, u32 trampoline) { multiboot_info_t *mbi; int i; alloc = trampoline; -mbi = reloc_mbi_struct(mbi_old, sizeof(*mbi)); +mbi = _p(copy_mem(mbi_old, sizeof(*mbi))); if ( mbi->flags & MBI_CMDLINE ) -mbi->cmdline = (u32)reloc_mbi_string((char *)mbi->cmdline); +mbi->cmdline = copy_string(mbi->cmdline); if ( mbi->flags & MBI_MODULES ) { -module_t *mods = reloc_mbi_struct( -(module_t *)mbi->mods_addr, mbi->mods_count * sizeof(module_t)); +module_t *mods; -mbi->mods_addr = (u32)mods; +mbi->mods_addr = copy_mem(mbi->mods_addr, mbi->mods_count * sizeof(module_t)); + +mods = _p(mbi->mods_addr); for ( i = 0; i < mbi->mods_count; i++ ) { if ( mods[i].string ) -mods[i].string = (u32)reloc_mbi_string((char *)mods[i].string); +mods[i].string = copy_string(mods[i].string); } } if ( mbi->flags & MBI_MEMMAP ) -mbi->mmap_addr = (u32)reloc_mbi_struct( -(memory_map_t *)mbi->mmap_addr, mbi->mmap_length); +mbi->mmap_addr = copy_mem(mbi->mmap_addr, mbi->mmap_length); if ( mbi->flags & MBI_LOADERNAME ) -mbi->boot_loader_name = (u32)reloc_mbi_string( -(char *)mbi->boot_loader_name); +mbi->boot_loader_name = copy_string(mbi->boot_loader_name); /* Mask features we don't understand or don't relocate. */ mbi->flags &= (MBI_MEMLIMITS | -- 1.7.10.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v6 01/15] x86: properly calculate ELF end of image address
Currently ELF end of image address is calculated using first line from "nm -nr xen/xen-syms" output. However, today usually it contains random symbol address not related to end of image in any way. So, it looks that for years that stuff have been working just by lucky coincidence. Hence, it have to be changed to something more reliable. So, let's take ELF end of image address by reading _end symbol address from nm output. Signed-off-by: Daniel Kiper--- xen/arch/x86/Makefile |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index d3875c5..a4fe740 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -91,7 +91,7 @@ endif $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) 0x10 \ - `$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'` + `$(NM) -nr $(TARGET)-syms | awk '$$3 == "_end" {print "0x"$$1}'` .PHONY: tests tests: -- 1.7.10.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v6 00/15] x86: multiboot2 protocol support
Hi, I am sending sixth version of multiboot2 protocol support for legacy BIOS and EFI platforms. This patch series release contains fixes for all known issues except one. During last review Jan pointed out that there is no memory size check for trampoline region returned by efi_multiboot2(). I investigated this issue and neighborhood quite deeply. Unfortunately I discovered a lot of similar issues in original early assembly code which does low memory allocations. E.g. there are a few arbitrary size checks without any explanations why this figures not another. Additionally, they take only into consideration area for trampoline though low memory is used for stack and boot data storage. I do not mention that there is no hard check which would fail if there is insufficient low memory, etc. So, I am going to describe all issues in separate thread (probably tomorrow) and hammer out relevant solutions. Probably relevant patches with fixes should be applied before this series but maybe not. Anyway, I hope that fixes for above described issues will have no big impact on this patch series. That is why I decided to post it without fixing low memory allocation issues. This way review can be done in parallel with discussion about possible fixes. The final goal is xen.efi binary file which could be loaded by EFI loader, multiboot (v1) protocol (only on legacy BIOS platforms) and multiboot2 protocol. This way we will have: - smaller Xen code base, - one code base for xen.gz and xen.efi, - one build method for xen.gz and xen.efi; xen.efi will be extracted from xen(-syms) file using objcopy or special custom tool, - xen.efi build will not so strongly depend on a given GCC and binutils version. Here is short list of changes since v5: - new patches: 01, 11, 12, 14, - changed patches: 02, 04-10, 13. I hope that (at least some) features provided by this patch series will be included in Xen 4.8 release in one way or another. If you are not interested in this patch series at all please drop me a line and I will remove you from distribution list. Daniel .gitignore|5 +- xen/arch/x86/Makefile |8 +- xen/arch/x86/Rules.mk |4 + xen/arch/x86/boot/Makefile| 12 +- xen/arch/x86/boot/build32.mk |2 + xen/arch/x86/boot/cmdline.S | 367 xen/arch/x86/boot/cmdline.c | 339 xen/arch/x86/boot/defs.h | 52 xen/arch/x86/boot/edd.S |3 - xen/arch/x86/boot/head.S | 536 ++ xen/arch/x86/boot/reloc.c | 223 +++--- xen/arch/x86/boot/trampoline.S| 21 +++- xen/arch/x86/boot/video.S |7 -- xen/arch/x86/boot/wakeup.S|4 +- xen/arch/x86/boot/x86_64.S| 44 +++ xen/arch/x86/dmi_scan.c |4 +- xen/arch/x86/domain_page.c|2 +- xen/arch/x86/efi/Makefile | 12 +- xen/arch/x86/efi/efi-boot.h | 60 -- xen/arch/x86/efi/stub.c | 46 ++- xen/arch/x86/mpparse.c|4 +- xen/arch/x86/setup.c | 36 +++--- xen/arch/x86/shutdown.c |5 +- xen/arch/x86/time.c |2 +- xen/arch/x86/x86_64/asm-offsets.c | 15 +++ xen/arch/x86/xen.lds.S| 10 +- xen/common/efi/boot.c | 68 ++- xen/common/efi/runtime.c | 23 +++- xen/common/version.c |2 +- xen/drivers/acpi/osl.c|2 +- xen/include/asm-x86/page.h|2 +- xen/include/xen/efi.h |9 +- xen/include/xen/multiboot2.h | 182 33 files changed, 1543 insertions(+), 568 deletions(-) Daniel Kiper (15): x86: properly calculate ELF end of image address x86/boot/reloc: create generic alloc and copy functions x86/boot/reloc: rename some variables and rearrange code a bit x86: add multiboot2 protocol support efi: create efi_enabled() x86: allow EFI reboot method neither on EFI platforms... efi: build xen.gz with EFI code x86/efi: create new early memory allocator x86: add multiboot2 protocol support for EFI platforms x86/boot: implement early command line parser in C x86: change default load address from 1 MiB to 2 MiB x86/setup: use XEN_IMG_OFFSET instead of... x86: make Xen early boot code relocatable x86/boot: rename sym_phys() to sym_offs() x86: add multiboot2 protocol support for relocatable images ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v6 05/15] efi: create efi_enabled()
First of all we need to differentiate between legacy BIOS and EFI platforms during runtime, not during build, because one image will have legacy and EFI code and can be executed on both platforms. Additionally, we need more fine grained knowledge about EFI environment and check for EFI platform and EFI loader separately to properly support multiboot2 protocol. In general Xen loaded by this protocol uses memory mappings and loaded modules in similar way to Xen loaded by multiboot (v1) protocol. Hence, create efi_enabled() which checks available features in efi_flags. This patch defines EFI_BOOT, EFI_LOADER and EFI_RS features. EFI_BOOT is equal to old efi_enabled == 1. EFI_RS ease control on runtime services usage. EFI_LOADER tells that Xen was loaded directly from EFI as PE executable. Suggested-by: Jan BeulichSigned-off-by: Daniel Kiper --- v6 - suggestions/fixes: - define efi_enabled() as "bool efi_enabled(unsigned int feature)" instead of "bool_t efi_enabled(int feature)" (suggested by Jan Beulich), - define efi_flags as unsigned int (suggested by Jan Beulich), - various minor cleanups and fixes (suggested by Jan Beulich). v5 - suggestions/fixes: - squash three patches into one (suggested by Jan Beulich), - introduce all features at once (suggested by Jan Beulich), - efi_enabled() returns bool_t instead of unsigned int (suggested by Jan Beulich), - update commit message. v4 - suggestions/fixes: - rename EFI_PLATFORM to EFI_BOOT (suggested by Jan Beulich), - move EFI_BOOT definition to efi struct definition (suggested by Jan Beulich), - remove unneeded efi.flags initialization (suggested by Jan Beulich), - use __set_bit() instead of set_bit() if possible (suggested by Jan Beulich), - do efi_enabled() cleanup (suggested by Jan Beulich), - improve comments (suggested by Jan Beulich), - improve commit message. v3 - suggestions/fixes: - define efi struct in xen/arch/x86/efi/stub.c in earlier patch (suggested by Jan Beulich), - improve comments (suggested by Jan Beulich), - improve commit message (suggested by Jan Beulich). --- xen/arch/x86/dmi_scan.c|4 ++-- xen/arch/x86/domain_page.c |2 +- xen/arch/x86/efi/stub.c|7 --- xen/arch/x86/mpparse.c |4 ++-- xen/arch/x86/setup.c | 12 +++- xen/arch/x86/shutdown.c|2 +- xen/arch/x86/time.c|2 +- xen/common/efi/boot.c | 19 +++ xen/common/efi/runtime.c | 14 -- xen/common/version.c |2 +- xen/drivers/acpi/osl.c |2 +- xen/include/xen/efi.h |8 ++-- 12 files changed, 49 insertions(+), 29 deletions(-) diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c index b049e31..8dcb640 100644 --- a/xen/arch/x86/dmi_scan.c +++ b/xen/arch/x86/dmi_scan.c @@ -238,7 +238,7 @@ const char *__init dmi_get_table(paddr_t *base, u32 *len) { static unsigned int __initdata instance; - if (efi_enabled) { + if (efi_enabled(EFI_BOOT)) { if (efi_smbios3_size && !(instance & 1)) { *base = efi_smbios3_address; *len = efi_smbios3_size; @@ -696,7 +696,7 @@ static void __init dmi_decode(struct dmi_header *dm) void __init dmi_scan_machine(void) { - if ((!efi_enabled ? dmi_iterate(dmi_decode) : + if ((!efi_enabled(EFI_BOOT) ? dmi_iterate(dmi_decode) : dmi_efi_iterate(dmi_decode)) == 0) dmi_check_system(dmi_blacklist); else diff --git a/xen/arch/x86/domain_page.c b/xen/arch/x86/domain_page.c index d86f8fe..7541b91 100644 --- a/xen/arch/x86/domain_page.c +++ b/xen/arch/x86/domain_page.c @@ -36,7 +36,7 @@ static inline struct vcpu *mapcache_current_vcpu(void) * domain's page tables but current may point at another domain's VCPU. * Return NULL as though current is not properly set up yet. */ -if ( efi_enabled && efi_rs_using_pgtables() ) +if ( efi_enabled(EFI_RS) && efi_rs_using_pgtables() ) return NULL; /* diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c index 07c2bd0..0bfaa74 100644 --- a/xen/arch/x86/efi/stub.c +++ b/xen/arch/x86/efi/stub.c @@ -4,9 +4,10 @@ #include #include -#ifndef efi_enabled -const bool_t efi_enabled = 0; -#endif +bool efi_enabled(unsigned int feature) +{ +return false; +} void __init efi_init_memory(void) { } diff --git a/xen/arch/x86/mpparse.c b/xen/arch/x86/mpparse.c index ef6557c..c3d5bdc 100644 --- a/xen/arch/x86/mpparse.c +++ b/xen/arch/x86/mpparse.c @@ -564,7 +564,7 @@ static inline void __init construct_default_ISA_mptable(int mpc_default_type) static __init void efi_unmap_mpf(void) { - if (efi_enabled) + if (efi_enabled(EFI_BOOT)) clear_fixmap(FIX_EFI_MPF); } @@ -722,7 +722,7 @@
[Xen-devel] [ovmf test] 100894: all pass - PUSHED
flight 100894 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/100894/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 8c0b0b34f7875571ee9d3a2a1a28484cef36d969 baseline version: ovmf 5654835bd1381dfad9483b767e55db7caaeec907 Last test of basis 100890 2016-09-12 10:44:23 Z0 days Testing same since 100894 2016-09-12 13:14:28 Z0 days1 attempts People who touched revisions under test: Laszlo ErsekThomas Huth 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 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 : + branch=ovmf + revision=8c0b0b34f7875571ee9d3a2a1a28484cef36d969 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x '!=' x/home/osstest/repos/lock ']' ++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock ++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push ovmf 8c0b0b34f7875571ee9d3a2a1a28484cef36d969 + branch=ovmf + revision=8c0b0b34f7875571ee9d3a2a1a28484cef36d969 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']' + . ./cri-common ++ . ./cri-getconfig ++ umask 002 + select_xenbranch + case "$branch" in + tree=ovmf + xenbranch=xen-unstable + '[' xovmf = xlinux ']' + linuxbranch= + '[' x = x ']' + qemuubranch=qemu-upstream-unstable + select_prevxenbranch ++ ./cri-getprevxenbranch xen-unstable + prevxenbranch=xen-4.7-testing + '[' x8c0b0b34f7875571ee9d3a2a1a28484cef36d969 = x ']' + : tested/2.6.39.x + . ./ap-common ++ : osst...@xenbits.xen.org +++ getconfig OsstestUpstream +++ perl -e ' use Osstest; readglobalconfig(); print $c{"OsstestUpstream"} or die $!; ' ++ : ++ : git://xenbits.xen.org/xen.git ++ : osst...@xenbits.xen.org:/home/xen/git/xen.git ++ : git://xenbits.xen.org/qemu-xen-traditional.git ++ : git://git.kernel.org ++ : git://git.kernel.org/pub/scm/linux/kernel/git ++ : git ++ : git://xenbits.xen.org/xtf.git ++ : osst...@xenbits.xen.org:/home/xen/git/xtf.git ++ : git://xenbits.xen.org/xtf.git ++ : git://xenbits.xen.org/libvirt.git ++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git ++ : git://xenbits.xen.org/libvirt.git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/rumprun.git ++ : git://git.seabios.org/seabios.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/seabios.git ++ : git://xenbits.xen.org/osstest/seabios.git ++ : https://github.com/tianocore/edk2.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/linux-firmware.git ++ : osst...@xenbits.xen.org:/home/osstest/ext/linux-firmware.git ++ : git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
Re: [Xen-devel] Getting started with Xen Project
Okay. I have a basic knowledge of x86 assembly language. I will need to get familiar with coding in OVMF. I feel that with some assignments and resources (like a minor bug in some OVMF code, or tutorial links) and guidance, a prospective mentee might reach the level required by the project. I would like to know the views of the people looking into the project. Regards, Aashaka On Mon, Sep 12, 2016 at 6:00 PM, Roger Pau Monnéwrote: > On Sat, Sep 10, 2016 at 03:51:33PM +0530, Aashaka Shah wrote: > > Hello! I am Aashaka. I like computer architecture and have a good > knowledge > > of C. While browsing Outreachy projects, I came across the project about > > adding PVH support to OVMF binaries. Since it looked interesting, I > started > > reading the mailing-list link mentioned in the project description and > also > > the Beginners section in navigation by audience column. > > > > Currently, I have very little, in fact, no, experience developing with > the > > Xen Project. I would like to know how to continue with getting familiar > > with the terms, setting up the codebase and making contributions. > > Hello, > > Anthony (who I've added to the Cc) already started looking into this, so I > think you should better coordinate with him regarding this project. > > Please note that this project will involve quite a lot of low-level coding > in OVMF, and some knowledge of x86 assembly is required, again Anthony will > be able to provide more insight, and whether this is still a suitable > project for OPW or not. > > Roger. > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [qemu-mainline test] 100892: regressions - FAIL
flight 100892 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/100892/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-libvirt-qcow2 6 xen-boot fail REGR. vs. 100877 Regressions which are regarded as allowable (not blocking): test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 100877 test-amd64-amd64-xl-rtds 9 debian-install fail like 100877 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stopfail like 100877 Tests which did not succeed, but are not blocking: test-armhf-armhf-xl-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl-xsm 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 guest-saverestorefail never pass test-armhf-armhf-xl-credit2 12 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 12 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt 14 guest-saverestorefail never pass test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail never pass test-armhf-armhf-xl-rtds 12 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 saverestore-support-checkfail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass test-amd64-i386-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-i386-libvirt 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-amd64-xl-pvh-amd 11 guest-start fail never pass test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail never pass test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-amd64-amd64-xl-pvh-intel 11 guest-start fail never pass test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2 fail never pass version targeted for testing: qemuuc569c537e5c60ee9b9ed92b7a57d766c78b71318 baseline version: qemuuc2a57aae9a1c3dd7de77daf5478df10379aeeebf Last test of basis 100877 2016-09-10 15:16:34 Z2 days Testing same since 100892 2016-09-12 12:13:11 Z0 days1 attempts People who touched revisions under test: Greg KurzGreg Kurz Jason Wang Ladi Prosek Longpeng(Mike) Marcel Apfelbaum Michael S. Tsirkin Peter Maydell Stefan Hajnoczi Thomas Huth jobs: build-amd64-xsm pass build-armhf-xsm pass build-i386-xsm pass build-amd64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-xl pass test-armhf-armhf-xl
[Xen-devel] [ovmf baseline-only test] 67699: all pass
This run is configured for baseline tests only. flight 67699 ovmf real [real] http://osstest.xs.citrite.net/~osstest/testlogs/logs/67699/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 5654835bd1381dfad9483b767e55db7caaeec907 baseline version: ovmf e9fec7326ad2f27fe368c830da055d1044b18e95 Last test of basis67697 2016-09-12 07:46:25 Z0 days Testing same since67699 2016-09-12 12:47:30 Z0 days1 attempts People who touched revisions under test: Dandan Bijobs: 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 pass sg-report-flight on osstest.xs.citrite.net logs: /home/osstest/logs images: /home/osstest/images Logs, config files, etc. are available at http://osstest.xs.citrite.net/~osstest/testlogs/logs Test harness code can be found at http://xenbits.xensource.com/gitweb?p=osstest.git;a=summary Push not applicable. commit 5654835bd1381dfad9483b767e55db7caaeec907 Author: Dandan Bi Date: Mon Sep 5 14:55:49 2016 +0800 MdeModulePkg/HiiDB: Handle the "" tag in correctly This patch is to fix the incorrect logic when handling the "" tag in . 1. In UEFI spec, the "" tag is in upper case, but using the lower case in current codes by mistake. 2. The logic in checking the ReadOnly flag is not correct. Whether having "" tag must be consistent with the result of "ExtractReadOnlyFromOpCode" function. Cc: Liming Gao Cc: Eric Dong Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Dandan Bi Reviewed-by: Eric Dong Reviewed-by: Liming Gao commit 7d467158e095a8f231f1a65c9f7ca3627debf763 Author: Dandan Bi Date: Thu Sep 8 15:04:51 2016 +0800 MdeModulePkg/UiApp: Fix incorrect question id For a question, its question id can not be zero. This patch is to fix the issue that using zero as question id. Cc: Liming Gao Cc: Eric Dong Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Dandan Bi Reviewed-by: Eric Dong Reviewed-by: Liming Gao ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] PC8 Residency on Broadwell hardware
On Sep 12, 2016, at 2:00 AM, Jan Beulich> wrote: On 12.09.16 at 10:47, > wrote: c/s 350bc1a9d4 "x86: support newer Intel CPU models" changed the set of MSRs read by Xeon Broadwell hardware (specifically, model 79 / 0x47). Rereading the manual, it does indeed indicate that this MSR is available. However, experimentally it is not. All Broadwell hardware XenServer has (both SDPs and production systems) reliably take a #GP fault when trying to read this MSR. Haswell hardware appears fine (and indeed, was reading that MSR before). Where did you find the info? I think you are talking about MSR_PKG_C8_RESIDENCY (630H). The SDM says (35.13): "Processors with signatures 06_3DH and 06_47H support the MSR interfaces listed in Table 35-18, Table 35-19, Table 35-20, Table 35-23, Table 35-27, Table 35-28, Table 35-32, and Table 35-33.” Intel: Please can you confirm whether the documentation is correct? If it is, do you have any idea why the MSR might not be available? I can't spot anything relevant in the platform errata documents. And indeed I had requested this same information already well over a week ago, without having heard back. Sorry, it seems that the email was buried in the pile of the emails that I received while I was in Toronto. Jan --- Jun Intel Open Source Technology Center ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 04/24] xen: credit2: properly schedule migration of a running vcpu.
On 17/08/16 18:18, Dario Faggioli wrote: > If wanting to migrate a vcpu that is actually running, > we need to ask the scheduler to chime in as soon as > possible, to have the vcpu itself stopped and actually > moved. > > Make sure this happens by, after setting all the relevant > flags, raising the scheduler softirq. > > Signed-off-by: Dario FaggioliAcked-by: George Dunlap > --- > Cc: George Dunlap > Cc: Anshul Makkar > --- > xen/common/sched_credit2.c |1 + > 1 file changed, 1 insertion(+) > > diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c > index a5a744f..12dfd20 100644 > --- a/xen/common/sched_credit2.c > +++ b/xen/common/sched_credit2.c > @@ -1667,6 +1667,7 @@ static void migrate(const struct scheduler *ops, > svc->migrate_rqd = trqd; > __set_bit(_VPF_migrating, >vcpu->pause_flags); > __set_bit(__CSFLAG_runq_migrate_request, >flags); > +cpu_raise_softirq(svc->vcpu->processor, SCHEDULE_SOFTIRQ); > SCHED_STAT_CRANK(migrate_requested); > } > else > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC] x86/vm_event: Allow returning i-cache for emulation
On Mon, Sep 12, 2016 at 10:02 AM, Jan Beulichwrote: On 12.09.16 at 17:48, wrote: >> On Mon, Sep 12, 2016 at 8:56 AM, Jan Beulich wrote: >>> >>> On 09.09.16 at 17:41, wrote: >>> > @@ -1783,7 +1786,14 @@ static int _hvm_emulate_one(struct >>> hvm_emulate_ctxt *hvmemul_ctxt, >>> > pfec |= PFEC_user_mode; >>> > >>> > hvmemul_ctxt->insn_buf_eip = regs->eip; >>> > -if ( !vio->mmio_insn_bytes ) >>> > + >>> > +if ( unlikely(hvmemul_ctxt->set_context_insn) ) >>> > +{ >>> > +memcpy(hvmemul_ctxt->insn_buf, curr->arch.vm_event->emul_ >>> data.data, >>> > + curr->arch.vm_event->emul_data.size); >>> > +hvmemul_ctxt->insn_buf_bytes = curr->arch.vm_event->emul_ >>> data.size; >>> > +} >>> > +else if ( !vio->mmio_insn_bytes ) >>> >>> I'm not sure about this ordering: Do you really mean to allow an >>> external entity to override insn bytes e.g. in an MMIO retry, i.e. >>> allowing the retried insn to be different from the original one? >>> >> >> I'm not sure but there shouldn't be INT3's coming from MMIO, right? So we >> would not have a response-opportunity to override it. > > Can't you modify insns at any time from the monitoring app? If > so, wouldn't there be a chance for things to change between > the original insn invocation and the retry (after qemu completed > the request to it)? No, right now it's only allowed in response to an INT3 event as that's really the only case where I see this being needed. We could extend it's scope so we can trigger it in response to any kind of vm_event, but right now I don't see that being necessary. > >>> And additionally I think you need to deal with the case of the >>> supplied data not being a full insn. There shouldn't be any >>> fetching from guest memory in that case imo, emulation should >>> just fail. >>> >> >> So the idea was to allow partial "masking" of the i-cache. For example, if >> all I want to hide is the 0xCC then it's enough to return 1 byte in >> vm_event, the rest can be (and already is) grabbed from memory. > > Oh, interesting. That makes it even more problematic, imo. Can you > ensure that your patching any whatever the guest may do to its own > insn stream actually remains consistent, even in situations like the > insn crossing a page boundary? IOW what you suggest seems pretty > fragile to me, but would in any case need spelling out very clearly. Ah, good point. With LibVMI we already handle reading a buffer with VA addressing even if the PA layout crosses page boundaries, so it's going to be simpler and safer to have the client return a full buffer as you suggested. Thanks, Tamas ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 2/6] x86/domctl: Fix TOCTOU race with the use of XEN_DOMCTL_getvcpuextstate
A toolstack must call XEN_DOMCTL_getvcpuextstate twice; first to find the size of the buffer to use, and a second time to get the actual content. The reported size was based on v->arch.xcr0_accum, but a guest which extends its xcr0_accum between the two hypercalls will cause the toolstack to fail the evc->size != size check, as the provided buffer is now too small. This causes a hard error during the final phase of migration. Instead, return a size based on xfeature_mask, which is the maximum size Xen will ever permit. The hypercall must now tolerate a toolstack-provided buffer which is overly large (for the case where a guest isn't using all available xsave states), and should write back how much data was actually written into the buffer. As the query for size now has no dependence on vcpu state, the vcpu_pause() can be omitted for a small performance improvement. Signed-off-by: Andrew CooperReviewed-by: Jan Beulich --- xen/arch/x86/domctl.c | 20 +++- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index c9355ce..815bd33 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -1054,19 +1054,25 @@ long arch_do_domctl( unsigned int size; ret = 0; -vcpu_pause(v); -size = PV_XSAVE_SIZE(v->arch.xcr0_accum); if ( (!evc->size && !evc->xfeature_mask) || guest_handle_is_null(evc->buffer) ) { +/* + * A query for the size of buffer to use. Must return the + * maximum size we ever might hand back to userspace, bearing + * in mind that the vcpu might increase its xcr0_accum between + * this query for size, and the following query for data. + */ evc->xfeature_mask = xfeature_mask; -evc->size = size; -vcpu_unpause(v); +evc->size = PV_XSAVE_SIZE(xfeature_mask); goto vcpuextstate_out; } -if ( evc->size != size || evc->xfeature_mask != xfeature_mask ) +vcpu_pause(v); +size = PV_XSAVE_SIZE(v->arch.xcr0_accum); + +if ( evc->size < size || evc->xfeature_mask != xfeature_mask ) ret = -EINVAL; if ( !ret && copy_to_guest_offset(evc->buffer, offset, @@ -1103,6 +1109,10 @@ long arch_do_domctl( } vcpu_unpause(v); + +/* Specify how much data we actually wrote into the buffer. */ +if ( !ret ) +evc->size = size; } else { -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 6/6] x86/xstate: Fix latent bugs in compress_xsave_states()
compress_xsave_states() mustn't read xstate_bv or xcomp_bv before first confirming that the input buffer is large enough. It also doesn't cope with compressed input. Make all of these problems the callers responsbility to ensure. Simplify the decompression logic by inlining get_xsave_addr(). As xstate_bv is previously validated, dest won't ever been NULL. Signed-off-by: Andrew Cooper--- CC: Jan Beulich v2: * Inline get_xsave_addr() to simplify the logic * Drop the TODO --- xen/arch/x86/xstate.c | 45 +++-- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c index ed9c4c7..9be98e6 100644 --- a/xen/arch/x86/xstate.c +++ b/xen/arch/x86/xstate.c @@ -154,15 +154,6 @@ static void setup_xstate_comp(uint16_t *comp_offsets, ASSERT(offset <= xsave_cntxt_size); } -static void *get_xsave_addr(struct xsave_struct *xsave, -const uint16_t *comp_offsets, -unsigned int xfeature_idx) -{ -ASSERT(xsave_area_compressed(xsave)); -return (1ul << xfeature_idx) & xsave->xsave_hdr.xstate_bv ? - (void *)xsave + comp_offsets[xfeature_idx] : NULL; -} - /* * Serialise a vcpus xsave state into a representation suitable for the * toolstack. @@ -229,14 +220,28 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size) } } +/* + * Deserialise a toolstack's xsave state representation suitably for a vcpu. + * + * Internally a vcpus xsave state may be compressed or uncompressed, depending + * on the features in use, but the ABI with the toolstack is strictly + * uncompressed. + * + * It is the callers responsibility to ensure that the source buffer contains + * xsave state, is uncompressed, and is exactly the right size. + */ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size) { struct xsave_struct *xsave = v->arch.xsave_area; +void *dest; uint16_t comp_offsets[sizeof(xfeature_mask)*8]; -u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv; -u64 valid; +u64 xstate_bv, valid; -ASSERT(!xsave_area_compressed(src)); +BUG_ON(!v->arch.xcr0_accum); +BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum)); +BUG_ON(xsave_area_compressed(src)); + +xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv; if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) ) { @@ -260,18 +265,22 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size) * Copy each region from the non-compacted offset to the * possibly compacted offset. */ +dest = xsave; valid = xstate_bv & ~XSTATE_FP_SSE; while ( valid ) { u64 feature = valid & -valid; unsigned int index = fls(feature) - 1; -void *dest = get_xsave_addr(xsave, comp_offsets, index); -if ( dest ) -{ -ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size); -memcpy(dest, src + xstate_offsets[index], xstate_sizes[index]); -} +/* + * We previously verified xstate_bv. If we don't have valid + * comp_offset[] information, something is very broken. + */ +BUG_ON(!comp_offsets[index]); +BUG_ON((xstate_offsets[index] + xstate_sizes[index]) > size); + +memcpy(dest + comp_offsets[index], src + xstate_offsets[index], + xstate_sizes[index]); valid &= ~feature; } -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 3/6] x86/domctl: Simplfy XEN_DOMCTL_getvcpuextstate when xsave is not in use
Older guests will not use xsave even if it is available. As such, their xcr0_accum will be 0 at the point of migrate. If it is empty, forgo the memory allocation and serialisation into a zero-length buffer. Signed-off-by: Andrew CooperReviewed-by: Jan Beulich --- xen/arch/x86/domctl.c | 13 +++-- 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index 815bd33..5aa9f3a 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -1087,11 +1087,13 @@ long arch_do_domctl( ret = -EFAULT; offset += sizeof(v->arch.xcr0_accum); -if ( !ret ) + +/* Serialise xsave state, if there is any. */ +if ( !ret && size > PV_XSAVE_HDR_SIZE ) { -void *xsave_area; +unsigned int xsave_size = size - PV_XSAVE_HDR_SIZE; +void *xsave_area = xmalloc_bytes(xsave_size); -xsave_area = xmalloc_bytes(size); if ( !xsave_area ) { ret = -ENOMEM; @@ -1099,11 +1101,10 @@ long arch_do_domctl( goto vcpuextstate_out; } -expand_xsave_states(v, xsave_area, -size - PV_XSAVE_HDR_SIZE); +expand_xsave_states(v, xsave_area, xsave_size); if ( copy_to_guest_offset(evc->buffer, offset, xsave_area, - size - PV_XSAVE_HDR_SIZE) ) + xsave_size) ) ret = -EFAULT; xfree(xsave_area); } -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 5/6] x86/domctl: Fix migration of guests which are not using xsave
c/s da62246e "x86/xsaves: enable xsaves/xrstors/xsavec in xen" broke migration of PV guests which were not using xsave. In such a case, compress_xsave_states() gets passed a zero length buffer. The first thing it tries to do is ASSERT() on user-provided data, if it hadn't already wandered off the end of the buffer to do so. Perform more verification of the input buffer before passing it to compress_xsave_states(). This involves making xsave_area_compressed() public. Similar problems exist on the HVM side, so make equivalent adjustments there. This doesn't manifest in general, as hvm_save_cpu_xsave_states() elides the entire record if xsave isn't used, but is a problem if a caller were to construct an xsave record manually. Signed-off-by: Andrew CooperReviewed-by: Jan Beulich --- v2: * Use a shadow variable in hvm_load_cpu_xsave_states() rather than mutating the stream itself. Make desc const while at it. * Alter the alignment of xsave_area_compressed() --- xen/arch/x86/domctl.c| 12 +--- xen/arch/x86/hvm/hvm.c | 27 ++- xen/arch/x86/xstate.c| 6 -- xen/include/asm-x86/xstate.h | 6 ++ 4 files changed, 37 insertions(+), 14 deletions(-) diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index 5aa9f3a..2a2fe04 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -1158,7 +1158,15 @@ long arch_do_domctl( goto vcpuextstate_out; } -if ( evc->size <= PV_XSAVE_SIZE(_xcr0_accum) ) +if ( evc->size == PV_XSAVE_HDR_SIZE ) +; /* Nothing to restore. */ +else if ( evc->size < PV_XSAVE_HDR_SIZE + XSTATE_AREA_MIN_SIZE ) +ret = -EINVAL; /* Can't be legitimate data. */ +else if ( xsave_area_compressed(_xsave_area) ) +ret = -EOPNOTSUPP; /* Don't support compressed data. */ +else if ( evc->size != PV_XSAVE_SIZE(_xcr0_accum) ) +ret = -EINVAL; /* Not legitimate data. */ +else { vcpu_pause(v); v->arch.xcr0 = _xcr0; @@ -1169,8 +1177,6 @@ long arch_do_domctl( evc->size - PV_XSAVE_HDR_SIZE); vcpu_unpause(v); } -else -ret = -EINVAL; xfree(receive_buf); } diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index ca96643..7bad845 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -1265,8 +1265,8 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h) int err; struct vcpu *v; struct hvm_hw_cpu_xsave *ctxt; -struct hvm_save_descriptor *desc; -unsigned int i, desc_start; +const struct hvm_save_descriptor *desc; +unsigned int i, desc_start, desc_length; /* Which vcpu is this? */ vcpuid = hvm_load_instance(h); @@ -1325,7 +1325,8 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h) return err; } size = HVM_CPU_XSAVE_SIZE(ctxt->xcr0_accum); -if ( desc->length > size ) +desc_length = desc->length; +if ( desc_length > size ) { /* * Xen 4.3.0, 4.2.3 and older used to send longer-than-needed @@ -1345,6 +1346,23 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h) printk(XENLOG_G_WARNING "HVM%d.%u restore mismatch: xsave length %#x > %#x\n", d->domain_id, vcpuid, desc->length, size); +/* Rewind desc_length to ignore the extraneous zeros. */ +desc_length = size; +} + +if ( xsave_area_compressed((const void *)>save_area) ) +{ +printk(XENLOG_G_WARNING + "HVM%d.%u restore: compressed xsave state not supported\n", + d->domain_id, vcpuid); +return -EOPNOTSUPP; +} +else if ( desc_length != size ) +{ +printk(XENLOG_G_WARNING + "HVM%d.%u restore mismatch: xsave length %#x != %#x\n", + d->domain_id, vcpuid, desc_length, size); +return -EINVAL; } /* Checking finished */ @@ -1353,8 +1371,7 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h) if ( ctxt->xcr0_accum & XSTATE_NONLAZY ) v->arch.nonlazy_xstate_used = 1; compress_xsave_states(v, >save_area, - min(desc->length, size) - - offsetof(struct hvm_hw_cpu_xsave,save_area)); + size - offsetof(struct hvm_hw_cpu_xsave, save_area)); return 0; } diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c index 2684190..ed9c4c7 100644 --- a/xen/arch/x86/xstate.c +++ b/xen/arch/x86/xstate.c @@ -86,12 +86,6 @@ uint64_t get_msr_xss(void) return this_cpu(xss); } -static bool_t xsave_area_compressed(const struct
[Xen-devel] [PATCH v2 1/6] x86/domctl: Introduce PV_XSAVE_HDR_SIZE and remove its opencoding
Also remove opencoding of PV_XSAVE_SIZE(). Undefine both when they are done with. No functional change. Signed-off-by: Andrew CooperReviewed-by: Jan Beulich --- xen/arch/x86/domctl.c | 21 - 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index a904fd6..c9355ce 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -1037,7 +1037,8 @@ long arch_do_domctl( struct vcpu *v; uint32_t offset = 0; -#define PV_XSAVE_SIZE(xcr0) (2 * sizeof(uint64_t) + xstate_ctxt_size(xcr0)) +#define PV_XSAVE_HDR_SIZE (2 * sizeof(uint64_t)) +#define PV_XSAVE_SIZE(xcr0) (PV_XSAVE_HDR_SIZE + xstate_ctxt_size(xcr0)) ret = -ESRCH; if ( (evc->vcpu >= d->max_vcpus) || @@ -1093,10 +1094,10 @@ long arch_do_domctl( } expand_xsave_states(v, xsave_area, -size - 2 * sizeof(uint64_t)); +size - PV_XSAVE_HDR_SIZE); if ( copy_to_guest_offset(evc->buffer, offset, xsave_area, - size - 2 * sizeof(uint64_t)) ) + size - PV_XSAVE_HDR_SIZE) ) ret = -EFAULT; xfree(xsave_area); } @@ -1110,9 +,8 @@ long arch_do_domctl( const struct xsave_struct *_xsave_area; ret = -EINVAL; -if ( evc->size < 2 * sizeof(uint64_t) || - evc->size > 2 * sizeof(uint64_t) + - xstate_ctxt_size(xfeature_mask) ) +if ( evc->size < PV_XSAVE_HDR_SIZE || + evc->size > PV_XSAVE_SIZE(xfeature_mask) ) goto vcpuextstate_out; receive_buf = xmalloc_bytes(evc->size); @@ -1131,11 +1131,11 @@ long arch_do_domctl( _xcr0 = *(uint64_t *)receive_buf; _xcr0_accum = *(uint64_t *)(receive_buf + sizeof(uint64_t)); -_xsave_area = receive_buf + 2 * sizeof(uint64_t); +_xsave_area = receive_buf + PV_XSAVE_HDR_SIZE; if ( _xcr0_accum ) { -if ( evc->size >= 2 * sizeof(uint64_t) + XSTATE_AREA_MIN_SIZE ) +if ( evc->size >= PV_XSAVE_HDR_SIZE + XSTATE_AREA_MIN_SIZE ) ret = validate_xstate(_xcr0, _xcr0_accum, &_xsave_area->xsave_hdr); } @@ -1155,7 +1155,7 @@ long arch_do_domctl( if ( _xcr0_accum & XSTATE_NONLAZY ) v->arch.nonlazy_xstate_used = 1; compress_xsave_states(v, _xsave_area, - evc->size - 2 * sizeof(uint64_t)); + evc->size - PV_XSAVE_HDR_SIZE); vcpu_unpause(v); } else @@ -1164,6 +1164,9 @@ long arch_do_domctl( xfree(receive_buf); } +#undef PV_XSAVE_HDR_SIZE +#undef PV_XSAVE_SIZE + vcpuextstate_out: if ( domctl->cmd == XEN_DOMCTL_getvcpuextstate ) copyback = 1; -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 0/6] Fix multiple issues with xsave state handling on migrate
Patch 5 is the primary bugfix of this series, which is broken in Xen 4.7 as well as master. There are multiple latent security issues which would be exposed at the point support for the first compressed xsave state was added, but are in currently-dead code. Changes from v1: * Inline and remove get_xsave_addr(). This simplifies the logic. * Whitespace & alignment fixes. * Use a shadow variable in hvm_load_cpu_xsave_states() * Drop the TODO BUG_ON() from compress_xsave_states() Andrew Cooper (6): x86/domctl: Introduce PV_XSAVE_HDR_SIZE and remove its opencoding x86/domctl: Fix TOCTOU race with the use of XEN_DOMCTL_getvcpuextstate x86/domctl: Simplfy XEN_DOMCTL_getvcpuextstate when xsave is not in use x86/xstate: Fix latent bugs in expand_xsave_states() x86/domctl: Fix migration of guests which are not using xsave x86/xstate: Fix latent bugs in compress_xsave_states() xen/arch/x86/domctl.c| 62 --- xen/arch/x86/hvm/hvm.c | 27 +++--- xen/arch/x86/xstate.c| 87 xen/include/asm-x86/xstate.h | 6 +++ 4 files changed, 124 insertions(+), 58 deletions(-) -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 4/6] x86/xstate: Fix latent bugs in expand_xsave_states()
Without checking the size input, the memcpy() for the uncompressed path might read off the end of the vcpu's xsave_area. Both callers pass the approprite size, so hold them to it with a BUG_ON(). The compressed path is currently dead code, but its attempt to avoid leaking uninitalised data was incomplete. Work around this by zeroing the whole rest of the buffer before decompression. The loop skips all bits which aren't set in xstate_bv, meaning that the memset() was dead code. The logic is more obvious with get_xsave_addr() expanded inline, allowing for quite a lot of simplification, including all the NULL pointer logic. Signed-off-by: Andrew Cooper--- CC: Jan Beulich v2: * get_xsave_addr() expanded inline to simplify the logic substantially. --- xen/arch/x86/xstate.c | 36 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c index 6e4a0d3..2684190 100644 --- a/xen/arch/x86/xstate.c +++ b/xen/arch/x86/xstate.c @@ -169,13 +169,30 @@ static void *get_xsave_addr(struct xsave_struct *xsave, (void *)xsave + comp_offsets[xfeature_idx] : NULL; } +/* + * Serialise a vcpus xsave state into a representation suitable for the + * toolstack. + * + * Internally a vcpus xsave state may be compressed or uncompressed, depending + * on the features in use, but the ABI with the toolstack is strictly + * uncompressed. + * + * It is the callers responsibility to ensure that there is xsave state to + * serialise, and that the provided buffer is exactly the right size. + */ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size) { struct xsave_struct *xsave = v->arch.xsave_area; +const void *src; uint16_t comp_offsets[sizeof(xfeature_mask)*8]; u64 xstate_bv = xsave->xsave_hdr.xstate_bv; u64 valid; +/* Check there is state to serialise (i.e. at least an XSAVE_HDR) */ +BUG_ON(!v->arch.xcr0_accum); +/* Check there is the correct room to decompress into. */ +BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum)); + if ( !(xsave->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) ) { memcpy(dest, xsave, size); @@ -189,6 +206,7 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size) * Copy legacy XSAVE area and XSAVE hdr area. */ memcpy(dest, xsave, XSTATE_AREA_MIN_SIZE); +memset(dest + XSTATE_AREA_MIN_SIZE, 0, size - XSTATE_AREA_MIN_SIZE); ((struct xsave_struct *)dest)->xsave_hdr.xcomp_bv = 0; @@ -196,20 +214,22 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size) * Copy each region from the possibly compacted offset to the * non-compacted offset. */ +src = xsave; valid = xstate_bv & ~XSTATE_FP_SSE; while ( valid ) { u64 feature = valid & -valid; unsigned int index = fls(feature) - 1; -const void *src = get_xsave_addr(xsave, comp_offsets, index); -if ( src ) -{ -ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size); -memcpy(dest + xstate_offsets[index], src, xstate_sizes[index]); -} -else -memset(dest + xstate_offsets[index], 0, xstate_sizes[index]); +/* + * We previously verified xstate_bv. If there isn't valid + * comp_offsets[] information, something is very broken. + */ +BUG_ON(!comp_offsets[index]); +BUG_ON((xstate_offsets[index] + xstate_sizes[index]) > size); + +memcpy(dest + xstate_offsets[index], src + comp_offsets[index], + xstate_sizes[index]); valid &= ~feature; } -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 0/3] Significant changes to Xen Project Governance (governance.html)
I made some significant proposed changes to governance.html based on a number of issues that were raised in a number of surveys last year, and via other means, as well as in the recent discussions related to governance.html changes (the issue of too many committers in XAPI and XAPI being able to hijack the entire project). In any case, the changes are expressed in three patches governance.pandoc, which is the pandoc source for governance.html: - Code motion changes to make real patches easier to read No content has been changed An index was added Fixed some minor typos and formatting issues - Added comment sections to highlight problem areas The intention here is to highlight the issues to be addressed - Significant changes to decision making; some new roles; minor changes Introduces governance changes Adds some new roles Minor formatting changes, such as missing anchors, wrong Deletes addressed open issues in comments Add additional comments to raise questions or provide background info The intention is to show you my thought processes behind the proposal and to prompt questions. The patch series is based on git://xenbits.xen.org/people/larsk/governance.git You can see the diff's easily in http://xenbits.xen.org/gitweb/?p=people/larsk/governance.git;a=shortlog;h=refs/heads/2016-overhaul Open Issues to be fixed - Fix up tables as these don't render properly as html Also see http://rapporter.github.io/pander/pandoc_table.html --- Changes since v1 - Agreed and changed counting schemes for lazy consensus/votinh - Added Community Decisions with Funding and Legal Implications - Clarified AB role in - Removed comments where superceded by decisions - Adapted sections with dependencies -- 2.5.4 (Apple Git-61) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 2/3] Added comment sections to highight problem areas
These are marked by - ... - blocks that will be removed in the published version Signed-off-by: Lars Kurth--- governance.pandoc | 91 +-- 1 file changed, 88 insertions(+), 3 deletions(-) diff --git a/governance.pandoc b/governance.pandoc index 2ce780c..86e4433 100644 --- a/governance.pandoc +++ b/governance.pandoc @@ -1,3 +1,4 @@ + This document has come in effect in June 2011 and will be reviewed periodically (see revision sections). The last modification has been made in July 2016. @@ -54,8 +55,23 @@ The Xen Project is a meritocracy. The more you contribute the more responsibility you will earn. Leadership roles in Xen are also merit-based and earned by peer acclaim. + + - +I moved the "Roles" section up and split it into two sections with unmodified content +- Xen Project Wide Roles +- Project Team Roles + - + Xen Project Wide Roles {#roles-global} -- + + - +MINOR ISSUES TO BE ADDRESSED LATER: +- Sub-projects and Teams would benefit from some forward references to highlight the + difference between incubation mature projects. +- Also we should clarify what assets a sub-project owns. +- Add the role of Community Manager as it used throughout the document + - ### Sub-projects and Teams @@ -103,6 +119,15 @@ behind the project. Project Team Roles {#roles-local} -- + - +ISSUES TO BE ADDRESSED LATER: +- Fix minor Inaccuracies and Improvements +- Allow for customization of roles by sub-projects (but this definition is the default) +- Allow for Security Response Team +- Allow for sub-projects to be lead by a Project Leadership Team (which may include a + Project Lead) + - + ### Maintainers Maintainers own one or several components in the Xen tree. A maintainer reviews @@ -131,6 +156,10 @@ referees should disagreements amongst committers of the project arise. The project lead typically also has write access to resources, such as the web page of a specific project. + - +Moved this section + - + Making Contributions {#contributions} @@ -147,18 +176,46 @@ documents: - [Contribution Guidelines](/help/contribution-guidelines.html) -Decision Making, Conflict Resolution, Role Nominations and Elections -{#decisions} + - +Consolidated all Decision Making Related topics into one section +- I changed the order of the sections from ... + "Consensus Decision Making, Conflict Resolution, Elections and Formal Votes" to + "Consensus Decision Making, Formal Votes, Conflict Resolution, Elections" +- I changed header titles and fixed the headline + +Otherwise the relevant sections remain identical, with the exception of comment +sections that I added, which highlight issues that are to be addressed. + - + +Decision Making, Conflict Resolution, Role Nominations and Elections {#decisions} + - +ISSUES TO BE ADDRESSED LATER: +- Add a pre-amble explaining the different decision making mechanisms and when they + apply +- Add a section about review and commit, which is the primary means of making + code related decisions + - + ### Consensus Decision Making + - +ISSUES TO BE ADDRESSED LATER: +- The "Consensus Decision Making" section is totally wrong. It does not describe + "Lazy Consensus" + - + Sub-projects or teams hosted on Xenproject.org are normally auto-governing and
[Xen-devel] [PATCH v2 3/3] Significant changes to decision making; some new roles and minor changes
Added RTC Policy Added +2 ... -2 scheme for votes Clarified lazy consensus (tallying and lazy voting) Added Informal Votes/Surveys Added Project Team Leadership role and Decision making Added Community Decisions with Funding and Legal Implications Changed Project Wide Decision making: per project based scheme Clarified scope of Decision making Modified sections which have dependencies on changes about Signed-off-by: Lars Kurth--- governance.pandoc | 773 -- 1 file changed, 581 insertions(+), 192 deletions(-) diff --git a/governance.pandoc b/governance.pandoc index 86e4433..04a1dc6 100644 --- a/governance.pandoc +++ b/governance.pandoc @@ -1,6 +1,5 @@ - This document has come in effect in June 2011 and will be reviewed periodically -(see revision sections). The last modification has been made in July 2016. +(see revision sections). The last modification has been made in September 2016. Content --- @@ -12,8 +11,10 @@ Content - [Making Contributions](#contributions) - [Decision Making, Conflict Resolution, Role Nominations and Elections](#decisions) -- [Formal Votes](#formal-votes) +- [Project Wide Decision Making](#project-decisions) +- [Community Decisions with Funding and Legal Implications](#funding-and-legal) - [Project Governance](#project-governance) +- [Per Sub-Project Governance Specialisations](#specialisations) Goals {#goals} - @@ -55,23 +56,13 @@ The Xen Project is a meritocracy. The more you contribute the more responsibility you will earn. Leadership roles in Xen are also merit-based and earned by peer acclaim. - - - -I moved the "Roles" section up and split it into two sections with unmodified content -- Xen Project Wide Roles -- Project Team Roles - - +### Local Decision Making -Xen Project Wide Roles {#roles-global} +The Xen Project consists of a number of sub-projects: each sub-project makes +technical and other decisions that solely affect it locally. + +Xen Project Wide Roles {#roles-global} -- - - - -MINOR ISSUES TO BE ADDRESSED LATER: -- Sub-projects and Teams would benefit from some forward references to highlight the - difference between incubation mature projects. -- Also we should clarify what assets a sub-project owns. -- Add the role of Community Manager as it used throughout the document - - ### Sub-projects and Teams @@ -80,9 +71,22 @@ the [Project Governance](#project-governance) (or Project Lifecycle) as outlined in this document. Sub-projects (sometimes simply referred to as projects) are run by individuals and are often referred to as teams to highlight the collaborative nature of development. For example, each -sub-project has a [team portal](/developers/teams.html) on Xenproject.org. +sub-project has a [team portal](/developers/teams.html) on Xenproject.org. +Sub-projects own and are responsible for a collection of source repositories +and other resources (e.g. test infrastructure, CI infrastructure, ...), which +we call **sub-project assets** (or team assets) in this document. -### Xen Project Advisory Board +Sub-projects can either be **incubation projects** or **mature projects** as +outlined in [Basic Project Life Cycle](#project-governance). In line with the +meritocratic principle, mature projects have more influence than incubation +projects, on [project wide decisions](#project-decisions). + +### Community Manager + +The Xen Project has a community manager, whose primary role it is to support +the entire Xen Project Community. + +### Xen Project Advisory Board {#roles-ab} The [Xen Project Advisory Board](/join.html) consists of members who are committed to steering the project to advance its market and technical success, @@ -92,7 +96,7 @@ shared project infrastructure, marketing and events, and managing the Xen Project trademark. The Advisory Board leaves all technical decisions to the open source meritocracy. -### The Linux Foundation +### The Linux Foundation {#roles-lf} The Xen Project is a [Linux Foundation](/linux-foundation.html) Collaborative Project. Collaborative Projects are independently funded software projects that @@ -111,30 +115,60 @@ members or other distinguished community members. ### Sponsor To form a new sub-project or team on Xenproject.org, we require a sponsor to -support the creation of the new project. A sponsor can be a project lead or -committer of a mature project, a member of the advisory board or the community -manager. This ensures that a
[Xen-devel] [PATCH v2 1/3] Code motion changes to make real patches easier to read
Added TOC Re-arranged sections compared to previous version of document Added new anchors where needed Split Roles section into two sections The actual content was not changed (with the exception of minor typos that I spotted) Signed-off-by: Lars Kurth--- governance.pandoc | 207 +- 1 file changed, 113 insertions(+), 94 deletions(-) diff --git a/governance.pandoc b/governance.pandoc index 60fc942..2ce780c 100644 --- a/governance.pandoc +++ b/governance.pandoc @@ -1,9 +1,20 @@ - -This document has come in effect in June 2011 and will be -reviewed periodically (see revision sections). The last modification has been -made in May 2013. - -Goals +This document has come in effect in June 2011 and will be reviewed periodically +(see revision sections). The last modification has been made in July 2016. + +Content +--- + +- [Goals](#goals) +- [Principles](#principles) +- [Xen Project Wide Roles](#roles-global) +- [Project Team Roles](#roles-local) +- [Making Contributions](#contributions) +- [Decision Making, Conflict Resolution, Role Nominations and +Elections](#decisions) +- [Formal Votes](#formal-votes) +- [Project Governance](#project-governance) + +Goals {#goals} - The goals of Xen Project Governance are to: @@ -22,7 +33,7 @@ going elsewhere - Set clear expectations to vendors, upstream and downstream projects and community members -Principles +Principles {#principles} -- ### Openness @@ -43,71 +54,8 @@ The Xen Project is a meritocracy. The more you contribute the more responsibility you will earn. Leadership roles in Xen are also merit-based and earned by peer acclaim. -### Consensus Decision Making - -Sub-projects or teams hosted on Xenproject.org are normally auto-governing and -driven by the people who volunteer for the job. This functions well for most -cases. When more formal decision making and coordination is required, decisions -are taken with a lazy consensus approach: a few positive votes with no negative -vote are enough to get going. - -Voting is done with numbers: - -- +1 : a positive vote -- 0 : abstain, have no opinion -- -1 : a negative vote - -A negative vote should include an alternative proposal or a detailed -explanation of the reasons for the negative vote. The project community then -tries to gather consensus on an alternative proposal that resolves the issue. -In the great majority of cases, the concerns leading to the negative vote can -be addressed. - -### Conflict Resolution - - Refereeing - -Sub-projects and teams hosted on Xenproject.org are not democracies but -meritocracies. In situations where there is disagreement on issues related to -the day-to-day running of the project, Committers and Project Leads are -expected to act as referees and make a decision on behalf of the community. -Referees should however consider whether making a decision may be divisive and -damaging for the community. In such cases, the committer community of the -project can privately vote on an issue, giving the decision more weight. - - Last Resort - -In some rare cases, the lazy consensus approach may lead to the community being -paralyzed. Thus, as a last resort when consensus cannot be achieved on a -question internal to a project, the final decision will be made by a private -majority vote amongst the committers and project lead. If the vote is tied, the -project lead gets an extra vote to break the tie. - -For questions that affect several projects, committers and project leads of -mature projects will hold a private majority vote. If the vote is tied, the -[Xen Project Advisory Board](/join.html) will break the tie through a casting -vote. - -Roles -- - -### Maintainers - -Maintainers own one or several components in the Xen tree. A maintainer reviews -and approves changes that affect their components. It is a maintainer's prime -responsibility to review, comment on, co-ordinate and accept patches from other -community member's and to maintain the design cohesion of their components. -Maintainers are listed in a MAINTAINERS file in the root of the source tree. - -### Committers - -Committers are Maintainers that are allowed to commit changes into the source -code repository. The committer acts on the wishes of the maintainers and -applies changes that have been approved by the respective maintainer(s) to the -source tree. Due to their status in the community, committers can also act as -referees should disagreements amongst maintainers arise. Committers are listed -on the sub-project's team portal (e.g. [Hypervisor team -portal](/developers/teams/hypervisor.html)). +Xen Project Wide Roles {#roles-global} +-- ### Sub-projects and Teams @@ -118,16 +66,6 @@ projects) are run by individuals and are often referred to as teams to highlight the collaborative nature of development. For
Re: [Xen-devel] [PATCH 6/6] x86/xstate: Fix latent bugs in compress_xsave_states()
>>> On 12.09.16 at 17:28,wrote: > On 12/09/16 14:47, Jan Beulich wrote: > On 12.09.16 at 14:59, wrote: >>> On 12/09/16 13:27, Jan Beulich wrote: >>> On 12.09.16 at 11:51, wrote: > +xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv; > > if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) ) > { > +/* > + * TODO: This logic doesn't currently handle restoration of xsave > + * state which would force the vcpu from uncompressed to > compressed. > + */ > +BUG_ON(xstate_bv & XSTATE_XSAVES_ONLY); I don't think this is a valid concern of yours: The function can't be used to restore features not xcr0_accum anyway (or else that field would need updating). Afaict validate_xstate() already prevents this as intended. >>> This is all currently dead code. I guess the question really depends on >>> what we plan to do with compressed states. >>> >>> Strictly speaking, no XSAVES state can every be present in xcr0, by >>> design. If we retroactively consider xcr0_accum to be "all states in >>> use", >> I think that's the only viable model, considering how the domctl works: >> xcr0_accum needs to represent the combination of features ever >> enabled in XCR0 and XSS. > > In which case we should really rename it to xstate_accum then. Right. >>> then the if condition in context does become relevant when Xen >>> starts supporting XSAVES-only components. >>> >>> In such a case, it is definitely wrong to memcpy() the uncompressed >>> buffer, as Xen will try and use xrstors and corrupt all guest state. >> How? If the guest never enabled any bit in XSS, how can any such >> bit be set in xstate_bv (which is always a subset of XCR0|XSS). > > Currently it cant. This is a preemptive catch for whomever tries > implementing the first XSS state, and doesn't test migration between > older and newer Xen. I still don't see why you say "currently" when this is an architectural (of how Xen behaves, not hardware architecture) thing: xcr0_accum, as its name says, only ever gets bits added to it, and if some bit is clear there it can't possibly be set in xstate_bv or xcomp_bv (for the latter of course with the exception of the compressed bit). So by having made it past if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) ) { the BUG_ON() you add can't possibly trigger (or else we have much more severe problems). In particular this has nothing to do with whether any XSS states are being used. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 2/2] qdisk - hw/block/xen_disk: grant copy implementation
On Wed, Sep 07, 2016 at 12:41:00PM +0200, Paulina Szubarczyk wrote: > Copy data operated on during request from/to local buffers to/from > the grant references. > > Before grant copy operation local buffers must be allocated what is > done by calling ioreq_init_copy_buffers. For the 'read' operation, > first, the qemu device invokes the read operation on local buffers > and on the completion grant copy is called and buffers are freed. > For the 'write' operation grant copy is performed before invoking > write by qemu device. > > A new value 'feature_grant_copy' is added to recognize when the > grant copy operation is supported by a guest. > > Signed-off-by: Paulina SzubarczykAcked-by: Roger Pau Monné Just a couple of minor comments below, but the implementation looks fine to me. > --- > Changes since v5: > -added checking of every interface in the configure file. Based on > the Roger's comment that xengnttab_map_grant_ref was added prior > xengnttab_grant_copy, thus do not need to be check again here > I dropped this check. > > --- > configure | 55 > hw/block/xen_disk.c | 157 > ++-- > include/hw/xen/xen_common.h | 14 > 3 files changed, 221 insertions(+), 5 deletions(-) > > diff --git a/configure b/configure > index 4b808f9..3f44d38 100755 > --- a/configure > +++ b/configure > @@ -1956,6 +1956,61 @@ EOF > /* > * If we have stable libs the we don't want the libxc compat > * layers, regardless of what CFLAGS we may have been given. > + * > + * Also, check if xengnttab_grant_copy_segment_t is defined and > + * grant copy operation is implemented. > + */ > +#undef XC_WANT_COMPAT_EVTCHN_API > +#undef XC_WANT_COMPAT_GNTTAB_API > +#undef XC_WANT_COMPAT_MAP_FOREIGN_API > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#if !defined(HVM_MAX_VCPUS) > +# error HVM_MAX_VCPUS not defined > +#endif > +int main(void) { > + xc_interface *xc = NULL; > + xenforeignmemory_handle *xfmem; > + xenevtchn_handle *xe; > + xengnttab_handle *xg; > + xen_domain_handle_t handle; > + xengnttab_grant_copy_segment_t* seg = NULL; > + > + xs_daemon_open(); > + > + xc = xc_interface_open(0, 0, 0); > + xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0); > + xc_domain_add_to_physmap(0, 0, XENMAPSPACE_gmfn, 0, 0); > + xc_hvm_inject_msi(xc, 0, 0xf000, 0x); > + xc_hvm_create_ioreq_server(xc, 0, HVM_IOREQSRV_BUFIOREQ_ATOMIC, NULL); > + xc_domain_create(xc, 0, handle, 0, NULL, NULL); > + > + xfmem = xenforeignmemory_open(0, 0); > + xenforeignmemory_map(xfmem, 0, 0, 0, 0, 0); > + > + xe = xenevtchn_open(0, 0); > + xenevtchn_fd(xe); > + > + xg = xengnttab_open(0, 0); > + xengnttab_grant_copy(xg, 0, seg); > + > + return 0; > +} > +EOF > + compile_prog "" "$xen_libs $xen_stable_libs" > +then > +xen_ctrl_version=480 > +xen=yes > + elif > + cat > $TMPC < +/* > + * If we have stable libs the we don't want the libxc compat > + * layers, regardless of what CFLAGS we may have been given. > */ > #undef XC_WANT_COMPAT_EVTCHN_API > #undef XC_WANT_COMPAT_GNTTAB_API > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c > index 3b8ad33..3739e13 100644 > --- a/hw/block/xen_disk.c > +++ b/hw/block/xen_disk.c > @@ -119,6 +119,9 @@ struct XenBlkDev { > unsigned intpersistent_gnt_count; > unsigned intmax_grants; > > +/* Grant copy */ > +gbooleanfeature_grant_copy; > + > /* qemu block driver */ > DriveInfo *dinfo; > BlockBackend*blk; > @@ -489,6 +492,108 @@ static int ioreq_map(struct ioreq *ioreq) > return 0; > } > > +#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 480 > + > +static void free_buffers(struct ioreq *ioreq) > +{ > +int i; > + > +for (i = 0; i < ioreq->v.niov; i++) { > +ioreq->page[i] = NULL; > +} > + > +qemu_vfree(ioreq->pages); > +} > + > +static int ioreq_init_copy_buffers(struct ioreq *ioreq) > +{ > +int i; > + > +if (ioreq->v.niov == 0) { > +return 0; > +} > + > +ioreq->pages = qemu_memalign(XC_PAGE_SIZE, ioreq->v.niov * XC_PAGE_SIZE); > + > +for (i = 0; i < ioreq->v.niov; i++) { > +ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE; > +ioreq->v.iov[i].iov_base = ioreq->page[i]; > +} > + > +return 0; > +} > + > +static int ioreq_copy(struct ioreq *ioreq) > +{ > +xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev; > +xengnttab_grant_copy_segment_t segs[BLKIF_MAX_SEGMENTS_PER_REQUEST]; > +int i, count, rc; > +int64_t file_blk = ioreq->blkdev->file_blk; > + > +if (ioreq->v.niov == 0) { > +return 0; > +} > + > +count = ioreq->v.niov; > + > +for (i = 0; i < count; i++) { This newline here... > + > +if (ioreq->req.operation == BLKIF_OP_READ) { > +segs[i].flags =
Re: [Xen-devel] [RFC] x86/vm_event: Allow returning i-cache for emulation
>>> On 12.09.16 at 17:48,wrote: > On Mon, Sep 12, 2016 at 8:56 AM, Jan Beulich wrote: >> >>> On 09.09.16 at 17:41, wrote: >> > @@ -1783,7 +1786,14 @@ static int _hvm_emulate_one(struct >> hvm_emulate_ctxt *hvmemul_ctxt, >> > pfec |= PFEC_user_mode; >> > >> > hvmemul_ctxt->insn_buf_eip = regs->eip; >> > -if ( !vio->mmio_insn_bytes ) >> > + >> > +if ( unlikely(hvmemul_ctxt->set_context_insn) ) >> > +{ >> > +memcpy(hvmemul_ctxt->insn_buf, curr->arch.vm_event->emul_ >> data.data, >> > + curr->arch.vm_event->emul_data.size); >> > +hvmemul_ctxt->insn_buf_bytes = curr->arch.vm_event->emul_ >> data.size; >> > +} >> > +else if ( !vio->mmio_insn_bytes ) >> >> I'm not sure about this ordering: Do you really mean to allow an >> external entity to override insn bytes e.g. in an MMIO retry, i.e. >> allowing the retried insn to be different from the original one? >> > > I'm not sure but there shouldn't be INT3's coming from MMIO, right? So we > would not have a response-opportunity to override it. Can't you modify insns at any time from the monitoring app? If so, wouldn't there be a chance for things to change between the original insn invocation and the retry (after qemu completed the request to it)? >> And additionally I think you need to deal with the case of the >> supplied data not being a full insn. There shouldn't be any >> fetching from guest memory in that case imo, emulation should >> just fail. >> > > So the idea was to allow partial "masking" of the i-cache. For example, if > all I want to hide is the 0xCC then it's enough to return 1 byte in > vm_event, the rest can be (and already is) grabbed from memory. Oh, interesting. That makes it even more problematic, imo. Can you ensure that your patching any whatever the guest may do to its own insn stream actually remains consistent, even in situations like the insn crossing a page boundary? IOW what you suggest seems pretty fragile to me, but would in any case need spelling out very clearly. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 09/18] livepatch/arm/x86: Check payload for for unwelcomed symbols.
>>> On 11.09.16 at 22:35,wrote: > --- a/xen/arch/arm/livepatch.c > +++ b/xen/arch/arm/livepatch.c > @@ -117,6 +117,20 @@ bool_t arch_livepatch_symbol_ok(const struct > livepatch_elf *elf, > return true; > } > > +int arch_livepatch_symbol_check(const struct livepatch_elf *elf, > +const struct livepatch_elf_sym *sym) Perhaps also better to return bool here? > @@ -255,6 +256,14 @@ static int elf_get_sym(struct livepatch_elf *elf, const > void *data) > > sym[i].sym = s; > sym[i].name = strtab_sec->data + delta; > +/* On ARM we should NEVER see $t* symbols. */ If you really mean to have such an arch-specific comment in common code, may I recommend to use "e.g." or something similar? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 08/18] livepatch: ARM 32|64: Ignore mapping symbols: $[d, a, x]
>>> On 11.09.16 at 22:35,wrote: > Those symbols are used to help final linkers to replace insn. > The ARM ELF specification mandates that they are present > to denote the start of certain CPU features. There are two > variants of it - short and long format. > > Either way - we can ignore these symbols. > > Reviewed-by: Andrew Cooper [x86 bits] > Signed-off-by: Konrad Rzeszutek Wilk > > --- > Cc: Konrad Rzeszutek Wilk > Cc: Ross Lagerwall > Cc: Stefano Stabellini > Cc: Julien Grall Cc: Jan Beulich > Cc: Andrew Cooper > > v1: First submission > v2: Update the order of symbols, fix title > Add {} in after the first if - per Jan's recommendation. > v3: Add Andrew's Review tag > Make the function return an bool_t. Yet it should have been bool. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC] x86/vm_event: Allow returning i-cache for emulation
On Mon, Sep 12, 2016 at 8:56 AM, Jan Beulichwrote: > >>> On 09.09.16 at 17:41, wrote: > > When emulating instructions the emulator maintains a small i-cache > fetched > > from the guest memory. Under certain scenarios this memory region may > contain > > instructions that a monitor subscriber would prefer to hide, namely > INT3, and > > instead would prefer to emulate a different instruction in-place. > > > > This patch extends the vm_event interface to allow returning this > i-cache via > > the vm_event response. > > Please try to explain better what this change is about, perhaps along > the lines of what George used as description, which you then > confirmed. > Sure. > > > @@ -1783,7 +1786,14 @@ static int _hvm_emulate_one(struct > hvm_emulate_ctxt *hvmemul_ctxt, > > pfec |= PFEC_user_mode; > > > > hvmemul_ctxt->insn_buf_eip = regs->eip; > > -if ( !vio->mmio_insn_bytes ) > > + > > +if ( unlikely(hvmemul_ctxt->set_context_insn) ) > > +{ > > +memcpy(hvmemul_ctxt->insn_buf, curr->arch.vm_event->emul_ > data.data, > > + curr->arch.vm_event->emul_data.size); > > +hvmemul_ctxt->insn_buf_bytes = curr->arch.vm_event->emul_ > data.size; > > +} > > +else if ( !vio->mmio_insn_bytes ) > > I'm not sure about this ordering: Do you really mean to allow an > external entity to override insn bytes e.g. in an MMIO retry, i.e. > allowing the retried insn to be different from the original one? > I'm not sure but there shouldn't be INT3's coming from MMIO, right? So we would not have a response-opportunity to override it. > > And additionally I think you need to deal with the case of the > supplied data not being a full insn. There shouldn't be any > fetching from guest memory in that case imo, emulation should > just fail. > So the idea was to allow partial "masking" of the i-cache. For example, if all I want to hide is the 0xCC then it's enough to return 1 byte in vm_event, the rest can be (and already is) grabbed from memory. > > @@ -1927,17 +1937,19 @@ void hvm_mem_access_emulate_one(enum emul_kind > kind, unsigned int trapnr, > > struct hvm_emulate_ctxt ctx = {{ 0 }}; > > int rc; > > > > +gdprintk(XENLOG_WARNING, "memaccess emulate one called\n"); > > + > > hvm_emulate_prepare(, guest_cpu_user_regs()); > > > > -switch ( kind ) > > -{ > > -case EMUL_KIND_NOWRITE: > > +if ( kind == EMUL_KIND_NOWRITE ) > > rc = hvm_emulate_one_no_write(); > > -break; > > -case EMUL_KIND_SET_CONTEXT: > > -ctx.set_context = 1; > > -/* Intentional fall-through. */ > > -default: > > +else > > +{ > > + if ( kind == EMUL_KIND_SET_CONTEXT_DATA ) > > +ctx.set_context_data = 1; > > + else if ( kind == EMUL_KIND_SET_CONTEXT_INSN ) > > +ctx.set_context_insn = 1; > > + > > rc = hvm_emulate_one(); > > } > > Could you try to retain the switch() statement? > Sure. > > > --- a/xen/arch/x86/vm_event.c > > +++ b/xen/arch/x86/vm_event.c > > @@ -66,6 +66,16 @@ void vm_event_toggle_singlestep(struct domain *d, > struct vcpu *v) > > hvm_toggle_singlestep(v); > > } > > > > +void vm_event_interrupt_emulate_check(struct vcpu *v, > vm_event_response_t *rsp) > > +{ > > +if ( !!(rsp->flags & VM_EVENT_FLAG_EMULATE) && > > + !!(rsp->flags & VM_EVENT_FLAG_SET_EMUL_INSN_DATA) ) > > Please avoid !! when not really needed. > Ack. > > > +{ > > +v->arch.vm_event->emulate_flags = rsp->flags; > > +v->arch.vm_event->emul_data = rsp->data.emul_data; > > +} > > +} > > Overall I'm having difficulty associating the function's name with > what it does. > Yea, I'm not too happy with it either. Will move some things around. > > > --- a/xen/common/vm_event.c > > +++ b/xen/common/vm_event.c > > @@ -407,8 +407,11 @@ void vm_event_resume(struct domain *d, struct > vm_event_domain *ved) > > vm_event_register_write_resume(v, ); > > break; > > > > +case VM_EVENT_REASON_SOFTWARE_BREAKPOINT: > > +vm_event_interrupt_emulate_check(v, ); > > +break; > > + > > #ifdef CONFIG_HAS_MEM_ACCESS > > -case VM_EVENT_REASON_MEM_ACCESS: > > mem_access_resume(v, ); > > break; > > I don't think you meant to remove that line? > Oops. > > Jan > > Thanks, Tamas ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 07/18] livepatch: ARM/x86: Check displacement of old_addr and new_addr
>>> On 11.09.16 at 22:35,wrote: > If the distance is too great we are in trouble - as our relocation I think you mean "large" or big" here. > distance can surely be clipped, or still have a valid width - but > cause an overflow of distance. > > On various architectures the maximum displacement for a unconditional > branch/jump varies. ARM32 is +/- 32MB, ARM64 is +/- 128MB while x86 > for 32-bit relocations is +/- 2G. > > Note: On x86 we could use the 64-bit jmpq instruction which > would provide much bigger displacement to do a jump, but we would > still have issues with the new function not being able to reach > any of the old functions (as all the relocations would assume 32-bit > displacement). With jmpq you mean the indirect variant? That additionally would require a register or memory location you can clobber to load/store the address into. > --- a/xen/include/xen/livepatch.h > +++ b/xen/include/xen/livepatch.h > @@ -12,6 +12,7 @@ struct livepatch_elf_sym; > struct xen_sysctl_livepatch_op; > > #include > +#include /* For -ENOSYS or -EOVERFLOW */ > #ifdef CONFIG_LIVEPATCH > > /* > @@ -66,16 +67,32 @@ int arch_livepatch_secure(const void *va, unsigned int > pages, enum va_type types > void arch_livepatch_init(void); > > #include /* For struct livepatch_func. */ > -#include /* For PATCH_INSN_SIZE. */ > +#include /* For LIVEPATCH_ARCH_RANGE and PATCH_INSN_SIZE */ > int arch_livepatch_verify_func(const struct livepatch_func *func); > > -static inline size_t arch_livepatch_insn_len(const struct livepatch_func > *func) > +static inline > +unsigned int arch_livepatch_insn_len(const struct livepatch_func *func) > { > if ( !func->new_addr ) > return func->new_size; > > return PATCH_INSN_SIZE; > } > + > +static inline int arch_livepatch_verify_distance(const struct livepatch_func > *func) Just like mentioned earlier for arch_livepatch_insn_len() I don't think the arch prefix is a good choice when the function isn't arch-specific. > +{ > +long offset; > +long range = (long)LIVEPATCH_ARCH_RANGE; > + > +if ( !func->new_addr ) /* Ignore NOPs. */ > +return 0; > + > +offset = ((long)func->old_addr - (long)func->new_addr); Please avoid casts when they're not really needed. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 01/18] arm/x86: Add HAS_[ALTERNATIVE|EX_TABLE]
Hi, On 12/09/16 16:28, Jan Beulich wrote: On 11.09.16 at 22:35,wrote: x86 implements all of them by default - and we just add two extra HAS_ variables to be declared in autoconf.h. ARM 64 only has alternative while ARM 32 has none of them. The ARM64 is going to be a bit funny as there is an ALTERNATIVE already and we end up selecting the HAS_ALTERNATIVE whenever the ALTERNATIVE is selected. How about replacing ALTERNATIVE by HAS_ALTERNATIVE? FWIW, I was about to suggest the same. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 6/6] x86/xstate: Fix latent bugs in compress_xsave_states()
On 12/09/16 14:47, Jan Beulich wrote: On 12.09.16 at 14:59,wrote: >> On 12/09/16 13:27, Jan Beulich wrote: >> On 12.09.16 at 11:51, wrote: void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size) { struct xsave_struct *xsave = v->arch.xsave_area; uint16_t comp_offsets[sizeof(xfeature_mask)*8]; -u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv; -u64 valid; +u64 xstate_bv, valid; + +BUG_ON(!v->arch.xcr0_accum); +BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum)); +BUG_ON(xsave_area_compressed(src)); -ASSERT(!xsave_area_compressed(src)); >>> Same remark here as on the earlier patch wrt BUG_ON() vs ASSERT(). >> Same answer. > Well, it's certainly a matter of taste how much of the above to consider > bounds checking. I for one would take only the middle one as such. The first is necessary if you want the second, and important as the calling convention involves the caller modifying xcr0* to match the incoming state. The compressed check is admittedly different, but it is currently an ABI requirement that the data is uncompressed. This could be lifted if we alter the ABI. > +xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv; if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) ) { +/* + * TODO: This logic doesn't currently handle restoration of xsave + * state which would force the vcpu from uncompressed to compressed. + */ +BUG_ON(xstate_bv & XSTATE_XSAVES_ONLY); >>> I don't think this is a valid concern of yours: The function can't be >>> used to restore features not xcr0_accum anyway (or else that >>> field would need updating). Afaict validate_xstate() already prevents >>> this as intended. >> This is all currently dead code. I guess the question really depends on >> what we plan to do with compressed states. >> >> Strictly speaking, no XSAVES state can every be present in xcr0, by >> design. If we retroactively consider xcr0_accum to be "all states in >> use", > I think that's the only viable model, considering how the domctl works: > xcr0_accum needs to represent the combination of features ever > enabled in XCR0 and XSS. In which case we should really rename it to xstate_accum then. > >> then the if condition in context does become relevant when Xen >> starts supporting XSAVES-only components. >> >> In such a case, it is definitely wrong to memcpy() the uncompressed >> buffer, as Xen will try and use xrstors and corrupt all guest state. > How? If the guest never enabled any bit in XSS, how can any such > bit be set in xstate_bv (which is always a subset of XCR0|XSS). Currently it cant. This is a preemptive catch for whomever tries implementing the first XSS state, and doesn't test migration between older and newer Xen. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 01/18] arm/x86: Add HAS_[ALTERNATIVE|EX_TABLE]
>>> On 11.09.16 at 22:35,wrote: > x86 implements all of them by default - and we just > add two extra HAS_ variables to be declared in autoconf.h. > > ARM 64 only has alternative while ARM 32 has none of them. > The ARM64 is going to be a bit funny as there is an > ALTERNATIVE already and we end up selecting the HAS_ALTERNATIVE > whenever the ALTERNATIVE is selected. How about replacing ALTERNATIVE by HAS_ALTERNATIVE? > --- a/xen/arch/arm/Kconfig > +++ b/xen/arch/arm/Kconfig > @@ -45,8 +45,12 @@ config ACPI > config HAS_GICV3 > bool > > +config HAS_ALTERNATIVE > + bool This needlessly repeats what you add to common/Kconfig. > @@ -22,6 +24,7 @@ config X86 > select NUMA > select VGA > > + > config ARCH_DEFCONFIG Please don't. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] BUG_ON() vs ASSERT()
All, in https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg01201.html and https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg01210.html Andrew basically suggests that we should switch away from using ASSERT() and over to BUG_ON() in perhaps quite broad a set of cases. And honestly I'm not convinced of this: We've been adding quite a few ASSERT()s over the last years with the aim of doing sanity checking in debug builds, without adding overhead to non- debug builds. I can certainly see possible cases where using BUG_ON() to prevent further possible damage is appropriate, but I don't think we should overdo here. Thanks for other's opinions, Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 00/16] Xen ARM DomU ACPI support
Hi Shannon, On 02/09/16 03:55, Shannon Zhao wrote: From: Shannon ZhaoThe design of this feature is described as below. Firstly, the toolstack (libxl) generates the ACPI tables according the number of vcpus and gic controller. Then, it copies these ACPI tables to DomU non-RAM memory map space and passes them to UEFI firmware through the "ARM multiboot" protocol. At last, UEFI gets the ACPI tables through the "ARM multiboot" protocol and installs these tables like the usual way and passes both ACPI and DT information to the Xen DomU. Currently libxl only generates RSDP, XSDT, GTDT, MADT, FADT, DSDT tables since it's enough now. This has been tested using guest kernel with the Dom0 ACPI support patches which could be fetched from linux master or: https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/log/?h=efi/arm-xen The UEFI binary could be fetched from or built from edk2 master branch: http://people.linaro.org/~shannon.zhao/DomU_ACPI/XEN_EFI.fd This series can be fetched from: https://git.linaro.org/people/shannon.zhao/xen.git domu_acpi_v5 This branch is based on a fairly out of date xen. Do you have a branch rebased on the latest upstream + Boris ACPI v3? Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 16/16] libxl/arm: Add the size of ACPI tables to maxmem
Hi Shannon, On 02/09/16 03:55, Shannon Zhao wrote: From: Shannon ZhaoHere it adds the ACPI tables size to set the target maxmem to avoid providing less available memory for guest. Signed-off-by: Shannon Zhao --- tools/libxl/libxl_arch.h| 2 +- tools/libxl/libxl_arm.c | 18 +- tools/libxl/libxl_arm.h | 4 tools/libxl/libxl_arm_acpi.c| 20 tools/libxl/libxl_arm_no_acpi.c | 6 ++ tools/libxl/libxl_dom.c | 2 +- tools/libxl/libxl_x86.c | 2 +- 7 files changed, 50 insertions(+), 4 deletions(-) diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h index 337061f..d62fa4c 100644 --- a/tools/libxl/libxl_arch.h +++ b/tools/libxl/libxl_arch.h @@ -30,7 +30,7 @@ int libxl__arch_domain_save_config(libxl__gc *gc, /* arch specific internal domain creation function */ _hidden int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config, - uint32_t domid); + libxl__domain_build_state *state, uint32_t domid); /* setup arch specific hardware description, i.e. DTB on ARM */ _hidden diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index e73d65e..c7d4f65 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -101,8 +101,24 @@ int libxl__arch_domain_save_config(libxl__gc *gc, } int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config, - uint32_t domid) + libxl__domain_build_state *state, uint32_t domid) { +libxl_domain_build_info *const info = _config->b_info; +libxl_ctx *ctx = libxl__gc_owner(gc); +int size; + +/* Add the size of ACPI tables to maxmem if ACPI is enabled for guest. */ +if (libxl_defbool_val(info->acpi)) { +size = libxl__get_acpi_size(gc, info, state); +if (size < 0) +return ERROR_FAIL; +if (xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb + +LIBXL_MAXMEM_CONSTANT + (size + 1023) / 1024)) { I still have some concern about use info->target_memkb + LIBXL_MAXMEM_CONSTANT here. What if the generic code decide to change the computation? We may forgot to replicate here. My suggestion on the previous version was to have in the common code xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb + LIBXL_MAXMEM_CONSTANT + libxl__arch_memory_constant()); Or a similar name. +LOGE(ERROR, "Couldn't set max memory"); +return ERROR_FAIL; +} +} + return 0; } Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 03/24] xen: credit1: return the 'time remaining to the limit' as next timeslice.
On 17/08/16 18:17, Dario Faggioli wrote: > If vcpu x has run for 200us, and sched_ratelimit_us is > 1000us, continue running x _but_ return 1000us-200us as > the next time slice. This way, next scheduling point will > happen in 800us, i.e., exactly at the point when x crosses > the threshold, and can be descheduled (if appropriate). > > Right now (without this patch), we're always returning > sched_ratelimit_us (1000us, in the example above), which > means we're (potentially) allowing x to run more than > it should have been able to (even when considering rate > limiting into account). Part of the reason I went with this in the first place was because I wanted to avoid really short timers. Part of the reason for the ratelimit was actually to limit the amount of time spent in the scheduler. Since we expect ratelimit to normally be pretty short, waiting for the whole ratelimit time seemed like a good idea. Thoughts? -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 15/16] libxl/arm: Initialize domain param HVM_PARAM_CALLBACK_IRQ
Hi Shannon, On 02/09/16 03:55, Shannon Zhao wrote: From: Shannon ZhaoThe guest kernel will get the event channel interrupt information via domain param HVM_PARAM_CALLBACK_IRQ. Initialize it here. Signed-off-by: Shannon Zhao Acked-by: Julien Grall Regards, --- tools/libxl/libxl_arm.c | 13 + 1 file changed, 13 insertions(+) diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index 6f0bc70..e73d65e 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -900,8 +900,21 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc, struct xc_dom_image *dom) { int rc; +uint64_t val; assert(info->type == LIBXL_DOMAIN_TYPE_PV); + +/* Set the value of domain param HVM_PARAM_CALLBACK_IRQ. */ +val = (uint64_t)HVM_PARAM_CALLBACK_TYPE_PPI << HVM_PARAM_CALLBACK_IRQ_TYPE_SHIFT; +/* Active-low level-sensitive */ +val |= (HVM_PARAM_CALLBACK_TYPE_PPI_FLAG_LOW_LEVEL << +HVM_PARAM_CALLBACK_TYPE_PPI_FLAG_SHIFT); +val |= GUEST_EVTCHN_PPI; +rc = xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CALLBACK_IRQ, + val); +if (rc) +return rc; + rc = libxl__prepare_dtb(gc, info, state, dom); if (rc) goto out; -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 11/16] libxl/arm: Construct ACPI DSDT table
Hi Shannon, On 02/09/16 03:55, Shannon Zhao wrote: From: Shannon ZhaoCopy the static DSDT table into ACPI blob. Signed-off-by: Shannon Zhao Acked-by: Julien Grall Regards, --- tools/libxl/libxl_arm_acpi.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c index 407f9d5..30e4d66 100644 --- a/tools/libxl/libxl_arm_acpi.c +++ b/tools/libxl/libxl_arm_acpi.c @@ -329,6 +329,15 @@ static void make_acpi_fadt(libxl__gc *gc, struct xc_dom_image *dom, acpitables[FADT].size); } +static void make_acpi_dsdt(libxl__gc *gc, struct xc_dom_image *dom, + struct acpitable acpitables[]) +{ +uint64_t offset = acpitables[DSDT].addr - GUEST_ACPI_BASE; +void *dsdt = dom->acpi_modules[0].data + offset; + +memcpy(dsdt, dsdt_anycpu_arm, dsdt_anycpu_arm_len); +} + int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info, libxl__domain_build_state *state, struct xc_dom_image *dom) @@ -365,6 +374,7 @@ int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info, goto out; make_acpi_fadt(gc, dom, acpitables); +make_acpi_dsdt(gc, dom, acpitables); out: return rc; -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 10/16] libxl/arm: Construct ACPI FADT table
Hi Shannon, On 02/09/16 03:55, Shannon Zhao wrote: From: Shannon ZhaoSigned-off-by: Shannon Zhao Acked-by: Julien Grall Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 09/16] libxl/arm: Construct ACPI MADT table
Hi Shannon, On 02/09/16 03:55, Shannon Zhao wrote: From: Shannon ZhaoAccording to the GIC version, construct the MADT table. Signed-off-by: Shannon Zhao Acked-by: Julien Grall Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 07/16] libxl/arm: Construct ACPI GTDT table
Hi Shannon, On 02/09/16 03:55, Shannon Zhao wrote: From: Shannon ZhaoConstruct GTDT table with the interrupt information of timers. Signed-off-by: Shannon Zhao --- tools/libxl/libxl_arm_acpi.c | 38 ++ 1 file changed, 38 insertions(+) diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c index 93ba2d1..ab49d57 100644 --- a/tools/libxl/libxl_arm_acpi.c +++ b/tools/libxl/libxl_arm_acpi.c @@ -25,10 +25,24 @@ typedef uint8_t u8; typedef uint16_t u16; typedef uint32_t u32; typedef uint64_t u64; +typedef int64_t s64; #include #include +#ifndef BITS_PER_LONG +#ifdef _LP64 +#define BITS_PER_LONG 64 +#else +#define BITS_PER_LONG 32 +#endif +#endif +#define ACPI_MACHINE_WIDTH __BITS_PER_LONG +#define COMPILER_DEPENDENT_INT64 int64_t +#define COMPILER_DEPENDENT_UINT64 uint64_t + +#include + _hidden extern const unsigned char dsdt_anycpu_arm[]; _hidden @@ -190,6 +204,29 @@ static void make_acpi_xsdt(libxl__gc *gc, struct xc_dom_image *dom, acpitables[XSDT].size); } +static void make_acpi_gtdt(libxl__gc *gc, struct xc_dom_image *dom, + struct acpitable acpitables[]) +{ +uint64_t offset = acpitables[GTDT].addr - GUEST_ACPI_BASE; +struct acpi_table_gtdt *gtdt = (void *)dom->acpi_modules[0].data + offset; + +gtdt->non_secure_el1_interrupt = GUEST_TIMER_PHYS_NS_PPI; +gtdt->non_secure_el1_flags = + (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE) + |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY); +gtdt->virtual_timer_interrupt = GUEST_TIMER_VIRT_PPI; +gtdt->virtual_timer_flags = + (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE) + |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY); + +gtdt->counter_block_addresss = 0x; +gtdt->counter_read_block_address = 0x; NIT: You could have used ~((uint64_t)0). This is less error-prone than the 0xFF..FF. Regardless: Acked-by: Julien Grall Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 01/24] xen: credit1: small optimization in Credit1's tickling logic.
On 17/08/16 18:17, Dario Faggioli wrote: > If, when vcpu x wakes up, there are no idle pcpus in x's > soft-affinity, we just go ahead and look at its hard > affinity. This basically means that, if, in __runq_tickle(), > new_idlers_empty is true, balance_step is equal to > CSCHED_BALANCE_HARD_AFFINITY, and that calling > csched_balance_cpumask() for whatever vcpu, would just > return the vcpu's cpu_hard_affinity. > > Therefore, don't bother calling it (it's just pure > overhead) and use cpu_hard_affinity directly. > > For this very reason, this patch should only be > a (slight) optimization, and entail no functional > change. > > As a side note, it would make sense to do what the > patch does, even if we could be inside the > [[ new_idlers_empty && new->pri > cur->pri ]] if > with balance_step equal to CSCHED_BALANCE_SOFT_AFFINITY. > In fact, what is actually happening is: > - vcpu x is waking up, and (since there aren't suitable >idlers, and it's entitled for it) it is preempting >vcpu y; > - vcpu y's hard-affinity is a superset of its >soft-affinity mask. > > Therefore, it makes sense to use wider possible mask, > as by doing that, we maximize the probability of > finding an idle pcpu in there, to which we can send > vcpu y, which then will be able to run. > > While there, also fix the comment, which included > an awkward parenthesis nesting. > > Signed-off-by: Dario FaggioliAcked-by: George Dunlap > --- > Cc: George Dunlap > Cc: Anshul Makkar > Cc: David Vrabel > --- > xen/common/sched_credit.c |8 +++- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c > index 220ff0d..6eccf09 100644 > --- a/xen/common/sched_credit.c > +++ b/xen/common/sched_credit.c > @@ -424,9 +424,9 @@ static inline void __runq_tickle(struct csched_vcpu *new) > /* > * If there are no suitable idlers for new, and it's higher > * priority than cur, check whether we can migrate cur away. > - * (We have to do it indirectly, via _VPF_migrating, instead > + * We have to do it indirectly, via _VPF_migrating (instead > * of just tickling any idler suitable for cur) because cur > - * is running.) > + * is running. > * > * If there are suitable idlers for new, no matter priorities, > * leave cur alone (as it is running and is, likely, cache-hot) > @@ -435,9 +435,7 @@ static inline void __runq_tickle(struct csched_vcpu *new) > */ > if ( new_idlers_empty && new->pri > cur->pri ) > { > -csched_balance_cpumask(cur->vcpu, balance_step, > - cpumask_scratch_cpu(cpu)); > -if ( cpumask_intersects(cpumask_scratch_cpu(cpu), > +if ( cpumask_intersects(cur->vcpu->cpu_hard_affinity, > _mask) ) > { > SCHED_VCPU_STAT_CRANK(cur, kicked_away); > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 06/16] libxl/arm: Construct ACPI XSDT table
Hi Shannon, On 02/09/16 03:55, Shannon Zhao wrote: From: Shannon ZhaoSigned-off-by: Shannon Zhao Acked-by: Julien Grall Regards, --- tools/libxl/libxl_arm_acpi.c | 30 ++ 1 file changed, 30 insertions(+) diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c index 83ad954..93ba2d1 100644 --- a/tools/libxl/libxl_arm_acpi.c +++ b/tools/libxl/libxl_arm_acpi.c @@ -161,6 +161,35 @@ static void make_acpi_rsdp(libxl__gc *gc, struct xc_dom_image *dom, acpitables[RSDP].size); } +static void make_acpi_header(struct acpi_table_header *h, const char *sig, + size_t len, uint8_t rev) +{ +memcpy(h->signature, sig, 4); +h->length = len; +h->revision = rev; +memcpy(h->oem_id, ACPI_OEM_ID, sizeof(h->oem_id)); +memcpy(h->oem_table_id, ACPI_OEM_TABLE_ID, sizeof(h->oem_table_id)); +h->oem_revision = 0; +memcpy(h->asl_compiler_id, ACPI_ASL_COMPILER_ID, + sizeof(h->asl_compiler_id)); +h->asl_compiler_revision = 0; +h->checksum = 0; +} + +static void make_acpi_xsdt(libxl__gc *gc, struct xc_dom_image *dom, + struct acpitable acpitables[]) +{ +uint64_t offset = acpitables[XSDT].addr - GUEST_ACPI_BASE; +struct acpi_table_xsdt *xsdt = (void *)dom->acpi_modules[0].data + offset; + +xsdt->table_offset_entry[0] = acpitables[MADT].addr; +xsdt->table_offset_entry[1] = acpitables[GTDT].addr; +xsdt->table_offset_entry[2] = acpitables[FADT].addr; +make_acpi_header(>header, "XSDT", acpitables[XSDT].size, 1); +calculate_checksum(xsdt, offsetof(struct acpi_table_header, checksum), + acpitables[XSDT].size); +} + int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info, libxl__domain_build_state *state, struct xc_dom_image *dom) @@ -190,6 +219,7 @@ int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info, goto out; make_acpi_rsdp(gc, dom, acpitables); +make_acpi_xsdt(gc, dom, acpitables); out: return rc; -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC] x86/vm_event: Allow returning i-cache for emulation
On Sep 12, 2016 08:59, "George Dunlap"wrote: > > On 12/09/16 15:56, Jan Beulich wrote: > On 09.09.16 at 17:41, wrote: > >> When emulating instructions the emulator maintains a small i-cache fetched > >> from the guest memory. Under certain scenarios this memory region may contain > >> instructions that a monitor subscriber would prefer to hide, namely INT3, and > >> instead would prefer to emulate a different instruction in-place. > >> > >> This patch extends the vm_event interface to allow returning this i-cache via > >> the vm_event response. > > > > Please try to explain better what this change is about, perhaps along > > the lines of what George used as description, which you then > > confirmed. > > > >> @@ -1783,7 +1786,14 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt, > >> pfec |= PFEC_user_mode; > >> > >> hvmemul_ctxt->insn_buf_eip = regs->eip; > >> -if ( !vio->mmio_insn_bytes ) > >> + > >> +if ( unlikely(hvmemul_ctxt->set_context_insn) ) > >> +{ > >> +memcpy(hvmemul_ctxt->insn_buf, curr->arch.vm_event->emul_data.data, > >> + curr->arch.vm_event->emul_data.size); > >> +hvmemul_ctxt->insn_buf_bytes = curr->arch.vm_event->emul_data.size; > >> +} > >> +else if ( !vio->mmio_insn_bytes ) > > > > I'm not sure about this ordering: Do you really mean to allow an > > external entity to override insn bytes e.g. in an MMIO retry, i.e. > > allowing the retried insn to be different from the original one? > > > > And additionally I think you need to deal with the case of the > > supplied data not being a full insn. There shouldn't be any > > fetching from guest memory in that case imo, emulation should > > just fail. > > This patch was marked "RFC", which to me means, "This isn't ready for a > full review, but what do you think of the basic idea"? > > I would wait until Tamas submits a non-RFC patch before doing a detailed > review, as it's quite possible this was just a prototype he didn't want > to bother fixing up until he knew it would be accepted. > > -George Or that it even makes sense for others too :) I still appreciate the review though. Tamas ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 02/24] xen: credit1: fix mask to be used for tickling in Credit1
On Thu, Aug 18, 2016 at 12:42 AM, Dario Faggioliwrote: > On Wed, 2016-08-17 at 19:17 +0200, Dario Faggioli wrote: >> If there are idle pcpus inside the waking vcpu's >> soft-affinity mask, we should really tickle one >> of them (this is one of the purposes of the >> __runq_tickle() function itself!), not just >> any idle pcpu. >> >> The issue has been introduced in 02ea5031825d >> ("credit1: properly deal with pCPUs not in any cpupool"), >> where the usage of idle_mask is changed, without >> updating the bottom of the function, where it >> is also referenced. >> >> Signed-off-by: Dario Faggioli Acked-by: George Dunlap ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC] x86/vm_event: Allow returning i-cache for emulation
On 12/09/16 15:56, Jan Beulich wrote: On 09.09.16 at 17:41,wrote: >> When emulating instructions the emulator maintains a small i-cache fetched >> from the guest memory. Under certain scenarios this memory region may contain >> instructions that a monitor subscriber would prefer to hide, namely INT3, and >> instead would prefer to emulate a different instruction in-place. >> >> This patch extends the vm_event interface to allow returning this i-cache via >> the vm_event response. > > Please try to explain better what this change is about, perhaps along > the lines of what George used as description, which you then > confirmed. > >> @@ -1783,7 +1786,14 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt >> *hvmemul_ctxt, >> pfec |= PFEC_user_mode; >> >> hvmemul_ctxt->insn_buf_eip = regs->eip; >> -if ( !vio->mmio_insn_bytes ) >> + >> +if ( unlikely(hvmemul_ctxt->set_context_insn) ) >> +{ >> +memcpy(hvmemul_ctxt->insn_buf, curr->arch.vm_event->emul_data.data, >> + curr->arch.vm_event->emul_data.size); >> +hvmemul_ctxt->insn_buf_bytes = curr->arch.vm_event->emul_data.size; >> +} >> +else if ( !vio->mmio_insn_bytes ) > > I'm not sure about this ordering: Do you really mean to allow an > external entity to override insn bytes e.g. in an MMIO retry, i.e. > allowing the retried insn to be different from the original one? > > And additionally I think you need to deal with the case of the > supplied data not being a full insn. There shouldn't be any > fetching from guest memory in that case imo, emulation should > just fail. This patch was marked "RFC", which to me means, "This isn't ready for a full review, but what do you think of the basic idea"? I would wait until Tamas submits a non-RFC patch before doing a detailed review, as it's quite possible this was just a prototype he didn't want to bother fixing up until he knew it would be accepted. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 05/16] libxl/arm: Construct ACPI RSDP table
Hi Shannon, On 02/09/16 03:55, Shannon Zhao wrote: From: Shannon ZhaoConstruct ACPI RSDP table and add a helper to calculate the ACPI table checksum. Signed-off-by: Shannon Zhao Acked-by: Julien Grall Regards, --- tools/libxl/libxl_arm_acpi.c | 39 +++ 1 file changed, 39 insertions(+) diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c index b91f3f6..83ad954 100644 --- a/tools/libxl/libxl_arm_acpi.c +++ b/tools/libxl/libxl_arm_acpi.c @@ -34,6 +34,10 @@ extern const unsigned char dsdt_anycpu_arm[]; _hidden extern const int dsdt_anycpu_arm_len; +#define ACPI_OEM_ID "Xen" +#define ACPI_OEM_TABLE_ID "ARM" +#define ACPI_ASL_COMPILER_ID "XL" + enum { RSDP, XSDT, @@ -126,6 +130,37 @@ out: return rc; } +static void calculate_checksum(void *table, uint32_t checksum_offset, + uint32_t length) +{ +uint8_t *p, sum = 0; + +p = table; +p[checksum_offset] = 0; + +while (length--) +sum = sum + *p++; + +p = table; +p[checksum_offset] = -sum; +} + +static void make_acpi_rsdp(libxl__gc *gc, struct xc_dom_image *dom, + struct acpitable acpitables[]) +{ +uint64_t offset = acpitables[RSDP].addr - GUEST_ACPI_BASE; +struct acpi_table_rsdp *rsdp = (void *)dom->acpi_modules[0].data + offset; + +memcpy(rsdp->signature, "RSD PTR ", sizeof(rsdp->signature)); +memcpy(rsdp->oem_id, ACPI_OEM_ID, sizeof(rsdp->oem_id)); +rsdp->length = acpitables[RSDP].size; +rsdp->revision = 0x02; +rsdp->xsdt_physical_address = acpitables[XSDT].addr; +calculate_checksum(rsdp, + offsetof(struct acpi_table_rsdp, extended_checksum), + acpitables[RSDP].size); +} + int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info, libxl__domain_build_state *state, struct xc_dom_image *dom) @@ -151,6 +186,10 @@ int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info, dom->acpi_modules[0].guest_addr_out = GUEST_ACPI_BASE; rc = libxl__estimate_acpi_size(gc, info, dom, xc_config, acpitables); +if (rc) +goto out; + +make_acpi_rsdp(gc, dom, acpitables); out: return rc; -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC] x86/vm_event: Allow returning i-cache for emulation
>>> On 09.09.16 at 17:41,wrote: > When emulating instructions the emulator maintains a small i-cache fetched > from the guest memory. Under certain scenarios this memory region may contain > instructions that a monitor subscriber would prefer to hide, namely INT3, and > instead would prefer to emulate a different instruction in-place. > > This patch extends the vm_event interface to allow returning this i-cache via > the vm_event response. Please try to explain better what this change is about, perhaps along the lines of what George used as description, which you then confirmed. > @@ -1783,7 +1786,14 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt > *hvmemul_ctxt, > pfec |= PFEC_user_mode; > > hvmemul_ctxt->insn_buf_eip = regs->eip; > -if ( !vio->mmio_insn_bytes ) > + > +if ( unlikely(hvmemul_ctxt->set_context_insn) ) > +{ > +memcpy(hvmemul_ctxt->insn_buf, curr->arch.vm_event->emul_data.data, > + curr->arch.vm_event->emul_data.size); > +hvmemul_ctxt->insn_buf_bytes = curr->arch.vm_event->emul_data.size; > +} > +else if ( !vio->mmio_insn_bytes ) I'm not sure about this ordering: Do you really mean to allow an external entity to override insn bytes e.g. in an MMIO retry, i.e. allowing the retried insn to be different from the original one? And additionally I think you need to deal with the case of the supplied data not being a full insn. There shouldn't be any fetching from guest memory in that case imo, emulation should just fail. > @@ -1927,17 +1937,19 @@ void hvm_mem_access_emulate_one(enum emul_kind kind, > unsigned int trapnr, > struct hvm_emulate_ctxt ctx = {{ 0 }}; > int rc; > > +gdprintk(XENLOG_WARNING, "memaccess emulate one called\n"); > + > hvm_emulate_prepare(, guest_cpu_user_regs()); > > -switch ( kind ) > -{ > -case EMUL_KIND_NOWRITE: > +if ( kind == EMUL_KIND_NOWRITE ) > rc = hvm_emulate_one_no_write(); > -break; > -case EMUL_KIND_SET_CONTEXT: > -ctx.set_context = 1; > -/* Intentional fall-through. */ > -default: > +else > +{ > + if ( kind == EMUL_KIND_SET_CONTEXT_DATA ) > +ctx.set_context_data = 1; > + else if ( kind == EMUL_KIND_SET_CONTEXT_INSN ) > +ctx.set_context_insn = 1; > + > rc = hvm_emulate_one(); > } Could you try to retain the switch() statement? > --- a/xen/arch/x86/vm_event.c > +++ b/xen/arch/x86/vm_event.c > @@ -66,6 +66,16 @@ void vm_event_toggle_singlestep(struct domain *d, struct > vcpu *v) > hvm_toggle_singlestep(v); > } > > +void vm_event_interrupt_emulate_check(struct vcpu *v, vm_event_response_t > *rsp) > +{ > +if ( !!(rsp->flags & VM_EVENT_FLAG_EMULATE) && > + !!(rsp->flags & VM_EVENT_FLAG_SET_EMUL_INSN_DATA) ) Please avoid !! when not really needed. > +{ > +v->arch.vm_event->emulate_flags = rsp->flags; > +v->arch.vm_event->emul_data = rsp->data.emul_data; > +} > +} Overall I'm having difficulty associating the function's name with what it does. > --- a/xen/common/vm_event.c > +++ b/xen/common/vm_event.c > @@ -407,8 +407,11 @@ void vm_event_resume(struct domain *d, struct > vm_event_domain *ved) > vm_event_register_write_resume(v, ); > break; > > +case VM_EVENT_REASON_SOFTWARE_BREAKPOINT: > +vm_event_interrupt_emulate_check(v, ); > +break; > + > #ifdef CONFIG_HAS_MEM_ACCESS > -case VM_EVENT_REASON_MEM_ACCESS: > mem_access_resume(v, ); > break; I don't think you meant to remove that line? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 03/16] libxl/arm: Generate static ACPI DSDT table
Hi Shannon, On 02/09/16 03:55, Shannon Zhao wrote: From: Shannon ZhaoIt uses static DSDT table like the way x86 uses. Currently the DSDT table only contains processor device objects and it generates the maximal objects which so far is 128. While the GUEST_MAX_VCPUS is defined under __XEN__ or __XEN_TOOLS__, it needs to add -D__XEN_TOOLS__ to compile mk_dsdt.c. Also only check iasl for aarch64 in configure since ACPI on ARM32 is not supported. Signed-off-by: Shannon Zhao Acked-by: Julien Grall Regards, --- Note: this patch needs to be rebased on Boris's v3 patchset for only generating dsdt_anycpu_arm.c for ARM64. --- tools/configure.ac| 2 +- tools/libacpi/Makefile| 13 - tools/libacpi/mk_dsdt.c | 27 ++- tools/libxl/Makefile | 5 - tools/libxl/libxl_arm_acpi.c | 5 + xen/arch/arm/domain.c | 1 + xen/include/public/arch-arm.h | 3 +++ 7 files changed, 52 insertions(+), 4 deletions(-) diff --git a/tools/configure.ac b/tools/configure.ac index 0229d44..b4e0c80 100644 --- a/tools/configure.ac +++ b/tools/configure.ac @@ -335,7 +335,7 @@ dnl "host" here means the platform on which the hypervisor and tools is dnl going to run, not the platform on which we are building (known as dnl "build" in gnu speak). case "$host_cpu" in -i[[3456]]86|x86_64) +i[[3456]]86|x86_64|aarch64) AX_PATH_PROG_OR_FAIL([IASL], [iasl]) ;; esac diff --git a/tools/libacpi/Makefile b/tools/libacpi/Makefile index d741ac5..b1965cc 100644 --- a/tools/libacpi/Makefile +++ b/tools/libacpi/Makefile @@ -19,6 +19,7 @@ MK_DSDT = $(ACPI_BUILD_DIR)/mk_dsdt # Sources to be generated C_SRC = $(addprefix $(ACPI_BUILD_DIR)/, dsdt_anycpu.c dsdt_15cpu.c dsdt_anycpu_qemu_xen.c dsdt_pvh.c) +C_SRC += $(ACPI_BUILD_DIR)/dsdt_anycpu_arm.c H_SRC = $(addprefix $(ACPI_BUILD_DIR)/, ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h) vpath iasl $(PATH) @@ -32,7 +33,7 @@ $(H_SRC): $(ACPI_BUILD_DIR)/%.h: %.asl iasl cd $(CURDIR) $(MK_DSDT): mk_dsdt.c - $(HOSTCC) $(HOSTCFLAGS) $(CFLAGS_xeninclude) -o $@ mk_dsdt.c + $(HOSTCC) $(HOSTCFLAGS) $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -o $@ mk_dsdt.c $(ACPI_BUILD_DIR)/dsdt_anycpu_qemu_xen.asl: dsdt.asl dsdt_acpi_info.asl $(MK_DSDT) awk 'NR > 1 {print s} {s=$$0}' $< > $@ @@ -62,6 +63,16 @@ $(ACPI_BUILD_DIR)/dsdt_pvh.c: iasl $(ACPI_BUILD_DIR)/dsdt_pvh.asl echo "int dsdt_pvh_len=sizeof(dsdt_pvh);" >>$@ rm -f $(ACPI_BUILD_DIR)/$*.aml $(ACPI_BUILD_DIR)/$*.hex +$(ACPI_BUILD_DIR)/dsdt_anycpu_arm.asl: $(MK_DSDT) + printf "DefinitionBlock (\"DSDT.aml\", \"DSDT\", 3, \"Xen\", \"ARM\", 1)\n{" > $@ + $(MK_DSDT) --debug=$(debug) >> $@ + +$(ACPI_BUILD_DIR)/dsdt_anycpu_arm.c: iasl $(ACPI_BUILD_DIR)/dsdt_anycpu_arm.asl + iasl -vs -p $(ACPI_BUILD_DIR)/$* -tc $(ACPI_BUILD_DIR)/dsdt_anycpu_arm.asl + sed -e 's/AmlCode/dsdt_anycpu_arm/g' $(ACPI_BUILD_DIR)/$*.hex >$@ + echo "int dsdt_anycpu_arm_len=sizeof(dsdt_anycpu_arm);" >>$@ + rm -f $(ACPI_BUILD_DIR)/$*.aml $(ACPI_BUILD_DIR)/$*.hex + iasl: @echo @echo "ACPI ASL compiler (iasl) is needed" diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c index 7d76784..a56e0f0 100644 --- a/tools/libacpi/mk_dsdt.c +++ b/tools/libacpi/mk_dsdt.c @@ -17,7 +17,11 @@ #include #include #include +#if defined(__i386__) || defined(__x86_64__) #include +#elif defined(__aarch64__) +#include +#endif static unsigned int indent_level; static bool debug = false; @@ -104,10 +108,16 @@ static struct option options[] = { int main(int argc, char **argv) { -unsigned int slot, dev, intx, link, cpu, max_cpus = HVM_MAX_VCPUS; +unsigned int slot, dev, intx, link, cpu, max_cpus; dm_version dm_version = QEMU_XEN_TRADITIONAL; bool no_dm = 0; +#if defined(__i386__) || defined(__x86_64__) +max_cpus = HVM_MAX_VCPUS; +#elif defined(__aarch64__) +max_cpus = GUEST_MAX_VCPUS; +#endif + for ( ; ; ) { int opt = getopt_long(argc, argv, "", options, NULL); @@ -161,6 +171,7 @@ int main(int argc, char **argv) / Processor start / push_block("Scope", "\\_SB"); +#if defined(__i386__) || defined(__x86_64__) /* MADT checksum */ stmt("OperationRegion", "MSUM, SystemMemory, \\_SB.MSUA, 1"); push_block("Field", "MSUM, ByteAcc, NoLock, Preserve"); @@ -174,6 +185,7 @@ int main(int argc, char **argv) pop_block(); stmt("Return", "Buffer() {0, 8, 0xff, 0xff, 0, 0, 0, 0}"); pop_block(); +#endif /* Define processor objects and control methods. */ for ( cpu = 0; cpu < max_cpus; cpu++) @@ -182,6 +194,11 @@ int main(int argc, char **argv) stmt("Name", "_HID, \"ACPI0007\""); +stmt("Name", "_UID, %d", cpu); +#if defined(__aarch64__) +pop_block(); +continue; +#endif /* Name this processor's MADT LAPIC
Re: [Xen-devel] [PATCH v5 02/16] libxl/arm: prepare for constructing ACPI tables
Hi Shannon, On 02/09/16 03:55, Shannon Zhao wrote: From: Shannon ZhaoIt only constructs the ACPI tables for 64-bit ARM DomU when user enables acpi because 32-bit DomU doesn't support ACPI. And the generation codes are only built for 64-bit toolstack. Signed-off-by: Shannon Zhao Acked-by: Julien Grall Regards, --- tools/libxl/Makefile| 7 + tools/libxl/libxl_arm.c | 24 +++- tools/libxl/libxl_arm.h | 33 ++ tools/libxl/libxl_arm_acpi.c| 62 + tools/libxl/libxl_arm_no_acpi.c | 34 ++ xen/include/public/arch-arm.h | 4 +++ 6 files changed, 163 insertions(+), 1 deletion(-) create mode 100644 tools/libxl/libxl_arm.h create mode 100644 tools/libxl/libxl_arm_acpi.c create mode 100644 tools/libxl/libxl_arm_no_acpi.c diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile index a148374..afd93de 100644 --- a/tools/libxl/Makefile +++ b/tools/libxl/Makefile @@ -90,6 +90,13 @@ acpi: LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o libxl_psr.o libxl_x86_acpi.o LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o libxl_libfdt_compat.o +ifeq ($(CONFIG_ARM_64),y) +LIBXL_OBJS-y += libxl_arm_acpi.o +libxl_arm_acpi.o: libxl_arm_acpi.c + $(CC) -c $(CFLAGS) -I../../xen/include/ -o $@ libxl_arm_acpi.c +else +LIBXL_OBJS-$(CONFIG_ARM) += libxl_arm_no_acpi.o +endif ifeq ($(CONFIG_NetBSD),y) LIBXL_OBJS-y += libxl_netbsd.o diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index 8ec5cd5..333c9a1 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -1,6 +1,7 @@ #include "libxl_internal.h" #include "libxl_arch.h" #include "libxl_libfdt_compat.h" +#include "libxl_arm.h" #include #include @@ -885,8 +886,29 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc, libxl__domain_build_state *state, struct xc_dom_image *dom) { +int rc; + assert(info->type == LIBXL_DOMAIN_TYPE_PV); -return libxl__prepare_dtb(gc, info, state, dom); +rc = libxl__prepare_dtb(gc, info, state, dom); +if (rc) goto out; + +if (!libxl_defbool_val(info->acpi)) { +LOG(DEBUG, "Generating ACPI tables is disabled by user."); +rc = 0; +goto out; +} + +if (strcmp(dom->guest_type, "xen-3.0-aarch64")) { +/* ACPI is only supported for 64-bit guest currently. */ +LOG(ERROR, "Can not enable libxl option 'acpi' for %s", dom->guest_type); +rc = ERROR_FAIL; +goto out; +} + +rc = libxl__prepare_acpi(gc, info, state, dom); + +out: +return rc; } static void finalise_one_memory_node(libxl__gc *gc, void *fdt, diff --git a/tools/libxl/libxl_arm.h b/tools/libxl/libxl_arm.h new file mode 100644 index 000..1c01177 --- /dev/null +++ b/tools/libxl/libxl_arm.h @@ -0,0 +1,33 @@ +/* + * Copyright (C) 2016 Linaro Ltd. + * + * Author: Shannon Zhao + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published + * by the Free Software Foundation; version 2.1 only. with the special + * exception on linking described in file LICENSE. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + */ + +#include "libxl_internal.h" +#include "libxl_arch.h" + +#include + +_hidden +int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info, +libxl__domain_build_state *state, +struct xc_dom_image *dom); + +/* + * Local variables: + * mode: C + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c new file mode 100644 index 000..810aed8 --- /dev/null +++ b/tools/libxl/libxl_arm_acpi.c @@ -0,0 +1,62 @@ +/* + * ARM DomU ACPI generation + * + * Copyright (C) 2016 Linaro Ltd. + * + * Author: Shannon Zhao + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published + * by the Free Software Foundation; version 2.1 only. with the special + * exception on linking described in file LICENSE. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + */ + +#include "libxl_arm.h" + +#include + +/* Below typedefs are useful for the headers under acpi/ */ +typedef
Re: [Xen-devel] [RFC] x86/vm_event: Allow returning i-cache for emulation
On 12/09/16 15:31, Tamas Lengyel wrote: > On Sep 12, 2016 08:17, "George Dunlap"wrote: >> >> On 09/09/16 16:41, Tamas K Lengyel wrote: >>> When emulating instructions the emulator maintains a small i-cache > fetched >>> from the guest memory. Under certain scenarios this memory region may > contain >>> instructions that a monitor subscriber would prefer to hide, namely > INT3, and >>> instead would prefer to emulate a different instruction in-place. >>> >>> This patch extends the vm_event interface to allow returning this > i-cache via >>> the vm_event response. >> >> So do you have a problem right now with stale caches (i.e., you modify >> an INT3 back to something else in guest RAM but the emulator still >> emulates the INT3)? Or is the idea here that instead of doing the >> replace-singlestep-replace loop, you just tell the emulator, "Here, >> emulate this instead" (without removing the INT3 from guest memory at > all)? >> >> (Or am I completely missing the point here?) >> > > Hi George, > it's the latter! This would make tracing with int3s a bit more flexible on > multi-vcpu guests as there would be no racecondition. I use altp2m right > now to get around this problem but it's always good to have a backup in > case altp2m is disabled. OK -- in that case, it sounds like a good idea (particularly since there's a race I hadn't considered). :-) -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] Device model operation hypercall (DMOP, re qemu depriv)
>>> On 09.09.16 at 17:16,wrote: > The following code illustrates this idea: > > typedef struct dm_op_buffer { > XEN_GUEST_HANDLE(void) h; > size_t len; > } dm_op_buffer_t; This implies that we'll lose all type safety on the handles passed, as is also emphasized by the use of raw_copy_from_guest() in the code outline further down. > int > HYPERVISOR_device_model_op( > domid_t domid, > unsigned int nr_buffers, > XEN_GUEST_HANDLE_PARAM(dm_op_buffer_t) buffers) Along the same lines this will add implicit agreements (presumably solely written out as comments, or maybe via manifest constants) about which element of the buffer array has which meaning, quite contrary to the otherwise enforced agreement (through use of structure fields). > int copy_dm_buffer_from_guest( > void *dst,/* Kernel destination buffer */ > size_t max_len, /* Size of destination buffer */ > XEN_GUEST_HANDLE_PARAM(dm_op_buffer_t) buffers, >/* dm_op_buffers passed into DMOP */ > unsigned int nr_buffers, /* total number of dm_op_buffers */ > unsigned int idx) /* Index of buffer we require */ Looking at other hypercalls, it is not uncommon that arrays get read element by element. While of course this function can be extended suitably (perhaps with additional macro wrappers to deal with the base types of such arrays), it will then become more cumbersome to use, extending the "Minor stylistic issue" mentioned in the disadvantages section further down. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC] x86/vm_event: Allow returning i-cache for emulation
On Sep 12, 2016 08:17, "George Dunlap"wrote: > > On 09/09/16 16:41, Tamas K Lengyel wrote: > > When emulating instructions the emulator maintains a small i-cache fetched > > from the guest memory. Under certain scenarios this memory region may contain > > instructions that a monitor subscriber would prefer to hide, namely INT3, and > > instead would prefer to emulate a different instruction in-place. > > > > This patch extends the vm_event interface to allow returning this i-cache via > > the vm_event response. > > So do you have a problem right now with stale caches (i.e., you modify > an INT3 back to something else in guest RAM but the emulator still > emulates the INT3)? Or is the idea here that instead of doing the > replace-singlestep-replace loop, you just tell the emulator, "Here, > emulate this instead" (without removing the INT3 from guest memory at all)? > > (Or am I completely missing the point here?) > Hi George, it's the latter! This would make tracing with int3s a bit more flexible on multi-vcpu guests as there would be no racecondition. I use altp2m right now to get around this problem but it's always good to have a backup in case altp2m is disabled. Tamas ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 35/38] arm/p2m: Adjust debug information to altp2m
Hello Sergej, On 16/08/16 23:17, Sergej Proskurin wrote: Signed-off-by: Sergej Proskurin--- Cc: Stefano Stabellini Cc: Julien Grall --- v2: Dump p2m information of the hostp2m and all altp2m views. --- xen/arch/arm/p2m.c | 20 1 file changed, 20 insertions(+) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index dea3038..86e2a1d 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -162,6 +162,26 @@ void dump_p2m_lookup(struct domain *d, paddr_t addr) dump_pt_walk(page_to_maddr(p2m->root), addr, P2M_ROOT_LEVEL, P2M_ROOT_PAGES); +printk("\n"); + +if ( altp2m_active(d) ) +{ +unsigned int i; + +for ( i = 0; i < MAX_ALTP2M; i++ ) +{ +if ( d->arch.altp2m_p2m[i] == NULL ) +continue; + +p2m = d->arch.altp2m_p2m[i]; + +printk("AP2M[%d] @ %p mfn:0x%lx\n", s/%d/%u/ as the p2m index is unsigned s/0x%lx/%#PRI_mfn/ because we should avoid open format from now. +i, p2m->root, page_to_mfn(p2m->root)); + +dump_pt_walk(page_to_maddr(p2m->root), addr, P2M_ROOT_LEVEL, P2M_ROOT_PAGES); +printk("\n"); +} +} } void p2m_save_state(struct vcpu *p) Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 34/38] arm/p2m: Add HVMOP_altp2m_change_gfn
Hello Sergej, On 16/08/16 23:17, Sergej Proskurin wrote: This commit adds the functionality to change mfn mappings for specified gfn's in altp2m views. This mechanism can be used within the context of VMI, e.g., to establish stealthy debugging. Signed-off-by: Sergej Proskurin--- Cc: Stefano Stabellini Cc: Julien Grall --- v3: Moved the altp2m_lock to guard access to d->arch.altp2m_vttbr[idx] in altp2m_change_gfn. Locked hp2m to prevent hp2m entries from being modified while the function "altp2m_change_gfn" is active. Removed setting ap2m->mem_access_enabled in "altp2m_change_gfn", as we do not need explicitly splitting pages at this point. Extended checks allowing to change gfn's in p2m_ram_(rw|ro) memory only. Moved the funtion "remove_altp2m_entry" out of this commit. --- xen/arch/arm/altp2m.c| 98 xen/arch/arm/hvm.c | 7 +++- xen/include/asm-arm/altp2m.h | 6 +++ 3 files changed, 110 insertions(+), 1 deletion(-) diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c index 2009bad..fa8d526 100644 --- a/xen/arch/arm/altp2m.c +++ b/xen/arch/arm/altp2m.c @@ -301,6 +301,104 @@ out: return rc; } +int altp2m_change_gfn(struct domain *d, + unsigned int idx, + gfn_t old_gfn, + gfn_t new_gfn) +{ +struct p2m_domain *hp2m, *ap2m; +mfn_t mfn; +p2m_access_t p2ma; +p2m_type_t p2mt; +unsigned int page_order; +int rc = -EINVAL; + +hp2m = p2m_get_hostp2m(d); +ap2m = d->arch.altp2m_p2m[idx]; + +altp2m_lock(d); Similar comment as a previous patch, I am not sure to understand you lock using altp2m_lock and not p2m_write_lock(ap2m). The latter would make more sense as it will limit the number of TLB flush because all the change will be done in a batch. +p2m_read_lock(hp2m); + +if ( idx >= MAX_ALTP2M || d->arch.altp2m_p2m[idx] == NULL ) +goto out; + +mfn = p2m_lookup_attr(ap2m, old_gfn, , NULL, _order); + +/* Check whether the page needs to be reset. */ +if ( gfn_eq(new_gfn, INVALID_GFN) ) +{ +/* If mfn is mapped by old_gfn, remove old_gfn from the altp2m table. */ +if ( !mfn_eq(mfn, INVALID_MFN) ) +{ +rc = remove_altp2m_entry(ap2m, old_gfn, mfn, page_order); From my understanding, altp2m_change_gfn is working on page granularity. So here, you would remove a superpage even if the user requested to remove a specific gfn. +if ( rc ) +{ +rc = -EINVAL; +goto out; +} +} + +rc = 0; +goto out; +} + +/* Check hostp2m if no valid entry in altp2m present. */ +if ( mfn_eq(mfn, INVALID_MFN) ) +{ +mfn = p2m_lookup_attr(hp2m, old_gfn, , , _order); +if ( mfn_eq(mfn, INVALID_MFN) || + /* Allow changing gfn's in p2m_ram_(rw|ro) memory only. */ + ((p2mt != p2m_ram_rw) && (p2mt != p2m_ram_ro)) ) +{ +rc = -EINVAL; +goto out; +} + +/* If this is a superpage, copy that first. */ +if ( page_order != THIRD_ORDER ) +{ +rc = modify_altp2m_entry(ap2m, old_gfn, mfn, p2mt, p2ma, page_order); +if ( rc ) +{ +rc = -EINVAL; +goto out; +} +} +} + +mfn = p2m_lookup_attr(ap2m, new_gfn, , , _order); + +/* If new_gfn is not part of altp2m, get the mapping information from hp2m */ +if ( mfn_eq(mfn, INVALID_MFN) ) +mfn = p2m_lookup_attr(hp2m, new_gfn, , , _order); + +if ( mfn_eq(mfn, INVALID_MFN) || + /* Allow changing gfn's in p2m_ram_(rw|ro) memory only. */ + ((p2mt != p2m_ram_rw) && (p2mt != p2m_ram_ro)) ) +{ +rc = -EINVAL; +goto out; +} + +/* Set mem access attributes - currently supporting only one (4K) page. */ I don't understand why memaccess matters here. p2m_set_entry is handling for you the memaccess case. +page_order = THIRD_ORDER; +rc = modify_altp2m_entry(ap2m, old_gfn, mfn, p2mt, p2ma, page_order); +if ( rc ) +{ +rc = -EINVAL; +goto out; +} + +rc = 0; + +out: +p2m_read_unlock(hp2m); +altp2m_unlock(d); + +return rc; +} + + static void altp2m_vcpu_reset(struct vcpu *v) { struct altp2mvcpu *av = _vcpu(v); diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c index df78893..c754ad1 100644 --- a/xen/arch/arm/hvm.c +++ b/xen/arch/arm/hvm.c @@ -145,7 +145,12 @@ static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) break; case HVMOP_altp2m_change_gfn: -rc = -EOPNOTSUPP; +if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 ) +rc = -EINVAL; +else +rc = altp2m_change_gfn(d, a.u.change_gfn.view, +
Re: [Xen-devel] [PATCH v3 33/38] arm/p2m: Add altp2m paging mechanism
Hello Sergej, On 16/08/16 23:17, Sergej Proskurin wrote: This commit adds the function "altp2m_lazy_copy" implementing the altp2m paging mechanism. The function "altp2m_lazy_copy" lazily copies the hostp2m's mapping into the currently active altp2m view on 2nd stage translation faults on instruction or data access. Signed-off-by: Sergej Proskurin--- Cc: Stefano Stabellini Cc: Julien Grall --- v3: Cosmetic fixes. Locked hostp2m in the function "altp2m_lazy_copy" to avoid a mapping being changed in hostp2m before it has been inserted into the valtp2m view. Removed unnecessary calls to "p2m_mem_access_check" in the functions "do_trap_instr_abort_guest" and "do_trap_data_abort_guest" after a translation fault has been handled by the function "altp2m_lazy_copy". Adapted "altp2m_lazy_copy" to return the value "true" if the encountered translation fault encounters a valid entry inside of the currently active altp2m view. If multiple vcpu's are using the same altp2m, it is likely that both generate a translation fault, whereas the first one will be already handled by "altp2m_lazy_copy". With this change the 2nd vcpu will retry accessing the faulting address. Changed order of altp2m checking and MMIO emulation within the function "do_trap_data_abort_guest". Now, altp2m is checked and handled only if the MMIO does not have to be emulated. Changed the function prototype of "altp2m_lazy_copy". This commit removes the unnecessary struct p2m_domain* from the previous function prototype. Also, this commit removes the unnecessary argument gva. Finally, this commit changes the address of the function parameter gpa from paddr_t to gfn_t and renames it to gfn. Moved the altp2m handling mechanism into a separate function "try_handle_altp2m". Moved the functions "p2m_altp2m_check" and "altp2m_switch_vcpu_altp2m_by_id" out of this patch. Moved applied code movement into a separate patch. --- xen/arch/arm/altp2m.c| 62 xen/arch/arm/traps.c | 35 + xen/include/asm-arm/altp2m.h | 5 3 files changed, 102 insertions(+) diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c index 11272e9..2009bad 100644 --- a/xen/arch/arm/altp2m.c +++ b/xen/arch/arm/altp2m.c @@ -165,6 +165,68 @@ out: return rc; } +/* + * The function altp2m_lazy_copy returns "false" on error. The return value + * "true" signals that either the mapping has been successfully lazy-copied + * from the hostp2m to the currently active altp2m view or that the altp2m view + * holds already a valid mapping. The latter is the case if multiple vcpu's + * using the same altp2m view generate a translation fault that is led back in + * both cases to the same mapping and the first fault has been already handled. + */ +bool_t altp2m_lazy_copy(struct vcpu *v, +gfn_t gfn, +struct npfec npfec) Please don't introduce parameters that are not used at all. +{ +struct domain *d = v->domain; +struct p2m_domain *hp2m = p2m_get_hostp2m(d), *ap2m = NULL; +p2m_type_t p2mt; +p2m_access_t p2ma; +mfn_t mfn; +unsigned int page_order; +int rc; + +ap2m = altp2m_get_altp2m(v); +if ( ap2m == NULL) +return false; + +/* Check if entry is part of the altp2m view. */ +mfn = p2m_lookup_attr(ap2m, gfn, NULL, NULL, NULL); +if ( !mfn_eq(mfn, INVALID_MFN) ) +/* + * If multiple vcpu's are using the same altp2m, it is likely that both s/vcpu's/vcpus/ + * generate a translation fault, whereas the first one will be handled + * successfully and the second will encounter a valid mapping that has + * already been added as a result of the previous translation fault. + * In this case, the 2nd vcpu need to retry accessing the faulting s/need/needs/ + * address. + */ +return true; + +/* + * Lock hp2m to prevent the hostp2m to change a mapping before it is added + * to the altp2m view. + */ +p2m_read_lock(hp2m); + +/* Check if entry is part of the host p2m view. */ +mfn = p2m_lookup_attr(hp2m, gfn, , , _order); +if ( mfn_eq(mfn, INVALID_MFN) ) +goto out; + +rc = modify_altp2m_entry(ap2m, gfn, mfn, p2mt, p2ma, page_order); +if ( rc ) +{ +gdprintk(XENLOG_ERR, "altp2m[%d] failed to set entry for %#"PRI_gfn" -> %#"PRI_mfn"\n", p2midx is unsigned so this should be %u. + altp2m_vcpu(v).p2midx, gfn_x(gfn), mfn_x(mfn)); +domain_crash(hp2m->domain); You already have the domain in hand with 'd'. +} + +out: +p2m_read_unlock(hp2m); + +return true; +} + static inline void altp2m_reset(struct p2m_domain *p2m) { p2m_write_lock(p2m); diff --git
Re: [Xen-devel] [RFC] x86/vm_event: Allow returning i-cache for emulation
On 09/09/16 16:41, Tamas K Lengyel wrote: > When emulating instructions the emulator maintains a small i-cache fetched > from the guest memory. Under certain scenarios this memory region may contain > instructions that a monitor subscriber would prefer to hide, namely INT3, and > instead would prefer to emulate a different instruction in-place. > > This patch extends the vm_event interface to allow returning this i-cache via > the vm_event response. So do you have a problem right now with stale caches (i.e., you modify an INT3 back to something else in guest RAM but the emulator still emulates the INT3)? Or is the idea here that instead of doing the replace-singlestep-replace loop, you just tell the emulator, "Here, emulate this instead" (without removing the INT3 from guest memory at all)? (Or am I completely missing the point here?) -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] x86/xstate: Fix latent bugs in expand_xsave_states()
>>> On 12.09.16 at 15:57,wrote: > On 12/09/16 13:43, Jan Beulich wrote: > On 12.09.16 at 14:29, wrote: >>> On 12/09/16 12:41, Jan Beulich wrote: >>> On 12.09.16 at 11:51, wrote: > @@ -205,11 +222,9 @@ void expand_xsave_states(struct vcpu *v, void *dest, > unsigned int size) > > if ( src ) > { > -ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= > size); > +BUG_ON((xstate_offsets[index] + xstate_sizes[index]) <= > size); Surely converting an ASSERT() to BUG_ON() means inverting the relational operator used? >>> Very true. It is unfortunate that all of this is dead code, and >>> impossible to test. I also had half a mind to explicitly #if 0 it out >>> to leave people in no illusion that it ever might have been tested. >> So with this correct, the patch is then >> Reviewed-by: Jan Beulich > > The discussion on this patch has shown that the "if ( src )" is > unconditionally true, and as such, I would like to remove it. Would > your R-b stand with this hunk altered similarly to the final hunk in > patch 6 (with the BUG_ON() logic adjustment, and updated wording) ? Yes. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] x86/xstate: Fix latent bugs in expand_xsave_states()
On 12/09/16 13:43, Jan Beulich wrote: On 12.09.16 at 14:29,wrote: >> On 12/09/16 12:41, Jan Beulich wrote: >> On 12.09.16 at 11:51, wrote: @@ -205,11 +222,9 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size) if ( src ) { -ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size); +BUG_ON((xstate_offsets[index] + xstate_sizes[index]) <= size); >>> Surely converting an ASSERT() to BUG_ON() means inverting the >>> relational operator used? >> Very true. It is unfortunate that all of this is dead code, and >> impossible to test. I also had half a mind to explicitly #if 0 it out >> to leave people in no illusion that it ever might have been tested. > So with this correct, the patch is then > Reviewed-by: Jan Beulich The discussion on this patch has shown that the "if ( src )" is unconditionally true, and as such, I would like to remove it. Would your R-b stand with this hunk altered similarly to the final hunk in patch 6 (with the BUG_ON() logic adjustment, and updated wording) ? ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 32/38] arm/p2m: Code movement in instr/data abort handlers
Hi Sergej, On 16/08/16 23:17, Sergej Proskurin wrote: This commit moves code in the functions "do_trap_data_(instr|abort)_guest" without changing the original functionality. The code movement is limited to moving the struct npfec out of the switch statements in both functions. This commit acts as a basis for the following commit implementing the altp2m paging mechanism. Looking at the implementation on the next patch, it is passed as an argument but never used but the caller. So in the current state, this patch is not useful at all. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 6/6] x86/xstate: Fix latent bugs in compress_xsave_states()
>>> On 12.09.16 at 14:59,wrote: > On 12/09/16 13:27, Jan Beulich wrote: > On 12.09.16 at 11:51, wrote: >>> void compress_xsave_states(struct vcpu *v, const void *src, unsigned int >>> size) >>> { >>> struct xsave_struct *xsave = v->arch.xsave_area; >>> uint16_t comp_offsets[sizeof(xfeature_mask)*8]; >>> -u64 xstate_bv = ((const struct xsave_struct >>> *)src)->xsave_hdr.xstate_bv; >>> -u64 valid; >>> +u64 xstate_bv, valid; >>> + >>> +BUG_ON(!v->arch.xcr0_accum); >>> +BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum)); >>> +BUG_ON(xsave_area_compressed(src)); >>> >>> -ASSERT(!xsave_area_compressed(src)); >> Same remark here as on the earlier patch wrt BUG_ON() vs ASSERT(). > > Same answer. Well, it's certainly a matter of taste how much of the above to consider bounds checking. I for one would take only the middle one as such. >>> +xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv; >>> >>> if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) ) >>> { >>> +/* >>> + * TODO: This logic doesn't currently handle restoration of xsave >>> + * state which would force the vcpu from uncompressed to >>> compressed. >>> + */ >>> +BUG_ON(xstate_bv & XSTATE_XSAVES_ONLY); >> I don't think this is a valid concern of yours: The function can't be >> used to restore features not xcr0_accum anyway (or else that >> field would need updating). Afaict validate_xstate() already prevents >> this as intended. > > This is all currently dead code. I guess the question really depends on > what we plan to do with compressed states. > > Strictly speaking, no XSAVES state can every be present in xcr0, by > design. If we retroactively consider xcr0_accum to be "all states in > use", I think that's the only viable model, considering how the domctl works: xcr0_accum needs to represent the combination of features ever enabled in XCR0 and XSS. > then the if condition in context does become relevant when Xen > starts supporting XSAVES-only components. > > In such a case, it is definitely wrong to memcpy() the uncompressed > buffer, as Xen will try and use xrstors and corrupt all guest state. How? If the guest never enabled any bit in XSS, how can any such bit be set in xstate_bv (which is always a subset of XCR0|XSS). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] Device model operation hypercall (DMOP, re qemu depriv)
On 09/09/16 16:16, Jennifer Herbert wrote: > The following code illustrates this idea: > > typedef struct dm_op_buffer { > XEN_GUEST_HANDLE(void) h; > size_t len; > } dm_op_buffer_t; > > int > HYPERVISOR_device_model_op( > domid_t domid, > unsigned int nr_buffers, > XEN_GUEST_HANDLE_PARAM(dm_op_buffer_t) buffers) > > @domid: the domain the hypercall operates on. > @nr_buffers; the number of buffers in the @buffers array. > > @buffers: an array of buffers. @buffers[0] contains device_model_op - the > structure describing the sub-op and its parameters. @buffers[1], > @buffers[2] > etc. may be used by a sub-op for passing additional buffers. > > struct device_model_op { > uint32_t op; > union { > struct op_1 op1; > struct op_2 op2; > /* etc... */ > } u; > }; > > It is forbidden for the above struct (device_model_op) to contain any > guest handles - if they are needed, they should instead be in > HYPERVISOR_device_model_op->buffers. Sounds plausible to me. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/6] x86/domctl: Fix migration of guests which are not using xsave
>>> On 12.09.16 at 14:46,wrote: > On 12/09/16 13:14, Jan Beulich wrote: > On 12.09.16 at 11:51, wrote: >>> --- a/xen/arch/x86/domctl.c >>> +++ b/xen/arch/x86/domctl.c >>> @@ -1158,7 +1158,15 @@ long arch_do_domctl( >>> goto vcpuextstate_out; >>> } >>> >>> -if ( evc->size <= PV_XSAVE_SIZE(_xcr0_accum) ) >>> +if ( evc->size == PV_XSAVE_HDR_SIZE ) >>> +; /* Nothing to restore. */ >>> +else if ( evc->size < PV_XSAVE_HDR_SIZE + XSTATE_AREA_MIN_SIZE >>> ) >>> +ret = -EINVAL; /* Can't be legitimate data. */ >>> +else if ( xsave_area_compressed(_xsave_area) ) >>> +ret = -EOPNOTSUPP; /* Don't support compressed data. */ >>> +else if ( evc->size != PV_XSAVE_SIZE(_xcr0_accum) ) >>> +ret = -EINVAL; /* Not legitimate data. */ >> Can't this be moved ahead of the xsave_area_compressed() check, >> eliminating (as redundant) the check that's there right now? > > No. That doesn't catch the case where _xcr0_accum is 0, which will > cause the xsave_area_compressed(_xsave_area) check to wander off the end > of the buffer. Oh, indeed. >> In any event the tightening to != you do here supports my desire to >> not do any relaxation of the size check in patch 2. > > The two cases are different, and should not be conflated. > >> Or alternatively >> you would want to consider restoring the behavior from prior to >> the xsaves change, where the <= which you now remove was still >> okay. > > I looked back through the history and couldn't find any justification > for the check being <= as opposed to being more strict. We absolutely > don't want to be in the case where we only restore part of an xstate > component. Indeed I don't think there was ever any justification provided, it just happened to be that way (perhaps having in mind that the FXSAVE layout had unused space at the end). >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -1345,6 +1345,23 @@ static int hvm_load_cpu_xsave_states(struct domain >>> *d, hvm_domain_context_t *h) >>> printk(XENLOG_G_WARNING >>> "HVM%d.%u restore mismatch: xsave length %#x > %#x\n", >>> d->domain_id, vcpuid, desc->length, size); >>> +/* Rewind desc->length to ignore the extraneous zeros. */ >>> +desc->length = size; >> This is slightly ugly, as it will prevent eventually constifying dest (which >> it really should have been from the beginning). > > Hmm yes. I hadn't considered that it was actually mutating the hvm > domain context buffer. I will switch to a shadow variable instead. > >> >>> --- a/xen/include/asm-x86/xstate.h >>> +++ b/xen/include/asm-x86/xstate.h >>> @@ -131,4 +131,10 @@ static inline bool_t xstate_all(const struct vcpu *v) >>> (v->arch.xcr0_accum & XSTATE_LAZY & ~XSTATE_FP_SSE); >>> } >>> >>> +static inline bool __nonnull(1) >>> +xsave_area_compressed(const struct xsave_struct *xsave_area) >> This is certainly odd indentation. > > It is where my editor naturally indents to. It would appear that emacs > is mistaking the __nonnull(1) as the function name, and presuming that > the following line is K style parameters. With these two small things taken care of, Reviewed-by: Jan Beulich ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/6] x86/domctl: Fix TOCTOU race with the use of XEN_DOMCTL_getvcpuextstate
>>> On 12.09.16 at 11:51,wrote: > A toolstack must call XEN_DOMCTL_getvcpuextstate twice; first to find the size > of the buffer to use, and a second time to get the actual content. > > The reported size was based on v->arch.xcr0_accum, but a guest which extends > its xcr0_accum between the two hypercalls will cause the toolstack to fail the > evc->size != size check, as the provided buffer is now too small. This causes > a hard error during the final phase of migration. > > Instead, return return a size based on xfeature_mask, which is the maximum > size Xen will ever permit. The hypercall must now tolerate a > toolstack-provided buffer which is overly large (for the case where a guest > isn't using all available xsave states), and should write back how much data > was actually written into the buffer. > > As the query for size now has no dependence on vcpu state, the vcpu_pause() > can be omitted for a small performance improvement. > > Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/6] x86/domctl: Fix TOCTOU race with the use of XEN_DOMCTL_getvcpuextstate
>>> On 12.09.16 at 15:09,wrote: > On 12/09/16 13:33, Jan Beulich wrote: > On 12.09.16 at 14:02, wrote: >>> On 12/09/16 12:17, Jan Beulich wrote: >>> On 12.09.16 at 11:51, wrote: > A toolstack must call XEN_DOMCTL_getvcpuextstate twice; first to find the >>> size > of the buffer to use, and a second time to get the actual content. > > The reported size was based on v->arch.xcr0_accum, but a guest which > extends > its xcr0_accum between the two hypercalls will cause the toolstack to > fail >>> the > evc->size != size check, as the provided buffer is now too small. This > causes > a hard error during the final phase of migration. > > Instead, return return a size based on xfeature_mask, which is the maximum > size Xen will ever permit. The hypercall must now tolerate a > toolstack-provided buffer which is overly large (for the case where a > guest > isn't using all available xsave states), and should write back how much > data > was actually written into the buffer. To be honest, I'm of two minds here. Part of me thinks this is an okay change. However, in particular ... > --- a/xen/arch/x86/domctl.c > +++ b/xen/arch/x86/domctl.c > @@ -1054,19 +1054,25 @@ long arch_do_domctl( > unsigned int size; > > ret = 0; > -vcpu_pause(v); > > -size = PV_XSAVE_SIZE(v->arch.xcr0_accum); > if ( (!evc->size && !evc->xfeature_mask) || > guest_handle_is_null(evc->buffer) ) > { > +/* > + * A query for the size of buffer to use. Must return > the > + * maximum size we ever might hand back to userspace, >>> bearing > + * in mind that the vcpu might increase its xcr0_accum >>> between > + * this query for size, and the following query for data. > + */ > evc->xfeature_mask = xfeature_mask; > -evc->size = size; > -vcpu_unpause(v); > +evc->size = PV_XSAVE_SIZE(xfeature_mask); > goto vcpuextstate_out; > } > > -if ( evc->size != size || evc->xfeature_mask != > xfeature_mask ) > +vcpu_pause(v); > +size = PV_XSAVE_SIZE(v->arch.xcr0_accum); > + > +if ( evc->size < size || evc->xfeature_mask != xfeature_mask > ) ... the relaxation from != to < looks somewhat fragile to me, going forward. >>> What is fragile about it? It is a very common pattern in general, and >>> we use it elsewhere. >> If we get handed too large a buffer, what meaning do you want to >> assign to the extra bytes? Them being meaningless is just one >> possible interpretation. > > I am confused, and I think you are too. There is no meaning to the > bytes; this is getvcpuextstate, not setvcpuextstate. Oh, indeed. I'm sorry. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/6] x86/domctl: Fix TOCTOU race with the use of XEN_DOMCTL_getvcpuextstate
On 12/09/16 13:33, Jan Beulich wrote: On 12.09.16 at 14:02,wrote: >> On 12/09/16 12:17, Jan Beulich wrote: >> On 12.09.16 at 11:51, wrote: A toolstack must call XEN_DOMCTL_getvcpuextstate twice; first to find the >> size of the buffer to use, and a second time to get the actual content. The reported size was based on v->arch.xcr0_accum, but a guest which extends its xcr0_accum between the two hypercalls will cause the toolstack to fail >> the evc->size != size check, as the provided buffer is now too small. This causes a hard error during the final phase of migration. Instead, return return a size based on xfeature_mask, which is the maximum size Xen will ever permit. The hypercall must now tolerate a toolstack-provided buffer which is overly large (for the case where a guest isn't using all available xsave states), and should write back how much data was actually written into the buffer. >>> To be honest, I'm of two minds here. Part of me thinks this is an >>> okay change. However, in particular ... >>> --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -1054,19 +1054,25 @@ long arch_do_domctl( unsigned int size; ret = 0; -vcpu_pause(v); -size = PV_XSAVE_SIZE(v->arch.xcr0_accum); if ( (!evc->size && !evc->xfeature_mask) || guest_handle_is_null(evc->buffer) ) { +/* + * A query for the size of buffer to use. Must return the + * maximum size we ever might hand back to userspace, >> bearing + * in mind that the vcpu might increase its xcr0_accum >> between + * this query for size, and the following query for data. + */ evc->xfeature_mask = xfeature_mask; -evc->size = size; -vcpu_unpause(v); +evc->size = PV_XSAVE_SIZE(xfeature_mask); goto vcpuextstate_out; } -if ( evc->size != size || evc->xfeature_mask != xfeature_mask ) +vcpu_pause(v); +size = PV_XSAVE_SIZE(v->arch.xcr0_accum); + +if ( evc->size < size || evc->xfeature_mask != xfeature_mask ) >>> ... the relaxation from != to < looks somewhat fragile to me, going >>> forward. >> What is fragile about it? It is a very common pattern in general, and >> we use it elsewhere. > If we get handed too large a buffer, what meaning do you want to > assign to the extra bytes? Them being meaningless is just one > possible interpretation. I am confused, and I think you are too. There is no meaning to the bytes; this is getvcpuextstate, not setvcpuextstate. All it means is that Xen doesn't need to use all the buffer space provided by the toolstack to write the current state into. It is exactly the same concept as read(fd, buf, 4096) returning 1024. Only some of the potential buffer was actually filled by the call. > >>> Did you consider dealing with the issue in the tool stack? >> Yes, and rejected doing so. >> >>> It can't be that hard to repeat the size query in case data retrieval fails. >> In principle, this is true if the size case was distinguishable from all >> the other cases which currently return -EINVAL. > It easily is, as long as the caller know what size it used on the most > recent earlier call (since it would then notice it having grown). > @@ -1103,6 +1109,10 @@ long arch_do_domctl( } vcpu_unpause(v); + +/* Specify how much data we actually wrote into the buffer. */ +if ( !ret ) +evc->size = size; >>> ... if this got written on the earlier error path, there wouldn't even >>> be a need to retry the size query: Data retrieval could be retried >>> with the new size right after enlarging the buffer. >> True, but userspace hypercalls are expensive for the domain in question, >> and take a global spinlock in Xen which is expensive for the system as a >> whole. Looking forward to PVH control domains, any hypercall will be >> far more expensive (a vmexit/entry vs syscall/sysret). >> >> As such, I am not happy introducing unnecessary hypercalls, as they are >> entirely detrimental to dom0 and Xen. > Let's be realistic: How often do you expect xcr0_accum to change > in practice between the size query and the data retrieval? Very very slim, but not 0. > I'm perfectly fine fixing the theoretical issue, but I don't think there's > a performance aspect to be considered when deciding how to deal > with it. This doesn't mean we should fix it in an inefficient way. ~Andrew
Re: [Xen-devel] [PATCH 6/6] x86/xstate: Fix latent bugs in compress_xsave_states()
On 12/09/16 13:27, Jan Beulich wrote: On 12.09.16 at 11:51,wrote: >> void compress_xsave_states(struct vcpu *v, const void *src, unsigned int >> size) >> { >> struct xsave_struct *xsave = v->arch.xsave_area; >> uint16_t comp_offsets[sizeof(xfeature_mask)*8]; >> -u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv; >> -u64 valid; >> +u64 xstate_bv, valid; >> + >> +BUG_ON(!v->arch.xcr0_accum); >> +BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum)); >> +BUG_ON(xsave_area_compressed(src)); >> >> -ASSERT(!xsave_area_compressed(src)); > Same remark here as on the earlier patch wrt BUG_ON() vs ASSERT(). Same answer. > >> +xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv; >> >> if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) ) >> { >> +/* >> + * TODO: This logic doesn't currently handle restoration of xsave >> + * state which would force the vcpu from uncompressed to compressed. >> + */ >> +BUG_ON(xstate_bv & XSTATE_XSAVES_ONLY); > I don't think this is a valid concern of yours: The function can't be > used to restore features not xcr0_accum anyway (or else that > field would need updating). Afaict validate_xstate() already prevents > this as intended. This is all currently dead code. I guess the question really depends on what we plan to do with compressed states. Strictly speaking, no XSAVES state can every be present in xcr0, by design. If we retroactively consider xcr0_accum to be "all states in use", then the if condition in context does become relevant when Xen starts supporting XSAVES-only components. In such a case, it is definitely wrong to memcpy() the uncompressed buffer, as Xen will try and use xrstors and corrupt all guest state. > >> @@ -262,11 +281,13 @@ void compress_xsave_states(struct vcpu *v, const void >> *src, unsigned int size) >> unsigned int index = fls(feature) - 1; >> void *dest = get_xsave_addr(xsave, comp_offsets, index); >> >> -if ( dest ) >> -{ >> -ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size); >> -memcpy(dest, src + xstate_offsets[index], xstate_sizes[index]); >> -} >> +/* >> + * We previously verified xstate_bv. If we don't have valid >> + * comp_offset[] information, something is very broken. >> + */ >> +BUG_ON(!dest); > Are you sure? In compressed format bits in xstate_bv may be clear > while xcomp_bv has them set. In such a case there's nowhere to > copy to, and hence dest is NULL when coming here. This logic was based on my wrong analysis. However, this loop only executes for bits set in xstate_bv, so "if ( dest )" will never be false. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 6/6] x86/xstate: Fix latent bugs in compress_xsave_states()
>>> On 12.09.16 at 11:51,wrote: > void compress_xsave_states(struct vcpu *v, const void *src, unsigned int > size) > { > struct xsave_struct *xsave = v->arch.xsave_area; > uint16_t comp_offsets[sizeof(xfeature_mask)*8]; > -u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv; > -u64 valid; > +u64 xstate_bv, valid; > + > +BUG_ON(!v->arch.xcr0_accum); > +BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum)); > +BUG_ON(xsave_area_compressed(src)); > > -ASSERT(!xsave_area_compressed(src)); Same remark here as on the earlier patch wrt BUG_ON() vs ASSERT(). > +xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv; > > if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) ) > { > +/* > + * TODO: This logic doesn't currently handle restoration of xsave > + * state which would force the vcpu from uncompressed to compressed. > + */ > +BUG_ON(xstate_bv & XSTATE_XSAVES_ONLY); I don't think this is a valid concern of yours: The function can't be used to restore features not xcr0_accum anyway (or else that field would need updating). Afaict validate_xstate() already prevents this as intended. > @@ -262,11 +281,13 @@ void compress_xsave_states(struct vcpu *v, const void > *src, unsigned int size) > unsigned int index = fls(feature) - 1; > void *dest = get_xsave_addr(xsave, comp_offsets, index); > > -if ( dest ) > -{ > -ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size); > -memcpy(dest, src + xstate_offsets[index], xstate_sizes[index]); > -} > +/* > + * We previously verified xstate_bv. If we don't have valid > + * comp_offset[] information, something is very broken. > + */ > +BUG_ON(!dest); Are you sure? In compressed format bits in xstate_bv may be clear while xcomp_bv has them set. In such a case there's nowhere to copy to, and hence dest is NULL when coming here. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/6] x86/domctl: Fix migration of guests which are not using xsave
On 12/09/16 13:14, Jan Beulich wrote: On 12.09.16 at 11:51,wrote: >> --- a/xen/arch/x86/domctl.c >> +++ b/xen/arch/x86/domctl.c >> @@ -1158,7 +1158,15 @@ long arch_do_domctl( >> goto vcpuextstate_out; >> } >> >> -if ( evc->size <= PV_XSAVE_SIZE(_xcr0_accum) ) >> +if ( evc->size == PV_XSAVE_HDR_SIZE ) >> +; /* Nothing to restore. */ >> +else if ( evc->size < PV_XSAVE_HDR_SIZE + XSTATE_AREA_MIN_SIZE ) >> +ret = -EINVAL; /* Can't be legitimate data. */ >> +else if ( xsave_area_compressed(_xsave_area) ) >> +ret = -EOPNOTSUPP; /* Don't support compressed data. */ >> +else if ( evc->size != PV_XSAVE_SIZE(_xcr0_accum) ) >> +ret = -EINVAL; /* Not legitimate data. */ > Can't this be moved ahead of the xsave_area_compressed() check, > eliminating (as redundant) the check that's there right now? No. That doesn't catch the case where _xcr0_accum is 0, which will cause the xsave_area_compressed(_xsave_area) check to wander off the end of the buffer. > > In any event the tightening to != you do here supports my desire to > not do any relaxation of the size check in patch 2. The two cases are different, and should not be conflated. > Or alternatively > you would want to consider restoring the behavior from prior to > the xsaves change, where the <= which you now remove was still > okay. I looked back through the history and couldn't find any justification for the check being <= as opposed to being more strict. We absolutely don't want to be in the case where we only restore part of an xstate component. > >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -1345,6 +1345,23 @@ static int hvm_load_cpu_xsave_states(struct domain >> *d, hvm_domain_context_t *h) >> printk(XENLOG_G_WARNING >> "HVM%d.%u restore mismatch: xsave length %#x > %#x\n", >> d->domain_id, vcpuid, desc->length, size); >> +/* Rewind desc->length to ignore the extraneous zeros. */ >> +desc->length = size; > This is slightly ugly, as it will prevent eventually constifying dest (which > it really should have been from the beginning). Hmm yes. I hadn't considered that it was actually mutating the hvm domain context buffer. I will switch to a shadow variable instead. > >> --- a/xen/include/asm-x86/xstate.h >> +++ b/xen/include/asm-x86/xstate.h >> @@ -131,4 +131,10 @@ static inline bool_t xstate_all(const struct vcpu *v) >> (v->arch.xcr0_accum & XSTATE_LAZY & ~XSTATE_FP_SSE); >> } >> >> +static inline bool __nonnull(1) >> +xsave_area_compressed(const struct xsave_struct *xsave_area) > This is certainly odd indentation. It is where my editor naturally indents to. It would appear that emacs is mistaking the __nonnull(1) as the function name, and presuming that the following line is K style parameters. It is certainly awkward that the attributes need to be that way around. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] x86/xstate: Fix latent bugs in expand_xsave_states()
>>> On 12.09.16 at 14:29,wrote: > On 12/09/16 12:41, Jan Beulich wrote: > On 12.09.16 at 11:51, wrote: >>> @@ -205,11 +222,9 @@ void expand_xsave_states(struct vcpu *v, void *dest, >>> unsigned int size) >>> >>> if ( src ) >>> { >>> -ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size); >>> +BUG_ON((xstate_offsets[index] + xstate_sizes[index]) <= size); >> Surely converting an ASSERT() to BUG_ON() means inverting the >> relational operator used? > > Very true. It is unfortunate that all of this is dead code, and > impossible to test. I also had half a mind to explicitly #if 0 it out > to leave people in no illusion that it ever might have been tested. So with this correct, the patch is then Reviewed-by: Jan Beulich ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] x86/xstate: Fix latent bugs in expand_xsave_states()
>>> On 12.09.16 at 14:29,wrote: > On 12/09/16 12:41, Jan Beulich wrote: > On 12.09.16 at 11:51, wrote: >>> @@ -205,11 +222,9 @@ void expand_xsave_states(struct vcpu *v, void *dest, >>> unsigned int size) >>> >>> if ( src ) >>> { >>> -ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size); >>> +BUG_ON((xstate_offsets[index] + xstate_sizes[index]) <= size); >>> memcpy(dest + xstate_offsets[index], src, xstate_sizes[index]); >>> } >>> -else >>> -memset(dest + xstate_offsets[index], 0, xstate_sizes[index]); >> So I have difficulty seeing why this memset() wasn't sufficient: It >> precisely covers for the respective component being in default >> state. > > No it doesn't. The loop skips over all bits which are not set in xstate_bv. Well, yes, I had corrected myself in the following sentence, resulting in me just asking for the commit message to get clarified. > I had (erroneously) come to the conclusion that the "if ( src )" check > only caught the case where we had bad comp_offsets[] information, but > rereading the logic, that case would actually corrupt the legacy SSE header. > > Overall, it turns out that the "if ( src )" is unconditionally taken. Oh, I see (same applies to my then wrong comment on patch 6): We iterate over xstate_bv here, and components with their flag set in xstate_bv won't see NULL coming back from get_xsave_addr(). I'm sorry for the noise then. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable-smoke test] 100891: tolerable all pass - PUSHED
flight 100891 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/100891/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass version targeted for testing: xen d45fae589b8d8b6d36c211dcc46d767dda730b61 baseline version: xen a3fe74e4345e66ddb7aa514395260a5e5f8b0cdc Last test of basis 100862 2016-09-10 00:01:51 Z2 days Failing since100889 2016-09-12 09:01:21 Z0 days2 attempts Testing same since 100891 2016-09-12 11:01:22 Z0 days1 attempts People who touched revisions under test: Andrew CooperJan Beulich Juergen Gross Tim Deegan Wei Liu jobs: build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl 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 : + branch=xen-unstable-smoke + revision=d45fae589b8d8b6d36c211dcc46d767dda730b61 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x '!=' x/home/osstest/repos/lock ']' ++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock ++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push xen-unstable-smoke d45fae589b8d8b6d36c211dcc46d767dda730b61 + branch=xen-unstable-smoke + revision=d45fae589b8d8b6d36c211dcc46d767dda730b61 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']' + . ./cri-common ++ . ./cri-getconfig ++ umask 002 + select_xenbranch + case "$branch" in + tree=xen + xenbranch=xen-unstable-smoke + qemuubranch=qemu-upstream-unstable + '[' xxen = xlinux ']' + linuxbranch= + '[' xqemu-upstream-unstable = x ']' + select_prevxenbranch ++ ./cri-getprevxenbranch xen-unstable-smoke + prevxenbranch=xen-4.7-testing + '[' xd45fae589b8d8b6d36c211dcc46d767dda730b61 = x ']' + : tested/2.6.39.x + . ./ap-common ++ : osst...@xenbits.xen.org +++ getconfig OsstestUpstream +++ perl -e ' use Osstest; readglobalconfig(); print $c{"OsstestUpstream"} or die $!; ' ++ : ++ : git://xenbits.xen.org/xen.git ++ : osst...@xenbits.xen.org:/home/xen/git/xen.git ++ : git://xenbits.xen.org/qemu-xen-traditional.git ++ : git://git.kernel.org ++ : git://git.kernel.org/pub/scm/linux/kernel/git ++ : git ++ : git://xenbits.xen.org/xtf.git ++ : osst...@xenbits.xen.org:/home/xen/git/xtf.git ++ : git://xenbits.xen.org/xtf.git ++ : git://xenbits.xen.org/libvirt.git ++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git ++ : git://xenbits.xen.org/libvirt.git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/rumprun.git ++ : git://git.seabios.org/seabios.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/seabios.git ++ : git://xenbits.xen.org/osstest/seabios.git ++ : https://github.com/tianocore/edk2.git ++ :
[Xen-devel] [ovmf test] 100890: all pass - PUSHED
flight 100890 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/100890/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 5654835bd1381dfad9483b767e55db7caaeec907 baseline version: ovmf e9fec7326ad2f27fe368c830da055d1044b18e95 Last test of basis 100888 2016-09-12 05:44:23 Z0 days Testing same since 100890 2016-09-12 10:44:23 Z0 days1 attempts People who touched revisions under test: Dandan Bijobs: 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 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 : + branch=ovmf + revision=5654835bd1381dfad9483b767e55db7caaeec907 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x '!=' x/home/osstest/repos/lock ']' ++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock ++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push ovmf 5654835bd1381dfad9483b767e55db7caaeec907 + branch=ovmf + revision=5654835bd1381dfad9483b767e55db7caaeec907 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']' + . ./cri-common ++ . ./cri-getconfig ++ umask 002 + select_xenbranch + case "$branch" in + tree=ovmf + xenbranch=xen-unstable + '[' xovmf = xlinux ']' + linuxbranch= + '[' x = x ']' + qemuubranch=qemu-upstream-unstable + select_prevxenbranch ++ ./cri-getprevxenbranch xen-unstable + prevxenbranch=xen-4.7-testing + '[' x5654835bd1381dfad9483b767e55db7caaeec907 = x ']' + : tested/2.6.39.x + . ./ap-common ++ : osst...@xenbits.xen.org +++ getconfig OsstestUpstream +++ perl -e ' use Osstest; readglobalconfig(); print $c{"OsstestUpstream"} or die $!; ' ++ : ++ : git://xenbits.xen.org/xen.git ++ : osst...@xenbits.xen.org:/home/xen/git/xen.git ++ : git://xenbits.xen.org/qemu-xen-traditional.git ++ : git://git.kernel.org ++ : git://git.kernel.org/pub/scm/linux/kernel/git ++ : git ++ : git://xenbits.xen.org/xtf.git ++ : osst...@xenbits.xen.org:/home/xen/git/xtf.git ++ : git://xenbits.xen.org/xtf.git ++ : git://xenbits.xen.org/libvirt.git ++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git ++ : git://xenbits.xen.org/libvirt.git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/rumprun.git ++ : git://git.seabios.org/seabios.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/seabios.git ++ : git://xenbits.xen.org/osstest/seabios.git ++ : https://github.com/tianocore/edk2.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/linux-firmware.git ++ : osst...@xenbits.xen.org:/home/osstest/ext/linux-firmware.git ++ : git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git ++ :
Re: [Xen-devel] [PATCH 2/6] x86/domctl: Fix TOCTOU race with the use of XEN_DOMCTL_getvcpuextstate
>>> On 12.09.16 at 14:02,wrote: > On 12/09/16 12:17, Jan Beulich wrote: > On 12.09.16 at 11:51, wrote: >>> A toolstack must call XEN_DOMCTL_getvcpuextstate twice; first to find the > size >>> of the buffer to use, and a second time to get the actual content. >>> >>> The reported size was based on v->arch.xcr0_accum, but a guest which extends >>> its xcr0_accum between the two hypercalls will cause the toolstack to fail > the >>> evc->size != size check, as the provided buffer is now too small. This >>> causes >>> a hard error during the final phase of migration. >>> >>> Instead, return return a size based on xfeature_mask, which is the maximum >>> size Xen will ever permit. The hypercall must now tolerate a >>> toolstack-provided buffer which is overly large (for the case where a guest >>> isn't using all available xsave states), and should write back how much data >>> was actually written into the buffer. >> To be honest, I'm of two minds here. Part of me thinks this is an >> okay change. However, in particular ... >> >>> --- a/xen/arch/x86/domctl.c >>> +++ b/xen/arch/x86/domctl.c >>> @@ -1054,19 +1054,25 @@ long arch_do_domctl( >>> unsigned int size; >>> >>> ret = 0; >>> -vcpu_pause(v); >>> >>> -size = PV_XSAVE_SIZE(v->arch.xcr0_accum); >>> if ( (!evc->size && !evc->xfeature_mask) || >>> guest_handle_is_null(evc->buffer) ) >>> { >>> +/* >>> + * A query for the size of buffer to use. Must return the >>> + * maximum size we ever might hand back to userspace, > bearing >>> + * in mind that the vcpu might increase its xcr0_accum > between >>> + * this query for size, and the following query for data. >>> + */ >>> evc->xfeature_mask = xfeature_mask; >>> -evc->size = size; >>> -vcpu_unpause(v); >>> +evc->size = PV_XSAVE_SIZE(xfeature_mask); >>> goto vcpuextstate_out; >>> } >>> >>> -if ( evc->size != size || evc->xfeature_mask != xfeature_mask ) >>> +vcpu_pause(v); >>> +size = PV_XSAVE_SIZE(v->arch.xcr0_accum); >>> + >>> +if ( evc->size < size || evc->xfeature_mask != xfeature_mask ) >> ... the relaxation from != to < looks somewhat fragile to me, going >> forward. > > What is fragile about it? It is a very common pattern in general, and > we use it elsewhere. If we get handed too large a buffer, what meaning do you want to assign to the extra bytes? Them being meaningless is just one possible interpretation. >> Did you consider dealing with the issue in the tool stack? > > Yes, and rejected doing so. > >> It can't be that hard to repeat the size query in case data retrieval fails. > > In principle, this is true if the size case was distinguishable from all > the other cases which currently return -EINVAL. It easily is, as long as the caller know what size it used on the most recent earlier call (since it would then notice it having grown). >>> @@ -1103,6 +1109,10 @@ long arch_do_domctl( >>> } >>> >>> vcpu_unpause(v); >>> + >>> +/* Specify how much data we actually wrote into the buffer. */ >>> +if ( !ret ) >>> +evc->size = size; >> ... if this got written on the earlier error path, there wouldn't even >> be a need to retry the size query: Data retrieval could be retried >> with the new size right after enlarging the buffer. > > True, but userspace hypercalls are expensive for the domain in question, > and take a global spinlock in Xen which is expensive for the system as a > whole. Looking forward to PVH control domains, any hypercall will be > far more expensive (a vmexit/entry vs syscall/sysret). > > As such, I am not happy introducing unnecessary hypercalls, as they are > entirely detrimental to dom0 and Xen. Let's be realistic: How often do you expect xcr0_accum to change in practice between the size query and the data retrieval? I'm perfectly fine fixing the theoretical issue, but I don't think there's a performance aspect to be considered when deciding how to deal with it. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] Getting started with Xen Project
On Sat, Sep 10, 2016 at 03:51:33PM +0530, Aashaka Shah wrote: > Hello! I am Aashaka. I like computer architecture and have a good knowledge > of C. While browsing Outreachy projects, I came across the project about > adding PVH support to OVMF binaries. Since it looked interesting, I started > reading the mailing-list link mentioned in the project description and also > the Beginners section in navigation by audience column. > > Currently, I have very little, in fact, no, experience developing with the > Xen Project. I would like to know how to continue with getting familiar > with the terms, setting up the codebase and making contributions. Hello, Anthony (who I've added to the Cc) already started looking into this, so I think you should better coordinate with him regarding this project. Please note that this project will involve quite a lot of low-level coding in OVMF, and some knowledge of x86 assembly is required, again Anthony will be able to provide more insight, and whether this is still a suitable project for OPW or not. Roger. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] x86/xstate: Fix latent bugs in expand_xsave_states()
On 12/09/16 12:41, Jan Beulich wrote: On 12.09.16 at 11:51,wrote: >> @@ -176,6 +187,11 @@ void expand_xsave_states(struct vcpu *v, void *dest, >> unsigned int size) >> u64 xstate_bv = xsave->xsave_hdr.xstate_bv; >> u64 valid; >> >> +/* Check there is state to serialise (i.e. at least an XSAVE_HDR) */ >> +BUG_ON(!v->arch.xcr0_accum); >> +/* Check there is the correct room to decompress into. */ >> +BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum)); > Further down I see you convert an ASSERT() to BUG_ON(), but I > wonder why you do that and why the two above can't be ASSERT() > too. xstate_ctxt_size() is not always cheap. This isn't a fastpath, and the cpuid work will make xstate_ctxt_size() into an O(1) operation. Furthermore, following the investigation of XSA-186, I will not use assertions for bounds checking. The potential damage of omitting the check far outweighs the overhead of the unconditional check. > >> @@ -189,6 +205,7 @@ void expand_xsave_states(struct vcpu *v, void *dest, >> unsigned int size) >> * Copy legacy XSAVE area and XSAVE hdr area. >> */ >> memcpy(dest, xsave, XSTATE_AREA_MIN_SIZE); >> +memset(dest + XSTATE_AREA_MIN_SIZE, 0, size - XSTATE_AREA_MIN_SIZE); >> >> ((struct xsave_struct *)dest)->xsave_hdr.xcomp_bv = 0; >> >> @@ -205,11 +222,9 @@ void expand_xsave_states(struct vcpu *v, void *dest, >> unsigned int size) >> >> if ( src ) >> { >> -ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size); >> +BUG_ON((xstate_offsets[index] + xstate_sizes[index]) <= size); > Surely converting an ASSERT() to BUG_ON() means inverting the > relational operator used? Very true. It is unfortunate that all of this is dead code, and impossible to test. I also had half a mind to explicitly #if 0 it out to leave people in no illusion that it ever might have been tested. > >> memcpy(dest + xstate_offsets[index], src, xstate_sizes[index]); >> } >> -else >> -memset(dest + xstate_offsets[index], 0, xstate_sizes[index]); > So I have difficulty seeing why this memset() wasn't sufficient: It > precisely covers for the respective component being in default > state. No it doesn't. The loop skips over all bits which are not set in xstate_bv. I had (erroneously) come to the conclusion that the "if ( src )" check only caught the case where we had bad comp_offsets[] information, but rereading the logic, that case would actually corrupt the legacy SSE header. Overall, it turns out that the "if ( src )" is unconditionally taken. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [distros-debian-sid test] 67696: tolerable FAIL
flight 67696 distros-debian-sid real [real] http://osstest.xs.citrite.net/~osstest/testlogs/logs/67696/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-amd64-i386-i386-sid-netboot-pvgrub 9 debian-di-install fail like 67637 test-amd64-i386-amd64-sid-netboot-pygrub 9 debian-di-install fail like 67637 test-amd64-amd64-i386-sid-netboot-pygrub 9 debian-di-install fail like 67637 test-armhf-armhf-armhf-sid-netboot-pygrub 9 debian-di-install fail like 67637 test-amd64-amd64-amd64-sid-netboot-pvgrub 9 debian-di-install fail like 67637 baseline version: flight 67637 jobs: build-amd64 pass build-armhf pass build-i386 pass build-amd64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-amd64-sid-netboot-pvgrubfail test-amd64-i386-i386-sid-netboot-pvgrub fail test-amd64-i386-amd64-sid-netboot-pygrub fail test-armhf-armhf-armhf-sid-netboot-pygrubfail test-amd64-amd64-i386-sid-netboot-pygrub fail sg-report-flight on osstest.xs.citrite.net logs: /home/osstest/logs images: /home/osstest/images Logs, config files, etc. are available at http://osstest.xs.citrite.net/~osstest/testlogs/logs Test harness code can be found at http://xenbits.xensource.com/gitweb?p=osstest.git;a=summary Push not applicable. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/6] x86/domctl: Fix migration of guests which are not using xsave
>>> On 12.09.16 at 11:51,wrote: > --- a/xen/arch/x86/domctl.c > +++ b/xen/arch/x86/domctl.c > @@ -1158,7 +1158,15 @@ long arch_do_domctl( > goto vcpuextstate_out; > } > > -if ( evc->size <= PV_XSAVE_SIZE(_xcr0_accum) ) > +if ( evc->size == PV_XSAVE_HDR_SIZE ) > +; /* Nothing to restore. */ > +else if ( evc->size < PV_XSAVE_HDR_SIZE + XSTATE_AREA_MIN_SIZE ) > +ret = -EINVAL; /* Can't be legitimate data. */ > +else if ( xsave_area_compressed(_xsave_area) ) > +ret = -EOPNOTSUPP; /* Don't support compressed data. */ > +else if ( evc->size != PV_XSAVE_SIZE(_xcr0_accum) ) > +ret = -EINVAL; /* Not legitimate data. */ Can't this be moved ahead of the xsave_area_compressed() check, eliminating (as redundant) the check that's there right now? In any event the tightening to != you do here supports my desire to not do any relaxation of the size check in patch 2. Or alternatively you would want to consider restoring the behavior from prior to the xsaves change, where the <= which you now remove was still okay. > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -1345,6 +1345,23 @@ static int hvm_load_cpu_xsave_states(struct domain *d, > hvm_domain_context_t *h) > printk(XENLOG_G_WARNING > "HVM%d.%u restore mismatch: xsave length %#x > %#x\n", > d->domain_id, vcpuid, desc->length, size); > +/* Rewind desc->length to ignore the extraneous zeros. */ > +desc->length = size; This is slightly ugly, as it will prevent eventually constifying dest (which it really should have been from the beginning). > --- a/xen/include/asm-x86/xstate.h > +++ b/xen/include/asm-x86/xstate.h > @@ -131,4 +131,10 @@ static inline bool_t xstate_all(const struct vcpu *v) > (v->arch.xcr0_accum & XSTATE_LAZY & ~XSTATE_FP_SSE); > } > > +static inline bool __nonnull(1) > +xsave_area_compressed(const struct xsave_struct *xsave_area) This is certainly odd indentation. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 29/38] arm/p2m: Add HVMOP_altp2m_set_mem_access
Hi Sergej, On 16/08/16 23:17, Sergej Proskurin wrote: The HVMOP_altp2m_set_mem_access allows to set gfn permissions of (currently one page at a time) of a specific altp2m view. In case the view does not hold the requested gfn entry, it will be first copied from the host's p2m table and then modified as requested. Signed-off-by: Sergej Proskurin--- Cc: Stefano Stabellini Cc: Julien Grall --- v2: Prevent the page reference count from being falsely updated on altp2m modification. Therefore, we add a check determining whether the target p2m is a hostp2m before p2m_put_l3_page is called. v3: Cosmetic fixes. Added the functionality to set/get the default_access also in/from the requested altp2m view. Read-locked hp2m in "altp2m_set_mem_access". Moved the functions "p2m_is_(hostp2m|altp2m)" out of this commit. Moved the funtion "modify_altp2m_entry" out of this commit. Moved the function "p2m_lookup_attr" out of this commit. Moved guards for "p2m_put_l3_page" out of this commit. --- xen/arch/arm/altp2m.c| 53 xen/arch/arm/hvm.c | 7 +++- xen/arch/arm/p2m.c | 82 xen/include/asm-arm/altp2m.h | 12 +++ 4 files changed, 131 insertions(+), 23 deletions(-) diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c index ba345b9..03b8ce5 100644 --- a/xen/arch/arm/altp2m.c +++ b/xen/arch/arm/altp2m.c @@ -80,6 +80,59 @@ int altp2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx) return rc; } +int altp2m_set_mem_access(struct domain *d, + struct p2m_domain *hp2m, + struct p2m_domain *ap2m, + p2m_access_t a, + gfn_t gfn) +{ +p2m_type_t p2mt; +p2m_access_t old_a; +mfn_t mfn; +unsigned int page_order; +int rc; + +altp2m_lock(d); Why do you have this lock? This will serialize all the mem access modification even if the ap2m is different. I am also surprised that you only lock the altp2m in modify_altp2m_entry. IHMO, the change in the same a2pm should be serialized. +p2m_read_lock(hp2m); + +/* Check if entry is part of the altp2m view. */ +mfn = p2m_lookup_attr(ap2m, gfn, , NULL, _order); + +/* Check host p2m if no valid entry in ap2m. */ +if ( mfn_eq(mfn, INVALID_MFN) ) +{ +/* Check if entry is part of the host p2m view. */ +mfn = p2m_lookup_attr(hp2m, gfn, , _a, _order); As mentioned in patch #27, p2m_lookup_attr will take a read reference on the hp2m lock. However you already did it above. Anyway, I really think that p2m_lookup_attr should not exist and ever caller be replaced by a direct call to p2m_get_entry. +if ( mfn_eq(mfn, INVALID_MFN) || + ((p2mt != p2m_ram_rw) && (p2mt != p2m_ram_ro)) ) Please use p2m_is_ram rather than open-coding the check on p2mt. However, I don't understand why access restriction on altp2m is limited to RAM whilst the hostp2m allows restriction on any type of page. +{ +rc = -ESRCH; +goto out; +} + +/* If this is a superpage, copy that first. */ +if ( page_order != THIRD_ORDER ) +{ +rc = modify_altp2m_entry(ap2m, gfn, mfn, p2mt, old_a, page_order); +if ( rc < 0 ) +{ +rc = -ESRCH; +goto out; +} +} +} + +/* Set mem access attributes - currently supporting only one (4K) page. */ +page_order = THIRD_ORDER; This looks pointless, please use THIRD_ORDER directly as argument. +rc = modify_altp2m_entry(ap2m, gfn, mfn, p2mt, a, page_order); + +out: +p2m_read_unlock(hp2m); +altp2m_unlock(d); + +return rc; +} + static void altp2m_vcpu_reset(struct vcpu *v) { struct altp2mvcpu *av = _vcpu(v); diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c index 9ac3422..df78893 100644 --- a/xen/arch/arm/hvm.c +++ b/xen/arch/arm/hvm.c @@ -136,7 +136,12 @@ static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) break; case HVMOP_altp2m_set_mem_access: -rc = -EOPNOTSUPP; +if ( a.u.set_mem_access.pad ) +rc = -EINVAL; +else +rc = p2m_set_mem_access(d, _gfn(a.u.set_mem_access.gfn), 1, 0, 0, +a.u.set_mem_access.hvmmem_access, +a.u.set_mem_access.view); break; case HVMOP_altp2m_change_gfn: diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index df2b85b..8dee02187 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1913,7 +1913,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, uint32_t start, uint32_t mask, xenmem_access_t access, unsigned int altp2m_idx)
Re: [Xen-devel] [PATCH 2/6] x86/domctl: Fix TOCTOU race with the use of XEN_DOMCTL_getvcpuextstate
On 12/09/16 12:17, Jan Beulich wrote: On 12.09.16 at 11:51,wrote: >> A toolstack must call XEN_DOMCTL_getvcpuextstate twice; first to find the >> size >> of the buffer to use, and a second time to get the actual content. >> >> The reported size was based on v->arch.xcr0_accum, but a guest which extends >> its xcr0_accum between the two hypercalls will cause the toolstack to fail >> the >> evc->size != size check, as the provided buffer is now too small. This >> causes >> a hard error during the final phase of migration. >> >> Instead, return return a size based on xfeature_mask, which is the maximum >> size Xen will ever permit. The hypercall must now tolerate a >> toolstack-provided buffer which is overly large (for the case where a guest >> isn't using all available xsave states), and should write back how much data >> was actually written into the buffer. > To be honest, I'm of two minds here. Part of me thinks this is an > okay change. However, in particular ... > >> --- a/xen/arch/x86/domctl.c >> +++ b/xen/arch/x86/domctl.c >> @@ -1054,19 +1054,25 @@ long arch_do_domctl( >> unsigned int size; >> >> ret = 0; >> -vcpu_pause(v); >> >> -size = PV_XSAVE_SIZE(v->arch.xcr0_accum); >> if ( (!evc->size && !evc->xfeature_mask) || >> guest_handle_is_null(evc->buffer) ) >> { >> +/* >> + * A query for the size of buffer to use. Must return the >> + * maximum size we ever might hand back to userspace, >> bearing >> + * in mind that the vcpu might increase its xcr0_accum >> between >> + * this query for size, and the following query for data. >> + */ >> evc->xfeature_mask = xfeature_mask; >> -evc->size = size; >> -vcpu_unpause(v); >> +evc->size = PV_XSAVE_SIZE(xfeature_mask); >> goto vcpuextstate_out; >> } >> >> -if ( evc->size != size || evc->xfeature_mask != xfeature_mask ) >> +vcpu_pause(v); >> +size = PV_XSAVE_SIZE(v->arch.xcr0_accum); >> + >> +if ( evc->size < size || evc->xfeature_mask != xfeature_mask ) > ... the relaxation from != to < looks somewhat fragile to me, going > forward. What is fragile about it? It is a very common pattern in general, and we use it elsewhere. > Did you consider dealing with the issue in the tool stack? Yes, and rejected doing so. > It can't be that hard to repeat the size query in case data retrieval fails. In principle, this is true if the size case was distinguishable from all the other cases which currently return -EINVAL. > Such retry logic would be well bounded in terms of iterations it can > potentially take. In fact ... > >> @@ -1103,6 +1109,10 @@ long arch_do_domctl( >> } >> >> vcpu_unpause(v); >> + >> +/* Specify how much data we actually wrote into the buffer. */ >> +if ( !ret ) >> +evc->size = size; > ... if this got written on the earlier error path, there wouldn't even > be a need to retry the size query: Data retrieval could be retried > with the new size right after enlarging the buffer. True, but userspace hypercalls are expensive for the domain in question, and take a global spinlock in Xen which is expensive for the system as a whole. Looking forward to PVH control domains, any hypercall will be far more expensive (a vmexit/entry vs syscall/sysret). As such, I am not happy introducing unnecessary hypercalls, as they are entirely detrimental to dom0 and Xen. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH net-next] xen-netfront: avoid packet loss when ethernet header crosses page boundary
David Vrabelwrites: > On 22/08/16 16:42, Vitaly Kuznetsov wrote: >> Small packet loss is reported on complex multi host network configurations >> including tunnels, NAT, ... My investigation led me to the following check >> in netback which drops packets: >> >> if (unlikely(txreq.size < ETH_HLEN)) { >> netdev_err(queue->vif->dev, >>"Bad packet size: %d\n", txreq.size); >> xenvif_tx_err(queue, , extra_count, idx); >> break; >> } >> >> But this check itself is legitimate. SKBs consist of a linear part (which >> has to have the ethernet header) and (optionally) a number of frags. >> Netfront transmits the head of the linear part up to the page boundary >> as the first request and all the rest becomes frags so when we're >> reconstructing the SKB in netback we can't distinguish between original >> frags and the 'tail' of the linear part. The first SKB needs to be at >> least ETH_HLEN size. So in case we have an SKB with its linear part >> starting too close to the page boundary the packet is lost. >> >> I see two ways to fix the issue: >> - Change the 'wire' protocol between netfront and netback to start keeping >> the original SKB structure. We'll have to add a flag indicating the fact >> that the particular request is a part of the original linear part and not >> a frag. We'll need to know the length of the linear part to pre-allocate >> memory. >> - Avoid transmitting SKBs with linear parts starting too close to the page >> boundary. That seems preferable short-term and shouldn't bring >> significant performance degradation as such packets are rare. That's what >> this patch is trying to achieve with skb_copy(). >> >> Signed-off-by: Vitaly Kuznetsov > > We should probably fix the backend to handle this (by grant copying a > minimum amount in the linear area, but since netfront needs to work with > older netback. > > Acked-by: David Vrabel David, could you please pick this up for net-next or are there any concerns remaining? Thanks, -- Vitaly ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] x86/xstate: Fix latent bugs in expand_xsave_states()
>>> On 12.09.16 at 11:51,wrote: > @@ -176,6 +187,11 @@ void expand_xsave_states(struct vcpu *v, void *dest, > unsigned int size) > u64 xstate_bv = xsave->xsave_hdr.xstate_bv; > u64 valid; > > +/* Check there is state to serialise (i.e. at least an XSAVE_HDR) */ > +BUG_ON(!v->arch.xcr0_accum); > +/* Check there is the correct room to decompress into. */ > +BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum)); Further down I see you convert an ASSERT() to BUG_ON(), but I wonder why you do that and why the two above can't be ASSERT() too. xstate_ctxt_size() is not always cheap. > @@ -189,6 +205,7 @@ void expand_xsave_states(struct vcpu *v, void *dest, > unsigned int size) > * Copy legacy XSAVE area and XSAVE hdr area. > */ > memcpy(dest, xsave, XSTATE_AREA_MIN_SIZE); > +memset(dest + XSTATE_AREA_MIN_SIZE, 0, size - XSTATE_AREA_MIN_SIZE); > > ((struct xsave_struct *)dest)->xsave_hdr.xcomp_bv = 0; > > @@ -205,11 +222,9 @@ void expand_xsave_states(struct vcpu *v, void *dest, > unsigned int size) > > if ( src ) > { > -ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size); > +BUG_ON((xstate_offsets[index] + xstate_sizes[index]) <= size); Surely converting an ASSERT() to BUG_ON() means inverting the relational operator used? > memcpy(dest + xstate_offsets[index], src, xstate_sizes[index]); > } > -else > -memset(dest + xstate_offsets[index], 0, xstate_sizes[index]); So I have difficulty seeing why this memset() wasn't sufficient: It precisely covers for the respective component being in default state. Or wait - this was fine if intermediate bits were clear in xstate_bv, but not if clear-but-valid ones weren't followed by another set one. Nor would gaps between components have been taken care of. I think the commit message could be made more explicit in this regard (of course unless I'm overlooking yet another aspect). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel