Re: [RFC PATCH 1/12] x86/Hyper-V: Add visibility parameter for vmbus_establish_gpadl()

Thu, 04 Mar 2021 22:06:35 -0800 

Hi Vitaly:
     Thanks for your review.

On 3/4/2021 12:27 AM, Vitaly Kuznetsov wrote:

Tianyu Lan <ltyker...@gmail.com> writes:


From: Tianyu Lan <tianyu....@microsoft.com>

Add visibility parameter for vmbus_establish_gpadl() and prepare
to change host visibility when create gpadl for buffer.



"No functional change" as you don't actually use the parameter.


Yes, will add it into commit log.




Signed-off-by: Sunil Muthuswamy <sunil...@microsoft.com>
Co-Developed-by: Sunil Muthuswamy <sunil...@microsoft.com>
Signed-off-by: Tianyu Lan <tianyu....@microsoft.com>


Nit: Sunil's SoB looks misleading because the patch is from you,
Co-Developed-by should be sufficient.



Will update.


---
  arch/x86/include/asm/hyperv-tlfs.h |  9 +++++++++
  drivers/hv/channel.c               | 20 +++++++++++---------
  drivers/net/hyperv/netvsc.c        |  8 ++++++--
  drivers/uio/uio_hv_generic.c       |  7 +++++--
  include/linux/hyperv.h             |  3 ++-
  5 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/arch/x86/include/asm/hyperv-tlfs.h 
b/arch/x86/include/asm/hyperv-tlfs.h
index e6cd3fee562b..fb1893a4c32b 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -236,6 +236,15 @@ enum hv_isolation_type {
  /* TSC invariant control */
  #define HV_X64_MSR_TSC_INVARIANT_CONTROL      0x40000118
+/* Hyper-V GPA map flags */ 
+#define HV_MAP_GPA_PERMISSIONS_NONE            0x0
+#define HV_MAP_GPA_READABLE                    0x1
+#define HV_MAP_GPA_WRITABLE                    0x2
+
+#define VMBUS_PAGE_VISIBLE_READ_ONLY HV_MAP_GPA_READABLE
+#define VMBUS_PAGE_VISIBLE_READ_WRITE (HV_MAP_GPA_READABLE|HV_MAP_GPA_WRITABLE)
+#define VMBUS_PAGE_NOT_VISIBLE HV_MAP_GPA_PERMISSIONS_NONE
+


Are these x86-only? If not, then we should probably move these defines
to include/asm-generic/hyperv-tlfs.h. In case they are, we should do
something as we're using them from arch neutral places.

Also, could you please add a comment stating that these flags define
host's visibility of a page and not guest's (this seems to be not
obvious at least to me).







  /*
   * Declare the MSR used to setup pages used to communicate with the 
hypervisor.
   */
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 0bd202de7960..daa21cc72beb 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -242,7 +242,7 @@ EXPORT_SYMBOL_GPL(vmbus_send_modifychannel);
   */
  static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
                               u32 size, u32 send_offset,
-                              struct vmbus_channel_msginfo **msginfo)
+                              struct vmbus_channel_msginfo **msginfo, u32 
visibility)
  {
        int i;
        int pagecount;
@@ -391,7 +391,7 @@ static int create_gpadl_header(enum hv_gpadl_type type, 
void *kbuffer,
  static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
                                   enum hv_gpadl_type type, void *kbuffer,
                                   u32 size, u32 send_offset,
-                                  u32 *gpadl_handle)
+                                  u32 *gpadl_handle, u32 visibility)
  {
        struct vmbus_channel_gpadl_header *gpadlmsg;
        struct vmbus_channel_gpadl_body *gpadl_body;
@@ -405,7 +405,8 @@ static int __vmbus_establish_gpadl(struct vmbus_channel 
*channel,
        next_gpadl_handle =
                (atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1);
- ret = create_gpadl_header(type, kbuffer, size, send_offset, &msginfo); 
+       ret = create_gpadl_header(type, kbuffer, size, send_offset,
+                                 &msginfo, visibility);
        if (ret)
                return ret;
@@ -496,10 +497,10 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel, 
   * @gpadl_handle: some funky thing
   */
  int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
-                         u32 size, u32 *gpadl_handle)
+                         u32 size, u32 *gpadl_handle, u32 visibility)
  {
        return __vmbus_establish_gpadl(channel, HV_GPADL_BUFFER, kbuffer, size,
-                                      0U, gpadl_handle);
+                                      0U, gpadl_handle, visibility);
  }
  EXPORT_SYMBOL_GPL(vmbus_establish_gpadl);
@@ -610,10 +611,11 @@ static int __vmbus_open(struct vmbus_channel *newchannel, 
        newchannel->ringbuffer_gpadlhandle = 0;
err = __vmbus_establish_gpadl(newchannel, HV_GPADL_RING, 
-                                     page_address(newchannel->ringbuffer_page),
-                                     (send_pages + recv_pages) << PAGE_SHIFT,
-                                     newchannel->ringbuffer_send_offset << 
PAGE_SHIFT,
-                                     &newchannel->ringbuffer_gpadlhandle);
+                       page_address(newchannel->ringbuffer_page),
+                       (send_pages + recv_pages) << PAGE_SHIFT,
+                       newchannel->ringbuffer_send_offset << PAGE_SHIFT,
+                       &newchannel->ringbuffer_gpadlhandle,
+                       VMBUS_PAGE_VISIBLE_READ_WRITE);


Nit: I liked the original alignment more and we can avoid the unneeded
code churn.


        if (err)
                goto error_clean_ring;
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c 
index 2353623259f3..bb72c7578330 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -333,7 +333,8 @@ static int netvsc_init_buf(struct hv_device *device,
         */
        ret = vmbus_establish_gpadl(device->channel, net_device->recv_buf,
                                    buf_size,
-                                   &net_device->recv_buf_gpadl_handle);
+                                   &net_device->recv_buf_gpadl_handle,
+                                   VMBUS_PAGE_VISIBLE_READ_WRITE);
        if (ret != 0) {
                netdev_err(ndev,
                        "unable to establish receive buffer's gpadl\n");
@@ -422,10 +423,13 @@ static int netvsc_init_buf(struct hv_device *device,
        /* Establish the gpadl handle for this buffer on this
         * channel.  Note: This call uses the vmbus connection rather
         * than the channel to establish the gpadl handle.
+        * Send buffer should theoretically be only marked as "read-only", but
+        * the netvsp for some reason needs write capabilities on it.
         */
        ret = vmbus_establish_gpadl(device->channel, net_device->send_buf,
                                    buf_size,
-                                   &net_device->send_buf_gpadl_handle);
+                                   &net_device->send_buf_gpadl_handle,
+                                   VMBUS_PAGE_VISIBLE_READ_WRITE);
        if (ret != 0) {
                netdev_err(ndev,
                           "unable to establish send buffer's gpadl\n");
diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
index 0330ba99730e..813a7bee5139 100644
--- a/drivers/uio/uio_hv_generic.c
+++ b/drivers/uio/uio_hv_generic.c
@@ -29,6 +29,7 @@
  #include <linux/hyperv.h>
  #include <linux/vmalloc.h>
  #include <linux/slab.h>
+#include <asm/mshyperv.h>
#include "../hv/hyperv_vmbus.h" @@ -295,7 +296,8 @@ hv_uio_probe(struct hv_device *dev, 
        }
ret = vmbus_establish_gpadl(channel, pdata->recv_buf, 
-                                   RECV_BUFFER_SIZE, &pdata->recv_gpadl);
+                                   RECV_BUFFER_SIZE, &pdata->recv_gpadl,
+                                   VMBUS_PAGE_VISIBLE_READ_WRITE);
        if (ret)
                goto fail_close;
@@ -315,7 +317,8 @@ hv_uio_probe(struct hv_device *dev, 
        }
ret = vmbus_establish_gpadl(channel, pdata->send_buf, 
-                                   SEND_BUFFER_SIZE, &pdata->send_gpadl);
+                                   SEND_BUFFER_SIZE, &pdata->send_gpadl,
+                                   VMBUS_PAGE_VISIBLE_READ_ONLY);


Actually, this is the only place where you use 'READ_ONLY' mapping --
which makes me wonder if it's actually worth it or we can hard-code
VMBUS_PAGE_VISIBLE_READ_WRITE for now and avoid this additional
parameter.
Another option is to set host visibility out of vmbus_establish_gpadl(). There are three places calling vmbus_establish_gpadl(). Vmbus, netvsc and uio drivers. 


        if (ret)
                goto fail_close;
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h 
index f1d74dcf0353..016fdca20d6e 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1179,7 +1179,8 @@ extern int vmbus_sendpacket_mpb_desc(struct vmbus_channel 
*channel,
  extern int vmbus_establish_gpadl(struct vmbus_channel *channel,
                                      void *kbuffer,
                                      u32 size,
-                                     u32 *gpadl_handle);
+                                     u32 *gpadl_handle,
+                                     u32 visibility);
extern int vmbus_teardown_gpadl(struct vmbus_channel *channel, 
                                     u32 gpadl_handle);

Reply via email to