Re: [Qemu-devel] [PATCH] e1000e: Do not auto-clear ICR bits which aren't set in EIAC

[Jason Wang <jasowang@redhat.com>]

On 2018年06月13日 10:29, Jason Wang wrote:
>
>
> On 2018年06月13日 10:26, Philippe Mathieu-Daudé wrote:
>> Hi Jason,
>>
>> On 06/12/2018 11:18 PM, Jason Wang wrote:
>>> On 2018年06月13日 03:00, Philippe Mathieu-Daudé wrote:
>>>> Cc'ing Jason who is also listed as co-maintainer:
>>>>
>>>>     ./scripts/get_maintainer.pl -f hw/net/e1000e_core.c
>>>>     Dmitry Fleytman <dmitry.fleytman@gmail.com> (maintainer:e1000e)
>>>>     Jason Wang <jasowang@redhat.com> (odd fixer:Network devices)
>>>>
>>>> On 06/12/2018 03:43 PM, Jan Kiszka wrote:
>>>>> On 2018-06-12 20:38, Philippe Mathieu-Daudé wrote:
>>>>>> On 06/12/2018 03:30 PM, Jan Kiszka wrote:
>>>>>>> On 2018-06-12 20:11, Philippe Mathieu-Daudé wrote:
>>>>>>>> Hi Jan,
>>>>>>>>
>>>>>>>> On 06/12/2018 02:22 PM, Jan Kiszka wrote:
>>>>>>>>> On 2018-05-22 09:00, Jan Kiszka wrote:
>>>>>>>>>> On 2018-04-16 17:29, Peter Maydell wrote:
>>>>>>>>>>> On 16 April 2018 at 16:25, Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>>> wrote:
>>>>>>>>>>>> On 2018-04-01 23:17, Jan Kiszka wrote:
>>>>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>>>>>
>>>>>>>>>>>>> The spec does not justify clearing of any
>>>>>>>>>>>>> E1000_ICR_OTHER_CAUSES when
>>>>>>>>>>>>> E1000_ICR_OTHER is set in EIAC. In fact, removing this code
>>>>>>>>>>>>> fixes the
>>>>>>>>>>>>> issue the Linux driver runs into since 4aea7a5c5e94 ("e1000e:
>>>>>>>>>>>>> Avoid
>>>>>>>>>>>>> receiver overrun interrupt bursts") and was worked around by
>>>>>>>>>>>>> 745d0bd3af99 ("e1000e: Remove Other from EIAC").
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>
>>>>>>>>>>>>> This resolves the issue I reported on February 18 ("e1000e:
>>>>>>>>>>>>> MSI-X
>>>>>>>>>>>>> problem with recent Linux drivers").
>>>>>>>>>>>>>
>>>>>>>>>>>>>    hw/net/e1000e_core.c | 4 ----
>>>>>>>>>>>>>    1 file changed, 4 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>>>>>>>>>>>>> index ecf9b15555..d38f025c0f 100644
>>>>>>>>>>>>> --- a/hw/net/e1000e_core.c
>>>>>>>>>>>>> +++ b/hw/net/e1000e_core.c
>>>>>>>>>>>>> @@ -2022,10 +2022,6 @@ e1000e_msix_notify_one(E1000ECore
>>>>>>>>>>>>> *core, uint32_t cause, uint32_t int_cfg)
>>>>>>>>>>>>>
>>>>>>>>>>>>>        effective_eiac = core->mac[EIAC] & cause;
>>>>>>>>>>>>>
>>>>>>>>>>>>> -    if (effective_eiac == E1000_ICR_OTHER) {
>>>>>>>>>>>>> -        effective_eiac |= E1000_ICR_OTHER_CAUSES;
>>>>>>>>>>>>> -    }
>>>>>>>>>>>>> -
>>>>>>>>>>>>>        core->mac[ICR] &= ~effective_eiac;
>>>>>>>>>>>>>
>>>>>>>>>>>>>        if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
>>>>>>>>>>>>>
>>>>>>>>>>>> Ping for this - as well as
>>>>>>>>>>>> https://patchwork.ozlabs.org/patch/895476.
>>>>>>>>>>>>
>>>>>>>>>>>> Given that q35 uses e1000e by default and many Linux kernel
>>>>>>>>>>>> versions no
>>>>>>>>>>>> longer work, this should likely go into upcoming and stable
>>>>>>>>>>>> versions
>>>>>>>>>>> I'd rather not put it into 2.12 at this point in the release
>>>>>>>>>>> cycle unless it's a regression from 2.11, I think.
>>>>>>>>>> Second ping - nothing hit the repo so far, nor did I receive
>>>>>>>>>> feedback.
>>>>>>>>>>
>>>>>>>>> And another ping. For both.
>>>>>>>>>
>>>>>>>>> These days I had to help someone with a broken QEMU setup that
>>>>>>>>> failed
>>>>>>>>> installing from network. It turned out that "modprobe e1000e
>>>>>>>>> IntMode=0"
>>>>>>>>> was needed to workaround the issues my patches address.
>>>>>>>> What about the IMS register? It is set just after.
>>>>>>>>
>>>>>>>> Looking at b38636b8372, can you test this patch?
>>>>>>>>
>>>>>>>> -- >8 --
>>>>>>>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>>>>>>>> index c93c4661ed..a484b68a5a 100644
>>>>>>>> --- a/hw/net/e1000e_core.c
>>>>>>>> +++ b/hw/net/e1000e_core.c
>>>>>>>> @@ -2022,13 +2022,13 @@ e1000e_msix_notify_one(E1000ECore *core,
>>>>>>>> uint32_t cause, uint32_t int_cfg)
>>>>>>>>
>>>>>>>>        effective_eiac = core->mac[EIAC] & cause;
>>>>>>>>
>>>>>>>> -    if (effective_eiac == E1000_ICR_OTHER) {
>>>>>>>> -        effective_eiac |= E1000_ICR_OTHER_CAUSES;
>>>>>>>> -    }
>>>>>>>> -
>>>>>>>>        core->mac[ICR] &= ~effective_eiac;
>>>>>>>>
>>>>>>>>        if (!(core->mac[CTRL_EXT] & E1000_CTRL_EXT_IAME)) {
>>>>>>>> +        if (effective_eiac == E1000_ICR_OTHER) {
>>>>>>>> +            effective_eiac |= E1000_ICR_OTHER_CAUSES;
>>>>>>>> +        }
>>>>>>>> +
>>>>>>>>            core->mac[IMS] &= ~effective_eiac;
>>>>>>>>        }
>>>>>>>>    }
>>>>>>>>
>>>>>>> Before testing this: What would be the reasoning for this change?
>>>>>> Not breaking the purpose of b38636b8372 :)
>>>>> I disagree on this expansion of bit 31 ("other causes"). I see no
>>>>> indication in the spec that setting this bit for autoclear has more
>>>>> impact than on the very same bit itself. Therefore I'm asking for a
>>>>> reasoning - based on the spec.
>>>>     Interrupt Cause Registers
>>>>
>>>>       Other bits in this register are the legacy indication of
>>>>       interrupts as the MDIC complete, management and link status
>>>>       change. There is a specific Other Cause bit that is set if
>>>>       one of these bits are set, this bit can be mapped to a
>>>>       specific MSI-X interrupt message.
>>>>
>>>> I spent half an hour reading the relevant parts of the spec and can't
>>>> figure out, so I'll let the authors of b38636b8372 to review your 
>>>> patch.
>>>>
>>>> Regards,
>>>>
>>>> Phil.
>>> Looking at EIAC part of the spec:
>>>
>>> Bits 24:20 in this register enables clearing of the corresponding 
>>> bit in
>>> ICR following
>>> interrupt generation. When a bit is set, the corresponding bit in ICR
>>> and in IMS is
>>> automatically cleared following an interrupt.
>> Thanks for looking at this.
>>
>>> It looks to me that only the other bit itself need to be cleared.
>> So no need to set the other_causes bits, thus Jan patch is correct?
>>
>> Thanks,
>>
>> Phil.
>>
>
> Yes, I think so. But I think we can wait for few days to see if Dmitry 
> have come comments.
>
> Thanks
>

Ok, I applied this first.

Thanks