Re: [RFC v9 15/29] vfio: Set up nested stage mappings

[Kunkun Jiang <jiangkunkun@huawei.com>]

Hi Eric,

On 2021/4/11 20:08, Eric Auger wrote:
> In nested mode, legacy vfio_iommu_map_notify cannot be used as
> there is no "caching" mode and we do not trap on map.
>
> On Intel, vfio_iommu_map_notify was used to DMA map the RAM
> through the host single stage.
>
> With nested mode, we need to setup the stage 2 and the stage 1
> separately. This patch introduces a prereg_listener to setup
> the stage 2 mapping.
>
> The stage 1 mapping, owned by the guest, is passed to the host
> when the guest invalidates the stage 1 configuration, through
> a dedicated PCIPASIDOps callback. Guest IOTLB invalidations
> are cascaded downto the host through another IOMMU MR UNMAP
> notifier.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> v7 -> v8:
> - properly handle new IOMMUTLBEntry fields and especially
>    propagate DOMAIN and PASID based invalidations
>
> v6 -> v7:
> - remove PASID based invalidation
>
> v5 -> v6:
> - add error_report_err()
> - remove the abort in case of nested stage case
>
> v4 -> v5:
> - use VFIO_IOMMU_SET_PASID_TABLE
> - use PCIPASIDOps for config notification
>
> v3 -> v4:
> - use iommu_inv_pasid_info for ASID invalidation
>
> v2 -> v3:
> - use VFIO_IOMMU_ATTACH_PASID_TABLE
> - new user API
> - handle leaf
>
> v1 -> v2:
> - adapt to uapi changes
> - pass the asid
> - pass IOMMU_NOTIFIER_S1_CFG when initializing the config notifier
> ---
>   hw/vfio/common.c     | 139 +++++++++++++++++++++++++++++++++++++++++--
>   hw/vfio/pci.c        |  21 +++++++
>   hw/vfio/trace-events |   2 +
>   3 files changed, 157 insertions(+), 5 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 0cd7ef2139..e369d451e7 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -595,6 +595,73 @@ static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
>       return true;
>   }
>   
> +/* Propagate a guest IOTLB invalidation to the host (nested mode) */
> +static void vfio_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> +{
> +    VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
> +    struct vfio_iommu_type1_cache_invalidate ustruct = {};
> +    VFIOContainer *container = giommu->container;
> +    int ret;
> +
> +    assert(iotlb->perm == IOMMU_NONE);
> +
> +    ustruct.argsz = sizeof(ustruct);
> +    ustruct.flags = 0;
> +    ustruct.info.argsz = sizeof(struct iommu_cache_invalidate_info);
> +    ustruct.info.version = IOMMU_CACHE_INVALIDATE_INFO_VERSION_1;
> +    ustruct.info.cache = IOMMU_CACHE_INV_TYPE_IOTLB;
> +
> +    switch (iotlb->granularity) {
> +    case IOMMU_INV_GRAN_DOMAIN:
> +        ustruct.info.granularity = IOMMU_INV_GRANU_DOMAIN;
> +        break;
> +    case IOMMU_INV_GRAN_PASID:
> +    {
> +        struct iommu_inv_pasid_info *pasid_info;
> +        int archid = -1;
> +
> +        pasid_info = &ustruct.info.granu.pasid_info;
> +        ustruct.info.granularity = IOMMU_INV_GRANU_PASID;
> +        if (iotlb->flags & IOMMU_INV_FLAGS_ARCHID) {
> +            pasid_info->flags |= IOMMU_INV_ADDR_FLAGS_ARCHID;
> +            archid = iotlb->arch_id;
> +        }
> +        pasid_info->archid = archid;
> +        trace_vfio_iommu_asid_inv_iotlb(archid);
> +        break;
> +    }
> +    case IOMMU_INV_GRAN_ADDR:
> +    {
> +        hwaddr start = iotlb->iova + giommu->iommu_offset;
> +        struct iommu_inv_addr_info *addr_info;
> +        size_t size = iotlb->addr_mask + 1;
> +        int archid = -1;
> +
> +        addr_info = &ustruct.info.granu.addr_info;
> +        ustruct.info.granularity = IOMMU_INV_GRANU_ADDR;
> +        if (iotlb->leaf) {
> +            addr_info->flags |= IOMMU_INV_ADDR_FLAGS_LEAF;
> +        }
> +        if (iotlb->flags & IOMMU_INV_FLAGS_ARCHID) {
> +            addr_info->flags |= IOMMU_INV_ADDR_FLAGS_ARCHID;
> +            archid = iotlb->arch_id;
> +        }
> +        addr_info->archid = archid;
> +        addr_info->addr = start;
> +        addr_info->granule_size = size;
> +        addr_info->nb_granules = 1;
> +        trace_vfio_iommu_addr_inv_iotlb(archid, start, size,
> +                                        1, iotlb->leaf);
> +        break;
> +    }
Should we pass a size to  host kernel here, even if vSMMU doesn't support
RIL or guest kernel doesn't use RIL?

It will cause TLBI issue in  this scenario: Guest kernel issues a TLBI cmd
without "range" (tg = 0) to invalidate a 2M huge page. Then qemu passed
the iova and size (4K) to host kernel. Finally, host kernel issues a 
TLBI cmd
with "range" (4K) which can not invalidate the TLB entry of 2M huge page.
(pSMMU supports RIL)

Thanks,
Kunkun Jiang
> +    }
> +
> +    ret = ioctl(container->fd, VFIO_IOMMU_CACHE_INVALIDATE, &ustruct);
> +    if (ret) {
> +        error_report("%p: failed to invalidate CACHE (%d)", container, ret);
> +    }
> +}
> +
>   static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>   {
>       VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
> @@ -776,6 +843,35 @@ static void vfio_dma_unmap_ram_section(VFIOContainer *container,
>       }
>   }
>   
> +static void vfio_prereg_listener_region_add(MemoryListener *listener,
> +                                            MemoryRegionSection *section)
> +{
> +    VFIOContainer *container =
> +        container_of(listener, VFIOContainer, prereg_listener);
> +    Error *err = NULL;
> +
> +    if (!memory_region_is_ram(section->mr)) {
> +        return;
> +    }
> +
> +    vfio_dma_map_ram_section(container, section, &err);
> +    if (err) {
> +        error_report_err(err);
> +    }
> +}
> +static void vfio_prereg_listener_region_del(MemoryListener *listener,
> +                                     MemoryRegionSection *section)
> +{
> +    VFIOContainer *container =
> +        container_of(listener, VFIOContainer, prereg_listener);
> +
> +    if (!memory_region_is_ram(section->mr)) {
> +        return;
> +    }
> +
> +    vfio_dma_unmap_ram_section(container, section);
> +}
> +
>   static void vfio_listener_region_add(MemoryListener *listener,
>                                        MemoryRegionSection *section)
>   {
> @@ -879,9 +975,10 @@ static void vfio_listener_region_add(MemoryListener *listener,
>       memory_region_ref(section->mr);
>   
>       if (memory_region_is_iommu(section->mr)) {
> +        IOMMUNotify notify;
>           VFIOGuestIOMMU *giommu;
>           IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
> -        int iommu_idx;
> +        int iommu_idx, flags;
>   
>           trace_vfio_listener_region_add_iommu(iova, end);
>           /*
> @@ -900,8 +997,18 @@ static void vfio_listener_region_add(MemoryListener *listener,
>           llend = int128_sub(llend, int128_one());
>           iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr,
>                                                          MEMTXATTRS_UNSPECIFIED);
> -        iommu_notifier_init(&giommu->n, vfio_iommu_map_notify,
> -                            IOMMU_NOTIFIER_IOTLB_EVENTS,
> +
> +        if (container->iommu_type == VFIO_TYPE1_NESTING_IOMMU) {
> +            /* IOTLB unmap notifier to propagate guest IOTLB invalidations */
> +            flags = IOMMU_NOTIFIER_UNMAP;
> +            notify = vfio_iommu_unmap_notify;
> +        } else {
> +            /* MAP/UNMAP IOTLB notifier */
> +            flags = IOMMU_NOTIFIER_IOTLB_EVENTS;
> +            notify = vfio_iommu_map_notify;
> +        }
> +
> +        iommu_notifier_init(&giommu->n, notify, flags,
>                               section->offset_within_region,
>                               int128_get64(llend),
>                               iommu_idx);
> @@ -921,7 +1028,9 @@ static void vfio_listener_region_add(MemoryListener *listener,
>               goto fail;
>           }
>           QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
> -        memory_region_iommu_replay(giommu->iommu, &giommu->n);
> +        if (flags & IOMMU_NOTIFIER_MAP) {
> +            memory_region_iommu_replay(giommu->iommu, &giommu->n);
> +        }
>   
>           return;
>       }
> @@ -1205,10 +1314,16 @@ static const MemoryListener vfio_memory_listener = {
>       .log_sync = vfio_listener_log_sync,
>   };
>   
> +static MemoryListener vfio_memory_prereg_listener = {
> +    .region_add = vfio_prereg_listener_region_add,
> +    .region_del = vfio_prereg_listener_region_del,
> +};
> +
>   static void vfio_listener_release(VFIOContainer *container)
>   {
>       memory_listener_unregister(&container->listener);
> -    if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
> +    if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU ||
> +        container->iommu_type == VFIO_TYPE1_NESTING_IOMMU) {
>           memory_listener_unregister(&container->prereg_listener);
>       }
>   }
> @@ -1858,6 +1973,20 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>               vfio_get_iommu_info_migration(container, info);
>           }
>           g_free(info);
> +
> +        if (container->iommu_type == VFIO_TYPE1_NESTING_IOMMU) {
> +            container->prereg_listener = vfio_memory_prereg_listener;
> +            memory_listener_register(&container->prereg_listener,
> +                                     &address_space_memory);
> +            if (container->error) {
> +                memory_listener_unregister(&container->prereg_listener);
> +                ret = -1;
> +                error_propagate_prepend(errp, container->error,
> +                                    "RAM memory listener initialization failed "
> +                                    "for container");
> +                goto free_container_exit;
> +            }
> +        }
>           break;
>       }
>       case VFIO_SPAPR_TCE_v2_IOMMU:
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 5c65aa0a98..cad7deec71 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2773,6 +2773,25 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>       vdev->req_enabled = false;
>   }
>   
> +static int vfio_iommu_set_pasid_table(PCIBus *bus, int32_t devfn,
> +                                      IOMMUConfig *config)
> +{
> +    PCIDevice *pdev = bus->devices[devfn];
> +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> +    VFIOContainer *container = vdev->vbasedev.group->container;
> +    struct vfio_iommu_type1_set_pasid_table info;
> +
> +    info.argsz = sizeof(info);
> +    info.flags = VFIO_PASID_TABLE_FLAG_SET;
> +    memcpy(&info.config, &config->pasid_cfg, sizeof(config->pasid_cfg));
> +
> +    return ioctl(container->fd, VFIO_IOMMU_SET_PASID_TABLE, &info);
> +}
> +
> +static PCIPASIDOps vfio_pci_pasid_ops = {
> +    .set_pasid_table = vfio_iommu_set_pasid_table,
> +};
> +
>   static void vfio_realize(PCIDevice *pdev, Error **errp)
>   {
>       VFIOPCIDevice *vdev = VFIO_PCI(pdev);
> @@ -3084,6 +3103,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>       vfio_register_req_notifier(vdev);
>       vfio_setup_resetfn_quirk(vdev);
>   
> +    pci_setup_pasid_ops(pdev, &vfio_pci_pasid_ops);
> +
>       return;
>   
>   out_deregister:
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index 936d29d150..43696afc15 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -120,6 +120,8 @@ vfio_region_sparse_mmap_header(const char *name, int index, int nr_areas) "Devic
>   vfio_region_sparse_mmap_entry(int i, unsigned long start, unsigned long end) "sparse entry %d [0x%lx - 0x%lx]"
>   vfio_get_dev_region(const char *name, int index, uint32_t type, uint32_t subtype) "%s index %d, %08x/%0x8"
>   vfio_dma_unmap_overflow_workaround(void) ""
> +vfio_iommu_addr_inv_iotlb(int asid, uint64_t addr, uint64_t size, uint64_t nb_granules, bool leaf) "nested IOTLB invalidate asid=%d, addr=0x%"PRIx64" granule_size=0x%"PRIx64" nb_granules=0x%"PRIx64" leaf=%d"
> +vfio_iommu_asid_inv_iotlb(int asid) "nested IOTLB invalidate asid=%d"
>   
>   # platform.c
>   vfio_platform_base_device_init(char *name, int groupid) "%s belongs to group #%d"