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

[Philippe Mathieu-Daudé]

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;
     }
 }
-- 

Thanks,

Phil.