* [Qemu-devel] [PATCH 1/6] e1000: Cosmetic and alignment fixes
2015-10-18 7:53 [Qemu-devel] [PATCH 0/6] e1000: Various fixes and registers' implementation Leonid Bloch
@ 2015-10-18 7:53 ` Leonid Bloch
2015-10-18 7:53 ` [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers Leonid Bloch
` (5 subsequent siblings)
6 siblings, 0 replies; 21+ messages in thread
From: Leonid Bloch @ 2015-10-18 7:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Dmitry Fleytman, Jason Wang, Leonid Bloch, Shmulik Ladkani
This fixes some alignment and cosmetic issues. The changes are made
in order that the following patches in this series will look like
integral parts of the code surrounding them, while conforming better
to the coding style. Although some changes in unrelated areas are
also made.
Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
---
hw/net/e1000.c | 130 ++++++++++++++++++++++++++--------------------------
hw/net/e1000_regs.h | 2 +-
2 files changed, 67 insertions(+), 65 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 09c9e9d..6d57663 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -41,20 +41,20 @@
#ifdef E1000_DEBUG
enum {
- DEBUG_GENERAL, DEBUG_IO, DEBUG_MMIO, DEBUG_INTERRUPT,
- DEBUG_RX, DEBUG_TX, DEBUG_MDIC, DEBUG_EEPROM,
- DEBUG_UNKNOWN, DEBUG_TXSUM, DEBUG_TXERR, DEBUG_RXERR,
+ DEBUG_GENERAL, DEBUG_IO, DEBUG_MMIO, DEBUG_INTERRUPT,
+ DEBUG_RX, DEBUG_TX, DEBUG_MDIC, DEBUG_EEPROM,
+ DEBUG_UNKNOWN, DEBUG_TXSUM, DEBUG_TXERR, DEBUG_RXERR,
DEBUG_RXFILTER, DEBUG_PHY, DEBUG_NOTYET,
};
-#define DBGBIT(x) (1<<DEBUG_##x)
+#define DBGBIT(x) (1<<DEBUG_##x)
static int debugflags = DBGBIT(TXERR) | DBGBIT(GENERAL);
-#define DBGOUT(what, fmt, ...) do { \
+#define DBGOUT(what, fmt, ...) do { \
if (debugflags & DBGBIT(what)) \
fprintf(stderr, "e1000: " fmt, ## __VA_ARGS__); \
} while (0)
#else
-#define DBGOUT(what, fmt, ...) do {} while (0)
+#define DBGOUT(what, fmt, ...) do {} while (0)
#endif
#define IOPORT_SIZE 0x40
@@ -118,7 +118,7 @@ typedef struct E1000State_st {
} tx;
struct {
- uint32_t val_in; // shifted in from guest driver
+ uint32_t val_in; /* shifted in from guest driver */
uint16_t bitnum_in;
uint16_t bitnum_out;
uint16_t reading;
@@ -155,19 +155,19 @@ typedef struct E1000BaseClass {
#define E1000_DEVICE_GET_CLASS(obj) \
OBJECT_GET_CLASS(E1000BaseClass, (obj), TYPE_E1000_BASE)
-#define defreg(x) x = (E1000_##x>>2)
+#define defreg(x) x = (E1000_##x>>2)
enum {
- defreg(CTRL), defreg(EECD), defreg(EERD), defreg(GPRC),
- defreg(GPTC), defreg(ICR), defreg(ICS), defreg(IMC),
- defreg(IMS), defreg(LEDCTL), defreg(MANC), defreg(MDIC),
- defreg(MPC), defreg(PBA), defreg(RCTL), defreg(RDBAH),
- defreg(RDBAL), defreg(RDH), defreg(RDLEN), defreg(RDT),
- defreg(STATUS), defreg(SWSM), defreg(TCTL), defreg(TDBAH),
- defreg(TDBAL), defreg(TDH), defreg(TDLEN), defreg(TDT),
- defreg(TORH), defreg(TORL), defreg(TOTH), defreg(TOTL),
- defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC),
- defreg(RA), defreg(MTA), defreg(CRCERRS),defreg(VFTA),
- defreg(VET), defreg(RDTR), defreg(RADV), defreg(TADV),
+ defreg(CTRL), defreg(EECD), defreg(EERD), defreg(GPRC),
+ defreg(GPTC), defreg(ICR), defreg(ICS), defreg(IMC),
+ defreg(IMS), defreg(LEDCTL), defreg(MANC), defreg(MDIC),
+ defreg(MPC), defreg(PBA), defreg(RCTL), defreg(RDBAH),
+ defreg(RDBAL), defreg(RDH), defreg(RDLEN), defreg(RDT),
+ defreg(STATUS), defreg(SWSM), defreg(TCTL), defreg(TDBAH),
+ defreg(TDBAL), defreg(TDH), defreg(TDLEN), defreg(TDT),
+ defreg(TORH), defreg(TORL), defreg(TOTH), defreg(TOTL),
+ defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC),
+ defreg(RA), defreg(MTA), defreg(CRCERRS), defreg(VFTA),
+ defreg(VET), defreg(RDTR), defreg(RADV), defreg(TADV),
defreg(ITR),
};
@@ -226,12 +226,12 @@ enum { NPHYWRITEOPS = ARRAY_SIZE(phyreg_writeops) };
enum { PHY_R = 1, PHY_W = 2, PHY_RW = PHY_R | PHY_W };
static const char phy_regcap[0x20] = {
- [PHY_STATUS] = PHY_R, [M88E1000_EXT_PHY_SPEC_CTRL] = PHY_RW,
- [PHY_ID1] = PHY_R, [M88E1000_PHY_SPEC_CTRL] = PHY_RW,
- [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_STATUS] = PHY_R, [M88E1000_EXT_PHY_SPEC_CTRL] = PHY_RW,
+ [PHY_ID1] = PHY_R, [M88E1000_PHY_SPEC_CTRL] = PHY_RW,
+ [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_AUTONEG_EXP] = PHY_R,
};
@@ -510,17 +510,17 @@ set_eecd(E1000State *s, int index, uint32_t val)
s->eecd_state.old_eecd = val & (E1000_EECD_SK | E1000_EECD_CS |
E1000_EECD_DI|E1000_EECD_FWE_MASK|E1000_EECD_REQ);
- if (!(E1000_EECD_CS & val)) // CS inactive; nothing to do
- return;
- if (E1000_EECD_CS & (val ^ oldval)) { // CS rise edge; reset state
- s->eecd_state.val_in = 0;
- s->eecd_state.bitnum_in = 0;
- s->eecd_state.bitnum_out = 0;
- s->eecd_state.reading = 0;
+ if (!(E1000_EECD_CS & val)) /* CS inactive; nothing to do */
+ return;
+ if (E1000_EECD_CS & (val ^ oldval)) { /* CS rise edge; reset state */
+ s->eecd_state.val_in = 0;
+ s->eecd_state.bitnum_in = 0;
+ s->eecd_state.bitnum_out = 0;
+ s->eecd_state.reading = 0;
}
- if (!(E1000_EECD_SK & (val ^ oldval))) // no clock edge
+ if (!(E1000_EECD_SK & (val ^ oldval))) /* no clock edge */
return;
- if (!(E1000_EECD_SK & val)) { // falling edge
+ if (!(E1000_EECD_SK & val)) { /* falling edge */
s->eecd_state.bitnum_out++;
return;
}
@@ -621,11 +621,11 @@ xmit_seg(E1000State *s)
css = tp->ipcss;
DBGOUT(TXSUM, "frames %d size %d ipcss %d\n",
frames, tp->size, css);
- if (tp->ip) { // IPv4
+ if (tp->ip) { /* IPv4 */
stw_be_p(tp->data+css+2, tp->size - css);
stw_be_p(tp->data+css+4,
be16_to_cpup((uint16_t *)(tp->data+css+4))+frames);
- } else // IPv6
+ } else /* IPv6 */
stw_be_p(tp->data+css+4, tp->size - css);
css = tp->tucss;
len = tp->size - css;
@@ -634,8 +634,8 @@ xmit_seg(E1000State *s)
sofar = frames * tp->mss;
stl_be_p(tp->data+css+4, ldl_be_p(tp->data+css+4)+sofar); /* seq */
if (tp->paylen - sofar > tp->mss)
- tp->data[css + 13] &= ~9; // PSH, FIN
- } else // UDP
+ tp->data[css + 13] &= ~9; /* PSH, FIN */
+ } else /* UDP */
stw_be_p(tp->data+css+4, len);
if (tp->sum_needed & E1000_TXD_POPTS_TXSM) {
unsigned int phsum;
@@ -679,7 +679,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
struct e1000_tx *tp = &s->tx;
s->mit_ide |= (txd_lower & E1000_TXD_CMD_IDE);
- if (dtype == E1000_TXD_CMD_DEXT) { // context descriptor
+ if (dtype == E1000_TXD_CMD_DEXT) { /* context descriptor */
op = le32_to_cpu(xp->cmd_and_length);
tp->ipcss = xp->lower_setup.ip_fields.ipcss;
tp->ipcso = xp->lower_setup.ip_fields.ipcso;
@@ -694,7 +694,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
tp->tcp = (op & E1000_TXD_CMD_TCP) ? 1 : 0;
tp->tse = (op & E1000_TXD_CMD_TSE) ? 1 : 0;
tp->tso_frames = 0;
- if (tp->tucso == 0) { // this is probably wrong
+ if (tp->tucso == 0) { /* this is probably wrong */
DBGOUT(TXSUM, "TCP/UDP: cso 0!\n");
tp->tucso = tp->tucss + (tp->tcp ? 16 : 6);
}
@@ -718,7 +718,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
stw_be_p(tp->vlan_header + 2,
le16_to_cpu(dp->upper.fields.special));
}
-
+
addr = le64_to_cpu(dp->buffer_addr);
if (tp->tse && tp->cptse) {
msh = tp->hdr_len + tp->mss;
@@ -1206,20 +1206,21 @@ set_ims(E1000State *s, int index, uint32_t val)
set_ics(s, 0, 0);
}
-#define getreg(x) [x] = mac_readreg
+#define getreg(x) [x] = mac_readreg
static uint32_t (*macreg_readops[])(E1000State *, int) = {
- getreg(PBA), getreg(RCTL), getreg(TDH), getreg(TXDCTL),
- getreg(WUFC), getreg(TDT), getreg(CTRL), getreg(LEDCTL),
- getreg(MANC), getreg(MDIC), getreg(SWSM), getreg(STATUS),
- getreg(TORL), getreg(TOTL), getreg(IMS), getreg(TCTL),
- getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS),
- getreg(TDBAL), getreg(TDBAH), getreg(RDBAH), getreg(RDBAL),
- getreg(TDLEN), getreg(RDLEN), getreg(RDTR), getreg(RADV),
- getreg(TADV), getreg(ITR),
-
- [TOTH] = mac_read_clr8, [TORH] = mac_read_clr8, [GPRC] = mac_read_clr4,
- [GPTC] = mac_read_clr4, [TPR] = mac_read_clr4, [TPT] = mac_read_clr4,
- [ICR] = mac_icr_read, [EECD] = get_eecd, [EERD] = flash_eerd_read,
+ getreg(PBA), getreg(RCTL), getreg(TDH), getreg(TXDCTL),
+ getreg(WUFC), getreg(TDT), getreg(CTRL), getreg(LEDCTL),
+ getreg(MANC), getreg(MDIC), getreg(SWSM), getreg(STATUS),
+ getreg(TORL), getreg(TOTL), getreg(IMS), getreg(TCTL),
+ getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS),
+ getreg(TDBAL), getreg(TDBAH), getreg(RDBAH), getreg(RDBAL),
+ getreg(TDLEN), getreg(RDLEN), getreg(RDTR), getreg(RADV),
+ getreg(TADV), getreg(ITR),
+
+ [TOTH] = mac_read_clr8, [TORH] = mac_read_clr8,
+ [GPRC] = mac_read_clr4, [GPTC] = mac_read_clr4, [TPR] = mac_read_clr4,
+ [TPT] = mac_read_clr4,
+ [ICR] = mac_icr_read, [EECD] = get_eecd, [EERD] = flash_eerd_read,
[CRCERRS ... MPC] = &mac_readreg,
[RA ... RA+31] = &mac_readreg,
[MTA ... MTA+127] = &mac_readreg,
@@ -1227,17 +1228,18 @@ static uint32_t (*macreg_readops[])(E1000State *, int) = {
};
enum { NREADOPS = ARRAY_SIZE(macreg_readops) };
-#define putreg(x) [x] = mac_writereg
+#define putreg(x) [x] = mac_writereg
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),
- [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,
- [IMC] = set_imc, [IMS] = set_ims, [ICR] = set_icr,
- [EECD] = set_eecd, [RCTL] = set_rx_control, [CTRL] = set_ctrl,
- [RDTR] = set_16bit, [RADV] = set_16bit, [TADV] = set_16bit,
+ putreg(PBA), putreg(EERD), putreg(SWSM), putreg(WUFC),
+ putreg(TDBAL), putreg(TDBAH), putreg(TXDCTL), putreg(RDBAH),
+ putreg(RDBAL), putreg(LEDCTL), putreg(VET),
+
+ [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,
+ [IMC] = set_imc, [IMS] = set_ims, [ICR] = set_icr,
+ [EECD] = set_eecd, [RCTL] = set_rx_control, [CTRL] = set_ctrl,
+ [RDTR] = set_16bit, [RADV] = set_16bit, [TADV] = set_16bit,
[ITR] = set_16bit,
[RA ... RA+31] = &mac_writereg,
[MTA ... MTA+127] = &mac_writereg,
diff --git a/hw/net/e1000_regs.h b/hw/net/e1000_regs.h
index 60b96aa..afd81cc 100644
--- a/hw/net/e1000_regs.h
+++ b/hw/net/e1000_regs.h
@@ -158,7 +158,7 @@
#define E1000_PHY_CTRL 0x00F10 /* PHY Control Register in CSR */
#define FEXTNVM_SW_CONFIG 0x0001
#define E1000_PBA 0x01000 /* Packet Buffer Allocation - RW */
-#define E1000_PBS 0x01008 /* Packet Buffer Size */
+#define E1000_PBS 0x01008 /* Packet Buffer Size - RW */
#define E1000_EEMNGCTL 0x01010 /* MNG EEprom Control */
#define E1000_FLASH_UPDATES 1000
#define E1000_EEARBC 0x01024 /* EEPROM Auto Read Bus Control */
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers
2015-10-18 7:53 [Qemu-devel] [PATCH 0/6] e1000: Various fixes and registers' implementation Leonid Bloch
2015-10-18 7:53 ` [Qemu-devel] [PATCH 1/6] e1000: Cosmetic and alignment fixes Leonid Bloch
@ 2015-10-18 7:53 ` Leonid Bloch
2015-10-20 5:40 ` Jason Wang
2015-10-18 7:53 ` [Qemu-devel] [PATCH 3/6] e1000: Fixing the received/transmitted packets' counters Leonid Bloch
` (4 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Leonid Bloch @ 2015-10-18 7:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Dmitry Fleytman, Jason Wang, Leonid Bloch, Shmulik Ladkani
These registers appear in Intel's specs, but were not implemented.
These registers are now implemented trivially, i.e. they are initiated
with zero values, and if they are RW, they can be written or read by the
driver, or read only if they are R (essentially retaining their zero
values). For these registers no other procedures are performed.
The registers implemented here are:
Transmit:
RW: AIT
Management:
RW: WUC WUS IPAV* IP6AT* IP4AT* FFLT* WUPM* FFMT* FFVT*
Diagnostic:
RW: RDFH RDFT RDFHS RDFTS RDFPC PBM*
Statistic:
RW: FCRUC TDFH TDFT TDFHS TDFTS TDFPC
R: RNBC TSCTFC MGTPRC MGTPDC MGTPTC RFC RJC SCC ECOL
LATECOL MCC COLC DC TNCRS SEC CEXTERR RLEC XONRXC
XONTXC XOFFRXC XOFFTXC
Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
---
hw/net/e1000.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++---
hw/net/e1000_regs.h | 6 ++++++
2 files changed, 55 insertions(+), 3 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 6d57663..6f754ac 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -168,7 +168,17 @@ enum {
defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC),
defreg(RA), defreg(MTA), defreg(CRCERRS), defreg(VFTA),
defreg(VET), defreg(RDTR), defreg(RADV), defreg(TADV),
- defreg(ITR),
+ defreg(ITR), defreg(FCRUC), defreg(TDFH), defreg(TDFT),
+ defreg(TDFHS), defreg(TDFTS), defreg(TDFPC), defreg(RDFH),
+ defreg(RDFT), defreg(RDFHS), defreg(RDFTS), defreg(RDFPC),
+ defreg(IPAV), defreg(WUC), defreg(WUS), defreg(AIT),
+ defreg(IP6AT), defreg(IP4AT), defreg(FFLT), defreg(FFMT),
+ defreg(FFVT), defreg(WUPM), defreg(PBM), defreg(SCC),
+ defreg(ECOL), defreg(MCC), defreg(LATECOL), defreg(COLC),
+ defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
+ defreg(RLEC), defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
+ defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
+ defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
};
static void
@@ -1114,6 +1124,18 @@ mac_readreg(E1000State *s, int index)
}
static uint32_t
+mac_low11_read(E1000State *s, int index)
+{
+ return s->mac_reg[index] & 0x7ff;
+}
+
+static uint32_t
+mac_low13_read(E1000State *s, int index)
+{
+ return s->mac_reg[index] & 0x1fff;
+}
+
+static uint32_t
mac_icr_read(E1000State *s, int index)
{
uint32_t ret = s->mac_reg[ICR];
@@ -1215,16 +1237,31 @@ static uint32_t (*macreg_readops[])(E1000State *, int) = {
getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS),
getreg(TDBAL), getreg(TDBAH), getreg(RDBAH), getreg(RDBAL),
getreg(TDLEN), getreg(RDLEN), getreg(RDTR), getreg(RADV),
- getreg(TADV), getreg(ITR),
+ getreg(TADV), getreg(ITR), getreg(FCRUC), getreg(IPAV),
+ getreg(WUC), getreg(WUS), getreg(AIT), getreg(SCC),
+ getreg(ECOL), getreg(MCC), getreg(LATECOL), getreg(COLC),
+ getreg(DC), getreg(TNCRS), getreg(SEC), getreg(CEXTERR),
+ getreg(RLEC), getreg(XONRXC), getreg(XONTXC), getreg(XOFFRXC),
+ getreg(XOFFTXC), getreg(RFC), getreg(RJC), getreg(RNBC),
+ getreg(TSCTFC), getreg(MGTPRC), getreg(MGTPDC), getreg(MGTPTC),
[TOTH] = mac_read_clr8, [TORH] = mac_read_clr8,
[GPRC] = mac_read_clr4, [GPTC] = mac_read_clr4, [TPR] = mac_read_clr4,
[TPT] = mac_read_clr4,
[ICR] = mac_icr_read, [EECD] = get_eecd, [EERD] = flash_eerd_read,
+ [RDFH] = mac_low13_read, [RDFT] = mac_low13_read,
+ [RDFHS] = mac_low13_read, [RDFTS] = mac_low13_read, [RDFPC] = mac_low13_read,
+ [TDFH] = mac_low11_read, [TDFT] = mac_low11_read,
+ [TDFHS] = mac_low13_read, [TDFTS] = mac_low13_read, [TDFPC] = mac_low13_read,
[CRCERRS ... MPC] = &mac_readreg,
+ [IP6AT ... IP6AT+3] = &mac_readreg, [IP4AT ... IP4AT+6] = &mac_readreg,
+ [FFLT ... FFLT+6] = &mac_low11_read,
[RA ... RA+31] = &mac_readreg,
+ [WUPM ... WUPM+31] = &mac_readreg,
[MTA ... MTA+127] = &mac_readreg,
[VFTA ... VFTA+127] = &mac_readreg,
+ [FFMT ... FFMT+254] = &mac_readreg, [FFVT ... FFVT+254] = &mac_readreg,
+ [PBM ... PBM+16383] = &mac_readreg,
};
enum { NREADOPS = ARRAY_SIZE(macreg_readops) };
@@ -1232,7 +1269,11 @@ enum { NREADOPS = ARRAY_SIZE(macreg_readops) };
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(RDBAL), putreg(LEDCTL), putreg(VET), putreg(FCRUC),
+ putreg(TDFH), putreg(TDFT), putreg(TDFHS), putreg(TDFTS),
+ putreg(TDFPC), putreg(RDFH), putreg(RDFT), putreg(RDFHS),
+ putreg(RDFTS), putreg(RDFPC), putreg(IPAV), putreg(WUC),
+ putreg(WUS), putreg(AIT),
[TDLEN] = set_dlen, [RDLEN] = set_dlen, [TCTL] = set_tctl,
[TDT] = set_tctl, [MDIC] = set_mdic, [ICS] = set_ics,
@@ -1241,9 +1282,14 @@ static void (*macreg_writeops[])(E1000State *, int, uint32_t) = {
[EECD] = set_eecd, [RCTL] = set_rx_control, [CTRL] = set_ctrl,
[RDTR] = set_16bit, [RADV] = set_16bit, [TADV] = set_16bit,
[ITR] = set_16bit,
+ [IP6AT ... IP6AT+3] = &mac_writereg, [IP4AT ... IP4AT+6] = &mac_writereg,
+ [FFLT ... FFLT+6] = &mac_writereg,
[RA ... RA+31] = &mac_writereg,
+ [WUPM ... WUPM+31] = &mac_writereg,
[MTA ... MTA+127] = &mac_writereg,
[VFTA ... VFTA+127] = &mac_writereg,
+ [FFMT ... FFMT+254] = &mac_writereg, [FFVT ... FFVT+254] = &mac_writereg,
+ [PBM ... PBM+16383] = &mac_writereg,
};
enum { NWRITEOPS = ARRAY_SIZE(macreg_writeops) };
diff --git a/hw/net/e1000_regs.h b/hw/net/e1000_regs.h
index afd81cc..1c40244 100644
--- a/hw/net/e1000_regs.h
+++ b/hw/net/e1000_regs.h
@@ -158,6 +158,7 @@
#define E1000_PHY_CTRL 0x00F10 /* PHY Control Register in CSR */
#define FEXTNVM_SW_CONFIG 0x0001
#define E1000_PBA 0x01000 /* Packet Buffer Allocation - RW */
+#define E1000_PBM 0x10000 /* Packet Buffer Memory - RW */
#define E1000_PBS 0x01008 /* Packet Buffer Size - RW */
#define E1000_EEMNGCTL 0x01010 /* MNG EEprom Control */
#define E1000_FLASH_UPDATES 1000
@@ -191,6 +192,11 @@
#define E1000_RAID 0x02C08 /* Receive Ack Interrupt Delay - RW */
#define E1000_TXDMAC 0x03000 /* TX DMA Control - RW */
#define E1000_KABGTXD 0x03004 /* AFE Band Gap Transmit Ref Data */
+#define E1000_RDFH 0x02410 /* Receive Data FIFO Head Register - RW */
+#define E1000_RDFT 0x02418 /* Receive Data FIFO Tail Register - RW */
+#define E1000_RDFHS 0x02420 /* Receive Data FIFO Head Saved Register - RW */
+#define E1000_RDFTS 0x02428 /* Receive Data FIFO Tail Saved Register - RW */
+#define E1000_RDFPC 0x02430 /* Receive Data FIFO Packet Count - RW */
#define E1000_TDFH 0x03410 /* TX Data FIFO Head - RW */
#define E1000_TDFT 0x03418 /* TX Data FIFO Tail - RW */
#define E1000_TDFHS 0x03420 /* TX Data FIFO Head Saved - RW */
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers
2015-10-18 7:53 ` [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers Leonid Bloch
@ 2015-10-20 5:40 ` Jason Wang
2015-10-21 9:13 ` Leonid Bloch
0 siblings, 1 reply; 21+ messages in thread
From: Jason Wang @ 2015-10-20 5:40 UTC (permalink / raw)
To: Leonid Bloch, qemu-devel; +Cc: Dmitry Fleytman, Leonid Bloch, Shmulik Ladkani
On 10/18/2015 03:53 PM, Leonid Bloch wrote:
> These registers appear in Intel's specs, but were not implemented.
> These registers are now implemented trivially, i.e. they are initiated
> with zero values, and if they are RW, they can be written or read by the
> driver, or read only if they are R (essentially retaining their zero
> values). For these registers no other procedures are performed.
>
> The registers implemented here are:
>
> Transmit:
> RW: AIT
>
> Management:
> RW: WUC WUS IPAV* IP6AT* IP4AT* FFLT* WUPM* FFMT* FFVT*
My version of DSM (Revision) said WUS is read only.
>
> Diagnostic:
> RW: RDFH RDFT RDFHS RDFTS RDFPC PBM*
For those diagnostic register, isn't it better to warn the incomplete
emulation instead of trying to give all zero values silently? I suspect
this can make diagnostic software think the device is malfunction?
>
> Statistic:
> RW: FCRUC TDFH TDFT TDFHS TDFTS TDFPC
TDFH TDFT TDFHS TDFTS TDFPC should be Diagnostic?
> R: RNBC TSCTFC MGTPRC MGTPDC MGTPTC RFC RJC SCC ECOL
> LATECOL MCC COLC DC TNCRS SEC CEXTERR RLEC XONRXC
> XONTXC XOFFRXC XOFFTXC
>
> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
> ---
> hw/net/e1000.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++---
> hw/net/e1000_regs.h | 6 ++++++
> 2 files changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 6d57663..6f754ac 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -168,7 +168,17 @@ enum {
> defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC),
> defreg(RA), defreg(MTA), defreg(CRCERRS), defreg(VFTA),
> defreg(VET), defreg(RDTR), defreg(RADV), defreg(TADV),
> - defreg(ITR),
> + defreg(ITR), defreg(FCRUC), defreg(TDFH), defreg(TDFT),
> + defreg(TDFHS), defreg(TDFTS), defreg(TDFPC), defreg(RDFH),
> + defreg(RDFT), defreg(RDFHS), defreg(RDFTS), defreg(RDFPC),
> + defreg(IPAV), defreg(WUC), defreg(WUS), defreg(AIT),
> + defreg(IP6AT), defreg(IP4AT), defreg(FFLT), defreg(FFMT),
> + defreg(FFVT), defreg(WUPM), defreg(PBM), defreg(SCC),
> + defreg(ECOL), defreg(MCC), defreg(LATECOL), defreg(COLC),
> + defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
> + defreg(RLEC), defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
> + defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
> + defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
> };
>
> static void
> @@ -1114,6 +1124,18 @@ mac_readreg(E1000State *s, int index)
> }
>
> static uint32_t
> +mac_low11_read(E1000State *s, int index)
> +{
> + return s->mac_reg[index] & 0x7ff;
> +}
> +
> +static uint32_t
> +mac_low13_read(E1000State *s, int index)
> +{
> + return s->mac_reg[index] & 0x1fff;
> +}
> +
> +static uint32_t
> mac_icr_read(E1000State *s, int index)
> {
> uint32_t ret = s->mac_reg[ICR];
> @@ -1215,16 +1237,31 @@ static uint32_t (*macreg_readops[])(E1000State *, int) = {
> getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS),
> getreg(TDBAL), getreg(TDBAH), getreg(RDBAH), getreg(RDBAL),
> getreg(TDLEN), getreg(RDLEN), getreg(RDTR), getreg(RADV),
> - getreg(TADV), getreg(ITR),
> + getreg(TADV), getreg(ITR), getreg(FCRUC), getreg(IPAV),
> + getreg(WUC), getreg(WUS), getreg(AIT), getreg(SCC),
For AIT should we use low16_read() ?
And low4_read() for FFMT?
> + getreg(ECOL), getreg(MCC), getreg(LATECOL), getreg(COLC),
> + getreg(DC), getreg(TNCRS), getreg(SEC), getreg(CEXTERR),
> + getreg(RLEC), getreg(XONRXC), getreg(XONTXC), getreg(XOFFRXC),
> + getreg(XOFFTXC), getreg(RFC), getreg(RJC), getreg(RNBC),
> + getreg(TSCTFC), getreg(MGTPRC), getreg(MGTPDC), getreg(MGTPTC),
>
> [TOTH] = mac_read_clr8, [TORH] = mac_read_clr8,
> [GPRC] = mac_read_clr4, [GPTC] = mac_read_clr4, [TPR] = mac_read_clr4,
> [TPT] = mac_read_clr4,
> [ICR] = mac_icr_read, [EECD] = get_eecd, [EERD] = flash_eerd_read,
> + [RDFH] = mac_low13_read, [RDFT] = mac_low13_read,
> + [RDFHS] = mac_low13_read, [RDFTS] = mac_low13_read, [RDFPC] = mac_low13_read,
> + [TDFH] = mac_low11_read, [TDFT] = mac_low11_read,
> + [TDFHS] = mac_low13_read, [TDFTS] = mac_low13_read, [TDFPC] = mac_low13_read,
> [CRCERRS ... MPC] = &mac_readreg,
> + [IP6AT ... IP6AT+3] = &mac_readreg, [IP4AT ... IP4AT+6] = &mac_readreg,
> + [FFLT ... FFLT+6] = &mac_low11_read,
> [RA ... RA+31] = &mac_readreg,
> + [WUPM ... WUPM+31] = &mac_readreg,
> [MTA ... MTA+127] = &mac_readreg,
> [VFTA ... VFTA+127] = &mac_readreg,
> + [FFMT ... FFMT+254] = &mac_readreg, [FFVT ... FFVT+254] = &mac_readreg,
> + [PBM ... PBM+16383] = &mac_readreg,
> };
> enum { NREADOPS = ARRAY_SIZE(macreg_readops) };
>
> @@ -1232,7 +1269,11 @@ enum { NREADOPS = ARRAY_SIZE(macreg_readops) };
> 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(RDBAL), putreg(LEDCTL), putreg(VET), putreg(FCRUC),
> + putreg(TDFH), putreg(TDFT), putreg(TDFHS), putreg(TDFTS),
> + putreg(TDFPC), putreg(RDFH), putreg(RDFT), putreg(RDFHS),
> + putreg(RDFTS), putreg(RDFPC), putreg(IPAV), putreg(WUC),
> + putreg(WUS), putreg(AIT),
>
> [TDLEN] = set_dlen, [RDLEN] = set_dlen, [TCTL] = set_tctl,
> [TDT] = set_tctl, [MDIC] = set_mdic, [ICS] = set_ics,
> @@ -1241,9 +1282,14 @@ static void (*macreg_writeops[])(E1000State *, int, uint32_t) = {
> [EECD] = set_eecd, [RCTL] = set_rx_control, [CTRL] = set_ctrl,
> [RDTR] = set_16bit, [RADV] = set_16bit, [TADV] = set_16bit,
> [ITR] = set_16bit,
> + [IP6AT ... IP6AT+3] = &mac_writereg, [IP4AT ... IP4AT+6] = &mac_writereg,
> + [FFLT ... FFLT+6] = &mac_writereg,
> [RA ... RA+31] = &mac_writereg,
> + [WUPM ... WUPM+31] = &mac_writereg,
> [MTA ... MTA+127] = &mac_writereg,
> [VFTA ... VFTA+127] = &mac_writereg,
> + [FFMT ... FFMT+254] = &mac_writereg, [FFVT ... FFVT+254] = &mac_writereg,
> + [PBM ... PBM+16383] = &mac_writereg,
> };
>
> enum { NWRITEOPS = ARRAY_SIZE(macreg_writeops) };
> diff --git a/hw/net/e1000_regs.h b/hw/net/e1000_regs.h
> index afd81cc..1c40244 100644
> --- a/hw/net/e1000_regs.h
> +++ b/hw/net/e1000_regs.h
> @@ -158,6 +158,7 @@
> #define E1000_PHY_CTRL 0x00F10 /* PHY Control Register in CSR */
> #define FEXTNVM_SW_CONFIG 0x0001
> #define E1000_PBA 0x01000 /* Packet Buffer Allocation - RW */
> +#define E1000_PBM 0x10000 /* Packet Buffer Memory - RW */
> #define E1000_PBS 0x01008 /* Packet Buffer Size - RW */
> #define E1000_EEMNGCTL 0x01010 /* MNG EEprom Control */
> #define E1000_FLASH_UPDATES 1000
> @@ -191,6 +192,11 @@
> #define E1000_RAID 0x02C08 /* Receive Ack Interrupt Delay - RW */
> #define E1000_TXDMAC 0x03000 /* TX DMA Control - RW */
> #define E1000_KABGTXD 0x03004 /* AFE Band Gap Transmit Ref Data */
> +#define E1000_RDFH 0x02410 /* Receive Data FIFO Head Register - RW */
> +#define E1000_RDFT 0x02418 /* Receive Data FIFO Tail Register - RW */
> +#define E1000_RDFHS 0x02420 /* Receive Data FIFO Head Saved Register - RW */
> +#define E1000_RDFTS 0x02428 /* Receive Data FIFO Tail Saved Register - RW */
> +#define E1000_RDFPC 0x02430 /* Receive Data FIFO Packet Count - RW */
> #define E1000_TDFH 0x03410 /* TX Data FIFO Head - RW */
> #define E1000_TDFT 0x03418 /* TX Data FIFO Tail - RW */
> #define E1000_TDFHS 0x03420 /* TX Data FIFO Head Saved - RW */
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers
2015-10-20 5:40 ` Jason Wang
@ 2015-10-21 9:13 ` Leonid Bloch
2015-10-22 7:19 ` Jason Wang
0 siblings, 1 reply; 21+ messages in thread
From: Leonid Bloch @ 2015-10-21 9:13 UTC (permalink / raw)
To: Jason Wang; +Cc: Dmitry Fleytman, Leonid Bloch, qemu-devel, Shmulik Ladkani
Hi Jason, thanks for the review!
On Tue, Oct 20, 2015 at 8:40 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
>
> On 10/18/2015 03:53 PM, Leonid Bloch wrote:
> > These registers appear in Intel's specs, but were not implemented.
> > These registers are now implemented trivially, i.e. they are initiated
> > with zero values, and if they are RW, they can be written or read by the
> > driver, or read only if they are R (essentially retaining their zero
> > values). For these registers no other procedures are performed.
> >
> > The registers implemented here are:
> >
> > Transmit:
> > RW: AIT
> >
> > Management:
> > RW: WUC WUS IPAV* IP6AT* IP4AT* FFLT* WUPM* FFMT* FFVT*
>
> My version of DSM (Revision) said WUS is read only.
This seems to be a typo in the specs. We also have the specs where it
is said that WUS's read only, but exactly two lines below it - writing
to it is mentioned. Additionally, in the specs for newer Intel's
devices, where the offset and the functionality of WUS are exactly the
same, it is written that WUS is RW.
>
> >
> > Diagnostic:
> > RW: RDFH RDFT RDFHS RDFTS RDFPC PBM*
>
> For those diagnostic register, isn't it better to warn the incomplete
> emulation instead of trying to give all zero values silently? I suspect
> this can make diagnostic software think the device is malfunction?
That's a good point. What do you think about keeping the zero values,
but printing out a warning (via DBGOUT) on each read/write attempt?
>
> >
> > Statistic:
> > RW: FCRUC TDFH TDFT TDFHS TDFTS TDFPC
>
> TDFH TDFT TDFHS TDFTS TDFPC should be Diagnostic?
Yes, they are. Thanks, I'll reword.
>
> > R: RNBC TSCTFC MGTPRC MGTPDC MGTPTC RFC RJC SCC ECOL
> > LATECOL MCC COLC DC TNCRS SEC CEXTERR RLEC XONRXC
> > XONTXC XOFFRXC XOFFTXC
> >
> > Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
> > Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
> > ---
> > hw/net/e1000.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++---
> > hw/net/e1000_regs.h | 6 ++++++
> > 2 files changed, 55 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> > index 6d57663..6f754ac 100644
> > --- a/hw/net/e1000.c
> > +++ b/hw/net/e1000.c
> > @@ -168,7 +168,17 @@ enum {
> > defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC),
> > defreg(RA), defreg(MTA), defreg(CRCERRS), defreg(VFTA),
> > defreg(VET), defreg(RDTR), defreg(RADV), defreg(TADV),
> > - defreg(ITR),
> > + defreg(ITR), defreg(FCRUC), defreg(TDFH), defreg(TDFT),
> > + defreg(TDFHS), defreg(TDFTS), defreg(TDFPC), defreg(RDFH),
> > + defreg(RDFT), defreg(RDFHS), defreg(RDFTS), defreg(RDFPC),
> > + defreg(IPAV), defreg(WUC), defreg(WUS), defreg(AIT),
> > + defreg(IP6AT), defreg(IP4AT), defreg(FFLT), defreg(FFMT),
> > + defreg(FFVT), defreg(WUPM), defreg(PBM), defreg(SCC),
> > + defreg(ECOL), defreg(MCC), defreg(LATECOL), defreg(COLC),
> > + defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
> > + defreg(RLEC), defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
> > + defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
> > + defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
> > };
> >
> > static void
> > @@ -1114,6 +1124,18 @@ mac_readreg(E1000State *s, int index)
> > }
> >
> > static uint32_t
> > +mac_low11_read(E1000State *s, int index)
> > +{
> > + return s->mac_reg[index] & 0x7ff;
> > +}
> > +
> > +static uint32_t
> > +mac_low13_read(E1000State *s, int index)
> > +{
> > + return s->mac_reg[index] & 0x1fff;
> > +}
> > +
> > +static uint32_t
> > mac_icr_read(E1000State *s, int index)
> > {
> > uint32_t ret = s->mac_reg[ICR];
> > @@ -1215,16 +1237,31 @@ static uint32_t (*macreg_readops[])(E1000State *, int) = {
> > getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS),
> > getreg(TDBAL), getreg(TDBAH), getreg(RDBAH), getreg(RDBAL),
> > getreg(TDLEN), getreg(RDLEN), getreg(RDTR), getreg(RADV),
> > - getreg(TADV), getreg(ITR),
> > + getreg(TADV), getreg(ITR), getreg(FCRUC), getreg(IPAV),
> > + getreg(WUC), getreg(WUS), getreg(AIT), getreg(SCC),
>
> For AIT should we use low16_read() ?
Contrary to registers where lowXX_read() is used, for AIT the specs
say that the higher bits should be written with 0b, and not "read as
0b". That's my reasoning for that. What do you think?
>
> And low4_read() for FFMT?
Why? The specs say nothing about the reserved bits there...
>
> > + getreg(ECOL), getreg(MCC), getreg(LATECOL), getreg(COLC),
> > + getreg(DC), getreg(TNCRS), getreg(SEC), getreg(CEXTERR),
> > + getreg(RLEC), getreg(XONRXC), getreg(XONTXC), getreg(XOFFRXC),
> > + getreg(XOFFTXC), getreg(RFC), getreg(RJC), getreg(RNBC),
> > + getreg(TSCTFC), getreg(MGTPRC), getreg(MGTPDC), getreg(MGTPTC),
> >
> > [TOTH] = mac_read_clr8, [TORH] = mac_read_clr8,
> > [GPRC] = mac_read_clr4, [GPTC] = mac_read_clr4, [TPR] = mac_read_clr4,
> > [TPT] = mac_read_clr4,
> > [ICR] = mac_icr_read, [EECD] = get_eecd, [EERD] = flash_eerd_read,
> > + [RDFH] = mac_low13_read, [RDFT] = mac_low13_read,
> > + [RDFHS] = mac_low13_read, [RDFTS] = mac_low13_read, [RDFPC] = mac_low13_read,
> > + [TDFH] = mac_low11_read, [TDFT] = mac_low11_read,
> > + [TDFHS] = mac_low13_read, [TDFTS] = mac_low13_read, [TDFPC] = mac_low13_read,
> > [CRCERRS ... MPC] = &mac_readreg,
> > + [IP6AT ... IP6AT+3] = &mac_readreg, [IP4AT ... IP4AT+6] = &mac_readreg,
> > + [FFLT ... FFLT+6] = &mac_low11_read,
> > [RA ... RA+31] = &mac_readreg,
> > + [WUPM ... WUPM+31] = &mac_readreg,
> > [MTA ... MTA+127] = &mac_readreg,
> > [VFTA ... VFTA+127] = &mac_readreg,
> > + [FFMT ... FFMT+254] = &mac_readreg, [FFVT ... FFVT+254] = &mac_readreg,
> > + [PBM ... PBM+16383] = &mac_readreg,
> > };
> > enum { NREADOPS = ARRAY_SIZE(macreg_readops) };
> >
> > @@ -1232,7 +1269,11 @@ enum { NREADOPS = ARRAY_SIZE(macreg_readops) };
> > 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(RDBAL), putreg(LEDCTL), putreg(VET), putreg(FCRUC),
> > + putreg(TDFH), putreg(TDFT), putreg(TDFHS), putreg(TDFTS),
> > + putreg(TDFPC), putreg(RDFH), putreg(RDFT), putreg(RDFHS),
> > + putreg(RDFTS), putreg(RDFPC), putreg(IPAV), putreg(WUC),
> > + putreg(WUS), putreg(AIT),
> >
> > [TDLEN] = set_dlen, [RDLEN] = set_dlen, [TCTL] = set_tctl,
> > [TDT] = set_tctl, [MDIC] = set_mdic, [ICS] = set_ics,
> > @@ -1241,9 +1282,14 @@ static void (*macreg_writeops[])(E1000State *, int, uint32_t) = {
> > [EECD] = set_eecd, [RCTL] = set_rx_control, [CTRL] = set_ctrl,
> > [RDTR] = set_16bit, [RADV] = set_16bit, [TADV] = set_16bit,
> > [ITR] = set_16bit,
> > + [IP6AT ... IP6AT+3] = &mac_writereg, [IP4AT ... IP4AT+6] = &mac_writereg,
> > + [FFLT ... FFLT+6] = &mac_writereg,
> > [RA ... RA+31] = &mac_writereg,
> > + [WUPM ... WUPM+31] = &mac_writereg,
> > [MTA ... MTA+127] = &mac_writereg,
> > [VFTA ... VFTA+127] = &mac_writereg,
> > + [FFMT ... FFMT+254] = &mac_writereg, [FFVT ... FFVT+254] = &mac_writereg,
> > + [PBM ... PBM+16383] = &mac_writereg,
> > };
> >
> > enum { NWRITEOPS = ARRAY_SIZE(macreg_writeops) };
> > diff --git a/hw/net/e1000_regs.h b/hw/net/e1000_regs.h
> > index afd81cc..1c40244 100644
> > --- a/hw/net/e1000_regs.h
> > +++ b/hw/net/e1000_regs.h
> > @@ -158,6 +158,7 @@
> > #define E1000_PHY_CTRL 0x00F10 /* PHY Control Register in CSR */
> > #define FEXTNVM_SW_CONFIG 0x0001
> > #define E1000_PBA 0x01000 /* Packet Buffer Allocation - RW */
> > +#define E1000_PBM 0x10000 /* Packet Buffer Memory - RW */
> > #define E1000_PBS 0x01008 /* Packet Buffer Size - RW */
> > #define E1000_EEMNGCTL 0x01010 /* MNG EEprom Control */
> > #define E1000_FLASH_UPDATES 1000
> > @@ -191,6 +192,11 @@
> > #define E1000_RAID 0x02C08 /* Receive Ack Interrupt Delay - RW */
> > #define E1000_TXDMAC 0x03000 /* TX DMA Control - RW */
> > #define E1000_KABGTXD 0x03004 /* AFE Band Gap Transmit Ref Data */
> > +#define E1000_RDFH 0x02410 /* Receive Data FIFO Head Register - RW */
> > +#define E1000_RDFT 0x02418 /* Receive Data FIFO Tail Register - RW */
> > +#define E1000_RDFHS 0x02420 /* Receive Data FIFO Head Saved Register - RW */
> > +#define E1000_RDFTS 0x02428 /* Receive Data FIFO Tail Saved Register - RW */
> > +#define E1000_RDFPC 0x02430 /* Receive Data FIFO Packet Count - RW */
> > #define E1000_TDFH 0x03410 /* TX Data FIFO Head - RW */
> > #define E1000_TDFT 0x03418 /* TX Data FIFO Tail - RW */
> > #define E1000_TDFHS 0x03420 /* TX Data FIFO Head Saved - RW */
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers
2015-10-21 9:13 ` Leonid Bloch
@ 2015-10-22 7:19 ` Jason Wang
2015-10-22 14:05 ` Leonid Bloch
0 siblings, 1 reply; 21+ messages in thread
From: Jason Wang @ 2015-10-22 7:19 UTC (permalink / raw)
To: Leonid Bloch; +Cc: Dmitry Fleytman, Leonid Bloch, qemu-devel, Shmulik Ladkani
On 10/21/2015 05:13 PM, Leonid Bloch wrote:
> Hi Jason, thanks for the review!
>
> On Tue, Oct 20, 2015 at 8:40 AM, Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>> On 10/18/2015 03:53 PM, Leonid Bloch wrote:
>>> These registers appear in Intel's specs, but were not implemented.
>>> These registers are now implemented trivially, i.e. they are initiated
>>> with zero values, and if they are RW, they can be written or read by the
>>> driver, or read only if they are R (essentially retaining their zero
>>> values). For these registers no other procedures are performed.
>>>
>>> The registers implemented here are:
>>>
>>> Transmit:
>>> RW: AIT
>>>
>>> Management:
>>> RW: WUC WUS IPAV* IP6AT* IP4AT* FFLT* WUPM* FFMT* FFVT*
>> My version of DSM (Revision) said WUS is read only.
> This seems to be a typo in the specs. We also have the specs where it
> is said that WUS's read only, but exactly two lines below it - writing
> to it is mentioned. Additionally, in the specs for newer Intel's
> devices, where the offset and the functionality of WUS are exactly the
> same, it is written that WUS is RW.
Ok.
>>> Diagnostic:
>>> RW: RDFH RDFT RDFHS RDFTS RDFPC PBM*
>> For those diagnostic register, isn't it better to warn the incomplete
>> emulation instead of trying to give all zero values silently? I suspect
>> this can make diagnostic software think the device is malfunction?
> That's a good point. What do you think about keeping the zero values,
> but printing out a warning (via DBGOUT) on each read/write attempt?
This is fine for me.
>>> Statistic:
>>> RW: FCRUC TDFH TDFT TDFHS TDFTS TDFPC
>> TDFH TDFT TDFHS TDFTS TDFPC should be Diagnostic?
> Yes, they are. Thanks, I'll reword.
>>> R: RNBC TSCTFC MGTPRC MGTPDC MGTPTC RFC RJC SCC ECOL
>>> LATECOL MCC COLC DC TNCRS SEC CEXTERR RLEC XONRXC
>>> XONTXC XOFFRXC XOFFTXC
>>>
>>> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
>>> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
>>> ---
>>> hw/net/e1000.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++---
>>> hw/net/e1000_regs.h | 6 ++++++
>>> 2 files changed, 55 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>>> index 6d57663..6f754ac 100644
>>> --- a/hw/net/e1000.c
>>> +++ b/hw/net/e1000.c
>>> @@ -168,7 +168,17 @@ enum {
>>> defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC),
>>> defreg(RA), defreg(MTA), defreg(CRCERRS), defreg(VFTA),
>>> defreg(VET), defreg(RDTR), defreg(RADV), defreg(TADV),
>>> - defreg(ITR),
>>> + defreg(ITR), defreg(FCRUC), defreg(TDFH), defreg(TDFT),
>>> + defreg(TDFHS), defreg(TDFTS), defreg(TDFPC), defreg(RDFH),
>>> + defreg(RDFT), defreg(RDFHS), defreg(RDFTS), defreg(RDFPC),
>>> + defreg(IPAV), defreg(WUC), defreg(WUS), defreg(AIT),
>>> + defreg(IP6AT), defreg(IP4AT), defreg(FFLT), defreg(FFMT),
>>> + defreg(FFVT), defreg(WUPM), defreg(PBM), defreg(SCC),
>>> + defreg(ECOL), defreg(MCC), defreg(LATECOL), defreg(COLC),
>>> + defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
>>> + defreg(RLEC), defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
>>> + defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
>>> + defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
>>> };
>>>
>>> static void
>>> @@ -1114,6 +1124,18 @@ mac_readreg(E1000State *s, int index)
>>> }
>>>
>>> static uint32_t
>>> +mac_low11_read(E1000State *s, int index)
>>> +{
>>> + return s->mac_reg[index] & 0x7ff;
>>> +}
>>> +
>>> +static uint32_t
>>> +mac_low13_read(E1000State *s, int index)
>>> +{
>>> + return s->mac_reg[index] & 0x1fff;
>>> +}
>>> +
>>> +static uint32_t
>>> mac_icr_read(E1000State *s, int index)
>>> {
>>> uint32_t ret = s->mac_reg[ICR];
>>> @@ -1215,16 +1237,31 @@ static uint32_t (*macreg_readops[])(E1000State *, int) = {
>>> getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS),
>>> getreg(TDBAL), getreg(TDBAH), getreg(RDBAH), getreg(RDBAL),
>>> getreg(TDLEN), getreg(RDLEN), getreg(RDTR), getreg(RADV),
>>> - getreg(TADV), getreg(ITR),
>>> + getreg(TADV), getreg(ITR), getreg(FCRUC), getreg(IPAV),
>>> + getreg(WUC), getreg(WUS), getreg(AIT), getreg(SCC),
>> For AIT should we use low16_read() ?
> Contrary to registers where lowXX_read() is used, for AIT the specs
> say that the higher bits should be written with 0b, and not "read as
> 0b". That's my reasoning for that. What do you think?
I think it's better to test this behavior on real card.
>> And low4_read() for FFMT?
> Why? The specs say nothing about the reserved bits there...
If I read the spec (Revision 3.7 13.6.10) correctly, only low 4 bits
were used for byte mask.
[...]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers
2015-10-22 7:19 ` Jason Wang
@ 2015-10-22 14:05 ` Leonid Bloch
2015-10-23 3:10 ` Jason Wang
0 siblings, 1 reply; 21+ messages in thread
From: Leonid Bloch @ 2015-10-22 14:05 UTC (permalink / raw)
To: Jason Wang; +Cc: Dmitry Fleytman, Leonid Bloch, qemu-devel, Shmulik Ladkani
On Thu, Oct 22, 2015 at 10:19 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
>
> On 10/21/2015 05:13 PM, Leonid Bloch wrote:
> > Hi Jason, thanks for the review!
> >
> > On Tue, Oct 20, 2015 at 8:40 AM, Jason Wang <jasowang@redhat.com> wrote:
> >>
> >>
> >> On 10/18/2015 03:53 PM, Leonid Bloch wrote:
> >>> These registers appear in Intel's specs, but were not implemented.
> >>> These registers are now implemented trivially, i.e. they are initiated
> >>> with zero values, and if they are RW, they can be written or read by the
> >>> driver, or read only if they are R (essentially retaining their zero
> >>> values). For these registers no other procedures are performed.
> >>>
> >>> The registers implemented here are:
> >>>
> >>> Transmit:
> >>> RW: AIT
> >>>
> >>> Management:
> >>> RW: WUC WUS IPAV* IP6AT* IP4AT* FFLT* WUPM* FFMT* FFVT*
> >> My version of DSM (Revision) said WUS is read only.
> > This seems to be a typo in the specs. We also have the specs where it
> > is said that WUS's read only, but exactly two lines below it - writing
> > to it is mentioned. Additionally, in the specs for newer Intel's
> > devices, where the offset and the functionality of WUS are exactly the
> > same, it is written that WUS is RW.
>
> Ok.
>
> >>> Diagnostic:
> >>> RW: RDFH RDFT RDFHS RDFTS RDFPC PBM*
> >> For those diagnostic register, isn't it better to warn the incomplete
> >> emulation instead of trying to give all zero values silently? I suspect
> >> this can make diagnostic software think the device is malfunction?
> > That's a good point. What do you think about keeping the zero values,
> > but printing out a warning (via DBGOUT) on each read/write attempt?
>
> This is fine for me.
>
> >>> Statistic:
> >>> RW: FCRUC TDFH TDFT TDFHS TDFTS TDFPC
> >> TDFH TDFT TDFHS TDFTS TDFPC should be Diagnostic?
> > Yes, they are. Thanks, I'll reword.
> >>> R: RNBC TSCTFC MGTPRC MGTPDC MGTPTC RFC RJC SCC ECOL
> >>> LATECOL MCC COLC DC TNCRS SEC CEXTERR RLEC XONRXC
> >>> XONTXC XOFFRXC XOFFTXC
> >>>
> >>> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
> >>> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
> >>> ---
> >>> hw/net/e1000.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++---
> >>> hw/net/e1000_regs.h | 6 ++++++
> >>> 2 files changed, 55 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> >>> index 6d57663..6f754ac 100644
> >>> --- a/hw/net/e1000.c
> >>> +++ b/hw/net/e1000.c
> >>> @@ -168,7 +168,17 @@ enum {
> >>> defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC),
> >>> defreg(RA), defreg(MTA), defreg(CRCERRS), defreg(VFTA),
> >>> defreg(VET), defreg(RDTR), defreg(RADV), defreg(TADV),
> >>> - defreg(ITR),
> >>> + defreg(ITR), defreg(FCRUC), defreg(TDFH), defreg(TDFT),
> >>> + defreg(TDFHS), defreg(TDFTS), defreg(TDFPC), defreg(RDFH),
> >>> + defreg(RDFT), defreg(RDFHS), defreg(RDFTS), defreg(RDFPC),
> >>> + defreg(IPAV), defreg(WUC), defreg(WUS), defreg(AIT),
> >>> + defreg(IP6AT), defreg(IP4AT), defreg(FFLT), defreg(FFMT),
> >>> + defreg(FFVT), defreg(WUPM), defreg(PBM), defreg(SCC),
> >>> + defreg(ECOL), defreg(MCC), defreg(LATECOL), defreg(COLC),
> >>> + defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
> >>> + defreg(RLEC), defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
> >>> + defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
> >>> + defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
> >>> };
> >>>
> >>> static void
> >>> @@ -1114,6 +1124,18 @@ mac_readreg(E1000State *s, int index)
> >>> }
> >>>
> >>> static uint32_t
> >>> +mac_low11_read(E1000State *s, int index)
> >>> +{
> >>> + return s->mac_reg[index] & 0x7ff;
> >>> +}
> >>> +
> >>> +static uint32_t
> >>> +mac_low13_read(E1000State *s, int index)
> >>> +{
> >>> + return s->mac_reg[index] & 0x1fff;
> >>> +}
> >>> +
> >>> +static uint32_t
> >>> mac_icr_read(E1000State *s, int index)
> >>> {
> >>> uint32_t ret = s->mac_reg[ICR];
> >>> @@ -1215,16 +1237,31 @@ static uint32_t (*macreg_readops[])(E1000State *, int) = {
> >>> getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS),
> >>> getreg(TDBAL), getreg(TDBAH), getreg(RDBAH), getreg(RDBAL),
> >>> getreg(TDLEN), getreg(RDLEN), getreg(RDTR), getreg(RADV),
> >>> - getreg(TADV), getreg(ITR),
> >>> + getreg(TADV), getreg(ITR), getreg(FCRUC), getreg(IPAV),
> >>> + getreg(WUC), getreg(WUS), getreg(AIT), getreg(SCC),
> >> For AIT should we use low16_read() ?
> > Contrary to registers where lowXX_read() is used, for AIT the specs
> > say that the higher bits should be written with 0b, and not "read as
> > 0b". That's my reasoning for that. What do you think?
>
> I think it's better to test this behavior on real card.
What can be tested exactly? That the higher 16 bits read as 0 with a
real card? All the specs say is that these bits should be written with
0 (which the Linux driver, at least, indeed complies to). There is no
requirement anywhere for these bits to be read as 0, regardless of
their actual values.
>
> >> And low4_read() for FFMT?
> > Why? The specs say nothing about the reserved bits there...
>
> If I read the spec (Revision 3.7 13.6.10) correctly, only low 4 bits
> were used for byte mask.
Yes, only the lower 4 bits are used. But why the others need to be read as 0?
>
> [...]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers
2015-10-22 14:05 ` Leonid Bloch
@ 2015-10-23 3:10 ` Jason Wang
2015-10-25 19:39 ` Leonid Bloch
0 siblings, 1 reply; 21+ messages in thread
From: Jason Wang @ 2015-10-23 3:10 UTC (permalink / raw)
To: Leonid Bloch; +Cc: Dmitry Fleytman, Leonid Bloch, qemu-devel, Shmulik Ladkani
On 10/22/2015 10:05 PM, Leonid Bloch wrote:
> On Thu, Oct 22, 2015 at 10:19 AM, Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>> On 10/21/2015 05:13 PM, Leonid Bloch wrote:
>>> Hi Jason, thanks for the review!
>>>
>>> On Tue, Oct 20, 2015 at 8:40 AM, Jason Wang <jasowang@redhat.com> wrote:
>>>>
>>>> On 10/18/2015 03:53 PM, Leonid Bloch wrote:
>>>>> These registers appear in Intel's specs, but were not implemented.
>>>>> These registers are now implemented trivially, i.e. they are initiated
>>>>> with zero values, and if they are RW, they can be written or read by the
>>>>> driver, or read only if they are R (essentially retaining their zero
>>>>> values). For these registers no other procedures are performed.
>>>>>
>>>>> The registers implemented here are:
>>>>>
>>>>> Transmit:
>>>>> RW: AIT
>>>>>
>>>>> Management:
>>>>> RW: WUC WUS IPAV* IP6AT* IP4AT* FFLT* WUPM* FFMT* FFVT*
>>>> My version of DSM (Revision) said WUS is read only.
>>> This seems to be a typo in the specs. We also have the specs where it
>>> is said that WUS's read only, but exactly two lines below it - writing
>>> to it is mentioned. Additionally, in the specs for newer Intel's
>>> devices, where the offset and the functionality of WUS are exactly the
>>> same, it is written that WUS is RW.
>> Ok.
>>
>>>>> Diagnostic:
>>>>> RW: RDFH RDFT RDFHS RDFTS RDFPC PBM*
>>>> For those diagnostic register, isn't it better to warn the incomplete
>>>> emulation instead of trying to give all zero values silently? I suspect
>>>> this can make diagnostic software think the device is malfunction?
>>> That's a good point. What do you think about keeping the zero values,
>>> but printing out a warning (via DBGOUT) on each read/write attempt?
>> This is fine for me.
>>
>>>>> Statistic:
>>>>> RW: FCRUC TDFH TDFT TDFHS TDFTS TDFPC
>>>> TDFH TDFT TDFHS TDFTS TDFPC should be Diagnostic?
>>> Yes, they are. Thanks, I'll reword.
>>>>> R: RNBC TSCTFC MGTPRC MGTPDC MGTPTC RFC RJC SCC ECOL
>>>>> LATECOL MCC COLC DC TNCRS SEC CEXTERR RLEC XONRXC
>>>>> XONTXC XOFFRXC XOFFTXC
>>>>>
>>>>> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
>>>>> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
>>>>> ---
>>>>> hw/net/e1000.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++---
>>>>> hw/net/e1000_regs.h | 6 ++++++
>>>>> 2 files changed, 55 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>>>>> index 6d57663..6f754ac 100644
>>>>> --- a/hw/net/e1000.c
>>>>> +++ b/hw/net/e1000.c
>>>>> @@ -168,7 +168,17 @@ enum {
>>>>> defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC),
>>>>> defreg(RA), defreg(MTA), defreg(CRCERRS), defreg(VFTA),
>>>>> defreg(VET), defreg(RDTR), defreg(RADV), defreg(TADV),
>>>>> - defreg(ITR),
>>>>> + defreg(ITR), defreg(FCRUC), defreg(TDFH), defreg(TDFT),
>>>>> + defreg(TDFHS), defreg(TDFTS), defreg(TDFPC), defreg(RDFH),
>>>>> + defreg(RDFT), defreg(RDFHS), defreg(RDFTS), defreg(RDFPC),
>>>>> + defreg(IPAV), defreg(WUC), defreg(WUS), defreg(AIT),
>>>>> + defreg(IP6AT), defreg(IP4AT), defreg(FFLT), defreg(FFMT),
>>>>> + defreg(FFVT), defreg(WUPM), defreg(PBM), defreg(SCC),
>>>>> + defreg(ECOL), defreg(MCC), defreg(LATECOL), defreg(COLC),
>>>>> + defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
>>>>> + defreg(RLEC), defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
>>>>> + defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
>>>>> + defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
>>>>> };
>>>>>
>>>>> static void
>>>>> @@ -1114,6 +1124,18 @@ mac_readreg(E1000State *s, int index)
>>>>> }
>>>>>
>>>>> static uint32_t
>>>>> +mac_low11_read(E1000State *s, int index)
>>>>> +{
>>>>> + return s->mac_reg[index] & 0x7ff;
>>>>> +}
>>>>> +
>>>>> +static uint32_t
>>>>> +mac_low13_read(E1000State *s, int index)
>>>>> +{
>>>>> + return s->mac_reg[index] & 0x1fff;
>>>>> +}
>>>>> +
>>>>> +static uint32_t
>>>>> mac_icr_read(E1000State *s, int index)
>>>>> {
>>>>> uint32_t ret = s->mac_reg[ICR];
>>>>> @@ -1215,16 +1237,31 @@ static uint32_t (*macreg_readops[])(E1000State *, int) = {
>>>>> getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS),
>>>>> getreg(TDBAL), getreg(TDBAH), getreg(RDBAH), getreg(RDBAL),
>>>>> getreg(TDLEN), getreg(RDLEN), getreg(RDTR), getreg(RADV),
>>>>> - getreg(TADV), getreg(ITR),
>>>>> + getreg(TADV), getreg(ITR), getreg(FCRUC), getreg(IPAV),
>>>>> + getreg(WUC), getreg(WUS), getreg(AIT), getreg(SCC),
>>>> For AIT should we use low16_read() ?
>>> Contrary to registers where lowXX_read() is used, for AIT the specs
>>> say that the higher bits should be written with 0b, and not "read as
>>> 0b". That's my reasoning for that. What do you think?
>> I think it's better to test this behavior on real card.
> What can be tested exactly? That the higher 16 bits read as 0 with a
> real card?
Yes.
> All the specs say is that these bits should be written with
> 0 (which the Linux driver, at least, indeed complies to). There is no
> requirement anywhere for these bits to be read as 0, regardless of
> their actual values.
We should emulate the real card behavior even if it was buggy or
conflicts with spec. We've met several undocumented e1000 behavior in
the past, so we need to be careful to prevent us from debugging such
issue in the future. Leaving a known issue is much better in this case
if we don't know what real card will give us.
>>>> And low4_read() for FFMT?
>>> Why? The specs say nothing about the reserved bits there...
>> If I read the spec (Revision 3.7 13.6.10) correctly, only low 4 bits
>> were used for byte mask.
> Yes, only the lower 4 bits are used. But why the others need to be read as 0?
I'm not sure, so this needs to be tested on real card also.
>> [...]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers
2015-10-23 3:10 ` Jason Wang
@ 2015-10-25 19:39 ` Leonid Bloch
0 siblings, 0 replies; 21+ messages in thread
From: Leonid Bloch @ 2015-10-25 19:39 UTC (permalink / raw)
To: Jason Wang; +Cc: Dmitry Fleytman, Leonid Bloch, qemu-devel, Shmulik Ladkani
Dear Jason,
You were right both about the AIT and the FFMT! On a physical card,
the reserved bits in both of them read as zeros, even if they were set
to ones right before the reading.
V2 of the patches, with all the remarks addressed, is on the way.
Thanks again for your review.
Leonid.
___
On Fri, Oct 23, 2015 at 6:10 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 10/22/2015 10:05 PM, Leonid Bloch wrote:
>> On Thu, Oct 22, 2015 at 10:19 AM, Jason Wang <jasowang@redhat.com> wrote:
>>>
>>>
>>> On 10/21/2015 05:13 PM, Leonid Bloch wrote:
>>>> Hi Jason, thanks for the review!
>>>>
>>>> On Tue, Oct 20, 2015 at 8:40 AM, Jason Wang <jasowang@redhat.com> wrote:
>>>>>
>>>>> On 10/18/2015 03:53 PM, Leonid Bloch wrote:
>>>>>> These registers appear in Intel's specs, but were not implemented.
>>>>>> These registers are now implemented trivially, i.e. they are initiated
>>>>>> with zero values, and if they are RW, they can be written or read by the
>>>>>> driver, or read only if they are R (essentially retaining their zero
>>>>>> values). For these registers no other procedures are performed.
>>>>>>
>>>>>> The registers implemented here are:
>>>>>>
>>>>>> Transmit:
>>>>>> RW: AIT
>>>>>>
>>>>>> Management:
>>>>>> RW: WUC WUS IPAV* IP6AT* IP4AT* FFLT* WUPM* FFMT* FFVT*
>>>>> My version of DSM (Revision) said WUS is read only.
>>>> This seems to be a typo in the specs. We also have the specs where it
>>>> is said that WUS's read only, but exactly two lines below it - writing
>>>> to it is mentioned. Additionally, in the specs for newer Intel's
>>>> devices, where the offset and the functionality of WUS are exactly the
>>>> same, it is written that WUS is RW.
>>> Ok.
>>>
>>>>>> Diagnostic:
>>>>>> RW: RDFH RDFT RDFHS RDFTS RDFPC PBM*
>>>>> For those diagnostic register, isn't it better to warn the incomplete
>>>>> emulation instead of trying to give all zero values silently? I suspect
>>>>> this can make diagnostic software think the device is malfunction?
>>>> That's a good point. What do you think about keeping the zero values,
>>>> but printing out a warning (via DBGOUT) on each read/write attempt?
>>> This is fine for me.
>>>
>>>>>> Statistic:
>>>>>> RW: FCRUC TDFH TDFT TDFHS TDFTS TDFPC
>>>>> TDFH TDFT TDFHS TDFTS TDFPC should be Diagnostic?
>>>> Yes, they are. Thanks, I'll reword.
>>>>>> R: RNBC TSCTFC MGTPRC MGTPDC MGTPTC RFC RJC SCC ECOL
>>>>>> LATECOL MCC COLC DC TNCRS SEC CEXTERR RLEC XONRXC
>>>>>> XONTXC XOFFRXC XOFFTXC
>>>>>>
>>>>>> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
>>>>>> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
>>>>>> ---
>>>>>> hw/net/e1000.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++---
>>>>>> hw/net/e1000_regs.h | 6 ++++++
>>>>>> 2 files changed, 55 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>>>>>> index 6d57663..6f754ac 100644
>>>>>> --- a/hw/net/e1000.c
>>>>>> +++ b/hw/net/e1000.c
>>>>>> @@ -168,7 +168,17 @@ enum {
>>>>>> defreg(TPR), defreg(TPT), defreg(TXDCTL), defreg(WUFC),
>>>>>> defreg(RA), defreg(MTA), defreg(CRCERRS), defreg(VFTA),
>>>>>> defreg(VET), defreg(RDTR), defreg(RADV), defreg(TADV),
>>>>>> - defreg(ITR),
>>>>>> + defreg(ITR), defreg(FCRUC), defreg(TDFH), defreg(TDFT),
>>>>>> + defreg(TDFHS), defreg(TDFTS), defreg(TDFPC), defreg(RDFH),
>>>>>> + defreg(RDFT), defreg(RDFHS), defreg(RDFTS), defreg(RDFPC),
>>>>>> + defreg(IPAV), defreg(WUC), defreg(WUS), defreg(AIT),
>>>>>> + defreg(IP6AT), defreg(IP4AT), defreg(FFLT), defreg(FFMT),
>>>>>> + defreg(FFVT), defreg(WUPM), defreg(PBM), defreg(SCC),
>>>>>> + defreg(ECOL), defreg(MCC), defreg(LATECOL), defreg(COLC),
>>>>>> + defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
>>>>>> + defreg(RLEC), defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
>>>>>> + defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
>>>>>> + defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
>>>>>> };
>>>>>>
>>>>>> static void
>>>>>> @@ -1114,6 +1124,18 @@ mac_readreg(E1000State *s, int index)
>>>>>> }
>>>>>>
>>>>>> static uint32_t
>>>>>> +mac_low11_read(E1000State *s, int index)
>>>>>> +{
>>>>>> + return s->mac_reg[index] & 0x7ff;
>>>>>> +}
>>>>>> +
>>>>>> +static uint32_t
>>>>>> +mac_low13_read(E1000State *s, int index)
>>>>>> +{
>>>>>> + return s->mac_reg[index] & 0x1fff;
>>>>>> +}
>>>>>> +
>>>>>> +static uint32_t
>>>>>> mac_icr_read(E1000State *s, int index)
>>>>>> {
>>>>>> uint32_t ret = s->mac_reg[ICR];
>>>>>> @@ -1215,16 +1237,31 @@ static uint32_t (*macreg_readops[])(E1000State *, int) = {
>>>>>> getreg(RDH), getreg(RDT), getreg(VET), getreg(ICS),
>>>>>> getreg(TDBAL), getreg(TDBAH), getreg(RDBAH), getreg(RDBAL),
>>>>>> getreg(TDLEN), getreg(RDLEN), getreg(RDTR), getreg(RADV),
>>>>>> - getreg(TADV), getreg(ITR),
>>>>>> + getreg(TADV), getreg(ITR), getreg(FCRUC), getreg(IPAV),
>>>>>> + getreg(WUC), getreg(WUS), getreg(AIT), getreg(SCC),
>>>>> For AIT should we use low16_read() ?
>>>> Contrary to registers where lowXX_read() is used, for AIT the specs
>>>> say that the higher bits should be written with 0b, and not "read as
>>>> 0b". That's my reasoning for that. What do you think?
>>> I think it's better to test this behavior on real card.
>> What can be tested exactly? That the higher 16 bits read as 0 with a
>> real card?
>
> Yes.
>
>> All the specs say is that these bits should be written with
>> 0 (which the Linux driver, at least, indeed complies to). There is no
>> requirement anywhere for these bits to be read as 0, regardless of
>> their actual values.
>
> We should emulate the real card behavior even if it was buggy or
> conflicts with spec. We've met several undocumented e1000 behavior in
> the past, so we need to be careful to prevent us from debugging such
> issue in the future. Leaving a known issue is much better in this case
> if we don't know what real card will give us.
>
>>>>> And low4_read() for FFMT?
>>>> Why? The specs say nothing about the reserved bits there...
>>> If I read the spec (Revision 3.7 13.6.10) correctly, only low 4 bits
>>> were used for byte mask.
>> Yes, only the lower 4 bits are used. But why the others need to be read as 0?
>
> I'm not sure, so this needs to be tested on real card also.
>
>>> [...]
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 3/6] e1000: Fixing the received/transmitted packets' counters
2015-10-18 7:53 [Qemu-devel] [PATCH 0/6] e1000: Various fixes and registers' implementation Leonid Bloch
2015-10-18 7:53 ` [Qemu-devel] [PATCH 1/6] e1000: Cosmetic and alignment fixes Leonid Bloch
2015-10-18 7:53 ` [Qemu-devel] [PATCH 2/6] e1000: Trivial implementation of various MAC registers Leonid Bloch
@ 2015-10-18 7:53 ` Leonid Bloch
2015-10-18 7:53 ` [Qemu-devel] [PATCH 4/6] e1000: Fixing the received/transmitted octets' counters Leonid Bloch
` (3 subsequent siblings)
6 siblings, 0 replies; 21+ messages in thread
From: Leonid Bloch @ 2015-10-18 7:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Dmitry Fleytman, Jason Wang, Leonid Bloch, Shmulik Ladkani
According to Intel's specs, these counters (as all the other Statistic
registers) stick at 0xffffffff when the maximum value is reached.
Previously, they would reset after the max. value.
Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
---
hw/net/e1000.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 6f754ac..5530285 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -575,6 +575,14 @@ putsum(uint8_t *data, uint32_t n, uint32_t sloc, uint32_t css, uint32_t cse)
}
}
+static inline void
+inc_reg_if_not_full(E1000State *s, int index)
+{
+ if (s->mac_reg[index] != 0xffffffff) {
+ s->mac_reg[index]++;
+ }
+}
+
static inline int
vlan_enabled(E1000State *s)
{
@@ -669,8 +677,8 @@ xmit_seg(E1000State *s)
e1000_send_packet(s, tp->vlan, tp->size + 4);
} else
e1000_send_packet(s, tp->data, tp->size);
- s->mac_reg[TPT]++;
- s->mac_reg[GPTC]++;
+ inc_reg_if_not_full(s, TPT);
+ s->mac_reg[GPTC] = s->mac_reg[TPT];
n = s->mac_reg[TOTL];
if ((s->mac_reg[TOTL] += s->tx.size) < n)
s->mac_reg[TOTH]++;
@@ -1083,8 +1091,8 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
}
} while (desc_offset < total_size);
- s->mac_reg[GPRC]++;
- s->mac_reg[TPR]++;
+ inc_reg_if_not_full(s, TPR);
+ s->mac_reg[GPRC] = s->mac_reg[TPR];
/* TOR - Total Octets Received:
* This register includes bytes received in a packet from the <Destination
* Address> field through the <CRC> field, inclusively.
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH 4/6] e1000: Fixing the received/transmitted octets' counters
2015-10-18 7:53 [Qemu-devel] [PATCH 0/6] e1000: Various fixes and registers' implementation Leonid Bloch
` (2 preceding siblings ...)
2015-10-18 7:53 ` [Qemu-devel] [PATCH 3/6] e1000: Fixing the received/transmitted packets' counters Leonid Bloch
@ 2015-10-18 7:53 ` Leonid Bloch
2015-10-20 6:16 ` Jason Wang
2015-10-18 7:53 ` [Qemu-devel] [PATCH 5/6] e1000: Fixing the packet address filtering procedure Leonid Bloch
` (2 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Leonid Bloch @ 2015-10-18 7:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Dmitry Fleytman, Jason Wang, Leonid Bloch, Shmulik Ladkani
Previously, the lower parts of these counters (TORL, TOTL) were
resetting after reaching their maximal values, and since the continuation
of counting in the higher parts (TORH, TOTH) was triggered by an
overflow event of the lower parts, the count was not correct.
Additionally, TORH and TOTH were counting the corresponding frames, and
not the octets, as they supposed to do.
Additionally, these 64-bit registers did not stick at their maximal
values when (and if) they reached them.
This fix resolves all the issues mentioned above, and makes the octet
counters behave according to Intel's specs.
Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
---
hw/net/e1000.c | 34 ++++++++++++++++++++++++++--------
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 5530285..7f977b6 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -583,6 +583,28 @@ inc_reg_if_not_full(E1000State *s, int index)
}
}
+static void
+grow_8reg_if_not_full(E1000State *s, int index, int size)
+{
+ uint32_t lo = s->mac_reg[index];
+ uint32_t hi = s->mac_reg[index+1];
+
+ if (lo == 0xffffffff) {
+ if ((hi += size) > s->mac_reg[index+1]) {
+ s->mac_reg[index+1] = hi;
+ } else if (s->mac_reg[index+1] != 0xffffffff) {
+ s->mac_reg[index+1] = 0xffffffff;
+ }
+ } else {
+ if (((lo += size) < s->mac_reg[index])
+ && (s->mac_reg[index] = 0xffffffff)) { /* setting low to full */
+ s->mac_reg[index+1] += ++lo;
+ } else {
+ s->mac_reg[index] = lo;
+ }
+ }
+}
+
static inline int
vlan_enabled(E1000State *s)
{
@@ -632,7 +654,7 @@ static void
xmit_seg(E1000State *s)
{
uint16_t len, *sp;
- unsigned int frames = s->tx.tso_frames, css, sofar, n;
+ unsigned int frames = s->tx.tso_frames, css, sofar;
struct e1000_tx *tp = &s->tx;
if (tp->tse && tp->cptse) {
@@ -678,10 +700,8 @@ xmit_seg(E1000State *s)
} else
e1000_send_packet(s, tp->data, tp->size);
inc_reg_if_not_full(s, TPT);
+ grow_8reg_if_not_full(s, TOTL, s->tx.size);
s->mac_reg[GPTC] = s->mac_reg[TPT];
- n = s->mac_reg[TOTL];
- if ((s->mac_reg[TOTL] += s->tx.size) < n)
- s->mac_reg[TOTH]++;
}
static void
@@ -1096,11 +1116,9 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
/* TOR - Total Octets Received:
* This register includes bytes received in a packet from the <Destination
* Address> field through the <CRC> field, inclusively.
+ * Always include FCS length (4) in size.
*/
- n = s->mac_reg[TORL] + size + /* Always include FCS length. */ 4;
- if (n < s->mac_reg[TORL])
- s->mac_reg[TORH]++;
- s->mac_reg[TORL] = n;
+ grow_8reg_if_not_full(s, TORL, size+4);
n = E1000_ICS_RXT0;
if ((rdt = s->mac_reg[RDT]) < s->mac_reg[RDH])
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH 4/6] e1000: Fixing the received/transmitted octets' counters
2015-10-18 7:53 ` [Qemu-devel] [PATCH 4/6] e1000: Fixing the received/transmitted octets' counters Leonid Bloch
@ 2015-10-20 6:16 ` Jason Wang
2015-10-21 12:20 ` Leonid Bloch
0 siblings, 1 reply; 21+ messages in thread
From: Jason Wang @ 2015-10-20 6:16 UTC (permalink / raw)
To: Leonid Bloch, qemu-devel; +Cc: Dmitry Fleytman, Leonid Bloch, Shmulik Ladkani
On 10/18/2015 03:53 PM, Leonid Bloch wrote:
> Previously, the lower parts of these counters (TORL, TOTL) were
> resetting after reaching their maximal values, and since the continuation
> of counting in the higher parts (TORH, TOTH) was triggered by an
> overflow event of the lower parts, the count was not correct.
>
> Additionally, TORH and TOTH were counting the corresponding frames, and
> not the octets, as they supposed to do.
>
> Additionally, these 64-bit registers did not stick at their maximal
> values when (and if) they reached them.
>
> This fix resolves all the issues mentioned above, and makes the octet
> counters behave according to Intel's specs.
>
> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
> ---
> hw/net/e1000.c | 34 ++++++++++++++++++++++++++--------
> 1 file changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 5530285..7f977b6 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -583,6 +583,28 @@ inc_reg_if_not_full(E1000State *s, int index)
> }
> }
>
> +static void
> +grow_8reg_if_not_full(E1000State *s, int index, int size)
> +{
> + uint32_t lo = s->mac_reg[index];
> + uint32_t hi = s->mac_reg[index+1];
> +
> + if (lo == 0xffffffff) {
> + if ((hi += size) > s->mac_reg[index+1]) {
> + s->mac_reg[index+1] = hi;
> + } else if (s->mac_reg[index+1] != 0xffffffff) {
> + s->mac_reg[index+1] = 0xffffffff;
> + }
> + } else {
> + if (((lo += size) < s->mac_reg[index])
> + && (s->mac_reg[index] = 0xffffffff)) { /* setting low to full */
> + s->mac_reg[index+1] += ++lo;
> + } else {
> + s->mac_reg[index] = lo;
> + }
> + }
> +}
How about something easier:
uint64_t sum = s->mac_reg[index] | (uint64_t)s->mac_reg[index+1] <<32;
if (sum + size < sum) {
sum = 0xffffffffffffffff;
} else {
sum += size;
}
s->max_reg[index] = sum;
s->max_reg[index+1] = sum >> 32;
> +
> static inline int
> vlan_enabled(E1000State *s)
> {
> @@ -632,7 +654,7 @@ static void
> xmit_seg(E1000State *s)
> {
> uint16_t len, *sp;
> - unsigned int frames = s->tx.tso_frames, css, sofar, n;
> + unsigned int frames = s->tx.tso_frames, css, sofar;
> struct e1000_tx *tp = &s->tx;
>
> if (tp->tse && tp->cptse) {
> @@ -678,10 +700,8 @@ xmit_seg(E1000State *s)
> } else
> e1000_send_packet(s, tp->data, tp->size);
> inc_reg_if_not_full(s, TPT);
> + grow_8reg_if_not_full(s, TOTL, s->tx.size);
> s->mac_reg[GPTC] = s->mac_reg[TPT];
> - n = s->mac_reg[TOTL];
> - if ((s->mac_reg[TOTL] += s->tx.size) < n)
> - s->mac_reg[TOTH]++;
> }
>
> static void
> @@ -1096,11 +1116,9 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
> /* TOR - Total Octets Received:
> * This register includes bytes received in a packet from the <Destination
> * Address> field through the <CRC> field, inclusively.
> + * Always include FCS length (4) in size.
> */
> - n = s->mac_reg[TORL] + size + /* Always include FCS length. */ 4;
> - if (n < s->mac_reg[TORL])
> - s->mac_reg[TORH]++;
> - s->mac_reg[TORL] = n;
> + grow_8reg_if_not_full(s, TORL, size+4);
>
> n = E1000_ICS_RXT0;
> if ((rdt = s->mac_reg[RDT]) < s->mac_reg[RDH])
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH 4/6] e1000: Fixing the received/transmitted octets' counters
2015-10-20 6:16 ` Jason Wang
@ 2015-10-21 12:20 ` Leonid Bloch
2015-10-22 7:20 ` Jason Wang
0 siblings, 1 reply; 21+ messages in thread
From: Leonid Bloch @ 2015-10-21 12:20 UTC (permalink / raw)
To: Jason Wang; +Cc: Dmitry Fleytman, Leonid Bloch, qemu-devel, Shmulik Ladkani
On Tue, Oct 20, 2015 at 9:16 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 10/18/2015 03:53 PM, Leonid Bloch wrote:
>> Previously, the lower parts of these counters (TORL, TOTL) were
>> resetting after reaching their maximal values, and since the continuation
>> of counting in the higher parts (TORH, TOTH) was triggered by an
>> overflow event of the lower parts, the count was not correct.
>>
>> Additionally, TORH and TOTH were counting the corresponding frames, and
>> not the octets, as they supposed to do.
>>
>> Additionally, these 64-bit registers did not stick at their maximal
>> values when (and if) they reached them.
>>
>> This fix resolves all the issues mentioned above, and makes the octet
>> counters behave according to Intel's specs.
>>
>> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
>> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
>> ---
>> hw/net/e1000.c | 34 ++++++++++++++++++++++++++--------
>> 1 file changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>> index 5530285..7f977b6 100644
>> --- a/hw/net/e1000.c
>> +++ b/hw/net/e1000.c
>> @@ -583,6 +583,28 @@ inc_reg_if_not_full(E1000State *s, int index)
>> }
>> }
>>
>> +static void
>> +grow_8reg_if_not_full(E1000State *s, int index, int size)
>> +{
>> + uint32_t lo = s->mac_reg[index];
>> + uint32_t hi = s->mac_reg[index+1];
>> +
>> + if (lo == 0xffffffff) {
>> + if ((hi += size) > s->mac_reg[index+1]) {
>> + s->mac_reg[index+1] = hi;
>> + } else if (s->mac_reg[index+1] != 0xffffffff) {
>> + s->mac_reg[index+1] = 0xffffffff;
>> + }
>> + } else {
>> + if (((lo += size) < s->mac_reg[index])
>> + && (s->mac_reg[index] = 0xffffffff)) { /* setting low to full */
>> + s->mac_reg[index+1] += ++lo;
>> + } else {
>> + s->mac_reg[index] = lo;
>> + }
>> + }
>> +}
>
> How about something easier:
>
> uint64_t sum = s->mac_reg[index] | (uint64_t)s->mac_reg[index+1] <<32;
> if (sum + size < sum) {
> sum = 0xffffffffffffffff;
> } else {
> sum += size;
> }
> s->max_reg[index] = sum;
> s->max_reg[index+1] = sum >> 32;
Yes, that is better! Few small changes:
uint64_t sum = s->mac_reg[index] | (uint64_t)s->mac_reg[index+1] << 32;
if (sum + size < sum) {
sum = ~0;
} else {
sum += size;
}
s->mac_reg[index] = sum;
s->mac_reg[index+1] = sum >> 32;
>
>> +
>> static inline int
>> vlan_enabled(E1000State *s)
>> {
>> @@ -632,7 +654,7 @@ static void
>> xmit_seg(E1000State *s)
>> {
>> uint16_t len, *sp;
>> - unsigned int frames = s->tx.tso_frames, css, sofar, n;
>> + unsigned int frames = s->tx.tso_frames, css, sofar;
>> struct e1000_tx *tp = &s->tx;
>>
>> if (tp->tse && tp->cptse) {
>> @@ -678,10 +700,8 @@ xmit_seg(E1000State *s)
>> } else
>> e1000_send_packet(s, tp->data, tp->size);
>> inc_reg_if_not_full(s, TPT);
>> + grow_8reg_if_not_full(s, TOTL, s->tx.size);
>> s->mac_reg[GPTC] = s->mac_reg[TPT];
>> - n = s->mac_reg[TOTL];
>> - if ((s->mac_reg[TOTL] += s->tx.size) < n)
>> - s->mac_reg[TOTH]++;
>> }
>>
>> static void
>> @@ -1096,11 +1116,9 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
>> /* TOR - Total Octets Received:
>> * This register includes bytes received in a packet from the <Destination
>> * Address> field through the <CRC> field, inclusively.
>> + * Always include FCS length (4) in size.
>> */
>> - n = s->mac_reg[TORL] + size + /* Always include FCS length. */ 4;
>> - if (n < s->mac_reg[TORL])
>> - s->mac_reg[TORH]++;
>> - s->mac_reg[TORL] = n;
>> + grow_8reg_if_not_full(s, TORL, size+4);
>>
>> n = E1000_ICS_RXT0;
>> if ((rdt = s->mac_reg[RDT]) < s->mac_reg[RDH])
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH 4/6] e1000: Fixing the received/transmitted octets' counters
2015-10-21 12:20 ` Leonid Bloch
@ 2015-10-22 7:20 ` Jason Wang
0 siblings, 0 replies; 21+ messages in thread
From: Jason Wang @ 2015-10-22 7:20 UTC (permalink / raw)
To: Leonid Bloch; +Cc: Dmitry Fleytman, Leonid Bloch, qemu-devel, Shmulik Ladkani
On 10/21/2015 08:20 PM, Leonid Bloch wrote:
> On Tue, Oct 20, 2015 at 9:16 AM, Jason Wang <jasowang@redhat.com> wrote:
>> >
>> >
>> > On 10/18/2015 03:53 PM, Leonid Bloch wrote:
>>> >> Previously, the lower parts of these counters (TORL, TOTL) were
>>> >> resetting after reaching their maximal values, and since the continuation
>>> >> of counting in the higher parts (TORH, TOTH) was triggered by an
>>> >> overflow event of the lower parts, the count was not correct.
>>> >>
>>> >> Additionally, TORH and TOTH were counting the corresponding frames, and
>>> >> not the octets, as they supposed to do.
>>> >>
>>> >> Additionally, these 64-bit registers did not stick at their maximal
>>> >> values when (and if) they reached them.
>>> >>
>>> >> This fix resolves all the issues mentioned above, and makes the octet
>>> >> counters behave according to Intel's specs.
>>> >>
>>> >> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
>>> >> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
>>> >> ---
>>> >> hw/net/e1000.c | 34 ++++++++++++++++++++++++++--------
>>> >> 1 file changed, 26 insertions(+), 8 deletions(-)
>>> >>
>>> >> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>>> >> index 5530285..7f977b6 100644
>>> >> --- a/hw/net/e1000.c
>>> >> +++ b/hw/net/e1000.c
>>> >> @@ -583,6 +583,28 @@ inc_reg_if_not_full(E1000State *s, int index)
>>> >> }
>>> >> }
>>> >>
>>> >> +static void
>>> >> +grow_8reg_if_not_full(E1000State *s, int index, int size)
>>> >> +{
>>> >> + uint32_t lo = s->mac_reg[index];
>>> >> + uint32_t hi = s->mac_reg[index+1];
>>> >> +
>>> >> + if (lo == 0xffffffff) {
>>> >> + if ((hi += size) > s->mac_reg[index+1]) {
>>> >> + s->mac_reg[index+1] = hi;
>>> >> + } else if (s->mac_reg[index+1] != 0xffffffff) {
>>> >> + s->mac_reg[index+1] = 0xffffffff;
>>> >> + }
>>> >> + } else {
>>> >> + if (((lo += size) < s->mac_reg[index])
>>> >> + && (s->mac_reg[index] = 0xffffffff)) { /* setting low to full */
>>> >> + s->mac_reg[index+1] += ++lo;
>>> >> + } else {
>>> >> + s->mac_reg[index] = lo;
>>> >> + }
>>> >> + }
>>> >> +}
>> >
>> > How about something easier:
>> >
>> > uint64_t sum = s->mac_reg[index] | (uint64_t)s->mac_reg[index+1] <<32;
>> > if (sum + size < sum) {
>> > sum = 0xffffffffffffffff;
>> > } else {
>> > sum += size;
>> > }
>> > s->max_reg[index] = sum;
>> > s->max_reg[index+1] = sum >> 32;
> Yes, that is better! Few small changes:
>
> uint64_t sum = s->mac_reg[index] | (uint64_t)s->mac_reg[index+1] << 32;
>
> if (sum + size < sum) {
> sum = ~0;
> } else {
> sum += size;
> }
> s->mac_reg[index] = sum;
> s->mac_reg[index+1] = sum >> 32;
>
>> >
Looks good to me.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 5/6] e1000: Fixing the packet address filtering procedure
2015-10-18 7:53 [Qemu-devel] [PATCH 0/6] e1000: Various fixes and registers' implementation Leonid Bloch
` (3 preceding siblings ...)
2015-10-18 7:53 ` [Qemu-devel] [PATCH 4/6] e1000: Fixing the received/transmitted octets' counters Leonid Bloch
@ 2015-10-18 7:53 ` Leonid Bloch
2015-10-18 7:53 ` [Qemu-devel] [PATCH 6/6] e1000: Implementing various counters Leonid Bloch
2015-10-20 6:37 ` [Qemu-devel] [PATCH 0/6] e1000: Various fixes and registers' implementation Jason Wang
6 siblings, 0 replies; 21+ messages in thread
From: Leonid Bloch @ 2015-10-18 7:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Dmitry Fleytman, Jason Wang, Leonid Bloch, Shmulik Ladkani
Previously, if promiscuous unicast was enabled, a packet was received
straight away, even if it was a multicast or a broadcast packet. This
patch fixes that behavior, while making the filtering procedure a bit
more human-readable.
Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
---
hw/net/e1000.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 7f977b6..0ce7a7e 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -872,6 +872,7 @@ receive_filter(E1000State *s, const uint8_t *buf, int size)
static const uint8_t bcast[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
static const int mta_shift[] = {4, 3, 2, 0};
uint32_t f, rctl = s->mac_reg[RCTL], ra[2], *rp;
+ int isbcast = !memcmp(buf, bcast, sizeof bcast), ismcast = (buf[0] & 1);
if (is_vlan_packet(s, buf) && vlan_rx_filter_enabled(s)) {
uint16_t vid = be16_to_cpup((uint16_t *)(buf + 14));
@@ -881,14 +882,17 @@ receive_filter(E1000State *s, const uint8_t *buf, int size)
return 0;
}
- if (rctl & E1000_RCTL_UPE) // promiscuous
+ if (!isbcast && !ismcast && (rctl & E1000_RCTL_UPE)) { /* promiscuous ucast */
return 1;
+ }
- if ((buf[0] & 1) && (rctl & E1000_RCTL_MPE)) // promiscuous mcast
+ if (ismcast && (rctl & E1000_RCTL_MPE)) { /* promiscuous mcast */
return 1;
+ }
- if ((rctl & E1000_RCTL_BAM) && !memcmp(buf, bcast, sizeof bcast))
+ if (isbcast && (rctl & E1000_RCTL_BAM)) { /* broadcast enabled */
return 1;
+ }
for (rp = s->mac_reg + RA; rp < s->mac_reg + RA + 32; rp += 2) {
if (!(rp[1] & E1000_RAH_AV))
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* [Qemu-devel] [PATCH 6/6] e1000: Implementing various counters
2015-10-18 7:53 [Qemu-devel] [PATCH 0/6] e1000: Various fixes and registers' implementation Leonid Bloch
` (4 preceding siblings ...)
2015-10-18 7:53 ` [Qemu-devel] [PATCH 5/6] e1000: Fixing the packet address filtering procedure Leonid Bloch
@ 2015-10-18 7:53 ` Leonid Bloch
2015-10-20 6:34 ` Jason Wang
2015-10-20 6:37 ` [Qemu-devel] [PATCH 0/6] e1000: Various fixes and registers' implementation Jason Wang
6 siblings, 1 reply; 21+ messages in thread
From: Leonid Bloch @ 2015-10-18 7:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Dmitry Fleytman, Jason Wang, Leonid Bloch, Shmulik Ladkani
This implements the following Statistic registers (various counters)
according to Intel's specs:
TSCTC GOTCL GOTCH GORCL GORCH MPRC BPRC RUC ROC
BPTC MPTC PTC... PRC...
Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
---
hw/net/e1000.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 75 insertions(+), 8 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 0ce7a7e..afb0ebe 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -37,6 +37,8 @@
#include "e1000_regs.h"
+static const uint8_t bcast[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+
#define E1000_DEBUG
#ifdef E1000_DEBUG
@@ -178,7 +180,13 @@ enum {
defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
defreg(RLEC), defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
- defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
+ defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC),
+ defreg(RUC), defreg(ROC), defreg(GORCL), defreg(GORCH),
+ defreg(GOTCL), defreg(GOTCH), defreg(BPRC), defreg(MPRC),
+ defreg(TSCTC), defreg(PRC64), defreg(PRC127), defreg(PRC255),
+ defreg(PRC511), defreg(PRC1023), defreg(PRC1522), defreg(PTC64),
+ defreg(PTC127), defreg(PTC255), defreg(PTC511), defreg(PTC1023),
+ defreg(PTC1522), defreg(MPTC), defreg(BPTC)
};
static void
@@ -583,6 +591,16 @@ inc_reg_if_not_full(E1000State *s, int index)
}
}
+static inline void
+inc_tx_bcast_or_mcast_count(E1000State *s, unsigned char *arr)
+{
+ if (!memcmp(arr, bcast, sizeof bcast)) {
+ inc_reg_if_not_full(s, BPTC);
+ } else if (arr[0] & 1) {
+ inc_reg_if_not_full(s, MPTC);
+ }
+}
+
static void
grow_8reg_if_not_full(E1000State *s, int index, int size)
{
@@ -605,6 +623,24 @@ grow_8reg_if_not_full(E1000State *s, int index, int size)
}
}
+static void
+increase_size_stats(E1000State *s, const int *size_regs, int size)
+{
+ if (size > 1023) {
+ inc_reg_if_not_full(s, size_regs[5]);
+ } else if (size > 511) {
+ inc_reg_if_not_full(s, size_regs[4]);
+ } else if (size > 255) {
+ inc_reg_if_not_full(s, size_regs[3]);
+ } else if (size > 127) {
+ inc_reg_if_not_full(s, size_regs[2]);
+ } else if (size > 64) {
+ inc_reg_if_not_full(s, size_regs[1]);
+ } else if (size == 64) {
+ inc_reg_if_not_full(s, size_regs[0]);
+ }
+}
+
static inline int
vlan_enabled(E1000State *s)
{
@@ -656,6 +692,8 @@ xmit_seg(E1000State *s)
uint16_t len, *sp;
unsigned int frames = s->tx.tso_frames, css, sofar;
struct e1000_tx *tp = &s->tx;
+ static const int PTCregs[6] = {PTC64, PTC127, PTC255, PTC511,
+ PTC1023, PTC1522};
if (tp->tse && tp->cptse) {
css = tp->ipcss;
@@ -673,8 +711,11 @@ xmit_seg(E1000State *s)
if (tp->tcp) {
sofar = frames * tp->mss;
stl_be_p(tp->data+css+4, ldl_be_p(tp->data+css+4)+sofar); /* seq */
- if (tp->paylen - sofar > tp->mss)
+ if (tp->paylen - sofar > tp->mss) {
tp->data[css + 13] &= ~9; /* PSH, FIN */
+ } else if (frames) {
+ inc_reg_if_not_full(s, TSCTC);
+ }
} else /* UDP */
stw_be_p(tp->data+css+4, len);
if (tp->sum_needed & E1000_TXD_POPTS_TXSM) {
@@ -697,11 +738,19 @@ xmit_seg(E1000State *s)
memmove(tp->data, tp->data + 4, 8);
memcpy(tp->data + 8, tp->vlan_header, 4);
e1000_send_packet(s, tp->vlan, tp->size + 4);
- } else
+ inc_tx_bcast_or_mcast_count(s, tp->vlan);
+ increase_size_stats(s, PTCregs, tp->size + 4);
+ } else {
e1000_send_packet(s, tp->data, tp->size);
+ inc_tx_bcast_or_mcast_count(s, tp->data);
+ increase_size_stats(s, PTCregs, tp->size);
+ }
+
inc_reg_if_not_full(s, TPT);
grow_8reg_if_not_full(s, TOTL, s->tx.size);
s->mac_reg[GPTC] = s->mac_reg[TPT];
+ s->mac_reg[GOTCL] = s->mac_reg[TOTL];
+ s->mac_reg[GOTCH] = s->mac_reg[TOTH];
}
static void
@@ -869,7 +918,6 @@ start_xmit(E1000State *s)
static int
receive_filter(E1000State *s, const uint8_t *buf, int size)
{
- static const uint8_t bcast[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
static const int mta_shift[] = {4, 3, 2, 0};
uint32_t f, rctl = s->mac_reg[RCTL], ra[2], *rp;
int isbcast = !memcmp(buf, bcast, sizeof bcast), ismcast = (buf[0] & 1);
@@ -887,10 +935,12 @@ receive_filter(E1000State *s, const uint8_t *buf, int size)
}
if (ismcast && (rctl & E1000_RCTL_MPE)) { /* promiscuous mcast */
+ inc_reg_if_not_full(s, MPRC);
return 1;
}
if (isbcast && (rctl & E1000_RCTL_BAM)) { /* broadcast enabled */
+ inc_reg_if_not_full(s, BPRC);
return 1;
}
@@ -912,8 +962,10 @@ receive_filter(E1000State *s, const uint8_t *buf, int size)
f = mta_shift[(rctl >> E1000_RCTL_MO_SHIFT) & 3];
f = (((buf[5] << 8) | buf[4]) >> f) & 0xfff;
- if (s->mac_reg[MTA + (f >> 5)] & (1 << (f & 0x1f)))
+ if (s->mac_reg[MTA + (f >> 5)] & (1 << (f & 0x1f))) {
+ inc_reg_if_not_full(s, MPRC);
return 1;
+ }
DBGOUT(RXFILTER,
"dropping, inexact filter mismatch: %02x:%02x:%02x:%02x:%02x:%02x MO %d MTA[%d] %x\n",
buf[0], buf[1], buf[2], buf[3], buf[4], buf[5],
@@ -1002,6 +1054,8 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
size_t desc_offset;
size_t desc_size;
size_t total_size;
+ static const int PRCregs[6] = {PRC64, PRC127, PRC255, PRC511,
+ PRC1023, PRC1522};
if (!(s->mac_reg[STATUS] & E1000_STATUS_LU)) {
return -1;
@@ -1015,6 +1069,7 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
if (size < sizeof(min_buf)) {
iov_to_buf(iov, iovcnt, 0, min_buf, size);
memset(&min_buf[size], 0, sizeof(min_buf) - size);
+ inc_reg_if_not_full(s, RUC);
min_iov.iov_base = filter_buf = min_buf;
min_iov.iov_len = size = sizeof(min_buf);
iovcnt = 1;
@@ -1030,6 +1085,7 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
(size > MAXIMUM_ETHERNET_VLAN_SIZE
&& !(s->mac_reg[RCTL] & E1000_RCTL_LPE)))
&& !(s->mac_reg[RCTL] & E1000_RCTL_SBP)) {
+ inc_reg_if_not_full(s, ROC);
return size;
}
@@ -1115,6 +1171,7 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
}
} while (desc_offset < total_size);
+ increase_size_stats(s, PRCregs, total_size);
inc_reg_if_not_full(s, TPR);
s->mac_reg[GPRC] = s->mac_reg[TPR];
/* TOR - Total Octets Received:
@@ -1123,6 +1180,8 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
* Always include FCS length (4) in size.
*/
grow_8reg_if_not_full(s, TORL, size+4);
+ s->mac_reg[GORCL] = s->mac_reg[TORL];
+ s->mac_reg[GORCH] = s->mac_reg[TORH];
n = E1000_ICS_RXT0;
if ((rdt = s->mac_reg[RDT]) < s->mac_reg[RDH])
@@ -1274,10 +1333,18 @@ static uint32_t (*macreg_readops[])(E1000State *, int) = {
getreg(RLEC), getreg(XONRXC), getreg(XONTXC), getreg(XOFFRXC),
getreg(XOFFTXC), getreg(RFC), getreg(RJC), getreg(RNBC),
getreg(TSCTFC), getreg(MGTPRC), getreg(MGTPDC), getreg(MGTPTC),
-
- [TOTH] = mac_read_clr8, [TORH] = mac_read_clr8,
+ getreg(GORCL), getreg(GOTCL),
+
+ [TOTH] = mac_read_clr8, [TORH] = mac_read_clr8, [GOTCH] = mac_read_clr8,
+ [GORCH] = mac_read_clr8,
+ [PRC64] = mac_read_clr4, [PRC127] = mac_read_clr4, [PRC255] = mac_read_clr4,
+ [PRC511] = mac_read_clr4, [PRC1023] = mac_read_clr4, [PRC1522] = mac_read_clr4,
+ [PTC64] = mac_read_clr4, [PTC127] = mac_read_clr4, [PTC255] = mac_read_clr4,
+ [PTC511] = mac_read_clr4, [PTC1023] = mac_read_clr4, [PTC1522] = mac_read_clr4,
[GPRC] = mac_read_clr4, [GPTC] = mac_read_clr4, [TPR] = mac_read_clr4,
- [TPT] = mac_read_clr4,
+ [TPT] = mac_read_clr4, [RUC] = mac_read_clr4, [ROC] = mac_read_clr4,
+ [BPRC] = mac_read_clr4, [MPRC] = mac_read_clr4, [TSCTC] = mac_read_clr4,
+ [BPTC] = mac_read_clr4, [MPTC] = mac_read_clr4,
[ICR] = mac_icr_read, [EECD] = get_eecd, [EERD] = flash_eerd_read,
[RDFH] = mac_low13_read, [RDFT] = mac_low13_read,
[RDFHS] = mac_low13_read, [RDFTS] = mac_low13_read, [RDFPC] = mac_low13_read,
--
2.4.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] e1000: Implementing various counters
2015-10-18 7:53 ` [Qemu-devel] [PATCH 6/6] e1000: Implementing various counters Leonid Bloch
@ 2015-10-20 6:34 ` Jason Wang
2015-10-21 9:20 ` Leonid Bloch
0 siblings, 1 reply; 21+ messages in thread
From: Jason Wang @ 2015-10-20 6:34 UTC (permalink / raw)
To: Leonid Bloch, qemu-devel; +Cc: Dmitry Fleytman, Leonid Bloch, Shmulik Ladkani
On 10/18/2015 03:53 PM, Leonid Bloch wrote:
> This implements the following Statistic registers (various counters)
> according to Intel's specs:
>
> TSCTC GOTCL GOTCH GORCL GORCH MPRC BPRC RUC ROC
> BPTC MPTC PTC... PRC...
>
> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
> ---
> hw/net/e1000.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 75 insertions(+), 8 deletions(-)
>
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 0ce7a7e..afb0ebe 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -37,6 +37,8 @@
>
> #include "e1000_regs.h"
>
> +static const uint8_t bcast[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
> +
> #define E1000_DEBUG
>
> #ifdef E1000_DEBUG
> @@ -178,7 +180,13 @@ enum {
> defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
> defreg(RLEC), defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
> defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
> - defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
> + defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC),
> + defreg(RUC), defreg(ROC), defreg(GORCL), defreg(GORCH),
> + defreg(GOTCL), defreg(GOTCH), defreg(BPRC), defreg(MPRC),
> + defreg(TSCTC), defreg(PRC64), defreg(PRC127), defreg(PRC255),
> + defreg(PRC511), defreg(PRC1023), defreg(PRC1522), defreg(PTC64),
> + defreg(PTC127), defreg(PTC255), defreg(PTC511), defreg(PTC1023),
> + defreg(PTC1522), defreg(MPTC), defreg(BPTC)
> };
>
> static void
> @@ -583,6 +591,16 @@ inc_reg_if_not_full(E1000State *s, int index)
> }
> }
>
> +static inline void
> +inc_tx_bcast_or_mcast_count(E1000State *s, unsigned char *arr)
> +{
> + if (!memcmp(arr, bcast, sizeof bcast)) {
> + inc_reg_if_not_full(s, BPTC);
> + } else if (arr[0] & 1) {
> + inc_reg_if_not_full(s, MPTC);
> + }
> +}
> +
> static void
> grow_8reg_if_not_full(E1000State *s, int index, int size)
> {
> @@ -605,6 +623,24 @@ grow_8reg_if_not_full(E1000State *s, int index, int size)
> }
> }
>
> +static void
> +increase_size_stats(E1000State *s, const int *size_regs, int size)
> +{
> + if (size > 1023) {
> + inc_reg_if_not_full(s, size_regs[5]);
> + } else if (size > 511) {
> + inc_reg_if_not_full(s, size_regs[4]);
> + } else if (size > 255) {
> + inc_reg_if_not_full(s, size_regs[3]);
> + } else if (size > 127) {
> + inc_reg_if_not_full(s, size_regs[2]);
> + } else if (size > 64) {
> + inc_reg_if_not_full(s, size_regs[1]);
> + } else if (size == 64) {
> + inc_reg_if_not_full(s, size_regs[0]);
> + }
> +}
> +
> static inline int
> vlan_enabled(E1000State *s)
> {
> @@ -656,6 +692,8 @@ xmit_seg(E1000State *s)
> uint16_t len, *sp;
> unsigned int frames = s->tx.tso_frames, css, sofar;
> struct e1000_tx *tp = &s->tx;
> + static const int PTCregs[6] = {PTC64, PTC127, PTC255, PTC511,
> + PTC1023, PTC1522};
>
> if (tp->tse && tp->cptse) {
> css = tp->ipcss;
> @@ -673,8 +711,11 @@ xmit_seg(E1000State *s)
> if (tp->tcp) {
> sofar = frames * tp->mss;
> stl_be_p(tp->data+css+4, ldl_be_p(tp->data+css+4)+sofar); /* seq */
> - if (tp->paylen - sofar > tp->mss)
> + if (tp->paylen - sofar > tp->mss) {
> tp->data[css + 13] &= ~9; /* PSH, FIN */
> + } else if (frames) {
> + inc_reg_if_not_full(s, TSCTC);
> + }
> } else /* UDP */
> stw_be_p(tp->data+css+4, len);
> if (tp->sum_needed & E1000_TXD_POPTS_TXSM) {
> @@ -697,11 +738,19 @@ xmit_seg(E1000State *s)
> memmove(tp->data, tp->data + 4, 8);
> memcpy(tp->data + 8, tp->vlan_header, 4);
> e1000_send_packet(s, tp->vlan, tp->size + 4);
> - } else
> + inc_tx_bcast_or_mcast_count(s, tp->vlan);
> + increase_size_stats(s, PTCregs, tp->size + 4);
> + } else {
> e1000_send_packet(s, tp->data, tp->size);
> + inc_tx_bcast_or_mcast_count(s, tp->data);
> + increase_size_stats(s, PTCregs, tp->size);
> + }
> +
Why not put above just inside e1000_send_packet() to avoid code duplication?
> inc_reg_if_not_full(s, TPT);
> grow_8reg_if_not_full(s, TOTL, s->tx.size);
> s->mac_reg[GPTC] = s->mac_reg[TPT];
> + s->mac_reg[GOTCL] = s->mac_reg[TOTL];
> + s->mac_reg[GOTCH] = s->mac_reg[TOTH];
> }
>
> static void
> @@ -869,7 +918,6 @@ start_xmit(E1000State *s)
> static int
> receive_filter(E1000State *s, const uint8_t *buf, int size)
> {
> - static const uint8_t bcast[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
> static const int mta_shift[] = {4, 3, 2, 0};
> uint32_t f, rctl = s->mac_reg[RCTL], ra[2], *rp;
> int isbcast = !memcmp(buf, bcast, sizeof bcast), ismcast = (buf[0] & 1);
> @@ -887,10 +935,12 @@ receive_filter(E1000State *s, const uint8_t *buf, int size)
> }
>
> if (ismcast && (rctl & E1000_RCTL_MPE)) { /* promiscuous mcast */
> + inc_reg_if_not_full(s, MPRC);
> return 1;
> }
>
> if (isbcast && (rctl & E1000_RCTL_BAM)) { /* broadcast enabled */
> + inc_reg_if_not_full(s, BPRC);
> return 1;
> }
>
> @@ -912,8 +962,10 @@ receive_filter(E1000State *s, const uint8_t *buf, int size)
>
> f = mta_shift[(rctl >> E1000_RCTL_MO_SHIFT) & 3];
> f = (((buf[5] << 8) | buf[4]) >> f) & 0xfff;
> - if (s->mac_reg[MTA + (f >> 5)] & (1 << (f & 0x1f)))
> + if (s->mac_reg[MTA + (f >> 5)] & (1 << (f & 0x1f))) {
> + inc_reg_if_not_full(s, MPRC);
> return 1;
> + }
> DBGOUT(RXFILTER,
> "dropping, inexact filter mismatch: %02x:%02x:%02x:%02x:%02x:%02x MO %d MTA[%d] %x\n",
> buf[0], buf[1], buf[2], buf[3], buf[4], buf[5],
> @@ -1002,6 +1054,8 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
> size_t desc_offset;
> size_t desc_size;
> size_t total_size;
> + static const int PRCregs[6] = {PRC64, PRC127, PRC255, PRC511,
> + PRC1023, PRC1522};
>
> if (!(s->mac_reg[STATUS] & E1000_STATUS_LU)) {
> return -1;
> @@ -1015,6 +1069,7 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
> if (size < sizeof(min_buf)) {
> iov_to_buf(iov, iovcnt, 0, min_buf, size);
> memset(&min_buf[size], 0, sizeof(min_buf) - size);
> + inc_reg_if_not_full(s, RUC);
> min_iov.iov_base = filter_buf = min_buf;
> min_iov.iov_len = size = sizeof(min_buf);
> iovcnt = 1;
> @@ -1030,6 +1085,7 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
> (size > MAXIMUM_ETHERNET_VLAN_SIZE
> && !(s->mac_reg[RCTL] & E1000_RCTL_LPE)))
> && !(s->mac_reg[RCTL] & E1000_RCTL_SBP)) {
> + inc_reg_if_not_full(s, ROC);
> return size;
> }
>
> @@ -1115,6 +1171,7 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
> }
> } while (desc_offset < total_size);
>
> + increase_size_stats(s, PRCregs, total_size);
> inc_reg_if_not_full(s, TPR);
> s->mac_reg[GPRC] = s->mac_reg[TPR];
> /* TOR - Total Octets Received:
> @@ -1123,6 +1180,8 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
> * Always include FCS length (4) in size.
> */
> grow_8reg_if_not_full(s, TORL, size+4);
> + s->mac_reg[GORCL] = s->mac_reg[TORL];
> + s->mac_reg[GORCH] = s->mac_reg[TORH];
>
> n = E1000_ICS_RXT0;
> if ((rdt = s->mac_reg[RDT]) < s->mac_reg[RDH])
> @@ -1274,10 +1333,18 @@ static uint32_t (*macreg_readops[])(E1000State *, int) = {
> getreg(RLEC), getreg(XONRXC), getreg(XONTXC), getreg(XOFFRXC),
> getreg(XOFFTXC), getreg(RFC), getreg(RJC), getreg(RNBC),
> getreg(TSCTFC), getreg(MGTPRC), getreg(MGTPDC), getreg(MGTPTC),
> -
> - [TOTH] = mac_read_clr8, [TORH] = mac_read_clr8,
> + getreg(GORCL), getreg(GOTCL),
> +
> + [TOTH] = mac_read_clr8, [TORH] = mac_read_clr8, [GOTCH] = mac_read_clr8,
> + [GORCH] = mac_read_clr8,
> + [PRC64] = mac_read_clr4, [PRC127] = mac_read_clr4, [PRC255] = mac_read_clr4,
> + [PRC511] = mac_read_clr4, [PRC1023] = mac_read_clr4, [PRC1522] = mac_read_clr4,
> + [PTC64] = mac_read_clr4, [PTC127] = mac_read_clr4, [PTC255] = mac_read_clr4,
> + [PTC511] = mac_read_clr4, [PTC1023] = mac_read_clr4, [PTC1522] = mac_read_clr4,
> [GPRC] = mac_read_clr4, [GPTC] = mac_read_clr4, [TPR] = mac_read_clr4,
> - [TPT] = mac_read_clr4,
> + [TPT] = mac_read_clr4, [RUC] = mac_read_clr4, [ROC] = mac_read_clr4,
> + [BPRC] = mac_read_clr4, [MPRC] = mac_read_clr4, [TSCTC] = mac_read_clr4,
> + [BPTC] = mac_read_clr4, [MPTC] = mac_read_clr4,
> [ICR] = mac_icr_read, [EECD] = get_eecd, [EERD] = flash_eerd_read,
> [RDFH] = mac_low13_read, [RDFT] = mac_low13_read,
> [RDFHS] = mac_low13_read, [RDFTS] = mac_low13_read, [RDFPC] = mac_low13_read,
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH 6/6] e1000: Implementing various counters
2015-10-20 6:34 ` Jason Wang
@ 2015-10-21 9:20 ` Leonid Bloch
0 siblings, 0 replies; 21+ messages in thread
From: Leonid Bloch @ 2015-10-21 9:20 UTC (permalink / raw)
To: Jason Wang; +Cc: Dmitry Fleytman, Leonid Bloch, qemu-devel, Shmulik Ladkani
On Tue, Oct 20, 2015 at 9:34 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 10/18/2015 03:53 PM, Leonid Bloch wrote:
>> This implements the following Statistic registers (various counters)
>> according to Intel's specs:
>>
>> TSCTC GOTCL GOTCH GORCL GORCH MPRC BPRC RUC ROC
>> BPTC MPTC PTC... PRC...
>>
>> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
>> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
>> ---
>> hw/net/e1000.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 75 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>> index 0ce7a7e..afb0ebe 100644
>> --- a/hw/net/e1000.c
>> +++ b/hw/net/e1000.c
>> @@ -37,6 +37,8 @@
>>
>> #include "e1000_regs.h"
>>
>> +static const uint8_t bcast[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
>> +
>> #define E1000_DEBUG
>>
>> #ifdef E1000_DEBUG
>> @@ -178,7 +180,13 @@ enum {
>> defreg(DC), defreg(TNCRS), defreg(SEC), defreg(CEXTERR),
>> defreg(RLEC), defreg(XONRXC), defreg(XONTXC), defreg(XOFFRXC),
>> defreg(XOFFTXC), defreg(RFC), defreg(RJC), defreg(RNBC),
>> - defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC)
>> + defreg(TSCTFC), defreg(MGTPRC), defreg(MGTPDC), defreg(MGTPTC),
>> + defreg(RUC), defreg(ROC), defreg(GORCL), defreg(GORCH),
>> + defreg(GOTCL), defreg(GOTCH), defreg(BPRC), defreg(MPRC),
>> + defreg(TSCTC), defreg(PRC64), defreg(PRC127), defreg(PRC255),
>> + defreg(PRC511), defreg(PRC1023), defreg(PRC1522), defreg(PTC64),
>> + defreg(PTC127), defreg(PTC255), defreg(PTC511), defreg(PTC1023),
>> + defreg(PTC1522), defreg(MPTC), defreg(BPTC)
>> };
>>
>> static void
>> @@ -583,6 +591,16 @@ inc_reg_if_not_full(E1000State *s, int index)
>> }
>> }
>>
>> +static inline void
>> +inc_tx_bcast_or_mcast_count(E1000State *s, unsigned char *arr)
>> +{
>> + if (!memcmp(arr, bcast, sizeof bcast)) {
>> + inc_reg_if_not_full(s, BPTC);
>> + } else if (arr[0] & 1) {
>> + inc_reg_if_not_full(s, MPTC);
>> + }
>> +}
>> +
>> static void
>> grow_8reg_if_not_full(E1000State *s, int index, int size)
>> {
>> @@ -605,6 +623,24 @@ grow_8reg_if_not_full(E1000State *s, int index, int size)
>> }
>> }
>>
>> +static void
>> +increase_size_stats(E1000State *s, const int *size_regs, int size)
>> +{
>> + if (size > 1023) {
>> + inc_reg_if_not_full(s, size_regs[5]);
>> + } else if (size > 511) {
>> + inc_reg_if_not_full(s, size_regs[4]);
>> + } else if (size > 255) {
>> + inc_reg_if_not_full(s, size_regs[3]);
>> + } else if (size > 127) {
>> + inc_reg_if_not_full(s, size_regs[2]);
>> + } else if (size > 64) {
>> + inc_reg_if_not_full(s, size_regs[1]);
>> + } else if (size == 64) {
>> + inc_reg_if_not_full(s, size_regs[0]);
>> + }
>> +}
>> +
>> static inline int
>> vlan_enabled(E1000State *s)
>> {
>> @@ -656,6 +692,8 @@ xmit_seg(E1000State *s)
>> uint16_t len, *sp;
>> unsigned int frames = s->tx.tso_frames, css, sofar;
>> struct e1000_tx *tp = &s->tx;
>> + static const int PTCregs[6] = {PTC64, PTC127, PTC255, PTC511,
>> + PTC1023, PTC1522};
>>
>> if (tp->tse && tp->cptse) {
>> css = tp->ipcss;
>> @@ -673,8 +711,11 @@ xmit_seg(E1000State *s)
>> if (tp->tcp) {
>> sofar = frames * tp->mss;
>> stl_be_p(tp->data+css+4, ldl_be_p(tp->data+css+4)+sofar); /* seq */
>> - if (tp->paylen - sofar > tp->mss)
>> + if (tp->paylen - sofar > tp->mss) {
>> tp->data[css + 13] &= ~9; /* PSH, FIN */
>> + } else if (frames) {
>> + inc_reg_if_not_full(s, TSCTC);
>> + }
>> } else /* UDP */
>> stw_be_p(tp->data+css+4, len);
>> if (tp->sum_needed & E1000_TXD_POPTS_TXSM) {
>> @@ -697,11 +738,19 @@ xmit_seg(E1000State *s)
>> memmove(tp->data, tp->data + 4, 8);
>> memcpy(tp->data + 8, tp->vlan_header, 4);
>> e1000_send_packet(s, tp->vlan, tp->size + 4);
>> - } else
>> + inc_tx_bcast_or_mcast_count(s, tp->vlan);
>> + increase_size_stats(s, PTCregs, tp->size + 4);
>> + } else {
>> e1000_send_packet(s, tp->data, tp->size);
>> + inc_tx_bcast_or_mcast_count(s, tp->data);
>> + increase_size_stats(s, PTCregs, tp->size);
>> + }
>> +
>
> Why not put above just inside e1000_send_packet() to avoid code duplication?
Yes, you are right!
>
>> inc_reg_if_not_full(s, TPT);
>> grow_8reg_if_not_full(s, TOTL, s->tx.size);
>> s->mac_reg[GPTC] = s->mac_reg[TPT];
>> + s->mac_reg[GOTCL] = s->mac_reg[TOTL];
>> + s->mac_reg[GOTCH] = s->mac_reg[TOTH];
>> }
>>
>> static void
>> @@ -869,7 +918,6 @@ start_xmit(E1000State *s)
>> static int
>> receive_filter(E1000State *s, const uint8_t *buf, int size)
>> {
>> - static const uint8_t bcast[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
>> static const int mta_shift[] = {4, 3, 2, 0};
>> uint32_t f, rctl = s->mac_reg[RCTL], ra[2], *rp;
>> int isbcast = !memcmp(buf, bcast, sizeof bcast), ismcast = (buf[0] & 1);
>> @@ -887,10 +935,12 @@ receive_filter(E1000State *s, const uint8_t *buf, int size)
>> }
>>
>> if (ismcast && (rctl & E1000_RCTL_MPE)) { /* promiscuous mcast */
>> + inc_reg_if_not_full(s, MPRC);
>> return 1;
>> }
>>
>> if (isbcast && (rctl & E1000_RCTL_BAM)) { /* broadcast enabled */
>> + inc_reg_if_not_full(s, BPRC);
>> return 1;
>> }
>>
>> @@ -912,8 +962,10 @@ receive_filter(E1000State *s, const uint8_t *buf, int size)
>>
>> f = mta_shift[(rctl >> E1000_RCTL_MO_SHIFT) & 3];
>> f = (((buf[5] << 8) | buf[4]) >> f) & 0xfff;
>> - if (s->mac_reg[MTA + (f >> 5)] & (1 << (f & 0x1f)))
>> + if (s->mac_reg[MTA + (f >> 5)] & (1 << (f & 0x1f))) {
>> + inc_reg_if_not_full(s, MPRC);
>> return 1;
>> + }
>> DBGOUT(RXFILTER,
>> "dropping, inexact filter mismatch: %02x:%02x:%02x:%02x:%02x:%02x MO %d MTA[%d] %x\n",
>> buf[0], buf[1], buf[2], buf[3], buf[4], buf[5],
>> @@ -1002,6 +1054,8 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
>> size_t desc_offset;
>> size_t desc_size;
>> size_t total_size;
>> + static const int PRCregs[6] = {PRC64, PRC127, PRC255, PRC511,
>> + PRC1023, PRC1522};
>>
>> if (!(s->mac_reg[STATUS] & E1000_STATUS_LU)) {
>> return -1;
>> @@ -1015,6 +1069,7 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
>> if (size < sizeof(min_buf)) {
>> iov_to_buf(iov, iovcnt, 0, min_buf, size);
>> memset(&min_buf[size], 0, sizeof(min_buf) - size);
>> + inc_reg_if_not_full(s, RUC);
>> min_iov.iov_base = filter_buf = min_buf;
>> min_iov.iov_len = size = sizeof(min_buf);
>> iovcnt = 1;
>> @@ -1030,6 +1085,7 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
>> (size > MAXIMUM_ETHERNET_VLAN_SIZE
>> && !(s->mac_reg[RCTL] & E1000_RCTL_LPE)))
>> && !(s->mac_reg[RCTL] & E1000_RCTL_SBP)) {
>> + inc_reg_if_not_full(s, ROC);
>> return size;
>> }
>>
>> @@ -1115,6 +1171,7 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
>> }
>> } while (desc_offset < total_size);
>>
>> + increase_size_stats(s, PRCregs, total_size);
>> inc_reg_if_not_full(s, TPR);
>> s->mac_reg[GPRC] = s->mac_reg[TPR];
>> /* TOR - Total Octets Received:
>> @@ -1123,6 +1180,8 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt)
>> * Always include FCS length (4) in size.
>> */
>> grow_8reg_if_not_full(s, TORL, size+4);
>> + s->mac_reg[GORCL] = s->mac_reg[TORL];
>> + s->mac_reg[GORCH] = s->mac_reg[TORH];
>>
>> n = E1000_ICS_RXT0;
>> if ((rdt = s->mac_reg[RDT]) < s->mac_reg[RDH])
>> @@ -1274,10 +1333,18 @@ static uint32_t (*macreg_readops[])(E1000State *, int) = {
>> getreg(RLEC), getreg(XONRXC), getreg(XONTXC), getreg(XOFFRXC),
>> getreg(XOFFTXC), getreg(RFC), getreg(RJC), getreg(RNBC),
>> getreg(TSCTFC), getreg(MGTPRC), getreg(MGTPDC), getreg(MGTPTC),
>> -
>> - [TOTH] = mac_read_clr8, [TORH] = mac_read_clr8,
>> + getreg(GORCL), getreg(GOTCL),
>> +
>> + [TOTH] = mac_read_clr8, [TORH] = mac_read_clr8, [GOTCH] = mac_read_clr8,
>> + [GORCH] = mac_read_clr8,
>> + [PRC64] = mac_read_clr4, [PRC127] = mac_read_clr4, [PRC255] = mac_read_clr4,
>> + [PRC511] = mac_read_clr4, [PRC1023] = mac_read_clr4, [PRC1522] = mac_read_clr4,
>> + [PTC64] = mac_read_clr4, [PTC127] = mac_read_clr4, [PTC255] = mac_read_clr4,
>> + [PTC511] = mac_read_clr4, [PTC1023] = mac_read_clr4, [PTC1522] = mac_read_clr4,
>> [GPRC] = mac_read_clr4, [GPTC] = mac_read_clr4, [TPR] = mac_read_clr4,
>> - [TPT] = mac_read_clr4,
>> + [TPT] = mac_read_clr4, [RUC] = mac_read_clr4, [ROC] = mac_read_clr4,
>> + [BPRC] = mac_read_clr4, [MPRC] = mac_read_clr4, [TSCTC] = mac_read_clr4,
>> + [BPTC] = mac_read_clr4, [MPTC] = mac_read_clr4,
>> [ICR] = mac_icr_read, [EECD] = get_eecd, [EERD] = flash_eerd_read,
>> [RDFH] = mac_low13_read, [RDFT] = mac_low13_read,
>> [RDFHS] = mac_low13_read, [RDFTS] = mac_low13_read, [RDFPC] = mac_low13_read,
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 0/6] e1000: Various fixes and registers' implementation
2015-10-18 7:53 [Qemu-devel] [PATCH 0/6] e1000: Various fixes and registers' implementation Leonid Bloch
` (5 preceding siblings ...)
2015-10-18 7:53 ` [Qemu-devel] [PATCH 6/6] e1000: Implementing various counters Leonid Bloch
@ 2015-10-20 6:37 ` Jason Wang
2015-10-21 13:32 ` Leonid Bloch
6 siblings, 1 reply; 21+ messages in thread
From: Jason Wang @ 2015-10-20 6:37 UTC (permalink / raw)
To: Leonid Bloch, qemu-devel; +Cc: Dmitry Fleytman, Leonid Bloch, Shmulik Ladkani
On 10/18/2015 03:53 PM, Leonid Bloch wrote:
> This series fixes several issues with incorrect packet/octet counting in
> e1000's Statistic registers, fixes a bug in the packet address filtering
> procedure, and implements many MAC registers that were absent before.
> Additionally, some cosmetic changes are made.
>
> Leonid Bloch (6):
> e1000: Cosmetic and alignment fixes
> e1000: Trivial implementation of various MAC registers
> e1000: Fixing the received/transmitted packets' counters
> e1000: Fixing the received/transmitted octets' counters
> e1000: Fixing the packet address filtering procedure
> e1000: Implementing various counters
>
> hw/net/e1000.c | 313 ++++++++++++++++++++++++++++++++++++++--------------
> hw/net/e1000_regs.h | 8 +-
> 2 files changed, 236 insertions(+), 85 deletions(-)
>
Looks good to me overall, just few comments in individual patches.
A question here, is there any real user/OSes that tries to use those
registers? If not, maintain them sees a burden and it's a little bit
hard the test them unless unit-test were implemented for those
registers. And I'd like to know the test status of this series. At least
both windows and linux guest need to be tested.
Thanks
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH 0/6] e1000: Various fixes and registers' implementation
2015-10-20 6:37 ` [Qemu-devel] [PATCH 0/6] e1000: Various fixes and registers' implementation Jason Wang
@ 2015-10-21 13:32 ` Leonid Bloch
2015-10-22 7:22 ` Jason Wang
0 siblings, 1 reply; 21+ messages in thread
From: Leonid Bloch @ 2015-10-21 13:32 UTC (permalink / raw)
To: Jason Wang; +Cc: Dmitry Fleytman, Leonid Bloch, qemu-devel, Shmulik Ladkani
[-- Attachment #1: Type: text/plain, Size: 8657 bytes --]
Hi Jason, thanks again for reviewing,
On Tue, Oct 20, 2015 at 9:37 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 10/18/2015 03:53 PM, Leonid Bloch wrote:
>> This series fixes several issues with incorrect packet/octet counting in
>> e1000's Statistic registers, fixes a bug in the packet address filtering
>> procedure, and implements many MAC registers that were absent before.
>> Additionally, some cosmetic changes are made.
>>
>> Leonid Bloch (6):
>> e1000: Cosmetic and alignment fixes
>> e1000: Trivial implementation of various MAC registers
>> e1000: Fixing the received/transmitted packets' counters
>> e1000: Fixing the received/transmitted octets' counters
>> e1000: Fixing the packet address filtering procedure
>> e1000: Implementing various counters
>>
>> hw/net/e1000.c | 313
++++++++++++++++++++++++++++++++++++++--------------
>> hw/net/e1000_regs.h | 8 +-
>> 2 files changed, 236 insertions(+), 85 deletions(-)
>>
>
> Looks good to me overall, just few comments in individual patches.
>
> A question here, is there any real user/OSes that tries to use those
> registers? If not, maintain them sees a burden and it's a little bit
> hard the test them unless unit-test were implemented for those
> registers. And I'd like to know the test status of this series. At least
> both windows and linux guest need to be tested.
>
> Thanks
While we did not encounter any actual drivers that malfunction because of
the lack of these registers, implementing them makes the device closer to
Intel's specs, and reduces the chances that some OSes, currently or in the
future, may misbehave because of the missing registers. The implementation
of these additional registers seems as a natural continuation of this
series, the main purpose of which is to fix several bugs in this device.
As for testing, it was performed, obviously, in several different scenarios
with Linux (Fedora 22) + Windows (2012R2) guests. No regressions (and no
statistically significant deviations) were found. Please find
representative results (TCP, 1 stream) for both Linux and Windows guests
below:
Fedora 22 guest -- receive
5 +-+------------------------------------------------------------------+
| A
4.5 +-+ A A A B
4 +-+ A A |
| A B |
3.5 +-+ |
| A |
G 3 +-+ B |
b 2.5 +-+ |
/ | |
s 2 +-+ |
| A |
1.5 +-+ |
| |
1 +-+ A |
0.5 +-+ A |
A + + + + + + + + + + +
0 +-+---+------+-----+-----+-----+------+-----+-----+-----+------+-----+
32B 64B 128B 256B 512B 1KB 2KB 4KB 8KB 16KB 32KB
64KB
+--------Buffer size--------+
Mean-old -- A Mean-new -- B
Fedora 22 guest -- transmit
2 +-+------------------------------------------------------------------+
| B A
1.8 +-+ A |
1.6 +-+ |
| |
1.4 +-+ A |
| |
G 1.2 +-+ |
b 1 +-+ |
/ | A |
s 0.8 +-+ |
| |
0.6 +-+ A |
| |
0.4 +-+ A |
0.2 +-+ A |
+ + + + A + + + + + + +
0 A-+---A------A-----A-----+-----+------+-----+-----+-----+------+-----+
32B 64B 128B 256B 512B 1KB 2KB 4KB 8KB 16KB 32KB
64KB
+--------Buffer size--------+
Mean-old -- A Mean-new -- B
Windows 2012R2 guest -- receive
3.5 +-+------------------------------------------------------------------A
| A A B
3 +-+ |
| A |
| |
2.5 +-+ |
| A |
G 2 +-+ |
b | |
/ | A |
s 1.5 +-+ |
| B |
1 +-+ A |
| |
| A |
0.5 +-+ A |
+ A A + + + + + + + + +
0 A-+---+------+-----+-----+-----+------+-----+-----+-----+------+-----+
32B 64B 128B 256B 512B 1KB 2KB 4KB 8KB 16KB 32KB
64KB
+--------Buffer size--------+
Mean-old -- A Mean-new -- B
Windows 2012R2 guest --transmit
2.5 +-+------------------------------------------------------------------+
| A A
| B |
2 +-+ A |
| |
| A |
| |
G 1.5 +-+ B |
b | A |
/ | |
s 1 +-+ |
| B |
| A A |
| A |
0.5 +-+ A A |
| A |
A + + + + + + + + + + +
0 +-+---+------+-----+-----+-----+------+-----+-----+-----+------+-----+
32B 64B 128B 256B 512B 1KB 2KB 4KB 8KB 16KB 32KB
64KB
+--------Buffer size--------+
Mean-old -- A Mean-new -- B
Regards,
Leonid.
___
[-- Attachment #2: Type: text/html, Size: 16375 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Qemu-devel] [PATCH 0/6] e1000: Various fixes and registers' implementation
2015-10-21 13:32 ` Leonid Bloch
@ 2015-10-22 7:22 ` Jason Wang
0 siblings, 0 replies; 21+ messages in thread
From: Jason Wang @ 2015-10-22 7:22 UTC (permalink / raw)
To: Leonid Bloch; +Cc: Dmitry Fleytman, Leonid Bloch, qemu-devel, Shmulik Ladkani
On 10/21/2015 09:32 PM, Leonid Bloch wrote:
> Hi Jason, thanks again for reviewing,
>
> On Tue, Oct 20, 2015 at 9:37 AM, Jason Wang <jasowang@redhat.com
> <mailto:jasowang@redhat.com>> wrote:
> >
> >
> > On 10/18/2015 03:53 PM, Leonid Bloch wrote:
> >> This series fixes several issues with incorrect packet/octet
> counting in
> >> e1000's Statistic registers, fixes a bug in the packet address
> filtering
> >> procedure, and implements many MAC registers that were absent before.
> >> Additionally, some cosmetic changes are made.
> >>
> >> Leonid Bloch (6):
> >> e1000: Cosmetic and alignment fixes
> >> e1000: Trivial implementation of various MAC registers
> >> e1000: Fixing the received/transmitted packets' counters
> >> e1000: Fixing the received/transmitted octets' counters
> >> e1000: Fixing the packet address filtering procedure
> >> e1000: Implementing various counters
> >>
> >> hw/net/e1000.c | 313
> ++++++++++++++++++++++++++++++++++++++--------------
> >> hw/net/e1000_regs.h | 8 +-
> >> 2 files changed, 236 insertions(+), 85 deletions(-)
> >>
> >
> > Looks good to me overall, just few comments in individual patches.
> >
> > A question here, is there any real user/OSes that tries to use those
> > registers? If not, maintain them sees a burden and it's a little bit
> > hard the test them unless unit-test were implemented for those
> > registers. And I'd like to know the test status of this series. At least
> > both windows and linux guest need to be tested.
> >
> > Thanks
>
> While we did not encounter any actual drivers that malfunction because
> of the lack of these registers, implementing them makes the device
> closer to Intel's specs, and reduces the chances that some OSes,
> currently or in the future, may misbehave because of the missing
> registers. The implementation of these additional registers seems as a
> natural continuation of this series, the main purpose of which is to
> fix several bugs in this device.
>
> As for testing, it was performed, obviously, in several different
> scenarios with Linux (Fedora 22) + Windows (2012R2) guests. No
> regressions (and no statistically significant deviations) were found.
> Please find representative results (TCP, 1 stream) for both Linux and
> Windows guests below:
>
> Fedora 22 guest -- receive
> 5
> +-+------------------------------------------------------------------+
> |
> A
> 4.5 +-+ A A A
> B
> 4 +-+ A A
> |
> | A B
> |
> 3.5 +-+
> |
> | A
> |
> G 3 +-+ B
> |
> b 2.5 +-+
> |
> / |
> |
> s 2 +-+
> |
> | A
> |
> 1.5 +-+
> |
> |
> |
> 1 +-+ A
> |
> 0.5 +-+ A
> |
> A + + + + + + + + + +
> +
> 0
> +-+---+------+-----+-----+-----+------+-----+-----+-----+------+-----+
> 32B 64B 128B 256B 512B 1KB 2KB 4KB 8KB 16KB
> 32KB 64KB
> +--------Buffer size--------+
> Mean-old -- A Mean-new -- B
>
>
> Fedora 22 guest -- transmit
> 2
> +-+------------------------------------------------------------------+
> | B
> A
> 1.8 +-+ A
> |
> 1.6 +-+
> |
> |
> |
> 1.4 +-+ A
> |
> |
> |
> G 1.2 +-+
> |
> b 1 +-+
> |
> / | A
> |
> s 0.8 +-+
> |
> |
> |
> 0.6 +-+ A
> |
> |
> |
> 0.4 +-+ A
> |
> 0.2 +-+ A
> |
> + + + + A + + + + + +
> +
> 0
> A-+---A------A-----A-----+-----+------+-----+-----+-----+------+-----+
> 32B 64B 128B 256B 512B 1KB 2KB 4KB 8KB 16KB
> 32KB 64KB
> +--------Buffer size--------+
> Mean-old -- A Mean-new -- B
>
>
> Windows 2012R2 guest -- receive
> 3.5
> +-+------------------------------------------------------------------A
> | A A
> B
> 3 +-+
> |
> | A
> |
> |
> |
> 2.5 +-+
> |
> | A
> |
> G 2 +-+
> |
> b |
> |
> / | A
> |
> s 1.5 +-+
> |
> | B
> |
> 1 +-+ A
> |
> |
> |
> | A
> |
> 0.5 +-+ A
> |
> + A A + + + + + + + +
> +
> 0
> A-+---+------+-----+-----+-----+------+-----+-----+-----+------+-----+
> 32B 64B 128B 256B 512B 1KB 2KB 4KB 8KB 16KB
> 32KB 64KB
> +--------Buffer size--------+
> Mean-old -- A Mean-new -- B
>
>
> Windows 2012R2 guest --transmit
> 2.5
> +-+------------------------------------------------------------------+
> | A
> A
> | B
> |
> 2 +-+ A
> |
> |
> |
> | A
> |
> |
> |
> G 1.5 +-+ B
> |
> b | A
> |
> / |
> |
> s 1 +-+
> |
> | B
> |
> | A A
> |
> | A
> |
> 0.5 +-+ A A
> |
> | A
> |
> A + + + + + + + + + +
> +
> 0
> +-+---+------+-----+-----+-----+------+-----+-----+-----+------+-----+
> 32B 64B 128B 256B 512B 1KB 2KB 4KB 8KB 16KB
> 32KB 64KB
> +--------Buffer size--------+
> Mean-old -- A Mean-new -- B
>
>
> Regards,
> Leonid.
>
Result looks good.
Thanks for the testing.
^ permalink raw reply [flat|nested] 21+ messages in thread