From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-2.mimecast.com ([207.211.31.81]:25877 "EHLO us-smtp-delivery-1.mimecast.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726049AbgAVLUo (ORCPT ); Wed, 22 Jan 2020 06:20:44 -0500 Subject: Re: [kvm-unit-tests PATCH v8 6/6] s390x: SCLP unit test From: David Hildenbrand References: <20200120184256.188698-1-imbrenda@linux.ibm.com> <20200120184256.188698-7-imbrenda@linux.ibm.com> <35e59971-c09e-2808-1be6-f2ccd555c4f6@redhat.com> <42c5b040-733d-4e5b-0276-5b94315336bb@redhat.com> <997a62b7-7ab7-6119-4948-e8779e639101@redhat.com> <4d09b567-c2ae-ec9d-59d0-bd259a86b14d@redhat.com> <946e1194-4607-c928-6d66-9e306dc1216a@redhat.com> Message-ID: <88ac3980-9ae5-24df-ca01-89a8b32b7b22@redhat.com> Date: Wed, 22 Jan 2020 12:20:35 +0100 MIME-Version: 1.0 In-Reply-To: <946e1194-4607-c928-6d66-9e306dc1216a@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Sender: linux-s390-owner@vger.kernel.org List-ID: To: Thomas Huth , Claudio Imbrenda , kvm@vger.kernel.org Cc: linux-s390@vger.kernel.org, borntraeger@de.ibm.com, frankja@linux.ibm.com On 22.01.20 11:40, David Hildenbrand wrote: > On 22.01.20 11:39, Thomas Huth wrote: >> On 22/01/2020 11.32, David Hildenbrand wrote: >>> On 22.01.20 11:31, Thomas Huth wrote: >>>> On 22/01/2020 11.22, David Hildenbrand wrote: >>>>> On 22.01.20 11:10, David Hildenbrand wrote: >>>>>> On 20.01.20 19:42, Claudio Imbrenda wrote: >>>>>>> SCLP unit test. Testing the following: >>>>>>> >>>>>>> * Correctly ignoring instruction bits that should be ignored >>>>>>> * Privileged instruction check >>>>>>> * Check for addressing exceptions >>>>>>> * Specification exceptions: >>>>>>> - SCCB size less than 8 >>>>>>> - SCCB unaligned >>>>>>> - SCCB overlaps prefix or lowcore >>>>>>> - SCCB address higher than 2GB >>>>>>> * Return codes for >>>>>>> - Invalid command >>>>>>> - SCCB too short (but at least 8) >>>>>>> - SCCB page boundary violation >>>>>>> >>>>>>> Signed-off-by: Claudio Imbrenda >>>>>>> Reviewed-by: Thomas Huth >>>>>>> Acked-by: Janosch Frank >>>>>>> --- >>>>>>> s390x/Makefile | 1 + >>>>>>> s390x/sclp.c | 474 ++++++++++++++++++++++++++++++++++++++= ++++++ >>>>>>> s390x/unittests.cfg | 8 + >>>>>>> 3 files changed, 483 insertions(+) >>>>>>> create mode 100644 s390x/sclp.c >>>>>>> >>>>>>> diff --git a/s390x/Makefile b/s390x/Makefile >>>>>>> index 3744372..ddb4b48 100644 >>>>>>> --- a/s390x/Makefile >>>>>>> +++ b/s390x/Makefile >>>>>>> @@ -16,6 +16,7 @@ tests +=3D $(TEST_DIR)/diag288.elf >>>>>>> tests +=3D $(TEST_DIR)/stsi.elf >>>>>>> tests +=3D $(TEST_DIR)/skrf.elf >>>>>>> tests +=3D $(TEST_DIR)/smp.elf >>>>>>> +tests +=3D $(TEST_DIR)/sclp.elf >>>>>>> tests_binary =3D $(patsubst %.elf,%.bin,$(tests)) >>>>>>> =20 >>>>>>> all: directories test_cases test_cases_binary >>>>>>> diff --git a/s390x/sclp.c b/s390x/sclp.c >>>>>>> new file mode 100644 >>>>>>> index 0000000..215347e >>>>>>> --- /dev/null >>>>>>> +++ b/s390x/sclp.c >>>>>>> @@ -0,0 +1,474 @@ >>>>>>> +/* >>>>>>> + * Service Call tests >>>>>>> + * >>>>>>> + * Copyright (c) 2019 IBM Corp >>>>>>> + * >>>>>>> + * Authors: >>>>>>> + * Claudio Imbrenda >>>>>>> + * >>>>>>> + * This code is free software; you can redistribute it and/or mo= dify it >>>>>>> + * under the terms of the GNU General Public License version 2. >>>>>>> + */ >>>>>>> + >>>>>>> +#include >>>>>>> +#include >>>>>>> +#include >>>>>>> +#include >>>>>>> +#include >>>>>>> + >>>>>>> +#define PGM_NONE 1 >>>>>>> +#define PGM_BIT_SPEC (1ULL << PGM_INT_CODE_SPECIFICATION) >>>>>>> +#define PGM_BIT_ADDR (1ULL << PGM_INT_CODE_ADDRESSING) >>>>>>> +#define PGM_BIT_PRIV (1ULL << PGM_INT_CODE_PRIVILEGED_OPERATION) >>>>>>> +#define MKPTR(x) ((void *)(uint64_t)(x)) >>>>>>> + >>>>>>> +#define LC_SIZE (2 * PAGE_SIZE) >>>>>>> + >>>>>>> +static uint8_t pagebuf[LC_SIZE] __attribute__((aligned(LC_SIZE))= ); /* scratch pages used for some tests */ >>>>>>> +static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZ= E))); /* temporary lowcore for test_sccb_prefix */ >>>>>>> +static uint8_t sccb_template[PAGE_SIZE]; /* SCCB template to = be used */ >>>>>>> +static uint32_t valid_code; /* valid command code for READ = SCP INFO */ >>>>>>> +static struct lowcore *lc; >>>>>>> + >>>>>>> +/** >>>>>>> + * Perform one service call, handling exceptions and interrupts. >>>>>>> + */ >>>>>>> +static int sclp_service_call_test(unsigned int command, void *sc= cb) >>>>>>> +{ >>>>>>> + int cc; >>>>>>> + >>>>>>> + sclp_mark_busy(); >>>>>>> + sclp_setup_int(); >>>>>>> + cc =3D servc(command, __pa(sccb)); >>>>>>> + if (lc->pgm_int_code) { >>>>>>> + sclp_handle_ext(); >>>>>>> + return 0; >>>>>>> + } >>>>>>> + if (!cc) >>>>>>> + sclp_wait_busy(); >>>>>>> + return cc; >>>>>>> +} >>>>>>> + >>>>>>> +/** >>>>>>> + * Perform one test at the given address, optionally using the S= CCB template, >>>>>>> + * checking for the expected program interrupts and return codes= . >>>>>>> + * >>>>>>> + * The parameter buf_len indicates the number of bytes of the te= mplate that >>>>>>> + * should be copied to the test address, and should be 0 when th= e test >>>>>>> + * address is invalid, in which case nothing is copied. >>>>>>> + * >>>>>>> + * The template is used to simplify tests where the same buffer = content is >>>>>>> + * used many times in a row, at different addresses. >>>>>>> + * >>>>>>> + * Returns true in case of success or false in case of failure >>>>>>> + */ >>>>>>> +static bool test_one_sccb(uint32_t cmd, uint8_t *addr, uint16_t = buf_len, uint64_t exp_pgm, uint16_t exp_rc) >>>>>>> +{ >>>>>>> + SCCBHeader *h =3D (SCCBHeader *)addr; >>>>>>> + int res, pgm; >>>>>>> + >>>>>>> + /* Copy the template to the test address if needed */ >>>>>>> + if (buf_len) >>>>>>> + memcpy(addr, sccb_template, buf_len); >>>>>>> + if (exp_pgm !=3D PGM_NONE) >>>>>>> + expect_pgm_int(); >>>>>>> + /* perform the actual call */ >>>>>>> + res =3D sclp_service_call_test(cmd, h); >>>>>>> + if (res) { >>>>>>> + report_info("SCLP not ready (command %#x, address %p, cc %d)",= cmd, addr, res); >>>>>>> + return false; >>>>>>> + } >>>>>>> + pgm =3D clear_pgm_int(); >>>>>>> + /* Check if the program exception was one of the expected ones = */ >>>>>>> + if (!((1ULL << pgm) & exp_pgm)) { >>>>>>> + report_info("First failure at addr %p, buf_len %d, cmd %#x, pg= m code %d", >>>>>>> + addr, buf_len, cmd, pgm); >>>>>>> + return false; >>>>>>> + } >>>>>>> + /* Check if the response code is the one expected */ >>>>>>> + if (exp_rc && exp_rc !=3D h->response_code) { >>>>>>> + report_info("First failure at addr %p, buf_len %d, cmd %#x, re= sp code %#x", >>>>>>> + addr, buf_len, cmd, h->response_code); >>>>>>> + return false; >>>>>>> + } >>>>>>> + return true; >>>>>>> +} >>>>>>> + >>>>>>> +/** >>>>>>> + * Wrapper for test_one_sccb to be used when the template should= not be >>>>>>> + * copied and the memory address should not be touched. >>>>>>> + */ >>>>>>> +static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t ex= p_pgm, uint16_t exp_rc) >>>>>>> +{ >>>>>>> + return test_one_sccb(cmd, addr, 0, exp_pgm, exp_rc); >>>>>>> +} >>>>>>> + >>>>>>> +/** >>>>>>> + * Wrapper for test_one_sccb to set up a simple SCCB template. >>>>>>> + * >>>>>>> + * The parameter sccb_len indicates the value that will be saved= in the SCCB >>>>>>> + * length field of the SCCB, buf_len indicates the number of byt= es of >>>>>>> + * template that need to be copied to the actual test address. I= n many cases >>>>>>> + * it's enough to clear/copy the first 8 bytes of the buffer, wh= ile the SCCB >>>>>>> + * itself can be larger. >>>>>>> + * >>>>>>> + * Returns true in case of success or false in case of failure >>>>>>> + */ >>>>>>> +static bool test_one_simple(uint32_t cmd, uint8_t *addr, uint16_= t sccb_len, >>>>>>> + uint16_t buf_len, uint64_t exp_pgm, uint16_t exp_rc) >>>>>>> +{ >>>>>>> + memset(sccb_template, 0, sizeof(sccb_template)); >>>>>>> + ((SCCBHeader *)sccb_template)->length =3D sccb_len; >>>>>>> + return test_one_sccb(cmd, addr, buf_len, exp_pgm, exp_rc); >>>>>> >>>>>> Doing a fresh ./configure + make on RHEL7 gives me >>>>>> >>>>>> [linux1@rhkvm01 kvm-unit-tests]$ make >>>>>> gcc -std=3Dgnu99 -ffreestanding -I /home/linux1/git/kvm-unit-test= s/lib -I /home/linux1/git/kvm-unit-tests/lib/s390x -I lib -O2 -march=3DzE= C12 -fno-delete-null-pointer-checks -g -MMD -MF s390x/.sclp.d -Wall -Wwri= te-strings -Wempty-body -Wuninitialized -Wignored-qualifiers -Werror -fo= mit-frame-pointer -Wno-frame-address -fno-pic -Wclobbered -Wunus= ed-but-set-parameter -Wmissing-parameter-type -Wold-style-declaration -= Woverride-init -Wmissing-prototypes -Wstrict-prototypes -c -o s390x/scl= p.o s390x/sclp.c >>>>>> s390x/sclp.c: In function 'test_one_simple': >>>>>> s390x/sclp.c:121:2: error: dereferencing type-punned pointer will = break strict-aliasing rules [-Werror=3Dstrict-aliasing] >>>>>> ((SCCBHeader *)sccb_template)->length =3D sccb_len; >>>>>> ^ >>>>>> s390x/sclp.c: At top level: >>>>>> cc1: error: unrecognized command line option "-Wno-frame-address" = [-Werror] >>>>>> cc1: all warnings being treated as errors >>>>>> make: *** [s390x/sclp.o] Error 1 >>>>> >>>>> The following makes it work: >>>>> >>>>> >>>>> diff --git a/s390x/sclp.c b/s390x/sclp.c >>>>> index c13fa60..0b8117a 100644 >>>>> --- a/s390x/sclp.c >>>>> +++ b/s390x/sclp.c >>>>> @@ -117,8 +117,10 @@ static bool test_one_ro(uint32_t cmd, uint8_t = *addr, uint64_t exp_pgm, uint16_t >>>>> static bool test_one_simple(uint32_t cmd, uint8_t *addr, uint16_t = sccb_len, >>>>> uint16_t buf_len, uint64_t exp_pgm, uint16_= t exp_rc) >>>>> { >>>>> + SCCBHeader *header =3D (void *)sccb_template; >>>>> + >>>>> memset(sccb_template, 0, sizeof(sccb_template)); >>>>> - ((SCCBHeader *)sccb_template)->length =3D sccb_len; >>>>> + header->length =3D sccb_len; >>>> >>>> While that might silence the compiler warning, we still might get >>>> aliasing problems here, I think. >>>> The right way to solve this problem is to turn sccb_template into a >>>> union of the various structs/arrays that you want to use and then ac= cess >>>> the fields through the union instead ("type-punning through union"). >>> >>> We do have the exact same thing in lib/s390x/sclp.c already, no? >> >> Maybe we should carefully check that code, too... >> >>> Especially, new compilers don't seem to care? >> >> I've seen horrible bugs due to these aliasing problems in the past - >> without compiler warnings showing up! Certain versions of GCC assume >> that they can re-order code with pointers that point to types of >> different sizes, i.e. in the above example, I think they could assume >> that they could re-order the memset() and the header->length =3D ... l= ine. >> I'd feel better if we play safe and use a union here. >=20 > Should we simply allow type-punning? For now I squashed diff --git a/s390x/sclp.c b/s390x/sclp.c index c13fa60..7d92bf3 100644 --- a/s390x/sclp.c +++ b/s390x/sclp.c @@ -26,7 +26,12 @@ static uint8_t pagebuf[LC_SIZE] __attribute__((aligned(LC_SIZE))); /* scratch pages used for some tests */ static uint8_t prefix_buf[LC_SIZE] __attribute__((aligned(LC_SIZE))); /* temporary lowcore for test_sccb_prefix */ -static uint8_t sccb_template[PAGE_SIZE]; /* SCCB template to be used = */ +/* SCCB template to be used */ +static union { + uint8_t raw[PAGE_SIZE]; + SCCBHeader header; + WriteEventData data; +} sccb_template; static uint32_t valid_code; /* valid command code for READ SCP INFO= */ static struct lowcore *lc; @@ -69,7 +74,7 @@ static bool test_one_sccb(uint32_t cmd, uint8_t *addr, uint16_t buf_len, uint64_ /* Copy the template to the test address if needed */ if (buf_len) - memcpy(addr, sccb_template, buf_len); + memcpy(addr, sccb_template.raw, buf_len); if (exp_pgm !=3D PGM_NONE) expect_pgm_int(); /* perform the actual call */ @@ -117,8 +122,8 @@ static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t static bool test_one_simple(uint32_t cmd, uint8_t *addr, uint16_t sccb_l= en, uint16_t buf_len, uint64_t exp_pgm, uint16_t exp_rc) { - memset(sccb_template, 0, sizeof(sccb_template)); - ((SCCBHeader *)sccb_template)->length =3D sccb_len; + memset(sccb_template.raw, 0, sizeof(sccb_template.raw)); + sccb_template.header.length =3D sccb_len; return test_one_sccb(cmd, addr, buf_len, exp_pgm, exp_rc); } @@ -299,10 +304,10 @@ static void test_boundary(void) { const uint32_t cmd =3D SCLP_CMD_WRITE_EVENT_DATA; const uint16_t res =3D SCLP_RC_SCCB_BOUNDARY_VIOLATION; - WriteEventData *sccb =3D (WriteEventData *)sccb_template; + WriteEventData *sccb =3D &sccb_template.data; int len, offset; - memset(sccb_template, 0, sizeof(sccb_template)); + memset(sccb_template.raw, 0, sizeof(sccb_template.raw)); sccb->h.function_code =3D SCLP_FC_NORMAL_WRITE; for (len =3D 32; len <=3D 4096; len++) { offset =3D len & 7 ? len & ~7 : len - 8; @@ -323,10 +328,10 @@ static void test_toolong(void) { const uint32_t cmd =3D SCLP_CMD_WRITE_EVENT_DATA; const uint16_t res =3D SCLP_RC_SCCB_BOUNDARY_VIOLATION; - WriteEventData *sccb =3D (WriteEventData *)sccb_template; + WriteEventData *sccb =3D &sccb_template.data; int len; - memset(sccb_template, 0, sizeof(sccb_template)); + memset(sccb_template.raw, 0, sizeof(sccb_template.raw)); sccb->h.function_code =3D SCLP_FC_NORMAL_WRITE; for (len =3D 4097; len < 8192; len++) { sccb->h.length =3D len; --=20 2.24.1 --=20 Thanks, David / dhildenb