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

[Bharata B Rao]

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 <address@hidden>
> > 
> > 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.