We have a use case (vfio-ccw) where a CRW is already built and
ready to use.  Rather than teasing out the components just to
reassemble it later, let's rework this code so we can queue a
fully-qualified CRW directly.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/s390x/css.c         | 44 ++++++++++++++++++++++++++++--------------
 include/hw/s390x/css.h |  1 +
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index a44faa3549..a72c09adbe 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -2170,30 +2170,23 @@ void css_subch_assign(uint8_t cssid, uint8_t ssid, uint16_t schid,
     }
 }
 
-void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
-                   int chain, uint16_t rsid)
+void css_queue_crw_cont(CRW crw)
 {
     CrwContainer *crw_cont;
 
-    trace_css_crw(rsc, erc, rsid, chain ? "(chained)" : "");
+    trace_css_crw((crw.flags & CRW_FLAGS_MASK_RSC) >> 8,
+                  crw.flags & CRW_FLAGS_MASK_ERC,
+                  crw.rsid,
+                  (crw.flags & CRW_FLAGS_MASK_C) ? "(chained)" : "");
+
     /* TODO: Maybe use a static crw pool? */
     crw_cont = g_try_new0(CrwContainer, 1);
     if (!crw_cont) {
         channel_subsys.crws_lost = true;
         return;
     }
-    crw_cont->crw.flags = (rsc << 8) | erc;
-    if (solicited) {
-        crw_cont->crw.flags |= CRW_FLAGS_MASK_S;
-    }
-    if (chain) {
-        crw_cont->crw.flags |= CRW_FLAGS_MASK_C;
-    }
-    crw_cont->crw.rsid = rsid;
-    if (channel_subsys.crws_lost) {
-        crw_cont->crw.flags |= CRW_FLAGS_MASK_R;
-        channel_subsys.crws_lost = false;
-    }
+
+    crw_cont->crw = crw;
 
     QTAILQ_INSERT_TAIL(&channel_subsys.pending_crws, crw_cont, sibling);
 
@@ -2204,6 +2197,27 @@ void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
     }
 }
 
+void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
+                   int chain, uint16_t rsid)
+{
+    CRW crw;
+
+    crw.flags = (rsc << 8) | erc;
+    if (solicited) {
+        crw.flags |= CRW_FLAGS_MASK_S;
+    }
+    if (chain) {
+        crw.flags |= CRW_FLAGS_MASK_C;
+    }
+    crw.rsid = rsid;
+    if (channel_subsys.crws_lost) {
+        crw.flags |= CRW_FLAGS_MASK_R;
+        channel_subsys.crws_lost = false;
+    }
+
+    css_queue_crw_cont(crw);
+}
+
 void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, uint16_t schid,
                            int hotplugged, int add)
 {
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 7e3a5e7433..1aa7b80f5b 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -205,6 +205,7 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src);
 void css_inject_io_interrupt(SubchDev *sch);
 void css_reset(void);
 void css_reset_sch(SubchDev *sch);
+void css_queue_crw_cont(CRW crw);
 void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
                    int chain, uint16_t rsid);
 void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, uint16_t schid,
-- 
2.17.1

On Fri, 17 Apr 2020 04:34:39 +0200
Eric Farman <farman@linux.ibm.com> wrote:

> We have a use case (vfio-ccw) where a CRW is already built and
> ready to use.  Rather than teasing out the components just to
> reassemble it later, let's rework this code so we can queue a
> fully-qualified CRW directly.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  hw/s390x/css.c         | 44 ++++++++++++++++++++++++++++--------------
>  include/hw/s390x/css.h |  1 +
>  2 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index a44faa3549..a72c09adbe 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -2170,30 +2170,23 @@ void css_subch_assign(uint8_t cssid, uint8_t ssid, uint16_t schid,
>      }
>  }
>  
> -void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
> -                   int chain, uint16_t rsid)
> +void css_queue_crw_cont(CRW crw)

Don't really like this name, as it makes me think of 'continuation'
instead of 'container'.

css_queue_crw_container?
css_crw_add_to_queue?

