Re: [Qemu-devel] [RFC] QEMU/KVM PowerPC: virtio and guest endianness

Re: [Qemu-devel] [RFC] QEMU/KVM PowerPC: virtio and guest endianness

[Alexander Graf]

CC'ing qemu-devel - please use qemu-ppc@ only as a tag, every mail needs to go to qemu-devel as well.

On 03.10.2013, at 16:29, Greg Kurz wrote:

> Hi,
> 
> There have been some work on the topic lately but no agreement has
> been reached yet. I want to consolidate the facts in a single thread of
> mail and re-start the discussion. Please find below a recap of what we
> have as of today:
> 
> From a virtio POV, guest endianness is reflected by the endianness of
> the interrupt vectors (ILE bit in the LPCR register). The guest kernel
> relies on the H_SET_MODE_RESOURCE_LE hcall to set this bit, early in the
> boot process.
> 
> Rusty sent a patchset on qemu-devel@ to provide the necessary bits to
> perform byteswap in the QEMU:
> 
> http://patchwork.ozlabs.org/patch/266451/
> http://patchwork.ozlabs.org/patch/266452/
> http://patchwork.ozlabs.org/patch/266450/
> (plus other enablement patches for virtio drivers, not essential for
> the discussion).
> 
> In non-KVM mode, QEMU implements the H_SET_MODE_RESOURCE_LE and updates
> its internal value for LPCR when the guest requests it. Rusty's patchset
> works out-of-the-box in this mode: I could successfully setup and use a
> 9p share over virtio transport (broader virtio testing still to be done
> though).
> 
> When using KVM, the story is different : QEMU is not on this
> endianness change flow anymore, providing KVM has the following
> patch from Anton:
> 
> http://patchwork.ozlabs.org/patch/277079/
> 
> There are *at least* two approaches to bring back endianness knowledge
> to QEMU: polling (1) and propagation (2).
> 
> (1) QEMU must retrieve LPCR from the kernel using the following API:
> 
> http://patchwork.ozlabs.org/patch/273029/
> 
> (2) KVM can resume execution to the host and thus propagating
> H_SET_MODE_RESOURCE_LE to QEMU. Laurent came up with a patch on
> linuxppc-dev@ to do this:
> 
> http://patchwork.ozlabs.org/patch/278590/
> 
> I would say (1) is a standard and sane way of addressing the issue:
> since the LPCR register value is held by KVM, it makes sense to
> introduce an API to get/set it. Then, it is up to QEMU to use this API.
> 
> We can dumbly do the polling in all the places where byteswapping
> matters: it is clearly sub-optimized, especially since the LPCR_ILE bit
> doesn't change so often. Rusty suggested we can retrieve it at virtio
> device reset time and cache it, since an endianness change after the
> devices have started to be used is non-sensical.
> 
> I have searched for an appropriate place to add the polling and I must
> admit I did not find any... I am no QEMU expert but I suspect we would
> need some kind of arch specific hook to be called from the virtio code
> to do this... :-\ I hope I am wrong, please correct me if so.

Just put it into the normal register sync function and call cpu_synchronize_state() on virtio reset.

> On the other hand, (2) looks a bit hacky: KVM usually returns to the
> host when it cannot fully handle the h_call. Propagating may look like
> a useless path to follow from a KVM POV. From a QEMU POV, things are
> different: propagation will trig the fallback code in QEMU, already
> working in non-KVM mode. Nothing more to be done.

We have to decide which scheme to follow. There are 2 way we can / should handle registers usually:

  a) owned by QEMU
  b) owned by KVM

If they're owned by QEMU, every hypercall needs to go into QEMU which then propagates that change through an ioctl back into KVM.
If they're owned by KVM, QEMU needs to fetch them whenever it needs to

As a general rule of thumb path b is easier to hack up, path a is easier to maintain long term. Which is pretty much what you're seeing here.

> I have a better feeling for (2) because:
> - 2-liner patch in KVM
> - no extra code change in QEMU
> - already *partially* tested

I don't understand. QEMU would get triggered, then have to propagate things back into KVM. We definitely do _not_ want KVM to do magic, then tell QEMU to handle a hypercall again.

> Also, I understood Rusty is working on the next virtio specification
> which should address the endian issue: probably not worth to add too
> many temporary lines in the QEMU code...

Does 3.13 support LE mode? Does 3.13 support the new and shiny virtio spec? There's a good chance we'd have to deal with guest kernels that can do LE, but not sane virtio.

> Of course, I probably lack some essential knowledge that would be
> more favorable to (1)... so please comment and argue ! :)

I think a 100% QEMU implementation that just goes through all vcpus and does a simple SET_ONE_REG for LPCR to set ILE would be the best. Anton's patch isn't in Linus' tree yet, right? So all it takes is a partial revert of that one to not handle the actual hypercall in KVM. And some code in kvmppc_set_lpcr() to also set intr_msr (not changing it is a bug today already).


Alex

Re: [Qemu-devel] [Qemu-ppc] [RFC] QEMU/KVM PowerPC: virtio and guest endianness

[Greg Kurz]

