DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] vhost: protect log addr translation in iotlb updates
@ 2020-02-13 10:04 Adrian Moreno
  2020-02-13 11:09 ` Kevin Traynor
  2020-02-13 16:18 ` Maxime Coquelin
  0 siblings, 2 replies; 5+ messages in thread
From: Adrian Moreno @ 2020-02-13 10:04 UTC (permalink / raw)
  To: dev; +Cc: maxime.coquelin, Tiwei Bie, Zhihong Wang, Adrian Moreno, stable

Currently, the log address translation only  happens in the vhost-user's
translate_ring_addresses(). However, the IOTLB update handler is not
checking if it was mapped to re-trigger that translation.

Since the log address mapping could fail, check it on iotlb updates.
Also, check it on vring_translate() so we do not dirty pages if the
logging address is not yet ready.

Additionally, properly protect the accesses to the iotlb structures.

Fixes: 657414f3788a ("vhost: protect log addr translation in iotlb updates")
Cc: stable@dpdk.org

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 lib/librte_vhost/vhost.c      | 55 +++++++++++++++++++++++++++++++++
 lib/librte_vhost/vhost.h      | 15 +++++++--
 lib/librte_vhost/vhost_user.c | 57 +++++++++++++++--------------------
 3 files changed, 92 insertions(+), 35 deletions(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index c819a8477..442d896ef 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -353,6 +353,56 @@ free_device(struct virtio_net *dev)
 	rte_free(dev);
 }
 
+static __rte_always_inline int
+log_translate(struct virtio_net *dev, struct vhost_virtqueue *vq)
+{
+	if (likely(!(vq->ring_addrs.flags & (1 << VHOST_VRING_F_LOG))))
+		return 0;
+
+	vq->log_guest_addr = translate_log_addr(dev, vq,
+						vq->ring_addrs.log_guest_addr);
+	if (vq->log_guest_addr == 0)
+		return -1;
+
+	return 0;
+}
+
+/*
+ * Converts vring log address to GPA
+ * If IOMMU is enabled, the log address is IOVA
+ * If IOMMU not enabled, the log address is already GPA
+ *
+ * Caller should have iotlb_lock read-locked
+ */
+uint64_t
+translate_log_addr(struct virtio_net *dev, struct vhost_virtqueue *vq,
+		uint64_t log_addr)
+{
+	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)) {
+		const uint64_t exp_size = sizeof(uint64_t);
+		uint64_t hva, gpa;
+		uint64_t size = exp_size;
+
+		hva = vhost_iova_to_vva(dev, vq, log_addr,
+					&size, VHOST_ACCESS_RW);
+
+		if (size != exp_size)
+			return 0;
+
+		gpa = hva_to_gpa(dev, hva, exp_size);
+		if (!gpa) {
+			VHOST_LOG_CONFIG(ERR,
+				"VQ: Failed to find GPA for log_addr: 0x%" PRIx64 " hva: 0x%" PRIx64 "\n",
+				log_addr, hva);
+			return 0;
+		}
+		return gpa;
+
+	} else
+		return log_addr;
+}
+
+/* Caller should have iotlb_lock read-locked */
 static int
 vring_translate_split(struct virtio_net *dev, struct vhost_virtqueue *vq)
 {
@@ -391,6 +441,7 @@ vring_translate_split(struct virtio_net *dev, struct vhost_virtqueue *vq)
 	return 0;
 }
 
+/* Caller should have iotlb_lock read-locked */
 static int
 vring_translate_packed(struct virtio_net *dev, struct vhost_virtqueue *vq)
 {
@@ -437,6 +488,10 @@ vring_translate(struct virtio_net *dev, struct vhost_virtqueue *vq)
 		if (vring_translate_split(dev, vq) < 0)
 			return -1;
 	}
+
+	if (log_translate(dev, vq) < 0)
+		return -1;
+
 	vq->access_ok = 1;
 
 	return 0;
diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index 686ce42a2..2087d1400 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -462,14 +462,23 @@ static __rte_always_inline void
 vhost_log_cache_used_vring(struct virtio_net *dev, struct vhost_virtqueue *vq,
 			uint64_t offset, uint64_t len)
 {
-	vhost_log_cache_write(dev, vq, vq->log_guest_addr + offset, len);
+	if (unlikely(dev->features & (1ULL << VHOST_F_LOG_ALL))) {
+		if (unlikely(vq->log_guest_addr == 0))
+			return;
+		__vhost_log_cache_write(dev, vq, vq->log_guest_addr + offset,
+					len);
+	}
 }
 
 static __rte_always_inline void
 vhost_log_used_vring(struct virtio_net *dev, struct vhost_virtqueue *vq,
 		     uint64_t offset, uint64_t len)
 {
-	vhost_log_write(dev, vq->log_guest_addr + offset, len);
+	if (unlikely(dev->features & (1ULL << VHOST_F_LOG_ALL))) {
+		if (unlikely(vq->log_guest_addr == 0))
+			return;
+		__vhost_log_write(dev, vq->log_guest_addr + offset, len);
+	}
 }
 
 static __rte_always_inline void
@@ -626,6 +635,8 @@ void *vhost_alloc_copy_ind_table(struct virtio_net *dev,
 			struct vhost_virtqueue *vq,
 			uint64_t desc_addr, uint64_t desc_len);
 int vring_translate(struct virtio_net *dev, struct vhost_virtqueue *vq);
+uint64_t translate_log_addr(struct virtio_net *dev, struct vhost_virtqueue *vq,
+		uint64_t log_addr);
 void vring_invalidate(struct virtio_net *dev, struct vhost_virtqueue *vq);
 
 static __rte_always_inline uint64_t
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 69b84a882..5db9f148f 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -656,13 +656,11 @@ ring_addr_to_vva(struct virtio_net *dev, struct vhost_virtqueue *vq,
 {
 	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)) {
 		uint64_t vva;
-		uint64_t req_size = *size;
 
-		vva = vhost_user_iotlb_cache_find(vq, ra,
+		vhost_user_iotlb_rd_lock(vq);
+		vva = vhost_iova_to_vva(dev, vq, ra,
 					size, VHOST_ACCESS_RW);
-		if (req_size != *size)
-			vhost_user_iotlb_miss(dev, (ra + *size),
-					      VHOST_ACCESS_RW);
+		vhost_user_iotlb_rd_unlock(vq);
 
 		return vva;
 	}
@@ -670,37 +668,16 @@ ring_addr_to_vva(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	return qva_to_vva(dev, ra, size);
 }
 
-/*
- * Converts vring log address to GPA
- * If IOMMU is enabled, the log address is IOVA
- * If IOMMU not enabled, the log address is already GPA
- */
 static uint64_t
-translate_log_addr(struct virtio_net *dev, struct vhost_virtqueue *vq,
-		uint64_t log_addr)
+log_addr_to_gpa(struct virtio_net *dev, struct vhost_virtqueue *vq)
 {
-	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)) {
-		const uint64_t exp_size = sizeof(struct vring_used) +
-			sizeof(struct vring_used_elem) * vq->size;
-		uint64_t hva, gpa;
-		uint64_t size = exp_size;
-
-		hva = vhost_iova_to_vva(dev, vq, log_addr,
-					&size, VHOST_ACCESS_RW);
-		if (size != exp_size)
-			return 0;
+	uint64_t log_gpa;
 
-		gpa = hva_to_gpa(dev, hva, exp_size);
-		if (!gpa) {
-			VHOST_LOG_CONFIG(ERR,
-				"VQ: Failed to find GPA for log_addr: 0x%" PRIx64 " hva: 0x%" PRIx64 "\n",
-				log_addr, hva);
-			return 0;
-		}
-		return gpa;
+	vhost_user_iotlb_rd_lock(vq);
+	log_gpa = translate_log_addr(dev, vq, vq->ring_addrs.log_guest_addr);
+	vhost_user_iotlb_rd_unlock(vq);
 
-	} else
-		return log_addr;
+	return log_gpa;
 }
 
 static struct virtio_net *
@@ -712,7 +689,7 @@ translate_ring_addresses(struct virtio_net *dev, int vq_index)
 
 	if (addr->flags & (1 << VHOST_VRING_F_LOG)) {
 		vq->log_guest_addr =
-			translate_log_addr(dev, vq, addr->log_guest_addr);
+			log_addr_to_gpa(dev, vq);
 		if (vq->log_guest_addr == 0) {
 			VHOST_LOG_CONFIG(DEBUG,
 				"(%d) failed to map log_guest_addr.\n",
@@ -2229,6 +2206,13 @@ is_vring_iotlb_split(struct vhost_virtqueue *vq, struct vhost_iotlb_msg *imsg)
 	if (ra->used_user_addr < end && (ra->used_user_addr + len) > start)
 		return 1;
 
+	if (ra->flags & (1 << VHOST_VRING_F_LOG)) {
+		len = sizeof(uint64_t);
+		if (ra->log_guest_addr < end &&
+		    (ra->log_guest_addr + len) > start)
+			return 1;
+	}
+
 	return 0;
 }
 
@@ -2254,6 +2238,13 @@ is_vring_iotlb_packed(struct vhost_virtqueue *vq, struct vhost_iotlb_msg *imsg)
 	if (ra->used_user_addr < end && (ra->used_user_addr + len) > start)
 		return 1;
 
+	if (ra->flags & (1 << VHOST_VRING_F_LOG)) {
+		len = sizeof(uint64_t);
+		if (ra->log_guest_addr < end &&
+		    (ra->log_guest_addr + len) > start)
+			return 1;
+	}
+
 	return 0;
 }
 
-- 
2.24.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: protect log addr translation in iotlb updates
  2020-02-13 10:04 [dpdk-dev] [PATCH] vhost: protect log addr translation in iotlb updates Adrian Moreno
@ 2020-02-13 11:09 ` Kevin Traynor
  2020-02-13 11:25   ` Adrian Moreno
  2020-02-13 16:18 ` Maxime Coquelin
  1 sibling, 1 reply; 5+ messages in thread
From: Kevin Traynor @ 2020-02-13 11:09 UTC (permalink / raw)
  To: Adrian Moreno, dev; +Cc: maxime.coquelin, Tiwei Bie, Zhihong Wang, stable

On 13/02/2020 10:04, Adrian Moreno wrote:
> Currently, the log address translation only  happens in the vhost-user's
> translate_ring_addresses(). However, the IOTLB update handler is not
> checking if it was mapped to re-trigger that translation.
> 
> Since the log address mapping could fail, check it on iotlb updates.
> Also, check it on vring_translate() so we do not dirty pages if the
> logging address is not yet ready.
> 
> Additionally, properly protect the accesses to the iotlb structures.
> 
> Fixes: 657414f3788a ("vhost: protect log addr translation in iotlb updates")

This fixes looks incorrect, it's suspiciously the same name and I
couldn't find the commit-id in upstream.

> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  lib/librte_vhost/vhost.c      | 55 +++++++++++++++++++++++++++++++++
>  lib/librte_vhost/vhost.h      | 15 +++++++--
>  lib/librte_vhost/vhost_user.c | 57 +++++++++++++++--------------------
>  3 files changed, 92 insertions(+), 35 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index c819a8477..442d896ef 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -353,6 +353,56 @@ free_device(struct virtio_net *dev)
>  	rte_free(dev);
>  }
>  
> +static __rte_always_inline int
> +log_translate(struct virtio_net *dev, struct vhost_virtqueue *vq)
> +{
> +	if (likely(!(vq->ring_addrs.flags & (1 << VHOST_VRING_F_LOG))))
> +		return 0;
> +
> +	vq->log_guest_addr = translate_log_addr(dev, vq,
> +						vq->ring_addrs.log_guest_addr);
> +	if (vq->log_guest_addr == 0)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +/*
> + * Converts vring log address to GPA
> + * If IOMMU is enabled, the log address is IOVA
> + * If IOMMU not enabled, the log address is already GPA
> + *
> + * Caller should have iotlb_lock read-locked
> + */
> +uint64_t
> +translate_log_addr(struct virtio_net *dev, struct vhost_virtqueue *vq,
> +		uint64_t log_addr)
> +{
> +	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)) {
> +		const uint64_t exp_size = sizeof(uint64_t);
> +		uint64_t hva, gpa;
> +		uint64_t size = exp_size;
> +
> +		hva = vhost_iova_to_vva(dev, vq, log_addr,
> +					&size, VHOST_ACCESS_RW);
> +
> +		if (size != exp_size)
> +			return 0;
> +
> +		gpa = hva_to_gpa(dev, hva, exp_size);
> +		if (!gpa) {
> +			VHOST_LOG_CONFIG(ERR,
> +				"VQ: Failed to find GPA for log_addr: 0x%" PRIx64 " hva: 0x%" PRIx64 "\n",
> +				log_addr, hva);
> +			return 0;
> +		}
> +		return gpa;
> +
> +	} else
> +		return log_addr;
> +}
> +
> +/* Caller should have iotlb_lock read-locked */
>  static int
>  vring_translate_split(struct virtio_net *dev, struct vhost_virtqueue *vq)
>  {
> @@ -391,6 +441,7 @@ vring_translate_split(struct virtio_net *dev, struct vhost_virtqueue *vq)
>  	return 0;
>  }
>  
> +/* Caller should have iotlb_lock read-locked */
>  static int
>  vring_translate_packed(struct virtio_net *dev, struct vhost_virtqueue *vq)
>  {
> @@ -437,6 +488,10 @@ vring_translate(struct virtio_net *dev, struct vhost_virtqueue *vq)
>  		if (vring_translate_split(dev, vq) < 0)
>  			return -1;
>  	}
> +
> +	if (log_translate(dev, vq) < 0)
> +		return -1;
> +
>  	vq->access_ok = 1;
>  
>  	return 0;
> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> index 686ce42a2..2087d1400 100644
> --- a/lib/librte_vhost/vhost.h
> +++ b/lib/librte_vhost/vhost.h
> @@ -462,14 +462,23 @@ static __rte_always_inline void
>  vhost_log_cache_used_vring(struct virtio_net *dev, struct vhost_virtqueue *vq,
>  			uint64_t offset, uint64_t len)
>  {
> -	vhost_log_cache_write(dev, vq, vq->log_guest_addr + offset, len);
> +	if (unlikely(dev->features & (1ULL << VHOST_F_LOG_ALL))) {
> +		if (unlikely(vq->log_guest_addr == 0))
> +			return;
> +		__vhost_log_cache_write(dev, vq, vq->log_guest_addr + offset,
> +					len);
> +	}
>  }
>  
>  static __rte_always_inline void
>  vhost_log_used_vring(struct virtio_net *dev, struct vhost_virtqueue *vq,
>  		     uint64_t offset, uint64_t len)
>  {
> -	vhost_log_write(dev, vq->log_guest_addr + offset, len);
> +	if (unlikely(dev->features & (1ULL << VHOST_F_LOG_ALL))) {
> +		if (unlikely(vq->log_guest_addr == 0))
> +			return;
> +		__vhost_log_write(dev, vq->log_guest_addr + offset, len);
> +	}
>  }
>  
>  static __rte_always_inline void
> @@ -626,6 +635,8 @@ void *vhost_alloc_copy_ind_table(struct virtio_net *dev,
>  			struct vhost_virtqueue *vq,
>  			uint64_t desc_addr, uint64_t desc_len);
>  int vring_translate(struct virtio_net *dev, struct vhost_virtqueue *vq);
> +uint64_t translate_log_addr(struct virtio_net *dev, struct vhost_virtqueue *vq,
> +		uint64_t log_addr);
>  void vring_invalidate(struct virtio_net *dev, struct vhost_virtqueue *vq);
>  
>  static __rte_always_inline uint64_t
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 69b84a882..5db9f148f 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -656,13 +656,11 @@ ring_addr_to_vva(struct virtio_net *dev, struct vhost_virtqueue *vq,
>  {
>  	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)) {
>  		uint64_t vva;
> -		uint64_t req_size = *size;
>  
> -		vva = vhost_user_iotlb_cache_find(vq, ra,
> +		vhost_user_iotlb_rd_lock(vq);
> +		vva = vhost_iova_to_vva(dev, vq, ra,
>  					size, VHOST_ACCESS_RW);
> -		if (req_size != *size)
> -			vhost_user_iotlb_miss(dev, (ra + *size),
> -					      VHOST_ACCESS_RW);
> +		vhost_user_iotlb_rd_unlock(vq);
>  
>  		return vva;
>  	}
> @@ -670,37 +668,16 @@ ring_addr_to_vva(struct virtio_net *dev, struct vhost_virtqueue *vq,
>  	return qva_to_vva(dev, ra, size);
>  }
>  
> -/*
> - * Converts vring log address to GPA
> - * If IOMMU is enabled, the log address is IOVA
> - * If IOMMU not enabled, the log address is already GPA
> - */
>  static uint64_t
> -translate_log_addr(struct virtio_net *dev, struct vhost_virtqueue *vq,
> -		uint64_t log_addr)
> +log_addr_to_gpa(struct virtio_net *dev, struct vhost_virtqueue *vq)
>  {
> -	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)) {
> -		const uint64_t exp_size = sizeof(struct vring_used) +
> -			sizeof(struct vring_used_elem) * vq->size;
> -		uint64_t hva, gpa;
> -		uint64_t size = exp_size;
> -
> -		hva = vhost_iova_to_vva(dev, vq, log_addr,
> -					&size, VHOST_ACCESS_RW);
> -		if (size != exp_size)
> -			return 0;
> +	uint64_t log_gpa;
>  
> -		gpa = hva_to_gpa(dev, hva, exp_size);
> -		if (!gpa) {
> -			VHOST_LOG_CONFIG(ERR,
> -				"VQ: Failed to find GPA for log_addr: 0x%" PRIx64 " hva: 0x%" PRIx64 "\n",
> -				log_addr, hva);
> -			return 0;
> -		}
> -		return gpa;
> +	vhost_user_iotlb_rd_lock(vq);
> +	log_gpa = translate_log_addr(dev, vq, vq->ring_addrs.log_guest_addr);
> +	vhost_user_iotlb_rd_unlock(vq);
>  
> -	} else
> -		return log_addr;
> +	return log_gpa;
>  }
>  
>  static struct virtio_net *
> @@ -712,7 +689,7 @@ translate_ring_addresses(struct virtio_net *dev, int vq_index)
>  
>  	if (addr->flags & (1 << VHOST_VRING_F_LOG)) {
>  		vq->log_guest_addr =
> -			translate_log_addr(dev, vq, addr->log_guest_addr);
> +			log_addr_to_gpa(dev, vq);
>  		if (vq->log_guest_addr == 0) {
>  			VHOST_LOG_CONFIG(DEBUG,
>  				"(%d) failed to map log_guest_addr.\n",
> @@ -2229,6 +2206,13 @@ is_vring_iotlb_split(struct vhost_virtqueue *vq, struct vhost_iotlb_msg *imsg)
>  	if (ra->used_user_addr < end && (ra->used_user_addr + len) > start)
>  		return 1;
>  
> +	if (ra->flags & (1 << VHOST_VRING_F_LOG)) {
> +		len = sizeof(uint64_t);
> +		if (ra->log_guest_addr < end &&
> +		    (ra->log_guest_addr + len) > start)
> +			return 1;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -2254,6 +2238,13 @@ is_vring_iotlb_packed(struct vhost_virtqueue *vq, struct vhost_iotlb_msg *imsg)
>  	if (ra->used_user_addr < end && (ra->used_user_addr + len) > start)
>  		return 1;
>  
> +	if (ra->flags & (1 << VHOST_VRING_F_LOG)) {
> +		len = sizeof(uint64_t);
> +		if (ra->log_guest_addr < end &&
> +		    (ra->log_guest_addr + len) > start)
> +			return 1;
> +	}
> +
>  	return 0;
>  }
>  
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: protect log addr translation in iotlb updates
  2020-02-13 11:09 ` Kevin Traynor
@ 2020-02-13 11:25   ` Adrian Moreno
  0 siblings, 0 replies; 5+ messages in thread
From: Adrian Moreno @ 2020-02-13 11:25 UTC (permalink / raw)
  To: Kevin Traynor, dev; +Cc: maxime.coquelin, Tiwei Bie, Zhihong Wang, stable

On 2/13/20 12:09 PM, Kevin Traynor wrote:
> On 13/02/2020 10:04, Adrian Moreno wrote:
>> Currently, the log address translation only  happens in the vhost-user's
>> translate_ring_addresses(). However, the IOTLB update handler is not
>> checking if it was mapped to re-trigger that translation.
>>
>> Since the log address mapping could fail, check it on iotlb updates.
>> Also, check it on vring_translate() so we do not dirty pages if the
>> logging address is not yet ready.
>>
>> Additionally, properly protect the accesses to the iotlb structures.
>>
>> Fixes: 657414f3788a ("vhost: protect log addr translation in iotlb updates")
> 
> This fixes looks incorrect, it's suspiciously the same name and I
> couldn't find the commit-id in upstream.
> 
Thanks for spotting that Kevin. You're right.
The correct line should be:

Fixes: fbda9f145927 ("vhost: translate incoming log address to GPA")

I'll wait for more comments and repost with the correct Fixes line.

Adrian.
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> ---
>>  lib/librte_vhost/vhost.c      | 55 +++++++++++++++++++++++++++++++++
>>  lib/librte_vhost/vhost.h      | 15 +++++++--
>>  lib/librte_vhost/vhost_user.c | 57 +++++++++++++++--------------------
>>  3 files changed, 92 insertions(+), 35 deletions(-)
>>
>> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
>> index c819a8477..442d896ef 100644
>> --- a/lib/librte_vhost/vhost.c
>> +++ b/lib/librte_vhost/vhost.c
>> @@ -353,6 +353,56 @@ free_device(struct virtio_net *dev)
>>  	rte_free(dev);
>>  }
>>  
>> +static __rte_always_inline int
>> +log_translate(struct virtio_net *dev, struct vhost_virtqueue *vq)
>> +{
>> +	if (likely(!(vq->ring_addrs.flags & (1 << VHOST_VRING_F_LOG))))
>> +		return 0;
>> +
>> +	vq->log_guest_addr = translate_log_addr(dev, vq,
>> +						vq->ring_addrs.log_guest_addr);
>> +	if (vq->log_guest_addr == 0)
>> +		return -1;
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Converts vring log address to GPA
>> + * If IOMMU is enabled, the log address is IOVA
>> + * If IOMMU not enabled, the log address is already GPA
>> + *
>> + * Caller should have iotlb_lock read-locked
>> + */
>> +uint64_t
>> +translate_log_addr(struct virtio_net *dev, struct vhost_virtqueue *vq,
>> +		uint64_t log_addr)
>> +{
>> +	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)) {
>> +		const uint64_t exp_size = sizeof(uint64_t);
>> +		uint64_t hva, gpa;
>> +		uint64_t size = exp_size;
>> +
>> +		hva = vhost_iova_to_vva(dev, vq, log_addr,
>> +					&size, VHOST_ACCESS_RW);
>> +
>> +		if (size != exp_size)
>> +			return 0;
>> +
>> +		gpa = hva_to_gpa(dev, hva, exp_size);
>> +		if (!gpa) {
>> +			VHOST_LOG_CONFIG(ERR,
>> +				"VQ: Failed to find GPA for log_addr: 0x%" PRIx64 " hva: 0x%" PRIx64 "\n",
>> +				log_addr, hva);
>> +			return 0;
>> +		}
>> +		return gpa;
>> +
>> +	} else
>> +		return log_addr;
>> +}
>> +
>> +/* Caller should have iotlb_lock read-locked */
>>  static int
>>  vring_translate_split(struct virtio_net *dev, struct vhost_virtqueue *vq)
>>  {
>> @@ -391,6 +441,7 @@ vring_translate_split(struct virtio_net *dev, struct vhost_virtqueue *vq)
>>  	return 0;
>>  }
>>  
>> +/* Caller should have iotlb_lock read-locked */
>>  static int
>>  vring_translate_packed(struct virtio_net *dev, struct vhost_virtqueue *vq)
>>  {
>> @@ -437,6 +488,10 @@ vring_translate(struct virtio_net *dev, struct vhost_virtqueue *vq)
>>  		if (vring_translate_split(dev, vq) < 0)
>>  			return -1;
>>  	}
>> +
>> +	if (log_translate(dev, vq) < 0)
>> +		return -1;
>> +
>>  	vq->access_ok = 1;
>>  
>>  	return 0;
>> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
>> index 686ce42a2..2087d1400 100644
>> --- a/lib/librte_vhost/vhost.h
>> +++ b/lib/librte_vhost/vhost.h
>> @@ -462,14 +462,23 @@ static __rte_always_inline void
>>  vhost_log_cache_used_vring(struct virtio_net *dev, struct vhost_virtqueue *vq,
>>  			uint64_t offset, uint64_t len)
>>  {
>> -	vhost_log_cache_write(dev, vq, vq->log_guest_addr + offset, len);
>> +	if (unlikely(dev->features & (1ULL << VHOST_F_LOG_ALL))) {
>> +		if (unlikely(vq->log_guest_addr == 0))
>> +			return;
>> +		__vhost_log_cache_write(dev, vq, vq->log_guest_addr + offset,
>> +					len);
>> +	}
>>  }
>>  
>>  static __rte_always_inline void
>>  vhost_log_used_vring(struct virtio_net *dev, struct vhost_virtqueue *vq,
>>  		     uint64_t offset, uint64_t len)
>>  {
>> -	vhost_log_write(dev, vq->log_guest_addr + offset, len);
>> +	if (unlikely(dev->features & (1ULL << VHOST_F_LOG_ALL))) {
>> +		if (unlikely(vq->log_guest_addr == 0))
>> +			return;
>> +		__vhost_log_write(dev, vq->log_guest_addr + offset, len);
>> +	}
>>  }
>>  
>>  static __rte_always_inline void
>> @@ -626,6 +635,8 @@ void *vhost_alloc_copy_ind_table(struct virtio_net *dev,
>>  			struct vhost_virtqueue *vq,
>>  			uint64_t desc_addr, uint64_t desc_len);
>>  int vring_translate(struct virtio_net *dev, struct vhost_virtqueue *vq);
>> +uint64_t translate_log_addr(struct virtio_net *dev, struct vhost_virtqueue *vq,
>> +		uint64_t log_addr);
>>  void vring_invalidate(struct virtio_net *dev, struct vhost_virtqueue *vq);
>>  
>>  static __rte_always_inline uint64_t
>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>> index 69b84a882..5db9f148f 100644
>> --- a/lib/librte_vhost/vhost_user.c
>> +++ b/lib/librte_vhost/vhost_user.c
>> @@ -656,13 +656,11 @@ ring_addr_to_vva(struct virtio_net *dev, struct vhost_virtqueue *vq,
>>  {
>>  	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)) {
>>  		uint64_t vva;
>> -		uint64_t req_size = *size;
>>  
>> -		vva = vhost_user_iotlb_cache_find(vq, ra,
>> +		vhost_user_iotlb_rd_lock(vq);
>> +		vva = vhost_iova_to_vva(dev, vq, ra,
>>  					size, VHOST_ACCESS_RW);
>> -		if (req_size != *size)
>> -			vhost_user_iotlb_miss(dev, (ra + *size),
>> -					      VHOST_ACCESS_RW);
>> +		vhost_user_iotlb_rd_unlock(vq);
>>  
>>  		return vva;
>>  	}
>> @@ -670,37 +668,16 @@ ring_addr_to_vva(struct virtio_net *dev, struct vhost_virtqueue *vq,
>>  	return qva_to_vva(dev, ra, size);
>>  }
>>  
>> -/*
>> - * Converts vring log address to GPA
>> - * If IOMMU is enabled, the log address is IOVA
>> - * If IOMMU not enabled, the log address is already GPA
>> - */
>>  static uint64_t
>> -translate_log_addr(struct virtio_net *dev, struct vhost_virtqueue *vq,
>> -		uint64_t log_addr)
>> +log_addr_to_gpa(struct virtio_net *dev, struct vhost_virtqueue *vq)
>>  {
>> -	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)) {
>> -		const uint64_t exp_size = sizeof(struct vring_used) +
>> -			sizeof(struct vring_used_elem) * vq->size;
>> -		uint64_t hva, gpa;
>> -		uint64_t size = exp_size;
>> -
>> -		hva = vhost_iova_to_vva(dev, vq, log_addr,
>> -					&size, VHOST_ACCESS_RW);
>> -		if (size != exp_size)
>> -			return 0;
>> +	uint64_t log_gpa;
>>  
>> -		gpa = hva_to_gpa(dev, hva, exp_size);
>> -		if (!gpa) {
>> -			VHOST_LOG_CONFIG(ERR,
>> -				"VQ: Failed to find GPA for log_addr: 0x%" PRIx64 " hva: 0x%" PRIx64 "\n",
>> -				log_addr, hva);
>> -			return 0;
>> -		}
>> -		return gpa;
>> +	vhost_user_iotlb_rd_lock(vq);
>> +	log_gpa = translate_log_addr(dev, vq, vq->ring_addrs.log_guest_addr);
>> +	vhost_user_iotlb_rd_unlock(vq);
>>  
>> -	} else
>> -		return log_addr;
>> +	return log_gpa;
>>  }
>>  
>>  static struct virtio_net *
>> @@ -712,7 +689,7 @@ translate_ring_addresses(struct virtio_net *dev, int vq_index)
>>  
>>  	if (addr->flags & (1 << VHOST_VRING_F_LOG)) {
>>  		vq->log_guest_addr =
>> -			translate_log_addr(dev, vq, addr->log_guest_addr);
>> +			log_addr_to_gpa(dev, vq);
>>  		if (vq->log_guest_addr == 0) {
>>  			VHOST_LOG_CONFIG(DEBUG,
>>  				"(%d) failed to map log_guest_addr.\n",
>> @@ -2229,6 +2206,13 @@ is_vring_iotlb_split(struct vhost_virtqueue *vq, struct vhost_iotlb_msg *imsg)
>>  	if (ra->used_user_addr < end && (ra->used_user_addr + len) > start)
>>  		return 1;
>>  
>> +	if (ra->flags & (1 << VHOST_VRING_F_LOG)) {
>> +		len = sizeof(uint64_t);
>> +		if (ra->log_guest_addr < end &&
>> +		    (ra->log_guest_addr + len) > start)
>> +			return 1;
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> @@ -2254,6 +2238,13 @@ is_vring_iotlb_packed(struct vhost_virtqueue *vq, struct vhost_iotlb_msg *imsg)
>>  	if (ra->used_user_addr < end && (ra->used_user_addr + len) > start)
>>  		return 1;
>>  
>> +	if (ra->flags & (1 << VHOST_VRING_F_LOG)) {
>> +		len = sizeof(uint64_t);
>> +		if (ra->log_guest_addr < end &&
>> +		    (ra->log_guest_addr + len) > start)
>> +			return 1;
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>>
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: protect log addr translation in iotlb updates
  2020-02-13 10:04 [dpdk-dev] [PATCH] vhost: protect log addr translation in iotlb updates Adrian Moreno
  2020-02-13 11:09 ` Kevin Traynor
@ 2020-02-13 16:18 ` Maxime Coquelin
  2020-02-13 16:27   ` Maxime Coquelin
  1 sibling, 1 reply; 5+ messages in thread
From: Maxime Coquelin @ 2020-02-13 16:18 UTC (permalink / raw)
  To: Adrian Moreno, dev; +Cc: Tiwei Bie, Zhihong Wang, stable



On 2/13/20 11:04 AM, Adrian Moreno wrote:
> Currently, the log address translation only  happens in the vhost-user's
> translate_ring_addresses(). However, the IOTLB update handler is not
> checking if it was mapped to re-trigger that translation.
> 
> Since the log address mapping could fail, check it on iotlb updates.
> Also, check it on vring_translate() so we do not dirty pages if the
> logging address is not yet ready.
> 
> Additionally, properly protect the accesses to the iotlb structures.
> 
> Fixes: 657414f3788a ("vhost: protect log addr translation in iotlb updates")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  lib/librte_vhost/vhost.c      | 55 +++++++++++++++++++++++++++++++++
>  lib/librte_vhost/vhost.h      | 15 +++++++--
>  lib/librte_vhost/vhost_user.c | 57 +++++++++++++++--------------------
>  3 files changed, 92 insertions(+), 35 deletions(-)
> 


Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

I will fix the fixline while applying.

Thanks!
Maxime


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: protect log addr translation in iotlb updates
  2020-02-13 16:18 ` Maxime Coquelin
@ 2020-02-13 16:27   ` Maxime Coquelin
  0 siblings, 0 replies; 5+ messages in thread
From: Maxime Coquelin @ 2020-02-13 16:27 UTC (permalink / raw)
  To: Adrian Moreno, dev; +Cc: Tiwei Bie, Zhihong Wang, stable



On 2/13/20 5:18 PM, Maxime Coquelin wrote:
> 
> 
> On 2/13/20 11:04 AM, Adrian Moreno wrote:
>> Currently, the log address translation only  happens in the vhost-user's
>> translate_ring_addresses(). However, the IOTLB update handler is not
>> checking if it was mapped to re-trigger that translation.
>>
>> Since the log address mapping could fail, check it on iotlb updates.
>> Also, check it on vring_translate() so we do not dirty pages if the
>> logging address is not yet ready.
>>
>> Additionally, properly protect the accesses to the iotlb structures.
>>
>> Fixes: 657414f3788a ("vhost: protect log addr translation in iotlb updates")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> ---
>>  lib/librte_vhost/vhost.c      | 55 +++++++++++++++++++++++++++++++++
>>  lib/librte_vhost/vhost.h      | 15 +++++++--
>>  lib/librte_vhost/vhost_user.c | 57 +++++++++++++++--------------------
>>  3 files changed, 92 insertions(+), 35 deletions(-)
>>
> 
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> I will fix the fixline while applying.

Applied to dpdk-next-virtio/master

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-02-13 16:27 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-13 10:04 [dpdk-dev] [PATCH] vhost: protect log addr translation in iotlb updates Adrian Moreno
2020-02-13 11:09 ` Kevin Traynor
2020-02-13 11:25   ` Adrian Moreno
2020-02-13 16:18 ` Maxime Coquelin
2020-02-13 16:27   ` Maxime Coquelin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).