qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Philippe Mathieu-Daudé
Subject: Re: [Qemu-devel] [PATCH] e1000e: Do not auto-clear ICR bits which aren't set in EIAC
Date: Tue, 12 Jun 2018 16:00:48 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0

Cc'ing Jason who is also listed as co-maintainer:

  ./scripts/get_maintainer.pl -f hw/net/e1000e_core.c
  Dmitry Fleytman <address@hidden> (maintainer:e1000e)
  Jason Wang <address@hidden> (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 <address@hidden> wrote:
>>>>>>>> On 2018-04-01 23:17, Jan Kiszka wrote:
>>>>>>>>> From: Jan Kiszka <address@hidden>
>>>>>>>>>
>>>>>>>>> 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 <address@hidden>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> 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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]