[kvm-unit-tests PATCH v2 0/2] s390x: dump support for PV tests

[kvm-unit-tests PATCH v2 0/2] s390x: dump support for PV tests

[Nico Boehr]

v1->v2:
---
- add newline after genprotimg_args (thanks Janosch)
- add a comment explaining what the CCK is (thanks Janosch)

With the upcoming possibility to dump PV guests under s390x, we should
be able to dump kvm-unit-tests for debugging, too.

Add the necessary flags to genprotimg to allow dumping.

Nico Boehr (2):
  s390x: factor out common args for genprotimg
  s390x: create persistent comm-key

 s390x/Makefile | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

-- 
2.36.1

[kvm-unit-tests PATCH v2 1/2] s390x: factor out common args for genprotimg

[Nico Boehr]

Upcoming changes will add more arguments to genprotimg. To avoid
duplicating this logic, move the arguments to genprotimg to a variable.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 s390x/Makefile | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/s390x/Makefile b/s390x/Makefile
index efd5e0c13102..d17055ebe6a8 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -165,11 +165,13 @@ $(SNIPPET_DIR)/c/%.hdr: $(SNIPPET_DIR)/c/%.gbin $(HOST_KEY_DOCUMENT)
 %.bin: %.elf
 	$(OBJCOPY) -O binary  $< $@
 
+genprotimg_args = --host-key-document $(HOST_KEY_DOCUMENT) --no-verify
+
 %selftest.pv.bin: %selftest.bin $(HOST_KEY_DOCUMENT) $(patsubst %.pv.bin,%.parmfile,$@)
-	$(GENPROTIMG) --host-key-document $(HOST_KEY_DOCUMENT) --parmfile $(patsubst %.pv.bin,%.parmfile,$@) --no-verify --image $< -o $@
+	$(GENPROTIMG) $(genprotimg_args) --parmfile $(patsubst %.pv.bin,%.parmfile,$@) --image $< -o $@
 
 %.pv.bin: %.bin $(HOST_KEY_DOCUMENT)
-	$(GENPROTIMG) --host-key-document $(HOST_KEY_DOCUMENT) --no-verify --image $< -o $@
+	$(GENPROTIMG) $(genprotimg_args) --image $< -o $@
 
 $(snippet_asmlib): $$(patsubst %.o,%.S,$$@) $(asm-offsets)
 	$(CC) $(CFLAGS) -c -nostdlib -o $@ $<
-- 
2.36.1

Re: [kvm-unit-tests PATCH v2 1/2] s390x: factor out common args for genprotimg

[Janosch Frank]

On 8/25/22 15:15, Nico Boehr wrote:
> Upcoming changes will add more arguments to genprotimg. To avoid
> duplicating this logic, move the arguments to genprotimg to a variable.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

> ---
>   s390x/Makefile | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index efd5e0c13102..d17055ebe6a8 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -165,11 +165,13 @@ $(SNIPPET_DIR)/c/%.hdr: $(SNIPPET_DIR)/c/%.gbin $(HOST_KEY_DOCUMENT)
>   %.bin: %.elf
>   	$(OBJCOPY) -O binary  $< $@
>   
> +genprotimg_args = --host-key-document $(HOST_KEY_DOCUMENT) --no-verify
> +
>   %selftest.pv.bin: %selftest.bin $(HOST_KEY_DOCUMENT) $(patsubst %.pv.bin,%.parmfile,$@)
> -	$(GENPROTIMG) --host-key-document $(HOST_KEY_DOCUMENT) --parmfile $(patsubst %.pv.bin,%.parmfile,$@) --no-verify --image $< -o $@
> +	$(GENPROTIMG) $(genprotimg_args) --parmfile $(patsubst %.pv.bin,%.parmfile,$@) --image $< -o $@
>   
>   %.pv.bin: %.bin $(HOST_KEY_DOCUMENT)
> -	$(GENPROTIMG) --host-key-document $(HOST_KEY_DOCUMENT) --no-verify --image $< -o $@
> +	$(GENPROTIMG) $(genprotimg_args) --image $< -o $@
>   
>   $(snippet_asmlib): $$(patsubst %.o,%.S,$$@) $(asm-offsets)
>   	$(CC) $(CFLAGS) -c -nostdlib -o $@ $<

[kvm-unit-tests PATCH v2 2/2] s390x: create persistent comm-key

[Nico Boehr]

To decrypt the dump of a PV guest, the comm-key (CCK) is required. Until
now, no comm-key was provided to genprotimg, therefore decrypting the
dump of a kvm-unit-test under PV was not possible.

This patch makes sure that we create a random CCK if there's no
$(TEST_DIR)/comm.key file.

Also allow dumping of PV tests by passing the appropriate PCF to
genprotimg (bit 34). --x-pcf is used to be compatible with older
genprotimg versions, which don't support --enable-dump. 0xe0 is the
default PCF value and only bit 34 is added.

