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

[Bin Meng]

On Tue, Jul 13, 2021 at 5:02 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2021/7/13 下午4:36, Bin Meng 写道:
> > On Tue, Jul 13, 2021 at 3:03 PM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> 在 2021/7/13 上午7:06, Bin Meng 写道:
> >>> On Mon, Jul 5, 2021 at 1:57 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>>> On Mon, Jul 5, 2021 at 12:21 PM Jason Wang <jasowang@redhat.com> wrote:
> >>>>> 在 2021/7/2 下午5:24, 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>
> >>>>>> ---
> >>>>>>
> >>>>>> (no changes since v1)
> >>>>>>
> >>>>>>     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,
> >>>>> I wonder if we need a compat flag for this, since we change the 
> >>>>> behavior.
> >>>>>
> >>>>> (See e1000_properties[])
> >>>>>
> >>>> No we don't need to since it does not break migration.
> >>> Ping?
> >>
> >> I admit migration "works" but it doesn't mean it's not broken. It
> >> changes the guest visible default value of VET register, so it may break
> >> things silently for the guest.
> >>
> >> For old machine types, we should stick the value to the one without this
> >> fix.
> > Could you please propose a solution on how to handle such a scenario
> > in a generic way in QEMU? (+Peter)
>
>
> Well, I think I've suggested you to have a look at how things is done in
> for handling such compatibility in e1000_properties.
>
>
> >
> > The POR reset value is wrong in QEMU and has carried forward the wrong
> > value for years, and correcting it to its right value needs to do
> > what?
>
>
> We should stick to the wrong behavior for old machine types.
>
> That's all.

So that means the following SD patch is also wrong (+Philippe) which
changes the default value of capability register.
http://patchwork.ozlabs.org/project/qemu-devel/patch/20210623185921.24113-1-joannekoong@gmail.com/

Can we get some agreement among maintainers?

Regards,
Bin