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

[Eric Farman <farman@linux.ibm.com>]

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.)

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