Re: [PATCH 7/7] [v6] drivers/virt: introduce Freescale hypervisor management driver

[Arnd Bergmann]

On Thursday 09 June 2011 22:52:06 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 

Acked-by: Arnd Bergmann 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

[PATCH 7/7] [v6] drivers/virt: introduce Freescale hypervisor management driver

[Timur Tabi]

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 
---
 Documentation/ioctl/ioctl-number.txt |1 +
 drivers/Kconfig  |2 +
 drivers/Makefile |3 +
 drivers/virt/Kconfig |   32 ++
 drivers/virt/Makefile|5 +
 drivers/virt/fsl_hypervisor.c|  937 ++
 include/linux/Kbuild |1 +
 include/linux/fsl_hypervisor.h   |  241 +
 8 files changed, 1222 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/Documentation/ioctl/ioctl-number.txt 
b/Documentation/ioctl/ioctl-number.txt
index a0a5d82..9b8a8bd 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -301,6 +301,7 @@ Code  Seq#(hex) Include FileComments

 0xAE   all linux/kvm.h Kernel-based Virtual Machine

+0xAF   00-1F   linux/fsl_hypervisor.h  Freescale hypervisor
 0xB0   all RATIO devices   in development:

 0xB1   00-1F   PPPoX   
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..1d3b8eb
--- /dev/null
+++ b/drivers/virt/fsl_hypervisor.c
@@ -0,0 +1,937 @@
+/*
+ * Freescale Hypervisor Management Driver
+
+ * Copyright (C) 2008-2011 Freescale Semiconductor, Inc.
+ * Author: Timur Tabi 
+ *
+ * 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 fro

Re: [PATCH 7/7] [v5] drivers/virt: introduce Freescale hypervisor management driver

[Timur Tabi]

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

Re: [PATCH 7/7] [v5] drivers/virt: introduce Freescale hypervisor management driver

[Arnd Bergmann]

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]

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

[Arnd Bergmann]

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

[Timur Tabi]

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

[Arnd Bergmann]

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

[Randy Dunlap]

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

[Timur Tabi]

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

[Randy Dunlap]

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 
> ---
>  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

[PATCH 7/7] [v5] drivers/virt: introduce Freescale hypervisor management driver

[Timur Tabi]

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 
---
 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 
+ *
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+#include 
+
+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)
+{
+   struct fsl_hv_ioctl_restart param;
+
+   /* Get the parameters from the user */
+   if (copy_from_user(¶m, p, sizeof(struct fsl_hv_ioctl_restart)))
+   return -EFAULT;
+
+   param.ret = fh_partition_restart(param.partition);
+
+   if 

Re: [PATCH 1/3] Staging: hv: netvsc: Fix a bug in accounting transmit slots

[Greg KH]

On Thu, Jun 09, 2011 at 05:23:50PM +, KY Srinivasan wrote:
> > -Original Message-
> > From: Greg KH [mailto:g...@kroah.com]
> > Sent: Thursday, June 09, 2011 1:05 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 1/3] Staging: hv: netvsc: Fix a bug in accounting 
> > transmit slots
> > 
> > On Mon, Jun 06, 2011 at 03:57:35PM -0700, K. Y. Srinivasan wrote:
> > 
> > I only got 2 of these three patches, what happened to the third one?
> > 
> > Care to resend all 3 of these to ensure that I get them all?
> 
> There really were only 2 patches I wanted to send. If you want I could 
> reformat
> and send only the 2 I wanted to send.

Please do, as when you send something that says 1/3 and 2/3 and 3/3
never shows up, we have no idea what is going on...

thanks,

greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

RE: [PATCH 1/3] Staging: hv: netvsc: Fix a bug in accounting transmit slots

[KY Srinivasan]

> -Original Message-
> From: Greg KH [mailto:g...@kroah.com]
> Sent: Thursday, June 09, 2011 1:05 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 1/3] Staging: hv: netvsc: Fix a bug in accounting 
> transmit slots
> 
> On Mon, Jun 06, 2011 at 03:57:35PM -0700, K. Y. Srinivasan wrote:
> 
> I only got 2 of these three patches, what happened to the third one?
> 
> Care to resend all 3 of these to ensure that I get them all?

There really were only 2 patches I wanted to send. If you want I could reformat
and send only the 2 I wanted to send.

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/3] Staging: hv: netvsc: Fix a bug in accounting transmit slots

[Greg KH]

On Mon, Jun 06, 2011 at 03:57:35PM -0700, K. Y. Srinivasan wrote:

I only got 2 of these three patches, what happened to the third one?

Care to resend all 3 of these to ensure that I get them all?

thanks,

greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 7/7] [v4] drivers/virt: introduce Freescale hypervisor management driver

[Randy Dunlap]

On 06/09/11 09:36, Timur Tabi wrote:
> Randy Dunlap wrote:
>> But it sounds like virt/ needs virt/host/ and virt/guest/ to me.
> 
> I'm okay with that idea, except there's a consensus that drivers should be in
> drivers/.
> 

Like sound/ ?

but what makes it a "driver"?

-- 
~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] [v4] drivers/virt: introduce Freescale hypervisor management driver

[Randy Dunlap]

On 06/09/11 00:38, Arnd Bergmann wrote:
> On Thursday 09 June 2011 01:10:09 Randy Dunlap wrote:
>> On Wed, 8 Jun 2011 17:45:54 -0500 Timur Tabi wrote:
>>
>>> Add the drivers/virt directory, which houses drivers that support
>>> virtualization environments, and add the Freescale hypervisor management
>>> driver.
>>
>> It can't go in linux/virt or linux/virt/fsl instead?  why drivers/ ?
>>
>> or maybe linux/virt should be drivers/virt ?
> 
> See discussion for v2 of this patch. I suggested that drivers/firmware and 
> virt/
> as options, the counterarguments were that drivers/firmware is for passive
> firmware as opposed to firmware that acts as a hypervisor, and that virt/ is
> for the host side of hypervisors like kvm, not for guests.

OK, I read that thread.  Didn't see a real consensus there.

If you were not the drivers/misc/ maintainer, would you mind if this
driver lived in drivers/misc/?  I wouldn't.

But it sounds like virt/ needs virt/host/ and virt/guest/ to me.


> The driver in here most closely resembles the xen dom0 model, where a
> priviledged guest controls other guests, but unlike xen there is a single
> driver file, so there is no need to have drivers/fsl-hv directory just
> for this one file. We do have a number of other hypervisors that fit in the
> same category, so they can be added here later.


-- 
~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 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Mark Wu]

On 06/09/2011 05:14 AM, Tejun Heo wrote:
> Hello,
> 
> On Thu, Jun 09, 2011 at 08:51:05AM +0930, Rusty Russell wrote:
>> On Wed, 08 Jun 2011 09:08:29 -0400, Mark Wu  wrote:
>>> Hi Rusty,
>>> Yes, I can't figure out an instance of disk probing in parallel either, but 
>>> as
>>> per the following commit, I think we still need use lock for safety. What's 
>>> your opinion?
>>>
>>> commit 4034cc68157bfa0b6622efe368488d3d3e20f4e6
>>> Author: Tejun Heo 
>>> Date:   Sat Feb 21 11:04:45 2009 +0900
>>>
>>> [SCSI] sd: revive sd_index_lock
>>>
>>> Commit f27bac2761cab5a2e212dea602d22457a9aa6943 which converted sd to
>>> use ida instead of idr incorrectly removed sd_index_lock around id
>>> allocation and free.  idr/ida do have internal locks but they protect
>>> their free object lists not the allocation itself.  The caller is
>>> responsible for that.  This missing synchronization led to the same id
>>> being assigned to multiple devices leading to oops.
>>
>> I'm confused.  Tejun, Greg, anyone can probes happen in parallel?
>>
>> If so, I'll have to review all my drivers.
> 
> Unless async is explicitly used, probe happens sequentially.  IOW, if
> there's no async_schedule() call, things won't happen in parallel.
> That said, I think it wouldn't be such a bad idea to protect ida with
> spinlock regardless unless the probe code explicitly requires
> serialization.
> 
> Thanks.
> 
Since virtio blk driver doesn't use async probe, it needn't use spinlock to 
protect ida.
So remove the lock from patch.

>From fbb396df9dbf8023f1b268be01b43529a3993d57 Mon Sep 17 00:00:00 2001
From: Mark Wu 
Date: Thu, 9 Jun 2011 06:34:07 -0400
Subject: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

Current index allocation in virtio-blk is based on a monotonically
increasing variable "index". It could cause some confusion about disk
name in the case of hot-plugging disks. And it's impossible to find the
lowest available index by just maintaining a simple index. So it's
changed to use ida to allocate index via referring to the index
allocation in scsi disk.

Signed-off-by: Mark Wu 
---
 drivers/block/virtio_blk.c |   28 +++-
 1 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 079c088..bf81ab6 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -8,10 +8,13 @@
 #include 
 #include 
 #include 
+#include 
 
 #define PART_BITS 4
 
-static int major, index;
+static int major;
+static DEFINE_IDA(vd_index_ida);
+
 struct workqueue_struct *virtblk_wq;
 
 struct virtio_blk
@@ -23,6 +26,7 @@ struct virtio_blk
 
/* The disk structure for the kernel. */
struct gendisk *disk;
+   u32 index;
 
/* Request tracking. */
struct list_head reqs;
@@ -343,12 +347,23 @@ static int __devinit virtblk_probe(struct virtio_device 
*vdev)
struct request_queue *q;
int err;
u64 cap;
-   u32 v, blk_size, sg_elems, opt_io_size;
+   u32 v, blk_size, sg_elems, opt_io_size, index;
u16 min_io_size;
u8 physical_block_exp, alignment_offset;
 
