Re: CVS commit: src/sys/arch/x86/x86

[Paul Goyette]

hehehe - porn iterators - love it!


On Fri, 15 Oct 2021, Jason Thorpe wrote:


I demand this change be reverted.

(/s)


On Oct 15, 2021, at 11:12 AM, Jared D. McNeill  wrote:

Module Name:src
Committed By:   jmcneill
Date:   Fri Oct 15 18:12:48 UTC 2021

Modified Files:
src/sys/arch/x86/x86: tsc.c

Log Message:
Fix typo in comment: "porniters" -> "pointers"


To generate a diff of this commit:
cvs rdiff -u -r1.56 -r1.57 src/sys/arch/x86/x86/tsc.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.



-- thorpej


!DSPAM:6169cf82254421105921466!




++--+--+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses:|
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com|
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org  |
| & Network Engineer |  | pgoyett...@gmail.com |
++--+--+

Re: CVS commit: src/sys/arch/x86/x86

[Jason Thorpe]

I demand this change be reverted.

(/s)

> On Oct 15, 2021, at 11:12 AM, Jared D. McNeill  wrote:
> 
> Module Name:  src
> Committed By: jmcneill
> Date: Fri Oct 15 18:12:48 UTC 2021
> 
> Modified Files:
>   src/sys/arch/x86/x86: tsc.c
> 
> Log Message:
> Fix typo in comment: "porniters" -> "pointers"
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.56 -r1.57 src/sys/arch/x86/x86/tsc.c
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
> 

-- thorpej

Re: CVS commit: src/sys/arch/x86

[Taylor R Campbell]

> Module Name:src
> Committed By:   jdolecek
> Date:   Sat Aug  1 12:36:36 UTC 2020
> 
> Modified Files:
> src/sys/arch/x86/pci: pci_intr_machdep.c
> src/sys/arch/x86/x86: mainbus.c
> 
> Log Message:
> reorder includes to pull __HAVE_PCI_MSI_MSIX properly via
> 

If  requires a file to be included
("opt_pci.h"?) in order for its definition of __HAVE_PCI_MSI_MSIX to
work, why doesn't  include that file
directly?

Or, if I didn't follow exactly what's going on here, why is it
necessary to reorder the includes?

I think we should generally treat it as a bug if including two header
files in different orders gives different outcomes, and fix it by
fixing the header files rather than by adjusting the ordering of
includes in every file where they're used.

re: CVS commit: src/sys/arch/x86/x86

[matthew green]

"Jaromir Dolecek" writes:
> Module Name:  src
> Committed By: jdolecek
> Date: Wed Jun 24 22:28:08 UTC 2020
> 
> Modified Files:
>   src/sys/arch/x86/x86: multiboot2.c
> 
> Log Message:
> don't try allocating 16KB of scratch space on stack
> 
> it's too early for kmem_alloc(), so use static variable in BSS; it's used
> post reloc, so don't need to use the RELOC() macros
> 
> XXX compile-tested only on i386

this one is quite a lot of lost wasted space.  can you find a
way to reuse these pages?  or use the x86-specific ways to
get early memory...

thanks.


.mrg.

Re: CVS commit: src/sys/arch/x86/x86

[Joerg Sonnenberger]

On Wed, Jun 24, 2020 at 10:28:08PM +, Jaromir Dolecek wrote:
> Module Name:  src
> Committed By: jdolecek
> Date: Wed Jun 24 22:28:08 UTC 2020
> 
> Modified Files:
>   src/sys/arch/x86/x86: multiboot2.c
> 
> Log Message:
> don't try allocating 16KB of scratch space on stack
> 
> it's too early for kmem_alloc(), so use static variable in BSS; it's used
> post reloc, so don't need to use the RELOC() macros

Why? This is the initial early stack, not the normal LWP stack.

Joerg

Re: CVS commit: src/sys/arch/x86/x86

[Christos Zoulas]

In article <20200606135850.ge14...@pony.stderr.spb.ru>,
Valery Ushakov   wrote:
>On Sat, Jun 06, 2020 at 11:25:19 +0200, Kamil Rytarowski wrote:
>
>> On 06.06.2020 09:42, Simon Burge wrote:
>> > "Kamil Rytarowski" wrote:
>> > 
>> >> Module Name:  src
>> >> Committed By: kamil
>> >> Date: Fri Jun  5 21:48:04 UTC 2020
>> >>
>> >> Modified Files:
>> >>
>> >>   src/sys/arch/x86/x86: cpu_rng.c
>> >>
>> >> Log Message:
>> >>
>> >> Change const unsigned to preprocessor define
>> >>
>> >> Fixes GCC -O0 build with the stack protector.
>> > 
>> > Surely a gcc bug?  This almost certainly needs an
>> > /* XXX gcc stack protector -O0 bug */ comment and
>> > possibly an entry in doc/HACKS as well otherwise
>> > someone will come along later and de-uglify this
>> > change.
>> 
>> This is not really a GCC bug, as C const is not constexpr.  It's
>> also not the only place with such logic and such workaround.  C++
>> fixed it and have real const.
>
>Doesn't -Wvla help catching these?  Should we enable it?

I think it might catch too much... But we can try it...

christos

Re: CVS commit: src/sys/arch/x86/x86

[Valery Ushakov]

On Sat, Jun 06, 2020 at 11:25:19 +0200, Kamil Rytarowski wrote:

> On 06.06.2020 09:42, Simon Burge wrote:
> > "Kamil Rytarowski" wrote:
> > 
> >> Module Name:   src
> >> Committed By:  kamil
> >> Date:  Fri Jun  5 21:48:04 UTC 2020
> >>
> >> Modified Files:
> >>
> >>src/sys/arch/x86/x86: cpu_rng.c
> >>
> >> Log Message:
> >>
> >> Change const unsigned to preprocessor define
> >>
> >> Fixes GCC -O0 build with the stack protector.
> > 
> > Surely a gcc bug?  This almost certainly needs an
> > /* XXX gcc stack protector -O0 bug */ comment and
> > possibly an entry in doc/HACKS as well otherwise
> > someone will come along later and de-uglify this
> > change.
> 
> This is not really a GCC bug, as C const is not constexpr.  It's
> also not the only place with such logic and such workaround.  C++
> fixed it and have real const.

Doesn't -Wvla help catching these?  Should we enable it?

-uwe

Re: CVS commit: src/sys/arch/x86/x86

[Kamil Rytarowski]

On 06.06.2020 09:42, Simon Burge wrote:
> "Kamil Rytarowski" wrote:
> 
>> Module Name: src
>> Committed By:kamil
>> Date:Fri Jun  5 21:48:04 UTC 2020
>>
>> Modified Files:
>>
>>  src/sys/arch/x86/x86: cpu_rng.c
>>
>> Log Message:
>>
>> Change const unsigned to preprocessor define
>>
>> Fixes GCC -O0 build with the stack protector.
> 
> Surely a gcc bug?  This almost certainly needs an
> /* XXX gcc stack protector -O0 bug */ comment and
> possibly an entry in doc/HACKS as well otherwise
> someone will come along later and de-uglify this
> change.
> 
> Cheers,
> Simon.
> 

This is not really a GCC bug, as C const is not constexpr. It's also not
the only place with such logic and such workaround. C++ fixed it and
have real const.



signature.asc
Description: OpenPGP digital signature

Re: CVS commit: src/sys/arch/x86/x86

[Simon Burge]

"Kamil Rytarowski" wrote:

> Module Name:  src
> Committed By: kamil
> Date: Fri Jun  5 21:48:04 UTC 2020
>
> Modified Files:
>
>   src/sys/arch/x86/x86: cpu_rng.c
>
> Log Message:
>
> Change const unsigned to preprocessor define
>
> Fixes GCC -O0 build with the stack protector.

Surely a gcc bug?  This almost certainly needs an
/* XXX gcc stack protector -O0 bug */ comment and
possibly an entry in doc/HACKS as well otherwise
someone will come along later and de-uglify this
change.

Cheers,
Simon.

Re: CVS commit: src/sys/arch/x86/x86

[Kengo NAKAHARA]

Hi,

Hmm, but TSC drift is still observed on recent (high clock) CPUs.
I will have researched and fix later.

On 2020/04/03 12:05, Kengo NAKAHARA wrote:

Module Name:src
Committed By:   knakahara
Date:   Fri Apr  3 03:05:39 UTC 2020

Modified Files:
src/sys/arch/x86/x86: tsc.c

Log Message:
Fix TSC drift is observed almost every time wrongly.

Ths "TSC drift" in tsc_tc_init() means the cpu_cc_skew delta between
first measurement (in cpu_start_secondary) and second measurement
(in cpu_boot_secondary), that is, the TSC drift is expected to be
almost zero.  However, the second measument in current implementation
is added extra cpu_cc_skew accidentally, so current delta value means
almost cpu_cc_skew wrongly.

tsc_sync_bp and tsc_sync_ap should use rdtsc() to get raw values.

Advised by nonaka@n.o, thanks.


To generate a diff of this commit:
cvs rdiff -u -r1.38 -r1.39 src/sys/arch/x86/x86/tsc.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.



--
//
Internet Initiative Japan Inc.

Device Engineering Section,
Product Development Department,
Product Division,
Technology Unit

Kengo NAKAHARA 

Re: CVS commit: src/sys/arch/x86/acpi

[Martin Husemann]

On Wed, Mar 18, 2020 at 09:44:03PM +0200, Yorick Hardy wrote:
> Dear Andrew,
> 
> On 2020-03-14, Andrew Doran wrote:
> > Module Name:src
> > Committed By:   ad
> > Date:   Sat Mar 14 13:50:46 UTC 2020
> > 
> > Modified Files:
> > src/sys/arch/x86/acpi: acpi_cpu_md.c
> > 
> > Log Message:
> > Put ACPI idle under ACPICPU_ENABLE_C3 until the wrinkles are ironed out.
> > This seems well written and basically all good, but currently doesn't enter
> > a low power state, and imposes a big performance penalty.  Proposed on
> > port-i386 & port-amd64.
> > 
> > 
> > To generate a diff of this commit:
> > cvs rdiff -u -r1.81 -r1.82 src/sys/arch/x86/acpi/acpi_cpu_md.c
> > 
> > Please note that diffs are not public domain; they are subject to the
> > copyright notices on the relevant files.
> 
> This change seems to cause my machine to hang while booting. Reverting
> the change seems to fix the problem. Here are the details:

Same here, see kern/55080.

Martin

Re: CVS commit: src/sys/arch/x86/acpi

[Yorick Hardy]

Dear Andrew,

On 2020-03-14, Andrew Doran wrote:
> Module Name:  src
> Committed By: ad
> Date: Sat Mar 14 13:50:46 UTC 2020
> 
> Modified Files:
>   src/sys/arch/x86/acpi: acpi_cpu_md.c
> 
> Log Message:
> Put ACPI idle under ACPICPU_ENABLE_C3 until the wrinkles are ironed out.
> This seems well written and basically all good, but currently doesn't enter
> a low power state, and imposes a big performance penalty.  Proposed on
> port-i386 & port-amd64.
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.81 -r1.82 src/sys/arch/x86/acpi/acpi_cpu_md.c
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.

This change seems to cause my machine to hang while booting. Reverting
the change seems to fix the problem. Here are the details:

  $ cpuctl identify 0
  cpu0: highest basic info 0005
  cpu0: highest extended info 801b
  cpu0: "AMD Athlon(tm) II X3 450 Processor"
  cpu0: AMD Family 10h (686-class), 3200.22 MHz
  cpu0: family 0x10 model 0x5 stepping 0x3 (id 0x100f53)
  cpu0: features 
0x178bfbff
  cpu0: features 0x178bfbff
  cpu0: features1 0x802009
  cpu0: features2 0xefd3fbff
  cpu0: features2 0xefd3fbff
  cpu0: features3 0x37ff
  cpu0: features3 0x37ff
  cpu0: I-cache 64KB 64B/line 2-way, D-cache 64KB 64B/line 2-way
  cpu0: L2 cache 512KB 64B/line 16-way
  cpu0: ITLB 32 4KB entries fully associative, 16 2MB entries fully associative
  cpu0: DTLB 48 4KB entries fully associative, 48 2MB entries fully associative
  cpu0: L2 ITLB 512 4KB entries 4-way
  cpu0: L2 DTLB 512 4KB entries 4-way, 128 2MB entries 2-way
  cpu0: L1 1GB page DTLB 48 1GB entries fully associative
  cpu0: L2 1GB page DTLB 16 1GB entries 8-way
  cpu0: Initial APIC ID 0
  cpu0: Cluster/Package ID 0
  cpu0: Core ID 0
  cpu0: SMT ID 0
  cpu0: MONITOR/MWAIT extensions 0x3
  cpu0: monitor-line size 64
  cpu0: AMD Power Management features: 0x1f9
  cpu0: SVM Rev. 1
  cpu0: SVM NASID 64
  cpu0: SVM features 0xf
  cpu0: UCode version: 0x1c8
  
  $ dmesg | fgrep acpicpu
  [ 1.014781] acpicpu0 at cpu0: ACPI CPU
  [ 1.014781] acpicpu0: C1: HLT, lat   0 us, pow 0 mW
  [ 1.014781] acpicpu0: P0: FFH, lat   4 us, pow 34375 mW, 3200 MHz
  [ 1.014781] acpicpu0: P1: FFH, lat   4 us, pow 25245 mW, 2500 MHz
  [ 1.014781] acpicpu0: P2: FFH, lat   4 us, pow 22200 mW, 2000 MHz
  [ 1.014781] acpicpu0: P3: FFH, lat   4 us, pow 12095 mW,  800 MHz
  [ 1.014781] acpicpu0: T0: I/O, lat   1 us, pow 0 mW, 100 %
  [ 1.014781] acpicpu0: T1: I/O, lat   1 us, pow 0 mW,  88 %
  [ 1.014781] acpicpu0: T2: I/O, lat   1 us, pow 0 mW,  76 %
  [ 1.014781] acpicpu0: T3: I/O, lat   1 us, pow 0 mW,  64 %
  [ 1.014781] acpicpu0: T4: I/O, lat   1 us, pow 0 mW,  52 %
  [ 1.014781] acpicpu0: T5: I/O, lat   1 us, pow 0 mW,  40 %
  [ 1.014781] acpicpu0: T6: I/O, lat   1 us, pow 0 mW,  28 %
  [ 1.014781] acpicpu0: T7: I/O, lat   1 us, pow 0 mW,  16 %
  [ 1.014781] acpicpu1 at cpu1: ACPI CPU
  [ 1.014781] acpicpu2 at cpu2: ACPI CPU
  [ 1.017385] acpicpu0 at cpu0: ACPI CPU
  [ 1.017385] acpicpu0: C1: HLT, lat   0 us, pow 0 mW
  [ 1.017385] acpicpu0: P0: FFH, lat   4 us, pow 34375 mW, 3200 MHz
  [ 1.017385] acpicpu0: P1: FFH, lat   4 us, pow 25245 mW, 2500 MHz
  [ 1.017385] acpicpu0: P2: FFH, lat   4 us, pow 22200 mW, 2000 MHz
  [ 1.017385] acpicpu0: P3: FFH, lat   4 us, pow 12095 mW,  800 MHz
  [ 1.017385] acpicpu0: T0: I/O, lat   1 us, pow 0 mW, 100 %
  [ 1.017385] acpicpu0: T1: I/O, lat   1 us, pow 0 mW,  88 %
  [ 1.017385] acpicpu0: T2: I/O, lat   1 us, pow 0 mW,  76 %
  [ 1.017385] acpicpu0: T3: I/O, lat   1 us, pow 0 mW,  64 %
  [ 1.017385] acpicpu0: T4: I/O, lat   1 us, pow 0 mW,  52 %
  [ 1.017385] acpicpu0: T5: I/O, lat   1 us, pow 0 mW,  40 %
  [ 1.017385] acpicpu0: T6: I/O, lat   1 us, pow 0 mW,  28 %
  [ 1.017385] acpicpu0: T7: I/O, lat   1 us, pow 0 mW,  16 %
  [ 1.017385] acpicpu1 at cpu1: ACPI CPU
  [ 1.017385] acpicpu2 at cpu2: ACPI CPU

-- 
Kind regards,

Yorick Hardy

Re: CVS commit: src/sys/arch/x86/x86

[Andrew Doran]

On Tue, Mar 17, 2020 at 10:38:14PM +, Andrew Doran wrote:

> Log Message:
> - Change some expensive checks DEBUG -> DIAGNOSTIC.

That was meant to be the other way around, oops.

Andrew

Re: CVS commit: src/sys/arch/x86

[Jason Thorpe]

We should absolutely verify this under DEBUG.

-- thorpej
Sent from my iPhone.

> On Jan 12, 2020, at 11:25 AM, Joerg Sonnenberger  wrote:
> 
> On Sun, Jan 12, 2020 at 01:01:12PM +, Andrew Doran wrote:
>> Module Name:src
>> Committed By:ad
>> Date:Sun Jan 12 13:01:12 UTC 2020
>> 
>> Modified Files:
>>src/sys/arch/x86/include: pmap.h pmap_pv.h
>>src/sys/arch/x86/x86: pmap.c vm_machdep.c x86_tlb.c
>> 
>> Log Message:
>> x86 pmap:
>> 
>> - It turns out that every page the pmap frees is necessarily zeroed.  Tell
>>  the VM system about this and use the pmap as a source of pre-zeroed pages.
> 
> Would it make sense to actually verify this under DEBUG? I fear the
> debugging we have to do if a chance ever violates this assumption.
> 
> Joerg

Re: CVS commit: src/sys/arch/x86

[Andrew Doran]

On Sun, Jan 12, 2020 at 08:25:27PM +0100, Joerg Sonnenberger wrote:
> On Sun, Jan 12, 2020 at 01:01:12PM +, Andrew Doran wrote:
> > Module Name:src
> > Committed By:   ad
> > Date:   Sun Jan 12 13:01:12 UTC 2020
> > 
> > Modified Files:
> > src/sys/arch/x86/include: pmap.h pmap_pv.h
> > src/sys/arch/x86/x86: pmap.c vm_machdep.c x86_tlb.c
> > 
> > Log Message:
> > x86 pmap:
> > 
> > - It turns out that every page the pmap frees is necessarily zeroed.  Tell
> >   the VM system about this and use the pmap as a source of pre-zeroed pages.
> 
> Would it make sense to actually verify this under DEBUG? I fear the
> debugging we have to do if a chance ever violates this assumption.

Yup we already do exactly this!

Andrew

Re: CVS commit: src/sys/arch/x86

[Joerg Sonnenberger]

On Sun, Jan 12, 2020 at 01:01:12PM +, Andrew Doran wrote:
> Module Name:  src
> Committed By: ad
> Date: Sun Jan 12 13:01:12 UTC 2020
> 
> Modified Files:
>   src/sys/arch/x86/include: pmap.h pmap_pv.h
>   src/sys/arch/x86/x86: pmap.c vm_machdep.c x86_tlb.c
> 
> Log Message:
> x86 pmap:
> 
> - It turns out that every page the pmap frees is necessarily zeroed.  Tell
>   the VM system about this and use the pmap as a source of pre-zeroed pages.

Would it make sense to actually verify this under DEBUG? I fear the
debugging we have to do if a chance ever violates this assumption.

Joerg

Re: CVS commit: src/sys/arch/x86/x86

[Andrew Doran]

On Tue, Dec 03, 2019 at 01:14:14PM +0100, Kamil Rytarowski wrote:

