[PATCH v2 0/1] vfio-ccw: Enable transparent CCW IPL from DASD [QEMU]

[PATCH v2 0/1] vfio-ccw: Enable transparent CCW IPL from DASD [QEMU]

[Jared Rossi]

Remove the explicit prefetch check when using vfio-ccw devices.
This check does not trigger in practice as all Linux channel programs
are intended to use prefetch.

QEMU part to make use of the corresponding kernel patch posted here:

https://lore.kernel.org/kvm/20200506212440.31323-1-jrossi@linux.ibm.com/

It is no longer required to force the PFCH flag, but the option is
still supported.


Jared Rossi (1):
  vfio-ccw: allow non-prefetch ORBs

 hw/vfio/ccw.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

-- 
2.17.0

[PATCH v2 1/1] vfio-ccw: allow non-prefetch ORBs

[Jared Rossi]

Remove the explicit prefetch check when using vfio-ccw devices.
This check does not trigger in practice as all Linux channel programs
are intended to use prefetch.

It is no longer required to force the PFCH flag when using vfio-ccw
devices.

Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
 hw/vfio/ccw.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 50cc2ec75c..e649377b68 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -74,16 +74,9 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
     struct ccw_io_region *region = vcdev->io_region;
     int ret;
 
-    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
-        if (!(vcdev->force_orb_pfch)) {
-            warn_once_pfch(vcdev, sch, "requires PFCH flag set");
-            sch_gen_unit_exception(sch);
-            css_inject_io_interrupt(sch);
-            return IOINST_CC_EXPECTED;
-        } else {
-            sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
-            warn_once_pfch(vcdev, sch, "PFCH flag forced");
-        }
+    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH) && vcdev->force_orb_pfch) {
+        sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
+        warn_once_pfch(vcdev, sch, "PFCH flag forced");
     }
 
     QEMU_BUILD_BUG_ON(sizeof(region->orb_area) != sizeof(ORB));
-- 
2.17.0

Re: [PATCH v2 1/1] vfio-ccw: allow non-prefetch ORBs

[Cornelia Huck]

On Tue, 12 May 2020 14:15:35 -0400
Jared Rossi <jrossi@linux.ibm.com> wrote:

> Remove the explicit prefetch check when using vfio-ccw devices.
> This check does not trigger in practice as all Linux channel programs
> are intended to use prefetch.
> 
> It is no longer required to force the PFCH flag when using vfio-ccw
> devices.

That's not quite true: Only kernels that include the currently-queued
patch do not require it. Maybe

"Newer Linux kernel versions do not require to force the PFCH flag with
vfio-ccw devices anymore."

?

> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>  hw/vfio/ccw.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 50cc2ec75c..e649377b68 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -74,16 +74,9 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
>      struct ccw_io_region *region = vcdev->io_region;
>      int ret;
>  
> -    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
> -        if (!(vcdev->force_orb_pfch)) {
> -            warn_once_pfch(vcdev, sch, "requires PFCH flag set");
> -            sch_gen_unit_exception(sch);
> -            css_inject_io_interrupt(sch);
> -            return IOINST_CC_EXPECTED;
> -        } else {
> -            sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
> -            warn_once_pfch(vcdev, sch, "PFCH flag forced");
> -        }
> +    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH) && vcdev->force_orb_pfch) {
> +        sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
> +        warn_once_pfch(vcdev, sch, "PFCH flag forced");
>      }
>  
>      QEMU_BUILD_BUG_ON(sizeof(region->orb_area) != sizeof(ORB));

Let me spell out what happens:
- PFCH bit set -> no change
- PFCH bit not set, but force_orb_pfch set -> no change
- neither PFCH bit nor force_orb_pfch set:
  - older kernels: QEMU makes the request, the kernel rejects it, guest
    gets a unit exception (same result for the guest as before, only a
    different code flow)
  - newer kernels: QEMU makes the request, the kernel forwards the
    request (logging a rate-limited warning); the result depends on
    whether the guest actually tries to rewrite the channel program or
    not

I think that is what we want, and I think I'll queue this patch with
the tweaked commit message, but I'd like a second opinion.

(We should also deprecate force_orb_pfch in the future.)

Re: [PATCH v2 1/1] vfio-ccw: allow non-prefetch ORBs

[Jared Rossi]

On 2020-05-14 11:20, Cornelia Huck wrote:
> On Tue, 12 May 2020 14:15:35 -0400
> Jared Rossi <jrossi@linux.ibm.com> wrote:
> 
>> Remove the explicit prefetch check when using vfio-ccw devices.
>> This check does not trigger in practice as all Linux channel programs
>> are intended to use prefetch.
>> 
>> It is no longer required to force the PFCH flag when using vfio-ccw
>> devices.
> 
> That's not quite true: Only kernels that include the currently-queued
> patch do not require it. Maybe
> 
> "Newer Linux kernel versions do not require to force the PFCH flag with
> vfio-ccw devices anymore."
> 
> ?
> 

This is a good point and your proposed message is reasonable.

>> 
>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>> ---
>>  hw/vfio/ccw.c | 13 +++----------
>>  1 file changed, 3 insertions(+), 10 deletions(-)
>> 
>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>> index 50cc2ec75c..e649377b68 100644
>> --- a/hw/vfio/ccw.c
>> +++ b/hw/vfio/ccw.c
>> @@ -74,16 +74,9 @@ static IOInstEnding 
>> vfio_ccw_handle_request(SubchDev *sch)
>>      struct ccw_io_region *region = vcdev->io_region;
>>      int ret;
>> 
>> -    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
>> -        if (!(vcdev->force_orb_pfch)) {
>> -            warn_once_pfch(vcdev, sch, "requires PFCH flag set");
>> -            sch_gen_unit_exception(sch);
>> -            css_inject_io_interrupt(sch);
>> -            return IOINST_CC_EXPECTED;
>> -        } else {
>> -            sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
>> -            warn_once_pfch(vcdev, sch, "PFCH flag forced");
>> -        }
>> +    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH) && 
>> vcdev->force_orb_pfch) {
>> +        sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
>> +        warn_once_pfch(vcdev, sch, "PFCH flag forced");
>>      }
>> 
>>      QEMU_BUILD_BUG_ON(sizeof(region->orb_area) != sizeof(ORB));
> 
> Let me spell out what happens:
> - PFCH bit set -> no change
> - PFCH bit not set, but force_orb_pfch set -> no change
> - neither PFCH bit nor force_orb_pfch set:
>   - older kernels: QEMU makes the request, the kernel rejects it, guest
>     gets a unit exception (same result for the guest as before, only a
>     different code flow)
>   - newer kernels: QEMU makes the request, the kernel forwards the
>     request (logging a rate-limited warning); the result depends on
>     whether the guest actually tries to rewrite the channel program or
>     not
> 

This is correct, but I think it is worth noting that while the exception
is the same in the case of new QEMU + old kernel, the logging is 
different.
The old kernel code did not issue any warning if a non-prefetch ORB was
rejected, it simply raised the exception. In reality, the old kernel 
code
path was not accessible because QEMU would always reject ORBs before 
then
with the "requires PFCH flag set" message.  The new QEMU code does not
issue a warning in this case.

I considered keeping a warning for the non-prefetch path, but it seemed
excessive to me, since it causes a redundant warning when used with the
new kernel code (which I expect to be the case normally). Do you think
some sort of warning should still be issued by QEMU in this case, even
if it is redundant with the kernel warning?

> I think that is what we want, and I think I'll queue this patch with
> the tweaked commit message, but I'd like a second opinion.
> 
> (We should also deprecate force_orb_pfch in the future.)

Re: [PATCH v2 1/1] vfio-ccw: allow non-prefetch ORBs

[Eric Farman]

On 5/14/20 2:39 PM, Jared Rossi wrote:
> On 2020-05-14 11:20, Cornelia Huck wrote:
>> On Tue, 12 May 2020 14:15:35 -0400
>> Jared Rossi <jrossi@linux.ibm.com> wrote:
>>
>>> Remove the explicit prefetch check when using vfio-ccw devices.
>>> This check does not trigger in practice as all Linux channel programs
>>> are intended to use prefetch.
>>>
>>> It is no longer required to force the PFCH flag when using vfio-ccw
>>> devices.
>>
>> That's not quite true: Only kernels that include the currently-queued
>> patch do not require it. Maybe
>>
>> "Newer Linux kernel versions do not require to force the PFCH flag with
>> vfio-ccw devices anymore."

I like it.

