On 13/11/2019 13.40, Claudio Imbrenda wrote:
> On Wed, 13 Nov 2019 10:34:02 +0100
> Thomas Huth <thuth@xxxxxxxxxx> wrote:
[...]
>>> +/**
>>> + * Perform one test at the given address, optionally using the
>>> SCCB template,
>>
>> I think you should at least mention the meaning of the "len" parameter
>> here, otherwise this is rather confusing (see below, my comment to
>> sccb_template).
>
> I'll rename it and add comments
>
>>> + * checking for the expected program interrupts and return codes.
>>> + * Returns 1 in case of success or 0 in case of failure
>>
>> Could use bool with true + false instead.
>>
>>> + */
>>> +static int test_one_sccb(uint32_t cmd, uint8_t *addr, uint16_t
>>> len, uint64_t exp_pgm, uint16_t exp_rc) +{
>>> + SCCBHeader *h = (SCCBHeader *)addr;
>>> + int res, pgm;
>>> +
>>> + /* Copy the template to the test address if needed */
>>> + if (len)
>>> + memcpy(addr, sccb_template, len);
>>
>> Honestly, that sccb_template is rather confusing. Why does the caller
>> has to provide both, the data in the sccb_template and the "addr"
>> variable for yet another buffer? Wouldn't it be simpler if the caller
>> simply sets up everything in a place of choice and then only passes
>> the "addr" to the buffer?
>
> because you will test the same buffer at different addresses. this
> mechanism abstracts this. instead of having to clear the buffer and set
> the values for each address, you can simply set the template once and
> then call the same function, changing only the target address.
>
> also, the target address is not always a buffer, in many cases it is in
> fact an invalid address, which should generate exceptions.
Hmm, ok, I guess some additional comments like this in the source code
would be helpful.
>>> + expect_pgm_int();
>>> + res = sclp_service_call_test(cmd, h);
>>> + if (res) {
>>> + report_info("SCLP not ready (command %#x, address
>>> %p, cc %d)", cmd, addr, res);
>>> + return 0;
>>> + }
>>> + pgm = 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, size %d,
>>> cmd %#x, pgm code %d", addr, len, cmd, pgm);
>>> + return 0;
>>> + }
>>> + /* Check if the response code is the one expected */
>>> + if (exp_rc && (exp_rc != h->response_code)) {
>>
>> You can drop the parentheses around "exp_rc != h->response_code".
>
> fine, although I don't understand you hatred toward parentheses :)
I took a LISP class at university once ... never quite recovered from
that...
No, honestly, the problem is rather that these additional parentheses
slow me down when I read the source code. If I see such if-statements,
my brain starts to think something like "There are parentheses here, so
there must be some additional non-trivial logic in this if-statement...
let's try to understand that..." and it takes a second to realize that
it's not the case and the parentheses are just superfluous.
>>> +/**
>>> + * Test SCCBs whose address is in the lowcore or prefix area.
>>> + */
>>> +static void test_sccb_prefix(void)
>>> +{
>>> + uint32_t prefix, new_prefix;
>>> + int offset;
>>> +
>>> + /* can't actually trash the lowcore, unsurprisingly things
>>> break if we do */
>>> + for (offset = 0; offset < 8192; offset += 8)
>>> + if (!test_one_sccb(valid_code, MKPTR(offset), 0,
>>> PGM_BIT_SPEC, 0))
>>> + break;
>>> + report("SCCB low pages", offset == 8192);
>>> +
>>> + memcpy(prefix_buf, 0, 8192);
>>> + new_prefix = (uint32_t)(intptr_t)prefix_buf;
>>> + asm volatile("stpx %0" : "=Q" (prefix));
>>> + asm volatile("spx %0" : : "Q" (new_prefix) : "memory");
>>> +
>>> + for (offset = 0; offset < 8192; offset += 8)
>>> + if (!test_one_simple(valid_code, MKPTR(new_prefix
>>> + offset), 8, 8, PGM_BIT_SPEC, 0))
>>> + break;
>>> + report("SCCB prefix pages", offset == 8192);
>>> +
>>> + memcpy(prefix_buf, 0, 8192);
>>
>> What's that memcpy() good for? A comment would be helpful.
>
> we just moved the prefix to a temporary one, and thrashed the old one.
> we can't simply set the old prefix and call it a day, things will break.
Did the test really trash the old one? ... hmm, I guess I got the code
wrong, that prefix addressing is always so confusing. Is SCLP working
with absolute or real addresses?
Thomas
