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

[Bin Meng]

On Wed, Jul 14, 2021 at 11:10 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2021/7/13 下午5:11, 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/
>
>
> It should compat capareg for the old value for old machine types.

Yeah, it's already a property for the SD controller model but someone
views it as a bug because the model implements 64-bit but not
reporting it in the capability register.

>
>
> >
> > Can we get some agreement among maintainers?
>
>
> It's not about the agreement but about to have a stable ABI. I don't
> know the case for sd but e1000 is used in various  and we work hard to
> unbreak the migration compatibility among downstream versions. Git log
> on e1000.c will tell you more.

Agreement or stable ABI, whatever we call, but we should be in some consistency.

IMHO maintainers should reach an agreement to some extent on how
compatibility should be achieved. I just found silly to add a property
to fix a real bug in the model, and we preserve the bug all over
releases.

I can find plenty of examples in the current QEMU tree that were
accepted that changed the bugous register behavior, but it was not
asked to add new properties to keep the bugos behavior.

e.g.: commit ce8e43760e8e ("hw/net: fsl_etsec: Reverse the RCTRL.RSF logic")

Regards,
Bin