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 Wed, 13 Nov 2019 14:05:24 +0100
Thomas Huth <thuth@xxxxxxxxxx> wrote:

[...]

> Hmm, ok, I guess some additional comments like this in the source code
> would be helpful.

ok, I'll add plenty

> >>> +	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.

on the other hand it helps people who don't remember the C operator
precedence by heart :)

but yeah won't be in v4

> >>> +/**
> >>> + * 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))

here ^ (read below)

> >>> +			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

yes, the default prefix is 0 and we are writing at absolute 0 (see
above where exactly). the SCLP call *should* not succeed anyway, but we
need to test it

I'm reworking this function because it possibly doesn't test all cases.
I added plenty of comments there too to explain what's going on

> wrong, that prefix addressing is always so confusing. Is SCLP working
> with absolute or real addresses?

yes.

both lowcore (real 0, absolute $PREFIX<<13) and absolute 0 (real
$PREFIX<<13, absolute 0) are forbidden for SCLP, so real or absolute
does not make a difference.




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux