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

From: Pierre Morel <pmorel@linux.ibm.com>
To: Janosch Frank <frankja@linux.ibm.com>, linux-s390@vger.kernel.org
Cc: thuth@redhat.com, kvm@vger.kernel.org, imbrenda@linux.ibm.com,
	david@redhat.com, nrb@linux.ibm.com, nsg@linux.ibm.com,
	cohuck@redhat.com
Subject: Re: [kvm-unit-tests PATCH v3 2/2] s390x: sclp: Implement SCLP_RC_INSUFFICIENT_SCCB_LENGTH
Date: Thu, 1 Jun 2023 12:15:02 +0200	[thread overview]
Message-ID: <1c24e467-3b57-d7a9-be28-fcaf5e265b80@linux.ibm.com> (raw)
In-Reply-To: <3dc8e019-a3c1-8446-08ed-f76a9064f954@linux.ibm.com>

On 6/1/23 10:03, Janosch Frank wrote:
> 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?

yes up to 248

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

yes

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

yes

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

OK

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

Yes, I can let it here. I found it has little sense to give the length 
as parameter.

Retrying with extended length in sclp_read_scp_info() is what is done 
isn'it?

It does not change a lot to let the first used size here so I will let 
it here.

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