Re: [kvm-unit-tests PATCH 7/8] s390x: snippets: Add PV support

[Janosch Frank <frankja@linux.ibm.com>]

On 11/26/21 14:28, Janosch Frank wrote:
> On 11/23/21 12:22, Claudio Imbrenda wrote:
>> On Tue, 23 Nov 2021 10:39:55 +0000
>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>
>>> To create a pv-snippet we need to generate an se-header. This can be
>>> done using a currently not released tool. This tool creates a
>>> se-header similar to `genprotimg` with the difference that the image
>>> itself will not be encrypted.
>>>
>>> The image for which we want to create a header must be a binary and
>>> padded to 4k. Therefore, we convert the compiled snippet to a binary,
>>> padd it to 4k, generate the header and convert it back to s390-64bit
>>> elf.
>>>
>>> The name of the tool can be specified using the config argument
>>> `--gen-se-header=`. The pv-snipptes will only be built when this
>>> option is specified. Furthermore, the Hostkey-Document must be
>>> specified. If not the build will be skipped.
>>>
>>> The host-snippet relation can be specified using the `pv-snippets`
>>> variable in s390x/Makefile, similar to the non-pv-snippets in
>>> 2f6fdb4a (s390x: snippets: Add snippet compilation, 2021-06-22)
>>>
>>> Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
>>
>> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>>
>> but see a comment below
>>
>>> ---
>>>    .gitignore          |  2 ++
>>>    configure           |  8 ++++++
>>>    lib/s390x/snippet.h |  7 +++++
>>>    s390x/Makefile      | 67 +++++++++++++++++++++++++++++++++++++--------
>>>    4 files changed, 73 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/.gitignore b/.gitignore
>>> index 3d5be622..28a197bf 100644
>>> --- a/.gitignore
>>> +++ b/.gitignore
>>> @@ -25,3 +25,5 @@ cscope.*
>>>    /api/dirty-log-perf
>>>    /s390x/*.bin
>>>    /s390x/snippets/*/*.gbin
>>> +/s390x/snippets/*/*.hdr
>>> +/s390x/snippets/*/*.*obj
>>> \ No newline at end of file
>>> diff --git a/configure b/configure
>>> index 1d4d855e..9210912f 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -26,6 +26,7 @@ target=
>>>    errata_force=0
>>>    erratatxt="$srcdir/errata.txt"
>>>    host_key_document=
>>> +gen_se_header=
>>>    page_size=
>>>    earlycon=
>>>    
>>> @@ -54,6 +55,9 @@ usage() {
>>>    	    --host-key-document=HOST_KEY_DOCUMENT
>>>    	                           Specify the machine-specific host-key document for creating
>>>    	                           a PVM image with 'genprotimg' (s390x only)
>>> +	    --gen-se-header=GEN_SE_HEADER
>>> +	                           Provide an executable to generate a PV header
>>> +				   requires --host-key-document. (s390x-snippets only)
>>>    	    --page-size=PAGE_SIZE
>>>    	                           Specify the page size (translation granule) (4k, 16k or
>>>    	                           64k, default is 64k, arm64 only)
>>> @@ -127,6 +131,9 @@ while [[ "$1" = -* ]]; do
>>>    	--host-key-document)
>>>    	    host_key_document="$arg"
>>>    	    ;;
>>> +	--gen-se-header)
>>> +	    gen_se_header="$arg"
>>> +	    ;;
>>>    	--page-size)
>>>    	    page_size="$arg"
>>>    	    ;;
>>> @@ -341,6 +348,7 @@ U32_LONG_FMT=$u32_long
>>>    WA_DIVIDE=$wa_divide
>>>    GENPROTIMG=${GENPROTIMG-genprotimg}
>>>    HOST_KEY_DOCUMENT=$host_key_document
>>> +GEN_SE_HEADER=$gen_se_header
>>>    EOF
>>>    if [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
>>>        echo "TARGET=$target" >> config.mak
>>> diff --git a/lib/s390x/snippet.h b/lib/s390x/snippet.h
>>> index 8e4765f8..6b77a8a9 100644
>>> --- a/lib/s390x/snippet.h
>>> +++ b/lib/s390x/snippet.h
>>> @@ -14,10 +14,17 @@
>>>    	_binary_s390x_snippets_##type##_##file##_gbin_start
>>>    #define SNIPPET_NAME_END(type, file) \
>>>    	_binary_s390x_snippets_##type##_##file##_gbin_end
>>> +#define SNIPPET_HDR_START(type, file) \
>>> +	_binary_s390x_snippets_##type##_##file##_hdr_start
>>> +#define SNIPPET_HDR_END(type, file) \
>>> +	_binary_s390x_snippets_##type##_##file##_hdr_end
>>> +
>>>    
>>>    /* Returns the length of the snippet */
>>>    #define SNIPPET_LEN(type, file) \
>>>    	((uintptr_t)SNIPPET_NAME_END(type, file) - (uintptr_t)SNIPPET_NAME_START(type, file))
>>> +#define SNIPPET_HDR_LEN(type, file) \
>>> +	((uintptr_t)SNIPPET_HDR_END(type, file) - (uintptr_t)SNIPPET_HDR_START(type, file))
>>>    
>>>    /*
>>>     * C snippet instructions start at 0x4000 due to the prefix and the
>>> diff --git a/s390x/Makefile b/s390x/Makefile
>>> index f95f2e61..55e6d962 100644
>>> --- a/s390x/Makefile
>>> +++ b/s390x/Makefile
>>> @@ -26,12 +26,20 @@ tests += $(TEST_DIR)/edat.elf
>>>    tests += $(TEST_DIR)/mvpg-sie.elf
>>>    tests += $(TEST_DIR)/spec_ex-sie.elf
>>>    
>>> +ifneq ($(HOST_KEY_DOCUMENT),)
>>> +ifneq ($(GEN_SE_HEADER),)
>>> +tests += $(pv-tests)
>>> +endif
>>> +endif
>>> +
>>>    tests_binary = $(patsubst %.elf,%.bin,$(tests))
>>>    ifneq ($(HOST_KEY_DOCUMENT),)
>>>    tests_pv_binary = $(patsubst %.bin,%.pv.bin,$(tests_binary))
>>>    else
>>>    tests_pv_binary =
>>> +GEN_SE_HEADER =
>>>    endif
>>> +snippets-obj = $(patsubst %.gbin,%.gobj,$(snippets))
>>>    
>>>    all: directories test_cases test_cases_binary test_cases_pv
>>>    
>>> @@ -82,26 +90,59 @@ asmlib = $(TEST_DIR)/cstart64.o $(TEST_DIR)/cpu.o
>>>    FLATLIBS = $(libcflat)
>>>    
>>>    SNIPPET_DIR = $(TEST_DIR)/snippets
>>> -snippet_asmlib = $(SNIPPET_DIR)/c/cstart.o lib/auxinfo.o
>>> +snippet_asmlib = $(SNIPPET_DIR)/c/cstart.o
>>> +snippet_lib = $(snippet_asmlib) lib/auxinfo.o
>>>    
>>>    # perquisites (=guests) for the snippet hosts.
>>>    # $(TEST_DIR)/<snippet-host>.elf: snippets = $(SNIPPET_DIR)/<c/asm>/<snippet>.gbin
>>>    $(TEST_DIR)/mvpg-sie.elf: snippets = $(SNIPPET_DIR)/c/mvpg-snippet.gbin
>>>    $(TEST_DIR)/spec_ex-sie.elf: snippets = $(SNIPPET_DIR)/c/spec_ex.gbin
>>>    
>>> -$(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o $(FLATLIBS)
>>> -	$(OBJCOPY) -O binary $(patsubst %.gbin,%.o,$@) $@
>>> -	$(OBJCOPY) -I binary -O elf64-s390 -B "s390:64-bit" $@ $@
>>> +ifneq ($(GEN_SE_HEADER),)
>>> +snippets += $(pv-snippets)
>>> +tests += $(pv-tests)
>>> +snippet-hdr-obj = $(patsubst %.gbin,%.hdr.obj,$(pv-snippets))
>>> +else
>>> +snippet-hdr-obj =
>>> +endif
>>> +
>>> +# the asm/c snippets %.o have additional generated files as dependencies
>>> +$(SNIPPET_DIR)/asm/%.o: $(SNIPPET_DIR)/asm/%.S $(asm-offsets)
>>> +	$(CC) $(CFLAGS) -c -nostdlib -o $@ $<
>>> +
>>> +$(SNIPPET_DIR)/c/%.o: $(SNIPPET_DIR)/c/%.c $(asm-offsets)
>>> +	$(CC) $(CFLAGS) -c -nostdlib -o $@ $<
>>> +
>>> +$(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o
>>> +	$(OBJCOPY) -O binary -j ".rodata" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $(patsubst %.gbin,%.o,$@) $@
>>> +	truncate -s '%4096' $@
>>> +
>>> +$(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS)
>>> +	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/c/flat.lds $(patsubst %.gbin,%.o,$@) $(snippet_lib) $(FLATLIBS)
>>> +	$(OBJCOPY) -O binary -j ".rodata" -j ".lowcore" -j ".text" -j ".data" -j ".bss" --set-section-flags .bss=alloc,load,contents $@ $@
>>> +	truncate -s '%4096' $@
>>> +
>>> +$(SNIPPET_DIR)/asm/%.hdr: $(SNIPPET_DIR)/asm/%.gbin $(HOST_KEY_DOCUMENT)
>>> +	$(GEN_SE_HEADER) -k $(HOST_KEY_DOCUMENT) -c $<,0x4000,0x00000000000000420000000000000000 --psw-addr 0x4000 -o $@
>>> +
>>> +$(SNIPPET_DIR)/c/%.hdr: $(SNIPPET_DIR)/c/%.gbin $(HOST_KEY_DOCUMENT)
>>> +	$(GEN_SE_HEADER) -k $(HOST_KEY_DOCUMENT) -c $<,0x0,0x00000000000000420000000000000000 --psw-addr 0x4000 -o $@
>>
>> are they supposed to have different addresses?
>> the C files start at 0x4000, while the asm ones at 0
> 
> That's a mistake I'll need to fix.

On second thought this is correct since it's the starting address of the 
component and not the PSW entry. The psw entry is the next argument.
The C snippets currently have data in the first 4 pages so we can 
encrypt from offset 0.

The question that remains is: do we need the data at 0x0 - 0x4000?
The reset and restart PSWs are not really necessary since we don't start 
the snippets as a lpar or in simulation where we use these PSWs.
The stackptr is just that, a ptr AFAIK so there shouldn't be data on 
0x3000 (but I'll look that up anyway).

> 
>>
>>> +
>>> +.SECONDARY:
>>> +%.gobj: %.gbin
>>> +	$(OBJCOPY) -I binary -O elf64-s390 -B "s390:64-bit" $< $@
>>> +
>>> +.SECONDARY:
>>> +%.hdr.obj: %.hdr
>>> +	$(OBJCOPY) -I binary -O elf64-s390 -B "s390:64-bit" $< $@
>>>    
>>> -$(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_asmlib) $(FLATLIBS)
>>> -	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/c/flat.lds $(patsubst %.gbin,%.o,$@) $(snippet_asmlib) $(FLATLIBS)
>>> -	$(OBJCOPY) -O binary $@ $@
>>> -	$(OBJCOPY) -I binary -O elf64-s390 -B "s390:64-bit" $@ $@
>>>    
>>>    .SECONDEXPANSION:
>>> -%.elf: $$(snippets) %.o $(FLATLIBS) $(SRCDIR)/s390x/flat.lds $(asmlib)
>>> +%.elf: $(FLATLIBS) $(asmlib) $(SRCDIR)/s390x/flat.lds $$(snippets-obj) $$(snippet-hdr-obj) %.o
>>>    	$(CC) $(CFLAGS) -c -o $(@:.elf=.aux.o) $(SRCDIR)/lib/auxinfo.c -DPROGNAME=\"$@\"
>>> -	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/flat.lds $(filter %.o, $^) $(FLATLIBS) $(snippets) $(@:.elf=.aux.o)
>>> +	@$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/flat.lds \
>>> +		$(filter %.o, $^) $(FLATLIBS) $(snippets-obj) $(snippet-hdr-obj) $(@:.elf=.aux.o) || \
>>> +		{ echo "Failure probably caused by missing definition of gen-se-header executable"; exit 1; }
>>>    	$(RM) $(@:.elf=.aux.o)
>>>    	@chmod a-x $@
>>>    
>>> @@ -114,8 +155,12 @@ $(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_asmlib) $(FLATLIBS)
>>>    %.pv.bin: %.bin $(HOST_KEY_DOCUMENT)
>>>    	$(GENPROTIMG) --host-key-document $(HOST_KEY_DOCUMENT) --no-verify --image $< -o $@
>>>    
>>> +$(snippet_asmlib): $$(patsubst %.o,%.S,$$@) $(asm-offsets)
>>> +	$(CC) $(CFLAGS) -c -nostdlib -o $@ $<
>>> +
>>> +
>>>    arch_clean: asm_offsets_clean
>>> -	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(TEST_DIR)/.*.d $(SNIPPET_DIR)/c/*.{o,gbin} $(SNIPPET_DIR)/c/.*.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
>>>    
>>>    generated-files = $(asm-offsets)
>>>    $(tests:.elf=.o) $(asmlib) $(cflatobjs): $(generated-files)
>>
>