Re: [RFC PATCH 2/9] iommu: Add ability to map/unmap the number of pages

[Oleksandr Tyshchenko <olekstysh@gmail.com>]

On Wed, Apr 19, 2017 at 8:31 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Oleksandr,
Hi, Julien

>
>
> On 15/03/17 20:05, Oleksandr Tyshchenko wrote:
>>
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Extend the IOMMU code with new APIs and platform callbacks.
>> These new map_pages/unmap_pages API do almost the same thing
>> as existing map_page/unmap_page ones except the formers can
>> handle the number of pages. So do new platform callbacks.
>>
>> Currently, this patch requires to modify neither
>> existing IOMMU drivers nor P2M code.
>> But, the patch might be rewritten to replace existing
>> single-page stuff with the multi-page one followed by modifications
>> of all related parts.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>  xen/drivers/passthrough/iommu.c | 50
>> ++++++++++++++++++++++++++++++++---------
>>  xen/include/xen/iommu.h         | 16 ++++++++++---
>>  2 files changed, 52 insertions(+), 14 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/iommu.c
>> b/xen/drivers/passthrough/iommu.c
>> index 5e81813..115698f 100644
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -249,22 +249,37 @@ void iommu_domain_destroy(struct domain *d)
>>      arch_iommu_domain_destroy(d);
>>  }
>>
>> -int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long
>> mfn,
>> -                   unsigned int flags)
>> +int iommu_map_pages(struct domain *d, unsigned long gfn, unsigned long
>> mfn,
>> +                    unsigned long page_count, unsigned int flags)
>
>
> It would be nice if you can use mfn_t and gfn_t instead of unsigned long for
> any new functions. They are typesafe and avoid confusion between gfn and
> mfn.
ok

>
>
>>  {
>>      const struct domain_iommu *hd = dom_iommu(d);
>> -    int rc;
>> +    int rc = 0;
>> +    unsigned long i;
>>
>>      if ( !iommu_enabled || !hd->platform_ops )
>>          return 0;
>>
>> -    rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
>> +    if ( hd->platform_ops->map_pages )
>> +        rc = hd->platform_ops->map_pages(d, gfn, mfn, page_count, flags);
>> +    else
>> +    {
>> +        for ( i = 0; i < page_count; i++ )
>> +        {
>> +            rc = hd->platform_ops->map_page(d, gfn + i, mfn + i, flags);
>> +            if ( unlikely(rc) )
>> +            {
>> +                /* TODO Do we need to unmap if map failed? */
>> +                break;
>> +            }
>> +        }
>> +    }
>> +
>>      if ( unlikely(rc) )
>>      {
>>          if ( !d->is_shutting_down && printk_ratelimit() )
>>              printk(XENLOG_ERR
>> -                   "d%d: IOMMU mapping gfn %#lx to mfn %#lx failed:
>> %d\n",
>> -                   d->domain_id, gfn, mfn, rc);
>> +                   "d%d: IOMMU mapping gfn %#lx to mfn %#lx page count
>> %lu failed: %d\n",
>> +                   d->domain_id, gfn, mfn, page_count, rc);
>>
>>          if ( !is_hardware_domain(d) )
>>              domain_crash(d);
>> @@ -273,21 +288,34 @@ int iommu_map_page(struct domain *d, unsigned long
>> gfn, unsigned long mfn,
>>      return rc;
>>  }
>>
>> -int iommu_unmap_page(struct domain *d, unsigned long gfn)
>> +int iommu_unmap_pages(struct domain *d, unsigned long gfn,
>> +                      unsigned long page_count)
>>  {
>>      const struct domain_iommu *hd = dom_iommu(d);
>> -    int rc;
>> +    int ret, rc = 0;
>> +    unsigned long i;
>>
>>      if ( !iommu_enabled || !hd->platform_ops )
>>          return 0;
>>
>> -    rc = hd->platform_ops->unmap_page(d, gfn);
>> +    if ( hd->platform_ops->unmap_pages )
>> +        rc = hd->platform_ops->unmap_pages(d, gfn, page_count);
>> +    else
>> +    {
>> +        for ( i = 0; i < page_count; i++ )
>> +        {
>> +            ret = hd->platform_ops->unmap_page(d, gfn + i);
>> +            if ( likely(!rc) )
>> +                rc = ret;
>> +        }
>> +    }
>> +
>>      if ( unlikely(rc) )
>>      {
>>          if ( !d->is_shutting_down && printk_ratelimit() )
>>              printk(XENLOG_ERR
>> -                   "d%d: IOMMU unmapping gfn %#lx failed: %d\n",
>> -                   d->domain_id, gfn, rc);
>> +                   "d%d: IOMMU unmapping gfn %#lx page count %lu failed:
>> %d\n",
>> +                   d->domain_id, gfn, page_count, rc);
>>
>>          if ( !is_hardware_domain(d) )
>>              domain_crash(d);
>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>> index 5803e3f..0446ed3 100644
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -76,9 +76,14 @@ void iommu_teardown(struct domain *d);
>>  #define IOMMUF_readable  (1u<<_IOMMUF_readable)
>>  #define _IOMMUF_writable 1
>>  #define IOMMUF_writable  (1u<<_IOMMUF_writable)
>> -int __must_check iommu_map_page(struct domain *d, unsigned long gfn,
>> -                                unsigned long mfn, unsigned int flags);
>> -int __must_check iommu_unmap_page(struct domain *d, unsigned long gfn);
>> +int __must_check iommu_map_pages(struct domain *d, unsigned long gfn,
>> +                                 unsigned long mfn, unsigned long
>> page_count,
>> +                                 unsigned int flags);
>> +int __must_check iommu_unmap_pages(struct domain *d, unsigned long gfn,
>> +                                   unsigned long page_count);
>> +
>> +#define iommu_map_page(d,gfn,mfn,flags)
>> (iommu_map_pages(d,gfn,mfn,1,flags))
>> +#define iommu_unmap_page(d,gfn)         (iommu_unmap_pages(d,gfn,1))
>>
>>  enum iommu_feature
>>  {
>> @@ -170,7 +175,12 @@ struct iommu_ops {
>>      void (*teardown)(struct domain *d);
>>      int __must_check (*map_page)(struct domain *d, unsigned long gfn,
>>                                   unsigned long mfn, unsigned int flags);
>> +    int __must_check (*map_pages)(struct domain *d, unsigned long gfn,
>> +                                  unsigned long mfn, unsigned long
>> page_count,
>> +                                  unsigned int flags);
>>      int __must_check (*unmap_page)(struct domain *d, unsigned long gfn);
>> +    int __must_check (*unmap_pages)(struct domain *d, unsigned long gfn,
>> +                                    unsigned long page_count);
>>      void (*free_page_table)(struct page_info *);
>>  #ifdef CONFIG_X86
>>      void (*update_ire_from_apic)(unsigned int apic, unsigned int reg,
>> unsigned int value);
>>
>
> Cheers,
>
> --
> Julien Grall



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel