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

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

[-- Attachment #1: Type: text/plain, Size: 3438 bytes --]

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 <aik@ozlabs.ru>

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.

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

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