From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34426) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLF6B-0001xS-O4 for qemu-devel@nongnu.org; Wed, 05 Mar 2014 11:55:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WLF66-0003zK-26 for qemu-devel@nongnu.org; Wed, 05 Mar 2014 11:55:15 -0500 Received: from cantor2.suse.de ([195.135.220.15]:56102 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLF65-0003zE-KJ for qemu-devel@nongnu.org; Wed, 05 Mar 2014 11:55:09 -0500 Message-ID: <531756EC.9010307@suse.de> Date: Wed, 05 Mar 2014 17:55:08 +0100 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1393924102-2325-1-git-send-email-romain@dolbeau.org> <1394028846-9408-1-git-send-email-romain@dolbeau.org> In-Reply-To: <1394028846-9408-1-git-send-email-romain@dolbeau.org> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3] e1000: add the ability to select among several specific types of e1000[e]; 82566DM emulation ; some pointers to documentations and details. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Romain Dolbeau , qemu-devel@nongnu.org Cc: Stefan Hajnoczi Am 05.03.2014 15:14, schrieb Romain Dolbeau: > Try to implement proper QOM Sorry for not getting back to you earlier but you seem to have found out. Some small nits below. >=20 > Signed-off-by: Romain Dolbeau > --- > hw/net/e1000.c | 394 +++++++++++++++++++++++++++++++++++++++++++= ++++---- > hw/net/e1000_regs.h | 149 ++++++++++++++++--- > 2 files changed, 492 insertions(+), 51 deletions(-) >=20 > diff --git a/hw/net/e1000.c b/hw/net/e1000.c > index 8387443..9737ecf 100644 > --- a/hw/net/e1000.c > +++ b/hw/net/e1000.c > @@ -1,8 +1,10 @@ > /* > * QEMU e1000 emulation > * > - * Software developer's manual: > - * http://download.intel.com/design/network/manuals/8254x_GBe_SDM.pdf > + * Software developer's manual (PCI, PCI-X): > + * > + * Software developer's manual (PCIe): > + * > * > * Nir Peleg, Tutis Systems Ltd. for Qumranet Inc. > * Copyright (c) 2008 Qumranet > @@ -10,6 +12,8 @@ > * Copyright (c) 2007 Dan Aloni > * Copyright (c) 2004 Antony T Curtis > * > + * Additional modifications (c) 2014 Romain Dolbeau > + * > * This library is free software; you can redistribute it and/or > * modify it under the terms of the GNU Lesser General Public > * License as published by the Free Software Foundation; either > @@ -58,6 +62,7 @@ static int debugflags =3D DBGBIT(TXERR) | DBGBIT(GENE= RAL); > =20 > #define IOPORT_SIZE 0x40 > #define PNPMMIO_SIZE 0x20000 > +#define FLASH_RSIZE 0x80 > #define MIN_BUF_SIZE 60 /* Min. octets in an ethernet frame sans = FCS */ > =20 > /* this is the size past which hardware will drop packets when setting= LPE=3D0 */ > @@ -69,10 +74,12 @@ static int debugflags =3D DBGBIT(TXERR) | DBGBIT(GE= NERAL); > =20 > /* > * HW models: > - * E1000_DEV_ID_82540EM works with Windows and Linux > - * E1000_DEV_ID_82573L OK with windoze and Linux 2.6.22, > - * appears to perform better than 82540EM, but breaks with Linux 2.6.1= 8 > + * E1000_DEV_ID_82540EM works with Windows and Linux, and is the defa= ult > * E1000_DEV_ID_82544GC_COPPER appears to work; not well tested > + * E1000_DEV_ID_82545EM_COPPER appears to work with OSX 10.9[.1]; not= well tested > + * E1000_DEV_ID_ICH9_IGP_AMT appears to work with Linux kernel 3.12; = not well tested > + * It seems those 3 are mostly identical anyway, so picking one > + * over the others is a matter of guest support. > * Others never tested > */ > enum { E1000_DEVID =3D E1000_DEV_ID_82540EM }; > @@ -81,10 +88,24 @@ enum { E1000_DEVID =3D E1000_DEV_ID_82540EM }; > * May need to specify additional MAC-to-PHY entries -- > * Intel's Windows driver refuses to initialize unless they match > */ > -enum { > - PHY_ID2_INIT =3D E1000_DEVID =3D=3D E1000_DEV_ID_82573L ? 0xcc2 : > - E1000_DEVID =3D=3D E1000_DEV_ID_82544GC_COPPER ? 0x= c30 : > - /* default to E1000_DEV_ID_82540EM */ 0xc20 > +/* PHY_ID1: > + * Most 8254x uses 0x141, but 82541xx and 82547GI/EI uses 0x2a8, > + * and so do the 631xESB/632xESB, 82571EB/82572EI. > + * The 82573E/82573V/82573L and 82563EB/82564EB also uses 0x141. > + * page 250 > + * page 305 > + */ > +const uint16_t PHY_ID1_INIT[][2] =3D { > + { E1000_DEV_ID_80003ES2LAN_COPPER_DPT, 0x2a8 }, > + { E1000_DEV_ID_ICH9_IGP_AMT, 0x2a8 }, > + { -1, 0x141 } /* default for 82540EM and many others */ > +}; > +const uint16_t PHY_ID2_INIT[][2] =3D { > + { E1000_DEV_ID_82573L, 0xcc2 }, > + { E1000_DEV_ID_82544GC_COPPER, 0xc30 }, > + { E1000_DEV_ID_ICH9_IGP_AMT, 0x390 }, > + { -1, 0xc20 } /* default for 82540EM and many others; this one > + is a lot more device-specific than phy_id1 */ > }; > =20 > typedef struct E1000State_st { > @@ -95,12 +116,15 @@ typedef struct E1000State_st { > NICState *nic; > NICConf conf; > MemoryRegion mmio; > + MemoryRegion flash; > MemoryRegion io; > =20 > uint32_t mac_reg[0x8000]; > uint16_t phy_reg[0x20]; > uint16_t eeprom_data[64]; > + uint16_t flash_reg[FLASH_RSIZE]; > =20 > + uint32_t rxbuf_edesc; > uint32_t rxbuf_size; > uint32_t rxbuf_min_shift; > struct e1000_tx { > @@ -151,7 +175,7 @@ typedef struct E1000State_st { > uint32_t compat_flags; > } E1000State; > =20 > -#define TYPE_E1000 "e1000" > +#define TYPE_E1000 "e1000_abstract" Convention is dashes, maybe "common-e1000"? or "base-e1000"? > =20 > #define E1000(obj) \ > OBJECT_CHECK(E1000State, (obj), TYPE_E1000) > @@ -170,6 +194,7 @@ enum { > defreg(RA), defreg(MTA), defreg(CRCERRS),defreg(VFTA), > defreg(VET), defreg(RDTR), defreg(RADV), defreg(TADV), > defreg(ITR), > + defreg(EXTCNF_CTRL), defreg(FWSM), defreg(KABGTXD), defreg(FACTPS)= , > }; > =20 > static void > @@ -229,13 +254,19 @@ static const char phy_regcap[0x20] =3D { > [PHY_CTRL] =3D PHY_RW, [PHY_1000T_CTRL] =3D PHY_RW, > [PHY_LP_ABILITY] =3D PHY_R, [PHY_1000T_STATUS] =3D PHY_R, > [PHY_AUTONEG_ADV] =3D PHY_RW, [M88E1000_RX_ERR_CNTR] =3D PHY_R, > - [PHY_ID2] =3D PHY_R, [M88E1000_PHY_SPEC_STATUS] =3D PHY_R > + [PHY_ID2] =3D PHY_R, [M88E1000_PHY_SPEC_STATUS] =3D PHY_R= , > + [ICH_IGP_PHY_REGADD_ALT_MDIO] =3D PHY_RW /* ICH IGP ? */, > + [PHY_AUTONEG_EXP] =3D PHY_RW, /* ICH IGP ? */ > + [PHY_EXT_STATUS] =3D PHY_RW, /* ICH IGP ? */ > + [M88E1000_INT_ENABLE] =3D PHY_RW, /* ICH IGP ? */ > + [M88E1000_INT_STATUS] =3D PHY_RW, /* ICH IGP ? */ > }; > =20 > static const uint16_t phy_reg_init[] =3D { > [PHY_CTRL] =3D 0x1140, > [PHY_STATUS] =3D 0x794d, /* link initially up with not completed a= utoneg */ > - [PHY_ID1] =3D 0x141, [PHY_ID2] =3D PHY_ID2_INIT, > + /* both phy_id will be replaced */ > + [PHY_ID1] =3D 0x141, [PHY_ID2] =3D 0xc20, > [PHY_1000T_CTRL] =3D 0x0e00, [M88E1000_PHY_SPEC_CTRL] =3D 0x360, > [M88E1000_EXT_PHY_SPEC_CTRL] =3D 0x0d60, [PHY_AUTONEG_ADV] =3D 0xd= e1, > [PHY_LP_ABILITY] =3D 0x1e0, [PHY_1000T_STATUS] =3D 0x3c00, > @@ -269,10 +300,11 @@ static void > set_interrupt_cause(E1000State *s, int index, uint32_t val) > { > PCIDevice *d =3D PCI_DEVICE(s); > + PCIDeviceClass *pdc =3D PCI_DEVICE_GET_CLASS(d); > uint32_t pending_ints; > uint32_t mit_delay; > =20 > - if (val && (E1000_DEVID >=3D E1000_DEV_ID_82547EI_MOBILE)) { > + if (val && (pdc->device_id >=3D E1000_DEV_ID_82547EI_MOBILE)) { > /* Only for 8257x */ > val |=3D E1000_ICR_INT_ASSERTED; > } > @@ -375,8 +407,11 @@ rxbufsize(uint32_t v) > static void e1000_reset(void *opaque) > { > E1000State *d =3D opaque; > + PCIDevice *dev =3D PCI_DEVICE(d); > + PCIDeviceClass *pdc =3D PCI_DEVICE_GET_CLASS(dev); You could just do PCI_DEVICE_GET_CLASS(d) to spare the dev variable. In particular "dev" for PCIDevice is not so nice in case we ever need DeviceState here. > uint8_t *macaddr =3D d->conf.macaddr.a; > int i; > + uint16_t phy_id1 =3D -1, phy_id2 =3D -1; > =20 > timer_del(d->autoneg_timer); > timer_del(d->mit_timer); > @@ -385,6 +420,24 @@ static void e1000_reset(void *opaque) > d->mit_ide =3D 0; > memset(d->phy_reg, 0, sizeof d->phy_reg); > memmove(d->phy_reg, phy_reg_init, sizeof phy_reg_init); > + for (i =3D 0 ; PHY_ID1_INIT[i][0] !=3D (uint16_t)-1 ; i++) { Extra spaces before semicolons. > + if (PHY_ID1_INIT[i][0] =3D=3D pdc->device_id) { > + phy_id1 =3D PHY_ID1_INIT[i][1]; > + } > + } > + if (phy_id1 =3D=3D (uint16_t)-1) { > + phy_id1 =3D PHY_ID1_INIT[i][1]; > + } > + for (i =3D 0 ; PHY_ID2_INIT[i][0] !=3D (uint16_t)-1 ; i++) { Dito. > + if (PHY_ID2_INIT[i][0] =3D=3D pdc->device_id) { > + phy_id2 =3D PHY_ID2_INIT[i][1]; > + } > + } > + if (phy_id2 =3D=3D (uint16_t)-1) { > + phy_id2 =3D PHY_ID2_INIT[i][1]; > + } > + d->phy_reg[PHY_ID1] =3D phy_id1; > + d->phy_reg[PHY_ID2] =3D phy_id2; > memset(d->mac_reg, 0, sizeof d->mac_reg); > memmove(d->mac_reg, mac_reg_init, sizeof mac_reg_init); > d->rxbuf_min_shift =3D 1; > @@ -402,6 +455,13 @@ static void e1000_reset(void *opaque) > d->mac_reg[RA + 1] |=3D (i < 2) ? macaddr[i + 4] << (8 * i) : = 0; > } > qemu_format_nic_info_str(qemu_get_queue(d->nic), macaddr); > + > + /* reset flash (clear locks) */ > + memset(d->flash_reg, 0, FLASH_RSIZE*sizeof(uint16_t)); Spaces around operators please. > + union ich8_hws_flash_status hsfsts; > + hsfsts.regval =3D 0; > + hsfsts.hsf_status.fldesvalid =3D 1; > + d->flash_reg[ICH_FLASH_HSFSTS] =3D hsfsts.regval; > } > =20 > static void > @@ -409,6 +469,9 @@ set_ctrl(E1000State *s, int index, uint32_t val) > { > /* RST is self clearing */ > s->mac_reg[CTRL] =3D val & ~E1000_CTRL_RST; > + if (val & E1000_CTRL_RST) { > + e1000_reset(s); > + } > } > =20 > static void > @@ -1017,7 +1080,17 @@ e1000_receive_iov(NetClientState *nc, const stru= ct 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 =3D 0; > + edesc.wb.lower.hi_dword.rss =3D 0; > + edesc.wb.upper.status_error =3D /* desc.errors << 24 | */ = desc.status; Why is this commented out? > + edesc.wb.upper.length =3D desc.length; > + edesc.wb.upper.vlan =3D desc.special; > + pci_dma_write(d, base, &edesc, sizeof(edesc)); > + } > =20 > if (++s->mac_reg[RDH] * sizeof(desc) >=3D s->mac_reg[RDLEN]) > s->mac_reg[RDH] =3D 0; > @@ -1173,6 +1246,7 @@ static uint32_t (*macreg_readops[])(E1000State *,= int) =3D { > getreg(TDBAL), getreg(TDBAH), getreg(RDBAH), getreg(RDBAL), > getreg(TDLEN), getreg(RDLEN), getreg(RDTR), getreg(RADV), > getreg(TADV), getreg(ITR), > + getreg(EXTCNF_CTRL), getreg(FWSM), getreg(KABGTXD), getreg(FACTPS)= , > =20 > [TOTH] =3D mac_read_clr8, [TORH] =3D mac_read_clr8, [GPRC] =3D mac= _read_clr4, > [GPTC] =3D mac_read_clr4, [TPR] =3D mac_read_clr4, [TPT] =3D mac_r= ead_clr4, > @@ -1189,6 +1263,7 @@ static void (*macreg_writeops[])(E1000State *, in= t, uint32_t) =3D { > putreg(PBA), putreg(EERD), putreg(SWSM), putreg(WUFC), > putreg(TDBAL), putreg(TDBAH), putreg(TXDCTL), putreg(RDBAH), > putreg(RDBAL), putreg(LEDCTL), putreg(VET), > + putreg(EXTCNF_CTRL), putreg(FWSM), putreg(KABGTXD), putreg(FACTPS)= , > [TDLEN] =3D set_dlen, [RDLEN] =3D set_dlen, [TCTL] =3D set_tctl, > [TDT] =3D set_tctl, [MDIC] =3D set_mdic, [ICS] =3D set_ics, > [TDH] =3D set_16bit, [RDH] =3D set_16bit, [RDT] =3D set_rdt, > @@ -1234,6 +1309,73 @@ e1000_mmio_read(void *opaque, hwaddr addr, unsig= ned size) > return 0; > } > =20 > + > + > + White lines intentional? > +static void > +e1000_flash_write(void *opaque, hwaddr addr, uint64_t val, > + unsigned size) > +{ > + E1000State *s =3D opaque; > + unsigned int index =3D addr % 2048; > + > + if (index < FLASH_RSIZE) { > + s->flash_reg[index] =3D val & 0xFFFF; > + switch (index) { > + case ICH_FLASH_HSFSTS: { > + break; > + } Braces not strictly necessary here (no variables). > + case ICH_FLASH_HSFCTL: { > + union ich8_hws_flash_ctrl ctrl; > + ctrl.regval =3D s->flash_reg[ICH_FLASH_HSFCTL]; > + if (ctrl.hsf_ctrl.flcgo) { > + /* says we're done, clear go, > + copy data to proper register */ > + union ich8_hws_flash_status hsfsts; > + int fldbcount; > + uint16_t offset; > + uint16_t res; > + hsfsts.regval =3D s->flash_reg[ICH_FLASH_HSFSTS]; > + hsfsts.hsf_status.flcdone =3D 1; > + hsfsts.hsf_status.flcerr =3D 0; > + s->flash_reg[ICH_FLASH_HSFSTS] =3D hsfsts.regval; > + fldbcount =3D ctrl.hsf_ctrl.fldbcount; > + ctrl.hsf_ctrl.flcgo =3D 0; > + s->flash_reg[ICH_FLASH_HSFCTL] =3D ctrl.regval; > + offset =3D s->flash_reg[ICH_FLASH_FADDR] >> 1; > + res =3D s->eeprom_data[offset]; > + if (fldbcount =3D=3D 0) { > + if (s->flash_reg[ICH_FLASH_FADDR] % 2) { > + res =3D res >> 8; > + } else { > + res =3D res & 0x00FF; > + } > + } > + s->flash_reg[ICH_FLASH_FDATA0] =3D res; > + } > + } > + default: > + break; > + } > + } else { > + DBGOUT(UNKNOWN, "Flash unknown write addr=3D0x%08x,val=3D0x%08= "PRIx64"\n", > + index<<2, val); Spaces > + } > +} > + > +static uint64_t > +e1000_flash_read(void *opaque, hwaddr addr, unsigned size) > +{ > + E1000State *s =3D opaque; > + unsigned int index =3D addr % 2048; > + > + if (index < FLASH_RSIZE) { > + return s->flash_reg[index]; > + } > + DBGOUT(UNKNOWN, "Flash unknown read addr=3D0x%08x\n", index<<2); Spaces > + return 0; > +} > + > static const MemoryRegionOps e1000_mmio_ops =3D { > .read =3D e1000_mmio_read, > .write =3D e1000_mmio_write, > @@ -1244,6 +1386,16 @@ static const MemoryRegionOps e1000_mmio_ops =3D = { > }, > }; > =20 > +static const MemoryRegionOps e1000_flash_ops =3D { > + .read =3D e1000_flash_read, > + .write =3D e1000_flash_write, > + .endianness =3D DEVICE_LITTLE_ENDIAN, > + .impl =3D { > + .min_access_size =3D 4, > + .max_access_size =3D 4, > + }, > +}; > + > static uint64_t e1000_io_read(void *opaque, hwaddr addr, > unsigned size) > { > @@ -1425,6 +1577,8 @@ static const VMStateDescription vmstate_e1000 =3D= { > VMSTATE_UINT32(mac_reg[TXDCTL], E1000State), > VMSTATE_UINT32(mac_reg[WUFC], E1000State), > VMSTATE_UINT32(mac_reg[VET], E1000State), > + VMSTATE_UINT32(mac_reg[EXTCNF_CTRL], E1000State), > + VMSTATE_UINT32(mac_reg[FWSM], E1000State), > VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, RA, 32), > VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, MTA, 128), > VMSTATE_UINT32_SUB_ARRAY(mac_reg, E1000State, VFTA, 128), You can't just add fields here, you need to use VMSTATE_UINT32_V() and increment the version number. Or use a subsection conditional on whatever makes these fields relevant. Keep in mind that -device e1000 from v1.7 would have the fields initialized to whatever init/reset set them to. > @@ -1440,15 +1594,106 @@ static const VMStateDescription vmstate_e1000 = =3D { > } > }; > =20 > +/* > + * The content of EEPROM is documented in the documentation > + * PCI/X: "Table 5-2. Ethernet Controller Address Map" page 98 (except= 82544GC/EI and 82541ER) > + * PCI/X: "Table 5-3. 82544GC/EI and 82541ER EEPROM Address Map" page = 102 > + * PCIe: "Table 5-2. Ethernet Controller EEPROM Map" page 134 > + */ > static const uint16_t e1000_eeprom_template[64] =3D { > - 0x0000, 0x0000, 0x0000, 0x0000, 0xffff, 0x0000, 0x0000, = 0x0000, > - 0x3000, 0x1000, 0x6403, E1000_DEVID, 0x8086, E1000_DEVID, 0x8086, = 0x3040, > - 0x0008, 0x2000, 0x7e14, 0x0048, 0x1000, 0x00d8, 0x0000, = 0x2700, > - 0x6cc9, 0x3150, 0x0722, 0x040b, 0x0984, 0x0000, 0xc000, = 0x0706, > - 0x1008, 0x0000, 0x0f04, 0x7fff, 0x4d01, 0xffff, 0xffff, = 0xffff, > - 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, = 0xffff, > - 0x0100, 0x4000, 0x121c, 0xffff, 0xffff, 0xffff, 0xffff, = 0xffff, > - 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, = 0x0000, > + /* 00h - 02h: Ethernet address, will be overridden */ > + 0x0000, 0x0000, 0x0000, > + /* 03h: compatibility, this seems a bit device-specific > + and probably should be overridden */ > + 0x0000, > + /* 04h: compatibility (PCIe) or SerDes config (most PCI/X) or LED = */ > + 0xffff, > + /* 05h - 07h: EEprom version & OEM (PCIe other than 82573), > + compatibility (most PCI/X, 82573) */ > + 0x0000, 0x0000, 0x0000, > + /* 08h - 09h: PBA (irrelevant) */ > + 0x3000, 0x1000, > + /* 0ah: init control 1 */ > + 0x6403, > + /* 0bh - 0eh: PCI ID, will be overridden */ > + E1000_DEVID, 0x8086, E1000_DEVID, 0x8086, > + /* 0fh: init control 2 */ > + 0x3040, > + /* 10h - 12h: seem quite device-specific with several variants */ > + 0x0008, 0x2000, 0x7e14, > + /* 13h: management */ > + 0x0048, > + /* 14h: init control 3 (2nd LAN?), not 82573 */ > + 0x1000, > + /* 15h - 16h: IPv4 (PCI/X) or reserved (PCIe), not 82573 */ > + 0x00d8, 0x0000, > + /* 17h - 1Eh: Another batch of variants; IPv6 LAN in PCI/X > + * but is FW Config Start Address (17h, most PCIe) followed > + * by PCI init configuration and stuff > + */ > + 0x2700, 0x6cc9, 0x3150, 0x0722, 0x040b, 0x0984, 0x0000, 0xc000, > + /* 1fh: mostly reserved (PCI/X), LED conf (PCIe) */ > + 0x0706, > + /* 20h - 21h: software defined pin controls, 21h: mostly control *= / > + 0x1008, 0x0000, > + /* 22h - 23h: LAN power, management control (not 82573) */ > + 0x0f04, 0x7fff, > + /* 24h: init control 3 (again?) */ > + 0x4d01, > + /* 25h-2eh: either IP4/6 (PCI/X) or mostly reserved (PCIe) */ > + 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, > + 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, > + /* 2fh: LEDCTL Default (PCI/X) or Vital Product Data (VPD) Pointer= (PCIe) */ > + 0xffff, > + /* 30h-3eh: mostly PXE/boot stuff */ > + 0x0100, 0x4000, 0x121c, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, > + 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, > + /* 3fh: checksum to be computed */ > + 0x0000 > +}; > +static const uint16_t e1000_ich8_flash_template[64] =3D { > + /* 00h - 02h: Ethernet address, will be overridden */ > + 0x0000, 0x0000, 0x0000, > + /* 03h - 04h: reserved */ > + 0x0800, 0xFFFF, > + /* 05h - 07h: image version information, reserved*/ > + 0x0000, 0xFFFF, 0xFFFF, > + /* 08h - 09h: PBA (irrelevant) */ > + 0x3000, 0x1000, > + /* 0ah: init control 1 */ > + 0x10c7, > + /* 0bh - 0eh: PCI ID, will be overridden */ > + E1000_DEVID, 0x8086, E1000_DEVID, 0x8086, > + /* 0fh: device revision id (ich9), reserved (ich8) */ > + 0x0000, /* fixme */ > + /* 10h - 12h: LAN power consumption, reserved */ > + 0x0D01, 0x0000, 0x0000, > + /* 13h: Shared Initialization Control Word */ > + 0x9607, > + /* 14h - 16h: extended configuration word 1-3 */ > + 0x7020, 0x3800, 0x0000, > + /* 17h - 18h: LEDCTL 1, LEDCTL 0 2 */ > + 0x8d07, 0x0684, > + /* 19h - 1ah: future initialization words */ > + (0x0181 | 0x0040), 0x0800, > + /* 1bh - 1dh: reserved */ > + 0x0000, 0x294C, 0x294C, > + /* 1eh - 1fh: device id (some devices) */ > + 0x10BE, 0x10BF, > + /* 20h - 21h: reserved */ > + 0x294C, 0x294C, > + /* 22h - 23h: device id (some devices) */ > + 0x10bd, 0x294C, > + /* 24h-2fh: reserved */ > + 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, > + 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, > + 0xffff, 0xffff, > + /* 30h-3eh: mostly PXE/boot stuff & reserved */ > + 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, > + 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, > + 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, > + /* 3fh: checksum to be computed */ > + 0x0000 > }; > =20 > /* PCI interface */ > @@ -1468,6 +1713,10 @@ e1000_mmio_setup(E1000State *d) > for (i =3D 0; excluded_regs[i] !=3D PNPMMIO_SIZE; i++) > memory_region_add_coalescing(&d->mmio, excluded_regs[i] + 4, > excluded_regs[i+1] - excluded_reg= s[i] - 4); > + > + memory_region_init_io(&d->flash, OBJECT(d), &e1000_flash_ops, d, > + "e1000-flash", FLASH_RSIZE); > + > memory_region_init_io(&d->io, OBJECT(d), &e1000_io_ops, d, "e1000-= io", IOPORT_SIZE); > } > =20 > @@ -1506,6 +1755,7 @@ static NetClientInfo net_e1000_info =3D { > static int pci_e1000_init(PCIDevice *pci_dev) > { > DeviceState *dev =3D DEVICE(pci_dev); > + PCIDeviceClass *pdc =3D PCI_DEVICE_GET_CLASS(pci_dev); > E1000State *d =3D E1000(pci_dev); > uint8_t *pci_conf; > uint16_t checksum =3D 0; > @@ -1523,7 +1773,9 @@ static int pci_e1000_init(PCIDevice *pci_dev) > =20 > pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mm= io); > =20 > - 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->fl= ash); > + > + pci_register_bar(pci_dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->io); > =20 > memmove(d->eeprom_data, e1000_eeprom_template, > sizeof e1000_eeprom_template); I wonder if this change is migration-compatible? > @@ -1531,11 +1783,38 @@ static int pci_e1000_init(PCIDevice *pci_dev) > macaddr =3D d->conf.macaddr.a; > for (i =3D 0; i < 3; i++) > d->eeprom_data[i] =3D (macaddr[2*i+1]<<8) | macaddr[2*i]; > + /* update eeprom with the proper device_id */ > + d->eeprom_data[11] =3D pdc->device_id; > + d->eeprom_data[13] =3D pdc->device_id; > for (i =3D 0; i < EEPROM_CHECKSUM_REG; i++) > checksum +=3D d->eeprom_data[i]; > checksum =3D (uint16_t) EEPROM_SUM - checksum; > d->eeprom_data[EEPROM_CHECKSUM_REG] =3D checksum; > - > + d->rxbuf_edesc =3D 0; > + if (pdc->device_id =3D=3D E1000_DEV_ID_ICH9_IGP_AMT) { > + /* fixme: other devices */ Is this a FIXME that you intend to fix before it's applied? Otherwise suggest to use "FIXME:" for highlighting in editors. > + for (i =3D 0 ; i < EEPROM_CHECKSUM_REG; i++) { Extra space > + d->eeprom_data[i] =3D e1000_ich8_flash_template[i]; > + } > + for (i =3D 0; i < 3; i++) { > + d->eeprom_data[i] =3D (macaddr[2*i+1]<<8) | macaddr[2*i]; Spaces around << at least please, not sure about file's surrounding style - Stefan's call. > + } > + d->eeprom_data[11] =3D pdc->device_id; > + d->eeprom_data[13] =3D pdc->device_id; > + checksum =3D 0; > + for (i =3D 0; i < EEPROM_CHECKSUM_REG; i++) { > + checksum +=3D d->eeprom_data[i]; > + } > + checksum =3D (uint16_t) EEPROM_SUM - checksum; > + d->eeprom_data[EEPROM_CHECKSUM_REG] =3D checksum; > + /* init flash registers */ > + memset(d->flash_reg, 0, FLASH_RSIZE*sizeof(uint16_t)); Spaces > + union ich8_hws_flash_status hsfsts; Move to beginning of if block? > + hsfsts.regval =3D 0; > + hsfsts.hsf_status.fldesvalid =3D 1; > + d->flash_reg[ICH_FLASH_HSFSTS] =3D hsfsts.regval; > + d->rxbuf_edesc =3D 1; > + } > d->nic =3D qemu_new_nic(&net_e1000_info, &d->conf, > object_get_typename(OBJECT(d)), dev->id, d); > =20 > @@ -1551,7 +1830,8 @@ static int pci_e1000_init(PCIDevice *pci_dev) > =20 > static void qdev_e1000_reset(DeviceState *dev) > { > - E1000State *d =3D E1000(dev); > + PCIDevice *pci_dev =3D PCI_DEVICE(dev); > + E1000State *d =3D E1000(pci_dev); Just E1000(dev), they no longer depend on each other. > e1000_reset(d); > } > =20 > @@ -1564,17 +1844,26 @@ static Property e1000_properties[] =3D { > DEFINE_PROP_END_OF_LIST(), > }; > =20 > +typedef struct E1000Info E1000Info; > +struct E1000Info { > + const char *name; > + uint16_t vendor_id; > + uint16_t device_id; > + uint8_t revision; > +}; > + > static void e1000_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc =3D DEVICE_CLASS(klass); > PCIDeviceClass *k =3D PCI_DEVICE_CLASS(klass); > + E1000Info *info =3D data; const > =20 > k->init =3D pci_e1000_init; > k->exit =3D pci_e1000_uninit; > k->romfile =3D "efi-e1000.rom"; > - k->vendor_id =3D PCI_VENDOR_ID_INTEL; > - k->device_id =3D E1000_DEVID; > - k->revision =3D 0x03; > + k->vendor_id =3D info->vendor_id; > + k->device_id =3D info->device_id; > + k->revision =3D info->revision; > k->class_id =3D PCI_CLASS_NETWORK_ETHERNET; > set_bit(DEVICE_CATEGORY_NETWORK, dc->categories); > dc->desc =3D "Intel Gigabit Ethernet"; > @@ -1583,16 +1872,59 @@ static void e1000_class_init(ObjectClass *klass= , void *data) > dc->props =3D e1000_properties; > } > =20 > -static const TypeInfo e1000_info =3D { > +static E1000Info e1000_info_array[] =3D { static const > + { > + .name =3D "e1000", > + .vendor_id =3D PCI_VENDOR_ID_INTEL, > + .device_id =3D E1000_DEVID, > + .revision =3D 0x03, > + }, > + { > + .name =3D "82540EM", > + .vendor_id =3D PCI_VENDOR_ID_INTEL, > + .device_id =3D E1000_DEV_ID_82540EM, > + .revision =3D 0x03, > + }, > + { > + .name =3D "82544GC", > + .vendor_id =3D PCI_VENDOR_ID_INTEL, > + .device_id =3D E1000_DEV_ID_82544GC_COPPER, > + .revision =3D 0x03, > + }, > + { > + .name =3D "82545EM", > + .vendor_id =3D PCI_VENDOR_ID_INTEL, > + .device_id =3D E1000_DEV_ID_82545EM_COPPER, > + .revision =3D 0x03, > + }, > + { > + .name =3D "82566DM", > + .vendor_id =3D PCI_VENDOR_ID_INTEL, > + .device_id =3D E1000_DEV_ID_ICH9_IGP_AMT, > + .revision =3D 0x00, > + } > +}; > + > +static const TypeInfo e1000_info_abstract =3D { > .name =3D TYPE_E1000, > .parent =3D TYPE_PCI_DEVICE, > .instance_size =3D sizeof(E1000State), > - .class_init =3D e1000_class_init, > + .abstract =3D true, > }; > =20 > static void e1000_register_types(void) > { > - type_register_static(&e1000_info); > + int i; > + type_register_static(&e1000_info_abstract); > + for (i =3D 0; i < ARRAY_SIZE(e1000_info_array); i++) { > + TypeInfo e1000_info =3D { > + .name =3D e1000_info_array[i].name, > + .parent =3D TYPE_E1000, > + .class_data =3D e1000_info_array + i, (void *)&e1000_info_array[i] would match above usage. > + .class_init =3D e1000_class_init, > + }; > + type_register(&e1000_info); > + } > } > =20 > type_init(e1000_register_types) [snip] Otherwise looks good now! Thanks, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg