Re: CVS commit: src/sys/arch
On Tue, 8 Nov 2011 08:49:22 +0530, Cherry G. Mathew wrote:
Please keep ci_pae_l3_pdir to a uint32_t and back out its paddr_t
type.
Unless you can convince me that your code will do the right
thing(tm), the
PDP for PAE should to be allocated below the 4GiB physical boundary;
unless
mistaken, the uvm_km_alloc(kernel_map, ...) does not enforce that.
Besides,
%cr3 is still a 32 bits entity under PAE. Trying to stash a 64 bits
paddr_t
in %cr3 is a really bad idea.
See comments
http://nxr.netbsd.org/xref/src/sys/arch/x86/x86/pmap.c#1588
This doesn't hold true for Xen. See:
http://lxr.xensource.com/lxr/source/xen/arch/x86/mm.c#L509
Xen does the 4GB translation for the VM by maintaining its own
shadow. I just took the easier route ( after initially using the x86
pmap 4GB cr3 code ), but if you're thinking of a unified a
xen/non-xen implementation we'll have to re-think this one.
Okay; then all users of cr3 have to be documented (_especially_ the
native x86 code), as there at least two places where there's implicit
uint64_t = uint32_t truncation: lcr3() and pcb_cr3. A comment above
these should be enough. Same goes for having paddr_t for L3 PD in struct
cpu_info.
- are you sure that these have to be
defined(PAE) || defined(__x86_64__) ?
- please use for (i = 0; i PTP_LEVELS - 1; i++ ) { ... }. I will
have to
identify these blocks later when GENERIC will include both PAE and
!PAE
pmaps. PTP_LEVELS makes it a lot easier.
ok, will watchout for it - do you want to do this one yourself ?
Please do :)
--
Jean-Yves Migeon
jeanyves.mig...@free.fr
Re: CVS commit: src/sys/arch
On 8 November 2011 05:50, Jean-Yves Migeon jeanyves.mig...@free.fr wrote: On 06.11.2011 16:18, Cherry G. Mathew wrote: Module Name: src Committed By: cherry Date: Sun Nov 6 15:18:19 UTC 2011 Modified Files: src/sys/arch/amd64/include: pmap.h src/sys/arch/i386/include: pmap.h src/sys/arch/x86/include: cpu.h src/sys/arch/x86/x86: pmap.c src/sys/arch/xen/x86: cpu.c x86_xpmap.c Log Message: [merging from cherry-xenmp] make pmap_kernel() shadow PMD per-cpu and MP aware. Some comments. ... -/* flag to be used for kernel mappings: PG_u on Xen/amd64, 0 otherwise */ -#if defined(XEN) defined(__x86_64__) -#define PG_k PG_u -#else -#define PG_k 0 -#endif - Are you sure that all the mapping sites are safe (PT/PD bits), given the pmap split between pmap/xen_xpmap.c? Ok, I realise I've broken the build with this one - apologies. For some odd reason my tree built ok ( even after nuking the obj dir) The current bandaid by christos and njoly is incorrect. I propose re-exporting PG_k to x86/include/pmap.h until (if ?) the xen pmap is completely independant of the x86 one. Please let me know if there are objections to this patch below: Thanks, -- ~Cherry Index: arch/x86/include/pmap.h === RCS file: /cvsroot/src/sys/arch/x86/include/pmap.h,v retrieving revision 1.44 diff -u -r1.44 pmap.h --- arch/x86/include/pmap.h 6 Nov 2011 11:40:47 - 1.44 +++ arch/x86/include/pmap.h 8 Nov 2011 13:35:16 - @@ -173,6 +173,13 @@ ((pmap)-pm_pdirpa[0] + (index) * sizeof(pd_entry_t)) #endif +/* flag to be used for kernel mappings: PG_u on Xen/amd64, 0 otherwise */ +#if defined(XEN) defined(__x86_64__) +#define PG_k PG_u +#else +#define PG_k 0 +#endif + /* * MD flags that we use for pmap_enter and pmap_kenter_pa: */ Index: arch/x86/x86/pmap.c === RCS file: /cvsroot/src/sys/arch/x86/x86/pmap.c,v retrieving revision 1.140 diff -u -r1.140 pmap.c --- arch/x86/x86/pmap.c 8 Nov 2011 12:44:29 - 1.140 +++ arch/x86/x86/pmap.c 8 Nov 2011 13:35:20 - @@ -211,11 +211,6 @@ #include xen/hypervisor.h #endif -/* If this is not needed anymore it should be GC'ed */ -#ifndef PG_k -#definePG_k0 -#endif - /* * general info: * Index: arch/xen/include/xenpmap.h === RCS file: /cvsroot/src/sys/arch/xen/include/xenpmap.h,v retrieving revision 1.30 diff -u -r1.30 xenpmap.h --- arch/xen/include/xenpmap.h 6 Nov 2011 11:40:47 - 1.30 +++ arch/xen/include/xenpmap.h 8 Nov 2011 13:35:20 - @@ -34,13 +34,6 @@ #include opt_xen.h #endif -/* flag to be used for kernel mappings: PG_u on Xen/amd64, 0 otherwise */ -#if defined(XEN) defined(__x86_64__) -#define PG_k PG_u -#else -#define PG_k 0 -#endif - #defineINVALID_P2M_ENTRY (~0UL) void xpq_queue_machphys_update(paddr_t, paddr_t); Index: arch/xen/x86/xen_pmap.c === RCS file: /cvsroot/src/sys/arch/xen/x86/xen_pmap.c,v retrieving revision 1.7 diff -u -r1.7 xen_pmap.c --- arch/xen/x86/xen_pmap.c 6 Nov 2011 11:40:47 - 1.7 +++ arch/xen/x86/xen_pmap.c 8 Nov 2011 13:35:21 - @@ -142,13 +142,6 @@ #include xen/hypervisor.h #endif -/* flag to be used for kernel mappings: PG_u on Xen/amd64, 0 otherwise */ -#if defined(XEN) defined(__x86_64__) -#define PG_k PG_u -#else -#define PG_k 0 -#endif - #define COUNT(x) /* nothing */ static pd_entry_t * const alternate_pdes[] = APDES_INITIALIZER;
Re: CVS commit: src/sys/arch
On 8 November 2011 15:20, Jean-Yves Migeon jeanyves.mig...@free.fr wrote:
On Tue, 8 Nov 2011 08:49:22 +0530, Cherry G. Mathew wrote:
Please keep ci_pae_l3_pdir to a uint32_t and back out its paddr_t type.
Unless you can convince me that your code will do the right thing(tm),
the
PDP for PAE should to be allocated below the 4GiB physical boundary;
unless
mistaken, the uvm_km_alloc(kernel_map, ...) does not enforce that.
Besides,
%cr3 is still a 32 bits entity under PAE. Trying to stash a 64 bits
paddr_t
in %cr3 is a really bad idea.
See comments http://nxr.netbsd.org/xref/src/sys/arch/x86/x86/pmap.c#1588
This doesn't hold true for Xen. See:
http://lxr.xensource.com/lxr/source/xen/arch/x86/mm.c#L509
Xen does the 4GB translation for the VM by maintaining its own
shadow. I just took the easier route ( after initially using the x86
pmap 4GB cr3 code ), but if you're thinking of a unified a
xen/non-xen implementation we'll have to re-think this one.
Okay; then all users of cr3 have to be documented (_especially_ the native
x86 code), as there at least two places where there's implicit uint64_t =
uint32_t truncation: lcr3() and pcb_cr3. A comment above these should be
enough. Same goes for having paddr_t for L3 PD in struct cpu_info.
hmm... I wonder if it would be cleaner to do this within a #ifdef
XEN/#endif ?
- are you sure that these have to be
defined(PAE) || defined(__x86_64__) ?
That's a crude way of making pmap_cpu_init_late() do nothing for i386
(non-pae) since i386 doesn't need shadow PMDs.
- please use for (i = 0; i PTP_LEVELS - 1; i++ ) { ... }. I will have
to
identify these blocks later when GENERIC will include both PAE and !PAE
pmaps. PTP_LEVELS makes it a lot easier.
ok, will watchout for it - do you want to do this one yourself ?
Please do :)
ok, I'll take flak for further breakage ;-P
Cheers,
--
~Cherry
Re: CVS commit: src/sys/arch
On 8 November 2011 19:11, Cherry G. Mathew cherry.g.mat...@gmail.com wrote: On 8 November 2011 05:50, Jean-Yves Migeon jeanyves.mig...@free.fr wrote: On 06.11.2011 16:18, Cherry G. Mathew wrote: Module Name: src Committed By: cherry Date: Sun Nov 6 15:18:19 UTC 2011 Modified Files: src/sys/arch/amd64/include: pmap.h src/sys/arch/i386/include: pmap.h src/sys/arch/x86/include: cpu.h src/sys/arch/x86/x86: pmap.c src/sys/arch/xen/x86: cpu.c x86_xpmap.c Log Message: [merging from cherry-xenmp] make pmap_kernel() shadow PMD per-cpu and MP aware. Some comments. ... -/* flag to be used for kernel mappings: PG_u on Xen/amd64, 0 otherwise */ -#if defined(XEN) defined(__x86_64__) -#define PG_k PG_u -#else -#define PG_k 0 -#endif - Are you sure that all the mapping sites are safe (PT/PD bits), given the pmap split between pmap/xen_xpmap.c? Ok, I realise I've broken the build with this one - apologies. For some odd reason my tree built ok ( even after nuking the obj dir) The current bandaid by christos and njoly is incorrect. I propose re-exporting PG_k to x86/include/pmap.h until (if ?) the xen pmap is completely independant of the x86 one. Please let me know if there are objections to this patch below: Thanks, -- ~Cherry Index: arch/x86/include/pmap.h === RCS file: /cvsroot/src/sys/arch/x86/include/pmap.h,v retrieving revision 1.44 diff -u -r1.44 pmap.h --- arch/x86/include/pmap.h 6 Nov 2011 11:40:47 - 1.44 +++ arch/x86/include/pmap.h 8 Nov 2011 13:35:16 - @@ -173,6 +173,13 @@ ((pmap)-pm_pdirpa[0] + (index) * sizeof(pd_entry_t)) #endif +/* flag to be used for kernel mappings: PG_u on Xen/amd64, 0 otherwise */ +#if defined(XEN) defined(__x86_64__) +#define PG_k PG_u +#else +#define PG_k 0 +#endif + /* * MD flags that we use for pmap_enter and pmap_kenter_pa: */ Index: arch/x86/x86/pmap.c === RCS file: /cvsroot/src/sys/arch/x86/x86/pmap.c,v retrieving revision 1.140 diff -u -r1.140 pmap.c --- arch/x86/x86/pmap.c 8 Nov 2011 12:44:29 - 1.140 +++ arch/x86/x86/pmap.c 8 Nov 2011 13:35:20 - @@ -211,11 +211,6 @@ #include xen/hypervisor.h #endif -/* If this is not needed anymore it should be GC'ed */ -#ifndef PG_k -#define PG_k 0 -#endif - /* * general info: * Index: arch/xen/include/xenpmap.h === RCS file: /cvsroot/src/sys/arch/xen/include/xenpmap.h,v retrieving revision 1.30 diff -u -r1.30 xenpmap.h --- arch/xen/include/xenpmap.h 6 Nov 2011 11:40:47 - 1.30 +++ arch/xen/include/xenpmap.h 8 Nov 2011 13:35:20 - @@ -34,13 +34,6 @@ #include opt_xen.h #endif -/* flag to be used for kernel mappings: PG_u on Xen/amd64, 0 otherwise */ -#if defined(XEN) defined(__x86_64__) -#define PG_k PG_u -#else -#define PG_k 0 -#endif - #define INVALID_P2M_ENTRY (~0UL) void xpq_queue_machphys_update(paddr_t, paddr_t); Index: arch/xen/x86/xen_pmap.c === RCS file: /cvsroot/src/sys/arch/xen/x86/xen_pmap.c,v retrieving revision 1.7 diff -u -r1.7 xen_pmap.c --- arch/xen/x86/xen_pmap.c 6 Nov 2011 11:40:47 - 1.7 +++ arch/xen/x86/xen_pmap.c 8 Nov 2011 13:35:21 - @@ -142,13 +142,6 @@ #include xen/hypervisor.h #endif -/* flag to be used for kernel mappings: PG_u on Xen/amd64, 0 otherwise */ -#if defined(XEN) defined(__x86_64__) -#define PG_k PG_u -#else -#define PG_k 0 -#endif - #define COUNT(x) /* nothing */ static pd_entry_t * const alternate_pdes[] = APDES_INITIALIZER; Committed. Thanks, -- ~Cherry
Re: CVS commit: src/external/bsd/atf/dist/atf-c
In article 2007202432.ga7...@britannica.bec.de, Joerg Sonnenberger jo...@britannica.bec.de wrote: On Mon, Nov 07, 2011 at 04:06:30PM +, Christos Zoulas wrote: Well, I tried to print the failing pattern in t_expand, and it silently got truncated. dprintf(3) has been part of TOG since 2006: (http://pubs.opengroup.org/onlinepubs/9699919799/functions/dprintf.html) So it would be preferable to implement it in terms of asprintf/write + or snprintf/malloc/write or even fdopen/fprintf instead of open coding it. From reading the patch, there are three different brances, right? Why don't you use writev for this with up to 6 elements in the vector? Done. christos
Re: CVS commit: src/share/man/man5
In article 4eb84f56.6060...@netbsd.org, Marc Balmer mbal...@netbsd.org wrote: Am 07.11.11 21:01, schrieb Joerg Sonnenberger: On Mon, Nov 07, 2011 at 02:27:03PM +0200, Jukka Ruohonen wrote: On Mon, Nov 07, 2011 at 10:20:38AM +0100, Joerg Sonnenberger wrote: The majority of interesting programs already have USE_FORT=yes. But USE_FORT is not the same thing as USE_SSP. It is stronger. How so? Please explain the difference. man ssp, /_FORTIFY/ christos
Re: CVS commit: src/sys/arch
On 08.11.2011 14:53, Cherry G. Mathew wrote:
On 8 November 2011 15:20, Jean-Yves Migeonjeanyves.mig...@free.fr
wrote:
On Tue, 8 Nov 2011 08:49:22 +0530, Cherry G. Mathew wrote:
Please keep ci_pae_l3_pdir to a uint32_t and back out its
paddr_t type.
Unless you can convince me that your code will do the right
thing(tm), the PDP for PAE should to be allocated below the
4GiB physical boundary; unless mistaken, the
uvm_km_alloc(kernel_map, ...) does not enforce that. Besides,
%cr3 is still a 32 bits entity under PAE. Trying to stash a 64
bits paddr_t in %cr3 is a really bad idea.
See comments
http://nxr.netbsd.org/xref/src/sys/arch/x86/x86/pmap.c#1588
This doesn't hold true for Xen. See:
http://lxr.xensource.com/lxr/source/xen/arch/x86/mm.c#L509
Xen does the4GB translation for the VM by maintaining its own
shadow. I just took the easier route ( after initially using the
x86 pmap 4GB cr3 code ), but if you're thinking of a unified a
xen/non-xen implementation we'll have to re-think this one.
Okay; then all users of cr3 have to be documented (_especially_
the
native
x86 code), as there at least two places where there's implicit
uint64_t =
uint32_t truncation: lcr3() and pcb_cr3. A comment above these
should be enough. Same goes for having paddr_t for L3 PD in struct
cpu_info.
hmm... I wonder if it would be cleaner to do this within a #ifdef
XEN/#endif ?
The cleanest way would be to share the code between x86 and Xen, keep
the allocation below 4GiB boundary for both, and use it everywhere in
the pmap code. Only the manipulation of the vcpu_guest_context_t
ctrlregs members would have to force this use.
Avoiding the use of macros is a big plus; it helps modularity.
- are you sure that these have to be defined(PAE) ||
defined(__x86_64__) ?
That's a crude way of making pmap_cpu_init_late() do nothing for
i386 (non-pae) since i386 doesn't need shadow PMDs.
In that case, test against i386_use_pae (0 == !PAE, 1 == PAE), and
simply return if !PAE. Avoid having loonnng macro blocks with
different levels of #ifdef. It's fairly difficult to untangle and
unpleasant to read.
Macros should only be used to fix compile-time issues (like: symbol
missing), or help code readability by reuse. Not for jumping around C
code and take shortcuts. This has to be done at execution time rather
than compile time.
- please use for (i = 0; i PTP_LEVELS - 1; i++ ) { ... }.
I will have
to identify these blocks later when GENERIC will include both
PAE and !PAE pmaps. PTP_LEVELS makes it a lot easier.
ok, will watchout for it - do you want to do this one yourself ?
Please do :)
ok, I'll take flak for further breakage ;-P
Expect stress testing for MP in the near future :o
--
Jean-Yves Migeon
jeanyves.mig...@free.fr
Re: CVS commit: src/external/bsd/atf/dist/atf-c
On 11/8/11 3:25 PM, Christos Zoulas wrote: In article 2007202432.ga7...@britannica.bec.de, Joerg Sonnenberger jo...@britannica.bec.de wrote: On Mon, Nov 07, 2011 at 04:06:30PM +, Christos Zoulas wrote: Well, I tried to print the failing pattern in t_expand, and it silently got truncated. dprintf(3) has been part of TOG since 2006: (http://pubs.opengroup.org/onlinepubs/9699919799/functions/dprintf.html) So it would be preferable to implement it in terms of asprintf/write + or snprintf/malloc/write or even fdopen/fprintf instead of open coding it. From reading the patch, there are three different brances, right? Why don't you use writev for this with up to 6 elements in the vector? Done. Did you even run any tests after doing this? This is broken. The assertion you added triggers immediately. -- Julio Merino / @jmmv
Re: CVS commit: src/external/bsd/atf/dist/atf-c
On Nov 8, 6:48pm, j...@netbsd.org (Julio Merino) wrote: -- Subject: Re: CVS commit: src/external/bsd/atf/dist/atf-c | On 11/8/11 3:25 PM, Christos Zoulas wrote: | In article 2007202432.ga7...@britannica.bec.de, | Joerg Sonnenberger jo...@britannica.bec.de wrote: | On Mon, Nov 07, 2011 at 04:06:30PM +, Christos Zoulas wrote: | Well, I tried to print the failing pattern in t_expand, and it silently | got truncated. dprintf(3) has been part of TOG since 2006: | (http://pubs.opengroup.org/onlinepubs/9699919799/functions/dprintf.html) | So it would be preferable to implement it in terms of asprintf/write + | or snprintf/malloc/write or even fdopen/fprintf instead of open coding it. | | From reading the patch, there are three different brances, right? | Why don't you use writev for this with up to 6 elements in the vector? | | Done. | | Did you even run any tests after doing this? | | This is broken. The assertion you added triggers immediately. Yes, I did. I actually wrote the test backwards 2ice because INV has inverted logic than regular assert for some reason. christos
Re: CVS commit: src/sys/arch
Hi, On 9 November 2011 02:02, Jean-Yves Migeon jeanyves.mig...@free.fr wrote: On 08.11.2011 14:53, Cherry G. Mathew wrote: On 8 November 2011 15:20, Jean-Yves Migeonjeanyves.mig...@free.fr wrote: On Tue, 8 Nov 2011 08:49:22 +0530, Cherry G. Mathew wrote: Please keep ci_pae_l3_pdir to a uint32_t and back out its paddr_t type. Unless you can convince me that your code will do the right thing(tm), the PDP for PAE should to be allocated below the 4GiB physical boundary; unless mistaken, the uvm_km_alloc(kernel_map, ...) does not enforce that. Besides, %cr3 is still a 32 bits entity under PAE. Trying to stash a 64 bits paddr_t in %cr3 is a really bad idea. See comments http://nxr.netbsd.org/xref/src/sys/arch/x86/x86/pmap.c#1588 This doesn't hold true for Xen. See: http://lxr.xensource.com/lxr/source/xen/arch/x86/mm.c#L509 Xen does the4GB translation for the VM by maintaining its own shadow. I just took the easier route ( after initially using the x86 pmap 4GB cr3 code ), but if you're thinking of a unified a xen/non-xen implementation we'll have to re-think this one. Okay; then all users of cr3 have to be documented (_especially_ the native x86 code), as there at least two places where there's implicit uint64_t = uint32_t truncation: lcr3() and pcb_cr3. A comment above these should be enough. Same goes for having paddr_t for L3 PD in struct cpu_info. hmm... I wonder if it would be cleaner to do this within a #ifdef XEN/#endif ? The cleanest way would be to share the code between x86 and Xen, keep the allocation below 4GiB boundary for both, and use it everywhere in the pmap code. Only the manipulation of the vcpu_guest_context_t ctrlregs members would have to force this use. Fair enough. Although the 4G tests would be a bit deceptive (since they're cosmetic) - I guess leaving a note in the code about the rationale behind this will help. - are you sure that these have to be defined(PAE) || defined(__x86_64__) ? That's a crude way of making pmap_cpu_init_late() do nothing for i386 (non-pae) since i386 doesn't need shadow PMDs. In that case, test against i386_use_pae (0 == !PAE, 1 == PAE), and simply return if !PAE. Avoid having loonnng macro blocks with different levels of #ifdef. It's fairly difficult to untangle and unpleasant to read. I agree - this looks better. Thanks, -- ~Cherry
Re: CVS commit: src/external/bsd/atf/dist/atf-c
On 11/8/11 7:32 PM, Christos Zoulas wrote: Yes, I did. I actually wrote the test backwards 2ice because INV has inverted logic than regular assert for some reason. It doesn't. INV(x) is the same as assert(x) -- or it is supposed to be -- but if it wasn't, things would have broken much earlier everywhere. -- Julio Merino / @jmmv
