Re: [Qemu-devel] [PATCH v7 3/5] IOMMU: enable intel_iommu map and unmap notifiers

[Liu, Yi L]

> From: "Aviv Ben-David" <address@hidden>
> 
> Adds a list of registered vtd_as's to intel iommu state to save
> iteration over each PCI device in a search of the corrosponding domain.
> 
> Signed-off-by: Aviv Ben-David <address@hidden>
> ---
>  hw/i386/intel_iommu.c          | 94 
> ++++++++++++++++++++++++++++++++++++++----
>  hw/i386/intel_iommu_internal.h |  2 +
>  include/hw/i386/intel_iommu.h  |  9 ++++
>  3 files changed, 98 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 05973b9..d872969 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -679,7 +679,7 @@ static int vtd_gpa_to_slpte(VTDContextEntry *ce, uint64_t 
> gpa,
>          }
>          *reads = (*reads) && (slpte & VTD_SL_R);
>          *writes = (*writes) && (slpte & VTD_SL_W);
> -        if (!(slpte & access_right_check)) {
> +        if (!(slpte & access_right_check) && !(flags & IOMMU_NO_FAIL)) {
>              VTD_DPRINTF(GENERAL, "error: lack of %s permission for "
>                          "gpa 0x%"PRIx64 " slpte 0x%"PRIx64,
>                          (flags & IOMMU_WO ? "write" : "read"), gpa, slpte);
> @@ -978,6 +978,23 @@ static VTDBus *vtd_find_as_from_bus_num(IntelIOMMUState 
> *s, uint8_t bus_num)
>      return vtd_bus;
>  }
>  
> +static int vtd_get_did_dev(IntelIOMMUState *s, uint8_t bus_num, uint8_t 
> devfn,
> +                           uint16_t *domain_id)
> +{
> +    VTDContextEntry ce;
> +    int ret_fr;
> +
> +    assert(domain_id);
> +
> +    ret_fr = vtd_dev_to_context_entry(s, bus_num, devfn, &ce);
> +    if (ret_fr) {
> +        return -1;
> +    }
> +
> +    *domain_id =  VTD_CONTEXT_ENTRY_DID(ce.hi);
> +    return 0;
> +}
> +
>  /* Do a context-cache device-selective invalidation.
>   * @func_mask: FM field after shifting
>   */
> @@ -1064,6 +1081,45 @@ static void 
> vtd_iotlb_domain_invalidate(IntelIOMMUState 
> *s, uint16_t domain_id)
>                                  &domain_id);
>  }
>  
> +static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
> +                                           uint16_t domain_id, hwaddr addr,
> +                                           uint8_t am)
> +{
> +    IntelIOMMUNotifierNode *node;
> +
> +    QLIST_FOREACH(node, &(s->notifiers_list), next) {
Aviv,

Regards to the s->notifiers_list, I didn't see the init op to it. Does it happen
in another patch? If so, it may be better to move it in this patch since this 
patch introduces both the definition and usage of notifiers_list.

If it is already clarified, then ignore it.

Thanks,
Yi L
> +        VTDAddressSpace *vtd_as = node->vtd_as;
> +        uint16_t vfio_domain_id;
> +        int ret = vtd_get_did_dev(s, pci_bus_num(vtd_as->bus), vtd_as->devfn,
> +                                  &vfio_domain_id);
> +
> +        if (!ret && domain_id == vfio_domain_id) {
> +            hwaddr original_addr = addr;
> +
> +            while (addr < original_addr + (1 << am) * VTD_PAGE_SIZE) {
> +                IOMMUTLBEntry entry = s->iommu_ops.translate(
> +                                                         
> &node->vtd_as->iommu,
> +                                                         addr,
> +                                                         IOMMU_NO_FAIL);
> +
> +                if (entry.perm == IOMMU_NONE &&
> +                        node->notifier_flag & IOMMU_NOTIFIER_UNMAP) {
> +                    entry.target_as = &address_space_memory;
> +                    entry.iova = addr & VTD_PAGE_MASK_4K;
> +                    entry.translated_addr = 0;
> +                    entry.addr_mask = ~VTD_PAGE_MASK(VTD_PAGE_SHIFT);
> +                    memory_region_notify_iommu(&node->vtd_as->iommu, entry);
> +                    addr += VTD_PAGE_SIZE;
> +                } else if (node->notifier_flag & IOMMU_NOTIFIER_MAP) {
> +                        memory_region_notify_iommu(&node->vtd_as->iommu, 
> entry);
> +                        addr += entry.addr_mask + 1;
> +                }
> +            }
> +        }
> +    }
> +}
> +
>  static void vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,
>                                        hwaddr addr, uint8_t am)
>  {
> @@ -1074,6 +1130,8 @@ static void vtd_iotlb_page_invalidate(IntelIOMMUState 
> *s, 
> uint16_t domain_id,
>      info.addr = addr;
>      info.mask = ~((1 << am) - 1);
>      g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page, &info);
> +
> +    vtd_iotlb_page_invalidate_notify(s, domain_id, addr, am);
>  }
>  
>  /* Flush IOTLB
> @@ -1999,15 +2057,37 @@ static void 
> vtd_iommu_notify_flag_changed(MemoryRegion 
> *iommu,
>                                            IOMMUNotifierFlag new)
>  {
>      VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
> +    IntelIOMMUState *s = vtd_as->iommu_state;
> +    IntelIOMMUNotifierNode *node = NULL;
> +    IntelIOMMUNotifierNode *next_node = NULL;
>  
> -    if (new & IOMMU_NOTIFIER_MAP) {
> -        error_report("Device at bus %s addr %02x.%d requires iommu "
> -                     "notifier which is currently not supported by "
> -                     "intel-iommu emulation",
> -                     vtd_as->bus->qbus.name, PCI_SLOT(vtd_as->devfn),
> -                     PCI_FUNC(vtd_as->devfn));
> +    if (!s->cache_mode_enabled && new & IOMMU_NOTIFIER_MAP) {
> +        error_report("We need to set cache_mode=1 for intel-iommu to enable "
> +                     "device assignment with IOMMU protection.");
>          exit(1);
>      }
> +
> +    /* Add new ndoe if no mapping was exising before this call */
> +    if (old == IOMMU_NOTIFIER_NONE) {
> +        node = g_malloc0(sizeof(*node));
> +        node->vtd_as = vtd_as;
> +        node->notifier_flag = new;
> +        QLIST_INSERT_HEAD(&s->notifiers_list, node, next);
> +        return;
> +    }
> +
> +    /* update notifier node with new flags */
> +    QLIST_FOREACH_SAFE(node, &s->notifiers_list, next, next_node) {
> +        if (node->vtd_as == vtd_as) {
> +            if (new == IOMMU_NOTIFIER_NONE) {
> +                QLIST_REMOVE(node, next);
> +                g_free(node);
> +            } else {
> +                node->notifier_flag = new;
> +            }
> +            return;
> +        }
> +    }
>  }
>  
>  static const VMStateDescription vtd_vmstate = {
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 00cbe0d..c1b9cfc 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -381,6 +381,8 @@ typedef struct VTDIOTLBPageInvInfo VTDIOTLBPageInvInfo;
>  #define VTD_PAGE_SHIFT_1G           30
>  #define VTD_PAGE_MASK_1G            (~((1ULL << VTD_PAGE_SHIFT_1G) - 1))
>  
> +#define VTD_PAGE_MASK(shift)        (~((1ULL << (shift)) - 1))
> +
>  struct VTDRootEntry {
>      uint64_t val;
>      uint64_t rsvd;
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index 749eef9..1a355c1 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -63,6 +63,7 @@ typedef union VTD_IR_TableEntry VTD_IR_TableEntry;
>  typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress;
>  typedef struct VTDIrq VTDIrq;
>  typedef struct VTD_MSIMessage VTD_MSIMessage;
> +typedef struct IntelIOMMUNotifierNode IntelIOMMUNotifierNode;
>  
>  /* Context-Entry */
>  struct VTDContextEntry {
> @@ -247,6 +248,12 @@ struct VTD_MSIMessage {
>  /* When IR is enabled, all MSI/MSI-X data bits should be zero */
>  #define VTD_IR_MSI_DATA          (0)
>  
> +struct IntelIOMMUNotifierNode {
> +    VTDAddressSpace *vtd_as;
> +    IOMMUNotifierFlag notifier_flag;
> +    QLIST_ENTRY(IntelIOMMUNotifierNode) next;
> +};
> +
>  /* The iommu (DMAR) device state struct */
>  struct IntelIOMMUState {
>      X86IOMMUState x86_iommu;
> @@ -284,6 +291,8 @@ struct IntelIOMMUState {
>      MemoryRegionIOMMUOps iommu_ops;
>      GHashTable *vtd_as_by_busptr;   /* VTDBus objects indexed by PCIBus* 
> reference */
>      VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects indexed by 
> bus number */
> +    /* list of registered notifiers */
> +    QLIST_HEAD(, IntelIOMMUNotifierNode) notifiers_list;
>  
>      /* interrupt remapping */
>      bool intr_enabled;              /* Whether guest enabled IR */
> -- 
> 1.9.1
>