From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45681) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1agNJ0-0004N0-24 for qemu-devel@nongnu.org; Wed, 16 Mar 2016 22:04:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1agNIy-0008Au-2F for qemu-devel@nongnu.org; Wed, 16 Mar 2016 22:04:53 -0400 Received: from mail-pf0-x242.google.com ([2607:f8b0:400e:c00::242]:34631) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1agNIx-0008A6-OY for qemu-devel@nongnu.org; Wed, 16 Mar 2016 22:04:52 -0400 Received: by mail-pf0-x242.google.com with SMTP id n5so8961303pfn.1 for ; Wed, 16 Mar 2016 19:04:50 -0700 (PDT) References: <1458021080-2145-1-git-send-email-aik@ozlabs.ru> <56E7C573.8090405@redhat.com> <56E7D8ED.4070908@ozlabs.ru> <56E7F2D5.2040201@redhat.com> <56E8C868.6090508@ozlabs.ru> <56E8F759.9040301@redhat.com> From: Alexey Kardashevskiy Message-ID: <56EA10BB.9050008@ozlabs.ru> Date: Thu, 17 Mar 2016 13:04:43 +1100 MIME-Version: 1.0 In-Reply-To: <56E8F759.9040301@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/16/2016 05:04 PM, Thomas Huth wrote: > On 16.03.2016 03:43, Alexey Kardashevskiy wrote: >> On 03/15/2016 10:32 PM, Thomas Huth wrote: >>> On 15.03.2016 10:42, 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 >>>>>> --- >>>>>> >>>>>> >>>>>> 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. >>> >>> What about that "if (kvmppc_has_cap_fixup_hcalls())" ? Could you please >>> check why this succeeds on your system , but the KVM_CAP_PPC_GET_PVINFO >>> call does not? >> >> KVM_CAP_PPC_FIXUP_HCALL is always enabled for CONFIG_PPC_BOOK3S_64, >> KVM_CAP_PPC_GET_PVINFO is only enabled for "!hv_enabled". > > Ah, that's the detail that I missed. Thanks a lot for the hint! > ... ok, then I think your patch is the right thing to do, but you should > maybe change the patch description a little bit (since this call still > might make sense on sPAPR, too). Well, yes but there is no clear indication yet whether the patch will be accepted at all so I will not bother reposting till then :) -- Alexey