Re: [RFC PATCH 12/21] i386/xen: set shared_info page

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@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.martins@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.martins@oracle.com>
> > > > Signed-off-by: David Woodhouse <dwmw@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.

> 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 / dgilbert@redhat.com / Manchester, UK