-   if (index_to_minor(index) >= 1 << MINORBITS)
-   return -ENOSPC;
+   do {
+   if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
+   return -ENOMEM;
+   err = ida_get_new(&vd_index_ida, &index);
+   } while (err == -EAGAIN);
+
+   if (err)
+   return err;
+
+   if (index_to_minor(index) >= 1 << MINORBITS) {
+   err =  -ENOSPC;
+   goto out_free_index;
+   }
 
/* We need to know how many segments before we allocate. */
err = virtio_config_val(vdev, VIRTIO_BLK_F_SEG_MAX,
@@ -421,7 +436,7 @@ static int __devinit virtblk_probe(struct virtio_device 
*vdev)
vblk->disk->private_data = vblk;
vblk->disk->fops = &virtblk_fops;
vblk->disk->driverfs_dev = &vdev->dev;
-   index++;
+   vblk->index = index;
 
/* configure queue flush support */
if (virtio_has_feature(vdev, VIRTIO_BLK_F_FLUSH))
@@ -516,6 +531,8 @@ out_free_vq:
vdev->config->del_vqs(vdev);
 out_free_vblk:
kfree(vblk);
+out_free_index:
+   ida_remove(&vd_index_ida, index);
 out:
return err;
 }
@@ -538,6 +555,7 @@ static void __devexit virtblk_remove(struct virtio_device 
*vdev)
mempool_destroy(vblk->pool);
vdev->config->del_vqs(vdev);
kfree(vblk);
+   ida_remove(&vd_index_ida, vblk->index);
 }
 
 static const struct virtio_device_id id_table[] = {
-- 
1.7.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Tejun Heo]

Hello,

On Thu, Jun 09, 2011 at 08:51:05AM +0930, Rusty Russell wrote:
> On Wed, 08 Jun 2011 09:08:29 -0400, Mark Wu  wrote:
> > Hi Rusty,
> > Yes, I can't figure out an instance of disk probing in parallel either, but 
> > as
> > per the following commit, I think we still need use lock for safety. What's 
> > your opinion?
> > 
> > commit 4034cc68157bfa0b6622efe368488d3d3e20f4e6
> > Author: Tejun Heo 
> > Date:   Sat Feb 21 11:04:45 2009 +0900
> > 
> > [SCSI] sd: revive sd_index_lock
> > 
> > Commit f27bac2761cab5a2e212dea602d22457a9aa6943 which converted sd to
> > use ida instead of idr incorrectly removed sd_index_lock around id
> > allocation and free.  idr/ida do have internal locks but they protect
> > their free object lists not the allocation itself.  The caller is
> > responsible for that.  This missing synchronization led to the same id
> > being assigned to multiple devices leading to oops.
> 
> I'm confused.  Tejun, Greg, anyone can probes happen in parallel?
> 
> If so, I'll have to review all my drivers.

Unless async is explicitly used, probe happens sequentially.  IOW, if
there's no async_schedule() call, things won't happen in parallel.
That said, I think it wouldn't be such a bad idea to protect ida with
spinlock regardless unless the probe code explicitly requires
serialization.

Thanks.

-- 
tejun
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 7/7] [v4] drivers/virt: introduce Freescale hypervisor management driver

[Arnd Bergmann]

On Thursday 09 June 2011 01:10:09 Randy Dunlap wrote:
> On Wed, 8 Jun 2011 17:45:54 -0500 Timur Tabi wrote:
> 
> > Add the drivers/virt directory, which houses drivers that support
> > virtualization environments, and add the Freescale hypervisor management
> > driver.
> 
> It can't go in linux/virt or linux/virt/fsl instead?  why drivers/ ?
> 
> or maybe linux/virt should be drivers/virt ?

See discussion for v2 of this patch. I suggested that drivers/firmware and virt/
as options, the counterarguments were that drivers/firmware is for passive
firmware as opposed to firmware that acts as a hypervisor, and that virt/ is
for the host side of hypervisors like kvm, not for guests.

The driver in here most closely resembles the xen dom0 model, where a
priviledged guest controls other guests, but unlike xen there is a single
driver file, so there is no need to have drivers/fsl-hv directory just
for this one file. We do have a number of other hypervisors that fit in the
same category, so they can be added here later.

Arnd
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: virtio scsi host draft specification, v3

[Paolo Bonzini]

On 06/09/2011 01:28 AM, Rusty Russell wrote:
>> >  after some preliminary discussion on the QEMU mailing list, I present a
>> >  draft specification for a virtio-based SCSI host (controller, HBA, you
>> >  name it).
>
> OK, I'm impressed.  This is very well written and it doesn't make any of
> the obvious mistakes wrt. virtio.

Thanks very much, and thanks to those who corrected my early mistakes.

> I assume you have an implementation, as well?

Unfortunately not; "we're working on it", which means I should start in 
July when I come back from vacation.

Do you prefer to wait for one before I make a patch to the LyX source? 
In the meanwhile, can you reserve a subsystem ID for me?

Paolo
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization