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

[David Gibson]

On Tue, Mar 15, 2016 at 08:42:05PM +1100, Alexey Kardashevskiy wrote:
> On 03/15/2016 07:18 PM, Thomas Huth wrote:
> >
> >  Hi Alexey,
> >
> >On 15.03.2016 06:51, 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>
> >>---
> >>
> >>
> >>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));
> >>
> >>-    return 0;
> >>+    return 1;
> >>  }
> >>
> >>  static inline int kvmppc_enable_hcall(KVMState *s, target_ulong hcall)
> >
> >Sorry, I have a hard time to understand what this is really good for. Is
> >it a patch for current QEMU or for older ones? If it is for older ones,
> >then why did you not CC: to qemu-stable?
> >If it is for current QEMU, then I've got some more questions about
> >things I do not understand:
> >
> >1) In your patch description, you talk about ePAPR and that the property
> >does not make sense for pseries. But why is this code then available at
> >all in spapr.c? ... there must be a reason for this, I think (like using
> >a different h-call on nested KVM-PR for example?)
> 
> 
> No, this is from old times when there was only PR KVM fully emulating
> powermac (not pseries) which needed to interact with the hypervisor and
> epapr_hypercall was chosen for this.

That's where it originated.  Howver I think it was also used to allow
PR KVM to work on PowerVM or old host KVMs which didn't do hypercall
passthrough for HV|PR.

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