Re: CVS commit: src/sys/dev/pci
> On Jan 9, 2020, at 9:02 PM, Masanobu SAITOH wrote: > > The reason why I moved stge_softc to if_stgereg.h was that ipgphy.c > required to check stge's chip revision. So, if there is no any objection, > I'll make new if_stgevar.h and share it with if_stge.c and ipgphy.c. That seems fine. Although it might be preferable to set a property on the parent dev_t and then query that from ipgphy, rather than accessing the softc. -- thorpej
Re: CVS commit: src/sys/dev/pci
On 2020/01/10 13:13, Jason Thorpe wrote: > > >> On Jan 9, 2020, at 6:11 PM, Masanobu SAITOH wrote: >> >> >> Before my change, struct stge_softc is already in if_stgereg.h, >> so I had thought it would be OK to move to it. > > Ah, I don't think I would have put it there when I wrote the driver > originally, so it must have been moved there at some point. Oh, it was me :) http://mail-index.netbsd.org/source-changes/2019/10/07/msg109768.html > In any case, moving it into if_stgereg.h was also an error. > >> >>> Please move them back to if_stge.c. Doing incorrect things simply to >>> reduce the diff against someone else's copy of the code is not something we >>> should be undertaking. >> Two options: >> >> a) Move some structs (including struct stge_softc) and defines >>to if_stge.c > > There is no reason to have if_stgevar.h, because no other modules need these > definitions, so it should all move to if_stge.c. The reason why I moved stge_softc to if_stgereg.h was that ipgphy.c required to check stge's chip revision. So, if there is no any objection, I'll make new if_stgevar.h and share it with if_stge.c and ipgphy.c. > Thanks! > >> >> b) Move them to new if_stgevar.h >> >> Which one is prefer? > > -- thorpej > -- --- SAITOH Masanobu (msai...@execsw.org msai...@netbsd.org)
Re: CVS commit: src/sys/dev/pci
> On Jan 9, 2020, at 6:11 PM, Masanobu SAITOH wrote: > > > Before my change, struct stge_softc is already in if_stgereg.h, > so I had thought it would be OK to move to it. Ah, I don't think I would have put it there when I wrote the driver originally, so it must have been moved there at some point. In any case, moving it into if_stgereg.h was also an error. > >> Please move them back to if_stge.c. Doing incorrect things simply to reduce >> the diff against someone else's copy of the code is not something we should >> be undertaking. > Two options: > > a) Move some structs (including struct stge_softc) and defines >to if_stge.c There is no reason to have if_stgevar.h, because no other modules need these definitions, so it should all move to if_stge.c. Thanks! > > b) Move them to new if_stgevar.h > > Which one is prefer? -- thorpej
re: CVS commit: src/sys/dev/pci
> Two options: > > a) Move some structs (including struct stge_softc) and defines > to if_stge.c > > b) Move them to new if_stgevar.h i've always preferred: - fooreg.h and foovar.h exist only when more than 1 file use them - fooreg.h ONLY has hardware constructs - foovar.h ONLY has software constructs though i tend to only delete single-use foovar.h, not fooreg.h. .mrg.
Re: CVS commit: src/sys/dev/pci
On 2020/01/09 23:08, Jason Thorpe wrote:
>
>
>> On Jan 9, 2020, at 2:54 AM, SAITOH Masanobu wrote:
>>
>> Module Name: src
>> Committed By:msaitoh
>> Date:Thu Jan 9 10:54:16 UTC 2020
>>
>> Modified Files:
>> src/sys/dev/pci: if_stge.c if_stgereg.h
>>
>> Log Message:
>> Reduce diff against OpenBSD. No functional change.
>>
>> - USE CSR_{READ,WRITE}_*() macro.
>> - Move some macros from if_stge.c to if_stgereg.h
>
> This diff is incorrect.
Yes. You're right.
> The macros that were moved to if_stgereg.h are not related to hardware /
> register definitions, but are purely software constructs.
Before my change, struct stge_softc is already in if_stgereg.h,
so I had thought it would be OK to move to it.
> Please move them back to if_stge.c. Doing incorrect things simply to reduce
> the diff against someone else's copy of the code is not something we should
> be undertaking.
Two options:
a) Move some structs (including struct stge_softc) and defines
to if_stge.c
b) Move them to new if_stgevar.h
Which one is prefer?
> -- thorpej
>
--
---
SAITOH Masanobu (msai...@execsw.org
msai...@netbsd.org)
Re: [x86 pmap changes] CVS commit: src/sys/arch
Le 08/01/2020 à 22:50, Andrew Doran a écrit : On Tue, Jan 07, 2020 at 09:39:22AM +0100, Maxime Villard wrote: Module Name:src Committed By: ad Date: Sat Jan 4 22:49:20 UTC 2020 Modified Files: src/sys/arch/x86/include: pmap.h pmap_pv.h src/sys/arch/x86/x86: pmap.c src/sys/arch/xen/x86: xen_pmap.c Log Message: x86 pmap improvements, reducing system time during a build by about 15% on my test machine: This breaks nvmm-intel. I have only given a quick glance, but this change already is wrong: - old_pp->pp_attrs |= pmap_ept_to_pp_attrs(opte); + old_pp->pp_attrs |= pmap_pte_to_pp_attrs(opte); This is an EPT function handling EPT PTEs, so "ept" was correct. Fixing this bug is not sufficient, so it seems that there are more bugs. Reverting the whole change puts nvmm-intel back in a functional state. You can test with this on an Intel CPU: # modload nvmm # /usr/tests/lib/libnvmm/./h_mem_assist This currently gives random crashes. With a couple of typos fixed (PTE -> EPT, now checked in) I see the same FPU DNA exception that Chavdar reports on current-users (in his case with a kernel which doesn't have these pmap changes). It's coming from: vmx_vcpu_guest_fpu_leave() -> fpu_area_save() -> fxsave() What I can tell you is that the fxsave area is definitely writable and correctly aligned but beyond that I have no idea what's causing it. Any suggestions? Cheers, Andrew This FPU issue should be fixed in the latest nvmm_x86_vmx.c, we still have STTS/CLTS (not needed but for debugging) as part of context switches, and when overhauling the FPU code I overlooked that VMX needs special CR0_TS care that SVM doesn't need. Note that dropping STTS/CLTS would probably increase cswitch performance, because updating cr0 is costly. Having said that, I am still hitting a KASSERT related to pmap: kernel diagnostic assertion "ptp->wire_count == 1" failed file ".../x86/x86/pmap.c", line 1969 pmap_freepages pmap_ept_free_ptp pmap_ept_remove pmap_remove uvm_unmap_remove uvmspace_free nvmm_ioctl sys_ioctl Maxime
Re: CVS commit: src/sys/dev/pci
> On Jan 9, 2020, at 2:54 AM, SAITOH Masanobu wrote:
>
> Module Name: src
> Committed By: msaitoh
> Date: Thu Jan 9 10:54:16 UTC 2020
>
> Modified Files:
> src/sys/dev/pci: if_stge.c if_stgereg.h
>
> Log Message:
> Reduce diff against OpenBSD. No functional change.
>
> - USE CSR_{READ,WRITE}_*() macro.
> - Move some macros from if_stge.c to if_stgereg.h
This diff is incorrect. The macros that were moved to if_stgereg.h are not
related to hardware / register definitions, but are purely software constructs.
Please move them back to if_stge.c. Doing incorrect things simply to reduce
the diff against someone else's copy of the code is not something we should be
undertaking.
-- thorpej
