Patchwork e1000: fix init values for command register

login
register
mail settings
Submitter Michael S. Tsirkin
Date Dec. 9, 2009, 8:35 p.m.
Message ID <20091209203537.GA7836@redhat.com>
Download mbox | patch
Permalink /patch/40761/
State New
Headers show

Comments

Michael S. Tsirkin - Dec. 9, 2009, 8:35 p.m.
Command register for e1000 was initialized to
values out of spec: all of bus master,
io, memory and interrupt disable bits were set.

This breaks the device now that we actually respect
the interrupt disable bit, unless the guest
happens to clear it. Fix, and make the device
more spec compliant, by not touching
the default.

There are implications for migration
from old qemu as well, will be addressed
separately.

Reported-by: Luiz Capitulino <lcapitulino@redhat.com>
Tested-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/e1000.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
Luiz Capitulino - Dec. 10, 2009, 1:03 p.m.
On Wed, 9 Dec 2009 22:35:37 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Command register for e1000 was initialized to
> values out of spec: all of bus master,
> io, memory and interrupt disable bits were set.
> 
> This breaks the device now that we actually respect
> the interrupt disable bit, unless the guest
> happens to clear it. Fix, and make the device
> more spec compliant, by not touching
> the default.
> 
> There are implications for migration
> from old qemu as well, will be addressed
> separately.

 FWIW, I've tested the others network cards (only for the slirp case)
and they worked fine.
Michael S. Tsirkin - Dec. 10, 2009, 6:14 p.m.
On Wed, Dec 09, 2009 at 10:35:37PM +0200, Michael S. Tsirkin wrote:
> Command register for e1000 was initialized to
> values out of spec: all of bus master,
> io, memory and interrupt disable bits were set.
> 
> This breaks the device now that we actually respect
> the interrupt disable bit, unless the guest
> happens to clear it. Fix, and make the device
> more spec compliant, by not touching
> the default.
> 
> There are implications for migration
> from old qemu as well, will be addressed
> separately.
> 
> Reported-by: Luiz Capitulino <lcapitulino@redhat.com>
> Tested-by: Luiz Capitulino <lcapitulino@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

This patch is also appropriate for 0.12, it will make migration 0.12 to
master less painful.
What's the process to get it there?
Should I create pci-for-0.12 branch on my tree?


> ---
>  hw/e1000.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 8566fe3..ad7a267 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -1089,7 +1089,6 @@ static int pci_e1000_init(PCIDevice *pci_dev)
>  
>      pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
>      pci_config_set_device_id(pci_conf, E1000_DEVID);
> -    *(uint16_t *)(pci_conf+0x04) = cpu_to_le16(0x0407);
>      *(uint16_t *)(pci_conf+0x06) = cpu_to_le16(0x0010);
>      pci_conf[0x08] = 0x03;
>      pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET);
> -- 
> 1.6.6.rc1.43.gf55cc

Patch

diff --git a/hw/e1000.c b/hw/e1000.c
index 8566fe3..ad7a267 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -1089,7 +1089,6 @@  static int pci_e1000_init(PCIDevice *pci_dev)
 
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
     pci_config_set_device_id(pci_conf, E1000_DEVID);
-    *(uint16_t *)(pci_conf+0x04) = cpu_to_le16(0x0407);
     *(uint16_t *)(pci_conf+0x06) = cpu_to_le16(0x0010);
     pci_conf[0x08] = 0x03;
     pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET);