* [dpdk-stable] [PATCH] vhost: protect log addr translation in iotlb updates
@ 2020-02-13 10:04 Adrian Moreno
2020-02-13 11:09 ` [dpdk-stable] [dpdk-dev] " Kevin Traynor
2020-02-13 16:18 ` [dpdk-stable] " 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-stable] [dpdk-dev] [PATCH] vhost: protect log addr translation in iotlb updates
2020-02-13 10:04 [dpdk-stable] [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 ` [dpdk-stable] " 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-stable] [dpdk-dev] [PATCH] vhost: protect log addr translation in iotlb updates
2020-02-13 11:09 ` [dpdk-stable] [dpdk-dev] " 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-stable] [PATCH] vhost: protect log addr translation in iotlb updates
2020-02-13 10:04 [dpdk-stable] [PATCH] vhost: protect log addr translation in iotlb updates Adrian Moreno
2020-02-13 11:09 ` [dpdk-stable] [dpdk-dev] " Kevin Traynor
@ 2020-02-13 16:18 ` Maxime Coquelin
2020-02-13 16:27 ` [dpdk-stable] [dpdk-dev] " 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-stable] [dpdk-dev] [PATCH] vhost: protect log addr translation in iotlb updates
2020-02-13 16:18 ` [dpdk-stable] " 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-stable] [PATCH] vhost: protect log addr translation in iotlb updates Adrian Moreno
2020-02-13 11:09 ` [dpdk-stable] [dpdk-dev] " Kevin Traynor
2020-02-13 11:25 ` Adrian Moreno
2020-02-13 16:18 ` [dpdk-stable] " Maxime Coquelin
2020-02-13 16:27 ` [dpdk-stable] [dpdk-dev] " 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).