Re: [PATCH v2 1/4] powerpc/pseries/vas: Define global hv_cop_caps struct

2022-02-08 Thread Haren Myneni 
On Tue, 2022-02-08 at 09:48 -0600, Nathan Lynch wrote:
> Haren Myneni  writes:
> > The coprocessor capabilities struct is used to get default and
> > QoS capabilities from the hypervisor during init, DLPAR event and
> > migration. So instead of allocating this struct for each event,
> > define global struct and reuse it, especially eliminate memory
> > allocation failure during migration.
> 
> Which allows the migration code to avoid adding an error path. I
> could
> go either way, but this approach seems fine to me assuming all users
> of
> the global object are guarded by an appropriate lock.
> 
> Acked-by: Nathan Lynch 
> 

Thanks for your suggestion. I will change the commit message as you
suggested to make it clear. yes, this struct is accessed with mutex. 

Thanks
Haren

Re: [PATCH v2 1/4] powerpc/pseries/vas: Define global hv_cop_caps struct

2022-02-08 Thread Nathan Lynch 
Haren Myneni  writes:
> The coprocessor capabilities struct is used to get default and
> QoS capabilities from the hypervisor during init, DLPAR event and
> migration. So instead of allocating this struct for each event,
> define global struct and reuse it, especially eliminate memory
> allocation failure during migration.

Which allows the migration code to avoid adding an error path. I could
go either way, but this approach seems fine to me assuming all users of
the global object are guarded by an appropriate lock.

Acked-by: Nathan Lynch

[PATCH v2 1/4] powerpc/pseries/vas: Define global hv_cop_caps struct

2022-02-05 Thread Haren Myneni 


The coprocessor capabilities struct is used to get default and
QoS capabilities from the hypervisor during init, DLPAR event and
migration. So instead of allocating this struct for each event,
define global struct and reuse it, especially eliminate memory
allocation failure during migration.

Also disable copy/paste feature flag if any capabilities HCALL
is failed.

Signed-off-by: Haren Myneni 
---
 arch/powerpc/platforms/pseries/vas.c | 49 
 1 file changed, 21 insertions(+), 28 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/vas.c 
b/arch/powerpc/platforms/pseries/vas.c
index 3400f4fc6609..933506b85dc6 100644
--- a/arch/powerpc/platforms/pseries/vas.c
+++ b/arch/powerpc/platforms/pseries/vas.c
@@ -26,6 +26,7 @@
 
 static struct vas_all_caps caps_all;
 static bool copypaste_feat;
+static struct hv_vas_cop_feat_caps hv_cop_caps;
 
 static struct vas_caps vascaps[VAS_MAX_FEAT_TYPE];
 static DEFINE_MUTEX(vas_pseries_mutex);
@@ -724,7 +725,6 @@ static int reconfig_close_windows(struct vas_caps *vcap, 
int excess_creds)
  */
 int vas_reconfig_capabilties(u8 type)
 {
-   struct hv_vas_cop_feat_caps *hv_caps;
struct vas_cop_feat_caps *caps;
int lpar_creds, new_creds;
struct vas_caps *vcaps;
@@ -738,17 +738,13 @@ int vas_reconfig_capabilties(u8 type)
vcaps = [type];
caps = >caps;
 
-   hv_caps = kmalloc(sizeof(*hv_caps), GFP_KERNEL);
-   if (!hv_caps)
-   return -ENOMEM;
-
mutex_lock(_pseries_mutex);
rc = h_query_vas_capabilities(H_QUERY_VAS_CAPABILITIES, vcaps->feat,
- (u64)virt_to_phys(hv_caps));
+ (u64)virt_to_phys(_cop_caps));
if (rc)
goto out;
 
-   new_creds = be16_to_cpu(hv_caps->target_lpar_creds);
+   new_creds = be16_to_cpu(hv_cop_caps.target_lpar_creds);
 
lpar_creds = atomic_read(>target_creds);
 
@@ -778,7 +774,6 @@ int vas_reconfig_capabilties(u8 type)
 
 out:
mutex_unlock(_pseries_mutex);
-   kfree(hv_caps);
return rc;
 }
 
@@ -821,9 +816,8 @@ static struct notifier_block pseries_vas_nb = {
 
 static int __init pseries_vas_init(void)
 {
-   struct hv_vas_cop_feat_caps *hv_cop_caps;
struct hv_vas_all_caps *hv_caps;
-   int rc;
+   int rc = 0;
 
/*
 * Linux supports user space COPY/PASTE only with Radix
@@ -849,39 +843,38 @@ static int __init pseries_vas_init(void)
 
sysfs_pseries_vas_init(_all);
 
-   hv_cop_caps = kmalloc(sizeof(*hv_cop_caps), GFP_KERNEL);
-   if (!hv_cop_caps) {
-   rc = -ENOMEM;
-   goto out;
-   }
/*
 * QOS capabilities available
 */
if (caps_all.feat_type & VAS_GZIP_QOS_FEAT_BIT) {
rc = get_vas_capabilities(VAS_GZIP_QOS_FEAT,
- VAS_GZIP_QOS_FEAT_TYPE, hv_cop_caps);
+ VAS_GZIP_QOS_FEAT_TYPE, _cop_caps);
 
if (rc)
-   goto out_cop;
+   goto out;
}
/*
 * Default capabilities available
 */
-   if (caps_all.feat_type & VAS_GZIP_DEF_FEAT_BIT) {
+   if (caps_all.feat_type & VAS_GZIP_DEF_FEAT_BIT)
rc = get_vas_capabilities(VAS_GZIP_DEF_FEAT,
- VAS_GZIP_DEF_FEAT_TYPE, hv_cop_caps);
-   if (rc)
-   goto out_cop;
-   }
+ VAS_GZIP_DEF_FEAT_TYPE, _cop_caps);
 
-   /* Processors can be added/removed only on LPAR */
-   if (copypaste_feat && firmware_has_feature(FW_FEATURE_LPAR))
-   of_reconfig_notifier_register(_vas_nb);
+   if (!rc && copypaste_feat) {
+   /* Processors can be added/removed only on LPAR */
+   if (firmware_has_feature(FW_FEATURE_LPAR))
+   of_reconfig_notifier_register(_vas_nb);
 
-   pr_info("GZIP feature is available\n");
+   pr_info("GZIP feature is available\n");
+   } else {
+   /*
+* Should not happen, but only when get default
+* capabilities HCALL failed. So disable copy paste
+* feature.
+*/
+   copypaste_feat = false;
+   }
 
-out_cop:
-   kfree(hv_cop_caps);
 out:
kfree(hv_caps);
return rc;
-- 
2.27.0