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

[David Gibson <david@gibson.dropbear.id.au>]

[-- 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 --]