Re: [RFC PATCH 12/21] i386/xen: set shared_info page
On Tue, 2022-12-06 at 10:00 +, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (
> phi...@linaro.org
> ) wrote:
> > +Juan/David/Claudio.
> >
> > On 6/12/22 03:20, David Woodhouse wrote:
> > > On Mon, 2022-12-05 at 23:17 +0100, Philippe Mathieu-Daudé wrote:
> > > > On 5/12/22 18:31, David Woodhouse wrote:
> > > > > From: Joao Martins <
> > > > > joao.m.mart...@oracle.com
> > > > > >
> > > > >
> > > > > This is done by implementing HYPERVISOR_memory_op specifically
> > > > > XENMEM_add_to_physmap with space XENMAPSPACE_shared_info. While
> > > > > Xen removes the page with its own, we instead use the gfn passed
> > > > > by the guest.
> > > > >
> > > > > Signed-off-by: Joao Martins <
> > > > > joao.m.mart...@oracle.com
> > > > > >
> > > > > Signed-off-by: David Woodhouse <
> > > > > d...@amazon.co.uk
> > > > > >
> > > > > ---
> > > > >accel/kvm/kvm-all.c | 6
> > > > >include/hw/core/cpu.h| 2 ++
> > > > >include/sysemu/kvm.h | 2 ++
> > > > >include/sysemu/kvm_int.h | 3 ++
> > > > >target/i386/cpu.h| 8 ++
> > > > >target/i386/trace-events | 1 +
> > > > >target/i386/xen-proto.h | 19 +
> > > > >target/i386/xen.c| 61
> > > > >
> > > > >8 files changed, 102 insertions(+)
> > > > >create mode 100644 target/i386/xen-proto.h
> > > >
> > > >
> > > > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > > > > index 8830546121..e57b693528 100644
> > > > > --- a/include/hw/core/cpu.h
> > > > > +++ b/include/hw/core/cpu.h
> > > > > @@ -443,6 +443,8 @@ struct CPUState {
> > > > >/* track IOMMUs whose translations we've cached in the TCG TLB
> > > > > */
> > > > >GArray *iommu_notifiers;
> > > > > +
> > > > > +struct XenState *xen_state;
> > > >
> > > > Since you define a type definition below, use it.
> > >
> > > Ack.
> > >
> > > More importantly though, some of that state needs to be persisted
> > > across live migration / live update.
> > >
> > > There is per-vCPU state (the GPAs for vcpu_info etc., upcall vector,
> > > timer info). I think I see how I could add that to the vmstate_x86_cpu
> > > defined in target/i386/machine.c.
> > >
> > > For the machine-wide state, where do I add that? Should I just
> > > instantiate a dummy device (a bit like TYPE_KVM_CLOCK, AFAICT) to hang
> > > that state off?
> >
> > XenState in CPUState indeed is an anti-pattern.
> >
> > As you said elsewhere (patch 2 maybe) your use is not a new accelerator
> > but a machine, so new state should go in a derived MachineState.
>
> I *think* the vmstate tends to be attached to a device rather than the
> machinetype itself; eg a PCIe bridge that the Machine instantiates.
> But yes, a 'dummy' type is fine for hanging vmstate off.
Below is an attempt at that. It adds a 'xen-overlay' device which hosts
the memory regions corresponding to "xenheap" pages, which need to be
mapped over guest GPAs on demand.
There's plenty to heckle here, but it basically seems to be working.
I've dumped the state (migrate "exec:cat>foo") and I can see the
correct shinfo_gpa there when the guest was running.
I added the device under hw/xen covered by CONFIG_XEN_EMU, and will
amend the existing shinfo patch to call xen_overlay_map_page() instead
of just *assuming* that there'll already be RAM there... which is true
for Linux guests but Windows uses an empty GFN instead of wasting a
page of real RAM.
There are some target-specific things to be migrated too, so if this
approach is sane then I'll probably add a similar dummy device in
target/i386/xen.c for the system-wide state in *addition* to...
> > Migration is not my area of expertise, but since only the xenfv machine
> > will use this configuration, it seems simpler to store the vCPUs
> > migration states there...
>
> As long as ordering issues don't bite; i.e. between loading the
> new Xen specific stuff and loading the main cpu; you can force
> order using the MIG_PRI_ constants on the .priority field.
>
> I was going to suggest maybe you could add it to vmstate_cpu_common
> as a subsection; but I don't *think* that's used for x86
> (I think that's vmstate_x86_cpu instead???)
... using vmstate_x86_cpu for the per-vCPU state, which seems fairly
straightforward.
---
From 6ac40ff7731bc2144aa7fa4015b9308c2eea8f3d Mon Sep 17 00:00:00 2001
From: David Woodhouse
Date: Wed, 7 Dec 2022 09:19:31 +
Subject: [PATCH] hw/xen: Add xen_overlay device for emulating shared xenheap
pages
For the shared info page and for grant tables, Xen shares its own pages
from the "Xen heap" to the guest. The guest requests that a given page
from a certain address space (XENMAPSPACE_shared_info, etc.) be mapped
to a given GPA using the XENMEM_add_to_physmap hypercall.
To support that in qemu when *emulating* Xen, create a memory region
(migratable) and allow it to be mapped as an overlay when requested.
Xen theoretically allows the same
Re: [RFC PATCH 12/21] i386/xen: set shared_info page
* Philippe Mathieu-Daudé (phi...@linaro.org) wrote:
> +Juan/David/Claudio.
>
> On 6/12/22 03:20, David Woodhouse wrote:
> > On Mon, 2022-12-05 at 23:17 +0100, Philippe Mathieu-Daudé wrote:
> > > On 5/12/22 18:31, David Woodhouse wrote:
> > > > From: Joao Martins
> > > >
> > > > This is done by implementing HYPERVISOR_memory_op specifically
> > > > XENMEM_add_to_physmap with space XENMAPSPACE_shared_info. While
> > > > Xen removes the page with its own, we instead use the gfn passed
> > > > by the guest.
> > > >
> > > > Signed-off-by: Joao Martins
> > > > Signed-off-by: David Woodhouse
> > > > ---
> > > >accel/kvm/kvm-all.c | 6
> > > >include/hw/core/cpu.h| 2 ++
> > > >include/sysemu/kvm.h | 2 ++
> > > >include/sysemu/kvm_int.h | 3 ++
> > > >target/i386/cpu.h| 8 ++
> > > >target/i386/trace-events | 1 +
> > > >target/i386/xen-proto.h | 19 +
> > > >target/i386/xen.c| 61
> > > >
> > > >8 files changed, 102 insertions(+)
> > > >create mode 100644 target/i386/xen-proto.h
> > >
> > >
> > > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > > > index 8830546121..e57b693528 100644
> > > > --- a/include/hw/core/cpu.h
> > > > +++ b/include/hw/core/cpu.h
> > > > @@ -443,6 +443,8 @@ struct CPUState {
> > > >/* track IOMMUs whose translations we've cached in the TCG TLB */
> > > >GArray *iommu_notifiers;
> > > > +
> > > > +struct XenState *xen_state;
> > >
> > > Since you define a type definition below, use it.
> >
> > Ack.
> >
> > More importantly though, some of that state needs to be persisted
> > across live migration / live update.
> >
> > There is per-vCPU state (the GPAs for vcpu_info etc., upcall vector,
> > timer info). I think I see how I could add that to the vmstate_x86_cpu
> > defined in target/i386/machine.c.
> >
> > For the machine-wide state, where do I add that? Should I just
> > instantiate a dummy device (a bit like TYPE_KVM_CLOCK, AFAICT) to hang
> > that state off?
>
> XenState in CPUState indeed is an anti-pattern.
>
> As you said elsewhere (patch 2 maybe) your use is not a new accelerator
> but a machine, so new state should go in a derived MachineState.
I *think* the vmstate tends to be attached to a device rather than the
machinetype itself; eg a PCIe bridge that the Machine instantiates.
But yes, a 'dummy' type is fine for hanging vmstate off.
> Migration is not my area of expertise, but since only the xenfv machine
> will use this configuration, it seems simpler to store the vCPUs
> migration states there...
As long as ordering issues don't bite; i.e. between loading the
new Xen specific stuff and loading the main cpu; you can force
order using the MIG_PRI_ constants on the .priority field.
I was going to suggest maybe you could add it to vmstate_cpu_common
as a subsection; but I don't *think* that's used for x86
(I think that's vmstate_x86_cpu instead???)
Dave
> Regards,
>
> Phil.
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [RFC PATCH 12/21] i386/xen: set shared_info page
+Juan/David/Claudio.
On 6/12/22 03:20, David Woodhouse wrote:
On Mon, 2022-12-05 at 23:17 +0100, Philippe Mathieu-Daudé wrote:
On 5/12/22 18:31, David Woodhouse wrote:
From: Joao Martins
This is done by implementing HYPERVISOR_memory_op specifically
XENMEM_add_to_physmap with space XENMAPSPACE_shared_info. While
Xen removes the page with its own, we instead use the gfn passed
by the guest.
Signed-off-by: Joao Martins
Signed-off-by: David Woodhouse
---
accel/kvm/kvm-all.c | 6
include/hw/core/cpu.h| 2 ++
include/sysemu/kvm.h | 2 ++
include/sysemu/kvm_int.h | 3 ++
target/i386/cpu.h| 8 ++
target/i386/trace-events | 1 +
target/i386/xen-proto.h | 19 +
target/i386/xen.c| 61
8 files changed, 102 insertions(+)
create mode 100644 target/i386/xen-proto.h
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 8830546121..e57b693528 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -443,6 +443,8 @@ struct CPUState {
/* track IOMMUs whose translations we've cached in the TCG TLB */
GArray *iommu_notifiers;
+
+struct XenState *xen_state;
Since you define a type definition below, use it.
Ack.
More importantly though, some of that state needs to be persisted
across live migration / live update.
There is per-vCPU state (the GPAs for vcpu_info etc., upcall vector,
timer info). I think I see how I could add that to the vmstate_x86_cpu
defined in target/i386/machine.c.
For the machine-wide state, where do I add that? Should I just
instantiate a dummy device (a bit like TYPE_KVM_CLOCK, AFAICT) to hang
that state off?
XenState in CPUState indeed is an anti-pattern.
As you said elsewhere (patch 2 maybe) your use is not a new accelerator
but a machine, so new state should go in a derived MachineState.
Migration is not my area of expertise, but since only the xenfv machine
will use this configuration, it seems simpler to store the vCPUs
migration states there...
Regards,
Phil.
Re: [RFC PATCH 12/21] i386/xen: set shared_info page
On Mon, 2022-12-05 at 23:17 +0100, Philippe Mathieu-Daudé wrote:
> On 5/12/22 18:31, David Woodhouse wrote:
> > From: Joao Martins
> >
> > This is done by implementing HYPERVISOR_memory_op specifically
> > XENMEM_add_to_physmap with space XENMAPSPACE_shared_info. While
> > Xen removes the page with its own, we instead use the gfn passed
> > by the guest.
> >
> > Signed-off-by: Joao Martins
> > Signed-off-by: David Woodhouse
> > ---
> > accel/kvm/kvm-all.c | 6
> > include/hw/core/cpu.h| 2 ++
> > include/sysemu/kvm.h | 2 ++
> > include/sysemu/kvm_int.h | 3 ++
> > target/i386/cpu.h| 8 ++
> > target/i386/trace-events | 1 +
> > target/i386/xen-proto.h | 19 +
> > target/i386/xen.c| 61
> > 8 files changed, 102 insertions(+)
> > create mode 100644 target/i386/xen-proto.h
>
>
> > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > index 8830546121..e57b693528 100644
> > --- a/include/hw/core/cpu.h
> > +++ b/include/hw/core/cpu.h
> > @@ -443,6 +443,8 @@ struct CPUState {
> >
> > /* track IOMMUs whose translations we've cached in the TCG TLB */
> > GArray *iommu_notifiers;
> > +
> > +struct XenState *xen_state;
>
> Since you define a type definition below, use it.
Ack.
More importantly though, some of that state needs to be persisted
across live migration / live update.
There is per-vCPU state (the GPAs for vcpu_info etc., upcall vector,
timer info). I think I see how I could add that to the vmstate_x86_cpu
defined in target/i386/machine.c.
For the machine-wide state, where do I add that? Should I just
instantiate a dummy device (a bit like TYPE_KVM_CLOCK, AFAICT) to hang
that state off?
smime.p7s
Description: S/MIME cryptographic signature
Re: [RFC PATCH 12/21] i386/xen: set shared_info page
On 5/12/22 18:31, David Woodhouse wrote:
From: Joao Martins
This is done by implementing HYPERVISOR_memory_op specifically
XENMEM_add_to_physmap with space XENMAPSPACE_shared_info. While
Xen removes the page with its own, we instead use the gfn passed
by the guest.
Signed-off-by: Joao Martins
Signed-off-by: David Woodhouse
---
accel/kvm/kvm-all.c | 6
include/hw/core/cpu.h| 2 ++
include/sysemu/kvm.h | 2 ++
include/sysemu/kvm_int.h | 3 ++
target/i386/cpu.h| 8 ++
target/i386/trace-events | 1 +
target/i386/xen-proto.h | 19 +
target/i386/xen.c| 61
8 files changed, 102 insertions(+)
create mode 100644 target/i386/xen-proto.h
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 8830546121..e57b693528 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -443,6 +443,8 @@ struct CPUState {
/* track IOMMUs whose translations we've cached in the TCG TLB */
GArray *iommu_notifiers;
+
+struct XenState *xen_state;
Since you define a type definition below, use it.
};
typedef QTAILQ_HEAD(CPUTailQ, CPUState) CPUTailQ;
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index e9a97eda8c..8e882fbe96 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -582,4 +582,6 @@ bool kvm_arch_cpu_check_are_resettable(void);
bool kvm_dirty_ring_enabled(void);
uint32_t kvm_dirty_ring_size(void);
+
+struct XenState *kvm_get_xen_state(KVMState *s);
Ditto.
#endif
diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
index 3b4adcdc10..0d89cfe273 100644
--- a/include/sysemu/kvm_int.h
+++ b/include/sysemu/kvm_int.h
@@ -110,6 +110,9 @@ struct KVMState
struct KVMDirtyRingReaper reaper;
NotifyVmexitOption notify_vmexit;
uint32_t notify_window;
+
+/* xen guest state */
+struct XenState xen;
Ditto.
};
void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 5ddd14467e..09c0281b8b 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -23,6 +23,14 @@
#include "sysemu/tcg.h"
#include "cpu-qom.h"
#include "kvm/hyperv-proto.h"
+#include "xen-proto.h"
+
+#ifdef TARGET_X86_64
+#define TARGET_LONG_BITS 64
+#else
+#define TARGET_LONG_BITS 32
+#endif
How come you don't have access to the definitions from "cpu-param.h" here?
Regards,
Phil.
