| Message ID | 20220729082633.277240-4-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: PV fixups | expand |
On 7/29/22 10:26, Janosch Frank wrote: > A linker script has a few benefits: > - We can easily define a lowcore and load the snippet from 0x0 instead > of 0x4000 which makes asm snippets behave like c snippets > - We can easily define an invalid PGM new PSW to ensure an exit on a > guest PGM > > As we gain another step and file extension by linking, a comment > explains which file extensions are generated and why. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > lib/s390x/snippet.h | 3 +-- > s390x/Makefile | 16 +++++++++++++--- > s390x/mvpg-sie.c | 2 +- > s390x/pv-diags.c | 6 +++--- > 4 files changed, 18 insertions(+), 9 deletions(-) > > diff --git a/lib/s390x/snippet.h b/lib/s390x/snippet.h > index b17b2a4c..57045994 100644 > --- a/lib/s390x/snippet.h > +++ b/lib/s390x/snippet.h > @@ -32,8 +32,7 @@ > > #define SNIPPET_PV_TWEAK0 0x42UL > #define SNIPPET_PV_TWEAK1 0UL > -#define SNIPPET_OFF_C 0 > -#define SNIPPET_OFF_ASM 0x4000 > +#define SNIPPET_UNPACK_OFF 0 > > > /* > diff --git a/s390x/Makefile b/s390x/Makefile > index ee34a1d7..9a0b48e2 100644 > --- a/s390x/Makefile > +++ b/s390x/Makefile > @@ -122,6 +122,13 @@ else > snippet-hdr-obj = > endif > > +# Each snippet will generate the following files (in order): \ > + *.o is a snippet that has been compiled \ > + *.ol is a snippet that has been linked \ > + *.gbin is a snippet that has been converted to binary \ > + *.gobj is the final format after converting the binary into a elf file again, \ > + it will be linked into the tests > + > # 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 $@ $< > @@ -129,8 +136,11 @@ $(SNIPPET_DIR)/asm/%.o: $(SNIPPET_DIR)/asm/%.S $(asm-offsets) > $(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,$@) $@ > +$(SNIPPET_DIR)/asm/%.ol: $(SNIPPET_DIR)/asm/%.o > + $(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/asm/flat.lds $< I think you forgot to include that linker script in the patch. Or did I miss anything? you can also use the $(SNIPPET_DIR) variable instead of searching for that directory yourself. > + > +$(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.ol > + $(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)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS) > @@ -139,7 +149,7 @@ $(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS) > 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 $@ > + $(GEN_SE_HEADER) -k $(HOST_KEY_DOCUMENT) -c $<,0x0,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 $@ > diff --git a/s390x/mvpg-sie.c b/s390x/mvpg-sie.c > index 46a2edb6..99f4859b 100644 > --- a/s390x/mvpg-sie.c > +++ b/s390x/mvpg-sie.c > @@ -87,7 +87,7 @@ static void setup_guest(void) > > snippet_setup_guest(&vm, false); > snippet_init(&vm, SNIPPET_NAME_START(c, mvpg_snippet), > - SNIPPET_LEN(c, mvpg_snippet), SNIPPET_OFF_C); > + SNIPPET_LEN(c, mvpg_snippet), SNIPPET_UNPACK_OFF); > > /* Enable MVPG interpretation as we want to test KVM and not ourselves */ > vm.sblk->eca = ECA_MVPGI; > diff --git a/s390x/pv-diags.c b/s390x/pv-diags.c > index 9ced68c7..5165937a 100644 > --- a/s390x/pv-diags.c > +++ b/s390x/pv-diags.c > @@ -28,7 +28,7 @@ static void test_diag_500(void) > > snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_diag_500), > SNIPPET_HDR_START(asm, snippet_pv_diag_500), > - size_gbin, size_hdr, SNIPPET_OFF_ASM); > + size_gbin, size_hdr, SNIPPET_UNPACK_OFF); > > sie(&vm); > report(vm.sblk->icptcode == ICPT_PV_INSTR && vm.sblk->ipa == 0x8302 && > @@ -83,7 +83,7 @@ static void test_diag_288(void) > > snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_diag_288), > SNIPPET_HDR_START(asm, snippet_pv_diag_288), > - size_gbin, size_hdr, SNIPPET_OFF_ASM); > + size_gbin, size_hdr, SNIPPET_UNPACK_OFF); > > sie(&vm); > report(vm.sblk->icptcode == ICPT_PV_INSTR && vm.sblk->ipa == 0x8302 && > @@ -124,7 +124,7 @@ static void test_diag_yield(void) > > snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_diag_yield), > SNIPPET_HDR_START(asm, snippet_pv_diag_yield), > - size_gbin, size_hdr, SNIPPET_OFF_ASM); > + size_gbin, size_hdr, SNIPPET_UNPACK_OFF); > > /* 0x44 */ > report_prefix_push("0x44"); Steffen Eiden
On 7/29/22 16:17, Steffen Eiden wrote: > > > On 7/29/22 10:26, Janosch Frank wrote: >> A linker script has a few benefits: >> - We can easily define a lowcore and load the snippet from 0x0 instead >> of 0x4000 which makes asm snippets behave like c snippets >> - We can easily define an invalid PGM new PSW to ensure an exit on a >> guest PGM >> >> As we gain another step and file extension by linking, a comment >> explains which file extensions are generated and why. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> lib/s390x/snippet.h | 3 +-- >> s390x/Makefile | 16 +++++++++++++--- >> s390x/mvpg-sie.c | 2 +- >> s390x/pv-diags.c | 6 +++--- >> 4 files changed, 18 insertions(+), 9 deletions(-) >> >> diff --git a/lib/s390x/snippet.h b/lib/s390x/snippet.h >> index b17b2a4c..57045994 100644 >> --- a/lib/s390x/snippet.h >> +++ b/lib/s390x/snippet.h >> @@ -32,8 +32,7 @@ >> >> #define SNIPPET_PV_TWEAK0 0x42UL >> #define SNIPPET_PV_TWEAK1 0UL >> -#define SNIPPET_OFF_C 0 >> -#define SNIPPET_OFF_ASM 0x4000 >> +#define SNIPPET_UNPACK_OFF 0 >> >> >> /* >> diff --git a/s390x/Makefile b/s390x/Makefile >> index ee34a1d7..9a0b48e2 100644 >> --- a/s390x/Makefile >> +++ b/s390x/Makefile >> @@ -122,6 +122,13 @@ else >> snippet-hdr-obj = >> endif >> >> +# Each snippet will generate the following files (in order): \ >> + *.o is a snippet that has been compiled \ >> + *.ol is a snippet that has been linked \ >> + *.gbin is a snippet that has been converted to binary \ >> + *.gobj is the final format after converting the binary into a elf file again, \ >> + it will be linked into the tests >> + >> # 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 $@ $< >> @@ -129,8 +136,11 @@ $(SNIPPET_DIR)/asm/%.o: $(SNIPPET_DIR)/asm/%.S $(asm-offsets) >> $(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,$@) $@ >> +$(SNIPPET_DIR)/asm/%.ol: $(SNIPPET_DIR)/asm/%.o >> + $(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/asm/flat.lds $< > > I think you forgot to include that linker script in the patch. > Or did I miss anything? > > you can also use the $(SNIPPET_DIR) variable instead of searching for > that directory yourself. Right on both cases, I'll fix that... > >> + >> +$(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.ol >> + $(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)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS) >> @@ -139,7 +149,7 @@ $(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS) >> 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 $@ >> + $(GEN_SE_HEADER) -k $(HOST_KEY_DOCUMENT) -c $<,0x0,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 $@ >> diff --git a/s390x/mvpg-sie.c b/s390x/mvpg-sie.c >> index 46a2edb6..99f4859b 100644 >> --- a/s390x/mvpg-sie.c >> +++ b/s390x/mvpg-sie.c >> @@ -87,7 +87,7 @@ static void setup_guest(void) >> >> snippet_setup_guest(&vm, false); >> snippet_init(&vm, SNIPPET_NAME_START(c, mvpg_snippet), >> - SNIPPET_LEN(c, mvpg_snippet), SNIPPET_OFF_C); >> + SNIPPET_LEN(c, mvpg_snippet), SNIPPET_UNPACK_OFF); >> >> /* Enable MVPG interpretation as we want to test KVM and not ourselves */ >> vm.sblk->eca = ECA_MVPGI; >> diff --git a/s390x/pv-diags.c b/s390x/pv-diags.c >> index 9ced68c7..5165937a 100644 >> --- a/s390x/pv-diags.c >> +++ b/s390x/pv-diags.c >> @@ -28,7 +28,7 @@ static void test_diag_500(void) >> >> snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_diag_500), >> SNIPPET_HDR_START(asm, snippet_pv_diag_500), >> - size_gbin, size_hdr, SNIPPET_OFF_ASM); >> + size_gbin, size_hdr, SNIPPET_UNPACK_OFF); >> >> sie(&vm); >> report(vm.sblk->icptcode == ICPT_PV_INSTR && vm.sblk->ipa == 0x8302 && >> @@ -83,7 +83,7 @@ static void test_diag_288(void) >> >> snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_diag_288), >> SNIPPET_HDR_START(asm, snippet_pv_diag_288), >> - size_gbin, size_hdr, SNIPPET_OFF_ASM); >> + size_gbin, size_hdr, SNIPPET_UNPACK_OFF); >> >> sie(&vm); >> report(vm.sblk->icptcode == ICPT_PV_INSTR && vm.sblk->ipa == 0x8302 && >> @@ -124,7 +124,7 @@ static void test_diag_yield(void) >> >> snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_diag_yield), >> SNIPPET_HDR_START(asm, snippet_pv_diag_yield), >> - size_gbin, size_hdr, SNIPPET_OFF_ASM); >> + size_gbin, size_hdr, SNIPPET_UNPACK_OFF); >> >> /* 0x44 */ >> report_prefix_push("0x44"); > > Steffen Eiden
Quoting Janosch Frank (2022-07-29 10:26:30) [...] > diff --git a/s390x/pv-diags.c b/s390x/pv-diags.c > index 9ced68c7..5165937a 100644 > --- a/s390x/pv-diags.c > +++ b/s390x/pv-diags.c > @@ -28,7 +28,7 @@ static void test_diag_500(void) > > snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_diag_500), > SNIPPET_HDR_START(asm, snippet_pv_diag_500), > - size_gbin, size_hdr, SNIPPET_OFF_ASM); > + size_gbin, size_hdr, SNIPPET_UNPACK_OFF); Does it even make sense to pass SNIPPET_UNPACK_OFF? Since this is now the same for C and ASM we could completely get rid of this argument, couldn't we?
diff --git a/lib/s390x/snippet.h b/lib/s390x/snippet.h index b17b2a4c..57045994 100644 --- a/lib/s390x/snippet.h +++ b/lib/s390x/snippet.h @@ -32,8 +32,7 @@ #define SNIPPET_PV_TWEAK0 0x42UL #define SNIPPET_PV_TWEAK1 0UL -#define SNIPPET_OFF_C 0 -#define SNIPPET_OFF_ASM 0x4000 +#define SNIPPET_UNPACK_OFF 0 /* diff --git a/s390x/Makefile b/s390x/Makefile index ee34a1d7..9a0b48e2 100644 --- a/s390x/Makefile +++ b/s390x/Makefile @@ -122,6 +122,13 @@ else snippet-hdr-obj = endif +# Each snippet will generate the following files (in order): \ + *.o is a snippet that has been compiled \ + *.ol is a snippet that has been linked \ + *.gbin is a snippet that has been converted to binary \ + *.gobj is the final format after converting the binary into a elf file again, \ + it will be linked into the tests + # 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 $@ $< @@ -129,8 +136,11 @@ $(SNIPPET_DIR)/asm/%.o: $(SNIPPET_DIR)/asm/%.S $(asm-offsets) $(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,$@) $@ +$(SNIPPET_DIR)/asm/%.ol: $(SNIPPET_DIR)/asm/%.o + $(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/snippets/asm/flat.lds $< + +$(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.ol + $(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)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS) @@ -139,7 +149,7 @@ $(SNIPPET_DIR)/c/%.gbin: $(SNIPPET_DIR)/c/%.o $(snippet_lib) $(FLATLIBS) 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 $@ + $(GEN_SE_HEADER) -k $(HOST_KEY_DOCUMENT) -c $<,0x0,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 $@ diff --git a/s390x/mvpg-sie.c b/s390x/mvpg-sie.c index 46a2edb6..99f4859b 100644 --- a/s390x/mvpg-sie.c +++ b/s390x/mvpg-sie.c @@ -87,7 +87,7 @@ static void setup_guest(void) snippet_setup_guest(&vm, false); snippet_init(&vm, SNIPPET_NAME_START(c, mvpg_snippet), - SNIPPET_LEN(c, mvpg_snippet), SNIPPET_OFF_C); + SNIPPET_LEN(c, mvpg_snippet), SNIPPET_UNPACK_OFF); /* Enable MVPG interpretation as we want to test KVM and not ourselves */ vm.sblk->eca = ECA_MVPGI; diff --git a/s390x/pv-diags.c b/s390x/pv-diags.c index 9ced68c7..5165937a 100644 --- a/s390x/pv-diags.c +++ b/s390x/pv-diags.c @@ -28,7 +28,7 @@ static void test_diag_500(void) snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_diag_500), SNIPPET_HDR_START(asm, snippet_pv_diag_500), - size_gbin, size_hdr, SNIPPET_OFF_ASM); + size_gbin, size_hdr, SNIPPET_UNPACK_OFF); sie(&vm); report(vm.sblk->icptcode == ICPT_PV_INSTR && vm.sblk->ipa == 0x8302 && @@ -83,7 +83,7 @@ static void test_diag_288(void) snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_diag_288), SNIPPET_HDR_START(asm, snippet_pv_diag_288), - size_gbin, size_hdr, SNIPPET_OFF_ASM); + size_gbin, size_hdr, SNIPPET_UNPACK_OFF); sie(&vm); report(vm.sblk->icptcode == ICPT_PV_INSTR && vm.sblk->ipa == 0x8302 && @@ -124,7 +124,7 @@ static void test_diag_yield(void) snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_diag_yield), SNIPPET_HDR_START(asm, snippet_pv_diag_yield), - size_gbin, size_hdr, SNIPPET_OFF_ASM); + size_gbin, size_hdr, SNIPPET_UNPACK_OFF); /* 0x44 */ report_prefix_push("0x44");
A linker script has a few benefits: - We can easily define a lowcore and load the snippet from 0x0 instead of 0x4000 which makes asm snippets behave like c snippets - We can easily define an invalid PGM new PSW to ensure an exit on a guest PGM As we gain another step and file extension by linking, a comment explains which file extensions are generated and why. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- lib/s390x/snippet.h | 3 +-- s390x/Makefile | 16 +++++++++++++--- s390x/mvpg-sie.c | 2 +- s390x/pv-diags.c | 6 +++--- 4 files changed, 18 insertions(+), 9 deletions(-)