[RFC PATCH v1 0/8] s390x/vfio-ccw: Channel Path Handling

[RFC PATCH v1 0/8] s390x/vfio-ccw: Channel Path Handling

[Eric Farman]

Here is a first pass at the channel-path handling code for vfio-ccw,
to take advantage of the corresponding kernel patches posted here:

https://lore.kernel.org/kvm/20191115025620.19593-1-farman@linux.ibm.com/

As with the KVM patches, these were originally written by Farhan Ali
this past summer, and my git notes log the changes I've made since
picking up this work.  There are two commits at the front of this
series that seem to be pre-reqs to the actual changes for this, which
is why the linux-headers update seems lost in the middle of this.

I've tried to be mindful of testing permutations of new/old kernel with
either new/old QEMU, and it seems to be in good shape.

Eric Farman (2):
  vfio-ccw: Refactor cleanup of regions
  vfio-ccw: Refactor ccw irq handler

Farhan Ali (6):
  vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO
  vfio-ccw: Don't inject an I/O interrupt if the subchannel is not
    enabled
  linux-headers: update
  vfio-ccw: Add support for the schib region
  vfio-ccw: Add support for the crw region
  vfio-ccw: Add support for the CRW irq

 hw/s390x/css.c                 |   8 +-
 hw/s390x/s390-ccw.c            |  20 ++++
 hw/vfio/ccw.c                  | 195 +++++++++++++++++++++++++++++----
 include/hw/s390x/css.h         |   3 +-
 include/hw/s390x/s390-ccw.h    |   1 +
 linux-headers/linux/vfio.h     |   3 +
 linux-headers/linux/vfio_ccw.h |  10 ++
 target/s390x/ioinst.c          |   3 +-
 8 files changed, 217 insertions(+), 26 deletions(-)

-- 
2.17.1

[RFC PATCH v1 1/8] vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO

[Eric Farman]

From: Farhan Ali <alifm@linux.ibm.com>

EIO is returned by vfio-ccw mediated device when the backing
host subchannel is not operational anymore. So return cc=3
back to the guest, rather than returning a unit check.
This way the guest can take appropriate action such as
issue an 'stsch'.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 hw/vfio/ccw.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 6863f6c69f..0919ddbeb8 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -114,6 +114,7 @@ again:
         return IOINST_CC_BUSY;
     case -ENODEV:
     case -EACCES:
+    case -EIO:
         return IOINST_CC_NOT_OPERATIONAL;
     case -EFAULT:
     default:
-- 
2.17.1

Re: [RFC PATCH v1 1/8] vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO

[Cornelia Huck]

On Fri, 15 Nov 2019 04:34:30 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> EIO is returned by vfio-ccw mediated device when the backing
> host subchannel is not operational anymore. So return cc=3
> back to the guest, rather than returning a unit check.
> This way the guest can take appropriate action such as
> issue an 'stsch'.

Hnm, I'm trying to recall whether that was actually a conscious choice,
but I can't quite remember... the change does make sense at a glance,
however.

> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>

I would need your s-o-b for that one, though :)

> ---
>  hw/vfio/ccw.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 6863f6c69f..0919ddbeb8 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -114,6 +114,7 @@ again:
>          return IOINST_CC_BUSY;
>      case -ENODEV:
>      case -EACCES:
> +    case -EIO:
>          return IOINST_CC_NOT_OPERATIONAL;
>      case -EFAULT:
>      default:

Re: [RFC PATCH v1 1/8] vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO

[Halil Pasic]

On Mon, 18 Nov 2019 19:13:34 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> > EIO is returned by vfio-ccw mediated device when the backing
> > host subchannel is not operational anymore. So return cc=3
> > back to the guest, rather than returning a unit check.
> > This way the guest can take appropriate action such as
> > issue an 'stsch'.  
> 
> Hnm, I'm trying to recall whether that was actually a conscious choice,
> but I can't quite remember... the change does make sense at a glance,
> however.

Is EIO returned if and only if the host subchannel/device is not
operational any more, or are there cases as well? Is the mapping
(cc to condition) documented? By the QEMU code I would think that
we already have ENODEV and EACCESS for 'not operational' -- no idea
why we need two codes though.

Regards,
Halil

Re: [RFC PATCH v1 1/8] vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO

[Cornelia Huck]

On Tue, 19 Nov 2019 12:23:40 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Mon, 18 Nov 2019 19:13:34 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > > EIO is returned by vfio-ccw mediated device when the backing
> > > host subchannel is not operational anymore. So return cc=3
> > > back to the guest, rather than returning a unit check.
> > > This way the guest can take appropriate action such as
> > > issue an 'stsch'.    
> > 
> > Hnm, I'm trying to recall whether that was actually a conscious choice,
> > but I can't quite remember... the change does make sense at a glance,
> > however.  
> 
> Is EIO returned if and only if the host subchannel/device is not
> operational any more, or are there cases as well? 

Ok, I walked through the kernel code, and it seems -EIO can happen
- when we try to do I/O while in the NOT_OPER or STANDBY states... cc 3
  makes sense in those cases
- when the cp is not initialized when trying to fetch the orb... which
  is an internal vfio-ccw kernel module error

Btw., this patch only changes one of the handlers; I think you have to
change all of start/halt/clear?

[Might also be good to double-check the handling for the different
instructions.]

> Is the mapping
> (cc to condition) documented? By the QEMU code I would think that
> we already have ENODEV and EACCESS for 'not operational' -- no idea
> why we need two codes though.

-ENODEV: device gone
-EACCES: no path operational

We should be able to distinguish between the two; in the 'no path
operational' case, the device may still be accessible with a different
path mask in the request.

Re: [RFC PATCH v1 1/8] vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO

[Eric Farman]

On 11/19/19 7:02 AM, Cornelia Huck wrote:
> On Tue, 19 Nov 2019 12:23:40 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> On Mon, 18 Nov 2019 19:13:34 +0100
>> Cornelia Huck <cohuck@redhat.com> wrote:
>>
>>>> EIO is returned by vfio-ccw mediated device when the backing
>>>> host subchannel is not operational anymore. So return cc=3
>>>> back to the guest, rather than returning a unit check.
>>>> This way the guest can take appropriate action such as
>>>> issue an 'stsch'.    
>>>
>>> Hnm, I'm trying to recall whether that was actually a conscious choice,
>>> but I can't quite remember... the change does make sense at a glance,
>>> however.  
>>
>> Is EIO returned if and only if the host subchannel/device is not
>> operational any more, or are there cases as well? 
> 
> Ok, I walked through the kernel code, and it seems -EIO can happen
> - when we try to do I/O while in the NOT_OPER or STANDBY states... cc 3
>   makes sense in those cases
> - when the cp is not initialized when trying to fetch the orb... which
>   is an internal vfio-ccw kernel module error
> 
> Btw., this patch only changes one of the handlers; I think you have to
> change all of start/halt/clear?

Correct; this patch must've been written before the halt/clear handlers
landed and I missed that nuance in the rebase.  I'll fix that up...

> 
> [Might also be good to double-check the handling for the different
> instructions.]

...and do the double-checking.

> 
>> Is the mapping
>> (cc to condition) documented? By the QEMU code I would think that
>> we already have ENODEV and EACCESS for 'not operational' -- no idea
>> why we need two codes though.
> 
> -ENODEV: device gone
> -EACCES: no path operational
> 
> We should be able to distinguish between the two; in the 'no path
> operational' case, the device may still be accessible with a different
> path mask in the request.
> 

As long as we don't ignore the guest LPM.  Gotta drop that patch.  ;)

Re: [RFC PATCH v1 1/8] vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO

[Halil Pasic]

On Tue, 19 Nov 2019 13:02:20 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 19 Nov 2019 12:23:40 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Mon, 18 Nov 2019 19:13:34 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> > 
> > > > EIO is returned by vfio-ccw mediated device when the backing
> > > > host subchannel is not operational anymore. So return cc=3
> > > > back to the guest, rather than returning a unit check.
> > > > This way the guest can take appropriate action such as
> > > > issue an 'stsch'.    
> > > 
> > > Hnm, I'm trying to recall whether that was actually a conscious choice,
> > > but I can't quite remember... the change does make sense at a glance,
> > > however.  
> > 
> > Is EIO returned if and only if the host subchannel/device is not
> > operational any more, or are there cases as well? 
> 
> Ok, I walked through the kernel code, and it seems -EIO can happen

Thanks Connie for having a look.

> - when we try to do I/O while in the NOT_OPER or STANDBY states... cc 3
>   makes sense in those cases

I do understand NOT_OPER, but I'm not sure about STANDBY.

Here is what the PoP says about cc 3 for SSCH.
"""
Condition code 3 is set, and no other action is
taken, when the subchannel is not operational for
START SUBCHANNEL. A subchannel is not opera-
tional for START SUBCHANNEL if the subchannel is
not provided in the channel subsystem, has no valid
device number associated with it, or is not enabled.
"""

Are we guaranteed to reflect one of these conditions back?

Under what circumstances do we expect that our request will
find the device in STANDBY?

> - when the cp is not initialized when trying to fetch the orb... which
>   is an internal vfio-ccw kernel module error


So the answer seems to be, no EIO is also used for something else than
'device not operational' in a sense of the s390 IO architecture (cc=3
and stuff).

AFAIR the idea was that EIO means something is broken, and we decided
to reflect that as an unit check (because the broader device -- the
actual device + our pass-through code == device for the guest) is broken.
So I think it was a conscious choice.

Regards,
Halil

> 
> Btw., this patch only changes one of the handlers; I think you have to
> change all of start/halt/clear?
> 
> [Might also be good to double-check the handling for the different
> instructions.]
> 
> > Is the mapping
> > (cc to condition) documented? By the QEMU code I would think that
> > we already have ENODEV and EACCESS for 'not operational' -- no idea
> > why we need two codes though.
> 
> -ENODEV: device gone
> -EACCES: no path operational
> 
> We should be able to distinguish between the two; in the 'no path
> operational' case, the device may still be accessible with a different
> path mask in the request.
> 

Re: [RFC PATCH v1 1/8] vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO

[Cornelia Huck]

On Tue, 19 Nov 2019 18:59:11 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Tue, 19 Nov 2019 13:02:20 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Tue, 19 Nov 2019 12:23:40 +0100
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> > > On Mon, 18 Nov 2019 19:13:34 +0100
> > > Cornelia Huck <cohuck@redhat.com> wrote:
> > >   
> > > > > EIO is returned by vfio-ccw mediated device when the backing
> > > > > host subchannel is not operational anymore. So return cc=3
> > > > > back to the guest, rather than returning a unit check.
> > > > > This way the guest can take appropriate action such as
> > > > > issue an 'stsch'.      
> > > > 
> > > > Hnm, I'm trying to recall whether that was actually a conscious choice,
> > > > but I can't quite remember... the change does make sense at a glance,
> > > > however.    
> > > 
> > > Is EIO returned if and only if the host subchannel/device is not
> > > operational any more, or are there cases as well?   
> > 
> > Ok, I walked through the kernel code, and it seems -EIO can happen  
> 
> Thanks Connie for having a look.
> 
> > - when we try to do I/O while in the NOT_OPER or STANDBY states... cc 3
> >   makes sense in those cases  
> 
> I do understand NOT_OPER, but I'm not sure about STANDBY.
> 
> Here is what the PoP says about cc 3 for SSCH.
> """
> Condition code 3 is set, and no other action is
> taken, when the subchannel is not operational for
> START SUBCHANNEL. A subchannel is not opera-
> tional for START SUBCHANNEL if the subchannel is
> not provided in the channel subsystem, has no valid
> device number associated with it, or is not enabled.
> """
> 
> Are we guaranteed to reflect one of these conditions back?
> 
> Under what circumstances do we expect that our request will
> find the device in STANDBY?

IIRC, the subchannel is not enabled when the device is in STANDBY?

Anyway, it seems the check here is more like a safety measure, in case
we messed up.

> 
> > - when the cp is not initialized when trying to fetch the orb... which
> >   is an internal vfio-ccw kernel module error  
> 
> 
> So the answer seems to be, no EIO is also used for something else than
> 'device not operational' in a sense of the s390 IO architecture (cc=3
> and stuff).
> 
> AFAIR the idea was that EIO means something is broken, and we decided
> to reflect that as an unit check (because the broader device -- the
> actual device + our pass-through code == device for the guest) is broken.
> So I think it was a conscious choice.

Hm, if you put it like that... maybe leaving it as -EIO makes more sense.

The main question is: What happens if userspace triggers I/O to be
started and we find the device to have become not operational? Can we
even switch the state to NOT_OPER before we try the ssch (which will
fail with cc 3)? If not, it's probably safe to leave the -EIO in place.

Re: [RFC PATCH v1 1/8] vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO

[Eric Farman]

On 11/18/19 1:13 PM, Cornelia Huck wrote:
> On Fri, 15 Nov 2019 04:34:30 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> From: Farhan Ali <alifm@linux.ibm.com>
>>
>> EIO is returned by vfio-ccw mediated device when the backing
>> host subchannel is not operational anymore. So return cc=3
>> back to the guest, rather than returning a unit check.
>> This way the guest can take appropriate action such as
>> issue an 'stsch'.
> 
> Hnm, I'm trying to recall whether that was actually a conscious choice,
> but I can't quite remember... the change does make sense at a glance,
> however.
> 
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> 
> I would need your s-o-b for that one, though :)

Oops.  :)

> 
>> ---
>>  hw/vfio/ccw.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>> index 6863f6c69f..0919ddbeb8 100644
>> --- a/hw/vfio/ccw.c
>> +++ b/hw/vfio/ccw.c
>> @@ -114,6 +114,7 @@ again:
>>          return IOINST_CC_BUSY;
>>      case -ENODEV:
>>      case -EACCES:
>> +    case -EIO:
>>          return IOINST_CC_NOT_OPERATIONAL;
>>      case -EFAULT:
>>      default:
> 

[RFC PATCH v1 2/8] vfio-ccw: Don't inject an I/O interrupt if the subchannel is not enabled

[Eric Farman]

From: Farhan Ali <alifm@linux.ibm.com>

According to PoPs, when the SCHIB.PMCW bit 8 is 0 status presented by
the device is not made available to the program. So don't inject an
interrupt in the guest if the guest OS has not enabled the
subchannel.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---

Notes:
    v0->v1: [EF]
     - Update commit message, as the bit in question is bit 8 not 15

 hw/vfio/ccw.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 0919ddbeb8..0590a6f512 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -230,6 +230,11 @@ static void vfio_ccw_io_notifier_handler(void *opaque)
         return;
     }
 
+    /* Virtual subchannel is not enabled */
+    if (!(schib->pmcw.flags & PMCW_FLAGS_MASK_ENA)) {
+        return;
+    }
+
     size = pread(vcdev->vdev.fd, region, vcdev->io_region_size,
                  vcdev->io_region_offset);
     if (size == -1) {
-- 
2.17.1

Re: [RFC PATCH v1 2/8] vfio-ccw: Don't inject an I/O interrupt if the subchannel is not enabled

[Cornelia Huck]

On Fri, 15 Nov 2019 04:34:31 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> According to PoPs, when the SCHIB.PMCW bit 8 is 0 status presented by
> the device is not made available to the program. So don't inject an
> interrupt in the guest if the guest OS has not enabled the
> subchannel.

Have you managed to trigger this state in real life?

> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v0->v1: [EF]
>      - Update commit message, as the bit in question is bit 8 not 15
> 
>  hw/vfio/ccw.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 0919ddbeb8..0590a6f512 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -230,6 +230,11 @@ static void vfio_ccw_io_notifier_handler(void *opaque)
>          return;
>      }
>  
> +    /* Virtual subchannel is not enabled */
> +    if (!(schib->pmcw.flags & PMCW_FLAGS_MASK_ENA)) {

How can that happen? We should not be able to disable the subchannel
while it is in active use, should we? I fear I'm missing something
here...

> +        return;
> +    }
> +
>      size = pread(vcdev->vdev.fd, region, vcdev->io_region_size,
>                   vcdev->io_region_offset);
>      if (size == -1) {

Re: [RFC PATCH v1 2/8] vfio-ccw: Don't inject an I/O interrupt if the subchannel is not enabled

[Eric Farman]

On 11/18/19 1:23 PM, Cornelia Huck wrote:
> On Fri, 15 Nov 2019 04:34:31 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> From: Farhan Ali <alifm@linux.ibm.com>
>>
>> According to PoPs, when the SCHIB.PMCW bit 8 is 0 status presented by
>> the device is not made available to the program. So don't inject an
>> interrupt in the guest if the guest OS has not enabled the
>> subchannel.
> 
> Have you managed to trigger this state in real life?

No, and I have no notes on how this came to be.  I'll try running some
of the config tests with this reverted, and see if I can find any weirdness.

> 
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>
>> Notes:
>>     v0->v1: [EF]
>>      - Update commit message, as the bit in question is bit 8 not 15
>>
>>  hw/vfio/ccw.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>> index 0919ddbeb8..0590a6f512 100644
>> --- a/hw/vfio/ccw.c
>> +++ b/hw/vfio/ccw.c
>> @@ -230,6 +230,11 @@ static void vfio_ccw_io_notifier_handler(void *opaque)
>>          return;
>>      }
>>  
>> +    /* Virtual subchannel is not enabled */
>> +    if (!(schib->pmcw.flags & PMCW_FLAGS_MASK_ENA)) {
> 
> How can that happen? We should not be able to disable the subchannel
> while it is in active use, should we? I fear I'm missing something
> here...

Me too...  Hrm...

> 
>> +        return;
>> +    }
>> +
>>      size = pread(vcdev->vdev.fd, region, vcdev->io_region_size,
>>                   vcdev->io_region_offset);
>>      if (size == -1) {
> 

[RFC PATCH v1 3/8] linux-headers: update

[Eric Farman]

From: Farhan Ali <alifm@linux.ibm.com>

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---

Notes:
    v0->v1: [EF]
     - Run scripts/update-linux-headers.sh properly, but do not
       add resulting changes to linux-headers/asm-mips/

 linux-headers/linux/vfio.h     |  3 +++
 linux-headers/linux/vfio_ccw.h | 10 ++++++++++
 2 files changed, 13 insertions(+)

diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index fb10370d29..9e227348b3 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -378,6 +378,8 @@ struct vfio_region_gfx_edid {
 
 /* sub-types for VFIO_REGION_TYPE_CCW */
 #define VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD	(1)
+#define VFIO_REGION_SUBTYPE_CCW_SCHIB		(2)
+#define VFIO_REGION_SUBTYPE_CCW_CRW		(3)
 
 /*
  * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped
@@ -577,6 +579,7 @@ enum {
 
 enum {
 	VFIO_CCW_IO_IRQ_INDEX,
+	VFIO_CCW_CRW_IRQ_INDEX,
 	VFIO_CCW_NUM_IRQS
 };
 
diff --git a/linux-headers/linux/vfio_ccw.h b/linux-headers/linux/vfio_ccw.h
index fcc3e69ef5..8654c9afca 100644
--- a/linux-headers/linux/vfio_ccw.h
+++ b/linux-headers/linux/vfio_ccw.h
@@ -34,4 +34,14 @@ struct ccw_cmd_region {
 	__u32 ret_code;
 } __attribute__((packed));
 
+struct ccw_schib_region {
+#define SCHIB_AREA_SIZE 52
+	__u8 schib_area[SCHIB_AREA_SIZE];
+} __attribute__((packed));
+
+struct ccw_crw_region {
+	__u32 crw0;
+	__u32 crw1;
+} __attribute__((packed));
+
 #endif
-- 
2.17.1

[RFC PATCH v1 4/8] vfio-ccw: Refactor cleanup of regions

[Eric Farman]

While we're at it, add a g_free() for the async_cmd_region that
is the last thing currently created.  g_free() knows how to handle
NULL pointers, so this makes it easier to remember what cleanups
need to be performed when new regions are added.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/vfio/ccw.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 0590a6f512..3e32bc1819 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -376,8 +376,7 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp)
     vcdev->io_region_size = info->size;
     if (sizeof(*vcdev->io_region) != vcdev->io_region_size) {
         error_setg(errp, "vfio: Unexpected size of the I/O region");
-        g_free(info);
-        return;
+        goto out_err;
     }
 
     vcdev->io_region_offset = info->offset;
@@ -390,15 +389,20 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp)
         vcdev->async_cmd_region_size = info->size;
         if (sizeof(*vcdev->async_cmd_region) != vcdev->async_cmd_region_size) {
             error_setg(errp, "vfio: Unexpected size of the async cmd region");
-            g_free(vcdev->io_region);
-            g_free(info);
-            return;
+            goto out_err;
         }
         vcdev->async_cmd_region_offset = info->offset;
         vcdev->async_cmd_region = g_malloc0(info->size);
     }
 
     g_free(info);
+    return;
+
+out_err:
+    g_free(vcdev->async_cmd_region);
+    g_free(vcdev->io_region);
+    g_free(info);
+    return;
 }
 
 static void vfio_ccw_put_region(VFIOCCWDevice *vcdev)
-- 
2.17.1

Re: [RFC PATCH v1 4/8] vfio-ccw: Refactor cleanup of regions

[Cornelia Huck]

On Fri, 15 Nov 2019 04:34:33 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> While we're at it, add a g_free() for the async_cmd_region that
> is the last thing currently created.  g_free() knows how to handle
> NULL pointers, so this makes it easier to remember what cleanups
> need to be performed when new regions are added.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  hw/vfio/ccw.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[RFC PATCH v1 5/8] vfio-ccw: Add support for the schib region

[Eric Farman]

From: Farhan Ali <alifm@linux.ibm.com>

The schib region can be used to obtain the latest SCHIB from the host
passthrough subchannel. Since the guest SCHIB is virtualized,
we currently only update the path related information so that the
guest is aware of any path related changes when it issues the
'stsch' instruction.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---

Notes:
    v0->v1: [EF]
     - Change various incarnations of "update chp status" to
       "handle_store", to reflect the STSCH instruction that will
       drive this code
     - Remove temporary variable for casting/testing purposes in
       s390_ccw_store(), and add a block comment of WHY its there.
     - Add a few comments to vfio_ccw_handle_store()

 hw/s390x/css.c              |  8 ++++--
 hw/s390x/s390-ccw.c         | 20 +++++++++++++
 hw/vfio/ccw.c               | 57 +++++++++++++++++++++++++++++++++++++
 include/hw/s390x/css.h      |  3 +-
 include/hw/s390x/s390-ccw.h |  1 +
 target/s390x/ioinst.c       |  3 +-
 6 files changed, 87 insertions(+), 5 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 844caab408..6ee6a96d75 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1335,11 +1335,15 @@ static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src)
     }
 }
 
-int css_do_stsch(SubchDev *sch, SCHIB *schib)
+IOInstEnding css_do_stsch(SubchDev *sch, SCHIB *schib)
 {
+    int ret = IOINST_CC_EXPECTED;
+
+    ret = s390_ccw_store(sch);
+
     /* Use current status. */
     copy_schib_to_guest(schib, &sch->curr_status);
-    return 0;
+    return ret;
 }
 
 static void copy_pmcw_from_guest(PMCW *dest, const PMCW *src)
diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index 0c5a5b60bd..be0b379338 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -51,6 +51,26 @@ int s390_ccw_clear(SubchDev *sch)
     return cdc->handle_clear(sch);
 }
 
+IOInstEnding s390_ccw_store(SubchDev *sch)
+{
+    S390CCWDeviceClass *cdc = NULL;
+    int ret = IOINST_CC_EXPECTED;
+
+    /*
+     * This only applies to passthrough devices, so we can't unconditionally
+     * set this variable like we would for halt/clear.
+     */
+    if (object_dynamic_cast(OBJECT(sch->driver_data), TYPE_S390_CCW)) {
+        cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data);
+    }
+
+    if (cdc && cdc->handle_store) {
+        ret = cdc->handle_store(sch);
+    }
+
+    return ret;
+}
+
 static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
                                   char *sysfsdev,
                                   Error **errp)
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 3e32bc1819..2ff223470f 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -41,6 +41,9 @@ struct VFIOCCWDevice {
     uint64_t async_cmd_region_size;
     uint64_t async_cmd_region_offset;
     struct ccw_cmd_region *async_cmd_region;
+    uint64_t schib_region_size;
+    uint64_t schib_region_offset;
+    struct ccw_schib_region *schib_region;
     EventNotifier io_notifier;
     bool force_orb_pfch;
     bool warned_orb_pfch;
@@ -124,6 +127,45 @@ again:
     }
 }
 
+static IOInstEnding vfio_ccw_handle_store(SubchDev *sch)
+{
+    S390CCWDevice *cdev = sch->driver_data;
+    VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
+    SCHIB *schib = &sch->curr_status;
+    struct ccw_schib_region *region = vcdev->schib_region;
+    SCHIB *s = (SCHIB *)region->schib_area;
+    int ret;
+
+    /* schib region not available so nothing else to do */
+    if (!region) {
+        return IOINST_CC_EXPECTED;
+    }
+
+    memset(region, 0, sizeof(*region));
+    ret = pread(vcdev->vdev.fd, region, vcdev->schib_region_size,
+                vcdev->schib_region_offset);
+
+    if (ret == -1) {
+        /* Assume device is damaged, regardless of errno */
+        return IOINST_CC_NOT_OPERATIONAL;
+    }
+
+    /*
+     * Selectively copy path-related bits of the SCHIB,
+     * rather than copying the entire struct.
+     */
+    schib->pmcw.pnom = s->pmcw.pnom;
+    schib->pmcw.lpum = s->pmcw.lpum;
+    schib->pmcw.pam = s->pmcw.pam;
+    schib->pmcw.pom = s->pmcw.pom;
+
+    if (s->scsw.flags & SCSW_FLAGS_MASK_PNO) {
+        schib->scsw.flags |= SCSW_FLAGS_MASK_PNO;
+    }
+
+    return IOINST_CC_EXPECTED;
+}
+
 static int vfio_ccw_handle_clear(SubchDev *sch)
 {
     S390CCWDevice *cdev = sch->driver_data;
@@ -395,10 +437,23 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp)
         vcdev->async_cmd_region = g_malloc0(info->size);
     }
 
+    ret = vfio_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW,
+                                   VFIO_REGION_SUBTYPE_CCW_SCHIB, &info);
+    if (!ret) {
+        vcdev->schib_region_size = info->size;
+        if (sizeof(*vcdev->schib_region) != vcdev->schib_region_size) {
+            error_setg(errp, "vfio: Unexpected size of the schib region");
+            goto out_err;
+        }
+        vcdev->schib_region_offset = info->offset;
+        vcdev->schib_region = g_malloc(info->size);
+    }
+
     g_free(info);
     return;
 
 out_err:
+    g_free(vcdev->schib_region);
     g_free(vcdev->async_cmd_region);
     g_free(vcdev->io_region);
     g_free(info);
@@ -407,6 +462,7 @@ out_err:
 
 static void vfio_ccw_put_region(VFIOCCWDevice *vcdev)
 {
+    g_free(vcdev->schib_region);
     g_free(vcdev->async_cmd_region);
     g_free(vcdev->io_region);
 }
@@ -582,6 +638,7 @@ static void vfio_ccw_class_init(ObjectClass *klass, void *data)
     cdc->handle_request = vfio_ccw_handle_request;
     cdc->handle_halt = vfio_ccw_handle_halt;
     cdc->handle_clear = vfio_ccw_handle_clear;
+    cdc->handle_store = vfio_ccw_handle_store;
 }
 
 static const TypeInfo vfio_ccw_info = {
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index f46bcafb16..7e3a5e7433 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -218,6 +218,7 @@ IOInstEnding do_subchannel_work_passthrough(SubchDev *sub);
 
 int s390_ccw_halt(SubchDev *sch);
 int s390_ccw_clear(SubchDev *sch);
+IOInstEnding s390_ccw_store(SubchDev *sch);
 
 typedef enum {
     CSS_IO_ADAPTER_VIRTIO = 0,
@@ -242,7 +243,7 @@ SubchDev *css_find_subch(uint8_t m, uint8_t cssid, uint8_t ssid,
                          uint16_t schid);
 bool css_subch_visible(SubchDev *sch);
 void css_conditional_io_interrupt(SubchDev *sch);
-int css_do_stsch(SubchDev *sch, SCHIB *schib);
+IOInstEnding css_do_stsch(SubchDev *sch, SCHIB *schib);
 bool css_schid_final(int m, uint8_t cssid, uint8_t ssid, uint16_t schid);
 IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *schib);
 IOInstEnding css_do_xsch(SubchDev *sch);
diff --git a/include/hw/s390x/s390-ccw.h b/include/hw/s390x/s390-ccw.h
index fffb54562f..4a43803ef2 100644
--- a/include/hw/s390x/s390-ccw.h
+++ b/include/hw/s390x/s390-ccw.h
@@ -37,6 +37,7 @@ typedef struct S390CCWDeviceClass {
     IOInstEnding (*handle_request) (SubchDev *sch);
     int (*handle_halt) (SubchDev *sch);
     int (*handle_clear) (SubchDev *sch);
+    IOInstEnding (*handle_store) (SubchDev *sch);
 } S390CCWDeviceClass;
 
 #endif
diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index c437a1d8c6..63b4c84215 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -257,8 +257,7 @@ void ioinst_handle_stsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb,
     sch = css_find_subch(m, cssid, ssid, schid);
     if (sch) {
         if (css_subch_visible(sch)) {
-            css_do_stsch(sch, &schib);
-            cc = 0;
+            cc = css_do_stsch(sch, &schib);
         } else {
             /* Indicate no more subchannels in this css/ss */
             cc = 3;
-- 
2.17.1

Re: [RFC PATCH v1 5/8] vfio-ccw: Add support for the schib region

[Cornelia Huck]

On Fri, 15 Nov 2019 04:34:34 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> The schib region can be used to obtain the latest SCHIB from the host
> passthrough subchannel. Since the guest SCHIB is virtualized,
> we currently only update the path related information so that the
> guest is aware of any path related changes when it issues the
> 'stsch' instruction.

One important information here is that you tweak the generic stsch
handling, although the behaviour remains the same for non-vfio
subchannels.

> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v0->v1: [EF]
>      - Change various incarnations of "update chp status" to
>        "handle_store", to reflect the STSCH instruction that will
>        drive this code
>      - Remove temporary variable for casting/testing purposes in
>        s390_ccw_store(), and add a block comment of WHY its there.
>      - Add a few comments to vfio_ccw_handle_store()
> 
>  hw/s390x/css.c              |  8 ++++--
>  hw/s390x/s390-ccw.c         | 20 +++++++++++++
>  hw/vfio/ccw.c               | 57 +++++++++++++++++++++++++++++++++++++
>  include/hw/s390x/css.h      |  3 +-
>  include/hw/s390x/s390-ccw.h |  1 +
>  target/s390x/ioinst.c       |  3 +-
>  6 files changed, 87 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 844caab408..6ee6a96d75 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -1335,11 +1335,15 @@ static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src)
>      }
>  }
>  
> -int css_do_stsch(SubchDev *sch, SCHIB *schib)
> +IOInstEnding css_do_stsch(SubchDev *sch, SCHIB *schib)
>  {
> +    int ret = IOINST_CC_EXPECTED;

It's a bit useless initializing ret here, given that you use it right
below :)

> +

Maybe add a comment here:

       /*
        * For some subchannels, we may want to update parts of
        * the schib (e.g. update path masks from the host device
        * for passthrough subchannels).
        */

> +    ret = s390_ccw_store(sch);
> +
>      /* Use current status. */
>      copy_schib_to_guest(schib, &sch->curr_status);
> -    return 0;
> +    return ret;
>  }
>  
>  static void copy_pmcw_from_guest(PMCW *dest, const PMCW *src)
> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
> index 0c5a5b60bd..be0b379338 100644
> --- a/hw/s390x/s390-ccw.c
> +++ b/hw/s390x/s390-ccw.c
> @@ -51,6 +51,26 @@ int s390_ccw_clear(SubchDev *sch)
>      return cdc->handle_clear(sch);
>  }
>  
> +IOInstEnding s390_ccw_store(SubchDev *sch)
> +{
> +    S390CCWDeviceClass *cdc = NULL;
> +    int ret = IOINST_CC_EXPECTED;
> +
> +    /*
> +     * This only applies to passthrough devices, so we can't unconditionally
> +     * set this variable like we would for halt/clear.

Hm, can we maybe handle this differently? We have a generic ccw_cb in
the subchannel structure for ccw interpretation; would it make sense to
add a generic callback for stsch there as well?

(This works fine, though. Might want to add the check for halt/clear as
well, but that might be a bit overkill.)

> +     */
> +    if (object_dynamic_cast(OBJECT(sch->driver_data), TYPE_S390_CCW)) {
> +        cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data);
> +    }
> +
> +    if (cdc && cdc->handle_store) {
> +        ret = cdc->handle_store(sch);
> +    }
> +
> +    return ret;
> +}
> +
>  static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
>                                    char *sysfsdev,
>                                    Error **errp)
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 3e32bc1819..2ff223470f 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -41,6 +41,9 @@ struct VFIOCCWDevice {
>      uint64_t async_cmd_region_size;
>      uint64_t async_cmd_region_offset;
>      struct ccw_cmd_region *async_cmd_region;
> +    uint64_t schib_region_size;
> +    uint64_t schib_region_offset;
> +    struct ccw_schib_region *schib_region;
>      EventNotifier io_notifier;
>      bool force_orb_pfch;
>      bool warned_orb_pfch;
> @@ -124,6 +127,45 @@ again:
>      }
>  }
>  
> +static IOInstEnding vfio_ccw_handle_store(SubchDev *sch)
> +{
> +    S390CCWDevice *cdev = sch->driver_data;
> +    VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
> +    SCHIB *schib = &sch->curr_status;
> +    struct ccw_schib_region *region = vcdev->schib_region;
> +    SCHIB *s = (SCHIB *)region->schib_area;

You probably don't want to access region until after you checked it's
not NULL :)

> +    int ret;
> +
> +    /* schib region not available so nothing else to do */
> +    if (!region) {
> +        return IOINST_CC_EXPECTED;
> +    }
> +
> +    memset(region, 0, sizeof(*region));
> +    ret = pread(vcdev->vdev.fd, region, vcdev->schib_region_size,
> +                vcdev->schib_region_offset);
> +
> +    if (ret == -1) {
> +        /* Assume device is damaged, regardless of errno */
> +        return IOINST_CC_NOT_OPERATIONAL;

Not sure that's correct; cc 3 for stsch indicates that there are no
more subchannels with higher ids?

> +    }
> +
> +    /*
> +     * Selectively copy path-related bits of the SCHIB,
> +     * rather than copying the entire struct.
> +     */
> +    schib->pmcw.pnom = s->pmcw.pnom;
> +    schib->pmcw.lpum = s->pmcw.lpum;
> +    schib->pmcw.pam = s->pmcw.pam;
> +    schib->pmcw.pom = s->pmcw.pom;
> +
> +    if (s->scsw.flags & SCSW_FLAGS_MASK_PNO) {
> +        schib->scsw.flags |= SCSW_FLAGS_MASK_PNO;
> +    }
> +
> +    return IOINST_CC_EXPECTED;
> +}
> +

The rest of the patch looks good to me.

Re: [RFC PATCH v1 5/8] vfio-ccw: Add support for the schib region

[Eric Farman]

[Had two un-addressed QEMU comments on my todo list; let's do that now.]

On 11/20/19 6:13 AM, Cornelia Huck wrote:
> On Fri, 15 Nov 2019 04:34:34 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> From: Farhan Ali <alifm@linux.ibm.com>
>>
>> The schib region can be used to obtain the latest SCHIB from the host
>> passthrough subchannel. Since the guest SCHIB is virtualized,
>> we currently only update the path related information so that the
>> guest is aware of any path related changes when it issues the
>> 'stsch' instruction.
> 
> One important information here is that you tweak the generic stsch
> handling, although the behaviour remains the same for non-vfio
> subchannels.
> 
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>
>> Notes:
>>     v0->v1: [EF]
>>      - Change various incarnations of "update chp status" to
>>        "handle_store", to reflect the STSCH instruction that will
>>        drive this code
>>      - Remove temporary variable for casting/testing purposes in
>>        s390_ccw_store(), and add a block comment of WHY its there.
>>      - Add a few comments to vfio_ccw_handle_store()
>>
>>  hw/s390x/css.c              |  8 ++++--
>>  hw/s390x/s390-ccw.c         | 20 +++++++++++++
>>  hw/vfio/ccw.c               | 57 +++++++++++++++++++++++++++++++++++++
>>  include/hw/s390x/css.h      |  3 +-
>>  include/hw/s390x/s390-ccw.h |  1 +
>>  target/s390x/ioinst.c       |  3 +-
>>  6 files changed, 87 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 844caab408..6ee6a96d75 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -1335,11 +1335,15 @@ static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src)
>>      }
>>  }
>>  
>> -int css_do_stsch(SubchDev *sch, SCHIB *schib)
>> +IOInstEnding css_do_stsch(SubchDev *sch, SCHIB *schib)
>>  {
>> +    int ret = IOINST_CC_EXPECTED;
> 
> It's a bit useless initializing ret here, given that you use it right
> below :)
> 
>> +
> 
> Maybe add a comment here:
> 
>        /*
>         * For some subchannels, we may want to update parts of
>         * the schib (e.g. update path masks from the host device
>         * for passthrough subchannels).
>         */
> 
>> +    ret = s390_ccw_store(sch);
>> +
>>      /* Use current status. */
>>      copy_schib_to_guest(schib, &sch->curr_status);
>> -    return 0;
>> +    return ret;
>>  }
>>  
>>  static void copy_pmcw_from_guest(PMCW *dest, const PMCW *src)
>> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
>> index 0c5a5b60bd..be0b379338 100644
>> --- a/hw/s390x/s390-ccw.c
>> +++ b/hw/s390x/s390-ccw.c
>> @@ -51,6 +51,26 @@ int s390_ccw_clear(SubchDev *sch)
>>      return cdc->handle_clear(sch);
>>  }
>>  
>> +IOInstEnding s390_ccw_store(SubchDev *sch)
>> +{
>> +    S390CCWDeviceClass *cdc = NULL;
>> +    int ret = IOINST_CC_EXPECTED;
>> +
>> +    /*
>> +     * This only applies to passthrough devices, so we can't unconditionally
>> +     * set this variable like we would for halt/clear.
> 
> Hm, can we maybe handle this differently? We have a generic ccw_cb in
> the subchannel structure for ccw interpretation; would it make sense to
> add a generic callback for stsch there as well?

Might make things a little cleaner, but I am not sure.  I'm going to
leave a todo here while we sort out the rest of the kernel code, so I
can try to put something more beneficial together.

> 
> (This works fine, though. Might want to add the check for halt/clear as
> well, but that might be a bit overkill.)
> 
>> +     */
>> +    if (object_dynamic_cast(OBJECT(sch->driver_data), TYPE_S390_CCW)) {
>> +        cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data);
>> +    }
>> +
>> +    if (cdc && cdc->handle_store) {
>> +        ret = cdc->handle_store(sch);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>  static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
>>                                    char *sysfsdev,
>>                                    Error **errp)
>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>> index 3e32bc1819..2ff223470f 100644
>> --- a/hw/vfio/ccw.c
>> +++ b/hw/vfio/ccw.c
>> @@ -41,6 +41,9 @@ struct VFIOCCWDevice {
>>      uint64_t async_cmd_region_size;
>>      uint64_t async_cmd_region_offset;
>>      struct ccw_cmd_region *async_cmd_region;
>> +    uint64_t schib_region_size;
>> +    uint64_t schib_region_offset;
>> +    struct ccw_schib_region *schib_region;
>>      EventNotifier io_notifier;
>>      bool force_orb_pfch;
>>      bool warned_orb_pfch;
>> @@ -124,6 +127,45 @@ again:
>>      }
>>  }
>>  
>> +static IOInstEnding vfio_ccw_handle_store(SubchDev *sch)
>> +{
>> +    S390CCWDevice *cdev = sch->driver_data;
>> +    VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
>> +    SCHIB *schib = &sch->curr_status;
>> +    struct ccw_schib_region *region = vcdev->schib_region;
>> +    SCHIB *s = (SCHIB *)region->schib_area;
> 
> You probably don't want to access region until after you checked it's
> not NULL :)

:)

> 
>> +    int ret;
>> +
>> +    /* schib region not available so nothing else to do */
>> +    if (!region) {
>> +        return IOINST_CC_EXPECTED;
>> +    }
>> +
>> +    memset(region, 0, sizeof(*region));
>> +    ret = pread(vcdev->vdev.fd, region, vcdev->schib_region_size,
>> +                vcdev->schib_region_offset);
>> +
>> +    if (ret == -1) {
>> +        /* Assume device is damaged, regardless of errno */
>> +        return IOINST_CC_NOT_OPERATIONAL;
> 
> Not sure that's correct; cc 3 for stsch indicates that there are no
> more subchannels with higher ids?

POPS says cc=3 on stsch if the subchannel is not operational, and it's
not operational if it's not provided to the channel subsystem.  So,
basically yes.  I guess his idea was to call out a problem on the kvm
side, so should we just return CC_EXPECTED here, as if there's no region?

> 
>> +    }
>> +
>> +    /*
>> +     * Selectively copy path-related bits of the SCHIB,
>> +     * rather than copying the entire struct.
>> +     */
>> +    schib->pmcw.pnom = s->pmcw.pnom;
>> +    schib->pmcw.lpum = s->pmcw.lpum;
>> +    schib->pmcw.pam = s->pmcw.pam;
>> +    schib->pmcw.pom = s->pmcw.pom;
>> +
>> +    if (s->scsw.flags & SCSW_FLAGS_MASK_PNO) {
>> +        schib->scsw.flags |= SCSW_FLAGS_MASK_PNO;
>> +    }
>> +
>> +    return IOINST_CC_EXPECTED;
>> +}
>> +
> 
> The rest of the patch looks good to me.
> 

Re: [RFC PATCH v1 5/8] vfio-ccw: Add support for the schib region

[Cornelia Huck]

On Fri, 31 Jan 2020 15:15:00 -0500
Eric Farman <farman@linux.ibm.com> wrote:

> [Had two un-addressed QEMU comments on my todo list; let's do that now.]
> 
> On 11/20/19 6:13 AM, Cornelia Huck wrote:
> > On Fri, 15 Nov 2019 04:34:34 +0100
> > Eric Farman <farman@linux.ibm.com> wrote:

Oh, that was some time ago... hope I can still recall what I was
thinking back then.

> >   
> >> From: Farhan Ali <alifm@linux.ibm.com>
> >>
> >> The schib region can be used to obtain the latest SCHIB from the host
> >> passthrough subchannel. Since the guest SCHIB is virtualized,
> >> we currently only update the path related information so that the
> >> guest is aware of any path related changes when it issues the
> >> 'stsch' instruction.  
> > 
> > One important information here is that you tweak the generic stsch
> > handling, although the behaviour remains the same for non-vfio
> > subchannels.
> >   
> >>
> >> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> >> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> >> ---
> >>
> >> Notes:
> >>     v0->v1: [EF]
> >>      - Change various incarnations of "update chp status" to
> >>        "handle_store", to reflect the STSCH instruction that will
> >>        drive this code
> >>      - Remove temporary variable for casting/testing purposes in
> >>        s390_ccw_store(), and add a block comment of WHY its there.
> >>      - Add a few comments to vfio_ccw_handle_store()
> >>
> >>  hw/s390x/css.c              |  8 ++++--
> >>  hw/s390x/s390-ccw.c         | 20 +++++++++++++
> >>  hw/vfio/ccw.c               | 57 +++++++++++++++++++++++++++++++++++++
> >>  include/hw/s390x/css.h      |  3 +-
> >>  include/hw/s390x/s390-ccw.h |  1 +
> >>  target/s390x/ioinst.c       |  3 +-
> >>  6 files changed, 87 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >> index 844caab408..6ee6a96d75 100644
> >> --- a/hw/s390x/css.c
> >> +++ b/hw/s390x/css.c
> >> @@ -1335,11 +1335,15 @@ static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src)
> >>      }
> >>  }
> >>  
> >> -int css_do_stsch(SubchDev *sch, SCHIB *schib)
> >> +IOInstEnding css_do_stsch(SubchDev *sch, SCHIB *schib)
> >>  {
> >> +    int ret = IOINST_CC_EXPECTED;  
> > 
> > It's a bit useless initializing ret here, given that you use it right
> > below :)
> >   
> >> +  
> > 
> > Maybe add a comment here:
> > 
> >        /*
> >         * For some subchannels, we may want to update parts of
> >         * the schib (e.g. update path masks from the host device
> >         * for passthrough subchannels).
> >         */
> >   
> >> +    ret = s390_ccw_store(sch);
> >> +
> >>      /* Use current status. */
> >>      copy_schib_to_guest(schib, &sch->curr_status);
> >> -    return 0;
> >> +    return ret;
> >>  }
> >>  
> >>  static void copy_pmcw_from_guest(PMCW *dest, const PMCW *src)
> >> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
> >> index 0c5a5b60bd..be0b379338 100644
> >> --- a/hw/s390x/s390-ccw.c
> >> +++ b/hw/s390x/s390-ccw.c
> >> @@ -51,6 +51,26 @@ int s390_ccw_clear(SubchDev *sch)
> >>      return cdc->handle_clear(sch);
> >>  }
> >>  
> >> +IOInstEnding s390_ccw_store(SubchDev *sch)
> >> +{
> >> +    S390CCWDeviceClass *cdc = NULL;
> >> +    int ret = IOINST_CC_EXPECTED;
> >> +
> >> +    /*
> >> +     * This only applies to passthrough devices, so we can't unconditionally
> >> +     * set this variable like we would for halt/clear.  
> > 
> > Hm, can we maybe handle this differently? We have a generic ccw_cb in
> > the subchannel structure for ccw interpretation; would it make sense to
> > add a generic callback for stsch there as well?  
> 
> Might make things a little cleaner, but I am not sure.  I'm going to
> leave a todo here while we sort out the rest of the kernel code, so I
> can try to put something more beneficial together.

Yes, this looks like something that can be sorted out later.

What bothers me most here, I guess, is that it is not very obvious why
the functions above can assume the presence of the cdc object, while
this one can't. The secret sauce is in the different
do_subchannel_work_* callbacks, which reside in another file... maybe
amend the comment here to mention that? That would help and doesn't
involve code refactoring :)

> 
> > 
> > (This works fine, though. Might want to add the check for halt/clear as
> > well, but that might be a bit overkill.)
> >   
> >> +     */
> >> +    if (object_dynamic_cast(OBJECT(sch->driver_data), TYPE_S390_CCW)) {
> >> +        cdc = S390_CCW_DEVICE_GET_CLASS(sch->driver_data);
> >> +    }
> >> +
> >> +    if (cdc && cdc->handle_store) {
> >> +        ret = cdc->handle_store(sch);
> >> +    }
> >> +
> >> +    return ret;
> >> +}
> >> +
> >>  static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
> >>                                    char *sysfsdev,
> >>                                    Error **errp)
> >> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> >> index 3e32bc1819..2ff223470f 100644
> >> --- a/hw/vfio/ccw.c
> >> +++ b/hw/vfio/ccw.c
> >> @@ -41,6 +41,9 @@ struct VFIOCCWDevice {
> >>      uint64_t async_cmd_region_size;
> >>      uint64_t async_cmd_region_offset;
> >>      struct ccw_cmd_region *async_cmd_region;
> >> +    uint64_t schib_region_size;
> >> +    uint64_t schib_region_offset;
> >> +    struct ccw_schib_region *schib_region;
> >>      EventNotifier io_notifier;
> >>      bool force_orb_pfch;
> >>      bool warned_orb_pfch;
> >> @@ -124,6 +127,45 @@ again:
> >>      }
> >>  }
> >>  
> >> +static IOInstEnding vfio_ccw_handle_store(SubchDev *sch)
> >> +{
> >> +    S390CCWDevice *cdev = sch->driver_data;
> >> +    VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
> >> +    SCHIB *schib = &sch->curr_status;
> >> +    struct ccw_schib_region *region = vcdev->schib_region;
> >> +    SCHIB *s = (SCHIB *)region->schib_area;  
> > 
> > You probably don't want to access region until after you checked it's
> > not NULL :)  
> 
> :)
> 
> >   
> >> +    int ret;
> >> +
> >> +    /* schib region not available so nothing else to do */
> >> +    if (!region) {
> >> +        return IOINST_CC_EXPECTED;
> >> +    }
> >> +
> >> +    memset(region, 0, sizeof(*region));
> >> +    ret = pread(vcdev->vdev.fd, region, vcdev->schib_region_size,
> >> +                vcdev->schib_region_offset);
> >> +
> >> +    if (ret == -1) {
> >> +        /* Assume device is damaged, regardless of errno */
> >> +        return IOINST_CC_NOT_OPERATIONAL;  
> > 
> > Not sure that's correct; cc 3 for stsch indicates that there are no
> > more subchannels with higher ids?  
> 
> POPS says cc=3 on stsch if the subchannel is not operational, and it's
> not operational if it's not provided to the channel subsystem.  So,
> basically yes.  I guess his idea was to call out a problem on the kvm
> side, so should we just return CC_EXPECTED here, as if there's no region?

Yes, that looks like the better choice (out of the two possible
condition codes...) And also log a moan somewhere?

> 
> >   
> >> +    }
> >> +
> >> +    /*
> >> +     * Selectively copy path-related bits of the SCHIB,
> >> +     * rather than copying the entire struct.
> >> +     */
> >> +    schib->pmcw.pnom = s->pmcw.pnom;
> >> +    schib->pmcw.lpum = s->pmcw.lpum;
> >> +    schib->pmcw.pam = s->pmcw.pam;
> >> +    schib->pmcw.pom = s->pmcw.pom;
> >> +
> >> +    if (s->scsw.flags & SCSW_FLAGS_MASK_PNO) {
> >> +        schib->scsw.flags |= SCSW_FLAGS_MASK_PNO;
> >> +    }
> >> +
> >> +    return IOINST_CC_EXPECTED;
> >> +}
> >> +  
> > 
> > The rest of the patch looks good to me.
> >   
> 

[RFC PATCH v1 6/8] vfio-ccw: Add support for the crw region

[Eric Farman]

From: Farhan Ali <alifm@linux.ibm.com>

The crw region can be used to obtain information about
Channel Report Words (CRW) from vfio-ccw driver.

Currently only channel path related CRWs are passed to
QEMU from vfio-ccw driver.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---

Notes:
    v0->v1: [EF]
     - Fixed copy/paste error in error message (s/schib/CRW)

 hw/vfio/ccw.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 2ff223470f..2b1a83b94c 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -44,6 +44,9 @@ struct VFIOCCWDevice {
     uint64_t schib_region_size;
     uint64_t schib_region_offset;
     struct ccw_schib_region *schib_region;
+    uint64_t crw_region_size;
+    uint64_t crw_region_offset;
+    struct ccw_crw_region *crw_region;
     EventNotifier io_notifier;
     bool force_orb_pfch;
     bool warned_orb_pfch;
@@ -449,10 +452,24 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp)
         vcdev->schib_region = g_malloc(info->size);
     }
 
+    ret = vfio_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW,
+                                   VFIO_REGION_SUBTYPE_CCW_CRW, &info);
+
+    if (!ret) {
+        vcdev->crw_region_size = info->size;
+        if (sizeof(*vcdev->crw_region) != vcdev->crw_region_size) {
+            error_setg(errp, "vfio: Unexpected size of the CRW region");
+            goto out_err;
+        }
+        vcdev->crw_region_offset = info->offset;
+        vcdev->crw_region = g_malloc(info->size);
+    }
+
     g_free(info);
     return;
 
 out_err:
+    g_free(vcdev->crw_region);
     g_free(vcdev->schib_region);
     g_free(vcdev->async_cmd_region);
     g_free(vcdev->io_region);
@@ -462,6 +479,7 @@ out_err:
 
 static void vfio_ccw_put_region(VFIOCCWDevice *vcdev)
 {
+    g_free(vcdev->crw_region);
     g_free(vcdev->schib_region);
     g_free(vcdev->async_cmd_region);
     g_free(vcdev->io_region);
-- 
2.17.1

Re: [RFC PATCH v1 6/8] vfio-ccw: Add support for the crw region

[Cornelia Huck]

On Fri, 15 Nov 2019 04:34:35 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> The crw region can be used to obtain information about
> Channel Report Words (CRW) from vfio-ccw driver.
> 
> Currently only channel path related CRWs are passed to
> QEMU from vfio-ccw driver.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v0->v1: [EF]
>      - Fixed copy/paste error in error message (s/schib/CRW)
> 
>  hw/vfio/ccw.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)

Looks sane.

[RFC PATCH v1 7/8] vfio-ccw: Refactor ccw irq handler

[Eric Farman]

Make it easier to add new ones in the future.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 hw/vfio/ccw.c | 55 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 16 deletions(-)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 2b1a83b94c..b16526d5de 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -334,22 +334,35 @@ read_err:
     css_inject_io_interrupt(sch);
 }
 
-static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
+static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev, int irq,
+                                           Error **errp)
 {
     VFIODevice *vdev = &vcdev->vdev;
     struct vfio_irq_info *irq_info;
     size_t argsz;
     int fd;
+    EventNotifier *notifier;
+    IOHandler *fd_read;
+
+    switch (irq) {
+    case VFIO_CCW_IO_IRQ_INDEX:
+        notifier = &vcdev->io_notifier;
+        fd_read = vfio_ccw_io_notifier_handler;
+        break;
+    default:
+        error_setg(errp, "vfio: Unsupported device irq(%d) fd: %m", irq);
+        return;
+    }
 
-    if (vdev->num_irqs < VFIO_CCW_IO_IRQ_INDEX + 1) {
-        error_setg(errp, "vfio: unexpected number of io irqs %u",
+    if (vdev->num_irqs < irq + 1) {
+        error_setg(errp, "vfio: unexpected number of irqs %u",
                    vdev->num_irqs);
         return;
     }
 
     argsz = sizeof(*irq_info);
     irq_info = g_malloc0(argsz);
-    irq_info->index = VFIO_CCW_IO_IRQ_INDEX;
+    irq_info->index = irq;
     irq_info->argsz = argsz;
     if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
               irq_info) < 0 || irq_info->count < 1) {
@@ -357,37 +370,47 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
         goto out_free_info;
     }
 
-    if (event_notifier_init(&vcdev->io_notifier, 0)) {
+    if (event_notifier_init(notifier, 0)) {
         error_setg_errno(errp, errno,
-                         "vfio: Unable to init event notifier for IO");
+                         "vfio: Unable to init event notifier for irq (%d)", irq);
         goto out_free_info;
     }
 
-    fd = event_notifier_get_fd(&vcdev->io_notifier);
-    qemu_set_fd_handler(fd, vfio_ccw_io_notifier_handler, NULL, vcdev);
+    fd = event_notifier_get_fd(notifier);
+    qemu_set_fd_handler(fd, fd_read, NULL, vcdev);
 
-    if (vfio_set_irq_signaling(vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
+    if (vfio_set_irq_signaling(vdev, irq, 0,
                                VFIO_IRQ_SET_ACTION_TRIGGER, fd, errp)) {
         qemu_set_fd_handler(fd, NULL, NULL, vcdev);
-        event_notifier_cleanup(&vcdev->io_notifier);
+        event_notifier_cleanup(notifier);
     }
 
 out_free_info:
     g_free(irq_info);
 }
 
-static void vfio_ccw_unregister_io_notifier(VFIOCCWDevice *vcdev)
+static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev, int irq)
 {
     Error *err = NULL;
+    EventNotifier *notifier;
+
+    switch (irq) {
+    case VFIO_CCW_IO_IRQ_INDEX:
+        notifier = &vcdev->io_notifier;
+        break;
+    default:
+        error_report("vfio: Unsupported device irq(%d) fd: %m", irq);
+        return;
+    }
 
-    if (vfio_set_irq_signaling(&vcdev->vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
+    if (vfio_set_irq_signaling(&vcdev->vdev, irq, 0,
                                VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
         error_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name);
     }
 
-    qemu_set_fd_handler(event_notifier_get_fd(&vcdev->io_notifier),
+    qemu_set_fd_handler(event_notifier_get_fd(notifier),
                         NULL, NULL, vcdev);
-    event_notifier_cleanup(&vcdev->io_notifier);
+    event_notifier_cleanup(notifier);
 }
 
 static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp)
@@ -590,7 +613,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
         goto out_region_err;
     }
 
-    vfio_ccw_register_io_notifier(vcdev, &err);
+    vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, &err);
     if (err) {
         goto out_notifier_err;
     }
@@ -619,7 +642,7 @@ static void vfio_ccw_unrealize(DeviceState *dev, Error **errp)
     S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev);
     VFIOGroup *group = vcdev->vdev.group;
 
-    vfio_ccw_unregister_io_notifier(vcdev);
+    vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX);
     vfio_ccw_put_region(vcdev);
     vfio_ccw_put_device(vcdev);
     vfio_put_group(group);
-- 
2.17.1

Re: [RFC PATCH v1 7/8] vfio-ccw: Refactor ccw irq handler

[Cornelia Huck]

On Fri, 15 Nov 2019 04:34:36 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> Make it easier to add new ones in the future.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  hw/vfio/ccw.c | 55 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 39 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 2b1a83b94c..b16526d5de 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -334,22 +334,35 @@ read_err:
>      css_inject_io_interrupt(sch);
>  }
>  
> -static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
> +static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev, int irq,

Maybe make this unsigned?

> +                                           Error **errp)
>  {
>      VFIODevice *vdev = &vcdev->vdev;
>      struct vfio_irq_info *irq_info;
>      size_t argsz;
>      int fd;
> +    EventNotifier *notifier;
> +    IOHandler *fd_read;
> +
> +    switch (irq) {
> +    case VFIO_CCW_IO_IRQ_INDEX:
> +        notifier = &vcdev->io_notifier;
> +        fd_read = vfio_ccw_io_notifier_handler;
> +        break;
> +    default:
> +        error_setg(errp, "vfio: Unsupported device irq(%d) fd: %m", irq);

Hm, which errno is this supposed to print?

> +        return;
> +    }
>  
> -    if (vdev->num_irqs < VFIO_CCW_IO_IRQ_INDEX + 1) {
> -        error_setg(errp, "vfio: unexpected number of io irqs %u",
> +    if (vdev->num_irqs < irq + 1) {
> +        error_setg(errp, "vfio: unexpected number of irqs %u",
>                     vdev->num_irqs);
>          return;
>      }
>  
>      argsz = sizeof(*irq_info);
>      irq_info = g_malloc0(argsz);
> -    irq_info->index = VFIO_CCW_IO_IRQ_INDEX;
> +    irq_info->index = irq;
>      irq_info->argsz = argsz;
>      if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
>                irq_info) < 0 || irq_info->count < 1) {
> @@ -357,37 +370,47 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
>          goto out_free_info;
>      }
>  
> -    if (event_notifier_init(&vcdev->io_notifier, 0)) {
> +    if (event_notifier_init(notifier, 0)) {
>          error_setg_errno(errp, errno,
> -                         "vfio: Unable to init event notifier for IO");
> +                         "vfio: Unable to init event notifier for irq (%d)", irq);
>          goto out_free_info;
>      }
>  
> -    fd = event_notifier_get_fd(&vcdev->io_notifier);
> -    qemu_set_fd_handler(fd, vfio_ccw_io_notifier_handler, NULL, vcdev);
> +    fd = event_notifier_get_fd(notifier);
> +    qemu_set_fd_handler(fd, fd_read, NULL, vcdev);
>  
> -    if (vfio_set_irq_signaling(vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
> +    if (vfio_set_irq_signaling(vdev, irq, 0,
>                                 VFIO_IRQ_SET_ACTION_TRIGGER, fd, errp)) {
>          qemu_set_fd_handler(fd, NULL, NULL, vcdev);
> -        event_notifier_cleanup(&vcdev->io_notifier);
> +        event_notifier_cleanup(notifier);
>      }
>  
>  out_free_info:
>      g_free(irq_info);
>  }
>  
> -static void vfio_ccw_unregister_io_notifier(VFIOCCWDevice *vcdev)
> +static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev, int irq)

Also unsigned here?

>  {
>      Error *err = NULL;
> +    EventNotifier *notifier;
> +
> +    switch (irq) {
> +    case VFIO_CCW_IO_IRQ_INDEX:
> +        notifier = &vcdev->io_notifier;
> +        break;
> +    default:
> +        error_report("vfio: Unsupported device irq(%d) fd: %m", irq);

Same comment for the %m.

> +        return;
> +    }
>  
> -    if (vfio_set_irq_signaling(&vcdev->vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
> +    if (vfio_set_irq_signaling(&vcdev->vdev, irq, 0,
>                                 VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
>          error_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name);
>      }
>  
> -    qemu_set_fd_handler(event_notifier_get_fd(&vcdev->io_notifier),
> +    qemu_set_fd_handler(event_notifier_get_fd(notifier),
>                          NULL, NULL, vcdev);
> -    event_notifier_cleanup(&vcdev->io_notifier);
> +    event_notifier_cleanup(notifier);
>  }
>  
>  static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp)
> @@ -590,7 +613,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
>          goto out_region_err;
>      }
>  
> -    vfio_ccw_register_io_notifier(vcdev, &err);
> +    vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, &err);
>      if (err) {
>          goto out_notifier_err;
>      }
> @@ -619,7 +642,7 @@ static void vfio_ccw_unrealize(DeviceState *dev, Error **errp)
>      S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev);
>      VFIOGroup *group = vcdev->vdev.group;
>  
> -    vfio_ccw_unregister_io_notifier(vcdev);
> +    vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX);
>      vfio_ccw_put_region(vcdev);
>      vfio_ccw_put_device(vcdev);
>      vfio_put_group(group);

Otherwise, looks good.

Re: [RFC PATCH v1 7/8] vfio-ccw: Refactor ccw irq handler

[Eric Farman]

On 11/20/19 7:39 AM, Cornelia Huck wrote:
> On Fri, 15 Nov 2019 04:34:36 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> Make it easier to add new ones in the future.
>>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>  hw/vfio/ccw.c | 55 ++++++++++++++++++++++++++++++++++++---------------
>>  1 file changed, 39 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>> index 2b1a83b94c..b16526d5de 100644
>> --- a/hw/vfio/ccw.c
>> +++ b/hw/vfio/ccw.c
>> @@ -334,22 +334,35 @@ read_err:
>>      css_inject_io_interrupt(sch);
>>  }
>>  
>> -static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
>> +static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev, int irq,
> 
> Maybe make this unsigned?

Yeah, seems proper.

> 
>> +                                           Error **errp)
>>  {
>>      VFIODevice *vdev = &vcdev->vdev;
>>      struct vfio_irq_info *irq_info;
>>      size_t argsz;
>>      int fd;
>> +    EventNotifier *notifier;
>> +    IOHandler *fd_read;
>> +
>> +    switch (irq) {
>> +    case VFIO_CCW_IO_IRQ_INDEX:
>> +        notifier = &vcdev->io_notifier;
>> +        fd_read = vfio_ccw_io_notifier_handler;
>> +        break;
>> +    default:
>> +        error_setg(errp, "vfio: Unsupported device irq(%d) fd: %m", irq);
> 
> Hm, which errno is this supposed to print?

Um...  Dealers choice?  :)  I'll fix this up.

> 
>> +        return;
>> +    }
>>  
>> -    if (vdev->num_irqs < VFIO_CCW_IO_IRQ_INDEX + 1) {
>> -        error_setg(errp, "vfio: unexpected number of io irqs %u",
>> +    if (vdev->num_irqs < irq + 1) {
>> +        error_setg(errp, "vfio: unexpected number of irqs %u",
>>                     vdev->num_irqs);
>>          return;
>>      }
>>  
>>      argsz = sizeof(*irq_info);
>>      irq_info = g_malloc0(argsz);
>> -    irq_info->index = VFIO_CCW_IO_IRQ_INDEX;
>> +    irq_info->index = irq;
>>      irq_info->argsz = argsz;
>>      if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
>>                irq_info) < 0 || irq_info->count < 1) {
>> @@ -357,37 +370,47 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
>>          goto out_free_info;
>>      }
>>  
>> -    if (event_notifier_init(&vcdev->io_notifier, 0)) {
>> +    if (event_notifier_init(notifier, 0)) {
>>          error_setg_errno(errp, errno,
>> -                         "vfio: Unable to init event notifier for IO");
>> +                         "vfio: Unable to init event notifier for irq (%d)", irq);
>>          goto out_free_info;
>>      }
>>  
>> -    fd = event_notifier_get_fd(&vcdev->io_notifier);
>> -    qemu_set_fd_handler(fd, vfio_ccw_io_notifier_handler, NULL, vcdev);
>> +    fd = event_notifier_get_fd(notifier);
>> +    qemu_set_fd_handler(fd, fd_read, NULL, vcdev);
>>  
>> -    if (vfio_set_irq_signaling(vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
>> +    if (vfio_set_irq_signaling(vdev, irq, 0,
>>                                 VFIO_IRQ_SET_ACTION_TRIGGER, fd, errp)) {
>>          qemu_set_fd_handler(fd, NULL, NULL, vcdev);
>> -        event_notifier_cleanup(&vcdev->io_notifier);
>> +        event_notifier_cleanup(notifier);
>>      }
>>  
>>  out_free_info:
>>      g_free(irq_info);
>>  }
>>  
>> -static void vfio_ccw_unregister_io_notifier(VFIOCCWDevice *vcdev)
>> +static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev, int irq)
> 
> Also unsigned here?
> 
>>  {
>>      Error *err = NULL;
>> +    EventNotifier *notifier;
>> +
>> +    switch (irq) {
>> +    case VFIO_CCW_IO_IRQ_INDEX:
>> +        notifier = &vcdev->io_notifier;
>> +        break;
>> +    default:
>> +        error_report("vfio: Unsupported device irq(%d) fd: %m", irq);
> 
> Same comment for the %m.
> 
>> +        return;
>> +    }
>>  
>> -    if (vfio_set_irq_signaling(&vcdev->vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
>> +    if (vfio_set_irq_signaling(&vcdev->vdev, irq, 0,
>>                                 VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
>>          error_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name);
>>      }
>>  
>> -    qemu_set_fd_handler(event_notifier_get_fd(&vcdev->io_notifier),
>> +    qemu_set_fd_handler(event_notifier_get_fd(notifier),
>>                          NULL, NULL, vcdev);
>> -    event_notifier_cleanup(&vcdev->io_notifier);
>> +    event_notifier_cleanup(notifier);
>>  }
>>  
>>  static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp)
>> @@ -590,7 +613,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
>>          goto out_region_err;
>>      }
>>  
>> -    vfio_ccw_register_io_notifier(vcdev, &err);
>> +    vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, &err);
>>      if (err) {
>>          goto out_notifier_err;
>>      }
>> @@ -619,7 +642,7 @@ static void vfio_ccw_unrealize(DeviceState *dev, Error **errp)
>>      S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev);
>>      VFIOGroup *group = vcdev->vdev.group;
>>  
>> -    vfio_ccw_unregister_io_notifier(vcdev);
>> +    vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX);
>>      vfio_ccw_put_region(vcdev);
>>      vfio_ccw_put_device(vcdev);
>>      vfio_put_group(group);
> 
> Otherwise, looks good.
> 

