Mailing List Archive: [PATCH RFC 02/25] A collection of fixes to Xen common files

[PATCH RFC 02/25] A collection of fixes to Xen common files

stefano.stabellini at eu

Dec 6, 2011, 10:19 AM

Post #1 of 11 (310 views)

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

- call free_xenoprof_pages only ifdef xenoprof;

- define PRI_stime as PRId64 in an header file;

- respect boundaries in is_kernel_*;

- implement is_kernel_rodata.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com>
---
xen/common/domain.c | 2 ++
xen/common/sched_credit2.c | 6 ------
xen/include/xen/kernel.h | 12 +++++++++---
xen/include/xen/time.h | 1 +
4 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 9e355c8..b75fc1d 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -661,7 +661,9 @@ static void complete_domain_destroy(struct rcu_head *head)
sched_destroy_domain(d);

/* Free page used by xen oprofile buffer. */
+#ifdef xenoprof
free_xenoprof_pages(d);
+#endif

for ( i = d->max_vcpus - 1; i >= 0; i-- )
if ( (v = d->vcpu[i]) != NULL )
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 86c4439..73f7138 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -26,12 +26,6 @@
#include <xen/trace.h>
#include <xen/cpu.h>

-#if __i386__
-#define PRI_stime "lld"
-#else
-#define PRI_stime "ld"
-#endif
-
#define d2printk(x...)
//#define d2printk printk

diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
index fd03f74..d5b13e8 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -66,19 +66,25 @@
extern char _start[], _end[];
#define is_kernel(p) ({ \
char *__p = (char *)(unsigned long)(p); \
- (__p >= _start) && (__p <= _end); \
+ (__p >= _start) && (__p < _end); \
})

extern char _stext[], _etext[];
#define is_kernel_text(p) ({ \
char *__p = (char *)(unsigned long)(p); \
- (__p >= _stext) && (__p <= _etext); \
+ (__p >= _stext) && (__p < _etext); \
+})
+
+extern char _srodata[], _erodata[];
+#define is_kernel_rodata(p) ({ \
+ char *__p = (char *)(unsigned long)(p); \
+ (__p >= _srodata) && (__p < _erodata); \
})

extern char _sinittext[], _einittext[];
#define is_kernel_inittext(p) ({ \
char *__p = (char *)(unsigned long)(p); \
- (__p >= _sinittext) && (__p <= _einittext); \
+ (__p >= _sinittext) && (__p < _einittext); \
})

#endif /* _LINUX_KERNEL_H */
diff --git a/xen/include/xen/time.h b/xen/include/xen/time.h
index a194340..31c9ce5 100644
--- a/xen/include/xen/time.h
+++ b/xen/include/xen/time.h
@@ -30,6 +30,7 @@ struct vcpu;
*/

typedef s64 s_time_t;
+#define PRI_stime PRId64

s_time_t get_s_time(void);
unsigned long get_localtime(struct domain *d);
--
1.7.2.5

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH RFC 02/25] A collection of fixes to Xen common files [ In reply to ]

JBeulich at suse

Dec 7, 2011, 1:00 AM

Post #2 of 11 (301 views)

>>> On 06.12.11 at 19:19, <stefano.stabellini@eu.citrix.com> wrote:
> - call free_xenoprof_pages only ifdef xenoprof;

I can't find where this symbol would get defined, so effectively you're
removing the call unconditionally.

> +extern char _srodata[], _erodata[];

While I realize that this isn't currently the case on e.g. the respective
.text symbols, if you introduce new once, please add const to the
declarations *at least* when the section contents really are read only
(in many cases doing so even for writable sections is likely appropriate
too).

> +#define is_kernel_rodata(p) ({ \
> + char *__p = (char *)(unsigned long)(p); \
> + (__p >= _srodata) && (__p < _erodata); \

Obviously the above calls for adjustments here, too.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH RFC 02/25] A collection of fixes to Xen common files [ In reply to ]

George.Dunlap at eu

Dec 7, 2011, 3:14 AM

Post #3 of 11 (296 views)

On Wed, Dec 7, 2011 at 9:00 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 06.12.11 at 19:19, <stefano.stabellini@eu.citrix.com> wrote:
>> - call free_xenoprof_pages only ifdef xenoprof;
>
> I can't find where this symbol would get defined, so effectively you're
> removing the call unconditionally.

And when we do define it, convention says it should be in capitals,
not lower-case.

-George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH RFC 02/25] A collection of fixes to Xen common files [ In reply to ]

stefano.stabellini at eu

Dec 7, 2011, 3:20 AM

Post #4 of 11 (296 views)

On Wed, 7 Dec 2011, Jan Beulich wrote:
> >>> On 06.12.11 at 19:19, <stefano.stabellini@eu.citrix.com> wrote:
> > - call free_xenoprof_pages only ifdef xenoprof;
>
> I can't find where this symbol would get defined, so effectively you're
> removing the call unconditionally.

The symbol is already defined in xen/arch/x86/Rules.mk and
xen/arch/ia64/Rules.mk.

> > +extern char _srodata[], _erodata[];
>
> While I realize that this isn't currently the case on e.g. the respective
> .text symbols, if you introduce new once, please add const to the
> declarations *at least* when the section contents really are read only
> (in many cases doing so even for writable sections is likely appropriate
> too).

OK

> > +#define is_kernel_rodata(p) ({ \
> > + char *__p = (char *)(unsigned long)(p); \
> > + (__p >= _srodata) && (__p < _erodata); \
>
> Obviously the above calls for adjustments here, too.

sorry I am not sure what you mean

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH RFC 02/25] A collection of fixes to Xen common files [ In reply to ]

Ian.Campbell at citrix

Dec 7, 2011, 3:38 AM

Post #5 of 11 (295 views)

On Wed, 2011-12-07 at 11:20 +0000, Stefano Stabellini wrote:
> On Wed, 7 Dec 2011, Jan Beulich wrote:
> > >>> On 06.12.11 at 19:19, <stefano.stabellini@eu.citrix.com> wrote:
> > > - call free_xenoprof_pages only ifdef xenoprof;
> >
> > I can't find where this symbol would get defined, so effectively you're
> > removing the call unconditionally.
>
> The symbol is already defined in xen/arch/x86/Rules.mk and
> xen/arch/ia64/Rules.mk.

That's a Makefile variable -- it's not exposed to the C preprocessor, is
it?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH RFC 02/25] A collection of fixes to Xen common files [ In reply to ]

stefano.stabellini at eu

Dec 7, 2011, 4:39 AM

Post #6 of 11 (296 views)

On Wed, 7 Dec 2011, Ian Campbell wrote:
> On Wed, 2011-12-07 at 11:20 +0000, Stefano Stabellini wrote:
> > On Wed, 7 Dec 2011, Jan Beulich wrote:
> > > >>> On 06.12.11 at 19:19, <stefano.stabellini@eu.citrix.com> wrote:
> > > > - call free_xenoprof_pages only ifdef xenoprof;
> > >
> > > I can't find where this symbol would get defined, so effectively you're
> > > removing the call unconditionally.
> >
> > The symbol is already defined in xen/arch/x86/Rules.mk and
> > xen/arch/ia64/Rules.mk.
>
> That's a Makefile variable -- it's not exposed to the C preprocessor, is
> it?

Now I realize that I have made this mistake a few times: in this
patch and in patch #7.

I am going to add the missing symbols to config.h.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH RFC 02/25] A collection of fixes to Xen common files [ In reply to ]

Dec 7, 2011, 4:43 AM

Post #7 of 11 (299 views)

At 09:00 +0000 on 07 Dec (1323248401), Jan Beulich wrote:
> >>> On 06.12.11 at 19:19, <stefano.stabellini@eu.citrix.com> wrote:
> > - call free_xenoprof_pages only ifdef xenoprof;
>
> I can't find where this symbol would get defined, so effectively you're
> removing the call unconditionally.
>
> > +extern char _srodata[], _erodata[];
>
> While I realize that this isn't currently the case on e.g. the respective
> .text symbols, if you introduce new once, please add const to the
> declarations *at least* when the section contents really are read only
> (in many cases doing so even for writable sections is likely appropriate
> too).

