[10/10] virtio/s390: make airq summary indicators DMA
diff mbox series

Message ID 20190426183245.37939-11-pasic@linux.ibm.com
State New
Headers show
Series
  • s390: virtio: support protected virtualization
Related show

Commit Message

Halil Pasic April 26, 2019, 6:32 p.m. UTC
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 | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

Comments

Pierre Morel May 8, 2019, 3:11 p.m. UTC | #1
On 26/04/2019 20:32, Halil Pasic wrote:
> 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 | 24 +++++++++++++++++-------
>   1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index 613b18001a0c..6058b07fea08 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)
>   			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,8 @@ static void virtio_airq_handler(struct airq_struct *airq)
>   	read_unlock(&info->lock);
>   }
>   
> -static struct airq_info *new_airq_info(void)
> +/* call with airq_areas_lock held */
> +static struct airq_info *new_airq_info(int index)
>   {
>   	struct airq_info *info;
>   	int rc;
> @@ -252,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);
> @@ -273,8 +281,9 @@ static unsigned long get_airq_indicator(struct virtqueue *vqs[], int nvqs,
>   	unsigned long bit, flags;
>   
>   	for (i = 0; i < MAX_AIRQ_AREAS && !indicator_addr; i++) {
> +		/* TODO: this seems to be racy */

yes, my opinions too, was already racy, in my opinion, we need another 
patch in another series to fix this.

However, not sure about the comment.

>   		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;
> @@ -359,7 +368,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);
> @@ -624,7 +633,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;
> @@ -1500,6 +1509,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);
>
Cornelia Huck May 13, 2019, 12:20 p.m. UTC | #2
On Fri, 26 Apr 2019 20:32:45 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> 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 | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)

(...)

> @@ -237,7 +243,8 @@ static void virtio_airq_handler(struct airq_struct *airq)
>  	read_unlock(&info->lock);
>  }
>  
> -static struct airq_info *new_airq_info(void)
> +/* call with airq_areas_lock held */

Hm, where is airq_areas_lock defined? If it was introduced in one of
the previous patches, I have missed it.

> +static struct airq_info *new_airq_info(int index)
>  {
>  	struct airq_info *info;
>  	int rc;
Michael Mueller May 15, 2019, 1:33 p.m. UTC | #3
On 08.05.19 17:11, Pierre Morel wrote:
> On 26/04/2019 20:32, Halil Pasic wrote:
>> 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 | 24 +++++++++++++++++-------
>>   1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/s390/virtio/virtio_ccw.c 
>> b/drivers/s390/virtio/virtio_ccw.c
>> index 613b18001a0c..6058b07fea08 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)
>>               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,8 @@ static void virtio_airq_handler(struct airq_struct 
>> *airq)
>>       read_unlock(&info->lock);
>>   }
>> -static struct airq_info *new_airq_info(void)
>> +/* call with airq_areas_lock held */
>> +static struct airq_info *new_airq_info(int index)
>>   {
>>       struct airq_info *info;
>>       int rc;
>> @@ -252,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);
>> @@ -273,8 +281,9 @@ static unsigned long get_airq_indicator(struct 
>> virtqueue *vqs[], int nvqs,
>>       unsigned long bit, flags;
>>       for (i = 0; i < MAX_AIRQ_AREAS && !indicator_addr; i++) {
>> +        /* TODO: this seems to be racy */
> 
> yes, my opinions too, was already racy, in my opinion, we need another 
> patch in another series to fix this.
> 
> However, not sure about the comment.

I will drop this comment for v2 of this patch series.
We shall fix the race with a separate patch.

Michael

> 
>>           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;
>> @@ -359,7 +368,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);
>> @@ -624,7 +633,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;
>> @@ -1500,6 +1509,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);
>>
> 
> 
>
Michael Mueller May 15, 2019, 1:43 p.m. UTC | #4
On 13.05.19 14:20, Cornelia Huck wrote:
> On Fri, 26 Apr 2019 20:32:45 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> 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 | 24 +++++++++++++++++-------
>>   1 file changed, 17 insertions(+), 7 deletions(-)
> 
> (...)
> 
>> @@ -237,7 +243,8 @@ static void virtio_airq_handler(struct airq_struct *airq)
>>   	read_unlock(&info->lock);
>>   }
>>   
>> -static struct airq_info *new_airq_info(void)
>> +/* call with drivers/s390/virtio/virtio_ccw.cheld */
> 
> Hm, where is airq_areas_lock defined? If it was introduced in one of
> the previous patches, I have missed it.

There is no airq_areas_lock defined currently. My assumption is that
this will be used in context with the likely race condition this
part of the patch is talking about.

@@ -273,8 +281,9 @@ static unsigned long get_airq_indicator(struct 
virtqueue *vqs[], int nvqs,
  	unsigned long bit, flags;

  	for (i = 0; i < MAX_AIRQ_AREAS && !indicator_addr; i++) {
+		/* TODO: this seems to be racy */
  		if (!airq_areas[i])
-			airq_areas[i] = new_airq_info();
+			airq_areas[i] = new_airq_info(i);


As this shall be handled by a separate patch I will drop the comment
in regard to airq_areas_lock from this patch as well for v2.

Michael

> 
>> +static struct airq_info *new_airq_info(int index)
>>   {
>>   	struct airq_info *info;
>>   	int rc;
>
Cornelia Huck May 15, 2019, 1:50 p.m. UTC | #5
On Wed, 15 May 2019 15:43:23 +0200
Michael Mueller <mimu@linux.ibm.com> wrote:

> On 13.05.19 14:20, Cornelia Huck wrote:
> > On Fri, 26 Apr 2019 20:32:45 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> >> 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 | 24 +++++++++++++++++-------
> >>   1 file changed, 17 insertions(+), 7 deletions(-)  
> > 
> > (...)
> >   
> >> @@ -237,7 +243,8 @@ static void virtio_airq_handler(struct airq_struct *airq)
> >>   	read_unlock(&info->lock);
> >>   }
> >>   
> >> -static struct airq_info *new_airq_info(void)
> >> +/* call with drivers/s390/virtio/virtio_ccw.cheld */  
> > 
> > Hm, where is airq_areas_lock defined? If it was introduced in one of
> > the previous patches, I have missed it.  
> 
> There is no airq_areas_lock defined currently. My assumption is that
> this will be used in context with the likely race condition this
> part of the patch is talking about.
> 
> @@ -273,8 +281,9 @@ static unsigned long get_airq_indicator(struct 
> virtqueue *vqs[], int nvqs,
>   	unsigned long bit, flags;
> 
>   	for (i = 0; i < MAX_AIRQ_AREAS && !indicator_addr; i++) {
> +		/* TODO: this seems to be racy */
>   		if (!airq_areas[i])
> -			airq_areas[i] = new_airq_info();
> +			airq_areas[i] = new_airq_info(i);
> 
> 
> As this shall be handled by a separate patch I will drop the comment
> in regard to airq_areas_lock from this patch as well for v2.

Ok, that makes sense.
Halil Pasic May 15, 2019, 5:18 p.m. UTC | #6
On Wed, 15 May 2019 15:43:23 +0200
Michael Mueller <mimu@linux.ibm.com> wrote:

> > Hm, where is airq_areas_lock defined? If it was introduced in one of
> > the previous patches, I have missed it.  
> 
> There is no airq_areas_lock defined currently. My assumption is that
> this will be used in context with the likely race condition this
> part of the patch is talking about.

Right! I first started resolving the race, but then decided to discuss
the issue first, because if I were to just have hallucinated that race,
it would be a lots of wasted effort. Unfortunately I forgot to get rid
of this comment.

Regards,
Halil
Halil Pasic May 15, 2019, 5:23 p.m. UTC | #7
On Wed, 15 May 2019 15:33:02 +0200
Michael Mueller <mimu@linux.ibm.com> wrote:

> >> @@ -273,8 +281,9 @@ static unsigned long get_airq_indicator(struct 
> >> virtqueue *vqs[], int nvqs,
> >>       unsigned long bit, flags;
> >>       for (i = 0; i < MAX_AIRQ_AREAS && !indicator_addr; i++) {
> >> +        /* TODO: this seems to be racy */  
> > 
> > yes, my opinions too, was already racy, in my opinion, we need another 
> > patch in another series to fix this.
> > 
> > However, not sure about the comment.  
> 
> I will drop this comment for v2 of this patch series.
> We shall fix the race with a separate patch.

Unless there is somebody eager to address this real soon, I would prefer
keeping the comment as a reminder.

Thanks for shouldering the v2!

Regards,
Halil

Patch
diff mbox series

diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index 613b18001a0c..6058b07fea08 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)
 			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,8 @@  static void virtio_airq_handler(struct airq_struct *airq)
 	read_unlock(&info->lock);
 }
 
-static struct airq_info *new_airq_info(void)
+/* call with airq_areas_lock held */
+static struct airq_info *new_airq_info(int index)
 {
 	struct airq_info *info;
 	int rc;
@@ -252,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);
@@ -273,8 +281,9 @@  static unsigned long get_airq_indicator(struct virtqueue *vqs[], int nvqs,
 	unsigned long bit, flags;
 
 	for (i = 0; i < MAX_AIRQ_AREAS && !indicator_addr; i++) {
+		/* TODO: this seems to be racy */
 		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;
@@ -359,7 +368,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);
@@ -624,7 +633,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;
@@ -1500,6 +1509,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);