I am working on some other bugs and perf issues - and while working I noticed that both sparse and Coverity have reported some issues with Xen drivers. Please see attached various bug-fixes that I am proposing for 3.6.
Konrad Rzeszutek Wilk
2012-Jul-03 15:40 UTC
[PATCH 1/4] xen/p2m: Optimize the get_phys_to_machine function a bit.
Running perf tells me that the check for p2m_identity is done a lot - more than it should. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- arch/x86/xen/p2m.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c index 64effdc..161bcdc 100644 --- a/arch/x86/xen/p2m.c +++ b/arch/x86/xen/p2m.c @@ -406,7 +406,7 @@ unsigned long get_phys_to_machine(unsigned long pfn) * and in p2m_*missing, so returning the INVALID_P2M_ENTRY * would be wrong. */ - if (p2m_top[topidx][mididx] == p2m_identity) + if (unlikely(p2m_top[topidx][mididx] == p2m_identity)) return IDENTITY_FRAME(pfn); return p2m_top[topidx][mididx][idx]; -- 1.7.7.6
Konrad Rzeszutek Wilk
2012-Jul-03 15:40 UTC
[PATCH 2/4] xen/perf: Define .glob for the different hypercalls.
This allows us in perf to have this: 99.67% [kernel] [k] xen_hypercall_sched_op 0.11% [kernel] [k] xen_hypercall_xen_version instead of the borring ever-encompassing: 99.13% [kernel] [k] hypercall_page Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- arch/x86/xen/xen-head.S | 102 ++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 100 insertions(+), 2 deletions(-) diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S index aaa7291..f6ba51d 100644 --- a/arch/x86/xen/xen-head.S +++ b/arch/x86/xen/xen-head.S @@ -28,9 +28,107 @@ ENTRY(startup_xen) __FINIT .pushsection .text - .align PAGE_SIZE + .balign PAGE_SIZE ENTRY(hypercall_page) - .skip PAGE_SIZE +ENTRY(xen_hypercall_set_trap_table) + .skip 32 +ENTRY(xen_hypercall_mmu_update) + .skip 32 +ENTRY(xen_hypercall_set_gdt) + .skip 32 +ENTRY(xen_hypercall_stack_switch) + .skip 32 +ENTRY(xen_hypercall_set_callbacks) + .skip 32 +ENTRY(xen_hypercall_fpu_taskswitch) + .skip 32 +ENTRY(xen_hypercall_sched_op_compat) + .skip 32 +ENTRY(xen_hypercall_platform_op) + .skip 32 +ENTRY(xen_hypercall_set_debugreg) + .skip 32 +ENTRY(xen_hypercall_get_debugreg) + .skip 32 +ENTRY(xen_hypercall_update_descriptor) + .skip 32 +ENTRY(xen_hypercall_ni) + .skip 32 +ENTRY(xen_hypercall_memory_op) + .skip 32 +ENTRY(xen_hypercall_multicall) + .skip 32 +ENTRY(xen_hypercall_update_va_mapping) + .skip 32 +ENTRY(xen_hypercall_set_timer_op) + .skip 32 +ENTRY(xen_hypercall_event_channel_op_compat) + .skip 32 +ENTRY(xen_hypercall_xen_version) + .skip 32 +ENTRY(xen_hypercall_console_io) + .skip 32 +ENTRY(xen_hypercall_physdev_op_compat) + .skip 32 +ENTRY(xen_hypercall_grant_table_op) + .skip 32 +ENTRY(xen_hypercall_vm_assist) + .skip 32 +ENTRY(xen_hypercall_update_va_mapping_otherdomain) + .skip 32 +ENTRY(xen_hypercall_iret) + .skip 32 +ENTRY(xen_hypercall_vcpu_op) + .skip 32 +ENTRY(xen_hypercall_set_segment_base) + .skip 32 +ENTRY(xen_hypercall_mmuext_op) + .skip 32 +ENTRY(xen_hypercall_xsm_op) + .skip 32 +ENTRY(xen_hypercall_nmi_op) + .skip 32 +ENTRY(xen_hypercall_sched_op) + .skip 32 +ENTRY(xen_hypercall_callback_op) + .skip 32 +ENTRY(xen_hypercall_xenoprof_op) + .skip 32 +ENTRY(xen_hypercall_event_channel_op) + .skip 32 +ENTRY(xen_hypercall_physdev_op) + .skip 32 +ENTRY(xen_hypercall_hvm_op) + .skip 32 +ENTRY(xen_hypercall_sysctl) + .skip 32 +ENTRY(xen_hypercall_domctl) + .skip 32 +ENTRY(xen_hypercall_kexec_op) + .skip 32 +ENTRY(xen_hypercall_tmem_op) /* 38 */ + .skip 32 +ENTRY(xen_hypercall_rsvr) + .skip 320 +ENTRY(xen_hypercall_mca) /* 48 */ + .skip 32 +ENTRY(xen_hypercall_arch_1) + .skip 32 +ENTRY(xen_hypercall_arch_2) + .skip 32 +ENTRY(xen_hypercall_arch_3) + .skip 32 +ENTRY(xen_hypercall_arch_4) + .skip 32 +ENTRY(xen_hypercall_arch_5) + .skip 32 +ENTRY(xen_hypercall_arch_6) + .skip 32 +ENTRY(xen_hypercall_arch_7) + .skip 32 +ENTRY(xen_hypercall_other) + .skip 2272 + .popsection ELFNOTE(Xen, XEN_ELFNOTE_GUEST_OS, .asciz "linux") -- 1.7.7.6
Konrad Rzeszutek Wilk
2012-Jul-03 15:40 UTC
[PATCH 3/4] xen/acpi: Fix potential memory leak.
Coverity points out that we do not free in one case the pr_backup - and sure enough we forgot. Found by Coverity (CID 401970) Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- drivers/xen/xen-acpi-processor.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c index 7ff2569e..b590ee0 100644 --- a/drivers/xen/xen-acpi-processor.c +++ b/drivers/xen/xen-acpi-processor.c @@ -520,15 +520,18 @@ static int __init xen_acpi_processor_init(void) if (!pr_backup) { pr_backup = kzalloc(sizeof(struct acpi_processor), GFP_KERNEL); - memcpy(pr_backup, _pr, sizeof(struct acpi_processor)); + if (pr_backup) + memcpy(pr_backup, _pr, sizeof(struct acpi_processor)); } (void)upload_pm_data(_pr); } rc = check_acpi_ids(pr_backup); - if (rc) - goto err_unregister; kfree(pr_backup); + pr_backup = NULL; + + if (rc) + goto err_unregister; return 0; err_unregister: -- 1.7.7.6
Konrad Rzeszutek Wilk
2012-Jul-03 15:40 UTC
[PATCH 4/4] xen/hvc: Fix up checks when the info is allocated.
Coverity would complain about this - even thought it looks OK. CID 401957 Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- drivers/tty/hvc/hvc_xen.c | 15 ++++++--------- 1 files changed, 6 insertions(+), 9 deletions(-) diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c index 944eaeb..fbf41be 100644 --- a/drivers/tty/hvc/hvc_xen.c +++ b/drivers/tty/hvc/hvc_xen.c @@ -209,10 +209,8 @@ static int xen_hvm_console_init(void) info = kzalloc(sizeof(struct xencons_info), GFP_KERNEL | __GFP_ZERO); if (!info) return -ENOMEM; - } - - /* already configured */ - if (info->intf != NULL) + } else if (info->intf != NULL) { + /* already configured */ return 0; /* * If the toolstack (or the hypervisor) hasn''t set these values, the @@ -220,6 +218,7 @@ static int xen_hvm_console_init(void) * theoretically correct values, in practice they never are and they * mean that a legacy toolstack hasn''t initialized the pv console correctly. */ + */ r = hvm_get_parameter(HVM_PARAM_CONSOLE_EVTCHN, &v); if (r < 0 || v == 0) goto err; @@ -259,12 +258,10 @@ static int xen_pv_console_init(void) info = kzalloc(sizeof(struct xencons_info), GFP_KERNEL | __GFP_ZERO); if (!info) return -ENOMEM; - } - - /* already configured */ - if (info->intf != NULL) + } else if (info->intf != NULL) { + /* already configured */ return 0; - + } info->evtchn = xen_start_info->console.domU.evtchn; info->intf = mfn_to_virt(xen_start_info->console.domU.mfn); info->vtermno = HVC_COOKIE; -- 1.7.7.6
Jan Beulich
2012-Jul-03 16:07 UTC
Re: [PATCH 2/4] xen/perf: Define .glob for the different hypercalls.
>>> On 03.07.12 at 17:40, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > This allows us in perf to have this: > > 99.67% [kernel] [k] xen_hypercall_sched_op > 0.11% [kernel] [k] xen_hypercall_xen_version > > instead of the borring ever-encompassing: > > 99.13% [kernel] [k] hypercall_page > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > arch/x86/xen/xen-head.S | 102 > ++++++++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 100 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S > index aaa7291..f6ba51d 100644 > --- a/arch/x86/xen/xen-head.S > +++ b/arch/x86/xen/xen-head.S > @@ -28,9 +28,107 @@ ENTRY(startup_xen) > __FINIT > > .pushsection .text > - .align PAGE_SIZE > + .balign PAGE_SIZE > ENTRY(hypercall_page) > - .skip PAGE_SIZE > +ENTRY(xen_hypercall_set_trap_table) > + .skip 32 > +ENTRY(xen_hypercall_mmu_update) > + .skip 32 > +ENTRY(xen_hypercall_set_gdt) > + .skip 32 > +ENTRY(xen_hypercall_stack_switch) > + .skip 32 > +ENTRY(xen_hypercall_set_callbacks) > + .skip 32 > +ENTRY(xen_hypercall_fpu_taskswitch) > + .skip 32 > +ENTRY(xen_hypercall_sched_op_compat) > + .skip 32 > +ENTRY(xen_hypercall_platform_op) > + .skip 32 > +ENTRY(xen_hypercall_set_debugreg) > + .skip 32 > +ENTRY(xen_hypercall_get_debugreg) > + .skip 32 > +ENTRY(xen_hypercall_update_descriptor) > + .skip 32 > +ENTRY(xen_hypercall_ni) > + .skip 32 > +ENTRY(xen_hypercall_memory_op) > + .skip 32 > +ENTRY(xen_hypercall_multicall) > + .skip 32 > +ENTRY(xen_hypercall_update_va_mapping) > + .skip 32 > +ENTRY(xen_hypercall_set_timer_op) > + .skip 32 > +ENTRY(xen_hypercall_event_channel_op_compat) > + .skip 32 > +ENTRY(xen_hypercall_xen_version) > + .skip 32 > +ENTRY(xen_hypercall_console_io) > + .skip 32 > +ENTRY(xen_hypercall_physdev_op_compat) > + .skip 32 > +ENTRY(xen_hypercall_grant_table_op) > + .skip 32 > +ENTRY(xen_hypercall_vm_assist) > + .skip 32 > +ENTRY(xen_hypercall_update_va_mapping_otherdomain) > + .skip 32 > +ENTRY(xen_hypercall_iret) > + .skip 32 > +ENTRY(xen_hypercall_vcpu_op) > + .skip 32 > +ENTRY(xen_hypercall_set_segment_base) > + .skip 32 > +ENTRY(xen_hypercall_mmuext_op) > + .skip 32 > +ENTRY(xen_hypercall_xsm_op) > + .skip 32 > +ENTRY(xen_hypercall_nmi_op) > + .skip 32 > +ENTRY(xen_hypercall_sched_op) > + .skip 32 > +ENTRY(xen_hypercall_callback_op) > + .skip 32 > +ENTRY(xen_hypercall_xenoprof_op) > + .skip 32 > +ENTRY(xen_hypercall_event_channel_op) > + .skip 32 > +ENTRY(xen_hypercall_physdev_op) > + .skip 32 > +ENTRY(xen_hypercall_hvm_op) > + .skip 32 > +ENTRY(xen_hypercall_sysctl) > + .skip 32 > +ENTRY(xen_hypercall_domctl) > + .skip 32 > +ENTRY(xen_hypercall_kexec_op) > + .skip 32 > +ENTRY(xen_hypercall_tmem_op) /* 38 */ > + .skip 32 > +ENTRY(xen_hypercall_rsvr) > + .skip 320 > +ENTRY(xen_hypercall_mca) /* 48 */ > + .skip 32 > +ENTRY(xen_hypercall_arch_1) > + .skip 32 > +ENTRY(xen_hypercall_arch_2) > + .skip 32 > +ENTRY(xen_hypercall_arch_3) > + .skip 32 > +ENTRY(xen_hypercall_arch_4) > + .skip 32 > +ENTRY(xen_hypercall_arch_5) > + .skip 32 > +ENTRY(xen_hypercall_arch_6) > + .skip 32 > +ENTRY(xen_hypercall_arch_7) > + .skip 32 > +ENTRY(xen_hypercall_other) > + .skip 2272May I suggest that you use .balign PAGE_SIZE here again, avoiding the need to adjust the number with every future addition at the end? Jan> + > .popsection > > ELFNOTE(Xen, XEN_ELFNOTE_GUEST_OS, .asciz "linux")
Jan Beulich
2012-Jul-03 16:11 UTC
Re: [PATCH 4/4] xen/hvc: Fix up checks when the info is allocated.
>>> On 03.07.12 at 17:40, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > Coverity would complain about this - even thought it looks OK. > > CID 401957 > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > drivers/tty/hvc/hvc_xen.c | 15 ++++++--------- > 1 files changed, 6 insertions(+), 9 deletions(-) > > diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c > index 944eaeb..fbf41be 100644 > --- a/drivers/tty/hvc/hvc_xen.c > +++ b/drivers/tty/hvc/hvc_xen.c > @@ -209,10 +209,8 @@ static int xen_hvm_console_init(void) > info = kzalloc(sizeof(struct xencons_info), GFP_KERNEL | __GFP_ZERO); > if (!info) > return -ENOMEM; > - } > - > - /* already configured */ > - if (info->intf != NULL) > + } else if (info->intf != NULL) { > + /* already configured */ > return 0;Is this patch perhaps stale? There appears to be a missing } here, without which I can''t see how this would have built, ...> /* > * If the toolstack (or the hypervisor) hasn''t set these values, the > @@ -220,6 +218,7 @@ static int xen_hvm_console_init(void) > * theoretically correct values, in practice they never are and they > * mean that a legacy toolstack hasn''t initialized the pv console correctly. > */ > + */... and this one likely wouldn''t build either. Jan> r = hvm_get_parameter(HVM_PARAM_CONSOLE_EVTCHN, &v); > if (r < 0 || v == 0) > goto err; > @@ -259,12 +258,10 @@ static int xen_pv_console_init(void) > info = kzalloc(sizeof(struct xencons_info), GFP_KERNEL | __GFP_ZERO); > if (!info) > return -ENOMEM; > - } > - > - /* already configured */ > - if (info->intf != NULL) > + } else if (info->intf != NULL) { > + /* already configured */ > return 0; > - > + } > info->evtchn = xen_start_info->console.domU.evtchn; > info->intf = mfn_to_virt(xen_start_info->console.domU.mfn); > info->vtermno = HVC_COOKIE;
David Vrabel
2012-Jul-03 16:14 UTC
Re: [Xen-devel] [PATCH 2/4] xen/perf: Define .glob for the different hypercalls.
On 03/07/12 16:40, Konrad Rzeszutek Wilk wrote:> This allows us in perf to have this: > > 99.67% [kernel] [k] xen_hypercall_sched_op > 0.11% [kernel] [k] xen_hypercall_xen_version > > instead of the borring ever-encompassing: > > 99.13% [kernel] [k] hypercall_page...> +ENTRY(xen_hypercall_other) > + .skip 2272I can''t offhand think of way of doing this, but is there a less fragile way to do this? .skip PAGE_SIZE - N * 32 or something ? David
Ian Campbell
2012-Jul-03 16:17 UTC
Re: [Xen-devel] [PATCH 2/4] xen/perf: Define .glob for the different hypercalls.
On Tue, 2012-07-03 at 16:40 +0100, Konrad Rzeszutek Wilk wrote:> This allows us in perf to have this: > > 99.67% [kernel] [k] xen_hypercall_sched_op > 0.11% [kernel] [k] xen_hypercall_xen_version > > instead of the borring ever-encompassing: > > 99.13% [kernel] [k] hypercall_page > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>I don''t suppose it would be possible to automate this somehow would it? Perhaps a small perl snippet consuming the output of grep ^\#define.__HYPERVISOR_ include/xen/interface/xen.h ?> --- > arch/x86/xen/xen-head.S | 102 ++++++++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 100 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S > index aaa7291..f6ba51d 100644 > --- a/arch/x86/xen/xen-head.S > +++ b/arch/x86/xen/xen-head.S > @@ -28,9 +28,107 @@ ENTRY(startup_xen) > __FINIT > > .pushsection .text > - .align PAGE_SIZE > + .balign PAGE_SIZE > ENTRY(hypercall_page) > - .skip PAGE_SIZE > +ENTRY(xen_hypercall_set_trap_table) > + .skip 32 > +ENTRY(xen_hypercall_mmu_update) > + .skip 32 > +ENTRY(xen_hypercall_set_gdt) > + .skip 32 > +ENTRY(xen_hypercall_stack_switch) > + .skip 32 > +ENTRY(xen_hypercall_set_callbacks) > + .skip 32 > +ENTRY(xen_hypercall_fpu_taskswitch) > + .skip 32 > +ENTRY(xen_hypercall_sched_op_compat) > + .skip 32 > +ENTRY(xen_hypercall_platform_op) > + .skip 32 > +ENTRY(xen_hypercall_set_debugreg) > + .skip 32 > +ENTRY(xen_hypercall_get_debugreg) > + .skip 32 > +ENTRY(xen_hypercall_update_descriptor) > + .skip 32 > +ENTRY(xen_hypercall_ni) > + .skip 32 > +ENTRY(xen_hypercall_memory_op) > + .skip 32 > +ENTRY(xen_hypercall_multicall) > + .skip 32 > +ENTRY(xen_hypercall_update_va_mapping) > + .skip 32 > +ENTRY(xen_hypercall_set_timer_op) > + .skip 32 > +ENTRY(xen_hypercall_event_channel_op_compat) > + .skip 32 > +ENTRY(xen_hypercall_xen_version) > + .skip 32 > +ENTRY(xen_hypercall_console_io) > + .skip 32 > +ENTRY(xen_hypercall_physdev_op_compat) > + .skip 32 > +ENTRY(xen_hypercall_grant_table_op) > + .skip 32 > +ENTRY(xen_hypercall_vm_assist) > + .skip 32 > +ENTRY(xen_hypercall_update_va_mapping_otherdomain) > + .skip 32 > +ENTRY(xen_hypercall_iret) > + .skip 32 > +ENTRY(xen_hypercall_vcpu_op) > + .skip 32 > +ENTRY(xen_hypercall_set_segment_base) > + .skip 32 > +ENTRY(xen_hypercall_mmuext_op) > + .skip 32 > +ENTRY(xen_hypercall_xsm_op) > + .skip 32 > +ENTRY(xen_hypercall_nmi_op) > + .skip 32 > +ENTRY(xen_hypercall_sched_op) > + .skip 32 > +ENTRY(xen_hypercall_callback_op) > + .skip 32 > +ENTRY(xen_hypercall_xenoprof_op) > + .skip 32 > +ENTRY(xen_hypercall_event_channel_op) > + .skip 32 > +ENTRY(xen_hypercall_physdev_op) > + .skip 32 > +ENTRY(xen_hypercall_hvm_op) > + .skip 32 > +ENTRY(xen_hypercall_sysctl) > + .skip 32 > +ENTRY(xen_hypercall_domctl) > + .skip 32 > +ENTRY(xen_hypercall_kexec_op) > + .skip 32 > +ENTRY(xen_hypercall_tmem_op) /* 38 */ > + .skip 32 > +ENTRY(xen_hypercall_rsvr) > + .skip 320 > +ENTRY(xen_hypercall_mca) /* 48 */ > + .skip 32 > +ENTRY(xen_hypercall_arch_1) > + .skip 32 > +ENTRY(xen_hypercall_arch_2) > + .skip 32 > +ENTRY(xen_hypercall_arch_3) > + .skip 32 > +ENTRY(xen_hypercall_arch_4) > + .skip 32 > +ENTRY(xen_hypercall_arch_5) > + .skip 32 > +ENTRY(xen_hypercall_arch_6) > + .skip 32 > +ENTRY(xen_hypercall_arch_7) > + .skip 32 > +ENTRY(xen_hypercall_other) > + .skip 2272 > + > .popsection > > ELFNOTE(Xen, XEN_ELFNOTE_GUEST_OS, .asciz "linux")
Konrad Rzeszutek Wilk
2012-Jul-03 17:36 UTC
Re: [PATCH 4/4] xen/hvc: Fix up checks when the info is allocated.
On Tue, Jul 03, 2012 at 05:11:18PM +0100, Jan Beulich wrote:> >>> On 03.07.12 at 17:40, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > > Coverity would complain about this - even thought it looks OK. > > > > CID 401957 > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > --- > > drivers/tty/hvc/hvc_xen.c | 15 ++++++--------- > > 1 files changed, 6 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c > > index 944eaeb..fbf41be 100644 > > --- a/drivers/tty/hvc/hvc_xen.c > > +++ b/drivers/tty/hvc/hvc_xen.c > > @@ -209,10 +209,8 @@ static int xen_hvm_console_init(void) > > info = kzalloc(sizeof(struct xencons_info), GFP_KERNEL | __GFP_ZERO); > > if (!info) > > return -ENOMEM; > > - } > > - > > - /* already configured */ > > - if (info->intf != NULL) > > + } else if (info->intf != NULL) { > > + /* already configured */ > > return 0; > > Is this patch perhaps stale? There appears to be a missing } here, > without which I can''t see how this would have built, ...<sighs> Yes. That is what I get for cherry-picking and in a hurry picking the options.> > > /* > > * If the toolstack (or the hypervisor) hasn''t set these values, the > > @@ -220,6 +218,7 @@ static int xen_hvm_console_init(void) > > * theoretically correct values, in practice they never are and they > > * mean that a legacy toolstack hasn''t initialized the pv console correctly. > > */ > > + */ > > ... and this one likely wouldn''t build either.Heh. It certainly does not. The "real" patch has that fix.> > Jan > > > r = hvm_get_parameter(HVM_PARAM_CONSOLE_EVTCHN, &v); > > if (r < 0 || v == 0) > > goto err; > > @@ -259,12 +258,10 @@ static int xen_pv_console_init(void) > > info = kzalloc(sizeof(struct xencons_info), GFP_KERNEL | __GFP_ZERO); > > if (!info) > > return -ENOMEM; > > - } > > - > > - /* already configured */ > > - if (info->intf != NULL) > > + } else if (info->intf != NULL) { > > + /* already configured */ > > return 0; > > - > > + } > > info->evtchn = xen_start_info->console.domU.evtchn; > > info->intf = mfn_to_virt(xen_start_info->console.domU.mfn); > > info->vtermno = HVC_COOKIE; >
Konrad Rzeszutek Wilk
2012-Jul-03 17:38 UTC
Re: [PATCH 2/4] xen/perf: Define .glob for the different hypercalls.
On Tue, Jul 03, 2012 at 05:07:37PM +0100, Jan Beulich wrote:> >>> On 03.07.12 at 17:40, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > > This allows us in perf to have this: > > > > 99.67% [kernel] [k] xen_hypercall_sched_op > > 0.11% [kernel] [k] xen_hypercall_xen_version > > > > instead of the borring ever-encompassing: > > > > 99.13% [kernel] [k] hypercall_page > > > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > --- > > arch/x86/xen/xen-head.S | 102 > > ++++++++++++++++++++++++++++++++++++++++++++++- > > 1 files changed, 100 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S > > index aaa7291..f6ba51d 100644 > > --- a/arch/x86/xen/xen-head.S > > +++ b/arch/x86/xen/xen-head.S > > @@ -28,9 +28,107 @@ ENTRY(startup_xen) > > __FINIT > > > > .pushsection .text > > - .align PAGE_SIZE > > + .balign PAGE_SIZE > > ENTRY(hypercall_page) > > - .skip PAGE_SIZE > > +ENTRY(xen_hypercall_set_trap_table) > > + .skip 32 > > +ENTRY(xen_hypercall_mmu_update) > > + .skip 32 > > +ENTRY(xen_hypercall_set_gdt) > > + .skip 32 > > +ENTRY(xen_hypercall_stack_switch) > > + .skip 32 > > +ENTRY(xen_hypercall_set_callbacks) > > + .skip 32 > > +ENTRY(xen_hypercall_fpu_taskswitch) > > + .skip 32 > > +ENTRY(xen_hypercall_sched_op_compat) > > + .skip 32 > > +ENTRY(xen_hypercall_platform_op) > > + .skip 32 > > +ENTRY(xen_hypercall_set_debugreg) > > + .skip 32 > > +ENTRY(xen_hypercall_get_debugreg) > > + .skip 32 > > +ENTRY(xen_hypercall_update_descriptor) > > + .skip 32 > > +ENTRY(xen_hypercall_ni) > > + .skip 32 > > +ENTRY(xen_hypercall_memory_op) > > + .skip 32 > > +ENTRY(xen_hypercall_multicall) > > + .skip 32 > > +ENTRY(xen_hypercall_update_va_mapping) > > + .skip 32 > > +ENTRY(xen_hypercall_set_timer_op) > > + .skip 32 > > +ENTRY(xen_hypercall_event_channel_op_compat) > > + .skip 32 > > +ENTRY(xen_hypercall_xen_version) > > + .skip 32 > > +ENTRY(xen_hypercall_console_io) > > + .skip 32 > > +ENTRY(xen_hypercall_physdev_op_compat) > > + .skip 32 > > +ENTRY(xen_hypercall_grant_table_op) > > + .skip 32 > > +ENTRY(xen_hypercall_vm_assist) > > + .skip 32 > > +ENTRY(xen_hypercall_update_va_mapping_otherdomain) > > + .skip 32 > > +ENTRY(xen_hypercall_iret) > > + .skip 32 > > +ENTRY(xen_hypercall_vcpu_op) > > + .skip 32 > > +ENTRY(xen_hypercall_set_segment_base) > > + .skip 32 > > +ENTRY(xen_hypercall_mmuext_op) > > + .skip 32 > > +ENTRY(xen_hypercall_xsm_op) > > + .skip 32 > > +ENTRY(xen_hypercall_nmi_op) > > + .skip 32 > > +ENTRY(xen_hypercall_sched_op) > > + .skip 32 > > +ENTRY(xen_hypercall_callback_op) > > + .skip 32 > > +ENTRY(xen_hypercall_xenoprof_op) > > + .skip 32 > > +ENTRY(xen_hypercall_event_channel_op) > > + .skip 32 > > +ENTRY(xen_hypercall_physdev_op) > > + .skip 32 > > +ENTRY(xen_hypercall_hvm_op) > > + .skip 32 > > +ENTRY(xen_hypercall_sysctl) > > + .skip 32 > > +ENTRY(xen_hypercall_domctl) > > + .skip 32 > > +ENTRY(xen_hypercall_kexec_op) > > + .skip 32 > > +ENTRY(xen_hypercall_tmem_op) /* 38 */ > > + .skip 32 > > +ENTRY(xen_hypercall_rsvr) > > + .skip 320 > > +ENTRY(xen_hypercall_mca) /* 48 */ > > + .skip 32 > > +ENTRY(xen_hypercall_arch_1) > > + .skip 32 > > +ENTRY(xen_hypercall_arch_2) > > + .skip 32 > > +ENTRY(xen_hypercall_arch_3) > > + .skip 32 > > +ENTRY(xen_hypercall_arch_4) > > + .skip 32 > > +ENTRY(xen_hypercall_arch_5) > > + .skip 32 > > +ENTRY(xen_hypercall_arch_6) > > + .skip 32 > > +ENTRY(xen_hypercall_arch_7) > > + .skip 32 > > +ENTRY(xen_hypercall_other) > > + .skip 2272 > > May I suggest that you use > > .balign PAGE_SIZE > > here again, avoiding the need to adjust the number with every > future addition at the end?<nods> Sure thing.> > Jan > > > + > > .popsection > > > > ELFNOTE(Xen, XEN_ELFNOTE_GUEST_OS, .asciz "linux") > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
Konrad Rzeszutek Wilk
2012-Jul-03 17:44 UTC
Re: [Xen-devel] [PATCH 2/4] xen/perf: Define .glob for the different hypercalls.
On Tue, Jul 03, 2012 at 05:17:04PM +0100, Ian Campbell wrote:> On Tue, 2012-07-03 at 16:40 +0100, Konrad Rzeszutek Wilk wrote: > > This allows us in perf to have this: > > > > 99.67% [kernel] [k] xen_hypercall_sched_op > > 0.11% [kernel] [k] xen_hypercall_xen_version > > > > instead of the borring ever-encompassing: > > > > 99.13% [kernel] [k] hypercall_page > > > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > I don''t suppose it would be possible to automate this somehow would it? > Perhaps a small perl snippet consuming the output of > grep ^\#define.__HYPERVISOR_ include/xen/interface/xen.h > ?Another way would be to do: define NEXT_HYPERCALL(name) \ ENTRY(xen_hypercall_#name) \ .skip 32 And then NEXT_HYPERCALL(set_trap_table); NEXT_HYPERCALL(mmu_update); .. and so on. And also use the .balign PAGE_SIZE at the end to pad it. With the grep thingy I think we would also hit the large space between hypercall #38 and #48 - which has no calls.> > > --- > > arch/x86/xen/xen-head.S | 102 ++++++++++++++++++++++++++++++++++++++++++++++- > > 1 files changed, 100 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S > > index aaa7291..f6ba51d 100644 > > --- a/arch/x86/xen/xen-head.S > > +++ b/arch/x86/xen/xen-head.S > > @@ -28,9 +28,107 @@ ENTRY(startup_xen) > > __FINIT > > > > .pushsection .text > > - .align PAGE_SIZE > > + .balign PAGE_SIZE > > ENTRY(hypercall_page) > > - .skip PAGE_SIZE > > +ENTRY(xen_hypercall_set_trap_table) > > + .skip 32 > > +ENTRY(xen_hypercall_mmu_update) > > + .skip 32 > > +ENTRY(xen_hypercall_set_gdt) > > + .skip 32 > > +ENTRY(xen_hypercall_stack_switch) > > + .skip 32 > > +ENTRY(xen_hypercall_set_callbacks) > > + .skip 32 > > +ENTRY(xen_hypercall_fpu_taskswitch) > > + .skip 32 > > +ENTRY(xen_hypercall_sched_op_compat) > > + .skip 32 > > +ENTRY(xen_hypercall_platform_op) > > + .skip 32 > > +ENTRY(xen_hypercall_set_debugreg) > > + .skip 32 > > +ENTRY(xen_hypercall_get_debugreg) > > + .skip 32 > > +ENTRY(xen_hypercall_update_descriptor) > > + .skip 32 > > +ENTRY(xen_hypercall_ni) > > + .skip 32 > > +ENTRY(xen_hypercall_memory_op) > > + .skip 32 > > +ENTRY(xen_hypercall_multicall) > > + .skip 32 > > +ENTRY(xen_hypercall_update_va_mapping) > > + .skip 32 > > +ENTRY(xen_hypercall_set_timer_op) > > + .skip 32 > > +ENTRY(xen_hypercall_event_channel_op_compat) > > + .skip 32 > > +ENTRY(xen_hypercall_xen_version) > > + .skip 32 > > +ENTRY(xen_hypercall_console_io) > > + .skip 32 > > +ENTRY(xen_hypercall_physdev_op_compat) > > + .skip 32 > > +ENTRY(xen_hypercall_grant_table_op) > > + .skip 32 > > +ENTRY(xen_hypercall_vm_assist) > > + .skip 32 > > +ENTRY(xen_hypercall_update_va_mapping_otherdomain) > > + .skip 32 > > +ENTRY(xen_hypercall_iret) > > + .skip 32 > > +ENTRY(xen_hypercall_vcpu_op) > > + .skip 32 > > +ENTRY(xen_hypercall_set_segment_base) > > + .skip 32 > > +ENTRY(xen_hypercall_mmuext_op) > > + .skip 32 > > +ENTRY(xen_hypercall_xsm_op) > > + .skip 32 > > +ENTRY(xen_hypercall_nmi_op) > > + .skip 32 > > +ENTRY(xen_hypercall_sched_op) > > + .skip 32 > > +ENTRY(xen_hypercall_callback_op) > > + .skip 32 > > +ENTRY(xen_hypercall_xenoprof_op) > > + .skip 32 > > +ENTRY(xen_hypercall_event_channel_op) > > + .skip 32 > > +ENTRY(xen_hypercall_physdev_op) > > + .skip 32 > > +ENTRY(xen_hypercall_hvm_op) > > + .skip 32 > > +ENTRY(xen_hypercall_sysctl) > > + .skip 32 > > +ENTRY(xen_hypercall_domctl) > > + .skip 32 > > +ENTRY(xen_hypercall_kexec_op) > > + .skip 32 > > +ENTRY(xen_hypercall_tmem_op) /* 38 */ > > + .skip 32 > > +ENTRY(xen_hypercall_rsvr) > > + .skip 320 > > +ENTRY(xen_hypercall_mca) /* 48 */ > > + .skip 32 > > +ENTRY(xen_hypercall_arch_1) > > + .skip 32 > > +ENTRY(xen_hypercall_arch_2) > > + .skip 32 > > +ENTRY(xen_hypercall_arch_3) > > + .skip 32 > > +ENTRY(xen_hypercall_arch_4) > > + .skip 32 > > +ENTRY(xen_hypercall_arch_5) > > + .skip 32 > > +ENTRY(xen_hypercall_arch_6) > > + .skip 32 > > +ENTRY(xen_hypercall_arch_7) > > + .skip 32 > > +ENTRY(xen_hypercall_other) > > + .skip 2272 > > + > > .popsection > > > > ELFNOTE(Xen, XEN_ELFNOTE_GUEST_OS, .asciz "linux") >
David Vrabel
2012-Jul-09 15:41 UTC
Re: [Xen-devel] [PATCH 1/4] xen/p2m: Optimize the get_phys_to_machine function a bit.
On 03/07/12 16:40, Konrad Rzeszutek Wilk wrote:> Running perf tells me that the check for p2m_identity > is done a lot - more than it should.This patch doesn''t change the how often the check is done. Is this just bad wording of the description?> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > arch/x86/xen/p2m.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c > index 64effdc..161bcdc 100644 > --- a/arch/x86/xen/p2m.c > +++ b/arch/x86/xen/p2m.c > @@ -406,7 +406,7 @@ unsigned long get_phys_to_machine(unsigned long pfn) > * and in p2m_*missing, so returning the INVALID_P2M_ENTRY > * would be wrong. > */ > - if (p2m_top[topidx][mididx] == p2m_identity) > + if (unlikely(p2m_top[topidx][mididx] == p2m_identity)) > return IDENTITY_FRAME(pfn); > > return p2m_top[topidx][mididx][idx];
Konrad Rzeszutek Wilk
2012-Jul-09 16:14 UTC
Re: [Xen-devel] [PATCH 1/4] xen/p2m: Optimize the get_phys_to_machine function a bit.
On Mon, Jul 09, 2012 at 04:41:24PM +0100, David Vrabel wrote:> On 03/07/12 16:40, Konrad Rzeszutek Wilk wrote: > > Running perf tells me that the check for p2m_identity > > is done a lot - more than it should. > > This patch doesn''t change the how often the check is done. Is this just > bad wording of the description?Its bad wording plus it really does not change the assmebler code - I thought it did but since there is only one "if" case it does not matter how much I put unlikely in front of it - it is the only branch condition. Perhaps if I make the code be put in a variable (register) that would optimize this a bit more...> > > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > --- > > arch/x86/xen/p2m.c | 2 +- > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c > > index 64effdc..161bcdc 100644 > > --- a/arch/x86/xen/p2m.c > > +++ b/arch/x86/xen/p2m.c > > @@ -406,7 +406,7 @@ unsigned long get_phys_to_machine(unsigned long pfn) > > * and in p2m_*missing, so returning the INVALID_P2M_ENTRY > > * would be wrong. > > */ > > - if (p2m_top[topidx][mididx] == p2m_identity) > > + if (unlikely(p2m_top[topidx][mididx] == p2m_identity)) > > return IDENTITY_FRAME(pfn); > > > > return p2m_top[topidx][mididx][idx];