[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

RE: [PATCH V4 12/13] hv_netvsc: Add Isolation VM support for netvsc driver



From: Tianyu Lan <ltykernel@xxxxxxxxx> Sent: Friday, August 27, 2021 10:21 AM
> 
> In Isolation VM, all shared memory with host needs to mark visible
> to host via hvcall. vmbus_establish_gpadl() has already done it for
> netvsc rx/tx ring buffer. The page buffer used by vmbus_sendpacket_
> pagebuffer() stills need to be handled. Use DMA API to map/umap
> these memory during sending/receiving packet and Hyper-V swiotlb
> bounce buffer dma adress will be returned. The swiotlb bounce buffer
> has been masked to be visible to host during boot up.
> 
> Signed-off-by: Tianyu Lan <Tianyu.Lan@xxxxxxxxxxxxx>
> ---
> Change since v3:
>       * Add comment to explain why not to use dma_map_sg()
>       * Fix some error handle.
> ---
>  arch/x86/hyperv/ivm.c             |   1 +
>  drivers/net/hyperv/hyperv_net.h   |   5 ++
>  drivers/net/hyperv/netvsc.c       | 135 +++++++++++++++++++++++++++++-
>  drivers/net/hyperv/rndis_filter.c |   2 +
>  include/linux/hyperv.h            |   5 ++
>  5 files changed, 145 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
> index 84563b3c9f3a..08d8e01de017 100644
> --- a/arch/x86/hyperv/ivm.c
> +++ b/arch/x86/hyperv/ivm.c
> @@ -317,6 +317,7 @@ void *hv_map_memory(void *addr, unsigned long size)
> 
>       return vaddr;
>  }
> +EXPORT_SYMBOL_GPL(hv_map_memory);
> 
>  void hv_unmap_memory(void *addr)
>  {
> diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
> index aa7c9962dbd8..862419912bfb 100644
> --- a/drivers/net/hyperv/hyperv_net.h
> +++ b/drivers/net/hyperv/hyperv_net.h
> @@ -164,6 +164,7 @@ struct hv_netvsc_packet {
>       u32 total_bytes;
>       u32 send_buf_index;
>       u32 total_data_buflen;
> +     struct hv_dma_range *dma_range;
>  };
> 
>  #define NETVSC_HASH_KEYLEN 40
> @@ -1074,6 +1075,7 @@ struct netvsc_device {
> 
>       /* Receive buffer allocated by us but manages by NetVSP */
>       void *recv_buf;
> +     void *recv_original_buf;
>       u32 recv_buf_size; /* allocated bytes */
>       u32 recv_buf_gpadl_handle;
>       u32 recv_section_cnt;
> @@ -1082,6 +1084,7 @@ struct netvsc_device {
> 
>       /* Send buffer allocated by us */
>       void *send_buf;
> +     void *send_original_buf;
>       u32 send_buf_size;
>       u32 send_buf_gpadl_handle;
>       u32 send_section_cnt;
> @@ -1731,4 +1734,6 @@ struct rndis_message {
>  #define RETRY_US_HI  10000
>  #define RETRY_MAX    2000    /* >10 sec */
> 
> +void netvsc_dma_unmap(struct hv_device *hv_dev,
> +                   struct hv_netvsc_packet *packet);
>  #endif /* _HYPERV_NET_H */
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index f19bffff6a63..edd336b08c2c 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -153,8 +153,21 @@ static void free_netvsc_device(struct rcu_head *head)
>       int i;
> 
>       kfree(nvdev->extension);
> -     vfree(nvdev->recv_buf);
> -     vfree(nvdev->send_buf);
> +
> +     if (nvdev->recv_original_buf) {
> +             vunmap(nvdev->recv_buf);

In patch 11, you have added a hv_unmap_memory()
function as the inverse of hv_map_memory().  Since this
buffer was mapped with hv_map_memory() and you have
added that function, the cleanup should use
hv_unmap_memory() rather than calling vunmap() directly.

> +             vfree(nvdev->recv_original_buf);
> +     } else {
> +             vfree(nvdev->recv_buf);
> +     }
> +
> +     if (nvdev->send_original_buf) {
> +             vunmap(nvdev->send_buf);

Same here.

> +             vfree(nvdev->send_original_buf);
> +     } else {
> +             vfree(nvdev->send_buf);
> +     }
> +
>       kfree(nvdev->send_section_map);
> 
>       for (i = 0; i < VRSS_CHANNEL_MAX; i++) {
> @@ -347,6 +360,7 @@ static int netvsc_init_buf(struct hv_device *device,
>       unsigned int buf_size;
>       size_t map_words;
>       int i, ret = 0;
> +     void *vaddr;
> 
>       /* Get receive buffer area. */
>       buf_size = device_info->recv_sections * device_info->recv_section_size;
> @@ -382,6 +396,17 @@ static int netvsc_init_buf(struct hv_device *device,
>               goto cleanup;
>       }
> 
> +     if (hv_isolation_type_snp()) {
> +             vaddr = hv_map_memory(net_device->recv_buf, buf_size);

Since the netvsc driver is architecture neutral, this code also needs
to compile for ARM64.  A stub will be needed for hv_map_memory()
on the ARM64 side.  Same for hv_unmap_memory() as suggested
above.  Or better, move hv_map_memory() and hv_unmap_memory()
to an architecture neutral module such as hv_common.c.

Or if Christop's approach of creating the vmap_phys_addr() helper
comes to fruition, that's an even better approach since it will already
handle multiple architectures.

> +             if (!vaddr) {
> +                     ret = -ENOMEM;
> +                     goto cleanup;
> +             }
> +
> +             net_device->recv_original_buf = net_device->recv_buf;
> +             net_device->recv_buf = vaddr;
> +     }
> +
>       /* Notify the NetVsp of the gpadl handle */
>       init_packet = &net_device->channel_init_pkt;
>       memset(init_packet, 0, sizeof(struct nvsp_message));
> @@ -485,6 +510,17 @@ static int netvsc_init_buf(struct hv_device *device,
>               goto cleanup;
>       }
> 
> +     if (hv_isolation_type_snp()) {
> +             vaddr = hv_map_memory(net_device->send_buf, buf_size);
> +             if (!vaddr) {
> +                     ret = -ENOMEM;
> +                     goto cleanup;
> +             }
> +
> +             net_device->send_original_buf = net_device->send_buf;
> +             net_device->send_buf = vaddr;
> +     }
> +
>       /* Notify the NetVsp of the gpadl handle */
>       init_packet = &net_device->channel_init_pkt;
>       memset(init_packet, 0, sizeof(struct nvsp_message));
> @@ -775,7 +811,7 @@ static void netvsc_send_tx_complete(struct net_device 
> *ndev,
> 
>       /* Notify the layer above us */
>       if (likely(skb)) {
> -             const struct hv_netvsc_packet *packet
> +             struct hv_netvsc_packet *packet
>                       = (struct hv_netvsc_packet *)skb->cb;
>               u32 send_index = packet->send_buf_index;
>               struct netvsc_stats *tx_stats;
> @@ -791,6 +827,7 @@ static void netvsc_send_tx_complete(struct net_device 
> *ndev,
>               tx_stats->bytes += packet->total_bytes;
>               u64_stats_update_end(&tx_stats->syncp);
> 
> +             netvsc_dma_unmap(ndev_ctx->device_ctx, packet);
>               napi_consume_skb(skb, budget);
>       }
> 
> @@ -955,6 +992,87 @@ static void netvsc_copy_to_send_buf(struct netvsc_device 
> *net_device,
>               memset(dest, 0, padding);
>  }
> 
> +void netvsc_dma_unmap(struct hv_device *hv_dev,
> +                   struct hv_netvsc_packet *packet)
> +{
> +     u32 page_count = packet->cp_partial ?
> +             packet->page_buf_cnt - packet->rmsg_pgcnt :
> +             packet->page_buf_cnt;
> +     int i;
> +
> +     if (!hv_is_isolation_supported())
> +             return;
> +
> +     if (!packet->dma_range)
> +             return;
> +
> +     for (i = 0; i < page_count; i++)
> +             dma_unmap_single(&hv_dev->device, packet->dma_range[i].dma,
> +                              packet->dma_range[i].mapping_size,
> +                              DMA_TO_DEVICE);
> +
> +     kfree(packet->dma_range);
> +}
> +
> +/* netvsc_dma_map - Map swiotlb bounce buffer with data page of
> + * packet sent by vmbus_sendpacket_pagebuffer() in the Isolation
> + * VM.
> + *
> + * In isolation VM, netvsc send buffer has been marked visible to
> + * host and so the data copied to send buffer doesn't need to use
> + * bounce buffer. The data pages handled by vmbus_sendpacket_pagebuffer()
> + * may not be copied to send buffer and so these pages need to be
> + * mapped with swiotlb bounce buffer. netvsc_dma_map() is to do
> + * that. The pfns in the struct hv_page_buffer need to be converted
> + * to bounce buffer's pfn. The loop here is necessary becuase the

s/becuase/because/

> + * entries in the page buffer array are not necessarily full
> + * pages of data.  Each entry in the array has a separate offset and
> + * len that may be non-zero, even for entries in the middle of the
> + * array.  And the entries are not physically contiguous.  So each
> + * entry must be individually mapped rather than as a contiguous unit.
> + * So not use dma_map_sg() here.
> + */
> +int netvsc_dma_map(struct hv_device *hv_dev,
> +                struct hv_netvsc_packet *packet,
> +                struct hv_page_buffer *pb)
> +{
> +     u32 page_count =  packet->cp_partial ?
> +             packet->page_buf_cnt - packet->rmsg_pgcnt :
> +             packet->page_buf_cnt;
> +     dma_addr_t dma;
> +     int i;
> +
> +     if (!hv_is_isolation_supported())
> +             return 0;
> +
> +     packet->dma_range = kcalloc(page_count,
> +                                 sizeof(*packet->dma_range),
> +                                 GFP_KERNEL);
> +     if (!packet->dma_range)
> +             return -ENOMEM;
> +
> +     for (i = 0; i < page_count; i++) {
> +             char *src = phys_to_virt((pb[i].pfn << HV_HYP_PAGE_SHIFT)
> +                                      + pb[i].offset);
> +             u32 len = pb[i].len;
> +
> +             dma = dma_map_single(&hv_dev->device, src, len,
> +                                  DMA_TO_DEVICE);
> +             if (dma_mapping_error(&hv_dev->device, dma)) {
> +                     kfree(packet->dma_range);
> +                     return -ENOMEM;
> +             }
> +
> +             packet->dma_range[i].dma = dma;
> +             packet->dma_range[i].mapping_size = len;
> +             pb[i].pfn = dma >> HV_HYP_PAGE_SHIFT;
> +             pb[i].offset = offset_in_hvpage(dma);
> +             pb[i].len = len;
> +     }

Just to confirm, this driver does *not* set the DMA min_align_mask
like storvsc does.  So after the call to dma_map_single(), the offset
in the page could be different.  That's why you are updating
the pb[i].offset value.  Alternatively, you could set the DMA
min_align_mask, which would ensure the offset is unchanged.
I'm OK with either approach, though perhaps a comment is
warranted to explain, as this is a subtle issue.

> +
> +     return 0;
> +}
> +
>  static inline int netvsc_send_pkt(
>       struct hv_device *device,
>       struct hv_netvsc_packet *packet,
> @@ -995,14 +1113,24 @@ static inline int netvsc_send_pkt(
> 
>       trace_nvsp_send_pkt(ndev, out_channel, rpkt);
> 
> +     packet->dma_range = NULL;
>       if (packet->page_buf_cnt) {
>               if (packet->cp_partial)
>                       pb += packet->rmsg_pgcnt;
> 
> +             ret = netvsc_dma_map(ndev_ctx->device_ctx, packet, pb);
> +             if (ret) {
> +                     ret = -EAGAIN;
> +                     goto exit;
> +             }
> +
>               ret = vmbus_sendpacket_pagebuffer(out_channel,
>                                                 pb, packet->page_buf_cnt,
>                                                 &nvmsg, sizeof(nvmsg),
>                                                 req_id);
> +
> +             if (ret)
> +                     netvsc_dma_unmap(ndev_ctx->device_ctx, packet);
>       } else {
>               ret = vmbus_sendpacket(out_channel,
>                                      &nvmsg, sizeof(nvmsg),
> @@ -1010,6 +1138,7 @@ static inline int netvsc_send_pkt(
>                                      
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
>       }
> 
> +exit:
>       if (ret == 0) {
>               atomic_inc_return(&nvchan->queue_sends);
> 
> diff --git a/drivers/net/hyperv/rndis_filter.c 
> b/drivers/net/hyperv/rndis_filter.c
> index f6c9c2a670f9..448fcc325ed7 100644
> --- a/drivers/net/hyperv/rndis_filter.c
> +++ b/drivers/net/hyperv/rndis_filter.c
> @@ -361,6 +361,8 @@ static void rndis_filter_receive_response(struct 
> net_device *ndev,
>                       }
>               }
> 
> +             netvsc_dma_unmap(((struct net_device_context *)
> +                     netdev_priv(ndev))->device_ctx, &request->pkt);
>               complete(&request->wait_event);
>       } else {
>               netdev_err(ndev,
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 724a735d722a..139a43ad65a1 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1596,6 +1596,11 @@ struct hyperv_service_callback {
>       void (*callback)(void *context);
>  };
> 
> +struct hv_dma_range {
> +     dma_addr_t dma;
> +     u32 mapping_size;
> +};
> +
>  #define MAX_SRV_VER  0x7ffffff
>  extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp, u8 *buf, 
> u32 buflen,
>                               const int *fw_version, int fw_vercnt,
> --
> 2.25.1




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.