Re: [Qemu-devel] [PATCH] vfio-ccw: license text should indicate GPL v2 or later
* Cornelia Huck [2018-02-27 18:32:51 +0100]: > The license text currently specifies "any version" of the GPL. It > is unlikely that GPL v1 was ever intended; change this to the > standard "or any later version" text. > > Cc: Dong Jia Shi > Cc: Xiao Feng Ren > Cc: Pierre Morel > Signed-off-by: Cornelia Huck > --- > hw/vfio/ccw.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index 16713f2c52..4e5855741a 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -6,8 +6,8 @@ > *Xiao Feng Ren > *Pierre Morel > * > - * This work is licensed under the terms of the GNU GPL, version 2 or(at > - * your option) any version. See the COPYING file in the top-level > + * This work is licensed under the terms of the GNU GPL, version 2 or (at > + * your option) any later version. See the COPYING file in the top-level > * directory. > */ > Dunno why the word 'later' is missing... could be some copy/paste mistake. Reviewed-by: Dong Jia Shi -- Dong Jia Shi
Re: [Qemu-devel] [RFC PATCH 0/3] vfio: ccw: basic channel path event handling
my current working project... I >> really want to deliver a minimal function set in the near future >> firstly. If the current working does not hurt, can we defer channel path >> re-modelling and CHPIDs virtualization? > > The problem is, I'm not sure it does not hurt. For instance because of > the question below. I would also prefer having a fair idea of what > we need for the envisioned (complete) solution before introducing > kernel interfaces (to avoid: pity in the end we need something > different, and resulting cluttered interface). > >> >>> What if we have a clash? Lets say my dasd is only accessible via chp >>> 0.00 in the host. Currently we have a problem there, or (as the only >>> path would end up being ignored)? >> You mean the clash that sharing path 0.00 between a physical dasd and >> the virtio ccw devices? The problem I saw is in the checking of the >> chpid.is_virtual in css_do_rchp(). For a virtual chp, we should generate >> INIT CRWs; while for a non-virtual one, we shouldn't. If the chp is >> shared with virtual and non-virtual device, I don't know what to do >> then... >> >> I don't think we can fix this before we re-modelled the channel path. >> But this is neither introduced nor aggravated by this series, right? > > I'm not sure about the later. What prevents the guest from believing > the (to use your terminology shared) chp 0.00 has become unavailable > if 0.00 becomes unavailable in the host? > > Would that adversely affect the virtual devices? It would at > least look strange. AFAI read the code, this series does not hurt the vritual devices, which always have fixed path information in SCHIB. The strange thing is, the chp 0.00 is both virtual and non-virtual with mix use of virtual and non-virtual devices. And any existing problem is not caused or enhanced by this work. If we can not fix the problem without MCSS support or chp re-modelling in the near future, we should document the problem and the effect. I will do some test on this. > >> >> We'd have to document this either I think. >> >>> Note: this is another unpleasant effect of not having MCSSE in the >>> guest. The original design was to have a separate css for vfio, and >>> then we would not have this kind of a problem. (Virtualization of >>> chps would still be good for migration.) >> Nod. BTW, I don't say that I do not want channel path virtualization in >> the long run. It's just I want a minimal function set more currently >> from what I'm focusing perspective. >> >> THANKS for these insightful questions! >> -- Dong Jia Shi
Re: [Qemu-devel] [RFC PATCH 0/3] vfio: ccw: basic channel path event handling
* Dong Jia Shi [2018-01-30 11:37:43 +0800]: Damn.. Please just ignore the above mail. It's not the right draft. > Halil Pasic writes: > > Hi Halil, > > As you may noticed, Conny replied to this thread on my mail. Some of her > comments there could answer your questions. If that applies, I will just > say "See Conny's mail" in the following, and you can reply to that mail, > so we can consolidate further discussion. > > >>> * When a channel path malfunctions a CRW is generated and a machine > >>> check channel report pending is generated. Same goes for > >>> channel paths popping up (on HW level). Should not these get > >>> propagated? > >> AFAIR, channel path related CRW is not that reliable. I mean it seems > >> that it's not mandatory to generate CRWs for channel path malfunctions. > >> AFAIU, it depneds on machine models. For example, we won't get > >> CRW_ERC_INIT for a "path has come" event on many models I believe. And > >> that's why nobody noticed the misuse of CRW_ERC_IPARM and CRW_ERC_INIT > >> in the CRW handling logic for channel path CRWs. > >> Ref: > >> 2daace78a8c9 s390: chp: handle CRW_ERC_INIT for channel-path status change > >> > > > > Honestly, I forgot about this discussion. I've refreshed my memories by > > now, but I could not find why is it supposed to be architecturally OK > > to loose CRWs when for instance a chp goes away. > > > >> So my understanding for this is: it really a design decision for us to > >> propagate all of the channel path CRWs, or we just use other ways (like > >> using PNO and PNOM as this series shows). > > > > From what I read, CRWs did not seem optional. > > I wonder what should I read in order to change my mind. I'm not > > talking about the hardware in the wild -- but that could be buggy > > hardware. > > > Ah, can you point out the chapter that says CRWs is mandatory? > > AFAIU, PoP doesn't say, for example, chp gone will lead to a CRW, but it > only says when we got a chp gone CRW it means a chp has been gone. > > [See Conny's mail pls, and we can discuss there.] > > >> > >> Of course, CRW propagation is good to have. But as a discussion result > >> of a former thread, that is something we can consider only after we have > >> a good channel path re-modelling. That is the problem. We can try to > >> trigger CRW event in some work of machine checks handling enhancement > >> later. > >> > >>> * Kind of instead of the CRW you introduce a per device interrupt > >>> for channel path events on the qemu/kvm boundary. AFAIU for a chp > >>> event you do an STSCH for each (affected?) subchannel > >> Until here, yes and right. > >> > >>> and generate an irq. Right? Why is this a good idea. > >> This is not right. This series does not generate an irq for the guest. > > > > You misunderstood this. The word 'irq' this sentence is a short version > > of 'per device interrupt for channel path events on the qemu/kvm boundary' > > form the previous sentence. It's not an irq injected into the guest but > > a notification (which you call irq in '[RFC PATCH 2/3] vfio: ccw: > > introduce channel path irq') for QEMU. > > > I see now. I misunderstood. > > >> In QEMU, when it gets the notification of a channel path status change > >> event, it read the newest SCHIB from the schib region, and update the > >> SCHIB (patch related parts) for the virtual subchannel. The guest will > >> get the new SCHIB whenever it calls STSCH then. > > > > We are in agreement. I just wanted to say, if let's say 42 vfio-ccw devices > > are using the same chp and it goes away, you will generate 42 notifications. > Yes, and this is the only way we can do for now, since there is no good > chp model, we can't use CRWs to consolidate the notifications... Once we > had the model and we can use CRW to indicate chp event, this could be > easily removed and changed to that. > > Once I had a version which leverages chp CRWs. But since we had > agreement that CRW related code change will not be acceptable in the > QEMU side before we have chp re-modelling doen, I changed to this PNO > and PNOM implementation. > > > > >> > >> I think this is a good idea, because: > >> 1. This is complies the Linux implementation. Path status change could > >>be noticed by Linux. > >> 2. This provides enhancement wi
Re: [Qemu-devel] [RFC PATCH 0/3] vfio: ccw: basic channel path event handling
or instance because of > the question below. I would also prefer having a fair idea of what > we need for the envisioned (complete) solution before introducing > kernel interfaces (to avoid: pity in the end we need something > different, and resulting cluttered interface). > >> >>> What if we have a clash? Lets say my dasd is only accessible via chp >>> 0.00 in the host. Currently we have a problem there, or (as the only >>> path would end up being ignored)? >> You mean the clash that sharing path 0.00 between a physical dasd and >> the virtio ccw devices? The problem I saw is in the checking of the >> chpid.is_virtual in css_do_rchp(). For a virtual chp, we should generate >> INIT CRWs; while for a non-virtual one, we shouldn't. If the chp is >> shared with virtual and non-virtual device, I don't know what to do >> then... >> >> I don't think we can fix this before we re-modelled the channel path. >> But this is neither introduced nor aggravated by this series, right? > > I'm not sure about the later. What prevents the guest from believing > the (to use your terminology shared) chp 0.00 has become unavailable > if 0.00 becomes unavailable in the host? > > Would that adversely affect the virtual devices? It would at > least look strange. > >> >> We'd have to document this either I think. >> >>> Note: this is another unpleasant effect of not having MCSSE in the >>> guest. The original design was to have a separate css for vfio, and >>> then we would not have this kind of a problem. (Virtualization of >>> chps would still be good for migration.) >> Nod. BTW, I don't say that I do not want channel path virtualization in >> the long run. It's just I want a minimal function set more currently >> from what I'm focusing perspective. >> >> THANKS for these insightful questions! >> -- Dong Jia Shi
Re: [Qemu-devel] [RFC PATCH 0/3] vfio: ccw: basic channel path event handling
* Halil Pasic [2018-01-16 16:57:13 +0100]: > > > On 01/15/2018 09:59 AM, Dong Jia Shi wrote: > > * Halil Pasic [2018-01-12 19:10:20 +0100]: > > > >> > >> > >> On 01/11/2018 04:04 AM, Dong Jia Shi wrote: > >>> What are still missing, thus need to be offered in the next version are: > >>> - I/O termination and FSM state handling if currently we have I/O on the > >>> status > >>> switched path. > >>> - Vary on/off event is not sensible to a guest. > >> > >> I don't see a doc update. We do have documentation (in > >> Documentation/s390/vfio-ccw.txt) in which the uapi interface with the > >> regions and their purpose/usage is at least kind of explained. You are > >> changing this interface without updating the doc. > >> > >> I would like to see documentation on this because I'm under the > >> impression either the design is pretty convoluted or I did not > >> get it at all. > > Ah, I missed the documentation part. Thanks for pointing out. I will add > > it in the next cycle. > > > > I would have preferred having a doc update in this cycle. E.g. as > an answer to the cover letter. Ok. If you prefer this, let's try to clarify questions right here and update documentations in the next review cycle (if there is any). > > As previously pointed out I don't really understand your design. > I wanted to avoid summarizing the design myself (that is my understanding > of it), to then question the design. Fair enough. > > To give you a feeling of what I mean here some bullet points: > * Channel paths are css level resources (simplified). Yes, and it's the means for the machine to talk to the device. > * When a channel path malfunctions a CRW is generated and a machine > check channel report pending is generated. Same goes for > channel paths popping up (on HW level). Should not these get > propagated? AFAIR, channel path related CRW is not that reliable. I mean it seems that it's not mandatory to generate CRWs for channel path malfunctions. AFAIU, it depneds on machine models. For example, we won't get CRW_ERC_INIT for a "path has come" event on many models I believe. And that's why nobody noticed the misuse of CRW_ERC_IPARM and CRW_ERC_INIT in the CRW handling logic for channel path CRWs. Ref: 2daace78a8c9 s390: chp: handle CRW_ERC_INIT for channel-path status change So my understanding for this is: it really a design decision for us to propagate all of the channel path CRWs, or we just use other ways (like using PNO and PNOM as this series shows). Of course, CRW propagation is good to have. But as a discussion result of a former thread, that is something we can consider only after we have a good channel path re-modelling. That is the problem. We can try to trigger CRW event in some work of machine checks handling enhancement later. > * Kind of instead of the CRW you introduce a per device interrupt > for channel path events on the qemu/kvm boundary. AFAIU for a chp > event you do an STSCH for each (affected?) subchannel Until here, yes and right. > and generate an irq. Right? Why is this a good idea. This is not right. This series does not generate an irq for the guest. In QEMU, when it gets the notification of a channel path status change event, it read the newest SCHIB from the schib region, and update the SCHIB (patch related parts) for the virtual subchannel. The guest will get the new SCHIB whenever it calls STSCH then. I think this is a good idea, because: 1. This is complies the Linux implementation. Path status change could be noticed by Linux. 2. This provides enhancement with a small work. On the contrary, channel path re-modelling needs a lot of work. > * SCHIB.PMCW provides path information relevant for a given device. > This information is retrieved using store subchannel. Is your series > sufficient for making store subchannel work properly (correct and > reasonably up to date)? Introducing a SCHIB region is the basis of further STSCH hanlding work. This series depends on it, and only focuses on the update of the channel path related parts of it. And for these parts, this work I think is basically correct (more review comments are surely to be welcomed). For the rest parts, this does not change anything than what have. As replied to Conny in other mail of this thread: I think, if the other fields are handled by QEMU well, then we don't need to trigger update events for them. Currently I don't find things that need extra trigger. > Regarding PMCW it's supposed to get updated when io functions are > preformed by the css, AFAIR. Is that right? I think so. And also for some other cases, for example, when we configure on/off a channel path
Re: [Qemu-devel] [RFC PATCH 0/3] vfio: ccw: basic channel path event handling
* Pierre Morel [2018-01-15 11:21:47 +0100]: > On 15/01/2018 09:57, Dong Jia Shi wrote: > >* Cornelia Huck [2018-01-11 11:54:22 +0100]: > > > >>On Thu, 11 Jan 2018 04:04:18 +0100 > >>Dong Jia Shi wrote: > >> > >>>Hi Folks, > >>> > >>>Background > >>>== > >>> > >>>Some days ago, we had a discussion on the topic of channel path > >>>virtualization. > >>>Ref: > >>>Subject: [PATCH 0/3] Channel Path realted CRW generation > >>>Message-Id: <20170727015418.85407-1-bjsdj...@linux.vnet.ibm.com> > >>>URL: https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg08414.html > >>> > >>>Indeed that thread is not short and discussed many aspects in a > >>>non-concentrated manner. The parts those are most valuable to me are: > >>>1. a re-modelling for channel path is surely the best offer, but is not > >>>possible to have in the near future. > >>>2. to enhance the path related functionalities, using PNO and PNOM might > >>>be something we can do for now. This may be something that I could > >>> improve > >>>without model related arguments. > >>> > >>>So here I have this series targeting to add basic channel path event > >>>handling > >>>for vfio-ccw -- no touch of the channel path modelling in both the kernel > >>>and > >>>the QEMU side, but find a way to sync path status change to guest lazily > >>>using > >>>SCSW_FLAGS_MASK_PNO and pmcw->pnom. In short, I want to enhance path > >>>related > >>>stuff (to be more specific: sync up path status to the guest) on a best > >>>effort > >>>basis, which means in a way that won't get us invloed to do channel path > >>>re-modelling. > >>The guest should also get the updated PIM/PAM/POM, shouldn't it? > >> > >Yes. The following values will be updated for the guest: > >PMCW: > > - PIM/PAM/POM > > - PNOM > > - CHPIDs > >SCSW > > - PNOM bit > > > >See vfio_ccw_update_schib in patch #4 of the QEMU series. > > > >>>What benifit can we get from this? The administrator of a virtual machine > >>>can > >>>get uptodate (in some extent) status of the current using channel paths, so > >>>he/she can monitor paths status and get path problem noticed timely (see > >>>the > >>>example below). > >>> > >>>I think we can start a new round discussion based on this series. So > >>>reviewers > >>>can give their comments based on some code, and then we can decide if this > >>>is > >>>we want or not. > >>> > >>>As flagged with RFC, the intention of this series is to show what I have > >>>for > >>>now, and what could the code look like in general. Thus I can get some > >>>early > >>>feedbacks. I would expect to see opinions on: > >>>- is the target (mentioned above) of this series welcomed or not. > >>It certainly makes sense to have a way to get an updated schib. > >> > >:) > > I think so too, if the guest's administrator wants to be able to do > something. > > But I would like to see something about path virtualization. Me too... As pointed in the discussion thread (URL listed above), this is something that really hard to have in the near future. The question is do we want some enhancements like this without channel path re-modelling, or we want nothing until we have the re-modelling firstly? > Having more accurate information on hardware without virtualization is a > big handicap for migration and hotplug. > vfio-ccw does not support migration. What could be the handicap for that? :^) -- Dong Jia Shi
Re: [Qemu-devel] [RFC PATCH 1/3] vfio: ccw: introduce schib region
* Cornelia Huck [2018-01-15 13:24:22 +0100]:
> On Mon, 15 Jan 2018 10:50:00 +0100
> Pierre Morel wrote:
>
> > On 11/01/2018 04:04, Dong Jia Shi wrote:
> > > This introduces a new region for vfio-ccw to provide subchannel
> > > information for user space.
> > >
> > > Signed-off-by: Dong Jia Shi
> > > ---
> > > drivers/s390/cio/vfio_ccw_fsm.c | 21 ++
> > > drivers/s390/cio/vfio_ccw_ops.c | 79
> > > +++--
> > > drivers/s390/cio/vfio_ccw_private.h | 3 ++
> > > include/uapi/linux/vfio.h | 1 +
> > > include/uapi/linux/vfio_ccw.h | 6 +++
> > > 5 files changed, 90 insertions(+), 20 deletions(-)
> > >
>
> > > diff --git a/drivers/s390/cio/vfio_ccw_private.h
> > > b/drivers/s390/cio/vfio_ccw_private.h
> > > index 78a66d96756b..460c8b90d834 100644
> > > --- a/drivers/s390/cio/vfio_ccw_private.h
> > > +++ b/drivers/s390/cio/vfio_ccw_private.h
> > > @@ -28,6 +28,7 @@
> > >* @mdev: pointer to the mediated device
> > >* @nb: notifier for vfio events
> > >* @io_region: MMIO region to input/output I/O arguments/results
> > > + * @schib_region: MMIO region of subchannel information
> > >* @cp: channel program for the current I/O operation
> > >* @irb: irb info received from interrupt
> > >* @scsw: scsw info
> > > @@ -42,6 +43,7 @@ struct vfio_ccw_private {
> > > struct mdev_device *mdev;
> > > struct notifier_block nb;
> > > struct ccw_io_regionio_region;
> > > + struct ccw_schib_region schib_region;
> > >
> > > struct channel_program cp;
> > > struct irb irb;
> >
> > Hi,
> >
> > I have a problem with these patches: you have 3 definitions for the
> > subchannel status word:
> > 1: direct in the scsw entry of the vfio_ccw_private structure
> > 2: indirect in the IRB structure irb
> > 3: now in the scsw of the schib_region
> >
> > How do you keep them all in sync?
> >
For the first two cases, we might need to keep them synced in the kernel
if upper level application requires. Otherwise, we can let upper level
application do the sync.
The 3rd one is used only as an input parameter to indicate function
types. To be more specific, we currently only has interests for its
Function Control field. So, sync of this one is not applicable.
> > The direct scsw in io region seems to only serve as a trigger used by
> > userland, while
> > the IRB in the io region and the SCHIB in the schib region are updated
> > asynchronously,
> > from hardware, one by polling (scsw in schib region), one by IRQ (scsw
> > in irb in io region).
> >
> > I find this at least a source for obfuscation.
>
> I agree that this wants some more documentation.
Ok.
>
> However, some of this structure duplication is a consequence of the
> hardware design, because the scsw is present in both the schib (updated
> by stsch) and the irb (updated by tsch). There are cases where the irb
> is simply not enough (it does not contain a pmcw, and you can only do
> tsch on an enabled subchannel). So I think that we really need two
> structures for those two distinct operations (and everything
> interfacing with this needs to keep synced on the scsw, as current
> users of stsch/tsch already need to do now).
>
Nod.
> The direct scsw entry is used for initiating the different functions
> (only start right now), I don't really see a good alternative for that
> (and I also don't really see any problem with needed syncing.)
>
+1
--
Dong Jia Shi
Re: [Qemu-devel] [RFC PATCH 0/3] vfio: ccw: basic channel path event handling
* Halil Pasic [2018-01-12 19:10:20 +0100]: > > > On 01/11/2018 04:04 AM, Dong Jia Shi wrote: > > What are still missing, thus need to be offered in the next version are: > > - I/O termination and FSM state handling if currently we have I/O on the > > status > > switched path. > > - Vary on/off event is not sensible to a guest. > > I don't see a doc update. We do have documentation (in > Documentation/s390/vfio-ccw.txt) in which the uapi interface with the > regions and their purpose/usage is at least kind of explained. You are > changing this interface without updating the doc. > > I would like to see documentation on this because I'm under the > impression either the design is pretty convoluted or I did not > get it at all. Ah, I missed the documentation part. Thanks for pointing out. I will add it in the next cycle. -- Dong Jia Shi
Re: [Qemu-devel] [RFC PATCH 0/3] vfio: ccw: basic channel path event handling
* Cornelia Huck [2018-01-11 11:54:22 +0100]: > On Thu, 11 Jan 2018 04:04:18 +0100 > Dong Jia Shi wrote: > > > Hi Folks, > > > > Background > > == > > > > Some days ago, we had a discussion on the topic of channel path > > virtualization. > > Ref: > > Subject: [PATCH 0/3] Channel Path realted CRW generation > > Message-Id: <20170727015418.85407-1-bjsdj...@linux.vnet.ibm.com> > > URL: https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg08414.html > > > > Indeed that thread is not short and discussed many aspects in a > > non-concentrated manner. The parts those are most valuable to me are: > > 1. a re-modelling for channel path is surely the best offer, but is not > >possible to have in the near future. > > 2. to enhance the path related functionalities, using PNO and PNOM might > >be something we can do for now. This may be something that I could > > improve > >without model related arguments. > > > > So here I have this series targeting to add basic channel path event > > handling > > for vfio-ccw -- no touch of the channel path modelling in both the kernel > > and > > the QEMU side, but find a way to sync path status change to guest lazily > > using > > SCSW_FLAGS_MASK_PNO and pmcw->pnom. In short, I want to enhance path > > related > > stuff (to be more specific: sync up path status to the guest) on a best > > effort > > basis, which means in a way that won't get us invloed to do channel path > > re-modelling. > > The guest should also get the updated PIM/PAM/POM, shouldn't it? > Yes. The following values will be updated for the guest: PMCW: - PIM/PAM/POM - PNOM - CHPIDs SCSW - PNOM bit See vfio_ccw_update_schib in patch #4 of the QEMU series. > > > > What benifit can we get from this? The administrator of a virtual machine > > can > > get uptodate (in some extent) status of the current using channel paths, so > > he/she can monitor paths status and get path problem noticed timely (see the > > example below). > > > > I think we can start a new round discussion based on this series. So > > reviewers > > can give their comments based on some code, and then we can decide if this > > is > > we want or not. > > > > As flagged with RFC, the intention of this series is to show what I have for > > now, and what could the code look like in general. Thus I can get some early > > feedbacks. I would expect to see opinions on: > > - is the target (mentioned above) of this series welcomed or not. > > It certainly makes sense to have a way to get an updated schib. > :) > > - is the approach of this series good or bad. > > Still need to read :) > > > So I can either move on with this (or with other suggested approach) or > > leave > > it alone. > > > > Basic Introduction of The Patches > > = > > > > This is the kernel counterpart, which mainly does: > > 1. add a schib vfio region for userland to _store_ subchannel information. > > 2. add a channel path vfio irq to notify userland with chp status change > > event. > > 3. add .chp_event handler for vfio-ccw driver, so the driver handles chp > > event, > >and signals userland about the event. > > Do you plan to trigger schib updates for things other than path events? > This is surely a good question... and my answer is no. If the other fields are handled by QEMU well, then we don't need to trigger update events for them. Currently I don't find things that need extra trigger. > > > > With the above work, userland can be signaled with chp related event, and > > then > > it can read out uptodate SCHIB from the new region, and sync up path related > > information to the corresponding virtual subchannel. So a guest can sense > > the > > path update in some extent. > > That's basically what Linux could do before implementing chpid related > machine checks, so it should be at least helpful. > > > > > For the QEMU counterpart, please ref: > > [RFC PATCH 0/5] vfio/ccw: basic channel path event handling > > > > The QEMU counterpart mainly does: > > 1. add handling of the schib region, so that it can read out the newest > > schib > >information. > > 2. add handling of the chp irq, so that it can get notification of channel > > path > >status change. > > 3. once there is a chp status event, synchronize related information from > > the > >newest schib inform
Re: [Qemu-devel] [RFC PATCH 1/3] vfio: ccw: introduce schib region
* Cornelia Huck [2018-01-11 15:16:59 +0100]:
Hi Conny,
> On Thu, 11 Jan 2018 04:04:19 +0100
> Dong Jia Shi wrote:
>
> > This introduces a new region for vfio-ccw to provide subchannel
> > information for user space.
> >
> > Signed-off-by: Dong Jia Shi
> > ---
> > drivers/s390/cio/vfio_ccw_fsm.c | 21 ++
> > drivers/s390/cio/vfio_ccw_ops.c | 79
> > +++--
> > drivers/s390/cio/vfio_ccw_private.h | 3 ++
> > include/uapi/linux/vfio.h | 1 +
> > include/uapi/linux/vfio_ccw.h | 6 +++
> > 5 files changed, 90 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/s390/cio/vfio_ccw_fsm.c
> > b/drivers/s390/cio/vfio_ccw_fsm.c
> > index c30420c517b1..be081ccabea3 100644
> > --- a/drivers/s390/cio/vfio_ccw_fsm.c
> > +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> > @@ -172,6 +172,22 @@ static void fsm_irq(struct vfio_ccw_private *private,
> > complete(private->completion);
> > }
> >
> > +static void fsm_update_subch(struct vfio_ccw_private *private,
> > +enum vfio_ccw_event event)
> > +{
> > + struct subchannel *sch;
> > +
> > + sch = private->sch;
> > + if (cio_update_schib(sch)) {
>
> This implies device gone. Do we also want to trigger some event, or
> just wait until a machine check comes around and we're notified in the
> normal way? (Probably the latter.)
>
We'd need to handle machine checks better anyway, and we can trigger
event there. I think we can choose the latter one.
> > + private->schib_region.cc = 3;
> > + return;
> > + }
> > +
> > + private->schib_region.cc = 0;
> > + memcpy(private->schib_region.schib_area, &sch->schib,
> > + sizeof(sch->schib));
>
> We might want to add documentation that schib_area contains the schib
> from the last successful invocation of stsch (if any). That makes sense
> as the schib remains unchanged for cc=3 after stsch anyway, but it
> can't hurt to spell it out.
>
PoP doesn't say anything about the content of SCHIB when cc=3. So it's
fine to remain the last content I guess. I can add comments here and
document in vfio-ccw.txt. Ok?
> > +}
> > +
> > /*
> > * Device statemachine
> > */
> > @@ -180,25 +196,30 @@ fsm_func_t
> > *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
> > [VFIO_CCW_EVENT_NOT_OPER] = fsm_nop,
> > [VFIO_CCW_EVENT_IO_REQ] = fsm_io_error,
> > [VFIO_CCW_EVENT_INTERRUPT] = fsm_disabled_irq,
> > + [VFIO_CCW_EVENT_UPDATE_SUBCH] = fsm_update_subch,
> > },
> > [VFIO_CCW_STATE_STANDBY] = {
> > [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
> > [VFIO_CCW_EVENT_IO_REQ] = fsm_io_error,
> > [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq,
> > + [VFIO_CCW_EVENT_UPDATE_SUBCH] = fsm_update_subch,
> > },
> > [VFIO_CCW_STATE_IDLE] = {
> > [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
> > [VFIO_CCW_EVENT_IO_REQ] = fsm_io_request,
> > [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq,
> > + [VFIO_CCW_EVENT_UPDATE_SUBCH] = fsm_update_subch,
> > },
> > [VFIO_CCW_STATE_BOXED] = {
> > [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
> > [VFIO_CCW_EVENT_IO_REQ] = fsm_io_busy,
> > [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq,
> > + [VFIO_CCW_EVENT_UPDATE_SUBCH] = fsm_update_subch,
> > },
> > [VFIO_CCW_STATE_BUSY] = {
> > [VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
> > [VFIO_CCW_EVENT_IO_REQ] = fsm_io_busy,
> > [VFIO_CCW_EVENT_INTERRUPT] = fsm_irq,
> > + [VFIO_CCW_EVENT_UPDATE_SUBCH] = fsm_update_subch,
>
> Does it makes to trigger this through the state machine if we always do
> the same action and never change state?
Yes. Ah, are you implying that we can call update_subch directly without
state machine involved? If so, I agree. There seems no benifit to add
a new VFIO_CCW_EVENT_UPDATE_SUBCH event to the FSM.
>
> > },
> > };
>
> Else, looks good.
>
Thanks for the comments. :)
--
Dong Jia Shi
[Qemu-devel] [RFC PATCH 5/5] vfio/ccw: build schib with real schib information
The current implementation grabs chpids and path masks from
sysfs to build the schib and chp for the virtual subchannels.
Since now vfio-ccw provides a schib region for store subchannel
information. Let's leverage it to get the chipids and the masks,
and serve the virtual subchannels.
While we are at it, touch one line of the comment around by
adding white space to make it aligned.
Signed-off-by: Dong Jia Shi
---
hw/s390x/css.c | 83 -
hw/s390x/s390-ccw.c | 20 +++
hw/vfio/ccw.c | 28 +--
include/hw/s390x/s390-ccw.h | 3 +-
4 files changed, 39 insertions(+), 95 deletions(-)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index c1ec83f08f..b9bc489e55 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -2417,72 +2417,6 @@ SubchDev *css_create_sch(CssDevId bus_id, bool
squash_mcss, Error **errp)
return sch;
}
-static int css_sch_get_chpids(SubchDev *sch, CssDevId *dev_id)
-{
-char *fid_path;
-FILE *fd;
-uint32_t chpid[8];
-int i;
-PMCW *p = &sch->curr_status.pmcw;
-
-fid_path = g_strdup_printf("/sys/bus/css/devices/%x.%x.%04x/chpids",
- dev_id->cssid, dev_id->ssid, dev_id->devid);
-fd = fopen(fid_path, "r");
-if (fd == NULL) {
-error_report("%s: open %s failed", __func__, fid_path);
-g_free(fid_path);
-return -EINVAL;
-}
-
-if (fscanf(fd, "%x %x %x %x %x %x %x %x",
-&chpid[0], &chpid[1], &chpid[2], &chpid[3],
-&chpid[4], &chpid[5], &chpid[6], &chpid[7]) != 8) {
-fclose(fd);
-g_free(fid_path);
-return -EINVAL;
-}
-
-for (i = 0; i < ARRAY_SIZE(p->chpid); i++) {
-p->chpid[i] = chpid[i];
-}
-
-fclose(fd);
-g_free(fid_path);
-
-return 0;
-}
-
-static int css_sch_get_path_masks(SubchDev *sch, CssDevId *dev_id)
-{
-char *fid_path;
-FILE *fd;
-uint32_t pim, pam, pom;
-PMCW *p = &sch->curr_status.pmcw;
-
-fid_path = g_strdup_printf("/sys/bus/css/devices/%x.%x.%04x/pimpampom",
- dev_id->cssid, dev_id->ssid, dev_id->devid);
-fd = fopen(fid_path, "r");
-if (fd == NULL) {
-error_report("%s: open %s failed", __func__, fid_path);
-g_free(fid_path);
-return -EINVAL;
-}
-
-if (fscanf(fd, "%x %x %x", &pim, &pam, &pom) != 3) {
-fclose(fd);
-g_free(fid_path);
-return -EINVAL;
-}
-
-p->pim = pim;
-p->pam = pam;
-p->pom = pom;
-fclose(fd);
-g_free(fid_path);
-
-return 0;
-}
-
static int css_sch_get_chpid_type(uint8_t chpid, uint32_t *type,
CssDevId *dev_id)
{
@@ -2529,19 +2463,14 @@ int css_sch_build_schib(SubchDev *sch, CssDevId *dev_id)
/* We are dealing with I/O subchannels only. */
p->devno = sch->devno;
-/* Grab path mask from sysfs. */
-ret = css_sch_get_path_masks(sch, dev_id);
-if (ret) {
-return ret;
-}
-
-/* Grab chpids from sysfs. */
-ret = css_sch_get_chpids(sch, dev_id);
-if (ret) {
-return ret;
+/* Read schib from the physical device. */
+/* g_assert(sch->update_schib != NULL) ? */
+if (sch->update_schib &&
+(sch->update_schib(sch) != IOINST_CC_EXPECTED)) {
+return -ENODEV;
}
- /* Build chpid type. */
+/* Build chpid type. */
for (i = 0; i < ARRAY_SIZE(p->chpid); i++) {
if (p->chpid[i] && !css->chpids[p->chpid[i]].in_use) {
ret = css_sch_get_chpid_type(p->chpid[i], &type, dev_id);
diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index 491697137c..f658b87131 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -72,7 +72,18 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
cdev->hostid.valid = true;
}
-static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp)
+static void s390_ccw_pre_realize(S390CCWDevice *cdev, char *sysfsdev,
+ Error **errp)
+{
+Error *err = NULL;
+
+s390_ccw_get_dev_info(cdev, sysfsdev, &err);
+if (err) {
+error_propagate(errp, err);
+}
+}
+
+static void s390_ccw_realize(S390CCWDevice *cdev, Error **errp)
{
CcwDevice *ccw_dev = CCW_DEVICE(cdev);
CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev);
@@ -83,11 +94,6 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char
*sysfsdev, Error **errp)
int ret;
Error *err = NULL;
-s390_ccw_get_dev_info(cdev, sysfsdev, &err);
-if (err) {
-goto out_err_propagate;
-}
-
sch = css_create_sch(ccw_dev->devno, cbus->squash_mcss, &err);
if (!sch) {
goto out_mdevid_free;
@@ -1
[Qemu-devel] [RFC PATCH 4/5] vfio/ccw: update subchanel information block lazily
We want to sync up channel path related information between the
physical device and the virtual device. Thus here we read out
subchannel information block from the schib region, and update
the virtual sbuchannel information block.
Since the kernel side will signal userland once it has channel
path information update, we only do update if we were signaled.
We sets scsw.pno and pmcw.pnom to indicate path related event
for a guest. This is a bit lazy, but it works.
Signed-off-by: Dong Jia Shi
---
hw/s390x/css.c | 14 +--
hw/s390x/s390-ccw.c | 12 +
hw/vfio/ccw.c | 59 -
include/hw/s390x/css.h | 3 ++-
include/hw/s390x/s390-ccw.h | 1 +
target/s390x/ioinst.c | 3 +--
6 files changed, 86 insertions(+), 6 deletions(-)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 1c526fd7e2..c1ec83f08f 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1297,11 +1297,16 @@ static void copy_schib_to_guest(SCHIB *dest, const
SCHIB *src)
}
}
-int css_do_stsch(SubchDev *sch, SCHIB *schib)
+IOInstEnding css_do_stsch(SubchDev *sch, SCHIB *schib)
{
+if (sch->update_schib &&
+(sch->update_schib(sch) != IOINST_CC_EXPECTED)) {
+return IOINST_CC_NOT_OPERATIONAL;
+}
+
/* Use current status. */
copy_schib_to_guest(schib, &sch->curr_status);
-return 0;
+return IOINST_CC_EXPECTED;
}
static void copy_pmcw_from_guest(PMCW *dest, const PMCW *src)
@@ -1586,6 +1591,11 @@ int css_do_tsch_get_irb(SubchDev *sch, IRB *target_irb,
int *irb_len)
uint16_t stctl;
IRB irb;
+if (sch->update_schib &&
+(sch->update_schib(sch) != IOINST_CC_EXPECTED)) {
+return IOINST_CC_NOT_OPERATIONAL;
+}
+
if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
return 3;
}
diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index 4a9d4d2534..491697137c 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -28,6 +28,17 @@ IOInstEnding s390_ccw_cmd_request(SubchDev *sch)
return cdc->handle_request(sch);
}
+static IOInstEnding s390_ccw_update_schib(SubchDev *sch)
+{
+S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data);
+
+if (!cdc->update_schib) {
+/* g_assert_not_reached()? */
+return IOINST_CC_NOT_OPERATIONAL;
+}
+return cdc->update_schib(sch);
+}
+
static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
char *sysfsdev,
Error **errp)
@@ -83,6 +94,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char
*sysfsdev, Error **errp)
}
sch->driver_data = cdev;
sch->do_subchannel_work = do_subchannel_work_passthrough;
+sch->update_schib = s390_ccw_update_schib;
ccw_dev->sch = sch;
ret = css_sch_build_schib(sch, &cdev->hostid);
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index d812cecfe0..0eca3453af 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -33,6 +33,7 @@ typedef struct VFIOCCWDevice {
struct ccw_io_region *io_region;
EventNotifier io_notifier;
+bool schib_need_update;
uint64_t schib_region_size;
uint64_t schib_region_offset;
struct ccw_schib_region *schib_region;
@@ -97,6 +98,57 @@ again:
}
}
+static IOInstEnding vfio_ccw_update_schib(SubchDev *sch)
+{
+
+S390CCWDevice *cdev = sch->driver_data;
+VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
+struct ccw_schib_region *region = vcdev->schib_region;
+PMCW *p = &sch->curr_status.pmcw;
+SCSW *s = &sch->curr_status.scsw;
+SCHIB *schib;
+int size;
+int i;
+
+/*
+ * If there is no update that interested us since last read,
+ * we do not read then.
+ */
+if (!vcdev->schib_need_update) {
+return IOINST_CC_EXPECTED;
+}
+vcdev->schib_need_update = false;
+
+/* Read schib region, and update schib for virtual subchannel. */
+size = pread(vcdev->vdev.fd, region, vcdev->schib_region_size,
+ vcdev->schib_region_offset);
+if (size != vcdev->schib_region_size) {
+return IOINST_CC_NOT_OPERATIONAL;
+}
+if (region->cc) {
+g_assert(region->cc == IOINST_CC_NOT_OPERATIONAL);
+return region->cc;
+}
+
+schib = (SCHIB *)region->schib_area;
+
+/* Path mask. */
+p->pim = schib->pmcw.pim;
+p->pam = schib->pmcw.pam;
+p->pom = schib->pmcw.pom;
+
+/* We use PNO and PNOM to indicate path related events. */
+p->pnom = ~schib->pmcw.pam;
+s->flags |= SCSW_FLAGS_MASK_PNO;
+
+/* Chp id. */
+for (i = 0; i < ARRAY_SIZE(p->chpid); i++) {
+p->chpid[i] = schib->pmcw.chpid[i];
+}
+
+return region->cc;
+}
+
static void vfio_ccw_reset(DeviceState
[Qemu-devel] [RFC PATCH 2/5] vfio/ccw: get schib region info
vfio-ccw provides an MMIO region for store subchannel
information. We fetch this information via ioctls here,
then we can use it to update schib for virtual subchannels
later on.
While we are at it, also modify the comment and error
message for the config region a bit, to make these unified
with the schib likeness.
Signed-off-by: Dong Jia Shi
---
hw/vfio/ccw.c | 24 +++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 16713f2c52..e673695509 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -32,6 +32,10 @@ typedef struct VFIOCCWDevice {
uint64_t io_region_offset;
struct ccw_io_region *io_region;
EventNotifier io_notifier;
+
+uint64_t schib_region_size;
+uint64_t schib_region_offset;
+struct ccw_schib_region *schib_region;
} VFIOCCWDevice;
static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
@@ -268,9 +272,10 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev,
Error **errp)
return;
}
+/* Get I/O region info. */
ret = vfio_get_region_info(vdev, VFIO_CCW_CONFIG_REGION_INDEX, &info);
if (ret) {
-error_setg_errno(errp, -ret, "vfio: Error getting config info");
+error_setg_errno(errp, -ret, "vfio: Error getting config region info");
return;
}
@@ -283,13 +288,30 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev,
Error **errp)
vcdev->io_region_offset = info->offset;
vcdev->io_region = g_malloc0(info->size);
+g_free(info);
+/* Get SCHIB region info. */
+ret = vfio_get_region_info(vdev, VFIO_CCW_SCHIB_REGION_INDEX, &info);
+if (ret) {
+error_setg_errno(errp, -ret, "vfio: Error getting schib region info");
+return;
+}
+
+vcdev->schib_region_size = info->size;
+if (sizeof(*vcdev->schib_region) != vcdev->schib_region_size) {
+error_setg(errp, "vfio: Unexpected size of the schib region");
+g_free(info);
+return;
+}
+vcdev->schib_region_offset = info->offset;
+vcdev->schib_region = g_malloc0(info->size);
g_free(info);
}
static void vfio_ccw_put_region(VFIOCCWDevice *vcdev)
{
g_free(vcdev->io_region);
+g_free(vcdev->schib_region);
}
static void vfio_put_device(VFIOCCWDevice *vcdev)
--
2.13.5
[Qemu-devel] [RFC PATCH 0/5] vfio/ccw: basic channel path event handling
Hi Folks, This is the QEMU couterpart for the "basic channel path event handling" series. For more information, please refer to the kernel counterpart. Dong Jia Shi (5): vfio: linux-headers update for vfio-ccw vfio/ccw: get schib region info vfio/ccw: get irq info and set up handler for chp irq vfio/ccw: update subchanel information block lazily vfio/ccw: build schib with real schib information hw/s390x/css.c | 97 hw/s390x/s390-ccw.c| 32 +-- hw/vfio/ccw.c | 199 ++--- include/hw/s390x/css.h | 3 +- include/hw/s390x/s390-ccw.h| 4 +- linux-headers/linux/vfio.h | 2 + linux-headers/linux/vfio_ccw.h | 6 ++ target/s390x/ioinst.c | 3 +- 8 files changed, 224 insertions(+), 122 deletions(-) -- 2.13.5
[Qemu-devel] [RFC PATCH 3/3] vfio: ccw: handle chp event
This adds channel path related event handler for vfio-ccw.
This also signals userland when there is a chp event.
Signed-off-by: Dong Jia Shi
---
drivers/s390/cio/vfio_ccw_drv.c | 51 +
drivers/s390/cio/vfio_ccw_fsm.c | 22
drivers/s390/cio/vfio_ccw_private.h | 3 +++
3 files changed, 76 insertions(+)
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index ea6a2d0b2894..5f01f3e6742d 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -17,6 +17,7 @@
#include
+#include "chp.h"
#include "ioasm.h"
#include "css.h"
#include "vfio_ccw_private.h"
@@ -88,6 +89,15 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
private->state = VFIO_CCW_STATE_IDLE;
}
+static void vfio_ccw_sch_chp_todo(struct work_struct *work)
+{
+ struct vfio_ccw_private *private =
+ container_of(work, struct vfio_ccw_private, chp_work);
+
+ if (private->chp_trigger)
+ eventfd_signal(private->chp_trigger, 1);
+}
+
/*
* Css driver callbacks
*/
@@ -130,6 +140,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
goto out_disable;
INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
+ INIT_WORK(&private->chp_work, vfio_ccw_sch_chp_todo);
atomic_set(&private->avail, 1);
private->state = VFIO_CCW_STATE_STANDBY;
@@ -202,6 +213,45 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int
process)
return 0;
}
+static int vfio_ccw_sch_chp_event(struct subchannel *sch,
+ struct chp_link *link, int event)
+{
+ struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
+ int mask = chp_ssd_get_mask(&sch->ssd_info, link);
+
+ /* move these checks around? */
+ if (!private || !mask)
+ return 0;
+
+ if (cio_update_schib(sch))
+ return -ENODEV;
+
+ switch (event) {
+ case CHP_VARY_OFF:
+ sch->opm &= ~mask;
+ sch->lpm &= ~mask;
+ /* TODO: terminate current I/O on path. */
+ break;
+ case CHP_VARY_ON:
+ sch->opm |= mask;
+ sch->lpm |= mask;
+ break;
+ case CHP_OFFLINE:
+ /* TODO: terminate current I/O on path. */
+ break;
+ case CHP_ONLINE:
+ sch->lpm |= mask & sch->opm;
+ break;
+ default:
+ /* Not possible? */
+ return 0;
+ }
+
+ vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_UPDATE_CHP);
+
+ return 0;
+}
+
static struct css_device_id vfio_ccw_sch_ids[] = {
{ .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, },
{ /* end of list */ },
@@ -219,6 +269,7 @@ static struct css_driver vfio_ccw_sch_driver = {
.remove = vfio_ccw_sch_remove,
.shutdown = vfio_ccw_sch_shutdown,
.sch_event = vfio_ccw_sch_event,
+ .chp_event = vfio_ccw_sch_chp_event,
};
static int __init vfio_ccw_sch_init(void)
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index be081ccabea3..c400021134cb 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -188,6 +188,23 @@ static void fsm_update_subch(struct vfio_ccw_private
*private,
sizeof(sch->schib));
}
+static void fsm_update_chp(struct vfio_ccw_private *private,
+ enum vfio_ccw_event event)
+{
+ queue_work(vfio_ccw_work_q, &private->chp_work);
+
+}
+
+static void fsm_update_chp_busy(struct vfio_ccw_private *private,
+ enum vfio_ccw_event event)
+{
+ /*
+* TODO:
+* If we are having I/O on the current path, do
+* extra handling?
+*/
+}
+
/*
* Device statemachine
*/
@@ -197,29 +214,34 @@ fsm_func_t
*vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
[VFIO_CCW_EVENT_IO_REQ] = fsm_io_error,
[VFIO_CCW_EVENT_INTERRUPT] = fsm_disabled_irq,
[VFIO_CCW_EVENT_UPDATE_SUBCH] = fsm_update_subch,
+ [VFIO_CCW_EVENT_UPDATE_CHP] = fsm_nop,
},
[VFIO_CCW_STATE_STANDBY] = {
[VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
[VFIO_CCW_EVENT_IO_REQ] = fsm_io_error,
[VFIO_CCW_EVENT_INTERRUPT] = fsm_irq,
[VFIO_CCW_EVENT_UPDATE_SUBCH] = fsm_update_subch,
+ [VFIO_CCW_EVENT_UPDATE_CHP] = fsm_update_chp,
},
[VFIO_CCW_STATE_IDLE] = {
[VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
[VFIO_CCW_EVENT_IO_REQ] = fsm_io_request,
[VFIO_CCW_EVENT_INTERRUPT]
[Qemu-devel] [RFC PATCH 3/5] vfio/ccw: get irq info and set up handler for chp irq
vfio-ccw now resorts to the eventfd mechanism to communicate
with userland for channel path related event. To get notification
of the channel path event, userland basically needs to:
1. check the chp irq capability via issuing VFIO_DEVICE_GET_IRQ_INFO
ioctl with VFIO_CCW_CHP_IRQ_INDEX.
2. register an event notifier to get an eventfd fd.
3. register the eventfd for chp irq to kernel via
VFIO_DEVICE_SET_IRQS ioctl.
This work introduces vfio_ccw_chp_notifier_handler(), and refactors
vfio_ccw_(un)register_event_notifier() to do the above.
Based on this, future work could read channel path information
out once got the eventfd signal, and do further process.
Signed-off-by: Dong Jia Shi
---
hw/vfio/ccw.c | 92 +--
1 file changed, 70 insertions(+), 22 deletions(-)
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index e673695509..d812cecfe0 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -36,6 +36,8 @@ typedef struct VFIOCCWDevice {
uint64_t schib_region_size;
uint64_t schib_region_offset;
struct ccw_schib_region *schib_region;
+
+EventNotifier chp_notifier;
} VFIOCCWDevice;
static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
@@ -172,33 +174,56 @@ read_err:
css_inject_io_interrupt(sch);
}
-static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
+static void vfio_ccw_chp_notifier_handler(void *opaque)
+{
+VFIOCCWDevice *vcdev = opaque;
+
+if (!event_notifier_test_and_clear(&vcdev->chp_notifier)) {
+return;
+}
+
+/* TODO: further process on path informaion. */
+}
+
+static void vfio_ccw_register_event_notifier(VFIOCCWDevice *vcdev, int irq,
+ Error **errp)
{
VFIODevice *vdev = &vcdev->vdev;
struct vfio_irq_info *irq_info;
struct vfio_irq_set *irq_set;
size_t argsz;
int32_t *pfd;
-
-if (vdev->num_irqs < VFIO_CCW_IO_IRQ_INDEX + 1) {
-error_setg(errp, "vfio: unexpected number of io irqs %u",
- vdev->num_irqs);
+EventNotifier *notifier;
+IOHandler *fd_read;
+
+switch (irq) {
+case VFIO_CCW_IO_IRQ_INDEX:
+notifier = &vcdev->io_notifier;
+fd_read = vfio_ccw_io_notifier_handler;
+break;
+case VFIO_CCW_CHP_IRQ_INDEX:
+notifier = &vcdev->chp_notifier;
+fd_read = vfio_ccw_chp_notifier_handler;
+break;
+default:
+error_setg(errp, "vfio: Unsupported device irq(%d) fd: %m", irq);
return;
}
argsz = sizeof(*irq_info);
irq_info = g_malloc0(argsz);
-irq_info->index = VFIO_CCW_IO_IRQ_INDEX;
+irq_info->index = irq;
irq_info->argsz = argsz;
if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
irq_info) < 0 || irq_info->count < 1) {
-error_setg_errno(errp, errno, "vfio: Error getting irq info");
+error_setg_errno(errp, errno, "vfio: Error getting irq(%d) info", irq);
goto out_free_info;
}
-if (event_notifier_init(&vcdev->io_notifier, 0)) {
+if (event_notifier_init(notifier, 0)) {
error_setg_errno(errp, errno,
- "vfio: Unable to init event notifier for IO");
+ "vfio: Unable to init event notifier for irq(%d)",
+ irq);
goto out_free_info;
}
@@ -207,17 +232,18 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice
*vcdev, Error **errp)
irq_set->argsz = argsz;
irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
VFIO_IRQ_SET_ACTION_TRIGGER;
-irq_set->index = VFIO_CCW_IO_IRQ_INDEX;
+irq_set->index = irq;
irq_set->start = 0;
irq_set->count = 1;
pfd = (int32_t *) &irq_set->data;
-*pfd = event_notifier_get_fd(&vcdev->io_notifier);
-qemu_set_fd_handler(*pfd, vfio_ccw_io_notifier_handler, NULL, vcdev);
+*pfd = event_notifier_get_fd(notifier);
+qemu_set_fd_handler(*pfd, fd_read, NULL, vcdev);
if (ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
-error_setg(errp, "vfio: Failed to set up io notification");
+error_setg(errp, "vfio: Failed to set up notification for irq(%d)",
+ irq);
qemu_set_fd_handler(*pfd, NULL, NULL, vcdev);
-event_notifier_cleanup(&vcdev->io_notifier);
+event_notifier_cleanup(notifier);
}
g_free(irq_set);
@@ -226,30 +252,42 @@ out_free_info:
g_free(irq_info);
}
-static void vfio_ccw_unregister_io_notifier(VFIOCCWDevice *vcdev)
+static void vfio_ccw_unregister_event_notifier(VFIOCCWDevice *vcdev, int irq)
{
struct vfio_irq_set *irq_set;
size_t argsz;
int32_t *pfd;
+EventNotifier *notifier;
+
+switch (irq) {
+case VFIO_CCW_IO_IRQ_INDEX:
+notifier = &am
[Qemu-devel] [RFC PATCH 1/5] vfio: linux-headers update for vfio-ccw
This is a placeholder for a linux-headers update.
Signed-off-by: Dong Jia Shi
---
linux-headers/linux/vfio.h | 2 ++
linux-headers/linux/vfio_ccw.h | 6 ++
2 files changed, 8 insertions(+)
diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index 4312e961ff..24d85be022 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -457,11 +457,13 @@ enum {
enum {
VFIO_CCW_CONFIG_REGION_INDEX,
+ VFIO_CCW_SCHIB_REGION_INDEX,
VFIO_CCW_NUM_REGIONS
};
enum {
VFIO_CCW_IO_IRQ_INDEX,
+ VFIO_CCW_CHP_IRQ_INDEX,
VFIO_CCW_NUM_IRQS
};
diff --git a/linux-headers/linux/vfio_ccw.h b/linux-headers/linux/vfio_ccw.h
index 5bf96c3812..4944dd946e 100644
--- a/linux-headers/linux/vfio_ccw.h
+++ b/linux-headers/linux/vfio_ccw.h
@@ -22,4 +22,10 @@ struct ccw_io_region {
__u32 ret_code;
} __attribute__((packed));
+struct ccw_schib_region {
+__u8 cc;
+#define SCHIB_AREA_SIZE 52
+__u8 schib_area[SCHIB_AREA_SIZE];
+} __attribute__((packed));
+
#endif
--
2.13.5
[Qemu-devel] [RFC PATCH 2/3] vfio: ccw: introduce channel path irq
This introduces a new irq for vfio-ccw to provide channel path
related event for userland.
Signed-off-by: Dong Jia Shi
---
drivers/s390/cio/vfio_ccw_ops.c | 29 +
drivers/s390/cio/vfio_ccw_private.h | 2 ++
include/uapi/linux/vfio.h | 1 +
3 files changed, 24 insertions(+), 8 deletions(-)
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 9528fce2e7d9..972cfc8834fc 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -275,17 +275,20 @@ static int vfio_ccw_mdev_get_region_info(struct
vfio_region_info *info,
static int vfio_ccw_mdev_get_irq_info(struct vfio_irq_info *info)
{
- if (info->index != VFIO_CCW_IO_IRQ_INDEX)
+ switch (info->index) {
+ case VFIO_CCW_IO_IRQ_INDEX:
+ case VFIO_CCW_CHP_IRQ_INDEX:
+ info->count = 1;
+ info->flags = VFIO_IRQ_INFO_EVENTFD;
+ return 0;
+ default:
return -EINVAL;
-
- info->count = 1;
- info->flags = VFIO_IRQ_INFO_EVENTFD;
-
- return 0;
+ }
}
static int vfio_ccw_mdev_set_irqs(struct mdev_device *mdev,
uint32_t flags,
+ unsigned index,
void __user *data)
{
struct vfio_ccw_private *private;
@@ -295,7 +298,17 @@ static int vfio_ccw_mdev_set_irqs(struct mdev_device *mdev,
return -EINVAL;
private = dev_get_drvdata(mdev_parent_dev(mdev));
- ctx = &private->io_trigger;
+
+ switch (index) {
+ case VFIO_CCW_IO_IRQ_INDEX:
+ ctx = &private->io_trigger;
+ break;
+ case VFIO_CCW_CHP_IRQ_INDEX:
+ ctx = &private->chp_trigger;
+ break;
+ default:
+ return -EINVAL;
+ }
switch (flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
case VFIO_IRQ_SET_DATA_NONE:
@@ -433,7 +446,7 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,
return ret;
data = (void __user *)(arg + minsz);
- return vfio_ccw_mdev_set_irqs(mdev, hdr.flags, data);
+ return vfio_ccw_mdev_set_irqs(mdev, hdr.flags, hdr.index, data);
}
case VFIO_DEVICE_RESET:
return vfio_ccw_mdev_reset(mdev);
diff --git a/drivers/s390/cio/vfio_ccw_private.h
b/drivers/s390/cio/vfio_ccw_private.h
index 460c8b90d834..da86f82dd7b9 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -33,6 +33,7 @@
* @irb: irb info received from interrupt
* @scsw: scsw info
* @io_trigger: eventfd ctx for signaling userspace I/O results
+ * @chp_trigger: eventfd ctx for signaling userspace chp event
* @io_work: work for deferral process of I/O handling
*/
struct vfio_ccw_private {
@@ -50,6 +51,7 @@ struct vfio_ccw_private {
union scsw scsw;
struct eventfd_ctx *io_trigger;
+ struct eventfd_ctx *chp_trigger;
struct work_struct io_work;
} __aligned(8);
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index c60244debf71..179a7492d53a 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -463,6 +463,7 @@ enum {
enum {
VFIO_CCW_IO_IRQ_INDEX,
+ VFIO_CCW_CHP_IRQ_INDEX,
VFIO_CCW_NUM_IRQS
};
--
2.13.5
[Qemu-devel] [RFC PATCH 0/3] vfio: ccw: basic channel path event handling
--- 0.0.3f3f 0.0.0002 3390/0c 3990/e9 f0 f0 ff 42434445 #Notice: No change! [root@localhost ~]# chccwdev -e 3f3f Setting device 0.0.3f3f online dasd-eckd 0.0.3f3f: A channel path to the device has become operational dasd-eckd 0.0.3f3f: New DASD 3390/0C (CU 3990/01) with 30051 cylinders, 15 heads, 224 sectors dasd-eckd 0.0.3f3f: DASD with 4 KB/block, 21636720 KB total size, 48 KB/track, compatible disk layout dasda:VOL1/ 0X3F3F: dasda1 Done [root@guest ~]# lscss 0002 Device Subchan. DevType CU Type Use PIM PAM POM CHPIDs -- 0.0.3f3f 0.0.0002 3390/0c 3990/e9 f0 70 ff 42434445 #Notice: PAM value of path 0.42 changed. 5. On the host, configure on one path. [root@host ~]# chchp -c 1 42 6. On the guest, check the status again: [root@guest ~]# lscss 0002 Device Subchan. DevType CU Type Use PIM PAM POM CHPIDs -- 0.0.3f3f 0.0.0002 3390/0c 3990/e9 f0 70 ff 42434445 #Notice: No change! [root@localhost ~]# chccwdev -d 3f3f Setting device 0.0.3f3f offline Done [root@guest ~]# lscss 0002 Device Subchan. DevType CU Type Use PIM PAM POM CHPIDs -- 0.0.3f3f 0.0.0002 3390/0c 3990/e9 f0 f0 ff 42434445 #Notice: PAM changed again. Dong Jia Shi (3): vfio: ccw: introduce schib region vfio: ccw: introduce channel path irq vfio: ccw: handle chp event drivers/s390/cio/vfio_ccw_drv.c | 51 + drivers/s390/cio/vfio_ccw_fsm.c | 43 ++ drivers/s390/cio/vfio_ccw_ops.c | 108 ++-- drivers/s390/cio/vfio_ccw_private.h | 8 +++ include/uapi/linux/vfio.h | 2 + include/uapi/linux/vfio_ccw.h | 6 ++ 6 files changed, 190 insertions(+), 28 deletions(-) -- 2.13.5
[Qemu-devel] [RFC PATCH 1/3] vfio: ccw: introduce schib region
This introduces a new region for vfio-ccw to provide subchannel
information for user space.
Signed-off-by: Dong Jia Shi
---
drivers/s390/cio/vfio_ccw_fsm.c | 21 ++
drivers/s390/cio/vfio_ccw_ops.c | 79 +++--
drivers/s390/cio/vfio_ccw_private.h | 3 ++
include/uapi/linux/vfio.h | 1 +
include/uapi/linux/vfio_ccw.h | 6 +++
5 files changed, 90 insertions(+), 20 deletions(-)
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index c30420c517b1..be081ccabea3 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -172,6 +172,22 @@ static void fsm_irq(struct vfio_ccw_private *private,
complete(private->completion);
}
+static void fsm_update_subch(struct vfio_ccw_private *private,
+enum vfio_ccw_event event)
+{
+ struct subchannel *sch;
+
+ sch = private->sch;
+ if (cio_update_schib(sch)) {
+ private->schib_region.cc = 3;
+ return;
+ }
+
+ private->schib_region.cc = 0;
+ memcpy(private->schib_region.schib_area, &sch->schib,
+ sizeof(sch->schib));
+}
+
/*
* Device statemachine
*/
@@ -180,25 +196,30 @@ fsm_func_t
*vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
[VFIO_CCW_EVENT_NOT_OPER] = fsm_nop,
[VFIO_CCW_EVENT_IO_REQ] = fsm_io_error,
[VFIO_CCW_EVENT_INTERRUPT] = fsm_disabled_irq,
+ [VFIO_CCW_EVENT_UPDATE_SUBCH] = fsm_update_subch,
},
[VFIO_CCW_STATE_STANDBY] = {
[VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
[VFIO_CCW_EVENT_IO_REQ] = fsm_io_error,
[VFIO_CCW_EVENT_INTERRUPT] = fsm_irq,
+ [VFIO_CCW_EVENT_UPDATE_SUBCH] = fsm_update_subch,
},
[VFIO_CCW_STATE_IDLE] = {
[VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
[VFIO_CCW_EVENT_IO_REQ] = fsm_io_request,
[VFIO_CCW_EVENT_INTERRUPT] = fsm_irq,
+ [VFIO_CCW_EVENT_UPDATE_SUBCH] = fsm_update_subch,
},
[VFIO_CCW_STATE_BOXED] = {
[VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
[VFIO_CCW_EVENT_IO_REQ] = fsm_io_busy,
[VFIO_CCW_EVENT_INTERRUPT] = fsm_irq,
+ [VFIO_CCW_EVENT_UPDATE_SUBCH] = fsm_update_subch,
},
[VFIO_CCW_STATE_BUSY] = {
[VFIO_CCW_EVENT_NOT_OPER] = fsm_notoper,
[VFIO_CCW_EVENT_IO_REQ] = fsm_io_busy,
[VFIO_CCW_EVENT_INTERRUPT] = fsm_irq,
+ [VFIO_CCW_EVENT_UPDATE_SUBCH] = fsm_update_subch,
},
};
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 41eeb57d68a3..9528fce2e7d9 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -13,6 +13,11 @@
#include "vfio_ccw_private.h"
+#define VFIO_CCW_OFFSET_SHIFT 40
+#define VFIO_CCW_OFFSET_TO_INDEX(off) (off >> VFIO_CCW_OFFSET_SHIFT)
+#define VFIO_CCW_INDEX_TO_OFFSET(index)((u64)(index) <<
VFIO_CCW_OFFSET_SHIFT)
+#define VFIO_CCW_OFFSET_MASK (((u64)(1) << VFIO_CCW_OFFSET_SHIFT) - 1)
+
static int vfio_ccw_mdev_reset(struct mdev_device *mdev)
{
struct vfio_ccw_private *private;
@@ -168,14 +173,31 @@ static ssize_t vfio_ccw_mdev_read(struct mdev_device
*mdev,
loff_t *ppos)
{
struct vfio_ccw_private *private;
- struct ccw_io_region *region;
+ void *region;
+ u32 index;
+ loff_t pos;
+
+ index = VFIO_CCW_OFFSET_TO_INDEX(*ppos);
+ pos = (*ppos) & VFIO_CCW_OFFSET_MASK;
+ private = dev_get_drvdata(mdev_parent_dev(mdev));
- if (*ppos + count > sizeof(*region))
+ switch (index) {
+ case VFIO_CCW_CONFIG_REGION_INDEX:
+ if (pos + count > sizeof(struct ccw_io_region))
+ return -EINVAL;
+ region = &private->io_region;
+ break;
+ case VFIO_CCW_SCHIB_REGION_INDEX:
+ if (pos + count > sizeof(struct ccw_schib_region))
+ return -EINVAL;
+ region = &private->schib_region;
+ vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_UPDATE_SUBCH);
+ break;
+ default:
return -EINVAL;
+ }
- private = dev_get_drvdata(mdev_parent_dev(mdev));
- region = &private->io_region;
- if (copy_to_user(buf, (void *)region + *ppos, count))
+ if (copy_to_user(buf, region + pos, count))
return -EFAULT;
return count;
@@ -187,23 +209,35 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device
*mdev,
loff_t *
Re: [Qemu-devel] [PATCH 3/3] s390x: deprecate s390-squash-mcss machine prop
* Dong Jia Shi [2017-12-05 15:43:00 +0800]: > * Cornelia Huck [2017-12-04 17:11:24 +0100]: > > [...] > > This one looks good to me too, so: > Reviewed-by: Dong Jia Shi > > > > > (...) > > > > > > Looks sane. We should put a note into the 2.12 changelog as well. > > > > > > > > > > I agree. Who would be responsible for updating the changelog. I'm not > > > familiar with that process yet. To be honest, I wouldn't mind having > > > the changelog notice in your writing style. Guess would be better for > > > everyone ;). > > > > Just a one-liner once we have the 2.12 changelog page. > > > > > > > > There are also other things we identified as TODOs: > > > * Updating https://wiki.qemu.org/Features/Channel_I/O_Passthrough > > > (@Dong Jia, could you take this one) > Ok. Will do soon. > > > > * In tree and/or on wiki documentation which is up-to-date and > > > more verbose than commit messages are supposed to be (and Dong > > > Jia's write-up could be incorporated to). I see this one as > > > lower prio though. Any volunteers? > > > > Long-ish things with links etc. should probably go into the wiki. > > > > Anyway, feel free to go ahead (it's a wiki :) We can always change > > things later on. > > > Let me update the wiki based on my understanding (and style :) firstly, > then you can login and improve it as you wish. The following wiki is just updated. Please feel free to make additional correction: https://wiki.qemu.org/Features/Channel_I/O_Passthrough BTW, Conny mentioned before that we should also update this one: https://wiki.qemu.org/Features/LegacyRemoval Should it be done now? @Halil, if you would like to do the update operation, I will leave it to you. If you want to delegate, I can do it. -- Dong Jia Shi
Re: [Qemu-devel] [PATCH 3/3] s390x: deprecate s390-squash-mcss machine prop
* Dong Jia Shi [2017-12-05 15:43:00 +0800]: > * Cornelia Huck [2017-12-04 17:11:24 +0100]: > > [...] > > This one looks good to me too, so: > Reviewed-by: Dong Jia Shi > BTW, since we are deprecating s390-squash-mcss, I think any more improment on it (e.g. more accurate and helpful hint message) is not worthy. So I will not send such pathes in future, if nobody disagrees. [...] -- Dong Jia Shi
Re: [Qemu-devel] [PATCH 3/3] s390x: deprecate s390-squash-mcss machine prop
* Cornelia Huck [2017-12-04 17:11:24 +0100]: [...] This one looks good to me too, so: Reviewed-by: Dong Jia Shi > > (...) > > > > Looks sane. We should put a note into the 2.12 changelog as well. > > > > > > > I agree. Who would be responsible for updating the changelog. I'm not > > familiar with that process yet. To be honest, I wouldn't mind having > > the changelog notice in your writing style. Guess would be better for > > everyone ;). > > Just a one-liner once we have the 2.12 changelog page. > > > > > There are also other things we identified as TODOs: > > * Updating https://wiki.qemu.org/Features/Channel_I/O_Passthrough > > (@Dong Jia, could you take this one) Ok. Will do soon. > > * In tree and/or on wiki documentation which is up-to-date and > > more verbose than commit messages are supposed to be (and Dong > > Jia's write-up could be incorporated to). I see this one as > > lower prio though. Any volunteers? > > Long-ish things with links etc. should probably go into the wiki. > > Anyway, feel free to go ahead (it's a wiki :) We can always change > things later on. > Let me update the wiki based on my understanding (and style :) firstly, then you can login and improve it as you wish. -- Dong Jia Shi
Re: [Qemu-devel] [PATCH 2/3] s390x/css: advertise unrestricted cssids
* Halil Pasic [2017-12-04 16:07:27 +0100]:
>
>
> On 12/04/2017 12:15 PM, Cornelia Huck wrote:
> > On Fri, 1 Dec 2017 15:31:35 +0100
> > Halil Pasic wrote:
> >
> >> Let us advertise the changes introduced by "s390x/css: unrestrict cssids"
> >> to the management software (so it can tell are cssids unrestricted or
> >> restricted).
> >>
> >> Signed-off-by: Halil Pasic
> >> ---
> >>
> >> Boris says having the property on the virtual-css-bridge is good form
> >> Libvirt PoV. @Shalini: could you verify that things work out fine
> >> (provided we get at least a preliminary blessing from Connie).
> >>
> >> Consider squashing into "s390x/css: unrestrict cssids".
> >> ---
> >> hw/s390x/css-bridge.c | 11 +++
> >> 1 file changed, 11 insertions(+)
> >>
> >> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
> >> index c4a9735d71..c7e8998680 100644
> >> --- a/hw/s390x/css-bridge.c
> >> +++ b/hw/s390x/css-bridge.c
> >> @@ -123,6 +123,11 @@ static Property virtual_css_bridge_properties[] = {
> >> DEFINE_PROP_END_OF_LIST(),
> >> };
> >>
> >> +static bool prop_get_true(Object *obj, Error **errp)
> >> +{
> >> +return true;
> >> +}
> >> +
> >> static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
> >> {
> >> HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> >> @@ -131,6 +136,12 @@ static void virtual_css_bridge_class_init(ObjectClass
> >> *klass, void *data)
> >> hc->unplug = ccw_device_unplug;
> >> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> >> dc->props = virtual_css_bridge_properties;
> >> +object_class_property_add_bool(klass, "cssid-unrestricted",
> >> + prop_get_true, NULL, NULL);
> >> +object_class_property_set_description(klass, "cssid-unrestricted",
> >> +"A css device can use any cssid, regardless whether virtual"
> >
> > extra space -^
> >
>
> Nod.
>
> >> +" or not (read only, always true)",
>
> Do we need "." here ^ ?
>
> >> +NULL);
> >> }
> >>
> >> static const TypeInfo virtual_css_bridge_info = {
> >
> > Looks reasonable. If this works as expected, I'll squash it into the
> > previous patch.
> >
>
> I've just asked Shalini to verify the libvirt perspective.
>
> Supposed we verify this works as expected, I read I don't have to spin
> a v2 and you are going to fix the issues found yourself. Right?
Supposed this works as expected, you have my r-b. ;)
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH 1/3] s390x/css: unrestrict cssids
* Halil Pasic [2017-12-01 15:31:34 +0100]:
[...]
No comment for the message part.
The code looks good to me. So after squashing with patch #2:
Reviewed-by: Dong Jia Shi
> ---
> hw/s390x/3270-ccw.c| 2 +-
> hw/s390x/css.c | 28
> hw/s390x/s390-ccw.c| 2 +-
> hw/s390x/s390-virtio-ccw.c | 1 -
> hw/s390x/virtio-ccw.c | 2 +-
> include/hw/s390x/css.h | 12
> 6 files changed, 11 insertions(+), 36 deletions(-)
>
> diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c
> index 081e3ef6f4..3af13ea027 100644
> --- a/hw/s390x/3270-ccw.c
> +++ b/hw/s390x/3270-ccw.c
> @@ -104,7 +104,7 @@ static void emulated_ccw_3270_realize(DeviceState *ds,
> Error **errp)
> SubchDev *sch;
> Error *err = NULL;
>
> -sch = css_create_sch(cdev->devno, true, cbus->squash_mcss, errp);
> +sch = css_create_sch(cdev->devno, cbus->squash_mcss, errp);
> if (!sch) {
> return;
> }
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index f6b5c807cd..cd26f32050 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -2370,22 +2370,12 @@ const PropertyInfo css_devid_ro_propinfo = {
> .get = get_css_devid,
> };
>
> -SubchDev *css_create_sch(CssDevId bus_id, bool is_virtual, bool squash_mcss,
> - Error **errp)
> +SubchDev *css_create_sch(CssDevId bus_id, bool squash_mcss, Error **errp)
> {
> uint16_t schid = 0;
> SubchDev *sch;
>
> if (bus_id.valid) {
> -if (is_virtual != (bus_id.cssid == VIRTUAL_CSSID)) {
> -error_setg(errp, "cssid %hhx not valid for %s devices",
> - bus_id.cssid,
> - (is_virtual ? "virtual" : "non-virtual"));
> -return NULL;
> -}
> -}
> -
> -if (bus_id.valid) {
> if (squash_mcss) {
> bus_id.cssid = channel_subsys.default_cssid;
> } else if (!channel_subsys.css[bus_id.cssid]) {
> @@ -2396,19 +2386,8 @@ SubchDev *css_create_sch(CssDevId bus_id, bool
> is_virtual, bool squash_mcss,
> bus_id.devid, &schid, errp)) {
> return NULL;
> }
> -} else if (squash_mcss || is_virtual) {
> -bus_id.cssid = channel_subsys.default_cssid;
> -
> -if (!css_find_free_subch_and_devno(bus_id.cssid, &bus_id.ssid,
> - &bus_id.devid, &schid, errp)) {
> -return NULL;
> -}
> } else {
> -for (bus_id.cssid = 0; bus_id.cssid < MAX_CSSID; ++bus_id.cssid) {
> -if (bus_id.cssid == VIRTUAL_CSSID) {
> -continue;
> -}
> -
> +for (bus_id.cssid = channel_subsys.default_cssid;;) {
> if (!channel_subsys.css[bus_id.cssid]) {
> css_create_css_image(bus_id.cssid, false);
> }
> @@ -2418,7 +2397,8 @@ SubchDev *css_create_sch(CssDevId bus_id, bool
> is_virtual, bool squash_mcss,
> NULL)) {
> break;
> }
> -if (bus_id.cssid == MAX_CSSID) {
> +bus_id.cssid = (bus_id.cssid + 1) % MAX_CSSID;
> +if (bus_id.cssid == channel_subsys.default_cssid) {
> error_setg(errp, "Virtual channel subsystem is full!");
> return NULL;
> }
> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
> index 0ef232ec27..4a9d4d2534 100644
> --- a/hw/s390x/s390-ccw.c
> +++ b/hw/s390x/s390-ccw.c
> @@ -77,7 +77,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char
> *sysfsdev, Error **errp)
> goto out_err_propagate;
> }
>
> -sch = css_create_sch(ccw_dev->devno, false, cbus->squash_mcss, &err);
> +sch = css_create_sch(ccw_dev->devno, cbus->squash_mcss, &err);
> if (!sch) {
> goto out_mdevid_free;
> }
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 6a57f94197..4d65a50334 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -302,7 +302,6 @@ static void ccw_init(MachineState *machine)
> /*
> * Non mcss-e enabled guests only see the devices from the default
> * css, which is determined by the value of the squash_mcss property.
> - * Note: we must not squash non virtual devices to css 0xFE.
> */
> if (css_bus->squash_mcss) {
> ret = css_create_css_image(0, true);
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 184515ce9
Re: [Qemu-devel] [RFC PATCH v2 1/1] s390x/css: unrestrict cssids
* Cornelia Huck [2017-11-29 12:47:47 +0100]: [...] > > With this patch > > === > > > > +---+- > > | squashing off | squashing on > > +---+- > > auto id |F | F > > +---+- > > explicit id |S' | S > > +---+- > > > > T5. squashing off + auto id > > qemu-system-s390x: Unknown savevm section or instance > > '/fe.0.0003/virtio-rng' 0 > > qemu-system-s390x: load of migration failed: Invalid argument > > [Fail due to busid mismatch.] > > > > T6. squashing off + explicit given id > > qemu-system-s390x: vmstate: get_nullptr expected VMS_NULLPTR_MARKER > > qemu-system-s390x: Failed to load s390_css:css > > qemu-system-s390x: error while loading state for instance 0x0 of device > > 's390_css' > > qemu-system-s390x: load of migration failed: Invalid argument > > [Setting vfio-ccw.devno=non-fe.x.. (same as T1) > > Fail due to css mismatch - there is no css 0 in the new vm.] > > > > Succeed. > > [Setting vfio-ccw.devno=fe.x..] > > Don't you need to attach the vfio-ccw device later anyway? You have to > detach it from the source before you migrate, and I'd expect it to be > symmetric. Yes. After migrate, there is no problem to device_add the vfio-ccw device (id=vfio0,devno=fe.x.). The result (succeed for this case) does not change. > > > > > T7. squashing on + auto id > > qemu-system-s390x: Unknown savevm section or instance > > '/00.0.0003/virtio-rng' 0 > > qemu-system-s390x: load of migration failed: Invalid argument > > [Fail due to busid mismatch.] > > > > T8. squashing on + explicit given id > > Succeed. > > > > > > Notice: > > The differences of the test results between w and w/o this patch are in > > the "squashing off" cases. I think these are things that we can accept. > > Yes, I think that makes sense. If you want reliable migration, you need > to be specific with your ids. I'd just don't want us to break things > explicitly. > Fair enough. Got the message. -- Dong Jia Shi
Re: [Qemu-devel] [RFC PATCH v2 1/1] s390x/css: unrestrict cssids
* Halil Pasic [2017-11-29 17:30:15 +0100]:
>
>
> On 11/29/2017 12:47 PM, Cornelia Huck wrote:
> > On Wed, 29 Nov 2017 16:17:35 +0800
> > Dong Jia Shi wrote:
> >
> >> * Halil Pasic [2017-11-28 14:07:58 +0100]:
> >>
> >> [...]
> >>> The auto-generated bus ids are affected by both changes. We hope to not
> >>> encounter any auto-generated bus ids in production as Libvirt is always
> >>> explicit about the bus id. Since 8ed179c937 ("s390x/css: catch section
> >>> mismatch on load", 2017-05-18) the worst that can happen because the same
> >>> device ended up having a different bus id is a cleanly failed migration.
> >>> I find it hard to reason about the impact of changed auto-generated bus
> >>> ids on migration for command line users as I don't know which rules is
> >>> such an user supposed to follow.
> >> For this paragraph, Halil pointed to me a case that he is thinking of.
> >> 1. VM configuration with 3 devices:
> >> -device virtio (e.g. virtio-blk-ccw,id=disk0)
> >> -device vfio-ccw (e.g. id=vfio0)
> >> -device virtio (e.g. virtio-rng-ccw,id=rng0)
> >> 2. Start the vm.
> >> 3. device_del vfio0
> >> 4. migrate "exec:gzip -c > /tmp/tmp_vmstate.gz"
> >> 5. modify cmd line from step 1 by removing the vfio0 device, and adding:
> >>-incoming "exec:gzip -c -d /tmp/tmp_vmstate.gz"
> >>
> >> Let me list my test results here for everybody's reference.
> >>
> >> W/o this patch
> >> ==
> >>
> >> +---+-
> >> | squashing off | squashing on
> >> +---+-
> >> auto id |F | F
> >> +---+-
> >> explicit id |F | S
> >> +---+-
> >>
> >> T1. squashing off + auto id
> >> qemu-system-s390x: vmstate: get_nullptr expected VMS_NULLPTR_MARKER
> >> qemu-system-s390x: Failed to load s390_css:css
> >> qemu-system-s390x: error while loading state for instance 0x0 of device
> >> 's390_css'
> >> qemu-system-s390x: load of migration failed: Invalid argument
> >> [Fail due to css mismatch - there is no css 0 in the new vm.]
> >>
> >> T2. squashing off + explicit given id
> >> qemu-system-s390x: vmstate: get_nullptr expected VMS_NULLPTR_MARKER
> >> qemu-system-s390x: Failed to load s390_css:css
> >> qemu-system-s390x: error while loading state for instance 0x0 of device
> >> 's390_css'
> >> qemu-system-s390x: load of migration failed: Invalid argument
> >> [Fail due to css mismatch - there is no css 0 in the new vm.]
> > Hmm... so should we even try to migrate an empty css 0? It only exists
> > because we have created a device that we had to detach anyway because
> > it was non-migrateable...
> >
> > [Probably no easy way to deal with this, though.]
> >
>
> We could make the thing go away when the last device is gone.
Is it possible to free the empty css in a .pre_save handler somewhere?
> I see a general problem with implicitly generated shared stuff.
>
> Obviously we can't fix the past.
Nod.
>
> @Dong Jia:
>
> Thanks for doing the experiments and publishing your findings.
>
Just want to ease the review. No need mention. :)
--
Dong Jia Shi
Re: [Qemu-devel] [RFC PATCH v2 1/1] s390x/css: unrestrict cssids
* Dong Jia Shi [2017-11-29 16:17:35 +0800]: [...] > T6. squashing off + explicit given id > qemu-system-s390x: vmstate: get_nullptr expected VMS_NULLPTR_MARKER > qemu-system-s390x: Failed to load s390_css:css > qemu-system-s390x: error while loading state for instance 0x0 of device > 's390_css' > qemu-system-s390x: load of migration failed: Invalid argument > [Setting vfio-ccw.devno=non-fe.x.. (same as T1) s/T1/T2/ > Fail due to css mismatch - there is no css 0 in the new vm.] > > Succeed. > [Setting vfio-ccw.devno=fe.x..] > [...] -- Dong Jia Shi
Re: [Qemu-devel] [RFC PATCH v2 1/1] s390x/css: unrestrict cssids
* Halil Pasic [2017-11-28 14:07:58 +0100]:
[...]
>
> The auto-generated bus ids are affected by both changes. We hope to not
> encounter any auto-generated bus ids in production as Libvirt is always
> explicit about the bus id. Since 8ed179c937 ("s390x/css: catch section
> mismatch on load", 2017-05-18) the worst that can happen because the same
> device ended up having a different bus id is a cleanly failed migration.
> I find it hard to reason about the impact of changed auto-generated bus
> ids on migration for command line users as I don't know which rules is
> such an user supposed to follow.
For this paragraph, Halil pointed to me a case that he is thinking of.
1. VM configuration with 3 devices:
-device virtio (e.g. virtio-blk-ccw,id=disk0)
-device vfio-ccw (e.g. id=vfio0)
-device virtio (e.g. virtio-rng-ccw,id=rng0)
2. Start the vm.
3. device_del vfio0
4. migrate "exec:gzip -c > /tmp/tmp_vmstate.gz"
5. modify cmd line from step 1 by removing the vfio0 device, and adding:
-incoming "exec:gzip -c -d /tmp/tmp_vmstate.gz"
Let me list my test results here for everybody's reference.
W/o this patch
==
+---+-
| squashing off | squashing on
+---+-
auto id |F | F
+---+-
explicit id |F | S
+---+-
T1. squashing off + auto id
qemu-system-s390x: vmstate: get_nullptr expected VMS_NULLPTR_MARKER
qemu-system-s390x: Failed to load s390_css:css
qemu-system-s390x: error while loading state for instance 0x0 of device
's390_css'
qemu-system-s390x: load of migration failed: Invalid argument
[Fail due to css mismatch - there is no css 0 in the new vm.]
T2. squashing off + explicit given id
qemu-system-s390x: vmstate: get_nullptr expected VMS_NULLPTR_MARKER
qemu-system-s390x: Failed to load s390_css:css
qemu-system-s390x: error while loading state for instance 0x0 of device
's390_css'
qemu-system-s390x: load of migration failed: Invalid argument
[Fail due to css mismatch - there is no css 0 in the new vm.]
T3. squashing on + auto id
qemu-system-s390x: Unknown savevm section or instance '/00.0.0003/virtio-rng' 0
qemu-system-s390x: load of migration failed: Invalid argument
[Fail due to busid mismatch.]
T4. squashing on + explicit given id
Succeed.
With this patch
===
+---+-
| squashing off | squashing on
+---+-
auto id |F | F
+---+-
explicit id |S' | S
+---+-
T5. squashing off + auto id
qemu-system-s390x: Unknown savevm section or instance '/fe.0.0003/virtio-rng' 0
qemu-system-s390x: load of migration failed: Invalid argument
[Fail due to busid mismatch.]
T6. squashing off + explicit given id
qemu-system-s390x: vmstate: get_nullptr expected VMS_NULLPTR_MARKER
qemu-system-s390x: Failed to load s390_css:css
qemu-system-s390x: error while loading state for instance 0x0 of device
's390_css'
qemu-system-s390x: load of migration failed: Invalid argument
[Setting vfio-ccw.devno=non-fe.x.. (same as T1)
Fail due to css mismatch - there is no css 0 in the new vm.]
Succeed.
[Setting vfio-ccw.devno=fe.x..]
T7. squashing on + auto id
qemu-system-s390x: Unknown savevm section or instance '/00.0.0003/virtio-rng' 0
qemu-system-s390x: load of migration failed: Invalid argument
[Fail due to busid mismatch.]
T8. squashing on + explicit given id
Succeed.
Notice:
The differences of the test results between w and w/o this patch are in
the "squashing off" cases. I think these are things that we can accept.
[...]
--
Dong Jia Shi
Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids
* Cornelia Huck [2017-11-27 13:58:16 +0100]: > On Mon, 27 Nov 2017 10:20:56 +0800 > Dong Jia Shi wrote: > > > * Halil Pasic [2017-11-24 17:39:04 +0100]: > > > > > > > > > > > On 11/24/2017 05:15 PM, Cornelia Huck wrote: > > > >>> In theory this should work. > > > >>> > > > >>> In reality it seems more complicated. A per-device property is easy > > > >>> and can be > > > >>> inspected on the command line (e.g. -device virtio-blk-ccw,help), > > > >>> while a new > > > >>> machine property would require to change the qemu help output and > > > >>> qemu-options > > > >>> file (which makes it visible for all architectures). > > > >> And then we have the fun of describing, that this property is weird, > > > >> and can > > > >> not be set, and it's value does not matter. > > > > Well, that's the case for both, no? > > > > > > > > > I don't think we have to document _device_ properites in qemu-options.hx > > > I don't see any documented neither for virtio-ccw nor for vfio-ccw. The > > > machine properties, on the contrary, are documented in this file. > > Is it sane and possible to reuse the existing s390-squash-mcss property > > to achieve the goal? I mean, when it is false (which is the default > > value), can we treat it as "we are allowed to put devices everywhere"? > > Then we'd have the way to use a property of the -M to tell libvirt that > > devices can be everywhere? > > > > However then we can not drop it completely I guess, since Libvirt will > > depends on it. But we can ignore the operation of setting it's value to > > true. > > I don't think we should reuse it, as it would have rather confusing > semantics (which can't be easily sorted out unless you check for the > qemu version). > Right. This is something that I missed. So please ignore my noise. -- Dong Jia Shi
Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids
* Cornelia Huck [2017-11-27 17:56:07 +0100]: > On Mon, 27 Nov 2017 16:09:09 +0100 > Boris Fiuczynski wrote: > > > On 11/27/2017 03:13 PM, Halil Pasic wrote: > > > > > > > > > On 11/27/2017 02:19 PM, Cornelia Huck wrote: > > >> On Mon, 27 Nov 2017 14:11:57 +0100 > > >> Halil Pasic wrote: > > >> > > >>> On 11/27/2017 01:56 PM, Cornelia Huck wrote: > > >>>> On Fri, 24 Nov 2017 17:39:04 +0100 > > >>>> Halil Pasic wrote: > > >>>> > > >>>>> On 11/24/2017 05:15 PM, Cornelia Huck wrote: > > >> > > >>>>>> (Unless we simply make this a "default cssid" prop after all - then > > >>>>>> it > > >>>>>> would be more than just a simple indication for libvirt...) > > >>>>>> > > >>>>> > > >>>>> We are now talking about the "cssid-unrestricted" property. The > > >>>>> default > > >>>>> cssid is not something I would like to do any time soon. > > >>>> > > >>>> What's so bad about this? As said above, I think it would be much more > > >>>> useful. If libvirt can detect r/o vs. r/w for properties, we can simply > > >>>> start out with a r/o variant now... > > >>>> > > >>> > > >>> I'm not sure I understand you. Are you proposing the following: > > >>> Drop the restriction, but don't indicate this via a read only > > >>> "cssid-unrestricted" device property but via a "default-css" > > >>> read only machine property. > > >>> > > >>> Libvirt then should know that if "default-css" is present then > > >>> we don't have this virtual into 0xfe and non virtual into 0xfd > > >>> restriction any more. > > >> > > >> Yes. > > >> > > > > > > @Boris: > > > Does this work for you? Technically it's equivalent to having > > > "cssid-unrestricted" at machine level. > > > > > > I don't really like the idea of indicating unrestricted via > > > something mostly unrelated (at least for me it ain't obvious that > > > having the id of the default css exposed means we got rid of the > > > limitation.). But I'm tied of discussing this, so I'm willing to > > > compromise. > > > > > > Halil > > > > > I agree with Christian that we should not throw the "setting of a > > default cssid" together with "unrestricted cssid" and than use read/only > > and read/write to distinguish between the two! > > > > OK, let's step back and summarize a bit. > > Current situation: > - virtual devices must go into 0xfe, non-virtual devices must go into > non-0xfe > - 0xfe is the default cssid; non-mcss-e OSs see only devices in there > - to expose non-virtual devices to today's guests, you need to use the > squash-mcss parameter, which tries to put non-virtual devices into > the same place in css 0xfe When squashing, everything will be squashed to css 0 which is the deafult css in this case. [No effect to the conclusions below, but better to clarify.:-] > > This is problematic in various ways (potential for incomprehensible > errors, amongst others), so we agreed that the way to go is to drop the > virtual-vs-non-virtual cssid restrictions and allow any device in any > css image. > > Now, we need a way for libvirt to detect this, so that it knows that it > can put e.g. vfio-ccw devices in the same css image as virtio devices. > > Proposal 1: Use a device attribute to show that the device can be put > anywhere. This approach feels wrong to me (the non-restriction is not a > property of the individual device, but of the whole css resp. the > machine), so I would continue to veto this. > > Proposal 2: Export the default cssid as a machine property. If this > property exists, it also implies that devices can be put into any css > image (although it makes the most sense to put them into the default > css image as indicated by the property). Can be made r/w later if it is > too much for 2.12. > > Proposal 3: Add a machine property that indicates cssids are not > restricted. Later, optionally add a second property that exposes the > default cssid and makes it configurable. > > Personally, I prefer 2 (especially as this also allows to find out > where the best place to put devices for non-mcss-e enabled guests is), > but I could live with 3 as well (if making this r/w later would be > problematic for libvirt.) > > (In every case, we would deprecate and later remove the squash > parameter.) > > [As an aside, how hard is it to make the default cssid configurable? At > a glance, it does not seem too bad to me.] > -- Dong Jia Shi
Re: [Qemu-devel] [RFC PATCH 1/1] s390x/css: unresrict cssids
* Halil Pasic [2017-11-24 17:39:04 +0100]: > > > On 11/24/2017 05:15 PM, Cornelia Huck wrote: > >>> In theory this should work. > >>> > >>> In reality it seems more complicated. A per-device property is easy and > >>> can be > >>> inspected on the command line (e.g. -device virtio-blk-ccw,help), while a > >>> new > >>> machine property would require to change the qemu help output and > >>> qemu-options > >>> file (which makes it visible for all architectures). > >> And then we have the fun of describing, that this property is weird, and > >> can > >> not be set, and it's value does not matter. > > Well, that's the case for both, no? > > > I don't think we have to document _device_ properites in qemu-options.hx > I don't see any documented neither for virtio-ccw nor for vfio-ccw. The > machine properties, on the contrary, are documented in this file. Is it sane and possible to reuse the existing s390-squash-mcss property to achieve the goal? I mean, when it is false (which is the default value), can we treat it as "we are allowed to put devices everywhere"? Then we'd have the way to use a property of the -M to tell libvirt that devices can be everywhere? However then we can not drop it completely I guess, since Libvirt will depends on it. But we can ignore the operation of setting it's value to true. > > > > (Unless we simply make this a "default cssid" prop after all - then it > > would be more than just a simple indication for libvirt...) > > > > We are now talking about the "cssid-unrestricted" property. The default > cssid is not something I would like to do any time soon. -- Dong Jia Shi
Re: [Qemu-devel] [RFC PATCH v2 1/3] s390x/ccs: add ccw-testdev emulated device
* Halil Pasic [2017-11-08 17:54:20 +0100]: Hi Halil, > Add a fake device meant for testing the correctness of our css emulation. > > What we currently have is writing a Fibonacci sequence of uint32_t to the > device via ccw write. The write is going to fail if it ain't a Fibonacci > and indicate a device exception in scsw together with the proper residual > count. With this we can do basic verification of data integrity. > > Of course lot's of invalid inputs (besides basic data processing) can be > tested with that as well. > > We also have a no-op mode where the device just tells all-good! This > could be useful for fuzzing. > > Usage: > 1) fire up a qemu with something like -device ccw-testdev,devno=fe.0.0001 >on the command line > 2) exercise the device from the guest > > Signed-off-by: Halil Pasic > --- > > Introduction > > > While discussing v1 we (more or less) decided a test device for ccw is a > good idea. This is an RFC because there are some unresolved technical > questions I would like to discuss. > Regarding to test coverage, this mainly covers the cds_read. What we want would be surely more than this. So to extend the coverage, we need to design more modes (aka, test cases), right? If nobody disagrees with the basic idea of this series, I can start a review then. ;) [...] -- Dong Jia Shi
Re: [Qemu-devel] Questions about usability mess that caused by differentiating address based on devices types
* Cornelia Huck [2017-11-14 11:50:14 +0100]: Hallo Conny, After spending some time, just some updates for this one. > On Tue, 14 Nov 2017 16:25:47 +0800 > Dong Jia Shi wrote: > > > Dear Conny, > > > > Good day! > > > > Just now, our Libvirt folks pointed out a "usability mess" for the > > design of differentiating address based on devices classes (real | > > virtual). The complaints are mainly about the "s390-squash-mcss" > > property and restrictions to define virtual device in the special 0xFE > > css, and define real devices in non-0xFE. > > > > We have some discussions internally, but failed to get it cleared. As we > > think this is about the architecture, so hereby, I as a representative, > > forward our arguments and questions here to ask you for help: > > > > 1. What benifit do we get to put virtual devices in css 0xFE? > > Some background here (for the benefit of innocent bystanders): :) > > In the past, I had been involved with some cases where Linux guests > under z/VM died after a customer followed the recommended procedure to > vary off a path before applying service. That path was supposed to be a > path to a disk; unfortunately, z/VM had mapped all kinds of virtual > paths to it, including the only path to the console device. Oops. > > With that in mind, we wanted to make sure that qemu would not be > susceptible to the same problem; IOW, we wanted to make sure that > chpids etc. were not mashed together for devices that did not have > anything to do with each other. At one point in time, the idea came up > to use a reserved css for virtio devices, which was deemed an elegant > solution as 'real' devices were still something far in the future. (And > I was under the delusion that we would have MCSS-E support in Linux by > then; that has not happened...) > > So the basic idea of css 0xfe is: Maintain a clear separation between > devices emulated by qemu and pass-through devices (a more divisive > separation than by simply separating chpids). > Thanks for the information. I think now everybody are clear about the background. [I sometime found it is a pleasure to listen to your story. Clear and interesting.] > > > > 2. Since we could accept squashing virtual devices into css 0, can we > > accept to not trading 0xFE as a special css? > > Using css 0xfe seemed like a good idea; but as things worked out > differently in the meantime, it seems it causes more problems right now > than it avoids. > Have to agree. In particular after knowing the background. > > So that we can remove the restrictions for the cssid validation for each > > type of device. Even we could drop the s390-squash-mcss, and just allow > > the user to define any device in any css. > > Opening up the different csses for all devices might help, but we need > to be careful: > - We still want to keep the chpids separated. Probably not a problem > right now. > - We need to be able to point to a default css, especially as there are > no MCSS-E capable OSs around yet. > - You need to double check if there are further restrictions on the > allowed css ids. (I know that 0xff is reserved for special usage as > well; but I can't find out more.) > - Backwards compatibility and migration: We certainly don't want old > setups to break, and compat machines need to force the old scheme. > > All best tested out via a prototype :) > After a round of internal discussion, Halil now has a prototype. I think sooner he will post his patch with our internal agreement, and we can continuing talking based on that then. > > > > 3. If we have to keep the squash property, then when squashing, it's > > somewhat like "I don't care for the cssid", so is it possible for us to > > not check the cssid in the device devno? > > Libvirt would be benifited with this when automatically generating the > > addresses. > > I think we still need to keep the squashing around for compatibility, > but we may be able to give it the chop for something like 3.0. > > (And we probably need to keep the existing restrictions in compat mode.) > Can we just drop the squash property, right after we opened up all the csses? We do not support LGM for vfio-ccw, and there is no libvirt user until now. So what else case could it be to stop us from dropping it? > > > > 4. Error message for devno conflict is not helpful. For the following > > case: > > -M s390-ccw-virtio,s390-squash-mcss=on \ > > -drive > > file=/dev/disk/by-path/ccw-0.0.3f3e,if=none,id=drive-virtio-disk1,format=raw > > \ > > -device > > virtio-blk-
[Qemu-devel] Questions about usability mess that caused by differentiating address based on devices types
Dear Conny, Good day! Just now, our Libvirt folks pointed out a "usability mess" for the design of differentiating address based on devices classes (real | virtual). The complaints are mainly about the "s390-squash-mcss" property and restrictions to define virtual device in the special 0xFE css, and define real devices in non-0xFE. We have some discussions internally, but failed to get it cleared. As we think this is about the architecture, so hereby, I as a representative, forward our arguments and questions here to ask you for help: 1. What benifit do we get to put virtual devices in css 0xFE? 2. Since we could accept squashing virtual devices into css 0, can we accept to not trading 0xFE as a special css? So that we can remove the restrictions for the cssid validation for each type of device. Even we could drop the s390-squash-mcss, and just allow the user to define any device in any css. 3. If we have to keep the squash property, then when squashing, it's somewhat like "I don't care for the cssid", so is it possible for us to not check the cssid in the device devno? Libvirt would be benifited with this when automatically generating the addresses. 4. Error message for devno conflict is not helpful. For the following case: -M s390-ccw-virtio,s390-squash-mcss=on \ -drive file=/dev/disk/by-path/ccw-0.0.3f3e,if=none,id=drive-virtio-disk1,format=raw \ -device virtio-blk-ccw,devno=fe.0.,scsi=off,drive=drive-virtio-disk1,id=virtio-disk1,bootindex=0 \ -device vfio-ccw,devno=0.0.,sysfsdev=/sys/devices/css0/0.0.013f/6dfd3ec5-e8b3-4e18-a6fe-57bc9eceb920 \ We get this error message: qemu-system-s390x: -device vfio-ccw,devno=0.0.,sysfsdev=/sys/devices/css0/0.0.013f/6dfd3ec5-e8b3-4e18-a6fe-57bc9eceb920: Device 0.0. already exists By checking 0.0. from the cmd line, users can not find out the root cause - squashing, easily. So, if we have to keep the squash property, we could improve this message by adding a hint. To sum up, we got the feeling that, this mess is not only for Libvirt but also for QEMU cmd line users. And we are wondering if there is some way to improve it. Thanks, -- Dong Jia Shi
Re: [Qemu-devel] [PATCH v3 7/7] s390x: refactor error handling for MSCH handler
* Halil Pasic [2017-10-18 13:01:03 +0200]:
>
>
> On 10/18/2017 12:02 PM, Cornelia Huck wrote:
> > On Wed, 18 Oct 2017 12:00:05 +0200
> > Thomas Huth wrote:
> >
> >> On 17.10.2017 16:04, Halil Pasic wrote:
> >>> Simplify the error handling of the MSCH. Let the code detecting the
> >>> condition tell (in a less ambiguous way) how it's to be handled. No
> >>> changes in behavior.
> >>
> >> ok, so you claim no changes in behavior ...
> >>
> >>> Signed-off-by: Halil Pasic
> >>> ---
> >>> hw/s390x/css.c | 18 +-
> >>> include/hw/s390x/css.h | 2 +-
> >>> target/s390x/ioinst.c | 23 ---
> >>> 3 files changed, 10 insertions(+), 33 deletions(-)
> >>>
> >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >>> index b9e0329825..30fc236946 100644
> >>> --- a/hw/s390x/css.c
> >>> +++ b/hw/s390x/css.c
> >>> @@ -1347,28 +1347,24 @@ static void copy_schib_from_guest(SCHIB *dest,
> >>> const SCHIB *src)
> >>> }
> >>> }
> >>>
> >>> -int css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
> >>> +IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *orig_schib)
> >>> {
> >>> SCSW *s = &sch->curr_status.scsw;
> >>> PMCW *p = &sch->curr_status.pmcw;
> >>> uint16_t oldflags;
> >>> -int ret;
> >>> SCHIB schib;
> >>>
> >>> if (!(sch->curr_status.pmcw.flags & PMCW_FLAGS_MASK_DNV)) {
> >>> -ret = 0;
> >>> -goto out;
> >>> +return IOINST_CC_EXPECTED;
> >>> }
> >>>
> >>> if (s->ctrl & SCSW_STCTL_STATUS_PEND) {
> >>> -ret = -EINPROGRESS;
> >>> -goto out;
> >>> +return IOINST_CC_STATUS_PRESENT;
> >>> }
> >>>
> >>> if (s->ctrl &
> >>> (SCSW_FCTL_START_FUNC|SCSW_FCTL_HALT_FUNC|SCSW_FCTL_CLEAR_FUNC))
> >>> {
> >>> -ret = -EBUSY;
> >>> -goto out;
> >>> +return IOINST_CC_STATUS_PRESENT;
> >>> }
> >>
> >> ... but here you change -EBUSY (which got mapped to CC=2) to
> >> CC_STATUS_PRESENT which means CC=1. So that's a change in behavior. i.e.
> >> this is either a bug, or you should update the patch description with a
> >> justification for this change in behavior.
> >
> > Indeed, that's a bug. We still want cc 2.
> >
>
> Agree, it is a bug. It was OK in v1 but was already buggy in
> v2.
>
> Conny can you fix this up as well please?
>
> Thanks in advance!
>
I saw Conny fixed this in her branch. So:
Reviewed-by: Dong Jia Shi
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v3 6/7] s390x: refactor error handling for HSCH handler
* Halil Pasic [2017-10-17 16:04:52 +0200]:
> Simplify the error handling of the HSCH. Let the code detecting the
> condition tell (in a less ambiguous way) how it's to be handled. No
> changes in behavior.
>
> Signed-off-by: Halil Pasic
> ---
> hw/s390x/css.c | 18 +-
> include/hw/s390x/css.h | 2 +-
> target/s390x/ioinst.c | 23 ---
> 3 files changed, 10 insertions(+), 33 deletions(-)
>
[...]
> @@ -87,24 +85,11 @@ void ioinst_handle_hsch(S390CPU *cpu, uint64_t reg1)
> }
> trace_ioinst_sch_id("hsch", cssid, ssid, schid);
> sch = css_find_subch(m, cssid, ssid, schid);
> -if (sch && css_subch_visible(sch)) {
> -ret = css_do_hsch(sch);
> -}
> -switch (ret) {
> -case -ENODEV:
> -cc = 3;
> -break;
> -case -EBUSY:
> -cc = 2;
> -break;
> -case 0:
> -cc = 0;
> -break;
> -default:
> -cc = 1;
> -break;
> +if (!sch || !css_subch_visible(sch)) {
> +setcc(cpu, 3);
Ditto.
> +return;
> }
> -setcc(cpu, cc);
> +setcc(cpu, css_do_hsch(sch));
> }
>
> static int ioinst_schib_valid(SCHIB *schib)
> --
> 2.13.5
>
Reviewed-by: Dong Jia Shi
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v3 5/7] s390x: refactor error handling for CSCH handler
* Halil Pasic [2017-10-17 16:04:51 +0200]:
> Simplify the error handling of the cSCH. Let the code detecting the
> condition tell (in a less ambiguous way) how it's to be handled. No
> changes in behavior.
>
> Signed-off-by: Halil Pasic
> ---
[...]
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index 4ad07e9181..fd659e77a5 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -60,8 +60,6 @@ void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
> {
> int cssid, ssid, schid, m;
> SubchDev *sch;
> -int ret = -ENODEV;
> -int cc;
>
> if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
> program_interrupt(&cpu->env, PGM_OPERAND, 4);
> @@ -69,15 +67,11 @@ void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
> }
> trace_ioinst_sch_id("csch", cssid, ssid, schid);
> sch = css_find_subch(m, cssid, ssid, schid);
> -if (sch && css_subch_visible(sch)) {
> -ret = css_do_csch(sch);
> -}
> -if (ret == -ENODEV) {
> -cc = 3;
> -} else {
> -cc = 0;
> +if (!sch || !css_subch_visible(sch)) {
> +setcc(cpu, 3);
Same question here.
> +return;
> }
> -setcc(cpu, cc);
> +setcc(cpu, css_do_csch(sch));
> }
>
> void ioinst_handle_hsch(S390CPU *cpu, uint64_t reg1)
> --
> 2.13.5
>
If we agree to replace 3 with IOINST_CC_NOT_OPERATIONAL, maybe Conny
could fix it. Or we can do that in a following cleanup patch?
W/ or w/o the fix, for now:
Reviewed-by: Dong Jia Shi
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v3 4/7] s390x: refactor error handling for XSCH handler
* Halil Pasic [2017-10-17 16:04:50 +0200]:
> Simplify the error handling of the XSCH. Let the code detecting the
> condition tell (in a less ambiguous way) how it's to be handled. No
> changes in behavior.
>
> Signed-off-by: Halil Pasic
> ---
> hw/s390x/css.c | 17 +
> include/hw/s390x/css.h | 2 +-
> target/s390x/ioinst.c | 23 ---
> 3 files changed, 10 insertions(+), 32 deletions(-)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index ff5a05c34b..1b3be7729d 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1402,20 +1402,17 @@ out:
> return ret;
> }
>
> -int css_do_xsch(SubchDev *sch)
> +IOInstEnding css_do_xsch(SubchDev *sch)
> {
> SCSW *s = &sch->curr_status.scsw;
> PMCW *p = &sch->curr_status.pmcw;
> -int ret;
>
> if (~(p->flags) & (PMCW_FLAGS_MASK_DNV | PMCW_FLAGS_MASK_ENA)) {
> -ret = -ENODEV;
> -goto out;
> +return IOINST_CC_NOT_OPERATIONAL;
> }
>
> if (s->ctrl & SCSW_CTRL_MASK_STCTL) {
> -ret = -EINPROGRESS;
> -goto out;
> +return IOINST_CC_STATUS_PRESENT;
> }
>
> if (!(s->ctrl & SCSW_CTRL_MASK_FCTL) ||
> @@ -1423,8 +1420,7 @@ int css_do_xsch(SubchDev *sch)
> (!(s->ctrl &
> (SCSW_ACTL_RESUME_PEND | SCSW_ACTL_START_PEND | SCSW_ACTL_SUSP)))
> ||
> (s->ctrl & SCSW_ACTL_SUBCH_ACTIVE)) {
> -ret = -EBUSY;
> -goto out;
> +return IOINST_CC_BUSY;
> }
>
> /* Cancel the current operation. */
> @@ -1436,10 +1432,7 @@ int css_do_xsch(SubchDev *sch)
> sch->last_cmd_valid = false;
> s->dstat = 0;
> s->cstat = 0;
> -ret = 0;
> -
> -out:
> -return ret;
> +return IOINST_CC_EXPECTED;
> }
>
> int css_do_csch(SubchDev *sch)
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 10bde18452..3c319c9096 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -240,7 +240,7 @@ void css_conditional_io_interrupt(SubchDev *sch);
> int css_do_stsch(SubchDev *sch, SCHIB *schib);
> bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid);
> int css_do_msch(SubchDev *sch, const SCHIB *schib);
> -int css_do_xsch(SubchDev *sch);
> +IOInstEnding css_do_xsch(SubchDev *sch);
> int css_do_csch(SubchDev *sch);
> int css_do_hsch(SubchDev *sch);
> IOInstEnding css_do_ssch(SubchDev *sch, ORB *orb);
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index 16b5cf2fed..4ad07e9181 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -42,8 +42,6 @@ void ioinst_handle_xsch(S390CPU *cpu, uint64_t reg1)
> {
> int cssid, ssid, schid, m;
> SubchDev *sch;
> -int ret = -ENODEV;
> -int cc;
>
> if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
> program_interrupt(&cpu->env, PGM_OPERAND, 4);
> @@ -51,24 +49,11 @@ void ioinst_handle_xsch(S390CPU *cpu, uint64_t reg1)
> }
> trace_ioinst_sch_id("xsch", cssid, ssid, schid);
> sch = css_find_subch(m, cssid, ssid, schid);
> -if (sch && css_subch_visible(sch)) {
> -ret = css_do_xsch(sch);
> -}
> -switch (ret) {
> -case -ENODEV:
> -cc = 3;
> -break;
> -case -EBUSY:
> -cc = 2;
> -break;
> -case 0:
> - cc = 0;
> - break;
> -default:
> -cc = 1;
> -break;
> +if (!sch || !css_subch_visible(sch)) {
> +setcc(cpu, 3);
?:
s/3/IOINST_CC_NOT_OPERATIONAL/
This also applies to other alike cases.
> +return;
> }
> -setcc(cpu, cc);
> +setcc(cpu, css_do_xsch(sch));
> }
>
> void ioinst_handle_csch(S390CPU *cpu, uint64_t reg1)
> --
> 2.13.5
>
Reviewed-by: Dong Jia Shi
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v3 3/7] s390x: improve error handling for SSCH and RSCH
* Halil Pasic [2017-10-17 16:04:49 +0200]: > Simplify the error handling of the SSCH and RSCH handler avoiding > arbitrary and cryptic error codes being used to tell how the instruction > is supposed to end. Let the code detecting the condition tell how it's > to be handled in a less ambiguous way. It's best to handle SSCH and RSCH > in one go as the emulation of the two shares a lot of code. > > For passthrough this change isn't pure refactoring, but changes the way > kernel reported EFAULT is handled. After clarifying the kernel interface > we decided that EFAULT shall be mapped to unit exception. Same goes for > unexpected error codes and absence of required ORB flags. > > Signed-off-by: Halil Pasic > --- > hw/s390x/css.c | 84 > + > hw/s390x/s390-ccw.c | 11 +++--- > hw/vfio/ccw.c | 28 +++ > include/hw/s390x/css.h | 23 + > include/hw/s390x/s390-ccw.h | 2 +- > target/s390x/ioinst.c | 53 > 6 files changed, 75 insertions(+), 126 deletions(-) > Agree for what already planned to fix when applying. LGTM: Reviewed-by: Dong Jia Shi -- Dong Jia Shi
Re: [Qemu-devel] [PATCH v3 0/7] improve error handling for IO instr
* Cornelia Huck [2017-10-18 11:53:10 +0200]: > On Wed, 18 Oct 2017 16:23:47 +0800 > Dong Jia Shi wrote: > > > * Halil Pasic [2017-10-17 18:19:20 +0200]: > > > > [...] > > > > > >> Testing > > > >> === > > > >> > > > >> Nothing happened since v2 except for a quick smoke test. Dong Jia gave > > > >> v2 > > > >> a spin with a focus on vfio-ccw. @Dong Jia I would appreciate some > > > >> proper > > > >> testing, especially regarding the changes in vfio-ccw (patch #3). > > > > > > > > Looks sane to me (if needed, I can fix up the minor things I found). > > > > > > > > In addition to some testing, I'd appreciate some review from others as > > > > well. > > > > > > > > > > Of course, I'm fine with the fixes (won't answer individually). I think > > > both Dong Jia and Pierre have already put enough work in this to be > > > credited > > > with a tag, so I really hope they will get around to review this. I would > > > be especially happy with an Tested-by: Dong Jia since this series is quite > > > under-tested, and the changes in vfio-ccw aren't just minor. > > > > > > Of course I could come up with a test setup myself, but I hope Dong Jia > > > already has one, and he is certainly more involved with vfio-ccw. > > > > > Using Conny's s390-next branch + this series (#2-#7), I didn't notice > > any obvious problem during my fio test. So for the vfio-ccw related > > part: > > Tested-by: Dong Jia Shi > > Thanks! > > To which patches may I add the tag? :) > The test should cover the vfio-ccw related part in patch #3, I assume. So add it there? -- Dong Jia Shi
Re: [Qemu-devel] [PATCH v3 2/7] s390x/css: IO instr handler ending control
* Halil Pasic [2017-10-17 16:04:48 +0200]:
[...]
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 69b374730e..7e0dbd162f 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -99,6 +99,22 @@ typedef struct CcwDataStream {
> hwaddr cda;
> } CcwDataStream;
>
> +/*
> + * IO instructions conclude according this. Currently we have only
> + * cc codes. Valid values are 0,1,2,3 and the generic semantic for
> + * IO instructions is described briefly. For more details consult the PoP.
> + */
> +typedef enum IOInstEnding {
> +/* produced expected result */
> +IOINST_CC_EXPECTED = 0,
> +/* status conditions were present or produced alternate result */
> +IOINST_CC_STATUS_PRESENT = 1,
> +/* inst. ineffective because busy with previously initiated function */
> +IOINST_CC_BUSY = 2,
> +/* inst. ineffective because not operational */
> +IOINST_CC_NOT_OPERATIONAL = 3
> +} IOInstEnding;
> +
> typedef struct SubchDev SubchDev;
> struct SubchDev {
> /* channel-subsystem related things: */
> --
> 2.13.5
>
With what has been pointed out by Conny and Thomas addressed:
Reviewed-by: Dong Jia Shi
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v3 0/7] improve error handling for IO instr
* Halil Pasic [2017-10-17 18:19:20 +0200]: [...] > >> Testing > >> === > >> > >> Nothing happened since v2 except for a quick smoke test. Dong Jia gave v2 > >> a spin with a focus on vfio-ccw. @Dong Jia I would appreciate some proper > >> testing, especially regarding the changes in vfio-ccw (patch #3). > > > > Looks sane to me (if needed, I can fix up the minor things I found). > > > > In addition to some testing, I'd appreciate some review from others as > > well. > > > > Of course, I'm fine with the fixes (won't answer individually). I think > both Dong Jia and Pierre have already put enough work in this to be credited > with a tag, so I really hope they will get around to review this. I would > be especially happy with an Tested-by: Dong Jia since this series is quite > under-tested, and the changes in vfio-ccw aren't just minor. > > Of course I could come up with a test setup myself, but I hope Dong Jia > already has one, and he is certainly more involved with vfio-ccw. > Using Conny's s390-next branch + this series (#2-#7), I didn't notice any obvious problem during my fio test. So for the vfio-ccw related part: Tested-by: Dong Jia Shi [...] -- Dong Jia Shi
Re: [Qemu-devel] [PATCH v2 3/8] s390x: improve error handling for SSCH and RSCH
* Halil Pasic [2017-10-11 12:54:51 +0200]:
>
>
> On 10/11/2017 05:47 AM, Dong Jia Shi wrote:
> > * Halil Pasic [2017-10-04 17:41:39 +0200]:
> >
> >> Simplify the error handling of the SSCH and RSCH handler avoiding
> >> arbitrary and cryptic error codes being used to tell how the instruction
> >> is supposed to end. Let the code detecting the condition tell how it's
> >> to be handled in a less ambiguous way. It's best to handle SSCH and RSCH
> >> in one go as the emulation of the two shares a lot of code.
> >>
> >> For passthrough this change isn't pure refactoring, but changes the
> >> way kernel reported EFAULT is handled. After clarifying the kernel
> >> interface we decided that EFAULT shall be mapped to unit exception.
> >> Same goes for unexpected error codes.
> >>
> >> Signed-off-by: Halil Pasic
> >> ---
> >>
> >> AFAIR we decided in the previous round to rather do transformation
> >> and fixing in one patch than touch stuff twice. Hence this patch
> >> ain't pure transformation any more.
> >> ---
> >> hw/s390x/css.c | 83
> >> +
> >> hw/s390x/s390-ccw.c | 11 +++---
> >> hw/vfio/ccw.c | 30
> >> include/hw/s390x/css.h | 24 +
> >> include/hw/s390x/s390-ccw.h | 2 +-
> >> target/s390x/ioinst.c | 53 -
> >> 6 files changed, 77 insertions(+), 126 deletions(-)
> >>
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index 4f47dbc8b0..b2978c3bae 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -1003,12 +1003,11 @@ static void sch_handle_start_func_virtual(SubchDev
> >> *sch)
> >>
> >> }
> >>
> >> -static int sch_handle_start_func_passthrough(SubchDev *sch)
> >> +static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
> >> {
> >>
> >> PMCW *p = &sch->curr_status.pmcw;
> >> SCSW *s = &sch->curr_status.scsw;
> >> -int ret;
> >>
> >> ORB *orb = &sch->orb;
> >> if (!(s->ctrl & SCSW_ACTL_SUSP)) {
> >> @@ -1022,31 +1021,11 @@ static int
> >> sch_handle_start_func_passthrough(SubchDev *sch)
> >> */
> >> if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
> >> !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
> >> -return -EINVAL;
> >> +sch_gen_unit_exception(sch);
> >> +css_inject_io_interrupt(sch);
> >> +return (IOInstEnding){.cc = 0};
> > This behavior change is not mentioned in the commit message. Right?
> >
> No this particular change is not. Should I mention it explicitly?
I think so.
> Maybe "Same goes for unexpected error codes and absence of required
> ORB flags."
Sounds good for me.
[...]
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v2 3/8] s390x: improve error handling for SSCH and RSCH
* Halil Pasic [2017-10-10 12:06:23 +0200]:
>
>
> On 10/10/2017 10:13 AM, Dong Jia Shi wrote:
> > * Halil Pasic [2017-10-04 17:41:39 +0200]:
> >
> > [...]
> >
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index 4f47dbc8b0..b2978c3bae 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -1003,12 +1003,11 @@ static void sch_handle_start_func_virtual(SubchDev
> >> *sch)
> >>
> >> }
> >>
> >> -static int sch_handle_start_func_passthrough(SubchDev *sch)
> >> +static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
> >> {
> >>
> >> PMCW *p = &sch->curr_status.pmcw;
> >> SCSW *s = &sch->curr_status.scsw;
> >> -int ret;
> >>
> >> ORB *orb = &sch->orb;
> >> if (!(s->ctrl & SCSW_ACTL_SUSP)) {
> >> @@ -1022,31 +1021,11 @@ static int
> >> sch_handle_start_func_passthrough(SubchDev *sch)
> >> */
> >> if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
> >> !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
> >> -return -EINVAL;
> >> +sch_gen_unit_exception(sch);
> >> +css_inject_io_interrupt(sch);
> > Last cycle, we agreed to add some log here. Sth. like:
> > warn_report("vfio-ccw requires PFCH and C64 flags set...");
> >
> > I promised to do a fix for this piece of code. But since this patch
> > already fixed it, I guess what I have to do is to add the log only? Or
> > you would like to add it by yourself? ;)
> >
>
> I think I forgot this one. Should there be a v3 I could add this too.
> Otherwise I would not mind if you do it on top.
>
[...]
> >> @@ -1084,16 +1063,15 @@ int do_subchannel_work_passthrough(SubchDev *sch)
> >> /* TODO: Halt handling */
> >> sch_handle_halt_func(sch);
> >> } else if (s->ctrl & SCSW_FCTL_START_FUNC) {
> >> -ret = sch_handle_start_func_passthrough(sch);
> >> +return sch_handle_start_func_passthrough(sch);
> >> }
> >> -
> >> -return ret;
> >> +return (IOInstEnding){.cc = 0};
> >> }
> >>
> >> -static int do_subchannel_work(SubchDev *sch)
> >> +static IOInstEnding do_subchannel_work(SubchDev *sch)
> >> {
> >> if (!sch->do_subchannel_work) {
> >> -return -EINVAL;
> >> +return (IOInstEnding){.cc = 1};
> > This keeps the logic here as-is, so it is right.
> >
>
> Yep.
>
> > Anybody agrees that also adding an assert() here?
>
> With automated regression testing in place I'm for it, without
> I feel uncomfortable doing it myself. You could do this
> on top if you like.
Got it.
Marked. I will look back after this series.
[...]
>
> Except for the missing warning are you OK with the rest
> of the patch? I would like to re-claim your r-b (dropped
> because changes weren't just minor).
I replied to the patch thread - the main part looks good to me.
I will save my r-b for the next round. ;)
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v2 3/8] s390x: improve error handling for SSCH and RSCH
0x/css.h b/include/hw/s390x/css.h > index 66916b6546..2116c6b3c7 100644 > --- a/include/hw/s390x/css.h > +++ b/include/hw/s390x/css.h [...] > @@ -197,13 +208,14 @@ SubchDev *css_find_subch(uint8_t m, uint8_t cssid, > uint8_t ssid, > uint16_t schid); > bool css_subch_visible(SubchDev *sch); > void css_conditional_io_interrupt(SubchDev *sch); > + Extra change. > int css_do_stsch(SubchDev *sch, SCHIB *schib); > bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid); > int css_do_msch(SubchDev *sch, const SCHIB *schib); [...] -- Dong Jia Shi
Re: [Qemu-devel] [PATCH v2 3/8] s390x: improve error handling for SSCH and RSCH
* Halil Pasic [2017-10-04 17:41:39 +0200]:
[...]
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 4f47dbc8b0..b2978c3bae 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1003,12 +1003,11 @@ static void sch_handle_start_func_virtual(SubchDev
> *sch)
>
> }
>
> -static int sch_handle_start_func_passthrough(SubchDev *sch)
> +static IOInstEnding sch_handle_start_func_passthrough(SubchDev *sch)
> {
>
> PMCW *p = &sch->curr_status.pmcw;
> SCSW *s = &sch->curr_status.scsw;
> -int ret;
>
> ORB *orb = &sch->orb;
> if (!(s->ctrl & SCSW_ACTL_SUSP)) {
> @@ -1022,31 +1021,11 @@ static int sch_handle_start_func_passthrough(SubchDev
> *sch)
> */
> if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
> !(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
> -return -EINVAL;
> +sch_gen_unit_exception(sch);
> +css_inject_io_interrupt(sch);
Last cycle, we agreed to add some log here. Sth. like:
warn_report("vfio-ccw requires PFCH and C64 flags set...");
I promised to do a fix for this piece of code. But since this patch
already fixed it, I guess what I have to do is to add the log only? Or
you would like to add it by yourself? ;)
> +return (IOInstEnding){.cc = 0};
> }
> -
> -ret = s390_ccw_cmd_request(orb, s, sch->driver_data);
> -switch (ret) {
> -/* Currently we don't update control block and just return the cc code.
> */
> -case 0:
> -break;
> -case -EBUSY:
> -break;
> -case -ENODEV:
> -break;
> -case -EACCES:
> -/* Let's reflect an inaccessible host device by cc 3. */
> -ret = -ENODEV;
> -break;
> -default:
> - /*
> -* All other return codes will trigger a program check,
> -* or set cc to 1.
> -*/
> - break;
> -};
> -
> -return ret;
> +return s390_ccw_cmd_request(sch);
> }
>
> /*
[...]
> @@ -1084,16 +1063,15 @@ int do_subchannel_work_passthrough(SubchDev *sch)
> /* TODO: Halt handling */
> sch_handle_halt_func(sch);
> } else if (s->ctrl & SCSW_FCTL_START_FUNC) {
> -ret = sch_handle_start_func_passthrough(sch);
> +return sch_handle_start_func_passthrough(sch);
> }
> -
> -return ret;
> +return (IOInstEnding){.cc = 0};
> }
>
> -static int do_subchannel_work(SubchDev *sch)
> +static IOInstEnding do_subchannel_work(SubchDev *sch)
> {
> if (!sch->do_subchannel_work) {
> -return -EINVAL;
> +return (IOInstEnding){.cc = 1};
This keeps the logic here as-is, so it is right.
Anybody agrees that also adding an assert() here?
[...]
> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
> index 8614dda6f8..5d2c305b71 100644
> --- a/hw/s390x/s390-ccw.c
> +++ b/hw/s390x/s390-ccw.c
> @@ -18,15 +18,14 @@
> #include "hw/s390x/css-bridge.h"
> #include "hw/s390x/s390-ccw.h"
>
> -int s390_ccw_cmd_request(ORB *orb, SCSW *scsw, void *data)
> +IOInstEnding s390_ccw_cmd_request(SubchDev *sch)
> {
> -S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(data);
> +S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data);
>
> -if (cdc->handle_request) {
> -return cdc->handle_request(orb, scsw, data);
> -} else {
> -return -ENOSYS;
> +if (!cdc->handle_request) {
> +return (IOInstEnding){.cc = 1};
Same consideration as the last comment.
> }
> +return cdc->handle_request(sch);
> }
>
> static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
[...]
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v2 1/8] s390x/css: be more consistent if broken beyond repair
* Halil Pasic [2017-10-04 17:41:37 +0200]:
> Calling do_subchannel_work with no function control flags set in SCSW is
> a programming error. Currently the handle this differently in
?:
s/the/we/
> do_subchannel_work_virtual and do_subchannel_work_passthrough. Let's be
> consistent and guard with a common assert against this programming error.
>
> Signed-off-by: Halil Pasic
> ---
> hw/s390x/css.c | 16
> 1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index c59be1aad1..4f47dbc8b0 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1067,9 +1067,6 @@ int do_subchannel_work_virtual(SubchDev *sch)
> } else if (s->ctrl & SCSW_FCTL_START_FUNC) {
> /* Triggered by both ssch and rsch. */
> sch_handle_start_func_virtual(sch);
> -} else {
> -/* Cannot happen. */
> -return 0;
> }
> css_inject_io_interrupt(sch);
> return 0;
> @@ -1077,22 +1074,17 @@ int do_subchannel_work_virtual(SubchDev *sch)
>
> int do_subchannel_work_passthrough(SubchDev *sch)
> {
> -int ret;
> +int ret = 0;
> SCSW *s = &sch->curr_status.scsw;
>
> if (s->ctrl & SCSW_FCTL_CLEAR_FUNC) {
> /* TODO: Clear handling */
> sch_handle_clear_func(sch);
> -ret = 0;
> } else if (s->ctrl & SCSW_FCTL_HALT_FUNC) {
> /* TODO: Halt handling */
> sch_handle_halt_func(sch);
> -ret = 0;
A bit surprise to see these. I'm fine with these changes though.
> } else if (s->ctrl & SCSW_FCTL_START_FUNC) {
> ret = sch_handle_start_func_passthrough(sch);
> -} else {
> -/* Cannot happen. */
> -return -ENODEV;
> }
>
> return ret;
> @@ -1100,11 +1092,11 @@ int do_subchannel_work_passthrough(SubchDev *sch)
>
> static int do_subchannel_work(SubchDev *sch)
> {
> -if (sch->do_subchannel_work) {
> -return sch->do_subchannel_work(sch);
> -} else {
> +if (!sch->do_subchannel_work) {
> return -EINVAL;
> }
> +g_assert(sch->curr_status.scsw.ctrl & SCSW_CTRL_MASK_FCTL);
> +return sch->do_subchannel_work(sch);
> }
>
> static void copy_pmcw_to_guest(PMCW *dest, const PMCW *src)
With the fix of the message:
Reviewed-by: Dong Jia Shi
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
* Dong Jia Shi [2017-09-26 15:48:56 +0800]:
[...]
> > >
> > > Tried to test with the following method:
> > > 1. Start g1 (first level guest on kvm a host) with a virtio blk device
> > >defined:
> > > -drive
> > > file=/dev/disk/by-path/ccw-0.0.3f3e,if=none,id=drive-virtio-disk1,format=raw
> > > \
> > > -device
> > > virtio-blk-ccw,devno=fe.0.,scsi=off,drive=drive-virtio-disk1,id=virtio-disk1
> > > \
> > > 2. Login g1, and bind the subchannel of ccw device 0.0. with
> > >vfio-ccw drvier.
> > > 3. Create a mdev on the above subchannel.
> > > 4. Passthrough the mdev to g2, and try to start g2.
> > >
> > > The 4th step failed with the following message and hang:
> > > qemu-system-s390x: vfio-ccw: wirte I/O region: errno=4
> > > (BTW, 4 is EINTR.)
> > >
> > > I roughly guess this might be caused by:
> > > On the kvm host, virtio callback injects the I/O interrupt in a
> > > synchronzing manner. And this causes g1's I/O interrupt handler getting
> > > the interrupt and then signaling the Qemu instance on g1 with the I/O
> > > result, even before return of the pwrite().
> > >
> > > But, using gdb on the kvm host, I do see several ssch successfully
> > > executed. I will dig the root reason, and see if there is some way to
> > > fix the issue.
> >
> > Hm... would that be the ccws used for setting up a virtio device, and
> > the problems start once adapter interrupts become active?
> After a debugging, when starting g2, I got the following ccw sequence:
> 1. CCW_CMD_SENSE_ID 0xe4 [OK]
> 2. CCW_CMD_NOOP 0x03 [OK]
> 3. CCW_CMD_SET_VIRTIO_REV 0x83 [OK]
> 4. CCW_CMD_VDEV_RESET 0x33 [FAILED]
>
> So this is still in the phase of setting up the device.
>
> > Does it work if you modify the nested guest to use the old
> > per-subchannel indicators mechanism?
> It turns out the root reason for the pwrite failure is caused by a bug
> in the vfio-ccw driver:
> drivers/s390/cio/vfio_ccw_cp.c: ccwchain_fetch_direct()
> calls pfn_array_alloc_pin() with a zero @len parameter.
> So it results in a -EINVAL return.
>
> The current code assumes that a valid direct ccw always has its count
> value not equal to zero. However this is not true at least for the
> CCW_CMD_VDEV_RESET (0x33) command:
> (gdb) p/x ccw
> $5 = {cmd_code = 0x33, flags = 0x4, count = 0x0, cda = 0x0}
>
> With a temp fix on this problem, more ccws (e.g. 0x11, 0x12, 0x31, 0x72
> ...) could be translated and executed well. But finnaly the qemu process
> on g1 got a segmentation fault:
> User process fault: interruption code 0238 ilc:3 in
> libpthread-2.24.so[3ff84f8+1b000]
> Failing address: 000ce330b0b0 TEID: 000ce330b0b00800
> Fault in primary space mode while using user ASCE.
> AS:3b6cc1c7 R3:0024
> Segmentation fault
>
> dmesg on g1:
> [ 18.160413] User process fault: interruption code 0238 ilc:3 in
> libpthread-2.24.so[3ff84f8+1b000]
> [ 18.160462] Failing address: 000ce330b0b0 TEID: 000ce330b0b00800
> [ 18.160463] Fault in primary space mode while using user ASCE.
> [ 18.160470] AS:3b6cc1c7 R3:0024
> [ 18.160476] CPU: 1 PID: 2095 Comm: qemu-system-s39 Not tainted
> 4.13.0-01250-g6baa298-dirty #58
> [ 18.160477] Hardware name: IBM 2964 NC9 704 (KVM/Linux)
> [ 18.160479] task: 38ac8000 task.stack: 38e4c000
> [ 18.160480] User PSW : 070520018000 03ff84f93b8a
> [ 18.160483]R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:1 AS:0 CC:2 PM:0
> RI:0 EA:3
> [ 18.160486] User GPRS: 0001 03ff0003 000104be86b0
> 000104be86c6
> [ 18.160487] 00010001 0001049efb22
> 03ffc5dfe13f
> [ 18.160489]03ff643fee60 03ffc5dfe258
> 03ff643fe8c8
> [ 18.160490]03ff855a5000 0001049cc320 03ff643fe888
> 03ff643fe7e8
> [ 18.160503] User Code: 03ff84f93b7a: c0e5e7cbbrasl
> %r14,3ff84f90b10
> 03ff84f93b80: a7f4ffc4brc
> 15,3ff84f93b08
> #03ff84f93b84: e560ff0ctbegin 0,65292
> >03ff84f93b8a: b2220050ipm >%r5
> 03ff84f93b8e: 8850001csrl %r5,28
> 03ff84f93b92: a774001cbrc
> 7,3ff84f93bca
> 03ff84f93b96: e3002012lt %r0,0(%r2)
>
Re: [Qemu-devel] [PATCH 4/9] s390x: refactor error handling for SSCH and RSCH
* Halil Pasic [2017-09-25 12:57:31 +0200]: [restored Cc:] > > > On 09/25/2017 09:31 AM, Dong Jia Shi wrote: > > * Cornelia Huck [2017-09-08 11:59:50 +0200]: > > > >> On Fri, 8 Sep 2017 11:21:57 +0200 > >> Halil Pasic wrote: > >> > >>> On 09/08/2017 05:41 AM, Dong Jia Shi wrote: > >>>> Let' me summarize here, in case I misunderstand things. Now we have > >>>> two ways to choose: > >>>> > >>>> A. Kernel: no change. > >>>>Qemu : handle -EFAULT as option 2 by generating a program check. > >>>> > >>>> B. Kernel: return -EFAULT > >>>>+ > >>>>update the IRB area in the I/O region for option 1 to present > >>>>a unit check SCSW (with proper sense byte ECW), and for option > >>>>2 to present a program check. > >>>>Qemu : handle -EFAULT according to the information that the IRB area > >>>>provided. > >>> > >>> This is not what I was trying to say. You got my message regarding A, but > >>> B was supposed to be understood like this. > >>> > >>> Keep the current handling for option 1, that is return -EFAULT. For option > >>> 2 do what the spec says, execute the program until the bad address and > >>> then > >>> generate a program-check (SCSW) once the bad stuff has it's turn. Thus > >>> the only change in QEMU would be handling -EFAULT with an unit check > >>> (because > >>> now it's just option 1). > > Let me adding some context information here by copying some words from the > > previous mail in this thread: > > The only option 2 case in the kernel is ccwchain_fetch_idal() finding a > > bad idaw_iova. > > > > What you propose to do for this case is (correct me if I get it wrong): > > In ccwchain_fetch_idal(), we do not return -EFAULT, instead we return 0, > > and issuing the incompletely translated channel program with the bad > > address to the physical device. And QEMU will eventually get the SCSW > > with the program-check from the physical device I/O result, and inject > > it to guest for further handling. > > > > I guess that would be the cleanest. I would also be fine with not making > the physical device program-check (issuing a shortened channel program, > and doing the program check in software) but that's probably more > complicated to implement. That's far more complicated. I will try the simple approach. > > Is this understanding right? If so, I'm fine with that, and I can > > provide the fix in the kernel. > > > > That would be nice. Ok. -- Dong Jia Shi
Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
* Cornelia Huck [2017-09-21 10:54:02 +0200]:
> On Thu, 21 Sep 2017 16:45:47 +0800
> Dong Jia Shi wrote:
>
> > * Cornelia Huck [2017-09-07 10:08:17 +0200]:
> >
> > [...]
> >
> > > > I'm thinking of a method these days:
> > > > Could passing through an fully emulated ccw device (e.g. 3270), or a
> > > > virtio ccw device, in the level 1 kvm guest to a level 2 guest be a test
> > > > method for this?
> > > >
> > > > All of the CCWs will be translated to IDAL CCWs by vfio-ccw in the level
> > > > 1 guest (which is the level 2 kvm host) and issued to the level 1 kvm
> > > > host. So, those IDALs will eventually be handled by the emulated device,
> > > > or the virtio ccw device, on the level 1 kvm host...
> > > >
> > > > Some days ago, one of my colleague tried the emulated 3270 passing
> > > > through. She stucked with the problem that the level 1 kvm host handling
> > > > a senseid IDAL ccw as a Direct ccw.
> > > >
> > > > Maybe I could try to pass through a virtio ccw device. I don't think of
> > > > any obvious problem that would lead to fail. Any comment?
> > > >
> > >
> > > That actually looks like a good thing to try! Cool idea.
> > >
> >
> > Tried to test with the following method:
> > 1. Start g1 (first level guest on kvm a host) with a virtio blk device
> >defined:
> > -drive
> > file=/dev/disk/by-path/ccw-0.0.3f3e,if=none,id=drive-virtio-disk1,format=raw
> > \
> > -device
> > virtio-blk-ccw,devno=fe.0.,scsi=off,drive=drive-virtio-disk1,id=virtio-disk1
> > \
> > 2. Login g1, and bind the subchannel of ccw device 0.0. with
> >vfio-ccw drvier.
> > 3. Create a mdev on the above subchannel.
> > 4. Passthrough the mdev to g2, and try to start g2.
> >
> > The 4th step failed with the following message and hang:
> > qemu-system-s390x: vfio-ccw: wirte I/O region: errno=4
> > (BTW, 4 is EINTR.)
> >
> > I roughly guess this might be caused by:
> > On the kvm host, virtio callback injects the I/O interrupt in a
> > synchronzing manner. And this causes g1's I/O interrupt handler getting
> > the interrupt and then signaling the Qemu instance on g1 with the I/O
> > result, even before return of the pwrite().
> >
> > But, using gdb on the kvm host, I do see several ssch successfully
> > executed. I will dig the root reason, and see if there is some way to
> > fix the issue.
>
> Hm... would that be the ccws used for setting up a virtio device, and
> the problems start once adapter interrupts become active?
After a debugging, when starting g2, I got the following ccw sequence:
1. CCW_CMD_SENSE_ID 0xe4 [OK]
2. CCW_CMD_NOOP 0x03 [OK]
3. CCW_CMD_SET_VIRTIO_REV 0x83 [OK]
4. CCW_CMD_VDEV_RESET 0x33 [FAILED]
So this is still in the phase of setting up the device.
> Does it work if you modify the nested guest to use the old
> per-subchannel indicators mechanism?
It turns out the root reason for the pwrite failure is caused by a bug
in the vfio-ccw driver:
drivers/s390/cio/vfio_ccw_cp.c: ccwchain_fetch_direct()
calls pfn_array_alloc_pin() with a zero @len parameter.
So it results in a -EINVAL return.
The current code assumes that a valid direct ccw always has its count
value not equal to zero. However this is not true at least for the
CCW_CMD_VDEV_RESET (0x33) command:
(gdb) p/x ccw
$5 = {cmd_code = 0x33, flags = 0x4, count = 0x0, cda = 0x0}
With a temp fix on this problem, more ccws (e.g. 0x11, 0x12, 0x31, 0x72
...) could be translated and executed well. But finnaly the qemu process
on g1 got a segmentation fault:
User process fault: interruption code 0238 ilc:3 in
libpthread-2.24.so[3ff84f8+1b000]
Failing address: 000ce330b0b0 TEID: 000ce330b0b00800
Fault in primary space mode while using user ASCE.
AS:3b6cc1c7 R3:0024
Segmentation fault
dmesg on g1:
[ 18.160413] User process fault: interruption code 0238 ilc:3 in
libpthread-2.24.so[3ff84f8+1b000]
[ 18.160462] Failing address: 000ce330b0b0 TEID: 000ce330b0b00800
[ 18.160463] Fault in primary space mode while using user ASCE.
[ 18.160470] AS:3b6cc1c7 R3:0024
[ 18.160476] CPU: 1 PID: 2095 Comm: qemu-system-s39 Not tainted
4.13.0-01250-g6baa298-dirty #58
[ 18.160477] Hardware name: IBM 2964 NC9 704 (KVM/Linux)
[ 18.160479] task: 38ac8000 task.stack: 38e4c000
[ 18.160480] User PSW : 070520018000 03ff84f93b8a
[ 18.160483]R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:1 AS:0 CC:2 PM:0
RI:0 EA:3
[ 1
Re: [Qemu-devel] [PATCH 4/9] s390x: refactor error handling for SSCH and RSCH
* Cornelia Huck [2017-09-08 11:59:50 +0200]: > On Fri, 8 Sep 2017 11:21:57 +0200 > Halil Pasic wrote: > > > On 09/08/2017 05:41 AM, Dong Jia Shi wrote: > > > Let' me summarize here, in case I misunderstand things. Now we have > > > two ways to choose: > > > > > > A. Kernel: no change. > > >Qemu : handle -EFAULT as option 2 by generating a program check. > > > > > > B. Kernel: return -EFAULT > > >+ > > >update the IRB area in the I/O region for option 1 to present > > >a unit check SCSW (with proper sense byte ECW), and for option > > >2 to present a program check. > > >Qemu : handle -EFAULT according to the information that the IRB area > > >provided. > > > > This is not what I was trying to say. You got my message regarding A, but > > B was supposed to be understood like this. > > > > Keep the current handling for option 1, that is return -EFAULT. For option > > 2 do what the spec says, execute the program until the bad address and then > > generate a program-check (SCSW) once the bad stuff has it's turn. Thus > > the only change in QEMU would be handling -EFAULT with an unit check > > (because > > now it's just option 1). Let me adding some context information here by copying some words from the previous mail in this thread: The only option 2 case in the kernel is ccwchain_fetch_idal() finding a bad idaw_iova. What you propose to do for this case is (correct me if I get it wrong): In ccwchain_fetch_idal(), we do not return -EFAULT, instead we return 0, and issuing the incompletely translated channel program with the bad address to the physical device. And QEMU will eventually get the SCSW with the program-check from the physical device I/O result, and inject it to guest for further handling. Is this understanding right? If so, I'm fine with that, and I can provide the fix in the kernel. > > That makes sense to me. > -- Dong Jia Shi
Re: [Qemu-devel] [PATCH 4/9] s390x: refactor error handling for SSCH and RSCH
* Cornelia Huck [2017-09-08 12:02:38 +0200]: > On Fri, 8 Sep 2017 11:41:00 +0800 > Dong Jia Shi wrote: > > > I think the difficult part is in the Qemu side. For either A or B, in > > Qemu, we'd need to return a cc0 to indicate the channel program is > > accepted successfully by the device, and then update the virtual IRB and > > inject an I/O interrupt to notify the guest. > > > > The question is, now we need to map -EFAULT to cc0? This is too odd... > > It's not odd at all, if you consider these as two stages: > > - Initial acceptance of the command, set cc 0 to indicate you got it. > - Execution of the start function. This can then fail, of course. Ok. I got the idea! > > > And we need to find a proper place to do the interrupt injection. I > > guess it could be in the sch_handle_start_func_passthrough(). > > Probably, yes. > > > > > If we do not like such modification in the Qemu side, then A is not the > > way to go. > > > > And for B, we need to update the IRB area of the I/O region in the three > > cases listed in the former mail, and: > > 1. We need to set the ret_code as 0 for them, so that Qemu could map it > >to cc0. > > 2. We need to signal Qemu to fetch the IRB we generated with the checks. > > All of these are doable I think. > > > > Any comment for the above thoughts? > > > -- Dong Jia Shi
Re: [Qemu-devel] [PATCH v4 5/5] s390x/css: support ccw IDA
* Halil Pasic [2017-09-21 20:08:41 +0200]: > Let's add indirect data addressing support for our virtual channel > subsystem. This implementation does not bother with any kind of > prefetching. We simply step through the IDAL on demand. > > Signed-off-by: Halil Pasic > --- > hw/s390x/css.c | 114 > - > 1 file changed, 113 insertions(+), 1 deletion(-) > LGTM: Reviewed-by: Dong Jia Shi [...] -- Dong Jia Shi
Re: [Qemu-devel] [PATCH v4 4/5] 390x/css: introduce maximum data address checking
* Halil Pasic [2017-09-21 20:08:40 +0200]:
> The architecture mandates the addresses to be accessed on the first
> indirection level (that is, the data addresses without IDA, and the
> (M)IDAW addresses with (M)IDA) to be checked against an CCW format
> dependent limit maximum address. If a violation is detected, the storage
> access is not to be performed and a channel program check needs to be
> generated. As of today, we fail to do this check.
>
> Let us stick even closer to the architecture specification.
>
> Signed-off-by: Halil Pasic
> ---
> hw/s390x/css.c | 10 ++
> include/hw/s390x/css.h | 1 +
> 2 files changed, 11 insertions(+)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index e0d989829f..cd5580ebb8 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int
> len)
> return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
> }
>
> +static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1)
> +{
> +return (addr + len) < (ccw_fmt1 ? (1UL << 31) : (1UL << 24));
> +}
> +
> static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
>CcwDataStreamOp op)
> {
> @@ -804,6 +809,9 @@ static int ccw_dstream_rw_noflags(CcwDataStream *cds,
> void *buff, int len,
> if (ret <= 0) {
> return ret;
> }
> +if (!cds_ccw_addrs_ok(cds->cda, len, cds->flags & CDS_F_FMT)) {
> +return -EINVAL; /* channel program check */
> +}
> if (op == CDS_OP_A) {
> goto incr;
> }
> @@ -828,7 +836,9 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const
> *ccw, ORB const *orb)
> g_assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));
> cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
> (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
> + (orb->ctrl0 & ORB_CTRL0_MASK_FMT ? CDS_F_FMT : 0) |
> (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
> +
> cds->count = ccw->count;
> cds->cda_orig = ccw->cda;
> ccw_dstream_rewind(cds);
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 078356e94c..69b374730e 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -87,6 +87,7 @@ typedef struct CcwDataStream {
> #define CDS_F_MIDA 0x02
> #define CDS_F_I2K 0x04
> #define CDS_F_C64 0x08
> +#define CDS_F_FMT 0x10 /* CCW format-1 */
> #define CDS_F_STREAM_BROKEN 0x80
> uint8_t flags;
> uint8_t at_idaw;
> --
> 2.13.5
>
Reviewed-by: Dong Jia Shi
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
* Cornelia Huck [2017-09-07 10:08:17 +0200]: [...] > > I'm thinking of a method these days: > > Could passing through an fully emulated ccw device (e.g. 3270), or a > > virtio ccw device, in the level 1 kvm guest to a level 2 guest be a test > > method for this? > > > > All of the CCWs will be translated to IDAL CCWs by vfio-ccw in the level > > 1 guest (which is the level 2 kvm host) and issued to the level 1 kvm > > host. So, those IDALs will eventually be handled by the emulated device, > > or the virtio ccw device, on the level 1 kvm host... > > > > Some days ago, one of my colleague tried the emulated 3270 passing > > through. She stucked with the problem that the level 1 kvm host handling > > a senseid IDAL ccw as a Direct ccw. > > > > Maybe I could try to pass through a virtio ccw device. I don't think of > > any obvious problem that would lead to fail. Any comment? > > > > That actually looks like a good thing to try! Cool idea. > Tried to test with the following method: 1. Start g1 (first level guest on kvm a host) with a virtio blk device defined: -drive file=/dev/disk/by-path/ccw-0.0.3f3e,if=none,id=drive-virtio-disk1,format=raw \ -device virtio-blk-ccw,devno=fe.0.,scsi=off,drive=drive-virtio-disk1,id=virtio-disk1 \ 2. Login g1, and bind the subchannel of ccw device 0.0. with vfio-ccw drvier. 3. Create a mdev on the above subchannel. 4. Passthrough the mdev to g2, and try to start g2. The 4th step failed with the following message and hang: qemu-system-s390x: vfio-ccw: wirte I/O region: errno=4 (BTW, 4 is EINTR.) I roughly guess this might be caused by: On the kvm host, virtio callback injects the I/O interrupt in a synchronzing manner. And this causes g1's I/O interrupt handler getting the interrupt and then signaling the Qemu instance on g1 with the I/O result, even before return of the pwrite(). But, using gdb on the kvm host, I do see several ssch successfully executed. I will dig the root reason, and see if there is some way to fix the issue. -- Dong Jia Shi
Re: [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA
* Halil Pasic [2017-09-20 13:13:01 +0200]:
>
>
> On 09/20/2017 10:33 AM, Cornelia Huck wrote:
> > On Wed, 20 Sep 2017 15:42:38 +0800
> > Dong Jia Shi wrote:
> >
> >> * Halil Pasic [2017-09-19 20:27:45 +0200]:
> >>
> >>> Let's add indirect data addressing support for our virtual channel
> >>> subsystem. This implementation does not bother with any kind of
> >>> prefetching. We simply step through the IDAL on demand.
> >>>
> >>> Signed-off-by: Halil Pasic
> >>> Signed-off-by: Cornelia Huck
> >>> ---
> >>> hw/s390x/css.c | 117
> >>> -
> >>> 1 file changed, 116 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >>> index 2d37a9ddde..a3ce6d89b6 100644
> >>> --- a/hw/s390x/css.c
> >>> +++ b/hw/s390x/css.c
> >>> @@ -827,6 +827,121 @@ incr:
> >>> return 0;
> >>> }
> >>>
> >>> +/* returns values between 1 and bsz, where bsz is a power of 2 */
> >>> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
> >>> +{
> >>> +return bsz - (cda & (bsz - 1));
> >>> +}
> >>> +
> >>> +static inline uint64_t ccw_ida_block_size(uint8_t flags)
> >>> +{
> >>> +if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
> >>> +return 1ULL << 12;
> >>> +}
> >>> +return 1ULL << 11;
> >>> +}
> >>> +
> >>> +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1,
> >>> + bool idaw_fmt_2)
> >>> +{
> >>> +union {uint64_t fmt2; uint32_t fmt1; } idaw;
> >>> +int ret;
> >>> +hwaddr idaw_addr;
> >>> +
> >>> +if (idaw_fmt_2) {
> >>> +idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> >>> +if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0,
> >>> ccw_fmt1)) {
> >>> +return -EINVAL; /* channel program check */
> >>> +}
> >>> +ret = address_space_rw(&address_space_memory, idaw_addr,
> >> Ahh, just got one question here:
> >> Do we need to considerate endianess for idaw_addr?
> >
> > That is taken care of below.
> >
> > And the previous version worked on my laptop via tcg ;)
>
> Nod.
My fault!
I was thinking of the idaw_addr itself, not the content of it. Now I
realized that, since we already converted (cds->cda_orig) in
copy_ccw_from_guest(), there is no need to convert (idaw_addr +
idaw_size * idaw_index) anymore.
Please ingnore my noise. ;P
>
> >
> >>
> >>> + MEMTXATTRS_UNSPECIFIED, (void *)
> >>> &idaw.fmt2,
> >>> + sizeof(idaw.fmt2), false);
> >>> +cds->cda = be64_to_cpu(idaw.fmt2);
> >>> +} else {
> >>> +idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> >>> +if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0,
> >>> ccw_fmt1)) {
> >>> +return -EINVAL; /* channel program check */
> >>> +}
> >>> +ret = address_space_rw(&address_space_memory, idaw_addr,
> >>> + MEMTXATTRS_UNSPECIFIED, (void *)
> >>> &idaw.fmt1,
> >>> + sizeof(idaw.fmt1), false);
> >>> +cds->cda = be64_to_cpu(idaw.fmt1);
[...]
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA
* Halil Pasic [2017-09-20 18:46:57 +0200]:
>
>
> On 09/20/2017 01:18 PM, Cornelia Huck wrote:
> > On Wed, 20 Sep 2017 13:13:01 +0200
> > Halil Pasic wrote:
> >
> >> On 09/20/2017 10:33 AM, Cornelia Huck wrote:
> >>> On Wed, 20 Sep 2017 15:42:38 +0800
> >>> Dong Jia Shi wrote:
> >>>
> >>>> * Halil Pasic [2017-09-19 20:27:45 +0200]:
> >
> >>>>> + MEMTXATTRS_UNSPECIFIED, (void *)
> >>>>> &idaw.fmt2,
> >>>>> + sizeof(idaw.fmt2), false);
> >>>>> +cds->cda = be64_to_cpu(idaw.fmt2);
> >>>>> +} else {
> >>>>> +idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> >>>>> +if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0,
> >>>>> ccw_fmt1)) {
> >>>>> +return -EINVAL; /* channel program check */
> >>>>> +}
> >>>>> +ret = address_space_rw(&address_space_memory, idaw_addr,
> >>>>> + MEMTXATTRS_UNSPECIFIED, (void *)
> >>>>> &idaw.fmt1,
> >>>>> + sizeof(idaw.fmt1), false);
> >>>>> +cds->cda = be64_to_cpu(idaw.fmt1);
> >>>> Still need to check bit 0x8000 here I think.
> >>>
> >>> Yes, I think this is 'must be zero' for format-1 idaws, and not covered
> >>> by the ccw-format specific checks above. (Although the PoP can be a bit
> >>> confusing with many similar terms...)
> >>>
> >>
> >> It's taken care of in ccw_dstream_rw_ida before the actual
> >> access happens. Code looks like this:
> >> +if (!idaw_fmt2 && (cds->cda + iter_len) >= (1ULL << 31)) {
> >> +ret = -EINVAL; /* channel program check */
> >> +goto err;
> >> +}
> >>
> >> The idea was to have it similar to the non-indirect case.
> >
> >
> >
> > Ah, I was simply looking for the wrong pattern. Looks correct.
> >
> >
>
> Thinking about this some more. Since in case of IDA we are guaranteed
> to never cross a block boundary with a single IDAW we won't ever cross
> block boundary. So we can do the check in ida_read_next_idaw by checking
> bit 0x8000 on the ccw->cda. So we could keep idaw_fmt2 and ccw_fmt1
> local to ida_read_next_idaw and save one goto err. I think that would
> look a bit nicer than what I have here in v3. Agree?
Agree. That would also do the check in the first place. Sounds better.
>
> >>>>> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
> >>>>> + CcwDataStreamOp op)
> >>>>> +{
> >>>>> +uint64_t bsz = ccw_ida_block_size(cds->flags);
> >>>>> +int ret = 0;
> >>>>> +uint16_t cont_left, iter_len;
> >>>>> +const bool idaw_fmt2 = cds->flags & CDS_F_C64;
> >>>>> + bool ccw_fmt1 = cds->flags & CDS_F_FMT;
> >>>> Use 'const bool' either? Although I doubt the value of using const here.
> >>>> ;)
> >>>
> >>> Both being the same is still a good idea.
> >>>
> >>
> >> Yeah. For which one should I go (with const or without)?
> >
> > For the one you prefer :) (I'm not sure if the const adds value here.)
> >
>
> I think we generally don't care about const-ness in such situations,
> so I think I won't use consts.
>
> I intend to fix the issues we have found and do a v4 tomorrow, unless
> somebody screams -- could do it today but I would like to give Dong
> Jia an opportunity to react.
Thanks. I'm coming. :)
> On the other hand waiting more that that will IMHO do us no favor
> either (I think of our storage/memory hierarchy).
>
> Regards,
> Halil
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v3 4/5] 390x/css: introduce maximum data address checking
* Halil Pasic [2017-09-20 13:02:59 +0200]: > > > On 09/20/2017 10:25 AM, Cornelia Huck wrote: > > On Wed, 20 Sep 2017 15:47:51 +0800 > > Dong Jia Shi wrote: > > > >> * Halil Pasic [2017-09-19 20:27:44 +0200]: > > > >>> @@ -828,7 +836,9 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const > >>> *ccw, ORB const *orb) > >>> g_assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW)); > >>> cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) | > >>> (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) | > >>> + (orb->ctrl0 & ORB_CTRL0_MASK_FMT ? CDS_F_FMT : 0) | > >> This reminds me one more question: > >> Calling ccw_dsteram_init() after copy_ccw_from_guest() may lead to a > >> fmt-1 @ccw with an @orb that designates fmt-0 ccw. This sounds insane. > > > > That's just a consequence of us translating everything to format-1 > > ccws. A bit confusing, but no problem if we pay attention to the format > > bit everywhere it makes a difference. > > > > Agree. Ok. I'm fine with this. > > Halil -- Dong Jia Shi
Re: [Qemu-devel] [PATCH v3 4/5] 390x/css: introduce maximum data address checking
* Halil Pasic [2017-09-19 20:27:44 +0200]:
> The architecture mandates the addresses to be accessed on the first
> indirection level (that is, the data addresses without IDA, and the
> (M)IDAW addresses with (M)IDA) to be checked against an CCW format
> dependent limit maximum address. If a violation is detected, the storage
> access is not to be performed and a channel program check needs to be
> generated. As of today, we fail to do this check.
>
> Let us stick even closer to the architecture specification.
>
> Signed-off-by: Halil Pasic
> ---
> hw/s390x/css.c | 10 ++
> include/hw/s390x/css.h | 1 +
> 2 files changed, 11 insertions(+)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 6b0cd8861b..2d37a9ddde 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int
> len)
> return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
> }
>
> +static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1)
> +{
> +return (addr + len) < (ccw_fmt1 ? (1UL << 31) : (1UL << 24));
> +}
> +
> static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
>CcwDataStreamOp op)
> {
> @@ -804,6 +809,9 @@ static int ccw_dstream_rw_noflags(CcwDataStream *cds,
> void *buff, int len,
> if (ret <= 0) {
> return ret;
> }
> +if (!cds_ccw_addrs_ok(cds->cda, len, cds->flags & CDS_F_FMT)) {
> +return -EINVAL; /* channel program check */
> +}
> if (op == CDS_OP_A) {
> goto incr;
> }
> @@ -828,7 +836,9 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const
> *ccw, ORB const *orb)
> g_assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));
> cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
> (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
> + (orb->ctrl0 & ORB_CTRL0_MASK_FMT ? CDS_F_FMT : 0) |
This reminds me one more question:
Calling ccw_dsteram_init() after copy_ccw_from_guest() may lead to a
fmt-1 @ccw with an @orb that designates fmt-0 ccw. This sounds insane.
> (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
> +
> cds->count = ccw->count;
> cds->cda_orig = ccw->cda;
> ccw_dstream_rewind(cds);
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 078356e94c..69b374730e 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -87,6 +87,7 @@ typedef struct CcwDataStream {
> #define CDS_F_MIDA 0x02
> #define CDS_F_I2K 0x04
> #define CDS_F_C64 0x08
> +#define CDS_F_FMT 0x10 /* CCW format-1 */
> #define CDS_F_STREAM_BROKEN 0x80
> uint8_t flags;
> uint8_t at_idaw;
> --
> 2.13.5
>
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v3 5/5] s390x/css: support ccw IDA
* Halil Pasic [2017-09-19 20:27:45 +0200]:
> Let's add indirect data addressing support for our virtual channel
> subsystem. This implementation does not bother with any kind of
> prefetching. We simply step through the IDAL on demand.
>
> Signed-off-by: Halil Pasic
> Signed-off-by: Cornelia Huck
> ---
> hw/s390x/css.c | 117
> -
> 1 file changed, 116 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 2d37a9ddde..a3ce6d89b6 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -827,6 +827,121 @@ incr:
> return 0;
> }
>
> +/* returns values between 1 and bsz, where bsz is a power of 2 */
> +static inline uint16_t ida_continuous_left(hwaddr cda, uint64_t bsz)
> +{
> +return bsz - (cda & (bsz - 1));
> +}
> +
> +static inline uint64_t ccw_ida_block_size(uint8_t flags)
> +{
> +if ((flags & CDS_F_C64) && !(flags & CDS_F_I2K)) {
> +return 1ULL << 12;
> +}
> +return 1ULL << 11;
> +}
> +
> +static inline int ida_read_next_idaw(CcwDataStream *cds, bool ccw_fmt1,
> + bool idaw_fmt_2)
> +{
> +union {uint64_t fmt2; uint32_t fmt1; } idaw;
> +int ret;
> +hwaddr idaw_addr;
> +
> +if (idaw_fmt_2) {
> +idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> +if (idaw_addr & 0x07 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> +return -EINVAL; /* channel program check */
> +}
> +ret = address_space_rw(&address_space_memory, idaw_addr,
Ahh, just got one question here:
Do we need to considerate endianess for idaw_addr?
> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> + sizeof(idaw.fmt2), false);
> +cds->cda = be64_to_cpu(idaw.fmt2);
> +} else {
> +idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> +if (idaw_addr & 0x03 && cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
> +return -EINVAL; /* channel program check */
> +}
> +ret = address_space_rw(&address_space_memory, idaw_addr,
> + MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> + sizeof(idaw.fmt1), false);
> +cds->cda = be64_to_cpu(idaw.fmt1);
Still need to check bit 0x8000 here I think.
> +}
> +++(cds->at_idaw);
> +if (ret != MEMTX_OK) {
> +/* assume inaccessible address */
> +return -EINVAL; /* channel program check */
> +
Extra line.
> +}
> +return 0;
> +}
> +
> +static int ccw_dstream_rw_ida(CcwDataStream *cds, void *buff, int len,
> + CcwDataStreamOp op)
> +{
> +uint64_t bsz = ccw_ida_block_size(cds->flags);
> +int ret = 0;
> + uint16_t cont_left, iter_len;
> +const bool idaw_fmt2 = cds->flags & CDS_F_C64;
> +bool ccw_fmt1 = cds->flags & CDS_F_FMT;
Use 'const bool' either? Although I doubt the value of using const here.
;)
> +
> +ret = cds_check_len(cds, len);
> +if (ret <= 0) {
> +return ret;
> +}
[...]
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v3 3/5] virtio-ccw: use ccw data stream
* Halil Pasic [2017-09-19 20:27:43 +0200]: > Replace direct access which implicitly assumes no IDA > or MIDA with the new ccw data stream interface which should > cope with these transparently in the future. > > Signed-off-by: Halil Pasic > Reviewed-by: Pierre Morel > --- > hw/s390x/virtio-ccw.c | 157 > +++--- > 1 file changed, 46 insertions(+), 111 deletions(-) > Reviewed-by: Dong Jia Shi [...] -- Dong Jia Shi
Re: [Qemu-devel] [PATCH v3 1/5] s390x/css: introduce css data stream
* Halil Pasic [2017-09-19 20:27:41 +0200]:
[...]
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 0653d3c9be..078356e94c 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -75,6 +75,29 @@ typedef struct CMBE {
> uint32_t reserved[7];
> } QEMU_PACKED CMBE;
>
> +typedef enum CcwDataStreamOp {
> +CDS_OP_R = 0, /* read, false when used as is_write */
> +CDS_OP_W = 1, /* write, true when used as is_write */
> +CDS_OP_A = 2 /* advance, should not be used as is_write */
Looks good to me.
> +} CcwDataStreamOp;
> +
>
[...]
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA
* Halil Pasic [2017-09-19 14:04:03 +0200]:
I have no problem with the rest parts of the discussion in this thread.
>
>
> On 09/19/2017 12:57 PM, Cornelia Huck wrote:
> >>>>> +static inline int ida_read_next_idaw(CcwDataStream *cds)
> >>>>> +{
> >>>>> +union {uint64_t fmt2; uint32_t fmt1; } idaw;
> >>>>^
> >>>> Nit.
> >>>>
> >> Maybe checkpatch wanted it this way. My memories are blurry.
> >
> > I'd just leave it like that, tbh.
> >
> >>>>> +bool is_fmt2 = cds->flags & CDS_F_C64;
> >>>>> +int ret;
> >>>>> +hwaddr idaw_addr;
> >>>>> +
> >>>>> +if (is_fmt2) {
> >>>>> +idaw_addr = cds->cda_orig + sizeof(idaw.fmt2) * cds->at_idaw;
> >>>>> +if (idaw_addr & 0x07) {
> >>>>> +return -EINVAL; /* channel program check */
> >>>>> +}
> >>>>> +ret = address_space_rw(&address_space_memory, idaw_addr,
> >>>>> + MEMTXATTRS_UNSPECIFIED, (void *)
> >>>>> &idaw.fmt2,
> >>>>> + sizeof(idaw.fmt2), false);
> >>>>> +cds->cda = be64_to_cpu(idaw.fmt2);
>
>
> >>>>> +} else {
> >>>>> +idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
> >>>>> +if (idaw_addr & 0x03) {
> >>>> ?:
> >>>> (idaw_addr & 0x8003)
> >>> Yes.
> >>>
> >> I will double check this. Does not seem unreasonable but
> >> double-checking is better.
> > Please let me know. I think the architecture says that the bit must be
> > zero, and that we may (...) generate a channel program check.
> >
My fault... This is the address of an IDAW, not the content (data
address) in an IDAW. So what Halil pointed out is the right direction to
go I think.
I will review in the thread of the new version (v3).
>
> Not exactly. The more significant bits part of the check
> depend on the ccw format. This needs to be done for both
> idaw formats. I would need to introduce a new flag, or
> access the SubchDev to do this properly.
>
> Architecturally we also need to check the data addresses
> from which we read so we have nothing bigger than
> (1 << 31) - 1 if we are working with format-1 idaws.
Right. This is what I actually wanted to say.
>
> I also think we did not take proper care of proper
> maximum data address checks prior CwwDataStream which also
> depend on the ccw format (in absence of IDAW or MIDAW).
>
> The ccw format dependent maximum address checks are (1 << 24) - 1
> and (1 << 31) - 1 respectively for format-0 and format-1 (on
> the first indirection level that is for non-IDA for the data,
> and for (M)IDA for the (M)IDAWs).
>
> Reference:
> PoP pages 16-25 and 16-26 "Invalid IDAW or MIDAW Addre" and
> "Invalid Data Address".
>
> How shall we proceed?
>
> Halil
>
> >>>>
> >>>>> +return -EINVAL; /* channel program check */
> >>>>> +
> >>>>> +}
> >>>>> +ret = address_space_rw(&address_space_memory, idaw_addr,
> >>>>> + MEMTXATTRS_UNSPECIFIED, (void *)
> >>>>> &idaw.fmt1,
> >>>>> + sizeof(idaw.fmt1), false);
> >>>>> +cds->cda = be64_to_cpu(idaw.fmt1);>>>>> +}
> >>>>> +++(cds->at_idaw);
> >>>>> +if (ret != MEMTX_OK) {
> >>>>> +/* assume inaccessible address */
> >>>>> +return -EINVAL; /* channel program check */
> >>>>> +
> >>>>> +}
> >>>>> +return 0;
> >>>>> +}
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA
* Halil Pasic [2017-09-19 20:05:48 +0200]:
>
>
> On 09/19/2017 02:23 PM, Cornelia Huck wrote:
> >>>>> +{
> >>>>> +union {uint64_t fmt2; uint32_t fmt1; } idaw;
> >>>>^
> >>>> Nit.
> >>>>
> >> Maybe checkpatch wanted it this way. My memories are blurry.
> > I'd just leave it like that, tbh.
>
> Yes, if I remove the space before the } checkpatch.pl complains.
>
> Going to keep it as is for v3.
My bad. :-/
>
> Halil
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream
* Halil Pasic [2017-09-19 15:30:29 +0200]:
>
>
> On 09/19/2017 11:06 AM, Cornelia Huck wrote:
> > On Tue, 19 Sep 2017 11:37:30 +0800
> > Dong Jia Shi wrote:
> >
> >> * Halil Pasic [2017-09-13 13:50:28 +0200]:
> >>
> >>> Replace direct access which implicitly assumes no IDA
> >>> or MIDA with the new ccw data stream interface which should
> >>> cope with these transparently in the future.
> >>>
> >>> Signed-off-by: Halil Pasic
> >>>
> >>> ---
> >>>
> >>> v1 --> v2:
> >>> Removed todo comments on possible errno change as with
> >>> https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html
> >>> applied it does not matter any more.
> >>>
> >>> Error handling: At the moment we ignore errors reported
> >>> by stream ops to keep the change minimal. An add-on
> >>> patch improving on this is to be expected later.
> >> Add a TODO somewhere around the code as a reminder?
> >
> > I expect Halil to have it on his TODO list and send a patch later ;)
No problem with me.
> >
> >>
> >>> ---
> >>> hw/s390x/virtio-ccw.c | 156
> >>> +++---
> >>> 1 file changed, 46 insertions(+), 110 deletions(-)
> >>>
> >>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> >>> index b1976fdd19..a9baf6f7ab 100644
> >>> --- a/hw/s390x/virtio-ccw.c
> >>> +++ b/hw/s390x/virtio-ccw.c
> >> [...]
> >>
> >>> @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >>> } else {
> >>> VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> >>>
> >>> -features.index = address_space_ldub(&address_space_memory,
> >>> -ccw.cda
> >>> -+
> >>> sizeof(features.features),
> >>> -MEMTXATTRS_UNSPECIFIED,
> >>> -NULL);
> >>> +ccw_dstream_advance(&sch->cds, sizeof(features.features));
> >> How about:
> >> ccw_dstream_advance(&sch->cds, offsetof(VirtioFeatDesc, index));
> >
> > I think keeping sizeof(features.features) is better: It matches the old
> > code, and we really do jump over the features flag and revisit it
> > later...
I was thinking 'advance' to the offset of features.index, then 'read'
the value for features.index seems more natural, and you do not need to
care too much for what exaclty the elements are that you need to
'advance'.
Not big deal for me. I could also accept the original version.
> >
> >>
> >>> +ccw_dstream_read(&sch->cds, features.index);
> >>> if (features.index == 0) {
> >>> if (dev->revision >= 1) {
> >>> /* Don't offer legacy features for modern devices. */
> >>> @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >>> /* Return zeroes if the guest supports more feature
> >>> bits. */
> >>> features.features = 0;
> >>> }
> >>> -address_space_stl_le(&address_space_memory, ccw.cda,
> >>> - features.features,
> >>> MEMTXATTRS_UNSPECIFIED,
> >>> - NULL);
> >>> +ccw_dstream_rewind(&sch->cds);
> >>> +cpu_to_le32s(&features.features);
> >>> +ccw_dstream_write(&sch->cds, features.features);
> >>> sch->curr_status.scsw.count = ccw.count - sizeof(features);
> >> How about:
> >> sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
> >
> > Hm, I thought I had commented on this, but I seem to have missed
> > these...
> >
> > I'd prefer to do it as a follow-up patch.
No problem.
> >
> >>
> >> This also applies to other places.
> >>
> >>> ret = 0;
> >>> }
> >> [...]
> >>
> >>> @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >>> }
> >&
Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
* Halil Pasic [2017-09-08 12:28:03 +0200]: [snip] > > What Halil pointed out is that the ccw_cb implementation of 3270 > > emulation does not take care of IDALs. This is true. > > > > I can also do that right after this series, if Halil agrees. > > (The 3270 emulation authors are busy of other stuff these days. :() > > Generally, yes I agree. If it's trivial I will probably do it myself > for v2. I need to do a v2 anyway. > > Halil I saw you are working on this. So leave it to you. ;) -- Dong Jia Shi
Re: [Qemu-devel] [PATCH v2 4/4] s390x/css: support ccw IDA
}
> +ret = ida_read_next_idaw(cds);
> +if (ret) {
> +goto err;
> +}
> +cont_left = bsz;
> +} while (true);
> +return ret;
> +err:
> + cds->flags |= CDS_F_STREAM_BROKEN;
> +return ret;
> +}
> +
> void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
> {
> /*
> @@ -835,7 +942,7 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const
> *ccw, ORB const *orb)
> if (!(cds->flags & CDS_F_IDA)) {
> cds->op_handler = ccw_dstream_rw_noflags;
> } else {
> -assert(false);
> +cds->op_handler = ccw_dstream_rw_ida;
> }
> }
>
> --
> 2.13.5
>
Generally, the logic looks fine to me.
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v2 3/4] virtio-ccw: use ccw data stream
* Halil Pasic [2017-09-13 13:50:28 +0200]:
> Replace direct access which implicitly assumes no IDA
> or MIDA with the new ccw data stream interface which should
> cope with these transparently in the future.
>
> Signed-off-by: Halil Pasic
>
> ---
>
> v1 --> v2:
> Removed todo comments on possible errno change as with
> https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02441.html
> applied it does not matter any more.
>
> Error handling: At the moment we ignore errors reported
> by stream ops to keep the change minimal. An add-on
> patch improving on this is to be expected later.
Add a TODO somewhere around the code as a reminder?
> ---
> hw/s390x/virtio-ccw.c | 156
> +++---
> 1 file changed, 46 insertions(+), 110 deletions(-)
>
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index b1976fdd19..a9baf6f7ab 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
[...]
> @@ -394,11 +362,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> } else {
> VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
>
> -features.index = address_space_ldub(&address_space_memory,
> -ccw.cda
> -+ sizeof(features.features),
> -MEMTXATTRS_UNSPECIFIED,
> -NULL);
> +ccw_dstream_advance(&sch->cds, sizeof(features.features));
How about:
ccw_dstream_advance(&sch->cds, offsetof(VirtioFeatDesc, index));
> +ccw_dstream_read(&sch->cds, features.index);
> if (features.index == 0) {
> if (dev->revision >= 1) {
> /* Don't offer legacy features for modern devices. */
> @@ -417,9 +382,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> /* Return zeroes if the guest supports more feature bits. */
> features.features = 0;
> }
> -address_space_stl_le(&address_space_memory, ccw.cda,
> - features.features, MEMTXATTRS_UNSPECIFIED,
> - NULL);
> +ccw_dstream_rewind(&sch->cds);
> +cpu_to_le32s(&features.features);
> +ccw_dstream_write(&sch->cds, features.features);
> sch->curr_status.scsw.count = ccw.count - sizeof(features);
How about:
sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
This also applies to other places.
> ret = 0;
> }
[...]
> @@ -501,21 +459,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> }
> }
> len = MIN(ccw.count, vdev->config_len);
> -hw_len = len;
> if (!ccw.cda) {
> ret = -EFAULT;
> } else {
> -config = cpu_physical_memory_map(ccw.cda, &hw_len, 0);
> -if (!config) {
> -ret = -EFAULT;
> -} else {
> -len = hw_len;
> -/* XXX config space endianness */
> -memcpy(vdev->config, config, len);
> -cpu_physical_memory_unmap(config, hw_len, 0, hw_len);
> +ret = ccw_dstream_read_buf(&sch->cds, vdev->config, len);
> +if (!ret) {
Add a TODO here, and:
if (ccw_dstream_read_buf(&sch->cds, vdev->config, len)) {
ret = -EFAULT;
} else {
}
> virtio_bus_set_vdev_config(&dev->bus, vdev->config);
> sch->curr_status.scsw.count = ccw.count - len;
> -ret = 0;
> }
> }
> break;
[...]
> @@ -714,13 +654,9 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> ret = -EFAULT;
> break;
> }
> -revinfo.revision =
> -address_space_lduw_be(&address_space_memory, ccw.cda,
> - MEMTXATTRS_UNSPECIFIED, NULL);
> -revinfo.length =
> -address_space_lduw_be(&address_space_memory,
> - ccw.cda + sizeof(revinfo.revision),
> - MEMTXATTRS_UNSPECIFIED, NULL);
> +ccw_dstream_read_buf(&sch->cds, &revinfo, 4);
^
A magic number? O:)
> +be16_to_cpus(&revinfo.revision);
> +be16_to_cpus(&revinfo.length);
> if (ccw.count < len + revinfo.length ||
> (check_len && ccw.count > len + revinfo.length)) {
> ret = -EINVAL;
> --
> 2.13.5
>
Otherwise, looks good.
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v2 2/4] s390x/css: use ccw data stream
* Halil Pasic [2017-09-13 13:50:27 +0200]:
> Replace direct access which implicitly assumes no IDA
> or MIDA with the new ccw data stream interface which should
> cope with these transparently in the future.
>
> Signed-off-by: Halil Pasic
> ---
> hw/s390x/css.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index e8d2016563..6b0cd8861b 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -890,6 +890,7 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr
> ccw_addr,
> }
>
> /* Look at the command. */
> +ccw_dstream_init(&sch->cds, &ccw, &(sch->orb));
> switch (ccw.cmd_code) {
> case CCW_CMD_NOOP:
> /* Nothing to do. */
> @@ -903,8 +904,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr
> ccw_addr,
> }
> }
> len = MIN(ccw.count, sizeof(sch->sense_data));
> -cpu_physical_memory_write(ccw.cda, sch->sense_data, len);
> -sch->curr_status.scsw.count = ccw.count - len;
> +ccw_dstream_write_buf(&sch->cds, sch->sense_data, len);
> +sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
> memset(sch->sense_data, 0, sizeof(sch->sense_data));
> ret = 0;
> break;
> @@ -930,8 +931,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr
> ccw_addr,
> } else {
> sense_id.reserved = 0;
> }
> -cpu_physical_memory_write(ccw.cda, &sense_id, len);
> -sch->curr_status.scsw.count = ccw.count - len;
> +ccw_dstream_write_buf(&sch->cds, &sense_id, len);
> +sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
> ret = 0;
> break;
> }
> --
> 2.13.5
>
Reviewed-by: Dong Jia Shi
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v2 1/4] s390x/css: introduce css data stream
* Halil Pasic [2017-09-13 13:50:26 +0200]:
[...]
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 0653d3c9be..79acaf99b7 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -75,6 +75,29 @@ typedef struct CMBE {
> uint32_t reserved[7];
> } QEMU_PACKED CMBE;
>
> +typedef enum CcwDataStreamOp {
> +CDS_OP_R = 0,
> +CDS_OP_W = 1,
> +CDS_OP_A = 2
Nit:
typedef enum CcwDataStreamOp {
CDS_OP_R, /* read */
CDS_OP_W, /* write */
CDS_OP_A /* advance */
} CcwDataStreamOp;
(I just keep translating 'A' as append in my mind...)
> +} CcwDataStreamOp;
> +
[...]
> +static inline uint16_t ccw_dstream_avail(CcwDataStream *cds)
> +{
> +return ccw_dstream_good(cds) ? ccw_dstream_residual_count(cds) : 0;
^^
Nit.
> +}
[...]
With or w/o responses the nit picks:
Reviewed-by: Dong Jia Shi
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH 4/9] s390x: refactor error handling for SSCH and RSCH
* Cornelia Huck [2017-09-07 13:41:34 +0200]:
> On Thu, 7 Sep 2017 13:32:41 +0200
> Halil Pasic wrote:
>
> > On 09/07/2017 12:24 PM, Cornelia Huck wrote:
>
> > > So we would do an equipment check for the first two ("equipment", i.e.
> > > the software, is malfunctioning) and use a more appropriate way for the
> > > malformed idaw?
> > >
> >
> > You have probably missed my previous email where I state something very
> > similar (MID <2aa8cf98-c331-fe5a-0a7e-1a553c6c5...@linux.vnet.ibm.com>).
> > There I say: if we change the kernel code, yes, if we don't I prefer a
> > program check.
>
> Not missed, they crossed in mid-air.
>
> But I agree, the decision is Dong Jia's.
>
Let' me summarize here, in case I misunderstand things. Now we have
two ways to choose:
A. Kernel: no change.
Qemu : handle -EFAULT as option 2 by generating a program check.
B. Kernel: return -EFAULT
+
update the IRB area in the I/O region for option 1 to present
a unit check SCSW (with proper sense byte ECW), and for option
2 to present a program check.
Qemu : handle -EFAULT according to the information that the IRB area
provided.
I think the difficult part is in the Qemu side. For either A or B, in
Qemu, we'd need to return a cc0 to indicate the channel program is
accepted successfully by the device, and then update the virtual IRB and
inject an I/O interrupt to notify the guest.
The question is, now we need to map -EFAULT to cc0? This is too odd...
And we need to find a proper place to do the interrupt injection. I
guess it could be in the sch_handle_start_func_passthrough().
If we do not like such modification in the Qemu side, then A is not the
way to go.
And for B, we need to update the IRB area of the I/O region in the three
cases listed in the former mail, and:
1. We need to set the ret_code as 0 for them, so that Qemu could map it
to cc0.
2. We need to signal Qemu to fetch the IRB we generated with the checks.
All of these are doable I think.
Any comment for the above thoughts?
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
* Cornelia Huck [2017-09-07 12:52:20 +0200]: > On Thu, 7 Sep 2017 12:21:50 +0200 > Halil Pasic wrote: > > > On 09/07/2017 10:08 AM, Cornelia Huck wrote: > > > On Thu, 7 Sep 2017 15:31:09 +0800 > > > Dong Jia Shi wrote: > > > >> I'm thinking of a method these days: > > >> Could passing through an fully emulated ccw device (e.g. 3270), or a > > >> virtio ccw device, in the level 1 kvm guest to a level 2 guest be a test > > >> method for this? > > >> > > >> All of the CCWs will be translated to IDAL CCWs by vfio-ccw in the level > > >> 1 guest (which is the level 2 kvm host) and issued to the level 1 kvm > > >> host. So, those IDALs will eventually be handled by the emulated device, > > >> or the virtio ccw device, on the level 1 kvm host... > > >> > > >> Some days ago, one of my colleague tried the emulated 3270 passing > > >> through. She stucked with the problem that the level 1 kvm host handling > > >> a senseid IDAL ccw as a Direct ccw. > > >> > > >> Maybe I could try to pass through a virtio ccw device. I don't think of > > >> any obvious problem that would lead to fail. Any comment? > > >> > > > > > > That actually looks like a good thing to try! Cool idea. > > > > > > > I'm afraid that it would not work without some extra work. > > AFAIR Connie we said that the 3270 does not use any IDA, so > > I did not touch the 3270 emulation code in QEMU. To make > > the scenario viable one should convert the 3270 emulation > > to ccw data stream (unless the original implementation > > already took care of IDA, which I doubt). > > But the vfio-ccw code uses idals... no need to touch the 3270 emulation. > What Halil pointed out is that the ccw_cb implementation of 3270 emulation does not take care of IDALs. This is true. I can also do that right after this series, if Halil agrees. (The 3270 emulation authors are busy of other stuff these days. :() -- Dong Jia Shi
Re: [Qemu-devel] [PATCH 4/9] s390x: refactor error handling for SSCH and RSCH
* Halil Pasic [2017-09-06 16:43:42 +0200]:
>
>
> On 09/06/2017 04:20 PM, Cornelia Huck wrote:
> > On Wed, 6 Sep 2017 14:25:13 +0200
> > Halil Pasic wrote:
> >
> >> We have basically two possibilities/options which ask for different
> >> handling:
> >> 1) EFAULT is due to a bug in the vfio-ccw implementation
> >> (can be QEMU or kernel).
> >> 2) EFAULT is due to buggy channel program.
> >>
> >> Option 2) is basically to be handled with a channel-program check and
> >> setting primary secondary and alert status. For reference see PoP page
> >> 15-59 ("Designation of Storage Area"). An exception may be an invalid
> >> channel program address in the ORB. There the channel-program check ain't
> >> explicitly stated (although) I would expect one. It may be implied by the
> >> things on page 15-59 though.
> >>
> >> Option 1) is however very similar to other we have figured out that the
> >> implementation is broken situations and should be handled consequently.
> >> The current state of the discussion is with a unit exception.
> >>
> >> Does that make sense?
> >
> > I think the situation is slightly different here, though. For the orb
> > flags, we reject something out of hand because we have not implemented
> > it, and for that, unit exception sounds like a good fit. Processing
> > errors, however, are more similar to errors in the hardware, and as
> > such can probably be reported via something like equipment check.
> >
>
> Noted. Let's see what Dong Jia has to say, before we continuing a
> discussion on something (option 1) what may be irrelevant anyway.
>
> >>
> >> Now, Dong Jia, I need your help to figure out do we have option 1 or
> >> option 2 here? After quick look at the kernel code, it appears to me that
> >> I've seen both option 1 and option 2 (I'm afraid) -- but my assessment
> >> was really very superficial.
There are three cases (all in the kernel) that generate a -EFAULT ret
code:
a. vfio_ccw_mdev_write: copy_from_user() fails.
This is option 1.
b. ccwchain_fetch_tic
It's mostly likely that the vfio-ccw driver processed the ccw chains
wrongly. (Actually I can not think of any other reason.)
This is option 1.
c. ccwchain_fetch_idal
When we find that an IDAW contents an invalid address
This is option 2.
> >>
> >> I would expect option 2 to be handled differently (kernel provides the
> >> SCSW) though.
> >>
> >> Regards,
> >> Halil
> >>
> >
> >
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH 2/9] s390x: fix invalid use of cc 1 for SSCH
* Cornelia Huck [2017-09-06 13:25:38 +0200]: > On Wed, 6 Sep 2017 16:27:20 +0800 > Dong Jia Shi wrote: > > > * Halil Pasic [2017-09-05 19:20:43 +0200]: > > > > > > > > > > > On 09/05/2017 05:46 PM, Cornelia Huck wrote: > > > > On Tue, 5 Sep 2017 17:24:19 +0200 > > > > Halil Pasic wrote: > > > > > > > >> My problem with a program check (indicated by SCSW word 2 bit 10) is > > > >> that, in my reading of the architecture, the semantic behind it is: The > > > >> channel subsystem (not the cu or device) has detected, that the > > > >> the channel program (previously submitted as an ORB) is erroneous. > > > >> Which > > > >> programs are erroneous is specified by the architecture. What we have > > > >> here does not qualify. > > > >> > > > >> My idea was to rather blame the virtual hardware (device) and put no > > > >> blame > > > >> on the program nor he channel subsystem. This could be done using > > > >> device > > > >> status (unit check with command reject, maybe unit exception) or > > > >> interface > > > >> check. My train of thought was, the problem is not consistent across a > > > >> device type, so it has to be device specific. > > > > > > > > Unit exception might be a better way to express what is happening here. > > > > At least, it moves us away from cc 1 and not towards cc 3 :) > > > > > > > > > > I will do a follow up patch pursuing device exception. > > > > > > >> > > > >> Of course blaming the device could mislead the person encountering the > > > >> problem, and make him believe it's an non-virtual hardware problem. > > > >> > > > >> About the misleading, I think the best we can do is log out a message > > > >> indicating what really happened. > > > > > > > > Just document it in the code? If it doesn't happen with Linux as a > > > > guest, it is highly unlikely to be seen in the wild. > > > > > > > > > > > > > Well we have two problems here: > > > 1) Unit exception can be already defined by the device type for the > > > command (reference: > > > http://publibfp.dhe.ibm.com/cgi-bin/bookmgr/BOOKS/dz9ar110/2.6.10?DT=19920904110920). > > > I think this one is what you mean. And I agree that's best handled > > > with comment in code. > > Using unit check, with bit 3 byte 0 of the sense data set to 1, to > > indicate an 'Equipment check', sounds a bit more proper than unit > > exception. > > I don't agree: Equipment check sounds a lot more dire (and seems to > imply a malfunction). I like unit exception better. Got the point. Fair enough! -- Dong Jia Shi
Re: [Qemu-devel] [PATCH 5/5] s390x/ccs: add ccw-tester emulated device
* Cornelia Huck [2017-09-06 15:18:21 +0200]: > On Tue, 5 Sep 2017 13:16:45 +0200 > Halil Pasic wrote: > > > Add a fake device meant for testing the correctness of our css emulation. > > > > What we currently have is writing a Fibonacci sequence of uint32_t to the > > device via ccw write. The write is going to fail if it ain't a Fibonacci > > and indicate a device exception in scsw together with the proper residual > > count. > > > > Of course lot's of invalid inputs (besides basic data processing) can be > > tested with that as well. > > > > Usage: > > 1) fire up a qemu with something like -device ccw-tester,devno=fe.0.0001 > >on the command line > > 2) exercise the device from the guest > > > > Signed-off-by: Halil Pasic > > --- > > > > It may not make sense to merge this work in the current form, as it is > > solely for test purposes. > > --- > > hw/s390x/Makefile.objs | 1 + > > hw/s390x/ccw-tester.c | 179 > > + > > 2 files changed, 180 insertions(+) > > create mode 100644 hw/s390x/ccw-tester.c > > The main problem here is that you want to exercise a middle layer (the > css code) and need to write boilerplate code on both host and guest > side in order to be able to do so. > > In general, a device that accepts arbitrary channel programs looks > useful for testing purposes. I would split out processing of expected > responses out, though, so that it can be more easily reused for > different use cases. > > (I dimly recall other test devices...) > > For the guest tester: Can that be done via the qtest infrastructure > somehow? > I'm thinking of a method these days: Could passing through an fully emulated ccw device (e.g. 3270), or a virtio ccw device, in the level 1 kvm guest to a level 2 guest be a test method for this? All of the CCWs will be translated to IDAL CCWs by vfio-ccw in the level 1 guest (which is the level 2 kvm host) and issued to the level 1 kvm host. So, those IDALs will eventually be handled by the emulated device, or the virtio ccw device, on the level 1 kvm host... Some days ago, one of my colleague tried the emulated 3270 passing through. She stucked with the problem that the level 1 kvm host handling a senseid IDAL ccw as a Direct ccw. Maybe I could try to pass through a virtio ccw device. I don't think of any obvious problem that would lead to fail. Any comment? -- Dong Jia Shi
Re: [Qemu-devel] [PATCH 2/9] s390x: fix invalid use of cc 1 for SSCH
* Cornelia Huck [2017-09-05 17:46:06 +0200]: > On Tue, 5 Sep 2017 17:24:19 +0200 > Halil Pasic wrote: > > > My problem with a program check (indicated by SCSW word 2 bit 10) is > > that, in my reading of the architecture, the semantic behind it is: The > > channel subsystem (not the cu or device) has detected, that the > > the channel program (previously submitted as an ORB) is erroneous. Which > > programs are erroneous is specified by the architecture. What we have > > here does not qualify. > > > > My idea was to rather blame the virtual hardware (device) and put no blame > > on the program nor he channel subsystem. This could be done using device > > status (unit check with command reject, maybe unit exception) or interface > > check. My train of thought was, the problem is not consistent across a > > device type, so it has to be device specific. > > Unit exception might be a better way to express what is happening here. > At least, it moves us away from cc 1 and not towards cc 3 :) > > > > > Of course blaming the device could mislead the person encountering the > > problem, and make him believe it's an non-virtual hardware problem. > > > > About the misleading, I think the best we can do is log out a message > > indicating what really happened. > > Just document it in the code? If it doesn't happen with Linux as a > guest, it is highly unlikely to be seen in the wild. > > > > > In the end I don't care that deeply about vfio-ccw, and this problem > > already took me more time than I intended to spend on this. We have > > people driving this whole vfio-ccw stuff and I'm not one of them (I'm > > rather in the supporting role). > > > > I'm also fine with me being credited with a reported-by once the > > more involved people figure out what to do, and keeping the vfio-ccw > > stuff as is. Should we go with that option? > > If converting the reporting to a device status is straightforward > enough, let's do that. I'm fine with postponing this and waiting for a > real fix as well (I don't really have spare cycles to actually write > vfio-ccw code currently...) > I can do this after this series. [...] -- Dong Jia Shi
Re: [Qemu-devel] [PATCH 2/9] s390x: fix invalid use of cc 1 for SSCH
* Halil Pasic [2017-09-05 19:20:43 +0200]: > > > On 09/05/2017 05:46 PM, Cornelia Huck wrote: > > On Tue, 5 Sep 2017 17:24:19 +0200 > > Halil Pasic wrote: > > > >> My problem with a program check (indicated by SCSW word 2 bit 10) is > >> that, in my reading of the architecture, the semantic behind it is: The > >> channel subsystem (not the cu or device) has detected, that the > >> the channel program (previously submitted as an ORB) is erroneous. Which > >> programs are erroneous is specified by the architecture. What we have > >> here does not qualify. > >> > >> My idea was to rather blame the virtual hardware (device) and put no blame > >> on the program nor he channel subsystem. This could be done using device > >> status (unit check with command reject, maybe unit exception) or interface > >> check. My train of thought was, the problem is not consistent across a > >> device type, so it has to be device specific. > > > > Unit exception might be a better way to express what is happening here. > > At least, it moves us away from cc 1 and not towards cc 3 :) > > > > I will do a follow up patch pursuing device exception. > > >> > >> Of course blaming the device could mislead the person encountering the > >> problem, and make him believe it's an non-virtual hardware problem. > >> > >> About the misleading, I think the best we can do is log out a message > >> indicating what really happened. > > > > Just document it in the code? If it doesn't happen with Linux as a > > guest, it is highly unlikely to be seen in the wild. > > > > > Well we have two problems here: > 1) Unit exception can be already defined by the device type for the > command (reference: > http://publibfp.dhe.ibm.com/cgi-bin/bookmgr/BOOKS/dz9ar110/2.6.10?DT=19920904110920). > I think this one is what you mean. And I agree that's best handled > with comment in code. Using unit check, with bit 3 byte 0 of the sense data set to 1, to indicate an 'Equipment check', sounds a bit more proper than unit exception. > 2) The poor user/programmer is trying to figure out why things > don't work (why are we getting the unit exception)? I think that's > best remedied with producing something for the log (maybe a warning > with warn_report which states that the implementation vfio-ccw requires > the given flags). Fine with me. [...] -- Dong Jia Shi
Re: [Qemu-devel] [PATCH 4/9] s390x: refactor error handling for SSCH and RSCH
* Halil Pasic [2017-09-06 00:30:58 +0200]: [...] > > > >> But I guess, I was afraid of somebody blaming me for this > >> comment (such TODOs in production code are a bit strange -- what > >> does it mean: we did not bother to figure it out?). > > > > For one, the TODO is preexisting... and we really should remember to > > figure out if there's something better rather than just drop the > > comment. > > > > (And I sure hope nobody is using vfio-ccw in production yet...) > > > Since blame says the TODO has been around since 2017-05-17 > let me have a critical look at it. > > At a first glance I would say addressing exception for SSCH > is not what we want: the only possibility I see for address > exception for SSCH is due to the ORB address. But if that's > the case we will never reach the code in question. Agree. > So I suppose we can do better. As the comment said, I'm (still) in the state of 'wondering'. > > Adding Ren. @Ren: Do you agree with my analysis. If you do, > I could come up with a proposal what to do -- after some reading. If you have a better idea, and time, why not? ;) > > Regards, > Halil -- Dong Jia Shi
[Qemu-devel] [PATCH v3 1/2] s390x/css: use macro for event-information pending error recover code
Let's use a macro for the ERC (error recover code) when generating a
Channel Subsystem Event-information pending CRW (channel report word).
While we are at it, let's also add all other ERCs.
Signed-off-by: Dong Jia Shi
Reviewed-by: Halil Pasic
---
hw/s390x/css.c| 2 +-
include/hw/s390x/ioinst.h | 12 ++--
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 6a42b95cee..5321ca016b 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -2103,7 +2103,7 @@ void css_generate_chp_crws(uint8_t cssid, uint8_t chpid)
void css_generate_css_crws(uint8_t cssid)
{
if (!channel_subsys.sei_pending) {
-css_queue_crw(CRW_RSC_CSS, 0, 0, cssid);
+css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, cssid);
}
channel_subsys.sei_pending = true;
}
diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h
index 92d15655e4..5f2db6949d 100644
--- a/include/hw/s390x/ioinst.h
+++ b/include/hw/s390x/ioinst.h
@@ -201,8 +201,16 @@ typedef struct CRW {
#define CRW_FLAGS_MASK_A 0x0080
#define CRW_FLAGS_MASK_ERC 0x003f
-#define CRW_ERC_INIT 0x02
-#define CRW_ERC_IPI 0x04
+#define CRW_ERC_EVENT0x00 /* event information pending */
+#define CRW_ERC_AVAIL0x01 /* available */
+#define CRW_ERC_INIT 0x02 /* initialized */
+#define CRW_ERC_TERROR 0x03 /* temporary error */
+#define CRW_ERC_IPI 0x04 /* installed parm initialized */
+#define CRW_ERC_TERM 0x05 /* terminal */
+#define CRW_ERC_PERRN0x06 /* perm. error, facility not init */
+#define CRW_ERC_PERRI0x07 /* perm. error, facility init */
+#define CRW_ERC_PMOD 0x08 /* installed parameters modified */
+#define CRW_ERC_IPR 0x0A /* installed parameters restored */
#define CRW_RSC_SUBCH 0x3
#define CRW_RSC_CHP 0x4
--
2.11.2
[Qemu-devel] [PATCH v3 2/2] s390x/css: generate solicited crw for rchp completion signaling
A successful completion of rchp should signal a solicited channel path
initialized CRW (channel report word), while the current implementation
always generates an un-solicited one. Let's fix this.
Reported-by: Halil Pasic
Signed-off-by: Dong Jia Shi
Reviewed-by: Halil Pasic
---
hw/s390x/css.c | 16 ++--
include/hw/s390x/css.h | 3 ++-
2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 5321ca016b..bfa56f4b12 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1745,10 +1745,10 @@ int css_do_rchp(uint8_t cssid, uint8_t chpid)
}
/* We don't really use a channel path, so we're done here. */
-css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT,
+css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1,
channel_subsys.max_cssid > 0 ? 1 : 0, chpid);
if (channel_subsys.max_cssid > 0) {
-css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 0, real_cssid << 8);
+css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1, 0, real_cssid << 8);
}
return 0;
}
@@ -2028,7 +2028,8 @@ void css_subch_assign(uint8_t cssid, uint8_t ssid,
uint16_t schid,
}
}
-void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid)
+void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
+ int chain, uint16_t rsid)
{
CrwContainer *crw_cont;
@@ -2040,6 +2041,9 @@ void css_queue_crw(uint8_t rsc, uint8_t erc, int chain,
uint16_t rsid)
return;
}
crw_cont->crw.flags = (rsc << 8) | erc;
+if (solicited) {
+crw_cont->crw.flags |= CRW_FLAGS_MASK_S;
+}
if (chain) {
crw_cont->crw.flags |= CRW_FLAGS_MASK_C;
}
@@ -2086,9 +2090,9 @@ void css_generate_sch_crws(uint8_t cssid, uint8_t ssid,
uint16_t schid,
}
chain_crw = (channel_subsys.max_ssid > 0) ||
(channel_subsys.max_cssid > 0);
-css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, chain_crw ? 1 : 0, schid);
+css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0, chain_crw ? 1 : 0, schid);
if (chain_crw) {
-css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0,
+css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0, 0,
(guest_cssid << 8) | (ssid << 4));
}
/* RW_ERC_IPI --> clear pending interrupts */
@@ -2103,7 +2107,7 @@ void css_generate_chp_crws(uint8_t cssid, uint8_t chpid)
void css_generate_css_crws(uint8_t cssid)
{
if (!channel_subsys.sei_pending) {
-css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, cssid);
+css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, 0, cssid);
}
channel_subsys.sei_pending = true;
}
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 5c5fe6b202..5b017e1fc3 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -150,7 +150,8 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src);
void css_inject_io_interrupt(SubchDev *sch);
void css_reset(void);
void css_reset_sch(SubchDev *sch);
-void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid);
+void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
+ int chain, uint16_t rsid);
void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, uint16_t schid,
int hotplugged, int add);
void css_generate_chp_crws(uint8_t cssid, uint8_t chpid);
--
2.11.2
[Qemu-devel] [PATCH v3 0/2] ERC cleanup and CRW bugfix
This series is trying to: 1. clear up ERC related code 2. bugfix for channel path related CRW generation Change log -- v2->v3: Added Halil's R-B on both patches. Patch #1: Added ERC "installed parameters restored". v1->v2: Patch #1: Add all ERCs. Commit message update. Patch #2: Rename the new added parameter to more speaking name -- solicited. Dong Jia Shi (2): s390x/css: use macro for event-information pending error recover code s390x/css: generate solicited crw for rchp completion signaling hw/s390x/css.c| 16 ++-- include/hw/s390x/css.h| 3 ++- include/hw/s390x/ioinst.h | 12 ++-- 3 files changed, 22 insertions(+), 9 deletions(-) -- 2.11.2
Re: [Qemu-devel] [PATCH v2 2/2] s390x/css: generate solicited crw for rchp completion signaling
* Halil Pasic [2017-08-01 17:16:37 +0200]:
>
>
> On 08/01/2017 09:57 AM, Dong Jia Shi wrote:
> [..]
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -1745,10 +1745,10 @@ int css_do_rchp(uint8_t cssid, uint8_t chpid)
> > }
> >
> > /* We don't really use a channel path, so we're done here. */
> > -css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT,
> > +css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1,
> >channel_subsys.max_cssid > 0 ? 1 : 0, chpid);
> > if (channel_subsys.max_cssid > 0) {
> > -css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 0, real_cssid << 8);
> > +css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1, 0, real_cssid << 8);
> > }
> > return 0;
> > }
> > @@ -2028,7 +2028,8 @@ void css_subch_assign(uint8_t cssid, uint8_t ssid,
> > uint16_t schid,
> > }
> > }
> >
> > -void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid)
> > +void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
> > + int chain, uint16_t rsid)
>
> I think you could make the parameters solicited and chain bool (AFAIU
> they are conceptually bool) for clearer semantic. If you go with that
> you could also get rid of the superfluous ternary operator ( we have
> stuff like some_cond ? 1 : 0 for the chain parameter in more than
> one place.
>
> Btw. I find bool flags easy to mix up and difficult to read. I have no better
> idea how to write this (in C) though. I was considering throwing chain and
> solicited together into a single flags parameter, but looking at the client
> code
> it does not look like a good idea.
I think I just need to get used to differet tastes. ;)
>
> Besides the cosmetic considerations above LGTM
Thanks!
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v2 1/2] s390x/css: use macro for event-information pending error recover code
* Halil Pasic [2017-08-01 17:24:10 +0200]:
>
>
> On 08/01/2017 09:57 AM, Dong Jia Shi wrote:
> > Let's use a macro for the ERC (error recover code) when generating a
> > Channel Subsystem Event-information pending CRW (channel report word).
> >
> > While we are at it, let's also add all other ERCs.
> >
> > Signed-off-by: Dong Jia Shi
> > ---
> > hw/s390x/css.c| 2 +-
> > include/hw/s390x/ioinst.h | 11 +--
> > 2 files changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> > index 6a42b95cee..5321ca016b 100644
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -2103,7 +2103,7 @@ void css_generate_chp_crws(uint8_t cssid, uint8_t
> > chpid)
> > void css_generate_css_crws(uint8_t cssid)
> > {
> > if (!channel_subsys.sei_pending) {
> > -css_queue_crw(CRW_RSC_CSS, 0, 0, cssid);
> > +css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, cssid);
> > }
> > channel_subsys.sei_pending = true;
> > }
> > diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h
> > index 92d15655e4..f89019f78f 100644
> > --- a/include/hw/s390x/ioinst.h
> > +++ b/include/hw/s390x/ioinst.h
> > @@ -201,8 +201,15 @@ typedef struct CRW {
> > #define CRW_FLAGS_MASK_A 0x0080
> > #define CRW_FLAGS_MASK_ERC 0x003f
> >
> > -#define CRW_ERC_INIT 0x02
> > -#define CRW_ERC_IPI 0x04
> > +#define CRW_ERC_EVENT0x00 /* event information pending */
> > +#define CRW_ERC_AVAIL0x01 /* available */
> > +#define CRW_ERC_INIT 0x02 /* initialized */
> > +#define CRW_ERC_TERROR 0x03 /* temporary error */
> > +#define CRW_ERC_IPI 0x04 /* installed parm initialized */
> > +#define CRW_ERC_TERM 0x05 /* terminal */
> > +#define CRW_ERC_PERRN0x06 /* perm. error, facility not init */
> > +#define CRW_ERC_PERRI0x07 /* perm. error, facility init */
> > +#define CRW_ERC_PMOD 0x08 /* installed parameters modified */
>
> You have missed installed parameters restored from the PoP (above
> you say add all other).
Ok. Will add:
#define CRW_ERC_IPR 0x0A /* installed parameters restored */
>
> Other than that.
>
> LGTM
>
> >
> > #define CRW_RSC_SUBCH 0x3
> > #define CRW_RSC_CHP 0x4
> >
--
Dong Jia Shi
[Qemu-devel] [PATCH v2 2/2] s390x/css: generate solicited crw for rchp completion signaling
A successful completion of rchp should signal a solicited channel path
initialized CRW (channel report word), while the current implementation
always generates an un-solicited one. Let's fix this.
Reported-by: Halil Pasic
Signed-off-by: Dong Jia Shi
---
hw/s390x/css.c | 16 ++--
include/hw/s390x/css.h | 3 ++-
2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 5321ca016b..bfa56f4b12 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1745,10 +1745,10 @@ int css_do_rchp(uint8_t cssid, uint8_t chpid)
}
/* We don't really use a channel path, so we're done here. */
-css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT,
+css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1,
channel_subsys.max_cssid > 0 ? 1 : 0, chpid);
if (channel_subsys.max_cssid > 0) {
-css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 0, real_cssid << 8);
+css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1, 0, real_cssid << 8);
}
return 0;
}
@@ -2028,7 +2028,8 @@ void css_subch_assign(uint8_t cssid, uint8_t ssid,
uint16_t schid,
}
}
-void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid)
+void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
+ int chain, uint16_t rsid)
{
CrwContainer *crw_cont;
@@ -2040,6 +2041,9 @@ void css_queue_crw(uint8_t rsc, uint8_t erc, int chain,
uint16_t rsid)
return;
}
crw_cont->crw.flags = (rsc << 8) | erc;
+if (solicited) {
+crw_cont->crw.flags |= CRW_FLAGS_MASK_S;
+}
if (chain) {
crw_cont->crw.flags |= CRW_FLAGS_MASK_C;
}
@@ -2086,9 +2090,9 @@ void css_generate_sch_crws(uint8_t cssid, uint8_t ssid,
uint16_t schid,
}
chain_crw = (channel_subsys.max_ssid > 0) ||
(channel_subsys.max_cssid > 0);
-css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, chain_crw ? 1 : 0, schid);
+css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0, chain_crw ? 1 : 0, schid);
if (chain_crw) {
-css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0,
+css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0, 0,
(guest_cssid << 8) | (ssid << 4));
}
/* RW_ERC_IPI --> clear pending interrupts */
@@ -2103,7 +2107,7 @@ void css_generate_chp_crws(uint8_t cssid, uint8_t chpid)
void css_generate_css_crws(uint8_t cssid)
{
if (!channel_subsys.sei_pending) {
-css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, cssid);
+css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, 0, cssid);
}
channel_subsys.sei_pending = true;
}
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 5c5fe6b202..5b017e1fc3 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -150,7 +150,8 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src);
void css_inject_io_interrupt(SubchDev *sch);
void css_reset(void);
void css_reset_sch(SubchDev *sch);
-void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid);
+void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
+ int chain, uint16_t rsid);
void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, uint16_t schid,
int hotplugged, int add);
void css_generate_chp_crws(uint8_t cssid, uint8_t chpid);
--
2.11.2
[Qemu-devel] [PATCH v2 0/2] ERC cleanup and CRW bugfix (was: Channel Path realted CRW generation)
This series is trying to: 1. clear up ERC related code 2. bugfix for channel path related CRW generation Change log -- v1->v2: Patch #1: Add all ERCs. Commit message update. Patch #2: Rename the new added parameter to more speaking name -- solicited. Dong Jia Shi (2): s390x/css: use macro for event-information pending error recover code s390x/css: generate solicited crw for rchp completion signaling hw/s390x/css.c| 16 ++-- include/hw/s390x/css.h| 3 ++- include/hw/s390x/ioinst.h | 11 +-- 3 files changed, 21 insertions(+), 9 deletions(-) -- 2.11.2
[Qemu-devel] [PATCH v2 1/2] s390x/css: use macro for event-information pending error recover code
Let's use a macro for the ERC (error recover code) when generating a
Channel Subsystem Event-information pending CRW (channel report word).
While we are at it, let's also add all other ERCs.
Signed-off-by: Dong Jia Shi
---
hw/s390x/css.c| 2 +-
include/hw/s390x/ioinst.h | 11 +--
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 6a42b95cee..5321ca016b 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -2103,7 +2103,7 @@ void css_generate_chp_crws(uint8_t cssid, uint8_t chpid)
void css_generate_css_crws(uint8_t cssid)
{
if (!channel_subsys.sei_pending) {
-css_queue_crw(CRW_RSC_CSS, 0, 0, cssid);
+css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, cssid);
}
channel_subsys.sei_pending = true;
}
diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h
index 92d15655e4..f89019f78f 100644
--- a/include/hw/s390x/ioinst.h
+++ b/include/hw/s390x/ioinst.h
@@ -201,8 +201,15 @@ typedef struct CRW {
#define CRW_FLAGS_MASK_A 0x0080
#define CRW_FLAGS_MASK_ERC 0x003f
-#define CRW_ERC_INIT 0x02
-#define CRW_ERC_IPI 0x04
+#define CRW_ERC_EVENT0x00 /* event information pending */
+#define CRW_ERC_AVAIL0x01 /* available */
+#define CRW_ERC_INIT 0x02 /* initialized */
+#define CRW_ERC_TERROR 0x03 /* temporary error */
+#define CRW_ERC_IPI 0x04 /* installed parm initialized */
+#define CRW_ERC_TERM 0x05 /* terminal */
+#define CRW_ERC_PERRN0x06 /* perm. error, facility not init */
+#define CRW_ERC_PERRI0x07 /* perm. error, facility init */
+#define CRW_ERC_PMOD 0x08 /* installed parameters modified */
#define CRW_RSC_SUBCH 0x3
#define CRW_RSC_CHP 0x4
--
2.11.2
Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug
* Cornelia Huck [2017-08-01 09:24:20 +0200]: > On Tue, 1 Aug 2017 10:29:10 +0800 > Dong Jia Shi wrote: > > > * Cornelia Huck [2017-07-31 13:13:02 +0200]: > > > > > On Mon, 31 Jul 2017 11:51:37 +0800 > > > Dong Jia Shi 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 > > > > > > We could use this command line: > > -device > > vfio-ccw,sysfsdev=$MDEV_CCW013f,devno=0.0.1234,chpids=4245 > > > > Yes, that would work, should we want that. We can probably do without for now. > Let's deffer this too! -- Dong Jia Shi
Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug
* Cornelia Huck [2017-07-31 13:13:02 +0200]: > On Mon, 31 Jul 2017 11:51:37 +0800 > Dong Jia Shi wrote: > > > * Cornelia Huck [2017-07-27 13:59:10 +0200]: > > > > > On Thu, 27 Jul 2017 03:54:18 +0200 > > > Dong Jia Shi 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=1122 > > 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
Re: [Qemu-devel] [PATCH 0/3] Channel Path realted CRW generation
* Cornelia Huck [2017-07-31 10:54:47 +0200]: > On Fri, 28 Jul 2017 23:50:48 +0800 > Dong Jia Shi wrote: > > > * Cornelia Huck [2017-07-28 13:53:01 +0200]: > > > > > > You're bound to get different kinds of notifications: via a CRW with > > > > > source channel path, via event information retrievable via CHSC > > > > > (indicated by a CRW with source CSS), > > > > Ha, I was not awre of this one before! > > > > > > That's the 'link incident' and 'resource accessibility' stuff. > > My focus was trying to have the minimum stuff to make a Linux guest > > working well -- basically, my working on prototype targeted to make the > > output lschp and lscss corect and uptodate. I > > > > I will dig this and see if I need to do more stuff. > > You can probably skip this for now, unless you want to propagate the > ficon-related stuff. I don't even want to know about that now. ;) > Just plain channel-path related changes should already cover the > interesting stuff. > > > > > My prototype work tries to sync the belowing information from host > > > > kernel to qemu: > > > > 1. the real SCHIB, so stsch from guest could get the updated path > > > > masks. > > > > > > How far do you want to go with mirroring? I think you need to modify at > > > least the devno in the pmcw, no? > > I didn't think this very deep. For now, I only sync the PIM, POM, PAM > > and CHPIDs lazily. > > Also consider the pno bit and the pnom. Roger! > > > For devno... I need to think more. If the qemu command has a given > > "devno" for the vfio-ccw device, maybe we should not override its dev_id > > with the real one "device number". > > The guest should not be surprised by a different devno, so you need to > be sure everything is consistent. Ok. Will handle the device number. > > > > > > 3. still working on support CHSC store channel path description > > > > command. > > > > > > I'm currently wondering how many of those chscs are optional. OTOH, if > > > a modern Linux guest cannot work properly without them, it makes no > > > sense to leave them out. > > Nod. > > > > But I think I need to define the criteria for "work properly". For > > example, with the current code, a Linux guest with a passed through > > device works, while lschp shows the Cfg. as 3 (not recognized), and the > > Shared and PCHID as "-". For this case, do you think it "work properly"? > > It depends upon what you want to expose to the guest. Some > configuration checking or management tools might be reporting a > configuration deficiency (*might*, I do not know). This is helpful. > > Shared and PGID may be useful if the operator wants to perform some > maintenance on the hardware (so they can figure out which systems/disks > are affected), but the information should be available in the > hypervisor as well, so I'm not sure whether it's a big deal. > Oh! This information is also very helpful. Since I only want to expose the minimum information that the guest needs to work without serious problem. I think I can also deffer these stuff until we have the good chp modelling. -- Dong Jia Shi
Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug
* Halil Pasic [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 wrote: > > > >> * Cornelia Huck [2017-07-27 13:59:10 +0200]: > >> > >>> On Thu, 27 Jul 2017 03:54:18 +0200 > >>> Dong Jia Shi wrote: > [..] > >> > >> After thinking quite a while, if we do want to add a real device object > >> for a channel path, the most intractable problem (but not the only one) > >> for me is to find a good way to map the real path with the virtual one. > > Yeah, channel path virtualisation... IMHO our current solution is not > not really solving the problem. If we choose to go with Conny's idea, then yes, my prototype is totally in the wrong direction. > Say, do we care about a design which could work with live migration > (@Dong Jia)? Channel I/O pass through and live migration don't go together. I'm afraid, we did not considerate live migration yet. If we can migrate, that would be great! But we are still struggling in finding a way out the current swamp... > > >> How would we retrieve the information from the real one? We'd need the > >> host kernel to provide totally new interfaces for channel path > >> information synchronization and notification machenism. I don't think in > >> this case sysfs is the choice. Ioctls, vfio MMIO regions and eventfd > >> could be a better choice. I think, this is like we are trying to > >> passthru a channel path. So we'd need to have a new vfio device physical > >> driver (e.g. vfio-chp) to handle this... > > > > And that would run into the problem of (1) the chp objects not using a > > bus on Linux, and (2) implying dedicating the chpids, which we likely > > do not want. > > > > AFAIU this is about "real-virtual" vs "virutal-virtual". I would really like > to see some design here (@Dong Jia). I'm not sure any more where do we want > to go. If we can find a way to satisfy the requirements that Conny mentioned, I can prepare a design. Sadly, we are not there yet. > > [..] > >> > >>> > >>> tl;dr: I don't think we want chp crws until after we have a good chp > >>> model. > >> I have to agree. > > > > I'm wondering whether we should just defer this to later. We can live > > with no chp crw being created (except for rchp), as due to the crw > > generation being unreliable the guest OS has to handle path changes > > without crws anyway. > > > > We just need to make sure that pno and friends are appropriately > > reported to the guest. > > > > +1 Ha! This is very very clever! I will give it a try. If everything go well, I will agree with this happyly. ;) -- Dong Jia Shi
Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug
* Cornelia Huck [2017-07-31 10:41:47 +0200]: > On Mon, 31 Jul 2017 09:46:17 +0800 > Dong Jia Shi wrote: > > > * Cornelia Huck [2017-07-28 14:58:19 +0200]: > > > > Exposing real channel paths to the guest means that the guest OS needs > > > to be able to deal with path-related things, but OTOH it has more > > > control. As I don't think we'll ever want to support a guest OS that > > > does not also run under LPAR, I'd prefer that way. > > > > > My poor English... Sorry, I don't undersatnd the last sentence... > > Probably too many negations... let me rephrase. Negations kill my brain. ;) > > Looking at the guest OSes we want to support, it's currently Linux, > Linux, or Linux. And Linux runs fine under LPAR, as it is able to deal > with the details of channel path handling (and LPAR does not virtualize > anything of this). Nod. > > Of the other OSes, z/OS is way too insanely complex, z/VM does not make > much sense, and I don't see a case for z/TPF either. Which leaves > z/VSE, which should be able to deal with the path related stuff as well. Nod. > > So, I don't think we close any doors if we expose the path handling > complexities to the guest. > Understand now! Thanks a lot! -- Dong Jia Shi
Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug
* Cornelia Huck [2017-07-27 13:59:10 +0200]: > On Thu, 27 Jul 2017 03:54:18 +0200 > Dong Jia Shi 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=1122 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 lschp: CHPID Vary Cfg. Type Cmg Shared PCHID 0.42 1 1 1b21 0158 0.43 1 1 1b21 0159 0.44 1 1 1b21 01a0 0.45 1 1 1b2
Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug
* Cornelia Huck [2017-07-28 14:58:19 +0200]: [...] > > > > If I understand you correctly it ain't possible to handle these > > in the host (and let the guest a simple 'non-real' virtual > > channel path whose reliability depends on what the host does), > > or? > > It is possible. Mapping to a virtual channel path or not is basically a > design decision (IIRC, z/VM supports both). > > Mapping everything to a virtual chpid basically concentrates all > path-related handling in the hypervisor. This allows for a dumb guest > OS, but can make errors really hard to debug from the guest side. I understood this. > > Exposing real channel paths to the guest means that the guest OS needs > to be able to deal with path-related things, but OTOH it has more > control. As I don't think we'll ever want to support a guest OS that > does not also run under LPAR, I'd prefer that way. > My poor English... Sorry, I don't undersatnd the last sentence... [...] -- Dong Jia Shi
Re: [Qemu-devel] [PATCH 0/3] Channel Path realted CRW generation
ed and PCHID as "-". For this case, do you think it "work properly"?
>
> >
> > > (I had added the path-come CRW handling in Linux back then and
> > > afterwards wondered why we did not get it - I must have interpreted
> > > the PoP in the same way as you did.)
> > I've a bugfix patch in the kernel side, and it has been accepted by the
> > s390 maintainers. May be this could be a clue? Post it here:
> >
> > When channel path is identified as the report source code (RSC)
> > of a CRW, and initialized (CRW_ERC_INIT) is recognized as the
> > error recovery code (ERC) by the channel subsystem, it indicates
> > a "path has come" event.
> >
> > Let's handle this case in chp_process_crw().
> >
> > diff --git a/drivers/s390/cio/chp.c b/drivers/s390/cio/chp.c
> > index 7e0d4f724dda..432fc40990bd 100644
> > --- a/drivers/s390/cio/chp.c
> > +++ b/drivers/s390/cio/chp.c
> > @@ -559,6 +559,7 @@ static void chp_process_crw(struct crw *crw0, struct
> > crw *crw1,
> > chpid.id = crw0->rsid;
> > switch (crw0->erc) {
> > case CRW_ERC_IPARM: /* Path has come. */
> > + case CRW_ERC_INIT:
> > if (!chp_is_registered(chpid))
> > chp_new(chpid);
> > chsc_chp_online(chpid);
> >
> > Notice:
> > At the very beginning, I replaced CRW_ERC_IPARM with CRW_ERC_INIT. But
> > Sebstian Ott suggested:
> > "I don't know of a machine that actually implements a CRW
> > at all when a chpid is configured online on the SE/HMC.
> >
> > Because of potential regressions I don't want to remove CRW_ERC_IPARM
> > here. I'm good with adding CRW_ERC_INIT though."
>
> Yeah, that makes sense, especially with the confusing state channel
> path machine check handling is in from the architecture side.
>
> >
> > >
> > > I'll double check with how I'd interpret the PoP today.
> > >
> > Thanks.
>
> I have read through the PoP and the outcome is a bit disappointing.
> Much of it is a bit vague. I still think that you can err on the side
> of overindication, though.
>
I agree. Unless somebody tells me it's forbidden by the PoP explicitly,
or it will break Linux guest from working properly, I will would this as
the way to go.
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH 0/3] Channel Path realted CRW generation
* Cornelia Huck [2017-07-27 11:46:03 +0200]:
> On Thu, 27 Jul 2017 03:54:15 +0200
> Dong Jia Shi wrote:
>
> > This series is trying to:
> > 1. clear up CRW related code.
> > 2. generate the right channel path related CRW at the right time.
> >
> > I did this mainly because it's a requirement from my current work, that is
> > I'm
> > in preparation of a group of patch for channel path virtualization. I can
> > use
> > the inerface that provided by this series later, so as to, for vfio-ccw
> > devices, notify the guest with channel path status vary that happens on the
> > host side.
>
> Sounds cool.
>
> >
> > During an internal discussion, Halil and Pierre pointed out that for path
> > hotplug, generating a CRW seems logical, but how is it covered by the AR is
> > not
> > clear - we have problem in understanding some grammar ambiguous paragraphs.
> > While certain parts of the AR is not available outside, but I'm still
> > wondering
> > if the author ;) could give us some clue... BTW, we know that, in Linux
> > kernel
> > we had code that handles un-solicited chp crw, so we tend to believe it's
> > right
> > to generate channel path initialized CRW for path hotplug. It's just we can
> > not
> > find the reason from the document.
>
> I always found path notifications to be a bit odd. They depend on
> various things:
> - whether you're running under LPAR or under z/VM
> - whether it's a hardware condition (path failure) or something
> triggered by the admin (path vary on/off)
> - if it's admin triggered, where it was done (on the SE, by one of
> several mechanisms in CP, via SCLP)
These are clear.
During the internnal discussion, we wished to get the resources to test
all of these cases to verify. For the z/VM and SE stuff, it seems a bit
difficult. So we decided to go with a shortcut -- to ask you.
>
> You're bound to get different kinds of notifications: via a CRW with
> source channel path, via event information retrievable via CHSC
> (indicated by a CRW with source CSS),
Ha, I was not awre of this one before!
> via a PNO indication, or nothing at all.
>
> [Reminds me of a case where we got path gone CRWs under LPAR when a
> path was deactivated at the SE (which we would notice via PNO anyway),
> but no CRW when the path was reactivated - not very useful. When trying
> to report this as an issue, we got the answer that we of course need to
> use the OS interface to vary off the path beforehand. Silly penguins.]
... ...
>
> My recommendation would be to generate a fitting CRW if the wording
> allows to do so.
Nod. I'm trying this already.
> I would hope that getting as many useful indications as possible is
> most helpful to the OS.
Nod. Trying this too.
My prototype work tries to sync the belowing information from host
kernel to qemu:
1. the real SCHIB, so stsch from guest could get the updated path masks.
2. the Store Subchannel Description information, and with the new added
support for the SCLP read channel path information command, guest could
get the configure status of the path.
3. still working on support CHSC store channel path description command.
> (I had added the path-come CRW handling in Linux back then and
> afterwards wondered why we did not get it - I must have interpreted
> the PoP in the same way as you did.)
I've a bugfix patch in the kernel side, and it has been accepted by the
s390 maintainers. May be this could be a clue? Post it here:
When channel path is identified as the report source code (RSC)
of a CRW, and initialized (CRW_ERC_INIT) is recognized as the
error recovery code (ERC) by the channel subsystem, it indicates
a "path has come" event.
Let's handle this case in chp_process_crw().
diff --git a/drivers/s390/cio/chp.c b/drivers/s390/cio/chp.c
index 7e0d4f724dda..432fc40990bd 100644
--- a/drivers/s390/cio/chp.c
+++ b/drivers/s390/cio/chp.c
@@ -559,6 +559,7 @@ static void chp_process_crw(struct crw *crw0, struct
crw *crw1,
chpid.id = crw0->rsid;
switch (crw0->erc) {
case CRW_ERC_IPARM: /* Path has come. */
+ case CRW_ERC_INIT:
if (!chp_is_registered(chpid))
chp_new(chpid);
chsc_chp_online(chpid);
Notice:
At the very beginning, I replaced CRW_ERC_IPARM with CRW_ERC_INIT. But
Sebstian Ott suggested:
"I don't know of a machine that actually implements a CRW
at all when a chpid is configured online on the SE/HMC.
Because of potential regressions I don't want to remove CRW_ERC_IPARM
here. I'm good with adding CRW_ERC_INIT though."
>
> I'll double check with how I'd interpret the PoP today.
>
Thanks.
[...]
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH 2/3] s390x/css: generate solicited crw for rchp completion signaling
* Cornelia Huck [2017-07-27 13:22:59 +0200]:
[...]
> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
[...]
> > @@ -2028,7 +2028,7 @@ void css_subch_assign(uint8_t cssid, uint8_t ssid,
> > uint16_t schid,
> > }
> > }
> >
> > -void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid)
> > +void css_queue_crw(uint8_t rsc, uint8_t erc, int s, int chain, uint16_t
> > rsid)
>
> 's' is not a very speaking name...
>
> > {
> > CrwContainer *crw_cont;
> >
> > @@ -2040,6 +2040,9 @@ void css_queue_crw(uint8_t rsc, uint8_t erc, int
> > chain, uint16_t rsid)
> > return;
> > }
> > crw_cont->crw.flags = (rsc << 8) | erc;
> > +if (s) {
> > +crw_cont->crw.flags |= CRW_FLAGS_MASK_S;
>
> ...as it obviously causes the S flag to be set ;) Let's call it 'solicited'?
Sure. Will adopt.
>
> > +}
> > if (chain) {
> > crw_cont->crw.flags |= CRW_FLAGS_MASK_C;
> > }
> > @@ -2086,9 +2089,9 @@ void css_generate_sch_crws(uint8_t cssid, uint8_t
> > ssid, uint16_t schid,
> > }
> > chain_crw = (channel_subsys.max_ssid > 0) ||
> > (channel_subsys.max_cssid > 0);
> > -css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, chain_crw ? 1 : 0, schid);
> > +css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0, chain_crw ? 1 : 0, schid);
> > if (chain_crw) {
> > -css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0,
> > +css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0, 0,
> >(guest_cssid << 8) | (ssid << 4));
> > }
> > /* RW_ERC_IPI --> clear pending interrupts */
> > @@ -2103,7 +2106,7 @@ void css_generate_chp_crws(uint8_t cssid, uint8_t
> > chpid)
> > void css_generate_css_crws(uint8_t cssid)
> > {
> > if (!channel_subsys.sei_pending) {
> > -css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, cssid);
> > +css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, 0, cssid);
>
> Should we want to support OS-triggered channel path vary (via SCLP or
> otherwise) in the future,
Yes! I had a prototype of series to handle the OS-triggered chp vary,
and that needs...
> we'll probably need a version that generates a solicited crw.
...the new interface with the solicited bit param, which is provided by
patch #3:
void css_generate_chp_crws(uint8_t cssid, uint8_t chpid,
int hotplugged, int add, int s);
BTW, I need to renew the legal clearance before sending them out...
>
> > }
> > channel_subsys.sei_pending = true;
> > }
> > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> > index 5c5fe6b202..d03b4ffeac 100644
> > --- a/include/hw/s390x/css.h
> > +++ b/include/hw/s390x/css.h
> > @@ -150,7 +150,7 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src);
> > void css_inject_io_interrupt(SubchDev *sch);
> > void css_reset(void);
> > void css_reset_sch(SubchDev *sch);
> > -void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid);
> > +void css_queue_crw(uint8_t rsc, uint8_t erc, int s, int chain, uint16_t
> > rsid);
> > void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, uint16_t schid,
> > int hotplugged, int add);
> > void css_generate_chp_crws(uint8_t cssid, uint8_t chpid);
>
> Otherwise, patch looks good.
Thanks.
So, I only need to s/s/solicited for the new version of this one?
>
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH 1/3] s390x/css: use macro for event-information pending error recover code
* Cornelia Huck [2017-07-27 12:10:17 +0200]:
[...]
> > diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h
> > index 92d15655e4..c1d1052279 100644
> > --- a/include/hw/s390x/ioinst.h
> > +++ b/include/hw/s390x/ioinst.h
> > @@ -201,8 +201,9 @@ typedef struct CRW {
> > #define CRW_FLAGS_MASK_A 0x0080
> > #define CRW_FLAGS_MASK_ERC 0x003f
> >
> > -#define CRW_ERC_INIT 0x02
> > -#define CRW_ERC_IPI 0x04
> > +#define CRW_ERC_EVENT 0x00
>
> OK, that matches the name the Linux kernel uses.
A thief was caught by you. ;)
>
> Do we want to add all ERCs while we're at it?
>
> > +#define CRW_ERC_INIT 0x02
> > +#define CRW_ERC_IPI0x04
No problem for me. I can do that by stealing again:
#define CRW_ERC_EVENT0x00 /* event information pending */
#define CRW_ERC_AVAIL0x01 /* available */
#define CRW_ERC_INIT 0x02 /* initialized */
#define CRW_ERC_TERROR 0x03 /* temporary error */
#define CRW_ERC_IPI 0x04 /* installed parm initialized */
#define CRW_ERC_TERM 0x05 /* terminal */
#define CRW_ERC_PERRN0x06 /* perm. error, facility not init */
#define CRW_ERC_PERRI0x07 /* perm. error, facility init */
#define CRW_ERC_PMOD 0x08 /* installed parameters modified */
Want the comment or not? I like them.
> >
> > #define CRW_RSC_SUBCH 0x3
> > #define CRW_RSC_CHP 0x4
>
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH v2 1/1] s390x/css: check ccw address validity
* Halil Pasic [2017-07-27 17:48:42 +0200]:
> According to the PoP channel command words (CCW) must be doubleword
> aligned and 31 bit addressable for format 1 and 24 bit addressable for
> format 0 CCWs.
>
> If the channel subsystem encounters ccw address which does not satisfy
> this alignment requirement a program-check condition is recognised.
>
> The situation with 31 bit addressable is a bit more complicated: both the
> ORB and a format 1 CCW TIC hold the address of (the rest of) the channel
> program, that is the address of the next CCW in a word, and the PoP
> mandates that bit 0 of that word shall be zero -- or a program-check
> condition is to be recognized -- and does not belong to the field holding
> the ccw address.
>
> Since in code the corresponding fields span across the whole word (unlike
> in PoP where these are defined as 31 bit wide) we can check this by
> applying a mask. The 24 addressable case isn't affecting TIC because the
> address is composed of a halfword and a byte portion (no additional zero
> bit requirements) and just slightly complicates the ORB case where also
> bits 1-7 need to be zero.
>
> The same requirements (especially n-bit addressability) apply to the
> ccw addresses generated while chaining.
Cool. This is very clear.
>
> Let's make our CSS implementation follow the AR more closely.
>
> Signed-off-by: Halil Pasic
> ---
>
> This patch used to be a patch used to be a part of the series 'ccw
> interpretation AR compliance improvements' but the other patch was
> already applied.
>
> I would still like this one being in front of '390x/css: fix bits must be
> zero check for TIC' as the commit message of that change relies the
> changes done in this patch.
Nothing I could comment. So leave this to Conny.
>
> v1 -> v2
> * fixed condition (precedence was wrong -- thanks Dong Jia)
> * check on each iteration when chaining (every ccw of the channel
> program needs to be 31 or 24 bit addressable (if touched), not only
> the addresses in TIC and ORB)
> ---
> hw/s390x/css.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 6a42b95cee..177cbfc92d 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -795,6 +795,10 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr
> ccw_addr,
> if (!ccw_addr) {
> return -EIO;
> }
> +/* Check doubleword aligned and 31 or 24 (fmt 0) bit addressable. */
> +if (ccw_addr & (sch->ccw_fmt_1 ? 0x8007 : 0xff07)) {
> +return -EINVAL;
> +}
Reviewed-by: Dong Jia Shi
>
> /* Translate everything to format-1 ccws - the information is the same.
> */
> ccw = copy_ccw_from_guest(ccw_addr, sch->ccw_fmt_1);
> --
> 2.11.2
>
--
Dong Jia Shi
Re: [Qemu-devel] [PATCH 2/2] s390x/css: fix bits must be zero check for TIC
* Cornelia Huck [2017-07-27 10:32:14 +0200]:
> On Wed, 26 Jul 2017 00:44:42 +0200
> Halil Pasic wrote:
>
> > According to the PoP bit positions 0-3 and 8-32 of the format-1 CCW must
> > contain zeros. Bits 0-3 are already covered by cmd_code validity
> > checking, and bit 32 is covered by the CCW address checking.
> >
> > Bits 8-31 correspond to CCW1.flags and CCW1.count. Currently we only
> > check for the absence of certain flags. Let's fix this.
> >
> > Signed-off-by: Halil Pasic
> > ---
> > hw/s390x/css.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> > index d17e21b7af..1f04ce4a1b 100644
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -884,7 +884,8 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr
> > ccw_addr,
> > ret = -EINVAL;
> > break;
> > }
> > -if (ccw.flags & (CCW_FLAG_CC | CCW_FLAG_DC)) {
> > +if (ccw.flags || ccw.count) {
> > +/* We have already sanitized these if fmt 0. */
> > ret = -EINVAL;
> > break;
> > }
>
> Thanks, applied (with tweaked comment).
>
> Dong Jia: I've added your R-b, please let me know if that's not ok.
Yes, please. That's ok.
(Just cann't help to miss the chance to reply to you ;)
--
Dong Jia Shi
