[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

Hi Jan,

On 18/05/2021 16:13, Jan Beulich wrote:
On 18.05.2021 16:06, Julien Grall wrote:

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;
        mfn_t mfn;
        bool *dont_flush_p, dont_flush;
        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.
        dont_flush_p = &this_cpu(iommu_dont_flush_iotlb);
        dont_flush = *dont_flush_p;
        *dont_flush_p = false;
          rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
        *dont_flush_p = dont_flush;
         * With the lack of an IOMMU on some platforms, domains with
@@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d,
struct xen_add_to_physmap *xatp,
        xatp->gpfn += start;
        xatp->size -= start;
        if ( is_iommu_enabled(d) )
           this_cpu(iommu_dont_flush_iotlb) = 1;
           extra.ppage = &pages[0];
          while ( xatp->size > done )
@@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d,
struct xen_add_to_physmap *xatp,
        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;
          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?

Doable, sure, but I don't know whether Connor is up to going this
more extensive route.

That's a fair point. If that the case, then I prefer the #ifdef CONFIG_HAS_PASSTHROUGH version.

I can add an item in my todo list to introduce some helpers.


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.