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 <[email protected]> > > > > > > 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
signature.asc
Description: PGP signature
