[Qemu-devel] [PATCH for-2.11] spapr: Include "pre-plugged" DIMMS in ram size calculation at reset

David Gibson posted 1 patch 6 years, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20171201054125.25749-1-david@gibson.dropbear.id.au
Test checkpatch passed
Test docker passed
Test ppc passed
Test s390x passed
hw/ppc/spapr.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
[Qemu-devel] [PATCH for-2.11] spapr: Include "pre-plugged" DIMMS in ram size calculation at reset
Posted by David Gibson 6 years, 3 months ago
At guest reset time, we allocate a hash page table (HPT) for the guest
based on the guest's RAM size.  If dynamic HPT resizing is not available we
use the maximum RAM size, if it is we use the current RAM size.

But the "current RAM size" calculation is incorrect - we just use the
"base" ram_size from the machine structure.  This doesn't include any
pluggable DIMMs that are already plugged at reset time.

This means that if you try to start a 'pseries' machine with a DIMM
specified on the command line that's much larger than the "base" RAM size,
then the guest will get a woefully inadequate HPT.  This can lead to a
guest freeze during boot as it runs out of HPT space during initial MMU
setup.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a471de6cab..1ac7eb0f8c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1386,7 +1386,10 @@ void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr)
             && !spapr_ovec_test(spapr->ov5_cas, OV5_HPT_RESIZE))) {
         hpt_shift = spapr_hpt_shift_for_ramsize(MACHINE(spapr)->maxram_size);
     } else {
-        hpt_shift = spapr_hpt_shift_for_ramsize(MACHINE(spapr)->ram_size);
+        uint64_t current_ram_size;
+
+        current_ram_size = MACHINE(spapr)->ram_size + get_plugged_memory_size();
+        hpt_shift = spapr_hpt_shift_for_ramsize(current_ram_size);
     }
     spapr_reallocate_hpt(spapr, hpt_shift, &error_fatal);
 
-- 
2.14.3


Re: [Qemu-devel] [PATCH for-2.11] spapr: Include "pre-plugged" DIMMS in ram size calculation at reset
Posted by Greg Kurz 6 years, 3 months ago
On Fri,  1 Dec 2017 16:41:25 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> At guest reset time, we allocate a hash page table (HPT) for the guest
> based on the guest's RAM size.  If dynamic HPT resizing is not available we
> use the maximum RAM size, if it is we use the current RAM size.
> 
> But the "current RAM size" calculation is incorrect - we just use the
> "base" ram_size from the machine structure.  This doesn't include any
> pluggable DIMMs that are already plugged at reset time.
> 
> This means that if you try to start a 'pseries' machine with a DIMM
> specified on the command line that's much larger than the "base" RAM size,
> then the guest will get a woefully inadequate HPT.  This can lead to a
> guest freeze during boot as it runs out of HPT space during initial MMU
> setup.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Yeah I could easily reproduce the issue with:

-m 256M,maxmem=9G,slots=1 \
-object memory-backend-ram,id=ram0,size=8G \
-device pc-dimm,id=dimm0,memdev=ram0

and this patch indeed fixes the issue.

BTW, we already had a similar miscalculation of the current RAM size in
h_resize_hpt_prepare() that was addressed with commit db50f280cf5f7. Maybe
worth consolidating the calculation in some helper ?

Anyway,

Reviewed-by: Greg Kurz <groug@kaod.org>

and

Tested-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a471de6cab..1ac7eb0f8c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1386,7 +1386,10 @@ void spapr_setup_hpt_and_vrma(sPAPRMachineState *spapr)
>              && !spapr_ovec_test(spapr->ov5_cas, OV5_HPT_RESIZE))) {
>          hpt_shift = spapr_hpt_shift_for_ramsize(MACHINE(spapr)->maxram_size);
>      } else {
> -        hpt_shift = spapr_hpt_shift_for_ramsize(MACHINE(spapr)->ram_size);
> +        uint64_t current_ram_size;
> +
> +        current_ram_size = MACHINE(spapr)->ram_size + get_plugged_memory_size();
> +        hpt_shift = spapr_hpt_shift_for_ramsize(current_ram_size);
>      }
>      spapr_reallocate_hpt(spapr, hpt_shift, &error_fatal);
>