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

2016-03-19 Thread Alexey Kardashevskiy 

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(0x4808);
hc[3] = cpu_to_be32(bswap32(0x3860));

-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

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

2016-03-16 Thread Thomas Huth 
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(0x4808);
>hc[3] = cpu_to_be32(bswap32(0x3860));
>
> -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

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

2016-03-15 Thread Alexey Kardashevskiy 

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(0x4808);
   hc[3] = cpu_to_be32(bswap32(0x3860));

-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".




--
Alexey

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

2016-03-15 Thread Thomas Huth 
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(0x4808);
>>>   hc[3] = cpu_to_be32(bswap32(0x3860));
>>>
>>> -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

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

2016-03-15 Thread David Gibson 
On Tue, Mar 15, 2016 at 08:42:05PM +1100, 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(0x4808);
> >>  hc[3] = cpu_to_be32(bswap32(0x3860));
> >>
> >>-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.

That's where it originated.  Howver I think it was also used to allow
PR KVM to work on PowerVM or old host KVMs which didn't do hypercall
passthrough for HV|PR.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

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

2016-03-15 Thread Alexey Kardashevskiy 

On 03/15/2016 09:19 PM, Alexander Graf wrote:



On 15.03.16 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.


Without the property you lose KVM hypercalls, so mostly some PR
speedups.


Like what? I did grep and could not spot many for pseries, only 
KVM_HC_FEATURES and KVM_HC_PPC_MAP_MAGIC_PAGE. I am suggesting here to hide 
this property from "pseries" guests only.




For HV KVM, I don't think it makes a lot of sense to expose
KVM specific hypercalls, but I'm not sure it's a great idea to block the
path. With the infrastructure in place, we can at least add non-sPAPR PV
if we want to.


I am not suggesting removing it, only advertise it if the KVM supports it...




--
Alexey

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

2016-03-15 Thread Alexey Kardashevskiy 

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(0x4808);
  hc[3] = cpu_to_be32(bswap32(0x3860));


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

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

2016-03-15 Thread Alexander Graf 


On 15.03.16 10:59, 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.

The fallback code simply does "return -1" regardless of guest
endianness, so it makes every kvm hypercall fail.

I guess not supplying the sequence at all if the host kernel doesn't
implement kvm hypercalls (and thus doesn't expose the trampoline), yeah.
I wonder why I didn't do that back then, hrm ..


Alex

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

2016-03-15 Thread David Gibson 
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(0x4808);
>  hc[3] = cpu_to_be32(bswap32(0x3860));

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.

> -return 0;
> +return 1;
>  }
>  
>  static inline int kvmppc_enable_hcall(KVMState *s, target_ulong hcall)

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

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

2016-03-15 Thread Alexander Graf 


On 15.03.16 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.

Without the property you lose KVM hypercalls, so mostly some PR
speedups. For HV KVM, I don't think it makes a lot of sense to expose
KVM specific hypercalls, but I'm not sure it's a great idea to block the
path. With the infrastructure in place, we can at least add non-sPAPR PV
if we want to.


Alex

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

2016-03-15 Thread Alexey Kardashevskiy 

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(0x4808);
  hc[3] = cpu_to_be32(bswap32(0x3860));

-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

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

2016-03-15 Thread Thomas Huth 

 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(0x4808);
>  hc[3] = cpu_to_be32(bswap32(0x3860));
>  
> -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?)

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?

 Thomas

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

2016-03-14 Thread Alexey Kardashevskiy 
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(0x4808);
 hc[3] = cpu_to_be32(bswap32(0x3860));
 
-return 0;
+return 1;
 }
 
 static inline int kvmppc_enable_hcall(KVMState *s, target_ulong hcall)
-- 
2.5.0.rc3