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