Re: [PATCH v1] self_tests/kvm: sync_regs and reset tests for diag318

[Thomas Huth <thuth@redhat.com>]

On 15/10/2020 14.40, Janosch Frank wrote:
> On 10/14/20 9:27 PM, Collin Walling wrote:
>> The DIAGNOSE 0x0318 instruction, unique to s390x, is a privileged call
>> that must be intercepted via SIE, handled in userspace, and the
>> information set by the instruction is communicated back to KVM.
> 
> It might be nice to have a few words in here about what information can
> be set via the diag.
> 
>>
>> To test the instruction interception, an ad-hoc handler is defined which
>> simply has a VM execute the instruction and then userspace will extract
>> the necessary info. The handler is defined such that the instruction
>> invocation occurs only once. It is up the the caller to determine how the
>> info returned by this handler should be used.
>>
>> The diag318 info is communicated from userspace to KVM via a sync_regs
>> call. This is tested during a sync_regs test, where the diag318 info is
>> requested via the handler, then the info is stored in the appropriate
>> register in KVM via a sync registers call.
>>
>> The diag318 info is checked to be 0 after a normal and clear reset.
>>
>> If KVM does not support diag318, then the tests will print a message
>> stating that diag318 was skipped, and the asserts will simply test
>> against a value of 0.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> 
> Checkpatch throws lots of errors on this patch.
> Could you check if my workflow misteriously introduced windows line
> endings or if they were introduced on your side?

How did you feed the patch into checkpatch? IIRC mails are often sent with
CR-LF line endings by default - it's "git am" that is converting the line
endings back to the Unix default. So for running a patch through checkpatch,
you might need to do "git am" first and then export it again.

>> +uint64_t get_diag318_info(void)
>> +{
>> +	static uint64_t diag318_info;
>> +	static bool printed_skip;
>> +
>> +	/*
>> +	 * If KVM does not support diag318, then return 0 to
>> +	 * ensure tests do not break.
>> +	 */
>> +	if (!kvm_check_cap(KVM_CAP_S390_DIAG318)) {
>> +		if (!printed_skip) {
>> +			fprintf(stdout, "KVM_CAP_S390_DIAG318 not supported. "
> 
> Whitespace after .
> 
>> +				"Skipping diag318 test.\n");

It's a multi-line text, so the whitespace is needed, isn't it?

>> +			printed_skip = true;
>> +		}
>> +		return 0;
>> +	}

 Thomas