Re: [PATCH v3] powerpc: Add tests for sPAPR h-calls

[Thomas Huth <thuth@redhat.com>]

On 02.03.2016 11:07, Laurent Vivier wrote:
> 
> 
> On 02/03/2016 09:40, Thomas Huth wrote:
>> Introduce a test for sPAPR hypercalls, starting with the
>> three hypercalls H_SET_SPRG0, H_PAGE_INIT and H_RANDOM.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  v3:
>>  - Use report_xfail() in H_RANDOM test to report whether the
>>    h-call is available or not
>>
>>  v2:
>>  - Rebased to the final version of Andrew's initial ppc64
>>    support patches that got merged yesterday
>>  - Added a test for the H_RANDOM hypercall
>>
>>  Please note that you need the latest QEMU development version
>>  since some of the hypercalls have only been added recently.
>>
>>  In case somebody wants to review the description of the h-calls, you
>>  can find them in LoPAPR, chapters 14.5.4.3.1, 14.5.4.3.3 and 14.15.1.
>>  See: https://members.openpowerfoundation.org/document/dl/469
>>
>>  lib/powerpc/asm/hcall.h |   3 +
>>  powerpc/Makefile.common |   5 +-
>>  powerpc/spapr_hcall.c   | 167 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  powerpc/unittests.cfg   |   3 +
>>  4 files changed, 177 insertions(+), 1 deletion(-)
>>  create mode 100644 powerpc/spapr_hcall.c
>>
>> diff --git a/lib/powerpc/asm/hcall.h b/lib/powerpc/asm/hcall.h
>> index 750c655..f6f9ea8 100644
>> --- a/lib/powerpc/asm/hcall.h
>> +++ b/lib/powerpc/asm/hcall.h
>> @@ -15,8 +15,11 @@
>>  #define H_PRIVILEGE		-3
>>  #define H_PARAMETER		-4
>>  
>> +#define H_SET_SPRG0		0x24
>>  #define H_SET_DABR		0x28
>> +#define H_PAGE_INIT		0x2c
>>  #define H_PUT_TERM_CHAR		0x58
>> +#define H_RANDOM		0x300
>>  
>>  #ifndef __ASSEMBLY__
>>  /*
>> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
>> index b526668..2ce6494 100644
>> --- a/powerpc/Makefile.common
>> +++ b/powerpc/Makefile.common
>> @@ -5,7 +5,8 @@
>>  #
>>  
>>  tests-common = \
>> -	$(TEST_DIR)/selftest.elf
>> +	$(TEST_DIR)/selftest.elf \
>> +	$(TEST_DIR)/spapr_hcall.elf
>>  
>>  all: $(TEST_DIR)/boot_rom.bin test_cases
>>  
>> @@ -63,3 +64,5 @@ generated_files = $(asm-offsets)
>>  test_cases: $(generated_files) $(tests-common) $(tests)
>>  
>>  $(TEST_DIR)/selftest.elf: $(cstart.o) $(reloc.o) $(TEST_DIR)/selftest.o
>> +
>> +$(TEST_DIR)/spapr_hcall.elf: $(cstart.o) $(reloc.o) $(TEST_DIR)/spapr_hcall.o
>> diff --git a/powerpc/spapr_hcall.c b/powerpc/spapr_hcall.c
>> new file mode 100644
>> index 0000000..46731e1
>> --- /dev/null
>> +++ b/powerpc/spapr_hcall.c
...
>> +static int h_random(uint64_t *val)
>> +{
>> +	register uint64_t r3 asm("r3") = H_RANDOM;
>> +	register uint64_t r4 asm("r4");
>> +
>> +	asm volatile (" sc 1 " : "+r"(r3), "=r"(r4) : "r"(r3));
> 
> as you use "+r" with r3 in the output operands field, I think you don't
> have to declare it in the input operands field.

Ah, ok, ... gcc inline assembly is always confusing...
Paolo, Andrew, shall I sent a new version for fixing this nit, or is it
ok to keep it in the current, slightly redundant shape?

>> +	*val = r4;
>> +
>> +	return r3;
>> +}
>> +
>> +/**
>> + * Test H_RANDOM by calling it a couple of times to check whether all bit
>> + * positions really toggle (there should be no "stuck" bits in the output)
>> + */
>> +static void test_h_random(int argc, char **argv)
>> +{
>> +	uint64_t rval, val0, val1;
>> +	int rc, i;
>> +
>> +	if (argc > 1)
>> +		report_abort("Unsupported argument: '%s'", argv[1]);
>> +
>> +	/* H_RANDOM is optional - so check for sane return values first */
>> +	rc = h_random(&rval);
>> +	report_xfail("h-call available", rc = H_FUNCTION, rc = H_SUCCESS);
>> +	if (rc != H_SUCCESS)
>> +		return;
...
> Is it possible to use "ibm,hypertasfunctions" of /rtas node to know if
> it is supported (instead of H_FUNCTION)?

Theoretically yes, but QEMU still does not provide this in that property
yet, so testing for H_FUNCTION is currently the better solution.

Adding the missing entries to the hypertas property is on my TODO list,
there are also a couple of other entries missing in that property, but I
just did not have time for that yet...

 Thomas