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

[Cornelia Huck]

On Thu, 27 Jul 2017 18:15:07 +0200
Halil Pasic <address@hidden> wrote:

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

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

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

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

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

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