RE: [PATCH 5/6] VT-d: introduce helper to convert DID to domid_t

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Friday, November 12, 2021 5:50 PM
> 
> This is in preparation of adding another "translation" method. Take the
> combination of the extra validation both previously open-coded have been
> doing: Bounds check and bitmap check. But don't propagate the previous
> pointless check of whether ->domid_map[] was actually allocated, as
> failure there would lead to overall failure of IOMMU initialization
> anyway.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>

> 
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -45,6 +45,8 @@ void disable_intremap(struct vtd_iommu *
>  int iommu_alloc(struct acpi_drhd_unit *drhd);
>  void iommu_free(struct acpi_drhd_unit *drhd);
> 
> +domid_t did_to_domain_id(const struct vtd_iommu *iommu, unsigned int
> did);
> +
>  int iommu_flush_iec_global(struct vtd_iommu *iommu);
>  int iommu_flush_iec_index(struct vtd_iommu *iommu, u8 im, u16 iidx);
>  void clear_fault_bits(struct vtd_iommu *iommu);
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -123,15 +123,16 @@ static int context_get_domain_id(const s
> 
>      if ( iommu && context )
>      {
> -        unsigned int nr_dom = cap_ndoms(iommu->cap);
>          unsigned int dom_index = context_domain_id(*context);
> 
> -        if ( dom_index < nr_dom && iommu->domid_map )
> -            domid = iommu->domid_map[dom_index];
> -        else
> +        domid = did_to_domain_id(iommu, dom_index);
> +        if ( domid == DOMID_INVALID )
> +        {
>              dprintk(XENLOG_DEBUG VTDPREFIX,
> -                    "dom_index %u exceeds nr_dom %u or iommu has no
> domid_map\n",
> -                    dom_index, nr_dom);
> +                    "no domid for did %u (nr_dom %u)\n",
> +                    dom_index, cap_ndoms(iommu->cap));
> +            domid = -1;
> +        }
>      }
> 
>      return domid;
> @@ -193,6 +194,14 @@ static void check_cleanup_domid_map(stru
>      }
>  }
> 
> +domid_t did_to_domain_id(const struct vtd_iommu *iommu, unsigned int
> did)
> +{
> +    if ( did >= cap_ndoms(iommu->cap) || !test_bit(did, iommu-
> >domid_bitmap) )
> +        return DOMID_INVALID;
> +
> +    return iommu->domid_map[did];
> +}
> +
>  static void sync_cache(const void *addr, unsigned int size)
>  {
>      static unsigned long clflush_size = 0;
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -229,10 +229,7 @@ static int __must_check dev_invalidate_s
>      rc = queue_invalidate_wait(iommu, 0, 1, 1, 1);
>      if ( rc == -ETIMEDOUT )
>      {
> -        struct domain *d = NULL;
> -
> -        if ( test_bit(did, iommu->domid_bitmap) )
> -            d = rcu_lock_domain_by_id(iommu->domid_map[did]);
> +        struct domain *d = rcu_lock_domain_by_id(did_to_domain_id(iommu,
> did));
> 
>          /*
>           * In case the domain has been freed or the IOMMU domid bitmap is