From: "Andreas Färber" <afaerber@suse.de>
To: Romain Dolbeau <romain@dolbeau.org>, qemu-devel@nongnu.org
Cc: Stefan Hajnoczi <stefanha@redhat.com>
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.
Date: Wed, 05 Mar 2014 17:55:08 +0100 [thread overview]
Message-ID: <531756EC.9010307@suse.de> (raw)
In-Reply-To: <1394028846-9408-1-git-send-email-romain@dolbeau.org>
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.
>
> Signed-off-by: Romain Dolbeau <romain@dolbeau.org>
> ---
> hw/net/e1000.c | 394 +++++++++++++++++++++++++++++++++++++++++++++++----
> hw/net/e1000_regs.h | 149 ++++++++++++++++---
> 2 files changed, 492 insertions(+), 51 deletions(-)
>
> 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):
> + * <http://download.intel.com/design/network/manuals/8254x_GBe_SDM.pdf>
> + * Software developer's manual (PCIe):
> + * <http://www.intel.com/content/dam/www/public/us/en/documents/manuals/pcie-gbe-controllers-open-source-manual.pdf>
> *
> * 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 = DBGBIT(TXERR) | DBGBIT(GENERAL);
>
> #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 */
>
> /* this is the size past which hardware will drop packets when setting LPE=0 */
> @@ -69,10 +74,12 @@ static int debugflags = DBGBIT(TXERR) | DBGBIT(GENERAL);
>
> /*
> * 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.18
> + * E1000_DEV_ID_82540EM works with Windows and Linux, and is the default
> * 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 = E1000_DEV_ID_82540EM };
> @@ -81,10 +88,24 @@ enum { E1000_DEVID = 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 = E1000_DEVID == E1000_DEV_ID_82573L ? 0xcc2 :
> - E1000_DEVID == E1000_DEV_ID_82544GC_COPPER ? 0xc30 :
> - /* 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.
> + * <http://download.intel.com/design/network/manuals/8254x_GBe_SDM.pdf> page 250
> + * <http://www.intel.com/content/dam/www/public/us/en/documents/manuals/pcie-gbe-controllers-open-source-manual.pdf> page 305
> + */
> +const uint16_t PHY_ID1_INIT[][2] = {
> + { 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] = {
> + { 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 */
> };
>
> typedef struct E1000State_st {
> @@ -95,12 +116,15 @@ typedef struct E1000State_st {
> NICState *nic;
> NICConf conf;
> MemoryRegion mmio;
> + MemoryRegion flash;
> MemoryRegion io;
>
> uint32_t mac_reg[0x8000];
> uint16_t phy_reg[0x20];
> uint16_t eeprom_data[64];
> + uint16_t flash_reg[FLASH_RSIZE];
>
> + 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;
>
> -#define TYPE_E1000 "e1000"
> +#define TYPE_E1000 "e1000_abstract"
Convention is dashes, maybe "common-e1000"? or "base-e1000"?
>
> #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),
> };
>
> static void
> @@ -229,13 +254,19 @@ static const char phy_regcap[0x20] = {
> [PHY_CTRL] = PHY_RW, [PHY_1000T_CTRL] = PHY_RW,
> [PHY_LP_ABILITY] = PHY_R, [PHY_1000T_STATUS] = PHY_R,
> [PHY_AUTONEG_ADV] = PHY_RW, [M88E1000_RX_ERR_CNTR] = PHY_R,
> - [PHY_ID2] = PHY_R, [M88E1000_PHY_SPEC_STATUS] = PHY_R
> + [PHY_ID2] = PHY_R, [M88E1000_PHY_SPEC_STATUS] = PHY_R,
> + [ICH_IGP_PHY_REGADD_ALT_MDIO] = PHY_RW /* ICH IGP ? */,
> + [PHY_AUTONEG_EXP] = PHY_RW, /* ICH IGP ? */
> + [PHY_EXT_STATUS] = PHY_RW, /* ICH IGP ? */
> + [M88E1000_INT_ENABLE] = PHY_RW, /* ICH IGP ? */
> + [M88E1000_INT_STATUS] = PHY_RW, /* ICH IGP ? */
> };
>
> static const uint16_t phy_reg_init[] = {
> [PHY_CTRL] = 0x1140,
> [PHY_STATUS] = 0x794d, /* link initially up with not completed autoneg */
> - [PHY_ID1] = 0x141, [PHY_ID2] = PHY_ID2_INIT,
> + /* both phy_id will be replaced */
> + [PHY_ID1] = 0x141, [PHY_ID2] = 0xc20,
> [PHY_1000T_CTRL] = 0x0e00, [M88E1000_PHY_SPEC_CTRL] = 0x360,
> [M88E1000_EXT_PHY_SPEC_CTRL] = 0x0d60, [PHY_AUTONEG_ADV] = 0xde1,
> [PHY_LP_ABILITY] = 0x1e0, [PHY_1000T_STATUS] = 0x3c00,
> @@ -269,10 +300,11 @@ static void
> set_interrupt_cause(E1000State *s, int index, uint32_t val)
> {
> PCIDevice *d = PCI_DEVICE(s);
> + PCIDeviceClass *pdc = PCI_DEVICE_GET_CLASS(d);
> uint32_t pending_ints;
> uint32_t mit_delay;
>
> - if (val && (E1000_DEVID >= E1000_DEV_ID_82547EI_MOBILE)) {
> + if (val && (pdc->device_id >= E1000_DEV_ID_82547EI_MOBILE)) {
> /* Only for 8257x */
> val |= E1000_ICR_INT_ASSERTED;
> }
> @@ -375,8 +407,11 @@ rxbufsize(uint32_t v)
> static void e1000_reset(void *opaque)
> {
> E1000State *d = opaque;
> + PCIDevice *dev = PCI_DEVICE(d);
> + PCIDeviceClass *pdc = 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 = d->conf.macaddr.a;
> int i;
> + uint16_t phy_id1 = -1, phy_id2 = -1;
>
> timer_del(d->autoneg_timer);
> timer_del(d->mit_timer);
> @@ -385,6 +420,24 @@ static void e1000_reset(void *opaque)
> d->mit_ide = 0;
> memset(d->phy_reg, 0, sizeof d->phy_reg);
> memmove(d->phy_reg, phy_reg_init, sizeof phy_reg_init);
> + for (i = 0 ; PHY_ID1_INIT[i][0] != (uint16_t)-1 ; i++) {
Extra spaces before semicolons.
> + if (PHY_ID1_INIT[i][0] == pdc->device_id) {
> + phy_id1 = PHY_ID1_INIT[i][1];
> + }
> + }
> + if (phy_id1 == (uint16_t)-1) {
> + phy_id1 = PHY_ID1_INIT[i][1];
> + }
> + for (i = 0 ; PHY_ID2_INIT[i][0] != (uint16_t)-1 ; i++) {
Dito.
> + if (PHY_ID2_INIT[i][0] == pdc->device_id) {
> + phy_id2 = PHY_ID2_INIT[i][1];
> + }
> + }
> + if (phy_id2 == (uint16_t)-1) {
> + phy_id2 = PHY_ID2_INIT[i][1];
> + }
> + d->phy_reg[PHY_ID1] = phy_id1;
> + d->phy_reg[PHY_ID2] = 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 = 1;
> @@ -402,6 +455,13 @@ static void e1000_reset(void *opaque)
> d->mac_reg[RA + 1] |= (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 = 0;
> + hsfsts.hsf_status.fldesvalid = 1;
> + d->flash_reg[ICH_FLASH_HSFSTS] = hsfsts.regval;
> }
>
> static void
> @@ -409,6 +469,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);
> + }
> }
>
> static void
> @@ -1017,7 +1080,17 @@ 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;
> + edesc.wb.upper.status_error = /* desc.errors << 24 | */ desc.status;
Why is this commented out?
> + edesc.wb.upper.length = desc.length;
> + edesc.wb.upper.vlan = desc.special;
> + pci_dma_write(d, base, &edesc, sizeof(edesc));
> + }
>
> if (++s->mac_reg[RDH] * sizeof(desc) >= s->mac_reg[RDLEN])
> s->mac_reg[RDH] = 0;
> @@ -1173,6 +1246,7 @@ static uint32_t (*macreg_readops[])(E1000State *, int) = {
> 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),
>
> [TOTH] = mac_read_clr8, [TORH] = mac_read_clr8, [GPRC] = mac_read_clr4,
> [GPTC] = mac_read_clr4, [TPR] = mac_read_clr4, [TPT] = mac_read_clr4,
> @@ -1189,6 +1263,7 @@ static void (*macreg_writeops[])(E1000State *, int, uint32_t) = {
> 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] = set_dlen, [RDLEN] = set_dlen, [TCTL] = set_tctl,
> [TDT] = set_tctl, [MDIC] = set_mdic, [ICS] = set_ics,
> [TDH] = set_16bit, [RDH] = set_16bit, [RDT] = set_rdt,
> @@ -1234,6 +1309,73 @@ e1000_mmio_read(void *opaque, hwaddr addr, unsigned size)
> return 0;
> }
>
> +
> +
> +
White lines intentional?
> +static void
> +e1000_flash_write(void *opaque, hwaddr addr, uint64_t val,
> + unsigned size)
> +{
> + E1000State *s = opaque;
> + unsigned int index = addr % 2048;
> +
> + if (index < FLASH_RSIZE) {
> + s->flash_reg[index] = 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 = 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 = s->flash_reg[ICH_FLASH_HSFSTS];
> + hsfsts.hsf_status.flcdone = 1;
> + hsfsts.hsf_status.flcerr = 0;
> + s->flash_reg[ICH_FLASH_HSFSTS] = hsfsts.regval;
> + fldbcount = ctrl.hsf_ctrl.fldbcount;
> + ctrl.hsf_ctrl.flcgo = 0;
> + s->flash_reg[ICH_FLASH_HSFCTL] = ctrl.regval;
> + offset = s->flash_reg[ICH_FLASH_FADDR] >> 1;
> + res = s->eeprom_data[offset];
> + if (fldbcount == 0) {
> + if (s->flash_reg[ICH_FLASH_FADDR] % 2) {
> + res = res >> 8;
> + } else {
> + res = res & 0x00FF;
> + }
> + }
> + s->flash_reg[ICH_FLASH_FDATA0] = res;
> + }
> + }
> + default:
> + break;
> + }
> + } else {
> + DBGOUT(UNKNOWN, "Flash unknown write addr=0x%08x,val=0x%08"PRIx64"\n",
> + index<<2, val);
Spaces
> + }
> +}
> +
> +static uint64_t
> +e1000_flash_read(void *opaque, hwaddr addr, unsigned size)
> +{
> + E1000State *s = opaque;
> + unsigned int index = addr % 2048;
> +
> + if (index < FLASH_RSIZE) {
> + return s->flash_reg[index];
> + }
> + DBGOUT(UNKNOWN, "Flash unknown read addr=0x%08x\n", index<<2);
Spaces
> + return 0;
> +}
> +
> static const MemoryRegionOps e1000_mmio_ops = {
> .read = e1000_mmio_read,
> .write = e1000_mmio_write,
> @@ -1244,6 +1386,16 @@ static const MemoryRegionOps e1000_mmio_ops = {
> },
> };
>
> +static const MemoryRegionOps e1000_flash_ops = {
> + .read = e1000_flash_read,
> + .write = e1000_flash_write,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> + .impl = {
> + .min_access_size = 4,
> + .max_access_size = 4,
> + },
> +};
> +
> static uint64_t e1000_io_read(void *opaque, hwaddr addr,
> unsigned size)
> {
> @@ -1425,6 +1577,8 @@ static const VMStateDescription vmstate_e1000 = {
> 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 = {
> }
> };
>
> +/*
> + * 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] = {
> - 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] = {
> + /* 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
> };
>
> /* PCI interface */
> @@ -1468,6 +1713,10 @@ e1000_mmio_setup(E1000State *d)
> for (i = 0; excluded_regs[i] != PNPMMIO_SIZE; i++)
> memory_region_add_coalescing(&d->mmio, excluded_regs[i] + 4,
> excluded_regs[i+1] - excluded_regs[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);
> }
>
> @@ -1506,6 +1755,7 @@ static NetClientInfo net_e1000_info = {
> static int pci_e1000_init(PCIDevice *pci_dev)
> {
> DeviceState *dev = DEVICE(pci_dev);
> + PCIDeviceClass *pdc = PCI_DEVICE_GET_CLASS(pci_dev);
> E1000State *d = E1000(pci_dev);
> uint8_t *pci_conf;
> uint16_t checksum = 0;
> @@ -1523,7 +1773,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);
>
> 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 = 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: 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 = 0 ; i < EEPROM_CHECKSUM_REG; i++) {
Extra space
> + d->eeprom_data[i] = e1000_ich8_flash_template[i];
> + }
> + for (i = 0; i < 3; i++) {
> + d->eeprom_data[i] = (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] = pdc->device_id;
> + d->eeprom_data[13] = pdc->device_id;
> + checksum = 0;
> + 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;
> + /* 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 = 0;
> + hsfsts.hsf_status.fldesvalid = 1;
> + d->flash_reg[ICH_FLASH_HSFSTS] = hsfsts.regval;
> + d->rxbuf_edesc = 1;
> + }
> d->nic = qemu_new_nic(&net_e1000_info, &d->conf,
> object_get_typename(OBJECT(d)), dev->id, d);
>
> @@ -1551,7 +1830,8 @@ static int pci_e1000_init(PCIDevice *pci_dev)
>
> static void qdev_e1000_reset(DeviceState *dev)
> {
> - E1000State *d = E1000(dev);
> + PCIDevice *pci_dev = PCI_DEVICE(dev);
> + E1000State *d = E1000(pci_dev);
Just E1000(dev), they no longer depend on each other.
> e1000_reset(d);
> }
>
> @@ -1564,17 +1844,26 @@ static Property e1000_properties[] = {
> DEFINE_PROP_END_OF_LIST(),
> };
>
> +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 = DEVICE_CLASS(klass);
> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> + E1000Info *info = data;
const
>
> k->init = pci_e1000_init;
> k->exit = pci_e1000_uninit;
> k->romfile = "efi-e1000.rom";
> - k->vendor_id = PCI_VENDOR_ID_INTEL;
> - k->device_id = E1000_DEVID;
> - k->revision = 0x03;
> + k->vendor_id = info->vendor_id;
> + k->device_id = info->device_id;
> + k->revision = info->revision;
> k->class_id = PCI_CLASS_NETWORK_ETHERNET;
> set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
> dc->desc = "Intel Gigabit Ethernet";
> @@ -1583,16 +1872,59 @@ static void e1000_class_init(ObjectClass *klass, void *data)
> dc->props = e1000_properties;
> }
>
> -static const TypeInfo e1000_info = {
> +static E1000Info e1000_info_array[] = {
static const
> + {
> + .name = "e1000",
> + .vendor_id = PCI_VENDOR_ID_INTEL,
> + .device_id = E1000_DEVID,
> + .revision = 0x03,
> + },
> + {
> + .name = "82540EM",
> + .vendor_id = PCI_VENDOR_ID_INTEL,
> + .device_id = E1000_DEV_ID_82540EM,
> + .revision = 0x03,
> + },
> + {
> + .name = "82544GC",
> + .vendor_id = PCI_VENDOR_ID_INTEL,
> + .device_id = E1000_DEV_ID_82544GC_COPPER,
> + .revision = 0x03,
> + },
> + {
> + .name = "82545EM",
> + .vendor_id = PCI_VENDOR_ID_INTEL,
> + .device_id = E1000_DEV_ID_82545EM_COPPER,
> + .revision = 0x03,
> + },
> + {
> + .name = "82566DM",
> + .vendor_id = PCI_VENDOR_ID_INTEL,
> + .device_id = E1000_DEV_ID_ICH9_IGP_AMT,
> + .revision = 0x00,
> + }
> +};
> +
> +static const TypeInfo e1000_info_abstract = {
> .name = TYPE_E1000,
> .parent = TYPE_PCI_DEVICE,
> .instance_size = sizeof(E1000State),
> - .class_init = e1000_class_init,
> + .abstract = true,
> };
>
> static void e1000_register_types(void)
> {
> - type_register_static(&e1000_info);
> + int i;
> + type_register_static(&e1000_info_abstract);
> + for (i = 0; i < ARRAY_SIZE(e1000_info_array); i++) {
> + TypeInfo e1000_info = {
> + .name = e1000_info_array[i].name,
> + .parent = TYPE_E1000,
> + .class_data = e1000_info_array + i,
(void *)&e1000_info_array[i] would match above usage.
> + .class_init = e1000_class_init,
> + };
> + type_register(&e1000_info);
> + }
> }
>
> type_init(e1000_register_types)
[snip]
Otherwise looks good now!
Thanks,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
next prev parent reply other threads:[~2014-03-05 16:55 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-04 9:08 [Qemu-devel] [PATCH v2 1/1] e1000: add the ability to select among several specific types of e1000[e]; 82566DM emulation ; some pointers to documentations and details Romain Dolbeau
2014-03-05 14:14 ` [Qemu-devel] [PATCH v3] " Romain Dolbeau
2014-03-05 16:55 ` Andreas Färber [this message]
2014-03-06 9:01 ` Romain Dolbeau
2014-03-07 13:20 ` [Qemu-devel] [PATCH 0/2 V4] E1000 device selection & 82566DM support Romain Dolbeau
2014-03-07 13:20 ` [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 Romain Dolbeau
2014-03-10 12:56 ` Stefan Hajnoczi
2014-03-07 13:20 ` [Qemu-devel] [PATCH 2/2 V4] spaces around '<<' everywhere Romain Dolbeau
2014-03-07 13:33 ` Eric Blake
2014-03-10 12:57 ` [Qemu-devel] [PATCH 0/2 V4] E1000 device selection & 82566DM support Stefan Hajnoczi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=531756EC.9010307@suse.de \
--to=afaerber@suse.de \
--cc=qemu-devel@nongnu.org \
--cc=romain@dolbeau.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.