[PATCH v2 0/4] Unisolate LMBs DRC on removal error + cleanups

[PATCH v2 0/4] Unisolate LMBs DRC on removal error + cleanups

[Daniel Henrique Barboza]

changes from v1:
- patch 1: added David's r-b
- patch 2:
    * removed the DRCONF_MEM_RESERVED assumption for
    dlpar_memory_remove_by_ic()
    * reworded the commit msg
- patch 3: dropped. the differences between dlpar_memory_remove_by_ic()
  and dlpar_memory_remove_by_count() makes a helper function too complex
  to handle both cases
- (new) patch 3 and 4: minor enhancements

v1 link: https://lore.kernel.org/linuxppc-dev/20210430120917.217951-1-danielhb413@gmail.com/

Daniel Henrique Barboza (4):
  powerpc/pseries: Set UNISOLATE on dlpar_memory_remove_by_ic() error
  powerpc/pseries: check DRCONF_MEM_RESERVED in lmb_is_removable()
  powerpc/pseries: break early in dlpar_memory_remove_by_count() loops
  powerpc/pseries: minor enhancements in dlpar_memory_remove_by_ic()

 .../platforms/pseries/hotplug-memory.c        | 54 ++++++++++++++-----
 1 file changed, 40 insertions(+), 14 deletions(-)

-- 
2.31.1

[PATCH v2 1/4] powerpc/pseries: Set UNISOLATE on dlpar_memory_remove_by_ic() error

[Daniel Henrique Barboza]

As previously done in dlpar_cpu_remove() for CPUs, this patch changes
dlpar_memory_remove_by_ic() to unisolate the LMB DRC when the LMB is
failed to be removed. The hypervisor, seeing a LMB DRC that was supposed
to be removed being unisolated instead, can do error recovery on its
side.

This change is done in dlpar_memory_remove_by_ic() only because, as of
today, only QEMU is using this code path for error recovery (via the
PSERIES_HP_ELOG_ID_DRC_IC event). phyp treats it as a no-op.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 arch/powerpc/platforms/pseries/hotplug-memory.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 8377f1f7c78e..bb98574a84a2 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -551,6 +551,13 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
 			if (!drmem_lmb_reserved(lmb))
 				continue;
 
+			/*
+			 * Setting the isolation state of an UNISOLATED/CONFIGURED
+			 * device to UNISOLATE is a no-op, but the hypervisor can
+			 * use it as a hint that the LMB removal failed.
+			 */
+			dlpar_unisolate_drc(lmb->drc_index);
+
 			rc = dlpar_add_lmb(lmb);
 			if (rc)
 				pr_err("Failed to add LMB, drc index %x\n",
-- 
2.31.1

[PATCH v2 2/4] powerpc/pseries: check DRCONF_MEM_RESERVED in lmb_is_removable()

[Daniel Henrique Barboza]

DRCONF_MEM_RESERVED is a flag that represents the "Reserved Memory"
status in LOPAR v2.10, section 4.2.8. If a LMB is marked as reserved,
quoting LOPAR, "is not to be used or altered by the base OS". This flag
is read only in the kernel, being set by the firmware/hypervisor in the
DT. As an example, QEMU will set this flag in hw/ppc/spapr.c,
spapr_dt_dynamic_memory().

lmb_is_removable() does not check for DRCONF_MEM_RESERVED. This function
is used in dlpar_remove_lmb() as a guard before the removal logic. Since
it is failing to check for !RESERVED, dlpar_remove_lmb() will fail in a
later stage instead of failing in the validation when receiving a
reserved LMB as input.

lmb_is_removable() is also used in dlpar_memory_remove_by_count() to
evaluate if we have enough LMBs to complete the request. The missing
!RESERVED check in this case is causing dlpar_memory_remove_by_count()
to miscalculate the number of elegible LMBs for the removal, and can
make it error out later on instead of failing in the validation with the
'not enough LMBs to satisfy request' message.

Making a DRCONF_MEM_RESERVED check in lmb_is_removable() fixes all these
issues.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 arch/powerpc/platforms/pseries/hotplug-memory.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index bb98574a84a2..c21d9278c1ce 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -348,7 +348,8 @@ static int pseries_remove_mem_node(struct device_node *np)
 
 static bool lmb_is_removable(struct drmem_lmb *lmb)
 {
-	if (!(lmb->flags & DRCONF_MEM_ASSIGNED))
+	if ((lmb->flags & DRCONF_MEM_RESERVED) ||
+		!(lmb->flags & DRCONF_MEM_ASSIGNED))
 		return false;
 
 #ifdef CONFIG_FA_DUMP
-- 
2.31.1

Re: [PATCH v2 2/4] powerpc/pseries: check DRCONF_MEM_RESERVED in lmb_is_removable()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2355 bytes --]

On Wed, May 12, 2021 at 05:28:07PM -0300, Daniel Henrique Barboza wrote:
> DRCONF_MEM_RESERVED is a flag that represents the "Reserved Memory"
> status in LOPAR v2.10, section 4.2.8. If a LMB is marked as reserved,
> quoting LOPAR, "is not to be used or altered by the base OS". This flag
> is read only in the kernel, being set by the firmware/hypervisor in the
> DT. As an example, QEMU will set this flag in hw/ppc/spapr.c,
> spapr_dt_dynamic_memory().
> 
> lmb_is_removable() does not check for DRCONF_MEM_RESERVED. This function
> is used in dlpar_remove_lmb() as a guard before the removal logic. Since
> it is failing to check for !RESERVED, dlpar_remove_lmb() will fail in a
> later stage instead of failing in the validation when receiving a
> reserved LMB as input.
> 
> lmb_is_removable() is also used in dlpar_memory_remove_by_count() to
> evaluate if we have enough LMBs to complete the request. The missing
> !RESERVED check in this case is causing dlpar_memory_remove_by_count()
> to miscalculate the number of elegible LMBs for the removal, and can
> make it error out later on instead of failing in the validation with the
> 'not enough LMBs to satisfy request' message.
> 
> Making a DRCONF_MEM_RESERVED check in lmb_is_removable() fixes all these
> issues.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  arch/powerpc/platforms/pseries/hotplug-memory.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index bb98574a84a2..c21d9278c1ce 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -348,7 +348,8 @@ static int pseries_remove_mem_node(struct device_node *np)
>  
>  static bool lmb_is_removable(struct drmem_lmb *lmb)
>  {
> -	if (!(lmb->flags & DRCONF_MEM_ASSIGNED))
> +	if ((lmb->flags & DRCONF_MEM_RESERVED) ||
> +		!(lmb->flags & DRCONF_MEM_ASSIGNED))
>  		return false;
>  
>  #ifdef CONFIG_FA_DUMP

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 3/4] powerpc/pseries: break early in dlpar_memory_remove_by_count() loops

[Daniel Henrique Barboza]

After marking the LMBs as reserved depending on dlpar_remove_lmb() rc,
we evaluate whether we need to add the LMBs back or if we can release
the LMB DRCs. In both cases, a for_each_drmem_lmb() loop without a break
condition is used. This means that we're going to cycle through all LMBs
of the partition even after we're done with what we were going to do.

This patch adds break conditions in both loops to avoid this. The
'lmbs_removed' variable was renamed to 'lmbs_reserved', and it's now
being decremented each time a lmb reservation is removed, indicating
that the operation we're doing (adding back LMBs or releasing DRCs) is
completed.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 arch/powerpc/platforms/pseries/hotplug-memory.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index c21d9278c1ce..3c7ce5361ce3 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -402,7 +402,7 @@ static int dlpar_remove_lmb(struct drmem_lmb *lmb)
 static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
 {
 	struct drmem_lmb *lmb;
-	int lmbs_removed = 0;
+	int lmbs_reserved = 0;
 	int lmbs_available = 0;
 	int rc;
 
@@ -436,12 +436,12 @@ static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
 		 */
 		drmem_mark_lmb_reserved(lmb);
 
-		lmbs_removed++;
-		if (lmbs_removed == lmbs_to_remove)
+		lmbs_reserved++;
+		if (lmbs_reserved == lmbs_to_remove)
 			break;
 	}
 
-	if (lmbs_removed != lmbs_to_remove) {
+	if (lmbs_reserved != lmbs_to_remove) {
 		pr_err("Memory hot-remove failed, adding LMB's back\n");
 
 		for_each_drmem_lmb(lmb) {
@@ -454,6 +454,10 @@ static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
 				       lmb->drc_index);
 
 			drmem_remove_lmb_reservation(lmb);
+
+			lmbs_reserved--;
+			if (lmbs_reserved == 0)
+				break;
 		}
 
 		rc = -EINVAL;
@@ -467,6 +471,10 @@ static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
 				lmb->base_addr);
 
 			drmem_remove_lmb_reservation(lmb);
+
+			lmbs_reserved--;
+			if (lmbs_reserved == 0)
+				break;
 		}
 		rc = 0;
 	}
-- 
2.31.1

Re: [PATCH v2 3/4] powerpc/pseries: break early in dlpar_memory_remove_by_count() loops

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2922 bytes --]

On Wed, May 12, 2021 at 05:28:08PM -0300, Daniel Henrique Barboza wrote:
> After marking the LMBs as reserved depending on dlpar_remove_lmb() rc,
> we evaluate whether we need to add the LMBs back or if we can release
> the LMB DRCs. In both cases, a for_each_drmem_lmb() loop without a break
> condition is used. This means that we're going to cycle through all LMBs
> of the partition even after we're done with what we were going to do.
> 
> This patch adds break conditions in both loops to avoid this. The
> 'lmbs_removed' variable was renamed to 'lmbs_reserved', and it's now
> being decremented each time a lmb reservation is removed, indicating
> that the operation we're doing (adding back LMBs or releasing DRCs) is
> completed.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

The fact that DRCONF_MEM_RESERVED and DRMEM_LMB_RESERVED look so
similar but have totally different meanings doesn't make this easy to
follow :/.

> ---
>  arch/powerpc/platforms/pseries/hotplug-memory.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index c21d9278c1ce..3c7ce5361ce3 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -402,7 +402,7 @@ static int dlpar_remove_lmb(struct drmem_lmb *lmb)
>  static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
>  {
>  	struct drmem_lmb *lmb;
> -	int lmbs_removed = 0;
> +	int lmbs_reserved = 0;
>  	int lmbs_available = 0;
>  	int rc;
>  
> @@ -436,12 +436,12 @@ static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
>  		 */
>  		drmem_mark_lmb_reserved(lmb);
>  
> -		lmbs_removed++;
> -		if (lmbs_removed == lmbs_to_remove)
> +		lmbs_reserved++;
> +		if (lmbs_reserved == lmbs_to_remove)
>  			break;
>  	}
>  
> -	if (lmbs_removed != lmbs_to_remove) {
> +	if (lmbs_reserved != lmbs_to_remove) {
>  		pr_err("Memory hot-remove failed, adding LMB's back\n");
>  
>  		for_each_drmem_lmb(lmb) {
> @@ -454,6 +454,10 @@ static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
>  				       lmb->drc_index);
>  
>  			drmem_remove_lmb_reservation(lmb);
> +
> +			lmbs_reserved--;
> +			if (lmbs_reserved == 0)
> +				break;
>  		}
>  
>  		rc = -EINVAL;
> @@ -467,6 +471,10 @@ static int dlpar_memory_remove_by_count(u32 lmbs_to_remove)
>  				lmb->base_addr);
>  
>  			drmem_remove_lmb_reservation(lmb);
> +
> +			lmbs_reserved--;
> +			if (lmbs_reserved == 0)
> +				break;
>  		}
>  		rc = 0;
>  	}

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 4/4] powerpc/pseries: minor enhancements in dlpar_memory_remove_by_ic()

[Daniel Henrique Barboza]

We don't need the 'lmbs_available' variable to count the valid LMBs and
to check if we have less than 'lmbs_to_remove'. We must ensure that the
entire LMB range must be removed, so we can error out immediately if any
LMB in the range is marked as reserved.

Add a couple of comments explaining the reasoning behind the differences
we have in this function in contrast to what it is done in its sister
function, dlpar_memory_remove_by_count().

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 .../platforms/pseries/hotplug-memory.c        | 28 +++++++++++++------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 3c7ce5361ce3..ee88c1540fba 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -517,7 +517,6 @@ static int dlpar_memory_remove_by_index(u32 drc_index)
 static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
 {
 	struct drmem_lmb *lmb, *start_lmb, *end_lmb;
-	int lmbs_available = 0;
 	int rc;
 
 	pr_info("Attempting to hot-remove %u LMB(s) at %x\n",
@@ -530,18 +529,29 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
 	if (rc)
 		return -EINVAL;
 
-	/* Validate that there are enough LMBs to satisfy the request */
+	/*
+	 * Validate that all LMBs in range are not reserved. Note that it
+	 * is ok if they are !ASSIGNED since our goal here is to remove the
+	 * LMB range, regardless of whether some LMBs were already removed
+	 * by any other reason.
+	 *
+	 * This is a contrast to what is done in remove_by_count() where we
+	 * check for both RESERVED and !ASSIGNED (via lmb_is_removable()),
+	 * because we want to remove a fixed amount of LMBs in that function.
+	 */
 	for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
-		if (lmb->flags & DRCONF_MEM_RESERVED)
-			break;
-
-		lmbs_available++;
+		if (lmb->flags & DRCONF_MEM_RESERVED) {
+			pr_err("Memory at %llx (drc index %x) is reserved\n",
+				lmb->base_addr, lmb->drc_index);
+			return -EINVAL;
+		}
 	}
 
-	if (lmbs_available < lmbs_to_remove)
-		return -EINVAL;
-
 	for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
+		/*
+		 * dlpar_remove_lmb() will error out if the LMB is already
+		 * !ASSIGNED, but this case is a no-op for us.
+		 */
 		if (!(lmb->flags & DRCONF_MEM_ASSIGNED))
 			continue;
 
-- 
2.31.1

Re: [PATCH v2 4/4] powerpc/pseries: minor enhancements in dlpar_memory_remove_by_ic()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2964 bytes --]

On Wed, May 12, 2021 at 05:28:09PM -0300, Daniel Henrique Barboza wrote:
> We don't need the 'lmbs_available' variable to count the valid LMBs and
> to check if we have less than 'lmbs_to_remove'. We must ensure that the
> entire LMB range must be removed, so we can error out immediately if any
> LMB in the range is marked as reserved.
> 
> Add a couple of comments explaining the reasoning behind the differences
> we have in this function in contrast to what it is done in its sister
> function, dlpar_memory_remove_by_count().
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  .../platforms/pseries/hotplug-memory.c        | 28 +++++++++++++------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index 3c7ce5361ce3..ee88c1540fba 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -517,7 +517,6 @@ static int dlpar_memory_remove_by_index(u32 drc_index)
>  static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
>  {
>  	struct drmem_lmb *lmb, *start_lmb, *end_lmb;
> -	int lmbs_available = 0;
>  	int rc;
>  
>  	pr_info("Attempting to hot-remove %u LMB(s) at %x\n",
> @@ -530,18 +529,29 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
>  	if (rc)
>  		return -EINVAL;
>  
> -	/* Validate that there are enough LMBs to satisfy the request */
> +	/*
> +	 * Validate that all LMBs in range are not reserved. Note that it
> +	 * is ok if they are !ASSIGNED since our goal here is to remove the
> +	 * LMB range, regardless of whether some LMBs were already removed
> +	 * by any other reason.
> +	 *
> +	 * This is a contrast to what is done in remove_by_count() where we
> +	 * check for both RESERVED and !ASSIGNED (via lmb_is_removable()),
> +	 * because we want to remove a fixed amount of LMBs in that function.
> +	 */
>  	for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
> -		if (lmb->flags & DRCONF_MEM_RESERVED)
> -			break;
> -
> -		lmbs_available++;
> +		if (lmb->flags & DRCONF_MEM_RESERVED) {
> +			pr_err("Memory at %llx (drc index %x) is reserved\n",
> +				lmb->base_addr, lmb->drc_index);
> +			return -EINVAL;
> +		}
>  	}
>  
> -	if (lmbs_available < lmbs_to_remove)
> -		return -EINVAL;
> -
>  	for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
> +		/*
> +		 * dlpar_remove_lmb() will error out if the LMB is already
> +		 * !ASSIGNED, but this case is a no-op for us.
> +		 */
>  		if (!(lmb->flags & DRCONF_MEM_ASSIGNED))
>  			continue;
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/4] Unisolate LMBs DRC on removal error + cleanups

[Michael Ellerman]

On Wed, 12 May 2021 17:28:05 -0300, Daniel Henrique Barboza wrote:
> changes from v1:
> - patch 1: added David's r-b
> - patch 2:
>     * removed the DRCONF_MEM_RESERVED assumption for
>     dlpar_memory_remove_by_ic()
>     * reworded the commit msg
> - patch 3: dropped. the differences between dlpar_memory_remove_by_ic()
>   and dlpar_memory_remove_by_count() makes a helper function too complex
>   to handle both cases
> - (new) patch 3 and 4: minor enhancements
> 
> [...]

Applied to powerpc/next.

[1/4] powerpc/pseries: Set UNISOLATE on dlpar_memory_remove_by_ic() error
      https://git.kernel.org/powerpc/c/feb0e079f43dee055701c1a294785d37341d6f97
[2/4] powerpc/pseries: check DRCONF_MEM_RESERVED in lmb_is_removable()
      https://git.kernel.org/powerpc/c/2ad216b4d6ff0f92fc645c1bd8838f04fbf09b9d
[3/4] powerpc/pseries: break early in dlpar_memory_remove_by_count() loops
      https://git.kernel.org/powerpc/c/163e7921750f6cd965666f472c21de056c63dcbc
[4/4] powerpc/pseries: minor enhancements in dlpar_memory_remove_by_ic()
      https://git.kernel.org/powerpc/c/40999b041e03b32434b2f4a951668e9865a3cb6b

cheers