Unfortunately, recent versions of genprotimg removed the --x-comm-key
argument which was used by older versions to specify the CCK. To support
these versions, we need to parse the genprotimg help output and decide
which argument to use.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/Makefile | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/s390x/Makefile b/s390x/Makefile
index d17055ebe6a8..4e268f47b6ab 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -162,15 +162,30 @@ $(SNIPPET_DIR)/c/%.hdr: $(SNIPPET_DIR)/c/%.gbin $(HOST_KEY_DOCUMENT)
 	$(RM) $(@:.elf=.aux.o)
 	@chmod a-x $@
 
+# Secure Execution Customer Communication Key file
+# 32 bytes of key material, uses existing one if available
+comm-key = $(TEST_DIR)/comm.key
+$(comm-key):
+	dd if=/dev/urandom of=$@ bs=32 count=1 status=none
+
 %.bin: %.elf
 	$(OBJCOPY) -O binary  $< $@
 
-genprotimg_args = --host-key-document $(HOST_KEY_DOCUMENT) --no-verify
+GENPROTIMG_HAS_COMM_KEY = $(shell $(GENPROTIMG) --help | grep -q -- --comm-key && echo yes)
+ifeq ($(GENPROTIMG_HAS_COMM_KEY),yes)
+	GENPROTIMG_COMM_KEY = --comm-key $(comm-key)
+else
+	GENPROTIMG_COMM_KEY = --x-comm-key $(comm-key)
+endif
+# use x-pcf to be compatible with old genprotimg versions
+# allow dumping + PCKMO
+genprotimg_pcf = 0x200000e0
+genprotimg_args = --host-key-document $(HOST_KEY_DOCUMENT) --no-verify $(GENPROTIMG_COMM_KEY) --x-pcf $(genprotimg_pcf)
 
-%selftest.pv.bin: %selftest.bin $(HOST_KEY_DOCUMENT) $(patsubst %.pv.bin,%.parmfile,$@)
+%selftest.pv.bin: %selftest.bin $(HOST_KEY_DOCUMENT) $(patsubst %.pv.bin,%.parmfile,$@) $(comm-key)
 	$(GENPROTIMG) $(genprotimg_args) --parmfile $(patsubst %.pv.bin,%.parmfile,$@) --image $< -o $@
 
-%.pv.bin: %.bin $(HOST_KEY_DOCUMENT)
+%.pv.bin: %.bin $(HOST_KEY_DOCUMENT) $(comm-key)
 	$(GENPROTIMG) $(genprotimg_args) --image $< -o $@
 
 $(snippet_asmlib): $$(patsubst %.o,%.S,$$@) $(asm-offsets)
@@ -178,7 +193,7 @@ $(snippet_asmlib): $$(patsubst %.o,%.S,$$@) $(asm-offsets)
 
 
 arch_clean: asm_offsets_clean
-	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(SNIPPET_DIR)/*/*.{o,elf,*bin,*obj,hdr} $(SNIPPET_DIR)/asm/.*.d $(TEST_DIR)/.*.d lib/s390x/.*.d
+	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(SNIPPET_DIR)/*/*.{o,elf,*bin,*obj,hdr} $(SNIPPET_DIR)/asm/.*.d $(TEST_DIR)/.*.d lib/s390x/.*.d $(comm-key)
 
 generated-files = $(asm-offsets)
 $(tests:.elf=.o) $(asmlib) $(cflatobjs): $(generated-files)
-- 
2.36.1

Re: [kvm-unit-tests PATCH v2 2/2] s390x: create persistent comm-key

[Janosch Frank]

On 8/25/22 15:16, Nico Boehr wrote:
> To decrypt the dump of a PV guest, the comm-key (CCK) is required. Until
> now, no comm-key was provided to genprotimg, therefore decrypting the
> dump of a kvm-unit-test under PV was not possible.
> 
> This patch makes sure that we create a random CCK if there's no
> $(TEST_DIR)/comm.key file.
> 
> Also allow dumping of PV tests by passing the appropriate PCF to
> genprotimg (bit 34). --x-pcf is used to be compatible with older
> genprotimg versions, which don't support --enable-dump. 0xe0 is the
> default PCF value and only bit 34 is added.
> 
> Unfortunately, recent versions of genprotimg removed the --x-comm-key
> argument which was used by older versions to specify the CCK. To support
> these versions, we need to parse the genprotimg help output and decide
> which argument to use.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>

There are three minor issues that keep me from picking this

