From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52880) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fSvXL-0001TZ-GS for qemu-devel@nongnu.org; Tue, 12 Jun 2018 22:29:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fSvXH-0002o5-89 for qemu-devel@nongnu.org; Tue, 12 Jun 2018 22:29:27 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:57050 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fSvXH-0002nr-1e for qemu-devel@nongnu.org; Tue, 12 Jun 2018 22:29:23 -0400 References: <267f42a5-b7ce-379e-ffd1-f2611393d2ff@web.de> <6730e270-9906-a43c-68b0-7a09a0743fa5@siemens.com> <7bb24572-0fc1-00de-3552-aca1111059ed@amsat.org> <63ee188e-0c66-ce7e-03b2-fd58e4da9116@siemens.com> <3a128f8d-46a3-591f-ee09-8c9a3f2401ea@amsat.org> <3608e071-f66b-9ee5-3896-8562daa9b483@redhat.com> <49ec5839-bcaa-39d1-e815-377bcf7cdd6b@amsat.org> From: Jason Wang Message-ID: <6eeac4b0-7a10-de17-1fdd-8d4417934623@redhat.com> Date: Wed, 13 Jun 2018 10:29:16 +0800 MIME-Version: 1.0 In-Reply-To: <49ec5839-bcaa-39d1-e815-377bcf7cdd6b@amsat.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] e1000e: Do not auto-clear ICR bits which aren't set in EIAC List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , Jan Kiszka , Dmitry Fleytman Cc: Peter Maydell , qemu-devel , Alexander Graf On 2018=E5=B9=B406=E6=9C=8813=E6=97=A5 10:26, Philippe Mathieu-Daud=C3=A9= wrote: > Hi Jason, > > On 06/12/2018 11:18 PM, Jason Wang wrote: >> On 2018=E5=B9=B406=E6=9C=8813=E6=97=A5 03:00, Philippe Mathieu-Daud=C3= =A9 wrote: >>> Cc'ing Jason who is also listed as co-maintainer: >>> >>> =C2=A0=C2=A0 ./scripts/get_maintainer.pl -f hw/net/e1000e_core.c >>> =C2=A0=C2=A0 Dmitry Fleytman (maintainer= :e1000e) >>> =C2=A0=C2=A0 Jason Wang (odd fixer:Network dev= ices) >>> >>> On 06/12/2018 03:43 PM, Jan Kiszka wrote: >>>> On 2018-06-12 20:38, Philippe Mathieu-Daud=C3=A9 wrote: >>>>> On 06/12/2018 03:30 PM, Jan Kiszka wrote: >>>>>> On 2018-06-12 20:11, Philippe Mathieu-Daud=C3=A9 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"). >>>>>>>>>>>> >>>>>>>>>>>> =C2=A0 hw/net/e1000e_core.c | 4 ---- >>>>>>>>>>>> =C2=A0 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) >>>>>>>>>>>> >>>>>>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 effective_eiac =3D core->mac= [EIAC] & cause; >>>>>>>>>>>> >>>>>>>>>>>> -=C2=A0=C2=A0=C2=A0 if (effective_eiac =3D=3D E1000_ICR_OTHE= R) { >>>>>>>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 effective_eiac |= =3D E1000_ICR_OTHER_CAUSES; >>>>>>>>>>>> -=C2=A0=C2=A0=C2=A0 } >>>>>>>>>>>> - >>>>>>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 core->mac[ICR] &=3D ~effecti= ve_eiac; >>>>>>>>>>>> >>>>>>>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 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=3D0" >>>>>>>> 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) >>>>>>> >>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 effective_eiac =3D core->mac[EIAC= ] & cause; >>>>>>> >>>>>>> -=C2=A0=C2=A0=C2=A0 if (effective_eiac =3D=3D E1000_ICR_OTHER) { >>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 effective_eiac |=3D E= 1000_ICR_OTHER_CAUSES; >>>>>>> -=C2=A0=C2=A0=C2=A0 } >>>>>>> - >>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 core->mac[ICR] &=3D ~effective_ei= ac; >>>>>>> >>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!(core->mac[CTRL_EXT] & E1000= _CTRL_EXT_IAME)) { >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (effective_eiac =3D= =3D E1000_ICR_OTHER) { >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= effective_eiac |=3D E1000_ICR_OTHER_CAUSES; >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >>>>>>> + >>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 core->mac= [IMS] &=3D ~effective_eiac; >>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >>>>>>> =C2=A0 } >>>>>>> >>>>>> 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. >>> =C2=A0=C2=A0 Interrupt Cause Registers >>> >>> =C2=A0=C2=A0=C2=A0=C2=A0 Other bits in this register are the legacy = indication of >>> =C2=A0=C2=A0=C2=A0=C2=A0 interrupts as the MDIC complete, management= and link status >>> =C2=A0=C2=A0=C2=A0=C2=A0 change. There is a specific Other Cause bit= that is set if >>> =C2=A0=C2=A0=C2=A0=C2=A0 one of these bits are set, this bit can be = mapped to a >>> =C2=A0=C2=A0=C2=A0=C2=A0 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 pat= ch. >>> >>> 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=20 have come comments. Thanks