[Qemu-devel] [PATCH] target-ppc: ppc64 target's virtio can be either endian.

[Qemu-devel] [PATCH] target-ppc: ppc64 target's virtio can be either endian.

[Greg Kurz]

We base it on the OS endian, as reflected by the endianness of the
interrupt vectors (handled through the ILE bit in the LPCR register).

This patch does two things:
- make LPCR a KVM register
- implement virtio_get_byteswap() over LPCR

This patch requires to have the following defined in the linux headers:

$ grep LPCR linux-headers/asm-powerpc/kvm.h
#define KVM_REG_PPC_LPCR        (KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xb5)

Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 target-ppc/kvm.c         |    4 ++++
 target-ppc/misc_helper.c |   14 ++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 10d0cd9..b450a22 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -869,6 +869,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
                 DPRINTF("Warning: Unable to set VPA information to KVM\n");
             }
         }
+
+        kvm_put_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);
 #endif /* TARGET_PPC64 */
     }
 
@@ -1091,6 +1093,8 @@ int kvm_arch_get_registers(CPUState *cs)
                 DPRINTF("Warning: Unable to get VPA information from KVM\n");
             }
         }
+
+        kvm_get_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);
 #endif
     }
 
diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
index 616aab6..0e0743a 100644
--- a/target-ppc/misc_helper.c
+++ b/target-ppc/misc_helper.c
@@ -20,6 +20,8 @@
 #include "helper.h"
 
 #include "helper_regs.h"
+#include "hw/virtio/virtio.h"
+#include "sysemu/kvm.h"
 
 /*****************************************************************************/
 /* SPR accesses */
@@ -116,3 +118,15 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value)
 {
     hreg_store_msr(env, value, 0);
 }
+
+bool virtio_get_byteswap(void)
+{
+    PowerPCCPU *cp = POWERPC_CPU(first_cpu);
+    CPUPPCState *env = &cp->env;
+
+    if (kvm_enabled()) { 
+        kvm_arch_get_registers(first_cpu);
+    }
+
+    return env->spr[SPR_LPCR] & LPCR_ILE;
+}

Re: [Qemu-devel] [PATCH] target-ppc: ppc64 target's virtio can be either endian.

[Alexander Graf]

On 25.11.2013, at 16:35, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> We base it on the OS endian, as reflected by the endianness of the
> interrupt vectors (handled through the ILE bit in the LPCR register).
> 
> This patch does two things:
> - make LPCR a KVM register
> - implement virtio_get_byteswap() over LPCR
> 
> This patch requires to have the following defined in the linux headers:
> 
> $ grep LPCR linux-headers/asm-powerpc/kvm.h
> #define KVM_REG_PPC_LPCR        (KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xb5)
> 
> Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
> target-ppc/kvm.c         |    4 ++++
> target-ppc/misc_helper.c |   14 ++++++++++++++
> 2 files changed, 18 insertions(+)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 10d0cd9..b450a22 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -869,6 +869,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>                 DPRINTF("Warning: Unable to set VPA information to KVM\n");
>             }
>         }
> +
> +        kvm_put_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);
> #endif /* TARGET_PPC64 */
>     }
> 
> @@ -1091,6 +1093,8 @@ int kvm_arch_get_registers(CPUState *cs)
>                 DPRINTF("Warning: Unable to get VPA information from KVM\n");
>             }
>         }
> +
> +        kvm_get_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);
> #endif
>     }
> 
> diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
> index 616aab6..0e0743a 100644
> --- a/target-ppc/misc_helper.c
> +++ b/target-ppc/misc_helper.c
> @@ -20,6 +20,8 @@
> #include "helper.h"
> 
> #include "helper_regs.h"
> +#include "hw/virtio/virtio.h"
> +#include "sysemu/kvm.h"
> 
> /*****************************************************************************/
> /* SPR accesses */
> @@ -116,3 +118,15 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value)
> {
>     hreg_store_msr(env, value, 0);
> }
> +
> +bool virtio_get_byteswap(void)
> +{
> +    PowerPCCPU *cp = POWERPC_CPU(first_cpu);
> +    CPUPPCState *env = &cp->env;
> +
> +    if (kvm_enabled()) { 
> +        kvm_arch_get_registers(first_cpu);

This function is not defined when CONFIG_KVM is disabled.

Please do

    cpu_synchronize_state(first_cpu);

instead.

Also can't virtio_get_byteswap pass in the CPU pointer of the CPU that's calling in this moment? I'm not sure how racy it is to synchronize the first cpu while we're not in the first cpu's execution thread.

Either way, if we do this "right" we don't even have to jump through these hoops, as little endian setting simply happens steered from QEMU, so QEMU will have all knowledge about the guest's little endian mode without the need to synchronize any state.


Alex

Re: [Qemu-devel] [PATCH] target-ppc: ppc64 target's virtio can be either endian.

[Greg Kurz]

On Mon, 9 Dec 2013 16:33:59 +0100
Alexander Graf <agraf@suse.de> wrote:
> 
> On 25.11.2013, at 16:35, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > We base it on the OS endian, as reflected by the endianness of the
> > interrupt vectors (handled through the ILE bit in the LPCR register).
> > 
> > This patch does two things:
> > - make LPCR a KVM register
> > - implement virtio_get_byteswap() over LPCR
> > 
> > This patch requires to have the following defined in the linux headers:
> > 
> > $ grep LPCR linux-headers/asm-powerpc/kvm.h
> > #define KVM_REG_PPC_LPCR        (KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xb5)
> > 
> > Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> > target-ppc/kvm.c         |    4 ++++
> > target-ppc/misc_helper.c |   14 ++++++++++++++
> > 2 files changed, 18 insertions(+)
> > 
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index 10d0cd9..b450a22 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -869,6 +869,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
> >                 DPRINTF("Warning: Unable to set VPA information to
> > KVM\n"); }
> >         }
> > +
> > +        kvm_put_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);
> > #endif /* TARGET_PPC64 */
> >     }
> > 
> > @@ -1091,6 +1093,8 @@ int kvm_arch_get_registers(CPUState *cs)
> >                 DPRINTF("Warning: Unable to get VPA information from
> > KVM\n"); }
> >         }
> > +
> > +        kvm_get_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);
> > #endif
> >     }
> > 
> > diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
> > index 616aab6..0e0743a 100644
> > --- a/target-ppc/misc_helper.c
> > +++ b/target-ppc/misc_helper.c
> > @@ -20,6 +20,8 @@
> > #include "helper.h"
> > 
> > #include "helper_regs.h"
> > +#include "hw/virtio/virtio.h"
> > +#include "sysemu/kvm.h"
> > 
> > /*****************************************************************************/
> > /* SPR accesses */
> > @@ -116,3 +118,15 @@ void ppc_store_msr(CPUPPCState *env, target_ulong
> > value) {
> >     hreg_store_msr(env, value, 0);
> > }
> > +
> > +bool virtio_get_byteswap(void)
> > +{
> > +    PowerPCCPU *cp = POWERPC_CPU(first_cpu);
> > +    CPUPPCState *env = &cp->env;
> > +
> > +    if (kvm_enabled()) { 
> > +        kvm_arch_get_registers(first_cpu);
> 
> This function is not defined when CONFIG_KVM is disabled.
> 
> Please do
> 
>     cpu_synchronize_state(first_cpu);
> 
> instead.
> 

Oups... I'll fix that.

> Also can't virtio_get_byteswap pass in the CPU pointer of the CPU that's
> calling in this moment? I'm not sure how racy it is to synchronize the
> first cpu while we're not in the first cpu's execution thread.
> 

I kept the choices made by Rusty in the original serie. According to
these, I am not sure if we can use current_cpu:

https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg01156.html
https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg01504.html

> Either way, if we do this "right" we don't even have to jump through
> these hoops, as little endian setting simply happens steered from QEMU,
> so QEMU will have all knowledge about the guest's little endian mode
> without the need to synchronize any state.
> 

Sure. This is just primary work to get virtio working for cross-endian
cases. :)

Thanks for the review.

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH] target-ppc: ppc64 target's virtio can be either endian.

[Alexander Graf]

