[Qemu-devel] [PATCH v2 0/2] ERC cleanup and CRW bugfix (was: Channel Path realted CRW generation)

[Qemu-devel] [PATCH v2 0/2] ERC cleanup and CRW bugfix (was: Channel Path realted CRW generation)

[Dong Jia Shi]

This series is trying to:
1. clear up ERC related code
2. bugfix for channel path related CRW generation

Change log
----------
v1->v2:
Patch #1:
  Add all ERCs.
  Commit message update.
Patch #2:
  Rename the new added parameter to more speaking name -- solicited.

Dong Jia Shi (2):
  s390x/css: use macro for event-information pending error recover code
  s390x/css: generate solicited crw for rchp completion signaling

 hw/s390x/css.c            | 16 ++++++++++------
 include/hw/s390x/css.h    |  3 ++-
 include/hw/s390x/ioinst.h | 11 +++++++++--
 3 files changed, 21 insertions(+), 9 deletions(-)

-- 
2.11.2

[Qemu-devel] [PATCH v2 1/2] s390x/css: use macro for event-information pending error recover code

[Dong Jia Shi]

Let's use a macro for the ERC (error recover code) when generating a
Channel Subsystem Event-information pending CRW (channel report word).

While we are at it, let's also add all other ERCs.

Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
---
 hw/s390x/css.c            |  2 +-
 include/hw/s390x/ioinst.h | 11 +++++++++--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 6a42b95cee..5321ca016b 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -2103,7 +2103,7 @@ void css_generate_chp_crws(uint8_t cssid, uint8_t chpid)
 void css_generate_css_crws(uint8_t cssid)
 {
     if (!channel_subsys.sei_pending) {
-        css_queue_crw(CRW_RSC_CSS, 0, 0, cssid);
+        css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, cssid);
     }
     channel_subsys.sei_pending = true;
 }
diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h
index 92d15655e4..f89019f78f 100644
--- a/include/hw/s390x/ioinst.h
+++ b/include/hw/s390x/ioinst.h
@@ -201,8 +201,15 @@ typedef struct CRW {
 #define CRW_FLAGS_MASK_A 0x0080
 #define CRW_FLAGS_MASK_ERC 0x003f
 
-#define CRW_ERC_INIT 0x02
-#define CRW_ERC_IPI  0x04
+#define CRW_ERC_EVENT    0x00 /* event information pending */
+#define CRW_ERC_AVAIL    0x01 /* available */
+#define CRW_ERC_INIT     0x02 /* initialized */
+#define CRW_ERC_TERROR   0x03 /* temporary error */
+#define CRW_ERC_IPI      0x04 /* installed parm initialized */
+#define CRW_ERC_TERM     0x05 /* terminal */
+#define CRW_ERC_PERRN    0x06 /* perm. error, facility not init */
+#define CRW_ERC_PERRI    0x07 /* perm. error, facility init */
+#define CRW_ERC_PMOD     0x08 /* installed parameters modified */
 
 #define CRW_RSC_SUBCH 0x3
 #define CRW_RSC_CHP   0x4
-- 
2.11.2

[Qemu-devel] [PATCH v2 2/2] s390x/css: generate solicited crw for rchp completion signaling

[Dong Jia Shi]

A successful completion of rchp should signal a solicited channel path
initialized CRW (channel report word), while the current implementation
always generates an un-solicited one. Let's fix this.

Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
---
 hw/s390x/css.c         | 16 ++++++++++------
 include/hw/s390x/css.h |  3 ++-
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 5321ca016b..bfa56f4b12 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1745,10 +1745,10 @@ int css_do_rchp(uint8_t cssid, uint8_t chpid)
     }
 
     /* We don't really use a channel path, so we're done here. */
-    css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT,
+    css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1,
                   channel_subsys.max_cssid > 0 ? 1 : 0, chpid);
     if (channel_subsys.max_cssid > 0) {
-        css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 0, real_cssid << 8);
+        css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1, 0, real_cssid << 8);
     }
     return 0;
 }
@@ -2028,7 +2028,8 @@ void css_subch_assign(uint8_t cssid, uint8_t ssid, uint16_t schid,
     }
 }
 
-void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid)
+void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
+                   int chain, uint16_t rsid)
 {
     CrwContainer *crw_cont;
 
@@ -2040,6 +2041,9 @@ void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid)
         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;
     }
@@ -2086,9 +2090,9 @@ void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, uint16_t schid,
     }
     chain_crw = (channel_subsys.max_ssid > 0) ||
             (channel_subsys.max_cssid > 0);
