| Message ID | 1446545299-19446-1-git-send-email-bharata@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 >
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"); > }
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"); > > } >
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.
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.
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"); }
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(-)