RE: [PATCH v2] VT-d: avoid allocating domid_{bit,}map[] when possible

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

> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Friday, December 3, 2021 6:41 PM
> 
> When an IOMMU implements the full 16 bits worth of DID in context
> entries, there's no point going through a memory base translation table.
> For IOMMUs not using Caching Mode we can simply use the domain IDs
> verbatim, while for Caching Mode we need to avoid DID 0.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

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

> ---
> For the case where the memory tables are needed, xvzalloc_array() would
> of course be an option to use here as well, despite this being boot time
> allocations. Yet the introduction of xvmalloc() et al continues to be
> stuck ...
> ---
> v2: Use different BUILD_BUG_ON().
> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -62,11 +62,32 @@ static struct tasklet vtd_fault_tasklet;
>  static int setup_hwdom_device(u8 devfn, struct pci_dev *);
>  static void setup_hwdom_rmrr(struct domain *d);
> 
> +static bool domid_mapping(const struct vtd_iommu *iommu)
> +{
> +    return (const void *)iommu->domid_bitmap != (const void *)iommu-
> >domid_map;
> +}
> +
> +static domid_t convert_domid(const struct vtd_iommu *iommu, domid_t
> domid)
> +{
> +    /*
> +     * While we need to avoid DID 0 for caching-mode IOMMUs, maintain
> +     * the property of the transformation being the same in either
> +     * direction. By clipping to 16 bits we ensure that the resulting
> +     * DID will fit in the respective context entry field.
> +     */
> +    BUILD_BUG_ON(DOMID_MASK >= 0xffff);
> +
> +    return !cap_caching_mode(iommu->cap) ? domid : ~domid;
> +}
> +
>  static int domain_iommu_domid(const struct domain *d,
>                                const struct vtd_iommu *iommu)
>  {
>      unsigned int nr_dom, i;
> 
> +    if ( !domid_mapping(iommu) )
> +        return convert_domid(iommu, d->domain_id);
> +
>      nr_dom = cap_ndoms(iommu->cap);
>      i = find_first_bit(iommu->domid_bitmap, nr_dom);
>      while ( i < nr_dom )
> @@ -91,26 +112,32 @@ static int context_set_domain_id(struct
>                                   const struct domain *d,
>                                   struct vtd_iommu *iommu)
>  {
> -    unsigned int nr_dom, i;
> +    unsigned int i;
> 
>      ASSERT(spin_is_locked(&iommu->lock));
> 
> -    nr_dom = cap_ndoms(iommu->cap);
> -    i = find_first_bit(iommu->domid_bitmap, nr_dom);
> -    while ( i < nr_dom && iommu->domid_map[i] != d->domain_id )
> -        i = find_next_bit(iommu->domid_bitmap, nr_dom, i + 1);
> -
> -    if ( i >= nr_dom )
> +    if ( domid_mapping(iommu) )
>      {
> -        i = find_first_zero_bit(iommu->domid_bitmap, nr_dom);
> +        unsigned int nr_dom = cap_ndoms(iommu->cap);
> +
> +        i = find_first_bit(iommu->domid_bitmap, nr_dom);
> +        while ( i < nr_dom && iommu->domid_map[i] != d->domain_id )
> +            i = find_next_bit(iommu->domid_bitmap, nr_dom, i + 1);
> +
>          if ( i >= nr_dom )
>          {
> -            dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: no free domain ids\n");
> -            return -EBUSY;
> +            i = find_first_zero_bit(iommu->domid_bitmap, nr_dom);
> +            if ( i >= nr_dom )
> +            {
> +                dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: no free domain id\n");
> +                return -EBUSY;
> +            }
> +            iommu->domid_map[i] = d->domain_id;
> +            set_bit(i, iommu->domid_bitmap);
>          }
> -        iommu->domid_map[i] = d->domain_id;
> -        set_bit(i, iommu->domid_bitmap);
>      }
> +    else
> +        i = convert_domid(iommu, d->domain_id);
> 
>      context->hi |= (i & ((1 << DID_FIELD_WIDTH) - 1)) << DID_HIGH_OFFSET;
>      return 0;
> @@ -140,7 +167,12 @@ static int context_get_domain_id(const s
> 
>  static void cleanup_domid_map(struct domain *domain, struct vtd_iommu
> *iommu)
>  {
> -    int iommu_domid = domain_iommu_domid(domain, iommu);
> +    int iommu_domid;
> +
> +    if ( !domid_mapping(iommu) )
> +        return;
> +
> +    iommu_domid = domain_iommu_domid(domain, iommu);
> 
>      if ( iommu_domid >= 0 )
>      {
> @@ -196,7 +228,13 @@ 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) )
> +    if ( did >= min(cap_ndoms(iommu->cap), DOMID_MASK + 1) )
> +        return DOMID_INVALID;
> +
> +    if ( !domid_mapping(iommu) )
> +        return convert_domid(iommu, did);
> +
> +    if ( !test_bit(did, iommu->domid_bitmap) )
>          return DOMID_INVALID;
> 
>      return iommu->domid_map[did];
> @@ -1297,24 +1335,32 @@ int __init iommu_alloc(struct acpi_drhd_
>      if ( !ecap_coherent(iommu->ecap) )
>          vtd_ops.sync_cache = sync_cache;
> 
> -    /* allocate domain id bitmap */
>      nr_dom = cap_ndoms(iommu->cap);
> -    iommu->domid_bitmap = xzalloc_array(unsigned long,
> BITS_TO_LONGS(nr_dom));
> -    if ( !iommu->domid_bitmap )
> -        return -ENOMEM;
> 
> -    iommu->domid_map = xzalloc_array(domid_t, nr_dom);
> -    if ( !iommu->domid_map )
> -        return -ENOMEM;
> +    if ( nr_dom <= DOMID_MASK + cap_caching_mode(iommu->cap) )
> +    {
> +        /* Allocate domain id (bit) maps. */
> +        iommu->domid_bitmap = xzalloc_array(unsigned long,
> +                                            BITS_TO_LONGS(nr_dom));
> +        iommu->domid_map = xzalloc_array(domid_t, nr_dom);
> +        if ( !iommu->domid_bitmap || !iommu->domid_map )
> +            return -ENOMEM;
> 
> -    /*
> -     * If Caching mode is set, then invalid translations are tagged with
> -     * domain id 0. Hence reserve bit/slot 0.
> -     */
> -    if ( cap_caching_mode(iommu->cap) )
> +        /*
> +         * If Caching mode is set, then invalid translations are tagged
> +         * with domain id 0. Hence reserve bit/slot 0.
> +         */
> +        if ( cap_caching_mode(iommu->cap) )
> +        {
> +            iommu->domid_map[0] = DOMID_INVALID;
> +            __set_bit(0, iommu->domid_bitmap);
> +        }
> +    }
> +    else
>      {
> -        iommu->domid_map[0] = DOMID_INVALID;
> -        __set_bit(0, iommu->domid_bitmap);
> +        /* Don't leave dangling NULL pointers. */
> +        iommu->domid_bitmap = ZERO_BLOCK_PTR;
> +        iommu->domid_map = ZERO_BLOCK_PTR;
>      }
> 
>      return 0;
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -82,7 +82,7 @@
>  #define cap_plmr(c)        (((c) >> 5) & 1)
>  #define cap_rwbf(c)        (((c) >> 4) & 1)
>  #define cap_afl(c)        (((c) >> 3) & 1)
> -#define cap_ndoms(c)        (1 << (4 + 2 * ((c) & 0x7)))
> +#define cap_ndoms(c)        (1U << (4 + 2 * ((c) & 0x7)))
> 
>  /*
>   * Extended Capability Register