* Re: [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if TIMINCA is zero
@ 2016-05-06 23:43 ` Rustad, Mark D
0 siblings, 0 replies; 11+ messages in thread
From: Rustad, Mark D @ 2016-05-06 23:43 UTC (permalink / raw)
To: Denys Vlasenko
Cc: Kirsher, Jeffrey T, intel-wired-lan@lists.osuosl.org, LKML,
netdev@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 2189 bytes --]
Denys Vlasenko <dvlasenk@redhat.com> wrote:
> Users report that under VMWare, er32(TIMINCA) returns zero.
> This causes division by zero at init time as follows:
>
> ==> incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
> for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
> /* latch SYSTIMH on read of SYSTIML */
> systim_next = (cycle_t)er32(SYSTIML);
> systim_next |= (cycle_t)er32(SYSTIMH) << 32;
>
> time_delta = systim_next - systim;
> temp = time_delta;
> ====> rem = do_div(temp, incvalue);
>
> This change makes kernel survive this, and users report that
> NIC does work after this change.
>
> Since on real hardware incvalue is never zero, this should not affect
> real hardware use case.
>
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> CC: "Ruinskiy, Dima" <dima.ruinskiy@intel.com>
> CC: intel-wired-lan@lists.osuosl.org
> CC: netdev@vger.kernel.org
> CC: LKML <linux-kernel@vger.kernel.org>
> ---
> drivers/net/ethernet/intel/e1000e/netdev.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c
> b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 269087c..0626935 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -4315,7 +4315,8 @@ static cycle_t e1000e_cyclecounter_read(const
> struct cyclecounter *cc)
>
> time_delta = systim_next - systim;
> temp = time_delta;
> - rem = do_div(temp, incvalue);
> + /* VMWare users have seen incvalue of zero, don't div / 0 */
> + rem = incvalue ? do_div(temp, incvalue) : (time_delta != 0);
>
> systim = systim_next;
>
I seem to recall that this was rejected before because it really is
VMWare's bug and, if they fix it, any existing VMs that use this will just
work. Changing the driver will only fix it for vms that install a new
driver. I don't object to doing it, it just seems like not the most
effective place to address the issue.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if TIMINCA is zero
@ 2016-05-06 23:43 ` Rustad, Mark D
0 siblings, 0 replies; 11+ messages in thread
From: Rustad, Mark D @ 2016-05-06 23:43 UTC (permalink / raw)
To: Denys Vlasenko
Cc: Kirsher, Jeffrey T, intel-wired-lan@lists.osuosl.org, LKML,
netdev@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 2246 bytes --]
Denys Vlasenko <dvlasenk@redhat.com> wrote:
> Users report that under VMWare, er32(TIMINCA) returns zero.
> This causes division by zero at init time as follows:
>
> ==> incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
> for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
> /* latch SYSTIMH on read of SYSTIML */
> systim_next = (cycle_t)er32(SYSTIML);
> systim_next |= (cycle_t)er32(SYSTIMH) << 32;
>
> time_delta = systim_next - systim;
> temp = time_delta;
> ====> rem = do_div(temp, incvalue);
>
> This change makes kernel survive this, and users report that
> NIC does work after this change.
>
> Since on real hardware incvalue is never zero, this should not affect
> real hardware use case.
>
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> CC: "Ruinskiy, Dima" <dima.ruinskiy@intel.com>
> CC: intel-wired-lan@lists.osuosl.org
> CC: netdev@vger.kernel.org
> CC: LKML <linux-kernel@vger.kernel.org>
> ---
> drivers/net/ethernet/intel/e1000e/netdev.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c
> b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 269087c..0626935 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -4315,7 +4315,8 @@ static cycle_t e1000e_cyclecounter_read(const
> struct cyclecounter *cc)
>
> time_delta = systim_next - systim;
> temp = time_delta;
> - rem = do_div(temp, incvalue);
> + /* VMWare users have seen incvalue of zero, don't div / 0 */
> + rem = incvalue ? do_div(temp, incvalue) : (time_delta != 0);
>
> systim = systim_next;
>
I seem to recall that this was rejected before because it really is
VMWare's bug and, if they fix it, any existing VMs that use this will just
work. Changing the driver will only fix it for vms that install a new
driver. I don't object to doing it, it just seems like not the most
effective place to address the issue.
--
Mark Rustad, Networking Division, Intel Corporation
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread* [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if TIMINCA is zero
2016-05-06 23:43 ` Rustad, Mark D
@ 2016-05-10 21:00 ` Jarod Wilson
-1 siblings, 0 replies; 11+ messages in thread
From: Jarod Wilson @ 2016-05-10 21:00 UTC (permalink / raw)
To: intel-wired-lan
On Fri, May 06, 2016 at 11:43:17PM +0000, Rustad, Mark D wrote:
> Denys Vlasenko <dvlasenk@redhat.com> wrote:
>
> >Users report that under VMWare, er32(TIMINCA) returns zero.
> >This causes division by zero at init time as follows:
> >
> > ==> incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
> > for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
> > /* latch SYSTIMH on read of SYSTIML */
> > systim_next = (cycle_t)er32(SYSTIML);
> > systim_next |= (cycle_t)er32(SYSTIMH) << 32;
> >
> > time_delta = systim_next - systim;
> > temp = time_delta;
> > ====> rem = do_div(temp, incvalue);
> >
> >This change makes kernel survive this, and users report that
> >NIC does work after this change.
> >
> >Since on real hardware incvalue is never zero, this should not affect
> >real hardware use case.
...
> I seem to recall that this was rejected before because it really is VMWare's
> bug and, if they fix it, any existing VMs that use this will just work.
> Changing the driver will only fix it for vms that install a new driver. I
> don't object to doing it, it just seems like not the most effective place to
> address the issue.
You could also have people who never update VMWare, for whom a kernel
work-around would be better. I think it'd be best to address it both at
the driver level and the emulated hardware level, to improve things for
the most possible users. Those who update neither hypervisor or
kernel/driver, well, they reap what they sow.
--
Jarod Wilson
jarod at redhat.com
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if TIMINCA is zero
@ 2016-05-10 21:00 ` Jarod Wilson
0 siblings, 0 replies; 11+ messages in thread
From: Jarod Wilson @ 2016-05-10 21:00 UTC (permalink / raw)
To: Rustad, Mark D
Cc: Denys Vlasenko, netdev@vger.kernel.org,
intel-wired-lan@lists.osuosl.org, LKML
On Fri, May 06, 2016 at 11:43:17PM +0000, Rustad, Mark D wrote:
> Denys Vlasenko <dvlasenk@redhat.com> wrote:
>
> >Users report that under VMWare, er32(TIMINCA) returns zero.
> >This causes division by zero at init time as follows:
> >
> > ==> incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
> > for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
> > /* latch SYSTIMH on read of SYSTIML */
> > systim_next = (cycle_t)er32(SYSTIML);
> > systim_next |= (cycle_t)er32(SYSTIMH) << 32;
> >
> > time_delta = systim_next - systim;
> > temp = time_delta;
> > ====> rem = do_div(temp, incvalue);
> >
> >This change makes kernel survive this, and users report that
> >NIC does work after this change.
> >
> >Since on real hardware incvalue is never zero, this should not affect
> >real hardware use case.
...
> I seem to recall that this was rejected before because it really is VMWare's
> bug and, if they fix it, any existing VMs that use this will just work.
> Changing the driver will only fix it for vms that install a new driver. I
> don't object to doing it, it just seems like not the most effective place to
> address the issue.
You could also have people who never update VMWare, for whom a kernel
work-around would be better. I think it'd be best to address it both at
the driver level and the emulated hardware level, to improve things for
the most possible users. Those who update neither hypervisor or
kernel/driver, well, they reap what they sow.
--
Jarod Wilson
jarod@redhat.com
^ permalink raw reply [flat|nested] 11+ messages in thread* [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if TIMINCA is zero
2016-05-10 21:00 ` Jarod Wilson
@ 2016-05-11 3:59 ` Mark D Rustad
-1 siblings, 0 replies; 11+ messages in thread
From: Mark D Rustad @ 2016-05-11 3:59 UTC (permalink / raw)
To: intel-wired-lan
Jarod Wilson <jarod@redhat.com> wrote:
> On Fri, May 06, 2016 at 11:43:17PM +0000, Rustad, Mark D wrote:
>> Denys Vlasenko <dvlasenk@redhat.com> wrote:
>>
>>> Users report that under VMWare, er32(TIMINCA) returns zero.
>>> This causes division by zero at init time as follows:
>>>
>>> ==> incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
>>> for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
>>> /* latch SYSTIMH on read of SYSTIML */
>>> systim_next = (cycle_t)er32(SYSTIML);
>>> systim_next |= (cycle_t)er32(SYSTIMH) << 32;
>>>
>>> time_delta = systim_next - systim;
>>> temp = time_delta;
>>> ====> rem = do_div(temp, incvalue);
>>>
>>> This change makes kernel survive this, and users report that
>>> NIC does work after this change.
>>>
>>> Since on real hardware incvalue is never zero, this should not affect
>>> real hardware use case.
> ...
>> I seem to recall that this was rejected before because it really is
>> VMWare's
>> bug and, if they fix it, any existing VMs that use this will just work.
>> Changing the driver will only fix it for vms that install a new driver. I
>> don't object to doing it, it just seems like not the most effective
>> place to
>> address the issue.
>
> You could also have people who never update VMWare, for whom a kernel
> work-around would be better. I think it'd be best to address it both at
> the driver level and the emulated hardware level, to improve things for
> the most possible users. Those who update neither hypervisor or
> kernel/driver, well, they reap what they sow.
That is a sound argument for doing both. I would expect that there are more
frozen VM images than host environments, but I can certainly imagine that
some choose to freeze their host. Of course if everything is frozen there
is no point at all. :-)
I am on an extended vacation, and don't work on e1000e anyway, so I will
quit my kibitzing here.
--
Mark Rustad, MRustad at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 841 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20160510/ea7d2a34/attachment.asc>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Intel-wired-lan] [PATCH] e1000e: prevent division by zero if TIMINCA is zero
@ 2016-05-11 3:59 ` Mark D Rustad
0 siblings, 0 replies; 11+ messages in thread
From: Mark D Rustad @ 2016-05-11 3:59 UTC (permalink / raw)
To: Jarod Wilson
Cc: Mark Rustad, Denys Vlasenko, netdev@vger.kernel.org,
intel-wired-lan@lists.osuosl.org, LKML
[-- Attachment #1: Type: text/plain, Size: 2091 bytes --]
Jarod Wilson <jarod@redhat.com> wrote:
> On Fri, May 06, 2016 at 11:43:17PM +0000, Rustad, Mark D wrote:
>> Denys Vlasenko <dvlasenk@redhat.com> wrote:
>>
>>> Users report that under VMWare, er32(TIMINCA) returns zero.
>>> This causes division by zero at init time as follows:
>>>
>>> ==> incvalue = er32(TIMINCA) & E1000_TIMINCA_INCVALUE_MASK;
>>> for (i = 0; i < E1000_MAX_82574_SYSTIM_REREADS; i++) {
>>> /* latch SYSTIMH on read of SYSTIML */
>>> systim_next = (cycle_t)er32(SYSTIML);
>>> systim_next |= (cycle_t)er32(SYSTIMH) << 32;
>>>
>>> time_delta = systim_next - systim;
>>> temp = time_delta;
>>> ====> rem = do_div(temp, incvalue);
>>>
>>> This change makes kernel survive this, and users report that
>>> NIC does work after this change.
>>>
>>> Since on real hardware incvalue is never zero, this should not affect
>>> real hardware use case.
> ...
>> I seem to recall that this was rejected before because it really is
>> VMWare's
>> bug and, if they fix it, any existing VMs that use this will just work.
>> Changing the driver will only fix it for vms that install a new driver. I
>> don't object to doing it, it just seems like not the most effective
>> place to
>> address the issue.
>
> You could also have people who never update VMWare, for whom a kernel
> work-around would be better. I think it'd be best to address it both at
> the driver level and the emulated hardware level, to improve things for
> the most possible users. Those who update neither hypervisor or
> kernel/driver, well, they reap what they sow.
That is a sound argument for doing both. I would expect that there are more
frozen VM images than host environments, but I can certainly imagine that
some choose to freeze their host. Of course if everything is frozen there
is no point at all. :-)
I am on an extended vacation, and don't work on e1000e anyway, so I will
quit my kibitzing here.
--
Mark Rustad, MRustad@gmail.com
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread