Re: [kvm-unit-tests PATCH v3 2/2] s390x: sclp: Implement SCLP_RC_INSUFFICIENT_SCCB_LENGTH

[Janosch Frank <frankja@linux.ibm.com>]

On 5/30/23 14:40, Pierre Morel wrote:
> If SCLP_CMDW_READ_SCP_INFO fails due to a short buffer, retry
> with a greater buffer.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

You've been testing using all possible cpus, haven't you?

>   }
>   
> -static void sclp_read_scp_info(ReadInfo *ri, int length)
> +static bool sclp_read_scp_info_extended(unsigned int command, ReadInfo *ri)
> +{
> +	int cc;
> +
> +	if (!test_facility(140)) {
> +		report_abort("S390_FEAT_EXTENDED_LENGTH_SCCB missing");

That's the QEMU name for the facility, isn't it?
"extended-length-SCCB facility is missing" might be better since that's 
the name that the architecture specifies for that feature.

> +		return false;
> +	}
> +	if (ri->h.length > (2 * PAGE_SIZE)) {

sizeof() would reduce the locations that we have to touch if we ever 
want to increase the buffer in the future.

> +		report_abort("SCLP_READ_INFO expected size too big");
> +		return false;
> +	}
> +
> +	sclp_mark_busy();
> +	memset(&ri->h, 0, sizeof(ri->h));
> +	ri->h.length = 2 * PAGE_SIZE;

Same here

> +
> +	cc = sclp_service_call(command, ri);
> +	if (cc) {
> +		report_abort("SCLP_READ_INFO error");
> +		return false;
> +	}
> +	if (ri->h.response_code != SCLP_RC_NORMAL_READ_COMPLETION) {
> +		report_abort("SCLP_READ_INFO error %02x", ri->h.response_code);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static void sclp_read_scp_info(ReadInfo *ri)
>   {
>   	unsigned int commands[] = { SCLP_CMDW_READ_SCP_INFO_FORCED,
>   				    SCLP_CMDW_READ_SCP_INFO };
> +	int length = PAGE_SIZE;
>   	int i, cc;
>   
>   	for (i = 0; i < ARRAY_SIZE(commands); i++) {
> @@ -101,19 +133,29 @@ static void sclp_read_scp_info(ReadInfo *ri, int length)
>   		ri->h.length = length;
>   
>   		cc = sclp_service_call(commands[i], ri);
> -		if (cc)
> -			break;
> -		if (ri->h.response_code == SCLP_RC_NORMAL_READ_COMPLETION)
> +		if (cc) {
> +			report_abort("SCLP_READ_INFO error");
>   			return;
> -		if (ri->h.response_code != SCLP_RC_INVALID_SCLP_COMMAND)
> +		}
> +
> +		switch (ri->h.response_code) {
> +		case SCLP_RC_NORMAL_READ_COMPLETION:
> +			return;
> +		case SCLP_RC_INVALID_SCLP_COMMAND:
>   			break;
> +		case SCLP_RC_INSUFFICIENT_SCCB_LENGTH:
> +			sclp_read_scp_info_extended(commands[i], ri);
> +			return;
> +		default:
> +			report_abort("READ_SCP_INFO failed");
> +			return;
> +		}
>   	}
> -	report_abort("READ_SCP_INFO failed");
>   }
>   
>   void sclp_read_info(void)
>   {
> -	sclp_read_scp_info((void *)_read_info, SCCB_SIZE);

Why did you remove that?
You could have re-tried with the extended-length in 
sclp_read_scp_info(). Or you could return the rc and introduce a tiny 
function that tries both lengths depending on the rc.

> +	sclp_read_scp_info((void *)_read_info);
>   	read_info = (ReadInfo *)_read_info;
>   }
>