Re: [Qemu-devel] [RFC PATCH v0 0/2] Increase max memslots to 512 for PowerPC

[Thomas Huth <thuth@redhat.com>]

On 02.06.2016 06:39, Bharata B Rao wrote:
...
> Agreed. Here is the updated patch:
> 
> spapr: Increase hotpluggable memory slots to 256
> 
> From: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> KVM now supports 512 memslots on PowerPC (earlier it was 32). Allow half
> of it (256) to be used as hotpluggable memory slots.
> 
> Instead of hard coding the max value, use the KVM supplied value if KVM
> is enabled. Otherwise resort to the default value of 32.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c |   15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 44e401a..c82adef 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1816,11 +1816,22 @@ static void ppc_spapr_init(MachineState *machine)
>      /* initialize hotplug memory address space */
>      if (machine->ram_size < machine->maxram_size) {
>          ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
> +        /*
> +         * Number of memslots supported by KVM on PowerPC was increased
> +         * from 32 to 512. Let us limit the number of hotpluggable slots
> +         * to half of that (256). However ensure that number of slots
> +         * doesn't drop below 32 on older hosts.
> +         */

Using "hard-coded" information like "increased to 512" in comments is
true for the current state, but this has a risk of being out of date
soon. Once we change the memslots in the kernel, this comment is not
true anymore and might cause confusion. Better talk about leaving half
of the kernel memslots for PCI and other devices, or so.

> +        int max_memslots = kvm_enabled() ? kvm_get_max_memslots() / 2 :
> +                           SPAPR_MAX_RAM_SLOTS;
>  
> -        if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
> +        if (max_memslots < SPAPR_MAX_RAM_SLOTS) {
> +            max_memslots = SPAPR_MAX_RAM_SLOTS;
> +        }
> +        if (machine->ram_slots > max_memslots) {
>              error_report("Specified number of memory slots %"
>                           PRIu64" exceeds max supported %d",
> -                         machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
> +                         machine->ram_slots, max_memslots);
>              exit(1);
>          }

The changes to the code look fine to me. So once you've updated the
comment, feel free to add my "Reviewed-by".

 Thomas