On 09.12.2013, at 18:11, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> On Mon, 9 Dec 2013 16:33:59 +0100
> Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 25.11.2013, at 16:35, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>> 
>>> We base it on the OS endian, as reflected by the endianness of the
>>> interrupt vectors (handled through the ILE bit in the LPCR register).
>>> 
>>> This patch does two things:
>>> - make LPCR a KVM register
>>> - implement virtio_get_byteswap() over LPCR
>>> 
>>> This patch requires to have the following defined in the linux headers:
>>> 
>>> $ grep LPCR linux-headers/asm-powerpc/kvm.h
>>> #define KVM_REG_PPC_LPCR        (KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xb5)
>>> 
>>> Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>>> ---
>>> target-ppc/kvm.c         |    4 ++++
>>> target-ppc/misc_helper.c |   14 ++++++++++++++
>>> 2 files changed, 18 insertions(+)
>>> 
>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>> index 10d0cd9..b450a22 100644
>>> --- a/target-ppc/kvm.c
>>> +++ b/target-ppc/kvm.c
>>> @@ -869,6 +869,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>>                DPRINTF("Warning: Unable to set VPA information to
>>> KVM\n"); }
>>>        }
>>> +
>>> +        kvm_put_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);
>>> #endif /* TARGET_PPC64 */
>>>    }
>>> 
>>> @@ -1091,6 +1093,8 @@ int kvm_arch_get_registers(CPUState *cs)
>>>                DPRINTF("Warning: Unable to get VPA information from
>>> KVM\n"); }
>>>        }
>>> +
>>> +        kvm_get_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);
>>> #endif
>>>    }
>>> 
>>> diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
>>> index 616aab6..0e0743a 100644
>>> --- a/target-ppc/misc_helper.c
>>> +++ b/target-ppc/misc_helper.c
>>> @@ -20,6 +20,8 @@
>>> #include "helper.h"
>>> 
>>> #include "helper_regs.h"
>>> +#include "hw/virtio/virtio.h"
>>> +#include "sysemu/kvm.h"
>>> 
>>> /*****************************************************************************/
>>> /* SPR accesses */
>>> @@ -116,3 +118,15 @@ void ppc_store_msr(CPUPPCState *env, target_ulong
>>> value) {
>>>    hreg_store_msr(env, value, 0);
>>> }
>>> +
>>> +bool virtio_get_byteswap(void)
>>> +{
>>> +    PowerPCCPU *cp = POWERPC_CPU(first_cpu);
>>> +    CPUPPCState *env = &cp->env;
>>> +
>>> +    if (kvm_enabled()) { 
>>> +        kvm_arch_get_registers(first_cpu);
>> 
>> This function is not defined when CONFIG_KVM is disabled.
>> 
>> Please do
>> 
>>    cpu_synchronize_state(first_cpu);
>> 
>> instead.
>> 
> 
> Oups... I'll fix that.
> 
>> Also can't virtio_get_byteswap pass in the CPU pointer of the CPU that's
>> calling in this moment? I'm not sure how racy it is to synchronize the
>> first cpu while we're not in the first cpu's execution thread.
>> 
> 
> I kept the choices made by Rusty in the original serie. According to
> these, I am not sure if we can use current_cpu:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg01156.html
> https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg01504.html
> 
>> Either way, if we do this "right" we don't even have to jump through
>> these hoops, as little endian setting simply happens steered from QEMU,
>> so QEMU will have all knowledge about the guest's little endian mode
>> without the need to synchronize any state.
>> 
> 
> Sure. This is just primary work to get virtio working for cross-endian
> cases. :)

Yeah, but so far LE switching code is not upstream in KVM - and for TCG we don't need the cpu_synchronize_state() call at all.


Alex

Re: [Qemu-devel] [PATCH] target-ppc: ppc64 target's virtio can be either endian.

[Greg Kurz]

On Mon, 9 Dec 2013 18:18:26 +0100
Alexander Graf <agraf@suse.de> wrote:
> 
> On 09.12.2013, at 18:11, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > On Mon, 9 Dec 2013 16:33:59 +0100
> > Alexander Graf <agraf@suse.de> wrote:
> >> 
> >> On 25.11.2013, at 16:35, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> >> 
> >>> We base it on the OS endian, as reflected by the endianness of the
> >>> interrupt vectors (handled through the ILE bit in the LPCR register).
> >>> 
> >>> This patch does two things:
> >>> - make LPCR a KVM register
> >>> - implement virtio_get_byteswap() over LPCR
> >>> 
> >>> This patch requires to have the following defined in the linux
> >>> headers:
> >>> 
> >>> $ grep LPCR linux-headers/asm-powerpc/kvm.h
> >>> #define KVM_REG_PPC_LPCR        (KVM_REG_PPC | KVM_REG_SIZE_U32 |
> >>> 0xb5)
> >>> 
> >>> Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> >>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >>> ---
> >>> target-ppc/kvm.c         |    4 ++++
> >>> target-ppc/misc_helper.c |   14 ++++++++++++++
> >>> 2 files changed, 18 insertions(+)
> >>> 
> >>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> >>> index 10d0cd9..b450a22 100644
> >>> --- a/target-ppc/kvm.c
> >>> +++ b/target-ppc/kvm.c
> >>> @@ -869,6 +869,8 @@ int kvm_arch_put_registers(CPUState *cs, int
> >>> level) DPRINTF("Warning: Unable to set VPA information to
> >>> KVM\n"); }
> >>>        }
> >>> +
> >>> +        kvm_put_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);
> >>> #endif /* TARGET_PPC64 */
> >>>    }
> >>> 
> >>> @@ -1091,6 +1093,8 @@ int kvm_arch_get_registers(CPUState *cs)
> >>>                DPRINTF("Warning: Unable to get VPA information from
> >>> KVM\n"); }
> >>>        }
> >>> +
> >>> +        kvm_get_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);
> >>> #endif
> >>>    }
> >>> 
> >>> diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
> >>> index 616aab6..0e0743a 100644
> >>> --- a/target-ppc/misc_helper.c
> >>> +++ b/target-ppc/misc_helper.c
> >>> @@ -20,6 +20,8 @@
> >>> #include "helper.h"
> >>> 
> >>> #include "helper_regs.h"
> >>> +#include "hw/virtio/virtio.h"
> >>> +#include "sysemu/kvm.h"
> >>> 
> >>> /*****************************************************************************/
> >>> /* SPR accesses */
> >>> @@ -116,3 +118,15 @@ void ppc_store_msr(CPUPPCState *env, target_ulong
> >>> value) {
> >>>    hreg_store_msr(env, value, 0);
> >>> }
> >>> +
> >>> +bool virtio_get_byteswap(void)
> >>> +{
> >>> +    PowerPCCPU *cp = POWERPC_CPU(first_cpu);
> >>> +    CPUPPCState *env = &cp->env;
> >>> +
> >>> +    if (kvm_enabled()) { 
> >>> +        kvm_arch_get_registers(first_cpu);
> >> 
> >> This function is not defined when CONFIG_KVM is disabled.
> >> 
> >> Please do
> >> 
> >>    cpu_synchronize_state(first_cpu);
> >> 
> >> instead.
> >> 
> > 
> > Oups... I'll fix that.
> > 
> >> Also can't virtio_get_byteswap pass in the CPU pointer of the CPU
> >> that's calling in this moment? I'm not sure how racy it is to
> >> synchronize the first cpu while we're not in the first cpu's execution
> >> thread.
> >> 
> > 
> > I kept the choices made by Rusty in the original serie. According to
> > these, I am not sure if we can use current_cpu:
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg01156.html
> > https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg01504.html
> > 
> >> Either way, if we do this "right" we don't even have to jump through
> >> these hoops, as little endian setting simply happens steered from QEMU,
> >> so QEMU will have all knowledge about the guest's little endian mode
> >> without the need to synchronize any state.
> >> 
> > 
> > Sure. This is just primary work to get virtio working for cross-endian
> > cases. :)
> 
> Yeah, but so far LE switching code is not upstream in KVM - and for TCG
> we don't need the cpu_synchronize_state() call at all.
> 
> 
> Alex
> 

Sure, but for TCG, cpu_synchronize_state() does nothing since KVM is
not enabled... what would be wrong with that ?

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH] target-ppc: ppc64 target's virtio can be either endian.

