Re: [patch] azalia: Intel 300 Series HD Audio

2020-06-08 Thread Jonathan Gray
On Mon, Jun 08, 2020 at 09:14:36PM -0600, bobby wrote:
> On Sun, May 31, 2020 at 12:38:55PM +0200, Bruno Flueckiger wrote:
> > On 31.05., Benjamin Baier wrote:
> > > On Fri, 29 May 2020 11:25:44 +0200
> > > Bruno Flueckiger  wrote:
> > >
> > > > Hi,
> > > >
> > > > My brand new laptop HP EliteBook 850 G6 comes with an Intel 300 Series
> > > > HD Audio device rev 0x11. The device shows up as not configured in the
> > > > dmesg. The PCI config space of the device identifies its subclass as
> > > > PCI_SUBCLASS_MULTIMEDIA_AUDIO instead of PCI_SUBCLASS_MULTIMEDIA_HDAUDIO
> > > >
> > > > The patch below makes the device work just fine on my laptop.
> > > >
> > > > Cheers,
> > > > Bruno
> > > >
> > > > Index: sys/dev/pci/azalia.c
> > > > ===
> > > > RCS file: /cvs/src/sys/dev/pci/azalia.c,v
> > > > retrieving revision 1.255
> > > > diff -u -p -r1.255 azalia.c
> > > > --- sys/dev/pci/azalia.c18 Apr 2020 21:55:56 -  1.255
> > > > +++ sys/dev/pci/azalia.c28 May 2020 13:48:10 -
> > > > @@ -481,7 +481,8 @@ azalia_pci_match(struct device *parent,
> > > >
> > > > pa = aux;
> > > > if (PCI_CLASS(pa->pa_class) == PCI_CLASS_MULTIMEDIA
> > > > -   && PCI_SUBCLASS(pa->pa_class) == 
> > > > PCI_SUBCLASS_MULTIMEDIA_HDAUDIO)
> > > > +   && (PCI_SUBCLASS(pa->pa_class) == 
> > > > PCI_SUBCLASS_MULTIMEDIA_HDAUDIO
> > > > +   || PCI_SUBCLASS(pa->pa_class) == 
> > > > PCI_SUBCLASS_MULTIMEDIA_AUDIO))
> > > > return 1;
> > > > return 0;
> > > >  }
> > > >
> > >
> > > Hi.
> > >
> > > Does your Laptop run with the latest BIOS? There was one released on May 
> > > 11th.
> > > https://support.hp.com/de-de/drivers/selfservice/swdetails/hp-elitebook-850-g6-notebook-pc/26609805/swItemId/ob-251060-1
> > > The release notes state: Fixes an issue where the audio on the system 
> > > stops functioning after the Intel Active Management Technology (AMT) 
> > > option is disabled.
> > >
> > > The azalia patch is in but I would prefer HP to fix their BIOS instead.
> > >
> > > Greetings Ben
> > >
> > 
> > Hi Ben,
> > 
> > I've upgraded the firmware to the latest available release, but the
> > audio device still reports with the wrong subclass in its PCI config
> > space.
> > 
> > I agree that HP should rather fix their firmware. I still try to find
> > out how to report this problem to HP in a way that doesn't get ignored.
> > 
> > Cheers,
> > Bruno
> > 
> 
> My laptop, Lenovo C930, needs the same matching to attach azalia, I have the 
> newest bios fyi.  Patch and pcidump below.

thanks, committed with order sorted

> 
> 
> Index: sys/dev/pci/azalia.c
> ===
> RCS file: /cvs/src/sys/dev/pci/azalia.c,v
> retrieving revision 1.256
> diff -u -p -u -p -r1.256 azalia.c
> --- sys/dev/pci/azalia.c  31 May 2020 04:58:38 -  1.256
> +++ sys/dev/pci/azalia.c  9 Jun 2020 02:59:36 -
> @@ -475,7 +475,8 @@ azalia_configure_pci(azalia_t *az)
>  }
>  
>  const struct pci_matchid azalia_pci_devices[] = {
> - { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_300SERIES_U_HDA }
> + { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_300SERIES_U_HDA },
> + { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_200SERIES_U_HDA }
>  };
>  
>  int
> 
> 
> 
> Domain /dev/pci0:
>  0:0:0: Intel Core 8G Host
>   0x: Vendor ID: 8086, Product ID: 5914
>   0x0004: Command: 0006, Status: 2090
>   0x0008: Class: 06 Bridge, Subclass: 00 Host,
>   Interface: 00, Revision: 08
>   0x000c: BIST: 00, Header Type: 00, Latency Timer: 00,
>   Cache Line Size: 00
>   0x0010: BAR empty ()
>   0x0014: BAR empty ()
>   0x0018: BAR empty ()
>   0x001c: BAR empty ()
>   0x0020: BAR empty ()
>   0x0024: BAR empty ()
>   0x0028: Cardbus CIS: 
>   0x002c: Subsystem Vendor ID: 17aa Product ID: 3821
>   0x0030: Expansion ROM Base Address: 
>   0x0038: 
>   0x003c: Interrupt Pin: 00 Line: 00 Min Gnt: 00 Max Lat: 00
>   0x00e0: Capability 0x09: Vendor Specific
>  0:2:0: Intel UHD Graphics 620
>   0x: Vendor ID: 8086, Product ID: 5917
>   0x0004: Command: 0007, Status: 0010
>   0x0008: Class: 03 Display, Subclass: 00 VGA,
>   Interface: 00, Revision: 07
>   0x000c: BIST: 00, Header Type: 00, Latency Timer: 00,
>   Cache Line Size: 10
>   0x0010: BAR mem 64bit addr: 0x002ffa00/0x0100
>   0x0018: BAR mem prefetchable 64bit addr: 0x002fa000/0x1000
>   0x0020: BAR io addr: 0x3000/0x0040
>   0x0024: BAR empty ()
>   0x0028: Cardbus CIS: 
>   0x002c: Subsystem Vendor ID: 17aa Product ID: 3803
>   0x0030: Expansion ROM Base Address: 
>   0x0038: 
>   0x003c: Interrupt Pin: 01 Line: ff Min Gnt: 00 Max Lat: 00

Re: [patch] azalia: Intel 300 Series HD Audio

2020-06-08 Thread bobby
On Sun, May 31, 2020 at 12:38:55PM +0200, Bruno Flueckiger wrote:
> On 31.05., Benjamin Baier wrote:
> > On Fri, 29 May 2020 11:25:44 +0200
> > Bruno Flueckiger  wrote:
> >
> > > Hi,
> > >
> > > My brand new laptop HP EliteBook 850 G6 comes with an Intel 300 Series
> > > HD Audio device rev 0x11. The device shows up as not configured in the
> > > dmesg. The PCI config space of the device identifies its subclass as
> > > PCI_SUBCLASS_MULTIMEDIA_AUDIO instead of PCI_SUBCLASS_MULTIMEDIA_HDAUDIO
> > >
> > > The patch below makes the device work just fine on my laptop.
> > >
> > > Cheers,
> > > Bruno
> > >
> > > Index: sys/dev/pci/azalia.c
> > > ===
> > > RCS file: /cvs/src/sys/dev/pci/azalia.c,v
> > > retrieving revision 1.255
> > > diff -u -p -r1.255 azalia.c
> > > --- sys/dev/pci/azalia.c  18 Apr 2020 21:55:56 -  1.255
> > > +++ sys/dev/pci/azalia.c  28 May 2020 13:48:10 -
> > > @@ -481,7 +481,8 @@ azalia_pci_match(struct device *parent,
> > >
> > >   pa = aux;
> > >   if (PCI_CLASS(pa->pa_class) == PCI_CLASS_MULTIMEDIA
> > > - && PCI_SUBCLASS(pa->pa_class) == PCI_SUBCLASS_MULTIMEDIA_HDAUDIO)
> > > + && (PCI_SUBCLASS(pa->pa_class) == PCI_SUBCLASS_MULTIMEDIA_HDAUDIO
> > > + || PCI_SUBCLASS(pa->pa_class) == PCI_SUBCLASS_MULTIMEDIA_AUDIO))
> > >   return 1;
> > >   return 0;
> > >  }
> > >
> >
> > Hi.
> >
> > Does your Laptop run with the latest BIOS? There was one released on May 
> > 11th.
> > https://support.hp.com/de-de/drivers/selfservice/swdetails/hp-elitebook-850-g6-notebook-pc/26609805/swItemId/ob-251060-1
> > The release notes state: Fixes an issue where the audio on the system stops 
> > functioning after the Intel Active Management Technology (AMT) option is 
> > disabled.
> >
> > The azalia patch is in but I would prefer HP to fix their BIOS instead.
> >
> > Greetings Ben
> >
> 
> Hi Ben,
> 
> I've upgraded the firmware to the latest available release, but the
> audio device still reports with the wrong subclass in its PCI config
> space.
> 
> I agree that HP should rather fix their firmware. I still try to find
> out how to report this problem to HP in a way that doesn't get ignored.
> 
> Cheers,
> Bruno
> 

My laptop, Lenovo C930, needs the same matching to attach azalia, I have the 
newest bios fyi.  Patch and pcidump below.


Index: sys/dev/pci/azalia.c
===
RCS file: /cvs/src/sys/dev/pci/azalia.c,v
retrieving revision 1.256
diff -u -p -u -p -r1.256 azalia.c
--- sys/dev/pci/azalia.c31 May 2020 04:58:38 -  1.256
+++ sys/dev/pci/azalia.c9 Jun 2020 02:59:36 -
@@ -475,7 +475,8 @@ azalia_configure_pci(azalia_t *az)
 }
 
 const struct pci_matchid azalia_pci_devices[] = {
-   { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_300SERIES_U_HDA }
+   { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_300SERIES_U_HDA },
+   { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_200SERIES_U_HDA }
 };
 
 int



Domain /dev/pci0:
 0:0:0: Intel Core 8G Host
0x: Vendor ID: 8086, Product ID: 5914
0x0004: Command: 0006, Status: 2090
0x0008: Class: 06 Bridge, Subclass: 00 Host,
Interface: 00, Revision: 08
0x000c: BIST: 00, Header Type: 00, Latency Timer: 00,
Cache Line Size: 00
0x0010: BAR empty ()
0x0014: BAR empty ()
0x0018: BAR empty ()
0x001c: BAR empty ()
0x0020: BAR empty ()
0x0024: BAR empty ()
0x0028: Cardbus CIS: 
0x002c: Subsystem Vendor ID: 17aa Product ID: 3821
0x0030: Expansion ROM Base Address: 
0x0038: 
0x003c: Interrupt Pin: 00 Line: 00 Min Gnt: 00 Max Lat: 00
0x00e0: Capability 0x09: Vendor Specific
 0:2:0: Intel UHD Graphics 620
0x: Vendor ID: 8086, Product ID: 5917
0x0004: Command: 0007, Status: 0010
0x0008: Class: 03 Display, Subclass: 00 VGA,
Interface: 00, Revision: 07
0x000c: BIST: 00, Header Type: 00, Latency Timer: 00,
Cache Line Size: 10
0x0010: BAR mem 64bit addr: 0x002ffa00/0x0100
0x0018: BAR mem prefetchable 64bit addr: 0x002fa000/0x1000
0x0020: BAR io addr: 0x3000/0x0040
0x0024: BAR empty ()
0x0028: Cardbus CIS: 
0x002c: Subsystem Vendor ID: 17aa Product ID: 3803
0x0030: Expansion ROM Base Address: 
0x0038: 
0x003c: Interrupt Pin: 01 Line: ff Min Gnt: 00 Max Lat: 00
0x0040: Capability 0x09: Vendor Specific
0x0070: Capability 0x10: PCI Express
0x0100: Enhanced Capability 0x1b: Process Address Space ID
0x0200: Enhanced Capability 0x0f: Address Translation Services
0x0300: Enhanced Capability 0x13: Page Request Interface
0x00ac: Capability 0x05: Message 

Re: 6.7 snaps upgrade went fine - Intel ax200ngw not so much

2020-06-08 Thread sven falempin
On Mon, Jun 8, 2020 at 1:48 PM Stefan Sperling  wrote:
>
> On Fri, May 22, 2020 at 01:48:28PM -0400, sven falempin wrote:
> >  After a few days ... (free size too small  288 < 1024 /2 )
> >
> > Maybe this can help make the driver better.
> >
> > printf '%x\n' $((0x350+0xf7)) ; grep -A2 'if_iwx.c:515'  /tmp/iwx.dis
> > 447
> > /usr/src/sys/dev/pci/if_iwx.c:515
> >  447:   41 c7 86 28 2f 05 00movl   $0x0,0x52f28(%r14)
> >  44e:   00 00 00 00
> >
> > [0]-[current]-[~]
> > # cat -n /usr/src/sys/dev/pci/if_iwx.c | grep -C5 -E '  515'
> >510  /* free paging*/
> >511  for (i = 0; i < dram->paging_cnt; i++)
> >512  iwx_dma_contig_free(dram->paging);
> >513
> >514  free(dram->paging, M_DEVBUF, dram->paging_cnt *
> > sizeof(*dram->paging));
> >515  dram->paging_cnt = 0;
> >516  dram->paging = NULL;
> >517  }
> >518
> >519  int
> >520  iwx_get_num_sections(const struct iwx_fw_sects *fws, int start
>
> This should fix free with a wrong size in the error case, and avoids
> re-allocating a chunk of DMA memory (sc->ctxt_info_dma) every time the
> firmware gets loaded. Instead, this chunk is now allocated once at
> attach time. This seems to be the allocation that failed in your case.
>
> diff 66ecf2e2f524653126dce17a447a43b26ee90abb /usr/src
> blob - c3ca08c7a726326e37cda8645596a176051b6cf4
> file + sys/dev/pci/if_iwx.c
> --- sys/dev/pci/if_iwx.c
> +++ sys/dev/pci/if_iwx.c
> @@ -230,7 +230,7 @@ int iwx_alloc_fw_monitor_block(struct iwx_softc *, uin
>  intiwx_alloc_fw_monitor(struct iwx_softc *, uint8_t);
>  intiwx_apply_debug_destination(struct iwx_softc *);
>  intiwx_ctxt_info_init(struct iwx_softc *, const struct iwx_fw_sects *);
> -void   iwx_ctxt_info_free(struct iwx_softc *);
> +void   iwx_ctxt_info_free_fw_img(struct iwx_softc *);
>  void   iwx_ctxt_info_free_paging(struct iwx_softc *);
>  intiwx_init_fw_sec(struct iwx_softc *, const struct iwx_fw_sects *,
> struct iwx_context_info_dram *);
> @@ -535,52 +535,60 @@ iwx_init_fw_sec(struct iwx_softc *sc, const struct iwx
>  struct iwx_context_info_dram *ctxt_dram)
>  {
> struct iwx_self_init_dram *dram = >init_dram;
> -   int i, ret, lmac_cnt, umac_cnt, paging_cnt;
> +   int i, ret, fw_cnt = 0;
>
> KASSERT(dram->paging == NULL);
>
> -   lmac_cnt = iwx_get_num_sections(fws, 0);
> +   dram->lmac_cnt = iwx_get_num_sections(fws, 0);
> /* add 1 due to separator */
> -   umac_cnt = iwx_get_num_sections(fws, lmac_cnt + 1);
> +   dram->umac_cnt = iwx_get_num_sections(fws, dram->lmac_cnt + 1);
> /* add 2 due to separators */
> -   paging_cnt = iwx_get_num_sections(fws, lmac_cnt + umac_cnt + 2);
> +   dram->paging_cnt = iwx_get_num_sections(fws,
> +   dram->lmac_cnt + dram->umac_cnt + 2);
>
> -   dram->fw = mallocarray(umac_cnt + lmac_cnt, sizeof(*dram->fw),
> -   M_DEVBUF,  M_ZERO | M_NOWAIT);
> -   if (!dram->fw)
> +   dram->fw = mallocarray(dram->umac_cnt + dram->lmac_cnt,
> +   sizeof(*dram->fw), M_DEVBUF,  M_ZERO | M_NOWAIT);
> +   if (!dram->fw) {
> +   printf("%s: could not allocate memory for firmware 
> sections\n",
> +   DEVNAME(sc));
> return ENOMEM;
> -   dram->paging = mallocarray(paging_cnt, sizeof(*dram->paging),
> +   }
> +
> +   dram->paging = mallocarray(dram->paging_cnt, sizeof(*dram->paging),
> M_DEVBUF, M_ZERO | M_NOWAIT);
> -   if (!dram->paging)
> +   if (!dram->paging) {
> +   printf("%s: could not allocate memory for firmware paging\n",
> +   DEVNAME(sc));
> return ENOMEM;
> +   }
>
> /* initialize lmac sections */
> -   for (i = 0; i < lmac_cnt; i++) {
> +   for (i = 0; i < dram->lmac_cnt; i++) {
> ret = iwx_ctxt_info_alloc_dma(sc, >fw_sect[i],
> -  >fw[dram->fw_cnt]);
> +  >fw[fw_cnt]);
> if (ret)
> return ret;
> ctxt_dram->lmac_img[i] =
> -   htole64(dram->fw[dram->fw_cnt].paddr);
> +   htole64(dram->fw[fw_cnt].paddr);
> DPRINTF(("%s: firmware LMAC section %d at 0x%llx size 
> %lld\n", __func__, i,
> -   (unsigned long long)dram->fw[dram->fw_cnt].paddr,
> -   (unsigned long long)dram->fw[dram->fw_cnt].size));
> -   dram->fw_cnt++;
> +   (unsigned long long)dram->fw[fw_cnt].paddr,
> +   (unsigned long long)dram->fw[fw_cnt].size));
> +   fw_cnt++;
> }
>
> /* initialize umac sections */
> -   for (i = 0; i < umac_cnt; i++) {
> +   for (i = 0; i < dram->umac_cnt; i++) {
> /* access FW with +1 to make up for lmac 

Re: libkern: ffs() for arm64

2020-06-08 Thread Joerg Sonnenberger
On Mon, Jun 08, 2020 at 11:16:59PM +0200, Christian Weisgerber wrote:
> I blame dlg@ for making me scrutinize the POWER7 instruction set,
> which led me to the clz instruction, which led me to ffs().  I
> wanted to add this to libc, but then realized the futility because
> the compiler already inlines its optimized copy of ffs().  However,
> this optimization is disabled for the kernel with -ffreestanding.

int ffs(int x) {
 return __builtin_ffs(x);
}

doesn't work?

Joerg



Re: libkern: ffs() for arm64

2020-06-08 Thread Theo de Raadt
Either works for me.

Honestly

- either one is hard to understand without referring to the
  docs (and then realizing power numbers bits backwards). 
- It is simple enough it has to work or there will be chaos

and I'm not that afraid of more poeple seeing RETGUARD stubs :) 

Christian Weisgerber  wrote:

> On 2020-06-08, Christian Weisgerber  wrote:
> 
> > More archs to come...
> 
> Style question:
> Since this mostly comes down to embedding a single special instruction
> in between normal C operations, I wonder whether I should just do .c
> with asm() instead of .S, and leave all the boilerplate to the
> compiler?  It would save me from reading up on calling conventions,
> more assembly syntax, etc.
> 
> E.g.:
> 
> int ffs(int x)
> {
> x = x & -x;
> __asm volatile("clz %0, %0" : "=r" (x));
> return (32 - x);
> }
> 
> -- 
> Christian "naddy" Weisgerber  na...@mips.inka.de
> 



Re: libkern: ffs() for arm64

2020-06-08 Thread Todd C . Miller
On Mon, 08 Jun 2020 22:35:42 -, Christian Weisgerber wrote:

> Style question:
> Since this mostly comes down to embedding a single special instruction
> in between normal C operations, I wonder whether I should just do .c
> with asm() instead of .S, and leave all the boilerplate to the
> compiler?  It would save me from reading up on calling conventions,
> more assembly syntax, etc.

I think using inline assembler is fine if it makes things easier.

 - todd



Re: libkern: ffs() for arm64

2020-06-08 Thread Christian Weisgerber
On 2020-06-08, Christian Weisgerber  wrote:

> More archs to come...

Style question:
Since this mostly comes down to embedding a single special instruction
in between normal C operations, I wonder whether I should just do .c
with asm() instead of .S, and leave all the boilerplate to the
compiler?  It would save me from reading up on calling conventions,
more assembly syntax, etc.

E.g.:

int ffs(int x)
{
x = x & -x;
__asm volatile("clz %0, %0" : "=r" (x));
return (32 - x);
}

-- 
Christian "naddy" Weisgerber  na...@mips.inka.de



libkern: ffs() for arm64

2020-06-08 Thread Christian Weisgerber
Here is an optimized kernel ffs(3) for arm64.

I blame dlg@ for making me scrutinize the POWER7 instruction set,
which led me to the clz instruction, which led me to ffs().  I
wanted to add this to libc, but then realized the futility because
the compiler already inlines its optimized copy of ffs().  However,
this optimization is disabled for the kernel with -ffreestanding.

Honestly, I'm not sure I can claim to have written this.  The elegant
version below is adapted from clang output, because the compiler
is smarter than I am.

More archs to come...

Comments?  OK?

Index: lib/libkern/arch/arm64/ffs.S
===
RCS file: lib/libkern/arch/arm64/ffs.S
diff -N lib/libkern/arch/arm64/ffs.S
--- /dev/null   1 Jan 1970 00:00:00 -
+++ lib/libkern/arch/arm64/ffs.S8 Jun 2020 20:35:01 -
@@ -0,0 +1,17 @@
+/* $OpenBSD$ */
+/*
+ * Written by Christian Weisgerber .
+ * Public domain.
+ */
+
+#include 
+
+ENTRY(ffs)
+   RETGUARD_SETUP(ffs, x15)
+   rbitw1, w0
+   clz w1, w1
+   cmp w0, #0
+   csinc   w0, wzr, w1, eq
+   RETGUARD_CHECK(ffs, x15)
+   ret
+END(ffs)
-- 
Christian "naddy" Weisgerber  na...@mips.inka.de



netstat -R: list rdomains with associated ifs and tables

2020-06-08 Thread Remi Locherer
Hi,

to my knowledge there is no easy way to list all active rdomains or
routing tables. Other platforms have "show vrf" or similar commands
for an overview.

Here is my attempt at such a view for OpenBSD:

twister ..in/netstat$ obj/netstat -R
Rdomain   0
  Interfaces: lo0 iwm0 re0 enc0 pflog0
  Routing tables:
  0: Internet   8, Internet6  43, MPLS   0
  3: Internet   1, Internet6   0, MPLS   0
  7: Internet  130309, Internet6   1, MPLS   0

Rdomain  77
  Interfaces: vether77 lo77
  Routing tables:
 77: Internet   0, Internet6   0, MPLS   0

Rdomain 122
  Interfaces: vether122 lo122 pair122 vether1122 vether1123 vether1124 
vether1125 vether1126 vether1127
  Routing tables:
122: Internet  24, Internet6   0, MPLS   0

Rdomain 255
  Interfaces: vether255 lo255
  Routing tables:
255: Internet   3, Internet6   0, MPLS   0

twister ..in/netstat$

Comments? OKs?

Remi


Index: main.c
===
RCS file: /cvs/src/usr.bin/netstat/main.c,v
retrieving revision 1.116
diff -u -p -r1.116 main.c
--- main.c  28 Apr 2019 17:59:51 -  1.116
+++ main.c  30 May 2020 17:59:33 -
@@ -127,7 +127,7 @@ main(int argc, char *argv[])
tableid = getrtable();
 
while ((ch = getopt(argc, argv,
-   "AaBbc:deFf:ghI:iLlM:mN:np:P:qrsT:tuvW:w:")) != -1)
+   "AaBbc:deFf:ghI:iLlM:mN:np:P:qRrsT:tuvW:w:")) != -1)
switch (ch) {
case 'A':
Aflag = 1;
@@ -225,6 +225,9 @@ main(int argc, char *argv[])
case 'q':
qflag = 1;
break;
+   case 'R':
+   Rflag = 1;
+   break;
case 'r':
rflag = 1;
break;
@@ -318,6 +321,11 @@ main(int argc, char *argv[])
mroutepr();
if (af == AF_INET6 || af == AF_UNSPEC)
mroute6pr();
+   exit(0);
+   }
+
+   if (Rflag) {
+   rdomainpr();
exit(0);
}
 
Index: netstat.1
===
RCS file: /cvs/src/usr.bin/netstat/netstat.1,v
retrieving revision 1.86
diff -u -p -r1.86 netstat.1
--- netstat.1   17 Apr 2019 20:34:21 -  1.86
+++ netstat.1   8 Jun 2020 19:21:26 -
@@ -74,6 +74,8 @@
 .Op Fl i | I Ar interface
 .Nm
 .Op Fl W Ar interface
+.Nm
+.Op Fl R
 .Sh DESCRIPTION
 The
 .Nm
@@ -267,6 +269,9 @@ Otherwise the states of the matching soc
 Only show interfaces that have seen packets (or bytes if
 .Fl b
 is specified).
+.It Fl R
+Show all rdomains and list associated interfaces and routing tables
+with number of entries.
 .It Fl r
 Show the routing tables.
 The output is explained in more detail below.
Index: netstat.h
===
RCS file: /cvs/src/usr.bin/netstat/netstat.h,v
retrieving revision 1.74
diff -u -p -r1.74 netstat.h
--- netstat.h   28 Apr 2019 17:59:51 -  1.74
+++ netstat.h   7 Jun 2020 22:03:10 -
@@ -57,6 +57,7 @@ int   pflag;  /* show given protocol */
 intPflag;  /* show given PCB */
 intqflag;  /* only display non-zero values for output */
 intrflag;  /* show routing tables (or routing stats) */
+intRflag;  /* show rdomain and rtable summary */
 intsflag;  /* show protocol statistics */
 inttflag;  /* show i/f watchdog timers */
 intvflag;  /* be verbose */
@@ -112,6 +113,9 @@ voidrt_stats(void);
 void   pr_rthdr(int, int);
 void   pr_encaphdr(void);
 void   pr_family(int);
+
+void   rdomainpr(void);
+void   rttsummarypr(int);
 
 void   ip6_stats(char *);
 void   icmp6_stats(char *);
Index: route.c
===
RCS file: /cvs/src/usr.bin/netstat/route.c,v
retrieving revision 1.104
diff -u -p -r1.104 route.c
--- route.c 28 Jun 2019 13:35:02 -  1.104
+++ route.c 8 Jun 2020 19:29:58 -
@@ -51,6 +51,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "netstat.h"
 
@@ -346,4 +347,117 @@ rt_stats(void)
rtstat.rts_unreach, plural(rtstat.rts_unreach));
printf("\t%u use%s of a wildcard route\n",
rtstat.rts_wildcard, plural(rtstat.rts_wildcard));
+}
+
+/*
+ * Print rdomain and rtable summary
+ */
+
+void
+rdomainpr(void)
+{
+   struct if_data   *ifd;
+   struct ifaddrs   *ifap, *ifa;
+   struct rt_tableinfo   info;
+
+   int  rtt_dom[RT_TABLEID_MAX+1];
+   int  mib[6], rdom, rtt;
+   size_t   len;
+   char*old, *rdom_if[RT_TABLEID_MAX+1] = { };
+
+   getifaddrs();
+   for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
+   if (ifa->ifa_addr->sa_family != AF_LINK)
+   

Re: userland clock_gettime proof of concept

2020-06-08 Thread Paul Irofti
> This iteration of the diff adds bounds checking for tk_user and moves
> the usertc.c stub to every arch in libc as recommanded by deraadt@.
> It also fixes a gettimeofday issue reported by cheloha@ and tb@.

Forgot to add armv7 tk_nclock entries. Noticed by benno@, thanks!


diff --git lib/libc/arch/aarch64/gen/Makefile.inc 
lib/libc/arch/aarch64/gen/Makefile.inc
index a7b1b73f3ef..ee198f5d611 100644
--- lib/libc/arch/aarch64/gen/Makefile.inc
+++ lib/libc/arch/aarch64/gen/Makefile.inc
@@ -9,4 +9,4 @@ SRCS+=  fpgetmask.c fpgetround.c fpgetsticky.c
 SRCS+= fpsetmask.c fpsetround.c fpsetsticky.c
 SRCS+= fpclassifyl.c
 SRCS+= isfinitel.c isinfl.c isnanl.c isnormall.c
-SRCS+= signbitl.c
+SRCS+= signbitl.c usertc.c
diff --git lib/libc/arch/aarch64/gen/usertc.c lib/libc/arch/aarch64/gen/usertc.c
new file mode 100644
index 000..84a112c2ea3
--- /dev/null
+++ lib/libc/arch/aarch64/gen/usertc.c
@@ -0,0 +1,21 @@
+/* $OpenBSD$   */
+/*
+ * Copyright (c) 2020 Paul Irofti 
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include 
+#include 
+
+int (*const _tc_get_timecount)(struct timekeep *, uint64_t *) = NULL;
diff --git lib/libc/arch/alpha/gen/Makefile.inc 
lib/libc/arch/alpha/gen/Makefile.inc
index a44599d2cab..2a8abd32b61 100644
--- lib/libc/arch/alpha/gen/Makefile.inc
+++ lib/libc/arch/alpha/gen/Makefile.inc
@@ -3,5 +3,5 @@
 
 SRCS+= _setjmp.S fabs.S infinity.c ldexp.c modf.c nan.c setjmp.S
 SRCS+= flt_rounds.c fpgetmask.c fpgetround.c fpgetsticky.c fpsetmask.c \
-   fpsetround.c fpsetsticky.c
+   fpsetround.c fpsetsticky.c usertc.c
 SRCS+= sigsetjmp.S
diff --git lib/libc/arch/alpha/gen/usertc.c lib/libc/arch/alpha/gen/usertc.c
new file mode 100644
index 000..84a112c2ea3
--- /dev/null
+++ lib/libc/arch/alpha/gen/usertc.c
@@ -0,0 +1,21 @@
+/* $OpenBSD$   */
+/*
+ * Copyright (c) 2020 Paul Irofti 
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include 
+#include 
+
+int (*const _tc_get_timecount)(struct timekeep *, uint64_t *) = NULL;
diff --git lib/libc/arch/amd64/gen/Makefile.inc 
lib/libc/arch/amd64/gen/Makefile.inc
index e995309ed71..f6349e2b974 100644
--- lib/libc/arch/amd64/gen/Makefile.inc
+++ lib/libc/arch/amd64/gen/Makefile.inc
@@ -2,6 +2,7 @@
 
 SRCS+= _setjmp.S fabs.S infinity.c ldexp.c modf.S nan.c setjmp.S \
sigsetjmp.S
-SRCS+= fpclassifyl.c isfinitel.c isinfl.c isnanl.c isnormall.c signbitl.c
+SRCS+= fpclassifyl.c isfinitel.c isinfl.c isnanl.c isnormall.c signbitl.c \
+   usertc.c
 SRCS+= flt_rounds.S fpgetmask.S fpgetround.S fpgetsticky.S fpsetmask.S \
fpsetround.S fpsetsticky.S
diff --git lib/libc/arch/amd64/gen/usertc.c lib/libc/arch/amd64/gen/usertc.c
new file mode 100644
index 000..3f3052445cf
--- /dev/null
+++ lib/libc/arch/amd64/gen/usertc.c
@@ -0,0 +1,53 @@
+/* $OpenBSD$ */
+/*
+ * Copyright (c) 2020 Paul Irofti 
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */

Re: powerpc64: ldbrx/stdbrx for endian.h?

2020-06-08 Thread Theo de Raadt
Miod Vallat  wrote:

> > There is an interest in supporting PowerPC 970 ("G5").  That would
> > allow people to use more than 2G of RAM on the last generations of
> > Apple PowerMac machines.  Otherwise I don't think we are interested in
> > anything before POWER8.
> 
> Years ago, a decision was made to ditch 64-bit PA-RISC (mostly because
> toolchain limitations for 64-bit userland) and keep running a 32-bit
> kernels on 64-bit PA-RISC systems with a 32-bit PDC.

Sorry that is incorrect.  I was never able to do a full make build on
hppa64.  It was ditched because it didn't work.  

> At about the same time, it was also decided that G5 systems with more
> than 2 or 3 GB of memory were rare enough not to be worth supporting in
> 64-bit mode.
>
> Given the current "3GHz and gobs of ram or dust" party's line those days,
> I don't see a point in attempting to support G5 systems in 64-bit mode,
> if this seriously impacts support for sexier^Wmore modern designs.

That is also incorrect.  Our code on (most models) of Apple G5 systems
has always had some sort of DMA or other bug that makes drives not work.
(Maybe it is a bug in an exception or interrupt handler or a mapping
mistake?)  Anways, this frustrated efforts to get these machines working
even in 32 bit mode.  I suspect if the hardware busses had worked,
someone might have taken a more serious shot at 64 bit mode.

In both cases it has nothing to do with the amount of physical memory.
I suspect half our users of amd64 machines have <= 3G of ram, but
they aren't running in i386 mode, because expanded virtual address
matters far more.



Re: powerpc64: ldbrx/stdbrx for endian.h?

2020-06-08 Thread Miod Vallat


> There is an interest in supporting PowerPC 970 ("G5").  That would
> allow people to use more than 2G of RAM on the last generations of
> Apple PowerMac machines.  Otherwise I don't think we are interested in
> anything before POWER8.

Years ago, a decision was made to ditch 64-bit PA-RISC (mostly because
toolchain limitations for 64-bit userland) and keep running a 32-bit
kernels on 64-bit PA-RISC systems with a 32-bit PDC.

Before that, mickey@ was adamant that earlier hppa64 systems (pre-astro)
with 32-bit PCI busses and no way to have more than 2GB of memory (such
as C240) were not worth supporting in 64-bit mode.

At about the same time, it was also decided that G5 systems with more
than 2 or 3 GB of memory were rare enough not to be worth supporting in
64-bit mode.

Given the current "3GHz and gobs of ram or dust" party's line those days,
I don't see a point in attempting to support G5 systems in 64-bit mode,
if this seriously impacts support for sexier^Wmore modern designs.



Re: 6.7 snaps upgrade went fine - Intel ax200ngw not so much

2020-06-08 Thread Stefan Sperling
On Fri, May 22, 2020 at 01:48:28PM -0400, sven falempin wrote:
>  After a few days ... (free size too small  288 < 1024 /2 )
> 
> Maybe this can help make the driver better.
> 
> printf '%x\n' $((0x350+0xf7)) ; grep -A2 'if_iwx.c:515'  /tmp/iwx.dis
> 447
> /usr/src/sys/dev/pci/if_iwx.c:515
>  447:   41 c7 86 28 2f 05 00movl   $0x0,0x52f28(%r14)
>  44e:   00 00 00 00
> 
> [0]-[current]-[~]
> # cat -n /usr/src/sys/dev/pci/if_iwx.c | grep -C5 -E '  515'
>510  /* free paging*/
>511  for (i = 0; i < dram->paging_cnt; i++)
>512  iwx_dma_contig_free(dram->paging);
>513
>514  free(dram->paging, M_DEVBUF, dram->paging_cnt *
> sizeof(*dram->paging));
>515  dram->paging_cnt = 0;
>516  dram->paging = NULL;
>517  }
>518
>519  int
>520  iwx_get_num_sections(const struct iwx_fw_sects *fws, int start

This should fix free with a wrong size in the error case, and avoids
re-allocating a chunk of DMA memory (sc->ctxt_info_dma) every time the
firmware gets loaded. Instead, this chunk is now allocated once at
attach time. This seems to be the allocation that failed in your case.

diff 66ecf2e2f524653126dce17a447a43b26ee90abb /usr/src
blob - c3ca08c7a726326e37cda8645596a176051b6cf4
file + sys/dev/pci/if_iwx.c
--- sys/dev/pci/if_iwx.c
+++ sys/dev/pci/if_iwx.c
@@ -230,7 +230,7 @@ int iwx_alloc_fw_monitor_block(struct iwx_softc *, uin
 intiwx_alloc_fw_monitor(struct iwx_softc *, uint8_t);
 intiwx_apply_debug_destination(struct iwx_softc *);
 intiwx_ctxt_info_init(struct iwx_softc *, const struct iwx_fw_sects *);
-void   iwx_ctxt_info_free(struct iwx_softc *);
+void   iwx_ctxt_info_free_fw_img(struct iwx_softc *);
 void   iwx_ctxt_info_free_paging(struct iwx_softc *);
 intiwx_init_fw_sec(struct iwx_softc *, const struct iwx_fw_sects *,
struct iwx_context_info_dram *);
@@ -535,52 +535,60 @@ iwx_init_fw_sec(struct iwx_softc *sc, const struct iwx
 struct iwx_context_info_dram *ctxt_dram)
 {
struct iwx_self_init_dram *dram = >init_dram;
-   int i, ret, lmac_cnt, umac_cnt, paging_cnt;
+   int i, ret, fw_cnt = 0;
 
KASSERT(dram->paging == NULL);
 
-   lmac_cnt = iwx_get_num_sections(fws, 0);
+   dram->lmac_cnt = iwx_get_num_sections(fws, 0);
/* add 1 due to separator */
-   umac_cnt = iwx_get_num_sections(fws, lmac_cnt + 1);
+   dram->umac_cnt = iwx_get_num_sections(fws, dram->lmac_cnt + 1);
/* add 2 due to separators */
-   paging_cnt = iwx_get_num_sections(fws, lmac_cnt + umac_cnt + 2);
+   dram->paging_cnt = iwx_get_num_sections(fws,
+   dram->lmac_cnt + dram->umac_cnt + 2);
 
-   dram->fw = mallocarray(umac_cnt + lmac_cnt, sizeof(*dram->fw),
-   M_DEVBUF,  M_ZERO | M_NOWAIT);
-   if (!dram->fw)
+   dram->fw = mallocarray(dram->umac_cnt + dram->lmac_cnt,
+   sizeof(*dram->fw), M_DEVBUF,  M_ZERO | M_NOWAIT);
+   if (!dram->fw) {
+   printf("%s: could not allocate memory for firmware sections\n",
+   DEVNAME(sc));
return ENOMEM;
-   dram->paging = mallocarray(paging_cnt, sizeof(*dram->paging),
+   }
+
+   dram->paging = mallocarray(dram->paging_cnt, sizeof(*dram->paging),
M_DEVBUF, M_ZERO | M_NOWAIT);
-   if (!dram->paging)
+   if (!dram->paging) {
+   printf("%s: could not allocate memory for firmware paging\n",
+   DEVNAME(sc));
return ENOMEM;
+   }
 
/* initialize lmac sections */
-   for (i = 0; i < lmac_cnt; i++) {
+   for (i = 0; i < dram->lmac_cnt; i++) {
ret = iwx_ctxt_info_alloc_dma(sc, >fw_sect[i],
-  >fw[dram->fw_cnt]);
+  >fw[fw_cnt]);
if (ret)
return ret;
ctxt_dram->lmac_img[i] =
-   htole64(dram->fw[dram->fw_cnt].paddr);
+   htole64(dram->fw[fw_cnt].paddr);
DPRINTF(("%s: firmware LMAC section %d at 0x%llx size %lld\n", 
__func__, i,
-   (unsigned long long)dram->fw[dram->fw_cnt].paddr,
-   (unsigned long long)dram->fw[dram->fw_cnt].size));
-   dram->fw_cnt++;
+   (unsigned long long)dram->fw[fw_cnt].paddr,
+   (unsigned long long)dram->fw[fw_cnt].size));
+   fw_cnt++;
}
 
/* initialize umac sections */
-   for (i = 0; i < umac_cnt; i++) {
+   for (i = 0; i < dram->umac_cnt; i++) {
/* access FW with +1 to make up for lmac separator */
ret = iwx_ctxt_info_alloc_dma(sc,
-   >fw_sect[dram->fw_cnt + 1], >fw[dram->fw_cnt]);
+   >fw_sect[fw_cnt + 1], >fw[fw_cnt]);
if (ret)
return ret;

Re: pppx(4): rework to be clese to pseudo-interfaces

2020-06-08 Thread Vitaliy Makkoveev
On Mon, Jun 08, 2020 at 12:49:05PM +0300, Vitaliy Makkoveev wrote:
> 
> 
> > On 8 Jun 2020, at 11:34, Martin Pieuchot  wrote:
> > 
> > On 29/05/20(Fri) 13:22, Vitaliy Makkoveev wrote:
> >> This time pppx_add_session() has mixed initialisation order. It starts
> >> to initialise pipex(4) session, then initialises `ifnet', then links
> >> pipex(4) session, then continue to initialize `ifnet'.
> >> pppx_add_session() can sleep and pppx_if_start() can start to work with
> >> unlinked pipex(4) session.
> >> 
> >> Also pppx_if_destroy() can sleep and pppx_if_start() can access to this
> >> `pxi' with unlinked session. pppx_if_start() has checking of
> >> `IFF_RUNNING' flag but it's usesless.
> >> 
> >> Diff below does pppx_add_session() reordering. At first we initilize
> >> and attach `ifnet', at second we initialize and link pipex(4) session
> >> and at last we set `IFF_RUNNING' flag on `ifnet.
> > 
> > The fact that you have to call if_detach() if a field isn't validated
> > makes me wonder if this refactoring is better than the existing logic.
> 
> Except "req->pr_timeout_sec !=0” checking all userland input checks
> were copypasted prom pipex_add_session(). But pppx_add_session()
> inserts pppx(4) related code between checks and session initialization.
> So I decided to keep all pipex(4) related code together for further
> deduplication. Also we want to initialise `ifnet’ before session
> linking.
> 
> There is another way to rewrite pppx_add_session() and
> pipex_add_session(). We can split pipex_add_session() to two
> functions: pipex_init_session() and pipex_setup_session(). The first
> will do checks and insert new session with PIPEX_STATE_INITIAL state.
> The second will do the rest. So we can do checks before `ifnet’
> dances in x_add_session(). This time PIPEX_STATE_INITIAL declared
> but not used, at least pipex_lookup_by_session_id() should be
> refactored before.
> 
> Is this direction more acceptable?

Diff below illustrates this.

1. New function pipex_init_session() validates request and if it's
accepted creates new pipex session but keeps it unlinked. This session
is only inserted to `session_list'. Unlinked session can't be accessed
by stack.
2. New function pipex_link_session() connects session to stack.
3. New function pipex_unlink_session() disconnects session from the
stack.

Duplicated pipex(4) code from pppx_add_session was replaced by
pipex_init_session() and pipex_link_session(). pppx(4) related code is
not reordered.

Duplicated pipex(4) code from pppx_if_destroy() was replaced by
pipex_unlink_session().

Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.87
diff -u -p -r1.87 if_pppx.c
--- sys/net/if_pppx.c   29 May 2020 06:51:52 -  1.87
+++ sys/net/if_pppx.c   8 Jun 2020 12:07:09 -
@@ -659,14 +659,10 @@ pppx_add_session(struct pppx_dev *pxd, s
 {
struct pppx_if *pxi;
struct pipex_session *session;
-   struct pipex_hash_head *chain;
struct ifnet *ifp;
int unit, error = 0;
struct in_ifaddr *ia;
struct sockaddr_in ifaddr;
-#ifdef PIPEX_PPPOE
-   struct ifnet *over_ifp = NULL;
-#endif
 
/*
 * XXX: As long as `session' is allocated as part of a `pxi'
@@ -676,151 +672,20 @@ pppx_add_session(struct pppx_dev *pxd, s
if (req->pr_timeout_sec != 0)
return (EINVAL);
 
-   switch (req->pr_protocol) {
-#ifdef PIPEX_PPPOE
-   case PIPEX_PROTO_PPPOE:
-   over_ifp = ifunit(req->pr_proto.pppoe.over_ifname);
-   if (over_ifp == NULL)
-   return (EINVAL);
-   if (req->pr_peer_address.ss_family != AF_UNSPEC)
-   return (EINVAL);
-   break;
-#endif
-#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
-   case PIPEX_PROTO_PPTP:
-   case PIPEX_PROTO_L2TP:
-   switch (req->pr_peer_address.ss_family) {
-   case AF_INET:
-   if (req->pr_peer_address.ss_len != sizeof(struct 
sockaddr_in))
-   return (EINVAL);
-   break;
-#ifdef INET6
-   case AF_INET6:
-   if (req->pr_peer_address.ss_len != sizeof(struct 
sockaddr_in6))
-   return (EINVAL);
-   break;
-#endif
-   default:
-   return (EPROTONOSUPPORT);
-   }
-   if (req->pr_peer_address.ss_family !=
-   req->pr_local_address.ss_family ||
-   req->pr_peer_address.ss_len !=
-   req->pr_local_address.ss_len)
-   return (EINVAL);
-   break;
-#endif /* defined(PIPEX_PPTP) || defined(PIPEX_L2TP) */
-   default:
-   return (EPROTONOSUPPORT);
-   }
-
-   session = pipex_lookup_by_session_id(req->pr_protocol,
-   

Re: powerpc64: ldbrx/stdbrx for endian.h?

2020-06-08 Thread Christian Weisgerber
On 2020-06-08, Christian Weisgerber  wrote:

>> Did they also happen to add opcodes for doing swaps in registers?
>
> No.
> (I haven't looked at the vector instructions yet.)

PS: There's a vector permutate instruction, but AFAICT there is no
way to move data between general purpose and vector registers; you
have to go through memory.

-- 
Christian "naddy" Weisgerber  na...@mips.inka.de



Re: pppx(4): rework to be clese to pseudo-interfaces

2020-06-08 Thread Vitaliy Makkoveev



> On 8 Jun 2020, at 11:34, Martin Pieuchot  wrote:
> 
> On 29/05/20(Fri) 13:22, Vitaliy Makkoveev wrote:
>> This time pppx_add_session() has mixed initialisation order. It starts
>> to initialise pipex(4) session, then initialises `ifnet', then links
>> pipex(4) session, then continue to initialize `ifnet'.
>> pppx_add_session() can sleep and pppx_if_start() can start to work with
>> unlinked pipex(4) session.
>> 
>> Also pppx_if_destroy() can sleep and pppx_if_start() can access to this
>> `pxi' with unlinked session. pppx_if_start() has checking of
>> `IFF_RUNNING' flag but it's usesless.
>> 
>> Diff below does pppx_add_session() reordering. At first we initilize
>> and attach `ifnet', at second we initialize and link pipex(4) session
>> and at last we set `IFF_RUNNING' flag on `ifnet.
> 
> The fact that you have to call if_detach() if a field isn't validated
> makes me wonder if this refactoring is better than the existing logic.

Except "req->pr_timeout_sec !=0” checking all userland input checks
were copypasted prom pipex_add_session(). But pppx_add_session()
inserts pppx(4) related code between checks and session initialization.
So I decided to keep all pipex(4) related code together for further
deduplication. Also we want to initialise `ifnet’ before session
linking.

There is another way to rewrite pppx_add_session() and
pipex_add_session(). We can split pipex_add_session() to two
functions: pipex_init_session() and pipex_setup_session(). The first
will do checks and insert new session with PIPEX_STATE_INITIAL state.
The second will do the rest. So we can do checks before `ifnet’
dances in x_add_session(). This time PIPEX_STATE_INITIAL declared
but not used, at least pipex_lookup_by_session_id() should be
refactored before.

Is this direction more acceptable?

> 
>> Also we cleaning `IFF_RUNNING' flag before pppx(4) session destruction
>> in pppx_if_destroy().
> 
> This seems unrelated.
> 
>> Since we made `ifnet' and pipex(4) session initialization separately, we
>> are more close to remove duplicated code.
> 
> Can't we do that directly?  There are many blocks in this function:
> 
> - Allocate a `pxi'
> - Attach an interface (`ifp')
> - Insert an address on the interface
> - Link the `pxi' with the interface
> - Setup the session
> 
> Maybe user input validation should be done first. 

What is you talk to do directly?



Re: powerpc64: ldbrx/stdbrx for endian.h?

2020-06-08 Thread Mark Kettenis
> Date: Mon, 8 Jun 2020 13:28:55 +0200
> From: Christian Weisgerber 
> 
> powerpc has byte-swapping 16 and 32-bit load/stores and we use those
> in .
> 
> Starting with POWER7 (Power ISA v.2.06), there are also corresponding
> 64-bit instructions.  Do we want to use those on powerpc64?  Or do
> we want to keep compatibility with older processors?

There is an interest in supporting PowerPC 970 ("G5").  That would
allow people to use more than 2G of RAM on the last generations of
Apple PowerMac machines.  Otherwise I don't think we are interested in
anything before POWER8.

That said, I don't think the 64-bit bteswapping operations are used
very much in our kernel.  Seo the benefit of this may be small.  But
then a diff like this can be reversed without too much fuss.

ok kettenis@

> Index: arch/powerpc64/include/endian.h
> ===
> RCS file: /cvs/src/sys/arch/powerpc64/include/endian.h,v
> retrieving revision 1.1
> diff -u -p -r1.1 endian.h
> --- arch/powerpc64/include/endian.h   16 May 2020 17:11:14 -  1.1
> +++ arch/powerpc64/include/endian.h   8 Jun 2020 11:16:33 -
> @@ -36,7 +36,7 @@ __mswap16(volatile const __uint16_t *m)
>  
>   __asm("lhbrx %0, 0, %1"
>   : "=r" (v)
> -: "r" (m), "m" (*m));
> + : "r" (m), "m" (*m));
>  
>   return (v);
>  }
> @@ -48,7 +48,7 @@ __mswap32(volatile const __uint32_t *m)
>  
>   __asm("lwbrx %0, 0, %1"
>   : "=r" (v)
> -: "r" (m), "m" (*m));
> + : "r" (m), "m" (*m));
>  
>   return (v);
>  }
> @@ -56,11 +56,11 @@ __mswap32(volatile const __uint32_t *m)
>  static inline __uint64_t
>  __mswap64(volatile const __uint64_t *m)
>  {
> - __uint32_t *a = (__uint32_t *)m;
>   __uint64_t v;
>  
> - v = (__uint64_t)__mswap32(a + 1) << 32 |
> - (__uint64_t)__mswap32(a);
> + __asm("ldbrx %0, 0, %1"
> + : "=r" (v)
> + : "r" (m), "m" (*m));
>  
>   return (v);
>  }
> @@ -84,10 +84,9 @@ __swapm32(volatile __uint32_t *m, __uint
>  static inline void
>  __swapm64(volatile __uint64_t *m, __uint64_t v)
>  {
> - __uint32_t *a = (__uint32_t *)m;
> -
> - __swapm32(a + 1, v >> 32);
> - __swapm32(a, v);
> + __asm("stdbrx %1, 0, %2"
> + : "=m" (*m)
> + : "r" (v), "r" (m));
>  }
>  
>  #define __HAVE_MD_SWAPIO
> -- 
> Christian "naddy" Weisgerber  na...@mips.inka.de
> 
> 



Re: powerpc64: ldbrx/stdbrx for endian.h?

2020-06-08 Thread Christian Weisgerber
David Gwynne:

> > Starting with POWER7 (Power ISA v.2.06), there are also corresponding
> > 64-bit instructions.  Do we want to use those on powerpc64?  Or do
> > we want to keep compatibility with older processors?
> 
> I'm ok with using the instructions. I can't think of what benefit compat in 
> this space would actually provide.

Well, if people were to extend powerpc64 down to the PowerPC G5
(= POWER4) Apple machines, then it would not be possible to use
those instructions.

> Did they also happen to add opcodes for doing swaps in registers?

No.
(I haven't looked at the vector instructions yet.)

-- 
Christian "naddy" Weisgerber  na...@mips.inka.de



Re: urtwn(4) hardware crypto

2020-06-08 Thread Sebastian Benoit
Jonathan Matthew(jonat...@d14n.org) on 2020.06.05 21:54:30 +1000:
> This enables use of hardware crypto for CCMP in urtwn(4). As with other
> drivers, this reduces cpu usage significantly when moving lots of data.
> I've tested this on an assortment of hardware (RTL8188CUS, RTL8188EU,
> RTL8192EU) with no problems, and this is one of the few things that
> remains constant across a lot of Realtek wifi chips, but some wider
> testing couldn't hurt. Since this touches the code shared with rtwn(4),
> I've also tested that that still works.

Works on 

urtwn0 at uhub2 port 2 configuration 1 interface 0 "Realtek 802.11n NIC" rev 
2.00/0.00 addr 3
urtwn0: MAC/BB RTL8188EU, RF 6052 1T1R, address d4:6e:0e:29:e2:84

usb stick, on armv7.

In a quick test, download of a 10MB large file is faster with your diff, up
from 1MB/s to now 1.6MB/s.

/Benno

> 
> 
> Index: ic/r92creg.h
> ===
> RCS file: /cvs/src/sys/dev/ic/r92creg.h,v
> retrieving revision 1.24
> diff -u -p -r1.24 r92creg.h
> --- ic/r92creg.h  11 Mar 2019 06:19:33 -  1.24
> +++ ic/r92creg.h  5 Jun 2020 11:52:21 -
> @@ -688,6 +688,16 @@
>  #define R92C_CAMCMD_CLR  0x4000
>  #define R92C_CAMCMD_POLLING  0x8000
>  
> +/* Bits for R92C_SECCFG. */
> +#define R92C_SECCFG_TXUCKEY_DEF 0x0001
> +#define R92C_SECCFG_RXUCKEY_DEF  0x0002
> +#define R92C_SECCFG_TXENC_ENA0x0004
> +#define R92C_SECCFG_RXENC_ENA0x0008
> +#define R92C_SECCFG_CMP_A2   0x0010
> +#define R92C_SECCFG_MC_SRCH_DIS  0x0020
> +#define R92C_SECCFG_TXBCKEY_DEF 0x0040
> +#define R92C_SECCFG_RXBCKEY_DEF 0x0080
> +
>  /* IMR */
>   
>  /*Beacon DMA interrupt 6 */
> Index: ic/rtwn.c
> ===
> RCS file: /cvs/src/sys/dev/ic/rtwn.c,v
> retrieving revision 1.49
> diff -u -p -r1.49 rtwn.c
> --- ic/rtwn.c 9 Jan 2020 14:35:19 -   1.49
> +++ ic/rtwn.c 5 Jun 2020 11:52:22 -
> @@ -3154,6 +3154,14 @@ rtwn_init(struct ifnet *ifp)
>   /* Clear per-station keys table. */
>   rtwn_cam_init(sc);
>  
> + /* Enable decryption / encryption. */
> + if (sc->chip & RTWN_CHIP_USB) {
> + rtwn_write_2(sc, R92C_SECCFG,
> + R92C_SECCFG_TXUCKEY_DEF | R92C_SECCFG_RXUCKEY_DEF |
> + R92C_SECCFG_TXENC_ENA | R92C_SECCFG_RXENC_ENA |
> + R92C_SECCFG_TXBCKEY_DEF | R92C_SECCFG_RXBCKEY_DEF);
> + }
> +
>   /* Enable hardware sequence numbering. */
>   rtwn_write_1(sc, R92C_HWSEQ_CTRL, 0xff);
>  
> @@ -3204,14 +3212,14 @@ rtwn_init(struct ifnet *ifp)
>   ifq_clr_oactive(>if_snd);
>   ifp->if_flags |= IFF_RUNNING;
>  
> -#ifdef notyet
> - if (ic->ic_flags & IEEE80211_F_WEPON) {
> + if ((ic->ic_flags & IEEE80211_F_WEPON) &&
> + (sc->chip & RTWN_CHIP_USB)) {
>   /* Install WEP keys. */
>   for (i = 0; i < IEEE80211_WEP_NKID; i++)
>   ic->ic_set_key(ic, NULL, >ic_nw_keys[i]);
>   sc->sc_ops.wait_async(sc->sc_ops.cookie);
>   }
> -#endif
> +
>   if (ic->ic_opmode == IEEE80211_M_MONITOR)
>   ieee80211_new_state(ic, IEEE80211_S_RUN, -1);
>   else
> Index: usb/if_urtwn.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_urtwn.c,v
> retrieving revision 1.89
> diff -u -p -r1.89 if_urtwn.c
> --- usb/if_urtwn.c26 May 2020 06:04:30 -  1.89
> +++ usb/if_urtwn.c5 Jun 2020 11:52:22 -
> @@ -490,10 +490,8 @@ urtwn_attach(struct device *parent, stru
>  
>   ic->ic_updateslot = urtwn_updateslot;
>   ic->ic_updateedca = urtwn_updateedca;
> -#ifdef notyet
>   ic->ic_set_key = urtwn_set_key;
>   ic->ic_delete_key = urtwn_delete_key;
> -#endif
>   /* Override state transition machine. */
>   ic->ic_newstate = urtwn_newstate;
>  
> @@ -1035,6 +1033,10 @@ urtwn_set_key(struct ieee80211com *ic, s
>   struct urtwn_softc *sc = (struct urtwn_softc *)self;
>   struct urtwn_cmd_key cmd;
>  
> + /* Only handle keys for CCMP */
> + if (k->k_cipher != IEEE80211_CIPHER_CCMP)
> + return ieee80211_set_key(ic, ni, k);
> +
>   /* Defer setting of WEP keys until interface is brought up. */
>   if ((ic->ic_if.if_flags & (IFF_UP | IFF_RUNNING)) !=
>   (IFF_UP | IFF_RUNNING))
> @@ -1065,6 +1067,12 @@ urtwn_delete_key(struct ieee80211com *ic
>   struct urtwn_softc *sc = (struct urtwn_softc *)self;
>   struct urtwn_cmd_key cmd;
>  
> + /* Only handle keys for CCMP */
> + if (k->k_cipher != IEEE80211_CIPHER_CCMP) {
> + ieee80211_delete_key(ic, ni, k);
> + return;
> + }
> +
>   if (!(ic->ic_if.if_flags & IFF_RUNNING) ||
>   ic->ic_state != IEEE80211_S_RUN)
>   return; /* Nothing to do. */
> @@ -1084,6 +1092,52 @@ urtwn_delete_key_cb(struct urtwn_softc *
>   rtwn_delete_key(ic, cmd->ni, >key);
> 

Re: userland clock_gettime proof of concept

2020-06-08 Thread Stuart Henderson
On 2020/06/08 12:59, Paul Irofti wrote:
> This iteration of the diff adds bounds checking for tk_user and moves
> the usertc.c stub to every arch in libc as recommanded by deraadt@.
> It also fixes a gettimeofday issue reported by cheloha@ and tb@.
> 
> The acpihpet stub is still there, but it will be removed in the final
> diff.

Good timing as I wanted to update my kernel for the DRM update anyway
now that I don't have to pick that out of git :)

FWIW it's still working as expected on my uninteresting amd64.



Re: powerpc64: ldbrx/stdbrx for endian.h?

2020-06-08 Thread David Gwynne



> On 8 Jun 2020, at 9:28 pm, Christian Weisgerber  wrote:
> 
> powerpc has byte-swapping 16 and 32-bit load/stores and we use those
> in .
> 
> Starting with POWER7 (Power ISA v.2.06), there are also corresponding
> 64-bit instructions.  Do we want to use those on powerpc64?  Or do
> we want to keep compatibility with older processors?

I'm ok with using the instructions. I can't think of what benefit compat in 
this space would actually provide.

Did they also happen to add opcodes for doing swaps in registers?

dlg

> Index: arch/powerpc64/include/endian.h
> ===
> RCS file: /cvs/src/sys/arch/powerpc64/include/endian.h,v
> retrieving revision 1.1
> diff -u -p -r1.1 endian.h
> --- arch/powerpc64/include/endian.h   16 May 2020 17:11:14 -  1.1
> +++ arch/powerpc64/include/endian.h   8 Jun 2020 11:16:33 -
> @@ -36,7 +36,7 @@ __mswap16(volatile const __uint16_t *m)
> 
>   __asm("lhbrx %0, 0, %1"
>   : "=r" (v)
> -: "r" (m), "m" (*m));
> + : "r" (m), "m" (*m));
> 
>   return (v);
> }
> @@ -48,7 +48,7 @@ __mswap32(volatile const __uint32_t *m)
> 
>   __asm("lwbrx %0, 0, %1"
>   : "=r" (v)
> -: "r" (m), "m" (*m));
> + : "r" (m), "m" (*m));
> 
>   return (v);
> }
> @@ -56,11 +56,11 @@ __mswap32(volatile const __uint32_t *m)
> static inline __uint64_t
> __mswap64(volatile const __uint64_t *m)
> {
> - __uint32_t *a = (__uint32_t *)m;
>   __uint64_t v;
> 
> - v = (__uint64_t)__mswap32(a + 1) << 32 |
> - (__uint64_t)__mswap32(a);
> + __asm("ldbrx %0, 0, %1"
> + : "=r" (v)
> + : "r" (m), "m" (*m));
> 
>   return (v);
> }
> @@ -84,10 +84,9 @@ __swapm32(volatile __uint32_t *m, __uint
> static inline void
> __swapm64(volatile __uint64_t *m, __uint64_t v)
> {
> - __uint32_t *a = (__uint32_t *)m;
> -
> - __swapm32(a + 1, v >> 32);
> - __swapm32(a, v);
> + __asm("stdbrx %1, 0, %2"
> + : "=m" (*m)
> + : "r" (v), "r" (m));
> }
> 
> #define __HAVE_MD_SWAPIO
> -- 
> Christian "naddy" Weisgerber  na...@mips.inka.de
> 



powerpc64: ldbrx/stdbrx for endian.h?

2020-06-08 Thread Christian Weisgerber
powerpc has byte-swapping 16 and 32-bit load/stores and we use those
in .

Starting with POWER7 (Power ISA v.2.06), there are also corresponding
64-bit instructions.  Do we want to use those on powerpc64?  Or do
we want to keep compatibility with older processors?


Index: arch/powerpc64/include/endian.h
===
RCS file: /cvs/src/sys/arch/powerpc64/include/endian.h,v
retrieving revision 1.1
diff -u -p -r1.1 endian.h
--- arch/powerpc64/include/endian.h 16 May 2020 17:11:14 -  1.1
+++ arch/powerpc64/include/endian.h 8 Jun 2020 11:16:33 -
@@ -36,7 +36,7 @@ __mswap16(volatile const __uint16_t *m)
 
__asm("lhbrx %0, 0, %1"
: "=r" (v)
-: "r" (m), "m" (*m));
+   : "r" (m), "m" (*m));
 
return (v);
 }
@@ -48,7 +48,7 @@ __mswap32(volatile const __uint32_t *m)
 
__asm("lwbrx %0, 0, %1"
: "=r" (v)
-: "r" (m), "m" (*m));
+   : "r" (m), "m" (*m));
 
return (v);
 }
@@ -56,11 +56,11 @@ __mswap32(volatile const __uint32_t *m)
 static inline __uint64_t
 __mswap64(volatile const __uint64_t *m)
 {
-   __uint32_t *a = (__uint32_t *)m;
__uint64_t v;
 
-   v = (__uint64_t)__mswap32(a + 1) << 32 |
-   (__uint64_t)__mswap32(a);
+   __asm("ldbrx %0, 0, %1"
+   : "=r" (v)
+   : "r" (m), "m" (*m));
 
return (v);
 }
@@ -84,10 +84,9 @@ __swapm32(volatile __uint32_t *m, __uint
 static inline void
 __swapm64(volatile __uint64_t *m, __uint64_t v)
 {
-   __uint32_t *a = (__uint32_t *)m;
-
-   __swapm32(a + 1, v >> 32);
-   __swapm32(a, v);
+   __asm("stdbrx %1, 0, %2"
+   : "=m" (*m)
+   : "r" (v), "r" (m));
 }
 
 #define __HAVE_MD_SWAPIO
-- 
Christian "naddy" Weisgerber  na...@mips.inka.de



Re: userland clock_gettime proof of concept

2020-06-08 Thread Paul Irofti
On Fri, Jun 05, 2020 at 08:34:12PM +0300, Paul Irofti wrote:
> On 05.06.2020 20:25, Mark Kettenis wrote:
> > > Date: Fri, 5 Jun 2020 01:33:16 +0300
> > > From: Paul Irofti 
> > > 
> > > On Wed, Jun 03, 2020 at 05:13:42PM +0300, Paul Irofti wrote:
> > > > On 2020-05-31 20:46, Mark Kettenis wrote:
> > > > > Forget about all that for a moment.  Here is an alternative 
> > > > > suggestion:
> > > > > 
> > > > > On sparc64 we need to support both tick_timecounter and
> > > > > sys_tick_timecounter.  So we need some sort of clockid value to
> > > > > distnguish between those two.  I already suggested to use the tc_user
> > > > > field of the timecounter for that.  0 means that a timecounter is not
> > > > > usable in userland, a (small) positive integer means a specific
> > > > > timecounter type.  The code in libc will need to know whether a
> > > > > particular timecounter type can be supported.  My proposal would be to
> > > > > implement a function*on all architecture*  that takes the clockid as
> > > > > an argument and returns a pointer to the function that implements
> > > > > support for that timecounter.  On architectures without support, ir
> > > > > when called with a clockid that isn't supported, that function would
> > > > > simply return NULL.
> > > > 
> > > > I am sorry, but the more I try to implement this in a sane way, the more
> > > > obvious it is that it is not possible. I would rather have a define 
> > > > sausage
> > > > than something like this.
> > > > 
> > > > I will try to think of something else that avoids the defines, but I do 
> > > > not
> > > > think that your proposal is a valid solution.
> > > 
> > > OK. I think I found an elegant way around this using the Makefile
> > > system: if usertc.c is not present in the arch/${MACHINE}/gen, then a
> > > stub gen/usertc.c file is built that just sets the function pointer to
> > > NULL. This avoids the need for the define checks in dlfcn/init.c and I
> > > think fixes the rest of the issues discussed around this bit.
> > > 
> > > Also included in the diff are a few other fixes and regression tests.
> > > I left the rdtsc and acpihpet example (with no functional acpihpet
> > > support) just to show-case how we can handle multiple clocks on
> > > architectures that have them.
> > 
> > You're still using tk_user unconditionally.  If the kernel returns a
> > tk_user value that is larger than what's supported by libc you have an
> > out-of-bounds array access.
> > 
> > Also if the machine switches to a timecounter that has tk_user == 0
> > you have an out-of-bounds array access.  If that happens you need to
> > detect this and fall back on the system call.
> 
> Right. Even though we test in the beginning for tk_user=0 it might change
> until the access to tc_get_timecount(). I will fix this in my next diff.
> Thanks!

Hi,

This iteration of the diff adds bounds checking for tk_user and moves
the usertc.c stub to every arch in libc as recommanded by deraadt@.
It also fixes a gettimeofday issue reported by cheloha@ and tb@.

The acpihpet stub is still there, but it will be removed in the final
diff.

Paul


diff --git lib/libc/arch/aarch64/gen/Makefile.inc 
lib/libc/arch/aarch64/gen/Makefile.inc
index a7b1b73f3ef..ee198f5d611 100644
--- lib/libc/arch/aarch64/gen/Makefile.inc
+++ lib/libc/arch/aarch64/gen/Makefile.inc
@@ -9,4 +9,4 @@ SRCS+=  fpgetmask.c fpgetround.c fpgetsticky.c
 SRCS+= fpsetmask.c fpsetround.c fpsetsticky.c
 SRCS+= fpclassifyl.c
 SRCS+= isfinitel.c isinfl.c isnanl.c isnormall.c
-SRCS+= signbitl.c
+SRCS+= signbitl.c usertc.c
diff --git lib/libc/arch/aarch64/gen/usertc.c lib/libc/arch/aarch64/gen/usertc.c
new file mode 100644
index 000..84a112c2ea3
--- /dev/null
+++ lib/libc/arch/aarch64/gen/usertc.c
@@ -0,0 +1,21 @@
+/* $OpenBSD$   */
+/*
+ * Copyright (c) 2020 Paul Irofti 
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include 
+#include 
+
+int (*const _tc_get_timecount)(struct timekeep *, uint64_t *) = NULL;
diff --git lib/libc/arch/alpha/gen/Makefile.inc 
lib/libc/arch/alpha/gen/Makefile.inc
index a44599d2cab..2a8abd32b61 100644
--- lib/libc/arch/alpha/gen/Makefile.inc
+++ lib/libc/arch/alpha/gen/Makefile.inc
@@ -3,5 +3,5 @@
 
 SRCS+= _setjmp.S fabs.S infinity.c ldexp.c modf.c nan.c setjmp.S
 SRCS+= flt_rounds.c 

Re: pppx(4): rework to be clese to pseudo-interfaces

2020-06-08 Thread Martin Pieuchot
On 29/05/20(Fri) 13:22, Vitaliy Makkoveev wrote:
> This time pppx_add_session() has mixed initialisation order. It starts
> to initialise pipex(4) session, then initialises `ifnet', then links
> pipex(4) session, then continue to initialize `ifnet'.
> pppx_add_session() can sleep and pppx_if_start() can start to work with
> unlinked pipex(4) session.
> 
> Also pppx_if_destroy() can sleep and pppx_if_start() can access to this
> `pxi' with unlinked session. pppx_if_start() has checking of
> `IFF_RUNNING' flag but it's usesless.
> 
> Diff below does pppx_add_session() reordering. At first we initilize
> and attach `ifnet', at second we initialize and link pipex(4) session
> and at last we set `IFF_RUNNING' flag on `ifnet.

The fact that you have to call if_detach() if a field isn't validated
makes me wonder if this refactoring is better than the existing logic.

> Also we cleaning `IFF_RUNNING' flag before pppx(4) session destruction
> in pppx_if_destroy().

This seems unrelated.

> Since we made `ifnet' and pipex(4) session initialization separately, we
> are more close to remove duplicated code.

Can't we do that directly?  There are many blocks in this function:

- Allocate a `pxi'
- Attach an interface (`ifp')
- Insert an address on the interface
- Link the `pxi' with the interface
- Setup the session

Maybe user input validation should be done first. 

> Index: sys/net/if_pppx.c
> ===
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.87
> diff -u -p -r1.87 if_pppx.c
> --- sys/net/if_pppx.c 29 May 2020 06:51:52 -  1.87
> +++ sys/net/if_pppx.c 29 May 2020 09:27:47 -
> @@ -674,16 +674,111 @@ pppx_add_session(struct pppx_dev *pxd, s
>*  the timeout feature until this is fixed.
>*/
>   if (req->pr_timeout_sec != 0)
> - return (EINVAL);
> + return EINVAL;
> +
> + pxi = pool_get(pppx_if_pl, PR_WAITOK | PR_ZERO);
> +
> + /* try to set the interface up */
> + unit = pppx_if_next_unit();
> + if (unit < 0) {
> + error = ENOMEM;
> + goto out;
> + }
> +
> + pxi->pxi_unit = unit;
> + pxi->pxi_key.pxik_session_id = req->pr_session_id;
> + pxi->pxi_key.pxik_protocol = req->pr_protocol;
> + pxi->pxi_dev = pxd;
> +
> + /* this is safe without splnet since we're not modifying it */
> + if (RBT_FIND(pppx_ifs, _ifs, pxi) != NULL) {
> + error = EADDRINUSE;
> + goto out;
> + }
> +
> + if (RBT_INSERT(pppx_ifs, _ifs, pxi) != NULL)
> + panic("%s: pppx_ifs modified while lock was held", __func__);
> + LIST_INSERT_HEAD(>pxd_pxis, pxi, pxi_list);
> +
> + ifp = >pxi_if;
> + snprintf(ifp->if_xname, sizeof(ifp->if_xname), "%s%d", "pppx", unit);
> + ifp->if_mtu = req->pr_peer_mru; /* XXX */
> + ifp->if_flags = IFF_POINTOPOINT | IFF_MULTICAST | IFF_UP;
> + ifp->if_start = pppx_if_start;
> + ifp->if_output = pppx_if_output;
> + ifp->if_ioctl = pppx_if_ioctl;
> + ifp->if_rtrequest = p2p_rtrequest;
> + ifp->if_type = IFT_PPP;
> + IFQ_SET_MAXLEN(>if_snd, 1);
> + ifp->if_softc = pxi;
> + /* ifp->if_rdomain = req->pr_rdomain; */
> +
> + /* XXXSMP breaks atomicity */
> + NET_UNLOCK();
> + if_attach(ifp);
> + NET_LOCK();
> +
> + if_addgroup(ifp, "pppx");
> + if_alloc_sadl(ifp);
> +
> +#if NBPFILTER > 0
> + bpfattach(>if_bpf, ifp, DLT_LOOP, sizeof(u_int32_t));
> +#endif
> +
> + /* XXX ipv6 support?  how does the caller indicate it wants ipv6
> +  * instead of ipv4?
> +  */
> + memset(, 0, sizeof(ifaddr));
> + ifaddr.sin_family = AF_INET;
> + ifaddr.sin_len = sizeof(ifaddr);
> + ifaddr.sin_addr = req->pr_ip_srcaddr;
> +
> + ia = malloc(sizeof (*ia), M_IFADDR, M_WAITOK | M_ZERO);
> +
> + ia->ia_addr.sin_family = AF_INET;
> + ia->ia_addr.sin_len = sizeof(struct sockaddr_in);
> + ia->ia_addr.sin_addr = req->pr_ip_srcaddr;
> +
> + ia->ia_dstaddr.sin_family = AF_INET;
> + ia->ia_dstaddr.sin_len = sizeof(struct sockaddr_in);
> + ia->ia_dstaddr.sin_addr = req->pr_ip_address;
> +
> + ia->ia_sockmask.sin_family = AF_INET;
> + ia->ia_sockmask.sin_len = sizeof(struct sockaddr_in);
> + ia->ia_sockmask.sin_addr = req->pr_ip_netmask;
> +
> + ia->ia_ifa.ifa_addr = sintosa(>ia_addr);
> + ia->ia_ifa.ifa_dstaddr = sintosa(>ia_dstaddr);
> + ia->ia_ifa.ifa_netmask = sintosa(>ia_sockmask);
> + ia->ia_ifa.ifa_ifp = ifp;
> +
> + ia->ia_netmask = ia->ia_sockmask.sin_addr.s_addr;
> +
> + error = in_ifinit(ifp, ia, , 1);
> + if (error) {
> + printf("pppx: unable to set addresses for %s, error=%d\n",
> + ifp->if_xname, error);
> + goto out_detach;
> + }
> +
> + if_addrhooks_run(ifp);
> +
> + /* fake a pipex interface context */
> + pxi->pxi_ifcontext.ifnet_this = ifp;
> + 

Re: rewrite IPv6 source address selection

2020-06-08 Thread Florian Obser
Anyone? Tests on multi homed machines would be particularly interesting.

Thanks,
Florian

On Thu, Jun 04, 2020 at 07:33:32PM +0200, Florian Obser wrote:
> This should be easier to read and follows the 8 rules in Section 5 of
> RFC 6724.
> 
> I tried to hit all (implemented) rules of RFC 6724 and found only one
> behavioural difference, if there are two global unicast addresses
> configured on an interface, like this:
> 
> inet6 fe80::fce1:baff:fed4:35e3%vether1 prefixlen 64 scopeid 0xb3
> inet6 2001:db8:0:1::3 prefixlen 64
> inet6 2001:db8:0:1::5 prefixlen 64
> 
> and the default gateway is at 2001:db8:0:1::1 the old code would pick
> 2001:db8:0:1::5
> as source address to reach an off-link global unicast address while
> the new code will pick
> 2001:db8:0:1::3
> which I believe is better anyway.
> 
> I think the old behaviour comes from this:
>   if (oifp == ifp) /* (a) */
>   goto replace;
> but I have not verified that.
> 
> To review it's probably easiest to apply and read the result.
> 
> Tests, comments, OKs?
> 
> diff --git netinet6/in6.c netinet6/in6.c
> index ca8c78c7b9f..b1e62f4d850 100644
> --- netinet6/in6.c
> +++ netinet6/in6.c
> @@ -1337,11 +1337,7 @@ in6_ifawithscope(struct ifnet *oifp, struct in6_addr 
> *dst, u_int rdomain)
>   return (NULL);
>   }
>  
> - /*
> -  * We search for all addresses on all interfaces from the beginning.
> -  * Comparing an interface with the outgoing interface will be done
> -  * only at the final stage of tiebreaking.
> -  */
> + /* We search for all addresses on all interfaces from the beginning. */
>   TAILQ_FOREACH(ifp, , if_list) {
>   if (ifp->if_rdomain != rdomain)
>   continue;
> @@ -1363,36 +1359,13 @@ in6_ifawithscope(struct ifnet *oifp, struct in6_addr 
> *dst, u_int rdomain)
>   continue;
>  
>   TAILQ_FOREACH(ifa, >if_addrlist, ifa_list) {
> - int tlen = -1, dscopecmp, bscopecmp, matchcmp;
> + int tlen = -1;
>  
>   if (ifa->ifa_addr->sa_family != AF_INET6)
>   continue;
>  
>   src_scope = in6_addrscope(IFA_IN6(ifa));
>  
> -#ifdef ADDRSELECT_DEBUG  /* should be removed after 
> stabilization */
> - {
> - char adst[INET6_ADDRSTRLEN], asrc[INET6_ADDRSTRLEN];
> - char bestaddr[INET6_ADDRSTRLEN];
> -
> -
> - dscopecmp = IN6_ARE_SCOPE_CMP(src_scope, dst_scope);
> - printf("%s: dst=%s bestaddr=%s, "
> - "newaddr=%s, scope=%x, dcmp=%d, bcmp=%d, "
> - "matchlen=%d, flgs=%x\n", __func__,
> - inet_ntop(AF_INET6, dst, adst, sizeof(adst)),
> - (ia6_best == NULL) ? "none" :
> - inet_ntop(AF_INET6, _best->ia_addr.sin6_addr,
> - bestaddr, sizeof(bestaddr)),
> - inet_ntop(AF_INET6, IFA_IN6(ifa),
> - asrc, sizeof(asrc)),
> - src_scope, dscopecmp, ia6_best ?
> - IN6_ARE_SCOPE_CMP(src_scope, best_scope) : -1,
> - in6_matchlen(IFA_IN6(ifa), dst),
> - ifatoia6(ifa)->ia6_flags);
> - }
> -#endif
> -
>   /*
>* Don't use an address before completing DAD
>* nor a duplicated address.
> @@ -1401,7 +1374,16 @@ in6_ifawithscope(struct ifnet *oifp, struct in6_addr 
> *dst, u_int rdomain)
>   (IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED))
>   continue;
>  
> - /* XXX: is there any case to allow anycasts? */
> + /*
> +  * RFC 6724 allows anycast addresses as source address
> +  * because the restriction was removed in RFC 4291.
> +  * However RFC 4443 states that ICMPv6 responses
> +  * MUST use a unicast source address.
> +  *
> +  * XXX Skip anycast addresses for now since
> +  * icmp6_reflect() uses this function for source
> +  * address selection.
> +  */
>   if (ifatoia6(ifa)->ia6_flags & IN6_IFF_ANYCAST)
>   continue;
>  
> @@ -1421,26 +1403,26 @@ in6_ifawithscope(struct ifnet *oifp, struct in6_addr 
> *dst, u_int rdomain)
>*/
>  
>   /*
> -  * If ia6_best has a smaller scope than dst and
> -  * the current address has a larger one than
> -  * (or equal to) dst, always replace ia6_best.
> -  * Also, if