| Message ID | 20170801075727.31335-3-bjsdjshi@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
* 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!
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.
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);
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(-)