On Wed, 15 May 2019 16:08:18 -0400
Farhan Ali <alifm@xxxxxxxxxxxxx> wrote:
> On 05/15/2019 11:04 AM, Eric Farman wrote:
> >
> >
> > On 5/15/19 8:23 AM, Cornelia Huck wrote:
> >> On Wed, 15 May 2019 01:42:46 +0200
> >> Eric Farman <farman@xxxxxxxxxxxxx> wrote:
> >>
> >>> It is possible that a guest might issue a CCW with a length of zero,
> >>> and will expect a particular response. Consider this chain:
> >>>
> >>> Address Format-1 CCW
> >>> -------- -----------------
> >>> 0 33110EC0 346022CC 33177468
> >>> 1 33110EC8 CF200000 3318300C
> >>>
> >>> CCW[0] moves a little more than two pages, but also has the
> >>> Suppress Length Indication (SLI) bit set to handle the expectation
> >>> that considerably less data will be moved. CCW[1] also has the SLI
> >>> bit set, and has a length of zero. Once vfio-ccw does its magic,
> >>> the kernel issues a start subchannel on behalf of the guest with this:
> >>>
> >>> Address Format-1 CCW
> >>> -------- -----------------
> >>> 0 021EDED0 346422CC 021F0000
> >>> 1 021EDED8 CF240000 3318300C
> >>>
> >>> Both CCWs were converted to an IDAL and have the corresponding flags
> >>> set (which is by design), but only the address of the first data
> >>> address is converted to something the host is aware of. The second
> >>> CCW still has the address used by the guest, which happens to be (A)
> >>> (probably) an invalid address for the host, and (B) an invalid IDAW
> >>> address (doubleword boundary, etc.).
> >>>
> >>> While the I/O fails, it doesn't fail correctly. In this example, we
> >>> would receive a program check for an invalid IDAW address, instead of
> >>> a unit check for an invalid command.
> >>>
> >>> To fix this, revert commit 4cebc5d6a6ff ("vfio: ccw: validate the
> >>> count field of a ccw before pinning") and allow the individual fetch
> >>> routines to process them like anything else. We'll make a slight
> >>> adjustment to our allocation of the pfn_array (for direct CCWs) or
> >>> IDAL (for IDAL CCWs) memory, so that we have room for at least one
> >>> address even though no data will be transferred.
> >>>
> >>> Note that this doesn't provide us with a channel program that will
> >>> fail in the expected way. Since our length is zero, vfio_pin_pages()
> >
> > s/is/was/
> >
> >>> returns -EINVAL and cp_prefetch() will thus fail. This will be fixed
> >>> in the next patch.
> >>
> >> So, this failed before, and still fails, just differently?
> >
> > Probably. If the guest gave us a valid address, the pin might actually
> > work now whereas before it would fail because the length was zero. If
> > the address were also invalid,
> >
> > >IOW, this
> >> has no effect on bisectability?
> >
> > I think so, but I suppose that either (A) patch 5 and 6 could be
> > squashed together, or (B) I could move the "set pa_nr to zero" (or more
> > accurately, set it to ccw->count) pieces from patch 6 into this patch,
> > so that the vfio_pin_pages() call occurs like it does today.
> >
> >>
>
> While going through patch 5, I was confused as to why we need to pin
> pages if we are only trying to translate the addresses and no data
> transfer will take place with count==0. Well, you answer that in patch 6 :)
>
> So maybe it might be better to move parts of patch 6 to 5 or squash
> them, or maybe reverse the order.
I think this will get a bit unwieldy of squashed, so what about simply
moving code from 6 to 5? I think people are confused enough by the two
patches to make a change look like a good idea.
(I can queue patches 1-4 to get them out of the way :)
>
> Thanks
> Farhan
>
>
> >>>
> >>> Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
> >>> ---
> >>> drivers/s390/cio/vfio_ccw_cp.c | 26 ++++++++------------------
> >>> 1 file changed, 8 insertions(+), 18 deletions(-)
> >>
> >
>
