| Message ID | 20170727015418.85407-4-bjsdjshi@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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(-)
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(-) >
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.
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. >
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. > > >
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
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!
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
* 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... [...]
* 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(-) >
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 :)
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.
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.
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
* 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!
* 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. ;)
* 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!
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.
* 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!
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
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(-)