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/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/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/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/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/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/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/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/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/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/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/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/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/x86

[David Laight]

On Tue, Apr 01, 2014 at 07:16:18AM +, David Laight wrote:
 Module Name:  src
 Committed By: dsl
 Date: Tue Apr  1 07:16:18 UTC 2014
 
 Modified Files:
   src/sys/arch/x86/x86: x86_machdep.c
 
 Log Message:
 Revert most of the machdep sysctls to 32bit

I think I remember why I set to 64bit now.
Stopped all the lines wrapping 80 columns.
And having looked at how/where the data was stored thought it
wouldn't matter.

It is also a real PITA that the entire internals of the kernel sysctl
code are exposed to userspace.

David

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

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

[Jukka Ruohonen]

On Tue, Dec 10, 2013 at 02:02:41PM +0900, Masanobu SAITOH wrote:
  How about the following patch? Is it ok to commit?

Looks good to me!

- Jukka.

 Index: sys/arch/x86/acpi/acpi_cpu_md.c
 ===
 RCS file: /cvsroot/src/sys/arch/x86/acpi/acpi_cpu_md.c,v
 retrieving revision 1.74
 diff -u -r1.74 acpi_cpu_md.c
 --- sys/arch/x86/acpi/acpi_cpu_md.c   20 Nov 2013 13:52:30 -  1.74
 +++ sys/arch/x86/acpi/acpi_cpu_md.c   10 Dec 2013 04:55:11 -
 @@ -43,6 +43,7 @@
  #include x86/cpuvar.h
  #include x86/cpu_msr.h
  #include x86/machdep.h
 +#include x86/x86/tsc.h
  
  #include dev/acpi/acpica.h
  #include dev/acpi/acpi_cpu.h
 @@ -162,6 +163,16 @@
*/
   val |= ACPICPU_FLAG_C_APIC | ACPICPU_FLAG_C_TSC;
  
 + /*
 +  * Detect whether TSC is invariant. If it is not, we keep the flag to
 +  * note that TSC will not run at constant rate. Depending on the CPU,
 +  * this may affect P- and T-state changes, but especially relevant
 +  * are C-states; with variant TSC, states larger than C1 may
 +  * completely stop the counter.
 +  */
 + if (tsc_is_invariant())
 + val = ~ACPICPU_FLAG_C_TSC;
 +
   switch (cpu_vendor) {
  
   case CPUVENDOR_IDT:
 @@ -214,24 +225,6 @@
   val = ~ACPICPU_FLAG_C_APIC;
   }
  
 - /*
 -  * Detect whether TSC is invariant. If it is not,
 -  * we keep the flag to note that TSC will not run
 -  * at constant rate. Depending on the CPU, this may
 -  * affect P- and T-state changes, but especially
 -  * relevant are C-states; with variant TSC, states
 -  * larger than C1 may completely stop the counter.
 -  */
 - x86_cpuid(0x8000, regs);
 -
 - if (regs[0] = 0x8007) {
 -
 - x86_cpuid(0x8007, regs);
 -
 - if ((regs[3]  __BIT(8)) != 0)
 - val = ~ACPICPU_FLAG_C_TSC;
 - }
 -
   break;
  
   case CPUVENDOR_AMD:
 @@ -284,13 +277,10 @@
   case 0x15: /* AMD Bulldozer */
  
   /*
 -  * Like with Intel, detect invariant TSC,
 -  * MSR-based P-states, and AMD's turbo
 -  * (Core Performance Boost), respectively.
 +  * Like with Intel, detect MSR-based P-states,
 +  * and AMD's turbo (Core Performance Boost),
 +  * respectively.
*/
 - if ((regs[3]  CPUID_APM_TSC) != 0)
 - val = ~ACPICPU_FLAG_C_TSC;
 -
   if ((regs[3]  CPUID_APM_HWP) != 0)
   val |= ACPICPU_FLAG_P_FFH;
  
 Index: sys/arch/x86/x86/tsc.c
 ===
 RCS file: /cvsroot/src/sys/arch/x86/x86/tsc.c,v
 retrieving revision 1.34
 diff -u -r1.34 tsc.c
 --- sys/arch/x86/x86/tsc.c8 Dec 2013 04:07:38 -   1.34
 +++ sys/arch/x86/x86/tsc.c10 Dec 2013 04:55:11 -
 @@ -63,23 +63,19 @@
   .tc_quality = 3000,
  };
  
 -void
 -tsc_tc_init(void)
 +bool
 +tsc_is_invariant(void)
  {
   struct cpu_info *ci;
   uint32_t descs[4];
   uint32_t family;
   bool invariant;
  
 - if (!cpu_hascounter()) {
 - return;
 - }
 + if (!cpu_hascounter())
 + return false;
  
   ci = curcpu();
   invariant = false;
 - tsc_freq = ci-ci_data.cpu_cc_freq;
 - tsc_good = (cpu_feature[0]  CPUID_MSR) != 0 
 - (rdmsr(MSR_TSC) != 0 || rdmsr(MSR_TSC) != 0);
  
   if (cpu_vendor == CPUVENDOR_INTEL) {
   /*
 @@ -140,6 +136,24 @@
   }
   }
  
 + return invariant;
 +}
 +
 +void
 +tsc_tc_init(void)
 +{
 + struct cpu_info *ci;
 + bool invariant;
 +
 + if (!cpu_hascounter())
 + return;
 +
 + ci = curcpu();
 + tsc_freq = ci-ci_data.cpu_cc_freq;
 + tsc_good = (cpu_feature[0]  CPUID_MSR) != 0 
 + (rdmsr(MSR_TSC) != 0 || rdmsr(MSR_TSC) != 0);
 +
 + invariant = tsc_is_invariant();
   if (!invariant) {
   aprint_debug(TSC not known invariant on this CPU\n);
   tsc_timecounter.tc_quality = -100;
 Index: sys/arch/x86/x86/tsc.h
 ===
 RCS file: /cvsroot/src/sys/arch/x86/x86/tsc.h,v
 retrieving revision 1.4
 diff -u -r1.4 tsc.h
 --- sys/arch/x86/x86/tsc.h10 May 2008 16:12:32 -  1.4
 +++ sys/arch/x86/x86/tsc.h10 Dec 2013 04:55:11 -
 @@ -26,6 +26,7 @@
   * POSSIBILITY OF SUCH DAMAGE.
   */
  
 +bool tsc_is_invariant(void);
  void tsc_tc_init(void);
  void tsc_sync_ap(struct cpu_info *);
  void tsc_sync_bp(struct cpu_info *);
 
 
 -- 
 ---
 SAITOH Masanobu 

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

[Masanobu SAITOH]

(2013/12/10 19:15), Jukka Ruohonen wrote:

On Tue, Dec 10, 2013 at 02:02:41PM +0900, Masanobu SAITOH wrote:

  How about the following patch? Is it ok to commit?


Looks good to me!


 Committed!

 Thanks.

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

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

[Masanobu SAITOH]

(2013/12/09 0:24), SAITOH Masanobu wrote:

(2013/12/08 19:21), Jukka Ruohonen wrote:

On Sun, Dec 08, 2013 at 04:07:38AM +, SAITOH Masanobu wrote:

Module Name:src
Committed By:   msaitoh
Date:   Sun Dec  8 04:07:38 UTC 2013

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

Log Message:
  Update invariant TSC detect code from both Intel and AMD documents.
The best way to check whether the TSC counter is invariant or not is to check
CPUID 8007.


It might be worth to refactor this as a function, given that the code is
replicated in arch/x86/acpi/acpi_cpu_md.c.

- Jukkal.


  Yes, the code can be shared. I'm new to x86 and I'm not familiar
with acpi, so please do it if you can :)

  For last few months, I have been reading documents and code around
CPUID and MSR registers. While reading the code, I knew I had to check
ACPI stuff, but I've not done it yet :-



 How about the following patch? Is it ok to commit?

Index: sys/arch/x86/acpi/acpi_cpu_md.c
===
RCS file: /cvsroot/src/sys/arch/x86/acpi/acpi_cpu_md.c,v
retrieving revision 1.74
diff -u -r1.74 acpi_cpu_md.c
--- sys/arch/x86/acpi/acpi_cpu_md.c 20 Nov 2013 13:52:30 -  1.74
+++ sys/arch/x86/acpi/acpi_cpu_md.c 10 Dec 2013 04:55:11 -
@@ -43,6 +43,7 @@
 #include x86/cpuvar.h
 #include x86/cpu_msr.h
 #include x86/machdep.h
+#include x86/x86/tsc.h
 
 #include dev/acpi/acpica.h

 #include dev/acpi/acpi_cpu.h
@@ -162,6 +163,16 @@
 */
val |= ACPICPU_FLAG_C_APIC | ACPICPU_FLAG_C_TSC;
 
+	/*

+* Detect whether TSC is invariant. If it is not, we keep the flag to
+* note that TSC will not run at constant rate. Depending on the CPU,
+* this may affect P- and T-state changes, but especially relevant
+* are C-states; with variant TSC, states larger than C1 may
+* completely stop the counter.
+*/
+   if (tsc_is_invariant())
+   val = ~ACPICPU_FLAG_C_TSC;
+
switch (cpu_vendor) {
 
 	case CPUVENDOR_IDT:

@@ -214,24 +225,6 @@
val = ~ACPICPU_FLAG_C_APIC;
}
 
-		/*

-* Detect whether TSC is invariant. If it is not,
-* we keep the flag to note that TSC will not run
-* at constant rate. Depending on the CPU, this may
-* affect P- and T-state changes, but especially
-* relevant are C-states; with variant TSC, states
-* larger than C1 may completely stop the counter.
-*/
-   x86_cpuid(0x8000, regs);
-
-   if (regs[0] = 0x8007) {
-
-   x86_cpuid(0x8007, regs);
-
-   if ((regs[3]  __BIT(8)) != 0)
-   val = ~ACPICPU_FLAG_C_TSC;
-   }
-
break;
 
 	case CPUVENDOR_AMD:

@@ -284,13 +277,10 @@
case 0x15: /* AMD Bulldozer */
 
 			/*

-* Like with Intel, detect invariant TSC,
-* MSR-based P-states, and AMD's turbo
-* (Core Performance Boost), respectively.
+* Like with Intel, detect MSR-based P-states,
+* and AMD's turbo (Core Performance Boost),
+* respectively.
 */
-   if ((regs[3]  CPUID_APM_TSC) != 0)
-   val = ~ACPICPU_FLAG_C_TSC;
-
if ((regs[3]  CPUID_APM_HWP) != 0)
val |= ACPICPU_FLAG_P_FFH;
 
Index: sys/arch/x86/x86/tsc.c

===
RCS file: /cvsroot/src/sys/arch/x86/x86/tsc.c,v
retrieving revision 1.34
diff -u -r1.34 tsc.c
--- sys/arch/x86/x86/tsc.c  8 Dec 2013 04:07:38 -   1.34
+++ sys/arch/x86/x86/tsc.c  10 Dec 2013 04:55:11 -
@@ -63,23 +63,19 @@
.tc_quality = 3000,
 };
 
-void

-tsc_tc_init(void)
+bool
+tsc_is_invariant(void)
 {
struct cpu_info *ci;
uint32_t descs[4];
uint32_t family;
bool invariant;
 
-	if (!cpu_hascounter()) {

-   return;
-   }
+   if (!cpu_hascounter())
+   return false;
 
 	ci = curcpu();

invariant = false;
-   tsc_freq = ci-ci_data.cpu_cc_freq;
-   tsc_good = (cpu_feature[0]  CPUID_MSR) != 0 
-   (rdmsr(MSR_TSC) != 0 || rdmsr(MSR_TSC) != 0);
 
 	if (cpu_vendor == CPUVENDOR_INTEL) {

/*
@@ -140,6 +136,24 @@
}
}
 
+	return invariant;

+}
+
+void
+tsc_tc_init(void)
+{
+   struct cpu_info *ci;
+   bool invariant;
+
+   if (!cpu_hascounter())
+   return;
+
+   ci = curcpu();
+   tsc_freq = ci-ci_data.cpu_cc_freq;
+   tsc_good = (cpu_feature[0]  CPUID_MSR) != 0 
+   (rdmsr(MSR_TSC) != 0 || rdmsr(MSR_TSC) != 0);
+
+   invariant = 

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

[Jukka Ruohonen]

On Sun, Dec 08, 2013 at 04:07:38AM +, SAITOH Masanobu wrote:
 Module Name:  src
 Committed By: msaitoh
 Date: Sun Dec  8 04:07:38 UTC 2013
 
 Modified Files:
   src/sys/arch/x86/x86: tsc.c
 
 Log Message:
  Update invariant TSC detect code from both Intel and AMD documents.
 The best way to check whether the TSC counter is invariant or not is to check
 CPUID 8007.

It might be worth to refactor this as a function, given that the code is
replicated in arch/x86/acpi/acpi_cpu_md.c.

- Jukkal.

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

[SAITOH Masanobu]

(2013/12/08 19:21), Jukka Ruohonen wrote:
 On Sun, Dec 08, 2013 at 04:07:38AM +, SAITOH Masanobu wrote:
 Module Name: src
 Committed By:msaitoh
 Date:Sun Dec  8 04:07:38 UTC 2013

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

 Log Message:
  Update invariant TSC detect code from both Intel and AMD documents.
 The best way to check whether the TSC counter is invariant or not is to check
 CPUID 8007.
 
 It might be worth to refactor this as a function, given that the code is
 replicated in arch/x86/acpi/acpi_cpu_md.c.
 
 - Jukkal.

 Yes, the code can be shared. I'm new to x86 and I'm not familiar
with acpi, so please do it if you can :)

 For last few months, I have been reading documents and code around
CPUID and MSR registers. While reading the code, I knew I had to check
ACPI stuff, but I've not done it yet :-

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

  * 英語 - 自動検出
  * 英語
  * 日本語

  * 英語
  * 日本語

 javascript:void(0); #

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

[Masanobu SAITOH]

 Hi, Christos.

(2013/11/12 2:02), Christos Zoulas wrote:

Module Name:src
Committed By:   christos
Date:   Mon Nov 11 17:02:53 UTC 2013

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

Log Message:
CID 1128377: Comment out unreachable code; model is only 4 bits wide, so
none of these constants can ever match.


To generate a diff of this commit:
cvs rdiff -u -r1.14 -r1.15 src/sys/arch/x86/x86/intel_busclock.c

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


 Thank you for the change. I was wrong and our code is little
bad about using (extended) cpu family and (extended) cpu model.
I'll write the change to fix the problem and to reduce the code
duplication.

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

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

[David Laight]

On Wed, Jun 26, 2013 at 08:37:35PM -0400, Christos Zoulas wrote:
 Module Name:  src
 Committed By: christos
 Date: Thu Jun 27 00:37:35 UTC 2013
 
 Modified Files:
   src/sys/arch/x86/x86: tsc.c
 
 Log Message:
 detect a bad msr tsc and don't use it.

+   tsc_good = (cpu_feature[0]  CPUID_MSR) != 0  rdmsr(MSR_TSC) != 0;

There is a very small chance that the TSC will read as zero.
So the check should probably be:
tsc_good = (cpu_feature[0]  CPUID_MSR) != 0 
(rdmsr(MSR_TSC) != 0 || rdmsr(MSR_TSC) != 0);

The:
+extern int cpu_msr_tsc;
looks bogus - debug ??

David

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

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

[matthew green]

 Module Name:  src
 Committed By: christos
 Date: Wed Jun 26 20:52:28 UTC 2013
 
 Modified Files:
   src/sys/arch/x86/x86: identcpu.c
 
 Log Message:
 PR/47967: Jeff Rizzo: Add a probe for QEMU to disable it from claiming it
 has MSR_TSC. Fixes DTRACE crashing because it returned a frequency of 0.

wouldn't a better work around be to not use the serialise
version if it returns 0 always?


.mrg.

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

[Cherry G. Mathew]

On 26 February 2012 01:33, Cherry G. Mathew che...@netbsd.org wrote:
 Module Name:    src
 Committed By:   cherry
 Date:           Sat Feb 25 20:03:58 UTC 2012

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

 Log Message:
 Revert previous since it does a redundant xpq queue flush.
 xen_bcast_invlpg() flushes the queue for us.

As bouyer@ kindly pointed out offline.

Cheers,

-- 
~Cherry

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

[Thomas Klausner]

Hi Cherry!
On Fri, Feb 24, 2012 at 08:44:45AM +, Cherry G. Mathew wrote:
 Module Name:  src
 Committed By: cherry
 Date: Fri Feb 24 08:44:44 UTC 2012
 
 Modified Files:
   src/sys/arch/x86/x86: pmap.c
 
 Log Message:
 kernel page attribute change should be reflected on all cpus, since
 the page is going to be released after the _dtor() hook is called.

Can you please explain why you reverted the previous version and now use 
xen_bcast*?
 Thomas

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

[Cherry G. Mathew]

Hi Thomas,

On 24 February 2012 14:21, Thomas Klausner w...@netbsd.org wrote:
 Hi Cherry!
 On Fri, Feb 24, 2012 at 08:44:45AM +, Cherry G. Mathew wrote:
 Module Name:  src
 Committed By: cherry
 Date:         Fri Feb 24 08:44:44 UTC 2012

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

 Log Message:
 kernel page attribute change should be reflected on all cpus, since
 the page is going to be released after the _dtor() hook is called.

 Can you please explain why you reverted the previous version and now use 
 xen_bcast*?
  Thomas

That was a typo that I committed by mistake. xpq_bcast*() does not
exist. Sorry about that.

Cheers,

-- 
~Cherry

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

[Joerg Sonnenberger]

On Fri, Dec 09, 2011 at 05:32:51PM +, Chuck Silvers wrote:
 Module Name:  src
 Committed By: chs
 Date: Fri Dec  9 17:32:51 UTC 2011
 
 Modified Files:
   src/sys/arch/x86/x86: pmap.c
 
 Log Message:
 only use PG_G on leaf PTEs.
 go back to tlbflush(), all the global entries
 that we create in pmap_bootstrap() are permanent.

I think using tlbflushg is better, since it alters the global mapping.
It's not like this code path is performance critical.

Joerg

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

[Marc Balmer]

Am 18.10.11 06:27, schrieb Jukka Ruohonen:
 On Tue, Oct 18, 2011 at 12:07:45AM +, Jared D. McNeill wrote:
 Module Name: src
 Committed By:jmcneill
 Date:Tue Oct 18 00:07:45 UTC 2011

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

 Log Message:
 don't allow module autounload
 
 I wonder should autounloading be prohibited for all driver-class modules?

Why?  When the parent goes away, why not autounload a driver?

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

[Jukka Ruohonen]

On Tue, Oct 18, 2011 at 08:43:46AM +0200, Marc Balmer wrote:
 Am 18.10.11 06:27, schrieb Jukka Ruohonen:
  On Tue, Oct 18, 2011 at 12:07:45AM +, Jared D. McNeill wrote:
  Module Name:   src
  Committed By:  jmcneill
  Date:  Tue Oct 18 00:07:45 UTC 2011
 
  Modified Files:
 src/sys/arch/x86/x86: vmt.c
 
  Log Message:
  don't allow module autounload
  
  I wonder should autounloading be prohibited for all driver-class modules?
 
 Why?  When the parent goes away, why not autounload a driver?

I am not sure. But have we thought about all the consequences and corner-
cases? Unloading happens while modifying hardware state? Deferred calls
in the drivers? And so on? To me it also seems that if I manually load
a driver-module, I expect it to stay loaded until I unload it.

- Jukka.

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

[David Brownlee]

On 18 October 2011 10:07, Jukka Ruohonen jruoho...@iki.fi wrote:
 On Tue, Oct 18, 2011 at 08:43:46AM +0200, Marc Balmer wrote:
 Am 18.10.11 06:27, schrieb Jukka Ruohonen:
  On Tue, Oct 18, 2011 at 12:07:45AM +, Jared D. McNeill wrote:
  Module Name:       src
  Committed By:      jmcneill
  Date:              Tue Oct 18 00:07:45 UTC 2011
 
  Modified Files:
     src/sys/arch/x86/x86: vmt.c
 
  Log Message:
  don't allow module autounload
 
  I wonder should autounloading be prohibited for all driver-class modules?

 Why?  When the parent goes away, why not autounload a driver?

 I am not sure. But have we thought about all the consequences and corner-
 cases? Unloading happens while modifying hardware state? Deferred calls
 in the drivers? And so on? To me it also seems that if I manually load
 a driver-module, I expect it to stay loaded until I unload it.

Presumably whether to permit autounload should be an option that can
be specified at manual module load time, then the default is less
important.

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

[Jared McNeill]

I would argue that any manually loaded module shouldn't be autounloaded. 
What do you think about flagging modules as autoloaded and only 
autounloading the autoloaded ones?



On Tue, 18 Oct 2011, Jukka Ruohonen wrote:


On Tue, Oct 18, 2011 at 08:43:46AM +0200, Marc Balmer wrote:

Am 18.10.11 06:27, schrieb Jukka Ruohonen:

On Tue, Oct 18, 2011 at 12:07:45AM +, Jared D. McNeill wrote:

Module Name:src
Committed By:   jmcneill
Date:   Tue Oct 18 00:07:45 UTC 2011

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

Log Message:
don't allow module autounload


I wonder should autounloading be prohibited for all driver-class modules?


Why?  When the parent goes away, why not autounload a driver?


I am not sure. But have we thought about all the consequences and corner-
cases? Unloading happens while modifying hardware state? Deferred calls
in the drivers? And so on? To me it also seems that if I manually load
a driver-module, I expect it to stay loaded until I unload it.

- Jukka.

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

[Jukka Ruohonen]

On Tue, Oct 18, 2011 at 06:39:49AM -0400, Jared McNeill wrote:
 I would argue that any manually loaded module shouldn't be autounloaded. 
 What do you think about flagging modules as autoloaded and only 
 autounloading the autoloaded ones?

That sounds right to me.

As noted, generally I am not sure what my opinion about autounloading really
is. But it feels somewhat awkward and error-prone that drivers must protect
themselves against the autounloading kthread.

- Jukka.

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

[Iain Hibbert]

On Tue, 18 Oct 2011, Jukka Ruohonen wrote:

 On Tue, Oct 18, 2011 at 06:39:49AM -0400, Jared McNeill wrote:
  I would argue that any manually loaded module shouldn't be autounloaded.
  What do you think about flagging modules as autoloaded and only
  autounloading the autoloaded ones?

 That sounds right to me.

 As noted, generally I am not sure what my opinion about autounloading really
 is. But it feels somewhat awkward and error-prone that drivers must protect
 themselves against the autounloading kthread.

How often is autounloading actually effectively used anyway?  I mean, if
the module is loaded automatically, it is because the system found that it
was needed.

So, yes.. there situation where eg USB or PCMCIA devices might have a
transient need for a driver, but on the other hand, the overhead of a
driver being in memory is not that great considering that you used it
once, as the chance of re-use is significant (higher by far than all the
other drivers that have never been needed)

The real benefit of the modular system is that you don't need to load
hundreds of modules on the off chance that they will be used. A cron entry
could be used to flush unused modules if the sysop cares about that, why
do we need a kthread running?

iain

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

[Jukka Ruohonen]

On Tue, Oct 18, 2011 at 12:44:52PM +0100, Iain Hibbert wrote:
 The real benefit of the modular system is that you don't need to load
 hundreds of modules on the off chance that they will be used. A cron entry
 could be used to flush unused modules if the sysop cares about that, why
 do we need a kthread running?

A good question worth asking. And as far as I remember, the autounloading
cycle was as low as 10 seconds (!), whereas a proper scale would be measured
in minutes or hours in my opinion (or even as daily(5)).

- Jukka.

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

[Paul Goyette]

On Tue, 18 Oct 2011, Jukka Ruohonen wrote:


On Tue, Oct 18, 2011 at 06:39:49AM -0400, Jared McNeill wrote:

I would argue that any manually loaded module shouldn't be autounloaded.
What do you think about flagging modules as autoloaded and only
autounloading the autoloaded ones?


That sounds right to me.


I was under the impression that we would already only auto-unload 
modules that were auto-loaded.  The comment in module_thread() says


 *  Automatically unload modules.  We try once to unload autoloaded
 *  modules after module_autotime seconds.  ...

module_thread() skips modules with mod-mod_autotime of zero, and at the 
end of module_do_load() we have


if (autoload) {
/*
 * Arrange to try unloading the module after
 * a short delay.
 */
mod-mod_autotime = time_second + module_autotime;
module_thread_kick();
}

So mod-mod_autotime is only set if the module was autoloaded.


-
| Paul Goyette | PGP Key fingerprint: | E-mail addresses:   |
| Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com|
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net |
| Kernel Developer |  | pgoyette at netbsd.org  |
-

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

[Jared D. McNeill]

Well I loaded vmt manually and it was auto unloaded on me a few minutes later, 
that's why I made this change in the first place.

On 2011-10-18, at 7:57 AM, Paul Goyette wrote:

 On Tue, 18 Oct 2011, Jukka Ruohonen wrote:
 
 On Tue, Oct 18, 2011 at 06:39:49AM -0400, Jared McNeill wrote:
 I would argue that any manually loaded module shouldn't be autounloaded.
 What do you think about flagging modules as autoloaded and only
 autounloading the autoloaded ones?
 
 That sounds right to me.
 
 I was under the impression that we would already only auto-unload modules 
 that were auto-loaded.  The comment in module_thread() says
 
 * Automatically unload modules.  We try once to unload autoloaded
 * modules after module_autotime seconds.  ...
 
 module_thread() skips modules with mod-mod_autotime of zero, and at the end 
 of module_do_load() we have
 
   if (autoload) {
   /*
* Arrange to try unloading the module after
* a short delay.
*/
   mod-mod_autotime = time_second + module_autotime;
   module_thread_kick();
   }
 
 So mod-mod_autotime is only set if the module was autoloaded.
 
 
 -
 | Paul Goyette | PGP Key fingerprint: | E-mail addresses:   |
 | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com|
 | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net |
 | Kernel Developer |  | pgoyette at netbsd.org  |
 -

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

[Jukka Ruohonen]

On Tue, Oct 18, 2011 at 07:59:35AM -0400, Jared D. McNeill wrote:
 Well I loaded vmt manually and it was auto unloaded on me a few minutes
 later, that's why I made this change in the first place.
 
 On 2011-10-18, at 7:57 AM, Paul Goyette wrote:
 
  On Tue, 18 Oct 2011, Jukka Ruohonen wrote:
  
  On Tue, Oct 18, 2011 at 06:39:49AM -0400, Jared McNeill wrote:
  I would argue that any manually loaded module shouldn't be autounloaded.
  What do you think about flagging modules as autoloaded and only
  autounloading the autoloaded ones?
  
  That sounds right to me.
  
  I was under the impression that we would already only auto-unload
  modules that were auto-loaded.  The comment in module_thread() says
  
  *   Automatically unload modules.  We try once to unload autoloaded
  *   modules after module_autotime seconds.  ...

True. But like Jared, I've also seen magic unloading of some of my driver
modules. I guess there is a bug somewhere.

- Jukka.

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

[Jared D. McNeill]

No wonder, in the thread if uvmexp.free  uvmexp.freemin it skips the 
mod_autotime == 0 check.

On 2011-10-18, at 8:06 AM, Jukka Ruohonen wrote:

 On Tue, Oct 18, 2011 at 07:59:35AM -0400, Jared D. McNeill wrote:
 Well I loaded vmt manually and it was auto unloaded on me a few minutes
 later, that's why I made this change in the first place.
 
 On 2011-10-18, at 7:57 AM, Paul Goyette wrote:
 
 On Tue, 18 Oct 2011, Jukka Ruohonen wrote:
 
 On Tue, Oct 18, 2011 at 06:39:49AM -0400, Jared McNeill wrote:
 I would argue that any manually loaded module shouldn't be autounloaded.
 What do you think about flagging modules as autoloaded and only
 autounloading the autoloaded ones?
 
 That sounds right to me.
 
 I was under the impression that we would already only auto-unload
 modules that were auto-loaded.  The comment in module_thread() says
 
 *   Automatically unload modules.  We try once to unload autoloaded
 *   modules after module_autotime seconds.  ...
 
 True. But like Jared, I've also seen magic unloading of some of my driver
 modules. I guess there is a bug somewhere.
 
 - Jukka.

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

[Jared D. McNeill]

Fixed.

On 2011-10-18, at 8:06 AM, Jukka Ruohonen wrote:
 
 True. But like Jared, I've also seen magic unloading of some of my driver
 modules. I guess there is a bug somewhere.
 
 - Jukka.

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

[Warner Losh]

On Oct 18, 2011, at 4:39 AM, Jared McNeill wrote:
 I would argue that any manually loaded module shouldn't be autounloaded. What 
 do you think about flagging modules as autoloaded and only autounloading the 
 autoloaded ones?

If I manually load a dozen drivers at boot because I have a dozen different 
boards with different devices.  I'd kinda like the system to automatically 
figure out what isn't needed and unload those drivers.

Warner

 On Tue, 18 Oct 2011, Jukka Ruohonen wrote:
 
 On Tue, Oct 18, 2011 at 08:43:46AM +0200, Marc Balmer wrote:
 Am 18.10.11 06:27, schrieb Jukka Ruohonen:
 On Tue, Oct 18, 2011 at 12:07:45AM +, Jared D. McNeill wrote:
 Module Name:  src
 Committed By: jmcneill
 Date: Tue Oct 18 00:07:45 UTC 2011
 
 Modified Files:
   src/sys/arch/x86/x86: vmt.c
 
 Log Message:
 don't allow module autounload
 
 I wonder should autounloading be prohibited for all driver-class modules?
 
 Why?  When the parent goes away, why not autounload a driver?
 
 I am not sure. But have we thought about all the consequences and corner-
 cases? Unloading happens while modifying hardware state? Deferred calls
 in the drivers? And so on? To me it also seems that if I manually load
 a driver-module, I expect it to stay loaded until I unload it.
 
 - Jukka.
 
 
 
 

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

[Warner Losh]

On Oct 18, 2011, at 9:28 AM, Jukka Ruohonen wrote:

 On Tue, Oct 18, 2011 at 08:49:39AM -0600, Warner Losh wrote:
 
 On Oct 18, 2011, at 4:39 AM, Jared McNeill wrote:
 I would argue that any manually loaded module shouldn't be autounloaded. 
 What do you think about flagging modules as autoloaded and only 
 autounloading the autoloaded ones?
 
 If I manually load a dozen drivers at boot because I have a dozen
 different boards with different devices.  I'd kinda like the system to
 automatically figure out what isn't needed and unload those drivers.
 
 The general idea here probably is that you shouldn't actually load manually
 all those drivers, but the system should be able to autoload modules specific
 to your hardware. But we are not there yet. And AFAIR, neither is FreeBSD.

That's true.  It is a hard problem that people have been working around by 
manually loading things. :)

Warner

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

[Marc Balmer]

Am 18.10.11 12:39, schrieb Jared McNeill:
 I would argue that any manually loaded module shouldn't be autounloaded.
 What do you think about flagging modules as autoloaded and only
 autounloading the autoloaded ones?

I'd say that is a safe way.  I am for it.

 
 
 On Tue, 18 Oct 2011, Jukka Ruohonen wrote:
 
 On Tue, Oct 18, 2011 at 08:43:46AM +0200, Marc Balmer wrote:
 Am 18.10.11 06:27, schrieb Jukka Ruohonen:
 On Tue, Oct 18, 2011 at 12:07:45AM +, Jared D. McNeill wrote:
 Module Name:src
 Committed By:jmcneill
 Date:Tue Oct 18 00:07:45 UTC 2011

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

 Log Message:
 don't allow module autounload

 I wonder should autounloading be prohibited for all driver-class
 modules?

 Why?  When the parent goes away, why not autounload a driver?

 I am not sure. But have we thought about all the consequences and corner-
 cases? Unloading happens while modifying hardware state? Deferred calls
 in the drivers? And so on? To me it also seems that if I manually load
 a driver-module, I expect it to stay loaded until I unload it.

 - Jukka.




-- 
  \~.The NetBSD Foundation
   \~'   Marc Balmer, Developer / Marketing
  NetBSD
 \   mbal...@netbsd.org   http://www.NetBSD.org/

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

