[RFC PATCH 0/3] Add migration support for VFIO PCI devices in SMMUv3 nested stage mode

[RFC PATCH 0/3] Add migration support for VFIO PCI devices in SMMUv3 nested stage mode

[Kunkun Jiang]

Hi all,

Since the SMMUv3's nested translation stages[1] has been introduced by Eric, we
need to pay attention to the migration of VFIO PCI devices in SMMUv3 nested stage
mode. At present, it is not yet supported in QEMU. There are two problems in the
existing framework.

First, the current way to get dirty pages is not applicable to nested stage mode.
Because of the "Caching Mode", VTD can map the RAM through the host single stage
(giova->hpa). "vfio_listener_log_sync" gets dirty pages by transferring "giova"
to the kernel for the RAM block section of mapped MMIO region. In nested stage
mode, we setup the stage 2 (gpa->hpa) and the stage 1(giova->gpa) separately. So
it is inapplicable to get dirty pages by the current way in nested stage mode.

Second, it also need to pass stage 1 configurations to the destination host after
the migration. In Eric's patch, it passes the stage 1 configuration to the host on
each STE update for the devices set the PASID PciOps. The configuration will be
applied at physical level. But the data of physical level will not be sent to the
destination host. So we have to pass stage 1 configurations to the destination
host after the migration.

This Patch set includes patches as below:
Patch 1-2:
- Refactor the vfio_listener_log_sync and added a new function to get dirty pages
in nested stage mode.

Patch 3:
- Added the post_load function to vmstate_smmuv3 for passing stage 1 configuration
to the destination host after the migration.

@Eric, Could you please add this Patch set to your future version of
"vSMMUv3/pSMMUv3 2 stage VFIO integration", if you think this Patch set makes sense? :)

Best Regards
Kunkun Jiang

[1] [RFC,v7,00/26] vSMMUv3/pSMMUv3 2 stage VFIO integration
http://patchwork.ozlabs.org/project/qemu-devel/cover/20201116181349.11908-1-eric.auger@redhat.com/

Kunkun Jiang (3):
  vfio: Introduce helpers to mark dirty pages of a RAM section
  vfio: Add vfio_prereg_listener_log_sync in nested stage
  hw/arm/smmuv3: Post-load stage 1 configurations to the host

 hw/arm/smmuv3.c     | 60 +++++++++++++++++++++++++++++++++++++++++++++
 hw/arm/trace-events |  1 +
 hw/vfio/common.c    | 47 +++++++++++++++++++++++++++++------
 3 files changed, 100 insertions(+), 8 deletions(-)

-- 
2.23.0

[RFC PATCH 1/3] vfio: Introduce helpers to mark dirty pages of a RAM section

[Kunkun Jiang]

Extract part of the code from vfio_sync_dirty_bitmap to form a
new helper, which allows to mark dirty pages of a RAM section.
This helper will be called for nested stage.

Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
---
 hw/vfio/common.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 9225f10722..7c50905856 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1203,6 +1203,19 @@ err_out:
     return ret;
 }
 
+static int vfio_dma_sync_ram_section_dirty_bitmap(VFIOContainer *container,
+                                                  MemoryRegionSection *section)
+{
+    ram_addr_t ram_addr;
+
+    ram_addr = memory_region_get_ram_addr(section->mr) +
+               section->offset_within_region;
+
+    return vfio_get_dirty_bitmap(container,
+                       TARGET_PAGE_ALIGN(section->offset_within_address_space),
+                       int128_get64(section->size), ram_addr);
+}
+
 typedef struct {
     IOMMUNotifier n;
     VFIOGuestIOMMU *giommu;
@@ -1244,8 +1257,6 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
 static int vfio_sync_dirty_bitmap(VFIOContainer *container,
                                   MemoryRegionSection *section)
 {
-    ram_addr_t ram_addr;
-
     if (memory_region_is_iommu(section->mr)) {
         VFIOGuestIOMMU *giommu;
 
@@ -1274,12 +1285,7 @@ static int vfio_sync_dirty_bitmap(VFIOContainer *container,
         return 0;
     }
 
-    ram_addr = memory_region_get_ram_addr(section->mr) +
-               section->offset_within_region;
-
-    return vfio_get_dirty_bitmap(container,
-                       TARGET_PAGE_ALIGN(section->offset_within_address_space),
-                       int128_get64(section->size), ram_addr);
+    return vfio_dma_sync_ram_section_dirty_bitmap(container, section);
 }
 
 static void vfio_listerner_log_sync(MemoryListener *listener,
-- 
2.23.0

[RFC PATCH 2/3] vfio: Add vfio_prereg_listener_log_sync in nested stage

[Kunkun Jiang]

On Intel, the DMA mapped through the host single stage. Instead
we set up the stage 2 and stage 1 separately in nested mode as there
is no "Caching Mode".

Legacy vfio_listener_log_sync cannot be used in nested stage as we
don't need to pay close attention to stage 1 mapping. This patch adds
vfio_prereg_listener_log_sync to mark dirty pages in nested mode.

Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
---
 hw/vfio/common.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 7c50905856..af333e0dee 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1216,6 +1216,22 @@ static int vfio_dma_sync_ram_section_dirty_bitmap(VFIOContainer *container,
                        int128_get64(section->size), ram_addr);
 }
 
+static void vfio_prereg_listener_log_sync(MemoryListener *listener,
+                                          MemoryRegionSection *section)
+{
+    VFIOContainer *container =
+        container_of(listener, VFIOContainer, prereg_listener);
+
+    if (!memory_region_is_ram(section->mr) ||
+        !container->dirty_pages_supported) {
+        return;
+    }
+
+    if (vfio_devices_all_saving(container)) {
+        vfio_dma_sync_ram_section_dirty_bitmap(container, section);
+    }
+}
+
 typedef struct {
     IOMMUNotifier n;
     VFIOGuestIOMMU *giommu;
@@ -1260,6 +1276,14 @@ static int vfio_sync_dirty_bitmap(VFIOContainer *container,
     if (memory_region_is_iommu(section->mr)) {
         VFIOGuestIOMMU *giommu;
 
+        /*
+         * In nested mode, stage 2 and stage 1 are set up separately. We
+         * only need to focus on stage 2 mapping when marking dirty pages.
+         */
+        if (container->iommu_type == VFIO_TYPE1_NESTING_IOMMU) {
+            return 0;
+        }
+
         QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
             if (MEMORY_REGION(giommu->iommu) == section->mr &&
                 giommu->n.start == section->offset_within_region) {
@@ -1312,6 +1336,7 @@ static const MemoryListener vfio_memory_listener = {
 static MemoryListener vfio_memory_prereg_listener = {
     .region_add = vfio_prereg_listener_region_add,
     .region_del = vfio_prereg_listener_region_del,
+    .log_sync = vfio_prereg_listener_log_sync,
 };
 
 static void vfio_listener_release(VFIOContainer *container)
-- 
2.23.0

[RFC PATCH 3/3] hw/arm/smmuv3: Post-load stage 1 configurations to the host

[Kunkun Jiang]

In nested mode, we call the set_pasid_table() callback on each STE
update to pass the guest stage 1 configuration to the host and
apply it at physical level.

In the case of live migration, we need to manual call the
set_pasid_table() to load the guest stage 1 configurations to the
host. If this operation is fail, the migration is fail.

Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
---
 hw/arm/smmuv3.c     | 60 +++++++++++++++++++++++++++++++++++++++++++++
 hw/arm/trace-events |  1 +
 2 files changed, 61 insertions(+)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 6c6ed84e78..94ca15375c 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1473,6 +1473,65 @@ static void smmu_realize(DeviceState *d, Error **errp)
     smmu_init_irq(s, dev);
 }
 
+static int smmuv3_manual_set_pci_device_pasid_table(SMMUDevice *sdev)
+{
+#ifdef __linux__
+    IOMMUMemoryRegion *mr = &(sdev->iommu);
+    int sid = smmu_get_sid(sdev);
+    SMMUEventInfo event = {.type = SMMU_EVT_NONE, .sid = sid,
+                           .inval_ste_allowed = true};
+    IOMMUConfig iommu_config = {};
+    SMMUTransCfg *cfg;
+    int ret = -1;
+
+    cfg = smmuv3_get_config(sdev, &event);
+    if (!cfg) {
+        return ret;
+    }
+
+    iommu_config.pasid_cfg.argsz = sizeof(struct iommu_pasid_table_config);
+    iommu_config.pasid_cfg.version = PASID_TABLE_CFG_VERSION_1;
+    iommu_config.pasid_cfg.format = IOMMU_PASID_FORMAT_SMMUV3;
+    iommu_config.pasid_cfg.base_ptr = cfg->s1ctxptr;
+    iommu_config.pasid_cfg.pasid_bits = 0;
+    iommu_config.pasid_cfg.vendor_data.smmuv3.version = PASID_TABLE_SMMUV3_CFG_VERSION_1;
+
+    if (cfg->disabled || cfg->bypassed) {
+        iommu_config.pasid_cfg.config = IOMMU_PASID_CONFIG_BYPASS;
+    } else if (cfg->aborted) {
+        iommu_config.pasid_cfg.config = IOMMU_PASID_CONFIG_ABORT;
+    } else {
+        iommu_config.pasid_cfg.config = IOMMU_PASID_CONFIG_TRANSLATE;
+    }
+
+    ret = pci_device_set_pasid_table(sdev->bus, sdev->devfn, &iommu_config);
+    if (ret) {
+        error_report("Failed to pass PASID table to host for iommu mr %s (%m)",
+                     mr->parent_obj.name);
+    }
+
+    return ret;
+#endif
+}
+
+static int smmuv3_post_load(void *opaque, int version_id)
+{
+    SMMUv3State *s3 = opaque;
+    SMMUState *s = &(s3->smmu_state);
+    SMMUDevice *sdev;
+    int ret = 0;
+
+    QLIST_FOREACH(sdev, &s->devices_with_notifiers, next) {
+        trace_smmuv3_post_load_sdev(sdev->devfn, sdev->iommu.parent_obj.name);
+        ret = smmuv3_manual_set_pci_device_pasid_table(sdev);
+        if (ret) {
+            break;
+        }
+    }
+
+    return ret;
+}
+
 static const VMStateDescription vmstate_smmuv3_queue = {
     .name = "smmuv3_queue",
     .version_id = 1,
@@ -1491,6 +1550,7 @@ static const VMStateDescription vmstate_smmuv3 = {
     .version_id = 1,
     .minimum_version_id = 1,
     .priority = MIG_PRI_IOMMU,
+    .post_load = smmuv3_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(features, SMMUv3State),
         VMSTATE_UINT8(sid_size, SMMUv3State),
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index 35e562ab74..caa864dd72 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -53,4 +53,5 @@ smmuv3_notify_flag_add(const char *iommu) "ADD SMMUNotifier node for iommu mr=%s
 smmuv3_notify_flag_del(const char *iommu) "DEL SMMUNotifier node for iommu mr=%s"
 smmuv3_inv_notifiers_iova(const char *name, uint16_t asid, uint64_t iova, uint8_t tg, uint64_t num_pages) "iommu mr=%s asid=%d iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64
 smmuv3_notify_config_change(const char *name, uint8_t config, uint64_t s1ctxptr) "iommu mr=%s config=%d s1ctxptr=0x%"PRIx64
+smmuv3_post_load_sdev(int devfn, const char *name) "sdev devfn=%d iommu mr=%s"PRIx64
 
-- 
2.23.0

Re: [RFC PATCH 0/3] Add migration support for VFIO PCI devices in SMMUv3 nested stage mode

[Kunkun Jiang]

kindly ping,
Any comments and reviews are welcome.

Thanks.
Kunkun Jiang.

On 2021/2/19 17:42, Kunkun Jiang wrote:
> Hi all,
>
> Since the SMMUv3's nested translation stages[1] has been introduced by Eric, we
> need to pay attention to the migration of VFIO PCI devices in SMMUv3 nested stage
> mode. At present, it is not yet supported in QEMU. There are two problems in the
> existing framework.
>
> First, the current way to get dirty pages is not applicable to nested stage mode.
> Because of the "Caching Mode", VTD can map the RAM through the host single stage
> (giova->hpa). "vfio_listener_log_sync" gets dirty pages by transferring "giova"
> to the kernel for the RAM block section of mapped MMIO region. In nested stage
> mode, we setup the stage 2 (gpa->hpa) and the stage 1(giova->gpa) separately. So
> it is inapplicable to get dirty pages by the current way in nested stage mode.
>
> Second, it also need to pass stage 1 configurations to the destination host after
> the migration. In Eric's patch, it passes the stage 1 configuration to the host on
> each STE update for the devices set the PASID PciOps. The configuration will be
> applied at physical level. But the data of physical level will not be sent to the
> destination host. So we have to pass stage 1 configurations to the destination
> host after the migration.
>
> This Patch set includes patches as below:
> Patch 1-2:
> - Refactor the vfio_listener_log_sync and added a new function to get dirty pages
> in nested stage mode.
>
> Patch 3:
> - Added the post_load function to vmstate_smmuv3 for passing stage 1 configuration
> to the destination host after the migration.
>
> @Eric, Could you please add this Patch set to your future version of
> "vSMMUv3/pSMMUv3 2 stage VFIO integration", if you think this Patch set makes sense? :)
>
> Best Regards
> Kunkun Jiang
>
> [1] [RFC,v7,00/26] vSMMUv3/pSMMUv3 2 stage VFIO integration
> http://patchwork.ozlabs.org/project/qemu-devel/cover/20201116181349.11908-1-eric.auger@redhat.com/
>
> Kunkun Jiang (3):
>    vfio: Introduce helpers to mark dirty pages of a RAM section
>    vfio: Add vfio_prereg_listener_log_sync in nested stage
>    hw/arm/smmuv3: Post-load stage 1 configurations to the host
>
>   hw/arm/smmuv3.c     | 60 +++++++++++++++++++++++++++++++++++++++++++++
>   hw/arm/trace-events |  1 +
>   hw/vfio/common.c    | 47 +++++++++++++++++++++++++++++------
>   3 files changed, 100 insertions(+), 8 deletions(-)
>

Re: [RFC PATCH 1/3] vfio: Introduce helpers to mark dirty pages of a RAM section

[Auger Eric]

Hi Kunkun,

On 2/19/21 10:42 AM, Kunkun Jiang wrote:
> Extract part of the code from vfio_sync_dirty_bitmap to form a
> new helper, which allows to mark dirty pages of a RAM section.
> This helper will be called for nested stage.
> 
> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> ---
>  hw/vfio/common.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 9225f10722..7c50905856 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1203,6 +1203,19 @@ err_out:
>      return ret;
>  }
>  
> +static int vfio_dma_sync_ram_section_dirty_bitmap(VFIOContainer *container,
> +                                                  MemoryRegionSection *section)
> +{
> +    ram_addr_t ram_addr;
> +
> +    ram_addr = memory_region_get_ram_addr(section->mr) +
> +               section->offset_within_region;
> +
> +    return vfio_get_dirty_bitmap(container,
> +                       TARGET_PAGE_ALIGN(section->offset_within_address_space),
> +                       int128_get64(section->size), ram_addr);
> +}
> +
>  typedef struct {
>      IOMMUNotifier n;
>      VFIOGuestIOMMU *giommu;
> @@ -1244,8 +1257,6 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>  static int vfio_sync_dirty_bitmap(VFIOContainer *container,
>                                    MemoryRegionSection *section)
>  {
> -    ram_addr_t ram_addr;
> -
>      if (memory_region_is_iommu(section->mr)) {
>          VFIOGuestIOMMU *giommu;
>  
> @@ -1274,12 +1285,7 @@ static int vfio_sync_dirty_bitmap(VFIOContainer *container,
>          return 0;
>      }
>  
> -    ram_addr = memory_region_get_ram_addr(section->mr) +
> -               section->offset_within_region;
> -
> -    return vfio_get_dirty_bitmap(container,
> -                       TARGET_PAGE_ALIGN(section->offset_within_address_space),
this is now REAL_HOST_PAGE_ALIGN

Thanks

Eric
> -                       int128_get64(section->size), ram_addr);
> +    return vfio_dma_sync_ram_section_dirty_bitmap(container, section);
>  }
>  
>  static void vfio_listerner_log_sync(MemoryListener *listener,
> 

Re: [RFC PATCH 2/3] vfio: Add vfio_prereg_listener_log_sync in nested stage

[Auger Eric]

Hi Kunkun,

On 2/19/21 10:42 AM, Kunkun Jiang wrote:
> On Intel, the DMA mapped through the host single stage. Instead
> we set up the stage 2 and stage 1 separately in nested mode as there
> is no "Caching Mode".

You need to rewrite the above sentences, Missing ARM and also the 1st
sentences misses a verb.
> 
> Legacy vfio_listener_log_sync cannot be used in nested stage as we
> don't need to pay close attention to stage 1 mapping. This patch adds
> vfio_prereg_listener_log_sync to mark dirty pages in nested mode.
> 
> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> ---
>  hw/vfio/common.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 7c50905856..af333e0dee 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1216,6 +1216,22 @@ static int vfio_dma_sync_ram_section_dirty_bitmap(VFIOContainer *container,
>                         int128_get64(section->size), ram_addr);
>  }
>  
> +static void vfio_prereg_listener_log_sync(MemoryListener *listener,
> +                                          MemoryRegionSection *section)
> +{
> +    VFIOContainer *container =
> +        container_of(listener, VFIOContainer, prereg_listener);
> +
> +    if (!memory_region_is_ram(section->mr) ||
> +        !container->dirty_pages_supported) {
> +        return;
> +    }
> +
> +    if (vfio_devices_all_saving(container)) {
I fail to see where is this defined?
> +        vfio_dma_sync_ram_section_dirty_bitmap(container, section);
> +    }
> +}
> +
>  typedef struct {
>      IOMMUNotifier n;
>      VFIOGuestIOMMU *giommu;
> @@ -1260,6 +1276,14 @@ static int vfio_sync_dirty_bitmap(VFIOContainer *container,
>      if (memory_region_is_iommu(section->mr)) {
>          VFIOGuestIOMMU *giommu;
>  
> +        /*
> +         * In nested mode, stage 2 and stage 1 are set up separately. We
> +         * only need to focus on stage 2 mapping when marking dirty pages.
> +         */
> +        if (container->iommu_type == VFIO_TYPE1_NESTING_IOMMU) {
> +            return 0;
> +        }
> +
>          QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
>              if (MEMORY_REGION(giommu->iommu) == section->mr &&
>                  giommu->n.start == section->offset_within_region) {
> @@ -1312,6 +1336,7 @@ static const MemoryListener vfio_memory_listener = {
>  static MemoryListener vfio_memory_prereg_listener = {
>      .region_add = vfio_prereg_listener_region_add,
>      .region_del = vfio_prereg_listener_region_del,
> +    .log_sync = vfio_prereg_listener_log_sync,
>  };
>  
>  static void vfio_listener_release(VFIOContainer *container)
> 
Thanks

Eric

Re: [RFC PATCH 3/3] hw/arm/smmuv3: Post-load stage 1 configurations to the host

[Auger Eric]

Hi Kunkun,
On 2/19/21 10:42 AM, Kunkun Jiang wrote:
> In nested mode, we call the set_pasid_table() callback on each STE
> update to pass the guest stage 1 configuration to the host and
> apply it at physical level.
> 
> In the case of live migration, we need to manual call the
s/manual/manually
> set_pasid_table() to load the guest stage 1 configurations to the
> host. If this operation is fail, the migration is fail.
s/If this operation is fail, the migration is fail/If this operation
fails, the migration fails.
> 
> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> ---
>  hw/arm/smmuv3.c     | 60 +++++++++++++++++++++++++++++++++++++++++++++
>  hw/arm/trace-events |  1 +
>  2 files changed, 61 insertions(+)
> 
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 6c6ed84e78..94ca15375c 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1473,6 +1473,65 @@ static void smmu_realize(DeviceState *d, Error **errp)
>      smmu_init_irq(s, dev);
>  }
>  
> +static int smmuv3_manual_set_pci_device_pasid_table(SMMUDevice *sdev)
Can't you retrieve the associated sid and then call
smmuv3_notify_config_change()
> +{
> +#ifdef __linux__
> +    IOMMUMemoryRegion *mr = &(sdev->iommu);
> +    int sid = smmu_get_sid(sdev);
> +    SMMUEventInfo event = {.type = SMMU_EVT_NONE, .sid = sid,
> +                           .inval_ste_allowed = true};
> +    IOMMUConfig iommu_config = {};
> +    SMMUTransCfg *cfg;
> +    int ret = -1;
> +
> +    cfg = smmuv3_get_config(sdev, &event);
> +    if (!cfg) {
> +        return ret;
> +    }
> +
> +    iommu_config.pasid_cfg.argsz = sizeof(struct iommu_pasid_table_config);
> +    iommu_config.pasid_cfg.version = PASID_TABLE_CFG_VERSION_1;
> +    iommu_config.pasid_cfg.format = IOMMU_PASID_FORMAT_SMMUV3;
> +    iommu_config.pasid_cfg.base_ptr = cfg->s1ctxptr;
> +    iommu_config.pasid_cfg.pasid_bits = 0;
> +    iommu_config.pasid_cfg.vendor_data.smmuv3.version = PASID_TABLE_SMMUV3_CFG_VERSION_1;
> +
> +    if (cfg->disabled || cfg->bypassed) {
> +        iommu_config.pasid_cfg.config = IOMMU_PASID_CONFIG_BYPASS;
> +    } else if (cfg->aborted) {
> +        iommu_config.pasid_cfg.config = IOMMU_PASID_CONFIG_ABORT;
> +    } else {
> +        iommu_config.pasid_cfg.config = IOMMU_PASID_CONFIG_TRANSLATE;
> +    }
> +
> +    ret = pci_device_set_pasid_table(sdev->bus, sdev->devfn, &iommu_config);
> +    if (ret) {
> +        error_report("Failed to pass PASID table to host for iommu mr %s (%m)",
> +                     mr->parent_obj.name);
> +    }
> +
> +    return ret;
> +#endif
> +}
> +
> +static int smmuv3_post_load(void *opaque, int version_id)
> +{
> +    SMMUv3State *s3 = opaque;
> +    SMMUState *s = &(s3->smmu_state);
> +    SMMUDevice *sdev;
> +    int ret = 0;
> +
> +    QLIST_FOREACH(sdev, &s->devices_with_notifiers, next) {
> +        trace_smmuv3_post_load_sdev(sdev->devfn, sdev->iommu.parent_obj.name);
> +        ret = smmuv3_manual_set_pci_device_pasid_table(sdev);
> +        if (ret) {
> +            break;
> +        }
> +    }
> +
> +    return ret;
> +}
> +
>  static const VMStateDescription vmstate_smmuv3_queue = {
>      .name = "smmuv3_queue",
>      .version_id = 1,
> @@ -1491,6 +1550,7 @@ static const VMStateDescription vmstate_smmuv3 = {
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .priority = MIG_PRI_IOMMU,
> +    .post_load = smmuv3_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(features, SMMUv3State),
>          VMSTATE_UINT8(sid_size, SMMUv3State),
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index 35e562ab74..caa864dd72 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -53,4 +53,5 @@ smmuv3_notify_flag_add(const char *iommu) "ADD SMMUNotifier node for iommu mr=%s
>  smmuv3_notify_flag_del(const char *iommu) "DEL SMMUNotifier node for iommu mr=%s"
>  smmuv3_inv_notifiers_iova(const char *name, uint16_t asid, uint64_t iova, uint8_t tg, uint64_t num_pages) "iommu mr=%s asid=%d iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64
>  smmuv3_notify_config_change(const char *name, uint8_t config, uint64_t s1ctxptr) "iommu mr=%s config=%d s1ctxptr=0x%"PRIx64
> +smmuv3_post_load_sdev(int devfn, const char *name) "sdev devfn=%d iommu mr=%s"PRIx64
>  
> 
Thanks

Eric

Re: [RFC PATCH 0/3] Add migration support for VFIO PCI devices in SMMUv3 nested stage mode

[Auger Eric]

Hi Kunkun,

On 2/19/21 10:42 AM, Kunkun Jiang wrote:
> Hi all,
> 
> Since the SMMUv3's nested translation stages[1] has been introduced by Eric, we
> need to pay attention to the migration of VFIO PCI devices in SMMUv3 nested stage
> mode. At present, it is not yet supported in QEMU. There are two problems in the
> existing framework.
> 
> First, the current way to get dirty pages is not applicable to nested stage mode.
> Because of the "Caching Mode", VTD can map the RAM through the host single stage
> (giova->hpa). "vfio_listener_log_sync" gets dirty pages by transferring "giova"
> to the kernel for the RAM block section of mapped MMIO region. In nested stage
> mode, we setup the stage 2 (gpa->hpa) and the stage 1(giova->gpa) separately. So
> it is inapplicable to get dirty pages by the current way in nested stage mode.
> 
> Second, it also need to pass stage 1 configurations to the destination host after
> the migration. In Eric's patch, it passes the stage 1 configuration to the host on
> each STE update for the devices set the PASID PciOps. The configuration will be
> applied at physical level. But the data of physical level will not be sent to the
> destination host. So we have to pass stage 1 configurations to the destination
> host after the migration.
> 
> This Patch set includes patches as below:
> Patch 1-2:
> - Refactor the vfio_listener_log_sync and added a new function to get dirty pages
> in nested stage mode.
> 
> Patch 3:
> - Added the post_load function to vmstate_smmuv3 for passing stage 1 configuration
> to the destination host after the migration.
> 
> @Eric, Could you please add this Patch set to your future version of
> "vSMMUv3/pSMMUv3 2 stage VFIO integration", if you think this Patch set makes sense? :)
First of all, thank you for working on this. As you may have noticed I
sent a new RFC version yesterday (without including this). When time
allows, you may have a look at the comments I posted on your series. I
don't think I can test it at the moment so I may prefer to keep it
separate. Also be aware that the QEMU integration of nested has not
received much comments yet and is likely to evolve. The priority is to
get some R-b's on the kernel pieces, especially the SMMU part. With this
dependency resolved, things can't move forward I am afraid.

Thanks

Eric
> 
> Best Regards
> Kunkun Jiang
> 
> [1] [RFC,v7,00/26] vSMMUv3/pSMMUv3 2 stage VFIO integration
> http://patchwork.ozlabs.org/project/qemu-devel/cover/20201116181349.11908-1-eric.auger@redhat.com/
> 
> Kunkun Jiang (3):
>   vfio: Introduce helpers to mark dirty pages of a RAM section
>   vfio: Add vfio_prereg_listener_log_sync in nested stage
>   hw/arm/smmuv3: Post-load stage 1 configurations to the host
> 
>  hw/arm/smmuv3.c     | 60 +++++++++++++++++++++++++++++++++++++++++++++
>  hw/arm/trace-events |  1 +
>  hw/vfio/common.c    | 47 +++++++++++++++++++++++++++++------
>  3 files changed, 100 insertions(+), 8 deletions(-)
> 

Re: [RFC PATCH 1/3] vfio: Introduce helpers to mark dirty pages of a RAM section

[Kunkun Jiang]

Hi Eric,

On 2021/4/8 21:46, Auger Eric wrote:
> Hi Kunkun,
>
> On 2/19/21 10:42 AM, Kunkun Jiang wrote:
>> Extract part of the code from vfio_sync_dirty_bitmap to form a
>> new helper, which allows to mark dirty pages of a RAM section.
>> This helper will be called for nested stage.
>>
>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
>> ---
>>   hw/vfio/common.c | 22 ++++++++++++++--------
>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 9225f10722..7c50905856 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1203,6 +1203,19 @@ err_out:
>>       return ret;
>>   }
>>   
>> +static int vfio_dma_sync_ram_section_dirty_bitmap(VFIOContainer *container,
>> +                                                  MemoryRegionSection *section)
>> +{
>> +    ram_addr_t ram_addr;
>> +
>> +    ram_addr = memory_region_get_ram_addr(section->mr) +
>> +               section->offset_within_region;
>> +
>> +    return vfio_get_dirty_bitmap(container,
>> +                       TARGET_PAGE_ALIGN(section->offset_within_address_space),
>> +                       int128_get64(section->size), ram_addr);
>> +}
>> +
>>   typedef struct {
>>       IOMMUNotifier n;
>>       VFIOGuestIOMMU *giommu;
>> @@ -1244,8 +1257,6 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>   static int vfio_sync_dirty_bitmap(VFIOContainer *container,
>>                                     MemoryRegionSection *section)
>>   {
>> -    ram_addr_t ram_addr;
>> -
>>       if (memory_region_is_iommu(section->mr)) {
>>           VFIOGuestIOMMU *giommu;
>>   
>> @@ -1274,12 +1285,7 @@ static int vfio_sync_dirty_bitmap(VFIOContainer *container,
>>           return 0;
>>       }
>>   
>> -    ram_addr = memory_region_get_ram_addr(section->mr) +
>> -               section->offset_within_region;
>> -
>> -    return vfio_get_dirty_bitmap(container,
>> -                       TARGET_PAGE_ALIGN(section->offset_within_address_space),
> this is now REAL_HOST_PAGE_ALIGN
>
> Thanks
>
> Eric
Sorry for late replay.

Yes, it is REAL_HOST_PAGE_ALIGN now which modified by a patch I sent 
earlier.

I posted v2 a few days ago and I have modified TARGET_PAGE_ALIGN to
REAL_HOST_PAGE_ALIGN.[1]

[1] 
https://lore.kernel.org/qemu-devel/20210331101259.2153-2-jiangkunkun@huawei.com/

Thanks,
Kunkun Jiang
>> -                       int128_get64(section->size), ram_addr);
>> +    return vfio_dma_sync_ram_section_dirty_bitmap(container, section);
>>   }
>>   
>>   static void vfio_listerner_log_sync(MemoryListener *listener,
>>
> .

Re: [RFC PATCH 2/3] vfio: Add vfio_prereg_listener_log_sync in nested stage

[Kunkun Jiang]

Hi Eric,

On 2021/4/8 21:56, Auger Eric wrote:
> Hi Kunkun,
>
> On 2/19/21 10:42 AM, Kunkun Jiang wrote:
>> On Intel, the DMA mapped through the host single stage. Instead
>> we set up the stage 2 and stage 1 separately in nested mode as there
>> is no "Caching Mode".
> You need to rewrite the above sentences, Missing ARM and also the 1st
> sentences misses a verb.
Thanks for your review! I will fix it in the next version.
>> Legacy vfio_listener_log_sync cannot be used in nested stage as we
>> don't need to pay close attention to stage 1 mapping. This patch adds
>> vfio_prereg_listener_log_sync to mark dirty pages in nested mode.
>>
>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
>> ---
>>   hw/vfio/common.c | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 7c50905856..af333e0dee 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1216,6 +1216,22 @@ static int vfio_dma_sync_ram_section_dirty_bitmap(VFIOContainer *container,
>>                          int128_get64(section->size), ram_addr);
>>   }
>>   
>> +static void vfio_prereg_listener_log_sync(MemoryListener *listener,
>> +                                          MemoryRegionSection *section)
>> +{
>> +    VFIOContainer *container =
>> +        container_of(listener, VFIOContainer, prereg_listener);
>> +
>> +    if (!memory_region_is_ram(section->mr) ||
>> +        !container->dirty_pages_supported) {
>> +        return;
>> +    }
>> +
>> +    if (vfio_devices_all_saving(container)) {
> I fail to see where is this defined?
Keqian modified vfio_devices_all_saving to vfio_devices_all_dirty_tracking
in 758b96b61d5.

When I posted this series patches, it was vfio_devices_all_saving. In 
v2[1], I
have updated it based on the lasted qemu.

[1] 
https://lore.kernel.org/qemu-devel/20210331101259.2153-3-jiangkunkun@huawei.com/

Thanks,
Kunkun Jiang
>> +        vfio_dma_sync_ram_section_dirty_bitmap(container, section);
>> +    }
>> +}
>> +
>>   typedef struct {
>>       IOMMUNotifier n;
>>       VFIOGuestIOMMU *giommu;
>> @@ -1260,6 +1276,14 @@ static int vfio_sync_dirty_bitmap(VFIOContainer *container,
>>       if (memory_region_is_iommu(section->mr)) {
>>           VFIOGuestIOMMU *giommu;
>>   
>> +        /*
>> +         * In nested mode, stage 2 and stage 1 are set up separately. We
>> +         * only need to focus on stage 2 mapping when marking dirty pages.
>> +         */
>> +        if (container->iommu_type == VFIO_TYPE1_NESTING_IOMMU) {
>> +            return 0;
>> +        }
>> +
>>           QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
>>               if (MEMORY_REGION(giommu->iommu) == section->mr &&
>>                   giommu->n.start == section->offset_within_region) {
>> @@ -1312,6 +1336,7 @@ static const MemoryListener vfio_memory_listener = {
>>   static MemoryListener vfio_memory_prereg_listener = {
>>       .region_add = vfio_prereg_listener_region_add,
>>       .region_del = vfio_prereg_listener_region_del,
>> +    .log_sync = vfio_prereg_listener_log_sync,
>>   };
>>   
>>   static void vfio_listener_release(VFIOContainer *container)
>>
> Thanks
>
> Eric
>
> .

Re: [RFC PATCH 3/3] hw/arm/smmuv3: Post-load stage 1 configurations to the host

[Kunkun Jiang]

Hi Eric,

On 2021/4/12 16:34, Auger Eric wrote:
> Hi Kunkun,
> On 2/19/21 10:42 AM, Kunkun Jiang wrote:
>> In nested mode, we call the set_pasid_table() callback on each STE
>> update to pass the guest stage 1 configuration to the host and
>> apply it at physical level.
>>
>> In the case of live migration, we need to manual call the
> s/manual/manually
You are right.
>> set_pasid_table() to load the guest stage 1 configurations to the
>> host. If this operation is fail, the migration is fail.
> s/If this operation is fail, the migration is fail/If this operation
> fails, the migration fails.
Thanks for your careful review.
>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
>> ---
>>   hw/arm/smmuv3.c     | 60 +++++++++++++++++++++++++++++++++++++++++++++
>>   hw/arm/trace-events |  1 +
>>   2 files changed, 61 insertions(+)
>>
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index 6c6ed84e78..94ca15375c 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -1473,6 +1473,65 @@ static void smmu_realize(DeviceState *d, Error **errp)
>>       smmu_init_irq(s, dev);
>>   }
>>   
>> +static int smmuv3_manual_set_pci_device_pasid_table(SMMUDevice *sdev)
> Can't you retrieve the associated sid and then call
> smmuv3_notify_config_change()
Agreed. It can reuse the code of smmuv3_notify_config_change().
I will modify it in the next version.😁

But I need a return value to judge whether it is successful, which
may need to modify the type of smmuv3_notify_config_change().

Thanks,
Kunkun Jiang
>> +{
>> +#ifdef __linux__
>> +    IOMMUMemoryRegion *mr = &(sdev->iommu);
>> +    int sid = smmu_get_sid(sdev);
>> +    SMMUEventInfo event = {.type = SMMU_EVT_NONE, .sid = sid,
>> +                           .inval_ste_allowed = true};
>> +    IOMMUConfig iommu_config = {};
>> +    SMMUTransCfg *cfg;
>> +    int ret = -1;
>> +
>> +    cfg = smmuv3_get_config(sdev, &event);
>> +    if (!cfg) {
>> +        return ret;
>> +    }
>> +
>> +    iommu_config.pasid_cfg.argsz = sizeof(struct iommu_pasid_table_config);
>> +    iommu_config.pasid_cfg.version = PASID_TABLE_CFG_VERSION_1;
>> +    iommu_config.pasid_cfg.format = IOMMU_PASID_FORMAT_SMMUV3;
>> +    iommu_config.pasid_cfg.base_ptr = cfg->s1ctxptr;
>> +    iommu_config.pasid_cfg.pasid_bits = 0;
>> +    iommu_config.pasid_cfg.vendor_data.smmuv3.version = PASID_TABLE_SMMUV3_CFG_VERSION_1;
>> +
>> +    if (cfg->disabled || cfg->bypassed) {
>> +        iommu_config.pasid_cfg.config = IOMMU_PASID_CONFIG_BYPASS;
>> +    } else if (cfg->aborted) {
>> +        iommu_config.pasid_cfg.config = IOMMU_PASID_CONFIG_ABORT;
>> +    } else {
>> +        iommu_config.pasid_cfg.config = IOMMU_PASID_CONFIG_TRANSLATE;
>> +    }
>> +
>> +    ret = pci_device_set_pasid_table(sdev->bus, sdev->devfn, &iommu_config);
>> +    if (ret) {
>> +        error_report("Failed to pass PASID table to host for iommu mr %s (%m)",
>> +                     mr->parent_obj.name);
>> +    }
>> +
>> +    return ret;
>> +#endif
>> +}
>> +
>> +static int smmuv3_post_load(void *opaque, int version_id)
>> +{
>> +    SMMUv3State *s3 = opaque;
>> +    SMMUState *s = &(s3->smmu_state);
>> +    SMMUDevice *sdev;
>> +    int ret = 0;
>> +
>> +    QLIST_FOREACH(sdev, &s->devices_with_notifiers, next) {
>> +        trace_smmuv3_post_load_sdev(sdev->devfn, sdev->iommu.parent_obj.name);
>> +        ret = smmuv3_manual_set_pci_device_pasid_table(sdev);
>> +        if (ret) {
>> +            break;
>> +        }
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>   static const VMStateDescription vmstate_smmuv3_queue = {
>>       .name = "smmuv3_queue",
>>       .version_id = 1,
>> @@ -1491,6 +1550,7 @@ static const VMStateDescription vmstate_smmuv3 = {
>>       .version_id = 1,
>>       .minimum_version_id = 1,
>>       .priority = MIG_PRI_IOMMU,
>> +    .post_load = smmuv3_post_load,
>>       .fields = (VMStateField[]) {
>>           VMSTATE_UINT32(features, SMMUv3State),
>>           VMSTATE_UINT8(sid_size, SMMUv3State),
>> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
>> index 35e562ab74..caa864dd72 100644
>> --- a/hw/arm/trace-events
>> +++ b/hw/arm/trace-events
>> @@ -53,4 +53,5 @@ smmuv3_notify_flag_add(const char *iommu) "ADD SMMUNotifier node for iommu mr=%s
>>   smmuv3_notify_flag_del(const char *iommu) "DEL SMMUNotifier node for iommu mr=%s"
>>   smmuv3_inv_notifiers_iova(const char *name, uint16_t asid, uint64_t iova, uint8_t tg, uint64_t num_pages) "iommu mr=%s asid=%d iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64
>>   smmuv3_notify_config_change(const char *name, uint8_t config, uint64_t s1ctxptr) "iommu mr=%s config=%d s1ctxptr=0x%"PRIx64
>> +smmuv3_post_load_sdev(int devfn, const char *name) "sdev devfn=%d iommu mr=%s"PRIx64
>>   
>>
> Thanks
>
> Eric
>
> .

Re: [RFC PATCH 0/3] Add migration support for VFIO PCI devices in SMMUv3 nested stage mode

[Kunkun Jiang]

Hi Eric,

On 2021/4/12 16:40, Auger Eric wrote:
> Hi Kunkun,
>
> On 2/19/21 10:42 AM, Kunkun Jiang wrote:
>> Hi all,
>>
>> Since the SMMUv3's nested translation stages[1] has been introduced by Eric, we
>> need to pay attention to the migration of VFIO PCI devices in SMMUv3 nested stage
>> mode. At present, it is not yet supported in QEMU. There are two problems in the
>> existing framework.
>>
>> First, the current way to get dirty pages is not applicable to nested stage mode.
>> Because of the "Caching Mode", VTD can map the RAM through the host single stage
>> (giova->hpa). "vfio_listener_log_sync" gets dirty pages by transferring "giova"
>> to the kernel for the RAM block section of mapped MMIO region. In nested stage
>> mode, we setup the stage 2 (gpa->hpa) and the stage 1(giova->gpa) separately. So
>> it is inapplicable to get dirty pages by the current way in nested stage mode.
>>
>> Second, it also need to pass stage 1 configurations to the destination host after
>> the migration. In Eric's patch, it passes the stage 1 configuration to the host on
>> each STE update for the devices set the PASID PciOps. The configuration will be
>> applied at physical level. But the data of physical level will not be sent to the
>> destination host. So we have to pass stage 1 configurations to the destination
>> host after the migration.
>>
>> This Patch set includes patches as below:
>> Patch 1-2:
>> - Refactor the vfio_listener_log_sync and added a new function to get dirty pages
>> in nested stage mode.
>>
>> Patch 3:
>> - Added the post_load function to vmstate_smmuv3 for passing stage 1 configuration
>> to the destination host after the migration.
>>
>> @Eric, Could you please add this Patch set to your future version of
>> "vSMMUv3/pSMMUv3 2 stage VFIO integration", if you think this Patch set makes sense? :)
> First of all, thank you for working on this. As you may have noticed I
> sent a new RFC version yesterday (without including this). When time
> allows, you may have a look at the comments I posted on your series. I
> don't think I can test it at the moment so I may prefer to keep it
> separate. Also be aware that the QEMU integration of nested has not
> received much comments yet and is likely to evolve. The priority is to
> get some R-b's on the kernel pieces, especially the SMMU part. With this
> dependency resolved, things can't move forward I am afraid.
>
> Thanks
>
> Eric
Yes, I saw your latest version and comments. Thanks again for your review.

I will try my best to test your patches and come up with some useful ideas.
In the future, this series will be updated with your series of nested.
If conditions permit, I hope you can test it and give me some comments.

Best regards,
Kunkun Jiang
>> Best Regards
>> Kunkun Jiang
>>
>> [1] [RFC,v7,00/26] vSMMUv3/pSMMUv3 2 stage VFIO integration
>> http://patchwork.ozlabs.org/project/qemu-devel/cover/20201116181349.11908-1-eric.auger@redhat.com/
>>
>> Kunkun Jiang (3):
>>    vfio: Introduce helpers to mark dirty pages of a RAM section
>>    vfio: Add vfio_prereg_listener_log_sync in nested stage
>>    hw/arm/smmuv3: Post-load stage 1 configurations to the host
>>
>>   hw/arm/smmuv3.c     | 60 +++++++++++++++++++++++++++++++++++++++++++++
>>   hw/arm/trace-events |  1 +
>>   hw/vfio/common.c    | 47 +++++++++++++++++++++++++++++------
>>   3 files changed, 100 insertions(+), 8 deletions(-)
>>
> .