Re: [kvm-unit-tests PATCH v1 2/2] s390x: firq: floating interrupt test

[David Hildenbrand <david@redhat.com>]

>> +static void wait_for_sclp_int(void)
>> +{
>> +	/* Enable SCLP interrupts on this CPU only. */
>> +	ctl_set_bit(0, CTL0_SERVICE_SIGNAL);
>> +
>> +	set_flag(1);
> 
> why not just WRITE_ONCE/READ_ONCE?

Because I shamelessly copied that from s390x/smp.c ;)

>> +	set_flag(0);
>> +
>> +	/* Start CPU #1 and let it wait for the interrupt. */
>> +	psw.mask = extract_psw_mask();
>> +	psw.addr = (unsigned long)wait_for_sclp_int;
>> +	ret = smp_cpu_setup(1, psw);
>> +	if (ret) {
>> +		report_skip("cpu #1 not found");
> 
> ...which means that this will hang, and so will all the other report*
> functions. maybe you should manually unset the flag before calling the
> various report* functions.

Good point, thanks!

> 
>> +		goto out;
>> +	}
>> +
>> +	/* Wait until the CPU #1 at least enabled SCLP interrupts. */
>> +	wait_for_flag();
>> +
>> +	/*
>> +	 * We'd have to jump trough some hoops to sense e.g., via SIGP
>> +	 * CONDITIONAL EMERGENCY SIGNAL if CPU #1 is already in the
>> +	 * wait state.
>> +	 *
>> +	 * Although not completely reliable, use SIGP SENSE RUNNING STATUS
>> +	 * until not reported as running -- after all, our SCLP processing
>> +	 * will take some time as well and make races very rare.
>> +	 */
>> +	while(smp_sense_running_status(1));
>> +
>> +	h = alloc_page();
> 
> do you really need to dynamically allocate one page?
> is there a reason for not using a simple static buffer? (which you can
> have aligned and statically initialized)

I don't really have a strong opinion. I do prefer dynamic alloctions,
though, if there isn't a good reason not to use them. No need to mess
with page alignments manually.

> 
>> +	memset(h, 0, sizeof(*h));
> 
> otherwise, if you really want to allocate the memory, get rid of the
> memset; the allocator always returns zeroed memory (unless you
> explicitly ask not to by using flags)

Right. "special" FLAG_DONTZERO in that semantics in that allocator.

> 
>> +	h->length = 4096;
>> +	ret = servc(SCLP_CMDW_READ_CPU_INFO, __pa(h));
>> +	if (ret) {
>> +		report_fail("SCLP_CMDW_READ_CPU_INFO failed");
>> +		goto out_destroy;
>> +	}
>> +
>> +	/*
>> +	 * Wait until the interrupt gets delivered on CPU #1, marking the
> 
> why do you expect the interrupt to be delivered on CPU1? could it not
> be delivered on CPU0?

We don't enable SCLP interrupts + external interrupts on CPU #0 because
we'll only call sclp_setup_int() on CPU #1.

> 
>> +	 * SCLP requests as done.
>> +	 */
>> +	sclp_wait_busy();
> 
> this is logically not wrong (and should stay, because it makes clear
> what you are trying to do), but strictly speaking it's not needed since
> the report below will hang as long as the SCLP busy flag is set. 

Right. But it's really clearer to just have this in the code.


-- 
Thanks,

David / dhildenb