[Qemu-devel] [PATCH 0/3] Channel Path realted CRW generation

[Qemu-devel] [PATCH 0/3] Channel Path realted CRW generation

[Dong Jia Shi]

This series is trying to:
1. clear up CRW related code.
2. generate the right channel path related CRW at the right time.

I did this mainly because it's a requirement from my current work, that is I'm
in preparation of a group of patch for channel path virtualization. I can use
the inerface that provided by this series later, so as to, for vfio-ccw
devices, notify the guest with channel path status vary that happens on the
host side.

During an internal discussion, Halil and Pierre pointed out that for path
hotplug, generating a CRW seems logical, but how is it covered by the AR is not
clear - we have problem in understanding some grammar ambiguous paragraphs.
While certain parts of the AR is not available outside, but I'm still wondering
if the author ;) could give us some clue... BTW, we know that, in Linux kernel
we had code that handles un-solicited chp crw, so we tend to believe it's right
to generate channel path initialized CRW for path hotplug. It's just we can not
find the reason from the document.

Pierre also suggested to add an @erc param for css_generate_chp_crws() in patch3,
while others have a different opinion. This is for your consideration.

Best regards!

Dong Jia Shi (3):
  s390x/css: use macro for event-information pending error recover code
  s390x/css: generate solicited crw for rchp completion signaling
  s390x/css: generate channel path initialized CRW for channel path
    hotplug

 hw/s390x/3270-ccw.c       |  3 ++-
 hw/s390x/css.c            | 66 +++++++++++++++++++++++++++++++++++------------
 hw/s390x/s390-ccw.c       |  2 +-
 hw/s390x/virtio-ccw.c     |  3 ++-
 include/hw/s390x/css.h    | 10 ++++---
 include/hw/s390x/ioinst.h |  6 +++--
 6 files changed, 64 insertions(+), 26 deletions(-)

-- 
2.11.2

[Qemu-devel] [PATCH 1/3] 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).

Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
---
 hw/s390x/css.c            | 2 +-
 include/hw/s390x/ioinst.h | 5 +++--
 2 files changed, 4 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..c1d1052279 100644
--- a/include/hw/s390x/ioinst.h
+++ b/include/hw/s390x/ioinst.h
@@ -201,8 +201,9 @@ 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
+#define CRW_ERC_INIT   0x02
+#define CRW_ERC_IPI    0x04
 
 #define CRW_RSC_SUBCH 0x3
 #define CRW_RSC_CHP   0x4
-- 
2.11.2

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

[Cornelia Huck]

On Thu, 27 Jul 2017 03:54:16 +0200
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

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

Sounds reasonable.

> 
> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c            | 2 +-
>  include/hw/s390x/ioinst.h | 5 +++--
>  2 files changed, 4 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..c1d1052279 100644
> --- a/include/hw/s390x/ioinst.h
> +++ b/include/hw/s390x/ioinst.h
> @@ -201,8 +201,9 @@ 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

OK, that matches the name the Linux kernel uses.

Do we want to add all ERCs while we're at it?

> +#define CRW_ERC_INIT   0x02
> +#define CRW_ERC_IPI    0x04
>  
>  #define CRW_RSC_SUBCH 0x3
>  #define CRW_RSC_CHP   0x4

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

[Dong Jia Shi]

* Cornelia Huck <cohuck@redhat.com> [2017-07-27 12:10:17 +0200]:

[...]

> > diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h
> > index 92d15655e4..c1d1052279 100644
> > --- a/include/hw/s390x/ioinst.h
> > +++ b/include/hw/s390x/ioinst.h
> > @@ -201,8 +201,9 @@ 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
> 
> OK, that matches the name the Linux kernel uses.
A thief was caught by you. ;)

> 
> Do we want to add all ERCs while we're at it?
> 
> > +#define CRW_ERC_INIT   0x02
> > +#define CRW_ERC_IPI    0x04
No problem for me. I can do that by stealing again:

#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 */

Want the comment or not? I like them.

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

-- 
Dong Jia Shi

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

[Cornelia Huck]

On Fri, 28 Jul 2017 15:12:25 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Cornelia Huck <cohuck@redhat.com> [2017-07-27 12:10:17 +0200]:

> > Do we want to add all ERCs while we're at it?
> >   
> > > +#define CRW_ERC_INIT   0x02
> > > +#define CRW_ERC_IPI    0x04  
> No problem for me. I can do that by stealing again:
> 
> #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 */
> 
> Want the comment or not? I like them.

Yes, just do it.

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

[Qemu-devel] [PATCH 2/3] 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         | 15 +++++++++------
 include/hw/s390x/css.h |  2 +-
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 5321ca016b..60e1592d5c 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,7 @@ 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 s, int chain, uint16_t rsid)
 {
     CrwContainer *crw_cont;
 
@@ -2040,6 +2040,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 (s) {
+        crw_cont->crw.flags |= CRW_FLAGS_MASK_S;
+    }
     if (chain) {
         crw_cont->crw.flags |= CRW_FLAGS_MASK_C;
     }
@@ -2086,9 +2089,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 +2106,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..d03b4ffeac 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -150,7 +150,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(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid);
+void css_queue_crw(uint8_t rsc, uint8_t erc, int s, 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 2/3] s390x/css: generate solicited crw for rchp completion signaling

[Cornelia Huck]

On Thu, 27 Jul 2017 03:54:17 +0200
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

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

Sounds legit.

> 
> Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c         | 15 +++++++++------
>  include/hw/s390x/css.h |  2 +-
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 5321ca016b..60e1592d5c 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,7 @@ 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 s, int chain, uint16_t rsid)

's' is not a very speaking name...

>  {
>      CrwContainer *crw_cont;
>  
> @@ -2040,6 +2040,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 (s) {
> +        crw_cont->crw.flags |= CRW_FLAGS_MASK_S;

...as it obviously causes the S flag to be set ;) Let's call it 'solicited'?

> +    }
>      if (chain) {
>          crw_cont->crw.flags |= CRW_FLAGS_MASK_C;
>      }
> @@ -2086,9 +2089,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 +2106,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);

Should we want to support OS-triggered channel path vary (via SCLP or
otherwise) in the future, we'll probably need a version that generates a
solicited crw.

>      }
>      channel_subsys.sei_pending = true;
>  }
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 5c5fe6b202..d03b4ffeac 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -150,7 +150,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(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid);
> +void css_queue_crw(uint8_t rsc, uint8_t erc, int s, 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);

Otherwise, patch looks good.

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

[Dong Jia Shi]

* Cornelia Huck <cohuck@redhat.com> [2017-07-27 13:22:59 +0200]:

[...]

> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
[...]

> > @@ -2028,7 +2028,7 @@ 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 s, int chain, uint16_t rsid)
> 
> 's' is not a very speaking name...
> 
> >  {
> >      CrwContainer *crw_cont;
> >  
> > @@ -2040,6 +2040,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 (s) {
> > +        crw_cont->crw.flags |= CRW_FLAGS_MASK_S;
> 
> ...as it obviously causes the S flag to be set ;) Let's call it 'solicited'?
Sure. Will adopt.

> 
> > +    }
> >      if (chain) {
> >          crw_cont->crw.flags |= CRW_FLAGS_MASK_C;
> >      }
> > @@ -2086,9 +2089,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 +2106,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);
> 
> Should we want to support OS-triggered channel path vary (via SCLP or
> otherwise) in the future,
Yes! I had a prototype of series to handle the OS-triggered chp vary,
and that needs...

> we'll probably need a version that generates a solicited crw.
...the new interface with the solicited bit param, which is provided by
patch #3:
void css_generate_chp_crws(uint8_t cssid, uint8_t chpid,
                           int hotplugged, int add, int s);

BTW, I need to renew the legal clearance before sending them out...

> 
> >      }
> >      channel_subsys.sei_pending = true;
> >  }
> > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> > index 5c5fe6b202..d03b4ffeac 100644
> > --- a/include/hw/s390x/css.h
> > +++ b/include/hw/s390x/css.h
> > @@ -150,7 +150,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(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid);
> > +void css_queue_crw(uint8_t rsc, uint8_t erc, int s, 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);
> 
> Otherwise, patch looks good.
Thanks.

So, I only need to s/s/solicited for the new version of this one?

> 

-- 
Dong Jia Shi

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

[Cornelia Huck]

On Fri, 28 Jul 2017 15:25:26 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> So, I only need to s/s/solicited for the new version of this one?

Yes, that would be fine.

[Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Dong Jia Shi]

When a channel path is hot plugged into a CSS, we should generate
a channel path initialized CRW (channel report word). The current
code does not do that, instead it puts a stub function with a TODO
reminder there.

This implements the css_generate_chp_crws() function by:
1. refactor the existing code.
2. add an @add parameter to provide future callers with the
   capability of generating channel path permanent error with
   facility not initialized CRW.
3. add a @hotplugged parameter, so to opt out generating initialized
   CRWs for predefined channel paths.

Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
---
 hw/s390x/3270-ccw.c       |  3 ++-
 hw/s390x/css.c            | 55 ++++++++++++++++++++++++++++++++++++-----------
 hw/s390x/s390-ccw.c       |  2 +-
 hw/s390x/virtio-ccw.c     |  3 ++-
 include/hw/s390x/css.h    |  8 ++++---
 include/hw/s390x/ioinst.h |  1 +
 6 files changed, 53 insertions(+), 19 deletions(-)

diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c
index 1554aa2484..20305b8554 100644
--- a/hw/s390x/3270-ccw.c
+++ b/hw/s390x/3270-ccw.c
@@ -125,7 +125,8 @@ static void emulated_ccw_3270_realize(DeviceState *ds, Error **errp)
     sch->id.reserved = 0xff;
     sch->id.cu_type = EMULATED_CCW_3270_CU_TYPE;
     css_sch_build_virtual_schib(sch, (uint8_t)chpid,
-                                EMULATED_CCW_3270_CHPID_TYPE);
+                                EMULATED_CCW_3270_CHPID_TYPE,
+                                parent->hotplugged);
     sch->do_subchannel_work = do_subchannel_work_virtual;
     sch->ccw_cb = emulated_ccw_3270_cb;
 
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 60e1592d5c..ffa93e8a62 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1745,11 +1745,8 @@ 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, 1,
-                  channel_subsys.max_cssid > 0 ? 1 : 0, chpid);
-    if (channel_subsys.max_cssid > 0) {
-        css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1, 0, real_cssid << 8);
-    }
+    css_generate_chp_crws(cssid, chpid, 0, 1, 1);
+
     return 0;
 }
 
@@ -1791,7 +1788,7 @@ unsigned int css_find_free_chpid(uint8_t cssid)
 }
 
 static int css_add_chpid(uint8_t cssid, uint8_t chpid, uint8_t type,
-                         bool is_virt)
+                         bool is_virt, int hotplugged)
 {
     CssImage *css;
 
@@ -1807,12 +1804,13 @@ static int css_add_chpid(uint8_t cssid, uint8_t chpid, uint8_t type,
     css->chpids[chpid].type = type;
     css->chpids[chpid].is_virtual = is_virt;
 
-    css_generate_chp_crws(cssid, chpid);
+    css_generate_chp_crws(cssid, chpid, hotplugged, 1, 0);
 
     return 0;
 }
 
-void css_sch_build_virtual_schib(SubchDev *sch, uint8_t chpid, uint8_t type)
+void css_sch_build_virtual_schib(SubchDev *sch, uint8_t chpid, uint8_t type,
+                                 int hotplugged)
 {
     PMCW *p = &sch->curr_status.pmcw;
     SCSW *s = &sch->curr_status.scsw;
@@ -1829,7 +1827,7 @@ void css_sch_build_virtual_schib(SubchDev *sch, uint8_t chpid, uint8_t type)
     p->pam = 0x80;
     p->chpid[0] = chpid;
     if (!css->chpids[chpid].in_use) {
-        css_add_chpid(sch->cssid, chpid, type, true);
+        css_add_chpid(sch->cssid, chpid, type, true, hotplugged);
     }
 
     memset(s, 0, sizeof(SCSW));
@@ -2098,9 +2096,40 @@ void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, uint16_t schid,
     css_clear_io_interrupt(css_do_build_subchannel_id(cssid, ssid), schid);
 }
 
-void css_generate_chp_crws(uint8_t cssid, uint8_t chpid)
+void css_generate_chp_crws(uint8_t cssid, uint8_t chpid,
+                           int hotplugged, int add, int s)
 {
-    /* TODO */
+    uint8_t guest_cssid;
+    bool chain_crw;
+    int erc;
+
+    if (add && !s && !hotplugged) {
+        return;
+    }
+    if (channel_subsys.max_cssid == 0) {
+        /* Default cssid shows up as 0. */
+        guest_cssid = (cssid == channel_subsys.default_cssid) ? 0 : cssid;
+    } else {
+        /* Show real cssid to the guest. */
+        guest_cssid = cssid;
+    }
+    /*
+     * Only notify for higher subchannel sets/channel subsystems if the
+     * guest has enabled it.
+     */
+    if ((guest_cssid > channel_subsys.max_cssid) ||
+        ((channel_subsys.max_cssid == 0) &&
+         (cssid != channel_subsys.default_cssid))) {
+        return;
+    }
+
+    erc = add ? CRW_ERC_INIT : CRW_ERC_PERRN;
+    chain_crw = channel_subsys.max_cssid > 0;
+
+    css_queue_crw(CRW_RSC_CHP, erc, s, chain_crw ? 1 : 0, chpid);
+    if (chain_crw) {
+        css_queue_crw(CRW_RSC_CHP, erc, s, 0, cssid << 8);
+    }
 }
 
 void css_generate_css_crws(uint8_t cssid)
@@ -2433,7 +2462,7 @@ static int css_sch_get_chpid_type(uint8_t chpid, uint32_t *type,
  * guest subchannel information block without considering the migration feature.
  * We need to revisit this problem when we want to add migration support.
  */
-int css_sch_build_schib(SubchDev *sch, CssDevId *dev_id)
+int css_sch_build_schib(SubchDev *sch, CssDevId *dev_id, int hotplugged)
 {
     CssImage *css = channel_subsys.css[sch->cssid];
     PMCW *p = &sch->curr_status.pmcw;
@@ -2466,7 +2495,7 @@ int css_sch_build_schib(SubchDev *sch, CssDevId *dev_id)
             if (ret) {
                 return ret;
             }
-            css_add_chpid(sch->cssid, p->chpid[i], type, false);
+            css_add_chpid(sch->cssid, p->chpid[i], type, false, hotplugged);
         }
     }
 
diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index 8614dda6f8..96d9a28719 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -86,7 +86,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
     sch->do_subchannel_work = do_subchannel_work_passthrough;
 
     ccw_dev->sch = sch;
-    ret = css_sch_build_schib(sch, &cdev->hostid);
+    ret = css_sch_build_schib(sch, &cdev->hostid, parent->hotplugged);
     if (ret) {
         error_setg_errno(&err, -ret, "%s: Failed to build initial schib",
                          __func__);
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index b1976fdd19..9c1642e51d 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -783,7 +783,8 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev, Error **errp)
     ccw_dev->sch = sch;
     dev->indicators = NULL;
     dev->revision = -1;
-    css_sch_build_virtual_schib(sch, 0, VIRTIO_CCW_CHPID_TYPE);
+    css_sch_build_virtual_schib(sch, 0, VIRTIO_CCW_CHPID_TYPE,
+                                parent->hotplugged);
 
     trace_virtio_ccw_new_device(
         sch->cssid, sch->ssid, sch->schid, sch->devno,
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index d03b4ffeac..a8344ea360 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -142,8 +142,9 @@ int css_create_css_image(uint8_t cssid, bool default_image);
 bool css_devno_used(uint8_t cssid, uint8_t ssid, uint16_t devno);
 void css_subch_assign(uint8_t cssid, uint8_t ssid, uint16_t schid,
                       uint16_t devno, SubchDev *sch);
-void css_sch_build_virtual_schib(SubchDev *sch, uint8_t chpid, uint8_t type);
-int css_sch_build_schib(SubchDev *sch, CssDevId *dev_id);
+void css_sch_build_virtual_schib(SubchDev *sch, uint8_t chpid, uint8_t type,
+                                 int hotplugged);
+int css_sch_build_schib(SubchDev *sch, CssDevId *dev_id, int hotplugged);
 unsigned int css_find_free_chpid(uint8_t cssid);
 uint16_t css_build_subchannel_id(SubchDev *sch);
 void copy_scsw_to_guest(SCSW *dest, const SCSW *src);
@@ -153,7 +154,8 @@ void css_reset_sch(SubchDev *sch);
 void css_queue_crw(uint8_t rsc, uint8_t erc, int s, 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);
+void css_generate_chp_crws(uint8_t cssid, uint8_t chpid,
+                           int hotplugged, int add, int s);
 void css_generate_css_crws(uint8_t cssid);
 void css_clear_sei_pending(void);
 int s390_ccw_cmd_request(ORB *orb, SCSW *scsw, void *data);
diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h
index c1d1052279..ae2c230948 100644
--- a/include/hw/s390x/ioinst.h
+++ b/include/hw/s390x/ioinst.h
@@ -204,6 +204,7 @@ typedef struct CRW {
 #define CRW_ERC_EVENT  0x00
 #define CRW_ERC_INIT   0x02
 #define CRW_ERC_IPI    0x04
+#define CRW_ERC_PERRN  0x06
 
 #define CRW_RSC_SUBCH 0x3
 #define CRW_RSC_CHP   0x4
-- 
2.11.2

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Cornelia Huck]

On Thu, 27 Jul 2017 03:54:18 +0200
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> When a channel path is hot plugged into a CSS, we should generate
> a channel path initialized CRW (channel report word). The current
> code does not do that, instead it puts a stub function with a TODO
> reminder there.
> 
> This implements the css_generate_chp_crws() function by:
> 1. refactor the existing code.
> 2. add an @add parameter to provide future callers with the
>    capability of generating channel path permanent error with
>    facility not initialized CRW.
> 3. add a @hotplugged parameter, so to opt out generating initialized
>    CRWs for predefined channel paths.

I'm not 100% sure whether the logic is correct here. Let me elaborate:

The current code flow when hotplugging a device is:
- Generate the schib.
- Check if any of the chpids refers to a not yet existing channel path;
  generate it if that is the case.
- Post a crw for the subchannel.

The second step is where the current code seems to be not quite correct
already. It is fine for coldplugged devices, but I really think we need
to make sure that all referenced channel paths are in place before we
hotplug a new device. It was not really relevant when we just had one
very virtual channel path, and 3270 is experimental so it is not a
problem in practice.

This, of course, implies we need deeper changes. We need to create the
channel paths before the subchannel is created and refuse hotplug of a
device if not all channel paths it needs are defined. This means we
need some things before we can claim real channel path support:
- Have a way to specify channel paths on the command line resp. when
  hotplugging. This implies they need to be real objects.
- Have a way to specify which channel paths belong to a subchannel in
  the same context. Keep existing device types working with the current
  method.
- Give channel paths states: Defined, configured. The right time for a
  CRW is the transition between those states.
- Only queue a 'device come' CRW for a subchannel if at least one of
  its channel paths is in the configured state. Detach or make not
  operational a subchannel if all of its paths are deconfigured.

Something along those lines also matches better what I've seen on z/VM
or LPAR. I realize that it's not easy :(

tl;dr: I don't think we want chp crws until after we have a good chp
model.

> 
> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> ---
>  hw/s390x/3270-ccw.c       |  3 ++-
>  hw/s390x/css.c            | 55 ++++++++++++++++++++++++++++++++++++-----------
>  hw/s390x/s390-ccw.c       |  2 +-
>  hw/s390x/virtio-ccw.c     |  3 ++-
>  include/hw/s390x/css.h    |  8 ++++---
>  include/hw/s390x/ioinst.h |  1 +
>  6 files changed, 53 insertions(+), 19 deletions(-)

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Halil Pasic]

On 07/27/2017 01:59 PM, Cornelia Huck wrote:
> On Thu, 27 Jul 2017 03:54:18 +0200
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
>> When a channel path is hot plugged into a CSS, we should generate
>> a channel path initialized CRW (channel report word). The current
>> code does not do that, instead it puts a stub function with a TODO
>> reminder there.
>>
>> This implements the css_generate_chp_crws() function by:
>> 1. refactor the existing code.
>> 2. add an @add parameter to provide future callers with the
>>    capability of generating channel path permanent error with
>>    facility not initialized CRW.
>> 3. add a @hotplugged parameter, so to opt out generating initialized
>>    CRWs for predefined channel paths.
> 
> I'm not 100% sure whether the logic is correct here. Let me elaborate:
> 
> The current code flow when hotplugging a device is:
> - Generate the schib.
> - Check if any of the chpids refers to a not yet existing channel path;
>   generate it if that is the case.
> - Post a crw for the subchannel.
> 
> The second step is where the current code seems to be not quite correct
> already. It is fine for coldplugged devices, but I really think we need
> to make sure that all referenced channel paths are in place before we
> hotplug a new device. It was not really relevant when we just had one
> very virtual channel path, and 3270 is experimental so it is not a
> problem in practice.

What do you mean by not quite correct?  Are we somewhere in conflict with
the AR (if yes could you give me a pointer)? Or is it a modeling concern?
Or is it about the user interface design? Or any combination of these?

> 
> This, of course, implies we need deeper changes. We need to create the
> channel paths before the subchannel is created and refuse hotplug of a
> device if not all channel paths it needs are defined. This means we
> need some things before we can claim real channel path support:
> - Have a way to specify channel paths on the command line resp. when
>   hotplugging. This implies they need to be real objects.
> - Have a way to specify which channel paths belong to a subchannel in
>   the same context. Keep existing device types working with the current
>   method.
> - Give channel paths states: Defined, configured. The right time for a
>   CRW is the transition between those states.
> - Only queue a 'device come' CRW for a subchannel if at least one of
>   its channel paths is in the configured state. Detach or make not
>   operational a subchannel if all of its paths are deconfigured.
> 

AFAIU in your opinion our model is to simple and needs to get more
complex. What benefits do we expect from the added complexity? I mean
our current model works (and I'm not sure what limitations do we
want to get rid of, or even what are the relevant limitations we have.)

> Something along those lines also matches better what I've seen on z/VM
> or LPAR. I realize that it's not easy :(
> 
> tl;dr: I don't think we want chp crws until after we have a good chp
> model.

I fully agree with this point.

Regards,
Halil

> 
>>
>> Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/3270-ccw.c       |  3 ++-
>>  hw/s390x/css.c            | 55 ++++++++++++++++++++++++++++++++++++-----------
>>  hw/s390x/s390-ccw.c       |  2 +-
>>  hw/s390x/virtio-ccw.c     |  3 ++-
>>  include/hw/s390x/css.h    |  8 ++++---
>>  include/hw/s390x/ioinst.h |  1 +
>>  6 files changed, 53 insertions(+), 19 deletions(-)
> 

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Cornelia Huck]

On Thu, 27 Jul 2017 15:37:08 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 07/27/2017 01:59 PM, Cornelia Huck wrote:
> > On Thu, 27 Jul 2017 03:54:18 +0200
> > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> >   
> >> When a channel path is hot plugged into a CSS, we should generate
> >> a channel path initialized CRW (channel report word). The current
> >> code does not do that, instead it puts a stub function with a TODO
> >> reminder there.
> >>
> >> This implements the css_generate_chp_crws() function by:
> >> 1. refactor the existing code.
> >> 2. add an @add parameter to provide future callers with the
> >>    capability of generating channel path permanent error with
> >>    facility not initialized CRW.
> >> 3. add a @hotplugged parameter, so to opt out generating initialized
> >>    CRWs for predefined channel paths.  
> > 
> > I'm not 100% sure whether the logic is correct here. Let me elaborate:
> > 
> > The current code flow when hotplugging a device is:
> > - Generate the schib.
> > - Check if any of the chpids refers to a not yet existing channel path;
> >   generate it if that is the case.
> > - Post a crw for the subchannel.
> > 
> > The second step is where the current code seems to be not quite correct
> > already. It is fine for coldplugged devices, but I really think we need
> > to make sure that all referenced channel paths are in place before we
> > hotplug a new device. It was not really relevant when we just had one
> > very virtual channel path, and 3270 is experimental so it is not a
> > problem in practice.  
> 
> What do you mean by not quite correct?  Are we somewhere in conflict with
> the AR (if yes could you give me a pointer)? Or is it a modeling concern?
> Or is it about the user interface design? Or any combination of these?

Modeling. Conceptually, you need at least one channel path to access a
device; the subchannel is more or less a followup.

As up to now, we only had a dummy channel path simply to satisfy the
architecture, it did not really matter when it was 'created'. As it is
always 'there' from beginning, there was no need for any CRW either. If
there's now a need for real channel path modeling, we unfortunately
have to give this some thought :)

> 
> > 
> > This, of course, implies we need deeper changes. We need to create the
> > channel paths before the subchannel is created and refuse hotplug of a
> > device if not all channel paths it needs are defined. This means we
> > need some things before we can claim real channel path support:
> > - Have a way to specify channel paths on the command line resp. when
> >   hotplugging. This implies they need to be real objects.
> > - Have a way to specify which channel paths belong to a subchannel in
> >   the same context. Keep existing device types working with the current
> >   method.
> > - Give channel paths states: Defined, configured. The right time for a
> >   CRW is the transition between those states.
> > - Only queue a 'device come' CRW for a subchannel if at least one of
> >   its channel paths is in the configured state. Detach or make not
> >   operational a subchannel if all of its paths are deconfigured.
> >   
> 
> AFAIU in your opinion our model is to simple and needs to get more
> complex. What benefits do we expect from the added complexity? I mean
> our current model works (and I'm not sure what limitations do we
> want to get rid of, or even what are the relevant limitations we have.)

I was under the impression that we want to support multiple, 'real'
channel paths for vfio-ccw. Did I misunderstand?

> 
> > Something along those lines also matches better what I've seen on z/VM
> > or LPAR. I realize that it's not easy :(
> > 
> > tl;dr: I don't think we want chp crws until after we have a good chp
> > model.  
> 
> I fully agree with this point.

Great.

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Halil Pasic]

