e1000: Don't set the Capabilities List bit

[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(-)

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

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