Re: [Xen-devel] [PATCH] xen: 'keyhandler' is not used in null scheduler
Hello George, On 5/30/19 17:05, George Dunlap wrote: On May 30, 2019, at 6:47 AM, Baodong Chen wrote: So remove 'keyhandler.h' include. Also add 'static' prefix for 'schud_bull_def' Signed-off-by: Baodong Chen Thanks for the patch — these changes look good. I think the title would be better something like: xen/sched_null: Superficial clean-ups Then just list both in bullet points; something like: * Remove unused dependency ‘keyhandler.’h * Make sched_null_def static Would you mind re-sending the patch? You can add: Reviewed-by: George Dunlap Thanks for your review, resent. Thanks, -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: notifier: refine 'notifier_head', use 'list_head' directly
On 5/31/19 18:22, Julien Grall wrote:
Hi,
Missing commit message here.
Yes, will be added.
On 5/31/19 3:35 AM, Baodong Chen wrote:
Signed-off-by: Baodong Chen
---
xen/common/notifier.c | 25 ++---
xen/include/xen/notifier.h | 21 +++--
2 files changed, 21 insertions(+), 25 deletions(-)
diff --git a/xen/common/notifier.c b/xen/common/notifier.c
index 34488a8..f959a79 100644
--- a/xen/common/notifier.c
+++ b/xen/common/notifier.c
@@ -21,10 +21,10 @@
void __init notifier_chain_register(
struct notifier_head *nh, struct notifier_block *n)
{
- struct list_head *chain = &nh->head.chain;
+ struct list_head *chain = &nh->head;
struct notifier_block *nb;
- while ( chain->next != &nh->head.chain )
+ while ( chain->next != &nh->head )
{
nb = list_entry(chain->next, struct notifier_block, chain);
if ( n->priority > nb->priority )
@@ -35,19 +35,6 @@ void __init notifier_chain_register(
list_add(&n->chain, chain);
}
-/**
- * notifier_chain_unregister - Remove notifier from a raw notifier
chain
- * @nh: Pointer to head of the raw notifier chain
- * @n: Entry to remove from notifier chain
- *
- * Removes a notifier from a raw notifier chain.
- * All locking must be provided by the caller.
- */
-void __init notifier_chain_unregister(
- struct notifier_head *nh, struct notifier_block *n)
-{
- list_del(&n->chain);
-}
Why did you move the function?
My fault, should NOT touch this at all.
Patch will be resent.
Cheers,
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: put cpupool's member 'n_dom' after 'cpupool_id'
On 5/31/19 18:52, Julien Grall wrote:
Hi,
On 5/31/19 4:18 AM, Baodong Chen wrote:
Thus, sizeof(struct cpupool) will save 8 bytes for 64-bit system.
I am happy with the change, although AFAIK cpupool is not instantiated
that often. Are you planning to have more instantiation of it?
Cheers,
No, I'm not planning to create lots of cpupool instance.
I'm studying xen for a few weeks and my plan is:
run xen for embeded automotive use case with dom0less, null scheduler,
small code base for safety certified maybe a plus(not sure whether can
do this).
Signed-off-by: Baodong Chen
---
xen/include/xen/sched-if.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h
index 92bc7a0..f0cf210 100644
--- a/xen/include/xen/sched-if.h
+++ b/xen/include/xen/sched-if.h
@@ -213,9 +213,9 @@ static inline void sched_free_domdata(const
struct scheduler *s,
struct cpupool
{
int cpupool_id;
+ unsigned int n_dom;
cpumask_var_t cpu_valid; /* all cpus assigned to pool */
struct cpupool *next;
- unsigned int n_dom;
struct scheduler *sched;
atomic_t refcnt;
};
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: cpu: change 'cpu_hotplug_[begin|done]' to inline function
On 5/31/19 18:55, Julien Grall wrote:
Hi,
On 5/31/19 3:46 AM, Baodong Chen wrote:
Signed-off-by: Baodong Chen
---
xen/common/cpu.c | 10 --
xen/include/xen/cpu.h | 4 ++--
2 files changed, 2 insertions(+), 12 deletions(-)
diff --git a/xen/common/cpu.c b/xen/common/cpu.c
index f388d89..a526b55 100644
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -51,16 +51,6 @@ void put_cpu_maps(void)
spin_unlock_recursive(&cpu_add_remove_lock);
}
-bool_t cpu_hotplug_begin(void)
-{
- return get_cpu_maps();
-}
-
-void cpu_hotplug_done(void)
-{
- put_cpu_maps();
-}
-
static NOTIFIER_HEAD(cpu_chain);
void __init register_cpu_notifier(struct notifier_block *nb)
diff --git a/xen/include/xen/cpu.h b/xen/include/xen/cpu.h
index 4638c50..70a2df4 100644
--- a/xen/include/xen/cpu.h
+++ b/xen/include/xen/cpu.h
@@ -10,8 +10,8 @@ bool_t get_cpu_maps(void);
void put_cpu_maps(void);
/* Safely perform CPU hotplug and update cpu_online_map, etc. */
-bool_t cpu_hotplug_begin(void);
-void cpu_hotplug_done(void);
+static inline bool_t cpu_hotplug_begin(void) { return get_cpu_maps(); }
+static inline void cpu_hotplug_done(void) { put_cpu_maps(); }
The coding style should be:
static inline
{
...
}
Yes, clang-format automated format code for me, will be fixed.
/* Receive notification of CPU hotplug events. */
void register_cpu_notifier(struct notifier_block *nb);
Cheers,
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: cpu: change 'cpu_hotplug_[begin|done]' to inline function
On 5/31/19 19:30, Jan Beulich wrote:
On 31.05.19 at 12:55, wrote:
On 5/31/19 3:46 AM, Baodong Chen wrote:
--- a/xen/include/xen/cpu.h
+++ b/xen/include/xen/cpu.h
@@ -10,8 +10,8 @@ bool_t get_cpu_maps(void);
void put_cpu_maps(void);
/* Safely perform CPU hotplug and update cpu_online_map, etc. */
-bool_t cpu_hotplug_begin(void);
-void cpu_hotplug_done(void);
+static inline bool_t cpu_hotplug_begin(void) { return get_cpu_maps(); }
+static inline void cpu_hotplug_done(void) { put_cpu_maps(); }
Plus please switch to bool at the same time.
Yes, types like boot_t or u32 are getting rid of, so should Not be used.
Will be fixed.
Jan
.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: make tracebuffer configurable
On 5/31/19 19:10, Jan Beulich wrote:
On 30.05.19 at 12:17, wrote:
Default: enabled.
Can be disabled for smaller code footprint.
But you're aware that we're, for now at least, trying to limit the
number of independently selectable config options? Ones depending
on EXPERT are sort of an exception in certain cases.
Limit the number of independently selectable config sounds good to me.
Does the following looks good?
+config HAS_TRACEBUFFER
+ bool "Enable/Disable tracebuffer" if EXPERT = "y"
+ ---help---
+ Enable or disable tracebuffer function.
+ Xen internal running status(trace event) will be saved to
trace memory
+ when enabled.
+
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -368,4 +368,10 @@ config DOM0_MEM
Leave empty if you are not sure what to specify.
+config HAS_TRACEBUFFER
+ bool "Enable/Disable tracebuffer"
+ default y
+ ---help---
+ Enable or disable tracebuffer function.
HAS_* options should not come with a prompt, and should
only be "select"-ed by (normally) per-architecture Kconfig
files. If we are to permit having this option, then I also think
the help text needs expanding.
Tanks for your clarification.
--- a/xen/include/xen/trace.h
+++ b/xen/include/xen/trace.h
@@ -21,12 +21,14 @@
#ifndef __XEN_TRACE_H__
#define __XEN_TRACE_H__
-extern int tb_init_done;
#include
#include
#include
+#ifdef CONFIG_HAS_TRACEBUFFER
+
+extern int tb_init_done;
/* Used to initialise trace buffer functionality */
void init_trace_bufs(void);
I wonder if there hadn't been a reason for the declaration to live
up where it was. Also please separate independent blocks of code
by a blank line.
Roger that.
@@ -47,6 +49,20 @@ static inline void trace_var(u32 event, int cycles, int
extra,
void __trace_hypercall(uint32_t event, unsigned long op,
const xen_ulong_t *args);
+#else
+#define tb_init_done (0)
Perhaps better "false", and no real need for the parentheses afaict.
+static inline void init_trace_bufs(void) {}
+static inline int tb_control(struct xen_sysctl_tbuf_op *tbc) { return -ENOSYS;
}
+
+static inline int trace_will_trace_event(u32 event) { return 0; }
+static inline void trace_var(u32 event, int cycles, int extra,
+ const void *extra_data) {}
+static inline void __trace_var(u32 event, bool_t cycles, unsigned int extra,
+ const void *extra_data) {}
+static inline void __trace_hypercall(uint32_t event, unsigned long op,
+ const xen_ulong_t *args) {}
+#endif
We try to do away with u32 and friends, so please don't introduce
new uses - even less so when in one case here you already use
uint32_t. Similarly please use "bool" in favor of "bool_t".
Roger that.
Jan
.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: make tracebuffer configurable
On 5/31/19 20:58, George Dunlap wrote: On May 31, 2019, at 12:10 PM, Jan Beulich wrote: On 30.05.19 at 12:17, wrote: Default: enabled. Can be disabled for smaller code footprint. But you're aware that we're, for now at least, trying to limit the number of independently selectable config options? Ones depending on EXPERT are sort of an exception in certain cases. I’m trying to remember exactly what we have or haven’t decided. I take it you think we should avoid having a load of independently-selectable configurations to support? Baodong, what was your main purpose in adding a patch like this? Just to make things a bit tidier, or was it to try to go through and generate a far smaller hypervisor codebase (for instance, perhaps to make safety certification more tractable)? Hello George, yes yes, smaller code base for safety certification is under my thought. plan to run xen for automotive use case on arm (perhaps i.MX8 ) devices. I think we’ve talked about this before, but our basic options, as far as support, would be: 1. Have a single large config option which disabled large swathes of unused functionality 2. Have individual bits configurable, but have only a handful of “security supported” configurations. The idea with #2 is that we’d have a “certification” config that we tested and security supported, with all of these individual bits off, as well as “cloud” and “client” configs with all of these “optional” bits on (or some subset on, depending on what each community thought made the most sense for their use cafe). If people wanted to enable or disable individual config options outside fo those, they’d be taking a risk wrt breakage (not tested) or security issues (no XSA issued unless it affected one of the supported configs). I like the idea about 'certification' config. Rich / Daniel, am I on the right track here? Any thoughts? -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: make keyhanler configurable
On 5/31/19 18:39, Dario Faggioli wrote:
On Fri, 2019-05-31 at 09:58 +0800, Baodong Chen wrote:
keyhandler mainly used for debug usage by developers which can dump
xen module(eg. timer, scheduler) status at runtime by input
character from console.
Signed-off-by: Baodong Chen
---
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -368,4 +368,13 @@ config DOM0_MEM
Leave empty if you are not sure what to specify.
+config HAS_KEYHANDLER
+ bool "Enable/Disable keyhandler"
+ default y
+ ---help---
+ Enable or disable keyhandler function.
+ keyhandler mainly used for debug usage by developers which
can dump
+ xen module(eg. timer, scheduler) status at runtime by input
character
+ from console.
+
endmenu
I personally like the idea.
I've probably would have called the option CONFIG_KEYHANDLERS, even if
I can see that we have quite a few CONFIG_HAS_*.
But it's not for me to ask/decide, and I don't have a too strong
opinion on this anyway, so let's hear what others think.
I'd at least add the 'S', though (as in CONFIG_HAS_KEYHANDLERS).
Yes, can be fixed.
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -699,6 +699,7 @@ int cpupool_do_sysctl(struct
xen_sysctl_cpupool_op *op)
return ret;
}
+#ifdef CONFIG_HAS_KEYHANDLER
void dump_runq(unsigned char key)
{
unsigned longflags;
@@ -730,6 +731,7 @@ void dump_runq(unsigned char key)
local_irq_restore(flags);
spin_unlock(&cpupool_lock);
}
+#endif
static int cpu_callback(
struct notifier_block *nfb, unsigned long action, void *hcpu)
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1913,6 +1913,7 @@ void scheduler_free(struct scheduler *sched)
xfree(sched);
}
+#ifdef CONFIG_HAS_KEYHANDLER
void schedule_dump(struct cpupool *c)
{
unsigned int i;
@@ -1941,6 +1942,7 @@ void schedule_dump(struct cpupool *c)
SCHED_OP(sched, dump_cpu_state, i);
}
}
+#endif
void sched_tick_suspend(void)
{
Mmm... a lot of #ifdef-fery spread around quite a bit.. I have to admit
I don't especially like that.
Me too, can leave it as what is was.
but since schedule_dump prototype have external linkage.
so even no one will call it, it maybe still in output executable file,
right?
--- a/xen/include/xen/keyhandler.h
+++ b/xen/include/xen/keyhandler.h
@@ -48,4 +49,17 @@ void register_irq_keyhandler(unsigned char key,
/* Inject a keypress into the key-handling subsystem. */
extern void handle_keypress(unsigned char key, struct cpu_user_regs
*regs);
+#else
+static inline void initialize_keytable(void) {}
+static inline void register_keyhandler(unsigned char key,
keyhandler_fn_t *fn,
+ const char *desc, bool_t
diagnostic) {}
+static inline void register_irq_keyhandler(unsigned char key,
+ irq_keyhandler_fn_t *fn,
+ const char *desc,
+ bool_t diagnostic) {}
+
+static inline void handle_keypress(unsigned char key,
+ struct cpu_user_regs *regs) {}
So, with this last change, we have:
static DECLARE_TASKLET(keypress_tasklet, keypress_action, 0);
But since all keypress_action() does is calling handle_keypress(),
which is becoming a nop... can't we kill the tasklet alltogether?
the whole keyhandler.c will not compiled when config disabled.
am i misunderstood something?
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -171,8 +171,10 @@ extern unsigned int tainted;
extern char *print_tainted(char *str);
extern void add_taint(unsigned int taint);
+#ifdef CONFIG_HAS_KEYHANDLER
struct cpu_user_regs;
void dump_execstate(struct cpu_user_regs *);
+#endif
Yes. Or, you provide an empty stub of dump_execstate(), if
CONFIG_HAS_KEYHANDLER is not defined, which means we don't have to mess
with #ifdef-s at the caller site(s).
Of course, I'm not maintainer of this specific piece of code, but I'd
prefer this stub-based approach to be used in general ... ...
Agree, can be fixed.
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -994,8 +994,10 @@ int cpupool_add_domain(struct domain *d, int
poolid);
void cpupool_rm_domain(struct domain *d);
int cpupool_move_domain(struct domain *d, struct cpupool *c);
int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
+#ifdef CONFIG_HAS_KEYHANDLER
void schedule_dump(struct cpupool *c);
extern void dump_runq(unsigned char key);
+#endif
void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
... ... ... Like, for instance, in here.
But again, sine these changes are spread around many files, let's see
what others prefer, and use the same strategy everywhere.
Regards
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listin
Re: [Xen-devel] [PATCH] xen: make keyhanler configurable
On 5/31/19 18:53, Juergen Gross wrote:
On 31/05/2019 03:58, Baodong Chen wrote:
keyhandler mainly used for debug usage by developers which can dump
xen module(eg. timer, scheduler) status at runtime by input
character from console.
Signed-off-by: Baodong Chen
---
xen/arch/arm/gic.c | 2 ++
xen/arch/x86/apic.c | 2 ++
xen/common/Kconfig | 9 +
xen/common/Makefile | 2 +-
xen/common/cpupool.c | 2 ++
xen/common/schedule.c| 2 ++
xen/include/xen/keyhandler.h | 14 ++
xen/include/xen/lib.h| 2 ++
xen/include/xen/sched.h | 2 ++
9 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 6cc7dec..fff88c5 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -361,7 +361,9 @@ static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi
sgi)
/* Nothing to do, will check for events on return path */
break;
case GIC_SGI_DUMP_STATE:
+#ifdef CONFIG_HAS_KEYHANDLER
dump_execstate(regs);
+#endif
break;
case GIC_SGI_CALL_FUNCTION:
smp_call_function_interrupt();
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index fafc0bd..e5f004a 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1410,7 +1410,9 @@ void spurious_interrupt(struct cpu_user_regs *regs)
ack_APIC_irq();
if (this_cpu(state_dump_pending)) {
this_cpu(state_dump_pending) = false;
+#ifdef CONFIG_HAS_KEYHANDLER
dump_execstate(regs);
+#endif
return;
}
}
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index c838506..450541c 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -368,4 +368,13 @@ config DOM0_MEM
Leave empty if you are not sure what to specify.
+config HAS_KEYHANDLER
AFAIK the HAS_* config options are meant to be selected by other options
and not be user selectable.
So I think you should drop the "HAS_" and maybe use the plural as Dario
already suggested ("KEYHANDLERS").
Yes.
+ bool "Enable/Disable keyhandler"
+ default y
+ ---help---
+ Enable or disable keyhandler function.
+ keyhandler mainly used for debug usage by developers which can dump
+ xen module(eg. timer, scheduler) status at runtime by input character
+ from console.
I'd drop the "by developers". In case of customer problems with Xen
hosts the output of keyhandlers is requested on a rather regular base.
Agree, can be fixed.
+
endmenu
diff --git a/xen/common/Makefile b/xen/common/Makefile
index bca48e6..c7bcd26 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -16,7 +16,7 @@ obj-y += guestcopy.o
obj-bin-y += gunzip.init.o
obj-y += irq.o
obj-y += kernel.o
-obj-y += keyhandler.o
+obj-$(CONFIG_HAS_KEYHANDLER) += keyhandler.o
obj-$(CONFIG_KEXEC) += kexec.o
obj-$(CONFIG_KEXEC) += kimage.o
obj-y += lib.o
diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index 31ac323..721a729 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -699,6 +699,7 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
return ret;
}
+#ifdef CONFIG_HAS_KEYHANDLER
void dump_runq(unsigned char key)
{
unsigned longflags;
@@ -730,6 +731,7 @@ void dump_runq(unsigned char key)
local_irq_restore(flags);
spin_unlock(&cpupool_lock);
}
+#endif
static int cpu_callback(
struct notifier_block *nfb, unsigned long action, void *hcpu)
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 66f1e26..617c444 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1913,6 +1913,7 @@ void scheduler_free(struct scheduler *sched)
xfree(sched);
}
+#ifdef CONFIG_HAS_KEYHANDLER
void schedule_dump(struct cpupool *c)
{
unsigned int i;
@@ -1941,6 +1942,7 @@ void schedule_dump(struct cpupool *c)
SCHED_OP(sched, dump_cpu_state, i);
}
}
+#endif
void sched_tick_suspend(void)
{
diff --git a/xen/include/xen/keyhandler.h b/xen/include/xen/keyhandler.h
index 5131e86..1050b80 100644
--- a/xen/include/xen/keyhandler.h
+++ b/xen/include/xen/keyhandler.h
@@ -28,6 +28,7 @@ struct cpu_user_regs;
typedef void (irq_keyhandler_fn_t)(unsigned char key,
struct cpu_user_regs *regs);
+#ifdef CONFIG_HAS_KEYHANDLER
/* Initialize keytable with default handlers. */
void initialize_keytable(void);
@@ -48,4 +49,17 @@ void register_irq_keyhandler(unsigned char key,
/* Inject a keypress into the key-handling subsystem. */
extern void handle_keypress(unsigned char key, struct cpu_user_regs *regs);
+#else
+static inline void initialize_keytable(void) {}
+static inline void register_keyhandler(unsigned char key, keyhandler_fn_t *fn,
+ const char *desc, bool_t diagnostic) {}
+static inline void register_irq_keyhan
Re: [Xen-devel] [PATCH] xen: make keyhanler configurable
On 6/1/19 06:30, Andrew Cooper wrote: On 30/05/2019 18:58, Baodong Chen wrote: keyhandler mainly used for debug usage by developers which can dump xen module(eg. timer, scheduler) status at runtime by input character from console. Signed-off-by: Baodong Chen What is the motivation here? I don't have a specific objection to making this configurable (so long as it excises the entire keyhandler infrastructure, which is rather more than this patch does), but I also don't see why we would want to do so in the first place. In particular, it doesn't require a serial console to function correctly in the first place. This functionality can be used with `xl debug-keys $char; xl dmesg` IIUC the config cut nearly the entire keyhandler infrastructure. I'm interested in arm64 automotive use case, safety certification is nice to have. so the smaller code base is preferred. BTW, i heard the works "dom0less", is it possible run vms over xen without xl? ~Andrew . ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RESEND] xen: notifier: refine 'notifier_head', use 'list_head' directly
On 6/3/19 17:28, Jan Beulich wrote: On 03.06.19 at 03:33, wrote: 'notifier_block' can be replaced with 'list_head' when used for 'notifier_head', this make the a little more clear. Signed-off-by: Baodong Chen Oh, and also a remark regarding the title: Why "RESEND"? This should be used only if you re-send an entirely unchanged patch, perhaps because of a correction to the recipients list. Otherwise please increment the version number. Hello Jan, Thanks for guiding me to the right direction. This is my first experience sending patch using mail list. I will use version number instead of resend next time. Jan . ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RESEND] xen: notifier: refine 'notifier_head', use 'list_head' directly
On 6/3/19 17:27, Jan Beulich wrote:
On 03.06.19 at 03:33, wrote:
'notifier_block' can be replaced with 'list_head' when used for
'notifier_head', this make the a little more clear.
I guess you mean "... makes the code a little ..."?
Yes, fixed, see v1.
@@ -71,16 +71,16 @@ int notifier_call_chain(
{
int ret = NOTIFY_DONE;
struct list_head *cursor;
-struct notifier_block *nb;
+struct notifier_block *nb = NULL;
bool_t reverse = !!(val & NOTIFY_REVERSE);
-cursor = &(pcursor && *pcursor ? *pcursor : &nh->head)->chain;
+cursor = (pcursor && *pcursor ? &(*pcursor)->chain : &nh->head);
The outermost parentheses are now not really needed anymore.
Yes, fixed, see v1.
--- a/xen/include/xen/notifier.h
+++ b/xen/include/xen/notifier.h
@@ -29,13 +29,12 @@ struct notifier_block {
};
struct notifier_head {
-struct notifier_block head;
+struct list_head head;
};
-#define NOTIFIER_INIT(name) { .head.chain = LIST_HEAD_INIT(name.head.chain) }
Note the blanks immediately inside the figure braces - ...
Yes, fixed, see v1.
+#define NOTIFIER_HEAD(name)
\
+ struct notifier_head name = {.head = LIST_HEAD_INIT(name.head)}
... please don't break such style aspects, unless you know
it is something that needs fixing (for being in violation of our
style guidelines).
Yes, fixed, see v1.
Jan
.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: put cpupool's member 'n_dom' after 'cpupool_id'
On 6/3/19 17:30, Julien Grall wrote:
Hi,
On 03/06/2019 02:48, chenbaodong wrote:
On 5/31/19 18:52, Julien Grall wrote:
Hi,
On 5/31/19 4:18 AM, Baodong Chen wrote:
Thus, sizeof(struct cpupool) will save 8 bytes for 64-bit system.
I am happy with the change, although AFAIK cpupool is not
instantiated that often. Are you planning to have more instantiation
of it?
Cheers,
No, I'm not planning to create lots of cpupool instance.
I'm studying xen for a few weeks and my plan is:
run xen for embeded automotive use case with dom0less, null
scheduler, small code base for safety certified maybe a plus(not sure
whether can do this).
FWIW, there are discussion to get Xen safety certified. They are
captured on [1].
Cheers,
[1]
https://wiki.xenproject.org/wiki/Category:Safety_Certification/FuSa_SIG
Hello Julien, Thanks for your info.
I will follow the wiki link and see what i can do about safety
certification.
Signed-off-by: Baodong Chen
---
xen/include/xen/sched-if.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h
index 92bc7a0..f0cf210 100644
--- a/xen/include/xen/sched-if.h
+++ b/xen/include/xen/sched-if.h
@@ -213,9 +213,9 @@ static inline void sched_free_domdata(const
struct scheduler *s,
struct cpupool
{
int cpupool_id;
+ unsigned int n_dom;
cpumask_var_t cpu_valid; /* all cpus assigned to pool */
struct cpupool *next;
- unsigned int n_dom;
struct scheduler *sched;
atomic_t refcnt;
};
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: cpu: change 'cpu_hotplug_[begin|done]' to inline function
On 6/3/19 17:37, Julien Grall wrote:
Hi,
On 03/06/2019 02:52, chenbaodong wrote:
On 5/31/19 18:55, Julien Grall wrote:
Hi,
On 5/31/19 3:46 AM, Baodong Chen wrote:
Signed-off-by: Baodong Chen
---
xen/common/cpu.c | 10 --
xen/include/xen/cpu.h | 4 ++--
2 files changed, 2 insertions(+), 12 deletions(-)
diff --git a/xen/common/cpu.c b/xen/common/cpu.c
index f388d89..a526b55 100644
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -51,16 +51,6 @@ void put_cpu_maps(void)
spin_unlock_recursive(&cpu_add_remove_lock);
}
-bool_t cpu_hotplug_begin(void)
-{
- return get_cpu_maps();
-}
-
-void cpu_hotplug_done(void)
-{
- put_cpu_maps();
-}
-
static NOTIFIER_HEAD(cpu_chain);
void __init register_cpu_notifier(struct notifier_block *nb)
diff --git a/xen/include/xen/cpu.h b/xen/include/xen/cpu.h
index 4638c50..70a2df4 100644
--- a/xen/include/xen/cpu.h
+++ b/xen/include/xen/cpu.h
@@ -10,8 +10,8 @@ bool_t get_cpu_maps(void);
void put_cpu_maps(void);
/* Safely perform CPU hotplug and update cpu_online_map, etc. */
-bool_t cpu_hotplug_begin(void);
-void cpu_hotplug_done(void);
+static inline bool_t cpu_hotplug_begin(void) { return
get_cpu_maps(); }
+static inline void cpu_hotplug_done(void) { put_cpu_maps(); }
The coding style should be:
static inline
{
...
}
Yes, clang-format automated format code for me, will be fixed.
Hmmm, clang-format does not have Xen coding style support yet. Do you
have patches on top to handle it?
No, But the linux kernel seems already have it's clang-format support.
Guess can used by xen.
IMO i don't like the coding style in xen personally.
But it's code base has long years history. can insist on this or make
some changes.
I prefer clang-format personally, because no style issue in patch and
will make review easier.
Cheers,
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: make tracebuffer configurable
On 6/3/19 16:31, Jan Beulich wrote: On 03.06.19 at 05:07, wrote: On 5/31/19 19:10, Jan Beulich wrote: On 30.05.19 at 12:17, wrote: Default: enabled. Can be disabled for smaller code footprint. But you're aware that we're, for now at least, trying to limit the number of independently selectable config options? Ones depending on EXPERT are sort of an exception in certain cases. Limit the number of independently selectable config sounds good to me. Does the following looks good? +config HAS_TRACEBUFFER + bool "Enable/Disable tracebuffer" if EXPERT = "y" + ---help--- + Enable or disable tracebuffer function. + Xen internal running status(trace event) will be saved to trace memory + when enabled. + The EXPERT addition make introducing this fine by me. But its name is still wrong, and the help text also needs further improvement imo. Hi Jan, thanks for your kindly review and feedback. For this, would you please give your suggestions about the name and help text? Jan . ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: cpu: change 'cpu_hotplug_[begin|done]' to inline function
On 6/3/19 18:40, Julien Grall wrote:
Hi,
On 03/06/2019 11:22, chenbaodong wrote:
On 6/3/19 17:37, Julien Grall wrote:
Hi,
On 03/06/2019 02:52, chenbaodong wrote:
On 5/31/19 18:55, Julien Grall wrote:
Hi,
On 5/31/19 3:46 AM, Baodong Chen wrote:
Signed-off-by: Baodong Chen
---
xen/common/cpu.c | 10 --
xen/include/xen/cpu.h | 4 ++--
2 files changed, 2 insertions(+), 12 deletions(-)
diff --git a/xen/common/cpu.c b/xen/common/cpu.c
index f388d89..a526b55 100644
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -51,16 +51,6 @@ void put_cpu_maps(void)
spin_unlock_recursive(&cpu_add_remove_lock);
}
-bool_t cpu_hotplug_begin(void)
-{
- return get_cpu_maps();
-}
-
-void cpu_hotplug_done(void)
-{
- put_cpu_maps();
-}
-
static NOTIFIER_HEAD(cpu_chain);
void __init register_cpu_notifier(struct notifier_block *nb)
diff --git a/xen/include/xen/cpu.h b/xen/include/xen/cpu.h
index 4638c50..70a2df4 100644
--- a/xen/include/xen/cpu.h
+++ b/xen/include/xen/cpu.h
@@ -10,8 +10,8 @@ bool_t get_cpu_maps(void);
void put_cpu_maps(void);
/* Safely perform CPU hotplug and update cpu_online_map, etc. */
-bool_t cpu_hotplug_begin(void);
-void cpu_hotplug_done(void);
+static inline bool_t cpu_hotplug_begin(void) { return
get_cpu_maps(); }
+static inline void cpu_hotplug_done(void) { put_cpu_maps(); }
The coding style should be:
static inline
{
...
}
Yes, clang-format automated format code for me, will be fixed.
Hmmm, clang-format does not have Xen coding style support yet. Do
you have patches on top to handle it?
No, But the linux kernel seems already have it's clang-format
support. Guess can used by xen.
Most of Xen code base does not use Linux coding style. The only
exception is file imported directly from Linux to ease porting bug fixes.
Roger that.
IMO i don't like the coding style in xen personally.
I don't necessarily agree with all of it but some of the Linux style
are odd too.
yes yes, Linux style, eg: 'tab', i also dislike.
But it's code base has long years history. can insist on this or make
some changes.
Improvement to the coding style are always welcomed. Although, you
will notice that anything around coding style (in any project) is a
matter of taste and can be difficult to find an agreement.
Agree.
I prefer clang-format personally, because no style issue in patch
and will make review easier.
clang-format is not a coding style. It is a tool helping to format in
a specific coding style. There are effort to extend clang-format for
supporting Xen coding style.
Agree, For xen, it's worth to have it's '.clang-format' file according
to it's style.
Cheers,
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: make tracebuffer configurable
On 6/3/19 22:31, Jan Beulich wrote: On 03.06.19 at 16:08, wrote: On Jun 3, 2019, at 11:54 AM, Jan Beulich wrote: On 03.06.19 at 12:41, wrote: On 6/3/19 16:31, Jan Beulich wrote: On 03.06.19 at 05:07, wrote: On 5/31/19 19:10, Jan Beulich wrote: On 30.05.19 at 12:17, wrote: Default: enabled. Can be disabled for smaller code footprint. But you're aware that we're, for now at least, trying to limit the number of independently selectable config options? Ones depending on EXPERT are sort of an exception in certain cases. Limit the number of independently selectable config sounds good to me. Does the following looks good? +config HAS_TRACEBUFFER + bool "Enable/Disable tracebuffer" if EXPERT = "y" + ---help--- + Enable or disable tracebuffer function. + Xen internal running status(trace event) will be saved to trace memory + when enabled. + The EXPERT addition make introducing this fine by me. But its name is still wrong, and the help text also needs further improvement imo. Hi Jan, thanks for your kindly review and feedback. For this, would you please give your suggestions about the name and help text? As far as the name is concerned, the HAS_ should be dropped. I'm afraid I don't have a particular suggestion for the help text. You could at least give an idea what you think the help text should include, or some kind of guidance as to what would satisfy you. Obviously you shouldn’t be required to write everybody’s help text for them; but by the same token, everybody shouldn’t be required to read your mind. Is, “A description of the feature, along with the costs of enabling it” the sort of thing you had in mind? I had nothing in particular in mind. There ought to be other Kconfig options with at least half way reasonable help text, which I think could be used as guidance. Beyond that I think help text largely only re-stating what the prompt already says isn't helpful, and hence could as well be omitted. Update the help text and the HAS_ has been dropped in v1. Jan . ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v1] xen: make tracebuffer configurable
On 6/4/19 15:10, Jan Beulich wrote:
On 04.06.19 at 02:44, wrote:
--- a/xen/include/xen/trace.h
+++ b/xen/include/xen/trace.h
@@ -21,12 +21,15 @@
#ifndef __XEN_TRACE_H__
#define __XEN_TRACE_H__
+#ifdef CONFIG_TRACEBUFFER
extern int tb_init_done;
+#endif
If this is to stay up here (which I'm still not sure it needs to; I had
merely indicated that there likely is a reason for this without actually
knowing what that reason might be), then I think the #define needs
to go here as well, in an #else.
Yes, need to stay here because 'tb_init_done' used in 'asm-x86/trace.h'
which included by 'xen/trace.h' at line 32: #include
will be fixed in next version.
@@ -47,6 +50,20 @@ static inline void trace_var(u32 event, int cycles, int
extra,
void __trace_hypercall(uint32_t event, unsigned long op,
const xen_ulong_t *args);
+#else
+#define tb_init_done false
+static inline void init_trace_bufs(void) {}
+static inline int tb_control(struct xen_sysctl_tbuf_op *tbc) { return -ENOSYS;
}
-EOPNOTSUPP
Jan
.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v1] xen: make tracebuffer configurable
On 6/4/19 19:43, Jan Beulich wrote: On 04.06.19 at 12:49, wrote: On Jun 4, 2019, at 1:44 AM, Baodong Chen wrote: --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -368,4 +368,16 @@ config DOM0_MEM Leave empty if you are not sure what to specify. +config TRACEBUFFER + bool "Enable trace event logs" if EXPERT = "y" + ---help--- + Xen internal running status(trace event) will be saved to trace memory + when enabled. trace event data and config params can be read/changed + by system control hypercall at run time. + + This is used to record xen internal running status. + with a litte performance overhead. + Can be set to 'N' if you dont want this function. + When not configured, 'XEN_STSCTL_tbuf_op' command will result 'ENOSYS’. I think this would look better something like this: “Enable tracing infrastructure” “Enable in tracing infrastructure and pre-defined tracepoints within Xen. This will allow live information about Xen’s execution and performance to be collected at run time for debugging or performance analysis. Memory and execution overhead when not active is minimal." Also, I’m not 100% familiar with the kconfig syntax — I think we want tracing enabled by default unless actively disabled; is that what will happen here? Or will it only be enabled if EXPERT == ‘y’? Oh, indeed - there's a "default y" missing. Thanks for suggestion for the help text. and pointing out "default y" missing. Fixed in v3 and please discard v2. Jan . ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] xen: make tracebuffer configurable
On 6/5/19 18:38, George Dunlap wrote: On Jun 5, 2019, at 2:27 AM, Baodong Chen wrote: Xen internal running status(trace event) will be saved to trace memory when enabled. trace event data and config params can be read/changed by system control hypercall at run time. Can be disabled for smaller code footprint. Signed-off-by: Baodong Chen --- xen/common/Kconfig | 9 + xen/common/Makefile | 2 +- xen/include/xen/trace.h | 26 ++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/xen/common/Kconfig b/xen/common/Kconfig index c838506..d908fe1 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -368,4 +368,13 @@ config DOM0_MEM Leave empty if you are not sure what to specify. +config TRACEBUFFER + bool "Enable tracing infrastructure" if EXPERT = "y" + default y + ---help--- + Enable in tracing infrastructure and pre-defined tracepoints within Xen. Sorry, an editing mistake caused me to include a stray ‘in’ in this sentence when I suggested this text. :-) This could be removed on check-in. With that fixed, the commit message looks OK to me. Hello George, sorry for the late reply. Fixed according to your comments in v4. -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: vtimer: fix return value to void for virt_timer_[save|restore]
On 6/11/19 04:16, Julien Grall wrote:
Hi,
NIT: I would use "change" instead of "fix". I feel "fix" is more when
there are an actual bug.
Sound good to me.
On 6/10/19 6:07 AM, Baodong Chen wrote:
The original type is int and not used at all so fix to void.
The commit message is a bit unclear, you mention the type whereas the
key point is none of the callers are using the return value. So how
about:
"virt_timer_{save, return} always return 0 and none of the caller
actually check it. So change the return type to void."
If you are happy with it, I can make the modifications them on commit.
happy with it, please.
Cheers,
Signed-off-by: Baodong Chen
---
xen/arch/arm/vtimer.c | 6 ++
xen/include/asm-arm/vtimer.h | 4 ++--
2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index c99dd23..e6aebda 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -136,7 +136,7 @@ void vcpu_timer_destroy(struct vcpu *v)
kill_timer(&v->arch.phys_timer.timer);
}
-int virt_timer_save(struct vcpu *v)
+void virt_timer_save(struct vcpu *v)
{
ASSERT(!is_idle_vcpu(v));
@@ -149,10 +149,9 @@ int virt_timer_save(struct vcpu *v)
set_timer(&v->arch.virt_timer.timer,
ticks_to_ns(v->arch.virt_timer.cval +
v->domain->arch.virt_timer_base.offset -
boot_count));
}
- return 0;
}
-int virt_timer_restore(struct vcpu *v)
+void virt_timer_restore(struct vcpu *v)
{
ASSERT(!is_idle_vcpu(v));
@@ -163,7 +162,6 @@ int virt_timer_restore(struct vcpu *v)
WRITE_SYSREG64(v->domain->arch.virt_timer_base.offset, CNTVOFF_EL2);
WRITE_SYSREG64(v->arch.virt_timer.cval, CNTV_CVAL_EL0);
WRITE_SYSREG32(v->arch.virt_timer.ctl, CNTV_CTL_EL0);
- return 0;
}
static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, uint32_t
*r, bool read)
diff --git a/xen/include/asm-arm/vtimer.h b/xen/include/asm-arm/vtimer.h
index 91d88b3..9d4fb4c 100644
--- a/xen/include/asm-arm/vtimer.h
+++ b/xen/include/asm-arm/vtimer.h
@@ -24,8 +24,8 @@ extern int domain_vtimer_init(struct domain *d,
struct xen_arch_domainconfig *config);
extern int vcpu_vtimer_init(struct vcpu *v);
extern bool vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr);
-extern int virt_timer_save(struct vcpu *v);
-extern int virt_timer_restore(struct vcpu *v);
+extern void virt_timer_save(struct vcpu *v);
+extern void virt_timer_restore(struct vcpu *v);
extern void vcpu_timer_destroy(struct vcpu *v);
void vtimer_update_irqs(struct vcpu *v);
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: domain: remove redundant memset for arch's saved_context when creating vcpu
On 6/11/19 04:11, Julien Grall wrote: Hi, Thank you for the patch. The title should be at max 80 characters. So how about the following title? "xen/arm: domain: Remove redundant memset for v->arch.saved_context" Max 80 characters, roger that. On 6/10/19 6:15 AM, Baodong Chen wrote: Already done by clear_page() in alloc_vcpu_struct() Please try to make sentence in the commit message. For here I would suggest: "v->arch.saved_context is already zeroed in alloc_vcpu_struct() by clear_page(). So there are no need to memset it again in arch_vcpu_create()." If you are happy with the two changes, I can do them on commit. Thanks, please. Cheers, Signed-off-by: Baodong Chen --- xen/arch/arm/domain.c | 1 - 1 file changed, 1 deletion(-) diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index ff330b3..ad1b106 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -557,7 +557,6 @@ int arch_vcpu_create(struct vcpu *v) - sizeof(struct cpu_info)); memset(v->arch.cpu_info, 0, sizeof(*v->arch.cpu_info)); - memset(&v->arch.saved_context, 0, sizeof(v->arch.saved_context)); v->arch.saved_context.sp = (register_t)v->arch.cpu_info; v->arch.saved_context.pc = (register_t)continue_new_vcpu; ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: superficial clean-ups
On 6/11/19 17:45, Andrew Cooper wrote: On 11/06/2019 10:20, Baodong Chen wrote: * Remove redundant set 'DOMDYING_dead' domain_create() will set this when fail, thus no need set in arch_domain_create(). Its not redundant. It is necessary for correct cleanup. Hello Andrew, Thanks for your comments. Your concern is: when the arch_domain_create() fails, some cleanup work need to done in this function. and 'DOMDYING_dead' flags maybe needed to judge for correct cleanup? If so, it's not redundant. I'm curious why 'DOMDYING_dead' been set by fail path both in arch_domain_create() and domain_create(). All of this logic will be rewritten when the destroy paths are fully idempotent, and while ARM is fairing well in this regard, the common and x86 needs more work. ~Andrew . ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/scheduler: remove 'name' from 'struct scheduler'
On 6/11/19 18:04, George Dunlap wrote: On 6/11/19 2:35 AM, Baodong Chen wrote: 'struct scheduler' already has member 'opt_name' and 'sched_id', thus 'name' is a little redundant, so remove it. Signed-off-by: Baodong Chen It's not redundant; one is a longer-form human-readable description, another is a shorthand "option" description. You can't be saving more than what, 500 bytes here? I understand you're trying to cut things down as small as you can, but this seems a bit excessive. Hello George, Roger that. I thought besides 'name', both 'opt_name' and 'sched_id' can identify the scheduler and the 'name' mainly used in logs, thus can be removed. As your suggestion, can leave it as it is. -George . ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/coverage: wrap coverage related things under 'CONFIG_COVERAGE'
On 6/11/19 22:03, Jan Beulich wrote:
On 11.06.19 at 08:02, wrote:
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -240,12 +240,14 @@ SECTIONS
*(.altinstructions)
__alt_instructions_end = .;
+#if defined(CONFIG_COVERAGE)
. = ALIGN(8);
__ctors_start = .;
*(.ctors)
*(.init_array)
*(SORT(.init_array.*))
__ctors_end = .;
+#endif
How is this (only) coverage related? And how is making this conditional
going to help in any way?
Hello Jan,
When i read the code 'init_constructors()', i want to understand when
it's used.
I can not find any helper macros like '__init' in init.h, put things in
this section.
Also run under arm foundation platform, the section is empty.
So i check commit history and found it's commit logs: it is coverage
related.
And compiled with CONFIG_COVERAGE enabled, this section is not empty
anymore.
So the patch mainly want to clarify the code is coverage related,
which want to help newcomer easily understand this code.
Am i misunderstanding here?
And if we were to take this, then please use the shorter #ifdef.
Yes, can be fixed.
--- a/xen/common/lib.c
+++ b/xen/common/lib.c
@@ -491,15 +491,20 @@ unsigned long long parse_size_and_unit(const char *s,
const char **ps)
return ret;
}
+#if defined(CONFIG_COVERAGE)
typedef void (*ctor_func_t)(void);
extern const ctor_func_t __ctors_start[], __ctors_end[];
+#endif
Again - how does this help?
Want to clarify this is coverage related code.
+/* see 'docs/hypervisor-guide/code-coverage.rst' */
void __init init_constructors(void)
There's no mention of this function in the referenced docs file.
Same as above.
{
+#if defined(CONFIG_COVERAGE)
const ctor_func_t *f;
for ( f = __ctors_start; f < __ctors_end; ++f )
(*f)();
+#endif
/* Putting this here seems as good (or bad) as any other place. */
Again, besides lacking suitable reasoning you also should look
more closely, in this case where exactly it makes sense to place
the #endif.
The blank line here? If yes, can be removed. i missed this.
Jan
.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: superficial clean-ups
On 6/11/19 18:53, Andrew Cooper wrote: On 11/06/2019 11:33, chenbaodong wrote: On 6/11/19 17:45, Andrew Cooper wrote: On 11/06/2019 10:20, Baodong Chen wrote: * Remove redundant set 'DOMDYING_dead' domain_create() will set this when fail, thus no need set in arch_domain_create(). Its not redundant. It is necessary for correct cleanup. Hello Andrew, Thanks for your comments. Your concern is: when the arch_domain_create() fails, some cleanup work need to done in this function. and 'DOMDYING_dead' flags maybe needed to judge for correct cleanup? If so, it's not redundant. I'm curious why 'DOMDYING_dead' been set by fail path both in arch_domain_create() and domain_create(). Because various cleanup paths BUG_ON(!d->is_dying), including ones before hitting the main error path in domain_create(). Thanks for clarify, my fault, i missed (!d->is_dying) related check. And tested by force to run fail path in arch_domain_create(), but nothing happed in arch_domain_destory(). result is: Panic on CPU 0: Error creating domain 0 Seems the cleanup path run with success. Anyway, can leave it as what it was. ~Andrew . ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: superficial clean-ups
On 6/11/19 18:29, Juergen Gross wrote:
On 11.06.19 12:27, Andrew Cooper wrote:
On 11/06/2019 11:25, Juergen Gross wrote:
On 11.06.19 12:18, George Dunlap wrote:
On 6/11/19 10:20 AM, Baodong Chen wrote:
* Remove redundant set 'DOMDYING_dead'
domain_create() will set this when fail, thus no need
set in arch_domain_create().
* Set error when really happened
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 86341bc..d6cdcf8 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1894,9 +1894,11 @@ struct scheduler *scheduler_alloc(unsigned
int sched_id, int *perr)
return NULL;
found:
- *perr = -ENOMEM;
if ( (sched = xmalloc(struct scheduler)) == NULL )
+ {
+ *perr = -ENOMEM;
return NULL;
+ }
memcpy(sched, schedulers[i], sizeof(*sched));
if ( (*perr = SCHED_OP(sched, init)) != 0 )
I was going to say, this is a common idiom in the Xen code, and the
compiler will end up re-organizing things such that the write doesn't
happen anyway. But in this case, its' actually writing through a
pointer before and after a function call; I don't think the compiler
would actually be allowed to optimize this write away.
So, I guess I'd be OK with this particular hunk. Dario, any opinions?
I'd rather switch to PTR_ERR() here dropping the perr parameter.
+2 for this, but I was going to wait until core scheduling had gotten a
bit further before suggesting cleanup which is guaranteed to collide.
Sadly, it's fairly intrusive in the cpupool code as well.
I can add this to my list of scheduler cleanups to do.
Copy that.
Juergen
.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/coverage: wrap coverage related things under 'CONFIG_COVERAGE'
On 6/12/19 14:34, Jan Beulich wrote:
On 12.06.19 at 02:23, wrote:
On 6/11/19 22:03, Jan Beulich wrote:
On 11.06.19 at 08:02, wrote:
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -240,12 +240,14 @@ SECTIONS
*(.altinstructions)
__alt_instructions_end = .;
+#if defined(CONFIG_COVERAGE)
. = ALIGN(8);
__ctors_start = .;
*(.ctors)
*(.init_array)
*(SORT(.init_array.*))
__ctors_end = .;
+#endif
How is this (only) coverage related? And how is making this conditional
going to help in any way?
Hello Jan,
When i read the code 'init_constructors()', i want to understand when
it's used.
I can not find any helper macros like '__init' in init.h, put things in
this section.
Also run under arm foundation platform, the section is empty.
So i check commit history and found it's commit logs: it is coverage
related.
And compiled with CONFIG_COVERAGE enabled, this section is not empty
anymore.
So the patch mainly want to clarify the code is coverage related,
which want to help newcomer easily understand this code.
Am i misunderstanding here?
The code may have been _introduced_ for coverage, but are you
willing to guarantee it's coverage-only? Plus - what does removing
an empty section buy you?
Currently seems true, but not sure about the future, can not guarantee.
Personally guess this should not be used by xen, but use __init_call(fn)
like in init.h.
My purpose is to clarify the code is coverage related(at least currently
is).
If you are unhappy with it this way, how about just add a comment,
something like:
+/* currently only used by code coverage */
void __init init_constructors(void)
--- a/xen/common/lib.c
+++ b/xen/common/lib.c
@@ -491,15 +491,20 @@ unsigned long long parse_size_and_unit(const char *s,
const char **ps)
return ret;
}
+#if defined(CONFIG_COVERAGE)
typedef void (*ctor_func_t)(void);
extern const ctor_func_t __ctors_start[], __ctors_end[];
+#endif
Again - how does this help?
Want to clarify this is coverage related code.
If only it really was (provably).
+/* see 'docs/hypervisor-guide/code-coverage.rst' */
void __init init_constructors(void)
There's no mention of this function in the referenced docs file.
Same as above.
No. The reference makes no sense here without that doc somehow
mentioning the function you attach the comment to.
{
+#if defined(CONFIG_COVERAGE)
const ctor_func_t *f;
for ( f = __ctors_start; f < __ctors_end; ++f )
(*f)();
+#endif
/* Putting this here seems as good (or bad) as any other place. */
Again, besides lacking suitable reasoning you also should look
more closely, in this case where exactly it makes sense to place
the #endif.
The blank line here? If yes, can be removed. i missed this.
Removed? No. If anything there's one missing. You've inserted
the #ifdef after the blank line rather than before it.
Sorry for my expression, what you said here is exactly what i want.
Jan
.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/coverage: wrap coverage related things under 'CONFIG_COVERAGE'
On 6/12/19 15:58, Jan Beulich wrote: On 12.06.19 at 09:36, wrote: On 6/12/19 14:34, Jan Beulich wrote: On 12.06.19 at 02:23, wrote: On 6/11/19 22:03, Jan Beulich wrote: On 11.06.19 at 08:02, wrote: --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -240,12 +240,14 @@ SECTIONS *(.altinstructions) __alt_instructions_end = .; +#if defined(CONFIG_COVERAGE) . = ALIGN(8); __ctors_start = .; *(.ctors) *(.init_array) *(SORT(.init_array.*)) __ctors_end = .; +#endif How is this (only) coverage related? And how is making this conditional going to help in any way? Hello Jan, When i read the code 'init_constructors()', i want to understand when it's used. I can not find any helper macros like '__init' in init.h, put things in this section. Also run under arm foundation platform, the section is empty. So i check commit history and found it's commit logs: it is coverage related. And compiled with CONFIG_COVERAGE enabled, this section is not empty anymore. So the patch mainly want to clarify the code is coverage related, which want to help newcomer easily understand this code. Am i misunderstanding here? The code may have been _introduced_ for coverage, but are you willing to guarantee it's coverage-only? Plus - what does removing an empty section buy you? Currently seems true, but not sure about the future, can not guarantee. Personally guess this should not be used by xen, but use __init_call(fn) like in init.h. My purpose is to clarify the code is coverage related(at least currently is). If you are unhappy with it this way, how about just add a comment, something like: +/* currently only used by code coverage */ void __init init_constructors(void) I'd prefer if the entire patch was dropped, unless there really was a clear (and clearly spelled out) gain. Adding a comment like you suggest only calls for it going stale at some point. Copy that. Thanks for all your comments. Jan . ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: io: add function swap_mmio_handler()
On 6/12/19 17:08, Julien Grall wrote: Hi, On 6/12/19 6:42 AM, Baodong Chen wrote: Swap function can be used when calling sort(). or else, the default swap function generic_swap() is used, which is a little inefficient. I am not entirely convince this will be more efficient. mmio_handler does not fit in 64 bit, so the compiler may decide to do either multiple load or replace with a memcpy. Hello Julien, I have checked the disassemble result, and IIUC generic_swap has a loop so it should be a little inefficient. I'm not expert about hardware, please correct me if i'm wrong. 0022ee88 : 22ee88: d283 mov x3, #0x0 // #0 22ee8c: d503201f nop 22ee90: 38636825 ldrb w5, [x1, x3] 22ee94: 38636804 ldrb w4, [x0, x3] 22ee98: 38236805 strb w5, [x0, x3] 22ee9c: 38236824 strb w4, [x1, x3] 22eea0: 91000463 add x3, x3, #0x1 22eea4: 4b030044 sub w4, w2, w3 22eea8: 719f cmp w4, #0x0 22eeac: 542c b.gt 22ee90 22eeb0: d65f03c0 ret 22eeb4: d503201f nop 00242db8 : 242db8: a9400c22 ldp x2, x3, [x1] 242dbc: d10083ff sub sp, sp, #0x20 242dc0: a9401404 ldp x4, x5, [x0] 242dc4: a9000c02 stp x2, x3, [x0] 242dc8: a9410c02 ldp x2, x3, [x0, #16] 242dcc: a9411c26 ldp x6, x7, [x1, #16] 242dd0: a9011c06 stp x6, x7, [x0, #16] 242dd4: a9001424 stp x4, x5, [x1] 242dd8: a9010c22 stp x2, x3, [x1, #16] 242ddc: 910083ff add sp, sp, #0x20 242de0: d65f03c0 ret 242de4: d503201f nop So at best this feels some micro-optimization. But then, this is only call a limited number of time at each domain build. Is it really worth it? It's not hot path here. Not sure about worth. Personally i will try my best to do things well according to my understanding. On a side note, I have noticed you are sending a lot of optimization/clean-up patch. What is your end goal here? My goal is to understand how xen works well. If it is to improve the performance, then there are much bigger fish to fry within Xen code base. I am happy to point some of them based on where you are looking to improve. Surly i want to improve performance. Features like Fast Startup ( I learned from xen summit 2018, samsung automotive presentation). But currently i don't understand xen well, only a few weeks experience. I'm afraid can't catch big fish. Cheers, ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: io: add function swap_mmio_handler()
On 6/12/19 20:21, Julien Grall wrote: Hi, On 12/06/2019 11:08, chenbaodong wrote: On 6/12/19 17:08, Julien Grall wrote: Hi, On 6/12/19 6:42 AM, Baodong Chen wrote: Swap function can be used when calling sort(). or else, the default swap function generic_swap() is used, which is a little inefficient. I am not entirely convince this will be more efficient. mmio_handler does not fit in 64 bit, so the compiler may decide to do either multiple load or replace with a memcpy. Hello Julien, I have checked the disassemble result, and IIUC generic_swap has a loop so it should be a little inefficient. I'm not expert about hardware, please correct me if i'm wrong. I am not an hardware expert too... But as I pointed out below this is a micro-optimization. In other words, you are tailoring a specific function that may run faster now, but this is improvement is going to be lost as this is just a very tiny part of the domain creation. [...] So at best this feels some micro-optimization. But then, this is only call a limited number of time at each domain build. Is it really worth it? It's not hot path here. Not sure about worth. Personally i will try my best to do things well according to my understanding. Micro-optimization are always good, but you also have to factor the cost of maintaining and whether this will improve significantly Xen. On a side note, I have noticed you are sending a lot of optimization/clean-up patch. What is your end goal here? My goal is to understand how xen works well. If it is to improve the performance, then there are much bigger fish to fry within Xen code base. I am happy to point some of them based on where you are looking to improve. Surly i want to improve performance. Features like Fast Startup ( I learned from xen summit 2018, samsung automotive presentation). But currently i don't understand xen well, only a few weeks experience. We do have small task for newcomers that would improve Xen code base and also allow your to understand more some part of the code. If you have a specific area of interest, I can see if I have some small tasks there. I'm happy with this. Interested in arm platform for embedded and automotive use case. things like in this link: https://xenproject.org/developers/teams/embedded-and-automotive/ Cheers, ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: io: add function swap_mmio_handler()
On 6/25/19 16:46, Julien Grall wrote: HiStefano, On 25/06/2019 00:59, Stefano Stabellini wrote: On Mon, 24 Jun 2019, Julien Grall wrote: Hi, On 6/24/19 9:17 PM, Stefano Stabellini wrote: On Mon, 24 Jun 2019, Julien Grall wrote: Hi Stefano, On 24/06/2019 19:27, Stefano Stabellini wrote: On Mon, 24 Jun 2019, Stefano Stabellini wrote: On Thu, 13 Jun 2019, chenbaodong wrote: Let me add that if you prefer to document one of the other interfaces listed above in my email, you are welcome to pick another one. For example, we are also missing a doc about the DomU memory map, listing all memory regions with addresses and sizes, both MMIO and normal memory. For that, most of the information is: xen/include/public/arch-arm.h A well written in-code comment in arch-arm.h would be OK, or also a document under docs/misc/arm. Please no duplication, it is already quite hard to maintain one place. Instead, we should document all the headers in a documented format that can be extracted automatically. As we have no such thing today (as far as I am aware), please make a proposal with a bit more details, otherwise I don't think Baodong will be able to take the next step. I don't have a concrete proposal so far. Except that documentation outside of the headers is a no-go from my side. The goal of documenting within the headers rather than outside is you also help the developer of guest OS. A few weeks ago Ian Jackson pointed to docs/xen-headers for a potential syntax. Sadly, there are no documentation of the script so far. I haven't had time to look it so far. In that case, I'd suggest for Baodong to either pick the device tree documentation item (assuming you are OK with that one being under docs/misc/arm) or just write a normal in-code comment in arch-arm.h for the domU memory map not worrying about the format of the in-code comment for now. I don't think we have specific place for documenting device-tree so docs/misc/arm would be suitable. Regarding in-code comment in arch-arm.h This will always be an improvement to what we have. However, it would be good if someone take an action to formalize the documentation format. I will look into this. Currently interrupted by some other work, will be back soon. Cheers, ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
