Re: [kvm-unit-tests PATCH v3 2/2] s390x: SCLP unit test

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux