From: Vitaly Kuznetsov <vkuznets@redhat.com>
To: Tianyu Lan <ltykernel@gmail.com>
Cc: Tianyu Lan <Tianyu.Lan@microsoft.com>,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org, thomas.lendacky@amd.com,
brijesh.singh@amd.com, sunilmut@microsoft.com, kys@microsoft.com,
haiyangz@microsoft.com, sthemmin@microsoft.com,
wei.liu@kernel.org, tglx@linutronix.de, mingo@redhat.com,
bp@alien8.de, x86@kernel.org, hpa@zytor.com, davem@davemloft.net,
kuba@kernel.org, gregkh@linuxfoundation.org
Subject: Re: [RFC PATCH 1/12] x86/Hyper-V: Add visibility parameter for vmbus_establish_gpadl()
Date: Wed, 03 Mar 2021 17:27:38 +0100 [thread overview]
Message-ID: <87y2f4cidh.fsf@vitty.brq.redhat.com> (raw)
In-Reply-To: <20210228150315.2552437-2-ltykernel@gmail.com>
Tianyu Lan <ltykernel@gmail.com> writes:
> From: Tianyu Lan <Tianyu.Lan@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.
> Signed-off-by: Sunil Muthuswamy <sunilmut@microsoft.com>
> Co-Developed-by: Sunil Muthuswamy <sunilmut@microsoft.com>
> Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
Nit: Sunil's SoB looks misleading because the patch is from you,
Co-Developed-by should be sufficient.
> ---
> 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.
> 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);
--
Vitaly
next prev parent reply other threads:[~2021-03-04 0:30 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-28 15:03 [RFC PATCH 00/12] x86/Hyper-V: Add Hyper-V Isolation VM support Tianyu Lan
2021-02-28 15:03 ` [RFC PATCH 1/12] x86/Hyper-V: Add visibility parameter for vmbus_establish_gpadl() Tianyu Lan
2021-03-03 16:27 ` Vitaly Kuznetsov [this message]
2021-03-05 6:06 ` Tianyu Lan
2021-02-28 15:03 ` [RFC PATCH 2/12] x86/Hyper-V: Add new hvcall guest address host visibility support Tianyu Lan
2021-03-03 16:58 ` Vitaly Kuznetsov
2021-03-05 6:23 ` Tianyu Lan
2021-02-28 15:03 ` [RFC PATCH 3/12] x86/HV: Initialize GHCB page and shared memory boundary Tianyu Lan
2021-03-03 17:05 ` Vitaly Kuznetsov
2021-02-28 15:03 ` [RFC PATCH 4/12] HV: Add Write/Read MSR registers via ghcb Tianyu Lan
2021-03-03 17:16 ` Vitaly Kuznetsov
2021-03-05 6:37 ` Tianyu Lan
2021-02-28 15:03 ` [RFC PATCH 5/12] HV: Add ghcb hvcall support for SNP VM Tianyu Lan
2021-03-03 17:21 ` Vitaly Kuznetsov
2021-03-05 15:21 ` Tianyu Lan
2021-02-28 15:03 ` [RFC PATCH 6/12] HV/Vmbus: Add SNP support for VMbus channel initiate message Tianyu Lan
2021-02-28 15:03 ` [RFC PATCH 7/12] hv/vmbus: Initialize VMbus ring buffer for Isolation VM Tianyu Lan
2021-02-28 15:03 ` [RFC PATCH 8/12] x86/Hyper-V: Initialize bounce buffer page cache and list Tianyu Lan
2021-02-28 15:03 ` [RFC PATCH 9/12] x86/Hyper-V: Add new parameter for vmbus_sendpacket_pagebuffer()/mpb_desc() Tianyu Lan
2021-02-28 15:03 ` [RFC PATCH 10/12] HV: Add bounce buffer support for Isolation VM Tianyu Lan
2021-02-28 15:03 ` [RFC PATCH 11/12] HV/Netvsc: Add Isolation VM support for netvsc driver Tianyu Lan
2021-02-28 15:03 ` [RFC PATCH 12/12] HV/Storvsc: Add bounce buffer support for Storvsc Tianyu Lan
2021-03-01 6:54 ` Christoph Hellwig
2021-03-01 13:43 ` Tianyu Lan
2021-03-01 19:45 ` [EXTERNAL] " Sunil Muthuswamy
2021-03-02 16:03 ` Tianyu Lan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87y2f4cidh.fsf@vitty.brq.redhat.com \
--to=vkuznets@redhat.com \
--cc=Tianyu.Lan@microsoft.com \
--cc=bp@alien8.de \
--cc=brijesh.singh@amd.com \
--cc=davem@davemloft.net \
--cc=gregkh@linuxfoundation.org \
--cc=haiyangz@microsoft.com \
--cc=hpa@zytor.com \
--cc=kuba@kernel.org \
--cc=kys@microsoft.com \
--cc=linux-hyperv@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=ltykernel@gmail.com \
--cc=mingo@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=sthemmin@microsoft.com \
--cc=sunilmut@microsoft.com \
--cc=tglx@linutronix.de \
--cc=thomas.lendacky@amd.com \
--cc=wei.liu@kernel.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).