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

[Jason Wang <jasowang@redhat.com>]

在 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