Re: [PATCH 2/3] powerpc/pseries/vas: Add VAS migration handler
From: Nathan Lynch <hidden>
Date: 2022-01-31 16:51:49
Hi Haren, Mostly this seems OK to me. Some questions: Haren Myneni [off-list ref] 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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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.