[Qemu-devel] [PATCH v2 0/2] Fix eepro100 simple transmission, add i82559 PCI ID

[Qemu-devel] [PATCH v2 0/2] Fix eepro100 simple transmission, add i82559 PCI ID

[Mike Nawrocki]

This patch set does two things. First, it fixes the simple transmission mode
for the eepro100 chipset. Second, it adds the PCI device ID 0x1030 for the
i82559a chipset. This ID is set as the "alternate" PCI device ID for that
chipset, and may be selected via the new property "use-alt-device-id".

Mike Nawrocki (2):
  Fix eepro100 simple transmission mode
  Add new PCI ID for i82559a

 hw/net/eepro100.c    | 30 +++++++++++++++---------------
 include/hw/pci/pci.h |  1 +
 qemu-options.hx      |  2 +-
 3 files changed, 17 insertions(+), 16 deletions(-)

-- 
2.14.2

[Qemu-devel] [PATCH v2 1/2] Fix eepro100 simple transmission mode

[Mike Nawrocki]

The simple transmission mode was treating the area immediately after the
transmit command block (TCB) as if it were a transmit buffer descriptor,
when in reality it is simply the packet data. This change simply copies
the data following the TCB into the packet buffer.

Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
---
 hw/net/eepro100.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 80b8f47c4b..91dd058010 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -774,23 +774,11 @@ static void tx_command(EEPRO100State *s)
     }
     assert(tcb_bytes <= sizeof(buf));
     while (size < tcb_bytes) {
-        uint32_t tx_buffer_address = ldl_le_pci_dma(&s->dev, tbd_address);
-        uint16_t tx_buffer_size = lduw_le_pci_dma(&s->dev, tbd_address + 4);
-#if 0
-        uint16_t tx_buffer_el = lduw_le_pci_dma(&s->dev, tbd_address + 6);
-#endif
-        if (tx_buffer_size == 0) {
-            /* Prevent an endless loop. */
-            logout("loop in %s:%u\n", __FILE__, __LINE__);
-            break;
-        }
-        tbd_address += 8;
         TRACE(RXTX, logout
             ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n",
-             tx_buffer_address, tx_buffer_size));
-        tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
-        pci_dma_read(&s->dev, tx_buffer_address, &buf[size], tx_buffer_size);
-        size += tx_buffer_size;
+             tbd_address, tcb_bytes));
+        pci_dma_read(&s->dev, tbd_address, &buf[size], tcb_bytes);
+        size += tcb_bytes;
     }
     if (tbd_array == 0xffffffff) {
         /* Simplified mode. Was already handled by code above. */
-- 
2.14.2

Re: [Qemu-devel] [PATCH v2 1/2] Fix eepro100 simple transmission mode

[Thomas Huth]

On 06.11.2017 21:35, Mike Nawrocki wrote:
> The simple transmission mode was treating the area immediately after the
> transmit command block (TCB) as if it were a transmit buffer descriptor,
> when in reality it is simply the packet data. This change simply copies
> the data following the TCB into the packet buffer.
> 
> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
> ---
>  hw/net/eepro100.c | 18 +++---------------
>  1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 80b8f47c4b..91dd058010 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -774,23 +774,11 @@ static void tx_command(EEPRO100State *s)
>      }
>      assert(tcb_bytes <= sizeof(buf));
>      while (size < tcb_bytes) {
> -        uint32_t tx_buffer_address = ldl_le_pci_dma(&s->dev, tbd_address);
> -        uint16_t tx_buffer_size = lduw_le_pci_dma(&s->dev, tbd_address + 4);
> -#if 0
> -        uint16_t tx_buffer_el = lduw_le_pci_dma(&s->dev, tbd_address + 6);
> -#endif
> -        if (tx_buffer_size == 0) {
> -            /* Prevent an endless loop. */
> -            logout("loop in %s:%u\n", __FILE__, __LINE__);
> -            break;
> -        }
> -        tbd_address += 8;
>          TRACE(RXTX, logout
>              ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n",
> -             tx_buffer_address, tx_buffer_size));
> -        tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
> -        pci_dma_read(&s->dev, tx_buffer_address, &buf[size], tx_buffer_size);
> -        size += tx_buffer_size;
> +             tbd_address, tcb_bytes));
> +        pci_dma_read(&s->dev, tbd_address, &buf[size], tcb_bytes);
> +        size += tcb_bytes;
>      }
>      if (tbd_array == 0xffffffff) {
>          /* Simplified mode. Was already handled by code above. */
> 

 Hi,

I've got no clue why, but apparently this patch introduced a regression
in rc1: The tests/pxe-test is now failing on a big endian (s390x) host
when testing the i82550 device.
It's working fine with commit 8fa5ad6dfb66b6c695aa6a6b54b7031c (and
2.11-rc0), and it's broken starting with commit
1865e288a823c764cd4344d704e33344 ... do you have any clue what might be
wrong here now?

 Thomas

Re: [Qemu-devel] [PATCH v2 1/2] Fix eepro100 simple transmission mode

[Thomas Huth]

On 16.11.2017 17:59, Thomas Huth wrote:
> On 06.11.2017 21:35, Mike Nawrocki wrote:
>> The simple transmission mode was treating the area immediately after the
>> transmit command block (TCB) as if it were a transmit buffer descriptor,
>> when in reality it is simply the packet data. This change simply copies
>> the data following the TCB into the packet buffer.
>>
>> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
>> ---
>>  hw/net/eepro100.c | 18 +++---------------
>>  1 file changed, 3 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
>> index 80b8f47c4b..91dd058010 100644
>> --- a/hw/net/eepro100.c
>> +++ b/hw/net/eepro100.c
>> @@ -774,23 +774,11 @@ static void tx_command(EEPRO100State *s)
>>      }
>>      assert(tcb_bytes <= sizeof(buf));
>>      while (size < tcb_bytes) {
>> -        uint32_t tx_buffer_address = ldl_le_pci_dma(&s->dev, tbd_address);
>> -        uint16_t tx_buffer_size = lduw_le_pci_dma(&s->dev, tbd_address + 4);
>> -#if 0
>> -        uint16_t tx_buffer_el = lduw_le_pci_dma(&s->dev, tbd_address + 6);
>> -#endif
>> -        if (tx_buffer_size == 0) {
>> -            /* Prevent an endless loop. */
>> -            logout("loop in %s:%u\n", __FILE__, __LINE__);
>> -            break;
>> -        }
>> -        tbd_address += 8;
>>          TRACE(RXTX, logout
>>              ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n",
>> -             tx_buffer_address, tx_buffer_size));
>> -        tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
>> -        pci_dma_read(&s->dev, tx_buffer_address, &buf[size], tx_buffer_size);
>> -        size += tx_buffer_size;
>> +             tbd_address, tcb_bytes));
>> +        pci_dma_read(&s->dev, tbd_address, &buf[size], tcb_bytes);
>> +        size += tcb_bytes;
>>      }
>>      if (tbd_array == 0xffffffff) {
>>          /* Simplified mode. Was already handled by code above. */
>>
> 
>  Hi,
> 
> I've got no clue why, but apparently this patch introduced a regression
> in rc1: The tests/pxe-test is now failing on a big endian (s390x) host
> when testing the i82550 device.
> It's working fine with commit 8fa5ad6dfb66b6c695aa6a6b54b7031c (and
> 2.11-rc0), and it's broken starting with commit
> 1865e288a823c764cd4344d704e33344 ... do you have any clue what might be
> wrong here now?

This patch seems to fix the issue for me:

diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index a63ed2c..03e00f7 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -756,8 +756,8 @@ static void read_cb(EEPRO100State *s)
 
 static void tx_command(EEPRO100State *s)
 {
-    uint32_t tbd_array = le32_to_cpu(s->tx.tbd_array_addr);
-    uint16_t tcb_bytes = (le16_to_cpu(s->tx.tcb_bytes) & 0x3fff);
+    uint32_t tbd_array = s->tx.tbd_array_addr;
+    uint16_t tcb_bytes = s->tx.tcb_bytes & 0x3fff;
     /* Sends larger than MAX_ETH_FRAME_SIZE are allowed, up to 2600 bytes. */
     uint8_t buf[2600];
     uint16_t size = 0;

Does that make sense? I think the fields are already swapped in
read_cb(), so no need to swap them in tx_command() again?

 Thomas

Re: [Qemu-devel] [PATCH v2 1/2] Fix eepro100 simple transmission mode

[Michael Nawrocki]

On 11/16/2017 12:43 PM, Thomas Huth wrote:
> On 16.11.2017 17:59, Thomas Huth wrote:
>> On 06.11.2017 21:35, Mike Nawrocki wrote:
>>> The simple transmission mode was treating the area immediately after the
>>> transmit command block (TCB) as if it were a transmit buffer descriptor,
>>> when in reality it is simply the packet data. This change simply copies
>>> the data following the TCB into the packet buffer.
>>>
>>> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
>>> ---
>>>   hw/net/eepro100.c | 18 +++---------------
>>>   1 file changed, 3 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
>>> index 80b8f47c4b..91dd058010 100644
>>> --- a/hw/net/eepro100.c
>>> +++ b/hw/net/eepro100.c
>>> @@ -774,23 +774,11 @@ static void tx_command(EEPRO100State *s)
>>>       }
>>>       assert(tcb_bytes <= sizeof(buf));
>>>       while (size < tcb_bytes) {
>>> -        uint32_t tx_buffer_address = ldl_le_pci_dma(&s->dev, tbd_address);
>>> -        uint16_t tx_buffer_size = lduw_le_pci_dma(&s->dev, tbd_address + 4);
>>> -#if 0
>>> -        uint16_t tx_buffer_el = lduw_le_pci_dma(&s->dev, tbd_address + 6);
>>> -#endif
>>> -        if (tx_buffer_size == 0) {
>>> -            /* Prevent an endless loop. */
>>> -            logout("loop in %s:%u\n", __FILE__, __LINE__);
>>> -            break;
>>> -        }
>>> -        tbd_address += 8;
>>>           TRACE(RXTX, logout
>>>               ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n",
>>> -             tx_buffer_address, tx_buffer_size));
>>> -        tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
>>> -        pci_dma_read(&s->dev, tx_buffer_address, &buf[size], tx_buffer_size);
>>> -        size += tx_buffer_size;
>>> +             tbd_address, tcb_bytes));
>>> +        pci_dma_read(&s->dev, tbd_address, &buf[size], tcb_bytes);
>>> +        size += tcb_bytes;
>>>       }
>>>       if (tbd_array == 0xffffffff) {
>>>           /* Simplified mode. Was already handled by code above. */
>>>
>>
>>   Hi,
>>
>> I've got no clue why, but apparently this patch introduced a regression
>> in rc1: The tests/pxe-test is now failing on a big endian (s390x) host
>> when testing the i82550 device.
>> It's working fine with commit 8fa5ad6dfb66b6c695aa6a6b54b7031c (and
>> 2.11-rc0), and it's broken starting with commit
>> 1865e288a823c764cd4344d704e33344 ... do you have any clue what might be
>> wrong here now?
> 
> This patch seems to fix the issue for me:
> 
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index a63ed2c..03e00f7 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -756,8 +756,8 @@ static void read_cb(EEPRO100State *s)
>   
>   static void tx_command(EEPRO100State *s)
>   {
> -    uint32_t tbd_array = le32_to_cpu(s->tx.tbd_array_addr);
> -    uint16_t tcb_bytes = (le16_to_cpu(s->tx.tcb_bytes) & 0x3fff);
> +    uint32_t tbd_array = s->tx.tbd_array_addr;
> +    uint16_t tcb_bytes = s->tx.tcb_bytes & 0x3fff;
>       /* Sends larger than MAX_ETH_FRAME_SIZE are allowed, up to 2600 bytes. */
>       uint8_t buf[2600];
>       uint16_t size = 0;
> 
> Does that make sense? I think the fields are already swapped in
> read_cb(), so no need to swap them in tx_command() again?
> 
>   Thomas
> 

That makes sense to me. Not entirely sure why my patch caused the 
regression, but your patch seems like the correct behavior here.

Thanks,
Mike

[Qemu-devel] [PATCH v2 2/2] Add new PCI ID for i82559a

[Mike Nawrocki]

Adds a new PCI ID for the i82559a (0x8086 0x1030) interface. Enables
this ID with a new property "use-alt-device-id" to preserve
compatibility.

Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
---
 hw/net/eepro100.c    | 12 ++++++++++++
 include/hw/pci/pci.h |  1 +
 qemu-options.hx      |  2 +-
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 91dd058010..66f2d2b9e7 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -132,6 +132,7 @@ typedef struct {
     const char *name;
     const char *desc;
     uint16_t device_id;
+    uint16_t alt_device_id;
     uint8_t revision;
     uint16_t subsystem_vendor_id;
     uint16_t subsystem_id;
@@ -276,6 +277,7 @@ typedef struct {
     /* Quasi static device properties (no need to save them). */
     uint16_t stats_size;
     bool has_extended_tcb_support;
+    bool use_alt_device_id;
 } EEPRO100State;
 
 /* Word indices in EEPROM. */
@@ -1855,6 +1857,14 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
 
     TRACE(OTHER, logout("\n"));
 
+    /* By default, the i82559a adapter uses the legacy PCI ID (for the
+     * i82557). This allows the PCI ID to be changed to the alternate
+     * i82559 ID if needed.
+     */
+    if (s->use_alt_device_id) {
+        pci_config_set_device_id(s->dev.config, info->alt_device_id);
+    }
+
     s->device = info->device;
 
     e100_pci_reset(s, &local_err);
@@ -1974,6 +1984,7 @@ static E100PCIDeviceInfo e100_devices[] = {
         .desc = "Intel i82559A Ethernet",
         .device = i82559A,
         .device_id = PCI_DEVICE_ID_INTEL_82557,
+        .alt_device_id = PCI_DEVICE_ID_INTEL_82559,
         .revision = 0x06,
         .stats_size = 80,
         .has_extended_tcb_support = true,
@@ -2067,6 +2078,7 @@ static E100PCIDeviceInfo *eepro100_get_class(EEPRO100State *s)
 
 static Property e100_properties[] = {
     DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
+    DEFINE_PROP_BOOL("use-alt-device-id", EEPRO100State, use_alt_device_id, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 8d02a0a383..f30e2cfb72 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -70,6 +70,7 @@ extern bool pci_available;
 /* Intel (0x8086) */
 #define PCI_DEVICE_ID_INTEL_82551IT      0x1209
 #define PCI_DEVICE_ID_INTEL_82557        0x1229
+#define PCI_DEVICE_ID_INTEL_82559        0x1030
 #define PCI_DEVICE_ID_INTEL_82801IR      0x2922
 
 /* Red Hat / Qumranet (for QEMU) -- see pci-ids.txt */
diff --git a/qemu-options.hx b/qemu-options.hx
index 3728e9b4dd..a39c7e44b3 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2047,7 +2047,7 @@ that the card should have; this option currently only affects virtio cards; set
 @var{v} = 0 to disable MSI-X. If no @option{-net} option is specified, a single
 NIC is created.  QEMU can emulate several different models of network card.
 Valid values for @var{type} are
-@code{virtio}, @code{i82551}, @code{i82557b}, @code{i82559er},
+@code{virtio}, @code{i82551}, @code{i82557b}, @code{i82559a}, @code{i82559er},
 @code{ne2k_pci}, @code{ne2k_isa}, @code{pcnet}, @code{rtl8139},
 @code{e1000}, @code{smc91c111}, @code{lance} and @code{mcf_fec}.
 Not all devices are supported on all targets.  Use @code{-net nic,model=help}
-- 
2.14.2

Re: [Qemu-devel] [PATCH v2 2/2] Add new PCI ID for i82559a

[Michael S. Tsirkin]

On Mon, Nov 06, 2017 at 03:35:20PM -0500, Mike Nawrocki wrote:
> Adds a new PCI ID for the i82559a (0x8086 0x1030) interface. Enables
> this ID with a new property "use-alt-device-id" to preserve
> compatibility.
> 
> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
> ---
>  hw/net/eepro100.c    | 12 ++++++++++++
>  include/hw/pci/pci.h |  1 +
>  qemu-options.hx      |  2 +-
>  3 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 91dd058010..66f2d2b9e7 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -132,6 +132,7 @@ typedef struct {
>      const char *name;
>      const char *desc;
>      uint16_t device_id;
> +    uint16_t alt_device_id;
>      uint8_t revision;
>      uint16_t subsystem_vendor_id;
>      uint16_t subsystem_id;
> @@ -276,6 +277,7 @@ typedef struct {
>      /* Quasi static device properties (no need to save them). */
>      uint16_t stats_size;
>      bool has_extended_tcb_support;
> +    bool use_alt_device_id;
>  } EEPRO100State;
>  
>  /* Word indices in EEPROM. */
> @@ -1855,6 +1857,14 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
>  
>      TRACE(OTHER, logout("\n"));
>  
> +    /* By default, the i82559a adapter uses the legacy PCI ID (for the
> +     * i82557). This allows the PCI ID to be changed to the alternate
> +     * i82559 ID if needed.
> +     */
> +    if (s->use_alt_device_id) {
> +        pci_config_set_device_id(s->dev.config, info->alt_device_id);
> +    }
> +
>      s->device = info->device;
>  
>      e100_pci_reset(s, &local_err);
> @@ -1974,6 +1984,7 @@ static E100PCIDeviceInfo e100_devices[] = {
>          .desc = "Intel i82559A Ethernet",
>          .device = i82559A,
>          .device_id = PCI_DEVICE_ID_INTEL_82557,
> +        .alt_device_id = PCI_DEVICE_ID_INTEL_82559,
>          .revision = 0x06,
>          .stats_size = 80,
>          .has_extended_tcb_support = true,
> @@ -2067,6 +2078,7 @@ static E100PCIDeviceInfo *eepro100_get_class(EEPRO100State *s)
>  
>  static Property e100_properties[] = {
>      DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
> +    DEFINE_PROP_BOOL("use-alt-device-id", EEPRO100State, use_alt_device_id, 0),

0 -> false

>      DEFINE_PROP_END_OF_LIST(),
>  };
>  

Do I understand it correctly that the only reason we really
want it enabled by default, you only set it to 0 for
compatibility?
If yes there's a way to keep it enabled by default:
- add x- in front of the property, set to true by default,
  then tweak it to false in include/hw/compat.h


> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 8d02a0a383..f30e2cfb72 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -70,6 +70,7 @@ extern bool pci_available;
>  /* Intel (0x8086) */
>  #define PCI_DEVICE_ID_INTEL_82551IT      0x1209
>  #define PCI_DEVICE_ID_INTEL_82557        0x1229
> +#define PCI_DEVICE_ID_INTEL_82559        0x1030
>  #define PCI_DEVICE_ID_INTEL_82801IR      0x2922
>  
>  /* Red Hat / Qumranet (for QEMU) -- see pci-ids.txt */
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 3728e9b4dd..a39c7e44b3 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2047,7 +2047,7 @@ that the card should have; this option currently only affects virtio cards; set
>  @var{v} = 0 to disable MSI-X. If no @option{-net} option is specified, a single
>  NIC is created.  QEMU can emulate several different models of network card.
>  Valid values for @var{type} are
> -@code{virtio}, @code{i82551}, @code{i82557b}, @code{i82559er},
> +@code{virtio}, @code{i82551}, @code{i82557b}, @code{i82559a}, @code{i82559er},
>  @code{ne2k_pci}, @code{ne2k_isa}, @code{pcnet}, @code{rtl8139},
>  @code{e1000}, @code{smc91c111}, @code{lance} and @code{mcf_fec}.
>  Not all devices are supported on all targets.  Use @code{-net nic,model=help}
> -- 
> 2.14.2

Re: [Qemu-devel] [PATCH v2 2/2] Add new PCI ID for i82559a

[Michael Nawrocki]

On 11/07/2017 11:12 AM, Michael S. Tsirkin wrote:
> On Mon, Nov 06, 2017 at 03:35:20PM -0500, Mike Nawrocki wrote:
>> Adds a new PCI ID for the i82559a (0x8086 0x1030) interface. Enables
>> this ID with a new property "use-alt-device-id" to preserve
>> compatibility.
>>
>> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
>> ---
>>   hw/net/eepro100.c    | 12 ++++++++++++
>>   include/hw/pci/pci.h |  1 +
>>   qemu-options.hx      |  2 +-
>>   3 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
>> index 91dd058010..66f2d2b9e7 100644
>> --- a/hw/net/eepro100.c
>> +++ b/hw/net/eepro100.c
>> @@ -132,6 +132,7 @@ typedef struct {
>>       const char *name;
>>       const char *desc;
>>       uint16_t device_id;
>> +    uint16_t alt_device_id;
>>       uint8_t revision;
>>       uint16_t subsystem_vendor_id;
>>       uint16_t subsystem_id;
>> @@ -276,6 +277,7 @@ typedef struct {
>>       /* Quasi static device properties (no need to save them). */
>>       uint16_t stats_size;
>>       bool has_extended_tcb_support;
>> +    bool use_alt_device_id;
>>   } EEPRO100State;
>>   
>>   /* Word indices in EEPROM. */
>> @@ -1855,6 +1857,14 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
>>   
>>       TRACE(OTHER, logout("\n"));
>>   
>> +    /* By default, the i82559a adapter uses the legacy PCI ID (for the
>> +     * i82557). This allows the PCI ID to be changed to the alternate
>> +     * i82559 ID if needed.
>> +     */
>> +    if (s->use_alt_device_id) {
>> +        pci_config_set_device_id(s->dev.config, info->alt_device_id);
>> +    }
>> +
>>       s->device = info->device;
>>   
>>       e100_pci_reset(s, &local_err);
>> @@ -1974,6 +1984,7 @@ static E100PCIDeviceInfo e100_devices[] = {
>>           .desc = "Intel i82559A Ethernet",
>>           .device = i82559A,
>>           .device_id = PCI_DEVICE_ID_INTEL_82557,
>> +        .alt_device_id = PCI_DEVICE_ID_INTEL_82559,
>>           .revision = 0x06,
>>           .stats_size = 80,
>>           .has_extended_tcb_support = true,
>> @@ -2067,6 +2078,7 @@ static E100PCIDeviceInfo *eepro100_get_class(EEPRO100State *s)
>>   
>>   static Property e100_properties[] = {
>>       DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
>> +    DEFINE_PROP_BOOL("use-alt-device-id", EEPRO100State, use_alt_device_id, 0),
> 
> 0 -> false
> 

Gotcha.

>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
> 
> Do I understand it correctly that the only reason we really
> want it enabled by default, you only set it to 0 for
> compatibility?
> If yes there's a way to keep it enabled by default:
> - add x- in front of the property, set to true by default,
>    then tweak it to false in include/hw/compat.h
> 
> 

I think it ought to be enabled by default, and did set it to 0 for 
compatibility.

I went ahead and added "x-" to the front of the property, set it to true 
by default, and added the compat tweak. I put it in HW_COMPAT_2_10, 
which I think is the right place; let me know if anything else needs to 
be done. I'll go ahead and push out the fixes.

Thanks,
Mike

>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index 8d02a0a383..f30e2cfb72 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -70,6 +70,7 @@ extern bool pci_available;
>>   /* Intel (0x8086) */
>>   #define PCI_DEVICE_ID_INTEL_82551IT      0x1209
>>   #define PCI_DEVICE_ID_INTEL_82557        0x1229
>> +#define PCI_DEVICE_ID_INTEL_82559        0x1030
>>   #define PCI_DEVICE_ID_INTEL_82801IR      0x2922
>>   
>>   /* Red Hat / Qumranet (for QEMU) -- see pci-ids.txt */
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 3728e9b4dd..a39c7e44b3 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -2047,7 +2047,7 @@ that the card should have; this option currently only affects virtio cards; set
>>   @var{v} = 0 to disable MSI-X. If no @option{-net} option is specified, a single
>>   NIC is created.  QEMU can emulate several different models of network card.
>>   Valid values for @var{type} are
>> -@code{virtio}, @code{i82551}, @code{i82557b}, @code{i82559er},
>> +@code{virtio}, @code{i82551}, @code{i82557b}, @code{i82559a}, @code{i82559er},
>>   @code{ne2k_pci}, @code{ne2k_isa}, @code{pcnet}, @code{rtl8139},
>>   @code{e1000}, @code{smc91c111}, @code{lance} and @code{mcf_fec}.
>>   Not all devices are supported on all targets.  Use @code{-net nic,model=help}
>> -- 
>> 2.14.2

Re: [Qemu-devel] [PATCH v2 2/2] Add new PCI ID for i82559a

[Stefan Weil]

Am 06.11.2017 um 21:35 schrieb Mike Nawrocki:
> Adds a new PCI ID for the i82559a (0x8086 0x1030) interface. Enables
> this ID with a new property "use-alt-device-id" to preserve
> compatibility.
> 
> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
> ---
>  hw/net/eepro100.c    | 12 ++++++++++++
>  include/hw/pci/pci.h |  1 +
>  qemu-options.hx      |  2 +-
>  3 files changed, 14 insertions(+), 1 deletion(-)


Sorry that I missed this patch.
I think I should have an entry for eepro100 in MAINTAINERS.

Mike, which hardware uses i82559a with PCI device id 0x1030?

https://www.intel.com/content/www/us/en/support/articles/000005612/network-and-i-o/ethernet-products.html
only lists devices with
0x1229.

Thanks,
Stefan

Re: [Qemu-devel] [PATCH v2 2/2] Add new PCI ID for i82559a

[Jason Wang]

On 2017年11月15日 05:41, Stefan Weil wrote:
> Am 06.11.2017 um 21:35 schrieb Mike Nawrocki:
>> Adds a new PCI ID for the i82559a (0x8086 0x1030) interface. Enables
>> this ID with a new property "use-alt-device-id" to preserve
>> compatibility.
>>
>> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
>> ---
>>   hw/net/eepro100.c    | 12 ++++++++++++
>>   include/hw/pci/pci.h |  1 +
>>   qemu-options.hx      |  2 +-
>>   3 files changed, 14 insertions(+), 1 deletion(-)
>
> Sorry that I missed this patch.
> I think I should have an entry for eepro100 in MAINTAINERS.

Yes, please send a patch for this.

Thanks

>
> Mike, which hardware uses i82559a with PCI device id 0x1030?
>
> https://www.intel.com/content/www/us/en/support/articles/000005612/network-and-i-o/ethernet-products.html
> only lists devices with
> 0x1229.
>
> Thanks,
> Stefan

Re: [Qemu-devel] [PATCH v2 2/2] Add new PCI ID for i82559a

[Michael Nawrocki]

On 11/14/2017 04:41 PM, Stefan Weil wrote:
> Am 06.11.2017 um 21:35 schrieb Mike Nawrocki:
>> Adds a new PCI ID for the i82559a (0x8086 0x1030) interface. Enables
>> this ID with a new property "use-alt-device-id" to preserve
>> compatibility.
>>
>> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
>> ---
>>   hw/net/eepro100.c    | 12 ++++++++++++
>>   include/hw/pci/pci.h |  1 +
>>   qemu-options.hx      |  2 +-
>>   3 files changed, 14 insertions(+), 1 deletion(-)
> 
> 
> Sorry that I missed this patch.
> I think I should have an entry for eepro100 in MAINTAINERS.
> 
> Mike, which hardware uses i82559a with PCI device id 0x1030?
> 
> https://www.intel.com/content/www/us/en/support/articles/000005612/network-and-i-o/ethernet-products.html
> only lists devices with
> 0x1229.
> 
> Thanks,
> Stefan
> 

Hi Stefan,

I've got a VxWorks driver binary that explicitly looks for device ID 
0x1030 (which is admittedly not ideal). It seems like the "82559 
InBusiness 10/100" hardware uses this, though I've had trouble finding 
an official source. The following documents reference that ID:

https://pci-ids.ucw.cz/read/PC/8086/1030
http://ks.pams.ncsu.edu/pub/ncsuscyld/i386/misc/src/trees/hdstg2/modules/pcitable
https://cateee.net/lkddb/web-lkddb/E100.html

And I found a similar post on a different mailing list that might shed 
some light:
http://www.beowulf.org/pipermail/eepro100/2000-January/000760.html

It looks like the 8255x series of devices have a number of potential 
IDs; maybe a property to set a specific PCI device ID would work?

Thanks,
Mike

Re: [Qemu-devel] [PATCH v2 2/2] Add new PCI ID for i82559a

[Jason Wang]

On 2017年11月15日 21:09, Michael Nawrocki wrote:
> On 11/14/2017 04:41 PM, Stefan Weil wrote:
>> Am 06.11.2017 um 21:35 schrieb Mike Nawrocki:
>>> Adds a new PCI ID for the i82559a (0x8086 0x1030) interface. Enables
>>> this ID with a new property "use-alt-device-id" to preserve
>>> compatibility.
>>>
>>> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
>>> ---
>>>   hw/net/eepro100.c    | 12 ++++++++++++
>>>   include/hw/pci/pci.h |  1 +
>>>   qemu-options.hx      |  2 +-
>>>   3 files changed, 14 insertions(+), 1 deletion(-)
>>
>>
>> Sorry that I missed this patch.
>> I think I should have an entry for eepro100 in MAINTAINERS.
>>
>> Mike, which hardware uses i82559a with PCI device id 0x1030?
>>
>> https://www.intel.com/content/www/us/en/support/articles/000005612/network-and-i-o/ethernet-products.html 
>>
>> only lists devices with
>> 0x1229.
>>
>> Thanks,
>> Stefan
>>
>
> Hi Stefan,
>
> I've got a VxWorks driver binary that explicitly looks for device ID 
> 0x1030 (which is admittedly not ideal). It seems like the "82559 
> InBusiness 10/100" hardware uses this, though I've had trouble finding 
> an official source. The following documents reference that ID:
>
> https://pci-ids.ucw.cz/read/PC/8086/1030
> http://ks.pams.ncsu.edu/pub/ncsuscyld/i386/misc/src/trees/hdstg2/modules/pcitable 
>
> https://cateee.net/lkddb/web-lkddb/E100.html
>
> And I found a similar post on a different mailing list that might shed 
> some light:
> http://www.beowulf.org/pipermail/eepro100/2000-January/000760.html
>
> It looks like the 8255x series of devices have a number of potential 
> IDs; maybe a property to set a specific PCI device ID would work?

Maybe we should use the same method as used in e1000 which can have 
different ids depends on types.

Thanks

>
> Thanks,
> Mike

Re: [Qemu-devel] [PATCH v2 2/2] Add new PCI ID for i82559a

[Stefan Weil]

Am 15.11.2017 um 14:09 schrieb Michael Nawrocki:
> Hi Stefan,
>
> I've got a VxWorks driver binary that explicitly looks for device ID
> 0x1030 (which is admittedly not ideal). It seems like the "82559
> InBusiness 10/100" hardware uses this, though I've had trouble finding
> an official source. The following documents reference that ID:
>
> https://pci-ids.ucw.cz/read/PC/8086/1030
> http://ks.pams.ncsu.edu/pub/ncsuscyld/i386/misc/src/trees/hdstg2/modules/pcitable
>
> https://cateee.net/lkddb/web-lkddb/E100.html
>
> And I found a similar post on a different mailing list that might shed
> some light:
> http://www.beowulf.org/pipermail/eepro100/2000-January/000760.html
>
> It looks like the 8255x series of devices have a number of potential
> IDs; maybe a property to set a specific PCI device ID would work?
>
> Thanks,
> Mike

Yes, that might be a very general solution which could be applied to all
PCI devices.
It could even be extended to include the vendor ID or any PCI
configuration value
as well.

Nevertheless the technically correct solution would be a full emulation
of the
EEPROM. Then we could provide an EEPROM for the 82559 InBusiness 10/100",
and the data from that EEPROM would set the right PCI device ID.

Jason, until we get a better solution, the last commit should be
reverted before
the new QEMU version is made. That commit also added a wrong help text.

Regards
Stefan

Re: [Qemu-devel] [PATCH v2 2/2] Add new PCI ID for i82559a

[Jason Wang]

On 2017年11月16日 14:40, Stefan Weil wrote:
> Am 15.11.2017 um 14:09 schrieb Michael Nawrocki:
>> Hi Stefan,
>>
>> I've got a VxWorks driver binary that explicitly looks for device ID
>> 0x1030 (which is admittedly not ideal). It seems like the "82559
>> InBusiness 10/100" hardware uses this, though I've had trouble finding
>> an official source. The following documents reference that ID:
>>
>> https://pci-ids.ucw.cz/read/PC/8086/1030
>> http://ks.pams.ncsu.edu/pub/ncsuscyld/i386/misc/src/trees/hdstg2/modules/pcitable
>>
>> https://cateee.net/lkddb/web-lkddb/E100.html
>>
>> And I found a similar post on a different mailing list that might shed
>> some light:
>> http://www.beowulf.org/pipermail/eepro100/2000-January/000760.html
>>
>> It looks like the 8255x series of devices have a number of potential
>> IDs; maybe a property to set a specific PCI device ID would work?
>>
>> Thanks,
>> Mike
> Yes, that might be a very general solution which could be applied to all
> PCI devices.
> It could even be extended to include the vendor ID or any PCI
> configuration value
> as well.
>
> Nevertheless the technically correct solution would be a full emulation
> of the
> EEPROM. Then we could provide an EEPROM for the 82559 InBusiness 10/100",
> and the data from that EEPROM would set the right PCI device ID.
>
> Jason, until we get a better solution, the last commit should be
> reverted before
> the new QEMU version is made. That commit also added a wrong help text.
>
> Regards
> Stefan

Let me post a patch soon.

Thanks