[Qemu-devel] [RFC PATCH v0 0/2] spapr: Abort when HTAB size requirement can't be met

[Qemu-devel] [RFC PATCH v0 0/2] spapr: Abort when HTAB size requirement can't be met

[Bharata B Rao]

HTAB size is a factor of maximum memory size that is specified by maxmem=
command line option. In cases where there is shortage of host memory, host
will not be able to allocate contiguous memory for guest HTAB and will
instead allocate a smaller HTAB. This usually is not a problem but when
user starts hotplugging memory to the guest, we can run out of HTAB entries
and hence memory hotplug fails. This failure should have been handled
gracefully by the guest kernel, but currently it leads to guest kernel OOPS.
This will eventually get fixed when the handling of memory hotplug is
completely moved to kernel for PowerKVM.

Prevent such kernel failure by refusing to boot the guest when requested
HTAB size can't be allocated. However HTAB allocation happens in the
reset path from where it is too late to abort. Hence this patchset
moves the HTAB allocation to machine init and aborts if HTAB size
requirement isn't met.

Two previous related attempts to address this problem:

1. http://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg03325.html
   Aborts the guest from the reset path - not desirable.
2. https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02602.html
   Silently disables memory hotplug - not desirable.

This patchset applies against David Gibson's spapr-next.

Bharata B Rao (2):
  spapr: Allocate HTAB from machine init
  spapr: Abort when HTAB of requested size isn't allocated

 hw/ppc/spapr.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

-- 
2.1.0

[Qemu-devel] [RFC PATCH v0 1/2] spapr: Allocate HTAB from machine init

[Bharata B Rao]

Allocate HTAB from ppc_spapr_init() so that we can abort the guest
if requested HTAB size is't allocated by the host. However retain the
htab reset call in spapr_reset_htab() so that HTAB gets reset (and
not allocated) during machine reset.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7f4f196..4692122 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -979,7 +979,7 @@ static void emulate_spapr_hypercall(PowerPCCPU *cpu)
 #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
 #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
 
-static void spapr_reset_htab(sPAPRMachineState *spapr)
+static void spapr_alloc_htab(sPAPRMachineState *spapr)
 {
     long shift;
     int index;
@@ -1012,6 +1012,16 @@ static void spapr_reset_htab(sPAPRMachineState *spapr)
             DIRTY_HPTE(HPTE(spapr->htab, index));
         }
     }
+}
+
+static void spapr_reset_htab(sPAPRMachineState *spapr)
+{
+    /*
+     * We have already allocated the hash page table, this call will
+     * not again allocate but only result in clearing of hash page
+     * table entries.
+     */
+    kvmppc_reset_htab(spapr->htab_shift);
 
     /* Update the RMA size if necessary */
     if (spapr->vrma_adjust) {
@@ -1709,6 +1719,7 @@ static void ppc_spapr_init(MachineState *machine)
         }
         spapr->htab_shift++;
     }
+    spapr_alloc_htab(spapr);
 
     /* Set up Interrupt Controller before we create the VCPUs */
     spapr->icp = xics_system_init(machine,
-- 
2.1.0

Re: [Qemu-devel] [RFC PATCH v0 1/2] spapr: Allocate HTAB from machine init

[David Gibson]

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

On Tue, Sep 22, 2015 at 09:09:48AM +0530, Bharata B Rao wrote:
> Allocate HTAB from ppc_spapr_init() so that we can abort the guest
> if requested HTAB size is't allocated by the host. However retain the
> htab reset call in spapr_reset_htab() so that HTAB gets reset (and
> not allocated) during machine reset.

I was briefly worried about this, because I recall there as a reason
htab allocation got moved to the reset handler in the first place.
Looking at the git history, however, I've convinced myself this is
basically ok (because you preserve the call during reset to wipe clean
the htab).

> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7f4f196..4692122 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -979,7 +979,7 @@ static void emulate_spapr_hypercall(PowerPCCPU *cpu)
>  #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
>  #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
>  
> -static void spapr_reset_htab(sPAPRMachineState *spapr)
> +static void spapr_alloc_htab(sPAPRMachineState *spapr)
>  {
>      long shift;
>      int index;
> @@ -1012,6 +1012,16 @@ static void spapr_reset_htab(sPAPRMachineState *spapr)
>              DIRTY_HPTE(HPTE(spapr->htab, index));
>          }
>      }
> +}
> +
> +static void spapr_reset_htab(sPAPRMachineState *spapr)
> +{
> +    /*
> +     * We have already allocated the hash page table, this call will
> +     * not again allocate but only result in clearing of hash page
> +     * table entries.
> +     */
> +    kvmppc_reset_htab(spapr->htab_shift);

It's unlikely the kernel will give us less htab than we already have,
but we really should at least check for that.  Probably not much we
can do except abort() but at least we can give a useful error message.

>      /* Update the RMA size if necessary */
>      if (spapr->vrma_adjust) {
> @@ -1709,6 +1719,7 @@ static void ppc_spapr_init(MachineState *machine)
>          }
>          spapr->htab_shift++;
>      }
> +    spapr_alloc_htab(spapr);
>  
>      /* Set up Interrupt Controller before we create the VCPUs */
>      spapr->icp = xics_system_init(machine,

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH v0 1/2] spapr: Allocate HTAB from machine init

