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 <a...@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?
Thomas
