Message-Id: <878tcfex1x.fsf@bjsdjshi.ibm.com>
Date:   Tue, 30 Jan 2018 13:27:06 +0800
From:   Dong Jia Shi <bjsdjshi@...ux.vnet.ibm.com>
To:     Halil Pasic <pasic@...ux.vnet.ibm.com>
Cc:     linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
        kvm@...r.kernel.org, qemu-devel@...gnu.org, qemu-s390x@...gnu.org,
        cohuck@...hat.com, borntraeger@...ibm.com,
        pmorel@...ux.vnet.ibm.com
Subject: Re: [Qemu-devel] [RFC PATCH 0/3] vfio: ccw: basic channel path event handling

Halil Pasic <pasic@...ux.vnet.ibm.com> writes:
--text follows this line--
Hi Halil,
--text follows this line--
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.
See reply below #LabelA.

>
>> 
>> 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.
>
> I thing your answer is based on the misunderstanding explained above.
I see now.

>
> My idea was to be lazy in a different way. Instead of being lazy about
> reading the subsection, I was wondering why not do an STSCH in the host
> as a response to one being done in the guest.
>
> That means: if there is no activity on the passed trough devices, nothing
> needs to be done. (Except maybe the CRWs).
>
#LabelA:

I see your point. This is an interesting idea.

Pros:
- code simplification
- no chp event notification and handling

Cons:
- have to read schib region (or issue STSCH on host) for each STSCH
  request from a guest

I think code siplification is very acctractive, and performance is not
the most important issue. Any comments? @Conny

> The things aren't observable by the guest if it does not do STSCH anyway.
Nod. This is the idea that this series is based on.

>
>
>> 
>>> * 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).
>> 
>
> Please elaborate on that basically. Or are those actually just correct
> in your opinion?!
>
As listed in a former mail, the following values will be updated for the
guest:
PMCW:
  - PIM/PAM/POM
  - PNOM
  - CHPIDs
SCSW
  - PNOM bit                                                                                           

What else do we need?

>> 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.
>> 
>
> Fair.
>
>>> 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.
>> 
>
> Sorry, I ended up confusing opm with pom. That would have been illegal
> (as Connie has pointed out recently) because it can only  change
> only if the path is tried for IO.
What is opm? Sorry, I lost in translation... See Conny's mail, and let's
discuss there?

>
>
>>>  If yes what are the implications? Are the manipulations you do on
>>>  some PMCW fields architecturally correct?
>> If "architecturally correct" means what guest sees/senses is all obey to
>> the PoP, then I think so. We don't have to emulate the internal
>> behaviors (which could not be seen by OS) of CSS exactly when we
>> emulate, if the emulation does not impact on what Linux guest will see,
>> right?
>> 
>> I mean, the time point we update the PMCW does not has to be in the time
>> slot of io function performance. The guest would still have to get the
>> updated information through the calls of STSCH. We only need to provide
>> the updaetd result to the guest by handling STSCH well. And that's why
>> we say we do that lazily.
>> 
>> Nothing different (added value) will be seen by the guest, comparing
>> with the current lazy implementation I think.> 
>>> * The one thing I would very much appreciate as an user of vfio,
>>> and should in my understanding be the user story of this series
>>> (and the qemu counterpart of course) is the following. If my device
>>> (that is IO operation) failed because no path could be found on
>>> which the device is accessible, I would like to be able to identify
>>> that. Preferably the same way I would do for an LPAR guest. Is this
>>> series accomplishing that? 
>> With this the guest could lazily identify that. But since we have no
>> machine check propagation yet, for those cases which generate CRWs, it
>> might not be the same way for an LPAR guest I think.
>> 
>
> This was a yes/no question. I can't interpret your answer neither as
> yes or as no. Maybe a little clarification: I'm talking about a
> recent linux guest. 
See Conny's mail.

>
>>> * Why not just do proper STSCH for vfio-ccw?
>> I only did the interested parts (path related). For the other parts, the
>> current handling is enough, no? Anyway, after we have the schib region,
>> we can do further STSCH handling any time later we want then.
>> 
>
> I mean, your approach works on the premise, that the host will notify
> QEMU each time the content of the SCHIB (area) changes (modulo stuff) so
> it can do an update in QEMU, and give the guest an up to date virtualized
> SCHIB when it asks for it executing the STSCH instruction.
>
> I was asking myself. Why not instead while interpreting STSCH do a
> STSCH on the host device (that is passed through), and virtualize the
> result as necessary.
>
> It occurred to me if nothing simpler. 
I think I got your point now, and replied in the above #LabelA section.

>
>>> * Shouldn't we virtualize CHPIDs?
>> Nobody would say no for this. :) The problem is that this is something
>> big, and it's not a short-term goal in 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

Powered by blists - more mailing lists