Re: pax name_split() error

[Peter Bisroev]

On Sat, Feb 13, 2016 at 2:45 AM, Otto Moerbeek  wrote:
> On Fri, Feb 12, 2016 at 11:48:11PM -0500, Peter Bisroev wrote:
>
> ...
>
> This problem is already being discussed on bugs@ (with a different diff).
> I suggest to send this diff to bugs@ to keep the converation in one place.
> http://marc.info/?t=14549548113=1=2

Hi Otto, thank you for pointing this out, looking at the submission dates I
see why I have missed that post. I saw this issue last week but did not
file a bug report at that time. So when I did send it through there was one
already. Will double check next time to avoid this race condition :). Sorry,
my fault.

The diff on that thread appears to be the same as mine here, except for the
comments. Should I still send it through or just leave it here and keep that
thread clean?

>> Also, there is another tricky issue which still exhibits proper behavior
>> according to the spec:
>>   
>> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/pax.html#tag_20_92_13_06
>> yet it is not very consistent.
>>
>> If there is a directory name which is exactly 155 bytes long, and this
>> directory is being archived, pax will complain that that directory name is 
>> too
>> long, yet it will archive the files underneath that directory assuming they
>> fit into the TNMSZ name limit.
>>
>> In circumstances like these, what would be the most appropriate behavior for
>> pax? Because if that directory is empty, it will not be archived, but if 
>> there
>> is even a single file underneath it pax will complain, but still archive this
>> directory without an error when it is archiving the actual file.
>>
>> Please let me know if I can help further.
>
> This problem is related but different, I'll Cc: guenther to bring this to
> his attention.
>
> -Otto

Thank you Otto!
--peter

diff -e: mishandling of bare dots

[Martin Natano]

When diff encounters a line that consists of a single dot, it emits two
dots instead, stops the current command and emits a substitute command
to replace the double dot with a single one. Then it restarts the
(original) command if necessary and inserts further lines. This is done
because a single dot on a line does have special meaning in ed. (It
stops text insertion.)

However, there are multiple issues with the current implementation,
resulting in mangled output:

- The line number for the substitute command should be the number of the
most recently inserted line. diff instead uses the number of the first
inserted line of the current hunk. The first character of that line is
removed when applying the diff, while the superfluous dot is not.

- The line number of the restarted command is not adjusted for the
number of lines already inserted, resulting in the reordering of lines..

- When there is a bare dot in the replacement text of a change command,
too many lines are deleted, because a second change command is emitted.
An append command should be emitted instead, because the target lines
have already been removed by the first change command.

The following patch fixes all those issues.

Index: diffreg.c
===
RCS file: /cvs/src/usr.bin/diff/diffreg.c,v
retrieving revision 1.90
diff -u -p -r1.90 diffreg.c
--- diffreg.c   26 Oct 2015 12:52:27 -  1.90
+++ diffreg.c   13 Feb 2016 16:35:08 -
@@ -1075,8 +1075,12 @@ proceed:
 * back and restart where we left off.
 */
diff_output(".\n");
-   diff_output("%ds/.//\n", a);
+   diff_output("%ds/.//\n", a + i - 1);
a += i;
+   if (a > b)
+   b += i;
+   else
+   b = a - 1;
c += i;
goto restart;
}

natano

Re: Fix overflow check in sys/netinet6/in6.c

[Stefan Kempf]

Some thoughts about this:

If this particular type of undefined behavior is really a concern: maybe
looking for bounds/overflow checks that are incorrect besides undefined
behavior first is a better approach. A good way of fixing those will
be found, which could then be applied to the "just undefined behavior"
cases.

Details below.

Michael McConville wrote:
> time_second is a time_t, which we define as int64_t. The other operands
> used are of type uint32_t. Therefore, these checks get promoted to
> int64_t and the overflow being tested is undefined because it uses
> signed arithmetic.
> 
> I think that the below diff fixes the overflow check. However, I'm
> pretty tired at the moment and this is hard to do right, so review is
> appreciated.
 
If you know that lt->ia6t_[pv]time will stay an uint32_t forever,
then isn't checking for (time_second > INT64_MAX - lt->ia6t_vltime)
enough? The right side of the comparison will always be > 0 then.
If we somehow had time_second < 0, then your sanity check would still
work without checking for time_second > 0.
Don't think we ever have time_second < 0 though, since it's the
seconds since the Epoch.

In general, I'm not sure if rewriting these checks using *_MAX constants
is the best way to do it, because we'd have to review all of them once the
types of variables involved in the check change.

Clang and newer gcc versions have __builtin_add_overflow, but it's
compiler specific.

One final note, if we really care about that overflow check being
performed in in6_control:
(So this is about the overflow check here itself, not your diff).

The assignments:
ia6->ia6_lifetime.ia6t_expire =
time_second + ia6->ia6_lifetime.ia6t_vltime;
ia6->ia6_lifetime.ia6t_preferred =
time_second + ia6->ia6_lifetime.ia6t_pltime;

are done after the overflow checks. time_second is updated by
hardclock(), which is triggered by the timer interrupt, AFAIK.
If this code runs with interrupts enabled, time_second can yield
a different value for every read access. So the computation might
still overflow in this case.

Practically this should only overflow now when somebody sets the clock
far into the future.

I suppose this check comes from times where time_t was 32 bit.
 
> Index: netinet6/in6.c
> ===
> RCS file: /cvs/src/sys/netinet6/in6.c,v
> retrieving revision 1.183
> diff -u -p -r1.183 in6.c
> --- netinet6/in6.c21 Jan 2016 11:23:48 -  1.183
> +++ netinet6/in6.c12 Feb 2016 19:44:10 -
> @@ -70,6 +70,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -348,11 +349,13 @@ in6_control(struct socket *so, u_long cm
>   /* sanity for overflow - beware unsigned */
>   lt = >ifr_ifru.ifru_lifetime;
>   if (lt->ia6t_vltime != ND6_INFINITE_LIFETIME
> -  && lt->ia6t_vltime + time_second < time_second) {
> + && (time_second > 0
> + && time_second > INT64_MAX - lt->ia6t_vltime)) {
>   return EINVAL;
>   }
>   if (lt->ia6t_pltime != ND6_INFINITE_LIFETIME
> -  && lt->ia6t_pltime + time_second < time_second) {
> + && (time_second > 0
> + && time_second > INT64_MAX - lt->ia6t_pltime)) {
>   return EINVAL;
>   }
>   break;
> 

Revisiting the HDMI2 bug with i915 and docking stations.

[Edd Barrett]

Hey,

(CCing some people who have worked on the i915 code recently)

I've had this bug on my x240t since a while now. I mentioned it a while back on
this list[1], but I didn't look very deep back then.

In short: display port on docking station (HDMI2 in xrandr) doesn't always
detect a monitor. Sometimes you can get it to work by suspending and resuming.

Having found no clues from trying a load of different combinations of docking,
undocking, changing power states, plugging and unplugging monitors etc, I
finally decided to start reading code. I've made some progress, but I've hit a
brick wall. I'm hoping someone can point me in the right direction.

The code responsible for detecting monitors on HDMI ports is
intel_hdmi_detect() in intel_hdmi.c. This calls up into drm_get_edid() in
drm_edid.c. This then uses the i2c bus on the graphics card to query the port.
The i2c bus code is custom for the i915 graphics card: intel_i2c.c.

It seems that the difference between a successfully detected and a
unsuccessfully detected HDMI2 boils down to whether the call to I915_READ() on
line 210 of intel_i2c.c is successful.


   for (retries = 50; retries > 0; retries--) {
st = I915_READ(GMBUS2 + reg_offset);
if (st & (GMBUS_SATOER | GMBUS_HW_RDY))
break;
DELAY(1000);
}
if (st & GMBUS_SATOER) {
bus_err = 1;
goto out;
}


In the case where a monitor is plugged in but X fails to detect it, the call
returns GMBUS_SATOER. In cases where HDMI2 is detected properly, the function
exits without error.

I've gotten this far with printf debugging. The existence of my printf's seems
to have changed the behaviour, so I'm wondering if there is a race some place.
It used to be the case that a zzz/resume cycle would *always* cause HDMI2 to
be detected OK. But since adding my printfs, HDMI2 is not reliably detected
with the zzz/resume cycle any more.

I've not been able to find a description of GMBUS_SATOER anywhere, but I
think this should not happen, right? Hopefully I'm not chasing a red
herring.


[1]: 
http://openbsd-archive.7691.n7.nabble.com/Thinkpad-dock-only-provides-HDMI2-after-first-suspend-td273037.html


dmesg (including sleep/wake cycle) follows:


OpenBSD 5.9 (GENERIC.MP) #1870: Mon Feb  8 17:34:23 MST 2016
dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 16844521472 (16064MB)
avail mem = 16329822208 (15573MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.7 @ 0xdae9c000 (68 entries)
bios0: vendor LENOVO version "GCETA2WW (2.62 )" date 04/09/2015
bios0: LENOVO 3437CTO
acpi0 at bios0: rev 2
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP SLIC TCPA SSDT SSDT SSDT HPET APIC MCFG ECDT FPDT ASF! 
UEFI UEFI POAT SSDT SSDT DMAR UEFI DBG2
acpi0: wakeup devices LID_(S4) SLPB(S3) IGBE(S4) EXP3(S4) XHCI(S3) EHC1(S3) 
EHC2(S3) HDEF(S4)
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpihpet0 at acpi0: 14318179 Hz
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Core(TM) i5-3320M CPU @ 2.60GHz, 2594.57 MHz
cpu0: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,LONG,LAHF,PERF,ITSC,FSGSBASE,SMEP,ERMS,SENSOR,ARAT
cpu0: 256KB 64b/line 8-way L2 cache
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 10 var ranges, 88 fixed ranges
cpu0: apic clock running at 99MHz
cpu0: mwait min=64, max=64, C-substates=0.2.1.1.2, IBE
cpu1 at mainbus0: apid 2 (application processor)
cpu1: Intel(R) Core(TM) i5-3320M CPU @ 2.60GHz, 2594.10 MHz
cpu1: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,LONG,LAHF,PERF,ITSC,FSGSBASE,SMEP,ERMS,SENSOR,ARAT
cpu1: 256KB 64b/line 8-way L2 cache
cpu1: smt 0, core 1, package 0
ioapic0 at mainbus0: apid 2 pa 0xfec0, version 20, 24 pins
acpimcfg0 at acpi0 addr 0xf800, bus 0-63
acpiec0 at acpi0
acpiprt0 at acpi0: bus 0 (PCI0)
acpiprt1 at acpi0: bus -1 (PEG_)
acpiprt2 at acpi0: bus 2 (EXP1)
acpiprt3 at acpi0: bus 3 (EXP2)
acpiprt4 at acpi0: bus 4 (EXP3)
acpicpu0 at acpi0: C2(350@80 mwait.1@0x20), C1(1000@1 mwait.1), PSS
acpicpu1 at acpi0: C2(350@80 mwait.1@0x20), C1(1000@1 mwait.1), PSS
acpipwrres0 at acpi0: PUBS, resource for XHCI, EHC1, EHC2
acpitz0 at acpi0: critical temperature is 103 degC
acpibtn0 at acpi0: LID_
acpibtn1 at acpi0: SLPB
acpibat0 at acpi0: BAT0 model "45N1077" serial 14278 type LION oem "SANYO"
acpibat1 at acpi0: BAT1 not present
acpiac0 at acpi0: 

diff(1) performance

[Edd Barrett]

Hey,

I've not looked into this at all, but looks like diff(1) could be
optimised:

# With GNU diff:
$ time gdiff -u file1 file2 > out-gdiff
gdiff -u file1 file2 > out-gdiff  0.16s user 0.13s system 101% cpu 0.286 total

# With OpenBSD diff:
$ time diff -u file1 file2 > out-bdiff 
diff -u file1 file2 > out-bdiff  1005.24s user 0.33s system 99% cpu 16:46.44 
total

Admittedly the files are big (~10MB each), but still.

Good news is, the outcomes are the same bar the diff header syntax:

---8<---
$ gdiff -u out-bdiff out-gdiff 
--- out-bdiff   2016-02-13 18:15:42.991267362 +
+++ out-gdiff   2016-02-13 17:58:47.153912520 +
@@ -1,5 +1,5 @@
 file1  Wed Feb  4 13:20:14 2015
-+++ file2  Wed Feb  4 13:20:14 2015
+--- file1  2015-02-04 13:20:14.899249552 +
 file2  2015-02-04 13:20:14.859249471 +
 @@ -62,7 +62,7 @@
  TaskState::packetPending: 
  HandlerTask::fn: 
--->8---

Tarred up files: http://theunixzoo.co.uk/random/diff_files.tgz

-- 
Best Regards
Edd Barrett

http://www.theunixzoo.co.uk

Re: OpenSMTPD and mask-source flag.

[Joerg Jung]

On Fri, Feb 12, 2016 at 05:00:59PM -0500, Peter Bisroev wrote:
> > Just in case the previous diff is OK, I am attaching the patch to the
> > smtpd.conf man page.
> 
> Hi Gilles,
> 
> I apologize, my previous manpage diff did not include the information 
> regarding
> the fact that connections through local socket will always be tagged 'local'.
> 
> Please find the corrected manpage diff below.

Some minor comments inline below.
 
> Thank you!
> --peter
> 
> 
> Index: smtpd.conf.5
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
> retrieving revision 1.153
> diff -u -p -r1.153 smtpd.conf.5
> --- smtpd.conf.5  21 Jan 2016 23:40:27 -  1.153
> +++ smtpd.conf.5  12 Feb 2016 21:56:51 -
> @@ -654,6 +654,12 @@ of inflight envelopes falls below
>  Changing the default value might degrade performance.
>  .It Xo
>  .Bk -words
> +.Ic listen on socket
> +.Op Ic filter Ar name
> +.Op Ic mask-source
> +.Ek
> +.br
> +.Bk -words
>  .Ic listen on Ar interface
>  .Op Ar family
>  .Op Ic port Ar port
> @@ -671,11 +677,28 @@ Changing the default value might degrade
>  .Ek
>  .Xc
>  .Pp
> -Specify an
> +Specify a
> +.Ic socket
> +or an
> +.Ar interface
> +to listen on for incoming connections. A

New sentence -> new line, please.

> +.Ic listen on socket
> +directive is used to customize listening behavior for messages submitted
> +through local queues, for example, via the
> +.Xr mail 1
> +utility. This is an optional directive, and if not supplied,

New sentence, new line.

> +.Xr smtpd 8
> +will listen for connections on the
> +.Ic socket
> +only. Clients connecting through the

I do not like the wording "...only." here. Sounds ambiguous. However,
I'm not a native speaker. Also, new sentence, new line.

> +.Ic socket
> +will always be tagged with the 'local'
> +.Ic tag .
> +.Pp
> +To listen on a specific network interface, specify an
>  .Ar interface
> -and
> -.Ar port
> -to listen on.
> +and an optional
> +.Ar port .
>  An interface group, an IP address or a domain name may
>  be used in place of
>  .Ar interface .
> 

Re: diff(1) performance

[Michal Mazurek]

On 18:26:46, 13.02.16, Edd Barrett wrote:
> Hey,
> 
> I've not looked into this at all, but looks like diff(1) could be
> optimised:

It looks like both NetBSD and FreeBSD use GNU diff.

-- 
Michal Mazurek

Re: diff(1) performance

[Todd C. Miller]

GNU diff uses a superior (more modern) algorithm.  Changing that
means rewriting the guts of diff(1).  The GNU diff code includes
references to papers describing the algorithm.

 - todd

Re: OpenSMTPD and mask-source flag.

[Gilles Chehade]

On Fri, Feb 12, 2016 at 04:29:23PM -0500, Peter Bisroev wrote:
> Hi Gilles,
> 

Hi,

> While looking over smtp_enqueue(), I have noticed that setting of
> hostname is a noop. It looks like a leftover code from a bugfix in here
> (http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.sbin/smtpd/smtp.c.diff?r2=1.141=1.140=u)
> 
> I am including a diff to smtp.c below that includes the removal of that code
> (it also includes the changes to smtp.c in my first patch).
>

Yes, it's a leftover from a cleanup to remove the euid from headers,
it's a noop indeed.

I've adapted to -current so it can be applied without your other diff
(which is still pending review) and committed just a minute ago.

Thanks !

-- 
Gilles Chehade

https://www.poolp.org  @poolpOrg

Re: OpenSMTPD and mask-source flag.

[Gilles Chehade]

On Sat, Feb 13, 2016 at 08:32:23PM +0100, Joerg Jung wrote:
> On Fri, Feb 12, 2016 at 05:00:59PM -0500, Peter Bisroev wrote:
> > > Just in case the previous diff is OK, I am attaching the patch to the
> > > smtpd.conf man page.
> > 
> > Hi Gilles,
> > 
> > I apologize, my previous manpage diff did not include the information 
> > regarding
> > the fact that connections through local socket will always be tagged 
> > 'local'.
> > 
> > Please find the corrected manpage diff below.
> 
> Some minor comments inline below.
>  

I commited the man page after taking into consideration jung@'s comment
and rewording a paragraph.

I think jmc@ will find better wording than a german and a french ;-)


> > Index: smtpd.conf.5
> > ===
> > RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
> > retrieving revision 1.153
> > diff -u -p -r1.153 smtpd.conf.5
> > --- smtpd.conf.521 Jan 2016 23:40:27 -  1.153
> > +++ smtpd.conf.512 Feb 2016 21:56:51 -
> > @@ -654,6 +654,12 @@ of inflight envelopes falls below
> >  Changing the default value might degrade performance.
> >  .It Xo
> >  .Bk -words
> > +.Ic listen on socket
> > +.Op Ic filter Ar name
> > +.Op Ic mask-source
> > +.Ek
> > +.br
> > +.Bk -words
> >  .Ic listen on Ar interface
> >  .Op Ar family
> >  .Op Ic port Ar port
> > @@ -671,11 +677,28 @@ Changing the default value might degrade
> >  .Ek
> >  .Xc
> >  .Pp
> > -Specify an
> > +Specify a
> > +.Ic socket
> > +or an
> > +.Ar interface
> > +to listen on for incoming connections. A
> 
> New sentence -> new line, please.
> 
> > +.Ic listen on socket
> > +directive is used to customize listening behavior for messages submitted
> > +through local queues, for example, via the
> > +.Xr mail 1
> > +utility. This is an optional directive, and if not supplied,
> 
> New sentence, new line.
> 
> > +.Xr smtpd 8
> > +will listen for connections on the
> > +.Ic socket
> > +only. Clients connecting through the
> 
> I do not like the wording "...only." here. Sounds ambiguous. However,
> I'm not a native speaker. Also, new sentence, new line.
> 
> > +.Ic socket
> > +will always be tagged with the 'local'
> > +.Ic tag .
> > +.Pp
> > +To listen on a specific network interface, specify an
> >  .Ar interface
> > -and
> > -.Ar port
> > -to listen on.
> > +and an optional
> > +.Ar port .
> >  An interface group, an IP address or a domain name may
> >  be used in place of
> >  .Ar interface .
> > 
> 

-- 
Gilles Chehade

https://www.poolp.org  @poolpOrg

Re: OpenSMTPD and mask-source flag.

[Peter Bisroev]

Thank you Joerg for your comments on the manpage diff. But it looks like
Gilles has already committed the diff according to his previous response.
Thank you Gilles!

Just in case, I am including the updated diff as I reworded it as well.
Gilles, Joerg, could you please see if rewording makes the listen on
directive less ambiguous.

Thank you!
--peter



Index: smtpd.conf.5
===
RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
retrieving revision 1.153
diff -u -p -r1.153 smtpd.conf.5
--- smtpd.conf.521 Jan 2016 23:40:27 -  1.153
+++ smtpd.conf.513 Feb 2016 20:55:33 -
@@ -654,6 +654,12 @@ of inflight envelopes falls below
 Changing the default value might degrade performance.
 .It Xo
 .Bk -words
+.Ic listen on socket
+.Op Ic filter Ar name
+.Op Ic mask-source
+.Ek
+.br
+.Bk -words
 .Ic listen on Ar interface
 .Op Ar family
 .Op Ic port Ar port
@@ -671,11 +677,35 @@ Changing the default value might degrade
 .Ek
 .Xc
 .Pp
-Specify an
+Specify a
+.Ic socket
+or an
+.Ar interface
+to listen on for incoming connections.
+A
+.Ic listen on socket
+directive is used to customize the listening behavior for messages
+submitted through local queues, for example, via the
+.Xr mail 1
+utility.
+This is an optional directive.
+By default,
+.Xr smtpd 8
+will listen for local connections on the
+.Ic socket ,
+.Ic inet4
+and/or
+.Ic inet6
+interfaces will not be enabled.
+Clients connecting through the
+.Ic socket
+will always be tagged with the 'local'
+.Ic tag .
+.Pp
+To listen on a specific network interface, specify an
 .Ar interface
-and
-.Ar port
-to listen on.
+and an optional
+.Ar port .
 An interface group, an IP address or a domain name may
 be used in place of
 .Ar interface .

Re: Intel i219 network card support

[Jonathan Gray]

On Mon, Feb 08, 2016 at 01:53:20PM +0100, Christian Ehrhardt wrote:
> 
> Hi everyone,
> 
> based on the latest release of the Intel driver for FreeBSD (em-7.5.2),
> I've adapted the em(4) driver to support the skylake based i219
> onboard ethernet chips.
> 
> I do not have access to relevant specs, everything below is
> based on reading the Intel code referenced above.
> 
> The diff was initially done for 5.7, bluhm@ did the forward port
> to current. The resulting diff is below.
> 
> The major differences are:
> - The NVRAM access registers are no longer in a separate BAR. Instead
>   they are at offset 0xe000 in the MMIO bar. As a consequence the
>   BAR access registers must be accessed with 32-Bit memory operations.
> - There are several quirks where the i219 chips need special handling.
> 
> The most suspicious one of the quirks is where we need to flush the
> tx desc ring. We use the pointer list of the tx ring as a send buffer
> for a dummy packet that must be sent to flush the tx ring.
> 
> Here's a list of functions that have special handling for the
> SPT based cards in the uptream intel code that I did not port
> for various reasons:
> - The link check code in OpenBSD differs significantly from upstream
>   code. Special cases for SPT based chips in em_check_for_link have
>   been ignored.
> - e1000_check_for_copper_link_ich8lan has been ignored.
> - e1000_suspend_workaroudns_ich8lan has been ignored. Suspend does
>   seem to work on my I219_V chip, though.
> - NVRAM write is not supported for I217 based chips in OpenBSD and
>   the code is missing for I219 chips, too.
> 
> The patch works nicely on my I219_V chip, the other chips are
> untested. I may be able to do tests with I219_LM withhin the next
> few days, though.
> 
> If you own a skylake based onboard NIC, please give this diff a try.
> 
> Who should I ask for oks?
> 
>  regardsChristian

some comments on this inline:

> 
> Index: dev/pci/if_em.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_em.c,v
> retrieving revision 1.329
> diff -u -p -r1.329 if_em.c
> --- dev/pci/if_em.c   12 Jan 2016 00:05:21 -  1.329
> +++ dev/pci/if_em.c   4 Feb 2016 20:33:26 -
> @@ -145,6 +145,10 @@ const struct pci_matchid em_devices[] = 
>   { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_I218_V },
>   { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_I218_V_2 },
>   { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_I218_V_3 },
> + { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_I219_LM },
> + { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_I219_V },
> + { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_I219_LM2 },
> + { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_I219_V2 },
>   { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_82580_COPPER },
>   { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_82580_FIBER },
>   { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_82580_SERDES },
> @@ -253,6 +257,9 @@ int  em_dma_malloc(struct em_softc *, bu
>  void em_dma_free(struct em_softc *, struct em_dma_alloc *);
>  u_int32_t em_fill_descriptors(u_int64_t address, u_int32_t length,
> PDESC_ARRAY desc_array);
> +void em_flush_tx_ring(struct em_softc *);
> +void em_flush_rx_ring(struct em_softc *);
> +void em_flush_desc_rings(struct em_softc *);
>  
>  /*
>   *  OpenBSD Device Interface Entry Points
> @@ -435,6 +442,7 @@ em_attach(struct device *parent, struct 
>   case em_ich10lan:
>   case em_pch2lan:
>   case em_pch_lpt:
> + case em_pch_spt:
>   case em_80003es2lan:
>   /* 9K Jumbo Frame size */
>   sc->hw.max_frame_size = 9234;
> @@ -844,6 +852,7 @@ em_init(void *arg)
>   case em_pchlan:
>   case em_pch2lan:
>   case em_pch_lpt:
> + case em_pch_spt:
>   pba = E1000_PBA_26K;
>   break;
>   default:
> @@ -1506,10 +1515,12 @@ em_stop(void *arg, int softonly)
>   timeout_del(>timer_handle);
>   timeout_del(>tx_fifo_timer_handle);
>  
> - if (!softonly) {
> + if (!softonly)
>   em_disable_intr(sc);
> + if (sc->hw.mac_type == em_pch_spt)
> + em_flush_desc_rings(sc);
> + if (!softonly)
>   em_reset_hw(>hw);
> - }
>  
>   intr_barrier(sc->sc_intrhand);
>   ifq_barrier(>if_snd);
> @@ -1563,6 +1574,27 @@ em_identify_hardware(struct em_softc *sc
>   sc->hw.phy_init_script = TRUE;
>  }
>  
> +void
> +em_legacy_irq_quirk_spt(struct em_softc *sc)
> +{
> + uint32_treg;
> +
> + /* Legacy interrupt: SPT needs a quirk. */
> + if (sc->hw.mac_type != em_pch_spt)
> + return;
> + if (sc->legacy_irq == 0)
> + return;
> +
> + reg = EM_READ_REG(>hw, E1000_FEXTNVM7);
> + reg |= E1000_FEXTNVM7_SIDE_CLK_UNGATE;
> + EM_WRITE_REG(>hw, E1000_FEXTNVM7, reg);
> +
> + reg = 

Re: "Abort trap" when pledge()d and compiled with -pg

[Philip Guenther]

On Sat, Feb 13, 2016 at 12:31 PM, Michal Mazurek  wrote:
> When compiling a program that calls pledge(2) with "-pg" the resulting
> binary will execute seemingly fine, but at the very end die with:
> Abort trap (core dumped)
> I think the problem lies in a call to profil(2).
>
> Is this a bug or a feature?

Seems like a bug.  _mcleanup() is invoked via the atexit() in gcrt0.o
(c.f. lib/csu/crt0.c)

Fixing things so this works will require at least two things:
 1) pledge will need to always permit profil(NULL,0,0,0) for the
moncontrol(0) performed by _mcleanup()
 2) pledge will need to permit opening and writing to "gmon.out"
(ignore the $PROFDIR stuff) if and only if profil() had been used.

(The fallback code to use setitimer() if sysctl() fails seems
pointless: when would that fail and setitimer() succeed?)


Philip Guenther

Re: "Abort trap" when pledge()d and compiled with -pg

[Sebastien Marie]

On Sat, Feb 13, 2016 at 09:10:30PM -0800, Philip Guenther wrote:
> On Sat, Feb 13, 2016 at 12:31 PM, Michal Mazurek  wrote:
> > When compiling a program that calls pledge(2) with "-pg" the resulting
> > binary will execute seemingly fine, but at the very end die with:
> > Abort trap (core dumped)
> > I think the problem lies in a call to profil(2).
> >
> > Is this a bug or a feature?
> 
> Seems like a bug.  _mcleanup() is invoked via the atexit() in gcrt0.o
> (c.f. lib/csu/crt0.c)
 
I would said "feature" instead of bug :)

In fact, I don't think a pledged program should not be profiled...
Profiling is for developpment code, and pledge is more for
production-code.

If profiling is needed, pledge(2) should be disabled:

1. by commenting the pledge(2) call

2. by adding `#define pledge(pr,pa) 0' after unistd.h include

3. by passing -D'pledge(pr,pa)=_nopledge' as compiler option (but I
am unsure if it makes a use of uninitialized variable or if compiler
initialize it to 0 alone).


Eventually is it acceptable to provide an unistd.h that mask pledge(2)
(and issue a #warning) when compiling with -pg ?


> Fixing things so this works will require at least two things:
>  1) pledge will need to always permit profil(NULL,0,0,0) for the
> moncontrol(0) performed by _mcleanup()
>  2) pledge will need to permit opening and writing to "gmon.out"
> (ignore the $PROFDIR stuff) if and only if profil() had been used.
> 
> (The fallback code to use setitimer() if sysctl() fails seems
> pointless: when would that fail and setitimer() succeed?)

For a having a profiled program pledged it would need parts of:
  - "stdio" : issetugid(), getpid(), write(), close(), munmap()
  - "cpath" : open(O_CREAT)
and profil(2)

It is doable if we require at least "stdio" for profiling to work. else
it is too intrusive (require all previous syscall to be declared
PLEDGE_ALWAYS in `pledge_syscalls' array, and having a `pledge_profil'
call in each of them).


Additionnally, the fact to ignore $PROFDIR stuff would be more complex:
userland has no way to know the running program is pledged or not.


-- 
Sebastien Marie