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

[Jason Wang <jasowang@redhat.com>]

在 2021/7/14 上午11:42, 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.


That's the price for the stable ABI. See one of my recent fix - 
d83f46d189 virtio-pci: compat page aligned ATS. It keeps the "buggy" 
behavior to unbreak the migration.


>
> 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")


I guess it's simply because fsl_etsec is not used in any 
distributions/production environments or the maintainer may just not 
notice things like this.

But for e1000(e), we should stick to a stable ABI for consistency. 
Otherwise it would be very tricky to fix them after we saw real issues. 
We had learnt a lot during the past decade.

Thanks


>
> Regards,
> Bin
>