| Message ID | 20210701094651.1258613-2-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] hw/net: e1000: Correct the initial value of VET register | expand |
在 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
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
在 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 >
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
在 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 >
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