* Re: [PATCH v5 2/8] arm64: hyperv: Add hypercall and register access functions
From: Marc Zyngier @ 2019-11-06 10:19 UTC (permalink / raw)
To: Michael Kelley
Cc: will, catalin.marinas, mark.rutland, linux-arm-kernel, gregkh,
linux-kernel, linux-hyperv, devel, olaf, apw, vkuznets, jasowang,
marcelo.cerri, KY Srinivasan, Sunil Muthuswamy, boqun.feng
In-Reply-To: <1570129355-16005-3-git-send-email-mikelley@microsoft.com>
On 2019-10-03 20:12, Michael Kelley wrote:
> Add ARM64-specific code to make Hyper-V hypercalls and to
> access virtual processor synthetic registers via hypercalls.
> Hypercalls use a Hyper-V specific calling sequence with a non-zero
> immediate value per Section 2.9 of the SMC Calling Convention
> spec.
I find this "following the spec by actively sidestepping it" counter
productive. You (or rather the Hyper-V people) are reinventing the
wheel (of the slightly square variety) instead of using the standard
that the whole of the ARM ecosystem seems happy to take advantage
of.
I wonder what is the rational for this. If something doesn't quite
work for Hyper-V, I think we'd all like to know.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* RE:elektronicka faktura 71081403108
From: Ladislav Adamec @ 2019-11-07 7:34 UTC (permalink / raw)
To: linux-hyperv
[-- Attachment #1: Type: text/plain, Size: 75 bytes --]
__
Dobre odpoledne,
Vase fakturacniv priloze,
S pozdravem.
[-- Attachment #2: faktura_0003589.docx --]
[-- Type: application/octet-stream, Size: 46287 bytes --]
^ permalink raw reply
* RE: [PATCH v5 2/8] arm64: hyperv: Add hypercall and register access functions
From: Marc Zyngier @ 2019-11-07 9:10 UTC (permalink / raw)
To: Michael Kelley
Cc: will, catalin.marinas, mark.rutland, linux-arm-kernel, gregkh,
linux-kernel, linux-hyperv, devel, olaf, apw, vkuznets, jasowang,
marcelo.cerri, KY Srinivasan, Sunil Muthuswamy, boqun.feng
In-Reply-To: <DM5PR21MB013730D09CB8BA7658DE57F7D7790@DM5PR21MB0137.namprd21.prod.outlook.com>
On 2019-11-06 19:08, Michael Kelley wrote:
> From: Marc Zyngier <maz@kernel.org> Sent: Wednesday, November 6,
> 2019 2:20 AM
>>
>> On 2019-10-03 20:12, Michael Kelley wrote:
>> > Add ARM64-specific code to make Hyper-V hypercalls and to
>> > access virtual processor synthetic registers via hypercalls.
>> > Hypercalls use a Hyper-V specific calling sequence with a non-zero
>> > immediate value per Section 2.9 of the SMC Calling Convention
>> > spec.
>>
>> I find this "following the spec by actively sidestepping it" counter
>> productive. You (or rather the Hyper-V people) are reinventing the
>> wheel (of the slightly square variety) instead of using the standard
>> that the whole of the ARM ecosystem seems happy to take advantage
>> of.
>>
>> I wonder what is the rational for this. If something doesn't quite
>> work for Hyper-V, I think we'd all like to know.
>>
>
> I'll go another round internally with the Hyper-V people on this
> topic and impress upon them the desire of the Linux community to
> have Hyper-V adopt the true spirit of the spec. But I know they are
> fairly set in their approach at this point, regardless of the
> technical
> merits or lack thereof. Hyper-V is shipping and in use as a
> commercial
> product on ARM64 hardware, which makes it harder to change. I
> hope we can find a way to avoid a complete impasse ....
Hyper-V shipping with their own calling convention is fine by me. Linux
having to implement multiple calling conventions because the Hyper-V
folks refuse (for undisclosed reason) to adopt the standard isn't fine
at
all.
HV can perfectly retain its interface for Windows or other things, but
please *at least* implement the standard interface on which all
existing
operating systems rely.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* Re: [PATCH v3] x86/hyper-v: micro-optimize send_ipi_one case
From: Vitaly Kuznetsov @ 2019-11-07 13:26 UTC (permalink / raw)
To: Sasha Levin
Cc: linux-hyperv, linux-kernel, x86, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, Roman Kagan, Michael Kelley, Joe Perches
In-Reply-To: <20191027151938.7296-1-vkuznets@redhat.com>
Vitaly Kuznetsov <vkuznets@redhat.com> writes:
> When sending an IPI to a single CPU there is no need to deal with cpumasks.
> With 2 CPU guest on WS2019 I'm seeing a minor (like 3%, 8043 -> 7761 CPU
> cycles) improvement with smp_call_function_single() loop benchmark. The
> optimization, however, is tiny and straitforward. Also, send_ipi_one() is
> important for PV spinlock kick.
>
> I was also wondering if it would make sense to switch to using regular
> APIC IPI send for CPU > 64 case but no, it is twice as expesive (12650 CPU
> cycles for __send_ipi_mask_ex() call, 26000 for orig_apic.send_IPI(cpu,
> vector)).
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> Changes since v2:
> - Check VP number instead of CPU number against >= 64 [Michael]
> - Check for VP_INVAL
Hi Sasha,
do you have plans to pick this up for hyperv-next or should we ask x86
folks to?
Thanks!
--
Vitaly
^ permalink raw reply
* Re: [PATCH v1] tools/hv: async name resolution in kvp_daemon
From: Vitaly Kuznetsov @ 2019-11-07 13:39 UTC (permalink / raw)
To: Olaf Hering, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Sasha Levin, open list:Hyper-V CORE AND DRIVERS, open list
Cc: Olaf Hering
In-Reply-To: <20191024144943.26199-1-olaf@aepfle.de>
Olaf Hering <olaf@aepfle.de> writes:
> The hostname is resolved just once since commit 58125210ab3b ("Tools:
> hv: cache FQDN in kvp_daemon to avoid timeouts") to make sure the VM
> responds within the timeout limits to requests from the host.
>
> If for some reason getaddrinfo fails, the string returned by the
> "FullyQualifiedDomainName" request contains some error string, which is
> then used by tools on the host side.
>
> Adjust the code to resolve the current hostname in a separate thread.
> This thread loops until getaddrinfo returns success. During this time
> all "FullyQualifiedDomainName" requests will be answered with an empty
> string.
>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
> tools/hv/Makefile | 2 ++
> tools/hv/hv_kvp_daemon.c | 69 ++++++++++++++++++++++++++++++++----------------
> 2 files changed, 48 insertions(+), 23 deletions(-)
>
> diff --git a/tools/hv/Makefile b/tools/hv/Makefile
> index b57143d9459c..3b5481015a84 100644
> --- a/tools/hv/Makefile
> +++ b/tools/hv/Makefile
> @@ -22,6 +22,8 @@ ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
>
> ALL_SCRIPTS := hv_get_dhcp_info.sh hv_get_dns_info.sh hv_set_ifconfig.sh
>
> +$(OUTPUT)hv_kvp_daemon: LDFLAGS += -lpthread
> +
> all: $(ALL_PROGRAMS)
>
> export srctree OUTPUT CC LD CFLAGS
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index e9ef4ca6a655..22cf1c4dbf5c 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -41,6 +41,7 @@
> #include <net/if.h>
> #include <limits.h>
> #include <getopt.h>
> +#include <pthread.h>
>
> /*
> * KVP protocol: The user mode component first registers with the
> @@ -85,7 +86,7 @@ static char *processor_arch;
> static char *os_build;
> static char *os_version;
> static char *lic_version = "Unknown version";
> -static char full_domain_name[HV_KVP_EXCHANGE_MAX_VALUE_SIZE];
> +static char *full_domain_name;
> static struct utsname uts_buf;
>
> /*
> @@ -1327,27 +1328,53 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> return error;
> }
>
> -
> -static void
> -kvp_get_domain_name(char *buffer, int length)
> +/*
> + * Async retrival of Fully Qualified Domain Name because getaddrinfo takes an
> + * unpredictable amount of time to finish.
> + */
> +static void *kvp_getaddrinfo(void *p)
> {
> - struct addrinfo hints, *info ;
> - int error = 0;
> + char *tmp, **str_ptr = (char **)p;
> + char hostname[HOST_NAME_MAX + 1];
> + struct addrinfo *info, hints = {
> + .ai_family = AF_INET, /* Get only ipv4 addrinfo. */
> + .ai_socktype = SOCK_STREAM,
> + .ai_flags = AI_CANONNAME,
> + };
> + int ret;
> +
> + if (gethostname(hostname, sizeof(hostname) - 1) < 0)
> + goto out;
> +
> + do {
> + ret = getaddrinfo(hostname, NULL, &hints, &info);
> + if (ret)
> + sleep(1);
Is it only EAI_AGAIN or do you see any other return values which justify
the retry? I'm afraid that in case of a e.g. non-existing hostname we'll
be infinitely looping with EAI_FAIL.
> + } while (ret);
> +
> + ret = asprintf(&tmp, "%s", info->ai_canonname);
> + freeaddrinfo(info);
> + if (ret <= 0)
> + goto out;
> +
> + if (ret > HV_KVP_EXCHANGE_MAX_VALUE_SIZE)
> + tmp[HV_KVP_EXCHANGE_MAX_VALUE_SIZE - 1] = '\0';
> + *str_ptr = tmp;
>
> - gethostname(buffer, length);
> - memset(&hints, 0, sizeof(hints));
> - hints.ai_family = AF_INET; /*Get only ipv4 addrinfo. */
> - hints.ai_socktype = SOCK_STREAM;
> - hints.ai_flags = AI_CANONNAME;
> +out:
> + pthread_exit(NULL);
> +}
> +
> +static void kvp_obtain_domain_name(char **str_ptr)
> +{
> + pthread_t t;
>
> - error = getaddrinfo(buffer, NULL, &hints, &info);
> - if (error != 0) {
> - snprintf(buffer, length, "getaddrinfo failed: 0x%x %s",
> - error, gai_strerror(error));
> + if (pthread_create(&t, NULL, kvp_getaddrinfo, str_ptr)) {
> + syslog(LOG_ERR, "pthread_create failed; error: %d %s",
> + errno, strerror(errno));
> return;
> }
> - snprintf(buffer, length, "%s", info->ai_canonname);
> - freeaddrinfo(info);
> + pthread_detach(t);
I think this should be complemented with pthread_cancel/pthread_join
before exiting main().
> }
>
> void print_usage(char *argv[])
> @@ -1412,11 +1439,7 @@ int main(int argc, char *argv[])
> * Retrieve OS release information.
> */
> kvp_get_os_info();
> - /*
> - * Cache Fully Qualified Domain Name because getaddrinfo takes an
> - * unpredictable amount of time to finish.
> - */
> - kvp_get_domain_name(full_domain_name, sizeof(full_domain_name));
> + kvp_obtain_domain_name(&full_domain_name);
>
> if (kvp_file_init()) {
> syslog(LOG_ERR, "Failed to initialize the pools");
> @@ -1571,7 +1594,7 @@ int main(int argc, char *argv[])
>
> switch (hv_msg->body.kvp_enum_data.index) {
> case FullyQualifiedDomainName:
> - strcpy(key_value, full_domain_name);
> + strcpy(key_value, full_domain_name ? : "");
> strcpy(key_name, "FullyQualifiedDomainName");
> break;
> case IntegrationServicesVersion:
--
Vitaly
^ permalink raw reply
* Re: [PATCH v1] tools/hv: async name resolution in kvp_daemon
From: Olaf Hering @ 2019-11-07 13:48 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
open list:Hyper-V CORE AND DRIVERS, open list
In-Reply-To: <874kzfbybk.fsf@vitty.brq.redhat.com>
[-- Attachment #1: Type: text/plain, Size: 946 bytes --]
Am Thu, 07 Nov 2019 14:39:11 +0100
schrieb Vitaly Kuznetsov <vkuznets@redhat.com>:
> Olaf Hering <olaf@aepfle.de> writes:
> Is it only EAI_AGAIN or do you see any other return values which justify
> the retry? I'm afraid that in case of a e.g. non-existing hostname we'll
> be infinitely looping with EAI_FAIL.
I currently do not have a setup that reproduces the failure.
I think if this thread loops forever, so be it.
The report I have shows "getaddrinfo failed: 0xfffffffe Name or service not known" on the host side.
And that went away within the VM once "networking was fixed", whatever this means.
But hv_kvp_daemon would report the error string forever.
> > + pthread_detach(t);
> I think this should be complemented with pthread_cancel/pthread_join
> before exiting main().
If the thread is detached, it is exactly that: detached. Why do you think the main thread should wait for the detached thread?
Olaf
[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v1] tools/hv: async name resolution in kvp_daemon
From: Vitaly Kuznetsov @ 2019-11-07 14:15 UTC (permalink / raw)
To: Olaf Hering
Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
open list:Hyper-V CORE AND DRIVERS, open list
In-Reply-To: <20191107144850.37587edb.olaf@aepfle.de>
Olaf Hering <olaf@aepfle.de> writes:
> Am Thu, 07 Nov 2019 14:39:11 +0100
> schrieb Vitaly Kuznetsov <vkuznets@redhat.com>:
>
>> Olaf Hering <olaf@aepfle.de> writes:
>
>> Is it only EAI_AGAIN or do you see any other return values which justify
>> the retry? I'm afraid that in case of a e.g. non-existing hostname we'll
>> be infinitely looping with EAI_FAIL.
>
> I currently do not have a setup that reproduces the failure.
> I think if this thread loops forever, so be it.
>
> The report I have shows "getaddrinfo failed: 0xfffffffe Name or service not known" on the host side.
> And that went away within the VM once "networking was fixed", whatever this means.
> But hv_kvp_daemon would report the error string forever.
Looping forever with a permanent error is pretty unusual...
>
>> > + pthread_detach(t);
>> I think this should be complemented with pthread_cancel/pthread_join
>> before exiting main().
>
> If the thread is detached, it is exactly that: detached. Why do you think the main thread should wait for the detached thread?
Ah, my bad: you actually can't join a detached thread, scratch my
comment.
--
Vitaly
^ permalink raw reply
* Re: [PATCH v5 3/8] arm64: hyperv: Add memory alloc/free functions for Hyper-V size pages
From: Marc Zyngier @ 2019-11-07 14:19 UTC (permalink / raw)
To: Michael Kelley
Cc: will, catalin.marinas, mark.rutland, linux-arm-kernel, gregkh,
linux-kernel, linux-hyperv, devel, olaf, apw, vkuznets, jasowang,
marcelo.cerri, KY Srinivasan, Sunil Muthuswamy, boqun.feng
In-Reply-To: <1570129355-16005-4-git-send-email-mikelley@microsoft.com>
On 2019-10-03 20:12, Michael Kelley wrote:
> Add ARM64-specific code to allocate memory with HV_HYP_PAGE_SIZE
> size and alignment. These are for use when pages need to be shared
> with Hyper-V. Separate functions are needed as the page size used
> by Hyper-V may not be the same as the guest page size. Free
> operations are rarely done, so no attempt is made to combine
> freed pages into larger chunks.
>
> This code is built only when CONFIG_HYPERV is enabled.
>
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> ---
> arch/arm64/hyperv/hv_init.c | 68
> ++++++++++++++++++++++++++++++++++++++++++
> include/asm-generic/mshyperv.h | 5 ++++
> 2 files changed, 73 insertions(+)
>
> diff --git a/arch/arm64/hyperv/hv_init.c
> b/arch/arm64/hyperv/hv_init.c
> index 6808bc8..9c294f6 100644
> --- a/arch/arm64/hyperv/hv_init.c
> +++ b/arch/arm64/hyperv/hv_init.c
> @@ -15,10 +15,78 @@
> #include <linux/export.h>
> #include <linux/mm.h>
> #include <linux/hyperv.h>
> +#include <linux/spinlock.h>
> +#include <linux/list.h>
> +#include <linux/string.h>
> #include <asm-generic/bug.h>
> #include <asm/hyperv-tlfs.h>
> #include <asm/mshyperv.h>
>
> +
> +/*
> + * Functions for allocating and freeing memory with size and
> + * alignment HV_HYP_PAGE_SIZE. These functions are needed because
> + * the guest page size may not be the same as the Hyper-V page
> + * size. And while kalloc() could allocate the memory, it does not
> + * guarantee the required alignment. So a separate small memory
> + * allocator is needed. The free function is rarely used, so it
> + * does not try to combine freed pages into larger chunks.
Is this still needed now that kmalloc has alignment guarantees
(see 59bb47985c1d)?
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* Re: [PATCH v1] tools/hv: async name resolution in kvp_daemon
From: Olaf Hering @ 2019-11-07 14:20 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
open list:Hyper-V CORE AND DRIVERS, open list
In-Reply-To: <87zhh7ai26.fsf@vitty.brq.redhat.com>
[-- Attachment #1: Type: text/plain, Size: 314 bytes --]
Am Thu, 07 Nov 2019 15:15:45 +0100
schrieb Vitaly Kuznetsov <vkuznets@redhat.com>:
> Looping forever with a permanent error is pretty unusual...
That might be true, but how would you detect a permanent error?
Just because the DNS server is gone for one hour does not mean it will be gone forever.
Olaf
[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v1] tools/hv: async name resolution in kvp_daemon
From: Vitaly Kuznetsov @ 2019-11-07 14:44 UTC (permalink / raw)
To: Olaf Hering
Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
open list:Hyper-V CORE AND DRIVERS, open list
In-Reply-To: <20191107152059.6cae8f30.olaf@aepfle.de>
Olaf Hering <olaf@aepfle.de> writes:
> Am Thu, 07 Nov 2019 15:15:45 +0100
> schrieb Vitaly Kuznetsov <vkuznets@redhat.com>:
>
>> Looping forever with a permanent error is pretty unusual...
>
> That might be true, but how would you detect a permanent error?
> Just because the DNS server is gone for one hour does not mean it will be gone forever.
'man 3 getaddrinfo' lists the following:
EAI_ADDRFAMILY
EAI_AGAIN
EAI_BADFLAGS
EAI_FAIL
EAI_FAMILY
EAI_MEMORY
EAI_NODATA
EAI_NONAME
EAI_SERVICE
EAI_SOCKTYPE
EAI_SYSTEM
I *think* what you're aiming at is EAI_AGAIN and EAI_FAIL, the rest
should probably terminate the resolver thread (e.g. AF_INET is
unsupported or something like that).
--
Vitaly
^ permalink raw reply
* Re: [PATCH v3] x86/hyper-v: micro-optimize send_ipi_one case
From: Thomas Gleixner @ 2019-11-07 21:21 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Sasha Levin, linux-hyperv, linux-kernel, x86, K. Y. Srinivasan,
Haiyang Zhang, Stephen Hemminger, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, Roman Kagan, Michael Kelley, Joe Perches
In-Reply-To: <877e4bbyw2.fsf@vitty.brq.redhat.com>
On Thu, 7 Nov 2019, Vitaly Kuznetsov wrote:
> Vitaly Kuznetsov <vkuznets@redhat.com> writes:
>
> > When sending an IPI to a single CPU there is no need to deal with cpumasks.
> > With 2 CPU guest on WS2019 I'm seeing a minor (like 3%, 8043 -> 7761 CPU
> > cycles) improvement with smp_call_function_single() loop benchmark. The
> > optimization, however, is tiny and straitforward. Also, send_ipi_one() is
> > important for PV spinlock kick.
> >
> > I was also wondering if it would make sense to switch to using regular
> > APIC IPI send for CPU > 64 case but no, it is twice as expesive (12650 CPU
> > cycles for __send_ipi_mask_ex() call, 26000 for orig_apic.send_IPI(cpu,
> > vector)).
> >
> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > ---
> > Changes since v2:
> > - Check VP number instead of CPU number against >= 64 [Michael]
> > - Check for VP_INVAL
>
> Hi Sasha,
>
> do you have plans to pick this up for hyperv-next or should we ask x86
> folks to?
I'm picking up the constant TSC one anyway, so I can just throw that in as
well.
Thanks,
tglx
^ permalink raw reply
* [PATCH AUTOSEL 4.14 077/103] Drivers: hv: vmbus: Fix synic per-cpu context initialization
From: Sasha Levin @ 2019-11-08 11:42 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Michael Kelley, Dan Carpenter, K . Y . Srinivasan,
Greg Kroah-Hartman, Sasha Levin, linux-hyperv
In-Reply-To: <20191108114310.14363-1-sashal@kernel.org>
From: Michael Kelley <mikelley@microsoft.com>
[ Upstream commit f25a7ece08bdb1f2b3c4bbeae942682fc3a99dde ]
If hv_synic_alloc() errors out, the state of the per-cpu context
for some CPUs is unknown since the zero'ing is done as each
CPU is iterated over. In such case, hv_synic_cleanup() may try to
free memory based on uninitialized values. Fix this by zero'ing
the per-cpu context for all CPUs before doing any memory
allocations that might fail.
Signed-off-by: Michael Kelley <mikelley@microsoft.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hv/hv.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index fe041f22521da..23f312b4c6aa2 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -148,6 +148,17 @@ static void hv_init_clockevent_device(struct clock_event_device *dev, int cpu)
int hv_synic_alloc(void)
{
int cpu;
+ struct hv_per_cpu_context *hv_cpu;
+
+ /*
+ * First, zero all per-cpu memory areas so hv_synic_free() can
+ * detect what memory has been allocated and cleanup properly
+ * after any failures.
+ */
+ for_each_present_cpu(cpu) {
+ hv_cpu = per_cpu_ptr(hv_context.cpu_context, cpu);
+ memset(hv_cpu, 0, sizeof(*hv_cpu));
+ }
hv_context.hv_numa_map = kzalloc(sizeof(struct cpumask) * nr_node_ids,
GFP_ATOMIC);
@@ -157,10 +168,8 @@ int hv_synic_alloc(void)
}
for_each_present_cpu(cpu) {
- struct hv_per_cpu_context *hv_cpu
- = per_cpu_ptr(hv_context.cpu_context, cpu);
+ hv_cpu = per_cpu_ptr(hv_context.cpu_context, cpu);
- memset(hv_cpu, 0, sizeof(*hv_cpu));
tasklet_init(&hv_cpu->msg_dpc,
vmbus_on_msg_dpc, (unsigned long) hv_cpu);
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 156/205] Drivers: hv: vmbus: Fix synic per-cpu context initialization
From: Sasha Levin @ 2019-11-08 11:37 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Michael Kelley, Dan Carpenter, K . Y . Srinivasan,
Greg Kroah-Hartman, Sasha Levin, linux-hyperv
In-Reply-To: <20191108113752.12502-1-sashal@kernel.org>
From: Michael Kelley <mikelley@microsoft.com>
[ Upstream commit f25a7ece08bdb1f2b3c4bbeae942682fc3a99dde ]
If hv_synic_alloc() errors out, the state of the per-cpu context
for some CPUs is unknown since the zero'ing is done as each
CPU is iterated over. In such case, hv_synic_cleanup() may try to
free memory based on uninitialized values. Fix this by zero'ing
the per-cpu context for all CPUs before doing any memory
allocations that might fail.
Signed-off-by: Michael Kelley <mikelley@microsoft.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hv/hv.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 8e923e70e5945..12bc9fa211117 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -189,6 +189,17 @@ static void hv_init_clockevent_device(struct clock_event_device *dev, int cpu)
int hv_synic_alloc(void)
{
int cpu;
+ struct hv_per_cpu_context *hv_cpu;
+
+ /*
+ * First, zero all per-cpu memory areas so hv_synic_free() can
+ * detect what memory has been allocated and cleanup properly
+ * after any failures.
+ */
+ for_each_present_cpu(cpu) {
+ hv_cpu = per_cpu_ptr(hv_context.cpu_context, cpu);
+ memset(hv_cpu, 0, sizeof(*hv_cpu));
+ }
hv_context.hv_numa_map = kcalloc(nr_node_ids, sizeof(struct cpumask),
GFP_KERNEL);
@@ -198,10 +209,8 @@ int hv_synic_alloc(void)
}
for_each_present_cpu(cpu) {
- struct hv_per_cpu_context *hv_cpu
- = per_cpu_ptr(hv_context.cpu_context, cpu);
+ hv_cpu = per_cpu_ptr(hv_context.cpu_context, cpu);
- memset(hv_cpu, 0, sizeof(*hv_cpu));
tasklet_init(&hv_cpu->msg_dpc,
vmbus_on_msg_dpc, (unsigned long) hv_cpu);
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 156/191] x86/hyperv: Suppress "PCI: Fatal: No config space access function found"
From: Sasha Levin @ 2019-11-10 2:39 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Dexuan Cui, Thomas Gleixner, Michael Kelley, H. Peter Anvin,
KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
devel@linuxdriverproject.org, Olaf Aepfle, Andy Whitcroft,
Jason Wang, Vitaly Kuznetsov, Marcelo Cerri, Josh Poulson,
Sasha Levin, linux-hyperv
In-Reply-To: <20191110024013.29782-1-sashal@kernel.org>
From: Dexuan Cui <decui@microsoft.com>
[ Upstream commit 2f285f46240d67060061d153786740d4df53cd78 ]
A Generation-2 Linux VM on Hyper-V doesn't have the legacy PCI bus, and
users always see the scary warning, which is actually harmless.
Suppress it.
Signed-off-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: KY Srinivasan <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <sthemmin@microsoft.com>
Cc: "devel@linuxdriverproject.org" <devel@linuxdriverproject.org>
Cc: Olaf Aepfle <olaf@aepfle.de>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Marcelo Cerri <marcelo.cerri@canonical.com>
Cc: Josh Poulson <jopoulso@microsoft.com>
Link: https://lkml.kernel.org/r/ <KU1P153MB0166D977DC930996C4BF538ABF1D0@KU1P153MB0166.APCP153.PROD.OUTLOOK.COM
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
arch/x86/hyperv/hv_init.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 3fb8551552862..8a9cff1f129dc 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -17,6 +17,7 @@
*
*/
+#include <linux/efi.h>
#include <linux/types.h>
#include <asm/apic.h>
#include <asm/desc.h>
@@ -257,6 +258,22 @@ static int hv_cpu_die(unsigned int cpu)
return 0;
}
+static int __init hv_pci_init(void)
+{
+ int gen2vm = efi_enabled(EFI_BOOT);
+
+ /*
+ * For Generation-2 VM, we exit from pci_arch_init() by returning 0.
+ * The purpose is to suppress the harmless warning:
+ * "PCI: Fatal: No config space access function found"
+ */
+ if (gen2vm)
+ return 0;
+
+ /* For Generation-1 VM, we'll proceed in pci_arch_init(). */
+ return 1;
+}
+
/*
* This function is to be invoked early in the boot sequence after the
* hypervisor has been detected.
@@ -333,6 +350,8 @@ void __init hyperv_init(void)
hv_apic_init();
+ x86_init.pci.arch_init = hv_pci_init;
+
/*
* Register Hyper-V specific clocksource.
*/
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.14 085/109] x86/hyperv: Suppress "PCI: Fatal: No config space access function found"
From: Sasha Levin @ 2019-11-10 2:45 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Dexuan Cui, Thomas Gleixner, Michael Kelley, H. Peter Anvin,
KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
devel@linuxdriverproject.org, Olaf Aepfle, Andy Whitcroft,
Jason Wang, Vitaly Kuznetsov, Marcelo Cerri, Josh Poulson,
Sasha Levin, linux-hyperv
In-Reply-To: <20191110024541.31567-1-sashal@kernel.org>
From: Dexuan Cui <decui@microsoft.com>
[ Upstream commit 2f285f46240d67060061d153786740d4df53cd78 ]
A Generation-2 Linux VM on Hyper-V doesn't have the legacy PCI bus, and
users always see the scary warning, which is actually harmless.
Suppress it.
Signed-off-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: KY Srinivasan <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <sthemmin@microsoft.com>
Cc: "devel@linuxdriverproject.org" <devel@linuxdriverproject.org>
Cc: Olaf Aepfle <olaf@aepfle.de>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Marcelo Cerri <marcelo.cerri@canonical.com>
Cc: Josh Poulson <jopoulso@microsoft.com>
Link: https://lkml.kernel.org/r/ <KU1P153MB0166D977DC930996C4BF538ABF1D0@KU1P153MB0166.APCP153.PROD.OUTLOOK.COM
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
arch/x86/hyperv/hv_init.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 2e9d58cc371e6..2653b7b25d176 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -17,6 +17,7 @@
*
*/
+#include <linux/efi.h>
#include <linux/types.h>
#include <asm/hypervisor.h>
#include <asm/hyperv.h>
@@ -101,6 +102,22 @@ static int hv_cpu_init(unsigned int cpu)
return 0;
}
+static int __init hv_pci_init(void)
+{
+ int gen2vm = efi_enabled(EFI_BOOT);
+
+ /*
+ * For Generation-2 VM, we exit from pci_arch_init() by returning 0.
+ * The purpose is to suppress the harmless warning:
+ * "PCI: Fatal: No config space access function found"
+ */
+ if (gen2vm)
+ return 0;
+
+ /* For Generation-1 VM, we'll proceed in pci_arch_init(). */
+ return 1;
+}
+
/*
* This function is to be invoked early in the boot sequence after the
* hypervisor has been detected.
@@ -154,6 +171,8 @@ void hyperv_init(void)
hyper_alloc_mmu();
+ x86_init.pci.arch_init = hv_pci_init;
+
/*
* Register Hyper-V specific clocksource.
*/
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 123/191] vmbus: keep pointer to ring buffer page
From: Sasha Levin @ 2019-11-10 2:39 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Stephen Hemminger, Stephen Hemminger, Greg Kroah-Hartman,
Sasha Levin, linux-hyperv
In-Reply-To: <20191110024013.29782-1-sashal@kernel.org>
From: Stephen Hemminger <stephen@networkplumber.org>
[ Upstream commit 52a42c2a90226dc61c99bbd0cb096deeb52c334b ]
Avoid going from struct page to virt address (and back) by just
keeping pointer to the allocated pages instead of virt address.
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hv/channel.c | 20 +++++++++-----------
drivers/uio/uio_hv_generic.c | 5 +++--
include/linux/hyperv.h | 2 +-
3 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index fdb0f832fadef..5e515533e9cdb 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -91,11 +91,14 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
unsigned long flags;
int ret, err = 0;
struct page *page;
+ unsigned int order;
if (send_ringbuffer_size % PAGE_SIZE ||
recv_ringbuffer_size % PAGE_SIZE)
return -EINVAL;
+ order = get_order(send_ringbuffer_size + recv_ringbuffer_size);
+
spin_lock_irqsave(&newchannel->lock, flags);
if (newchannel->state == CHANNEL_OPEN_STATE) {
newchannel->state = CHANNEL_OPENING_STATE;
@@ -110,21 +113,17 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
/* Allocate the ring buffer */
page = alloc_pages_node(cpu_to_node(newchannel->target_cpu),
- GFP_KERNEL|__GFP_ZERO,
- get_order(send_ringbuffer_size +
- recv_ringbuffer_size));
+ GFP_KERNEL|__GFP_ZERO, order);
if (!page)
- page = alloc_pages(GFP_KERNEL|__GFP_ZERO,
- get_order(send_ringbuffer_size +
- recv_ringbuffer_size));
+ page = alloc_pages(GFP_KERNEL|__GFP_ZERO, order);
if (!page) {
err = -ENOMEM;
goto error_set_chnstate;
}
- newchannel->ringbuffer_pages = page_address(page);
+ newchannel->ringbuffer_page = page;
newchannel->ringbuffer_pagecount = (send_ringbuffer_size +
recv_ringbuffer_size) >> PAGE_SHIFT;
@@ -239,8 +238,7 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
error_free_pages:
hv_ringbuffer_cleanup(&newchannel->outbound);
hv_ringbuffer_cleanup(&newchannel->inbound);
- __free_pages(page,
- get_order(send_ringbuffer_size + recv_ringbuffer_size));
+ __free_pages(page, order);
error_set_chnstate:
newchannel->state = CHANNEL_OPEN_STATE;
return err;
@@ -666,8 +664,8 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
hv_ringbuffer_cleanup(&channel->outbound);
hv_ringbuffer_cleanup(&channel->inbound);
- free_pages((unsigned long)channel->ringbuffer_pages,
- get_order(channel->ringbuffer_pagecount * PAGE_SIZE));
+ __free_pages(channel->ringbuffer_page,
+ get_order(channel->ringbuffer_pagecount << PAGE_SHIFT));
out:
return ret;
diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
index e401be8321ab5..170fa1f8f00e0 100644
--- a/drivers/uio/uio_hv_generic.c
+++ b/drivers/uio/uio_hv_generic.c
@@ -131,11 +131,12 @@ static int hv_uio_ring_mmap(struct file *filp, struct kobject *kobj,
= container_of(kobj, struct vmbus_channel, kobj);
struct hv_device *dev = channel->primary_channel->device_obj;
u16 q_idx = channel->offermsg.offer.sub_channel_index;
+ void *ring_buffer = page_address(channel->ringbuffer_page);
dev_dbg(&dev->device, "mmap channel %u pages %#lx at %#lx\n",
q_idx, vma_pages(vma), vma->vm_pgoff);
- return vm_iomap_memory(vma, virt_to_phys(channel->ringbuffer_pages),
+ return vm_iomap_memory(vma, virt_to_phys(ring_buffer),
channel->ringbuffer_pagecount << PAGE_SHIFT);
}
@@ -224,7 +225,7 @@ hv_uio_probe(struct hv_device *dev,
/* mem resources */
pdata->info.mem[TXRX_RING_MAP].name = "txrx_rings";
pdata->info.mem[TXRX_RING_MAP].addr
- = (uintptr_t)dev->channel->ringbuffer_pages;
+ = (uintptr_t)page_address(dev->channel->ringbuffer_page);
pdata->info.mem[TXRX_RING_MAP].size
= dev->channel->ringbuffer_pagecount << PAGE_SHIFT;
pdata->info.mem[TXRX_RING_MAP].memtype = UIO_MEM_LOGICAL;
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index bbde887ed3931..c43e694fef7dd 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -739,7 +739,7 @@ struct vmbus_channel {
u32 ringbuffer_gpadlhandle;
/* Allocated memory for ring buffer */
- void *ringbuffer_pages;
+ struct page *ringbuffer_page;
u32 ringbuffer_pagecount;
struct hv_ring_buffer_info outbound; /* send to parent */
struct hv_ring_buffer_info inbound; /* receive from parent */
--
2.20.1
^ permalink raw reply related
* [PATCH] VFIO/VMBUS: Add VFIO VMBUS driver support
From: lantianyu1986 @ 2019-11-11 8:45 UTC (permalink / raw)
To: alex.williamson, cohuck, kys, haiyangz, sthemmin, sashal,
mchehab+samsung, davem, gregkh, robh, Jonathan.Cameron, paulmck,
michael.h.kelley
Cc: Tianyu Lan, linux-kernel, kvm, linux-hyperv, vkuznets
From: Tianyu Lan <Tianyu.Lan@microsoft.com>
This patch is to add VFIO VMBUS driver support in order to expose
VMBUS devices to user space drivers(Reference Hyper-V UIO driver).
DPDK now has netvsc PMD driver support and it may get VMBUS resources
via VFIO interface with new driver support.
So far, Hyper-V doesn't provide virtual IOMMU support and so this
driver needs to be used with VFIO noiommu mode.
Current netvsc PMD driver in DPDK doesn't have IRQ mode support.
This driver version skips IRQ control part and adds later when
there is a real request.
Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
---
MAINTAINERS | 1 +
drivers/vfio/Kconfig | 1 +
drivers/vfio/Makefile | 1 +
drivers/vfio/vmbus/Kconfig | 9 +
drivers/vfio/vmbus/vfio_vmbus.c | 407 ++++++++++++++++++++++++++++++++++++++++
include/uapi/linux/vfio.h | 12 ++
6 files changed, 431 insertions(+)
create mode 100644 drivers/vfio/vmbus/Kconfig
create mode 100644 drivers/vfio/vmbus/vfio_vmbus.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 55199ef7fa74..6f61fb351a5d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7574,6 +7574,7 @@ F: drivers/scsi/storvsc_drv.c
F: drivers/uio/uio_hv_generic.c
F: drivers/video/fbdev/hyperv_fb.c
F: drivers/iommu/hyperv-iommu.c
+F: drivers/vfio/vmbus/
F: net/vmw_vsock/hyperv_transport.c
F: include/clocksource/hyperv_timer.h
F: include/linux/hyperv.h
diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index fd17db9b432f..f4e075fcedbe 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -47,4 +47,5 @@ menuconfig VFIO_NOIOMMU
source "drivers/vfio/pci/Kconfig"
source "drivers/vfio/platform/Kconfig"
source "drivers/vfio/mdev/Kconfig"
+source "drivers/vfio/vmbus/Kconfig"
source "virt/lib/Kconfig"
diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
index de67c4725cce..acef916cba7f 100644
--- a/drivers/vfio/Makefile
+++ b/drivers/vfio/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_VFIO_SPAPR_EEH) += vfio_spapr_eeh.o
obj-$(CONFIG_VFIO_PCI) += pci/
obj-$(CONFIG_VFIO_PLATFORM) += platform/
obj-$(CONFIG_VFIO_MDEV) += mdev/
+obj-$(CONFIG_VFIO_VMBUS) += vmbus/
diff --git a/drivers/vfio/vmbus/Kconfig b/drivers/vfio/vmbus/Kconfig
new file mode 100644
index 000000000000..27a85daae55a
--- /dev/null
+++ b/drivers/vfio/vmbus/Kconfig
@@ -0,0 +1,9 @@
+config VFIO_VMBUS
+ tristate "VFIO support for vmbus devices"
+ depends on VFIO && HYPERV
+ help
+ Support for the VMBUS VFIO bus driver. Expose VMBUS
+ resources to user space drivers(e.g, DPDK and SPDK).
+
+ If you don't know what to do here, say N.
+
diff --git a/drivers/vfio/vmbus/vfio_vmbus.c b/drivers/vfio/vmbus/vfio_vmbus.c
new file mode 100644
index 000000000000..25d9cafa2c0a
--- /dev/null
+++ b/drivers/vfio/vmbus/vfio_vmbus.c
@@ -0,0 +1,407 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * VFIO VMBUS driver.
+ *
+ * Copyright (C) 2019, Microsoft, Inc.
+ *
+ * Author : Lan Tianyu <Tianyu.Lan@microsoft.com>
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/vfio.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/hyperv.h>
+
+#include "../../hv/hyperv_vmbus.h"
+
+
+#define DRIVER_VERSION "0.0.1"
+#define DRIVER_AUTHOR "Tianyu Lan <Tianyu.Lan@microsoft.com>"
+#define DRIVER_DESC "VFIO driver for VMBus devices"
+
+#define HV_RING_SIZE (512 * HV_HYP_PAGE_SIZE) /* 2M Ring size */
+#define SEND_BUFFER_SIZE (16 * 1024 * 1024)
+#define RECV_BUFFER_SIZE (31 * 1024 * 1024)
+
+struct vmbus_mem {
+ phys_addr_t addr;
+ resource_size_t size;
+};
+
+struct vfio_vmbus_device {
+ struct hv_device *hdev;
+ atomic_t refcnt;
+ struct vmbus_mem mem[VFIO_VMBUS_MAX_MAP_NUM];
+
+ void *recv_buf;
+ void *send_buf;
+};
+
+static void vfio_vmbus_channel_cb(void *context)
+{
+ struct vmbus_channel *chan = context;
+
+ /* Disable interrupts on channel */
+ chan->inbound.ring_buffer->interrupt_mask = 1;
+
+ /* Issue a full memory barrier after masking interrupt */
+ virt_mb();
+}
+
+static int vfio_vmbus_ring_mmap(struct file *filp, struct kobject *kobj,
+ struct bin_attribute *attr,
+ struct vm_area_struct *vma)
+{
+ struct vmbus_channel *channel
+ = container_of(kobj, struct vmbus_channel, kobj);
+ void *ring_buffer = page_address(channel->ringbuffer_page);
+
+ if (channel->state != CHANNEL_OPENED_STATE)
+ return -ENODEV;
+
+ return vm_iomap_memory(vma, virt_to_phys(ring_buffer),
+ channel->ringbuffer_pagecount << PAGE_SHIFT);
+}
+
+static const struct bin_attribute ring_buffer_bin_attr = {
+ .attr = {
+ .name = "ring",
+ .mode = 0600,
+ },
+ .size = 2 * HV_RING_SIZE,
+ .mmap = vfio_vmbus_ring_mmap,
+};
+
+static void
+vfio_vmbus_new_channel(struct vmbus_channel *new_sc)
+{
+ struct hv_device *hv_dev = new_sc->primary_channel->device_obj;
+ struct device *device = &hv_dev->device;
+ int ret;
+
+ /* Create host communication ring */
+ ret = vmbus_open(new_sc, HV_RING_SIZE, HV_RING_SIZE, NULL, 0,
+ vfio_vmbus_channel_cb, new_sc);
+ if (ret) {
+ dev_err(device, "vmbus_open subchannel failed: %d\n", ret);
+ return;
+ }
+
+ /* Disable interrupts on sub channel */
+ new_sc->inbound.ring_buffer->interrupt_mask = 1;
+ set_channel_read_mode(new_sc, HV_CALL_ISR);
+
+ ret = sysfs_create_bin_file(&new_sc->kobj, &ring_buffer_bin_attr);
+ if (ret)
+ dev_notice(&hv_dev->device,
+ "sysfs create ring bin file failed; %d\n", ret);
+}
+
+static int vfio_vmbus_open(void *device_data)
+{
+ struct vfio_vmbus_device *vdev = device_data;
+ struct hv_device *dev = vdev->hdev;
+ int ret;
+
+ vmbus_set_sc_create_callback(dev->channel, vfio_vmbus_new_channel);
+
+ ret = vmbus_connect_ring(dev->channel,
+ vfio_vmbus_channel_cb, dev->channel);
+ if (ret == 0)
+ dev->channel->inbound.ring_buffer->interrupt_mask = 1;
+
+ return 0;
+}
+
+static long vfio_vmbus_ioctl(void *device_data, unsigned int cmd,
+ unsigned long arg)
+{
+ struct vfio_vmbus_device *vdev = device_data;
+ unsigned long minsz;
+
+ if (cmd == VFIO_DEVICE_GET_INFO) {
+ struct vfio_device_info info;
+
+ minsz = offsetofend(struct vfio_device_info, num_irqs);
+
+ if (copy_from_user(&info, (void __user *)arg, minsz))
+ return -EFAULT;
+
+ if (info.argsz < minsz)
+ return -EINVAL;
+
+ info.flags = VFIO_DEVICE_FLAGS_VMBUS;
+ info.num_regions = VFIO_VMBUS_MAX_MAP_NUM;
+
+ return copy_to_user((void __user *)arg, &info, minsz) ?
+ -EFAULT : 0;
+ } else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
+ struct vfio_region_info info;
+
+ minsz = offsetofend(struct vfio_region_info, offset);
+
+ if (copy_from_user(&info, (void __user *)arg, minsz))
+ return -EFAULT;
+
+ if (info.argsz < minsz)
+ return -EINVAL;
+
+ if (info.index >= VFIO_VMBUS_MAX_MAP_NUM)
+ return -EINVAL;
+
+ info.offset = vdev->mem[info.index].addr;
+ info.size = vdev->mem[info.index].size;
+ info.flags = VFIO_REGION_INFO_FLAG_READ
+ | VFIO_REGION_INFO_FLAG_WRITE
+ | VFIO_REGION_INFO_FLAG_MMAP;
+
+ return copy_to_user((void __user *)arg, &info, minsz) ?
+ -EFAULT : 0;
+ }
+
+ return -ENOTTY;
+}
+
+static void vfio_vmbus_release(void *device_data)
+{
+ struct vfio_vmbus_device *vdev = device_data;
+
+ vmbus_disconnect_ring(vdev->hdev->channel);
+}
+
+static vm_fault_t vfio_vma_fault(struct vm_fault *vmf)
+{
+ struct vfio_vmbus_device *vdev = vmf->vma->vm_private_data;
+ struct vm_area_struct *vma = vmf->vma;
+ struct page *page;
+ unsigned long offset;
+ void *addr;
+ int index;
+
+ index = vma->vm_pgoff;
+
+ /*
+ * Page fault should only happen on mmap regiones
+ * and bypass GPADL indexes here.
+ */
+ if (index >= VFIO_VMBUS_MAX_MAP_NUM - 2)
+ return VM_FAULT_SIGBUS;
+
+ offset = (vmf->pgoff - index) << PAGE_SHIFT;
+ addr = (void *)(vdev->mem[index].addr + offset);
+
+ if (index == VFIO_VMBUS_SEND_BUF_MAP ||
+ index == VFIO_VMBUS_RECV_BUF_MAP)
+ page = vmalloc_to_page(addr);
+ else
+ page = virt_to_page(addr);
+
+ get_page(page);
+ vmf->page = page;
+
+ return 0;
+}
+
+static const struct vm_operations_struct vfio_logical_vm_ops = {
+ .fault = vfio_vma_fault,
+};
+
+static const struct vm_operations_struct vfio_physical_vm_ops = {
+#ifdef CONFIG_HAVE_IOREMAP_PROT
+ .access = generic_access_phys,
+#endif
+};
+
+static int vfio_vmbus_mmap(void *device_data, struct vm_area_struct *vma)
+{
+ struct vfio_vmbus_device *vdev = device_data;
+ int index;
+
+ if (vma->vm_end < vma->vm_start)
+ return -EINVAL;
+
+ /*
+ * Mmap should only happen on map regions
+ * and bypass GPADL index here.
+ */
+ if (vma->vm_pgoff >= VFIO_VMBUS_MAX_MAP_NUM - 2)
+ return -EINVAL;
+
+ index = vma->vm_pgoff;
+ vma->vm_private_data = vdev;
+
+ if (index == VFIO_VMBUS_TXRX_RING_MAP) {
+ u64 addr;
+
+ addr = vdev->mem[VFIO_VMBUS_TXRX_RING_MAP].addr;
+ vma->vm_ops = &vfio_physical_vm_ops;
+ return remap_pfn_range(vma,
+ vma->vm_start,
+ addr >> PAGE_SHIFT,
+ vma->vm_end - vma->vm_start,
+ vma->vm_page_prot);
+ } else {
+ vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
+ vma->vm_ops = &vfio_logical_vm_ops;
+ return 0;
+ }
+}
+
+static const struct vfio_device_ops vfio_vmbus_ops = {
+ .name = "vfio-vmbus",
+ .open = vfio_vmbus_open,
+ .release = vfio_vmbus_release,
+ .mmap = vfio_vmbus_mmap,
+ .ioctl = vfio_vmbus_ioctl,
+};
+
+static int vfio_vmbus_probe(struct hv_device *dev,
+ const struct hv_vmbus_device_id *dev_id)
+{
+ struct vmbus_channel *channel = dev->channel;
+ struct vfio_vmbus_device *vdev;
+ struct iommu_group *group;
+ u32 gpadl;
+ void *ring_buffer;
+ int ret;
+
+ group = vfio_iommu_group_get(&dev->device);
+ if (!group)
+ return -EINVAL;
+
+ vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
+ if (!vdev) {
+ vfio_iommu_group_put(group, &dev->device);
+ return -ENOMEM;
+ }
+
+ ret = vfio_add_group_dev(&dev->device, &vfio_vmbus_ops, vdev);
+ if (ret)
+ goto free_vdev;
+
+ vdev->hdev = dev;
+ ret = vmbus_alloc_ring(channel, HV_RING_SIZE, HV_RING_SIZE);
+ if (ret)
+ goto del_group_dev;
+
+ set_channel_read_mode(channel, HV_CALL_ISR);
+
+ ring_buffer = page_address(channel->ringbuffer_page);
+ vdev->mem[VFIO_VMBUS_TXRX_RING_MAP].addr
+ = virt_to_phys(ring_buffer);
+ vdev->mem[VFIO_VMBUS_TXRX_RING_MAP].size
+ = channel->ringbuffer_pagecount << PAGE_SHIFT;
+
+ vdev->mem[VFIO_VMBUS_INT_PAGE_MAP].addr
+ = (phys_addr_t)vmbus_connection.int_page;
+ vdev->mem[VFIO_VMBUS_INT_PAGE_MAP].size = PAGE_SIZE;
+
+ vdev->mem[VFIO_VMBUS_MON_PAGE_MAP].addr
+ = (phys_addr_t)vmbus_connection.monitor_pages[1];
+ vdev->mem[VFIO_VMBUS_MON_PAGE_MAP].size = PAGE_SIZE;
+
+ vdev->recv_buf = vzalloc(RECV_BUFFER_SIZE);
+ if (vdev->recv_buf == NULL) {
+ ret = -ENOMEM;
+ goto free_ring;
+ }
+
+ ret = vmbus_establish_gpadl(channel, vdev->recv_buf,
+ RECV_BUFFER_SIZE, &gpadl);
+ if (ret)
+ goto free_recv_buf;
+
+ vdev->mem[VFIO_VMBUS_RECV_BUF_MAP].addr
+ = (phys_addr_t)vdev->recv_buf;
+ vdev->mem[VFIO_VMBUS_RECV_BUF_MAP].size = RECV_BUFFER_SIZE;
+
+ /* Expose Recv GPADL via region info. */
+ vdev->mem[VFIO_VMBUS_RECV_GPADL].addr = gpadl;
+
+ vdev->send_buf = vzalloc(SEND_BUFFER_SIZE);
+ if (vdev->send_buf == NULL) {
+ ret = -ENOMEM;
+ goto free_recv_gpadl;
+ }
+
+ ret = vmbus_establish_gpadl(channel, vdev->send_buf,
+ SEND_BUFFER_SIZE, &gpadl);
+ if (ret)
+ goto free_send_buf;
+
+ vdev->mem[VFIO_VMBUS_SEND_BUF_MAP].addr
+ = (phys_addr_t)vdev->send_buf;
+ vdev->mem[VFIO_VMBUS_SEND_BUF_MAP].size = SEND_BUFFER_SIZE;
+
+ /* Expose Send GPADL via region info. */
+ vdev->mem[VFIO_VMBUS_SEND_GPADL].addr = gpadl;
+
+ ret = sysfs_create_bin_file(&channel->kobj, &ring_buffer_bin_attr);
+ if (ret)
+ dev_notice(&dev->device,
+ "sysfs create ring bin file failed; %d\n", ret);
+
+ return 0;
+
+ free_send_buf:
+ vfree(vdev->send_buf);
+ free_recv_gpadl:
+ vmbus_teardown_gpadl(channel, vdev->mem[VFIO_VMBUS_RECV_GPADL].addr);
+ free_recv_buf:
+ vfree(vdev->recv_buf);
+ free_ring:
+ vmbus_free_ring(channel);
+ del_group_dev:
+ vfio_del_group_dev(&dev->device);
+ free_vdev:
+ vfio_iommu_group_put(group, &dev->device);
+ kfree(vdev);
+ return -EINVAL;
+}
+
+static int vfio_vmbus_remove(struct hv_device *hdev)
+{
+ struct vfio_vmbus_device *vdev = vfio_del_group_dev(&hdev->device);
+ struct vmbus_channel *channel = hdev->channel;
+
+ if (!vdev)
+ return 0;
+
+ sysfs_remove_bin_file(&channel->kobj, &ring_buffer_bin_attr);
+
+ vmbus_teardown_gpadl(channel, vdev->mem[VFIO_VMBUS_SEND_GPADL].addr);
+ vmbus_teardown_gpadl(channel, vdev->mem[VFIO_VMBUS_RECV_GPADL].addr);
+ vfree(vdev->recv_buf);
+ vfree(vdev->send_buf);
+ vmbus_free_ring(channel);
+ vfio_iommu_group_put(hdev->device.iommu_group, &hdev->device);
+ kfree(vdev);
+
+ return 0;
+}
+
+static struct hv_driver hv_vfio_drv = {
+ .name = "hv_vfio",
+ .id_table = NULL,
+ .probe = vfio_vmbus_probe,
+ .remove = vfio_vmbus_remove,
+};
+
+static int __init vfio_vmbus_init(void)
+{
+ return vmbus_driver_register(&hv_vfio_drv);
+}
+
+static void __exit vfio_vmbus_exit(void)
+{
+ vmbus_driver_unregister(&hv_vfio_drv);
+}
+
+module_init(vfio_vmbus_init);
+module_exit(vfio_vmbus_exit);
+
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL v2");
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 9e843a147ead..611baf7a30e4 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -201,6 +201,7 @@ struct vfio_device_info {
#define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */
#define VFIO_DEVICE_FLAGS_CCW (1 << 4) /* vfio-ccw device */
#define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */
+#define VFIO_DEVICE_FLAGS_VMBUS (1 << 6) /* vfio-vmbus device */
__u32 num_regions; /* Max region index + 1 */
__u32 num_irqs; /* Max IRQ index + 1 */
};
@@ -564,6 +565,17 @@ enum {
VFIO_PCI_NUM_IRQS
};
+enum {
+ VFIO_VMBUS_TXRX_RING_MAP = 0,
+ VFIO_VMBUS_INT_PAGE_MAP,
+ VFIO_VMBUS_MON_PAGE_MAP,
+ VFIO_VMBUS_RECV_BUF_MAP,
+ VFIO_VMBUS_SEND_BUF_MAP,
+ VFIO_VMBUS_RECV_GPADL,
+ VFIO_VMBUS_SEND_GPADL,
+ VFIO_VMBUS_MAX_MAP_NUM,
+};
+
/*
* The vfio-ccw bus driver makes use of the following fixed region and
* IRQ index mapping. Unimplemented regions return a size of zero.
--
2.14.5
^ permalink raw reply related
* Re: [PATCH] VFIO/VMBUS: Add VFIO VMBUS driver support
From: Greg KH @ 2019-11-11 9:49 UTC (permalink / raw)
To: lantianyu1986
Cc: alex.williamson, cohuck, kys, haiyangz, sthemmin, sashal,
mchehab+samsung, davem, robh, Jonathan.Cameron, paulmck,
michael.h.kelley, Tianyu Lan, linux-kernel, kvm, linux-hyperv,
vkuznets
In-Reply-To: <20191111084507.9286-1-Tianyu.Lan@microsoft.com>
On Mon, Nov 11, 2019 at 04:45:07PM +0800, lantianyu1986@gmail.com wrote:
> +#define DRIVER_VERSION "0.0.1"
Never a need for DRIVER_VERSION as your driver just becomes part of the
main kernel tree, so please drop this. We keep trying to delete these
types of numbers and they keep coming back...
> +static void
> +vfio_vmbus_new_channel(struct vmbus_channel *new_sc)
> +{
> + struct hv_device *hv_dev = new_sc->primary_channel->device_obj;
> + struct device *device = &hv_dev->device;
> + int ret;
> +
> + /* Create host communication ring */
> + ret = vmbus_open(new_sc, HV_RING_SIZE, HV_RING_SIZE, NULL, 0,
> + vfio_vmbus_channel_cb, new_sc);
> + if (ret) {
> + dev_err(device, "vmbus_open subchannel failed: %d\n", ret);
> + return;
> + }
> +
> + /* Disable interrupts on sub channel */
> + new_sc->inbound.ring_buffer->interrupt_mask = 1;
> + set_channel_read_mode(new_sc, HV_CALL_ISR);
> +
> + ret = sysfs_create_bin_file(&new_sc->kobj, &ring_buffer_bin_attr);
No documentation on this new sysfs file?
And by creating it here, userspace is not notified of it, so tools will
not see it :(
> + if (ret)
> + dev_notice(&hv_dev->device,
> + "sysfs create ring bin file failed; %d\n", ret);
Doesn't the call spit out an error if something happens?
> + ret = sysfs_create_bin_file(&channel->kobj, &ring_buffer_bin_attr);
> + if (ret)
> + dev_notice(&dev->device,
> + "sysfs create ring bin file failed; %d\n", ret);
> +
Again, don't create sysfs files on your own, the bus code should be
doing this for you automatically and in a way that is race-free.
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH net-next 11/14] vsock: add multi-transports support
From: Jorgen Hansen @ 2019-11-11 13:53 UTC (permalink / raw)
To: 'Stefano Garzarella', netdev@vger.kernel.org
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org
In-Reply-To: <20191023095554.11340-12-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
Thanks a lot for working on this!
> With the multi-transports support, we can use vsock with nested VMs (using
> also different hypervisors) loading both guest->host and
> host->guest transports at the same time.
>
> Major changes:
> - vsock core module can be loaded regardless of the transports
> - vsock_core_init() and vsock_core_exit() are renamed to
> vsock_core_register() and vsock_core_unregister()
> - vsock_core_register() has a feature parameter (H2G, G2H, DGRAM)
> to identify which directions the transport can handle and if it's
> support DGRAM (only vmci)
> - each stream socket is assigned to a transport when the remote CID
> is set (during the connect() or when we receive a connection request
> on a listener socket).
How about allowing the transport to be set during bind as well? That
would allow an application to ensure that it is using a specific transport,
i.e., if it binds to the host CID, it will use H2G, and if it binds to something
else it will use G2H? You can still use VMADDR_CID_ANY if you want to
initially listen to both transports.
> The remote CID is used to decide which transport to use:
> - remote CID > VMADDR_CID_HOST will use host->guest transport
> - remote CID <= VMADDR_CID_HOST will use guest->host transport
> - listener sockets are not bound to any transports since no transport
> operations are done on it. In this way we can create a listener
> socket, also if the transports are not loaded or with VMADDR_CID_ANY
> to listen on all transports.
> - DGRAM sockets are handled as before, since only the vmci_transport
> provides this feature.
>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> RFC -> v1:
> - documented VSOCK_TRANSPORT_F_* flags
> - fixed vsock_assign_transport() when the socket is already assigned
> (e.g connection failed)
> - moved features outside of struct vsock_transport, and used as
> parameter of vsock_core_register()
> ---
> drivers/vhost/vsock.c | 5 +-
> include/net/af_vsock.h | 17 +-
> net/vmw_vsock/af_vsock.c | 237 ++++++++++++++++++------
> net/vmw_vsock/hyperv_transport.c | 26 ++-
> net/vmw_vsock/virtio_transport.c | 7 +-
> net/vmw_vsock/virtio_transport_common.c | 28 ++-
> net/vmw_vsock/vmci_transport.c | 31 +++-
> 7 files changed, 270 insertions(+), 81 deletions(-)
>
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index
> d89381166028..dddd85d9a147 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -130,7 +130,12 @@ static struct proto vsock_proto = { #define
> VSOCK_DEFAULT_BUFFER_MAX_SIZE (1024 * 256) #define
> VSOCK_DEFAULT_BUFFER_MIN_SIZE 128
>
> -static const struct vsock_transport *transport_single;
> +/* Transport used for host->guest communication */ static const struct
> +vsock_transport *transport_h2g;
> +/* Transport used for guest->host communication */ static const struct
> +vsock_transport *transport_g2h;
> +/* Transport used for DGRAM communication */ static const struct
> +vsock_transport *transport_dgram;
> static DEFINE_MUTEX(vsock_register_mutex);
>
> /**** UTILS ****/
> @@ -182,7 +187,7 @@ static int vsock_auto_bind(struct vsock_sock *vsk)
> return __vsock_bind(sk, &local_addr);
> }
>
> -static int __init vsock_init_tables(void)
> +static void vsock_init_tables(void)
> {
> int i;
>
> @@ -191,7 +196,6 @@ static int __init vsock_init_tables(void)
>
> for (i = 0; i < ARRAY_SIZE(vsock_connected_table); i++)
> INIT_LIST_HEAD(&vsock_connected_table[i]);
> - return 0;
> }
>
> static void __vsock_insert_bound(struct list_head *list, @@ -376,6 +380,62
> @@ void vsock_enqueue_accept(struct sock *listener, struct sock
> *connected) } EXPORT_SYMBOL_GPL(vsock_enqueue_accept);
>
> +/* Assign a transport to a socket and call the .init transport callback.
> + *
> + * Note: for stream socket this must be called when vsk->remote_addr is
> +set
> + * (e.g. during the connect() or when a connection request on a
> +listener
> + * socket is received).
> + * The vsk->remote_addr is used to decide which transport to use:
> + * - remote CID > VMADDR_CID_HOST will use host->guest transport
> + * - remote CID <= VMADDR_CID_HOST will use guest->host transport */
> +int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock
> +*psk) {
> + const struct vsock_transport *new_transport;
> + struct sock *sk = sk_vsock(vsk);
> +
> + switch (sk->sk_type) {
> + case SOCK_DGRAM:
> + new_transport = transport_dgram;
> + break;
> + case SOCK_STREAM:
> + if (vsk->remote_addr.svm_cid > VMADDR_CID_HOST)
> + new_transport = transport_h2g;
> + else
> + new_transport = transport_g2h;
> + break;
You already mentioned that you are working on a fix for loopback
here for the guest, but presumably a host could also do loopback.
If we select transport during bind to a specific CID, this comment
Isn't relevant, but otherwise, we should look at the local addr as
well, since a socket with local addr of host CID shouldn't use
the guest to host transport, and a socket with local addr > host CID
shouldn't use host to guest.
> + default:
> + return -ESOCKTNOSUPPORT;
> + }
> +
> + if (vsk->transport) {
> + if (vsk->transport == new_transport)
> + return 0;
> +
> + vsk->transport->release(vsk);
> + vsk->transport->destruct(vsk);
> + }
> +
> + if (!new_transport)
> + return -ENODEV;
> +
> + vsk->transport = new_transport;
> +
> + return vsk->transport->init(vsk, psk); }
> +EXPORT_SYMBOL_GPL(vsock_assign_transport);
> +
> +static bool vsock_find_cid(unsigned int cid) {
> + if (transport_g2h && cid == transport_g2h->get_local_cid())
> + return true;
> +
> + if (transport_h2g && cid == VMADDR_CID_HOST)
> + return true;
> +
> + return false;
> +}
> +
> static struct sock *vsock_dequeue_accept(struct sock *listener) {
> struct vsock_sock *vlistener;
> diff --git a/net/vmw_vsock/vmci_transport.c
> b/net/vmw_vsock/vmci_transport.c index 5955238ffc13..2eb3f16d53e7
> 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -1017,6 +1018,15 @@ static int vmci_transport_recv_listen(struct sock
> *sk,
> vsock_addr_init(&vpending->remote_addr, pkt->dg.src.context,
> pkt->src_port);
>
> + err = vsock_assign_transport(vpending, vsock_sk(sk));
> + /* Transport assigned (looking at remote_addr) must be the same
> + * where we received the request.
> + */
> + if (err || !vmci_check_transport(vpending)) {
We need to send a reset on error, i.e.,
vmci_transport_send_reset(sk, pkt);
> + sock_put(pending);
> + return err;
> + }
> +
> /* If the proposed size fits within our min/max, accept it. Otherwise
> * propose our own size.
> */
Thanks,
Jorgen
^ permalink raw reply
* Re: [PATCH v1] tools/hv: async name resolution in kvp_daemon
From: Olaf Hering @ 2019-11-11 15:55 UTC (permalink / raw)
To: Sasha Levin
Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
open list:Hyper-V CORE AND DRIVERS, open list
In-Reply-To: <20191102041856.GY1554@sasha-vm>
[-- Attachment #1: Type: text/plain, Size: 689 bytes --]
Am Sat, 2 Nov 2019 00:18:56 -0400
schrieb Sasha Levin <sashal@kernel.org>:
> make[1]: Leaving directory '/home/sasha/linux/tools/hv'
> gcc -O2 -Wall -g -D_GNU_SOURCE -Iinclude -lpthread hv_kvp_daemon-in.o -o hv_kvp_daemon
> /usr/bin/ld: hv_kvp_daemon-in.o: in function `kvp_obtain_domain_name':
> /home/sasha/linux/tools/hv/hv_kvp_daemon.c:1372: undefined reference to `pthread_create'
> /usr/bin/ld: /home/sasha/linux/tools/hv/hv_kvp_daemon.c:1377: undefined reference to `pthread_detach'
Is perhaps '-pthread' instead of -lpthread' required, as indicated by pthread_create(3)?
Not sure why it happens to work for me. But I will make this change for the upcoming v2.
Olaf
[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* RE: [PATCH net-next 12/14] vsock/vmci: register vmci_transport only when VMCI guest/host are active
From: Jorgen Hansen @ 2019-11-11 16:27 UTC (permalink / raw)
To: 'Stefano Garzarella', netdev@vger.kernel.org
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org
In-Reply-To: <20191023095554.11340-13-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
>
> To allow other transports to be loaded with vmci_transport,
> we register the vmci_transport as G2H or H2G only when a VMCI guest
> or host is active.
>
> To do that, this patch adds a callback registered in the vmci driver
> that will be called when a new host or guest become active.
> This callback will register the vmci_transport in the VSOCK core.
> If the transport is already registered, we ignore the error coming
> from vsock_core_register().
So today this is mainly an issue for the VMCI vsock transport, because
VMCI autoloads with vsock (and with this solution it can continue to
do that, so none of our old products break due to changed behavior,
which is great). Shouldn't vhost behave similar, so that any module
that registers a h2g transport only does so if it is in active use?
> --- a/drivers/misc/vmw_vmci/vmci_host.c
> +++ b/drivers/misc/vmw_vmci/vmci_host.c
> @@ -108,6 +108,11 @@ bool vmci_host_code_active(void)
> atomic_read(&vmci_host_active_users) > 0);
> }
>
> +int vmci_host_users(void)
> +{
> + return atomic_read(&vmci_host_active_users);
> +}
> +
> /*
> * Called on open of /dev/vmci.
> */
> @@ -338,6 +343,8 @@ static int vmci_host_do_init_context(struct
> vmci_host_dev *vmci_host_dev,
> vmci_host_dev->ct_type = VMCIOBJ_CONTEXT;
> atomic_inc(&vmci_host_active_users);
>
> + vmci_call_vsock_callback(true);
> +
Since we don't unregister the transport if user count drops back to 0, we could
just call this the first time, a VM is powered on after the module is loaded.
> retval = 0;
>
> out:
^ permalink raw reply
* RE: [PATCH net-next 13/14] vsock: prevent transport modules unloading
From: Jorgen Hansen @ 2019-11-11 16:36 UTC (permalink / raw)
To: 'Stefano Garzarella', netdev@vger.kernel.org
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org
In-Reply-To: <20191023095554.11340-14-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
> This patch adds 'module' member in the 'struct vsock_transport'
> in order to get/put the transport module. This prevents the
> module unloading while sockets are assigned to it.
>
> We increase the module refcnt when a socket is assigned to a
> transport, and we decrease the module refcnt when the socket
> is destructed.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> RFC -> v1:
> - fixed typo 's/tranport/transport/' in a comment (Stefan)
> ---
> drivers/vhost/vsock.c | 2 ++
> include/net/af_vsock.h | 2 ++
> net/vmw_vsock/af_vsock.c | 20 ++++++++++++++++----
> net/vmw_vsock/hyperv_transport.c | 2 ++
> net/vmw_vsock/virtio_transport.c | 2 ++
> net/vmw_vsock/vmci_transport.c | 1 +
> 6 files changed, 25 insertions(+), 4 deletions(-)
Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
^ permalink raw reply
* Re: [PATCH] VFIO/VMBUS: Add VFIO VMBUS driver support
From: Stephen Hemminger @ 2019-11-11 16:47 UTC (permalink / raw)
To: Greg KH
Cc: lantianyu1986, alex.williamson, cohuck, KY Srinivasan,
Haiyang Zhang, Stephen Hemminger, sashal, mchehab+samsung, davem,
robh, Jonathan.Cameron, paulmck, Michael Kelley, Tianyu Lan,
linux-kernel, kvm, linux-hyperv, vkuznets
In-Reply-To: <20191111094920.GA135867@kroah.com>
On Mon, 11 Nov 2019 01:49:20 -0800
"Greg KH" <gregkh@linuxfoundation.org> wrote:
> > + ret = sysfs_create_bin_file(&channel->kobj,
> &ring_buffer_bin_attr);
> > + if (ret)
> > + dev_notice(&dev->device,
> > + "sysfs create ring bin file failed; %d\n",
> ret);
> > +
>
> Again, don't create sysfs files on your own, the bus code should be
> doing this for you automatically and in a way that is race-free.
>
> thanks,
>
> greg k-h
The sysfs file is only created if the VFIO/UIO driveris used.
^ permalink raw reply
* RE: [PATCH net-next 14/14] vsock: fix bind() behaviour taking care of CID
From: Jorgen Hansen @ 2019-11-11 16:53 UTC (permalink / raw)
To: 'Stefano Garzarella', netdev@vger.kernel.org
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org
In-Reply-To: <20191023095554.11340-15-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
> When we are looking for a socket bound to a specific address,
> we also have to take into account the CID.
>
> This patch is useful with multi-transports support because it
> allows the binding of the same port with different CID, and
> it prevents a connection to a wrong socket bound to the same
> port, but with different CID.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> net/vmw_vsock/af_vsock.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
^ permalink raw reply
* Re: [PATCH net-next 11/14] vsock: add multi-transports support
From: Stefano Garzarella @ 2019-11-11 17:17 UTC (permalink / raw)
To: Jorgen Hansen
Cc: netdev@vger.kernel.org, Michael S. Tsirkin, kvm@vger.kernel.org,
Greg Kroah-Hartman, Jason Wang, David S. Miller, Dexuan Cui,
Haiyang Zhang, Sasha Levin, linux-kernel@vger.kernel.org,
Arnd Bergmann, Stefan Hajnoczi, linux-hyperv@vger.kernel.org,
K. Y. Srinivasan, Stephen Hemminger,
virtualization@lists.linux-foundation.org
In-Reply-To: <MWHPR05MB33761FE4DA27130C72FC5048DA740@MWHPR05MB3376.namprd05.prod.outlook.com>
On Mon, Nov 11, 2019 at 01:53:39PM +0000, Jorgen Hansen wrote:
> > From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> > Sent: Wednesday, October 23, 2019 11:56 AM
>
> Thanks a lot for working on this!
>
Thanks to you for the reviews!
> > With the multi-transports support, we can use vsock with nested VMs (using
> > also different hypervisors) loading both guest->host and
> > host->guest transports at the same time.
> >
> > Major changes:
> > - vsock core module can be loaded regardless of the transports
> > - vsock_core_init() and vsock_core_exit() are renamed to
> > vsock_core_register() and vsock_core_unregister()
> > - vsock_core_register() has a feature parameter (H2G, G2H, DGRAM)
> > to identify which directions the transport can handle and if it's
> > support DGRAM (only vmci)
> > - each stream socket is assigned to a transport when the remote CID
> > is set (during the connect() or when we receive a connection request
> > on a listener socket).
>
> How about allowing the transport to be set during bind as well? That
> would allow an application to ensure that it is using a specific transport,
> i.e., if it binds to the host CID, it will use H2G, and if it binds to something
> else it will use G2H? You can still use VMADDR_CID_ANY if you want to
> initially listen to both transports.
Do you mean for socket that will call the connect()?
For listener socket the "[PATCH net-next 14/14] vsock: fix bind() behaviour
taking care of CID" provides this behaviour.
Since the listener sockets don't use any transport specific callback
(they don't send any data to the remote peer), but they are used as placeholder,
we don't need to assign them to a transport.
>
>
> > The remote CID is used to decide which transport to use:
> > - remote CID > VMADDR_CID_HOST will use host->guest transport
> > - remote CID <= VMADDR_CID_HOST will use guest->host transport
> > - listener sockets are not bound to any transports since no transport
> > operations are done on it. In this way we can create a listener
> > socket, also if the transports are not loaded or with VMADDR_CID_ANY
> > to listen on all transports.
> > - DGRAM sockets are handled as before, since only the vmci_transport
> > provides this feature.
> >
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> > RFC -> v1:
> > - documented VSOCK_TRANSPORT_F_* flags
> > - fixed vsock_assign_transport() when the socket is already assigned
> > (e.g connection failed)
> > - moved features outside of struct vsock_transport, and used as
> > parameter of vsock_core_register()
> > ---
> > drivers/vhost/vsock.c | 5 +-
> > include/net/af_vsock.h | 17 +-
> > net/vmw_vsock/af_vsock.c | 237 ++++++++++++++++++------
> > net/vmw_vsock/hyperv_transport.c | 26 ++-
> > net/vmw_vsock/virtio_transport.c | 7 +-
> > net/vmw_vsock/virtio_transport_common.c | 28 ++-
> > net/vmw_vsock/vmci_transport.c | 31 +++-
> > 7 files changed, 270 insertions(+), 81 deletions(-)
> >
>
>
> > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index
> > d89381166028..dddd85d9a147 100644
> > --- a/net/vmw_vsock/af_vsock.c
> > +++ b/net/vmw_vsock/af_vsock.c
> > @@ -130,7 +130,12 @@ static struct proto vsock_proto = { #define
> > VSOCK_DEFAULT_BUFFER_MAX_SIZE (1024 * 256) #define
> > VSOCK_DEFAULT_BUFFER_MIN_SIZE 128
> >
> > -static const struct vsock_transport *transport_single;
> > +/* Transport used for host->guest communication */ static const struct
> > +vsock_transport *transport_h2g;
> > +/* Transport used for guest->host communication */ static const struct
> > +vsock_transport *transport_g2h;
> > +/* Transport used for DGRAM communication */ static const struct
> > +vsock_transport *transport_dgram;
> > static DEFINE_MUTEX(vsock_register_mutex);
> >
> > /**** UTILS ****/
> > @@ -182,7 +187,7 @@ static int vsock_auto_bind(struct vsock_sock *vsk)
> > return __vsock_bind(sk, &local_addr);
> > }
> >
> > -static int __init vsock_init_tables(void)
> > +static void vsock_init_tables(void)
> > {
> > int i;
> >
> > @@ -191,7 +196,6 @@ static int __init vsock_init_tables(void)
> >
> > for (i = 0; i < ARRAY_SIZE(vsock_connected_table); i++)
> > INIT_LIST_HEAD(&vsock_connected_table[i]);
> > - return 0;
> > }
> >
> > static void __vsock_insert_bound(struct list_head *list, @@ -376,6 +380,62
> > @@ void vsock_enqueue_accept(struct sock *listener, struct sock
> > *connected) } EXPORT_SYMBOL_GPL(vsock_enqueue_accept);
> >
> > +/* Assign a transport to a socket and call the .init transport callback.
> > + *
> > + * Note: for stream socket this must be called when vsk->remote_addr is
> > +set
> > + * (e.g. during the connect() or when a connection request on a
> > +listener
> > + * socket is received).
> > + * The vsk->remote_addr is used to decide which transport to use:
> > + * - remote CID > VMADDR_CID_HOST will use host->guest transport
> > + * - remote CID <= VMADDR_CID_HOST will use guest->host transport */
> > +int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock
> > +*psk) {
> > + const struct vsock_transport *new_transport;
> > + struct sock *sk = sk_vsock(vsk);
> > +
> > + switch (sk->sk_type) {
> > + case SOCK_DGRAM:
> > + new_transport = transport_dgram;
> > + break;
> > + case SOCK_STREAM:
> > + if (vsk->remote_addr.svm_cid > VMADDR_CID_HOST)
> > + new_transport = transport_h2g;
> > + else
> > + new_transport = transport_g2h;
> > + break;
>
> You already mentioned that you are working on a fix for loopback
> here for the guest, but presumably a host could also do loopback.
IIUC we don't support loopback in the host, because in this case the
application will use the CID_HOST as address, but if we are in a nested
VM environment we are in trouble.
Since several people asked about this feature at the KVM Forum, I would like
to add a new VMADDR_CID_LOCAL (i.e. using the reserved 1) and implement
loopback in the core.
What do you think?
> If we select transport during bind to a specific CID, this comment
Also in this case, are you talking about the peer that will call
connect()?
> Isn't relevant, but otherwise, we should look at the local addr as
> well, since a socket with local addr of host CID shouldn't use
> the guest to host transport, and a socket with local addr > host CID
> shouldn't use host to guest.
Yes, I agree, in my fix I'm looking at the local addr, and in L1 I'll
not allow to assign a CID to a nested L2 equal to the CID of L1 (in
vhost-vsock)
Maybe we can allow the host loopback (using CID_HOST), only if there isn't
G2H loaded, but also in this case I'd like to move the loopback in the vsock
core, since we can do that, also if there are no transports loaded.
>
>
> > + default:
> > + return -ESOCKTNOSUPPORT;
> > + }
> > +
> > + if (vsk->transport) {
> > + if (vsk->transport == new_transport)
> > + return 0;
> > +
> > + vsk->transport->release(vsk);
> > + vsk->transport->destruct(vsk);
> > + }
> > +
> > + if (!new_transport)
> > + return -ENODEV;
> > +
> > + vsk->transport = new_transport;
> > +
> > + return vsk->transport->init(vsk, psk); }
> > +EXPORT_SYMBOL_GPL(vsock_assign_transport);
> > +
> > +static bool vsock_find_cid(unsigned int cid) {
> > + if (transport_g2h && cid == transport_g2h->get_local_cid())
> > + return true;
> > +
> > + if (transport_h2g && cid == VMADDR_CID_HOST)
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > static struct sock *vsock_dequeue_accept(struct sock *listener) {
> > struct vsock_sock *vlistener;
>
>
> > diff --git a/net/vmw_vsock/vmci_transport.c
> > b/net/vmw_vsock/vmci_transport.c index 5955238ffc13..2eb3f16d53e7
> > 100644
> > --- a/net/vmw_vsock/vmci_transport.c
> > +++ b/net/vmw_vsock/vmci_transport.c
>
> > @@ -1017,6 +1018,15 @@ static int vmci_transport_recv_listen(struct sock
> > *sk,
> > vsock_addr_init(&vpending->remote_addr, pkt->dg.src.context,
> > pkt->src_port);
> >
> > + err = vsock_assign_transport(vpending, vsock_sk(sk));
> > + /* Transport assigned (looking at remote_addr) must be the same
> > + * where we received the request.
> > + */
> > + if (err || !vmci_check_transport(vpending)) {
>
> We need to send a reset on error, i.e.,
> vmci_transport_send_reset(sk, pkt);
Good catch, I'll fix in the v2.
Thanks,
Stefano
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox