linux-kernel - Re: [PATCH 1/2] xen/x86: Convert to hotplug state machine

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160817083309.fmkksdchqexaxncz@linutronix.de>
Date:	Wed, 17 Aug 2016 10:33:09 +0200
From:	Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:	Boris Ostrovsky <boris.ostrovsky@...cle.com>
Cc:	david.vrabel@...rix.com, jgross@...e.com,
	xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] xen/x86: Convert to hotplug state machine

On 2016-08-15 10:46:46 [-0400], Boris Ostrovsky wrote:
> Switch to new CPU hotplug infrastructure.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@...cle.com>
> Suggested-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> ---
>  arch/x86/xen/enlighten.c   |  115 +++++++++++++++++++++++++-------------------
>  include/linux/cpuhotplug.h |    2 +
>  2 files changed, 67 insertions(+), 50 deletions(-)
> 
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index c7f6b1f..2283976 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -1541,6 +1543,24 @@ static void __init xen_dom0_set_legacy_features(void)
>  	x86_platform.legacy.rtc = 1;
>  }
>  
> +static int xen_cpuhp_setup(void)
> +{
> +	int rc;
> +
> +	rc = cpuhp_setup_state_nocalls(CPUHP_XEN_PREPARE,
> +				       "XEN_HVM_GUEST_PREPARE",
> +				       xen_cpu_up_prepare, xen_cpu_up_cancel);

The old states UP_CANCEL is different from UP_PREPARE. The latter was
invoked only in the error case while your new callback is always
invoked. From looking at the code you free memory in
xen_cpu_up_cancel() which was allocated in xen_cpu_up_prepare() and
therefore I would name it xen_cpu_dead().
If you do find the time, you might manage to rework the code to avoid
using the _nocalls() function. If see this right, you use
xen_setup_vcpu_info_placement() for the init in the first place. This
uses for_each_possible_cpu macro. The cpuhp_setup_state() function would
perform the init for all CPUs before they come up.

> +	if (!rc) {
> +		rc = cpuhp_setup_state_nocalls(CPUHP_AP_XEN_ONLINE,

If there is no need to run this after KVM's CLK callback please use
CPUHP_AP_ONLINE_DYN. If there is such a need then please document it.

> +					       "XEN_HVM_GUEST_PREPARE",
> +					       xen_cpu_up_online, NULL);
> +		if (rc)
> +			cpuhp_remove_state_nocalls(CPUHP_XEN_PREPARE);
> +	}
> +
> +	return rc;
> +}
> +
>  /* First C function to be called on Xen boot */
>  asmlinkage __visible void __init xen_start_kernel(void)
>  {

Sebastian