Re: [Qemu-devel] [PATCH qemu] spapr/target-ppc/kvm: Only add hcall-instructions if KVM supports it

[Alexey Kardashevskiy]

On 03/15/2016 08:59 PM, David Gibson wrote:
> On Tue, Mar 15, 2016 at 04:51:20PM +1100, Alexey Kardashevskiy wrote:
> > ePAPR defines "hcall-instructions" device-tree property which contains
> > code to call hypercalls in ePAPR paravirtualized guests. However this
> > property is also present for pseries guests where it does not make sense,
> > even though it contains dummy code which simply fails.
> >
> > Instead of maintaining the property (which used to be BE only; then was
> > fixed to be endian-agnostic) and confusing the guest (which might think
> > there is ePAPR host while there is none), this simply does not
> > the property to the device tree if the host kernel does not implement it.
> >
> > In order to tell the machine code if the host kernel supports
> > KVM_CAP_PPC_GET_PVINFO, this changes kvmppc_get_hypercall() to return 1
> > if the host kernel does not implement it (which is HV KVM case).
> >
> > Signed-off-by: Alexey Kardashevskiy <address@hidden>
>
> So the idea of only adding the property when the host kernel supplies
> a suitable value seems good, but I'm a bit nervous about applying
> this, because I'm not sure what case the original fallback hypercall
> code was supposed to handle.
>
> agraf, if you could enlighten us with some history that could be good.
>
> > Alexander,
> >
> > We just got a bug report that LE guests would not boot under quite old QEMU
> > and we (powerkvm) wonder if it makes sense to backport endian-agnostic
> > hypercall code to older QEMU or it is simpler/more correct
> > not to have epapr-hypercall property in the tree.
> >
> >
> > ---
> >   hw/ppc/spapr.c   | 9 +++++----
> >   target-ppc/kvm.c | 2 +-
> >   2 files changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 43708a2..8130eb4 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -497,10 +497,11 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
> >                * Older KVM versions with older guest kernels were broken with 
> > the
> >                * magic page, don't allow the guest to map it.
> >                */
> > -            kvmppc_get_hypercall(first_cpu->env_ptr, hypercall,
> > -                                 sizeof(hypercall));
> > -            _FDT((fdt_property(fdt, "hcall-instructions", hypercall,
> > -                              sizeof(hypercall))));
> > +            if (!kvmppc_get_hypercall(first_cpu->env_ptr, hypercall,
> > +                                      sizeof(hypercall))) {
> > +                _FDT((fdt_property(fdt, "hcall-instructions", hypercall,
> > +                                   sizeof(hypercall))));
> > +            }
> >           }
> >           _FDT((fdt_end_node(fdt)));
> >       }
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index 776336b..e5183db 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -2001,7 +2001,7 @@ int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, 
> > int buf_len)
> >       hc[2] = cpu_to_be32(0x48000008);
> >       hc[3] = cpu_to_be32(bswap32(0x3860ffff));
>
> Since you're now returning a value which means the caller is supposed
> to ignore the hc code, there's not much point actually populating it above.


The return value means "no KVM support is here" rather than "ignore @buf 
content".

And the patch should have been "RFC" I suppose :)

> > -    return 0;
> > +    return 1;
> >   }
> >
> >   static inline int kvmppc_enable_hcall(KVMState *s, target_ulong hcall)


--
Alexey