[Alexander Graf]

> Am 10.12.2013 um 18:37 schrieb Greg Kurz <gkurz@linux.vnet.ibm.com>:
> 
> On Mon, 9 Dec 2013 18:18:26 +0100
> Alexander Graf <agraf@suse.de> wrote:
>> 
>>> On 09.12.2013, at 18:11, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>>> 
>>> On Mon, 9 Dec 2013 16:33:59 +0100
>>> Alexander Graf <agraf@suse.de> wrote:
>>>> 
>>>>> On 25.11.2013, at 16:35, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>>>>> 
>>>>> We base it on the OS endian, as reflected by the endianness of the
>>>>> interrupt vectors (handled through the ILE bit in the LPCR register).
>>>>> 
>>>>> This patch does two things:
>>>>> - make LPCR a KVM register
>>>>> - implement virtio_get_byteswap() over LPCR
>>>>> 
>>>>> This patch requires to have the following defined in the linux
>>>>> headers:
>>>>> 
>>>>> $ grep LPCR linux-headers/asm-powerpc/kvm.h
>>>>> #define KVM_REG_PPC_LPCR        (KVM_REG_PPC | KVM_REG_SIZE_U32 |
>>>>> 0xb5)
>>>>> 
>>>>> Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>>>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>>>>> ---
>>>>> target-ppc/kvm.c         |    4 ++++
>>>>> target-ppc/misc_helper.c |   14 ++++++++++++++
>>>>> 2 files changed, 18 insertions(+)
>>>>> 
>>>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>>>> index 10d0cd9..b450a22 100644
>>>>> --- a/target-ppc/kvm.c
>>>>> +++ b/target-ppc/kvm.c
>>>>> @@ -869,6 +869,8 @@ int kvm_arch_put_registers(CPUState *cs, int
>>>>> level) DPRINTF("Warning: Unable to set VPA information to
>>>>> KVM\n"); }
>>>>>       }
>>>>> +
>>>>> +        kvm_put_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);
>>>>> #endif /* TARGET_PPC64 */
>>>>>   }
>>>>> 
>>>>> @@ -1091,6 +1093,8 @@ int kvm_arch_get_registers(CPUState *cs)
>>>>>               DPRINTF("Warning: Unable to get VPA information from
>>>>> KVM\n"); }
>>>>>       }
>>>>> +
>>>>> +        kvm_get_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);
>>>>> #endif
>>>>>   }
>>>>> 
>>>>> diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
>>>>> index 616aab6..0e0743a 100644
>>>>> --- a/target-ppc/misc_helper.c
>>>>> +++ b/target-ppc/misc_helper.c
>>>>> @@ -20,6 +20,8 @@
>>>>> #include "helper.h"
>>>>> 
>>>>> #include "helper_regs.h"
>>>>> +#include "hw/virtio/virtio.h"
>>>>> +#include "sysemu/kvm.h"
>>>>> 
>>>>> /*****************************************************************************/
>>>>> /* SPR accesses */
>>>>> @@ -116,3 +118,15 @@ void ppc_store_msr(CPUPPCState *env, target_ulong
>>>>> value) {
>>>>>   hreg_store_msr(env, value, 0);
>>>>> }
>>>>> +
>>>>> +bool virtio_get_byteswap(void)
>>>>> +{
>>>>> +    PowerPCCPU *cp = POWERPC_CPU(first_cpu);
>>>>> +    CPUPPCState *env = &cp->env;
>>>>> +
>>>>> +    if (kvm_enabled()) { 
>>>>> +        kvm_arch_get_registers(first_cpu);
>>>> 
>>>> This function is not defined when CONFIG_KVM is disabled.
>>>> 
>>>> Please do
>>>> 
>>>>   cpu_synchronize_state(first_cpu);
>>>> 
>>>> instead.
>>> 
>>> Oups... I'll fix that.
>>> 
>>>> Also can't virtio_get_byteswap pass in the CPU pointer of the CPU
>>>> that's calling in this moment? I'm not sure how racy it is to
>>>> synchronize the first cpu while we're not in the first cpu's execution
>>>> thread.
>>> 
>>> I kept the choices made by Rusty in the original serie. According to
>>> these, I am not sure if we can use current_cpu:
>>> 
>>> https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg01156.html
>>> https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg01504.html
>>> 
>>>> Either way, if we do this "right" we don't even have to jump through
>>>> these hoops, as little endian setting simply happens steered from QEMU,
>>>> so QEMU will have all knowledge about the guest's little endian mode
>>>> without the need to synchronize any state.
>>> 
>>> Sure. This is just primary work to get virtio working for cross-endian
>>> cases. :)
>> 
>> Yeah, but so far LE switching code is not upstream in KVM - and for TCG
>> we don't need the cpu_synchronize_state() call at all.
>> 
>> 
>> Alex
> 
> Sure, but for TCG, cpu_synchronize_state() does nothing since KVM is
> not enabled... what would be wrong with that ?

That it could potentially break kvm.

Alex

> 
> Cheers.
> 
> --
> Greg
> 

Re: [Qemu-devel] [PATCH] target-ppc: ppc64 target's virtio can be either endian.

[Greg Kurz]

On Tue, 10 Dec 2013 18:48:45 +0100
Alexander Graf <agraf@suse.de> wrote:
> >> 
> >> Yeah, but so far LE switching code is not upstream in KVM - and for TCG
> >> we don't need the cpu_synchronize_state() call at all.
> >> 
> >> 
> >> Alex
> > 
> > Sure, but for TCG, cpu_synchronize_state() does nothing since KVM is
> > not enabled... what would be wrong with that ?
> 
> That it could potentially break kvm.
> 

Sorry to bother again but could you elaborate ?

> Alex
> 

--
Greg

Re: [Qemu-devel] [PATCH] target-ppc: ppc64 target's virtio can be either endian.

[Alexander Graf]

On 10.12.2013, at 20:44, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> On Tue, 10 Dec 2013 18:48:45 +0100
> Alexander Graf <agraf@suse.de> wrote:
>>>> 
>>>> Yeah, but so far LE switching code is not upstream in KVM - and for TCG
>>>> we don't need the cpu_synchronize_state() call at all.
>>>> 
>>>> 
>>>> Alex
>>> 
>>> Sure, but for TCG, cpu_synchronize_state() does nothing since KVM is
>>> not enabled... what would be wrong with that ?
>> 
>> That it could potentially break kvm.
>> 
> 
> Sorry to bother again but could you elaborate ?

Hrm, looking through the code it seems like it's safe to call cpu_synchronize_state() on a foreign cpu. Sorry for the fuss.


Alex

[Qemu-devel] [PATCH V2] target-ppc: ppc64 target's virtio can be either endian.

[Greg Kurz]

We base it on the OS endian, as reflected by the endianness of the
interrupt vectors (handled through the ILE bit in the LPCR register).

This patch does two things:
- make LPCR a KVM register
- implement virtio_get_byteswap() over LPCR

Using first_cpu to fetch the registers from KVM may look arbitrary
and awkward, but it is okay because KVM sets/unsets the ILE bit on
all CPUs.

Changes in v2:
- call cpu_synchronize_state() instead of kvm_arch_get_registers().

Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---

Alex,

If you are interested, I can resend the whole virtio+endian serie or
setup a git tree for you to pull from.

--
Greg

 target-ppc/kvm.c         |    4 ++++
 target-ppc/misc_helper.c |   12 ++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 10d0cd9..b450a22 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -869,6 +869,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
                 DPRINTF("Warning: Unable to set VPA information to KVM\n");
             }
         }
+
+        kvm_put_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);
 #endif /* TARGET_PPC64 */
     }
 
@@ -1091,6 +1093,8 @@ int kvm_arch_get_registers(CPUState *cs)
                 DPRINTF("Warning: Unable to get VPA information from KVM\n");
             }
         }
+
+        kvm_get_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);
 #endif
     }
 
diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
index 616aab6..e8fc8a3 100644
--- a/target-ppc/misc_helper.c
+++ b/target-ppc/misc_helper.c
@@ -20,6 +20,8 @@
 #include "helper.h"
 
 #include "helper_regs.h"
+#include "hw/virtio/virtio.h"
+#include "sysemu/kvm.h"
 
 /*****************************************************************************/
 /* SPR accesses */
@@ -116,3 +118,13 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value)
 {
     hreg_store_msr(env, value, 0);
 }
+
+bool virtio_get_byteswap(void)
+{
+    PowerPCCPU *cp = POWERPC_CPU(first_cpu);
+    CPUPPCState *env = &cp->env;
+
+    cpu_synchronize_state(first_cpu);
+
+    return env->spr[SPR_LPCR] & LPCR_ILE;
+}

Re: [Qemu-devel] [PATCH V2] target-ppc: ppc64 target's virtio can be either endian.

[Alexander Graf]

On 11.12.2013, at 16:59, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> We base it on the OS endian, as reflected by the endianness of the
> interrupt vectors (handled through the ILE bit in the LPCR register).
> 
> This patch does two things:
> - make LPCR a KVM register
> - implement virtio_get_byteswap() over LPCR
> 
> Using first_cpu to fetch the registers from KVM may look arbitrary
> and awkward, but it is okay because KVM sets/unsets the ILE bit on
> all CPUs.
> 
> Changes in v2:
> - call cpu_synchronize_state() instead of kvm_arch_get_registers().
> 
> Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
> 
> Alex,
> 
> If you are interested, I can resend the whole virtio+endian serie or
> setup a git tree for you to pull from.

I think this patch should really go in after / with the virtio-endian series, since as it is it would be dead code. So I'll just give you my ack here and hope whoever gets the virtio-endianness bits in also includes that with my reviewed-by line.

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

> 
> --
> Greg
> 
> target-ppc/kvm.c         |    4 ++++
> target-ppc/misc_helper.c |   12 ++++++++++++
> 2 files changed, 16 insertions(+)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 10d0cd9..b450a22 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -869,6 +869,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>                 DPRINTF("Warning: Unable to set VPA information to KVM\n");
>             }
>         }
> +
> +        kvm_put_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);
> #endif /* TARGET_PPC64 */
>     }
> 
> @@ -1091,6 +1093,8 @@ int kvm_arch_get_registers(CPUState *cs)
>                 DPRINTF("Warning: Unable to get VPA information from KVM\n");
>             }
>         }
> +
> +        kvm_get_one_spr(cs, KVM_REG_PPC_LPCR, SPR_LPCR);
> #endif
>     }
> 
> diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
> index 616aab6..e8fc8a3 100644
> --- a/target-ppc/misc_helper.c
> +++ b/target-ppc/misc_helper.c
> @@ -20,6 +20,8 @@
> #include "helper.h"
> 
> #include "helper_regs.h"
> +#include "hw/virtio/virtio.h"
> +#include "sysemu/kvm.h"
> 
> /*****************************************************************************/
> /* SPR accesses */
> @@ -116,3 +118,13 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value)
> {
>     hreg_store_msr(env, value, 0);
> }
> +
> +bool virtio_get_byteswap(void)
> +{
> +    PowerPCCPU *cp = POWERPC_CPU(first_cpu);
> +    CPUPPCState *env = &cp->env;
> +
> +    cpu_synchronize_state(first_cpu);
> +
> +    return env->spr[SPR_LPCR] & LPCR_ILE;
> +}
> 

