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

[Jan Beulich <jbeulich@xxxxxxxx>]

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;
>>>>>>    #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?

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

Jan