Re: [PATCH 7/7] [v5] drivers/virt: introduce Freescale hypervisor management driver
On Thu, Jun 09, 2011 at 10:33:07PM +0200, Arnd Bergmann wrote: On Thursday 09 June 2011 22:18:28 Timur Tabi wrote: Ok, I was really hoping to avoid doing this. Like I said, binary compatibility is important, and changing the type will break my existing apps. Are you insisting that I pick a new number? I definitely insist that you have a proper interface in the driver at the time that it gets merged, and that probably includes a collision-free ioctl code. This sort of stuff is one of the issues that should be being factored in to any decision not to publish and submit the kernel code - ABIs that haven't been reviewed upstream may well have this sort of issue. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
[PATCH 7/7] [v5] drivers/virt: introduce Freescale hypervisor management driver
Add the drivers/virt directory, which houses drivers that support
virtualization environments, and add the Freescale hypervisor management
driver.
The Freescale hypervisor management driver provides several services to
drivers and applications related to the Freescale hypervisor:
1. An ioctl interface for querying and managing partitions
2. A file interface to reading incoming doorbells
3. An interrupt handler for shutting down the partition upon receiving the
shutdown doorbell from a manager partition
4. A kernel interface for receiving callbacks when a managed partition
shuts down.
Signed-off-by: Timur Tabi ti...@freescale.com
---
drivers/Kconfig|2 +
drivers/Makefile |3 +
drivers/virt/Kconfig | 32 ++
drivers/virt/Makefile |5 +
drivers/virt/fsl_hypervisor.c | 983
include/linux/Kbuild |1 +
include/linux/fsl_hypervisor.h | 231 ++
7 files changed, 1257 insertions(+), 0 deletions(-)
create mode 100644 drivers/virt/Kconfig
create mode 100644 drivers/virt/Makefile
create mode 100644 drivers/virt/fsl_hypervisor.c
create mode 100644 include/linux/fsl_hypervisor.h
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 557a469..0371680 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -122,4 +122,6 @@ source drivers/hwspinlock/Kconfig
source drivers/clocksource/Kconfig
+source drivers/virt/Kconfig
+
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 3f135b6..bbe2918 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -119,3 +119,6 @@ obj-y += ieee802154/
obj-y += clk/
obj-$(CONFIG_HWSPINLOCK) += hwspinlock/
+
+# Virtualization drivers
+obj-$(CONFIG_VIRT_DRIVERS) += virt/
diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
new file mode 100644
index 000..2dcdbc9
--- /dev/null
+++ b/drivers/virt/Kconfig
@@ -0,0 +1,32 @@
+#
+# Virtualization support drivers
+#
+
+menuconfig VIRT_DRIVERS
+ bool Virtualization drivers
+ ---help---
+ Say Y here to get to see options for device drivers that support
+ virtualization environments.
+
+ If you say N, all options in this submenu will be skipped and
disabled.
+
+if VIRT_DRIVERS
+
+config FSL_HV_MANAGER
+ tristate Freescale hypervisor management driver
+ depends on FSL_SOC
+ help
+ The Freescale hypervisor management driver provides several services
+ to drivers and applications related to the Freescale hypervisor:
+
+ 1) An ioctl interface for querying and managing partitions.
+
+ 2) A file interface to reading incoming doorbells.
+
+ 3) An interrupt handler for shutting down the partition upon
+receiving the shutdown doorbell from a manager partition.
+
+ 4) A kernel interface for receiving callbacks when a managed
+partition shuts down.
+
+endif
diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
new file mode 100644
index 000..c47f04d
--- /dev/null
+++ b/drivers/virt/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for drivers that support virtualization
+#
+
+obj-$(CONFIG_FSL_HV_MANAGER) += fsl_hypervisor.o
diff --git a/drivers/virt/fsl_hypervisor.c b/drivers/virt/fsl_hypervisor.c
new file mode 100644
index 000..086085f
--- /dev/null
+++ b/drivers/virt/fsl_hypervisor.c
@@ -0,0 +1,983 @@
+/*
+ * Freescale Hypervisor Management Driver
+
+ * Copyright (C) 2008-2011 Freescale Semiconductor, Inc.
+ * Author: Timur Tabi ti...@freescale.com
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2. This program is licensed as is without any warranty of any
+ * kind, whether express or implied.
+ *
+ * The Freescale hypervisor management driver provides several services to
+ * drivers and applications related to the Freescale hypervisor:
+ *
+ * 1. An ioctl interface for querying and managing partitions.
+ *
+ * 2. A file interface to reading incoming doorbells.
+ *
+ * 3. An interrupt handler for shutting down the partition upon receiving the
+ *shutdown doorbell from a manager partition.
+ *
+ * 4. A kernel interface for receiving callbacks when a managed partition
+ *shuts down.
+ */
+
+#include linux/kernel.h
+#include linux/module.h
+#include linux/init.h
+#include linux/types.h
+#include linux/err.h
+#include linux/fs.h
+#include linux/miscdevice.h
+#include linux/mm.h
+#include linux/pagemap.h
+#include linux/slab.h
+#include linux/poll.h
+#include linux/of.h
+#include linux/reboot.h
+#include linux/uaccess.h
+#include linux/notifier.h
+
+#include linux/io.h
+#include asm/fsl_hcalls.h
+
+#include linux/fsl_hypervisor.h
+
+static BLOCKING_NOTIFIER_HEAD(failover_subscribers);
+
+/*
+ * Ioctl interface for FSL_HV_IOCTL_PARTITION_RESTART
+ *
+ * Restart a running partition
+ */
+static long ioctl_restart(struct fsl_hv_ioctl_restart __user *p)
+{
Re: [PATCH 7/7] [v5] drivers/virt: introduce Freescale hypervisor management driver
On Thu, 9 Jun 2011 14:13:14 -0500 Timur Tabi wrote:
Add the drivers/virt directory, which houses drivers that support
virtualization environments, and add the Freescale hypervisor management
driver.
The Freescale hypervisor management driver provides several services to
drivers and applications related to the Freescale hypervisor:
1. An ioctl interface for querying and managing partitions
2. A file interface to reading incoming doorbells
3. An interrupt handler for shutting down the partition upon receiving the
shutdown doorbell from a manager partition
4. A kernel interface for receiving callbacks when a managed partition
shuts down.
Signed-off-by: Timur Tabi ti...@freescale.com
---
drivers/Kconfig|2 +
drivers/Makefile |3 +
drivers/virt/Kconfig | 32 ++
drivers/virt/Makefile |5 +
drivers/virt/fsl_hypervisor.c | 983
include/linux/Kbuild |1 +
include/linux/fsl_hypervisor.h | 231 ++
7 files changed, 1257 insertions(+), 0 deletions(-)
create mode 100644 drivers/virt/Kconfig
create mode 100644 drivers/virt/Makefile
create mode 100644 drivers/virt/fsl_hypervisor.c
create mode 100644 include/linux/fsl_hypervisor.h
diff --git a/include/linux/fsl_hypervisor.h b/include/linux/fsl_hypervisor.h
new file mode 100644
index 000..d1ca2b1
--- /dev/null
+++ b/include/linux/fsl_hypervisor.h
@@ -0,0 +1,231 @@
[snip]
+/**
+ * enum fsl_hv_ioctl_cmd - ioctl commands
+ * @FSL_HV_IOCTL_PARTITION_RESTART: restart another partition
+ * @FSL_HV_IOCTL_PARTITION_GET_STATUS: get a partition's status
+ * @FSL_HV_IOCTL_PARTITION_START: boot another partition
+ * @FSL_HV_IOCTL_PARTITION_STOP: stop this or another partition
+ * @FSL_HV_IOCTL_MEMCPY: copy data from one partition to another
+ * @FSL_HV_IOCTL_DOORBELL: ring a doorbell
+ * @FSL_HV_IOCTL_GETPROP: get a property from another guest's device tree
+ * @FSL_HV_IOCTL_SETPROP: set a property in another guest's device tree
+ *
+ * This enum lists the available ioctl commands for the Freescale hypervisor
+ * management driver. The meaning
+ */
+enum fsl_hv_ioctl_cmd {
+ FSL_HV_IOCTL_PARTITION_RESTART = _IOWR(0, 1, struct
fsl_hv_ioctl_restart),
+ FSL_HV_IOCTL_PARTITION_GET_STATUS = _IOWR(0, 2, struct
fsl_hv_ioctl_status),
+ FSL_HV_IOCTL_PARTITION_START = _IOWR(0, 3, struct fsl_hv_ioctl_start),
+ FSL_HV_IOCTL_PARTITION_STOP = _IOWR(0, 4, struct fsl_hv_ioctl_stop),
+ FSL_HV_IOCTL_MEMCPY = _IOWR(0, 5, struct fsl_hv_ioctl_memcpy),
+ FSL_HV_IOCTL_DOORBELL = _IOWR(0, 6, struct fsl_hv_ioctl_doorbell),
+ FSL_HV_IOCTL_GETPROP = _IOWR(0, 7, struct fsl_hv_ioctl_prop),
+ FSL_HV_IOCTL_SETPROP = _IOWR(0, 8, struct fsl_hv_ioctl_prop),
+};
Missing an entry in Documentation/ioctl/ioctl-number.txt for 0 (with conflict!).
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 7/7] [v5] drivers/virt: introduce Freescale hypervisor management driver
Randy Dunlap wrote:
+enum fsl_hv_ioctl_cmd {
+ FSL_HV_IOCTL_PARTITION_RESTART = _IOWR(0, 1, struct
fsl_hv_ioctl_restart),
+ FSL_HV_IOCTL_PARTITION_GET_STATUS = _IOWR(0, 2, struct
fsl_hv_ioctl_status),
+ FSL_HV_IOCTL_PARTITION_START = _IOWR(0, 3, struct fsl_hv_ioctl_start),
+ FSL_HV_IOCTL_PARTITION_STOP = _IOWR(0, 4, struct fsl_hv_ioctl_stop),
+ FSL_HV_IOCTL_MEMCPY = _IOWR(0, 5, struct fsl_hv_ioctl_memcpy),
+ FSL_HV_IOCTL_DOORBELL = _IOWR(0, 6, struct fsl_hv_ioctl_doorbell),
+ FSL_HV_IOCTL_GETPROP = _IOWR(0, 7, struct fsl_hv_ioctl_prop),
+ FSL_HV_IOCTL_SETPROP = _IOWR(0, 8, struct fsl_hv_ioctl_prop),
+};
Missing an entry in Documentation/ioctl/ioctl-number.txt for 0 (with
conflict!).
If I change it from 0, I'm going to break binary compatibility with our apps. I
agree that maybe I shouldn't have picked 0, but considering how many conflicts
there already are, I wonder what the point is. Even if I pick a number that is
currently not listed in the chart, that doesn't mean that it's actually not
being used, or that it won't conflict in the future.
So is it okay to stick with 0, or do I need to pick a new number?
--
Timur Tabi
Linux kernel developer at Freescale
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 7/7] [v5] drivers/virt: introduce Freescale hypervisor management driver
On 06/09/11 12:47, Timur Tabi wrote:
Randy Dunlap wrote:
+enum fsl_hv_ioctl_cmd {
+ FSL_HV_IOCTL_PARTITION_RESTART = _IOWR(0, 1, struct
fsl_hv_ioctl_restart),
+ FSL_HV_IOCTL_PARTITION_GET_STATUS = _IOWR(0, 2, struct
fsl_hv_ioctl_status),
+ FSL_HV_IOCTL_PARTITION_START = _IOWR(0, 3, struct fsl_hv_ioctl_start),
+ FSL_HV_IOCTL_PARTITION_STOP = _IOWR(0, 4, struct fsl_hv_ioctl_stop),
+ FSL_HV_IOCTL_MEMCPY = _IOWR(0, 5, struct fsl_hv_ioctl_memcpy),
+ FSL_HV_IOCTL_DOORBELL = _IOWR(0, 6, struct fsl_hv_ioctl_doorbell),
+ FSL_HV_IOCTL_GETPROP = _IOWR(0, 7, struct fsl_hv_ioctl_prop),
+ FSL_HV_IOCTL_SETPROP = _IOWR(0, 8, struct fsl_hv_ioctl_prop),
+};
Missing an entry in Documentation/ioctl/ioctl-number.txt for 0 (with
conflict!).
If I change it from 0, I'm going to break binary compatibility with our apps.
I
agree that maybe I shouldn't have picked 0, but considering how many conflicts
there already are, I wonder what the point is. Even if I pick a number that
is
currently not listed in the chart, that doesn't mean that it's actually not
being used, or that it won't conflict in the future.
Yes, I understood that.
So is it okay to stick with 0, or do I need to pick a new number?
I wasn't suggesting that you change the 0, just note that it has conflicts,
like other ioctls do.
--
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 7/7] [v5] drivers/virt: introduce Freescale hypervisor management driver
Hi Timur, thanks for addressing the issues I pointed out. Unfortunately, I
have found a few more now:
On Thursday 09 June 2011 21:13:14 Timur Tabi wrote:
+ /* Make sure the application is called the right driver. */
+ if (_IOC_TYPE(cmd) != 0) {
+ pr_debug(fsl-hv: ioctl type %u should be 0\n,
_IOC_TYPE(cmd));
+ return -EINVAL;
+ }
+
+ /* Make sure the application set the direction flag correctly. */
+ if (_IOC_DIR(cmd) != (_IOC_READ | _IOC_WRITE)) {
+ pr_debug(fsl-hv: ioctl direction should be _IOWR\n);
+ return -EINVAL;
+ }
+
+ /*
+* Make sure the application is passing the right structure to us.
+* For backwards compatibility with older applications, we only check
+* if the size is too small, rather than unequal.
+*/
+
+ switch (_IOC_NR(cmd)) {
+ case (_IOC_NR(FSL_HV_IOCTL_PARTITION_RESTART)):
+ size = sizeof(struct fsl_hv_ioctl_restart);
+ if (_IOC_SIZE(cmd) size)
+ goto size_error;
+ ret = ioctl_restart(arg);
+ break;
As mentioned, it would be easier and more readable to just do
switch(cmd) {
case FSL_HV_IOCTL_PARTITION_RESTART:
...
case FSL_HV_IOCTL_PARTITION_GET_STATUS;
...
There is no need to check the bits individually when you can simply
compare the command number.
+/**
+ * enum fsl_hv_ioctl_cmd - ioctl commands
+ * @FSL_HV_IOCTL_PARTITION_RESTART: restart another partition
+ * @FSL_HV_IOCTL_PARTITION_GET_STATUS: get a partition's status
+ * @FSL_HV_IOCTL_PARTITION_START: boot another partition
+ * @FSL_HV_IOCTL_PARTITION_STOP: stop this or another partition
+ * @FSL_HV_IOCTL_MEMCPY: copy data from one partition to another
+ * @FSL_HV_IOCTL_DOORBELL: ring a doorbell
+ * @FSL_HV_IOCTL_GETPROP: get a property from another guest's device tree
+ * @FSL_HV_IOCTL_SETPROP: set a property in another guest's device tree
+ *
+ * This enum lists the available ioctl commands for the Freescale hypervisor
+ * management driver. The meaning
+ */
+enum fsl_hv_ioctl_cmd {
+ FSL_HV_IOCTL_PARTITION_RESTART = _IOWR(0, 1, struct
fsl_hv_ioctl_restart),
+ FSL_HV_IOCTL_PARTITION_GET_STATUS = _IOWR(0, 2, struct
fsl_hv_ioctl_status),
+ FSL_HV_IOCTL_PARTITION_START = _IOWR(0, 3, struct fsl_hv_ioctl_start),
+ FSL_HV_IOCTL_PARTITION_STOP = _IOWR(0, 4, struct fsl_hv_ioctl_stop),
+ FSL_HV_IOCTL_MEMCPY = _IOWR(0, 5, struct fsl_hv_ioctl_memcpy),
+ FSL_HV_IOCTL_DOORBELL = _IOWR(0, 6, struct fsl_hv_ioctl_doorbell),
+ FSL_HV_IOCTL_GETPROP = _IOWR(0, 7, struct fsl_hv_ioctl_prop),
+ FSL_HV_IOCTL_SETPROP = _IOWR(0, 8, struct fsl_hv_ioctl_prop),
+};
Using a #define here is usually preferred because then you can use #ifdef in a
user
application to check if a given value has been assigned.
More importantly, the code you have chose (0) conflicts with existing drivers
(frame buffer, scsi and wavefront among others). Please chose a free one and
add it to Documentation/ioctl/ioctl-number.txt in the same patch.
Arnd
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 7/7] [v5] drivers/virt: introduce Freescale hypervisor management driver
Arnd Bergmann wrote:
As mentioned, it would be easier and more readable to just do
switch(cmd) {
case FSL_HV_IOCTL_PARTITION_RESTART:
...
case FSL_HV_IOCTL_PARTITION_GET_STATUS;
...
There is no need to check the bits individually when you can simply
compare the command number.
But this will break backwards compatibility with older applications that used
the union as the size parameter. Although these applications won't compile with
the new header file, older already-compiled applications still work.
I will eventually update the applications to use the new header file, and at
that point I will modify the switch statement as you suggest. But until then,
I'd like to keep the code as-is.
+enum fsl_hv_ioctl_cmd {
+ FSL_HV_IOCTL_PARTITION_RESTART = _IOWR(0, 1, struct
fsl_hv_ioctl_restart),
+ FSL_HV_IOCTL_PARTITION_GET_STATUS = _IOWR(0, 2, struct
fsl_hv_ioctl_status),
+ FSL_HV_IOCTL_PARTITION_START = _IOWR(0, 3, struct
fsl_hv_ioctl_start),
+ FSL_HV_IOCTL_PARTITION_STOP = _IOWR(0, 4, struct
fsl_hv_ioctl_stop),
+ FSL_HV_IOCTL_MEMCPY = _IOWR(0, 5, struct fsl_hv_ioctl_memcpy),
+ FSL_HV_IOCTL_DOORBELL = _IOWR(0, 6, struct fsl_hv_ioctl_doorbell),
+ FSL_HV_IOCTL_GETPROP = _IOWR(0, 7, struct fsl_hv_ioctl_prop),
+ FSL_HV_IOCTL_SETPROP = _IOWR(0, 8, struct fsl_hv_ioctl_prop),
+};
Using a #define here is usually preferred because then you can use #ifdef in
a user
application to check if a given value has been assigned.
You're right -- I had enum on the brain.
More importantly, the code you have chose (0) conflicts with existing drivers
(frame buffer, scsi and wavefront among others). Please chose a free one and
add it to Documentation/ioctl/ioctl-number.txt in the same patch.
Ok, I was really hoping to avoid doing this. Like I said, binary compatibility
is important, and changing the type will break my existing apps. Are you
insisting that I pick a new number?
--
Timur Tabi
Linux kernel developer at Freescale
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 7/7] [v5] drivers/virt: introduce Freescale hypervisor management driver
On Thursday 09 June 2011 21:48:58 Randy Dunlap wrote: So is it okay to stick with 0, or do I need to pick a new number? I wasn't suggesting that you change the 0, just note that it has conflicts, like other ioctls do. We normally don't try to maintain binary compatibility with out of tree kernel patches. That only leads to inferior interfaces finding their way into the kernel. Arnd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 7/7] [v5] drivers/virt: introduce Freescale hypervisor management driver
On Thu, Jun 09, 2011 at 03:18:28PM -0500, Timur Tabi wrote: More importantly, the code you have chose (0) conflicts with existing drivers (frame buffer, scsi and wavefront among others). Please chose a free one and add it to Documentation/ioctl/ioctl-number.txt in the same patch. Ok, I was really hoping to avoid doing this. Like I said, binary compatibility is important, and changing the type will break my existing apps. Are you insisting that I pick a new number? Why is binary compatibility important? Isn't this a brand new driver for a brand new system? What userspace tools are out there in the wild for such a thing? confused, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 7/7] [v5] drivers/virt: introduce Freescale hypervisor management driver
On Thursday 09 June 2011 22:18:28 Timur Tabi wrote: More importantly, the code you have chose (0) conflicts with existing drivers (frame buffer, scsi and wavefront among others). Please chose a free one and add it to Documentation/ioctl/ioctl-number.txt in the same patch. Ok, I was really hoping to avoid doing this. Like I said, binary compatibility is important, and changing the type will break my existing apps. Are you insisting that I pick a new number? I definitely insist that you have a proper interface in the driver at the time that it gets merged, and that probably includes a collision-free ioctl code. You can probably make the driver support both the traditional and the new interface, but I would prefer if you kept that as a private patch on top a clean kernel driver. It's also a good idea to keep the header file clean and only define the new interface there, to ensure that all applications that are built in the future have to use the new interface. When you make the patch to add backwards compat support, just add it to the driver itself, not to the header. Arnd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 7/7] [v5] drivers/virt: introduce Freescale hypervisor management driver
Greg KH wrote: Why is binary compatibility important? Isn't this a brand new driver for a brand new system? What userspace tools are out there in the wild for such a thing? This driver (and the hypervisor it talks to, plus the apps, etc) has been in internal development for three years. There hasn't been a lot of effort internally to release this software upstream until recently. I personally have been complaining about it for quite some time, but I have no control over our internal release process. Even when the hardware has been announced and available for purchase, the BSP is sometimes only available under NDA. Eventually, everything is publicly released, and most of the code is pushed upstream, but it's a long and painful struggle at times. My concern has been dealing with the headaches of bug reports from customers, etc as they upgrade their kernel but not their apps, and then wonder why nothing works. But as Arnd pointed out, it really isn't as big of deal as I make it out to be. I can maintain compatibility internally. I blame my allergy medicine. -- Timur Tabi Linux kernel developer at Freescale ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
