diff mbox

[1/1] ata_piix: defer disks to the Hyper-V paravirtualised drivers by default

Message ID 1333042130-9634-1-git-send-email-apw@canonical.com
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Andy Whitcroft March 29, 2012, 5:28 p.m. UTC 
When we are hosted on a Hyper-V hypervisor the guest disks are exposed
both via the Hyper-V paravirtualised drivers and via an emulated SATA disk
controller.  We want to use the paravirtualised drivers where possible as
they are much more performant.  The Hyper-V paravirtualised drivers only
expose the virtual hard disk devices, the CDROM/DVD devices must still
be enumerated on the virtualised SATA controller.  As we have no control
over kernel probe order for these two drivers especially when one driver
is builtin to the kernel and the other a module, we need to prevent the
ata_piix driver from claiming the disks devices by default when running
on a Hyper-V hypervisor.

When enumerating the drives look at the aquired device ID and if it
appears to be a disk device then report it as disconnected.  Limit this
behaviour to when we have detected a Hyper-V hypervisor.  Finally allow
this behaviour to be overriden via a new module parameter.

BugLink: http://bugs.launchpad.net/bugs/929545
BugLink: http://bugs.launchpad.net/bugs/942316
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 drivers/ata/ata_piix.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

    This was discovered when doing some boot testing on a Hyper-V
    host.  Performance was found to be bad as out builtin ata_piix
    driver was grabbing the disks.  I am Cc:ing a couple of the
    Microsoft people who worked on the HV driver set as well.

    Note that the device id data is converted from device to host
    order in the caller which makes examining the data problematic.
    As this code only make sense on X86 and we know we are in
    matching order we can access the data without first fixing it.
    Alternativly we could add a new callback from the ata core
    after the code has been fixed for validation.

    Comments?

    -apw

Comments

Victor Miasnikov March 30, 2012, 9:14 a.m. UTC | #1 
Hi!

----- Original Message ----- 
From: "Andy Whitcroft" apw
Sent: Thursday, March 29, 2012 8:28 PM
Subject: [PATCH 1/1] ata_piix: defer disks to the Hyper-V paravirtualised drivers by default

> guest disks are exposed both via the Hyper-V paravirtualised drivers and via an emulated SATA disk
> controller.   . . .  we need to prevent the
> ata_piix driver from claiming the disks devices by default when running
> on a Hyper-V hypervisor.

 Yes

Thanks for patch!

>    Comments?

 This patch is  "UBUNTU Way" Solution described in

See
http://vvm.blog.tut.by/2012/03/29/linux-on-hyper-v-use-hv_storvsc-instead-of-ata_piix-to-handle-the-ide-disks-devices-but-not-for-the-dvd-rom-cd-rom-device-handling/
Or

http://marc.info/?l=linux-kernel&m=133302969709312&w=2
==
List: linux-kernel
Subject: Linux on Hyper-V -- use hv_storvsc instead of ata_piix to handle the IDE disks devices ( but not for the 
DVD-ROM / CD-ROM device handling) Fw: [PATCH RFC] ata_piix: ignore disks in a hyper-v guest
From: "Victor Miasnikov"
Date: 2012-03-29 14:00:05

. . .

"UBUNTU Way" Solution -- tested with
Ubuntu 12.04 LTS (Precise Pangolin) Daily Build 2012-03-13
precise-desktop-amd64.iso
, work Ok
. . .
==



i.e. "[PATCH 1/1] ata_piix: defer disks to the Hyper-V paravirtualised drivers by default"  is fully worked solution to 
the described problem


Best regards, Victor Miasnikov
Blog:  http://vvm.blog.tut.by/ 

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
KY Srinivasan April 10, 2012, 4:08 p.m. UTC | #2 
> -----Original Message-----
> From: Andy Whitcroft [mailto:apw@canonical.com]
> Sent: Thursday, March 29, 2012 1:29 PM
> To: Jeff Garzik; linux-ide@vger.kernel.org
> Cc: Andy Whitcroft; linux-kernel@vger.kernel.org; KY Srinivasan; Mike Sterling
> Subject: [PATCH 1/1] ata_piix: defer disks to the Hyper-V paravirtualised drivers
> by default
> 
> When we are hosted on a Hyper-V hypervisor the guest disks are exposed
> both via the Hyper-V paravirtualised drivers and via an emulated SATA disk
> controller.  We want to use the paravirtualised drivers where possible as
> they are much more performant.  The Hyper-V paravirtualised drivers only
> expose the virtual hard disk devices, the CDROM/DVD devices must still
> be enumerated on the virtualised SATA controller.  As we have no control
> over kernel probe order for these two drivers especially when one driver
> is builtin to the kernel and the other a module, we need to prevent the
> ata_piix driver from claiming the disks devices by default when running
> on a Hyper-V hypervisor.
> 
> When enumerating the drives look at the aquired device ID and if it
> appears to be a disk device then report it as disconnected.  Limit this
> behaviour to when we have detected a Hyper-V hypervisor.  Finally allow
> this behaviour to be overriden via a new module parameter.

Jeff,

Your feedback here would be greatly appreciated. If there are issues with this
please, do let us know.

Regards,

K. Y 
> 
> BugLink: http://bugs.launchpad.net/bugs/929545
> BugLink: http://bugs.launchpad.net/bugs/942316
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  drivers/ata/ata_piix.c |   29 +++++++++++++++++++++++++++++
>  1 files changed, 29 insertions(+), 0 deletions(-)
> 
>     This was discovered when doing some boot testing on a Hyper-V
>     host.  Performance was found to be bad as out builtin ata_piix
>     driver was grabbing the disks.  I am Cc:ing a couple of the
>     Microsoft people who worked on the HV driver set as well.
> 
>     Note that the device id data is converted from device to host
>     order in the caller which makes examining the data problematic.
>     As this code only make sense on X86 and we know we are in
>     matching order we can access the data without first fixing it.
>     Alternativly we could add a new callback from the ata core
>     after the code has been fixed for validation.
> 
>     Comments?
> 
>     -apw
> 
> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
> index 68013f9..64895f8 100644
> --- a/drivers/ata/ata_piix.c
> +++ b/drivers/ata/ata_piix.c
> @@ -94,6 +94,9 @@
>  #include <scsi/scsi_host.h>
>  #include <linux/libata.h>
>  #include <linux/dmi.h>
> +#ifdef CONFIG_X86
> +#include <asm/hypervisor.h>
> +#endif
> 
>  #define DRV_NAME	"ata_piix"
>  #define DRV_VERSION	"2.13"
> @@ -188,6 +191,29 @@ static int piix_pci_device_resume(struct pci_dev *pdev);
> 
>  static unsigned int in_module_init = 1;
> 
> +static int prefer_ms_hyperv = 1;
> +
> +unsigned int ata_piix_read_id(struct ata_device *dev,
> +					struct ata_taskfile *tf, u16 *id)
> +{
> +	int ret = ata_do_dev_read_id(dev, tf, id);
> +
> +#ifdef CONFIG_X86
> +	/* XXX: note that the device id is in little-endian order, the caller
> +	 * will shift it to host order, but we are working with little-endian.
> +	 * As this is _only_ used on x86 we can actually directly access it
> +	 * as host is also little-endian.
> +	 */
> +	if (!ret && prefer_ms_hyperv && x86_hyper == &x86_hyper_ms_hyperv
> &&
> +							ata_id_is_ata(id)) {
> +		ata_dev_printk(dev, KERN_WARNING, "ATA disk ignored
> deferring to Hyper-V paravirt driver\n");
> +
> +		return AC_ERR_DEV|AC_ERR_NODEV_HINT;
> +	}
> +#endif
> +	return ret;
> +}
> +
>  static const struct pci_device_id piix_pci_tbl[] = {
>  	/* Intel PIIX3 for the 430HX etc */
>  	{ 0x8086, 0x7010, PCI_ANY_ID, PCI_ANY_ID, 0, 0, piix_pata_mwdma },
> @@ -359,6 +385,7 @@ static struct ata_port_operations piix_pata_ops = {
>  	.set_piomode		= piix_set_piomode,
>  	.set_dmamode		= piix_set_dmamode,
>  	.prereset		= piix_pata_prereset,
> +	.read_id		= ata_piix_read_id,
>  };
> 
>  static struct ata_port_operations piix_vmw_ops = {
> @@ -1703,3 +1730,5 @@ static void __exit piix_exit(void)
> 
>  module_init(piix_init);
>  module_exit(piix_exit);
> +
> +module_param(prefer_ms_hyperv, int, 0);
> --
> 1.7.9.1
> 
> 
> 



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Whitcroft April 12, 2012, 3:55 p.m. UTC | #3 
On Thu, Mar 29, 2012 at 6:28 PM, Andy Whitcroft <apw@canonical.com> wrote:
> When we are hosted on a Hyper-V hypervisor the guest disks are exposed
> both via the Hyper-V paravirtualised drivers and via an emulated SATA disk
> controller.  We want to use the paravirtualised drivers where possible as
> they are much more performant.  The Hyper-V paravirtualised drivers only
> expose the virtual hard disk devices, the CDROM/DVD devices must still
> be enumerated on the virtualised SATA controller.  As we have no control
> over kernel probe order for these two drivers especially when one driver
> is builtin to the kernel and the other a module, we need to prevent the
> ata_piix driver from claiming the disks devices by default when running
> on a Hyper-V hypervisor.
>
> When enumerating the drives look at the aquired device ID and if it
> appears to be a disk device then report it as disconnected.  Limit this
> behaviour to when we have detected a Hyper-V hypervisor.  Finally allow
> this behaviour to be overriden via a new module parameter.
>
> BugLink: http://bugs.launchpad.net/bugs/929545
> BugLink: http://bugs.launchpad.net/bugs/942316
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  drivers/ata/ata_piix.c |   29 +++++++++++++++++++++++++++++
>  1 files changed, 29 insertions(+), 0 deletions(-)
>
>    This was discovered when doing some boot testing on a Hyper-V
>    host.  Performance was found to be bad as out builtin ata_piix
>    driver was grabbing the disks.  I am Cc:ing a couple of the
>    Microsoft people who worked on the HV driver set as well.
>
>    Note that the device id data is converted from device to host
>    order in the caller which makes examining the data problematic.
>    As this code only make sense on X86 and we know we are in
>    matching order we can access the data without first fixing it.
>    Alternativly we could add a new callback from the ata core
>    after the code has been fixed for validation.
>
>    Comments?
>
>    -apw
>
> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
> index 68013f9..64895f8 100644
> --- a/drivers/ata/ata_piix.c
> +++ b/drivers/ata/ata_piix.c
> @@ -94,6 +94,9 @@
>  #include <scsi/scsi_host.h>
>  #include <linux/libata.h>
>  #include <linux/dmi.h>
> +#ifdef CONFIG_X86
> +#include <asm/hypervisor.h>
> +#endif
>
>  #define DRV_NAME       "ata_piix"
>  #define DRV_VERSION    "2.13"
> @@ -188,6 +191,29 @@ static int piix_pci_device_resume(struct pci_dev *pdev);
>
>  static unsigned int in_module_init = 1;
>
> +static int prefer_ms_hyperv = 1;
> +
> +unsigned int ata_piix_read_id(struct ata_device *dev,
> +                                       struct ata_taskfile *tf, u16 *id)
> +{
> +       int ret = ata_do_dev_read_id(dev, tf, id);
> +
> +#ifdef CONFIG_X86
> +       /* XXX: note that the device id is in little-endian order, the caller
> +        * will shift it to host order, but we are working with little-endian.
> +        * As this is _only_ used on x86 we can actually directly access it
> +        * as host is also little-endian.
> +        */
> +       if (!ret && prefer_ms_hyperv && x86_hyper == &x86_hyper_ms_hyperv &&
> +                                                       ata_id_is_ata(id)) {
> +               ata_dev_printk(dev, KERN_WARNING, "ATA disk ignored deferring to Hyper-V paravirt driver\n");
> +
> +               return AC_ERR_DEV|AC_ERR_NODEV_HINT;
> +       }
> +#endif
> +       return ret;
> +}
> +
>  static const struct pci_device_id piix_pci_tbl[] = {
>        /* Intel PIIX3 for the 430HX etc */
>        { 0x8086, 0x7010, PCI_ANY_ID, PCI_ANY_ID, 0, 0, piix_pata_mwdma },
> @@ -359,6 +385,7 @@ static struct ata_port_operations piix_pata_ops = {
>        .set_piomode            = piix_set_piomode,
>        .set_dmamode            = piix_set_dmamode,
>        .prereset               = piix_pata_prereset,
> +       .read_id                = ata_piix_read_id,
>  };
>
>  static struct ata_port_operations piix_vmw_ops = {
> @@ -1703,3 +1730,5 @@ static void __exit piix_exit(void)
>
>  module_init(piix_init);
>  module_exit(piix_exit);
> +
> +module_param(prefer_ms_hyperv, int, 0);

Jeff, any thoughts on this patch?

-apw
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik April 12, 2012, 8:03 p.m. UTC | #4 
On 03/29/2012 01:28 PM, Andy Whitcroft wrote:
> When we are hosted on a Hyper-V hypervisor the guest disks are exposed
> both via the Hyper-V paravirtualised drivers and via an emulated SATA disk
> controller.  We want to use the paravirtualised drivers where possible as
> they are much more performant.  The Hyper-V paravirtualised drivers only
> expose the virtual hard disk devices, the CDROM/DVD devices must still
> be enumerated on the virtualised SATA controller.  As we have no control
> over kernel probe order for these two drivers especially when one driver
> is builtin to the kernel and the other a module, we need to prevent the
> ata_piix driver from claiming the disks devices by default when running
> on a Hyper-V hypervisor.
>
> When enumerating the drives look at the aquired device ID and if it
> appears to be a disk device then report it as disconnected.  Limit this
> behaviour to when we have detected a Hyper-V hypervisor.  Finally allow
> this behaviour to be overriden via a new module parameter.
>
> BugLink: http://bugs.launchpad.net/bugs/929545
> BugLink: http://bugs.launchpad.net/bugs/942316
> Signed-off-by: Andy Whitcroft<apw@canonical.com>
> ---
>   drivers/ata/ata_piix.c |   29 +++++++++++++++++++++++++++++
>   1 files changed, 29 insertions(+), 0 deletions(-)
>
>      This was discovered when doing some boot testing on a Hyper-V
>      host.  Performance was found to be bad as out builtin ata_piix
>      driver was grabbing the disks.  I am Cc:ing a couple of the
>      Microsoft people who worked on the HV driver set as well.
>
>      Note that the device id data is converted from device to host
>      order in the caller which makes examining the data problematic.
>      As this code only make sense on X86 and we know we are in
>      matching order we can access the data without first fixing it.
>      Alternativly we could add a new callback from the ata core
>      after the code has been fixed for validation.
>
>      Comments?
>
>      -apw
>
> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
> index 68013f9..64895f8 100644
> --- a/drivers/ata/ata_piix.c
> +++ b/drivers/ata/ata_piix.c
> @@ -94,6 +94,9 @@
>   #include<scsi/scsi_host.h>
>   #include<linux/libata.h>
>   #include<linux/dmi.h>
> +#ifdef CONFIG_X86
> +#include<asm/hypervisor.h>
> +#endif
>
>   #define DRV_NAME	"ata_piix"
>   #define DRV_VERSION	"2.13"
> @@ -188,6 +191,29 @@ static int piix_pci_device_resume(struct pci_dev *pdev);
>
>   static unsigned int in_module_init = 1;
>
> +static int prefer_ms_hyperv = 1;
> +
> +unsigned int ata_piix_read_id(struct ata_device *dev,
> +					struct ata_taskfile *tf, u16 *id)
> +{
> +	int ret = ata_do_dev_read_id(dev, tf, id);
> +
> +#ifdef CONFIG_X86
> +	/* XXX: note that the device id is in little-endian order, the caller
> +	 * will shift it to host order, but we are working with little-endian.
> +	 * As this is _only_ used on x86 we can actually directly access it
> +	 * as host is also little-endian.
> +	 */
> +	if (!ret&&  prefer_ms_hyperv&&  x86_hyper ==&x86_hyper_ms_hyperv&&
> +							ata_id_is_ata(id)) {
> +		ata_dev_printk(dev, KERN_WARNING, "ATA disk ignored deferring to Hyper-V paravirt driver\n");
> +
> +		return AC_ERR_DEV|AC_ERR_NODEV_HINT;
> +	}
> +#endif
> +	return ret;
> +}
> +
>   static const struct pci_device_id piix_pci_tbl[] = {
>   	/* Intel PIIX3 for the 430HX etc */
>   	{ 0x8086, 0x7010, PCI_ANY_ID, PCI_ANY_ID, 0, 0, piix_pata_mwdma },
> @@ -359,6 +385,7 @@ static struct ata_port_operations piix_pata_ops = {
>   	.set_piomode		= piix_set_piomode,
>   	.set_dmamode		= piix_set_dmamode,
>   	.prereset		= piix_pata_prereset,
> +	.read_id		= ata_piix_read_id,

Platform quirks should be handled in piix_init_one, not with this 
haphazard read_id callback.  If prefer_ms_hyperv && hypervisor-detected, 
then set ATA_HOST_IGNORE_ATA flag or similar.

	Jeff



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Whitcroft April 13, 2012, 7:37 a.m. UTC | #5 
On Thu, Apr 12, 2012 at 04:03:42PM -0400, Jeff Garzik wrote:

> Platform quirks should be handled in piix_init_one, not with this
> haphazard read_id callback.  If prefer_ms_hyperv &&
> hypervisor-detected, then set ATA_HOST_IGNORE_ATA flag or similar.

That does sound better.  I'll whip something up.

-apw
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Whitcroft April 14, 2012, 3:53 p.m. UTC | #6 
Add support for ignoring disks devices on specific host controllers to
libata.  Use this support to trigger us to ignore Hyper-V disk devices
on the emulated SATA device.

-apw

Andy Whitcroft (2):
  libata: add a host flag to ignore detected ATA devices
  ata_piix: defer disks to the Hyper-V drivers by default

 drivers/ata/ata_piix.c    |   20 ++++++++++++++++++++
 drivers/ata/libata-core.c |    7 +++++++
 include/linux/libata.h    |    1 +
 3 files changed, 28 insertions(+)
diff mbox

Patch

diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index 68013f9..64895f8 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -94,6 +94,9 @@ 
 #include <scsi/scsi_host.h>
 #include <linux/libata.h>
 #include <linux/dmi.h>
+#ifdef CONFIG_X86
+#include <asm/hypervisor.h>
+#endif
 
 #define DRV_NAME	"ata_piix"
 #define DRV_VERSION	"2.13"
@@ -188,6 +191,29 @@  static int piix_pci_device_resume(struct pci_dev *pdev);
 
 static unsigned int in_module_init = 1;
 
+static int prefer_ms_hyperv = 1;
+
+unsigned int ata_piix_read_id(struct ata_device *dev,
+					struct ata_taskfile *tf, u16 *id)
+{
+	int ret = ata_do_dev_read_id(dev, tf, id);
+
+#ifdef CONFIG_X86
+	/* XXX: note that the device id is in little-endian order, the caller
+	 * will shift it to host order, but we are working with little-endian.
+	 * As this is _only_ used on x86 we can actually directly access it
+	 * as host is also little-endian.
+	 */
+	if (!ret && prefer_ms_hyperv && x86_hyper == &x86_hyper_ms_hyperv &&
+							ata_id_is_ata(id)) {
+		ata_dev_printk(dev, KERN_WARNING, "ATA disk ignored deferring to Hyper-V paravirt driver\n");
+
+		return AC_ERR_DEV|AC_ERR_NODEV_HINT;
+	}
+#endif
+	return ret;
+}
+
 static const struct pci_device_id piix_pci_tbl[] = {
 	/* Intel PIIX3 for the 430HX etc */
 	{ 0x8086, 0x7010, PCI_ANY_ID, PCI_ANY_ID, 0, 0, piix_pata_mwdma },
@@ -359,6 +385,7 @@  static struct ata_port_operations piix_pata_ops = {
 	.set_piomode		= piix_set_piomode,
 	.set_dmamode		= piix_set_dmamode,
 	.prereset		= piix_pata_prereset,
+	.read_id		= ata_piix_read_id,
 };
 
 static struct ata_port_operations piix_vmw_ops = {
@@ -1703,3 +1730,5 @@  static void __exit piix_exit(void)
 
 module_init(piix_init);
 module_exit(piix_exit);
+
+module_param(prefer_ms_hyperv, int, 0);