| Message ID | 20190523162209.9543-9-mimu@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390: virtio: support protected virtualization | expand |
On Thu, 23 May 2019 18:22:09 +0200 Michael Mueller <mimu@linux.ibm.com> wrote: > From: Halil Pasic <pasic@linux.ibm.com> > > Hypervisor needs to interact with the summary indicators, so these > need to be DMA memory as well (at least for protected virtualization > guests). > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > --- > drivers/s390/virtio/virtio_ccw.c | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) (...) > @@ -1501,6 +1508,7 @@ static int __init virtio_ccw_init(void) > { > /* parse no_auto string before we do anything further */ > no_auto_parse(); > + summary_indicators = cio_dma_zalloc(MAX_AIRQ_AREAS); What happens if this fails? > return ccw_driver_register(&virtio_ccw_driver); > } > device_initcall(virtio_ccw_init);
On Mon, 27 May 2019 14:00:18 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Thu, 23 May 2019 18:22:09 +0200 > Michael Mueller <mimu@linux.ibm.com> wrote: > > > From: Halil Pasic <pasic@linux.ibm.com> > > > > Hypervisor needs to interact with the summary indicators, so these > > need to be DMA memory as well (at least for protected virtualization > > guests). > > > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > > --- > > drivers/s390/virtio/virtio_ccw.c | 22 +++++++++++++++------- > > 1 file changed, 15 insertions(+), 7 deletions(-) > > (...) > > > @@ -1501,6 +1508,7 @@ static int __init virtio_ccw_init(void) > > { > > /* parse no_auto string before we do anything further */ > > no_auto_parse(); > > + summary_indicators = cio_dma_zalloc(MAX_AIRQ_AREAS); > > What happens if this fails? Bad things could happen! How about adding if (!summary_indicators) virtio_ccw_use_airq = 0; /* fall back to classic */ ? Since it ain't very likely to happen, we could also just fail virtio_ccw_init() with -ENOMEM. Regards, Halil > > > return ccw_driver_register(&virtio_ccw_driver); > > } > > device_initcall(virtio_ccw_init); >
On Tue, 28 May 2019 16:33:42 +0200 Halil Pasic <pasic@linux.ibm.com> wrote: > On Mon, 27 May 2019 14:00:18 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > > > On Thu, 23 May 2019 18:22:09 +0200 > > Michael Mueller <mimu@linux.ibm.com> wrote: > > > > > From: Halil Pasic <pasic@linux.ibm.com> > > > > > > Hypervisor needs to interact with the summary indicators, so these > > > need to be DMA memory as well (at least for protected virtualization > > > guests). > > > > > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > > > --- > > > drivers/s390/virtio/virtio_ccw.c | 22 +++++++++++++++------- > > > 1 file changed, 15 insertions(+), 7 deletions(-) > > > > (...) > > > > > @@ -1501,6 +1508,7 @@ static int __init virtio_ccw_init(void) > > > { > > > /* parse no_auto string before we do anything further */ > > > no_auto_parse(); > > > + summary_indicators = cio_dma_zalloc(MAX_AIRQ_AREAS); > > > > What happens if this fails? > > Bad things could happen! > > How about adding > > if (!summary_indicators) > virtio_ccw_use_airq = 0; /* fall back to classic */ > > ? > > Since it ain't very likely to happen, we could also just fail > virtio_ccw_init() with -ENOMEM. How high are the chances of things working if we fail to allocate here? Returning with -ENOMEM is probably the more reasonable approach here.
On 28.05.19 16:33, Halil Pasic wrote: > On Mon, 27 May 2019 14:00:18 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > >> On Thu, 23 May 2019 18:22:09 +0200 >> Michael Mueller <mimu@linux.ibm.com> wrote: >> >>> From: Halil Pasic <pasic@linux.ibm.com> >>> >>> Hypervisor needs to interact with the summary indicators, so these >>> need to be DMA memory as well (at least for protected virtualization >>> guests). >>> >>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com> >>> --- >>> drivers/s390/virtio/virtio_ccw.c | 22 +++++++++++++++------- >>> 1 file changed, 15 insertions(+), 7 deletions(-) >> >> (...) >> >>> @@ -1501,6 +1508,7 @@ static int __init virtio_ccw_init(void) >>> { >>> /* parse no_auto string before we do anything further */ >>> no_auto_parse(); >>> + summary_indicators = cio_dma_zalloc(MAX_AIRQ_AREAS); >> >> What happens if this fails? > > Bad things could happen! > > How about adding > > if (!summary_indicators) > virtio_ccw_use_airq = 0; /* fall back to classic */ > > ? > > Since it ain't very likely to happen, we could also just fail > virtio_ccw_init() with -ENOMEM. That is what I'm currently doing in v3. > > Regards, > Halil > > >> >>> return ccw_driver_register(&virtio_ccw_driver); >>> } >>> device_initcall(virtio_ccw_init); >> > Michael
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c index 03c9f7001fb1..f666ed397dc0 100644 --- a/drivers/s390/virtio/virtio_ccw.c +++ b/drivers/s390/virtio/virtio_ccw.c @@ -140,11 +140,17 @@ static int virtio_ccw_use_airq = 1; struct airq_info { rwlock_t lock; - u8 summary_indicator; + u8 summary_indicator_idx; struct airq_struct airq; struct airq_iv *aiv; }; static struct airq_info *airq_areas[MAX_AIRQ_AREAS]; +static u8 *summary_indicators; + +static inline u8 *get_summary_indicator(struct airq_info *info) +{ + return summary_indicators + info->summary_indicator_idx; +} #define CCW_CMD_SET_VQ 0x13 #define CCW_CMD_VDEV_RESET 0x33 @@ -225,7 +231,7 @@ static void virtio_airq_handler(struct airq_struct *airq, bool floating) break; vring_interrupt(0, (void *)airq_iv_get_ptr(info->aiv, ai)); } - info->summary_indicator = 0; + *(get_summary_indicator(info)) = 0; smp_wmb(); /* Walk through indicators field, summary indicator not active. */ for (ai = 0;;) { @@ -237,7 +243,7 @@ static void virtio_airq_handler(struct airq_struct *airq, bool floating) read_unlock(&info->lock); } -static struct airq_info *new_airq_info(void) +static struct airq_info *new_airq_info(int index) { struct airq_info *info; int rc; @@ -253,7 +259,8 @@ static struct airq_info *new_airq_info(void) return NULL; } info->airq.handler = virtio_airq_handler; - info->airq.lsi_ptr = &info->summary_indicator; + info->summary_indicator_idx = index; + info->airq.lsi_ptr = get_summary_indicator(info); info->airq.lsi_mask = 0xff; info->airq.isc = VIRTIO_AIRQ_ISC; rc = register_adapter_interrupt(&info->airq); @@ -275,7 +282,7 @@ static unsigned long get_airq_indicator(struct virtqueue *vqs[], int nvqs, for (i = 0; i < MAX_AIRQ_AREAS && !indicator_addr; i++) { if (!airq_areas[i]) - airq_areas[i] = new_airq_info(); + airq_areas[i] = new_airq_info(i); info = airq_areas[i]; if (!info) return 0; @@ -360,7 +367,7 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev, if (!thinint_area) return; thinint_area->summary_indicator = - (unsigned long) &airq_info->summary_indicator; + (unsigned long) get_summary_indicator(airq_info); thinint_area->isc = VIRTIO_AIRQ_ISC; ccw->cmd_code = CCW_CMD_SET_IND_ADAPTER; ccw->count = sizeof(*thinint_area); @@ -625,7 +632,7 @@ static int virtio_ccw_register_adapter_ind(struct virtio_ccw_device *vcdev, } info = vcdev->airq_info; thinint_area->summary_indicator = - (unsigned long) &info->summary_indicator; + (unsigned long) get_summary_indicator(info); thinint_area->isc = VIRTIO_AIRQ_ISC; ccw->cmd_code = CCW_CMD_SET_IND_ADAPTER; ccw->flags = CCW_FLAG_SLI; @@ -1501,6 +1508,7 @@ static int __init virtio_ccw_init(void) { /* parse no_auto string before we do anything further */ no_auto_parse(); + summary_indicators = cio_dma_zalloc(MAX_AIRQ_AREAS); return ccw_driver_register(&virtio_ccw_driver); } device_initcall(virtio_ccw_init);