| Message ID | 20171010132159.15787-1-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/10/2017 15:21, David Gibson wrote: > In order to prevent the guest from forcing the allocation of large amounts > of qemu memory (or host kernel memory, in the case of KVM HV), we limit > the size of Hashed Page Table (HPT) it is allowed to allocated, based on > its RAM size. > > However, the current calculation is not correct: it only adds up the size > of plugged memory, ignoring the base memory size. This patch corrects it. > > While we're there, use get_plugged_memory_size() instead of directly > calling pc_existing_dimms_capacity(). The only difference is that it > will abort on failure, which is right: a failure here indicates something > wrong within qemu. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr_hcall.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index 8d72bb7c1c..06af1b15c0 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -494,7 +494,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, > return H_PARAMETER; > } > > - current_ram_size = pc_existing_dimms_capacity(&error_fatal); > + current_ram_size = ram_size + get_plugged_memory_size(); > > /* We only allow the guest to allocate an HPT one order above what > * we'd normally give them (to stop a small guest claiming a huge > According to the content of qmp_query_memory_size_summary(), it's the good way to compute the memory size... Reviewed-by: Laurent Vivier <lvivier@redhat.com>
On Wed, 11 Oct 2017 00:21:59 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > In order to prevent the guest from forcing the allocation of large amounts > of qemu memory (or host kernel memory, in the case of KVM HV), we limit > the size of Hashed Page Table (HPT) it is allowed to allocated, based on > its RAM size. > > However, the current calculation is not correct: it only adds up the size > of plugged memory, ignoring the base memory size. This patch corrects it. > > While we're there, use get_plugged_memory_size() instead of directly > calling pc_existing_dimms_capacity(). The only difference is that it > will abort on failure, which is right: a failure here indicates something > wrong within qemu. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr_hcall.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index 8d72bb7c1c..06af1b15c0 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -494,7 +494,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, > return H_PARAMETER; > } > > - current_ram_size = pc_existing_dimms_capacity(&error_fatal); > + current_ram_size = ram_size + get_plugged_memory_size(); current_ram_size is initialized earlier in this function: uint64_t current_ram_size = MACHINE(spapr)->ram_size; which is is initialized to ram_size in vl.c. Why not doing: current_ram_size += get_plugged_memory_size(); ? > > /* We only allow the guest to allocate an HPT one order above what > * we'd normally give them (to stop a small guest claiming a huge
On Wed, 2017-10-11 at 00:21 +1100, David Gibson wrote: > In order to prevent the guest from forcing the allocation of large amounts > of qemu memory (or host kernel memory, in the case of KVM HV), we limit > the size of Hashed Page Table (HPT) it is allowed to allocated, based on > its RAM size. > > However, the current calculation is not correct: it only adds up the size > of plugged memory, ignoring the base memory size. This patch corrects it. > > While we're there, use get_plugged_memory_size() instead of directly > calling pc_existing_dimms_capacity(). The only difference is that it > will abort on failure, which is right: a failure here indicates something > wrong within qemu. Does this change invalidate in any way the calculation performed by libvirt to figure out the memory locking limit for guests?
On 10/10/2017 16:44, Greg Kurz wrote: > On Wed, 11 Oct 2017 00:21:59 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > >> In order to prevent the guest from forcing the allocation of large amounts >> of qemu memory (or host kernel memory, in the case of KVM HV), we limit >> the size of Hashed Page Table (HPT) it is allowed to allocated, based on >> its RAM size. >> >> However, the current calculation is not correct: it only adds up the size >> of plugged memory, ignoring the base memory size. This patch corrects it. >> >> While we're there, use get_plugged_memory_size() instead of directly >> calling pc_existing_dimms_capacity(). The only difference is that it >> will abort on failure, which is right: a failure here indicates something >> wrong within qemu. >> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >> --- >> hw/ppc/spapr_hcall.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >> index 8d72bb7c1c..06af1b15c0 100644 >> --- a/hw/ppc/spapr_hcall.c >> +++ b/hw/ppc/spapr_hcall.c >> @@ -494,7 +494,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, >> return H_PARAMETER; >> } >> >> - current_ram_size = pc_existing_dimms_capacity(&error_fatal); >> + current_ram_size = ram_size + get_plugged_memory_size(); > > current_ram_size is initialized earlier in this function: > > uint64_t current_ram_size = MACHINE(spapr)->ram_size; > > which is is initialized to ram_size in vl.c. Why not doing: > > current_ram_size += get_plugged_memory_size(); > > ? I agree, it seems like the original intend of the first patch... Thanks, Laurent
On Tue, Oct 10, 2017 at 05:10:27PM +0200, Andrea Bolognani wrote: > On Wed, 2017-10-11 at 00:21 +1100, David Gibson wrote: > > In order to prevent the guest from forcing the allocation of large amounts > > of qemu memory (or host kernel memory, in the case of KVM HV), we limit > > the size of Hashed Page Table (HPT) it is allowed to allocated, based on > > its RAM size. > > > > However, the current calculation is not correct: it only adds up the size > > of plugged memory, ignoring the base memory size. This patch corrects it. > > > > While we're there, use get_plugged_memory_size() instead of directly > > calling pc_existing_dimms_capacity(). The only difference is that it > > will abort on failure, which is right: a failure here indicates something > > wrong within qemu. > > Does this change invalidate in any way the calculation performed > by libvirt to figure out the memory locking limit for guests? No.
On Tue, Oct 10, 2017 at 05:21:53PM +0200, Laurent Vivier wrote: > On 10/10/2017 16:44, Greg Kurz wrote: > > On Wed, 11 Oct 2017 00:21:59 +1100 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > >> In order to prevent the guest from forcing the allocation of large amounts > >> of qemu memory (or host kernel memory, in the case of KVM HV), we limit > >> the size of Hashed Page Table (HPT) it is allowed to allocated, based on > >> its RAM size. > >> > >> However, the current calculation is not correct: it only adds up the size > >> of plugged memory, ignoring the base memory size. This patch corrects it. > >> > >> While we're there, use get_plugged_memory_size() instead of directly > >> calling pc_existing_dimms_capacity(). The only difference is that it > >> will abort on failure, which is right: a failure here indicates something > >> wrong within qemu. > >> > >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > >> --- > >> hw/ppc/spapr_hcall.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > >> index 8d72bb7c1c..06af1b15c0 100644 > >> --- a/hw/ppc/spapr_hcall.c > >> +++ b/hw/ppc/spapr_hcall.c > >> @@ -494,7 +494,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, > >> return H_PARAMETER; > >> } > >> > >> - current_ram_size = pc_existing_dimms_capacity(&error_fatal); > >> + current_ram_size = ram_size + get_plugged_memory_size(); > > > > current_ram_size is initialized earlier in this function: > > > > uint64_t current_ram_size = MACHINE(spapr)->ram_size; > > > > which is is initialized to ram_size in vl.c. Why not doing: > > > > current_ram_size += get_plugged_memory_size(); > > > > ? > > I agree, it seems like the original intend of the first patch... Yes, I think so. However, splitting the calculation like that demonstrably misread someone reading the code (i.e. me), so I'm going to just ditch the initializerin the new spin.
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index 8d72bb7c1c..06af1b15c0 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -494,7 +494,7 @@ static target_ulong h_resize_hpt_prepare(PowerPCCPU *cpu, return H_PARAMETER; } - current_ram_size = pc_existing_dimms_capacity(&error_fatal); + current_ram_size = ram_size + get_plugged_memory_size(); /* We only allow the guest to allocate an HPT one order above what * we'd normally give them (to stop a small guest claiming a huge
In order to prevent the guest from forcing the allocation of large amounts of qemu memory (or host kernel memory, in the case of KVM HV), we limit the size of Hashed Page Table (HPT) it is allowed to allocated, based on its RAM size. However, the current calculation is not correct: it only adds up the size of plugged memory, ignoring the base memory size. This patch corrects it. While we're there, use get_plugged_memory_size() instead of directly calling pc_existing_dimms_capacity(). The only difference is that it will abort on failure, which is right: a failure here indicates something wrong within qemu. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr_hcall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)