A clocksource driver for HyperV
I am attaching a clocksource driver for HyperV.
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
From: K. Y. Srinivasan ksriniva...@novell.com
Subject: A clocksource for Linux guests hosted on HyperV.
References: None
Patch-mainline:
This patch is a clocksource implementation suitable for guests hosted on HyperV.
Time keeping in Linux guests hosted on HyperV is unstable. This clocksource
driver fixes the problem.
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
Index: linux/drivers/staging/hv/hv_timesource.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ linux/drivers/staging/hv/hv_timesource.c2010-04-05 13:33:36.0
-0600
@@ -0,0 +1,148 @@
+/*
+ * A clocksource for Linux running on HyperV.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan ksriniva...@novell.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include linux/version.h
+#include linux/clocksource.h
+#include linux/init.h
+#include linux/module.h
+#include linux/pci.h
+#include linux/dmi.h
+
+#define HV_CLOCK_SHIFT 22
+/*
+ * HyperV defined synthetic CPUID leaves:
+ */
+#define HV_CPUID_SIGNATURE 0x4000
+#define HV_CPUID_MIN 0x4005
+#define HV_HYPERVISOR_PRESENT_BIT 0x8000
+#define HV_CPUID_FEATURES 0x4003
+#define HV_CPUID_RECOMMENDATIONS 0x4004
+
+/*
+ * HyperV defined synthetic MSRs
+ */
+
+#define HV_X64_MSR_TIME_REF_COUNT 0x4020
+
+
+static cycle_t read_hv_clock(struct clocksource *arg)
+{
+ cycle_t current_tick;
+ /*
+* Read the partition counter to get the current tick count. This count
+* is set to 0 when the partition is created and is incremented in
+* 100 nanosecond units.
+*/
+ rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
+ return current_tick;
+}
+
+static struct clocksource clocksource_hyperv = {
+ .name = hyperv_clocksource,
+ .rating = 400, /* use this when running on Hyperv*/
+ .read = read_hv_clock,
+ .mask = CLOCKSOURCE_MASK(64),
+ .shift = HV_CLOCK_SHIFT,
+};
+
+static struct dmi_system_id __initconst
+hv_timesource_dmi_table[] __maybe_unused = {
+ {
+ .ident = Hyper-V,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, Microsoft Corporation),
+ DMI_MATCH(DMI_PRODUCT_NAME, Virtual Machine),
+ DMI_MATCH(DMI_BOARD_NAME, Virtual Machine),
+ },
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
+
+static struct pci_device_id __initconst
+hv_timesource_pci_table[] __maybe_unused = {
+ { PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
+ { 0 }
+};
+MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
+
+
+static int __init hv_detect_hyperv(void)
+{
+ u32 eax, ebx, ecx, edx;
+ static char hyp_signature[20];
+
+ cpuid(1, eax, ebx, ecx, edx);
+ if (!(ecx HV_HYPERVISOR_PRESENT_BIT)) {
+ printk(KERN_WARNING
+ Not on a Hypervisor\n);
+ return 1;
+ }
+ cpuid(HV_CPUID_SIGNATURE, eax, ebx, ecx, edx);
+ *(u32 *)(hyp_signature + 0) = ebx;
+ *(u32 *)(hyp_signature + 4) = ecx;
+ *(u32 *)(hyp_signature + 8) = edx;
+ hyp_signature[12] = 0;
+
+ if ((eax HV_CPUID_MIN) || (strcmp(Microsoft Hv, hyp_signature))) {
+ printk(KERN_WARNING
+ Not on HyperV; signature %s, eax %x\n,
+ hyp_signature, eax);
+ return 1;
+ }
+ /*
+* Extract the features, recommendations etc.
+*/
+ cpuid(HV_CPUID_FEATURES, eax, ebx, ecx, edx);
+ if (!(eax 0x10)) {
+ printk(KERN_WARNING HyperV Time Ref Counter not available!\n);
+ return 1;
+ }
+
+ cpuid(HV_CPUID_RECOMMENDATIONS, eax, ebx, ecx, edx);
+ printk(KERN_INFO HyperV recommendations: %x\n, eax);
+ printk(KERN_INFO HyperV spin count: %x\n, ebx);
+ return 0;
+}
+
+
+static int __init init_hv_clocksource(void)
+{
+ if
A clocksource driver for HyperV
I am attaching a clocksource driver for HyperV.
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
From: K. Y. Srinivasan ksriniva...@novell.com
Subject: A clocksource for Linux guests hosted on HyperV.
References: None
Patch-mainline:
This patch is a clocksource implementation suitable for guests hosted on HyperV.
Time keeping in Linux guests hosted on HyperV is unstable. This clocksource
driver fixes the problem.
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
Index: linux/drivers/staging/hv/hv_timesource.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ linux/drivers/staging/hv/hv_timesource.c2010-04-05 13:33:36.0
-0600
@@ -0,0 +1,148 @@
+/*
+ * A clocksource for Linux running on HyperV.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan ksriniva...@novell.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include linux/version.h
+#include linux/clocksource.h
+#include linux/init.h
+#include linux/module.h
+#include linux/pci.h
+#include linux/dmi.h
+
+#define HV_CLOCK_SHIFT 22
+/*
+ * HyperV defined synthetic CPUID leaves:
+ */
+#define HV_CPUID_SIGNATURE 0x4000
+#define HV_CPUID_MIN 0x4005
+#define HV_HYPERVISOR_PRESENT_BIT 0x8000
+#define HV_CPUID_FEATURES 0x4003
+#define HV_CPUID_RECOMMENDATIONS 0x4004
+
+/*
+ * HyperV defined synthetic MSRs
+ */
+
+#define HV_X64_MSR_TIME_REF_COUNT 0x4020
+
+
+static cycle_t read_hv_clock(struct clocksource *arg)
+{
+ cycle_t current_tick;
+ /*
+* Read the partition counter to get the current tick count. This count
+* is set to 0 when the partition is created and is incremented in
+* 100 nanosecond units.
+*/
+ rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
+ return current_tick;
+}
+
+static struct clocksource clocksource_hyperv = {
+ .name = hyperv_clocksource,
+ .rating = 400, /* use this when running on Hyperv*/
+ .read = read_hv_clock,
+ .mask = CLOCKSOURCE_MASK(64),
+ .shift = HV_CLOCK_SHIFT,
+};
+
+static struct dmi_system_id __initconst
+hv_timesource_dmi_table[] __maybe_unused = {
+ {
+ .ident = Hyper-V,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, Microsoft Corporation),
+ DMI_MATCH(DMI_PRODUCT_NAME, Virtual Machine),
+ DMI_MATCH(DMI_BOARD_NAME, Virtual Machine),
+ },
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
+
+static struct pci_device_id __initconst
+hv_timesource_pci_table[] __maybe_unused = {
+ { PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
+ { 0 }
+};
+MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
+
+
+static int __init hv_detect_hyperv(void)
+{
+ u32 eax, ebx, ecx, edx;
+ static char hyp_signature[20];
+
+ cpuid(1, eax, ebx, ecx, edx);
+ if (!(ecx HV_HYPERVISOR_PRESENT_BIT)) {
+ printk(KERN_WARNING
+ Not on a Hypervisor\n);
+ return 1;
+ }
+ cpuid(HV_CPUID_SIGNATURE, eax, ebx, ecx, edx);
+ *(u32 *)(hyp_signature + 0) = ebx;
+ *(u32 *)(hyp_signature + 4) = ecx;
+ *(u32 *)(hyp_signature + 8) = edx;
+ hyp_signature[12] = 0;
+
+ if ((eax HV_CPUID_MIN) || (strcmp(Microsoft Hv, hyp_signature))) {
+ printk(KERN_WARNING
+ Not on HyperV; signature %s, eax %x\n,
+ hyp_signature, eax);
+ return 1;
+ }
+ /*
+* Extract the features, recommendations etc.
+*/
+ cpuid(HV_CPUID_FEATURES, eax, ebx, ecx, edx);
+ if (!(eax 0x10)) {
+ printk(KERN_WARNING HyperV Time Ref Counter not available!\n);
+ return 1;
+ }
+
+ cpuid(HV_CPUID_RECOMMENDATIONS, eax, ebx, ecx, edx);
+ printk(KERN_INFO HyperV recommendations: %x\n, eax);
+ printk(KERN_INFO HyperV spin count: %x\n, ebx);
+ return 0;
+}
+
+
+static int __init init_hv_clocksource(void)
+{
+ if
Re: A clocksource driver for HyperV
On 4/5/2010 at 5:36 PM, in message 4bba57fb.2000...@goop.org , Jeremy
Fitzhardinge jer...@goop.org wrote:
On 04/05/2010 01:30 PM, Ky Srinivasan wrote:
+static cycle_t read_hv_clock(struct clocksource *arg)
+{
+cycle_t current_tick;
+/*
+ * Read the partition counter to get the current tick count. This count
+ * is set to 0 when the partition is created and is incremented in
+ * 100 nanosecond units.
+ */
+rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
+return current_tick;
+}
+
+static struct clocksource clocksource_hyperv = {
+.name = hyperv_clocksource,
Seems like a redundantly long name; any use of this string is going to
be in a context where it is obviously a clocksource. How about just
hyperv
+.rating = 400, /* use this when running on Hyperv*/
+.read = read_hv_clock,
+.mask = CLOCKSOURCE_MASK(64),
+.shift = HV_CLOCK_SHIFT,
+};
+
+static struct dmi_system_id __initconst
+hv_timesource_dmi_table[] __maybe_unused = {
+{
+.ident = Hyper-V,
+.matches = {
+DMI_MATCH(DMI_SYS_VENDOR, Microsoft Corporation),
+DMI_MATCH(DMI_PRODUCT_NAME, Virtual Machine),
+DMI_MATCH(DMI_BOARD_NAME, Virtual Machine),
+},
+},
+{ },
+};
+MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
So you use the DMI signatures to determine whether the module is needed,
but cpuid to work out if the feature is present?
+
+static struct pci_device_id __initconst
+hv_timesource_pci_table[] __maybe_unused = {
+{ PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
+{ 0 }
+};
+MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
And/or PCI?
Seems a bit... ad-hoc? Is this the official way to determine the
presence of Hyper-V?
The presence of HyperV and our ability to use the partition-wide counter
obviously is checked via probing the cpuid leaves. The DMI/PCI signatures are
used in auto-loading these modules.
+
+
+static int __init hv_detect_hyperv(void)
This looks generally useful. Should it be hidden away in the
clocksource driver, or in some common hyper-v code? Do other hyper-v
drivers have versions of this?
Good point. Right now, I can think of multiple drivers replicating this code.
We could include hyperV detection code in cpu/hypervisor.c . I will spin up a
patch for doing that shortly.
+{
+u32 eax, ebx, ecx, edx;
+static char hyp_signature[20];
20? static?
+
+cpuid(1,eax,ebx,ecx,edx);
+if (!(ecx HV_HYPERVISOR_PRESENT_BIT)) {
+printk(KERN_WARNING
+Not on a Hypervisor\n);
This just looks like noise, especially since it doesn't identify what is
generating the message. And if you compile this code in as =y
(non-modular) then it will complain every boot.
+return 1;
+}
+cpuid(HV_CPUID_SIGNATURE,eax,ebx,ecx,edx);
+*(u32 *)(hyp_signature + 0) = ebx;
+*(u32 *)(hyp_signature + 4) = ecx;
+*(u32 *)(hyp_signature + 8) = edx;
+hyp_signature[12] = 0;
+
+if ((eax HV_CPUID_MIN) || (strcmp(Microsoft Hv, hyp_signature))) {
memcmp, surely?
+printk(KERN_WARNING
+Not on HyperV; signature %s, eax %x\n,
+hyp_signature, eax);
+return 1;
+}
+/*
+ * Extract the features, recommendations etc.
+ */
+cpuid(HV_CPUID_FEATURES,eax,ebx,ecx,edx);
+if (!(eax 0x10)) {
+printk(KERN_WARNING HyperV Time Ref Counter not available!\n);
+return 1;
+}
+
+cpuid(HV_CPUID_RECOMMENDATIONS,eax,ebx,ecx,edx);
+printk(KERN_INFO HyperV recommendations: %x\n, eax);
+printk(KERN_INFO HyperV spin count: %x\n, ebx);
+return 0;
+}
+
+
+static int __init init_hv_clocksource(void)
+{
+if (hv_detect_hyperv())
+return -ENODEV;
+/*
+ * The time ref counter in HyperV is in 100ns units.
+ * The definition of mult is:
+ * mult/2^shift = ns/cyc = 100
+ * mult = (100 shift)
+ */
+clocksource_hyperv.mult = (100 HV_CLOCK_SHIFT);
Why not initialize this in the structure? It's just 10022 isn't it?
+printk(KERN_INFO Registering HyperV clock source\n);
+return clocksource_register(clocksource_hyperv);
+}
+
+module_init(init_hv_clocksource);
+MODULE_DESCRIPTION(HyperV based clocksource);
+MODULE_AUTHOR(K. Y. Srinivasan ksriniva...@novell.com );
+MODULE_LICENSE(GPL);
Index: linux/drivers/staging/hv/Makefile
===
--- linux.orig/drivers/staging/hv/Makefile 2010-04-05 13:02:06.0
-0600
+++ linux/drivers/staging/hv/Makefile2010-04-05 13:02:13.0
-0600
@@ -1,4 +1,4 @@
-obj-$(CONFIG_HYPERV
Re: A clocksource driver for HyperV
On 4/5/2010 at 5:36 PM, in message 4bba57fb.2000...@goop.org , Jeremy
Fitzhardinge jer...@goop.org wrote:
On 04/05/2010 01:30 PM, Ky Srinivasan wrote:
+static cycle_t read_hv_clock(struct clocksource *arg)
+{
+cycle_t current_tick;
+/*
+ * Read the partition counter to get the current tick count. This count
+ * is set to 0 when the partition is created and is incremented in
+ * 100 nanosecond units.
+ */
+rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
+return current_tick;
+}
+
+static struct clocksource clocksource_hyperv = {
+.name = hyperv_clocksource,
Seems like a redundantly long name; any use of this string is going to
be in a context where it is obviously a clocksource. How about just
hyperv
+.rating = 400, /* use this when running on Hyperv*/
+.read = read_hv_clock,
+.mask = CLOCKSOURCE_MASK(64),
+.shift = HV_CLOCK_SHIFT,
+};
+
+static struct dmi_system_id __initconst
+hv_timesource_dmi_table[] __maybe_unused = {
+{
+.ident = Hyper-V,
+.matches = {
+DMI_MATCH(DMI_SYS_VENDOR, Microsoft Corporation),
+DMI_MATCH(DMI_PRODUCT_NAME, Virtual Machine),
+DMI_MATCH(DMI_BOARD_NAME, Virtual Machine),
+},
+},
+{ },
+};
+MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
So you use the DMI signatures to determine whether the module is needed,
but cpuid to work out if the feature is present?
+
+static struct pci_device_id __initconst
+hv_timesource_pci_table[] __maybe_unused = {
+{ PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
+{ 0 }
+};
+MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
And/or PCI?
Seems a bit... ad-hoc? Is this the official way to determine the
presence of Hyper-V?
The presence of HyperV and our ability to use the partition-wide counter
obviously is checked via probing the cpuid leaves. The DMI/PCI signatures are
used in auto-loading these modules.
+
+
+static int __init hv_detect_hyperv(void)
This looks generally useful. Should it be hidden away in the
clocksource driver, or in some common hyper-v code? Do other hyper-v
drivers have versions of this?
Good point. Right now, I can think of multiple drivers replicating this code.
We could include hyperV detection code in cpu/hypervisor.c . I will spin up a
patch for doing that shortly.
+{
+u32 eax, ebx, ecx, edx;
+static char hyp_signature[20];
20? static?
+
+cpuid(1,eax,ebx,ecx,edx);
+if (!(ecx HV_HYPERVISOR_PRESENT_BIT)) {
+printk(KERN_WARNING
+Not on a Hypervisor\n);
This just looks like noise, especially since it doesn't identify what is
generating the message. And if you compile this code in as =y
(non-modular) then it will complain every boot.
+return 1;
+}
+cpuid(HV_CPUID_SIGNATURE,eax,ebx,ecx,edx);
+*(u32 *)(hyp_signature + 0) = ebx;
+*(u32 *)(hyp_signature + 4) = ecx;
+*(u32 *)(hyp_signature + 8) = edx;
+hyp_signature[12] = 0;
+
+if ((eax HV_CPUID_MIN) || (strcmp(Microsoft Hv, hyp_signature))) {
memcmp, surely?
+printk(KERN_WARNING
+Not on HyperV; signature %s, eax %x\n,
+hyp_signature, eax);
+return 1;
+}
+/*
+ * Extract the features, recommendations etc.
+ */
+cpuid(HV_CPUID_FEATURES,eax,ebx,ecx,edx);
+if (!(eax 0x10)) {
+printk(KERN_WARNING HyperV Time Ref Counter not available!\n);
+return 1;
+}
+
+cpuid(HV_CPUID_RECOMMENDATIONS,eax,ebx,ecx,edx);
+printk(KERN_INFO HyperV recommendations: %x\n, eax);
+printk(KERN_INFO HyperV spin count: %x\n, ebx);
+return 0;
+}
+
+
+static int __init init_hv_clocksource(void)
+{
+if (hv_detect_hyperv())
+return -ENODEV;
+/*
+ * The time ref counter in HyperV is in 100ns units.
+ * The definition of mult is:
+ * mult/2^shift = ns/cyc = 100
+ * mult = (100 shift)
+ */
+clocksource_hyperv.mult = (100 HV_CLOCK_SHIFT);
Why not initialize this in the structure? It's just 10022 isn't it?
+printk(KERN_INFO Registering HyperV clock source\n);
+return clocksource_register(clocksource_hyperv);
+}
+
+module_init(init_hv_clocksource);
+MODULE_DESCRIPTION(HyperV based clocksource);
+MODULE_AUTHOR(K. Y. Srinivasan ksriniva...@novell.com );
+MODULE_LICENSE(GPL);
Index: linux/drivers/staging/hv/Makefile
===
--- linux.orig/drivers/staging/hv/Makefile 2010-04-05 13:02:06.0
-0600
+++ linux/drivers/staging/hv/Makefile2010-04-05 13:02:13.0
-0600
@@ -1,4 +1,4 @@
-obj-$(CONFIG_HYPERV
A Clock source for HyperV
This is a resend of a patch I posted couple of weeks ago. This patch implements
a clocksource for HyperV. This patch depends on the HyperV detection patch I
had posted earler.
From: K. Y. Srinivasan ksriniva...@novell.com
Subject: A clocksource for Linux guests hosted on HyperV.
References: None
Patch-mainline:
This patch is a clocksource implementation suitable for guests hosted on HyperV.
Time keeping in Linux guests hosted on HyperV is unstable. This clocksource
driver fixes the problem.
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
Index: linux-2.6.34-rc4/drivers/staging/hv/hv_timesource.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ linux-2.6.34-rc4/drivers/staging/hv/hv_timesource.c 2010-04-15
13:45:46.0 -0600
@@ -0,0 +1,97 @@
+/*
+ * A clocksource for Linux running on HyperV.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan ksriniva...@novell.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include linux/version.h
+#include linux/clocksource.h
+#include linux/init.h
+#include linux/module.h
+#include linux/pci.h
+#include linux/dmi.h
+#include asm/hyperv.h
+
+#define HV_CLOCK_SHIFT 22
+
+static cycle_t read_hv_clock(struct clocksource *arg)
+{
+ cycle_t current_tick;
+ /*
+* Read the partition counter to get the current tick count. This count
+* is set to 0 when the partition is created and is incremented in
+* 100 nanosecond units.
+*/
+ rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
+ return current_tick;
+}
+
+static struct clocksource hyperv_cs = {
+ .name = hyperv_clocksource,
+ .rating = 400, /* use this when running on Hyperv*/
+ .read = read_hv_clock,
+ .mask = CLOCKSOURCE_MASK(64),
+ /*
+* The time ref counter in HyperV is in 100ns units.
+* The definition of mult is:
+* mult/2^shift = ns/cyc = 100
+* mult = (100 shift)
+*/
+ .mult = (100 HV_CLOCK_SHIFT),
+ .shift = HV_CLOCK_SHIFT,
+};
+
+static const struct dmi_system_id __initconst
+hv_timesource_dmi_table[] __maybe_unused = {
+ {
+ .ident = Hyper-V,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, Microsoft Corporation),
+ DMI_MATCH(DMI_PRODUCT_NAME, Virtual Machine),
+ DMI_MATCH(DMI_BOARD_NAME, Virtual Machine),
+ },
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
+
+static const struct pci_device_id __initconst
+hv_timesource_pci_table[] __maybe_unused = {
+ { PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
+ { 0 }
+};
+MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
+
+
+static int __init init_hv_clocksource(void)
+{
+ if ((boot_cpu_data.x86_hyper_vendor != X86_HYPER_VENDOR_MSFT) ||
+ !(boot_cpu_data.x86_hyper_features
+ HV_X64_MSR_TIME_REF_COUNT_AVAILABLE))
+ return -ENODEV;
+ printk(KERN_INFO Registering HyperV clock source\n);
+ return clocksource_register(hyperv_cs);
+}
+
+module_init(init_hv_clocksource);
+MODULE_DESCRIPTION(HyperV based clocksource);
+MODULE_AUTHOR(K. Y. Srinivasan ksriniva...@novell.com);
+MODULE_LICENSE(GPL);
Index: linux-2.6.34-rc4/drivers/staging/hv/Makefile
===
--- linux-2.6.34-rc4.orig/drivers/staging/hv/Makefile 2010-04-12
19:41:35.0 -0600
+++ linux-2.6.34-rc4/drivers/staging/hv/Makefile2010-04-15
13:09:07.0 -0600
@@ -1,4 +1,4 @@
-obj-$(CONFIG_HYPERV) += hv_vmbus.o
+obj-$(CONFIG_HYPERV) += hv_vmbus.o hv_timesource.o
obj-$(CONFIG_HYPERV_STORAGE) += hv_storvsc.o
obj-$(CONFIG_HYPERV_BLOCK) += hv_blkvsc.o
obj-$(CONFIG_HYPERV_NET) += hv_netvsc.o
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Patch to auto-load MSFT PV NIC driver
I am attaching a patch to auto-load hv_netvsc.
Regards,
K. Y
From: K. Y. Srinivasan ksriniva...@novell.com
Subject: Auto-load the hyperV PV net driver.
References: None
Patch-mainline:
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
Index: linux/drivers/staging/hv/netvsc_drv.c
===
--- linux.orig/drivers/staging/hv/netvsc_drv.c 2010-05-07 15:29:32.0
-0600
+++ linux/drivers/staging/hv/netvsc_drv.c 2010-05-07 15:30:00.0
-0600
@@ -29,6 +29,8 @@
#include linux/etherdevice.h
#include linux/skbuff.h
#include linux/in.h
+#include linux/dmi.h
+#include linux/pci.h
#include net/arp.h
#include net/route.h
#include net/sock.h
@@ -592,6 +594,20 @@ static int netvsc_drv_init(int (*drv_ini
return ret;
}
+static const struct dmi_system_id __initconst
+hv_netvsc_dmi_table[] __maybe_unused = {
+ {
+ .ident = Hyper-V,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, Microsoft Corporation),
+ DMI_MATCH(DMI_PRODUCT_NAME, Virtual Machine),
+ DMI_MATCH(DMI_BOARD_NAME, Virtual Machine),
+ },
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(dmi, hv_netvsc_dmi_table);
+
static int __init netvsc_init(void)
{
int ret;
@@ -599,6 +615,9 @@ static int __init netvsc_init(void)
DPRINT_ENTER(NETVSC_DRV);
DPRINT_INFO(NETVSC_DRV, Netvsc initializing);
+ if (!dmi_check_system(hv_netvsc_dmi_table))
+ return -ENODEV;
+
ret = netvsc_drv_init(NetVscInitialize);
DPRINT_EXIT(NETVSC_DRV);
@@ -613,6 +632,13 @@ static void __exit netvsc_exit(void)
DPRINT_EXIT(NETVSC_DRV);
}
+static const struct pci_device_id __initconst
+hv_netvsc_pci_table[] __maybe_unused = {
+ { PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
+ { 0 }
+};
+MODULE_DEVICE_TABLE(pci, hv_netvsc_pci_table);
+
module_param(netvsc_ringbuffer_size, int, S_IRUGO);
module_init(netvsc_init);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: Patch to auto-load MSFT PV NIC driver
On 5/10/2010 at 1:21 PM, in message 20100510172125.ga1...@kroah.com, Greg KH g...@kroah.com wrote: On Mon, May 10, 2010 at 05:05:02PM +, Hank Janssen wrote: Sent: Saturday, May 08, 2010 7:27 AM - Hank Janssen On Sat, May 08, 2010 at 01:52:01PM +, Hank Janssen wrote: I am not sure if this is the right approach. hv_netvsc takes a dependency on hv_vmbus. hv_vmbus does have the same DMI detection logic in it. But unless hv_vmbus has loaded up competely, hv_netvsc will fail on loadup. And I do not think we can guarantee that hv_vmbus has loaded yet. Yes you can, the dependancies in the module will take care of it. Try it, if you try to load the hv_netvsc module before hv_vmbus, modprobe will load hv_vmbus first. In my testing I have seen issues with timing. It takes a little while for VMBus to start getting the initialization taken care of with Hyper-V. And I have seen netvsc error with unresolved symbols because vmbus had not completed the initialization yet. Odd, that shouldn't happen, unless you are loading modules in parallel. This is indeed strange. Given the module dependency, modprobe is supposed to load all the dependencies first before loading the dependent module. I am wondering if there is some other race in the modules that may be causing this problem. K. Y If we switch to a proper discovery type bus, this should not be an issue. thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization
On 5/21/2010 at 4:55 PM, in message 20100521205527.gb9...@suse.de, Greg
KH
gre...@suse.de wrote:
On Fri, May 21, 2010 at 02:21:46PM -0600, Ky Srinivasan wrote:
On 5/21/2010 at 4:12 PM, in message 20100521201228.ga6...@suse.de,
Greg KH
gre...@suse.de wrote:
On Fri, May 21, 2010 at 07:58:26PM +, Haiyang Zhang wrote:
From: Haiyang Zhang haiya...@microsoft.com
Subject: staging: hv: Fix race condition on IC channel initialization
There is a possible race condition when hv_utils starts to load
immediately
after hv_vmbus is loading - null pointer error could happen.
This patch added an atomic counter to ensure all channels are ready before
vmbus_init() returns. So another module won't have any uninitialized
channel.
Better, but not quite ready...
+/* Counter of IC channels initialized */
+atomic_t hv_utils_initcnt = ATOMIC_INIT(0);
This doesn't need to be an atomic variable, does it really?
Why not have a simple bool variable vmbus_initialized or something.
It starts out as false, and then turns true when you are up and ready.
Then provide a function that tests it:
bool hv_vmbus_ready(void)
{
return vmbus_initialized
}
EXPORT_SYMBOL_GPL(hv_vmbus_ready);
I agree with Greg; I would go a step further and deal with this issue
as part of loading the bus driver. After all, we already have
dependencies established for various LIC drivers on the bus driver.
The fact that even after the bus driver is loaded we cannot reliably
load other drivers implies that there is an additional dependency that
is not currently being handled. Why can't we ensure that the bus
driver is fully initialized before we are done with loading the bus
driver.
Um, I think that is what this patch fixes :)
It just doesn't do it in a way that I think is very good...
Ok, my mistake. When I saw hv_vmbus_ready function being exported, I was under
the impression each of the drivers that depend on the bus driver to check if
the bus driver was properly initialized.
Regards,
K. Y
thanks,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization
On 5/21/2010 at 6:07 PM, in message
1fb5e1d5ca062146b38059374562df7266b8b...@tk5ex14mbxc128.redmond.corp.microsoft.
om, Haiyang Zhang haiya...@microsoft.com wrote:
From: Greg KH [mailto:gre...@suse.de]
+/* Counter of IC channels initialized */
+atomic_t hv_utils_initcnt = ATOMIC_INIT(0);
This doesn't need to be an atomic variable, does it really?
Why not have a simple bool variable vmbus_initialized or something.
It starts out as false, and then turns true when you are up and ready.
Then provide a function that tests it:
bool hv_vmbus_ready(void)
{
return vmbus_initialized
}
EXPORT_SYMBOL_GPL(hv_vmbus_ready);
this turns into a simple function call, again, never needing to know
about message types or any other mess.
This looks good. I will add the hv_vmbus_ready() function. It doesn't even
have to be exported symbol, because it's only used in vmbus module to ensure
all channels are ready before vmbus_init() returns. Other modules won't get
a
chance to see uninitialized channels after hv_vmbus is loaded.
Also, I'll cleanup the printk in hv_utils load/unload.
Regarding the atomic variable -- the channel offer processing function is
triggered by interrupts from host -- should we be concerned about counter++
racing with each other in two interrupts happening around the same time?
You would need to protect the increment, if interrupts are going to come in on
any cpu and update the counter. While in your current implementation interrupts
are only delivered on cpu0, it is still probably good to deal with the more
general case and protect the counter.
On a slightly different note, why don't you make the synchronization more
explicit than what you currently have: Rather than polling the variable in a
loop, why don't you put that context to sleep and the interrupt context that
updates the count would be responsible for issuing the wakeup when the
conditions are appropriate - when all channels are initialized.
Regards,
K. Y
Thanks,
- Haiyang
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
[PATCH]: An implementation of HyperV KVP functionality
I am enclosing a patch that implements the KVP (Key Value Pair) functionality
for Linux guests on HyperV. This functionality allows Microsoft Management
stack to query information from the guest. This functionality is implemented in
two parts: (a) A kernel component that communicates with the host and (b) A
user level daemon that implements data gathering. The attached patch
(kvp.patch) implements the kernel component. I am also attaching the code for
the user-level daemon (kvp_daemon.c) for reference.
Regards,
K. Y
From: K. Y. Srinivasan ksriniva...@novell.com
Subject: An implementation of key/value pair feature (KVP) for Linux
on HyperV.
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
Index: linux.trees.git/drivers/staging/hv/kvp.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ linux.trees.git/drivers/staging/hv/kvp.c2010-11-11 13:45:17.0
-0500
@@ -0,0 +1,404 @@
+/*
+ * An implementation of key value pair (KVP) functionality for Linux.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan ksriniva...@novell.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+
+#include linux/net.h
+#include linux/nls.h
+#include linux/connector.h
+
+#include logging.h
+#include osd.h
+#include vmbus.h
+#include vmbus_packet_format.h
+#include vmbus_channel_interface.h
+#include version_info.h
+#include channel.h
+#include vmbus_private.h
+#include vmbus_api.h
+#include utils.h
+#include kvp.h
+
+
+/*
+ *
+ * The following definitions are shared with the user-mode component; do not
+ * change any of this without making the corresponding changes in
+ * the KVP user-mode component.
+ */
+
+#define CN_KVP_VAL 0x1 /* This supports queries from the kernel */
+#define CN_KVP_USER_VAL 0x2 /* This supports queries from the user */
+
+
+/*
+ * KVP protocol: The user mode component first registers with the
+ * the kernel component. Subsequently, the kernel component requests, data
+ * for the specified keys. In response to this message the user mode component
+ * fills in the value corresponding to the specified key. We overload the
+ * sequence field in the cn_msg header to define our KVP message types.
+ *
+ * XXXKYS: Have a shared header file between the user and kernel (TODO)
+ */
+
+enum kvp_op {
+ KVP_REGISTER = 0, /* Register the user mode component */
+ KVP_KERNEL_GET,/*Kernel is requesting the value for the specified key*/
+ KVP_KERNEL_SET, /*Kernel is providing the value for the specified key*/
+ KVP_USER_GET, /*User is requesting the value for the specified key*/
+ KVP_USER_SET /*User is providing the value for the specified key*/
+};
+
+
+
+#define KVP_KEY_SIZE512
+#define KVP_VALUE_SIZE 2048
+
+
+typedef struct kvp_msg {
+ __u32 kvp_key; /* Key */
+ __u8 kvp_value[0]; /* Corresponding value */
+} kvp_msg_t;
+
+/*
+ * End of shared definitions.
+ */
+
+/*
+ * Registry value types.
+ */
+
+#define REG_SZ 1
+
+/*
+ * Array of keys we support in Linux.
+ *
+ */
+#define KVP_MAX_KEY10
+#define KVP_LIC_VERSION 1
+
+
+static char *kvp_keys[KVP_MAX_KEY] = {FullyQualifiedDomainName,
+ IntegrationServicesVersion,
+ NetworkAddressIPv4,
+ NetworkAddressIPv6,
+ OSBuildNumber,
+ OSName,
+ OSMajorVersion,
+ OSMinorVersion,
+ OSVersion,
+ ProcessorArchitecture,
+ };
+
+/*
+ * Global state maintained for transaction that is being processed.
+ * Note that only one transaction can be active at any point in time.
+ *
+ * This state is set when we receive a request from the host; we
+ * cleanup this state when the transaction is completed - when we respond
+ * to the host with the key value.
+ */
+
+static u8 *recv_buffer; /* the receive buffer that we allocated */
+static int recv_len; /* number of bytes received. */
+static struct vmbus_channel *recv_channel; /*chn on which we got the request*/
+static u64 recv_req_id; /* request ID. */
+static int kvp_current_index;
+
+static int
Re: [PATCH]: An implementation of HyperV KVP functionality
On 11/11/2010 at 3:49 PM, in message 2010124904.24010...@nehalam,
Stephen Hemminger shemmin...@vyatta.com wrote:
On Thu, 11 Nov 2010 13:03:10 -0700
Ky Srinivasan ksriniva...@novell.com wrote:
+static char *kvp_keys[KVP_MAX_KEY] = {FullyQualifiedDomainName,
+IntegrationServicesVersion,
+NetworkAddressIPv4,
+NetworkAddressIPv6,
+OSBuildNumber,
+OSName,
+OSMajorVersion,
+OSMinorVersion,
+OSVersion,
+ProcessorArchitecture,
+};
Minor nit:
static const char *kvp_keys[KVP_MAX_KEY] = {
FullQualifiedDomainName,
Will do.
...
+/*
+ * Global state maintained for transaction that is being processed.
+ * Note that only one transaction can be active at any point in time.
+ *
+ * This state is set when we receive a request from the host; we
+ * cleanup this state when the transaction is completed - when we respond
+ * to the host with the key value.
+ */
+
+static u8 *recv_buffer; /* the receive buffer that we allocated */
+static int recv_len; /* number of bytes received. */
+static struct vmbus_channel *recv_channel; /*chn on which we got the
request*/
+static u64 recv_req_id; /* request ID. */
+static int kvp_current_index;
+
I would put all the state variables for the transaction in one
structure,
Will do.
+static void kvp_timer_func(unsigned long __data)
+{
+ u32 key = *((u32 *)__data);
+ /*
+ * If the timer fires, the user-mode component has not responded;
+ * process the pending transaction.
+ */
+ kvp_respond_to_host(key, Guest timed out);
+}
delayed_work is sometimes better for things like this, since it
runs in user context and can sleep.
Although I don't need to block (sleep) in this code path, I agree with you
having a full context is preferable. I will make the appropriate changes.
+ case (KVP_MAX_KEY):
+ /*
+ * We don't support this key
+ * and any key beyond this.
+ */
+ icmsghdrp-status = HV_E_FAIL;
+ goto callback_done;
+
case labels do not need parens
Thank you; my next version of this patch will incorporate your feedback.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH]: An implementation of HyperV KVP functionality
On 11/11/2010 at 3:49 PM, in message 2010124904.24010...@nehalam,
Stephen Hemminger shemmin...@vyatta.com wrote:
On Thu, 11 Nov 2010 13:03:10 -0700
Ky Srinivasan ksriniva...@novell.com wrote:
+static char *kvp_keys[KVP_MAX_KEY] = {FullyQualifiedDomainName,
+IntegrationServicesVersion,
+NetworkAddressIPv4,
+NetworkAddressIPv6,
+OSBuildNumber,
+OSName,
+OSMajorVersion,
+OSMinorVersion,
+OSVersion,
+ProcessorArchitecture,
+};
Minor nit:
static const char *kvp_keys[KVP_MAX_KEY] = {
FullQualifiedDomainName,
Will do.
...
+/*
+ * Global state maintained for transaction that is being processed.
+ * Note that only one transaction can be active at any point in time.
+ *
+ * This state is set when we receive a request from the host; we
+ * cleanup this state when the transaction is completed - when we respond
+ * to the host with the key value.
+ */
+
+static u8 *recv_buffer; /* the receive buffer that we allocated */
+static int recv_len; /* number of bytes received. */
+static struct vmbus_channel *recv_channel; /*chn on which we got the
request*/
+static u64 recv_req_id; /* request ID. */
+static int kvp_current_index;
+
I would put all the state variables for the transaction in one
structure,
Will do.
+static void kvp_timer_func(unsigned long __data)
+{
+ u32 key = *((u32 *)__data);
+ /*
+ * If the timer fires, the user-mode component has not responded;
+ * process the pending transaction.
+ */
+ kvp_respond_to_host(key, Guest timed out);
+}
delayed_work is sometimes better for things like this, since it
runs in user context and can sleep.
Although I don't need to block (sleep) in this code path, I agree with you
having a full context is preferable. I will make the appropriate changes.
+ case (KVP_MAX_KEY):
+ /*
+ * We don't support this key
+ * and any key beyond this.
+ */
+ icmsghdrp-status = HV_E_FAIL;
+ goto callback_done;
+
case labels do not need parens
Thank you; my next version of this patch will incorporate your feedback.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH]: An implementation of HyperV KVP functionality
On 11/11/2010 at 3:49 PM, in message 2010124904.24010...@nehalam,
Stephen Hemminger shemmin...@vyatta.com wrote:
On Thu, 11 Nov 2010 13:03:10 -0700
Ky Srinivasan ksriniva...@novell.com wrote:
+static char *kvp_keys[KVP_MAX_KEY] = {FullyQualifiedDomainName,
+IntegrationServicesVersion,
+NetworkAddressIPv4,
+NetworkAddressIPv6,
+OSBuildNumber,
+OSName,
+OSMajorVersion,
+OSMinorVersion,
+OSVersion,
+ProcessorArchitecture,
+};
Minor nit:
static const char *kvp_keys[KVP_MAX_KEY] = {
FullQualifiedDomainName,
Will do.
...
+/*
+ * Global state maintained for transaction that is being processed.
+ * Note that only one transaction can be active at any point in time.
+ *
+ * This state is set when we receive a request from the host; we
+ * cleanup this state when the transaction is completed - when we respond
+ * to the host with the key value.
+ */
+
+static u8 *recv_buffer; /* the receive buffer that we allocated */
+static int recv_len; /* number of bytes received. */
+static struct vmbus_channel *recv_channel; /*chn on which we got the
request*/
+static u64 recv_req_id; /* request ID. */
+static int kvp_current_index;
+
I would put all the state variables for the transaction in one
structure,
Will do.
+static void kvp_timer_func(unsigned long __data)
+{
+ u32 key = *((u32 *)__data);
+ /*
+ * If the timer fires, the user-mode component has not responded;
+ * process the pending transaction.
+ */
+ kvp_respond_to_host(key, Guest timed out);
+}
delayed_work is sometimes better for things like this, since it
runs in user context and can sleep.
Although I don't need to block (sleep) in this code path, I agree with you
having a full context is preferable. I will make the appropriate changes.
+ case (KVP_MAX_KEY):
+ /*
+ * We don't support this key
+ * and any key beyond this.
+ */
+ icmsghdrp-status = HV_E_FAIL;
+ goto callback_done;
+
case labels do not need parens
Thank you; my next version of this patch will incorporate your feedback.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH]: An implementation of HyperV KVP functionality
On 11/11/2010 at 4:15 PM, in message 2010211548.ga31...@kroah.com,
Greg
KH g...@kroah.com wrote:
On Thu, Nov 11, 2010 at 01:03:10PM -0700, Ky Srinivasan wrote:
+/*
+ * An implementation of key value pair (KVP) functionality for Linux.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan ksriniva...@novell.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
This is all that is needed.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
You can delete this chunk.
Will do.
+/*
+ * KVP protocol: The user mode component first registers with the
+ * the kernel component. Subsequently, the kernel component requests, data
+ * for the specified keys. In response to this message the user mode
component
+ * fills in the value corresponding to the specified key. We overload the
+ * sequence field in the cn_msg header to define our KVP message types.
+ *
+ * XXXKYS: Have a shared header file between the user and kernel (TODO)
+ */
+
+enum kvp_op {
+KVP_REGISTER = 0, /* Register the user mode component */
+KVP_KERNEL_GET,/*Kernel is requesting the value for the specified key*/
+KVP_KERNEL_SET, /*Kernel is providing the value for the specified key*/
+KVP_USER_GET, /*User is requesting the value for the specified key*/
+KVP_USER_SET /*User is providing the value for the specified key*/
+};
As these values are shared between the kernel and userspace, you should
specifically define them.
Also, your spaces with the /* stuff is incorrect.
And, KVP? That's very generic, how about, HYPERV_KVP_... instead?
That fits the global namespace much better.
I will change the names; deal with the comments etc.
s/kvp_op/hyperv_kvp_op/ as well.
+#define KVP_KEY_SIZE512
+#define KVP_VALUE_SIZE 2048
+
+
+typedef struct kvp_msg {
+__u32 kvp_key; /* Key */
+__u8 kvp_value[0]; /* Corresponding value */
+} kvp_msg_t;
I thought that kvp_value was really KVP_VALUE_SIZE?
kvp_value is typed information and KVP_VALUE_SIZE specifies the maximum size of
the supported value. For instance if kvp_value is a string (which is the case
for all the values currently supported), KVP_VALUE_SIZE specifies the maximum
size of the string that will be supported.
And no typedefs, you did run your code through checkpatch.pl, right?
Why ignore the stuff it spits back at you?
I will fix this.
+static char *kvp_keys[KVP_MAX_KEY] = {FullyQualifiedDomainName,
+IntegrationServicesVersion,
+NetworkAddressIPv4,
+NetworkAddressIPv6,
+OSBuildNumber,
+OSName,
+OSMajorVersion,
+OSMinorVersion,
+OSVersion,
+ProcessorArchitecture,
+};
Why list these at all, as more might come in the future, and the kernel
really doesn't care about them, right?
The core HyperV KVP protocol is such that the host iterates through an index
(starting with 0); the guest is free to associate a key with the index and
return the associated value. The host side iteration is stopped when the guest
returns a failure on an index. MSFT has specified the keys and their ordinal
value in their KVP specification. The array you see above is part of that
specification.
+int
+kvp_init(void)
All of your global symbols should have hyperv_ on the front of them.
Will do.
--- linux.trees.git.orig/drivers/staging/hv/Makefile 2010-11-10
14:01:55.0
-0500
+++ linux.trees.git/drivers/staging/hv/Makefile 2010-11-11
11:24:54.0
-0500
@@ -2,7 +2,7 @@ obj-$(CONFIG_HYPERV) += hv_vmbus.o hv_t
obj-$(CONFIG_HYPERV_STORAGE)+= hv_storvsc.o
obj-$(CONFIG_HYPERV_BLOCK) += hv_blkvsc.o
obj-$(CONFIG_HYPERV_NET)+= hv_netvsc.o
-obj-$(CONFIG_HYPERV_UTILS) += hv_utils.o
+obj-$(CONFIG_HYPERV_UTILS) += hv_util.o
Ick, you just renamed the kernel module. Did you really mean to do
this? What tools are now going to break because you did this? (I'm
thinking installers here...)
Oops! I will fix that.
hv_vmbus-y := vmbus_drv.o osd.o \
vmbus.o hv.o connection.o channel.o \
@@ -10,3 +10,4 @@ hv_vmbus-y := vmbus_drv.o
Re: [PATCH]: An implementation of HyperV KVP functionality
On 11/11/2010 at 4:19 PM, in message 2010211904.gb31...@kroah.com,
Greg
KH g...@kroah.com wrote:
On Thu, Nov 11, 2010 at 01:03:10PM -0700, Ky Srinivasan wrote:
+/*
+ * Array of keys we support in Linux.
Not really, you can support any number of keys as the kernel shouldn't
care, or did I get it wrong?
We currently support only the keys that have been specified in the KVP
specification. I have a more detailed response on the core KVP protocol in
response to your other email on this topic.
+ *
+ */
+#define KVP_MAX_KEY 10
+#define KVP_LIC_VERSION 1
Um, this is a nice magic number, care to explain it a bit more?
As I noted in an earlier email, the KVP specification currently requires that
we support 10 keys and it also specifies the ordering of these keys. The
information for the key IntegrationServicesVersion, is only available in the
kernel (one of the other LIC drivers defines this information).
+static char *kvp_keys[KVP_MAX_KEY] = {FullyQualifiedDomainName,
+IntegrationServicesVersion,
Looks like it matches up with this, right? You might want to make that
a bit more tied together.
Yes; I will fix this.
+case (KVP_LIC_VERSION):
+kvp_transaction_active = true;
+kvp_respond_to_host(kvp_data-index,
+HV_DRV_VERSION);
Why are you doing this in the kernel? Why not do it from userspace like
all other messages?
This information is only available in the kernel (defined by another LIC
driver).
Regards,
K. Y
thanks,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH]: An implementation of HyperV KVP functionality
On 11/12/2010 at 1:47 PM, in message 20101112184753.ga20...@kroah.com,
Greg
KH g...@kroah.com wrote:
On Fri, Nov 12, 2010 at 11:06:18AM -0700, Ky Srinivasan wrote:
+typedef struct kvp_msg {
+ __u32 kvp_key; /* Key */
+ __u8 kvp_value[0]; /* Corresponding value */
+} kvp_msg_t;
I thought that kvp_value was really KVP_VALUE_SIZE?
kvp_value is typed information and KVP_VALUE_SIZE specifies the
maximum size of the supported value. For instance if kvp_value is a
string (which is the case for all the values currently supported),
KVP_VALUE_SIZE specifies the maximum size of the string that will be
supported.
So it's a variable length structure? How do you konw how long the
structure is, does that depend on the key?
kvp_value is a null terminated string. In the current implementation; the
kernel component sends an index (key) to the user mode and receives the
corresponding value - a string.
And no typedefs, you did run your code through checkpatch.pl, right?
Why ignore the stuff it spits back at you?
I will fix this.
+static char *kvp_keys[KVP_MAX_KEY] = {FullyQualifiedDomainName,
+ IntegrationServicesVersion,
+ NetworkAddressIPv4,
+ NetworkAddressIPv6,
+ OSBuildNumber,
+ OSName,
+ OSMajorVersion,
+ OSMinorVersion,
+ OSVersion,
+ ProcessorArchitecture,
+ };
Why list these at all, as more might come in the future, and the kernel
really doesn't care about them, right?
The core HyperV KVP protocol is such that the host iterates through an
index (starting with 0); the guest is free to associate a key with the
index and return the associated value. The host side iteration is
stopped when the guest returns a failure on an index. MSFT has
specified the keys and their ordinal value in their KVP specification.
The array you see above is part of that specification.
So you match on the string name of the key? I'm confused, as I didn't
think I saw you matching on the string name, only the key value.
Also, as the kernel isn't handling the key type (with one exception),
why even list these in the kernel at all? I'm all for documenting
stuff, but don't use code memory for documentation :)
When we respond back to the host, we need to specify the key for which the
value is being returned. Note that the host only iterates over an integer key
space while the guest is expected to return both the key name (guest is free to
associate the key name with the integer index) and the corresponding value. I
use, the array of strings kvp_keys[] to implement the mapping between the
integer index and the key name. Look at the function kvp_respond_to_host()
where we lookup the kvp_keys[] array prior to responding back to the host.
In the next iteration of this patch, I am thinking of moving index to key
mapping code into the user-level daemon, so the kernel side will not have to be
modified as the key-space expands (in the future).
--- linux.trees.git.orig/include/linux/connector.h2010-11-09
17:22:15.0
-0500
+++ linux.trees.git/include/linux/connector.h 2010-11-11
13:14:52.0
-0500
@@ -42,8 +42,9 @@
#define CN_VAL_DM_USERSPACE_LOG 0x1
#define CN_IDX_DRBD 0x8
#define CN_VAL_DRBD 0x1
+#define CN_KVP_IDX 0x9 /* MSFT KVP
functionality */
Did you reserve this number with anyone? Who?
I sent an email to Evgeniy Polyakov (john...@2ka.mipt). The mail
bounced back. I was hoping you would help me register this index.
Would it make sense to have a separate patch to deal with registering
this index?
Yes, as I can not modify non-staging code without an ack from that
maintainer. And I'm pretty sure that Evgeniy's address has just
changed, use Evgeniy Polyakov z...@ioremap.net instead.
Thanks I will do just that.
K. Y
thanks,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
[PATCH 1/3]: An implementation of HyperV KVP functionality
From: K. Y. Srinivasan ksriniva...@novell.com Subject: Reserve a connector index for implementing HyperV Key Value Pair (KVP) functionality. Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com Index: linux.trees.git/include/linux/connector.h === --- linux.trees.git.orig/include/linux/connector.h 2010-11-15 06:50:30.0 -0500 +++ linux.trees.git/include/linux/connector.h 2010-11-15 07:00:52.0 -0500 @@ -42,8 +42,9 @@ #define CN_VAL_DM_USERSPACE_LOG0x1 #define CN_IDX_DRBD0x8 #define CN_VAL_DRBD0x1 +#define CN_KVP_IDX 0x9 /* HyperV KVP */ -#define CN_NETLINK_USERS 8 +#define CN_NETLINK_USERS 9 /* * Maximum connector's message size. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
[PATCH 2/3]: An Implementation of HyperV KVP functionality
The hv_utils module will be composed of more than one file;
rename hv_utils.c to accommodate this without changing the module name.
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
From: K. Y. Srinivasan ksriniva...@novell.com
Subject: The hv_utils module will be composed of more than one file;
rename hv_utils.c to accommodate this without changing the module name.
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
Index: linux.trees.git/drivers/staging/hv/hv_util.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ linux.trees.git/drivers/staging/hv/hv_util.c2010-11-19
17:30:48.0 -0500
@@ -0,0 +1,311 @@
+/*
+ * Copyright (c) 2010, Microsoft Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * Authors:
+ * Haiyang Zhang haiya...@microsoft.com
+ * Hank Janssen hjans...@microsoft.com
+ */
+#include linux/kernel.h
+#include linux/init.h
+#include linux/module.h
+#include linux/slab.h
+#include linux/sysctl.h
+#include linux/reboot.h
+#include linux/dmi.h
+#include linux/pci.h
+
+#include logging.h
+#include osd.h
+#include vmbus.h
+#include vmbus_packet_format.h
+#include vmbus_channel_interface.h
+#include version_info.h
+#include channel.h
+#include vmbus_private.h
+#include vmbus_api.h
+#include utils.h
+
+
+static void shutdown_onchannelcallback(void *context)
+{
+ struct vmbus_channel *channel = context;
+ u8 *buf;
+ u32 buflen, recvlen;
+ u64 requestid;
+ u8 execute_shutdown = false;
+
+ struct shutdown_msg_data *shutdown_msg;
+
+ struct icmsg_hdr *icmsghdrp;
+ struct icmsg_negotiate *negop = NULL;
+
+ buflen = PAGE_SIZE;
+ buf = kmalloc(buflen, GFP_ATOMIC);
+
+ vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
+
+ if (recvlen 0) {
+ DPRINT_DBG(VMBUS, shutdown packet: len=%d, requestid=%lld,
+ recvlen, requestid);
+
+ icmsghdrp = (struct icmsg_hdr *)buf[
+ sizeof(struct vmbuspipe_hdr)];
+
+ if (icmsghdrp-icmsgtype == ICMSGTYPE_NEGOTIATE) {
+ prep_negotiate_resp(icmsghdrp, negop, buf);
+ } else {
+ shutdown_msg = (struct shutdown_msg_data *)buf[
+ sizeof(struct vmbuspipe_hdr) +
+ sizeof(struct icmsg_hdr)];
+
+ switch (shutdown_msg-flags) {
+ case 0:
+ case 1:
+ icmsghdrp-status = HV_S_OK;
+ execute_shutdown = true;
+
+ DPRINT_INFO(VMBUS, Shutdown request received -
+gracefull shutdown initiated);
+ break;
+ default:
+ icmsghdrp-status = HV_E_FAIL;
+ execute_shutdown = false;
+
+ DPRINT_INFO(VMBUS, Shutdown request received -
+Invalid request);
+ break;
+ };
+ }
+
+ icmsghdrp-icflags = ICMSGHDRFLAG_TRANSACTION
+ | ICMSGHDRFLAG_RESPONSE;
+
+ vmbus_sendpacket(channel, buf,
+ recvlen, requestid,
+ VmbusPacketTypeDataInBand, 0);
+ }
+
+ kfree(buf);
+
+ if (execute_shutdown == true)
+ orderly_poweroff(false);
+}
+
+/*
+ * Set guest time to host UTC time.
+ */
+static inline void do_adj_guesttime(u64 hosttime)
+{
+ s64 host_tns;
+ struct timespec host_ts;
+
+ host_tns = (hosttime - WLTIMEDELTA) * 100;
+ host_ts = ns_to_timespec(host_tns);
+
+ do_settimeofday(host_ts);
+}
+
+/*
+ * Synchronize time with host after reboot, restore, etc.
+ *
+ * ICTIMESYNCFLAG_SYNC flag bit indicates reboot, restore events of the VM.
+ * After reboot the flag ICTIMESYNCFLAG_SYNC is included in the first time
+ * message after the timesync channel is opened. Since the hv_utils module is
+ * loaded after hv_vmbus, the first message is usually missed. The other
+ * thing is,
[PATCH 3/3]: An implementation of HyperV KVP functionality
An implementation of key/value pair feature (KVP) for Linux on HyperV. In this
version of the patch I have addressed all the comments I have received to date.
I have also included the code for the user-level daemon here for your reference.
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
From: K. Y. Srinivasan ksriniva...@novell.com
Subject: An implementation of key/value pair feature (KVP) for Linux
on HyperV.
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
Index: linux.trees.git/drivers/staging/hv/Makefile
===
--- linux.trees.git.orig/drivers/staging/hv/Makefile2010-11-19
17:57:16.0 -0500
+++ linux.trees.git/drivers/staging/hv/Makefile 2010-11-19 17:57:17.0
-0500
@@ -10,4 +10,4 @@ hv_vmbus-y := vmbus_drv.o osd.o \
hv_storvsc-y := storvsc_drv.o storvsc.o
hv_blkvsc-y := blkvsc_drv.o blkvsc.o
hv_netvsc-y := netvsc_drv.o netvsc.o rndis_filter.o
-hv_utils-y := hv_util.o
+hv_utils-y := hv_util.o hv_kvp.o
Index: linux.trees.git/drivers/staging/hv/utils.h
===
--- linux.trees.git.orig/drivers/staging/hv/utils.h 2010-11-19
17:30:48.0 -0500
+++ linux.trees.git/drivers/staging/hv/utils.h 2010-11-19 17:57:17.0
-0500
@@ -102,6 +102,7 @@ struct ictimesync_data{
#define HV_SHUTDOWN_MSG0
#define HV_TIMESYNC_MSG1
#define HV_HEARTBEAT_MSG 2
+#define HV_KVP_MSG 3
struct hyperv_service_callback {
u8 msg_type;
Index: linux.trees.git/drivers/staging/hv/hv_kvp.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ linux.trees.git/drivers/staging/hv/hv_kvp.c 2010-11-19 18:02:21.0
-0500
@@ -0,0 +1,356 @@
+/*
+ * An implementation of key value pair (KVP) functionality for Linux.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan ksriniva...@novell.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+
+#include linux/net.h
+#include linux/nls.h
+#include linux/connector.h
+#include linux/workqueue.h
+
+#include logging.h
+#include osd.h
+#include vmbus.h
+#include vmbus_packet_format.h
+#include vmbus_channel_interface.h
+#include version_info.h
+#include channel.h
+#include vmbus_private.h
+#include vmbus_api.h
+#include utils.h
+#include hv_kvp.h
+
+
+
+/*
+ * Global state maintained for transaction that is being processed.
+ * Note that only one transaction can be active at any point in time.
+ *
+ * This state is set when we receive a request from the host; we
+ * cleanup this state when the transaction is completed - when we respond
+ * to the host with the key value.
+ */
+
+static struct {
+ bool active; /* transaction status - active or not */
+ u8 *recv_buffer; /* the receive buffer that we allocated */
+ int recv_len; /* number of bytes received. */
+ struct vmbus_channel *recv_channel; /* chn we got the request */
+ u64 recv_req_id; /* request ID. */
+} kvp_transaction;
+
+static int kvp_send_key(int index);
+
+static void kvp_respond_to_host(char *key, char *value, int error);
+static void kvp_work_func(struct work_struct *dummy);
+static void kvp_register(void);
+
+static DECLARE_DELAYED_WORK(kvp_work, kvp_work_func);
+
+static struct cb_id kvp_id = { CN_KVP_IDX, CN_KVP_VAL };
+static const char kvp_name[] = kvp_kernel_module;
+static int timeout_fired;
+
+/*
+ * Register the kernel component with the user-level daemon.
+ * As part of this registration, pass the LIC version number.
+ */
+
+static void
+kvp_register(void)
+{
+
+ struct cn_msg *msg;
+
+ msg = kzalloc(sizeof(*msg) + strlen(HV_DRV_VERSION) + 1 , GFP_ATOMIC);
+
+ if (msg) {
+ msg-id.idx = CN_KVP_IDX;
+ msg-id.val = CN_KVP_VAL;
+ msg-seq = KVP_REGISTER;
+ strcpy(msg-data, HV_DRV_VERSION);
+ msg-len = strlen(HV_DRV_VERSION) + 1;
+ cn_netlink_send(msg, 0, GFP_ATOMIC);
+ kfree(msg);
+ }
+}
+static void
+kvp_work_func(struct work_struct *dummy)
+{
+ /*
+* If the timer fires, the user-mode component has not responded;
+* process the pending transaction.
+*/
+
Re: [PATCH 2/3]: An Implementation of HyperV KVP functionality
On 11/24/2010 at 9:56 AM, in message 20101124145617.ga11...@ioremap.net,
Evgeniy Polyakov z...@ioremap.net wrote:
Hi.
I will ack connector part of course, but this hunk is actually quite
Thank you.
bad.
+static void shutdown_onchannelcallback(void *context)
+{
+struct vmbus_channel *channel = context;
+u8 *buf;
+u32 buflen, recvlen;
+u64 requestid;
+u8 execute_shutdown = false;
+
+struct shutdown_msg_data *shutdown_msg;
+
+struct icmsg_hdr *icmsghdrp;
+struct icmsg_negotiate *negop = NULL;
+
+buflen = PAGE_SIZE;
+buf = kmalloc(buflen, GFP_ATOMIC);
+
+vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
Boom. I did not read further, since this function returns void and thus
can not propagate error, which is likely not a good idea.
Hank and Haiyang (both copied here) are the authors of this code. My
contribution to this code (in this patch) has been to change the name of the
file! I will let Hank and Haiyang comment on your feedback.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/8] staging: hv: Convert camel case struct fields in storvsc_api.h to lowercase
Greg, My new KVP patches are ready to go. Should I wait for Hank's patches to be checked in before I send the KVP patches? Regards K. Y On 12/6/2010 at 3:26 PM, in message 1291667211-1865-2-git-send-email-hjans...@microsoft.com, Hank Janssen hjans...@microsoft.com wrote: From: Hank Janssen hjans...@microsoft.com Convert camel case struct fields in vstorage.h to lowercase Signed-off-by: Abhishek Kane v-abk...@microsoft.com Signed-off-by: Haiyang Zhang haiya...@microsoft.com Signed-off-by: Hank Janssen hjans...@microsoft.com --- drivers/staging/hv/blkvsc.c | 20 +++--- drivers/staging/hv/blkvsc_drv.c | 115 -- drivers/staging/hv/storvsc.c | 67 +++--- drivers/staging/hv/storvsc_api.h | 50 drivers/staging/hv/storvsc_drv.c | 91 +++--- 5 files changed, 175 insertions(+), 168 deletions(-) diff --git a/drivers/staging/hv/blkvsc.c b/drivers/staging/hv/blkvsc.c index d5b0abd..9ac04c3 100644 --- a/drivers/staging/hv/blkvsc.c +++ b/drivers/staging/hv/blkvsc.c @@ -51,12 +51,12 @@ static int BlkVscOnDeviceAdd(struct hv_device *Device, void *AdditionalInfo) * id. For IDE devices, the device instance id is formatted as * bus id * - device id - 8899 - . */ - deviceInfo-PathId = Device-deviceInstance.data[3] 24 | + deviceInfo-path_id = Device-deviceInstance.data[3] 24 | Device-deviceInstance.data[2] 16 | Device-deviceInstance.data[1] 8 | Device-deviceInstance.data[0]; - deviceInfo-TargetId = Device-deviceInstance.data[5] 8 | + deviceInfo-target_id = Device-deviceInstance.data[5] 8 | Device-deviceInstance.data[4]; return ret; @@ -75,7 +75,7 @@ int BlkVscInitialize(struct hv_driver *Driver) Driver-name = gBlkDriverName; memcpy(Driver-deviceType, gBlkVscDeviceType, sizeof(struct hv_guid)); - storDriver-RequestExtSize = sizeof(struct storvsc_request_extension); + storDriver-request_ext_size = sizeof(struct storvsc_request_extension); /* * Divide the ring buffer data size (which is 1 page less than the ring @@ -83,20 +83,20 @@ int BlkVscInitialize(struct hv_driver *Driver) * by the max request size (which is * vmbus_channel_packet_multipage_buffer + struct vstor_packet + u64) */ - storDriver-MaxOutstandingRequestsPerChannel = - ((storDriver-RingBufferSize - PAGE_SIZE) / + storDriver-max_outstanding_req_per_channel = + ((storDriver-ring_buffer_size - PAGE_SIZE) / ALIGN_UP(MAX_MULTIPAGE_BUFFER_PACKET + sizeof(struct vstor_packet) + sizeof(u64), sizeof(u64))); DPRINT_INFO(BLKVSC, max io outstd %u, - storDriver-MaxOutstandingRequestsPerChannel); + storDriver-max_outstanding_req_per_channel); /* Setup the dispatch table */ - storDriver-Base.OnDeviceAdd = BlkVscOnDeviceAdd; - storDriver-Base.OnDeviceRemove = StorVscOnDeviceRemove; - storDriver-Base.OnCleanup = StorVscOnCleanup; - storDriver-OnIORequest = StorVscOnIORequest; + storDriver-base.OnDeviceAdd = BlkVscOnDeviceAdd; + storDriver-base.OnDeviceRemove = StorVscOnDeviceRemove; + storDriver-base.OnCleanup = StorVscOnCleanup; + storDriver-on_io_request = StorVscOnIORequest; return ret; } diff --git a/drivers/staging/hv/blkvsc_drv.c b/drivers/staging/hv/blkvsc_drv.c index 3f81ca5..d65d69e 100644 --- a/drivers/staging/hv/blkvsc_drv.c +++ b/drivers/staging/hv/blkvsc_drv.c @@ -177,13 +177,13 @@ static int blkvsc_drv_init(int (*drv_init)(struct hv_driver *drv)) struct driver_context *drv_ctx = g_blkvsc_drv.drv_ctx; int ret; - storvsc_drv_obj-RingBufferSize = blkvsc_ringbuffer_size; + storvsc_drv_obj-ring_buffer_size = blkvsc_ringbuffer_size; /* Callback to client driver to complete the initialization */ - drv_init(storvsc_drv_obj-Base); + drv_init(storvsc_drv_obj-base); - drv_ctx-driver.name = storvsc_drv_obj-Base.name; - memcpy(drv_ctx-class_id, storvsc_drv_obj-Base.deviceType, + drv_ctx-driver.name = storvsc_drv_obj-base.name; + memcpy(drv_ctx-class_id, storvsc_drv_obj-base.deviceType, sizeof(struct hv_guid)); drv_ctx-probe = blkvsc_probe; @@ -230,8 +230,8 @@ static void blkvsc_drv_exit(void) device_unregister(current_dev); } - if (storvsc_drv_obj-Base.OnCleanup) - storvsc_drv_obj-Base.OnCleanup(storvsc_drv_obj-Base); + if (storvsc_drv_obj-base.OnCleanup) + storvsc_drv_obj-base.OnCleanup(storvsc_drv_obj-base); vmbus_child_driver_unregister(drv_ctx); @@ -262,7 +262,7 @@ static int
[PATCH 1/3]: An implementation of HyperV KVP functionality
This patch is re-based on the latest linux-next tree. From: K. Y. Srinivasan ksriniva...@novell.com Subject: Reserve a connector index for implementing HyperV Key Value Pair (KVP) functionality. Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com Index: linux.trees.git/include/linux/connector.h === --- linux.trees.git.orig/include/linux/connector.h 2010-11-05 16:45:32.0 -0400 +++ linux.trees.git/include/linux/connector.h 2010-12-07 06:58:34.0 -0500 @@ -42,8 +42,9 @@ #define CN_VAL_DM_USERSPACE_LOG0x1 #define CN_IDX_DRBD0x8 #define CN_VAL_DRBD0x1 +#define CN_KVP_IDX 0x9 /* HyperV KVP */ -#define CN_NETLINK_USERS 8 +#define CN_NETLINK_USERS 9 /* * Maximum connector's message size. From: K. Y. Srinivasan ksriniva...@novell.com Subject: Reserve a connector index for implementing HyperV Key Value Pair (KVP) functionality. Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com Index: linux.trees.git/include/linux/connector.h === --- linux.trees.git.orig/include/linux/connector.h 2010-11-05 16:45:32.0 -0400 +++ linux.trees.git/include/linux/connector.h 2010-12-07 06:58:34.0 -0500 @@ -42,8 +42,9 @@ #define CN_VAL_DM_USERSPACE_LOG0x1 #define CN_IDX_DRBD0x8 #define CN_VAL_DRBD0x1 +#define CN_KVP_IDX 0x9 /* HyperV KVP */ -#define CN_NETLINK_USERS 8 +#define CN_NETLINK_USERS 9 /* * Maximum connector's message size. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
[PATCH 2/3]: An implementation of HyperV KVP functionality
This patch is re-based on the latest linux-next tree.
From: K. Y. Srinivasan ksriniva...@novell.com
Subject: The hv_utils module will be composed of more than one file;
rename hv_utils.c to accommodate this without changing the module name.
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
Index: linux.trees.git/drivers/staging/hv/Makefile
===
--- linux.trees.git.orig/drivers/staging/hv/Makefile2010-12-07
07:04:41.0 -0500
+++ linux.trees.git/drivers/staging/hv/Makefile 2010-12-07 08:00:10.0
-0500
@@ -10,3 +10,4 @@ hv_vmbus-y := vmbus_drv.o osd.o \
hv_storvsc-y := storvsc_drv.o storvsc.o
hv_blkvsc-y := blkvsc_drv.o blkvsc.o
hv_netvsc-y := netvsc_drv.o netvsc.o rndis_filter.o
+hv_utils-y := hv_util.o
Index: linux.trees.git/drivers/staging/hv/hv_util.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ linux.trees.git/drivers/staging/hv/hv_util.c2010-12-07
08:00:10.0 -0500
@@ -0,0 +1,308 @@
+/*
+ * Copyright (c) 2010, Microsoft Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * Authors:
+ * Haiyang Zhang haiya...@microsoft.com
+ * Hank Janssen hjans...@microsoft.com
+ */
+#include linux/kernel.h
+#include linux/init.h
+#include linux/module.h
+#include linux/slab.h
+#include linux/sysctl.h
+#include linux/reboot.h
+#include linux/dmi.h
+#include linux/pci.h
+
+#include logging.h
+#include osd.h
+#include vmbus.h
+#include vmbus_packet_format.h
+#include vmbus_channel_interface.h
+#include version_info.h
+#include channel.h
+#include vmbus_private.h
+#include vmbus_api.h
+#include utils.h
+
+
+static void shutdown_onchannelcallback(void *context)
+{
+ struct vmbus_channel *channel = context;
+ u8 *buf;
+ u32 buflen, recvlen;
+ u64 requestid;
+ u8 execute_shutdown = false;
+
+ struct shutdown_msg_data *shutdown_msg;
+
+ struct icmsg_hdr *icmsghdrp;
+ struct icmsg_negotiate *negop = NULL;
+
+ buflen = PAGE_SIZE;
+ buf = kmalloc(buflen, GFP_ATOMIC);
+
+ vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
+
+ if (recvlen 0) {
+ DPRINT_DBG(VMBUS, shutdown packet: len=%d, requestid=%lld,
+ recvlen, requestid);
+
+ icmsghdrp = (struct icmsg_hdr *)buf[
+ sizeof(struct vmbuspipe_hdr)];
+
+ if (icmsghdrp-icmsgtype == ICMSGTYPE_NEGOTIATE) {
+ prep_negotiate_resp(icmsghdrp, negop, buf);
+ } else {
+ shutdown_msg = (struct shutdown_msg_data *)buf[
+ sizeof(struct vmbuspipe_hdr) +
+ sizeof(struct icmsg_hdr)];
+
+ switch (shutdown_msg-flags) {
+ case 0:
+ case 1:
+ icmsghdrp-status = HV_S_OK;
+ execute_shutdown = true;
+
+ DPRINT_INFO(VMBUS, Shutdown request received -
+gracefull shutdown initiated);
+ break;
+ default:
+ icmsghdrp-status = HV_E_FAIL;
+ execute_shutdown = false;
+
+ DPRINT_INFO(VMBUS, Shutdown request received -
+Invalid request);
+ break;
+ };
+ }
+
+ icmsghdrp-icflags = ICMSGHDRFLAG_TRANSACTION
+ | ICMSGHDRFLAG_RESPONSE;
+
+ vmbus_sendpacket(channel, buf,
+ recvlen, requestid,
+ VmbusPacketTypeDataInBand, 0);
+ }
+
+ kfree(buf);
+
+ if (execute_shutdown == true)
+ orderly_poweroff(false);
+}
+
+/*
+ * Set guest time to host UTC time.
+ */
+static inline void do_adj_guesttime(u64 hosttime)
+{
+ s64 host_tns;
+ struct timespec host_ts;
+
+ host_tns = (hosttime - WLTIMEDELTA) * 100;
+ host_ts = ns_to_timespec(host_tns);
+
+ do_settimeofday(host_ts);
+}
+
+/*
+ * Synchronize
[PATCH 3/3]: An implementation of HyperV KVP functionality
This patch is re-based on the latest linux-next tree.
From: K. Y. Srinivasan ksriniva...@novell.com
Subject: An implementation of key/value pair feature (KVP) for Linux
on HyperV.
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
Index: linux.trees.git/drivers/staging/hv/Makefile
===
--- linux.trees.git.orig/drivers/staging/hv/Makefile2010-12-07
07:04:48.0 -0500
+++ linux.trees.git/drivers/staging/hv/Makefile 2010-12-07 07:59:46.0
-0500
@@ -10,4 +10,4 @@ hv_vmbus-y := vmbus_drv.o osd.o \
hv_storvsc-y := storvsc_drv.o storvsc.o
hv_blkvsc-y := blkvsc_drv.o blkvsc.o
hv_netvsc-y := netvsc_drv.o netvsc.o rndis_filter.o
-hv_utils-y := hv_util.o
+hv_utils-y := hv_util.o hv_kvp.o
Index: linux.trees.git/drivers/staging/hv/utils.h
===
--- linux.trees.git.orig/drivers/staging/hv/utils.h 2010-12-07
06:55:53.0 -0500
+++ linux.trees.git/drivers/staging/hv/utils.h 2010-12-07 07:05:39.0
-0500
@@ -102,6 +102,7 @@ struct ictimesync_data{
#define HV_SHUTDOWN_MSG0
#define HV_TIMESYNC_MSG1
#define HV_HEARTBEAT_MSG 2
+#define HV_KVP_MSG 3
struct hyperv_service_callback {
u8 msg_type;
Index: linux.trees.git/drivers/staging/hv/hv_kvp.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ linux.trees.git/drivers/staging/hv/hv_kvp.c 2010-12-07 07:05:39.0
-0500
@@ -0,0 +1,356 @@
+/*
+ * An implementation of key value pair (KVP) functionality for Linux.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan ksriniva...@novell.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+
+#include linux/net.h
+#include linux/nls.h
+#include linux/connector.h
+#include linux/workqueue.h
+
+#include logging.h
+#include osd.h
+#include vmbus.h
+#include vmbus_packet_format.h
+#include vmbus_channel_interface.h
+#include version_info.h
+#include channel.h
+#include vmbus_private.h
+#include vmbus_api.h
+#include utils.h
+#include hv_kvp.h
+
+
+
+/*
+ * Global state maintained for transaction that is being processed.
+ * Note that only one transaction can be active at any point in time.
+ *
+ * This state is set when we receive a request from the host; we
+ * cleanup this state when the transaction is completed - when we respond
+ * to the host with the key value.
+ */
+
+static struct {
+ bool active; /* transaction status - active or not */
+ u8 *recv_buffer; /* the receive buffer that we allocated */
+ int recv_len; /* number of bytes received. */
+ struct vmbus_channel *recv_channel; /* chn we got the request */
+ u64 recv_req_id; /* request ID. */
+} kvp_transaction;
+
+static int kvp_send_key(int index);
+
+static void kvp_respond_to_host(char *key, char *value, int error);
+static void kvp_work_func(struct work_struct *dummy);
+static void kvp_register(void);
+
+static DECLARE_DELAYED_WORK(kvp_work, kvp_work_func);
+
+static struct cb_id kvp_id = { CN_KVP_IDX, CN_KVP_VAL };
+static const char kvp_name[] = kvp_kernel_module;
+static int timeout_fired;
+
+/*
+ * Register the kernel component with the user-level daemon.
+ * As part of this registration, pass the LIC version number.
+ */
+
+static void
+kvp_register(void)
+{
+
+ struct cn_msg *msg;
+
+ msg = kzalloc(sizeof(*msg) + strlen(HV_DRV_VERSION) + 1 , GFP_ATOMIC);
+
+ if (msg) {
+ msg-id.idx = CN_KVP_IDX;
+ msg-id.val = CN_KVP_VAL;
+ msg-seq = KVP_REGISTER;
+ strcpy(msg-data, HV_DRV_VERSION);
+ msg-len = strlen(HV_DRV_VERSION) + 1;
+ cn_netlink_send(msg, 0, GFP_ATOMIC);
+ kfree(msg);
+ }
+}
+static void
+kvp_work_func(struct work_struct *dummy)
+{
+ /*
+* If the timer fires, the user-mode component has not responded;
+* process the pending transaction.
+*/
+ kvp_respond_to_host(Unknown key, Guest timed out, timeout_fired);
+ timeout_fired = 1;
+}
+
+/*
+ * Callback when data is received from user mode.
+ */
+
+static void
+kvp_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
+{
+
[PATCH]: A daemon to support HyperV KVP functionality
From: K. Y. Srinivasan ksriniva...@novell.com
Subject: An implementation of key/value pair feature (KVP) for Linux
on HyperV.
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
Index: linux.trees.git/drivers/staging/hv/tools/hv_kvp_daemon.c
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ linux.trees.git/drivers/staging/hv/tools/hv_kvp_daemon.c2010-12-07
08:46:19.0 -0500
@@ -0,0 +1,470 @@
+/*
+ * An implementation of key value pair (KVP) functionality for Linux.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan ksriniva...@novell.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+
+#include sys/types.h
+#include sys/socket.h
+#include sys/poll.h
+#include sys/utsname.h
+#include linux/types.h
+#include stdio.h
+#include stdlib.h
+#include unistd.h
+#include string.h
+#include errno.h
+#include arpa/inet.h
+#include linux/connector.h
+#include linux/netlink.h
+#include sys/socket.h
+#include ifaddrs.h
+#include netdb.h
+#include syslog.h
+
+/*
+ * KYS: TODO. Need to register these in the kernel.
+ *
+ * The following definitions are shared with the in-kernel component; do not
+ * change any of this without making the corresponding changes in
+ * the KVP kernel component.
+ */
+#define CN_KVP_IDX 0x9 /* MSFT KVP functionality */
+#define CN_KVP_VAL 0x1 /* This supports queries from the kernel */
+#define CN_KVP_USER_VAL0x2 /* This supports queries from the
user */
+
+/*
+ * KVP protocol: The user mode component first registers with the
+ * the kernel component. Subsequently, the kernel component requests, data
+ * for the specified keys. In response to this message the user mode component
+ * fills in the value corresponding to the specified key. We overload the
+ * sequence field in the cn_msg header to define our KVP message types.
+ *
+ * We use this infrastructure for also supporting queries from user mode
+ * application for state that may be maintained in the KVP kernel component.
+ *
+ * XXXKYS: Have a shared header file between the user and kernel (TODO)
+ */
+
+enum kvp_op {
+ KVP_REGISTER = 0, /* Register the user mode component*/
+ KVP_KERNEL_GET, /*Kernel is requesting the value for the specified key*/
+ KVP_KERNEL_SET, /*Kernel is providing the value for the specified key*/
+ KVP_USER_GET, /*User is requesting the value for the specified key*/
+ KVP_USER_SET /*User is providing the value for the specified key*/
+};
+
+#define HV_KVP_EXCHANGE_MAX_KEY_SIZE 512
+#define HV_KVP_EXCHANGE_MAX_VALUE_SIZE 2048
+
+struct hv_ku_msg {
+ __u32 kvp_index;
+ __u8 kvp_key[HV_KVP_EXCHANGE_MAX_KEY_SIZE]; /* Key name */
+ __u8 kvp_value[HV_KVP_EXCHANGE_MAX_VALUE_SIZE]; /* Key value */
+};
+
+enum key_index {
+ FullyQualifiedDomainName = 0,
+ IntegrationServicesVersion, /*This key is serviced in the kernel*/
+ NetworkAddressIPv4,
+ NetworkAddressIPv6,
+ OSBuildNumber,
+ OSName,
+ OSMajorVersion,
+ OSMinorVersion,
+ OSVersion,
+ ProcessorArchitecture
+};
+
+/*
+ * End of shared definitions.
+ */
+
+static char kvp_send_buffer[4096];
+static char kvp_recv_buffer[4096];
+static struct sockaddr_nl addr;
+
+static char os_name[100];
+static char os_major[50];
+static char os_minor[50];
+static char processor_arch[50];
+static char os_build[100];
+static char *lic_version;
+
+void kvp_get_os_info(void)
+{
+ FILE*file;
+ char*eol;
+ struct utsname buf;
+
+ uname(buf);
+ strcpy(os_build, buf.release);
+ strcpy(processor_arch, buf.machine);
+
+ file = fopen(/etc/SuSE-release, r);
+ if (file != NULL)
+ goto kvp_osinfo_found;
+ file = fopen(/etc/redhat-release, r);
+ if (file != NULL)
+ goto kvp_osinfo_found;
+ /*
+* Add code for other supported platforms.
+*/
+
+ /*
+* We don't have information about the os.
+*/
+ strcpy(os_name, Linux);
+ strcpy(os_major, 0);
+ strcpy(os_minor, 0);
+ return;
+
+kvp_osinfo_found:
+ fgets(os_name, 99, file);
+ eol = index(os_name, '\n');
+ *eol = '\0';
+ fgets(os_major, 49,
Re: [PATCH 2/3]: An implementation of HyperV KVP functionality
On 12/7/2010 at 5:29 PM, in message 20101207222933.ga10...@ioremap.net,
Evgeniy Polyakov z...@ioremap.net wrote:
On Tue, Dec 07, 2010 at 03:25:56PM -0700, Ky Srinivasan
(ksriniva...@novell.com) wrote:
+static void shutdown_onchannelcallback(void *context)
+{
+struct vmbus_channel *channel = context;
+u8 *buf;
+u32 buflen, recvlen;
+u64 requestid;
+u8 execute_shutdown = false;
+
+struct shutdown_msg_data *shutdown_msg;
+
+struct icmsg_hdr *icmsghdrp;
+struct icmsg_negotiate *negop = NULL;
+
+buflen = PAGE_SIZE;
+buf = kmalloc(buflen, GFP_ATOMIC);
+
+vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
This did not change since previous review and this is wrong.
It is the right way to crash kernel.
I did not read further since this is a show-stopper imo.
Hank, do you want to respond to this comment.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3]: An implementation of HyperV KVP functionality
Care to fix this, and address the connector issues, and then resend them all? Evgeniy has acked the patch for adding a new connector index for implementing the KVP functionality. The concern Evgeniy had was with regards to the code currently in the upstream tree and Hank is planning to submit a patch to fix that issue. Regards, K. Y thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
[PATCH 1/4] Add a connector Index to support HyperV KVP functionality
From 5b7c16baf2f310a1e7e119c3df8fd976ca0e2f57 Mon Sep 17 00:00:00 2001 From: ksrinivasan ksriniva...@novell.com Date: Wed, 8 Dec 2010 09:35:13 -0700 Added a connector index to support key value/pair (KVP) functionality for Linux guests hosted on a HyperV platform. All KVP related data gathering will be done in a user-level daemon. The kernel component of KVP communicates with the data gathering daemon using this connector index. Signed-off-by: ksrinivasan ksriniva...@novell.com --- include/linux/connector.h |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/include/linux/connector.h b/include/linux/connector.h index 7e8ca75..2e9759c 100644 --- a/include/linux/connector.h +++ b/include/linux/connector.h @@ -42,8 +42,9 @@ #define CN_VAL_DM_USERSPACE_LOG0x1 #define CN_IDX_DRBD0x8 #define CN_VAL_DRBD0x1 +#define CN_KVP_IDX 0x9 /* HyperV KVP */ -#define CN_NETLINK_USERS 8 +#define CN_NETLINK_USERS 9 /* * Maximum connector's message size. -- 1.7.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
[PATCH 2/4] Rename the file hv_utils.c
From 453acddcf55f1e68c5baf9dc5e8f601d18de15b0 Mon Sep 17 00:00:00 2001
From: ksrinivasan ksriniva...@novell.com
Date: Wed, 8 Dec 2010 12:24:58 -0700
The hv_utils module will be composed of more than one file.
Rename hv_utils.c to accommodate this without changing the module name.
Signed-off-by: ksrinivasan ksriniva...@novell.com
---
drivers/staging/hv/Makefile |1 +
drivers/staging/hv/hv_util.c | 308 +
drivers/staging/hv/hv_utils.c | 308 -
3 files changed, 309 insertions(+), 308 deletions(-)
create mode 100644 drivers/staging/hv/hv_util.c
delete mode 100644 drivers/staging/hv/hv_utils.c
diff --git a/drivers/staging/hv/Makefile b/drivers/staging/hv/Makefile
index acd39bd..4c14138 100644
--- a/drivers/staging/hv/Makefile
+++ b/drivers/staging/hv/Makefile
@@ -10,3 +10,4 @@ hv_vmbus-y := vmbus_drv.o osd.o \
hv_storvsc-y := storvsc_drv.o storvsc.o
hv_blkvsc-y := blkvsc_drv.o blkvsc.o
hv_netvsc-y := netvsc_drv.o netvsc.o rndis_filter.o
+hv_utils-y := hv_util.o
diff --git a/drivers/staging/hv/hv_util.c b/drivers/staging/hv/hv_util.c
new file mode 100644
index 000..53e1e29
--- /dev/null
+++ b/drivers/staging/hv/hv_util.c
@@ -0,0 +1,308 @@
+/*
+ * Copyright (c) 2010, Microsoft Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * Authors:
+ * Haiyang Zhang haiya...@microsoft.com
+ * Hank Janssen hjans...@microsoft.com
+ */
+#include linux/kernel.h
+#include linux/init.h
+#include linux/module.h
+#include linux/slab.h
+#include linux/sysctl.h
+#include linux/reboot.h
+#include linux/dmi.h
+#include linux/pci.h
+
+#include logging.h
+#include osd.h
+#include vmbus.h
+#include vmbus_packet_format.h
+#include vmbus_channel_interface.h
+#include version_info.h
+#include channel.h
+#include vmbus_private.h
+#include vmbus_api.h
+#include utils.h
+
+
+static void shutdown_onchannelcallback(void *context)
+{
+ struct vmbus_channel *channel = context;
+ u8 *buf;
+ u32 buflen, recvlen;
+ u64 requestid;
+ u8 execute_shutdown = false;
+
+ struct shutdown_msg_data *shutdown_msg;
+
+ struct icmsg_hdr *icmsghdrp;
+ struct icmsg_negotiate *negop = NULL;
+
+ buflen = PAGE_SIZE;
+ buf = kmalloc(buflen, GFP_ATOMIC);
+
+ vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
+
+ if (recvlen 0) {
+ DPRINT_DBG(VMBUS, shutdown packet: len=%d, requestid=%lld,
+ recvlen, requestid);
+
+ icmsghdrp = (struct icmsg_hdr *)buf[
+ sizeof(struct vmbuspipe_hdr)];
+
+ if (icmsghdrp-icmsgtype == ICMSGTYPE_NEGOTIATE) {
+ prep_negotiate_resp(icmsghdrp, negop, buf);
+ } else {
+ shutdown_msg = (struct shutdown_msg_data *)buf[
+ sizeof(struct vmbuspipe_hdr) +
+ sizeof(struct icmsg_hdr)];
+
+ switch (shutdown_msg-flags) {
+ case 0:
+ case 1:
+ icmsghdrp-status = HV_S_OK;
+ execute_shutdown = true;
+
+ DPRINT_INFO(VMBUS, Shutdown request received -
+gracefull shutdown initiated);
+ break;
+ default:
+ icmsghdrp-status = HV_E_FAIL;
+ execute_shutdown = false;
+
+ DPRINT_INFO(VMBUS, Shutdown request received -
+Invalid request);
+ break;
+ };
+ }
+
+ icmsghdrp-icflags = ICMSGHDRFLAG_TRANSACTION
+ | ICMSGHDRFLAG_RESPONSE;
+
+ vmbus_sendpacket(channel, buf,
+ recvlen, requestid,
+ VmbusPacketTypeDataInBand, 0);
+ }
+
+ kfree(buf);
+
+ if (execute_shutdown == true)
+ orderly_poweroff(false);
+}
+
+/*
+ * Set guest time to host UTC time.
+ */
+static inline void do_adj_guesttime(u64 hosttime)
+{
+ s64 host_tns;
+ struct timespec
[PATCH 3/4] An Implementation of KVP functionality
From 2843393e8f50421e81e033806cd121cfb8cc7b6a Mon Sep 17 00:00:00 2001
From: ksrinivasan ksriniva...@novell.com
Date: Wed, 8 Dec 2010 13:46:46 -0700
This is an implementation of the key/value pair (KVP) functionality
for Linux guests hosted on HyperV. All guest specific data gathering for
KVP will be implemented in a user-level daemon. This kernel component packaged
as part of the hv_utils driver supports communication with the HyperV
host.
Signed-off-by: ksrinivasan ksriniva...@novell.com
---
drivers/staging/hv/Makefile |2 +-
drivers/staging/hv/channel_mgmt.c | 23 +++-
drivers/staging/hv/hv_kvp.c | 346 +
drivers/staging/hv/hv_kvp.h | 175 +++
drivers/staging/hv/hv_util.c | 17 ++
drivers/staging/hv/utils.h|1 +
6 files changed, 561 insertions(+), 3 deletions(-)
create mode 100644 drivers/staging/hv/hv_kvp.c
create mode 100644 drivers/staging/hv/hv_kvp.h
diff --git a/drivers/staging/hv/Makefile b/drivers/staging/hv/Makefile
index 4c14138..606ce7d 100644
--- a/drivers/staging/hv/Makefile
+++ b/drivers/staging/hv/Makefile
@@ -10,4 +10,4 @@ hv_vmbus-y := vmbus_drv.o osd.o \
hv_storvsc-y := storvsc_drv.o storvsc.o
hv_blkvsc-y := blkvsc_drv.o blkvsc.o
hv_netvsc-y := netvsc_drv.o netvsc.o rndis_filter.o
-hv_utils-y := hv_util.o
+hv_utils-y := hv_util.o hv_kvp.o
diff --git a/drivers/staging/hv/channel_mgmt.c
b/drivers/staging/hv/channel_mgmt.c
index 0f4d609..375f164 100644
--- a/drivers/staging/hv/channel_mgmt.c
+++ b/drivers/staging/hv/channel_mgmt.c
@@ -34,8 +34,8 @@ struct vmbus_channel_message_table_entry {
void (*messageHandler)(struct vmbus_channel_message_header *msg);
};
-#define MAX_MSG_TYPES3
-#define MAX_NUM_DEVICE_CLASSES_SUPPORTED 7
+#define MAX_MSG_TYPES4
+#define MAX_NUM_DEVICE_CLASSES_SUPPORTED 8
static const struct hv_guid
gSupportedDeviceClasses[MAX_NUM_DEVICE_CLASSES_SUPPORTED] = {
@@ -98,6 +98,15 @@ static const struct hv_guid
0xab, 0x55, 0x38, 0x2f, 0x3b, 0xd5, 0x42, 0x2d
}
},
+ /* {A9A0F4E7-5A45-4d96-B827-8A841E8C03E6} */
+ /* KVP */
+ {
+ .data = {
+ 0xe7, 0xf4, 0xa0, 0xa9, 0x45, 0x5a, 0x96, 0x4d,
+ 0xb8, 0x27, 0x8a, 0x84, 0x1e, 0x8c, 0x3, 0xe6
+ }
+ },
+
};
@@ -231,6 +240,16 @@ struct hyperv_service_callback hv_cb_utils[MAX_MSG_TYPES]
= {
.callback = chn_cb_negotiate,
.log_msg = Heartbeat channel functionality initialized
},
+ /* {A9A0F4E7-5A45-4d96-B827-8A841E8C03E6} */
+ /* KVP */
+ {
+ .data = {
+ 0xe7, 0xf4, 0xa0, 0xa9, 0x45, 0x5a, 0x96, 0x4d,
+ 0xb8, 0x27, 0x8a, 0x84, 0x1e, 0x8c, 0x3, 0xe6
+ },
+ .callback = chn_cb_negotiate,
+ .log_msg = KVP channel functionality initialized
+ },
};
EXPORT_SYMBOL(hv_cb_utils);
diff --git a/drivers/staging/hv/hv_kvp.c b/drivers/staging/hv/hv_kvp.c
new file mode 100644
index 000..16a95ea
--- /dev/null
+++ b/drivers/staging/hv/hv_kvp.c
@@ -0,0 +1,346 @@
+/*
+ * An implementation of key value pair (KVP) functionality for Linux.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan ksriniva...@novell.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ */
+
+
+#include linux/net.h
+#include linux/nls.h
+#include linux/connector.h
+#include linux/workqueue.h
+
+#include logging.h
+#include osd.h
+#include vmbus.h
+#include vmbus_packet_format.h
+#include vmbus_channel_interface.h
+#include version_info.h
+#include channel.h
+#include vmbus_private.h
+#include vmbus_api.h
+#include utils.h
+#include hv_kvp.h
+
+
+
+/*
+ * Global state maintained for transaction that is being processed.
+ * Note that only one transaction can be active at any point in time.
+ *
+ * This state is set when we receive a request from the host; we
+ * cleanup this state when the transaction is completed - when we respond
+ * to the host with the key value.
+ */
+
+static struct {
+ bool active; /* transaction status - active or not */
+ u8 *recv_buffer; /* the receive buffer that we allocated */
+ int recv_len; /* number of bytes received. */
+ struct vmbus_channel *recv_channel; /* chn we got the request */
+ u64 recv_req_id; /* request ID. */
+} kvp_transaction;
+
+static int kvp_send_key(int index);
+
+static void kvp_respond_to_host(char *key, char *value, int error);
+static void kvp_work_func(struct work_struct *dummy);
+static void kvp_register(void);
+
+static DECLARE_DELAYED_WORK(kvp_work, kvp_work_func);
+
+static struct cb_id kvp_id = { CN_KVP_IDX, CN_KVP_VAL };
+static const char
[PATCH 4/4] A daemon to gather guest specific information for KVP
From 718eed47e4c2eb740dc04a6729c8853424ac6965 Mon Sep 17 00:00:00 2001
From: ksrinivasan ksriniva...@novell.com
Date: Wed, 8 Dec 2010 14:10:35 -0700
This daemon gathers all the guest specific information needed to support the
HyperV KVP functionality. This daemon communicates with the kernel
component via a netlink channel.
Signed-off-by: ksrinivasan ksriniva...@novell.com
---
drivers/staging/hv/tools/hv_kvp_daemon.c | 470 ++
1 files changed, 470 insertions(+), 0 deletions(-)
create mode 100644 drivers/staging/hv/tools/hv_kvp_daemon.c
diff --git a/drivers/staging/hv/tools/hv_kvp_daemon.c
b/drivers/staging/hv/tools/hv_kvp_daemon.c
new file mode 100644
index 000..f5a2dd6
--- /dev/null
+++ b/drivers/staging/hv/tools/hv_kvp_daemon.c
@@ -0,0 +1,470 @@
+/*
+ * An implementation of key value pair (KVP) functionality for Linux.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan ksriniva...@novell.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+
+#include sys/types.h
+#include sys/socket.h
+#include sys/poll.h
+#include sys/utsname.h
+#include linux/types.h
+#include stdio.h
+#include stdlib.h
+#include unistd.h
+#include string.h
+#include errno.h
+#include arpa/inet.h
+#include linux/connector.h
+#include linux/netlink.h
+#include sys/socket.h
+#include ifaddrs.h
+#include netdb.h
+#include syslog.h
+
+/*
+ * KYS: TODO. Need to register these in the kernel.
+ *
+ * The following definitions are shared with the in-kernel component; do not
+ * change any of this without making the corresponding changes in
+ * the KVP kernel component.
+ */
+#define CN_KVP_IDX 0x9 /* MSFT KVP functionality */
+#define CN_KVP_VAL 0x1 /* This supports queries from the kernel */
+#define CN_KVP_USER_VAL0x2 /* This supports queries from the
user */
+
+/*
+ * KVP protocol: The user mode component first registers with the
+ * the kernel component. Subsequently, the kernel component requests, data
+ * for the specified keys. In response to this message the user mode component
+ * fills in the value corresponding to the specified key. We overload the
+ * sequence field in the cn_msg header to define our KVP message types.
+ *
+ * We use this infrastructure for also supporting queries from user mode
+ * application for state that may be maintained in the KVP kernel component.
+ *
+ * XXXKYS: Have a shared header file between the user and kernel (TODO)
+ */
+
+enum kvp_op {
+ KVP_REGISTER = 0, /* Register the user mode component*/
+ KVP_KERNEL_GET, /*Kernel is requesting the value for the specified key*/
+ KVP_KERNEL_SET, /*Kernel is providing the value for the specified key*/
+ KVP_USER_GET, /*User is requesting the value for the specified key*/
+ KVP_USER_SET /*User is providing the value for the specified key*/
+};
+
+#define HV_KVP_EXCHANGE_MAX_KEY_SIZE 512
+#define HV_KVP_EXCHANGE_MAX_VALUE_SIZE 2048
+
+struct hv_ku_msg {
+ __u32 kvp_index;
+ __u8 kvp_key[HV_KVP_EXCHANGE_MAX_KEY_SIZE]; /* Key name */
+ __u8 kvp_value[HV_KVP_EXCHANGE_MAX_VALUE_SIZE]; /* Key value */
+};
+
+enum key_index {
+ FullyQualifiedDomainName = 0,
+ IntegrationServicesVersion, /*This key is serviced in the kernel*/
+ NetworkAddressIPv4,
+ NetworkAddressIPv6,
+ OSBuildNumber,
+ OSName,
+ OSMajorVersion,
+ OSMinorVersion,
+ OSVersion,
+ ProcessorArchitecture
+};
+
+/*
+ * End of shared definitions.
+ */
+
+static char kvp_send_buffer[4096];
+static char kvp_recv_buffer[4096];
+static struct sockaddr_nl addr;
+
+static char os_name[100];
+static char os_major[50];
+static char os_minor[50];
+static char processor_arch[50];
+static char os_build[100];
+static char *lic_version;
+
+void kvp_get_os_info(void)
+{
+ FILE*file;
+ char*eol;
+ struct utsname buf;
+
+ uname(buf);
+ strcpy(os_build, buf.release);
+ strcpy(processor_arch, buf.machine);
+
+ file = fopen(/etc/SuSE-release, r);
+ if (file != NULL)
+ goto kvp_osinfo_found;
+ file = fopen(/etc/redhat-release, r);
+ if (file != NULL)
+ goto kvp_osinfo_found;
+ /*
+* Add code for other supported platforms.
+*/
+
+ /*
+
Re: [PATCH 2/4] Rename the file hv_utils.c
On 12/8/2010 at 5:34 PM, in message 20101208223400.gb6...@suse.de, Greg KH gre...@suse.de wrote: For the subject on the rest of the patches they would look like: Subject: [PATCH 2/4] Staging: hv: rename hv_utils.c to hv_util.c Note the Staging: hv: prefix. On Wed, Dec 08, 2010 at 03:26:10PM -0700, Ky Srinivasan wrote: From 453acddcf55f1e68c5baf9dc5e8f601d18de15b0 Mon Sep 17 00:00:00 2001 From: ksrinivasan ksriniva...@novell.com Date: Wed, 8 Dec 2010 12:24:58 -0700 The hv_utils module will be composed of more than one file. Rename hv_utils.c to accommodate this without changing the module name. Signed-off-by: ksrinivasan ksriniva...@novell.com Again, use your full name. I will resend the patch. Thanks, K. Y thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/4] Add a connector Index to support HyperV KVP functionality
On 12/8/2010 at 5:32 PM, in message 20101208223259.ga6...@suse.de, Greg KH gre...@suse.de wrote: Please make your subject a bit more descriptive, for this one, it would be: Subject: [PATCH 1/4] Connector: add index to support HyperV And what is a KVP? On Wed, Dec 08, 2010 at 03:20:59PM -0700, Ky Srinivasan wrote: From 5b7c16baf2f310a1e7e119c3df8fd976ca0e2f57 Mon Sep 17 00:00:00 2001 From: ksrinivasan ksriniva...@novell.com I doubt that's your full name you want as the changelog author information :) Date: Wed, 8 Dec 2010 09:35:13 -0700 Why is this in the body of your message? Added a connector index to support key value/pair (KVP) functionality for Linux guests hosted on a HyperV platform. All KVP related data gathering will be done in a user-level daemon. The kernel component of KVP communicates with the data gathering daemon using this connector index. Signed-off-by: ksrinivasan ksriniva...@novell.com Same error here with your name :( Third time's a charm? I hope so! I will resend all the patches. Thanks, K. Y greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/4] Rename the file hv_utils.c
On 12/8/2010 at 5:55 PM, in message 20101208225524.ga11...@ioremap.net,
Evgeniy Polyakov z...@ioremap.net wrote:
You must be kidding.
The same in hv_kvp_onchannelcallback() and probably anywhere else.
Hank, if it is ok with you, I will fix this issue across the board and submit
that as part of this patch set.
Regards,
K. Y
On Wed, Dec 08, 2010 at 03:26:10PM -0700, Ky Srinivasan
(ksriniva...@novell.com) wrote:
+static void shutdown_onchannelcallback(void *context)
+{
+struct vmbus_channel *channel = context;
+u8 *buf;
+u32 buflen, recvlen;
+u64 requestid;
+u8 execute_shutdown = false;
+
+struct shutdown_msg_data *shutdown_msg;
+
+struct icmsg_hdr *icmsghdrp;
+struct icmsg_negotiate *negop = NULL;
+
+buflen = PAGE_SIZE;
+buf = kmalloc(buflen, GFP_ATOMIC);
+
+vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
+
+static void timesync_onchannelcallback(void *context)
+{
+struct vmbus_channel *channel = context;
+u8 *buf;
+u32 buflen, recvlen;
+u64 requestid;
+struct icmsg_hdr *icmsghdrp;
+struct ictimesync_data *timedatap;
+
+buflen = PAGE_SIZE;
+buf = kmalloc(buflen, GFP_ATOMIC);
+
+vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
+static void heartbeat_onchannelcallback(void *context)
+{
+struct vmbus_channel *channel = context;
+u8 *buf;
+u32 buflen, recvlen;
+u64 requestid;
+struct icmsg_hdr *icmsghdrp;
+struct heartbeat_msg_data *heartbeat_msg;
+
+buflen = PAGE_SIZE;
+buf = kmalloc(buflen, GFP_ATOMIC);
+
+vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/1] Properly check return values of kmalloc and vmbus_recvpacket
On 12/9/2010 at 3:23 PM, in message
1291926209-17120-1-git-send-email-hjans...@microsoft.com, Hank Janssen
hjans...@microsoft.com wrote:
Correct ugly oversight, we need to check the return values of kmalloc
and vmbus_recvpacket and return if they fail. I also tightened up the
call to kmalloc.
Thanks to Evgeniy Polyakov z...@ioremap.net for pointing this out.
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/hv_utils.c | 48
1 files changed, 33 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index 53e1e29..ac68575 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -43,21 +43,27 @@ static void shutdown_onchannelcallback(void *context)
{
struct vmbus_channel *channel = context;
u8 *buf;
- u32 buflen, recvlen;
+ u32 recvlen;
u64 requestid;
u8 execute_shutdown = false;
+ int ret = 0;
struct shutdown_msg_data *shutdown_msg;
struct icmsg_hdr *icmsghdrp;
struct icmsg_negotiate *negop = NULL;
- buflen = PAGE_SIZE;
- buf = kmalloc(buflen, GFP_ATOMIC);
+ buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
- vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
+ if (!buf) {
+ printk(KERN_INFO
+Unable to allocate memory for
shutdown_onchannelcallback);
+ return;
+ }
+
+ ret = vmbus_recvpacket(channel, buf, PAGE_SIZE, recvlen, requestid);
- if (recvlen 0) {
+ if (ret == 0 recvlen 0) {
DPRINT_DBG(VMBUS, shutdown packet: len=%d, requestid=%lld,
recvlen, requestid);
@@ -151,17 +157,23 @@ static void timesync_onchannelcallback(void *context)
{
struct vmbus_channel *channel = context;
u8 *buf;
- u32 buflen, recvlen;
+ u32 recvlen;
u64 requestid;
struct icmsg_hdr *icmsghdrp;
struct ictimesync_data *timedatap;
+ int ret = 0;
- buflen = PAGE_SIZE;
- buf = kmalloc(buflen, GFP_ATOMIC);
+ buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
- vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
+ if (!buf) {
+ printk(KERN_INFO
+Unable to allocate memory for
timesync_onchannelcallback);
+ return;
+ }
- if (recvlen 0) {
+ ret = vmbus_recvpacket(channel, buf, PAGE_SIZE, recvlen, requestid);
+
+ if (ret == 0 recvlen 0) {
DPRINT_DBG(VMBUS, timesync packet: recvlen=%d, requestid=%lld,
recvlen, requestid);
@@ -197,17 +209,23 @@ static void heartbeat_onchannelcallback(void *context)
{
struct vmbus_channel *channel = context;
u8 *buf;
- u32 buflen, recvlen;
+ u32 recvlen;
u64 requestid;
struct icmsg_hdr *icmsghdrp;
struct heartbeat_msg_data *heartbeat_msg;
+ int ret = 0;
+
+ buf = kmalloc(PAGE_SIZE, GFP_ATOMIC);
- buflen = PAGE_SIZE;
- buf = kmalloc(buflen, GFP_ATOMIC);
+ if (!buf) {
+ printk(KERN_INFO
+ Unable to allocate memory for
heartbeat_onchannelcallback);
+ return;
+ }
- vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
+ ret = vmbus_recvpacket(channel, buf, PAGE_SIZE, recvlen, requestid);
- if (recvlen 0) {
+ if (ret == 0 recvlen 0) {
DPRINT_DBG(VMBUS, heartbeat packet: len=%d, requestid=%lld,
recvlen, requestid);
I am wondering if it might be better to allocate a page during module startup
for dealing with some of these services. Since the protocol guarantees that
there cannot be more than one outstanding request from the host side, having a
pre-allocated receive buffer would be a more robust solution - we don't have to
allocate memory when we cannot afford to sleep and thereby don't have to deal
with a class of failure conditions that are not easy to deal with. For instance
not being able to shut the guest down because of low memory situation would be
bad.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/1] hv: Use only one receive buffer per channel and kmalloc on initialize
On 12/13/2010 at 3:34 PM, in message
1292272498-29483-1-git-send-email-hjans...@microsoft.com, Hank Janssen
hjans...@microsoft.com wrote:
Correct issue with not checking kmalloc return value.
This fix now only uses one receive buffer for all hv_utils
channels, and will do only one kmalloc on init and will return
with a -ENOMEM if kmalloc fails on initialize.
Thanks to Evgeniy Polyakov z...@ioremap.net for pointing this out.
And thanks to Jesper Juhl j...@chaosbits.net and Ky Srinivasan
ksriniva...@novell.com for suggesting a better implementation of
my original patch.
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
Cc: Evgeniy Polyakov z...@ioremap.net
Cc: Jesper Juhl j...@chaosbits.net
Cc: Ky Srinivasan ksriniva...@novell.com
---
drivers/staging/hv/hv_utils.c | 84 +---
1 files changed, 44 insertions(+), 40 deletions(-)
diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index 53e1e29..e0ecc23 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -38,12 +38,14 @@
#include vmbus_api.h
#include utils.h
+static u8 *shut_txf_buf;
+static u8 *time_txf_buf;
+static u8 *hbeat_txf_buf;
static void shutdown_onchannelcallback(void *context)
{
struct vmbus_channel *channel = context;
- u8 *buf;
- u32 buflen, recvlen;
+ u32 recvlen;
u64 requestid;
u8 execute_shutdown = false;
@@ -52,24 +54,23 @@ static void shutdown_onchannelcallback(void *context)
struct icmsg_hdr *icmsghdrp;
struct icmsg_negotiate *negop = NULL;
- buflen = PAGE_SIZE;
- buf = kmalloc(buflen, GFP_ATOMIC);
-
- vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
+ vmbus_recvpacket(channel, shut_txf_buf,
+ PAGE_SIZE, recvlen, requestid);
if (recvlen 0) {
DPRINT_DBG(VMBUS, shutdown packet: len=%d, requestid=%lld,
recvlen, requestid);
- icmsghdrp = (struct icmsg_hdr *)buf[
+ icmsghdrp = (struct icmsg_hdr *)shut_txf_buf[
sizeof(struct vmbuspipe_hdr)];
if (icmsghdrp-icmsgtype == ICMSGTYPE_NEGOTIATE) {
- prep_negotiate_resp(icmsghdrp, negop, buf);
+ prep_negotiate_resp(icmsghdrp, negop, shut_txf_buf);
} else {
- shutdown_msg = (struct shutdown_msg_data *)buf[
- sizeof(struct vmbuspipe_hdr) +
- sizeof(struct icmsg_hdr)];
+ shutdown_msg =
+ (struct shutdown_msg_data *)shut_txf_buf[
+ sizeof(struct vmbuspipe_hdr) +
+ sizeof(struct icmsg_hdr)];
switch (shutdown_msg-flags) {
case 0:
@@ -93,13 +94,11 @@ static void shutdown_onchannelcallback(void *context)
icmsghdrp-icflags = ICMSGHDRFLAG_TRANSACTION
| ICMSGHDRFLAG_RESPONSE;
- vmbus_sendpacket(channel, buf,
+ vmbus_sendpacket(channel, shut_txf_buf,
recvlen, requestid,
VmbusPacketTypeDataInBand, 0);
}
- kfree(buf);
-
if (execute_shutdown == true)
orderly_poweroff(false);
}
@@ -150,28 +149,25 @@ static inline void adj_guesttime(u64 hosttime, u8
flags)
static void timesync_onchannelcallback(void *context)
{
struct vmbus_channel *channel = context;
- u8 *buf;
- u32 buflen, recvlen;
+ u32 recvlen;
u64 requestid;
struct icmsg_hdr *icmsghdrp;
struct ictimesync_data *timedatap;
- buflen = PAGE_SIZE;
- buf = kmalloc(buflen, GFP_ATOMIC);
-
- vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
+ vmbus_recvpacket(channel, time_txf_buf,
+ PAGE_SIZE, recvlen, requestid);
if (recvlen 0) {
DPRINT_DBG(VMBUS, timesync packet: recvlen=%d, requestid=%lld,
recvlen, requestid);
- icmsghdrp = (struct icmsg_hdr *)buf[
+ icmsghdrp = (struct icmsg_hdr *)time_txf_buf[
sizeof(struct vmbuspipe_hdr)];
if (icmsghdrp-icmsgtype == ICMSGTYPE_NEGOTIATE) {
- prep_negotiate_resp(icmsghdrp, NULL, buf);
+ prep_negotiate_resp(icmsghdrp, NULL, time_txf_buf);
} else {
- timedatap = (struct ictimesync_data *)buf[
+ timedatap = (struct ictimesync_data *)time_txf_buf[
sizeof(struct vmbuspipe_hdr) +
sizeof(struct icmsg_hdr)];
adj_guesttime
Re: [PATCH 1/1] hv: Use only one receive buffer per channel and kmalloc on initialize
On 12/13/2010 at 3:34 PM, in message
1292272498-29483-1-git-send-email-hjans...@microsoft.com, Hank Janssen
hjans...@microsoft.com wrote:
Correct issue with not checking kmalloc return value.
This fix now only uses one receive buffer for all hv_utils
channels, and will do only one kmalloc on init and will return
with a -ENOMEM if kmalloc fails on initialize.
Thanks to Evgeniy Polyakov z...@ioremap.net for pointing this out.
And thanks to Jesper Juhl j...@chaosbits.net and Ky Srinivasan
ksriniva...@novell.com for suggesting a better implementation of
my original patch.
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
Cc: Evgeniy Polyakov z...@ioremap.net
Cc: Jesper Juhl j...@chaosbits.net
Cc: Ky Srinivasan ksriniva...@novell.com
---
drivers/staging/hv/hv_utils.c | 84 +---
1 files changed, 44 insertions(+), 40 deletions(-)
diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index 53e1e29..e0ecc23 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -38,12 +38,14 @@
#include vmbus_api.h
#include utils.h
+static u8 *shut_txf_buf;
+static u8 *time_txf_buf;
+static u8 *hbeat_txf_buf;
static void shutdown_onchannelcallback(void *context)
{
struct vmbus_channel *channel = context;
- u8 *buf;
- u32 buflen, recvlen;
+ u32 recvlen;
u64 requestid;
u8 execute_shutdown = false;
@@ -52,24 +54,23 @@ static void shutdown_onchannelcallback(void *context)
struct icmsg_hdr *icmsghdrp;
struct icmsg_negotiate *negop = NULL;
- buflen = PAGE_SIZE;
- buf = kmalloc(buflen, GFP_ATOMIC);
-
- vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
+ vmbus_recvpacket(channel, shut_txf_buf,
+ PAGE_SIZE, recvlen, requestid);
Even if vmbus_recvpacket were to fail, why don't we just shut the guest down;
after all we already know that is the intent of the host.
Regards,
K. Y
if (recvlen 0) {
DPRINT_DBG(VMBUS, shutdown packet: len=%d, requestid=%lld,
recvlen, requestid);
- icmsghdrp = (struct icmsg_hdr *)buf[
+ icmsghdrp = (struct icmsg_hdr *)shut_txf_buf[
sizeof(struct vmbuspipe_hdr)];
if (icmsghdrp-icmsgtype == ICMSGTYPE_NEGOTIATE) {
- prep_negotiate_resp(icmsghdrp, negop, buf);
+ prep_negotiate_resp(icmsghdrp, negop, shut_txf_buf);
} else {
- shutdown_msg = (struct shutdown_msg_data *)buf[
- sizeof(struct vmbuspipe_hdr) +
- sizeof(struct icmsg_hdr)];
+ shutdown_msg =
+ (struct shutdown_msg_data *)shut_txf_buf[
+ sizeof(struct vmbuspipe_hdr) +
+ sizeof(struct icmsg_hdr)];
switch (shutdown_msg-flags) {
case 0:
@@ -93,13 +94,11 @@ static void shutdown_onchannelcallback(void *context)
icmsghdrp-icflags = ICMSGHDRFLAG_TRANSACTION
| ICMSGHDRFLAG_RESPONSE;
- vmbus_sendpacket(channel, buf,
+ vmbus_sendpacket(channel, shut_txf_buf,
recvlen, requestid,
VmbusPacketTypeDataInBand, 0);
}
- kfree(buf);
-
if (execute_shutdown == true)
orderly_poweroff(false);
}
@@ -150,28 +149,25 @@ static inline void adj_guesttime(u64 hosttime, u8
flags)
static void timesync_onchannelcallback(void *context)
{
struct vmbus_channel *channel = context;
- u8 *buf;
- u32 buflen, recvlen;
+ u32 recvlen;
u64 requestid;
struct icmsg_hdr *icmsghdrp;
struct ictimesync_data *timedatap;
- buflen = PAGE_SIZE;
- buf = kmalloc(buflen, GFP_ATOMIC);
-
- vmbus_recvpacket(channel, buf, buflen, recvlen, requestid);
+ vmbus_recvpacket(channel, time_txf_buf,
+ PAGE_SIZE, recvlen, requestid);
if (recvlen 0) {
DPRINT_DBG(VMBUS, timesync packet: recvlen=%d, requestid=%lld,
recvlen, requestid);
- icmsghdrp = (struct icmsg_hdr *)buf[
+ icmsghdrp = (struct icmsg_hdr *)time_txf_buf[
sizeof(struct vmbuspipe_hdr)];
if (icmsghdrp-icmsgtype == ICMSGTYPE_NEGOTIATE) {
- prep_negotiate_resp(icmsghdrp, NULL, buf);
+ prep_negotiate_resp(icmsghdrp, NULL, time_txf_buf);
} else {
- timedatap = (struct ictimesync_data *)buf[
+ timedatap = (struct ictimesync_data *)time_txf_buf
[PATCH 2/4] Staging: hv: Rename hv_utils.c to hv_util.c
The hv_utils module will be composed of more than one file;
rename hv_utils.c to accommodate this without changing the module name.
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
---
drivers/staging/hv/Makefile |1 +
drivers/staging/hv/hv_util.c | 315 +
drivers/staging/hv/hv_utils.c | 315 -
3 files changed, 316 insertions(+), 315 deletions(-)
create mode 100644 drivers/staging/hv/hv_util.c
delete mode 100644 drivers/staging/hv/hv_utils.c
diff --git a/drivers/staging/hv/Makefile b/drivers/staging/hv/Makefile
index acd39bd..4c14138 100644
--- a/drivers/staging/hv/Makefile
+++ b/drivers/staging/hv/Makefile
@@ -10,3 +10,4 @@ hv_vmbus-y := vmbus_drv.o osd.o \
hv_storvsc-y := storvsc_drv.o storvsc.o
hv_blkvsc-y := blkvsc_drv.o blkvsc.o
hv_netvsc-y := netvsc_drv.o netvsc.o rndis_filter.o
+hv_utils-y := hv_util.o
diff --git a/drivers/staging/hv/hv_util.c b/drivers/staging/hv/hv_util.c
new file mode 100644
index 000..0074581
--- /dev/null
+++ b/drivers/staging/hv/hv_util.c
@@ -0,0 +1,315 @@
+/*
+ * Copyright (c) 2010, Microsoft Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * Authors:
+ * Haiyang Zhang haiya...@microsoft.com
+ * Hank Janssen hjans...@microsoft.com
+ */
+#include linux/kernel.h
+#include linux/init.h
+#include linux/module.h
+#include linux/slab.h
+#include linux/sysctl.h
+#include linux/reboot.h
+#include linux/dmi.h
+#include linux/pci.h
+
+#include logging.h
+#include osd.h
+#include vmbus.h
+#include vmbus_packet_format.h
+#include vmbus_channel_interface.h
+#include version_info.h
+#include channel.h
+#include vmbus_private.h
+#include vmbus_api.h
+#include utils.h
+
+static u8 *shut_txf_buf;
+static u8 *time_txf_buf;
+static u8 *hbeat_txf_buf;
+
+static void shutdown_onchannelcallback(void *context)
+{
+ struct vmbus_channel *channel = context;
+ u32 recvlen;
+ u64 requestid;
+ u8 execute_shutdown = false;
+
+ struct shutdown_msg_data *shutdown_msg;
+
+ struct icmsg_hdr *icmsghdrp;
+ struct icmsg_negotiate *negop = NULL;
+
+ vmbus_recvpacket(channel, shut_txf_buf,
+PAGE_SIZE, recvlen, requestid);
+
+ if (recvlen 0) {
+ DPRINT_DBG(VMBUS, shutdown packet: len=%d, requestid=%lld,
+ recvlen, requestid);
+
+ icmsghdrp = (struct icmsg_hdr *)shut_txf_buf[
+ sizeof(struct vmbuspipe_hdr)];
+
+ if (icmsghdrp-icmsgtype == ICMSGTYPE_NEGOTIATE) {
+ prep_negotiate_resp(icmsghdrp, negop, shut_txf_buf);
+ } else {
+ shutdown_msg =
+ (struct shutdown_msg_data *)shut_txf_buf[
+ sizeof(struct vmbuspipe_hdr) +
+ sizeof(struct icmsg_hdr)];
+
+ switch (shutdown_msg-flags) {
+ case 0:
+ case 1:
+ icmsghdrp-status = HV_S_OK;
+ execute_shutdown = true;
+
+ DPRINT_INFO(VMBUS, Shutdown request received -
+gracefull shutdown initiated);
+ break;
+ default:
+ icmsghdrp-status = HV_E_FAIL;
+ execute_shutdown = false;
+
+ DPRINT_INFO(VMBUS, Shutdown request received -
+Invalid request);
+ break;
+ };
+ }
+
+ icmsghdrp-icflags = ICMSGHDRFLAG_TRANSACTION
+ | ICMSGHDRFLAG_RESPONSE;
+
+ vmbus_sendpacket(channel, shut_txf_buf,
+ recvlen, requestid,
+ VmbusPacketTypeDataInBand, 0);
+ }
+
+ if (execute_shutdown == true)
+ orderly_poweroff(false);
+}
+
+/*
+ * Set guest time to host UTC time.
+ */
+static inline void do_adj_guesttime(u64 hosttime)
+{
+ s64 host_tns;
+ struct timespec host_ts;
+
+ host_tns = (hosttime - WLTIMEDELTA) * 100;
+
[PATCH 3/4] Staging: hv: Implement key/value pair (KVP)
This is an implementation of the key value/pair (KVP) functionality
for Linux guests hosted on HyperV. This component communicates
with the host to support the KVP functionality.
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
---
drivers/staging/hv/Makefile |2 +-
drivers/staging/hv/channel_mgmt.c | 23 +++-
drivers/staging/hv/hv_kvp.c | 346 +
drivers/staging/hv/hv_kvp.h | 184
drivers/staging/hv/hv_util.c | 14 ++
drivers/staging/hv/utils.h|1 +
6 files changed, 567 insertions(+), 3 deletions(-)
create mode 100644 drivers/staging/hv/hv_kvp.c
create mode 100644 drivers/staging/hv/hv_kvp.h
diff --git a/drivers/staging/hv/Makefile b/drivers/staging/hv/Makefile
index 4c14138..606ce7d 100644
--- a/drivers/staging/hv/Makefile
+++ b/drivers/staging/hv/Makefile
@@ -10,4 +10,4 @@ hv_vmbus-y := vmbus_drv.o osd.o \
hv_storvsc-y := storvsc_drv.o storvsc.o
hv_blkvsc-y := blkvsc_drv.o blkvsc.o
hv_netvsc-y := netvsc_drv.o netvsc.o rndis_filter.o
-hv_utils-y := hv_util.o
+hv_utils-y := hv_util.o hv_kvp.o
diff --git a/drivers/staging/hv/channel_mgmt.c
b/drivers/staging/hv/channel_mgmt.c
index 0f4d609..375f164 100644
--- a/drivers/staging/hv/channel_mgmt.c
+++ b/drivers/staging/hv/channel_mgmt.c
@@ -34,8 +34,8 @@ struct vmbus_channel_message_table_entry {
void (*messageHandler)(struct vmbus_channel_message_header *msg);
};
-#define MAX_MSG_TYPES3
-#define MAX_NUM_DEVICE_CLASSES_SUPPORTED 7
+#define MAX_MSG_TYPES4
+#define MAX_NUM_DEVICE_CLASSES_SUPPORTED 8
static const struct hv_guid
gSupportedDeviceClasses[MAX_NUM_DEVICE_CLASSES_SUPPORTED] = {
@@ -98,6 +98,15 @@ static const struct hv_guid
0xab, 0x55, 0x38, 0x2f, 0x3b, 0xd5, 0x42, 0x2d
}
},
+ /* {A9A0F4E7-5A45-4d96-B827-8A841E8C03E6} */
+ /* KVP */
+ {
+ .data = {
+ 0xe7, 0xf4, 0xa0, 0xa9, 0x45, 0x5a, 0x96, 0x4d,
+ 0xb8, 0x27, 0x8a, 0x84, 0x1e, 0x8c, 0x3, 0xe6
+ }
+ },
+
};
@@ -231,6 +240,16 @@ struct hyperv_service_callback hv_cb_utils[MAX_MSG_TYPES]
= {
.callback = chn_cb_negotiate,
.log_msg = Heartbeat channel functionality initialized
},
+ /* {A9A0F4E7-5A45-4d96-B827-8A841E8C03E6} */
+ /* KVP */
+ {
+ .data = {
+ 0xe7, 0xf4, 0xa0, 0xa9, 0x45, 0x5a, 0x96, 0x4d,
+ 0xb8, 0x27, 0x8a, 0x84, 0x1e, 0x8c, 0x3, 0xe6
+ },
+ .callback = chn_cb_negotiate,
+ .log_msg = KVP channel functionality initialized
+ },
};
EXPORT_SYMBOL(hv_cb_utils);
diff --git a/drivers/staging/hv/hv_kvp.c b/drivers/staging/hv/hv_kvp.c
new file mode 100644
index 000..5458631
--- /dev/null
+++ b/drivers/staging/hv/hv_kvp.c
@@ -0,0 +1,346 @@
+/*
+ * An implementation of key value pair (KVP) functionality for Linux.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan ksriniva...@novell.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+
+#include linux/net.h
+#include linux/nls.h
+#include linux/connector.h
+#include linux/workqueue.h
+
+#include logging.h
+#include osd.h
+#include vmbus.h
+#include vmbus_packet_format.h
+#include vmbus_channel_interface.h
+#include version_info.h
+#include channel.h
+#include vmbus_private.h
+#include vmbus_api.h
+#include utils.h
+#include hv_kvp.h
+
+
+
+/*
+ * Global state maintained for transaction that is being processed.
+ * Note that only one transaction can be active at any point in time.
+ *
+ * This state is set when we receive a request from the host; we
+ * cleanup this state when the transaction is completed - when we respond
+ * to the host with the key value.
+ */
+
+static struct {
+ bool active; /* transaction status - active or not */
+ int recv_len; /* number of bytes received. */
+ struct vmbus_channel *recv_channel; /* chn we got the request */
+ u64 recv_req_id; /* request ID. */
+} kvp_transaction;
+
+static int kvp_send_key(int index);
+
+static void kvp_respond_to_host(char *key, char *value, int error);
+static void kvp_work_func(struct work_struct *dummy);
[PATCH 4/4] Staging: hv: Add a user-space daemon to support key/value pair (KVP)
All guest specific data gathering is implemented in a user-mode daemon.
The kernel component of KVP passes the key to this daemon and
the daemon is responsible for passing back the corresponding
value. This daemon communicates with the kernel
component via a netlink channel.
Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com
---
drivers/staging/hv/tools/hv_kvp_daemon.c | 470 ++
1 files changed, 470 insertions(+), 0 deletions(-)
create mode 100644 drivers/staging/hv/tools/hv_kvp_daemon.c
diff --git a/drivers/staging/hv/tools/hv_kvp_daemon.c
b/drivers/staging/hv/tools/hv_kvp_daemon.c
new file mode 100644
index 000..f5a2dd6
--- /dev/null
+++ b/drivers/staging/hv/tools/hv_kvp_daemon.c
@@ -0,0 +1,470 @@
+/*
+ * An implementation of key value pair (KVP) functionality for Linux.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan ksriniva...@novell.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+
+#include sys/types.h
+#include sys/socket.h
+#include sys/poll.h
+#include sys/utsname.h
+#include linux/types.h
+#include stdio.h
+#include stdlib.h
+#include unistd.h
+#include string.h
+#include errno.h
+#include arpa/inet.h
+#include linux/connector.h
+#include linux/netlink.h
+#include sys/socket.h
+#include ifaddrs.h
+#include netdb.h
+#include syslog.h
+
+/*
+ * KYS: TODO. Need to register these in the kernel.
+ *
+ * The following definitions are shared with the in-kernel component; do not
+ * change any of this without making the corresponding changes in
+ * the KVP kernel component.
+ */
+#define CN_KVP_IDX 0x9 /* MSFT KVP functionality */
+#define CN_KVP_VAL 0x1 /* This supports queries from the kernel */
+#define CN_KVP_USER_VAL0x2 /* This supports queries from the
user */
+
+/*
+ * KVP protocol: The user mode component first registers with the
+ * the kernel component. Subsequently, the kernel component requests, data
+ * for the specified keys. In response to this message the user mode component
+ * fills in the value corresponding to the specified key. We overload the
+ * sequence field in the cn_msg header to define our KVP message types.
+ *
+ * We use this infrastructure for also supporting queries from user mode
+ * application for state that may be maintained in the KVP kernel component.
+ *
+ * XXXKYS: Have a shared header file between the user and kernel (TODO)
+ */
+
+enum kvp_op {
+ KVP_REGISTER = 0, /* Register the user mode component*/
+ KVP_KERNEL_GET, /*Kernel is requesting the value for the specified key*/
+ KVP_KERNEL_SET, /*Kernel is providing the value for the specified key*/
+ KVP_USER_GET, /*User is requesting the value for the specified key*/
+ KVP_USER_SET /*User is providing the value for the specified key*/
+};
+
+#define HV_KVP_EXCHANGE_MAX_KEY_SIZE 512
+#define HV_KVP_EXCHANGE_MAX_VALUE_SIZE 2048
+
+struct hv_ku_msg {
+ __u32 kvp_index;
+ __u8 kvp_key[HV_KVP_EXCHANGE_MAX_KEY_SIZE]; /* Key name */
+ __u8 kvp_value[HV_KVP_EXCHANGE_MAX_VALUE_SIZE]; /* Key value */
+};
+
+enum key_index {
+ FullyQualifiedDomainName = 0,
+ IntegrationServicesVersion, /*This key is serviced in the kernel*/
+ NetworkAddressIPv4,
+ NetworkAddressIPv6,
+ OSBuildNumber,
+ OSName,
+ OSMajorVersion,
+ OSMinorVersion,
+ OSVersion,
+ ProcessorArchitecture
+};
+
+/*
+ * End of shared definitions.
+ */
+
+static char kvp_send_buffer[4096];
+static char kvp_recv_buffer[4096];
+static struct sockaddr_nl addr;
+
+static char os_name[100];
+static char os_major[50];
+static char os_minor[50];
+static char processor_arch[50];
+static char os_build[100];
+static char *lic_version;
+
+void kvp_get_os_info(void)
+{
+ FILE*file;
+ char*eol;
+ struct utsname buf;
+
+ uname(buf);
+ strcpy(os_build, buf.release);
+ strcpy(processor_arch, buf.machine);
+
+ file = fopen(/etc/SuSE-release, r);
+ if (file != NULL)
+ goto kvp_osinfo_found;
+ file = fopen(/etc/redhat-release, r);
+ if (file != NULL)
+ goto kvp_osinfo_found;
+ /*
+* Add code for other supported platforms.
+*/
+
+ /*
+* We don't have information about the
[PATCH] Staging: hv: Add code to create the device directory under /sys/block/hdx
Add code to create the device directory under sysfs. Signed-off-by: K. Y. Srinivasan ksriniva...@novell.com --- drivers/staging/hv/blkvsc_drv.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/staging/hv/blkvsc_drv.c b/drivers/staging/hv/blkvsc_drv.c index b3d05fc..4fb8094 100644 --- a/drivers/staging/hv/blkvsc_drv.c +++ b/drivers/staging/hv/blkvsc_drv.c @@ -368,6 +368,7 @@ static int blkvsc_probe(struct device *device) blkdev-gd-first_minor = 0; blkdev-gd-fops = block_ops; blkdev-gd-private_data = blkdev; + blkdev-gd-driverfs_dev = (blkdev-device_ctx-device); sprintf(blkdev-gd-disk_name, hd%c, 'a' + devnum); blkvsc_do_inquiry(blkdev); -- 1.7.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 1/3]: Staging: hv: Use native page allocation/free functions
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Friday, February 11, 2011 1:30 PM To: KY Srinivasan Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Hank Janssen Subject: Re: [PATCH 1/3]: Staging: hv: Use native page allocation/free functions On Fri, Feb 11, 2011 at 09:59:00AM -0800, K. Y. Srinivasan wrote: --- a/drivers/staging/hv/hv.c +++ b/drivers/staging/hv/hv.c @@ -230,7 +230,12 @@ int hv_init(void) * Allocate the hypercall page memory * virtaddr = osd_page_alloc(1); */ - virtaddr = osd_virtual_alloc_exec(PAGE_SIZE); +#ifdef __x86_64__ + virtaddr = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL_EXEC); #else + virtaddr = __vmalloc(PAGE_SIZE, GFP_KERNEL, + __pgprot(__PAGE_KERNEL (~_PAGE_NX))); #endif I'm not saying this patch is wrong at all, but I still don't understand why this is different depending on the architecture of the machine. Why is this necessary, it should be ok to do the same type of allocation no matter what the processor is, right? You are right Greg; I don't think there is a need to specify different page protection bits based on the architecture - PAGE_KERNEL_EXEC should be enough. However, this is the code that is currently in the tree - refer to osd.c. If it is ok with you, I could submit an additional patch to clean this up. Regards, K. Y thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 2/3]: Staging: hv: Use native wait primitives
-Original Message- From: Jiri Slaby [mailto:jirisl...@gmail.com] Sent: Tuesday, February 15, 2011 4:21 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives On 02/11/2011 06:59 PM, K. Y. Srinivasan wrote: In preperation for getting rid of the osd layer; change the code to use native wait interfaces. As part of this, fixed the buggy implementation in the osd_wait_primitive where the condition was cleared potentially after the condition was signalled. ... @@ -566,7 +567,11 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer, } } - osd_waitevent_wait(msginfo-waitevent); + wait_event_timeout(msginfo-waitevent, + msginfo-wait_condition, + msecs_to_jiffies(1000)); + BUG_ON(msginfo-wait_condition == 0); The added BUG_ONs all over the code look scary. These shouldn't be BUG_ONs at all. You should maybe warn and bail out, but not kill the whole machine. This is Linux code running as a guest on a Windows host; and so the guest cannot tolerate a failure of the host. In the cases where I have chosen to BUG_ON, there is no reasonable recovery possible when the host is non-functional (as determined by a non-responsive host). And looking at the code, more appropriate would be completion instead of wait events. And msecs_to_jiffies(1000) == HZ. Agreed. In this first round of cleanup, I chose to keep the primitives as they were in osd.c. Greg, if it is ok with you, I will send you a patch that fixes these issues on top of the patches I have already sent. Regards, K. Y @@ -689,7 +693,8 @@ static void vmbus_ongpadl_torndown( memcpy(msginfo-response.gpadl_torndown, gpadl_torndown, sizeof(struct vmbus_channel_gpadl_torndown)); - osd_waitevent_set(msginfo-waitevent); + msginfo-wait_condition = 1; + wake_up(msginfo-waitevent); break; } } @@ -730,7 +735,8 @@ static void vmbus_onversion_response( memcpy(msginfo-response.version_response, version_response, sizeof(struct vmbus_channel_version_response)); - osd_waitevent_set(msginfo-waitevent); + msginfo-wait_condition = 1; + wake_up(msginfo-waitevent); } } spin_unlock_irqrestore(vmbus_connection.channelmsg_lock, flags); regards, -- js ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 2/3]: Staging: hv: Use native wait primitives
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Tuesday, February 15, 2011 9:03 AM To: KY Srinivasan Cc: Jiri Slaby; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives On Tue, Feb 15, 2011 at 01:35:56PM +, KY Srinivasan wrote: -Original Message- From: Jiri Slaby [mailto:jirisl...@gmail.com] Sent: Tuesday, February 15, 2011 4:21 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives On 02/11/2011 06:59 PM, K. Y. Srinivasan wrote: In preperation for getting rid of the osd layer; change the code to use native wait interfaces. As part of this, fixed the buggy implementation in the osd_wait_primitive where the condition was cleared potentially after the condition was signalled. ... @@ -566,7 +567,11 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer, } } - osd_waitevent_wait(msginfo-waitevent); + wait_event_timeout(msginfo-waitevent, + msginfo-wait_condition, + msecs_to_jiffies(1000)); + BUG_ON(msginfo-wait_condition == 0); The added BUG_ONs all over the code look scary. These shouldn't be BUG_ONs at all. You should maybe warn and bail out, but not kill the whole machine. This is Linux code running as a guest on a Windows host; and so the guest cannot tolerate a failure of the host. In the cases where I have chosen to BUG_ON, there is no reasonable recovery possible when the host is non-functional (as determined by a non-responsive host). If you have a non-responsive host, wouldn't that imply that this guest code wouldn't run at all? :) The fact that on a particular transaction the host has not responded within an expected time interval does not necessarily mean that the guest code would not be running. There may be issues on the host side that may be either transient or permanent that may cause problems like this. Keep in mind, HyperV is a type 1 hypervisor that would schedule all VMs including the host and so, guest would get scheduled. Having BUG_ON() in drivers is not a good idea either way. Please remove these in future patches. In situations where there is not a reasonable rollback strategy (for instance in one of the cases, we are granting access to the guest physical pages to the host) we really have only 2 options: 1) Wait until the host responds. This wait could potentially be unbounded and in fact this was the way the code was to begin with. One of the reviewers had suggested that unbounded wait was to be corrected. 2) Wait for a specific period and if the host does not respond within a reasonable period, kill the guest since there is no recovery possible. I chose option 2, as part of addressing some of the prior review comments. If the consensus now is to go back to option 1, I am fine with that; I will send you a patch to rectify this. Regards, K. Y And looking at the code, more appropriate would be completion instead of wait events. And msecs_to_jiffies(1000) == HZ. Agreed. In this first round of cleanup, I chose to keep the primitives as they were in osd.c. Greg, if it is ok with you, I will send you a patch that fixes these issues on top of the patches I have already sent. Yes, that is fine. thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically
-Original Message-
From: Greg KH [mailto:gre...@suse.de]
Sent: Tuesday, February 15, 2011 11:00 AM
To: KY Srinivasan
Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen
Subject: Re: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically
On Tue, Feb 15, 2011 at 07:15:37AM -0800, K. Y. Srinivasan wrote:
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/vmbus_drv.c | 49 +++--
--
1 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 459c707..f279e6a 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -36,9 +36,7 @@
#include vmbus_private.h
-/* FIXME! We need to do this dynamically for PIC and APIC system */
-#define VMBUS_IRQ 0x5
-#define VMBUS_IRQ_VECTOR IRQ5_VECTOR
+static int vmbus_irq;
/* Main vmbus driver data structure */
struct vmbus_driver_context {
@@ -57,6 +55,25 @@ struct vmbus_driver_context {
struct vm_device device_ctx;
};
+/*
+ * Find an un-used IRQ that the VMBUS can use. If none is available;
return -1.
+ */
+
+static int vmbus_get_irq(void)
No extra blank line between function comment and function.
I will take care of this.
+{
+ unsigned int avail_irq_mask;
+ int irq = -1;
+ /*
Put an extra blank line after your variables please.
Will do.
+* Pick the first unused interrupt. HyperV can
+* interrupt us on any interrupt line we specify.
+*/
+ avail_irq_mask = probe_irq_on();
+ if (avail_irq_mask != 0)
+ irq = ffs(avail_irq_mask);
+ probe_irq_off(avail_irq_mask);
+ return irq;
+}
+
static int vmbus_match(struct device *device, struct device_driver
*driver);
static int vmbus_probe(struct device *device);
static int vmbus_remove(struct device *device);
@@ -80,7 +97,6 @@ EXPORT_SYMBOL(vmbus_loglevel);
/* (ALL_MODULES 16 | DEBUG_LVL_ENTEREXIT); */
/* (((VMBUS | VMBUS_DRV)16) | DEBUG_LVL_ENTEREXIT); */
-static int vmbus_irq = VMBUS_IRQ;
/* Set up per device attributes in /sys/bus/vmbus/devices/bus device */
static struct device_attribute vmbus_device_attrs[] = {
@@ -466,7 +482,7 @@ static int vmbus_bus_init(void)
struct hv_driver *driver = vmbus_drv.drv_obj;
struct vm_device *dev_ctx = vmbus_drv.device_ctx;
int ret;
- unsigned int vector;
+ unsigned int vector, prev_irq = ~0;
DPRINT_INFO(VMBUS, +++ HV Driver version = %s +++,
HV_DRV_VERSION);
@@ -518,19 +534,23 @@ static int vmbus_bus_init(void)
}
/* Get the interrupt resource */
- ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
- driver-name, NULL);
-
- if (ret != 0) {
- DPRINT_ERR(VMBUS_DRV, ERROR - Unable to request IRQ %d,
- vmbus_irq);
-
+get_irq_again:
+ vmbus_irq = vmbus_get_irq();
+ if ((vmbus_irq 0) || (prev_irq == vmbus_irq)) {
+ printk(KERN_WARNING VMBUS_DRV: Failed to allocate IRQ\n);
bus_unregister(vmbus_drv_ctx-bus);
-
ret = -1;
Please provide a real error code.
Will do.
goto cleanup;
}
- vector = VMBUS_IRQ_VECTOR;
+ prev_irq = vmbus_irq;
+
+ ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
+ driver-name, NULL);
+
+ if (ret != 0)
+ goto get_irq_again;
You just set up the potential for an endless loop. You almost _never_
want to goto backwards in a function. Please don't do this.
The loop is not endless. We need to take care of two conditions here:
1) From the point we selected an irq line to the point where we call
request_irq(), it is possible that someone else could have requested
the same line and so we need to close this window.
2) request_irq() may fail for reasons other than the fact that we
may have lost the line that we thought we got.
So, while we loopback, we check to see if the irq line we got is the same
that we got before (refer to the check soon after the call
to vmbus_get_irq()). This check would ensure that we would not loop
endlessly.
+
+ vector = IRQ0_VECTOR + vmbus_irq;
What's this for?
This is what gets passed to the hypervisor - refer to vmbus_dev_add().
This is existing code.
DPRINT_INFO(VMBUS_DRV, irq 0x%x vector 0x%x, vmbus_irq, vector);
And why are you still printing this out for the whole world to see?
Greg, this patch addressed a specific comment with regards dynamically allocate
the irq.
Hank is working on a patch to address the various DPRINTS in the code.
Regards,
K. Y
thanks,
greg k-h
RE: [PATCH 2/3]: Staging: hv: Use native wait primitives
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Tuesday, February 15, 2011 11:30 AM To: KY Srinivasan Cc: Jiri Slaby; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives On Tue, Feb 15, 2011 at 04:22:20PM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Tuesday, February 15, 2011 9:03 AM To: KY Srinivasan Cc: Jiri Slaby; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives On Tue, Feb 15, 2011 at 01:35:56PM +, KY Srinivasan wrote: -Original Message- From: Jiri Slaby [mailto:jirisl...@gmail.com] Sent: Tuesday, February 15, 2011 4:21 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives On 02/11/2011 06:59 PM, K. Y. Srinivasan wrote: In preperation for getting rid of the osd layer; change the code to use native wait interfaces. As part of this, fixed the buggy implementation in the osd_wait_primitive where the condition was cleared potentially after the condition was signalled. ... @@ -566,7 +567,11 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer, } } - osd_waitevent_wait(msginfo-waitevent); + wait_event_timeout(msginfo-waitevent, + msginfo-wait_condition, + msecs_to_jiffies(1000)); + BUG_ON(msginfo-wait_condition == 0); The added BUG_ONs all over the code look scary. These shouldn't be BUG_ONs at all. You should maybe warn and bail out, but not kill the whole machine. This is Linux code running as a guest on a Windows host; and so the guest cannot tolerate a failure of the host. In the cases where I have chosen to BUG_ON, there is no reasonable recovery possible when the host is non-functional (as determined by a non-responsive host). If you have a non-responsive host, wouldn't that imply that this guest code wouldn't run at all? :) The fact that on a particular transaction the host has not responded within an expected time interval does not necessarily mean that the guest code would not be running. There may be issues on the host side that may be either transient or permanent that may cause problems like this. Keep in mind, HyperV is a type 1 hypervisor that would schedule all VMs including the host and so, guest would get scheduled. Having BUG_ON() in drivers is not a good idea either way. Please remove these in future patches. In situations where there is not a reasonable rollback strategy (for instance in one of the cases, we are granting access to the guest physical pages to the host) we really have only 2 options: 1) Wait until the host responds. This wait could potentially be unbounded and in fact this was the way the code was to begin with. One of the reviewers had suggested that unbounded wait was to be corrected. 2) Wait for a specific period and if the host does not respond within a reasonable period, kill the guest since there is no recovery possible. Killing the guest is a very serious thing, causing all sorts of possible problems with it, right? If there was a reasonable rollback strategy, I would not be killing the guest. I chose option 2, as part of addressing some of the prior review comments. If the consensus now is to go back to option 1, I am fine with that; Unbounded waits aren't ok either, you need some sort of timeout. But, as this is a bit preferable to dieing, I suggest doing this, and comment the heck out of it to explain all of this for anyone who reads it. If I understand you correctly, you would be prefer to have unbounded waiting with comments justifying why we cannot have timeouts. I will roll out a patch once the tree stabilizes. Regards, K. Y thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Friday, February 18, 2011 5:07 PM To: KY Srinivasan Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically On Fri, Feb 18, 2011 at 10:00:04PM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Friday, February 18, 2011 4:14 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically On Tue, Feb 15, 2011 at 11:55:35AM -0800, K. Y. Srinivasan wrote: Signed-off-by: K. Y. Srinivasan k...@microsoft.com Signed-off-by: Haiyang Zhang haiya...@microsoft.com Signed-off-by: Hank Janssen hjans...@microsoft.com You didn't run this through checkpatch.pl. Please do so and fix the warning it gives you. Greg, I did run the checkpatch script against this patch and the only complaint I got was with regards to the IRQF_SAMPLE_RANDOM flag that I pass. As a virtual machine, this is the only external event that the VM is going to see and so I chose to keep this flag. Is there something that would replace this flag; looking at the Xen drivers they do pass this flag. But that flag is going away, right? And this really can't be a valid source of entropy as the HV channel is pretty predictable. Is it going away? What would replace this. Is all interrupt sources considered predictable? This is the only unpredictable thing happening in the VM and that is the reason I chose to keep the flag. If you are only using this because Xen does/did it, that's not a valid excuse :) Surely, you are joking. In any event I am sending you a new patch with that flag removed. Regards, K. Y thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Friday, February 18, 2011 5:29 PM To: KY Srinivasan Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically On Fri, Feb 18, 2011 at 10:16:05PM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Friday, February 18, 2011 5:07 PM To: KY Srinivasan Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically On Fri, Feb 18, 2011 at 10:00:04PM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Friday, February 18, 2011 4:14 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically On Tue, Feb 15, 2011 at 11:55:35AM -0800, K. Y. Srinivasan wrote: Signed-off-by: K. Y. Srinivasan k...@microsoft.com Signed-off-by: Haiyang Zhang haiya...@microsoft.com Signed-off-by: Hank Janssen hjans...@microsoft.com You didn't run this through checkpatch.pl. Please do so and fix the warning it gives you. Greg, I did run the checkpatch script against this patch and the only complaint I got was with regards to the IRQF_SAMPLE_RANDOM flag that I pass. As a virtual machine, this is the only external event that the VM is going to see and so I chose to keep this flag. Is there something that would replace this flag; looking at the Xen drivers they do pass this flag. But that flag is going away, right? And this really can't be a valid source of entropy as the HV channel is pretty predictable. Is it going away? What would replace this. Is all interrupt sources considered predictable? Did you read the file that the checkpatch script told you to about this entry? It is only after reading the document, I decided to keep that flag. Please note this is not a question of some interrupt sources not being a good source of entropy; for this VM this is the only source of interrupts. The document on this flag talked about how people were incorrectly marking their interrupt as an entropy source; in this case there is not much of a choice. This is the only unpredictable thing happening in the VM and that is the reason I chose to keep the flag. If you remove it, do we loose all entropy for the VM? If you are only using this because Xen does/did it, that's not a valid excuse :) Surely, you are joking. Not at all. To set the record straight here, this flag is in the existing code. After I ran checkpatch, I toyed with the idea of getting rid of this. Then I decided to keep it for all the reasons I mentioned earlier. In any event I am sending you a new patch with that flag removed. Have you tested to see if you now loose all entropy, and it causes problems or not? I am glad you asked me to test it. When I remove this flag, the entropy goes down significantly and this is not surprising. Looking at /proc/sys/kernel/entropy/entropy_avail, with the original patch after a couple of compiles the number would be in thousands. With that flag removed, I have the VM up for about an hour, even after a couple of compiles, the entropy number is yet to crack 200. Let me know how you want to proceed here. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Friday, February 18, 2011 8:03 PM To: KY Srinivasan Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically On Sat, Feb 19, 2011 at 12:56:16AM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Friday, February 18, 2011 5:29 PM To: KY Srinivasan Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically On Fri, Feb 18, 2011 at 10:16:05PM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Friday, February 18, 2011 5:07 PM To: KY Srinivasan Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically On Fri, Feb 18, 2011 at 10:00:04PM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Friday, February 18, 2011 4:14 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically On Tue, Feb 15, 2011 at 11:55:35AM -0800, K. Y. Srinivasan wrote: Signed-off-by: K. Y. Srinivasan k...@microsoft.com Signed-off-by: Haiyang Zhang haiya...@microsoft.com Signed-off-by: Hank Janssen hjans...@microsoft.com You didn't run this through checkpatch.pl. Please do so and fix the warning it gives you. Greg, I did run the checkpatch script against this patch and the only complaint I got was with regards to the IRQF_SAMPLE_RANDOM flag that I pass. As a virtual machine, this is the only external event that the VM is going to see and so I chose to keep this flag. Is there something that would replace this flag; looking at the Xen drivers they do pass this flag. But that flag is going away, right? And this really can't be a valid source of entropy as the HV channel is pretty predictable. Is it going away? What would replace this. Is all interrupt sources considered predictable? Did you read the file that the checkpatch script told you to about this entry? It is only after reading the document, I decided to keep that flag. Please note this is not a question of some interrupt sources not being a good source of entropy; for this VM this is the only source of interrupts. The document on this flag talked about how people were incorrectly marking their interrupt as an entropy source; in this case there is not much of a choice. This is the only unpredictable thing happening in the VM and that is the reason I chose to keep the flag. If you remove it, do we loose all entropy for the VM? If you are only using this because Xen does/did it, that's not a valid excuse :) Surely, you are joking. Not at all. To set the record straight here, this flag is in the existing code. After I ran checkpatch, I toyed with the idea of getting rid of this. Then I decided to keep it for all the reasons I mentioned earlier. In any event I am sending you a new patch with that flag removed. Have you tested to see if you now loose all entropy, and it causes problems or not? I am glad you asked me to test it. When I remove this flag, the entropy goes down significantly and this is not surprising. Looking at /proc/sys/kernel/entropy/entropy_avail, with the original patch after a couple of compiles the number would be in thousands. With that flag removed, I have the VM up for about an hour, even after a couple of compiles, the entropy number is yet to crack 200. Let me know how you want to proceed here. Ok, my fault, let's keep the original flag. Care to resend the patch you had originally sent? Thanks Greg; I just sent it. I will deal with irq balancing issue in a separate patch. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
-Original Message-
From: Thomas Gleixner [mailto:t...@linutronix.de]
Sent: Saturday, February 19, 2011 5:23 AM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang;
Hank
Janssen
Subject: Re: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
On Tue, 15 Feb 2011, K. Y. Srinivasan wrote:
-/* FIXME! We need to do this dynamically for PIC and APIC system */
-#define VMBUS_IRQ 0x5
-#define VMBUS_IRQ_VECTOR IRQ5_VECTOR
+static int vmbus_irq;
/* Main vmbus driver data structure */
struct vmbus_driver_context {
@@ -57,6 +55,27 @@ struct vmbus_driver_context {
struct vm_device device_ctx;
};
+/*
+ * Find an un-used IRQ that the VMBUS can use. If none is available;
+ * return -EBUSY.
+ */
+static int vmbus_get_irq(void)
+{
+ unsigned int avail_irq_mask;
+ int irq = -EBUSY;
+
+ /*
+* Pick the first unused interrupt. HyperV can
+* interrupt us on any interrupt line we specify.
+*/
+
+ avail_irq_mask = probe_irq_on();
+ if (avail_irq_mask != 0)
+ irq = ffs(avail_irq_mask);
+ probe_irq_off(avail_irq_mask);
+ return irq;
Please do not use probe_irq_on for dynamic irq allocation. Highjacking
the lower PIC irqs is really not a good idea. Depending on when this
runs, you might grab an irq required by a driver which gets loaded
later.
Could you please explain what you're trying to do here ?
The IRQ being allocated is for the VMBUS driver for Linux guests running on
a Windows virtualization platform (Hyper-V hypervisor).
The hypervisor is capable of notifying events on the VMBUS via
a guest specified interrupt line. Prior to this patch,
the code was statically selecting an interrupt line for
use by VMBUS. One of the long standing review comments
on that code was to make this irq allocation dynamic and that
is what this patch does. For the Linux guest running as a VM
on Hyper-V, the concern you raise is not an issue.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
-Original Message- From: Thomas Gleixner [mailto:t...@linutronix.de] Sent: Saturday, February 19, 2011 10:12 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically On Sat, 19 Feb 2011, KY Srinivasan wrote: From: Thomas Gleixner [mailto:t...@linutronix.de] Please do not use probe_irq_on for dynamic irq allocation. Highjacking the lower PIC irqs is really not a good idea. Depending on when this runs, you might grab an irq required by a driver which gets loaded later. Could you please explain what you're trying to do here ? The IRQ being allocated is for the VMBUS driver for Linux guests running on a Windows virtualization platform (Hyper-V hypervisor). The hypervisor is capable of notifying events on the VMBUS via a guest specified interrupt line. Prior to this patch, the code was statically selecting an interrupt line for use by VMBUS. One of the long standing review comments on that code was to make this irq allocation dynamic and that is what this patch does. For the Linux guest running as a VM on Hyper-V, the concern you raise is not an issue. That patch does a whole lot of useless crap. When grabbing some random irq from the PIC is not an issue, then what's the point of this probing, retry loop and the comments about racing ? What races here? That does not make sense at all. Like most virtualization platforms, Hyper-V also emulates the full PC platform. So, it is possible that the driver of some other emulated devices might register for the IRQ line we might have selected. That is the race this code addresses. For performance reasons, we want both storage and network traffic to go over the PV drivers. I don't know why the previous reviewer wanted to have that dynamic. That just does not make sense to me. Prior to this patch, we had a hard coded interrupt line for use by this driver. If that line was already in use, the load of this driver would fail. This would be a fatal issue especially for distributions that have embedded these PV drivers as part of their installation media. This patch deals with such collisions in a more graceful way - we would not bail until we have scanned all low interrupt lines. Btw, that whole interrupt handler with two tasklets, one of them scheduling work is just screaming threaded interrupt handler. We are in the process of cleaning up these drivers; I am looking at some of these and other issues. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
-Original Message- From: Thomas Gleixner [mailto:t...@linutronix.de] Sent: Sunday, February 20, 2011 11:16 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically On Sat, 19 Feb 2011, KY Srinivasan wrote: When grabbing some random irq from the PIC is not an issue, then what's the point of this probing, retry loop and the comments about racing ? What races here? That does not make sense at all. Like most virtualization platforms, Hyper-V also emulates the full PC platform. So, it is possible that the driver of some other emulated devices might register for the IRQ line we might have selected. That is the race this code addresses. For performance reasons, we want both storage and network traffic to go over the PV drivers. So in case your driver gets the interrupt line first, which the other driver wants to acquire as well, then what? Do you want to do that probe magic in the other driver as well? What if this is a regular device driver which gets its irq number from ACPI/PCI or whatever. Then that driver simply wont work as it's interrupt line is busy. I don't know why the previous reviewer wanted to have that dynamic. That just does not make sense to me. Prior to this patch, we had a hard coded interrupt line for use by this driver. If that line was already in use, the load of this driver would fail. This would be a fatal issue especially for distributions that have embedded these PV drivers as part of their installation media. This patch deals with such collisions in a more graceful way - we would not bail until we have scanned all low interrupt lines. So you trade breaking the PV stuff against breaking random other drivers? That doesn't sound like a brilliant idea. There are various ways to solve that proper. - You can provide the interrupt number from ACPI/PCI or whatever your HV provides as enumeration. - Use a fixed vector like XEN does for the event channel - Use dynamic allocation in the IOAPIC space like the kernel does for MSI(-X) Thanks, tglx I am not claiming that what I have done here is the best possible solution. However, I will submit to you that it is better than what we had here prior to this patch. I will address this and a whole lot of other issues in future patches. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically
-Original Message- From: Thomas Gleixner [mailto:t...@linutronix.de] Sent: Monday, February 21, 2011 6:03 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: RE: [PATCH]: Staging: hv: Allocate the vmbus irq dynamically On Mon, 21 Feb 2011, KY Srinivasan wrote: Like most virtualization platforms, Hyper-V also emulates the full PC platform. So, it is possible that the driver of some other emulated devices might register for the IRQ line we might have selected. That is the race this code addresses. For performance reasons, we want both storage and network traffic to go over the PV drivers. So in case your driver gets the interrupt line first, which the other driver wants to acquire as well, then what? Do you want to do that probe magic in the other driver as well? What if this is a regular device driver which gets its irq number from ACPI/PCI or whatever. Then that driver simply wont work as it's interrupt line is busy. I don't know why the previous reviewer wanted to have that dynamic. That just does not make sense to me. Prior to this patch, we had a hard coded interrupt line for use by this driver. If that line was already in use, the load of this driver would fail. This would be a fatal issue especially for distributions that have embedded these PV drivers as part of their installation media. This patch deals with such collisions in a more graceful way - we would not bail until we have scanned all low interrupt lines. So you trade breaking the PV stuff against breaking random other drivers? That doesn't sound like a brilliant idea. There are various ways to solve that proper. - You can provide the interrupt number from ACPI/PCI or whatever your HV provides as enumeration. - Use a fixed vector like XEN does for the event channel - Use dynamic allocation in the IOAPIC space like the kernel does for MSI(-X) Thanks, tglx I am not claiming that what I have done here is the best possible solution. However, I will submit to you that it is better than what we had here prior to this patch. I will address this and a whole lot of other issues in future patches. No, it's _NOT_ better in any way. You trade breaking your PV thing against breaking random other drivers. Care to explain why you think that's better ? The root device for the VM is bound to the PV driver on some distributions. So, if we cannot load the PV drivers, we do not have a system that boots. In general, the system performance without these PV drivers is so poor that for all practical purposes, having the PV drivers is almost a requirement for having a useable system. While the platform supports configuration of the VM with some emulated devices, it is not a recommended configuration (because of performance reasons) for Linux virtual machines on the Hyper-V platform. I have spent significantly more time debating this patch than developing this patch that I still think improves the current driver. I will leave it to Greg and other powers that be to decide if this patch will be accepted. Let me know what your verdict is. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 1/4] staging: hv: Fix the code depending on struct vmbus_driver_context data order
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Wednesday, February 23, 2011 4:27 PM
To: Haiyang Zhang
Cc: Hank Janssen; KY Srinivasan; Abhishek Kane (Mindtree Consulting PVT LTD);
gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
virtualizat...@lists.osdl.org
Subject: Re: [PATCH 1/4] staging: hv: Fix the code depending on struct
vmbus_driver_context data order
On Wed, Feb 23, 2011 at 12:19:55PM -0800, Haiyang Zhang wrote:
The patch fixed the code depending on the exact order of fields in the
struct vmbus_driver_context, so the unused field drv_ctx can be removed,
and drv_obj doesn't have to be the second field in this structure.
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/blkvsc_drv.c |2 ++
drivers/staging/hv/netvsc_drv.c |2 ++
drivers/staging/hv/storvsc_drv.c |2 ++
drivers/staging/hv/vmbus.h |2 +-
drivers/staging/hv/vmbus_drv.c | 14 +-
5 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/hv/blkvsc_drv.c
b/drivers/staging/hv/blkvsc_drv.c
index 36a0adb..293ab8e 100644
--- a/drivers/staging/hv/blkvsc_drv.c
+++ b/drivers/staging/hv/blkvsc_drv.c
@@ -177,6 +177,8 @@ static int blkvsc_drv_init(int (*drv_init)(struct
hv_driver
*drv))
struct driver_context *drv_ctx = g_blkvsc_drv.drv_ctx;
int ret;
+ drv_ctx-hv_drv = storvsc_drv_obj-base;
+
storvsc_drv_obj-ring_buffer_size = blkvsc_ringbuffer_size;
/* Callback to client driver to complete the initialization */
diff --git a/drivers/staging/hv/netvsc_drv.c
b/drivers/staging/hv/netvsc_drv.c
index 03f9740..364b6c7 100644
--- a/drivers/staging/hv/netvsc_drv.c
+++ b/drivers/staging/hv/netvsc_drv.c
@@ -500,6 +500,8 @@ static int netvsc_drv_init(int (*drv_init)(struct
hv_driver
*drv))
struct driver_context *drv_ctx = g_netvsc_drv.drv_ctx;
int ret;
+ drv_ctx-hv_drv = net_drv_obj-base;
+
net_drv_obj-ring_buf_size = ring_size * PAGE_SIZE;
net_drv_obj-recv_cb = netvsc_recv_callback;
net_drv_obj-link_status_change = netvsc_linkstatus_callback;
diff --git a/drivers/staging/hv/storvsc_drv.c
b/drivers/staging/hv/storvsc_drv.c
index a8427ff..33acee5 100644
--- a/drivers/staging/hv/storvsc_drv.c
+++ b/drivers/staging/hv/storvsc_drv.c
@@ -140,6 +140,8 @@ static int storvsc_drv_init(int (*drv_init)(struct
hv_driver
*drv))
struct storvsc_driver_object *storvsc_drv_obj = g_storvsc_drv.drv_obj;
struct driver_context *drv_ctx = g_storvsc_drv.drv_ctx;
+ drv_ctx-hv_drv = storvsc_drv_obj-base;
+
storvsc_drv_obj-ring_buffer_size = storvsc_ringbuffer_size;
/* Callback to client driver to complete the initialization */
diff --git a/drivers/staging/hv/vmbus.h b/drivers/staging/hv/vmbus.h
index 42f2adb..fd9d00f 100644
--- a/drivers/staging/hv/vmbus.h
+++ b/drivers/staging/hv/vmbus.h
@@ -30,8 +30,8 @@
struct driver_context {
struct hv_guid class_id;
-
struct device_driver driver;
+ struct hv_driver *hv_drv;
If you have a pointer to hv_driver, why do you need a full 'struct
device_driver' here? That sounds really wrong.
Actually, having 'struct device_driver' within a structure called
driver_context seems wrong, this should be what 'struct hv_driver'
really is, right?
We could certainly use better names to describe the layering here:
Think of driver_context as a representation of a generic hyperV device
driver. So, this structure will embed the generic struct device_driver.
The pointer to hv_drv allows for further specialization based on
the device type. For instance the block driver has its own hv_driver while
the network driver has its own instance of hv_driver. I am not defending
this layering, and I agree we could name these abstractions to be more
intuitive and also considerably improve the layering.
Regards,
K. Y
So no, I can't take this patch, as it isn't solving the root problem
here.
thanks,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH ] Staging: hv: Hyper-V driver cleanup
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Thursday, February 24, 2011 6:46 PM To: KY Srinivasan Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH ] Staging: hv: Hyper-V driver cleanup On Thu, Feb 24, 2011 at 03:20:58PM -0800, K. Y. Srinivasan wrote: This patch cleans up (a lot of the) naming issues that various reviewers have noted. It also gets rid of some unnecessary layering in the code. Whenever you have a patch description that says It also... you know you need to break this up into smaller, logical pieces. The name change was related to the layering issue. For instance I combined the Vm_device and hv_device abstractions to build the hyperv_device abstraction. Likewise, I combined the driver_context and the hv_driver abstractions to build the the hyperv_driver abstraction. Would breaking this patch up into two patches, one dealing with the device abstraction consolidation and the other dealing with the consolidation of driver abstractions satisfy your concern. Even if I partition this patch along these lines, it will still be a large set of patches; since these changes are pervasive. As it is, I can not take this patch. Please break it up into logical patches, each doing only one thing, so we can properly review it. At the lowest level, we have one abstraction for representing a hyperv device (struct hyperv_device) and one abstraction for representing a hyperv driver (struct hyperv_driver). This collapses an unnecessary layering that existed in the code for historical reasons. While the patch is large, it was generated by a very mechanical process (global search and replace). The code compiles cleanly and I have tested this code on a 2.6.38 kernel. There is no 2.6.38 kernel yet, so I find this very hard to believe :) My mistake; I did not specify the full output of uname -a on the box that I tested this code. This box is running the LINUX-NEXT kernel : 2.6.38-rc1-0.2-default. Regards, K. Y thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH ] Staging: hv: Hyper-V driver cleanup
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Thursday, February 24, 2011 7:32 PM To: KY Srinivasan Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH ] Staging: hv: Hyper-V driver cleanup On Fri, Feb 25, 2011 at 12:24:57AM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Thursday, February 24, 2011 6:46 PM To: KY Srinivasan Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH ] Staging: hv: Hyper-V driver cleanup On Thu, Feb 24, 2011 at 03:20:58PM -0800, K. Y. Srinivasan wrote: This patch cleans up (a lot of the) naming issues that various reviewers have noted. It also gets rid of some unnecessary layering in the code. Whenever you have a patch description that says It also... you know you need to break this up into smaller, logical pieces. The name change was related to the layering issue. For instance I combined the Vm_device and hv_device abstractions to build the hyperv_device abstraction. Likewise, I combined the driver_context and the hv_driver abstractions to build the the hyperv_driver abstraction. Would breaking this patch up into two patches, one dealing with the device abstraction consolidation and the other dealing with the consolidation of driver abstractions satisfy your concern. Even if I partition this patch along these lines, it will still be a large set of patches; since these changes are pervasive. pervasive patches are fine, just remember, each patch can only do one thing. It sounds like you want to do at least 2 patches here, if not a lot more. Look at my past patches when I combined things and removed a whole layer for how to do this in a very incremental, piece-by-piece fashion (i.e, move one field over at a time until the structure is gone, and then remove it entirely.) If it is ok with you, I will do two patches - one for dealing with consolidating device structure and the other for consolidating the driver abstractions. There is no 2.6.38 kernel yet, so I find this very hard to believe :) My mistake; I did not specify the full output of uname -a on the box that I tested this code. This box is running the LINUX-NEXT kernel : 2.6.38-rc1-0.2-default. linux-next should be farther along than -rc1 as -rc6 is currently out. While the hv code is from the tip of the tree; the kernel I am running is a little dated. Regards, K. Y confused, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 3/6] Staging: hv: Cleanup hyperv_device variable names
-Original Message- From: Dan Carpenter [mailto:erro...@gmail.com] Sent: Friday, February 25, 2011 10:33 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 3/6] Staging: hv: Cleanup hyperv_device variable names On Fri, Feb 25, 2011 at 06:06:32PM -0800, K. Y. Srinivasan wrote: Cleanup the names of variables that refer to the hyperv_device abstraction. I have a script I run on rename patches. For this patch the command would be: rename_rev.pl -nc -e 's/device_ctx/device_obj/g' -e 's/\bdev_ctx/dev/g' It show one variable was missed in storvsc_probe(). memset(host_device_ctx, 0, sizeof(struct host_device_context)); host_device_ctx-port = host-host_no; - host_device_ctx-device_ctx = device_obj; + host_device_ctx-device_obj = device_obj; host_device_ctx-request_pool = kmem_cache_create(dev_name(device_obj- device), @@ -271,7 +271,7 @@ static int storvsc_probe(struct device *device) return -1; } - /* host_device_ctx-port = device_info.PortNumber; */ + /* host_hyperv_dev-port = device_info.PortNumber; */ host_device_ctx-path = device_info.path_id; host_device_ctx-target = device_info.target_id; All the other host_device_ctx variables were renamed to host_device_obj. host_device_ctx is a scsi abstraction built on top of the hyperv_device abstraction. My goal was not to change the names of higher level scsi abstractions. Regards, K. Y regards, dan carpenter ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 1/6] Staging: hv: Unify hyper-v device abstractions
-Original Message- From: Steven Rostedt [mailto:rost...@goodmis.org] Sent: Friday, February 25, 2011 10:21 PM To: KY Srinivasan; r...@home.goodmis.org Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 1/6] Staging: hv: Unify hyper-v device abstractions Hi! Just an FYI, When sending out patch series, could you send out a [PATCH 0/6] first, and have all other patches a reply to that patch. Both git and quilt have ways to do this. My mistake; I numbered them 1 through 6 instead of 0 through 5. Sorry for the confusion. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 1/6] Staging: hv: Unify hyper-v device abstractions
-Original Message- From: Steven Rostedt [mailto:rost...@goodmis.org] Sent: Friday, February 25, 2011 10:21 PM To: KY Srinivasan; r...@home.goodmis.org Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 1/6] Staging: hv: Unify hyper-v device abstractions Hi! Just an FYI, When sending out patch series, could you send out a [PATCH 0/6] first, and have all other patches a reply to that patch. Both git and quilt have ways to do this. My mistake; I numbered the patches 1 through 6 as opposed to 0 through 5. Sorry for the confusion. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 1/6] Staging: hv: Unify hyper-v device abstractions
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Monday, February 28, 2011 9:35 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang;
Hank
Janssen
Subject: Re: [PATCH 1/6] Staging: hv: Unify hyper-v device abstractions
On Fri, Feb 25, 2011 at 06:05:30PM -0800, K. Y. Srinivasan wrote:
Hyper-V drivers have supported two device abstractions.
This patch implements a single device abstraction.
This patch or This patch series?
Greg, If you recall, last week I had sent a single patch that did what these 6
patches do.
While I agree, the description could have been more informative; I have tried
to
follow the guidelines I got in terms of what should go into each patch:
The first set of 3 patches (1 through 3) deal with the device related issues:
Patch 1: Combines the state in hv_device into vm_device to create a
single device abstraction called vm_device.
Patch 2: Changes the name of vm_device to hyperv_device.
Patch 3: Cleanup the names of variables that refer to hyperv_device
Next set of 3 patches (4 through 6) deal with the driver related issues:
Patch 4: Combines the state maintained in hv_driver into driver_context to
Create a single driver abstraction called driver_context.
Patch 5: Renames the driver context to hyperv_driver.
Patch 6: Cleanup the names of variables that refer to hyperv_driver.
I am beginning to wonder if perhaps I chose the wrong granularity in splitting
up these patches.
Regards,
K. Y
This simplifies the code and avoids duplication
of state.
This patch is confusing, you are renaming structures (from hv_ to vm_)
which I didn't think you wanted to do.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/blkvsc.c| 17 ---
drivers/staging/hv/blkvsc_drv.c| 14 +++---
drivers/staging/hv/channel_mgmt.c |1 +
drivers/staging/hv/channel_mgmt.h |2 +-
drivers/staging/hv/netvsc.c| 55 ---
drivers/staging/hv/netvsc.h|2 +-
drivers/staging/hv/netvsc_api.h| 12 +++---
drivers/staging/hv/netvsc_drv.c| 28 +---
drivers/staging/hv/rndis_filter.c | 19
drivers/staging/hv/storvsc.c | 37
drivers/staging/hv/storvsc_api.h |4 +-
drivers/staging/hv/storvsc_drv.c | 21 -
drivers/staging/hv/vmbus.h | 13 +++---
drivers/staging/hv/vmbus_api.h | 29 ++--
drivers/staging/hv/vmbus_drv.c | 84
+++-
drivers/staging/hv/vmbus_private.h | 12 +++---
16 files changed, 157 insertions(+), 193 deletions(-)
diff --git a/drivers/staging/hv/blkvsc.c b/drivers/staging/hv/blkvsc.c
index 7c8729b..ecface3 100644
--- a/drivers/staging/hv/blkvsc.c
+++ b/drivers/staging/hv/blkvsc.c
@@ -35,7 +35,8 @@ static const struct hv_guid g_blk_device_type = {
}
};
-static int blk_vsc_on_device_add(struct hv_device *device, void
*additional_info)
+static int blk_vsc_on_device_add(struct vm_device *device,
+ void *additional_info)
Huh? What was this change for? 80 column issues for function
definitions is not a big deal, if any, and should not be burried in a
patch that claims to do something else.
Still totally confused,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 2/6] Staging: hv: Rename vm_device to hyperv_device
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Monday, February 28, 2011 9:33 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang;
Hank
Janssen
Subject: Re: [PATCH 2/6] Staging: hv: Rename vm_device to hyperv_device
On Fri, Feb 25, 2011 at 06:06:03PM -0800, K. Y. Srinivasan wrote:
Rename the vm_device abstraction as hyperv_device.
That's a nice name, but it's the first one with a hyperv_ prefix in
this subsystem. Ok, second, but hyperv_service_context doesn't really
count as it's not really used.
Everything else is named hv_ here. Which is it going to be, hv_ or
hyperv_?
Either is fine, just be aware of the work involved if you pick
hyperv_...
As far as the device state and driver state in this subsystem is concerned;
I thought the consensus was to name them hyperv_*
You also rename vm_device_info to hyperv_device_info here, doing more
than one thing in a single patch.
Please don't do that, this should be 2 patches.
I changed all device related state to be consistently named. However,
I will break it up into 2 patches.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/blkvsc.c|4 +-
drivers/staging/hv/blkvsc_drv.c|8 ++--
drivers/staging/hv/channel_mgmt.h |2 +-
drivers/staging/hv/netvsc.c| 56
+++
drivers/staging/hv/netvsc.h|2 +-
drivers/staging/hv/netvsc_api.h| 12
drivers/staging/hv/netvsc_drv.c| 14
drivers/staging/hv/rndis_filter.c | 18 ++--
drivers/staging/hv/storvsc.c | 36 ---
drivers/staging/hv/storvsc_api.h |4 +-
drivers/staging/hv/storvsc_drv.c | 10 +++---
drivers/staging/hv/vmbus.h |6 ++--
drivers/staging/hv/vmbus_api.h |8 ++--
drivers/staging/hv/vmbus_drv.c | 50
drivers/staging/hv/vmbus_private.h | 12
15 files changed, 124 insertions(+), 118 deletions(-)
diff --git a/drivers/staging/hv/blkvsc.c b/drivers/staging/hv/blkvsc.c
index ecface3..47ccec2 100644
--- a/drivers/staging/hv/blkvsc.c
+++ b/drivers/staging/hv/blkvsc.c
@@ -35,8 +35,8 @@ static const struct hv_guid g_blk_device_type = {
}
};
-static int blk_vsc_on_device_add(struct vm_device *device,
- void *additional_info)
+static int
+blk_vsc_on_device_add(struct hyperv_device *device, void *additional_info)
Ick, why break the formatting like this? Please keep the return type on
the same line as the function name.
You do that in a number of places, please don't.
thanks,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 3/6] Staging: hv: Cleanup hyperv_device variable names
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Monday, February 28, 2011 9:44 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang;
Hank
Janssen
Subject: Re: [PATCH 3/6] Staging: hv: Cleanup hyperv_device variable names
On Fri, Feb 25, 2011 at 06:06:32PM -0800, K. Y. Srinivasan wrote:
Cleanup the names of variables that refer to the
hyperv_device abstraction.
Clean them up to be what? Shorter? Nice? Full of rounded edges so
that when we bump into them in the dark they don't poke us and cause us
to shreak in pain?
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Sweet, you cloned yourself, I thought only Alan Cox had achieved that
goal...
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/blkvsc_drv.c | 12 ++--
drivers/staging/hv/netvsc.c |4 +-
drivers/staging/hv/netvsc_drv.c | 36
drivers/staging/hv/storvsc_drv.c | 44 +-
drivers/staging/hv/vmbus_drv.c | 164
+++-
--
5 files changed, 130 insertions(+), 130 deletions(-)
diff --git a/drivers/staging/hv/blkvsc_drv.c
b/drivers/staging/hv/blkvsc_drv.c
index 58ab0e8..305a665 100644
--- a/drivers/staging/hv/blkvsc_drv.c
+++ b/drivers/staging/hv/blkvsc_drv.c
@@ -95,7 +95,7 @@ struct blkvsc_request {
/* Per device structure */
struct block_device_context {
/* point back to our device context */
- struct hyperv_device *device_ctx;
+ struct hyperv_device *device_obj;
Hey, I was right, it does have more rounded edges, nicely done.
-static int netvsc_device_add(struct hyperv_device *device,
- void *additional_info);
+static int
+netvsc_device_add(struct hyperv_device *device, void *additional_info);
Again with the function return value hiding. Please don't.
--- a/drivers/staging/hv/storvsc_drv.c
+++ b/drivers/staging/hv/storvsc_drv.c
@@ -43,7 +43,7 @@ struct host_device_context {
/* must be 1st field
* FIXME this is a bug */
/* point back to our device context */
- struct hyperv_device *device_ctx;
+ struct hyperv_device *device_obj;
I really don't understand this change at all. obj is just as vapid
and clueless as ctx is, and it seems very gratuitous to change this.
And I should know, I have made a lot of gratuitous renames in my time in
the kernel...
Greg, there are not that many options here. As I recall there was universal
objection to the use of *context/*ctx to refer to device or driver objects.
The name I chose is fairly descriptive of what it represents. If there is
consensus
on a better name, I will use it.
static int vmbus_uevent(struct device *device, struct kobj_uevent_env *env)
{
- struct hyperv_device *device_ctx = device_to_hyperv_device(device);
+ struct hyperv_device *device_obj = device_to_hyperv_device(device);
int ret;
DPRINT_INFO(VMBUS_DRV, generating uevent -
VMBUS_DEVICE_CLASS_GUID={
%02x%02x%02x%02x-%02x%02x-%02x%02x-
%02x%02x%02x%02x%02x%02x%02x%02x},
- device_ctx-class_id.data[3], device_ctx-class_id.data[2],
- device_ctx-class_id.data[1], device_ctx-class_id.data[0],
- device_ctx-class_id.data[5], device_ctx-class_id.data[4],
- device_ctx-class_id.data[7], device_ctx-class_id.data[6],
- device_ctx-class_id.data[8], device_ctx-class_id.data[9],
- device_ctx-class_id.data[10],
- device_ctx-class_id.data[11],
- device_ctx-class_id.data[12],
- device_ctx-class_id.data[13],
- device_ctx-class_id.data[14],
- device_ctx-class_id.data[15]);
+ device_obj-class_id.data[3], device_obj-class_id.data[2],
+ device_obj-class_id.data[1], device_obj-class_id.data[0],
+ device_obj-class_id.data[5], device_obj-class_id.data[4],
+ device_obj-class_id.data[7], device_obj-class_id.data[6],
+ device_obj-class_id.data[8], device_obj-class_id.data[9],
+ device_obj-class_id.data[10],
+ device_obj-class_id.data[11],
+ device_obj-class_id.data[12],
+ device_obj-class_id.data[13],
+ device_obj-class_id.data[14],
+ device_obj-class_id.data[15]);
After seeing this stuff so many times I'm waiting for a helper function
to be added for it in this subsystem. I'm sure you really don't want to
edit GUID printk-like functions ever again, right?
I will have a separate patch for this.
static void vmbus_device_release(struct device *device)
{
- struct
RE: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, February 28, 2011 9:53 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions On Fri, Feb 25, 2011 at 06:07:03PM -0800, K. Y. Srinivasan wrote: This patch combines the two driver abstractions into a single driver abstraction. Ah, how sweet. Unfortunatly you don't say how you did this. Nor do you describe _what_ those two driver abstractions were. Are we talking i2c and usb abstractions? gpio and spi? Driver core and platform? We want to know exactly what is going on here. My mistake; I will have a more descriptive comment. Think of writing something that when you look back, in 3 years, while staring at a Linux hyperv driver originally written for the 2.6.9 kernel, that somehow never got forward ported and you are tasked with doing this, that you can just do a simple 'git log drivers/staging/hv/' and instantly know just from the changelog comments exactly what you need to do to your driver to clean it up and properly get it to work on the new 8.2.2 kernel release. This changelog entry, would require you to go and dig through the guts of the patch itself, trying to figure out what abstractions you are talking about, and exactly how they were combined, all the while wondering _why_ they were combined. Please, think of your future self, you will thank him in the years to come by doing this properly. Not to mention making other's lives easier if you happen to have escaped this dire task by then. Oh, you have an extra space up there in the subject, please fix it next time. -int blk_vsc_initialize(struct hv_driver *driver) +int blk_vsc_initialize(struct driver_context *driver) struct driver_context? Oh please no. Greg; this is the patch that consolidates the state in struct hv_driver into struct driver_context. In the spirit of doing one thing in a patch; other relevant changes are made in: Patch[5/6]: Changes the name driver_context to hyperv_driver Patch[6/6]: Cleanup all variable names that refer to struct hyperv_driver. I realize that you are hopefully going to later rename this to something else, but remember, a few patches back you thought that the ctx name wasn't nice. And here you go resuscitating it from the graveyard of pointy bits. As I noted in a different email, may be the granularity I chose in breaking up these patches is causing all this confusion. And what happens if your future patch is rejected? You are stuck with a driver_context structure in a subsystem? That's a pretty big abuse of the global namespace, don't you think? It sounds like something that should go into include/linux/device.h Please be careful about names, they mean things, even when you think they don't. Greg, would it be better if I just had one patch for dealing with all device related issues and a Separate patch for dealing all driver related issues? Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 6/6] Staging: hv: Cleanup hyperv_driver variable names
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, February 28, 2011 9:59 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 6/6] Staging: hv: Cleanup hyperv_driver variable names On Fri, Feb 25, 2011 at 06:07:58PM -0800, K. Y. Srinivasan wrote: The title says it all. That's a horrible changelog comment. So bad that I would rather see an empty message than this one. Seriously, it give no description, and makes us think that the whole patch is obvious, when it really isn't. What did you change them to? What did you change them from? What was your motivation in changing them? How were you feeling when the names changed? Ok, maybe not the last one, but you get the idea. Greg, these changes (patches 1 through 6) change so much in this sub-system that until these changes go in, our cleanup efforts are stalled. That is the main reason I was so hasty in submitting these patches. Clearly, I need to provide a better changelog comment; and I will. Looking at your other comments, I am wondering if the granularity I chose for breaking up the changes that had to be done is also a significant part of the problem. If it is ok with you, I could generate a patch that deals with all device related issues and a patch that deals with all driver related issues. These patches obviously will do more than one thing (however they will all be related); but at least they won't have intermediate state that would be objectionable. Let me know. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Wednesday, March 02, 2011 12:41 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions On Wed, Mar 02, 2011 at 01:43:13AM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, February 28, 2011 9:53 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions On Fri, Feb 25, 2011 at 06:07:03PM -0800, K. Y. Srinivasan wrote: This patch combines the two driver abstractions into a single driver abstraction. Ah, how sweet. Unfortunatly you don't say how you did this. Nor do you describe _what_ those two driver abstractions were. Are we talking i2c and usb abstractions? gpio and spi? Driver core and platform? We want to know exactly what is going on here. My mistake; I will have a more descriptive comment. Think of writing something that when you look back, in 3 years, while staring at a Linux hyperv driver originally written for the 2.6.9 kernel, that somehow never got forward ported and you are tasked with doing this, that you can just do a simple 'git log drivers/staging/hv/' and instantly know just from the changelog comments exactly what you need to do to your driver to clean it up and properly get it to work on the new 8.2.2 kernel release. This changelog entry, would require you to go and dig through the guts of the patch itself, trying to figure out what abstractions you are talking about, and exactly how they were combined, all the while wondering _why_ they were combined. Please, think of your future self, you will thank him in the years to come by doing this properly. Not to mention making other's lives easier if you happen to have escaped this dire task by then. Oh, you have an extra space up there in the subject, please fix it next time. -int blk_vsc_initialize(struct hv_driver *driver) +int blk_vsc_initialize(struct driver_context *driver) struct driver_context? Oh please no. Greg; this is the patch that consolidates the state in struct hv_driver into struct driver_context. In the spirit of doing one thing in a patch; other relevant changes are made in: Patch[5/6]: Changes the name driver_context to hyperv_driver Patch[6/6]: Cleanup all variable names that refer to struct hyperv_driver. Yes, but on its own, this patch is wrong, that is not a valid name, even if it is a temporary name. Greg, the temporary name happens to be the name currently in use in the code - this is not the name I introduced. Think of this as the surviving data structure after the hv_driver state is consolidated into (the existing) driver_context data structure. I did this in the spirit of doing one thing at a time. If I am going to be picking a more appropriate name for the consolidated data structure; I might as well pick the final name that we want this unified driver abstraction to be called. I realize that you are hopefully going to later rename this to something else, but remember, a few patches back you thought that the ctx name wasn't nice. And here you go resuscitating it from the graveyard of pointy bits. As I noted in a different email, may be the granularity I chose in breaking up these patches is causing all this confusion. Yes, as I think you need to go much finer as you were doing more than one thing in these patches, and not describing them properly at all. Please try to redo them in a simpler manner, probably breaking it into more steps, so we can properly review them. Based on your comments on intermediate names, would you recommend that as part of consolidating the driver abstractions, I also rename this combined state. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Thursday, March 03, 2011 1:10 AM To: KY Srinivasan Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions On Thu, Mar 03, 2011 at 02:50:00AM +, KY Srinivasan wrote: struct driver_context? Oh please no. Greg; this is the patch that consolidates the state in struct hv_driver into struct driver_context. In the spirit of doing one thing in a patch; other relevant changes are made in: Patch[5/6]: Changes the name driver_context to hyperv_driver Patch[6/6]: Cleanup all variable names that refer to struct hyperv_driver. Yes, but on its own, this patch is wrong, that is not a valid name, even if it is a temporary name. Greg, the temporary name happens to be the name currently in use in the code - this is not the name I introduced. There is not a struct driver_context in the code that I see today, or am I missing something? That's my objection here, please don't use that name, it's not valid for a subsystem to use, even for a tiny bit. Look at the file vmbus.h you will see struct driver_context. This has been there for as long as I have seen this code. Think of this as the surviving data structure after the hv_driver state is consolidated into (the existing) driver_context data structure. I did this in the spirit of doing one thing at a time. If I am going to be picking a more appropriate name for the consolidated data structure; I might as well pick the final name that we want this unified driver abstraction to be called. Your final name is fine, it's the intermediate one I'm objecting to. Ok. How about 'struct hv_driver_context' instead? I realize that you are hopefully going to later rename this to something else, but remember, a few patches back you thought that the ctx name wasn't nice. And here you go resuscitating it from the graveyard of pointy bits. As I noted in a different email, may be the granularity I chose in breaking up these patches is causing all this confusion. Yes, as I think you need to go much finer as you were doing more than one thing in these patches, and not describing them properly at all. Please try to redo them in a simpler manner, probably breaking it into more steps, so we can properly review them. Based on your comments on intermediate names, would you recommend that as part of consolidating the driver abstractions, I also rename this combined state. Probably, if I understand what you are referring to. Please post code so that I really know what you are doing :) thanks, greg k-h Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Thursday, March 03, 2011 4:22 PM To: KY Srinivasan Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions On Thu, Mar 03, 2011 at 09:16:29PM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Thursday, March 03, 2011 1:10 AM To: KY Srinivasan Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Hank Janssen Subject: Re: [PATCH 4/6] Staging: hv: Unify the hyperv driver abstractions On Thu, Mar 03, 2011 at 02:50:00AM +, KY Srinivasan wrote: struct driver_context? Oh please no. Greg; this is the patch that consolidates the state in struct hv_driver into struct driver_context. In the spirit of doing one thing in a patch; other relevant changes are made in: Patch[5/6]: Changes the name driver_context to hyperv_driver Patch[6/6]: Cleanup all variable names that refer to struct hyperv_driver. Yes, but on its own, this patch is wrong, that is not a valid name, even if it is a temporary name. Greg, the temporary name happens to be the name currently in use in the code - this is not the name I introduced. There is not a struct driver_context in the code that I see today, or am I missing something? That's my objection here, please don't use that name, it's not valid for a subsystem to use, even for a tiny bit. Look at the file vmbus.h you will see struct driver_context. This has been there for as long as I have seen this code. Ok, I am rightly corrected, I totally missed that, you are right. Feel free to resend after addressing the other issues. I'll fix up the hv_mouse driver, you don't have to worry about that one if you don't want to, just ignore it please. Greg, I am working on a patch-set that hopefully will address all the concerns that were raised. As part of this effort, I will also deal with the mouse driver. I should have these patches out next week. Thanks for your patience here. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 00/16] Staging: hv: Consolidate driver and device abstractions
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, March 07, 2011 5:24 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/16] Staging: hv: Consolidate driver and device abstractions On Mon, Mar 07, 2011 at 01:01:35PM -0800, K. Y. Srinivasan wrote: Hyper-V has maintained both its class independent driver and device state in two independent data structures: Driver state: struct driver_context (vmbus.h) and struct hv_driver (vmbus_api.h) Device state: struct vm_device (vmbus.h) and struct hv_device (vmbus_api.h) While sruct driver_context and struct vm_device embed generic Linux abstractions of struct device_driver and struct device respectively; the lower level hyperv abstraction: struct hv_driver and struct hv_device have maintained state needed to communicate with the host. This partitioning made sense at a point in time when it was not clear how much of the hypervisor interaction would be open sourced. Given where we are today, there is no reason to keep this layering. This patchset consolidates all the driver state into a single structure: struct hv_driver while all the device state is consolidated into a single structure: struct hv_device. This consolidation simplifies the code while simultaneously getting rid of redundant state - for instance in the current code, both struct driver_context and struct hv_driver both have state to represent the class as well as instance id. We do this consolidation by moving state from higher level structures (struct driver_context and struct vm_device) to lower level structures (struct hv_driver and hv_device) respectively. While it has not been a goal of this effort to cleanup structure and variable names, this consolidation effort has resulted in some cleanup with regards to structure and variable names: a) Some of the badly named structures have been eliminated (struct driver_context etc.) while hopefully all newly introduced names are acceptable. Patches 1 through 11 deal with consolidating the device driver state while patches 12 through 16 deal with consolidating device state. Very nice job, now that's the way to break up and send a patch series. I've queued them all up now. Thanks Greg; coming from you it means a lot to me. I'm guessing that you will have follow-on patches now to complete the migration to the correct driver core use (i.e. proper driver and device usage?) Or do you want me to look into doing this? My immediate goal is to get the vmbus driver to exit staging. To that end I am working on a patch-set to cleanup vmbus_drv.c. I should have this patch-set fairly soon. Once that is done, I think we would have addressed all the structural/architectural issues of the vmbus driver that is preventing us from exiting staging. We are planning to address the issues with other drivers after we are done with the vmbus driver. As always, your help is greatly appreciated. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 00/16] Staging: hv: Consolidate driver and device abstractions
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, March 07, 2011 6:00 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/16] Staging: hv: Consolidate driver and device abstractions On Mon, Mar 07, 2011 at 10:45:15PM +, KY Srinivasan wrote: I'm guessing that you will have follow-on patches now to complete the migration to the correct driver core use (i.e. proper driver and device usage?) Or do you want me to look into doing this? My immediate goal is to get the vmbus driver to exit staging. To that end I am working on a patch-set to cleanup vmbus_drv.c. I should have this patch-set fairly soon. Once that is done, I think we would have addressed all the structural/architectural issues of the vmbus driver that is preventing us from exiting staging. Well, perhaps, let's not get ahead of ourselves here :) Certainly. I was being presumptuous when I said we would have addressed all the structural/architectural issues ... What I meant to say was that I would have addressed the problems that I know about. After I am done with the current cleanup, I will work on the remaining issues. We are planning to address the issues with other drivers after we are done with the vmbus driver. As always, your help is greatly appreciated. The issue I am referring to above still has to do with the vmbus core. The goal is to have the vmbus work like all other busses in the kernel. You register a hv_driver with some probe and disconnect callbacks, and the vmbus calls into the drivers when it needs to. You are almost there, using the struct device pointers directly, but a few more steps remain. I'll look into the details after your remaining cleanups, I don't want to get in the way of them. Thank you. You should have my patches fairly soon. Hopefully, I would have addressed the issues you have raised. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver
-Original Message-
From: Greg KH [mailto:gre...@suse.de]
Sent: Thursday, March 10, 2011 5:21 PM
To: KY Srinivasan
Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
virtualizat...@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane
(Mindtree Consulting PVT LTD); Hank Janssen
Subject: Re: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci
driver
On Thu, Mar 10, 2011 at 02:08:32PM -0800, K. Y. Srinivasan wrote:
Make vmbus driver a platform pci driver. This is
in preparation to cleaning up irq allocation for this
driver.
The idea is nice, but the nameing is a bit confusing.
We have platform drivers which are much different from what you are
doing here, you are just creating a normal pci driver.
Very minor comments below.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Mike Sterling mike.sterl...@microsoft.com
Signed-off-by: Abhishek Kane v-abk...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/vmbus_drv.c | 63 +++-
1 files changed, 36 insertions(+), 27 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 8b9394a..e4855ac 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -43,6 +43,8 @@
static struct device *root_dev; /* Root device */
+struct pci_dev *hv_pci_dev;
+
/* Main vmbus driver data structure */
struct vmbus_driver_context {
@@ -887,36 +889,24 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
}
}
-static struct dmi_system_id __initdata microsoft_hv_dmi_table[] = {
- {
- .ident = Hyper-V,
- .matches = {
- DMI_MATCH(DMI_SYS_VENDOR, Microsoft
Corporation),
- DMI_MATCH(DMI_PRODUCT_NAME, Virtual Machine),
- DMI_MATCH(DMI_BOARD_NAME, Virtual Machine),
- },
- },
- { },
-};
-MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);
You're sure it's safe to delete this now and just rely on the PCI ids,
right? For some wierd reason I thought we needed both to catch all
types of systems, but I can't remember why.
I have tested this; I don't think we need the dmi table.
-static int __init vmbus_init(void)
+
+static int __devinit hv_pci_probe(struct pci_dev *pdev,
+ const struct pci_device_id *ent)
{
- DPRINT_INFO(VMBUS_DRV,
- Vmbus initializing current log level 0x%x (%x,%x),
- vmbus_loglevel, HIWORD(vmbus_loglevel),
LOWORD(vmbus_loglevel));
- /* Todo: it is used for loglevel, to be ported to new kernel. */
+ int err;
- if (!dmi_check_system(microsoft_hv_dmi_table))
- return -ENODEV;
+ hv_pci_dev = pdev;
- return vmbus_bus_init();
-}
+ err = pci_enable_device(pdev);
+ if (err)
+ return err;
-static void __exit vmbus_exit(void)
-{
- vmbus_bus_exit();
- /* Todo: it is used for loglevel, to be ported to new kernel. */
+ err = vmbus_bus_init();
+ if (err)
+ pci_disable_device(pdev);
+
+ return err;
}
/*
@@ -931,10 +921,29 @@ static const struct pci_device_id
microsoft_hv_pci_table[] = {
};
MODULE_DEVICE_TABLE(pci, microsoft_hv_pci_table);
+static struct pci_driver platform_driver = {
hv_bus_driver?
+ .name = hv-platform-pci,
How about hv_bus as a name, as that's what this really is. It's a
bus adapter, like USB, Firewire, and all sorts of other bus
controllers.
Sure; I will make these changes. Would you mind if I submit these name changes
as a separate patch.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver
-Original Message-
From: Greg KH [mailto:gre...@suse.de]
Sent: Thursday, March 10, 2011 5:33 PM
To: KY Srinivasan
Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
virtualizat...@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane
(Mindtree Consulting PVT LTD); Hank Janssen
Subject: Re: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci
driver
On Thu, Mar 10, 2011 at 10:28:27PM +, KY Srinivasan wrote:
-Original Message-
From: Greg KH [mailto:gre...@suse.de]
Sent: Thursday, March 10, 2011 5:21 PM
To: KY Srinivasan
Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
virtualizat...@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane
(Mindtree Consulting PVT LTD); Hank Janssen
Subject: Re: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci
driver
On Thu, Mar 10, 2011 at 02:08:32PM -0800, K. Y. Srinivasan wrote:
Make vmbus driver a platform pci driver. This is
in preparation to cleaning up irq allocation for this
driver.
The idea is nice, but the nameing is a bit confusing.
We have platform drivers which are much different from what you are
doing here, you are just creating a normal pci driver.
Very minor comments below.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Mike Sterling mike.sterl...@microsoft.com
Signed-off-by: Abhishek Kane v-abk...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/vmbus_drv.c | 63 +++
-
1 files changed, 36 insertions(+), 27 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c
b/drivers/staging/hv/vmbus_drv.c
index 8b9394a..e4855ac 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -43,6 +43,8 @@
static struct device *root_dev; /* Root device */
+struct pci_dev *hv_pci_dev;
+
/* Main vmbus driver data structure */
struct vmbus_driver_context {
@@ -887,36 +889,24 @@ static irqreturn_t vmbus_isr(int irq, void
*dev_id)
}
}
-static struct dmi_system_id __initdata microsoft_hv_dmi_table[] = {
- {
- .ident = Hyper-V,
- .matches = {
- DMI_MATCH(DMI_SYS_VENDOR, Microsoft
Corporation),
- DMI_MATCH(DMI_PRODUCT_NAME, Virtual Machine),
- DMI_MATCH(DMI_BOARD_NAME, Virtual Machine),
- },
- },
- { },
-};
-MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);
You're sure it's safe to delete this now and just rely on the PCI ids,
right? For some wierd reason I thought we needed both to catch all
types of systems, but I can't remember why.
I have tested this; I don't think we need the dmi table.
Ok, if you are sure, that's fine with me.
How about hv_bus as a name, as that's what this really is. It's a
bus adapter, like USB, Firewire, and all sorts of other bus
controllers.
Sure; I will make these changes. Would you mind if I submit these name
changes as a separate patch.
How about just redo this patch? I haven't reviewed the others yet, so
you might want to wait a day to see if I don't like any of them either
:)
Ok; I will wait for the reviews.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 12/21] Staging: hv: Cleanup irq management
-Original Message- From: Hank Janssen Sent: Thursday, March 10, 2011 5:54 PM To: Thomas Gleixner Cc: KY Srinivasan; gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 12/21] Staging: hv: Cleanup irq management On Mar 10, 2011, at 14:46, Thomas Gleixner t...@linutronix.de wrote: } -vector = VMBUS_IRQ_VECTOR; -DPRINT_INFO(VMBUS_DRV, irq 0x%x vector 0x%x, vmbus_irq, vector); +vector = IRQ0_VECTOR + pdev-irq; +DPRINT_INFO(VMBUS_DRV, irq 0x%x vector 0x%x, pdev-irq, +IRQ0_VECTOR + pdev-irq); Why evaluating vector first and then not using it for that debug print thingy? Good point; I will fix this before Hank gets rid of the DPRINT_INFO altogether. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 13/21] [PATCH 13/21] Staging: hv: Rename vmbus_driver_context structure
-Original Message-
From: Thomas Gleixner [mailto:t...@linutronix.de]
Sent: Thursday, March 10, 2011 5:50 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang;
Mike
Sterling; Abhishek Kane (Mindtree Consulting PVT LTD); Hank Janssen
Subject: Re: [PATCH 13/21] [PATCH 13/21] Staging: hv: Rename
vmbus_driver_context structure
On Thu, 10 Mar 2011, K. Y. Srinivasan wrote:
Now that struct vmbus_driver_context is properly
cleaned up, rename this structure appropriately and
cleanup the code.
@@ -877,10 +873,10 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
/* Schedules a dpc if necessary */
if (ret 0) {
if (test_bit(0, (unsigned long *)ret))
- tasklet_schedule(vmbus_drv.msg_dpc);
+ tasklet_schedule(hv_bus.msg_dpc);
if (test_bit(1, (unsigned long *)ret))
- tasklet_schedule(vmbus_drv.event_dpc);
+ tasklet_schedule(hv_bus.event_dpc);
What's the plan to convert that tasklet/work stuff to threaded irqs ?
This is on my plate; I was going to work on this after I addressed all other
issues
preventing the vmbus driver from exiting staging.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver
-Original Message-
From: Greg KH [mailto:gre...@suse.de]
Sent: Thursday, March 10, 2011 5:33 PM
To: KY Srinivasan
Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
virtualizat...@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane
(Mindtree Consulting PVT LTD); Hank Janssen
Subject: Re: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci
driver
On Thu, Mar 10, 2011 at 10:28:27PM +, KY Srinivasan wrote:
-Original Message-
From: Greg KH [mailto:gre...@suse.de]
Sent: Thursday, March 10, 2011 5:21 PM
To: KY Srinivasan
Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
virtualizat...@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane
(Mindtree Consulting PVT LTD); Hank Janssen
Subject: Re: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci
driver
On Thu, Mar 10, 2011 at 02:08:32PM -0800, K. Y. Srinivasan wrote:
Make vmbus driver a platform pci driver. This is
in preparation to cleaning up irq allocation for this
driver.
The idea is nice, but the nameing is a bit confusing.
We have platform drivers which are much different from what you are
doing here, you are just creating a normal pci driver.
Very minor comments below.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Mike Sterling mike.sterl...@microsoft.com
Signed-off-by: Abhishek Kane v-abk...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/vmbus_drv.c | 63 +++
-
1 files changed, 36 insertions(+), 27 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c
b/drivers/staging/hv/vmbus_drv.c
index 8b9394a..e4855ac 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -43,6 +43,8 @@
static struct device *root_dev; /* Root device */
+struct pci_dev *hv_pci_dev;
+
/* Main vmbus driver data structure */
struct vmbus_driver_context {
@@ -887,36 +889,24 @@ static irqreturn_t vmbus_isr(int irq, void
*dev_id)
}
}
-static struct dmi_system_id __initdata microsoft_hv_dmi_table[] = {
- {
- .ident = Hyper-V,
- .matches = {
- DMI_MATCH(DMI_SYS_VENDOR, Microsoft
Corporation),
- DMI_MATCH(DMI_PRODUCT_NAME, Virtual Machine),
- DMI_MATCH(DMI_BOARD_NAME, Virtual Machine),
- },
- },
- { },
-};
-MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);
You're sure it's safe to delete this now and just rely on the PCI ids,
right? For some wierd reason I thought we needed both to catch all
types of systems, but I can't remember why.
I have tested this; I don't think we need the dmi table.
Ok, if you are sure, that's fine with me.
How about hv_bus as a name, as that's what this really is. It's a
bus adapter, like USB, Firewire, and all sorts of other bus
controllers.
Sure; I will make these changes. Would you mind if I submit these name
changes as a separate patch.
How about just redo this patch? I haven't reviewed the others yet, so
you might want to wait a day to see if I don't like any of them either
:)
Greg, I have redone this patch as well as [PATCH 12/21]. Do you want me send
you just these two
patches or the entire series including these two. Also, does this patch-set
address all of architectural
issues you had noted earlier in the vmbus core. Please let us know what else
needs to be done to
exit staging as far as the vmbus driver is concerned. I want get a head start
before the new week
begins! Also, we have patches ready for all DPRINT cleanup. Hank is holding
them off until
we finish addressing the architectural issues first.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Sunday, March 13, 2011 11:25 PM To: KY Srinivasan Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane (Mindtree Consulting PVT LTD); Hank Janssen Subject: Re: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver On Sat, Mar 12, 2011 at 11:23:05PM +, KY Srinivasan wrote: Greg, I have redone this patch as well as [PATCH 12/21]. Do you want me send you just these two patches or the entire series including these two. Just resend those two patches if that is easier. Will do. Also, does this patch-set address all of architectural issues you had noted earlier in the vmbus core. Please let us know what else needs to be done to exit staging as far as the vmbus driver is concerned. I want get a head start before the new week begins! Also, we have patches ready for all DPRINT cleanup. Hank is holding them off until we finish addressing the architectural issues first. I do not know if this addresses everything, sorry, I have not had the time to review all of them yet. Give me a few days at the least to go over them and apply them before I will be able to tell you this. Thanks for taking the time to look at this. Also note that there shouldn't be anything holding back the DPRINT stuff, why wait? If they apply on top of yours that should be fine, right? You are right. We will submit the DPRINT patches soon. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 00/21] Staging: hv: Cleanup vmbus driver
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, March 14, 2011 3:37 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/21] Staging: hv: Cleanup vmbus driver On Thu, Mar 10, 2011 at 01:59:42PM -0800, K. Y. Srinivasan wrote: This patch-set fixes the following issues in the vmbus driver (vmbus_drv.c): Cleanup root device management: (patches 1/21 through 10/21) 1) Get rid of the hv_driver code from the vmbus abstraction 2) Get rid of unnecessary call sequences and functions 3) Cleanup the management of the root device by using the standard mechanism for grouping devices under /sys/devices I've applied the first 9 patches, as I had questions on 10 and 11, and the others after that don't apply without those two applied. Let me know what the questions are on 10 and 11. When I first sent you the patch-set, you had suggested some name changes in 11. Thomas had a comment on 12. This morning I sent the corrected version of 11 as well as 12. So, care to resend the rest of the series when we've figured out the root device stuff? Sure. Let me know what the issues/concerns are with regards to root device handling. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 10/21] Staging: hv: Cleanup root device handling
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, March 14, 2011 3:34 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 10/21] Staging: hv: Cleanup root device handling On Thu, Mar 10, 2011 at 02:08:06PM -0800, K. Y. Srinivasan wrote: Now we can complete the cleanup of the root device management. Use the preferred APIs for creating and managing the root device. As part of this cleanup get rid of the root device object from vmbus_driver_context. I don't understand, what is the root device? This would be the device under /sys/devices that all other hyperv devices would be grouped under. This notion of the root device existed in the existing code; however its creation and management was unnecessarily complicated. Regards, K. Y The hyper-v bus controller? Signed-off-by: K. Y. Srinivasan k...@microsoft.com Signed-off-by: Haiyang Zhang haiya...@microsoft.com Signed-off-by: Mike Sterling mike.sterl...@microsoft.com Signed-off-by: Abhishek Kane v-abk...@microsoft.com Signed-off-by: Hank Janssen hjans...@microsoft.com --- drivers/staging/hv/vmbus_drv.c | 126 ++-- 1 files changed, 18 insertions(+), 108 deletions(-) diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c index b473f46..8b9394a 100644 --- a/drivers/staging/hv/vmbus_drv.c +++ b/drivers/staging/hv/vmbus_drv.c @@ -19,6 +19,7 @@ * Hank Janssen hjans...@microsoft.com */ #include linux/init.h +#include linux/err.h #include linux/module.h #include linux/device.h #include linux/irq.h @@ -40,6 +41,8 @@ #define VMBUS_IRQ 0x5 #define VMBUS_IRQ_VECTOR IRQ5_VECTOR +static struct device *root_dev; /* Root device */ This shouldn't be a raw struct device, should it? It should be of a type that shows exactly what it is. Is it a hyper_v device that talks on the bus? Or is it a platform device that controls all of the devices on the bus, and as such should be the root device of the bus tree? confused, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 10/21] Staging: hv: Cleanup root device handling
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Monday, March 14, 2011 3:59 PM To: KY Srinivasan Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 10/21] Staging: hv: Cleanup root device handling On Mon, Mar 14, 2011 at 07:54:29PM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Monday, March 14, 2011 3:34 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 10/21] Staging: hv: Cleanup root device handling On Thu, Mar 10, 2011 at 02:08:06PM -0800, K. Y. Srinivasan wrote: Now we can complete the cleanup of the root device management. Use the preferred APIs for creating and managing the root device. As part of this cleanup get rid of the root device object from vmbus_driver_context. I don't understand, what is the root device? This would be the device under /sys/devices that all other hyperv devices would be grouped under. This notion of the root device existed in the existing code; however its creation and management was unnecessarily complicated. But that is what your new pci device should be, not a separate one. Why not use that instead? Actually, how are things looking then? You have a pci device, with no children, yet the root device has the children devices? That doesn't really make sense now does it? Good point. I will cleanup the patches and send you the updated ones shortly. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 00/12] Staging: hv: Cleanup vmbus driver - Phase II
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Tuesday, March 15, 2011 6:05 PM To: KY Srinivasan Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/12] Staging: hv: Cleanup vmbus driver - Phase II On Tue, Mar 15, 2011 at 03:02:07PM -0700, K. Y. Srinivasan wrote: This patch-set fixes the following issues in the vmbus driver (vmbus_drv.c): snip Thanks for the patches, but as the .39 merge window is closed, I'll be holding on to these until after .39-rc1 is out before I can do anything with them. So don't be surprised if I don't respond to them for a few weeks. Don't worry, they aren't lost. :) If possible, I would love to get your feedback even if you cannot check in these patches. Also, if you can give me feedback as to what else would need to be fixed to exit staging as far as the vmbus driver is concerned, I can work on those over the next couple of weeks until the tree opens up. If it is ok with you we will send the DPRINT cleanup patches in the next couple of days. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 05/12] Staging: hv: Get rid of the forward declaration for vmbus_uevent
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, March 15, 2011 6:23 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 05/12] Staging: hv: Get rid of the forward declaration for vmbus_uevent On Tue, Mar 15, 2011 at 03:03:37PM -0700, K. Y. Srinivasan wrote: Get rid of the forward declaration of vmbus_uevent by moving the code around. There are 2 05/12 patches and they are different. Sorry about that Greg; I had an incorrect commit message that I fixed, but forgot to delete that patch that was generated with the incorrect commit message. Loose the patch that has the subject line: [PATCH 05/12] Get rid of the forward declaration for vmbus_uevent You want to keep the one that has the following subject line: [PATCH 05/12] Staging: hv: Get rid of the forward declaration for vmbus_uevent I could resend, if you prefer. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 00/22] Staging: hv: Cleanup-storage-drivers-phase-III
Sorry for the confusion Greg. I reworked some patches in Phase-II cleanup and I was meaning to resend them before you began applying the patches, That is the reason for the problem you are encountering. I just resent the Phase II series. If you can back out the phase II that you have applied and apply the ones I resent, everything should apply cleanly. Once again, I sorry for creating additional work for you. Regards, K. Y -Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, April 05, 2011 12:59 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/22] Staging: hv: Cleanup-storage-drivers-phase-III On Mon, Mar 28, 2011 at 10:00:15AM -0700, K. Y. Srinivasan wrote: This patch-set deals with some of the style isues in blkvsc_drv.c. We also get rid most of the dead code in this file: 1) Get rid of most of the forward declarations in this file. The only remaining forward declarations are to deal with circular dependencies. 2) Get rid of most of the dead code in the file. Some of the functions in this file are place holders - they are not fuly implementedi yet (blkvsc_ioctl(), blkvsc_media_changed(). We have retained some comments/dead code, to help us in the final implementation. For some reason, this patch series fails to apply after the first one. So I've not applied any of them, care to resync and resend? thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 00/22] Staging: hv: Cleanup-storage-drivers-phase-III
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, April 05, 2011 11:11 AM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/22] Staging: hv: Cleanup-storage-drivers-phase-III A: No. Q: Should I include quotations after my reply? http://daringfireball.net/2007/07/on_top On Tue, Apr 05, 2011 at 01:58:37PM +, KY Srinivasan wrote: Sorry for the confusion Greg. I reworked some patches in Phase-II cleanup and I was meaning to resend them before you began applying the patches, That is the reason for the problem you are encountering. Huh? I applied them in the order you sent them to me, how would that cause a patch to fail to apply properly? I just resent the Phase II series. If you can back out the phase II that you have applied and apply the ones I resent, everything should apply cleanly. Once again, I sorry for creating additional work for you. I can't back out anything, sorry. I'll drop any pending patches you have sent me now. Please resync with my staging git tree and resend them to properly apply and work. Greg, An upstream merge that occurred after the last tree closure was the cause of some of my patches not applying. I have fixed up all the patches and resent them for your consideration. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Hyper-V vmbus driver
Greg, Recently, you applied a patch-set from me that cleaned a bunch of architectural issues in the vmbus driver. With that patch-set, I think I have addressed all architectural issues that I am aware of. I was wondering if you would have the time to let me know what else would have to be addressed in the vmbus driver, before it could be considered ready for exiting staging. As always your help is greatly appreciated. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
irq allocation for pci drivers
Greg, Has something changed in the PCI code in the Linux-next tree - 2.6.39-rc1-12 with regards to irq allocation? Earlier (a few weeks ago), the vmbus driver would get the irq allocated as part of it being now a pci driver. Currently, however the system is setting pdev-irq to 0 and needless to say, request_irq() is failing. Thanks, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: irq allocation for pci drivers
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Tuesday, April 12, 2011 11:41 PM To: KY Srinivasan Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: irq allocation for pci drivers On Tue, Apr 12, 2011 at 11:57:22PM +, KY Srinivasan wrote: Greg, Anyway, I have no idea, please use 'git bisect' to track down the problem commit that causes this. Greg, This problem turned out to be on the host side. We don't have any issue with dynamic irq allocation on the win2k8 r2 hosts. Some older hosts have an issue and in any event it does not look like it is a Linux issue. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 05/22] Staging: hv: Get rid of the forward declaration of blkvsc_init_rw()
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Wednesday, April 20, 2011 4:35 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 05/22] Staging: hv: Get rid of the forward declaration of blkvsc_init_rw() On Wed, Apr 06, 2011 at 02:35:03PM -0700, K. Y. Srinivasan wrote: Get rid of the forward declaration of blkvsc_init_rw() by moving the code around. This patch fails to apply: Applying: Staging: hv: Get rid of the forward declaration of blkvsc_init_rw() error: patch failed: drivers/staging/hv/blkvsc_drv.c:1000 error: drivers/staging/hv/blkvsc_drv.c: patch does not apply Patch failed at 0005 Staging: hv: Get rid of the forward declaration of blkvsc_init_rw() So I'll stop at this patch in the series. As I look at the git log in today's tree, the topmost patch of mine that is applied is: Staging: hv: Get rid of blkvsc_check_events() This is a patch that is not even in the patch-set that you were currently applying. On April 6th, I sent three patch-sets (all these 3 were resends after the tree opened): -Staging-hv-Cleanup-storage-drivers-phase-II-fixup: This was a two patch series to fix-up some bugs. This has been applied. -Staging-hv-Cleanup-storage-drivers-phase-III: This was a 22 patch-set of which you applied 4 patches and the application of the fifth caused a failure (as noted by you in this mail). -Staging-hv-Cleanup-storage-drivers-phase-IV: This was a 22 patch set That should have been applied last. For some reason, after you applied 4 patches from the second set, looking at the git log, it looks like a patch from the third set was applied: Subject: [PATCH 01/22] Staging: hv: Get rid of blkvsc_check_events() This is the reason why [PATCH 05/22] Staging: hv: Get rid of the forward declaration of blkvsc_init_rw() failed to apply. I am curious if I did something wrong that might have caused this problem. With regards to rectifying this problem, if you can roll-back that last patch that was applied out of order, then I think everything should apply just fine. If this is not possible, I would need to re-spin the 40 odd patches that currently fail to apply. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 05/22] Staging: hv: Get rid of the forward declaration of blkvsc_init_rw()
-Original Message- From: devel-boun...@linuxdriverproject.org [mailto:devel- boun...@linuxdriverproject.org] On Behalf Of KY Srinivasan Sent: Thursday, April 21, 2011 8:41 PM To: Greg KH Cc: Abhishek Kane (Mindtree Consulting PVT LTD); Haiyang Zhang; gre...@suse.de; linux-ker...@vger.kernel.org; virtualizat...@lists.osdl.org; de...@linuxdriverproject.org Subject: RE: [PATCH 05/22] Staging: hv: Get rid of the forward declaration of blkvsc_init_rw() -Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Wednesday, April 20, 2011 4:35 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 05/22] Staging: hv: Get rid of the forward declaration of blkvsc_init_rw() On Wed, Apr 06, 2011 at 02:35:03PM -0700, K. Y. Srinivasan wrote: Get rid of the forward declaration of blkvsc_init_rw() by moving the code around. This patch fails to apply: Applying: Staging: hv: Get rid of the forward declaration of blkvsc_init_rw() error: patch failed: drivers/staging/hv/blkvsc_drv.c:1000 error: drivers/staging/hv/blkvsc_drv.c: patch does not apply Patch failed at 0005 Staging: hv: Get rid of the forward declaration of blkvsc_init_rw() So I'll stop at this patch in the series. As I look at the git log in today's tree, the topmost patch of mine that is applied is: Staging: hv: Get rid of blkvsc_check_events() This is a patch that is not even in the patch-set that you were currently applying. On April 6th, I sent three patch-sets (all these 3 were resends after the tree opened): -Staging-hv-Cleanup-storage-drivers-phase-II-fixup: This was a two patch series to fix-up some bugs. This has been applied. -Staging-hv-Cleanup-storage-drivers-phase-III: This was a 22 patch-set of which you applied 4 patches and the application of the fifth caused a failure (as noted by you in this mail). -Staging-hv-Cleanup-storage-drivers-phase-IV: This was a 22 patch set That should have been applied last. For some reason, after you applied 4 patches from the second set, looking at the git log, it looks like a patch from the third set was applied: Subject: [PATCH 01/22] Staging: hv: Get rid of blkvsc_check_events() This is the reason why [PATCH 05/22] Staging: hv: Get rid of the forward declaration of blkvsc_init_rw() failed to apply. I am curious if I did something wrong that might have caused this problem. With regards to rectifying this problem, if you can roll-back that last patch that was applied out of order, then I think everything should apply just fine. If this is not possible, I would need to re-spin the 40 odd patches that currently fail to apply. Greg, I am pretty much done with spinning up the patches that are yet to be applied with against this morning's tree. I will be sending them to you shortly. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: Hyper-V vmbus driver
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Saturday, April 23, 2011 11:21 AM To: Greg KH Cc: KY Srinivasan; de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; virtualizat...@lists.osdl.org Subject: Re: Hyper-V vmbus driver On Mon, Apr 11, 2011 at 12:07:08PM -0700, Greg KH wrote: Due to other external issues, my patch backlog is still not gotten through yet, sorry. Sometimes real life intrudes on the best of plans. I'll get to this when I get through the rest of your hv patches, and the other patches pending that I have in my queues. Thanks Greg. The latest re-send of my hv patches are against the tree: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging-2.6.git that I picked up on April 22, 2011. I hope there won't be any issues this time around. But, I would recommend you going through and looking at the code and verifying that you feel the bus code is ready. At a very quick glance, you should not have individual drivers have to set their 'struct device' pointers directly, that is something that the bus does, not the driver. The driver core will call your bus and your bus will then do the matching and call the probe function of the driver if needed. Are you referring to the fact that in the vmbus_match function, the current code binds the device specific driver to the corresponding hv_device structure? See the PCI driver structure for an example of this if you are curious. It should also allow you to get rid of that unneeded *priv pointer in the struct hv_driver. I am pretty sure, I can get rid of this. The way this code was originally structured, in the vmbus_match() function, you needed to get at the device specific driver pointer so that we could do the binding between the hv_device and the correspond device specific driver. The earlier code depended on the structure layout to map a pointer to the hv_driver to the corresponding device specific driver (net, block etc.) To get rid of this layout dependency, I introduced an addition field (priv) in the hv_driver. There is, I suspect sufficient state available to: (a) Not require the vmbus_match() function to do the binding. (b) And to get at the device specific driver structure from the generic driver structure without having to have an explicit mapping maintained in the hv_driver structure. Before, I go ahead and make these changes, Greg, can you confirm if I have captured your concerns correctly. You should be able to set that structure constant, like all other busses. Right now you can not which shows a design issue. I am a little confused here. While I agree with you that perhaps we could get rid the priv element in the hv_driver structure, what else would you want done here. So, take a look at that and let me know what you think. Once I hear from you, I will work on getting rid of the priv pointer from hv_driver structure as well as the code that currently does the binding in vmbus_match. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: Hyper-V vmbus driver
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Sunday, April 24, 2011 8:14 PM To: KY Srinivasan Cc: Greg KH; de...@linuxdriverproject.org; linux-ker...@vger.kernel.org; virtualizat...@lists.osdl.org Subject: Re: Hyper-V vmbus driver On Sun, Apr 24, 2011 at 04:18:24PM +, KY Srinivasan wrote: On Mon, Apr 11, 2011 at 12:07:08PM -0700, Greg KH wrote: Due to other external issues, my patch backlog is still not gotten through yet, sorry. Sometimes real life intrudes on the best of plans. I'll get to this when I get through the rest of your hv patches, and the other patches pending that I have in my queues. Thanks Greg. The latest re-send of my hv patches are against the tree: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging-2.6.git that I picked up on April 22, 2011. I hope there won't be any issues this time around. Me too :) Just curious; when are you planning to drain the hv patch queue next. But, I would recommend you going through and looking at the code and verifying that you feel the bus code is ready. At a very quick glance, you should not have individual drivers have to set their 'struct device' pointers directly, that is something that the bus does, not the driver. The driver core will call your bus and your bus will then do the matching and call the probe function of the driver if needed. Are you referring to the fact that in the vmbus_match function, the current code binds the device specific driver to the corresponding hv_device structure? Yes, that's the problem (well, kind of the problem.) You seem to be doing things a bit odd and that's due to the old way the code was written. First off, don't embed a struct bus_type in another structure, that's not needed at all. Why is that done? Anyway... Currently, struct bus_type is embedded in struct hv_bus that has very minimal additional state. I will clean this up. In your vmbus_match function, you should be matching to see if your device matches the driver that is passed to you. You do this by looking at some type of id. For the vmbus you should do this by looking at the GUID, right? And it looks like you do do this, so that's fine. And then your vmbus_probe() function calls the driver probe function, with the device it is to bind to. BUT, you need to have your probe function pass in the correct device type (i.e. struct hv_device, NOT struct device.) I will clean this up. That way, your hv_driver will have a type all its own, with probe functions that look nothing like the probe functions that 'struct driver' has in it. Look at 'struct pci_driver' for an example of this. Don't try to overload the probe/remove/suspend/etc functions of your hv_driver by using the base 'struct device_driver' callbacks, that's putting knowledge of the driver core into the individual hv drivers, where it's not needed at all. And, by doing that, you should be able to drop your private pointer in the hv_driver function completly, right? That shouldn't be needed at all. After sending you the mail this afternoon, I worked on patches that do exactly that. I did this with the current model where probe/remove/ etc. get a pointer to struct device. Within a specific driver you can always map a struct device pointer to the class specific device driver. I will keep that code; I will however do what you are suggesting here and make probe/remove etc. take a pointer to struct hv_device. See the PCI driver structure for an example of this if you are curious. It should also allow you to get rid of that unneeded *priv pointer in the struct hv_driver. I am pretty sure, I can get rid of this. The way this code was originally structured, in the vmbus_match() function, you needed to get at the device specific driver pointer so that we could do the binding between the hv_device and the correspond device specific driver. The earlier code depended on the structure layout to map a pointer to the hv_driver to the corresponding device specific driver (net, block etc.) To get rid of this layout dependency, I introduced an addition field (priv) in the hv_driver. There is, I suspect sufficient state available to: (a) Not require the vmbus_match() function to do the binding. No, you still want that, see above. The current code has the following assignment after a match is found: device_ctx-drv = drv-priv; What I meant was that I would get rid of this assignment (binding) since I can get that information quite easily in the class specific (net, block, etc.) where it is needed. (b) And to get at the device specific driver structure from the generic driver structure without having to have an explicit mapping maintained in the hv_driver structure. Kind of, see above for more details. If you want a good example, again, look at the PCI core code, it's
RE: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Tuesday, April 26, 2011 12:57 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code Do you have a repository containing the current state of your patche somewhere? There's been so much cleanup that it's hard to review these patches against the current mainline codebase. Christoph, Yesterday (April 25, 2011), Greg checked in all of the outstanding hv patches. So, if You checkout Greg's tree today, you will get the most recent hv codebase. This current patch-set is against Greg's current tree. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 18/25] Staging: hv: Get rid of struct hv_bus
-Original Message-
From: Greg KH [mailto:gre...@suse.de]
Sent: Tuesday, April 26, 2011 3:41 PM
To: KY Srinivasan
Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree
Consulting
PVT LTD); Hank Janssen
Subject: Re: [PATCH 18/25] Staging: hv: Get rid of struct hv_bus
On Tue, Apr 26, 2011 at 09:20:35AM -0700, K. Y. Srinivasan wrote:
Now, get rid of struct hv_bus. We will no longer be embedding
struct bus_type.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Abhishek Kane v-abk...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/vmbus_drv.c | 33 ++---
1 files changed, 14 insertions(+), 19 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index a3a7741..515311c 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -45,11 +45,6 @@ static struct pci_dev *hv_pci_dev;
static struct tasklet_struct msg_dpc;
static struct tasklet_struct event_dpc;
-/* Main vmbus driver data structure */
-struct hv_bus {
- struct bus_type bus;
-};
-
unsigned int vmbus_loglevel = (ALL_MODULES 16 | INFO_LVL);
EXPORT_SYMBOL(vmbus_loglevel);
/* (ALL_MODULES 16 | DEBUG_LVL_ENTEREXIT); */
@@ -405,14 +400,14 @@ static void vmbus_device_release(struct device
*device)
}
/* The one and only one */
-static struct hv_bus hv_bus = {
- .bus.name = vmbus,
- .bus.match =vmbus_match,
- .bus.shutdown = vmbus_shutdown,
- .bus.remove = vmbus_remove,
- .bus.probe =vmbus_probe,
- .bus.uevent = vmbus_uevent,
- .bus.dev_attrs =vmbus_device_attrs,
+static struct bus_type hv_bus = {
+ .name = vmbus,
+ .match =vmbus_match,
+ .shutdown = vmbus_shutdown,
+ .remove = vmbus_remove,
+ .probe =vmbus_probe,
+ .uevent = vmbus_uevent,
+ .dev_attrs =vmbus_device_attrs,
};
static const char *driver_name = hyperv;
@@ -550,14 +545,14 @@ static int vmbus_bus_init(struct pci_dev *pdev)
goto cleanup;
}
- hv_bus.bus.name = driver_name;
+ hv_bus.name = driver_name;
Why are you setting the name of the bus again? Shouldn't this line be
removed?
You are absolutely right. Since this redundancy was in the existing
code, should I send you a separate patch to fix this?
Regards,
K. Y
thanks,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 18/25] Staging: hv: Get rid of struct hv_bus
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Tuesday, April 26, 2011 4:58 PM To: KY Srinivasan Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD); Hank Janssen Subject: Re: [PATCH 18/25] Staging: hv: Get rid of struct hv_bus On Tue, Apr 26, 2011 at 08:23:25PM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Tuesday, April 26, 2011 3:41 PM To: KY Srinivasan Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD); Hank Janssen Subject: Re: [PATCH 18/25] Staging: hv: Get rid of struct hv_bus On Tue, Apr 26, 2011 at 09:20:35AM -0700, K. Y. Srinivasan wrote: Now, get rid of struct hv_bus. We will no longer be embedding struct bus_type. Signed-off-by: K. Y. Srinivasan k...@microsoft.com Signed-off-by: Haiyang Zhang haiya...@microsoft.com Signed-off-by: Abhishek Kane v-abk...@microsoft.com Signed-off-by: Hank Janssen hjans...@microsoft.com --- drivers/staging/hv/vmbus_drv.c | 33 ++--- 1 files changed, 14 insertions(+), 19 deletions(-) - hv_bus.bus.name = driver_name; + hv_bus.name = driver_name; Why are you setting the name of the bus again? Shouldn't this line be removed? You are absolutely right. Since this redundancy was in the existing code, should I send you a separate patch to fix this? A separate one after this series is fine. Done; I have sent the patch out. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, April 26, 2011 7:29 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code On Tue, Apr 26, 2011 at 09:19:45AM -0700, K. Y. Srinivasan wrote: This patch-set addresses some of the bus/driver model cleanup that Greg sugested over the last couple of days. In this patch-set we deal with the following issues: 1) Cleanup unnecessary state in struct hv_device and struct hv_driver to be compliant with the Linux Driver model. 2) Cleanup the vmbus_match() function to conform with the Linux Driver model. 3) Cleanup error handling in the vmbus_probe() and vmbus_child_device_register() functions. Fixed a bug in the probe failure path as part of this cleanup. 4) The Windows host cannot handle the vmbus_driver being unloaded and subsequently loaded. Cleanup the driver with this in mind. I've stopped at this patch (well, I applied one more, but you can see that.) I'd like to get some confirmation that this is really what you all want to do here before applying it. If it is, care to resend them with a bit more information about this issue and why you all are making it? Greg, this is restriction imposed by the Windows host: you cannot reload the Vmbus driver without rebooting the guest. If you cannot re-load, what good is it to be able to unload? Distros that integrate these drivers will load these drivers automatically on boot and there is not much point in being able to unload this since most likely the root device will be handled by these drivers. For systems that don't integrate these drivers; I don't see much point in allowing the driver to be unloaded, if you cannot reload the driver without rebooting the guest. If and when the Windows host supports reloading the vmbus driver, we can very easily add this functionality. The situation currently at best very misleading - you think you can unload the vmbus driver, only to discover that you have to reboot the guest! Anyway, other than this one, the series looks good. But you should follow-up with some driver structure changes like what Christoph said to do. I will send you a patch for this. After that, do you want another round of review of the code, or do you have more things you want to send in (like the name[64] removal?) I would prefer that we go through the review process. What is the process for this review? Is there a time window for people to respond. I am hoping I will be able to address all the review comments well in advance of the next closing of the tree, with the hope of taking the vmbus driver out of staging this go around (hope springs eternal in the human breast ...)! Regards, K. Y thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 11/25] Staging: hv: Get rid of the drv field in struct hv_device
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Tuesday, April 26, 2011 6:57 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang;
Abhishek Kane (Mindtree Consulting PVT LTD); Hank Janssen
Subject: Re: [PATCH 11/25] Staging: hv: Get rid of the drv field in struct
hv_device
On Tue, Apr 26, 2011 at 09:20:28AM -0700, K. Y. Srinivasan wrote:
Now, we can rid of the drv field in struct hv_device.
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Abhishek Kane v-abk...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/vmbus_api.h |3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/hv/vmbus_api.h b/drivers/staging/hv/vmbus_api.h
index 51fa952..02e3587 100644
--- a/drivers/staging/hv/vmbus_api.h
+++ b/drivers/staging/hv/vmbus_api.h
@@ -103,9 +103,6 @@ struct hv_driver {
/* Base device object */
struct hv_device {
- /* the driver for this device */
- struct hv_driver *drv;
-
char name[64];
FYI, in the future, you can also remove this name[64] field as well as I
don't think it's ever used...
I will send you a patch for this, once all the patches in this series are
applied.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 12/25] Staging: hv: Cleanup error handling in vmbus_child_device_register()
-Original Message-
From: Greg KH [mailto:g...@kroah.com]
Sent: Tuesday, April 26, 2011 6:51 PM
To: KY Srinivasan
Cc: gre...@suse.de; linux-ker...@vger.kernel.org;
de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang;
Abhishek Kane (Mindtree Consulting PVT LTD)
Subject: Re: [PATCH 12/25] Staging: hv: Cleanup error handling in
vmbus_child_device_register()
On Tue, Apr 26, 2011 at 09:20:29AM -0700, K. Y. Srinivasan wrote:
Cleanup error handling in vmbus_child_device_register().
Signed-off-by: K. Y. Srinivasan k...@microsoft.com
Signed-off-by: Haiyang Zhang haiya...@microsoft.com
Signed-off-by: Abhishek Kane v-abk...@microsoft.com
Signed-off-by: Hank Janssen hjans...@microsoft.com
---
drivers/staging/hv/vmbus_drv.c |7 ++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index d597dd4..4d569ad 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -720,11 +720,16 @@ int vmbus_child_device_register(struct hv_device
*child_device_obj)
*/
ret = device_register(child_device_obj-device);
+ if (ret)
+ return ret;
+
/* vmbus_probe() error does not get propergate to device_register(). */
ret = child_device_obj-probe_error;
Wait, why not? Why is the probe_error have to be saved off like this?
That seems like something is wrong here, this patch should not be
needed.
Well, you should check the return value of device_register, that is
needed, but this seems broken somehow.
The current code had comments claiming that the probe error was not
correctly propagated. Looking at the kernel side of the code, it was not clear
if device_register() could succeed while the probe might fail. In any event,
if you can guarantee that device_register() can return any probe related
errors, I agree with you that saving the probe error is an overkill. The
current code
saved the probe error and with new check I added with regards to the return
value of device_register, there is no correctness issue with this patch.
- if (ret)
+ if (ret) {
pr_err(Unable to register child device\n);
+ device_unregister(child_device_obj-device);
+ }
else
} else
is the preferred way.
Care to send a fixup patch to remove the probe_error field and fix this
formatting error up?
I will fix up the formatting issue.
Regards,
K. Y
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Wednesday, April 27, 2011 2:46 AM To: KY Srinivasan Cc: Greg KH; gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code On Wed, Apr 27, 2011 at 01:54:02AM +, KY Srinivasan wrote: I would prefer that we go through the review process. What is the process for this review? Is there a time window for people to respond. I am hoping I will be able to address all the review comments well in advance of the next closing of the tree, with the hope of taking the vmbus driver out of staging this go around (hope springs eternal in the human breast ...)! It would be useful if you'd send one driver at a time to the list as the full source to review. Did we make any progress on the naming discussion? In my opinion hv is a far to generic name for your drivers. Why not call it mshv dor the driver directory and prefixes? This topic was discussed at some great length back in Feb/March when I did a bunch of cleanup with regards how the driver and device data structures were layered. At that point, the consensus was to keep the hv prefix. As far as the core code is concerned, can you explain the use of the dev_add, dev_rm and cleanup methods and how they relate to the normal probe/remove/shutdown methods? While I am currently cleaning up our block drivers, my goal this go around is to work on getting the vmbus driver out of staging. I am hoping when I am ready for having you guys review the storage drivers, I will have dealt with the issues you raise here. As far as the storage drivers are concerned I still have issues with the architecture. I haven't seen any good explanation why you want to have the blkvsc and storvsc drivers different from each other. They both speak the same vmbus-level protocol and tunnel scsi commands over it. Why would you sometimes expose this SCSI protocol as a SCSI LLDD and sometimes as a block driver? What decides that a device is exported in a way to that blkvsc is bound to them vs storvsc? How do they look like on the windows side? From my understanding of the windows driver models both the recent storport model and the older scsiport model are more or less talking scsi to the driver anyway, so what is the difference between the two for a Windows guest? I had written up a brief note that I had sent out setting the stage for the first patch-set for cleaning up the block drivers. I am copying it here for your convenience: From: K. Y. Srinivasan k...@microsoft.com Date: Tue, 22 Mar 2011 11:54:46 -0700 Subject: [PATCH 00/16] Staging: hv: Cleanup storage drivers - Phase I This is first in a series of patch-sets aimed at cleaning up the storage drivers for Hyper-V. Before I get into the details of this patch-set, I think it is useful to give a brief overview of the storage related front-end drivers currently in the tree for Linux on Hyper-V: On the host side, Windows emulates the standard PC hardware to permit hosting of fully virtualized operating systems. To enhance disk I/O performance, we support a virtual block driver. This block driver currently handles disks that have been setup as IDE disks for the guest - as specified in the guest configuration. On the SCSI side, we emulate a SCSI HBA. Devices configured under the SCSI controller for the guest are handled via this emulated HBA (SCSI front-end). So, SCSI disks configured for the guest are handled through native SCSI upper-level drivers. If this SCSI front-end driver is not loaded, currently, the guest cannot see devices that have been configured as SCSI devices. So, while the virtual block driver described earlier could potentially handle all block devices, the implementation choices made on the host will not permit it. Also, the only SCSI device that can be currently configured for the guest is a disk device. Both the block device driver (hv_blkvsc) and the SCSI front-end driver (hv_storvsc) communicate with the host via unique channels that are implemented as bi-directional ring buffers. Each (storage) channel carries with it enough state to uniquely identify the device on the host side. Microsoft has chosen to use SCSI verbs for this storage channel communication. Also pleae get rid of struct storvsc_driver_object, it's just a very strange way to store file-scope variables, and useless indirection for the I/O submission handler. I will do this as part of storage cleanup I am currently doing. Thank you for taking the time to review the code. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 12/25] Staging: hv: Cleanup error handling in vmbus_child_device_register()
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Wednesday, April 27, 2011 8:26 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 12/25] Staging: hv: Cleanup error handling in vmbus_child_device_register() On Wed, Apr 27, 2011 at 02:11:48AM +, KY Srinivasan wrote: -Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Tuesday, April 26, 2011 6:51 PM To: KY Srinivasan Cc: gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 12/25] Staging: hv: Cleanup error handling in vmbus_child_device_register() On Tue, Apr 26, 2011 at 09:20:29AM -0700, K. Y. Srinivasan wrote: Cleanup error handling in vmbus_child_device_register(). Signed-off-by: K. Y. Srinivasan k...@microsoft.com Signed-off-by: Haiyang Zhang haiya...@microsoft.com Signed-off-by: Abhishek Kane v-abk...@microsoft.com Signed-off-by: Hank Janssen hjans...@microsoft.com --- drivers/staging/hv/vmbus_drv.c |7 ++- 1 files changed, 6 insertions(+), 1 deletions(-) diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c index d597dd4..4d569ad 100644 --- a/drivers/staging/hv/vmbus_drv.c +++ b/drivers/staging/hv/vmbus_drv.c @@ -720,11 +720,16 @@ int vmbus_child_device_register(struct hv_device *child_device_obj) */ ret = device_register(child_device_obj-device); + if (ret) + return ret; + /* vmbus_probe() error does not get propergate to device_register(). */ ret = child_device_obj-probe_error; Wait, why not? Why is the probe_error have to be saved off like this? That seems like something is wrong here, this patch should not be needed. Well, you should check the return value of device_register, that is needed, but this seems broken somehow. The current code had comments claiming that the probe error was not correctly propagated. Looking at the kernel side of the code, it was not clear if device_register() could succeed while the probe might fail. Of course it can, device_register() has nothing to do with the probe callback of the device itself. To think otherwise is to not understand the driver model and assume things that you should never be caring about. Think about it, if you register a device, you don't know at that point in time if a driver is currently loaded for it, and that it will be bound to that device. Nor do you care, as any needed notifications for new drivers will be sent to userspace, and they will be loaded at some random time in the future. So a probe() call might never be called for this device until some other time, running on some other processor, in some other thread. Drivers are allowed to return errors from their probe functions for valid reasons (i.e. this driver shouldn't bind to this device for a variety of good reasons.) No one cares about this, as the driver core handles it properly and will pass on to the next driver in the list that might be able to be bound to this device. So why do you care about the return value of the probe() call? It gets properly handled already by the driver core, why would your bus ever care about it? (Hint, no other bus does, as it makes no sense.) In any event, if you can guarantee that device_register() can return any probe related errors, I agree with you that saving the probe error is an overkill. The current code saved the probe error and with new check I added with regards to the return value of device_register, there is no correctness issue with this patch. As explained above, no, it will not return a probe error, as that makes no sense. If the code is wanting to rely on this, it is broken and must be fixed. I did not say that device_register would return the probe_error. On the contrary, the code I added explicitly ensures that proper cleanup is done for the failure of device_register and if the probe function were called on the same context executing the device_register call, the failure of the probe call as well (looking at the stack trace while in the probe function, this appeared to be the case currently). If you look at the existing code; the current code did not deal with the failure of device_register() - it over-writes the return value of device_register with that of the probe error. This patch fixed that problem. The current code dealt with the probe error by spinning up a work item to deal with the probe failure. This work item would try to unregister the device that was not fully registered and this caused a problem. I tried
RE: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
-Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Wednesday, April 27, 2011 8:19 AM To: KY Srinivasan Cc: Christoph Hellwig; Greg KH; gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code On Wed, Apr 27, 2011 at 11:47:03AM +, KY Srinivasan wrote: On the host side, Windows emulates the standard PC hardware to permit hosting of fully virtualized operating systems. To enhance disk I/O performance, we support a virtual block driver. This block driver currently handles disks that have been setup as IDE disks for the guest - as specified in the guest configuration. On the SCSI side, we emulate a SCSI HBA. Devices configured under the SCSI controller for the guest are handled via this emulated HBA (SCSI front-end). So, SCSI disks configured for the guest are handled through native SCSI upper-level drivers. If this SCSI front-end driver is not loaded, currently, the guest cannot see devices that have been configured as SCSI devices. So, while the virtual block driver described earlier could potentially handle all block devices, the implementation choices made on the host will not permit it. Also, the only SCSI device that can be currently configured for the guest is a disk device. Both the block device driver (hv_blkvsc) and the SCSI front-end driver (hv_storvsc) communicate with the host via unique channels that are implemented as bi-directional ring buffers. Each (storage) channel carries with it enough state to uniquely identify the device on the host side. Microsoft has chosen to use SCSI verbs for this storage channel communication. This doesn't really explain much at all. The only important piece of information I can read from this statement is that both blkvsc and storvsc only support disks, but not any other kind of device, and that chosing either one is an arbitrary seletin when setting up a VM configuration. But this still isn't an excuse to implement a block layer driver for a SCSI protocol, and it doesn't not explain in what way the two protocols actually differ. You really should implement blksvs as a SCSI LLDD, too - and from the looks of it it doesn't even have to be a separate one, but just adding the ids to storvsc would do the work. On the host-side, as part of configuring a guest you can specify block devices as being under an IDE controller or under a SCSI controller. Those are the only options you have. Devices configured under the IDE controller cannot be seen in the guest under the emulated SCSI front-end which is the scsi driver (storvsc_drv). So, when you do a bus scan in the emulated scsi front-end, the devices enumerated will not include block devices configured under the IDE controller. So, it is not clear to me how I can do what you are proposing given the restrictions imposed by the host. Regards, K. Y ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
-Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Friday, April 29, 2011 12:40 PM To: KY Srinivasan Cc: Christoph Hellwig; gre...@suse.de; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code On Fri, Apr 29, 2011 at 04:32:35PM +, KY Srinivasan wrote: -Original Message- From: Christoph Hellwig [mailto:h...@infradead.org] Sent: Wednesday, April 27, 2011 8:19 AM To: KY Srinivasan Cc: Christoph Hellwig; Greg KH; gre...@suse.de; linux- ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org Subject: Re: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code On Wed, Apr 27, 2011 at 11:47:03AM +, KY Srinivasan wrote: On the host side, Windows emulates the standard PC hardware to permit hosting of fully virtualized operating systems. To enhance disk I/O performance, we support a virtual block driver. This block driver currently handles disks that have been setup as IDE disks for the guest - as specified in the guest configuration. On the SCSI side, we emulate a SCSI HBA. Devices configured under the SCSI controller for the guest are handled via this emulated HBA (SCSI front-end). So, SCSI disks configured for the guest are handled through native SCSI upper-level drivers. If this SCSI front-end driver is not loaded, currently, the guest cannot see devices that have been configured as SCSI devices. So, while the virtual block driver described earlier could potentially handle all block devices, the implementation choices made on the host will not permit it. Also, the only SCSI device that can be currently configured for the guest is a disk device. Both the block device driver (hv_blkvsc) and the SCSI front-end driver (hv_storvsc) communicate with the host via unique channels that are implemented as bi-directional ring buffers. Each (storage) channel carries with it enough state to uniquely identify the device on the host side. Microsoft has chosen to use SCSI verbs for this storage channel communication. This doesn't really explain much at all. The only important piece of information I can read from this statement is that both blkvsc and storvsc only support disks, but not any other kind of device, and that chosing either one is an arbitrary seletin when setting up a VM configuration. But this still isn't an excuse to implement a block layer driver for a SCSI protocol, and it doesn't not explain in what way the two protocols actually differ. You really should implement blksvs as a SCSI LLDD, too - and from the looks of it it doesn't even have to be a separate one, but just adding the ids to storvsc would do the work. On the host-side, as part of configuring a guest you can specify block devices as being under an IDE controller or under a SCSI controller. Those are the only options you have. Devices configured under the IDE controller cannot be seen in the guest under the emulated SCSI front- end which is the scsi driver (storvsc_drv). Are you sure the libata core can't see this ide controller and connect to it? That way you would use the scsi system if you do that and you would need a much smaller ide driver, perhaps being able to merge it with your scsi driver. If we don't load the blkvsc driver, the emulated IDE controller exposed to the guest can and will be seen by the libata core. In this case though, your disk I/O will be taking the emulated path with the usual performance hit. When you load the blkvsc driver, the device access does not go through the emulated IDE controller. Blkvsc is truly a generic block driver that registers as a block driver in the guest and talks to an appropriate device driver on the host, communicating over the vmbus. In this respect, it is identical to block drivers we have for guests in other virtualization platforms (Xen etc.). The only difference is that on the host side, the only way you can assign a scsi disk to the guest is to configure this scsi disk under the scsi controller. So, while blkvsc is a generic block driver, because of the restrictions on the host side, it only ends up managing block devices that have IDE majors. We really don't want to write new IDE drivers anymore that don't use libata. As I noted earlier, it is incorrect to view Hyper-V blkvsc driver as an IDE driver. There is nothing IDE specific about it. It is very much like other block front-end drivers (like in Xen) that get their device information from the host and register the block device accordingly with the guest. It just happens that in the current version of the Windows host, only devices that are configured as IDE devices in the host end up being managed by this driver. To make this clear, in my recent
RE: [PATCH 08/25] Staging: hv: vmbus_driver cannot be unloaded; cleanup accordingly
-Original Message- From: Greg KH [mailto:gre...@suse.de] Sent: Friday, April 29, 2011 11:11 AM To: KY Srinivasan Cc: Greg KH; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; virtualizat...@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting PVT LTD) Subject: Re: [PATCH 08/25] Staging: hv: vmbus_driver cannot be unloaded; cleanup accordingly On Fri, Apr 29, 2011 at 01:49:21PM +, KY Srinivasan wrote: Just resend it with some more comments as I explained, and the rest, and I'll queue them up when I get caught up on my patch queue (I only have 381 to get through, the end is near!) Thanks Greg; soon after I sent you this patch-set, I had also sent a single patch to address a redundant assignment: 0001-Staging-hv-Do-not-re-set-the-bus-name.patch Please drop this patch. I will deal with this issue as part of the remaining patch-set I will be sending now. Regards, K. Y thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
