Re: [Qemu-devel] [PATCH 1/2] s390x/3270: IDA support for 3270 via CcwDataStream

[Cornelia Huck <cohuck@redhat.com>]

On Wed, 20 Sep 2017 19:23:13 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Let us convert the 3270 code so it uses the recently introduced
> CcwDataStream abstraction instead of blindly assuming direct data access.
> 
> This patch does not change behavior beyond introducing IDA support: for
> direct data access CCWs everything stays as-is. (If there are bugs, they
> are also preserved).
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> ---
>  hw/char/terminal3270.c      | 18 +++++++++++-------
>  hw/s390x/3270-ccw.c         |  4 ++--
>  include/hw/s390x/3270-ccw.h |  5 ++---
>  3 files changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
> index 28f599111d..c976a63cc2 100644
> --- a/hw/char/terminal3270.c
> +++ b/hw/char/terminal3270.c
> @@ -182,14 +182,18 @@ static void terminal_init(EmulatedCcw3270Device *dev, Error **errp)
>                               terminal_read, chr_event, NULL, t, NULL, true);
>  }
>  
> -static int read_payload_3270(EmulatedCcw3270Device *dev, uint32_t cda,
> -                             uint16_t count)
> +static inline CcwDataStream *get_cds(Terminal3270 *t)
> +{
> +    return &(CCW_DEVICE(&t->cdev)->sch->cds);
> +}
> +
> +static int read_payload_3270(EmulatedCcw3270Device *dev)
>  {
>      Terminal3270 *t = TERMINAL_3270(dev);
>      int len;
>  
> -    len = MIN(count, t->in_len);
> -    cpu_physical_memory_write(cda, t->inv, len);
> +    len = MIN(ccw_dstream_avail(get_cds(t)), t->in_len);
> +    ccw_dstream_write_buf(get_cds(t), t->inv, len);

CCW_DEVICE() as called by get_cds() goes through qom, which implies a
bit of overhead. Not sure if it makes sense to cache it in this
function so you don't go through it multiple times. (Dito for the other
callback.)

>      t->in_len -= len;
>  
>      return len;

Looks good.