From: Tianyu Lan <ltykernel@gmail.com>
To: Vitaly Kuznetsov <vkuznets@redhat.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: Fri, 5 Mar 2021 14:06:18 +0800 [thread overview]
Message-ID: <e4be1a18-c882-50ef-3ac7-7838c9dfa5ba@gmail.com> (raw)
In-Reply-To: <87y2f4cidh.fsf@vitty.brq.redhat.com>
Hi Vitaly:
Thanks for your review.
On 3/4/2021 12:27 AM, Vitaly Kuznetsov wrote:
> 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.
Yes, will add it into commit log.
>
>> 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.
>
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);
>
next prev parent reply other threads:[~2021-03-05 6:06 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
2021-03-05 6:06 ` Tianyu Lan [this message]
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=e4be1a18-c882-50ef-3ac7-7838c9dfa5ba@gmail.com \
--to=ltykernel@gmail.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=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=vkuznets@redhat.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.