Cc'ing Jason who is also listed as co-maintainer:
./scripts/get_maintainer.pl -f hw/net/e1000e_core.c
Dmitry Fleytman <dmitry.fleyt...@gmail.com> (maintainer:e1000e)
Jason Wang <jasow...@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.kis...@siemens.com> wrote:
>>>>>>>> On 2018-04-01 23:17, Jan Kiszka wrote:
>>>>>>>>> From: Jan Kiszka <jan.kis...@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.kis...@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.