[Marc Balmer]

Am 18.10.11 13:44, schrieb Iain Hibbert:
 On Tue, 18 Oct 2011, Jukka Ruohonen wrote:
 
 On Tue, Oct 18, 2011 at 06:39:49AM -0400, Jared McNeill wrote:
 I would argue that any manually loaded module shouldn't be autounloaded.
 What do you think about flagging modules as autoloaded and only
 autounloading the autoloaded ones?

When I manually load gpiosim, it will autoload gpio.  Now when I
manually modunload gpiosim, gpio will stay there.  But in this case, I
want gpio to be autounloaded eventually.


 That sounds right to me.

 As noted, generally I am not sure what my opinion about autounloading really
 is. But it feels somewhat awkward and error-prone that drivers must protect
 themselves against the autounloading kthread.
 
 How often is autounloading actually effectively used anyway?  I mean, if
 the module is loaded automatically, it is because the system found that it
 was needed.
 
 So, yes.. there situation where eg USB or PCMCIA devices might have a
 transient need for a driver, but on the other hand, the overhead of a
 driver being in memory is not that great considering that you used it
 once, as the chance of re-use is significant (higher by far than all the
 other drivers that have never been needed)
 
 The real benefit of the modular system is that you don't need to load
 hundreds of modules on the off chance that they will be used. A cron entry
 could be used to flush unused modules if the sysop cares about that, why
 do we need a kthread running?
 

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

[Paul Goyette]

On Tue, 18 Oct 2011, Jukka Ruohonen wrote:


On Tue, Oct 18, 2011 at 12:07:45AM +, Jared D. McNeill wrote:

Module Name:src
Committed By:   jmcneill
Date:   Tue Oct 18 00:07:45 UTC 2011

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

Log Message:
don't allow module autounload


I wonder should autounloading be prohibited for all driver-class modules?


Why?  Doesn't it make more sense to have a driver keep track of whether 
or not it has attached any instances, and prohibit the unload if the 
count is non-zero?   If a device appears later, the driver can always be 
re-loaded...



-
| Paul Goyette | PGP Key fingerprint: | E-mail addresses:   |
| Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com|
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net |
| Kernel Developer |  | pgoyette at netbsd.org  |
-