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

[Greg Kurz <gkurz@linux.vnet.ibm.com>]

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