* [PATCH v3 1/8] vfio-ccw: Introduce new helper functions to free/destroy regions
2020-04-17 2:29 [PATCH v3 0/8] s390x/vfio-ccw: Channel Path Handling [KVM] Eric Farman
@ 2020-04-17 2:29 ` Eric Farman
2020-04-17 2:29 ` [PATCH v3 2/8] vfio-ccw: Register a chp_event callback for vfio-ccw Eric Farman
` (7 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Eric Farman @ 2020-04-17 2:29 UTC (permalink / raw)
To: linux-s390, kvm
Cc: Cornelia Huck, Halil Pasic, Jason Herne, Jared Rossi, Eric Farman
From: Farhan Ali <alifm@linux.ibm.com>
Consolidate some of the cleanup code for the regions, so that
as more are added we reduce code duplication.
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
Notes:
v1->v2:
- Add Conny's r-b
v0->v1: [EF]
- Commit message
drivers/s390/cio/vfio_ccw_drv.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 339a6bc0339b..8715c1c2f1e1 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -116,6 +116,14 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
}
+static void vfio_ccw_free_regions(struct vfio_ccw_private *private)
+{
+ if (private->cmd_region)
+ kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
+ if (private->io_region)
+ kmem_cache_free(vfio_ccw_io_region, private->io_region);
+}
+
static int vfio_ccw_sch_probe(struct subchannel *sch)
{
struct pmcw *pmcw = &sch->schib.pmcw;
@@ -181,10 +189,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
cio_disable_subchannel(sch);
out_free:
dev_set_drvdata(&sch->dev, NULL);
- if (private->cmd_region)
- kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
- if (private->io_region)
- kmem_cache_free(vfio_ccw_io_region, private->io_region);
+ vfio_ccw_free_regions(private);
kfree(private->cp.guest_cp);
kfree(private);
return ret;
@@ -200,8 +205,7 @@ static int vfio_ccw_sch_remove(struct subchannel *sch)
dev_set_drvdata(&sch->dev, NULL);
- kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
- kmem_cache_free(vfio_ccw_io_region, private->io_region);
+ vfio_ccw_free_regions(private);
kfree(private->cp.guest_cp);
kfree(private);
@@ -304,6 +308,12 @@ static void vfio_ccw_debug_exit(void)
debug_unregister(vfio_ccw_debug_trace_id);
}
+static void vfio_ccw_destroy_regions(void)
+{
+ kmem_cache_destroy(vfio_ccw_cmd_region);
+ kmem_cache_destroy(vfio_ccw_io_region);
+}
+
static int __init vfio_ccw_sch_init(void)
{
int ret;
@@ -346,8 +356,7 @@ static int __init vfio_ccw_sch_init(void)
return ret;
out_err:
- kmem_cache_destroy(vfio_ccw_cmd_region);
- kmem_cache_destroy(vfio_ccw_io_region);
+ vfio_ccw_destroy_regions();
destroy_workqueue(vfio_ccw_work_q);
vfio_ccw_debug_exit();
return ret;
@@ -357,8 +366,7 @@ static void __exit vfio_ccw_sch_exit(void)
{
css_driver_unregister(&vfio_ccw_sch_driver);
isc_unregister(VFIO_CCW_ISC);
- kmem_cache_destroy(vfio_ccw_io_region);
- kmem_cache_destroy(vfio_ccw_cmd_region);
+ vfio_ccw_destroy_regions();
destroy_workqueue(vfio_ccw_work_q);
vfio_ccw_debug_exit();
}
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 2/8] vfio-ccw: Register a chp_event callback for vfio-ccw
2020-04-17 2:29 [PATCH v3 0/8] s390x/vfio-ccw: Channel Path Handling [KVM] Eric Farman
2020-04-17 2:29 ` [PATCH v3 1/8] vfio-ccw: Introduce new helper functions to free/destroy regions Eric Farman
@ 2020-04-17 2:29 ` Eric Farman
2020-04-17 10:29 ` Cornelia Huck
2020-04-17 2:29 ` [PATCH v3 3/8] vfio-ccw: Refactor the unregister of the async regions Eric Farman
` (6 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Eric Farman @ 2020-04-17 2:29 UTC (permalink / raw)
To: linux-s390, kvm
Cc: Cornelia Huck, Halil Pasic, Jason Herne, Jared Rossi, Eric Farman
From: Farhan Ali <alifm@linux.ibm.com>
Register the chp_event callback to receive channel path related
events for the subchannels managed by vfio-ccw.
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
Notes:
v2->v3:
- Add a call to cio_cancel_halt_clear() for CHP_VARY_OFF [CH]
v1->v2:
- Move s390dbf before cio_update_schib() call [CH]
v0->v1: [EF]
- Add s390dbf trace
drivers/s390/cio/vfio_ccw_drv.c | 45 +++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 8715c1c2f1e1..e48967c475e7 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -19,6 +19,7 @@
#include <asm/isc.h>
+#include "chp.h"
#include "ioasm.h"
#include "css.h"
#include "vfio_ccw_private.h"
@@ -262,6 +263,49 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
return rc;
}
+static int vfio_ccw_chp_event(struct subchannel *sch,
+ struct chp_link *link, int event)
+{
+ struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
+ int mask = chp_ssd_get_mask(&sch->ssd_info, link);
+ int retry = 255;
+
+ if (!private || !mask)
+ return 0;
+
+ VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
+ mdev_uuid(private->mdev), sch->schid.cssid,
+ sch->schid.ssid, sch->schid.sch_no,
+ mask, event);
+
+ if (cio_update_schib(sch))
+ return -ENODEV;
+
+ switch (event) {
+ case CHP_VARY_OFF:
+ /* Path logically turned off */
+ sch->opm &= ~mask;
+ sch->lpm &= ~mask;
+ cio_cancel_halt_clear(sch, &retry);
+ break;
+ case CHP_OFFLINE:
+ /* Path is gone */
+ cio_cancel_halt_clear(sch, &retry);
+ break;
+ case CHP_VARY_ON:
+ /* Path logically turned on */
+ sch->opm |= mask;
+ sch->lpm |= mask;
+ break;
+ case CHP_ONLINE:
+ /* Path became available */
+ sch->lpm |= mask & sch->opm;
+ break;
+ }
+
+ return 0;
+}
+
static struct css_device_id vfio_ccw_sch_ids[] = {
{ .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, },
{ /* end of list */ },
@@ -279,6 +323,7 @@ static struct css_driver vfio_ccw_sch_driver = {
.remove = vfio_ccw_sch_remove,
.shutdown = vfio_ccw_sch_shutdown,
.sch_event = vfio_ccw_sch_event,
+ .chp_event = vfio_ccw_chp_event,
};
static int __init vfio_ccw_debug_init(void)
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 2/8] vfio-ccw: Register a chp_event callback for vfio-ccw
2020-04-17 2:29 ` [PATCH v3 2/8] vfio-ccw: Register a chp_event callback for vfio-ccw Eric Farman
@ 2020-04-17 10:29 ` Cornelia Huck
2020-04-17 12:38 ` Eric Farman
0 siblings, 1 reply; 21+ messages in thread
From: Cornelia Huck @ 2020-04-17 10:29 UTC (permalink / raw)
To: Eric Farman; +Cc: linux-s390, kvm, Halil Pasic, Jason Herne, Jared Rossi
On Fri, 17 Apr 2020 04:29:55 +0200
Eric Farman <farman@linux.ibm.com> wrote:
> From: Farhan Ali <alifm@linux.ibm.com>
>
> Register the chp_event callback to receive channel path related
> events for the subchannels managed by vfio-ccw.
>
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>
> Notes:
> v2->v3:
> - Add a call to cio_cancel_halt_clear() for CHP_VARY_OFF [CH]
>
> v1->v2:
> - Move s390dbf before cio_update_schib() call [CH]
>
> v0->v1: [EF]
> - Add s390dbf trace
>
> drivers/s390/cio/vfio_ccw_drv.c | 45 +++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 8715c1c2f1e1..e48967c475e7 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -19,6 +19,7 @@
>
> #include <asm/isc.h>
>
> +#include "chp.h"
> #include "ioasm.h"
> #include "css.h"
> #include "vfio_ccw_private.h"
> @@ -262,6 +263,49 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
> return rc;
> }
>
> +static int vfio_ccw_chp_event(struct subchannel *sch,
> + struct chp_link *link, int event)
> +{
> + struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
> + int mask = chp_ssd_get_mask(&sch->ssd_info, link);
> + int retry = 255;
> +
> + if (!private || !mask)
> + return 0;
> +
> + VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
> + mdev_uuid(private->mdev), sch->schid.cssid,
> + sch->schid.ssid, sch->schid.sch_no,
> + mask, event);
> +
> + if (cio_update_schib(sch))
> + return -ENODEV;
> +
> + switch (event) {
> + case CHP_VARY_OFF:
> + /* Path logically turned off */
> + sch->opm &= ~mask;
> + sch->lpm &= ~mask;
> + cio_cancel_halt_clear(sch, &retry);
> + break;
> + case CHP_OFFLINE:
> + /* Path is gone */
> + cio_cancel_halt_clear(sch, &retry);
Looking at this again: While calling the same function for both
CHP_VARY_OFF and CHP_OFFLINE is the right thing to do,
cio_cancel_halt_clear() is probably not that function. I don't think we
want to terminate an I/O that does not use the affected path.
Looking at what the normal I/O subchannel driver does, it first checks
for the lpum and does not do anything if the affected path does not
show up there. Following down the git history rabbit hole, that basic
check (surviving several reworks) precedes the first git import, so
there's unfortunately no patch description explaining that. Looking at
the PoP, I cannot find a whole lot of details... I think some of the
path-handling stuff is explained in non-public documentation, and
whoever wrote the original code (probably me) relied on the information
there.
tl;dr: We probably should check the lpum here as well.
> + break;
> + case CHP_VARY_ON:
> + /* Path logically turned on */
> + sch->opm |= mask;
> + sch->lpm |= mask;
> + break;
> + case CHP_ONLINE:
> + /* Path became available */
> + sch->lpm |= mask & sch->opm;
> + break;
> + }
> +
> + return 0;
> +}
> +
> static struct css_device_id vfio_ccw_sch_ids[] = {
> { .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, },
> { /* end of list */ },
> @@ -279,6 +323,7 @@ static struct css_driver vfio_ccw_sch_driver = {
> .remove = vfio_ccw_sch_remove,
> .shutdown = vfio_ccw_sch_shutdown,
> .sch_event = vfio_ccw_sch_event,
> + .chp_event = vfio_ccw_chp_event,
> };
>
> static int __init vfio_ccw_debug_init(void)
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 2/8] vfio-ccw: Register a chp_event callback for vfio-ccw
2020-04-17 10:29 ` Cornelia Huck
@ 2020-04-17 12:38 ` Eric Farman
0 siblings, 0 replies; 21+ messages in thread
From: Eric Farman @ 2020-04-17 12:38 UTC (permalink / raw)
To: Cornelia Huck; +Cc: linux-s390, kvm, Halil Pasic, Jason Herne, Jared Rossi
On 4/17/20 6:29 AM, Cornelia Huck wrote:
> On Fri, 17 Apr 2020 04:29:55 +0200
> Eric Farman <farman@linux.ibm.com> wrote:
>
>> From: Farhan Ali <alifm@linux.ibm.com>
>>
>> Register the chp_event callback to receive channel path related
>> events for the subchannels managed by vfio-ccw.
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>
>> Notes:
>> v2->v3:
>> - Add a call to cio_cancel_halt_clear() for CHP_VARY_OFF [CH]
>>
>> v1->v2:
>> - Move s390dbf before cio_update_schib() call [CH]
>>
>> v0->v1: [EF]
>> - Add s390dbf trace
>>
>> drivers/s390/cio/vfio_ccw_drv.c | 45 +++++++++++++++++++++++++++++++++
>> 1 file changed, 45 insertions(+)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
>> index 8715c1c2f1e1..e48967c475e7 100644
>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>> @@ -19,6 +19,7 @@
>>
>> #include <asm/isc.h>
>>
>> +#include "chp.h"
>> #include "ioasm.h"
>> #include "css.h"
>> #include "vfio_ccw_private.h"
>> @@ -262,6 +263,49 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
>> return rc;
>> }
>>
>> +static int vfio_ccw_chp_event(struct subchannel *sch,
>> + struct chp_link *link, int event)
>> +{
>> + struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>> + int mask = chp_ssd_get_mask(&sch->ssd_info, link);
>> + int retry = 255;
>> +
>> + if (!private || !mask)
>> + return 0;
>> +
>> + VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
>> + mdev_uuid(private->mdev), sch->schid.cssid,
>> + sch->schid.ssid, sch->schid.sch_no,
>> + mask, event);
>> +
>> + if (cio_update_schib(sch))
>> + return -ENODEV;
>> +
>> + switch (event) {
>> + case CHP_VARY_OFF:
>> + /* Path logically turned off */
>> + sch->opm &= ~mask;
>> + sch->lpm &= ~mask;
>> + cio_cancel_halt_clear(sch, &retry);
>> + break;
>> + case CHP_OFFLINE:
>> + /* Path is gone */
>> + cio_cancel_halt_clear(sch, &retry);
>
> Looking at this again: While calling the same function for both
> CHP_VARY_OFF and CHP_OFFLINE is the right thing to do,
> cio_cancel_halt_clear() is probably not that function. I don't think we
> want to terminate an I/O that does not use the affected path.
>
> Looking at what the normal I/O subchannel driver does, it first checks
> for the lpum and does not do anything if the affected path does not
> show up there. Following down the git history rabbit hole, that basic
> check (surviving several reworks) precedes the first git import, so
> there's unfortunately no patch description explaining that. Looking at
> the PoP, I cannot find a whole lot of details... I think some of the
> path-handling stuff is explained in non-public documentation, and
> whoever wrote the original code (probably me) relied on the information
> there.
>
> tl;dr: We probably should check the lpum here as well.
Makes sense. I'll go through that other doc and fix this up accordingly.
>
>> + break;
>> + case CHP_VARY_ON:
>> + /* Path logically turned on */
>> + sch->opm |= mask;
>> + sch->lpm |= mask;
>> + break;
>> + case CHP_ONLINE:
>> + /* Path became available */
>> + sch->lpm |= mask & sch->opm;
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static struct css_device_id vfio_ccw_sch_ids[] = {
>> { .match_flags = 0x1, .type = SUBCHANNEL_TYPE_IO, },
>> { /* end of list */ },
>> @@ -279,6 +323,7 @@ static struct css_driver vfio_ccw_sch_driver = {
>> .remove = vfio_ccw_sch_remove,
>> .shutdown = vfio_ccw_sch_shutdown,
>> .sch_event = vfio_ccw_sch_event,
>> + .chp_event = vfio_ccw_chp_event,
>> };
>>
>> static int __init vfio_ccw_debug_init(void)
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 3/8] vfio-ccw: Refactor the unregister of the async regions
2020-04-17 2:29 [PATCH v3 0/8] s390x/vfio-ccw: Channel Path Handling [KVM] Eric Farman
2020-04-17 2:29 ` [PATCH v3 1/8] vfio-ccw: Introduce new helper functions to free/destroy regions Eric Farman
2020-04-17 2:29 ` [PATCH v3 2/8] vfio-ccw: Register a chp_event callback for vfio-ccw Eric Farman
@ 2020-04-17 2:29 ` Eric Farman
2020-04-17 2:29 ` [PATCH v3 4/8] vfio-ccw: Introduce a new schib region Eric Farman
` (5 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Eric Farman @ 2020-04-17 2:29 UTC (permalink / raw)
To: linux-s390, kvm
Cc: Cornelia Huck, Halil Pasic, Jason Herne, Jared Rossi, Eric Farman
This is mostly for the purposes of a later patch, since
we'll need to do the same thing later.
While we are at it, move the resulting function call to ahead
of the unregistering of the IOMMU notifier, so that it's done
in the reverse order of how it was created.
Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
Notes:
v1->v2:
- Add Conny's r-b
drivers/s390/cio/vfio_ccw_ops.c | 20 ++++++++++++--------
drivers/s390/cio/vfio_ccw_private.h | 1 +
2 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index f0d71ab77c50..d4fc84b8867f 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -181,7 +181,6 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev)
{
struct vfio_ccw_private *private =
dev_get_drvdata(mdev_parent_dev(mdev));
- int i;
if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
(private->state != VFIO_CCW_STATE_STANDBY)) {
@@ -191,15 +190,9 @@ static void vfio_ccw_mdev_release(struct mdev_device *mdev)
}
cp_free(&private->cp);
+ vfio_ccw_unregister_dev_regions(private);
vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
&private->nb);
-
- for (i = 0; i < private->num_regions; i++)
- private->region[i].ops->release(private, &private->region[i]);
-
- private->num_regions = 0;
- kfree(private->region);
- private->region = NULL;
}
static ssize_t vfio_ccw_mdev_read_io_region(struct vfio_ccw_private *private,
@@ -482,6 +475,17 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,
return 0;
}
+void vfio_ccw_unregister_dev_regions(struct vfio_ccw_private *private)
+{
+ int i;
+
+ for (i = 0; i < private->num_regions; i++)
+ private->region[i].ops->release(private, &private->region[i]);
+ private->num_regions = 0;
+ kfree(private->region);
+ private->region = NULL;
+}
+
static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,
unsigned int cmd,
unsigned long arg)
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 9b9bb4982972..ce3834159d98 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -53,6 +53,7 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,
unsigned int subtype,
const struct vfio_ccw_regops *ops,
size_t size, u32 flags, void *data);
+void vfio_ccw_unregister_dev_regions(struct vfio_ccw_private *private);
int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 4/8] vfio-ccw: Introduce a new schib region
2020-04-17 2:29 [PATCH v3 0/8] s390x/vfio-ccw: Channel Path Handling [KVM] Eric Farman
` (2 preceding siblings ...)
2020-04-17 2:29 ` [PATCH v3 3/8] vfio-ccw: Refactor the unregister of the async regions Eric Farman
@ 2020-04-17 2:29 ` Eric Farman
2020-04-21 9:24 ` Cornelia Huck
2020-04-17 2:29 ` [PATCH v3 5/8] vfio-ccw: Introduce a new CRW region Eric Farman
` (4 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Eric Farman @ 2020-04-17 2:29 UTC (permalink / raw)
To: linux-s390, kvm
Cc: Cornelia Huck, Halil Pasic, Jason Herne, Jared Rossi, Eric Farman
From: Farhan Ali <alifm@linux.ibm.com>
The schib region can be used by userspace to get the subchannel-
information block (SCHIB) for the passthrough subchannel.
This can be useful to get information such as channel path
information via the SCHIB.PMCW fields.
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
Notes:
v2->v3:
- Updated Copyright year and Authors [CH]
- Mention that schib region triggers a STSCH [CH]
v1->v2:
- Add new region info to Documentation/s390/vfio-ccw.rst [CH]
- Add a block comment to struct ccw_schib_region [CH]
v0->v1: [EF]
- Clean up checkpatch (#include, whitespace) errors
- Remove unnecessary includes from vfio_ccw_chp.c
- Add ret=-ENOMEM in error path for new region
- Add call to vfio_ccw_unregister_dev_regions() during error exit
path of vfio_ccw_mdev_open()
- New info on the module prologue
- Reorder cleanup of regions
Documentation/s390/vfio-ccw.rst | 19 +++++++-
drivers/s390/cio/Makefile | 2 +-
drivers/s390/cio/vfio_ccw_chp.c | 76 +++++++++++++++++++++++++++++
drivers/s390/cio/vfio_ccw_drv.c | 20 ++++++++
drivers/s390/cio/vfio_ccw_ops.c | 14 +++++-
drivers/s390/cio/vfio_ccw_private.h | 3 ++
include/uapi/linux/vfio.h | 1 +
include/uapi/linux/vfio_ccw.h | 10 ++++
8 files changed, 141 insertions(+), 4 deletions(-)
create mode 100644 drivers/s390/cio/vfio_ccw_chp.c
diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst
index fca9c4f5bd9c..98832d95f395 100644
--- a/Documentation/s390/vfio-ccw.rst
+++ b/Documentation/s390/vfio-ccw.rst
@@ -231,6 +231,22 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD.
Currently, CLEAR SUBCHANNEL and HALT SUBCHANNEL use this region.
+vfio-ccw schib region
+---------------------
+
+The vfio-ccw schib region is used to return Subchannel-Information
+Block (SCHIB) data to userspace::
+
+ struct ccw_schib_region {
+ #define SCHIB_AREA_SIZE 52
+ __u8 schib_area[SCHIB_AREA_SIZE];
+ } __packed;
+
+This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB.
+
+Reading this region triggers a STORE SUBCHANNEL to be issued to the
+associated hardware.
+
vfio-ccw operation details
--------------------------
@@ -333,7 +349,8 @@ through DASD/ECKD device online in a guest now and use it as a block
device.
The current code allows the guest to start channel programs via
-START SUBCHANNEL, and to issue HALT SUBCHANNEL and CLEAR SUBCHANNEL.
+START SUBCHANNEL, and to issue HALT SUBCHANNEL, CLEAR SUBCHANNEL,
+and STORE SUBCHANNEL.
vfio-ccw supports classic (command mode) channel I/O only. Transport
mode (HPF) is not supported.
diff --git a/drivers/s390/cio/Makefile b/drivers/s390/cio/Makefile
index 23eae4188876..a9235f111e79 100644
--- a/drivers/s390/cio/Makefile
+++ b/drivers/s390/cio/Makefile
@@ -21,5 +21,5 @@ qdio-objs := qdio_main.o qdio_thinint.o qdio_debug.o qdio_setup.o
obj-$(CONFIG_QDIO) += qdio.o
vfio_ccw-objs += vfio_ccw_drv.o vfio_ccw_cp.o vfio_ccw_ops.o vfio_ccw_fsm.o \
- vfio_ccw_async.o vfio_ccw_trace.o
+ vfio_ccw_async.o vfio_ccw_trace.o vfio_ccw_chp.o
obj-$(CONFIG_VFIO_CCW) += vfio_ccw.o
diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
new file mode 100644
index 000000000000..18f3b3e873a9
--- /dev/null
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Channel path related status regions for vfio_ccw
+ *
+ * Copyright IBM Corp. 2020
+ *
+ * Author(s): Farhan Ali <alifm@linux.ibm.com>
+ * Eric Farman <farman@linux.ibm.com>
+ */
+
+#include <linux/vfio.h>
+#include "vfio_ccw_private.h"
+
+static ssize_t vfio_ccw_schib_region_read(struct vfio_ccw_private *private,
+ char __user *buf, size_t count,
+ loff_t *ppos)
+{
+ unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+ loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+ struct ccw_schib_region *region;
+ int ret;
+
+ if (pos + count > sizeof(*region))
+ return -EINVAL;
+
+ mutex_lock(&private->io_mutex);
+ region = private->region[i].data;
+
+ if (cio_update_schib(private->sch)) {
+ ret = -ENODEV;
+ goto out;
+ }
+
+ memcpy(region, &private->sch->schib, sizeof(*region));
+
+ if (copy_to_user(buf, (void *)region + pos, count)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ ret = count;
+
+out:
+ mutex_unlock(&private->io_mutex);
+ return ret;
+}
+
+static ssize_t vfio_ccw_schib_region_write(struct vfio_ccw_private *private,
+ const char __user *buf, size_t count,
+ loff_t *ppos)
+{
+ return -EINVAL;
+}
+
+
+static void vfio_ccw_schib_region_release(struct vfio_ccw_private *private,
+ struct vfio_ccw_region *region)
+{
+
+}
+
+const struct vfio_ccw_regops vfio_ccw_schib_region_ops = {
+ .read = vfio_ccw_schib_region_read,
+ .write = vfio_ccw_schib_region_write,
+ .release = vfio_ccw_schib_region_release,
+};
+
+int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private)
+{
+ return vfio_ccw_register_dev_region(private,
+ VFIO_REGION_SUBTYPE_CCW_SCHIB,
+ &vfio_ccw_schib_region_ops,
+ sizeof(struct ccw_schib_region),
+ VFIO_REGION_INFO_FLAG_READ,
+ private->schib_region);
+}
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index e48967c475e7..8e05ed611c10 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -27,6 +27,7 @@
struct workqueue_struct *vfio_ccw_work_q;
static struct kmem_cache *vfio_ccw_io_region;
static struct kmem_cache *vfio_ccw_cmd_region;
+static struct kmem_cache *vfio_ccw_schib_region;
debug_info_t *vfio_ccw_debug_msg_id;
debug_info_t *vfio_ccw_debug_trace_id;
@@ -119,6 +120,8 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
static void vfio_ccw_free_regions(struct vfio_ccw_private *private)
{
+ if (private->schib_region)
+ kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
if (private->cmd_region)
kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
if (private->io_region)
@@ -156,6 +159,12 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
if (!private->cmd_region)
goto out_free;
+ private->schib_region = kmem_cache_zalloc(vfio_ccw_schib_region,
+ GFP_KERNEL | GFP_DMA);
+
+ if (!private->schib_region)
+ goto out_free;
+
private->sch = sch;
dev_set_drvdata(&sch->dev, private);
mutex_init(&private->io_mutex);
@@ -355,6 +364,7 @@ static void vfio_ccw_debug_exit(void)
static void vfio_ccw_destroy_regions(void)
{
+ kmem_cache_destroy(vfio_ccw_schib_region);
kmem_cache_destroy(vfio_ccw_cmd_region);
kmem_cache_destroy(vfio_ccw_io_region);
}
@@ -391,6 +401,16 @@ static int __init vfio_ccw_sch_init(void)
goto out_err;
}
+ vfio_ccw_schib_region = kmem_cache_create_usercopy("vfio_ccw_schib_region",
+ sizeof(struct ccw_schib_region), 0,
+ SLAB_ACCOUNT, 0,
+ sizeof(struct ccw_schib_region), NULL);
+
+ if (!vfio_ccw_schib_region) {
+ ret = -ENOMEM;
+ goto out_err;
+ }
+
isc_register(VFIO_CCW_ISC);
ret = css_driver_register(&vfio_ccw_sch_driver);
if (ret) {
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index d4fc84b8867f..22988d67b6bb 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -172,8 +172,18 @@ static int vfio_ccw_mdev_open(struct mdev_device *mdev)
ret = vfio_ccw_register_async_dev_regions(private);
if (ret)
- vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
- &private->nb);
+ goto out_unregister;
+
+ ret = vfio_ccw_register_schib_dev_regions(private);
+ if (ret)
+ goto out_unregister;
+
+ return ret;
+
+out_unregister:
+ vfio_ccw_unregister_dev_regions(private);
+ vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
+ &private->nb);
return ret;
}
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index ce3834159d98..d6601a8adf13 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -56,6 +56,7 @@ int vfio_ccw_register_dev_region(struct vfio_ccw_private *private,
void vfio_ccw_unregister_dev_regions(struct vfio_ccw_private *private);
int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
+int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private);
/**
* struct vfio_ccw_private
@@ -69,6 +70,7 @@ int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
* @io_mutex: protect against concurrent update of I/O regions
* @region: additional regions for other subchannel operations
* @cmd_region: MMIO region for asynchronous I/O commands other than START
+ * @schib_region: MMIO region for SCHIB information
* @num_regions: number of additional regions
* @cp: channel program for the current I/O operation
* @irb: irb info received from interrupt
@@ -87,6 +89,7 @@ struct vfio_ccw_private {
struct mutex io_mutex;
struct vfio_ccw_region *region;
struct ccw_cmd_region *cmd_region;
+ struct ccw_schib_region *schib_region;
int num_regions;
struct channel_program cp;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 015516bcfaa3..7a1abbd889bd 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -378,6 +378,7 @@ 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)
/*
* The MSIX mappable capability informs that MSIX data of a BAR can be mmapped
diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h
index cbecbf0cd54f..758bf214898d 100644
--- a/include/uapi/linux/vfio_ccw.h
+++ b/include/uapi/linux/vfio_ccw.h
@@ -34,4 +34,14 @@ struct ccw_cmd_region {
__u32 ret_code;
} __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];
+} __packed;
+
#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 4/8] vfio-ccw: Introduce a new schib region
2020-04-17 2:29 ` [PATCH v3 4/8] vfio-ccw: Introduce a new schib region Eric Farman
@ 2020-04-21 9:24 ` Cornelia Huck
0 siblings, 0 replies; 21+ messages in thread
From: Cornelia Huck @ 2020-04-21 9:24 UTC (permalink / raw)
To: Eric Farman; +Cc: linux-s390, kvm, Halil Pasic, Jason Herne, Jared Rossi
On Fri, 17 Apr 2020 04:29:57 +0200
Eric Farman <farman@linux.ibm.com> wrote:
> From: Farhan Ali <alifm@linux.ibm.com>
>
> The schib region can be used by userspace to get the subchannel-
> information block (SCHIB) for the passthrough subchannel.
> This can be useful to get information such as channel path
> information via the SCHIB.PMCW fields.
>
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>
> Notes:
> v2->v3:
> - Updated Copyright year and Authors [CH]
> - Mention that schib region triggers a STSCH [CH]
>
> v1->v2:
> - Add new region info to Documentation/s390/vfio-ccw.rst [CH]
> - Add a block comment to struct ccw_schib_region [CH]
>
> v0->v1: [EF]
> - Clean up checkpatch (#include, whitespace) errors
> - Remove unnecessary includes from vfio_ccw_chp.c
> - Add ret=-ENOMEM in error path for new region
> - Add call to vfio_ccw_unregister_dev_regions() during error exit
> path of vfio_ccw_mdev_open()
> - New info on the module prologue
> - Reorder cleanup of regions
>
> Documentation/s390/vfio-ccw.rst | 19 +++++++-
> drivers/s390/cio/Makefile | 2 +-
> drivers/s390/cio/vfio_ccw_chp.c | 76 +++++++++++++++++++++++++++++
> drivers/s390/cio/vfio_ccw_drv.c | 20 ++++++++
> drivers/s390/cio/vfio_ccw_ops.c | 14 +++++-
> drivers/s390/cio/vfio_ccw_private.h | 3 ++
> include/uapi/linux/vfio.h | 1 +
> include/uapi/linux/vfio_ccw.h | 10 ++++
> 8 files changed, 141 insertions(+), 4 deletions(-)
> create mode 100644 drivers/s390/cio/vfio_ccw_chp.c
>
(...)
> +static ssize_t vfio_ccw_schib_region_read(struct vfio_ccw_private *private,
> + char __user *buf, size_t count,
> + loff_t *ppos)
> +{
> + unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
> + loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
> + struct ccw_schib_region *region;
> + int ret;
> +
> + if (pos + count > sizeof(*region))
> + return -EINVAL;
> +
> + mutex_lock(&private->io_mutex);
> + region = private->region[i].data;
> +
> + if (cio_update_schib(private->sch)) {
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + memcpy(region, &private->sch->schib, sizeof(*region));
> +
> + if (copy_to_user(buf, (void *)region + pos, count)) {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + ret = count;
> +
> +out:
> + mutex_unlock(&private->io_mutex);
> + return ret;
> +}
> +
> +static ssize_t vfio_ccw_schib_region_write(struct vfio_ccw_private *private,
> + const char __user *buf, size_t count,
> + loff_t *ppos)
> +{
> + return -EINVAL;
> +}
I'm wondering if we should make this callback optional (not in this patch).
> +
> +
> +static void vfio_ccw_schib_region_release(struct vfio_ccw_private *private,
> + struct vfio_ccw_region *region)
> +{
> +
> +}
Same here.
> +
> +const struct vfio_ccw_regops vfio_ccw_schib_region_ops = {
> + .read = vfio_ccw_schib_region_read,
> + .write = vfio_ccw_schib_region_write,
> + .release = vfio_ccw_schib_region_release,
> +};
(...)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 5/8] vfio-ccw: Introduce a new CRW region
2020-04-17 2:29 [PATCH v3 0/8] s390x/vfio-ccw: Channel Path Handling [KVM] Eric Farman
` (3 preceding siblings ...)
2020-04-17 2:29 ` [PATCH v3 4/8] vfio-ccw: Introduce a new schib region Eric Farman
@ 2020-04-17 2:29 ` Eric Farman
2020-04-21 9:41 ` Cornelia Huck
2020-04-17 2:29 ` [PATCH v3 6/8] vfio-ccw: Refactor IRQ handlers Eric Farman
` (3 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Eric Farman @ 2020-04-17 2:29 UTC (permalink / raw)
To: linux-s390, kvm
Cc: Cornelia Huck, Halil Pasic, Jason Herne, Jared Rossi, Eric Farman
From: Farhan Ali <alifm@linux.ibm.com>
This region provides a mechanism to pass Channel Report Word(s)
that affect vfio-ccw devices, and need to be passed to the guest
for its awareness and/or processing.
The base driver (see crw_collect_info()) provides space for two
CRWs, as a subchannel event may have two CRWs chained together
(one for the ssid, one for the subcahnnel). All other CRWs will
only occupy the first one. Even though this support will also
only utilize the first one, we'll provide space for two also.
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
Notes:
v2->v3:
- Remove "if list-empty" check, since there's no list yet [EF]
- Reduce the CRW region to one fullword, instead of two [CH]
v1->v2:
- Add new region info to Documentation/s390/vfio-ccw.rst [CH]
- Add a block comment to struct ccw_crw_region [CH]
v0->v1: [EF]
- Clean up checkpatch (whitespace) errors
- Add ret=-ENOMEM in error path for new region
- Add io_mutex for region read (originally in last patch)
- Change crw1/crw2 to crw0/crw1
- Reorder cleanup of regions
Documentation/s390/vfio-ccw.rst | 16 +++++++++
drivers/s390/cio/vfio_ccw_chp.c | 53 +++++++++++++++++++++++++++++
drivers/s390/cio/vfio_ccw_drv.c | 20 +++++++++++
drivers/s390/cio/vfio_ccw_ops.c | 4 +++
drivers/s390/cio/vfio_ccw_private.h | 3 ++
include/uapi/linux/vfio.h | 1 +
include/uapi/linux/vfio_ccw.h | 8 +++++
7 files changed, 105 insertions(+)
diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst
index 98832d95f395..3338551ef642 100644
--- a/Documentation/s390/vfio-ccw.rst
+++ b/Documentation/s390/vfio-ccw.rst
@@ -247,6 +247,22 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB.
Reading this region triggers a STORE SUBCHANNEL to be issued to the
associated hardware.
+vfio-ccw crw region
+---------------------
+
+The vfio-ccw crw region is used to return Channel Report Word (CRW)
+data to userspace::
+
+ struct ccw_crw_region {
+ __u32 crw;
+ } __packed;
+
+This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_CRW.
+
+Currently, space is provided for a single CRW. Handling of chained
+CRWs (not implemented in vfio-ccw) can be accomplished by re-reading
+the region for additional CRW data.
+
vfio-ccw operation details
--------------------------
diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index 18f3b3e873a9..c1362aae61f5 100644
--- a/drivers/s390/cio/vfio_ccw_chp.c
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -74,3 +74,56 @@ int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private)
VFIO_REGION_INFO_FLAG_READ,
private->schib_region);
}
+
+static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
+ char __user *buf, size_t count,
+ loff_t *ppos)
+{
+ unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
+ loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
+ struct ccw_crw_region *region;
+ int ret;
+
+ if (pos + count > sizeof(*region))
+ return -EINVAL;
+
+ mutex_lock(&private->io_mutex);
+ region = private->region[i].data;
+
+ if (copy_to_user(buf, (void *)region + pos, count))
+ ret = -EFAULT;
+ else
+ ret = count;
+
+ mutex_unlock(&private->io_mutex);
+ return ret;
+}
+
+static ssize_t vfio_ccw_crw_region_write(struct vfio_ccw_private *private,
+ const char __user *buf, size_t count,
+ loff_t *ppos)
+{
+ return -EINVAL;
+}
+
+static void vfio_ccw_crw_region_release(struct vfio_ccw_private *private,
+ struct vfio_ccw_region *region)
+{
+
+}
+
+const struct vfio_ccw_regops vfio_ccw_crw_region_ops = {
+ .read = vfio_ccw_crw_region_read,
+ .write = vfio_ccw_crw_region_write,
+ .release = vfio_ccw_crw_region_release,
+};
+
+int vfio_ccw_register_crw_dev_regions(struct vfio_ccw_private *private)
+{
+ return vfio_ccw_register_dev_region(private,
+ VFIO_REGION_SUBTYPE_CCW_CRW,
+ &vfio_ccw_crw_region_ops,
+ sizeof(struct ccw_crw_region),
+ VFIO_REGION_INFO_FLAG_READ,
+ private->crw_region);
+}
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 8e05ed611c10..7893027c3a8f 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -28,6 +28,7 @@ struct workqueue_struct *vfio_ccw_work_q;
static struct kmem_cache *vfio_ccw_io_region;
static struct kmem_cache *vfio_ccw_cmd_region;
static struct kmem_cache *vfio_ccw_schib_region;
+static struct kmem_cache *vfio_ccw_crw_region;
debug_info_t *vfio_ccw_debug_msg_id;
debug_info_t *vfio_ccw_debug_trace_id;
@@ -120,6 +121,8 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
static void vfio_ccw_free_regions(struct vfio_ccw_private *private)
{
+ if (private->crw_region)
+ kmem_cache_free(vfio_ccw_crw_region, private->crw_region);
if (private->schib_region)
kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
if (private->cmd_region)
@@ -165,6 +168,12 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
if (!private->schib_region)
goto out_free;
+ private->crw_region = kmem_cache_zalloc(vfio_ccw_crw_region,
+ GFP_KERNEL | GFP_DMA);
+
+ if (!private->crw_region)
+ goto out_free;
+
private->sch = sch;
dev_set_drvdata(&sch->dev, private);
mutex_init(&private->io_mutex);
@@ -364,6 +373,7 @@ static void vfio_ccw_debug_exit(void)
static void vfio_ccw_destroy_regions(void)
{
+ kmem_cache_destroy(vfio_ccw_crw_region);
kmem_cache_destroy(vfio_ccw_schib_region);
kmem_cache_destroy(vfio_ccw_cmd_region);
kmem_cache_destroy(vfio_ccw_io_region);
@@ -411,6 +421,16 @@ static int __init vfio_ccw_sch_init(void)
goto out_err;
}
+ vfio_ccw_crw_region = kmem_cache_create_usercopy("vfio_ccw_crw_region",
+ sizeof(struct ccw_crw_region), 0,
+ SLAB_ACCOUNT, 0,
+ sizeof(struct ccw_crw_region), NULL);
+
+ if (!vfio_ccw_crw_region) {
+ ret = -ENOMEM;
+ goto out_err;
+ }
+
isc_register(VFIO_CCW_ISC);
ret = css_driver_register(&vfio_ccw_sch_driver);
if (ret) {
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 22988d67b6bb..edec0fb7ace8 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -178,6 +178,10 @@ static int vfio_ccw_mdev_open(struct mdev_device *mdev)
if (ret)
goto out_unregister;
+ ret = vfio_ccw_register_crw_dev_regions(private);
+ if (ret)
+ goto out_unregister;
+
return ret;
out_unregister:
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index d6601a8adf13..8289b6850e59 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -57,6 +57,7 @@ void vfio_ccw_unregister_dev_regions(struct vfio_ccw_private *private);
int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private);
+int vfio_ccw_register_crw_dev_regions(struct vfio_ccw_private *private);
/**
* struct vfio_ccw_private
@@ -71,6 +72,7 @@ int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private);
* @region: additional regions for other subchannel operations
* @cmd_region: MMIO region for asynchronous I/O commands other than START
* @schib_region: MMIO region for SCHIB information
+ * @crw_region: MMIO region for getting channel report words
* @num_regions: number of additional regions
* @cp: channel program for the current I/O operation
* @irb: irb info received from interrupt
@@ -90,6 +92,7 @@ struct vfio_ccw_private {
struct vfio_ccw_region *region;
struct ccw_cmd_region *cmd_region;
struct ccw_schib_region *schib_region;
+ struct ccw_crw_region *crw_region;
int num_regions;
struct channel_program cp;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 7a1abbd889bd..469f813749f1 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -379,6 +379,7 @@ 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
diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h
index 758bf214898d..faf57e691d4a 100644
--- a/include/uapi/linux/vfio_ccw.h
+++ b/include/uapi/linux/vfio_ccw.h
@@ -44,4 +44,12 @@ struct ccw_schib_region {
__u8 schib_area[SCHIB_AREA_SIZE];
} __packed;
+/*
+ * Used for returning Channel Report Word(s) to userspace.
+ * Note: this is controlled by a capability
+ */
+struct ccw_crw_region {
+ __u32 crw;
+} __packed;
+
#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 5/8] vfio-ccw: Introduce a new CRW region
2020-04-17 2:29 ` [PATCH v3 5/8] vfio-ccw: Introduce a new CRW region Eric Farman
@ 2020-04-21 9:41 ` Cornelia Huck
2020-04-21 11:02 ` Eric Farman
0 siblings, 1 reply; 21+ messages in thread
From: Cornelia Huck @ 2020-04-21 9:41 UTC (permalink / raw)
To: Eric Farman; +Cc: linux-s390, kvm, Halil Pasic, Jason Herne, Jared Rossi
On Fri, 17 Apr 2020 04:29:58 +0200
Eric Farman <farman@linux.ibm.com> wrote:
> From: Farhan Ali <alifm@linux.ibm.com>
>
> This region provides a mechanism to pass Channel Report Word(s)
> that affect vfio-ccw devices, and need to be passed to the guest
> for its awareness and/or processing.
>
> The base driver (see crw_collect_info()) provides space for two
> CRWs, as a subchannel event may have two CRWs chained together
> (one for the ssid, one for the subcahnnel). All other CRWs will
> only occupy the first one. Even though this support will also
> only utilize the first one, we'll provide space for two also.
This is no longer true?
>
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>
> Notes:
> v2->v3:
> - Remove "if list-empty" check, since there's no list yet [EF]
> - Reduce the CRW region to one fullword, instead of two [CH]
>
> v1->v2:
> - Add new region info to Documentation/s390/vfio-ccw.rst [CH]
> - Add a block comment to struct ccw_crw_region [CH]
>
> v0->v1: [EF]
> - Clean up checkpatch (whitespace) errors
> - Add ret=-ENOMEM in error path for new region
> - Add io_mutex for region read (originally in last patch)
> - Change crw1/crw2 to crw0/crw1
> - Reorder cleanup of regions
>
> Documentation/s390/vfio-ccw.rst | 16 +++++++++
> drivers/s390/cio/vfio_ccw_chp.c | 53 +++++++++++++++++++++++++++++
> drivers/s390/cio/vfio_ccw_drv.c | 20 +++++++++++
> drivers/s390/cio/vfio_ccw_ops.c | 4 +++
> drivers/s390/cio/vfio_ccw_private.h | 3 ++
> include/uapi/linux/vfio.h | 1 +
> include/uapi/linux/vfio_ccw.h | 8 +++++
> 7 files changed, 105 insertions(+)
>
> diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst
> index 98832d95f395..3338551ef642 100644
> --- a/Documentation/s390/vfio-ccw.rst
> +++ b/Documentation/s390/vfio-ccw.rst
> @@ -247,6 +247,22 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB.
> Reading this region triggers a STORE SUBCHANNEL to be issued to the
> associated hardware.
>
> +vfio-ccw crw region
> +---------------------
> +
> +The vfio-ccw crw region is used to return Channel Report Word (CRW)
> +data to userspace::
> +
> + struct ccw_crw_region {
> + __u32 crw;
> + } __packed;
> +
> +This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_CRW.
> +
> +Currently, space is provided for a single CRW. Handling of chained
> +CRWs (not implemented in vfio-ccw) can be accomplished by re-reading
> +the region for additional CRW data.
What about the following instead:
"Reading this region returns a CRW if one that is relevant for this
subchannel (e.g. one reporting changes in channel path state) is
pending, or all zeroes if not. If multiple CRWs are pending (including
possibly chained CRWs), reading this region again will return the next
one, until no more CRWs are pending and zeroes are returned. This is
similar to how STORE CHANNEL REPORT WORD works."
> +
> vfio-ccw operation details
> --------------------------
>
> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index 18f3b3e873a9..c1362aae61f5 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> @@ -74,3 +74,56 @@ int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private)
> VFIO_REGION_INFO_FLAG_READ,
> private->schib_region);
> }
> +
> +static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
> + char __user *buf, size_t count,
> + loff_t *ppos)
> +{
> + unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
> + loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
> + struct ccw_crw_region *region;
> + int ret;
> +
> + if (pos + count > sizeof(*region))
> + return -EINVAL;
> +
> + mutex_lock(&private->io_mutex);
> + region = private->region[i].data;
> +
> + if (copy_to_user(buf, (void *)region + pos, count))
> + ret = -EFAULT;
> + else
> + ret = count;
I see that you implemented it a bit differently in patch 7, but I think
resetting crw to 0 immediately after the copy_to_user() is cleaner. It
also can be done in this patch already :)
> +
> + mutex_unlock(&private->io_mutex);
> + return ret;
> +}
(...)
> diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h
> index 758bf214898d..faf57e691d4a 100644
> --- a/include/uapi/linux/vfio_ccw.h
> +++ b/include/uapi/linux/vfio_ccw.h
> @@ -44,4 +44,12 @@ struct ccw_schib_region {
> __u8 schib_area[SCHIB_AREA_SIZE];
> } __packed;
>
> +/*
> + * Used for returning Channel Report Word(s) to userspace.
s/Channel Report Word(s)/a Channel Report Word/ ?
> + * Note: this is controlled by a capability
> + */
> +struct ccw_crw_region {
> + __u32 crw;
> +} __packed;
> +
> #endif
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 5/8] vfio-ccw: Introduce a new CRW region
2020-04-21 9:41 ` Cornelia Huck
@ 2020-04-21 11:02 ` Eric Farman
2020-04-21 11:08 ` Cornelia Huck
0 siblings, 1 reply; 21+ messages in thread
From: Eric Farman @ 2020-04-21 11:02 UTC (permalink / raw)
To: Cornelia Huck; +Cc: linux-s390, kvm, Halil Pasic, Jason Herne, Jared Rossi
On 4/21/20 5:41 AM, Cornelia Huck wrote:
> On Fri, 17 Apr 2020 04:29:58 +0200
> Eric Farman <farman@linux.ibm.com> wrote:
>
>> From: Farhan Ali <alifm@linux.ibm.com>
>>
>> This region provides a mechanism to pass Channel Report Word(s)
>> that affect vfio-ccw devices, and need to be passed to the guest
>> for its awareness and/or processing.
>>
>> The base driver (see crw_collect_info()) provides space for two
>> CRWs, as a subchannel event may have two CRWs chained together
>> (one for the ssid, one for the subcahnnel). All other CRWs will
s/subcahnnel/subchannel/
>> only occupy the first one. Even though this support will also
>> only utilize the first one, we'll provide space for two also.
>
> This is no longer true?
Oops, yes.
>
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>
>> Notes:
>> v2->v3:
>> - Remove "if list-empty" check, since there's no list yet [EF]
>> - Reduce the CRW region to one fullword, instead of two [CH]
>>
>> v1->v2:
>> - Add new region info to Documentation/s390/vfio-ccw.rst [CH]
>> - Add a block comment to struct ccw_crw_region [CH]
>>
>> v0->v1: [EF]
>> - Clean up checkpatch (whitespace) errors
>> - Add ret=-ENOMEM in error path for new region
>> - Add io_mutex for region read (originally in last patch)
>> - Change crw1/crw2 to crw0/crw1
>> - Reorder cleanup of regions
>>
>> Documentation/s390/vfio-ccw.rst | 16 +++++++++
>> drivers/s390/cio/vfio_ccw_chp.c | 53 +++++++++++++++++++++++++++++
>> drivers/s390/cio/vfio_ccw_drv.c | 20 +++++++++++
>> drivers/s390/cio/vfio_ccw_ops.c | 4 +++
>> drivers/s390/cio/vfio_ccw_private.h | 3 ++
>> include/uapi/linux/vfio.h | 1 +
>> include/uapi/linux/vfio_ccw.h | 8 +++++
>> 7 files changed, 105 insertions(+)
>>
>> diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst
>> index 98832d95f395..3338551ef642 100644
>> --- a/Documentation/s390/vfio-ccw.rst
>> +++ b/Documentation/s390/vfio-ccw.rst
>> @@ -247,6 +247,22 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB.
>> Reading this region triggers a STORE SUBCHANNEL to be issued to the
>> associated hardware.
>>
>> +vfio-ccw crw region
>> +---------------------
>> +
>> +The vfio-ccw crw region is used to return Channel Report Word (CRW)
>> +data to userspace::
>> +
>> + struct ccw_crw_region {
>> + __u32 crw;
>> + } __packed;
>> +
>> +This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_CRW.
>> +
>> +Currently, space is provided for a single CRW. Handling of chained
>> +CRWs (not implemented in vfio-ccw) can be accomplished by re-reading
>> +the region for additional CRW data.
>
> What about the following instead:
>
> "Reading this region returns a CRW if one that is relevant for this
> subchannel (e.g. one reporting changes in channel path state) is
> pending, or all zeroes if not. If multiple CRWs are pending (including
> possibly chained CRWs), reading this region again will return the next
> one, until no more CRWs are pending and zeroes are returned. This is
> similar to how STORE CHANNEL REPORT WORD works."
Sounds good to me.
Hrm... Maybe coffee hasn't hit yet. Should I wire STCRW into this, or
just rely on the notification from the host to trigger the read?
>
>> +
>> vfio-ccw operation details
>> --------------------------
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
>> index 18f3b3e873a9..c1362aae61f5 100644
>> --- a/drivers/s390/cio/vfio_ccw_chp.c
>> +++ b/drivers/s390/cio/vfio_ccw_chp.c
>> @@ -74,3 +74,56 @@ int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private)
>> VFIO_REGION_INFO_FLAG_READ,
>> private->schib_region);
>> }
>> +
>> +static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
>> + char __user *buf, size_t count,
>> + loff_t *ppos)
>> +{
>> + unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
>> + loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
>> + struct ccw_crw_region *region;
>> + int ret;
>> +
>> + if (pos + count > sizeof(*region))
>> + return -EINVAL;
>> +
>> + mutex_lock(&private->io_mutex);
>> + region = private->region[i].data;
>> +
>> + if (copy_to_user(buf, (void *)region + pos, count))
>> + ret = -EFAULT;
>> + else
>> + ret = count;
>
> I see that you implemented it a bit differently in patch 7, but I think
> resetting crw to 0 immediately after the copy_to_user() is cleaner. It
> also can be done in this patch already :)
Ha, yes. I actually had it set that way, but didn't like the result in
patch 7. I'll revisit that.
>
>> +
>> + mutex_unlock(&private->io_mutex);
>> + return ret;
>> +}
>
> (...)
>
>> diff --git a/include/uapi/linux/vfio_ccw.h b/include/uapi/linux/vfio_ccw.h
>> index 758bf214898d..faf57e691d4a 100644
>> --- a/include/uapi/linux/vfio_ccw.h
>> +++ b/include/uapi/linux/vfio_ccw.h
>> @@ -44,4 +44,12 @@ struct ccw_schib_region {
>> __u8 schib_area[SCHIB_AREA_SIZE];
>> } __packed;
>>
>> +/*
>> + * Used for returning Channel Report Word(s) to userspace.
>
> s/Channel Report Word(s)/a Channel Report Word/ ?
Nod.
>
>> + * Note: this is controlled by a capability
>> + */
>> +struct ccw_crw_region {
>> + __u32 crw;
>> +} __packed;
>> +
>> #endif
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 5/8] vfio-ccw: Introduce a new CRW region
2020-04-21 11:02 ` Eric Farman
@ 2020-04-21 11:08 ` Cornelia Huck
2020-04-21 12:03 ` Eric Farman
0 siblings, 1 reply; 21+ messages in thread
From: Cornelia Huck @ 2020-04-21 11:08 UTC (permalink / raw)
To: Eric Farman; +Cc: linux-s390, kvm, Halil Pasic, Jason Herne, Jared Rossi
On Tue, 21 Apr 2020 07:02:03 -0400
Eric Farman <farman@linux.ibm.com> wrote:
> On 4/21/20 5:41 AM, Cornelia Huck wrote:
> > On Fri, 17 Apr 2020 04:29:58 +0200
> > Eric Farman <farman@linux.ibm.com> wrote:
> >> diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst
> >> index 98832d95f395..3338551ef642 100644
> >> --- a/Documentation/s390/vfio-ccw.rst
> >> +++ b/Documentation/s390/vfio-ccw.rst
> >> @@ -247,6 +247,22 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB.
> >> Reading this region triggers a STORE SUBCHANNEL to be issued to the
> >> associated hardware.
> >>
> >> +vfio-ccw crw region
> >> +---------------------
> >> +
> >> +The vfio-ccw crw region is used to return Channel Report Word (CRW)
> >> +data to userspace::
> >> +
> >> + struct ccw_crw_region {
> >> + __u32 crw;
> >> + } __packed;
> >> +
> >> +This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_CRW.
> >> +
> >> +Currently, space is provided for a single CRW. Handling of chained
> >> +CRWs (not implemented in vfio-ccw) can be accomplished by re-reading
> >> +the region for additional CRW data.
> >
> > What about the following instead:
> >
> > "Reading this region returns a CRW if one that is relevant for this
> > subchannel (e.g. one reporting changes in channel path state) is
> > pending, or all zeroes if not. If multiple CRWs are pending (including
> > possibly chained CRWs), reading this region again will return the next
> > one, until no more CRWs are pending and zeroes are returned. This is
> > similar to how STORE CHANNEL REPORT WORD works."
>
> Sounds good to me.
>
> Hrm... Maybe coffee hasn't hit yet. Should I wire STCRW into this, or
> just rely on the notification from the host to trigger the read?
Userspace is supposed to use this to get crws to inject into the guest,
no stcrw involved until the guest actually got the machine check for it.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 5/8] vfio-ccw: Introduce a new CRW region
2020-04-21 11:08 ` Cornelia Huck
@ 2020-04-21 12:03 ` Eric Farman
0 siblings, 0 replies; 21+ messages in thread
From: Eric Farman @ 2020-04-21 12:03 UTC (permalink / raw)
To: Cornelia Huck; +Cc: linux-s390, kvm, Halil Pasic, Jason Herne, Jared Rossi
On 4/21/20 7:08 AM, Cornelia Huck wrote:
> On Tue, 21 Apr 2020 07:02:03 -0400
> Eric Farman <farman@linux.ibm.com> wrote:
>
>> On 4/21/20 5:41 AM, Cornelia Huck wrote:
>>> On Fri, 17 Apr 2020 04:29:58 +0200
>>> Eric Farman <farman@linux.ibm.com> wrote:
>
>>>> diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst
>>>> index 98832d95f395..3338551ef642 100644
>>>> --- a/Documentation/s390/vfio-ccw.rst
>>>> +++ b/Documentation/s390/vfio-ccw.rst
>>>> @@ -247,6 +247,22 @@ This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_SCHIB.
>>>> Reading this region triggers a STORE SUBCHANNEL to be issued to the
>>>> associated hardware.
>>>>
>>>> +vfio-ccw crw region
>>>> +---------------------
>>>> +
>>>> +The vfio-ccw crw region is used to return Channel Report Word (CRW)
>>>> +data to userspace::
>>>> +
>>>> + struct ccw_crw_region {
>>>> + __u32 crw;
>>>> + } __packed;
>>>> +
>>>> +This region is exposed via region type VFIO_REGION_SUBTYPE_CCW_CRW.
>>>> +
>>>> +Currently, space is provided for a single CRW. Handling of chained
>>>> +CRWs (not implemented in vfio-ccw) can be accomplished by re-reading
>>>> +the region for additional CRW data.
>>>
>>> What about the following instead:
>>>
>>> "Reading this region returns a CRW if one that is relevant for this
>>> subchannel (e.g. one reporting changes in channel path state) is
>>> pending, or all zeroes if not. If multiple CRWs are pending (including
>>> possibly chained CRWs), reading this region again will return the next
>>> one, until no more CRWs are pending and zeroes are returned. This is
>>> similar to how STORE CHANNEL REPORT WORD works."
>>
>> Sounds good to me.
>>
>> Hrm... Maybe coffee hasn't hit yet. Should I wire STCRW into this, or
>> just rely on the notification from the host to trigger the read?
>
> Userspace is supposed to use this to get crws to inject into the guest,
> no stcrw involved until the guest actually got the machine check for it.
>
Ah, yes. Thanks!
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 6/8] vfio-ccw: Refactor IRQ handlers
2020-04-17 2:29 [PATCH v3 0/8] s390x/vfio-ccw: Channel Path Handling [KVM] Eric Farman
` (4 preceding siblings ...)
2020-04-17 2:29 ` [PATCH v3 5/8] vfio-ccw: Introduce a new CRW region Eric Farman
@ 2020-04-17 2:29 ` Eric Farman
2020-04-17 2:30 ` [PATCH v3 7/8] vfio-ccw: Wire up the CRW irq and CRW region Eric Farman
` (2 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Eric Farman @ 2020-04-17 2:29 UTC (permalink / raw)
To: linux-s390, kvm
Cc: Cornelia Huck, Halil Pasic, Jason Herne, Jared Rossi, Eric Farman
To simplify future expansion.
Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
Notes:
v1->v2:
- Add Conny's r-b
drivers/s390/cio/vfio_ccw_ops.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index edec0fb7ace8..f3033f8fc96d 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -391,17 +391,21 @@ static int vfio_ccw_mdev_get_region_info(struct vfio_region_info *info,
static int vfio_ccw_mdev_get_irq_info(struct vfio_irq_info *info)
{
- if (info->index != VFIO_CCW_IO_IRQ_INDEX)
+ switch (info->index) {
+ case VFIO_CCW_IO_IRQ_INDEX:
+ info->count = 1;
+ info->flags = VFIO_IRQ_INFO_EVENTFD;
+ break;
+ default:
return -EINVAL;
-
- info->count = 1;
- info->flags = VFIO_IRQ_INFO_EVENTFD;
+ }
return 0;
}
static int vfio_ccw_mdev_set_irqs(struct mdev_device *mdev,
uint32_t flags,
+ uint32_t index,
void __user *data)
{
struct vfio_ccw_private *private;
@@ -411,7 +415,14 @@ static int vfio_ccw_mdev_set_irqs(struct mdev_device *mdev,
return -EINVAL;
private = dev_get_drvdata(mdev_parent_dev(mdev));
- ctx = &private->io_trigger;
+
+ switch (index) {
+ case VFIO_CCW_IO_IRQ_INDEX:
+ ctx = &private->io_trigger;
+ break;
+ default:
+ return -EINVAL;
+ }
switch (flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
case VFIO_IRQ_SET_DATA_NONE:
@@ -583,7 +594,7 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,
return ret;
data = (void __user *)(arg + minsz);
- return vfio_ccw_mdev_set_irqs(mdev, hdr.flags, data);
+ return vfio_ccw_mdev_set_irqs(mdev, hdr.flags, hdr.index, data);
}
case VFIO_DEVICE_RESET:
return vfio_ccw_mdev_reset(mdev);
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 7/8] vfio-ccw: Wire up the CRW irq and CRW region
2020-04-17 2:29 [PATCH v3 0/8] s390x/vfio-ccw: Channel Path Handling [KVM] Eric Farman
` (5 preceding siblings ...)
2020-04-17 2:29 ` [PATCH v3 6/8] vfio-ccw: Refactor IRQ handlers Eric Farman
@ 2020-04-17 2:30 ` Eric Farman
2020-04-21 12:06 ` Cornelia Huck
2020-04-17 2:30 ` [PATCH v3 8/8] vfio-ccw: Add trace for CRW event Eric Farman
2020-04-21 15:35 ` [PATCH v3 0/8] s390x/vfio-ccw: Channel Path Handling [KVM] Cornelia Huck
8 siblings, 1 reply; 21+ messages in thread
From: Eric Farman @ 2020-04-17 2:30 UTC (permalink / raw)
To: linux-s390, kvm
Cc: Cornelia Huck, Halil Pasic, Jason Herne, Jared Rossi, Eric Farman
From: Farhan Ali <alifm@linux.ibm.com>
Use an IRQ to notify userspace that there is a CRW
pending in the region, related to path-availability
changes on the passthrough subchannel.
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
Notes:
v2->v3:
- Refactor vfio_ccw_alloc_crw() to accept rsc, erc, and rsid fields
of a CRW as input [CH]
- Copy the right amount of CRWs to the crw_region [EF]
- Use sizeof(target) for the memcpy, rather than sizeof(source) [EF]
- Ensure the CRW region is empty if no CRW is present [EF/CH]
- Refactor how data goes from private-to-region-to-user [CH]
- Reduce the number of CRWs from two to one [CH]
- s/vc_crw/crw/ [EF]
v1->v2:
- Remove extraneous 0x0 in crw.rsid assignment [CH]
- Refactor the building/queueing of a crw into its own routine [EF]
v0->v1: [EF]
- Place the non-refactoring changes from the previous patch here
- Clean up checkpatch (whitespace) errors
- s/chp_crw/crw/
- Move acquire/release of io_mutex in vfio_ccw_crw_region_read()
into patch that introduces that region
- Remove duplicate include from vfio_ccw_drv.c
- Reorder include in vfio_ccw_private.h
drivers/s390/cio/vfio_ccw_chp.c | 19 +++++++++++
drivers/s390/cio/vfio_ccw_drv.c | 49 +++++++++++++++++++++++++++++
drivers/s390/cio/vfio_ccw_ops.c | 4 +++
drivers/s390/cio/vfio_ccw_private.h | 9 ++++++
include/uapi/linux/vfio.h | 1 +
5 files changed, 82 insertions(+)
diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index c1362aae61f5..fbadd5cf1b5c 100644
--- a/drivers/s390/cio/vfio_ccw_chp.c
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -82,20 +82,39 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
struct ccw_crw_region *region;
+ struct vfio_ccw_crw *crw;
int ret;
if (pos + count > sizeof(*region))
return -EINVAL;
+ crw = list_first_entry_or_null(&private->crw,
+ struct vfio_ccw_crw, next);
+
+ if (crw)
+ list_del(&crw->next);
+
mutex_lock(&private->io_mutex);
region = private->region[i].data;
+ if (crw)
+ memcpy(®ion->crw, &crw->crw, sizeof(region->crw));
+ else
+ region->crw = 0;
+
if (copy_to_user(buf, (void *)region + pos, count))
ret = -EFAULT;
else
ret = count;
mutex_unlock(&private->io_mutex);
+
+ kfree(crw);
+
+ /* Notify the guest if more CRWs are on our queue */
+ if (!list_empty(&private->crw) && private->crw_trigger)
+ eventfd_signal(private->crw_trigger, 1);
+
return ret;
}
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 7893027c3a8f..a0d8d2560c12 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -108,6 +108,16 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
eventfd_signal(private->io_trigger, 1);
}
+static void vfio_ccw_crw_todo(struct work_struct *work)
+{
+ struct vfio_ccw_private *private;
+
+ private = container_of(work, struct vfio_ccw_private, crw_work);
+
+ if (!list_empty(&private->crw) && private->crw_trigger)
+ eventfd_signal(private->crw_trigger, 1);
+}
+
/*
* Css driver callbacks
*/
@@ -186,7 +196,9 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
if (ret)
goto out_free;
+ INIT_LIST_HEAD(&private->crw);
INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
+ INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
atomic_set(&private->avail, 1);
private->state = VFIO_CCW_STATE_STANDBY;
@@ -217,9 +229,15 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
static int vfio_ccw_sch_remove(struct subchannel *sch)
{
struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
+ struct vfio_ccw_crw *crw, *temp;
vfio_ccw_sch_quiesce(sch);
+ list_for_each_entry_safe(crw, temp, &private->crw, next) {
+ list_del(&crw->next);
+ kfree(crw);
+ }
+
vfio_ccw_mdev_unreg(sch);
dev_set_drvdata(&sch->dev, NULL);
@@ -281,6 +299,33 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
return rc;
}
+static void vfio_ccw_alloc_crw(struct vfio_ccw_private *private,
+ unsigned int rsc,
+ unsigned int erc,
+ unsigned int rsid)
+{
+ struct vfio_ccw_crw *crw;
+
+ /*
+ * If unable to allocate a CRW, just drop the event and
+ * carry on. The guest will either see a later one or
+ * learn when it issues its own store subchannel.
+ */
+ crw = kzalloc(sizeof(*crw), GFP_ATOMIC);
+ if (!crw)
+ return;
+
+ /*
+ * Build the CRW based on the inputs given to us.
+ */
+ crw->crw.rsc = rsc;
+ crw->crw.erc = erc;
+ crw->crw.rsid = rsid;
+
+ list_add_tail(&crw->next, &private->crw);
+ queue_work(vfio_ccw_work_q, &private->crw_work);
+}
+
static int vfio_ccw_chp_event(struct subchannel *sch,
struct chp_link *link, int event)
{
@@ -309,6 +354,8 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
case CHP_OFFLINE:
/* Path is gone */
cio_cancel_halt_clear(sch, &retry);
+ vfio_ccw_alloc_crw(private, CRW_RSC_CPATH, CRW_ERC_PERRN,
+ (link->chpid.cssid << 8) | link->chpid.id);
break;
case CHP_VARY_ON:
/* Path logically turned on */
@@ -318,6 +365,8 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
case CHP_ONLINE:
/* Path became available */
sch->lpm |= mask & sch->opm;
+ vfio_ccw_alloc_crw(private, CRW_RSC_CPATH, CRW_ERC_INIT,
+ (link->chpid.cssid << 8) | link->chpid.id);
break;
}
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index f3033f8fc96d..8b3ed5b45277 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -393,6 +393,7 @@ static int vfio_ccw_mdev_get_irq_info(struct vfio_irq_info *info)
{
switch (info->index) {
case VFIO_CCW_IO_IRQ_INDEX:
+ case VFIO_CCW_CRW_IRQ_INDEX:
info->count = 1;
info->flags = VFIO_IRQ_INFO_EVENTFD;
break;
@@ -420,6 +421,9 @@ static int vfio_ccw_mdev_set_irqs(struct mdev_device *mdev,
case VFIO_CCW_IO_IRQ_INDEX:
ctx = &private->io_trigger;
break;
+ case VFIO_CCW_CRW_IRQ_INDEX:
+ ctx = &private->crw_trigger;
+ break;
default:
return -EINVAL;
}
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 8289b6850e59..8723156b29ea 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -17,6 +17,7 @@
#include <linux/eventfd.h>
#include <linux/workqueue.h>
#include <linux/vfio_ccw.h>
+#include <asm/crw.h>
#include <asm/debug.h>
#include "css.h"
@@ -59,6 +60,11 @@ int vfio_ccw_register_async_dev_regions(struct vfio_ccw_private *private);
int vfio_ccw_register_schib_dev_regions(struct vfio_ccw_private *private);
int vfio_ccw_register_crw_dev_regions(struct vfio_ccw_private *private);
+struct vfio_ccw_crw {
+ struct list_head next;
+ struct crw crw;
+};
+
/**
* struct vfio_ccw_private
* @sch: pointer to the subchannel
@@ -98,9 +104,12 @@ struct vfio_ccw_private {
struct channel_program cp;
struct irb irb;
union scsw scsw;
+ struct list_head crw;
struct eventfd_ctx *io_trigger;
+ struct eventfd_ctx *crw_trigger;
struct work_struct io_work;
+ struct work_struct crw_work;
} __aligned(8);
extern int vfio_ccw_mdev_reg(struct subchannel *sch);
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 469f813749f1..907758cf6d60 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -579,6 +579,7 @@ enum {
enum {
VFIO_CCW_IO_IRQ_INDEX,
+ VFIO_CCW_CRW_IRQ_INDEX,
VFIO_CCW_NUM_IRQS
};
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 7/8] vfio-ccw: Wire up the CRW irq and CRW region
2020-04-17 2:30 ` [PATCH v3 7/8] vfio-ccw: Wire up the CRW irq and CRW region Eric Farman
@ 2020-04-21 12:06 ` Cornelia Huck
0 siblings, 0 replies; 21+ messages in thread
From: Cornelia Huck @ 2020-04-21 12:06 UTC (permalink / raw)
To: Eric Farman; +Cc: linux-s390, kvm, Halil Pasic, Jason Herne, Jared Rossi
On Fri, 17 Apr 2020 04:30:00 +0200
Eric Farman <farman@linux.ibm.com> wrote:
> From: Farhan Ali <alifm@linux.ibm.com>
>
> Use an IRQ to notify userspace that there is a CRW
> pending in the region, related to path-availability
> changes on the passthrough subchannel.
>
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>
> Notes:
> v2->v3:
> - Refactor vfio_ccw_alloc_crw() to accept rsc, erc, and rsid fields
> of a CRW as input [CH]
> - Copy the right amount of CRWs to the crw_region [EF]
> - Use sizeof(target) for the memcpy, rather than sizeof(source) [EF]
> - Ensure the CRW region is empty if no CRW is present [EF/CH]
> - Refactor how data goes from private-to-region-to-user [CH]
> - Reduce the number of CRWs from two to one [CH]
> - s/vc_crw/crw/ [EF]
>
> v1->v2:
> - Remove extraneous 0x0 in crw.rsid assignment [CH]
> - Refactor the building/queueing of a crw into its own routine [EF]
>
> v0->v1: [EF]
> - Place the non-refactoring changes from the previous patch here
> - Clean up checkpatch (whitespace) errors
> - s/chp_crw/crw/
> - Move acquire/release of io_mutex in vfio_ccw_crw_region_read()
> into patch that introduces that region
> - Remove duplicate include from vfio_ccw_drv.c
> - Reorder include in vfio_ccw_private.h
>
> drivers/s390/cio/vfio_ccw_chp.c | 19 +++++++++++
> drivers/s390/cio/vfio_ccw_drv.c | 49 +++++++++++++++++++++++++++++
> drivers/s390/cio/vfio_ccw_ops.c | 4 +++
> drivers/s390/cio/vfio_ccw_private.h | 9 ++++++
> include/uapi/linux/vfio.h | 1 +
> 5 files changed, 82 insertions(+)
>
(...)
> +static void vfio_ccw_alloc_crw(struct vfio_ccw_private *private,
Maybe vfio_ccw_queue_crw()? You do a bit more than allocating, and I
think allocating is already implied.
> + unsigned int rsc,
> + unsigned int erc,
> + unsigned int rsid)
> +{
> + struct vfio_ccw_crw *crw;
> +
> + /*
> + * If unable to allocate a CRW, just drop the event and
> + * carry on. The guest will either see a later one or
> + * learn when it issues its own store subchannel.
> + */
> + crw = kzalloc(sizeof(*crw), GFP_ATOMIC);
> + if (!crw)
> + return;
I suppose that's quite unlikely anyway, given how small the structure
is.
> +
> + /*
> + * Build the CRW based on the inputs given to us.
> + */
> + crw->crw.rsc = rsc;
> + crw->crw.erc = erc;
> + crw->crw.rsid = rsid;
We probably can add a chaining flag later, should we ever need it, as
this is an internal function anyway.
> +
> + list_add_tail(&crw->next, &private->crw);
> + queue_work(vfio_ccw_work_q, &private->crw_work);
> +}
> +
> static int vfio_ccw_chp_event(struct subchannel *sch,
> struct chp_link *link, int event)
> {
> @@ -309,6 +354,8 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
> case CHP_OFFLINE:
> /* Path is gone */
> cio_cancel_halt_clear(sch, &retry);
> + vfio_ccw_alloc_crw(private, CRW_RSC_CPATH, CRW_ERC_PERRN,
> + (link->chpid.cssid << 8) | link->chpid.id);
Not sure that cssid stuff is correct: The QEMU code generating crws for
chpids chains a second crw for the cssid (if MCSS-E has been enabled by
the guest). The Linux kernel does not enable MCSS-E, so it will always
have a cssid of 0; but I'm wondering if we could conceivably put a
vfio-ccw device into a non-default css in the guest? Not terribly
useful (as Linux does not enable MCSS-E), but allowed, I think.
> break;
> case CHP_VARY_ON:
> /* Path logically turned on */
> @@ -318,6 +365,8 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
> case CHP_ONLINE:
> /* Path became available */
> sch->lpm |= mask & sch->opm;
> + vfio_ccw_alloc_crw(private, CRW_RSC_CPATH, CRW_ERC_INIT,
> + (link->chpid.cssid << 8) | link->chpid.id);
> break;
> }
>
(...)
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 8/8] vfio-ccw: Add trace for CRW event
2020-04-17 2:29 [PATCH v3 0/8] s390x/vfio-ccw: Channel Path Handling [KVM] Eric Farman
` (6 preceding siblings ...)
2020-04-17 2:30 ` [PATCH v3 7/8] vfio-ccw: Wire up the CRW irq and CRW region Eric Farman
@ 2020-04-17 2:30 ` Eric Farman
2020-04-21 12:11 ` Cornelia Huck
2020-04-21 15:35 ` [PATCH v3 0/8] s390x/vfio-ccw: Channel Path Handling [KVM] Cornelia Huck
8 siblings, 1 reply; 21+ messages in thread
From: Eric Farman @ 2020-04-17 2:30 UTC (permalink / raw)
To: linux-s390, kvm
Cc: Cornelia Huck, Halil Pasic, Jason Herne, Jared Rossi, Eric Farman
Since CRW events are (should be) rare, let's put a trace
in that routine too.
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
drivers/s390/cio/vfio_ccw_drv.c | 1 +
drivers/s390/cio/vfio_ccw_trace.c | 1 +
drivers/s390/cio/vfio_ccw_trace.h | 30 ++++++++++++++++++++++++++++++
3 files changed, 32 insertions(+)
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index a0d8d2560c12..cd462b0849dc 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -336,6 +336,7 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
if (!private || !mask)
return 0;
+ trace_vfio_ccw_chp_event(private->sch->schid, mask, event);
VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
mdev_uuid(private->mdev), sch->schid.cssid,
sch->schid.ssid, sch->schid.sch_no,
diff --git a/drivers/s390/cio/vfio_ccw_trace.c b/drivers/s390/cio/vfio_ccw_trace.c
index 8c671d2519f6..4a0205905afc 100644
--- a/drivers/s390/cio/vfio_ccw_trace.c
+++ b/drivers/s390/cio/vfio_ccw_trace.c
@@ -9,6 +9,7 @@
#define CREATE_TRACE_POINTS
#include "vfio_ccw_trace.h"
+EXPORT_TRACEPOINT_SYMBOL(vfio_ccw_chp_event);
EXPORT_TRACEPOINT_SYMBOL(vfio_ccw_fsm_async_request);
EXPORT_TRACEPOINT_SYMBOL(vfio_ccw_fsm_event);
EXPORT_TRACEPOINT_SYMBOL(vfio_ccw_fsm_io_request);
diff --git a/drivers/s390/cio/vfio_ccw_trace.h b/drivers/s390/cio/vfio_ccw_trace.h
index f5d31887d413..62fb30598d47 100644
--- a/drivers/s390/cio/vfio_ccw_trace.h
+++ b/drivers/s390/cio/vfio_ccw_trace.h
@@ -17,6 +17,36 @@
#include <linux/tracepoint.h>
+TRACE_EVENT(vfio_ccw_chp_event,
+ TP_PROTO(struct subchannel_id schid,
+ int mask,
+ int event),
+ TP_ARGS(schid, mask, event),
+
+ TP_STRUCT__entry(
+ __field(u8, cssid)
+ __field(u8, ssid)
+ __field(u16, sch_no)
+ __field(int, mask)
+ __field(int, event)
+ ),
+
+ TP_fast_assign(
+ __entry->cssid = schid.cssid;
+ __entry->ssid = schid.ssid;
+ __entry->sch_no = schid.sch_no;
+ __entry->mask = mask;
+ __entry->event = event;
+ ),
+
+ TP_printk("schid=%x.%x.%04x mask=0x%x event=%d",
+ __entry->cssid,
+ __entry->ssid,
+ __entry->sch_no,
+ __entry->mask,
+ __entry->event)
+);
+
TRACE_EVENT(vfio_ccw_fsm_async_request,
TP_PROTO(struct subchannel_id schid,
int command,
--
2.17.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 8/8] vfio-ccw: Add trace for CRW event
2020-04-17 2:30 ` [PATCH v3 8/8] vfio-ccw: Add trace for CRW event Eric Farman
@ 2020-04-21 12:11 ` Cornelia Huck
0 siblings, 0 replies; 21+ messages in thread
From: Cornelia Huck @ 2020-04-21 12:11 UTC (permalink / raw)
To: Eric Farman; +Cc: linux-s390, kvm, Halil Pasic, Jason Herne, Jared Rossi
On Fri, 17 Apr 2020 04:30:01 +0200
Eric Farman <farman@linux.ibm.com> wrote:
> Since CRW events are (should be) rare, let's put a trace
> in that routine too.
>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> drivers/s390/cio/vfio_ccw_drv.c | 1 +
> drivers/s390/cio/vfio_ccw_trace.c | 1 +
> drivers/s390/cio/vfio_ccw_trace.h | 30 ++++++++++++++++++++++++++++++
> 3 files changed, 32 insertions(+)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/8] s390x/vfio-ccw: Channel Path Handling [KVM]
2020-04-17 2:29 [PATCH v3 0/8] s390x/vfio-ccw: Channel Path Handling [KVM] Eric Farman
` (7 preceding siblings ...)
2020-04-17 2:30 ` [PATCH v3 8/8] vfio-ccw: Add trace for CRW event Eric Farman
@ 2020-04-21 15:35 ` Cornelia Huck
2020-04-22 3:10 ` Eric Farman
8 siblings, 1 reply; 21+ messages in thread
From: Cornelia Huck @ 2020-04-21 15:35 UTC (permalink / raw)
To: Eric Farman; +Cc: linux-s390, kvm, Halil Pasic, Jason Herne, Jared Rossi
On Fri, 17 Apr 2020 04:29:53 +0200
Eric Farman <farman@linux.ibm.com> wrote:
> Here is a new pass at the channel-path handling code for vfio-ccw.
> Changes from previous versions are recorded in git notes for each patch.
>
> I dropped the "Remove inline get_schid()" patch from this version.
> When I made the change suggested in v2, it seemed rather frivolous and
> better to just drop it for the time being.
>
> I suspect that patches 5 and 7 would be better squashed together, but I
> have not done that here. For future versions, I guess.
The result also might get a bit large.
>
> With this, and the corresponding QEMU series (to be posted momentarily),
> applied I am able to configure off/on a CHPID (for example, by issuing
> "chchp -c 0/1 xx" on the host), and the guest is able to see both the
> events and reflect the updated path masks in its structures.
Basically, this looks good to me (modulo my comments).
One thing though that keeps coming up: do we need any kind of
serialization? Can there be any confusion from concurrent reads from
userspace, or are we sure that we always provide consistent data?
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 0/8] s390x/vfio-ccw: Channel Path Handling [KVM]
2020-04-21 15:35 ` [PATCH v3 0/8] s390x/vfio-ccw: Channel Path Handling [KVM] Cornelia Huck
@ 2020-04-22 3:10 ` Eric Farman
2020-04-22 10:27 ` Cornelia Huck
0 siblings, 1 reply; 21+ messages in thread
From: Eric Farman @ 2020-04-22 3:10 UTC (permalink / raw)
To: Cornelia Huck; +Cc: linux-s390, kvm, Halil Pasic, Jason Herne, Jared Rossi
On 4/21/20 11:35 AM, Cornelia Huck wrote:
> On Fri, 17 Apr 2020 04:29:53 +0200
> Eric Farman <farman@linux.ibm.com> wrote:
>
>> Here is a new pass at the channel-path handling code for vfio-ccw.
>> Changes from previous versions are recorded in git notes for each patch.
>>
>> I dropped the "Remove inline get_schid()" patch from this version.
>> When I made the change suggested in v2, it seemed rather frivolous and
>> better to just drop it for the time being.
>>
>> I suspect that patches 5 and 7 would be better squashed together, but I
>> have not done that here. For future versions, I guess.
>
> The result also might get a bit large.
True.
Not that someone would pick patch 5 and not 7, but vfio-ccw is broken
between them, because of a mismatch in IRQs. An example from hotplug:
error: internal error: unable to execute QEMU command 'device_add':
vfio: unexpected number of irqs 1
Maybe I just pull the CRW_IRQ definition into 5, and leave the wiring of
the CRW stuff in 7. That seems to leave a better behavior.
>
>>
>> With this, and the corresponding QEMU series (to be posted momentarily),
>> applied I am able to configure off/on a CHPID (for example, by issuing
>> "chchp -c 0/1 xx" on the host), and the guest is able to see both the
>> events and reflect the updated path masks in its structures.
>
> Basically, this looks good to me (modulo my comments).
Woo! Thanks for the feedback; I'm going to try to get them all
addressed in the next couple of days.
>
> One thing though that keeps coming up: do we need any kind of
> serialization? Can there be any confusion from concurrent reads from
> userspace, or are we sure that we always provide consistent data?
>
I'm feeling better with the rearrangement in this version of how we get
data from the queue of CRWs into the region and off to the guest. The
weirdness I described a few months ago seems to have been triggered by
one of the patches that's now been dropped. But I'll walk through this
code again once I get your latest comments applied.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/8] s390x/vfio-ccw: Channel Path Handling [KVM]
2020-04-22 3:10 ` Eric Farman
@ 2020-04-22 10:27 ` Cornelia Huck
0 siblings, 0 replies; 21+ messages in thread
From: Cornelia Huck @ 2020-04-22 10:27 UTC (permalink / raw)
To: Eric Farman; +Cc: linux-s390, kvm, Halil Pasic, Jason Herne, Jared Rossi
On Tue, 21 Apr 2020 23:10:20 -0400
Eric Farman <farman@linux.ibm.com> wrote:
> On 4/21/20 11:35 AM, Cornelia Huck wrote:
> > On Fri, 17 Apr 2020 04:29:53 +0200
> > Eric Farman <farman@linux.ibm.com> wrote:
> >
> >> Here is a new pass at the channel-path handling code for vfio-ccw.
> >> Changes from previous versions are recorded in git notes for each patch.
> >>
> >> I dropped the "Remove inline get_schid()" patch from this version.
> >> When I made the change suggested in v2, it seemed rather frivolous and
> >> better to just drop it for the time being.
> >>
> >> I suspect that patches 5 and 7 would be better squashed together, but I
> >> have not done that here. For future versions, I guess.
> >
> > The result also might get a bit large.
>
> True.
>
> Not that someone would pick patch 5 and not 7, but vfio-ccw is broken
> between them, because of a mismatch in IRQs. An example from hotplug:
>
> error: internal error: unable to execute QEMU command 'device_add':
> vfio: unexpected number of irqs 1
>
> Maybe I just pull the CRW_IRQ definition into 5, and leave the wiring of
> the CRW stuff in 7. That seems to leave a better behavior.
Ok, that makes sense.
>
> >
> >>
> >> With this, and the corresponding QEMU series (to be posted momentarily),
> >> applied I am able to configure off/on a CHPID (for example, by issuing
> >> "chchp -c 0/1 xx" on the host), and the guest is able to see both the
> >> events and reflect the updated path masks in its structures.
> >
> > Basically, this looks good to me (modulo my comments).
>
> Woo! Thanks for the feedback; I'm going to try to get them all
> addressed in the next couple of days.
>
> >
> > One thing though that keeps coming up: do we need any kind of
> > serialization? Can there be any confusion from concurrent reads from
> > userspace, or are we sure that we always provide consistent data?
> >
>
> I'm feeling better with the rearrangement in this version of how we get
> data from the queue of CRWs into the region and off to the guest. The
> weirdness I described a few months ago seems to have been triggered by
> one of the patches that's now been dropped. But I'll walk through this
> code again once I get your latest comments applied.
Ok. Might also be nice if somebody else could spend some cycles looking
at this (hint, hint :)
^ permalink raw reply [flat|nested] 21+ messages in thread