s390x/vfio_ccw: Channel Path Handling [QEMU]
v2 cover s390x/vfio_ccw: Channel Path Handling [QEMU]
Here is a new 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/[email protected]/
I did leave a couple things FIXMEs from v1 comments in here. Thought it'd be best to just get this out with the kernel code, to make sure things aren't too far in the weeds.
v1: https://lore.kernel.org/qemu-devel/[email protected]/
Eric Farman (2):
vfio-ccw: Refactor cleanup of regions
vfio-ccw: Refactor ccw irq handler
Farhan Ali (5):
vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO
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 | 13 +- hw/s390x/s390-ccw.c | 28 ++++ hw/vfio/ccw.c | 204 ++++++++++++++++++++++--- include/hw/s390x/css.h | 3 +- include/hw/s390x/s390-ccw.h | 1 + include/standard-headers/linux/input.h | 1 + linux-headers/asm-arm64/unistd.h | 1 + linux-headers/linux/vfio.h | 3 + linux-headers/linux/vfio_ccw.h | 19 +++ target/s390x/ioinst.c | 3 +- 10 files changed, 250 insertions(+), 26 deletions(-)
Signature · 1 line
2.17.1
RFC PATCH v2 1/7 vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO
From: Farhan Ali <[email protected]>
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'.
---
Notes:
v1->v2: [EF]
- Add s-o-b
- [Seems the discussion on v1 centered on the return code set in the kernel, rather than anything that needs to change here, unless I've missed something.]
hw/vfio/ccw.c | 1 + 1 file changed, 1 insertion(+)
hw/vfio/ccw.c +1 -0
@@ -114,6 +114,7 @@ again:return IOINST_CC_BUSY ;case -ENODEV :case -EACCES : +case -EIO :return IOINST_CC_NOT_OPERATIONAL ;case -EFAULT :default :
Signature · 1 line
2.17.1
Re: [RFC PATCH v2 1/7] vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO
Quoted 10 lines · On Thu, 6 Feb 2020 22:45:03 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
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 <[email protected]>Signed-off-by Eric Farman <[email protected]>
Quoted 8 lines
---
Notes:
v1->v2: [EF]
- Add s-o-b
- [Seems the discussion on v1 centered on the return code set in the kernel, rather than anything that needs to change here, unless I've missed something.]
I've stared at this and at the kernel code for some time again; and I'm not sure if "return -EIO == not operational" is even true. That said, I'm not sure a unit check is the right response, either. The only thing I'm sure about is that the kernel code needs some review of return codes and some documentation...
Quoted 16 lines
hw/vfio/ccw.c | 1 + 1 file changed, 1 insertion(+)hw/vfio/ccw.c +1
-0@@ -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 v2 1/7] vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO
Quoted 10 lines · On Tue, 24 Mar 2020 18:04:30 +0100 Cornelia Huck <[email protected]> wrote:
Quoted 7 lines · On Thu, 6 Feb 2020 22:45:03 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
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'.
I believe this is not the only situation when vfio-ccw returns EIO, or?
Quoted 11 lines
Quoted 11 lines
Signed-off-by Farhan Ali <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Notes:
v1->v2: [EF]
- Add s-o-b
- [Seems the discussion on v1 centered on the return code set in the kernel, rather than anything that needs to change here, unless I've missed something.]
Does this need to change here? If the kernel is supposed to return ENODEV then this does not need to change.
Quoted 6 lines
I've stared at this and at the kernel code for some time again; and I'm not sure if "return
-EIO== not operational" is even true. That said, I'm not sure a unit check is the right response, either. The only thing I'm sure about is that the kernel code needs some review of return codes and some documentation...
I could not agree more, this is semantically uapi and needs to be properly documented.
With regards to "linux error codes: vs "ionist cc's" an where the mapping is different example:
"""
/**
* cio_cancel_halt_clear - Cancel running I/O by performing cancel, halt
* and clear ordinally if subchannel is valid.
* @sch: subchannel on which to perform the cancel_halt_clear operation
* @iretry: the number of the times remained to retry the next operation *
* This should be called repeatedly since halt/clear are asynchronous
* operations. We do one try with cio_cancel, three tries with cio_halt,
* 255 tries with cio_clear. The caller should initialize @iretry with
* the value 255 for its first call to this, and keep using the same
* @iretry in the subsequent calls until it gets a non -EBUSY return. *
* Returns 0 if device now idle, -ENODEV for device not operational,
* -EBUSY if an interrupt is expected (either from halt/clear or from a
* status pending), and -EIO if out of retries.
*/
int cio_cancel_halt_clear(struct subchannel *sch, int *iretry)
"""
Here -ENODEV is not operational.
Regards,
Halil
Quoted 19 lines
Quoted 17 lines
hw/vfio/ccw.c | 1 + 1 file changed, 1 insertion(+)hw/vfio/ccw.c +1
-0@@ -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 v2 1/7] vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO
Quoted 64 lines · On Wed, 25 Mar 2020 03:24:28 +0100 Halil Pasic <[email protected]> wrote:
Quoted 10 lines · On Tue, 24 Mar 2020 18:04:30 +0100 Cornelia Huck <[email protected]> wrote:
Quoted 7 lines · On Thu, 6 Feb 2020 22:45:03 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
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'.
I believe this is not the only situation when vfio-ccw returns EIO, or?
Quoted 11 lines
Quoted 11 lines
Signed-off-by Farhan Ali <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Notes:
v1->v2: [EF]
- Add s-o-b
- [Seems the discussion on v1 centered on the return code set in the kernel, rather than anything that needs to change here, unless I've missed something.]Does this need to change here? If the kernel is supposed to return ENODEV then this does not need to change.
Quoted 6 lines
I've stared at this and at the kernel code for some time again; and I'm not sure if "return
-EIO== not operational" is even true. That said, I'm not sure a unit check is the right response, either. The only thing I'm sure about is that the kernel code needs some review of return codes and some documentation...I could not agree more, this is semantically uapi and needs to be properly documented.
With regards to "linux error codes: vs "ionist cc's" an where the mapping is different example:
"""
/**
*cio_cancel_halt_clear- Cancel running I/O by performing cancel, halt
* and clear ordinally if subchannel is valid.
* @sch: subchannel on which to perform thecancel_halt_clearoperation
* @iretry: the number of the times remained to retry the next operation *
* This should be called repeatedly since halt/clear are asynchronous
* operations. We do one try withcio_cancel, three tries withcio_halt,
* 255 tries withcio_clear. The caller should initialize @iretry with
* the value 255 for its first call to this, and keep using the same
* @iretry in the subsequent calls until it gets a non-EBUSYreturn. *
* Returns 0 if device now idle,-ENODEVfor device not operational,
*-EBUSYif an interrupt is expected (either from halt/clear or from a
* status pending), and-EIOif out of retries.
*/
intcio_cancel_halt_clear(struct subchannel *sch, int *iretry)
Quoted 2 lines
"""
Here-ENODEVis not operational.
Ok, I went through the kernel code and checked which error may be returned in which case (hope I caught all of them). Here's what I currently have:
Documentation/s390/vfio-ccw.rst +53 -1
@@ -210,7 +210,36 @@ Subchannel. irb_area stores the I/O result. -ret_code stores a return code for each access of the region. +ret_code stores a return code for each access of the region. The following +values may occur: + +0 + The operation was successful. + +-EOPNOTSUPP + The orb specified transport mode or an unidentified IDAW format, did not + specify prefetch mode, or the scsw specified a function other than the + start function. + +-EIO + A request was issued while the device was not in a state ready to accept + requests, or an internal error occurred. + +-EBUSY + The subchannel was status pending or busy, or a request is already active. + +-EAGAIN + A request was being processed, and the caller should retry. + +-EACCES + The channel path(s) used for the I/O were found to be not operational. + +-ENODEV + The device was found to be not operational. + +-EINVAL + The orb specified a chain longer than 255 ccws, or an internal error + occurred. This region is always available.
@@ -231,6 +260,29 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD. Currently, CLEAR SUBCHANNEL and HALT SUBCHANNEL use this region. +command specifies the command to be issued; ret_code stores a return code +for each access of the region. The following values may occur: + +0 + The operation was successful. + +-ENODEV + The device was found to be not operational. + +-EINVAL + A command other than halt or clear was specified. + +-EIO + A request was issued while the device was not in a state ready to accept + requests. + +-EAGAIN + A request was being processed, and the caller should retry. + +-EBUSY + The subchannel was status pending or busy while processing a halt request. + + vfio-ccw operation details --------------------------
Unless we interpret "device in wrong state" as "not operational", mapping -EIO to cc 3 is probably not the right thing to do; but generating a unit exception probably isn't either. Honestly, I'm unsure what the right thing to do here would be...
Another problem is that -EIO might signal "something went wrong in the kernel code" - should not happen, but would certainly not be the caller's fault, and they can't do anything about it. That "internal error" thing might also be signaled by -EINVAL (which is odd), but -EINVAL could also mean "the ccw chain is too long", for which -EOPNOTSUPP would probably be a better return code, as it's a limitation in the code, not the architecture, IIRC. Not sure if we can still change that, though (and QEMU handles both in the same way, anyway.)
The other return codes look sane, and the return codes for the async region also seem sane (but have the same issue with -EIO == "device in wrong state").
Looking at the QEMU handling, I think the -EIO handling is a bit questionable (without an obvious solution), while mapping -EBUSY to cc 2 is the only reasonable thing to do given that the interface does not differentiate between "busy" and "status pending". The rest seems sane.
Re: [RFC PATCH v2 1/7] vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO
Quoted 67 lines · Cornelia Huck
Quoted 64 lines · On Wed, 25 Mar 2020 03:24:28 +0100 Halil Pasic <[email protected]> wrote:
Quoted 10 lines · On Tue, 24 Mar 2020 18:04:30 +0100 Cornelia Huck <[email protected]> wrote:
Quoted 7 lines · On Thu, 6 Feb 2020 22:45:03 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
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'.
I believe this is not the only situation when vfio-ccw returns EIO, or?
Quoted 11 lines
Quoted 11 lines
Signed-off-by Farhan Ali <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Notes:
v1->v2: [EF]
- Add s-o-b
- [Seems the discussion on v1 centered on the return code set in the kernel, rather than anything that needs to change here, unless I've missed something.]Does this need to change here? If the kernel is supposed to return ENODEV then this does not need to change.
Quoted 6 lines
I've stared at this and at the kernel code for some time again; and I'm not sure if "return
-EIO== not operational" is even true. That said, I'm not sure a unit check is the right response, either. The only thing I'm sure about is that the kernel code needs some review of return codes and some documentation...I could not agree more, this is semantically uapi and needs to be properly documented.
With regards to "linux error codes: vs "ionist cc's" an where the mapping is different example:
"""
/**
*cio_cancel_halt_clear- Cancel running I/O by performing cancel, halt
* and clear ordinally if subchannel is valid.
* @sch: subchannel on which to perform thecancel_halt_clearoperation
* @iretry: the number of the times remained to retry the next operation *
* This should be called repeatedly since halt/clear are asynchronous
* operations. We do one try withcio_cancel, three tries withcio_halt,
* 255 tries withcio_clear. The caller should initialize @iretry with
* the value 255 for its first call to this, and keep using the same
* @iretry in the subsequent calls until it gets a non-EBUSYreturn. *
* Returns 0 if device now idle,-ENODEVfor device not operational,
*-EBUSYif an interrupt is expected (either from halt/clear or from a
* status pending), and-EIOif out of retries.
*/
intcio_cancel_halt_clear(struct subchannel *sch, int *iretry)
Quoted 6 lines
Quoted 2 lines
"""
Here-ENODEVis not operational.Ok, I went through the kernel code and checked which error may be returned in which case (hope I caught all of them). Here's what I currently have:
Thanks for doing all this mapping!
Quoted 73 lines
Documentation/s390/vfio-ccw.rst +53
-1@@ -210,7 +210,36 @@ Subchannel. irb_area stores the I/O result. -ret_code stores a return code for each access of the region. +ret_code stores a return code for each access of the region. The following +values may occur: + +0 + The operation was successful. + +-EOPNOTSUPP + The orb specified transport mode or an unidentified IDAW format, did not + specify prefetch mode, or the scsw specified a function other than the + start function. + +-EIO + A request was issued while the device was not in a state ready to accept + requests, or an internal error occurred. + +-EBUSY + The subchannel was status pending or busy, or a request is already active. + +-EAGAIN + A request was being processed, and the caller should retry. + +-EACCES + The channel path(s) used for the I/O were found to be not operational. + +-ENODEV + The device was found to be not operational. + +-EINVAL + The orb specified a chain longer than 255 ccws, or an internal error + occurred. This region is always available.@@ -231,6 +260,29 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD. Currently, CLEAR SUBCHANNEL and HALT SUBCHANNEL use this region. +command specifies the command to be issued; ret_code stores a return code +for each access of the region. The following values may occur: + +0 + The operation was successful. + +-ENODEV + The device was found to be not operational. + +-EINVAL + A command other than halt or clear was specified. + +-EIO + A request was issued while the device was not in a state ready to accept + requests. + +-EAGAIN + A request was being processed, and the caller should retry. + +-EBUSY + The subchannel was status pending or busy while processing a halt request. + + vfio-ccw operation details --------------------------
Quoted 4 lines
Unless we interpret "device in wrong state" as "not operational", mapping
-EIOto cc 3 is probably not the right thing to do; but generating a unit exception probably isn't either. Honestly, I'm unsure what the right thing to do here would be...
Me neither. I grepped my qemu logs for the past, ugh, too long for the error report that would precede these failures ("vfio-ccw: [wirte/write] I/O region failed with errno=%d"). Excluding the ones that were obviously the result of half-baked code, all instances were either -EBUSY or -ENODEV. Could I trigger one? Maybe. Is it likely? Doesn't seem to be.
It seems that getting -EIO would indicate we got ourselves out of sync, and started buttoning up the device again (or never having opened it in the first place), so a unit exception might be valid as a "hey something's screwy here" ?
Quoted 11 lines
Another problem is that
-EIOmight signal "something went wrong in the kernel code" - should not happen, but would certainly not be the caller's fault, and they can't do anything about it. That "internal error" thing might also be signaled by-EINVAL(which is odd), butDocumentation/s390/vfio-ccw.rst +0
-2-EINVAL could also mean "the ccw chain is too long", for which -EOPNOTSUPP would probably be a better return code, as it's alimitation in the code, not the architecture, IIRC. Not sure if we can still change that, though (and QEMU handles both in the same way, anyway.)
Quoted 9 lines
The other return codes look sane, and the return codes for the async region also seem sane (but have the same issue with
-EIO== "device in wrong state").Looking at the QEMU handling, I think the
-EIOhandling is a bit questionable (without an obvious solution), while mapping-EBUSYto cc 2 is the only reasonable thing to do given that the interface does not differentiate between "busy" and "status pending". The rest seems sane.
So maybe with all this data, and absent a better solution, it's best to just drop this patch from v3?
Re: [RFC PATCH v2 1/7] vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO
Quoted 162 lines · On Mon, 6 Apr 2020 14:21:17 -0400 Eric Farman <[email protected]> wrote:
Quoted 67 lines · Cornelia Huck
Quoted 64 lines · On Wed, 25 Mar 2020 03:24:28 +0100 Halil Pasic <[email protected]> wrote:
Quoted 10 lines · On Tue, 24 Mar 2020 18:04:30 +0100 Cornelia Huck <[email protected]> wrote:
Quoted 7 lines · On Thu, 6 Feb 2020 22:45:03 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
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'.
I believe this is not the only situation when vfio-ccw returns EIO, or?
Quoted 11 lines
Quoted 11 lines
Signed-off-by Farhan Ali <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Notes:
v1->v2: [EF]
- Add s-o-b
- [Seems the discussion on v1 centered on the return code set in the kernel, rather than anything that needs to change here, unless I've missed something.]Does this need to change here? If the kernel is supposed to return ENODEV then this does not need to change.
Quoted 6 lines
I've stared at this and at the kernel code for some time again; and I'm not sure if "return
-EIO== not operational" is even true. That said, I'm not sure a unit check is the right response, either. The only thing I'm sure about is that the kernel code needs some review of return codes and some documentation...I could not agree more, this is semantically uapi and needs to be properly documented.
With regards to "linux error codes: vs "ionist cc's" an where the mapping is different example:
"""
/**
*cio_cancel_halt_clear- Cancel running I/O by performing cancel, halt
* and clear ordinally if subchannel is valid.
* @sch: subchannel on which to perform thecancel_halt_clearoperation
* @iretry: the number of the times remained to retry the next operation *
* This should be called repeatedly since halt/clear are asynchronous
* operations. We do one try withcio_cancel, three tries withcio_halt,
* 255 tries withcio_clear. The caller should initialize @iretry with
* the value 255 for its first call to this, and keep using the same
* @iretry in the subsequent calls until it gets a non-EBUSYreturn. *
* Returns 0 if device now idle,-ENODEVfor device not operational,
*-EBUSYif an interrupt is expected (either from halt/clear or from a
* status pending), and-EIOif out of retries.
*/
intcio_cancel_halt_clear(struct subchannel *sch, int *iretry)Quoted 6 lines
Quoted 2 lines
"""
Here-ENODEVis not operational.Ok, I went through the kernel code and checked which error may be returned in which case (hope I caught all of them). Here's what I currently have:
Thanks for doing all this mapping!
Quoted 73 lines
Documentation/s390/vfio-ccw.rst +53
-1@@ -210,7 +210,36 @@ Subchannel. irb_area stores the I/O result. -ret_code stores a return code for each access of the region. +ret_code stores a return code for each access of the region. The following +values may occur: + +0 + The operation was successful. + +-EOPNOTSUPP + The orb specified transport mode or an unidentified IDAW format, did not + specify prefetch mode, or the scsw specified a function other than the + start function. + +-EIO + A request was issued while the device was not in a state ready to accept + requests, or an internal error occurred. + +-EBUSY + The subchannel was status pending or busy, or a request is already active. + +-EAGAIN + A request was being processed, and the caller should retry. + +-EACCES + The channel path(s) used for the I/O were found to be not operational. + +-ENODEV + The device was found to be not operational. + +-EINVAL + The orb specified a chain longer than 255 ccws, or an internal error + occurred. This region is always available.@@ -231,6 +260,29 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD. Currently, CLEAR SUBCHANNEL and HALT SUBCHANNEL use this region. +command specifies the command to be issued; ret_code stores a return code +for each access of the region. The following values may occur: + +0 + The operation was successful. + +-ENODEV + The device was found to be not operational. + +-EINVAL + A command other than halt or clear was specified. + +-EIO + A request was issued while the device was not in a state ready to accept + requests. + +-EAGAIN + A request was being processed, and the caller should retry. + +-EBUSY + The subchannel was status pending or busy while processing a halt request. + + vfio-ccw operation details --------------------------Quoted 4 lines
Unless we interpret "device in wrong state" as "not operational", mapping
-EIOto cc 3 is probably not the right thing to do; but generating a unit exception probably isn't either. Honestly, I'm unsure what the right thing to do here would be...Me neither. I grepped my qemu logs for the past, ugh, too long for the error report that would precede these failures ("vfio-ccw: [wirte/write] I/O region failed with errno=%d"). Excluding the ones that were obviously the result of half-baked code, all instances were either
-EBUSYor-ENODEV. Could I trigger one? Maybe. Is it likely? Doesn't seem to be.
Quoted 4 lines
It seems that getting
-EIOwould indicate we got ourselves out of sync, and started buttoning up the device again (or never having opened it in the first place), so a unit exception might be valid as a "hey something's screwy here" ?
I guess we have to report *some* kind of failure, and an unit exception is perhaps the least weird way to do so. So probably be best to just leave it alone, as it seems near impossible to trigger anyway.
Quoted 16 lines
Quoted 16 lines
Another problem is that
-EIOmight signal "something went wrong in the kernel code" - should not happen, but would certainly not be the caller's fault, and they can't do anything about it. That "internal error" thing might also be signaled by-EINVAL(which is odd), butDocumentation/s390/vfio-ccw.rst +0
-2-EINVAL could also mean "the ccw chain is too long", for which -EOPNOTSUPP would probably be a better return code, as it's alimitation in the code, not the architecture, IIRC. Not sure if we can still change that, though (and QEMU handles both in the same way, anyway.)
The other return codes look sane, and the return codes for the async region also seem sane (but have the same issue with
-EIO== "device in wrong state").
Quoted 8 lines
Quoted 5 lines
Looking at the QEMU handling, I think the
-EIOhandling is a bit questionable (without an obvious solution), while mapping-EBUSYto cc 2 is the only reasonable thing to do given that the interface does not differentiate between "busy" and "status pending". The rest seems sane.So maybe with all this data, and absent a better solution, it's best to just drop this patch from v3?
Yes, I agree. I'll post the documentation update as a separate patch.
Re: [RFC PATCH v2 1/7] vfio-ccw: Return IOINST_CC_NOT_OPERATIONAL for EIO
Quoted 85 lines · Cornelia Huck
Quoted 82 lines · On Mon, 6 Apr 2020 14:21:17 -0400 Eric Farman <[email protected]> wrote:
Quoted 67 lines · Cornelia Huck
Quoted 64 lines · On Wed, 25 Mar 2020 03:24:28 +0100 Halil Pasic <[email protected]> wrote:
Quoted 10 lines · On Tue, 24 Mar 2020 18:04:30 +0100 Cornelia Huck <[email protected]> wrote:
Quoted 7 lines · On Thu, 6 Feb 2020 22:45:03 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
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'.
I believe this is not the only situation when vfio-ccw returns EIO, or?
Quoted 11 lines
Quoted 11 lines
Signed-off-by Farhan Ali <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Notes:
v1->v2: [EF]
- Add s-o-b
- [Seems the discussion on v1 centered on the return code set in the kernel, rather than anything that needs to change here, unless I've missed something.]Does this need to change here? If the kernel is supposed to return ENODEV then this does not need to change.
Quoted 6 lines
I've stared at this and at the kernel code for some time again; and I'm not sure if "return
-EIO== not operational" is even true. That said, I'm not sure a unit check is the right response, either. The only thing I'm sure about is that the kernel code needs some review of return codes and some documentation...I could not agree more, this is semantically uapi and needs to be properly documented.
With regards to "linux error codes: vs "ionist cc's" an where the mapping is different example:
"""
/**
*cio_cancel_halt_clear- Cancel running I/O by performing cancel, halt
* and clear ordinally if subchannel is valid.
* @sch: subchannel on which to perform thecancel_halt_clearoperation
* @iretry: the number of the times remained to retry the next operation *
* This should be called repeatedly since halt/clear are asynchronous
* operations. We do one try withcio_cancel, three tries withcio_halt,
* 255 tries withcio_clear. The caller should initialize @iretry with
* the value 255 for its first call to this, and keep using the same
* @iretry in the subsequent calls until it gets a non-EBUSYreturn. *
* Returns 0 if device now idle,-ENODEVfor device not operational,
*-EBUSYif an interrupt is expected (either from halt/clear or from a
* status pending), and-EIOif out of retries.
*/
intcio_cancel_halt_clear(struct subchannel *sch, int *iretry)Quoted 6 lines
Quoted 2 lines
"""
Here-ENODEVis not operational.Ok, I went through the kernel code and checked which error may be returned in which case (hope I caught all of them). Here's what I currently have:
Thanks for doing all this mapping!
Quoted 5 lines
Documentation/s390/vfio-ccw.rst +0
-0
...snip...
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
The other return codes look sane, and the return codes for the async region also seem sane (but have the same issue with
-EIO== "device in wrong state").Looking at the QEMU handling, I think the
-EIOhandling is a bit questionable (without an obvious solution), while mapping-EBUSYto cc 2 is the only reasonable thing to do given that the interface does not differentiate between "busy" and "status pending". The rest seems sane.
Quoted 5 lines
Quoted 2 lines
So maybe with all this data, and absent a better solution, it's best to just drop this patch from v3?
Yes, I agree. I'll post the documentation update as a separate patch.
Sounds good; thanks!
RFC PATCH v2 2/7 linux-headers: update
In reply to Eric FarmanFrom: Farhan Ali <[email protected]>
---
Notes:
v1->v2: [EF]
- Re-ran 3 February 2020 (based on kernel tag v5.5)
v0->v1: [EF]
- Run scripts/update-linux-headers.sh properly, but do not add resulting changes to linux-headers/asm-mips/
include/standard-headers/linux/input.h | 1 + linux-headers/asm-arm64/unistd.h | 1 + linux-headers/linux/vfio.h | 3 +++ linux-headers/linux/vfio_ccw.h | 19 +++++++++++++++++++ 4 files changed, 24 insertions(+)
include/standard-headers/linux/input.h +1 -0
@@ -31,6 +31,7 @@ struct input_event {unsigned long __sec;#if defined(__sparc__) && defined(__arch64__)unsigned int __usec; +unsigned int __pad;#else unsigned long __usec;#endif
linux-headers/asm-arm64/unistd.h +1 -0
@@ -19,5 +19,6 @@#define __ARCH_WANT_NEW_STAT#define __ARCH_WANT_SET_GET_RLIMIT#define __ARCH_WANT_TIME32_SYSCALLS +#define __ARCH_WANT_SYS_CLONE3#include <asm-generic/unistd.h>
linux-headers/linux/vfio.h +3 -0
@@ -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 };
linux-headers/linux/vfio_ccw.h +19 -0
@@ -34,4 +34,23 @@ struct ccw_cmd_region {__u32 ret_code; } __attribute__((packed)); +/* +* Used for processing commands that read the subchannel-information block +* Reading this region triggers a stsch() to hardware +* Note: this is controlled by a capability +*/ +struct ccw_schib_region { +#define SCHIB_AREA_SIZE 52 +__u8 schib_area[SCHIB_AREA_SIZE ]; +} __attribute__((packed)); + +/* +* Used for returning Channel Report Word(s) to userspace. +* Note: this is controlled by a capability +*/ +struct ccw_crw_region { +__u32 crw0; +__u32 crw1; +} __attribute__((packed)); +#endif
Signature · 1 line
2.17.1
RFC PATCH v2 3/7 vfio-ccw: Refactor cleanup of regions
In reply to Eric FarmanWhile 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.
---
Notes:
v1-v2:
- Add Conny's r-b
hw/vfio/ccw.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
hw/vfio/ccw.c +9 -5
@@ -371,8 +371,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;
@@ -385,15 +384,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)
Signature · 1 line
2.17.1
RFC PATCH v2 4/7 vfio-ccw: Add support for the schib region
In reply to Eric FarmanFrom: Farhan Ali <[email protected]>
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.
---
Notes:
v1->v2:
- Remove silly variable intialization, and add a block comment, to css_do_stsch() [CH]
- Add a TODO statement to s390_ccw_store(), for myself to sort out while we go over kernel code more closely [CH/EF]
- In vfio_ccw_handle_store(),
- Set schib pointer once region is determined to be non-NULL [CH]
- Return cc=0 if pread() fails, and log an error [CH]
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 | 13 ++++++-- hw/s390x/s390-ccw.c | 28 +++++++++++++++++ hw/vfio/ccw.c | 63 +++++++++++++++++++++++++++++++++++++ include/hw/s390x/css.h | 3 +- include/hw/s390x/s390-ccw.h | 1 + target/s390x/ioinst.c | 3 +- 6 files changed, 106 insertions(+), 5 deletions(-)
hw/s390x/css.c +11 -2
@@ -1335,11 +1335,20 @@ 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; + +/* +* 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)
hw/s390x/s390-ccw.c +28 -0
@@ -51,6 +51,34 @@ 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. +* +* TODO from Conny on v1: +* "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)
hw/vfio/ccw.c +63 -0
97 lines not on this page · raw message
include/hw/s390x/css.h +2 -1
@@ -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);
include/hw/s390x/s390-ccw.h +1 -0
@@ -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
target/s390x/ioinst.c +1 -2
@@ -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 ;
Signature · 1 line
2.17.1
RFC PATCH v2 5/7 vfio-ccw: Add support for the crw region
In reply to Eric FarmanFrom: Farhan Ali <[email protected]>
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.
---
Notes:
v0->v1: [EF]
- Fixed copy/paste error in error message (s/schib/CRW)
hw/vfio/ccw.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
hw/vfio/ccw.c +18 -0
@@ -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;
@@ -450,10 +453,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);
@@ -463,6 +480,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);
Signature · 1 line
2.17.1
RFC PATCH v2 6/7 vfio-ccw: Refactor ccw irq handler
In reply to Eric FarmanMake it easier to add new ones in the future.
---
Notes:
v1->v2:
- Make irq parameter unsigned [CH]
- Remove extraneous %m from error_report calls [CH]
hw/vfio/ccw.c | 57 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 16 deletions(-)
hw/vfio/ccw.c +41 -16
@@ -335,22 +335,36 @@ 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, +unsigned 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)" , 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 ) {
@@ -358,37 +372,48 @@ 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, +unsigned 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)" , 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)
@@ -591,7 +616,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; }
@@ -620,7 +645,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);
Signature · 1 line
2.17.1
Re: [RFC PATCH v2 6/7] vfio-ccw: Refactor ccw irq handler
Quoted 5 lines · On Thu, 6 Feb 2020 22:45:08 +0100 Eric Farman <[email protected]> wrote:
Make it easier to add new ones in the future.
Signed-off-by Eric Farman <[email protected]>---
Quoted 7 lines
Notes:
v1->v2:
- Make irq parameter unsigned [CH]
- Remove extraneous %m fromerror_reportcalls [CH]hw/vfio/ccw.c | 57 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 16 deletions(-)
RFC PATCH v2 7/7 vfio-ccw: Add support for the CRW irq
In reply to Eric FarmanFrom: Farhan Ali <[email protected]>
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.
---
Notes:
v1->v2:
- Add a loop to continually read region while data is present, queueing CRWs as found [CH]
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 | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)
hw/vfio/ccw.c +51 -0
@@ -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; };
@@ -265,6 +266,40 @@ 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 rsc, erc; + +if (!event_notifier_test_and_clear (&vcdev->crw_notifier)) { +return ; + } + +do { +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); +break ; + } + +if (size ==0 || region->crw0 ==0 ) { +/* No more CRWs to queue */ +break ; + } + +memcpy (&crw, ®ion->crw0,sizeof (CRW )); + rsc = (crw.flags &0x0f00 ) >>8 ; + erc = crw.flags &0x003f ; +css_queue_crw (rsc, erc,0 ,0 , crw.rsid); + }while (1 ); +} +static void vfio_ccw_io_notifier_handler (void *opaque) {VFIOCCWDevice *vcdev = opaque;
@@ -351,6 +386,10 @@ static void vfio_ccw_register_irq_notifier(VFIOCCWDevice *vcdev, 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)" , irq);return ;
@@ -401,6 +440,9 @@ static void vfio_ccw_unregister_irq_notifier(VFIOCCWDevice *vcdev,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)" , irq);return ;
@@ -621,6 +663,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:
@@ -645,6 +695,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);
Signature · 1 line
2.17.1
Re: [RFC PATCH v2 7/7] vfio-ccw: Add support for the CRW irq
Quoted 45 lines · On Thu, 6 Feb 2020 22:45:09 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
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 <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Notes:
v1->v2:
- Add a loop to continually read region while data is present, queueing CRWs as found [CH]
v0->v1: [EF]
- Checkvcdev->crw_regionbefore 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 coolvfio_set_irq_signaling()stuff
- Unregister CRW IRQ before IO IRQ in unrealize
- s/crw1/crw0/hw/vfio/ccw.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)hw/vfio/ccw.c +13
-0@@ -265,6 +266,40 @@ 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 rsc, erc; + +if (!event_notifier_test_and_clear (&vcdev->crw_notifier)) { +return ; + } + + do {
hw/vfio/ccw.c static void vfio_ccw_reset(DeviceState *dev) · line 291
Quoted 10 lines
hw/vfio/ccw.c +10
-0+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); +break ; + } + +if (size ==0 || region->crw0 ==0 ) {
Does it make any sense to expect both of them as an indication that there are no more crws at the moment? Grabbing a zeroed crw makes the most sense as a stop condition, I think.
Also, I'm not sure anymore whether having space for two crws makes too much sense. If we have a case in the future where we get two chained crws, the code will retry anyway and just fetch the chained crw and queue it, wouldn't it?
hw/vfio/ccw.c static void vfio_ccw_reset(DeviceState *dev) · line 298
Quoted 7 lines
hw/vfio/ccw.c +7
-0+/* No more CRWs to queue */ +break ; + } + +memcpy (&crw, ®ion->crw0,sizeof (CRW )); + rsc = (crw.flags &0x0f00 ) >>8 ; + erc = crw.flags &0x003f ;
I think we already have something for that... ah yes, CRW_FLAGS_MASK_RSC and CRW_FLAGS_MASK_ERC.
hw/vfio/ccw.c static void vfio_ccw_reset(DeviceState *dev) · line 299
Quoted 1 line
hw/vfio/ccw.c +1
-0+css_queue_crw (rsc, erc,0 ,0 , crw.rsid);
...or maybe an alternative interface that allows us to queue a ready-made crw?
Quoted 6 lines
hw/vfio/ccw.c +3
-0+ }while (1 ); +} +static void vfio_ccw_io_notifier_handler (void *opaque) {VFIOCCWDevice *vcdev = opaque;
Re: [RFC PATCH v2 7/7] vfio-ccw: Add support for the CRW irq
Quoted 54 lines · Cornelia Huck
Quoted 51 lines · On Thu, 6 Feb 2020 22:45:09 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
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 <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Notes:
v1->v2:
- Add a loop to continually read region while data is present, queueing CRWs as found [CH]
v0->v1: [EF]
- Checkvcdev->crw_regionbefore 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 coolvfio_set_irq_signaling()stuff
- Unregister CRW IRQ before IO IRQ in unrealize
- s/crw1/crw0/hw/vfio/ccw.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)hw/vfio/ccw.c +19
-0@@ -265,6 +266,40 @@ 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 rsc, erc; + +if (!event_notifier_test_and_clear (&vcdev->crw_notifier)) { +return ; + } + + do { + 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);
Quoted 8 lines
hw/vfio/ccw.c static void vfio_ccw_reset(DeviceState *dev) · line 291
Quoted 4 lines
hw/vfio/ccw.c +4
-0+break ; + } + +if (size ==0 || region->crw0 ==0 ) {Does it make any sense to expect both of them as an indication that there are no more crws at the moment? Grabbing a zeroed crw makes the most sense as a stop condition, I think.
I think it was overkill on my part. Though it appears I am missing the "zeroing" of the region once it got read. Whoops. Okay, those are easy fixups.
Quoted 5 lines
Also, I'm not sure anymore whether having space for two crws makes too much sense. If we have a case in the future where we get two chained crws, the code will retry anyway and just fetch the chained crw and queue it, wouldn't it?
I suppose.
I thought the reason for including them now was to avoid "if region size == 4 vs 8 vs XX" logic at some mysterious time in the future. But certainly ripping it out so we only pass a single CRW at a time would simplify this quite a bit.
Quoted 11 lines
hw/vfio/ccw.c static void vfio_ccw_reset(DeviceState *dev) · line 298
Quoted 8 lines
hw/vfio/ccw.c +7
-0+/* No more CRWs to queue */ +break ; + } + +memcpy (&crw, ®ion->crw0,sizeof (CRW )); + rsc = (crw.flags &0x0f00 ) >>8 ; + erc = crw.flags &0x003f ;I think we already have something for that... ah yes,
CRW_FLAGS_MASK_RSCandCRW_FLAGS_MASK_ERC.
Huh, look at that. :)
Quoted 5 lines
hw/vfio/ccw.c static void vfio_ccw_reset(DeviceState *dev) · line 299
Quoted 2 lines
hw/vfio/ccw.c +1
-0+css_queue_crw (rsc, erc,0 ,0 , crw.rsid);...or maybe an alternative interface that allows us to queue a ready-made crw?
Sure, that would be nice. I'll add that as an additional patch to this series, prior to this one.
Quoted 8 lines
Quoted 7 lines
hw/vfio/ccw.c +3
-0+ }while (1 ); +} +static void vfio_ccw_io_notifier_handler (void *opaque) {VFIOCCWDevice *vcdev = opaque;
Re: [RFC PATCH v2 7/7] vfio-ccw: Add support for the CRW irq
Quoted 64 lines · On Mon, 6 Apr 2020 17:37:18 -0400 Eric Farman <[email protected]> wrote:
Quoted 54 lines · Cornelia Huck
Quoted 51 lines · On Thu, 6 Feb 2020 22:45:09 +0100 Eric Farman <[email protected]> wrote:
From: Farhan Ali <[email protected]>
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 <[email protected]>Signed-off-by Eric Farman <[email protected]>---
Notes:
v1->v2:
- Add a loop to continually read region while data is present, queueing CRWs as found [CH]
v0->v1: [EF]
- Checkvcdev->crw_regionbefore 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 coolvfio_set_irq_signaling()stuff
- Unregister CRW IRQ before IO IRQ in unrealize
- s/crw1/crw0/hw/vfio/ccw.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)hw/vfio/ccw.c +19
-0@@ -265,6 +266,40 @@ 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 rsc, erc; + +if (!event_notifier_test_and_clear (&vcdev->crw_notifier)) { +return ; + } + + do { + 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);Quoted 8 lines
hw/vfio/ccw.c static void vfio_ccw_reset(DeviceState *dev) · line 291
Quoted 4 lines
hw/vfio/ccw.c +4
-0+break ; + } + +if (size ==0 || region->crw0 ==0 ) {Does it make any sense to expect both of them as an indication that there are no more crws at the moment? Grabbing a zeroed crw makes the most sense as a stop condition, I think.
Quoted 3 lines
I think it was overkill on my part. Though it appears I am missing the "zeroing" of the region once it got read. Whoops. Okay, those are easy fixups.
Yes, just looking at the zeroed region (after changing the kernel part) seems like the right thing here.
Quoted 7 lines
Quoted 6 lines
Also, I'm not sure anymore whether having space for two crws makes too much sense. If we have a case in the future where we get two chained crws, the code will retry anyway and just fetch the chained crw and queue it, wouldn't it?
Quoted 6 lines
I suppose.
I thought the reason for including them now was to avoid "if region size == 4 vs 8 vs XX" logic at some mysterious time in the future. But certainly ripping it out so we only pass a single CRW at a time would simplify this quite a bit.
Yes, injecting in a loop is easy anyway.
Quoted 16 lines
Quoted 10 lines
hw/vfio/ccw.c static void vfio_ccw_reset(DeviceState *dev) · line 298
Quoted 9 lines
hw/vfio/ccw.c +7
-0+/* No more CRWs to queue */ +break ; + } + +memcpy (&crw, ®ion->crw0,sizeof (CRW )); + rsc = (crw.flags &0x0f00 ) >>8 ; + erc = crw.flags &0x003f ;Quoted 2 lines
I think we already have something for that... ah yes,
CRW_FLAGS_MASK_RSCandCRW_FLAGS_MASK_ERC.Huh, look at that. :)
Quoted 1 line
Quoted 7 lines
Quoted 4 lines
hw/vfio/ccw.c static void vfio_ccw_reset(DeviceState *dev) · line 299
Quoted 1 line
hw/vfio/ccw.c +1
-0+css_queue_crw (rsc, erc,0 ,0 , crw.rsid);...or maybe an alternative interface that allows us to queue a ready-made crw?
Sure, that would be nice. I'll add that as an additional patch to this series, prior to this one.
Agreed, makes sense.
Quoted 10 lines
Quoted 9 lines
Quoted 8 lines
hw/vfio/ccw.c +3
-0+ }while (1 ); +} +static void vfio_ccw_io_notifier_handler (void *opaque) {VFIOCCWDevice *vcdev = opaque;