* [Qemu-devel] [PATCH for-2.5 1/1] spapr: Handle failure of KVM_PPC_ALLOCATE_HTAB ioctl
@ 2015-11-03 10:08 Bharata B Rao
2015-11-03 16:19 ` Michael Roth
2015-11-09 4:24 ` David Gibson
0 siblings, 2 replies; 6+ messages in thread
From: Bharata B Rao @ 2015-11-03 10:08 UTC (permalink / raw)
To: qemu-devel; +Cc: Bharata B Rao, qemu-ppc, mdroth, david
KVM_PPC_ALLOCATE_HTAB ioctl can return -ENOMEM for KVM guests and QEMU
never handled this correctly. But this didn't cause any problems till
now as KVM_PPC_ALLOCATE_HTAB ioctl returned with smaller than requested
HTAB when enough contiguous memory wasn't available in the host.
After the proposed kernel change: https://patchwork.ozlabs.org/patch/530501/,
KVM_PPC_ALLOCATE_HTAB ioctl will not fallback to lower sized HTAB
allocation and will fail if requested HTAB size can't be met.
Check for such failures in QEMU and abort appropriately. This will
prevent guest kernel from hanging/freezing during early boot by doing
graceful exit when host is unable to allocate requested HTAB.
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
hw/ppc/spapr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e1202ce..ec6e141 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1022,7 +1022,7 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
shift = kvmppc_reset_htab(spapr->htab_shift);
- if (shift > 0) {
+ if (shift != 0) {
/* Kernel handles htab, we don't need to allocate one */
if (shift != spapr->htab_shift) {
error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
@@ -1055,7 +1055,7 @@ static void spapr_reset_htab(sPAPRMachineState *spapr)
int index;
shift = kvmppc_reset_htab(spapr->htab_shift);
- if (shift > 0) {
+ if (shift != 0) {
if (shift != spapr->htab_shift) {
error_setg(&error_abort, "Requested HTAB allocation failed during reset");
}
--
2.1.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 1/1] spapr: Handle failure of KVM_PPC_ALLOCATE_HTAB ioctl
2015-11-03 10:08 [Qemu-devel] [PATCH for-2.5 1/1] spapr: Handle failure of KVM_PPC_ALLOCATE_HTAB ioctl Bharata B Rao
@ 2015-11-03 16:19 ` Michael Roth
2015-11-09 4:24 ` David Gibson
1 sibling, 0 replies; 6+ messages in thread
From: Michael Roth @ 2015-11-03 16:19 UTC (permalink / raw)
To: Bharata B Rao, qemu-devel; +Cc: qemu-ppc, david
Quoting Bharata B Rao (2015-11-03 04:08:19)
> KVM_PPC_ALLOCATE_HTAB ioctl can return -ENOMEM for KVM guests and QEMU
> never handled this correctly. But this didn't cause any problems till
> now as KVM_PPC_ALLOCATE_HTAB ioctl returned with smaller than requested
> HTAB when enough contiguous memory wasn't available in the host.
> After the proposed kernel change: https://patchwork.ozlabs.org/patch/530501/,
> KVM_PPC_ALLOCATE_HTAB ioctl will not fallback to lower sized HTAB
> allocation and will fail if requested HTAB size can't be met.
>
> Check for such failures in QEMU and abort appropriately. This will
> prevent guest kernel from hanging/freezing during early boot by doing
> graceful exit when host is unable to allocate requested HTAB.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> hw/ppc/spapr.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e1202ce..ec6e141 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1022,7 +1022,7 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
>
> shift = kvmppc_reset_htab(spapr->htab_shift);
>
> - if (shift > 0) {
> + if (shift != 0) {
> /* Kernel handles htab, we don't need to allocate one */
> if (shift != spapr->htab_shift) {
> error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
> @@ -1055,7 +1055,7 @@ static void spapr_reset_htab(sPAPRMachineState *spapr)
> int index;
>
> shift = kvmppc_reset_htab(spapr->htab_shift);
> - if (shift > 0) {
> + if (shift != 0) {
> if (shift != spapr->htab_shift) {
> error_setg(&error_abort, "Requested HTAB allocation failed during reset");
> }
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 1/1] spapr: Handle failure of KVM_PPC_ALLOCATE_HTAB ioctl
2015-11-03 10:08 [Qemu-devel] [PATCH for-2.5 1/1] spapr: Handle failure of KVM_PPC_ALLOCATE_HTAB ioctl Bharata B Rao
2015-11-03 16:19 ` Michael Roth
@ 2015-11-09 4:24 ` David Gibson
2015-11-09 8:46 ` David Gibson
1 sibling, 1 reply; 6+ messages in thread
From: David Gibson @ 2015-11-09 4:24 UTC (permalink / raw)
To: Bharata B Rao; +Cc: qemu-ppc, qemu-devel, mdroth
[-- Attachment #1: Type: text/plain, Size: 2369 bytes --]
On Tue, Nov 03, 2015 at 03:38:19PM +0530, Bharata B Rao wrote:
> KVM_PPC_ALLOCATE_HTAB ioctl can return -ENOMEM for KVM guests and QEMU
> never handled this correctly. But this didn't cause any problems till
> now as KVM_PPC_ALLOCATE_HTAB ioctl returned with smaller than requested
> HTAB when enough contiguous memory wasn't available in the host.
> After the proposed kernel change: https://patchwork.ozlabs.org/patch/530501/,
> KVM_PPC_ALLOCATE_HTAB ioctl will not fallback to lower sized HTAB
> allocation and will fail if requested HTAB size can't be met.
>
> Check for such failures in QEMU and abort appropriately. This will
> prevent guest kernel from hanging/freezing during early boot by doing
> graceful exit when host is unable to allocate requested HTAB.
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
I'm going to apply this, since it fixes a real problem.
I'm not entirely happy with the way it's done though - I'd prefer to
see a separate case for (shift < 0) giving an unconditional error.
Handling both the HV success case and the failure case in that first
branch is unnecessarily subtle and confusing, IMO.
> ---
> hw/ppc/spapr.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e1202ce..ec6e141 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1022,7 +1022,7 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
>
> shift = kvmppc_reset_htab(spapr->htab_shift);
>
> - if (shift > 0) {
> + if (shift != 0) {
> /* Kernel handles htab, we don't need to allocate one */
> if (shift != spapr->htab_shift) {
> error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
> @@ -1055,7 +1055,7 @@ static void spapr_reset_htab(sPAPRMachineState *spapr)
> int index;
>
> shift = kvmppc_reset_htab(spapr->htab_shift);
> - if (shift > 0) {
> + if (shift != 0) {
> if (shift != spapr->htab_shift) {
> error_setg(&error_abort, "Requested HTAB allocation failed during reset");
> }
--
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: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 1/1] spapr: Handle failure of KVM_PPC_ALLOCATE_HTAB ioctl
2015-11-09 4:24 ` David Gibson
@ 2015-11-09 8:46 ` David Gibson
2015-11-09 12:12 ` Bharata B Rao
0 siblings, 1 reply; 6+ messages in thread
From: David Gibson @ 2015-11-09 8:46 UTC (permalink / raw)
To: Bharata B Rao; +Cc: qemu-ppc, qemu-devel, mdroth
[-- Attachment #1: Type: text/plain, Size: 2799 bytes --]
On Mon, Nov 09, 2015 at 03:24:15PM +1100, David Gibson wrote:
> On Tue, Nov 03, 2015 at 03:38:19PM +0530, Bharata B Rao wrote:
> > KVM_PPC_ALLOCATE_HTAB ioctl can return -ENOMEM for KVM guests and QEMU
> > never handled this correctly. But this didn't cause any problems till
> > now as KVM_PPC_ALLOCATE_HTAB ioctl returned with smaller than requested
> > HTAB when enough contiguous memory wasn't available in the host.
> > After the proposed kernel change: https://patchwork.ozlabs.org/patch/530501/,
> > KVM_PPC_ALLOCATE_HTAB ioctl will not fallback to lower sized HTAB
> > allocation and will fail if requested HTAB size can't be met.
> >
> > Check for such failures in QEMU and abort appropriately. This will
> > prevent guest kernel from hanging/freezing during early boot by doing
> > graceful exit when host is unable to allocate requested HTAB.
> >
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
>
> I'm going to apply this, since it fixes a real problem.
>
> I'm not entirely happy with the way it's done though - I'd prefer to
> see a separate case for (shift < 0) giving an unconditional error.
> Handling both the HV success case and the failure case in that first
> branch is unnecessarily subtle and confusing, IMO.
Ugh.. actually.. this patch seems to cause make check failures when
configured for powerpc guest on an x86 host. I haven't debugged yet,
but I'm guessing the shift != 0 is now catching the TCG (or PR) case
where we need to allocate the htab ourselves.
>
>
> > ---
> > hw/ppc/spapr.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index e1202ce..ec6e141 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1022,7 +1022,7 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
> >
> > shift = kvmppc_reset_htab(spapr->htab_shift);
> >
> > - if (shift > 0) {
> > + if (shift != 0) {
> > /* Kernel handles htab, we don't need to allocate one */
> > if (shift != spapr->htab_shift) {
> > error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
> > @@ -1055,7 +1055,7 @@ static void spapr_reset_htab(sPAPRMachineState *spapr)
> > int index;
> >
> > shift = kvmppc_reset_htab(spapr->htab_shift);
> > - if (shift > 0) {
> > + if (shift != 0) {
> > if (shift != spapr->htab_shift) {
> > error_setg(&error_abort, "Requested HTAB allocation failed during reset");
> > }
>
--
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: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 1/1] spapr: Handle failure of KVM_PPC_ALLOCATE_HTAB ioctl
2015-11-09 8:46 ` David Gibson
@ 2015-11-09 12:12 ` Bharata B Rao
2015-11-09 12:31 ` David Gibson
0 siblings, 1 reply; 6+ messages in thread
From: Bharata B Rao @ 2015-11-09 12:12 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, qemu-devel, mdroth
On Mon, Nov 09, 2015 at 07:46:55PM +1100, David Gibson wrote:
> On Mon, Nov 09, 2015 at 03:24:15PM +1100, David Gibson wrote:
> > On Tue, Nov 03, 2015 at 03:38:19PM +0530, Bharata B Rao wrote:
> > > KVM_PPC_ALLOCATE_HTAB ioctl can return -ENOMEM for KVM guests and QEMU
> > > never handled this correctly. But this didn't cause any problems till
> > > now as KVM_PPC_ALLOCATE_HTAB ioctl returned with smaller than requested
> > > HTAB when enough contiguous memory wasn't available in the host.
> > > After the proposed kernel change: https://patchwork.ozlabs.org/patch/530501/,
> > > KVM_PPC_ALLOCATE_HTAB ioctl will not fallback to lower sized HTAB
> > > allocation and will fail if requested HTAB size can't be met.
> > >
> > > Check for such failures in QEMU and abort appropriately. This will
> > > prevent guest kernel from hanging/freezing during early boot by doing
> > > graceful exit when host is unable to allocate requested HTAB.
> > >
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> >
> > I'm going to apply this, since it fixes a real problem.
> >
> > I'm not entirely happy with the way it's done though - I'd prefer to
> > see a separate case for (shift < 0) giving an unconditional error.
> > Handling both the HV success case and the failure case in that first
> > branch is unnecessarily subtle and confusing, IMO.
>
> Ugh.. actually.. this patch seems to cause make check failures when
> configured for powerpc guest on an x86 host. I haven't debugged yet,
> but I'm guessing the shift != 0 is now catching the TCG (or PR) case
> where we need to allocate the htab ourselves.
For ppc64 on x86, CONFIG_KVM doesn't get defined in config-target.h and
hence the HTAB reset routine that gets picked up is
static inline int kvmppc_reset_htab(int shift_hint)
{
return -1;
}
from target-ppc/kvm_ppc.h. I guess we should change this to return
0 so that we allocate HTAB ourselves. Negative values should always
mean error and we should abort in such cases.
Should I send the next version with above routine fixed to return 0
and spapr_alloc_htab/spapr_reset_htab changed to explicitly check and
fail for shift < 0 ?
I had tested both TCG and PR modes for ppc64 guest on ppc64 host where
both boot and reboot tests passed. Didn't realize that ppc64 emulation
on x86 could be different like this.
Regards,
Bharata.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 1/1] spapr: Handle failure of KVM_PPC_ALLOCATE_HTAB ioctl
2015-11-09 12:12 ` Bharata B Rao
@ 2015-11-09 12:31 ` David Gibson
0 siblings, 0 replies; 6+ messages in thread
From: David Gibson @ 2015-11-09 12:31 UTC (permalink / raw)
To: Bharata B Rao; +Cc: qemu-ppc, qemu-devel, mdroth
[-- Attachment #1: Type: text/plain, Size: 2877 bytes --]
On Mon, Nov 09, 2015 at 05:42:58PM +0530, Bharata B Rao wrote:
> On Mon, Nov 09, 2015 at 07:46:55PM +1100, David Gibson wrote:
> > On Mon, Nov 09, 2015 at 03:24:15PM +1100, David Gibson wrote:
> > > On Tue, Nov 03, 2015 at 03:38:19PM +0530, Bharata B Rao wrote:
> > > > KVM_PPC_ALLOCATE_HTAB ioctl can return -ENOMEM for KVM guests and QEMU
> > > > never handled this correctly. But this didn't cause any problems till
> > > > now as KVM_PPC_ALLOCATE_HTAB ioctl returned with smaller than requested
> > > > HTAB when enough contiguous memory wasn't available in the host.
> > > > After the proposed kernel change: https://patchwork.ozlabs.org/patch/530501/,
> > > > KVM_PPC_ALLOCATE_HTAB ioctl will not fallback to lower sized HTAB
> > > > allocation and will fail if requested HTAB size can't be met.
> > > >
> > > > Check for such failures in QEMU and abort appropriately. This will
> > > > prevent guest kernel from hanging/freezing during early boot by doing
> > > > graceful exit when host is unable to allocate requested HTAB.
> > > >
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > >
> > > I'm going to apply this, since it fixes a real problem.
> > >
> > > I'm not entirely happy with the way it's done though - I'd prefer to
> > > see a separate case for (shift < 0) giving an unconditional error.
> > > Handling both the HV success case and the failure case in that first
> > > branch is unnecessarily subtle and confusing, IMO.
> >
> > Ugh.. actually.. this patch seems to cause make check failures when
> > configured for powerpc guest on an x86 host. I haven't debugged yet,
> > but I'm guessing the shift != 0 is now catching the TCG (or PR) case
> > where we need to allocate the htab ourselves.
>
> For ppc64 on x86, CONFIG_KVM doesn't get defined in config-target.h and
> hence the HTAB reset routine that gets picked up is
>
> static inline int kvmppc_reset_htab(int shift_hint)
> {
> return -1;
> }
>
> from target-ppc/kvm_ppc.h. I guess we should change this to return
> 0 so that we allocate HTAB ourselves. Negative values should always
> mean error and we should abort in such cases.
Yes, that makes sense.
> Should I send the next version with above routine fixed to return 0
> and spapr_alloc_htab/spapr_reset_htab changed to explicitly check and
> fail for shift < 0 ?
Yes please.
> I had tested both TCG and PR modes for ppc64 guest on ppc64 host where
> both boot and reboot tests passed. Didn't realize that ppc64 emulation
> on x86 could be different like this.
I think it would also fail on a ppc64 host, if you explicitly disabled
KVM in the config.
--
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: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-11-09 15:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-03 10:08 [Qemu-devel] [PATCH for-2.5 1/1] spapr: Handle failure of KVM_PPC_ALLOCATE_HTAB ioctl Bharata B Rao
2015-11-03 16:19 ` Michael Roth
2015-11-09 4:24 ` David Gibson
2015-11-09 8:46 ` David Gibson
2015-11-09 12:12 ` Bharata B Rao
2015-11-09 12:31 ` David Gibson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).