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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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.


-- 
Alexey