On Fri, 4 Oct 2013 13:43:38 +0200
Alexander Graf <agraf@suse.de> wrote:

> CC'ing qemu-devel - please use qemu-ppc@ only as a tag, every mail
> needs to go to qemu-devel as well.
> 

Sure I will.

> On 03.10.2013, at 16:29, Greg Kurz wrote:
> [...]
> > I have searched for an appropriate place to add the polling and I
> > must admit I did not find any... I am no QEMU expert but I suspect
> > we would need some kind of arch specific hook to be called from the
> > virtio code to do this... :-\ I hope I am wrong, please correct me
> > if so.
> 
> Just put it into the normal register sync function and call
> cpu_synchronize_state() on virtio reset.
> 

Thanks, I will look into that.

> 
> We have to decide which scheme to follow. There are 2 way we can /
> should handle registers usually:
> 
>   a) owned by QEMU
>   b) owned by KVM
> 
> If they're owned by QEMU, every hypercall needs to go into QEMU which
> then propagates that change through an ioctl back into KVM. If
> they're owned by KVM, QEMU needs to fetch them whenever it needs to
> 
> As a general rule of thumb path b is easier to hack up, path a is
> easier to maintain long term. Which is pretty much what you're seeing
> here.
> 

Agreed.

> > I have a better feeling for (2) because:
> > - 2-liner patch in KVM
> > - no extra code change in QEMU
> > - already *partially* tested
> 
> I don't understand. QEMU would get triggered, then have to propagate
> things back into KVM. We definitely do _not_ want KVM to do magic,
> then tell QEMU to handle a hypercall again.
> 

My idea was to have KVM and QEMU be like *co-owners* of the LPCR
register... now I admit it was a bad idea ! :)

> > Also, I understood Rusty is working on the next virtio specification
> > which should address the endian issue: probably not worth to add too
> > many temporary lines in the QEMU code...
> 
> Does 3.13 support LE mode? Does 3.13 support the new and shiny virtio
> spec? There's a good chance we'd have to deal with guest kernels that
> can do LE, but not sane virtio.
> 

I don't know any details about 3.13 but I guess you are right, it is
unlikely the guests will have it.

> > Of course, I probably lack some essential knowledge that would be
> > more favorable to (1)... so please comment and argue ! :)
> 
> I think a 100% QEMU implementation that just goes through all vcpus
> and does a simple SET_ONE_REG for LPCR to set ILE would be the best.
> Anton's patch isn't in Linus' tree yet, right? So all it takes is a
> partial revert of that one to not handle the actual hypercall in KVM.
> And some code in kvmppc_set_lpcr() to also set intr_msr (not changing
> it is a bug today already).
> 
> 
> Alex
> 
> 

Thanks for your indications Alex.

Cheers.

--
Greg

Re: [Qemu-devel] [Qemu-ppc] [RFC] QEMU/KVM PowerPC: virtio and guest endianness

[Greg Kurz]

Answering to both Paul and Alex.

On Fri, 4 Oct 2013 13:54:25 +0200
Alexander Graf <agraf@suse.de> wrote:

> 
> On 04.10.2013, at 13:53, Paul Mackerras wrote:
> > 
> > I don't mind particularly whether H_SET_MODE for the endianness
> > setting gets handled in the kernel or in QEMU, but I don't think it
> > should be handled in both.  If you want QEMU to know about the
> > endianness setting immediately, make the kernel version do nothing
> > and get QEMU to handle it -- which if KVM is enabled will mean
> > iterating over all vcpus and getting them all to send the new LPCR
> > setting to the kernel via the SET_ONE_REG ioctl.
> > 
> > However, I want the setting of breakpoint registers (CIABR and
> > DAWR/X) via H_SET_MODE to happen in the kernel, preferably in real
> > mode, since that can happen on context switch and thus needs to be
> > quick.
> 

Paul,

As far as virtio is concerned, QEMU only needs to know about the guest
endiannes if a virtio device shows up. The virtio reset flow is a
good candiadate for that.

> I don't want to see a single hypercall be split across the QEMU/KVM
> barrier. So if there's a reasonable incentive to handle H_SET_MODE in
> KVM, we should handle all of it in KVM.
> 

Alex,

The appropriate solution would be then to let KVM implement the whole
H_SET_MODE hcall and own LPCR. QEMU will poll it with cpu_synchronize_state().
It seems to preserve all the requirements.

> 
> Alex
> 
> --

Thanks for your guidance.

Cheers.

--
Greg

Re: [Qemu-devel] [Qemu-ppc] [RFC] QEMU/KVM PowerPC: virtio and guest endianness

[Alexander Graf]

On 04.10.2013, at 16:08, Greg Kurz wrote:

