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

[Jan Kiszka]

On 2018-06-15 04:07, Jason Wang wrote:
> 
> 
> 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  (maintainer:e1000e)
>     Jason Wang  (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 
 wrote:
> On 2018-04-01 23:17, Jan Kiszka wrote:
>> From: Jan Kiszka 
>>
>> 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 
>> ---
>>
>> 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 ecf9b1..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
>

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

[Jason Wang]

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  (maintainer:e1000e)
    Jason Wang  (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 
wrote:

On 2018-04-01 23:17, Jan Kiszka wrote:

From: Jan Kiszka 

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

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 ecf9b1..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

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

[Jason Wang]

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  (maintainer:e1000e)
    Jason Wang  (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 
wrote:

On 2018-04-01 23:17, Jan Kiszka wrote:

From: Jan Kiszka 

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

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 ecf9b1..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

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

[Philippe Mathieu-Daudé]

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  (maintainer:e1000e)
>>    Jason Wang  (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 
> wrote:
>> On 2018-04-01 23:17, Jan Kiszka wrote:
>>> From: Jan Kiszka 
>>>
>>> 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 
>>> ---
>>>
>>> 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 ecf9b1..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 

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

[Jason Wang]

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  (maintainer:e1000e)
   Jason Wang  (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  wrote:

On 2018-04-01 23:17, Jan Kiszka wrote:

From: Jan Kiszka 

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

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 ecf9b1..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.

It looks to me that only the other bit itself need to be cleared.

Thanks

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

[Philippe Mathieu-Daudé]

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

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

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

[Jan Kiszka]

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  wrote:
>>> On 2018-04-01 23:17, Jan Kiszka wrote:
 From: Jan Kiszka 

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

 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 ecf9b1..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.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

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

[Philippe Mathieu-Daudé]

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  wrote:
>> On 2018-04-01 23:17, Jan Kiszka wrote:
>>> From: Jan Kiszka 
>>>
>>> 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 
>>> ---
>>>
>>> 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 ecf9b1..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 need to refresh my caches, the debugging session is now too long ago
> again.
> 
> Jan
> 

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

[Jan Kiszka]

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  wrote:
> On 2018-04-01 23:17, Jan Kiszka wrote:
>> From: Jan Kiszka 
>>
>> 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 
>> ---
>>
>> 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 ecf9b1..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?

I need to refresh my caches, the debugging session is now too long ago
again.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

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  wrote:
 On 2018-04-01 23:17, Jan Kiszka wrote:
> From: Jan Kiszka 
>
> 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 
> ---
>
> 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 ecf9b1..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.

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

[Jan Kiszka]

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  wrote:
>>> On 2018-04-01 23:17, Jan Kiszka wrote:
 From: Jan Kiszka 

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

 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 ecf9b1..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.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

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

[Jan Kiszka]

On 2018-04-16 17:29, Peter Maydell wrote:
> On 16 April 2018 at 16:25, Jan Kiszka  wrote:
>> On 2018-04-01 23:17, Jan Kiszka wrote:
>>> From: Jan Kiszka 
>>>
>>> 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 
>>> ---
>>>
>>> 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 ecf9b1..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.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

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

[Peter Maydell]

On 16 April 2018 at 16:25, Jan Kiszka  wrote:
> On 2018-04-01 23:17, Jan Kiszka wrote:
>> From: Jan Kiszka 
>>
>> 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 
>> ---
>>
>> 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 ecf9b1..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.

thanks
-- PMM

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

[Jan Kiszka]

On 2018-04-01 23:17, Jan Kiszka wrote:
> From: Jan Kiszka 
> 
> 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 
> ---
> 
> 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 ecf9b1..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

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

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

[no-reply]

Hi,

This series failed docker-mingw@fedora build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

Type: series
Message-id: 267f42a5-b7ce-379e-ffd1-f2611393d...@web.de
Subject: [Qemu-devel] [PATCH] e1000e: Do not auto-clear ICR bits which aren't 
set in EIAC

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=8
time make docker-test-mingw@fedora
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
665d663f3d e1000e: Do not auto-clear ICR bits which aren't set in EIAC

=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into '/var/tmp/patchew-tester-tmp-9hyebnaz/src/dtc'...
Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42'
  BUILD   fedora
make[1]: Entering directory '/var/tmp/patchew-tester-tmp-9hyebnaz/src'
  GEN 
/var/tmp/patchew-tester-tmp-9hyebnaz/src/docker-src.2018-04-01-17.24.18.15141/qemu.tar
Cloning into 
'/var/tmp/patchew-tester-tmp-9hyebnaz/src/docker-src.2018-04-01-17.24.18.15141/qemu.tar.vroot'...
done.
Checking out files:  13% (841/6066)   
Checking out files:  14% (850/6066)   
Checking out files:  15% (910/6066)   
Checking out files:  16% (971/6066)   
Checking out files:  17% (1032/6066)   
Checking out files:  18% (1092/6066)   
Checking out files:  19% (1153/6066)   
Checking out files:  19% (1172/6066)   
Checking out files:  20% (1214/6066)   
Checking out files:  21% (1274/6066)   
Checking out files:  22% (1335/6066)   
Checking out files:  23% (1396/6066)   
Checking out files:  24% (1456/6066)   
Checking out files:  25% (1517/6066)   
Checking out files:  26% (1578/6066)   
Checking out files:  27% (1638/6066)   
Checking out files:  28% (1699/6066)   
Checking out files:  29% (1760/6066)   
Checking out files:  30% (1820/6066)   
Checking out files:  30% (1860/6066)   
Checking out files:  31% (1881/6066)   
Checking out files:  32% (1942/6066)   
Checking out files:  33% (2002/6066)   
Checking out files:  34% (2063/6066)   
Checking out files:  35% (2124/6066)   
Checking out files:  36% (2184/6066)   
Checking out files:  37% (2245/6066)   
Checking out files:  38% (2306/6066)   
Checking out files:  39% (2366/6066)   
Checking out files:  40% (2427/6066)   
Checking out files:  41% (2488/6066)   
Checking out files:  42% (2548/6066)   
Checking out files:  43% (2609/6066)   
Checking out files:  44% (2670/6066)   
Checking out files:  44% (2693/6066)   
Checking out files:  45% (2730/6066)   
Checking out files:  46% (2791/6066)   
Checking out files:  46% (2815/6066)   
Checking out files:  47% (2852/6066)   
Checking out files:  48% (2912/6066)   
Checking out files:  49% (2973/6066)   
Checking out files:  50% (3033/6066)   
Checking out files:  51% (3094/6066)   
Checking out files:  52% (3155/6066)   
Checking out files:  52% (3203/6066)   
Checking out files:  53% (3215/6066)   
Checking out files:  54% (3276/6066)   
Checking out files:  55% (3337/6066)   
Checking out files:  56% (3397/6066)   
Checking out files:  57% (3458/6066)   
Checking out files:  57% (3463/6066)   
Checking out files:  58% (3519/6066)   
Checking out files:  59% (3579/6066)   
Checking out files:  60% (3640/6066)   
Checking out files:  61% (3701/6066)   
Checking out files:  62% (3761/6066)   
Checking out files:  63% (3822/6066)   
Checking out files:  64% (3883/6066)   
Checking out files:  65% (3943/6066)   
Checking out files:  66% (4004/6066)   
Checking out files:  67% (4065/6066)   
Checking out files:  68% (4125/6066)   
Checking out files:  68% (4142/6066)   
Checking out files:  69% (4186/6066)   
Checking out files:  70% (4247/6066)   
Checking out files:  71% (4307/6066)   
Checking out files:  72% (4368/6066)   
Checking out files:  73% (4429/6066)   
Checking out files:  74% (4489/6066)   
Checking out files:  74% (4507/6066)   
Checking out files:  75% (4550/6066)   
Checking out files:  76% (4611/6066)   
Checking out files:  77% (4671/6066)   
Checking out files:  78% (4732/6066)   
Checking out files:  79% (4793/6066)   
Checking out files:  80% (4853/6066)   
Checking out files:  81% (4914/6066)   
Checking out files:  82% (4975/6066)   
Checking out files:  83% (5035/6066)   
Checking out files:  84% (5096/6066)   
Checking out files:  85% (5157/6066)   
Checking out files:  86% (5217/6066)   
Checking out files:  87% (5278/6066)   
Checking out files:  87% (5326/6066)   
Checking out files:  88% (5339/6066)   
Checking out files:  89% (5399/6066)   
Checking out files:  90% (5460/6066)   
Checking out files:  91% (5521/6066)   
Checking out files:  92% (5581/6066)   
Checking out files:  93% (5642/6066)   
Checking out files:  94% (5703/6066)   
Checking out files:  95% (5763/6066)   
Checking out files:  96% (5824/6066)   
Checking out files:  97% 

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

[no-reply]

Hi,

This series failed docker-build@min-glib build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

Type: series
Message-id: 267f42a5-b7ce-379e-ffd1-f2611393d...@web.de
Subject: [Qemu-devel] [PATCH] e1000e: Do not auto-clear ICR bits which aren't 
set in EIAC

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=8
time make docker-test-build@min-glib
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
665d663f3d e1000e: Do not auto-clear ICR bits which aren't set in EIAC

=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into '/var/tmp/patchew-tester-tmp-0v9s6yo6/src/dtc'...
Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42'
  BUILD   min-glib
make[1]: Entering directory '/var/tmp/patchew-tester-tmp-0v9s6yo6/src'
  GEN 
/var/tmp/patchew-tester-tmp-0v9s6yo6/src/docker-src.2018-04-01-17.32.05.20960/qemu.tar
Cloning into 
'/var/tmp/patchew-tester-tmp-0v9s6yo6/src/docker-src.2018-04-01-17.32.05.20960/qemu.tar.vroot'...
done.
Checking out files:  42% (2568/6066)   
Checking out files:  43% (2609/6066)   
Checking out files:  44% (2670/6066)   
Checking out files:  45% (2730/6066)   
Checking out files:  45% (2751/6066)   
Checking out files:  46% (2791/6066)   
Checking out files:  47% (2852/6066)   
Checking out files:  48% (2912/6066)   
Checking out files:  49% (2973/6066)   
Checking out files:  50% (3033/6066)   
Checking out files:  51% (3094/6066)   
Checking out files:  52% (3155/6066)   
Checking out files:  53% (3215/6066)   
Checking out files:  54% (3276/6066)   
Checking out files:  55% (3337/6066)   
Checking out files:  56% (3397/6066)   
Checking out files:  56% (3430/6066)   
Checking out files:  57% (3458/6066)   
Checking out files:  58% (3519/6066)   
Checking out files:  59% (3579/6066)   
Checking out files:  60% (3640/6066)   
Checking out files:  61% (3701/6066)   
Checking out files:  62% (3761/6066)   
Checking out files:  63% (3822/6066)   
Checking out files:  64% (3883/6066)   
Checking out files:  65% (3943/6066)   
Checking out files:  66% (4004/6066)   
Checking out files:  67% (4065/6066)   
Checking out files:  68% (4125/6066)   
Checking out files:  69% (4186/6066)   
Checking out files:  70% (4247/6066)   
Checking out files:  71% (4307/6066)   
Checking out files:  72% (4368/6066)   
Checking out files:  73% (4429/6066)   
Checking out files:  74% (4489/6066)   
Checking out files:  75% (4550/6066)   
Checking out files:  76% (4611/6066)   
Checking out files:  77% (4671/6066)   
Checking out files:  78% (4732/6066)   
Checking out files:  79% (4793/6066)   
Checking out files:  80% (4853/6066)   
Checking out files:  81% (4914/6066)   
Checking out files:  82% (4975/6066)   
Checking out files:  83% (5035/6066)   
Checking out files:  84% (5096/6066)   
Checking out files:  85% (5157/6066)   
Checking out files:  86% (5217/6066)   
Checking out files:  87% (5278/6066)   
Checking out files:  88% (5339/6066)   
Checking out files:  89% (5399/6066)   
Checking out files:  90% (5460/6066)   
Checking out files:  91% (5521/6066)   
Checking out files:  92% (5581/6066)   
Checking out files:  93% (5642/6066)   
Checking out files:  94% (5703/6066)   
Checking out files:  95% (5763/6066)   
Checking out files:  96% (5824/6066)   
Checking out files:  97% (5885/6066)   
Checking out files:  98% (5945/6066)   
Checking out files:  99% (6006/6066)   
Checking out files: 100% (6066/6066)   
Checking out files: 100% (6066/6066), done.
Your branch is up-to-date with 'origin/test'.
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into 
'/var/tmp/patchew-tester-tmp-0v9s6yo6/src/docker-src.2018-04-01-17.32.05.20960/qemu.tar.vroot/dtc'...
Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42'
Submodule 'ui/keycodemapdb' (git://git.qemu.org/keycodemapdb.git) registered 
for path 'ui/keycodemapdb'
Cloning into 
'/var/tmp/patchew-tester-tmp-0v9s6yo6/src/docker-src.2018-04-01-17.32.05.20960/qemu.tar.vroot/ui/keycodemapdb'...
Submodule path 'ui/keycodemapdb': checked out 
'6b3d716e2b6472eb7189d3220552280ef3d832ce'
tar: 
/var/tmp/patchew-tester-tmp-0v9s6yo6/src/docker-src.2018-04-01-17.32.05.20960/qemu.tar:
 Wrote only 2048 of 10240 bytes
tar: Error is not recoverable: exiting now
failed to create tar file
  COPYRUNNER
RUN test-build in qemu:min-glib 
tar: Unexpected EOF in archive
tar: Unexpected EOF in archive
tar: Error is not recoverable: exiting now
/var/tmp/qemu/run: line 32: prep_fail: command not found
Environment variables:
HOSTNAME=0696834df35f
MAKEFLAGS= -j8
J=8
CCACHE_DIR=/var/tmp/ccache
EXTRA_CONFIGURE_OPTS=
V=
SHOW_ENV=1