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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan