Re: [PATCH 7/7] s390/cio: Remove vfio-ccw checks of command codes

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

On 5/7/19 4:52 AM, Pierre Morel wrote:
> On 06/05/2019 22:47, Eric Farman wrote:
>>
>>
>> On 5/6/19 11:39 AM, Eric Farman wrote:
>>>
>>>
>>> On 5/6/19 8:56 AM, Pierre Morel wrote:
>>>> On 03/05/2019 15:49, Eric Farman wrote:
>>>>> If the CCW being processed is a No-Operation, then by definition no
>>>>> data is being transferred.  Let's fold those checks into the normal
>>>>> CCW processors, rather than skipping out early.
>>>>>
>>>>> Likewise, if the CCW being processed is a "test" (an invented
>>>>> definition to simply mean it ends in a zero), let's permit that to go
>>>>> through to the hardware.  There's nothing inherently unique about
>>>>> those command codes versus one that ends in an eight [1], or any other
>>>>> otherwise valid command codes that are undefined for the device type
>>>>> in question.
>>>>>
>>>>> [1] POPS states that a x08 is a TIC CCW, and that having any 
>>>>> high-order
>>>>> bits enabled is invalid for format-1 CCWs.  For format-0 CCWs, the
>>>>> high-order bits are ignored.
>>>>>
>>>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>>>> ---
>>>>>   drivers/s390/cio/vfio_ccw_cp.c | 11 +++++------
>>>>>   1 file changed, 5 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c 
>>>>> b/drivers/s390/cio/vfio_ccw_cp.c
>>>>> index 36d76b821209..c0a52025bf06 100644
>>>>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>>>>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>>>>> @@ -289,8 +289,6 @@ static long copy_ccw_from_iova(struct 
>>>>> channel_program *cp,
>>>>>   #define ccw_is_read_backward(_ccw) (((_ccw)->cmd_code & 0x0F) == 
>>>>> 0x0C)
>>>>>   #define ccw_is_sense(_ccw) (((_ccw)->cmd_code & 0x0F) == 
>>>>> CCW_CMD_BASIC_SENSE)
>>>>> -#define ccw_is_test(_ccw) (((_ccw)->cmd_code & 0x0F) == 0)
>>>>> -
>>>>>   #define ccw_is_noop(_ccw) ((_ccw)->cmd_code == CCW_CMD_NOOP)
>>>>>   #define ccw_is_tic(_ccw) ((_ccw)->cmd_code == CCW_CMD_TIC)
>>>>> @@ -314,6 +312,10 @@ static inline int 
>>>>> ccw_does_data_transfer(struct ccw1 *ccw)
>>>>>       if (ccw->count == 0)
>>>>>           return 0;
>>>>> +    /* If the command is a NOP, then no data will be transferred */
>>>>> +    if (ccw_is_noop(ccw))
>>>>> +        return 0;
>>>>> +
>>>>>       /* If the skip flag is off, then data will be transferred */
>>>>>       if (!ccw_is_skip(ccw))
>>>>>           return 1;
>>>>> @@ -398,7 +400,7 @@ static void ccwchain_cda_free(struct ccwchain 
>>>>> *chain, int idx)
>>>>>   {
>>>>>       struct ccw1 *ccw = chain->ch_ccw + idx;
>>>>> -    if (ccw_is_test(ccw) || ccw_is_noop(ccw) || ccw_is_tic(ccw))
>>>>> +    if (ccw_is_tic(ccw))
>>>>
>>>>
>>>> AFAIR, we introduced this code to protect against noop and test with 
>>>> a non zero CDA.
>>>> This could go away only if there is somewhere the guaranty that noop 
>>>> have always a null CDA (same for test).
>>>
>>> What was generating either the null or "test" command codes?  I can 
>>> provide plenty of examples for both these command codes and how they 
>>> look coming out of vfio-ccw now.
>>
>> I've sent both x00 and x03 (NOP) CCWs with zero and non-zero CDAs to 
>> hardware without this patch.  I don't see anything particuarly 
>> surpising, so I'm not sure what the original code was attempting to 
>> protect.
>>
>> Maybe, since you question this in ccwchain_cda_free(), you're 
>> referring to commit 408358b50dea ("s390: vfio-ccw: Do not attempt to 
>> free no-op, test and tic cda."), which fixed up our attempt to clean 
>> things up that weren't allocated on the transmit side?  With this 
>> series, that is reverted, but the cda is indeed set to something that 
>> needs to be free'd (see below).  So maybe I should at least mention 
>> that commit here.
>>
>> Regardless, while the I/Os work/fail as I expect, the cda addresses 
>> themselves are wrong in much the same way I describe in patch 4.  Yes, 
>> without this patch we don't convert them to an IDAL so certain program 
>> checks aren't applicable.  But the addresses that we end up sending to 
>> the hardware are nonsensical, though potentially valid, locations.
>>
> 
> I am not comfortable with this.
> with NOOP no data transfer take place and the role of VFIO is to take 
> care about data transfer.
> So in my logic better do nothing and send the original CCW to the hardware.
> 
>>>
>>> The noop check is moved up into the "does data transfer" routine, to 
>>> determine whether the pages should be pinned or not.  Regardless of 
>>> whether or not the input CDA is null, we'll end up with a CCW 
>>> pointing to a valid IDAL of invalid addresses.
>>>
>>> The "test" command codes always struck me as funky, because x18 and 
>>> xF8 and everything in between that ends in x8 is architecturally 
>>> invalid too, but we don't check for them like we do for things that 
>>> end in x0. And there's a TON of other opcodes that are invalid for 
>>> today's ECKD devices, or perhaps were valid for older DASD but have 
>>> since been deprecated, or are only valid for non-DASD device types. 
>>> We have no logic to permit them, either.  If those CCWs had a 
>>> non-zero CDA, we either pin it successfully and let the targeted 
>>> device sort it out or an error occurs and we fail at that point.  
>>> (QEMU will see a "wirte" region error of -EINVAL because of 
>>> vfio_pin_pages())
> 
> The test command is AFAIU even more sensible that the NOOP command and 
> in my opinion should never be modified since it is highly device 
> dependent and do not induce data transfer anyway.
> 
> We even do not know how the CDA field may be used by the device.

Exactly, which is why I think sending an unpinned, non-translated, guest 
address to the hardware (which is what happens today) is a Bad Idea.  If 
the associated command code WERE going to cause the channel to modify 
any memory, the provided address from the guest would (best case) cause 
a program check if the address were not available, or some data 
corruption if it were.

> May be I am a little dramatic with this.
> Just to say that I would feel more comfortable if the test command reach 
> the device unchanged.
> 

As I say above, I disagree.  I'd rather that the command (test or 
otherwise) hit the channel (and the device if applicable) with a valid 
host address in ccw.cda, so that if any data transfer occurs we're not 
exposed.

If there's an application that wants to send a test CCW with an invalid 
CDA (and thus would fail the pin, as I have seen with NOP), then I guess 
I can add ccw_is_test() to ccw_does_data_transfer(), but since I still 
don't see the use case for test CCWs I'm not as thrilled about it.

Do you recall what caused them to be added originally?

>>>
>>>>
>>>>
>>>>
>>>>>           return;
>>>>>       kfree((void *)(u64)ccw->cda);
>>>>> @@ -723,9 +725,6 @@ static int ccwchain_fetch_one(struct ccwchain 
>>>>> *chain,
>>>>>   {
>>>>>       struct ccw1 *ccw = chain->ch_ccw + idx;
>>>>> -    if (ccw_is_test(ccw) || ccw_is_noop(ccw))
>>>>> -        return 0;
>>>>> -
>>>>>       if (ccw_is_tic(ccw))
>>>>>           return ccwchain_fetch_tic(chain, idx, cp);
>>>>>
>>>>
>>>>
> 
>