[Bharata B Rao]

On Wed, Sep 23, 2015 at 01:28:53PM +1000, David Gibson wrote:
> On Tue, Sep 22, 2015 at 09:09:48AM +0530, Bharata B Rao wrote:
> > Allocate HTAB from ppc_spapr_init() so that we can abort the guest
> > if requested HTAB size is't allocated by the host. However retain the
> > htab reset call in spapr_reset_htab() so that HTAB gets reset (and
> > not allocated) during machine reset.
> 
> I was briefly worried about this, because I recall there as a reason
> htab allocation got moved to the reset handler in the first place.
> Looking at the git history, however, I've convinced myself this is
> basically ok (because you preserve the call during reset to wipe clean
> the htab).
> 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 7f4f196..4692122 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -979,7 +979,7 @@ static void emulate_spapr_hypercall(PowerPCCPU *cpu)
> >  #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
> >  #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
> >  
> > -static void spapr_reset_htab(sPAPRMachineState *spapr)
> > +static void spapr_alloc_htab(sPAPRMachineState *spapr)
> >  {
> >      long shift;
> >      int index;
> > @@ -1012,6 +1012,16 @@ static void spapr_reset_htab(sPAPRMachineState *spapr)
> >              DIRTY_HPTE(HPTE(spapr->htab, index));
> >          }
> >      }
> > +}
> > +
> > +static void spapr_reset_htab(sPAPRMachineState *spapr)
> > +{
> > +    /*
> > +     * We have already allocated the hash page table, this call will
> > +     * not again allocate but only result in clearing of hash page
> > +     * table entries.
> > +     */
> > +    kvmppc_reset_htab(spapr->htab_shift);
> 
> It's unlikely the kernel will give us less htab than we already have,
> but we really should at least check for that.  Probably not much we
> can do except abort() but at least we can give a useful error message.

With the change I am doing here, this is no longer an allocation path.
Host kernel will just clear the HTAB and return the same htab_shift
that we passed here. So do you think it still makes sense to check
return value ?

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v0 1/2] spapr: Allocate HTAB from machine init

[David Gibson]

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

On Thu, Sep 24, 2015 at 09:11:52AM +0530, Bharata B Rao wrote:
> On Wed, Sep 23, 2015 at 01:28:53PM +1000, David Gibson wrote:
> > On Tue, Sep 22, 2015 at 09:09:48AM +0530, Bharata B Rao wrote:
> > > Allocate HTAB from ppc_spapr_init() so that we can abort the guest
> > > if requested HTAB size is't allocated by the host. However retain the
> > > htab reset call in spapr_reset_htab() so that HTAB gets reset (and
> > > not allocated) during machine reset.
> > 
> > I was briefly worried about this, because I recall there as a reason
> > htab allocation got moved to the reset handler in the first place.
> > Looking at the git history, however, I've convinced myself this is
> > basically ok (because you preserve the call during reset to wipe clean
> > the htab).
> > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/spapr.c | 13 ++++++++++++-
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 7f4f196..4692122 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -979,7 +979,7 @@ static void emulate_spapr_hypercall(PowerPCCPU *cpu)
> > >  #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
> > >  #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
> > >  
> > > -static void spapr_reset_htab(sPAPRMachineState *spapr)
> > > +static void spapr_alloc_htab(sPAPRMachineState *spapr)
> > >  {
> > >      long shift;
> > >      int index;
> > > @@ -1012,6 +1012,16 @@ static void spapr_reset_htab(sPAPRMachineState *spapr)
> > >              DIRTY_HPTE(HPTE(spapr->htab, index));
> > >          }
> > >      }
> > > +}
> > > +
> > > +static void spapr_reset_htab(sPAPRMachineState *spapr)
> > > +{
> > > +    /*
> > > +     * We have already allocated the hash page table, this call will
> > > +     * not again allocate but only result in clearing of hash page
> > > +     * table entries.
> > > +     */
> > > +    kvmppc_reset_htab(spapr->htab_shift);
> > 
> > It's unlikely the kernel will give us less htab than we already have,
> > but we really should at least check for that.  Probably not much we
> > can do except abort() but at least we can give a useful error message.
> 
> With the change I am doing here, this is no longer an allocation path.
> Host kernel will just clear the HTAB and return the same htab_shift
> that we passed here. So do you think it still makes sense to check
> return value ?

That's the current kernel behaviour, but the interface doesn't
guarantee that.  So, yes, I still think you have to check the return
value.

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [RFC PATCH v0 2/2] spapr: Abort when HTAB of requested size isn't allocated

[Bharata B Rao]

Terminate the guest when HTAB of requested size isn't allocated by
the host.

When memory hotplug is attempted on a guest that has booted with
less than requested HTAB size, the guest kernel will not be able
to gracefully fail the hotplug request. This patch will ensure that
we never end up in a situation where memory hotplug fails due to
less than requested HTAB size.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 4692122..66446af 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -992,6 +992,10 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
 
     if (shift > 0) {
         /* Kernel handles htab, we don't need to allocate one */
+        if (shift != spapr->htab_shift) {
+            error_setg(&error_abort, "Failed to allocated requested HTAB size");
+        }
+
         spapr->htab_shift = shift;
         kvmppc_kern_htab = true;
 
-- 
2.1.0

Re: [Qemu-devel] [RFC PATCH v0 2/2] spapr: Abort when HTAB of requested size isn't allocated

[David Gibson]

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

On Tue, Sep 22, 2015 at 09:09:49AM +0530, Bharata B Rao wrote:
> Terminate the guest when HTAB of requested size isn't allocated by
> the host.
> 
> When memory hotplug is attempted on a guest that has booted with
> less than requested HTAB size, the guest kernel will not be able
> to gracefully fail the hotplug request. This patch will ensure that
> we never end up in a situation where memory hotplug fails due to
> less than requested HTAB size.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 4692122..66446af 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -992,6 +992,10 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
>  
>      if (shift > 0) {
>          /* Kernel handles htab, we don't need to allocate one */
> +        if (shift != spapr->htab_shift) {
> +            error_setg(&error_abort, "Failed to allocated requested HTAB size");
> +        }
> +
>          spapr->htab_shift = shift;
>          kvmppc_kern_htab = true;
>  

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH v0 2/2] spapr: Abort when HTAB of requested size isn't allocated

[Bharata B Rao]

On Wed, Sep 23, 2015 at 01:29:51PM +1000, David Gibson wrote:
> On Tue, Sep 22, 2015 at 09:09:49AM +0530, Bharata B Rao wrote:
> > Terminate the guest when HTAB of requested size isn't allocated by
> > the host.
> > 
> > When memory hotplug is attempted on a guest that has booted with
> > less than requested HTAB size, the guest kernel will not be able
> > to gracefully fail the hotplug request. This patch will ensure that
> > we never end up in a situation where memory hotplug fails due to
> > less than requested HTAB size.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> > ---
> >  hw/ppc/spapr.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 4692122..66446af 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -992,6 +992,10 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
> >  
> >      if (shift > 0) {
> >          /* Kernel handles htab, we don't need to allocate one */
> > +        if (shift != spapr->htab_shift) {
> > +            error_setg(&error_abort, "Failed to allocated requested HTAB size");

Do you think we should have a bit more verbose message here so that
use knows what action to take after this abortion ? Like

"Failed to allocate requested HTAB size, try with lower maxmem"

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v0 2/2] spapr: Abort when HTAB of requested size isn't allocated

[David Gibson]

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

On Thu, Sep 24, 2015 at 09:13:58AM +0530, Bharata B Rao wrote:
> On Wed, Sep 23, 2015 at 01:29:51PM +1000, David Gibson wrote:
> > On Tue, Sep 22, 2015 at 09:09:49AM +0530, Bharata B Rao wrote:
> > > Terminate the guest when HTAB of requested size isn't allocated by
> > > the host.
> > > 
> > > When memory hotplug is attempted on a guest that has booted with
> > > less than requested HTAB size, the guest kernel will not be able
> > > to gracefully fail the hotplug request. This patch will ensure that
> > > we never end up in a situation where memory hotplug fails due to
> > > less than requested HTAB size.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > 
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > > ---
> > >  hw/ppc/spapr.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 4692122..66446af 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -992,6 +992,10 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
> > >  
> > >      if (shift > 0) {
> > >          /* Kernel handles htab, we don't need to allocate one */
> > > +        if (shift != spapr->htab_shift) {
> > > +            error_setg(&error_abort, "Failed to allocated requested HTAB size");
> 
> Do you think we should have a bit more verbose message here so that
> use knows what action to take after this abortion ? Like
> 
> "Failed to allocate requested HTAB size, try with lower maxmem"

Uh, yeah, I guess that's better.

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]