Re: [kvm-unit-tests PATCH v2 6/6] s390x: Add linemode console

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 07.12.18 17:19, David Hildenbrand wrote:
> On 05.12.18 16:39, Janosch Frank wrote:
>> z/VM isn't fond of vt220, so we need line mode when running under z/VM.
>>
>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
>> ---
>>  lib/s390x/sclp-console.c | 199 ++++++++++++++++++++++++++++++++++++++++++-----
>>  lib/s390x/sclp.h         |  67 ++++++++++++++++
>>  2 files changed, 245 insertions(+), 21 deletions(-)
>>
>> diff --git a/lib/s390x/sclp-console.c b/lib/s390x/sclp-console.c
>> index 13ab03d..c31e178 100644
>> --- a/lib/s390x/sclp-console.c
>> +++ b/lib/s390x/sclp-console.c
>> @@ -11,23 +11,183 @@
[...]
>> +
>> +static bool initialized = false;
> 
> false is the default after your BSS clearing patch.

Yes, already fixed

> 
>> +
>> +static void sclp_print_ascii(const char *str)
>> +{
>> +	int len = strlen(str);
>> +	WriteEventData *sccb = (void *)_sccb;
>> +
>> +	while (sclp_busy)
>> +		/* Wait for SCLP request to complete */;
>> +	sclp_busy = true;
>> +	memset(sccb, 0, sizeof(*sccb));
>> +	sccb->h.length = sizeof(WriteEventData) + len;
>> +	sccb->h.function_code = SCLP_FC_NORMAL_WRITE;
>> +	sccb->ebh.length = sizeof(EventBufferHeader) + len;
>> +	sccb->ebh.type = SCLP_EVENT_ASCII_CONSOLE_DATA;
>> +	memcpy(sccb->data, str, len);
>> +
>> +	sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb);
> 
> Shouldn't we wait for a response? (see below)

Yes, already fixed

> 
>> +}
>> +
>> +static void sclp_print_lm(const char *str)
>> +{
>> +	unsigned char *ptr, *end, ch;
>> +	unsigned int count, offset, len;
>> +	struct write_sccb *sccb;
>> +	struct msg_buf *msg;
>> +	struct mdb *mdb;
>> +	struct mto *mto;
>> +	struct go *go;
>> +
>> +	while (sclp_busy)
>> +		/* Wait for SCLP request to complete */;
>> +	sclp_busy = true;
>> +	sccb = (struct write_sccb *) _sccb;
>> +	end = (unsigned char *) sccb + 4096 - 1;
>> +	memset(sccb, 0, sizeof(*sccb));
>> +	ptr = (unsigned char *) &sccb->msg.mdb.mto;
>> +	len = strlen(str);
>> +	offset = 0;
>> +	do {
>> +		for (count = sizeof(*mto); offset < len; count++) {
>> +			ch = str[offset++];
>> +			if ((ch == 0x0a) || (ptr + count > end))
>> +				break;
>> +			ptr[count] = _ascebc[ch];
>> +		}
>> +		mto = (struct mto *) ptr;
>> +		memset(mto, 0, sizeof(*mto));
>> +		mto->length = count;
>> +		mto->type = 4;
>> +		mto->line_type_flags = LNTPFLGS_ENDTEXT;
>> +		ptr += count;
>> +	} while ((offset < len) && (ptr + sizeof(*mto) <= end));
>> +	len = ptr - (unsigned char *) sccb;
>> +	sccb->header.length = len - offsetof(struct write_sccb, header);
>> +	msg = &sccb->msg;
>> +	msg->header.type = EVTYP_MSG;
>> +	msg->header.length = len - offsetof(struct write_sccb, msg.header);
>> +	mdb = &msg->mdb;
>> +	mdb->header.type = 1;
>> +	mdb->header.tag = 0xD4C4C240;
>> +	mdb->header.revision_code = 1;
>> +	mdb->header.length = len - offsetof(struct write_sccb, msg.mdb.header);
>> +	go = &mdb->go;
>> +	go->length = sizeof(*go);
>> +	go->type = 1;
>> +	sclp_service_call(SCLP_CMD_WRITE_EVENT_DATA, sccb);
> 
> Shouldn't we wait for a response? (see below)

Yes, already fixed

> 
>> +}
>> +
>> +/*
>> + * SCLP needs to be initialized by setting a send and receive mask,
>> + * indicating which messages the control program (we) want(s) to
>> + * send/receive.
>> + */
>> +static bool sclp_set_write_mask(void)
>>  {
>>  	WriteEventMask *sccb = (void *)_sccb;
>>  
>>  	while (sclp_busy)
>>  		/* Wait for SCLP request to complete */;
> 
> Can this actually happen?
> 
> I'd say whoever issues a request has to wait for the response. Somebody
> who didn't, doesn't have to wait.

We could put the sclp_wait_busy() at the end of the sclp call function
and effectively make it synchronous by enforcing the wait. We don't
really need the async performance gain.

> 
> (for multiple CPUs later on that has to be guarded by a lock in addition
> to the bool)

Yeah, smp is currently on another branch, so I might just start to add
another patch and introduce locks. :-)

> 
>>  	sclp_busy = true;
>> +	memset(_sccb, 0, sizeof(*sccb));
>>  	sccb->h.length = sizeof(WriteEventMask);
>> -	sccb->mask_length = sizeof(unsigned int);
>> -	sccb->receive_mask = SCLP_EVENT_MASK_MSG_ASCII;
>> -	sccb->cp_receive_mask = SCLP_EVENT_MASK_MSG_ASCII;
>> -	sccb->send_mask = SCLP_EVENT_MASK_MSG_ASCII;
>> -	sccb->cp_send_mask = SCLP_EVENT_MASK_MSG_ASCII;
>> +	sccb->h.function_code = 0;
>> +	sccb->mask_length = sizeof(sccb_mask_t);
>> +
>> +	/* For now we don't process sclp input. */
>> +	sccb->cp_receive_mask = 0;
>> +	/* We send ASCII and line mode. */
>> +	sccb->cp_send_mask = SCLP_EVENT_MASK_MSG_ASCII | SCLP_EVENT_MASK_MSG;
>>  
>>  	sclp_service_call(SCLP_CMD_WRITE_EVENT_MASK, sccb);
>> +	while (sclp_busy) {
>> +	}
>> +	if (sccb->h.response_code != SCLP_RC_NORMAL_COMPLETION)
>> +		return false;
> 
> Hmm, what do do in this case?  I don't see that the caller is fixed up
> (you changed the return type from void to bool)

I don't know, there used to be a TODO there...
I thought about calling exit(1);

> 
> I "initialized" actually worth keeping around? I'd say let's just
> continue as if it succeeded. (we cannot really do anything about it, can
> we?)

There is the problem of prints that come before the initialization.
PGM interrupts while sclp_busy == true is also fun if they want to print.



Attachment: signature.asc
Description: OpenPGP digital signature


[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