>>
>> ?
>>
> 
> This is a good point and your proposed message is reasonable.
> 
>>>
>>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>>> ---
>>>  hw/vfio/ccw.c | 13 +++----------
>>>  1 file changed, 3 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>>> index 50cc2ec75c..e649377b68 100644
>>> --- a/hw/vfio/ccw.c
>>> +++ b/hw/vfio/ccw.c
>>> @@ -74,16 +74,9 @@ static IOInstEnding
>>> vfio_ccw_handle_request(SubchDev *sch)
>>>      struct ccw_io_region *region = vcdev->io_region;
>>>      int ret;
>>>
>>> -    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
>>> -        if (!(vcdev->force_orb_pfch)) {
>>> -            warn_once_pfch(vcdev, sch, "requires PFCH flag set");
>>> -            sch_gen_unit_exception(sch);
>>> -            css_inject_io_interrupt(sch);
>>> -            return IOINST_CC_EXPECTED;
>>> -        } else {
>>> -            sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
>>> -            warn_once_pfch(vcdev, sch, "PFCH flag forced");
>>> -        }
>>> +    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH) &&
>>> vcdev->force_orb_pfch) {
>>> +        sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
>>> +        warn_once_pfch(vcdev, sch, "PFCH flag forced");
>>>      }
>>>
>>>      QEMU_BUILD_BUG_ON(sizeof(region->orb_area) != sizeof(ORB));
>>
>> Let me spell out what happens:
>> - PFCH bit set -> no change
>> - PFCH bit not set, but force_orb_pfch set -> no change
>> - neither PFCH bit nor force_orb_pfch set:
>>   - older kernels: QEMU makes the request, the kernel rejects it, guest
>>     gets a unit exception (same result for the guest as before, only a
>>     different code flow)
>>   - newer kernels: QEMU makes the request, the kernel forwards the
>>     request (logging a rate-limited warning); the result depends on
>>     whether the guest actually tries to rewrite the channel program or
>>     not
>>
> 
> This is correct, but I think it is worth noting that while the exception
> is the same in the case of new QEMU + old kernel, the logging is different.
> The old kernel code did not issue any warning if a non-prefetch ORB was
> rejected, it simply raised the exception. In reality, the old kernel code
> path was not accessible because QEMU would always reject ORBs before then
> with the "requires PFCH flag set" message.  The new QEMU code does not
> issue a warning in this case.
> 
> I considered keeping a warning for the non-prefetch path, but it seemed
> excessive to me, since it causes a redundant warning when used with the
> new kernel code (which I expect to be the case normally). Do you think
> some sort of warning should still be issued by QEMU in this case, even
> if it is redundant with the kernel warning?

Hrm...  Keeping the warning out of QEMU might be beneficial.  Sure, when
running with new kernels the message will be redundant, but if running
with an old kernel the result will just be a silent error.

> 
>> I think that is what we want, and I think I'll queue this patch with
>> the tweaked commit message, but I'd like a second opinion.

I don't have a strong opinion of the messaging, but think everything
else looks fine.  If you'd like to queue this patch with the tweaked
commit message:

Reviewed-by: Eric Farman <farman@linux.ibm.com>

>>
>> (We should also deprecate force_orb_pfch in the future.)

+1

Re: [PATCH v2 1/1] vfio-ccw: allow non-prefetch ORBs

[Cornelia Huck]

On Fri, 15 May 2020 07:06:29 -0400
Eric Farman <farman@linux.ibm.com> wrote:

> On 5/14/20 2:39 PM, Jared Rossi wrote:
> > On 2020-05-14 11:20, Cornelia Huck wrote:  
> >> On Tue, 12 May 2020 14:15:35 -0400
> >> Jared Rossi <jrossi@linux.ibm.com> wrote:
> >>  
> >>> Remove the explicit prefetch check when using vfio-ccw devices.
> >>> This check does not trigger in practice as all Linux channel programs
> >>> are intended to use prefetch.
> >>>
> >>> It is no longer required to force the PFCH flag when using vfio-ccw
> >>> devices.  
> >>
> >> That's not quite true: Only kernels that include the currently-queued
> >> patch do not require it. Maybe
> >>
> >> "Newer Linux kernel versions do not require to force the PFCH flag with
> >> vfio-ccw devices anymore."  
> 
> I like it.
> 
> >>
> >> ?
> >>  
> > 
> > This is a good point and your proposed message is reasonable.

