Re: [PATCH 2/3] powerpc/pseries/vas: Add VAS migration handler

[Nathan Lynch <nathanl@linux.ibm.com>]

Hi Haren,

Mostly this seems OK to me. Some questions:

Haren Myneni <haren@linux.ibm.com> writes:
> Since the VAS windows belong to the VAS hardware resource, the
> hypervisor expects the partition to close them on source partition
> and reopen them after the partition migrated on the destination
> machine.

Not clear to me what "expects" really means here. Would it be accurate
to say "requires" instead? If the OS fails to close the windows before
suspend, what happens?


> This handler is called before pseries_suspend() to close these
> windows and again invoked after migration. All active windows
> for both default and QoS types will be closed and mark them
> in-active and reopened after migration with this handler.
> During the migration, the user space receives paste instruction
> failure if it issues copy/paste on these in-active windows.

OK, I assume that's tolerable to existing users, is that correct? I.e.
users should already be prepared to incur arbitrary paste instruction
failures.

>
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/mobility.c |  5 ++
>  arch/powerpc/platforms/pseries/vas.c      | 86 +++++++++++++++++++++++
>  arch/powerpc/platforms/pseries/vas.h      |  6 ++
>  3 files changed, 97 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
> index 85033f392c78..70004243e25e 100644
> --- a/arch/powerpc/platforms/pseries/mobility.c
> +++ b/arch/powerpc/platforms/pseries/mobility.c
> @@ -26,6 +26,7 @@
>  #include <asm/machdep.h>
>  #include <asm/rtas.h>
>  #include "pseries.h"
> +#include "vas.h"	/* vas_migration_handler() */
>  #include "../../kernel/cacheinfo.h"
>  
>  static struct kobject *mobility_kobj;
> @@ -669,12 +670,16 @@ static int pseries_migrate_partition(u64 handle)
>  	if (ret)
>  		return ret;
>  
> +	vas_migration_handler(VAS_SUSPEND);
> +

vas_migration_handler() can return an error value. Is that OK to ignore
before going into the suspend?

>  	ret = pseries_suspend(handle);
>  	if (ret == 0)
>  		post_mobility_fixup();
>  	else
>  		pseries_cancel_migration(handle, ret);
>  
> +	vas_migration_handler(VAS_RESUME);
> +

No concerns here though. Nothing to be done about errors encountered in
the resume path.

>  	return ret;
>  }
>  
> diff --git a/arch/powerpc/platforms/pseries/vas.c b/arch/powerpc/platforms/pseries/vas.c
> index e4797fc73553..b53e3fe02971 100644
> --- a/arch/powerpc/platforms/pseries/vas.c
> +++ b/arch/powerpc/platforms/pseries/vas.c
> @@ -873,6 +873,92 @@ static struct notifier_block pseries_vas_nb = {
>  	.notifier_call = pseries_vas_notifier,
>  };
>  
> +/*
> + * For LPM, all windows have to be closed on the source partition
> + * before migration and reopen them on the destination partition
> + * after migration. So closing windows during suspend and
> + * reopen them during resume.
> + */
> +int vas_migration_handler(int action)
> +{
> +	struct hv_vas_cop_feat_caps *hv_caps;
> +	struct vas_cop_feat_caps *caps;
> +	int lpar_creds, new_creds = 0;
> +	struct vas_caps *vcaps;
> +	int i, rc = 0;
> +
> +	hv_caps = kmalloc(sizeof(*hv_caps), GFP_KERNEL);
> +	if (!hv_caps)
> +		return -ENOMEM;
> +
> +	mutex_lock(&vas_pseries_mutex);
> +
> +	for (i = 0; i < VAS_MAX_FEAT_TYPE; i++) {
> +		vcaps = &vascaps[i];
> +		caps = &vcaps->caps;
> +		lpar_creds = atomic_read(&caps->target_creds);
> +
> +		rc = h_query_vas_capabilities(H_QUERY_VAS_CAPABILITIES,
> +					      vcaps->feat,
> +					      (u64)virt_to_phys(hv_caps));
> +		if (!rc) {
> +			new_creds = be16_to_cpu(hv_caps->target_lpar_creds);
> +			/*
> +			 * Should not happen. But incase print messages, close
> +			 * all windows in the list during suspend and reopen
> +			 * windows based on new lpar_creds on the destination
> +			 * system.
> +			 */
> +			if (lpar_creds != new_creds) {
> +				pr_err("state(%d): lpar creds: %d HV lpar creds: %d\n",
> +					action, lpar_creds, new_creds);
> +				pr_err("Used creds: %d, Active creds: %d\n",
> +					atomic_read(&caps->used_creds),
> +					vcaps->num_wins - vcaps->close_wins);
> +			}
> +		} else {
> +			pr_err("state(%d): Get VAS capabilities failed with %d\n",
> +				action, rc);
> +			/*
> +			 * We can not stop migration with the current lpm
> +			 * implementation. So continue closing all windows in
> +			 * the list (during suspend) and return without
> +			 * opening windows (during resume) if VAS capabilities
> +			 * HCALL failed.
> +			 */
> +			if (action == VAS_RESUME)
> +				goto out;
> +		}
> +
> +		switch (action) {
> +		case VAS_SUSPEND:
> +			rc = reconfig_close_windows(vcaps, vcaps->num_wins,
> +							true);
> +			break;
> +		case VAS_RESUME:
> +			atomic_set(&caps->target_creds, new_creds);
> +			rc = reconfig_open_windows(vcaps, new_creds, true);
> +			break;
> +		default:
> +			/* should not happen */
> +			pr_err("Invalid migration action %d\n", action);
> +			rc = -EINVAL;
> +			goto out;
> +		}
> +
> +		/*
> +		 * Ignore errors during suspend and return for resume.
> +		 */
> +		if (rc && (action == VAS_RESUME))
> +			goto out;
> +	}
> +
> +out:
> +	mutex_unlock(&vas_pseries_mutex);
> +	kfree(hv_caps);
> +	return rc;
> +}

The control flow with respect to releasing the allocation and the mutex
looks correct to me. I also verified that H_QUERY_VAS_CAPABILITIES does
not return a busy/retry status, so this code appears to handle all the
architected statuses for that call.