On 07/27/2017 04:14 PM, Cornelia Huck wrote:
> On Thu, 27 Jul 2017 15:37:08 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 07/27/2017 01:59 PM, Cornelia Huck wrote:
>>> On Thu, 27 Jul 2017 03:54:18 +0200
>>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
>>>   
>>>> When a channel path is hot plugged into a CSS, we should generate
>>>> a channel path initialized CRW (channel report word). The current
>>>> code does not do that, instead it puts a stub function with a TODO
>>>> reminder there.
>>>>
>>>> This implements the css_generate_chp_crws() function by:
>>>> 1. refactor the existing code.
>>>> 2. add an @add parameter to provide future callers with the
>>>>    capability of generating channel path permanent error with
>>>>    facility not initialized CRW.
>>>> 3. add a @hotplugged parameter, so to opt out generating initialized
>>>>    CRWs for predefined channel paths.  
>>>
>>> I'm not 100% sure whether the logic is correct here. Let me elaborate:
>>>
>>> The current code flow when hotplugging a device is:
>>> - Generate the schib.
>>> - Check if any of the chpids refers to a not yet existing channel path;
>>>   generate it if that is the case.
>>> - Post a crw for the subchannel.
>>>
>>> The second step is where the current code seems to be not quite correct
>>> already. It is fine for coldplugged devices, but I really think we need
>>> to make sure that all referenced channel paths are in place before we
>>> hotplug a new device. It was not really relevant when we just had one
>>> very virtual channel path, and 3270 is experimental so it is not a
>>> problem in practice.  
>>
>> What do you mean by not quite correct?  Are we somewhere in conflict with
>> the AR (if yes could you give me a pointer)? Or is it a modeling concern?
>> Or is it about the user interface design? Or any combination of these?
> 
> Modeling. Conceptually, you need at least one channel path to access a
> device; the subchannel is more or less a followup.
> 
> As up to now, we only had a dummy channel path simply to satisfy the
> architecture, it did not really matter when it was 'created'. As it is
> always 'there' from beginning, there was no need for any CRW either. If
> there's now a need for real channel path modeling, we unfortunately
> have to give this some thought :)
> 

We have considered this 'always there' internally. Was my train of thought
to but then I came to the conclusion it ain't necessarily true. One can
IPL with initrd and end up with a guest with no ccw devices whatsoever
(IMHO; the default net can be disabled -net none).

>>
>>>
>>> This, of course, implies we need deeper changes. We need to create the
>>> channel paths before the subchannel is created and refuse hotplug of a
>>> device if not all channel paths it needs are defined. This means we
>>> need some things before we can claim real channel path support:
>>> - Have a way to specify channel paths on the command line resp. when
>>>   hotplugging. This implies they need to be real objects.
>>> - Have a way to specify which channel paths belong to a subchannel in
>>>   the same context. Keep existing device types working with the current
>>>   method.
>>> - Give channel paths states: Defined, configured. The right time for a
>>>   CRW is the transition between those states.
>>> - Only queue a 'device come' CRW for a subchannel if at least one of
>>>   its channel paths is in the configured state. Detach or make not
>>>   operational a subchannel if all of its paths are deconfigured.
>>>   
>>
>> AFAIU in your opinion our model is to simple and needs to get more
>> complex. What benefits do we expect from the added complexity? I mean
>> our current model works (and I'm not sure what limitations do we
>> want to get rid of, or even what are the relevant limitations we have.)
> 
> I was under the impression that we want to support multiple, 'real'
> channel paths for vfio-ccw. Did I misunderstand?
> 

You assumed correctly. I've asked Dong Jia a very similar question when
this first popped up as a part of his channel path virtualization work
(he is referring to at the beginning of this cover letter).

Unfortunately that question is still unanswered. I've speculated maybe you
were involved in the decision of opting for 'real' channel paths, so this
is why I asked.

So my intention was to ask: What benefits do we expect from these 'real'
virtual channel paths? 

(I believe models make sense (and can be judged) only in a context of a
problem. So before voting for a more complex model, I would like too
understand the problem.)

Regards,
Halil

>>
>>> Something along those lines also matches better what I've seen on z/VM
>>> or LPAR. I realize that it's not easy :(
>>>
>>> tl;dr: I don't think we want chp crws until after we have a good chp
>>> model.  
>>
>> I fully agree with this point.
> 
> Great.
> 

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Cornelia Huck]

On Thu, 27 Jul 2017 18:15:07 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 07/27/2017 04:14 PM, Cornelia Huck wrote:
> > On Thu, 27 Jul 2017 15:37:08 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> On 07/27/2017 01:59 PM, Cornelia Huck wrote:  
> >>> On Thu, 27 Jul 2017 03:54:18 +0200
> >>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> >>>     
> >>>> When a channel path is hot plugged into a CSS, we should generate
> >>>> a channel path initialized CRW (channel report word). The current
> >>>> code does not do that, instead it puts a stub function with a TODO
> >>>> reminder there.
> >>>>
> >>>> This implements the css_generate_chp_crws() function by:
> >>>> 1. refactor the existing code.
> >>>> 2. add an @add parameter to provide future callers with the
> >>>>    capability of generating channel path permanent error with
> >>>>    facility not initialized CRW.
> >>>> 3. add a @hotplugged parameter, so to opt out generating initialized
> >>>>    CRWs for predefined channel paths.    
> >>>
> >>> I'm not 100% sure whether the logic is correct here. Let me elaborate:
> >>>
> >>> The current code flow when hotplugging a device is:
> >>> - Generate the schib.
> >>> - Check if any of the chpids refers to a not yet existing channel path;
> >>>   generate it if that is the case.
> >>> - Post a crw for the subchannel.
> >>>
> >>> The second step is where the current code seems to be not quite correct
> >>> already. It is fine for coldplugged devices, but I really think we need
> >>> to make sure that all referenced channel paths are in place before we
> >>> hotplug a new device. It was not really relevant when we just had one
> >>> very virtual channel path, and 3270 is experimental so it is not a
> >>> problem in practice.    
> >>
> >> What do you mean by not quite correct?  Are we somewhere in conflict with
> >> the AR (if yes could you give me a pointer)? Or is it a modeling concern?
> >> Or is it about the user interface design? Or any combination of these?  
> > 
> > Modeling. Conceptually, you need at least one channel path to access a
> > device; the subchannel is more or less a followup.
> > 
> > As up to now, we only had a dummy channel path simply to satisfy the
> > architecture, it did not really matter when it was 'created'. As it is
> > always 'there' from beginning, there was no need for any CRW either. If
> > there's now a need for real channel path modeling, we unfortunately
> > have to give this some thought :)
> >   
> 
> We have considered this 'always there' internally. Was my train of thought
> to but then I came to the conclusion it ain't necessarily true. One can
> IPL with initrd and end up with a guest with no ccw devices whatsoever
> (IMHO; the default net can be disabled -net none).

Yes. But this is an edge case that was never relevant. We should do it
correctly, of course; but it isn't a problem in practice.

(If I'm not mistaken, Linux scans for chpids it does not know yet on
device hotplug anyway, due to the reasons I outlined in my other mail.)

> 
> >>  
> >>>
> >>> This, of course, implies we need deeper changes. We need to create the
> >>> channel paths before the subchannel is created and refuse hotplug of a
> >>> device if not all channel paths it needs are defined. This means we
> >>> need some things before we can claim real channel path support:
> >>> - Have a way to specify channel paths on the command line resp. when
> >>>   hotplugging. This implies they need to be real objects.
> >>> - Have a way to specify which channel paths belong to a subchannel in
> >>>   the same context. Keep existing device types working with the current
> >>>   method.
> >>> - Give channel paths states: Defined, configured. The right time for a
> >>>   CRW is the transition between those states.
> >>> - Only queue a 'device come' CRW for a subchannel if at least one of
> >>>   its channel paths is in the configured state. Detach or make not
> >>>   operational a subchannel if all of its paths are deconfigured.
> >>>     
> >>
> >> AFAIU in your opinion our model is to simple and needs to get more
> >> complex. What benefits do we expect from the added complexity? I mean
> >> our current model works (and I'm not sure what limitations do we
> >> want to get rid of, or even what are the relevant limitations we have.)  
> > 
> > I was under the impression that we want to support multiple, 'real'
> > channel paths for vfio-ccw. Did I misunderstand?
> >   
> 
> You assumed correctly. I've asked Dong Jia a very similar question when
> this first popped up as a part of his channel path virtualization work
> (he is referring to at the beginning of this cover letter).
> 
> Unfortunately that question is still unanswered. I've speculated maybe you
> were involved in the decision of opting for 'real' channel paths, so this
> is why I asked.
> 
> So my intention was to ask: What benefits do we expect from these 'real'
> virtual channel paths? 

Path grouping and friends come to mind. This depends on whether you
want to pass-through channel paths to the guest, of course, but you
really need management to deal with things like reserve/release on ECKD
correctly. Also failover etc. Preferred channel paths are not relevant
on modern hardware anymore, fortunately (AFAIK).

> 
> (I believe models make sense (and can be judged) only in a context of a
> problem. So before voting for a more complex model, I would like too
> understand the problem.)
> 
> Regards,
> Halil
> 
> >>  
> >>> Something along those lines also matches better what I've seen on z/VM
> >>> or LPAR. I realize that it's not easy :(
> >>>
> >>> tl;dr: I don't think we want chp crws until after we have a good chp
> >>> model.    
> >>
> >> I fully agree with this point.  
> > 
> > Great.
> >   
> 

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Halil Pasic]

On 07/28/2017 12:11 PM, Cornelia Huck wrote:
> On Thu, 27 Jul 2017 18:15:07 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 07/27/2017 04:14 PM, Cornelia Huck wrote:
>>> On Thu, 27 Jul 2017 15:37:08 +0200
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>   
>>>> On 07/27/2017 01:59 PM, Cornelia Huck wrote:  
>>>>> On Thu, 27 Jul 2017 03:54:18 +0200
>>>>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
>>>>>     
>>>>>> When a channel path is hot plugged into a CSS, we should generate
>>>>>> a channel path initialized CRW (channel report word). The current
>>>>>> code does not do that, instead it puts a stub function with a TODO
>>>>>> reminder there.
>>>>>>
>>>>>> This implements the css_generate_chp_crws() function by:
>>>>>> 1. refactor the existing code.
>>>>>> 2. add an @add parameter to provide future callers with the
>>>>>>    capability of generating channel path permanent error with
>>>>>>    facility not initialized CRW.
>>>>>> 3. add a @hotplugged parameter, so to opt out generating initialized
>>>>>>    CRWs for predefined channel paths.    
>>>>>
>>>>> I'm not 100% sure whether the logic is correct here. Let me elaborate:
>>>>>
>>>>> The current code flow when hotplugging a device is:
>>>>> - Generate the schib.
>>>>> - Check if any of the chpids refers to a not yet existing channel path;
>>>>>   generate it if that is the case.
>>>>> - Post a crw for the subchannel.
>>>>>
>>>>> The second step is where the current code seems to be not quite correct
>>>>> already. It is fine for coldplugged devices, but I really think we need
>>>>> to make sure that all referenced channel paths are in place before we
>>>>> hotplug a new device. It was not really relevant when we just had one
>>>>> very virtual channel path, and 3270 is experimental so it is not a
>>>>> problem in practice.    
>>>>
>>>> What do you mean by not quite correct?  Are we somewhere in conflict with
>>>> the AR (if yes could you give me a pointer)? Or is it a modeling concern?
>>>> Or is it about the user interface design? Or any combination of these?  
>>>
>>> Modeling. Conceptually, you need at least one channel path to access a
>>> device; the subchannel is more or less a followup.
>>>
>>> As up to now, we only had a dummy channel path simply to satisfy the
>>> architecture, it did not really matter when it was 'created'. As it is
>>> always 'there' from beginning, there was no need for any CRW either. If
>>> there's now a need for real channel path modeling, we unfortunately
>>> have to give this some thought :)
>>>   
>>
>> We have considered this 'always there' internally. Was my train of thought
>> to but then I came to the conclusion it ain't necessarily true. One can
>> IPL with initrd and end up with a guest with no ccw devices whatsoever
>> (IMHO; the default net can be disabled -net none).
> 
> Yes. But this is an edge case that was never relevant. We should do it
> correctly, of course; but it isn't a problem in practice.
> 
> (If I'm not mistaken, Linux scans for chpids it does not know yet on
> device hotplug anyway, due to the reasons I outlined in my other mail.)
> 

Yes, I have tested the scenario described yesterday and it worked with
no problem.

>>
>>>>  
>>>>>
>>>>> This, of course, implies we need deeper changes. We need to create the
>>>>> channel paths before the subchannel is created and refuse hotplug of a
>>>>> device if not all channel paths it needs are defined. This means we
>>>>> need some things before we can claim real channel path support:
>>>>> - Have a way to specify channel paths on the command line resp. when
>>>>>   hotplugging. This implies they need to be real objects.
>>>>> - Have a way to specify which channel paths belong to a subchannel in
>>>>>   the same context. Keep existing device types working with the current
>>>>>   method.
>>>>> - Give channel paths states: Defined, configured. The right time for a
>>>>>   CRW is the transition between those states.
>>>>> - Only queue a 'device come' CRW for a subchannel if at least one of
>>>>>   its channel paths is in the configured state. Detach or make not
>>>>>   operational a subchannel if all of its paths are deconfigured.
>>>>>     
>>>>
>>>> AFAIU in your opinion our model is to simple and needs to get more
>>>> complex. What benefits do we expect from the added complexity? I mean
>>>> our current model works (and I'm not sure what limitations do we
>>>> want to get rid of, or even what are the relevant limitations we have.)  
>>>
>>> I was under the impression that we want to support multiple, 'real'
>>> channel paths for vfio-ccw. Did I misunderstand?
>>>   
>>
>> You assumed correctly. I've asked Dong Jia a very similar question when
>> this first popped up as a part of his channel path virtualization work
>> (he is referring to at the beginning of this cover letter).
>>
>> Unfortunately that question is still unanswered. I've speculated maybe you
>> were involved in the decision of opting for 'real' channel paths, so this
>> is why I asked.
>>
>> So my intention was to ask: What benefits do we expect from these 'real'
>> virtual channel paths? 
> 
> Path grouping and friends come to mind. This depends on whether you
> want to pass-through channel paths to the guest, of course, but you
> really need management to deal with things like reserve/release on ECKD
> correctly.

