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  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.

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

[Dong Jia Shi]

* Halil Pasic  [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 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

[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 
Signed-off-by: Dong Jia Shi 
---
 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