-    css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, chain_crw ? 1 : 0, schid);
+    css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0, chain_crw ? 1 : 0, schid);
     if (chain_crw) {
-        css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0,
+        css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0, 0,
                       (guest_cssid << 8) | (ssid << 4));
     }
     /* RW_ERC_IPI --> clear pending interrupts */
@@ -2103,7 +2107,7 @@ void css_generate_chp_crws(uint8_t cssid, uint8_t chpid)
 void css_generate_css_crws(uint8_t cssid)
 {
     if (!channel_subsys.sei_pending) {
-        css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, cssid);
+        css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, 0, cssid);
     }
     channel_subsys.sei_pending = true;
 }
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 5c5fe6b202..5b017e1fc3 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -150,7 +150,8 @@ 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(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid);
+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,
                            int hotplugged, int add);
 void css_generate_chp_crws(uint8_t cssid, uint8_t chpid);
-- 
2.11.2

Re: [Qemu-devel] [PATCH v2 2/2] s390x/css: generate solicited crw for rchp completion signaling

[Halil Pasic]

On 08/01/2017 09:57 AM, Dong Jia Shi wrote:
[..]
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1745,10 +1745,10 @@ int css_do_rchp(uint8_t cssid, uint8_t chpid)
>      }
> 
>      /* We don't really use a channel path, so we're done here. */
> -    css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT,
> +    css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1,
>                    channel_subsys.max_cssid > 0 ? 1 : 0, chpid);
>      if (channel_subsys.max_cssid > 0) {
> -        css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 0, real_cssid << 8);
> +        css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1, 0, real_cssid << 8);
>      }
>      return 0;
>  }
> @@ -2028,7 +2028,8 @@ void css_subch_assign(uint8_t cssid, uint8_t ssid, uint16_t schid,
>      }
>  }
> 
> -void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid)
> +void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
> +                   int chain, uint16_t rsid)

I think you could make the parameters solicited and chain bool (AFAIU
they are conceptually bool) for clearer semantic. If you go with that
you could also get rid of the superfluous ternary operator ( we have
stuff like some_cond ? 1 : 0 for the chain parameter in more than
one place.

Btw. I find bool flags easy to mix up and difficult to read. I have no better
idea how to write this (in C) though. I was considering throwing chain and
solicited together into a single flags parameter, but looking at the client code
it does not look like a good idea.

Besides the cosmetic considerations above LGTM

Re: [Qemu-devel] [PATCH v2 1/2] s390x/css: use macro for event-information pending error recover code

[Halil Pasic]

On 08/01/2017 09:57 AM, Dong Jia Shi wrote:
> Let's use a macro for the ERC (error recover code) when generating a
> Channel Subsystem Event-information pending CRW (channel report word).
> 
> While we are at it, let's also add all other ERCs.
> 
> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c            |  2 +-
>  include/hw/s390x/ioinst.h | 11 +++++++++--
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 6a42b95cee..5321ca016b 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -2103,7 +2103,7 @@ void css_generate_chp_crws(uint8_t cssid, uint8_t chpid)
>  void css_generate_css_crws(uint8_t cssid)
>  {
>      if (!channel_subsys.sei_pending) {
> -        css_queue_crw(CRW_RSC_CSS, 0, 0, cssid);
> +        css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, cssid);
>      }
>      channel_subsys.sei_pending = true;
>  }
> diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h
> index 92d15655e4..f89019f78f 100644
> --- a/include/hw/s390x/ioinst.h
> +++ b/include/hw/s390x/ioinst.h
> @@ -201,8 +201,15 @@ typedef struct CRW {
>  #define CRW_FLAGS_MASK_A 0x0080
>  #define CRW_FLAGS_MASK_ERC 0x003f
> 
> -#define CRW_ERC_INIT 0x02
> -#define CRW_ERC_IPI  0x04
> +#define CRW_ERC_EVENT    0x00 /* event information pending */
> +#define CRW_ERC_AVAIL    0x01 /* available */
> +#define CRW_ERC_INIT     0x02 /* initialized */
> +#define CRW_ERC_TERROR   0x03 /* temporary error */
> +#define CRW_ERC_IPI      0x04 /* installed parm initialized */
> +#define CRW_ERC_TERM     0x05 /* terminal */
> +#define CRW_ERC_PERRN    0x06 /* perm. error, facility not init */
> +#define CRW_ERC_PERRI    0x07 /* perm. error, facility init */
> +#define CRW_ERC_PMOD     0x08 /* installed parameters modified */

You have missed installed parameters restored from the PoP (above
you say add all other).

Other than that.

LGTM

> 
>  #define CRW_RSC_SUBCH 0x3
>  #define CRW_RSC_CHP   0x4
> 

Re: [Qemu-devel] [PATCH v2 1/2] s390x/css: use macro for event-information pending error recover code

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-08-01 17:24:10 +0200]:

> 
> 
> On 08/01/2017 09:57 AM, Dong Jia Shi wrote:
> > Let's use a macro for the ERC (error recover code) when generating a
> > Channel Subsystem Event-information pending CRW (channel report word).
> > 
> > While we are at it, let's also add all other ERCs.
> > 
> > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> > ---
> >  hw/s390x/css.c            |  2 +-
> >  include/hw/s390x/ioinst.h | 11 +++++++++--
> >  2 files changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> > index 6a42b95cee..5321ca016b 100644
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -2103,7 +2103,7 @@ void css_generate_chp_crws(uint8_t cssid, uint8_t chpid)
> >  void css_generate_css_crws(uint8_t cssid)
> >  {
> >      if (!channel_subsys.sei_pending) {
> > -        css_queue_crw(CRW_RSC_CSS, 0, 0, cssid);
> > +        css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, cssid);
> >      }
> >      channel_subsys.sei_pending = true;
> >  }
> > diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h
> > index 92d15655e4..f89019f78f 100644
> > --- a/include/hw/s390x/ioinst.h
> > +++ b/include/hw/s390x/ioinst.h
> > @@ -201,8 +201,15 @@ typedef struct CRW {
> >  #define CRW_FLAGS_MASK_A 0x0080
> >  #define CRW_FLAGS_MASK_ERC 0x003f
> > 
> > -#define CRW_ERC_INIT 0x02
> > -#define CRW_ERC_IPI  0x04
> > +#define CRW_ERC_EVENT    0x00 /* event information pending */
> > +#define CRW_ERC_AVAIL    0x01 /* available */
> > +#define CRW_ERC_INIT     0x02 /* initialized */
> > +#define CRW_ERC_TERROR   0x03 /* temporary error */
> > +#define CRW_ERC_IPI      0x04 /* installed parm initialized */
> > +#define CRW_ERC_TERM     0x05 /* terminal */
> > +#define CRW_ERC_PERRN    0x06 /* perm. error, facility not init */
> > +#define CRW_ERC_PERRI    0x07 /* perm. error, facility init */
> > +#define CRW_ERC_PMOD     0x08 /* installed parameters modified */
> 
> You have missed installed parameters restored from the PoP (above
> you say add all other).
Ok. Will add:
#define CRW_ERC_IPR      0x0A /* installed parameters restored */

> 
> Other than that.
> 
> LGTM
> 
> > 
> >  #define CRW_RSC_SUBCH 0x3
> >  #define CRW_RSC_CHP   0x4
> > 

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH v2 2/2] s390x/css: generate solicited crw for rchp completion signaling

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-08-01 17:16:37 +0200]:

> 
> 
> On 08/01/2017 09:57 AM, Dong Jia Shi wrote:
> [..]
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -1745,10 +1745,10 @@ int css_do_rchp(uint8_t cssid, uint8_t chpid)
> >      }
> > 
> >      /* We don't really use a channel path, so we're done here. */
> > -    css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT,
> > +    css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1,
> >                    channel_subsys.max_cssid > 0 ? 1 : 0, chpid);
> >      if (channel_subsys.max_cssid > 0) {
> > -        css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 0, real_cssid << 8);
> > +        css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1, 0, real_cssid << 8);
> >      }
> >      return 0;
> >  }
> > @@ -2028,7 +2028,8 @@ void css_subch_assign(uint8_t cssid, uint8_t ssid, uint16_t schid,
> >      }
> >  }
> > 
> > -void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid)
> > +void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
> > +                   int chain, uint16_t rsid)
> 
> I think you could make the parameters solicited and chain bool (AFAIU
> they are conceptually bool) for clearer semantic. If you go with that
> you could also get rid of the superfluous ternary operator ( we have
> stuff like some_cond ? 1 : 0 for the chain parameter in more than
> one place.
> 
> Btw. I find bool flags easy to mix up and difficult to read. I have no better
> idea how to write this (in C) though. I was considering throwing chain and
> solicited together into a single flags parameter, but looking at the client code
> it does not look like a good idea.
I think I just need to get used to differet tastes. ;)

