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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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