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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

+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.

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.