Re: [kvm-unit-tests RFC 1/2] s390x: Add guest snippet support

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

On 6/21/21 3:28 PM, Thomas Huth wrote:
> On 21/06/2021 14.39, Janosch Frank wrote:
>> On 6/21/21 2:32 PM, Thomas Huth wrote:
>>> On 21/06/2021 14.19, Janosch Frank wrote:
>>>> On 6/21/21 12:10 PM, Thomas Huth wrote:
>>>>> On 20/05/2021 11.47, Janosch Frank wrote:
>>>>>> Snippets can be used to easily write and run guest (SIE) tests.
>>>>>> The snippet is linked into the test binaries and can therefore be
>>>>>> accessed via a ptr.
>>>>>>
>>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>>> ---
>>>>>>     .gitignore                |  2 ++
>>>>>>     s390x/Makefile            | 28 ++++++++++++++++++---
>>>>>>     s390x/snippets/c/cstart.S | 13 ++++++++++
>>>>>>     s390x/snippets/c/flat.lds | 51 +++++++++++++++++++++++++++++++++++++++
>>>>>>     4 files changed, 91 insertions(+), 3 deletions(-)
>>>>>>     create mode 100644 s390x/snippets/c/cstart.S
>>>>>>     create mode 100644 s390x/snippets/c/flat.lds
>>>>>>
>>>>>> diff --git a/.gitignore b/.gitignore
>>>>>> index 784cb2dd..29d3635b 100644
>>>>>> --- a/.gitignore
>>>>>> +++ b/.gitignore
>>>>>> @@ -22,3 +22,5 @@ cscope.*
>>>>>>     /api/dirty-log
>>>>>>     /api/dirty-log-perf
>>>>>>     /s390x/*.bin
>>>>>> +/s390x/snippets/*/*.bin
>>>>>> +/s390x/snippets/*/*.gbin
>>>>>> diff --git a/s390x/Makefile b/s390x/Makefile
>>>>>> index 8de926ab..fe267011 100644
>>>>>> --- a/s390x/Makefile
>>>>>> +++ b/s390x/Makefile
>>>>>> @@ -75,11 +75,33 @@ OBJDIRS += lib/s390x
>>>>>>     asmlib = $(TEST_DIR)/cstart64.o $(TEST_DIR)/cpu.o
>>>>>>     
>>>>>>     FLATLIBS = $(libcflat)
>>>>>> -%.elf: %.o $(FLATLIBS) $(SRCDIR)/s390x/flat.lds $(asmlib)
>>>>>> +
>>>>>> +SNIPPET_DIR = $(TEST_DIR)/snippets
>>>>>> +
>>>>>> +# C snippets that need to be linked
>>>>>> +snippets-c =
>>>>>> +
>>>>>> +# ASM snippets that are directly compiled and converted to a *.gbin
>>>>>> +snippets-a =
>>>>>
>>>>> Could you please call this snippets-s instead of ...-a ? The -a suffix looks
>>>>> like an archive to me otherwise.
>>>>
>>>> Sure
>>>>
>>>>>
>>>>>> +snippets = $(snippets-a)$(snippets-c)
>>>>>
>>>>> Shouldn't there be a space between the two?
>>>>
>>>> Yes, already fixed that a long while ago
>>>> I thought I had sent out a new version already, maybe that was an
>>>> illusion as I can't seem to find it right now.
>>>>
>>>>>
>>>>>> +snippets-o += $(patsubst %.gbin,%.o,$(snippets))
>>>>>> +
>>>>>> +$(snippets-a): $(snippets-o) $(FLATLIBS)
>>>>>> +	$(OBJCOPY) -O binary $(patsubst %.gbin,%.o,$@) $@
>>>>>> +	$(OBJCOPY) -I binary -O elf64-s390 -B "s390:64-bit" $@ $@
>>>>>> +
>>>>>> +$(snippets-c): $(snippets-o) $(SNIPPET_DIR)/c/cstart.o  $(FLATLIBS)
>>>>>> +	$(CC) $(LDFLAGS) -o $@ -T $(SNIPPET_DIR)/c/flat.lds \
>>>>>> +		$(filter %.o, $^) $(FLATLIBS)
>>>>>> +	$(OBJCOPY) -O binary $@ $@
>>>>>> +	$(OBJCOPY) -I binary -O elf64-s390 -B "s390:64-bit" $@ $@
>>>>>> +
>>>>>> +%.elf: $(snippets) %.o $(FLATLIBS) $(SRCDIR)/s390x/flat.lds $(asmlib)
>>>>>>     	$(CC) $(CFLAGS) -c -o $(@:.elf=.aux.o) \
>>>>>>     		$(SRCDIR)/lib/auxinfo.c -DPROGNAME=\"$@\"
>>>>>>     	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/flat.lds \
>>>>>> -		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
>>>>>> +		$(filter %.o, $^) $(FLATLIBS) $(snippets) $(@:.elf=.aux.o)
>>>>>
>>>>> Does this link the snippets into all elf files? ... wouldn't it be better to
>>>>> restrict it somehow to the files that really need them?
>>>>
>>>> Yes it does.
>>>> I'd like to avoid having to specify a makefile rule for every test that
>>>> uses snippets as we already have more than the mvpg one in the queue.
>>>>
>>>> So I'm having Steffen looking into a solution for this problem. My first
>>>> idea was to bring the used snippets into the unittests.cfg but I
>>>> disliked that we then would have compile instructions in another file.
>>>> Maybe there's a way to include that into the makefile in a clever way?
>>>
>>> I haven't tried, but maybe you could replace the $(snippets) in the last
>>> line with
>>>
>>>    $(wildcard snippets/$@.gbin)
>>>
>>> or something similar?
>>
>> That starts falling apart when multiple tests use the same snippet, no?
> 
> That's true ... Maybe something like:
> 
>   $(filter %.gbin,$^)

That filters all files that are not gbins from the prereqs, right?
In what way is that different to linking all the snippets? After all
they are currently a prereq for %.elf or am I missing something here?

Just to expand our use-case a bit more with an example since the code is
still too rough to post right now:

We test diagnose handling for Secure Execution / PV in one host test
where we have a gbin each for most diagnose codes 0x44, 0x9c, 0x288,
0x500 (0x308 needs a whole test and possible multiple gbins on its own
due to its complexity).
Other test bundles are PV IO, misc (stsi, sthyi, ...) and of course
other VSIE instruction handling tests in the future.

> 
> ?
> 
>   Thomas
>