Re: [Qemu-devel] [PATCH v4 06/07] s390: sclp ascii console support

[Christian Borntraeger]

On 30/07/12 16:02, Alexander Graf wrote:

>> +    qemu_irq sclp_read_vt220;
> 
> I'm sure this one wants a name that indicates it's an irq line ;)

ok.

> 
>> +} SCLPConsole;
>> +
>> +/* character layer call-back functions */
>> +
>> +/* Return number of bytes that fit into iov buffer */
>> +static int chr_can_read(void *opaque)
>> +{
>> +    int can_read;
>> +    SCLPConsole *scon = opaque;
>> +
>> +    qemu_mutex_lock(&scon->event.lock);
> 
> Explicit locks now?

IIRC Heinz introduced the locks since we have multiple threads working on the 
same
kind of buffers (the cpu threads and the main thread). We could not verify that 
the
main thread has the big qemu lock in all cases. Furthermore, this makes the 
code already
thread safe if we get rid of the big qemu lock somewhen. But I agree, we have 
to double
check why there are two different kind of locks.

[...]

>> +    /* if new data do not fit into current buffer */
>> +    if (scon->iov_data_len + size > SIZE_BUFFER_VT220) {
>> +        /* character layer sent more than allowed */
>> +        qemu_mutex_unlock(&scon->event.lock);
>> +        return;
> 
> So we drop the bytes it did send? Isn't there usually some can_read function 
> from the char layer that we can indicate to that we have enough space?
> 
> If so, then this should be an assert(), right?

Probably yes. Will double check.

[..]

>> +    SCLPConsole *cons = DO_UPCAST(SCLPConsole, event, event);
>> +
>> +    /* first byte is hex 0 saying an ascii string follows */
>> +    *buf++ = '\0';
>> +    avail--;
>> +    /* if all data fit into provided SCLP buffer */
>> +    if (avail >= cons->iov_sclp_rest) {
>> +        /* copy character byte-stream to SCLP buffer */
>> +        memcpy(buf, cons->iov_sclp, cons->iov_sclp_rest);
>> +        *size = cons->iov_sclp_rest + 1;
>> +        cons->iov_sclp = cons->iov;
>> +        cons->iov_bs = cons->iov;
>> +        cons->iov_data_len = 0;
>> +        cons->iov_sclp_rest = 0;
>> +        event->event_pending = false;
>> +        /* data provided and no more data pending */
>> +    } else {
>> +        /* if provided buffer is too small, just copy part */
>> +        memcpy(buf, cons->iov_sclp, avail);
>> +        *size = avail + 1;
>> +        cons->iov_sclp_rest -= avail;
>> +        cons->iov_sclp += avail;
>> +        /* more data pending */
>> +    }
>> +}
> 
> I'm wondering whether we actually need this indirection from
> 
>   chr layer -> buffer -> sclp buffer.
> 
> Why can't we just do
> 
>   chr layer -> sclp buffer?
> 

The sclp interface is a bit different here. On an input event, the hw generated 
an service
interrupt with the event pending bit set. Then the guest kernel does a read 
event data sclp
call with input buffer. The input data has to copied into that and then 
returned via an
additional interrupt. 

Without the buffering our can_read function could only return 0 as size since 
there is yet no
buffer. Makes sense? 

Christian