Re: [PATCH v3 2/5] xen/common: Guard iommu symbols with CONFIG_HAS_PASSTHROUGH

[Julien Grall <julien@xxxxxxx>]

On 18/05/2021 07:27, Jan Beulich wrote:
> On 18.05.2021 06:11, Connor Davis wrote:
> > On 5/17/21 9:42 AM, Julien Grall wrote:
> > > Hi Jan,
> > >
> > > On 17/05/2021 12:16, Jan Beulich wrote:
> > > > On 14.05.2021 20:53, Connor Davis wrote:
> > > > > --- a/xen/common/memory.c
> > > > > +++ b/xen/common/memory.c
> > > > > @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned
> > > > > long gmfn)
> > > > >        p2m_type_t p2mt;
> > > > >    #endif
> > > > >        mfn_t mfn;
> > > > > +#ifdef CONFIG_HAS_PASSTHROUGH
> > > > >        bool *dont_flush_p, dont_flush;
> > > > > +#endif
> > > > >        int rc;
> > > > >      #ifdef CONFIG_X86
> > > > > @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d,
> > > > > unsigned long gmfn)
> > > > >         * Since we're likely to free the page below, we need to suspend
> > > > >         * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes.
> > > > >         */
> > > > > +#ifdef CONFIG_HAS_PASSTHROUGH
> > > > >        dont_flush_p = &this_cpu(iommu_dont_flush_iotlb);
> > > > >        dont_flush = *dont_flush_p;
> > > > >        *dont_flush_p = false;
> > > > > +#endif
> > > > >          rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
> > > > >    +#ifdef CONFIG_HAS_PASSTHROUGH
> > > > >        *dont_flush_p = dont_flush;
> > > > > +#endif
> > > > >          /*
> > > > >         * With the lack of an IOMMU on some platforms, domains with
> > > > > DMA-capable
> > > > > @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d,
> > > > > struct xen_add_to_physmap *xatp,
> > > > >        xatp->gpfn += start;
> > > > >        xatp->size -= start;
> > > > >    +#ifdef CONFIG_HAS_PASSTHROUGH
> > > > >        if ( is_iommu_enabled(d) )
> > > > >        {
> > > > >           this_cpu(iommu_dont_flush_iotlb) = 1;
> > > > >           extra.ppage = &pages[0];
> > > > >        }
> > > > > +#endif
> > > > >          while ( xatp->size > done )
> > > > >        {
> > > > > @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d,
> > > > > struct xen_add_to_physmap *xatp,
> > > > >            }
> > > > >        }
> > > > >    +#ifdef CONFIG_HAS_PASSTHROUGH
> > > > >        if ( is_iommu_enabled(d) )
> > > > >        {
> > > > >            int ret;
> > > > > @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d,
> > > > > struct xen_add_to_physmap *xatp,
> > > > >            if ( unlikely(ret) && rc >= 0 )
> > > > >                rc = ret;
> > > > >        }
> > > > > +#endif
> > > > >          return rc;
> > > > >    }
> > > > I wonder whether all of these wouldn't better become CONFIG_X86:
> > > > ISTR Julien indicating that he doesn't see the override getting used
> > > > on Arm. (Julien, please correct me if I'm misremembering.)
> > > Right, so far, I haven't been in favor to introduce it because:
> > >     1) The P2M code may free some memory. So you can't always ignore
> > > the flush (I think this is wrong for the upper layer to know when this
> > > can happen).
> > >     2) It is unclear what happen if the IOMMU TLBs and the PT contains
> > > different mappings (I received conflicted advice).
> > >
> > > So it is better to always flush and as early as possible.
> > So keep it as is or switch to CONFIG_X86?
> Please switch, unless anyone else voices a strong opinion towards
> keeping as is.
I would like to avoid adding more #ifdef CONFIG_X86 in the common code. 
Can we instead provide a wrapper for them?
Cheers,

--
Julien Grall