Re: [Qemu-devel] [PULL 7/8] Add new PCI ID for i82559a

[Stefan Weil]

Hi,

I currently think that this patch is wrong and should be reverted.

It fixes a certain use case by hacking the PCI device id, but does
not model the way how that device id is set on the real hardware
correctly.

As far as I know, all i82559 have a default PCI device id of 0x1229.
It can be changed by the EEPROM configuration, but not all network
cards do have an EEPROM.

See for example this URL for more information:
http://zoo.cs.yale.edu/classes/cs422/2010/ref/82559_eeprom.pdf

The correct solution is modeling the EEPROM and allowing QEMU
users to provide an EEPROM‌ file.

Cheers
Stefan

Am 14.11.2017 um 03:11 schrieb Jason Wang:
> From: Mike Nawrocki <address@hidden>
> 
> Adds a new PCI ID for the i82559a (0x8086 0x1030) interface. The
> "x-use-alt-device-id" property controls whether this new ID is to be
> used, and is true by default, and set to false in a compat entry.
> 
> Signed-off-by: Mike Nawrocki <address@hidden>
> Reviewed-by: Michael S. Tsirkin <address@hidden>
> Signed-off-by: Jason Wang <address@hidden>
> ---
>  hw/net/eepro100.c    | 13 +++++++++++++
>  include/hw/compat.h  |  4 ++++
>  include/hw/pci/pci.h |  1 +
>  qemu-options.hx      |  2 +-
>  4 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 91dd058..a63ed2c 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 && strcmp(info->name, "i82559a") == 0) {
> +        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,8 @@ static E100PCIDeviceInfo 
> *eepro100_get_class(EEPRO100State *s)
>  
>  static Property e100_properties[] = {
>      DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
> +    DEFINE_PROP_BOOL("x-use-alt-device-id", EEPRO100State, use_alt_device_id,
> +                     true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index cf389b4..f96212c 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -10,6 +10,10 @@
>          .driver   = "virtio-tablet-device",\
>          .property = "wheel-axis",\
>          .value    = "false",\
> +    },{\
> +        .driver   = "i82559a",\
> +        .property = "x-use-alt-device-id",\
> +        .value    = "false",\
>      },
>  
>  #define HW_COMPAT_2_9 \
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 8d02a0a..f30e2cf 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 3728e9b..a39c7e4 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
> address@hidden, @code{i82551}, @code{i82557b}, @code{i82559er},
> address@hidden, @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}
>