Re: [RFC PATCH v1 03/10] vfio-ccw: Use subchannel lpm in the orb

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 19 Nov 2019 10:16:30 -0500
Eric Farman <farman@linux.ibm.com> wrote:

> On 11/19/19 8:00 AM, Cornelia Huck wrote:
> > On Fri, 15 Nov 2019 03:56:13 +0100
> > Eric Farman <farman@linux.ibm.com> wrote:
> >   
> >> From: Farhan Ali <alifm@linux.ibm.com>
> >>
> >> The subchannel logical path mask (lpm) would have the most
> >> up to date information of channel paths that are logically
> >> available for the subchannel.
> >>
> >> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> >> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> >> ---
> >>
> >> Notes:
> >>     v0->v1: [EF]
> >>      - None; however I am greatly confused by this one.  Thoughts?  
> > 
> > I think it's actually wrong.
> >   
> >>
> >>  drivers/s390/cio/vfio_ccw_cp.c | 4 +---
> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> >> index 3645d1720c4b..d4a86fb9d162 100644
> >> --- a/drivers/s390/cio/vfio_ccw_cp.c
> >> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> >> @@ -779,9 +779,7 @@ union orb *cp_get_orb(struct channel_program *cp, u32 intparm, u8 lpm)
> >>  	orb->cmd.intparm = intparm;
> >>  	orb->cmd.fmt = 1;
> >>  	orb->cmd.key = PAGE_DEFAULT_KEY >> 4;
> >> -
> >> -	if (orb->cmd.lpm == 0)
> >> -		orb->cmd.lpm = lpm;  
> > 
> > In the end, the old code will use the lpm from the subchannel
> > structure, if userspace did not supply anything to be used.
> >   
> >> +	orb->cmd.lpm = lpm;  
> > 
> > The new code will always discard any lpm userspace has supplied and
> > replace it with the lpm from the subchannel structure. This feels
> > wrong; what if the I/O is supposed to be restricted to a subset of the
> > paths?  
> 
> I had the same opinion, but didn't want to flat-out discard it from his
> series without a second look.  :)

:)

> 
> > 
> > If we want to include the current value of the subchannel lpm in the
> > requests, we probably want to AND the masks instead.  
> 
> Then we'd be on the hook to return some sort of error if the result is
> zero.  Is it better to just send it to hw as-is, and let the response
> come back naturally?  (Which is what we do today.)

But if a chpid is logically varied off, it is removed from the lpm,
right? Therefore, the caller really should get a 'no path' indication
back, shouldn't it?

> 
> >   
> >>  
> >>  	chain = list_first_entry(&cp->ccwchain_list, struct ccwchain, next);
> >>  	cpa = chain->ch_ccw;  
> >   
>