I'll use it, then :)

> >   
> >>>
> >>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> >>> ---
> >>>  hw/vfio/ccw.c | 13 +++----------
> >>>  1 file changed, 3 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> >>> index 50cc2ec75c..e649377b68 100644
> >>> --- a/hw/vfio/ccw.c
> >>> +++ b/hw/vfio/ccw.c
> >>> @@ -74,16 +74,9 @@ static IOInstEnding
> >>> vfio_ccw_handle_request(SubchDev *sch)
> >>>      struct ccw_io_region *region = vcdev->io_region;
> >>>      int ret;
> >>>
> >>> -    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
> >>> -        if (!(vcdev->force_orb_pfch)) {
> >>> -            warn_once_pfch(vcdev, sch, "requires PFCH flag set");
> >>> -            sch_gen_unit_exception(sch);
> >>> -            css_inject_io_interrupt(sch);
> >>> -            return IOINST_CC_EXPECTED;
> >>> -        } else {
> >>> -            sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
> >>> -            warn_once_pfch(vcdev, sch, "PFCH flag forced");
> >>> -        }
> >>> +    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH) &&
> >>> vcdev->force_orb_pfch) {
> >>> +        sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
> >>> +        warn_once_pfch(vcdev, sch, "PFCH flag forced");
> >>>      }
> >>>
> >>>      QEMU_BUILD_BUG_ON(sizeof(region->orb_area) != sizeof(ORB));  
> >>
> >> Let me spell out what happens:
> >> - PFCH bit set -> no change
> >> - PFCH bit not set, but force_orb_pfch set -> no change
> >> - neither PFCH bit nor force_orb_pfch set:
> >>   - older kernels: QEMU makes the request, the kernel rejects it, guest
> >>     gets a unit exception (same result for the guest as before, only a
> >>     different code flow)
> >>   - newer kernels: QEMU makes the request, the kernel forwards the
> >>     request (logging a rate-limited warning); the result depends on
> >>     whether the guest actually tries to rewrite the channel program or
> >>     not
> >>  
> > 
> > This is correct, but I think it is worth noting that while the exception
> > is the same in the case of new QEMU + old kernel, the logging is different.
> > The old kernel code did not issue any warning if a non-prefetch ORB was
> > rejected, it simply raised the exception. In reality, the old kernel code
> > path was not accessible because QEMU would always reject ORBs before then
> > with the "requires PFCH flag set" message.  The new QEMU code does not
> > issue a warning in this case.
> > 
> > I considered keeping a warning for the non-prefetch path, but it seemed
> > excessive to me, since it causes a redundant warning when used with the
> > new kernel code (which I expect to be the case normally). Do you think
> > some sort of warning should still be issued by QEMU in this case, even
> > if it is redundant with the kernel warning?  
> 
> Hrm...  Keeping the warning out of QEMU might be beneficial.  Sure, when
> running with new kernels the message will be redundant, but if running
> with an old kernel the result will just be a silent error.

I don't think we need to care about that situation that much; I'd hope
that any distribution will pick both patches (or at least not the QEMU
patch without the kernel patch).

> 
> >   
> >> I think that is what we want, and I think I'll queue this patch with
> >> the tweaked commit message, but I'd like a second opinion.  
> 
> I don't have a strong opinion of the messaging, but think everything
> else looks fine.  If you'd like to queue this patch with the tweaked
> commit message:

Ok, then I'll just go ahead and queue it.

> 
> Reviewed-by: Eric Farman <farman@linux.ibm.com>
> 
> >>
> >> (We should also deprecate force_orb_pfch in the future.)  
> 
> +1
> 

Re: [PATCH v2 1/1] vfio-ccw: allow non-prefetch ORBs

[Cornelia Huck]

On Tue, 12 May 2020 14:15:35 -0400
Jared Rossi <jrossi@linux.ibm.com> wrote:

> Remove the explicit prefetch check when using vfio-ccw devices.
> This check does not trigger in practice as all Linux channel programs
> are intended to use prefetch.
> 
> It is no longer required to force the PFCH flag when using vfio-ccw
> devices.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>  hw/vfio/ccw.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)

Thanks, applied (with updated commit message.)