Re: [Qemu-devel] [PATCH V2] target-ppc: ppc64 target's virtio can be either endian.

[Greg Kurz]

On Wed, 11 Dec 2013 17:11:49 +0100
Alexander Graf <agraf@suse.de> wrote:
> 
> I think this patch should really go in after / with the virtio-endian
> series, since as it is it would be dead code. So I'll just give you my
> ack here and hope whoever gets the virtio-endianness bits in also
> includes that with my reviewed-by line.
> 
> Reviewed-by: Alexander Graf <agraf@suse.de>
> 
> 
> Alex
> 

Thanks for your time Alex.

Anthony,

Would you have time to review the virtio-endian patchset from Rusty and I ?
I have made a bundle out of the patches that were sent to qemu-devel@:

http://patchwork.ozlabs.org/bundle/groug/qemu-virtio-endian/?order=date

The last patch (ppc64 target's virtio can be either endian) is the
enablement for ppc64: it supports both TCG and KVM.

Thanks.

--
Greg

[Qemu-devel] [PATCH] target-ppc: ppc64 target's virtio can be either endian. (v3)

[Greg Kurz]

We base it on the OS endian, as reflected by the endianness of the
interrupt vectors (handled through the ILE bit in the LPCR register).

This patch implements virtio_get_byteswap() over LPCR.

Using first_cpu to fetch the registers from KVM may look arbitrary
and awkward, but it is okay because KVM sets/unsets the ILE bit on
all CPUs.

Changes in v3:
-  dropped the explicit calls to kvm_[get|put]_one_reg as LPCR is
   now properly handled by the generic SPR code.
Changes in v2:
- call cpu_synchronize_state() instead of kvm_arch_get_registers().

Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Reviewed-by: Alexander Graf <agraf@suse.de>
---
 target-ppc/misc_helper.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
index 616aab6..e8fc8a3 100644
--- a/target-ppc/misc_helper.c
+++ b/target-ppc/misc_helper.c
@@ -20,6 +20,8 @@
 #include "helper.h"
 
 #include "helper_regs.h"
+#include "hw/virtio/virtio.h"
+#include "sysemu/kvm.h"
 
 /*****************************************************************************/
 /* SPR accesses */
@@ -116,3 +118,13 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value)
 {
     hreg_store_msr(env, value, 0);
 }
+
+bool virtio_get_byteswap(void)
+{
+    PowerPCCPU *cp = POWERPC_CPU(first_cpu);
+    CPUPPCState *env = &cp->env;
+
+    cpu_synchronize_state(first_cpu);
+
+    return env->spr[SPR_LPCR] & LPCR_ILE;
+}

Re: [Qemu-devel] [PATCH] target-ppc: ppc64 target's virtio can be either endian. (v3)

[Alexander Graf]

On 17.01.2014, at 14:52, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> We base it on the OS endian, as reflected by the endianness of the
> interrupt vectors (handled through the ILE bit in the LPCR register).
> 
> This patch implements virtio_get_byteswap() over LPCR.
> 
> Using first_cpu to fetch the registers from KVM may look arbitrary
> and awkward, but it is okay because KVM sets/unsets the ILE bit on
> all CPUs.
> 
> Changes in v3:
> -  dropped the explicit calls to kvm_[get|put]_one_reg as LPCR is
>   now properly handled by the generic SPR code.
> Changes in v2:
> - call cpu_synchronize_state() instead of kvm_arch_get_registers().
> 
> Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Reviewed-by: Alexander Graf <agraf@suse.de>
> ---
> target-ppc/misc_helper.c |   12 ++++++++++++
> 1 file changed, 12 insertions(+)
> 
> diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
> index 616aab6..e8fc8a3 100644
> --- a/target-ppc/misc_helper.c
> +++ b/target-ppc/misc_helper.c
> @@ -20,6 +20,8 @@
> #include "helper.h"
> 
> #include "helper_regs.h"
> +#include "hw/virtio/virtio.h"
> +#include "sysemu/kvm.h"
> 
> /*****************************************************************************/
> /* SPR accesses */
> @@ -116,3 +118,13 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value)
> {
>     hreg_store_msr(env, value, 0);
> }
> +
> +bool virtio_get_byteswap(void)
> +{
> +    PowerPCCPU *cp = POWERPC_CPU(first_cpu);
> +    CPUPPCState *env = &cp->env;
> +
> +    cpu_synchronize_state(first_cpu);

LPCR_ILE only gets modified by QEMU, so you can even get rid of the synchronize call :).

Either way, this can only go in after the generic virtio byteswap mechanics are merged.


Alex

> +
> +    return env->spr[SPR_LPCR] & LPCR_ILE;
> +}
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target-ppc: ppc64 target's virtio can be either endian.

[Benjamin Herrenschmidt]

On Mon, 2013-12-09 at 16:33 +0100, Alexander Graf wrote:
> 
> Also can't virtio_get_byteswap pass in the CPU pointer of the CPU
> that's calling in this moment? I'm not sure how racy it is to
> synchronize the first cpu while we're not in the first cpu's execution
> thread.

The LPCR bit is always set on all CPUs.

> Either way, if we do this "right" we don't even have to jump through
> these hoops, as little endian setting simply happens steered from
> QEMU, so QEMU will have all knowledge about the guest's little endian
> mode without the need to synchronize any state.

Ben.

Re: [Qemu-devel] [Qemu-ppc] [PATCH] target-ppc: ppc64 target's virtio can be either endian.

[Alexander Graf]

On 09.12.2013, at 20:44, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Mon, 2013-12-09 at 16:33 +0100, Alexander Graf wrote:
>> 
>> Also can't virtio_get_byteswap pass in the CPU pointer of the CPU
>> that's calling in this moment? I'm not sure how racy it is to
>> synchronize the first cpu while we're not in the first cpu's execution
>> thread.
> 
> The LPCR bit is always set on all CPUs.

Yes and no. In HV KVM, yes. With sPAPR, yes. If we want to emulate a host machine, LPCR is a per-core register and may be different.

But that's not the point. The point is that I don't think it's good practice to call cpu_synchronize_state() of a foreign cpu, since it gives me headaches to imagine in what twisted race conditions things could go wrong there.

What happens when the first cpu is executing code while we call the synchronize call? It should lock in vcpu_load(), but that means we're not stuck. So we may have a secondary cpu that waits for the first cpu to exit. But why would it exit at all? It doesn't have to. Maybe it waits inside of the guest to see the virtio probing complete first.

As I said, I don't even want to start imagining what can go wrong.

So instead, I'd prefer if we model it sPAPR'ish: Treat VM-wide hypercalls as such. Handle it in QEMU, save the "LE" state in some machine state and query it from there. Then we're all safe.


Alex