thr3ads.net - Xen devel - [PATCH] various Xen fixes for v3.6 (v1). [Jul 2012]

If this information is useful, please help other people find it:
Share via:

Konrad Rzeszutek Wilk

2012-Jul-03 15:40 UTC

[PATCH] various Xen fixes for v3.6 (v1).

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

head link

[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

head link

[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

head link

[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

head link

[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

head link

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 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?

Jan
> +
>  .popsection
>  
>  	ELFNOTE(Xen, XEN_ELFNOTE_GUEST_OS,       .asciz "linux")

Jan Beulich

2012-Jul-03 16:11 UTC

head link

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

head link

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 2272
I 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

head link

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

head link

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

head link

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

head link

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

head link

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

head link

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];

Reasonably Related Threads

Search for more maybe matching threads

Xen devel - Jul 2012 - [PATCH] various Xen fixes for v3.6 (v1).

[PATCH] various Xen fixes for v3.6 (v1).

[PATCH 1/4] xen/p2m: Optimize the get_phys_to_machine function a bit.

[PATCH 2/4] xen/perf: Define .glob for the different hypercalls.

[PATCH 3/4] xen/acpi: Fix potential memory leak.

[PATCH 4/4] xen/hvc: Fix up checks when the info is allocated.

Re: [PATCH 2/4] xen/perf: Define .glob for the different hypercalls.

Re: [PATCH 4/4] xen/hvc: Fix up checks when the info is allocated.

Re: [Xen-devel] [PATCH 2/4] xen/perf: Define .glob for the different hypercalls.

Re: [Xen-devel] [PATCH 2/4] xen/perf: Define .glob for the different hypercalls.

Re: [PATCH 4/4] xen/hvc: Fix up checks when the info is allocated.

Re: [PATCH 2/4] xen/perf: Define .glob for the different hypercalls.

Re: [Xen-devel] [PATCH 2/4] xen/perf: Define .glob for the different hypercalls.

Re: [Xen-devel] [PATCH 1/4] xen/p2m: Optimize the get_phys_to_machine function a bit.

Re: [Xen-devel] [PATCH 1/4] xen/p2m: Optimize the get_phys_to_machine function a bit.

Reasonably Related Threads