From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37831) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afmH8-0005Oa-Tw for qemu-devel@nongnu.org; Tue, 15 Mar 2016 06:32:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1afmGY-00076Q-LN for qemu-devel@nongnu.org; Tue, 15 Mar 2016 06:32:30 -0400 Received: from mail-pf0-x241.google.com ([2607:f8b0:400e:c00::241]:34449) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afmGY-00075k-92 for qemu-devel@nongnu.org; Tue, 15 Mar 2016 06:31:54 -0400 Received: by mail-pf0-x241.google.com with SMTP id n5so2206645pfn.1 for ; Tue, 15 Mar 2016 03:31:54 -0700 (PDT) References: <1458021080-2145-1-git-send-email-aik@ozlabs.ru> <20160315095952.GF9032@voom> From: Alexey Kardashevskiy Message-ID: <56E7E494.8090300@ozlabs.ru> Date: Tue, 15 Mar 2016 21:31:48 +1100 MIME-Version: 1.0 In-Reply-To: <20160315095952.GF9032@voom> 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: David Gibson Cc: Paul Mackerras , qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Alexander Graf 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 > > 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