> ---
>   s390x/Makefile | 23 +++++++++++++++++++----
>   1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index d17055ebe6a8..4e268f47b6ab 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -162,15 +162,30 @@ $(SNIPPET_DIR)/c/%.hdr: $(SNIPPET_DIR)/c/%.gbin $(HOST_KEY_DOCUMENT)
>   	$(RM) $(@:.elf=.aux.o)
>   	@chmod a-x $@
>   
> +# Secure Execution Customer Communication Key file
> +# 32 bytes of key material, uses existing one if available
> +comm-key = $(TEST_DIR)/comm.key
> +$(comm-key):
> +	dd if=/dev/urandom of=$@ bs=32 count=1 status=none
> +
>   %.bin: %.elf
>   	$(OBJCOPY) -O binary  $< $@
>   
> -genprotimg_args = --host-key-document $(HOST_KEY_DOCUMENT) --no-verify

Add comment:


# The genprotimg arguments for the cck changed over time so we need to 
figure out which argument to use in order to set the cck

> +GENPROTIMG_HAS_COMM_KEY = $(shell $(GENPROTIMG) --help | grep -q -- --comm-key && echo yes)
> +ifeq ($(GENPROTIMG_HAS_COMM_KEY),yes)
> +	GENPROTIMG_COMM_KEY = --comm-key $(comm-key)
> +else
> +	GENPROTIMG_COMM_KEY = --x-comm-key $(comm-key)
> +endif

I'd like to have a \n here as well

> +# use x-pcf to be compatible with old genprotimg versions
> +# allow dumping + PCKMO
> +genprotimg_pcf = 0x200000e0
> +genprotimg_args = --host-key-document $(HOST_KEY_DOCUMENT) --no-verify $(GENPROTIMG_COMM_KEY) --x-pcf $(genprotimg_pcf)
>   
> -%selftest.pv.bin: %selftest.bin $(HOST_KEY_DOCUMENT) $(patsubst %.pv.bin,%.parmfile,$@)
> +%selftest.pv.bin: %selftest.bin $(HOST_KEY_DOCUMENT) $(patsubst %.pv.bin,%.parmfile,$@) $(comm-key)
>   	$(GENPROTIMG) $(genprotimg_args) --parmfile $(patsubst %.pv.bin,%.parmfile,$@) --image $< -o $@
>   
> -%.pv.bin: %.bin $(HOST_KEY_DOCUMENT)
> +%.pv.bin: %.bin $(HOST_KEY_DOCUMENT) $(comm-key)
>   	$(GENPROTIMG) $(genprotimg_args) --image $< -o $@
>   
>   $(snippet_asmlib): $$(patsubst %.o,%.S,$$@) $(asm-offsets)
> @@ -178,7 +193,7 @@ $(snippet_asmlib): $$(patsubst %.o,%.S,$$@) $(asm-offsets)
>   
>   
>   arch_clean: asm_offsets_clean
> -	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(SNIPPET_DIR)/*/*.{o,elf,*bin,*obj,hdr} $(SNIPPET_DIR)/asm/.*.d $(TEST_DIR)/.*.d lib/s390x/.*.d
> +	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(SNIPPET_DIR)/*/*.{o,elf,*bin,*obj,hdr} $(SNIPPET_DIR)/asm/.*.d $(TEST_DIR)/.*.d lib/s390x/.*.d $(comm-key)

Hmmmmmmm(TM)

My first thought was that I'd be pretty angry if the CCK changes on a 
distclean. But the only scenario where this would matter is when the 
tests are provided to another system.

I'm still a bit torn about deleting the CCK especially as there will 
always be a CCK in the SE header no matter if we specify one or not.

>   
>   generated-files = $(asm-offsets)
>   $(tests:.elf=.o) $(asmlib) $(cflatobjs): $(generated-files)

Re: [kvm-unit-tests PATCH v2 2/2] s390x: create persistent comm-key

[Nico Boehr]

Quoting Janosch Frank (2022-09-06 11:50:46)
> Hmmmmmmm(TM)
> 
> My first thought was that I'd be pretty angry if the CCK changes on a 
> distclean. But the only scenario where this would matter is when the 
> tests are provided to another system.
> 
> I'm still a bit torn about deleting the CCK especially as there will 
> always be a CCK in the SE header no matter if we specify one or not.

I really don't have a strong opinion about this. I think it makes sense to clean
up stuff a Makefile has left behind. But I am honestly just as fine with
removing this.

Re: [kvm-unit-tests PATCH v2 2/2] s390x: create persistent comm-key

[Janosch Frank]

On 9/6/22 17:31, Nico Boehr wrote:
> Quoting Janosch Frank (2022-09-06 11:50:46)
>> Hmmmmmmm(TM)
>>
>> My first thought was that I'd be pretty angry if the CCK changes on a
>> distclean. But the only scenario where this would matter is when the
>> tests are provided to another system.
>>
>> I'm still a bit torn about deleting the CCK especially as there will
>> always be a CCK in the SE header no matter if we specify one or not.
> 
> I really don't have a strong opinion about this. I think it makes sense to clean
> up stuff a Makefile has left behind. But I am honestly just as fine with
> removing this.

Keep it as is, we can still change it when we get complaints. :)