> On 03.12.2019 12:50, Juergen Hannken-Illjes wrote:
> > Module Name:src
> > Committed By:   hannken
> > Date:   Tue Dec  3 11:50:45 UTC 2019
> > 
> > Modified Files:
> > src/sys/arch/x86/x86: x86_machdep.c
> > 
> > Log Message:
> > Make sure the assignment to "idepth" is done inside the loop to prevent
> > preemption between loop end and dereference of "l_cpu->ci_depth".
> > 
> > 
> > To generate a diff of this commit:
> > cvs rdiff -u -r1.131 -r1.132 src/sys/arch/x86/x86/x86_machdep.c
> > 
> > Please note that diffs are not public domain; they are subject to the
> > copyright notices on the relevant files.
> > 
> > 
> > Modified files:
> > 
> > Index: src/sys/arch/x86/x86/x86_machdep.c
> > diff -u src/sys/arch/x86/x86/x86_machdep.c:1.131 
> > src/sys/arch/x86/x86/x86_machdep.c:1.132
> > --- src/sys/arch/x86/x86/x86_machdep.c:1.131Tue Dec  3 11:50:16 2019
> > +++ src/sys/arch/x86/x86/x86_machdep.c  Tue Dec  3 11:50:45 2019
> > @@ -1,4 +1,4 @@
> > -/* $NetBSD: x86_machdep.c,v 1.131 2019/12/03 11:50:16 hannken Exp $
> > */
> > +/* $NetBSD: x86_machdep.c,v 1.132 2019/12/03 11:50:45 hannken Exp $
> > */
> >  
> >  /*-
> >   * Copyright (c) 2002, 2006, 2007 YAMAMOTO Takashi,
> > @@ -31,7 +31,7 @@
> >   */
> >  
> >  #include 
> > -__KERNEL_RCSID(0, "$NetBSD: x86_machdep.c,v 1.131 2019/12/03 11:50:16 
> > hannken Exp $");
> > +__KERNEL_RCSID(0, "$NetBSD: x86_machdep.c,v 1.132 2019/12/03 11:50:45 
> > hannken Exp $");
> >  
> >  #include "opt_modular.h"
> >  #include "opt_physmem.h"
> > @@ -350,7 +350,7 @@ bool
> >  cpu_intr_p(void)
> >  {
> > uint64_t ncsw;
> > -   int idepth;
> > +   volatile int idepth;
> > lwp_t *l;
> >  
> > l = curlwp;
> > 
> 
> Thanks!
> 
> This looks like to be in need to be propagated to:
> src/sys/arch/arm/arm/arm_machdep.c, src/sys/arch/mips/mips/cpu_subr.c,
> src/sys/arch/sparc/sparc/intr.c, src/sys/arch/sparc64/sparc64/machdep.c,
> src/sys/arch/usermode/dev/cpu.c,

I will take care of that later today.

Andrew

Re: CVS commit: src/sys/arch/x86/x86

[Kamil Rytarowski]

On 03.12.2019 12:50, Juergen Hannken-Illjes wrote:
> Module Name:  src
> Committed By: hannken
> Date: Tue Dec  3 11:50:45 UTC 2019
> 
> Modified Files:
>   src/sys/arch/x86/x86: x86_machdep.c
> 
> Log Message:
> Make sure the assignment to "idepth" is done inside the loop to prevent
> preemption between loop end and dereference of "l_cpu->ci_depth".
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.131 -r1.132 src/sys/arch/x86/x86/x86_machdep.c
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
> 
> 
> Modified files:
> 
> Index: src/sys/arch/x86/x86/x86_machdep.c
> diff -u src/sys/arch/x86/x86/x86_machdep.c:1.131 
> src/sys/arch/x86/x86/x86_machdep.c:1.132
> --- src/sys/arch/x86/x86/x86_machdep.c:1.131  Tue Dec  3 11:50:16 2019
> +++ src/sys/arch/x86/x86/x86_machdep.cTue Dec  3 11:50:45 2019
> @@ -1,4 +1,4 @@
> -/*   $NetBSD: x86_machdep.c,v 1.131 2019/12/03 11:50:16 hannken Exp $
> */
> +/*   $NetBSD: x86_machdep.c,v 1.132 2019/12/03 11:50:45 hannken Exp $
> */
>  
>  /*-
>   * Copyright (c) 2002, 2006, 2007 YAMAMOTO Takashi,
> @@ -31,7 +31,7 @@
>   */
>  
>  #include 
> -__KERNEL_RCSID(0, "$NetBSD: x86_machdep.c,v 1.131 2019/12/03 11:50:16 
> hannken Exp $");
> +__KERNEL_RCSID(0, "$NetBSD: x86_machdep.c,v 1.132 2019/12/03 11:50:45 
> hannken Exp $");
>  
>  #include "opt_modular.h"
>  #include "opt_physmem.h"
> @@ -350,7 +350,7 @@ bool
>  cpu_intr_p(void)
>  {
>   uint64_t ncsw;
> - int idepth;
> + volatile int idepth;
>   lwp_t *l;
>  
>   l = curlwp;
> 

Thanks!

This looks like to be in need to be propagated to:
src/sys/arch/arm/arm/arm_machdep.c, src/sys/arch/mips/mips/cpu_subr.c,
src/sys/arch/sparc/sparc/intr.c, src/sys/arch/sparc64/sparc64/machdep.c,
src/sys/arch/usermode/dev/cpu.c,



signature.asc
Description: OpenPGP digital signature

Re: CVS commit: src/sys/arch/x86

[Alexander Nasonov]

Maxime Villard wrote:
> Module Name:  src
> Committed By: maxv
> Date: Wed Nov 27 06:24:33 UTC 2019
> 
> Modified Files:
>   src/sys/arch/x86/include: cpu.h fpu.h
>   src/sys/arch/x86/x86: cpu.c fpu.c
> 
> Log Message:
> Add a small API for in-kernel FPU operations.
> 
>   fpu_kern_enter();
>   /* do FPU stuff */
>   fpu_kern_leave();

Is it now possible to use AES-NI instructions for cgd disk encryption?

-- 
Alex

Re: CVS commit: src/sys/arch/x86

[Christoph Badura]

Hi,

On Tue, Nov 12, 2019 at 06:00:13PM +, Maxime Villard wrote:
> Committed By: maxv
> Date: Tue Nov 12 18:00:13 UTC 2019
> Modified Files:
>   src/sys/arch/x86/include: specialreg.h
>   src/sys/arch/x86/x86: spectre.c
> Log Message:
> Mitigation for CVE-2019-11135: TSX Asynchronous Abort (TAA).

are you planing to document the new sysctls properly? Preferably without
TLA-soup.

--chris

Re: CVS commit: src/sys/arch/x86/pci

[Kengo NAKAHARA]

Hi,

On 2019/08/19 14:25, Kengo NAKAHARA wrote:

Module Name:src
Committed By:   knakahara
Date:   Mon Aug 19 05:25:38 UTC 2019

Modified Files:
src/sys/arch/x86/pci: if_vmx.c

Log Message:
add vmx(4) basic statistics counters.

Sorry, I have forgotten this TODO in r1.40 commit message.


To generate a diff of this commit:
cvs rdiff -u -r1.47 -r1.48 src/sys/arch/x86/pci/if_vmx.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


I have finished my task for vmx(4), so I think if_vmx.c can be moved
to sys/dev/pci/ directory as pointed out by mrg@n.o.

However, it would require some htole* codes to become MI correctly.
cf. 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/vmxnet3/vmxnet3_drv.c?h=v5.3-rc5=115924b6bdc7cc6bf7da5b933b09281e1f4e17a9


It would be required to test on big endian architectures before moving
to sys/dev/pci directory.


Thanks,

--
//
Internet Initiative Japan Inc.

Device Engineering Section,
Product Development Department,
Product Division,
Technology Unit

Kengo NAKAHARA 

Re: CVS commit: src/sys/arch/x86/x86

[Masanobu SAITOH]

On 2019/05/16 13:20, Jason Thorpe wrote:
> 
> 
>> On May 15, 2019, at 9:19 PM, Maxime Villard  wrote:
>>
>> Le 16/05/2019 à 04:36, SAITOH Masanobu a écrit :
>>> Module Name:src
>>> Committed By:   msaitoh
>>> Date:   Thu May 16 02:36:30 UTC 2019
>>> Modified Files:
>>> src/sys/arch/x86/x86: procfs_machdep.c
>>> Log Message:
>>>  Use ci_feat_val[7] instead of directly getting cpuid 7 edx.
>>> To generate a diff of this commit:
>>> cvs rdiff -u -r1.28 -r1.29 src/sys/arch/x86/x86/procfs_machdep.c
>>> Please note that diffs are not public domain; they are subject to the
>>> copyright notices on the relevant files.
>>
>> The microcode updates CPUID7, if you read ci_feat_val you have the
>> initial value, not the updated value. So reading CPUID7 was the right
>> thing to do.
> 
> Should the microcode update procedure fix up the cached copy?

I think so, but our code doesn't do it.

 There are some code which copy a cpuid value to ci_feat_val[] and
modify some bits for workaround and use the copy. ci_feat_val[]
is better than trusting current cpuid values for consistency.

We should provide user an interface to know the current behavior
of the kernel. I think ci_feat_val[] is the data to explain the
behavior. If so, we should use ci_feat_val[] in x86/procfs_machdep.c.
But, sadly, ci_feat_val[] is not updated, so it might better to
use real cpuid value than ci_feat_val[]. The latest x86/procfs_machdep.c
(rev. 1.31) refers ci_feat_val[0..6] and not refer ci_feat_val[7].
It's inconsistent.

 If we update ci_feat_val, we should have callback function to
reflect the change. Callback is not required for many bits but
it's required for some others.

 I have a code locally to know which bit is changed after updating
microcode:

--
Index: x86/cpu_ucode_intel.c
===
RCS file: /cvsroot/src/sys/arch/x86/x86/cpu_ucode_intel.c,v
retrieving revision 1.17
diff -u -p -r1.17 cpu_ucode_intel.c
--- x86/cpu_ucode_intel.c   10 May 2019 18:21:01 -  1.17
+++ x86/cpu_ucode_intel.c   16 May 2019 04:28:13 -
@@ -178,9 +178,14 @@ cpu_ucode_intel_apply(struct cpu_ucode_s
 {
uint32_t ucodetarget, oucodeversion, nucodeversion;
struct intel1_ucode_header *uh;
+   struct cpu_info *ci;
+   struct cpu_info oldci;
+   int i;
int platformid, cpuid, error;
size_t newbufsize = 0;
void *uha;
+   uint64_t msr = 0;
+   u_int descs[4];
 
if (sc->loader_version != CPU_UCODE_LOADER_INTEL1 ||
cpuno != CPU_UCODE_CURRENT_CPU)
@@ -221,15 +226,62 @@ cpu_ucode_intel_apply(struct cpu_ucode_s
intel_getcurrentucode(, );
cpuid = curcpu()->ci_index;
 
-   kpreempt_enable();
-
if (nucodeversion != ucodetarget) {
+   kpreempt_enable();
error = EIO;
goto out;
}
 
-   printf("cpu %d: ucode 0x%x->0x%x\n", cpuid, oucodeversion,
-   nucodeversion);
+   ci = curcpu();
+   oldci = *ci;
+#if 1
+   /*
+* Update cpu_info.
+*
+* XXX cpu_probe() is currently not intended to call multiple time
+* on a cpu.
+*/
+   cpu_probe(curcpu());
+#endif
+   if (ci->ci_max_cpuid >= 7) {
+   x86_cpuid(7, descs);
+   if (descs[3] & CPUID_SEF_ARCH_CAP)
+   msr = rdmsr(MSR_IA32_ARCH_CAPABILITIES);
+   }
+   kpreempt_enable();
+
+   if ((ci->ci_max_cpuid >= 7) && (descs[3] & CPUID_SEF_ARCH_CAP))
+   printf("cpu%d: MSR_IA32_ARCH_CAPABILITIES=0x%lx\n", cpuid,
+   msr);
+   printf("cpu%d: ucode 0x%x->0x%x\n", cpuid,
+  oucodeversion, nucodeversion);
+   for (i = 0; i < __arraycount(ci->ci_feat_val); i++) {
+   uint32_t old, new, dif, set, unset;
+
+   old = oldci.ci_feat_val[i];
+   new = ci->ci_feat_val[i];
+   if (old != new) {
+   printf("cpu%d: ci_feat_val[%d] changed from %08x to "
+   "%08x\n", cpuid, i, old, new);
+   dif = old ^ new;
+   set = new & dif;
+   unset = old & dif;
+
+   if (set != 0)
+   printf("cpu%d:   set: %08x\n", cpuid, set);
+   if (unset != 0)
+   printf("cpu%d: unset: %08x\n", cpuid, unset);
+
+   /*
+* Call hook if you want to do something.
+*
+* WARNING. If HyperThreading is enabled and the
+* microcode is updated to new one, another CPU's
+* feature bits also be changed and we cannot hook
+* for the CPU here.
+*/
+   }
+   }
 
 out:
if (newbufsize != 0)
--

Re: CVS commit: src/sys/arch/x86/x86

[Masanobu SAITOH]

On 2019/05/16 13:19, Maxime Villard wrote:
> Le 16/05/2019 à 04:36, SAITOH Masanobu a écrit :
>> Module Name:    src
>> Committed By:    msaitoh
>> Date:    Thu May 16 02:36:30 UTC 2019
>>
>> Modified Files:
>> src/sys/arch/x86/x86: procfs_machdep.c
>>
>> Log Message:
>>   Use ci_feat_val[7] instead of directly getting cpuid 7 edx.
>>
>>
>> To generate a diff of this commit:
>> cvs rdiff -u -r1.28 -r1.29 src/sys/arch/x86/x86/procfs_machdep.c
>>
>> Please note that diffs are not public domain; they are subject to the
>> copyright notices on the relevant files.
> 
> The microcode updates CPUID7, if you read ci_feat_val you have the
> initial value, not the updated value. So reading CPUID7 was the right
> thing to do.

 We have no callback or something like it to reflect the change
to ci_feat_val, so you're right.

I'll revert it.

-- 
---
SAITOH Masanobu (msai...@execsw.org
 msai...@netbsd.org)

Re: CVS commit: src/sys/arch/x86/x86

[Jason Thorpe]

> On May 15, 2019, at 9:19 PM, Maxime Villard  wrote:
> 
> Le 16/05/2019 à 04:36, SAITOH Masanobu a écrit :
>> Module Name: src
>> Committed By:msaitoh
>> Date:Thu May 16 02:36:30 UTC 2019
>> Modified Files:
>>  src/sys/arch/x86/x86: procfs_machdep.c
>> Log Message:
>>  Use ci_feat_val[7] instead of directly getting cpuid 7 edx.
>> To generate a diff of this commit:
>> cvs rdiff -u -r1.28 -r1.29 src/sys/arch/x86/x86/procfs_machdep.c
>> Please note that diffs are not public domain; they are subject to the
>> copyright notices on the relevant files.
> 
> The microcode updates CPUID7, if you read ci_feat_val you have the
> initial value, not the updated value. So reading CPUID7 was the right
> thing to do.

Should the microcode update procedure fix up the cached copy?

-- thorpej

Re: CVS commit: src/sys/arch/x86/x86

[Maxime Villard]

Le 16/05/2019 à 04:36, SAITOH Masanobu a écrit :

Module Name:src
Committed By:   msaitoh
Date:   Thu May 16 02:36:30 UTC 2019

Modified Files:
src/sys/arch/x86/x86: procfs_machdep.c

Log Message:
  Use ci_feat_val[7] instead of directly getting cpuid 7 edx.


To generate a diff of this commit:
cvs rdiff -u -r1.28 -r1.29 src/sys/arch/x86/x86/procfs_machdep.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


The microcode updates CPUID7, if you read ci_feat_val you have the
initial value, not the updated value. So reading CPUID7 was the right
thing to do.

Re: CVS commit: src/sys/arch/x86/acpi

[Jason Thorpe]

> On Mar 3, 2019, at 10:31 AM, m...@netbsd.org wrote:
> 
> Maybe we can use the longer name to avoid the confusion? PG_WIRED
> 
> (PG_W as writable exists in other archs, and there's precedence for
> PG_WIRED too)

Agreed, PG_WIRED is a better name for this.

-- thorpej

Re: CVS commit: src/sys/arch/x86/acpi

[maya]

On Sun, Mar 03, 2019 at 05:33:33PM +, Maxime Villard wrote:
> Module Name:  src
> Committed By: maxv
> Date: Sun Mar  3 17:33:33 UTC 2019
> 
> Modified Files:
>   src/sys/arch/x86/acpi: acpi_machdep.c
> 
> Log Message:
> Fix bug, PG_W is 'wired', not 'writable'.

Maybe we can use the longer name to avoid the confusion? PG_WIRED

(PG_W as writable exists in other archs, and there's precedence for
PG_WIRED too)

Re: CVS commit: src/sys/arch/x86

[Maxime Villard]

Le 13/02/2019 à 10:08, Cherry G.Mathew a écrit :

(resent to source-changes-d@)

"Maxime Villard"  writes:

  - There is no recursive slot possible, so we can't use pmap_map_ptes().
Rather, we walk down the EPT trees via the direct map, and that's
actually a lot simpler (and probably faster too...).


Does this mean that nvmm hosts have to have __HAVE_DIRECT_MAP ?


Yes, and all of them do in practice by default (GENERIC), so it's not a
problem.

It becomes a problem on certain special configurations such as KASAN that
disable the direct map. In this case EPT is not compiled. So if you use
KASAN but don't use NVMM+Intel there is no problem.

In fact, maybe I should add direct map support in KASAN. Initially I didn't
do it because I wanted to force all kernel allocations into pmap_kenter_pa,
to have 100% KASAN coverage of the KVA. But if I was using two separate
flags, such as

__HAVE_DIRECT_MAP = whether the kernel has a direct map
__USE_DIRECT_MAP  = whether the allocators can use the direct map

In KASAN we could have __HAVE_DIRECT_MAP=1 and __USE_DIRECT_MAP=0, meaning
that EPT can use the direct map internally but nobody in UVM/etc can use
the direct map for allocations. This would maintain KASAN coverage and
would enable KASAN+EPT support.


  - The kernel is never mapped in an EPT pmap. An EPT pmap cannot be loaded
on the host. This has two sub-consequences: at creation time we must
zero out all of the top-level PTEs, and at destruction time we force
the page out of the pool cache and into the pool, to ensure that a next
allocation will invoke pmap_pdp_ctor() to create a native pmap and not
recycle some stale EPT entries.


Can you not use a separate poolcache ? This could isolate host/guest
related memory pressure as well ?


The poolcache I was talking about is the one that stores the top-level
page of the page tables (PML4). It is only a 4KB page, and there is only
one such page per guest. Therefore there is no need to separate more,
one page per guest is pretty insignificant.

However, it is true that we could separate the caches of the non-top-level
page tables (L3, L2 and L1). But I think the interest is not huge: under
pressure UVM will unmap guest pages, and when it happens, we also free
the empty L3/L2/L1 pages, so the pressure is "naturally" reduced in the
shared poolcaches.

Re: CVS commit: src/sys/arch/x86

[Cherry G . Mathew]

(resent to source-changes-d@)

"Maxime Villard"  writes:


[...]

>
> Contrary to AMD-SVM, Intel-VMX uses a different set of PTE bits from
> native, and this has three important consequences:
>
>  - We can't use the native PTE bits, so each time we want to modify the
>page tables, we need to know whether we're dealing with a native pmap
>or an EPT pmap. This is accomplished with callbacks, that handle
>everything PTE-related.
>

I like this approach - perhaps it's a way to separate out other similar
pmaps (OT).

>  - There is no recursive slot possible, so we can't use pmap_map_ptes().
>Rather, we walk down the EPT trees via the direct map, and that's
>actually a lot simpler (and probably faster too...).
>

Does this mean that nvmm hosts have to have __HAVE_DIRECT_MAP ?
If so, it might be worth having a separate kernel conf rather than
GENERIC (I don't know how this works now). I recently built an amd64
kernel without __HAVE_DIRECT_MAP and was quite impressed that it
actually booted. That's a nice to have feature.

>  - The kernel is never mapped in an EPT pmap. An EPT pmap cannot be loaded
>on the host. This has two sub-consequences: at creation time we must
>zero out all of the top-level PTEs, and at destruction time we force
>the page out of the pool cache and into the pool, to ensure that a next
>allocation will invoke pmap_pdp_ctor() to create a native pmap and not
>recycle some stale EPT entries.

Can you not use a separate poolcache ? This could isolate host/guest
related memory pressure as well ?

-- 
~cherry

Re: CVS commit: src/sys/arch/x86/x86

[Cherry G . Mathew]

Maxime Villard  writes:

> Can we do something about it now? It's been more than a week, and the issue is
> still there. NVMM still doesn't modload, same for procfs, and GENERIC_KASLR
> doesn't work either.
>
> This needs to be fixed now, and we should not start adding random hacks all
> over the place (like the one below).
>

Sorry for the delay - I've rolled back the changes.

http://mail-index.netbsd.org/source-changes/2019/01/06/msg102113.html

The XEN related ones I will roll back if enough people complain. I'm
meanwhile investigating other options.

Thanks for your patience.
-- 
~cherry

Re: CVS commit: src/sys/arch/x86/x86

[Maxime Villard]

Can we do something about it now? It's been more than a week, and the issue is
still there. NVMM still doesn't modload, same for procfs, and GENERIC_KASLR
doesn't work either.

This needs to be fixed now, and we should not start adding random hacks all
over the place (like the one below).


Le 05/01/2019 à 21:32, Christos Zoulas a écrit :

Module Name:src
Committed By:   christos
Date:   Sat Jan  5 20:32:02 UTC 2019

Modified Files:
src/sys/arch/x86/x86: procfs_machdep.c

Log Message:
Comment out rcr0 use until the weak symbol mess is undone.


To generate a diff of this commit:
cvs rdiff -u -r1.25 -r1.26 src/sys/arch/x86/x86/procfs_machdep.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Re: CVS commit: src/sys/arch/x86

[Maxime Villard]

Le 07/12/2018 à 17:29, Jaromír Doleček a écrit :

Maybe I missed something earlier - does KASLR being enabled by default
mean that x86 now doesn't any more use the direct map to copy memory
pages?


No. The direct map is still there and still used, the only thing is that
its location is randomized.

You are probably confusing with KASAN, which indeed doesn't have a
direct map, for specific reasons.

In all cases, GENERIC stays with a direct map.

Re: CVS commit: src/sys/arch/x86

[Jaromír Doleček]

Maybe I missed something earlier - does KASLR being enabled by default
mean that x86 now doesn't any more use the direct map to copy memory
pages?

Jaromir
Le ven. 7 déc. 2018 à 16:47, Maxime Villard  a écrit :
>
> Module Name:src
> Committed By:   maxv
> Date:   Fri Dec  7 15:47:11 UTC 2018
>
> Modified Files:
> src/sys/arch/x86/conf: files.x86
> src/sys/arch/x86/x86: pmap.c
>
> Log Message:
> Add an option to have a static kernel memory layout. This option is
> disabled by default - that is to say, KASLR remains enabled by default.
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.103 -r1.104 src/sys/arch/x86/conf/files.x86
> cvs rdiff -u -r1.312 -r1.313 src/sys/arch/x86/x86/pmap.c
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>

re: CVS commit: src/sys/arch/x86/x86

[matthew green]

Joerg Sonnenberger writes:
> On Tue, Dec 04, 2018 at 07:27:22PM +, Cherry G. Mathew wrote:
> > Module Name:src
> > Committed By:   cherry
> > Date:   Tue Dec  4 19:27:22 UTC 2018
> > 
> > Modified Files:
> > src/sys/arch/x86/x86: cpu.c intr.c
> > 
> > Log Message:
> > Hypothetically speaking, if one were to want to compile a
> > 
> > 'no options MULTIPROCESSOR'
> > 
> > kernel, these files may trip up the build.
> > 
> > Fix them by moving around the #defines as originally intended.
> 
> Well, MULTIPROCESSOR is *not* meant to be optional on x86.

true, but it's always been very simple to keep working.  i've
commited simple fixes like this maybe twice before..


.mrg.

Re: CVS commit: src/sys/arch/x86/x86

[Joerg Sonnenberger]

On Tue, Dec 04, 2018 at 07:27:22PM +, Cherry G. Mathew wrote:
> Module Name:  src
> Committed By: cherry
> Date: Tue Dec  4 19:27:22 UTC 2018
> 
> Modified Files:
>   src/sys/arch/x86/x86: cpu.c intr.c
> 
> Log Message:
> Hypothetically speaking, if one were to want to compile a
> 
> 'no options MULTIPROCESSOR'
> 
> kernel, these files may trip up the build.
> 
> Fix them by moving around the #defines as originally intended.

Well, MULTIPROCESSOR is *not* meant to be optional on x86.

Joerg

Re: CVS commit: src/sys/arch/x86/pci

[Joerg Sonnenberger]

On Mon, Sep 10, 2018 at 02:49:23AM +, Cherry G. Mathew wrote:
> Module Name:  src
> Committed By: cherry
> Date: Mon Sep 10 02:49:23 UTC 2018
> 
> Modified Files:
>   src/sys/arch/x86/pci: pci_intr_machdep.c
> 
> Log Message:
> In the NIOAPIC case, we do not need to support "legacy" irqs,
> ie; We don't need to simultaneously pass back the irq in the
> range 0 < irq < 16 (which are sometimes described as "legacy"
> in src

It should be kept in mind that the majority of the NIOAPIC code is
completely wrong by nature as it makes a runtime behavior choice based
on a build-time variable. :(

Joerg

re: CVS commit: src/sys/arch/x86

[matthew green]

"Maxime Villard" writes:
> Module Name:  src
> Committed By: maxv
> Date: Tue Aug  7 10:50:12 UTC 2018
> 
> Modified Files:
>   src/sys/arch/x86/include: specialreg.h
>   src/sys/arch/x86/x86: errata.c
> 
> Log Message:
> Add five errata for AMD Family 17h (Ryzen etc), tested by Patrick Welche,
> thanks. Also add two errata for Family 16h, not yet tested, so not yet
> enabled.

i enabled these on my family 16h system and it seems fine:

cpu0: "AMD E2-7110 APU with AMD Radeon R2 Graphics"
cpu0: AMD Family 16h (686-class), 1796.70 MHz
cpu0: family 0x16 model 0x30 stepping 0x1 (id 0x730f01)

so you can probably turn them on now.  thanks!


.mrg.

Re: CVS commit: src/sys/arch/x86/include

[Paul Goyette]

On Sun, 15 Jul 2018, Paul Goyette wrote:


Any chance that this will fix kern/52919?

If not, can we do some additional re-arrangement of struct cpu_info to 
address the problem?


And in any case, shouldn't this cause a bump in kernel version, since 
you've changed a structure that is shared between kernel and modules?


:)



Modified Files:
src/sys/arch/x86/include: cpu.h

Log Message:
Hum. Move the __HAVE_DIRECT_MAP block a little below, otherwise dynamically
loaded kernel modules use a wrong offset for some ci_* fields. Found when
modloading tprof_amd on an AMD 10h, the read of ci_signature was at a
wrong address, and the cpu family was not detected correctly.


+--+--++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses:  |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com   |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org |
+--+--++

Re: CVS commit: src/sys/arch/x86/include

[Paul Goyette]

Any chance that this will fix kern/52919?

If not, can we do some additional re-arrangement of struct cpu_info to 
address the problem?




On Sun, 15 Jul 2018, Maxime Villard wrote:


Module Name:src
Committed By:   maxv
Date:   Sun Jul 15 08:47:43 UTC 2018

Modified Files:
src/sys/arch/x86/include: cpu.h

Log Message:
Hum. Move the __HAVE_DIRECT_MAP block a little below, otherwise dynamically
loaded kernel modules use a wrong offset for some ci_* fields. Found when
modloading tprof_amd on an AMD 10h, the read of ci_signature was at a
wrong address, and the cpu family was not detected correctly.


To generate a diff of this commit:
cvs rdiff -u -r1.94 -r1.95 src/sys/arch/x86/include/cpu.h

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


!DSPAM:5b4b0a35163311665672011!




+--+--++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses:  |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com   |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org |
+--+--++

Re: CVS commit: src/sys/arch/x86/x86

[Kamil Rytarowski]

On 08.07.2018 15:56, Christos Zoulas wrote:
> In article <20180708092413.gb8...@mail.duskware.de>,
> Martin Husemann   wrote:
>> On Sun, Jul 08, 2018 at 10:49:53AM +0200, Jaromír Dole?ek wrote:
 Module Name:src
 Committed By:   kamil
 Date:   Sat Jul  7 23:05:50 UTC 2018

 Modified Files:
 src/sys/arch/x86/x86: mpbios.c

 Log Message:
 Remove unaligned access to mpbios_page[]

 Replace unaligned pointer dereference with a more portable construct that
 is free from Undefined Behavior semantics.

 sys/arch/x86/x86/mpbios.c:308:11, load of misaligned address 
 0x800031c7a413 for type 'const
>> __uint16_t' which requires 2 byte alignment
>>
>>
>> Can we please do NOT do such stupid changes?
>>
>> This is a bogus error message, please restore the original code!
> 
> These changes are pointless; how much code will we need to change
> to silence mis-aligned warnings? These changes are also dangerous
> when it comes to reading from devices (where multiple reads can
> behave differently). If you want to silence the warnings, use
> __attribute__, but even that is of questionable use. I'd venture
> to say, misaligned warnings on cpus where aligned access (to do
> the aligning) is costlier than direct misaligned accesses (like
> x86) are useless. In addition, it is not like we would ever turn
> on the force alignment bit on an x86 cpu and have things work!
> 
> So my preference would be to revert the change and take this up
> to tech-kern first (how misaligned accesses should be treated),
> because the situation is not that simple (when it comes to things
> like SSE operations etc.)
> http://pzemtsov.github.io/2016/11/06/bug-story-alignment-on-x86.html
> 
> 
> Best,
> 
> christos
> 

I've started a tech-kern discussion on the request.

Regarding the number of assignment issues to address:
 - acpica
 - mpbios.c [this one]
 - arch/x86/x86/patch.c [load of address with insufficient space]
 - sys/kern/subr_disk_mbr.c disklabel struct misaligned
 - IPv6, TCP
 - amd64/amd64/kobj_machdep.c (ELF)
 - XHCI

http://netbsd.org/~kamil/kubsan/0007-boot-real-hardware.txt

I'm going to revert it for a while, until there will be a conclusion.



signature.asc
Description: OpenPGP digital signature

Re: CVS commit: src/sys/arch/x86/x86

[Kamil Rytarowski]

On 08.07.2018 10:49, Jaromír Doleček wrote:
> Shouldn't this:
> 
> memtop |= (uint16_t)mpbios_page[0x414] << 8;
> 
> be actually << 16 to keep the same semantics?
> 

No. This is a 2-byte (x86 word) variable. One byte has to be stored with
the 8 bits shift.

If it would be differently it would probably brick the booting process.

> Jaromir
> Le dim. 8 juil. 2018 à 01:05, Kamil Rytarowski  a écrit :
>>
>> Module Name:src
>> Committed By:   kamil
>> Date:   Sat Jul  7 23:05:50 UTC 2018
>>
>> Modified Files:
>> src/sys/arch/x86/x86: mpbios.c
>>
>> Log Message:
>> Remove unaligned access to mpbios_page[]
>>
>> Replace unaligned pointer dereference with a more portable construct that
>> is free from Undefined Behavior semantics.
>>
>> sys/arch/x86/x86/mpbios.c:308:11, load of misaligned address 
>> 0x800031c7a413 for type 'const __uint16_t' which requires 2 byte 
>> alignment
>>
>> Detected with Kernel Undefined Behavior Sanitizer
>>
>>
>> To generate a diff of this commit:
>> cvs rdiff -u -r1.66 -r1.67 src/sys/arch/x86/x86/mpbios.c
>>
>> Please note that diffs are not public domain; they are subject to the
>> copyright notices on the relevant files.
>>




signature.asc
Description: OpenPGP digital signature

Re: CVS commit: src/sys/arch/x86/x86

[Christos Zoulas]

In article <20180708092413.gb8...@mail.duskware.de>,
Martin Husemann   wrote:
>On Sun, Jul 08, 2018 at 10:49:53AM +0200, Jaromír Dole?ek wrote:
>> > Module Name:src
>> > Committed By:   kamil
>> > Date:   Sat Jul  7 23:05:50 UTC 2018
>> >
>> > Modified Files:
>> > src/sys/arch/x86/x86: mpbios.c
>> >
>> > Log Message:
>> > Remove unaligned access to mpbios_page[]
>> >
>> > Replace unaligned pointer dereference with a more portable construct that
>> > is free from Undefined Behavior semantics.
>> >
>> > sys/arch/x86/x86/mpbios.c:308:11, load of misaligned address 
>> > 0x800031c7a413 for type 'const
>__uint16_t' which requires 2 byte alignment
>
>
>Can we please do NOT do such stupid changes?
>
>This is a bogus error message, please restore the original code!

These changes are pointless; how much code will we need to change
to silence mis-aligned warnings? These changes are also dangerous
when it comes to reading from devices (where multiple reads can
behave differently). If you want to silence the warnings, use
__attribute__, but even that is of questionable use. I'd venture
to say, misaligned warnings on cpus where aligned access (to do
the aligning) is costlier than direct misaligned accesses (like
x86) are useless. In addition, it is not like we would ever turn
on the force alignment bit on an x86 cpu and have things work!

So my preference would be to revert the change and take this up
to tech-kern first (how misaligned accesses should be treated),
because the situation is not that simple (when it comes to things
like SSE operations etc.)
http://pzemtsov.github.io/2016/11/06/bug-story-alignment-on-x86.html


Best,

christos

Re: CVS commit: src/sys/arch/x86/x86

[Martin Husemann]

On Sun, Jul 08, 2018 at 10:49:53AM +0200, Jaromír Dole?ek wrote:
> > Module Name:src
> > Committed By:   kamil
> > Date:   Sat Jul  7 23:05:50 UTC 2018
> >
> > Modified Files:
> > src/sys/arch/x86/x86: mpbios.c
> >
> > Log Message:
> > Remove unaligned access to mpbios_page[]
> >
> > Replace unaligned pointer dereference with a more portable construct that
> > is free from Undefined Behavior semantics.
> >
> > sys/arch/x86/x86/mpbios.c:308:11, load of misaligned address 
> > 0x800031c7a413 for type 'const __uint16_t' which requires 2 byte 
> > alignment


Can we please do NOT do such stupid changes?

This is a bogus error message, please restore the original code!

Martin

Re: CVS commit: src/sys/arch/x86/x86

[Jaromír Doleček]

Shouldn't this:

memtop |= (uint16_t)mpbios_page[0x414] << 8;

be actually << 16 to keep the same semantics?

Jaromir
Le dim. 8 juil. 2018 à 01:05, Kamil Rytarowski  a écrit :
>
> Module Name:src
> Committed By:   kamil
> Date:   Sat Jul  7 23:05:50 UTC 2018
>
> Modified Files:
> src/sys/arch/x86/x86: mpbios.c
>
> Log Message:
> Remove unaligned access to mpbios_page[]
>
> Replace unaligned pointer dereference with a more portable construct that
> is free from Undefined Behavior semantics.
>
> sys/arch/x86/x86/mpbios.c:308:11, load of misaligned address 
> 0x800031c7a413 for type 'const __uint16_t' which requires 2 byte alignment
>
> Detected with Kernel Undefined Behavior Sanitizer
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.66 -r1.67 src/sys/arch/x86/x86/mpbios.c
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>

Re: CVS commit: src/sys/arch/x86/x86

[Maxime Villard]

Le 22/06/2018 à 03:40, matthew green a écrit :

"Maxime Villard" writes:

Module Name:src
Committed By:   maxv
Date:   Tue Jun 19 09:25:13 UTC 2018

Modified Files:
src/sys/arch/x86/x86: fpu.c

Log Message:
When using EagerFPU, create the fpu state in execve at IPL_HIGH.


why splhigh instead of kpreempt_disable()?


Nick Hudson asked me the same question yesterday, here's the relevant part
of my answer (quoting his mail, I guess he doesn't mind).

The fpu code already runs at splhigh, and this disables preemption. So I
used splhigh too.




 Message transféré 
Sujet : Re: Fwd: CVS commit: src/sys/arch/x86/x86
Date : Thu, 21 Jun 2018 07:23:33 +0100
De : Nick Hudson 
Pour : Maxime Villard 

On 21/06/2018 07:21, Maxime Villard wrote:

Le 21/06/2018 à 08:11, Nick Hudson a écrit :

On 21/06/2018 06:42, Maxime Villard wrote:

Le 21/06/2018 à 07:34, Maxime Villard a écrit :

Le 21/06/2018 à 07:21, Nick Hudson a écrit :

Surely, kpreempt_{disable,enable}() is what you really mean?

Nick


No, I mean splhigh. splhigh does prevent preemption, is that 
incorrect?


If you want to disable preemption then kpreempt_disable() is what you 
want.


If you want to disable interrupts (and as a consequence pre-emption) 
then splhigh is what you want.


I firmly believe you want the former.

Why disable interupts unnecessarily? This is "bad practice" and 
shouldn't be left for others to cargo cult copy.


There is a KASSERT in fpusave_cpu(), which I didn't introduce. We want
IPL_HIGH, unconditionally.

So no, I don't want the former, the FPU code wants the latter. Then we 
can say that maybe splhigh is not needed after all in fpusave_cpu; I won't 
risk introducing random races.


In the commit I said "disable preemption" because I had in mind the 
problem where a context switch occurs as a result of preemption.


Up to you... really don't know why FPU code needs interrupts disabled.

re: CVS commit: src/sys/arch/x86/x86

[matthew green]

"Maxime Villard" writes:
> Module Name:  src
> Committed By: maxv
> Date: Tue Jun 19 09:25:13 UTC 2018
> 
> Modified Files:
>   src/sys/arch/x86/x86: fpu.c
> 
> Log Message:
> When using EagerFPU, create the fpu state in execve at IPL_HIGH.

why splhigh instead of kpreempt_disable()?


.mrg.

Re: CVS commit: src/sys/arch/x86/x86

[Jason Thorpe]

> On Jun 7, 2018, at 7:59 AM, m...@netbsd.org wrote:
> 
> You've changed a default and selectively fixed the one driver that
> people noticed breaks from it. How do you know the rest aren't broken?

As you know, there is very little certainty in this world.  For example, how 
can I be certain that I won’t be hit by a bus when crossing the street? (Pun 
intended.)  At best, I can take precautions to minimize my risk.

In this case, here are the precautions I have taken:

- I have used a method of probing for i2c devices that is widely considered to 
be the best approach for doing so in a general way.

- The method I have selected is equivalent to the SMBus “quick” command.

- The overwhelming majority of “intelligent” i2c controllers that provide an 
“exec” interface to our i2c subsystem are, in fact, specifically SMBus 
controllers, and thus will, by their nature, support this operation.

- The other general-purpose “intelligent” i2c controllers that provide an 
“exec” interface should also, at the hardware layer, should support this 
operation because it’s a completely legal i2c bus sequence that should be free 
of side-effects.

- Per a lengthy discussion on tech-kern, we know of a specific, odd case of 
“intelligent” i2c controller that is fairly neutered, which we have 
special-cased.  It is truly the odd-ball.

- For the non-intelligent i2c controllers that use the i2c subsystem’s bit-bang 
logic, if this method doesn’t work, it’s simply a software bug that should be 
fixed.

Of course, you’re also ignoring the fact that i2c configuration was already 
somewhat broken in -current on a bunch of platforms, due to the effects of 
fixing another bug.  So, at the very least, this is moving the needle forward 
after a consensus among various stakeholders that this was a good approach.

In all my years of being involved with NetBSD, I have always made a good faith 
effort to improve the system to benefit the community, and attempt to correct 
my mistakes promptly when I make them.  If there’s one thing I’m certain of, 
it’s that.




> 
> On Thu, Jun 07, 2018 at 01:35:31PM +, Jason R Thorpe wrote:
>> Module Name: src
>> Committed By:thorpej
>> Date:Thu Jun  7 13:35:31 UTC 2018
>> 
>> Modified Files:
>>  src/sys/arch/x86/x86: x86_autoconf.c
>> 
>> Log Message:
>> In device_register(), if the device is an "iic" child of "imcsmb",
>> attach a I2C_PROP_INDIRECT_DEVICE_WHITELIST property that limits
>> the allowed devices to "spdmem" and "sdtemp".  Also set the
>> I2C_PROP_INDIRECT_PROBE_STRATEGY property to I2C_PROBE_STRATEGY_NONE,
>> since that controller can't issue any of the "quick" commands.
>> 
>> XXX It would be nice to be able to do this in the imcsmb driver
>> itself, but the way autoconfiguration works makes that infeasible.
>> 
>> 
>> To generate a diff of this commit:
>> cvs rdiff -u -r1.76 -r1.77 src/sys/arch/x86/x86/x86_autoconf.c
>> 
>> Please note that diffs are not public domain; they are subject to the
>> copyright notices on the relevant files.
>> 
> 
>> Modified files:
>> 
>> Index: src/sys/arch/x86/x86/x86_autoconf.c
>> diff -u src/sys/arch/x86/x86/x86_autoconf.c:1.76 
>> src/sys/arch/x86/x86/x86_autoconf.c:1.77
>> --- src/sys/arch/x86/x86/x86_autoconf.c:1.76 Thu Nov  9 01:02:56 2017
>> +++ src/sys/arch/x86/x86/x86_autoconf.c  Thu Jun  7 13:35:31 2018
>> @@ -1,4 +1,4 @@
>> -/*  $NetBSD: x86_autoconf.c,v 1.76 2017/11/09 01:02:56 christos Exp $   
>> */
>> +/*  $NetBSD: x86_autoconf.c,v 1.77 2018/06/07 13:35:31 thorpej Exp $
>> */
>> 
>> /*-
>>  * Copyright (c) 1990 The Regents of the University of California.
>> @@ -35,7 +35,7 @@
>>  */
>> 
>> #include 
>> -__KERNEL_RCSID(0, "$NetBSD: x86_autoconf.c,v 1.76 2017/11/09 01:02:56 
>> christos Exp $");
>> +__KERNEL_RCSID(0, "$NetBSD: x86_autoconf.c,v 1.77 2018/06/07 13:35:31 
>> thorpej Exp $");
>> 
>> #include 
>> #include 
>> @@ -54,6 +54,8 @@ __KERNEL_RCSID(0, "$NetBSD: x86_autoconf
>> #include 
>> #include 
>> 
>> +#include 
>> +
>> #include "acpica.h"
>> #include "wsdisplay.h"
>> 
>> @@ -547,6 +549,36 @@ device_register(device_t dev, void *aux)
>> {
>>  device_t isaboot, pciboot;
>> 
>> +/*
>> + * The Intel Integrated Memory Controller has a built-in i2c
>> + * controller that's rather limited in capability; it is intended
>> + * only for reading memory module EERPOMs and sensors.
>> + */
>> +if (device_is_a(dev, "iic") &&
>> +device_is_a(dev->dv_parent, "imcsmb")) {
>> +static const char *imcsmb_device_whitelist[] = {
>> +"spdmem",
>> +"sdtemp",
>> +NULL,
>> +};
>> +prop_array_t whitelist = prop_array_create();
>> +prop_dictionary_t props = device_properties(dev);
>> +int i;
>> +
>> +for (i = 0; imcsmb_device_whitelist[i] != NULL; i++) {
>> +prop_string_t pstr = prop_string_create_cstring_nocopy(
>> +   

Re: CVS commit: src/sys/arch/x86/x86

[maya]

You've changed a default and selectively fixed the one driver that
people noticed breaks from it. How do you know the rest aren't broken?

On Thu, Jun 07, 2018 at 01:35:31PM +, Jason R Thorpe wrote:
> Module Name:  src
> Committed By: thorpej
> Date: Thu Jun  7 13:35:31 UTC 2018
> 
> Modified Files:
>   src/sys/arch/x86/x86: x86_autoconf.c
> 
> Log Message:
> In device_register(), if the device is an "iic" child of "imcsmb",
> attach a I2C_PROP_INDIRECT_DEVICE_WHITELIST property that limits
> the allowed devices to "spdmem" and "sdtemp".  Also set the
> I2C_PROP_INDIRECT_PROBE_STRATEGY property to I2C_PROBE_STRATEGY_NONE,
> since that controller can't issue any of the "quick" commands.
> 
> XXX It would be nice to be able to do this in the imcsmb driver
> itself, but the way autoconfiguration works makes that infeasible.
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.76 -r1.77 src/sys/arch/x86/x86/x86_autoconf.c
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
> 

> Modified files:
> 
> Index: src/sys/arch/x86/x86/x86_autoconf.c
> diff -u src/sys/arch/x86/x86/x86_autoconf.c:1.76 
> src/sys/arch/x86/x86/x86_autoconf.c:1.77
> --- src/sys/arch/x86/x86/x86_autoconf.c:1.76  Thu Nov  9 01:02:56 2017
> +++ src/sys/arch/x86/x86/x86_autoconf.c   Thu Jun  7 13:35:31 2018
> @@ -1,4 +1,4 @@
> -/*   $NetBSD: x86_autoconf.c,v 1.76 2017/11/09 01:02:56 christos Exp $   
> */
> +/*   $NetBSD: x86_autoconf.c,v 1.77 2018/06/07 13:35:31 thorpej Exp $
> */
>  
>  /*-
>   * Copyright (c) 1990 The Regents of the University of California.
> @@ -35,7 +35,7 @@
>   */
>  
>  #include 
> -__KERNEL_RCSID(0, "$NetBSD: x86_autoconf.c,v 1.76 2017/11/09 01:02:56 
> christos Exp $");
> +__KERNEL_RCSID(0, "$NetBSD: x86_autoconf.c,v 1.77 2018/06/07 13:35:31 
> thorpej Exp $");
>  
>  #include 
>  #include 
> @@ -54,6 +54,8 @@ __KERNEL_RCSID(0, "$NetBSD: x86_autoconf
>  #include 
>  #include 
>  
> +#include 
> +
>  #include "acpica.h"
>  #include "wsdisplay.h"
>  
> @@ -547,6 +549,36 @@ device_register(device_t dev, void *aux)
>  {
>   device_t isaboot, pciboot;
>  
> + /*
> +  * The Intel Integrated Memory Controller has a built-in i2c
> +  * controller that's rather limited in capability; it is intended
> +  * only for reading memory module EERPOMs and sensors.
> +  */
> + if (device_is_a(dev, "iic") &&
> + device_is_a(dev->dv_parent, "imcsmb")) {
> + static const char *imcsmb_device_whitelist[] = {
> + "spdmem",
> + "sdtemp",
> + NULL,
> + };
> + prop_array_t whitelist = prop_array_create();
> + prop_dictionary_t props = device_properties(dev);
> + int i;
> +
> + for (i = 0; imcsmb_device_whitelist[i] != NULL; i++) {
> + prop_string_t pstr = prop_string_create_cstring_nocopy(
> + imcsmb_device_whitelist[i]);
> + (void) prop_array_add(whitelist, pstr);
> + prop_object_release(pstr);
> + }
> + (void) prop_dictionary_set(props,
> +I2C_PROP_INDIRECT_DEVICE_WHITELIST,
> +whitelist);
> + (void) prop_dictionary_set_cstring_nocopy(props,
> +I2C_PROP_INDIRECT_PROBE_STRATEGY,
> +I2C_PROBE_STRATEGY_NONE);
> + }
> +
>   device_acpi_register(dev, aux);
>  
>   isaboot = device_isa_register(dev, aux);
> 

Re: CVS commit: src/sys/arch/x86/x86

[Maxime Villard]

Le 07/02/2018 à 23:49, Maya Rashish a écrit :

Module Name:src
Committed By:   maya
Date:   Wed Feb  7 22:49:32 UTC 2018

Modified Files:
src/sys/arch/x86/x86: identcpu.c

Log Message:
stopgap fix: restrict XSAVEOPT to Intel CPUs

The current code causes floating point miscalculations on AMD Ryzen.
PR port-amd64/52966: amd64 FPU handling broken on AMD


To generate a diff of this commit:
cvs rdiff -u -r1.67 -r1.68 src/sys/arch/x86/x86/identcpu.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


Thanks, mlelstv told me about this a week ago, and I just hadn't gotten
around to fixing it yet.

It would be better to disable XSAVEOPT entirely (including on Intel), until
I add a bunch of KASSERTs to ensure the spec is respected.

Maxime

Re: CVS commit: src/sys/arch/x86/pci

[Paul Goyette]

On Thu, 25 Jan 2018, Patrick Welche wrote:


Module Name:src
Committed By:   prlw1
Date:   Thu Jan 25 15:01:05 UTC 2018

Modified Files:
src/sys/arch/x86/pci: amdzentemp.c

Log Message:
Unused variable build fix. (now void *aux is unused)


To generate a diff of this commit:
cvs rdiff -u -r1.1 -r1.2 src/sys/arch/x86/pci/amdzentemp.c


@@ -98,8 +98,6 @@ CFATTACH_DECL_NEW(amdzentemp, sizeof(str
 static int
 amdzentemp_match(device_t parent, cfdata_t match, void *aux)
 {
-   struct pci_attach_args *pa = aux;
-
KASSERT(PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD);

cfdata_t parent_cfdata = device_cfdata(parent);


Please leave the blank line for KNF reasons!



+--+--++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses:  |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com   |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org |
+--+--++

Re: CVS commit: src/sys/arch/x86/x86

[Masanobu SAITOH]

On 2018/01/11 18:18, SAITOH Masanobu wrote:

Module Name:src
Committed By:   msaitoh
Date:   Thu Jan 11 09:18:16 UTC 2018

Modified Files:
src/sys/arch/x86/x86: cpu.c

Log Message:
  Changing CR4 register may change cpuid values. For example, setting
CR4_OSXSAVE sets CPUID2_OSXSAVE. The CPUID2_OSXSAVE is in ci_feat_val[1],
so update it after changing CR4.


To generate a diff of this commit:
cvs rdiff -u -r1.144 -r1.145 src/sys/arch/x86/x86/cpu.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.



XXX:
 - Other than ci_feat_val[1] might be changed.
 - Updating microcode may also changes cpuid values.

--
---
SAITOH Masanobu (msai...@execsw.org
 msai...@netbsd.org)

Re: CVS commit: src/sys/arch/x86/x86

[Kamil Rytarowski]

This is really impressive and great work!

On 21.10.2017 21:05, Alistair Crooks wrote:
> Oh, nice! This is great :)
> 
> Thank you for all your work on this, pkboot, kaslr and the pmap.
> 
> Best,
> Alistair
> 
> On 21 October 2017 at 01:27, Maxime Villard  wrote:
>> Module Name:src
>> Committed By:   maxv
>> Date:   Sat Oct 21 08:27:19 UTC 2017
>>
>> Modified Files:
>> src/sys/arch/x86/x86: sys_machdep.c
>>
>> Log Message:
>> Forbid 64bit entries. That's it, now we support USER_LDT on amd64.
>>
>>
>> To generate a diff of this commit:
>> cvs rdiff -u -r1.42 -r1.43 src/sys/arch/x86/x86/sys_machdep.c
>>
>> Please note that diffs are not public domain; they are subject to the
>> copyright notices on the relevant files.
>>




signature.asc
Description: OpenPGP digital signature

Re: CVS commit: src/sys/arch/x86/x86

[Alistair Crooks]

Oh, nice! This is great :)

Thank you for all your work on this, pkboot, kaslr and the pmap.

Best,
Alistair

On 21 October 2017 at 01:27, Maxime Villard  wrote:
> Module Name:src
> Committed By:   maxv
> Date:   Sat Oct 21 08:27:19 UTC 2017
>
> Modified Files:
> src/sys/arch/x86/x86: sys_machdep.c
>
> Log Message:
> Forbid 64bit entries. That's it, now we support USER_LDT on amd64.
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.42 -r1.43 src/sys/arch/x86/x86/sys_machdep.c
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>

Re: CVS commit: src/sys/arch/x86/x86

[Christos Zoulas]

In article <20171017054709.b160cf...@cvs.netbsd.org>,
Maya Rashish  wrote:
>-=-=-=-=-=-
>
>Module Name:   src
>Committed By:  maya
>Date:  Tue Oct 17 05:47:09 UTC 2017
>
>Modified Files:
>   src/sys/arch/x86/x86: vmt.c
>
>Log Message:
>Check that the host supports GET_SPEED as well as GET_VERSION
>before deciding vmt_probe has succeeded.
>
>qemu supports GET_VERSION but not the RPC protocol so the probe succeeds
>but the attach fails, resulting in "vmt0: failed to open backdoor RPC
>channel (TCLO protocol)".  All known versions of vmware support GET_SPEED
>and no known qemu versions do, so this prevents it from attempting to
>attach (and failing) on qemu while still working on vmware.
>
>stop checking vmt_type to avoid having to adapt this code.
>
>- Taken from openbsd

But you can still get and print the type from ecx? Or that does not work
anymore?

christos

Re: CVS commit: src/sys/arch/x86/x86

[Joerg Sonnenberger]

On Thu, Oct 05, 2017 at 02:52:55PM +0200, Kamil Rytarowski wrote:
> An idea borrowed from the OpenBSD approach with wxneeded partition
> (mount) property.

I think that one pretty much completely misses the point.

Joerg

Re: CVS commit: src/sys/arch/x86/x86

[Kamil Rytarowski]

On 04.10.2017 08:35, Alexander Nasonov wrote:
> Maxime Villard wrote:
>> In the first mail, you said that it was better to have a all-or-nothing
>> sysctl, which is *exactly* what I just committed.
> 
> Yes, sysctl is better than giving rdtsc to root only. But "better"
> alone isn't strong enough to count me as a supporter.
> 
>> In the second one, as a reply to me, you were indeed talking about
>> more granular control -- but with vdso, which we don't have, so
>> it's basically not doable.
> 
> IMO, it's more important to have vdso than to control rdtsc.
> 
>> (PS: there is no point in having it done in a note section either, since
>> unpriv user can still create a binary with rdtsc enabled and side channel
>> the kernel.)
> 
> Mount all user-writable partitions with noexec.
> 

An idea borrowed from the OpenBSD approach with wxneeded partition
(mount) property.

Add fine-grained control over aslr, mprotect, segvguard, rdtsc, compat_*
etc as a mount option. With this approach we can grant certain features
to individual users or individual groups of people.

By default everything could be enforced. I would put my Opera binaries
in /home on my desktop.

I would benefit from it, being able to test-build language runtimes on a
dedicated mount point without shutting off global aslr/mprotect/similar
and without debugging why thing break the build and what needs to be
touched with paxctl(8).



signature.asc
Description: OpenPGP digital signature

Re: CVS commit: src/sys/arch/x86/x86

[Alexander Nasonov]

Maxime Villard wrote:
> In the first mail, you said that it was better to have a all-or-nothing
> sysctl, which is *exactly* what I just committed.

Yes, sysctl is better than giving rdtsc to root only. But "better"
alone isn't strong enough to count me as a supporter.

> In the second one, as a reply to me, you were indeed talking about
> more granular control -- but with vdso, which we don't have, so
> it's basically not doable.

IMO, it's more important to have vdso than to control rdtsc.

> (PS: there is no point in having it done in a note section either, since
> unpriv user can still create a binary with rdtsc enabled and side channel
> the kernel.)

Mount all user-writable partitions with noexec.

-- 
Alex

Re: CVS commit: src/sys/arch/x86/x86

[Maxime Villard]

Le 03/10/2017 à 22:47, Alexander Nasonov a écrit :

Maxime Villard wrote:

In case you didn't notice, this sysctl results directly from the answers I
got, and is not my original plan (about which I changed my mind as a
consequence of the conversation). So now tell me exactly which point I didn't
consider and which reply I ignored. The thread is here?[1], go ahead, I'm
watching you.

[1] https://marc.info/?l=netbsd-tech-kern=149071535930695=2


Some people (including me) wanted more granular control. Your sysctl
doesn't give such control.


You replied two times in this thread.

https://marc.info/?l=netbsd-tech-kern=149074135206958=2
https://marc.info/?l=netbsd-tech-kern=149133012012784=2

In the first mail, you said that it was better to have a all-or-nothing
sysctl, which is *exactly* what I just committed. In the second one, as a
reply to me, you were indeed talking about more granular control -- but with
vdso, which we don't have, so it's basically not doable.

Maxime

(PS: there is no point in having it done in a note section either, since
unpriv user can still create a binary with rdtsc enabled and side channel
the kernel.)

Re: CVS commit: src/sys/arch/x86/x86

[Alexander Nasonov]

Maxime Villard wrote:
> In case you didn't notice, this sysctl results directly from the answers I
> got, and is not my original plan (about which I changed my mind as a
> consequence of the conversation). So now tell me exactly which point I didn't
> consider and which reply I ignored. The thread is here?[1], go ahead, I'm
> watching you.
> 
> [1] https://marc.info/?l=netbsd-tech-kern=149071535930695=2

Some people (including me) wanted more granular control. Your sysctl
doesn't give such control.

-- 
Alex

Re: CVS commit: src/sys/arch/x86/x86

[maya]

If pax mprotect is an example then maxv should just go around rm -rf'ing
any parts of the tree he doesn't like without even checking that the
kernel builds afterwards, since that's the way we do things around here.

It was months until I could run meld again, even disabling it for just
python was frowned upon. I don't see a bikeshed to discuss turning it on
on tech-kern, it was just done with zero preparation for anything in
pkgsrc and with no discussion whatsoever.

Re: CVS commit: src/sys/arch/x86/x86

[Maxime Villard]

Le 03/10/2017 à 21:14, Joerg Sonnenberger a écrit :

I'm not responding to this nonsensical thread anymore, everything got told
months ago. The option is here, people don't need to give a damn about it
unless they explicitly want to - which is still legitimate in some cases,
including for kaslr, whether it pleases you or not. There are plenty of
useless sysctls to complain about if you like.


Funny. You've been ignoring the replies you got month ago, so of course
there is nothing new to discuss. Frankly, I don't find that acceptable
behavior.


The stupidity of emails like that is the real funny thing here. I don't see
any point in such conversations, but since throwing shit at the fan is the
only thing you find useful (and so do I apparently), let's continue.

In case you didn't notice, this sysctl results directly from the answers I
got, and is not my original plan (about which I changed my mind as a
consequence of the conversation). So now tell me exactly which point I didn't
consider and which reply I ignored. The thread is here [1], go ahead, I'm
watching you.

[1] https://marc.info/?l=netbsd-tech-kern=149071535930695=2

Re: CVS commit: src/sys/arch/x86/x86

[Joerg Sonnenberger]

On Tue, Oct 03, 2017 at 06:54:52PM +0200, Maxime Villard wrote:
> Just like disabling va0 or enabling PaX mprotect; if you feel like these are
> no issues, then what's the fuss. You would be well served to read the "rdtsc
> is still enabled by default" part of my email.

Disabling va0 is low impact. Enabling PaX mprotect by default is far
from it. It doesn't help that again the set of people enforcing this
policies and the set of people that work on actually fixing the fallout
is wildly disjunct.

> I'm not responding to this nonsensical thread anymore, everything got told
> months ago. The option is here, people don't need to give a damn about it
> unless they explicitly want to - which is still legitimate in some cases,
> including for kaslr, whether it pleases you or not. There are plenty of
> useless sysctls to complain about if you like.

Funny. You've been ignoring the replies you got month ago, so of course
there is nothing new to discuss. Frankly, I don't find that acceptable
behavior.

Joerg

Re: CVS commit: src/sys/arch/x86/x86

[Warner Losh]

On Mon, Oct 2, 2017 at 4:09 PM, Taylor R Campbell <
campbell+netbsd-source-change...@mumble.net> wrote:

> > Date: Mon, 2 Oct 2017 21:42:11 +0200
> > From: Joerg Sonnenberger 
> >
> > On Mon, Oct 02, 2017 at 07:23:16PM +, Maxime Villard wrote:
> > > Add a machdep.tsc_user_enable sysctl, to enable/disable the rdtsc
> > > instruction in usermode. It defaults to enabled.
> >
> > Do we really need this change? I've said it before, I consider this a
> > really stupid idea and effectively useless complexity. rdtsc is not
> > necessary for precision measurement as long as an attacker is willing to
> > waste CPU time, i.e. having one core spinning incrementing a counter and
> > reading that one of a second core will give fairly accurate measurements
> > as long as both cores are near each other. It's normally not that
> > difficult to ensure that.
>
> Concur.  The way to thwart timing side channel attacks is not to
> pretend attackers don't have stop-watches; it's to avoid the variable
> timing that creates the side channels in the first place.
>

Even if you don't have the ability to change the defective hardware?

Why should I provide an attacker a stop watch? I want him/her to build
their own that has the potential to be accurate enough, but is necessarily
less accurate than the one I'm denying them access to.

Warner

Re: CVS commit: src/sys/arch/x86/x86

[Warner Losh]

On Mon, Oct 2, 2017 at 4:43 PM, Joerg Sonnenberger  wrote:

> On Mon, Oct 02, 2017 at 04:25:34PM -0600, Warner Losh wrote:
> > Even if you don't have the ability to change the defective hardware?
>
> The hardware is not defective. We are not talking about variable timing
> for basic arithmetic operations based on the operand value. Outside
> maybe division, that could be considered a hardware bug. A believe
> that timing of complex operations like memory access should be constant
> is IMO completely misguided for general purpose computing. Outside a
> very narrow range of hardware, it is absurd to even consider it.


On other processors, the issues are needing to do timing attacks due to
flaws in the hardware. For x86, no such bugs are known. History suggests it
is unwise to take the absence of evidence to be evidence of absence

> Why should I provide an attacker a stop watch? I want him/her to build
> > their own that has the potential to be accurate enough, but is
> necessarily
> > less accurate than the one I'm denying them access to.
>
> It has been said before: this is breaking completely valid use cases
> without actually fixing anything. It is security by obscurity at its
> best.
>

I'm not advocating turning it off by default. I'm saying that it would be
useful to allow removal of access to the counter as a mitigation technique,
perhaps while waiting for a more general / complete fix for a kernel bug to
be available.

Warner

Re: CVS commit: src/sys/arch/x86/x86

[David Holland]

On Tue, Oct 03, 2017 at 06:54:52PM +0200, Maxime Villard wrote:
 > I'm not responding to this nonsensical thread anymore,

The problem is, you keep acting unilaterally without having gathered a
consensus, then ignoring the resulting objections and demanding to
have everything your way and only your way.

Your ideas about how to proceed (about this or any of the other recent
issues) are not the only possible correct ways forward.

(Obviously, mine are.)

-- 
David A. Holland
dholl...@netbsd.org

Re: CVS commit: src/sys/arch/x86/x86

[Maxime Villard]

Le 03/10/2017 à 13:26, Joerg Sonnenberger a écrit :

At the same time, it has interesting interactions with power management
and the instruction queue.


The queue is flushed by a serializing instruction executed right before, which
is the recommended use case; the interaction with power management - and
by this I suppose you mean that some bioses change the cpu frequency depending
on the battery, temp, etc - also affects the software clock an attacker might
be running. In either case, rdtsc remains more accurate.


An additional thing we could do, if
we had proper NUMA support, would be to add a mode where the kernel schedules
unprivileged applications on a cluster, and the rest of the kernel and suid
LWPs on the other clusters. But that's far, far from us.


It would also kill performance.


no, but that's a different debate, so I'm leaving it there


Hiding the TSC doesn't solve any problem for this configuration.


solves problems, no; makes it harder to exploit problems, yes


All this sysctl provides is a very false sense of security and a way to
randomly break applications


Just like disabling va0 or enabling PaX mprotect; if you feel like these are
no issues, then what's the fuss. You would be well served to read the "rdtsc
is still enabled by default" part of my email.

I'm not responding to this nonsensical thread anymore, everything got told
months ago. The option is here, people don't need to give a damn about it
unless they explicitly want to - which is still legitimate in some cases,
including for kaslr, whether it pleases you or not. There are plenty of
useless sysctls to complain about if you like.

Re: CVS commit: src/sys/arch/x86/x86

[Joerg Sonnenberger]

On Tue, Oct 03, 2017 at 08:17:27AM +0200, Maxime Villard wrote:
> What is clear, however, is that the effort needed to perform accurate
> measurements in software is much higher than that needed to invoke a hardware
> instruction that will instantly give about the most accurate answer possible.

Have you tried getting reliable answers out of rdtsc lately? It is
suprisingly difficult. TSC is nice for two properties:
- it is cheap as in it consumes few cycles by itself
- it has a high resolution

At the same time, it has interesting interactions with power management
and the instruction queue. As such, "the most accurate answer" is not
true either.

> Disabling rdtsc is almost the only thing we can do; we can't update the
> hardware to kill the variable timing.

Variable timing won't go away. Any "security" technique that can't deal
with it is doomed to failure.

> An additional thing we could do, if
> we had proper NUMA support, would be to add a mode where the kernel schedules
> unprivileged applications on a cluster, and the rest of the kernel and suid
> LWPs on the other clusters. But that's far, far from us.

It would also kill performance. Again, cycle timing rarely matters and
hiding the TSC won't fix any of the cases where it does. Those are
algorithmic issues. Hiding the TSC can make it harder to apply noise in
those unfixable cases though.

> So the sysctl is here, and is enabled. People don't need to care about it by
> default. The use case I see is for ssh servers running kaslr kernels.

Hiding the TSC doesn't solve any problem for this configuration. All
this sysctl provides is a very false sense of security and a way to
randomly break applications. That means it creates a support cost for
little to no gain.

Joerg

Re: CVS commit: src/sys/arch/x86/x86

[Joerg Sonnenberger]

On Mon, Oct 02, 2017 at 04:25:34PM -0600, Warner Losh wrote:
> Even if you don't have the ability to change the defective hardware?

The hardware is not defective. We are not talking about variable timing
for basic arithmetic operations based on the operand value. Outside
maybe division, that could be considered a hardware bug. A believe
that timing of complex operations like memory access should be constant
is IMO completely misguided for general purpose computing. Outside a
very narrow range of hardware, it is absurd to even consider it.

> Why should I provide an attacker a stop watch? I want him/her to build
> their own that has the potential to be accurate enough, but is necessarily
> less accurate than the one I'm denying them access to.

It has been said before: this is breaking completely valid use cases
without actually fixing anything. It is security by obscurity at its
best.

Joerg

Re: CVS commit: src/sys/arch/x86/x86

[Taylor R Campbell]

> Date: Mon, 2 Oct 2017 16:25:34 -0600
> From: Warner Losh 
> 
> Why should I provide an attacker a stop watch? I want him/her to build
> their own that has the potential to be accurate enough, but is necessarily
> less accurate than the one I'm denying them access to.

There are plenty of legitimate applications of stop-watches.  It would
be great to forbid the attacker from doing arithmetic on your CPU too,
but if you forbid all your applications from using it, well, the chunk
of magic sand in your computer becomes about as useful as a chunk of
inert sand.

Re: CVS commit: src/sys/arch/x86/x86

[Taylor R Campbell]

> Date: Mon, 2 Oct 2017 21:42:11 +0200
> From: Joerg Sonnenberger 
> 
> On Mon, Oct 02, 2017 at 07:23:16PM +, Maxime Villard wrote:
> > Add a machdep.tsc_user_enable sysctl, to enable/disable the rdtsc
> > instruction in usermode. It defaults to enabled.
> 
> Do we really need this change? I've said it before, I consider this a
> really stupid idea and effectively useless complexity. rdtsc is not
> necessary for precision measurement as long as an attacker is willing to 
> waste CPU time, i.e. having one core spinning incrementing a counter and
> reading that one of a second core will give fairly accurate measurements
> as long as both cores are near each other. It's normally not that
> difficult to ensure that.

Concur.  The way to thwart timing side channel attacks is not to
pretend attackers don't have stop-watches; it's to avoid the variable
timing that creates the side channels in the first place.

Re: CVS commit: src/sys/arch/x86/x86

[Joerg Sonnenberger]

On Mon, Oct 02, 2017 at 07:23:16PM +, Maxime Villard wrote:
> Module Name:  src
> Committed By: maxv
> Date: Mon Oct  2 19:23:16 UTC 2017
> 
> Modified Files:
>   src/sys/arch/x86/x86: tsc.c tsc.h x86_machdep.c
> 
> Log Message:
> Add a machdep.tsc_user_enable sysctl, to enable/disable the rdtsc
> instruction in usermode. It defaults to enabled.

Do we really need this change? I've said it before, I consider this a
really stupid idea and effectively useless complexity. rdtsc is not
necessary for precision measurement as long as an attacker is willing to 
waste CPU time, i.e. having one core spinning incrementing a counter and
reading that one of a second core will give fairly accurate measurements
as long as both cores are near each other. It's normally not that
difficult to ensure that.

Joerg

Re: CVS commit: src/sys/arch/x86/include

[Maxime Villard]

Le 29/09/2017 à 05:17, Ryota Ozaki a écrit :

Module Name:src
Committed By:   ozaki-r
Date:   Fri Sep 29 03:17:18 UTC 2017

Modified Files:
src/sys/arch/x86/include: pmap.h

Log Message:
Fix build

sys/arch/x86/x86/cpu.c:920:20: error: 'pmap_largepages' undeclared (first use 
in this function)
   smp_data.large = (pmap_largepages != 0);
 ^


To generate a diff of this commit:
cvs rdiff -u -r1.67 -r1.68 src/sys/arch/x86/include/pmap.h

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


mmh yes, I patched my test machine but apparently didn't commit the updated
diff

Re: CVS commit: src/sys/arch/x86/x86

[Kengo NAKAHARA]

Hi maxv@n.o,

On 2017/08/08 2:12, Maxime Villard wrote:
> Le 07/08/2017 à 10:38, Kengo NAKAHARA a écrit :
>> "intrctl affinity" in NetBSD-current causes panic. Here is the backtrace
>> 
>> panic: kernel diagnostic assertion "mutex_owned(_lock) || !mp_online" 
>> failed: file 
>> "/disk4/home/k-nakahara/repos/netbsd-src/sys/arch/x86/x86/idt.c", line 122
>> fatal breakpoint trap in supervisor mode
>> trap type 1 code 0 rip 0x802249e5 cs 0x8 rflags 0x246 cr2 
>> 0x72ca08412d98 ilevel 0 rsp 0xe4010e898dc0
>> curlwp 0xe4010e88a0c0 pid 0.22 lowest kstack 0xe4010e8952c0
>> Stopped in pid 0.22 (system) at netbsd:breakpoint+0x5:  leave
>> db{1}> bt
>> breakpoint() at netbsd:breakpoint+0x5
>> vpanic() at netbsd:vpanic+0x140
>> ch_voltag_convert_in() at netbsd:ch_voltag_convert_in
>> idt_vec_set() at netbsd:idt_vec_set+0x82
>> intr_activate_xcall() at netbsd:intr_activate_xcall+0x8a
>> xc_thread() at netbsd:xc_thread+0xd4
>> 
>>
>> ozaki-r@n.o bisects and finds the following commit causes this panic.
>>
>> On 2017/08/01 3:54, Maxime Villard wrote:
>>> Module Name:src
>>> Committed By:   maxv
>>> Date:   Mon Jul 31 18:54:40 UTC 2017
>>>
>>> Modified Files:
>>> src/sys/arch/x86/x86: intr.c
>>>
>>> Log Message:
>>> Use idt_vec_set instead.
>>>
>>>
>>> To generate a diff of this commit:
>>> cvs rdiff -u -r1.101 -r1.102 src/sys/arch/x86/x86/intr.c
>>>
>>> Please note that diffs are not public domain; they are subject to the
>>> copyright notices on the relevant files.
>>
>> I think setgate() must be called with kpreempt_disable() and
>> x86_disable_intr(). However, I don't know why setgate() must be called
>> with mutex_onwed(_lock). Could you tell me why mutex_onwed(_lock)
>> is required?
> 
> setgate already disables preemption on amd64, but not on i386. I guess
> this needs to be investigated.

I see. Hmm, I also research about it when I can.


> Otherwise it's fixed, thanks

I test latest kernel(including idt.c:r1.5), and I confirm the bug is
fixed. Thank you.


Thanks,

-- 
//
Internet Initiative Japan Inc.

Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit

Kengo NAKAHARA 

Re: CVS commit: src/sys/arch/x86/x86

[Maxime Villard]

Le 07/08/2017 à 10:38, Kengo NAKAHARA a écrit :

Hi maxv@n.o,

"intrctl affinity" in NetBSD-current causes panic. Here is the backtrace

panic: kernel diagnostic assertion "mutex_owned(_lock) || !mp_online" failed: file 
"/disk4/home/k-nakahara/repos/netbsd-src/sys/arch/x86/x86/idt.c", line 122
fatal breakpoint trap in supervisor mode
trap type 1 code 0 rip 0x802249e5 cs 0x8 rflags 0x246 cr2 
0x72ca08412d98 ilevel 0 rsp 0xe4010e898dc0
curlwp 0xe4010e88a0c0 pid 0.22 lowest kstack 0xe4010e8952c0
Stopped in pid 0.22 (system) at netbsd:breakpoint+0x5:  leave
db{1}> bt
breakpoint() at netbsd:breakpoint+0x5
vpanic() at netbsd:vpanic+0x140
ch_voltag_convert_in() at netbsd:ch_voltag_convert_in
idt_vec_set() at netbsd:idt_vec_set+0x82
intr_activate_xcall() at netbsd:intr_activate_xcall+0x8a
xc_thread() at netbsd:xc_thread+0xd4


ozaki-r@n.o bisects and finds the following commit causes this panic.

On 2017/08/01 3:54, Maxime Villard wrote:

Module Name:src
Committed By:   maxv
Date:   Mon Jul 31 18:54:40 UTC 2017

Modified Files:
src/sys/arch/x86/x86: intr.c

Log Message:
Use idt_vec_set instead.


To generate a diff of this commit:
cvs rdiff -u -r1.101 -r1.102 src/sys/arch/x86/x86/intr.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


I think setgate() must be called with kpreempt_disable() and
x86_disable_intr(). However, I don't know why setgate() must be called
with mutex_onwed(_lock). Could you tell me why mutex_onwed(_lock)
is required?


setgate already disables preemption on amd64, but not on i386. I guess
this needs to be investigated.

Otherwise it's fixed, thanks

Maxime

Re: CVS commit: src/sys/arch/x86/x86

[Kengo NAKAHARA]

Hi maxv@n.o,

"intrctl affinity" in NetBSD-current causes panic. Here is the backtrace

panic: kernel diagnostic assertion "mutex_owned(_lock) || !mp_online" 
failed: file "/disk4/home/k-nakahara/repos/netbsd-src/sys/arch/x86/x86/idt.c", 
line 122
fatal breakpoint trap in supervisor mode
trap type 1 code 0 rip 0x802249e5 cs 0x8 rflags 0x246 cr2 
0x72ca08412d98 ilevel 0 rsp 0xe4010e898dc0
curlwp 0xe4010e88a0c0 pid 0.22 lowest kstack 0xe4010e8952c0
Stopped in pid 0.22 (system) at netbsd:breakpoint+0x5:  leave
db{1}> bt
breakpoint() at netbsd:breakpoint+0x5
vpanic() at netbsd:vpanic+0x140
ch_voltag_convert_in() at netbsd:ch_voltag_convert_in
idt_vec_set() at netbsd:idt_vec_set+0x82
intr_activate_xcall() at netbsd:intr_activate_xcall+0x8a
xc_thread() at netbsd:xc_thread+0xd4


ozaki-r@n.o bisects and finds the following commit causes this panic.

On 2017/08/01 3:54, Maxime Villard wrote:
> Module Name:  src
> Committed By: maxv
> Date: Mon Jul 31 18:54:40 UTC 2017
> 
> Modified Files:
>   src/sys/arch/x86/x86: intr.c
> 
> Log Message:
> Use idt_vec_set instead.
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.101 -r1.102 src/sys/arch/x86/x86/intr.c
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.

I think setgate() must be called with kpreempt_disable() and
x86_disable_intr(). However, I don't know why setgate() must be called
with mutex_onwed(_lock). Could you tell me why mutex_onwed(_lock)
is required?

BTW, do you have any idea to fix this panic?


Thanks,

-- 
//
Internet Initiative Japan Inc.

Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit

Kengo NAKAHARA 

Re: CVS commit: src/sys/arch/x86/x86

[Kamil Rytarowski]

I've found this article:

http://coypu.sdf.org/20170525-qemu-smp

On 31.05.2017 08:51, Jaromír Doleček wrote:
> You rock! Thank you.
> 
> 2017-05-31 2:19 GMT+02:00 Maya Rashish  >:
> 
> Module Name:src
> Committed By:   maya
> Date:   Wed May 31 00:19:17 UTC 2017
> 
> Modified Files:
> src/sys/arch/x86/x86: cpu.c
> 
> Log Message:
> Do not pause many times between testing if the CPU can go.
> 
> This only impacts QEMU as QEMU's implementation of pause is
> significantly slower than its implementation of nop.
> 
> PR kern/51623: running qemu-x86_64 with -smp 4 - the additional
> CPUs don't start.
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.125 -r1.126 src/sys/arch/x86/x86/cpu.c
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
> 
> 




signature.asc
Description: OpenPGP digital signature

Re: CVS commit: src/sys/arch/x86/x86

[Jaromír Doleček]

You rock! Thank you.

2017-05-31 2:19 GMT+02:00 Maya Rashish :

> Module Name:src
> Committed By:   maya
> Date:   Wed May 31 00:19:17 UTC 2017
>
> Modified Files:
> src/sys/arch/x86/x86: cpu.c
>
> Log Message:
> Do not pause many times between testing if the CPU can go.
>
> This only impacts QEMU as QEMU's implementation of pause is
> significantly slower than its implementation of nop.
>
> PR kern/51623: running qemu-x86_64 with -smp 4 - the additional
> CPUs don't start.
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.125 -r1.126 src/sys/arch/x86/x86/cpu.c
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>
>

Re: CVS commit: src/sys/arch/x86/pci

[coypu]

>  #define VMXNET3_CORE_LOCK_ASSERT(_sc)mutex_owned((_sc)->vmx_mtx)
>  
>  #define VMXNET3_RXQ_LOCK_ASSERT(_rxq)\
>  mutex_owned((_rxq)->vxrxq_mtx)

>  #define VMXNET3_TXQ_LOCK_ASSERT(_txq)\
>  mutex_owned((_txq)->vxtxq_mtx)

These should probably assert for real (coverity is
complaining of this :)), but I'm not sure that the
asserts don't fire.

Re: CVS commit: src/sys/arch/x86

[Jaromír Doleček]

I see Robert added hack meanwhile, I'll look at proper fix tomorrow.

Jaromir

2016-10-16 0:23 GMT+02:00 Joerg Sonnenberger :
> On Sat, Oct 15, 2016 at 04:46:15PM +, Jaromir Dolecek wrote:
>> Module Name:  src
>> Committed By: jdolecek
>> Date: Sat Oct 15 16:46:14 UTC 2016
>>
>> Modified Files:
>>   src/sys/arch/x86/acpi: acpi_machdep.c
>>   src/sys/arch/x86/include: isa_machdep.h
>>   src/sys/arch/x86/isa: isa_machdep.c
>>   src/sys/arch/x86/pci: pciide_machdep.c
>>
>> Log Message:
>> provide intr xname
>
> You broke the Xen kernels on AMD64. Xen's machine/intr.h version is
> different.
>
> Joerg

Re: CVS commit: src/sys/arch/x86

[Robert Elz]

Date:Sun, 16 Oct 2016 02:14:27 +0200
From:Joerg Sonnenberger 
Message-ID:  <20161016001427.ga7...@britannica.bec.de>

  | > I haven't compile tested it yet, but I think it needs:
  | See second sentence :)

Obviously I had ... I just didn't originally interpret it as intended.

When my build finishes (or aborts), and if Jaromir hasn't fixed it in the
interim, I'll see if I can see something that will put the build back to
a working state (without reverting changes) - even if it isn't as complete
as it should be.

It may be that adding

#define isa_intr_establish_xname(a,b,c,d,e,f,g) isa_intr_establish(a,b,c,d,e,f)

in xen/include/intr.h might be enough to cobble things together for now.

kre

Re: CVS commit: src/sys/arch/x86

[Joerg Sonnenberger]

On Sun, Oct 16, 2016 at 06:59:53AM +0700, Robert Elz wrote:
> Date:Sun, 16 Oct 2016 00:23:20 +0200
> From:Joerg Sonnenberger 
> Message-ID:  <20161015222320.ga4...@britannica.bec.de>
> 
>   | On Sat, Oct 15, 2016 at 04:46:15PM +, Jaromir Dolecek wrote:
> 
>   | > Modified Files:
>   | > src/sys/arch/x86/acpi: acpi_machdep.c
> 
>   | > Log Message:
>   | > provide intr xname
>   | 
>   | You broke the Xen kernels on AMD64. Xen's machine/intr.h version is
>   | different.
> 
> I haven't compile tested it yet, but I think it needs:

See second sentence :)

Joerg

Re: CVS commit: src/sys/arch/x86

[Robert Elz]

Date:Sun, 16 Oct 2016 00:23:20 +0200
From:Joerg Sonnenberger 
Message-ID:  <20161015222320.ga4...@britannica.bec.de>

  | On Sat, Oct 15, 2016 at 04:46:15PM +, Jaromir Dolecek wrote:

  | > Modified Files:
  | >   src/sys/arch/x86/acpi: acpi_machdep.c

  | > Log Message:
  | > provide intr xname
  | 
  | You broke the Xen kernels on AMD64. Xen's machine/intr.h version is
  | different.

I haven't compile tested it yet, but I think it needs:

Index: acpi_machdep.c
===
RCS file: /cvsroot/src/sys/arch/x86/acpi/acpi_machdep.c,v
retrieving revision 1.14
diff -u -r1.14 acpi_machdep.c
--- acpi_machdep.c  15 Oct 2016 16:46:14 -  1.14
+++ acpi_machdep.c  15 Oct 2016 23:58:12 -
@@ -53,6 +53,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 

Re: CVS commit: src/sys/arch/x86

[Joerg Sonnenberger]

On Sat, Oct 15, 2016 at 04:46:15PM +, Jaromir Dolecek wrote:
> Module Name:  src
> Committed By: jdolecek
> Date: Sat Oct 15 16:46:14 UTC 2016
> 
> Modified Files:
>   src/sys/arch/x86/acpi: acpi_machdep.c
>   src/sys/arch/x86/include: isa_machdep.h
>   src/sys/arch/x86/isa: isa_machdep.c
>   src/sys/arch/x86/pci: pciide_machdep.c
> 
> Log Message:
> provide intr xname

You broke the Xen kernels on AMD64. Xen's machine/intr.h version is
different.

Joerg

Re: CVS commit: src/sys/arch/x86/x86

[Maxime Villard]

Le 20/07/2016 à 07:07, Takashi YAMAMOTO a écrit :



On Wed, Jul 20, 2016 at 3:54 AM, Maxime Villard > wrote:

Module Name:src
Committed By:   maxv
Date:   Tue Jul 19 18:54:45 UTC 2016

Modified Files:
src/sys/arch/x86/x86: pmap.c

Log Message:
This loop makes no sense at all.


- can you provide more meaningful commit message?

- why don't you remove it then?



Look at the loop. It does not do anything. The bug was introduced six years
ago in this commit [1], and there is clearly a mistake: the guy just added
a loop around the code, but now the 'continue' refers to that new loop and
not the underlying one.

I could have fixed it to keep the original behavior, but in fact, I don't
even see why this hack is relevant. This hack is supposed to make sure we
never write-protect the PTE space, but as far as I can tell, the userland
space is below it, and the kernel space is above it. So if the kernel or
userland ever tries to write-protect this area, UVM will figure out it is
not included in the space and will reject the request.

In all cases, even if there were a way for someone to write-protect the PTE
space, this hack would still be wrong: some pages in the range wouldn't be
protected, and this is something the caller may not handle properly. And
even beyond that, if write-protecting the PTE space were possible, it would
be a huge flow in the design.

So all this makes absolutely no sense at all. I didn't want to spend too
much time on it, so I just added an XXX, for the next passer-by. I guess the
best thing to do would be turning the 'continue' to a 'panic'; or just
removing the loop completely, since the KASSERT below does mostly the same
thing.

[1] 
http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/arch/x86/x86/pmap.c?only_with_tag=MAIN#rev1.112

Re: CVS commit: src/sys/arch/x86/x86

[Takashi YAMAMOTO]

On Wed, Jul 20, 2016 at 3:54 AM, Maxime Villard  wrote:

> Module Name:src
> Committed By:   maxv
> Date:   Tue Jul 19 18:54:45 UTC 2016
>
> Modified Files:
> src/sys/arch/x86/x86: pmap.c
>
> Log Message:
> This loop makes no sense at all.
>

- can you provide more meaningful commit message?

- why don't you remove it then?


>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.211 -r1.212 src/sys/arch/x86/x86/pmap.c
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>
>

Re: CVS commit: src/sys/arch/x86/x86

[Kengo NAKAHARA]

Hi,

On 2016/07/12 0:21, David Holland wrote:
> On Mon, Jul 11, 2016 at 09:42:20AM +, Kengo NAKAHARA wrote:
>  > Log Message:
>  > strncpy should use destination buf length instead of source buf length.
>  > 
>  > pointed out by nonaka@n.o.
> 
> this should almost certainly be strlcpy, not strncpy.

Thank you for your point out. I fix it.


Thanks,

-- 
//
Internet Initiative Japan Inc.

Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit

Kengo NAKAHARA 

Re: CVS commit: src/sys/arch/x86/x86

[David Holland]

On Mon, Jul 11, 2016 at 09:42:20AM +, Kengo NAKAHARA wrote:
 > Log Message:
 > strncpy should use destination buf length instead of source buf length.
 > 
 > pointed out by nonaka@n.o.

this should almost certainly be strlcpy, not strncpy.

-- 
David A. Holland
dholl...@netbsd.org

Re: CVS commit: src/sys/arch/x86/x86

[Maxime Villard]

Le 01/07/2016 à 16:22, matthew green a écrit :

We use only one L4 slot for the direct map, which means that we cannot
map more than 512GB. Panic properly if this limit is reached.


thanks for making the failure mode clear.

it would be nice to remove this limitation, and support upto
at least 16TB of ram.  systems with well over 512GB are not
entirely uncommon in 2016, and the future is coming ;)



NetBSD cannot support up to 512GB. The page levels used for the direct map are
stored below the IOM area in physical memory. If the machine has more than
~160GB, and the CPU does not support superpages, the page levels and the IOM
area collide, and the last page levels basically get overwritten.

Re: CVS commit: src/sys/arch/x86/x86

[Maxime Villard]

Le 01/07/2016 à 16:24, matthew green a écrit :

Log Message:
Surprisingly enough, the kernel expects the CPU to support large pages
when creating the direct map on amd64. Therefore, the amd64 CPUs that do
not support large pages basically don't work on NetBSD.

It looks like it has always been this way; add a KASSERT to panic
properly in case we come across one of these CPUs.


this seems kind of useless.  it's part of the definition of amd64.


Yes, I found it in AMD64 Vol.2 p.169 right after committing this change. It
still remains useful, since it makes sure we first went through the largepages
initialization stuff.

re: CVS commit: src/sys/arch/x86/x86

[matthew green]

> Log Message:
> Surprisingly enough, the kernel expects the CPU to support large pages
> when creating the direct map on amd64. Therefore, the amd64 CPUs that do
> not support large pages basically don't work on NetBSD.
> 
> It looks like it has always been this way; add a KASSERT to panic
> properly in case we come across one of these CPUs.

this seems kind of useless.  it's part of the definition of amd64.
there are heaps of things that would fail besides this (and not
just netbsd.)  should we assert other basic features exist instead
of assuming they do because of the context?  i don't think so.


.mrg.

re: CVS commit: src/sys/arch/x86/x86

[matthew green]

> We use only one L4 slot for the direct map, which means that we cannot
> map more than 512GB. Panic properly if this limit is reached.

thanks for making the failure mode clear.

it would be nice to remove this limitation, and support upto
at least 16TB of ram.  systems with well over 512GB are not
entirely uncommon in 2016, and the future is coming ;)

(we'll need a bunch of fixes in pmap and uvm to go beyond
this - we have some 32 bit page number limitations to deal
with.)


.mrg.

Re: CVS commit: src/sys/arch/x86/x86

[coypu]

On Fri, Jul 01, 2016 at 11:44:05AM +, Maxime Villard wrote:
> Log Message:
> Use pmap_bootstrap_valloc and pmap_bootstrap_palloc under XEN at least
> once, for these not to appear as unused functions (not tested, but I
> guess).

Maybe __used or ifndef XEN?

Re: CVS commit: src/sys/arch/x86/x86

[Jean-Yves Migeon]

Le 09/10/2015 06:49, Masanobu SAITOH a écrit :
> On 2015/10/06 6:10, Jean-Yves Migeon wrote:
>>> Log Message:
>>> kmem_free() the address returned by kmem_alloc().  found by Brainy.
>>> use the newly aligned location if we needed it.  found by kre.
>>>
>>>
>>> To generate a diff of this commit:
>>> cvs rdiff -u -r1.8 -r1.9 src/sys/arch/x86/x86/cpu_ucode_intel.c
>>
>> IMHO this should be pulled-up to -6 and -7.
>>
>> Any argument against? If the old code worked, it's pure luck.
> 
>  netbsd-6 doesn't support the microcode update function for Intel
> CPU. That bug should be pulled up to netbsd-7 (and netbsd-7-1) branch.

Makes sense. I'll check and ask for this pullup.

-- 
Jean-Yves Migeon

Re: CVS commit: src/sys/arch/x86/x86

[Masanobu SAITOH]

On 2015/10/06 6:10, Jean-Yves Migeon wrote:

Le 04/10/2015 19:52, matthew green a écrit :

Module Name:src
Committed By:   mrg
Date:   Sun Oct  4 17:52:50 UTC 2015

Modified Files:
src/sys/arch/x86/x86: cpu_ucode_intel.c

Log Message:
kmem_free() the address returned by kmem_alloc().  found by Brainy.
use the newly aligned location if we needed it.  found by kre.


To generate a diff of this commit:
cvs rdiff -u -r1.8 -r1.9 src/sys/arch/x86/x86/cpu_ucode_intel.c


IMHO this should be pulled-up to -6 and -7.

Any argument against? If the old code worked, it's pure luck.


 netbsd-6 doesn't support the microcode update function for Intel
CPU. That bug should be pulled up to netbsd-7 (and netbsd-7-1) branch.


--
---
SAITOH Masanobu (msai...@execsw.org
 msai...@netbsd.org)

Re: CVS commit: src/sys/arch/x86/x86

[Jean-Yves Migeon]

Le 04/10/2015 19:52, matthew green a écrit :
> Module Name:  src
> Committed By: mrg
> Date: Sun Oct  4 17:52:50 UTC 2015
> 
> Modified Files:
>   src/sys/arch/x86/x86: cpu_ucode_intel.c
> 
> Log Message:
> kmem_free() the address returned by kmem_alloc().  found by Brainy.
> use the newly aligned location if we needed it.  found by kre.
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.8 -r1.9 src/sys/arch/x86/x86/cpu_ucode_intel.c

IMHO this should be pulled-up to -6 and -7.

Any argument against? If the old code worked, it's pure luck.

-- 
Jean-Yves Migeon

Re: CVS commit: src/sys/arch/x86/pci

[Masanobu SAITOH]

On 2015/08/13 13:52, SAITOH Masanobu wrote:

Module Name:src
Committed By:   msaitoh
Date:   Thu Aug 13 04:52:40 UTC 2015

Modified Files:
src/sys/arch/x86/pci: msipic.c

Log Message:
  Add workaround for PCI prefetchable bit in msipic_construct_msix_pic().
Some chips (e.g. Intel 82599) report SERR and MSI-X interrupt doesn't work.
This problem might not be the driver's bug but our PCI common part or VMs'
bug. See fxp(4), ixgbe(4) and ixgbe(4). All of them has the same workaround


s/ixgbe(4) and ixgbe(4)/bge(4) and ixgbe(4)/


related to prefetchable bit. For the MSI-X table area, it should not have side
effect by prefetching. Until we find a real reason, we ignore the prefetchable
bit.


To generate a diff of this commit:
cvs rdiff -u -r1.6 -r1.7 src/sys/arch/x86/pci/msipic.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.




--
---
SAITOH Masanobu (msai...@execsw.org
 msai...@netbsd.org)

Re: CVS commit: src/sys/arch/x86/x86

[Masanobu SAITOH]

On 2015/05/18 22:09, SAITOH Masanobu wrote:

Module Name:src
Committed By:   msaitoh
Date:   Mon May 18 13:09:55 UTC 2015

Modified Files:
src/sys/arch/x86/x86: cpu.c

Log Message:
  PS. Revert previous.


To generate a diff of this commit:
cvs rdiff -u -r1.114 -r1.115 src/sys/arch/x86/x86/cpu.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


 Sorry. I accidentally committed my test implementation to support
Intel rdrand instruction for rnd(4). That feasture should be attached
as a driver via cpufeature bus.

--
---
SAITOH Masanobu (msai...@execsw.org
 msai...@netbsd.org)

Re: CVS commit: src/sys/arch/x86/pci

[Masao Uebayashi]

On Sat, Oct 18, 2014 at 5:55 AM, Masao Uebayashi uebay...@netbsd.org wrote:
 Module Name:src
 Committed By:   uebayasi
 Date:   Fri Oct 17 20:55:21 UTC 2014

 Modified Files:
 src/sys/arch/x86/pci: files.pci

 Log Message:
 Fix another indirect circular dependency (agp_* - (agpbus) - pchb - abp_*).
 Fixes no agp* build.  Reported  build-tested by Kurt Schreiner.

Correction: agp_* - agp - (agpbus) - pchb - agp_*

Here agpbus is an interface attribute (bus).

Re: CVS commit: src/sys/arch/x86/x86

[David Laight]

On Tue, Oct 14, 2014 at 03:16:56AM +, John Nemeth wrote:
 Module Name:  src
 Committed By: jnemeth
 Date: Tue Oct 14 03:16:56 UTC 2014
 
 Modified Files:
   src/sys/arch/x86/x86: identcpu.c
 
 Log Message:
 Force x86_xsave_features to 0 when running under XEN for AMD
 processors.  This prevents the use of xsave and xrstor thus fixing
 the problem in PR/49150.  The basic problem is that the way AMD
 implements those instructions means that information can leak
 between domains so XEN treats them as privileged.
 
 XXX If anybody else comes up with a better / more proper fix, go
 for it.  However, this solves the problem I was having.  And, given
 that XEN being broken is pretty much a show-stopper for a release,
 something needed to be done.

One option would be to detect the fault at runtime.

David

-- 
David Laight: da...@l8s.co.uk

Re: CVS commit: src/sys/arch/x86/acpi

[Jukka Ruohonen]

On Thu, Apr 17, 2014 at 12:01:24PM -0400, Christos Zoulas wrote:
 Module Name:  src
 Committed By: christos
 Date: Thu Apr 17 16:01:24 UTC 2014
 
 Modified Files:
   src/sys/arch/x86/acpi: acpi_cpu_md.c
 
 Log Message:
 CID/1203191: Out of bounds read

Oh, nasty. The code was pretty much copy-pasted from ../x86/x86/est.c,
which might be therefore worth to briefly check too.

- Jukka.