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: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