Re: CVS commit: src/sys/dev/fdt

[Jason Thorpe]

> On Jun 20, 2018, at 12:29 PM, Jared McNeill  wrote:
>> Sure, but where is the code for the brcm2835 platform that *processes* it?  
>> I see stuff for parsing console / fb options, but not for standard boot 
>> flags.
> 
> Firmware puts the string in /chosen/bootargs and we pick it up here: 
> https://nxr.netbsd.org/xref/src/sys/arch/evbarm/fdt/fdt_machdep.c#378 
> 
> 
> The standard args are then processed here: 
> https://nxr.netbsd.org/xref/src/sys/arch/evbarm/fdt/fdt_machdep.c#491 
> 
Ok, got it, thanks.

-- thorpej

Re: CVS commit: src/sys/dev/fdt

[Jared McNeill]

> On Jun 20, 2018, at 4:25 PM, Jason Thorpe  wrote:
> 
>> On Jun 20, 2018, at 9:56 AM, Jared McNeill  wrote:
>> 
>> On Wed, 20 Jun 2018, Jason Thorpe wrote:
>> 
>>> ofctl(8) is pretty useless for this purpose because it doesn't show you 
>>> which driver instance has attached to a given device tree node.  As for 
>>> passing generic boot args, it's not obvious how one does that on the RPI 
>>> (and I don't even see the hook that processes the generic boot args 
>>> bcm283x_platform).
>> 
>> https://www.raspberrypi.org/documentation/configuration/cmdline-txt.md
> 
> Sure, but where is the code for the brcm2835 platform that *processes* it?  I 
> see stuff for parsing console / fb options, but not for standard boot flags.

Firmware puts the string in /chosen/bootargs and we pick it up here: 
https://nxr.netbsd.org/xref/src/sys/arch/evbarm/fdt/fdt_machdep.c#378 


The standard args are then processed here: 
https://nxr.netbsd.org/xref/src/sys/arch/evbarm/fdt/fdt_machdep.c#491 

Re: CVS commit: src/sys/compat/sys

[Taylor R Campbell]

> Date: Fri, 15 Jun 2018 19:55:28 +0700
> From: Robert Elz 
> 
> Another way would be
> 
>   static const struct timespec50 zts = { 0 };
> and
>   *ts50 = zts;

I have nothing substantive to add about the question at hand, but one
tiny nit: there is no need for the initializer in this case, and
indeed if you have -Werror=missing-field-initializers as I think we do
by default, writing out the full initializer is a pain.

It suffices to say

static const struct timespec50 zts;

and the implementation will guarantee it is initialized to all
zero/null as appropriate.

Re: CVS commit: src/sys/dev/fdt

[Jason Thorpe]

> On Jun 20, 2018, at 9:56 AM, Jared McNeill  wrote:
> 
> On Wed, 20 Jun 2018, Jason Thorpe wrote:
> 
>> ofctl(8) is pretty useless for this purpose because it doesn't show you 
>> which driver instance has attached to a given device tree node.  As for 
>> passing generic boot args, it's not obvious how one does that on the RPI 
>> (and I don't even see the hook that processes the generic boot args 
>> bcm283x_platform).
> 
> https://www.raspberrypi.org/documentation/configuration/cmdline-txt.md


Sure, but where is the code for the brcm2835 platform that *processes* it?  I 
see stuff for parsing console / fb options, but not for standard boot flags.

-- thorpej

Re: CVS commit: src/sys/arch

[Jaromír Doleček]

2018-06-20 18:03 GMT+02:00 Maxime Villard :
> I haven't tested but looking at the code it seems to me that this part of
> your
> change breaks DAZ on native, and I don't see how it can help xsave on xen by
> the way

Thanks, I'll change the conditional to avoid any potential change of
behaviour for native.

Yes, I know that fxsave is different than xsave, but on Xen it seems
to be somehow connected.

The conditional on XSAVE wad been necessary on Xen according to my
testing - with no-xsave that function faulted, with xsave active it
didn't.

The only difference in CPUID for working and faulting code was the
OSXSAVE bit. Maybe Xen somehow traps/disables fxsave also with
no-xsave, or maybe requires bigger alignment for some reason. I'll
check if  I can find something to make the fxsave case work for Xen.

Jaromir

Re: CVS commit: src/sys/dev/fdt

[Jared McNeill]

On Wed, 20 Jun 2018, Jason Thorpe wrote:


ofctl(8) is pretty useless for this purpose because it doesn't show you which 
driver instance has attached to a given device tree node.  As for passing 
generic boot args, it's not obvious how one does that on the RPI (and I don't 
even see the hook that processes the generic boot args bcm283x_platform).


https://www.raspberrypi.org/documentation/configuration/cmdline-txt.md

Re: CVS commit: src/sys/dev/fdt

[Jason Thorpe]

> On Jun 20, 2018, at 2:06 AM, Jared McNeill  wrote:
> 
> Not a fan of this, some of these paths can get fairly long (boot the 
> VEXPRESS_A15 kernel to see for yourself) and the path is only really useful 
> if you are debugging something.

Sigh.

> If you want to see the debugging information we already have a way to do that 
> with boot -x. You can also inspect the whole tree at runtime with ofctl(8).

ofctl(8) is pretty useless for this purpose because it doesn't show you which 
driver instance has attached to a given device tree node.  As for passing 
generic boot args, it's not obvious how one does that on the RPI (and I don't 
even see the hook that processes the generic boot args bcm283x_platform).


> 
> 
> On Wed, 20 Jun 2018, Jason R Thorpe wrote:
> 
>> Module Name: src
>> Committed By:thorpej
>> Date:Wed Jun 20 05:59:18 UTC 2018
>> 
>> Modified Files:
>>  src/sys/dev/fdt: fdtbus.c
>> 
>> Log Message:
>> In fdtbus_print(), aprint_normal the path to the device (rather than
>> aprint_debug).  This info is every bit as useful as, say, PCI device
>> locations.
>> 
>> 
>> To generate a diff of this commit:
>> cvs rdiff -u -r1.17 -r1.18 src/sys/dev/fdt/fdtbus.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

[Maxime Villard]

Le 20/06/2018 à 16:31, Jaromír Doleček a écrit :

The function is ever called only with x86_fpu_save >= FPU_SAVE_FXSAVE,
so there should be no change in functionality AFAICS.


There is a change in functionality, you added an xsave check in a function
that is fxsave-specific. xsave != fxsave.

The function calls fxsave to get the MXCSR_MASK value. There is absolutely
no reason to set the value to the default __INITIAL_MXCSR_MASK__ if xsave
is not supported -- the availability of xsave does not dictate what should
be the MXCSR_MASK value.

The operation of this function is described in the Intel SDM, section 11.6.3
of 253665-sdm-vol-1.pdf, page 276/482:

1. Establish a 512-byte FXSAVE area in memory.
2. Clear the FXSAVE area to all 0s.
3. Execute the FXSAVE instruction, using the address of the first byte of the
   cleared FXSAVE area as a source operand.
4. Check the value in the MXCSR_MASK field in the FXSAVE image (bytes 28
   through 31).

I haven't tested but looking at the code it seems to me that this part of your
change breaks DAZ on native, and I don't see how it can help xsave on xen by
the way

Re: CVS commit: src/sys/arch

[Jaromír Doleček]

The function is ever called only with x86_fpu_save >= FPU_SAVE_FXSAVE,
so there should be no change in functionality AFAICS.

2018-06-20 10:34 GMT+02:00 Maxime Villard :
> Le 19/06/2018 à 21:50, Jaromir Dolecek a écrit :
>>
>> Module Name:src
>> Committed By:   jdolecek
>> Date:   Tue Jun 19 19:50:19 UTC 2018
>>
>> Modified Files:
>> src/sys/arch/x86/include: fpu.h
>> src/sys/arch/x86/x86: cpu.c fpu.c identcpu.c
>> src/sys/arch/xen/x86: cpu.c
>>
>> Log Message:
>> fix FPU initialization on Xen to allow e.g. AVX when supported by
>> hardware;
>> only use XSAVE when the the CPUID OSXSAVE bit is set, as this seems to be
>> reliable indication
>>
>> tested with Xen 4.2.6 DOM0/DOMU on Intel CPU, without and with no-xsave
>> flag,
>> so should work also on those AMD CPUs, which have XSAVE disabled by
>> default;
>> also tested with Xen DOM0 4.8.3
>>
>> fixes PR kern/50332 by Torbjorn Granlund; sorry it took three years to
>> address
>>
>> XXX pullup netbsd-8
>
>
> I don't understand, and it looks wrong.
>
>  -- fxsave
>
> fpuinit_mxcsr_mask calls fxsave, not xsave. The availability of fxsave is
> controlled by CR4_OSFXSR, not CR4_OSXSAVE. It seems to me that the check you
> added in fpuinit_mxcsr_mask is wrong, because it is confusing fxsave and
> xsave.
>
> [x86_fpu_save = FPU_SAVE_FXSAVE] guarantees fxsave is supported.
>
> fpuinit_mxcsr_mask is called only when this condition is true, so there is
> no
> need for a check in the first place.
>
> The reason I disabled the code on Xen, if I remember correctly, was because
> of a stack alignment problem. For some reason this variable
>
> union savefpu fpusave __aligned(16);
>
> was not always 16-byte aligned on Xen. So fxsave would fault.
>
>  -- xsave
>
> Please keep the initialization logic consistent.
>
> xen/x86/cpu.c
> 554 if (cpu_feature[1] & CPUID2_OSXSAVE)
> 555 cr4 |= CR4_OSXSAVE;
> 556 else {
> 557 x86_xsave_features = 0;
> 558 x86_fpu_save = FPU_SAVE_FXSAVE;
> 559 }
>
> If the availability of xsave on xen is determined by CPUID2_OSXSAVE (as
> opposed to CPUID2_XSAVE on native), then put the code in identcpu.c just
> like native.
>
> That is to say
>
> x86/x86/identcpu.c
> 804 /* See if xsave (for AVX) is supported */
> 805 if ((ci->ci_feat_val[1] & CPUID2_XSAVE) == 0)
> 806 return;
> XXX
> XXX #ifdef XEN
> XXX /* On Xen we also need to check CPUID2_OSXSAVE */
> XXX if ((ci->ci_feat_val[1] & CPUID2_OSXSAVE) == 0)
> XXX return;
> XXX #endif
>
> Note also that your change seems wrong on i386 too, because you enable
> fxsave
> if we don't have xsave, while we may have neither.

