[Qemu-devel] [PATCH] e1000: Don't set the Capabilities List bit

[Qemu-devel] [PATCH] e1000: Don't set the Capabilities List bit

[dann frazier]

[Originally sent to qemu-kvm list, but I was redirected here]

The Capabilities Pointer is NULL, so this bit shouldn't be set. The state of
this bit doesn't appear to change any behavior on Linux/Windows versions we've
tested, but it does cause Windows' PCI/PCI Express Compliance Test to balk.

I happen to have a physical 82540EM controller, and it also sets the
Capabilities Bit, but it actually has items on the capabilities list to go
with it :)

Signed-off-by: dann frazier <dann.frazier@canonical.com>
---
 hw/e1000.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/hw/e1000.c b/hw/e1000.c
index 6a3a941..ce8fc8b 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -1151,8 +1151,6 @@ static int pci_e1000_init(PCIDevice *pci_dev)
 
     pci_conf = d->dev.config;
 
-    /* TODO: we have no capabilities, so why is this bit set? */
-    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST);
     /* TODO: RST# value should be 0, PCI spec 6.2.4 */
     pci_conf[PCI_CACHE_LINE_SIZE] = 0x10;
 
-- 
1.7.6.3

Re: [Qemu-devel] [PATCH] e1000: Don't set the Capabilities List bit

[Anthony Liguori]

On 09/21/2011 03:06 PM, dann frazier wrote:
> [Originally sent to qemu-kvm list, but I was redirected here]
>
> The Capabilities Pointer is NULL, so this bit shouldn't be set. The state of
> this bit doesn't appear to change any behavior on Linux/Windows versions we've
> tested, but it does cause Windows' PCI/PCI Express Compliance Test to balk.
>
> I happen to have a physical 82540EM controller, and it also sets the
> Capabilities Bit, but it actually has items on the capabilities list to go
> with it :)
>
> Signed-off-by: dann frazier<dann.frazier@canonical.com>

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
>   hw/e1000.c |    2 --
>   1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 6a3a941..ce8fc8b 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -1151,8 +1151,6 @@ static int pci_e1000_init(PCIDevice *pci_dev)
>
>       pci_conf = d->dev.config;
>
> -    /* TODO: we have no capabilities, so why is this bit set? */
> -    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST);
>       /* TODO: RST# value should be 0, PCI spec 6.2.4 */
>       pci_conf[PCI_CACHE_LINE_SIZE] = 0x10;
>

Re: [Qemu-devel] [PATCH] e1000: Don't set the Capabilities List bit

[Philipp Hahn]

[-- Attachment #1: Type: text/plain, Size: 2578 bytes --]

Hello,

On Wednesday 21 September 2011 22:06:25 dann frazier wrote:
> The Capabilities Pointer is NULL, so this bit shouldn't be set. The state
> of this bit doesn't appear to change any behavior on Linux/Windows versions
> we've tested, but it does cause Windows' PCI/PCI Express Compliance Test to
> balk.
...
> +++ b/hw/e1000.c
> @@ -1151,8 +1151,6 @@ static int pci_e1000_init(PCIDevice *pci_dev)
...
> -    /* TODO: we have no capabilities, so why is this bit set? */
> -    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST);

I think that change introduced a upgrade problem:
We have several VMs initially setup with qemu-0.12 and 0.14. Over the time of 
years we've created lots of snapshots, which we'd like to keep.
Now it's time to upgrade qemu(-kvm) on our hosts to qemu-1.1.2, but trying to 
load the old save states failes with
  qemu: warning: error while loading state for instance 0x0 of 
device '0000:00:03.0/e1000'