Pass-through means dedicated in this case (that is the passed trough paths
are not used by the host -- correct me if my understanding is wrong).

This whole multipath/grouping stuff is currently a gray spot for me. I've
tired to revisit the corresponding parts of the AR, but there ain't much
on it in the documents I have.

Is the protocol for managing multipath equipment (device, maybe also CU)
dependent? The PoP (and the IO AR) talk about set-multipath-mode type
of command and similar (disband, resign) but how this type of command
looks like -- no idea. From the kernel code one can learn more details,
but that's just an implementation.

Can you recommend me a publication where the controls you talk about
are specified?

> Also failover etc. Preferred channel paths are not relevant
> on modern hardware anymore, fortunately (AFAIK).
>

If I understand you correctly it ain't possible to handle these
in the host (and let the guest a simple 'non-real' virtual
channel path whose reliability depends on what the host does),
or?

Thanks for the explanations!

Halil

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Cornelia Huck]

On Fri, 28 Jul 2017 14:32:11 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 07/28/2017 12:11 PM, Cornelia Huck wrote:
> > On Thu, 27 Jul 2017 18:15:07 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> >> So my intention was to ask: What benefits do we expect from these 'real'
> >> virtual channel paths?   
> > 
> > Path grouping and friends come to mind. This depends on whether you
> > want to pass-through channel paths to the guest, of course, but you
> > really need management to deal with things like reserve/release on ECKD
> > correctly.  
> 
> Pass-through means dedicated in this case (that is the passed trough paths
> are not used by the host -- correct me if my understanding is wrong).

There's nothing that speaks against path sharing, I think. Especially
as e.g. SetPGID is "first one gets to set it".

> 
> This whole multipath/grouping stuff is currently a gray spot for me. I've
> tired to revisit the corresponding parts of the AR, but there ain't much
> on it in the documents I have.
> 
> Is the protocol for managing multipath equipment (device, maybe also CU)
> dependent? The PoP (and the IO AR) talk about set-multipath-mode type
> of command and similar (disband, resign) but how this type of command
> looks like -- no idea. From the kernel code one can learn more details,
> but that's just an implementation.

Path selection and friends should all be in the base I/O documentation
(maybe something in the common I/O device commands, as well.) Path
grouping is device-specific.

> 
> Can you recommend me a publication where the controls you talk about
> are specified?

For SetPGID etc., I'd recommend the DASD documentation (the one
specifying the channel commands supported). Don't have the publication
number handy, sorry.

> 
> > Also failover etc. Preferred channel paths are not relevant
> > on modern hardware anymore, fortunately (AFAIK).
> >  
> 
> If I understand you correctly it ain't possible to handle these
> in the host (and let the guest a simple 'non-real' virtual
> channel path whose reliability depends on what the host does),
> or?

It is possible. Mapping to a virtual channel path or not is basically a
design decision (IIRC, z/VM supports both).

Mapping everything to a virtual chpid basically concentrates all
path-related handling in the hypervisor. This allows for a dumb guest
OS, but can make errors really hard to debug from the guest side.

Exposing real channel paths to the guest means that the guest OS needs
to be able to deal with path-related things, but OTOH it has more
control. As I don't think we'll ever want to support a guest OS that
does not also run under LPAR, I'd prefer that way.

> 
> Thanks for the explanations!

You're welcome!

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Halil Pasic]

On 07/28/2017 02:58 PM, Cornelia Huck wrote:
> On Fri, 28 Jul 2017 14:32:11 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 07/28/2017 12:11 PM, Cornelia Huck wrote:
>>> On Thu, 27 Jul 2017 18:15:07 +0200
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>>>> So my intention was to ask: What benefits do we expect from these 'real'
>>>> virtual channel paths?   
>>>
>>> Path grouping and friends come to mind. This depends on whether you
>>> want to pass-through channel paths to the guest, of course, but you
>>> really need management to deal with things like reserve/release on ECKD
>>> correctly.  
>>
>> Pass-through means dedicated in this case (that is the passed trough paths
>> are not used by the host -- correct me if my understanding is wrong).
> 
> There's nothing that speaks against path sharing, I think.

That is a nice to hear. I could not form an opinion on this
myself yet. Theoretically we speak about shared physical resources here,
and in such situations I'm wary of interference. A quick look into
the AR documents was not conclusive.

I'm still trying to figure out this whole channel path handling,
and frankly you are a big help right now.

> Especially as e.g. SetPGID is "first one gets to set it".

Hm, I don't understand this. (I've found a description of SETPGID
in "IBM 3880 Storage Control Models ... " but could not get your
point based no that.)

> 
>>
>> This whole multipath/grouping stuff is currently a gray spot for me. I've
>> tired to revisit the corresponding parts of the AR, but there ain't much
>> on it in the documents I have.
>>
>> Is the protocol for managing multipath equipment (device, maybe also CU)
>> dependent? The PoP (and the IO AR) talk about set-multipath-mode type
>> of command and similar (disband, resign) but how this type of command
>> looks like -- no idea. From the kernel code one can learn more details,
>> but that's just an implementation.
> 
> Path selection and friends should all be in the base I/O documentation
> (maybe something in the common I/O device commands, as well.) Path
> grouping is device-specific.
> 

Nod.

>>
>> Can you recommend me a publication where the controls you talk about
>> are specified?
> 
> For SetPGID etc., I'd recommend the DASD documentation (the one
> specifying the channel commands supported). Don't have the publication
> number handy, sorry.
> 

Thanks, based on your input I've found the pub mentioned above.

>>
>>> Also failover etc. Preferred channel paths are not relevant
>>> on modern hardware anymore, fortunately (AFAIK).
>>>  
>>
>> If I understand you correctly it ain't possible to handle these
>> in the host (and let the guest a simple 'non-real' virtual
>> channel path whose reliability depends on what the host does),
>> or?
> 
> It is possible. Mapping to a virtual channel path or not is basically a
> design decision (IIRC, z/VM supports both).
> 
> Mapping everything to a virtual chpid basically concentrates all
> path-related handling in the hypervisor. This allows for a dumb guest
> OS, but can make errors really hard to debug from the guest side.
> 

IMHO the same is true for virtio for example (the abstraction
hides the backend and the backing: if there is a problem there it's
hard to debug from the guest side).

Because of my lack of understanding, this option appeared simpler to
me: clear ownership, and probably also less places where things can
go wrong.

> Exposing real channel paths to the guest means that the guest OS needs
> to be able to deal with path-related things, but OTOH it has more
> control. As I don't think we'll ever want to support a guest OS that
> does not also run under LPAR, I'd prefer that way.

Nod. And this makes a full circle, namely the question of benefit of
having more control. But since we did one full circle I'm much smarter
now than at the beginning.

Thank you very much for all the background information and for your
patience.

Regards,
Halil

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Cornelia Huck]

On Fri, 28 Jul 2017 16:29:14 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 07/28/2017 02:58 PM, Cornelia Huck wrote:
> > On Fri, 28 Jul 2017 14:32:11 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> On 07/28/2017 12:11 PM, Cornelia Huck wrote:  
> >>> On Thu, 27 Jul 2017 18:15:07 +0200
> >>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:  
> >   
> >>>> So my intention was to ask: What benefits do we expect from these 'real'
> >>>> virtual channel paths?     
> >>>
> >>> Path grouping and friends come to mind. This depends on whether you
> >>> want to pass-through channel paths to the guest, of course, but you
> >>> really need management to deal with things like reserve/release on ECKD
> >>> correctly.    
> >>
> >> Pass-through means dedicated in this case (that is the passed trough paths
> >> are not used by the host -- correct me if my understanding is wrong).  
> > 
> > There's nothing that speaks against path sharing, I think.  
> 
> That is a nice to hear. I could not form an opinion on this
> myself yet. Theoretically we speak about shared physical resources here,
> and in such situations I'm wary of interference. A quick look into
> the AR documents was not conclusive.

I'm afraid that much of it will be either underdocumented, confusingly
worded, or even druidic knowledge. Experimenting a bit might be helpful.

> 
> I'm still trying to figure out this whole channel path handling,
> and frankly you are a big help right now.

Thanks.

> 
> > Especially as e.g. SetPGID is "first one gets to set it".  
> 
> Hm, I don't understand this. (I've found a description of SETPGID
> in "IBM 3880 Storage Control Models ... " but could not get your
> point based no that.)

The first OS that does SetPGID after a reset (or removal of a PGID),
sets it. Subsequent SetPGIDs are rejected if the PGID does not match.
(See the SensePGID/SetPGID handling in the Linux common I/O layer -
this is needed e.g. for running under z/VM.)


> >>  
> >>> Also failover etc. Preferred channel paths are not relevant
> >>> on modern hardware anymore, fortunately (AFAIK).
> >>>    
> >>
> >> If I understand you correctly it ain't possible to handle these
> >> in the host (and let the guest a simple 'non-real' virtual
> >> channel path whose reliability depends on what the host does),
> >> or?  
> > 
> > It is possible. Mapping to a virtual channel path or not is basically a
> > design decision (IIRC, z/VM supports both).
> > 
> > Mapping everything to a virtual chpid basically concentrates all
> > path-related handling in the hypervisor. This allows for a dumb guest
> > OS, but can make errors really hard to debug from the guest side.
> >   
> 
> IMHO the same is true for virtio for example (the abstraction
> hides the backend and the backing: if there is a problem there it's
> hard to debug from the guest side).

In a way, yes. But it is way more on the virtual side of things :)

> 
> Because of my lack of understanding, this option appeared simpler to
> me: clear ownership, and probably also less places where things can
> go wrong.

My gut feeling is that exposing channel paths is the easier way in the
long run.

> 
> > Exposing real channel paths to the guest means that the guest OS needs
> > to be able to deal with path-related things, but OTOH it has more
> > control. As I don't think we'll ever want to support a guest OS that
> > does not also run under LPAR, I'd prefer that way.  
> 
> Nod. And this makes a full circle, namely the question of benefit of
> having more control. But since we did one full circle I'm much smarter
> now than at the beginning.
> 
> Thank you very much for all the background information and for your
> patience.

Well, I hope I'm not confusing everyone too much :)

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Dong Jia Shi]

* Cornelia Huck <cohuck@redhat.com> [2017-07-28 14:58:19 +0200]:

[...]

> > 
> > If I understand you correctly it ain't possible to handle these
> > in the host (and let the guest a simple 'non-real' virtual
> > channel path whose reliability depends on what the host does),
> > or?
> 
> It is possible. Mapping to a virtual channel path or not is basically a
> design decision (IIRC, z/VM supports both).
> 
> Mapping everything to a virtual chpid basically concentrates all
> path-related handling in the hypervisor. This allows for a dumb guest
> OS, but can make errors really hard to debug from the guest side.
I understood this.

> 
> Exposing real channel paths to the guest means that the guest OS needs
> to be able to deal with path-related things, but OTOH it has more
> control. As I don't think we'll ever want to support a guest OS that
> does not also run under LPAR, I'd prefer that way.
> 
My poor English... Sorry, I don't undersatnd the last sentence...

[...]

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Cornelia Huck]

On Mon, 31 Jul 2017 09:46:17 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Cornelia Huck <cohuck@redhat.com> [2017-07-28 14:58:19 +0200]:

> > Exposing real channel paths to the guest means that the guest OS needs
> > to be able to deal with path-related things, but OTOH it has more
> > control. As I don't think we'll ever want to support a guest OS that
> > does not also run under LPAR, I'd prefer that way.
> >   
> My poor English... Sorry, I don't undersatnd the last sentence...

Probably too many negations... let me rephrase.

Looking at the guest OSes we want to support, it's currently Linux,
Linux, or Linux. And Linux runs fine under LPAR, as it is able to deal
with the details of channel path handling (and LPAR does not virtualize
anything of this).

Of the other OSes, z/OS is way too insanely complex, z/VM does not make
much sense, and I don't see a case for z/TPF either. Which leaves
z/VSE, which should be able to deal with the path related stuff as well.

So, I don't think we close any doors if we expose the path handling
complexities to the guest.

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Dong Jia Shi]

* Cornelia Huck <cohuck@redhat.com> [2017-07-31 10:41:47 +0200]:

> On Mon, 31 Jul 2017 09:46:17 +0800
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
> > * Cornelia Huck <cohuck@redhat.com> [2017-07-28 14:58:19 +0200]:
> 
> > > Exposing real channel paths to the guest means that the guest OS needs
> > > to be able to deal with path-related things, but OTOH it has more
> > > control. As I don't think we'll ever want to support a guest OS that
> > > does not also run under LPAR, I'd prefer that way.
> > >   
> > My poor English... Sorry, I don't undersatnd the last sentence...
> 
> Probably too many negations... let me rephrase.
Negations kill my brain. ;)

> 
> Looking at the guest OSes we want to support, it's currently Linux,
> Linux, or Linux. And Linux runs fine under LPAR, as it is able to deal
> with the details of channel path handling (and LPAR does not virtualize
> anything of this).
Nod.

> 
> Of the other OSes, z/OS is way too insanely complex, z/VM does not make
> much sense, and I don't see a case for z/TPF either. Which leaves
> z/VSE, which should be able to deal with the path related stuff as well.
Nod.

> 
> So, I don't think we close any doors if we expose the path handling
> complexities to the guest.
> 
Understand now! Thanks a lot!

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Dong Jia Shi]

* Cornelia Huck <cohuck@redhat.com> [2017-07-27 13:59:10 +0200]:

> On Thu, 27 Jul 2017 03:54:18 +0200
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
> > When a channel path is hot plugged into a CSS, we should generate
> > a channel path initialized CRW (channel report word). The current
> > code does not do that, instead it puts a stub function with a TODO
> > reminder there.
> > 
> > This implements the css_generate_chp_crws() function by:
> > 1. refactor the existing code.
> > 2. add an @add parameter to provide future callers with the
> >    capability of generating channel path permanent error with
> >    facility not initialized CRW.
> > 3. add a @hotplugged parameter, so to opt out generating initialized
> >    CRWs for predefined channel paths.
> 
> I'm not 100% sure whether the logic is correct here. Let me elaborate:
> 
> The current code flow when hotplugging a device is:
> - Generate the schib.
> - Check if any of the chpids refers to a not yet existing channel path;
>   generate it if that is the case.
> - Post a crw for the subchannel.
> 
> The second step is where the current code seems to be not quite correct
> already. It is fine for coldplugged devices, but I really think we need
> to make sure that all referenced channel paths are in place before we
> hotplug a new device. It was not really relevant when we just had one
> very virtual channel path, and 3270 is experimental so it is not a
> problem in practice.
vfio-ccw hotplug could also live with the current mechanism - just
generate the chp according to its CHPIDs information. What the problem
in practice for it then? Channel path status change could be synchronize
by adding more MMIO regions and eventfd irq for vfio-ccw.

> 
> This, of course, implies we need deeper changes. We need to create the
> channel paths before the subchannel is created and refuse hotplug of a
> device if not all channel paths it needs are defined. This means we
> need some things before we can claim real channel path support:
> - Have a way to specify channel paths on the command line resp. when
>   hotplugging. This implies they need to be real objects.
> - Have a way to specify which channel paths belong to a subchannel in
>   the same context. Keep existing device types working with the current
>   method.
If we want to adopt the unified modelling for all kinds of devices, then
we require the user to define chps before define devices.

We could defaulty always have a virtio reserved chp 0 defined on each
css, so we do not need to touch the current virtio devices command line.
Defining more chps or changing chpid for virtio devices does not provide
added values.

For emulated device, we can define chpids for use. E.g.:
-device chp,cssid=fe,chpid=11 \
-device chp,cssid=fe,chpid=22 \
-chardev socket,id=terminal0,host=0.0.0.0,port=23,nowait,server,tn3270 \
-device x-terminal3270,chardev=terminal0,id=terminal3270_0,devno=fe.0.000a,chpids=1122000000000000

Or, I think, we could let Qemu automatically find a free chp for them.
Sine, the same as the virtio devices, defining more chps or changing
chpid for emulated devices does provide added values either. In this
case, we do not need to touch the emualted device command line too.

When defining a vfio-ccw device, since the real subchannel implicitly
indicates the chps it bound to, we grasp the CHPIDs from sysfs (or, with
my current work, we could even retrieve these information from a new
added MMIO region). In this case, defining some channel path devices
separately does not make sense to me.

After thinking quite a while, if we do want to add a real device object
for a channel path, the most intractable problem (but not the only one)
for me is to find a good way to map the real path with the virtual one.
How would we retrieve the information from the real one? We'd need the
host kernel to provide totally new interfaces for channel path
information synchronization and notification machenism. I don't think in
this case sysfs is the choice. Ioctls, vfio MMIO regions and eventfd
could be a better choice. I think, this is like we are trying to
passthru a channel path. So we'd need to have a new vfio device physical
driver (e.g. vfio-chp) to handle this...

And, if we finnaly find a way to solve the above problem, we may have
some commandline as the follows, and there is still other problems. E.g.:

lscss:
MDEV                                  Subchan.  PIM PAM POM  CHPIDs
------------------------------------------------------------------------------
6dfd3ec5-e8b3-4e18-a6fe-57bc9eceb920  0.0.013f  f0  f0  ff   42434445 00000000

lschp:
CHPID  Vary  Cfg.  Type  Cmg  Shared  PCHID
============================================
0.42   1     1     1b    2    1       0158 
0.43   1     1     1b    2    1       0159 
0.44   1     1     1b    2    1       01a0 
0.45   1     1     1b    2    1       01a1

Suppose we want to pass through the above mdev ($MDEV_CCW013f), we could
have the following command line:
-device vfio-chp,sysfsdev=$MDEV_CHP42,cssid=0,chpid=42 \
-device vfio-chp,sysfsdev=$MDEV_CHP43,cssid=0,chpid=43 \
-device vfio-chp,sysfsdev=$MDEV_CHP44,cssid=0,chpid=44 \
-device vfio-chp,sysfsdev=$MDEV_CHP45,cssid=0,chpid=45 \
-device vfio-ccw,sysfsdev=$MDEV_CCW013f,devno=0.0.1234,chpids=4243444500000000

The above vfio-ccw devices can not use any other CHP besides the above
4. Defining only some of the 4 CHPs for the vfio-ccw device does not
sounds reasonable. So isn't it redundant to explicitly define all of the
4 chps in command line for the vfio-ccw device? Since, itself already
provides the CHPIDs information...

> - Give channel paths states: Defined, configured. The right time for a
>   CRW is the transition between those states.
Sounds reasonable.

> - Only queue a 'device come' CRW for a subchannel if at least one of
>   its channel paths is in the configured state. Detach or make not
>   operational a subchannel if all of its paths are deconfigured.
Sounds reasonable too.

> 
> Something along those lines also matches better what I've seen on z/VM
> or LPAR. I realize that it's not easy :(
Yes... I don't find out a way that can satisfy all of the above
requirements for now...

> 
> tl;dr: I don't think we want chp crws until after we have a good chp
> model.
I have to agree.

> 
> > 
> > Signed-off-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> > ---
> >  hw/s390x/3270-ccw.c       |  3 ++-
> >  hw/s390x/css.c            | 55 ++++++++++++++++++++++++++++++++++++-----------
> >  hw/s390x/s390-ccw.c       |  2 +-
> >  hw/s390x/virtio-ccw.c     |  3 ++-
> >  include/hw/s390x/css.h    |  8 ++++---
> >  include/hw/s390x/ioinst.h |  1 +
> >  6 files changed, 53 insertions(+), 19 deletions(-)
> 

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Cornelia Huck]

On Mon, 31 Jul 2017 11:51:37 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Cornelia Huck <cohuck@redhat.com> [2017-07-27 13:59:10 +0200]:
> 
> > On Thu, 27 Jul 2017 03:54:18 +0200
> > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> >   
> > > When a channel path is hot plugged into a CSS, we should generate
> > > a channel path initialized CRW (channel report word). The current
> > > code does not do that, instead it puts a stub function with a TODO
> > > reminder there.
> > > 
> > > This implements the css_generate_chp_crws() function by:
> > > 1. refactor the existing code.
> > > 2. add an @add parameter to provide future callers with the
> > >    capability of generating channel path permanent error with
> > >    facility not initialized CRW.
> > > 3. add a @hotplugged parameter, so to opt out generating initialized
> > >    CRWs for predefined channel paths.  
> > 
> > I'm not 100% sure whether the logic is correct here. Let me elaborate:
> > 
> > The current code flow when hotplugging a device is:
> > - Generate the schib.
> > - Check if any of the chpids refers to a not yet existing channel path;
> >   generate it if that is the case.
> > - Post a crw for the subchannel.
> > 
> > The second step is where the current code seems to be not quite correct
> > already. It is fine for coldplugged devices, but I really think we need
> > to make sure that all referenced channel paths are in place before we
> > hotplug a new device. It was not really relevant when we just had one
> > very virtual channel path, and 3270 is experimental so it is not a
> > problem in practice.  
> vfio-ccw hotplug could also live with the current mechanism - just
> generate the chp according to its CHPIDs information. What the problem
> in practice for it then? Channel path status change could be synchronize
> by adding more MMIO regions and eventfd irq for vfio-ccw.

I'm not sure that there is a problem in practice (I suppose there
isn't), but it feels weird. The user plugs a device, magically the
paths are created.

> 
> > 
> > This, of course, implies we need deeper changes. We need to create the
> > channel paths before the subchannel is created and refuse hotplug of a
> > device if not all channel paths it needs are defined. This means we
> > need some things before we can claim real channel path support:
> > - Have a way to specify channel paths on the command line resp. when
> >   hotplugging. This implies they need to be real objects.
> > - Have a way to specify which channel paths belong to a subchannel in
> >   the same context. Keep existing device types working with the current
> >   method.  
> If we want to adopt the unified modelling for all kinds of devices, then
> we require the user to define chps before define devices.

Yes.

> 
> We could defaulty always have a virtio reserved chp 0 defined on each
> css, so we do not need to touch the current virtio devices command line.

I think that's even mandatory, or we break backwards compatibility.

> Defining more chps or changing chpid for virtio devices does not provide
> added values.

Agreed.

> 
> For emulated device, we can define chpids for use. E.g.:
> -device chp,cssid=fe,chpid=11 \
> -device chp,cssid=fe,chpid=22 \
> -chardev socket,id=terminal0,host=0.0.0.0,port=23,nowait,server,tn3270 \
> -device x-terminal3270,chardev=terminal0,id=terminal3270_0,devno=fe.0.000a,chpids=1122000000000000

If we go that route (which I'm not too sure of), we should rather
reference the chp objects by id instead of providing a to-be-parsed
parameter.

> 
> Or, I think, we could let Qemu automatically find a free chp for them.
> Sine, the same as the virtio devices, defining more chps or changing
> chpid for emulated devices does provide added values either. In this
> case, we do not need to touch the emualted device command line too.

We should keep this working for compat (even if it's an x- device).

> 
> When defining a vfio-ccw device, since the real subchannel implicitly
> indicates the chps it bound to, we grasp the CHPIDs from sysfs (or, with
> my current work, we could even retrieve these information from a new
> added MMIO region). In this case, defining some channel path devices
> separately does not make sense to me.

We might want to pass only a subset of the channel paths to guest. This
can only work if we can define individual chp objects.

> 
> After thinking quite a while, if we do want to add a real device object
> for a channel path, the most intractable problem (but not the only one)
> for me is to find a good way to map the real path with the virtual one.
> How would we retrieve the information from the real one? We'd need the
> host kernel to provide totally new interfaces for channel path
> information synchronization and notification machenism. I don't think in
> this case sysfs is the choice. Ioctls, vfio MMIO regions and eventfd
> could be a better choice. I think, this is like we are trying to
> passthru a channel path. So we'd need to have a new vfio device physical
> driver (e.g. vfio-chp) to handle this...

And that would run into the problem of (1) the chp objects not using a
bus on Linux, and (2) implying dedicating the chpids, which we likely
do not want.

> 
> And, if we finnaly find a way to solve the above problem, we may have
> some commandline as the follows, and there is still other problems. E.g.:
> 
> lscss:
> MDEV                                  Subchan.  PIM PAM POM  CHPIDs
> ------------------------------------------------------------------------------
> 6dfd3ec5-e8b3-4e18-a6fe-57bc9eceb920  0.0.013f  f0  f0  ff   42434445 00000000
> 
> lschp:
> CHPID  Vary  Cfg.  Type  Cmg  Shared  PCHID
> ============================================
> 0.42   1     1     1b    2    1       0158 
> 0.43   1     1     1b    2    1       0159 
> 0.44   1     1     1b    2    1       01a0 
> 0.45   1     1     1b    2    1       01a1
> 
> Suppose we want to pass through the above mdev ($MDEV_CCW013f), we could
> have the following command line:
> -device vfio-chp,sysfsdev=$MDEV_CHP42,cssid=0,chpid=42 \
> -device vfio-chp,sysfsdev=$MDEV_CHP43,cssid=0,chpid=43 \
> -device vfio-chp,sysfsdev=$MDEV_CHP44,cssid=0,chpid=44 \
> -device vfio-chp,sysfsdev=$MDEV_CHP45,cssid=0,chpid=45 \
> -device vfio-ccw,sysfsdev=$MDEV_CCW013f,devno=0.0.1234,chpids=4243444500000000
> 
> The above vfio-ccw devices can not use any other CHP besides the above
> 4. Defining only some of the 4 CHPs for the vfio-ccw device does not
> sounds reasonable. So isn't it redundant to explicitly define all of the
> 4 chps in command line for the vfio-ccw device? Since, itself already
> provides the CHPIDs information...

See my comments above.

> 
> > - Give channel paths states: Defined, configured. The right time for a
> >   CRW is the transition between those states.  
> Sounds reasonable.
> 
> > - Only queue a 'device come' CRW for a subchannel if at least one of
> >   its channel paths is in the configured state. Detach or make not
> >   operational a subchannel if all of its paths are deconfigured.  
> Sounds reasonable too.
> 
> > 
> > Something along those lines also matches better what I've seen on z/VM
> > or LPAR. I realize that it's not easy :(  
> Yes... I don't find out a way that can satisfy all of the above
> requirements for now...

Even if you can, it sounds like a lot of work :(

> 
> > 
> > tl;dr: I don't think we want chp crws until after we have a good chp
> > model.  
> I have to agree.

I'm wondering whether we should just defer this to later. We can live
with no chp crw being created (except for rchp), as due to the crw
generation being unreliable the guest OS has to handle path changes
without crws anyway.

We just need to make sure that pno and friends are appropriately
reported to the guest.

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Halil Pasic]

On 07/31/2017 01:13 PM, Cornelia Huck wrote:
> On Mon, 31 Jul 2017 11:51:37 +0800
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
>> * Cornelia Huck <cohuck@redhat.com> [2017-07-27 13:59:10 +0200]:
>>
>>> On Thu, 27 Jul 2017 03:54:18 +0200
>>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
[..]
>>
>> After thinking quite a while, if we do want to add a real device object
>> for a channel path, the most intractable problem (but not the only one)
>> for me is to find a good way to map the real path with the virtual one.

Yeah, channel path virtualisation... IMHO our current solution is not
not really solving the problem. Say, do we  care about a design which
could work with live migration (@Dong Jia)?

>> How would we retrieve the information from the real one? We'd need the
>> host kernel to provide totally new interfaces for channel path
>> information synchronization and notification machenism. I don't think in
>> this case sysfs is the choice. Ioctls, vfio MMIO regions and eventfd
>> could be a better choice. I think, this is like we are trying to
>> passthru a channel path. So we'd need to have a new vfio device physical
>> driver (e.g. vfio-chp) to handle this...
> 
> And that would run into the problem of (1) the chp objects not using a
> bus on Linux, and (2) implying dedicating the chpids, which we likely
> do not want.
> 

AFAIU this is about "real-virtual" vs "virutal-virtual". I would really like
to see some design here (@Dong Jia). I'm not sure any more where do we want to go.

[..]
>>
>>>
>>> tl;dr: I don't think we want chp crws until after we have a good chp
>>> model.  
>> I have to agree.
> 
> I'm wondering whether we should just defer this to later. We can live
> with no chp crw being created (except for rchp), as due to the crw
> generation being unreliable the guest OS has to handle path changes
> without crws anyway.
> 
> We just need to make sure that pno and friends are appropriately
> reported to the guest.
> 

+1 

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Dong Jia Shi]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-07-31 14:30:32 +0200]:

> 
> 
> On 07/31/2017 01:13 PM, Cornelia Huck wrote:
> > On Mon, 31 Jul 2017 11:51:37 +0800
> > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> > 
> >> * Cornelia Huck <cohuck@redhat.com> [2017-07-27 13:59:10 +0200]:
> >>
> >>> On Thu, 27 Jul 2017 03:54:18 +0200
> >>> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> [..]
> >>
> >> After thinking quite a while, if we do want to add a real device object
> >> for a channel path, the most intractable problem (but not the only one)
> >> for me is to find a good way to map the real path with the virtual one.
> 
> Yeah, channel path virtualisation... IMHO our current solution is not
> not really solving the problem.
If we choose to go with Conny's idea, then yes, my prototype is totally
in the wrong direction.

> Say, do we  care about a design which could work with live migration
> (@Dong Jia)?
Channel I/O pass through and live migration don't go together.

I'm afraid, we did not considerate live migration yet. If we can
migrate, that would be great! But we are still struggling in finding a
way out the current swamp...

> 
> >> How would we retrieve the information from the real one? We'd need the
> >> host kernel to provide totally new interfaces for channel path
> >> information synchronization and notification machenism. I don't think in
> >> this case sysfs is the choice. Ioctls, vfio MMIO regions and eventfd
> >> could be a better choice. I think, this is like we are trying to
> >> passthru a channel path. So we'd need to have a new vfio device physical
> >> driver (e.g. vfio-chp) to handle this...
> > 
> > And that would run into the problem of (1) the chp objects not using a
> > bus on Linux, and (2) implying dedicating the chpids, which we likely
> > do not want.
> > 
> 
> AFAIU this is about "real-virtual" vs "virutal-virtual". I would really like
> to see some design here (@Dong Jia). I'm not sure any more where do we want to go.
If we can find a way to satisfy the requirements that Conny mentioned, I
can prepare a design. Sadly, we are not there yet.

> 
> [..]
> >>
> >>>
> >>> tl;dr: I don't think we want chp crws until after we have a good chp
> >>> model.  
> >> I have to agree.
> > 
> > I'm wondering whether we should just defer this to later. We can live
> > with no chp crw being created (except for rchp), as due to the crw
> > generation being unreliable the guest OS has to handle path changes
> > without crws anyway.
> > 
> > We just need to make sure that pno and friends are appropriately
> > reported to the guest.
> > 
> 
> +1 
Ha! This is very very clever!

I will give it a try. If everything go well, I will agree with this
happyly. ;)

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Dong Jia Shi]

* Cornelia Huck <cohuck@redhat.com> [2017-07-31 13:13:02 +0200]:

> On Mon, 31 Jul 2017 11:51:37 +0800
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
> > * Cornelia Huck <cohuck@redhat.com> [2017-07-27 13:59:10 +0200]:
> > 
> > > On Thu, 27 Jul 2017 03:54:18 +0200
> > > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> > >   
> > > > When a channel path is hot plugged into a CSS, we should generate
> > > > a channel path initialized CRW (channel report word). The current
> > > > code does not do that, instead it puts a stub function with a TODO
> > > > reminder there.
> > > > 
> > > > This implements the css_generate_chp_crws() function by:
> > > > 1. refactor the existing code.
> > > > 2. add an @add parameter to provide future callers with the
> > > >    capability of generating channel path permanent error with
> > > >    facility not initialized CRW.
> > > > 3. add a @hotplugged parameter, so to opt out generating initialized
> > > >    CRWs for predefined channel paths.  
> > > 
> > > I'm not 100% sure whether the logic is correct here. Let me elaborate:
> > > 
> > > The current code flow when hotplugging a device is:
> > > - Generate the schib.
> > > - Check if any of the chpids refers to a not yet existing channel path;
> > >   generate it if that is the case.
> > > - Post a crw for the subchannel.
> > > 
> > > The second step is where the current code seems to be not quite correct
> > > already. It is fine for coldplugged devices, but I really think we need
> > > to make sure that all referenced channel paths are in place before we
> > > hotplug a new device. It was not really relevant when we just had one
> > > very virtual channel path, and 3270 is experimental so it is not a
> > > problem in practice.  
> > vfio-ccw hotplug could also live with the current mechanism - just
> > generate the chp according to its CHPIDs information. What the problem
> > in practice for it then? Channel path status change could be synchronize
> > by adding more MMIO regions and eventfd irq for vfio-ccw.
> 
> I'm not sure that there is a problem in practice (I suppose there
> isn't), but it feels weird. The user plugs a device, magically the
> paths are created.
Understand.

> 
> > 
> > > 
> > > This, of course, implies we need deeper changes. We need to create the
> > > channel paths before the subchannel is created and refuse hotplug of a
> > > device if not all channel paths it needs are defined. This means we
> > > need some things before we can claim real channel path support:
> > > - Have a way to specify channel paths on the command line resp. when
> > >   hotplugging. This implies they need to be real objects.
> > > - Have a way to specify which channel paths belong to a subchannel in
> > >   the same context. Keep existing device types working with the current
> > >   method.  
> > If we want to adopt the unified modelling for all kinds of devices, then
> > we require the user to define chps before define devices.
> 
> Yes.
> 
> > 
> > We could defaulty always have a virtio reserved chp 0 defined on each
> > css, so we do not need to touch the current virtio devices command line.
> 
> I think that's even mandatory, or we break backwards compatibility.
Nod.

> 
> > Defining more chps or changing chpid for virtio devices does not provide
> > added values.
> 
> Agreed.
> 
> > 
> > For emulated device, we can define chpids for use. E.g.:
> > -device chp,cssid=fe,chpid=11 \
> > -device chp,cssid=fe,chpid=22 \
> > -chardev socket,id=terminal0,host=0.0.0.0,port=23,nowait,server,tn3270 \
> > -device x-terminal3270,chardev=terminal0,id=terminal3270_0,devno=fe.0.000a,chpids=1122000000000000
> 
> If we go that route (which I'm not too sure of), we should rather
> reference the chp objects by id instead of providing a to-be-parsed
> parameter.
Got this and Nod.

> 
> > 
> > Or, I think, we could let Qemu automatically find a free chp for them.
> > Sine, the same as the virtio devices, defining more chps or changing
> > chpid for emulated devices does provide added values either. In this
> > case, we do not need to touch the emualted device command line too.
> 
> We should keep this working for compat (even if it's an x- device).
Yes. Finding a free chp when needed, is what the emulated device
currently does. So this will be compatable with the current stuff.

> 
> > 
> > When defining a vfio-ccw device, since the real subchannel implicitly
> > indicates the chps it bound to, we grasp the CHPIDs from sysfs (or, with
> > my current work, we could even retrieve these information from a new
> > added MMIO region). In this case, defining some channel path devices
> > separately does not make sense to me.
> 
> We might want to pass only a subset of the channel paths to guest. This
> can only work if we can define individual chp objects.
Why would we want this?

We can add, for example, a "chpids" parameter for the "vfio-ccw" device
to limit its chpids to a subset that we want it to have? E.g.:

For this mdev:
MDEV                                  Subchan.  PIM PAM POM  CHPIDs
------------------------------------------------------------------------------
6dfd3ec5-e8b3-4e18-a6fe-57bc9eceb920  0.0.013f  f0  f0  ff   42434445 00000000

We could use this command line:
-device vfio-ccw,sysfsdev=$MDEV_CCW013f,devno=0.0.1234,chpids=4245000000000000
                                                              ^^^^

> 
> > 
> > After thinking quite a while, if we do want to add a real device object
> > for a channel path, the most intractable problem (but not the only one)
> > for me is to find a good way to map the real path with the virtual one.
> > How would we retrieve the information from the real one? We'd need the
> > host kernel to provide totally new interfaces for channel path
> > information synchronization and notification machenism. I don't think in
> > this case sysfs is the choice. Ioctls, vfio MMIO regions and eventfd
> > could be a better choice. I think, this is like we are trying to
> > passthru a channel path. So we'd need to have a new vfio device physical
> > driver (e.g. vfio-chp) to handle this...
> 
> And that would run into the problem of (1) the chp objects not using a
> bus on Linux, and (2) implying dedicating the chpids, which we likely
> do not want.
Yes. Tough problem.

> 
> > 
> > And, if we finnaly find a way to solve the above problem, we may have
> > some commandline as the follows, and there is still other problems. E.g.:
> > 
> > lscss:
> > MDEV                                  Subchan.  PIM PAM POM  CHPIDs
> > ------------------------------------------------------------------------------
> > 6dfd3ec5-e8b3-4e18-a6fe-57bc9eceb920  0.0.013f  f0  f0  ff   42434445 00000000
> > 
> > lschp:
> > CHPID  Vary  Cfg.  Type  Cmg  Shared  PCHID
> > ============================================
> > 0.42   1     1     1b    2    1       0158 
> > 0.43   1     1     1b    2    1       0159 
> > 0.44   1     1     1b    2    1       01a0 
> > 0.45   1     1     1b    2    1       01a1
> > 
> > Suppose we want to pass through the above mdev ($MDEV_CCW013f), we could
> > have the following command line:
> > -device vfio-chp,sysfsdev=$MDEV_CHP42,cssid=0,chpid=42 \
> > -device vfio-chp,sysfsdev=$MDEV_CHP43,cssid=0,chpid=43 \
> > -device vfio-chp,sysfsdev=$MDEV_CHP44,cssid=0,chpid=44 \
> > -device vfio-chp,sysfsdev=$MDEV_CHP45,cssid=0,chpid=45 \
> > -device vfio-ccw,sysfsdev=$MDEV_CCW013f,devno=0.0.1234,chpids=4243444500000000
> > 
> > The above vfio-ccw devices can not use any other CHP besides the above
> > 4. Defining only some of the 4 CHPs for the vfio-ccw device does not
> > sounds reasonable. So isn't it redundant to explicitly define all of the
> > 4 chps in command line for the vfio-ccw device? Since, itself already
> > provides the CHPIDs information...
> 
> See my comments above.
Done.

> 
> > 
> > > - Give channel paths states: Defined, configured. The right time for a
> > >   CRW is the transition between those states.  
> > Sounds reasonable.
> > 
> > > - Only queue a 'device come' CRW for a subchannel if at least one of
> > >   its channel paths is in the configured state. Detach or make not
> > >   operational a subchannel if all of its paths are deconfigured.  
> > Sounds reasonable too.
> > 
> > > 
> > > Something along those lines also matches better what I've seen on z/VM
> > > or LPAR. I realize that it's not easy :(  
> > Yes... I don't find out a way that can satisfy all of the above
> > requirements for now...
> 
> Even if you can, it sounds like a lot of work :(
Sounds like that will take a year to accomplish...

> 
> > 
> > > 
> > > tl;dr: I don't think we want chp crws until after we have a good chp
> > > model.  
> > I have to agree.
> 
> I'm wondering whether we should just defer this to later. We can live
> with no chp crw being created (except for rchp), as due to the crw
> generation being unreliable the guest OS has to handle path changes
> without crws anyway.
I know this. But how couldn't I get the idea to defer the crw
generation?! :D

> 
> We just need to make sure that pno and friends are appropriately
> reported to the guest.
> 
Will try!

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Cornelia Huck]

On Tue, 1 Aug 2017 10:29:10 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Cornelia Huck <cohuck@redhat.com> [2017-07-31 13:13:02 +0200]:
> 
> > On Mon, 31 Jul 2017 11:51:37 +0800
> > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> > > When defining a vfio-ccw device, since the real subchannel implicitly
> > > indicates the chps it bound to, we grasp the CHPIDs from sysfs (or, with
> > > my current work, we could even retrieve these information from a new
> > > added MMIO region). In this case, defining some channel path devices
> > > separately does not make sense to me.  
> > 
> > We might want to pass only a subset of the channel paths to guest. This
> > can only work if we can define individual chp objects.  
> Why would we want this?

For example, if you know that a reconfiguration is coming on soon, you
can just exclude the paths that will go away anyway and the guest will
never know about them. Or for preferred pathing, although that one
fortunately seems to have died out.

Not very strong reasons to spend time on this, though.

> 
> We can add, for example, a "chpids" parameter for the "vfio-ccw" device
> to limit its chpids to a subset that we want it to have? E.g.:
> 
> For this mdev:
> MDEV                                  Subchan.  PIM PAM POM  CHPIDs
> ------------------------------------------------------------------------------
> 6dfd3ec5-e8b3-4e18-a6fe-57bc9eceb920  0.0.013f  f0  f0  ff   42434445 00000000
> 
> We could use this command line:
> -device vfio-ccw,sysfsdev=$MDEV_CCW013f,devno=0.0.1234,chpids=4245000000000000
>                                                               ^^^^

Yes, that would work, should we want that. We can probably do without for now.

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Dong Jia Shi]

* Cornelia Huck <cohuck@redhat.com> [2017-08-01 09:24:20 +0200]:

> On Tue, 1 Aug 2017 10:29:10 +0800
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
> > * Cornelia Huck <cohuck@redhat.com> [2017-07-31 13:13:02 +0200]:
> > 
> > > On Mon, 31 Jul 2017 11:51:37 +0800
> > > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
> > > > When defining a vfio-ccw device, since the real subchannel implicitly
> > > > indicates the chps it bound to, we grasp the CHPIDs from sysfs (or, with
> > > > my current work, we could even retrieve these information from a new
> > > > added MMIO region). In this case, defining some channel path devices
> > > > separately does not make sense to me.  
> > > 
> > > We might want to pass only a subset of the channel paths to guest. This
> > > can only work if we can define individual chp objects.  
> > Why would we want this?
> 
> For example, if you know that a reconfiguration is coming on soon, you
> can just exclude the paths that will go away anyway and the guest will
> never know about them. Or for preferred pathing, although that one
> fortunately seems to have died out.
> 
> Not very strong reasons to spend time on this, though.
Got it.

> 
> > 
> > We can add, for example, a "chpids" parameter for the "vfio-ccw" device
> > to limit its chpids to a subset that we want it to have? E.g.:
> > 
> > For this mdev:
> > MDEV                                  Subchan.  PIM PAM POM  CHPIDs
> > ------------------------------------------------------------------------------
> > 6dfd3ec5-e8b3-4e18-a6fe-57bc9eceb920  0.0.013f  f0  f0  ff   42434445 00000000
> > 
> > We could use this command line:
> > -device vfio-ccw,sysfsdev=$MDEV_CCW013f,devno=0.0.1234,chpids=4245000000000000
> >                                                               ^^^^
> 
> Yes, that would work, should we want that. We can probably do without for now.
> 
Let's deffer this too!

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 0/3] Channel Path realted CRW generation

[Cornelia Huck]

On Thu, 27 Jul 2017 03:54:15 +0200
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> This series is trying to:
> 1. clear up CRW related code.
> 2. generate the right channel path related CRW at the right time.
> 
> I did this mainly because it's a requirement from my current work, that is I'm
> in preparation of a group of patch for channel path virtualization. I can use
> the inerface that provided by this series later, so as to, for vfio-ccw
> devices, notify the guest with channel path status vary that happens on the
> host side.

Sounds cool.

> 
> During an internal discussion, Halil and Pierre pointed out that for path
> hotplug, generating a CRW seems logical, but how is it covered by the AR is not
> clear - we have problem in understanding some grammar ambiguous paragraphs.
> While certain parts of the AR is not available outside, but I'm still wondering
> if the author ;) could give us some clue... BTW, we know that, in Linux kernel
> we had code that handles un-solicited chp crw, so we tend to believe it's right
> to generate channel path initialized CRW for path hotplug. It's just we can not
> find the reason from the document.

I always found path notifications to be a bit odd. They depend on
various things:
- whether you're running under LPAR or under z/VM
- whether it's a hardware condition (path failure) or something
  triggered by the admin (path vary on/off)
- if it's admin triggered, where it was done (on the SE, by one of
  several mechanisms in CP, via SCLP)

You're bound to get different kinds of notifications: via a CRW with
source channel path, via event information retrievable via CHSC
(indicated by a CRW with source CSS), via a PNO indication, or nothing
at all.

[Reminds me of a case where we got path gone CRWs under LPAR when a
path was deactivated at the SE (which we would notice via PNO anyway),
but no CRW when the path was reactivated - not very useful. When trying
to report this as an issue, we got the answer that we of course need to
use the OS interface to vary off the path beforehand. Silly penguins.]

My recommendation would be to generate a fitting CRW if the wording
allows to do so. I would hope that getting as many useful indications
as possible is most helpful to the OS. (I had added the path-come CRW
handling in Linux back then and afterwards wondered why we did not get
it - I must have interpreted the PoP in the same way as you did.)

I'll double check with how I'd interpret the PoP today.

> 
> Pierre also suggested to add an @erc param for css_generate_chp_crws() in patch3,
> while others have a different opinion. This is for your consideration.
> 
> Best regards!
> 
> Dong Jia Shi (3):
>   s390x/css: use macro for event-information pending error recover code
>   s390x/css: generate solicited crw for rchp completion signaling
>   s390x/css: generate channel path initialized CRW for channel path
>     hotplug
> 
>  hw/s390x/3270-ccw.c       |  3 ++-
>  hw/s390x/css.c            | 66 +++++++++++++++++++++++++++++++++++------------
>  hw/s390x/s390-ccw.c       |  2 +-
>  hw/s390x/virtio-ccw.c     |  3 ++-
>  include/hw/s390x/css.h    | 10 ++++---
>  include/hw/s390x/ioinst.h |  6 +++--
>  6 files changed, 64 insertions(+), 26 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/3] Channel Path realted CRW generation

[Dong Jia Shi]

* Cornelia Huck <cohuck@redhat.com> [2017-07-27 11:46:03 +0200]:

> On Thu, 27 Jul 2017 03:54:15 +0200
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
> > This series is trying to:
> > 1. clear up CRW related code.
> > 2. generate the right channel path related CRW at the right time.
> > 
> > I did this mainly because it's a requirement from my current work, that is I'm
> > in preparation of a group of patch for channel path virtualization. I can use
> > the inerface that provided by this series later, so as to, for vfio-ccw
> > devices, notify the guest with channel path status vary that happens on the
> > host side.
> 
> Sounds cool.
> 
> > 
> > During an internal discussion, Halil and Pierre pointed out that for path
> > hotplug, generating a CRW seems logical, but how is it covered by the AR is not
> > clear - we have problem in understanding some grammar ambiguous paragraphs.
> > While certain parts of the AR is not available outside, but I'm still wondering
> > if the author ;) could give us some clue... BTW, we know that, in Linux kernel
> > we had code that handles un-solicited chp crw, so we tend to believe it's right
> > to generate channel path initialized CRW for path hotplug. It's just we can not
> > find the reason from the document.
> 
> I always found path notifications to be a bit odd. They depend on
> various things:
> - whether you're running under LPAR or under z/VM
> - whether it's a hardware condition (path failure) or something
>   triggered by the admin (path vary on/off)
> - if it's admin triggered, where it was done (on the SE, by one of
>   several mechanisms in CP, via SCLP)
These are clear.

During the internnal discussion, we wished to get the resources to test
all of these cases to verify. For the z/VM and SE stuff, it seems a bit
difficult. So we decided to go with a shortcut -- to ask you.

> 
> You're bound to get different kinds of notifications: via a CRW with
> source channel path, via event information retrievable via CHSC
> (indicated by a CRW with source CSS),
Ha, I was not awre of this one before!

> via a PNO indication, or nothing at all.
> 
> [Reminds me of a case where we got path gone CRWs under LPAR when a
> path was deactivated at the SE (which we would notice via PNO anyway),
> but no CRW when the path was reactivated - not very useful. When trying
> to report this as an issue, we got the answer that we of course need to
> use the OS interface to vary off the path beforehand. Silly penguins.]
... ...

> 
> My recommendation would be to generate a fitting CRW if the wording
> allows to do so.
Nod. I'm trying this already.

> I would hope that getting as many useful indications as possible is
> most helpful to the OS.
Nod. Trying this too.
My prototype work tries to sync the belowing information from host
kernel to qemu:
1. the real SCHIB, so stsch from guest could get the updated path masks.
2. the Store Subchannel Description information, and with the new added
support for the SCLP read channel path information command, guest could
get the configure status of the path.
3. still working on support CHSC store channel path description command.

> (I had added the path-come CRW handling in Linux back then and
> afterwards wondered why we did not get it - I must have interpreted
> the PoP in the same way as you did.)
I've a bugfix patch in the kernel side, and it has been accepted by the
s390 maintainers. May be this could be a clue? Post it here:

When channel path is identified as the report source code (RSC)
of a CRW, and initialized (CRW_ERC_INIT) is recognized as the
error recovery code (ERC) by the channel subsystem, it indicates
a "path has come" event.

Let's handle this case in chp_process_crw().

diff --git a/drivers/s390/cio/chp.c b/drivers/s390/cio/chp.c
index 7e0d4f724dda..432fc40990bd 100644
--- a/drivers/s390/cio/chp.c
+++ b/drivers/s390/cio/chp.c
@@ -559,6 +559,7 @@ static void chp_process_crw(struct crw *crw0, struct
crw *crw1,
        chpid.id = crw0->rsid;
        switch (crw0->erc) {
        case CRW_ERC_IPARM: /* Path has come. */
+       case CRW_ERC_INIT:
                if (!chp_is_registered(chpid))
                        chp_new(chpid);
                chsc_chp_online(chpid);

Notice:
At the very beginning, I replaced CRW_ERC_IPARM with CRW_ERC_INIT. But
Sebstian Ott suggested:
"I don't know of a machine that actually implements a CRW
at all when a chpid is configured online on the SE/HMC.

Because of potential regressions I don't want to remove CRW_ERC_IPARM
here. I'm good with adding CRW_ERC_INIT though."

> 
> I'll double check with how I'd interpret the PoP today.
> 
Thanks.

[...]

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 0/3] Channel Path realted CRW generation

[Cornelia Huck]

On Fri, 28 Jul 2017 17:21:08 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Cornelia Huck <cohuck@redhat.com> [2017-07-27 11:46:03 +0200]:
> 
> > On Thu, 27 Jul 2017 03:54:15 +0200
> > Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> >   
> > > This series is trying to:
> > > 1. clear up CRW related code.
> > > 2. generate the right channel path related CRW at the right time.
> > > 
> > > I did this mainly because it's a requirement from my current work, that is I'm
> > > in preparation of a group of patch for channel path virtualization. I can use
> > > the inerface that provided by this series later, so as to, for vfio-ccw
> > > devices, notify the guest with channel path status vary that happens on the
> > > host side.  
> > 
> > Sounds cool.
> >   
> > > 
> > > During an internal discussion, Halil and Pierre pointed out that for path
> > > hotplug, generating a CRW seems logical, but how is it covered by the AR is not
> > > clear - we have problem in understanding some grammar ambiguous paragraphs.
> > > While certain parts of the AR is not available outside, but I'm still wondering
> > > if the author ;) could give us some clue... BTW, we know that, in Linux kernel
> > > we had code that handles un-solicited chp crw, so we tend to believe it's right
> > > to generate channel path initialized CRW for path hotplug. It's just we can not
> > > find the reason from the document.  
> > 
> > I always found path notifications to be a bit odd. They depend on
> > various things:
> > - whether you're running under LPAR or under z/VM
> > - whether it's a hardware condition (path failure) or something
> >   triggered by the admin (path vary on/off)
> > - if it's admin triggered, where it was done (on the SE, by one of
> >   several mechanisms in CP, via SCLP)  
> These are clear.
> 
> During the internnal discussion, we wished to get the resources to test
> all of these cases to verify. For the z/VM and SE stuff, it seems a bit
> difficult. So we decided to go with a shortcut -- to ask you.

Unfortunately, my memory is not perfect - and I've seen changes in
behaviour between different versions of the hardware etc. as well...

> 
> > 
> > You're bound to get different kinds of notifications: via a CRW with
> > source channel path, via event information retrievable via CHSC
> > (indicated by a CRW with source CSS),  
> Ha, I was not awre of this one before!

That's the 'link incident' and 'resource accessibility' stuff.

> 
> > via a PNO indication, or nothing at all.
> > 
> > [Reminds me of a case where we got path gone CRWs under LPAR when a
> > path was deactivated at the SE (which we would notice via PNO anyway),
> > but no CRW when the path was reactivated - not very useful. When trying
> > to report this as an issue, we got the answer that we of course need to
> > use the OS interface to vary off the path beforehand. Silly penguins.]  
> ... ...
> 
> > 
> > My recommendation would be to generate a fitting CRW if the wording
> > allows to do so.  
> Nod. I'm trying this already.
> 
> > I would hope that getting as many useful indications as possible is
> > most helpful to the OS.  
> Nod. Trying this too.
> My prototype work tries to sync the belowing information from host
> kernel to qemu:
> 1. the real SCHIB, so stsch from guest could get the updated path masks.

How far do you want to go with mirroring? I think you need to modify at
least the devno in the pmcw, no?

> 2. the Store Subchannel Description information, and with the new added
> support for the SCLP read channel path information command, guest could
> get the configure status of the path.

That's also a chsc, right?

> 3. still working on support CHSC store channel path description command.

I'm currently wondering how many of those chscs are optional. OTOH, if
a modern Linux guest cannot work properly without them, it makes no
sense to leave them out.

> 
> > (I had added the path-come CRW handling in Linux back then and
> > afterwards wondered why we did not get it - I must have interpreted
> > the PoP in the same way as you did.)  
> I've a bugfix patch in the kernel side, and it has been accepted by the
> s390 maintainers. May be this could be a clue? Post it here:
> 
> When channel path is identified as the report source code (RSC)
> of a CRW, and initialized (CRW_ERC_INIT) is recognized as the
> error recovery code (ERC) by the channel subsystem, it indicates
> a "path has come" event.
> 
> Let's handle this case in chp_process_crw().
> 
> diff --git a/drivers/s390/cio/chp.c b/drivers/s390/cio/chp.c
> index 7e0d4f724dda..432fc40990bd 100644
> --- a/drivers/s390/cio/chp.c
> +++ b/drivers/s390/cio/chp.c
> @@ -559,6 +559,7 @@ static void chp_process_crw(struct crw *crw0, struct
> crw *crw1,
>         chpid.id = crw0->rsid;
>         switch (crw0->erc) {
>         case CRW_ERC_IPARM: /* Path has come. */
> +       case CRW_ERC_INIT:
>                 if (!chp_is_registered(chpid))
>                         chp_new(chpid);
>                 chsc_chp_online(chpid);
> 
> Notice:
> At the very beginning, I replaced CRW_ERC_IPARM with CRW_ERC_INIT. But
> Sebstian Ott suggested:
> "I don't know of a machine that actually implements a CRW
> at all when a chpid is configured online on the SE/HMC.
> 
> Because of potential regressions I don't want to remove CRW_ERC_IPARM
> here. I'm good with adding CRW_ERC_INIT though."

Yeah, that makes sense, especially with the confusing state channel
path machine check handling is in from the architecture side.

> 
> > 
> > I'll double check with how I'd interpret the PoP today.
> >   
> Thanks.

I have read through the PoP and the outcome is a bit disappointing.
Much of it is a bit vague. I still think that you can err on the side
of overindication, though.

Re: [Qemu-devel] [PATCH 0/3] Channel Path realted CRW generation

[Dong Jia Shi]

* Cornelia Huck <cohuck@redhat.com> [2017-07-28 13:53:01 +0200]:

[...]

> > > > During an internal discussion, Halil and Pierre pointed out that for path
> > > > hotplug, generating a CRW seems logical, but how is it covered by the AR is not
> > > > clear - we have problem in understanding some grammar ambiguous paragraphs.
> > > > While certain parts of the AR is not available outside, but I'm still wondering
> > > > if the author ;) could give us some clue... BTW, we know that, in Linux kernel
> > > > we had code that handles un-solicited chp crw, so we tend to believe it's right
> > > > to generate channel path initialized CRW for path hotplug. It's just we can not
> > > > find the reason from the document.  
> > > 
> > > I always found path notifications to be a bit odd. They depend on
> > > various things:
> > > - whether you're running under LPAR or under z/VM
> > > - whether it's a hardware condition (path failure) or something
> > >   triggered by the admin (path vary on/off)
> > > - if it's admin triggered, where it was done (on the SE, by one of
> > >   several mechanisms in CP, via SCLP)  
> > These are clear.
> > 
> > During the internnal discussion, we wished to get the resources to test
> > all of these cases to verify. For the z/VM and SE stuff, it seems a bit
> > difficult. So we decided to go with a shortcut -- to ask you.
> 
> Unfortunately, my memory is not perfect
Still a very efficient shortcut for me. ;)

> - and I've seen changes in behaviour between different versions of the
> hardware etc. as well...
...

> 
> > 
> > > 
> > > You're bound to get different kinds of notifications: via a CRW with
> > > source channel path, via event information retrievable via CHSC
> > > (indicated by a CRW with source CSS),  
> > Ha, I was not awre of this one before!
> 
> That's the 'link incident' and 'resource accessibility' stuff.
My focus was trying to have the minimum stuff to make a Linux guest
working well -- basically, my working on prototype targeted to make the
output lschp and lscss corect and uptodate. I

I will dig this and see if I need to do more stuff.

> 
> > 
> > > via a PNO indication, or nothing at all.
> > > 
> > > [Reminds me of a case where we got path gone CRWs under LPAR when a
> > > path was deactivated at the SE (which we would notice via PNO anyway),
> > > but no CRW when the path was reactivated - not very useful. When trying
> > > to report this as an issue, we got the answer that we of course need to
> > > use the OS interface to vary off the path beforehand. Silly penguins.]  
> > ... ...
> > 
> > > 
> > > My recommendation would be to generate a fitting CRW if the wording
> > > allows to do so.  
> > Nod. I'm trying this already.
> > 
> > > I would hope that getting as many useful indications as possible is
> > > most helpful to the OS.  
> > Nod. Trying this too.
> > My prototype work tries to sync the belowing information from host
> > kernel to qemu:
> > 1. the real SCHIB, so stsch from guest could get the updated path masks.
> 
> How far do you want to go with mirroring? I think you need to modify at
> least the devno in the pmcw, no?
I didn't think this very deep. For now, I only sync the PIM, POM, PAM
and CHPIDs lazily.

For devno... I need to think more. If the qemu command has a given
"devno" for the vfio-ccw device, maybe we should not override its dev_id
with the real one "device number".

> 
> > 2. the Store Subchannel Description information,
Ref. chp_ssd_get_mask().
I can get the valid CHPIDs for those channel paths defined for the
device associated with the specified subchannel.

> > and with the new added support for the SCLP read channel path
> > information command, guest could get the configure status of the
> > path.
> 
> That's also a chsc, right?
Right.

> 
> > 3. still working on support CHSC store channel path description command.
> 
> I'm currently wondering how many of those chscs are optional. OTOH, if
> a modern Linux guest cannot work properly without them, it makes no
> sense to leave them out.
Nod.

But I think I need to define the criteria for "work properly". For
example, with the current code, a Linux guest with a passed through
device works, while lschp shows the Cfg. as 3 (not recognized), and the
Shared and PCHID as "-". For this case, do you think it "work properly"?

> 
> > 
> > > (I had added the path-come CRW handling in Linux back then and
> > > afterwards wondered why we did not get it - I must have interpreted
> > > the PoP in the same way as you did.)  
> > I've a bugfix patch in the kernel side, and it has been accepted by the
> > s390 maintainers. May be this could be a clue? Post it here:
> > 
> > When channel path is identified as the report source code (RSC)
> > of a CRW, and initialized (CRW_ERC_INIT) is recognized as the
> > error recovery code (ERC) by the channel subsystem, it indicates
> > a "path has come" event.
> > 
> > Let's handle this case in chp_process_crw().
> > 
> > diff --git a/drivers/s390/cio/chp.c b/drivers/s390/cio/chp.c
> > index 7e0d4f724dda..432fc40990bd 100644
> > --- a/drivers/s390/cio/chp.c
> > +++ b/drivers/s390/cio/chp.c
> > @@ -559,6 +559,7 @@ static void chp_process_crw(struct crw *crw0, struct
> > crw *crw1,
> >         chpid.id = crw0->rsid;
> >         switch (crw0->erc) {
> >         case CRW_ERC_IPARM: /* Path has come. */
> > +       case CRW_ERC_INIT:
> >                 if (!chp_is_registered(chpid))
> >                         chp_new(chpid);
> >                 chsc_chp_online(chpid);
> > 
> > Notice:
> > At the very beginning, I replaced CRW_ERC_IPARM with CRW_ERC_INIT. But
> > Sebstian Ott suggested:
> > "I don't know of a machine that actually implements a CRW
> > at all when a chpid is configured online on the SE/HMC.
> > 
> > Because of potential regressions I don't want to remove CRW_ERC_IPARM
> > here. I'm good with adding CRW_ERC_INIT though."
> 
> Yeah, that makes sense, especially with the confusing state channel
> path machine check handling is in from the architecture side.
> 
> > 
> > > 
> > > I'll double check with how I'd interpret the PoP today.
> > >   
> > Thanks.
> 
> I have read through the PoP and the outcome is a bit disappointing.
> Much of it is a bit vague. I still think that you can err on the side
> of overindication, though.
> 
I agree. Unless somebody tells me it's forbidden by the PoP explicitly,
or it will break Linux guest from working properly, I will would this as
the way to go.

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 0/3] Channel Path realted CRW generation

[Cornelia Huck]

On Fri, 28 Jul 2017 23:50:48 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> * Cornelia Huck <cohuck@redhat.com> [2017-07-28 13:53:01 +0200]:

> > > > You're bound to get different kinds of notifications: via a CRW with
> > > > source channel path, via event information retrievable via CHSC
> > > > (indicated by a CRW with source CSS),    
> > > Ha, I was not awre of this one before!  
> > 
> > That's the 'link incident' and 'resource accessibility' stuff.  
> My focus was trying to have the minimum stuff to make a Linux guest
> working well -- basically, my working on prototype targeted to make the
> output lschp and lscss corect and uptodate. I
> 
> I will dig this and see if I need to do more stuff.

You can probably skip this for now, unless you want to propagate the
ficon-related stuff. Just plain channel-path related changes should
already cover the interesting stuff.

> > > My prototype work tries to sync the belowing information from host
> > > kernel to qemu:
> > > 1. the real SCHIB, so stsch from guest could get the updated path masks.  
> > 
> > How far do you want to go with mirroring? I think you need to modify at
> > least the devno in the pmcw, no?  
> I didn't think this very deep. For now, I only sync the PIM, POM, PAM
> and CHPIDs lazily.

Also consider the pno bit and the pnom.

> For devno... I need to think more. If the qemu command has a given
> "devno" for the vfio-ccw device, maybe we should not override its dev_id
> with the real one "device number".

The guest should not be surprised by a different devno, so you need to
be sure everything is consistent.


> > > 3. still working on support CHSC store channel path description command.  
> > 
> > I'm currently wondering how many of those chscs are optional. OTOH, if
> > a modern Linux guest cannot work properly without them, it makes no
> > sense to leave them out.  
> Nod.
> 
> But I think I need to define the criteria for "work properly". For
> example, with the current code, a Linux guest with a passed through
> device works, while lschp shows the Cfg. as 3 (not recognized), and the
> Shared and PCHID as "-". For this case, do you think it "work properly"?

It depends upon what you want to expose to the guest. Some
configuration checking or management tools might be reporting a
configuration deficiency (*might*, I do not know).

Shared and PGID may be useful if the operator wants to perform some
maintenance on the hardware (so they can figure out which systems/disks
are affected), but the information should be available in the
hypervisor as well, so I'm not sure whether it's a big deal.

Re: [Qemu-devel] [PATCH 0/3] Channel Path realted CRW generation

[Dong Jia Shi]

* Cornelia Huck <cohuck@redhat.com> [2017-07-31 10:54:47 +0200]:

> On Fri, 28 Jul 2017 23:50:48 +0800
> Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:
> 
> > * Cornelia Huck <cohuck@redhat.com> [2017-07-28 13:53:01 +0200]:
> 
> > > > > You're bound to get different kinds of notifications: via a CRW with
> > > > > source channel path, via event information retrievable via CHSC
> > > > > (indicated by a CRW with source CSS),    
> > > > Ha, I was not awre of this one before!  
> > > 
> > > That's the 'link incident' and 'resource accessibility' stuff.  
> > My focus was trying to have the minimum stuff to make a Linux guest
> > working well -- basically, my working on prototype targeted to make the
> > output lschp and lscss corect and uptodate. I
> > 
> > I will dig this and see if I need to do more stuff.
> 
> You can probably skip this for now, unless you want to propagate the
> ficon-related stuff.
I don't even want to know about that now. ;)

> Just plain channel-path related changes should already cover the
> interesting stuff.
> 
> > > > My prototype work tries to sync the belowing information from host
> > > > kernel to qemu:
> > > > 1. the real SCHIB, so stsch from guest could get the updated path masks.  
> > > 
> > > How far do you want to go with mirroring? I think you need to modify at
> > > least the devno in the pmcw, no?  
> > I didn't think this very deep. For now, I only sync the PIM, POM, PAM
> > and CHPIDs lazily.
> 
> Also consider the pno bit and the pnom.
Roger!

> 
> > For devno... I need to think more. If the qemu command has a given
> > "devno" for the vfio-ccw device, maybe we should not override its dev_id
> > with the real one "device number".
> 
> The guest should not be surprised by a different devno, so you need to
> be sure everything is consistent.
Ok. Will handle the device number.

> 
> 
> > > > 3. still working on support CHSC store channel path description command.  
> > > 
> > > I'm currently wondering how many of those chscs are optional. OTOH, if
> > > a modern Linux guest cannot work properly without them, it makes no
> > > sense to leave them out.  
> > Nod.
> > 
> > But I think I need to define the criteria for "work properly". For
> > example, with the current code, a Linux guest with a passed through
> > device works, while lschp shows the Cfg. as 3 (not recognized), and the
> > Shared and PCHID as "-". For this case, do you think it "work properly"?
> 
> It depends upon what you want to expose to the guest. Some
> configuration checking or management tools might be reporting a
> configuration deficiency (*might*, I do not know).
This is helpful.

> 
> Shared and PGID may be useful if the operator wants to perform some
> maintenance on the hardware (so they can figure out which systems/disks
> are affected), but the information should be available in the
> hypervisor as well, so I'm not sure whether it's a big deal.
> 
Oh! This information is also very helpful.

Since I only want to expose the minimum information that the guest needs
to work without serious problem. I think I can also deffer these stuff
until we have the good chp modelling.

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 0/3] Channel Path realted CRW generation

[Cornelia Huck]

On Tue, 1 Aug 2017 10:12:31 +0800
Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com> wrote:

> Since I only want to expose the minimum information that the guest needs
> to work without serious problem. I think I can also deffer these stuff
> until we have the good chp modelling.

Yes, that's probably the best way to proceed.