[PATCH 1/1] spapr/rtas: Add MinMem to ibm, get-system-parameter RTAS call

[PATCH 1/1] spapr/rtas: Add MinMem to ibm, get-system-parameter RTAS call

[Leonardo Bras]

Add support for MinMem SPLPAR Characteristic on emulated
RTAS call ibm,get-system-parameter.

MinMem represents Minimum Memory, that is described in LOPAPR as:
The minimum amount of main store that is needed to power on the
partition. Minimum memory is expressed in MB of storage.

This  provides a way for the OS to discern hotplugged LMBs and
LMBs that have started with the VM, allowing it to better provide
a way for memory hot-removal.

Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com>
---
 hw/ppc/spapr_rtas.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 9fb8c8632a..0f3fbca7af 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -276,10 +276,12 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
 
     switch (parameter) {
     case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: {
-        char *param_val = g_strdup_printf("MaxEntCap=%d,"
+        char *param_val = g_strdup_printf("MinMem=%" PRIu64 ","
+                                          "MaxEntCap=%d,"
                                           "DesMem=%" PRIu64 ","
                                           "DesProcs=%d,"
                                           "MaxPlatProcs=%d",
+                                          ms->ram_size / MiB,
                                           ms->smp.max_cpus,
                                           ms->ram_size / MiB,
                                           ms->smp.cpus,
-- 
2.24.1

Re: [PATCH 1/1] spapr/rtas: Add MinMem to ibm,get-system-parameter RTAS call

[David Gibson]

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

On Fri, Mar 20, 2020 at 09:39:22PM -0300, Leonardo Bras wrote:
> Add support for MinMem SPLPAR Characteristic on emulated
> RTAS call ibm,get-system-parameter.
> 
> MinMem represents Minimum Memory, that is described in LOPAPR as:
> The minimum amount of main store that is needed to power on the
> partition. Minimum memory is expressed in MB of storage.

Where exactly does LoPAPR say that?  The version I'm looking at
doesn't describe MinMem all that clearly, other than to say it must be
<= DesMem, which currently has the same value here.

> This  provides a way for the OS to discern hotplugged LMBs and
> LMBs that have started with the VM, allowing it to better provide
> a way for memory hot-removal.

This seems a bit dubious.  Surely we should have this information from
the dynamic-reconfiguration-memory stuff already?  Trying to discern
this from purely a number seems very fragile - wouldn't that mean
making assumptions about how qemu / the host is laying out it's fixed
and dynamic memory which might not be justified?


> 
> Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com>
> ---
>  hw/ppc/spapr_rtas.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 9fb8c8632a..0f3fbca7af 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -276,10 +276,12 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>  
>      switch (parameter) {
>      case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: {
> -        char *param_val = g_strdup_printf("MaxEntCap=%d,"
> +        char *param_val = g_strdup_printf("MinMem=%" PRIu64 ","
> +                                          "MaxEntCap=%d,"
>                                            "DesMem=%" PRIu64 ","
>                                            "DesProcs=%d,"
>                                            "MaxPlatProcs=%d",
> +                                          ms->ram_size / MiB,
>                                            ms->smp.max_cpus,
>                                            ms->ram_size / MiB,
>                                            ms->smp.cpus,

-- 
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 1/1] spapr/rtas: Add MinMem to ibm,get-system-parameter RTAS call

[Leonardo Bras]

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

On Mon, 2020-03-23 at 14:24 +1100, David Gibson wrote:
> On Fri, Mar 20, 2020 at 09:39:22PM -0300, Leonardo Bras wrote:
> > Add support for MinMem SPLPAR Characteristic on emulated
> > RTAS call ibm,get-system-parameter.
> > 
> > MinMem represents Minimum Memory, that is described in LOPAPR as:
> > The minimum amount of main store that is needed to power on the
> > partition. Minimum memory is expressed in MB of storage.
> 
> Where exactly does LoPAPR say that?  The version I'm looking at
> doesn't describe MinMem all that clearly, other than to say it must be
> <= DesMem, which currently has the same value here.

Please look for "Minimum Memory". It's on Table 237. SPLPAR Terms. 

> > This  provides a way for the OS to discern hotplugged LMBs and
> > LMBs that have started with the VM, allowing it to better provide
> > a way for memory hot-removal.
> 
> This seems a bit dubious.  Surely we should have this information from
> the dynamic-reconfiguration-memory stuff already?  Trying to discern
> this from purely a number seems very fragile - wouldn't that mean
> making assumptions about how qemu / the host is laying out it's fixed
> and dynamic memory which might not be justified?

I agree. 
I previously sent a RFC proposing the usage of a new flag for this same
reason [1], which I thank you for positive feedback.

Even though I think using a flag is a better solution, I am also
working in this other option (MinMem), that would use parameter already
available on the platform, in case the new flag don't get approved.

So far, using MinMem looks like a very complex solution on kernel side,
and I think it's a poor solution.

I decided to send this patch because it's a simple change to the
platform support, that should cause no harm and could even be useful to
other OSes.


Best regards,
Leonardo

[1] http://patchwork.ozlabs.org/patch/1249931/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/1] spapr/rtas: Add MinMem to ibm,get-system-parameter RTAS call

[David Gibson]

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

On Mon, Mar 23, 2020 at 07:07:21PM -0300, Leonardo Bras wrote:
> On Mon, 2020-03-23 at 14:24 +1100, David Gibson wrote:
> > On Fri, Mar 20, 2020 at 09:39:22PM -0300, Leonardo Bras wrote:
> > > Add support for MinMem SPLPAR Characteristic on emulated
> > > RTAS call ibm,get-system-parameter.
> > > 
> > > MinMem represents Minimum Memory, that is described in LOPAPR as:
> > > The minimum amount of main store that is needed to power on the
> > > partition. Minimum memory is expressed in MB of storage.
> > 
> > Where exactly does LoPAPR say that?  The version I'm looking at
> > doesn't describe MinMem all that clearly, other than to say it must be
> > <= DesMem, which currently has the same value here.
> 
> Please look for "Minimum Memory". It's on Table 237. SPLPAR Terms. 

Ah, thanks.

Hm, yes.  In the way we manage VMs with KVM and qemu, I don't think we
cal really draw any meaningful distinction between MinMem and DesMem,
so it's reasonble for them to have the same value.

> > > This  provides a way for the OS to discern hotplugged LMBs and
> > > LMBs that have started with the VM, allowing it to better provide
> > > a way for memory hot-removal.
> > 
> > This seems a bit dubious.  Surely we should have this information from
> > the dynamic-reconfiguration-memory stuff already?  Trying to discern
> > this from purely a number seems very fragile - wouldn't that mean
> > making assumptions about how qemu / the host is laying out it's fixed
> > and dynamic memory which might not be justified?
> 
> I agree. 
> I previously sent a RFC proposing the usage of a new flag for this same
> reason [1], which I thank you for positive feedback.
> 
> Even though I think using a flag is a better solution, I am also
> working in this other option (MinMem), that would use parameter already
> available on the platform, in case the new flag don't get approved.
> 
> So far, using MinMem looks like a very complex solution on kernel side,
> and I think it's a poor solution.
> 
> I decided to send this patch because it's a simple change to the
> platform support, that should cause no harm and could even be useful to
> other OSes.

Hm, ok.

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