Re: CVS commit: src/sys/arch

[Maxime Villard]

Le 19/06/2018 à 21:50, Jaromir Dolecek a écrit :

Module Name:src
Committed By:   jdolecek
Date:   Tue Jun 19 19:50:19 UTC 2018

Modified Files:
src/sys/arch/x86/include: fpu.h
src/sys/arch/x86/x86: cpu.c fpu.c identcpu.c
src/sys/arch/xen/x86: cpu.c

Log Message:
fix FPU initialization on Xen to allow e.g. AVX when supported by hardware;
only use XSAVE when the the CPUID OSXSAVE bit is set, as this seems to be
reliable indication

tested with Xen 4.2.6 DOM0/DOMU on Intel CPU, without and with no-xsave flag,
so should work also on those AMD CPUs, which have XSAVE disabled by default;
also tested with Xen DOM0 4.8.3

fixes PR kern/50332 by Torbjorn Granlund; sorry it took three years to address

XXX pullup netbsd-8


I don't understand, and it looks wrong.

 -- fxsave

fpuinit_mxcsr_mask calls fxsave, not xsave. The availability of fxsave is
controlled by CR4_OSFXSR, not CR4_OSXSAVE. It seems to me that the check you
added in fpuinit_mxcsr_mask is wrong, because it is confusing fxsave and
xsave.

[x86_fpu_save = FPU_SAVE_FXSAVE] guarantees fxsave is supported.

fpuinit_mxcsr_mask is called only when this condition is true, so there is no
need for a check in the first place.

The reason I disabled the code on Xen, if I remember correctly, was because
of a stack alignment problem. For some reason this variable

union savefpu fpusave __aligned(16);

was not always 16-byte aligned on Xen. So fxsave would fault.

 -- xsave

Please keep the initialization logic consistent.

xen/x86/cpu.c
554 if (cpu_feature[1] & CPUID2_OSXSAVE)
555 cr4 |= CR4_OSXSAVE;
556 else {
557 x86_xsave_features = 0;
558 x86_fpu_save = FPU_SAVE_FXSAVE;
559 }

If the availability of xsave on xen is determined by CPUID2_OSXSAVE (as
opposed to CPUID2_XSAVE on native), then put the code in identcpu.c just
like native.

That is to say

x86/x86/identcpu.c
804 /* See if xsave (for AVX) is supported */
805 if ((ci->ci_feat_val[1] & CPUID2_XSAVE) == 0)
806 return;
XXX
XXX #ifdef XEN
XXX /* On Xen we also need to check CPUID2_OSXSAVE */
XXX if ((ci->ci_feat_val[1] & CPUID2_OSXSAVE) == 0)
XXX return;
XXX #endif

Note also that your change seems wrong on i386 too, because you enable fxsave
if we don't have xsave, while we may have neither.

Re: CVS commit: src/sys/dev/fdt

[Jared McNeill]

Not a fan of this, some of these paths can get fairly long (boot the 
VEXPRESS_A15 kernel to see for yourself) and the path is 
only really useful if you are debugging something.


If you want to see the debugging information we already have a way to 
do that with boot -x. You can also inspect the whole tree at runtime with 
ofctl(8).



On Wed, 20 Jun 2018, Jason R Thorpe wrote:


Module Name:src
Committed By:   thorpej
Date:   Wed Jun 20 05:59:18 UTC 2018

Modified Files:
src/sys/dev/fdt: fdtbus.c

Log Message:
In fdtbus_print(), aprint_normal the path to the device (rather than
aprint_debug).  This info is every bit as useful as, say, PCI device
locations.


To generate a diff of this commit:
cvs rdiff -u -r1.17 -r1.18 src/sys/dev/fdt/fdtbus.c

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