* [PATCH 1/3] hw/net: e1000: Correct the initial value of VET register @ 2021-07-01 9:46 Bin Meng 2021-07-01 9:46 ` [PATCH 2/3] hw/net: e1000e: " Bin Meng 2021-07-01 9:46 ` [PATCH 3/3] hw/net: e1000e: Don't zero out the VLAN tag in the legacy RX descriptor Bin Meng 0 siblings, 2 replies; 8+ messages in thread From: Bin Meng @ 2021-07-01 9:46 UTC (permalink / raw) To: Jason Wang, qemu-devel; +Cc: Bin Meng, Christina Wang, Markus Carlstedt From: Christina Wang <christina.wang@windriver.com> The initial value of VLAN Ether Type (VET) register is 0x8100, as per the manual and real hardware. While Linux e1000 driver always writes VET register to 0x8100, it is not always the case for everyone. Drivers relying on the reset value of VET won't be able to transmit and receive VLAN frames in QEMU. Reported-by: Markus Carlstedt <markus.carlstedt@windriver.com> Signed-off-by: Christina Wang <christina.wang@windriver.com> Signed-off-by: Bin Meng <bin.meng@windriver.com> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- hw/net/e1000.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/net/e1000.c b/hw/net/e1000.c index 4f75b44cfc..20cbba6411 100644 --- a/hw/net/e1000.c +++ b/hw/net/e1000.c @@ -29,6 +29,7 @@ #include "hw/pci/pci.h" #include "hw/qdev-properties.h" #include "migration/vmstate.h" +#include "net/eth.h" #include "net/net.h" #include "net/checksum.h" #include "sysemu/sysemu.h" @@ -254,6 +255,7 @@ static const uint32_t mac_reg_init[] = { [MANC] = E1000_MANC_EN_MNG2HOST | E1000_MANC_RCV_TCO_EN | E1000_MANC_ARP_EN | E1000_MANC_0298_EN | E1000_MANC_RMCP_EN, + [VET] = ETH_P_VLAN, }; /* Helper function, *curr == 0 means the value is not set */ -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] hw/net: e1000e: Correct the initial value of VET register 2021-07-01 9:46 [PATCH 1/3] hw/net: e1000: Correct the initial value of VET register Bin Meng @ 2021-07-01 9:46 ` Bin Meng 2021-07-02 3:29 ` Jason Wang 2021-07-01 9:46 ` [PATCH 3/3] hw/net: e1000e: Don't zero out the VLAN tag in the legacy RX descriptor Bin Meng 1 sibling, 1 reply; 8+ messages in thread From: Bin Meng @ 2021-07-01 9:46 UTC (permalink / raw) To: Jason Wang, qemu-devel; +Cc: Bin Meng, Christina Wang, Markus Carlstedt From: Christina Wang <christina.wang@windriver.com> The initial value of VLAN Ether Type (VET) register is 0x8100, as per the manual and real hardware. While Linux e1000e driver always writes VET register to 0x8100, it is not always the case for everyone. Drivers relying on the reset value of VET won't be able to transmit and receive VLAN frames in QEMU. Unlike e1000 in QEMU, e1000e uses a field 'vet' in "struct E1000Core" to cache the value of VET register, but the cache only gets updated when VET register is written. To always get a consistent VET value no matter VET is written or remains its reset value, drop the 'vet' field and use 'core->mac[VET]' directly. Reported-by: Markus Carlstedt <markus.carlstedt@windriver.com> Signed-off-by: Christina Wang <christina.wang@windriver.com> Signed-off-by: Bin Meng <bin.meng@windriver.com> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- hw/net/e1000e_core.h | 2 -- hw/net/e1000e.c | 6 ++---- hw/net/e1000e_core.c | 11 ++++++----- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h index 4ddb4d2c39..07d722bc68 100644 --- a/hw/net/e1000e_core.h +++ b/hw/net/e1000e_core.h @@ -105,8 +105,6 @@ struct E1000Core { uint32_t itr_guest_value; uint32_t eitr_guest_value[E1000E_MSIX_VEC_NUM]; - uint16_t vet; - uint8_t permanent_mac[ETH_ALEN]; NICState *owner_nic; diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c index a8a77eca95..1797e4a7cb 100644 --- a/hw/net/e1000e.c +++ b/hw/net/e1000e.c @@ -602,8 +602,8 @@ static const VMStateDescription e1000e_vmstate_intr_timer = { static const VMStateDescription e1000e_vmstate = { .name = "e1000e", - .version_id = 1, - .minimum_version_id = 1, + .version_id = 2, + .minimum_version_id = 2, .pre_save = e1000e_pre_save, .post_load = e1000e_post_load, .fields = (VMStateField[]) { @@ -645,8 +645,6 @@ static const VMStateDescription e1000e_vmstate = { VMSTATE_UINT32_ARRAY(core.eitr_guest_value, E1000EState, E1000E_MSIX_VEC_NUM), - VMSTATE_UINT16(core.vet, E1000EState), - VMSTATE_STRUCT_ARRAY(core.tx, E1000EState, E1000E_NUM_QUEUES, 0, e1000e_vmstate_tx, struct e1000e_tx), VMSTATE_END_OF_LIST() diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c index b75f2ab8fc..38b3e3b784 100644 --- a/hw/net/e1000e_core.c +++ b/hw/net/e1000e_core.c @@ -35,6 +35,7 @@ #include "qemu/osdep.h" #include "qemu/log.h" +#include "net/eth.h" #include "net/net.h" #include "net/tap.h" #include "hw/pci/msi.h" @@ -731,7 +732,7 @@ e1000e_process_tx_desc(E1000ECore *core, if (e1000x_vlan_enabled(core->mac) && e1000x_is_vlan_txd(txd_lower)) { net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, - le16_to_cpu(dp->upper.fields.special), core->vet); + le16_to_cpu(dp->upper.fields.special), core->mac[VET]); } if (e1000e_tx_pkt_send(core, tx, queue_index)) { e1000e_on_tx_done_update_stats(core, tx->tx_pkt); @@ -1012,7 +1013,7 @@ e1000e_receive_filter(E1000ECore *core, const uint8_t *buf, int size) { uint32_t rctl = core->mac[RCTL]; - if (e1000x_is_vlan_packet(buf, core->vet) && + if (e1000x_is_vlan_packet(buf, core->mac[VET]) && e1000x_vlan_rx_filter_enabled(core->mac)) { uint16_t vid = lduw_be_p(buf + 14); uint32_t vfta = ldl_le_p((uint32_t *)(core->mac + VFTA) + @@ -1686,7 +1687,7 @@ e1000e_receive_iov(E1000ECore *core, const struct iovec *iov, int iovcnt) } net_rx_pkt_attach_iovec_ex(core->rx_pkt, iov, iovcnt, iov_ofs, - e1000x_vlan_enabled(core->mac), core->vet); + e1000x_vlan_enabled(core->mac), core->mac[VET]); e1000e_rss_parse_packet(core, core->rx_pkt, &rss_info); e1000e_rx_ring_init(core, &rxr, rss_info.queue); @@ -2397,8 +2398,7 @@ static void e1000e_set_vet(E1000ECore *core, int index, uint32_t val) { core->mac[VET] = val & 0xffff; - core->vet = le16_to_cpu(core->mac[VET]); - trace_e1000e_vlan_vet(core->vet); + trace_e1000e_vlan_vet(core->mac[VET]); } static void @@ -3442,6 +3442,7 @@ static const uint32_t e1000e_mac_reg_init[] = { [RXCSUM] = E1000_RXCSUM_IPOFLD | E1000_RXCSUM_TUOFLD, [ITR] = E1000E_MIN_XITR, [EITR...EITR + E1000E_MSIX_VEC_NUM - 1] = E1000E_MIN_XITR, + [VET] = ETH_P_VLAN, }; void -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] hw/net: e1000e: Correct the initial value of VET register 2021-07-01 9:46 ` [PATCH 2/3] hw/net: e1000e: " Bin Meng @ 2021-07-02 3:29 ` Jason Wang 2021-07-02 4:43 ` Bin Meng 0 siblings, 1 reply; 8+ messages in thread From: Jason Wang @ 2021-07-02 3:29 UTC (permalink / raw) To: Bin Meng, qemu-devel; +Cc: Bin Meng, Christina Wang, Markus Carlstedt 在 2021/7/1 下午5:46, Bin Meng 写道: > From: Christina Wang <christina.wang@windriver.com> > > The initial value of VLAN Ether Type (VET) register is 0x8100, as per > the manual and real hardware. > > While Linux e1000e driver always writes VET register to 0x8100, it is > not always the case for everyone. Drivers relying on the reset value > of VET won't be able to transmit and receive VLAN frames in QEMU. > > Unlike e1000 in QEMU, e1000e uses a field 'vet' in "struct E1000Core" > to cache the value of VET register, but the cache only gets updated > when VET register is written. To always get a consistent VET value > no matter VET is written or remains its reset value, drop the 'vet' > field and use 'core->mac[VET]' directly. > > Reported-by: Markus Carlstedt <markus.carlstedt@windriver.com> > Signed-off-by: Christina Wang <christina.wang@windriver.com> > Signed-off-by: Bin Meng <bin.meng@windriver.com> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > --- > > hw/net/e1000e_core.h | 2 -- > hw/net/e1000e.c | 6 ++---- > hw/net/e1000e_core.c | 11 ++++++----- > 3 files changed, 8 insertions(+), 11 deletions(-) > > diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h > index 4ddb4d2c39..07d722bc68 100644 > --- a/hw/net/e1000e_core.h > +++ b/hw/net/e1000e_core.h > @@ -105,8 +105,6 @@ struct E1000Core { > uint32_t itr_guest_value; > uint32_t eitr_guest_value[E1000E_MSIX_VEC_NUM]; > > - uint16_t vet; > - > uint8_t permanent_mac[ETH_ALEN]; > > NICState *owner_nic; > diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c > index a8a77eca95..1797e4a7cb 100644 > --- a/hw/net/e1000e.c > +++ b/hw/net/e1000e.c > @@ -602,8 +602,8 @@ static const VMStateDescription e1000e_vmstate_intr_timer = { > > static const VMStateDescription e1000e_vmstate = { > .name = "e1000e", > - .version_id = 1, > - .minimum_version_id = 1, > + .version_id = 2, > + .minimum_version_id = 2, > .pre_save = e1000e_pre_save, > .post_load = e1000e_post_load, > .fields = (VMStateField[]) { > @@ -645,8 +645,6 @@ static const VMStateDescription e1000e_vmstate = { > VMSTATE_UINT32_ARRAY(core.eitr_guest_value, E1000EState, > E1000E_MSIX_VEC_NUM), > > - VMSTATE_UINT16(core.vet, E1000EState), This is not the suggested way. We'd better not bump version in this case. How about update vet during post_load? Thanks > - > VMSTATE_STRUCT_ARRAY(core.tx, E1000EState, E1000E_NUM_QUEUES, 0, > e1000e_vmstate_tx, struct e1000e_tx), > VMSTATE_END_OF_LIST() > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c > index b75f2ab8fc..38b3e3b784 100644 > --- a/hw/net/e1000e_core.c > +++ b/hw/net/e1000e_core.c > @@ -35,6 +35,7 @@ > > #include "qemu/osdep.h" > #include "qemu/log.h" > +#include "net/eth.h" > #include "net/net.h" > #include "net/tap.h" > #include "hw/pci/msi.h" > @@ -731,7 +732,7 @@ e1000e_process_tx_desc(E1000ECore *core, > if (e1000x_vlan_enabled(core->mac) && > e1000x_is_vlan_txd(txd_lower)) { > net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, > - le16_to_cpu(dp->upper.fields.special), core->vet); > + le16_to_cpu(dp->upper.fields.special), core->mac[VET]); > } > if (e1000e_tx_pkt_send(core, tx, queue_index)) { > e1000e_on_tx_done_update_stats(core, tx->tx_pkt); > @@ -1012,7 +1013,7 @@ e1000e_receive_filter(E1000ECore *core, const uint8_t *buf, int size) > { > uint32_t rctl = core->mac[RCTL]; > > - if (e1000x_is_vlan_packet(buf, core->vet) && > + if (e1000x_is_vlan_packet(buf, core->mac[VET]) && > e1000x_vlan_rx_filter_enabled(core->mac)) { > uint16_t vid = lduw_be_p(buf + 14); > uint32_t vfta = ldl_le_p((uint32_t *)(core->mac + VFTA) + > @@ -1686,7 +1687,7 @@ e1000e_receive_iov(E1000ECore *core, const struct iovec *iov, int iovcnt) > } > > net_rx_pkt_attach_iovec_ex(core->rx_pkt, iov, iovcnt, iov_ofs, > - e1000x_vlan_enabled(core->mac), core->vet); > + e1000x_vlan_enabled(core->mac), core->mac[VET]); > > e1000e_rss_parse_packet(core, core->rx_pkt, &rss_info); > e1000e_rx_ring_init(core, &rxr, rss_info.queue); > @@ -2397,8 +2398,7 @@ static void > e1000e_set_vet(E1000ECore *core, int index, uint32_t val) > { > core->mac[VET] = val & 0xffff; > - core->vet = le16_to_cpu(core->mac[VET]); > - trace_e1000e_vlan_vet(core->vet); > + trace_e1000e_vlan_vet(core->mac[VET]); > } > > static void > @@ -3442,6 +3442,7 @@ static const uint32_t e1000e_mac_reg_init[] = { > [RXCSUM] = E1000_RXCSUM_IPOFLD | E1000_RXCSUM_TUOFLD, > [ITR] = E1000E_MIN_XITR, > [EITR...EITR + E1000E_MSIX_VEC_NUM - 1] = E1000E_MIN_XITR, > + [VET] = ETH_P_VLAN, > }; > > void ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] hw/net: e1000e: Correct the initial value of VET register 2021-07-02 3:29 ` Jason Wang @ 2021-07-02 4:43 ` Bin Meng 2021-07-02 5:46 ` Jason Wang 0 siblings, 1 reply; 8+ messages in thread From: Bin Meng @ 2021-07-02 4:43 UTC (permalink / raw) To: Jason Wang Cc: Bin Meng, Christina Wang, qemu-devel@nongnu.org Developers, Markus Carlstedt On Fri, Jul 2, 2021 at 11:29 AM Jason Wang <jasowang@redhat.com> wrote: > > > 在 2021/7/1 下午5:46, Bin Meng 写道: > > From: Christina Wang <christina.wang@windriver.com> > > > > The initial value of VLAN Ether Type (VET) register is 0x8100, as per > > the manual and real hardware. > > > > While Linux e1000e driver always writes VET register to 0x8100, it is > > not always the case for everyone. Drivers relying on the reset value > > of VET won't be able to transmit and receive VLAN frames in QEMU. > > > > Unlike e1000 in QEMU, e1000e uses a field 'vet' in "struct E1000Core" > > to cache the value of VET register, but the cache only gets updated > > when VET register is written. To always get a consistent VET value > > no matter VET is written or remains its reset value, drop the 'vet' > > field and use 'core->mac[VET]' directly. > > > > Reported-by: Markus Carlstedt <markus.carlstedt@windriver.com> > > Signed-off-by: Christina Wang <christina.wang@windriver.com> > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > --- > > > > hw/net/e1000e_core.h | 2 -- > > hw/net/e1000e.c | 6 ++---- > > hw/net/e1000e_core.c | 11 ++++++----- > > 3 files changed, 8 insertions(+), 11 deletions(-) > > > > diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h > > index 4ddb4d2c39..07d722bc68 100644 > > --- a/hw/net/e1000e_core.h > > +++ b/hw/net/e1000e_core.h > > @@ -105,8 +105,6 @@ struct E1000Core { > > uint32_t itr_guest_value; > > uint32_t eitr_guest_value[E1000E_MSIX_VEC_NUM]; > > > > - uint16_t vet; > > - > > uint8_t permanent_mac[ETH_ALEN]; > > > > NICState *owner_nic; > > diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c > > index a8a77eca95..1797e4a7cb 100644 > > --- a/hw/net/e1000e.c > > +++ b/hw/net/e1000e.c > > @@ -602,8 +602,8 @@ static const VMStateDescription e1000e_vmstate_intr_timer = { > > > > static const VMStateDescription e1000e_vmstate = { > > .name = "e1000e", > > - .version_id = 1, > > - .minimum_version_id = 1, > > + .version_id = 2, > > + .minimum_version_id = 2, > > .pre_save = e1000e_pre_save, > > .post_load = e1000e_post_load, > > .fields = (VMStateField[]) { > > @@ -645,8 +645,6 @@ static const VMStateDescription e1000e_vmstate = { > > VMSTATE_UINT32_ARRAY(core.eitr_guest_value, E1000EState, > > E1000E_MSIX_VEC_NUM), > > > > - VMSTATE_UINT16(core.vet, E1000EState), > > > This is not the suggested way. We'd better not bump version in this case. > > How about update vet during post_load? But core.vet is removed in this patch. Not sure how to handle this? Regards, Bin ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] hw/net: e1000e: Correct the initial value of VET register 2021-07-02 4:43 ` Bin Meng @ 2021-07-02 5:46 ` Jason Wang 2021-07-02 6:12 ` Bin Meng 0 siblings, 1 reply; 8+ messages in thread From: Jason Wang @ 2021-07-02 5:46 UTC (permalink / raw) To: Bin Meng Cc: Bin Meng, Christina Wang, qemu-devel@nongnu.org Developers, Markus Carlstedt 在 2021/7/2 下午12:43, Bin Meng 写道: > On Fri, Jul 2, 2021 at 11:29 AM Jason Wang <jasowang@redhat.com> wrote: >> >> 在 2021/7/1 下午5:46, Bin Meng 写道: >>> From: Christina Wang <christina.wang@windriver.com> >>> >>> The initial value of VLAN Ether Type (VET) register is 0x8100, as per >>> the manual and real hardware. >>> >>> While Linux e1000e driver always writes VET register to 0x8100, it is >>> not always the case for everyone. Drivers relying on the reset value >>> of VET won't be able to transmit and receive VLAN frames in QEMU. >>> >>> Unlike e1000 in QEMU, e1000e uses a field 'vet' in "struct E1000Core" >>> to cache the value of VET register, but the cache only gets updated >>> when VET register is written. To always get a consistent VET value >>> no matter VET is written or remains its reset value, drop the 'vet' >>> field and use 'core->mac[VET]' directly. >>> >>> Reported-by: Markus Carlstedt <markus.carlstedt@windriver.com> >>> Signed-off-by: Christina Wang <christina.wang@windriver.com> >>> Signed-off-by: Bin Meng <bin.meng@windriver.com> >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>> --- >>> >>> hw/net/e1000e_core.h | 2 -- >>> hw/net/e1000e.c | 6 ++---- >>> hw/net/e1000e_core.c | 11 ++++++----- >>> 3 files changed, 8 insertions(+), 11 deletions(-) >>> >>> diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h >>> index 4ddb4d2c39..07d722bc68 100644 >>> --- a/hw/net/e1000e_core.h >>> +++ b/hw/net/e1000e_core.h >>> @@ -105,8 +105,6 @@ struct E1000Core { >>> uint32_t itr_guest_value; >>> uint32_t eitr_guest_value[E1000E_MSIX_VEC_NUM]; >>> >>> - uint16_t vet; >>> - >>> uint8_t permanent_mac[ETH_ALEN]; >>> >>> NICState *owner_nic; >>> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c >>> index a8a77eca95..1797e4a7cb 100644 >>> --- a/hw/net/e1000e.c >>> +++ b/hw/net/e1000e.c >>> @@ -602,8 +602,8 @@ static const VMStateDescription e1000e_vmstate_intr_timer = { >>> >>> static const VMStateDescription e1000e_vmstate = { >>> .name = "e1000e", >>> - .version_id = 1, >>> - .minimum_version_id = 1, >>> + .version_id = 2, >>> + .minimum_version_id = 2, >>> .pre_save = e1000e_pre_save, >>> .post_load = e1000e_post_load, >>> .fields = (VMStateField[]) { >>> @@ -645,8 +645,6 @@ static const VMStateDescription e1000e_vmstate = { >>> VMSTATE_UINT32_ARRAY(core.eitr_guest_value, E1000EState, >>> E1000E_MSIX_VEC_NUM), >>> >>> - VMSTATE_UINT16(core.vet, E1000EState), >> >> This is not the suggested way. We'd better not bump version in this case. >> >> How about update vet during post_load? > But core.vet is removed in this patch. Not sure how to handle this? Keep using core.vet, sync core.vet with mac[VET] during post_load. Thanks > > Regards, > Bin > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] hw/net: e1000e: Correct the initial value of VET register 2021-07-02 5:46 ` Jason Wang @ 2021-07-02 6:12 ` Bin Meng 2021-07-02 7:01 ` Jason Wang 0 siblings, 1 reply; 8+ messages in thread From: Bin Meng @ 2021-07-02 6:12 UTC (permalink / raw) To: Jason Wang Cc: Bin Meng, Christina Wang, qemu-devel@nongnu.org Developers, Markus Carlstedt On Fri, Jul 2, 2021 at 1:47 PM Jason Wang <jasowang@redhat.com> wrote: > > > 在 2021/7/2 下午12:43, Bin Meng 写道: > > On Fri, Jul 2, 2021 at 11:29 AM Jason Wang <jasowang@redhat.com> wrote: > >> > >> 在 2021/7/1 下午5:46, Bin Meng 写道: > >>> From: Christina Wang <christina.wang@windriver.com> > >>> > >>> The initial value of VLAN Ether Type (VET) register is 0x8100, as per > >>> the manual and real hardware. > >>> > >>> While Linux e1000e driver always writes VET register to 0x8100, it is > >>> not always the case for everyone. Drivers relying on the reset value > >>> of VET won't be able to transmit and receive VLAN frames in QEMU. > >>> > >>> Unlike e1000 in QEMU, e1000e uses a field 'vet' in "struct E1000Core" > >>> to cache the value of VET register, but the cache only gets updated > >>> when VET register is written. To always get a consistent VET value > >>> no matter VET is written or remains its reset value, drop the 'vet' > >>> field and use 'core->mac[VET]' directly. > >>> > >>> Reported-by: Markus Carlstedt <markus.carlstedt@windriver.com> > >>> Signed-off-by: Christina Wang <christina.wang@windriver.com> > >>> Signed-off-by: Bin Meng <bin.meng@windriver.com> > >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > >>> --- > >>> > >>> hw/net/e1000e_core.h | 2 -- > >>> hw/net/e1000e.c | 6 ++---- > >>> hw/net/e1000e_core.c | 11 ++++++----- > >>> 3 files changed, 8 insertions(+), 11 deletions(-) > >>> > >>> diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h > >>> index 4ddb4d2c39..07d722bc68 100644 > >>> --- a/hw/net/e1000e_core.h > >>> +++ b/hw/net/e1000e_core.h > >>> @@ -105,8 +105,6 @@ struct E1000Core { > >>> uint32_t itr_guest_value; > >>> uint32_t eitr_guest_value[E1000E_MSIX_VEC_NUM]; > >>> > >>> - uint16_t vet; > >>> - > >>> uint8_t permanent_mac[ETH_ALEN]; > >>> > >>> NICState *owner_nic; > >>> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c > >>> index a8a77eca95..1797e4a7cb 100644 > >>> --- a/hw/net/e1000e.c > >>> +++ b/hw/net/e1000e.c > >>> @@ -602,8 +602,8 @@ static const VMStateDescription e1000e_vmstate_intr_timer = { > >>> > >>> static const VMStateDescription e1000e_vmstate = { > >>> .name = "e1000e", > >>> - .version_id = 1, > >>> - .minimum_version_id = 1, > >>> + .version_id = 2, > >>> + .minimum_version_id = 2, > >>> .pre_save = e1000e_pre_save, > >>> .post_load = e1000e_post_load, > >>> .fields = (VMStateField[]) { > >>> @@ -645,8 +645,6 @@ static const VMStateDescription e1000e_vmstate = { > >>> VMSTATE_UINT32_ARRAY(core.eitr_guest_value, E1000EState, > >>> E1000E_MSIX_VEC_NUM), > >>> > >>> - VMSTATE_UINT16(core.vet, E1000EState), > >> > >> This is not the suggested way. We'd better not bump version in this case. > >> > >> How about update vet during post_load? > > But core.vet is removed in this patch. Not sure how to handle this? > > > Keep using core.vet, sync core.vet with mac[VET] during post_load. > But keeping using core.vet in the e1000e_core.c will cause mismatch with mac[VET] as the commit message says. We can still keep the 'vet' field in the "struct E1000Core", and keep the "VMSTATE_UINT16(core.vet, E1000EState)" here, but it is useless in the new code. Regards, Bin ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] hw/net: e1000e: Correct the initial value of VET register 2021-07-02 6:12 ` Bin Meng @ 2021-07-02 7:01 ` Jason Wang 0 siblings, 0 replies; 8+ messages in thread From: Jason Wang @ 2021-07-02 7:01 UTC (permalink / raw) To: Bin Meng Cc: Bin Meng, Christina Wang, qemu-devel@nongnu.org Developers, Markus Carlstedt 在 2021/7/2 下午2:12, Bin Meng 写道: > On Fri, Jul 2, 2021 at 1:47 PM Jason Wang <jasowang@redhat.com> wrote: >> >> 在 2021/7/2 下午12:43, Bin Meng 写道: >>> On Fri, Jul 2, 2021 at 11:29 AM Jason Wang <jasowang@redhat.com> wrote: >>>> 在 2021/7/1 下午5:46, Bin Meng 写道: >>>>> From: Christina Wang <christina.wang@windriver.com> >>>>> >>>>> The initial value of VLAN Ether Type (VET) register is 0x8100, as per >>>>> the manual and real hardware. >>>>> >>>>> While Linux e1000e driver always writes VET register to 0x8100, it is >>>>> not always the case for everyone. Drivers relying on the reset value >>>>> of VET won't be able to transmit and receive VLAN frames in QEMU. >>>>> >>>>> Unlike e1000 in QEMU, e1000e uses a field 'vet' in "struct E1000Core" >>>>> to cache the value of VET register, but the cache only gets updated >>>>> when VET register is written. To always get a consistent VET value >>>>> no matter VET is written or remains its reset value, drop the 'vet' >>>>> field and use 'core->mac[VET]' directly. >>>>> >>>>> Reported-by: Markus Carlstedt <markus.carlstedt@windriver.com> >>>>> Signed-off-by: Christina Wang <christina.wang@windriver.com> >>>>> Signed-off-by: Bin Meng <bin.meng@windriver.com> >>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>> --- >>>>> >>>>> hw/net/e1000e_core.h | 2 -- >>>>> hw/net/e1000e.c | 6 ++---- >>>>> hw/net/e1000e_core.c | 11 ++++++----- >>>>> 3 files changed, 8 insertions(+), 11 deletions(-) >>>>> >>>>> diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h >>>>> index 4ddb4d2c39..07d722bc68 100644 >>>>> --- a/hw/net/e1000e_core.h >>>>> +++ b/hw/net/e1000e_core.h >>>>> @@ -105,8 +105,6 @@ struct E1000Core { >>>>> uint32_t itr_guest_value; >>>>> uint32_t eitr_guest_value[E1000E_MSIX_VEC_NUM]; >>>>> >>>>> - uint16_t vet; >>>>> - >>>>> uint8_t permanent_mac[ETH_ALEN]; >>>>> >>>>> NICState *owner_nic; >>>>> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c >>>>> index a8a77eca95..1797e4a7cb 100644 >>>>> --- a/hw/net/e1000e.c >>>>> +++ b/hw/net/e1000e.c >>>>> @@ -602,8 +602,8 @@ static const VMStateDescription e1000e_vmstate_intr_timer = { >>>>> >>>>> static const VMStateDescription e1000e_vmstate = { >>>>> .name = "e1000e", >>>>> - .version_id = 1, >>>>> - .minimum_version_id = 1, >>>>> + .version_id = 2, >>>>> + .minimum_version_id = 2, >>>>> .pre_save = e1000e_pre_save, >>>>> .post_load = e1000e_post_load, >>>>> .fields = (VMStateField[]) { >>>>> @@ -645,8 +645,6 @@ static const VMStateDescription e1000e_vmstate = { >>>>> VMSTATE_UINT32_ARRAY(core.eitr_guest_value, E1000EState, >>>>> E1000E_MSIX_VEC_NUM), >>>>> >>>>> - VMSTATE_UINT16(core.vet, E1000EState), >>>> This is not the suggested way. We'd better not bump version in this case. >>>> >>>> How about update vet during post_load? >>> But core.vet is removed in this patch. Not sure how to handle this? >> >> Keep using core.vet, sync core.vet with mac[VET] during post_load. >> > But keeping using core.vet in the e1000e_core.c will cause mismatch > with mac[VET] as the commit message says. > > We can still keep the 'vet' field in the "struct E1000Core", and keep > the "VMSTATE_UINT16(core.vet, E1000EState)" here, but it is useless in > the new code. The point is to unbreak migration to old version. Thanks > > Regards, > Bin > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] hw/net: e1000e: Don't zero out the VLAN tag in the legacy RX descriptor 2021-07-01 9:46 [PATCH 1/3] hw/net: e1000: Correct the initial value of VET register Bin Meng 2021-07-01 9:46 ` [PATCH 2/3] hw/net: e1000e: " Bin Meng @ 2021-07-01 9:46 ` Bin Meng 1 sibling, 0 replies; 8+ messages in thread From: Bin Meng @ 2021-07-01 9:46 UTC (permalink / raw) To: Jason Wang, qemu-devel; +Cc: Bin Meng, Christina Wang, Markus Carlstedt From: Christina Wang <christina.wang@windriver.com> In the legacy RX descriptor mode, VLAN tag was saved to d->special by e1000e_build_rx_metadata() in e1000e_write_lgcy_rx_descr(), but it was then zeroed out again at the end of the call, which is wrong. Fixes: c89d416a2b0f ("e1000e: Don't zero out buffer address in rx descriptor") Reported-by: Markus Carlstedt <markus.carlstedt@windriver.com> Signed-off-by: Christina Wang <christina.wang@windriver.com> Signed-off-by: Bin Meng <bin.meng@windriver.com> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- hw/net/e1000e_core.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c index 38b3e3b784..738c7169e4 100644 --- a/hw/net/e1000e_core.c +++ b/hw/net/e1000e_core.c @@ -1286,7 +1286,6 @@ e1000e_write_lgcy_rx_descr(E1000ECore *core, uint8_t *desc, &d->special); d->errors = (uint8_t) (le32_to_cpu(status_flags) >> 24); d->status = (uint8_t) le32_to_cpu(status_flags); - d->special = 0; } static inline void -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-07-02 7:02 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-01 9:46 [PATCH 1/3] hw/net: e1000: Correct the initial value of VET register Bin Meng 2021-07-01 9:46 ` [PATCH 2/3] hw/net: e1000e: " Bin Meng 2021-07-02 3:29 ` Jason Wang 2021-07-02 4:43 ` Bin Meng 2021-07-02 5:46 ` Jason Wang 2021-07-02 6:12 ` Bin Meng 2021-07-02 7:01 ` Jason Wang 2021-07-01 9:46 ` [PATCH 3/3] hw/net: e1000e: Don't zero out the VLAN tag in the legacy RX descriptor Bin Meng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).