[OSSTEST PATCH v2 00/12] XSM test cases for OSSTest

[OSSTEST PATCH v2 00/12] XSM test cases for OSSTest

[Wei Liu]

Hi all

This patch series attempts to duplicate some Debian test cases for XSM. This
is version 2.

Tests duplicated for xen-unstable branch:
  build-{i386,amd64,armhf}-xsm
  test-amd64-{i386,amd64}-xl-xsm
  test-armhf-armhf-xl-xsm
  test-amd64-{i386,amd64}-xl-qemuu-debianhvm-amd64-xsm
  test-amd64-(i386,amd64}-xl-qemut-debianhvm-amd64-xsm

Those loose ends mentioned in RFC series have more or less been resolved, hence
I drop RFC tag.

The main issue at the point is that default policy is too strict. With help
from Daniel we should be able to resolve that in a timely manner. In any case
this issue should not be a blocker of accepting this test case.

Changes in this version:
1. address IanC's comments regarding coding style.
2. fix a bug in previous version that duplicates HVM test cases for pre-4.5
   branches.

Wei.

Wei Liu (12):
  README: list chiark-utils-bin as requirement
  gitignore: ignore images directory
  ts-xen-build-prep: install checkpolicy
  ts-xen-build: build with XSM support if requested
  mfi-common: create build-$arch-xsm job
  Debian.pm: pass in XSM configuration to bootloader setup routines
  Debian.pm: load flask policy in uboot
  ts-xen-install: install Xen with XSM support if requested
  mfi-common: use XSM build if job name contains -xsm suffix
  make-flight: create XSM test jobs
  ts-debian-install: add in seclabel if XSM is enabled
  ts-debian-hvm-install: add in seclabel if XSM is enabled

 .gitignore            |    1 +
 Osstest/Debian.pm     |   50 ++++++++++++++++-----
 README                |    1 +
 make-flight           |   54 +++++++++++++++++++----
 mfi-common            |   32 +++++++++++++-
 ts-debian-hvm-install |   14 ++++--
 ts-debian-install     |   11 ++++-
 ts-xen-build          |    2 +
 ts-xen-build-prep     |    2 +-
 ts-xen-install        |  115 ++++++++++++++++++++++++++++++++++++++++++++++++-
 10 files changed, 254 insertions(+), 28 deletions(-)

-- 
1.7.10.4

[OSSTEST PATCH v2 01/12] README: list chiark-utils-bin as requirement

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 README |    1 +
 1 file changed, 1 insertion(+)

diff --git a/README b/README
index 9a85549..ee1cd7a 100644
--- a/README
+++ b/README
@@ -204,6 +204,7 @@ To run osstest in standalone mode:
      pax rsync
      curl
      netcat
+     chiark-utils-bin
 
  - Optional: ipmitool
 
-- 
1.7.10.4

[OSSTEST PATCH v2 02/12] gitignore: ignore images directory

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 .gitignore |    1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 4f2a839..8dad470 100644
--- a/.gitignore
+++ b/.gitignore
@@ -27,3 +27,4 @@ logs
 id_rsa_osstest
 id_rsa_osstest.pub
 overlay-local
+images
-- 
1.7.10.4

[OSSTEST PATCH v2 03/12] ts-xen-build-prep: install checkpolicy

[Wei Liu]

This is used to complie Flask policy.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 ts-xen-build-prep |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ts-xen-build-prep b/ts-xen-build-prep
index 05a7857..edef9a9 100755
--- a/ts-xen-build-prep
+++ b/ts-xen-build-prep
@@ -178,7 +178,7 @@ sub prep () {
                                autoconf automake libtool xsltproc
                                libxml2-utils libxml2-dev libnl-dev
                                libdevmapper-dev w3c-dtd-xhtml
-			       ccache));
+			       ccache checkpolicy));
 
     target_cmd_root($ho, "chmod -R a+r /usr/share/git-core/templates");
     # workaround for Debian #595728
-- 
1.7.10.4

[OSSTEST PATCH v2 04/12] ts-xen-build: build with XSM support if requested

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 ts-xen-build |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/ts-xen-build b/ts-xen-build
index 661f186..e0f0844 100755
--- a/ts-xen-build
+++ b/ts-xen-build
@@ -34,6 +34,7 @@ sub checkout () {
     build_clone($ho, 'xen', $builddir, 'xen');
 
     my $debug_build = $r{xen_build_debug} || 'y';
+    my $build_xsm = $r{enable_xsm} || 'n';
 
     # Do not set this unless you know what you are doing. This arm
     # option makes the build specific to a particular type of
@@ -47,6 +48,7 @@ sub checkout () {
         cd $builddir/xen
 	>.config
 	echo >>.config debug=$debug_build
+	echo >>.config XSM_ENABLE=$build_xsm
 	echo >>.config GIT_HTTP=y
 	echo >>.config LIBLEAFDIR_x86_64=lib
 	echo >>.config QEMU_REMOTE='$r{tree_qemu}'
-- 
1.7.10.4

[OSSTEST PATCH v2 05/12] mfi-common: create build-$arch-xsm job

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 mfi-common |   24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/mfi-common b/mfi-common
index afb6239..c3e1909 100644
--- a/mfi-common
+++ b/mfi-common
@@ -49,6 +49,7 @@ create_build_jobs () {
   local want_xend build_defxend build_extraxend
   local enable_ovmf
   local build_hostflags
+  local test_xsm
 
   for arch in ${BUILD_ARCHES- i386 amd64 armhf }; do
 
@@ -134,14 +135,34 @@ create_build_jobs () {
     *) enable_ovmf=true;
     esac
 
+    # Test XSM from 4.5 onwards
+    case "$xenbranch" in
+    xen-3.*-testing) test_xsm="n";;
+    xen-4.0-testing) test_xsm="n";;
+    xen-4.1-testing) test_xsm="n";;
+    xen-4.2-testing) test_xsm="n";;
+    xen-4.3-testing) test_xsm="n";;
+    xen-4.4-testing) test_xsm="n";;
+    *) test_xsm="n y";
+    esac
+
     eval "
         arch_runvars=\"\$ARCH_RUNVARS_$arch\"
     "
 
     build_hostflags=share-build-$suite-$arch,arch-$arch,suite-$suite,purpose-build
 
-    ./cs-job-create $flight build-$arch build                                \
+    for xsm in $test_xsm; do
+      if [ x$xsm = xy ] ; then
+        xsm_suffix="-xsm"
+        enable_xsm=y
+      else
+        xsm_suffix=""
+        enable_xsm=n
+      fi
+      ./cs-job-create $flight build-$arch$xsm_suffix build                   \
                 arch=$arch enable_xend=$build_defxend enable_ovmf=$enable_ovmf\
+                enable_xsm=$enable_xsm                                       \
         tree_qemu=$TREE_QEMU                                                 \
         tree_qemuu=$TREE_QEMU_UPSTREAM                                       \
         tree_xen=$TREE_XEN                                                   \
@@ -153,6 +174,7 @@ create_build_jobs () {
                 revision_qemu=$REVISION_QEMU                                 \
                 revision_qemuu=$REVISION_QEMU_UPSTREAM                       \
                 revision_seabios=$REVISION_SEABIOS
+    done
 
     if [ $build_extraxend = "true" ] ; then
     ./cs-job-create $flight build-$arch-xend build                           \
-- 
1.7.10.4

[OSSTEST PATCH v2 06/12] Debian.pm: pass in XSM configuration to bootloader setup routines

[Wei Liu]

Change to Uboot will come in another patch. GRUB 1 is ignored, as
currently OSSTest only has Wheezy which has GRUB 2.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 Osstest/Debian.pm |   32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
index ab09abb..4092351 100644
--- a/Osstest/Debian.pm
+++ b/Osstest/Debian.pm
@@ -45,9 +45,9 @@ BEGIN {
 
 #---------- manipulation of Debian bootloader setup ----------
 
-sub debian_boot_setup ($$$$;$) {
+sub debian_boot_setup ($$$$$;$) {
     # $xenhopt==undef => is actually a guest, do not set up a hypervisor
-    my ($ho, $want_kernver, $xenhopt, $distpath, $hooks) = @_;
+    my ($ho, $want_kernver, $want_xsm, $xenhopt, $distpath, $hooks) = @_;
 
     target_kernkind_check($ho);
     target_kernkind_console_inittab($ho,$ho,"/");
@@ -72,11 +72,11 @@ sub debian_boot_setup ($$$$;$) {
 
     my $bootloader;
     if ( $ho->{Flags}{'need-uboot-bootscr'} ) {
-	$bootloader= setupboot_uboot($ho, $want_kernver, $xenhopt, $kopt);
+	$bootloader= setupboot_uboot($ho, $want_kernver, $want_xsm, $xenhopt, $kopt);
     } elsif ($ho->{Suite} =~ m/lenny/) {
-        $bootloader= setupboot_grub1($ho, $want_kernver, $xenhopt, $kopt);
+        $bootloader= setupboot_grub1($ho, $want_kernver, $want_xsm, $xenhopt, $kopt);
     } else {
-        $bootloader= setupboot_grub2($ho, $want_kernver, $xenhopt, $kopt);
+        $bootloader= setupboot_grub2($ho, $want_kernver, $want_xsm, $xenhopt, $kopt);
     }
 
     $bootloader->{UpdateConfig}($ho);
@@ -112,8 +112,8 @@ sub bl_getmenu_open ($$$) {
     return $f;
 }
 
-sub setupboot_uboot ($$$) {
-    my ($ho,$want_kernver,$xenhopt,$xenkopt) = @_;
+sub setupboot_uboot ($$$$) {
+    my ($ho,$want_kernver,$want_xsm,$xenhopt,$xenkopt) = @_;
     my $bl= { };
 
     $bl->{UpdateConfig}= sub {
@@ -194,13 +194,17 @@ END
     return $bl;
 }
 
-sub setupboot_grub1 ($$$) {
-    my ($ho,$want_kernver,$xenhopt,$xenkopt) = @_;
+sub setupboot_grub1 ($$$$) {
+    my ($ho,$want_kernver,$want_xsm,$xenhopt,$xenkopt) = @_;
     my $bl= { };
 
     my $rmenu= "/boot/grub/menu.lst";
     my $lmenu= "$stash/$ho->{Name}--menu.lst.out";
 
+    if ($want_xsm) {
+	die "Enabling XSM with GRUB is not supported";
+    }
+
     target_editfile_root($ho, $rmenu, sub {
         while (<::EI>) {
             if (m/^## ## Start Default/ ..
@@ -274,8 +278,8 @@ sub setupboot_grub1 ($$$) {
     return $bl;
 }
 
-sub setupboot_grub2 ($$$) {
-    my ($ho,$want_kernver,$xenhopt,$xenkopt) = @_;
+sub setupboot_grub2 ($$$$) {
+    my ($ho,$want_kernver,$want_xsm,$xenhopt,$xenkopt) = @_;
     my $bl= { };
 
     my $rmenu= '/boot/grub/grub.cfg';
@@ -302,6 +306,9 @@ sub setupboot_grub2 ($$$) {
 			 $entry->{KernVer} ne $want_kernver) {
 		    logm("(skipping entry at $entry->{StartLine};".
 			 " kernel $entry->{KernVer}, not $want_kernver)");
+		} elsif ($want_xsm && !defined $entry->{Xenpolicy}) {
+		    logm("(skipping entry at $entry->{StartLine};".
+			 " XSM policy file not present)");
 		} else {
 		    # yes!
 		    last;
@@ -334,6 +341,9 @@ sub setupboot_grub2 ($$$) {
             if (m/^\s*module\s*\/(initrd\S+)/) {
                 $entry->{Initrd}= $1;
             }
+	    if (m/^\s*module\s*\/(xenpolicy\S+)/) {
+                $entry->{Xenpolicy}= $1;
+            }
         }
         die 'grub 2 bootloader entry not found' unless $entry;
 
-- 
1.7.10.4

[OSSTEST PATCH v2 07/12] Debian.pm: load flask policy in uboot

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/Debian.pm |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
index 4092351..5aaab83 100644
--- a/Osstest/Debian.pm
+++ b/Osstest/Debian.pm
@@ -123,6 +123,22 @@ sub setupboot_uboot ($$$$) {
 	my $kern = "vmlinuz-$want_kernver";
 	my $initrd = "initrd.img-$want_kernver";
 
+	my $flask_commands = "";
+	if ($want_xsm) {
+	    my $flaskpolicy = $r{flaskpoilcy};
+	    $flask_commands = <<END;
+
+setenv flask_policy_addr_r 0x1200000
+flaskpolicy=`readlink /boot/$flaskpolicy`
+ext2load scsi 0 \\\${flask_policy_addr_r} \$flaskpolicy
+fdt mknod /chosen module\@2
+fdt set /chosen/module\@2 compatible "xen,xsm-policy"
+fdt set /chosen/module\@2 reg <\\\${flask_policy_addr_r} \\\${filesize}>
+echo Loaded $flaskpolicy to \\\${flask_policy_addr_r} (\\\${filesize})
+
+END
+	}
+
 	my $root= target_guest_lv_name($ho,"root");
 
 	logm("Xen options: $xenhopt");
@@ -176,6 +192,8 @@ fdt set /chosen/module\@1 compatible "xen,linux-initrd" "xen,multiboot-module"
 fdt set /chosen/module\@1 reg <\\\${ramdisk_addr_r} \\\${filesize}>
 echo Loaded $initrd to \\\${ramdisk_addr_r} (\\\${filesize})
 
+${flask_commands}
+
 fdt print /chosen
 
 echo Booting \\\${xen_addr_r} - \\\${fdt_addr}
-- 
1.7.10.4

[OSSTEST PATCH v2 08/12] ts-xen-install: install Xen with XSM support if requested

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 ts-xen-install |  115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 114 insertions(+), 1 deletion(-)

diff --git a/ts-xen-install b/ts-xen-install
index 4d34d1f..3bc4d2e 100755
--- a/ts-xen-install
+++ b/ts-xen-install
@@ -46,6 +46,8 @@ if (@ARGV and $ARGV[0] eq '--check') {
 
 our $ho;
 
+my $enable_xsm = $r{enable_xsm} =~ m/y/ ? 1 : 0;
+
 my %distpath;
 
 sub packages () {
@@ -73,6 +75,15 @@ sub extract () {
 				   $r{"${part}buildjob"}, \%distpath);
     }
     target_cmd_root($ho, '/sbin/ldconfig');
+    if ($enable_xsm) {
+        my $flaskpolicy = target_cmd_output_root($ho,
+            'find /boot -name \'xenpolicy-*\' -exec basename {} \;');
+	# there should only be one xenpolicy file for a clean install
+	my $c = () = $flaskpolicy =~ /xenpolicy/g;
+	die "Too many XSM policy files $c" if $c > 1;
+	die "XSM policy file is required" if $c == 0;
+	store_runvar("flaskpolicy", $flaskpolicy);
+    }
 }
 
 sub adjustconfig () {
@@ -133,6 +144,86 @@ sub adjustconfig () {
     setup_cxfabric($ho);
 }
 
+
+sub grub_patch () {
+    return << 'END';
+--- /etc/grub.d/20_linux_xen.orig	2014-09-22 11:39:09.120630051 +0100
++++ /etc/grub.d/20_linux_xen	2014-09-22 11:43:07.069802099 +0100
+@@ -63,10 +63,27 @@
+   recovery="$4"
+   args="$5"
+   xen_args="$6"
+-  if ${recovery} ; then
+-    title="$(gettext_quoted "%s, with Linux %s and XEN %s (recovery mode)")"
++  xsm="$7"
++  # If user want to enable XSM support, make sure there's corresponding
++  # policy file.
++  if ${xsm} ; then
++      xenpolicy=`echo xenpolicy-$xen_version`
++      if test ! -e "${xen_dirname}/${xenpolicy}" ; then
++          return
++      fi
++      xen_args=`echo $xen_args flask_enabled=1 flask_enforcing=1`
++      if ${recovery} ; then
++          title="$(gettext_quoted "%s, with Xen %s (XSM enabled) and Linux %s (recovery mode)")"
++      else
++          title="$(gettext_quoted "%s, with Xen %s (XSM enabled) and Linux %s")"
++      fi
+   else
+-    title="$(gettext_quoted "%s, with Linux %s and XEN %s")"
++      xenpolicy=""
++      if ${recovery} ; then
++	  title="$(gettext_quoted "%s, with Linux %s and XEN %s (recovery mode)")"
++      else
++	  title="$(gettext_quoted "%s, with Linux %s and XEN %s")"
++      fi
+   fi
+   printf "menuentry '${title}' ${CLASS} {\n" "${os}" "${version}" "${xen_version}"
+   save_default_entry | sed -e "s/^/\t/"
+@@ -88,6 +105,13 @@
+ 	module	${rel_dirname}/${initrd}
+ EOF
+   fi
++  if test -n "${xenpolicy}" ; then
++    message="$(gettext_printf "Loading XSM policy ...")"
++    cat << EOF
++	echo	'$message'
++	module	${rel_dirname}/${xenpolicy}
++EOF
++  fi
+   cat << EOF
+ }
+ EOF
+@@ -98,7 +122,7 @@
+ 	version=$(echo $basename | sed -e "s,^[^0-9]*-,,g")
+         if grub_file_is_not_garbage "$i" && grep -qx 'CONFIG_XEN_\(DOM0\|PRIVILEGED_GUEST\)=y' /boot/config-${version} 2> /dev/null ; then echo -n "$i " ; fi
+       done`
+-xen_list=`for i in /boot/xen*; do
++xen_list=`for i in /boot/xen[-.]*; do
+         if grub_file_is_not_garbage "$i" ; then echo -n "$i " ; fi
+       done`
+ prepare_boot_cache=
+@@ -137,10 +161,14 @@
+ 	fi
+ 
+ 	linux_entry "${OS}" "${version}" "${xen_version}" false \
+-	    "${GRUB_CMDLINE_LINUX} ${GRUB_CMDLINE_LINUX_DEFAULT}" "${GRUB_CMDLINE_XEN} ${GRUB_CMDLINE_XEN_DEFAULT}"
++	    "${GRUB_CMDLINE_LINUX} ${GRUB_CMDLINE_LINUX_DEFAULT}" "${GRUB_CMDLINE_XEN} ${GRUB_CMDLINE_XEN_DEFAULT}" false
++	linux_entry "${OS}" "${version}" "${xen_version}" false \
++	    "${GRUB_CMDLINE_LINUX} ${GRUB_CMDLINE_LINUX_DEFAULT}" "${GRUB_CMDLINE_XEN} ${GRUB_CMDLINE_XEN_DEFAULT}" true
+ 	if [ "x${GRUB_DISABLE_LINUX_RECOVERY}" != "xtrue" ]; then
+ 	    linux_entry "${OS}" "${version}" "${xen_version}" true \
+-		"single ${GRUB_CMDLINE_LINUX}" "${GRUB_CMDLINE_XEN}"
++		"single ${GRUB_CMDLINE_LINUX}" "${GRUB_CMDLINE_XEN}" false
++	    linux_entry "${OS}" "${version}" "${xen_version}" true \
++		"single ${GRUB_CMDLINE_LINUX}" "${GRUB_CMDLINE_XEN}" true
+ 	fi
+ 
+ 	list=`echo $list | tr ' ' '\n' | grep -vx $linux | tr '\n' ' '`
+
+END
+}
+
 sub setupboot () {
     my $xenhopt= "conswitch=x watchdog";
 
@@ -170,8 +261,30 @@ sub setupboot () {
         };
     }
 
+    if ($enable_xsm) {
+	die if !defined($r{flaskpolicy});
+	target_putfilecontents_root_stash($ho, 10, grub_patch(), "grub.patch");
+	target_cmd_root($ho, << 'END');
+if test ! -e /etc/grub.d/20_linux_xen ; then
+  case `uname -m` in
+    x86*) echo '/etc/grub.d/20_linux_xen doesn't exist, abort'
+          exit 1 ;;
+    arm*) echo '/etc/grub.d/20_linux_xen doesn't exist on ARM, not patching'
+          exit 0 ;;
+    *)    echo 'unknown architecture, abort'
+          exit 1;;
+  esac
+elif ! grep -q -- xenpolicy /etc/grub.d/20_linux_xen ; then
+  patch -p0 /etc/grub.d/20_linux_xen  < grub.patch
+else
+  echo 'Grub script already supports XSM, not patching'
+  exit 1
+fi
+END
+    }
+
     my $want_kernver = get_runvar('kernel_ver',$r{'kernbuildjob'});
-    debian_boot_setup($ho, $want_kernver, $xenhopt, \%distpath, \@hooks);
+    debian_boot_setup($ho, $want_kernver, $enable_xsm, $xenhopt, \%distpath, \@hooks);
 
     logm("ready to boot Xen");
 }
-- 
1.7.10.4

[OSSTEST PATCH v2 09/12] mfi-common: use XSM build if job name contains -xsm suffix

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 mfi-common |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/mfi-common b/mfi-common
index c3e1909..8db599f 100644
--- a/mfi-common
+++ b/mfi-common
@@ -274,6 +274,14 @@ job_create_test () {
   buildjob="${bfi}build-$dom0arch"
   tsbuildjob=
 
+  # Use xsm build job if job contains -xsm suffix
+  case "$job" in
+    *-xsm) xenbuildjob="$xenbuildjob-xsm"
+           buildjob="$buildjob-xsm"
+           ;;
+    *) ;;
+  esac
+
   case "$xenbranch:$toolstack" in
     xen-3.*-testing:*) ;;
     xen-4.0-testing:*) ;;
-- 
1.7.10.4

[OSSTEST PATCH v2 10/12] make-flight: create XSM test jobs

[Wei Liu]

Duplicate Debian PV and HVM test jobs for XSM testing.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 make-flight |   54 ++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 46 insertions(+), 8 deletions(-)

diff --git a/make-flight b/make-flight
index 9963a46..0b04ed4 100755
--- a/make-flight
+++ b/make-flight
@@ -200,27 +200,41 @@ do_hvm_win7_x64_tests () {
 do_hvm_debian_test_one () {
   testname=$1
   bios=$2
-  job_create_test test-$xenarch$kern-$dom0arch-xl$qemuu_suffix-$testname-amd64\
+  xsm=$3
+
+  xsm_suffix=""
+  if [ x$xsm = xy ] ; then
+      xsm_suffix="-xsm"
+  fi
+
+  job_create_test test-$xenarch$kern-$dom0arch-xl$qemuu_suffix-$testname-amd64$xsm_suffix\
     test-debianhvm xl $xenarch $dom0arch $qemuu_runvar \
+    enable_xsm=$xsm                             \
     debianhvm_image=debian-7.2.0-amd64-CD-1.iso \
     bios=$bios \
     all_hostflags=$most_hostflags,hvm
 }
 
 do_hvm_debian_tests() {
+  test_xsm="$@"
+
   if [ $xenarch != amd64 ]; then
     return
   fi
 
   # QEMU upstream supports ovmf and seabios
   if [ "x$qemuu_suffix" == "x-qemuu" ]; then
-    do_hvm_debian_test_one ovmf ovmf
-    do_hvm_debian_test_one debianhvm seabios
+    do_hvm_debian_test_one ovmf ovmf n
+    for xsm in $test_xsm ; do
+      do_hvm_debian_test_one debianhvm seabios $xsm
+    done
   fi
 
   # QEMU traditional supports rombios
   if [ "x$qemuu_suffix" == "x-qemut" ]; then
-    do_hvm_debian_test_one debianhvm rombios
+    for xsm in $test_xsm ; do
+      do_hvm_debian_test_one debianhvm rombios $xsm
+    done
   fi
 }
 
@@ -281,13 +295,37 @@ do_passthrough_tests () {
   done
 }
 
-test_matrix_do_one () {
+do_pv_linux_xl_test_one () {
+  xsm=$1
 
-  # Basic PV Linux test with xl
+  xsm_suffix=""
+  if [ x$xsm = xy ] ; then
+      xsm_suffix="-xsm"
+  fi
 
-  job_create_test test-$xenarch$kern-$dom0arch-xl test-debian xl \
+  job_create_test test-$xenarch$kern-$dom0arch-xl$xsm_suffix test-debian xl \
             $xenarch $dom0arch                                   \
+            enable_xsm=$xsm                                      \
             $debian_runvars all_hostflags=$most_hostflags
+}
+
+test_matrix_do_one () {
+
+  # xsm tests for versions > 4.4, arch amd64 and armhf
+  case "$xenbranch:$xenarch" in
+  xen-3.*-testing:*) test_xsm="n";;
+  xen-4.0-testing:*) test_xsm="n";;
+  xen-4.1-testing:*) test_xsm="n";;
+  xen-4.2-testing:*) test_xsm="n";;
+  xen-4.3-testing:*) test_xsm="n";;
+  xen-4.4-testing:*) test_xsm="n";;
+  *:amd64|armhf)     test_xsm="n y";;
+  esac
+
+  # Basic PV Linux test with xl
+  for xsm in $test_xsm ; do
+    do_pv_linux_xl_test_one $xsm
+  done
 
   job_create_test test-$xenarch$kern-$dom0arch-libvirt test-debian libvirt \
             $xenarch $dom0arch                                       \
@@ -342,7 +380,7 @@ test_matrix_do_one () {
     do_hvm_win7_x64_tests
     do_hvm_rhel6_tests
 
-    do_hvm_debian_tests
+    do_hvm_debian_tests $test_xsm
 
   done # qemuu_suffix
 
-- 
1.7.10.4

[OSSTEST PATCH v2 11/12] ts-debian-install: add in seclabel if XSM is enabled

[Wei Liu]

... otherwise the guest cannot be created.

Also changed cfg_xend to cfg while I was there.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 ts-debian-install |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/ts-debian-install b/ts-debian-install
index 58ea743..c42ae09 100755
--- a/ts-debian-install
+++ b/ts-debian-install
@@ -89,8 +89,15 @@ END
             $initrd_opt \\
             $archarg
 END
-    my $cfg_xend= "/etc/xen/$gho->{Name}.cfg";
-    store_runvar("$gho->{Guest}_cfgpath", $cfg_xend);
+    my $cfg= "/etc/xen/$gho->{Name}.cfg";
+
+    if ($r{enable_xsm} =~ m/y/) {
+	target_cmd_root($ho, <<"END");
+echo "seclabel='system_u:system_r:domU_t'" >> $cfg
+END
+    }
+
+    store_runvar("$gho->{Guest}_cfgpath", $cfg);
     store_runvar("$gho->{Guest}_swap_lv", "$gho->{Name}-swap");
 }
 
-- 
1.7.10.4

[OSSTEST PATCH v2 12/12] ts-debian-hvm-install: add in seclabel if XSM is enabled

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 ts-debian-hvm-install |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
index 37eade2..3c557ef 100755
--- a/ts-debian-hvm-install
+++ b/ts-debian-hvm-install
@@ -152,10 +152,10 @@ sub prep () {
     target_putfilecontents_root_stash($ho, 10, preseed(),
                                       $preseed_file_path);
 
-    more_prepareguest_hvm($ho,$gho, $ram_mb, $disk_mb,
-                          OnReboot => 'preserve',
-                          Bios => $r{bios},
-                          PostImageHook => sub {
+    my $cfg = more_prepareguest_hvm($ho,$gho, $ram_mb, $disk_mb,
+				    OnReboot => 'preserve',
+				    Bios => $r{bios},
+				    PostImageHook => sub {
         my $cmds = iso_copy_content_from_image($gho, $newiso);
         $cmds .= prepare_initrd($initrddir,$newiso,$preseed_file_path);
         target_cmd_root($ho, $cmds, $isotimeout);
@@ -167,6 +167,12 @@ sub prep () {
 
         iso_create_genisoimage($ho, $gho->{Rimage}, $newiso, $isotimeout, @isogen_opts);
     });
+
+    if ($r{enable_xsm} =~ m/y/) {
+	target_cmd_root($ho, <<"END");
+echo "seclabel='system_u:system_r:domU_t'" >> $cfg
+END
+    }
 }
 
 # If host has >8G free memory, create a guest with 4G memory to catch
-- 
1.7.10.4

Re: [OSSTEST PATCH v2 00/12] XSM test cases for OSSTest

[Wei Liu]

Ping?

I'm just looking at my queue of unfinished stuffs. In any case this is
not very urgent and can wait. Just want to make sure it has not fallen
through the crack.

Wei.

Re: [OSSTEST PATCH v2 00/12] XSM test cases for OSSTest

[Ian Jackson]

Wei Liu writes ("Re: [OSSTEST PATCH v2 00/12] XSM test cases for OSSTest"):
> Ping?
> 
> I'm just looking at my queue of unfinished stuffs. In any case this is
> not very urgent and can wait. Just want to make sure it has not fallen
> through the crack.

Thanks.  I'll finish Ian's series since I'm halfway through and then
go and read yours...

Ian.

Re: [OSSTEST PATCH v2 01/12] README: list chiark-utils-bin as requirement

[Ian Jackson]

Wei Liu writes ("[OSSTEST PATCH v2 01/12] README: list chiark-utils-bin as requirement"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[OSSTEST PATCH v2 02/12] gitignore: ignore images directory

[Ian Jackson]

Wei Liu writes ("[OSSTEST PATCH v2 02/12] gitignore: ignore images directory"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [OSSTEST PATCH v2 03/12] ts-xen-build-prep: install checkpolicy

[Ian Jackson]

Wei Liu writes ("[OSSTEST PATCH v2 03/12] ts-xen-build-prep: install checkpolicy"):
> This is used to complie Flask policy.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [OSSTEST PATCH v2 04/12] ts-xen-build: build with XSM support if requested

[Ian Jackson]

Wei Liu writes ("[OSSTEST PATCH v2 04/12] ts-xen-build: build with XSM support if requested"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [OSSTEST PATCH v2 05/12] mfi-common: create build-$arch-xsm job

[Ian Jackson]

Wei Liu writes ("[OSSTEST PATCH v2 05/12] mfi-common: create build-$arch-xsm job"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> +    for xsm in $test_xsm; do
> +      if [ x$xsm = xy ] ; then
> +        xsm_suffix="-xsm"
> +        enable_xsm=y
> +      else
> +        xsm_suffix=""
> +        enable_xsm=n
> +      fi

Why not
   for enable_xsm in ...
and do away with one of the variables ?

And that if would probably be clearer as a case.

Thanks,
Ian.

Re: [OSSTEST PATCH v2 05/12] mfi-common: create build-$arch-xsm job

[Wei Liu]

On Fri, Oct 10, 2014 at 04:32:36PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[OSSTEST PATCH v2 05/12] mfi-common: create build-$arch-xsm job"):
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > +    for xsm in $test_xsm; do
> > +      if [ x$xsm = xy ] ; then
> > +        xsm_suffix="-xsm"
> > +        enable_xsm=y
> > +      else
> > +        xsm_suffix=""
> > +        enable_xsm=n
> > +      fi
> 
> Why not
>    for enable_xsm in ...
> and do away with one of the variables ?
> 
> And that if would probably be clearer as a case.
> 

No problem.

Wei

Re: [OSSTEST PATCH v2 06/12] Debian.pm: pass in XSM configuration to bootloader setup routines

[Ian Jackson]

Wei Liu writes ("[OSSTEST PATCH v2 06/12] Debian.pm: pass in XSM configuration to bootloader setup routines"):
> Change to Uboot will come in another patch. GRUB 1 is ignored, as
> currently OSSTest only has Wheezy which has GRUB 2.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [OSSTEST PATCH v2 07/12] Debian.pm: load flask policy in uboot

[Ian Jackson]

Wei Liu writes ("[OSSTEST PATCH v2 07/12] Debian.pm: load flask policy in uboot"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  Osstest/Debian.pm |   18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
> index 4092351..5aaab83 100644
> --- a/Osstest/Debian.pm
> +++ b/Osstest/Debian.pm
> @@ -123,6 +123,22 @@ sub setupboot_uboot ($$$$) {
>  	my $kern = "vmlinuz-$want_kernver";
>  	my $initrd = "initrd.img-$want_kernver";
>  
> +	my $flask_commands = "";
> +	if ($want_xsm) {
> +	    my $flaskpolicy = $r{flaskpoilcy};
                                 ^^^^^^^^^^^

Does this actually work ? :-).

I'd like an ack from Ian C on this one.

Ian.

Re: [OSSTEST PATCH v2 08/12] ts-xen-install: install Xen with XSM support if requested

[Ian Jackson]

Wei Liu writes ("[OSSTEST PATCH v2 08/12] ts-xen-install: install Xen with XSM support if requested"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  ts-xen-install |  115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 114 insertions(+), 1 deletion(-)
> 
> diff --git a/ts-xen-install b/ts-xen-install
> index 4d34d1f..3bc4d2e 100755
> --- a/ts-xen-install
> +++ b/ts-xen-install
> @@ -46,6 +46,8 @@ if (@ARGV and $ARGV[0] eq '--check') {
>  
>  our $ho;
>  
> +my $enable_xsm = $r{enable_xsm} =~ m/y/ ? 1 : 0;
> +
>  my %distpath;
>  
>  sub packages () {
> @@ -73,6 +75,15 @@ sub extract () {
>  				   $r{"${part}buildjob"}, \%distpath);
>      }
>      target_cmd_root($ho, '/sbin/ldconfig');
> +    if ($enable_xsm) {
> +        my $flaskpolicy = target_cmd_output_root($ho,
> +            'find /boot -name \'xenpolicy-*\' -exec basename {} \;');
> +	# there should only be one xenpolicy file for a clean install
> +	my $c = () = $flaskpolicy =~ /xenpolicy/g;
> +	die "Too many XSM policy files $c" if $c > 1;
> +	die "XSM policy file is required" if $c == 0;
> +	store_runvar("flaskpolicy", $flaskpolicy);

I don't much like this, I'm afraid.

I think this filename or version should be recorded by ts-xen-build
and the value used here.  Cf the `kernel_ver' runvar saved by
ts-kernel-build and used by ts-xen-install.

> +sub grub_patch () {
> +    return << 'END';

I'm afraid this isn't right.

Firstly, if we are patching this, the patch ought to be in a separate
file.

Secondly, overlay/etc/grub.d/20_linux_xen already exists (mentioning
Debian bug #633127).  This is automatically copied into the installed
system.  So any change should be made there.

Thirdly, you should mention the bug report that you have filed (you've
filed one, right?) about the need to patch in this support.

> +	target_cmd_root($ho, << 'END');
> +if test ! -e /etc/grub.d/20_linux_xen ; then
> +  case `uname -m` in
> +    x86*) echo '/etc/grub.d/20_linux_xen doesn't exist, abort'
> +          exit 1 ;;

Surely the right thing here is just to carry on.  Perhaps the file was
improved and renamed or something.  (If this it is to be done like
this at all, which I doubt - see above.)

Thanks,
Ian.

Re: [OSSTEST PATCH v2 09/12] mfi-common: use XSM build if job name contains -xsm suffix

[Ian Jackson]

Wei Liu writes ("[OSSTEST PATCH v2 09/12] mfi-common: use XSM build if job name contains -xsm suffix"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
...
> +  # Use xsm build job if job contains -xsm suffix
> +  case "$job" in
> +    *-xsm) xenbuildjob="$xenbuildjob-xsm"
> +           buildjob="$buildjob-xsm"
> +           ;;
> +    *) ;;
> +  esac

Please don't try pattern-matching $job like this.  Instead, invent an
extra argument to job_create_test.  Either `buildjobsuffix' applying
to buildjob and xenbuildjob or `xsmsuffix' or perhaps just the
xsm_enable value.

Thanks,
Ian.

Re: [OSSTEST PATCH v2 07/12] Debian.pm: load flask policy in uboot

[Wei Liu]

On Fri, Oct 10, 2014 at 04:39:51PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[OSSTEST PATCH v2 07/12] Debian.pm: load flask policy in uboot"):
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  Osstest/Debian.pm |   18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
> > index 4092351..5aaab83 100644
> > --- a/Osstest/Debian.pm
> > +++ b/Osstest/Debian.pm
> > @@ -123,6 +123,22 @@ sub setupboot_uboot ($$$$) {
> >  	my $kern = "vmlinuz-$want_kernver";
> >  	my $initrd = "initrd.img-$want_kernver";
> >  
> > +	my $flask_commands = "";
> > +	if ($want_xsm) {
> > +	    my $flaskpolicy = $r{flaskpoilcy};
>                                  ^^^^^^^^^^^
> 
> Does this actually work ? :-).
> 

Of course it doesn't! Fixed!

> I'd like an ack from Ian C on this one.
> 

He acked it already. ;-)

Wei.

> Ian.

Re: [OSSTEST PATCH v2 10/12] make-flight: create XSM test jobs

[Ian Jackson]

Wei Liu writes ("[OSSTEST PATCH v2 10/12] make-flight: create XSM test jobs"):
> Duplicate Debian PV and HVM test jobs for XSM testing.

> -  job_create_test test-$xenarch$kern-$dom0arch-xl$qemuu_suffix-$testname-amd64\
> +  xsm=$3
> +
> +  xsm_suffix=""
> +  if [ x$xsm = xy ] ; then
> +      xsm_suffix="-xsm"
> +  fi

I think you should pass $xsm as a new argument to job_create_test and
move this (repeated, in this patch) suffix handling into
job_create_test.

> +  # xsm tests for versions > 4.4, arch amd64 and armhf
> +  case "$xenbranch:$xenarch" in
> +  xen-3.*-testing:*) test_xsm="n";;
> +  xen-4.0-testing:*) test_xsm="n";;
> +  xen-4.1-testing:*) test_xsm="n";;
> +  xen-4.2-testing:*) test_xsm="n";;
> +  xen-4.3-testing:*) test_xsm="n";;
> +  xen-4.4-testing:*) test_xsm="n";;
> +  *:amd64|armhf)     test_xsm="n y";;

I seem to remember this table appearing in a previous patch in this
series, so now you have two copies of it.  Please have only one.


Also, when you make changes to make-flight etc. (that is, for this
patch, and the other relevant ones in this series) you should do a
before-and-after check with standalone-generate-dump-flight-runvars
(or mg-show-flight-runvars directly) - diffing the results.

You should state the results of that diff in your commit message.  In
this case I'd expect you to assert that the existing jobs were
unchanged and list the new jobs to be included.

Ian.

Re: [OSSTEST PATCH v2 11/12] ts-debian-install: add in seclabel if XSM is enabled

[Ian Jackson]

Wei Liu writes ("[OSSTEST PATCH v2 11/12] ts-debian-install: add in seclabel if XSM is enabled"):
> ... otherwise the guest cannot be created.

Surely it is a bug that this is necessary ?  xl shuld do the right
thing by default.

Daniel ?

(This is no barrier to the series going in without this patch: it is
OK to introduce tests which currently fail.)

> Also changed cfg_xend to cfg while I was there.

That, tiny as it is, is very significant, and needs to be in a
separate patch.

Thanks,
Ian.

Re: [OSSTEST PATCH v2 12/12] ts-debian-hvm-install: add in seclabel if XSM is enabled

[Ian Jackson]

Wei Liu writes ("[OSSTEST PATCH v2 12/12] ts-debian-hvm-install: add in seclabel if XSM is enabled"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

See my comment on the previous patch.

Also, if it _is_ necessary to edit the configs like this, it should be
done in one place, not two.  If there is not currently one place where
that could be done, then you will have to make one (sorry).

Thanks,
Ian.

Re: [OSSTEST PATCH v2 07/12] Debian.pm: load flask policy in uboot

[Ian Jackson]

Wei Liu writes ("Re: [OSSTEST PATCH v2 07/12] Debian.pm: load flask policy in uboot"):
> On Fri, Oct 10, 2014 at 04:39:51PM +0100, Ian Jackson wrote:
> > Does this actually work ? :-).
> 
> Of course it doesn't! Fixed!

Thanks.  With that change:

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

> > I'd like an ack from Ian C on this one.
> 
> He acked it already. ;-)

Obviously he must have tested it thoroughly :-).

Ian.

Re: [OSSTEST PATCH v2 08/12] ts-xen-install: install Xen with XSM support if requested

[Wei Liu]

On Fri, Oct 10, 2014 at 04:51:12PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[OSSTEST PATCH v2 08/12] ts-xen-install: install Xen with XSM support if requested"):
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  ts-xen-install |  115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 114 insertions(+), 1 deletion(-)
> > 
> > diff --git a/ts-xen-install b/ts-xen-install
> > index 4d34d1f..3bc4d2e 100755
> > --- a/ts-xen-install
> > +++ b/ts-xen-install
> > @@ -46,6 +46,8 @@ if (@ARGV and $ARGV[0] eq '--check') {
> >  
> >  our $ho;
> >  
> > +my $enable_xsm = $r{enable_xsm} =~ m/y/ ? 1 : 0;
> > +
> >  my %distpath;
> >  
> >  sub packages () {
> > @@ -73,6 +75,15 @@ sub extract () {
> >  				   $r{"${part}buildjob"}, \%distpath);
> >      }
> >      target_cmd_root($ho, '/sbin/ldconfig');
> > +    if ($enable_xsm) {
> > +        my $flaskpolicy = target_cmd_output_root($ho,
> > +            'find /boot -name \'xenpolicy-*\' -exec basename {} \;');
> > +	# there should only be one xenpolicy file for a clean install
> > +	my $c = () = $flaskpolicy =~ /xenpolicy/g;
> > +	die "Too many XSM policy files $c" if $c > 1;
> > +	die "XSM policy file is required" if $c == 0;
> > +	store_runvar("flaskpolicy", $flaskpolicy);
> 
> I don't much like this, I'm afraid.
> 
> I think this filename or version should be recorded by ts-xen-build
> and the value used here.  Cf the `kernel_ver' runvar saved by
> ts-kernel-build and used by ts-xen-install.
> 

No problem.

> > +sub grub_patch () {
> > +    return << 'END';
> 
> I'm afraid this isn't right.
> 
> Firstly, if we are patching this, the patch ought to be in a separate
> file.
> 

OK.

> Secondly, overlay/etc/grub.d/20_linux_xen already exists (mentioning
> Debian bug #633127).  This is automatically copied into the installed
> system.  So any change should be made there.
> 

OK, I missed the overlay file. No wonder the patch I wrote for my dev
machine did work on live OSSTest host!

> Thirdly, you should mention the bug report that you have filed (you've
> filed one, right?) about the need to patch in this support.
> 

No, I haven't. My plan was to file a bug after this series is merged.

But it looks like you would like me to file a bug first. I can do this
as well.

> > +	target_cmd_root($ho, << 'END');
> > +if test ! -e /etc/grub.d/20_linux_xen ; then
> > +  case `uname -m` in
> > +    x86*) echo '/etc/grub.d/20_linux_xen doesn't exist, abort'
> > +          exit 1 ;;
> 
> Surely the right thing here is just to carry on.  Perhaps the file was
> improved and renamed or something.  (If this it is to be done like
> this at all, which I doubt - see above.)
> 

If I'm to change the overlay file there won't be need to patch it
anymore.

Wei.

> Thanks,
> Ian.

Re: [OSSTEST PATCH v2 11/12] ts-debian-install: add in seclabel if XSM is enabled

[Wei Liu]

On Fri, Oct 10, 2014 at 05:01:57PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[OSSTEST PATCH v2 11/12] ts-debian-install: add in seclabel if XSM is enabled"):
> > ... otherwise the guest cannot be created.
> 
> Surely it is a bug that this is necessary ?  xl shuld do the right
> thing by default.
> 

Well, xl is doing the right thing. Xen denies starting a guest without a
seclabel. I think this is policy related, so it shouldn't be classified
as a bug.

> Daniel ?
> 
> (This is no barrier to the series going in without this patch: it is
> OK to introduce tests which currently fail.)
> 
> > Also changed cfg_xend to cfg while I was there.
> 
> That, tiny as it is, is very significant, and needs to be in a
> separate patch.
> 

Done.

Wei.

> Thanks,
> Ian.

Re: [OSSTEST PATCH v2 12/12] ts-debian-hvm-install: add in seclabel if XSM is enabled

[Wei Liu]

On Fri, Oct 10, 2014 at 05:03:05PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[OSSTEST PATCH v2 12/12] ts-debian-hvm-install: add in seclabel if XSM is enabled"):
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> See my comment on the previous patch.
> 
> Also, if it _is_ necessary to edit the configs like this, it should be
> done in one place, not two.  If there is not currently one place where
> that could be done, then you will have to make one (sorry).
> 

I once thought about this, but for PV guest the config file comes from
xen-tools which is invoked from ts-debian-install while for HVM guest it
comes from a template inside OSSTest, what would be your suggestion to
unify them?

Wei.

> Thanks,
> Ian.

Re: [OSSTEST PATCH v2 08/12] ts-xen-install: install Xen with XSM support if requested

[Ian Jackson]

Wei Liu writes ("Re: [OSSTEST PATCH v2 08/12] ts-xen-install: install Xen with XSM support if requested"):
> On Fri, Oct 10, 2014 at 04:51:12PM +0100, Ian Jackson wrote:
> > Secondly, overlay/etc/grub.d/20_linux_xen already exists (mentioning
> > Debian bug #633127).  This is automatically copied into the installed
> > system.  So any change should be made there.
> 
> OK, I missed the overlay file. No wonder the patch I wrote for my dev
> machine did work on live OSSTest host!

Heh.

> > Thirdly, you should mention the bug report that you have filed (you've
> > filed one, right?) about the need to patch in this support.
> 
> No, I haven't. My plan was to file a bug after this series is merged.
> 
> But it looks like you would like me to file a bug first. I can do this
> as well.

Yes, please.

> > Surely the right thing here is just to carry on.  Perhaps the file was
> > improved and renamed or something.  (If this it is to be done like
> > this at all, which I doubt - see above.)
> 
> If I'm to change the overlay file there won't be need to patch it
> anymore.

Right.

Ina.

Re: [OSSTEST PATCH v2 11/12] ts-debian-install: add in seclabel if XSM is enabled

[Ian Jackson]

Wei Liu writes ("Re: [OSSTEST PATCH v2 11/12] ts-debian-install: add in seclabel if XSM is enabled"):
> On Fri, Oct 10, 2014 at 05:01:57PM +0100, Ian Jackson wrote:
> > Surely it is a bug that this is necessary ?  xl shuld do the right
> > thing by default.
> 
> Well, xl is doing the right thing. Xen denies starting a guest without a
> seclabel. I think this is policy related, so it shouldn't be classified
> as a bug.

You haven't asked xl to `start a guest without a seclabel'.

You have asked xl to `start a guest'.

xl should do whatever is necessary to implement your wishes (assuming
your wishes are reasonable, of course).  If guests have to have
seclabels, xl should arrange to give them seclabels.  If you don't
specify the seclabel, xl should figure out what seclabel to give them.

And most of this ought probably to be in libxl, probably, rather than
xl.

Ian.

Re: [OSSTEST PATCH v2 11/12] ts-debian-install: add in seclabel if XSM is enabled

[Wei Liu]

On Fri, Oct 10, 2014 at 05:41:08PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [OSSTEST PATCH v2 11/12] ts-debian-install: add in seclabel if XSM is enabled"):
> > On Fri, Oct 10, 2014 at 05:01:57PM +0100, Ian Jackson wrote:
> > > Surely it is a bug that this is necessary ?  xl shuld do the right
> > > thing by default.
> > 
> > Well, xl is doing the right thing. Xen denies starting a guest without a
> > seclabel. I think this is policy related, so it shouldn't be classified
> > as a bug.
> 
> You haven't asked xl to `start a guest without a seclabel'.
> 
> You have asked xl to `start a guest'.
> 
> xl should do whatever is necessary to implement your wishes (assuming
> your wishes are reasonable, of course).

I agree. And it's reasonable for hypervisor to reject this request. I
think this is policy related.

> If guests have to have
> seclabels, xl should arrange to give them seclabels.  If you don't
> specify the seclabel, xl should figure out what seclabel to give them.
> 

I don't see it this way as there's no documentation on what the
"default seclabel" is.

I think this is one is for Daniel.

Wei.

> And most of this ought probably to be in libxl, probably, rather than
> xl.
> 
> Ian.

Re: [OSSTEST PATCH v2 12/12] ts-debian-hvm-install: add in seclabel if XSM is enabled

[Ian Jackson]

Wei Liu writes ("Re: [OSSTEST PATCH v2 12/12] ts-debian-hvm-install: add in seclabel if XSM is enabled"):
> On Fri, Oct 10, 2014 at 05:03:05PM +0100, Ian Jackson wrote:
> > Also, if it _is_ necessary to edit the configs like this, it should be
> > done in one place, not two.  If there is not currently one place where
> > that could be done, then you will have to make one (sorry).
> 
> I once thought about this, but for PV guest the config file comes from
> xen-tools which is invoked from ts-debian-install while for HVM guest it
> comes from a template inside OSSTest, what would be your suggestion to
> unify them?

It might be necessary to invent a general-purpose config-fiddling
function which was called in both places.  But I think this is
hypothetical, really: the config out of xen-tools should work.  That
it doesn't is a bug, as I'm arguing in the other subthread.

Ian.

Re: [OSSTEST PATCH v2 11/12] ts-debian-install: add in seclabel if XSM is enabled

[Ian Jackson]

Wei Liu writes ("Re: [OSSTEST PATCH v2 11/12] ts-debian-install: add in seclabel if XSM is enabled"):
> On Fri, Oct 10, 2014 at 05:41:08PM +0100, Ian Jackson wrote:
> > xl should do whatever is necessary to implement your wishes (assuming
> > your wishes are reasonable, of course).
> 
> I agree. And it's reasonable for hypervisor to reject this request. I
> think this is policy related.

Indeed, I have no objection to the hypervisor's policy setup.

> > If guests have to have seclabels, xl should arrange to give them
> > seclabels.  If you don't specify the seclabel, xl should figure
> > out what seclabel to give them.
> 
> I don't see it this way as there's no documentation on what the
> "default seclabel" is.

Maybe it should be documented, or configurable.

> I think this is one is for Daniel.

Yes.

In the meantime I am happy to take your series without these
seclabel-in-config patches.  The result will be some tests which fail
until the underlying problem is resolved.

Ian.

Re: [OSSTEST PATCH v2 11/12] ts-debian-install: add in seclabel if XSM is enabled

[Wei Liu]

On Fri, Oct 10, 2014 at 06:26:57PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [OSSTEST PATCH v2 11/12] ts-debian-install: add in seclabel if XSM is enabled"):
> > On Fri, Oct 10, 2014 at 05:41:08PM +0100, Ian Jackson wrote:
> > > xl should do whatever is necessary to implement your wishes (assuming
> > > your wishes are reasonable, of course).
> > 
> > I agree. And it's reasonable for hypervisor to reject this request. I
> > think this is policy related.
> 
> Indeed, I have no objection to the hypervisor's policy setup.
> 
> > > If guests have to have seclabels, xl should arrange to give them
> > > seclabels.  If you don't specify the seclabel, xl should figure
> > > out what seclabel to give them.
> > 
> > I don't see it this way as there's no documentation on what the
> > "default seclabel" is.
> 
> Maybe it should be documented, or configurable.
> 
> > I think this is one is for Daniel.
> 
> Yes.
> 
> In the meantime I am happy to take your series without these
> seclabel-in-config patches.  The result will be some tests which fail
> until the underlying problem is resolved.
> 

I'm fine with this approach.

Wei.

> Ian.

Re: [OSSTEST PATCH v2 10/12] make-flight: create XSM test jobs

[Wei Liu]

On Fri, Oct 10, 2014 at 05:00:15PM +0100, Ian Jackson wrote:
[...]
> > +  # xsm tests for versions > 4.4, arch amd64 and armhf
> > +  case "$xenbranch:$xenarch" in
> > +  xen-3.*-testing:*) test_xsm="n";;
> > +  xen-4.0-testing:*) test_xsm="n";;
> > +  xen-4.1-testing:*) test_xsm="n";;
> > +  xen-4.2-testing:*) test_xsm="n";;
> > +  xen-4.3-testing:*) test_xsm="n";;
> > +  xen-4.4-testing:*) test_xsm="n";;
> > +  *:amd64|armhf)     test_xsm="n y";;
> 
> I seem to remember this table appearing in a previous patch in this
> series, so now you have two copies of it.  Please have only one.
> 

I don't think it's right to make have only one. That would mean leaking
a global variable to another script.

It's a bit like test_xend in make-flight v.s. build_defxend in
mfi-common. I think changing test_xsm to build_xsm in mfi-common may
make things clearer.

Wei.

Re: [OSSTEST PATCH v2 11/12] ts-debian-install: add in seclabel if XSM is enabled

[Ian Campbell]

On Fri, 2014-10-10 at 18:26 +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [OSSTEST PATCH v2 11/12] ts-debian-install: add in seclabel if XSM is enabled"):
> > On Fri, Oct 10, 2014 at 05:41:08PM +0100, Ian Jackson wrote:
> > > xl should do whatever is necessary to implement your wishes (assuming
> > > your wishes are reasonable, of course).
> > 
> > I agree. And it's reasonable for hypervisor to reject this request. I
> > think this is policy related.
> 
> Indeed, I have no objection to the hypervisor's policy setup.
> 
> > > If guests have to have seclabels, xl should arrange to give them
> > > seclabels.  If you don't specify the seclabel, xl should figure
> > > out what seclabel to give them.
> > 
> > I don't see it this way as there's no documentation on what the
> > "default seclabel" is.
> 
> Maybe it should be documented, or configurable.

It will have to be configurable since the user is at liberty to use
whatever policy they want, including writing their own from scratch, and
could give their domain labels any name they like, so there is no
universal sensible default. We could set a default relating to the
example policy which we ship but that is about all we can do.

There also needs to be an option to force the seclabel to be explicitly
specified for every domain, to allow people who have more complex setups
to not worry about some domain getting the default policy/permissions.

Ian.

Re: [OSSTEST PATCH v2 11/12] ts-debian-install: add in seclabel if XSM is enabled

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST PATCH v2 11/12] ts-debian-install: add in seclabel if XSM is enabled"):
> On Fri, 2014-10-10 at 18:26 +0100, Ian Jackson wrote:
> > Maybe it should be documented, or configurable.
> 
> It will have to be configurable since the user is at liberty to use
> whatever policy they want, including writing their own from scratch, and
> could give their domain labels any name they like, so there is no
> universal sensible default. We could set a default relating to the
> example policy which we ship but that is about all we can do.

Yes, indeed.

> There also needs to be an option to force the seclabel to be explicitly
> specified for every domain, to allow people who have more complex setups
> to not worry about some domain getting the default policy/permissions.

Indeed.  In the case where this forces creation failure it would be
nice to detect it a bit earlier than when the hypervisor says `no'.

Ian.

Re: [OSSTEST PATCH v2 10/12] make-flight: create XSM test jobs

[Ian Jackson]

Wei Liu writes ("Re: [OSSTEST PATCH v2 10/12] make-flight: create XSM test jobs"):
> On Fri, Oct 10, 2014 at 05:00:15PM +0100, Ian Jackson wrote:
> [...]
> > > +  # xsm tests for versions > 4.4, arch amd64 and armhf
> > > +  case "$xenbranch:$xenarch" in
> > > +  xen-3.*-testing:*) test_xsm="n";;
> > > +  xen-4.0-testing:*) test_xsm="n";;
> > > +  xen-4.1-testing:*) test_xsm="n";;
> > > +  xen-4.2-testing:*) test_xsm="n";;
> > > +  xen-4.3-testing:*) test_xsm="n";;
> > > +  xen-4.4-testing:*) test_xsm="n";;
> > > +  *:amd64|armhf)     test_xsm="n y";;
> > 
> > I seem to remember this table appearing in a previous patch in this
> > series, so now you have two copies of it.  Please have only one.
> 
> I don't think it's right to make have only one. That would mean leaking
> a global variable to another script.

It doesn't have to mean that.  There could be a shell function in
mfi-common.  Cf branch_wants_rumpkernel_tests.

> It's a bit like test_xend in make-flight v.s. build_defxend in
> mfi-common. I think changing test_xsm to build_xsm in mfi-common may
> make things clearer.

The xend stuff is different because the two tables have different
contents.  (Because of the different defaults in various versions of
Xen.)

Ian.

Re: [OSSTEST PATCH v2 10/12] make-flight: create XSM test jobs

[Wei Liu]

On Tue, Oct 14, 2014 at 04:41:07PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [OSSTEST PATCH v2 10/12] make-flight: create XSM test jobs"):
> > On Fri, Oct 10, 2014 at 05:00:15PM +0100, Ian Jackson wrote:
> > [...]
> > > > +  # xsm tests for versions > 4.4, arch amd64 and armhf
> > > > +  case "$xenbranch:$xenarch" in
> > > > +  xen-3.*-testing:*) test_xsm="n";;
> > > > +  xen-4.0-testing:*) test_xsm="n";;
> > > > +  xen-4.1-testing:*) test_xsm="n";;
> > > > +  xen-4.2-testing:*) test_xsm="n";;
> > > > +  xen-4.3-testing:*) test_xsm="n";;
> > > > +  xen-4.4-testing:*) test_xsm="n";;
> > > > +  *:amd64|armhf)     test_xsm="n y";;
> > > 
> > > I seem to remember this table appearing in a previous patch in this
> > > series, so now you have two copies of it.  Please have only one.
> > 
> > I don't think it's right to make have only one. That would mean leaking
> > a global variable to another script.
> 
> It doesn't have to mean that.  There could be a shell function in
> mfi-common.  Cf branch_wants_rumpkernel_tests.
> 

I see. I will make a function called xenbranch_wants_xsm_tests.

> > It's a bit like test_xend in make-flight v.s. build_defxend in
> > mfi-common. I think changing test_xsm to build_xsm in mfi-common may
> > make things clearer.
> 
> The xend stuff is different because the two tables have different
> contents.  (Because of the different defaults in various versions of
> Xen.)
> 
> Ian.