Naming is hard :(

>  {
>      CrwContainer *crw_cont;
>  
> -    trace_css_crw(rsc, erc, rsid, chain ? "(chained)" : "");
> +    trace_css_crw((crw.flags & CRW_FLAGS_MASK_RSC) >> 8,
> +                  crw.flags & CRW_FLAGS_MASK_ERC,
> +                  crw.rsid,
> +                  (crw.flags & CRW_FLAGS_MASK_C) ? "(chained)" : "");
> +
>      /* TODO: Maybe use a static crw pool? */
>      crw_cont = g_try_new0(CrwContainer, 1);
>      if (!crw_cont) {
>          channel_subsys.crws_lost = true;
>          return;

Now that we actually pass something in, do we want to inform the caller
whether the crw was queued or not?

>      }
> -    crw_cont->crw.flags = (rsc << 8) | erc;
> -    if (solicited) {
> -        crw_cont->crw.flags |= CRW_FLAGS_MASK_S;
> -    }
> -    if (chain) {
> -        crw_cont->crw.flags |= CRW_FLAGS_MASK_C;
> -    }
> -    crw_cont->crw.rsid = rsid;
> -    if (channel_subsys.crws_lost) {
> -        crw_cont->crw.flags |= CRW_FLAGS_MASK_R;
> -        channel_subsys.crws_lost = false;
> -    }
> +
> +    crw_cont->crw = crw;
>  
>      QTAILQ_INSERT_TAIL(&channel_subsys.pending_crws, crw_cont, sibling);
>  

Generally, looks sane to me.

On 4/21/20 8:28 AM, Cornelia Huck wrote:
> On Fri, 17 Apr 2020 04:34:39 +0200
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> We have a use case (vfio-ccw) where a CRW is already built and
>> ready to use.  Rather than teasing out the components just to
>> reassemble it later, let's rework this code so we can queue a
>> fully-qualified CRW directly.
>>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>  hw/s390x/css.c         | 44 ++++++++++++++++++++++++++++--------------
>>  include/hw/s390x/css.h |  1 +
>>  2 files changed, 30 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index a44faa3549..a72c09adbe 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -2170,30 +2170,23 @@ void css_subch_assign(uint8_t cssid, uint8_t ssid, uint16_t schid,
>>      }
>>  }
>>  
>> -void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
>> -                   int chain, uint16_t rsid)
>> +void css_queue_crw_cont(CRW crw)
> 
> Don't really like this name, as it makes me think of 'continuation'
> instead of 'container'.
> 
> css_queue_crw_container?
> css_crw_add_to_queue?
> 
> Naming is hard :(

Yeah, I don't like it either.  Just took it from the variable "crw_cont"
in the original code, but it does seem more like a continuation.  Since
the "container" is something built in that routine, maybe the
"add_to_queue" variant fits better.

> 
>>  {
>>      CrwContainer *crw_cont;
>>  
>> -    trace_css_crw(rsc, erc, rsid, chain ? "(chained)" : "");
>> +    trace_css_crw((crw.flags & CRW_FLAGS_MASK_RSC) >> 8,
>> +                  crw.flags & CRW_FLAGS_MASK_ERC,
>> +                  crw.rsid,
>> +                  (crw.flags & CRW_FLAGS_MASK_C) ? "(chained)" : "");
>> +
>>      /* TODO: Maybe use a static crw pool? */
>>      crw_cont = g_try_new0(CrwContainer, 1);
>>      if (!crw_cont) {
>>          channel_subsys.crws_lost = true;
>>          return;
> 
> Now that we actually pass something in, do we want to inform the caller
> whether the crw was queued or not?

Hrm...  Well I guess we could use it to break out of our loop in patch
7.  But for the existing callers of css_queue_crw(), it doesn't provide
much value to anyone but css_generate_css_crws().  I'll poke at this a bit.

> 
>>      }
>> -    crw_cont->crw.flags = (rsc << 8) | erc;
>> -    if (solicited) {
>> -        crw_cont->crw.flags |= CRW_FLAGS_MASK_S;
>> -    }
>> -    if (chain) {
>> -        crw_cont->crw.flags |= CRW_FLAGS_MASK_C;
>> -    }
>> -    crw_cont->crw.rsid = rsid;
>> -    if (channel_subsys.crws_lost) {
>> -        crw_cont->crw.flags |= CRW_FLAGS_MASK_R;
>> -        channel_subsys.crws_lost = false;
>> -    }
>> +
>> +    crw_cont->crw = crw;
>>  
>>      QTAILQ_INSERT_TAIL(&channel_subsys.pending_crws, crw_cont, sibling);
>>  
> 
> Generally, looks sane to me.
>