Re: [PATCH V2 10/10] iommu/arm: Remove code duplication in all IOMMU drivers

[Oleksandr <olekstysh@xxxxxxxxx>]

On 27.01.22 13:54, Julien Grall wrote:
> Hi,

Hi Julien


> On 20/12/2021 21:15, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > All IOMMU drivers on Arm perform almost the same generic actions in
> > hwdom_init callback. Move this code to common arch_iommu_hwdom_init()
> > in order to get rid of code duplication.
> >
> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> > Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > ---
> > Changes V1 -> V2:
> >     - add R-b
> > ---
> >   xen/drivers/passthrough/arm/iommu.c      |  7 +++++++
> >   xen/drivers/passthrough/arm/ipmmu-vmsa.c |  8 --------
> >   xen/drivers/passthrough/arm/smmu-v3.c    | 10 ----------
> >   xen/drivers/passthrough/arm/smmu.c       | 10 ----------
> >   4 files changed, 7 insertions(+), 28 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/arm/iommu.c 
> > b/xen/drivers/passthrough/arm/iommu.c
> > index ee653a9..fc45318 100644
> > --- a/xen/drivers/passthrough/arm/iommu.c
> > +++ b/xen/drivers/passthrough/arm/iommu.c
> > @@ -134,6 +134,13 @@ void arch_iommu_domain_destroy(struct domain *d)
> >     void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
> >   {
> > +    /* Set to false options not supported on ARM. */
> > +    if ( iommu_hwdom_inclusive )
> > +        printk(XENLOG_WARNING "map-inclusive dom0-iommu option is 
> > not supported on ARM\n");
> > +    iommu_hwdom_inclusive = false;
> > +    if ( iommu_hwdom_reserved == 1 )
> > +        printk(XENLOG_WARNING "map-reserved dom0-iommu option is not 
> > supported on ARM\n");
> > +    iommu_hwdom_reserved = 0;
> >   }
> >     /*
> > diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c 
> > b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> > index 1224ea4..64d8ddc 100644
> > --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> > +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> > @@ -1325,14 +1325,6 @@ static int ipmmu_iommu_domain_init(struct 
> > domain *d)
> >     static void __hwdom_init ipmmu_iommu_hwdom_init(struct domain *d)
> >   {
> > -    /* Set to false options not supported on ARM. */
> > -    if ( iommu_hwdom_inclusive )
> > -        printk(XENLOG_WARNING "ipmmu: map-inclusive dom0-iommu 
> > option is not supported on ARM\n");
> > -    iommu_hwdom_inclusive = false;
> > -    if ( iommu_hwdom_reserved == 1 )
> > -        printk(XENLOG_WARNING "ipmmu: map-reserved dom0-iommu option 
> > is not supported on ARM\n");
> > -    iommu_hwdom_reserved = 0;
> > -
> >       arch_iommu_hwdom_init(d);
> >   }
> With this change, ipmmu_iommu_domain_init() is just a call to 
> arch_iommu_hwdom_init(). The two functions have the same prototype, so 
> could we simply drop the former and update the .hwdom_init to directly 
> point to the latter?
Yes, good idea, will update. Thank you.


> >   diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> > b/xen/drivers/passthrough/arm/smmu-v3.c
> > index d115df7..ca8b5c7 100644
> > --- a/xen/drivers/passthrough/arm/smmu-v3.c
> > +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> > @@ -3404,16 +3404,6 @@ static int 
> > arm_smmu_iommu_xen_domain_init(struct domain *d)
> >     static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
> >   {
> > -    /* Set to false options not supported on ARM. */
> > -    if (iommu_hwdom_inclusive)
> > -        printk(XENLOG_WARNING
> > -        "map-inclusive dom0-iommu option is not supported on ARM\n");
> > -    iommu_hwdom_inclusive = false;
> > -    if (iommu_hwdom_reserved == 1)
> > -        printk(XENLOG_WARNING
> > -        "map-reserved dom0-iommu option is not supported on ARM\n");
> > -    iommu_hwdom_reserved = 0;
> > -
> >       arch_iommu_hwdom_init(d);
> >   }
> Same here.
>
> > diff --git a/xen/drivers/passthrough/arm/smmu.c 
> > b/xen/drivers/passthrough/arm/smmu.c
> > index c9dfc4c..ec18df7 100644
> > --- a/xen/drivers/passthrough/arm/smmu.c
> > +++ b/xen/drivers/passthrough/arm/smmu.c
> > @@ -2851,16 +2851,6 @@ static int arm_smmu_iommu_domain_init(struct 
> > domain *d)
> >     static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
> >   {
> > -    /* Set to false options not supported on ARM. */
> > -    if ( iommu_hwdom_inclusive )
> > -        printk(XENLOG_WARNING
> > -        "map-inclusive dom0-iommu option is not supported on ARM\n");
> > -    iommu_hwdom_inclusive = false;
> > -    if ( iommu_hwdom_reserved == 1 )
> > -        printk(XENLOG_WARNING
> > -        "map-reserved dom0-iommu option is not supported on ARM\n");
> > -    iommu_hwdom_reserved = 0;
> > -
> >       arch_iommu_hwdom_init(d);
> >   }
> Same here.
>
> Cheers,
--
Regards,

Oleksandr Tyshchenko