Re: [Qemu-devel] [PATCH 1/2 V4] e1000: add the ability to select among several specific types of e1000[e]; 82566DM emulation ; some pointers to documentations and details.

[Stefan Hajnoczi]

On Fri, Mar 07, 2014 at 02:20:13PM +0100, Romain Dolbeau wrote:

Subject: e1000: add the ability to select among several specific types of 
e1000[e]; 82566DM emulation ; some pointers to documentations and details.

This subject suggests that the patch does several different things that
should be split into several smaller, logical steps:

Patch 1: e1000: introduce e1000-base class
Patch 2: e1000: add device names for more NIC models
Patch 3: e1000: add 82566DM emulation
...

Do them step-by-step and the patches will be cleaner, easier to review,
and the commit messages will be easy to come up with.

> Try to implement proper QOM
> 
> some cleaning up, and (hopefully proper) E1000State versioning support

Once the patch is split into logical steps the commit descriptions will
be easy to improve.  They should contain the rationale for the change.

> @@ -409,6 +468,9 @@ set_ctrl(E1000State *s, int index, uint32_t val)
>  {
>      /* RST is self clearing */
>      s->mac_reg[CTRL] = val & ~E1000_CTRL_RST;
> +    if (val & E1000_CTRL_RST) {
> +        e1000_reset(s);
> +    }

Maybe this should be a separate commit.  Is this a bugfix?

> @@ -1017,7 +1079,21 @@ e1000_receive_iov(NetClientState *nc, const struct 
> iovec *iov, int iovcnt)
>          } else { // as per intel docs; skip descriptors with null buf addr
>              DBGOUT(RX, "Null RX descriptor!!\n");
>          }
> -        pci_dma_write(d, base, &desc, sizeof(desc));
> +        if (!s->rxbuf_edesc) {
> +            pci_dma_write(d, base, &desc, sizeof(desc));
> +        } else { /* extended rx descriptor */
> +            union e1000_rx_desc_extended edesc;
> +            edesc.wb.lower.mrq = 0;
> +            edesc.wb.lower.hi_dword.rss = 0;
> +            /* note: we deliberately ignore desc.errors here, as it comes
> +             * from what we pci_dma_read earlier, and that wasn't
> +             * e1000_rx_desc but a e1000_rx_desc_extended.
> +             */
> +            edesc.wb.upper.status_error = desc.status;
> +            edesc.wb.upper.length = desc.length;
> +            edesc.wb.upper.vlan = desc.special;

The descriptor fields need to be little-endian.  The host could be
big-endian so use cpu_to_le32() and friends.

> @@ -1234,6 +1312,69 @@ e1000_mmio_read(void *opaque, hwaddr addr, unsigned 
> size)
>      return 0;
>  }
>  
> +static void
> +e1000_flash_write(void *opaque, hwaddr addr, uint64_t val,
> +                 unsigned size)
> +{
> +    E1000State *s = opaque;
> +    unsigned int index = addr % 2048;

Why this magic number?

> +static const uint16_t e1000_ich8_flash_template[64] = {

Where does this template come from and what is the relationship between
the EEPROM and the flash?  Does the device have both EEPROM and flash?

> @@ -1523,7 +1774,9 @@ static int pci_e1000_init(PCIDevice *pci_dev)
>  
>      pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
>  
> -    pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->io);
> +    pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->flash);
> +
> +    pci_register_bar(pci_dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->io);

How are the BARs numbered on the real device?  Do all models have flash?

We cannot change the BARs, it would break migration between old and new
QEMU.

>  
>      memmove(d->eeprom_data, e1000_eeprom_template,
>          sizeof e1000_eeprom_template);
> @@ -1531,11 +1784,42 @@ static int pci_e1000_init(PCIDevice *pci_dev)
>      macaddr = d->conf.macaddr.a;
>      for (i = 0; i < 3; i++)
>          d->eeprom_data[i] = (macaddr[2*i+1]<<8) | macaddr[2*i];
> +    /* update eeprom with the proper device_id */
> +    d->eeprom_data[11] = pdc->device_id;
> +    d->eeprom_data[13] = pdc->device_id;
>      for (i = 0; i < EEPROM_CHECKSUM_REG; i++)
>          checksum += d->eeprom_data[i];
>      checksum = (uint16_t) EEPROM_SUM - checksum;
>      d->eeprom_data[EEPROM_CHECKSUM_REG] = checksum;
> -
> +    d->rxbuf_edesc = 0;
> +    if (pdc->device_id == E1000_DEV_ID_ICH9_IGP_AMT) {
> +        /* FIXME: this changes the default values only
> +         * for one device type, whereas we probably should
> +         * have a more generic per-device way of specifying
> +         * the eeprom/flash content & extended descriptor
> +         * support */

You can define a method that e1000 subclasses can implement to
post-initialized ->eeprom_data[].  The the if statement isn't needed.

>  static void e1000_register_types(void)
>  {
> -    type_register_static(&e1000_info);
> +    int i;
> +    type_register_static(&e1000_info_abstract);

The naming is inconsistent.  Please use:

"e1000-base" with E1000_BASE() as the macro and e1000_base_info as the
TypeInfo.  (Or name it slightly differently if you like but keep it
consistent.)

> @@ -542,9 +587,9 @@
>  #define E1000_EEPROM_SWDPIN0   0x0001   /* SWDPIN 0 EEPROM Value */
>  #define E1000_EEPROM_LED_LOGIC 0x0020   /* Led Logic Word */
>  #define E1000_EEPROM_RW_REG_DATA   16   /* Offset to data in EEPROM 
> read/write registers */
> -#define E1000_EEPROM_RW_REG_DONE   0x10 /* Offset to READ/WRITE done bit */
> +#define E1000_EEPROM_RW_REG_DONE   0x2  /* Offset to READ/WRITE done bit */

Please put a change like this in a separate commit and explain the
reasoning.

>  #define E1000_EEPROM_RW_REG_START  1    /* First bit for telling part to 
> start operation */
> -#define E1000_EEPROM_RW_ADDR_SHIFT 8    /* Shift to the address bits */
> +#define E1000_EEPROM_RW_ADDR_SHIFT 2    /* Shift to the address bits */

This can go together with the E1000_EEPROM_RW_REG_DONE change.

> +/* ICH GbE Flash Hardware Sequencing Flash Status Register bit breakdown */
> +/* Offset 04h HSFSTS */
> +union ich8_hws_flash_status {
> +        struct ich8_hsfsts {
> +                uint16_t flcdone:1;  /* bit 0 Flash Cycle Done */
> +                uint16_t flcerr:1;   /* bit 1 Flash Cycle Error */
> +                uint16_t dael:1;     /* bit 2 Direct Access error Log */
> +                uint16_t berasesz:2; /* bit 4:3 Sector Erase Size */
> +                uint16_t flcinprog:1;        /* bit 5 flash cycle in 
> Progress */
> +                uint16_t reserved1:2;        /* bit 13:6 Reserved */
> +                uint16_t reserved2:6;        /* bit 13:6 Reserved */
> +                uint16_t fldesvalid:1;       /* bit 14 Flash Descriptor 
> Valid */
> +                uint16_t flockdn:1;  /* bit 15 Flash Config Lock-Down */
> +        } hsf_status;

I don't think you can use C bitfields since the bit-ordering is up to
the compiler.  It may or may not match the hardware register layout!

Instead, use uint16_t and define bitmask constants so you can use
bitwise-AND/OR.