Re: [PATCH] hw/i386/amd_iommu: Allow migration

[Joao Martins <joao.m.martins@oracle.com>]

On 20/11/2024 07:31, Suravee Suthikulpanit wrote:
> Add migration support for AMD IOMMU model by saving necessary AMDVIState
> parameters for MMIO registers, device table, command buffer, and event
> buffers.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  hw/i386/amd_iommu.c | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 13af7211e1..3d2bb9d81e 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1673,7 +1673,41 @@ static Property amdvi_properties[] = {
>  
>  static const VMStateDescription vmstate_amdvi_sysbus = {
>      .name = "amd-iommu",
> -    .unmigratable = 1
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .priority = MIG_PRI_IOMMU,
> +    .fields = (VMStateField[]) {
> +      /* Updated in  amdvi_handle_control_write() */
> +      VMSTATE_BOOL(enabled, AMDVIState),

no xtsup ?  I guess you are relying on the dest command line having xtsup=on
like intel-iommu

> +      VMSTATE_BOOL(ga_enabled, AMDVIState),
> +      VMSTATE_BOOL(ats_enabled, AMDVIState),
> +      VMSTATE_BOOL(cmdbuf_enabled, AMDVIState),
> +      VMSTATE_BOOL(completion_wait_intr, AMDVIState),
> +      VMSTATE_BOOL(evtlog_enabled, AMDVIState),
> +      VMSTATE_BOOL(evtlog_intr, AMDVIState),
> +      /* Updated in amdvi_handle_devtab_write() */
> +      VMSTATE_UINT64(devtab, AMDVIState),
> +      VMSTATE_UINT64(devtab_len, AMDVIState),
> +      /* Updated in amdvi_handle_cmdbase_write() */
> +      VMSTATE_UINT64(cmdbuf, AMDVIState),
> +      VMSTATE_UINT64(cmdbuf_len, AMDVIState),
> +      /* Updated in amdvi_handle_cmdhead_write() */
> +      VMSTATE_UINT32(cmdbuf_head, AMDVIState),
> +      /* Updated in amdvi_handle_cmdtail_write() */
> +      VMSTATE_UINT32(cmdbuf_tail, AMDVIState),
> +      /* Updated in amdvi_handle_evtbase_write() */
> +      VMSTATE_UINT64(evtlog, AMDVIState),
> +      VMSTATE_UINT32(evtlog_len, AMDVIState),
> +      /* Updated in amdvi_handle_evthead_write() */
> +      VMSTATE_UINT32(evtlog_head, AMDVIState),
> +      /* Updated in amdvi_handle_evttail_write() */
> +      VMSTATE_UINT32(evtlog_tail, AMDVIState),

Are we missing:

	ppr_log
	pprlog_len
	pprlog_head
	pprlog_tail

  ?

Although perhaps excluding it was deliberate given that these aren't actually
fed with PPR log entries, only register initialization. Given no PPR entries are
generated it's doing nothing useful.

Out of correctness this is guest initialized data, so perhaps it should be
included such that it doesn't suddenly see different values on destination. In
theory you 'just' need to wire in qemu a VF generating PPR log entries for page
requests, so the log will eventually be what you need to migrate anyhow like the
event log...?

> +      /* MMIO registers */
> +      VMSTATE_UINT8_ARRAY(mmior, AMDVIState, AMDVI_MMIO_SIZE),
> +      VMSTATE_UINT8_ARRAY(romask, AMDVIState, AMDVI_MMIO_SIZE),
> +      VMSTATE_UINT8_ARRAY(w1cmask, AMDVIState, AMDVI_MMIO_SIZE),
> +      VMSTATE_END_OF_LIST()
> +    }
>  };
>  
>  static void amdvi_sysbus_instance_init(Object *klass)