From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46722) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aflUX-0006SB-BL for qemu-devel@nongnu.org; Tue, 15 Mar 2016 05:42:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aflUU-0002TO-8U for qemu-devel@nongnu.org; Tue, 15 Mar 2016 05:42:17 -0400 Received: from mail-pf0-x243.google.com ([2607:f8b0:400e:c00::243]:34395) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aflUT-0002R0-Tb for qemu-devel@nongnu.org; Tue, 15 Mar 2016 05:42:14 -0400 Received: by mail-pf0-x243.google.com with SMTP id n5so2055646pfn.1 for ; Tue, 15 Mar 2016 02:42:12 -0700 (PDT) References: <1458021080-2145-1-git-send-email-aik@ozlabs.ru> <56E7C573.8090405@redhat.com> From: Alexey Kardashevskiy Message-ID: <56E7D8ED.4070908@ozlabs.ru> Date: Tue, 15 Mar 2016 20:42:05 +1100 MIME-Version: 1.0 In-Reply-To: <56E7C573.8090405@redhat.com> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH qemu] spapr/target-ppc/kvm: Only add hcall-instructions if KVM supports it List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth , qemu-devel@nongnu.org Cc: qemu-ppc@nongnu.org, Paul Mackerras , Alexander Graf , David Gibson 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 >> --- >> >> >> 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. > 2) The code in spapr.c is already protected with a > if (kvmppc_has_cap_fixup_hcalls()) ... > and that CAP should only be there if the PVINFO CAP is available, too. > So I don't see how you could run into that problem anyway where PVINFO > is _not_ available but the FIXUP_HCALL CAP _is_ available? HV KVM guest calls (on pseries machine as well): kvm_guest_init kvm_para_has_feature kvm_arch_para_features kvm_para_available - this returns "1" epapr_hypercall0_1(KVM_HC_FEATURES) This epapr_hypercall0_1() calls a binary blob from "hcall-instructions". And fails if the guest is LE and the blob from BE-only times. -- Alexey