> Answering to both Paul and Alex.
> 
> On Fri, 4 Oct 2013 13:54:25 +0200
> Alexander Graf <agraf@suse.de> wrote:
> 
>> 
>> On 04.10.2013, at 13:53, Paul Mackerras wrote:
>>> 
>>> I don't mind particularly whether H_SET_MODE for the endianness
>>> setting gets handled in the kernel or in QEMU, but I don't think it
>>> should be handled in both.  If you want QEMU to know about the
>>> endianness setting immediately, make the kernel version do nothing
>>> and get QEMU to handle it -- which if KVM is enabled will mean
>>> iterating over all vcpus and getting them all to send the new LPCR
>>> setting to the kernel via the SET_ONE_REG ioctl.
>>> 
>>> However, I want the setting of breakpoint registers (CIABR and
>>> DAWR/X) via H_SET_MODE to happen in the kernel, preferably in real
>>> mode, since that can happen on context switch and thus needs to be
>>> quick.
>> 
> 
> Paul,
> 
> As far as virtio is concerned, QEMU only needs to know about the guest
> endiannes if a virtio device shows up. The virtio reset flow is a
> good candiadate for that.
> 
>> I don't want to see a single hypercall be split across the QEMU/KVM
>> barrier. So if there's a reasonable incentive to handle H_SET_MODE in
>> KVM, we should handle all of it in KVM.
>> 
> 
> Alex,
> 
> The appropriate solution would be then to let KVM implement the whole
> H_SET_MODE hcall and own LPCR. QEMU will poll it with cpu_synchronize_state().
> It seems to preserve all the requirements.

Yes. Since breakpoint registers are part of H_SET_MODE, we want to have it owned by KVM rather than QEMU. I still don't know what those PAPR people think they're doing, shoving completely unrelated things into the same hypercall though :).


Alex

[Qemu-devel] [PATCH 0/2] virtio: guest endianness support

[Greg Kurz]

This patchset is a followup to Rusty's "virtio for endian curious guests" serie:

https://lists.nongnu.org/archive/html/qemu-devel/2013-08/msg01502.html

It brings guest endianness knowledge to the virtio drivers when running in
KVM mode, on PowerPC.

The first patch is only here to have KVM_REG_PPC_LPCR defined. It is not
needed unless you wish to build and do not already have KVM_REG_PPC_LPCR in
linux-headers/asm-powerpc/kvm.h. The interesting code is in the second patch.

Cheers.

---

Greg Kurz (2):
      linux-headers: POWER8 partial update
      virtio: refresh registers at reset time


 hw/virtio/virtio.c              |    5 +++++
 linux-headers/asm-powerpc/kvm.h |    3 +++
 target-ppc/kvm.c                |    4 ++++
 3 files changed, 12 insertions(+)

-- 
Greg Kurz

[Qemu-devel] [PATCH 1/2] linux-headers: POWER8 partial update

[Greg Kurz]

Add definition for KVM_REG_PPC_LPCR, taken from:

https://github.com/agraf/linux-2.6/commit/1a87967d4c

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 linux-headers/asm-powerpc/kvm.h |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/linux-headers/asm-powerpc/kvm.h b/linux-headers/asm-powerpc/kvm.h
index 0fb1a6e..bb5b4ce 100644
--- a/linux-headers/asm-powerpc/kvm.h
+++ b/linux-headers/asm-powerpc/kvm.h
@@ -499,6 +499,9 @@ struct kvm_get_htab_header {
 #define KVM_REG_PPC_TLB3PS	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x9a)
 #define KVM_REG_PPC_EPTCFG	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0x9b)
 
+/* POWER8 registers */
+#define KVM_REG_PPC_LPCR        (KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xb5)
+
 /* PPC64 eXternal Interrupt Controller Specification */
 #define KVM_DEV_XICS_GRP_SOURCES	1	/* 64-bit source attributes */
 

[Qemu-devel] [PATCH 2/2] virtio: refresh registers at reset time

[Greg Kurz]

We need to support the guest endianness as soon as a virtio device shows
up. Alex suggested this can achieved by calling cpu_synchronize_state().

To have it working on PowerPC, we need to add LPCR in the sync register
functions.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/virtio.c |    5 +++++
 target-ppc/kvm.c   |    4 ++++
 2 files changed, 9 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index bc728d8..4a294e1 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -19,6 +19,7 @@
 #include "qemu/atomic.h"
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/virtio-access.h"
+#include "sysemu/kvm.h"
 
 /*
  * The alignment to use between consumer and producer parts of vring.
@@ -566,6 +567,10 @@ void virtio_reset(void *opaque)
         vdev->vq[i].signalled_used_valid = false;
         vdev->vq[i].notification = true;
     }
+
+    if (current_cpu) {
+        cpu_synchronize_state(current_cpu);
+    }
 }
 
 uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr)
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index b77ce5e..69ebe2a 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
     }
 

Re: [Qemu-devel] [PATCH 2/2] virtio: refresh registers at reset time

[Rusty Russell]

Greg Kurz <gkurz@linux.vnet.ibm.com> writes:
> We need to support the guest endianness as soon as a virtio device shows
> up. Alex suggested this can achieved by calling cpu_synchronize_state().
>
> To have it working on PowerPC, we need to add LPCR in the sync register
> functions.
>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

Excellent!  Alex, if you take this, I'll be happy to rebase and re-test
the virtio endianness patches on top.

Cheers,
Rusty.