[RFC PATCH v1 8/8] vfio-ccw: Add support for the CRW irq

[Eric Farman]

From: Farhan Ali <alifm@linux.ibm.com>

The CRW irq will be used by vfio-ccw to notify the userspace
about any CRWs the userspace needs to handle. Let's add support
for it.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---

Notes:
    v0->v1: [EF]
     - Check vcdev->crw_region before registering the irq,
       in case host kernel does not have matching support
     - Split the refactoring changes to an earlier (new) patch
       (and don't remove the "num_irqs" check in the register
       routine, but adjust it to the check the input variable)
     - Don't revert the cool vfio_set_irq_signaling() stuff
     - Unregister CRW IRQ before IO IRQ in unrealize
     - s/crw1/crw0/

 hw/vfio/ccw.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index b16526d5de..b3f4120118 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -48,6 +48,7 @@ struct VFIOCCWDevice {
     uint64_t crw_region_offset;
     struct ccw_crw_region *crw_region;
     EventNotifier io_notifier;
+    EventNotifier crw_notifier;
     bool force_orb_pfch;
     bool warned_orb_pfch;
 };
@@ -259,6 +260,34 @@ static void vfio_ccw_reset(DeviceState *dev)
     ioctl(vcdev->vdev.fd, VFIO_DEVICE_RESET);
 }
 
+static void vfio_ccw_crw_notifier_handler(void *opaque)
+{
+    VFIOCCWDevice *vcdev = opaque;
+    struct ccw_crw_region *region = vcdev->crw_region;
+    CRW crw;
+    int size;
+    uint8_t erc;
+    uint16_t rsid;
+
+    if (!event_notifier_test_and_clear(&vcdev->crw_notifier)) {
+        return;
+    }
+
+    memset(region, 0, sizeof(*region));
+    size = pread(vcdev->vdev.fd, region, vcdev->crw_region_size,
+                 vcdev->crw_region_offset);
+
+    if (size == -1) {
+        error_report("vfio-ccw: Read crw region failed with errno=%d", errno);
+        return;
+    }
+
+    memcpy(&crw, &region->crw0, sizeof(CRW));
+    erc = crw.flags & 0x003f;
+    rsid = crw.rsid;
+    css_queue_crw(CRW_RSC_CHP, erc, 0, 0, rsid);
+}
+
 static void vfio_ccw_io_notifier_handler(void *opaque)
 {
     VFIOCCWDevice *vcdev = opaque;
@@ -349,6 +378,10 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev, int irq,
         notifier = &vcdev->io_notifier;
         fd_read = vfio_ccw_io_notifier_handler;
         break;
+    case VFIO_CCW_CRW_IRQ_INDEX:
+        notifier = &vcdev->crw_notifier;
+        fd_read = vfio_ccw_crw_notifier_handler;
+        break;
     default:
         error_setg(errp, "vfio: Unsupported device irq(%d) fd: %m", irq);
         return;
@@ -398,6 +431,9 @@ static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev, int irq)
     case VFIO_CCW_IO_IRQ_INDEX:
         notifier = &vcdev->io_notifier;
         break;
+    case VFIO_CCW_CRW_IRQ_INDEX:
+        notifier = &vcdev->crw_notifier;
+        break;
     default:
         error_report("vfio: Unsupported device irq(%d) fd: %m", irq);
         return;
@@ -618,6 +654,14 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
         goto out_notifier_err;
     }
 
+    if (vcdev->crw_region) {
+        vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX, &err);
+        if (err) {
+            vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX);
+            goto out_notifier_err;
+        }
+    }
+
     return;
 
 out_notifier_err:
@@ -642,6 +686,7 @@ static void vfio_ccw_unrealize(DeviceState *dev, Error **errp)
     S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev);
     VFIOGroup *group = vcdev->vdev.group;
 
+    vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX);
     vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX);
     vfio_ccw_put_region(vcdev);
     vfio_ccw_put_device(vcdev);
-- 
2.17.1

Re: [RFC PATCH v1 8/8] vfio-ccw: Add support for the CRW irq

[Cornelia Huck]

On Fri, 15 Nov 2019 04:34:37 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> From: Farhan Ali <alifm@linux.ibm.com>
> 
> The CRW irq will be used by vfio-ccw to notify the userspace
> about any CRWs the userspace needs to handle. Let's add support
> for it.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     v0->v1: [EF]
>      - Check vcdev->crw_region before registering the irq,
>        in case host kernel does not have matching support
>      - Split the refactoring changes to an earlier (new) patch
>        (and don't remove the "num_irqs" check in the register
>        routine, but adjust it to the check the input variable)
>      - Don't revert the cool vfio_set_irq_signaling() stuff

Only the uncool stuff? ;)

>      - Unregister CRW IRQ before IO IRQ in unrealize
>      - s/crw1/crw0/
> 
>  hw/vfio/ccw.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index b16526d5de..b3f4120118 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -48,6 +48,7 @@ struct VFIOCCWDevice {
>      uint64_t crw_region_offset;
>      struct ccw_crw_region *crw_region;
>      EventNotifier io_notifier;
> +    EventNotifier crw_notifier;
>      bool force_orb_pfch;
>      bool warned_orb_pfch;
>  };
> @@ -259,6 +260,34 @@ static void vfio_ccw_reset(DeviceState *dev)
>      ioctl(vcdev->vdev.fd, VFIO_DEVICE_RESET);
>  }
>  
> +static void vfio_ccw_crw_notifier_handler(void *opaque)
> +{
> +    VFIOCCWDevice *vcdev = opaque;
> +    struct ccw_crw_region *region = vcdev->crw_region;
> +    CRW crw;
> +    int size;
> +    uint8_t erc;
> +    uint16_t rsid;
> +
> +    if (!event_notifier_test_and_clear(&vcdev->crw_notifier)) {
> +        return;
> +    }

Referring back to the comments I made for the kernel part
(https://lore.kernel.org/kvm/20191119195236.35189d5b.cohuck@redhat.com/),
I think we may have a problem when we have multiple crws pending.

IIUC, the kernel does not provide any guarantees that we get exactly
one interrupt per crw. I'm wondering whether it would make sense to
mimic the behaviour of stcrw, i.e.

(a) get a notification
(b) read the region to obtain a crw
(c) do whatever needs to be done
(d) repeat (b) and (c) until (b) does not return a valid crw

That way, we can also accommodate arbitrary lengths of crw chains, as
we do not have to reserve space for a pre-defined number of crws in the
region.

> +
> +    memset(region, 0, sizeof(*region));
> +    size = pread(vcdev->vdev.fd, region, vcdev->crw_region_size,
> +                 vcdev->crw_region_offset);
> +
> +    if (size == -1) {
> +        error_report("vfio-ccw: Read crw region failed with errno=%d", errno);
> +        return;
> +    }
> +
> +    memcpy(&crw, &region->crw0, sizeof(CRW));
> +    erc = crw.flags & 0x003f;
> +    rsid = crw.rsid;
> +    css_queue_crw(CRW_RSC_CHP, erc, 0, 0, rsid);
> +}
> +
>  static void vfio_ccw_io_notifier_handler(void *opaque)
>  {
>      VFIOCCWDevice *vcdev = opaque;