> 
> Besides the cosmetic considerations above LGTM
Thanks!

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH v2 1/2] s390x/css: use macro for event-information pending error recover code

[Cornelia Huck]

On Wed, 2 Aug 2017 09:15:18 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Halil Pasic <pasic@linux.vnet.ibm.com> [2017-08-01 17:24:10 +0200]:
> 
> > 
> > 
> > On 08/01/2017 09:57 AM, Dong Jia Shi wrote:  
> > > Let's use a macro for the ERC (error recover code) when generating a
> > > Channel Subsystem Event-information pending CRW (channel report word).
> > > 
> > > While we are at it, let's also add all other ERCs.
> > > 
> > > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> > > ---
> > >  hw/s390x/css.c            |  2 +-
> > >  include/hw/s390x/ioinst.h | 11 +++++++++--
> > >  2 files changed, 10 insertions(+), 3 deletions(-)

(...)

> > > diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h
> > > index 92d15655e4..f89019f78f 100644
> > > --- a/include/hw/s390x/ioinst.h
> > > +++ b/include/hw/s390x/ioinst.h
> > > @@ -201,8 +201,15 @@ typedef struct CRW {
> > >  #define CRW_FLAGS_MASK_A 0x0080
> > >  #define CRW_FLAGS_MASK_ERC 0x003f
> > > 
> > > -#define CRW_ERC_INIT 0x02
> > > -#define CRW_ERC_IPI  0x04
> > > +#define CRW_ERC_EVENT    0x00 /* event information pending */
> > > +#define CRW_ERC_AVAIL    0x01 /* available */
> > > +#define CRW_ERC_INIT     0x02 /* initialized */
> > > +#define CRW_ERC_TERROR   0x03 /* temporary error */
> > > +#define CRW_ERC_IPI      0x04 /* installed parm initialized */
> > > +#define CRW_ERC_TERM     0x05 /* terminal */
> > > +#define CRW_ERC_PERRN    0x06 /* perm. error, facility not init */
> > > +#define CRW_ERC_PERRI    0x07 /* perm. error, facility init */
> > > +#define CRW_ERC_PMOD     0x08 /* installed parameters modified */  
> > 
> > You have missed installed parameters restored from the PoP (above
> > you say add all other).  
> Ok. Will add:
> #define CRW_ERC_IPR      0x0A /* installed parameters restored */

Makes sense.

> 
> > 
> > Other than that.
> > 
> > LGTM
> >   
> > > 
> > >  #define CRW_RSC_SUBCH 0x3
> > >  #define CRW_RSC_CHP   0x4
> > >   
> 

Re: [Qemu-devel] [PATCH v2 2/2] s390x/css: generate solicited crw for rchp completion signaling

[Cornelia Huck]

On Tue, 1 Aug 2017 17:16:37 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 08/01/2017 09:57 AM, Dong Jia Shi wrote:
> [..]
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -1745,10 +1745,10 @@ int css_do_rchp(uint8_t cssid, uint8_t chpid)
> >      }
> > 
> >      /* We don't really use a channel path, so we're done here. */
> > -    css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT,
> > +    css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1,
> >                    channel_subsys.max_cssid > 0 ? 1 : 0, chpid);
> >      if (channel_subsys.max_cssid > 0) {
> > -        css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 0, real_cssid << 8);
> > +        css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1, 0, real_cssid << 8);
> >      }
> >      return 0;
> >  }
> > @@ -2028,7 +2028,8 @@ void css_subch_assign(uint8_t cssid, uint8_t ssid, uint16_t schid,
> >      }
> >  }
> > 
> > -void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid)
> > +void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
> > +                   int chain, uint16_t rsid)  
> 
> I think you could make the parameters solicited and chain bool (AFAIU
> they are conceptually bool) for clearer semantic. If you go with that
> you could also get rid of the superfluous ternary operator ( we have
> stuff like some_cond ? 1 : 0 for the chain parameter in more than
> one place.

Just adding the new parameter is the minimum change, and we should keep
it consistent. I'm not convinced about converting to bool yet.

> 
> Btw. I find bool flags easy to mix up and difficult to read. I have no better
> idea how to write this (in C) though. I was considering throwing chain and
> solicited together into a single flags parameter, but looking at the client code
> it does not look like a good idea.

Yes, combining them would do nothing for the code.