[PATCH 1/3] hw/net: e1000: Correct the initial value of VET register

[PATCH 1/3] hw/net: e1000: Correct the initial value of VET register

[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 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

[PATCH 2/3] hw/net: e1000e: Correct the initial value of VET register

[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),
-
         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

[PATCH 3/3] hw/net: e1000e: Don't zero out the VLAN tag in the legacy RX descriptor

[Bin Meng]

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

Re: [PATCH 2/3] hw/net: e1000e: Correct the initial value of VET register

[Jason Wang]

在 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

Re: [PATCH 2/3] hw/net: e1000e: Correct the initial value of VET register

[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?

Regards,
Bin

Re: [PATCH 2/3] hw/net: e1000e: Correct the initial value of VET register

[Jason Wang]

在 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
>

Re: [PATCH 2/3] hw/net: e1000e: Correct the initial value of VET register

[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.

Regards,
Bin

Re: [PATCH 2/3] hw/net: e1000e: Correct the initial value of VET register

[Jason Wang]

在 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
>