I traced it down to get_pci_config_device() failung for the e1000, because of 
that changed bit:
(gdb) print /x config@256 
$23 = {0x86, 0x80, 0xe, 0x10, 0x7, 0x0, 0x18, 0x0, 0x3, 0x0, 0x0, 0x2, 0x0, 
0x0, 0x0, 0x0, 0x0, 0x0, 0x2, 0xf2, 0x41, 0xc0, 0x0 <repeats 22 times>, 0xf4, 
0x1a, 0x0, 0x11, 0x0,  0x0, 0x4, 0xf2, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 
0x0, 0xb, 0x1, 0x0 <repeats 194 times>}
(gdb) print /x *s->config@256 
$25 = {0x86, 0x80, 0xe, 0x10, 0x0, 0x0, 0x0, 0x0, 0x3, 0x0, 0x0, 0x2, 0x0, 
0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x0 <repeats 23 times>, 0xf4, 0x1a, 
0x0, 0x11, 0x0 <repeats 13 times>, 0x1, 0x0 <repeats 194 times>}

Since cmask[PCI_STATUS=6] = PCI_STATUS_CAP_LIST=0x10 marks that bit as 
unmodifiable, the functions returns an error and aborts loading the saved 
state.

Is this problem known?
Is such an upgrade of qemu supposed to work?
Has somebody an idea how to fix this issue?

Sincerely
Philipp Hahn

PS: It would be nice if the error message could indicate an error because of 
an incompatible PCI configuration. I had a very similar problem with the 
rtl8139 card, where the ROM size was changed due to the upgrade from 
etherboot to iPXE.
PPS: It's slightly confusing to get a 'warning' message starting with 'error 
loading ...'
-- 
Philipp Hahn           Open Source Software Engineer      hahn@univention.de
Univention GmbH        be open.                       fon: +49 421 22 232- 0
Mary-Somerville-Str.1  D-28359 Bremen                 fax: +49 421 22 232-99
                                                   http://www.univention.de/

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [Qemu-devel] [PATCH] e1000: Don't set the Capabilities List bit

[Philipp Hahn]

[-- Attachment #1.1: Type: text/plain, Size: 1074 bytes --]

Hello,

On Friday 19 October 2012 11:59:24 Philipp Hahn wrote:
> On Wednesday 21 September 2011 22:06:25 dann frazier wrote:
...
> > -    /* TODO: we have no capabilities, so why is this bit set? */
> > -    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST);
...
> Since cmask[PCI_STATUS=6] = PCI_STATUS_CAP_LIST=0x10 marks that bit as
> unmodifiable, the functions returns an error and aborts loading the saved
> state.
...
> Has somebody an idea how to fix this issue?

I'm no expert on PCI issues, but since e1000 drivers don't seem to much care 
about the Capability List bit, perhaps the attached patch to ignore the bit 
for e1000 would be in oder?
At least it fixes my problem.
Comments welcomed.

Sincerely
Philipp Hahn
-- 
Philipp Hahn           Open Source Software Engineer      hahn@univention.de
Univention GmbH        be open.                       fon: +49 421 22 232- 0
Mary-Somerville-Str.1  D-28359 Bremen                 fax: +49 421 22 232-99
                                                   http://www.univention.de/

[-- Attachment #1.2: 0003-Bug-24702-e1000-pci-config.patch --]
[-- Type: text/x-diff, Size: 945 bytes --]

e1000: Ignore the Capabilities List bit

dd8e93799f13ef82d83c185b8e71e049452f7d40 unconditionally removed the
PCI_STATUS_CAP_LIST bit from PCI_STATUS, because the e1000 does not have
capabilities. This breaks upgrades from before qemu-0.15, because there the bit
is still set and get_pci_config_device() refused to load incompatible save
states.

Remove the Capabilities List bit from the list of compatible bits, so it is not
validated for an exact match.

Signed-off-by: Philipp Hahn <hahn@univention.de>

--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -1224,6 +1224,9 @@ static int pci_e1000_init(PCIDevice *pci_dev)
 
     pci_conf = d->dev.config;
 
+    /* Ignore capability bit, which was set until qemu-0.15 */
+    d->dev.cmask[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
+    d->dev.wmask[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
     /* TODO: RST# value should be 0, PCI spec 6.2.4 */
     pci_conf[PCI_CACHE_LINE_SIZE] = 0x10;
 

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]