Re: [Qemu-devel] [PATCH for-2.5 1/1] spapr: Handle failure of KVM_PPC_ALLOCATE_HTAB ioctl

[David Gibson <david@gibson.dropbear.id.au>]

[-- 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 --]