Is this just a question of style or is there any practical consequence?
(not that I object to adding 'const' here; I'm just curious)

Tim.

> > +#define is_kernel_rodata(p) ({ \
> > + char *__p = (char *)(unsigned long)(p); \
> > + (__p >= _srodata) && (__p < _erodata); \
>
> Obviously the above calls for adjustments here, too.
>
> Jan
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH RFC 02/25] A collection of fixes to Xen common files [ In reply to ]

JBeulich at suse

Dec 7, 2011, 4:47 AM

Post #8 of 11 (294 views)

>>> On 07.12.11 at 12:20, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
wrote:
> On Wed, 7 Dec 2011, Jan Beulich wrote:
>> >>> On 06.12.11 at 19:19, <stefano.stabellini@eu.citrix.com> wrote:
>> > +extern char _srodata[], _erodata[];
>>
>> While I realize that this isn't currently the case on e.g. the respective
>> .text symbols, if you introduce new once, please add const to the
>> declarations *at least* when the section contents really are read only
>> (in many cases doing so even for writable sections is likely appropriate
>> too).
>
> OK
>
>
>> > +#define is_kernel_rodata(p) ({ \
>> > + char *__p = (char *)(unsigned long)(p); \
>> > + (__p >= _srodata) && (__p < _erodata); \
>>
>> Obviously the above calls for adjustments here, too.
>
> sorry I am not sure what you mean

Should be 'const char *' at least in the declaration of 'p' (but perhaps
also in the cast).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH RFC 02/25] A collection of fixes to Xen common files [ In reply to ]

JBeulich at suse

Dec 7, 2011, 4:49 AM

Post #9 of 11 (298 views)

>>> On 07.12.11 at 13:43, Tim Deegan <tim@xen.org> wrote:
> At 09:00 +0000 on 07 Dec (1323248401), Jan Beulich wrote:
>> >>> On 06.12.11 at 19:19, <stefano.stabellini@eu.citrix.com> wrote:
>> > - call free_xenoprof_pages only ifdef xenoprof;
>>
>> I can't find where this symbol would get defined, so effectively you're
>> removing the call unconditionally.
>>
>> > +extern char _srodata[], _erodata[];
>>
>> While I realize that this isn't currently the case on e.g. the respective
>> .text symbols, if you introduce new once, please add const to the
>> declarations *at least* when the section contents really are read only
>> (in many cases doing so even for writable sections is likely appropriate
>> too).
>
> Is this just a question of style or is there any practical consequence?
> (not that I object to adding 'const' here; I'm just curious)

Pointers to non-modifiable data should never lack const, to avoid them
accidentally (perhaps through several layers of functions) being passed
to something that does actually write through them. (This is on the
same page as avoiding casts in general when possible, but specifically
such that cast away const-ness.)

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH RFC 02/25] A collection of fixes to Xen common files [ In reply to ]

david.vrabel at citrix

Dec 7, 2011, 4:53 AM

Post #10 of 11 (297 views)

On 06/12/11 18:19, stefano.stabellini@eu.citrix.com wrote:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> - call free_xenoprof_pages only ifdef xenoprof;

I think you want empty inline stub functions on ARM instead of an #ifdef
here.

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH RFC 02/25] A collection of fixes to Xen common files [ In reply to ]

stefano.stabellini at eu

Dec 7, 2011, 4:58 AM

Post #11 of 11 (296 views)

On Wed, 7 Dec 2011, David Vrabel wrote:
> On 06/12/11 18:19, stefano.stabellini@eu.citrix.com wrote:
> > From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >
> > - call free_xenoprof_pages only ifdef xenoprof;
>
> I think you want empty inline stub functions on ARM instead of an #ifdef
> here.

Generally I would prefer stub functions, but in this case, considering
that a makefile symbol already exists to avoid compiling xenoprof, I
think it is better to make sure xenoprof can actually be compiled out
successfully.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel