diff mbox

[1/2] s390x/3270: IDA support for 3270 via CcwDataStream

Message ID 20170920172314.102710-2-pasic@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Halil Pasic Sept. 20, 2017, 5:23 p.m. UTC 
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(-)

Comments

Cornelia Huck Sept. 21, 2017, 9:15 a.m. UTC | #1 
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.
Halil Pasic Sept. 21, 2017, 11:22 a.m. UTC | #2 
On 09/21/2017 11:15 AM, Cornelia Huck wrote:
>> +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.)
> 

I've cargo-culted this way of getting CCW_DEVICE(&t->cdev) to the CcwDevice
form terminal_read (the pattern used at multiple places in the file). As
far as I can tell, the overhead basically depends on CONFIG_QOM_CAST_DEBUG.

I have no idea what do we have in production, but my guess is, that
ONFIG_QOM_CAST_DEBUG makes only sense for development and testing
(especially if proper test coverage is assumed).
Can you enlighten me?

CCW_DEVICE() may contain a run-time check (depending on CONFIG_QOM_CAST_DEBUG),
we however can make sure things are OK at compile time. This brings
me to the next question. Does it even make sense to use OBJECT_CHECK based
constructs when going from specific to general (we don't actually need a
cast here)? Obviously, for the other direction we really need a cast, so doing
a run-time check there does indeed provide added value.

Halil


>>      t->in_len -= len;
>>  
>>      return len;
Cornelia Huck Sept. 21, 2017, 12:05 p.m. UTC | #3 
On Thu, 21 Sep 2017 13:22:44 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/21/2017 11:15 AM, Cornelia Huck wrote:
> >> +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.)
> >   
> 
> I've cargo-culted this way of getting CCW_DEVICE(&t->cdev) to the CcwDevice
> form terminal_read (the pattern used at multiple places in the file). As
> far as I can tell, the overhead basically depends on CONFIG_QOM_CAST_DEBUG.
> 
> I have no idea what do we have in production, but my guess is, that
> ONFIG_QOM_CAST_DEBUG makes only sense for development and testing
> (especially if proper test coverage is assumed).
> Can you enlighten me?
> 
> CCW_DEVICE() may contain a run-time check (depending on CONFIG_QOM_CAST_DEBUG),
> we however can make sure things are OK at compile time. This brings
> me to the next question. Does it even make sense to use OBJECT_CHECK based
> constructs when going from specific to general (we don't actually need a
> cast here)? Obviously, for the other direction we really need a cast, so doing
> a run-time check there does indeed provide added value.

The basic rule seems to be "use a qom cast, unless you are on a fast
path" - even though qom debug makes the most sense while developing.

But let's not turn that into a big discussion: It's probably not really
worth optimizing here.
Halil Pasic Sept. 21, 2017, 4:11 p.m. UTC | #4 
On 09/21/2017 02:05 PM, Cornelia Huck wrote:
> On Thu, 21 Sep 2017 13:22:44 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 09/21/2017 11:15 AM, Cornelia Huck wrote:
>>>> +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.)
>>>   
>>
>> I've cargo-culted this way of getting CCW_DEVICE(&t->cdev) to the CcwDevice
>> form terminal_read (the pattern used at multiple places in the file). As
>> far as I can tell, the overhead basically depends on CONFIG_QOM_CAST_DEBUG.
>>
>> I have no idea what do we have in production, but my guess is, that
>> ONFIG_QOM_CAST_DEBUG makes only sense for development and testing
>> (especially if proper test coverage is assumed).
>> Can you enlighten me?
>>
>> CCW_DEVICE() may contain a run-time check (depending on CONFIG_QOM_CAST_DEBUG),
>> we however can make sure things are OK at compile time. This brings
>> me to the next question. Does it even make sense to use OBJECT_CHECK based
>> constructs when going from specific to general (we don't actually need a
>> cast here)? Obviously, for the other direction we really need a cast, so doing
>> a run-time check there does indeed provide added value.
> 
> The basic rule seems to be "use a qom cast, unless you are on a fast
> path" - even though qom debug makes the most sense while developing.
> 

Does this imply "don't use qom cast on fast path"? I guess that would
mean that we basically expect production builds being with CONFIG_QOM_CAST_DEBUG
defined.

> But let's not turn that into a big discussion: It's probably not really
> worth optimizing here.
>

I agree about the optimization. OTOH I do believe establishing best
practices is important. I'm afraid, I'm seeing much 'more monkey see
monkey do' than healthy (in such an environment prior art is even
more important). I believe, understanding a best practice (candidate)
should always be a part of establishing a best practice. 

Sorry, I may be inappropriate (you requested to not turn this into
a big discussion, and I'm afraid this goes in that direction). If
I'm please ignore the stuff above this line.

So, there is nothing to be addressed about about this series so far.
Does this mean good for inclusion once prerequisites are met -- unless
somebody finds something?

Regards,
Halil
Cornelia Huck Sept. 22, 2017, 1:38 p.m. UTC | #5 
On Thu, 21 Sep 2017 18:11:06 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> So, there is nothing to be addressed about about this series so far.
> Does this mean good for inclusion once prerequisites are met -- unless
> somebody finds something?

It is in my pipeline, and I currently don't see anything wrong with it.

More reviews are still welcome, though, as always :)
diff mbox

Patch

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);
     t->in_len -= len;
 
     return len;
@@ -222,11 +226,11 @@  static int insert_IAC_escape_char(uint8_t *outv, int out_len)
  * Write 3270 outbound to socket.
  * Return the count of 3270 data field if succeeded, zero if failed.
  */
-static int write_payload_3270(EmulatedCcw3270Device *dev, uint8_t cmd,
-                              uint32_t cda, uint16_t count)
+static int write_payload_3270(EmulatedCcw3270Device *dev, uint8_t cmd)
 {
     Terminal3270 *t = TERMINAL_3270(dev);
     int retval = 0;
+    int count = ccw_dstream_avail(get_cds(t));
 
     assert(count <= (OUTPUT_BUFFER_SIZE - 3) / 2);
 
@@ -244,7 +248,7 @@  static int write_payload_3270(EmulatedCcw3270Device *dev, uint8_t cmd,
         return count;
     }
     t->outv[0] = cmd;
-    cpu_physical_memory_read(cda, &t->outv[1], count);
+    ccw_dstream_read_buf(get_cds(t), &t->outv[1], count);
     t->out_len = count + 1;
 
     t->out_len = insert_IAC_escape_char(t->outv, t->out_len);
diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c
index 1554aa2484..eaca28e224 100644
--- a/hw/s390x/3270-ccw.c
+++ b/hw/s390x/3270-ccw.c
@@ -28,7 +28,7 @@  static int handle_payload_3270_read(EmulatedCcw3270Device *dev, CCW1 *ccw)
         return -EFAULT;
     }
 
-    len = ck->read_payload_3270(dev, ccw->cda, ccw->count);
+    len = ck->read_payload_3270(dev);
     ccw_dev->sch->curr_status.scsw.count = ccw->count - len;
 
     return 0;
@@ -45,7 +45,7 @@  static int handle_payload_3270_write(EmulatedCcw3270Device *dev, CCW1 *ccw)
         return -EFAULT;
     }
 
-    len = ck->write_payload_3270(dev, ccw->cmd_code, ccw->cda, ccw->count);
+    len = ck->write_payload_3270(dev, ccw->cmd_code);
 
     if (len <= 0) {
         return -EIO;
diff --git a/include/hw/s390x/3270-ccw.h b/include/hw/s390x/3270-ccw.h
index 46bee2533c..9d1d18e2bd 100644
--- a/include/hw/s390x/3270-ccw.h
+++ b/include/hw/s390x/3270-ccw.h
@@ -45,9 +45,8 @@  typedef struct EmulatedCcw3270Class {
     CCWDeviceClass parent_class;
 
     void (*init)(EmulatedCcw3270Device *, Error **);
-    int (*read_payload_3270)(EmulatedCcw3270Device *, uint32_t, uint16_t);
-    int (*write_payload_3270)(EmulatedCcw3270Device *, uint8_t, uint32_t,
-                              uint16_t);
+    int (*read_payload_3270)(EmulatedCcw3270Device *);
+    int (*write_payload_3270)(EmulatedCcw3270Device *, uint8_t);
 } EmulatedCcw3270Class;
 
 #endif